Review 5371

Review 5371 — ci: publish PR previews as real GitHub deployments

PR: #5371 · author cixzhang Verdict: request changes (engineering) — posted as a comment. GitHub accepts neither an approval nor a change-request from a PR's own author, so the mechanical verdict is comment.

HEAD REVIEWED

aebf786abec11aa9b892ec6d1817d82a3513ab34

Every claim below was verified at this commit. Re-checked at the end of the run: unmoved, updatedAt 2026-08-27T03:48:28Z. Merge-base with main: 789ef5745cfd5fcf8caf7cc58b4065fba9ba3681.

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: three independent runtime decisions and ~122 non-comment added lines, over the ≤50-line / one-decision bar. Promoted at first read of the diff, not later.

PROBLEM

WHY 1: The Storybook and Sandbox previews exist only as a bot comment, so GitHub itself does not know they exist.

WHY 2: A reviewer who does not scroll the comment thread never finds the preview, and any surface that reads deployments rather than comments — the PR's own deployment box, the commit, the Deployments API — shows nothing at all.

WHY 3: The preview is the cheapest way to see what a change does to the library. When the only route to it is parsing a comment, reviewers judge the diff instead of the rendered result, which is the failure mode the previews were built to prevent.

USER-FACING PROBLEM: A reviewer opening a PR in anything that reads GitHub's deployments sees no preview, so they review the change without ever looking at what it renders.

PROBLEM SEVERITY: harmful friction — the review completes, but the preview is reachable only by hunting a comment thread, and is invisible to every deployment-aware surface. Nobody is blocked; nothing is unreachable.

Evidence: the only preview link on this PR today is inside the github-actions[bot] "PR Analysis Report" comment. GET /repos/facebook/astryx/deployments?environment=Storybook and ...=Sandbox both return 0 records; Preview (Vercel) returns 100+, which is the shape being copied.

VERDICT: clear

SOLUTION

When a preview is published, the repo now tells GitHub about it in GitHub's own vocabulary: this commit has a Storybook site here and a Sandbox site there. GitHub then shows it everywhere it shows deployments, and anything that asks GitHub what a commit deployed gets the links back instead of nothing. When the preview is later deleted because the pull request closed, the same system is told the site is gone so the links stop being offered. Responsibility stays with the jobs that already publish and delete the preview directory, so there is no second list to keep in sync.

SOLUTION (3 decisions · ~122 non-comment lines of 176 added)

1. Publish each preview as a GitHub Deployment on the PR's head commit, in environments Storybook and Sandbox — problem: previews invisible to deployment-aware surfaces 1a. Carry the full head SHA through the CI metadata artifact so the trusted job can name a commit — enabling; ships with 1, cannot ship apart
2. Retire those records when the closed PR's preview directory is deleted — problem: a link that outlives the site it points at
3. Record from the manual re-deploy too, replacing the automatic run's failure — problem: a rescue re-deploy leaves a stale failure standing

More than two decisions is normally a signal to split, and this one does not split. Decision 1 alone ships the dead-link problem decision 2 exists to prevent; decision 3 alone records nothing. There is no intermediate state we would ship.

Decision 2 does not do what it says. cleanup-previews.yml:298 looks up deployments?sha=${head_sha} where head_sha is gh pr view --json headRefOid — the PR's final commit. Records are written once per push, per environment. Counted in one unit, records:

	records
created across the 20 most recently closed PRs (75 head commits × 2 environments)	150
retired by cleanup (final commit of each of 20 PRs × 2 environments)	40
left success and active, still advertising a deleted pr/<n>/ directory	110

13 of the 20 PRs pushed more than once; one pushed 18 times.

Reproduction:

gh pr list --repo facebook/astryx --state closed --limit 20 --json number,headRefName \
  --jq '.[]|"\(.number) \(.headRefName)"' | while read -r n b; do
    gh api "repos/facebook/astryx/actions/workflows/ci.yml/runs?branch=$b&per_page=100" \
      --jq '[.workflow_runs[].head_sha]|unique|length'; done
# 75 head SHAs over 20 PRs; x2 environments = 150 records, 40 retirable.

BURDEN: low — no product runtime, no state, no dependency. Four extra API calls per push, two permanent repo environments, and two hand-duplicated copies of one ~30-line script.

BURDEN MATCH: proportionate — the capability is a dozen API calls and two permissions. The one real cost is the deliberate duplication, and it is where the second finding landed.

VERDICT: BLOCKS — retirement reaches 40 of the 150 records the sample would create; the other 110 keep a live link to a directory the same job deleted.

ARCHITECTURE

OWNER: the two workflows that already own the preview's lifetime — deploy-preview.yml publishes pr/<n>/, cleanup-previews.yml deletes it. The records are attached to exactly those two, plus the manual re-deploy. TIER 1: none — no component system, no shared library primitive. GitHub's Deployments API is the reused system, and the repo already runs it for Vercel's Preview environment. TIER 2: none. SEAMS: the pr-meta.json artifact (the trust boundary between untrusted PR code and the privileged workflow_run job) · the fork-PR path · the manual workflow_dispatch re-deploy · the daily cron cleanup · a metadata file written before headSha existed. BEHAVIOR UNIT: inline shell, duplicated in two workflows on purpose — deploy-preview.yml checks out nothing (that is its security property), so it cannot call a local composite action. The reasoning is stated in the diff and it is correct.

seam	driven result
fork PR head SHA	not driven — static lane. Source-level: ci.yml:253 is github.event.pull_request.head.sha, the real head, not the refs/pull/N/merge commit; the base repo holds it as refs/pull/N/head. Author's probe on #5370 reports it created.
old metadata without headSha	not driven — static lane. Source-level: deploy-preview.yml:90-93 blanks a non-40-hex value and :202 skips the record, so the deploy still happens. Correct.
manual re-deploy	fails — see BREAKING. redeploy-preview.yml:119 is a bare if: always(); deploy-preview.yml:202 guards the same script with steps.verify.outputs.ready == 'true'. The two copies have already diverged.
daily cron cleanup	not driven — static lane. Source-level: the retire step is gated on steps.cleanup.outputs.deleted_prs != '', which is only written after the DELETED -eq 0 early exit and before the push; a failed push exits 1 and the step is skipped. Sound.

Placement is right and the duplication is argued. The consequence of the duplication is not a placement question — it is the behaviour defect below.

VERDICT: note — two inline copies of one contract, justified, and already disagreeing on when to record. The consequence is filed under BREAKING against the line that causes it.

IMPACT

Builders and reviewers. Everyone who opens a PR in this repo, and anything that reads deployments rather than comments. Today the preview box on a PR is Vercel's Preview only; after this it is Preview, Storybook and Sandbox, with the Storybook and Sandbox links where GitHub puts links instead of in the middle of a bot comment. Nobody has to opt in and nothing changes for an end user of the library.

What landing this newly exposes. Deployment records are permanent objects on the repo, and today there are none in these two environments. After this lands the repo starts accumulating them at 2 per push: on the 20-PR sample, 150 records of which 110 are never retired and keep offering a pr/<n>/ link that 404s. That is new — the existing dead link on a closed PR is one bot comment; this adds 5.5 more per closed PR, on the repo's Deployments page and to every unfiltered API read. It is not pre-existing debt and it does not go away on its own; a later fix does not retroactively retire what shipped in the meantime.

VERDICT: note — the change reaches every reviewer positively; the un-retired records are finding 1's cost, restated here as who pays.

API

no API change. Nothing in packages/ is touched; the diff is four files under .github/workflows/.

OSSIFICATION: the closest thing to permanent surface here is the two environment names, Storybook and Sandbox. Both already exist on the repo (GET /repos/facebook/astryx/environments lists them alongside Preview, Production, canary, github-pages, copilot), so nothing new is being named. transient_environment: true is the right marker for a per-PR site. Class: Preview, Vercel's, on this same repo — a landed member, same shape. Cost of being wrong: renaming an environment later orphans its records; low, and reversible by a sweep.

VERDICT: clear

THEMING

n/a — CI only, no styles.

STRUCTURAL ONLY — no styles, no strings, no roles.
  grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle" <4 changed files>
      -> 4 hits, all issue numbers inside comments (#2941, #4290, #4530, #4672)
  grep -nE "aria-|role=|useTranslator|@astryx\." <4 changed files>
      -> 0

no new theme targets.

VERDICT: clear

BREAKING

BEHAVIOR: yes — one new defect. redeploy-preview.yml:119 records on every exit path, not only the ones that deployed something:

      - name: Record the preview as a GitHub deployment
        if: always()

STATE=failure unless steps.deploy.outcome is exactly success, so skipped and cancelled both post failure to both environments. Two reachable paths:

• Artifacts expired. #5519 landed on main on 2026-08-26 and rewrote this workflow to download CI artifacts instead of rebuilding. Its new "Verify preview artifacts are present" step exit 1s with an actionable message — "missing or expired — re-run CI on PR #N; deploy-preview will publish the fresh preview automatically." That is a clean stop where nothing was deployed and nothing is wrong. Merging this PR on top makes that stop stamp failure on both environments. Storybook artifacts are retention-days: 30, so this is the ordinary case on any PR older than a month — exactly when someone reaches for a manual re-deploy.
• Cancelled run. concurrency: cancel-in-progress: true on pr-preview-${{ inputs.pr_number }}; always() runs on cancellation by design. Of the last 12 Re-deploy Preview runs, 3 ended cancelled or failure (35 runs total since April).

deploy-preview.yml:202 gets this right for the same script:

        if: always() && steps.parse.outputs.head_sha != '' && steps.verify.outputs.ready == 'true'

The PR body's own claim — "A run that never had artifacts to deploy … records nothing at all" — is true of deploy-preview.yml and false of redeploy-preview.yml.

Behaviour states the diff moves through: empty — no PRs deleted, retire step skipped by its if, correct · error — push fails after 5 attempts, cleanup exits 1, retire step skipped, correct · boundary — a metadata file with no headSha blanks and skips, correct · loading / disabled / controlled: not reachable — no component, no state, no props.

API: no — nothing in packages/, no exported symbol, no signature. VISUAL: no — no rendering path is touched; four .yml files under .github/workflows/, grep above returns zero style and zero DOM hits. CI's visual-acceptance on this head reports "No stable visual scope. (PR #5371)". THEME: no — no targets, tokens or overrides exist in these files.

VERDICT: BLOCKS — the manual re-deploy posts failure for both environments on exits where nothing was deployed and the live preview is unchanged.

PERFORMANCE & RESOURCES

EFFECTS: zero. No React, no component, no hook — the diff is four GitHub Actions workflow files.

RENDER: no reachable change — nothing renders. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency, no shipped byte. jq and gh are both already on the runner and already used by these workflows.

API-call and object cost, counted rather than adjectived:

• +4 GitHub API calls per push (2 creates + 2 statuses) in deploy-preview, and the same in a manual re-deploy.
• +2 permanent deployment records per push. The repo records 1 today (Vercel's Preview), so this triples per-push deployment objects, 1 → 3.
• Cleanup: 1 gh pr view + 2 list calls + N status POSTs per deleted PR, only on runs that actually deleted something.
• The part that accumulates is finding 1's: 110 of 150 records on the 20-PR sample are never retired. A count that goes up and stays up.

Nothing here is a user-perceived cost. The record count is a cost we carry, and whether it is acceptable is a decision the retirement fix removes rather than one anyone has to make.

VERDICT: note — no runtime cost; +2 permanent records per push, of which 110 of 150 are currently never retired (finding 1).

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: genuinely non-rendering. The complete changed-path list is .github/workflows/ci.yml, .github/workflows/cleanup-previews.yml, .github/workflows/deploy-preview.yml, .github/workflows/redeploy-preview.yml (gh pr diff 5371 --name-only, 4 of 4). No component, story, style, template or doc file is touched; the structural grep in THEMING returns zero style and zero DOM/ARIA hits. CI's visual-acceptance job on this exact head returns "No stable visual scope. (PR #5371)" and pr-a11y / pr-rtl are skipping for the same reason. No frames, and none are owed.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

n/a, with the check rather than the assertion. §1: no rendered output, no interactive element, no ARIA attribute, no focus path — grep -nE "aria-|role=" over the four changed files returns 0. pr-a11y is skipping on this head because no component changed, which is the correct result and not a gap. §9: no user-visible or AT-visible string is added; the only new strings are workflow log lines and a deployment description (Preview for PR #N) that GitHub renders in its own chrome, not in the product. No direction, no logical-property, no locale surface.

VERDICT: clear

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	BLOCKS — retirement reaches 40 of 150 records
ARCHITECTURE	note — two copies of one contract, already diverged
IMPACT	note — 110 un-retired records per 20 closed PRs, newly created
API	clear
THEMING	clear
BREAKING	BLOCKS — re-deploy posts failure where nothing deployed
PERFORMANCE	note — +2 permanent records per push, 110 of 150 never retired
VISUAL	clear
A11Y & I18N	clear

GOAL: partly met — the recording half works: ci.yml:253 carries the true PR head (github.event.pull_request.head.sha, not the merge commit), deploy-preview.yml:226-245 creates and statuses it, and the author's live probes on #5348, #5345, #5343 and #5370 show it end to end. The retirement half reaches 40 of the 150 records the sample would create. The manual re-deploy records, but mislabels its non-deploy exits.

DISPOSITION:

1. Retirement is keyed to the PR head, records are keyed to the commit → blocks now
2. Manual re-deploy records failure on skipped/cancelled exits → blocks now
3. record() runs under set -euo pipefail and calls Storybook before Sandbox, so a transient API error leaves one environment recorded, the other absent, and the run red → accepted: Deploy PR Preview runs on workflow_run and is not a PR check, so the red run is invisible on the PR; the next push re-records both. Cost is a missing box until then, paid by whoever reads the PR. Raising it would be a third finding on a two-finding review; noted here, not asked for.
4. main moved under all four files since this head (10 commits, ci.yml +229/−9) → no finding: a local --no-commit merge of origin/main into this head is conflict-free and semantically intact — id: deploy lands on the right step, steps.pr-info.outputs.head_sha and PR_NUMBER both survive. Checked because a four-day-old PR against fast-moving CI files needs it; reported because a clean result is a result.

ADVICE: proven existing pattern, for finding 2 — deploy-preview.yml:202 already guards this exact script with steps.verify.outputs.ready == 'true', one file over, in this same PR. For finding 1, bounded outcome criteria only, no prescribed implementation.

AUTHOR CAN PROCEED: yes —

1. After a closed PR is cleaned up, no deployment in Storybook or Sandbox for any commit of that PR is still active. (The description the diff already writes, Preview for PR #N, identifies them; walking the PR's commits also does. Author's choice.)
2. A Re-deploy Preview run that deployed nothing — skipped or cancelled — writes no deployment status at all.

WORST OUTCOME: "Someone asks for a manual re-deploy of a month-old PR, is told to re-run CI because the artifacts expired, and the PR is left publicly claiming both previews failed while the live preview is untouched." → request changes.

JUDGEMENT NEEDED: none — the capability question is already settled by the author's own direction comment on this PR (2026-08-27): "keep the deployment integration. Storybook and Sandbox previews should be first-class GitHub Deployments with cleanup, not links discoverable only by parsing bot comments." Both findings are defects against the PR's own stated contract, not design calls.

ENGINEERING VERDICT: request changes. MECHANICAL POSTED VERDICT: comment — GitHub accepts neither an approval nor a change-request from a PR's own author. Nothing was posted: this run was read-only.

1. [BLOCKS] Records are written per commit; retirement queries only the PR's final commit → after a closed PR is cleaned up, every earlier push still advertises a live Storybook and Sandbox link to the pr/<n>/ directory the same job deleted — 110 of 150 records across the 20 most recently closed PRs · cleanup-previews.yml:298
2. [BLOCKS] if: always() with no deploy-outcome guard, unlike its twin → someone re-deploys a month-old PR, is told the artifacts expired and to re-run CI, and the PR is left claiming both previews failed while the live one is fine · redeploy-preview.yml:119

REVIEW

Thanks — the required_contexts and auto_inactive notes each save someone a day, and the probe table is the right evidence.

Two things before it lands.

Records are written per commit and retired per PR head. Over the last 20 closed PRs that is 150 records against 40 retirements: 110 success records keep a live link to a pr/<n>/ directory the same job just deleted.

And the manual re-deploy's always() has no equivalent of deploy-preview's verify.ready guard — the two inline copies have already drifted — so with #5519 on main its "artifacts expired, re-run CI" stop posts failure for both environments while the preview is still live.

For the retirement, does matching the description you already write feel right, or would you rather walk the PR's commits?

[Reviewed by Robohands]

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

• .github/workflows/cleanup-previews.yml:298 — Only the head commit. Every earlier push's record stays active pointing at the directory just deleted.
• .github/workflows/redeploy-preview.yml:119 — Hmm, always() fires on skipped and cancelled too. deploy-preview gates on verify.ready; this doesn't.

EVIDENCE I DID NOT SPEND

• actionlint before/after on the four files: 33 findings each side. Every difference is a line-number shift of a pre-existing SC2086/SC2034, plus the SC2129 in deploy-preview.yml genuinely removed. The test plan's claim is honest, including the one new unquoted $GITHUB_OUTPUT matching its neighbours.
• The trust boundary holds: PR_NUMBER and HEAD_SHA come from an artifact written by PR-triggered CI, and deploy-preview.yml:79-93 regex-validates both before they reach a privileged API call or a URL. deployments: write in the workflow_run context adds no reachable injection.
• Vercel keeps every commit's Preview deployment success and active — 8 of them on closed #5601. That is fine for Vercel because its URL is per-deployment and never deleted; this PR's URL is per-PR and is deleted, which is exactly why the retirement has to match the recording's granularity.

LIMITATIONS

• Static/CI lane. Nothing was installed, built or executed: no workflow run, no pnpm install, no browser. Evidence is source reading at the exact head, read-only GitHub API queries, two actionlint runs, and one local --no-commit merge preview.
• What GitHub's UI renders for a stale active deployment was not checked. Finding 1 is stated at the API level, which is what the PR's own body names as the audience; no claim is made about a button.
• Remote CI owns execution of these workflows. workflow_run workflows run from the default branch, so this PR cannot exercise its own changes — the first real run through this YAML happens after merge.

REVIEW LOOP

Full lane. Two gates: gate 1 failed on a unit mismatch between the private slots and the public text (a commit count set against a PR count in one clause) and an unfilled TIME slot; gate 2 came back clean with no violations. Reviewer, critic and evidence were all performed in one session — no pass was delegated.

TIME

TIME  total 22m
  setup       2m   dedicated worktree at the PR head; no install, no build
                   (warm main reused: yes — read-only, for origin/main and the merge check)
  reading     7m   brief + critic in full, loop version, presentation, harness, the 4 workflow files
  measuring   6m   11 read-only GitHub API queries, 1 local merge preview,
                   2 actionlint runs (base vs head), 0 re-measures
  writing     4m   presentation + critic pass + round-2 revision
  publishing  3m   fresh wiki clone, this page, the index row, lock + rebase + push
  waste       1m   one grep over the four changed files quoted its file list wrong and
                   had to be re-run; one candidate finding (that the recorded SHA
                   might be the merge commit) took a file read to kill.

Remote CI wait: 0m — CI was already complete and green on this exact head before the run started (19 checks; pr-a11y, pr-rtl and the review-clear chain correctly skipping). Nothing was queued or waited on.

What changed before posting

Posted as drafted in review 5043574438. No judgment or wording changed after the clean gate.

---

Round 2 — rebased maintenance review

PR

#5371 ci: publish PR previews as real GitHub deployments by cixzhang (bucket: the maintainer)

HEAD REVIEWED

b204b467ac4d850cb5518d996e63f978d3b2502f

VERSIONS

LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.15

LANE

LANE: full WHY: This is a conflicted re-review of repository-wide deployment infrastructure with a prior blocking review and a privileged API permission.

PROBLEM

WHY 1: Storybook and Sandbox preview sites exist, but GitHub does not have Deployment records for them. WHY 2: Reviewers and tools that use GitHub's deployment surfaces cannot discover those previews without parsing a bot comment. WHY 3: The preview is the fastest way to inspect a change's rendered behavior, so hiding it from normal review surfaces makes visual verification less likely. USER-FACING PROBLEM: A reviewer using GitHub's deployment-aware surfaces cannot find either rendered PR preview and may review the code without inspecting the result. PROBLEM SEVERITY: harmful friction — the preview exists, but the normal deployment discovery path is empty. NEW FEATURE CASE: The maintainer publicly settled the need in this direction comment; current API reads return zero Storybook and zero Sandbox deployments, while the bot comment contains both URLs. EARLY STOP: clear — the repository owner explicitly wants this capability and the current system cannot expose it.

VERDICT: clear

SOLUTION

The existing publisher now tells GitHub whenever it publishes a PR preview, using one deployment record for Storybook and one for Sandbox. The same owner records a failed publish as failed and identifies every record for a deleted PR by its stable description. Automatic publishing, manual publishing, and cleanup therefore share one lifecycle implementation instead of copying it into each workflow.

SOLUTION (2 decisions · 251 production/workflow additions, 15 deletions)

1. The shared publisher creates and statuses both transient deployments after an actual preview publish attempt — serves deployment discovery and truthful failure state.
2. Cleanup returns the PR numbers whose directories it removed, and the same publisher retires every matching historical deployment — prevents links from outliving the deleted site.

cixzhang's prior review found that "Records are written per commit and retired per PR head" and that manual re-deploy's always() "has no equivalent of deploy-preview's verify.ready guard." Both blockers are resolved. Retirement now matches the stable Preview for PR #<n> description across every deployment in both environments (gh-pages-publisher.mjs:216-261), not only the final head. Manual re-deploy reaches the publisher only after its artifact verification succeeds (redeploy-preview.yml:92-113), so expired artifacts produce no false failure; both publish paths call the same implementation.

BURDEN: medium — one existing publisher gains one GitHub API adapter and two lifecycle helpers; three workflows add only permission/head wiring; 150 test lines cover payloads, failure propagation, historical retirement, cleanup output, and workflow structure. BURDEN MATCH: proportionate — real deployment lifecycle requires create, status, and inactive operations, while the prior duplicated workflow implementations and the no-longer-needed CI metadata change are removed.

VERDICT: clear

ARCHITECTURE

OWNER: .github/scripts/lib/gh-pages-publisher.mjs, the existing owner of every gh-pages publication and cleanup mutation. TIER 1: GitHub Deployments REST API, reused through one request adapter. TIER 2: none. SEAMS: automatic workflow_run publish, manual workflow_dispatch re-deploy, cleanup after a PR closes, failed gh-pages push, no-change re-publish, and multiple commits on one PR. BEHAVIOR UNIT: pure lifecycle helpers around the existing publisher functions — focused tests inject the API boundary and exercise both status paths. COMPLEXITY BUDGET: one lifecycle owner, two fixed environment projections, zero new durable maps/schemas, and three thin workflow call sites. ACTUAL BURDEN: 6 files; publisher +244/−14, tests +150/−1, workflows +7; one API adapter, one record helper, one retirement helper, zero new jobs, zero new concurrency groups, zero duplicate workflow implementations. BURDEN TREND: aebf786 → b204b46 shrank from separate recording scripts in two workflows plus cleanup-by-head to one publisher-owned lifecycle. File count grew from 4 to 6 because the centralized implementation and its tests replace workflow-local shell. RESET TRIGGER: not triggered — the rebase removed duplicate owners and representations rather than adding another one.

domain fact	one authoritative writable source	generated / immutable projections	other writable copies
trusted PR identity	Resolve trusted preview target output	--pr and --head CLI arguments	none
deployment lifecycle	gh-pages-publisher.mjs record/retire helpers	Storybook and Sandbox records	none
previews deleted in this cleanup	cleanupPreviews() result	deletedPrs passed to retirement	none

seam	driven result
automatic publish	workflow passes the API-validated PR number and 40-character head to the shared publisher
manual re-deploy	artifact verification exits before the publisher; no deployment status is written for an expired artifact
failed publish	focused test proves the publisher records failure and rethrows the original publish error
multiple PR commits	focused test proves every deployment whose stable description matches the deleted PR is retired
concurrent gh-pages writers	existing shared publication queue remains the only writer coordinator; no job or concurrency group is duplicated

VERDICT: clear

IMPACT

Reviewers of every non-draft PR get native Storybook and Sandbox deployment links on the commit and PR timeline. A failed publish is visible as failed, and closing the PR retires all links to the deleted pr/<n>/ site. Existing preview contents, URLs, and publishing concurrency do not change. NEW FEATURE IMPACT: reviewers can discover and open both previews through GitHub's deployment surfaces; the current failure is demonstrated by zero records in both environments and the owner-approved direction above.

VERDICT: clear

API

No package or component API changes. The repository automation adds two existing environment names, Storybook and Sandbox, to GitHub's Deployments API; the stable description is internal lifecycle identity, not consumer surface.

	change	public?	class	doc'd?	verdict
+	GitHub Deployment for Storybook	repository UI/API	same class as Vercel Preview	PR body	ok — owner-approved
+	GitHub Deployment for Sandbox	repository UI/API	same class as Vercel Preview	PR body	ok — owner-approved

OSSIFICATION: the environment names were already chosen by the settled PR direction. Removing them later would orphan deployment history, so creation and retirement intentionally use the same fixed names and description function.

VERDICT: clear

THEMING

No theming change. All changed paths are repository automation or its tests; no component, token, target, CSS, or rendered DOM changes.

VERDICT: clear

BREAKING

BEHAVIOR: intentional repository-automation change only. Successful and failed preview publishes now create truthful Deployment statuses; cleanup deactivates every matching historical record. Expired manual re-deploys still fail before publishing and now write no false deployment failure. API: no package API or caller behavior changes. VISUAL: no rendered product or preview content changes; only GitHub's native deployment metadata appears. THEME: no target, token, variable, or override changes.

VERDICT: clear

PERFORMANCE & RESOURCES

EFFECTS: zero — no React code.

Effect + deps	external system	why render/handler cannot do it	measured render cost	lifetime + cleanup	focused test
none	GitHub Actions only	n/a	zero	n/a	n/a

RENDER: no application runtime path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or shipped-byte change. Automation cost: four Deployment API writes per publish. Cleanup performs two paginated environment reads only when it deleted a PR preview, then one inactive-status write per matching historical record.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: the six changed files are GitHub workflow, publisher, and test files. No product render, preview artifact bytes, CSS, DOM, or component behavior changes.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No rendered control, focus path, ARIA state, user-facing product string, locale behavior, or direction behavior changes. CI correctly skips component-only a11y/RTL work for this automation change.

VERDICT: clear

JUDGEMENT

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

GOAL: met — the payload tests assert two transient deployments with required_contexts: [], truthful success/failure statuses with auto_inactive: false, and all matching historical records becoming inactive; workflow tests prove both publishers pass the trusted head through the one shared command. DISPOSITION: prior retirement blocker → fixed by description-matched pagination; prior false-failure blocker → fixed by removing the independent always() recorder and calling the shared publisher only after artifact verification; no new negative finding. ADVICE: omitted — no remaining defect. AUTHOR CAN PROCEED: yes — no implementation or design decision remains. WORST OUTCOME: none found → approve.

JUDGEMENT NEEDED: none — the capability was publicly settled by the maintainer; this round verifies the implementation and prior fixes.

approve and merge (engineering verdict; the author account can only post a comment mechanically)

REVIEW

Thanks — both blockers are fixed. Cleanup retires every matching historical record, and an expired manual re-deploy records nothing. The shared publisher keeps the paths together. Ready.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

None.

EVIDENCE I DID NOT SPEND

• origin/main already centralizes every gh-pages writer in gh-pages-publisher.mjs through #5629, #5631, and #5674; it contains no Deployment API integration and the live Storybook and Sandbox environment queries each return zero records.
• Rebase used rerere.enabled=false; all three conflicts kept the centralized workflows. The clean ci.yml merge was removed because trusted head identity already comes from Resolve trusted preview target.
• Workflow structure check found one jobs block, one top-level permissions block, one job-level deployments: write, and one concurrency block in each changed workflow.

TIME

TIME total 22m setup 4m dedicated worktree, rebase with rerere disabled, fast pnpm 11 install (warm main reused: yes) reading 7m manifesto, brief, critic, harness, prior review, current main, history, workflows, publisher and tests measuring 7m 49 focused tests twice, actionlint, repository checks, API reads, structure/public guards; 1 re-measure after fixing queue-boundary attribution writing 4m PR history/body, presentation, critic passes, and review publication waste 2m broad first reads were truncated and had to be repeated in bounded chunks; one actionlint run rediscovered origin/main's existing SC2129

Remote CI wait: 16m — 19 checks passed, 6 component/visual/review-chain jobs skipped as expected; merge state is CLEAN.

WHAT I COULD NOT VERIFY

• GitHub runs workflow_run workflow definitions from the default branch, so the real create/status calls cannot execute from this PR before merge. The exact payload shape was previously live-probed; this head preserves it in focused assertions.

What changed before posting

Posted as drafted in review 5058715449. The first critic pass required an explicit quotation of cixzhang's prior findings in the private presentation; the public 27-word review was unchanged. The second critic pass was clean.

---

Round 3 — current-main rebase review

PR

#5371 ci: publish PR previews as real GitHub deployments by cixzhang (bucket: the maintainer)

HEAD REVIEWED

9445b3b98077efc651742c3e2ab65e971d66bbcb

VERSIONS

LOOP VERSION: 2.0.0 AUDIT RUBRIC: 1.15

LANE

LANE: full WHY: This is a conflicted re-review of privileged repository automation with two prior blockers, four intervening changes to the same publisher/workflows, and no pre-merge live path for workflow_run deployment writes.

CONTEXT & ROUTING

CHANGED CODE: The shared gh-pages publisher creates and statuses one GitHub Deployment per actually published Storybook/Sandbox target; cleanup retires all matching historical records before deleting a closed PR's preview; three workflows add only deployments: write. NEAREST CURRENT COMPONENT CONTRACT: .github/scripts/lib/pr-preview.mjs on current main — the trusted publication result independently marks Storybook and Sandbox available only after the gh-pages bytes and manifest are committed. CURRENT FAMILY: deploy-preview.yml, redeploy-preview.yml, cleanup-previews.yml, and pr-comment.yml on current main; automatic and manual publication already converge on the same pr-preview publisher command and the comment reads its result artifact. CURRENT API/THEMING/SYSTEM RECORDS: cixzhang's current direction on PR #5371: Storybook and Sandbox remain first-class GitHub Deployments with cleanup. DRAFT CONTEXT: Review-5371 rounds 1–2 and b204b467, used only to reconcile the prior blockers and approved behavior; neither governs current-main implementation. STOPPED AT: records — the current owner direction settles whether the capability belongs; current code/result tests settle its implementation boundary. REGRESSION EVIDENCE: current main has zero Storybook and zero Sandbox deployments; focused tests prove the current head records only result-confirmed targets, reports an attempted target's publish failure, retires every historical description match, and keeps the preview directory when retirement fails so the next cleanup retries. OWNER QUESTION (PRIVATE): none.

candidate issue	classification	evidence	disposition
first-class Storybook/Sandbox deployments	settled	owner direction comment plus zero current records	apply decision
prior review: cleanup retired only the final head	preserves	description-matched pagination retires all matching deployments	no finding
prior review: expired manual re-deploy wrote false failures	preserves	missing artifacts fail before the shared publisher is called	no finding
retirement API failure after directory deletion could strand active records	preserves	retirement now runs before deletion; focused failure test proves the directory remains for retry	no finding
current publisher supports independent Storybook/Sandbox availability	preserves	deployment targets come from the publisher result, not an assumed pair	no finding

PROBLEM

WHY 1: Storybook and Sandbox preview sites exist, but GitHub has no Deployment records for them. WHY 2: Reviewers and tools using GitHub's deployment surfaces cannot discover either preview without parsing a bot comment. WHY 3: The previews are the cheapest way to inspect rendered behavior, so hiding them from normal review surfaces makes rendered verification less likely. USER-FACING PROBLEM: A reviewer using GitHub's deployment-aware surfaces cannot find the rendered PR previews and may review code without inspecting its output. PROBLEM SEVERITY: harmful friction — the preview exists, but the normal deployment discovery path is empty. NEW FEATURE CASE: The repository owner explicitly adopted this capability; current API reads return zero Storybook and zero Sandbox deployments while PR comments carry both URLs. EARLY STOP: clear — current authority establishes the need and current code does not provide it.

VERDICT: clear

SOLUTION

The existing publisher reports only the preview targets it actually committed, and those confirmed targets become transient GitHub Deployments. Automatic and manual publication use that same owner. Cleanup retires all records for a closed PR before removing its directory, so either both lifecycle steps finish or the still-present directory makes retirement retryable.

SOLUTION (2 decisions · 284 production/workflow additions of 486 total additions)

1. Project each result-confirmed Storybook/Sandbox publication into GitHub's Deployments API — serves native discovery and truthful per-target state.
2. Retire every historical record for a closed PR before deleting its preview directory — prevents dead links and preserves retryability on API failure.

The prior review said, “Cleanup retires every matching historical record, and an expired manual re-deploy records nothing.” This round re-verifies both claims against current main, preserves them, and adds a regression boundary for retirement failure without changing the approved capability.

BURDEN: medium — one existing publisher gains one API adapter plus record/retire helpers; three workflows add one permission each; focused tests cover payloads, independent targets, publish failure, historical retirement, cleanup identity, and retry ordering. BURDEN MATCH: proportionate — the capability needs create/status/inactive operations, while all publication and lifecycle decisions remain in the existing owner.

VERDICT: clear

ARCHITECTURE

OWNER: .github/scripts/lib/gh-pages-publisher.mjs, the existing owner of gh-pages publication and cleanup mutations. TIER 1: GitHub Deployments REST API, projected from the trusted preview result. TIER 2: none. SEAMS: automatic workflow_run publish, manual workflow_dispatch re-deploy, independently missing artifacts, failed gh-pages push, retirement API failure, cleanup push retry, and multiple commits on one PR. BEHAVIOR UNIT: pure lifecycle helpers around the existing publisher; API calls are injected in focused tests. COMPLEXITY BUDGET: two runtime decisions, one lifecycle owner, zero new durable schemas/maps, and three permission-only workflow edits. ACTUAL BURDEN: 6 files; publisher +281/−19, tests +202/−1, workflows +3; one API adapter, one record wrapper, one retirement helper, zero new jobs, zero new concurrency groups, zero workflow-local Deployment API copies. BURDEN TREND: b204b467 → current head grew only to preserve current-main independent-target truth and retry-safe cleanup; owners, workflows, and durable representations stayed flat. RESET TRIGGER: not triggered — the rebase keeps one owner/source and removes the old workflow-local implementation shape.

domain fact	one authoritative writable source	generated / immutable projections	other writable copies
which preview targets exist	publishPrPreview() result	Storybook/Sandbox Deployment records	none
deployment lifecycle	shared publisher helpers	GitHub deployment statuses	none
which PR previews cleanup will remove	current gh-pages tree reconciled with open PRs	deletedPrs callback argument	none

seam	driven result
automatic publish	existing reusable workflow passes validated identity and target artifacts to one publisher command
manual re-deploy	missing artifacts fail before the publisher; no Deployment record is written
independent targets	focused test proves only result-confirmed targets are recorded
failed publish	focused test proves only attempted targets receive failure, then the original error is rethrown
historical cleanup	focused test proves every description match in both environments becomes inactive
retirement failure	focused test proves the stale preview directory remains in gh-pages for retry
workflow structure	diff adds no job, concurrency group, publisher, or inline Deployment endpoint

VERDICT: clear

IMPACT

Reviewers of every eligible PR gain native Storybook and Sandbox links in GitHub's deployment surfaces. Missing artifacts create no record, failed attempted publishes are visibly failed, and closed PR records become inactive without leaving dead links. NEW FEATURE IMPACT: reviewers can discover the exact published targets without parsing comments; the owner-approved capability is absent from current main and observable in the deployment API.

VERDICT: clear

API

No Astryx package or component API changes.

	change	public?	class	doc'd?	verdict
+	GitHub Deployment for confirmed Storybook preview	repository UI/API	same class as Vercel Preview	PR body	settled by owner direction
+	GitHub Deployment for confirmed Sandbox preview	repository UI/API	same class as Vercel Preview	PR body	settled by owner direction

PUBLIC API GATE: not applicable — no package API row. API ROW: none. NON-DERIVABLE NEED: not applicable to package API; repository deployment discovery is absent on current main. MEANING: Storybook and Sandbox records name the published preview target directly. PREDICTABILITY: only result-confirmed targets are created; attempted failures are failed; cleanup makes historical records inactive. CAPABILITY: focused tests exercise payload and lifecycle behavior; the real workflow_run mutation cannot execute before landing. DOCS OBLIGATION: not required — no consumer usage or Astryx API promise changes; the PR body documents repository behavior. OSSIFICATION: the environment names are the explicitly adopted repository contract. Creation and retirement share one description function, so lifecycle identity cannot drift between writers.

VERDICT: clear

THEMING

No theming change. Changed paths are repository automation and tests; no token, target, style, component, or DOM changes.

VERDICT: clear

BREAKING

BEHAVIOR: intentional repository-automation addition only; existing preview bytes, URLs, artifact selection, comments, jobs, and concurrency remain unchanged. API: no package API or caller behavior changes. VISUAL: no Astryx-rendered output changes; only GitHub's native deployment metadata appears. THEME: no target, token, variable, or override changes.

VERDICT: clear

PERFORMANCE & RESOURCES

EFFECTS: zero — no React code.

Effect + deps	external system	why render/handler cannot do it	measured render cost	lifetime + cleanup	focused test
none	GitHub Actions only	n/a	zero	n/a	n/a

RENDER: no application runtime path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or shipped-byte change. Automation cost: two API writes per available target. Cleanup performs two paginated environment reads when it removes a PR preview, then one inactive-status write per matching historical record.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: all changed files are GitHub workflow, publisher, or test files. No product render, preview artifact bytes, CSS, DOM, or component behavior changes.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No rendered control, focus path, ARIA state, user-facing product string, locale behavior, or direction behavior changes. Component a11y/RTL checks are not applicable to this repository-automation change.

VERDICT: clear

JUDGEMENT

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

GOAL: met — 61 focused tests pass; actionlint, Prettier, check:repo, diff, structure, and public-repo guards pass; exact-head CI has 21 successful checks/statuses and 4 expected component/visual skips; the PR is mergeable and CLEAN. DISPOSITION: both prior blockers remain fixed; the newly found retirement-error path is fixed and regression-tested; no remaining negative finding. ADVICE: omitted — no remaining defect. AUTHOR CAN PROCEED: yes — no implementation or design decision remains. WORST OUTCOME: none found → approve.

JUDGEMENT NEEDED: none — current owner direction settles the capability; this review verifies implementation and regression boundaries.

approve and merge (engineering verdict; the author account can only post a comment mechanically)

REVIEW

Thanks — the prior cleanup and expired-artifact blockers remain fixed. The rebase keeps one publisher and records only confirmed Storybook/Sandbox targets. Ready.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

None.

EVIDENCE I DID NOT SPEND

• Current main independently tracks Storybook and Sandbox availability in a signed publisher result, but contains no Deployment API calls and the live named-environment queries each return zero records.
• The rebase used rerere disabled and kept the four newer publisher/preview changes intact; only deployment-record capability remains in the PR delta.
• No changed workflow adds a job, concurrency group, second publisher, or inline Deployment API call; each adds deployments: write exactly once.

TIME

TIME total 51m setup 7m dedicated worktree, two rerere-disabled rebases, fast pnpm 11 install (warm main reused: yes) reading 11m manifesto, current brief/critic/harness, prior artifact/reviews, current main/history, publisher/workflows/tests measuring 10m 61 focused tests, actionlint, Prettier, check:repo, API/structure/public guards writing 15m conflict resolution, retry-order correctness fix, presentation, and critic passes publishing 4m two safe force-pushes, PR body refresh, pending artifact/review publication waste 4m bounded re-reads after rule-file truncation; the first final head was superseded when main advanced during validation

Remote CI wait: 29m, overlapping review work — 21 checks/statuses passed and 4 component/visual jobs skipped as expected; merge state is CLEAN.

WHAT I COULD NOT VERIFY

• GitHub runs workflow_run definitions from the default branch, so real Storybook/Sandbox Deployment writes cannot execute from this PR before landing. The current head exercises the exact payload and lifecycle contract through injected API tests.

What changed before posting

Posted as drafted in review 5070461697. Critic pass 1 held the artifact until exact-head CI and time evidence were complete; critic pass 2 was clean. This round found and fixed the retirement-failure ordering before publication.