test: guard the wiring, not just the unit by dlovell · Pull Request #110 · xorq-labs/devcontainer

dlovell · 2026-08-04T16:57:34Z

Found by the structural audit of 88d16b4.

The #95/#96/#97 family — restate-and-compare, fails open — is fixed for
expectations. It survived for wiring: whether production actually
reaches the guarded unit, with the right argv, at the right stage. A suite can
be rung 2 on the fact and rung 3 on the call.

Three holes. Each was green before this change, mutation-proven on scratch
clones; the real tree was never modified.

#	Mutation	Before	Now
1	Delete `chown_named_volume_targets` from `setup()`	21 / 0, `run-all` exit 0	24 / 1 failed
2	Drop the `volume-perms` argv0 from the injection line	21 / 0, `run-all` exit 0	24 / 1 failed
3	`stages: [manual]` on the ADR-0003 hook	14 / 0	15 / 1 failed

1 — setup() need not call the chown at all. Every named-volume mount point
stays root-owned on every cold start, and tests/run-all exits 0. CLAUDE.md
invariant 24 cited this suite with no hedge. Fixed by extracting setup()'s
body (the awk '/^setup \{/,/^\}/' idiom test-classic-args-sync.sh
already uses) and asserting the call.

2 — the <argv0> contract was unguarded. sh -c <script> <argv0> <args...>
puts the first operand in $0, so dropping it swallows the first mount point,
which is then silently never chowned. The suite composed its own invocation
rather than reading the executed one — the same one-directional shape as #95,
where two suites restated ensure_nix_base()'s grep and neither read it. It now
reads the real line, asserts an argv0 is present, and feeds the extracted value
into its own invocation.

3 — the ADR-0003 gate could be switched off silently. With
stages: [manual], a commit carrying a broken live .gitignore went through
clean (rc 0); with the hook restored the same commit was blocked (rc 1).
The suite asserted entry: and always_run: by grep, which say nothing about
when the hook fires.

Checked hermetically, by parsing the hook block rather than invoking
pre-commit: pre-commit is not installed in the Bash-tests CI job (test.yml
is checkout + git identity + bash tests/run-all), so running it there would
either fail or skip — and a skip is the same "verification never ran" shape this
PR exists to remove.

Invariants 23 and 24 now state the wiring halves they were relying on.

Clean tree: tests/run-all 28 suites / exit 0; pre-commit run --all-files
clean. Mutations recorded in both suite headers.

The audit also suggests ADR-0005 may want to name this rung explicitly — its
rungs describe how an expectation is obtained, not whether the guarded unit is
reached from production. Not done here; worth a separate decision.

🤖 Generated with Claude Code

