-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5696
#5696 fix(ci): preserve visual reports during preview deploy by cixzhang (bucket: the maintainer)
5e31be6cbd4f73e80cb3fd39dba31b7ed3d8b377
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.15.1
LANE: full WHY: one small runtime decision and no public/rendered surface, but this is a write-credential trust boundary fed by PR-controlled static artifacts, and the stated ownership guarantee needs deletion, copy, retry, and identity checks. PRIOR REVIEW: checked; no human or Copilot review exists at this head, only automated preview/report comments.
WHY 1: A successful visual report can become unavailable after a later preview deploy replaces the PR directory that contains it. WHY 2: A maintainer reviewing or accepting changed pixels then loses the exact evidence behind the status. WHY 3: Without durable exact-head evidence, visual review cannot be audited or safely relied on for release decisions.
USER-FACING PROBLEM: A maintainer revisiting a PR after a retry or redeploy can lose the trusted pixels they need to judge that PR. PROBLEM SEVERITY: broken task — the evidence link can stop resolving, so the review cannot be completed from its recorded proof. NEW FEATURE CASE: not a new feature. EARLY STOP: clear — the failure and affected maintainer are demonstrated.
VERDICT: clear
A preview deploy owns the PR's Storybook and Sandbox files, while the visual publisher owns the sibling evidence directory. The change leaves that directory in place while clearing the old preview, then copies the new preview files. That solves deletion but does not yet stop preview input from writing back into the reserved directory.
SOLUTION (1 decision · 6 runtime lines of 16 added lines)
- Preserve
pr/<number>/visual/while replacing the rest of that PR's preview tree — serves the stated evidence-loss problem.
BURDEN: low — one local helper, no state, Effects, listeners, schemas, dependencies, or workflow changes; one focused fixture assertion. BURDEN MATCH: proportionate — subtree ownership belongs in the shared publisher that performs the replacement.
VERDICT: BLOCKS — deletion respects the boundary, but the following copy can still cross it.
OWNER: the shared gh-pages publisher owns subtree mutation; the immutable visual publisher owns pr/<number>/visual/ bytes.
TIER 1: the shared publication queue and immutable-path publisher are reused.
TIER 2: none.
SEAMS: automatic preview deploy, manual redeploy, immutable evidence publish, push retry, cleanup, compaction, and stable-site publication.
BEHAVIOR UNIT: inline helper — the boundary is small and directly exercised through publishPrPreview against a real bare Git remote.
COMPLEXITY BUDGET: 1 runtime decision, 1 owner, 0 new durable representations, 0 new modules, 0 new workflows.
ACTUAL BURDEN: 2 changed files; 6 runtime lines; 0 new owners/modules/schemas/workflows; 1 added fixture assertion in a 25-test publisher suite.
BURDEN TREND: first reviewed head → current head: flat; this is the first review of the only head.
RESET TRIGGER: not triggered — one ownership defect, and contraction is derivable.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| same-PR visual evidence bytes | immutable evidence publisher at pr/<number>/visual/<head>/<run>/<attempt>
|
PR comment links and acceptance records identify those immutable bytes |
BLOCKS: copyContents(storybookDir, destination) can write the same reserved path from the PR-controlled preview artifact |
| seam | driven result |
|---|---|
| ordinary preview replacement | passes: stale preview files are removed and existing same-PR evidence remains |
| rejected push followed by fresh retry | passes: a concurrently published newer same-PR evidence file survives the retry |
| identical redeploy | passes: second publish is a no-op and evidence remains |
| other PR subtree | passes: another PR's evidence is unchanged |
preview artifact containing top-level visual/
|
fails: artifact bytes overwrite newer trusted evidence bytes |
| open/closed cleanup | passes in the focused suite: open PR evidence remains; a closed PR subtree is removed |
The deletion and copy phases must enforce the same ownership boundary. The exact-head implementation has one source for visual evidence in intent but two writers in behavior.
VERDICT: BLOCKS — the preview artifact remains a second writer for the immutable evidence namespace.
Normal current Storybook output contains no top-level visual/, so the common deploy now preserves evidence. But the artifact is produced from PR-controlled content; any top-level visual/ entry can replace the evidence a maintainer is trying to inspect. All other PRs remain isolated by the validated numeric pr/<number> destination.
NEW FEATURE IMPACT: not a new feature.
VERDICT: BLOCKS — the stated all-evidence guarantee is not fail-closed at the untrusted artifact boundary.
No public package API changes. The internal publisher contract changes one ownership rule: preview replacement may mutate pr/<number>/ except its reserved visual/ child.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
preview replacement preserves the visual child |
internal CI contract | sibling-owned subtree boundary | covered by focused test | finding — copy does not enforce the same boundary |
OSSIFICATION: local helper only; no exported surface. The existing immutable-path publisher and shared queue are the class, and the cost of getting the boundary wrong is mutable evidence behind a supposedly immutable URL.
VERDICT: note — no public API; the internal ownership defect is carried by ARCHITECTURE.
No styles, tokens, targets, variables, or rendered component contracts changed; exact-diff style grep returned zero.
VERDICT: clear
BEHAVIOR: intended preview behavior changes from deleting all PR content to preserving visual evidence; the normal path works, but a colliding source entry still changes evidence bytes. API: no public API or consumer call-site change. VISUAL: no rendered component pixels, DOM, geometry, or color change. THEME: no theme surface change.
VERDICT: BLOCKS — the changed behavior does not hold for every accepted source tree.
EFFECTS: zero.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
| none | n/a | n/a | no render path | no listeners/resources | publisher suite |
RENDER: no React/render path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or shipped package change. The replacement adds one directory listing and preserves the existing per-entry deletion shape; no reachable performance degradation was found.
VERDICT: clear
VISUAL CHECK: not applicable WHY: both changed files are a filesystem publisher and its Node test; no styles, DOM, component output, or user-facing pixels change. The acceptance evidence is exact byte content in isolated Git repositories, not screenshots.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
Exact-diff accessibility/i18n grep returned zero. No roles, ARIA, strings, locale, direction, focus, or interaction surface changed; exact-head pr-a11y, pr-rtl, and stable visual jobs correctly skipped because no component changed.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — copy can cross the reserved boundary |
| ARCHITECTURE | BLOCKS — preview remains a second writer of evidence |
| IMPACT | BLOCKS — trusted evidence bytes remain replaceable |
| API | note — internal contract only |
| THEMING | clear |
| BREAKING | BLOCKS — behavior is incomplete for accepted source trees |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear |
GOAL: not met — the normal and retry paths preserve same-PR evidence, but an accepted Storybook source tree containing visual/evidence.json replaced seeded newer evidence with its own bytes.
DISPOSITION: preview-source collision → blocks now.
ADVICE: bounded outcome criteria — a preview deploy must be unable to write any entry under pr/<number>/visual/, and a regression must exercise a colliding source entry as well as deletion/retry preservation.
AUTHOR CAN PROCEED: yes — ownership and verification criteria are settled; implementation HOW remains with the author.
WORST OUTCOME: “A PR-controlled preview artifact can replace the trusted visual evidence a maintainer is trying to review.” → request changes.
JUDGEMENT NEEDED: none — this is enforcement of the existing subtree ownership contract.
request changes
- [BLOCKS] preview copy can write the sibling-owned
visual/subtree → a maintainer can follow a successful visual report and receive preview-artifact bytes instead of the trusted evidence ·.github/scripts/lib/gh-pages-publisher.mjs:1547-1590
Semantic verdict: request changes
Thanks—this fixes the deletion path, but a later preview deploy can still replace the trusted visual evidence a maintainer is trying to review. replacePreviewContents() skips visual, then copyContents(storybookDir, destination) accepts it again; my exact-head probe changed seeded “newer trusted evidence” to PR-controlled artifact bytes. Please make pr/<number>/visual/ write-protected for both removal and copy, and add that collision as a regression. Retry, idempotency, other-PR, and cleanup paths otherwise held.
[Reviewed by Robohands]
None.
- Exact-head focused publisher suite: 25/25 pass. Its new assertion turns red against the parent deletion behavior; a separate reserved-source collision probe stays red at the reviewed head.
- A rejected-push control preserved three same-PR evidence trees, including newer bytes added by the competing writer; the identical redeploy was a no-op and another PR remained unchanged. Automatic deploy also validates PR number, full head SHA, head repo id/ref, run id, and artifact hash before publish.
- Syntax, Prettier, repository-defined strict ESLint, filtered actionlint,
check:repo, and diff-check pass. Exact-head CI passes 626 files / 13,117 tests; build, Storybook, Sandbox, docsite, lint, and required statuses are green.
TIME total 25m setup 8m fresh worktree, current rules/wiki, untrusted-script inspection, dependency seed; warm main not needed reading 5m diff, full publisher/test, callers, ownership, history, prior feedback measuring 6m focused suite, deletion mutant, collision probe, retry/idempotency control, repository checks writing 6m draft, two critic passes, wiki and GitHub delivery waste 2m unfiltered actionlint and forced no-ignore ESLint only reproduced repository baseline/config exclusions
- No live gh-pages mutation was performed; publisher behavior was exercised against isolated bare Git remotes.
The critic rewrite added the prior-review check, a warm opening, and an exact-head code link. The second pass made no further changes; the REVIEW block was posted verbatim.
#5696 fix(ci): preserve visual reports during preview deploy by cixzhang (bucket: the maintainer)
1660f8bef9c97a6acef7f9b04a35b54fbe10181b
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.15.1
LANE: full
WHY: the code delta is only one guard, but this is a write-credential trust boundary, the prior round requested changes, and approval depends on collision shape, mutation ordering, retry, idempotency, cleanup, and subtree-isolation evidence.
PRIOR REVIEW: Round 1 asked that both deletion and copy enforce the reserved pr/<number>/visual/ boundary. This round verifies that exact blocker rather than re-deriving it.
WHY 1: A successful visual report could disappear or be replaced when a later preview deployment rewrote the PR directory containing it. WHY 2: A maintainer reviewing or accepting changed pixels could then lose the exact evidence behind the recorded status. WHY 3: Visual review cannot be audited or safely used for release decisions when its trusted evidence is mutable by a PR-controlled preview artifact.
USER-FACING PROBLEM: A maintainer revisiting a PR after a preview retry or redeploy must receive the same trusted visual evidence bytes, not missing or PR-controlled replacements. PROBLEM SEVERITY: broken task — mutable or missing evidence makes the recorded visual decision unverifiable. NEW FEATURE CASE: not a new feature. EARLY STOP: clear — the prior-head negative control reproduces the overwrite and identifies the affected maintainer task.
VERDICT: clear
Every top-level visual path that can arrive through the configured artifact transport is checked before the publisher opens or edits a Pages checkout. A collision stops the publish; otherwise preview replacement keeps the existing evidence subtree while replacing Storybook and Sandbox. The trusted visual publisher therefore remains the only writer of those evidence bytes.
SOLUTION (1 decision · 9 runtime lines of 50 added lines)
- Reject a Storybook artifact containing top-level
visualbefore preview mutation, while preserving the existing normal replacement boundary — serves the stated evidence-integrity problem.
BURDEN: low — one synchronous path-presence guard, no state, dependency, schema, module, workflow, or public surface; one focused regression added to the existing bare-Git integration suite. BURDEN MATCH: proportionate — the shared publisher that accepts the untrusted source tree enforces the boundary before acquiring a destination checkout.
VERDICT: clear
OWNER: the shared gh-pages publisher owns preview-tree mutation; the immutable visual publisher remains the sole writer of pr/<number>/visual/ bytes.
TIER 1: shared publication queue and immutable-path publisher reused.
TIER 2: none.
SEAMS: automatic preview deploy, manual redeploy, artifact extraction, ordinary replacement, rejected source, rejected push/fresh-tip retry, identical redeploy, cleanup, and other-PR subtrees.
BEHAVIOR UNIT: inline guard — one path invariant immediately before the existing mutation loop, exercised through publishPrPreview against isolated bare Git remotes.
COMPLEXITY BUDGET: 1 runtime decision, 1 mutation owner, 1 authoritative evidence writer, 0 new durable representations, 0 new modules, 0 new workflows.
ACTUAL BURDEN: 2 changed files; 9 runtime lines total (3 added this round); 0 new owners/modules/schemas/workflows; 26 focused publisher tests plus artifact-shape controls.
BURDEN TREND: 5e31be6c → 1660f8be: grew by one 3-line preflight guard and one 31-line regression; ownership and durable representations stayed flat, justified by the prior overwrite finding.
RESET TRIGGER: not triggered — the fix removes the second writer in behavior without adding another owner, representation, workflow, or synchronization guard.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| same-PR visual evidence bytes | immutable visual publisher at pr/<number>/visual/<head>/<run>/<attempt>
|
review links and acceptance records identify those immutable bytes | none — a colliding preview source is rejected before checkout |
| seam | driven result |
|---|---|
top-level visual directory |
rejects with the reserved-path error before creating a checkout; destination tree unchanged |
top-level visual regular file |
same pre-mutation rejection |
live file and directory symlinks named visual
|
same pre-mutation rejection; the configured artifact uploader follows live symlinks, so these arrive materialized at that name |
| dangling symlink | the configured uploader omits it; a direct-only control fails later without a push, so it cannot replace evidence |
| case and path normalization | normalized source roots still reject; on production Ubuntu only exact lowercase visual aliases the reserved path, while the review Mac conservatively rejects case aliases too |
| ordinary preview replacement | stale preview files are replaced; the same-PR visual Git tree and another PR's full Git tree remain byte-identical |
| rejected push followed by fresh retry | newer same-PR trusted evidence from the competing writer survives the retry byte-identically |
| identical redeploy | returns published: false; branch and destination tree hashes do not move |
| open/closed cleanup | removes only stale preview content; open-PR evidence and the other open PR remain byte-identical |
Any visual path that can arrive through the configured artifact transport now fails before the mutation loop, so deletion and copy share one fail-closed ownership boundary.
VERDICT: clear
Maintainers can revisit a visual result after ordinary deploys, retries, and cleanup and receive the same-PR evidence bytes. A PR-controlled Storybook artifact can no longer replace those bytes, while preview updates for that PR and every other PR continue normally. NEW FEATURE IMPACT: not a new feature.
VERDICT: clear
No public package API changes. The internal publisher contract now rejects a Storybook source that names its reserved evidence child before any preview checkout or destination mutation.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
publishPrPreview: top-level visual is invalid input |
internal CI contract | sibling-owned subtree boundary | focused integration test | ok |
OSSIFICATION: local internal guard only; no exported surface. It enforces the already-established immutable-path ownership contract rather than creating a second policy source.
VERDICT: clear
No styles, tokens, targets, variables, DOM, or rendered component contract changed; the exact delta is confined to the filesystem publisher and its Node integration test.
VERDICT: clear
BEHAVIOR: intentional internal change — a colliding preview artifact is rejected before mutation; ordinary replacement, retry, idempotency, and cleanup behavior remain intact. API: no public call site, type, default, or package export changes. VISUAL: no component pixels, DOM, geometry, or color changes. THEME: no target, token, variable, or override changes.
VERDICT: clear
EFFECTS: zero.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
| none | n/a | n/a | no render path | no listeners/resources | publisher suite |
RENDER: no React or rendering path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or shipped package change. The normal path adds one filesystem existence check before the existing clone/copy work; no new scaling dimension or retained resource is introduced.
VERDICT: clear
VISUAL CHECK: not applicable WHY: both changed paths are a Node filesystem publisher and its integration test; no style, DOM, component output, or user-facing pixels change. Acceptance evidence is exact Git-tree and byte identity in isolated repositories.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
The exact delta adds no rendered element, role, ARIA, focus/keyboard path, user-facing string, locale, direction, or translation key. Component a11y and RTL jobs are outside this non-rendering change's path.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear |
GOAL: met — the prior head overwrote seeded trusted bytes, while this exact head rejects the identical collision before checkout and leaves the destination Git tree unchanged; normal, retry, idempotent, cleanup, and cross-PR controls all retain their expected hashes. DISPOSITION: prior preview-source collision → resolved at this exact head; no remaining negative finding. ADVICE: omitted — the implementation and bounded evidence now satisfy the existing subtree-ownership contract. AUTHOR CAN PROCEED: yes — no code or evidence change remains; because the PR is a draft, the public disposition is an approve-on-undraft comment. WORST OUTCOME: none found → approve-on-undraft comment.
JUDGEMENT NEEDED: none — this enforces an existing internal ownership boundary and adds no public or design surface.
comment — semantic verdict: approve once undrafted
No remaining findings.
Semantic verdict: approve
Thanks—this is an approve once you undraft. A reserved visual artifact now rejects before any destination mutation; retry, idempotency, cleanup, and cross-PR isolation still hold.
[Reviewed by Robohands]
None.
- Exact-head publisher suite: 26/26 pass. The prior-head negative control overwrites
visual/evidence.json; the same input at this head rejects and leaves the whole destination Git tree byte-identical. - Ten additional controls cover directory/file/live-symlink collisions, normalized roots, normal replacement, latest-tip retry, idempotency, cleanup, same-PR evidence bytes, and other-PR isolation.
upload-artifact@v7follows live symlinks and omits broken ones; the direct dangling-link control cannot push. - Syntax, Prettier, repository strict ESLint, filtered actionlint,
check:repo, and exact diff-check pass. Exact-head CI run 33323083372 is green: 626 test files / 13,118 tests pass; build, Storybook, Sandbox, docsite, lint, smoke, dependency, theme-layer, fixture-contrast,review-required, andvisual-acceptancepass; component-only visual/a11y/RTL jobs correctly skip.
TIME total 24m setup 6m fresh exact-head worktree, current kit/rubric/wiki, untrusted delta inspection; warm main not needed reading 7m prior review, delta, publisher/callers, artifact transport, ownership and history measuring 7m focused suite, 12 boundary/negative controls, repository checks, exact CI writing 6m presentation, two critic passes, wiki and GitHub delivery waste 1m unfiltered actionlint and forced no-ignore ESLint reproduced unchanged repository exclusions/baseline diagnostics
- No live gh-pages mutation was performed; all publisher mutation evidence used isolated bare Git remotes.
The first critic pass narrowed the artifact-reachability and destination-mutation wording and added pinned transport evidence. The second pass made no further changes; the REVIEW block was posted verbatim.