754add31e2
This follows the user's rulings: CI should not care which bash or which standard utilities it runs, and jobs take what the ci dev shell provides. - checkers.sh: remove find_modern_bash, the 5.1 floor (improvise-fd0j) and exit 3. Suites run under $BASH with PATH unchanged. - checkers-test.sh: remove sections 2, 5 and 5b (the fake bashes and utilbin). They existed only to pin the floor. - scripts/kb: drop the BSD `stat -f` fallback. kb needs the dev shell for tpl, and that shell's stat is GNU. - ci.yml: remove every `shell: bash` override, so every step runs in the dev shell. Remove the web-toolchain version probe and the Playwright version/browser probe. The shell's PLAYWRIGHT_BROWSERS_PATH pins the browsers. npm ci stays because @playwright/test is not nix-provided. - flake.nix: update the comments that promised those CI checks. Reasoning: context/code/tooling.md section 5. Evidence: context/test-evidence/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
748 lines
43 KiB
Markdown
748 lines
43 KiB
Markdown
# The repository's own checkers
|
||
|
||
`scripts/` holds the gates this repo runs on itself. They are code, and they are
|
||
held to the rule they exist to enforce: a check nobody has seen fail is
|
||
indistinguishable from one that always passes, so each of them carries a
|
||
self-test whose cases are injected faults
|
||
(`doctrine(check_the_checker)`).
|
||
|
||
Four docstrings in `scripts/check-comment-runs.py` point here. This is the file
|
||
they mean.
|
||
|
||
---
|
||
|
||
## 1. The comment cap
|
||
|
||
`check-comment-runs.py` fails a `.rs` file carrying more than two consecutive
|
||
comment lines. The rule is `context/design-principles.md` §6: reasoning lives in
|
||
`context/` prose, where it can be read, corrected and audited, not in a comment
|
||
that nothing checks against the code.
|
||
|
||
The whole difficulty is deciding what *is* a comment line. Ten silent successes
|
||
have been found in this one function, every one of them failing in the direction
|
||
that looks like passing — a file that should have been flagged and was not:
|
||
|
||
- `"test" in line` matched `#[cfg(test)]`, so every cfg-gated fixture cleared a
|
||
gate written to reject exactly those. **Anchor the match**; an unanchored
|
||
substring is a filter that over-matches, and a filter that over-matches fails
|
||
in the direction that looks like success. (The same class as the bd ID audit's
|
||
exclusion list, and as `_JS_TEST`'s exclusion of `.skip`/`.only`.)
|
||
- A `proptest!` flag was set and never cleared, so the first `proptest!` in a
|
||
file made every later `fn` — helpers included — count as a test.
|
||
- An attribute holding a bracket inside a string (`#[cfg(feature = "[")]`)
|
||
left the bracket depth unbalanced and exempted the rest of the file. Every
|
||
question is therefore asked of the **scanner**, never of the raw line: the
|
||
scanner knows Rust strings, raw strings, char literals and nested block
|
||
comments, and the attribute depth is counted over code text only.
|
||
- A fence that opens and never closes exempted everything after it.
|
||
|
||
Two rules that are decisions rather than accidents. A ``` fence **suspends** the
|
||
run rather than ending it, so two lines above a fence and two below is a run of
|
||
four — a reader meets it as one uninterrupted block. And only a *runnable* fence
|
||
earns the exemption: rustdoc executes a bare ``` or ```rust and does not execute
|
||
```text, ```ignore or ```no_run, so the rest is prose wearing a fence.
|
||
|
||
## 2. The evidence check
|
||
|
||
`--check-evidence` holds `context/test-evidence/` and the tests it describes to
|
||
each other, in two directions:
|
||
|
||
- **Orphans.** Every heading shaped like a test name must name a test that runs.
|
||
- **Unstated.** Every test whose doc comment cites `context/test-evidence/` must
|
||
have an entry there.
|
||
|
||
Neither direction is "every test has evidence". Most tests have none and should
|
||
not be forced to have one; what is checkable is a *claim*, and both directions
|
||
are claims. The failure they close is the same one a dead ticket ID is: a
|
||
pointer that reads as a promise the thing exists.
|
||
|
||
### A leaf is not an identity
|
||
|
||
The names cargo lists are **module paths**, and the checker keeps them whole.
|
||
Six leaf names collide in this workspace — two of them cited in evidence — so
|
||
reducing a path to its last segment made a heading satisfiable by a test in
|
||
another module answering another question. Renaming
|
||
`ui::project::tests::a_load_that_fails_is_not_a_rebase`, the one its entry
|
||
actually describes, left the checker at *579 entries, all name a test that
|
||
runs*, exit 0: the surviving `ui::host::tests` namesake answered for it.
|
||
|
||
So a bare leaf resolves only when it is **unique**, and an ambiguous one is a
|
||
**refusal** naming the candidates — not a warning, because a warning re-creates
|
||
the problem for anyone who does not read it. The heading is then written with
|
||
its module path, which is also what makes the entry say which test it is about.
|
||
|
||
Note the two halves are both needed and neither is sufficient. The refusal
|
||
forces qualification but cannot survive a rename on its own: rename one of a
|
||
colliding pair and the leaf becomes *unique*, silently resolving to the
|
||
survivor. Qualification is what closes that, and the refusal is what makes
|
||
somebody write it.
|
||
|
||
What stays open is collision *across binaries*: `--list` prints module paths
|
||
with no crate and no binary, so two crates sharing one could not be told apart
|
||
(improvise-sxsf). Measured 2026-09-21 and still latent — 1444 names, **zero**
|
||
duplicate full paths, and zero duplicate `(crate, path)` pairs.
|
||
|
||
The fix the ticket proposes — `--list` per package with the crate prefixed —
|
||
was measured and is **declined twice over**. It costs 45.2s against 2.4s for
|
||
the one workspace call (11 members, warm), 18× for an unreachable case, and
|
||
`--check-evidence` needs the `--ignored` twin too, so the real figure is 22
|
||
invocations. More to the point **it does not close the class it names**: a
|
||
package is not a binary. `improvise` alone carries five integration test
|
||
binaries beside its lib, and an integration test's top-level `fn` lists with no
|
||
module path at all — 60 of the 1444 names are bare — so `-p improvise` prefixes
|
||
all six identically and `tests/a.rs::foo` still cannot be told from
|
||
`tests/b.rs::foo`. A fix that ever becomes worth having is per-*target*
|
||
(`--lib`, `--test <name>`), not per-package, and the ticket's proposal would
|
||
have bought 18× for a partial cure.
|
||
|
||
### The names come from cargo, not from the source
|
||
|
||
A heading is diffed against `cargo test --workspace -- --list`, minus the names
|
||
that `--list --ignored` reports. It used to be scraped out of `.rs` text, and
|
||
four shapes defeated that, all of them "evidence for a check that never runs"
|
||
(improvise-rm4s):
|
||
|
||
| shape | why the scrape accepted it |
|
||
|---|---|
|
||
| `#[test] fn x` inside a raw string literal | the scanner reads inside strings |
|
||
| `#[cfg(feature = "…")] #[test]` | the feature is never enabled |
|
||
| `#[cfg(target_arch = "wasm32")] #[test]` | `cargo test --workspace` is native |
|
||
| `#[test] #[ignore]` | it compiles, and never runs |
|
||
|
||
The third is the realistic one here: this repo has real wasm32-only code and a
|
||
`wasm` CI job that only runs `cargo check`. Adopting the stricter source cost
|
||
nothing — all 539 headings resolved against a real listing on the commit that
|
||
made the change — which is the moment to tighten a gate rather than the moment
|
||
to argue about it.
|
||
|
||
Demonstrated before the fix, by injecting all four shapes into `src/draw.rs` and
|
||
`context/test-evidence/tui.md` at once: the previous checker printed *543
|
||
entries, all name a test that exists*, exit 0. The current one names all four.
|
||
|
||
What this costs: `--check-evidence` now needs a toolchain and a compiled
|
||
workspace, about three seconds warm. A missing `cargo` says so rather than
|
||
raising. The comment-cap check is unaffected and needs neither.
|
||
|
||
`--rust-tests FILE` takes a saved listing for a caller that has one, and it
|
||
**requires `--rust-tests-ignored FILE`**: a first version took one file, called
|
||
`cargo_test_names` with one argument, and so skipped the ignored rule entirely
|
||
while printing the same success line as the honest path. A flag that is
|
||
strictly more permissive and indistinguishable in the log is a bypass, not a
|
||
convenience — so the pair is mandatory and the success line **names its
|
||
source**. CI passes neither and runs cargo itself.
|
||
|
||
A **missing** `context/test-evidence/` is not an early return either. It used to
|
||
be, and it exited 0 while 170 tests cited a directory that was not there; the
|
||
walk now yields nothing and every citation is reported unstated, which is loud.
|
||
|
||
Note what the self-test can and cannot carry here. The listing *reader* is
|
||
testable from fixtures — a leaf name taken off a module path, an `--ignored`
|
||
name subtracted, cargo's own prose not mistaken for a test — and those cases
|
||
are in `SELF_TEST_LISTING`. The four *source shapes* are not: once `defined`
|
||
comes from cargo, a fixture asserting "a heading with an empty listing is an
|
||
orphan" is the same case however the Rust beside it is spelled, and writing
|
||
four of them would be four copies of one assertion dressed as coverage. The
|
||
evidence for those is the injection above, against the real tree.
|
||
|
||
What it does **not** close: `citing` is still scraped, so a citation inside a
|
||
string literal would be reported as a test owing an entry. That is a false
|
||
positive in the safe direction — loud, not silent — and it has no instance.
|
||
|
||
### A demotion is a reviewer's job, not a gate's
|
||
|
||
A heading is classified by **shape**: a bare identifier is a Rust test, a quoted
|
||
string a JS test, anything else is prose about the section (`Helper — x`,
|
||
`Note — y`, a file's own scope). So any orphaned heading can be retired by
|
||
rewriting it with a space and a dash, and nothing can check whether the prose is
|
||
honest — no mechanism could, because the question is whether a sentence is true
|
||
(improvise-9za9).
|
||
|
||
Two things make that tolerable. The unsafe direction is the one that fails
|
||
loudly: prose that *looks* like an identifier is classified as a Rust claim and
|
||
reported as an orphan, so the error you can make by accident is the noisy one.
|
||
And **the mechanism is `git diff context/test-evidence/`** — a demotion shows
|
||
up there as a `### identifier` line becoming prose, in the diff every reviewer
|
||
already reads. The live instance, PR #69 demoting a heading while PR #72
|
||
restored its coverage under another name, is what says a reviewer rather than a
|
||
gate has to catch it.
|
||
|
||
The printed count of prose headings is a **convenience, not a guard**: nothing
|
||
stores the previous count and nobody diffs two CI logs, so a retirement moves a
|
||
number no one is watching. Saying otherwise would claim a detectability it
|
||
cannot support, which is the class of defect §7's adjacent-check section is
|
||
about.
|
||
|
||
### A flag adds a question; it never selects one away
|
||
|
||
`--check-evidence` used to **early-return**, so the flagged run answered the
|
||
evidence question and said nothing at all about the comment-run cap. Three
|
||
agents hit that independently in one campaign, and in one case a local gate went
|
||
green while CI went red on a three-line doc comment (improvise-ijql). It is now
|
||
additive: `main` runs `check_cap` first, then `check_evidence`, and returns the
|
||
worse of the two.
|
||
|
||
The ticket offered a cheaper shape — leave the early return and print one line
|
||
saying the cap did not run — on the precedent of PR #83 making `--rust-tests`
|
||
name its source. That precedent does not reach this case, and the distinction is
|
||
worth keeping. **A provenance line and a disclaimer are not the same object.**
|
||
Naming which listing the names came from qualifies an answer the run actually
|
||
gave; saying "the cap did not run" qualifies an answer it did not give, while
|
||
still **exiting 0**. Exit 0 is what CI branches on and what a human reads as
|
||
clean, so a disclaimer leaves the gate exactly as skippable and merely documents
|
||
the skip. The rule the fix is written to is therefore stated over the exit code:
|
||
**a tree with a long comment run must not pass any invocation that can exit 0.**
|
||
|
||
Which invocations those are is a decision, not a sweep. `--self-test` and
|
||
`--list-doctests` are excluded and neither is an exception to the rule: the
|
||
first scans no tree, and the second is a *listing* that renders no verdict. Only
|
||
the two that answer "is this tree clean" are in `SELF_TEST_DISPATCH`.
|
||
|
||
That table is checked by running the script as a **subprocess** against a
|
||
temporary tree, which is the only level at which this bug is visible — every
|
||
unit in this file was correct, and the defect lived in `main`'s dispatch. Both
|
||
arms are needed and both were demonstrated: restoring the early return fails
|
||
`DISPATCH: --check-evidence: exited 0 over a four-line comment run`, and forcing
|
||
the cap to always report a violation fails the clean arm for both invocations,
|
||
which is what stops an always-fail from satisfying the first. The temporary tree
|
||
also has no `context/test-evidence/` and no JS roots, so the evidence half is
|
||
vacuously clean there and the cap is the only thing that can decide the exit
|
||
code — the isolation is what makes the assertion name one question.
|
||
|
||
### Two headings, one test
|
||
|
||
An orphan is a heading whose test nobody can find; the mirror image is a test
|
||
that **two** headings answer for, and it is invisible to every rule above —
|
||
each copy names a test that runs, so each copy passes. PR #92 shipped two
|
||
entries under one heading where the older described a fixture the test no
|
||
longer had and repeated a claim an adversarial review had measured false. A
|
||
reader meeting the older one first has no way to know a successor exists two
|
||
thousand lines down, so the stale copy reads as current. That is worse than an
|
||
orphan, which at least announces itself as broken.
|
||
|
||
It is a **refusal and not a warning**, on the precedent `improvise-rm4s` set for
|
||
the ambiguous leaf: the two are the same defect seen from opposite ends — one
|
||
name answered by two tests, and one test answered by two names — so they are
|
||
answered the same way. The refusal names the file and every line, in file order.
|
||
|
||
The key is the **resolved test, not the spelling**. `### f` beside
|
||
`### m::tests::f` is two headings for one test, and grouping by the string would
|
||
miss it precisely because qualification is the repair `rm4s` asks for; a heading
|
||
qualified in one file and bare in another would have been the first way this
|
||
came back. Resolution reuses the same leaf table the ambiguity rule uses, so a
|
||
leaf that is genuinely ambiguous canonicalises to itself and is reported as
|
||
ambiguous rather than as a duplicate — two *different* tests sharing a leaf are
|
||
not two entries for one test, and conflating them would file the wrong repair.
|
||
The check is over the whole directory rather than per file, because a heading
|
||
keys a test name and a test name is unique workspace-wide once qualified.
|
||
|
||
What it still cannot see, and neither can any of the others: an entry that
|
||
exists, names a live test, is the only one that does — and describes it
|
||
backwards.
|
||
|
||
## 3. The others
|
||
|
||
`scripts/kb` is the only writer of `prolog-sessions/kb.pl`; its own reasoning is
|
||
in `context/code/kb-tool.md`. `scripts/node-test.sh`, `scripts/kb-test.sh`,
|
||
`scripts/pr-watch-test.sh` and `scripts/assemble-stamp-test.sh` are the same
|
||
shape as this file's subject: a guard, plus a suite that makes the guard refuse
|
||
what it claims to refuse. Each is wired into `.github/workflows/ci.yml` in the
|
||
same job as the thing it guards, because a check that only runs locally is a
|
||
convention.
|
||
|
||
`scripts/check-ci-graph.py` is the workflow's own gate: `pages-deploy` must
|
||
`needs` every other job in `ci.yml`. The rule "every check job" was written down
|
||
(improvise-x7cq) and a job added during review of PR #83 was still ungated with
|
||
nothing noticing, which is what moved it from a sentence to a check. It reads
|
||
the job keys and the `needs:` list off indentation, tracking which job a
|
||
`needs:` belongs to — taking the first one seen would read another job's list as
|
||
the deploy's and pass. `README.md` therefore states the rule and deliberately
|
||
**does not list the jobs**: a second copy with nothing holding it to the first
|
||
is what went stale, twice.
|
||
|
||
### pr-watch and mergeability (improvise-edpl, improvise-zlj2)
|
||
|
||
`scripts/pr-watch` is what the orchestrator's merge procedure stands on, and
|
||
until improvise-edpl its verdict failed only on `mergeable == CONFLICTING`.
|
||
GitHub computes mergeability lazily: for a while after the base moves it reports
|
||
`UNKNOWN` for both `mergeable` and `mergeStateStatus`. PRs #172 and #179 both
|
||
exited 0 at `UNKNOWN/UNKNOWN … 0 failed`. A PR that has *just become*
|
||
conflicting is in exactly that window, so it could pass the watch. It is the
|
||
same shape as improvise-b4ks: an uncertain state read as success.
|
||
|
||
The rule now:
|
||
|
||
- **`UNKNOWN` in either field is not an answer, so it is waited on.** On
|
||
timeout it exits 3, the same code as a check still running. It never exits 0.
|
||
- **`MERGEABLE/BLOCKED` with no failed check is waited on too** (improvise-zlj2).
|
||
A required check that never runs therefore times out at exit 3, and does not
|
||
exit 1.
|
||
- **A known answer ends the wait early**: a failed check (exit 1 whatever
|
||
mergeability says, since waiting would turn a known red into a timeout), or
|
||
`mergeable == CONFLICTING`. "Whatever mergeability says" includes
|
||
`MERGEABLE/BLOCKED`, which is how a failed *required* check reports here, so
|
||
it is the commonest red this script sees (improvise-9pfs).
|
||
- **A PR that is no longer open ends the wait on the first poll that sees it
|
||
closed, with exit 5** (improvise-1qaa). That poll is the first when the PR was
|
||
already merged, and can come mid-wait when someone merges it during the watch.
|
||
A merged PR reports `UNKNOWN/UNKNOWN` permanently (#189, read live), so the
|
||
UNKNOWN wait slept the whole timeout there and then blamed mergeability. A
|
||
closed PR's mergeability is whatever it was when it closed, so it can land on
|
||
either side of the wait. #74 reads `CONFLICTING/DIRTY`, which ends the loop at
|
||
once, but as a misleading exit 1. #46 has green checks and reads
|
||
`MERGEABLE/BLOCKED`, which the loop waits on, so it slept the full timeout
|
||
too (improvise-29w2). The suite carries all three shapes, and only the
|
||
waited-on ones can tell whether the state is checked at the top of the loop.
|
||
The script now fetches `state`. Exit 5 is
|
||
its own code, not 1 or 0, because under merge-as-soon-as-green "someone else
|
||
already merged it" is a normal answer, and the caller should hear it as that.
|
||
- **The head checks come first, and the ended check after them**
|
||
(improvise-24lz). A PR that merged at a head the watcher never saw is the
|
||
PR #67 hazard in a new form. It exits 2, whether the head moved during the
|
||
watch or `--expect` names another. Only then is a closed PR exit 5. On the
|
||
first poll the pin block only records the head, so an already-merged PR
|
||
still ends on its first read.
|
||
- **The pass condition is an allowlist**: `mergeable == MERGEABLE` *and*
|
||
`mergeStateStatus` in `CLEAN`, `UNSTABLE` or `HAS_HOOKS`. Anything else exits
|
||
1. That includes `null` and any state GitHub adds later, so the check fails
|
||
closed.
|
||
|
||
Why each state is where it is. The `main` ruleset (`gh api
|
||
repos/fiddlerwoaroof/improvise/rules/branches/main`, read 2026-09-22) requires
|
||
three status checks. It requires no approvals, and its required checks are not
|
||
strict.
|
||
|
||
- `BLOCKED` with a `MERGEABLE` flag means a required check has not passed. A
|
||
failing one is already in the rollup, so the script has exited 1 before it
|
||
reads the state. What is left is a required check that **has not registered
|
||
yet**, and every healthy push passes through that state. The required
|
||
`Browser harness (OPFS contract + shell)` check comes from the `browser` job,
|
||
which `needs: build`. GitHub creates its check run only after `build`
|
||
completes. For a second or three per push, then, every registered check is
|
||
`COMPLETED/SUCCESS`, the required one does not exist, and GitHub reports
|
||
`MERGEABLE/BLOCKED`. The first version of this rule refused `BLOCKED`, and so
|
||
exited 1 on a healthy PR whenever a poll landed in that gap. The reviewer put
|
||
that at 5–15% of watches at the 20s interval. An "unrun" required check and a
|
||
"not yet registered" one look the same from the rollup, so the state is
|
||
waited on. A check that genuinely never runs costs a timeout, exit 3, not a
|
||
false exit 1. An earlier version of this section said that nothing else
|
||
produces `BLOCKED` here. That was wrong: the `needs:` gap produces it.
|
||
- `DIRTY` can wear `MERGEABLE` for a moment while the two fields settle. It is
|
||
refused.
|
||
- `BEHIND` needs strict required checks, which this repo does not set. `DRAFT`
|
||
cannot be merged. Both are refused.
|
||
- `UNSTABLE` means a non-required check is unhappy. It stays a pass, as it was
|
||
before. It is safe because the loop only reads the state once every
|
||
registered check is `COMPLETED`, and any failed one has already exited 1. So
|
||
whatever `UNSTABLE` is reporting, it is not a check in the rollup that is
|
||
still running or red.
|
||
- `HAS_HOOKS` is GitHub Enterprise's "mergeable, with pre-receive hooks". It is
|
||
harmless to allow.
|
||
|
||
The suite enumerates the whole `mergeable × mergeStateStatus` table, plus an
|
||
unshipped value and `null`, rather than hand-picking cases. Why: improvise-90ea.
|
||
|
||
The suite also plays the `needs:` gap through, poll by poll: build running, then
|
||
build done with no harness check, then the harness running, then `CLEAN`. That
|
||
sequence must exit 0. The rollup it starts from is PR #189's, copied from
|
||
`gh pr view --json`. Each earlier poll is derived from it with `jq`, by dropping
|
||
entries or rewinding them to `IN_PROGRESS`.
|
||
|
||
### Why the Pages deploy fails on "Found 0 artifact(s)" (improvise-864a)
|
||
|
||
`actions/deploy-pages@v5` intermittently fails a push to `main` with *No
|
||
artifacts named "github-pages" were found for this workflow run*, while the
|
||
upload step was green in that same run. The ticket framed this as concurrent
|
||
merges racing on one Pages environment, and PR #46 proposed skipping the deploy
|
||
unless the commit is the tip of `main`. **Both the framing and the guard are
|
||
wrong, and measurement says so.**
|
||
|
||
**What was measured.** All 100 push-to-`main` CI runs from 2026-09-19T22:02Z to
|
||
2026-09-21T05:02Z, from the run, job, step and log APIs (job metadata and logs
|
||
outlive artifacts, which expire in a day and are all 404 now — so this is
|
||
reproducible where the ticket's own artifact-count table is not):
|
||
|
||
| deploys | with another run's SUCCESSFUL deploy completing inside this run's own upload→deploy window |
|
||
|---|---|
|
||
| 12 artifact-not-found failures | **12 (100%)** |
|
||
| 56 successes | 2 (3.6%) |
|
||
| 4 other failures | 0 — all four are the unrelated *Create Pages site failed: Resource not accessible by integration*, from before `enablement: false` (improvise-3gy) |
|
||
|
||
**The direction is settled, and it is neither of the two the ticket offered.**
|
||
The oldest run of the 17:47Z batch failed at 17:47:23Z — *before* the next
|
||
deploy in the batch had started (17:47:31Z) and before any Pages deployment
|
||
existed at all, since `deploy-pages` fetches the artifact metadata *before* it
|
||
creates a deployment. So the failure cannot have been caused by this run's own
|
||
deploy, by supersession, or by a concurrent deploy. The artifact was already
|
||
gone. **The reaper is another run's *successful* deployment.**
|
||
|
||
**It is scoped by artifact NAME, not by run.** In run 35560726456 the build job
|
||
finalized three artifacts seconds apart — `harness-serve`, `wasm-bundles` and
|
||
`github-pages`, each with an id — and 2m21s later that run's own lookup reported
|
||
*Found 2 artifact(s)* and no `github-pages`. Same run, same token, same call,
|
||
same retention: the two siblings survived and only `github-pages` was missing.
|
||
That one observation rules out the whole family of general explanations —
|
||
service flakiness, eventual consistency, retention, a token scope, a v3/v4
|
||
backend mismatch — because any of them would have taken the siblings too.
|
||
|
||
**Four things this kills.** Queue depth is not the mechanism: the three deploys
|
||
of the 17:47Z batch never queued, running 9s and 17s apart with each failure
|
||
taking one second, so the `pages` group serialized nothing. Retention is not it
|
||
either — all three shared `retention-days: 1`, and since the *newest*-finalized
|
||
expires last, expiry can produce 0/1/1 or 0/0/1 but never the ticket's 0/1/0.
|
||
Nothing in `deploy-pages` deletes an artifact (an upstream feature request asks
|
||
for exactly that capability, which is only open because nothing does). And the
|
||
action-version theory dies on the interleaving: successes and failures alternate
|
||
minute by minute on identically-resolved action SHAs.
|
||
|
||
**One clause of the ticket's own correction is falsified, and the conclusion
|
||
still holds.** The correction argued the tip-of-`main` guard would have skipped
|
||
the one deploy that published "and let 52507a6 deploy and fail anyway" — which
|
||
assumed 52507a6's artifact loss was independent of its neighbour. It was not:
|
||
5f871f7's deploy completed at 17:47:40Z, four seconds before 52507a6's began.
|
||
Fewer successful deploys really is the right lever, so PR #46 aimed at the right
|
||
quantity for the wrong reason. **It should still not be shipped**, because the
|
||
association is not a mechanism and the mechanism is not deterministic: 5f871f7
|
||
had a foreign deploy land inside its own window too and survived. A guard
|
||
measured at nothing, against a probabilistic effect, is
|
||
`doctrine(measure_the_obvious_fix_before_reporting_it_as_the_fix)` exactly.
|
||
|
||
**What the evidence does license.** The risk lives entirely in the upload→deploy
|
||
window — every victim had a foreign deploy land inside its own — so shortening
|
||
that window is the one lever with evidence behind it, and consolidating the
|
||
build already did some of that. Closing it properly means no two runs holding a
|
||
`github-pages` artifact across a deployment, which is a real change to the job
|
||
graph with real costs and is not attempted here. A retry is **not** a fix: the
|
||
artifact is gone rather than transiently unlistable, so a retry cannot find it.
|
||
|
||
### 4. The release preflight, and why a loud failure was not enough
|
||
|
||
`scripts/release-preflight.sh` answers, before a tag exists, the question the
|
||
Release workflow answers after. **The procedure is `RELEASING.md`**; this
|
||
section is the argument behind it, and the split is deliberate — a checklist
|
||
followed at 2am is not read for its reasoning, and one that carries it does not
|
||
get followed.
|
||
|
||
**The root cause was a missing document, not a missing check.** Nothing in this
|
||
repo said *bump the version before you tag* — `release.yml` is cargo-dist's
|
||
autogenerated mechanism, `README.md` §Installation describes the result, and
|
||
`context/plan.md` Phase 3 carries one-time *setup* instructions from the launch
|
||
plan. So the preflight is the second half of the fix; `RELEASING.md` is the
|
||
first.
|
||
|
||
**What happened.** `v0.1.0-rc3` was pushed while every
|
||
crate still said `0.1.0-rc2`. dist matches a tag against the root package's
|
||
version, refused with *"This workspace doesn't have anything for dist to
|
||
Release!"*, and the four jobs behind `plan` skipped. **Nothing was wrong with
|
||
the workflow**, which is worth stating plainly because the instinct on a failed
|
||
release run is to edit the YAML. The trigger is correct, the pattern matches,
|
||
the workflow is enabled; `dist plan --tag=v0.1.0-rc2` succeeds today and
|
||
`--tag=v0.1.0-rc3` reproduces the CI error exactly.
|
||
|
||
**The decisive evidence is a shipped release, not that local run**, and an
|
||
earlier draft of this passage rested on the weaker half. `v0.1.0-rc1` and
|
||
`v0.1.0-rc2` both carry **complete published GitHub Releases** from
|
||
2026-04-11 — tarballs for all four targets, their `.sha256` files,
|
||
`sha256.sum`, `dist-manifest.json`, `source.tar.gz` and
|
||
`improvise-installer.sh` (`gh release view v0.1.0-rc2 --json assets`). So the
|
||
workflow is not merely *expected* to work: it **demonstrably ran end to end**
|
||
the last time a tag and the version agreed. Note how nearly the opposite was
|
||
concluded — `gh run list` came back empty and was read as "only one tag push
|
||
ever ran this", when Actions logs simply age out at 90 days and those tags are
|
||
five months old. **An empty log is a statement about retention, not about
|
||
history**; the artefact a run *published* outlives the run, so ask the releases
|
||
before asking the runs. "The four jobs behind `plan` skipped" is true of the
|
||
rc3 run alone and must not be read as "no release has ever been built".
|
||
|
||
**A tag push has failed this way before — and how much that proves is worth
|
||
being exact about.** `gh run list --workflow=release.yml --event=push` keeps the
|
||
whole tag history, and it reads: rc1 **failed twice** (14s each) before
|
||
succeeding on the third push (3m25s), rc2 succeeded first time (3m9s), rc3
|
||
failed (21s). All three failures carry the **identical signature** — `plan:
|
||
failure` with `build-local-artifacts`, `build-global-artifacts`, `host` and
|
||
`announce` all `skipped` — checked per job, not inferred from the durations.
|
||
|
||
**A shared signature is not a shared cause, and rc1's is not knowable now.**
|
||
Only rc3's was reproduced (`dist plan --tag=v0.1.0-rc3`); rc1's logs aged out at
|
||
90 days, and its two failures sit on commits titled *"feat: cargo dist info"*
|
||
and *"chore: update version"*, i.e. during initial setup, where `plan` could as
|
||
easily have failed on an incomplete `dist` config. So the honest claim is: this
|
||
failure **shape** has now cost four pushes across two releases, and the one time
|
||
anyone has diagnosed it, it was the version/tag disagreement. That is ample
|
||
reason for `RELEASING.md` and a check before the irreversible step, and it is
|
||
**not** a claim that the same mistake was made three times — a first draft of
|
||
this paragraph said exactly that, from the durations alone, which is the same
|
||
asserted-not-asked error the two paragraphs around it correct.
|
||
|
||
The signature is worth memorising because it is so cheap to read: **`plan` fails
|
||
in ~20s with four jobs skipped**, where a real build takes about three minutes.
|
||
Note
|
||
the workflow also triggers on `pull_request`, where it plans without a tag and
|
||
passes in ~20s — so a *green* 20s Release run on a PR and a *failed* 20s one on
|
||
a tag look alike at a glance and mean opposite things.
|
||
|
||
**Why a loud, precise failure still cost something.** dist named the right
|
||
answer in its own error text — *"`--tag=v0.1.0-rc2` will Announce: improvise"*.
|
||
The problem is not the message, it is **when** it arrives: a tag is public and
|
||
immutable by the time CI reads it, so the repair is a new version rather than an
|
||
edit, and the tag that failed stays published forever. A check that runs
|
||
after the irreversible step is a check in the wrong place, however good its
|
||
message. So the preflight runs *before* tagging and prints the tag command only
|
||
once it has passed — the honest path made the easy one, the same shape as
|
||
`Basis` having no public constructor (gotcha 43).
|
||
|
||
**"All three remotes" was wrong when it was finally checked.** `aa4e901`'s
|
||
commit message said `v0.1.0-rc3` had been pushed to all three, and that was
|
||
copied from there into this section and into `RELEASING.md` before anyone ran
|
||
`git ls-remote`. Run, it showed the tag on `gh` alone. **That commit message is
|
||
superseded on this point and cannot be corrected**, which is why
|
||
`context/design-principles.md` §6 says an argument must not live only in one.
|
||
The irony was local: it sat two paragraphs from *an empty log is a statement
|
||
about retention, not about history* — the same failure, a remote-shaped fact
|
||
asserted rather than asked.
|
||
|
||
**Then the fact changed again, within hours, and that is the better lesson.**
|
||
Three measurements of the same thing in one session: present on all three
|
||
remotes and locally; then on `gh` alone; then **nowhere at all** — `rc3` was
|
||
deleted while this branch was in review. Only the first reading was careless.
|
||
The second and third were each correct when taken, by two different people, and
|
||
they disagree.
|
||
|
||
So the rule this section reaches for is **not** "ask rather than assert". It is
|
||
that **a remote-shaped fact has a shelf life even after you ask**: a verified
|
||
answer decays, and prose cannot distinguish a stale line from a live one, so a
|
||
reader at 2am has no way to tell which they are holding. A document that records
|
||
an inventory is therefore wrong on a schedule it does not control. This section
|
||
and `RELEASING.md` consequently record **how to ask** — `git tag -l`,
|
||
`git ls-remote --tags <remote>` — and deliberately record no inventory of which
|
||
tags exist where. What never decayed is the principle: a wrong tag is left dead
|
||
and the next version is cut. Note which half of the original correction survives
|
||
and which did not: *the commit message was wrong* is durable, because it is a
|
||
fact about a fixed text; *the tag is on `gh` alone* was true and is now false.
|
||
The keeper is the one whose subject cannot change.
|
||
|
||
**The primary check is textual and the `dist plan` run is a second opinion.**
|
||
An absent `dist` must not read as a pass, so the script says which of the two
|
||
ran. It also tolerates a locally-installed dist whose version differs from the
|
||
one pinned in `dist-workspace.toml`: that stops dist before it plans, and
|
||
reporting it as a refusal would train the reader to ignore refusals.
|
||
|
||
**Only the root package's version can disagree with a tag.**
|
||
`dist-workspace.toml` says `members = ["cargo:."]`, so `gp-tree` sitting at
|
||
`0.1.0` while everything else is on `0.1.0-rc2` is not a finding. The script
|
||
lists such crates and says so rather than failing, because a deliberate split
|
||
should stay deliberate and a silent one should be visible.
|
||
|
||
**The test's first version was vacuous, and the fault injection is what showed
|
||
it.** `scripts/release-preflight-test.sh` originally asserted only *did it
|
||
refuse*. Every negative case passed with the tag comparison replaced by
|
||
`if false;` — because the script refuses a **dirty working tree** first, and
|
||
this repo's tree is almost always dirty, bd auto-exporting `.beads/issues.jsonl`
|
||
on nearly every command. Four of five checks were answering *is the tree dirty*
|
||
while claiming to answer *does it catch a bad tag*. The test now runs the
|
||
negative cases against a clean throwaway checkout **and asserts the refusal
|
||
names the version mismatch**, so a refusal for the wrong reason is a failure
|
||
rather than a pass. Two rival faults were injected against the repaired
|
||
version — the comparison disabled, and `expected` derived from the tag itself —
|
||
and each produces four failures and exit 1.
|
||
|
||
That is `context/design-principles.md` §7's "a check that looks like the answer,
|
||
answering a neighbouring question", found in a checker written to prevent
|
||
exactly that class, within an hour of writing it.
|
||
|
||
**And the repair opened a second blind spot in the same place — which is the
|
||
more useful half.** Moving the negative cases to a clean checkout is what made
|
||
the tag comparison reachable at all, and it thereby removed the only
|
||
circumstance in which the test could observe the **dirty-tree guard**.
|
||
Disabling that guard outright left all six checks green and the script exiting
|
||
`0`: the guard whose *precedence* caused the original vacuity had become the one
|
||
thing the suite could not see, and the repair's own shape is what hid it. A
|
||
vacuity and its repair are not two events here; the repair relocated the blind
|
||
spot rather than closing it.
|
||
|
||
The check that closes it is therefore aimed the other way — a tag that
|
||
**matches** the version, run against a dirtied checkout, so the comparison
|
||
passes and a refusal can only be the tree. That is §7's *assert the direction
|
||
the default cannot supply* applied to a **fixture** rather than to a default:
|
||
the clean checkout is the fixture, and the discriminating input is the one that
|
||
makes it dirty. The two faults are independent in both directions — with the
|
||
comparison disabled the dirty-tree check still passes, and with the guard
|
||
disabled the three tag checks still pass — which is what makes the seventh
|
||
check worth having rather than a restatement of the other six.
|
||
|
||
**What is still unchecked, and why that is a decision.** The `dist plan` second
|
||
opinion can be disabled — its refusal arm rewritten never to match — with the
|
||
suite green, because every input the test feeds it is one the textual check has
|
||
already refused. That follows from the arm's own design rather than being an
|
||
oversight: it runs *after* the primary check, it is allowed to be absent
|
||
entirely, and an absent `dist` already prints that only the textual check ran.
|
||
Pinning it would mean installing a `dist` matching `cargo-dist-version`, which
|
||
is the very mismatch the script tolerates on purpose. Stated here rather than
|
||
guarded, so the next reader does not mistake the silence for coverage.
|
||
|
||
**Where it runs.** The `kb` job, beside `scripts/pr-watch-test.sh` — this
|
||
repo keeps its check_the_checkers there, not in `test`. The step needs no
|
||
trealla and no nix of its own; it is `sh`, `git archive` and `awk`.
|
||
|
||
---
|
||
|
||
## 5. One shell, one environment: `scripts/checkers.sh` (improvise-f9qf)
|
||
|
||
`scripts/pr-reviews-test.sh` passed **19/19 locally and 2/19 in CI on the same
|
||
commit**, because a multi-assignment `local` does not evaluate left to right on
|
||
bash 5 while macOS's bash 3.2 tolerates it. Two real bugs were invisible to the
|
||
machine that wrote them. This section is what that cost to close, and the half
|
||
of it that is not obvious is worth more than the half that is.
|
||
|
||
### The obvious half: two platform splits, neither detected
|
||
|
||
At `22ab58f` there were sixteen shell scripts in `scripts/`: eleven
|
||
`#!/usr/bin/env bash`, four `#!/bin/sh`, one `#!/usr/bin/env sh`. That is *two*
|
||
splits, not one. `bash` is 3.2 on macOS and 5.2 on the CI runner; `sh` is
|
||
bash-in-sh-mode on macOS and **dash** on the runner. Neither split had anything
|
||
watching it, and the second had never been noticed at all.
|
||
|
||
Both are now `#!/usr/bin/env bash`, and `checkers.sh` refuses any other shell
|
||
shebang in `scripts/`. Converting the `sh` scripts cost nothing — they were
|
||
already a subset — and the point is not that dash is bad, it is that **a script
|
||
whose interpreter depends on the machine is a script nothing has tested**.
|
||
|
||
### The half that decides the design: direnv gives the right bash and the wrong environment
|
||
|
||
The cheap fix looks like "run scripts through direnv", and `.envrc` does put a
|
||
nix bash **5.3.15** on `PATH`, which really is CI's semantics. It is not enough,
|
||
and the reason generalises far past this ticket.
|
||
|
||
`pkgs.mkShell` exports a pile of generically-named lowercase variables that CI
|
||
does not have. Measured inside this repo's dev shell: `name`, `out`, `outputs`,
|
||
`system`, `shell`, `builder`, `stdenv`, `phases`, `patches`, `buildInputs`,
|
||
`nativeBuildInputs`, `doCheck`, `strictDeps` and about twenty more. A script
|
||
under `set -u` that reads one of them by accident is **silent inside the dev
|
||
shell and fatal in CI**.
|
||
|
||
That is exactly what happened. `fixture_file`'s broken third clause read `$name`
|
||
— and inside the dev shell `$name` was `nix-shell-env`, so `set -u` never fired,
|
||
the `[ -f "$f" ] ||` fallback re-evaluated `$name` correctly on its own line,
|
||
and the suite passed. Reproduced at `051f541^`, on one machine, in one sitting:
|
||
|
||
| how the suite was run | result |
|
||
|---|---|
|
||
| macOS `/bin/bash` 3.2 | 19 passed, 0 failed |
|
||
| `direnv exec . ./scripts/pr-reviews-test.sh` (nix bash 5.3) | 19 passed, 0 failed |
|
||
| the same, with `name` unset | **2 passed, 17 failed** |
|
||
|
||
The middle row is the finding. Two different reasons for green, one of them the
|
||
*correct interpreter*, and the whole point of reaching for direnv was that it
|
||
would not be green. **A dev shell is not a reproduction of CI; it is a third
|
||
environment**, and it is the one that hides `set -u` faults.
|
||
|
||
### The decision, and the two options that were weighed with numbers
|
||
|
||
A **nix shebang** was the ticket's first option and the user's own suggestion.
|
||
Measured warm on the orchestrator's machine, 20 invocations each:
|
||
|
||
| | ms per invocation |
|
||
|---|---|
|
||
| ambient `bash -c true` | 5.1 |
|
||
| `nix shell nixpkgs#bash --command true` | 61.8 |
|
||
| `nix-shell -p bash --run true` (the manpage shebang form) | 675.2 |
|
||
|
||
61.8 ms is affordable; 675 ms is 132× the ambient cost, and
|
||
`scripts/beads-merge-driver.sh` runs inside git's merge machinery. But the cost
|
||
that decides it is not startup: **only the `kb` job installs nix.** `build`,
|
||
`test`, `wasm`, `js`, `harness-lint`, `browser` and `pages-deploy` have none, so
|
||
a nix shebang on `assemble-stamp-test.sh` or `node-test.sh` would need an
|
||
installer step added to three more jobs — paid on every run, to reach a bash
|
||
those runners already have. Declined.
|
||
|
||
**zsh** was the other option. It is not a shebang change: word splitting differs,
|
||
arrays index from 1, globbing differs. Porting sixteen scripts means reading
|
||
sixteen scripts, and the review cost is the whole diff rather than the shebang
|
||
line. The runner also does not ship zsh, so every job would need an `apt-get`.
|
||
Declined.
|
||
|
||
What was adopted instead fixes the *environment* a suite runs in, in
|
||
one place, with no per-invocation nix cost:
|
||
|
||
**`scripts/checkers.sh` is the one entry point for every check_the_checker.** CI
|
||
calls it and the local gate calls it, so there is no second invocation to drift.
|
||
It runs every suite under the bash that runs it: in CI and under direnv, that
|
||
is the dev shell's bash. It does two things a bare `./scripts/X-test.sh` does
|
||
not:
|
||
|
||
1. **Scrubs the environment to an allowlist** (`env -i` plus `PATH`, `HOME`,
|
||
`TMPDIR`, the `NIX_*` and `GITHUB_*` names, extensible through
|
||
`CHECKERS_KEEP`). This is the part that closes the direnv hole above, and it
|
||
is a *positive* list on purpose: the failure mode of a missing name is a loud
|
||
"command not found", where the failure mode of an extra name is the silence
|
||
this section is about.
|
||
2. **Enforces the shebang rule**, so the `sh`/`bash` split cannot come back.
|
||
|
||
**The runner no longer polices the bash version or where tools live.** It used
|
||
to pin a bash of at least 5.1 (`find_modern_bash`: try the ambient bash, fall
|
||
back to `nix shell nixpkgs#bash`, and exit 3 if neither was available). The
|
||
floor was 5.1 because `pr-reviews-test.sh` uses `wait -n -p` (improvise-fd0j).
|
||
All of that was removed on the user's ruling, when CI moved to the NixOS
|
||
runners: "we shouldn't care which version of bash and other standard utilities
|
||
are running in CI", and "most jobs should just take what comes in the ci dev
|
||
shell".
|
||
|
||
So every CI step runs inside `nix develop .#ci`, which provides the bash and the
|
||
GNU coreutils. The runner calls suites with `$BASH` and an unchanged `PATH`. A
|
||
script that needs a newer bash feature fails where it uses it, not at a version
|
||
gate.
|
||
|
||
Two things went with the floor:
|
||
|
||
- **Its self-tests.** `checkers-test.sh` sections 2, 5 and 5b faked old
|
||
bashes, or stripped `PATH` down to hand-picked utilities, to prove the floor
|
||
held. Those were the tests that broke on the NixOS runners, where `/usr/bin`
|
||
and `/bin` hold only `env` and `sh` (#296, improvise-3uow).
|
||
- **The hard-coded tool paths** in the suites (`/bin/sleep` and `/bin/date` in
|
||
`pr-reviews-test.sh`) and `scripts/kb`'s BSD `stat -f` fallback. `scripts/kb`
|
||
already needs the dev shell for `tpl`, so GNU `stat` is always present when it
|
||
runs.
|
||
|
||
**A suite is `X-test.sh` where `X` or `X.sh` is present beside it.** That rule
|
||
exists because `scripts/node-test.sh` matches `*-test.sh` and is a test
|
||
*runner*, not a suite — the first version of the glob ran it with no arguments,
|
||
which printed its usage inside an otherwise green gate. The naming convention
|
||
("a check_the_checker is named after the thing it checks") was already true; it
|
||
is now load-bearing.
|
||
|
||
### What the old local gate actually was
|
||
|
||
The gate in circulation was `for t in scripts/*-test.sh; do sh "$t"; done`. That
|
||
loop **overrides every shebang with `/bin/sh`**, so on macOS it ran every suite
|
||
under bash 3.2 in sh-mode no matter what the file said — the worst of the
|
||
available shells, chosen by the gate rather than by any script. It is replaced
|
||
by `./scripts/checkers.sh`.
|
||
|
||
### What is still not covered
|
||
|
||
`web/*.sh` is outside `scripts/` and outside the shebang check; `web/build.sh`
|
||
and `web/assemble.sh` are still whatever their own first lines say. The
|
||
allowlist is a list, so a suite that comes to need a new variable fails loudly
|
||
and someone edits it — deliberate, but it is a maintenance edge. And the
|
||
scrubbing protects the *suites*; a production script run by hand inside the dev
|
||
shell still inherits `name` and friends. Closing that would mean a guard in
|
||
every script, which `scripts/beads-merge-driver.sh` — running inside git's merge
|
||
machinery, possibly outside direnv — is a good reason not to write.
|
||
|
||
### The seventh site is not a shell file
|
||
|
||
Seven places ran a repo script through an explicit `sh`, overriding the callee's
|
||
own shebang — five in `scripts/`, one in `web/assemble.sh`, and one in
|
||
`dist-workspace.toml`:
|
||
|
||
```toml
|
||
build = ["sh", "scripts/build-web-release.sh"]
|
||
```
|
||
|
||
All seven now invoke the script directly. The TOML one is the interesting one,
|
||
and it is why the shebang rule alone is not the whole answer: **`checkers.sh`
|
||
scans first lines, so it cannot see an interpreter named in a build
|
||
configuration.** A script may declare `#!/usr/bin/env bash`, pass the gate, and
|
||
still be handed to dash by whatever invokes it. The only defence there is
|
||
reading, which is how this one was found — by grepping for the *call* after
|
||
fixing the *declaration*, rather than stopping at the gate going green.
|