The #95/#96/#97 family — restate-and-compare, fails open — is fixed for EXPECTATIONS. It survived for WIRING: whether production actually reaches the guarded unit, with the right argv, at the right stage. A suite can be rung 2 on the fact and rung 3 on the call. Three holes, each mutation-proven green before this change. 1. setup() need not call the chown at all. Deleting chown_named_volume_targets from setup() left test-volume-chown-guard.sh at 21/0 and tests/run-all at exit 0, while every named-volume mount point stays root-owned on every cold start. Now the suite extracts setup()'s body and asserts the call. 2. The <argv0> injection contract was unguarded. `sh -c <script> <argv0> <args...>` puts the first operand in $0; drop it and the first mount point is swallowed and never chowned. The suite composed its own invocation instead of reading the executed one — the same one-directional shape as #95. It now reads the real line, asserts an argv0 is present, and uses the extracted value in its own invocation. 3. The ADR-0003 pre-commit gate could be moved off the commit stage. Adding `stages: [manual]` left test-gitignore-agents-reinclusion.sh at 14/0 while a commit with a broken live .gitignore went through clean (rc 0; rc 1 with the hook restored). The greps for `entry:` and `always_run:` say nothing about when the hook fires. Checked hermetically by parsing the hook block — pre-commit is not installed in the Bash-tests CI job, and skipping there would be the same "verification never ran" shape this fixes. Invariants 23 and 24 now state the wiring halves they relied on. Found by the structural audit of 88d16b4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The stage check shipped guarding only half the YAML it needed to. - `stages:` can be written flow-style (`stages: [manual]`) or block-style (`stages:` then indented `- manual`). The parse read only the flow form, so the block form produced an EMPTY match and took the "no stages: key" pass — the ADR-0003 gate still silently disabled with the suite green. That is the same fail-open shape this PR exists to remove, shipped inside its own fix. Now: no key -> pass; key with unparseable values -> FAIL closed; otherwise membership. All seven spellings verified. - `grep -q` on setup()'s body matched comment text, so `# chown_named_volume_targets (disabled)` — the most common way code gets turned off — passed at 25/0 with every named volume left root-owned. Anchored to a real call at line start. Also replaced a fixed `grep -A5` window for the always_run assertion. It was pre-existing and fails closed, but it produced a spurious FAIL whenever the hook block grew by two lines, which is exactly what testing the block-form stages does. It now reads the same extracted hook block as the stage check. Both mutations recorded in the suite headers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Third fail-open in this PR, same class as the first two, found by the third review round. The stage parser tokenized the whole line including YAML comments, so: stages: [manual] # run via pre-commit run --hook-stage manual passed at 16/0 with the ADR-0003 gate fully disabled — "pre-commit" from the COMMENT satisfied the membership test. That is not a contrived fixture: the natural thing to write when moving a hook to manual is a note saying how to invoke it, and that sentence contains the token. The block form failed the same way. Also `tr -d ','` glued `[pre-commit,manual]` — legitimate config that does fire on commit — into the single token `pre-commitmanual` and spuriously FAILed. Wrong direction to be dangerous, but it would misfire on a real edit. Both fixed in the same pipeline: strip `[[:space:]]#.*$` first, then map commas to newlines instead of deleting them. Ten spellings now verified and recorded in the header, up from seven. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Fourth round on this PR, and the first that addresses the cause rather than the instance. The structural fault is visible in the files themselves: each round anchored exactly the assertion a reviewer had demonstrated and left every sibling of the same kind untouched. test-gitignore-agents-reinclusion.sh :127 :133 anchored :171 :178 bare test-volume-chown-guard.sh :188 anchored :176 bare :171 and :176 are precisely the two holes the scoped audit then found live on this branch — the branch titled "guard the wiring, not just the unit" passed at 16/0 and 25/0 with the wiring commented out. :178 is the same class and had never been tested. So this adds tests/lib/shellsrc.sh — a normalizer for bash sources, the third such helper after tests/lib/dockerfile.sh (#87) and tests/lib/workflow-paths.sh (#93), each created by the PR that needed it — and migrates ALL THREE assertions, not the two that were demonstrated. assert_shell_wired strips comments before matching, so "a commented-out call is not wiring" holds by construction rather than by each author remembering an anchor. One bug worth recording: the first cut piped into `grep -q`, which exits on first match; sed then dies of SIGPIPE and pipefail reports 141, turning a successful match into a FAIL. Fail-closed, but wrong. It captures first now. Repo-wide migration of the other ~10 suites that parse bash sources is deliberately NOT here; that is the audit's separate disposal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ugh them Round 4 claimed to fix every sibling and did not. Review found two more, both green: a comment on the `stages:` KEY line (the parser's own sed required whitespace before the `#`, which the awk had already eaten), and the bare grep for the `dc exec ... sh -c "$script"` line — the ONE line that runs the injected script — which read the raw file and then extracted the argv0 out of comment text. That is the fifth and sixth instance of this class in this PR, so this round stops fixing instances. All four remaining raw reads in these two suites now go through shell_strip_comments, and setup_body goes through shell_function_body instead of a seventh hand-rolled range. There is one comment-stripper and one body extractor, and nothing in these files reads a source without them. shell_function_body moves here from #118, where it did not belong — the normalizer should be complete in the PR that introduces it. Also from review: - assert_shell_wired PASSED on an empty pattern (`grep -qF -- ""` matches every line) and treated a multi-line pattern as an OR. Both are what an extracted pattern looks like when its extraction broke. Both now fail. - The header claimed "a match means live code". It does not: it means the text is not in a comment. `if false; then ...; fi` and `echo "skipping <name>"` both satisfy these assertions with the wiring dead. Recorded as accepted limits of grep-over-source, with the alternative named. - The `ls-files -- .claude/agents` pin false-FAILed on an ordinary requote. Quoting-tolerant now — a guard that reddens on a no-op reformat trains people to edit the test. Both halves of the amended §2 pair now hold in both suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Live on main: `assert_contains "ensure_image" "$ensure_up_body"` matched comment text, so commenting out ensure_up()'s calls left the suite at 14 passed / 0 failed and `tests/run-all` at exit 0, with no image made present before `dc up`. Uses assert-on-stripped-source and the shared shell_function_body, both from tests/lib/shellsrc.sh (added by #110, extended here with the body extractor). The extractor matters independently: this file had TWO hand-rolled `sed -n '/^f()/,/^}$/p'` ranges, and reformatting `ensure_up()` to `ensure_up ()` — valid bash, shellcheck-clean — emptied them, turning assertions red for the wrong reason and making "dc_up no longer passes --build" vacuous. One spelling now, returning non-zero rather than empty when the function is absent. Correction to the audit that found this: its mutation commented ONE of two ensure_image call sites, so its stated consequence was overstated — the other site still called it, and the assertion was right to stay green. Commenting both is the honest demonstration and is what the header records. Stacked on #110 because assert_shell_wired/shell_function_body live there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…d it Two problems with the figure this amendment rests on, both mine. I took 19/12 from the audit and never re-ran it before writing it into an ADR — the same unverified-propagation that put a false claim into #112 and had to be caught in review. Re-running it now gives 24 blocks, and the cited command reports 12 of them as later-round when the true number is 16. The command grepped `review round|audit round|second|third`, and #110 went on to need a FOURTH and FIFTH round, which that pattern does not match. So the ADR shipped a measurement command that undercounts by three — a guard-adjacent measurement missing cases, which is precisely the defect the amendment is about. Now 24/16 (67%), counted on `round`, which is a clean discriminator: no first-time record uses the word, so it needs no ordinal list to keep current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

