-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5520
Versions: Review Loop 1.0.0 · Component Audit Rubric 1.10
Verdict: request changes
#5520 ci: validate Visual Baseline inputs before promotion by bhamodi (bucket: contributor)
c5181e640e2327c81571568060e2bc686dfa4f1d
LOOP VERSION: 1.0.0 AUDIT RUBRIC: 1.10
A release operator can mistype a source run or supply a path-like shot key; without validation, material outside the reviewed Release Gate capture can be trusted as the permanent baseline. This is a release-integrity problem for builders and everyone receiving the package cut from that baseline.
VERDICT: clear
The workflow checks that the named run belongs to Release Gate on main and is terminal before downloading it. The baseline writer rejects keys outside the filename alphabet before constructing paths. Dispatch text reaches shell steps through environment variables, so it remains data.
SOLUTION (3 decisions · about 30 runtime lines of 39 additions)
- Check source-run workflow, branch, and terminal status. [stated problem]
- Check promoted/pruned key alphabet before path construction. [stated problem]
- Remove direct shell interpolation of dispatch text. [input-safety hardening]
VERDICT: clear — three small, coherent input-safety decisions
OWNER: the Visual Baseline promotion transaction (gate.mjs accept and the downloaded capture it consumes)
TIER 1: none
TIER 2: none
SEAMS: workflow dispatch → run metadata → downloaded manifest/verdict/PNGs → accept() → gh-pages baseline
BEHAVIOR UNIT: pure validation utility — source eligibility can be tested without Actions and must run before baseline mutation
The pre-download metadata check is useful but cannot establish that the visual leg succeeded: GitHub reports failed terminal runs as status=completed, failed captures are uploaded under if: always(), and accept ignores verdict.status. The current CLI promoted one shot from a fixture whose visual verdict was failed. The downloaded capture is the first boundary that owns the fact being validated, so the check must finish there before mutation.
VERDICT: BLOCKS — a failed visual capture remains promotable
A release operator can select a Release Gate run whose visual leg failed, receive a successful promotion, and replace trusted baseline files from a capture the release contract says to hold. Later visual comparisons then treat those untrusted pixels as correct.
VERDICT: BLOCKS — the new check does not fail closed for its stated source-run class
No public package API changes. Existing workflow-dispatch inputs keep their names and types; unsafe values are intended to fail. No new exported helper or permanent consumer surface is added.
Ossification: none.
VERDICT: clear
No styles, theme targets, tokens, or rendered component state change.
VERDICT: clear
- API: no package API change.
- Visual: no rendered component output changes.
- Theme: no theme surface changes.
- Behavior: path-like keys and wrong-workflow runs now fail; valid promotions are intended to remain unchanged.
VERDICT: clear
Effects: zero. Listeners/observers/layout/bundle: none. A manual promotion adds one GitHub API request and three local jq reads; no end-user runtime path is touched and no degradation was measured.
VERDICT: clear
Not renderable: this is workflow and filesystem validation only; the diff adds no DOM, geometry, color, or component output.
VERDICT: clear
No component string, role, ARIA, focus, keyboard, locale, or direction path changes. Operator-facing failure behavior is covered under ARCHITECTURE/IMPACT.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear — 3 coherent input-safety decisions |
| ARCHITECTURE | BLOCKS — failed visual capture remains promotable |
| IMPACT | BLOCKS — source check is not fail-closed |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear |
| A11Y & I18N | clear |
JUDGEMENT NEEDED: none — correctness against the existing release contract.
GOAL: partly met — wrong workflow/branch and unsafe key shapes fail, but a Release Gate whose visual leg failed still passes the new terminal-status check and remains promotable.
DISPOSITION: failed visual capture accepted → blocks now; the release contract explicitly says a crashed visual gate holds, and current accept promoted from verdict.status=failed. (visual-baseline.yml:75-77, gate.mjs:455-472)
ADVICE: bounded direction — retain the early metadata check, then validate the downloaded visual result before the first baseline write. The required outcome is that a failed visual verdict cannot promote any shot; a focused regression test can drive the CLI with a failed verdict fixture.
AUTHOR CAN PROCEED: yes — require a usable visual result after download, leave the baseline untouched on failure, and add a regression case proving verdict.status=failed exits nonzero.
WORST OUTCOME: “A release operator can select a Release Gate run whose visual leg failed, receive a successful promotion, and replace trusted baseline files from a capture the release contract says to hold.” → request changes
request changes
- [BLOCKS] Terminal run status does not establish that the visual capture succeeded.
→ a failed gate can replace trusted baseline files ·
.github/workflows/visual-baseline.yml:75-77
Thanks — I need to correct my earlier read: a failed visual gate still passes this source check.
GitHub reports failed terminal runs as status=completed. The capture is uploaded with if: always(), and the current accept command promoted a test shot from verdict.status=failed, so this can replace baseline files from a gate the release contract says to hold.
Could you validate the downloaded visual result before promotion and add a failed-capture regression case? If you'd rather talk it through with someone, we're in Discord.
[Reviewed by Robohands]
-
.github/workflows/visual-baseline.yml:75— Terminal failure is alsocompleted; validate the downloaded visual verdict before promotion.
- Traversal, slash/backslash, dot-only, empty, Unicode slash, and shell payloads all failed before mutation; the filename guard works for both promote and prune.
- Quoted environment-variable routing prevented command execution in commit and summary steps.
- Pre-existing boundary gaps, not charged to this contributor: an alphabet-valid unknown or missing key exits 0 and can produce a partial/empty decision;
a, bfails on the leading space. The prior trim/test inlines remain valid.
#5520 ci: validate Visual Baseline inputs before promotion by bhamodi (bucket: contributor)
776e7442d579ce804ca057e7da0f8ec3b53719a6
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: this is a fork PR changing workflow and CI execution surfaces, with a standing request-changes and a release-baseline trust boundary.
Our request-changes at c5181e640e2327c81571568060e2bc686dfa4f1d asked for failed-capture rejection plus a regression case. The current head was checked point by point:
| prior finding | current status | evidence |
|---|---|---|
| failed visual verdict can be promoted | fixed | workflow accepts only pass/changed; command rejects failed/skipped before accept()
|
| failed-capture regression case | not fixed | full-tree test search finds no verdict-acceptance case; remote exact-head test ran baseline.test.mjs with nine tests, none covering verdict rejection |
a, b dispatch input fails on its space |
fixed | keys are trimmed before validation |
| shot-key guard had no test | fixed |
baseline.test.mjs covers path-shaped and dot-only promote/prune keys |
The author’s response extends our prior review; it does not contradict another reviewer. Mainline visual-acceptance work and open maintainer PR #5600 overlap publication, but neither replaces this source/verdict validation.
WHY 1: manual baseline promotion previously trusted an operator-entered run id and path-like shot keys, and a completed run could still carry a failed visual verdict. WHY 2: an operator could promote pixels the release gate did not stand behind, or write outside the intended shot-key namespace. WHY 3: those pixels become the trusted baseline for later release decisions, so a bad promotion makes future comparisons certify the wrong result. USER-FACING PROBLEM: a maintainer promoting reviewed visual changes can silently replace the release baseline with pixels from the wrong or failed gate run. PROBLEM SEVERITY: broken task — safe promotion cannot be trusted when failed evidence can become the baseline.
VERDICT: clear
The promotion workflow first verifies that the chosen run is a completed Release Gate run on main. After downloading the capture, it accepts only a verdict the gate stood behind, then validates and normalizes every requested shot name before any baseline write. Operator text is passed as data rather than interpolated into shell source.
SOLUTION (5 decisions · ~82 semantic runtime lines of 256 added / 54 removed)
- Require the source run to be Release Gate on main and completed. [stated problem]
- Require the downloaded verdict to be pass or changed before promotion. [review response]
- Reject path-shaped promoted and pruned keys. [stated problem]
- Trim comma-separated keys from the dispatch form. [prior inline]
- Pass keys, run id, and reason through environment variables. [stated problem]
BURDEN: low — one API read, local JSON checks, a filename predicate, no new dependency or long-lived process. BURDEN MATCH: proportionate — a release-integrity boundary earns fail-closed validation before mutation.
VERDICT: BLOCKS — the prior request for a failed-capture regression case remains unsatisfied on this high-risk release path
OWNER: the Visual Baseline promotion transaction
TIER 1: none
TIER 2: none
SEAMS: dispatch input → Actions run metadata → downloaded capture/verdict → gate.mjs accept → baseline manifest and PNGs
BEHAVIOR UNIT: inline workflow validation plus the accept command — statically readable; verdict rejection lacks a focused test
| seam | checked result |
|---|---|
| wrong workflow, branch, or non-terminal run | rejected by the metadata step |
| failed or skipped downloaded verdict | rejected before baseline fetch/mutation |
| missing, malformed, or unknown downloaded verdict | rejected by the workflow’s positive allowlist |
| path-shaped promote/prune key | rejected in accept(); remote unit test ran |
a, b dispatch input |
trimmed before key validation |
| current maintainer PR #5600 | changes queue/publish serialization, not this source/verdict boundary |
The production workflow now fails closed before mutation. Static inspection confirms the previous correctness blocker is fixed; the missing regression test is the only surviving prior ask.
VERDICT: BLOCKS — this release-boundary behavior has no durable regression evidence
The current production workflow prevents maintainers from promoting a failed or skipped Release Gate capture and keeps path-shaped names out of the baseline store. Valid pass/changed promotions remain available, including comma-separated keys with spaces.
VERDICT: clear
No published package API changes. The workflow dispatch inputs keep their names and types; gate.mjs accept changes only internal acceptance behavior.
OSSIFICATION: none in shipped package surface.
VERDICT: clear
No styles, tokens, theme targets, selectors, or rendered component output change.
VERDICT: clear
BEHAVIOR: invalid source runs, failed/skipped verdicts, and path-shaped keys now fail; a, b now succeeds as intended.
API: no published API change.
VISUAL: no rendered code or component output changes.
THEME: no theme contract changes.
VERDICT: clear
EFFECTS: zero. RENDER: no shipped runtime path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package change. One GitHub API request and local JSON checks are added to a manually dispatched CI job; no performance degradation was measured.
VERDICT: clear
VISUAL CHECK: not applicable
WHY: all four changed files are under .github/; the diff adds no DOM, CSS, story, geometry, paint, or component behavior. Exact-head Stable visual regression, pr-a11y, and pr-rtl were skipped by path scope.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
No component strings, roles, ARIA, focus, keyboard, locale, or direction path changes. New text is CI log output for maintainers.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — prior failed-capture regression case is missing |
| ARCHITECTURE | BLOCKS — release-boundary behavior lacks durable regression evidence |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: met — static control flow now rejects a failed/skipped downloaded verdict before baseline mutation; valid pass/changed captures continue. DISPOSITION: missing failed-capture regression case → blocks now; it is an explicit unsatisfied item from the standing request on a high-risk release path. ADVICE: bounded outcome criteria — a focused automated case must fail if a failed capture can reach any baseline write. AUTHOR CAN PROCEED: yes — add the prior requested regression case without changing the proven production behavior. WORST OUTCOME: “a later edit can reopen failed-capture promotion while the exact-head suite remains green” → request changes
JUDGEMENT NEEDED: none — correctness and regression evidence against the existing release contract.
request changes
- [BLOCKS] The failed-verdict behavior is fixed, but its requested regression case is absent.
→ a later edit can reopen failed-capture promotion while the exact-head suite remains green ·
.github/scripts/visual-gate/gate.mjs:671
Thanks — the failed-capture guard fixes the production path, and the trim/key-test nits are fixed. One part of our previous request is still missing: exact-head CI runs baseline.test.mjs, but no test exercises the new verdict guard, so removing it still leaves the suite green.
Could you add the failed-capture regression case before this lands? If you’d rather talk it through with someone, we’re in Discord.
[Reviewed by Robohands]
None.
- Remote exact-head CI ran 600 test files / 12,630 tests;
baseline.test.mjsran nine tests and passed. - The command-layer backstop rejects the two current non-promotable statuses; the workflow’s positive allowlist also rejects missing, malformed, and future unknown states.
- The PR body still describes the first-round shape and does not record the verdict check added in response; this is a non-blocking re-review note, not a second ask.
TIME total 10m setup/rules 2m full brief, critic, version, rubric, measurement contract; isolated worktree install/build/server 0m prohibited by fork workflow/script changes browser/a11y 0m non-rendering change; remote scoped jobs skipped focused tests 0m local execution prohibited; remote relevant test file passed code/history analysis 5m prior reviews/wiki, old→new delta, execution surfaces, main drift, related work critique + wiki publication 3m two presentations, two critic gates, fork-wiki update CI wait 0m all exact-head checks were already terminal total active 10m waste 1m initial old→new file diff included rebased main changes before author-patch isolation
- The workflow-dispatch path was not executed locally because this fork PR changes workflow and CI scripts.
- Vercel is red on this exact head; it is not a required check and no changed file is in the deployment surface.
Not posted: this was a read-only re-review. The proposed request-changes text above is the clean gate output.