-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5629
#5629 ci: centralize gh-pages publishing by cixzhang (bucket: the maintainer)
71c00d5920914a74c7c40a719ce88ab09276177d — every claim below was verified at this commit.
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14
LANE: full WHY: this adds a shared CI publication subsystem, a durable queue/holder schema, two publication strategies, cross-workflow coordination, privileged branch mutation, and a future stack contract.
WHY 1: Independent workflows can publish different owned paths to gh-pages at the same time, and GitHub Actions concurrency does not preserve every pending writer.
WHY 2: A lost or clobbered writer leaves maintainers without the site or release evidence that workflow was responsible for publishing.
WHY 3: Release and preview decisions depend on each publication surviving unrelated writers.
USER-FACING PROBLEM: A maintainer running overlapping publication workflows can lose or overwrite one workflow’s output instead of receiving every independently owned artifact.
PROBLEM SEVERITY: broken task — the affected workflow cannot complete its publication reliably under contention.
NEW FEATURE CASE: the repository maintainer is consolidating all gh-pages writers after existing workflow concurrency proved unable to preserve a FIFO queue; the two migrated writers are the first consumers.
EARLY STOP: clear — the repository owner authored the capability and documented the follow-up migration class.
VERDICT: clear
Each publisher writes a durable ticket, waits until the oldest ticket owns the branch, publishes only its declared scope, and releases the ticket. One owner now coordinates whole-tree and scoped writes, while each publishing strategy preserves paths owned by other workflows.
SOLUTION (4 decisions · ~789 runtime lines of 1,166 changed lines)
- Store FIFO tickets plus one holder on
gh-pages, ordered by GitHub run id. - Publish the stable site as an orphan tree while carrying unrelated sparse paths forward.
- Publish release-gate reports as scoped normal commits with retry after a lost race.
- Migrate the two workflows from independent shell publishers to the shared owner.
BURDEN: high — one durable schema, one shared state machine, GitHub run-status recovery, two git publication strategies, and 17 focused tests. BURDEN MATCH: proportionate — strict cross-workflow serialization cannot be represented by an Actions concurrency group, and the shared owner replaces rather than duplicates publication logic.
VERDICT: clear
OWNER: the shared gh-pages publisher owns cross-workflow publication order and cleanup.
TIER 1: the existing baseline publication lock supplies the durable FIFO precedent; the new publisher generalizes it across scopes.
TIER 2: none.
SEAMS: whole-tree orphan publication, scoped release-report publication, stale/cancelled run recovery, push-race retry, and future writer migration.
BEHAVIOR UNIT: pure utility/module — exported queue and publisher operations are driven against local bare remotes in focused tests.
COMPLEXITY BUDGET: one queue owner, one durable ticket/holder representation, one liveness authority, two publication strategies, and two migrated workflows.
ACTUAL BURDEN: 6 files; 789 runtime lines; 1 shared module; 1 durable JSON schema; 2 workflows; 17 focused tests.
BURDEN TREND: first reviewed head → current head: flat — one exact head was reviewed.
RESET TRIGGER: not triggered — one owner and one writable queue represent publication order.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| publication order and holder | ticket and holder.json files on gh-pages
|
run id/scope copied into logs | none |
| workflow liveness | GitHub Actions run state | immutable run id in each ticket | none |
| owned publication scope | the command-to-scope mapping in the publisher | scope copied into the ticket | none |
| seam | driven result |
|---|---|
| whole-tree orphan writer | preserves unrelated preview, report, baseline, and queue paths in the focused test |
| scoped release-report writer | preserves stable site and baseline paths in the focused test |
| rejected push | reclones and republishes from the source checkout in the focused test |
| concurrent active writer | BLOCKS — the waiting path needs an Actions API permission neither migrated job grants |
VERDICT: BLOCKS — the shared queue cannot wait behind another active run with the migrated jobs’ tokens.
When publication does not overlap, maintainers get the same stable site and release report through one owner. When it does overlap, the second publisher reaches the run-status check with no Actions read permission and aborts instead of waiting, so the race this subsystem exists to handle still loses a publication. NEW FEATURE IMPACT: maintainers should be able to run any two publication workflows without cancelling or clobbering either output; current workflow permissions prevent that under contention.
VERDICT: BLOCKS — the stated contention case remains broken.
No package or consumer API changes. The internal CLI adds stable-site --source <dir> and release-gate --source <dir>, plus a versioned ticket {repository, runId, scope}; these are repository-owned workflow contracts.
| change | public? | class | doc’d? | verdict | |
|---|---|---|---|---|---|
+ |
stable-site --source <dir> |
no — repository workflow | whole-tree publisher | workflow call site | clear |
+ |
release-gate --source <dir> |
no — repository workflow | scoped publisher | workflow call site | clear |
+ |
ticket v1: repository/runId/scope | public branch data, internal protocol | durable queue precedent | tests | clear |
OSSIFICATION: these contracts remain repository-internal and match the existing baseline lock’s run-id queue model; no installed consumer can depend on them.
VERDICT: clear
No CSS, tokens, theme targets, rendered elements, or style contracts change.
VERDICT: clear
BEHAVIOR: single-writer publication preserves the prior output; the new overlapping-writer path aborts before waiting because its status lookup is unauthorized. API: no consumer API change. VISUAL: no rendered UI change. THEME: no theme change.
VERDICT: BLOCKS — concurrent publication is the feature’s defining behavior and is not operational with current permissions.
EFFECTS: zero. RENDER: no application render path changes. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package bundle change. CI cost: each publication adds durable queue mutations and shallow sparse clones; retries and a 75-minute wait bound are explicit.
VERDICT: clear
VISUAL CHECK: not applicable WHY: the diff changes only workflows, Node publication scripts, and tests; it changes no UI, CSS, rendered markup, story, or component behavior.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
No rendered element, interaction, role, accessible string, locale formatting, or direction-sensitive path changes. Component a11y and RTL jobs were correctly skipped for this non-component diff.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | BLOCKS — waiting requires an ungranted Actions permission |
| IMPACT | BLOCKS — an overlapping publisher aborts instead of waiting |
| API | clear |
| THEMING | clear |
| BREAKING | BLOCKS — defining contention path cannot run |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear — not applicable |
GOAL: partly met — 17/17 focused tests prove local queue order, path preservation, retry, and pruning, but both migrated jobs grant only contents: write; runStatus() calls an endpoint that requires Actions read access.
DISPOSITION: missing actions: read in both publisher jobs → blocks now.
ADVICE: proven existing pattern — the repository’s existing baseline-lock callers grant actions: read, and GitHub documents that unspecified permissions become none; grant the same minimum permission and lock it with the workflow test.
AUTHOR CAN PROCEED: yes — grant actions: read to the deploy and release-gate publish jobs, then add a focused assertion that both shared-publisher callers retain it.
WORST OUTCOME: “The second overlapping publication exits before waiting, so its site or release report is never published.” → request changes.
JUDGEMENT NEEDED: none — this is a correctness defect against the documented GitHub token contract.
request changes
- The contention path calls the Actions workflow-run endpoint without
actions: readin either migrated job. → when stable-site and release-report publication overlap, the waiting run gets a permission failure and exits instead of publishing ·.github/workflows/deploy.yml:208,.github/workflows/release-gate.yml:249,.github/scripts/lib/gh-pages-publisher.mjs:358
Semantic verdict: request changes
Thanks for pulling these publishers behind one durable owner — the local queue, preservation, and retry tests are solid. The contention path checks the current holder through GitHub’s “Get a workflow run” endpoint, but both migrated jobs grant only contents: write; because unspecified scopes become none, that lookup has no required Actions read access. The first overlapping publication therefore exits instead of waiting, which is the race this stack needs to close.
Could we grant actions: read to both jobs and cover that permission contract in the workflow test before building the next migration on top?
None — the defect spans the shared status lookup and both workflow permission blocks.
- The existing baseline-lock callers already grant
actions: readoractions: writearound the same workflow-run lookup. - PR CI is green, but neither main-only deploy nor scheduled release-gate publication executes on this pull request.
- No dependency, package hook, or lockfile changed; the reviewed same-repository scripts were safe to execute in isolated local-remotes tests.
TIME total 9m
- setup/rules: 3m — fresh rule and rubric versions, untrusted-code inspection, exact-head/claim verification
- worktree/install: 1m 11s — dedicated exact-head worktree, lockfile-matched install
- build/server: 0m — not applicable
- browser/a11y: 0m — non-rendering change
- focused tests: 40s — 2 files, 17/17 green; syntax and actionlint also green
- code/history: 3m — state machine, permissions, retry/idempotency, and existing-lock precedent
- critique/wiki: 2m — presentation, critic pass, record/index update
- CI wait: 0m — exact-head CI was already complete
- waste: 1m — initial large rule reads hit output truncation and were repeated in chunks
- A real overlapping GitHub Actions publication was not dispatched because that would mutate
gh-pages; the token contract and repository precedent settle the failure path without doing so.
At exact head 71c00d5920914a74c7c40a719ce88ab09276177d: 14 check runs succeeded, 5 path-appropriate jobs skipped, and all 3 commit statuses succeeded. The main CI run is 33146205343.
- Vitest: 2 files, 17/17 tests passed.
-
node --check: both new publisher entrypoints passed. -
actionlint: both changed workflows passed with the repository’s documented runner/shellcheck ignores. - Security preflight: all six changed workflow/script/test files read before checkout; no dependency, package hook, or lockfile change; test execution used local bare remotes and a fake
ghbinary.
Posted as drafted after one critic pass.
GitHub review: https://github.com/facebook/astryx/pull/5629#pullrequestreview-5048479069
#5629 ci: centralize gh-pages publishing by cixzhang (bucket: the maintainer)
88195bb8e6765f7fd10fd5abf7521e09d157bc42 — every claim below was verified at this commit.
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.14.1
LANE: full WHY: this is a re-review of a prior blocker in a shared publication subsystem; the unresolved prior review and privileged cross-workflow coordination exclude the fast lane.
WHY 1: Independent workflows can publish different owned paths to gh-pages at the same time, and Actions concurrency does not preserve every pending writer.
WHY 2: A lost or clobbered writer leaves maintainers without the site or release evidence that workflow was responsible for publishing.
WHY 3: Release and preview decisions depend on each publication surviving unrelated writers.
USER-FACING PROBLEM: A maintainer running overlapping publication workflows can lose or overwrite one workflow’s output instead of receiving every independently owned artifact.
PROBLEM SEVERITY: broken task — the affected workflow cannot complete its publication reliably under contention.
NEW FEATURE CASE: the repository maintainer is consolidating all gh-pages writers after existing workflow concurrency proved unable to preserve a FIFO queue; the two migrated writers remain the first consumers.
EARLY STOP: clear — the repository owner authored the capability and documented the follow-up migration class.
VERDICT: clear
The shared publisher stores a durable ticket, waits until the oldest live run owns the branch, publishes only its declared scope, and releases the ticket. This round grants both migrated jobs the read access required by that liveness check and adds a workflow-level contract test for both permissions and both publisher call sites.
SOLUTION (1 delta decision · 2 configuration lines and 20 test lines)
- Grant the shared publisher’s two callers Actions run read access and lock that minimum contract in the workflow test.
BURDEN: low for this delta — no new state, owner, schema, workflow, or runtime branch; two least-privilege grants plus one static contract test. BURDEN MATCH: proportionate — the grant is the minimum permission required by the existing run-status lookup.
VERDICT: clear
OWNER: the shared gh-pages publisher owns cross-workflow publication order and cleanup.
TIER 1: the existing baseline publication lock supplies the durable FIFO precedent; the new publisher generalizes it across scopes.
TIER 2: none.
SEAMS: whole-tree orphan publication, scoped release-report publication, stale/cancelled run recovery, push-race retry, and future writer migration.
BEHAVIOR UNIT: pure utility/module — queue and publisher operations are driven against local bare remotes; workflow contracts are checked as repository text.
COMPLEXITY BUDGET: one queue owner, one durable ticket/holder representation, one liveness authority, two publication strategies, and two migrated workflows.
ACTUAL BURDEN: 6 files; 789 runtime lines; 1 shared module; 1 durable JSON schema; 2 workflows; 18 focused tests.
BURDEN TREND: 71c00d59 → 88195bb8: flat — two permission grants and one contract test add no owner, durable representation, or runtime decision.
RESET TRIGGER: not triggered — one owner and one writable queue still represent publication order.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| publication order and holder | ticket and holder.json files on gh-pages
|
run id/scope copied into logs | none |
| workflow liveness | GitHub Actions run state | immutable run id in each ticket | none |
| owned publication scope | the command-to-scope mapping in the publisher | scope copied into the ticket | none |
| seam | driven result |
|---|---|
| whole-tree orphan writer | preserves unrelated preview, report, baseline, and queue paths in the focused test |
| scoped release-report writer | preserves stable site and baseline paths in the focused test |
| rejected push | reclones and republishes from the source checkout in the focused test |
| concurrent active writer | the local queue test waits behind an in-progress run; both workflow jobs now grant actions: read, and the contract test binds those grants to both publisher call sites |
The prior review asked for actions: read on both publisher jobs plus a workflow contract assertion. The exact old→new delta contains only those two grants and that test; all affected seams now have one owner and the permission needed to query their liveness authority.
VERDICT: clear
Single-writer behavior remains unchanged. Under overlap, both migrated publishers can now read the blocking workflow run’s state and remain in the queue rather than exiting on an authorization failure. NEW FEATURE IMPACT: maintainers can run the two migrated publication workflows concurrently without the prior permission failure preventing the waiting publication.
VERDICT: clear
No package or consumer API change. The repository-owned CLI and ticket schema are unchanged from the prior reviewed head.
OSSIFICATION: this delta adds no callable surface, accepted value, schema field, or default.
VERDICT: clear
No CSS, token, theme target, rendered element, or style contract changes; the delta is limited to workflow permissions and a text-level workflow contract test.
VERDICT: clear
BEHAVIOR: fixes the prior overlap failure by authorizing the existing run-status lookup; single-writer and publication strategy behavior is unchanged. API: no consumer API change. VISUAL: no rendered UI change. THEME: no theme change.
VERDICT: clear
EFFECTS: zero. RENDER: no application render path changes. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package bundle change. CI cost: the delta adds no API calls or queue work; it only authorizes the existing liveness lookup and adds a test assertion.
VERDICT: clear
VISUAL CHECK: not applicable WHY: the old→new delta changes only two workflow permission maps and a Node test that reads workflow text; it changes no runtime UI, CSS, markup, story, or component behavior.
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 | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear — not applicable |
GOAL: met — the old→new delta grants actions: read to both jobs, the contract test binds those grants to both shared-publisher call sites, 18/18 focused tests pass, and actionlint plus syntax checks are green.
DISPOSITION: the prior permission blocker is resolved; no negative findings remain.
ADVICE: omitted — no remaining finding needs a remedy.
AUTHOR CAN PROCEED: yes — there is no remaining review action; the draft can be marked ready when the author chooses.
WORST OUTCOME: “none found” → semantic approve.
JUDGEMENT NEEDED: none — this delta satisfies the prior correctness acceptance criteria without changing the subsystem model.
comment (semantic approve; GitHub cannot accept a self-approval and the PR is draft)
No findings.
Semantic verdict: approve
We asked for actions: read on both publisher jobs and a contract test. This head adds both; focused tests pass. Approve once undrafted.
None — no findings remain.
- The old→new delta is exactly 22 added lines across the two workflows and their existing concurrency test.
- Current
mainis still the reviewed head’s parent, so no intervening base change invalidates the result. - A live overlapping publication was not dispatched because it would mutate
gh-pages; the local active-run path plus the exact workflow permission contract settle the prior blocker without shared-state mutation.
TIME total 8m
- setup/rules: 2m — fresh loop 1.9.1 and audit 1.14.1, prior record, untrusted-code preflight
- worktree/install: 1m — dedicated exact-head checkout and lockfile-matched install
- build/server: 0m — not applicable
- browser/a11y: 0m — non-rendering change
- focused tests: 1m — 2 files, 18/18 green; syntax and actionlint also green
- code/history: 1m — exact old→new delta, permission/wait radius, caller census, and current-main relation
- critique/wiki: 3m — presentation, critic pass, delivery, record/index update
- CI wait: 0m — exact-head CI was already complete
- waste: 1m — a wiki REST lookup and one shell grep were retried via the correct routes
- A live overlapping GitHub Actions publication was not dispatched because that would mutate
gh-pages; local active-run behavior and the static workflow permission contract were verified instead.
At exact head 88195bb8e6765f7fd10fd5abf7521e09d157bc42: 14 check runs succeeded, 7 path-appropriate jobs skipped, and all 3 commit statuses succeeded. The main CI run is 33148369432.
- Vitest: 2 files, 18/18 tests passed.
-
node --check: both publisher entrypoints passed. -
actionlint: both changed workflows passed with the repository’s documented runner/shellcheck ignores. - Security preflight: all six changed workflow/script/test files were read before checkout; no dependency, package hook, or lockfile changed; focused tests use local bare remotes and a fake
ghbinary.
Posted as drafted after one critic pass.
GitHub review: https://github.com/facebook/astryx/pull/5629#pullrequestreview-5048649621