I removed a statistic that could not be re-derived and replaced it with a universal that its own examples falsify. Same defect, restated in prose. "Every fail-open since ADR-0005 LANDED was found AFTER the author's §2 run" is wrong for three of the five cases it then lists: test-completions-sync.sh (143bbf7), test-image-fingerprint.sh (fc2ca02) and test-nix-base-pin.sh (343632a) all predate dd18207, and none carried a mutation record at dd18207^ — their authors never ran §2 because §2 did not exist. The surviving claim is narrower and stronger: no fail-open here has been found by the guard's own author, and #93's and #110's guards DID carry recorded §2 runs and grew fail-opens anyway. One mutation aimed at the invariant is insufficient even when performed. Also: the paragraph quoted its own prior numbers inverted — "19 of 12" and "24 of 16", hits exceeding totals — in the passage whose subject is quoting your own numbers accurately. Now 12 of 19 and 16 of 24. Promoting the first amendment to `##` split the Decision section, orphaning `### 3. Derive over restate`. Moved after Alternatives considered, matching ADR-0002's placement, with a pointer under §2. CLAUDE.md's restatement had dropped "landed" (garden-pathing the sentence), dropped "or auditor" though most finds were the auditor's, and dropped the "say which invariant makes it so" clause that fences the byte-form carve-out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Third false "every sibling" claim on this PR, found the same way as the first two — by someone else looking. - tests/test-gitignore-agents-reinclusion.sh:206 still read the RAW config, twelve lines below the last assertion round 5 fixed, with the stripped copy already in scope. Commenting out the entire `shellcheck (extensionless)` hook left it PASSING while dev/check-gitignore-agents was linted by nothing. It now reads $_precommit_live in this shell — a `bash -c` subshell does not inherit it — and fails closed on a missing files: pattern. - The stage check failed closed on unreadable VALUES and open on unreadable KEYS: `"stages": [manual]` is honoured by pre-commit, passes yamllint, and was invisible to a bare `stages:` match. - `default_stages: [manual]` at the top level moves the hook off the commit stage from outside the block this suite parses, so the "no stages: key" branch passed while the gate was off. That branch now requires default_stages to be absent or commit-inclusive. shellsrc.sh's docstring still carried the "a match means live code" overclaim round 5 said it had removed — I fixed the file header and left the function's own. It also now records that bash starts a comment at ANY word-initial `#`, so `:;#real_call` is dead code the stripper leaves intact: a genuine false-pass direction, not the fail-closed truncation already documented. No completeness claim in this commit message. Six rounds is enough evidence that I cannot certify this by inspection; see #123 for the seam that would make the question decidable instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…d it Two problems with the figure this amendment rests on, both mine. I took 19/12 from the audit and never re-ran it before writing it into an ADR — the same unverified-propagation that put a false claim into #112 and had to be caught in review. Re-running it now gives 24 blocks, and the cited command reports 12 of them as later-round when the true number is 16. The command grepped `review round|audit round|second|third`, and #110 went on to need a FOURTH and FIFTH round, which that pattern does not match. So the ADR shipped a measurement command that undercounts by three — a guard-adjacent measurement missing cases, which is precisely the defect the amendment is about. Now 24/16 (67%), counted on `round`, which is a clean discriminator: no first-time record uses the word, so it needs no ordinal list to keep current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

