fix(ci): restore optional eval matrix output - #2768
Conversation
There was a problem hiding this comment.
Looks good — this fixes a real bug and adds a regression test for it.
- Fixed
eval-configoutput in.github/workflows/e2e-tests.yml: theEVALSshell variable was referenced in the output echo but never assigned, soeval-configalways emitted an empty string. - Hardened all matrix-gating
if:conditions (multi-node-config,eval-config,agentic-config, etc.) to treat an empty/unset output the same as'[]', preventingfromJson('')failures if an output var is ever missing again. - Added
utils/test_ci_priority.py::test_e2e_matrix_outputs_reference_initialized_shell_variables, a static check that everyecho "key=$VAR"in the get-jobs step has a matchingVAR=$(...)assignment — verified by hand that it passes against the current workflow file and would have caught the original bug.
Extended reasoning...
Overview
This PR touches only .github/workflows/e2e-tests.yml and utils/test_ci_priority.py. It fixes a shell-variable bug in the get-jobs job of the e2e workflow (the EVALS variable used in the eval-config output was never assigned before this change, so that output was always an empty string) and defensively updates the downstream job if: conditions to guard against empty-string outputs in general, not just the literal '[]']. It also adds a new unit test that statically parses the workflow YAML to assert every echo "key=$VAR" output line has a corresponding VAR=$(...) assignment, which is a regression test for exactly this class of bug.
Security risks
None. This is CI/workflow configuration and a Python unit test that reads a local file with Path.read_text() and applies regex — no new secrets handling, no new external inputs, no auth/permission logic touched.
Level of scrutiny
Low-to-moderate is appropriate: CI workflow YAML can be subtle, but the diff is small, self-contained, and mechanically verifiable — I traced the pre-change code to confirm EVALS was genuinely unassigned (a real latent bug) and manually replayed the new test's regex against the updated workflow content to confirm it would pass (all echoed output vars have matching assignments) and would have failed against the old file (missing EVALS=).
Other factors
No CODEOWNERS entry singles out these files beyond the repo-wide catch-all. The bug hunter found no issues, and the PR conversation timeline shows no outstanding third-party objections. The change is small, logically consistent between the fix and its accompanying test, and does not touch security- or auth-sensitive code paths.
Summary
eval-configRoot cause
PR #2309 removed the
EVALS=...assignment while retainingecho "eval-config=$EVALS". Non-eval dispatches therefore published an absent output, andtest-sweep-evalsattemptedfromJson('').Validation
actionlint .github/workflows/e2e-tests.ymlpython -m pytest utils/test_ci_priority.py -q(19 passed)eval-config=[]Note
Low Risk
CI workflow and test-only changes; no runtime product or security impact.
Overview
Fixes a regression where
eval-configwas published without definingEVALS, so non-eval e2e runs could leave the output empty andtest-sweep-evalswould fail onfromJson('').The
get-jobsstep again builds the single-node eval matrix (non-prefill, non-agentic,run-eval) throughscore_matrix evalbefore writingeval-config.All optional sweep jobs now skip when their matrix output is missing or
[], not only when it is an empty JSON array, so downstreamfromJsonis not invoked on blank strings.Adds
test_e2e_matrix_outputs_reference_initialized_shell_variablesinutils/test_ci_priority.pyto require every matrixGITHUB_OUTPUTecho to use a shell variable that was assigned in the same workflow block.Reviewed by Cursor Bugbot for commit 037fd01. Bugbot is set up for automated code reviews on this repo. Configure here.