Review 5600

Review — facebook/astryx #5600 @ 948e2ec

PR

#5600 ci: add trusted visual acceptance recovery dispatch — by cixzhang (bucket: the maintainer)

HEAD REVIEWED

948e2ec64c9a857b86b9ae06088745964f3b1d99 <- every claim below was verified at this commit

Worktree pinned at that sha, git status clean. Rebase claim verified independently: all ten changed files have byte-identical blob hashes at 9170c53 and 948e2ec. origin/main is f24d0e9, which is HEAD~1 — main has not moved under any changed path (R31c clean, no conflict).

LANE: full WHY: 2362 added lines, a privileged trust boundary, a new cross-workflow module, and three prior reviews to reconcile. Not eligible on any fast condition.

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

PRIOR REVIEW (R1e / R31b)

Three reviews on this PR, all ours, all COMMENTED — nothing stands as CHANGES_REQUESTED, so nothing needs clearing.

c16e6d0 — "the durable queue and the status projection both look right. One non-blocking nit: on a plain merge the failure status now reads 'Recovery refused', for a recovery nobody ran."
f0fc64a — "latest-completed vs newer-active holds… One non-blocking nit: the automatic lane still inherits recovery-shaped failure wording."
b98a6f8 — "operational failure now outranks deferral… CLEAR, with the existing non-blocking wording nit."

I am extending, not contradicting. The wording nit is still live and I have now proved it rather than read it (S9 below). The unreviewed delta since b98a6f8 is +74/−20 across four files and it closes a real hole — see GOAL.

PROBLEM

WHY 1: A post-merge visual acceptance can fail, and the only retry available re-runs the historical workflow version that already failed — the same stale code, so it fails the same way.

WHY 2: The reviewed acceptance is immutable, so the person who did that review cannot get their approved pixels into the baseline at all. They hand-edit the baseline branch or the change stays unpromoted.

WHY 3: An unpromoted acceptance means every later release gate compares against a baseline that never took the approved pixels, so it reports a visual diff on a change a human already said yes to — and the gate's signal stops meaning anything.

USER-FACING PROBLEM: A maintainer who reviewed and accepted a visual change watches it merge, watches promotion fail, and has no safe way to finish the job; every release gate after that shows them a diff they already approved.

PROBLEM SEVERITY: broken task — the accept→promote path has no recovery, and separately the old shared Actions concurrency group could cancel an older pending accepted publisher outright, losing a promotion with no trace.

The second half is worth naming because it is invisible: the old concurrency: group: visual-acceptance-head-…, cancel-in-progress: false (visual-acceptance-promote.yml:21 on f24d0e9) keeps exactly one pending run, so a third arrival evicts the second — and a cancelled promotion leaves no status behind to notice.

VERDICT: clear

SOLUTION

Explain it like I'm five

Promotion no longer trusts anything the person asking for it typed: it asks GitHub who the PR is, proves the merge commit is already part of the main line, and only then does anything privileged. Because two different things can want to write the baseline at once, everyone now takes a numbered ticket written to disk instead of relying on the CI platform to queue them — the platform's queue throws people out, a written list does not. If a fresher CI run is still going, promotion stops before touching anything and says "waiting", so the check goes yellow rather than red or green. It only says "done" once the pixels are published, the next gate has been kicked off, and the ticket has been handed back.

Decisions

SOLUTION  (6 decisions · 1278 runtime lines added, 1084 test)
  1. main-only workflow_dispatch; the only human input is a PR number     [the fix]
  2. server-resolved identity + merge-reachable-from-main trust boundary
     before any privileged step                                          [the fix]
  3. recovery runs current-main control code against a separately
     checked-out merged tree (.visual-merged-source)                     [the fix]
  4. durable FIFO queue on gh-pages replaces the Actions concurrency
     group for all three publishers
  5. latest completed CI attempt is the evidence identity; a newer ACTIVE
     attempt defers to pending; a cancelled retry is not evidence
  6. success only after publication + fresh gate dispatch + lock release;
     every other outcome projects a bounded linked failure; a plain merge
     with no acceptance stays status-silent

