The production-hook-fire-log isolation guard is per-test, but its claim is per-suite #809
Closed
opened 2026-08-21 20:07:56 +02:00 by timothy
·
5 comments
No Branch/Tag Specified
main
renovate/meziantou.analyzer-3.x
release/v26.15.0-notes
fix/830-add-items-error-surface
renovate/lucene.net
renovate/cliwrap-3.x
issue-806-guard-populations
renovate/dotnet-monorepo
scratch/767b-poisoned
scratch/767b-control
release/v26.14.0-notes
release/v26.14.0
renovate/sqlitepclraw.bundle_e_sqlite3-3.x
docs/510-skill-logo-bug-policy
fix/510-watermark-resolution-policy
fix/629-verdict-classifier-falseopens
fix/609-decisions-edit-token-scope
issue-135-clear-to-none
release/v26.12.0-notes
fix/409b-lastscan-api-parity
fix/401-updatechannel-mirror-422
fix/327-playlist-rename-validation
fix/410-scancancel-log-level
fix/409-447-librariesscreen-neverscanned
fix/338-zap-exit-code
fix/367-plex-budget-message
fix/310-debom-legacy-cs
ci/604-lane-rebalance
feat/388-design-mirror
feat/247-test-ownership
feat/247-primary-action
feat/357-player-owned-playback
feat/357-jellyfin-plugin-poc
fix/289-mcp-hardening
issue58-mcp
feat/244-channels-extract
ci/auto-bump-prod-compose
feat/multi-rerun-collections-api
feat/collections-api
feat/quick-wins
feat/185-docs-part2
feat/140-collections-screen
feat/146-channel-edit
feat/147-classic-ui-link
issue22-renovate-dashboard
feat/91-cutover
feat/63-composite-create
feat/65-library-browse
feat/85-epg
feat/86-schedule-editor
feat/109-dashboard-data
feat/99-session-tracking
fix/dockerfile-node-tag
feat/59-spa-foundation
docs/59-ui-redesign-brief
feat/102-json-guide
feat/111-schedule-durations
feat/104-artwork-upload
feat/103-media-sources-api
feat/playouts-read-api
feat/108-health-api
feat/105-picker-list-endpoints
issue-97-channel-state-api
issue42-jellyfin-musicvideos
issue46-rest-api-error-contract
dependabot/nuget/ErsatzTV.FFmpeg.Tests/multi-d307a2e06f
qsv-improvements
hdr-vulkan-cuda-test
v26.15.0
v26.14.0
v26.13.0
v26.12.0
v26.11.0
v26.10.0
v26.9.0
v26.8.0
v26.7.0
blazor-final
v26.6.0
v26.5.0
v26.4.0
v26.3.1
v26.3.0
v26.2.0
v26.1.1
v26.1.0
v25.9.0
v25.8.0
v25.7.1
v25.7.0
v25.6.0
v25.5.0
v25.4.0
v25.3.1
v25.3.0
v25.2.0
v25.1.0
v0.8.8-beta
v0.8.7-beta
v0.8.6-beta
v0.8.5-beta
v0.8.4-beta
v0.8.3-beta
v0.8.2-beta
v0.8.1-beta
v0.8.0-beta
v0.7.9-beta
v0.7.8-beta
v0.7.7-beta
v0.7.6-beta
v0.7.5-beta
v0.7.4-beta
v0.7.3-beta
v0.7.2-beta
v0.7.1-beta
v0.7.0-beta
v0.6.9-beta
v0.6.8-beta
v0.6.7-beta
v0.6.6-beta
v0.6.5-beta
v0.6.4-beta
v0.6.3-beta
v0.6.2-beta
v0.6.1-beta
v0.6.0-beta
v0.5.8-beta
v0.5.7-beta
v0.5.6-beta
v0.5.5-beta
v0.5.4-beta
v0.5.3-beta
v0.5.2-beta
v0.5.1-beta
v0.5.0-beta
v0.4.5-alpha
v0.4.4-alpha
v0.4.3-alpha
v0.4.2-alpha
v0.4.1-alpha
v0.4.0-alpha
v0.3.8-alpha
v0.3.7-alpha
develop
v0.3.6-alpha
v0.3.5-alpha
v0.3.4-alpha
v0.3.3-alpha
v0.3.2-alpha
v0.3.1-alpha
v0.3.0-alpha
v0.2.5-alpha
v0.2.4-alpha
v0.2.3-alpha
v0.2.2-alpha
v0.2.1-alpha
v0.2.0-alpha
v0.1.5-alpha
v0.1.4-alpha
v0.1.3-alpha
v0.1.2-alpha
v0.1.1-alpha
v0.1.0-alpha
v0.0.62-alpha
v0.0.61-alpha
v0.0.60-alpha
v0.0.59-alpha
v0.0.58-alpha
v0.0.57-alpha
v0.0.56-alpha
v0.0.55-alpha
v0.0.54-alpha
v0.0.53-alpha
v0.0.52-alpha
v0.0.51-alpha
v0.0.50-alpha
v0.0.49-prealpha
v0.0.48-prealpha
v0.0.47-prealpha
v0.0.46-prealpha
v0.0.45-prealpha
v0.0.44-prealpha
v0.0.43-prealpha
v0.0.42-prealpha
v0.0.41-prealpha
v0.0.40-prealpha
v0.0.39-prealpha
v0.0.38-prealpha
v0.0.37-prealpha
v0.0.36-prealpha
v0.0.35-prealpha
v0.0.34-prealpha
v0.0.33-prealpha
v0.0.32-prealpha
v0.0.31-prealpha
v0.0.30-prealpha
v0.0.29-prealpha
v0.0.28-prealpha
v0.0.27-prealpha
v0.0.26-prealpha
v0.0.25-prealpha
v0.0.24-prealpha
v0.0.23-prealpha
v0.0.22-prealpha
v0.0.21-prealpha
v0.0.20-prealpha
v0.0.19-prealpha
v0.0.18-prealpha
v0.0.17-prealpha
v0.0.16-prealpha
v0.0.15-prealpha
v0.0.14-prealpha
v0.0.13-prealpha
v0.0.12-prealpha
v0.0.11-prealpha
v0.0.10-prealpha
v0.0.9-prealpha
v0.0.8-prealpha
v0.0.7-prealpha
v0.0.6-prealpha
v0.0.5-prealpha
v0.0.4-prealpha
v0.0.3-prealpha
v0.0.2-prealpha
v0.0.1-prealpha
Labels
Clear labels
ad-hoc
api
bug
ci-cd
content
dependencies
enhancement
frontend
in-progress
jellyfin
parked
priority: high
priority: low
priority: medium
review
security
One-off / ad-hoc work not tracked by a dedicated issue
REST API / HTTP endpoints
Something isn't working
Build, test, deploy pipeline
Channel content / schedules / playlists
Dependency updates (Renovate)
New feature or improvement
ChicoryTV React SPA frontend
Claimed by an active session — do not pick up
Jellyfin tuner / IPTV integration
Excluded from automatic queue pickup; work only when explicitly selected
Adversarial review finding
Security / vulnerability fix
Milestone
No items
No Milestone
Projects
Clear projects
No projects
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#809
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Spawned by #785, which shipped the defect this guard exists to catch and was not caught by it.
The gap
scripts/tests/conftest.py's autouseisolate_hook_fire_logfixture monkeypatchesETV_HOOK_FIRE_LOG_DIRintoos.environat test setup, so hooks driven as subprocesses log to a tmp dir instead of$HOME/.cache/ersatztv/hook-fire/. That is the #776 invariant: tests never write to the real log at all.test_hook_fire_log.py::test_the_suite_does_not_write_to_the_PRODUCTION_logis the guard, and its docstring states the scope as:What it actually checks is narrower:
ETV_HOOK_FIRE_LOG_DIRis set for the test currently running;Neither reaches another test file. A suite that does
_ENV = {**os.environ, ...}at MODULE level captures the environment as it was at import/collection time — before the autouse fixture ran — and hands that stale mapping to every subprocess it launches.Measured
scripts/tests/test_worktree_ownership_guard.py(added in #785) did exactly that. Before it was caught, the production log held 1,488 records for its two synthetic session ids (session-aaaa-1111,session-bbbb-2222); one further run added 28 more. Every test in the file passed throughout, andtest_the_suite_does_not_write_to_the_PRODUCTION_logstayed green, because the fire-log library is fail-open by design — a hook whose logging misfires must behave exactly as an uninstrumented one, so nothing can surface from the hook side.Why it matters beyond tidiness:
scripts/hook-fire-log.sh reportis the surfacedocs/guard-inventory.mdcites as the observability claim for every guard still gradedNONE. A log carrying test artifacts answers a different question while looking identical — the inference problem #776 exists to abolish, one layer up. The stray records also outlive the session: they accumulate on dev machines and on the persistent CI runner's$HOME.What #785 shipped instead
A per-file pin:
test_worktree_ownership_guard.py::test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dirasserts the env handed to subprocesses carries the fixture's dir. Witnessed red by reintroducing the import-time snapshot. That protects one file, which is the same shape as the guard it complements — the next suite to snapshotos.environis unprotected again.Scope
The honest difficulty, stated so it is not rediscovered: the obvious general check — diff the production log dir before and after each test — is racy, because a real interactive session's hooks fire into that same directory while the suite runs. A guard that goes red because the developer used the Bash tool during a test run teaches its readers to ignore it, which is #806's failure shape.
So the likely answers are structural rather than observational, e.g.:
os.environ(a source-text predicate — notefixing-a-parser-bug-introduces-the-next-one; budget rounds or reject it);run_hook()helper inconftest.pythat every hook-driving suite uses, so there is one place that builds the env and no per-file opportunity to snapshot it. This is thefix-the-boundary-not-the-siteoption and is probably the right one.Also worth correcting either way: the existing guard's docstring should state the scope it checks rather than the scope it wants.
Done-when
test_hook_fire_log.py, or the guard's docstring is narrowed to what it checks and the gap recorded{**os.environ}snapshot in another suite)docs/decisions/records/testing/guard-derives-population-from-source.md)Observed on 2026-08-22 while running the full
scripts/testssuite locally during #772/#792:The same suite passed on an immediate clean re-run (910 passed) and the file passes in isolation (78 passed). The difference between the two runs was another agent session doing tool calls on this machine at the same time.
That fits this issue's subject and sharpens it: the assertion compares mtimes of the REAL
~/.cache/ersatztv/hook-fire/*.jsonlbefore and after driving its own hook, so it does not actually measure "did THIS SUITE write to the production log" — it measures "did anything on this host write to it during that window". Two Claude sessions on one machine is the normal working mode in this repo (CLAUDE.md-> "Working in parallel with other sessions"), and every one of them fires merge-consent/BOM/worktree hooks on ordinary tool calls.So the guard has a false-positive mode that scales with exactly the workflow the repo encourages, and its red says "the suite polluted the production log" when the true statement is "someone did". Worth folding into the fix here: the per-suite claim needs a source of truth that can attribute a write, not a whole-directory mtime — e.g. compare the SET of records, or key on a marker the suite's own fires carry.
No action taken on this branch; recording it because a mid-session red here reads as a real isolation failure and cost a re-run to dismiss.
Observation from #788 (2026-08-26), offered as evidence rather than a fix — this is a second, distinct manifestation of the permeability this issue is about.
test_the_suite_does_not_write_to_the_PRODUCTION_logfailed once during a full-suite run and did not reproduce on a re-run of the identical tree. An independent reviewer working the same branch saw the same failure and the same non-reproduction, so it is not local to one machine or one checkout.The mechanism is different from the stale-module-level-
_ENVone in the body. The guard snapshots mtimes under the real~/.cache/ersatztv/hook-fire/at test start and compares after driving its own hook — so any concurrent writer to that shared directory during the window fails it, no stale env required. Concurrent Claude Code sessions and subagents fire hooks continuously and write there; three sessions were active on this repo at the time.So the per-test/per-suite scope gap this issue names has a sibling: the guard reads a shared mutable directory it does not own, which makes it non-deterministic under concurrency independent of how well
conftest.pyisolates the suite. Worth folding into the fix, since widening the claim from per-test to per-suite would not close this one — a per-suite snapshot over a shared dir has the same exposure, just a longer window.No action taken here; #788 measured it and moved on.
Evidence from an unrelated session (the #786/#789 work), recorded because it is a concrete
reproduction rather than a theory.
test_hook_fire_log.py::test_the_suite_does_not_write_to_the_PRODUCTION_logwent red on a fullpytest scripts/testsrun, on a diff that touches no hook and no fire-log file:Cause, measured not inferred. The test snapshots
st_mtime_nsof every~/.cache/ersatztv/hook-fire/*.jsonlbefore its own hook run and compares after. That directory isin
$HOME— shared across every worktree and every concurrent Claude session. A cold-reviewsubagent was running in its own worktree at the time. During the window:
Three session logs written inside the comparison window; the mtime multiset moved; the test fired.
It is green in isolation, both before and after the change (
pytest scripts/tests/test_hook_fire_log.py→78 passed, 2 skipped), and the same full suite was green onruns where no second session was active (1161 / 1175 passed).
This is exactly the gap this issue names — the isolation is per-test (
ETV_HOOK_FIRE_LOG_DIRis setfor the test's own hook) while the CLAIM is per-suite ("the suite does not write to the production
log"). What the assertion actually measures is "nothing on this machine wrote to the shared
directory", which is not a property of the suite at all. CI does not hit it because one job runs
alone; a developer with a second session open does, and the red names a file they did not touch.
Worth noting for whoever fixes it:
ETV_HOOK_FIRE_LOG_DIRbeing set correctly is genuinely assertedat the top of the same test, and that part is sound. It is only the before/after mtime comparison
over a
$HOMEpath that is machine-scoped rather than suite-scoped. Refs #822.Claiming #809 + #822 together as a single-mechanism bundle: both are the same guard
(
scripts/tests/test_hook_fire_log.py::test_the_suite_does_not_write_to_the_PRODUCTION_log) and thesame conftest isolation seam. #822 is the oracle reading global mutable state; #809 is the isolation
being per-file rather than per-suite. Both bodies independently land on the same structural answer —
make the production path unreachable from the suite by construction, with one place that builds the
hook subprocess env — so fixing them separately would mean building the mechanism twice.
Claude Code session, worktree off
origin/main.Closing record
Outcome: Fixed in http://192.168.1.95:3000/timothy/ersatztv/pulls/874, together with #822 — the two are one mechanism. The isolation is now structural and cross-suite:
scripts/tests/conftest.py'spytest_configuresetsETV_HOOK_FIRE_LOG_DIRbefore collection, and asubprocess.Popenwrapper (scripts/tests/hook_fire_isolation.py) fails any launch that does not CARRY an isolated dir. The guard's docstring was not narrowed — the gap was closed.Root cause: The isolation was an autouse function fixture, which runs at test setup. By then every test module has been imported, so it could not reach a module holding a
{**os.environ}snapshot — and, not anticipated by this issue, it could not reach a module- or session-scoped fixture either, since those are set up outside any single test. Measured on the pre-change tree: of 83 launches resolving to$HOME/.cache/ersatztv/hook-fire, 37 were during collection and 44 inside such fixtures; only 2 resembled the module-level-snapshot route this issue names.Decisions/conventions changed: Added
testing.suite-isolated-from-production-hook-fire-log(docs/decisions/records/testing/); catalog regenerated.Reusable knowledge:
os.environdicts (and flagged that such a predicate costs rounds). A source scan would have found nothing and reported the suite clean; only instrumentingPopenand running the suite produced the real population.Popenalone.subprocess.run,callandcheck_outputall reach it, so a census wrappingrunas well counts every launch twice — the first census here did, and doubled every published figure until cold review refuted it.pytest_configureruns before collection, so a value set there is captured by a module-level{**os.environ}— which turns the #785 defect class from something to detect into something that cannot happen.${VAR:-${OTHER:-x}}default. A check comparing against one resolved production path is blind to the other; 81 of the 83 from-scratch launches here landed in the branch such a check cannot see.assert isolation_violation(os.environ) is Noneprints the whole environment, API keys included, into the failure output — pytest rewrites the expression and reprs every sub-expression. Bind the result first.Popenacceptscwdas str/bytes/PathLikebut not an int fd, and accepts a bytes-keyed or bytes-valuedenv. A guard that raisesTypeErroron a legal launch is broken, not strict.Verification:
PYTHONPATH=. python3 -m pytest scripts/tests -q→ 1228 passed / 2 skipped (base 1224/2). Green withHOMEunset,HOME=/tmpandHOME="". Nine clause mutations witnessed red, each disarmed alone and restored. Zero unintended guard rejections. Six cold adversarial rounds, three cross-family; the sixth found no code or documentation defects.Deferred: The launch guard does not see a child started outside
Popen(os.execve,os.posix_spawn), one that re-execs, or a script that unsets the variable itself; containment is a string comparison, so a case-variant spelling on a case-insensitive filesystem is cleared; and "carries an isolated dir" is wider than "carries a good dir". None occur in the suite today and all are enumerated inProductionLogGuard's docstring rather than left to be rediscovered.Docs updated:
docs/guard-inventory.md(narrative rewritten to the closed state, both withdrawn shapes recorded so neither is re-adopted), the new decision record + regenerateddocs/decisions/README.md,docs/README.md,scripts/hook-fire-log.sh, and stale docstrings intest_worktree_ownership_guard.pyandtest_guard_populations_derive_from_git.py.