Skip to content

Review 5611

Jump to bottom
Cindy Zhang edited this page Aug 27, 2026 · 1 revision

PR

#5611 fix(ci): bind recovery capture to merged SHA by cixzhang (bucket: the maintainer)

HEAD REVIEWED

eb5c6a7fa06fda65c6b0ef6f0db26faf340bf6ef <- every claim below was verified at this commit

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE: full WHY: the diff sits in a pull_request_target privileged promotion job carrying allow-unsafe-pr-checkout: true — a trust boundary, which fast lane excludes outright.

PRIOR REVIEW: none on the PR. pulls/5611/reviews → empty; pulls/5611/comments → empty; the only issue comments are the Vercel and PR-Analysis bots. Nothing to extend or contradict. The blocker being reconciled is an unposted loop draft from an earlier head, so it is checked by mutation (M1) rather than quoted.

RISK CLASS: low-risk, all four conditions answered — no new API surface (no prop, export, type or theme target); no behaviour regression (the only behaviour that moves is a lane that currently always refuses); no performance or resource regression (zero Effects, listeners, deps, bundle delta); nothing got bigger (nothing renders).

COMMIT TYPE: fix(ci): is honest. It claims a CI fix with nothing consumer-visible, and the diff is one CI env key plus tests — no package, component, story or style file.

PROBLEM

WHY 1: The trusted recovery recapture stamped its manifest with the recovery run's own commit instead of the historical merge it had just checked out and built. WHY 2: Promotion compares that stamp to the resolved merge SHA and refuses on mismatch, so the recovery lane cannot finish. WHY 3: The visual gate exists so a human's "yes" to a frame is the thing that reaches the baseline. When the only recovery path always refuses, that "yes" is spent and discarded and the frames stay out of the baseline.

USER-FACING PROBLEM: A maintainer dispatching recovery for #5162 cannot complete it by any route, and the 124 frames a human already reviewed stay out of the baseline — so the next contributor whose PR touches those keys is shown 124 "added" frames that were already approved, and has to review them again.

PROBLEM SEVERITY: broken task — the dispatch has no path to success.

run 33102388128  workflow_dispatch  conclusion: failure
  failing step: "Verify and promote the baseline"
  Error: visual acceptance refused: capture was produced for
    e6507a95fb13cae83f94e2d4b58009eaf584f1c6   <- current main at the time
    not e1079ca567ce5e3a32ad0d145776385e455ce7fc   <- the resolved merge for #5162

VERDICT: clear

SOLUTION

The recapture has to tell the gate which commit it photographed. It was reading the workflow run's own commit, which on a recovery dispatch is whatever main is today, not the historical merge being recovered. The workflow now states the resolved merge explicitly, through the override the PR-time capture already uses. Promotion's identity check is untouched, so a capture that still names the wrong commit is still refused.

SOLUTION  (1 decision · 1 runtime line of 82 added)
  1. recapture identity comes from ASTRYX_VISUAL_SHA, not a shadowed GITHUB_SHA   [the fix]
     visual-acceptance-promote.yml:317 — traces to the stated problem

The other 81 added lines are two tests and one fail-closed mutation case. All four deletions are mechanical — the env key, a helper split (workflowStep extracted from workflowStepScript), and one fixture destructure widened by capture — so no assertion is removed or loosened anywhere, and deletions: 4 accounts for every one.

BURDEN: low — one env: key in one step. No runtime state, Effect, listener, timer, shared coupling or public surface. BURDEN MATCH: proportionate.

VERDICT: clear

ARCHITECTURE

OWNER: captureIdentity() (gate.mjs:50-55) owns capture identity; the workflow owns which commit
TIER 1: reuses the existing ASTRYX_VISUAL_SHA override — no new mechanism
TIER 2: none
SEAMS: the two lanes reaching this step — pull_request_target types:[closed], workflow_dispatch
BEHAVIOR UNIT: n/a — a workflow env binding, no component behaviour

The implementation lives with its owner: gate.mjs:51 already reads ASTRYX_VISUAL_SHA ?? GITHUB_SHA ?? null, the override predates this PR, and pr-comment.yml:302 already drives it — so the finding a first read invites, "this invents a private env contract", is dead on the grep.

Seams, proved two ways because Actions cannot be executed here:

SEAM                          RESULT
workflow_dispatch (recovery)  driven in production — run 33102388128 failed on exactly this
                              identity check; the fix binds the value it wanted
pull_request_target closed    both triggers present, and the recapture step occurs exactly
                              once, so the lanes share it   (asserted at test:347)
fork PR merge                 .visual-merged-source is a separate path with
                              allow-unsafe-pr-checkout; control scripts run from the trusted
                              current-main checkout at :192-195. Unchanged by this PR

