Review 5633

Review 5633 — route remaining gh-pages writers through publisher

PR

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

HEAD REVIEWED

33bbe50edd5cd3b665440b43cad3ba63a143b0ab — every claim below was verified at this exact head. Its sole parent is approved #5631 head 29dafeba0e6cef644c36bd9e99725baa09259dfe, whose sole parent is approved #5629 head 88195bb8e6765f7fd10fd5abf7521e09d157bc42.

VERSIONS

LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14.4

LANE

LANE: full WHY: this is a privileged multi-workflow migration in a stacked publication subsystem. Trust boundaries, global concurrency, retention, retries, terminal cleanup, and five workflow call sites exclude the fast lane.

PROBLEM

WHY 1: Preview, cleanup, compaction, and vibe screenshot publishers still mutate overlapping parts of gh-pages outside one durable authority. WHY 2: An overlap can fail a publication, strand queue ownership, target the wrong preview path, or leave retained data unmanaged. WHY 3: Maintainers rely on every published preview, report, baseline, and acceptance artifact remaining available and attributable to the run that produced it.

USER-FACING PROBLEM: A maintainer running or inspecting overlapping repository workflows can receive no preview, stale retained data, or a publication blocked behind ownership that should have been released. PROBLEM SEVERITY: broken task — multiple migrated workflows fail outright or cannot serialize under the state the shared publisher is designed to handle. NEW FEATURE CASE: not a product feature — this extends the repository-owned publisher approved in #5629 and #5631 to the remaining writer class. EARLY STOP: clear — the approved parents establish the need; this round judges whether the incremental migration preserves their contract.

VERDICT: clear

SOLUTION

Every migrated job is meant to enter the same durable publication queue, update only the subtree it owns, retry from the latest branch tip, and release both rollout authorities when it finishes. Preview identity must come from the trusted workflow run rather than from the artifact being published. Cleanup must keep its existing path and age policies while compaction owns the whole tree exclusively.

SOLUTION (4 runtime decisions · 481 added publisher lines in a 728-addition / 434-deletion diff)

1. Publish automatic and manual PR previews through one pr/<number> subtree writer.
2. Reconcile stale previews, stale root artifacts, duplicate template assets, and report screenshot retention through the shared queue.
3. Publish vibe screenshot subtrees through the same queue.
4. Compact the whole branch under the same exclusive authority and remove its own queue state in the compacted commit.

BURDEN: high — 9 changed files, 5 workflow call sites, 4 new publication operations, shared queue/liveness/security contracts, and 5 new focused tests. BURDEN MATCH: proportionate in intent — these are the remaining overlapping writers — but incomplete caller contracts and one remaining direct writer leave the centralization goal unmet.

VERDICT: BLOCKS — preview execution, trusted identity, liveness permissions, retention, direct-writer exclusivity, and compaction terminal cleanup are not all preserved.

ARCHITECTURE

OWNER: the shared gh-pages publisher owns every current publication turn and each command owns only its declared subtree; trusted workflows own PR identity. TIER 1: the global run-id FIFO, dual-queue rollout bridge, and GitHub Actions run liveness approved in #5629 and #5631. TIER 2: none. SEAMS: automatic fork preview, manual redeploy, closed-preview cleanup, 30-day report retention, vibe report/screenshot coexistence, direct/manual writers, whole-tree compaction, rejected push retry, no-op rerun, and terminal release. BEHAVIOR UNIT: pure utility/module — publishers and queue operations are executable against isolated bare remotes; workflow contracts are repository text. COMPLEXITY BUDGET: one current publication authority, one trusted PR identity source, one owner per mutable subtree, no direct writer outside the queue, atomic claim/release across both rollout roots, and one focused regression per migrated contract. ACTUAL BURDEN: 9 files; 728 additions / 434 deletions; 481 added publisher lines; 5 workflow call sites; 2 queue roots; 4 publisher integration tests plus 1 workflow-routing test. Three callers omit the liveness permission, two callers omit trusted code checkout, auto-preview omits its ready guard, compaction clears one queue root, and one vibe report writer remains outside the queue. BURDEN TREND: approved parent 29dafeba → 33bbe50e grows only for four stated migrations, but caller-specific wiring and cleanup exceptions add multiple unenforced representations of the parent contract. RESET TRIGGER: triggered — this second successor still encodes the shared publisher contract separately at each caller, and the resulting permission, identity, checkout, release, and direct-writer gaps require one contract-driven migration boundary rather than more local guards.

domain fact	one authoritative writable source	generated / immutable projections	other writable copies
current publication order	shared run-id FIFO plus temporary same-commit legacy projection	run id/scope in logs	deploy-report.ts still writes gh-pages directly — BLOCKS
workflow liveness	GitHub Actions run state	immutable run id in both queue tickets	three migrated jobs cannot read it — BLOCKS
automatic preview identity	workflow-run head repository/branch resolved through the Pull Requests API	artifact identity cross-check	pr-meta.json currently chooses the destination and artifact hash — BLOCKS
cleanup retention policy	open-PR API plus path history	deletion list replayed on each fresh tip	depth-1 history cannot identify an old screenshot subtree — BLOCKS
compaction terminal ownership	both queue roots cleared in the compacted tree	compact commit contains no current holder/ticket	only the shared root is cleared — BLOCKS