I removed a statistic that could not be re-derived and replaced it with a universal that its own examples falsify. Same defect, restated in prose. "Every fail-open since ADR-0005 LANDED was found AFTER the author's §2 run" is wrong for three of the five cases it then lists: test-completions-sync.sh (143bbf7), test-image-fingerprint.sh (fc2ca02) and test-nix-base-pin.sh (343632a) all predate dd18207, and none carried a mutation record at dd18207^ — their authors never ran §2 because §2 did not exist. The surviving claim is narrower and stronger: no fail-open here has been found by the guard's own author, and #93's and #110's guards DID carry recorded §2 runs and grew fail-opens anyway. One mutation aimed at the invariant is insufficient even when performed. Also: the paragraph quoted its own prior numbers inverted — "19 of 12" and "24 of 16", hits exceeding totals — in the passage whose subject is quoting your own numbers accurately. Now 12 of 19 and 16 of 24. Promoting the first amendment to `##` split the Decision section, orphaning `### 3. Derive over restate`. Moved after Alternatives considered, matching ADR-0002's placement, with a pointer under §2. CLAUDE.md's restatement had dropped "landed" (garden-pathing the sentence), dropped "or auditor" though most finds were the auditor's, and dropped the "say which invariant makes it so" clause that fences the byte-form carve-out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Found by the scoped audit of the fail-open extraction class. **The evidence is named cases, not a statistic** (the ADR text says why: an earlier draft of this PR claimed "12 of 19", then "16 of 24", and the command it published to reproduce that returned 3 — the population spans open branches and moves hourly, and the `round` marker is a floor anyway, so the amendment deliberately quotes no count). The named cases: no fail-open in this repo has been found by the guard's own author — every one came from a reviewer or an auditor. A `[[ ]]` dispatch arm invisible to a strict-shape parser (#96), COPY coverage satisfied by a comment (#97), a pin anchor restated rather than read (#95), a workflow omitting itself from its own trigger paths (#109), and — in the PR that existed to fix this class — **five** successive rounds of comment- and spelling-blind parses (#110). **Why that is structural, not a lapse.** "Break the invariant" aims the mutation at the thing the author is already thinking about. It reliably confirms the hole they just closed and reliably misses the parsing assumption they did not know they had made. The first three cases predate this ADR, so their authors never ran §2 at all — that is the weaker half of the case. The stronger half is #93's and #110's guards, which DID carry recorded §2 runs and grew fail-opens anyway: a single mutation aimed at the invariant is not enough even when it is performed. ## The amendment §2 now asks for a **pair**: 1. **Form-only** — reformat the parsed source without changing meaning (wrap a line, requote, swap YAML flow for block, add a comment mentioning the identifier). Must stay **green** *and the assertion count must not fall*. A silent drop is the fail-open; that is what an evaporating `mapfile` looks like. 2. **Semantic, in a form you did not write** — comment the call out rather than deleting it; set the key in block form rather than flow. Must go **red**. Cost: two extra runs per new or materially-changed guard. Against that, these would have caught the post-ADR fail-opens above, each of which instead cost a review round. §3 gains one sentence: **the rung is a property of an assertion, not of a suite.** `test-volume-chown-guard.sh` was rung 2 on the chown semantics and rung 3 on the driver line, and the derived half vouched for the restated one. CLAUDE.md's Conventions bullet, which restates §2, is updated in the same PR per the ADR convention. ## Explicitly not proposed No new rung, and no new annotation kind for coverage or wiring. Coverage is already rung-2-able — §3's own rung-2 example (parsing a Dockerfile's COPYs rather than listing them) *is* set-derivation, and #101/#109 fixed the coverage holes by climbing the existing ladder. Wiring is a fact to state in the invariant, as #110 does, not a guard *kind*; folding it into `test:`/`tool:`/`ci:`/`structural`/`unguarded` would mix the guard-kind axis with the fact axis. Docs only. `tests/run-all` 28 suites / exit 0; `pre-commit run --all-files` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: t <t@t>

