-
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 · exact head 8aa506b243b99813ec995536051dd1b534f0a7ba
Verdict: HOLD — request changes
The simplified design fixes the original false-removal path: ordinary and PR comparisons cannot infer removals, PR acceptance and promotion are add/update-only, and only a canonical stable-release capture may report a removal.
The contraction is substantial and correct in direction:
- before: 23 files, +3164/−206
- now: 10 files, +653/−221
- removed: two ownership snapshots, the story-owner registry, copied built-index fixture, standalone metadata test, and caller-controlled removal authority
| Domain fact | Intended source | Finding |
|---|---|---|
| package release eligibility | workspace package.json
|
one source |
| stories that exist | built Storybook index | one source |
| complete release plan | internal stable-release planner | one source; caller scope refused |
| accepted baseline membership | baseline | no owner — membership is re-derived each run and cannot reconcile with the store |
Complexity budget: one removal-reporting authority, one source for release membership, one readable verdict, zero new registries.
Actual burden: 10 files, about 430 runtime lines, one lane, one derived plan record, zero new registries.
Burden trend: sharply down from the prior 23-file design.
Reset trigger: not triggered; this is one model-level finding and its correction should continue the contraction rather than add another representation.
The durable baseline records what a maintainer accepted, but the new release lane re-derives whether each accepted key belongs every run. The store cannot record or reconcile that membership.
Driven against the built index and live baseline:
- deleting a real baseline story makes classification throw instead of producing a removal verdict;
- 94 accepted keys fall outside the derived lane, including two Core-titled Layer stories whose file imports a Lab component;
- a capture failure also throws before a verdict/report is written.
The consequence is the inverse of the intended contract: the only lane allowed to report deletions can end without publishing today's verdict, leaving the release cut with yesterday's answer.
Acceptance criteria:
- Deleting any currently accepted story produces a readable verdict, not a refusal.
- Every release-lane outcome, including capture failure, publishes a verdict the cut can identify as current.
- No accepted key is permanently both uncapturable and unprunable.
- A change to baseline membership is reported as a decision, never applied silently.
Contraction direction: let the baseline own its membership, and turn disagreement between the stored baseline and derived metadata into a readable verdict rather than another authority map.
- Exact-head CI, Lint, and CLI Smoke Test passed.
- Visual-gate suite: 239 tests passed across 18 files.
- Production resolver handled the built index; the canonical plan remained 884 shots.
- PR comparison, acceptance, post-merge promotion, and explicit enumerated prune tests passed.
- No component, visual, theme, accessibility, or internationalization surface changed.
Semantic verdict: request changes
Thanks — this is the right direction, and the contraction is real: the everyday lanes can no longer conclude a deletion, which was the whole bug.
What I'd like to change is where “which shots the release owns” lives. Today it is re-derived from the built Storybook index on every run and the baseline has no way to record or reconcile that answer, so the baseline is a truth nothing can converge. Two things follow: deleting any story makes the run refuse instead of report — the one deletion this authority exists to announce is the one it cannot — and 94 accepted keys now fall outside the lane, neither capturable nor removable.
Ideally I'd like the baseline to own its own membership, so a disagreement is a verdict the cut can read rather than a run that ends without one.
Where should that record live?
[Reviewed by Robohands]
#5608 — fix(ci): keep accepted stable shots in the release gate by @cixzhang (bucket: the maintainer)
a89027dfc94cbf9c8bedcc2f94544aa0277f63fb
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14
LANE: full WHY: trusted release authority, baseline mutation, workflow permissions, and an unresolved prior architecture block exclude the fast lane.
Our prior round at 8aa506b243b99813ec995536051dd1b534f0a7ba asked for the baseline to own its membership and for every release-lane failure to publish a current readable verdict. The only new commit is a merge of current main (936256386b74d67f88a41adee235b91699977b2f); it changes none of the ten PR-owned files, so this round extends rather than contradicts that request.
The merge tree is byte-identical to Git's automatic merge. Current main replaces the old cancellable initializer with the trusted PR Comment workflow_run publisher. At this head there is no cancelled initializer/check, and the final publisher resolves PR #5608, source CI run 33147772785, and head a89027d before writing visual-acceptance: success — No stable visual scope. Transient requested/in-progress publisher runs may be cancelled by the shared head-branch lock; the completed run is stale-guarded and succeeded.
WHY 1: a partial daily plan previously made accepted baseline shots look deleted.
WHY 2: release maintainers could not distinguish a real deletion from a plan omission.
WHY 3: a release gate without a current, readable answer trains the cut to consume stale output.
USER-FACING PROBLEM: when an accepted story disappears, package eligibility changes, or a capture fails, the maintainer cutting the release needs today's explicit decision; this head can instead drop keys from consideration or exit before publishing one.
PROBLEM SEVERITY: broken task — the release cut consumes the published latest verdict, and the only removal-reporting lane can fail before replacing it.
NEW FEATURE CASE: not a new feature.
EARLY STOP: clear — the failure and intended outcome are established by the PR body, prior review, source, and exact-head live-data probe.
VERDICT: clear
Ordinary PR comparison and promotion become add/update-only. A separate internal release command creates the only plan allowed to report removals, annotates shots with package/theme eligibility, and requires exact plan/capture identity before pruning. This fixes false removals, but it still derives baseline membership from today's index and package metadata instead of giving the durable baseline one convergent owner.
SOLUTION (3 decisions · about 430 runtime lines of 653 additions)
- Ordinary and trusted PR lanes never infer or apply removals.
- The daily release lane internally constructs and validates the sole removal-authoritative plan.
- Current Storybook/package metadata reclassifies the stored baseline before that plan and comparison. [NO STATED MIGRATION/RECONCILIATION FOR THE STORED MEMBERSHIP]
BURDEN: high — ten files, a new release lane, persisted plan proof, package/theme classification, acceptance restrictions, explicit pruning, and four focused test surfaces. BURDEN MATCH: proportionate in size to a release-authority fix, but incomplete until the stored membership has one source and every outcome produces a verdict.
VERDICT: BLOCKS — durable baseline membership is still re-derived rather than owned and reconciled
OWNER: the accepted baseline should own accepted shot membership; the current Storybook index and package manifests can propose/classify changes to it.
TIER 1: existing visual baseline, Storybook index, workspace package manifests, and trusted default-branch workflows.
TIER 2: none.
SEAMS: ordinary PR comparison, trusted PR capture, post-merge promotion, daily release capture, explicit prune, release-report publication.
TRUST / PERMISSIONS: release capture and a11y jobs have contents: read; only the publisher has contents: write. The trusted PR Comment publisher runs default-branch code, resolves the current PR/head and source CI run through GitHub, reads Actions artifacts, and writes statuses/comments/evidence; it never checks out or executes PR code with credentials. Release publication is non-cancelling and retries pushes; transient PR Comment invalidations may cancel each other, while the completed publisher is stale-head guarded.
BEHAVIOR UNIT: pure planner/comparator functions plus one release workflow; focused tests can drive all state transitions without rendering UI.
COMPLEXITY BUDGET: one removal-reporting authority, one authoritative source for accepted membership, one readable current verdict for every outcome, zero parallel owner registries.
ACTUAL BURDEN: 10 files; about 430 runtime lines; one new release command/lane; one derived plan record; package/theme eligibility projections; four focused test files with 77 exact-head tests.
BURDEN TREND: first reviewed head f98c6dc → current grew materially; the abandoned 23-file design → current shrank to 10 files; prior blocked head 8aa506b → current is flat because the main merge changed zero PR-owned files.
RESET TRIGGER: not triggered — this is the same unresolved model-level finding, and no attempted fix added another representation since the prior round.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| package release eligibility | workspace package.json
|
shot metadata | none |
| stories that exist now | built Storybook index | release-plan story list | none |
| complete release plan | internal release planner | sorted keys + digest in capture/verdict | none |
| accepted baseline membership | none | today's index/package classification silently filters the stored manifest | baseline files cannot converge with the classification |
| seam | driven result |
|---|---|
| ordinary/PR comparison | no removals, as intended |
| PR acceptance/promotion | add/update-only; removed evidence refused |
| exact current baseline + current index | 974 stored keys → 880 release-eligible; 94 stored keys are outside the only removal-reporting lane |
| delete one accepted story from the index |
stableBaseline() throws Legacy baseline shot … cannot be classified before a verdict/report |
| capture failure |
compareReleaseCaptures() throws before buildVerdict(); the focused test explicitly expects rejection |
| current-main integration | auto-merge tree exact; no PR-owned file changed; prior blocker persists |
The representative source is plan.mjs lines 136–175: missing legacy membership throws, unstable membership is silently dropped, and baseline coverage skips any story absent from today's index. gate.mjs lines 297–428 builds the report only after those operations and the failure-refusing comparison.
VERDICT: BLOCKS — baseline membership has no authoritative owner, so the release answer can be absent or silently incomplete
The release maintainer is affected in the deleted-story, reclassified-package/theme, and capture-failure states. They can see yesterday's published release answer instead of a current explicit deletion, reclassification, or failure decision. Existing ordinary PR flows improve: they no longer infer removals. NEW FEATURE IMPACT: not a new feature.
VERDICT: BLOCKS — the only removal-reporting lane can fail to publish today's answer
No published package API change. The changed surface is internal CI CLI/workflow behavior: gate.mjs release, release-plan metadata, package/theme eligibility metadata, add/update-only PR acceptance, and enumerated accept --prune.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
gate.mjs release with internally fixed tiers/scope |
internal CI CLI | sibling of check with stronger authority |
workflow comments/body | clear |
+ |
release-plan {version,lane,authority,keys,digest}
|
internal immutable capture proof | existing manifest context | source/tests | clear |
~ |
PR acceptance/promotion rejects removals | internal CI behavior | add/update lane | source/tests | clear |
+ |
explicit enumerated accept --prune
|
internal operator behavior | existing accept command |
source/tests | note — unreachable for the 94 silently filtered keys |
OSSIFICATION: no consumer package surface. The internal authority contract is still expensive to change because release history and baseline state depend on it; the one-source block is therefore architecture, not public API.
VERDICT: note — internal authority surface is coherent except for unreachable filtered membership
No CSS, tokens, targets, style contracts, or rendered theme behavior change. Theme package metadata is read only to classify release eligibility.
VERDICT: clear
BEHAVIOR: yes, intentional inside CI — ordinary/PR lanes stop reporting removals and the release lane becomes sole authority. API: no published API. VISUAL: no — no rendering path, component, story, or style changed. THEME: no rendered theme contract; package eligibility metadata is read.
VERDICT: note — intentional internal behavior change, with the unresolved failure state above
EFFECTS: zero.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
| none | none | n/a | n/a | n/a | n/a |
RENDER: no reachable UI render change. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or lockfile delta. The daily release capture grows to the canonical plan (the body reports 884 shots), which is the intended cost and remains under the workflow's 45-minute cap; this round did not dispatch a release run.
VERDICT: note — larger scheduled capture is intentional; no new runtime dependency or process leak
VISUAL CHECK: not applicable
WHY: all ten changed files are CI scripts/tests or release-gate.yml; there is no component, story, style, or rendered output change. The exact-head trusted scope publisher independently resolved No stable visual scope. No Storybook server or browser was started.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
No rendered element, accessible state, focus path, translation, locale formatting, or directional layout changes. Exact-head pr-a11y and pr-rtl are path-skipped, not treated as evidence.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — stored membership is not owned/reconciled |
| ARCHITECTURE | BLOCKS — one durable fact has no authoritative source |
| IMPACT | BLOCKS — current release answer can be absent/stale |
| API | note — internal surface only |
| THEMING | clear |
| BREAKING | note — intended CI behavior change |
| PERFORMANCE | note — bounded scheduled-capture cost |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: partly met — false removals are gone from ordinary lanes, but the release authority still cannot always publish a current readable deletion/failure decision. DISPOSITION: one architecture finding blocks now. Acceptance criteria: (1) every accepted key converges through one baseline-owned membership source; (2) deletion, eligibility reclassification, and capture failure each publish a current readable verdict; (3) no stored key is silently both uncapturable and unprunable. ADVICE: bounded outcome criteria — the one-source analysis proves the contraction direction; no speculative implementation is prescribed. AUTHOR CAN PROCEED: yes — the three outcome criteria above are complete and do not require another owner decision. WORST OUTCOME: “The release cut consumes yesterday's answer because today's sole removal-authority exited before publishing, while stored keys silently fell outside its lane.” → request changes.
JUDGEMENT NEEDED: none.
request changes
- Accepted baseline membership has no authoritative owner.
→ the release maintainer, after a story deletion/reclassification/capture failure, can receive no current verdict and cannot act on the stored key ·
plan.mjs:136→ confirmed independently by the 974→880 live-baseline probe and by the exact-head focused test/source path that throws beforebuildVerdict().
Semantic verdict: request changes
We asked for the baseline to own its membership at 8aa506b; this merge changes none of the ten PR-owned files, and the blocker remains.
At this exact head, the live 974-key baseline is reclassified to 880 keys: 94 accepted keys are silently outside the only removal-reporting lane. Removing one accepted story also makes stableBaseline() throw, and a capture failure takes the same pre-verdict exit. The release maintainer can therefore reach the cut with yesterday's published answer instead of a current readable deletion/failure decision.
Please converge every accepted key through one baseline-owned membership source, and publish a current verdict for deletion, reclassification, and capture-failure outcomes rather than dropping keys or exiting before the report.
[Reviewed by Robohands]
None — the defect is cross-cutting and the summary carries one representative source anchor.
- All six required contexts are exact-head success:
build,lint,test,docsite-test,review-required, andvisual-acceptance; Vercel is also success. - Focused exact-head Vitest: 4 files, 77/77 tests passed. Node syntax checks passed. Local actionlint has exactly the same five configured-runner/shell warnings on base and head; remote exact-head lint passed.
- No dependency, lifecycle-script, workflow-secret, private-residue, or public-package delta was found.
TIME total 13m setup 3m fresh exact-head worktree, rule/wiki versions, untrusted-code guard reading 4m prior rounds/comments, changed runtime/tests/workflows, current-main integration measuring 3m focused tests, live baseline/index probe, required-CI and trusted-run identity writing 3m draft, critic checks, review/wiki delivery waste 1m selected one unrelated visual-acceptance issue-comment run before tracing the correct PR Comment publisher
- The scheduled Release Gate itself was not dispatched; the request explicitly forbids release/recovery dispatch, and the blocker is established before browser capture.
Posted as drafted in the exact-head GitHub COMMENT review after re-reading the unchanged head and all six required contexts.