ci: parallel functional tests with dynamic matrix strategy - #148
Conversation
|
🤖 Review · |
PR Summary by QodoCI: parallel functional tests via dynamic agent matrix + roll-up check
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. FILE_COUNT unsanitized in ::warning
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 Finished Review · ✅ Success · Started 11:38 AM UTC · Completed 11:56 AM UTC |
ReviewRe-review of 99c0f1a (prior review at 265a4b3, provenance: app-verified). This PR restructures the functional-tests workflow from a single sequential job into a 4-job pipeline (gate → detect → matrix functional-tests → roll-up) and adds a review agent eval suite. Two commits since the prior review address most prior findings: gate added to roll-up needs with GATE_RESULT checking, Prior findings addressed
FindingsMedium
Low
SummarySubstantial progress since the prior review: 6 of 8 prior findings are fully resolved, and the remaining medium finding (roll-up gate gap) has been partially addressed by adding Previous runReviewRe-review of 265a4b3 (prior review at 6f11dd3, provenance: app-verified). FindingsMedium
Low
First runReviewRe-review of 6f11dd3 (prior review at 961cf67, provenance: app-verified). FindingsMedium
Low
First runHigh
Medium
Previous runReviewRe-review of 265a4b3 (prior review at 6f11dd3, provenance: app-verified). This PR restructures the functional-tests workflow from a single sequential job into a 4-job pipeline (gate → detect → matrix functional-tests → roll-up) and adds a review agent eval suite. The architecture is sound — matrix-based parallelism with a stable roll-up job name for branch protection is the standard GitHub Actions pattern for dynamic test matrices. Prior findings addressed
FindingsMedium
Low
SummaryThe high-severity security findings from the first review (command injection, HEREDOC delimiter injection) are both fixed, and yq integrity verification was added. The remaining medium finding (roll-up gate gap) has been flagged in three consecutive reviews — it is worth resolving, but has a straightforward workaround via branch protection configuration. The PR is well-tested (18 test scenarios for the new script), the architecture is sound, and the overall security posture has improved significantly. Previous run (2)ReviewRe-review of 6f11dd3 (prior review at 961cf67, provenance: app-verified). FindingsMedium
Low
First runHigh
Medium
Previous run (3)ReviewRe-review of 6f11dd3 (prior review at 961cf67, provenance: app-verified). This PR restructures the functional-tests workflow from a single sequential job into a 4-job pipeline (gate → detect → matrix functional-tests → roll-up) and adds a review agent eval suite. The restructuring is well-designed — matrix-based parallelism with a stable roll-up job name for branch protection is the standard GitHub Actions pattern for dynamic test matrices. Prior findings addressed
FindingsMedium
Low
SummarySignificant security improvements since the prior review — the high-severity command injection and HEREDOC delimiter issues are both fixed, and yq integrity verification was added. The remaining medium finding (roll-up gate gap) is worth addressing but is not blocking given that branch protection can be configured with Previous run (4)ReviewFindingsHigh
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass (4 agents: Claude, Claude, Gemini, Codex) — new findings not already covered by the prior qodo-code-review pass. Full triage report was shared inline in the session; posting the unique Medium+ findings here.
Note: 3 of qodo-code-review's existing findings (roll-up masking gate/detect failures, unverified yq download, unwired test suite) are still open/unreplied and match findings independently raised by this squad — not reposted here to avoid duplication, but worth resolving alongside these.
|
🤖 Finished Fix · ❌ Failure · Started 11:57 AM UTC · Completed 12:17 PM UTC |
961cf67 to
6f11dd3
Compare
|
🤖 Finished Review · ✅ Success · Started 2:08 PM UTC · Completed 2:21 PM UTC |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 4:22 PM UTC · Completed 4:33 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
- Add gate to roll-up job needs list so unauthorized PRs don't get a green functional-tests-complete check - Rename expected_labels to required_labels in review eval for consistency with triage eval - Add tests for empty stdin and duplicate file inputs Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Re-review squad pass (3 agents: Claude, Claude, Gemini) against the current head (265a4b3), after the 3 follow-up fix commits. No CRITICAL or HIGH issues found — verified the matrix.agent injection fix, GITHUB_OUTPUT delimiter fix, yq checksum pinning, push-event diff-based selection, and test-suite CI wiring are all correctly implemented.
One new MEDIUM regression from the fix itself, posted inline (extract_refs pipefail/grep-empty false-failure — currently dormant, no active harness file triggers it).
Worth noting for follow-up (not blocking this approval):
- The roll-up job (functional-tests-complete) now includes
gateinneedsand checksGATE_RESULT, but this doesn't fully close the previously-flagged gap (fullsend-ai-review comment on line 352/373):check-e2e-authorizationnever fails thegatejob on a denied/unauthorized PR (it only setsauthorized=falseand posts a comment), soGATE_RESULTstayssuccessin that case and the roll-up still can't distinguish "denied authorization, nothing ran" from "legitimately nothing to test." Still open, unreplied. - A couple of LOW items from earlier bot passes remain open and unreplied (judge-naming inconsistency in eval/review/eval.yaml, missing empty-stdin/duplicate-input test cases) — not re-flagged here since already tracked.
Approving based on no blocking issues in the current diff.
|
🤖 Finished Review · ✅ Success · Started 5:17 PM UTC · Completed 5:27 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
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>
Restructure the functional-tests workflow into four jobs: - gate: PR authorization (unchanged) - detect: lightweight job that checks out the PR, runs select-eval-agents.sh, and outputs a JSON array of agent names - functional-tests: matrix over detected agents, each leg does full setup and runs run-functional.sh independently and in parallel - functional-tests-complete: roll-up job with a stable name for branch protection required checks The matrix approach gives true parallelism (triage and review run simultaneously on separate runners), independent failure isolation (fail-fast: false), and clean per-agent log streams. The roll-up job handles all dynamic matrix shapes including empty (no agents to test). Artifact names are per-agent (eval-results-<agent>) to avoid conflicts between matrix legs. 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>
…d FIXTURE_NUMBER The functional-tests-complete roll-up job only checked for "failure" from matrix legs. A cancelled leg (e.g., runner preemption) would silently pass the roll-up, giving a false green on the required check. Add a cancelled check alongside failure. Also add a :? guard on FIXTURE_NUMBER in run-fullsend.sh for consistency with the other fixture variables — gives a clear error message if the before_each hook fails to set it. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…ilures Address review feedback: - Pass matrix.agent through env var to prevent expression injection in the run: block (the critical security finding) - Validate agent names against ^[a-zA-Z0-9_-]+$ in select-eval-agents.sh as defense-in-depth against malicious harness filenames - Remove 2>/dev/null from yq in extract_refs so parse failures surface instead of silently producing empty ref lists - Replace process substitution with command substitution for extract_refs so set -e can catch failures - Add sha256 verification for yq downloads (matching script-test.yml) - Use random heredoc delimiter instead of static GHEOF - Add truncation detection for PR file lists (matching merge_group path) - Check detect job result in the roll-up job so a broken detect cannot silently pass the required check - Invoke select-eval-agents.sh via bash for consistency - Add select-eval-agents-test.sh to Makefile script-test target - Add tests for malformed YAML and invalid agent names Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…filter embedded vars
- Add push event diff-based selection using github.event.before/after
compare API, same pattern as merge_group — avoids running all agents
unconditionally on every push to main
- Filter variable references containing $ anywhere in the value, not
just at the start — prevents mid-path vars like env/${AGENT}.env from
passing through as unresolvable literals
- Add test for embedded variable filtering
Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
- Add gate to roll-up job needs list so unauthorized PRs don't get a green functional-tests-complete check - Rename expected_labels to required_labels in review eval for consistency with triage eval - Add tests for empty stdin and duplicate file inputs Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
When every tracked field in a harness YAML is a variable reference,
grep -v filters out all lines and exits 1. Under pipefail, this made
extract_refs return non-zero, which the caller misinterpreted as a yq
parse failure. Wrap grep in { ... || true; } so an empty match set is
treated as legitimate empty output rather than a fatal error.
Add a test covering this scenario.
Signed-off-by: Ralph Bean <rbean@redhat.com>
Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
99c0f1a to
feaaed4
Compare
|
🤖 Finished Review · ✅ Success · Started 7:21 PM UTC · Completed 7:38 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 7:29 PM UTC · Completed 7:39 PM UTC |
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
|
PR #148 was a human-authored CI restructuring (parallel functional tests with dynamic matrix strategy) that went through 4 successful review rounds, 1 failed fix agent run, and 5 human fix commits before merging ~8 hours after creation. Total agent cost was ~$23.62 ($18.78 review + $4.84 fix). Review quality was strong: the review agent caught critical security issues (command injection, heredoc injection, error handling gaps) on the first pass and tracked findings across re-reviews with a clear status table. The fix agent produced correct fixes but its push was rejected because the GitHub App token lacks Proposals
Proposals filed
|
The 001-clean-approve review eval case has been broken since PR #148 merged. The post-review script returns 422 errors from inline review comments citing invalid line numbers, which prevents the ready-for-merge label from being applied. This causes the required_labels judge to fail (pass_rate=0.0%), blocking functional-tests-complete and the entire merge queue. Remove the case directory to unblock the merge queue. The review eval infrastructure (eval.yaml, hooks, judges, thresholds) is preserved. The test case should be reintroduced after the line number accuracy issue is fixed (see PR #197). Closes #202
2 participants
Summary
gate→detect→functional-tests(matrix) →functional-tests-complete(roll-up)detectjob runsselect-eval-agents.sh(from ci: dynamically select eval agents from changed files #145) and outputs a JSON array of agents to testfunctional-tests-completeis a stable roll-up job name suitable for branch protection required checks — it passes when all matrix legs pass (or when no agents need testing) and fails if any leg failsSupersedes #145 (includes the
select-eval-agents.shscript and tests from that PR).Why matrix over sequential loop?
fail-fast: falsefunctional-tests-complete) works with branch protection regardless of which/how many agents are in the matrixTest plan
select-eval-agents.shpass🤖 Generated with Claude Code