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.