-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5608
#5608 — fix(ci): keep accepted stable shots in the release gate · by @cixzhang · bucket: the maintainer Verdict: approve · merge held for a human (release governance) · reviewed read-only, nothing posted
f98c6dc9499d07f3a35b7f539194fd7cda88c697 — every claim below was verified at this commit.
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: ~132 runtime lines (fast caps near 50); the change alters behaviour on four read paths and had to be driven against the live baseline and the landed #5600 publication path together.
pulls/5608/reviews and pulls/5608/comments are both empty. No prior human or loop review, no standing CHANGES_REQUESTED of ours — nothing to extend, narrow or contradict.
main is e6507a95fb1 (#5600), and that is exactly this PR's merge-base: one commit, cleanly rebased, MERGEABLE. Nothing moved under it.
WHY 1: The daily release gate reports baseline keys as removed that nothing removed. Today's 03:49 PT run (33064690860) planned 428 shots against a 686-key baseline and reported 260 removals, 0 changed. WHY 2: The person reading that summary before the 08:00 cut cannot tell a real deletion from an artefact of a narrower plan, so the removal column stops being information and becomes noise they learn to skip. WHY 3: The gate exists so a release can be held for a real visual regression. When a reviewed shot enters the baseline through a deeper PR plan and no daily plan can reach it again, that shot stops being watched — the baseline's promise ("this picture was accepted, and we keep checking it") quietly stops being kept.
USER-FACING PROBLEM: The maintainer cutting the release reads a 260-line removal list they cannot act on, while 868 reviewed Core shots go un-rephotographed by any daily run — 196 of them Carousel and 168 OverflowList, the two largest components in the baseline (measured against the live 974-key manifest, not taken from the body).
PROBLEM SEVERITY: harmful friction — the cut still completes and a real changed still surfaces on the 428 shots that are planned; what is lost is signal legibility and coverage breadth. Not broken task: the gate still runs, and no release has been shown to have shipped a regression through the gap.
VERDICT: clear
The daily gate decides what to photograph from a representative sample of the system, while a reviewed pull request can add deeper pictures to the shared record of accepted ones. Now the daily run also re-photographs everything already in that record, so a picture someone reviewed keeps being checked instead of dropping out because today's sample happened not to reach it. Both sides of the comparison are also limited to the packages that actually keep a stable record, so pictures from experimental packages stop being counted as things that vanished. If a story or a theme genuinely no longer exists, its picture is still not put back — it stays an explicit disappearance for a person to review and prune.
SOLUTION (2 decisions · ~132 runtime lines of 284)
- Scope BOTH sides of the comparison to the stable package set, on every read that compares — serves the body's "reported 94 legacy Lab/charts/vega keys even though those packages are explicitly outside the stable release surface"
- Union the still-reproducible stable baseline into the derived daily plan, behind
--cover-baseline— serves the body's "reviewed stable keys enter the baseline without any future daily plan being able to recapture them"
Mechanism for 1, not separate decisions: the trusted publisher reading config through loadConfig instead of a hand-rolled read, and trustedPlan's hardcoded ['Core'] becoming config.stableStoryPackages (visual-acceptance.mjs:719 — const indexed = storiesInPackages(). The second is worth crediting: it was a hand-maintained copy of a config value, and removing it is a drift fix.
R1j audit — for each decision, the body sentence saying why the old behaviour was wrong is quoted above. No unargued change found; no changed default; no widened union handed to a consumer.
Is this THE fix, or A fix? The root cause is that two planners write into one shared record with no shared notion of what that record promises: the daily gate derives a representative plan, a trusted PR can derive a deeper one, and the baseline both compare against belongs to neither. An unevadable fix has to live where a plan is derived from the record rather than beside it — which is where this puts it (lib/plan.mjs:169), reached through the one function every command already calls for its plan (gate.mjs:138 plan(), and check() calls it at gate.mjs:249). I then went looking for the next place the same failure can happen and there is not one: all seven readers of a baseline manifest are enumerated below, and each is either a comparison that is now scoped or a write that is deliberately not. This is the fix, not a patch.
BURDEN: low — no runtime state, zero Effects/listeners/timers/observers, no shared-system coupling, no published surface. Two pure functions and one CLI flag, with focused unit tests. BURDEN MATCH: proportionate.
VERDICT: clear
OWNER: lib/plan.mjs owns which shots exist; lib/baseline.mjs and visual-acceptance.mjs promote() own what the baseline contains. The change puts the scoping filter in the first and leaves the second alone.
TIER 1: reuses shotKey, toShotBase, MODES, readStoryIndex, storiesInPackages, loadConfig, readBaseline. No new mechanism, no parallel protocol.
TIER 2: none.
SEAMS: seven manifest readers — four that compare, three that write.
BEHAVIOR UNIT: pure utility. baselineInPackages (lib/plan.mjs:136) and withBaselineCoverage (lib/plan.mjs:169) are pure and unit-tested; nothing inline, no state machine, no Effects.
The placement question is the whole review, and it has one answer: the filter is applied on every read that compares, and on no read that writes. Enumerated at head:
| reader | anchored line at head | role | scoped? | correct? |
|---|---|---|---|---|
gate.mjs:294 |
const baselineManifest = baselineInPackages(unscopedBaselineManifest, storyPackages); |
compare | yes | yes |
gate.mjs:182 |
const {manifest} = readBaseline(baselineDir); (fed to the union) |
compare | yes | yes |
publish-pr-report.mjs:139 |
const baselineManifest = baselineInPackages( |
compare | yes | yes |
visual-acceptance.mjs:605 |
const baseline = baselineInPackages( |
compare | yes | yes |
lib/baseline.mjs:101 |
const {manifest} = readBaseline(baselineDir); |
write (accept) |
no | yes — must not prune |
visual-acceptance.mjs:370 |
const manifest = readJSON(manifestFile); |
write (accept snapshot) | no | yes — must not prune |
visual-acceptance.mjs:802 |
const manifest = readJSON(manifestFile); |
write (promote — the #5600 path) | no | yes — must not prune |
Driven seams, not read:
| seam | driven result |
|---|---|
| daily plan, live 974-key manifest + this head's Storybook index | works — 868/868 stable keys reproduced, 0 missing |
| daily plan, story no longer in the index | works — not re-added; stays a removal |
| daily plan, theme no longer shipped | works — not re-added; stays a removal |
--story-packages '*' opt-out |
works — returns the manifest identity (lib/plan.mjs:144) |
| trusted PR report with a canary baseline key | works — removed: [] (publish-pr-report.test.mjs:298, passes) |
| trusted plan, newly promoted stable theme, canary key present | works — (visual-acceptance.test.mjs:527, passes) |
| baseline promotion (#5600 recovery path) with a canary key present | works — the canary key survives two idempotent promotes byte-identical (sha256 unchanged); driven in a scratch arm, then reverted |
Shot-shape equivalence checked rather than assumed: withBaselineCoverage builds the shot at lib/plan.mjs:183 and inserts it at :194 as planned.set(key, {...shot, key, reasons: ['baseline']}), which is character-for-character what buildPlan's own add() produces at :265 — shots.set(key, {...shot, key, reasons: [reason]}). A baseline-derived shot and a planned shot are indistinguishable to the capture.
One gap, and it is the review's only finding: nothing pins the unscoped half. The three write readers are correct today by construction, and no shipped test asserts it. Confirmed two independent ways — the new Lab/Drawer fixture appears at publish-pr-report.test.mjs:299, visual-acceptance.test.mjs:535 and in lib/plan.test.mjs, but in no promote test; and routing promote through baselineInPackages leaves all 235 tests passing while dropping the out-of-scope key from the written manifest.
VERDICT: note — the scoped/unscoped split this PR creates is load-bearing and untested on the unscoped side
Nobody using Astryx sees anything. This reaches two people, and both of them are us.
- The maintainer reading the daily gate summary stops seeing a 260-line removal list that mostly means nothing and starts seeing one where every entry is a story or theme that actually went away. That is the difference between a column you skip and a column you act on.
- The same person, at the next cut, gets 868 reviewed Core shots back under daily watch. A regression in Carousel (196 baseline shots) or OverflowList (168) now surfaces at the cut instead of waiting for a PR that happens to touch it.
What landing this newly exposes: the 106 Lab/Charts/Vega keys in the baseline stop being reported at all, and stop being prunable through the ordinary accept path — an acceptance can only prune what the report calls removed. They are stale, they are outside the stable release surface by the config's own definition, and the README names --story-packages '*' as the audit that reaches them. Nothing renders them and nobody is hurt; the cost is that they sit there until someone runs that audit, and this PR is the moment that becomes true.
VERDICT: note — 106 stale canary keys become invisible to the normal path; documented, with a named audit route
No published package API change. 0 lines under packages/ or apps/; no changeset, correctly.
Internal CI surface, enumerated:
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
gate.mjs --cover-baseline (boolean, default off) |
internal — CI script flag, not a package export |
--story-packages, --tiers, --plan-file — the landed plan-shaping flags |
README + the usage line (gate.mjs:607) |
ok |
+ |
baselineInPackages(manifest, packages): manifest |
internal module export |
storiesInPackages(stories, packages) at lib/plan.mjs:122 — same name shape, same argument order, same job one level up |
JSDoc | ok |
+ |
withBaselineCoverage(plan, {stories, baselineManifest, themes}): Shot[] |
internal module export |
buildPlan(...) — same producer role, same Shot[] return |
JSDoc | ok |
OSSIFICATION: none of the three is published. All three are removable in one PR with no deprecation cycle and no codemod, because nothing outside this directory imports them. baselineInPackages is a new instance of an existing idea (storiesInPackages), not a new concept — it passes on citation. --cover-baseline is off by default and set in exactly one place (release-gate.yml:112), so the cost of being wrong is one line.
VERDICT: clear
n/a — no styles, no targets, no tokens, no astryx-* classes. grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle" over all 10 changed files → 0.
The diff does read theme names, so the drift question is live and the sweep is clean: the available-theme set is config.defaultTheme plus every directory under packages/themes except the probe theme, resolved through loadThemeOverrides (gate.mjs:186 — themes: [config.defaultTheme, ...Object.keys(themeOverrides)],). Derived, not a hand-maintained list. Checked live: all seven themes present in the baseline (butter, chocolate, gothic, matcha, neutral, stone, y2k) exist as theme packages, so no current key is dropped for an unrecognised theme.
VERDICT: clear
BEHAVIOR: yes, intentionally — that change is the fix. Three reachable paths move: the daily plan size and removal set, the trusted PR report's removal set, and the trusted plan's baseline-theme set. States walked rather than assumed:
-
empty / zero — no baseline on gh-pages:
readBaselinereturnsexists: falseand the union runs over{}, a no-op. The workflow already prints its "this run establishes one" notice. -
malformed — a manifest whose
shotsis missing, an array, or not an object is returned unchanged (lib/plan.mjs:142), soreadTrustedBaseline's existing validation still fires instead of being bypassed. -
opt-out —
--story-packages '*'returns the manifest identity (lib/plan.mjs:144); the audit path is preserved. -
boundary — story removed, theme retired, mode outside
['light','dark'](lib/plan.mjs:178-179), or a key that disagrees with its ownstoryId/theme/mode(lib/plan.mjs:188,if (shotKey(shot) !== key) continue;): all four refuse re-entry and stay removals. Two of the four driven, two closed by that guard. - disabled / controlled — not reachable; the diff adds no state and no default.
API: no — no export, type, prop or default changed. VISUAL: no — nothing rendered moves, resizes or recolors. The diff writes no CSS and adds, removes or moves no DOM. What changes is which pictures are taken, never what a picture looks like. THEME: no — no target, token or override touched; theme names are read only.
Consumer who can be broken: none outside this repo. These are CI scripts, not a published package.
Commit-type honesty: fix(ci) claims a CI-scoped fix with nothing consumer-visible. Checked and true — 0 lines under packages/ or apps/, 0 dependency lines, no changeset, and the only behaviour that moves is the gate's own.
VERDICT: note — a deliberate behaviour change on three internal paths, each walked above
EFFECTS: zero. grep -nE "useEffect|useLayoutEffect|addEventListener|ResizeObserver|MutationObserver" over all 10 changed files → 0. No React in this diff.
RENDER: n/a — no React. LISTENERS/OBSERVERS: none added. LAYOUT: none. BUNDLE: no dependency added; git diff -- '*package.json' 'pnpm-lock.yaml' → 0 lines.
The real cost is CI wall clock, measured on the pipeline rather than estimated. From today's daily run 33064690860, job "Visual regression":
| before (measured) | after (projected at the same rate) | |
|---|---|---|
| stories scouted | 385 → 3m34s | unchanged — scout is plan-size independent |
| shots planned | 428 | ~870 (868 covered stable keys + the run's own 2 additions) |
| capture + compare + report | 2m53s (~0.40 s/shot) | ~5m50s |
| "Run the visual gate" step | 6m27s | ~9m24s |
| whole job | 22m21s | ~25m20s |
| job cap | timeout-minutes: 45 |
~20m headroom remains |
Four consecutive daily runs put the job at 21m15s / 21m47s / 22m21s / 21m18s, so the 22m base is stable rather than a sample of one. The visual-capture artifact roughly doubles in file count at 14-day retention.
Who pays: we do, about three minutes of CI a day. This is not the same outcome reached more expensively — it is a larger outcome, bounded by the size of the baseline it protects, so it does not compound. What would remove it: nothing short of giving the coverage back, either by dropping --cover-baseline or by narrowing --tiers; there is no cheaper way to photograph the same set. The number is not in the PR body; it belongs in the Evidence list so the next person meets a choice rather than a surprise.
Reproduction: a banked one-off plan probe against this head's Storybook index and the published baseline manifest; timings from gh api repos/facebook/astryx/actions/runs/33064690860/jobs.
VERDICT: note — a measured ~3 min/day CI increase inside a 45-minute cap; it is what the capability costs, and the decision to buy it is the author's
VISUAL CHECK: not applicable
WHY: no rendering path is changed. All 10 changed files are CI scripts and one workflow — 9 under .github/scripts/visual-gate/, plus .github/workflows/release-gate.yml; git diff --name-only origin/main...HEAD -- packages/ apps/ → 0. The styling grep above returns 0 and the ARIA/role/translator grep returns 0. pr-a11y, pr-rtl and Stable visual regression are all skipped by path filter at this head, and the PR Analysis Report says "No new or modified components detected" with no bundle change.
No frames, and the reason is the acceptable one: not renderable — CI scripts and a workflow. BREAKING · Visual is no, so the empty slot and that answer are consistent rather than contradictory.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
n/a, with what was checked rather than an assertion. grep -nE "aria-|role=|useTranslator|t\('@astryx" over all 10 changed files → 0. The diff renders no element, exposes no state, adds no focus path and adds no user-visible or AT-visible string. .github/a11y-baseline.json is untouched and gains no entries; the a11y audit on this head reports no violations; pr-a11y is skipped by path filter, which is correct for a diff with no component in it.
Public-docs placement: the only new prose is 11 lines in .github/scripts/visual-gate/README.md and two workflow comments. That README is the gate's own operating doc, read by whoever runs the gate; it is not shipped caller documentation (packages/cli/assets/docs/, a .doc.mjs, or the docsite). Process material belongs exactly there, so the placement is right rather than a finding.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | note — the unscoped write half is untested |
| IMPACT | note — 106 stale canary keys become invisible to the normal path |
| API | clear |
| THEMING | clear |
| BREAKING | note — deliberate behaviour change on three internal paths |
| PERFORMANCE | note — ~3 min/day CI, measured, inside a 45-minute cap |
| VISUAL | clear |
| A11Y & I18N | 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 data. The strongest sentence anywhere is about a future maintainer and needs another change to reach them. The four notes do not compound — a test gap, a documented consequence, an intended behaviour change and a priced cost — and none is a precondition of another.
RISK CLASS: not low-risk, and stated rather than implied. It adds internal exports, it changes behaviour on purpose, and it carries a measured CI cost — three of R19's four conditions fail. That is why it does not post or merge unattended, independently of the PR's own instruction.
GOAL: met — measured on both sides.
-
Before, on
main, from the real pipeline: run 33064690860 loggedVisual gate: 428 shots (surface, theme-matrix)againstBaseline: 686 shotsand summarised| 428 | 0 | 2 | 260 | 0 |— 260 removals that nothing removed. - After, at this head against the live 974-key manifest and this head's Storybook index: 868 of 868 stable keys reproduced, 0 missing; 106 out-of-scope keys ignored rather than reported; a removed story and a retired theme both refused re-entry.
-
Both invariants together: the promote write is unscoped at
visual-acceptance.mjs:802, and a canary key present before promotion survives two idempotent promotes byte-identical. #5600's own suite — promotion identity, canonical-pixel equality, baseline-preimage conflict, and the ten publication-lock/queue tests — passes at this head. 18 files, 235 tests, all green locally in 37s, and CI's owntestjob went green on this exact head at 18:36:32Z.
DISPOSITION
- No shipped test pins the promotion write as unscoped → follow-up, non-blocking. The behaviour is already correct; the gap is a guard on an invariant this PR creates. Independently landable, ~14 lines in the existing promote harness, named with its fixture below. The intermediate state is acceptable because nothing today routes that read through the filter, and the two-way confirmation proves the current code correct rather than merely untested.
-
106 legacy canary keys become unreported and unprunable through the normal path → accepted, and the decision is the author's, recorded in her own PR body and in the README with
--story-packages '*'as the audit route. The keys are outside the stable release surface by the config's own definition. - BREAKING · behaviour = yes → not raised as a finding: it is the fix itself, every reachable state is walked in that slot, and the one consequence that reaches a person is item 2.
- ~3 min/day of CI → the price of the capability, not a degradation: bounded by baseline size, ~20 minutes of headroom under the job cap, and it buys exactly the coverage the PR exists for. The decision to spend it is the author's and shipping this PR is that decision; the only gap is that the number is not written down, and one line in the body's Evidence list closes it.
ADVICE: proven existing pattern — the fixture for 1 already exists twice in this diff (publish-pr-report.test.mjs:299, visual-acceptance.test.mjs:535), and the promote harness at visual-acceptance.test.mjs:764 already builds everything else it needs. Nothing invented.
AUTHOR CAN PROCEED: yes — nothing blocks. Acceptance criterion for the note, if she takes it: a promote run leaves an out-of-scope baseline key byte-identical in the written manifest.
WORST OUTCOME: "the next person who scopes that read for consistency deletes the 106 Lab/Charts/Vega keys from the published baseline, and every one of the 235 tests stays green" → note. That is a hazard for a future change, not a defect in this one: the current code is correct, driven, and nothing today reaches the bad state.
JUDGEMENT NEEDED: none — no package API, no theme target, no design call. The three new pieces are internal CI surface, and baselineInPackages matches its landed sibling storiesInPackages in name, shape and argument order.
EXIT STATE: approved, merge held for a human. The PR's own body says "Do not merge automatically; the release process remains human-gated", so it is not armed for auto-merge and is tracked as approved-and-human-gated on the release list. CI at this head: every required check is green — including test, which ran the .github/scripts suite and passed at 18:36:32Z — except the Vercel commit status, which is red (see What I could not verify); initialize shows cancelled from a superseded run. pr-a11y, pr-rtl and Stable visual regression are skipped by path filter, which is correct here and means green is not evidence for them either way.
POSTING NOTE: no [Full review] wiki link on this one. That link is for teammates in .github/ENGOWNERS/.github/DESIGNOWNERS whose review she has not seen; this is her own PR, and the brief excludes it. The record still gets this page. The attribution line is appended at post time.
Findings
-
[not blocking]No shipped test pins the promotion write as unscoped → the next person who scopes that read for consistency deletes the 106 Lab/Charts/Vega keys from the published baseline, and every one of the 235 tests stays green ·.github/scripts/visual-gate/visual-acceptance.test.mjs:764
Thanks, this holds up — I drove both invariants. Nit: nothing pins the promotion write as unscoped, so scoping it later drops the canary keys and the suite stays green.
[Reviewed by Robohands]
-
.github/scripts/visual-gate/visual-acceptance.test.mjs:764— Might want a canary key here — nothing catches it if promote ever gets scoped.
- The probe theme is excluded from the available-theme set, so a probe-tier shot could never be re-covered and would read as a removal. Zero probe keys exist in the live 974-key baseline, so nothing is affected today.
- A flaky Core story added to
excludeStorieswould have its baseline keys read as removals rather than ignored. Pre-existing onmain, unchanged here, and no Core story is currently excluded. -
publish-pr-report.mjsnow tolerates a missingvisual-gate.config.jsonby falling back to defaults where it previously threw.gate.mjs:63already behaved that way, so this makes the trusted publisher consistent with its sibling rather than loosening a fail-closed path — which is why it is not a finding.
TIME total 25m
setup 2m head worktree was already cut; pnpm install 10s (warm main reused: no)
reading 8m brief + critic in full, PR body, diff, gate.mjs/plan.mjs/visual-acceptance.mjs
measuring 9m 1 coverage probe against live data, 3 vitest runs (head, scratch arm, mutant arm),
1 repeat after a lost call, CI + job-log queries, anchor verification
writing 6m draft, critic pass, rewrite, second critic pass
waste 3m tried to reuse dependencies from another checkout; the test runner correctly aborted,
and a clean install took 10 seconds. One interrupted mutant run was repeated.
- Whether the red
Vercelstatus at this head is this PR's doing. It is red here and on #5606 and #5599, green on #5605 and #5597, and this diff touches 0 lines underapps/orpackages/. I did not open the Vercel build log, so I am not calling it unrelated. - The exact post-fix plan size on the real pipeline. 868 is the covered-stable count from the live manifest and this head's Storybook index; the union with the derived surface/theme-matrix plan needs a full
pnpm buildto reproduce exactly, which I did not run. The daily's own "2 added" makes ~870 the expectation, and the CI projection rides on it.
(not yet posted — read-only review, awaiting the maintainer's judgement)
After the read-only review settled, exact-head CI completed successfully at f98c6dc: CI run 33102925523 passed test, build, build-storybook, build-sandbox, docsite-test, and theme-layers.
A clean production-shaped plan on that head produced 788 Core shots against the captured 786-key stable baseline used by this investigation: 0 stable keys missing, 0 canary shots, with OverflowList 168 and Carousel 196. The review's independent live-baseline probe measured 868/868 against the newer published baseline; both measurements satisfy the invariant at their respective baseline snapshots.
The non-required Vercel preview is external residue, not a source-build failure: its GitHub deployment status was created and failed at the same timestamp (no build interval), the diff touches only .github/, and every exact-head GitHub application build above passed.