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
1142 lines
63 KiB
Markdown
1142 lines
63 KiB
Markdown
# Test evidence — the repository's own gates
|
||
|
||
Evidence for the checks in `scripts/` and for the tests that hold CI's shape.
|
||
The arguments behind the checkers themselves are in `context/code/tooling.md`;
|
||
this file records what fault each test was watched to catch.
|
||
|
||
Note what is deliberately absent. `scripts/check-comment-runs.py --self-test`,
|
||
`scripts/kb-test.sh` and `scripts/assemble-stamp-test.sh` are check-the-checker
|
||
suites whose cases are already injected faults, and none of their case names is
|
||
a Rust `fn` or a JS `test(` the evidence checker could resolve — so their
|
||
evidence is the case list itself plus `context/code/tooling.md`, and inventing
|
||
headings for them here would be headings naming nothing.
|
||
|
||
### "no test in web/tests/ reaches for a built bundle"
|
||
|
||
The `js` CI job runs `node --test web/tests/*.test.js` and installs nothing; the
|
||
`browser` job runs `web/tests/bundle/*.test.js` after three wasm-pack builds.
|
||
The moment a flat `web/tests/` file reads `web/pkg/`, the fast job inherits the
|
||
slow one's dependency — and the likely repair is to add a build step to it,
|
||
silently spending the ~4m30s the split exists to avoid. Until improvise-4ktt
|
||
that constraint was a directory convention and a glob, checked by nothing, and
|
||
it failed in the direction that looks like ordinary breakage: a confusing ENOENT
|
||
in CI on a machine where the bundle happens not to be built.
|
||
|
||
Fault injected, 2026-09-21: appended
|
||
`const injected = readFileSync("../pkg/improvise_wasm_client.js");` to
|
||
`web/tests/grid_plan.test.js`. The test failed naming that file and pointing at
|
||
`web/tests/bundle/`. Reverted; green again.
|
||
|
||
Two things the test does that a simpler one would not. It asserts its own
|
||
filename is among the files it scans, so it cannot become the one place a
|
||
violation may hide. And it strips comments before matching, because this file's
|
||
own header and the test's header both name the directory in prose — a lexical
|
||
grep over raw text would have had to exempt them, and an exemption is where the
|
||
next violation goes.
|
||
|
||
**What it cannot see**, stated because the check itself uses the evasion: a path
|
||
assembled at runtime (`"../" + "p" + "kg"`) is invisible to a lexical match, and
|
||
the test's own `FORBIDDEN` constant is built exactly that way to stay out of its
|
||
own scan. That is a deliberate, disclosed hole — closing it needs a parser, and
|
||
the failure it would catch is one somebody has to write on purpose, where this
|
||
gate exists for the one written by accident.
|
||
|
||
### "the subdirectory that MAY use a bundle is not the one being checked"
|
||
|
||
The companion, and the one that stops the first from passing vacuously. If
|
||
`web/tests/bundle/` were emptied or its files stopped touching the bundle, the
|
||
first test would still be green while having nothing to be scoped *against* —
|
||
"no flat test uses the bundle" is uninteresting if no test anywhere does. This
|
||
asserts the other half of the split exists and really does what the flat half
|
||
is forbidden to do.
|
||
|
||
Fault injected, 2026-09-21: moved `web/tests/bundle/wasm_surface.test.js` out of
|
||
the tree. The test failed with "web/tests/bundle/ is empty — the split lost its
|
||
other half" rather than passing. Restored; green again.
|
||
|
||
### "no test in web/tests/ imports a module that reaches the bundle"
|
||
|
||
The companion to the lexical check above, and the reason that one is not
|
||
enough: `import "../app.js"` needs the bundle *transitively* and names nothing
|
||
forbidden, so the `js` job breaks loudly but unattributed. `app.js` and
|
||
`worker.js` are the two entry points that load the bundle; `shell.js` and
|
||
`serial.js` do not, and `serial.test.js` imports one legitimately.
|
||
|
||
Fault injected, 2026-09-21: appended `import "../app.js";` to
|
||
`web/tests/grid_plan.test.js`. The lexical test stayed green; this one failed
|
||
naming the file. Reverted.
|
||
|
||
### Note — the checker's own blockers, found by review of PR #83
|
||
|
||
Two gates in `scripts/check-comment-runs.py` produced a pass while not doing
|
||
what their prose said. Both were in the mode the same PR rewrote, which is the
|
||
part worth keeping: writing the doctrine down does not exempt the writer.
|
||
|
||
**A leaf name is not an identity.** Six leaf names collide in this workspace and
|
||
two were cited in evidence. Renaming
|
||
`ui::project::tests::a_load_that_fails_is_not_a_rebase` — the one its entry
|
||
describes — left the checker printing *all name a test that runs*, exit 0, with
|
||
the heading answered by the `ui::host::tests` namesake. Now an ambiguous leaf is
|
||
refused with both candidates named, and the two entries are qualified.
|
||
Re-verified both ways, 2026-09-21: the refusal fires on the unqualified
|
||
headings and names exactly those two; with the heading qualified, the same
|
||
rename is reported as an orphan rather than absorbed.
|
||
|
||
**`--rust-tests` was strictly more permissive and said the same words.** It
|
||
called `cargo_test_names` with one argument, so the `--ignored` subtraction
|
||
never ran, and the success line was byte-identical to the honest path's.
|
||
Injecting `### eyeball_the_rendered_hints_and_pages` orphaned on the default
|
||
path and passed on the flag's. Now the flag requires
|
||
`--rust-tests-ignored` (one alone exits 2) and the success line names its
|
||
source.
|
||
|
||
**A missing evidence directory exited 0.** The early return preceded the
|
||
citation scan, so deleting `context/test-evidence/` reported nothing while 170
|
||
citations pointed at a directory that was not there. Verified 2026-09-21 by
|
||
moving the directory aside: exit 1, *170 tests cite context/test-evidence and
|
||
have no entry there*.
|
||
|
||
### Note — the flag that selected the cap away (improvise-ijql)
|
||
|
||
`scripts/check-comment-runs.py --check-evidence` early-returned in `main`, so
|
||
the flagged run never answered the comment-run cap and printed nothing about
|
||
having skipped it. The reviewer's position when it was first raised was that CI
|
||
runs all three spellings (`ci.yml`, the three `check-comment-runs.py` steps in
|
||
the `test` job) so this was documentation only. The counter that carried is that
|
||
the flagged form is the **expensive** one — it needs a compiled workspace — which
|
||
makes it the one a human runs alone; three agents hit it independently in one
|
||
campaign, and in one case a local gate went green while CI went red on a
|
||
three-line doc comment.
|
||
|
||
Reproduced before the fix, 2026-09-21, in a temporary tree holding one `.rs`
|
||
file with a four-line comment run:
|
||
|
||
```
|
||
$ check-comment-runs.py src --check-evidence --rust-tests … --rust-tests-ignored …
|
||
test-evidence: 0 entries, all name a test that runs …
|
||
EXIT=0
|
||
$ check-comment-runs.py src
|
||
1 comment runs longer than 2 lines:
|
||
src/bad.rs:1 4 lines
|
||
EXIT=1
|
||
```
|
||
|
||
Two invocations over one tree, opposite verdicts, and the one that exits 0 is
|
||
the one a human is more likely to run by itself.
|
||
|
||
The guard is `_dispatch_self_test`, which runs the script as a **subprocess** —
|
||
the only level at which this is visible, since every unit in the file was
|
||
correct and the defect was in `main`'s dispatch. Both arms were demonstrated:
|
||
|
||
| injected fault | what failed |
|
||
|---|---|
|
||
| restore the `--check-evidence` early return | `DISPATCH: --check-evidence: exited 0 over a four-line comment run` |
|
||
| make the cap always report a violation | `DISPATCH: bare: refused a clean tree` *and* the same for `--check-evidence` |
|
||
|
||
The second arm is what stops an always-failing cap from satisfying the first,
|
||
and it is why `SELF_TEST_DISPATCH` asserts a clean tree passes as well as a
|
||
dirty one failing. Why the fix is additive rather than a printed disclaimer, and
|
||
why `--self-test`/`--list-doctests` are excluded from the table rather than
|
||
exempted from the rule: `context/code/tooling.md` §2, "A flag adds a question".
|
||
|
||
### Note — the stamp gate's discrimination, measured
|
||
|
||
`scripts/assemble-stamp-test.sh` separates three outcomes rather than two, which
|
||
is what makes it evidence rather than a smoke test. Measured 2026-09-21 over its
|
||
nine cases: the real script passes 9; deleting the final `test -z "$missing"`
|
||
(the PR #69 injection, which the inline version survived at exit 0) passes 4 and
|
||
fails 5; a script that does nothing passes 2; one that always fails passes 0.
|
||
|
||
The PR body first cited "4 of 9" for the middle row, from a run taken when the
|
||
suite had eight cases. A figure is what the next reader checks, so it is
|
||
recorded here where it can be re-measured rather than only in a message nothing
|
||
can audit (design-principles §6).
|
||
|
||
### Note — the stamp gate matches literally
|
||
|
||
`scripts/assemble-stamp.sh` greps with `-F`. As a regex, the `.` in
|
||
`improvise_wasm_client.js?v=abc` matches any character, so a stamp corrupted in
|
||
exactly that position reads as present — a pass in the one direction a guard
|
||
must not fail in. Inherited from the inline version this was extracted from,
|
||
and found by reading the extraction rather than by anything failing.
|
||
|
||
Fault injected, 2026-09-21: dropping the `-F` makes
|
||
`improvise_wasm_clientXjs?v=abc` satisfy the pattern, and
|
||
`a pattern is matched literally, not as a regex` goes from pass to fail.
|
||
|
||
### Note — the doc site's derived checks
|
||
|
||
`scripts/build-doc-site.mjs` carries two gates that no test names, because the
|
||
generator *is* the test: it runs on every assembly and exits non-zero.
|
||
|
||
Faults injected, 2026-09-21, each observed:
|
||
|
||
| injected | reported |
|
||
|---|---|
|
||
| a new `context/code/newarea.md` | `context/code/newarea.md is neither published nor a named omission` |
|
||
| `context/plan.md` moved out of the tree | `context/plan.md is listed as omitted but does not exist` |
|
||
| a line appended below the generator in `web/assemble.sh` | `web/assemble.sh must invoke this generator LAST`, naming the offending command |
|
||
| a trailing `echo "see scripts/build-doc-site.mjs"` | the same, once the check asks for an INVOCATION rather than a mention — `.includes` had accepted it (review, PR #83) |
|
||
|
||
`scripts/check-ci-graph.py` is the third, and it is a script rather than a
|
||
stanza because its own refusals need checking: adding a `zz-new-check:` job to
|
||
`ci.yml` reports *pages-deploy does not need `zz-new-check`* and exits 1, where
|
||
before the rule was prose and nothing noticed. Its five self-test graphs include
|
||
the one a naive parse gets wrong — another job's `needs:` read as the deploy's.
|
||
|
||
The second arm is the one that would not get written first, and it is the point:
|
||
without it, deleting or renaming an omitted file leaves an entry asserting a
|
||
reason for a file nobody can find — the same failure as a dead ticket id, which
|
||
`AGENTS.md` already calls worse than no id.
|
||
|
||
The first arm was filed as latent and was live when it was written:
|
||
`context/code/kb-tool.md` had been unpublished, unargued and unnoticed since it
|
||
was added.
|
||
|
||
|
||
### Duplicate evidence headings — `SELF_TEST_DUPLICATES`
|
||
|
||
improvise-hsoe. `--check-evidence` verified that a heading names a test that
|
||
**runs**, and a duplicate heading names one just as well, so both copies pass.
|
||
PR #92 shipped two entries under
|
||
`a_layout_change_moves_the_layout_version_and_no_record`, and two more pairs
|
||
sat in `tui.md` — one of them asserting that `THE_MEMBERSHIP_CORRECT_SPANS` was
|
||
"a live tripwire, do not remove it" some four hundred lines above the entry
|
||
recording that improvise-gd83 had removed it. A reader meeting the older copy
|
||
first takes it as current, which is worse than an orphan: an orphan announces
|
||
itself as broken.
|
||
|
||
The fault is injected by the six cases in `SELF_TEST_DUPLICATES`, and three of
|
||
them are the ones that would not get written first. Two *different* tests
|
||
sharing a leaf must come back **ambiguous and not duplicate**, because the
|
||
wrong diagnosis files the wrong repair. A prose heading must be free to repeat,
|
||
because it claims no test. And the bare and module-qualified spellings of one
|
||
test must collide — grouping by the string rather than by the resolved test
|
||
would have missed exactly the case improvise-rm4s's qualification repair
|
||
creates, which is the first way this would have come back.
|
||
|
||
The live tree was itself the first injection: before the three merges the check
|
||
exited 1 naming all three pairs with both line numbers; after them it exits 0.
|
||
The clean arms (two headings naming two tests, and prose repeated) are what
|
||
stop an always-refuse from satisfying the rest.
|
||
|
||
### The PR review reader's own suite — `scripts/pr-reviews-test.sh`
|
||
|
||
improvise-xgra. Like `kb-test.sh` and `pr-watch-test.sh`, its cases are already
|
||
injected faults and none of their names is a Rust `fn` the evidence checker
|
||
could resolve, so the case list is the evidence. What is recorded here is the
|
||
five faults it was watched to fail against, and which case caught each — a
|
||
suite that only asked "did it print something" passes all five.
|
||
|
||
Fault 1, **the inline surface dropped entirely**, which is the live bug as it
|
||
shipped: `inline: []` in both fetch paths. Eight cases fail, including both
|
||
`--wait` arms. This is the one that matters, because the old script's behaviour
|
||
*was* this fault and it exited 0 on PR #122's review body while printing none
|
||
of the five rulings underneath it.
|
||
|
||
Fault 2, **an inline comment counted rather than compared**:
|
||
`((.inline|length) > 0)` in place of the `commit_id == head` test. One case
|
||
fails, `an inline comment at an EARLIER head does not satisfy --wait`, and that
|
||
is the pair the two inline fixtures exist to discriminate — the at-head case
|
||
passes under this fault, so on its own it proves nothing.
|
||
|
||
Fault 3, **the time sort dropped** (`| .` for `| sort_by(.at)`). Only
|
||
`the three surfaces interleave in time order` fails: it reads the three bodies
|
||
back as `SECOND THIRD FIRST`, the per-endpoint concatenation order. Every
|
||
content assertion still passes, because each body is still printed.
|
||
|
||
Fault 4, **no `original_line` fallback**. Only the outdated-comment case fails.
|
||
`line` is null exactly when the line has moved under the comment, so a renderer
|
||
reading `.line` alone prints `draw.rs:null` on precisely the comments whose
|
||
location is hardest to recover by hand.
|
||
|
||
Fault 5, **`--wait` widened to any issue-level comment**. Only
|
||
`an issue-level comment does not satisfy --wait` fails. That is the fault a
|
||
well-meaning repair would introduce — "wait for anyone to say anything" — and
|
||
the fixture that separates it is a PR thread comment with no `commit_id` at
|
||
all.
|
||
|
||
Two cases are there for the contract rather than the bug:
|
||
`a review body at the current head still satisfies --wait` and
|
||
`--wait 0 over an empty PR is success`, because other briefs depend on exit 0
|
||
and exit 1 meaning what they did before. `no PR number is exit 4` pins the
|
||
third code, which is distinguishable from 1 only if nothing collapses them.
|
||
|
||
`the usage warns about a trailing $? reader` asserts a sentence rather than a
|
||
behaviour, which is unusual and deliberate: the `; echo "exit=$?"` trap costs a
|
||
wrong diagnosis every time it is hit, the warning is the only guard against it,
|
||
and a header is exactly the thing a later edit trims without noticing.
|
||
|
||
Three more cases were added after ChatGPT Codex reviewed PR #133, and the pair
|
||
of bugs it found is the reason the suite has to run in CI rather than only on
|
||
the author's machine. Both produce **exit 2 with no output whatsoever**, which
|
||
is jq refusing an invalid `--argjson` — so a shell fault at the top of the
|
||
script arrives wearing jq's exit code, and 17 of 19 cases failed identically
|
||
with nothing to read.
|
||
|
||
The first, a multi-assignment `local` whose third clause reads its first, is
|
||
fine on macOS's bash 3.2 and fatal on bash 5 under `set -u`. **The suite passed
|
||
19/19 locally and 2/19 in CI on the same commit**, which is the case for wiring
|
||
it into the `kb` job in the same change rather than afterwards.
|
||
|
||
The second is `gh api --paginate` emitting one array per page. Fault 6 removes
|
||
`| pages_to_array` from the fixture path and exactly the three new cases fail —
|
||
`a comment on the first page survives pagination`, `…the second page…`, and
|
||
`a paginated thread still satisfies --wait` (exit 2, the live signature). The
|
||
fixture carries two concatenated arrays and both paths flatten through the same
|
||
helper; a fixture holding one array could not have caught this, because it
|
||
would exercise a shape the live fetch never produces. No PR in this repo has
|
||
yet had enough inline comments to reach a second page, so the test is the only
|
||
thing that will ever see it before a user does.
|
||
|
||
### The checker runner's own suite — `scripts/checkers-test.sh`
|
||
|
||
The bug behind improvise-f9qf was **invisible to a local run**, so the evidence
|
||
that matters here is not "the suite is green" but "the suite goes red when the
|
||
fault is put back, in the place the fault was not seen."
|
||
|
||
**The fault, injected against the real script.** Restoring `pr-reviews`'
|
||
multi-assignment `local` — the exact text `051f541` replaced — and running the
|
||
suite three ways on one machine in one sitting:
|
||
|
||
| gate | result |
|
||
|---|---|
|
||
| `sh scripts/pr-reviews-test.sh` (the old local gate) | 22 passed, 0 failed |
|
||
| `direnv exec . ./scripts/pr-reviews-test.sh` | 22 passed, 0 failed |
|
||
| `./scripts/checkers.sh pr-reviews` | **2 passed, 20 failed**, exit 1 |
|
||
|
||
The middle row is the one that earns the design. The dev shell supplies the
|
||
*correct* bash and would still have shipped this, because `pkgs.mkShell` exports
|
||
`name` and the broken clause read `$name`. Only scrubbing the environment as
|
||
well as pinning the shell makes the fault visible, and the 2/20 shape matches
|
||
the 2/19 CI actually reported.
|
||
|
||
**`checkers-test.sh` itself is twelve cases, and each injects rather than
|
||
asserts.** The two that carry the argument:
|
||
|
||
- *a suite reading an inherited dev-shell variable fails under the runner* — a
|
||
fixture suite whose body is `echo "$name"` under `set -euo pipefail`, run with
|
||
`name=nix-shell-env` exported, exactly as direnv leaves it. It is paired with
|
||
*and passes when run directly with that variable exported*, which asserts the
|
||
**same fixture passes the old way**. Without that pair the first case is a
|
||
test of `set -u` and proves nothing about the runner.
|
||
- *a bash-3.2-only construct fails under the runner's pinned bash* — a fixture
|
||
suite containing `local a="$1" b="$a/x"`, which is the bug's shape reduced to
|
||
two lines. Green on bash 3.2, fatal on bash 5.
|
||
|
||
The rest separate the runner's outcomes from each other: a `#!/bin/sh` script in
|
||
the directory is refused **and the same sandbox passes once it is removed** (so
|
||
the refusal is not standing in for some other complaint); a failing suite fails
|
||
and a passing one passes; an old bash with no nix is exit 3 rather than a quiet
|
||
run; an unknown suite name is exit 4 rather than a silent no-op; and `--list`
|
||
names exactly the suites present, including the case where an `X-test.sh` has no
|
||
`X` beside it — `scripts/node-test.sh` is a test *runner*, and the first version
|
||
of the glob ran it as a suite, where it printed its usage inside a green gate.
|
||
|
||
**What is asserted about the runner's own environment, and what is not.** The
|
||
allowlist is checked only in the sense that seven real suites pass through it;
|
||
there is no case asserting a *particular* name is absent beyond `name`. A suite
|
||
that grows a dependency on some other inherited variable will fail loudly on
|
||
first run rather than be caught here. Stated rather than guarded.
|
||
|
||
### The two-workspace fmt gate — `scripts/fmt-workspaces-test.sh`
|
||
|
||
The bug (improvise-b4ks) was a **gate decoupled from what it checks while
|
||
wearing the exit code of a real failure**. From the repo root,
|
||
`cargo fmt -p opfs-contract -- --check` exits **1** with `package
|
||
'opfs-contract' is not a member of the workspace` — `--check` never runs. A
|
||
caller reading `$?` cannot tell "this crate is misformatted" from "nothing was
|
||
checked", so the gate could neither report success when the crate was dirty nor
|
||
failure when it was clean. Reproduced at `7cef9f8`, exit 1, before any fix.
|
||
|
||
`scripts/fmt-workspaces.sh` replaces it: it discovers every `[workspace]` root
|
||
and checks each with `-p <member>` scoped by that root's own `--manifest-path`.
|
||
Exit **2** is a usage error or a cargo that could not run; **1** is a formatting
|
||
verdict; they are never the same number.
|
||
|
||
**Why `-p` and not the two obvious spellings** — both were measured, and both
|
||
fail in a direction that looks like success:
|
||
|
||
- `--manifest-path <root> --all` **crosses the workspace boundary**. `--all`
|
||
formats "all packages, and also their local path-based dependencies", and
|
||
`harness/opfs-contract` depends on `improvise-io` by path, so the inner root
|
||
reports the *outer* tree's files. Measured: an injected fault in `src/draw.rs`
|
||
was reported by the harness root's own invocation.
|
||
- `--manifest-path <root>` bare is **blind to the members**. The repo root is a
|
||
package (`improvise`) as well as a workspace, so the bare form checks only
|
||
that package. Measured: an injected fault in `crates/improvise-core/src/format.rs`
|
||
exited **0**.
|
||
|
||
**Four rivals were injected into the script and each failed the suite**:
|
||
`--all` for `-p` (the boundary leak, caught by counting — the outer file must be
|
||
named exactly once); the bare manifest-path (caught by the outer-fault case);
|
||
`return 2` collapsed to `return 1` on the usage arms (two cases); and "no
|
||
workspace root found" returning 0 instead of 2.
|
||
|
||
**The fixture had to be made faithful twice, and each vacuity was found only by
|
||
injecting the rival — not by reading.** The suite passed under a rival at both
|
||
stages, which is the only reason the gaps were visible:
|
||
|
||
- The first fixture's nested crate had **no path dependency** into the outer
|
||
tree, so `--all` had nothing to leak through and the boundary rival passed.
|
||
- The first fixture's outer root was a **virtual manifest** with no `[package]`,
|
||
where the bare form happens to reach every member, so that rival passed too.
|
||
|
||
Both are properties this repo really has, and a fixture that omits them tests a
|
||
repository that does not exist. That is the generalisable point: **a
|
||
check_the_checker's fixture must reproduce the structural features the hazard
|
||
depends on, and the only way to know it does is to inject the rival and watch
|
||
it fail.** A green suite over a simplified fixture is the same non-evidence as a
|
||
green suite over a vacuous assertion.
|
||
|
||
**What is not covered.** The script asks `cargo metadata` for members, so a root
|
||
whose metadata succeeds but whose member list is wrong is not detectable here.
|
||
*Narrowed by improvise-90ea, below:* a list that **omits** a package in the tree
|
||
is now refused; one that names a package it should not is still not detected.
|
||
And `harness/opfs-contract`'s own `cargo fmt -p opfs-contract` step in the
|
||
`harness-lint` job is left as it is: it resolves correctly because that step
|
||
sets `working-directory`, and the ticket's own note confirms CI as written was
|
||
never the broken caller.
|
||
|
||
#### The exit contract, narrowed after review — `scripts/fmt-workspaces-test.sh`
|
||
|
||
ChatGPT Codex on PR #159 found that the contract above promised more than the
|
||
script maintained: both formatter invocations end `|| rc=1`, so **any** failure
|
||
after the `cargo fmt --version` probe was reported as "misformatted" even
|
||
though no verdict had been produced. The finding was correct and was measured
|
||
rather than assumed:
|
||
|
||
- an **unparseable source** (`pub fn a( {{{`) exits **1** — identical to a diff;
|
||
- an **unknown `rustfmt.toml` option** is only a warning, and exits **0**.
|
||
|
||
Of the two remedies offered, the contract was **narrowed** rather than the
|
||
failure re-classified, because parsing rustfmt's prose to tell a diff from a
|
||
parse error is precisely the check-adjacent-to-its-subject this repo keeps
|
||
getting wrong. The line the split actually needs is **"did the gate run"**:
|
||
|
||
- exit 1 — rustfmt did not certify a root clean. A diff or a refusal to parse;
|
||
either way **a file is named** and the tree is what is wrong.
|
||
- exit 2 — the gate did not run. Usage, no cargo, no rustfmt, `cargo metadata`
|
||
failed, no `[workspace]` root. **No source file is named.** Every such cause
|
||
is checked *above* the loop, which is what makes the narrowing true rather
|
||
than hopeful. (Since improvise-90ea, a package no root reaches is also exit 2,
|
||
and it names that package's `Cargo.toml`, which is the reason no verdict
|
||
exists. The line is "no verdict", not "no output"; improvise-ximf.)
|
||
|
||
Both directions are now pinned, and two further rivals were injected:
|
||
|
||
- deleting the `cargo fmt --version` probe;
|
||
- turning `|| rc=1` into `|| return 2`, which collapses a verdict into the
|
||
usage code — the mirror of the original bug, and caught by three cases.
|
||
|
||
**The first of those exposed a vacuous test, again only by injection.** The
|
||
original stub was a `cargo` that failed for every argument — but such a cargo
|
||
also breaks `cargo metadata`, so the run exited 2 on the *metadata* arm and the
|
||
probe could be deleted with the suite still green. A missing rustfmt component
|
||
is `cargo metadata` **succeeding** while only `cargo fmt` fails, so the stub now
|
||
delegates every other subcommand to the real cargo and fails `fmt` alone. Both
|
||
stubs are kept, because they exercise different arms.
|
||
|
||
That is the third fixture correction on this one checker, all three found the
|
||
same way: the suite passed *under a rival*. The generalisable rule is that a
|
||
stub must fail in the **shape** of the real fault, not merely fail — a stub that
|
||
breaks more than the real thing breaks satisfies the test through the wrong arm,
|
||
and reads exactly like coverage.
|
||
|
||
#### The root package, and why the fourth fix is structural — `scripts/fmt-workspaces-test.sh`
|
||
|
||
A post-merge review of PR #159 found a **fourth** fixture vacuity
|
||
(improvise-90ea). The fixture's outer root is a package (`root-pkg`) as well as
|
||
a workspace, which is right, but no case ever dirtied **the root package's own
|
||
`src/lib.rs`**. Only the member `a/` and `nested/` were dirtied. So a script
|
||
that formats every member *except* the root package passed 17/17. In this repo
|
||
the root package is `improvise`, the whole TUI. Reproduced at `40b8913` with
|
||
the review's rival in `members_of` (drop the package whose manifest sits at
|
||
`workspace_root`): the suite passed 17/17 and `--list` left `improvise` out.
|
||
|
||
Adding one more hand-picked case would have fixed the fourth and not the fifth.
|
||
Four vacuities in one checker all have the same cause: **the cases were chosen
|
||
by hand, one per hazard somebody had thought of.** So the fix is structural,
|
||
and it has two halves because it has to cover two sets of packages:
|
||
|
||
- **Every package the fixture has.** The two hand-picked fault cases are
|
||
replaced by a loop over every `[package]` manifest in the fixture. Each one
|
||
gets its own misformatted `src/lib.rs`, must exit 1, and must be named
|
||
**exactly once**, so the `--all` boundary check now covers every package
|
||
rather than one. The enumeration is a `grep` for `[package]`, deliberately
|
||
*not* the script's `[workspace]` + `cargo metadata` discovery, since that
|
||
discovery is what is under test. A count assertion (three packages) stops
|
||
the loop going vacuous if the fixture loses a package. A package added to
|
||
the fixture gets its case automatically.
|
||
- **Every package the tree has.** The loop proves only that the script handles
|
||
the fixture's *shapes*. A real tree can hold a package no root lists (an
|
||
`exclude`d crate, say), and nothing in the fixture would say so. So the gate
|
||
itself now checks an invariant **before it formats anything**: every
|
||
`[package]` manifest the find reaches must be a member of some root, or it
|
||
exits **2** and names the package. That is exit 2 and not 1 because no
|
||
verdict was produced for that package, which is what exit 2 means here.
|
||
Paths are compared through `realpath`, because on macOS `mktemp` lives under
|
||
`/var` while cargo reports `/private/var`.
|
||
|
||
**Rivals injected, and where each failed** (the suite went from 17 to 21 cases):
|
||
|
||
- **The review's skip-the-root rival.** It now fails the fixture at `a fault in
|
||
src/lib.rs is reported` (exit 0). With the invariant in place it fails
|
||
**six** cases (exit 2, "no workspace root reaches"). It also fails the
|
||
**real repo** with no fixture involved: `fmt-workspaces.sh` exits 2 naming
|
||
`Cargo.toml`. That last one is the property worth having. The gate refuses
|
||
its own blindness in CI, not only in its test suite.
|
||
- **Disabling the invariant** (`if false`). It fails `a package no workspace
|
||
root reaches is exit 2, not a pass` with **exit 0**, even though that
|
||
package's `src/lib.rs` is misformatted. That exit 0 *is* the hazard: an
|
||
unreached package is silently clean.
|
||
- **The two earlier rivals, re-run against the loop.** `--all` for `-p` fails
|
||
both `a/src/lib.rs` and `src/lib.rs` "named exactly once". The second of
|
||
those was not predicted: the nested root's `--all` also reaches the root
|
||
package. The bare manifest-path form fails `a fault in a/src/lib.rs is
|
||
reported`.
|
||
|
||
The real-repo control from the ticket still holds: a misformatted fn appended
|
||
to `src/ui/text.rs` is reported (`Diff in …/src/ui/text.rs:72`).
|
||
|
||
**What is still not covered.** A root whose member list *includes* something it
|
||
should not is not detected, and a package in a directory the find excludes
|
||
(`target/`, `.claude/`, `node_modules/`) is outside the invariant by
|
||
construction.
|
||
|
||
### The kb tool's completeness guard and atomic write — `scripts/kb-test.sh` cases 18–22
|
||
|
||
improvise-usus: `scripts/kb add` wrote onto a working-tree `kb.pl` that had lost
|
||
276 of its 801 clauses, and `kb_consistent` passed. The argument for the fix
|
||
(compare with HEAD by key, record `kb rm` removals in a ledger, publish by
|
||
rename) is in `context/code/kb-tool.md`. This entry covers what each case was
|
||
watched to catch.
|
||
|
||
**Reproduced at `40b8913` before any fix.** A copy of HEAD's `kb.pl` with 34 of
|
||
its 35 `location/3` facts removed still passes `kb_consistent`, and
|
||
`kb add` onto it printed `775 before, 1 in delta, 776 after` and exited 0.
|
||
Against the unfixed tool, the new cases gave 33 passed, 5 failed: 18, 19, 20b,
|
||
21 and 22 failed. Case 20 passed there because an rm followed by an add has no
|
||
guard to trip.
|
||
|
||
**The fixture has to fail in the incident's shape.** The ticket's "truncate to
|
||
half" fixture also breaks `kb_consistent`, because the rules live in the second
|
||
half. So the unfixed tool refused it too, by the wrong guard. Removing a whole
|
||
predicate is an existence error, and removing cited clauses fails the citation
|
||
check. Case 18 keeps one `location/3` fact and asserts that the fixture passes
|
||
`kb_consistent` before using it. Case 19 keeps the ticket's half-truncation, and
|
||
`said()` is what makes it evidence: the completeness guard runs first and must
|
||
be the one that answers.
|
||
|
||
**Rivals injected into `scripts/kb`, one at a time. Each failed only its own
|
||
case:**
|
||
|
||
- baseline taken from the file's own pre-write state rather than HEAD: 18, 19
|
||
and 20b failed (35/38 passed). This is the guard that would have passed the
|
||
real incident.
|
||
- ledger never consulted: 20 and 20b failed. A legitimate rm then blocks every
|
||
later write.
|
||
- guard stands down whenever the ledger is non-empty: 20b failed.
|
||
- candidate built by plain `mktemp` in `$TMPDIR`: 22 failed on the directory
|
||
check.
|
||
- candidate copied over `kb.pl` before the publish point: 22 failed as
|
||
modified.
|
||
- unreadable baseline exiting 1 rather than 3: 21 failed.
|
||
|
||
**A rival can fail wider than its fault.** The first form of the "stands down"
|
||
rival skipped `merge complete` altogether. That also skipped writing
|
||
`$cand.ledger`, so the later `mv` crashed under `set -e` and five unrelated
|
||
cases failed. It proved nothing until it was rewritten to carry the ledger
|
||
forward.
|
||
|
||
**Checked by hand, not in the suite:** the ledger prunes an entry once the key
|
||
is re-added and once HEAD stops having it (a committed removal).
|
||
|
||
### The kb completeness guard's follow-ups — `scripts/kb-test.sh` cases 19b, 20b, 23–25
|
||
|
||
improvise-ih3k, improvise-yea8 and improvise-za97, filed by the review of PR
|
||
#166. The argument is in `context/code/kb-tool.md`. This entry covers what each
|
||
case was watched to catch.
|
||
|
||
**Reproduced at `f909cad` before any fix.** The new cases 23, 23c and 24 gave
|
||
40 passed, 3 failed. 23 failed because an add onto a kb that had lost one of
|
||
two `work_done(<fixture id>,bug_fix,…)` clauses exited 0. 23c failed
|
||
because `rm` of that paired key was refused as "a clause was dropped": its
|
||
expected total assumed one clause per key. The tickets did not predict that
|
||
one, and it meant a pair could not be resolved through the tool at all. 24
|
||
failed because the stale ledger entry excused the loss.
|
||
|
||
**The ticket's own reproduction defeats a content stamp.** The first fix
|
||
stamped the ledger with the kb's sha256, and case 24 still failed. `kb.pl` is
|
||
canonical, so `grep -v` of one clause from HEAD is byte-identical to what `kb
|
||
rm` of that clause writes. The stamp therefore also carries the inode and the
|
||
nanosecond mtime.
|
||
|
||
**Case 20b had to change, and why that is not a weakening.** It made its
|
||
second loss with `sed -i` on the kb. Under the fix, any edit outside the tool
|
||
voids the ledger, so 20b then refused two clauses instead of one and failed on
|
||
its "1 of" assertion. It was testing the new voiding rather than what its
|
||
comment names. The second loss now comes from giving the *baseline* an extra
|
||
clause (`KB_BASELINE`). The kb file stays exactly as the tool wrote it, the
|
||
ledger stays valid, and the case again isolates "excuses only what it names".
|
||
It also now asserts the unexcused clause is named.
|
||
|
||
**Rivals injected, one at a time. Each failed only the cases listed:**
|
||
|
||
- keys compared as a set (`min(1, N)` per key) in `complete`: 23 only.
|
||
- `rm`'s expected total back to `before - |delta|`: 23c's rm step only.
|
||
- ledger pruned by key presence rather than count: 23c's final write only.
|
||
- ledger honoured whenever it exists: 24 and 25.
|
||
- ledger stamped by content alone: 24 only. 25 cannot see it, because its
|
||
intervening `add` makes the restored-then-lost file differ in content
|
||
anyway. 24 is the case that carries identity.
|
||
- ledger re-stamped only by `rm`, keeping the old stamp on other writes: 24b
|
||
(as named), 20b and 23c. All three write again after an add.
|
||
- ledger stamped with the kb as read rather than the candidate: 20, 20b, 23c,
|
||
24b and 25. That is wider than any one case, as expected, since no later
|
||
write can ever match.
|
||
- the `gh/main` `scripts/kb` (old refusal text, no stamp): 19b, 24 and 25.
|
||
|
||
**Case 25 is the first to run the production paths.** Every other case sets
|
||
`KB_FILE`, so `ledger_path`'s `git rev-parse --git-path` branch and
|
||
`baseline_to`'s `git show HEAD` were unexercised. It copies the tool into a
|
||
throwaway `git init` repo and checks four things. `rm` records to the git-path
|
||
ledger. `git status` shows only `kb.pl` modified, so the ledger is invisible
|
||
to git. A later `add` is excused. After `git checkout --` and a hand loss, the
|
||
`add` is refused.
|
||
|
||
**Checked by hand, not in the suite:** a `SIGKILL` at the publish point (the
|
||
`KB_TEST_KILL` seam) on a copy inside `prolog-sessions/` leaves the candidate,
|
||
its `.base`, `.ledger`, `.removed` and `.stamped` siblings and `.lock`. `git
|
||
status --untracked-files=all` and `git add -A --dry-run` both show none of
|
||
them, and `git check-ignore -v` names `.gitignore:40` (`prolog-sessions/*`).
|
||
|
||
### The pr-watch mergeability cases — `scripts/pr-watch-test.sh`
|
||
|
||
improvise-edpl. The cases are injected faults in themselves. Like the other
|
||
`scripts/` suites, none of their names is a Rust `fn`, so the case list is the
|
||
evidence. The reasoning is in `context/code/tooling.md`, "pr-watch and
|
||
mergeability".
|
||
|
||
**The bug, reproduced before the fix.** Against the script as it stood at
|
||
`ae96b8c`, 19 of the 34 new cases failed and all 19 exited **0**.
|
||
`UNKNOWN that resolves to CONFLICTING fails` is PR #172's shape exactly: every
|
||
check green, `UNKNOWN/UNKNOWN` on the first poll, `CONFLICTING/DIRTY` on the
|
||
second. The old script broke out of the loop on the first poll and passed. The
|
||
fixtures are hand-built. They are the subset of the `gh pr view --json` shape
|
||
that the script reads: `headRefOid`, the two mergeability fields, and the
|
||
rollup's `status` and `conclusion`. They are not a captured payload. A stub that
|
||
errors instead would have exited 4, a wider shape than the fault.
|
||
|
||
**Rival 1: treat `UNKNOWN` as an answer.** The wait condition was replaced with
|
||
`if true`. 13 cases fail: every `UNKNOWN` row of the table,
|
||
`MERGEABLE/UNKNOWN`, the timeout case, and
|
||
`UNKNOWN that resolves to MERGEABLE/CLEAN passes`. That last one exits 1 under
|
||
this rival, so the fix cannot pass by refusing everything uncertain. It has to
|
||
wait. `UNKNOWN that resolves to CONFLICTING fails` still passes under this
|
||
rival, because the allowlist refuses `UNKNOWN` as well. Rival 2 does not
|
||
separate it either: its first poll is `UNKNOWN`, which rival 2 still waits on.
|
||
What separates it is the original fault, which combined both rivals. An earlier
|
||
version of this entry said rival 2 separated it. That was wrong.
|
||
|
||
**Rival 2: the old verdict.** The allowlist was replaced with `exit 0` after the
|
||
`MERGEABLE` test. Five cases fail: `MERGEABLE/BEHIND`, `DIRTY`, `DRAFT`,
|
||
`SOMETHING_NEW` and `null`. These are the states that look mergeable by the flag
|
||
and are not. At `128a2a0` it was six, with `BLOCKED` among them. Since
|
||
improvise-zlj2, `BLOCKED` is waited on, so this rival never reaches the verdict
|
||
for it. The entry below covers that.
|
||
|
||
`a failed check fails without waiting on mergeability` runs at `--timeout 0`.
|
||
A script that waited on `UNKNOWN` before reading the rollup would report that
|
||
known red as exit 3.
|
||
|
||
### The pr-watch needs-gap and pinned-head cases — `scripts/pr-watch-test.sh`
|
||
|
||
improvise-zlj2 and improvise-h3ni. These were found by review of PR #186 at
|
||
`128a2a0`, which was ruled do-not-merge.
|
||
|
||
**zlj2: a healthy PR exited 1.** The required `Browser harness` check run
|
||
exists only after the `build` job completes (`needs: build`). In that gap
|
||
GitHub reports `MERGEABLE/BLOCKED` while every registered check is green, and
|
||
the #186 rule refused `BLOCKED`. The rollup in the new cases is PR #189's,
|
||
captured verbatim with `gh pr view --json`. It has thirteen entries with every
|
||
field GitHub returns. Each gap state drops or rewinds entries of that rollup
|
||
with `jq`. Against `128a2a0` three cases failed, all with exit 1:
|
||
|
||
- `the needs-gap before the required check registers is not a failure`: the
|
||
healthy sequence should exit 0.
|
||
- `a required check that never registers is a timeout`: should exit 3.
|
||
- `table: MERGEABLE/BLOCKED`: should exit 3, not 1.
|
||
|
||
**Rival for zlj2:** remove `MERGEABLE/BLOCKED` from the wait. The same three
|
||
cases fail with exit 1. That rival is `128a2a0`'s code.
|
||
|
||
**h3ni: nothing pinned the head across the UNKNOWN wait.** The case is
|
||
`a head that moves during the UNKNOWN wait is not a pass`. Its polls are
|
||
UNKNOWN@a, then UNKNOWN@b, then CLEAN@b, and it must exit 2. It passed on the
|
||
first run, so it is a guard for behaviour the script already has, not a
|
||
reproduction.
|
||
|
||
**Rival for h3ni:** re-pin the head whenever `mergeable` is UNKNOWN. That case
|
||
alone fails, exiting 0. All 44 others pass. That is the review's point: the
|
||
older `a head that moves mid-wait is not a pass` starts from MERGEABLE, so it
|
||
cannot see this rival.
|
||
|
||
### The pr-watch red-under-BLOCKED and ended-PR cases — `scripts/pr-watch-test.sh`
|
||
|
||
improvise-9pfs and improvise-1qaa. Both were found by the re-review of PR #186
|
||
at `04230a7`.
|
||
|
||
**9pfs: the commonest red had no case.** Every red fixture wore `UNSTABLE` or
|
||
`UNKNOWN`. Here, though, a failed *required* check reports `MERGEABLE/BLOCKED`,
|
||
and the table enumerates green rollups only. The new case is
|
||
`a failed required check under MERGEABLE/BLOCKED fails without waiting`. It uses
|
||
PR #189's rollup with `Test and lint (host)` rewritten to `FAILURE`, runs at
|
||
`--timeout 0`, and expects exit 1. It passed at `202e7eac`, since the script
|
||
already broke on any failed check, so it pins existing behaviour. **Rival:** the
|
||
failed-check break gated on `state != BLOCKED`. That case alone fails, exiting 3
|
||
where 1 is wanted, and the other 45 cases pass. Nothing else in the suite could
|
||
see a refactor that turned every required-check red into a timeout.
|
||
|
||
**1qaa: a merged PR slept the whole timeout.** Two case pairs. Each uses the
|
||
shape GitHub returned live, with green checks. MERGED is `UNKNOWN/UNKNOWN`
|
||
(#189 once merged). CLOSED is `CONFLICTING/DIRTY` (#74). The CLOSED fixture was
|
||
`UNKNOWN/UNKNOWN` at `b13355c7`, and review found that shape unreal. Each pair
|
||
asserts exit 5, and that the fixture was read **once**, using the poll counter
|
||
`fetch` keeps on disk. Against the script at `202e7eac`:
|
||
|
||
- MERGED: exit 3 after 102 polls within a 5s timeout.
|
||
- CLOSED: exit 1 on the first poll. `CONFLICTING` already ended the wait, but
|
||
as "not mergeable".
|
||
|
||
Two rivals:
|
||
|
||
- **The state checked only when the timeout expires.** The MERGED read-once case
|
||
fails (44 polls), and so does the CLOSED exit-code case, because `CONFLICTING`
|
||
breaks to exit 1 before the timeout branch is reached. The poll count is what
|
||
pins "does not wait" for MERGED, which is why each case is a pair.
|
||
- **Exit 1 instead of 5.** Both exit-code cases fail with exit 1. That keeps
|
||
"already merged" apart from "not mergeable".
|
||
|
||
**29w2: the CLOSED cases could not discriminate.** Found reviewing #253 at
|
||
`8cdea3a4`. The only CLOSED shape was #74's `CONFLICTING/DIRTY`, and that ends
|
||
the loop on the first poll whether or not the state is checked there. So
|
||
**rival R8** passed 52 of 52. R8 exits 5 for MERGED at the loop top, but for
|
||
CLOSED only after the loop breaks. Run live against #46 with `--timeout 15`, R8
|
||
exited 3 and `8cdea3a4` exited 5. A third shape is added, #46's: CLOSED, green
|
||
checks, `MERGEABLE/BLOCKED`, which the loop waits on. #74's shape stays.
|
||
|
||
- **R8:** both new cases fail, the exit-code case with exit 3 and the read-once
|
||
case after 65 polls. The other 52 pass.
|
||
- **The state checked only at timeout:** the new read-once case fails (77 polls),
|
||
alongside the MERGED read-once case and the `CONFLICTING/DIRTY` exit-code case.
|
||
|
||
### The pr-watch ended-PR ordering cases — `scripts/pr-watch-test.sh`
|
||
|
||
improvise-24lz, found reviewing #253 at `b13355c7`. That head checked `state`
|
||
after `--expect` but **before** pinning the head. So a watch with no `--expect`
|
||
that saw head a running, and then head b MERGED, exited 5 where it should have
|
||
exited 2. It accepted a merge at a head the watcher never saw, which is the
|
||
PR #67 hazard. Two cases:
|
||
|
||
- `a head that moves and then merges is not reported as merged`: head a with a
|
||
running check, then head b MERGED. Expects 2. At `b13355c7` it exited 5.
|
||
- `a PR merged at a head other than --expect is a head mismatch`: head b MERGED,
|
||
`--expect aaaa`. Expects 2. It passed at `b13355c7`, where `--expect` already
|
||
came first.
|
||
|
||
The fix moves the `state` check below the pin block. On the first poll that
|
||
block only records the head, so an already-merged PR still ends on its first
|
||
read. Both new cases pass at `202e7eac`, since it never reads `state`; they pin
|
||
the order of the new code. **Rivals:**
|
||
|
||
- **The state checked before `--expect` and before the pin.** Both new cases fail
|
||
with exit 5, and the other 50 pass.
|
||
- **The `b13355c7` placement.** Only the moved-then-merged case fails.
|
||
|
||
## Every JSON parser reads numbers exactly (improvise-dz8c)
|
||
|
||
### `every_crate_that_can_parse_json_enables_float_roundtrip`
|
||
|
||
It replaces a check over three crates listed by hand. It reads
|
||
`cargo metadata --no-deps` and fails naming every workspace package, and
|
||
every target-specific entry, whose **normal** serde_json dependency lacks
|
||
`float_roundtrip`. The rule and why it was chosen over the alternatives are in
|
||
`context/code/protocol.md`. It is red at `1158c23`, naming `improvise`,
|
||
`improvise-command`, `improvise-io` and `improvise-protocol`.
|
||
|
||
**Faults injected, each run:**
|
||
|
||
- **The feature removed from `improvise-io` alone.** The check fails naming
|
||
`improvise-io (all targets)`. So did
|
||
`a_legacy_json_workbook_loads_its_numbers_bit_for_bit`, until improvise-tt22
|
||
removed the legacy load path and that test with it.
|
||
`a_json_import_stores_its_numbers_bit_for_bit` stays green, because
|
||
`improvise-command` enables the feature and unification reaches io in its
|
||
build.
|
||
- **The feature removed from `improvise-command` alone.** The check fails naming
|
||
`improvise-command (all targets)`. The command crate's own import test
|
||
**stays green**: `cargo tree -p improvise-command` shows io's declaration
|
||
switching the feature on. So the manifest check is the only guard that
|
||
sees this case. It is also the case the retired build-graph check would
|
||
have passed.
|
||
- **A throwaway workspace member**, `zz-throwaway`, with normal dependencies on
|
||
`improvise-protocol` and `serde_json` and no feature (the lockfile updated,
|
||
as a real new crate would be). The check fails naming
|
||
`zz-throwaway (all targets)`. With the unconditional dependency carrying the
|
||
feature and a `cfg(target_arch = "wasm32")` one lacking it, the check fails
|
||
naming `zz-throwaway (cfg(target_arch = "wasm32"))`. That is stricter than
|
||
the build needs, since a wasm build unifies the two entries, and cheap to
|
||
satisfy. The crate was then removed.
|
||
|
||
The `checked.len() >= 2` assertion is the positive signal. If a cargo change
|
||
renamed the metadata fields, the check would find no serde_json dependency and
|
||
pass vacuously without it.
|
||
|
||
### The re-poll a second boundary could skip — `scripts/pr-reviews-test.sh` (improvise-qnpi)
|
||
|
||
**What broke.** "an inline comment arriving mid-wait is seen" failed once in
|
||
two identical runs of `checkers.sh` on a loaded machine. `pr-reviews --wait`
|
||
computed `deadline = now + WAIT` and then tested `now < deadline` *before* its
|
||
first re-poll. With `--wait 1`, a second boundary falling between those two
|
||
reads of the clock made the loop run zero times: exit 1 with one poll, whatever
|
||
the second poll would have shown.
|
||
|
||
**Reproduced under load.** A loop running that fixture with one busy loop per
|
||
core failed once, as `exit 1, polls=1`, around run 52; the fixed script ran
|
||
200 times under the same load with no failure. That is evidence of the
|
||
mechanism, not proof: the window is a few milliseconds per run.
|
||
|
||
**The case that proves it.** "a second passing before the first re-poll does
|
||
not skip it" puts a `date` on `PATH` that advances one second per read (a
|
||
loaded machine, compressed) and a no-op `sleep`. Against the old loop it fails
|
||
every time, exit 1 (22 passed, 1 failed). The fix checks the deadline *after*
|
||
each poll, so any `--wait` above 0 polls at least once more. A larger timeout
|
||
would not have helped: the case uses the shimmed clock, where only the order of
|
||
check and poll matters.
|
||
|
||
### The harness lock that every build rewrote — `harness/build.sh` (improvise-mkbp)
|
||
|
||
**What broke.** `harness/opfs-contract/Cargo.lock` pinned `pest 2.9.1` while
|
||
its path dependencies require `pest = "2.9.2"`, so the committed lock was
|
||
invalid for its own manifest and every cargo invocation rewrote it.
|
||
|
||
**Why both halves.** The resolved lock is committed, and `build.sh` now starts
|
||
with `cargo metadata --locked` against it. The lock alone would repeat the bug
|
||
at the next workspace dependency bump; `--locked` alone fails today. The check
|
||
is its own step because `wasm-pack build … -- --locked` does not work: with the
|
||
stale lock restored, that version exited 0 and still rewrote the lock, because
|
||
wasm-pack re-resolves before the flag reaches `cargo build`. The standalone
|
||
check, against the same stale lock, exits 101 in seconds and leaves the file
|
||
untouched.
|
||
|
||
**The assemble half.** `web/assemble.sh` starts with `rm -rf` of its output,
|
||
which is what keeps the deployed tree free of stale files, so it is not taught
|
||
to spare a harness directory. `opfs.spec.js` instead asserts `/opfs/index.html`
|
||
is served, naming the fix. Injected by running `web/assemble.sh harness/serve`
|
||
alone: 4 chromium failures, each carrying that message, where it used to fail
|
||
with no hint of the cause.
|
||
|
||
### Polling past the second poll — `scripts/pr-reviews-test.sh` (improvise-7epn)
|
||
|
||
**What was unguarded.** Every multi-poll fixture resolved on poll 2, and the
|
||
timeout case expects exit 1 whether the loop polls twice or twenty times. So
|
||
replacing the post-poll deadline check with an unconditional `break` left the
|
||
suite at 23 passed, 0 failed, and `--wait N` would have polled at most twice.
|
||
|
||
**Two cases, both on the one-tick-per-read clock.** "a review arriving on the
|
||
third poll is seen" empties polls 1 and 2 and puts an inline comment at the head
|
||
in `last`, with `--wait 10`, and asserts exit 0 *and* exactly 3 polls. "--wait
|
||
keeps polling until its deadline" runs the empty fixture with `--wait 5` and
|
||
asserts exit 1 after at least 5 polls. The second one is the timeout path. Its
|
||
exit code alone cannot tell a loop that stopped early from one that reached the
|
||
deadline, so it counts polls.
|
||
|
||
**Rival, injected and watched.** `break` in place of
|
||
`[ "$(date +%s)" -lt "$deadline" ] || break`: 23 passed, 2 failed. The failures
|
||
were exactly the two new cases, each "exit 1 after 2 polls". The first attempt
|
||
to inject used `sed -i ''`, which the GNU sed on this PATH read as a filename.
|
||
Nothing changed and the suite passed 25/25, which looks like a vacuous rival.
|
||
The second attempt asserted the replacement landed before running.
|
||
|
||
**Still not covered.** Deleting the check outright makes the timeout case loop
|
||
forever rather than fail. The suite has no wall-clock guard, so that fault shows
|
||
as a hang, not a FAIL.
|
||
|
||
### A stale harness lock names its remedy — `harness/build.sh` (improvise-lnsh)
|
||
|
||
**What broke.** With a stale `harness/opfs-contract/Cargo.lock`, `build.sh`
|
||
exited 101 and printed only cargo's text, whose `help:` line says to remove
|
||
`--locked`. That is the one change that brings improvise-mkbp back. The real
|
||
remedy was only in a source comment.
|
||
|
||
**Reproduced, then fixed, by hand.** Changing `pest`'s locked version from
|
||
`2.9.2` to `2.9.1` in the lock reproduces the stale state. Before the fix:
|
||
exit 101 and cargo's hint only. After: exit 1, and the script's own two lines
|
||
follow cargo's, telling the reader not to remove `--locked` and naming
|
||
`cargo update -w --manifest-path harness/opfs-contract/Cargo.toml`. The lock
|
||
was untouched both times. Running that command on the stale lock restored it
|
||
byte for byte (`cmp` against the committed file). With the committed lock,
|
||
`build.sh` still runs to "harness/serve ready".
|
||
|
||
**Not automated.** There is no suite for `harness/build.sh`. A test would have
|
||
to corrupt a tracked file and call cargo, which needs the crates.io index, from
|
||
`checkers.sh`, whose environment is scrubbed. The by-hand run above is the
|
||
evidence.
|
||
|
||
### The timeout case asserts the exact poll count, and a hang fails — `scripts/pr-reviews-test.sh` (improvise-7fl8)
|
||
|
||
**What was unguarded.** "--wait keeps polling until its deadline" asserted *at
|
||
least* 5 polls for `--wait 5`. On the one-tick-per-read clock the correct loop
|
||
polls exactly 6 times: one initial poll, then re-polls until the post-poll clock
|
||
read reaches the deadline. The 7epn entry above says the poll count tells a
|
||
loop that stopped early from one that reached the deadline. That was true only
|
||
for a loop stopping two or more ticks early. Both rivals from the #218 review
|
||
passed the whole suite at 25/25, re-run here before the change:
|
||
- a loop capped at 5 polls whatever the deadline;
|
||
- `deadline = now + WAIT - 1`.
|
||
|
||
**The fix** asserts `polls = WAIT + 1`, which the fake clock makes exact. It
|
||
also catches an over-polling `-le`. Against the new assertion both rivals give
|
||
24 passed, 1 failed, each "exit 1 after 5 polls, wanted 1 after 6".
|
||
|
||
**The hang, which was wider than the ticket said.** 7epn left open that
|
||
deleting the deadline check hangs the suite. It does not hang only at the
|
||
fake-clock case. It hangs first at "an inline comment at an EARLIER head does
|
||
not satisfy --wait", a real-clock `--wait 1` case, and at every other case that
|
||
should time out. So the guard wraps *every* call. `"$REVIEWS"` is now a function
|
||
that runs the script alongside a `/bin/sleep 20` watchdog and waits for
|
||
whichever finishes first (`wait -n -p`, bash 5.1+). If the watchdog wins, it
|
||
kills the script and returns 124. `/bin/sleep` is spelled out because the
|
||
fake-clock cases put a no-op `sleep` first on `PATH`. A first version that
|
||
killed a watchdog subshell left an orphaned `sleep`, and could signal a
|
||
recycled pid.
|
||
|
||
**Rival: the deadline check deleted.** The suite now finishes in 93 s with 21
|
||
passed and 4 failed. The four timeout cases fail with exit 124; the fake-clock
|
||
one has made about 400 polls by then. Before the guard the same rival never
|
||
finished, and an outer `timeout 30` killed it at exit 124.
|
||
|
||
### The bash floor is 5.1 — `scripts/checkers-test.sh` (improvise-fd0j)
|
||
|
||
**What broke.** `pr-reviews-test.sh` calls `wait -n -p`, and `-p` arrived in
|
||
bash 5.1. `checkers.sh`'s `find_modern_bash` accepted any major version 5. So a
|
||
bash 5.0 on `PATH` was pinned, and that suite then failed on an invalid option
|
||
rather than on anything it tests.
|
||
|
||
**Fix.** The probe reads major and minor in one call and requires
|
||
`major*100 + minor >= 501`. The exit-3 message and the header say 5.1.
|
||
`context/code/tooling.md` states why the floor is 5.1.
|
||
|
||
**Cases** (section 5b). A fake bash answering `5 0` with no nix → exit 3. The
|
||
control, a fake answering `5 1` → exit 0, proves that refusal comes from the
|
||
version and not from the fake.
|
||
|
||
**Rivals**, run against the real suite in a copied sandbox:
|
||
- **Floor 5.0** (`-ge 500`) fails "a bash 5.0 with no nix is exit 3, not 5.x"
|
||
and nothing else.
|
||
- **The pre-fix runner** fails only the 5.1 control. Its probe asked for the
|
||
major version alone, so the fake's `5 1` was not a number and was refused.
|
||
The 5.0 case passes against it for the same reason, so that case alone would
|
||
not have distinguished the old code. The 5.0-floor rival is the one that
|
||
does.
|
||
|
||
### The dead-ID audit compares positively — `scripts/bd-audit-ids-test.sh` (improvise-52eq)
|
||
|
||
**What broke.** The audit recipe in `AGENTS.md` ended
|
||
`bd show $(cat ids) 2>&1 | grep -E '^(Issue … not found|…)' || echo "all resolve"`.
|
||
It looked for the *absence* of a negative line. So a bd that was missing,
|
||
failed, or never ran printed no such line, and the audit reported "all
|
||
resolve". In this session one attempt at the audit printed "all resolve"
|
||
because its output file did not exist.
|
||
|
||
**The fix.** `scripts/bd-audit-ids` keeps the file scan and the exclusions as
|
||
they were. It then takes the known ids from `bd list --all --limit 0 --json`
|
||
and `comm -23`s the cited set against them. It exits 3 if bd is missing, fails,
|
||
prints no JSON list, or lists no issues. On the tree at `12c81dd` it reports
|
||
460 cited ids, the same count the old pipeline produced there.
|
||
|
||
**The suite** runs a throwaway git repo and a fake bd that answers only `list`.
|
||
It has eight cases: a pass, a dead id (exit 1, named), four shapes of a bd that
|
||
does not answer (no output, `[]`, a failing exit, not on PATH), the real tree
|
||
against a silent bd, and `--help`. The passing fixture also cites
|
||
`improvise-core` and keeps a dead id in a git-ignored `harness/serve/` file.
|
||
So the pass needs both the exclusions and git's file list.
|
||
|
||
**Rivals, each run:**
|
||
- **The old recipe**, pasted in as the script's tail: 1 passed, 7 failed.
|
||
Every "bd did not answer" case exited 0 with "all resolve", which is the
|
||
bug. The dead-id case failed only because the fake bd refuses `show`. That is
|
||
one more bd that did not answer, so it is not evidence the old recipe misses a
|
||
dead id against a working bd, which it catches.
|
||
- **The dead-id comparison disabled** (`: >` for `comm -23`): only "a dead id
|
||
is exit 1, and named" failed, with exit 0.
|
||
- **An empty exclusion list**: only the passing case failed, reporting
|
||
`improvise-core` as dead.
|
||
|
||
After each rival the original was restored (`cmp` against a saved copy) and the
|
||
suite re-run at 8 passed.
|
||
|
||
### The audit tells grep's no-match from its read error — `scripts/bd-audit-ids-test.sh` (improvise-lqs0)
|
||
|
||
**What broke.** `bd-audit-ids` treated xargs exit 123 as "a grep batch matched
|
||
nothing". xargs also exits 123 when grep exits 2, so a cited file the scan could
|
||
not read passed the audit, and any dead id inside it was missed. The usage text
|
||
promised exit 4 for that. Two smaller mismatches with the stated contract:
|
||
- run outside a git repository, the audit exited 1, which is the "dead id" code;
|
||
- given a bd list whose objects carry no `id`, it reported every cited id dead
|
||
(exit 1) where "bd did not answer" is 3.
|
||
|
||
**Reproduced before the fix, with six new cases:** 10 passed, 4 failed.
|
||
- a dangling symlink among the cited files: exit 1, the read error ignored;
|
||
- an unreadable (`chmod 000`) file holding the only dead id: **exit 0, "all
|
||
resolve"**, which is the false pass;
|
||
- outside a repository: exit 1;
|
||
- id-less JSON: exit 1.
|
||
|
||
The control (both files readable, dead id found: exit 1) and the deleted-file
|
||
case passed before and after.
|
||
|
||
**The fix:**
|
||
- Each grep batch runs through `bash -c` and maps exit 1 to success and exit 2
|
||
to 255. xargs stops on 255 with 124, so a read error is exit 4.
|
||
- A tracked file deleted from the working tree is filtered out **on purpose**.
|
||
Its ids are leaving the tree, which is the case the ticket asked to decide
|
||
rather than leave to accident. A dangling symlink is still an entry, so it
|
||
still counts as a read error.
|
||
- Outside a repository is exit 4.
|
||
- bd's output must be a JSON array whose every element has a string `id`, or
|
||
the audit is exit 3.
|
||
|
||
**Rivals, each run against the suite (the script restored and checked with
|
||
`cmp` after each):**
|
||
- **read error mapped to success** (`-le 2`): fails both read-error cases, 12
|
||
passed and 2 failed;
|
||
- **the deleted-file filter removed**: fails "a tracked file deleted from the
|
||
working tree is not scanned" with exit 4. So the decision is pinned, not
|
||
incidental;
|
||
- **the old `cd "$(git rev-parse --show-toplevel)"`**: fails "run outside a git
|
||
repository is exit 4" with exit 1. A first attempt at this rival still exited
|
||
4 and proved nothing; it was replaced;
|
||
- **the id check reduced to `type == "array"`**: fails the id-less case with
|
||
exit 1.
|
||
|
||
The `chmod 000` case is skipped when the suite runs as root, where the file
|
||
stays readable. The dangling-symlink case covers the same path for every user.
|
||
|
||
### Absent is not deleted — `scripts/bd-audit-ids-test.sh` (improvise-tdfd)
|
||
|
||
**What broke.** The file filter `[ -e "$f" ] || [ -L "$f" ]` dropped every
|
||
tracked path missing from the tree, and took each one to be a deletion. A path
|
||
marked `--skip-worktree` (or left out by a sparse checkout) is absent too, but
|
||
it is not deleted. Its ids were never read, and the audit passed. ChatGPT
|
||
Codex found this on PR #244. The adversarial review reproduced it with
|
||
`context/adr/001-concurrent-edits.md`, the only citation of one id: exit 1 → 0.
|
||
|
||
**Fix, Codex's suggestion.** An absent path is skipped only when
|
||
`git ls-files -d` lists it. Any other absent tracked path is exit 4, and the
|
||
path is named. The xargs comment no longer claims 124. That is GNU; BSD exits
|
||
1. The script tests nonzero, so both work.
|
||
|
||
**Red first.** "a skip-worktree file missing from the tree is exit 4, and
|
||
named" exited 0 against the pre-fix script, reporting "all 1 cited ids
|
||
resolve".
|
||
|
||
**Rival:** every absent path reported as unread (the `ls-files -d` test
|
||
removed). It fails "a tracked file deleted from the working tree is not
|
||
scanned" alone, so the deleted-file case is what stops the fix from
|
||
over-refusing. The rival ran from an ignored copy inside the worktree. A copy
|
||
under `/tmp` fails "the real tree does not pass against a silent bd" for a
|
||
reason unrelated to the rival: there is no repository there.
|
||
|
||
### Section 5b runs on a hermetic PATH — `scripts/checkers-test.sh`
|
||
|
||
**What broke.** Every PR's "Knowledge base consistency" job went red at
|
||
`666de199` with "a bash 5.0 with no nix is exit 3, not 5.x (exit 1, wanted 3)"
|
||
and "a bash 5.1 is accepted (exit 1, wanted 0)". Neither PR alone was wrong. The
|
||
NixOS-runner change (`af44653f`) found that `/usr/bin:/bin` on NixOS holds only
|
||
`env` and `sh`, so the runner dies on `dirname` with exit 1. It fixed section 5
|
||
by moving it to a `utilbin` of linked tools. #237 (`e45a9c42`) added section 5b
|
||
on its own branch, with the old `PATH=$work/fakebin:/usr/bin:/bin`. The two
|
||
merged with no conflict, and each had passed CI only on its own side.
|
||
|
||
**Reproduced** on macOS by running both 5b invocations with `/usr/bin:/bin`
|
||
replaced by a directory holding only `env` and `sh`. Both exited 1 on
|
||
`dirname: command not found`. Those are the CI exits.
|
||
|
||
**Fix.** 5b uses `utilbin` as well. `utilbin` gains `head` and `env`, because
|
||
the 5.1 control runs past the version check into `check_shebangs` and
|
||
`run_suite`.
|
||
|
||
**Rivals**, each run in the suite:
|
||
- **`dirname` left out of `utilbin`** fails all three section-5 cases with the
|
||
exact CI pair: exit 1 where 3 was wanted, and exit 1 where 0 was wanted. The
|
||
NixOS failure now reproduces on any host, because no case reads the host's
|
||
`/usr/bin` any more.
|
||
- **`env` left out** fails "a bash 5.1 is accepted" alone.
|
||
- **`head` left out** fails nothing. `check_shebangs` calls `head` inside
|
||
`check_shebangs || rc=1`, where `set -e` is off. A missing `head` makes every
|
||
first line empty, and the shebang check then passes without checking
|
||
anything. That is a defect in `checkers.sh` and is not fixed here.
|
||
|
||
### The pr-reviews watchdog resolves its tools from PATH — `scripts/pr-reviews-test.sh` (improvise-3uow)
|
||
|
||
**What broke.** The suite hard-coded `/bin/sleep` for the watchdog in
|
||
`bounded_reviews` and `/bin/date` for the fake clock's pass-through. On the
|
||
NixOS runners `/bin` holds only `sh`. With no `/bin/sleep`, the watchdog exits
|
||
at once. `wait -n` then sees it finish before `pr-reviews`, and every bounded
|
||
call reports 124 ("killed after 20s"). CI never showed this, because the kb job
|
||
stopped one step earlier on the section-5b failure (#296).
|
||
|
||
**Reproduced** by pointing both paths at `/nonexistent/`. The result was 0
|
||
passed and 25 failed, every `--wait` case exiting 124: the same shape the
|
||
reviewer simulated.
|
||
|
||
**Fix.** `REAL_SLEEP` and `REAL_DATE` come from `type -P`, before any case puts
|
||
its stub `sleep` and `date` first on PATH. `type -P` returns a path even if
|
||
`sleep` has been loaded as a builtin. `REAL_DATE` is exported, because the fake
|
||
clock is a separate script.
|
||
|
||
**Rivals:**
|
||
- **`REAL_SLEEP=/nonexistent/sleep`** fails the whole suite, 0 of 25, so the
|
||
resolved path is the one the watchdog runs.
|
||
- **The `REAL_DATE` half has no discriminating rival.** `pr-reviews` only ever
|
||
calls `date +%s`, so the fake clock's pass-through branch never runs. It was
|
||
fixed so that it cannot break later, but no test can see it today.
|
||
|
||
**Scope sweep.** `grep -rnE '/(usr/)?bin/[a-z]'` over `scripts/`, `web/*.sh`,
|
||
`harness/*.sh`, `harness/tests` and `.github/workflows` finds only `#!/bin/sh`
|
||
shebangs, and `/bin/sh` exists on NixOS. It also found a
|
||
`${PATH:-/usr/bin:/bin}` fallback in `checkers.sh`, which is now removed (next
|
||
entry).
|
||
|
||
### The bash floor and its self-tests are retired — `scripts/checkers.sh`
|
||
|
||
**The ruling.** The user ruled that CI should not care which version of bash or
|
||
of the standard utilities it runs, and that CI jobs should use whatever the
|
||
`.#ci` dev shell provides. So `checkers.sh` no longer pins a bash:
|
||
`find_modern_bash`, the 5.1 floor (improvise-fd0j) and exit 3 are gone. Suites
|
||
run under `$BASH`, the bash running the runner, with `PATH` unchanged. The
|
||
reasoning is in `context/code/tooling.md` §5.
|
||
|
||
**Tests removed.** Three sections of `checkers-test.sh` were removed. They
|
||
existed only to pin the floor, and the entries above that cite them describe
|
||
cases that no longer exist:
|
||
- Section 2, "a bash-3.2-only construct fails under the runner's pinned bash".
|
||
It asserted a bash-5 behaviour, so its result depended on the bash version.
|
||
- Section 5, "an old bash with no nix is exit 3".
|
||
- Section 5b, "a bash 5.0 with no nix is exit 3, not 5.x" and "a bash 5.1 is
|
||
accepted", together with the `utilbin` that #296 added for them.
|
||
|
||
The suite keeps 10 cases: environment scrubbing (with its direct-run pair), the
|
||
shebang rule (with its control), fail and pass, unknown suite, and `--list`.
|
||
|
||
**Kept deliberately.** The environment allowlist. That is what the
|
||
`set -u`/`$name` evidence above earns, and it does not depend on any version.
|
||
|
||
**Also removed with the ruling.** `scripts/kb`'s `stat -f` fallback: `kb`
|
||
already requires the dev shell for `tpl`, and the dev shell's `stat` is GNU.
|
||
From `ci.yml`, the toolchain version probes ("the dev shell supplies the pinned
|
||
web toolchain", "Playwright matches the dev shell's browsers") and every
|
||
`shell: bash` override, so every step runs in `nix develop .#ci`.
|