…ch-commit history (#122) Three commits, one story: what an audit is required to leave behind, and what the auditor is able to read. ## 1. `810dbad` — ADR-0006 amendment: committing an audit report is optional Reverses the "audits get a baseline" requirement. Closure is untouched — every surviving shape is still filed, landed, or recorded as an accepted `unguarded:` invariant — but the report itself is no longer mandatory. Why: the report is the second copy and the weaker one. A finding's durable form is an issue, a landed guard, or an `unguarded:` invariant, and all three live where the next reader already looks; a committed report restates that in prose nothing keeps true. It is the restate-over-derive shape ADR-0005 rejects, applied to the auditor's own output. A mandatory artifact also taxes the run that was already easiest to skip — the failure mode is not a missing file, it is a skipped audit. The cost is accepted explicitly rather than waved off: with no guaranteed prior report there is no guaranteed baseline, so measurements may not form a time series and a checked-and-dropped list may be re-litigated. Recorded in `unguarded:` form with a revisit trigger. Existing reports and the directory stay. ## 2. `934e999` — the auditor reads branch commits; the vocabulary is derived, not named **Branch history as an input.** `main` is squash-merged, so a hunk fixed three times on a branch lands as one clean commit and the fix-the-fix chain is unreachable from `git log`. Both audit agents excluded that history — `pr-reviewer` reviews the net `base...head` diff and deliberately anchors its history probe to `origin/main`, and the auditor is repo-scoped — so nobody read it. This PR's own subject demonstrates the loss. #90, which created these agents, contains: ``` 1254335 fix(agents): repair the log command; drop the ADR-0005 hard dependency ab9e77d fix(agents): anchor the repeat-offender log to origin/main; tidy audit ``` Two corrections to the same three lines of prose. On `main` that is one commit, `ba15520`. `gh pr view <n> --json commits` still returns the pre-squash sequence after merge, so this needs no pre-merge window. Scoped to PRs the other probes already implicated, so it stays one API call per candidate rather than per PR in the window. The squash fact is written as a check, not an assertion, so a change of merge strategy is noticed instead of assumed. Also adds "a set of merged PRs" to the scope list, making a retrospective over recent reviews a first-class scope. **Vocabulary drift.** #100 (`04e9eee`) typed the invariant ledger into the ADR-0005 vocabulary and touched `CLAUDE.md` only. `structural-auditor.md` reads that ledger and still named `(—)` in two places — a marker with zero remaining instances — so an agent following the instruction searched for a token that no longer exists and would have skipped every accepted-risk invariant in the repo. Silent under-coverage, in the agent whose job is finding unguarded couplings. Fixed by removing the copy rather than guarding it: the cross-section bullet names the categories and defers to the ledger's own header for the markers. A legacy untyped citation is now itself a finding, matching "type it when you touch it". **`pr-reviewer`** gains a note that the `origin/main` anchor also puts the branch's own commits out of scope, and that this is a handoff to the auditor. Without it the exclusion reads as an oversight and the next editor removes the anchor, restoring the self-trigger `ab9e77de` fixed. ## 3. `0d35e4c` — stop enumerating the audit reports by name The amendment named the two files in `docs/audits/` as "the existing reports". #112 commits a third under the now-optional rule, so that list goes stale on merge — a second copy of a directory listing, which is the shape the amendment itself argues against. De-enumerated in all three places with the reason stated inline. ## Verification - `pre-commit run --files` on all three files: clean. The `check-gitignore-agents` probe passes, confirming the agent files can actually be staged (ADR-0003). - The `gh pr view` command was **run as written** before being embedded. The two commands this file has shipped previously were both wrong on first landing; embedding a third unverified one would have repeated the exact failure this PR documents. - No test covers `.claude/agents/`, which is the point of the issue below. ## Related - #121 — the finding this work produced, filed rather than fixed here: `.claude/agents/*.md` ships shell commands and ledger vocabulary with no guard, and both halves have now broken once. This PR fixes the two symptoms; the mechanism is open. Deliberately **not** `Closes` — and #121 argues accept-with-a-reason is a legitimate disposal. - Merge-tested against all seven open PRs. Clean against `main`, #112, #113, #110, #109, #108. - **#117** conflicts with this branch only (it is clean against `main`): it rewrites the "Guards follow ADR-0005" bullet while `810dbad` extends the "A structural audit…" bullet directly beneath it. Adjacent lines, independent content — whoever merges second keeps both. - **#81** already conflicts with `main` independently of this branch, but note it edits the *same sentence* `810dbad` does: it replaces "ADR-0004 is reserved by #81" with the real entry while this adds ADR-0006. Both additions must survive the resolution; taking either side wholesale drops an ADR from the index. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011KsnnuPRJW5sUPppm3RSRo --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: t <t@t>

dlovell and others added 2 commits August 4, 2026 16:57

dlovell mentioned this pull request Aug 4, 2026

docs(audits): commit the 2026-08-04 audit report per ADR-0006 #114

Closed

dlovell and others added 2 commits August 4, 2026 17:43

dlovell mentioned this pull request Aug 4, 2026

dev/devcontainer has no test seam, which is why wiring guards must parse source #123

Open

dlovell mentioned this pull request Aug 4, 2026

tests/lib/dockerfile.sh and tests/lib/workflow-paths.sh are the subject of no test — their fail-open hardenings revert with every suite green #127

Open

Navigation Menu

Uh oh!

test: guard the wiring, not just the unit - #110

test: guard the wiring, not just the unit#110
dlovell wants to merge 6 commits into
mainfrom
fix/guard-the-wiring

dlovell commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

dlovell commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant