-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5520
bhamodi · open, approve drafted and held · reviewed at c5181e640e2 · view on GitHub
Verdict: approve with two nits — held, not for anything in the review
The review would take this: the run-id check is the valuable half and it fails closed, and neither nit is unlivable. It did not merge because the workflow half has no CI signal anywhere — visual-baseline.yml is dispatch-only, nothing in the repo lints workflow YAML, and the visual job skipped — and it sits in the release path, so it wants one human read of the YAML before it goes in.
This is the honest "prevents a defect nobody has hit yet" case, and the PR says so rather than dressing it as a vulnerability. The person is whoever dispatches Visual Baseline — a maintainer, or the release cut. They type a Release Gate run id into a form, and today nothing checks it, so a mistyped id that happens to name some other run promotes that run's screenshots into the baseline, and every future gate run then compares against pictures nobody reviewed. The smaller half: the shot keys they type are joined straight into shots/<key>.png paths, and the strings they type are interpolated into a shell.
Straight about the size of it: only someone with write access can dispatch this workflow, so this is operator error and defence in depth rather than an exploitable hole. The run-id half is the part with a real failure story behind it.
What it catches on main today is nothing, by construction. shotKey() (plan.mjs:397-399) replaces everything outside [a-zA-Z0-9._-] with _ at the moment a key is created, so no key the system generates can violate the new rule — run against adversarial input, storyId: '../../evil' becomes .._.._evil__.-., which passes. That is the correct shape for a guard on a trust boundary, and it is why the honest framing is prevention.
(3 decisions · ~39 lines, all runtime)
- The dispatched run id must name a completed Release Gate run on
main(visual-baseline.yml:62-79). - Shot keys must match
shotKey()'s alphabet before being joined to a path (baseline.mjs:104-108). - Dispatch inputs reach the push and summary shells through
env:rather than${{ }}(visual-baseline.yml:114-117,139-142).
Promoting a screenshot into the baseline is the moment a picture becomes the truth everything else is measured against, so the job now refuses to start from anything it cannot vouch for. Before it downloads anything it asks GitHub what the run id it was given actually is, and stops unless that run is the daily gate, on main, and finished. Then, before any screenshot name becomes a filename, it checks the name looks like a name and not a path.
Three decisions is over the usual threshold and a split would be wrong here: they are one hardening of one job, each a few lines, and shipping 1 without 3 would leave the job half-done in a way nobody would choose. Decision 3 is the only one the summary paragraph does not name — a one-line body edit, not a PR.
The key check is in the right place, and that is worth saying rather than assuming. accept() is the only thing that ever writes the baseline manifest — the file's own docblock claims it (baseline.mjs:12-13) and the claim was checked: the only importers are gate.mjs:35 and baseline.test.mjs:9, and every other consumer of a shot key (compare.mjs:99-119, gate.mjs:176-177) reads keys out of a manifest rather than from input. Validating at the single write point covers every reader, now and later. The other place a key becomes a filename is capture.mjs:388, and those keys come from shotKey(), sanitised at generation — a different shape, so the class is closed rather than patched.
The run-id check is likewise complete for what it can reach. release-gate.yml triggers on schedule and workflow_dispatch only, so no fork can produce a run of it and head_branch alone is sufficient — the missing head_repository comparison is not a hole. It checks status, not conclusion, which is right: a gate run that found changes exits 2 and concludes failure, and that is exactly the run you promote from.
Nobody sees anything. No component, no CSS, no published package: accept() lives under .github/ and is imported by exactly two files, both in the repo's own CI. The people it reaches are the handful who dispatch Visual Baseline, and what changes for them is that the job now refuses two inputs it used to accept — one of which they will type by accident.
Landing it newly exposes nothing; the run-id check makes a class of silent operator error loud, which is the point.
No API change. Nothing here is exported from a package, published to npm, or reachable from a barrel — accept() has two importers, and there is no package.json under .github/.
n/a — structural only, no styles. The style grep over both changed files returns one hit, and it is a PR number inside a YAML comment. No new theme targets, none removed, no token touched.
The new internal surface is one regex constant inside accept(). It commits the repo to [a-zA-Z0-9._-] as the shot-key alphabet, which shotKey() already enforced at generation — so it writes down an invariant that already held rather than adding one. Cheap to change: two files, no consumers.
No consumer can be broken: this is not a package. .github/scripts/visual-gate is not published and is not private-flagged either — it has no package.json at all, and its only entry point is gate.mjs, run by CI.
-
API — no. Nothing exported, nothing renamed, no signature moved;
accept()'s parameters are unchanged. - Visual — no. Nothing renders; neither changed file produces markup.
- Theme — no. No target, token or override is touched.
-
Behaviour — yes, one.
accept()used to skip a key it could not find (baseline.mjs:116,if (!fs.existsSync(source)) continue;) and now throws on a key that does not match the alphabet, aborting the whole promotion.
The reachable states were walked: --keys all, where keys come from the capture manifest and are all shotKey() output; an explicit comma list, which is the finding; and --prune, where keys come from verdict.removed and are baseline manifest keys, also shotKey() output. Empty, loading, error, disabled and boundary states are not reachable — no component, no state, no default moved.
Zero effects: no React in this diff at all — no effect, no subscription, no listener, no observer, no layout read. Two costs, counted rather than described: one extra HTTPS round trip per promotion (visual-baseline.yml:72), on a dispatch job with a 20-minute timeout that runs a handful of times a month; and one regex test per key, bounded by the number of shots in the capture — 614 at the largest tier (sources.mjs:47), so at worst 614 tests against short strings, once. CI pays, in a job nobody is waiting on. No wall clock was measured and no reproduction banked, because there is no number worth re-running.
No frames, and the reason is that nothing here is renderable: one GitHub Actions workflow and one Node CLI helper, neither of which produces markup. What was checked instead of looking: both changed files against the style and markup greps, and CI's own visual job, which skipped on this head — correctly, because no component changed. The visual breaking axis is "no", so the two answers agree.
n/a — nothing rendered, no strings. aria-|role=|useTranslator|t('@astryx over both changed files → 0. Not auto-covered and not looked at either, because there is nothing to look at: the a11y and RTL jobs both skipped on this head, which is the right outcome rather than a gap. The one new user-facing string is a CI error message (visual-baseline.yml:78), which is workflow log output, and every existing message in this workflow is the same.
approve, with two nits, neither of them unlivable.
1. A hand-typed `a, b` in the dispatch form now fails the whole promotion
→ the person promoting two shots during the release window gets
`Invalid shot key: " core-badge--solid__y2k-light"` and has to work
out that it means a space. `requested.split(',')` at gate.mjs:463
does not trim, and the leading-space key fails the new test.
One `.map(k => k.trim())` removes it entirely. · baseline.mjs:105
Not a regression: on main that same input silently promotes one shot
of two and reports success, which is worse. Loud beats silent — but
correct beats loud, and it is one line.
2. The new guard has no test, in a file that already tests this function
→ the next refactor of accept() can delete it and nothing goes red.
baseline.test.mjs:43 already has a describe('accept') with the
harness built; the visual job skips on this PR, so that unit test is
the only thing that will ever exercise this code. · baseline.mjs:104
No design call is involved: this is defect prevention against the existing contract, with no capability, prop, export or theme target added. There was no prior review to extend or contradict.
Three things found and not spent on the author. Vercel is red here and on five of this author's PRs while green on four others, so it is the fork rather than this change — worth knowing before anyone sends him after it. The ubuntu-slim runner does have gh and jq: 17 workflows use it, and two existing workflows already call them there, which killed a finding mid-sentence. And ${{ github.token }} is still interpolated into a shell one step above the ones this PR fixes — pre-existing, not operator-typed, not injectable, and his only if he wants it.
Not verified: the workflow half cannot be executed anywhere but production. visual-baseline.yml is dispatch-only, nothing in CI lints workflow YAML — no actionlint, no yamllint anywhere in .github/workflows or package.json — and the visual job skipped. Every piece was verified by reading: API field names against the runs schema, gh and jq availability by precedent, step ordering, and actions: read already granted. But nobody will know it runs until someone dispatches it, and that is what the hold is for.
Thanks, this is good — the run-id check is the valuable one and it fails closed. Two nits inline.
Inline: .github/scripts/visual-gate/lib/baseline.mjs:105 — a, b from the dispatch form throws on the space. Might need a trim first. · :104 — baseline.test.mjs already has an accept block — worth a case for this.
One review, two gate passes.
-
Gate 1 — failed on one thing, and it was in the posted text. The merge trailer had been written inside the quoted comment, below the attribution: a thirty-word internal deliberation about whether the loop should merge an outside contributor's work unattended, addressed to nobody, in a public comment on
facebook/astryx. The loop's own vocabulary went with it. Everything the review is for — the trim trap, the placement of the guard, the honest "this catches nothing today" — survived the draft untouched. -
Gate 2 — clean. The trailer moved out of the quoted block, leaving the comment as the summary plus the attribution and nothing else. Every anchor was opened at the PR head; the "catches nothing today" count was moved up to where a reader looks for it; the Vercel claim was checked in both directions before being stated, five of his against four of everyone else's; and green CI was not taken at face value — the existing
accepttests genuinely ran, while the visual, a11y and RTL jobs are recorded as skipped rather than as passes.
Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inlines. The hold is not a doubt about the change and there is nothing for the author to do — it is that the workflow half is unexecutable outside production and unlinted anywhere, in a file that sits in the release path, so it wants one human read of the YAML. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5520) line goes in before the attribution.