seam	driven result
PR preview subtree	replacement preserves report, baseline, visual evidence, and concurrent unrelated writes; a second identical publish is a no-op — pass
manual redeploy	resolves the requested PR’s current head/run before artifact download — identity pass; publisher script is absent because no trusted checkout occurs — fail
automatic fork preview	artifact inputs remain static and scoped to pr/<number>, but the job neither resolves trusted PR identity nor cross-checks artifact PR/head/run — fail
cleanup subtree	closed preview and legacy hash deletion preserve live previews, immutable visual evidence, and concurrent unrelated writes; a second cleanup is a no-op — pass
30-day report retention	a screenshot committed 58 days before the current tip survives because the depth-1 clone exposes no old path commit — fail
vibe screenshot subtree	screenshot replacement preserves report HTML, previews, baselines, and concurrent unrelated writes; a second identical publish is a no-op — pass
vibe report coexistence	internal/vibe-tests/src/deploy-report.ts still clones and directly pushes the same reports/<id> scope — fail
whole-tree compaction	force-with-lease retry preserves a concurrent file and all preview/report/baseline paths in a one-commit result — pass
compaction terminal cleanup	the compacted result removes the shared ticket/holder but retains the matching legacy ticket/holder — fail
queued overlap	parent 32-test queue matrix plus current 37-test suite cover both current orders, simultaneous claim, retry, atomic ordinary release, and stale holder cleanup — runtime pass when caller permissions exist
workflow caller contract	executable census finds missing trusted checkout on 2 preview jobs, missing ready guard on automatic preview, and missing actions: read on cleanup/compact/vibe — fail

VERDICT: BLOCKS — publication order, identity, retention, and terminal state have more than one effective source or an unenforced caller contract. Contraction requires every writer to enter one publisher API, every queued caller to receive its required permissions and trusted code, preview paths to derive from trusted run identity, and both rollout roots to be cleared atomically.

IMPACT

Automatic and manual previews invoke a script absent from their workspaces, so their publish steps fail before writing. Under overlap, cleanup, compaction, and vibe screenshots cannot query the current holder; successful compaction leaves legacy ownership behind; the direct vibe report deployer remains unsynchronized. A fork-produced metadata artifact can still select the preview destination without the repository’s established trusted identity cross-check, and old screenshots do not age out under the intended 30-day rule. NEW FEATURE IMPACT: not a new product feature — maintainers should gain reliable, isolated publication across the remaining writers, but these reachable paths do not yet receive it.

VERDICT: BLOCKS — the defining migration outcome is absent in several ordinary and adversarial states.

API

No installed-consumer or package API changes. The repository-owned CLI adds pr-preview, cleanup-previews, vibe-screenshots, and compact commands; their path inputs reject invalid PR numbers and traversal.

	change	public?	class	doc’d?	verdict
+	four repository publication commands	no — CI/operator surface	existing shared publisher command family	usage string + workflows/tests	note — caller contract is incomplete

OSSIFICATION: this is internal repository infrastructure, but its command-to-scope map and required caller capabilities become the shared contract for every later migration. The architecture findings must be fixed before more writers depend on it.

VERDICT: note — validation is sound; the blocking identity/permission/ownership gaps belong to ARCHITECTURE.

THEMING

No CSS, token, theme target, rendered element, or style contract changes.

VERDICT: clear

BREAKING

BEHAVIOR: yes — both preview jobs cannot locate the publisher; automatic preview no longer skips absent artifacts; three jobs cannot resolve queued blockers; 30-day screenshot retention is ineffective; compaction publishes a stale legacy owner. API: no installed-consumer API change. VISUAL: no rendered UI change; only publication availability and repository state change. THEME: no theme change.

VERDICT: BLOCKS — existing preview, cleanup, compaction, and vibe publication behavior regresses or remains outside the promised shared authority.

PERFORMANCE & RESOURCES

EFFECTS: zero. RENDER: no application render path changes. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package change. CI/runtime cost: queue waiting adds GitHub run-status reads and shallow branch clones by design; retries are bounded. The migrated writers passed local rejected-push retry and no-op/idempotency probes.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: the exact diff changes workflow YAML, shell/Node publication scripts, and tests. It changes no UI, CSS, DOM, story, or rendered component path.

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 contract incomplete
ARCHITECTURE	BLOCKS — identity/order/retention/terminal sources remain split
IMPACT	BLOCKS — reachable jobs fail or remain unsafe
API	note — internal commands only
THEMING	clear
BREAKING	BLOCKS — publication behavior regresses
PERFORMANCE	clear
VISUAL	clear — not applicable
A11Y & I18N	clear — not applicable

GOAL: not met — 37/37 focused tests pass, but independent exact-head probes reproduce missing workflow code/guards/permissions, retained legacy compaction ownership, ineffective 30-day retention, and a direct writer outside the queue. DISPOSITION: preview checkout/guard failure → blocks now; missing liveness permissions and compaction dual-root cleanup → blocks now; direct vibe report writer → blocks now; automatic preview trusted identity gap → blocks now; 30-day retention failure → blocks now. ADVICE: proven existing pattern + bounded outcome criteria — mirror pr-comment.yml’s trusted workflow-run/API identity resolution and artifact cross-check; preserve the parent’s actions: read caller contract; route every writer through the shared queue; clear both queue roots in the compacted tree; prove retention with history older than the cutoff. AUTHOR CAN PROCEED: yes — the acceptance criteria are explicit and do not require a new human-owned design choice. WORST OUTCOME: “both preview jobs fail before publication, while overlapping or terminal writers can remain outside one enforceable owner” → request changes.

JUDGEMENT NEEDED: none — these are reproducible correctness and security-boundary defects against the approved parent contract.

request changes (self-authored fallback COMMENT; no merge or auto-merge)

1. Preview execution and identity are not bound to trusted default-branch code and run identity → maintainers get failed previews, while a fork artifact remains authoritative for another PR’s destination · .github/workflows/deploy-preview.yml:44-131, .github/workflows/redeploy-preview.yml:104-112

2. Shared publication authority is not exclusive or terminal → overlaps fail without liveness permission, compaction leaves a legacy owner, and manual vibe report publication bypasses the queue · .github/workflows/cleanup-previews.yml:58-70, .github/scripts/lib/gh-pages-publisher.mjs:1903-1968, internal/vibe-tests/src/deploy-report.ts:157-249

3. The 30-day screenshot policy cannot observe old path history → report screenshots remain indefinitely while cleanup reports success for its other deletions · .github/scripts/lib/gh-pages-publisher.mjs:1651-1666

REVIEW

Semantic verdict: request changes

Thanks for moving these writers behind one publisher. Three boundaries still break it:

• deploy-preview.yml:123 and redeploy-preview.yml:104 call a publisher they never check out, so every deploy fails; automatic preview also lost its artifact-ready guard.
• The publisher contract remains per-caller: cleanup, compaction, and vibe lack actions: read; compaction clears only one queue; deploy-report.ts still pushes outside it.
• Automatic preview trusts pr-meta.json for PR/path identity instead of cross-checking the run like pr-comment.yml. Cleanup’s retention check also leaves a 58-day-old screenshot past its 30-day policy.

Could we restore trusted checkout/guards, give every caller the same contract, bind preview identity to the run/API, and route every write and cleanup through that authority?

[Reviewed by Robohands]

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

None — the three findings cross workflow/module boundaries and belong in the summary.

EVIDENCE I DID NOT SPEND

• Preview, cleanup, vibe screenshots, and compaction each preserved their owned/unowned paths through an injected rejected push; repeated preview/vibe publication was a no-op and repeated cleanup deleted zero items.
• The repository-wide writer census found no direct writer in the five migrated workflow files, but did find the reachable report:deploy package command ending in git push origin gh-pages for the overlapping vibe report subtree.
• No dependency, lockfile, package hook, rendered path, private residue, or unrelated main change appears in the exact delta.

TIME

TIME total 18m

• setup/rules: 4m — loop 1.9.1, official audit 1.14.4, public kit, parent approval rounds, exact metadata, and untrusted-code preflight
• worktree/install: 1m — dedicated detached checkout and lockfile-matched dependency clone/reconcile
• build/server: 0m — not applicable
• browser/a11y: 0m — non-rendering change
• focused tests: 3m — 37/37 focused tests, actionlint, syntax, diff-check, and check:repo
• code/history/probes: 5m — caller/permission/writer census plus isolated retry, idempotency, subtree, retention, and compaction probes
• critique/wiki/delivery: 5m — presentation, two critic passes, exact-head recheck, fallback COMMENT, record/index updates, and readback
• CI wait: 0m — no substantive Actions run exists at this stacked head
• waste: 2m — one shell variable shadowed the command path and the first writer census matched JavaScript .push() calls

WHAT I COULD NOT VERIFY

• A live GitHub Actions overlap was not dispatched because that would mutate shared gh-pages; production publisher code and schemas were driven against isolated bare remotes instead.
• Full repository CI has not run for this stacked exact head.

Current CI and landing gate

At exact head 33bbe50edd5cd3b665440b43cad3ba63a143b0ab, Meta CLA, Vercel Preview Comments, and Vercel are green; GitHub reports zero Actions workflow runs for the head. That absence is separate from the code verdict. Landing remains gated in order: #5629 lands and #5631 receives substantive exact-head CI and lands; then this exact head must receive substantive GitHub Actions CI before landing.

Required invariant evidence

• Owned-subtree preservation: pass in isolated bare remotes for automatic/manual preview’s shared implementation, cleanup, vibe screenshots, and whole-tree compaction; concurrent unrelated commits survived each injected push rejection.
• Fork PR identity and isolation: fail for automatic preview — destination PR/hash come from pr-meta.json with only syntax validation; unlike pr-comment.yml, no trusted workflow-run/API resolution or artifact identity cross-check exists. Manual redeploy resolves the current PR head/run correctly.
• Retention semantics: fail — live previews, visual evidence, report HTML, and unrelated roots are preserved, and closed/legacy previews are removed; a 58-day-old report screenshot survives because the publisher’s depth-1 clone cannot resolve its old path commit.
• Exclusive compaction under shared authority: fail — queue acquisition and force-with-lease retry are sound, but compaction publishes its legacy ticket/holder and deploy-report.ts still writes the same branch outside the authority.
• Zero direct writers in migrated scope: fail — workflow files no longer push directly, but internal/vibe-tests/src/deploy-report.ts remains reachable through report:deploy and directly pushes gh-pages.
• Permissions/security: fail — cleanup, compaction, and vibe screenshot jobs omit actions: read; both preview jobs omit trusted publisher checkout; automatic preview omits the established trusted identity cross-check.
• Concurrency/retry/idempotency/terminal cleanup: mixed — parent queue matrix and exact-head writer probes pass ordering, retry, unrelated-write preservation, and repeat no-ops; compaction terminal cleanup fails for the legacy root.
• Stack assumptions: pass — 33bbe50e has sole parent 29dafeba, which has sole parent 88195bb8; current main has not changed any reviewed file since the approved bottom parent.

Focused evidence

• Checked-in Vitest: 2 files / 37 tests passed in 64.65s.
• Writer matrix: preview, cleanup, vibe screenshots, and compaction each retried after an injected rejected push and preserved unrelated content; preview/vibe second publish returned published: false; second cleanup returned deleted: 0.
• Compaction cleanup probe: one-commit tree and content preservation passed; shared ticket/holder were absent, legacy ticket/holder remained present.
• Retention probe: closed preview and legacy hash directory were removed; live preview and report HTML survived; the 58-day-old screenshot remained.
• Workflow contract probe: trusted checkout false for both preview jobs; automatic artifact-ready guard false; actions: read false for cleanup, compaction, and vibe screenshots.
• Repository checks: actionlint passed 11 relevant workflows; Node syntax passed both publisher entrypoints; shell syntax passed the wrapper; git diff --check and pnpm check:repo passed.
• Security preflight: all nine changed workflow/script/test files were read before execution; no dependency, lockfile, package-hook, or off-registry change exists.

What changed before posting

Posted as drafted after two critic passes; the public fallback comment is 118 words.

Delivery

GitHub review: https://github.com/facebook/astryx/pull/5633#pullrequestreview-5050154867

Cleanup

No long-lived process was started. The dedicated code/wiki worktrees and scratch probes are removed at run completion.

Round 2 — f07522f4

PR

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

HEAD REVIEWED

f07522f4127d9630f17f77d458bf075f3eef7019 — every claim below was verified at this exact head. Its sole parent is approved #5631 head 29dafeba0e6cef644c36bd9e99725baa09259dfe, whose sole parent is approved #5629 head 88195bb8e6765f7fd10fd5abf7521e09d157bc42.

VERSIONS

LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14.4

LANE

LANE: full WHY: this is a privileged migration re-review with an unresolved prior review, trusted workflow_run identity, artifact isolation, retention history, dual-queue compaction, report-byte precedence, and a stacked landing gate.

PROBLEM

WHY 1: Preview, cleanup, compaction, and vibe report writers must publish overlapping gh-pages state without trusting fork metadata or losing another writer’s content. WHY 2: A skipped readiness check or wrong conflict winner can turn an expected clean skip into a failed job or silently retain stale report bytes. WHY 3: Maintainers rely on previews and report screenshots matching the exact successful run while every independent publication remains available.

USER-FACING PROBLEM: A maintainer inspecting a draft, stale, or artifact-missing CI run can receive a red deploy instead of a clean skip, and a newly generated vibe screenshot can remain old after a successful report publication. PROBLEM SEVERITY: broken task — automatic deployment still executes outside its readiness predicate, and one migrated writer can publish stale bytes for a path the current report supplied. NEW FEATURE CASE: not a product feature — this completes the repository-owned publication migration approved in #5629 and #5631. EARLY STOP: clear — the approved parents and prior round establish the need; this round judges the named repair invariants.

VERDICT: clear

SOLUTION

The trusted workflow first resolves one current open, non-draft PR from GitHub’s API, then cross-checks the artifact before downloading preview payloads. The shared publisher now owns retention, report publication, screenshots, and whole-tree compaction, while every queued workflow has liveness access. The final deploy step still ignores the readiness outputs, and report conflict resolution lets old remote screenshots overwrite matching new local bytes.

SOLUTION (5 delta decisions · 8 files / +420 −141 since the blocked head)

1. Resolve automatic preview identity and eligibility from trusted API state before artifact cross-check.
2. Clone full history for the 30-day screenshot decision.
3. Remove terminal legacy migration state during exclusive compaction.
4. Route the direct vibe report writer through the shared publisher.
5. Grant the remaining queued jobs actions: read and extend the workflow contract tests.

BURDEN: high — the full PR changes 10 files / +1,145 −572 across five workflows, one shared publisher, one report caller, two queue roots, trusted API/artifact boundaries, and 43 focused tests. BURDEN MATCH: proportionate in ownership, but incomplete in behavior — the added resolver and report migration do not yet preserve their final readiness and current-source-wins contracts.

VERDICT: BLOCKS — the prior readiness and direct-writer migration acceptance criteria remain only partly satisfied.

ARCHITECTURE

OWNER: the shared gh-pages publisher owns publication order and mutable subtree writes; trusted default-branch workflows own PR identity and readiness. TIER 1: the global run-id FIFO, temporary same-commit legacy projection, and Actions run liveness approved in #5629 and #5631. TIER 2: none. SEAMS: successful current PR, draft PR, stale head, wrong repository, failed CI, missing metadata, forged metadata, missing preview payloads, 29/30/58-day retention, report/screenshot collision, rejected push, subtree preservation, terminal queue cleanup, and whole-tree compaction. BEHAVIOR UNIT: pure utility/module plus workflow contract — trusted resolution is driven with mocked API state; publisher operations are driven against isolated bare remotes. COMPLEXITY BUDGET: one publication owner, one trusted PR identity source, one final readiness predicate, one current-source-wins rule with remote fill-in only for absent report paths, one retention source, and no payload writer outside the publisher. ACTUAL BURDEN: 10 full-PR files; +1,145 −572; 5 changed workflows; 1 shared publisher module; 2 queue roots; 43 checked-in focused tests; old→new repair delta 8 files / +420 −141. The trusted resolver has three intermediate readiness outputs but the final deploy step consumes none; report staging has two byte sources with the old remote copy applied last. BURDEN TREND: 33bbe50e → f07522f4 grows for the requested resolver, full-history retention, terminal cleanup, report migration, permissions, and tests; writable ownership contracts to one publisher, but final readiness and same-path byte precedence remain split. RESET TRIGGER: not triggered — the fix adds no new owner or durable representation; both blockers have derivable contraction criteria inside the existing workflow and publisher.

domain fact	one authoritative writable source	generated / immutable projections	other writable copies
preview PR/head/repository	trusted Pull Requests API matched to workflow_run	artifact PR/head/run cross-check	none
preview readiness	trusted, artifact, and payload readiness outputs	step logs	final deploy ignores all three — BLOCKS
publication order	shared run-id FIFO with temporary same-commit legacy projection	run id/scope logs	no payload writer outside the publisher
report screenshot bytes	current report source, with old remote bytes filling only absent paths	published report subtree	old remote copy overwrites matching current source — BLOCKS
retention cutoff	full gh-pages path history and fixed current time	deletion list replayed on fresh tip	none
compaction terminal state	shared active queue after removing the compactor	one-commit compacted tree	terminal legacy queue removed

seam	driven result
trusted current PR	resolves exact PR/head/repository and proceeds — pass
draft / failed CI	resolver returns ready=false, but unguarded final deploy still runs — fail
stale head / wrong repository	trusted resolver fails closed — pass
forged artifact PR/head/run	each adversarial artifact exits before payload download — pass
missing metadata or preview payload	intermediate readiness becomes false, but final deploy still runs — fail
retention	58-day screenshots deleted; exactly-30-day and 29-day screenshots, report HTML, live preview, baseline, and an injected concurrent write survive — pass
report migration	direct push removed and unrelated report/preview/baseline paths survive; a same-name local screenshot loses to the old remote byte — fail
compaction	terminal legacy directory removed, active shared successor preserved, forced-lease retry keeps a concurrent write, and final history has one commit — pass
workflow permissions	cleanup, compaction, and vibe screenshot jobs each have actions: read plus contents: write — pass
direct writer census	no payload writer remains outside the shared publisher; the approved legacy module writes only rollout queue metadata — pass

