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/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 64482c0bf..d3f5288d0 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 @@ -170,14 +171,25 @@ 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. 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..a44a96bb6 --- /dev/null +++ b/scripts/tests/test_bom_guard_detection.py @@ -0,0 +1,184 @@ +"""`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. + """ + 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() + linked = [] + for tool in sorted(words - {"xxd"}): + found = shutil.which(tool) + if found and not (bindir / tool).exists(): + (bindir / tool).symlink_to(found) + 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)} + + +# ------------------------------------------------------------------------------------------------ +# 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'"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}" + ) + + +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") + + # 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"", ( + f"disarming the BOM comparison did not stop detection, so it is not load-bearing: {out!r}" + )