Six is over the R29 threshold and I am saying the number. But I checked each one against the body and all six trace to a problem stated there — the "Why" and "Risk" sections name the recovery gap, the concurrency-cancellation gap, the retry-is-not-evidence rule and the fail-closed rule explicitly. Nothing piggybacks, and the 54/46 runtime-to-test ratio means the bulk is not hiding the decisions. Decision 4 could genuinely ship alone — it fixes a pre-existing cancellation bug and is what visual-baseline.yml needs — and I am not asking for that split: three prior reviews passed on this shape, this head is a byte-identical rebase of reviewed content, and re-opening it now buys a round trip and no safety.

BURDEN: high — a new module, durable cross-workflow state on gh-pages, a 90-minute privileged job, three publishers that must all adopt the lock. BURDEN MATCH: proportionate — the problem class is "a reviewed decision is lost or unrecoverable", and a written queue is the smallest thing that survives a runner dying mid-publish.

VERDICT: clear

ARCHITECTURE

OWNER: the visual-gate promotion pipeline (.github/scripts/visual-gate/)
TIER 1: none — no component-system participation; not React code
TIER 2: none
SEAMS: three publishers (pull_request_target close · workflow_dispatch recovery ·
       visual-baseline.yml manual), plus release-gate.yml's gh-pages prune
BEHAVIOR UNIT: two named pure modules — promotion-identity.mjs (identity,
       CI evidence, status projection) and baseline-publication-lock.mjs
       (FIFO queue + atomic holder) — each with focused tests

Does the implementation live with its owner and survive every seam? Yes, and I drove them rather than reading them.

SEAM                                   DRIVEN RESULT
automatic close, acceptance present    works — S2 publishes, status success
automatic close, no acceptance         works — S8 writes no status at all
manual recovery dispatch               works — same promote job, recovery='true'
manual visual-baseline.yml             works — exactly one enqueue/wait/release
release-gate prune                     works — publication-queue excluded, and
                                       `sort -V` puts `acceptances/` last so it
                                       always survives `head -n -20`

The prune deserves a line because it looked like a defect and is not: the exclusion list gains publication-queue but not acceptances, and the acceptance records are exactly what recovery depends on. Reproduced the sort locally — sort -V orders numeric run directories before acceptances/, so acceptances/ always lands inside the kept tail. Pre-existing shape, correct today.

The queue is a state machine and it is extracted into its own module with real-git harnesses (simultaneous claim, late claim, stale-404 self-heal, corrupt-holder refusal, stray-file cleanup). That is the right boundary, and the lifecycle owner and the implementation unit are the same module.

VERDICT: clear

IMPACT

Nobody using Astryx sees anything. Zero files under packages/, no published artifact changes, no runtime code ships. The audience is maintainers of this repo, and there are three things they will feel:

The person who accepted a visual change and watched promotion fail now has a button. Before, their only option ran the same broken code again.
The person who merges third in a burst no longer has their promotion cancelled by the second one arriving — the failure nobody could see before, because a cancelled promotion leaves nothing behind.
The person reading a merged PR's visual-acceptance check sees yellow where main showed red, when a CI retry was still running at merge time.

What landing this newly exposes: nothing re-drives promotion when the deferred retry completes. Verified, not inferred — no file references visual-acceptance-promote, and none of the four workflow_run hooks reaches it (cleanup-previews, deploy-preview, pr-comment, review-clear). So a deferred promotion waits on a person noticing a pending check on an already-merged PR and dispatching by hand. The stale baseline is not new — main also left it unpromoted here, and with no recovery path at all — but main screamed and this whispers, and merged PRs are not where anyone looks.

VERDICT: note — deferral has no automatic exit; the pending status is the only prompt · visual-acceptance-promote.yml:9-17

API

no API change — nothing a consumer can import moves.

Two internal surfaces are new and worth enumerating because they are now permanent for this repo's operators:

	change	public?	class	doc'd?	verdict
`+`	`visual-acceptance-promote.yml` input `pr_number` (required)	no — repo workflow, main-only dispatch	`visual-baseline.yml` already takes an operator dispatch	header comment	ok
`+`	on-disk queue format `gh-pages:visual-gate/publication-queue/{<runId>.json, holder.json}` = `{version:1, repository, runId}`	no — internal to `gh-pages`	`visual-gate/acceptances/**/current.json` uses the same `version: 1` shape	module docblock	ok

