-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5588
cixzhang · open · view on GitHub
Verdict: comment — semantic approve (final posted review) · do not merge automatically; CI/release process change
#5588 fix(ci): restore visual acceptance publication by cixzhang (bucket: the maintainer)
aa619316ef86334d32688af7a07d6c11b3dbb46e
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full WHY: fast lane is ineligible because this changes a privileged GitHub workflow boundary, status/label behavior, and source-run identity across more than one runtime decision.
WHY 1: The required visual-acceptance context could get stranded when label writes ran with the wrong permission and trusted recapture recorded the publisher run identity instead of the source CI run.
WHY 2: A maintainer could finish reviewing or accepting stable visual evidence and still have GitHub block the merge on stale status/label projections.
WHY 3: The visual gate exists to make pixel acceptance a reliable required status, so a projection failure turns accepted visual work into CI-infrastructure debugging.
USER-FACING PROBLEM: A maintainer reviewing a stable visual PR can finish the pixel decision and still have GitHub block merge on stale visual-acceptance status/label projections.
PROBLEM SEVERITY: harmful friction — the work is recoverable, but the valid merge path is blocked after the evidence decision is made.
VERDICT: clear
The initializer only classifies the PR and writes the required status. The trusted publisher now owns label invalidation and PR projections with the permission those PR mutations need. Trusted recapture carries the source CI identity in non-reserved ASTRYX_* variables, so published evidence is checked against the run that produced the Storybook artifact. Clean captures can succeed without an acceptance record because there is no pixel delta to accept.
SOLUTION (4 decisions · ~71 runtime/workflow changed lines of 184)
- Move
visual-approvedinvalidation out of thepull_request_targetinitializer and into the trustedworkflow_runpublisher. - Use
pull-requests: writefor PR label/comment projections instead ofissues: write. - Carry trusted capture identity through
ASTRYX_CAPTURE_SHA,ASTRYX_SOURCE_RUN_ID, andASTRYX_SOURCE_RUN_ATTEMPT, centralized incaptureContext(). - Treat a trusted clean capture as
success: cleanwithout requiring/accept-visual.
BURDEN: medium — privileged workflow permissions and required-status projection change, but no public package API, no dependency, no persistent runner, and the identity mapping is isolated in one pure helper. BURDEN MATCH: proportionate — the broken task is the required status itself, and the fix stays inside the workflow owner with focused tests.
VERDICT: clear
OWNER: visual acceptance workflow plus the visual-gate CLI evidence publisher.
TIER 1: GitHub Actions pull_request_target/workflow_run, commit statuses, PR labels/comments, gh-pages immutable visual evidence.
TIER 2: none.
SEAMS: initializer, trusted publisher, trusted recapture, acceptance comment, clean/no-scope evidence states.
BEHAVIOR UNIT: pure utility — captureContext() is the identity boundary and is unit-tested; workflow placement is covered by workflow architecture tests.
| seam | driven result |
|---|---|
| initializer | no PR-write permission and no label mutation; workflow test asserts read/status-only shape |
| trusted publisher invalidation | pending status precedes label removal; workflow test asserts the order and permission |
| trusted recapture |
captureContext() prefers ASTRYX_* source identity; unit test covers publisher and normal PR paths |
| acceptance/comment projection | PR-write projection shape remains in the mutating jobs; workflow test covers the routing |
| clean trusted evidence |
visual-acceptance state returns success: clean; targeted test covers it |
VERDICT: clear
Maintainers of stable visual PRs should no longer see a reviewed pixel bundle stuck because the workflow could not delete visual-approved or because the trusted capture looked like the publisher run. Contributors see the same required visual-acceptance context, but clean/no-scope captures can clear without an irrelevant acceptance step. No app user sees a rendered UI change.
VERDICT: clear
No package API change.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
manifest.context.sha/runId/runAttempt may come from ASTRYX_* source-run variables in trusted recapture |
internal visual-gate evidence contract | existing evidence identity fields | tests | ok |
OSSIFICATION: internal workflow/evidence surface only; the permanent public user-facing API does not change.
VERDICT: clear
No theme target, token, CSS variable, component class, or style contract changes.
VERDICT: clear
BEHAVIOR: yes, intentionally — label invalidation moves from initializer to trusted publisher, and clean trusted captures now resolve success without acceptance. API: no public package API change; internal evidence context keeps the same keys. VISUAL: no — no rendered component code changed. THEME: no.
VERDICT: clear
EFFECTS: zero.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
| none | none | none | no render path | none | n/a |
RENDER: no rendered React path changed. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or shipped package bundle change. Workflow cost is unchanged in kind: one status write and one label deletion move into an already-running trusted job; the expanded visual-gate suite passed 116 tests.
VERDICT: clear
VISUAL CHECK: not applicable WHY: changed paths are GitHub workflows and Node visual-gate scripts, not component render/style paths; the PR's visual and RTL checks are skipped because no components changed. Storybook port chosen for this review was 6268, but Storybook was not started.
VERDICT: clear
No user-facing component markup, keyboard path, focus management, ARIA, locale catalog, or direction-sensitive layout changes. The only strings are GitHub workflow status/comment text in the visual-acceptance operational surface.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: met — diff moves label invalidation to the trusted publisher, centralizes source-run identity in captureContext(), adds clean-capture success, and the visual-gate suite passed 116 tests plus actionlint on both changed workflows.
DISPOSITION: no negative findings.
ADVICE: omitted — no remedy needed.
AUTHOR CAN PROCEED: yes — CI is green and no human-owned API/design/cost decision remains.
WORST OUTCOME: “A maintainer reviewing a stable visual PR can finish the pixel decision and still have GitHub block merge on stale visual-acceptance status/label projections.” → approve, because this PR removes that path and the focused workflow tests pass.
JUDGEMENT NEEDED: none — infrastructure defect fix against the existing visual-acceptance contract.
approve and merge
Thanks, this looks good. The trusted publisher now owns label writes, and ASTRYX_* keeps recapture identity tied to the source CI run.
[Reviewed by Robohands]
- CI is green on the reviewed head; the public checks already show it.
- I did not start Storybook because this is a non-rendering workflow/script PR.
- I picked Storybook port 6268 and left it unused.
TIME total 8m
setup 2m worktree + fast-install donor clone; warm main reused: yes; Storybook port 6268 picked but not started
reading 2m brief/critic excerpts, PR body, complete diff, changed workflows/scripts
measuring 2m actionlint, 24-test targeted run, then 116-test visual-gate suite; 0 re-measures
writing 2m presentation + critic pass + gate file
waste 1m first $KIT env lookup failed, and the 24-test run was subsumed by the later full visual-gate suite
- The accepted-label add path only runs after a real
/accept-visualcomment; locally I verified the workflow route and tests, not a live GitHub mutation.
The first pass was clean and suggested one optional PR-body clarification. The body now states that clean/no-scope captures remain successful without acceptance. A fresh second full pass reviewed the ready exact head, reran the 116-test visual-gate suite and workflow lint, and returned a clean semantic approve. Both public comments were posted verbatim; no code findings required a fix round.