Skip to content

Review 5518

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 2 revisions

#5518 — ci: resolve the preview deploy target from the CI run's head commit

bhamodi · open, request changes drafted · reviewed at 0fbe445b30a · view on GitHub

Verdict: request changes — the hash half is right, and the PR-number half dies on exactly the seam the workflow was written for

The review asks for the PR to be resolved by the run's own fork and branch rather than by commits/<sha>/pulls, which returns [] for a fork head, and for the unresolved path to stay a hard error. It did not merge because fork previews would regress to a green skip: the contributor's Storybook and Sandbox links 404, the deploy run finishes green, and the only notice is a ::warning:: in a log nobody opens.

Problem

Unstated in the body, and having to reconstruct it is the first finding. The body is four descriptive bullets with no sentence saying what was wrong before; the only rationale anywhere is a line the diff adds to the file header — "artifact contents never decide where a preview lands."

Reconstructed: pr-meta.json is written by ci.yml on the pull_request event, and a fork PR can edit ci.yml in its own diff, so prNumber is PR-controlled and it names a gh-pages write path (pr/${PR_NUMBER} under the checkout, deploy-preview.yml:158). A fork could aim its preview at another PR's directory. Bounded — the old code validated ^[0-9]+$, so there is no traversal — but real: a maintainer opens PR #A's preview link and is shown PR #B's build. Nobody in the repo has reported hitting it.

Solution

(3 decisions, effectively 2 · +27 −16, one file, all runtime)

  1. SHORT_HASH from workflow_run.head_sha[0:7] instead of from the artifact (:76).
  2. PR_NUMBER from commits/<head_sha>/pulls, filtered to an exact head.sha match (:81).
  3. An unresolvable PR stops being exit 1 and becomes a ::warning:: plus a green skip (:84) — a consequence of 2, and the one decision the body never argues.

The deploy job used to ask the CI run's own uploaded file where to publish. That file is written by a job the pull request can change, so the pull request effectively picked its own destination. This change stops asking the file and asks GitHub instead — which commit did this run build, and which pull request is that commit the tip of — keeping the file only as a yes/no signal that previews were built at all. Decisions 1 and 2 must ship together.

The owner is right: deploy-preview.yml is the trusted, post-CI publisher for every PR preview. The implementation does not survive the seam the file exists for, and the file says so itself at :7-10: "workflow_run runs from the base repo with the repo's own write token, so it can deploy previews for fork PRs too — one code path for every PR." Driven against the live API at this head, a same-repo PR resolves and a fork PR returns [], sets deploy=false, and goes green:

PR head repo commits/<sha>/pulls preview live on gh-pages today
#5538 facebook/astryx 1 result yes
#5533 a fork 0 yes
#5524 a fork 0 yes
#5515 a second fork 0 yes
#5512 a second fork 0 yes

Confirmed two further ways. The gh-pages pr/ inventory holds 52 directories, and of the 18 most recent, 13 are fork PRs across two different fork owners — all 13 deploy today, none would resolve under the new lookup. And review-clear.yml:59 already documents the trap in this repo — "pull_requests is empty for forks" — with :72 resolving by head_repository.owner.login plus head_branch instead, and a branch-name scan as a second fallback. This is not a missing commit: repos/facebook/astryx/commits/<fork sha> returns the object. GitHub's PR-association index simply does not cover commits that live only under refs/pull/N/head.

Impact

Every PR opened against the repo, from the moment it merges. Nothing opts in.

  • Fork contributors, the majority. They push, CI goes green, the PR Analysis Report posts a Storybook link, and the link 404s. The deploy run is green, so nothing on the PR page says a deploy was skipped. Thirteen of the last eighteen previews are these people.
  • Same-repo authors. No change — resolution succeeds, and to the same number the artifact carried.
  • Maintainers. They lose visual review on exactly the PRs where they most need it: the ones whose branch they cannot check out casually.

What landing it newly exposes: #5517, open, same author, moves pr-comment.yml onto run.pull_requests with the same commits/<sha>/pulls fallback. Both legs are empty for forks. If both land, fork PRs lose the preview and the comment that links to it — worth saying before either merges, and not this PR's to fix.

API

No API change — a CI workflow. No prop, export, type, default or theme target.

Theme targets

n/a — CI YAML only. The style grep over the changed workflow returns 0, and no target is added, removed or renamed.

Ossification

Nothing new is frozen. The only surface is the pr_number and short_hash step outputs, which already existed under those names and are consumed unchanged at :99, :113 and :138.

Breaking

There is a consumer who can be broken: everyone who opens a PR. This is repo infrastructure rather than a published package, so there is no private: true escape.

  • API — no. No call site, no signature.
  • Visual — no. Nothing rendered changes; the user-visible artefact is an HTTP 404 on a preview URL, which is behaviour.
  • Theme — no.
  • Behaviour — the axis that moves. A same-repo PR with an unmoved head is unchanged. A fork PR with an unmoved head was deployed and is now skipped, green. A head force-pushed mid-run was a stale overwrite and is now skipped, which is the intended improvement. A docsite-only PR with no artifact is unchanged. An artifact present with malformed metadata was exit 1 and red, and is now a green skip. An expired artifact is unchanged.

The far side of the new bound is the no-match branch, and the fork rows are exactly that drive. The default case — same-repo, matching head — is the one that reports "no change", which is why inspection alone concluded the change was safe.

Performance & resources

No React, so zero effects. One REST call is added per completed CI run (GET /repos/{owner}/{repo}/commits/{sha}/pulls), not in a loop and not growing with anything; two jq invocations and two grep calls are removed. Net runner time is under a second either way, and there is no rate-limit concern — the same job already spends two download-artifact calls and up to five clones. The library pays one request; nobody perceives it.

