Build ErsatzTV Image / CI toolchain image resolves (pull_request) Successful in 35s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 57s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 37s
PR Gates / Docs update reminder (pull_request) Successful in 1m0s
PR Gates / decisions lifecycle (pull_request) Successful in 20s
PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 17s
review-verdict/h10 Review-verdict: MERGEABLE @ a7d91bf (base: main)
Review verdict / Set review-verdict status (pull_request_target) Successful in 45s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m25s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m17s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped
PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 19m27s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 6m4s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 8s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7s
`docs.no-session-narrative` reaches every durable artifact, but its detector scanned only `docs/**/*.md` and root markdown, and nothing had ever swept the rest. The issue named four sites from one grep and called them a floor. Deriving the population instead — a whitespace-joined sweep over every tracked file outside the detector, for the detector's own phrasings plus the attribution and review-round class #812 found — gave 453 sites in 108 files at `fb5592971`, and a second pass for phrasings the first list missed (hyphenated `round-N`, "an earlier version", "the reviewer proved") added residuals in the same files. Every site was classified with #812's three dispositions (CUT / SEVER / KEEP with its sub-kind) under the who-benefits test; the per-site manifests are on the PR. The rejected designs, tested-and-rejected fixtures, measurements and traps stay; the attribution of who found them and the round in which they were found go. The detector's population grows to `.claude/`, `.gitea/`, `.husky/` and `scripts/` regardless of extension, minus the detector and its own test (whose fixtures ARE the phrasings) and minus `scripts/tests/fixtures/` (test data, including decision-record copies — the same reasoning as the records' own exemption, and what keeps the record's depth measurement true), and `--all` lists tracked REGULAR files only — a symlink's content is its target and a gitlink has none. The #812 argument for leaving `docs/superpowers/**` in the population runs the other way here: `--diff` sees only ADDED lines, and 287 of the 453 sites were under 30 days old — this corpus is where narrative is being added, so the advisory nudge has reach. Density agrees: 56 line-mode hits over the 113 regular files the predicate admits, against 9 over 66 docs files before #812. `web/` and C# stay out on the same measurement (3 of 74 PATTERNS-matching sites, ~4,600 files). The predicate did not grow: PATTERNS matched 74 of 453 sites, and widening the word list to the attribution class is the treadmill the withdrawn parity test ran on. The population oracle is restated over segments with the new arms, the synthetic cross product gains the process heads and non-markdown extensions, a fixture witnesses that a tracked symlink is neither scanned nor counted, a `.py.bak` axis separates a by-name exemption from a `startswith` over the same tuple, and eight mutants (drop the process arm, drop the by-name exemption, exempt by `startswith`, drop or add a prefix, drop the fixtures exemption, list only markdown, drop the symlink filter, test the mode per row instead of per path) each redden it. A pre-existing silent drop in `--diff` goes with it: git tab-terminates a `+++` filename that contains a space, and the kept tab made `is_scanned_path` refuse the file with no notice — fixed, with a positive control and its own mutant. Code is unchanged by construction, measured per file type against `origin/main`: Python modules are AST-equal with docstrings stripped, except `#` lines inside the embedded fixture programs (string literals) of three test modules; workflows differ only in `#` lines inside `run:` block scalars; shell, C#, TypeScript and jq are equal with comment lines stripped. The stated exceptions: the detector and its test, 26 vitest titles that carried review-round or severity labels or a reviewer attribution (call sites whose title changed — every changed title line walked back to its `it(` / `it.each(...)(` anchor, so a `' + '` concatenation counts once), two registry note strings and the mutation manifest's prose fields. scripts/tests: 1565 passed. Web: lint, typecheck, 1319 tests green. Closes #876. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEcBoFw7ctrf3Nb7R7x7wk
256 lines
15 KiB
Python
256 lines
15 KiB
Python
"""Resolve where a subprocess would write its hook-fire records, and require an isolated one.
|
|
|
|
Split out of `conftest.py` so it can be imported: a test that drives these functions, and a suite
|
|
that has to build a subprocess environment from scratch, both need them, and importing `conftest`
|
|
under its own name would construct a SECOND `ProductionLogGuard` — whose `inspect.signature` call
|
|
would then read the already-wrapped `Popen` and fail at import. A plain module is imported once and
|
|
cached, so there is one guard and one production path per session.
|
|
|
|
See `conftest.py`'s docstring for what the two isolation layers are and why neither observes the
|
|
production directory (ersatztv#776, #809, #822).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import inspect
|
|
import os
|
|
import subprocess
|
|
from collections.abc import Mapping
|
|
from pathlib import Path
|
|
|
|
ENV_VAR = "ETV_HOOK_FIRE_LOG_DIR"
|
|
|
|
# `scripts/hook-fire-log.sh`'s `etv_hook_fire_log_dir()` is one parameter expansion:
|
|
# ${ETV_HOOK_FIRE_LOG_DIR:-${HOME:-/tmp}/.cache/ersatztv/hook-fire}
|
|
# Both `:-` defaults are reproduced below rather than restated as a literal path, because a guard
|
|
# that hardcodes where the sink writes stops guarding the moment the sink moves — and does so
|
|
# silently, by comparing against a directory nothing uses any more. The agreement is not left as an
|
|
# argument: `test_hook_fire_log.py::test_the_resolver_agrees_with_the_SINKS_OWN_expansion` runs both
|
|
# over the same matrix of environments.
|
|
_DEFAULT_SUFFIX = (".cache", "ersatztv", "hook-fire")
|
|
_HOME_FALLBACK = "/tmp" # noqa: S108 — mirrors the sink's own `${HOME:-/tmp}`
|
|
|
|
|
|
def resolved_log_dir(env: Mapping[str, str]) -> Path:
|
|
"""The directory `etv_hook_fire_log_dir` yields for a child launched with `env`.
|
|
|
|
Shell's `:-` treats an EMPTY value as absent, so `""` must fall through to the default exactly
|
|
as it does there; `env.get(k) or default` is that semantic and `env.get(k, default)` is not.
|
|
|
|
SCOPE, because the obvious wider claim would be false: this models the log DIRECTORY, not the
|
|
final file. `etv_hook_fire_record` writes to `${ETV_HOOK_FIRE_LOG_FILE:-...}`, so an inherited
|
|
`ETV_HOOK_FIRE_LOG_FILE` would in principle steer a record anywhere. It cannot in practice —
|
|
`etv_hook_fire_begin` recomputes that variable from this directory before any write, on both of
|
|
its paths, and nothing exports it — so it is deliberately not modelled here rather than
|
|
modelled wrongly. The one path reaching a write WITHOUT `begin` is the `record` CLI subcommand,
|
|
which does honour an inherited value; it has no call site in this repo. If a future change stops
|
|
recomputing the variable, or a caller starts using `record`, this becomes a real gap. (Cited by
|
|
SYMBOL rather than line number: the commit that added this note also shifted every line number
|
|
it first carried, by editing the header of the very file it points at.)
|
|
"""
|
|
return Path(env.get(ENV_VAR) or Path(env.get("HOME") or _HOME_FALLBACK, *_DEFAULT_SUFFIX))
|
|
|
|
|
|
# Resolved from a mapping holding only HOME — what the sink sees when the variable is unset. The
|
|
# mapping carries no `ETV_HOOK_FIRE_LOG_DIR`, so this reads the sink's DEFAULT regardless of what
|
|
# this session later exports; it does not depend on running before `pytest_configure`.
|
|
PRODUCTION_LOG_DIR = resolved_log_dir({"HOME": os.environ.get("HOME", "")})
|
|
|
|
# BOTH of the sink's default branches, because there are two shared logs and an explicit value can
|
|
# name either. `${HOME:-/tmp}` means an empty mapping yields `/tmp/.cache/ersatztv/hook-fire`, which
|
|
# is machine-wide, survives every run and is readable with `hook-fire-log.sh report --dir` exactly
|
|
# like the `$HOME` one. Requiring the variable (below) closes the OMISSION route for both branches;
|
|
# this tuple is what stops the variable being set TO one of them. Where `HOME` is unset the two
|
|
# collapse to the same path, which is harmless.
|
|
SHARED_LOG_DIRS = (PRODUCTION_LOG_DIR, resolved_log_dir({}))
|
|
|
|
|
|
def _anchored(path: Path, cwd) -> str:
|
|
"""Absolute realpath, resolving a RELATIVE value against the CHILD's working directory.
|
|
|
|
`Popen(cwd=...)` changes the directory the child resolves a relative `ETV_HOOK_FIRE_LOG_DIR`
|
|
against, so anchoring at the parent's cwd models the wrong process. That is a LIVE BYPASS:
|
|
`ETV_HOOK_FIRE_LOG_DIR=.cache/ersatztv/hook-fire` with `cwd=$HOME` lands
|
|
exactly on the shared log while a parent-anchored check sees an unrelated path under the repo
|
|
and clears it.
|
|
|
|
THREE cwd shapes, not four. `Popen` accepts str, bytes and `PathLike`; it calls `os.fsencode`,
|
|
which rejects an int, so a file DESCRIPTOR is not a launch shape and needs no handling here —
|
|
measured on CPython 3.9 and 3.13, and pinned by
|
|
`test_the_guard_JUDGES_every_cwd_SHAPE_that_Popen_ACCEPTS`. A branch for the int case is DEAD
|
|
code, because `os.fspath` already raises `TypeError` on
|
|
an int, and deleting it left that branch's own test green. `os.fsdecode(os.fspath(...))`
|
|
normalises the three shapes that do exist; `os.fspath` alone leaves
|
|
`os.path.join` raising on mixing str with bytes.
|
|
|
|
realpath, not `==`: `/tmp` is `/private/tmp` on macOS and `$HOME` may itself be a symlink.
|
|
"""
|
|
value = os.fsdecode(os.fspath(path))
|
|
if os.path.isabs(value):
|
|
return os.path.realpath(value) # `cwd` cannot change where an absolute path points
|
|
base = os.getcwd() if cwd is None else os.fsdecode(os.fspath(cwd))
|
|
return os.path.realpath(os.path.join(base, value))
|
|
|
|
|
|
def _decoded(env: Mapping) -> dict[str, str]:
|
|
"""Normalise a subprocess environment to str keys and values, ONCE, at the boundary.
|
|
|
|
`Popen` accepts a bytes-keyed and/or bytes-valued `env` on POSIX, and both reach here from a
|
|
legal launch. Unnormalised, a bytes KEY makes the `ETV_HOOK_FIRE_LOG_DIR` lookup miss and the
|
|
launch is rejected as carrying no isolated dir — a false positive on a correctly isolated
|
|
child — while a bytes VALUE makes `Path(...)` raise `TypeError`. Both are the "red on a
|
|
legitimate launch" failure ersatztv#809 explicitly forbids, and they are the same shape as the
|
|
bytes-`cwd` regression `_anchored` records.
|
|
|
|
Done here rather than at each lookup so there is one place that knows about the two encodings,
|
|
and no later reader has to remember which call sites handle which.
|
|
"""
|
|
return {os.fsdecode(k): os.fsdecode(v) for k, v in env.items()}
|
|
|
|
|
|
def isolation_violation(env: Mapping | None, cwd=None) -> str | None:
|
|
"""The message for a child that would log outside this session's isolation, else `None`.
|
|
|
|
THE RULE IS "CARRIES AN ISOLATED DIRECTORY", NOT "IS NOT THE PRODUCTION ONE", and the difference
|
|
is the whole point. `etv_hook_fire_log_dir` has TWO fallback branches — `$HOME/.cache/...` and,
|
|
when `HOME` is unset or empty, `/tmp/.cache/...` — so a check that compares against a single
|
|
resolved production path models one of them and is blind to the other. Measured on this suite:
|
|
of the 83 launches whose environment is built from scratch, 81 carry no `HOME` either, so they
|
|
resolve to `/tmp/.cache/ersatztv/hook-fire` — a machine-wide, run-to-run persistent hook-fire
|
|
log that a production-path comparison waves straight through. Requiring the variable
|
|
covers both branches and every branch a future sink might add, and it does not depend on what
|
|
`HOME` happens to be on the machine running the suite — which is also what stops the check
|
|
firing on legitimate launches wherever `HOME` is unset or is itself `/tmp`.
|
|
|
|
`env=None` means the child inherits `os.environ`, which `pytest_configure` has already isolated
|
|
— but it is RESOLVED rather than assumed safe, so a test that deletes the variable is caught by
|
|
this clause instead of by nothing.
|
|
"""
|
|
env = _decoded(os.environ if env is None else env)
|
|
if not env.get(ENV_VAR):
|
|
return (
|
|
f"this test launched a subprocess carrying no {ENV_VAR}, so `scripts/hook-fire-log.sh` "
|
|
f"falls back to its own default — {resolved_log_dir(env)} — which is a hook-fire log "
|
|
"shared between runs and between everyone using this machine. Any hook the child drives "
|
|
"appends there, and the fire-log library is fail-open by design, so the leak is "
|
|
"invisible from the hook side and every assertion stays green. Build the child's "
|
|
f"environment from `os.environ` (conftest.py has already isolated it), or copy {ENV_VAR} "
|
|
"into it explicitly if it must be built from scratch."
|
|
)
|
|
# An explicit value still must not BE a shared log — the variable can be set wrongly as easily
|
|
# as it can be omitted, and the likeliest wrong value is the sink's own default transliterated
|
|
# into a `dict.get(k, "<default>")`, which is how BOTH branches become reachable again.
|
|
target = _anchored(resolved_log_dir(env), cwd)
|
|
for shared in SHARED_LOG_DIRS:
|
|
# Absolute by construction, so `cwd` cannot change where they point.
|
|
root = os.path.realpath(shared)
|
|
# CONTAINMENT, not equality: `etv_hook_fire_report` reads its directory with
|
|
# `find "$dir" -name '*.jsonl' -type f`, which RECURSES, so a subdirectory of a shared log
|
|
# is picked up by `hook-fire-log.sh report --dir <shared>` exactly like a file at its root.
|
|
# An equality test calls `$HOME/.cache/ersatztv/hook-fire/sub` clean while records written
|
|
# there show up in the report — the same "looks identical, answers a different question"
|
|
# failure the whole isolation exists to remove.
|
|
if target == root or target.startswith(root + os.sep):
|
|
return (
|
|
f"this test launched a subprocess whose {ENV_VAR} is inside a shared hook-fire log "
|
|
f"({shared}) — one of the two directories `etv_hook_fire_log_dir` falls back to, "
|
|
"both of which outlive the run, are shared with everyone using this machine, and "
|
|
"are read RECURSIVELY by `hook-fire-log.sh report --dir`. Point it at a directory "
|
|
"this test session owns and deletes."
|
|
)
|
|
return None
|
|
|
|
|
|
class ProductionLogGuard:
|
|
"""Resolve every subprocess launch against the sink's rule, at the launch itself.
|
|
|
|
Only `Popen` is wrapped, because `subprocess.run`, `check_output` and `call` all reach the
|
|
module-global `Popen` by name at call time — so wrapping the one covers the four, and a second
|
|
wrapper would be a second place to keep in step with the first.
|
|
|
|
Its population is every launch that actually happens. That is the point: a list of hook-driving
|
|
suites, or a scan of their source, would have to be kept in step with the suite, and
|
|
`testing.guard-derives-population-from-source` is the record of what that costs.
|
|
|
|
WHAT IT DOES NOT SEE, stated so the next reader does not have to rediscover it. A child started
|
|
outside `Popen` — `os.execve`, `os.posix_spawn`, `os.system` — is not routed through this at
|
|
all. Neither is a child that is launched correctly and then re-execs, nor a shell script the
|
|
suite writes that unsets `ETV_HOOK_FIRE_LOG_DIR` itself before sourcing the sink: the guard
|
|
inspects the environment handed OVER, not what the child does with it afterwards. Nor is a
|
|
caller that bound `Popen` BY NAME (`from subprocess import Popen`) before `pytest_configure`
|
|
installed this — only a pytest plugin imports that early, and none here does. None of these
|
|
occur in this suite today, and each would be a deliberate act rather than the accident this
|
|
exists to catch; the accident is an environment assembled without the variable, and that is
|
|
visible at the launch.
|
|
|
|
A test that DELETES the variable from `os.environ` and then launches with `env=None` IS caught,
|
|
because `isolation_violation` resolves `os.environ` rather than assuming it safe.
|
|
|
|
Nor does it check that the directory is a GOOD one, only that it is not a shared hook-fire log:
|
|
`ETV_HOOK_FIRE_LOG_DIR=/tmp` or `=$HOME/.cache/ersatztv` passes while writing a session file into
|
|
a machine-wide, run-to-run persistent directory. The rule's name — "carries an isolated dir" —
|
|
is wider than what is implemented, which is "carries a dir that is not one of the two shared
|
|
logs". Nothing does that today. A bytes-keyed or bytes-valued `env` IS handled, at the boundary.
|
|
|
|
One more residual, measured rather than assumed: containment is a STRING comparison, and macOS
|
|
volumes are case-insensitive, so `$HOME/.cache/ErsatzTV/hook-fire` is the same directory as
|
|
`$HOME/.cache/ersatztv/hook-fire` and is cleared. Left as a note rather than fixed with an
|
|
inode comparison, deliberately: nothing spells it that way, the likeliest wrong value — the
|
|
sink's own default transliterated — matches case exactly, and two consecutive fixes here have
|
|
each created the next finding, which is this corpus's stated trigger for subtracting a layer
|
|
rather than adding one.
|
|
"""
|
|
|
|
def __init__(self) -> None:
|
|
self._real_popen = subprocess.Popen
|
|
# Derived from the real signature, never a literal index: `env` is a positional parameter of
|
|
# `Popen.__init__`, and a hand-counted position would silently start reading `cwd` — or
|
|
# nothing — if CPython ever reordered it, leaving the guard passing on every launch.
|
|
params = list(inspect.signature(self._real_popen.__init__).parameters)
|
|
self._env_pos = params.index("env") - 1 # `self` is not passed at the call site
|
|
# `cwd` too, because a RELATIVE log dir resolves against the CHILD's directory, not ours.
|
|
self._cwd_pos = params.index("cwd") - 1
|
|
self.checked = 0
|
|
self.installed = False
|
|
|
|
def install(self) -> None:
|
|
guard = self
|
|
real = self._real_popen
|
|
|
|
class GuardedPopen(real): # type: ignore[misc,valid-type]
|
|
# Declared in the class body, not attached afterwards, so it is a real attribute a type
|
|
# checker can see — and so `test_a_launch_NOT_CARRYING_...` can assert the guard is
|
|
# INSTALLED before concluding anything from its own launch going through.
|
|
etv_production_log_guard = guard
|
|
|
|
def __init__(self, *args, **kwargs):
|
|
guard.check(args, kwargs)
|
|
super().__init__(*args, **kwargs)
|
|
|
|
GuardedPopen.__name__ = real.__name__
|
|
GuardedPopen.__qualname__ = real.__qualname__
|
|
subprocess.Popen = GuardedPopen
|
|
self.installed = True
|
|
|
|
def uninstall(self) -> None:
|
|
subprocess.Popen = self._real_popen
|
|
self.installed = False
|
|
|
|
def _argument(self, name: str, pos: int, args: tuple, kwargs: dict):
|
|
"""What a `Popen` call passed for `name`, whether by keyword or in its positional slot."""
|
|
if name in kwargs:
|
|
return kwargs[name]
|
|
return args[pos] if len(args) > pos else None
|
|
|
|
def env_of(self, args: tuple, kwargs: dict):
|
|
return self._argument("env", self._env_pos, args, kwargs)
|
|
|
|
def cwd_of(self, args: tuple, kwargs: dict):
|
|
return self._argument("cwd", self._cwd_pos, args, kwargs)
|
|
|
|
def check(self, args: tuple, kwargs: dict) -> None:
|
|
self.checked += 1
|
|
violation = isolation_violation(self.env_of(args, kwargs), self.cwd_of(args, kwargs))
|
|
if violation is not None:
|
|
raise AssertionError(violation)
|