Skip to content

fix(ci): track profiles/providers in select-eval-agents.sh #279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fullsend-ai-coder wants to merge 1 commit into from
Closed

fix(ci): track profiles/providers in select-eval-agents.sh #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions .github/scripts/select-eval-agents-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ host_files:
dest: /tmp/.gcp-credentials.json
skills:
- skills/issue-labels
openshell:
profiles:
- "https://raw.githubusercontent.com/fullsend-ai/agents/main/profiles/fullsend-vertex-ai.yaml#sha256=abc123"
- "https://raw.githubusercontent.com/fullsend-ai/agents/main/profiles/fullsend-github-ro.yaml#sha256=def456"
providers:
- "https://raw.githubusercontent.com/fullsend-ai/agents/main/providers/vertex-ai.yaml#sha256=ghi789"
- "https://raw.githubusercontent.com/fullsend-ai/agents/main/providers/github-ro.yaml#sha256=jkl012"
Comment on lines +49 to +55

@waynesun09 waynesun09 Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] premature-decision — "multi-segment ref" test-plan claim isn't backed by any test

The PR description checks off: "URL stripping handles multi-segment refs (refs/pull/211/head, refs/heads/main, v1.0.0)". But every fixture URL added in this PR (here and the matching block for review.yaml) uses only the single-segment ref main — none use refs/pull/.../head or a tag. This matters because PR #211's actual harness files use exactly the multi-segment form today, e.g. .../agents/refs/pull/211/head/profiles/fullsend-vertex-ai.yaml#sha256=.... That case does resolve correctly against the current sed pipeline (verified by hand), but that verification is an unchecked, ad-hoc claim rather than committed regression coverage — a future edit to the sed pattern could break multi-segment handling without any test failing.

Suggestion: add a fixture/test case using a multi-segment ref (e.g. refs/heads/main/profiles/...) so the checked box is actually backed by an assertion.

Flagged independently by all 3 reviewers (Claude, Claude, Grok).

@maruiz93 maruiz93 Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moot — the sed URL-stripping that would handle multi-segment refs has been removed (paths are local after #211 merged). No URL parsing means no multi-segment ref handling needed. PR closed as superseded.

YAML

cat > "$tmpdir/harness/review.yaml" << 'YAML'
Expand All @@ -69,6 +76,13 @@ skills:
- skills/code-review
plugins:
- plugins/gopls-lsp
openshell:
profiles:
- "https://raw.githubusercontent.com/fullsend-ai/agents/main/profiles/fullsend-vertex-ai.yaml#sha256=abc123"
- "https://raw.githubusercontent.com/fullsend-ai/agents/main/profiles/fullsend-github-code.yaml#sha256=mno345"
providers:
- "https://raw.githubusercontent.com/fullsend-ai/agents/main/providers/vertex-ai.yaml#sha256=ghi789"
- "https://raw.githubusercontent.com/fullsend-ai/agents/main/providers/github-code.yaml#sha256=pqr678"
YAML

# Agent with no eval config — should never be selected
Expand Down Expand Up @@ -396,6 +410,60 @@ else
fi
cleanup_fixture "$FIXTURE"

# ---------------------------------------------------------------------------
# Test: modifying a profile file selects agents referencing it
# ---------------------------------------------------------------------------
run_test
FIXTURE="$(setup_fixture)"
RESULT=$(echo "profiles/fullsend-github-ro.yaml" | "$SELECT_SCRIPT" --repo-root "$FIXTURE")
if [[ "$RESULT" == "triage" ]]; then
pass "profile file change selects agent"
else
fail "profile file change selects agent (got: '$RESULT')"
fi
cleanup_fixture "$FIXTURE"

# ---------------------------------------------------------------------------
# Test: modifying a provider file selects agents referencing it
# ---------------------------------------------------------------------------
run_test
FIXTURE="$(setup_fixture)"
RESULT=$(echo "providers/github-code.yaml" | "$SELECT_SCRIPT" --repo-root "$FIXTURE")
if [[ "$RESULT" == "review" ]]; then
pass "provider file change selects agent"
else
fail "provider file change selects agent (got: '$RESULT')"
fi
cleanup_fixture "$FIXTURE"

# ---------------------------------------------------------------------------
# Test: modifying a shared profile selects all agents referencing it
# ---------------------------------------------------------------------------
run_test
FIXTURE="$(setup_fixture)"
RESULT=$(echo "profiles/fullsend-vertex-ai.yaml" | "$SELECT_SCRIPT" --repo-root "$FIXTURE" | sort)
EXPECTED=$(printf "review\ntriage")
if [[ "$RESULT" == "$EXPECTED" ]]; then
pass "shared profile change selects all referencing agents"
else
fail "shared profile change selects all referencing agents (got: '$RESULT', expected: '$EXPECTED')"
fi
cleanup_fixture "$FIXTURE"

# ---------------------------------------------------------------------------
# Test: modifying a shared provider selects all agents referencing it
# ---------------------------------------------------------------------------
run_test
FIXTURE="$(setup_fixture)"
RESULT=$(echo "providers/vertex-ai.yaml" | "$SELECT_SCRIPT" --repo-root "$FIXTURE" | sort)
EXPECTED=$(printf "review\ntriage")
if [[ "$RESULT" == "$EXPECTED" ]]; then
pass "shared provider change selects all referencing agents"
else
fail "shared provider change selects all referencing agents (got: '$RESULT', expected: '$EXPECTED')"
fi
cleanup_fixture "$FIXTURE"

# ---------------------------------------------------------------------------
# Summary
# ---------------------------------------------------------------------------
Expand Down
9 changes: 7 additions & 2 deletions .github/scripts/select-eval-agents.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,14 @@ extract_refs() {
(.host_files[]?.src),
(.skills[]?),
(.plugins[]?),
.forge.github.pre_script, .forge.github.post_script
.forge.github.pre_script, .forge.github.post_script,
(.openshell.profiles[]?),
(.providers[]?)
Comment on lines +43 to +44

@waynesun09 waynesun09 Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] premature-decision — fix locks in a schema from an unmerged, still-open PR

The new extraction fields (.openshell.profiles[]?, .providers[]?) and the URL-stripping regex on line 48 are built entirely against the schema introduced by #211 ("refactor(sandbox): migrate to portable URL-referenced providers and profiles"), which is still OPEN/unmerged (and currently has merge conflicts with main). Today's field names/URL/fragment format happen to match #211's current diff, but:

  • Neither profiles/ nor providers/ exists on main yet, so this code path is currently unreachable/unexercised against any real harness file — only against synthetic fixtures written by this same PR.
  • If refactor(sandbox): migrate to local-path providers and profiles #211's schema shifts at all during its own review (key rename, different fragment delimiter, etc.), this extraction will silently stop matching — with no test to catch it, quietly reintroducing the exact "changes select zero agents" blind spot this PR exists to fix.

Suggestion: sequence this to land together with (or rebased onto) #211 rather than independently, or add a comment tying this extraction to the specific #211 commit/schema it depends on so drift is traceable.

Flagged independently by all 3 reviewers (Claude, Claude, Grok).

@maruiz93 maruiz93 Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #211 merged on Aug 17, so this concern is resolved — the schema is on main and the extraction runs against real harness files. The PR itself has been closed as superseded: after rebase, all substantive changes (yq extraction + tests) were already on main via #211.

] | .[] | select(. != null)
' "$harness_file" | { grep -v '\$' || true; } | sort -u
' "$harness_file" \
| { grep -v '\$' || true; } \
| sed -E 's/#.*//; s|^https://raw\.githubusercontent\.com/.*/(profiles/)|\1|; s|^https://raw\.githubusercontent\.com/.*/(providers/)|\1|' \

@waynesun09 waynesun09 Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] premature-decision — greedy match on profiles//providers/ isn't anchored, so nested paths silently mis-map

Verified directly:

$ echo 'https://raw.githubusercontent.com/fullsend-ai/agents/main/testdata/profiles/fixture.yaml#sha256=zzz' \
  | sed -E 's/#.*//; s|^https://raw\.githubusercontent\.com/.*/(profiles/)|\1|; s|^https://raw\.githubusercontent\.com/.*/(providers/)|\1|'
profiles/fixture.yaml

The correct repo-relative path here is testdata/profiles/fixture.yaml, but the script produces profiles/fixture.yaml — silently dropping the testdata/ prefix. The regex can't distinguish "the repo's top-level profiles/ directory" from "any path segment literally named profiles appearing anywhere after the ref." The current repo layout has no nested profiles/providers directories, so this doesn't misfire today, but it's a silent-wrong-answer (not fail-loud) class of bug sitting dormant with no test guarding the assumption.

Suggestion: anchor the match to the known org/repo and ref boundary rather than a bare "contains profiles/ somewhere" match, and add a test fixture with a nested path segment to lock in the intended behavior.

Flagged independently by 2 of 3 reviewers (Claude, Claude) and confirmed by direct reproduction above.

@maruiz93 maruiz93 Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. After #211 merged, profiles and providers use local paths (not raw.githubusercontent.com URLs), making the sed URL-stripping a no-op. We removed it during rebase conflict resolution. The greedy regex concern is moot since the sed pipeline is gone. PR closed as superseded — all substantive changes already landed on main.

| sort -u
}

# For each harness file with an eval config, check if any changed file is relevant.
Expand Down
Loading