Skip to content

fix(ci): restore optional eval matrix output - #2768

Merged
cquil11 merged 1 commit into
mainfrom
codex/fix-empty-eval-matrix
Aug 28, 2026
Merged

fix(ci): restore optional eval matrix output#2768
cquil11 merged 1 commit into
mainfrom
codex/fix-empty-eval-matrix

Conversation

@cquil11

@cquil11 cquil11 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • restore construction of the single-node eval matrix before publishing eval-config
  • skip optional matrix jobs when their producer output is absent as well as when it is an empty JSON array
  • add a regression test requiring every published matrix shell variable to be initialized

Root cause

PR #2309 removed the EVALS=... assignment while retaining echo "eval-config=$EVALS". Non-eval dispatches therefore published an absent output, and test-sweep-evals attempted fromJson('').

Validation

  • actionlint .github/workflows/e2e-tests.yml
  • python -m pytest utils/test_ci_priority.py -q (19 passed)
  • current no-evals config generation produces eval-config=[]

Note

Low Risk
CI workflow and test-only changes; no runtime product or security impact.

Overview
Fixes a regression where eval-config was published without defining EVALS, so non-eval e2e runs could leave the output empty and test-sweep-evals would fail on fromJson('').

The get-jobs step again builds the single-node eval matrix (non-prefill, non-agentic, run-eval) through score_matrix eval before writing eval-config.

All optional sweep jobs now skip when their matrix output is missing or [], not only when it is an empty JSON array, so downstream fromJson is not invoked on blank strings.

Adds test_e2e_matrix_outputs_reference_initialized_shell_variables in utils/test_ci_priority.py to require every matrix GITHUB_OUTPUT echo 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.

@cquil11
cquil11 requested a review from a team August 28, 2026 04:19
@cquil11
cquil11 merged commit e18a521 into main Aug 28, 2026
8 checks passed
@cquil11
cquil11 deleted the codex/fix-empty-eval-matrix branch August 28, 2026 04:20

@claude claude Bot left a comment

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.

Looks good — this fixes a real bug and adds a regression test for it.

  • Fixed eval-config output in .github/workflows/e2e-tests.yml: the EVALS shell variable was referenced in the output echo but never assigned, so eval-config always 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 '[]', preventing fromJson('') 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 every echo "key=$VAR" in the get-jobs step has a matching VAR=$(...) 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant