-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5631
#5631 ci: route visual gh-pages writers through publisher by cixzhang (bucket: the maintainer)
311ba8cdc3c0f3e94207d86bc151559e8902758a — every claim below was verified at this commit, whose sole parent is approved #5629 head 88195bb8e6765f7fd10fd5abf7521e09d157bc42.
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14.1
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.
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
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)
- Publish immutable PR visual evidence through the shared queue and refresh the state checkout afterward.
- Archive visual acceptance records through the same publisher while preserving trusted identity inputs.
- Promote accepted post-merge pixels only while holding the shared baseline turn.
- 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.
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.
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.
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.
No CSS, tokens, theme targets, rendered elements, or style contracts change.
VERDICT: clear
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.
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 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: not triggered — no proven visual defect
No rendered element, interaction, role, accessible string, locale formatting, or direction-sensitive behavior changes.
VERDICT: clear
| 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)
-
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 -
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
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:144now enters the shared wait path, but itsacceptjob grants noactions: 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]
None — both defects cross a workflow permission block and publication-owner boundary.
- 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 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
- 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.
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.
- 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: readexceptvisual-acceptance.yml’sacceptjob. - Roll-forward probe: run 100 acquired
.astryx-gh-pages/publication-queue/holder.jsonwhile run 101 simultaneously acquiredvisual-gate/publication-queue/holder.jsonagainst 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
ghbinary.
Posted as drafted after one critic pass; the public review is 107 words.
GitHub review: https://github.com/facebook/astryx/pull/5631#pullrequestreview-5048724688
#5631 ci: route visual gh-pages writers through publisher by cixzhang (bucket: the maintainer)
d64f87286431e6003d6b5da1df379706f09844e9 — every claim below was verified at this commit, whose sole parent is approved #5629 head 88195bb8e6765f7fd10fd5abf7521e09d157bc42.
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14.1
LANE: full WHY: this is a delta re-review of two prior blockers in a privileged branch-publication migration; the source-of-truth remedy adds a rollout bridge and must be checked across every publication scope.
WHY 1: Independent workflow generations can mutate gh-pages while consulting different publication queues.
WHY 2: Two runs that both believe they own publication can race one branch update, fail one workflow, or overwrite evidence that a maintainer relies on.
WHY 3: Visual acceptance and release decisions are trustworthy only when publication ownership remains singular across deployment roll-forward.
USER-FACING PROBLEM: A maintainer running workflows across the migration boundary can receive two simultaneous publication owners instead of one durable order for every artifact. PROBLEM SEVERITY: broken task — one of the supposedly serialized publications can race or fail under overlap. NEW FEATURE CASE: not a new product feature — this extends the approved repository publication subsystem to visual writers. EARLY STOP: clear — the owner-authored migration and approved parent establish the need; this round judges whether the prior concurrency acceptance criteria are met.
VERDICT: clear
Current baseline publishers write both a shared ticket and a compatibility ticket that the previous baseline workflow understands, then acquire the old holder before the shared holder. The acceptance job also gains the Actions access required to inspect a blocking run. The compatibility path is applied only to the baseline scope, so other current shared publishers do not observe an old baseline holder.
SOLUTION (2 delta decisions · 363 runtime lines plus 130 test/config lines)
- Grant the migrated acceptance job
actions: readand bind every shared-publisher caller’s permission contract in the workflow test. - Duplicate baseline queue lifecycle into the shared publisher so new baseline writers acquire both old and new holders during rollout.
BURDEN: high for the delta — a second queue parser, mutator, blocker scan, holder claim, wait loop, and release path are copied into the shared publisher. BURDEN MATCH: disproportionate at this head — the added compatibility state machine prevents baseline-vs-baseline dual ownership but does not cover the other shared publication scopes that can run while an old baseline holder exists.
The prior review asked for actions: read on the acceptance job and one publication owner across old/new rollout. The permission ask is satisfied. The baseline-specific case is satisfied, but the broader ownership ask remains open.
VERDICT: BLOCKS — the rollout bridge leaves current non-baseline scopes outside the old-holder boundary.
OWNER: the shared gh-pages publisher owns branch publication order across every scope.
TIER 1: the approved #5629 publisher and its global run-id FIFO.
TIER 2: none.
SEAMS: old baseline workflow → current baseline writer, old baseline workflow → current report/site/evidence/acceptance writer, stale-run cleanup, rejected-push retry, and release after partial failure.
BEHAVIOR UNIT: pure utility/module — both production CLIs were driven against one local bare remote with a fake run-status endpoint.
COMPLEXITY BUDGET: one authoritative current queue; at most one temporary compatibility projection spanning every current writer while old runs can exist; one active holder across both roots.
ACTUAL BURDEN: 7 PR files; 892 added publisher lines overall; 363 publisher lines added in this round; 1 shared publisher plus the retained executable old lock; 2 writable queue roots; 4 migrated visual workflows; 25 focused tests.
BURDEN TREND: 311ba8cd → d64f8728: grew by 363 runtime lines and a complete legacy queue state machine while the stated migration problem stayed the same.
RESET TRIGGER: triggered — the remedy for split publication authority adds a second writable compatibility representation and scope guard, yet a second model-level dual-holder path remains.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| current publication order | .astryx-gh-pages/publication-queue |
run id/scope in logs | legacy queue is still writable by old runs and by current baseline runs |
| rollout compatibility | current publisher should project every current writer into the old holder boundary until old runs drain | old ticket/holder with the same run id | current non-baseline writers do not project or observe that boundary — BLOCKS |
| workflow liveness | GitHub Actions run state | immutable run id in tickets | none |
| seam | driven result |
|---|---|
| acceptance permission | current workflow has actions: read; the contract test fails against 311ba8cd and passes at this head |
| old baseline run 899 → current baseline run 901 | current baseline wait refuses behind the old holder; the added regression fails against the prior publisher and passes here |
| old baseline run 899 → current report writer 900 | both actual production CLIs acquire different holder files simultaneously against one remote — BLOCKS |
| token/argument boundary | bridge paths are constants, run/repository identity is validated, and subprocess calls use argument vectors rather than a shell |
VERDICT: BLOCKS — publication order still has two active authorities for old-baseline/current-nonbaseline overlap; contraction requires every current shared scope to observe the compatibility holder while old runs can exist, or a proven disjoint-lane model replacing the global-owner claim.
The acceptance permission failure is fixed, and a current baseline writer no longer passes an active old baseline ticket. But a current report/site/evidence/acceptance writer can still acquire the shared holder while that old baseline run owns the legacy holder, so maintainers can still see concurrent branch publication during roll-forward.
VERDICT: BLOCKS — one reachable migration state still violates the single-owner promise.
No package or installed-consumer API changes. The repository-internal queue contract now adds a legacy compatibility adapter limited to visual-gate/baseline; actions: read is the minimum permission required by the existing run-status lookup.
OSSIFICATION: installed consumers cannot depend on this surface, but repository automation does. The compatibility adapter remains a second internal contract until the rollout boundary covers every writer and is later removed.
VERDICT: note — internal compatibility surface grew; the correctness block is owned by ARCHITECTURE.
No CSS, token, target, rendered element, or style contract changes.
VERDICT: clear
BEHAVIOR: baseline-vs-baseline rollout now serializes, but old-baseline/current-nonbaseline rollout can still dual-hold and race branch publication. API: no consumer API change. VISUAL: no rendered UI change. THEME: no theme change.
VERDICT: BLOCKS — the migration’s defining single-owner behavior remains incomplete across publication scopes.
EFFECTS: zero. RENDER: no application render path changes. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no package or dependency change. CI cost: each current baseline run now performs queue mutation and polling against both roots; no measured application-runtime degradation is claimed.
VERDICT: clear
VISUAL CHECK: not applicable WHY: the delta changes workflow permissions, Node publication coordination, and tests; no UI, CSS, markup, story, or rendered component behavior changes.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
The delta adds no rendered element, interaction, role, accessible string, locale formatting, or direction-sensitive behavior.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — compatibility applies only to baseline scope |
| ARCHITECTURE | BLOCKS — old baseline and current non-baseline writers can dual-hold |
| IMPACT | BLOCKS — concurrent publication remains reachable during rollout |
| API | note — internal compatibility contract grew |
| THEMING | clear |
| BREAKING | BLOCKS — single-owner migration is incomplete |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear — not applicable |
GOAL: partly met — actions: read is present and its negative control fails on the prior head; the baseline-specific bridge test also fails on the prior head and passes here. A separate probe using the actual old lock and current publisher proves run 899 can hold visual-gate/publication-queue/holder.json while a current visual-gate/reports run 900 holds .astryx-gh-pages/publication-queue/holder.json.
DISPOSITION: missing acceptance permission → resolved; baseline-vs-baseline dual-holder → resolved; old-baseline/current-nonbaseline dual-holder → blocks now; absent exact-head substantive GitHub CI → landing gate, not a code finding.
ADVICE: bounded outcome criteria — while any old baseline run can exist, every current shared-publisher scope must observe the old holder boundary, or the system must prove and test genuinely disjoint authorities instead of claiming one global owner.
AUTHOR CAN PROCEED: yes — extend the rollout contract across all current scopes and add a production-old-lock/current-nonbaseline regression that fails when both holder files exist simultaneously.
WORST OUTCOME: “An old baseline publisher and a current report/site publisher can both believe they own gh-pages and push concurrently.” → request changes.
JUDGEMENT NEEDED: none — this is a reproduced concurrency defect against the already-approved global publication-owner contract.
request changes (self-authored fallback COMMENT)
- The compatibility bridge is guarded to the baseline scope while the shared queue represents all publication scopes.
→ during rollout, an active old baseline run and a current report/site/evidence/acceptance run can acquire different holders and mutate the same branch concurrently ·
.github/scripts/lib/gh-pages-publisher.mjs:803
Semantic verdict: request changes
Thanks for closing the permission path — actions: read is present and the negative control fails on the prior head.
The rollout bridge still only consults the legacy queue for visual-gate/baseline. I drove the production old lock at run 899 and a current visual-gate/reports writer at run 900 against one bare remote; both acquired different holders simultaneously. The new baseline-only regression passes, but the scope guard leaves every other shared publisher outside the bridge, so old and new publication ownership still overlap during rollout.
Could we make every current shared publisher observe the legacy holder while old runs can exist, then cover that cross-scope production case?
[Reviewed by Robohands]
None — the finding crosses the global shared holder and the legacy baseline holder.
- Focused Vitest is 2 files / 25 tests green at the exact head; actionlint, both Node syntax checks, and
pnpm check:repoare green. - The added baseline bridge regression and workflow permission regression both fail under
311ba8cd, confirming they detect the two prior cases rather than merely passing. - Security preflight found no dependency, lockfile, package-hook, executable-mode, shell-evaluation, or user-controlled queue-path expansion in the delta.
TIME total 10m
- setup/rules: 3m — fresh loop 1.9.1, audit 1.14.1, parent Round 2, prior #5631 review, full untrusted diff, and exact-head setup
- worktree/install: 1m — dedicated detached worktree and lockfile-matched install
- build/server: 0m — not applicable
- browser/a11y: 0m — non-rendering change
- focused tests: 1m — 25/25 Vitest, actionlint, syntax, and check:repo
- code/history: 3m — old→new delta, permissions, bridge state machine, current-main check, two negative controls, and actual old-lock/current-publisher probes
- critique/wiki: 2m — presentation, critic pass, delivery, and record/index update
- CI wait: 0m — delivery triggered one Review signal run; both jobs skipped, so no substantive repository CI ran
- waste: 1m — large rule/rubric reads exceeded one output window and were repeated in smaller chunks
- A live GitHub Actions overlap was not dispatched because it would mutate shared
gh-pages; the same production CLIs and schemas were driven against an isolated bare remote instead. - Full repository CI has not run for this stacked draft head.
At exact head d64f87286431e6003d6b5da1df379706f09844e9, GitHub reports Meta CLA and Vercel Preview Comments green plus the Vercel status green. Delivery triggered one Review signal run, but both jobs skipped; no substantive repository CI/test workflow has run. That absent inherited CI is a separate landing gate: after #5629 lands and this PR targets main, substantive exact-head GitHub CI must run before landing; it is not being used as a code blocker or as evidence of correctness here.
- Vitest: 2 files, 25/25 tests passed in 38.31s.
- Negative controls: the bridge regression and permission regression both fail against prior head
311ba8cd. - Production rollout probe: old baseline run 899 and current report run 900 acquired legacy/shared holders simultaneously; current baseline run 901 correctly refused behind old run 899.
-
actionlint: all six relevant workflows passed with the repository’s documented ignores. -
node --check: both publisher entrypoints passed. -
pnpm check:repo: sync, boundaries, changesets, demo media, executable bits, CLI structure, use-client, portable scripts, i18n catalog, and CLDR weekdays all passed. - Security radius: permission growth is read-only Actions access; queue paths are constants; repository/run/scope identity is validated; subprocesses use argument vectors; tests use isolated bare remotes and a fake
gh.
Posted as drafted after one critic pass; the public review is 113 words.
GitHub review: https://github.com/facebook/astryx/pull/5631#pullrequestreview-5048893503
#5633 is not code-review-safe to start while this parent remains semantically blocked; it was not inspected or touched in this run.