From ff779be30e7bf986e15e827c7ff8122c26a6660b Mon Sep 17 00:00:00 2001 From: Cindy Zhang <2781099+cixzhang@users.noreply.github.com> Date: Wed, 26 Aug 2026 23:38:42 -0700 Subject: [PATCH] fix(ci): restore visual acceptance publication --- .../scripts/visual-gate/gate-context.test.mjs | 27 +++++++-- .github/scripts/visual-gate/gate.mjs | 9 ++- .../visual-gate/visual-acceptance.test.mjs | 8 +++ .../visual-gate/workflow-concurrency.test.mjs | 56 +++++++++++++------ .github/workflows/pr-comment.yml | 21 +++++-- .github/workflows/visual-acceptance.yml | 11 +--- 6 files changed, 91 insertions(+), 41 deletions(-) diff --git a/.github/scripts/visual-gate/gate-context.test.mjs b/.github/scripts/visual-gate/gate-context.test.mjs index 177770e759782..f19b45b767891 100644 --- a/.github/scripts/visual-gate/gate-context.test.mjs +++ b/.github/scripts/visual-gate/gate-context.test.mjs @@ -13,8 +13,10 @@ const SCRIPT = path.join( 'gate.mjs', ); const IDENTITY_ENV = [ + 'ASTRYX_VISUAL_SHA', 'ASTRYX_VISUAL_RUN_ID', 'ASTRYX_VISUAL_RUN_ATTEMPT', + 'GITHUB_SHA', 'GITHUB_RUN_ID', 'GITHUB_RUN_ATTEMPT', ]; @@ -60,8 +62,13 @@ afterEach(() => fs.rmSync(root, {recursive: true, force: true})); describe('visual gate capture identity', () => { it('records the GitHub run identity by default', () => { expect( - captureContext({GITHUB_RUN_ID: '123', GITHUB_RUN_ATTEMPT: '2'}), + captureContext({ + GITHUB_SHA: 'a'.repeat(40), + GITHUB_RUN_ID: '123', + GITHUB_RUN_ATTEMPT: '2', + }), ).toMatchObject({ + sha: 'a'.repeat(40), runId: '123', runAttempt: '2', }); @@ -70,26 +77,38 @@ describe('visual gate capture identity', () => { it('records an explicit trusted recapture identity ahead of workflow defaults', () => { expect( captureContext({ + GITHUB_SHA: 'a'.repeat(40), GITHUB_RUN_ID: '999', GITHUB_RUN_ATTEMPT: '4', + ASTRYX_VISUAL_SHA: 'b'.repeat(40), ASTRYX_VISUAL_RUN_ID: '123', ASTRYX_VISUAL_RUN_ATTEMPT: '2', }), - ).toMatchObject({runId: '123', runAttempt: '2'}); + ).toMatchObject({ + sha: 'b'.repeat(40), + runId: '123', + runAttempt: '2', + }); }); it('records missing identity as null', () => { - expect(captureContext()).toMatchObject({runId: null, runAttempt: null}); + expect(captureContext()).toMatchObject({ + sha: null, + runId: null, + runAttempt: null, + }); }); it('preserves invalid explicit identity for downstream rejection', () => { expect( captureContext({ + GITHUB_SHA: 'a'.repeat(40), GITHUB_RUN_ID: '999', GITHUB_RUN_ATTEMPT: '4', + ASTRYX_VISUAL_SHA: 'invalid', ASTRYX_VISUAL_RUN_ID: '', ASTRYX_VISUAL_RUN_ATTEMPT: 'invalid', }), - ).toMatchObject({runId: '', runAttempt: 'invalid'}); + ).toMatchObject({sha: 'invalid', runId: '', runAttempt: 'invalid'}); }); }); diff --git a/.github/scripts/visual-gate/gate.mjs b/.github/scripts/visual-gate/gate.mjs index 1c29d52ff8a76..e41d7dd21a563 100644 --- a/.github/scripts/visual-gate/gate.mjs +++ b/.github/scripts/visual-gate/gate.mjs @@ -47,7 +47,8 @@ const flag = name => { return index === -1 ? null : argv[index + 1]; }; const has = name => argv.includes(`--${name}`); -const captureRunIdentity = () => ({ +const captureIdentity = () => ({ + sha: process.env.ASTRYX_VISUAL_SHA ?? process.env.GITHUB_SHA ?? null, runId: process.env.ASTRYX_VISUAL_RUN_ID ?? process.env.GITHUB_RUN_ID ?? null, runAttempt: process.env.ASTRYX_VISUAL_RUN_ATTEMPT ?? process.env.GITHUB_RUN_ATTEMPT ?? null, @@ -202,11 +203,10 @@ async function runCapture(shots) { // contributor's head and the base separately: acceptance binds to the head // humans reviewed, while post-merge verification bridges to the final // squash commit by comparing rendered hashes. - sha: process.env.GITHUB_SHA ?? null, + ...captureIdentity(), headSha: process.env.ASTRYX_PR_HEAD_SHA ?? null, baseSha: process.env.ASTRYX_PR_BASE_SHA ?? null, ref: process.env.GITHUB_REF ?? null, - ...captureRunIdentity(), }; fs.writeFileSync( path.join(outDir, 'manifest.json'), @@ -242,11 +242,10 @@ async function check() { generatedAt: new Date().toISOString(), reason: `${shots.length} shots exceeds the ${maxShots}-shot budget${components.length ? ` (${components.length} components touched)` : ''} — too broad to review shot by shot here. The daily release gate covers this change against the full baseline.`, context: { - sha: process.env.GITHUB_SHA ?? null, + ...captureIdentity(), headSha: process.env.ASTRYX_PR_HEAD_SHA ?? null, baseSha: process.env.ASTRYX_PR_BASE_SHA ?? null, ref: process.env.GITHUB_REF ?? null, - ...captureRunIdentity(), tiers, scoped: components.length > 0, components, diff --git a/.github/scripts/visual-gate/visual-acceptance.test.mjs b/.github/scripts/visual-gate/visual-acceptance.test.mjs index 3e3f1f576f9b5..fda925ff7a187 100644 --- a/.github/scripts/visual-gate/visual-acceptance.test.mjs +++ b/.github/scripts/visual-gate/visual-acceptance.test.mjs @@ -280,6 +280,14 @@ describe('visual acceptance', () => { }); }); + it('returns success for a clean trusted capture without acceptance', () => { + writeEvidence({run: 124, status: 'pass'}); + expect(JSON.parse(run('state', {pages, pr: 42, head: HEAD}))).toMatchObject({ + state: 'success', + reason: 'clean', + }); + }); + it('derives a trusted component plan from baseline themes and the Storybook index', () => { const storybook = path.join(root, 'storybook'); fs.mkdirSync(storybook); diff --git a/.github/scripts/visual-gate/workflow-concurrency.test.mjs b/.github/scripts/visual-gate/workflow-concurrency.test.mjs index 49ade917b241c..0833383f93bd2 100644 --- a/.github/scripts/visual-gate/workflow-concurrency.test.mjs +++ b/.github/scripts/visual-gate/workflow-concurrency.test.mjs @@ -47,6 +47,14 @@ describe('visual acceptance workflow concurrency', () => { 'group: visual-acceptance-head-${{ github.event.pull_request.head.repo.id }}-${{ github.event.pull_request.head.ref }}', ); expect(initialize).toContain('cancel-in-progress: true'); + expect(initialize).toContain('pull-requests: read'); + expect(initialize).not.toContain('pull-requests: write'); + expect(initialize).not.toContain('issues: write'); + expect(initialize).not.toContain('removeLabel'); + expect(initialize).toContain( + "state: scope.hasStableVisual ? 'pending' : 'success'", + ); + expect(initialize).toContain("'No stable visual scope.'"); expect(authorize).not.toContain(': write'); expect(authorize).not.toContain('actions/checkout'); expect(authorize).not.toContain('issues.createComment'); @@ -56,43 +64,57 @@ describe('visual acceptance workflow concurrency', () => { 'group: visual-acceptance-head-${{ needs.authorize.outputs.head_repo_id }}-${{ needs.authorize.outputs.head_ref }}', ); expect(accept).toContain('cancel-in-progress: false'); - }); - - it('grants PR mutation permission wherever labels and comments are projected', () => { - const value = workflow('visual-acceptance.yml'); - const initialize = value.slice( - value.indexOf(' initialize:'), - value.indexOf(' authorize:'), - ); - const accept = value.slice(value.indexOf(' accept:')); - - expect(initialize).toContain('pull-requests: write'); expect(accept).toContain('pull-requests: write'); + expect(accept).not.toContain('issues: write'); }); - it('uses the same head identity for post-merge promotion', () => { - const value = workflow('visual-acceptance-promote.yml'); + it('invalidates the advisory label only in the trusted workflow_run publisher', () => { + const value = workflow('pr-comment.yml'); + const invalidate = value.slice( + value.indexOf(' invalidate:'), + value.indexOf(' comment:'), + ); - expect(value).toContain( - 'group: visual-acceptance-head-${{ github.event.pull_request.head.repo.id }}-${{ github.event.pull_request.head.ref }}', + expect(invalidate).toContain('pull-requests: write'); + expect(invalidate).toContain('statuses: write'); + expect(invalidate).toContain('createCommitStatus'); + expect(invalidate).toContain('issues.removeLabel'); + expect(invalidate.indexOf('createCommitStatus')).toBeLessThan( + invalidate.indexOf('issues.removeLabel'), ); - expect(value).not.toContain('visual-acceptance-pr-'); + const publisher = value.slice(value.indexOf(' comment:')); + expect(publisher).toContain("if (state.reason === 'accepted')"); + expect(publisher).toContain('issues.addLabels'); + expect(publisher).toContain('issues.removeLabel'); }); - it('passes triggering CI identity through dedicated capture variables', () => { + it('passes source CI identity without overriding reserved GitHub variables', () => { const value = workflow('pr-comment.yml'); const capture = value.slice( value.indexOf(' - name: Capture the trusted stable visual scope'), value.indexOf(' # The Storybook bundle is untrusted.'), ); + expect(capture).toContain( + 'ASTRYX_VISUAL_SHA: ${{ steps.identity.outputs.head_sha }}', + ); expect(capture).toContain( 'ASTRYX_VISUAL_RUN_ID: ${{ steps.identity.outputs.run_id }}', ); expect(capture).toContain( 'ASTRYX_VISUAL_RUN_ATTEMPT: ${{ steps.identity.outputs.run_attempt }}', ); + expect(capture).not.toContain('GITHUB_SHA:'); expect(capture).not.toContain('GITHUB_RUN_ID:'); expect(capture).not.toContain('GITHUB_RUN_ATTEMPT:'); }); + + it('uses the same head identity for post-merge promotion', () => { + const value = workflow('visual-acceptance-promote.yml'); + + expect(value).toContain( + 'group: visual-acceptance-head-${{ github.event.pull_request.head.repo.id }}-${{ github.event.pull_request.head.ref }}', + ); + expect(value).not.toContain('visual-acceptance-pr-'); + }); }); diff --git a/.github/workflows/pr-comment.yml b/.github/workflows/pr-comment.yml index 8094ee13ccb50..be002ae05768a 100644 --- a/.github/workflows/pr-comment.yml +++ b/.github/workflows/pr-comment.yml @@ -78,21 +78,33 @@ jobs: needs.resolve.outputs.valid == 'true' runs-on: ubuntu-slim permissions: + pull-requests: write statuses: write steps: - - name: Mark same-head reruns pending before evidence can change + - name: Invalidate visible approval for the new evidence run uses: actions/github-script@v9 with: retries: 3 script: | + const {owner, repo} = context.repo; const run = context.payload.workflow_run; + const pr = Number('${{ needs.resolve.outputs.pr_number }}'); + const head = '${{ needs.resolve.outputs.head_sha }}'; await github.rest.repos.createCommitStatus({ - ...context.repo, - sha: '${{ needs.resolve.outputs.head_sha }}', + owner, + repo, + sha: head, context: 'visual-acceptance', state: 'pending', description: `CI run ${run.id}/${run.run_attempt} is producing fresh visual evidence.`, }); + try { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: pr, name: 'visual-approved', + }); + } catch (error) { + if (error.status !== 404) throw error; + } comment: needs: resolve @@ -103,7 +115,6 @@ jobs: runs-on: 2-core-ubuntu-arm permissions: pull-requests: write - issues: write statuses: write actions: read contents: write # immutable validated visual evidence on gh-pages @@ -269,7 +280,7 @@ jobs: - name: Capture the trusted stable visual scope if: steps.identity.outputs.valid == 'true' && steps.scope.outputs.stable == 'true' env: - GITHUB_SHA: ${{ steps.identity.outputs.head_sha }} + ASTRYX_VISUAL_SHA: ${{ steps.identity.outputs.head_sha }} ASTRYX_VISUAL_RUN_ID: ${{ steps.identity.outputs.run_id }} ASTRYX_VISUAL_RUN_ATTEMPT: ${{ steps.identity.outputs.run_attempt }} ASTRYX_PR_HEAD_SHA: ${{ steps.identity.outputs.head_sha }} diff --git a/.github/workflows/visual-acceptance.yml b/.github/workflows/visual-acceptance.yml index 1b41169925fd0..067eb5f60158f 100644 --- a/.github/workflows/visual-acceptance.yml +++ b/.github/workflows/visual-acceptance.yml @@ -25,8 +25,7 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - pull-requests: write - issues: write + pull-requests: read statuses: write steps: - name: Checkout trusted default-branch code @@ -86,13 +85,6 @@ jobs: ? 'Stable visual capture is still running.' : 'No stable visual scope.', }); - try { - await github.rest.issues.removeLabel({ - owner, repo, issue_number: pr.number, name: 'visual-approved', - }); - } catch (error) { - if (error.status !== 404) throw error; - } authorize: if: >- @@ -208,7 +200,6 @@ jobs: permissions: contents: write pull-requests: write - issues: write statuses: write steps: - name: Report a refused decision