From 487387072778b3c99ad9c35b9b3b617df46aa303 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 14 Aug 2026 17:14:39 +0200 Subject: [PATCH 1/2] fix: the BOM guard was fail-open wherever xxd is not installed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pretooluse-bom-guard.sh` detected a BOM with `head -c3 | xxd -p`. `xxd` ships with vim and is ABSENT on this repo's Linux CI runner — verified on the runner host — so the command substitution yielded empty, never equalled `efbbbf`, and the guard allowed every BOM in silence. It has been fail-open on any machine without vim since it was written. `od -A n -t x1 -N 3` is POSIX and byte-identical on macOS and Linux. Why nobody noticed: `docs/guard-inventory.md` graded this guard NONE — no proof it could go red — and every review of it ran on macOS, where xxd exists. The environment was a sampled population and the sample was unanimous. It surfaced only because an unrelated change (#776) added an assertion that this hook must reach a real decision, and that assertion ran on Linux. So it ships with the proof it never had: the real hook driven end to end, driven again with xxd removed from PATH (the regression), a negative control on a clean file, an anti-vacuity check that the fixture really stages a BOM, and a clause-level mutation — disarm the comparison in an isolated copy and detection must stop. Verified RED against the pre-fix hook and green against this one. The inventory row moves NONE -> MUTATION, and the "gaps ranked by what a silent failure would let through" list records that its first entry was ranked correctly for a worse reason than intended. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/hooks/pretooluse-bom-guard.sh | 6 +- docs/guard-inventory.md | 15 +- scripts/tests/test_bom_guard_detection.py | 158 ++++++++++++++++++++++ 3 files changed, 174 insertions(+), 5 deletions(-) create mode 100644 scripts/tests/test_bom_guard_detection.py diff --git a/.claude/hooks/pretooluse-bom-guard.sh b/.claude/hooks/pretooluse-bom-guard.sh index b8430b2a1..b687c433c 100755 --- a/.claude/hooks/pretooluse-bom-guard.sh +++ b/.claude/hooks/pretooluse-bom-guard.sh @@ -66,7 +66,11 @@ while IFS= read -r f; do esac p="$root/$f" [ -f "$p" ] || continue - if [ "$(head -c3 "$p" 2>/dev/null | xxd -p 2>/dev/null)" = "efbbbf" ]; then + # `od`, NOT `xxd`. `xxd` ships with vim and is absent on plain Linux hosts including this repo's + # CI runner, where the command substitution yielded empty, never equalled `efbbbf`, and this guard + # therefore passed every BOM in silence. It has been fail-open on any host without vim since it + # was written. `od -A n -t x1 -N 3` is POSIX and produces byte-identical output on macOS and Linux. + if [ "$(od -A n -t x1 -N 3 < "$p" 2>/dev/null | tr -d ' \n')" = "efbbbf" ]; then bad="${bad} ${f}"$'\n' fi done < /tmp/.bom-guard-files.$$ diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 64482c0bf..4fa3a2b1f 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -82,7 +82,7 @@ all. | `.claude/hooks/pretooluse-agent-model.sh` | an Agent dispatch naming no model (asks) | GUARD | NONE | — | | `.claude/hooks/pretooluse-agent-ram.sh` | an Agent dispatch under 10% free RAM | GUARD | NONE | — | | `.claude/hooks/pretooluse-bash-guard.sh` | a Bash call setting `ETV_UPDATE_GOLDENS` | GUARD | NONE | — | -| `.claude/hooks/pretooluse-bom-guard.sh` | a commit/push carrying a BOM in a touched `.cs` | GUARD | NONE | — | +| `.claude/hooks/pretooluse-bom-guard.sh` | a commit/push carrying a BOM in a touched `.cs` | GUARD | MUTATION | `test_bom_guard_detection.py::test_DISARMING_the_BOM_comparison_stops_detection` | | `.claude/hooks/pretooluse-merge-consent.sh` | a PR merge without derived consent | GUARD | BEHAVIOUR-ONLY | `test_merge_consent_exemption.py::test_protected_path_on_a_LATER_page_is_still_seen` | | `.claude/hooks/pretooluse-nav-guard.sh` | a browser navigate to a streaming URL | GUARD | NONE | — | | `.claude/hooks/pretooluse-worktree-guard.sh` | a commit/merge in a foreign worktree | GUARD | NONE | — | @@ -105,6 +105,7 @@ all. | `scripts/post-review-verdict.sh` | nothing (writes the verdict status) | GUARD | BEHAVIOUR-ONLY | `test_post_review_verdict.py::test_never_retargets_the_verdict_at_the_new_head` | | `scripts/pr-changed-files.sh` | the verdict exemption, on an incomplete enumeration | GUARD | BEHAVIOUR-ONLY | `test_pr_changed_files.py::test_a_SHORT_page_does_not_end_the_enumeration` | | `scripts/update-openapi.sh` | nothing (regenerates the spec) | TOOLING | NONE | — | +| `scripts/tests/test_bom_guard_detection.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_build_catalog.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_check_review_verdict.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_ci_dropped_step_guard.py` | the `script-tests` job | PROOF | NONE | — | @@ -123,8 +124,8 @@ all. ## What the numbers say -31 guards, 4 tooling scripts, 12 proof files. **4 guards carry a mutation proof; 6 are -behaviour-only; 21 have none.** These figures are asserted against the table by +31 guards, 4 tooling scripts, 13 proof files. **5 guards carry a mutation proof; 6 are +behaviour-only; 20 have none.** These figures are asserted against the table by `test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it, which is the duplication family this change argues against. Both cold reviewers found the error @@ -177,7 +178,13 @@ past. The gaps are not uniform in cost, and the ranking that matters is *what a silent failure would let through*, not test count: -1. `pretooluse-bom-guard.sh` — guards a defect that has recurred three times (#311, #402, #405). +1. ~~`pretooluse-bom-guard.sh`~~ — **now proven, and it was fail-open the whole time.** Ranked + first here because the defect it guards has recurred three times (#311, #402, #405); that + ranking turned out to be right for a worse reason than intended. It detected a BOM with + `xxd -p`, and `xxd` ships with vim and is **absent on the Linux CI runner**, so the comparison + never matched and every BOM was allowed in silence. `od` now, with a clause-level mutation proof. + The lesson for the rows below: an unproven guard is not merely untested, it is a guard whose + *current* behaviour nobody has established. 2. `pretooluse-worktree-guard.sh` + `posttooluse-worktree-marker.sh` — a two-file mechanism guarding #289, where a regression in either half is invisible and the two halves have never been tested together. diff --git a/scripts/tests/test_bom_guard_detection.py b/scripts/tests/test_bom_guard_detection.py new file mode 100644 index 000000000..8959943fc --- /dev/null +++ b/scripts/tests/test_bom_guard_detection.py @@ -0,0 +1,158 @@ +"""`pretooluse-bom-guard.sh` actually detects a BOM — including where `xxd` does not exist. + +The guard compared `head -c3 | xxd -p` against `efbbbf`. **`xxd` ships with vim and is absent +on plain Linux hosts, including this repo's CI runner** (verified there directly). On such a host the +command substitution yields the empty string, never equals `efbbbf`, and the guard allows every BOM +in silence. It had been fail-open on any machine without vim since it was written, and nothing +noticed because `docs/guard-inventory.md` graded it `NONE` — no proof it could go red. + +It surfaced only when an unrelated change (ersatztv#776) added an assertion that this hook must +reach a real decision, and that assertion ran on Linux. Every prior review of this guard ran on +macOS, where `xxd` exists — the *environment* was a sampled population, and the sample was +unanimous. + +So this file exists to make the guard's detection load-bearing rather than assumed: + + * it drives the real hook end to end, through the real payload shape; + * it drives it with `xxd` REMOVED FROM PATH, which is the regression; + * and it performs a clause-level mutation — the comparison is disarmed in an isolated copy and the + guard must stop detecting — which is what `testing.guard-ships-with-mutation-proof` asks for and + what this guard has never had. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +HOOK = REPO_ROOT / ".claude" / "hooks" / "pretooluse-bom-guard.sh" + +BOM = b"\xef\xbb\xbf" + + +def _git(cwd: Path, *args: str) -> None: + subprocess.run( + ["git", *args], cwd=str(cwd), check=True, capture_output=True, + env={**os.environ, "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@e", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@e"}, + ) + + +def _repo_with(tmp_path: Path, name: str, content: bytes) -> Path: + # The guard scopes itself to repositories whose toplevel path matches `*ersatztv*`, so the + # directory name is load-bearing, not decoration. + repo = tmp_path / "ersatztv-scratch" + repo.mkdir() + _git(repo, "init", "-q", ".") + (repo / name).write_bytes(content) + _git(repo, "add", name) + return repo + + +def _run(hook: Path, repo: Path, env: dict) -> tuple[int, bytes]: + payload = json.dumps({ + "session_id": "s", "hook_event_name": "PreToolUse", "tool_name": "Bash", + "cwd": str(repo), "tool_input": {"command": "git commit -m x"}, + }) + p = subprocess.run(["bash", str(hook)], input=payload.encode(), capture_output=True, + cwd=str(repo), env=env, timeout=60) + return p.returncode, p.stdout + + +def _path_without_xxd(tmp_path: Path) -> dict: + """A PATH that resolves everything the hook needs EXCEPT `xxd`. + + Rebuilding PATH from symlinks rather than just dropping directories, because `xxd` usually lives + in the same directory as `git` and `od`; removing that directory would starve the hook of tools + it legitimately needs and the test would pass for the wrong reason. + """ + bindir = tmp_path / "nobin" + bindir.mkdir() + for tool in ("bash", "git", "od", "tr", "sort", "head", "grep", "sed", "jq", "rm", "printf", + "cat", "dirname", "basename", "mktemp", "date"): + found = shutil.which(tool) + if found: + (bindir / tool).symlink_to(found) + assert shutil.which("od", path=str(bindir)), "the stand-in PATH lost `od` itself" + assert not shutil.which("xxd", path=str(bindir)), "the stand-in PATH still resolves xxd" + return {**os.environ, "PATH": str(bindir)} + + +# ------------------------------------------------------------------------------------------------ +# ANTI-VACUITY — if the fixture stops producing a BOM file, every assertion below is meaningless. +# ------------------------------------------------------------------------------------------------ + + +def test_the_fixture_really_stages_a_BOM(tmp_path): + repo = _repo_with(tmp_path, "Bad.cs", BOM + b"class A {}\n") + assert (repo / "Bad.cs").read_bytes()[:3] == BOM + staged = subprocess.run(["git", "diff", "--name-only", "--cached"], cwd=str(repo), + capture_output=True, text=True).stdout.split() + assert staged == ["Bad.cs"], f"nothing was staged, so the guard would have nothing to read: {staged}" + + +# ------------------------------------------------------------------------------------------------ +# THE GUARD DECIDES +# ------------------------------------------------------------------------------------------------ + + +def test_a_staged_BOM_is_DENIED(tmp_path): + repo = _repo_with(tmp_path, "Bad.cs", BOM + b"class A {}\n") + rc, out = _run(HOOK, repo, dict(os.environ)) + assert rc == 0, "the hook communicates by printing, and must always exit 0" + assert b'"permissionDecision": "deny"' in out or b'"permissionDecision":"deny"' in out, ( + f"a staged BOM-carrying .cs was not denied: {out!r}" + ) + + +def test_a_staged_BOM_is_DENIED_when_xxd_DOES_NOT_EXIST(tmp_path): + """THE REGRESSION. This is the case that was silently allowed on every host without vim.""" + repo = _repo_with(tmp_path, "Bad.cs", BOM + b"class A {}\n") + rc, out = _run(HOOK, repo, _path_without_xxd(tmp_path)) + assert rc == 0 + assert b"deny" in out, ( + "with `xxd` absent the guard allowed a BOM. That is the fail-open this file exists to " + f"prevent, and it is the state every non-vim host was in: {out!r}" + ) + + +def test_a_clean_file_is_ALLOWED(tmp_path): + """The negative control. A guard that denies everything would pass the tests above.""" + repo = _repo_with(tmp_path, "Good.cs", b"class A {}\n") + rc, out = _run(HOOK, repo, _path_without_xxd(tmp_path)) + assert rc == 0 + assert out == b"", f"a BOM-free file was not allowed silently: {out!r}" + + +# ------------------------------------------------------------------------------------------------ +# THE MUTATION PROOF — disarm the comparison alone, detection must stop +# ------------------------------------------------------------------------------------------------ + + +def test_DISARMING_the_BOM_comparison_stops_detection(tmp_path): + """`testing.guard-ships-with-mutation-proof`, which this guard has never carried. + + The clause is disarmed in an isolated copy — the comparison is pointed at a byte sequence no + file starts with — and the guard must go quiet. If it still denies, the deny is coming from + somewhere other than the clause the guard is supposed to hang on, and the tests above prove + nothing about it. + """ + text = HOOK.read_text() + marker = '= "efbbbf" ]; then' + assert marker in text, ( + "the BOM comparison has moved; retarget this mutation rather than loosening it — a mutation " + "that silently stops mutating is the failure this file is about" + ) + mutated = tmp_path / "mutated-bom-guard.sh" + mutated.write_text(text.replace(marker, '= "deadbeef" ]; then', 1)) + + repo = _repo_with(tmp_path, "Bad.cs", BOM + b"class A {}\n") + rc, out = _run(mutated, repo, dict(os.environ)) + assert rc == 0 + assert out == b"", ( + f"disarming the BOM comparison did not stop detection, so it is not load-bearing: {out!r}" + ) -- 2.47.3 From 6a6ce36b2cfa4e6bffd8eb527e1261c64fd56733 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 14 Aug 2026 17:30:01 +0200 Subject: [PATCH 2/2] fix(797): the twin recipe was still broken, and the mutation proof was green over a dead guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent reviews of #798. No Blocker; the functional fix is confirmed sound — one reviewer ran a 17-input matrix on BSD od, GNU od, and inside the actual CI runner image, identical in every cell, with `xxd` confirmed absent there. Everything below is what they found around it. THE FIX STOPPED AT THE HOOK. `docs/decisions/records/process/bom-format-detection-recipe.md` (active) and `docs/contributing.md:120` both still prescribed `head -c3 | xxd -p | grep -q '^efbbbf'` as the standing manual pre-push check — same absent xxd, same silent all-clean, on the path a human runs PRECISELY when the hook is being bypassed with --no-verify. Worse, that record's "Verify your detector" paragraph rejected an `od` form by name, so the active record argued against the hook this PR ships. Both corrected. The record keeps its original warning, which is still right and was never about `od` as a tool: the form that reported all-clean over 19 dirty files was `od -An -c | grep '357 273 277'` — octal character output, whose spacing and escaping vary. The replacement is `od -A n -t x1 -N 3`, hexadecimal bytes, measured across the matrix above. The record now says its own prescribed detector had been an all-clean detector for three weeks, which is the thing it was written to prevent. THE MUTATION PROOF WAS GREEN OVER A DEAD GUARD. Run against the pre-fix hook on Linux, `test_DISARMING_the_BOM_comparison_stops_detection` PASSED while the guard detected nothing at all — "the mutant is silent" is trivially true when the original is silent too. The inventory row cites that function by name. It now asserts the unmutated guard DOES detect, first. THE STAND-IN PATH STARVED THE HOOK. `_path_without_xxd` hand-listed six tools the hook never calls and omitted `tail`, which it does call for cd-detection — so a payload whose cwd differs from the repo failed open for a reason unrelated to xxd. The list is derived from the hook's own source now, with a required-set assertion so a starved PATH fails loudly instead of passing quietly. Also: the inventory narrative restated the NONE count and drifted the moment this guard was regraded, invisibly to the summary parser. The duplicate count is removed rather than corrected. Refs: #797 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing.md | 2 +- docs/decisions/README.md | 2 +- .../process/bom-format-detection-recipe.md | 24 ++++++++++--- docs/guard-inventory.md | 7 +++- scripts/tests/test_bom_guard_detection.py | 36 ++++++++++++++++--- 5 files changed, 58 insertions(+), 13 deletions(-) diff --git a/docs/contributing.md b/docs/contributing.md index 30a1b0c38..c6253d6eb 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -117,7 +117,7 @@ talks exclusively to the REST API. Every former Blazor route now 302-redirects t `io.open(..., encoding='utf-8-sig')` *writes one back*, and sed/perl round-trips keep it. This cost two sessions a red CI job on 2026-07-17 alone (PR #405, 6 files; PR #402, 19), which is what the guard is for. To check by hand: - `for f in $(git diff --name-only origin/main...HEAD --cached -- '*.cs'; git diff --name-only -- '*.cs') ; do head -c3 "$f" | xxd -p | grep -q '^efbbbf' && echo "BOM: $f"; done` (covers branch + staged + dirty — a brand-new staged file is absent from `origin/main...HEAD`) + `for f in $(git diff --name-only origin/main...HEAD --cached -- '*.cs'; git diff --name-only -- '*.cs') ; do [ "$(od -A n -t x1 -N 3 < "$f" | tr -d ' \n')" = efbbbf ] && echo "BOM: $f"; done` (covers branch + staged + dirty — a brand-new staged file is absent from `origin/main...HEAD`) (note `dotnet format --include` needs **bash** — `mapfile` is bash-only, and under zsh the file list comes out empty, which looks exactly like the tool silently doing nothing). (A one-time repo-wide normalization is a separate, unmade decision; the touched-file rule is the standing one.) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 40d690926..f39e9c9de 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -103,7 +103,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `media.remote-stream-probe` | `ValidatePlayoutItemPath` probes the Plex/Jellyfin/Emby remote-stream URL via `IRemoteStreamProber` before returning it; only a redirected 404 fails closed (`PlayoutItemNotAvailableFromMediaServer`), everything else fails open, and there is no toggle. | 2026-07-19 | [link](records/media/remote-stream-probe.md) | | `media.remote-stream-probe-externaljson` | External-JSON playout channels' `StreamRemotely` now probes the remote-stream URL through the same `IRemoteStreamProber` seam as the generated-playout path, closing the #473 scope gap for a channel kind with no DB `PlayoutItem` rows. | 2026-07-20 | [link](records/media/remote-stream-probe-externaljson.md) | | `media.source-mgmt-write-api` | Media-source management (local/Plex/Jellyfin/Emby) is a REST write API + SPA under `/app/libraries/*`, wrapping existing MediatR commands 1:1 with no new commands or DB migration; connection GETs never leak a stored `apiKey`, and each PUT-replace family's identity contract is documented per-family (not assumed uniform). | 2026-07-11 | [link](records/media/source-mgmt-write-api.md) | -| `process.bom-format-detection-recipe` | Before any push touching `.cs`, detect BOMs with the `xxd` byte check and verify the format gate with `dotnet format --include` run under `bash -c`, never bare zsh. | 2026-07-21 | [link](records/process/bom-format-detection-recipe.md) | +| `process.bom-format-detection-recipe` | Before any push touching `.cs`, detect BOMs with the `od -A n -t x1 -N 3` byte check and verify the format gate with `dotnet format --include` run under `bash -c`, never bare zsh. NOT `xxd`: it ships with vim and is absent on plain Linux hosts including this repo's CI runner, where the substitution yields empty, never matches, and the check reports all-clean — the same all-clean-detector failure this record was written about, in the detector it prescribed. | 2026-07-21 | [link](records/process/bom-format-detection-recipe.md) | | `process.branch-off-feature-branch` | To fix work on an unmerged feature branch, branch off that branch and land by fast-forward push — and after creating a worktree, drive the first Edit/Read from ITS absolute paths and `git status` it before building. | 2026-07-21 | [link](records/process/branch-off-feature-branch.md) | | `process.build-concurrency-limits` | Run at most 3–4 concurrent dotnet/npm builds on this Mac, gate launches on FREE RAM rather than CPU load, and never set `ETV_UPDATE_GOLDENS` / `ETV_UPDATE_PLAYOUT_GOLDENS`. | 2026-07-21 | [link](records/process/build-concurrency-limits.md) | | `process.codex-cheap-worker-launch` | For bounded tool-bearing selector/recon work, launch a Codex worker with `codex exec -m gpt-5.4-mini -c model_reasoning_effort=low -s read-only`; `spawn_agent` buys parallelism but no cost savings. | 2026-07-21 | [link](records/process/codex-cheap-worker-launch.md) | diff --git a/docs/decisions/records/process/bom-format-detection-recipe.md b/docs/decisions/records/process/bom-format-detection-recipe.md index 7cbb93948..e938df43e 100644 --- a/docs/decisions/records/process/bom-format-detection-recipe.md +++ b/docs/decisions/records/process/bom-format-detection-recipe.md @@ -1,11 +1,11 @@ --- key: process.bom-format-detection-recipe -title: '2026-07-21 — BOM/format pre-push detection: use the `xxd` loop, and run `dotnet format --include` under `bash -c` (#542)' +title: '2026-07-21 — BOM/format pre-push detection: use the `od` byte check, and run `dotnet format --include` under `bash -c` (#542, detector corrected #797)' status: active since: '2026-07-21' supersedes: none superseded-by: none -rule: Before any push touching `.cs`, detect BOMs with the `xxd` byte check and verify the format gate with `dotnet format --include` run under `bash -c`, never bare zsh. +rule: 'Before any push touching `.cs`, detect BOMs with the `od -A n -t x1 -N 3` byte check and verify the format gate with `dotnet format --include` run under `bash -c`, never bare zsh. NOT `xxd`: it ships with vim and is absent on plain Linux hosts including this repo''s CI runner, where the substitution yields empty, never matches, and the check reports all-clean — the same all-clean-detector failure this record was written about, in the detector it prescribed.' signals: 'UTF-8 BOM · `efbbbf` · `dotnet format --verify-no-changes` · `--include` · `mapfile` · zsh vs bash · #311 format gate · detector verification · paths: `ErsatzTV.sln`, `.editorconfig` · issues: #542, #311, #70, PR #402, PR #405' mechanics: Local pre-push shell; CI's format job recipe (`ci.format-gate-folder-mode`). The BOM *policy* is `release.format-as-you-touch-rebase`; this record is the mechanics only. --- @@ -13,11 +13,25 @@ mechanics: Local pre-push shell; CI's format job recipe (`ci.format-gate-folder- Detection loop, verbatim: ```bash -for f in $(git diff --name-only origin/main...HEAD -- '*.cs'); do head -c3 "$f" | xxd -p | grep -q "^efbbbf" && echo "BOM: $f"; done +for f in $(git diff --name-only origin/main...HEAD -- '*.cs'); do [ "$(od -A n -t x1 -N 3 < "$f" | tr -d ' \n')" = efbbbf ] && echo "BOM: $f"; done ``` -**Verify your detector.** An `od -An -c | grep '357 273 277'` check reported all-clean while 19 files -were actually dirty. A detector that can only say "ok" is worse than none — use the `xxd` form or read +**The prescribed detector was itself an all-clean detector, corrected 2026-08-14 (#797).** This +record was written because a detector that can only say "ok" is worse than none. It then prescribed +`xxd -p`, and **`xxd` ships with vim and is absent on plain Linux hosts, including this repo's CI +runner** — where the substitution yields the empty string, never matches `efbbbf`, and the loop +prints nothing for a tree full of BOMs. The rule failed its own test, in the recipe it recommends, +for three weeks. + +Read the original warning precisely, because it is still right and it is not about `od`: the form +that reported all-clean over 19 dirty files was `od -An -c | grep '357 273 277'` — octal character +output, whose spacing and escaping vary. The replacement is `od -A n -t x1 -N 3`, which emits +hexadecimal bytes. That is a different invocation of the same tool, and it was measured across 17 +inputs (empty, sub-3-byte, exactly the BOM, BOM+NUL, binary, UTF-16 BOM, unreadable, 20 MB, awkward +filenames) on BSD `od`, GNU `od`, and inside the actual CI runner image: identical in every cell. +`tr -d ' \n'` is load-bearing — BSD pads to a fixed column width and GNU does not. + +**Verify your detector** — use the form above, or read the bytes directly, and distrust a clean result you didn't prove can go dirty. **`dotnet format --include` DOES work here** — an earlier note claiming it silently no-ops was WRONG. diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 4fa3a2b1f..d3f5288d0 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -171,7 +171,12 @@ dangerous direction — a token that a reviewer means as `BLOCKED` silently read approval, which writes a green `review-verdict/h10`. A token added to one script and not the other is untested by either, which is the whole of #788. -The 21 include every `PreToolUse` hook except merge-consent — the same population #776 found we +**That unproven set carries no number here on purpose.** It restated the count, drifted the moment +the BOM guard was regraded, and `test_the_summary_counts_match_the_table` cannot see it — the parser +checks the formatted summary sentence and nothing else. A second hand-maintained copy of a number is +the duplication family this file argues against, so the copy is removed rather than corrected: the +set is every `PreToolUse` hook except merge-consent and the BOM guard, and the count is in the +summary above. It is also the population #776 found we cannot even observe firing. A guard that is neither proven nor observable is an assertion about the past. diff --git a/scripts/tests/test_bom_guard_detection.py b/scripts/tests/test_bom_guard_detection.py index 8959943fc..a44a96bb6 100644 --- a/scripts/tests/test_bom_guard_detection.py +++ b/scripts/tests/test_bom_guard_detection.py @@ -70,14 +70,29 @@ def _path_without_xxd(tmp_path: Path) -> dict: in the same directory as `git` and `od`; removing that directory would starve the hook of tools it legitimately needs and the test would pass for the wrong reason. """ + import re + + # DERIVED from the hook, not hand-listed. The hand-listed version symlinked six tools the hook + # never calls and omitted `tail`, which it does call for `cd`-detection — so a payload whose cwd + # differs from the repo took a `tail: command not found` path and the guard ALLOWED a BOM for a + # reason that had nothing to do with xxd. A stand-in PATH that starves the subject is a test + # passing for the wrong reason, which is the whole subject of this file. + words = set(re.findall(r"\b([a-z][a-z0-9_-]*)\b", HOOK.read_text())) bindir = tmp_path / "nobin" bindir.mkdir() - for tool in ("bash", "git", "od", "tr", "sort", "head", "grep", "sed", "jq", "rm", "printf", - "cat", "dirname", "basename", "mktemp", "date"): + linked = [] + for tool in sorted(words - {"xxd"}): found = shutil.which(tool) - if found: + if found and not (bindir / tool).exists(): (bindir / tool).symlink_to(found) - assert shutil.which("od", path=str(bindir)), "the stand-in PATH lost `od` itself" + linked.append(tool) + # `head` is deliberately NOT here: the `od` change removed the only `head` call, and asserting a + # tool the hook no longer uses is how a stand-in PATH drifts from its subject. + for required in ("bash", "git", "od", "tr", "sort", "tail"): + assert shutil.which(required, path=str(bindir)), ( + f"the stand-in PATH lost `{required}`, which the hook needs — the test would then pass " + "because the guard was starved, not because it detected anything" + ) assert not shutil.which("xxd", path=str(bindir)), "the stand-in PATH still resolves xxd" return {**os.environ, "PATH": str(bindir)} @@ -114,7 +129,7 @@ def test_a_staged_BOM_is_DENIED_when_xxd_DOES_NOT_EXIST(tmp_path): repo = _repo_with(tmp_path, "Bad.cs", BOM + b"class A {}\n") rc, out = _run(HOOK, repo, _path_without_xxd(tmp_path)) assert rc == 0 - assert b"deny" in out, ( + assert b'"permissionDecision": "deny"' in out or b'"permissionDecision":"deny"' in out, ( "with `xxd` absent the guard allowed a BOM. That is the fail-open this file exists to " f"prevent, and it is the state every non-vim host was in: {out!r}" ) @@ -151,6 +166,17 @@ def test_DISARMING_the_BOM_comparison_stops_detection(tmp_path): mutated.write_text(text.replace(marker, '= "deadbeef" ]; then', 1)) repo = _repo_with(tmp_path, "Bad.cs", BOM + b"class A {}\n") + + # POSITIVE CONTROL FIRST. Without it this test passes when the guard detects NOTHING AT ALL — + # verified: run against the pre-fix hook on Linux, where xxd is absent, and "the mutant is + # silent" was trivially true. A green mutation proof over a dead check is the exact failure + # `guard-ships-with-mutation-proof` exists to stop, and the inventory row cites THIS function. + rc_live, out_live = _run(HOOK, repo, dict(os.environ)) + assert rc_live == 0 and b"deny" in out_live, ( + "the UNMUTATED guard did not detect the BOM, so 'the mutant is silent' proves nothing about " + f"the clause: {out_live!r}" + ) + rc, out = _run(mutated, repo, dict(os.environ)) assert rc == 0 assert out == b"", ( -- 2.47.3