fix(809,822): isolate the suite from the production hook-fire log by construction #874

Merged

timothy merged 1 commits from fix/809-822-hook-fire-log-isolation into main 2026-08-29 04:32:27 +02:00

Conversation 1 Commits 1 Files Changed 13

+945 -74

timothy commented 2026-08-29 04:09:36 +02:00

Owner

Copy Link

Copy Source

Closes two issues that are one mechanism: the guard meant to keep pytest scripts/tests out of the
shared hook-fire log had a racy oracle (#822) and per-test scope (#809). Both issue bodies
independently arrive at the same answer — make the shared path unreachable rather than watch it — so
fixing them separately would have built the mechanism twice.

What was wrong

#822 — the oracle was global mutable state. The guard snapshotted st_mtime_ns across
~/.cache/ersatztv/hook-fire/ and required no change. That directory is written by every live
Claude Code session on the machine, so an unrelated session firing a hook inside the window failed
the suite with a test run modified the production hook-fire log — an accusation about the suite
when the writer was another process. Seen on three separate branches, green on every re-run.

#809 — the isolation was per test. The autouse fixture runs at test setup, after every module
has been imported, so it never reached a module holding a {**os.environ} snapshot (the #785
defect).

What measuring changed about the diagnosis. #809 proposes a source-text scan for module-level
dicts, flagging in advance that such a predicate costs rounds. Instrumenting subprocess.Popen
across a full run and asking which launches resolve to $HOME/.cache/ersatztv/hook-fire found
83 — 37 during collection and 44 inside module- or session-scoped fixtures, which are set
up outside any single test and so were never covered by the autouse fixture at all; only 2
resembled the route the issue names. A source scan would have reported the suite clean.

The fix — two layers, closing different routes

1. pytest_configure sets ETV_HOOK_FIRE_LOG_DIR before collection. Ordering verified by
   execution: a module-level {**os.environ} does carry a value set there.
2. A subprocess.Popen wrapper fails any launch that does not carry the isolated directory.

A whitelist, not a blacklist, and that is the point. The sink's default has two branches, and a
comparison against a single resolved production path models one and is blind to the other:
${HOME:-/tmp} means a child with no HOME lands in /tmp/.cache/ersatztv/hook-fire, equally
shared and equally persistent — 81 of the 83 from-scratch launches were exactly that shape.
Requiring the variable covers both branches and does not depend on what HOME is on the machine.
Its corollary is that a from-scratch environment must carry the variable, one line each.

The resolver reimplements one shell expansion and fails open, so it is differential-tested
against etv_hook_fire_log_dir every run — including the empty-value cases where dict.get(k, d)
and ${k:-d} disagree, and normalisation cases where shell concatenation and Path spell the same
directory differently, which is why it compares realpath identity rather than bytes.

Verification

• PYTHONPATH=. python3 -m pytest scripts/tests -q — 1224 passed / 2 skipped on this branch's
  base, 1228 / 2 here (four added tests). Zero unintended guard rejections.
• Same two guard files green with HOME unset, HOME=/tmp, and HOME="" (96 passed each) —
  the false-positive scenario cold review raised, answered by execution.
• Reds witnessed, each by disarming one clause and restoring it: the production-path blacklist
  leaves the /tmp branch unguarded; parent-anchored resolution leaves a relative value with a
  child cwd unguarded; equality-instead-of-containment clears a subdirectory of a shared log;
  dropping the env/cwd boundary normalisation crashes on a legal bytes launch; ignoring the child's
  cwd leaves the shape test green until the binding assertions were added; removing the
  pre-collection assignment leaks an import-time snapshot into a fake HOME; removing the wrapper
  install un-guards a launch; breaking the resolver's empty-value semantics reddens the differential;
  with both layers off the cross-suite assertion fires on its own.
• #822's race reproduced on demand: with a thread touching a file in the shared directory, the
  withdrawn mtime oracle goes RED while the replacement stays GREEN and records provably land in the
  isolated dir.
• ruff check / ruff format --check / pyright clean. (One pre-existing pyright complaint in a
  stub HTTP handler is present at origin/main too.)

Review

Six cold adversarial rounds, in isolated worktrees, alternating same-family and cross-family
(Codex / GPT-5.6). Five of them found a real defect, several created by the previous round's fix:
the unguarded /tmp branch; HOME-dependent false positives; a double-counted census (wrapping
subprocess.run as well as Popen counted every launch twice); a relative-cwd bypass;
equality-not-containment; a TypeError on a legal bytes cwd; and its twin on a bytes env. Round
six found no code or documentation defects and independently re-derived the disputed launch
count as 65 rather than 345, confirming the 83 = 65+14+2+2 breakdown.

Where two consecutive rounds produced findings created by the previous fix, this applies the
corpus's own STOP-AND-SUBTRACT rule rather than adding another layer: the file-descriptor cwd
branch was deleted (it guarded an input Popen rejects, and was dead anyway), and the last pass
subtracts prose — the durable docs no longer restate refuted figures or carry round-by-round
chronology, which docs.no-session-narrative puts in the commit message. Traps and rejected
approaches stay, per that rule's carve-out.

Found while writing the tests

• pytest's assert rewriting printed the whole environment — API keys included — into the failure
  output, because the violation was computed inside the assert. Binding it first fixes it;
  verified by grepping the output for a key name (0 occurrences).
• os.environ["HOME"] would KeyError where HOME is unset — a legal environment the sink has a
  ${HOME:-/tmp} default for. Fixed at both sites.
• The cwd shape test was vacuous: it used an absolute log dir, which short-circuits before cwd
  is consulted at all.

Docs

• docs/guard-inventory.md — this guard's narrative rewritten to the closed state, with both
  withdrawn shapes recorded so neither is re-adopted.
• New record testing.suite-isolated-from-production-hook-fire-log, catalog regenerated.
• Stale prose corrected in test_worktree_ownership_guard.py,
  test_guard_populations_derive_from_git.py, scripts/hook-fire-log.sh and docs/README.md.

fixes #809
fixes #822

Closes two issues that are one mechanism: the guard meant to keep `pytest scripts/tests` out of the shared hook-fire log had a racy oracle (#822) and per-test scope (#809). Both issue bodies independently arrive at the same answer — make the shared path unreachable rather than watch it — so fixing them separately would have built the mechanism twice. ## What was wrong **#822 — the oracle was global mutable state.** The guard snapshotted `st_mtime_ns` across `~/.cache/ersatztv/hook-fire/` and required no change. That directory is written by every live Claude Code session on the machine, so an unrelated session firing a hook inside the window failed the suite with `a test run modified the production hook-fire log` — an accusation about the suite when the writer was another process. Seen on three separate branches, green on every re-run. **#809 — the isolation was per test.** The autouse fixture runs at test setup, after every module has been imported, so it never reached a module holding a `{**os.environ}` snapshot (the #785 defect). **What measuring changed about the diagnosis.** #809 proposes a source-text scan for module-level dicts, flagging in advance that such a predicate costs rounds. Instrumenting `subprocess.Popen` across a full run and asking which launches resolve to `$HOME/.cache/ersatztv/hook-fire` found **83** — **37** during collection and **44** inside module- or session-scoped fixtures, which are set up outside any single test and so were never covered by the autouse fixture at all; only **2** resembled the route the issue names. A source scan would have reported the suite clean. ## The fix — two layers, closing different routes 1. `pytest_configure` sets `ETV_HOOK_FIRE_LOG_DIR` **before collection**. Ordering verified by execution: a module-level `{**os.environ}` does carry a value set there. 2. A `subprocess.Popen` wrapper fails any launch that does not **carry** the isolated directory. **A whitelist, not a blacklist, and that is the point.** The sink's default has two branches, and a comparison against a single resolved production path models one and is blind to the other: `${HOME:-/tmp}` means a child with no `HOME` lands in `/tmp/.cache/ersatztv/hook-fire`, equally shared and equally persistent — **81 of the 83** from-scratch launches were exactly that shape. Requiring the variable covers both branches and does not depend on what `HOME` is on the machine. Its corollary is that a from-scratch environment must carry the variable, one line each. The resolver reimplements one shell expansion and **fails open**, so it is differential-tested against `etv_hook_fire_log_dir` every run — including the empty-value cases where `dict.get(k, d)` and `${k:-d}` disagree, and normalisation cases where shell concatenation and `Path` spell the same directory differently, which is why it compares `realpath` identity rather than bytes. ## Verification - `PYTHONPATH=. python3 -m pytest scripts/tests -q` — **1224 passed / 2 skipped** on this branch's base, **1228 / 2** here (four added tests). Zero *unintended* guard rejections. - Same two guard files green with **`HOME` unset, `HOME=/tmp`, and `HOME=""`** (96 passed each) — the false-positive scenario cold review raised, answered by execution. - **Reds witnessed, each by disarming one clause and restoring it:** the production-path blacklist leaves the `/tmp` branch unguarded; parent-anchored resolution leaves a relative value with a child `cwd` unguarded; equality-instead-of-containment clears a subdirectory of a shared log; dropping the env/cwd boundary normalisation crashes on a legal bytes launch; ignoring the child's `cwd` leaves the shape test green until the binding assertions were added; removing the pre-collection assignment leaks an import-time snapshot into a fake `HOME`; removing the wrapper install un-guards a launch; breaking the resolver's empty-value semantics reddens the differential; with both layers off the cross-suite assertion fires on its own. - **#822's race reproduced on demand:** with a thread touching a file in the shared directory, the withdrawn mtime oracle goes RED while the replacement stays GREEN and records provably land in the isolated dir. - `ruff check` / `ruff format --check` / `pyright` clean. (One pre-existing pyright complaint in a stub HTTP handler is present at `origin/main` too.) ## Review **Six cold adversarial rounds**, in isolated worktrees, alternating same-family and cross-family (Codex / GPT-5.6). Five of them found a real defect, several created by the previous round's fix: the unguarded `/tmp` branch; `HOME`-dependent false positives; a **double-counted census** (wrapping `subprocess.run` as well as `Popen` counted every launch twice); a relative-`cwd` bypass; equality-not-containment; a `TypeError` on a legal bytes `cwd`; and its twin on a bytes `env`. Round six found **no code or documentation defects** and independently re-derived the disputed launch count as 65 rather than 345, confirming the `83 = 65+14+2+2` breakdown. Where two consecutive rounds produced findings created by the previous fix, this applies the corpus's own STOP-AND-SUBTRACT rule rather than adding another layer: the file-descriptor `cwd` branch was **deleted** (it guarded an input `Popen` rejects, and was dead anyway), and the last pass subtracts *prose* — the durable docs no longer restate refuted figures or carry round-by-round chronology, which `docs.no-session-narrative` puts in the commit message. Traps and rejected approaches stay, per that rule's carve-out. ## Found while writing the tests - pytest's assert rewriting printed the **whole environment — API keys included** — into the failure output, because the violation was computed inside the `assert`. Binding it first fixes it; verified by grepping the output for a key name (0 occurrences). - `os.environ["HOME"]` would `KeyError` where `HOME` is unset — a legal environment the sink has a `${HOME:-/tmp}` default for. Fixed at both sites. - The cwd shape test was **vacuous**: it used an absolute log dir, which short-circuits before `cwd` is consulted at all. ## Docs - `docs/guard-inventory.md` — this guard's narrative rewritten to the closed state, with both withdrawn shapes recorded so neither is re-adopted. - New record `testing.suite-isolated-from-production-hook-fire-log`, catalog regenerated. - Stale prose corrected in `test_worktree_ownership_guard.py`, `test_guard_populations_derive_from_git.py`, `scripts/hook-fire-log.sh` and `docs/README.md`. fixes #809 fixes #822

timothy added 1 commit 2026-08-29 04:09:37 +02:00

fix(809,822): isolate the suite from the production hook-fire log by construction ... 1d14161889

The guard meant to keep `pytest scripts/tests` out of the shared hook-fire log
had two independent defects, and both issue bodies land on the same answer: make
the shared path unreachable rather than watch it.

#822 — its oracle was global mutable state. It snapshotted `st_mtime_ns` across
`~/.cache/ersatztv/hook-fire/` and required no change, but that directory is
written by every live Claude Code session on the machine, so an unrelated
session firing a hook inside the window failed the suite with `a test run
modified the production hook-fire log`. Seen on three separate branches, green
on every immediate re-run. Reproduced on demand while writing this: with a
thread touching a file there, the withdrawn oracle goes RED and the replacement
stays GREEN.

#809 — its isolation was per test. The autouse fixture runs at test setup, after
every module has been imported, so it never reached a module holding a
`{**os.environ}` snapshot (#785, 58 records per run).

Measuring changed the diagnosis twice. #809 proposes a source-text scan for
module-level dicts, flagging in advance that such a predicate costs rounds.
Instrumenting `subprocess.Popen` across a full run and asking which launches
resolve to `$HOME/.cache/ersatztv/hook-fire` found 83 — 37 during collection and
44 inside module- or session-scoped fixtures, which are set up outside any single
test and so were never covered by the autouse fixture at all; only 2 resembled
the route the issue names. A source scan would have reported the suite clean.

Instrument `Popen` ALONE: `subprocess.run` reaches it, so a census wrapping both
counts every launch twice. The first one here did, and published 166/74/88/4
until cold review refuted it as exactly 2x — the figures above are a single
re-measurement on this branch's own base. Every count is now quoted with the
predicate that produced it, because the from-scratch launches also number 83: a
different set of the same size, which is precisely what a later reader
reconciles wrongly.

So: `pytest_configure` sets `ETV_HOOK_FIRE_LOG_DIR` before collection (ordering
verified by execution, not read), and a `Popen` wrapper fails any launch that
does not CARRY the isolated directory.

That second rule is deliberately "carries an isolated dir" and not "is not the
production one" — cold review found the first draft's blacklist modelled one of
the sink's two default branches and was blind to the other. `${HOME:-/tmp}`
means a child with no HOME lands in `/tmp/.cache/ersatztv/hook-fire`, which is
equally shared and equally persistent; measured, 81 of the 83 from-scratch
launches were exactly that shape. The whitelist covers both branches and does not depend
on what HOME is on the machine running the suite — which is also what stops it
firing where HOME is unset or is itself /tmp. A second round then found the same
blind spot surviving in the clause that rejects an explicit WRONG value, which
compared against the $HOME path alone; it now rejects both shared logs. Its
corollary is that a from-scratch environment must carry the variable, one line
each. No count of those sites is kept in the docs: it is a hand-written
population nothing derives, and this commit falsified its own by adding sites in
the tests that prove the rule. The guard enumerates them.

The resolver reimplements one shell expansion and fails OPEN, so it is
differential-tested against `etv_hook_fire_log_dir` every run — the empty-value
cases where `dict.get(k, d)` and `${k:-d}` disagree, and the normalisation cases
where shell concatenation and `Path` spell the same directory differently, which
is why it compares realpath identity rather than bytes.

Proofs, each witnessed red by disarming one clause and restoring it: reverting
to the production-path blacklist leaves the `/tmp` branch unguarded (DID NOT
RAISE); removing the pre-collection assignment makes an import-time snapshot
leak into a fake HOME (both arms nested pytest, launch guard disabled in both so
the comparison is about that clause alone); removing the wrapper install
un-guards a launch; breaking the resolver's empty-value semantics reddens the
differential; and with both layers off the cross-suite assertion fires on its
own. A fourth round, again on two fronts, found the `cwd` plumbing raising
`TypeError` on bytes — a shape `Popen` legally accepts — so a legal launch
crashed the guard instead of being judged. The first fix for that also added a
branch for an int file descriptor, and cold review then measured two things: a
descriptor is NOT a launch shape (`Popen` calls `os.fsencode(cwd)`, which rejects
an int), and the branch was dead anyway because `os.fspath` already raises on
one — deleting it left its own test green. Two consecutive rounds whose finding
was created by the previous round's fix is this corpus's stated trigger to
subtract rather than guard, so the branch and its unreachable fail-closed arm are
gone; `cwd` is normalised across the three shapes that exist, and the premise
that there are three is pinned by a test that reddens if CPython ever adds a
fourth. The same round caught that test being vacuous — it used an ABSOLUTE log
dir, which short-circuits before `cwd` is consulted at all.

A fifth round found the last of that family: `Popen` also accepts a bytes-keyed
or bytes-valued `env` on POSIX, and the guard rejected one such launch as
carrying no isolated dir and crashed on the other — both reds on a correctly
isolated child. Fixed at the boundary rather than per lookup: the environment is
normalised to str once, in one place, and a test pins that the normalisation
does not blunt the guard. That round also showed the cwd shape test proved only
"does not crash": ignoring the child's `cwd` entirely left it green, so it now
judges the same relative value against two different cwds and requires two
different verdicts.

One finding of that round is recorded as REFUTED rather than fixed: it reported
the from-scratch launch count as 391 rather than 83. Re-measured directly —
instrumenting `Popen` and running `test_ci_dropped_step_guard.py` alone gives 65
launches, matching the census artifact, against the 345 claimed for that file.
The 83 = 65 + 14 + 2 + 2 breakdown stands.

Five rounds each falsified another hand-written claim in the prose, so the last
pass subtracts prose rather than adding more: the durable docs no longer restate
refuted figures or carry round-by-round chronology, which is `docs.no-session-
narrative` — that belongs here, in the commit message. The traps and the rejected
approaches stay, per that rule's carve-out. Full suite
green (1224 passed/2 skipped on this branch's base, 1228/2 after — the four added
tests) with zero UNINTENDED guard rejections — the only ones are those the
launch-guard test provokes on purpose —
so the strictness costs nothing on a clean checkout.

A third round, this one cross-family, found the same blind spot once more in a
form neither earlier round reached: a RELATIVE `ETV_HOOK_FIRE_LOG_DIR` resolves
against the CHILD's working directory, so `.cache/ersatztv/hook-fire` with
`cwd=$HOME` lands exactly on the shared log while a guard anchoring it at its own
cwd sees an unrelated path under the repo and clears it. Reproduced, then closed
by anchoring at the launch's `cwd=`; both `env` and `cwd` positions are read from
`inspect.signature` rather than hand-counted. Witnessed red by reverting to
parent-anchored resolution. That round also caught a count left at 82 in one file
where every other copy said 83, and two docstrings still describing records as
leaking into the REAL log when they now land in the session directory.

Two defects found in the new tests while writing them. pytest's assert rewriting
printed the whole environment — API keys included — into the failure output,
because the violation was computed inside the `assert` expression. And
`os.environ["HOME"]` would KeyError where HOME is unset, which is a legal
environment the sink has a `${HOME:-/tmp}` default for.

fixes #809
fixes #822

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RK43vYYMULNirkJ824ggud

timothy commented 2026-08-29 04:11:49 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 1d14161

Six cold rounds (3 cross-family). Round 6: no code or doc defects; independently re-derived the disputed launch count as 65, confirming 83=65+14+2+2. Suite 1228/2, green with HOME unset//tmp/empty. Nine clause mutations witnessed red.

Review-verdict: MERGEABLE @ 1d14161 Six cold rounds (3 cross-family). Round 6: no code or doc defects; independently re-derived the disputed launch count as 65, confirming 83=65+14+2+2. Suite 1228/2, green with HOME unset//tmp/empty. Nine clause mutations witnessed red.

timothy merged commit b6b3520bdb into main 2026-08-29 04:32:27 +02:00

timothy deleted branch fix/809-822-hook-fire-log-isolation 2026-08-29 04:32:29 +02:00

timothy referenced this issue from a commit 2026-08-29 10:43:47 +02:00

fix(819): state the wiring residual honestly, and correct the checkout/binary claim

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#874