-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5518
bhamodi · open, approval drafted (not posted) · latest reviewed at c7f86b736ec70f27e73ffed63f40340744d98087 · 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.
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.
(3 decisions, effectively 2 · +27 −16, one file, all runtime)
-
SHORT_HASHfromworkflow_run.head_sha[0:7]instead of from the artifact (:76). -
PR_NUMBERfromcommits/<head_sha>/pulls, filtered to an exacthead.shamatch (:81). - An unresolvable PR stops being
exit 1and 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.
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.
No API change — a CI workflow. No prop, export, type, default or theme target.
n/a — CI YAML only. The style grep over the changed workflow returns 0, and no target is added, removed or renamed.
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.
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 1and 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.
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.
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.
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.
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.
Thanks — the hardening instinct is right, and the hash half is correct.
The PR lookup doesn't survive a fork, though.
commits/<sha>/pullsonly 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 ongh-pagesare fork PRs, and every one would take the newdeploy=falsepath and finish green, so a contributor's preview link 404s with nothing saying why.
review-clear.yml:59already wrote this down;:72is the shape that works — resolve fromhead_repository.owner.login+head_branch, then comparehead.shafor 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.
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.
Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inlines. Fork previews would regress at that reviewed head. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5518) line goes in before the attribution.
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
Verdict: approve with one non-blocking PR-description nit. This was a read-only review; no GitHub review was posted.
The valid request-changes at 0fbe445b… asked for three things: resolve fork PRs from the run's head owner and branch, fail when built previews cannot be attributed, and retain the exact-SHA stale-run guard. Current head does all three. The exact CI run has pull_requests: [], owner bhamodi, branch ci/deploy-preview-pin-to-run, and head c7f86b7…; the workflow's owner:branch query returns #5518 at that same head, while commits/<sha>/pulls remains empty.
The later request-changes text on #5518 is not about this PR's code. It cites b4124d4…, says “analysis comment,” and asks for pr-comment.yml/generator changes; b4124d4… is #5517's old head, and the same review was posted to #5517 one second later. #5517 has since merged those fixes. It is reconciled here rather than treated as an open #5518 finding.
The privileged preview publisher previously trusted a PR number and artifact hash from an artifact produced by pull-request-controlled CI. A fork could therefore select another PR's preview directory, so a maintainer reviewing one change could be shown another build under a legitimate preview URL. This is preventative work: no incident was reported, but the reachable failure breaks the visual-review task.
VERDICT: clear.
The trusted job now takes its identity from GitHub's completed CI run, resolves the PR from the run's fork owner and branch, and requires the PR's current head to equal the run's exact SHA. It separates an unattributable built preview (red error) from a real PR that moved on after the run (clean stale-run skip). Two decisions, 38 additions and 14 deletions in one workflow; one REST lookup, no new dependency or persistent state.
VERDICT: clear.
Owner: deploy-preview.yml, the trusted post-CI publisher. Checked seams: fork and same-repo heads, force-pushed/advanced heads, missing analysis artifact, missing preview artifacts, and current main's separately landed analysis-comment/visual path. The exact run payload and live API query prove the fork seam now resolves; the exact-SHA comparison preserves stale-run safety.
VERDICT: clear.
Every PR preview uses this path after merge. Contributors retain working preview links, and maintainers can trust that a published preview belongs to the exact CI run and current PR head. No application or package consumer changes.
VERDICT: clear.
No public library API change. Existing workflow outputs remain pr_number, short_hash, and deploy, with unchanged consumers. Nothing new ossifies.
VERDICT: clear.
No theme, style, target, token, DOM, or component changes; the sole changed path is GitHub Actions YAML.
VERDICT: clear.
API: no. Visual: no. Theme: no. Behavior: intended hardening only—current fork and same-repo heads publish, a stale run skips, an unattributable built preview fails red, and missing artifacts retain their existing skips.
VERDICT: clear.
Zero effects. One owner:branch REST lookup replaces one commit-association lookup; there is no loop, new runner, dependency, listener, or build.
VERDICT: clear.
VISUAL CHECK: not applicable. The only changed file is workflow YAML; no rendered source, DOM, geometry, color, or visible endpoint changes. Exact-head visual status reports no stable visual scope, so no frames are owed.
VERDICT: clear.
REMEDY SEARCH: not triggered — no proven visual defect.
No accessibility or internationalization surface is reachable from the workflow-only diff. Component-scoped a11y, RTL, and visual jobs correctly skipped; no baseline, role, ARIA, locale, direction, or application string changed.
VERDICT: clear.
GOAL: met. The live owner:branch lookup resolves #5518 from the exact run payload, and publishing remains gated on equality with the run's head SHA.
All code findings from the applicable prior review are fixed. The PR body still describes the superseded commits-to-pulls lookup and a green unmatched-run skip; that is a non-blocking before-merge nit, not a reason to re-block a contributor after the code correction.
AUTHOR CAN PROCEED: yes. Update the Summary, Changes, and Test plan to describe owner:branch lookup, a hard error for unresolved built previews, and a clean skip only for a resolved stale head.
WORST OUTCOME: none found in the current code.
Event: APPROVE
Thanks — the fork lookup and unmatched-run failure are fixed, while the exact-SHA guard still holds. One nit: the body still describes the superseded lookup and green skip.
[Reviewed by Robohands]
No inline comments.
Gate 3: clean. The approval body is 26 words before attribution; every prior review was reconciled; the exact head, public-safety gate, non-rendering visual decision, and version pair were rechecked.
Exact-head CI, lint, CLI smoke, internal registry, build, test, and docsite checks completed successfully. Vercel failed on this head and on the old reviewed head; it is not a required status and this workflow-only diff does not touch the docsite. The changed workflow_run workflow cannot execute until it is on the default branch, so it was not run locally; its identity lookup was reproduced read-only against the exact run payload.
- Setup and rules: 2m
- Install/build/server: 0m
- Browser/a11y: 0m
- Focused tests: 0m
- Code/history analysis: 4m
- Critique and wiki publication: 3m
- CI wait: 0m
- Total active: 9m
- Waste: 1m, included above
Nothing was posted. This re-review was requested read-only.