-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5791
#5791 fix(ci): guide visual acceptance reliably by cixzhang (bucket: the maintainer)
3881003490098fb3a96a906b8559e71e8bd2008a
LOOP VERSION: 2.0.0 AUDIT RUBRIC: 1.15.1
LANE: full WHY: Security/trust workflow change: privileged GitHub Actions path, gh-pages evidence authority, maintainer-only authorization, and stale-command handling.
CHANGED CODE: Visual acceptance comment copy, evidence path construction, accept-command authorization, report hint text, and tests for those flows.
NEAREST CURRENT COMPONENT CONTRACT: CI visual acceptance workflow; no component API.
CURRENT FAMILY: pr-comment.yml publishes trusted visual reports, visual-acceptance.yml records maintainer decisions, and gh-pages-publisher.mjs writes immutable evidence/records.
CURRENT API/THEMING/SYSTEM RECORDS: Maintainer permission is effective maintain/admin; PR code remains untrusted in privileged workflows.
DRAFT CONTEXT: none.
STOPPED AT: changed CI workflow and adjacent publisher/formatter code — enough to resolve the security and guidance questions.
REGRESSION EVIDENCE: Focused tests and adversarial Node checks cover command visibility before publication, exact evidence path validation, stale/malformed command guidance, effective maintainer authorization, and trusted-code checkout.
OWNER QUESTION (PRIVATE): none.
| candidate issue | classification | evidence | disposition |
|---|---|---|---|
| Command appears before immutable evidence exists | preserves |
buildVisualSection only receives report/image URLs when VISUAL_PUBLISHED === 'true'; no-report adversarial checks omit /accept-visual. |
no finding |
| Evidence preflight must fail closed on gh-pages | preserves |
visualAcceptanceEvidencePath requires positive integers + full lowercase SHA; authorize checks repos.getContent on gh-pages before permission acceptance. |
no finding |
| Maintainer-only authorization unchanged | preserves | collaborator permission uses user.permissions.maintain/admin; raw admin provenance alone does not authorize. |
no finding |
| Stale/malformed/missing-evidence commands need guidance | preserves | workflow refusal strings tell the commenter to use the latest report, wait for publication, rerun CI if needed, or provide a real pixel reason. | no finding |
| No untrusted PR code in privileged context | preserves | privileged authorize/accept jobs check out default-branch code; PR-built artifacts are only read through trusted publisher/identity checks. | no finding |
| Changed files are not formatted | settled | Prettier check names two changed files. | request changes |
WHY 1: A visual-acceptance command before published evidence lets a maintainer copy a command that cannot yet prove which immutable frames it accepts. WHY 2: Maintainers need the command to correspond to a real report they can inspect. WHY 3: The release gate should only record deliberate visual decisions against trusted evidence. USER-FACING PROBLEM: Maintainers reviewing changed pixels could accept stale or unpublished frames. PROBLEM SEVERITY: harmful friction/security guardrail — the command and accept path operate in privileged GitHub workflows. NEW FEATURE CASE: not a new feature. EARLY STOP: clear.
VERDICT: clear
The report formatter withholds the accept command until a trusted report URL exists. The accept workflow resolves the current PR head, checks the latest completed CI run, verifies the exact evidence file on gh-pages, then checks effective maintainer permission before recording anything. Refusal paths now tell the commenter what to do next.
SOLUTION (2 decisions · workflow/tooling-only)
- Gate command display and recording on published immutable evidence.
- Preserve maintainer-only authorization while improving refusal guidance.
BURDEN: medium — CI workflow authority, status/label mutation, and gh-pages evidence coordination. BURDEN MATCH: proportionate — this is the gate that records stable visual decisions.
VERDICT: clear
OWNER: visual acceptance CI workflow.
TIER 1: GitHub Actions, PR comment publisher, gh-pages publisher.
TIER 2: none.
SEAMS: buildVisualSection, visualAcceptanceEvidencePath, visual-acceptance.yml authorize/accept jobs.
BEHAVIOR UNIT: pure helper plus workflow authorization step — focused tests and static/actionlint checks cover it.
COMPLEXITY BUDGET: one evidence identity helper; no new privileged checkout of PR code.
ACTUAL BURDEN: 7 files, 105 insertions / 9 deletions.
BURDEN TREND: flat — tightens existing flow.
RESET TRIGGER: not triggered.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| visual evidence identity | trusted PR comment publisher and gh-pages path | report URLs, evidence.json | none found |
| visual acceptance authorization | GitHub collaborator permission capabilities | acceptance record/status/label | none found |
VERDICT: clear
Internal maintainers get a safer visual review flow: the command appears only when the report exists, stale commands point back to the latest report, and non-maintainers still cannot accept stable visual changes.
VERDICT: clear
No public package API change.
VERDICT: clear
No theming change.
VERDICT: clear
No consumer-facing breaking change.
VERDICT: clear
Adds one GitHub contents lookup during /accept-visual authorization. That is proportionate and only runs on explicit maintainer commands.
VERDICT: clear
WHY: Non-rendering CI/workflow guidance; no UI component visual obligation. EVIDENCE: Changed files are workflow/scripts only, and the visual output under review is text in PR/report comments.
VERDICT: clear
Existing safe pattern is reused: trusted default-branch workflow code resolves PR identity, publishes immutable gh-pages evidence, and only then projects status/labels back to the PR.
VERDICT: clear
No user-facing component or localized runtime string change. New strings are GitHub workflow/report guidance.
VERDICT: clear
DISPOSITION: comment — semantic request changes because the draft PR cannot receive a request-changes review. GOAL: met — security/guidance behavior passes focused and adversarial checks. ADVICE: run Prettier on the two named changed files. AUTHOR CAN PROCEED: yes. WORST OUTCOME: If merged as-is, the safety behavior works, but the repo's format gate remains red on two changed files.
Semantic verdict: request changes
The visual-acceptance safety behavior looks right: the command is only shown after published immutable evidence, the accept path checks the exact gh-pages evidence before maintainer authorization, stale and malformed commands get actionable messages, and the privileged workflow stays on trusted default-branch code.
Before this undrafts, please run Prettier on the touched visual formatting files. pnpm exec prettier --check .github/scripts/lib/visual-format.js .github/scripts/lib/visual-format.test.mjs fails on this head, so the repo's format gate is red even though the focused behavior checks pass.
[Reviewed by Robohands]
Full review ~5m active validation plus CI wait/checking.
-
npx --yes pnpm@11.10.0 install --frozen-lockfile --ignore-scriptspassed; no package/lockfile changes, scripts ignored. - Focused Vitest: 3 files, 66 tests passed.
- Adversarial Node checks passed for no pre-publication command, invalid evidence identities, stale/missing guidance strings, maintainer capability handling, and trusted workflow structure.
-
actionlint .github/workflows/visual-acceptance.ymlpassed;pr-comment.ymlpassed when ignoring the existing custom2-core-ubuntu-armlabel warning. -
git diff --check origin/main...HEADpassed. -
pnpm check:repopassed. - Prettier failed on
.github/scripts/lib/visual-format.jsand.github/scripts/lib/visual-format.test.mjs. - Exact-head CI was still running during review; several jobs had passed and no exact-head CI failure was observed before posting.
posted as drafted
https://github.com/facebook/astryx/pull/5791#pullrequestreview-5068847739
aa8458511112b3b1f5423b1c3c8864f15aee250d
The prior review requested formatting only. The safety behavior was already verified, but .github/scripts/lib/visual-format.js and .github/scripts/lib/visual-format.test.mjs failed Prettier.
Only the two formatting files changed since 3881003490098fb3a96a906b8559e71e8bd2008a. The formatting-only diff preserves the security behavior already reviewed.
- Diff since prior reviewed head: only
.github/scripts/lib/visual-format.jsand.github/scripts/lib/visual-format.test.mjschanged. - Focused Vitest: 3 files, 66 tests passed.
- Prettier check passed on all touched files.
-
actionlint .github/workflows/visual-acceptance.ymlpassed;pr-comment.ymlpassed with the existing custom-runner-label warning ignored. -
git diff --check origin/main...HEADpassed. -
pnpm check:repopassed. - Adversarial checks passed again for no pre-publication command, invalid evidence identities, stale/missing guidance strings, maintainer capability handling, and trusted workflow structure.
- Current exact-head CI at review time: Spec owner gate and Internal Registry passed; Lint, CLI Smoke Test, CI, visual-acceptance, and Vercel were still pending/in progress.
Semantic verdict: approve once undrafted
The formatting blocker from the last review is fixed. I rechecked the touched visual-acceptance path: the command still only appears after immutable evidence is published, the accept workflow still checks the exact gh-pages evidence before maintainer authorization, stale/malformed/missing-evidence commands still give actionable guidance, and the privileged workflow still runs trusted default-branch code rather than PR code.
[Reviewed by Robohands]
https://github.com/facebook/astryx/pull/5791#pullrequestreview-5068888760
posted as drafted