Ossification: both are versioned and both are internal to this repo's CI. Being wrong costs a workflow edit and a one-time queue cleanup, not a deprecation cycle. Neither reaches a builder.

VERDICT: clear

THEMING

n/a — structural/CI only.

grep -nE '#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle'
  <the 6 changed non-test files>   → 1 hit

The single hit is #5123 inside an untouched comment at visual-baseline.yml:19. No new theme targets, no tokens, no styles.

VERDICT: clear

BREAKING

Consumer who can break: none. git diff --name-only f24d0e9 948e2ec | grep -c '^packages/' → 0; no package.json and no lockfile change.

API — no. No exported surface moves.
Visual — no. Nothing renders (THEMING's grep, VISUAL EVIDENCE's paths).
Theme — no. No target, token or override is touched.
Behaviour — yes, deliberately. Walked, not assumed:

state	driven	result
plain merge, acceptance present	S2	publishes, `success`
plain merge, no acceptance	S8	promote skipped, no status written — matches main's silence
resolve-time deferral	S1	promote RUNS, defer step succeeds, 15 of 18 steps skip, `pending`
deferral found at load-and-verify	S3	ticket released, `pending`
deferral found at publish time	S4	holder released, `pending`
gate dispatch fails after publish	S5	`failure`, description names it
lock release fails after publish	S6	`failure`, description names it
acceptance superseded mid-flight	S7	`failure`, no publication
pre-trust-boundary bad input	—	`head_sha` never set, `project-status` never runs, no status touched
post-trust-boundary refusal	S9	`failure` with the resolver's own description

Bound tested past its edge rather than through the middle: resolveCIState driven with a newer completed attempt (supersedes → refuse), a newer cancelled attempt (neither supersedes nor defers), and a newer active attempt (defers).

The one behaviour change a reader should not miss is the signal, and it is the same finding IMPACT carries: an active CI retry used to produce a hard failure status (Mark a post-merge verification failure, old workflow) and now produces pending. Deliberate and stated — pending is the honest state — but quieter. visual-baseline.yml also loses concurrency: group: visual-baseline; its timeout-minutes: 20 → 90 is the load-bearing consequence, priced in PERFORMANCE.

VERDICT: clear — every axis walked; the one behaviour change is IMPACT's note, counted once

PERFORMANCE & RESOURCES

Effects: zero. This is not React code. grep -nE 'useEffect|useLayoutEffect|addEventListener|ResizeObserver' over the six changed non-test files → 0 hits. No mandatory Effect gate applies.

The real resource question is CI, and it is a genuine new cost:

cost	mechanism	who pays
runner minutes while queued	`waitForTurn` polls every 30 s; each tick is a shallow `--filter=blob:none --sparse` clone of `gh-pages` plus one `gh api` call per blocker · `baseline-publication-lock.mjs:324-391`	us
a second full `pnpm install` + build	the merged tree is installed and built separately from trusted main · `visual-acceptance-promote.yml:296-308`	us

Both are the capability's price, not the same outcome reached more expensively. The platform queue was cheaper and wrong — it cancels — so durable ordering cannot be had for free. The lock is deliberately acquired after capture and immediately before the push (Wait at :329, Verify and promote at :335), so the serialized window is the push, not the build; the 75-minute ceiling only binds behind a wedged holder. The second install is what buys the trust boundary: control code comes from main, only the Storybook bundle comes from the merged tree.

I did not measure real wall-clock wait time and have no production data, so there is no number here and I am not asserting one. Nothing is degraded that I measured; nothing is waved through as negligible.

VERDICT: clear — cost named, mechanism named, who pays named, wall time explicitly not measured

VISUAL EVIDENCE

