-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5699
#5699 fix(ci): accept tall visual regression sheets by cixzhang (bucket: the maintainer)
dbc8a3a2d93a484f7a54e2733d879d98fd8c5a50
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.15.1
LANE: fast WHY: one small CI validation decision, no package API, no rendered component output, no new dependency, no workflow permission change, and one focused publisher suite plus adversarial PNG fixtures settle the goal. PRIOR REVIEW: checked; no human, Copilot, or inline reviews existed before this pass.
WHY 1: The trusted visual publisher rejected full-page visual sheets taller than 5000px, including #5671's 1024×7673 output.
WHY 2: A maintainer then cannot inspect or accept the actual changed pixels even though the image is a legitimate screenshot sheet.
WHY 3: Visual acceptance loses trust if the evidence pipeline drops valid output while still needing to reject malformed or resource-hostile PNGs.
USER-FACING PROBLEM: A maintainer reviewing a tall full-page visual regression sheet gets blocked by infrastructure instead of seeing the evidence needed to judge the PR.
PROBLEM SEVERITY: broken task — #5671 produced a legitimate 1024×7673 sheet that the old edge-only limit rejected.
NEW FEATURE CASE: not a new feature.
EARLY STOP: clear — this is maintainer-owned CI infrastructure fixing a concrete visual-evidence failure.
VERDICT: clear
The publisher now separates width, height, and total-pixel limits instead of using one square edge cap. It reads the PNG signature and IHDR dimensions before decoding, rejects impossible or too-large dimensions, then still canonicalizes the PNG and verifies the decoded dimensions match the header before publishing trusted bytes. That allows tall narrow sheets while preserving the old 25M-pixel memory budget.
SOLUTION (1 decision · 23 runtime lines of 31 added lines)
- Replace the single 5000px edge cap with
5000width,10000height, and25,000,000total pixels, plus pre-decode IHDR validation — serves the tall-sheet evidence problem while preserving the previous pixel budget.
BURDEN: low — one pure helper, no state, listeners, schemas, dependencies, workflow permissions, or public surface. BURDEN MATCH: proportionate — the trust boundary is already centralized in the trusted publisher's PNG copy/canonicalization path.
VERDICT: clear
OWNER: the trusted visual publisher owns evidence-file validation before copying into publishable output.
TIER 1: existing capture identity, scope validation, manifest key canonicalization, regular-file/symlink rejection, pngjs canonicalization, comparison, report rendering, and acceptance flow are reused.
TIER 2: none.
SEAMS: PR-produced PNG bytes, manifest metadata, baseline PNG bytes, canonicalized output, changed/added/removed evidence, and acceptance promotion.
BEHAVIOR UNIT: pure utility — pngDimensions(bytes, label) validates the header dimensions and is exercised through the existing publisher command.
COMPLEXITY BUDGET: 1 runtime decision, 1 validation owner, 0 new durable representations, 0 new modules, 0 new workflows.
ACTUAL BURDEN: 2 changed files; ~23 runtime lines; 2 focused tests; no new owners/modules/schemas/workflows.
BURDEN TREND: first reviewed head → current head: flat; one head reviewed.
RESET TRIGGER: not triggered — one local policy split, no second representation of image truth.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| trusted PNG dimensions | canonical PNG bytes after header preflight and decode agreement | evidence manifest width/height, diff canvas size, report output | none |
| resource budget | publisher constants: 12MiB compressed bytes, 5000 width, 10000 height, 25M pixels | focused rejection tests | none |
| visual verdict authority | default-branch trusted publisher recomputes comparison from canonicalized bytes |
evidence.json, verdict.json, acceptance records |
PR-authored verdict.json remains ignored |
| seam | driven result |
|---|---|
| #5671-sized sheet | accepted 1024×7673 and recorded the same dimensions in trusted evidence |
| committed tall regression | accepted 2×7673 and recorded the same dimensions in trusted evidence |
| invalid PNG header | rejected before it could become evidence |
| zero width | rejected as invalid dimensions |
| width over limit | forged 5001×1 header rejected |
| height over limit | forged 1×10001 header rejected |
| pixel budget | forged 5000×5001 header rejected |
| decode/canonicalization | existing canonicalization test still proves raw PNG bytes are rewritten, hashes update to the canonical bytes, and promotion matches canonical bytes |
| PR-authored verdict | existing test still proves a forged pass verdict is ignored and the trusted changed verdict is recomputed |
VERDICT: clear
Maintainers can review and accept legitimate tall full-page visual sheets instead of being blocked by the old square cap. Malformed, over-wide, over-tall, over-budget, zero-dimension, symlink, traversal-shaped, and identity-mismatched inputs still fail closed. NEW FEATURE IMPACT: not a new feature.
VERDICT: clear
No public package API, CLI API, exported type, component prop, or consumer-visible surface changed. The internal evidence contract now accepts a taller-but-not-larger PNG shape.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
trusted PNG validation splits width/height/pixel limits | internal CI contract | visual evidence validation | covered by focused tests | ok |
OSSIFICATION: internal helper only; the class is existing trusted visual evidence. The cost of being wrong is either rejecting valid review evidence or accepting too-large/malformed images, and both sides are covered.
VERDICT: clear
No styles, theme targets, tokens, variables, or rendered component contracts changed.
VERDICT: clear
BEHAVIOR: intentionally less restrictive for tall sheets that remain under the pixel budget; stricter for forged headers that exceed width/height/pixel bounds before decode. API: no public API or consumer call-site change. VISUAL: no component pixels change; only visual-evidence acceptance changes. THEME: no theme surface change.
VERDICT: clear
EFFECTS: zero.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
| none | n/a | n/a | no render path | no listeners/resources | publisher suite and adversarial fixtures |
RENDER: no React/render path.
LISTENERS/OBSERVERS: none.
LAYOUT: none.
BUNDLE: no dependency or shipped package change.
RESOURCE LIMITS: maximum compressed file size remains 12MiB; maximum decoded pixel budget remains 25M pixels, matching the previous 5000×5000 allowance. JavaScript multiplication cannot overflow in the accepted branch because width and height are each bounded before the product can exceed a safe integer range.
VERDICT: clear
VISUAL CHECK: not applicable WHY: both changed files are Node CI publisher validation and tests; no component, Storybook, DOM, style, theme, or user-facing rendered pixel changed. The required evidence is the trusted publisher's exact-head behavior: it accepts valid tall PNG sheets, canonicalizes them, recomputes verdict/evidence, and rejects invalid inputs.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
No roles, ARIA, keyboard/focus behavior, strings, locale formatting, direction, or rendered surface changed. Exact-head pr-a11y and pr-rtl correctly skipped because this PR does not touch components.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear |
GOAL: met — legitimate tall visual sheets are accepted without relaxing the existing pixel budget or PNG trust boundary. DISPOSITION: no blockers found; because the PR is draft and self-authored, public delivery is a semantic approval comment rather than a formal approval review. ADVICE: none. AUTHOR CAN PROCEED: yes. WORST OUTCOME: a malformed or too-large image becoming trusted evidence — mitigated by header preflight, byte limit, decode/canonicalization, dimension agreement, and focused rejection checks.
JUDGEMENT NEEDED: none.
approve once undraft
Semantic verdict: approve once undraft.
🟢 No review blockers found — this preserves the trusted visual-evidence limits while accepting legitimate tall sheets. Triage: CI bug fix · non-breaking · low blast radius → fast path · checks: PNG header/dimension preflight, pixel budget arithmetic/overflow, decode safety, canonicalization, and evidence metadata.
I checked exact head dbc8a3a2d93a484f7a54e2733d879d98fd8c5a50: the focused publisher suite passes, the #5671-sized 1024×7673 sheet now publishes as trusted evidence, and forged over-width, over-height, over-pixel-budget, zero-width, and non-PNG inputs reject before they can become trusted output. CI is green. Full evidence: https://github.com/cixzhang/astryx/wiki/Review-5699
Because this PR is still draft and self-authored, GitHub may not accept a formal approval event; this is the semantic approval for the reviewed head.
[Reviewed by Agentcloud]
None.
- Exact-head focused publisher suite:
npx --yes pnpm@11.10.0 exec vitest run .github/scripts/visual-gate/publish-pr-report.test.mjs --reporter=verbose→ 1 file, 11 tests passed. - Adversarial exact-head fixture results:
1024×7673accepted and recorded as changed evidence;2×7673accepted; forged5001×1,1×10001,5000×5001,0×1, and invalid-header inputs rejected. - Static/delivery checks:
git diff --check origin/main...HEADpassed; all exact-head CI workflows completed success, with component-only a11y/RTL/stable visual jobs skipped as expected for this CI-only diff;review-requiredandvisual-acceptancestatuses are success. - Code references: header/dimension preflight and decode agreement are in
publish-pr-report.mjslines 50–112; tall and over-budget regressions are inpublish-pr-report.test.mjslines 363–382.
TIME total 15m setup 3m claim, exact-head worktree, current rules, untrusted-diff check reading 4m diff, trust boundary, publisher flow, canonicalization, prior reviews, #5671 context measuring 5m focused suite, adversarial PNG fixtures, CI/status inspection, diff check writing 3m review, self-critic pass, wiki and GitHub delivery waste 1m cleaned a logging bug in the first adversarial probe output
- No live gh-pages visual acceptance mutation was performed; publisher behavior was exercised through isolated fixture directories, and CI had already reported success for the exact head.
The critic pass tightened the public comment to name the semantic verdict first, keep the draft/self-authored limitation explicit, and avoid exposing local paths or private review labels.