Skip to content

refactor!: move the gather's decisions into assemble.mjs, add max-prs - #69

Merged
coretl merged 2 commits into
mainfrom
review/gather-kernel-and-caps
Aug 24, 2026
Merged

coretl merged 2 commits into
mainfrom
review/gather-kernel-and-caps

Conversation

@coretl

@coretl coretl commented Aug 24, 2026 •

Copy link
Copy Markdown
Contributor

Review pass over the publish pipeline. The theme: every decision the gather makes now lives in the tested kernel, and the workflow does IO only.

select-artifacts

New subcommand deciding which artifact becomes the default branch and which PRs get a preview, given the artifacts list and the open-PR list (with fork approval pre-resolved, since that costs an API call each). It replaces ~50 lines of inline bash — the SHA index, the jq default-branch pick, the eligibility loop, the rank/cap loop — taking that step from 92 non-comment lines to 44.

It fixed two defects the bash had:

Fork isolation at an identical head SHA. The default branch chose its SHA with the head_repository_id guard, then fetched the artifact from an index built without one:

dsha=$(jq '... select(head_branch == $b and head_repository_id == $r) ... | .head_sha')
queue "$DEFAULT" "$dsha" ...   # BY_SHA[$dsha] — unfiltered

A fork's pull_request run executes in the upstream repo's Actions, so a fork PR whose head SHA matched the default branch's could have won that lookup and been staged as site/<default>. Narrow — it needs an effectively-empty PR — but it is the fork boundary in a workflow holding pages: write. The kernel uses the filtered artifact directly and never re-looks-up by SHA.

Tie-break drift. The two caps broke timestamp ties in opposite directions — sort -r kept PR #9 over #10, byDateDesc kept 1.10 over 1.9. Both now share one comparator.

max-prs

Matches max-releases: cap the previews the site carries, ranked by build date, applied after eligibility so an expired artifact or an unapproved fork PR never consumes a slot. Defaults to 0 (unlimited), so bumping the pin changes nothing until you set it.

This repo's publish.yml now sets the recommended 30/20, making it identical to a consumer's copy but for the uses: path — the file people copy is the file that is exercised. A shape check asserts both are literals, since that is what makes the policy unbypassable by a manual dispatch.

Cache key

select-releases emits its own key rather than having the workflow re-derive it with awk | sort | sha256sum. The digest is byte-identical to the pipeline it replaces (verified against live data at three cap values), so existing cache entries stay warm through the upgrade.

Removals

  • Site-size guard. actions/deploy-pages already compares the uploaded artifact against 1 GB and warns, reading the stored size — the same number the local pack was approximating. The limit and the recovery now live in the docs and in the publish.yml people copy. Note the tradeoff: their warning fires only once you are already over, so there is no early signal.
  • _sources. The write went some time ago; the deprecated read fallback has done its job.
  • docs.yml's version-name output. Nothing consumed it since the injection step, and it implied a coupling that does not exist — both sides derive the name independently. Now pinned by a test instead.
  • Comment narration. History and changelog prose removed throughout; traps and contracts kept.

Serial vs parallel downloads

Release-zip downloads are serial again. With the cache the steady state is 0–1 downloads (measured on recent runs: 0–2 s warm), which did not justify a three-pass download-and-stage structure. PR artifacts can never be cached — a head SHA changes on every push — so those stay parallel at -P 8.

Worker hardening

The parallel worker takes its row as positional args over a NUL stream instead of xargs -I{}, which pasted the row into the script text. Git permits $ and () in a branch name, and the default-branch label embeds it:

with -I{}:            label=[label 0 end]        ← $(id -u) ran
with positional args: label=[label $(id -u) end] ← inert

Not reachable today, but "inert by construction" beats "inert because nothing currently populates it" in a privileged workflow.

Tests

48 unit checks (was 34), 79 harness/shape checks (was 71). New coverage for cacheKey, selectArtifacts, the version-name contract, the cache-key step outputs, and the worker's argument wiring. Each new guard was mutation-tested — I broke the thing it protects and confirmed the check fails.

Worth recording: the harness caught a bug in my own kernel (a comparator reading .date on objects carrying created_at, which tied every artifact and returned the oldest). The unit tests would not have — I would have written them against the same wrong field. Both layers earned their place.

For consumers

Bumping the pin is a no-op unless you opt in: max-prs defaults to 0. To adopt the caps, copy the updated publish.yml from the tutorial. The only true break is docs.yml's removed version-name output.

Linking the preview back to the PR

The workflow_run migration moved publishing off the PR's critical path, which was right — but it also meant nothing on a PR told you the preview existed or where it was. This restores the link without restoring the problem.

