Review 5631

Review 5631 — route visual gh-pages writers through publisher

PR

#5631 ci: route visual gh-pages writers through publisher by cixzhang (bucket: the maintainer)

HEAD REVIEWED

311ba8cdc3c0f3e94207d86bc151559e8902758a — every claim below was verified at this commit, whose sole parent is approved #5629 head 88195bb8e6765f7fd10fd5abf7521e09d157bc42.

VERSIONS

LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14.1

LANE

LANE: full WHY: this is a privileged workflow migration spanning four publication paths, two queue implementations, cross-run liveness, retries, immutable evidence, and a stacked successor contract.

PROBLEM

WHY 1: Independent workflows can mutate overlapping gh-pages paths concurrently, and Actions concurrency does not preserve every pending writer. WHY 2: A lost evidence, acceptance, or baseline write leaves maintainers without the durable review state needed to accept or release visual changes. WHY 3: Visual review is only trustworthy when every accepted artifact remains immutable and every baseline mutation has one serialized owner.

USER-FACING PROBLEM: A maintainer handling overlapping visual workflows can lose evidence, acceptance, or baseline state even though each individual workflow reports a locally valid publication attempt. PROBLEM SEVERITY: broken task — missing evidence or split publication ownership can prevent visual acceptance or publish an unreviewed baseline state. NEW FEATURE CASE: not a new product feature — this extends the repository-owned publication subsystem approved in #5629 to the visual workflow class. EARLY STOP: clear — the repository owner authored the migration and the prior review established the shared-publisher need and model.

VERDICT: clear

SOLUTION

Visual evidence, acceptance records, accepted baseline promotion, and manual baseline promotion are moved from independent shell publishers onto the shared FIFO publisher. Each operation re-clones the latest branch state on a rejected push, preserves unrelated paths, and keeps immutable records write-once. The workflows continue to resolve trusted identities before privileged publication.

SOLUTION (4 migration decisions · 529 added runtime/script lines of 869 additions)

1. Publish immutable PR visual evidence through the shared queue and refresh the state checkout afterward.
2. Archive visual acceptance records through the same publisher while preserving trusted identity inputs.
3. Promote accepted post-merge pixels only while holding the shared baseline turn.
4. Move manual baseline promotion onto the same turn and retry model.

BURDEN: high — four privileged mutation adapters, one shared queue, two retained executable queue state machines, five-attempt publication retries, and 22 focused tests across publisher and workflow contracts. BURDEN MATCH: disproportionate at this head — the migration intends to contract publication ownership, but it leaves the retired baseline lock executable and independently writable.

VERDICT: BLOCKS — the migration does not yet leave one publication-order authority.

ARCHITECTURE

OWNER: the shared gh-pages publisher should own every publication turn and branch mutation. TIER 1: the publisher accepted in #5629, with GitHub Actions run state as the liveness authority. TIER 2: none. SEAMS: immutable evidence, acceptance archive, accepted baseline promotion, manual baseline promotion, stale-run pruning, rejected-push retry, and roll-forward from the former baseline lock. BEHAVIOR UNIT: pure utility/module — publisher operations are exported and driven against local bare remotes; workflow contracts are checked as text. COMPLEXITY BUDGET: one publication-order owner, one durable queue/holder representation, four migrated operation adapters, and no second writable lock protocol. ACTUAL BURDEN: 7 changed files; 529 added publisher lines; 1 shared publisher plus 1 retained executable baseline-lock implementation; 2 durable queue roots; 4 migrated workflows; 22 focused tests. BURDEN TREND: parent 88195bb8 → current 311ba8cd: grew by four migration adapters, while publication-order representations remain at two instead of contracting to one. RESET TRIGGER: not triggered — this is the first model-level finding on this PR, and the contraction direction is derivable: one queue must govern both old and new runs during rollout.

domain fact	one authoritative writable source	generated / immutable projections	other writable copies
publication order and holder	.astryx-gh-pages/publication-queue	run id/scope in logs	visual-gate/publication-queue remains writable through the old executable — BLOCKS
workflow-run liveness	GitHub Actions run state	immutable run id in each ticket	none; the migrated acceptance job lacks permission to read it — BLOCKS
visual evidence identity	immutable pr/<pr>/visual/<head>/<run>/<attempt> path	report URLs and status projection	none
acceptance/baseline identity	immutable acceptance record plus recaptured merge manifest	projected status and decision log	none

seam	driven result
immutable evidence repeat	identical bytes are idempotent; different bytes refuse in the local integration test
acceptance archive	acceptance record and reviewed image archive publish through the shared queue in the local integration test
accepted baseline	accepted pixels promote only after a shared-turn assertion in the local integration test
active shared-queue blocker	waitForPublicationTurn() calls the Actions run endpoint; visual-acceptance.yml’s migrated accept job grants no actions: read
old/new rollout	a local bare-remote probe acquired a shared-queue holder for run 100 and an old baseline-queue holder for run 101 simultaneously
successor writers	preview deploy, redeploy, cleanup, and vibe screenshot writers remain outside the shared queue; the next migration must preserve pr/<n>/visual/** and use the one authority rather than add another queue

VERDICT: BLOCKS — two independent queue roots can both grant a publication turn, and one newly migrated caller cannot query the shared queue’s liveness authority.

IMPACT

An acceptance command that overlaps another queued publisher reaches the shared wait path without Actions read permission and exits before recording the decision. During roll-forward, an already-running old baseline workflow and a new workflow can each acquire a different holder and mutate the same branch without seeing the other. NEW FEATURE IMPACT: maintainers should gain one durable publication order for every visual artifact; this head centralizes the happy path but does not yet make contention or rollout single-owner.

VERDICT: BLOCKS — the defining overlap and migration states remain unsafe.

API

No package or consumer API changes. The repository-internal publisher CLI adds enqueue, wait, release, immutable-path, visual-acceptance-record, visual-baseline-accepted, and visual-baseline-manual; the visual workflows are their only new callers.

	change	public?	class	doc’d?	verdict
+	shared queue lifecycle commands with explicit scope	no — repository workflow contract	inherited publisher protocol	workflow call sites/tests	clear
+	immutable evidence and acceptance publication commands	no — repository workflow contract	scoped immutable writers	workflow call sites/tests	clear
+	accepted/manual baseline publication commands	no — repository workflow contract	baseline writers	workflow call sites/tests	clear

OSSIFICATION: no installed consumer can depend on these commands, but repository automation can; the retained old executable is therefore a competing internal contract until removed or made a one-way compatibility boundary.

VERDICT: BLOCKS — publication-order API ownership remains split between two executable CLIs.

THEMING

No CSS, tokens, theme targets, rendered elements, or style contracts change.

VERDICT: clear

BREAKING

BEHAVIOR: intended behavior changes by routing four writers through one queue; overlapping visual acceptance currently fails without Actions read permission, and old/new rollout can grant two holders. API: no consumer API change. VISUAL: no rendered UI change. THEME: no theme change.

VERDICT: BLOCKS — the migrated contention behavior is not operational in every caller and is not single-owner across rollout.

PERFORMANCE & RESOURCES

EFFECTS: zero. RENDER: no application render path changes. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package bundle change; pngjs was already declared. CI cost: each migrated publication adds queue clones/mutations and may wait up to 75 minutes; retry loops are bounded and re-clone current branch state.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: all seven changed files are workflow YAML, Node publication scripts, or tests; no UI, CSS, rendered markup, story, or component behavior changes.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No rendered element, interaction, role, accessible string, locale formatting, or direction-sensitive behavior changes.

VERDICT: clear

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	BLOCKS — migration leaves two publication-order implementations
ARCHITECTURE	BLOCKS — two queue roots can grant ownership; one caller lacks liveness permission
IMPACT	BLOCKS — acceptance overlap and rollout remain unsafe
API	BLOCKS — two executable internal queue contracts remain
THEMING	clear
BREAKING	BLOCKS — migrated contention behavior is incomplete
PERFORMANCE	clear
VISUAL	clear — not applicable
A11Y & I18N	clear — not applicable

GOAL: partly met — local integration tests prove immutable evidence, acceptance archive, accepted baseline promotion, preservation, and retry foundations, but a static permission probe and a driven dual-queue probe prove two contention paths remain broken. DISPOSITION: missing actions: read in the migrated acceptance job → blocks now; independently writable old/new queue roots during rollout → blocks now; absent exact-head repository CI on this stacked draft → report, but focused local evidence settles the two findings. ADVICE: bounded outcome criteria — every shared-publisher caller must retain the permission needed to inspect blocking runs, and old plus new workflow versions must never be able to hold publication ownership simultaneously; lock both contracts with focused tests. AUTHOR CAN PROCEED: yes — add the missing permission and make rollout converge on one authoritative queue, with a regression test that fails when old and new holders can coexist. WORST OUTCOME: “An old and a new publisher can each believe it owns baseline publication and push conflicting visual state.” → request changes.

JUDGEMENT NEEDED: none — both findings are reproducible correctness defects with bounded acceptance criteria.

request changes (self-authored fallback COMMENT)

1. The migrated visual-acceptance job calls the shared wait path without Actions read permission. → when an acceptance overlaps any active publisher, it exits before recording the maintainer’s decision · .github/workflows/visual-acceptance.yml:144

2. The migration leaves the former baseline lock executable against a different queue root. → a pre-merge run and post-merge run can both hold publication ownership and race the same baseline · .github/scripts/visual-gate/lib/baseline-publication-lock.mjs:15

REVIEW

Semantic verdict: request changes

Thanks for moving the visual writers behind the shared publisher. Two concurrency gaps still leave the migration unsafe:

• visual-acceptance.yml:144 now enters the shared wait path, but its accept job grants no actions: read; an acceptance overlapping any publisher exits before recording.
• The executable former baseline lock still writes a separate queue. A local roll-forward probe acquired old and new holders simultaneously, so pre-merge and post-merge runs can each believe they own baseline publication.

Could we retain the required permission for every queued caller, leave one authoritative queue across rollout, and cover both contracts before stacking the next migration?

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

None — both defects cross a workflow permission block and publication-owner boundary.

EVIDENCE I DID NOT SPEND

• The four new publisher operations preserve unrelated sparse paths and rebuild their commit from a fresh clone after a rejected push.
• The acceptance reason and visual identities remain argument-vector data; no new shell interpolation, dependency, package hook, or off-registry resolution was added.
• Preview deploy/redeploy currently replace pr/<n> wholesale and therefore remain part of the next migration’s preservation contract; this is inherited behavior and is not charged to this delta.

TIME

TIME total 10m

• setup/rules: 3m — fresh loop 1.9.1, audit 1.14.1, prior round, feedback, untrusted-code preflight, and exact-head setup
• worktree/install: 1m — dedicated detached checkout and lockfile-matched install (3.9s combined)
• build/server: 0m — not applicable
• browser/a11y: 0m — non-rendering change
• focused tests: 2m — publisher/workflow Vitest, syntax, and actionlint
• code/history: 2m — permissions, state ownership, retry/idempotency, stack base, successor seam, and two local probes
• critique/wiki: 2m — presentation, critic pass, delivery, record/index update
• CI wait: 0m — no repository Actions run exists for this stacked exact head
• waste: 1m — one unavailable search binary and one mirror-hash command were retried with portable tools

WHAT I COULD NOT VERIFY

• A live overlapping GitHub Actions publication was not dispatched because that would mutate shared gh-pages; both failure paths were verified with static workflow contracts and local bare remotes.
• Full repository CI has not run for this stacked draft head.

Current CI

At exact head 311ba8cdc3c0f3e94207d86bc151559e8902758a, GitHub reports Meta CLA and Vercel Preview Comments green plus the Vercel status green. Posting the review triggered Review signal run 33150530911, whose two jobs were skipped; no substantive repository CI/test workflow has run because the draft is stacked on the open #5629 branch.

Focused evidence

• Vitest: 2 files, 22/22 tests passed in 84.68s.
• Syntax: all three changed Node files pass node --check.
• Actionlint 1.7.12: all four changed workflows pass with the repository’s documented runner/shellcheck ignores.
• Permission contract: every new shared-publisher caller has actions: read except visual-acceptance.yml’s accept job.
• Roll-forward probe: run 100 acquired .astryx-gh-pages/publication-queue/holder.json while run 101 simultaneously acquired visual-gate/publication-queue/holder.json against the same local bare remote.
• Security preflight: all seven changed workflow/script/test files were read before checkout or execution; no dependency, package hook, lockfile, or executable-mode change; focused tests use local bare remotes and a fake gh binary.

What changed before posting

Posted as drafted after one critic pass; the public review is 107 words.

Delivery

GitHub review: https://github.com/facebook/astryx/pull/5631#pullrequestreview-5048724688