VISUAL CHECK: not applicable
WHY: every changed path is .github/workflows/*.yml or
     .github/scripts/visual-gate/** — `git diff --name-only f24d0e9 948e2ec |
     grep -c '^packages/'` returns 0, no story, component, style or DOM file is
     touched, and the style/token grep above returns one hit inside a comment.
     Nothing renders, so there is no frame to take.

Decided from the changed-path list and two greps, not from the ci: prefix.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

n/a — structural/CI only.

grep -nE 'aria-|role=|useTranslator|@astryx\.' <the 6 changed non-test files> → 0 hits

No rendered output, no AT-facing string, no catalog key, no direction-sensitive code. a11y-baseline.json is untouched, so nothing is buying silence.

VERDICT: clear

JUDGEMENT

PROBLEM        clear
SOLUTION       clear
ARCHITECTURE   clear
IMPACT         note — deferral has no automatic exit
API            clear
THEMING        clear
BREAKING       clear
PERFORMANCE    clear
VISUAL         clear
A11Y & I18N    clear

Reading the bodies, not the labels: no slot's prose contains a sentence about someone stuck, unable to reach something, unable to exit, hearing nothing, or losing data. IMPACT's note and BREAKING's signal paragraph are one finding seen from two sides, counted once below.

GOAL: met — and the delta since the last reviewed head is what met it. At b98a6f8, recoveryOperationResult({jobResult:'skipped', mutationDeferred:true}) returned deferred, so promotionStatusProjection returned {state:'pending'}: a promote job skipped for any reason at all, with the resolve-time deferral flag set, projected pending. At 948e2ec the same input returns failure. Measured side by side with both module versions loaded in one process. The fix is structural rather than a guard — the job now always runs, and deferral must be confirmed by a step that actually executed on trusted main (visual-acceptance-promote.yml:202-213).

Every named invariant was then driven against the shipped YAML, not the JS helpers — nine scenarios, all matching (~/astryx/probe-kit/promote-workflow-run.mjs, ASTRYX_WT=<worktree> node promote-workflow-run.mjs). S1 is the one the invariant list cares about: the promote job runs, Confirm trusted active-retry deferral succeeds and emits the deferral, and 15 of 18 steps skip — every status write, the enqueue, the merged checkout, both installs, the build, the capture, the publish, the gate dispatch and the lock release. Final status composes needs.promote.result + recovery_complete + mutation_deferred + failure_description through the production recoveryOperationResult.

DISPOSITION:

Recovery refused: prefix reaches the automatic lane → accepted — the cost is a confusing sentence on a maintainer-facing status; Cindy made this call three times on this PR, most recently at b98a6f8 ("CLEAR, with the existing non-blocking wording nit"). Hers to accept, and she did.
Deferral has no automatic exit → blocks now: no. Not a defect: main had no exit either and no recovery path at all, so this is strictly better. It is a design question about whether a hand dispatch is the intended close, and it holds nothing. It is the review's closing question.

No orphans: every negative finding above appears here exactly once, and no finding is filed against a line whose deletion would not fix it.

ADVICE: proven existing pattern — the fix for finding 1 is already in the same map. 'active-ci-retry' at promotion-identity.mjs:19-20 is written lane-neutrally ("Visual promotion deferred: …"); the other fourteen entries carry Recovery refused:. Match the neutral one.

AUTHOR CAN PROCEED: yes — nothing blocks, nothing is withheld, and the one open question is a design preference that does not gate the merge.

WORST OUTCOME: "The person reading a merged PR's visual-acceptance check sees yellow where main showed red, when a CI retry was still running at merge time." → compatible with note. Nobody is stuck, the state is honest, the consequence is a slower human reaction — and the dispatch this PR adds is the thing that fixes it.

JUDGEMENT NEEDED: none for the code — no new package API, no design surface, no measured degradation I am accepting. R26 still stands: this is a subsystem — a concept the repo did not have (durable cross-workflow publication ordering), its own module, and three publishers must adopt it for it to pay off. The loop does not merge a subsystem unattended, and the PR body says the same. Terminal state: reviewed, recommended, waiting on visual-governance ownership. I would land it.

CLEAR

1. [not blocking] every refusal but `active-ci-retry` says "Recovery refused"
   → a maintainer reading a plain merge's failed status is told a recovery was
     refused, for a recovery nobody ran        · promotion-identity.mjs:11-37
     (carried from three prior reviews; accepted by Cindy each time)

REVIEW

Supersedes my earlier reviews. Reviewed exact head 948e2ec64c9a857b86b9ae06088745964f3b1d99 — byte-identical rebase over current main.

Thanks — making a real trusted step confirm the deferral is the right fix; a skipped promote job can no longer read as pending. CLEAR, with the same non-blocking wording nit.

Nothing re-drives promotion once the deferred retry finishes, so the pending status waits on a hand dispatch. Is that the intended close?

Full record: https://github.com/cixzhang/astryx/wiki/Review-5600

[Reviewed by Robohands]

(65 words. Prior three on this PR: 48, 52, 60.)

INLINE (1)

.github/scripts/visual-gate/lib/promotion-identity.mjs:11 — Hmm, every code but active-ci-retry says "Recovery refused" — a plain merge never ran a recovery.

Anchored line at this head: const FAILURE_DESCRIPTIONS = {

EVIDENCE I DID NOT SPEND

recoveryComplete() (promotion-identity.mjs:58-68) is exported and drives the test matrix but is never called in production — the workflow reimplements it as a YAML if at :436-443. I chased this as a drift finding, deleted the publication_confirmed clause from that condition, and re-ran: all 58 tests still passed and the behaviour was unchanged, because Run a fresh release gate is itself gated on publication_confirmed, so the clause is redundant. The finding died on its own second confirmation. Reverted; worktree clean.
The body never explains visual-baseline.yml's timeout-minutes: 20 → 90, which is the visible tell that waiting moved from GitHub's free pre-job queue into a billed in-job poll loop. One sentence in the description would record it. Not a finding — no reachable state, no person, no code change.
Two checks that could have become false findings: allow-unsafe-pr-checkout is a first use in this repo but is a real declared input on upstream actions/checkout (action.yml:101); and actionlint on both changed workflows reports exactly one warning (label "2-core-ubuntu-arm" is unknown) which 21 other workflows on f24d0e9 already trigger.

TIME

TIME  total 22m
  setup        1m   worktree already pinned at the head; no browser, no install
                    (warm main reused: n/a — nothing renders)
  reading      8m   kit (240 KB across two files), diff, both new modules, the
                    old workflow on main, three prior reviews
  measuring    7m   77 focused tests · 9-scenario shipped-YAML harness ·
                    old-vs-new two-module A/B · mutation A/B · actionlint ·
                    check:changesets / portable-scripts / executable-bits
  writing      6m   draft + two critic passes + rewrite
  waste        2m   `npx vitest@latest --reporter=basic` — not a valid reporter
                    in that version, and the repo's own binary was right there;
                    then one harness re-run after forgetting to model the
                    completion step's unconditional output write

Re-measures: 1 (the harness re-run above). Not counted as waste: the mutation A/B that killed my own second finding, and the sort -V check that killed a third — that is the rule working. Banked back to probe-kit/: promote-workflow-simulate.mjs (a small GitHub-expression evaluator + step-graph runner) and promote-workflow-run.mjs (the nine scenarios).

WHAT I COULD NOT VERIFY

Real queue wait time under load. The lock is acquired immediately before the push, so the serialized window should be seconds, but I have no production data and did not simulate a wedged holder against a real 90-minute job.
CI on this exact head. CI is in_progress at 948e2ec; Visual acceptance is cancelled; Lint, CLI Smoke Test and Internal Registry are green. Green-looking is not green here — the focused suites I ran locally are the evidence, not the PR page.

Review 5600

Review — facebook/astryx #5600 @ 948e2ec

PR

HEAD REVIEWED

VERSIONS

PRIOR REVIEW (R1e / R31b)

PROBLEM

SOLUTION

Explain it like I'm five

Decisions

ARCHITECTURE

IMPACT

API

THEMING

BREAKING

PERFORMANCE & RESOURCES

VISUAL EVIDENCE

REMEDY SEARCH

A11Y & I18N

JUDGEMENT

REVIEW

INLINE (1)

EVIDENCE I DID NOT SPEND

TIME

WHAT I COULD NOT VERIFY

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!