-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5592
cixzhang · merged · view on GitHub · squash commit d0536cd
Verdict: COMMENT — semantic approve with notes (Round 2 drafted; no public PR action) · exact-head review of 1efb73cc462d0677eba824f90bda9adbbc3f6c9f
1efb73cc462d0677eba824f90bda9adbbc3f6c9f — pinned against both GitHub's PR head and the detached worktree. No other PR head was reviewed.
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full WHY: this changes a privileged workflow authorization boundary and an immutable acceptance-record schema.
The prior round's two blockers are resolved. The branch now contains #5591's effective maintain/admin normalization, and dependency-free accept starts and completes from an isolated archive with no node_modules. #5593 landed six minutes after this head and independently added an install to the archive job, so main now has two mechanisms covering the dependency failure.
VERDICT: note — the failure is fixed here, but the later main change leaves redundant setup after merge.
The workflow and CLI share one pure identity parser/checker. It preserves GitHub's reported permission, effective capability, and role; authorizes effective maintain/admin or exact Repo Owner; and archives all three values. pngjs resolution moved from module load to the synchronous pixel-comparison path used only by promotion.
SOLUTION (3 decisions · about 60 runtime lines of 242 changed lines)
- Share identity normalization and archive the reported values.
- Admit the exact
Repo Ownerrole while refusing ordinary writers and role variants. - Resolve
pngjsonly when promotion compares pixels.
VERDICT: note — the exact-name role fallback is not needed by any current collaborator because all 11 Repo Owners already report effective maintain.
OWNER: visual-acceptance workflow and visual-gate CLI.
SEAMS: collaborator API payload → identity helper → job outputs → quoted CLI arguments → immutable record → record validation; promotion → lazy pixel dependency.
BEHAVIOR UNIT: authorization.mjs, a pure module reused at authorization, archive, and validation boundaries.
Driven evidence:
- isolated archive with no
node_modules:accept,state, andplancomplete at this head; the sameacceptprobe fails at the prior base on eagerpngjsresolution; - exact workflow import expression executes under
github-script's CJS shape withGITHUB_WORKSPACEset; - the branch merges with current main without conflict, and the 14-file suite remains green on that merged tree.
VERDICT: clear
The 33 current collaborators retain the same authorization outcome as main: 11 Repo Owners and 9 admins pass; 13 ordinary writers do not. Accepted records now preserve permission: write, effectivePermission: maintain, and roleName: Repo Owner instead of synthesizing a permission. Legacy maintain/admin records without the new fields still validate.
The literal role fallback has no current beneficiary. If an organization owner later assigns the exact display name Repo Owner to a role without maintain/admin capability, that identity would pass on its name alone. This is a non-blocking note because no current collaborator occupies that state.
VERDICT: note
No public package API changes. Internal additions are the shared identity helpers, two accept flags, and additive immutable-record fields. The record shape is backward-compatible in the driven legacy matrix.
VERDICT: clear
No theme target, token, CSS variable, component class, or style contract changes.
VERDICT: clear
API: no public API change. VISUAL: no rendered component path changes. THEME: no. BEHAVIOR: intended authorization expansion only; all current collaborators receive the same decision as main, and exact role casing is enforced. Legacy records remain valid.
VERDICT: clear
No React effects, listeners, observers, render work, or package dependency changes. pngjs is resolved once, through cached createRequire, only when promotion first compares pixels. After #5593, the merged archive job also installs dependencies that this path no longer needs; that cost was not measured.
VERDICT: note — redundant install on the merged accept path, not a regression introduced by this exact head.
VISUAL CHECK: not applicable WHY: all six changed files are workflow, Node CLI, and tests. Stable visual, a11y, and RTL checks are correctly skipped by path scope.
VERDICT: clear
No markup, focus, keyboard, ARIA, locale, translation, or direction-sensitive behavior changes.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | note — later main change also fixes the dependency failure |
| SOLUTION | note — exact role-name fallback is redundant for current owners |
| ARCHITECTURE | clear |
| IMPACT | note — display-name authority could widen if that role's capabilities change |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | note — merged accept path now installs an unused dependency |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: met — dependency-free acceptance executes; exact owner and effective maintain/admin paths pass; ordinary write and role variants fail; new and legacy records validate; workflow plumbing and permissions remain fail-closed.
DISPOSITION: semantic approve. Three non-blocking follow-ups: prefer capability authority over the redundant role-name fallback; reconcile the lazy load with #5593's install; restore a test pinning the authorize job's no-PR-ref checkout invariant.
AUTHOR CAN PROCEED: yes — nothing is withheld.
WORST OUTCOME: a future read-only custom role named exactly Repo Owner is admitted by display name. No current collaborator is in that state, so this is a note rather than a block.
comment — semantic approve with notes
-
roleName === 'Repo Owner'is authority by display name → all 11 current Repo Owners already report effective maintain; if the role later loses that capability, a read-only identity would still be admitted ·.github/scripts/visual-gate/authorization.mjs:27 - Main now has two fixes for the archive dependency
→ #5593 installs dependencies in a job whose
acceptpath no longer needs them ·.github/scripts/visual-gate/visual-acceptance.mjs:606 - The workflow regression test no longer pins the no-PR-ref checkout invariant
→ a future
ref:edit could let pull-request-controlled code run before authorization without this suite catching it ·.github/scripts/visual-gate/workflow-concurrency.test.mjs:59
Exact public comment drafted for this self-authored PR; not posted:
Thanks — isolated
acceptnow passes withoutnode_modules, and #5591's effective maintain/admin normalization remains intact alongside exactRepo Owner. Semantic approve.[Reviewed by Robohands]
- PASS — visual-gate Vitest suite at the exact head: 14 files, 135 tests (7.02s).
- PASS — same suite on current main merged with this head: 14 files, 136 tests (7.64s); #5593's added test survives.
- PASS — isolated archive with no
node_modules: 18 authorization cases plus 7 identity payloads; dependency-freeacceptworks at this head and fails at the prior base on eagerpngjsresolution. - PASS — exact
Repo Owner+permission=write; effective maintain/admin; refusal of ordinary write and lowercase, uppercase, padded, newline, no-space, owner-only, and prefixed role variants. - PASS — honest new-record round trip and legacy maintain/admin records with new fields absent.
- PASS —
actionlint .github/workflows/visual-acceptance.yml; merge-tree against current main has zero conflicts. - CI — exact head at review time: 16 success, 6 expected skips, 0 failures; after merge, 18 success and 8 expected skips; all three commit statuses are green.
- PASS — 4 focused files / 43 tests, including the isolated archive acceptance child process.
- PASS — full visual-gate suite: 14 files / 135 tests.
- PASS — an additional archive copy with neither local nor parent
node_modulesreachesaccept()validation without resolvingpngjs. - PASS — #5591's
user.permissions.admin/maintainnormalization remains in the shared identity parser; exactRepo Ownerpasses, ordinarywrite, lowercase, and padded role variants fail. - PASS —
actionlint .github/workflows/visual-acceptance.yml. - No PR review, comment, push, or merge action was taken by this re-review session; the PR was squash-merged independently at 12:56 AM PT.
- The changed
authorizejob has not executed on a runner at this head:pull_request_targetuses the base workflow, whileauthorizeandacceptrequire an issue comment. Its YAML path was driven locally and checked with actionlint; local Node was 26.7.0, runner Node is 24. - Whether the organization can later change the
Repo Ownercustom role's capabilities without changing its name.
TIME total 25m · setup 3m · reading 6m · measuring 7m · writing 6m · waste 3m
#5592 fix(ci): authorize Repo Owners for visual acceptance by cixzhang (bucket: the maintainer)
1f85e0c7055b733f7646e30b4c36a3d373c91d37 — every claim below is about this exact commit. GitHub showed a later current head, 6d4cf2570235f389f922e7f3fec93d7c198d9847, during the review; that later head was not reviewed.
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full
WHY: this changes a privileged workflow authorization boundary and an immutable acceptance-record schema; the pinned head also predates an overlapping authorization fix now on main.
WHY 1: GitHub reports three repository owners with top-level permission: write and exact role_name: Repo Owner, so the old coarse maintain/admin check refuses their visual decisions.
WHY 2: A repository owner can inspect and approve changed pixels but cannot record that decision, leaving the required visual status unresolved.
WHY 3: The visual gate exists to make a human pixel decision durable and reproducible; an authorization or archive failure blocks otherwise merge-ready visual changes.
USER-FACING PROBLEM: A repository owner reviewing a stable visual PR can provide the exact acceptance command and still be unable to complete the required visual decision.
PROBLEM SEVERITY: broken task — the decision cannot be archived, and the required gate cannot clear.
VERDICT: clear
The workflow reads GitHub's reported permission and role, admits only effective maintain/admin or the exact repository-owner role, and carries both values into the durable decision record. The record validator repeats the same check, so changing stored role text cannot manufacture authority. However, the archive command still loads a pixel-comparison package before command dispatch, and that job does not install packages, so the accepted decision never reaches the record.
SOLUTION (1 security decision · ~30 runtime lines of 83 changed lines)
- Treat exact
Repo Owneras equivalent authority while retaining maintain/admin, carry reported permission plus role through the workflow, and validate the same pair in immutable records.
BURDEN: medium — one pure authorization helper plus workflow output/CLI/record plumbing inside a privileged gate; no state, listener, timer, or public package surface. BURDEN MATCH: proportionate — the security decision is narrow, exact, and tested, but the dependency-free archive environment is not exercised.
VERDICT: BLOCKS — the real archive path exits before command dispatch because pngjs is unavailable.
OWNER: visual-acceptance workflow and its visual-gate CLI own authorization and durable decision validation.
TIER 1: GitHub collaborator-permission API, trusted pull_request_target/issue_comment workflow, and gh-pages immutable acceptance records.
TIER 2: none.
SEAMS: API response → authorization helper → job outputs → shell arguments → acceptance writer → acceptance validator → public status/comment projection.
BEHAVIOR UNIT: pure utility — isVisualAcceptanceMaintainer() is independently executable and reused at authorization, write, and read boundaries.
| seam | driven result |
|---|---|
| live collaborator response |
cixzhang, imdreamrunner, and josephfarina each report permission: write, role_name: Repo Owner, and user.permissions.maintain: true
|
| helper boundary | exact Repo Owner passes; write, repo owner, Repo Owner , and REPO OWNER fail |
| workflow output plumbing |
response.data.role_name → role_name output → ROLE_NAME → quoted --role-name
|
| new immutable record | stores decision.permission: write and decision.roleName: Repo Owner; validation repeats the exact authority check |
| old immutable records | supplemental tests pass for legacy maintain and admin records with no roleName
|
| public refusal | unauthorized callers receive the existing generic maintainer refusal; archive failure produces the generic infrastructure-failure comment |
| dependency-free archive job | fails before accept() with ERR_MODULE_NOT_FOUND: pngjs; the job has no package install |
current main
|
pinned head predates #5591; git merge-tree reports a content conflict in the workflow, so its effective user.permissions normalization is not safely composed here |
The exact role boundary and record validation are correctly owned. The runnable command boundary is not: a promotion-only dependency is imported by every command, and the pinned workflow cannot merge cleanly with the effective-permission fix now on main.
VERDICT: BLOCKS — the owner path is unrunnable in its deployed environment, and the pinned head conflicts with current authorization logic.
Repository owners gain the intended authority in the pure helper, while ordinary writers remain refused. In the actual workflow at this head, an authorized owner still gets a failed visual-acceptance status and the public “could not be recorded” infrastructure comment because the archive process cannot load pngjs; no immutable decision is written. A careless conflict resolution against current main could also drop #5591's effective maintain/admin path and re-refuse non-owner maintainers.
VERDICT: BLOCKS — the stated owner task still cannot complete end to end.
No public package API change.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
`decision.roleName: string | null` | internal durable acceptance record | same identity evidence as decision.permission
|
workflow/tests |
~ |
accepted identity = effective maintain/admin or exact Repo Owner
|
internal privileged workflow contract | existing maintainer authorization | workflow/tests | ok at the helper/record boundary |
OSSIFICATION: the durable version-1 record gains one optional field. Existing maintain/admin records without it remain valid; write records only validate when the archived role is exactly Repo Owner. Being wrong would either admit ordinary writers or invalidate historical decisions, and supplemental tests rule out both at this head.
VERDICT: clear
No theme target, token, CSS variable, component class, or style contract changes.
VERDICT: clear
BEHAVIOR: intentionally expands authorization to exact repository owners; ordinary write remains refused. Operationally, the valid accept path still fails before recording. API: no public package API change; the internal record addition is backward-compatible for legacy maintain/admin records. VISUAL: no rendered component path changes. THEME: no.
VERDICT: BLOCKS — the intended new behavior is unreachable in the deployed archive job.
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 React render path changes.
LISTENERS/OBSERVERS: none.
LAYOUT: none.
BUNDLE: no dependency is added, but pngjs is eagerly resolved for the dependency-free accept and state commands even though only promote() compares decoded pixels. This is an availability failure, not a size claim.
VERDICT: BLOCKS — a promotion-only package dependency prevents the acceptance process from starting.
VISUAL CHECK: not applicable WHY: all changed paths are GitHub workflow and Node authorization/record plumbing; no rendered component, style, story, or visual endpoint changes. Exact-head CI marks stable visual, a11y, and RTL jobs skipped for no component scope.
VERDICT: clear
No application markup, keyboard path, focus behavior, ARIA, locale formatting, translated product string, or direction-sensitive layout changes. The workflow keeps authorization refusals generic rather than publishing the actor's permission or role; the accepted-path failure is likewise projected as a generic infrastructure error.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — archive command cannot start without installed packages |
| ARCHITECTURE | BLOCKS — dependency boundary fails; pinned head conflicts with current authorization logic |
| IMPACT | BLOCKS — authorized owner still cannot record a decision |
| API | clear |
| THEMING | clear |
| BREAKING | BLOCKS — intended owner behavior is unreachable |
| PERFORMANCE | BLOCKS — eager promotion-only dependency breaks availability |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: not met — the exact role helper and immutable-record checks pass, but a dependency-free copy of the production archive command exits with ERR_MODULE_NOT_FOUND: pngjs before accept() runs.
DISPOSITION: [BLOCKS] lazy-load pngjs only in promote() and add a no-node_modules acceptance test; [BLOCKS] rebase/resolve current main while preserving #5591's user.permissions.admin/maintain normalization plus the exact role fallback.
ADVICE: proven — moving pngjs resolution into the only function that uses decoded pixels lets dependency-free accept, state, and plan start; an isolated-copy test reproduces the deployed job. The current-main conflict and live API shape establish the second acceptance criterion.
AUTHOR CAN PROCEED: yes — the required outcomes are exact: dependency-free accept passes, exact owner role passes, ordinary write and role variants fail, legacy maintain/admin records validate, and the rebased workflow retains effective permission booleans.
WORST OUTCOME: “A repository owner gives a valid visual decision, but the gate posts an infrastructure failure and writes no immutable acceptance record.” → request changes.
JUDGEMENT NEEDED: none — both corrections have executable acceptance criteria.
request changes
- [BLOCKS] the production archive command imports
pngjswithout installing it → a repository owner with a valid decision gets a failed required status and no acceptance record ·.github/scripts/visual-gate/visual-acceptance.mjs:15 - [BLOCKS] this pinned head predates and conflicts with the effective-permission fix on
main→ a conflict resolution can re-refuse maintainers whose authority is reported only underuser.permissions·.github/workflows/visual-acceptance.yml:184-206
Thanks — the Repo Owner boundary itself is exact, and ordinary write plus altered role spellings stay refused.
This head still cannot record an accepted decision: visual-acceptance.mjs:15 imports pngjs, while the archive job installs no packages, so an isolated acceptance exits before writing the record. It also conflicts with #5591; the resolution needs to preserve that PR's effective maintain/admin normalization.
Could you lazy-load pngjs inside promote(), cover dependency-free accept, and rebase onto #5591?
[Reviewed by Robohands]
None — both findings are cross-file workflow boundaries and belong in the summary comment.
- The live collaborator API returned the same exact role/casing for all three named owners; no
author_associationpath is introduced. - Exact-head CI has 21 check runs: 14 success, 6 expected skips, and 1 initializer cancellation caused by the later pushed head; all three commit statuses are success.
- GitHub showed PR head
6d4cf2570235f389f922e7f3fec93d7c198d9847during this review. Per the request, no conclusion here applies to that later head.
- PASS — pinned-head focused suite with unrelated global setup disabled: 3 files, 26 tests.
- PASS — supplemental immutable-record matrix: legacy maintain/admin without role metadata; exact owner accepted; wrong casing/spacing refused (22 tests total in the augmented file).
- RED→GREEN — direct owner-role probe is absent/fails on merge-base
c0c7f8b991c; passes pinned head; ordinary write remains false. - EXPECTED FAIL — isolated production-shaped
acceptwith nonode_modules:ERR_MODULE_NOT_FOUND: pngjsbefore command dispatch. - PASS —
actionlint .github/workflows/visual-acceptance.yml. - CI — exact head: 14 success, 6 skipped, 1 superseded-run cancellation;
test,lint,build,build-storybook,build-sandbox, smoke, dependency, docsite, component, theme, and scope checks succeeded.
TIME total 11m setup 2m exact-head detached worktree plus two fresh wiki clones; warm main reused: yes reading 3m current kit/version, manifesto, repo guidance, PR body/history, six changed files measuring 4m 26 focused tests, 22-test supplemental matrix, 4 direct probes, actionlint, merge-tree, exact-head CI writing 2m full presentation plus two critic passes waste 2m first Vitest invocation inherited unrelated global setup; reran with a minimal review config
- No live
/accept-visualmutation was run; the task is read-only. The production failure was reproduced with the same dependency-free process shape. - The later PR head was deliberately not reviewed.
No public review was posted. The first critic pass required the archive blocker's second confirmation and separated it from the current-main conflict. The rewrite added the isolated-process reproduction, exact live API matrix, immutable-record compatibility probe, and a concrete consequence for each finding. The second critic pass kept the semantic request-changes verdict, cut implementation detail from the public comment, and confirmed its two asks are independently executable.