-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5604
PR: #5604 · author cixzhang
Verdict: APPROVE — judgment only; no public PR action taken.
cc907ca26c97adf6be6c5010c216b671af588f8c
Pinned from the GitHub PR, fetched through refs/pull/5604/head, checked again against the remote ref before setup, and checked again before this record was written. Base: be5278b0eefd5d3340c2188da5ec2122f36e02e3.
LOOP VERSION: 1.5.0
AUDIT RUBRIC: 1.13
LANE: full
WHY: this changes a privileged workflow_run publisher, the immutable visual-evidence schema, and the required visual-acceptance status. Security/trust work is never fast-lane eligible.
WHY 1: accepted broad component work can exceed the 240-shot PR budget, then the trusted comment workflow expands the same broad scope into thousands of captures anyway. #5516 is the live case: its 17 component paths plus packages/core/src/utils/interactionOverlay.stylex.ts classify as broadStableVisual: true.
WHY 2: the maintainer waits for a browser recapture the PR policy already delegated, then the required visual-acceptance status can still fail after almost all frames were captured.
WHY 3: the required status is meant to prove the visual decision has a trusted owner. Repeating the release gate inside the PR workflow adds delay and failure surface without adding ownership: the daily gate remains the authority for broad stable changes.
USER-FACING PROBLEM: a maintainer with an accepted broad PR can remain unable to merge because the trusted PR workflow repeats a release-owned capture and fails late.
PROBLEM SEVERITY: broken task — the required status blocks the merge even though the documented policy has already delegated this class to the daily release gate.
VERDICT: clear
Default-branch code still resolves the current PR and exact CI run, then classifies changed paths from the GitHub API. When that trusted classifier says the stable scope is broad, the workflow now reads the trusted baseline count and emits a skipped record instead of downloading Storybook or launching a browser. The same immutable publication path exposes that record, and the existing comment/status projection names it as deferred rather than accepted.
SOLUTION (3 decisions · about 125 runtime lines of 434 changed lines)
- Split broad scope before the Storybook download and capture steps; narrow scope retains the existing path.
- Add a validated skipped-evidence producer and resolve it through the same PR/head/run/attempt-specific immutable publication path.
- Render skipped evidence as
success/deferred, with an explicit report and no/accept-visualcommand.
BURDEN: low — one internal CLI command, one report branch, and workflow conditions; no package surface, runtime state, dependency, browser code, or new baseline writer.
BURDEN MATCH: proportionate — the trusted status remains evidence-backed while removing work the release gate already owns.
VERDICT: clear
OWNER: the trusted PR Comment workflow owns PR-scoped evidence; the Release Gate owns broad visual capture; Visual Baseline remains the only baseline writer.
TIER 1: reuses the default-branch scope classifier, existing evidence schema, gh-pages path, comment formatter, and required commit status.
TIER 2: none.
SEAMS: CI workflow_run → exact PR identity → API-derived changed files → default-branch scope classifier → trusted baseline → skipped evidence → immutable gh-pages publication → comment/status projection.
BEHAVIOR UNIT: pure Node CLI plus workflow orchestration. trusted-defer is independently executable; the workflow test pins which steps may run in broad and narrow branches.
| seam | driven result |
|---|---|
| PR/head/run/attempt identity | generated record matched all four; independently mutating PR, head, run, attempt, or verdict attempt made state fail closed |
| motivating broad classification | #5516 classified broad from its real GitHub file list under unchanged default-branch policy |
| main negative | main produced a capture plan for the broad fixture; this head refused the plan and produced skipped evidence |
| narrow component scope | main and this head produced byte-identical two-shot plans |
| immutable publication | destination is pr/<pr>/visual/<head>/<run>/<attempt>; identical retries succeed, different bytes at the same path fail |
| baseline ownership |
release-gate.yml and visual-baseline.yml are unchanged; the former reads/captures daily and the latter remains the sole baseline writer |
The PR cannot provide the classifier or the skipped record: workflow_run executes and checks out default-branch code, reads changed filenames from the GitHub API, and generates the record locally. A PR can touch a path that trusted policy correctly classifies as broad; it cannot submit its own broadStableVisual value or publish its own skipped evidence.
VERDICT: clear
Broad stable PRs stop downloading Storybook, installing Chromium, and recapturing the baseline in the trusted comment workflow. They receive a required status of success with reason deferred, an immutable report that says capture was skipped, and no acceptance instruction.
Narrow component and theme PRs still download, plan, capture, compare, publish, and wait for explicit acceptance exactly as before. Releases still pay the full visual check once daily before the cut, and only the separately dispatched Visual Baseline workflow can change accepted baseline bytes.
VERDICT: clear
No public package API changes.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
visual-acceptance.mjs trusted-defer |
internal workflow CLI | broad trusted PR evidence | tests/workflow | ok |
~ |
skipped evidence requires capture: null, deltas: [], nonzero baseline count, reason, and matching identity context |
internal durable record | existing visual evidence schema | tests | ok |
~ |
skipped state projects to {state: success, reason: deferred}
|
internal status contract | existing clean/accepted/failed states | tests | ok |
OSSIFICATION: all additions are repo-internal and have one trusted caller. Removing or reshaping them later does not affect a published package; the durable path remains versioned and identity-specific.
VERDICT: clear
No target, token, variable, selector, style, or theme contract changes. The word “visual” here names CI evidence, not component rendering.
VERDICT: clear
BEHAVIOR: intentionally changed only for broadStableVisual: true: trusted PR capture becomes explicit deferral. Narrow behavior was compared against main and is byte-identical.
API: no public API change.
VISUAL: no component or page pixels change.
THEME: no target or override changes.
VERDICT: clear
EFFECTS: zero.
RENDER: no React or browser render path ships.
LISTENERS/OBSERVERS: none.
LAYOUT: none.
BUNDLE: no package output changes.
The CI resource change is deliberately negative: broad scope no longer downloads the Storybook artifact, installs Chromium, or starts capture. Narrow scope uses the same plan and capture commands. The historical wall-clock saving was not re-measured locally because the reviewed path is a trusted Actions workflow and cannot run from the PR's own workflow file before merge.
VERDICT: clear
VISUAL CHECK: not applicable
WHY: every changed path is under .github/scripts/ or .github/workflows/; no component, DOM, CSS, theme, story, or rendered application path changes. Exact-head Stable visual regression, a11y, and RTL jobs correctly skipped for no component scope.
VERDICT: clear
No rendered element, accessible name, role, focus path, keyboard path, translated string, locale formatter, or directional style changes. Human-readable workflow output is escaped by the existing report formatter; the new skipped report escapes its reason before insertion.
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 — main still produced a broad capture plan for the control fixture; this head refused capture, emitted exact skipped evidence, returned success/deferred, and preserved the main narrow plan byte-for-byte.
DISPOSITION: no negative findings.
ADVICE: omitted — no defect found.
AUTHOR CAN PROCEED: yes — every required trust property was verified; no human-owned API, design, architecture, or cost decision remains.
WORST OUTCOME: none found.
JUDGEMENT NEEDED: none — CI correctness fix against the existing broad-scope ownership policy.
APPROVE.
Thanks — this is clean. Broad scope now defers with exact immutable evidence, while narrow capture stays byte-identical and release-gate baseline ownership is untouched.
[Reviewed by Robohands]
None.
- PASS — focused visual-gate suite: 18 files, 195 tests.
- PASS —
actionlintonpr-comment.ymlwith the repository's2-core-ubuntu-armrunner label allowlisted. - PASS — Prettier on the four changed files covered by the PR's formatted-file check.
- PASS —
git diff --check; isolated review worktree remained clean. - PASS — main negative: main generated the broad capture plan; this head refused it and emitted validated deferred evidence.
- PASS — adversarial identity probes: wrong PR, head, run, attempt, and verdict attempt all failed closed.
- PASS — narrow component plan was byte-identical between main and this head.
- PASS — current exact-head CI: build, test, lint, check-components, theme-layers, Storybook, sandbox, and dependency checks succeeded; component-scoped visual/a11y/RTL jobs correctly skipped.
- A hand-mutated record can preserve the exact
headShawhile changing the auxiliarytestedSha, andstatestill accepts it. That is not reachable through this publisher:trusted-deferconstructs both from the same trusted head, validates before writing, and immutable publication rejects replacement. The identity that gates status — PR,headSha, run, and attempt — fails closed. - A hand-mutated record can also change its archived
trustedScopeafter generation. That is likewise outside the write path: broad classification and the second broad assertion both run before the immutable bytes exist. - A stock Prettier pass over every changed
.mjsreports two files dirty, but the same two base files are already dirty under that formatter; this head introduces no formatting regression and the repository does not enforce that stock configuration for them.
- The changed
workflow_runYAML cannot execute from the PR head: GitHub runs the default-branch workflow. The CLI branches, workflow conditions, formatter, and state machine were driven locally; the first real gh-pages publication through this exact YAML occurs after merge. - The review did not push to gh-pages, issue an acceptance command, or perform any public PR action.
Full lane: draft → critic → rewrite → critic. Two critic passes. The final 24-word public comment is within the approval cap, contains no private control data, and makes no claim beyond the verified seams.
TIME total 13m · setup 2m · reading 6m · measuring 4m · writing 1m · waste 1m (stock Prettier against two already-dirty base files)
Not posted. The task explicitly required judgment only and no public GitHub PR action; the text above is the exact draft.