The prior review asked for trusted checkout/readiness, one caller contract, terminal legacy cleanup, direct-writer migration, trusted preview identity, and working 30-day retention. This round confirms the API/artifact identity, permissions, retention, queue cleanup, and direct-writer ownership fixes; it narrows rather than contradicts the prior verdict because the final readiness consumer and report-byte conflict are still wrong.

VERDICT: BLOCKS — readiness and same-path report content still have two effective authorities. Contraction requires the deploy step to consume every readiness output and the current report source to win while old remote screenshots fill only paths it omitted.

IMPACT

Draft, failed-CI, metadata-missing, and payload-missing automatic previews still reach the publisher with an empty PR number or missing directories and turn an intended green skip into a failed job. A report rerun that supplies a corrected screenshot under an existing filename publishes the previous remote byte instead; the exact probe observed remote-old where the new source supplied local-new. NEW FEATURE IMPACT: not a new feature — the migration should make publication reliable without changing output semantics, but these two ordinary paths do not yet receive that result.

VERDICT: BLOCKS — two reachable repair paths remain observably wrong.

API

No installed-consumer or package API change. The repository-only publisher adds trusted preview/report commands and keeps validated relative destination paths.

	change	public?	class	doc’d?	verdict
+	trusted preview resolution and vibe-report publication	no — repository CI/operator surface	existing publisher command family	workflow/test names and CLI usage	note — final consumers are incomplete

OSSIFICATION: these commands are internal repository infrastructure, but their readiness and conflict rules become the contract for every later writer. Correct those rules before the migration is treated as complete.

VERDICT: note — the blocking defects belong to ARCHITECTURE and BREAKING.

THEMING

No component CSS, token, theme target, variable, or style contract changes.

VERDICT: clear

BREAKING

BEHAVIOR: yes — intended clean skips still execute deployment, and current local screenshot bytes can be replaced by prior remote bytes. API: no installed-consumer API change. VISUAL: no component/story render path changes used by the verdict; the screenshot finding is byte precedence, not a visual-quality judgement. THEME: no theme change.

VERDICT: BLOCKS — existing workflow skip and report refresh behavior regress.

PERFORMANCE & RESOURCES

EFFECTS: zero. RENDER: no application render path change used by the verdict. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package change. CI/runtime cost: daily cleanup now clones full gh-pages history so path age is observable; the current branch has 59 commits / 7.56 MiB and cloned in 1.14s on the review host, while weekly compaction bounds growth. Focused tests completed in 115.66s.

VERDICT: clear — the measured maintenance cost is bounded and serves the retention policy.

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: the verdict is based on workflow control flow and exact file-byte precedence; no Astryx component, story, CSS, DOM, theme, or application render path changes.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect used by the verdict

A11Y & I18N

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

VERDICT: clear

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	BLOCKS — final readiness and report precedence remain wrong
ARCHITECTURE	BLOCKS — two facts still have disconnected authorities
IMPACT	BLOCKS — draft/missing-artifact deploys fail and refreshed screenshots remain stale
API	note — internal commands only
THEMING	clear
BREAKING	BLOCKS — skip and refresh semantics regress
PERFORMANCE	clear
VISUAL	clear — not applicable to verdict
A11Y & I18N	clear — not applicable

GOAL: partly met — trusted/adversarial identity, permissions, 58-day retention, terminal legacy cleanup, direct-writer removal, subtree preservation, and exclusive compaction pass; the final readiness guard and current-source screenshot precedence fail independently while all 43 checked-in tests stay green. DISPOSITION: prior trusted identity/API cross-check → resolved; adversarial stale/repository/artifact rejection → resolved; draft/failed/missing-artifact final readiness → blocks now; 30-day retention → resolved; terminal legacy cleanup → resolved; direct writer removal → resolved; report same-path preservation → blocks now; permissions → resolved; absent exact-head Actions CI → landing gate after both parents land, not the code finding. ADVICE: bounded outcome criteria — consume all three readiness outputs at the deploy step; when merging a report subtree, preserve old screenshots only for paths absent from the current source. AUTHOR CAN PROCEED: yes — both acceptance criteria are explicit and require no new owner, public concept, or design choice. WORST OUTCOME: “a draft or artifact-missing run turns red, while a successful report rerun silently keeps an old screenshot” → request changes.

JUDGEMENT NEEDED: none — both defects are reproduced against the approved publication model.

request changes (self-authored fallback COMMENT; no merge or auto-merge)

1. Automatic preview readiness is computed but not enforced at the final writer → draft, failed, metadata-missing, and payload-missing runs still invoke publication and fail · .github/workflows/deploy-preview.yml:174-182

2. The migrated report writer gives old remote bytes precedence over current local bytes → a corrected same-name screenshot remains stale after a successful publication · .github/scripts/lib/gh-pages-publisher.mjs:1822-1828

REVIEW

Semantic verdict: request changes

Thanks — the trusted PR resolver, permissions, retention, compaction cleanup, and direct-writer migration are now present. Two prior boundaries remain:

• The automatic deploy step still has no readiness condition. Draft, failed, or missing-artifact paths set ready=false, but deployment still runs with an empty PR or directories; 43/43 checked-in tests pass because they only search earlier readiness strings.
• The report publisher copies new local screenshots first, then overwrites matching names with the old remote subtree. An isolated exact-head publish kept remote-old instead of local-new, reversing the previous writer’s documented local-wins behavior.

Could we gate deployment on trusted, artifact, and file readiness, then preserve remote screenshots only when the new report did not supply that path?

[Reviewed by Robohands]

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

None — each finding spans producer and final consumer behavior.

EVIDENCE I DID NOT SPEND

• The trusted resolver passed current/open and associated-commit fallback cases; draft and failed CI returned not-ready; stale head and wrong repository failed closed; forged artifact PR, head, and run each exited before payload handling.
• Full-history retention replay kept the exact 30-day boundary and 29-day screenshots while deleting the 58-day subtree after an injected rejected push.

TIME

TIME total 16m

• setup/rules: 4m — loop 1.9.1, official audit 1.14.4, prior round, parent approval rounds, exact metadata, claim, and executable preflight
• worktree/install: 1m — dedicated detached exact-head checkout and lockfile-matched dependency clone/reconcile
• build/server: 0m — not applicable
• browser/a11y: 0m — no component or application render path in the verdict
• focused tests: 4m — 43/43 checked-in tests, actionlint, syntax, formatting, diff-check, and check:repo
• code/history/probes: 4m — trusted/adversarial cases, writer census, report collision, retention boundaries, rejected pushes, compaction, parent chain, and CI census
• critique/wiki/delivery: 2m — presentation, critic pass, exact-head recheck, fallback COMMENT, record/index update, and readback
• CI wait: 0m — no substantive Actions run existed before delivery; the posted review triggered one skipped Review signal run
• waste: 1m — one quoted scan and one local HTTP preview setup were discarded

WHAT I COULD NOT VERIFY

• A live GitHub Actions overlap was not dispatched because it would mutate shared gh-pages; exact production functions were driven against isolated bare remotes instead.
• Full inherited repository CI has not run for this stacked exact head.

Current CI and landing gate

At exact head f07522f4127d9630f17f77d458bf075f3eef7019, Meta CLA and both Vercel surfaces are green. Delivery triggered Review signal run 33164938536, whose two jobs skipped; no substantive GitHub Actions CI ran. Landing remains separately gated in order: open #5629 must land; open draft #5631 must receive substantive exact-head CI and land; then this exact head must receive substantive GitHub Actions CI. No merge or auto-merge was requested or armed.

Required invariant evidence

• Trusted preview identity: pass — default-branch checkout precedes a trusted API resolution of exact open PR/head/ref/repository; artifact PR/head/run are cross-checked afterward.
• Adversarial/stale/draft rejection: mixed — stale head, wrong repository, failed CI, draft, and forged artifact identity are rejected by their resolver; draft/failed/missing-artifact paths still execute the unguarded final deploy and fail.
• Retention: pass — 58-day screenshots delete; exactly-30-day and 29-day screenshots, report HTML, live preview, baseline, and concurrent unrelated writes survive.
• Compaction migration state: pass — terminal legacy holder/ticket state is removed while an active shared successor remains.
• Direct writer migration: mixed — deploy-report.ts no longer pushes directly and no payload writer remains outside the publisher, but same-name old remote screenshots overwrite current local report bytes.
• Permissions: pass — cleanup, compaction, and vibe screenshot jobs each grant actions: read and contents: write.
• Subtree preservation: pass — report, preview, baseline, visual evidence, live screenshots, and injected unrelated writes survive their non-owning publishers.
• Exclusive compaction: pass — a forced stale lease retries from the new tip, preserves the concurrent write and all scoped content, and publishes a one-commit history.

Focused evidence

• Checked-in Vitest: 2 files / 43 tests passed in 115.66s.
• Independent control matrix: 15/19 passed; intended failures were the missing final readiness guard, local screenshot losing to the old remote byte, and two adjacent reports-index presentation checks.
• Retention/compaction radius: 2/2 independent probes passed, including 29/30/58-day boundaries and injected rejected pushes.
• Repository checks: actionlint passed 11 workflows; both publisher entrypoints and the shell wrapper passed syntax; Prettier, diff-check, and pnpm check:repo passed.
• Writer census: no direct payload writer remains outside .github/scripts/lib/gh-pages-publisher.mjs; the parent-approved legacy lock writes rollout queue metadata only.
• Stack/CI: exact parent chain is f07522f4 → approved 29dafeba → approved/green 88195bb8; current main did not touch the reviewed files after the bottom parent; the exact head has one skipped Review signal run and zero substantive Actions CI.

What changed before posting

Posted as drafted after one critic pass; the public fallback comment is 120 words.

Delivery

GitHub review: https://github.com/facebook/astryx/pull/5633#pullrequestreview-5050396765

Cleanup

No long-lived process was required. Dedicated code/wiki worktrees and scratch probes are removed at run completion.

Round 3 — 051e991c

PR

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

HEAD REVIEWED

051e991c0d8de7f9b17991d81ec27d076141c30b — every claim below was verified at this exact head. Its sole parent is approved #5631 head 29dafeba0e6cef644c36bd9e99725baa09259dfe, whose sole parent is approved #5629 head 88195bb8e6765f7fd10fd5abf7521e09d157bc42.

VERSIONS

LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14.4

LANE

LANE: full WHY: this is a third-round re-review of two correctness findings in a privileged stacked publication migration. Trusted API readiness, artifact isolation, byte precedence, retention, whole-tree compaction, retries, terminal cleanup, permissions, and writer exclusivity all remain in scope.

PROBLEM

WHY 1: The prior head computed preview readiness without enforcing it at the final writer, and copied old remote screenshots over same-name current report bytes. WHY 2: Draft, failed, stale, or artifact-missing runs could turn an intended skip into a failed deploy, while a corrected screenshot could remain stale after a successful publication. WHY 3: Maintainers rely on each preview and report matching the exact eligible run and the newest generated evidence.

USER-FACING PROBLEM: A maintainer inspecting a preview or report could receive a red deployment for an ineligible run or see an old screenshot where the current run supplied a replacement. PROBLEM SEVERITY: broken task — the preview workflow could execute outside its trusted readiness boundary, and successful report publication could expose stale bytes. NEW FEATURE CASE: not a product feature — this completes the repository-owned publication migration approved in #5629 and #5631. EARLY STOP: clear — the approved parents and two prior rounds establish the need; this round checks the exact repair criteria.

VERDICT: clear

SOLUTION

After artifact verification, the automatic preview workflow asks GitHub for the PR again and publishes only if it is still open, non-draft, and on the exact trusted head, branch, and repository. Report staging now copies the old destination first and overlays the current source second, so current bytes win while remote-only files survive.

SOLUTION (2 delta decisions · 4 files / +59 −6 since the blocked head)

