fix: prevent duplicate "Download container images" step in codex evals job#45991
Conversation
…n evals job Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
In buildEvalsJobSteps, buildPullAWFContainersStep was called unconditionally. For engine: codex, buildEvalsEngineSteps → generateMCPSetup also emits the Docker download step, producing a duplicate that trips duplicate-step validation. Fix: skip buildPullAWFContainersStep for codex (same pattern as the detection job fix in threat_detection_steps.go). Add regression tests for both the codex case (no duplicate) and non-codex engines (step still present). Also add evals sections to changeset.md, necromancer.md, and duplicate-code-detector.md, which were excluded pending this fix, and recompile all affected workflows. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45991 does not have the 'implementation' label and has only 70 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR fixes a gh-aw compiler validation failure for engine: codex workflows with evals: by preventing the evals job from emitting two steps with the same name (Download container images). It also adds eval question blocks to several workflows and includes the resulting regenerated workflow outputs.
Changes:
- Avoid generating a duplicate
Download container imagesstep in theevalsjob for the Codex engine by skipping the extra pre-pull step. - Add regression tests to ensure Codex produces exactly one download step and non-Codex engines still include the download step.
- Add
evals:sections to multiple workflow markdown sources and recompile the corresponding generated workflow YAMLs.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/evals_steps.go | Skip buildPullAWFContainersStep for Codex to prevent duplicate “Download container images” in the evals job. |
| pkg/workflow/evals_steps_test.go | Add regression tests covering Codex/no-duplicate and non-Codex/still-present behavior. |
| .github/workflows/changeset.md | Add evals: questions to the workflow source. |
| .github/workflows/necromancer.md | Add evals: questions to the workflow source. |
| .github/workflows/duplicate-code-detector.md | Add evals: questions to the workflow source. |
| .github/workflows/changeset.lock.yml | Regenerated compiled workflow reflecting evals support and related jobs/steps. |
| .github/workflows/necromancer.lock.yml | Regenerated compiled workflow reflecting evals support and related jobs/steps. |
| .github/workflows/duplicate-code-detector.lock.yml | Regenerated compiled workflow reflecting evals support and related jobs/steps. |
| .github/workflows/skillet.lock.yml | Regenerated compiled workflow output after recompile. |
| .github/workflows/release.lock.yml | Regenerated compiled workflow output after recompile. |
| .github/workflows/hourly-ci-cleaner.lock.yml | Regenerated compiled workflow output after recompile. |
| .github/workflows/cli-consistency-checker.lock.yml | Regenerated compiled workflow output after recompile (includes strict-policy related output changes). |
| .github/workflows/avenger.lock.yml | Regenerated compiled workflow output after recompile. |
| .github/workflows/agentic-auto-upgrade.yml | Regenerated auto-upgrade workflow; schedule cron changed in this diff. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Low
| // For codex, buildEvalsEngineSteps calls generateMCPSetup which already emits | ||
| // the Docker download step via generateDownloadDockerImagesStep, so skip here | ||
| // to avoid a duplicate "Download container images" step name. | ||
| if c.getEvalsEngineID(data) != "codex" { |
There was a problem hiding this comment.
Fixed in 7ff45da — replaced "codex" with string(constants.CodexEngine).
| on: | ||
| schedule: | ||
| - cron: "11 4 * * 6" # Weekly (auto-upgrade) | ||
| - cron: "21 3 * * 5" # Weekly (auto-upgrade) | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Reverted in 9fced06 — the cron change was an inadvertent side-effect of running make recompile (the scatter is supposed to be deterministic but apparently drifted). The file is back to "11 4 * * 6" matching the pre-PR state.
There was a problem hiding this comment.
The fix is correct and well-reasoned. The root cause (unconditional buildPullAWFContainersStep in buildEvalsJobSteps colliding with the codex MCP setup path that already emits the same step) is clearly identified, and the guard matches the existing pattern used elsewhere (engine.GetID() == codex). Tests cover both the regression (exactly 1 step for codex) and the normal path (step still present for non-codex engines). LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 21.6 AIC · ⌖ 4.29 AIC · ⊞ 5K
|
Hey
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (6 tests)
Code Quality Highlights✅ 0% implementation tests (threshold: 30%) — all tests verify behavioral contracts and design invariants, not implementation details. ✅ No violations — proper build tags, no mock libraries (gomock/testify/mock), all assertions have descriptive failure messages. ✅ High-value regression test — ✅ Edge case coverage — multiple engine types (arc-dind vs. standard, codex vs. non-codex), step ordering constraints, secret resolution. Verdict✅ Passed. 0% implementation tests (0% < 30% threshold). No coding violations. Tests provide robust coverage of behavioral contracts and regression prevention for the fix. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix is correct, well-targeted, and properly regression-tested.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause correctly identified:
buildEvalsJobStepsunconditionally emitsbuildPullAWFContainersStep, whilebuildEvalsEngineSteps → generateMCPSetupalso emits it for codex. - ✅ Fix mirrors the established pattern in
threat_detection_steps.go— good consistency. - ✅ Two regression tests: one asserts exactly-one download step for codex, one asserts non-codex engines still get the step.
- ✅ Guard
c.getEvalsEngineID(data) != "codex"is tightly scoped to the only engine that callsgenerateMCPSetup(confirmed atevals_steps.go:218). - ✅ PR description accurately explains root cause; no documentation gap.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 30.1 AIC · ⌖ 4.35 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Review: non-blocking observations, consider before next similar patch
The immediate bug fix (duplicate step for codex evals) is correct and the regression tests are a good addition. Three non-blocking issues worth addressing before this pattern spreads to more engines:
Findings summary
Medium — fragile negative allowlist (evals_steps.go:47): The guard != "codex" is structurally decoupled from the real condition that causes the duplicate (whether generateMCPSetup is called). A new engine routed through generateMCPSetup would silently re-introduce the same bug. See inline comment.
Medium — asymmetric guard vs threat_detection (evals_steps.go:47): threat_detection_steps.go:41 includes an || usingExternalDetector term; this guard does not. Either document why that case doesn't apply to evals, or mirror the shape. See inline comment.
Low — weak assertion in non-codex test (evals_steps_test.go:1417): strings.Contains won't catch a double-emission. Use strings.Count(...) == 1 to match the codex test. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 66.8 AIC · ⌖ 4.71 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/evals_steps.go:47
Fragile negative allowlist — any new engine routed through generateMCPSetup will silently re-introduce the duplicate step.
<details>
<summary>💡 Details & suggested fix</summary>
The guard reads:
if c.getEvalsEngineID(data) != "codex" {
steps = append(steps, c.buildPullAWFContainersStep(data)...)
}This works today because only codex reaches generateMCPSetup inside buildEvalsEngineSteps (line 218). But the check at line 47 is structurally decoupled from that decision …
pkg/workflow/evals_steps.go:47
Guard in evals_steps.go is asymmetric with the equivalent guard in threat_detection_steps.go — the external-detector case is silently unhandled.
<details>
<summary>💡 Details</summary>
threat_detection_steps.go:41 uses:
if c.getThreatDetectionEngineID(data) != "codex" || usingExternalDetector {
steps = append(steps, c.buildPullAWFContainersStep(data)...)
}The evals guard added here uses only != "codex" with no usingExternalDetector equivalent. If an analogous fea…
pkg/workflow/evals_steps_test.go:1417
Non-codex test uses strings.Contains — it won't catch a double-emission for non-codex engines.
<details>
<summary>💡 Suggested fix</summary>
strings.Contains passes even if the step appears twice. Use the same count-based assertion as the codex test:
count := strings.Count(allSteps, "name: Download container images")
if count != 1 {
t.Errorf("expected exactly one 'Download container images' step for %s evals job, got %d", engine, count)
}This makes the non-codex test …
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… in evals_steps.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
engine: codexworkflows withevals:triggered a compiler validation error —duplicate step 'Download container images' found in job 'evals'— because the step was emitted twice: once bybuildPullAWFContainersStep(unconditional inbuildEvalsJobSteps) and again bygenerateMCPSetupinsidebuildEvalsEngineSteps(codex always needs AWF gateway config, even with no MCP tools).The detection job had the same issue and was already patched in
threat_detection_steps.go.Changes
pkg/workflow/evals_steps.go— skipbuildPullAWFContainersStepfor codex;generateMCPSetupalready emits the download step for that engine:pkg/workflow/evals_steps_test.go— two new regression tests:TestBuildEvalsJobStepsCodexNoDuplicateContainerDownload— exactly one download step for codexTestBuildEvalsJobStepsNonCodexIncludesContainerDownload— copilot/claude still get the stepchangeset.md,necromancer.md,duplicate-code-detector.md— addevals:sections, which were held back pending this fix; recompile all 257 workflows (0 errors)