Visual evidence

No frames, because nothing is renderable: 43 changed lines of GitHub Actions YAML in one file. No geometry, no colour, no DOM, no strings — established by the style grep (0 hits) and the markup grep (0 hits), and consistent with the visual breaking axis being "no".

The evidence carrying this review is tabular instead: the live API results and the gh-pages inventory above. Both reproduce from a single API call per row, and both reach the drafted comment as numbers rather than as an assertion.

A11y & i18n

Nothing here is touched, and this is what was checked. aria-|role=|useTranslator|t('@astryx over the workflow → 0: no AT surface, no catalog string, no key. The two strings the diff adds are a ::warning:: and a shell comment, both log-only and read by a maintainer in a run log rather than by a user. CI's a11y job reports no violations on this head with no baseline entry added. Direction, focus, keyboard and target size are not reachable, because nothing is rendered.

Adjacent and deliberately filed elsewhere: the ::warning:: being the only channel that tells anyone a deploy was skipped is a discoverability problem, and it sits under Breaking as the second finding.

Judgement

request changes. The stated goal is partly met. The hash half is met and verified — five workflow runs are keyed on this fork PR's head sha, so workflow_run.head_sha is the PR head and not a merge commit, and ci.yml:240 names the artifacts from exactly that value. The PR-number half is not: the payload does not carry it for forks, and the API lookup chosen to replace it returns [] for fork heads. The uncovered member of the class is every fork PR, and it cannot follow separately, because merging half of this removes a working capability.

1. commits/<sha>/pulls returns [] for a fork head
   → every fork contributor's Storybook and Sandbox preview 404s, on a
     green run                    · .github/workflows/deploy-preview.yml:81
   confirmed three ways: live API on 4 fork PRs against 1 same-repo
   control · the gh-pages inventory (13 of the last 18 previews are
   forks) · review-clear.yml:59 documents the same trap in this repo.

2. the unresolved path is a warning, not an error
   → nothing on the PR page says a deploy was skipped, so finding 1 stays
     invisible until someone opens a run log
                                  · .github/workflows/deploy-preview.yml:84

3. [not blocking] the body never says what the old behaviour cost
   → the next reader cannot tell hardening from tidy-up, and the changelog
     inherits that                · PR body

Findings 1 and 2 are one ask — 2 is what makes 1 invisible, and fixing 1 removes the reachable case. The unstated rationale costs a reader rather than a user and is repairable in two lines, so it rides in the comment as a clause. The worst outcome — "a contributor gets a 404 on the preview link in their PR comment and nothing says why" — is not compatible with approve. No design call is involved: this is a defect against the workflow's own stated contract.

Three things found and not charged to the author. workflow_run.head_sha genuinely is the PR head, so the deleted ^[0-9a-f]{7,40}$ check really is redundant. Eighteen check-runs ran on this head, but nothing in CI exercises deploy-preview.yml — it only runs post-merge on workflow_run — so green here proves nothing about the change, on the very files that configure CI. And the concurrency group already keys on head_branch at :35, which is both available at group time and the value the correct fix needs; the payload already carries what the lookup wants.

Not verified: whether the workflow's GITHUB_TOKEN sees a different PR-association index than a user token — very unlikely on a public repo where the commit object resolves for both, but it could not be run from inside Actions — and the workflow itself was never executed, since running a fork's workflow change is exactly what the safety step forbids.

The review, as drafted

Thanks — the hardening instinct is right, and the hash half is correct.

The PR lookup doesn't survive a fork, though. commits/<sha>/pulls only indexes commits on a base-repo branch, so a fork head comes back [] — #5533 and #5515, two different forks, both empty, both with previews live right now. Thirteen of the last eighteen preview directories on gh-pages are fork PRs, and every one would take the new deploy=false path and finish green, so a contributor's preview link 404s with nothing saying why.

review-clear.yml:59 already wrote this down; :72 is the shape that works — resolve from head_repository.owner.login + head_branch, then compare head.sha for your staleness guard. Worth putting the reason in the body while you're there.

Does resolving by branch still give you the guard you were after? If you'd rather talk it through with someone, we're in Discord.

Inline: .github/workflows/deploy-preview.yml:81 — Returns [] for a fork head; review-clear.yml:72 resolves by head_repository owner + head_branch instead. · :84 — This used to be a hard error. Now an unresolved PR is a green skip and nothing surfaces it.

Rounds

One review, two gate passes.

  • Gate 1 — failed on five counts, and two of them are the loop's chronic ones. The comment ran 160 words against a 150 cap, with every word over it in the warm open and in a sentence restating the author's own summary back to him. The closing line asked whether the artifact-controlled PR number had ever bitten him — a curiosity question in the position the ask should hold, so the author would answer the motivation and leave the block untouched. The optional third finding was rendered as that closing question, which is the loudest position in the text, so the comment's shape said the opposite of the finding's label. And the draft bucketed the author as a collaborator on the strength of 86 merged PRs while the permission API returns read, which is the call that decides whether a contributor with an objection has anywhere to take it.
  • Gate 2 — clean. 160 words became 139 by deletion rather than compression; the closing line is now the ask, with the sign-off after it; the third finding is a subordinate clause mid-paragraph while the two blocking ones hold the emphatic positions; and the credit is four words of information rather than eighteen of flattery. Both anchors were opened at head, the head was re-fetched and unmoved, and the fix is cited to a sibling workflow rather than invented.

Status

Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inlines. The presentation's merge trailer is a hold, and the reason is the finding itself — fork previews regress — rather than any repo gate. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5518) line goes in before the attribution.

Clone this wiki locally