Is there a next place the same failure can happen? No. There are exactly two gate.mjs capture invocations in the whole workflow directory, and after this PR both name their own identity — pr-comment.yml:302 and visual-acceptance-promote.yml:317. The class is closed.

Sweep that ran clean: pr-comment.yml sets three identity vars and this step sets one. Deliberate, not an omission — the skipped-evidence path checks all three (visual-acceptance.mjs:204-210, guarded by fail('skipped evidence run identity mismatch')), the promote path checks only context.sha (:798), and the recapture really was produced by the run whose id it reports.

VERDICT: clear

IMPACT

Nobody's pixels move and no app changes. Two people are reached, both ours: the maintainer running recovery, whose dispatch currently cannot finish and after this reaches the pixel comparison; and the next contributor whose PR touches those 124 keys, who is shown frames a human already accepted on #5162 and asked to review them again.

verified read-only against gh-pages
  acceptance record for #5162   124 keys · all kind: added · all with an AFTER hash
                                · all with no baseline preimage
  derived plan                  124 entries, key-for-key identical to the record
  today's baseline              974 shots — 0 of the 124 present

What landing this newly exposes: nothing. The failure was fail-closed, and the production log proves it: promotion refused the mismatched capture rather than writing it. No baseline was corrupted; the bug cost a red run, not bad pixels.

VERDICT: clear

API

no API change. No prop, export, type, flag or CLI argument is added or altered.

Ossification: none. ASTRYX_VISUAL_SHA is an internal CI env var with no consumer outside this repo, already read at gate.mjs:51 and already driven at pr-comment.yml:302. This PR adds a second consumer of an existing contract — a new instance of an existing idea, which passes on citation, not a new concept.

VERDICT: clear

THEMING

n/a — structural only.

grep -cE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle"
  .github/scripts/visual-gate/visual-promotion-workflow.test.mjs
  .github/workflows/visual-acceptance-promote.yml          → 0

No new theme targets.

VERDICT: clear

BREAKING

Is there a consumer who can be broken? These are repository CI files: no published package, no installed consumer.

  • API — no. Nothing exported changed.
  • Visual — no. Zero of the two changed paths match ^packages/|\.stories\.|\.tsx$|\.css, and the PR Analysis Report reports "No new or modified components detected" and "No component packages changed".
  • Theme — no, per the grep above.

Behaviour — the recovery lane goes from always-refused to able to proceed. The states this diff can reach, walked:

state walked
correct merge SHA capture stamps the merge; context.sha === mergeSha passes
wrong merge SHA still refused — driven, mutation M3
empty/absent override ?? does not swallow '', so context.sha is '' and fails the !== check; gate-context.test.mjs already covers "preserves invalid explicit identity for downstream rejection"
empty · loading · error · disabled · boundary not reachable — the diff adds no state, no default, no prop

The only bound here is the identity equality, and it is driven from both sides: equal (M0, 234 pass) and unequal (M3).

VERDICT: clear

PERFORMANCE & RESOURCES

Effects: zero added, zero changed, zero moved — no React code is in the diff. No listener, observer, timer, dependency or bundle delta; the PR Analysis Report reports no component packages changed. CI cost is unchanged: same steps, same two installs, same Storybook build, one env: key renamed.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable
WHY: both changed paths are non-rendering — visual-acceptance-promote.yml (one env key)
     and visual-promotion-workflow.test.mjs (a test). Zero component/story/style files in
     the diff; style and role/aria greps return 0 on both files; the PR Analysis Report
     reports no components and no component packages changed.

No frames, and the reason is the acceptable one: nothing rendered changes because nothing rendering is touched. The evidence carrying this review is executable instead, banked so it can be re-run — one control and three mutations, one worktree at eb5c6a7, restored clean after each.

repro: cd ~/astryx/worktrees/visual-recovery-merged-capture-sha
       npx pnpm@11.10.0 exec vitest run \
         .github/scripts/visual-gate[/visual-promotion-workflow.test.mjs]

M0  control, unmutated                       PASS  18 files / 234 tests (matches the PR body)
M1  gate.mjs manifest.context: replace
    `...captureIdentity(),` with
    `sha: process.env.GITHUB_SHA ?? null,`   FAIL  "expected 'manifest.context = {…' to
                                                   contain '...captureIdentity(),'"
M2  workflow :317 ASTRYX_VISUAL_SHA
    reverted to GITHUB_SHA                   FAIL  both new tests, on the
                                                   ASTRYX_VISUAL_SHA assertion
M3  visual-acceptance.mjs:798 guard
    replaced with `if (false)`               FAIL  "the wrong merged capture identity"
                                                   stops failing closed

M1 answers the earlier blocker. Scoping the source assertion to the manifest.context slice (test:323-327) means a deliberate raw-GITHUB_SHA there is now caught; under a whole-file assertion it would not have been, because captureIdentity() also appears at gate.mjs:206 and :245.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

