-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5595
cixzhang · open · view on GitHub
Verdict: comment — semantic approve (exact-head review) · exact head 081cd498eb40b6b68d7ca8a4fb46caa60d3eb0e2 · do not merge automatically; visual-governance process change
The head moved between rounds. Gate 1 reviewed b07ae6b and requested changes on a
proven, reproduced block. At 09:27Z the author force-pushed 081cd49, which fixes that
block. Under R14d a finding the head has already fixed is deleted, and a verdict that
flips is a new draft rather than an edit — so this is a new draft at the new head, not a
critic rewrite of gate 1.
What carried over: nothing from gate 1's findings. The block is gone (re-verified below, two ways), and gate 1's own critic failures (R2h inline duplication, R16d dispositions without an owner) are corrected here.
Versions are unchanged across the head move (loop 1.5.0, rubric 1.13), so the gate-1 pair still applies.
#5595 fix(ci): retry visual promotion after preview cleanup by cixzhang (bucket: the maintainer)
081cd498eb40b6b68d7ca8a4fb46caa60d3eb0e2 (supersedes b07ae6b38e88b3c04b67679dd55d260facbc04b9, reviewed in gate 1)
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full
WHY: the diff changes a merge-time trust boundary, adds a new shared module, and carries two independent runtime decisions — three fast-lane conditions fail (no security/trust concern, no new architecture owner, one runtime decision). Never eligible.
WHY 1: a merged PR whose visual delta was explicitly accepted fails to promote its reviewed pixels into the baseline. Promotion re-derived "is this still the current visual result" from the ephemeral PR preview evidence at gh-pages:pr/<n>/visual/<head>/, and cleanup-previews.yml:143-149 deletes pr/<n> as soon as the PR is closed — it reconciles the on-disk pr/ tree against gh pr list on every CI completion plus a daily cron. The five-attempt push-retry widens the window: each attempt re-clones gh-pages, so cleanup only has to win once.
WHY 2: the baseline keeps the OLD pixels, so the next person to touch that story is shown a changed delta for pixels that were already reviewed and merged, and has to run /accept-visual again on someone else's approved change.
WHY 3: the gate exists so a human only looks at pixels that actually moved. A baseline that lags merges makes it a false-alarm generator, and a gate that cries wolf is a gate people accept without looking — the exact failure the explicit-acceptance design exists to prevent.
USER-FACING PROBLEM: a maintainer merges an accepted visual change; the baseline silently keeps the old frame, and the next contributor to touch that story is asked to approve a diff nobody introduced. PROBLEM SEVERITY: harmful friction — the merge completes and no data is lost, but approval work is repeated by a different person and the gate's signal degrades.
Live, not hypothetical. The three most recent failing promotion runs all died on exactly the line this PR removes:
##[error]Accepted evidence is no longer the latest visual result
runs 33054565987, 33054520691, 33054515447 — all 2026-08-27 08:36–08:40Z. Denominator: gh run list --workflow=visual-acceptance-promote.yml --limit 100 → 18 runs, 6 failed / 11 success / 1 skipped.
VERDICT: clear
A promotion asks two questions before it touches the baseline: is this decision still the current one, and do the merged pixels match the reviewed ones. It used to answer the first by re-reading throwaway preview files a merged PR no longer has. It now answers it from things that outlive the merge — the pointer saying which decision is current, the immutable decision itself, and GitHub's own answer for which CI attempt is latest — so a retry minutes after cleanup still knows what was approved. For the second question, both the picture a human approved and the picture taken after the merge are now re-encoded through one shared encoder with every setting pinned, so "the same picture" becomes a hash comparison instead of a pixel-by-pixel one, and the durable baseline stores that same canonical form.
SOLUTION (2 runtime decisions · ~98 runtime lines of 1102)
- Authorize each promotion attempt from the durable
current.jsonpointer and the immutable acceptance record, binding PR / head / run / attempt, and drop the ephemeral-evidence precondition —visual-acceptance-promote.yml:123-141[the fix; traces to stated problem 1] - Introduce one shared PNG canonicalizer with frozen encoder options, run both the publisher and the promoter through it, compare canonical hashes, and store canonical bytes in the baseline —
lib/canonical-png.mjs,publish-pr-report.mjs:78,visual-acceptance.mjs:723-758[traces to stated problem 2] - Reformat
visual-acceptance.mjsandpublish-pr-report.mjsto the repo's prettier style — 176 of 1102 changed lines [NO STATED PROBLEM]
The split is measured, not estimated. Prettier-normalising each base file with the repo's own config and diffing against head:
| file | changed | substantive | reflow |
|---|---|---|---|
visual-acceptance.mjs |
132 | 46 | 86 |
publish-pr-report.mjs |
96 | 6 | 90 |
Both base files are prettier-dirty and nothing enforces it — prettier is absent from CI and lint-staged covers only *.{ts,tsx,md} — so the reflow was optional. publish-pr-report.mjs is the one worth naming: a 96-line diff on the trust boundary that decides which bytes a human is shown, for six real lines. The other 828 lines are tests.
Early 7d hypothesis, written under SOLUTION before the remaining slots: everything I find will be a child of "the two questions were collapsed into one gate, and splitting them changed the equality notion for both." Round 1 confirmed it. Round 2 refutes it, and that is the more useful result: decision 2 no longer changes the equality notion, it makes both sides share one — so the findings that survive are not children of that choice at all. They are about how far the new encoder reaches.
BURDEN: low — no runtime state, no Effects, listeners, timers or observers, nothing shipped (.github/ is in no package's files array — 0 of 20 checked — and no packages/*/src file references visual-gate). The cost is one 29-line internal module and its test matrix, which the PR pays for.
BURDEN MATCH: proportionate.
VERDICT: note — decision 3 traces to no stated problem and hides a six-line change inside a ninety-six-line diff
OWNER: the visual-acceptance promotion job — visual-acceptance-promote.yml plus visual-acceptance.mjs promote. Not a component; no Astryx primitive owns this.
TIER 1: reuses the gh-pages record layout, authorization.mjs, gate.mjs capture, and now a shared lib/canonical-png.mjs.
TIER 2: none.
SEAMS: four independent writers/readers of one gh-pages tree — pr-comment.yml (publishes evidence), visual-acceptance.yml (records the decision, reconciles), cleanup-previews.yml (deletes previews), visual-baseline.yml (manual/release promotion).
BEHAVIOR UNIT: lib/canonical-png.mjs is the right unit and it is new. One exported function, options frozen in one place, and canonical-png.test.mjs tests the property that matters — two differently-encoded inputs (a deflateLevel: 0 / filterType: 0 encode carrying a gAMA chunk versus a default encode) converge to identical canonical bytes. The promotion step's shell is a testable unit too: visual-promotion-workflow.test.mjs:84-98 slices the run: block out of the YAML and drives it under bash with shimmed git/gh/sleep.
| seam | driven result |
|---|---|
cleanup deletes pr/<n> between push attempts |
works — race injected at the first push, retry succeeds, pr/42 absent from the final tree |
superseded current.json pointer |
fails closed — "newer visual acceptance superseded" |
| newer CI run for the same head | fails closed — "not from the latest completed CI attempt" |
| record identity disagreeing with its own path | fails closed — "record identity does not match this merged head" |
| tampered archived AFTER pixels | fails closed — "archived AFTER is missing or changed" |
| baseline moved under the decision | fails closed — "baseline conflict" |
| reviewed AFTER (publisher) vs post-merge shot (capture) | works — both canonicalize; verified with a real Chromium capture, see BREAKING |
| an acceptance already on gh-pages, recorded before this lands | works — verified, see BREAKING |
gate.mjs accept writing the same baseline |
does not canonicalize — finding 1 |
The removal of the state precondition is sound, and I checked each guarantee it carried rather than taking the PR's word: a newer evidence run is now caught by GitHub's own latest-CI-attempt answer instead of by whichever directory sorted highest, and "reviewed baseline is stale" is caught inside promote() by the preimage check (visual-acceptance.mjs:695-699) atomically with the push retry rather than one clone earlier. R3d — the next place the same failure can happen: the other two state callers are visual-acceptance.yml:307 and pr-comment.yml:469, both pre-merge, both reading evidence that is live when they read it. No second instance of that shape.
Note, not a block: workflowStepScript() finds its step by matching - name: and stripping exactly ten leading spaces. It throws loudly if the step name moves, but a re-indent of that block would dedent wrongly rather than fail.
VERDICT: note — the YAML-slicing harness is indentation-coupled
The people this reaches are Astryx's own maintainers and contributors. No end user of any app sees anything and no published package changes.
For a maintainer: an accepted visual change that merges now actually updates the baseline, including when the preview is deleted mid-retry. Six of the last eighteen promotions failed; that class stops failing. The next contributor to touch that story stops being asked to re-approve pixels somebody already approved.
What landing this newly exposes, and it is the honest half: the canonical form reaches only the keys a PR acceptance promotes. The live baseline holds 520 shots and all 8 recorded decisions came through the manual/release path (runId, no pr field) — so on the day this lands, 0 of 520 shots are canonical, and gate.mjs accept keeps writing raw bytes for the ones it promotes. Nobody is worse off than yesterday; the improvement is just much smaller than the diff suggests, and the mixed baseline is a new thing to reason about that was not there before.
VERDICT: note — the canonical form reaches 0 of 520 baseline shots on landing
No API change. Nothing exported from a package, no prop, no theme target, no type. .github/scripts/ is unreachable from any package barrel (grep -rn visual-gate packages/*/src packages/*/package.json → 0).
OSSIFICATION: one new internal surface, lib/canonical-png.mjs exporting canonicalizePng() and CANONICAL_PNG_OPTIONS. Class or single case: a class, with two landed members in this same diff — the publisher (publish-pr-report.mjs:78) and the promoter (visual-acceptance.mjs:727), which is exactly the two-member class R1g-surface asks for and the reason the module deserves to exist rather than being a local helper. What already exists and why it does not reach: nothing did — the two paths each had their own inline PNG.sync.write, which is the defect. Cost of being wrong: near zero. It is repo-internal, has two in-tree callers, ships in no package, and is deletable in one commit. CANONICAL_PNG_OPTIONS being frozen and asserted field-by-field in its own test is the right level of permanence for a value that decides durable bytes.
VERDICT: clear
STRUCTURAL ONLY — no styles, no strings, no roles.
grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle" <8 changed files> → 0
No theme target, token or variable is added, removed or renamed.
VERDICT: clear
BEHAVIOR: changed, and evidenced safe. Promotion's equality test moves from decoded-pixel comparison to canonical-hash comparison, and the baseline's stored representation moves from raw capture bytes to canonical bytes. Both were driven, not read.
Round 1's block was here and it is gone. At b07ae6b the check was recapturedSha256 !== entry.afterSha256 — raw Chromium bytes against a pngjs re-encode, which can never be equal. At 081cd49 both sides run through canonicalizePng() first.
Measured at this head, against warm main on :6100 with a real core-button--primary capture:
raw Chromium 2740 B 225554c9b46eef96
legacy publish 3356 B f1c5f89310ab773d
canonical publish 3356 B f1c5f89310ab773d
legacy === canonical true
idempotent true
1. NEW shape (AFTER canonical, recapture raw) : PROMOTE ok
baseline now holds canonical bytes: true
manifest sha matches the file : true
2. LEGACY acceptance already on gh-pages : PROMOTE ok
~/astryx/review-artifacts/pr-5595/probe/promote-canonical-recheck.cjs — REPO=<worktree> node promote-canonical-recheck.cjs. Two things it settles that reading could not:
-
Migration. An acceptance recorded on gh-pages before this lands has an
afterSha256written by the oldcopyPng. For a real capture the old default encode and the new pinned encode produce identical bytes, so in-flight acceptances still promote. This was the finding I expected to find and did not. -
Idempotency.
canonicalizePng(canonical) === canonical, which is what lets the baseline hold canonical bytes without drifting on re-promotion.
Control from round 1, same fixture shape, still on record: PROMOTE: ok at parent 1d94f85, PROMOTE: FAILED at b07ae6b, and now PROMOTE ok at 081cd49 — so the number moved in both directions for the right reason.
API: no — nothing exported changes. VISUAL: no. Nothing renders differently; the change is which bytes a CI job stores. THEME: no — grep above.
Consumer who can be broken: the repo's own merge pipeline. It is exercised on every accepted visual change, and the exercise passes.
VERDICT: clear — the equality change is evidenced safe in both directions, including for acceptances already on record
EFFECTS: zero. A Node CLI and a workflow; no React, no listener, no observer, no timer in the diff.
RENDER: no reachable change.
LISTENERS/OBSERVERS: none added or removed.
LAYOUT: none.
BUNDLE: no delta — .github/ is in no package's files array (0 of 20) and no packages/*/src file references visual-gate.
Two cost changes, both in once-per-merge or once-per-CI-run jobs, neither reaching a person. promote() now decodes and re-encodes each shot instead of decoding two; deflateLevel: 9 with deflateStrategy: 3 is the slower end of pngjs. Against that, compare.mjs:94's byte fast path becomes reachable for canonical keys, where today it can never hit — the baseline holds raw bytes and trustedManifest hashes the re-encoded copy, so 520 of 520 shots fall through to a full decode plus pixelmatch on every run. I did not measure either; the second is worth more than the first and finding 1 is what would collect it.
VERDICT: clear — zero Effects, nothing ships, and the two cost deltas sit in once-per-merge jobs; unmeasured and unreachable by any user
VISUAL CHECK: not applicable
WHY: genuinely non-rendering. All eight changed paths are .github/scripts/visual-gate/*.mjs and .github/workflows/*.yml; no shipped package imports them (grep -rn visual-gate packages/*/src packages/*/package.json → 0) and no package's files array includes .github (0 of 20). No CSS, no DOM, no element added or removed — the structural grep is 0 on both lines. The change alters which bytes are stored in a baseline, not what any component renders.
The claims in BREAKING are byte-level, not claims about anything a person would see, so their evidence is hashes and a driven promote at three revisions, all with banked reproductions. The frames from the encoder probe are at ~/astryx/review-artifacts/pr-5595/probe/{raw,reencoded}.png; they are deliberately pixel-identical and looking at them proves nothing the hashes do not.
VERDICT: clear — not applicable, with changed-path evidence
Nothing here is touched, and here is what I checked:
grep -nE "aria-|role=|useTranslator|t\('@astryx" <8 changed files> → 0
No rendered output, no user-visible or AT-visible string, no locale formatting, no direction-sensitive code. pr-a11y does not run on this diff and does not need to. The only human-read strings are the workflow's own error messages, which are CI log output.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | note — decision 3 traces to no stated problem; 6 real lines inside a 96-line diff |
| ARCHITECTURE | note — the YAML-slicing harness is indentation-coupled |
| IMPACT | note — the canonical form reaches 0 of 520 baseline shots on landing |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable, with changed-path evidence |
| A11Y & I18N | clear |
Reading the bodies rather than the labels. Three notes, and I checked whether they compound into something none of them is alone: they do not. SOLUTION's is about reviewability of this diff, ARCHITECTURE's about a test harness's brittleness, IMPACT's about reach. Only IMPACT's has a person on the end of it, and that person is no worse off than yesterday. Nothing in any slot body finishes the sentence "a user is stuck / cannot reach this / hears nothing / loses their place" — I looked for it specifically, because gate 1's block did finish that sentence and this head is where it stopped.
The one place a finding could be filed at the wrong level: IMPACT's note and PERFORMANCE's second cost delta are the same fact seen from two ends, and lib/baseline.mjs:109 is where it is caused. Filed there.
GOAL: met — the stated goal is that an accepted baseline stops going stale after preview cleanup. Driven: the race harness promotes successfully with cleanup winning the first push, and the real promote command accepts a production-shaped recapture at this head (PROMOTE ok) where it refused at the previous one (PROMOTE FAILED) and at the parent it only passed because the pixel compare bridged the encoders. Both stated problems are answered and neither answer costs the other.
DISPOSITION:
-
gate.mjs accept/lib/baseline.mjs:109still writes raw bytes to the same baseline → accepted, non-blocking. Who pays: nobody today — the gate is exactly as fast as yesterday for those 520 keys and no verdict changes, becausecompare.mjsfalls through to pixelmatch and correctly reports unchanged. Why acceptable: it is a pre-existing shape this PR does not worsen, and completing it is a one-line change that wants its own PR rather than growing this one (R9 — never gate on a prerequisite refactor). Who decided: me, and it is inside my authority because no user-facing outcome moves and no public surface is involved. Named to the author as the review's one nit so the decision is visible rather than silent. - 176 lines of optional prettier reflow, 90 of them in
publish-pr-report.mjs→ accepted, non-blocking. Who pays: the next reviewer of that file and anyone runninggit blameon the publisher. Why acceptable: zero runtime effect, it moves both files to the repo's own configured style, and asking for a split now costs more than it saves on a fix that six failing promotions are waiting on. Who decided: me; it is a reviewability cost with no user on the end of it. Recorded rather than waved past. -
workflowStepScript()indentation coupling → accepted, non-blocking. Who pays: whoever re-indents that workflow step. Why acceptable: it throws loudly on the likelier failure (the step name moving), and the harness is new coverage that did not exist at all yesterday. Who decided: me. - Unclaimed improvement, recorded because an unclaimed change is an unreviewed one: making the baseline canonical is what makes
compare.mjs:94's byte fast path reachable at all. Nothing in the PR body says so, and it is the strongest argument for finishing the job inlib/baseline.mjs.
ADVICE: omitted as a remedy, given as a direction. I did not build the lib/baseline.mjs change and will not prescribe one. The direction is bounded: the same shared encoder already exists and has two callers, so a third at lib/baseline.mjs:109 would make the whole baseline canonical and light up the byte fast path for all 520 keys. Acceptance would be that a shot promoted through gate.mjs accept and a shot promoted through PR acceptance hash identically for identical pixels. That is a separate PR.
AUTHOR CAN PROCEED: yes — nothing blocks, and the one nit is optional with a stated acceptance criterion. No new owner, public API, concept, system boundary or accepted measured cost is being withheld.
WORST OUTCOME: "on the day this lands, 0 of 520 shots are canonical, and gate.mjs accept keeps writing raw bytes for the ones it promotes" → approve. That sentence describes an improvement that is smaller than it looks, not harm; nobody is worse off than before the PR, and it is compatible with approve.
JUDGEMENT NEEDED: none — a correctness fix and a CI hardening against the existing contract. No new public surface, no new concept, no design call. lib/canonical-png.mjs is internal with two landed callers in the same diff.
MERGE: hold(the author's own PR body says "Do not merge automatically. Visual-governance ownership must merge this process change." The review itself does not block.)
private_cleanup_needed: false — the public diff and body carry no internal URL, tracker id, hostname, unixname or private context.
approve and merge
- [not blocking]
gate.mjs acceptwrites raw capture bytes into the baseline this PR just taught to be canonical → the byte fast path in the comparison stays dark for the 520 shots that path owns, and the baseline becomes a mix of two representations ·.github/scripts/visual-gate/lib/baseline.mjs:109
Thanks — canonicalising both sides is right, and I saw a real capture promote. Nit: gate.mjs accept still writes raw bytes to the same baseline.
-
.github/scripts/visual-gate/lib/baseline.mjs:109— Still raw here, so the whole baseline stays mixed until this one goes canonical too.
- Acceptances already recorded on gh-pages still promote — the old default encode and the new pinned one produce identical bytes for a real capture. I expected a migration break and there isn't one.
- 520 baseline shots today, all 8 decisions through the manual path, so the canonical form reaches nothing on day one.
-
#5520 (bhamodi) is adjacent, not competing — it validates the manual baseline path, and its one shared file is
lib/baseline.mjs, which finding 1 also points at.
TIME total 24m
setup 3m two worktrees (b07ae6b, then 081cd49 after the force-push) + clone-install ×2 (warm main reused: yes, :6100)
reading 6m brief, critic, harness, 8 changed files, and the 5 workflows that share the gh-pages tree
measuring 7m 4 suite runs (head, reverted-source, control, new head) + 3 probes, 2 re-measures
writing 6m two full presentations, because the head moved mid-loop
waste 2m prettier-normalised the base file outside the repo, so it used stock config and reported 389 "substantive" lines instead of 23; then repeated the same mistake with a .basetmp extension prettier refuses to parse, which silently reported 0 reflow. Both redone inside the worktree with a real .mjs name.
- I did not run the promotion job on a GitHub runner. The canonicalization is proven locally against the same two code paths CI uses, but the exact bytes Chromium emits on
2-core-ubuntu-armwere not observed. - The legacy-acceptance migration is proven for one real capture, where the old default encode happened to equal the new pinned one. A capture carrying a gAMA chunk or a non-RGBA colour type would diverge, and I did not survey the 520 live shots for one.
- I did not confirm that all 6 recent promotion failures are the cleanup race; I opened 3 and all 3 were.
Graded blind against the rules, on the artifact above.
## #5595
VERDICT CHECK: correct — no slot says BLOCKS or [Needs human judgement], GOAL is met with
driven evidence, every negative fact has a disposition, and the author can
proceed. Approve is the right verdict and the private hold is the author's
own instruction, not a softened block. R14d was applied correctly: the
previous block was deleted rather than thinned, and the flip was written as
a new draft.
LENGTH: REVIEW 24 words → ok (cap 30). One inline, 15 words → ok (cap 15).
VIOLATIONS: none.
R2h — pass. The inline says what is wrong at that line and does not repeat the summary's
clause; the summary says "writes raw bytes to the same baseline", the inline says
the baseline stays mixed until it changes. Different jobs.
R16d — pass. Nothing in BREAKING is non-"no" except an evidenced-safe behaviour change,
and it reaches JUDGEMENT. All three notes appear in DISPOSITION with who pays, why
it is acceptable, and who decided — the omission gate 1 failed on.
R16e — pass. No [BLOCKS] line and the verdict is not request-changes. Consistent.
R18g — pass. Two unmeasured costs are named as unmeasured and neither carries an
adjective. No "negligible".
R15e / R39 — pass. The obligation decision is stated with changed-path evidence, and the
byte-level claims correctly do not pretend to be visual ones.
R2l — pass. The remedy is explicitly withheld and given as a bounded direction citing a
mechanism that already exists in the diff, not an invented patch.
R32 — pass. The class was verified rather than the case: migration, idempotency, and the
second baseline writer were each driven or counted.
R20 — pass. `waste` names two instrument errors specifically, including one repeated.
WOULD SHE HAVE POSTED IT: yes. 24 words, one nit with an anchor, no scorecard language, no
closing question on an approve, and the substance is a fix she can land.
REWRITE: none.
## PATTERN
Gate 1's failure (inlines written from the summary) does not recur. The failure worth
naming across both rounds is the reviewer's own instrument: twice a prettier
normalisation ran with the wrong config or the wrong file extension and produced a
confidently wrong line count. The single change that would prevent it is to make the
normalisation assert that it changed something before its output is used as a measurement.
GATE 2: PASS — no violations. Loop converged in two rounds.