ci: dynamically select eval agents from changed files - #145
Conversation
Adds eval/review/ with a single case (001-clean-approve) that creates a PR adding multiply/divide functions to a simple Python calc module. The PR is clean and well-tested — the review agent should approve it and produce a ready-for-merge label. Also extends eval/scripts/run-fullsend.sh to export PR_NUMBER and REPO_FULL_NAME for pull_request fixtures, which the review agent's pre-review.sh requires. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
The workflow was hardcoded to only run triage evals. Add a step to also run the review eval cases. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Replace hardcoded agent names in the functional-tests workflow with a script that parses harness/*.yaml to determine which agents' tests to run based on the changed files in a PR. This allows new agents to be added (with their eval configs) in a single PR without also needing to modify the workflow file — solving the pull_request_target chicken-and-egg problem. The select-eval-agents.sh script reads changed files on stdin, extracts all file path references from each harness YAML (agent prompt, doc, policy, scripts, host_files, skills, plugins, schemas, forge scripts), and outputs agent names whose referenced files were touched. Only agents with an eval/<agent>/eval.yaml config are candidates. Includes 15 test cases covering direct harness changes, transitive references (env files, shared scripts, skill/plugin subdirectories), multi-agent selection, exclusion of agents without eval configs, and variable host_file path filtering. Signed-off-by: Ryan Beans <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 10:47 AM UTC · Completed 11:00 AM UTC |
PR Summary by QodoCI: dynamically select eval agents and add review functional eval
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. ::warning:: echoes $FILE_COUNT
|
| if [ "$FILE_COUNT" -ge 300 ]; then | ||
| echo "::warning::Compare API returned $FILE_COUNT files (possible truncation at 300) — running functional tests as a precaution" | ||
| echo "relevant=true" >> "$GITHUB_OUTPUT" | ||
| echo "::warning::Compare API returned $FILE_COUNT files (possible truncation at 300) — running all functional tests as a precaution" |
There was a problem hiding this comment.
1. ::warning:: echoes $file_count 📜 Skill insight ⛨ Security
The workflow emits GitHub Actions workflow commands with an interpolated variable ($FILE_COUNT) that is not sanitized. This can allow workflow-command injection if the value contains control characters, ::, or encoded newlines, violating the requirement to sanitize every interpolated value individually.
Agent Prompt
## Issue description
A GitHub Actions workflow command (`::warning::...`) interpolates `$FILE_COUNT` without sanitization. Per the checklist, every interpolated value in a workflow command must be sanitized individually (for `::`, `%0A/%0D`, ANSI escapes, and control characters).
## Issue Context
This occurs in the `functional-tests.yml` workflow in the step that warns about Compare API truncation. Even if `$FILE_COUNT` is expected to be numeric, the compliance requirement is explicit that each interpolated value must be sanitized.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[104-104]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - 'env/**' | ||
| - 'common/**' | ||
| - 'policies/**' | ||
| - 'skills/**' | ||
| - 'plugins/**' | ||
| - 'docs/**' |
There was a problem hiding this comment.
2. Protected .github/ paths modified 📜 Skill insight § Compliance
This PR modifies protected governance/infrastructure paths under .github/ (workflows and scripts). Per policy, such changes require explicit human review and must not be auto-approved.
Agent Prompt
## Issue description
Protected governance/infrastructure paths are modified in this PR (e.g., `.github/workflows/...`, `.github/scripts/...`). These changes must not be auto-approved and should receive explicit human review.
## Issue Context
The compliance requirement triggers on *any* modification to protected paths.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[18-23]
- .github/scripts/select-eval-agents.sh[1-90]
- .github/scripts/select-eval-agents-test.sh[1-310]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| { | ||
| echo 'files<<GHEOF' | ||
| echo "$FILES" | ||
| echo 'GHEOF' | ||
| } >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
3. Github_output injection risk 🐞 Bug ⛨ Security
The workflow writes the PR file list to $GITHUB_OUTPUT using a fixed heredoc delimiter (GHEOF), so a crafted filename containing a line equal to that delimiter can terminate the value early and inject additional output records. In pull_request_target this lets an untrusted PR manipulate step outputs (e.g., selected agents / whether later steps run).
Agent Prompt
## Issue description
The workflow writes untrusted, PR-controlled file paths into `$GITHUB_OUTPUT` using a constant delimiter (`GHEOF`). If any filename includes a newline-delimited `GHEOF` line, it can break the multiline output format and inject additional outputs.
## Issue Context
This output comes from GitHub API (`gh api .../pulls/.../files`) and is attacker-controlled in `pull_request_target` runs.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[86-117]
- .github/workflows/functional-tests.yml[141-161]
## Suggested fix
- Generate a per-step random delimiter (e.g., `DELIM=$(uuidgen)` or `openssl rand -hex 16`) and use it for the multiline output blocks.
- Prefer `printf '%s\n' "$FILES"`/`printf '%s\n' "$AGENTS"` over `echo` to avoid implementation-defined behavior.
- Apply the same hardening to both `files<<...` and `agents<<...` blocks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| extract_refs() { | ||
| local harness_file="$1" | ||
| yq -r ' | ||
| [ | ||
| .agent, .doc, .policy, .pre_script, .post_script, | ||
| .validation_loop.script, .validation_loop.schema, | ||
| (.host_files[]?.src), | ||
| (.skills[]?), | ||
| (.plugins[]?), | ||
| .forge.github.pre_script, .forge.github.post_script | ||
| ] | .[] | select(. != null) | ||
| ' "$harness_file" 2>/dev/null | grep -v '^\$' | sort -u | ||
| } | ||
|
|
||
| # For each harness file with an eval config, check if any changed file is relevant. | ||
| for harness_file in "$REPO_ROOT"/harness/*.yaml; do | ||
| [[ -f "$harness_file" ]] || continue | ||
| agent="$(basename "$harness_file" .yaml)" | ||
|
|
||
| # Only consider agents that have eval configs | ||
| [[ -f "$REPO_ROOT/eval/$agent/eval.yaml" ]] || continue | ||
|
|
||
| # Collect all paths this agent cares about | ||
| mapfile -t REFS < <(extract_refs "$harness_file") | ||
|
|
There was a problem hiding this comment.
4. Yq failures skip agents 🐞 Bug ☼ Reliability
select-eval-agents.sh suppresses yq parse errors (2>/dev/null) and consumes the extraction via process substitution without checking exit status. If yq fails to parse a harness YAML, REFS becomes empty and referenced-file changes can fail to select the correct agent, skipping functional tests.
Agent Prompt
## Issue description
`extract_refs()` hides `yq` errors and its failure status is not checked (it is consumed via process substitution into `mapfile`). This can silently produce an empty reference set and cause incorrect agent selection (missing required functional tests).
## Issue Context
This script is used to decide which functional tests to run; under-selection means CI coverage gaps.
## Fix Focus Areas
- .github/scripts/select-eval-agents.sh[33-57]
## Suggested fix
- Stop discarding yq stderr (`2>/dev/null`) so failures are diagnosable.
- Capture and check exit status explicitly (e.g., run `refs=$(extract_refs ...)` and check `$?`), or write refs to a temp file and check the producing command.
- Decide a fail-safe policy on parse failure (recommended: select the agent or all agents, or fail the step so CI doesn’t silently skip tests).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if [ -n "$CHANGED_FILES" ]; then | ||
| AGENTS=$(echo "$CHANGED_FILES" | .github/scripts/select-eval-agents.sh) | ||
| else | ||
| # push/workflow_dispatch: run all agents with eval configs | ||
| AGENTS=$(for d in eval/*/eval.yaml; do basename "$(dirname "$d")"; done) | ||
| fi |
There was a problem hiding this comment.
5. Eval glob can yield '*' 🐞 Bug ☼ Reliability
The workflow’s fallback agent enumeration uses for d in eval/*/eval.yaml without nullglob, so if no eval configs match it can emit a literal * agent name. This can make the job attempt ./eval/run-functional.sh '*' and fail unexpectedly.
Agent Prompt
## Issue description
The fallback agent enumeration assumes `eval/*/eval.yaml` matches at least one file; without `nullglob`, an empty match produces a literal pattern and yields `*` as an agent name.
## Issue Context
This happens when the workflow is in the “no changed-files list” branch and the repo has no eval configs matching that layout.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[146-151]
## Suggested fix
- Use `shopt -s nullglob` with an array:
- `shopt -s nullglob; configs=(eval/*/eval.yaml)`
- If `${#configs[@]}==0`, set `AGENTS=""` (or emit a notice) and skip.
- Or use `find eval -mindepth 2 -maxdepth 2 -name eval.yaml` and extract the parent directory names.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| #!/usr/bin/env bash | ||
| # Tests for select-eval-agents.sh | ||
| set -euo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| SELECT_SCRIPT="${SCRIPT_DIR}/select-eval-agents.sh" | ||
| FAILURES=0 | ||
| TESTS=0 | ||
|
|
||
| fail() { | ||
| echo "FAIL: $1" | ||
| FAILURES=$((FAILURES + 1)) | ||
| } | ||
|
|
||
| pass() { | ||
| echo "PASS: $1" | ||
| } | ||
|
|
||
| run_test() { | ||
| TESTS=$((TESTS + 1)) | ||
| } | ||
|
|
There was a problem hiding this comment.
6. New tests not in ci 🐞 Bug ⚙ Maintainability
The PR adds .github/scripts/select-eval-agents-test.sh but it is not invoked by the repo’s existing make script-test target used in CI. As a result, regressions in select-eval-agents.sh can merge without the stated unit tests running.
Agent Prompt
## Issue description
A new unit test script was added but is not wired into the standard script-test target, so CI will not execute it.
## Issue Context
`.github/workflows/script-test.yml` runs `make script-test`; the Makefile target currently doesn’t include the new test.
## Fix Focus Areas
- Makefile[18-28]
- .github/workflows/script-test.yml[35-35]
## Suggested fix
- Add a line to `script-test:`:
- `$(call run-timed,bash .github/scripts/select-eval-agents-test.sh)`
- Ensure any dependencies the test needs (notably `yq`) are already installed in the script-test workflow (they are today).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| with: | ||
| ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.sha || github.sha }} | ||
| persist-credentials: false | ||
| allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} | ||
| submodules: true | ||
|
|
||
| - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| with: | ||
| python-version: "3.12" | ||
|
|
||
| - name: Install uv | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0 | ||
|
|
||
| - name: Install agent-eval-harness | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| run: uv pip install --system -e 'eval/.agent-eval-harness[anthropic]' | ||
|
|
||
| - name: Install yq | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| run: | | ||
| curl -sSfL "https://github.com/mikefarah/yq/releases/download/v4.47.1/yq_linux_amd64" -o /usr/local/bin/yq | ||
| chmod +x /usr/local/bin/yq | ||
|
|
||
| - name: Select eval agents | ||
| id: agents | ||
| env: | ||
| CHANGED_FILES: ${{ steps.changes.outputs.files }} | ||
| run: | | ||
| if [ -n "$CHANGED_FILES" ]; then | ||
| AGENTS=$(echo "$CHANGED_FILES" | .github/scripts/select-eval-agents.sh) | ||
| else | ||
| # push/workflow_dispatch: run all agents with eval configs | ||
| AGENTS=$(for d in eval/*/eval.yaml; do basename "$(dirname "$d")"; done) | ||
| fi | ||
| if [ -z "$AGENTS" ]; then | ||
| echo "::notice::No agents need functional testing for these changes" | ||
| else | ||
| echo "Selected agents: $(echo "$AGENTS" | tr '\n' ' ')" | ||
| fi | ||
| { | ||
| echo 'agents<<GHEOF' | ||
| echo "$AGENTS" | ||
| echo 'GHEOF' | ||
| } >> "$GITHUB_OUTPUT" | ||
|
|
There was a problem hiding this comment.
7. Setup runs before selection 🐞 Bug ➹ Performance
The workflow installs Python, uv, and agent-eval-harness before determining whether any agents are selected for testing. When no agents are selected, these setup steps still run and increase CI time/cost unnecessarily.
Agent Prompt
## Issue description
Several expensive setup steps run unconditionally even when `Select eval agents` would produce an empty list and the job would otherwise skip running tests.
## Issue Context
Selection requires checkout and `yq`, but not Python/uv/harness installation.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[119-162]
- .github/workflows/functional-tests.yml[163-171]
## Suggested fix
- Reorder steps to: checkout → install yq (or use a preinstalled yq) → select agents → conditionally install Python/uv/agent-eval-harness/fullsend/etc only when agents are non-empty.
- This keeps behavior the same while reducing runtime for PRs that select no agents.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Review — commentThis PR replaces the hardcoded Three medium-severity findings are worth discussing before merge. Findings1. Pipe subshell in test loop — first agent failure aborts remaining agents (medium · correctness)File: The test loop pipes echo "$SELECTED_AGENTS" | while read -r agent; do
./eval/run-functional.sh "$agent"
doneWith GitHub Actions' default Remediation: Use a here-string to avoid the subshell and track failures independently: failed=0
while read -r agent; do
[ -z "$agent" ] && continue
echo "=========================================="
echo "Running functional tests: $agent"
echo "=========================================="
./eval/run-functional.sh "$agent" || failed=1
done <<< "$SELECTED_AGENTS"
[ "$failed" -ne 0 ] && exit 12. GHEOF heredoc delimiter collision via crafted filenames (medium · injection-defense)File: The step writes PR filenames into echo 'files<<GHEOF'
echo "$FILES"
echo 'GHEOF'Filenames are fetched from the GitHub API and are attacker-controlled in fork PRs. A file literally named This is mitigated by the Remediation: Use a randomized delimiter: DELIM="GHEOF_$(openssl rand -hex 8)"
{ echo "files<<${DELIM}"; echo "$FILES"; echo "${DELIM}"; } >> "$GITHUB_OUTPUT"3. Shared eval infrastructure changes no longer trigger agent tests on PRs (medium · coverage-regression)File: The old grep-based filter matched Similarly, Push-to-main still runs all agents (correct), but the coverage gap on PRs means infrastructure regressions are only caught after merge. Remediation: Add a wildcard check in for changed in "${CHANGED_FILES[@]}"; do
case "$changed" in
eval/scripts/*|config.yaml|.github/scripts/*|.github/workflows/functional-tests.yml)
# Shared infrastructure — select all agents with eval configs
for hf in "$REPO_ROOT"/harness/*.yaml; do
[[ -f "$hf" ]] || continue
a="$(basename "$hf" .yaml)"
[[ -f "$REPO_ROOT/eval/$a/eval.yaml" ]] && echo "$a"
done | sort -u
exit 0
;;
esac
done4. Unconditional setup steps when no agents are selected (low · efficiency)File: The Functionally correct — the test step itself is properly guarded. Notes
|
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass (3 agents: claude-coder, claude-researcher, gemini-code-review). 5 unique medium+ findings posted inline below. 4 other medium+ findings from the squad (GITHUB_OUTPUT heredoc injection, new tests not wired into CI, unconditional checkout/install steps, yq errors swallowed) matched existing unreplied comments from qodo-code-review[bot] and were skipped to avoid duplicates.
| if [[ "$selected" == true ]]; then | ||
| echo "$agent" | ||
| fi | ||
| done |
There was a problem hiding this comment.
[HIGH] Selector silently skips functional tests for changes to shared eval infra, CI scripts, and config.yaml
extract_refs() (lines 33-44) only inspects .agent, .doc, .policy, .pre_script, .post_script, .validation_loop.*, host_files[].src, skills[], plugins[], and forge.github.*. There's no rule for eval/scripts/**, .github/scripts/** (including this script itself — no self-regression coverage), .github/workflows/functional-tests.yml, or config.yaml.
The PR description says agent selection "subsumes" the old static grep-based path filter, but that filter matched all of these paths and ran tests unconditionally; this selector matches zero agents for them. Not hypothetical: this PR's own change to eval/scripts/run-fullsend.sh only gets tested because it ships alongside eval/review/eval.yaml in the same diff. A follow-up PR touching only the shared runner or this selector script gets zero functional-test signal, including at the merge_group gate before landing on main.
Suggestion: add a fallback rule — changes under eval/scripts/**, .github/scripts/**, .github/workflows/functional-tests.yml, or config.yaml should select all eval-configured agents (same as the push/workflow_dispatch fallback), not zero. Add a regression test asserting this.
| run: ./eval/run-functional.sh triage | ||
| SELECTED_AGENTS: ${{ steps.agents.outputs.agents }} | ||
| run: | | ||
| echo "$SELECTED_AGENTS" | while read -r agent; do |
There was a problem hiding this comment.
[HIGH] Multi-agent loop aborts on the first failing agent
echo "$SELECTED_AGENTS" | while read -r agent; do
[ -z "$agent" ] && continue
...
./eval/run-functional.sh "$agent"
doneGitHub Actions run: steps execute with bash -eo pipefail by default. If the first selected agent's eval fails, the step aborts immediately and any remaining selected agents never run in that CI invocation — with no indication that they were skipped due to an earlier failure rather than not being selected. Previously only one agent ever ran per invocation, so this failure-isolation gap is new behavior introduced by generalizing to a loop.
Suggestion: capture each agent's exit code, continue the loop, and exit non-zero at the end after all selected agents have run, e.g.:
overall_rc=0
while read -r agent; do
[ -z "$agent" ] && continue
./eval/run-functional.sh "$agent" || overall_rc=1
done <<< "$SELECTED_AGENTS"
exit "$overall_rc"| (.skills[]?), | ||
| (.plugins[]?), | ||
| .forge.github.pre_script, .forge.github.post_script | ||
| ] | .[] | select(. != null) |
There was a problem hiding this comment.
[MEDIUM] premature-decision: hardcoded field list in extract_refs unvalidated against harness schema drift
This enumerated field list (.agent, .doc, .policy, etc.) is stated as exhaustive in the script's header comment but isn't validated against the actual harness schema, which already shows drift across agents (e.g. env.runner/runner_env-style path fields used elsewhere). Nothing enforces that a newly-added path-bearing field gets added to this list — the failure mode is silent under-selection that stays green in CI.
Suggestion: either walk all string leaf values generically (filtering to path-looking strings) instead of an explicit field allowlist, or add a lint/test that fails when a harness file contains an unrecognized *_script/path-shaped key not covered here.
| agent="$(basename "$harness_file" .yaml)" | ||
|
|
||
| # Only consider agents that have eval configs | ||
| [[ -f "$REPO_ROOT/eval/$agent/eval.yaml" ]] || continue |
There was a problem hiding this comment.
[MEDIUM] premature-decision: harness/<agent>.yaml ↔ eval/<agent>/ 1:1 naming convention assumed, never enforced
agent="$(basename "$harness_file" .yaml)" then checks eval/$agent/eval.yaml — this hardcodes an exact-name match convention with no schema/lint enforcement. A future rename of one side without the other would silently disable functional-test selection for that agent with no error surfaced anywhere in CI.
Suggestion: add a lint check (or a documentation note) asserting this naming convention is load-bearing for CI coverage, and add a test case that a harness/eval name mismatch produces an explicit warning rather than silent exclusion.
| EVALS_HOST_CREDENTIALS: ${{ env.HOST_GOOGLE_APPLICATION_CREDENTIALS }} | ||
| FULLSEND_DIR: ${{ github.workspace }} | ||
| run: ./eval/run-functional.sh triage | ||
| SELECTED_AGENTS: ${{ steps.agents.outputs.agents }} |
There was a problem hiding this comment.
[MEDIUM] premature-decision: adding a new eval config now grants code execution with cloud secrets, with no workflow-file diff for reviewers to notice
This is explicitly the PR's stated goal (avoid modifying workflow YAML to add agents), but it also means a PR adding harness/evil.yaml + eval/evil/eval.yaml gets its runner auto-executed here with GCP WIF credentials and EVAL_GH_TOKEN, with zero diff to this workflow file to flag the trust-model change during ok-to-test triage. Not a bug, but an unstated tradeoff worth calling out for maintainers doing PR authorization.
Suggestion: document this explicitly in the PR description and/or as a comment near the gate job so maintainers labeling external PRs ok-to-test understand the new blast radius of adding a harness+eval pair.
|
Superseded by #148. |
|
🤖 Finished Retro · ✅ Success · Started 4:56 PM UTC · Completed 5:07 PM UTC |
Retro: PR #145 — ci: dynamically select eval agents from changed filesPR #145 was a human-authored PR by @ralphbean that replaced hardcoded functional test agent names with dynamic selection based on changed files and harness config references. It was closed without merge after review feedback, superseded by PR #148 which restructures the approach into a parallel matrix strategy. Timeline
Review quality assessmentOverlap was good. The fullsend review agent independently identified 3 of the same core issues found by qodo and the human squad: (1) pipe subshell loop aborting on first failure, (2) GHEOF heredoc delimiter collision risk, (3) shared eval infrastructure changes no longer triggering tests. The remediations included ready-to-use code examples. Severity underrating recurred. The review agent rated the shared infrastructure coverage regression as medium. The human squad rated it HIGH — correctly, since it removes merge-gate signal for a class of changes that the old behavior tested. This severity gap flipped the verdict from Security gap. The human squad identified that the dynamic agent selection pattern allows new harness+eval config pairs to inherit GCP WIF credentials and EVAL_GH_TOKEN without any workflow file modification — removing a review safeguard. The fullsend security sub-agent did not flag this trust model expansion. Existing issues with new evidence
Proposals filed |
Summary
.github/scripts/select-eval-agents.shwhich parsesharness/*.yamlto determine which agents need functional testing based on changed files in a PRrun-functional.sh triage/run-functional.sh reviewsteps with a loop over dynamically selected agentsrelevantfiles grep check — agent selection subsumes itenv/**,common/**,policies/**,skills/**,plugins/**,docs/**)This solves the
pull_request_targetchicken-and-egg problem: new agents can be added with their eval configs in a single PR without also needing to modify the workflow YAML.Test plan
select-eval-agents-test.shcovering direct harness changes, transitive references, multi-agent selection, exclusion of agents without eval configs, and variable path filtering🤖 Generated with Claude Code