d6adb9e0e3
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
1918 lines
105 KiB
Markdown
1918 lines
105 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.
|
||
|
||
### Every checkers.sh suite runs in CI — `scripts/check-ci-graph.py` (improvise-dak5)
|
||
|
||
**What was unguarded.** `ci.yml` names its `checkers.sh` suites by hand. A new
|
||
`scripts/X-test.sh` beside `scripts/X` is picked up by `checkers.sh` locally
|
||
and never runs in CI unless someone adds it to a step. Found on #226, where
|
||
`bd-audit-ids` had to be added by hand.
|
||
|
||
**Planted in the real tree.** `scripts/zz-planted.sh` plus
|
||
`scripts/zz-planted-test.sh` appear in `checkers.sh --list`, and
|
||
`check-ci-graph.py` then exits 1 with *`zz-planted` is a checkers.sh suite that
|
||
no CI step runs; add it to ci.yml*. As a control, appending `zz-planted` to the
|
||
named step clears it (exit 0, "all 9 checkers.sh suites run in CI"). Both files
|
||
and the `ci.yml` edit were then removed, and the real run is back to 8 suites,
|
||
exit 0.
|
||
|
||
**Seven self-test suite lists.** They cover: a suite no step runs; suites named
|
||
across several steps (one inside `nix shell … --command`); a bare run covering
|
||
everything; `--self-test` covering `checkers` and nothing else; a bare mention
|
||
in a YAML comment, which must *not* count as a bare run; names continued onto
|
||
the next line with a backslash; and a step naming a suite that does not exist.
|
||
|
||
**Rivals, each run against `--self-test`:**
|
||
- **YAML comments no longer skipped:** exactly the comment case fails, because
|
||
the bare mention there reads as "everything runs" and the missing suite
|
||
passes.
|
||
- **`suites_run` always answering "everything"** (the vacuous shape): five
|
||
cases fail, every one that expects a suite to be named.
|
||
|
||
The script was restored after each (`cmp` against a saved copy), and the
|
||
self-test is back to "5 graphs and 7 suite lists judged".
|
||
|
||
### The suite check asks checkers.sh what a step runs — `scripts/check-ci-graph.py` (improvise-maax)
|
||
|
||
**What broke.** The adversarial review of #228 at `cee61b94`, and ChatGPT
|
||
Codex's P1 on it, found the first version failing open in four shapes. Each let
|
||
the check pass while a suite never ran:
|
||
- a bare `scripts/checkers.sh` outside a command read as "every suite runs":
|
||
after an inline `#` (Codex), in a step `name:`, or as `echo`'s argument;
|
||
- `checkers.sh --list` alone read as every suite, though `--list` runs nothing;
|
||
- `--self-test zz-new kb` counted `zz-new` and `kb`, though `--self-test`
|
||
discards them;
|
||
- `--list kb zz-new` counted both.
|
||
|
||
Two more were loud but wrong: only the first invocation on a line counted, and
|
||
quoted names kept their quotes.
|
||
|
||
**The fix moves the argument rules back into `checkers.sh`.** It gains
|
||
`--would-run`, which is honoured only as the first argument and reports rather
|
||
than runs. `check-ci-graph.py` finds invocations: `run:` text only, comments
|
||
stripped by bash's rule, command position only. It then asks `--would-run`
|
||
about each one.
|
||
|
||
**Every new case run against the OLD parser** (`/tmp/ccg-old.py`, the file at
|
||
`cee61b94`): eight of the sixteen suite cases give the wrong answer there. The
|
||
five false passes are `--list` alone, the inline comment (Codex), the non-run
|
||
block scalar, the step `name:` and `echo`'s argument. Wrong in a noisier way:
|
||
`--self-test zz-new kb` (the old parser counts `kb`, so it reports only a
|
||
bogus "zz-new is not a suite"), `--list kb zz-new` (the same), and `a && b`
|
||
(the second suite is missed).
|
||
|
||
**A vacuity fixed before the rivals ran.** As first written, the bare-mention
|
||
cases required `zz-new`, which no real `checkers.sh` ever runs. A false bare
|
||
run then still left exactly `zz-new` unrun, the expected answer, so the
|
||
any-word-position rival passed all sixteen. The cases now require `kb` and
|
||
`pr-watch`, and the real step runs only `kb`. A false bare run is then the only
|
||
way `pr-watch` counts as run.
|
||
|
||
**Rivals, one function swapped in at a time and each run against the real
|
||
`--self-test`:**
|
||
- **no comment stripping:** fails only "a suite named inside a trailing shell
|
||
comment is not run";
|
||
- **every line scanned** (run: prefixes still recognised): fails "a
|
||
command-shaped line in a non-run block scalar" and the block-run case;
|
||
- **`checkers.sh` accepted in any word position:** fails only "a bare mention
|
||
as echo's argument";
|
||
- **argv modelled naively** (names, else everything): fails the `--self-test`
|
||
case and both `--list` cases;
|
||
- **first invocation per line only:** fails "every invocation on a line
|
||
counts" and the block-run case;
|
||
- **`--would-run` honoured in any position**, in `checkers.sh` itself: fails
|
||
`checkers-test.sh`'s "--would-run anywhere but first is a usage error" (exit
|
||
0, wanted 4) and the parser's "a step spelling --would-run runs nothing".
|
||
|
||
One more rival crashed instead: an early every-line rival hit `shlex`'s
|
||
"No escaped character" on a lone trailing backslash. So a line that cannot be
|
||
parsed now contributes no invocation, which fails closed. Everything was
|
||
restored (`cmp` for `checkers.sh`) and re-run: `--self-test` reports 16 suite
|
||
lists and `checkers-test.sh` 17/17. A planted `zz-planted` suite still fails
|
||
the real run, naming it.
|
||
|
||
### The suite gate reads a table, not shell — `scripts/check-ci-graph.py` (improvise-st8q, improvise-tvte)
|
||
|
||
**What broke, twice over.** The #228 re-review at `c1417359` showed the maax
|
||
parser still failing open. On the real `ci.yml`, two changes each turned
|
||
exit 1 into exit 0:
|
||
- a `GITHUB_STEP_SUMMARY` heredoc holding a bare `./scripts/checkers.sh`;
|
||
- folding the kb step as `run: >`.
|
||
|
||
That is because `run:` values were read one line at a time. `"--command" in
|
||
words` also matched any command at all. And step and job `if:` were ignored
|
||
(tvte). Each round of parsing had been answered by another shell or YAML form.
|
||
|
||
**The structural fix.** `checkers.sh` owns a job→suites table, and each CI job
|
||
makes one call, `checkers.sh --ci <job>`. The gate parses no shell. It counts a
|
||
step only when all of these hold:
|
||
- the step sits at step-key indentation inside `steps:`;
|
||
- its whole `run:` value is that call;
|
||
- the call names its own job;
|
||
- the step carries no `if:`, `continue-on-error:`, `shell:` or covering
|
||
`defaults:`.
|
||
|
||
Every other line mentioning `scripts/checkers.sh` is reported. A key with an
|
||
inline value owns deeper lines, and an open quote owns lines until it closes,
|
||
so content is never read as structure.
|
||
|
||
**The reviewer's attacks replayed on the real `ci.yml`** (kb step mutated,
|
||
restored after each):
|
||
- the heredoc: exit 1, naming the line and the seven unrun suites;
|
||
- `run: >`: exit 1, the same;
|
||
- `if:` on the kb step: exit 1, "conditional (if)".
|
||
|
||
**Self-test: 19 suite lists.** Every case asks the real `checkers.sh` table.
|
||
|
||
**Rivals, each a source edit loaded and run against the real `--self-test`,
|
||
each failing a case only it can:**
|
||
- **No quote tracking:** fails "an exact run: line inside a multi-line quoted
|
||
value does not count" and nothing else. The first quoted case could not
|
||
isolate this rule, because its content line was not `run:`-shaped anyway.
|
||
It was replaced.
|
||
- **No owner rule:** fails "a whole job inside a top-level block scalar does not
|
||
count".
|
||
- **Disqualifiers ignored:** fails the step `if:`, job `if:`, job `if:` after
|
||
its steps, `continue-on-error` and both `defaults:` cases.
|
||
- **Any job may run any row:** fails "a job running another job's row is
|
||
refused".
|
||
- **The call may be embedded in other shell:** fails "shell control flow around
|
||
the call does not count".
|
||
- **Stray mentions not reported:** fails the nine cases that expect a line to
|
||
be named.
|
||
- **Steps counted outside `steps:`:** fails "a step-shaped item under a job key
|
||
other than steps: does not count". The first form of this rival passed,
|
||
because it changed only where a step starts while the step-key match still
|
||
required `steps:`. The rival was rewritten to treat every job key with an
|
||
empty value as opening steps.
|
||
|
||
**`checkers-test.sh` gains five `--ci` cases.** The rival that drops "`--ci`
|
||
takes a job and nothing else" fails two of them, `--ci` beside a suite name
|
||
and beside `--self-test`, each exit 0 where 4 was wanted.
|
||
|
||
**Stated, not guarded:** a step earlier in the same job that rewrites
|
||
`scripts/checkers.sh` or its table (`context/code/tooling.md`).
|
||
|
||
### A continued run: and a skipped need do not count — `scripts/check-ci-graph.py` (improvise-1be2, improvise-bbwa)
|
||
|
||
**What broke (1be2, P2, fail-open).** The gate matched `run:` against its own
|
||
physical line only. A plain YAML scalar folds a deeper-indented next line into
|
||
the value, so a kb step whose `run:` continued onto a line reading `|| true`
|
||
ran `./scripts/checkers.sh --ci kb || true`. It counted as the exact call, and
|
||
the gate reported "all 9 suites run". A failing suite could then never fail CI.
|
||
|
||
**What broke (bbwa, P3).** The gate disqualified `if:` on the step and on its
|
||
own job, but not on a job it `needs:`. GitHub skips a job whose need was
|
||
skipped, so `if: false` on a needed job meant the call never ran, while the
|
||
gate counted it.
|
||
|
||
**Fix.** A `run:` whose value owns a following non-blank line is not counted,
|
||
and the step is named. `needs:` is read in all three forms (`[a, b]`, a
|
||
scalar, a block list) and walked transitively. A job-level `if:` anywhere in
|
||
that closure disqualifies the step as "conditional (needs K, which has if)".
|
||
The "no CI step runs" message now names the fix site: a row of `ci_suites` in
|
||
`scripts/checkers.sh`, not `ci.yml`.
|
||
|
||
**Red first.** The three new cases (a continuation line, `needs:` inline and
|
||
as a block list) each failed with `saw []`, which is the false pass.
|
||
|
||
**Self-test: 23 suite lists.** One of the 23 is the two-hop case, added after
|
||
the rival below survived the first 22.
|
||
|
||
**Rivals, each a source edit run against the real `--self-test`:**
|
||
- **The continuation is not marked:** fails the 1be2 case only.
|
||
- **No needs flag:** fails the three needs cases.
|
||
- **Block-list `needs:` not read:** fails "the same, with needs: written as a
|
||
list" only.
|
||
- **No transitive walk:** passed all 22 cases at first, because every needs
|
||
case was one hop. "The same, two needs: away" (`kb` needs `mid`, `mid` needs
|
||
`gate`) was added, and the rival now fails it alone.
|
||
|
||
**Stated, not guarded:** a needed job that *fails* also skips the job, and an
|
||
earlier `actions/checkout` with `ref:` replaces the tree
|
||
(`context/code/tooling.md`).
|
||
|
||
### The gate refuses a key or needs: it cannot read plainly — `scripts/check-ci-graph.py` (improvise-w0j4)
|
||
|
||
**What broke (P2, fail-open).** Found in the delta review of #228 at
|
||
`2aafff3b`. The bbwa fix read `needs:` names as raw text, and it recognised job
|
||
and step keys only in the bare `key:` form. The review put a `gate` job with
|
||
`if: false` in the real `ci.yml` and gave `kb` a `needs:` on it. Every one of
|
||
these spellings still exited 0 with "all 9 suites run":
|
||
- `needs: ["gate"]`, `needs: 'gate'`, and a block item `- "gate"`;
|
||
- `needs: &d [gate]`, and `needs: &d` followed by a block list;
|
||
- a flow list split over two lines.
|
||
|
||
yq read `kb` as needing `gate` in every case. The disqualifier keys had the
|
||
same gap: `"if":`, `'continue-on-error':`, `if :`, and `? if` then `: false`.
|
||
|
||
**Fix: the review's suggestion, refuse rather than parse.** Every structural
|
||
line inside `jobs:` must be one plain form. That means a job name, a job key, a
|
||
step key, a `needs:` value that is empty, one bare name or a one-line flow list
|
||
of bare names, or a bare block item. Top-level keys must be plain everywhere.
|
||
Anything else is reported, naming the line, and the gate fails.
|
||
|
||
**Red first.** Eleven refusal cases were added. All eleven failed with `saw []`
|
||
against `2aafff3b`, which is the false pass. Two controls, `needs: [ gate ] #
|
||
c` and `needs: gate`, still read `gate` and are conditional. Two more cases,
|
||
a quoted job name and a quoted top-level key, were added with the fix.
|
||
|
||
**Replayed on the real `ci.yml`.** Each attack was mutated in, run, and
|
||
restored, with a byte comparison after the last. All eleven spellings now exit
|
||
1, and the plain `needs: [gate]` control exits 1 as conditional.
|
||
|
||
**The 1be2 boundary.** The earlier case put the continuation two columns past
|
||
the key. So a rival that marks multiline only past `owner + 1` passed all 23
|
||
cases. On the real `ci.yml`, a 9-space `|| true` under the kb step's `run:` at
|
||
column 8 then exited 0. The new case continues exactly one column past the
|
||
key. The review's rival fails it and nothing else. On the real `ci.yml` the
|
||
9-space continuation now exits 1.
|
||
|
||
**Rivals, each a source edit run against the real `--self-test` (39 cases):**
|
||
- `needs:` value accepted as anything: fails the five inline or anchored or
|
||
split `needs:` cases.
|
||
- Block items accepted as anything: fails the quoted-item case.
|
||
- Step keys not checked: fails the quoted step key.
|
||
- Job keys not checked: fails `"if":`, `'continue-on-error':`, `if :`, and `?`.
|
||
- Job names not checked: fails the quoted job name.
|
||
- Top-level keys not checked: fails the quoted top-level key.
|
||
- Refusals collected but not reported: fails all thirteen.
|
||
- The review's `owner + 1` rival: fails the boundary case alone.
|
||
|
||
**Stated, not guarded.** Content under job keys other than `steps:` and
|
||
`needs:` is not checked. Neither is anything deeper than a step's own keys.
|
||
The gate reads no structure there.
|
||
|
||
### A job's keys start at indent 4, or its lines are refused — `scripts/check-ci-graph.py` (improvise-rjaa)
|
||
|
||
**What broke (P2, fail-open).** Found in the delta review of #228 at
|
||
`3eafd4de`. w0j4's `_readable` accepted any line deeper than 4 outside `steps:`
|
||
and `needs:` as content. A *needed* job whose own keys sat at indent 5 or 6,
|
||
which is valid YAML, therefore had its `if:` and `needs:` read as content. The
|
||
review's three plants each exited 0 with `kb` skipped:
|
||
- a 6-space `gate` job, with `kb` given `needs: gate`;
|
||
- a 5-space `gate` job, with `kb` given `needs: [gate]`;
|
||
- a 6-space `mid` job holding `needs: gate`, with `kb` given `needs: mid`.
|
||
|
||
**Fix, as the review suggested.** A job's first key must be at indent 4. After
|
||
that, a line at indent 5 to 8 outside `steps:` and `needs:` is content only
|
||
once its job has a 4-space key. Such a line can only be the content of an
|
||
empty-valued key, because a key with an inline value already owns every deeper
|
||
line.
|
||
|
||
**Red first.** All three plants, as self-test cases, failed with `saw []`. A
|
||
control, `env:` then `FOO: bar` at indent 6, stays accepted as content.
|
||
|
||
**Replayed on the real `ci.yml`.** Each plant was mutated in, run, and
|
||
restored. All three now exit 1, naming the first misplaced line.
|
||
|
||
**Rivals, each run against the real `--self-test` (43 cases):**
|
||
- The pre-fix rule (any line past 4 is content): fails the three plants.
|
||
- `job_keyed` never reset at a new job: fails the same three. The previous
|
||
job's key would otherwise license the next job's misplaced lines.
|
||
|
||
**Refused loudly, not read:** an indentless block list, a block list at indent
|
||
8, and a quoted top-level key such as `"on":`. Each is valid YAML, and each is
|
||
named by line.
|
||
|
||
### A comment on a read line is refused, and the matchers share one string — `scripts/check-ci-graph.py` (improvise-j49r)
|
||
|
||
**What broke (P2, fail-open).** Found in the delta review of #228 at
|
||
`5738e8e0`. `_readable` checked the comment-stripped line, while `_JOB.match`
|
||
in both `ci_steps` and `read_graph` ran on the raw line. A header
|
||
` gate: # skipped on purpose` was therefore accepted and never opened. Its
|
||
`if: false` went to the previous job, and `read_graph` did not list `gate`.
|
||
Planted before ` wasm:` in the real `ci.yml`, with `kb` given `needs: gate`,
|
||
the gate exited 0: "pages-deploy needs all 7 other jobs; all 9 suites run".
|
||
|
||
**The audit found a second instance.** The stripper cut `A: "x # y"` to
|
||
`A: "x`, so `_opens_quote` held the following lines open, and a job-level
|
||
`if: false` after them was never read. Planted under `kb` in the real
|
||
`ci.yml`, this also exited 0 at `5738e8e0`.
|
||
|
||
**Fix.** A line containing ` #` is refused, with its own message, and the
|
||
stripper is deleted, so every matcher reads the raw line. `read_graph` opens a
|
||
job on `_JOB_OPEN` (` name:` followed by whitespace or the end of the line).
|
||
That is wider than `_JOB`, because it refuses nothing. The argument is in
|
||
`context/code/tooling.md`.
|
||
|
||
**Red first** against `5738e8e0`:
|
||
- the suite case for the header: `saw []`;
|
||
- the `read_graph` case for the header: `saw []`;
|
||
- the `"x # y"` case at indent 10: `saw []`.
|
||
|
||
The `"x" # note` case read correctly at `5738e8e0`, because the stripper cut it
|
||
cleanly. It guards the alternative of reading raw lines without refusing: under
|
||
that alternative the raw value `"x" # note` opens the quote tracker. Two old
|
||
cases were changed. The positive "own row" case lost its trailing comment, and
|
||
that form is now its own refusal case. The w0j4 padded-list control lost its
|
||
`# c` and is still a control.
|
||
|
||
**Replayed on the real `ci.yml`.** Each plant was mutated in, run and
|
||
restored, and the file was compared byte for byte (sha1 `a00288a8…`). Four
|
||
plants were run: j49r as reviewed, j49r with `gate` also in pages-deploy's
|
||
`needs`, `"x # y"` under `kb`, and `"x" # note` under `kb`. At `5738e8e0` the
|
||
first and third exited 0. Now all four exit 1 and name the line.
|
||
|
||
**Rivals, each a source edit run against the real `--self-test` (47 cases).**
|
||
The review's eleven still fail where the review thread said they would. The
|
||
continuation rival fails two cases, the needs flag five, block-list one,
|
||
transitive one, flow-only two, `owner + 1` one, `if_jobs` kb-only five,
|
||
`_readable` always True fifteen, quoted `_NEEDS_VALUE` one, and each rjaa rival
|
||
three. Four rivals are new:
|
||
- Raw lines, no refusal: fails the run-line, `"x" # note` and `"x # y"` cases.
|
||
- The stripped line everywhere, no refusal (the other option): fails all four
|
||
suite cases.
|
||
- Refusal only inside `jobs:` at indent 8 or less: fails the indent-10
|
||
`"x # y"` case alone.
|
||
- `read_graph` opening on the exact `_JOB`: fails the `read_graph` case alone.
|
||
|
||
The header suite case does not separate the raw-without-refusal rival. There
|
||
the exact `_JOB` on the raw line refuses the header anyway, and the complaint
|
||
has the same text.
|
||
|
||
### A multi-line quoted value is refused, and the quote tracker is gone — `scripts/check-ci-graph.py` (improvise-6zf1)
|
||
|
||
**What broke (P2, fail-open, predates j49r).** Found in the delta review of #228
|
||
at `588094b7`. The tracker closed a multi-line quoted value at the first line
|
||
containing its quote character, including an escaped `\"` or a doubled `''`,
|
||
which YAML does not treat as a close. A later content line such as `b: "` then
|
||
opened a phantom string that swallowed the job's real `if: false`. The review
|
||
planted a `gate` job whose `name:` spans lines, with `kb` given `needs: gate`
|
||
and `gate` added to pages-deploy's `needs`. yq-go read `gate.if == false`, and
|
||
the gate exited 0 in both quote styles.
|
||
|
||
**Fix.** A line whose quoted value does not close on its own line is refused,
|
||
with its own message. The tracker is deleted. The real `ci.yml` has no
|
||
multi-line quoted value (scanned with `_ANY_KEY` and `_opens_quote`: 0).
|
||
|
||
**Red first** against `588094b7`: both plant cases (double and single) gave
|
||
`saw []`.
|
||
|
||
**New cases, and why each is there.**
|
||
- The two plants. Each now draws the refusal, and also "conditional (needs
|
||
gate, which has if)", because with the tracker gone the job's `if:` is read.
|
||
- An opening line ending in an escaped `\"`, and one ending in a doubled `''`,
|
||
each followed by `needs: ok` and a readable closing line (`x: y"`). yq-go
|
||
reads each as one string, with `kb.needs == "gate"`. If `_opens_quote`
|
||
called either value closed, nothing would be refused, and the gate would
|
||
read `needs: ok` in place of kb's real need. These cases are what make
|
||
`_opens_quote`'s escape handling load-bearing and tested.
|
||
- Two refused styles the review listed: a trailing comment on a step, and
|
||
`run: echo "a # b"`. Both were already refused by the j49r rule, so these
|
||
cases document behaviour rather than fix a bug. The third style, a quoted
|
||
`with:` value containing ` #`, was already the j49r indent-10 case.
|
||
|
||
**Changed cases.**
|
||
- "an exact run: line inside a multi-line quoted value does not count" is now
|
||
"a run: line inside a multi-line quoted value: the quote is refused". It
|
||
draws the refusal of the opening line and a plain refusal of the closing
|
||
`b"`. Only the exit is guaranteed after a refused line.
|
||
- "a closed quote followed by a comment is refused (j49r)" now reads the
|
||
job-level `if:` correctly. Its expectation is the refusal plus "conditional
|
||
(job if)", which is what the j49r evidence wanted but could not have while
|
||
the tracker existed.
|
||
|
||
**Replayed on the real `ci.yml`.** I planted the review's three shapes: double
|
||
quote, single quote, and double quote with the reopening line at indent 4. At
|
||
`588094b7` each exited 0. Now each exits 1, naming the opening line. All seven
|
||
plants from both rounds were restored byte for byte (sha1 `a00288a8…`).
|
||
|
||
**Rivals, each a source edit run against the real `--self-test` (53 cases).**
|
||
All 18 fail:
|
||
- the review's eleven;
|
||
- the four from j49r;
|
||
- three new ones:
|
||
- a multi-line quote is not refused: fails the five 6zf1 cases;
|
||
- `_opens_quote` ignores `\"`: fails the escaped-`\"` case alone;
|
||
- `_opens_quote` ignores `''`: fails the doubled-`''` case alone.
|
||
|
||
The j49r rival "no comment refusal" now fails four cases, not three: the
|
||
`"x" # note` case is also caught by the quote refusal, because the raw value
|
||
`"x" # note` does not end in its quote.
|
||
|
||
**Checked, not a finding.** A flow value (`{…}` or `[…]`) that continues onto
|
||
another line is not refused. yq-go rejects a continuation at a lesser
|
||
indentation (`did not find expected ',' or '}'`), which is loud. At the key's
|
||
own indentation the continuation is read as a key, which fails closed: a
|
||
spurious `if:`, or a `needs:` value the exact `_NEEDS_VALUE` refuses.
|
||
|
||
### Every value is one of a few one-line shapes — `scripts/check-ci-graph.py` (improvise-0wn3)
|
||
|
||
**What broke (P2, fail-open).** Found in the delta review of #228 at
|
||
`9dc8a643`. A multi-line flow value was not refused:
|
||
|
||
```
|
||
- name: [a, "b
|
||
run: nix shell nixpkgs#trealla --command ./scripts/checkers.sh --ci kb
|
||
c: x"]
|
||
```
|
||
|
||
go-yaml, PyYAML 6.0.3 and Psych 5.2.2 read this step as a single `name` string
|
||
with no `run:`. The gate read the middle line as the step's `run:` and counted
|
||
it. The single-quoted form and a flow map did the same. All three exited 0 on
|
||
the real `ci.yml`.
|
||
|
||
**This retracts the 6zf1 entry's "Checked, not a finding" note above.** That
|
||
note argued a flow continuation "fails closed", but it only considered closing
|
||
lines that are not spelled like a step key.
|
||
|
||
**Fix: an allowlist over values, not one more refused construct.** Every value
|
||
the gate reads must be one of: empty; a block-scalar header; a quoted scalar
|
||
closed on its line; a one-line flow with no quotes, nesting or `#`; or a plain
|
||
scalar not starting with a YAML indicator. That covers a key's value, a list
|
||
item's, or a line of its own. Anything else is refused. The argument is in
|
||
`context/code/tooling.md`.
|
||
|
||
The real `ci.yml` has 153 plain values, 41 empty, 2 one-line flows and 5 block
|
||
headers, and none is refused. When a line is both an unreadable key and an
|
||
unreadable value, the key refusal's message wins, so w0j4's cases keep their
|
||
diagnosis.
|
||
|
||
**Red first** against `9dc8a643`. All seven new cases gave `saw []`:
|
||
- the review's three: `[` double-quoted, `[` single-quoted, `{`;
|
||
- `[a, "b]`, which ends in its closer with its quote still open. This is what
|
||
the review's proposed fix ("does not close on the same line") would pass if
|
||
it were read as "ends in `]`";
|
||
- a flow opened on a line of its own, under an empty `name:`. yq-go reads it
|
||
as one string too;
|
||
- an anchored flow (`&a [x, "b`) and a tagged one (`!!seq [x, "b`), each
|
||
confirmed with yq-go to swallow the `run:`.
|
||
|
||
**Replayed on the real `ci.yml`.** I replaced kb's checkers step with each of
|
||
the review's three plants. At `9dc8a643` all three exit 0. Now all three exit
|
||
1, naming the opening line. All ten plants from the four rounds exit 1, and
|
||
the file was restored byte for byte (sha1 `a00288a8…`).
|
||
|
||
**Rivals, each a source edit run against the real `--self-test` (60 cases).**
|
||
All 22 fail. The review's sixteen are my 01–11, the comment rival, the
|
||
`read_graph` rival, the old quote-refusal rival (now "value allowlist removed",
|
||
which fails 12 cases), and both `_opens_quote` rivals. The four new ones:
|
||
- a flow need only end in its closer: fails `[a, "b]` alone;
|
||
- any flow start allowed: fails the five flow cases;
|
||
- only a key's value is checked: fails the own-line flow case alone;
|
||
- anchors and tags read as plain: fails the anchored and tagged cases.
|
||
|
||
**A valid style newly refused.** A quoted key below the `_readable` depth, such
|
||
as `"FOO": bar` under `env:`, is refused, because the gate cannot tell it from
|
||
an unclosed quote. Checked directly: refused, and the plain `FOO: bar` control
|
||
is accepted.
|
||
|
||
### A line break YAML knows and the gate does not is refused — `scripts/check-ci-graph.py` (improvise-xpzh)
|
||
|
||
**What broke (P2, fail-open).** Found in the delta review of #228 at
|
||
`7ae871cb`. The gate splits on `\n` only, but YAML also breaks lines at U+0085,
|
||
U+2028 and U+2029. `runs-on: ubuntu-latest<U+2028> if: false` on the real
|
||
`kb` job is one line to the gate and two to go-yaml. yq-go reads
|
||
`kb.if == false` with each of the three characters, and the gate exited 0.
|
||
|
||
**Fix.** `_UNSAFE_CHAR` is checked on every line, before the comment and owner
|
||
skips. It covers C0 except tab, DEL, all of C1 (which holds U+0085), U+2028 and
|
||
U+2029. The workflow is read through `read_workflow`, whose universal newlines
|
||
fold CR and CRLF. The argument, including why all non-ASCII is not refused, is
|
||
in `context/code/tooling.md`. The real `ci.yml` has 0 CR bytes, no BOM and 27
|
||
printable non-ASCII characters, none of them in the refused set.
|
||
|
||
**Red first** against `7ae871cb`. All five new suite cases gave `saw []`:
|
||
- one per break character, each on a `runs-on:` line;
|
||
- a U+2028 inside a full-line comment;
|
||
- a U+2028 inside a `run: |` body, on the job's last step. yq-go ends the
|
||
block there and reads the `if:` as a job key.
|
||
|
||
**Controls.** Two cases write a real file and read it back through
|
||
`read_workflow`:
|
||
- CRLF line ends are accepted with no complaint;
|
||
- a lone CR is a line break, so the following `if:` makes kb conditional, as
|
||
YAML reads it.
|
||
|
||
**Replayed on the real `ci.yml`.** The three plants, one per character, on
|
||
kb's `runs-on:`, exit 0 at `7ae871cb`. Now all three exit 1, naming line 255.
|
||
All 13 plants from the five rounds exit 1. The file was written with
|
||
`newline=""` so the characters landed literally, and it was restored byte for
|
||
byte (sha1 `a00288a8…`).
|
||
|
||
**Rivals, each a source edit run against the real `--self-test` (65 suite
|
||
lists, 2 files).** All 27 fail. The 22 from earlier rounds fail as before.
|
||
Five are new:
|
||
|
||
| Rival | Fails |
|
||
|---|---|
|
||
| The character check removed | the five new cases |
|
||
| C1 allowed | the U+0085 case alone |
|
||
| Comments not checked | the comment case alone |
|
||
| Owned content not checked | the block-scalar case, and the comment case |
|
||
| The read does not fold CR (`newline=""`) | both file controls |
|
||
|
||
The owned-content rival also fails the comment case because `owner` still
|
||
holds the preceding `runs-on:` key's column when that line is checked.
|
||
|
||
### Two checks of the harness lock, not one — `ci.yml` (improvise-dndu)
|
||
|
||
The CI graph runs two different harness-lock checks, and neither stands in for
|
||
the other:
|
||
- **the kb row of `ci_suites`** runs `check-harness-lock-test.sh`, the
|
||
*self-test*. It pins the script's three answers against path-only fixture
|
||
workspaces. It says the script *can* tell stale from current from "did not
|
||
run". It never looks at the real harness lock.
|
||
- **the `test` job's step** "The harness lock matches the manifests it
|
||
resolves against" runs `check-harness-lock.sh --allow-network` against the
|
||
real `harness/opfs-contract` manifest. This is the check that actually fails
|
||
a stale lock.
|
||
|
||
The recompose of #245 onto #228 dropped the second while keeping the first.
|
||
The merge resolved the `ci.yml` conflict with `git checkout --theirs`, which
|
||
takes the other side's **whole file**, not just the conflicting hunk. So the
|
||
network step, which was not part of the conflict, went with it. Nothing
|
||
noticed:
|
||
- `check-ci-graph` only asks that every `checkers.sh` suite has a CI row, and
|
||
the self-test still had one;
|
||
- every other gate is local, and the lock was current.
|
||
|
||
The merge commit's message even said the step was unchanged. Review and Codex
|
||
both caught it: `git diff 1bec7c0f f41a946b -- .github/workflows/ci.yml` was
|
||
empty, so #245 no longer changed `ci.yml` at all.
|
||
|
||
The repair restores the four lines at their original place, after the
|
||
formatting step. The check that would have caught the loss is the one the
|
||
repair used: diff the resolved file against **each** parent, and against the
|
||
merge base, then confirm every change one side made to the base survives.
|
||
Against #245's head, the restored `ci.yml` differs only where #228 moved the
|
||
kb step's suite list into its table.
|
||
|
||
### A `defaults: run: shell:` no longer disqualifies a step — `scripts/check-ci-graph.py` (the user's ruling on #228)
|
||
|
||
**What changed, and why.** The gate refused to count any step under a
|
||
workflow- or job-level `defaults:` block. The user ruled it "a silly check".
|
||
It also broke on contact with main: PR #287 added a workflow-level
|
||
`defaults: run: shell: nix develop .#ci --command bash …`, and on the merged
|
||
tree every suite read as run by no CI step. The refusal is removed.
|
||
|
||
**Cases.** Two cases pinned the refusal: "a job's defaults: can change the
|
||
shell, so it does not count" and "workflow-level defaults: disqualifies every
|
||
step". Both are deleted. The new case "a step under a defaults: run: shell: is
|
||
counted" carries both a workflow-level default in #287's shape and a
|
||
job-level one, and expects no complaint.
|
||
|
||
**Red first.** Against the code before the removal, the new case drew both
|
||
steps as "conditional (workflow defaults)" and kb's as "(job defaults, workflow
|
||
defaults)", so both rows went unrun.
|
||
|
||
**Kept.** A step's own `shell:` still disqualifies it; the ruling did not cover
|
||
that. The w0j4 case refusing a quoted `"defaults":` key is about key spelling,
|
||
and it stands.
|
||
|
||
### 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.
|
||
|
||
### Deleted means git will record it — `scripts/bd-audit-ids-test.sh` (improvise-cota)
|
||
|
||
**What broke.** The tdfd fix above asked `git ls-files -d` whether an absent path
|
||
was deleted. `ls-files -d` also lists an absent path marked
|
||
`--assume-unchanged`. But `git status` shows nothing for that path, and
|
||
`commit -a` would never record the deletion, so its citations stay in `HEAD`.
|
||
The audit skipped the file and exited 0. This is the tdfd shape again: absent,
|
||
not a deletion git will record, and never read. It was found in the delta
|
||
review of PR #244 at `33d563a9`.
|
||
|
||
**Fix, the reviewer's oracle.** A path counts as deleted only when
|
||
`git diff --name-only --diff-filter=D` lists it. That compares the index
|
||
with the working tree and honours both index flags: it hides a
|
||
skip-worktree path and an assume-unchanged path alike, and lists a plain
|
||
unstaged delete. A staged `git rm` never reaches the question, because the path
|
||
has already left the index that `ls-files --cached` enumerates.
|
||
|
||
**Red first.** "an assume-unchanged file missing from the tree is exit 4, and
|
||
named" exited 0 against `33d563a9`, reporting "all 1 cited ids resolve".
|
||
|
||
**Rivals.**
|
||
- The `ls-files -d` oracle, which is the red state above.
|
||
- Every absent path reported as unread (`elif true`). It fails "a tracked file
|
||
deleted from the working tree is not scanned" and nothing else, so the
|
||
deliberate skip of a real deletion survives the stricter oracle.
|
||
|
||
This rival was edited in place in the worktree script, not copied to `/tmp`, for
|
||
the reason the tdfd entry gives.
|
||
|
||
### 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` comes from `type -P`, before any case puts its stub
|
||
`sleep` first on PATH. `type -P` returns a path even if `sleep` has been loaded
|
||
as a builtin. The fake clock's `/bin/date` pass-through was deleted rather than
|
||
re-pointed. `pr-reviews` only ever calls `date +%s`, so the branch never ran
|
||
(found by Copilot on #297). The fake now refuses any other arguments with
|
||
exit 2, so a new `date` call in `pr-reviews` shows up as a failure instead of
|
||
untested code.
|
||
|
||
**Rivals:**
|
||
- **`REAL_SLEEP=/nonexistent/sleep`** fails the whole suite, 0 of 25, so the
|
||
resolved path is the one the watchdog runs.
|
||
- **`pr-reviews` changed to call `date -u +%s`** fails "a review arriving on
|
||
the third poll is seen" and "--wait keeps polling until its deadline" (23 of
|
||
25 pass). The fake's refusal is therefore visible, not swallowed.
|
||
|
||
**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`.
|
||
|
||
### A thread past ARG_MAX is read from stdin — `scripts/pr-reviews-test.sh` (improvise-5jxt)
|
||
|
||
**What broke.** `fetch` handed the review, inline-comment and issue-comment
|
||
JSON to one `jq -n --argjson …` call as arguments. Once #210's comments
|
||
outgrew the kernel's argument limit, `execve` refused to start jq:
|
||
`jq: Argument list too long`, exit 126, with nothing printed. That happened
|
||
with or without `--wait`. The script is how agents read reviews, so an ADR
|
||
steward was blocked on it.
|
||
|
||
**Fix.** A `combine` helper sends the three documents to `jq -s` on stdin
|
||
through `printf`. `printf` is a bash builtin, so no `execve` sees the data.
|
||
Both paths use `combine`: the fixture path and the live `gh` path. The other
|
||
jq calls in `pr-reviews` already read `$json` from a pipe. `--arg head` carries
|
||
only a 40-character SHA.
|
||
|
||
**The fixture.** Each of the three surfaces carries about 2 MB: a review body,
|
||
an inline comment followed by a small one after it, and an issue comment. 2 MB
|
||
exceeds both macOS's total `ARG_MAX` (1 MB) and Linux's per-argument
|
||
`MAX_ARG_STRLEN` (128 KB). Every surface is large, so a fix that moves only one
|
||
document off argv still fails. The file is built with `head -c`, `tr` and
|
||
`printf`, so it depends on no tool version and no path.
|
||
|
||
**Red first.** Before the fix, "a thread larger than ARG_MAX is still read" and
|
||
"… still satisfies --wait" exited 126, as #210 did live.
|
||
|
||
**Rivals** (each 25 passed, 5 failed; the five are the new cases):
|
||
- **The pre-fix script**, restored from `gh/main`: exit 126 on both run cases,
|
||
and all three print assertions miss.
|
||
- **A partial fix** that pipes the review and inline documents but still
|
||
passes the issue comments with `--argjson issue "$3"`: also exit 126. So the
|
||
large issue fixture is load-bearing, not decoration.
|
||
|
||
**Live.** `./scripts/pr-reviews 210` exits 0 with its output sent to a file.
|
||
The output includes the user's inline answers on
|
||
`context/adr/007-stable-identities.md` from 2026-09-24T19:50:22Z onward
|
||
("full unicode", "Just a plane number.", …).
|
||
|
||
**Scope sweep.** `scripts/pr-watch` passes its JSON through `printf '%s' | jq`,
|
||
never as an argument. `grep -rn 'argjson\|--arg '` over `scripts/`,
|
||
`.agents/` and `.claude/skills/` finds no other data argument.
|
||
|
||
### An agent authors its commits, and the committer is left alone — `scripts/agent-git-test.sh` (improvise-1xfb.3)
|
||
|
||
**What was missing.** Every agent commit was authored as the repository owner,
|
||
so history could not say which agent wrote what. `scripts/agent-git` runs one
|
||
git command with `GIT_AUTHOR_NAME="Claude ($BEADS_ACTOR)"` and refuses, exit 2,
|
||
when `BEADS_ACTOR` is unset or empty.
|
||
|
||
**Why not `git -c user.name=…`,** the mechanism the ticket and the brief both
|
||
offered first. `user.name` is the default for the committer as well as the
|
||
author, so it renames both. The ticket asks for the committer to be left
|
||
alone. Probed in a scratch repo: a `-c user.name` commit reads
|
||
`Claude (u) <o@x> | Claude (u)` as author and committer. A `GIT_AUTHOR_NAME`
|
||
commit reads `Claude (t) <o@x> | Owner`. The same probe showed that a merge
|
||
takes `GIT_AUTHOR_NAME` too, and that a plain `--amend` keeps the author it
|
||
amends.
|
||
|
||
**A cherry-pick keeps its source's author** (Codex P2 and the review on PR
|
||
#310). The first version of the rule listed `cherry-pick` among the wrapper's
|
||
subcommands, but git ignores `GIT_AUTHOR_NAME` there. That is the behaviour
|
||
this repo wants, since rule 8 allows only `cherry-pick -x` of commits that keep
|
||
their own author. So the contract names only `commit` and `merge`. The case "a
|
||
cherry-pick keeps its source's author" picks a commit authored `Claude
|
||
(agent-b2)` with the wrapper set to `agent-a1`, and asserts `agent-b2`. A pick
|
||
that failed would leave the owner-authored base at HEAD and fail the case too.
|
||
|
||
**Rivals** (run against the first eleven cases).
|
||
- **`exec git -c user.name="Claude ($BEADS_ACTOR)" "$@"`**: 10 passed,
|
||
1 failed, "the committer is untouched" (got `Claude (agent-a1)`).
|
||
- **The refusal removed** (`if false`): 7 passed, 4 failed. They are "an unset
|
||
BEADS_ACTOR is refused" (exit 1 from `set -u`, not 2), "the refusal names
|
||
BEADS_ACTOR", "an empty BEADS_ACTOR is refused" (exit 0: the commit landed
|
||
under the name `Claude ()`), and "a refusal commits nothing". So `set -u`
|
||
alone would not cover the empty case.
|
||
|
||
**Fixture.** The repo's identity comes from its own config, with
|
||
`GIT_CONFIG_GLOBAL=/dev/null` and `GIT_CONFIG_NOSYSTEM=1`, and every `GIT_*`
|
||
identity variable is unset first. So a caller's own identity cannot make the
|
||
author or committer assertions agree by accident.
|
||
|
||
### The repo-stats pipeline computes what it claims to — `scripts/repo-stats-test.sh` (improvise-1xfb.5.1)
|
||
|
||
**What it pins.** The fixture repository has one commit before the window, a
|
||
side branch merged with `--no-ff`, a shrink in the next ISO week, and one file
|
||
in every area. So each number the report prints is known by construction. The
|
||
suite runs the real pipeline, code-maat and improvise included, and asserts
|
||
improvise's exported tables: each area's added, deleted and deletion ratio;
|
||
prose, code and the `ProseToCode` row; the week split; and the per-day totals.
|
||
It also checks the doc sizes and the coupling flags, and that an improvise that
|
||
refuses or warns makes the pipeline exit 3.
|
||
|
||
**Two fixture choices are load-bearing.**
|
||
- The repo map's `4.` item sits after the `---` that closes the Gotchas
|
||
section, with no `##` heading between them. In an earlier fixture a `##`
|
||
heading followed the `---`, so the `---` rule could be deleted and the suite
|
||
stayed green.
|
||
- `churn.csv` must equal code-maat's `abs-churn` on every day. That compares
|
||
two independent derivations of one number, one of them improvise's
|
||
aggregation. The fixture has a binary-free, rename-free history, so the two
|
||
agree exactly.
|
||
|
||
**Rivals**, each injected into the script or into `repo-stats.improvise` and run
|
||
through `checkers.sh repo-stats`:
|
||
- Rust src matched before Rust tests: the rust src and rust tests rows fail.
|
||
- `DeletionRatio = deleted / (added + deleted)`: six ratio rows fail.
|
||
- `ProseToCode = code / prose`: the `ProseToCode` row fails.
|
||
- `--no-merges` dropped: the commit count fails (6, wanted 5).
|
||
- The `---` end of the Gotchas section ignored: the gotcha count fails (4, wanted 3).
|
||
- A crate crossing as "the crates differ", so a crate against no crate counts:
|
||
the crate-crossing case fails.
|
||
- `kb.pl` counted as prose: the prose row and the ratio fail.
|
||
- The category padding removed: the import skipped every record, and both
|
||
pipeline runs failed with exit 3. That rival is retired: folder import
|
||
(improvise-lpzu) keeps sparse records, and the padding is gone. The suite
|
||
passes 41 of 41 without it.
|
||
- Months instead of ISO weeks: both week cases fail.
|
||
- A `warning:` from improvise ignored: the warning-stub case fails.
|
||
|
||
### A window of empty commits, and four faults the first fixture missed — `scripts/repo-stats-test.sh` (improvise-1xfb.5.1)
|
||
|
||
**What broke.** ChatGPT Codex found it on PR #313, and the adversarial review
|
||
reproduced it. When every non-merge commit in the window is `--allow-empty`,
|
||
the log has commit headers and no files. `build_dir` took `churn.csv`'s header
|
||
from `rows[0]`, raised `IndexError` and exited 1. That exit is outside the
|
||
pipeline's failure split: it is neither a number nor improvise's exit 3.
|
||
|
||
**Fix.** `churn.csv` has a fixed header. When no file changed, the script
|
||
skips code-maat and improvise, writes the header alone, and exits 0.
|
||
|
||
**Red first.** The new case is a second fixture repository whose only
|
||
in-window commit is empty. Restoring the `rows[0]` header fails its run case
|
||
with the traceback.
|
||
|
||
**The review's four surviving faults, each now killed:**
|
||
- **`--no-renames` dropped.** The fixture now renames `scripts/foo.sh` to
|
||
`scripts/bar.sh`. Without the flag git reports one `{foo.sh => bar.sh}` entity
|
||
with no lines changed, not a delete and an add. Four cases fail: the scripts/CI
|
||
row, the code row, the ratio row and the coupling count.
|
||
- **`crate_of` forgetting the root crate.** `src/main.rs` now co-changes with
|
||
both crates. The crossing list loses three pairs.
|
||
- **A failing `script` call ignored.** A third stub exits 0 for `import` and
|
||
refuses `script`. The pipeline then reads exports that were never written and
|
||
exits 1, not 3.
|
||
- **`.beads/` and `site/` left out of generated.** Both are in the fixture now.
|
||
The generated row and the other row both fail.
|
||
|
||
**Also counted now:** `docs/guide.md` as other docs and `.codex/config.toml` as
|
||
agent docs. The coupling count is 16, not 15: `web/app.js` and
|
||
`scripts/foo.sh` change together in two commits, the 06-04 one and the rename.
|
||
|
||
### The pipeline on folder import — `scripts/repo-stats-test.sh` (improvise-1xfb.5.1)
|
||
|
||
Once #314 merged, `improvise import <dir>` replaced the file list, the `-`
|
||
padding and the generated per-file views. The suite needed no change of
|
||
expectations and still passed 41 of 41. That shows only that the numbers
|
||
agree. It does **not** show that the model is right: the suite could not tell
|
||
sparse from padded. The delta review re-added `pad_categories` and the suite
|
||
still passed 41 of 41, while the model then had a `|-|` item in 13 places and
|
||
the `churn` view seated `agent-rules` on rows and paged everything else. The
|
||
view case counted 10 views either way.
|
||
|
||
**What separates them now:**
|
||
- **"the sparse model has no padding item"**: no line of the model names `|-|`.
|
||
- **"the churn view seats …"**: four cases, one per seat. `date` is on rows, and
|
||
`agent-rules`, `coupled` and `crosses-crate`, which are coupling-only, are on
|
||
`none`. The churn view puts `entity` on columns, not `area` as the review
|
||
guessed. What discriminates is `date` on rows and the coupling-only
|
||
categories on `none`, which a padded CSV cannot give, because it claims all
|
||
nine categories.
|
||
|
||
**Rivals:**
|
||
- **Padding re-added** (the review's own injection): 5 cases fail. They are
|
||
the `|-|` case, with 161 lines naming it in the fixture model, and all four
|
||
seat cases.
|
||
- **A report view named like a file stem**: `By day` renamed to `churn` leaves
|
||
9 views, and the view-count case fails.
|
||
- **The foreign-CSV guard removed**: "a foreign CSV in --out is refused with
|
||
exit 2" fails with exit 0. The guard is the answer to the review's
|
||
non-blocking finding that `import .` picks up any CSV in `--out`. It refuses
|
||
instead of clearing. Deleting files in a directory the caller named is the
|
||
wrong default, and the script's own five CSVs are overwritten on every run,
|
||
so a rerun into the same `--out` is still accepted. The same fixture run
|
||
writes JSON and then text into one directory, which shows that.
|