Skip to content

fix: act on the CI/Actions review — prerelease ranking, injection surface, wedge detection - #70

Merged
coretl merged 11 commits into
mainfrom
review/actions-remediation
Aug 25, 2026
Merged

coretl merged 11 commits into
mainfrom
review/actions-remediation

Conversation

@coretl

@coretl coretl commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Remediation of an external CI/Actions review of .github/workflows/*, assemble/assemble.mjs, test/ and the docs that describe them. Every claim was reproduced against the code before being acted on; the two findings I handled differently from the plan are called out at the bottom.

One commit per theme — reviewable in order.

Behavioural fixes

Prereleases were ranked and detected wrongly (the worst of these). isPrerelease needed the marker to follow a digit immediately, so every hyphenated semver spelling read as stable, and git tag -l --sort=-v:refname independently ranks 1.1.0-beta.1 above 1.1.0. Together: preferredVersion returned the beta, so stable/ aliased it and the root redirect pointed at it. Reproduced in a scratch repo before fixing.

  • one MARKERS list covering PEP 440 and the hyphenated/dotted forms, digit-anchored (so release-1.0 stays stable) with a trailing lookahead (so 1.0-candidate does too)
  • ordering moves into compareTags, so "what order" and "what counts as a prerelease" share that list instead of needing versionsort.suffix kept in step by hand
  • release.yml's case globs were case-sensitive where the regex is not (1.0.0RC1 was published stable). It now uses the identical list via grep -Eqi, and test_release_prerelease.py runs 27 tag spellings through both implementations — the drift guard the "mirrors assemble.mjs" comment was standing in for
  • default branch threaded through ordering (generate --default-branch), so a develop consumer is not sorted in among the pr-<n> previews

The origin verify was blind on the commonest deploy. It compared the live switcher.json with the assembled one — but that file lists only the version set, so an ordinary docs edit on the default branch reproduces it byte-for-byte and got == want on attempt 1 whether or not the origin moved. generate now also writes site/deploy-id.txt (run + attempt + UTC), which the verify polls as the primary assertion with switcher.json kept as secondary. Safe at the site root: discoverVersions returns directories only.

Three narrow gather defects, each where the comment already stated the intended rule.

  • Number(values["max-releases"]) || 0 turned "3O" into 0 — i.e. unlimited — so a typo in a consumer's publish.yml silently disabled the guard against a 1 GB deploy failure. parseCap throws.
  • Two "newest artifact" tie-breaks: newest() broke on id, the per-SHA index kept whatever the API returned last (page order). Both go through the one comparator now.
  • Release zips were selected by asset id but downloaded by tag, so a release re-cut between listing and download would cache new bytes under the old id — the exact failure id-addressing was chosen to prevent. Fetched by asset id, with a zip-magic check, since gh api > file writes an error body before exiting non-zero.

Pages deploys queue instead of cancelling. The old cancel-in-progress: true was argued from artifact correctness, which is true and not the question: the kill lands inside deploy-pages, between upload and the backend finishing. Cost is bounded — a group holds one pending run — and stated in the comment.

base-path input so the site can be served from /. The engine resolved the Pages URL from the API precisely so a CNAME works, and the docs advertised it, but docs.yml hardcoded /<repo> — switcher links right, every page they point at 404ing its assets. New how-to.

Hardening

docs.yml interpolated contexts straight into shell. Git accepts $( ), backticks and quotes in ref names, and this is a reusable workflow: a consumer on push: branches: ['**'] hands that string to anyone who can push a branch. Values now come through env:, and the resulting name is validated against [A-Za-z0-9._-].

The general guard matters more than the fix: test_shape.py now fails on any ${{ inside any run: body, across every workflow, with no allow-list — those three lines were the only ones in the repo, so the rule starts clean. renderRedirect escapes HTML metacharacters too, since the version name it renders is validated one file away.

Also: permissions and timeout-minutes on everything (both asserted structurally — the timeout check skips uses:-only jobs, which cannot set one); actionlint pinned to 1.7.12 with a recorded SHA-256 and a Renovate custom manager; fetch-depth: 1 + fetch-tags: true for the consumer checkout, which only ever needed the tags.

Docs and comment drift

  • workflows.md still described a three-rung gather ending "the current build highest" — a pre-feat!: publish on workflow_run, deleting the trampoline, shim and injection #66 survival; there is no current-build rung under workflow_run
  • publish.yml claimed the local uses: means "the working tree is what gets tested". True post-merge, misleading pre-merge: GitHub runs the default branch's copy, so a PR touching the engine is never exercised by a real deploy — the harness is what covers it
  • stable/ is materialised as a full copy by upload-pages-artifact --dereference, and the size report ran before the symlink existed and would not have followed it. Moved after, with du -shL, and the duplication named in keep-the-site-small.md
  • documents what approving a fork preview grants: the preview runs on the org-wide ORG.github.io origin, so it is a review of the built output, not a "CI is green" check
  • pr-<n> entries in the public switcher are now documented as deliberate in both the reference and CLAUDE.md, so nobody "fixes" them later

Deviations from the review's plan

  • .mvs collision: documented as a reserved path rather than renamed — renaming churns the harness for a near-zero-probability clash.
  • Silent truncation: added the gh pr list --limit 200 warning; skipped the artifact-pagination ceiling. Confirmed the existing "N non-expired 'docs' artifact(s)" line already counts fetched, not selected.

Not done, deliberately: replacing the stable/ copy with a redirect would avoid the duplication but breaks deep links into stable/<page> — worth an issue, not a commit here.

Verification

npm test (64 checks), npm run test:workflows (gather + shape + prerelease parity), npx prek run --all-files, actionlint clean, npm run docs strict.

New coverage: prerelease/ordering cases, parseCap rejections, tie-break stability under input reordering, renderRedirect escaping, the wedge cases against a mock origin (new mock curl), asset-id download incl. failure leaving nothing cached, malformed cap exiting non-zero, PR-list truncation, base-path composition, and the four version names that must be rejected.

🤖 Generated with Claude Code

coretl and others added 11 commits August 25, 2026 09:42
`isPrerelease` required the marker to follow a digit IMMEDIATELY, so every
hyphenated semver spelling (`1.1.0-beta.1`, `2.0.0-rc.1`, `v1.2.3-alpha`) read as
stable — and `git tag --sort=-v:refname` independently ranks such a tag ABOVE the
release it qualifies. Together that made `preferredVersion` return the beta, so
`stable/` aliased it and the root redirect pointed at it.

Widen the marker set (PEP 440 + the hyphenated/dotted forms + the long spellings)
and keep the digit anchor, so `release-1.0` and `beta-program` stay stable; a
trailing lookahead keeps `1.0-candidate` out too. Order the tags here instead of
in git: `compareTags` compares digit runs numerically and ranks a prerelease below
its own release, so "what order" and "what counts as a prerelease" share one
marker list rather than needing `versionsort.suffix` kept in step by hand.

release.yml's `--prerelease` test carried the same rule in `case` globs, which are
case-sensitive where the regex is not (`1.0.0RC1` was published stable). It now
uses the identical marker list via `grep -Eqi`, and a new harness test extracts
both implementations and cross-checks 27 tag spellings through each — the drift
guard the previous "mirrors assemble.mjs" comment was standing in for.

Also stop hardcoding main/master while we are in the ordering: the engine already
knows the default branch, so thread it through `generate --default-branch`. A
consumer whose default branch is `develop` had it sorted into the leftover bucket
among the pr-<n> previews, and it could never be the preferred version.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rsion name

publish-gh-pages.yml documents the rule — every context value comes through `env:`,
never interpolated into the script — but docs.yml's "Compute version name" step
substituted `github.ref_name` straight into the body, where GitHub expands it as
script text before bash parses it. Git accepts `$( )`, backticks and quotes in a ref
name, and docs.yml is REUSABLE: a consumer triggering it on `push: branches: ['**']`
hands that string, and so shell execution in their docs job, to anyone who can push
a branch.

The ref now comes through `env:`, and the resulting name is checked against
`[A-Za-z0-9._-]` rather than assumed clean — the old comment's "no sanitisation"
was only true given THIS repo's trigger set. A rejected name fails the build with
the likely cause named.

The general guard matters more than the fix: test_shape.py now walks every job of
every workflow and fails on any `${{` inside a `run:` body, with no allow-list —
those three lines were the only ones in the repo, so the rule starts clean.

`renderRedirect` also escapes HTML metacharacters now. The version name it renders
is validated one file away, and loosening that character set should not be able to
inject markup here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ci.yml` set `permissions` only on its `release` job, and `_lint.yml`/`_test.yml`
declared none at all — so the token those jobs received was whatever the repo or org
default happened to be, potentially write-all, for two jobs that need nothing but a
checkout. No job anywhere set `timeout-minutes`, leaving a hung build-command or a
stuck `deploy-pages` to burn the 6-hour default.

`release.yml` now declares the `contents: write` it actually uses; the caller still
has to grant it, since a reusable workflow can only narrow what it is given.

test_shape.py asserts both, so a new workflow cannot quietly skip them. The timeout
check only applies to jobs with `steps` — a job that is just `uses:` cannot set one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…as uploaded

The origin verify existed to catch a wedged Pages backend serving stale bytes while
every deploy reports OK. It compared the live `switcher.json` with the assembled one
— but that file lists only the version set, so an ordinary docs edit on the default
branch reproduces it byte-for-byte. `got == want` on attempt 1 whether or not the
origin moved, i.e. the detector was blind in exactly the case where a wedge goes
unnoticed longest.

`generate` now also writes `site/deploy-id.txt` (run + attempt + UTC), which is
unique per deploy by construction, and the verify polls that as the primary
assertion with `switcher.json` kept as a secondary one — a current stamp with a
stale switcher means a partially-served deploy. A file at the site root is safe:
`discoverVersions` returns directories only.

While in the same step, move the size report there from the extract step and measure
with `du -shL`. It ran BEFORE the `stable/` symlink was created, and would not have
followed it anyway, while `upload-pages-artifact` tars with `--dereference` — so the
newest release is uploaded twice and the number an operator tunes `max-releases`
against omitted it. It now names the duplication explicitly, as does
keep-the-site-small.md.

The harness gets a mock `curl`, so the wedge cases are tested rather than reasoned
about: stale stamp with an identical switcher.json fails, both current passes on the
first attempt.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y claim

Three narrow defects in the gather, each where the code's own comment already
stated the intended rule:

- **A mistyped cap disabled the cap.** `Number(values["max-releases"]) || 0` turns
  `"3O"` into NaN and NaN into 0 — which means unlimited, so a typo in a consumer's
  publish.yml silently switched off the guard that exists to stop a 1 GB deploy
  failure. `parseCap` accepts a non-negative integer or throws, and the step exits
  non-zero.
- **Two tie-break rules for "newest artifact".** `newest()` breaks a timestamp tie on
  id; the per-SHA index kept whichever artifact the API returned last, i.e. page
  order — not a rule at all, and reachable whenever two artifacts share a second (a
  matrix, a re-run). Both now go through the one comparator, as the comment claimed.
- **Release zips selected by asset id, downloaded by tag.** The cache is
  id-addressed because "an id names immutable bytes", but `gh release download <tag>
  -p docs.zip` would hand a release re-cut since the listing to the OLD id, cached
  for every later deploy. Fetch the asset endpoint by id instead, and check the zip
  magic: with stdout redirected to a file, a failed request still writes an error
  body, so the exit status alone would let a non-zip into the cache.

Also warn when `gh pr list --limit 200` truncates: 201 open PRs looked exactly like
200, with nothing in the log to explain why some PRs had no preview.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The engine resolves the site's URL from the Pages API precisely so a custom domain
works, and the reference doc advertised that — but docs.yml hardcoded
`BASE_URL=/<repo>/<version>`. On a CNAME'd domain or an ORG.github.io repo the site
is served at `/`, so switcher.json's links were right while every page they pointed
at had 404ing assets: the worse half of the two failure modes.

`base-path` defaults to `""` (→ `/<repo>`, unchanged for every current consumer) and
takes `/` for a root-served site. BASE_URL is now composed in the compute step rather
than an `env:` expression, because normalising the slashes needs a shell.

Adds docs/how-to/use-a-custom-domain.md, and scopes the reference's CNAME claim to
what is actually true of each half.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The harness ran docs.yml's compute step for the version-name contract only. It now
also reads back BASE_URL — the other half of that contract — across the base-path
default, a root-served site and a slash-normalisation case, and asserts the four
version names that must be rejected outright.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cancel-in-progress: true` was justified from artifact correctness — every deploy
reconstructs the whole site, so the replacement gathers whatever the cancelled one
would have. That is still true and still not the question: the cancellation lands
inside `deploy-pages`, in the window between the artifact upload and the Pages
backend finishing with it, and that state is not ours to reason about. GitHub's own
Pages starter workflow uses `false` for this reason, and this workflow carries a
whole step for detecting a wedged origin, so the failure mode is not hypothetical
here.

The cost is bounded and worth stating: a concurrency group holds one pending run, and
a newer arrival replaces the pending one before it starts, so a burst of pushes costs
one extra full-site deploy rather than a queue of them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`_lint.yml` resolved `rhysd/actionlint/releases/latest` at run time and executed the
tarball unverified — the one unpinned, unchecked binary in a repo that pins
`astral-sh/setup-uv` to a full version and explains why in a comment. It also made
every lint run depend on the releases API being up and un-rate-limited.

The version is now a literal with its SHA-256 next to it, checked before extraction.
renovate.json gets a custom manager for the version; Renovate cannot compute the new
digest, so a bump PR fails the checksum until the digest is updated in the same PR —
fail-closed, and obvious when it happens.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`publish-gh-pages.yml` cloned the consumer's whole repo (`fetch-depth: 0`) for one
reason: `assemble.mjs` runs `git tag` to order their versions. `fetch-depth: 1` plus
`fetch-tags: true` gets exactly that, on every deploy of every consumer — some with
years of history. docs.yml is deliberately left cloning in full, since a
build-command may run `git describe`.

Also names `.mvs/` as a reserved path in the consumer's workspace, in the reference
and next to the checkout that creates it, and documents what approving a fork preview
actually grants: an approved preview runs on the org-wide `ORG.github.io` origin, so
it is a review of the built output, not a "CI is green" check.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ates them

- workflows.md described a gather with three rungs "…then the current build highest".
  There is no current-build rung: #66 moved publishing to `workflow_run`, which fires
  after the triggering run completed, so that build is simply the newest CI artifact.
  Now describes the two real rungs plus the default branch's cache fallback.
- publish.yml claimed the local `uses:` means "the working tree is what gets tested".
  True post-merge, misleading pre-merge: under `workflow_run` GitHub runs the DEFAULT
  branch's copy, and `uses: ./…` resolves against that same commit, so a PR touching
  the engine is never exercised by a real deploy before it merges — the harness is
  what covers it. That second half is the part a maintainer needs.
- The version-name contract said "no sanitisation"; it is validated now.
- CLAUDE.md and architecture.md carried the old prerelease rule, `cancel-in-progress:
  true`, the pre-symlink `du -sh`, and no mention of `base-path`.
- Documents, in both the reference and CLAUDE.md, that `pr-<n>` entries in the public
  switcher are deliberate — so the next reader does not "fix" them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coretl
coretl merged commit 6f5b95b into main Aug 25, 2026
5 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