1. Add one final trusted API readiness check directly before automatic publication and make the publisher consume its output.
2. Make current report source authoritative for same paths while retaining old remote paths absent from the source.

BURDEN: low for the delta — one bounded API recheck, one staging-order rule, one workflow contract test, and one added byte fixture over the existing publisher. BURDEN MATCH: proportionate — each decision directly closes one prior reproduced failure without adding an owner, durable schema, queue, or workflow.

VERDICT: clear

ARCHITECTURE

OWNER: trusted default-branch workflow code owns preview eligibility; the shared publisher owns publication order and report subtree reconciliation. TIER 1: the global run-id FIFO, temporary same-commit legacy projection, and Actions run liveness approved in the parent stack. TIER 2: none. SEAMS: current/draft/closed/stale/wrong-repository preview state, API failure, missing artifacts, same-name and remote-only report files, rejected push, retention boundaries, whole-tree compaction, and terminal queue cleanup. BEHAVIOR UNIT: pure utility/module plus workflow contract — exact publisher functions run against isolated bare remotes; the final GitHub script was executed with mocked PR states. COMPLEXITY BUDGET: no new owner, workflow, queue, or durable representation; one final readiness authority; one current-source-wins reconciliation rule; one focused regression for each prior defect. ACTUAL BURDEN: full PR 10 files / +1,198 −572; exact repair delta 4 files / +59 −6; one final API step; one staging-order change; 44 checked-in focused tests. The readiness regression is asserted; the same-name byte fixture is added but its value is not asserted. BURDEN TREND: first reviewed 33bbe50e → f07522f4 grew for the requested migration repairs; f07522f4 → 051e991c adds only the final readiness consumer and byte-precedence contraction, with no new owner or representation. RESET TRIGGER: not triggered — the prior model-level gaps are contracted inside the existing owners. The remaining test note is verification debt, not another runtime model.

domain fact	one authoritative writable source	generated / immutable projections	other writable copies
preview PR/head/repository	trusted Pull Requests API matched to the workflow run	artifact identity cross-check	none
final preview readiness	final API response after artifact verification	final-preview.ready consumed by the publish step	none
report bytes	current report source	old remote paths fill only source-absent paths	none in runtime; checked-in same-name assertion missing
publication order	shared run-id FIFO with temporary same-commit legacy projection	run id/scope logs	no payload writer outside the publisher
retention cutoff	full gh-pages path history and fixed current time	deletion plan replayed on each fresh tip	none
compaction terminal state	compacted tree after removing the current turn and legacy root	one-commit history	active shared successor tickets preserved

seam	driven result
current open PR	final recheck returns ready and publication remains eligible — pass
draft / closed	final recheck returns not ready — pass
stale head / stale branch / wrong repository	final recheck returns not ready — pass
API failure	final check throws and no ready output can authorize publication — pass
missing metadata or preview payload	the final step is skipped unless all three earlier readiness outputs are true; publish requires the final output — pass
same-name report screenshot	isolated publication writes local-new over remote-old — pass
remote-only report screenshot	isolated publication retains remote-only — pass
rejected report push / repeat publish	concurrent unrelated write survives retry; second identical publish is a no-op — pass
retention	58-day screenshots delete; exactly-30-day and 29-day screenshots, report HTML, live preview, baseline, and concurrent unrelated write survive — pass
compaction	rejected lease retries from the new tip; history becomes one commit; legacy root and current turn disappear; active successor and unrelated content survive — pass
permissions	every queued workflow caller retains Actions read access and required contents access — pass
writer census	production push calls exist only in the shared publisher and the parent-approved legacy queue metadata bridge — pass
checked-in same-name regression	reverting to remote-wins still leaves the named publisher test green because same.png is never read — note before landing

Round 2 asked for a final readiness consumer and current-source screenshot precedence. Both runtime asks are satisfied at this head; the non-blocking extension is to make the newly added same-name fixture assert the byte rule it is intended to protect.

VERDICT: note — runtime ownership and behavior are correct; the same-name byte regression lacks a checked assertion.

IMPACT

Eligible previews now publish only after a final trusted state check; ineligible or stale runs stop before the writer. A current report screenshot replaces its same-name remote predecessor, while unrelated old screenshots and all non-owned subtrees remain available. NEW FEATURE IMPACT: not a new feature — maintainers regain the skip and refresh behavior the migration intended to preserve.

VERDICT: clear

API

No installed-consumer or package API change. The repository-only workflow contract adds final-preview.ready; report reconciliation keeps the existing vibe-report command and source shape.

	change	public?	class	doc’d?	verdict
+	final-preview.ready gate after exact PR API recheck	no — repository workflow	existing readiness outputs	workflow/test names	clear
~	vibe-report current-source-wins precedence	no — repository publisher	legacy report writer behavior	source/test title	note — exact byte assertion missing

OSSIFICATION: installed consumers cannot depend on either contract. Repository automation can, so the byte rule should be locked before landing, but no new public concept or human-owned decision is introduced.

VERDICT: note — internal behavior is correct; one regression assertion is missing.

THEMING

No component CSS, token, theme target, variable, or style contract changes.

VERDICT: clear

BREAKING

BEHAVIOR: fixes the two prior regressions; final readiness fails closed and current local bytes win same-path report conflicts. API: no installed-consumer API change. VISUAL: no component/story render path changes; screenshot files are publication payload, and the verdict uses exact bytes rather than visual judgement. THEME: no theme change.

VERDICT: clear

PERFORMANCE & RESOURCES

EFFECTS: zero. RENDER: no application render path changes. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package change. CI/runtime cost: an eligible automatic preview adds one Pull Requests API lookup immediately before publication, with three bounded retries. The report overlay and focused test additions add no application-runtime work.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: the delta changes workflow control flow, Node file reconciliation, and tests. No Astryx component, CSS, DOM, story, theme, or rendered application path changes; screenshot correctness was verified by exact file bytes.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

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

