-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5517
bhamodi · open, request changes drafted · reviewed at b4124d48505 · view on GitHub
Verdict: request changes — the shell hole is genuinely closed, and the replacement lookup resolves nothing on a fork
The review asks for one thing: resolve the PR number a way that works for a fork head, because GET /commits/{sha}/pulls matches only head branches in this repository and returns [] for every fork PR, so the analysis comment disappears from the PRs the workflow was written to serve. It did not merge because that is a visible regression for the majority of open PRs, and nothing on the PR page would say it had happened.
The PR Analysis Report job runs on workflow_run with pull-requests: write and built a shell command line out of strings it read back from the pr-analysis artifact. JSON.stringify quotes for JSON, not for sh, so $(…) inside a value survives into the command. Verified in a real shell: execSync on https://evil.example/$(echo PWNED)/ delivers https://evil.example/PWNED/, while execFileSync delivers the literal string.
The person is a fork contributor. A pull_request run executes the PR's own workflow definition, so a fork PR that edits ci.yml can write anything into pr-meta.json and the privileged job then runs it — and for anyone with a merged PR, no maintainer approval is needed. That is command execution in the base repo's job, and the ability to rewrite the bot comment on any PR in the repo. The PR body describes the change and never says the old behaviour was exploitable; the header comment the diff adds does state the rule — "derived from the workflow_run context, never read from the artifact… invoked without a shell" — so the rationale is recorded in the file rather than the body.
(2 decisions · +34 −13, one file, all of it one embedded script)
- Resolve the PR number and the three preview URLs from
workflow_runrather than frompr-meta.json— the trust boundary. -
execSync(<string>)→execFileSync('node', args)— the shell.
The job used to ask the report artifact two questions: which PR is this, and where do its previews live. It now asks the event that woke it, and hands the generator its arguments directly rather than writing out a command line for a shell to re-parse. The artifact goes back to being report data only. Both decisions trace to the stated problem and they ship together — dropping the shell is only sufficient once the values stop coming from an untrusted place.
The derivation lives with its owner, the privileged reporting job in .github/workflows/pr-comment.yml. The defect is that the resolver it chose cannot see the one seam this workflow exists for; the workflow's own header says it was written because "the report never appeared on external-contributor PRs". Driven against the live API at this head rather than read off the source: a same-repo PR resolves (/commits/<sha>/pulls → [5538], [5268,5536], [5268,5535,5536]); a fork PR fails, with workflow_run.pull_requests empty and /commits/<sha>/pulls returning [] on #5517, #5518 and #5521; a docsite-only PR is unchanged, returning before the lookup; and a force-push mid-run is new behaviour — the stale run posts nothing rather than an outdated comment.
Decision 2 is the fix rather than a fix: pr-comment.yml:138 was the only shell interpolation in any workflow's inline script (git grep execSync -- .github/workflows returns that one line), and deploy-preview.yml already validates its two artifact fields with a regex, so the class is closed for the privileged jobs. The residue — analysis.json content reaching the comment body unescaped — is #5521's subject and already open.
Every fork PR loses its PR Analysis Report: no Storybook link, no Sandbox link, no bundle table, no a11y summary. An outside contributor opens a PR and the thing that tells them whether their change is fine never appears, with no signal that anything is missing, because the job exits on a warning nobody reads.
98 of the roughly 180 open PRs today are from forks — 34 from one account, 18, 17 and 17 from three more, and eight other accounts behind them. This PR is one of them, so on its own next push it would silence its own report. Same-repo PRs are unaffected, and nothing user-facing ships: no package, no runtime, no npm consumer.
What landing it newly exposes: pr-meta.json keeps storybookUrl and sandboxUrl fields that nothing reads any more, and the gh-pages URL layout is spelled in a second file.
No API change — a CI workflow, no package, no export, no prop, no type.
n/a — structural only, and not source at all. The style grep over the changed workflow returns 0, no token is touched, and no astryx-* class is added or renamed.
Nothing public is added, so nothing ossifies. Internal to the repo, the change writes the gh-pages preview URL layout into a second file (pr-comment.yml:110-112 alongside ci.yml:247-248), so the layout is now spelled twice with nothing holding the two together. The cheapest close is deleting storybookUrl and sandboxUrl from ci.yml's pr-meta.json, since after this PR nothing reads them.
- API — no. Nothing importable changes.
- Visual — no. Nothing renders, and the comment's text is byte-identical.
- Theme — no. No target, token or override is touched.
-
Behaviour — yes, and it is the review. A same-repo PR gets an identical comment. A fork PR goes from a posted comment to no comment at all — the job warns and returns. A docsite-only PR or one with no artifact keeps its quiet return. An artifact present without
pr-meta.jsonnow proceeds, since that file is no longer needed. And a force-push between CI ending and the job starting drops the stale comment instead of posting it, with the newer run posting instead.
Empty, loading, error, disabled and boundary states are not reachable — this is a workflow, it has no component states.
No React, so no effects. One network call is added — listPullRequestsAssociatedWithCommit, once per CI run, and only when workflow_run.pull_requests is empty, which is every fork run — in a job that already makes at least two REST calls. No listeners, no observers, no layout reads, no bundle change, no new dependency. Against that, one sh process per run is removed.
No frames, and the reason is that nothing here is renderable: a CI workflow, no geometry, no colour, no DOM.
The equivalent evidence is that the derived strings match what ships today, checked against the PR Analysis Report already posted on this PR — which main's copy of this workflow generated, so it is the real before:
| live comment today | new code derives | ||
|---|---|---|---|
| storybook | https://facebook.github.io/astryx/pr/5517/ |
https://${owner}.github.io/${repo}/pr/${prNumber}/ |
identical |
| sandbox | https://facebook.github.io/astryx/pr/5517/sandbox/ |
`${storybookUrl}sandbox/` |
identical |
| run | the CI run's own URL | run.html_url |
identical |
The fork-resolution failure was found by reading and confirmed by running, which is the second independent confirmation a block owes: /commits/<sha>/pulls returns [] for the heads of #5517, #5518 and #5521, and [5538] for a same-repo control that passes; the runs API reports pull_requests: 0; and ?head=<owner>:<branch> resolves the PR, which is the working form. The commit itself is present in the base repo — commits/<sha> resolves for all three — so this is not a missing object. The endpoint matches head branches in this repository, and a fork's branch is not one.
Nothing here is touched, and this is what was checked: aria-|role=|useTranslator|t('@astryx over the workflow → 0. No user-visible or AT-facing string is added, the comment's rendered text is unchanged, and nothing renders, so no direction question exists.
Adjacent and not this PR's: generate-pr-comment.js interpolates artifact fields raw into HTML — extLink at :57 drops a value straight into an href — which is fork-controlled content in a bot comment. That is #5521, by the same author, already open.
request changes. The stated goal is half met: the shell hole is genuinely closed, measured both ways, and the three derived URLs are byte-identical to what is on the PR right now. The other half — resolving the PR from the payload — does not work on the case this workflow exists for.
1. /commits/{sha}/pulls does not match a fork PR's head, and
workflow_run.pull_requests is empty there
→ every external contributor's PR stops getting its analysis comment,
and nothing tells anyone it is gone · pr-comment.yml:94
2. [not blocking] the gh-pages URL layout is now written in two files
→ whoever moves the preview path next leaves every PR comment
linking a 404 · pr-comment.yml:110
The worst sentence anywhere in the slots — "every fork PR loses its PR Analysis Report; an outside contributor opens a PR and the thing that tells them whether their change is fine never appears" — is not one a note can sit beside. No design call is involved: this is a defect in a CI change, measured against a contract the workflow's own header comment states.
Two things found and not spent on the author. The force-push behaviour change is accepted — the newer run posts within the minute, and #5518 makes the same trade deliberately for previews, so it is not worth a round trip. And #5518 carries the identical lookup, which is named in the comment as the PR that follows this one rather than promised as work.
Not verified: whether the Actions GITHUB_TOKEN sees the same empty result the review's user token saw on /commits/{sha}/pulls. The endpoint's behaviour is about branch ownership rather than visibility, and the same-repo control passed on the same token, but it could not be run as the Actions token.
Thanks for this — the shell hole was real, and
execFileSyncis the right fix.The PR lookup doesn't reach fork PRs.
GET /commits/{sha}/pullsonly matches head branches in this repo, so a fork head returns[]— I ran it against three open fork PRs including this one.workflow_run.pull_requestsis empty on forks too, so the job warns and returns: no analysis comment on any fork PR. That's 98 of the open ones today.Two ways out.
deploy-preview.yml:70keeps readingprNumberfrom the artifact and validates it with^[0-9]+$— digits can't reach a shell, and it stays exact. Or resolve it from the run's own fork and branch:head: `${run.head_repository.owner.login}:${run.head_branch}`#5518 has the same lookup and can follow this one. Which would you rather?
If you'd rather talk it through with someone, we're in Discord.
Inline: .github/workflows/pr-comment.yml:94 — Hmm, this returns [] for a fork head, and workflow_run.pull_requests is empty there too. ?head=owner:branch matches.
One review, two gate passes.
-
Gate 1 — failed on seven counts, every one of them the private brief leaking into the posted text. The inline was 24 words against a 20-word cap and read like the verdict rather than a pointer — a copy of the finding's consequence chain with a line number on it. The review offered "the safe version" without naming it, when the thing meant is a concrete regex on the artifact's
prNumber. A clause about the workflow's history was doing work the 98 number already carried. The comment named #5518 without saying which PR lands first, leaving the author to guess whether he was blocked on someone else's work. The force-push behaviour row never reached the judgement, so it was neither a finding nor explicitly not one. And the time slot was empty, which is a fail on its own. -
Gate 2 — clean. All seven answered: the inline is 15 words and in her register, the remedy is named rather than gestured at, the background clause is cut, the ordering with #5518 is stated, the force-push row is disposed of explicitly, and the time slot is filled. Both anchors were opened at the PR head, and the gate recorded one thing worth keeping for the loop: the stop-list hit on
.github/workflows/meant no install and no Storybook, and the evidence this review actually needed was live API calls and a four-line shell reproduction — three minutes of measurement against the thirty-five a worktree-and-build run costs.
Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inline. The presentation's merge trailer is a hold, and the hold is the finding itself rather than a repo gate — the fork lookup resolves nothing, so the review asks for a change before it can be an approve. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5517) line goes in before the attribution.
#5517 ci: resolve fork PRs by head repo and branch in pr-comment by bhamodi (bucket: contributor)
f10b579c8037240c51f103fd8ba57ac2ee8a4a5a — every claim below was verified at this commit.
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full
WHY: This changes a privileged workflow_run trust boundary, follows two standing changes-requested reviews, and contains two independent decisions. It is not fast-lane eligible.
WHY 1: Fork CI runs expose neither a populated workflow_run.pull_requests list nor an association through commits/{sha}/pulls, so the privileged reporter cannot identify the PR.
WHY 2: The report and trusted visual evidence never publish, leaving an accepted fork contributor with a green change that cannot clear the repository's review gate.
WHY 3: That blocks safe merge and release of otherwise accepted external contributions, including #5508, #5510, and #5516.
USER-FACING PROBLEM: A fork contributor whose change has passed review cannot get the trusted report that clears the repository gate, so their accepted PR remains unmergeable. PROBLEM SEVERITY: broken task — the accepted contribution cannot complete the merge path.
VERDICT: clear — the body states the failure, affected audience, and trust concern, and live API data reproduces it.
When GitHub does not attach a PR to the source CI run, the workflow asks the base repository for an open PR whose head owner and branch match that run. It then requires one candidate and checks the current head identity before any privileged work continues. Separately, report values are passed to the comment generator as process arguments rather than through a shell command string.
SOLUTION (2 decisions · 23 added / 13 removed workflow lines)
- Resolve an empty
run.pull_requestsfallback with the server-sidehead=<owner>:<branch>filter in both identity steps — fixes the stated fork-resolution failure. - Replace string-built
execSyncwithexecFileSync('node', args)— closes the stated shell-interpolation path.
BURDEN: low — one filtered REST request replaces one commit-association request on the fallback path; no new state, dependency, loop, or long-lived resource. BURDEN MATCH: proportionate — the two local changes repair the broken trust path without widening permissions or moving ownership.
VERDICT: clear
OWNER: the default-branch PR Comment workflow owns trusted run-to-PR identity and report publication.
TIER 1: GitHub Actions workflow_run payload plus GitHub Pulls API; existing visual-acceptance identity checks are preserved.
TIER 2: none.
SEAMS: fork and same-repo heads; same branch names under different owners; force-pushed or renamed heads; deleted forks; ambiguous candidates; more than 100 globally open PRs.
BEHAVIOR UNIT: inline — each identity step performs one lookup and a short fail-closed validation; the existing trusted workflow test guards the surrounding lock and mutation order.
| seam | driven result |
|---|---|
| Fork PRs #5508, #5510, #5516 | each CI run has pull_requests: []; commits/{sha}/pulls returns []; the owner-qualified head query returns exactly that PR with matching SHA/repo/base |
| Same-repo #5597 | CI run carries pull_requests: [5597], so the fallback is bypassed; the same head query also returns [5597]
|
| Same branch text under different owners |
jiunshinn:<branch> returns #5508; bhamodi:<branch> and facebook:<branch> return none — owner scope prevents cross-fork collision |
| Stale or wrong identity | exact SHA rejects a force-pushed head; the publishing resolver also rejects mismatched head repo and base repo |
| Ambiguous owner+branch | the exact-one guard rejects zero or multiple candidates before mutation; current 174-open-PR data has no ambiguous owner+branch pair |
| Renamed branch | the old name returns no candidate and fails closed; a new CI run carries the new name |
| Deleted fork | closed #4995 has head.repo: null, while its stored CI run retains the original head repository; the open-state query returns no candidate and fails closed |
| 174 open PRs (>100) | filtering is server-side; each tested head query returns one result on the first page, so global pagination does not truncate the candidate set |
| Current main |
#5587, #5588, #5591, #5592, and #5594 are ancestors of the PR parent; #5595 is newer, touches no pr-comment.yml, and git merge-tree reports a clean merge |
The implementation remains with the privileged workflow, uses the API as the identity source, and fails closed at every stale or ambiguous seam. The first resolver checks SHA after an owner+branch query against the base repo; the publishing resolver additionally repeats explicit head-repo and base-repo checks before write permissions are used.
VERDICT: clear
The change reaches fork contributors: 112 of 174 currently open PRs are fork heads. For #5508, #5510, and #5516, it restores the report and trusted visual-evidence path that currently stops at “found none.” Same-repo contributors keep the populated run.pull_requests path and see no behavior change.
VERDICT: clear
No package, component, prop, type, export, default, or caller-facing API changes.
OSSIFICATION: none — the workflow changes an internal repository trust path only.
VERDICT: clear
No target, token, variable, style, class, or rendered element changes.
VERDICT: clear
BEHAVIOR: intended fix — fork runs go from no report to one report; same-repo runs keep their existing path; stale, renamed, deleted, and ambiguous identities fail closed rather than mutating another PR. API: no — nothing importable changes. VISUAL: no — the changed file is CI configuration and renders no product UI. THEME: no — no target, token, or override changes.
VERDICT: clear
EFFECTS: zero; this is not React. RENDER: no render work. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or shipped bundle change. NETWORK/PROCESS: one server-filtered Pulls API call replaces the commit-association fallback; the generator still starts one Node child, but no shell process or shell re-parsing remains.
VERDICT: clear
VISUAL CHECK: not applicable
WHY: the sole changed path is .github/workflows/pr-comment.yml; it changes identity lookup and process invocation, with no DOM, CSS, geometry, color, or product rendering.
VERDICT: clear
No user-visible or assistive-technology string, role, ARIA state, focus path, locale data, direction rule, or rendered surface changes.
VERDICT: clear
| 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 — three live fork CI runs expose empty PR associations, their commit-association endpoint returns none, and the new owner-qualified head query returns exactly the matching open PR; the argv probe preserves a shell payload literally with no side effect. DISPOSITION: both prior blockers are resolved; no negative finding remains. ADVICE: omitted — no unaddressed defect or cited alternative remains. AUTHOR CAN PROCEED: yes — the exact head is ready for approval without another code change. WORST OUTCOME: none found.
PRIOR REVIEW: cixzhang's first review asked for a lookup that reaches fork heads; the second asked to merge main, preserve its trusted visual path, retain exact identity checks, and use execFileSync. This head satisfies each ask rather than contradicting either review.
JUDGEMENT NEEDED: none — this restores an existing CI contract and adds no public concept or design choice.
SEMANTIC VERDICT: approve
No findings.
Thanks — this resolves the fork lookup and shell invocation I asked for. The exact-head checks and main’s visual path are preserved. Good to merge.
[Reviewed by Robohands]
None.
- No duplicate branch name across different fork owners appears among the 174 currently open PRs; the live owner-scope probe still proves those names cannot collide.
- A deleted-fork PR supplies no open candidate, while the historical CI run retains its original head-repository record.
- The first identity step has no redundant explicit repo/base check; its API endpoint and owner-qualified head filter already establish both before the exact-SHA check.
-
actionlint -no-color -ignore 'label "2-core-ubuntu-arm" is unknown' .github/workflows/pr-comment.yml— PASS. The ignored custom-runner diagnostic is pre-existing and unrelated. -
node node_modules/vitest/vitest.mjs run .github/scripts/visual-gate/workflow-concurrency.test.mjs --reporter=verbose— PASS, 1 file / 7 tests. - Extracted all six
actions/github-scriptbodies, wrapped them only for parsing, and rannode --check— PASS, 6/6. - Independent
execFileSync(process.execPath, argv)shell-payload probe — literal argument preserved; shell side effect absent. - Live GitHub REST checks on #5508, #5510, #5516, same-repo #5597, and deleted-fork #4995.
-
git merge-tree --write-tree origin/main f10b579c8037240c51f103fd8ba57ac2ee8a4a5a— clean. - Exact-head GitHub CI —
testPASS (600 files, 12,629 tests; 5 skipped),lintPASS,buildPASS.
TIME total 26m setup 6m dedicated trusted-base worktree + exact-head workflow materialization; warm dependencies reused reading 7m current kit, prior reviews, PR body, full workflow, and main drift measuring 8m live APIs, merge compatibility, actionlint, syntax, focused workflow test, argv probe writing 5m draft, R1–R15 critique, rewrite, second critique, wiki record waste 1m first actionlint run omitted the repository's custom runner-label exception; PyYAML was unavailable
- The privileged workflow itself was not executed from the fork head; the safety gate permits static checks and trusted tests, not running changed fork workflow code with repository credentials.
- No currently open PR has a deleted head repository or a renamed branch mid-run; those paths were checked fail-closed with a historical deleted-fork record and a missing old-name query.
Thanks — this resolves the fork lookup and shell invocation I asked for. I verified #5508, #5510, and #5516 each resolve exactly once; stale SHA and repository mismatches fail closed; 174 open PRs do not affect the server-side filter; and main merges cleanly. Good to merge.
[Reviewed by Robohands]
VERDICT CHECK: correct — contributor, high-risk trust change, and both prior blockers are resolved. LENGTH: 46 words before attribution → FAIL; approve cap is 30. WOULD SHE HAVE POSTED IT: no — the evidence belongs in the private presentation, not the approval.
| rule | critique |
|---|---|
| R1 | PASS — contributor bucket and higher trust risk are reflected; approval follows resolved blockers. |
| R2 | FAIL — 43 words exceeds the 30-word approve cap. |
| R3 | PASS — no request-changes scaffolding or extra findings. |
| R4 | PASS — it leads with the resolved problem, not an ask. |
| R5 | PASS — “fork lookup” names the affected class, not one implementation line. |
| R6 | PASS — the three PRs are evidence for the class, not rationale for a new system. |
| R7 | PASS — no unsupported consolidation advice. |
| R8 | PASS — duplication is not promoted into a finding. |
| R9 | PASS — no prerequisite refactor gates the fix. |
| R10 | PASS — no API surface exists. |
| R11 | PASS — merge compatibility is established and no conflict sentence is needed. |
| R12 | FAIL — the semicolon-heavy evidence list reads like a report, not Cindy's short approving voice. |
| R13 | PASS — no inherited debt is charged to the contributor. |
| R14 | PASS — every factual clause is backed by exact-head API or merge evidence. |
| R15 | PASS — no visual claim is made; the presentation records why visuals do not apply. |
VIOLATIONS:
- R2 — “I verified #5508, #5510, and #5516…” pushes the approval to 43 words.
- R12 — “stale SHA and repository mismatches… 174 open PRs… main merges cleanly” is private evidence, not approval voice.
REWRITE:
Thanks — this resolves the fork lookup and shell invocation I asked for. The exact-head checks and main’s visual path are preserved. Good to merge.
[Reviewed by Robohands]
VERDICT CHECK: correct — semantic approve. LENGTH: 25 words before attribution → PASS. VIOLATIONS: none. WOULD SHE HAVE POSTED IT: yes — it names the prior asks, says what remained intact, and ends with the action.
| rule | critique |
|---|---|
| R1 | PASS — contributor default and higher-risk evidence support approval. |
| R2 | PASS — 24 words, below the 30-word cap. |
| R3 | PASS — no request-changes content survives. |
| R4 | PASS — resolved problems come before verdict. |
| R5 | PASS — names the fork-lookup class. |
| R6 | PASS — no one-instance system claim. |
| R7 | PASS — no consolidation advice. |
| R8 | PASS — no duplication nit. |
| R9 | PASS — no prerequisite refactor. |
| R10 | PASS — no public API. |
| R11 | PASS — no conflict remains. |
| R12 | PASS — warm, plain, short, and decisive. |
| R13 | PASS — no inherited debt. |
| R14 | PASS — exact-head claims are verified. |
| R15 | PASS — no visible claim; visual evidence is correctly not applicable. |
The first draft overspent exact evidence in the public approval. Keeping API counts and edge-case proof in the presentation leaves the review short without weakening the judgement.
Round 3 drafted at the exact head. No review, comment, inline, branch push, or merge action was performed on facebook/astryx.