On a successful deploy the engine posts a docs-preview commit status on the triggering commit, with the published URL as target_url. Two properties, both asserted in test_shape.py:

  • It can only ever be green. if: success() plus state=success only. A wedged Pages origin is not the PR author's to fix, so a status that could turn red would hand that straight back to them. A missing status means "not published (yet)".
  • It fails closed. The version dir is resolved from the workflow_run payload (or inputs.pr for a dispatched fork preview) and the status is posted only if that dir is actually in the tree just deployed. No link beats a link that 404s.

Fork previews link back to the exact SHA the approval was pinned to, so the contributor gets the URL from the maintainer's dispatch.

Seven harness checks cover the paths: PR build, default-branch build, dispatched fork preview, an unresolvable PR, and a manual re-deploy with no triggering commit.

While there: the Approve step now takes inputs.pr through env: instead of interpolating it into the script — the convention the rest of that file already documents and follows.

🤖 Generated with Claude Code

coretl and others added 2 commits August 24, 2026 12:45
Review pass over the publish pipeline. The theme is that every decision the
gather makes now lives in the tested kernel, and the workflow does IO only.

New `select-artifacts` subcommand: given the artifacts list and the open-PR
list (with fork approval pre-resolved, since that costs an API call each), it
decides which artifact becomes the default branch and which PRs get a preview.
That replaces ~50 lines of inline bash — the SHA index, the jq default-branch
pick, the eligibility loop and the rank/cap loop — and it fixed two defects the
bash had:

  - The default branch chose its SHA *with* the fork guard, then re-looked-up
    the artifact in an index built *without* one. A fork's pull_request run
    executes in the upstream repo's Actions, so a fork PR whose head SHA matched
    the default branch's could have been staged as site/<default>. The kernel
    uses the filtered artifact directly and never re-looks-up by SHA.
  - The release cap and the PR cap broke timestamp ties in opposite directions
    (`sort -r` kept PR #9 over #10; byDateDesc kept 1.10 over 1.9). Both now
    share one comparator.

`select-releases` likewise emits its own cache key rather than having the
workflow re-derive it with awk/sort/sha256sum. The digest is byte-identical to
the pipeline it replaces, so existing cache entries stay warm.

New `max-prs` input, matching `max-releases`: cap the previews the site carries,
ranked by build date, applied after eligibility so an expired artifact or an
unapproved fork PR never consumes a slot. This repo's publish.yml now sets the
recommended 30/20, making it identical to a consumer's copy but for the `uses:`
path — the file people copy is the file that is exercised.

Also:

  - Release-zip downloads are serial again. With the cache the steady state is
    0-1 downloads (measured: 0-2s warm), which did not justify a three-pass
    download-and-stage structure. PR artifacts can never be cached, so those
    stay parallel.
  - Removed the site-size guard. actions/deploy-pages already compares the
    uploaded artifact against 1 GB and warns, reading the stored size — the same
    number the local pack was approximating. The limit and the recovery are in
    the docs and in the publish.yml people copy.
  - Removed _sources entirely: the write went years ago, and the deprecated read
    fallback has done its job.
  - The parallel worker takes its row as positional args over a NUL stream,
    not via `xargs -I{}`, which pasted the row into the script *text* — a branch
    name may contain `$(...)`, and this workflow holds pages:write.
  - docs.yml's `version-name` output is gone; nothing consumed it since the
    injection step, and it implied a coupling that does not exist (both sides
    derive the name independently, now pinned by a test).
  - Comments trimmed throughout: history and changelog narration removed, traps
    and contracts kept.

Tests: 48 unit checks (was 34), 79 harness/shape checks (was 71). New coverage
for cacheKey, selectArtifacts, the version-name contract, the cache-key outputs,
and the worker's argument wiring — each mutation-tested.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
On success the engine posts a `docs-preview` commit status on the triggering
commit, with the published URL as its target — so a PR carries a link to its own
preview, and a branch or tag build links the version it produced. Restores the
one thing the workflow_run migration cost that was worth having, without the
thing it was fixing.

Two properties, both asserted in test_shape.py:

  - `if: success()` and `state=success` only. Publishing lives off the PR's
    critical path because a wedged Pages origin is not the author's to fix; a
    status that could turn red would hand that straight back. A missing status
    means "not published (yet)".
  - Fails CLOSED. The dest is resolved from the workflow_run payload (or from
    `inputs.pr` for a dispatched fork preview) and posted only if that version
    dir is actually in the tree just deployed — no link is better than one that
    404s.

The Approve step now also outputs the head SHA it pinned, so a fork preview links
back to the same commit it was approved against, and takes `inputs.pr` through
`env:` rather than interpolating it into the script — the rule the rest of this
file already follows.

Also adds BUILT_BRANCH to the harness env, which mirrors the real job.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coretl
coretl merged commit ecb3287 into main Aug 24, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant