From 08cc447e8acaa96ae80819c100dd62aab36e4545 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 3 Nov 2026 17:42:33 -0800 Subject: [PATCH] fix(evals): screen build-image keys the CI image on the merge commit it tests (#3962) * test(evals): build-image must key the screen's image on its merge commit (fails on main) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(evals): screen build-image keys the CI image on the merge commit it tests Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes (superseded screens build no image) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- .github/workflows/evals.yml | 11 ++++++++++- .../gsta-260-screen-image-merge-lock.md | 2 ++ test/evals-train-workflow.test.ts | 19 ++++++++++++++++--- 3 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 changelog.d/gsta-260-screen-image-merge-lock.md diff --git a/.github/workflows/evals.yml b/.github/workflows/evals.yml index b662c8867..f74e4a8b7 100644 --- a/.github/workflows/evals.yml +++ b/.github/workflows/evals.yml @@ -76,7 +76,13 @@ jobs: # (EV6, fork port wave 2). Skip the build for dependabot; the evals job's # explicit actor guard mirrors it because no eval test selects on a lockfile-only # diff — a maintainer's next push rebuilds the image with real perms. - if: github.actor != 'dependabot[bot]' + # A dispatched screen's slices test inputs.screen_merge_sha, so the image is + # keyed and built from that commit's bun.lock, not the dispatch ref's (screen + # 41662815506 ran Playwright 1.64.1 against main's Chromium). It waits for the + # route job, which refuses a screen of a fork or closed PR, and skips a + # superseded screen, whose slices plan nothing. + needs: [route] + if: github.actor != 'dependabot[bot]' && needs.route.outputs.superseded != 'true' runs-on: ubicloud-standard-8 timeout-minutes: 15 permissions: @@ -87,6 +93,9 @@ jobs: runtime-id: ${{ steps.runtime.outputs.id }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + ref: ${{ inputs.screen_merge_sha || github.sha }} + persist-credentials: false - id: meta # Key on Dockerfile + lockfile only. package.json is deliberately NOT diff --git a/changelog.d/gsta-260-screen-image-merge-lock.md b/changelog.d/gsta-260-screen-image-merge-lock.md new file mode 100644 index 000000000..9b044eabf --- /dev/null +++ b/changelog.d/gsta-260-screen-image-merge-lock.md @@ -0,0 +1,2 @@ +### For contributors +- **A dispatched screen now builds its CI image from the commit it tests.** evals.yml's `build-image` job used to check out the dispatch ref (main) and key the image on main's `bun.lock`, while the slices checked out the PR's merge commit. A PR that changed the lockfile was then tested against main's installed browsers. Screen run 41662815506 for #3947 failed eight cases with `Executable doesn't exist … chromium_headless_shell-1248` because the image had only revision 1234. `build-image` now checks out `screen_merge_sha` (or `github.sha` for every other run) before it hashes and builds, without persisting the token. It also waits for the `route` job: a refused dispatch (a fork or closed PR, for one) and a superseded screen build no image. diff --git a/test/evals-train-workflow.test.ts b/test/evals-train-workflow.test.ts index 4e098f78e..839f7af6e 100644 --- a/test/evals-train-workflow.test.ts +++ b/test/evals-train-workflow.test.ts @@ -224,7 +224,20 @@ describe('under a train block (the gate edits, each only on its mode)', () => { expect(sim.steps('slices-report')).not.toContain('Write the train record'); expect(sim.step('slices-comment', 'Post PR comment')!.env).toMatchObject({ PR_NUMBER: '42', SCREEN_POLICY: 'narrow', SCREEN_MERGE_SHA: SHA.merge }); const superseded = simulate(evals, { github: dispatch(), inputs, outputs: { route: { ...route, superseded: 'true' } } }); - expect(superseded.jobs).toEqual(['build-image', 'route']); + expect(superseded.jobs).toEqual(['route']); + }); + + test('build-image keys and builds the CI image from the commit the slices test, only after the route accepts the run', () => { + const inputs = { ...INPUTS, evals_all: false, nonce: 'pr.screen.narrow-x', expected_sha: SHA.merge, base_ref: 'main', base_sha: SHA.base, screen_pr: '42', screen_merge_sha: SHA.merge, screen_policy: 'narrow' }; + const route = { ...ROUTE, ...train, mode: 'screen', screen_head: SHA.screenHead, screen_base: SHA.screenBase, screen_branch: 'feature' }; + const screen = simulate(evals, { github: dispatch(), inputs, outputs: { route } }); + expect(screen.checkout('build-image')?.ref).toBe(SHA.merge); + expect(screen.checkout('build-image')?.ref).toBe(screen.checkout('eval-slices')?.ref); + expect(screen.checkout('build-image')?.['persist-credentials']).toBe(false); + const steps = evals.jobs['build-image']!.steps; + expect(steps.findIndex(st => st.uses?.startsWith('actions/checkout@'))).toBeLessThan(steps.findIndex(st => st.id === 'meta')); + expect(simulate(evals, { github: dispatch(), inputs, outputs: { route }, results: { route: 'failure' } }).jobs).not.toContain('build-image'); + for (const github of [pr(), dispatch()]) expect(simulate(evals, { github, inputs: github.event_name === 'pull_request' ? {} : INPUTS, outputs: { route: { ...ROUTE, mode: github.event_name === 'pull_request' ? 'pr' : 'dispatch' } } }).checkout('build-image')?.ref).toBe(SHA.main); }); test('a train gate runs the full gate, writes its record from the report and publishes the check; it never reuses or binds', () => { @@ -262,9 +275,9 @@ describe('under a train block (the gate edits, each only on its mode)', () => { expect(job.steps.find(st => st.name === 'Verify the prefix checkout')!.run).toContain('echo "short=${PREFIX_SHA:0:12}"'); }); - test('a refused dispatch starts nothing paid', () => { + test('a refused dispatch starts nothing paid and builds no image', () => { const sim = simulate(evals, { github: dispatch('train/t1'), inputs: { ...INPUTS, nonce: 'gate.bisect-x' }, outputs: { route: { ...ROUTE, mode: '' } }, results: { route: 'failure' } }); - expect(sim.jobs).toEqual(['build-image', 'route']); + expect(sim.jobs).toEqual(['route']); }); });