Nothing rendered, no strings, no roles, no direction.

grep -cE "aria-|role=|useTranslator|t\('@astryx"  <both changed files>  → 0
  • §1 automated — the PR Analysis Report says "No accessibility violations detected". Auto-covered, and honestly it is not evidence either way here: pr-a11y scopes to changed components and none changed.
  • §1 manual — not reachable. No interactive element, focus path, announcement or keyboard surface is in the diff.
  • §9 strings and direction — no user-visible or AT-visible string added; .github/a11y-baseline.json untouched, so nothing buys silence.

Checked by me: the greps above and the changed-file list.

VERDICT: clear

JUDGEMENT

PROBLEM        clear          API            clear          VISUAL         clear
SOLUTION       clear          THEMING        clear          A11Y & I18N    clear
ARCHITECTURE   clear          BREAKING       clear
IMPACT         clear          PERFORMANCE    clear

Read against the bodies, not the labels: no slot's prose contains a sentence about a person who is stuck, cannot reach something, hears nothing, or loses their place. The one harm sentence anywhere — a contributor re-reviewing 124 already-approved frames — is harm this PR removes.

GOAL: met, for the binding, which is what the PR claims. The production failure named the exact mismatch; captureIdentity() resolves ASTRYX_VISUAL_SHA first, so context.sha will equal merge_sha and pass the guard at visual-acceptance.mjs:798; M2 shows reverting the line reintroduces the condition the tests catch. What it does not claim, correctly: the pixel comparison after the identity check is unchanged and still has to pass on the next dispatch.

DISPOSITION: 1. the identity test never captures → note, not blocking, no follow-up — the assertions beside it cover the behaviour (M1 and M2 both fail if either half breaks).

ADVICE: proven existing pattern — gate-context.test.mjs:77 is the landed home for runtime identity precedence. Cited, not invented.

AUTHOR CAN PROCEED: yes — nothing blocked, nothing withheld.

WORST OUTCOME: "the next contributor whose PR touches those keys is shown 124 'added' frames that were already approved" → that is main today and the state this PR ends. Compatible with approve.

JUDGEMENT NEEDED: none — defect fix against the existing contract. No capability, no new concept, no new surface.

Verdict: approve.

1. [not blocking] the test named `captures the resolved historical merge` runs
   `gate.mjs check --max-shots 0`, which takes the skipped-verdict branch and reads
   verdict.json — no capture manifest is ever written
   → whoever opens this test to ask "is the recapture manifest bound to the merge SHA?"
     reads an executed proof and gets a string match, and gate-context.test.mjs:77
     already proves the same precedence the same way
                                        · visual-promotion-workflow.test.mjs:331

REVIEW

Good — reverting the line reproduces the failure the tests catch. Nit: the identity test runs check, so no capture manifest is written.

INLINE (0-3, only if genuinely line-anchored)

  • .github/scripts/visual-gate/visual-promotion-workflow.test.mjs:331 — Hmm, this runs check, not capture. gate-context.test.mjs:77 probably already covers the precedence.

EVIDENCE I DID NOT SPEND

  • #5608 overlap: none. No shared file; it does not touch captureIdentity() or the manifest.context block this test string-matches (its nearest hunk adds an unrelated legacyBaselinePackages const). Merge order is free either way.
  • CI on this head is 7 runs, not the ~19 a component PR gets, because path filters exclude everything but the .github lane; visual-acceptance sits pending and the Visual acceptance run was cancelled. Expected for a CI-only diff, and the suite that matters was run locally (M0).
  • The failed run's log line is the whole diagnosis; it is quoted in PROBLEM and does not need re-arguing to her.

TIME

TIME  total 14m
  setup        1m   worktree already at the exact head, node_modules present (warm main reused: yes)
  reading      4m   brief + critic (~6,900 lines of kit), PR body, diff, workflow, gate.mjs
  measuring    5m   1 control + 3 mutation runs, 124-record verification, failed-run log, capture-site grep
  writing      4m   draft, two critic passes, rewrite
  waste        1m   two greps mangled by shell quoting and re-run; one probe of the acceptance
                    record written against a guessed JSON shape before reading the real one

WHAT I COULD NOT VERIFY

  • Whether the pull_request_target types:[closed] lane was also mis-binding before this PR. GitHub documents GITHUB_SHA there as the base branch's last commit, which for a just-merged PR is usually the merge commit — so the close lane was probably right by coincidence and drifted only when main advanced first. Read from docs, not driven. The fix removes the coincidence either way.
  • Whether a step-level env: GITHUB_SHA: is ignored by the runner as a general rule, or was ignored only here. The production log proves it did not take effect on run 33102388128, which is all this PR needs; the general rule is unverified.

Clone this wiki locally