VERDICT: clear

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	note — checked-in same-name byte assertion missing
IMPACT	clear
API	note — internal regression assertion missing
THEMING	clear
BREAKING	clear
PERFORMANCE	clear
VISUAL	clear — not applicable
A11Y & I18N	clear — not applicable

GOAL: met — final readiness passes the current case and fails closed for draft, closed, stale-head, stale-branch, wrong-repository, and API-error cases; an isolated rejected-push report publication produces same=local-new, retains oldOnly=remote-only, preserves an unrelated concurrent write, and is idempotent. Retention, compaction, permissions, writer exclusivity, and all 44 checked-in focused tests pass. DISPOSITION: Round 2 final readiness blocker → resolved; Round 2 same-name byte-precedence blocker → resolved in current code and independent execution; missing checked-in byte assertion → non-blocking before-landing note under the re-review cadence; absent substantive exact-head Actions CI → landing gate after both parents land, separate from the code verdict. ADVICE: bounded outcome criterion — assert that reports/vibe-report/screenshots/same.png contains new bytes; the current fixture already creates that file, and the remote-wins mutant currently passes the named test. AUTHOR CAN PROCEED: yes — code review is clear; add the one assertion before landing, then run inherited exact-head CI after both parents land. WORST OUTCOME: “a future report precedence regression can return while focused CI remains green” → note before landing; current exact code is correct.

JUDGEMENT NEEDED: none — the byte assertion is ordinary regression coverage, not a new owner, API, or design decision.

approve (self-authored fallback COMMENT; no merge or auto-merge)

1. The same-name fixture is not asserted. → a future report precedence regression can return while focused CI remains green · .github/scripts/lib/gh-pages-publisher.test.mjs:1272

REVIEW

Semantic verdict: approve

Both prior defects are fixed. Please assert screenshots/same.png is new bytes before landing—the current fixture leaves the precedence regression unguarded. Landing still waits for both parents and exact-head CI.

[Reviewed by Robohands]

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

None — the note is carried in the semantic-approval comment.

EVIDENCE I DID NOT SPEND

• The exact previous→current delta is 4 files / +59 −6; current runtime production is the old destination overlayed by the new source, reversing the faulty copy order without changing queue ownership.
• The legacy direct writer copied local screenshots after the existing gh-pages subtree, so current-source-wins is preserved behavior rather than a new report policy.
• The official rubric page header and history identify 1.14.4; its older “Current” sentence still says 1.14.3, so this round uses the published header/history version consistently with the prior round.

TIME

TIME total 12m

• setup/rules: 3m — fresh loop, rubric, parent approvals, all prior #5633 rounds, exact metadata, untrusted CI, and isolated worktree
• worktree/install: 1m — dedicated detached exact-head worktree and lockfile-matched dependency clone/reconcile
• build/server: 0m — not applicable
• browser/a11y: 0m — non-rendering change
• focused tests: 2m — 44/44 Vitest, actionlint, syntax, Prettier, diff-check, and check:repo
• code/history/probes: 3m — final readiness adversarial matrix; byte precedence, retry, idempotency, 29/30/58-day retention, compaction, permissions, writers, parent chain, and exact CI
• critique/wiki/delivery: 2m — presentation, critic pass, exact-head recheck, fallback COMMENT, record/index update, and readback
• CI wait: 0m — no substantive exact-head Actions run exists
• waste: 1m — the first Prettier command included a shell file with no parser; the mutation wrapper’s shell-status capture failed after the test result but restored the source cleanly

WHAT I COULD NOT VERIFY

• A live GitHub Actions overlap was not dispatched because it would mutate shared gh-pages; exact production functions and schemas were driven against isolated bare remotes instead.
• Full inherited repository CI has not run at this stacked exact head.

Current CI and landing gate

After delivery, exact head 051e991c0d8de7f9b17991d81ec27d076141c30b has one Review signal run, completed with both jobs skipped; two successful service check runs (Meta CLA and Vercel Preview Comments); one successful Vercel commit status; and zero substantive GitHub Actions CI. #5629 remains open on main; #5631 remains an open draft on #5629. Landing remains separately gated in order: #5629 lands; #5631 receives substantive exact-head CI and lands; then this exact #5633 head receives substantive inherited GitHub Actions CI. No merge or auto-merge was requested or armed.

Focused evidence

• Checked-in Vitest: 2 files / 44 tests passed in 111.13s.
• Independent readiness execution: current open PR → ready; draft, closed, stale SHA, stale ref, and wrong repository → not ready; API failure → failed closed. Source order is artifact verification → final API recheck → guarded publisher.
• Independent report probe: first push intentionally rejected; retry preserved the concurrent write; same-name current byte won; remote-only screenshot survived; second identical publication was a no-op.
• Independent retention probe: 58-day screenshots deleted; exactly-30-day and 29-day screenshots, report HTML, live preview, baseline, and concurrent write survived; second cleanup deleted zero paths.
• Independent compaction probe: an injected stale lease retried from the fresh tip; final history is one commit; the legacy root and compactor turn are gone; active successor, preview, report, baseline, and concurrent write survive.
• Regression negative control: reverting only report staging to remote-wins leaves the named publisher test green (1 passed / 23 skipped), because the new same.png fixture has no byte assertion.
• Repository checks: actionlint passed all 11 listed workflows; both Node entrypoints and the shell wrapper passed syntax; Prettier, git diff --check, and pnpm check:repo passed.
• Writer/permission census: production push invocations are limited to the shared publisher and parent-approved legacy queue metadata bridge; every queued workflow caller retains required Actions/contents access.
• Security/public-safety preflight: no dependency, lockfile, package hook, executable-mode change, shell-evaluated untrusted identity, user-controlled queue path, credential expansion, or public-repo private residue was introduced.

What changed before posting

Posted as drafted after one critic pass; the public semantic-approval comment is 29 substantive words.

Delivery

GitHub review: https://github.com/facebook/astryx/pull/5633#pullrequestreview-5050545236

Cleanup

No long-lived process was started. The dedicated code/wiki worktrees and scratch probes are removed at run completion.