-
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