-
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)
ff779be30e7bf986e15e827c7ff8122c26a6660b
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 (2 decisions · 91 additions / 41 deletions total)
- Move
visual-approvedinvalidation out of thepull_request_targetinitializer and into the trustedworkflow_runpublisher; mutating jobs keeppull-requests: write. - Reuse #5587’s
ASTRYX_VISUAL_RUN_ID/ASTRYX_VISUAL_RUN_ATTEMPTimplementation and addASTRYX_VISUAL_SHA, so trusted recapture records the PR head without duplicating identity logic.
The existing clean-capture state remains covered: a trusted pass resolves success: clean without /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 CLI/workflow glue — captureIdentity() extends #5587’s landed identity boundary and is covered by gate-context; 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 |
captureIdentity() prefers ASTRYX_VISUAL_SHA plus #5587’s source run/attempt variables; unit tests cover 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 post-rebase visual-gate plus visual-scope suite passed 128 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, extends #5587’s source-run identity with ASTRYX_VISUAL_SHA, preserves clean-capture success, and the post-rebase suite passed 128 tests plus actionlint and check:repo.
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, still looks good after the #5587 rebase.
[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 18m setup 4m exact-head worktree + fast-install donor clone; warm main reused: yes reading 6m full kit, prior reviews, PR body, #5587 overlap, complete diff measuring 5m 128 tests, actionlint, check:repo, exact-head CI/status checks writing 3m presentation + clean critic pass waste 2m one glob retry, one visual-scope fixture retry, one known runner-label retry
- 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 original clean review was invalidated when #5587 landed overlapping workflow and gate code. The PR was rebased onto c0c7f8b991c, duplicate run-identity logic was removed, and only the missing source-SHA and least-privilege publisher ownership remained. A fresh full pass reviewed exact head ff779be30e7bf986e15e827c7ff8122c26a6660b, reran 128 tests, actionlint, check:repo, and exact-head CI, and returned a clean semantic approve.