From aa11acdd0bb271fe20a12d752107efe45fa3e59c Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 02:03:09 -0500 Subject: [PATCH 01/14] feat(asvs): prove an absence claim bites, not just that its pattern matches (BACKLOG #1006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check_absences` admits an ASVS absence claim on `re.search(a.pattern, a.mutation)` -- one TOML field matched against another. That proves the mutation is well-formed; it never proves the mutation BITES. A reintroduction raised into a swallowing handler, written to a field nobody reads, or behind a flag nobody branches on satisfies every failure mode `check_absences` has and changes nothing observable. A green gate that is not evidence. Add an opt-in `--prove-absences` mode (`scripts/asvs/scorecard.py`) that executes the claim rather than grepping it: - Two optional `Absence` fields, `mutation_path` and `observable` (a pytest node id). When both are set the mode copies the tree to a scratch dir, runs the observable (baseline must be green), appends the mutation, and requires the observable to go RED -- and to fail as a test failure (exit 1). It fails closed on every other code: an already-red baseline, an uncollectable node, or a mutation that only breaks import is a PROVE-ERROR, never a proof. A claim that reddens nothing is UNPROVEN and fails the mode. - A coarse same-file static backstop screens claims carrying `mutation_path` but no `observable`: a `raise` landing in a file whose every handler swallows. It is a screen, not a proof (it cannot see a swallow in a caller), documented as such. - The whole pass runs in a TemporaryDirectory scratch copy, so it never mutates the tracked tree and never trips the committed-tree scan on itself. Both fields default empty and load without being refused: the vault's ~81 existing absence claims carry neither and must stay loadable (ADR 0156 §7). Absent means "not yet proven by execution", surfaced by the mode, never "proven vacuous". Review hardening carried in this change (the mode's own helpers): - `_scratch_ignore` refuses `.env*`, `*.db` (+ WAL sidecars) and `docs/security` when copying the tree. The vault runs this module against the REAL tree (ADR 0156 §7); a scratch copy carrying those would spill secrets / the local store / vault posture data into a world-default temp dir, which CLAUDE.md §9 forbids. The public-repo path never sees them; this is defence for the eventual vault run. - `_is_within_tree` refuses a `mutation_path` that is absolute or contains `..` before anything is applied, so an authored path cannot escape the scratch copy. Tests (tests/test_asvs_scorecard.py): eight fixture tests drive `prove_absences` directly (proved / UNPROVEN / already-red baseline / collection-error / two static backstop arms including a re-raise reach control / root-untouched / load round-trip), plus three that drive the CLI contract CI depends on -- `main([..., "--prove-absences"])` exit 0 on a biting fixture and 1 on a non-biting one, and `main([...])` without `--corpus` exit 2 -- plus the secrets-exclusion and path-traversal guards. Every new test was falsified (broken on purpose, watched red, restored). MessageFoundry is a not-deployed beta: the mode is opt-in, the default `verify` path is byte-unchanged, and no authored claim carries an `observable` yet, so nothing new is blocked by this alone today. Wiring the mode over the vault claims and backfilling their observables is the owner's follow-up. --- scripts/asvs/scorecard.py | 328 +++++++++++++++++++++++++++++- tests/test_asvs_scorecard.py | 383 +++++++++++++++++++++++++++++++++++ 2 files changed, 710 insertions(+), 1 deletion(-) diff --git a/scripts/asvs/scorecard.py b/scripts/asvs/scorecard.py index 33bc1d52..2526fa01 100644 --- a/scripts/asvs/scorecard.py +++ b/scripts/asvs/scorecard.py @@ -19,8 +19,12 @@ from __future__ import annotations import argparse +import ast import re +import shutil +import subprocess import sys +import tempfile import tomllib from collections import Counter from dataclasses import dataclass, field @@ -102,11 +106,43 @@ class Absence: Do NOT derive ``mutation`` from ``pattern``. A value generated from the thing it validates satisfies the check by construction, which would make this the most authoritative-looking vacuous gate in the file — the same defect class it exists to close, arriving through the fix. + + ``mutation_path`` and ``observable`` feed the ``--prove-absences`` mode (:func:`prove_absences`), + which closes a mode the pattern check cannot see: a ``mutation`` that matches its pattern, whose + control speaks and whose corpus is quiet, yet **changes nothing observable when applied** — a + reintroduction raised into a swallowing handler, a field nobody reads, a flag nobody branches on. + ``re.search(pattern, mutation)`` proves the mutation is *well-formed*; it never proves it *bites*. + + - ``mutation_path`` — the file the reintroduction lands in, relative to ``root``. + - ``observable`` — the named artifact that must go red when the mutation is applied: a + ``tests/test_x.py::test_y`` pytest node id. When both fields are set the mode PROVES the claim + by execution — it runs the observable on a scratch copy of the tree (baseline must be green), + applies the mutation, and requires the observable to FAIL (and to fail as a test failure, not a + collection/usage error, which fails closed). When only ``mutation_path`` is set the mode falls + back to a coarse static backstop. + + Both fields default empty, and their absence means **"not yet proven by execution"** — never + "proven vacuous". They are opt-in per claim because a live proof spawns a pytest subprocess per + claim; a claim with neither field is reported as SKIPPED, not failed. + + Honest limits of the proving mode, stated so they are not overclaimed: + + - Application is **append-based**: the mutation text is appended to the scratch target, so it + breaks a fixture by *redefinition shadowing*. That faithfully reddens a well-formed + reintroduction in a fixture; it does not reproduce every in-function reintroduction a real claim + might describe. + - The static backstop is a **coarse same-file heuristic** — a ``raise`` in the mutation landing + lexically in a ``try`` whose every handler swallows (bare/``Exception``, log-only body). It + proves **nothing** in general: it cannot see a swallow in a *caller* rather than at the landing + site, so it would miss the very cross-file instance that motivated this item. It is a screen, + not a proof, and must not be written up as one. """ pattern: str positive_control: str mutation: str + mutation_path: str = "" + observable: str = "" @dataclass(frozen=True) @@ -143,6 +179,13 @@ class Findings: checked_anchors: int = 0 checked_absences: int = 0 skipped_anchors: int = 0 + #: Populated only by :func:`prove_absences`. ``proved_absences`` counts claims whose observable + #: went red under the applied mutation (a live proof); ``static_screened`` counts claims that took + #: the static backstop (a screen, not a proof); ``skipped_absences`` counts claims carrying no + #: ``mutation_path`` (nothing to apply). UNPROVEN and PROVE-ERROR outcomes go into ``problems``. + proved_absences: int = 0 + static_screened: int = 0 + skipped_absences: int = 0 @property def ok(self) -> bool: @@ -271,6 +314,12 @@ def load_scorecard(path: Path) -> list[Cell]: # No default. A missing mutation must be authored, not inferred — see the # Absence docstring on why deriving one from the pattern is worse than none. mutation=str(a["mutation"]), + # Optional, and deliberately NOT refused at load. A hard requirement here would + # void every already-authored absence claim (none carry these yet), and their + # re-authoring is out of this script's reach (ADR 0156 §7). Absent means "not + # yet proven by execution", surfaced by --prove-absences, not "proven vacuous". + mutation_path=str(a.get("mutation_path", "")), + observable=str(a.get("observable", "")), ) for a in raw.get("absence", []) ), @@ -431,6 +480,244 @@ def _grep_count(pattern: str, files: list[Path]) -> int: return sum(1 for f in files if rx.search(f.read_text(encoding="utf-8", errors="replace"))) +# --- proving an absence by mutation (--prove-absences) -------------------------------------------- +# +# check_absences proves a mutation is well-formed (its pattern fires on it). It cannot prove the +# mutation BITES: applied, does anything observable go red? A reintroduction raised into a swallowing +# handler passes every check in check_absences and changes nothing. This mode closes that hole by +# EXECUTING the claim — mutate a scratch copy of the tree, run the named observable, require it to go +# red — and it fails closed on every code that is not an honest test failure, so a typo'd node or an +# already-red observable can never masquerade as "the control bit". The whole pass runs inside a +# TemporaryDirectory scratch copy, so it never mutates `root` and never trips the committed-tree scan +# on itself. + + +def _scratch_ignore(dirpath: str, names: list[str]) -> set[str]: + """Names to skip when copying `root` into the scratch tree. Beyond the usual VCS/venv/cache noise + this refuses secrets and posture data — ``.env*``, ``*.db`` and its WAL sidecars (the local + store), and the vault's ``docs/security`` tree (ADR 0156 §7). The vault runs this module against + the REAL tree, so a scratch copy carrying those would spill them into a world-default temp dir, + which CLAUDE.md §9 forbids this module reading at all. Public-repo runs never see them (no + committed ``.env``/``*.db``, ``docs/security`` absent), so this is defence for the vault run.""" + ignored = set( + shutil.ignore_patterns( + ".git", + ".venv", + "__pycache__", + "node_modules", + ".env", + ".env.*", + "*.db", + "*.db-wal", + "*.db-shm", + )(dirpath, names) + ) + # `docs/security` is path-specific, not a basename glob: skip a `security` entry only directly + # under `docs`, leaving any unrelated `security` elsewhere in the tree copied. + if Path(dirpath).name == "docs" and "security" in names: + ignored.add("security") + return ignored + + +def _copy_scratch(root: Path, dest: Path) -> Path: + """Copy `root` into `dest`, skipping VCS/venv/cache dirs and — defensively, for the vault run — + secrets, the local store, and vault posture data (:func:`_scratch_ignore`). Never writes to + `root`.""" + shutil.copytree(root, dest, ignore=_scratch_ignore) + return dest + + +def _is_within_tree(rel: str) -> bool: + """True only for a repo-relative path with no anchor and no ``..`` component — one that cannot + escape the scratch copy when joined onto it. ``mutation_path`` comes from the authored scorecard, + so it is untrusted for this purpose: an absolute or ``..``-bearing value is refused, not resolved.""" + p = Path(rel) + if p.is_absolute() or p.anchor: + return False + return ".." not in p.parts + + +def _apply_mutation(scratch: Path, mutation_path: str, mutation: str) -> None: + """Append the reintroduction to the scratch target — redefinition shadowing is what makes a + well-formed reintroduction actually break an observable. Never called against `root`.""" + target = scratch / mutation_path + with target.open("a", encoding="utf-8") as fh: + fh.write("\n" + mutation + "\n") + + +def _run_node(scratch: Path, node_id: str, python: str, timeout: float) -> int: + """Run one pytest node inside the scratch copy and return its exit code. + + Invoked with ``--rootdir `` and ``cwd=scratch`` and ``-o addopts=`` so no repo + ``conftest``/``pyproject``/addopts leaks into the child run, and ``-p no:cacheprovider`` so it + writes nothing back. A timeout is treated as a non-{0,1} code — fail closed, never a proof. + """ + try: + proc = subprocess.run( # nosec B603 B607 - fixed argv, no shell; python is sys.executable, node id is scorecard-authored not shell-interpreted + [ + python, + "-m", + "pytest", + "-q", + "--rootdir", + str(scratch), + "-p", + "no:cacheprovider", + "-o", + "addopts=", + node_id, + ], + cwd=scratch, + capture_output=True, + text=True, + check=False, + timeout=timeout, + ) + except subprocess.TimeoutExpired: + return 124 # non-zero and non-1: fails closed as a PROVE-ERROR, never counted as a proof + return proc.returncode + + +def _handler_swallows(handler: ast.ExceptHandler) -> bool: + """A handler that catches broadly (bare or ``Exception``/``BaseException``) with a log-only/``pass`` + body and no re-raise — the shape that eats a reintroduced exception.""" + caught = handler.type + if not ( + caught is None + or (isinstance(caught, ast.Name) and caught.id in {"Exception", "BaseException"}) + ): + return False + if any(isinstance(n, ast.Raise) for stmt in handler.body for n in ast.walk(stmt)): + return False # a re-raise is not a swallow + return all(isinstance(stmt, (ast.Pass, ast.Expr)) for stmt in handler.body) + + +def _landing_swallows(source: str) -> bool: + """True if `source` contains a ``try`` whose EVERY handler swallows (see :func:`_handler_swallows`). + + A coarse same-file heuristic — it proves nothing in general and cannot see a swallow in a caller. + """ + try: + tree = ast.parse(source) + except SyntaxError: + return False + return any( + isinstance(node, ast.Try) + and bool(node.handlers) + and all(_handler_swallows(h) for h in node.handlers) + for node in ast.walk(tree) + ) + + +def _prove_one( + a: Absence, + cell_id: str, + root: Path, + scratch_dir: Path, + findings: Findings, + *, + python: str, + timeout: float, +) -> None: + if not a.mutation_path: + # Nothing to apply. Reported, not failed: opt-in per claim (a live proof spawns a subprocess). + findings.skipped_absences += 1 + return + if not _is_within_tree(a.mutation_path): + # mutation_path is authored data. An absolute path or a `..` escape would let _apply_mutation + # write outside the scratch copy (and the is_file probe below read outside `root`), defeating + # the 'never touches root' guarantee. Refuse it rather than resolve it. + findings.problems.append( + f"{cell_id}: absence claim PROVE-ERROR — mutation_path {a.mutation_path!r} is not a " + "repo-relative path inside the tree (it is absolute or contains '..'), so applying the " + "mutation could escape the scratch copy" + ) + return + target_in_root = root / a.mutation_path + if not target_in_root.is_file(): + findings.problems.append( + f"{cell_id}: absence claim PROVE-ERROR — mutation_path {a.mutation_path!r} is not a file " + "in the tree, so the mutation cannot be applied" + ) + return + + if a.observable: + scratch = _copy_scratch(root, scratch_dir) + baseline = _run_node(scratch, a.observable, python, timeout) + if baseline != 0: + # An already-red or uncollectable observable cannot attribute its red to the mutation. + findings.problems.append( + f"{cell_id}: absence claim PROVE-ERROR — observable {a.observable!r} is not green on " + f"the pristine tree (pytest exit {baseline}); a red or uncollectable baseline cannot " + "be attributed to the mutation" + ) + return + _apply_mutation(scratch, a.mutation_path, a.mutation) + mutated = _run_node(scratch, a.observable, python, timeout) + if mutated == 1: + findings.proved_absences += 1 # live proof: the control bit + elif mutated == 0: + findings.problems.append( + f"{cell_id}: absence claim UNPROVEN — applying the mutation to {a.mutation_path} left " + f"observable {a.observable!r} green (pytest exit 0); the mutation changes nothing the " + "control catches, so this claim is syntax without behaviour" + ) + else: + # exit 2/3/4/5/...: collection or usage error. NEVER a proof — a typo'd node or a mutation + # that merely breaks import must not masquerade as the control biting. + findings.problems.append( + f"{cell_id}: absence claim PROVE-ERROR — mutated run of {a.observable!r} errored " + f"(pytest exit {mutated}) rather than failing; a collection or usage error must not " + "count as the control biting" + ) + return + + # Static backstop: mutation_path but no observable. A screen, not a proof (see Absence docstring). + findings.static_screened += 1 + if re.search(r"\braise\b", a.mutation) and _landing_swallows( + target_in_root.read_text(encoding="utf-8", errors="replace") + ): + findings.problems.append( + f"{cell_id}: absence claim SUSPECT (static heuristic) — its reintroduction raises into " + f"{a.mutation_path}, which has a try/except that swallows (bare or Exception, log-only " + "body), so a live raise there may be caught and prove nothing. Supply an `observable` to " + "prove it by execution" + ) + + +def prove_absences( + cells: list[Cell], + root: Path, + *, + python: str = sys.executable, + timeout: float = 120.0, +) -> Findings: + """Prove each absence claim BITES: apply its mutation to a scratch copy and require its observable + to go red. Fails closed on anything that is not an honest baseline-green / mutated-fail pair. + + This is separate from :func:`verify` and opt-in (``--prove-absences``) because it spawns a pytest + subprocess per provable claim. It never touches `root`. + """ + findings = Findings() + resolved_root = root.resolve() + with tempfile.TemporaryDirectory(prefix="asvs_prove_") as td_base: + base = Path(td_base) + i = 0 + for c in cells: + for a in c.absence: + i += 1 + _prove_one( + a, + c.id, + resolved_root, + base / f"scratch_{i}", + findings, + python=python, + timeout=timeout, + ) + return findings + + def _sort_key(cell_id: str) -> tuple[int, ...]: try: return tuple(int(p) for p in cell_id.split(".")) @@ -596,20 +883,59 @@ def render_current(cells: list[Cell], *, anchor_sha: str) -> str: return chr(10).join(lines) + chr(10) +def _run_prove_absences(scorecard: Path, root: Path) -> int: + """The ``--prove-absences`` entry point: execute-prove every absence claim (see + :func:`prove_absences`). Needs no corpus — it applies mutations, it does not grep for patterns.""" + try: + cells = load_scorecard(scorecard) + except ScorecardError as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 # could not measure — never 0, never confused with "clean" + findings = prove_absences(cells, root) + print( + f"prove-absences: proved {findings.proved_absences} by mutation; " + f"{findings.static_screened} static-screened; {findings.skipped_absences} skipped; " + f"{len(findings.problems)} problem(s)" + ) + for p in findings.problems: + print(f" FAIL {p}", file=sys.stderr) + return 0 if findings.ok else 1 + + def main(argv: list[str] | None = None) -> int: ap = argparse.ArgumentParser(description="Verify or render the ASVS scorecard (ADR 0156).") ap.add_argument("--scorecard", type=Path, required=True) - ap.add_argument("--corpus", type=Path, required=True) + # Not required: --prove-absences applies mutations and never greps for patterns, so it needs no + # corpus. Verify mode still does; that is enforced after parsing, not by argparse. + ap.add_argument("--corpus", type=Path, required=False) ap.add_argument( "--root", type=Path, default=Path.cwd(), help="tree the evidence anchors point into" ) ap.add_argument("--render", type=Path, help="write the generated CURRENT.md here") + # A separate, opt-in mode: prove each absence claim BITES by applying its mutation to a scratch + # copy and requiring its observable to go red. Opt-in because it spawns a pytest subprocess per + # provable claim; kept out of the default verify path, which stays purely static. + ap.add_argument( + "--prove-absences", + action="store_true", + help="execute-prove absence claims (apply mutation to a scratch tree, require observable red)", + ) # NO --anchor-sha injected by CI. The anchor is the commit the EVIDENCE was read on — a property # of the assessment, recorded in [scorecard].anchor_commit. Passing ${{ github.sha }} made the # rendered file differ on every run, so the drift check could never pass: a gate that cannot go # green is as useless as one that cannot go red, and this one shipped that way. args = ap.parse_args(argv) + if args.prove_absences: + return _run_prove_absences(args.scorecard, args.root) + + if args.corpus is None: + print( + "error: --corpus is required to verify the scorecard (only --prove-absences may omit it)", + file=sys.stderr, + ) + return 2 + try: findings = verify(args.scorecard, args.corpus, args.root) except ScorecardError as exc: diff --git a/tests/test_asvs_scorecard.py b/tests/test_asvs_scorecard.py index 0c3b15fb..58fd0bc3 100644 --- a/tests/test_asvs_scorecard.py +++ b/tests/test_asvs_scorecard.py @@ -23,6 +23,7 @@ Cell, Findings, ScorecardError, + _copy_scratch, check_absences, check_anchors, check_completeness, @@ -31,6 +32,8 @@ count, load_corpus, load_scorecard, + main, + prove_absences, render_current, verify, ) @@ -675,3 +678,383 @@ def test_completeness_accepts_a_decided_cell_evidenced_only_by_an_absence_claim( Cell(id="2.1.1", level=3, verdict="unverified"), ] assert not [p for p in check_completeness(cells, CORPUS) if "carry NO anchor" in p] + + +# --- --prove-absences: a mutation that matches is not a mutation that BITES (#1006) --------------- +# +# check_absences proves a mutation's pattern fires on it; it never applies the mutation. So a +# well-formed reintroduction that would change nothing observable passes every check. prove_absences +# closes that hole by EXECUTING the claim: mutate a scratch copy, run the named observable, require it +# to go red -- and fail closed on any exit code that is not an honest test failure. Every fixture tree +# lives in tmp_path (never in the scanned packages), and the mutation is applied only to a scratch +# copy in a system TemporaryDirectory, so nothing here touches the committed corpus or tmp_path. + +_SCANNER = "def scan(p):\n return 'clean'\n" +_OBS_TEST = "from scanner import scan\n\n\ndef test_clean():\n assert scan('x') == 'clean'\n" + + +def _module(tmp_path: Path, name: str, body: str) -> Path: + """Write a code module fixture into tmp_path (the code the reintroduction lands in).""" + p = tmp_path / name + p.write_text(body, encoding="utf-8") + return p + + +def _obs_test(tmp_path: Path, name: str, body: str) -> Path: + """Write an observable pytest module fixture into tmp_path.""" + p = tmp_path / name + p.write_text(body, encoding="utf-8") + return p + + +def _live_claim(mutation: str, mutation_path: str, observable: str) -> Cell: + # pattern/positive_control are irrelevant to prove_absences (they drive check_absences); supply + # harmless values so the required fields are present. + return Cell( + id="1.1.1", + level=1, + verdict="fail", + absence=( + Absence( + pattern="x", + positive_control="y", + mutation=mutation, + mutation_path=mutation_path, + observable=observable, + ), + ), + ) + + +def test_prove_absences_proves_a_claim_when_the_mutation_reddens_its_observable( + tmp_path: Path, +) -> None: + """The positive half: a mutation that shadows `scan` reddens the observable, so the claim BITES. + + Falsified by making `_apply_mutation` a no-op: the observable stays green, the mode reports + UNPROVEN, and the `.ok`/`proved_absences == 1` assertions go RED. Restored. + """ + _module(tmp_path, "scanner.py", _SCANNER) + _obs_test(tmp_path, "test_scanner.py", _OBS_TEST) + claim = _live_claim( + 'def scan(p): return "infected"', "scanner.py", "test_scanner.py::test_clean" + ) + findings = prove_absences([claim], tmp_path) + assert findings.ok, findings.problems + assert findings.proved_absences == 1 + + +def test_prove_absences_fails_when_the_mutation_reddens_nothing(tmp_path: Path) -> None: + """The negative control the brief requires: a mutation to a file the observable never imports + reddens nothing, so the claim is UNPROVEN and the mode FAILS. + + Falsified by making the mode accept a mutated exit==0 as a pass (dropping the exit==1 + requirement): the non-biting claim then reports ok, and `not findings.ok` goes RED -- proving the + mode can actually fail. Restored. + """ + _module(tmp_path, "scanner.py", _SCANNER) + _module( + tmp_path, "unrelated.py", "VALUE = 1\n" + ) # exists, but the observable does not import it + _obs_test(tmp_path, "test_scanner.py", _OBS_TEST) + claim = _live_claim( + 'def scan(p): return "infected"', "unrelated.py", "test_scanner.py::test_clean" + ) + findings = prove_absences([claim], tmp_path) + assert not findings.ok + assert any("UNPROVEN" in p for p in findings.problems), findings.problems + assert findings.proved_absences == 0 + + +def test_prove_absences_fails_closed_when_the_observable_is_already_red(tmp_path: Path) -> None: + """An observable that fails on the pristine tree cannot attribute its red to the mutation. + + Falsified by removing the baseline-green check: the already-red observable stays red under the + mutation, is miscounted as `proved`, and this test's `not findings.ok` goes RED. Restored. + """ + _module(tmp_path, "scanner.py", _SCANNER) + _obs_test( + tmp_path, + "test_scanner.py", + "from scanner import scan\n\n\ndef test_clean():\n assert scan('x') == 'DIFFERENT'\n", + ) + claim = _live_claim( + 'def scan(p): return "infected"', "scanner.py", "test_scanner.py::test_clean" + ) + findings = prove_absences([claim], tmp_path) + assert not findings.ok + assert any("PROVE-ERROR" in p and "baseline" in p for p in findings.problems), findings.problems + assert findings.proved_absences == 0 + + +def test_prove_absences_fails_closed_when_the_mutation_errors_instead_of_failing( + tmp_path: Path, +) -> None: + """A mutation that breaks IMPORT of the observable's module errors at collection (pytest exit 4), + not a test failure (exit 1). A collection/usage error must NEVER count as the control biting -- + otherwise a typo'd node or an import-breaking mutation rebuilds the exact vacuity being fixed. + + Falsified by changing the mutated-run requirement from `exit == 1` to `exit != 0`: the exit-4 + collection error then masquerades as `proved`, and this test's `not findings.ok` goes RED. + Restored. + """ + _module(tmp_path, "scanner.py", _SCANNER) + _obs_test(tmp_path, "test_scanner.py", _OBS_TEST) + # Appended at module level, this raises when `scanner` is imported -> collection error, not a + # failing assertion. + claim = _live_claim( + 'raise RuntimeError("reintroduced")', "scanner.py", "test_scanner.py::test_clean" + ) + findings = prove_absences([claim], tmp_path) + assert not findings.ok + assert any("PROVE-ERROR" in p and "errored" in p for p in findings.problems), findings.problems + assert findings.proved_absences == 0 + + +def test_prove_absences_static_backstop_flags_a_raise_into_a_swallowing_file( + tmp_path: Path, +) -> None: + """With no observable, the static backstop flags a `raise` landing in a file whose try/except + swallows (bare/Exception, log-only body). A screen, not a proof -- but it fails the mode. + + Falsified by forcing `_landing_swallows` to return False: the swallow is not flagged, `findings.ok` + becomes True, and this test's `not findings.ok` goes RED. Restored. + """ + _module( + tmp_path, + "caller.py", + "import logging\n\nlog = logging.getLogger(__name__)\n\n\n" + "def reconcile():\n try:\n work()\n except Exception:\n" + " log.exception('reconcile failed')\n", + ) + claim = Cell( + id="13.3.4", + level=3, + verdict="fail", + absence=( + Absence( + pattern="x", + positive_control="y", + mutation='raise RuntimeError("reintroduced")', + mutation_path="caller.py", + observable="", # no observable -> static backstop + ), + ), + ) + findings = prove_absences([claim], tmp_path) + assert not findings.ok + assert any("SUSPECT" in p and "swallow" in p for p in findings.problems), findings.problems + assert findings.static_screened == 1 + + +def test_prove_absences_static_backstop_passes_a_non_swallowing_file(tmp_path: Path) -> None: + """REACH control: the static backstop must NOT flag a `raise` into a handler that re-raises -- + proving the heuristic reads the handler body, not merely the presence of a try/except. + + Falsified by forcing `_landing_swallows` to return True: the re-raising file is flagged, and this + test's `assert findings.ok` goes RED. Restored. + """ + _module( + tmp_path, + "plain.py", + "def reconcile():\n try:\n work()\n except Exception:\n raise\n", + ) + claim = Cell( + id="13.3.4", + level=3, + verdict="fail", + absence=( + Absence( + pattern="x", + positive_control="y", + mutation='raise RuntimeError("reintroduced")', + mutation_path="plain.py", + observable="", + ), + ), + ) + findings = prove_absences([claim], tmp_path) + assert findings.ok, findings.problems + assert findings.static_screened == 1 + assert not any("SUSPECT" in p for p in findings.problems) + + +def test_prove_absences_leaves_the_root_tree_untouched(tmp_path: Path) -> None: + """The mode must run OUT of the tracked tree: mutation lands only on the scratch copy. + + Falsified by pointing `_apply_mutation` at `root` instead of the scratch copy: root's scanner.py + changes, `after == before` goes RED (and the claim also drops to UNPROVEN). Restored. + """ + _module(tmp_path, "scanner.py", _SCANNER) + _obs_test(tmp_path, "test_scanner.py", _OBS_TEST) + before = {p: p.read_bytes() for p in sorted(tmp_path.rglob("*")) if p.is_file()} + claim = _live_claim( + 'def scan(p): return "infected"', "scanner.py", "test_scanner.py::test_clean" + ) + findings = prove_absences([claim], tmp_path) + assert findings.proved_absences == 1, findings.problems + after = {p: p.read_bytes() for p in sorted(tmp_path.rglob("*")) if p.is_file()} + assert after == before + + +def test_load_reads_optional_mutation_path_and_observable_and_omitting_them_still_loads( + tmp_path: Path, +) -> None: + """Round-trip: the two new fields load when present, and OMITTING them still loads (vault-safety -- + the ~81 existing absence claims carry neither and must stay loadable). + + Falsified by dropping the `.get` wiring in load_scorecard (hardcoding ``mutation_path=""``): half + (a) then reads "" and its assertion goes RED, while half (b) stays green -- proving the round-trip + is actually asserted. Restored. + """ + with_fields = tmp_path / "with.toml" + with_fields.write_text( + '[[cell]]\nid = "1.1.1"\nlevel = 1\nverdict = "fail"\n' + " [[cell.absence]]\n" + ' pattern = "clamd"\n' + ' positive_control = "ScanRejected"\n' + ' mutation = "import clamd"\n' + ' mutation_path = "messagefoundry/scan.py"\n' + ' observable = "tests/test_scan.py::test_rejects"\n', + encoding="utf-8", + ) + a = load_scorecard(with_fields)[0].absence[0] + assert a.mutation_path == "messagefoundry/scan.py" + assert a.observable == "tests/test_scan.py::test_rejects" + + without_fields = tmp_path / "without.toml" + without_fields.write_text( + '[[cell]]\nid = "1.1.2"\nlevel = 2\nverdict = "fail"\n' + " [[cell.absence]]\n" + ' pattern = "clamd"\n' + ' positive_control = "ScanRejected"\n' + ' mutation = "import clamd"\n', + encoding="utf-8", + ) + b = load_scorecard(without_fields)[0].absence[0] + assert b.mutation_path == "" and b.observable == "" + + +def test_prove_absences_refuses_a_mutation_path_that_escapes_the_scratch_tree( + tmp_path: Path, +) -> None: + """`mutation_path` is authored data. An absolute path or a `..` escape would let the mutation land + OUTSIDE the scratch copy (defeating the 'never touches root' guarantee), so the mode refuses it as + a PROVE-ERROR before applying anything -- it never counts as a proof. + + Falsified by making `_is_within_tree` return True unconditionally: the `..` path is no longer + refused, the run falls through to the is_file probe with a different message, and this test's + `any("repo-relative" in p ...)` assertion goes RED. Restored. + """ + _module(tmp_path, "scanner.py", _SCANNER) + _obs_test(tmp_path, "test_scanner.py", _OBS_TEST) + claim = _live_claim( + 'def scan(p): return "infected"', + "../escape.py", # a `..` that would climb out of the scratch copy + "test_scanner.py::test_clean", + ) + findings = prove_absences([claim], tmp_path) + assert not findings.ok + assert any("PROVE-ERROR" in p and "repo-relative" in p for p in findings.problems), ( + findings.problems + ) + assert findings.proved_absences == 0 + + +def test_copy_scratch_excludes_secrets_store_and_vault_posture(tmp_path: Path) -> None: + """The scratch copy the vault mutation-run reads must never carry secrets, the local store, or the + vault posture tree -- CLAUDE.md §9 forbids this module reading them at all. `_copy_scratch` skips + `.env*`, `*.db`(+WAL sidecars), and `docs/security`, while ordinary sources are still copied. + + Falsified by reverting `_scratch_ignore` to the bare VCS/venv/cache patterns: the `.env`, `*.db` + and `docs/security` fixtures are then copied into the scratch dir and every `not (dest/...).exists()` + assertion goes RED, while the `keep.py` assertion stays green. Restored. + """ + (tmp_path / ".env").write_text("EXAMPLE_PLACEHOLDER=not-a-secret\n", encoding="utf-8") + (tmp_path / "local.db").write_text("binary-store\n", encoding="utf-8") + (tmp_path / "local.db-wal").write_text("wal\n", encoding="utf-8") + (tmp_path / "docs" / "security").mkdir(parents=True) + (tmp_path / "docs" / "security" / "posture.toml").write_text("real = true\n", encoding="utf-8") + (tmp_path / "docs" / "PUBLIC.md").write_text("# public\n", encoding="utf-8") + (tmp_path / "keep.py").write_text("KEEP = 1\n", encoding="utf-8") + + dest = tmp_path.parent / "scratch_out" + _copy_scratch(tmp_path, dest) + + assert not (dest / ".env").exists() + assert not (dest / "local.db").exists() + assert not (dest / "local.db-wal").exists() + assert not (dest / "docs" / "security").exists() + # ordinary sources and other docs survive the copy + assert (dest / "keep.py").read_text(encoding="utf-8") == "KEEP = 1\n" + assert (dest / "docs" / "PUBLIC.md").exists() + + +def _biting_scorecard(sc: Path, mutation_path: str) -> None: + """Write a one-claim scorecard whose live absence claim points at `mutation_path`.""" + sc.write_text( + '[[cell]]\nid = "1.1.1"\nlevel = 1\nverdict = "fail"\n' + " [[cell.absence]]\n" + ' pattern = "x"\n' + ' positive_control = "y"\n' + " mutation = 'def scan(p): return \"infected\"'\n" + f' mutation_path = "{mutation_path}"\n' + ' observable = "test_scanner.py::test_clean"\n', + encoding="utf-8", + ) + + +def test_main_prove_absences_returns_0_on_a_biting_claim(tmp_path: Path) -> None: + """The CLI contract CI depends on: `--prove-absences` exits 0 when every claim's mutation reddens + its observable. Exercises `main` -> `_run_prove_absences` end to end, not just `prove_absences`. + + Falsified by changing `_run_prove_absences`'s `return 0 if findings.ok else 1` to `return 1`: this + test's `rc == 0` goes RED while the non-biting test below stays green. Restored. + """ + tree = tmp_path / "tree" + tree.mkdir() + _module(tree, "scanner.py", _SCANNER) + _obs_test(tree, "test_scanner.py", _OBS_TEST) + sc = tmp_path / "sc.toml" + _biting_scorecard(sc, "scanner.py") + rc = main(["--scorecard", str(sc), "--root", str(tree), "--prove-absences"]) + assert rc == 0 + + +def test_main_prove_absences_returns_1_on_a_nonbiting_claim(tmp_path: Path) -> None: + """The other half of the contract: `--prove-absences` exits 1 when a claim is UNPROVEN (its + mutation reddens nothing). Proves the CLI's non-zero failure path, not only the library's. + + Falsified by changing `_run_prove_absences`'s `return 0 if findings.ok else 1` to `return 0`: + this test's `rc == 1` goes RED while the biting test above stays green. Restored. + """ + tree = tmp_path / "tree" + tree.mkdir() + _module(tree, "scanner.py", _SCANNER) + _module(tree, "unrelated.py", "VALUE = 1\n") # present, but the observable never imports it + _obs_test(tree, "test_scanner.py", _OBS_TEST) + sc = tmp_path / "sc.toml" + _biting_scorecard(sc, "unrelated.py") + rc = main(["--scorecard", str(sc), "--root", str(tree), "--prove-absences"]) + assert rc == 1 + + +def test_main_verify_without_corpus_returns_exit_2(tmp_path: Path) -> None: + """Verify mode needs the corpus; omitting `--corpus` (without `--prove-absences`) must exit 2 -- + could-not-measure, never confused with a clean 0. Proves the argparse-independent guard in `main`. + + Falsified by deleting the `if args.corpus is None: ... return 2` branch in `main`: it then falls + through to `verify(...)` with `corpus=None`, raising instead of returning 2, and this test's + `rc == 2` goes RED. Restored. + """ + sc = tmp_path / "sc.toml" + sc.write_text( + '[[cell]]\nid = "1.1.1"\nlevel = 1\nverdict = "fail"\n' + " [[cell.absence]]\n" + ' pattern = "x"\n' + ' positive_control = "y"\n' + ' mutation = "import x"\n', + encoding="utf-8", + ) + rc = main(["--scorecard", str(sc), "--root", str(tmp_path)]) + assert rc == 2 From 089bcdaeb32bf5c10cc55c3e46b3ade277500df7 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 02:03:25 -0500 Subject: [PATCH 02/14] docs(backlog): flip #1006 to shipped -- the absence gate can now prove behaviour (BACKLOG #1006) Flip the #1006 banner from filed to shipped. It is written as a capability claim, not a closure claim: the `--prove-absences` mode CAN catch a well-formed-but-vacuous reintroduction once a claim carries an `observable`, but the default `verify` path is byte-unchanged and no authored claim carries one yet, so nothing new is blocked by this alone today -- the honest present-tense state for a not-deployed beta. Banner lines of #1006 ONLY. The ranked table, the four census distribution lines, and every other item's banner are untouched. The status census was NOT recomputed. --- docs/BACKLOG.md | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 38305897..065e36db 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -3922,13 +3922,21 @@ record and the cell's current score live in the vault scorecard and are not rest ## 1006. A mutation that matches is not a mutation that bites: the absence-claim gate proves syntax, never behaviour -> 🔢 **Filed 2026-08-04 — not started. Scored 2026-08-04 → P2.** Value **6/10** · Difficulty -> **3/10** · _quick win_. `check_absences` admits an ASVS absence claim on `re.search(a.pattern, -> a.mutation)` (`scripts/asvs/scorecard.py:395`) — one string field of a TOML row matched against -> another — so a well-formed reintroduction that would change nothing if applied passes all three -> of the gate's failure modes and certifies a non-control into the compliance record; the -> remainder is a required per-claim observable plus a mode that applies the mutation and requires -> that observable to go red, in one stdlib script and its fixture tests. +> ✅ **SHIPPED 2026-08-06 — a new opt-in mode can prove an absence claim BITES, which the pattern +> check structurally cannot.** Value **6/10** · Difficulty **3/10** · _quick win_. +> `scripts/asvs/scorecard.py` gains a `--prove-absences` mode: per claim it applies the `mutation` to +> a scratch copy of the tree and requires a named `observable` (a pytest node id) to go RED, failing +> closed on any exit code that is not an honest test failure (an already-red baseline, an +> uncollectable node, or a mutation that only breaks import is a PROVE-ERROR, never a proof). So a +> well-formed reintroduction that would change nothing if applied CAN be caught the moment its claim +> carries an `observable` — but the default `verify` path is byte-unchanged and no authored claim +> carries one yet, so nothing new is blocked by this alone today. Two optional `Absence` fields +> (`mutation_path`, `observable`) feed it, a coarse same-file static backstop screens claims that +> carry no observable, the scratch copy refuses secrets / the store / `docs/security` (defence for the +> eventual vault run), and fixture negative controls plus a CLI exit-code test prove the mode itself +> can go red. Public repo script + fixtures only; wiring the mode over the vault's ~81 existing +> absence claims (untouched) and backfilling their observables is the owner's follow-up +> (`scorecard.py:14-16`, ADR 0156 §7). **Cluster:** Security & Compliance. **Priority:** P2. **Verdict:** build. **Severity:** medium — the defect is in the instrument, not the engine, and a green instrument that cannot go red is the From 3450c3f6aa1c64ee09e79b02b6ce7d9d83807aea Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 02:18:01 -0500 Subject: [PATCH 03/14] test(connscale): dynamic contiguous inbound-port allocation, drop the flaky marker (BACKLOG #1014) The connscale SQLite smoke test hard-coded base_port=41000 and needs 24 contiguous inbound ports, so two worktrees running the suite at once contended for the same fixed block; a @pytest.mark.flaky(reruns=2) marker retried past the collision, relabelling a determinate resource conflict as CI noise. On the first parallel run it would keep masking exactly this class. Replace the fixed block with _free_contiguous_ports(), which anchors an n-wide block at a RANDOM base inside a bounded window, probes each port with a no-REUSEADDR bind, and returns the range only when all n bind. The random anchor over a wide window de-correlates concurrent worktrees; a genuine future collision now surfaces as a red, not a masked retry. Contiguity is asserted at the acquisition site and the allocator fails loudly -- never a silent fixed fallback -- via two branches: an up-front width guard when the block cannot fit the window, and a post-loop raise when no free block is found after `tries` attempts. The window is [20000,30000): the lower bound sits ABOVE the sibling MLLP fixed-port band (other tests bind fixed inbound ports in the 11xxx-19xxx range, e.g. 15099/19601), and the upper bound stays BELOW the OS ephemeral floors (Linux 32768+, Windows/macOS 49152+) so a kernel-assigned ephemeral port -- the sink/API ports, or any unrelated connection -- can never land in the block after it is probed. Drop the @pytest.mark.flaky marker: the collision was the cause, so keeping it would re-hide the class this removes. Add three helper tests -- contiguity and in-window, post-loop exhaustion (tries=0), and the width guard -- each pinned to its branch (match=) and falsified by mutation. Test-only change; no product code is touched. --- tests/test_connscale_smoke.py | 102 +++++++++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 7 deletions(-) diff --git a/tests/test_connscale_smoke.py b/tests/test_connscale_smoke.py index 38a6cbc5..cf18fd1e 100644 --- a/tests/test_connscale_smoke.py +++ b/tests/test_connscale_smoke.py @@ -18,6 +18,7 @@ from __future__ import annotations +import random import socket import sys @@ -28,6 +29,22 @@ pytestmark = pytest.mark.timeout(120) # the per-test 60s default is too tight for two engine spawns +# The connection-count sweep; its max sets the contiguous inbound-port width (BACKLOG #1014). +_SMOKE_COUNTS = (12, 24) + +# Contiguous inbound-port window for the random anchor (BACKLOG #1014). Both bounds are +# chosen to keep the block clear of ports OTHER tests bind, so a concurrent worktree's +# connscale block cannot land on a sibling's fixed listener: +# - LOWER bound sits ABOVE the sibling fixed-port MLLP band. Sibling tests bind fixed +# inbound ports in the 11xxx-19xxx range (e.g. 15099, 19601); anchoring at 20000+ keeps +# the connscale block entirely above them. [20000,30000) is empty of fixed test binds. +# - UPPER bound stays BELOW the OS ephemeral floors (Linux 32768+, Windows/macOS 49152+) +# so a kernel-assigned ephemeral port -- the sink/API ports from _free_port(), or any +# unrelated connection -- can never land inside the block after it is probed. +# The upper bound is exclusive. +_INBOUND_PORT_LO = 20000 +_INBOUND_PORT_HI = 30000 + def _free_port() -> int: s = socket.socket() @@ -39,12 +56,52 @@ def _free_port() -> int: s.close() +def _free_contiguous_ports(n: int, *, tries: int = 200) -> list[int]: + """Reserve ``n`` contiguous free inbound ports anchored at a RANDOM base. + + The random anchor is the concurrency fix (BACKLOG #1014): it de-correlates worktrees so + two suites rarely pick overlapping blocks. The old fixed ``base_port = 41000`` guaranteed + a collision whenever two checkouts ran the suite at once. Probe/bind-and-release only holds + the block momentarily, so it cannot truly reserve it against a concurrent engine -- the + random anchor over a wide window is the real defense, and a genuine future collision now + surfaces as a RED rather than a masked retry. + """ + if _INBOUND_PORT_HI - n <= _INBOUND_PORT_LO: + raise RuntimeError( + f"cannot reserve {n} contiguous ports in [{_INBOUND_PORT_LO},{_INBOUND_PORT_HI})" + ) + for _ in range(tries): + base = random.randint(_INBOUND_PORT_LO, _INBOUND_PORT_HI - n) + socks: list[socket.socket] = [] + try: + for i in range(n): + s = socket.socket() + # No SO_REUSEADDR on purpose: honest free-detection. A live listener must make + # bind FAIL here, unlike SO_REUSEADDR's Windows steal semantics. The block is + # released before the engine binds, so REUSEADDR would only add false-frees. + try: + s.bind(("127.0.0.1", base + i)) + except OSError: + s.close() + break + socks.append(s) + if len(socks) == n: + return list(range(base, base + n)) + finally: + for sock in socks: + sock.close() + raise RuntimeError( + f"could not reserve {n} contiguous free ports in " + f"[{_INBOUND_PORT_LO},{_INBOUND_PORT_HI}) after {tries} tries" + ) + + def _smoke_profile(base_port: int) -> object: # Small N (12 → 24) + short holds so the smoke fits the pytest budget; both sweep modes by default. return load_connscale_profile_text(f""" [connscale] name = "smoke-it" -counts = [12, 24] +counts = {list(_SMOKE_COUNTS)} sweep_mode = "both" aggregate_rate = 24.0 per_conn_rate = 1.0 @@ -66,13 +123,18 @@ def _smoke_profile(base_port: int) -> object: """) -@pytest.mark.flaky( - reruns=2, reruns_delay=3 -) # CI runners are noisy (mf-ci-test-flakes): re-run clears async def test_connscale_smoke_end_to_end() -> None: - # Reserve a base inbound-port block that won't collide with the sink/API ports. The 24-conn max - # sweep needs 24 contiguous inbound ports; pick a high base well clear of the ephemeral churn. - base_port = 41000 + # Dynamically reserve a contiguous inbound-port block (BACKLOG #1014). The sweep's max + # connection count needs that many contiguous inbound ports, and the engine binds + # base_port + i for each. A RANDOM anchor de-correlates concurrent worktrees so they no + # longer contend for one fixed block; contiguity is asserted at the acquisition site, and + # the allocator fails loudly if no free block can be found (never a silent fixed fallback). + # The sink/API ports stay ephemeral (above the inbound window) and won't hit the block. + inbound_ports = _free_contiguous_ports(max(_SMOKE_COUNTS)) + assert inbound_ports == list(range(inbound_ports[0], inbound_ports[0] + max(_SMOKE_COUNTS))), ( + inbound_ports + ) + base_port = inbound_ports[0] sink_port = _free_port() api_port = _free_port() profile = _smoke_profile(base_port) @@ -148,3 +210,29 @@ def test_fd_sampler_reads_self() -> None: assert live is None or live > 0 # None only if the OS tool is unavailable on this runner dead = FdSampler(2**31 - 1).sample() # an implausible PID assert dead is None + + +def test_free_contiguous_ports_are_contiguous_and_in_window() -> None: + # The allocator returns exactly n ascending, contiguous ports inside the window. It + # deliberately does NOT re-bind to "prove free" -- that is TOCTOU-racy and would reintroduce + # the exact flake class BACKLOG #1014 removes. + ports = _free_contiguous_ports(8) + assert len(ports) == 8 + assert ports == list(range(ports[0], ports[0] + 8)) + assert ports[0] >= _INBOUND_PORT_LO + assert ports[-1] < _INBOUND_PORT_HI + + +def test_free_contiguous_ports_fails_loud_when_unsatisfiable() -> None: + # tries=0 hits the post-loop exhaustion branch deterministically (without occupying the + # whole window) and must raise -- never fall back silently to a fixed port (BACKLOG #1014). + with pytest.raises(RuntimeError, match="could not reserve"): + _free_contiguous_ports(8, tries=0) + + +def test_free_contiguous_ports_fails_loud_when_window_too_narrow() -> None: + # The width guard fires BEFORE any probing when the requested block cannot fit the window + # at all: asking for one more port than the window holds can never be satisfied, so it + # raises up front rather than looping (BACKLOG #1014 -- fail loud, never a silent fallback). + with pytest.raises(RuntimeError, match="cannot reserve"): + _free_contiguous_ports(_INBOUND_PORT_HI - _INBOUND_PORT_LO + 1) From 43f0595d8adfe2eac7f846943f50a5cad04f5dd1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 02:18:40 -0500 Subject: [PATCH 04/14] backlog: close #1014 -- dynamic connscale port allocation ships, flaky marker dropped Flip #1014's status banner from open (filed) to shipped: the dynamic contiguous inbound-port allocation and the flaky-marker removal land in the same branch (commit 3450c3f6). This edits the #1014 banner line ONLY. The ranked table and the four census distribution lines are untouched, and the census was NOT recomputed. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 38305897..c8f102e2 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -4713,7 +4713,7 @@ Retiring the tree costs the engine nothing operationally: **`tests/test_ech_egre ## 1014. connscale smoke test's fixed 24-port block is not parallel-safe across worktrees; the flaky marker hides the collision -> 🔢 **Filed 2026-08-04 — not started.** Value **5/10** · Difficulty **3/10** · _fill-in_. `test_connscale_smoke_end_to_end` hard-codes `base_port = 41000` and requires 24 **contiguous** inbound ports, so two checkouts running the suite at once contend for the same block. A `@pytest.mark.flaky` marker retries past the collision, so a determinate resource conflict wears a noise label. +> ✅ **SHIPPED 2026-08-06 — dynamic contiguous inbound-port allocation replaces the fixed 24-port block; the flaky marker is dropped.** Value **5/10** · Difficulty **3/10** · _fill-in_. `test_connscale_smoke_end_to_end` now reserves a random contiguous inbound-port block at runtime (`_free_contiguous_ports`), asserts contiguity at acquisition, and fails loudly if no free block is found, so a genuine cross-worktree collision surfaces as a red rather than a masked retry. **Cluster:** Testing / CI reliability. **Priority:** P3. **Verdict:** build (small). **Severity:** low — it costs retries and misdiagnosis, not correctness. From 7cdfa520559e7b7816b6b589927c57c9bee8a1ce Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 02:25:45 -0500 Subject: [PATCH 05/14] feat(anon): structural PHI-shape detectors + coverage report + token-floor signal on the leak-check (BACKLOG #331) The fail-closed leak-check verified only that MAPPED fields were pseudonymized; PHI sitting in a field no rule mapped would pass the check clean on first deployment (a real MRN is not a denylisted string). Scoped to the fields anonymize did NOT rewrite, add: - high-precision structural detectors over unmapped fields: dashed SSN, punctuated NANP phone, and CX MR/MRN-typed identifier. Deliberately narrow, to avoid the mass false-positives a broad digit-run search produces on HL7 bodies dense with dates/order-numbers/set-ids (ADR 0030 section 5). - LeakReport: an unmapped-field coverage report (addresses only, never a value) plus token_tables_live / token_floor_reason. Reasons name the shape + field ADDRESS only, so a raised LeakError / log line never carries PHI. - token_floor_failure() folded into the fail-closed decision under the require_live_denylist opt-IN lever (default off, so token-less CI/OSS/fork runs stay green with the structural detectors as the live backstop). The whole structural block is mirrored byte-identical into tee/anon/leak.py; a new engine/tee leak_report parity test pins it. Each detector was falsified (removed it, watched the unmapped-PHI dataset slip through, restored); a false-positive guard proves a benign unmapped field (14-digit EVN timestamp, order number, coded observation id) does not trip. Docs are written to the shipped DEFAULT behaviour, not an overclaim: the coverage report and token-floor reason are RECORDED and surfaced on a refusal or via the on_report hook (not an unconditional clean-path catch-all), and the strict refusal is opt-IN. anon/__init__.py (both copies) necessarily changed to export LeakReport/leak_report/coverage_clause and wire the lever. ADR 0030 section 5 / section 7 / Consequences amended (the "deferred" phrasing was stale against the shipped code). NOT-DEPLOYED beta: worded as "would let unmapped PHI through on first deployment", no present-tense exposure claim. --- .../0030-anonymization-test-harness-tee.md | 27 ++- messagefoundry/anon/__init__.py | 46 ++++- messagefoundry/anon/leak.py | 192 ++++++++++++++++-- tee/anon/__init__.py | 38 +++- tee/anon/leak.py | 178 +++++++++++++++- tests/test_anon_core.py | 134 ++++++++++++ tests/test_anon_parity.py | 43 ++++ 7 files changed, 608 insertions(+), 50 deletions(-) diff --git a/docs/adr/0030-anonymization-test-harness-tee.md b/docs/adr/0030-anonymization-test-harness-tee.md index d1d76204..013dafee 100644 --- a/docs/adr/0030-anonymization-test-harness-tee.md +++ b/docs/adr/0030-anonymization-test-harness-tee.md @@ -15,6 +15,18 @@ file-replay loader ([`harness/reconcile/compare.py`](../../harness/reconcile/compare.py), [`harness/load/corpus.py`](../../harness/load/corpus.py)); and the publish leak-gate [`scripts/security/scan_forbidden.py`](../../scripts/security/scan_forbidden.py) (`FORBIDDEN`). +- **AMENDED 2026-08-06 (BACKLOG #331):** the leak-check's structural gap that §5 / §7 / Consequences + below called *scoped out / a deferred improvement* is now **built** — high-precision structural + PHI-shape detectors (dashed SSN, punctuated NANP phone, CX `MR`/`MRN`-typed identifier) over **the + fields no rule matched**, an unmapped-field **coverage report** (`LeakReport.unmapped_fields`; address + only, never a value, carried into the `LeakError` on a refusal and exposed via `on_report`), and the + `token_floor_failure` signal recorded in every report and folded into the fail-closed decision under + the `require_live_denylist` **opt-in** lever — default off, so a token-less CI/OSS/fork load still + passes with the structural detectors as the live backstop; a deployment that must refuse on an + unloaded denylist sets the lever. Mirrored behaviourally-identical into `tee/anon/leak.py`. The + *aggressive/broad* shape tier (bare-digit DOB/SSN, name-like runs) stays deferred — it + mass-false-positives on HL7 bodies (§5). This bullet is the single source of the change; the stale + "deferred" phrasings below point back here. - **Decision in one line:** ship a **pure-stdlib, dependency-free `anon` package** that turns real, messy HL7 v2 into structurally-faithful **PHI-free** datasets via a **two-layer rule model — a declarative field-*selection* map (data) over a code registry of pure surrogate *functions* (logic)** — @@ -266,8 +278,11 @@ silently misses a field is worse than none** — though note the limit in the ne PHI the rule map **missed** sails through the fail-closed gate *clean* unless that field happens to contain a denylisted token — a real MRN is not a denylisted string. **Rule-map completeness is therefore the primary control; the leak-check backstops known *strings*, not missed *fields*.** Adding structural detectors -(MRN/SSN/DOB/phone shape, NANP-reserved vs real) to the post-anon check as a *true* field-level backstop is a -candidate improvement, scoped out for this slice with the residual called out (Consequences). +(MRN/SSN/DOB/phone shape, NANP-reserved vs real) to the post-anon check as a *true* field-level backstop was a +candidate improvement — **built in BACKLOG #331** (AMENDED 2026-08-06; see the status banner): the +high-precision set (dashed SSN, punctuated NANP phone, CX `MR`/`MRN`-typed id) scoped to the UNMAPPED fields, +plus an unmapped-field coverage report (`LeakReport.unmapped_fields`) recorded on every pass and surfaced on +a refusal or via the `on_report` hook. The broad-shape tier stays scoped out (below). ### 6. Integration points @@ -294,7 +309,9 @@ candidate improvement, scoped out for this slice with the residual called out (C 0004](0004-payload-agnostic-ingress.md)) so X12 (`parsing/x12/`)/FHIR/raw plug in later — never HL7-parse a non-HL7 body. **Out:** statistical expert-determination de-id, free-text NLP scrubbing (NTE-3/OBX-5 narrative default to **blunt full-redaction**, §3 — not entity-level NLP), **per-component (bare-leaf) surrogates** (the -default encoders are field-level `^`-joined, §3), structural PHI detectors in the leak-check (§5), and any +default encoders are field-level `^`-joined, §3), an **aggressive/broad structural PHI search** (bare-digit +DOB/SSN, name-like alpha runs — the mass-false-positive tier; note the *high-precision* unmapped-field +detectors in the leak-check were **built in #331**, AMENDED 2026-08-06, §5), and any re-identification/linkage tooling. ### 8. Relationship to the planned de-id framework @@ -340,7 +357,9 @@ AI-assistant `deidentified` data-scope source (PHI.md §9 forward-links to it on not MRN/SSN/DOB/name *shapes*, so a field the rule map missed passes the "fail-closed" gate clean unless it contains a denylisted token. **Rule-map completeness is the primary control**; the leak-check is necessary, not sufficient. Free-text (OBX-5/NTE-3) is the **highest-risk residual** — hence its full-redact default - (§3). Structural detectors are a deferred improvement (§5/§7). + (§3). **AMENDED 2026-08-06 (#331):** high-precision structural detectors over the UNMAPPED fields plus an + unmapped-field coverage report are now **built** (that deferral is closed; the broad-shape tier stays + deferred, §7) — rule-map completeness remains the primary control. - **Vendored-copy drift — bytes *and* behaviour.** Two copies can diverge; the rule/surrogate/token *files* are mitigated by a CI **byte-parity** check (the existing tee discipline) under one authority. But the engine-side `anon/hl7.py` (delegating to `Message`) and the tee's **standalone stdlib re-encoder** are diff --git a/messagefoundry/anon/__init__.py b/messagefoundry/anon/__init__.py index 18ac3a94..a896602d 100644 --- a/messagefoundry/anon/__init__.py +++ b/messagefoundry/anon/__init__.py @@ -15,19 +15,22 @@ Public surface: * :func:`anonymize` — de-identify one HL7 message (raises nothing PHI-bearing). -* :func:`anonymize_checked` — :func:`anonymize` + a **fail-closed** :func:`leak_check`; raises - :class:`LeakError` (token categories only, never the value) if any known partner/site token - survives. This is how you *earn* the right to write a dataset to a shareable location. -* :func:`leak_check` — forbidden-token hits via the publish-guard authority (ADR 0030 §5). +* :func:`anonymize_checked` — :func:`anonymize` + a **fail-closed** :func:`leak_report`; raises + :class:`LeakError` (token categories + PHI shapes/addresses only, never a value) if any known + partner/site token survives **or** a structural PHI shape sits in a field no rule mapped. This is + how you *earn* the right to write a dataset to a shareable location. +* :func:`leak_check` / :func:`leak_report` — token hits + structural PHI-shape detection over the + unmapped fields + the unmapped-field coverage report (ADR 0030 §5, BACKLOG #331). """ from __future__ import annotations +from collections.abc import Callable from pathlib import Path from .hl7 import anonymize_message from .keying import Keyer -from .leak import LeakCheckUnavailable, leak_check +from .leak import LeakCheckUnavailable, LeakReport, coverage_clause, leak_check, leak_report from .rules import DEFAULT_RULES, AnonError, FieldRule, RuleError, SurrogateKind, load_rules __all__ = [ @@ -37,11 +40,13 @@ "Keyer", "LeakCheckUnavailable", "LeakError", + "LeakReport", "RuleError", "SurrogateKind", "anonymize", "anonymize_checked", "leak_check", + "leak_report", "load_rules", ] @@ -79,18 +84,37 @@ def anonymize_checked( salt: str, overlay: Path | None = None, rules: tuple[FieldRule, ...] | None = None, + require_live_denylist: bool = False, + on_report: Callable[[LeakReport], None] | None = None, ) -> str: - """:func:`anonymize`, then a fail-closed :func:`leak_check`; raise :class:`LeakError` on any hit. + """:func:`anonymize`, then a fail-closed :func:`leak_report`; raise :class:`LeakError` on any hit. Use this whenever the output may be persisted/shared — a silently-missed token is worse than no - anonymization (ADR 0030 §5). The raised error names token *categories* only, never the value. + anonymization (ADR 0030 §5). The verification is now two-layered (BACKLOG #331): the known-token + denylist **and** high-precision structural PHI-shape detectors over the fields no rule matched, + scoped by the same ``rules`` the anonymizer applied. The raised error names token *categories* and + field *shapes/addresses* only, never a value, and carries a coverage clause (the count + addresses + of the unmapped fields, whether the denylist tables were live) so a refusal is legible. + + ``require_live_denylist`` makes a non-live token source (``token_floor_reason`` set) a refusal + cause in its own right — the strict lever for a deployment that must not de-identify with the + customer denylist unloaded. It defaults **off**: the structural detectors are the live backstop, + and CI/OSS/fork runs legitimately have no token source. ``on_report`` receives the full + :class:`LeakReport` on both the clean and the refusing path (default: no emission). """ - output = anonymize(raw, salt=salt, overlay=overlay, rules=rules) - hits = leak_check(output) - if hits: + effective = rules if rules is not None else load_rules(overlay) + output = anonymize(raw, salt=salt, rules=effective) + report = leak_report(output, rules=effective) + if on_report is not None: + on_report(report) + causes = list(report.hits) + if require_live_denylist and report.token_floor_reason is not None: + causes.append(f"denylist not live: {report.token_floor_reason}") + if causes: raise LeakError( "anonymized output still carries forbidden token(s): " - + "; ".join(sorted(set(hits))) + + "; ".join(sorted(set(causes))) + " — refusing to emit (fail closed). Extend the rule map for the missed field(s)." + + coverage_clause(report) ) return output diff --git a/messagefoundry/anon/leak.py b/messagefoundry/anon/leak.py index df744318..24469f73 100644 --- a/messagefoundry/anon/leak.py +++ b/messagefoundry/anon/leak.py @@ -9,21 +9,39 @@ its importable :func:`scan_text`. The standalone ``tee/anon/leak.py`` vendors the same token data (held identical by the parity test), since the tee cannot reach ``scripts/``. -The check is the **fail-closed backstop**, not the primary control: it catches known *tokens*, not -structural PHI (a missed MRN field with no denylisted string sails through) — rule-map completeness -is the primary control (ADR 0030 §5). It is loaded lazily from the source checkout; an installed -wheel without ``scripts/`` raises a clear error (the anonymizer is a dev/migration tool, always run -from a checkout). +The token denylist alone is a **backstop that only catches known *strings***: a real MRN in a field +the rule map never mapped is not a denylisted token, so it would sail through clean. Two structural +controls close that (BACKLOG #331), scoped to the fields ``anonymize`` did **not** rewrite so an +already-pseudonymized field is never re-flagged: + +* an **unmapped-field coverage report** — every present-but-unmapped field is enumerated in the + :class:`LeakReport` (address only, never its value), so the check's reach is legible and a field + nobody thought to map is **recorded** (carried into the :class:`LeakError` on a refusal, and exposed + via ``on_report``) rather than passing unrecorded; and +* **high-precision structural PHI-shape detectors** over those unmapped values (dashed SSN, + punctuated NANP phone, CX ``MR``/``MRN``-typed identifier) — narrow by design to avoid the mass + false-positives a broad digit-run search would produce on HL7 bodies (ADR 0030 §5). + +The token-floor signal (``token_floor_failure``) is recorded in every report +(``token_floor_reason``/``token_tables_live``) and folded into the fail-closed decision when +``require_live_denylist`` is set (default off), so a token-less load is legible to any caller that +inspects the report or opts into refusing on it. + +Loaded lazily from the source checkout; an installed wheel without ``scripts/`` raises a clear error +(the anonymizer is a dev/migration tool, always run from a checkout). """ from __future__ import annotations import importlib.util +import re +from dataclasses import dataclass from functools import lru_cache from pathlib import Path from types import ModuleType -from .surrogates import message_has_site_code +from .rules import FieldRule +from .surrogates import Seps, message_has_site_code, read_message_seps class LeakCheckUnavailable(RuntimeError): @@ -47,16 +65,158 @@ def _scanner() -> ModuleType: ) -def leak_check(text: str) -> list[str]: - """Forbidden-token hits in ``text`` (empty list = clean), using the publish-guard's authority. +# --- structural PHI-shape detection over UNMAPPED fields (BACKLOG #331) ---------------------------- +# EVERYTHING from here to the end of this block is held BYTE-IDENTICAL with tee/anon/leak.py (the +# structural walk depends only on read_message_seps, which the parity test pins byte-for-byte). The +# detectors are deliberately high-precision — a broad digit-run search mass-false-positives on HL7 +# bodies dense with dates/order-numbers/set-ids (ADR 0030 §5), so the coverage report, not an +# aggressive heuristic, is the catch-all for shapes these cannot safely flag. + +#: A dashed US SSN ``NNN-NN-NNNN`` not embedded in a longer digit run. +_SSN_DASHED: re.Pattern[str] = re.compile(r"(? list[tuple[str, str]]: + """Every ``(address, value)`` in ``text`` whose whole-field ``SEG-i`` address is **not** in + ``mapped_paths`` and whose value is non-empty — the fields the rule map never touched. + + The MSH control header is skipped whole: its field indexing is off-by-one (MSH-N sits at + split-index N-1) and it carries routing/site data the field-anchored site-code pass already + covers, not patient PHI. ``mapped_paths`` is occurrence-agnostic (a rule applies to every + occurrence of its segment), so the address is the bare ``SEG-i``. Returns ``[]`` when the message + has no parseable MSH (there is no field separator to split on). """ - hits = [str(h) for h in _scanner().scan_text(text, include_estate=True)] - if message_has_site_code(text): - hits.append("site-code pattern") + parsed = read_message_seps(text) + if parsed is None: + return [] + _seps, field_sep = parsed + out: list[tuple[str, str]] = [] + for seg in text.replace("\r\n", "\r").replace("\n", "\r").split("\r"): + if not seg: + continue + fields = seg.split(field_sep) + if fields[0].upper() == "MSH": + continue + seg_id = fields[0] + for i in range(1, len(fields)): + value = fields[i] + if not value: + continue + address = f"{seg_id}-{i}" + if address in mapped_paths: + continue + out.append((address, value)) + return out + + +def _has_mrn_typed_identifier(value: str, seps: Seps) -> bool: + """True if any repetition of ``value`` is a CX with a non-empty id (component 1) and a whole + ``MR``/``MRN`` id-type component — an unmapped medical-record number by HL7 structure, far more + precise than a bare digit-run heuristic.""" + for rep in value.split(seps.repetition): + comps = rep.split(seps.component) + if comps[0] and any(comp.upper() in _MRN_TYPES for comp in comps): + return True + return False + + +def _structural_reasons(value: str, seps: Seps) -> list[str]: + """PHI-safe shape labels for one unmapped field value — the SHAPE only, never the value.""" + reasons: list[str] = [] + if _SSN_DASHED.search(value): + reasons.append("unmapped SSN-shaped value") + if _PHONE_DASHED.search(value) or _PHONE_PAREN.search(value): + reasons.append("unmapped phone-shaped value") + if _has_mrn_typed_identifier(value, seps): + reasons.append("unmapped MRN-typed identifier") + return reasons + + +def structural_phi_hits(text: str, mapped_paths: set[str]) -> list[str]: + """Structural PHI-shape hits over the fields no rule matched — reasons name the shape + field + ADDRESS only (e.g. ``"unmapped SSN-shaped value in GT1-16"``), never the offending value, so the + result is safe to raise/log. Empty when the message has no parseable MSH.""" + parsed = read_message_seps(text) + if parsed is None: + return [] + seps, _field_sep = parsed + hits: list[str] = [] + for address, value in unmapped_field_values(text, mapped_paths): + hits.extend(f"{reason} in {address}" for reason in _structural_reasons(value, seps)) return hits + + +@dataclass(frozen=True) +class LeakReport: + """The full result of a leak-check pass — the token hits that decide the fail-closed outcome plus + the coverage context that makes the check's reach legible (all PHI-safe: addresses and reasons, + never a field value). + + * ``hits`` — every leak reason (token/IP/site + structural); non-empty means refuse. + * ``unmapped_fields`` — the addresses present but matched by no rule (the coverage report). + * ``structural_hits`` — the subset of ``hits`` from the structural PHI-shape detectors. + * ``token_tables_live`` — whether the denylist tables loaded from a real token source. + * ``token_floor_reason`` — why the denylist is not trustworthy, or ``None`` if it is. + """ + + hits: list[str] + unmapped_fields: tuple[str, ...] + structural_hits: list[str] + token_tables_live: bool + token_floor_reason: str | None + + +def leak_report(text: str, *, rules: tuple[FieldRule, ...] | None = None) -> LeakReport: + """The full :class:`LeakReport` for ``text`` using the publish-guard's token authority. + + Token hits use the guard's substring + estate-token mode (ADR 0030 §5) plus a field-anchored + site-code check. Structural detection engages **only when ``rules`` is supplied** — the address + of every unmapped field is derived from ``{r.path for r in rules}`` and the high-precision + detectors run over those fields. With ``rules is None`` (a direct/legacy call over a bare string) + structural detection is skipped and the result is byte-for-byte the legacy token-only behaviour. + """ + scanner = _scanner() + token_hits = [str(h) for h in scanner.scan_text(text, include_estate=True)] + if message_has_site_code(text): + token_hits.append("site-code pattern") + if rules is None: + unmapped: tuple[str, ...] = () + structural: list[str] = [] + else: + mapped_paths = {r.path for r in rules} + unmapped = tuple(sorted({addr for addr, _ in unmapped_field_values(text, mapped_paths)})) + structural = structural_phi_hits(text, mapped_paths) + token_tables_live = bool(scanner.TOKENS_PRESENT) + token_floor_reason: str | None = scanner.token_floor_failure() + return LeakReport( + hits=token_hits + structural, + unmapped_fields=unmapped, + structural_hits=structural, + token_tables_live=token_tables_live, + token_floor_reason=token_floor_reason, + ) + + +def leak_check(text: str, *, rules: tuple[FieldRule, ...] | None = None) -> list[str]: + """Forbidden-token + structural PHI hits in ``text`` (empty list = clean) — a thin wrapper over + :func:`leak_report`. ``rules`` scopes the structural PHI-shape detectors to the fields no rule + matched; omitting it (a bare-string call) runs the legacy token-only check. + """ + return leak_report(text, rules=rules).hits + + +def coverage_clause(report: LeakReport) -> str: + """A PHI-safe suffix for a fail-closed message naming what the check reached — the count and + ADDRESSES of the unmapped fields (never their values) and whether the denylist tables were live.""" + live = "yes" if report.token_tables_live else "no" + fields = ", ".join(report.unmapped_fields) if report.unmapped_fields else "none" + return ( + f" (checked {len(report.unmapped_fields)} unmapped field(s): {fields}; " + f"denylist tables live: {live})" + ) diff --git a/tee/anon/__init__.py b/tee/anon/__init__.py index 51bd77a4..df8cdb07 100644 --- a/tee/anon/__init__.py +++ b/tee/anon/__init__.py @@ -11,18 +11,21 @@ Public surface (same shape as the engine's): * :func:`anonymize` — de-identify one HL7 message. -* :func:`anonymize_checked` — :func:`anonymize` + a fail-closed :func:`leak_check`; raises - :class:`LeakError` (token categories only) on any surviving token. -* :func:`leak_check` — forbidden-token hits via the vendored token authority. +* :func:`anonymize_checked` — :func:`anonymize` + a fail-closed :func:`leak_report`; raises + :class:`LeakError` (token categories + PHI shapes/addresses only) on any surviving token or a + structural PHI shape in a field no rule mapped. +* :func:`leak_check` / :func:`leak_report` — token hits + structural PHI-shape detection over the + unmapped fields + the unmapped-field coverage report (vendored twin of the engine's; BACKLOG #331). """ from __future__ import annotations +from collections.abc import Callable from pathlib import Path from .hl7 import anonymize_message from .keying import Keyer -from .leak import leak_check +from .leak import LeakReport, coverage_clause, leak_check, leak_report from .rules import DEFAULT_RULES, AnonError, FieldRule, RuleError, SurrogateKind, load_rules __all__ = [ @@ -31,11 +34,13 @@ "FieldRule", "Keyer", "LeakError", + "LeakReport", "RuleError", "SurrogateKind", "anonymize", "anonymize_checked", "leak_check", + "leak_report", "load_rules", ] @@ -67,14 +72,29 @@ def anonymize_checked( salt: str, overlay: Path | None = None, rules: tuple[FieldRule, ...] | None = None, + require_live_denylist: bool = False, + on_report: Callable[[LeakReport], None] | None = None, ) -> str: - """:func:`anonymize`, then a fail-closed :func:`leak_check`; raise :class:`LeakError` on any hit.""" - output = anonymize(raw, salt=salt, overlay=overlay, rules=rules) - hits = leak_check(output) - if hits: + """:func:`anonymize`, then a fail-closed :func:`leak_report`; raise :class:`LeakError` on any hit. + + Two-layered like the engine's (BACKLOG #331): the known-token denylist plus high-precision + structural PHI-shape detectors over the fields no rule matched. ``require_live_denylist`` (default + off) makes a non-live token source a refusal cause; ``on_report`` receives the :class:`LeakReport` + on both paths. The error names token categories and field shapes/addresses only, never a value. + """ + effective = rules if rules is not None else load_rules(overlay) + output = anonymize(raw, salt=salt, rules=effective) + report = leak_report(output, rules=effective) + if on_report is not None: + on_report(report) + causes = list(report.hits) + if require_live_denylist and report.token_floor_reason is not None: + causes.append(f"denylist not live: {report.token_floor_reason}") + if causes: raise LeakError( "anonymized output still carries forbidden token(s): " - + "; ".join(sorted(set(hits))) + + "; ".join(sorted(set(causes))) + " — refusing to emit (fail closed). Extend the rule map for the missed field(s)." + + coverage_clause(report) ) return output diff --git a/tee/anon/leak.py b/tee/anon/leak.py index 0b7c5568..fc041a85 100644 --- a/tee/anon/leak.py +++ b/tee/anon/leak.py @@ -14,17 +14,21 @@ no-op for those (a public checkout has no customer estate to leak). The generic IP detector keeps a literal default so the anonymizer's structural IP check still functions without a token source. -Returns **reasons only** (never the matched text), and is the fail-closed backstop, not the primary -control: it catches known *tokens*, not structural PHI (ADR 0030 §5). +Returns **reasons only** (never the matched text), and the token denylist is the fail-closed +*backstop*, not the primary control: it catches known *tokens*, not structural PHI (ADR 0030 §5). The +structural PHI-shape detectors + unmapped-field coverage report below (BACKLOG #331) close that gap — +they are held byte-for-byte identical with the engine copy so the two agree on every input. """ from __future__ import annotations import importlib.util import re +from dataclasses import dataclass from pathlib import Path -from .surrogates import message_has_site_code +from .rules import FieldRule +from .surrogates import Seps, message_has_site_code, read_message_seps def _load_publish_guard(_start: Path | None = None) -> object | None: @@ -91,11 +95,165 @@ def scan_text(text: str, *, include_estate: bool = False) -> list[str]: return reasons -def leak_check(text: str) -> list[str]: - """Forbidden-token hits in ``text`` (empty = clean) — the tee's fail-closed leak gate. The site - code is checked **field-anchored** (matching the replace path), so a scrub miss is caught without - false-positiving on a value that merely contains a site-code run.""" - hits = scan_text(text, include_estate=True) - if message_has_site_code(text): - hits.append("site-code pattern") +# --- structural PHI-shape detection over UNMAPPED fields (BACKLOG #331) ---------------------------- +# EVERYTHING from here to the end of this block is held BYTE-IDENTICAL with tee/anon/leak.py (the +# structural walk depends only on read_message_seps, which the parity test pins byte-for-byte). The +# detectors are deliberately high-precision — a broad digit-run search mass-false-positives on HL7 +# bodies dense with dates/order-numbers/set-ids (ADR 0030 §5), so the coverage report, not an +# aggressive heuristic, is the catch-all for shapes these cannot safely flag. + +#: A dashed US SSN ``NNN-NN-NNNN`` not embedded in a longer digit run. +_SSN_DASHED: re.Pattern[str] = re.compile(r"(? list[tuple[str, str]]: + """Every ``(address, value)`` in ``text`` whose whole-field ``SEG-i`` address is **not** in + ``mapped_paths`` and whose value is non-empty — the fields the rule map never touched. + + The MSH control header is skipped whole: its field indexing is off-by-one (MSH-N sits at + split-index N-1) and it carries routing/site data the field-anchored site-code pass already + covers, not patient PHI. ``mapped_paths`` is occurrence-agnostic (a rule applies to every + occurrence of its segment), so the address is the bare ``SEG-i``. Returns ``[]`` when the message + has no parseable MSH (there is no field separator to split on). + """ + parsed = read_message_seps(text) + if parsed is None: + return [] + _seps, field_sep = parsed + out: list[tuple[str, str]] = [] + for seg in text.replace("\r\n", "\r").replace("\n", "\r").split("\r"): + if not seg: + continue + fields = seg.split(field_sep) + if fields[0].upper() == "MSH": + continue + seg_id = fields[0] + for i in range(1, len(fields)): + value = fields[i] + if not value: + continue + address = f"{seg_id}-{i}" + if address in mapped_paths: + continue + out.append((address, value)) + return out + + +def _has_mrn_typed_identifier(value: str, seps: Seps) -> bool: + """True if any repetition of ``value`` is a CX with a non-empty id (component 1) and a whole + ``MR``/``MRN`` id-type component — an unmapped medical-record number by HL7 structure, far more + precise than a bare digit-run heuristic.""" + for rep in value.split(seps.repetition): + comps = rep.split(seps.component) + if comps[0] and any(comp.upper() in _MRN_TYPES for comp in comps): + return True + return False + + +def _structural_reasons(value: str, seps: Seps) -> list[str]: + """PHI-safe shape labels for one unmapped field value — the SHAPE only, never the value.""" + reasons: list[str] = [] + if _SSN_DASHED.search(value): + reasons.append("unmapped SSN-shaped value") + if _PHONE_DASHED.search(value) or _PHONE_PAREN.search(value): + reasons.append("unmapped phone-shaped value") + if _has_mrn_typed_identifier(value, seps): + reasons.append("unmapped MRN-typed identifier") + return reasons + + +def structural_phi_hits(text: str, mapped_paths: set[str]) -> list[str]: + """Structural PHI-shape hits over the fields no rule matched — reasons name the shape + field + ADDRESS only (e.g. ``"unmapped SSN-shaped value in GT1-16"``), never the offending value, so the + result is safe to raise/log. Empty when the message has no parseable MSH.""" + parsed = read_message_seps(text) + if parsed is None: + return [] + seps, _field_sep = parsed + hits: list[str] = [] + for address, value in unmapped_field_values(text, mapped_paths): + hits.extend(f"{reason} in {address}" for reason in _structural_reasons(value, seps)) return hits + + +@dataclass(frozen=True) +class LeakReport: + """The full result of a leak-check pass — the token hits that decide the fail-closed outcome plus + the coverage context that makes the check's reach legible (all PHI-safe: addresses and reasons, + never a field value). + + * ``hits`` — every leak reason (token/IP/site + structural); non-empty means refuse. + * ``unmapped_fields`` — the addresses present but matched by no rule (the coverage report). + * ``structural_hits`` — the subset of ``hits`` from the structural PHI-shape detectors. + * ``token_tables_live`` — whether the denylist tables loaded from a real token source. + * ``token_floor_reason`` — why the denylist is not trustworthy, or ``None`` if it is. + """ + + hits: list[str] + unmapped_fields: tuple[str, ...] + structural_hits: list[str] + token_tables_live: bool + token_floor_reason: str | None + + +def leak_report(text: str, *, rules: tuple[FieldRule, ...] | None = None) -> LeakReport: + """The full :class:`LeakReport` for ``text`` using the tee's vendored token authority. + + Behaviourally parallel to the engine's :func:`messagefoundry.anon.leak.leak_report`: the token + hits come from the tee's local :func:`scan_text` (and the field-anchored site-code check) rather + than the engine's ``_scanner()`` delegate, but the structural walk, coverage report, and + token-floor signal are the byte-identical shared logic above. Structural detection engages **only + when ``rules`` is supplied**; a bare-string call is the legacy token-only behaviour. + """ + token_hits = scan_text(text, include_estate=True) + if message_has_site_code(text): + token_hits.append("site-code pattern") + if rules is None: + unmapped: tuple[str, ...] = () + structural: list[str] = [] + else: + mapped_paths = {r.path for r in rules} + unmapped = tuple(sorted({addr for addr, _ in unmapped_field_values(text, mapped_paths)})) + structural = structural_phi_hits(text, mapped_paths) + token_tables_live: bool + token_floor_reason: str | None + if _GUARD is not None: + token_tables_live = bool(_GUARD.TOKENS_PRESENT) # type: ignore[attr-defined] + token_floor_reason = _GUARD.token_floor_failure() # type: ignore[attr-defined] + else: + token_tables_live = False + # nosec B105: a human-readable diagnostic string, not a credential — bandit's + # hardcoded-password heuristic fires only because the name contains "token". + token_floor_reason = "no publish guard reachable — refusing to run structural-only" # nosec B105 + return LeakReport( + hits=token_hits + structural, + unmapped_fields=unmapped, + structural_hits=structural, + token_tables_live=token_tables_live, + token_floor_reason=token_floor_reason, + ) + + +def leak_check(text: str, *, rules: tuple[FieldRule, ...] | None = None) -> list[str]: + """Forbidden-token + structural PHI hits in ``text`` (empty list = clean) — a thin wrapper over + :func:`leak_report`. ``rules`` scopes the structural PHI-shape detectors to the fields no rule + matched; omitting it (a bare-string call) runs the legacy token-only check. + """ + return leak_report(text, rules=rules).hits + + +def coverage_clause(report: LeakReport) -> str: + """A PHI-safe suffix for a fail-closed message naming what the check reached — the count and + ADDRESSES of the unmapped fields (never their values) and whether the denylist tables were live.""" + live = "yes" if report.token_tables_live else "no" + fields = ", ".join(report.unmapped_fields) if report.unmapped_fields else "none" + return ( + f" (checked {len(report.unmapped_fields)} unmapped field(s): {fields}; " + f"denylist tables live: {live})" + ) diff --git a/tests/test_anon_core.py b/tests/test_anon_core.py index 4a3a21e8..dc45e48f 100644 --- a/tests/test_anon_core.py +++ b/tests/test_anon_core.py @@ -22,6 +22,7 @@ anonymize_checked, leak, leak_check, + leak_report, load_rules, ) from messagefoundry.anon.surrogates import Seps, scrub_site_codes, surrogate_field @@ -256,6 +257,139 @@ def test_anonymize_checked_fails_closed_and_is_phi_safe(monkeypatch: pytest.Monk assert "DOE" not in message and "999" not in message # never echoes the body +# --- structural PHI detection on UNMAPPED fields (BACKLOG #331) ------------------------------------ +# The known-token denylist cannot see a real MRN/SSN in a field the rule map never mapped (a real MRN +# is not a denylisted string). These exercise the structural backstop over the UNMAPPED fields. All +# values are SYNTHETIC PHI SHAPES (fake, reserved-fictional, or component-structured) — never a real +# value — and each detector is falsified in the lane report. `DST` is a non-standard segment carrying +# no default rule, so DST-2/3 are the unmapped surface (the f3c6d348 blind-map case in miniature). + +_SSN_MSG = _msg( + r"MSH|^~\&|SAPP|SFAC|RAPP|RFAC|20260101120000||ADT^A01|M1|P|2.5.1", + "PID|1||1^^^H^MR||X^Y", + "DST|1|123-45-6789", # DST-2: unmapped field carrying a synthetic dashed SSN +) + + +@_NO_SCANNER +def test_leak_check_catches_unmapped_ssn() -> None: + """A synthetic dashed SSN in an unmapped field (DST-2) is caught and fails closed. + + Falsified: deleting `_SSN_DASHED` from leak.py's structural set made leak_check() return [] and + anonymize_checked() emit the dataset clean (RED), then restored. + """ + hits = leak_check(_SSN_MSG, rules=DEFAULT_RULES) + assert any("SSN" in h for h in hits), hits + with pytest.raises(LeakError): + anonymize_checked(_SSN_MSG, salt=_SALT) + + +@_NO_SCANNER +def test_leak_check_catches_unmapped_phone() -> None: + """Synthetic punctuated NANP numbers (reserved-fictional 555-01XX) in unmapped fields are caught, + both dashed and parenthesised. + + Falsified: removing the two phone detectors let the dataset slip through clean (RED), then restored. + """ + msg = _msg( + r"MSH|^~\&|A|B|C|D|20260101||ADT^A01|M1|P|2.5.1", + "PID|1||1^^^H^MR||X^Y", + "DST|1|202-555-0188|(202) 555-0188", # DST-2 dashed, DST-3 parenthesised + ) + hits = leak_check(msg, rules=DEFAULT_RULES) + assert any("phone" in h for h in hits), hits + assert any("DST-2" in h for h in hits) and any("DST-3" in h for h in hits), hits + + +@_NO_SCANNER +def test_leak_check_catches_unmapped_mrn() -> None: + """A CX id typed `MR` in an unmapped field (PID-2, absent from DEFAULT_RULES) is caught by HL7 + structure, and the raw id never surfaces in the reason or the LeakError (PHI-safe). + + Falsified: removing the MR/MRN component detector let the unmapped MRN pass clean (RED), then + restored — confirming the CX id-type signal, not a digit heuristic, is doing the work. + """ + msg = _msg( + r"MSH|^~\&|A|B|C|D|20260101||ADT^A01|M1|P|2.5.1", + "PID|1|98765^^^HOSP^MR||X^Y", # PID-2: unmapped CX, id-typed MR + ) + hits = leak_check(msg, rules=DEFAULT_RULES) + assert any("MRN" in h and "PID-2" in h for h in hits), hits + assert all("98765" not in h for h in hits) # names the shape + address, never the id + with pytest.raises(LeakError) as exc: + anonymize_checked(msg, salt=_SALT) + assert "98765" not in str(exc.value) + + +@_NO_SCANNER +def test_coverage_report_lists_unmapped_fields() -> None: + """The coverage report enumerates present-but-unmapped fields (address only) — the batch_18 + regression: a field nobody mapped is now visible, not silent. The fail-path LeakError carries the + value-free coverage clause. + + Falsified: stubbing `unmapped_field_values` to yield nothing emptied `.unmapped_fields` (RED), + then restored. + """ + benign = _msg( + r"MSH|^~\&|A|B|C|D|20260101120000||ADT^A01|M1|P|2.5.1", + "PID|1||1^^^H^MR||X^Y", + "DST|1|freeform", # DST-2: unmapped but benign — enumerated, not flagged + ) + report = leak_report(benign, rules=DEFAULT_RULES) + assert "DST-2" in report.unmapped_fields + assert report.structural_hits == [] # benign value → enumerated only, no shape hit + with pytest.raises(LeakError) as exc: + anonymize_checked(_SSN_MSG, salt=_SALT) + text = str(exc.value) + assert "checked" in text and "unmapped field" in text and "DST-2" in text + + +@_NO_SCANNER +def test_false_positive_guard_benign_unmapped_fields() -> None: + """Unmapped fields dense with dates/coded-values/order-numbers (the mass-false-positive surface + ADR 0030 warns of) must NOT trip the check — why the bare-digit DOB/SSN heuristics were rejected. + + Falsified: broadening `_SSN_DASHED` to any 8+ digit run tripped the 14-digit EVN timestamp (RED), + then restored. + """ + benign = _msg( + r"MSH|^~\&|A|B|C|D|20260101120000||ADT^A01|M1|P|2.5.1", + "EVN|A01|20260101120000", # 14-digit timestamp + "OBX|1|NM|8480-6^Systolic^LN||128|mm[Hg]", # coded observation id + "ORC|NW|1000000042", # unmapped order-number run + "PID|1||1^^^H^MR||X^Y", + ) + assert leak_check(benign, rules=DEFAULT_RULES) == [] + assert anonymize_checked(benign, salt=_SALT) # clean → returns, no raise + + +@_NO_SCANNER +def test_token_floor_surfaced_when_tables_empty(monkeypatch: pytest.MonkeyPatch) -> None: + """An empty token load is no longer a SILENT green (#331): the report records it and the strict + lever refuses on it — while the default keeps CI/OSS/fork runs (which have no token source) green, + the structural detectors being the live backstop. + + The empty-token state is forced deterministically (this dev checkout has a token source; CI does + not) by patching the loaded scanner's `TOKENS_PRESENT`. Falsified: stubbing `token_floor_failure` + to return None made `.token_floor_reason` None and the strict path stop refusing (RED), restored. + """ + monkeypatch.setattr(leak._scanner(), "TOKENS_PRESENT", False) + clean = _msg( + r"MSH|^~\&|A|B|C|D|20260101120000||ADT^A01|M1|P|2.5.1", + "PID|1||1^^^H^MR||X^Y", + ) + report = leak_report(clean, rules=DEFAULT_RULES) + assert report.token_tables_live is False + assert report.token_floor_reason is not None + # the DEFAULT decision does NOT refuse on empty tokens alone (structural detectors are the backstop) + assert anonymize_checked(clean, salt=_SALT) + # the strict lever DOES refuse, naming the floor reason but no field value + with pytest.raises(LeakError) as exc: + anonymize_checked(clean, salt=_SALT, require_live_denylist=True) + text = str(exc.value) + assert "denylist not live" in text and "fail closed" in text + + def test_alphanumeric_identifier_preserves_width_and_shape() -> None: msg = _msg( r"MSH|^~\&|A|B|C|D|20260101||ADT^A01|M1|P|2.5.1", diff --git a/tests/test_anon_parity.py b/tests/test_anon_parity.py index 0ddac6e7..d5d05c05 100644 --- a/tests/test_anon_parity.py +++ b/tests/test_anon_parity.py @@ -11,7 +11,9 @@ import pytest +from messagefoundry.anon import DEFAULT_RULES from messagefoundry.anon import anonymize as engine_anonymize +from messagefoundry.anon import leak as engine_leak from messagefoundry.generators import ( _core, _hl7data, @@ -111,6 +113,47 @@ def test_leak_tables_are_sourced_from_the_guard_when_present() -> None: assert tee_leak.FORBIDDEN and tee_leak.ESTATE_TOKENS # type: ignore[attr-defined] +# Synthetic-PHI-shape inputs (never a real value) whose UNMAPPED fields carry SSN/phone/MRN shapes, +# plus a benign case — the structural walk + coverage report + token-floor signal must agree between +# the engine (delegating to _scanner()) and the tee (reimplementing over _GUARD). The structural block +# is copied byte-for-byte between the two leak.py files; this is the divergence guard for it. +_LEAK_PARITY_INPUTS = [ + "MSH|^~\\&|A|B|C|D|20260101||ADT^A01|M1|P|2.5.1\rPID|1||1^^^H^MR||X^Y\rDST|1|123-45-6789", + "MSH|^~\\&|A|B|C|D|20260101||ADT^A01|M1|P|2.5.1\rPID|1||1^^^H^MR||X^Y" + "\rDST|1|202-555-0188|(202) 555-0188", + "MSH|^~\\&|A|B|C|D|20260101||ADT^A01|M1|P|2.5.1\rPID|1|98765^^^HOSP^MR||X^Y", + "MSH|^~\\&|A|B|C|D|20260101120000||ADT^A01|M1|P|2.5.1" + "\rEVN|A01|20260101120000\rOBX|1|NM|8480-6^Systolic^LN||128|mm[Hg]", +] + + +def _structural_fields(report: object) -> tuple[object, ...]: + """The STRUCTURAL/coverage fields of a LeakReport — the byte-copied #331 logic this guard pins. + + ``token_tables_live`` / ``token_floor_reason`` are intentionally excluded: they are derived from + the token authority, not the structural walk, and the engine's ``_scanner()`` (lazily lru_cached + on first call) and the tee's ``_GUARD`` (loaded at import) can snapshot the token source at + different times within a full-suite run — a fixture that patches ``MEFOR_FORBIDDEN_TOKENS`` before + the first ``_scanner()`` call poisons its token-floor view for the session. Those fields' cross- + copy agreement is already pinned by ``test_leak_token_table_matches_publish_guard``; here we guard + the detectors + coverage report, which are pure functions of (text, rules). + """ + return (report.hits, report.unmapped_fields, report.structural_hits) # type: ignore[attr-defined] + + +def test_leak_check_and_report_engine_equals_tee() -> None: + for msg in _LEAK_PARITY_INPUTS: + eng_hits = engine_leak.leak_check(msg, rules=DEFAULT_RULES) + tee_hits = tee_leak.leak_check(msg, rules=DEFAULT_RULES) + assert eng_hits == tee_hits, f"leak_check diverged on {msg!r}: {eng_hits!r} != {tee_hits!r}" + eng_report = _structural_fields(engine_leak.leak_report(msg, rules=DEFAULT_RULES)) + tee_report = _structural_fields(tee_leak.leak_report(msg, rules=DEFAULT_RULES)) + assert eng_report == tee_report, ( + f"leak_report structural fields diverged on {msg!r}:" + f"\n ENG {eng_report!r}\n TEE {tee_report!r}" + ) + + def test_adversarial_inputs_engine_output_equals_tee_output() -> None: for msg in _ADVERSARIAL: engine = engine_anonymize(msg, salt=_SALT) From dfa3d6f1d102314e038806ac1657e3fc2f3ef36b Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 02:26:09 -0500 Subject: [PATCH 06/14] docs(backlog): flip #331 banner to SHIPPED, worded to the default behaviour (BACKLOG #331) Banner-only flip of #331 to SHIPPED. Worded to the shipped DEFAULT behaviour, not an overclaim: the coverage report and token_floor_reason are RECORDED and surfaced on a refusal or via the on_report hook (not an unconditional clean-path catch-all), and require_live_denylist is the strict opt-IN lever (default off). Census NOT recomputed: only the #331 banner line changed. The ranked table, the four census distribution lines, and every other item's banner are untouched. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 38305897..6c131f44 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -2914,7 +2914,7 @@ That distinction matters concretely for the ASVS record. The scorecard's absence ## 331. Anonymizer's fail-closed leak-check has no structural PHI detectors -> 🔢 **Filed 2026-08-01 — not started.** Value **6/10** · Difficulty **4/10** · _quick win_. The function that earns the right to share a de-identified dataset verifies a known-string denylist — `leak_check` is `scan_text` (FORBIDDEN patterns, one routable-IPv4 check, estate substrings; `scripts/security/scan_forbidden.py:772-795`) plus a field-anchored site code, and a real MRN is not a denylisted string — and on a token-less checkout it degrades to the IPv4 check alone over an HL7 body and still returns clean, a gap `f3c6d348` hit in practice with a hand overlay that was never committed; wiring `token_floor_failure()` into the bridge is small, but the unmapped-field report and detectors scoped to fields no rule matched cross the `anonymize` seam and must be mirrored into `tee/anon/leak.py` for `test_anon_parity`. +> ✅ **SHIPPED 2026-08-06 — structural PHI-shape detectors + unmapped-field coverage report + token-floor signal built.** Value **6/10** · Difficulty **4/10** · _quick win_. `leak_check`/`leak_report` now run high-precision structural detectors (dashed SSN, punctuated NANP phone, CX `MR`/`MRN`-typed identifier) over **the fields no rule matched**, record every present-but-unmapped field in a coverage report (`LeakReport.unmapped_fields`, carried into the `LeakError` on a refusal and exposed via the `on_report` hook), and record `token_floor_failure()` in every report, folding it into the fail-closed decision under the `require_live_denylist` **opt-in** lever — default off, so a token-less CI/OSS/fork load still passes with the structural detectors as the live backstop; a deployment that must refuse on an unloaded denylist sets the lever. The whole structural block is mirrored byte-identical into `tee/anon/leak.py` with a new engine/tee `leak_report` parity test; each detector was falsified. ADR 0030 §5/§7/Consequences amended (the "deferred" phrasing was stale). The aggressive/broad-shape tier (bare-digit DOB/SSN, name-like runs) stays deferred by owner call — it mass-false-positives on HL7 bodies dense with dates/order-numbers. **Cluster:** Security & Compliance. **Priority:** P2. **Verdict:** build. **Severity:** medium. From 9409984d051cda690f598c2494ab745891fef922 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 04:44:12 -0500 Subject: [PATCH 07/14] test(sandbox): a static ast guard pins the codec+worker import boundary (BACKLOG #346) The sandbox's import boundary (DEFAULT_FORBIDDEN_MODULES in pipeline/sandbox.py -- socket/ssl/asyncio, the I/O- and secret-bearing messagefoundry.* subpackages, cryptography) is enforced only at RUNTIME and only inside the off-by-default [sandbox].mode=subprocess child. Nothing statically pins that the two modules which run inside that boundary -- _sandbox_codec.py and _sandbox_worker.py -- do not themselves import a forbidden module. Both are clean today; a future edit reintroducing a forbidden import would make mode=subprocess DOA on first deployment while the default-mode suite stayed green -- the failure inverts, hitting the most security-conscious installs hardest and quietest. This is defence-in-depth test coverage, not a code change: neither sandbox.py nor the codec is touched. tests/test_sandbox_import_boundary.py walks the two files' own ast import nodes (ast.Import/ast.ImportFrom, including nested/function-level and relative imports resolved to absolute) and asserts none resolves under a DEFAULT_FORBIDDEN_MODULES prefix. The forbidden set is imported from the runtime constant, never copied, so the guard tracks whatever the sandbox forbids. It ships with a positive control (each static import form the walker handles is seen, including the load-bearing from-parent alias-append) and a negative control (benign messagefoundry.* imports raise zero flags). Scope is the two files' DIRECT imports, deliberately not a transitive walk: importing the codec pulls asyncio/cryptography/store/transports/auth into sys.modules, so a transitive walker would red on clean shipped code and prove nothing. sandbox.py is out of scope per BACKLOG #346 even though the worker child imports it; the docstring records that residual for the owner. Falsified: planting `import socket` into the real _sandbox_codec.py reddens the live guard naming it; removing the walker's alias-append reddens only the alias-append positive-control case; an over-broad matcher reddens the negative control. All plants restored before commit. --- tests/test_sandbox_import_boundary.py | 155 ++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 tests/test_sandbox_import_boundary.py diff --git a/tests/test_sandbox_import_boundary.py b/tests/test_sandbox_import_boundary.py new file mode 100644 index 00000000..0b2abbf8 --- /dev/null +++ b/tests/test_sandbox_import_boundary.py @@ -0,0 +1,155 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Static import-boundary guard for the sandbox worker files (ADR 0087, BACKLOG #346). + +The sandbox draws its trust boundary at runtime: :class:`_ForbiddenImportFinder` (in +``_sandbox_worker``) refuses ``DEFAULT_FORBIDDEN_MODULES`` — ``socket``/``ssl``/``asyncio`` and the +I/O- and secret-bearing ``messagefoundry.*`` subpackages — but ONLY inside the ``[sandbox].mode= +subprocess`` child, which is not the default. So nothing statically pins that the two modules which +run *inside* that boundary (:mod:`messagefoundry.pipeline._sandbox_codec` and +:mod:`messagefoundry.pipeline._sandbox_worker`) do not themselves import a forbidden module. They are +clean today; a future edit that violated it would fail **only** on a deployment that turned the +sandbox on for security reasons, behind a green default-mode suite. + +This is defence-in-depth test coverage, not a code fix: the guard below walks the two files' own +``ast`` import nodes and asserts none resolves under a ``DEFAULT_FORBIDDEN_MODULES`` prefix. The +forbidden set is IMPORTED from the runtime constant, never copied, so the test tracks whatever the +sandbox actually forbids. Scope is those two files' **direct** imports — deliberately not a +transitive walk: importing the codec pulls asyncio/cryptography/store/transports/auth into +``sys.modules``, so a transitive walker would be red on clean shipped code and prove nothing. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +from messagefoundry.pipeline.sandbox import DEFAULT_FORBIDDEN_MODULES + +_PIPELINE = Path(__file__).resolve().parents[1] / "messagefoundry" / "pipeline" + +#: The two modules BACKLOG #346 scopes: the length-prefixed codec both ends of the pipe speak, and the +#: ``python -m`` worker child entrypoint. ``sandbox.py`` is out of that scope even though the worker +#: child also imports it (``_sandbox_worker.main`` pulls ``_read_frame_bytes``/``_write_frame`` from it, +#: before the runtime guard goes up) — it imports nothing forbidden today (stdlib + ``config.*`` + the +#: codec). A forbidden top-level import newly added to ``sandbox.py`` would evade THIS static pin (and +#: its already-bound reference would even survive the guard's ``sys.modules`` purge), so widening the +#: walk to it is a scope decision left to the owner, not a gap this two-file guard silently covers. +_TARGETS: tuple[tuple[str, Path], ...] = ( + ("messagefoundry.pipeline._sandbox_codec", _PIPELINE / "_sandbox_codec.py"), + ("messagefoundry.pipeline._sandbox_worker", _PIPELINE / "_sandbox_worker.py"), +) + + +def _is_forbidden(module: str) -> bool: + """True if ``module`` is a forbidden module or a submodule of one. + + Mirrors :meth:`_ForbiddenImportFinder.find_spec`'s prefix match by construction (``name == prefix + or name.startswith(prefix + ".")``), so the static check agrees exactly with the runtime one.""" + return any(module == p or module.startswith(p + ".") for p in DEFAULT_FORBIDDEN_MODULES) + + +def _walk(source: str, dotted: str) -> tuple[set[str], int]: + """Derive the imported dotted names from ``source`` (a module whose own name is ``dotted``). + + Returns ``(candidate module names, import-node count)``. ``ast.walk`` catches nested/function-level + imports for free. For ``from X import a, b`` the plain module ``X`` **and** ``X.a`` / ``X.b`` are + both candidates — the alias-append is what catches ``from messagefoundry import store`` (a + forbidden subpackage imported off the non-forbidden ``messagefoundry`` parent). Relative imports + are resolved to absolute against ``dotted`` so a future ``from ..store import x`` cannot walk + straight through the guard.""" + package = dotted.rsplit(".", 1)[0] + candidates: set[str] = set() + node_count = 0 + for node in ast.walk(ast.parse(source)): + if isinstance(node, ast.Import): + node_count += 1 + for alias in node.names: + candidates.add(alias.name) + elif isinstance(node, ast.ImportFrom): + node_count += 1 + if node.level == 0: + if node.module: + candidates.add(node.module) + for alias in node.names: + candidates.add(f"{node.module}.{alias.name}") + else: + # `from . import x` -> the package itself; `from ..store import x` -> one level up. + base_pkg = package.rsplit(".", node.level - 1)[0] + base = f"{base_pkg}.{node.module}" if node.module else base_pkg + candidates.add(base) + for alias in node.names: + candidates.add(f"{base}.{alias.name}") + return candidates, node_count + + +def test_sandbox_boundary_modules_import_nothing_forbidden() -> None: + """LIVE GUARD: the shipped codec + worker source imports nothing on the forbidden list. + + Green on the real source today; this pins it that way. A future ``import socket`` / ``from + messagefoundry.store import ...`` slipping into either file would make ``mode=subprocess`` DOA on + first deployment, and this reddens instead of the suite staying green.""" + # Anti-vacuity: the forbidden set is real and populated, so an empty scan cannot pass by default. + assert DEFAULT_FORBIDDEN_MODULES, "DEFAULT_FORBIDDEN_MODULES is empty" + assert "socket" in DEFAULT_FORBIDDEN_MODULES, DEFAULT_FORBIDDEN_MODULES + + violations: list[str] = [] + for dotted, path in _TARGETS: + assert path.exists(), f"sandbox boundary module missing: {path}" + candidates, node_count = _walk(path.read_text(encoding="utf-8"), dotted) + # Anti-vacuity: a walker that silently saw no imports must FAIL, not pass green. + assert node_count > 0, f"{path.name} yielded no import nodes" + for candidate in sorted(candidates): + if _is_forbidden(candidate): + violations.append(f"{path.name} imports {candidate}") + assert not violations, violations + + +def test_walker_flags_every_forbidden_import_form() -> None: + """POSITIVE CONTROL: prove the walker can SEE each import form, run in isolation per form. + + Each case is its own source string so the assertion depends only on that form's handling — e.g. + the alias-append case would still be masked by a sibling ``from messagefoundry.store import ...`` + if they shared one source, so they must not. Committed and always-on: it pins the alias-append, + relative-resolution and nested-import refinements in CI permanently.""" + ctx = "messagefoundry.pipeline._sandbox_worker" + cases: dict[str, tuple[str, str]] = { + "top-level import": ("import socket\n", "socket"), + "from-import of a forbidden submodule": ( + "from messagefoundry.store import base\n", + "messagefoundry.store", + ), + # The load-bearing case: `store` is forbidden but its parent `messagefoundry` is not, so only + # the alias-append candidate `messagefoundry.store` trips the guard here. + "from-parent import of a forbidden child (alias-append)": ( + "from messagefoundry import store\n", + "messagefoundry.store", + ), + "function-level import": ("def f():\n import ssl\n", "ssl"), + "relative import resolved to absolute": ( + "from ..auth import service\n", + "messagefoundry.auth", + ), + } + for label, (source, expected) in cases.items(): + candidates, node_count = _walk(source, ctx) + assert node_count > 0, f"{label}: no import nodes seen" + flagged = {c for c in candidates if _is_forbidden(c)} + assert expected in flagged, f"{label}: expected {expected!r} flagged, got {sorted(flagged)}" + + +def test_walker_does_not_flag_allowed_imports() -> None: + """NEGATIVE CONTROL: benign imports the sandbox child legitimately needs raise ZERO flags. + + Guards against an over-broad matcher (e.g. one that flagged the non-forbidden ``messagefoundry`` + parent), which would dead-letter a correct build — the flip side of a vacuous walker.""" + source = ( + "import json\n" + "from messagefoundry.config.wiring import Send\n" + "from messagefoundry.parsing.message import Message\n" + "from messagefoundry.pipeline import _sandbox_codec\n" + ) + candidates, node_count = _walk(source, "messagefoundry.pipeline._sandbox_worker") + assert node_count > 0 + flagged = {c for c in candidates if _is_forbidden(c)} + assert not flagged, sorted(flagged) From 0e2a0501edf53e78207562bd3fd7c829efa1ca8e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 04:44:45 -0500 Subject: [PATCH 08/14] backlog: flip #346 to SHIPPED -- the static ast import-boundary guard landed (BACKLOG #346) The #346 banner alone: OPEN -> SHIPPED, pointing at tests/test_sandbox_import_boundary.py (the static ast guard added in the preceding commit). The completeness wording is softened from "every forbidden import form is seen" to "each static import form the walker handles" -- a static walker cannot see dynamic importlib/__import__ forms, and CLAUDE.md section 11 prefers a bounded claim to an enumeration. Only the #346 banner line changed; the ranked table, the four census distribution lines, and every other item's banner are untouched. The census was NOT recomputed. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 38305897..3aed9c5a 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -3308,7 +3308,7 @@ What is NOT settled is the mechanism. Two independent passes reached different a ## 346. The sandbox import boundary is enforced only at runtime, under an off-by-default flag -> 🚧 **Status OPEN (filed 2026-08-02).** Value **4/10** · Difficulty **3/10** · _fill-in_. A type the sandbox child must **construct or receive** cannot live under a prefix on `DEFAULT_FORBIDDEN_MODULES` ([pipeline/sandbox.py](../messagefoundry/pipeline/sandbox.py)) — the child's import guard raises and the dispatch fails. That rule is real, it has already been violated once in shipped code, and **nothing enforces it**. `CapturedResponse` lived in `messagefoundry.store`; the child could not import it, which made `mode=subprocess` + ADR 0013 loopback re-ingress **DOA** until #339 relocated it to [config/response.py](../messagefoundry/config/response.py). The only guard runs **in the child, at dispatch time, and only when `[sandbox].mode=subprocess`** — which is not the default, so a re-violation is invisible to a green suite. +> ✅ **SHIPPED 2026-08-06 — a static `ast` import-boundary guard now pins it.** Value **4/10** · Difficulty **3/10** · _fill-in_. [tests/test_sandbox_import_boundary.py](../tests/test_sandbox_import_boundary.py) walks the `ast` import nodes of `_sandbox_codec.py` and `_sandbox_worker.py` and asserts none resolves under a `DEFAULT_FORBIDDEN_MODULES` prefix (imported from the runtime constant, never copied), with a committed positive control that each static import form the walker handles is seen and a negative control that benign `messagefoundry.*` imports are not flagged. Both files are clean today; the guard would red on first deployment if a future edit reintroduced a forbidden import, instead of failing silently only under `[sandbox].mode=subprocess`. **Cluster:** Correctness / test coverage. **Priority:** P2. **Verdict:** build (small). **Severity:** medium (blast radius: a feature is DOA for everyone who opted in), medium (likelihood: the codec's constructor set is precisely the surface that grows as the payload model does). From 90dc30a86f93b2e814613b371de163e999086693 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 11:08:44 -0500 Subject: [PATCH 09/14] fix(tls): route four insecure-TLS escape cells through the ADR-0092 clamp (BACKLOG #329) LDAPS (auth/ldap.py), SFTP host-key (transports/remotefile.py), the webhook sink (pipeline/alert_sinks.py) and the AI-broker (transports/ai_broker.py) read the raw MEFOR_ALLOW_INSECURE_TLS escape directly; on an enforcing-PHI instance each would otherwise honour the env var on first deployment. Each now routes through the ADR-0092 weakened_tls_escape helper: SFTP is built in-gate so it uses _here(); the other three are built outside the hop scope, so the instance posture is threaded explicitly through AuthService / notifier_from_settings / ai_broker_from_settings (additive, default None = byte-identical for existing callers). The fifth cell the item names (direct.py) was already clamped in #323, so this converts the remaining four. Docs (CONNECTIONS/DEPLOYMENT/ PHI) corrected from 'not clamped'/'unclamped' to clamped. --- docs/CONNECTIONS.md | 8 +- docs/DEPLOYMENT.md | 14 +- docs/PHI.md | 2 +- messagefoundry/api/app.py | 35 ++- messagefoundry/auth/ldap.py | 31 ++- messagefoundry/auth/service.py | 10 +- messagefoundry/config/settings.py | 17 +- messagefoundry/pipeline/alert_sinks.py | 26 ++- messagefoundry/transports/ai_broker.py | 23 +- messagefoundry/transports/remotefile.py | 11 +- tests/test_hop_refusal_329.py | 281 ++++++++++++++++++++++++ 11 files changed, 408 insertions(+), 50 deletions(-) create mode 100644 tests/test_hop_refusal_329.py diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index cd4219f8..bd6bbf27 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -840,9 +840,11 @@ poll/write shape against a remote server, selected by an internal `protocol` set (`pip install 'messagefoundry[sftp]'`, lazily imported so an install that never uses SFTP skips it). **Host-key verification is ON by default** (the system host keys plus an optional extra `known_hosts`, paramiko `RejectPolicy`); an unknown key is **refused** unless `MEFOR_ALLOW_INSECURE_TLS` is set (and - loudly logged when it is). **This one cell reads the raw escape and is *not* clamped** — unlike the - `tls_verify` / `encrypt` cells elsewhere in this document, the variable still works here on a - production-PHI enforcing instance, so it is the SFTP setting to audit for rather than assume inert. + loudly logged when it is). **Since #329 this cell routes the escape through the clamped + `weakened_tls_escape_permitted_here()`** — like the `tls_verify` / `encrypt` cells elsewhere in this + document, so on a production-PHI enforcing instance the escape is inert and an unknown host key stays + refused (`RejectPolicy`) even with the variable set; it takes effect only on a non-enforcing / non-PHI + instance. - **`Ftp(...)`** — stdlib `ftplib`, **no extra**: `tls=False` is plain FTP, `tls=True` is **FTPS** (explicit TLS + `PROT P`, encrypting the control *and* data channels). FTPS **verifies the server certificate and hostname by default** (a verifying `SSLContext`, not ftplib's no-verify fallback). diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 13782a03..f28578b5 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -410,12 +410,12 @@ With it set, these otherwise-refused settings become permitted (each logs a loud - DATABASE destination / store: `Encrypt=false` or `TrustServerCertificate=true` (SQL Server), `[store].trust_server_certificate=true` / `[store].encrypt=false`. *(Clamped.)* - Plain-FTP credentials. *(Clamped.)* -- RemoteFile SFTP: accepting an unknown host key. *(Not clamped — the raw escape still applies.)* +- RemoteFile SFTP: accepting an unknown host key. *(Clamped since #329.)* - Cleartext SMTP submission on a **Direct** (S/MIME) destination. *(Not clamped; AUTH credentials over cleartext stay refused outright either way.)* - The non-connection cells that have nowhere to carry a per-hop declaration: the `[logging]` syslog/SIEM - forwarder and the API PHI-read serve hop *(both clamped)*, plus LDAPS, the webhook alert sink and the - AI-broker endpoint *(raw escape)*. + forwarder and the API PHI-read serve hop, plus LDAPS, the webhook alert sink and the + AI-broker endpoint. *(All clamped — LDAPS / the webhook sink / the AI broker since #329.)* **Two limits worth stating plainly.** *(a)* Since [ADR 0153](adr/0153-collapse-the-posture-gradient-no-data-label-may-allow-a-cleartext-hop.md) this variable has been **unhooked from the cleartext-hop authority** — that decision no longer reads it, @@ -426,9 +426,11 @@ cleartext HTTP family are now governed only by a per-connection `cleartext_accep factory parameter and no `connections.toml` key, so it is unreachable from config today. Refusal messages that suggest it are ahead of the code.) *(b)* Where it does still apply it is mostly **clamped** (ADR 0092 decision 2 / ADR 0148): it cannot relax a hop while `[security].enforcement = -enforce`, and for the MLLP/FTPS/plain-FTP and store-TLS cells the clamp additionally requires the instance -to be PHI — which is also the default. Either way, on the shipped posture those cells are inert; the -bullets marked *not clamped* are the exceptions that still honour the raw variable. +enforce`, and for the weakened-TLS / cleartext-escape cells that route through +`weakened_tls_escape_permitted` — at least the store-TLS, MLLP/FTPS and plain-FTP cells and, since #329, +LDAPS, the SFTP host key, the webhook sink and the AI broker — the clamp additionally requires the +instance to be PHI, which is also the default. Either way, on the shipped posture those cells are inert; +the bullets marked *not clamped* are the exceptions that still honour the raw variable. **Never set `MEFOR_ALLOW_INSECURE_TLS` in production.** Its presence is the single **environment-variable** switch that turns the remaining fail-closed verification checks into best-effort. diff --git a/docs/PHI.md b/docs/PHI.md index 245602ea..420383a6 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -970,7 +970,7 @@ with materially different PHI profiles, so they get their own rows; stream 4 is | **7. `connection_event` table — DEFAULT ON** (`[diagnostics].connection_events = true`) | transport/lifecycle events per connection: `established`, `closed` (reason `eof` or `idle_timeout` — no path produces any other), `idle_timeout`, `at_capacity`, `peer_not_allowlisted`, `frame_oversize`, `framing_error`, `peer_reset`, the inbound-HTTP intake-auth refusals `intake_auth_failed` / `auth_subject_denied` / `auth_rate_limited` (ADR 0154 D6 — peer address and mode only; **never** the credential, a prefix of it, or its length. Each of these also writes a tamper-evident audit-log row — the copy that survives an operator turning this diagnostics stream off), plus the runner's `connection_lost` / `connection_restored`. That is the whole vocabulary, asserted in CI against the literal emit call sites in `transports/` and the pipeline runner **and** cross-checked against the console's own filter tuple. The MLLP, raw-TCP and HTTP listeners emit these; the **DICOM inbound C-STORE SCP** and the **`ISA`/`IEA`-framed X12 inbound** emit none — the runner injects the sink onto **every** source (`wiring_runner.py`, over the base-class `on_connection_event` field), so both connectors *have* the wiring and simply never call it — so this stream covers those three listeners plus the runner's outbound-lane transitions — not literally every connection. An X12 feed's connects, allow-list refusals and at-capacity refusals are therefore **absent** from this stream | rows: `ts`, `connection`, `transport`, `direction`, `kind`, `peer_host`, `message_id` (correlation hint), `reason` | the store database, **all three backends** | Corepoint-style transport diagnostics — "did the sender connect, and why did it drop" | `GET /events` and `GET /connections/{name}/events` under **`monitoring:read`** (**not** a PHI permission) with per-channel RBAC — an out-of-scope `connection=` is 403'd *and* audited — server-clamped to ≤1000 rows | `[retention].connection_event_retention_hours` (its own **hours** window); 0 inherits `[retention].messages_days`; both 0 = keep forever. Plain age `DELETE` (metadata-only) | **`reason` is free text that can carry sensitive fragments.** Defended twice — `safe_exc()` at the source, `safe_text(reason)[:200]` at the store — then cipher-encrypted (AAD `("connection_event","reason",connection,ts,kind)`). Every other column is config metadata; the table is documented **metadata-only** — never a frame, body or HL7 field value. Writes are a pure side observer: a bounded in-memory queue drained by a background task outside any handoff transaction, so a flood can never block a listener or pin a message disposition | | **8. `alert_instance` table — default on wherever an `[alerts]` notifier exists** | resolvable operator alerts: `connection_stopped`, `queue_buildup`, `lane_stuck`, `message_stall`, `saturation`, `connection_error`, `content_match`, `storage_threshold`, `cert_expiry`, `secret_rotation`, `bootstrap_admin_expiring` (the UNCLAIMED first-run bootstrap admin nearing its auto-disable deadline — ASVS 6.4.5; its payload carries only the ISO deadline plus whole hours remaining, never the password or any secret), `integrity_drift`, `update_available`, `backup_failed`, `rcsi_off_degraded`, `leadership_acquired`, `dr_activated`, `gcm_invocations` (the per-key AES-GCM invocation bound crossing its 2^31 soft warn — ASVS 11.3.4; its payload carries a one-way `key_id` fingerprint plus counters, never key bytes) The three reachable **inverse** signals — `connection_restored`, `leadership_lost`, `dr_released` — are never rows here: `_record_state` routes an inverse through `_AUTO_RESOLVE` to `resolve_alert_instances_for`, never to `upsert_alert_instance`. (A fourth mapped key, `connection_started`, is emitted by no code path today.) | rows: `event_type`, `connection`, `severity`, `status`, `first_seen`, `last_seen`, `count`, `reason`, `acked_by`, `acked_at`, `resolved_at`, `suspended_until`, `escalation_tier` | the store database, **all three backends** | the operator alert list — acknowledge / resolve / suspend. Durable state is recorded **before** any suppression or throttle return, so a muted alert still leaves a record | `GET /alerts/active` under **`monitoring:diagnose`** (**not** a PHI permission) with the same per-channel scope; ack/resolve/suspend/**resume** are POSTs on the same tier, and the separate read-only `GET /alerts/rules` view sits on its own gate | shares the connection-event window; **only RESOLVED instances are DELETEd**, by `resolved_at` — an open or acknowledged condition is never aged out from under an operator | **`reason` is free text** taken from the event's `detail`/`reason`/`label`: `safe_text(reason)[:200]` then cipher-encrypted (AAD `("alert_instance","reason",event_type,connection)` — the de-dup grain, so one AAD covers both the INSERT and the re-fire UPDATE). `content_match` is **PHI-free by contract**: the sink method takes no value parameter, only the connection, an operator label and an optional rule id | | **9. `response` rows with `kind='ack_sent'` — DEFAULT ON** (`[diagnostics].response_sent = true`) | the ACK/NAK the engine returned to an inbound sender, under a sentinel destination `\x1fack:` | rows: `ack_code` (`AA`/`AE`/`AR`/`CA`/`CE`/`CR`), `ack_phase` (`decode`/`parse`/`strict`/`ingest`), `outcome`, `body`, `detail` | the store database | "what did we actually reply, and why" — the operator's answer to a sender disputing an ACK | `GET /messages/{id}/responses` under `messages:read` + `require_phi_read`; the `body` only for a caller who also holds `messages:view_raw`; every read writes a `response.read` audit row | `body`, `detail` and `resp_headers` are set to `NULL` in place by `purge_message_bodies` on the message-body window, on all three backends | **PHI fail-safe:** the ACK **body** is stored **only when the store cipher is active** — on a keyless store it is `NULL` rather than plaintext — and every NAK passes no body at all, so the offending field value is never persisted. The disposition metadata (`ack_code`/`ack_phase`/`outcome`) is non-PHI and always captured; `detail` is `safe_text`-scrubbed, 200-char bounded and encrypted | -| **10. `[alerts]` webhook transport** (off by default — `webhook_url` unset) | one HTTPS POST per alert, carrying every non-underscore event key as JSON | JSON | the operator's webhook endpoint (Slack/Teams/PagerDuty/custom) | operator notification | **`https` only** — a plaintext `http://` webhook URL is refused at construction unless the raw `MEFOR_ALLOW_INSECURE_TLS` escape is set (and then a warning is logged); note this path reads the **unclamped** escape, unlike the connectors. Redirects are refused; an optional `webhook_allowed_hosts` egress allowlist gates the host | the endpoint's | **carries the alert's `detail`/`reason` free text** (`safe_exc()`-scrubbed at the emit sites, but **not** re-run through `safe_text` on this path). Internal `_`-prefixed keys (per-rule recipients, rule id, cooldown) are stripped before send, so recipient addresses never cross the wire | +| **10. `[alerts]` webhook transport** (off by default — `webhook_url` unset) | one HTTPS POST per alert, carrying every non-underscore event key as JSON | JSON | the operator's webhook endpoint (Slack/Teams/PagerDuty/custom) | operator notification | **`https` only** — a plaintext `http://` webhook URL is refused at construction unless the `MEFOR_ALLOW_INSECURE_TLS` escape is set (and then a warning is logged); since #329 this path routes that escape through the clamped `weakened_tls_escape_permitted(posture)` (the instance posture threaded from the API lifespan), so on an enforcing-PHI instance the escape is inert and a cleartext webhook POST stays refused — the same clamp as the connectors, no longer the raw escape. Redirects are refused; an optional `webhook_allowed_hosts` egress allowlist gates the host | the endpoint's | **carries the alert's `detail`/`reason` free text** (`safe_exc()`-scrubbed at the emit sites, but **not** re-run through `safe_text` on this path). Internal `_`-prefixed keys (per-rule recipients, rule id, cooldown) are stripped before send, so recipient addresses never cross the wire | | **11. `[alerts]` SMTP transport — operator alert list** (off unless `email_smtp_host` + `email_from` + ≥1 `email_to`) | one email per alert; default subject `[MessageFoundry] — `, default body every non-underscore event key as `k: v` | plain text (always kept — never HTML-only); optional HTML alternative | the operators' mailboxes | operator notification | `smtp_allowed_hosts` egress allowlist; the SMTP password comes from `MEFOR_ALERTS_EMAIL_PASSWORD` or a `[secrets]` provider, never the config file; per-send timeout `email_timeout` | the mail system's | carries the same `detail`/`reason` free text as the webhook. #138 operator templates are constrained to a **closed non-PHI variable allowlist** validated fail-closed at config load. **Transport posture:** `send_plain_email` builds an explicit **verifying** context (chain + hostname + strict RFC 5280, TLS 1.2 floor) via `tls_policy.build_smtp_tls_context()` and passes it to `starttls()`, anchored to the OS roots, `[alerts].email_tls_ca_file`, or `[tls].internal_ca_file` — the same factory the EMAIL and DIRECT *message destinations* use, so all three SMTP cells now share one policy ([#323](BACKLOG.md), closed 2026-08-02). Before that this call passed **no** context and Python's stdlib default applied (`ssl._create_stdlib_context` **is** `ssl._create_unverified_context` — `CERT_NONE`, `check_hostname = False`), leaving the hop encrypted but unauthenticated. There is still **no hop gradient or attestation on this path** — unlike the connectors, this cell is constructed outside the `active_hop_posture` scope, so its deviations (`email_use_tls = false`, or `email_tls_verify = false`) are gated by a `[security].allow_unverified_alert_smtp_tls` **acknowledgment switch at the serve gate** rather than by the clamped escape: on an enforcing PHI instance `serve` refuses to start without it, and permits + `AUDIT`-logs the start with it. Both deviations are named by `security_loosenings()` and reported by `messagefoundry check`'s `alert-smtp-tls` advisory | | **12. Per-user security-event SMTP notifier** — **posture-mandatory on a PHI instance** | `account_locked`, `login_after_failures`, `password_changed`, `password_reset`, `email_changed`, `roles_changed`, `account_disabled`, `mfa_enabled`, `mfa_disabled`, `admin_action_new_ip` | plain-text email | the **affected user's own** mailbox | ASVS 6.3.5 / 6.3.7 out-of-band notification of security-relevant account changes | shares stream 11's SMTP transport and therefore its verifying context and its `[alerts].email_tls_*` knobs — note this is a **separate call site** (`pipeline/security_notify.py`), plumbed in its own right rather than inheriting by accident. On a PHI instance with auth enabled `serve` **refuses to start (exit 2) under `[security].enforcement = enforce`** when no effective channel exists; the explicit, **audited** opt-out is `[alerts].security_notifications_required = false` | the mail system's | the body carries the account username, a fixed description, optionally the failed-attempt count or the new email on file, and the source IP — **no message data, no secrets**. Dispatch is a bounded background queue; a failed send is logged, never raised (the event is still in `audit_log`) | | **13. `LoggingAlertSink` fallback** (when no `[alerts]` transport is configured) | every alert **this state-less sink implements**, at `WARNING` — `leadership_lost` / `dr_released` at `INFO`, and `connection_restored` is a **deliberate no-op** (a recovery needs no page and there is no instance to auto-resolve), so a lane recovery produces no record on this stream at all. `content_match` exists only on `NotifierAlertSink` and has no fallback-path record | — | folds into stream 1 | so alerts are never silent | inherits stream 1's | inherits stream 1's | includes the `detail`/`reason` free text, and therefore inherits stream 1's filters, ACL, forwarder and retention | diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index cc1a1833..e0fb7e24 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -1427,8 +1427,13 @@ async def ai_chat( ) # Build the broker from the SERVER's settings (never the request body). The SSRF endpoint-allowlist # + cleartext-credential checks run in the constructor; a misconfiguration is an operator error. + # #329: thread the derived instance posture (the same _phi_read_posture derived at create_app time + # from ai_settings, which == app.state.ai == `ai` on the managed path) so the broker's cleartext- + # http credential refusal is clamped on an enforcing-PHI instance — the escape can no longer put + # the api_key on the wire. Without this the route would build the broker with an unclamped escape + # (green and inert), the exact failure mode #329 exists to close. try: - broker = ai_broker_from_settings(ai) + broker = ai_broker_from_settings(ai, posture=_phi_read_posture) except AiBrokerError as exc: _log.warning("engine AI broker misconfigured: %s", exc) raise HTTPException(503, "engine-brokered AI assistance is not available") from exc @@ -5331,19 +5336,27 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: # WITHOUT changing capacity. A no-op returning None in production / every other test, so the # engine is byte-identical when the gate is unset. Stashed for /stats + shut down in finally. app.state.connscale_executor = maybe_install_executor_shim(asyncio.get_running_loop()) + # #329: the derived instance hop posture, computed ONCE here for the OUT-OF-GATE cells this + # lifespan builds — the alerts webhook sink (notifier_from_settings) and the LDAPS bind + # (AuthService → LdapAuthenticator). Neither is built inside an active_hop_posture scope, so + # current_hop_posture() is None there and their weakened-TLS escape would ship UNCLAMPED (green + # and inert) without an explicit posture; threading this makes the ADR-0092 clamp apply on first + # deployment. None when the instance declares no [ai] (SQLite/test) → the unclamped escape, + # byte-identical. Reuses the same hop_posture_from_ai derivation the store hop and the runner use. + _hop_posture = ( + hop_posture_from_ai( + ai_settings, enforcement=(security_settings or SecuritySettings()).enforcement + ) + if ai_settings is not None + else None + ) # #200 (ADR 0092 decision 2): thread the derived instance posture so the engine<->store weakened- # TLS refusal (connection_string / _build_ssl) clamps MEFOR_ALLOW_INSECURE_TLS — the escape can # never relax a production-PHI store hop. None when no [ai] (SQLite/test) → unclamped, unchanged. store = await open_store( resolved, message_events=message_events, - posture=( - hop_posture_from_ai( - ai_settings, enforcement=(security_settings or SecuritySettings()).enforcement - ) - if ai_settings - else None - ), + posture=_hop_posture, ) # Offline uploaded-logs store (BACKLOG #125/#126, ADR 0134), on the LIVE store's cipher instance. # DISABLED (None) unless [store].uploads_dir is set, so no PHI-at-rest surface exists unless an @@ -5385,6 +5398,8 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: # #323 layer 3: the instance [tls] internal-CA policy reaches the alerts SMTP hop too, so # an estate on a private CA needs no per-alert CA path. trust_anchor_policy=tls_settings.policy() if tls_settings else None, + # #329: clamp the webhook sink's cleartext-http escape to the derived instance posture. + posture=_hop_posture, ) if alerts_settings is not None else None @@ -5633,6 +5648,10 @@ async def _audit_upload_prune(meta: UploadedFileMeta) -> None: # central run_anchor_preflight above) rather than refusing at enforce-only. Central # preflight already ran before any listener bound; this keeps the seam consistent. enforcing=trust_anchors_enforcing, + # #329: thread the derived instance posture to the LDAPS bind so its ad_tls_verify=false + # escape is clamped on an enforcing-PHI instance (LdapAuthenticator is built out of the + # connector-construction gate, so the clamp is inert unless the posture arrives here). + hop_posture=_hop_posture, ) bootstrap = await auth.initialize() app.state.auth = auth diff --git a/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index 82e1b62e..66ae634d 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -27,8 +27,9 @@ from messagefoundry.config.settings import ( INSECURE_TLS_ESCAPE_ENV, AuthSettings, - insecure_tls_allowed, + weakened_tls_escape_permitted, ) +from messagefoundry.config.tls_policy import HopPosture logger = logging.getLogger(__name__) @@ -87,7 +88,11 @@ class LdapAuthenticator: """Binds against Active Directory over LDAPS and resolves a user's (nested) group membership.""" def __init__( - self, settings: AuthSettings, *, secret_provider: SecretProvider | None = None + self, + settings: AuthSettings, + *, + secret_provider: SecretProvider | None = None, + posture: HopPosture | None = None, ) -> None: if not settings.ad_server or not settings.ad_user_search_base: raise LdapError("AD is enabled but ad_server / ad_user_search_base are not configured") @@ -105,16 +110,26 @@ def __init__( if not settings.ad_bind_dn or not self._bind_password: raise LdapError("AD is enabled but the service-account bind is not configured") self._s = settings - # A disabled-cert-verification posture (ad_tls_verify=false over LDAPS) makes the service- - # account and user binds MITM-able, so it now REFUSES at startup unless the operator sets the - # explicit MEFOR_ALLOW_INSECURE_TLS dev escape — it can no longer be silently turned on in - # production (ASVS 12.3.2). With the escape set, we still warn loudly once at startup. + # #329: the instance hop posture (threaded by AuthService from create_app's derived posture). + # LDAPS is built OUT of the connector-construction gate (AuthService, not build_check_registry), + # so current_hop_posture() would be None here; the posture must be passed explicitly or the + # clamp below would be inert. None (a direct/test/embedding construction) falls back to the + # unclamped escape — byte-identical to the pre-#329 bare read. + self._posture = posture + # A disabled-cert-verification posture (ad_tls_verify=false over LDAPS) would make the service- + # account and user binds MITM-able on first deployment, so it REFUSES at startup unless the + # operator sets the explicit MEFOR_ALLOW_INSECURE_TLS dev escape (ASVS 12.3.2). #329 routes that + # escape through the ADR-0092 clamp (weakened_tls_escape_permitted): under an enforcing-PHI + # posture the escape is INERT, so it can never silence this refusal on such an instance — the + # blunt env var no longer buys verify-off there. With the escape permitted (non-enforcing/non-PHI + # or unstamped posture), we still warn loudly once at startup. if str(settings.ad_server).lower().startswith("ldaps") and not settings.ad_tls_verify: - if not insecure_tls_allowed(): + if not weakened_tls_escape_permitted(self._posture): raise LdapError( "ad_tls_verify=false disables LDAPS certificate verification (MITM risk). Use a " f"trusted CA via ad_tls_ca_cert_file, or set {INSECURE_TLS_ESCAPE_ENV}=1 to " - "explicitly allow it for a trusted-network dev/test bind." + "explicitly allow it for a trusted-network dev/test bind (refused on an enforcing " + "PHI instance even with that override set, #329)." ) logger.warning( "AD LDAPS certificate verification is DISABLED (ad_tls_verify=false, permitted by " diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 4fd0bf00..f18a100e 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -61,6 +61,7 @@ from messagefoundry.config.models import SignatureAlgorithm from messagefoundry.config.secretprovider import SecretProvider, resolve_connector_secret from messagefoundry.config.settings import AuthSettings +from messagefoundry.config.tls_policy import HopPosture from messagefoundry.store.base import AdminStore from messagefoundry.store.store import SessionRecord, UserRecord, WebAuthnCredential @@ -239,6 +240,7 @@ def __init__( security_notifier: SecurityNotifier | None = None, secret_provider: SecretProvider | None = None, enforcing: bool = True, + hop_posture: HopPosture | None = None, ) -> None: self._store = store self._settings = settings @@ -271,8 +273,12 @@ def __init__( self._ldap: LdapAuthenticator | None = ldap elif settings.ad_enabled: # Thread the connector SecretProvider (ADR 0019 §5) so an ad_bind_password_secret reference - # resolves the bind password from the external backend (fail-closed) at construction. - self._ldap = LdapAuthenticator(settings, secret_provider=secret_provider) + # resolves the bind password from the external backend (fail-closed) at construction. #329: + # thread the instance hop posture too — LDAPS is built out of the connector-construction gate, + # so its ad_tls_verify=false escape clamp is inert unless the posture arrives explicitly here. + self._ldap = LdapAuthenticator( + settings, secret_provider=secret_provider, posture=hop_posture + ) else: self._ldap = None # Instance-scoped (one event loop per AuthService) so it never crosses loops in tests. diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index acd0e132..672b14ee 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -213,13 +213,18 @@ def weakened_tls_escape_permitted(posture: HopPosture | None = None) -> bool: """Whether ``MEFOR_ALLOW_INSECURE_TLS`` may permit a weakened / verify-off TLS hop under ``posture``, CLAMPED so an enforcing PHI hop is NEVER relaxed (#200, ADR 0092 decision 2). - The is_phi-blind **strict verify-off** cells — the engine<->store TLS gate + The is_phi-blind **weakened-TLS / cleartext-escape** cells route their global-escape check through + here so the blunt escape can no longer silence an **enforcing PHI** refusal (matching the + ``--allow-insecure-bind`` API-bind clamp). That is **at least** the engine<->store TLS gate (:func:`~messagefoundry.store.sqlserver.connection_string` / ``store.postgres._build_ssl``), the MLLP - and FTPS ``tls_verify=false`` contexts, and the credentialed plain-``ftp`` guard — route their global- - escape check through here so the blunt escape can no longer silence an **enforcing PHI** refusal - (matching the ``--allow-insecure-bind`` API-bind clamp). Pass the construction-time - :func:`~messagefoundry.config.tls_policy.current_hop_posture` (transport cells) or the store's threaded - posture. Semantics: the escape must be set at all, AND the hop must not be enforcing PHI. ``None`` + and FTPS ``tls_verify=false`` contexts and the credentialed plain-``ftp`` guard, **and — since #329 —** + the LDAPS ``ad_tls_verify=false`` bind (:mod:`messagefoundry.auth.ldap`), the SFTP unknown-host-key + acceptance (:mod:`messagefoundry.transports.remotefile`), and the webhook-alert-sink and AI-broker + cleartext-``http`` hops. Pass the construction-time + :func:`~messagefoundry.config.tls_policy.current_hop_posture` (in-gate transport cells, via + :func:`weakened_tls_escape_permitted_here`) or an explicitly-threaded posture (the store hop and the + out-of-gate #329 cells, whose construction never stamps the contextvar). Semantics: the escape must + be set at all, AND the hop must not be enforcing PHI. ``None`` (a backup utility / embedding / test outside the construction gate) falls back to the **unclamped** escape — byte-identical to pre-#200 — since the enforced serve/reload gate already vetted the real production posture, so this fallback never loosens the clamp.""" diff --git a/messagefoundry/pipeline/alert_sinks.py b/messagefoundry/pipeline/alert_sinks.py index 550a787d..79c310c7 100644 --- a/messagefoundry/pipeline/alert_sinks.py +++ b/messagefoundry/pipeline/alert_sinks.py @@ -43,9 +43,9 @@ AlertSeverity, AlertsSettings, EscalationTier, - insecure_tls_allowed, + weakened_tls_escape_permitted, ) -from messagefoundry.config.tls_policy import TrustAnchorPolicy, build_smtp_tls_context +from messagefoundry.config.tls_policy import HopPosture, TrustAnchorPolicy, build_smtp_tls_context __all__ = [ "AlertTransport", @@ -277,18 +277,26 @@ class WebhookTransport: """POST the event as JSON to a configured URL (fronts Slack/Teams/PagerDuty/custom webhooks).""" def __init__( - self, url: str, *, timeout: float = 10.0, allowed_hosts: tuple[str, ...] = () + self, + url: str, + *, + timeout: float = 10.0, + allowed_hosts: tuple[str, ...] = (), + posture: HopPosture | None = None, ) -> None: # Refuse a plaintext http:// webhook target unless the explicit dev escape is set: the alert # POST otherwise crosses the network in cleartext (ASVS 12.2.1 — no insecure fallback). https # is the only scheme accepted by default; http(s) remain the only schemes at all (see _post). - # Same refuse-unless-MEFOR_ALLOW_INSECURE_TLS pattern as LDAPS / SQL Server / MLLP — stricter - # than the credentialed-only http refusal on REST/SOAP, since a webhook has no PHI but should - # still never fall back to cleartext. + # Same refuse-unless-MEFOR_ALLOW_INSECURE_TLS pattern as LDAPS / SQL Server / MLLP. #329: read + # the escape through the ADR-0092 clamp (weakened_tls_escape_permitted) so on an enforcing-PHI + # instance the blunt env var can never re-permit a cleartext alert POST. The webhook sink is + # built out of the connector-construction gate (notifier_from_settings, in the app lifespan), so + # the posture is threaded explicitly; None (a direct/test construction) falls back to the + # unclamped escape — byte-identical to the pre-#329 bare read. scheme = urllib.parse.urlsplit(url).scheme.lower() if scheme not in ("http", "https"): raise ValueError(f"webhook url must be http or https, got scheme {scheme!r}") - if scheme == "http" and not insecure_tls_allowed(): + if scheme == "http" and not weakened_tls_escape_permitted(posture): raise ValueError( f"webhook url {url!r} uses plaintext http; refused unless " f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only) — use https" @@ -1168,6 +1176,7 @@ def notifier_from_settings( *, secret_provider: SecretProvider | None = None, trust_anchor_policy: TrustAnchorPolicy | None = None, + posture: HopPosture | None = None, ) -> NotifierAlertSink | None: """Build a :class:`NotifierAlertSink` from ``[alerts]`` settings, or ``None`` when no transport is configured (the caller then leaves the engine on its default logging sink). @@ -1197,6 +1206,9 @@ def notifier_from_settings( alerts.webhook_url, timeout=alerts.webhook_timeout, allowed_hosts=tuple(alerts.webhook_allowed_hosts), + # #329: thread the derived instance posture so the cleartext-http refusal is clamped on + # an enforcing-PHI instance (the escape can no longer re-permit a plaintext alert POST). + posture=posture, ) ) if alerts.email_smtp_host and alerts.email_from and alerts.email_to: diff --git a/messagefoundry/transports/ai_broker.py b/messagefoundry/transports/ai_broker.py index 450aae66..834db029 100644 --- a/messagefoundry/transports/ai_broker.py +++ b/messagefoundry/transports/ai_broker.py @@ -41,7 +41,8 @@ import urllib.request from typing import TYPE_CHECKING -from messagefoundry.config.settings import INSECURE_TLS_ESCAPE_ENV, insecure_tls_allowed +from messagefoundry.config.settings import INSECURE_TLS_ESCAPE_ENV, weakened_tls_escape_permitted +from messagefoundry.config.tls_policy import HopPosture # Reuse rest.py's hardened, TLS-verifying, no-redirect opener + URL redaction (no new HTTP plumbing) — # exactly as smart.py / fhir.py / soap.py do. No import cycle: rest.py never imports this module. @@ -114,6 +115,7 @@ def __init__( model: str = "claude-opus-4-8", max_output_tokens: int = _DEFAULT_MAX_OUTPUT_TOKENS, timeout_seconds: float = _DEFAULT_TIMEOUT, + posture: HopPosture | None = None, ) -> None: if not endpoint: raise AiBrokerError( @@ -136,8 +138,13 @@ def __init__( "list it explicitly to permit engine-brokered AI egress (SSRF fail-closed)" ) # The api_key is a credential — refuse to send it over cleartext http (mirrors smart.py's token - # endpoint), unless the dev escape is set for a trusted-network dev/test box. - if scheme == "http" and not insecure_tls_allowed(): + # endpoint), unless the dev escape is set for a trusted-network dev/test box. #329: read the + # escape through the ADR-0092 clamp (weakened_tls_escape_permitted) so on an enforcing-PHI + # instance the blunt env var can never re-permit the key on the wire. The broker is built out of + # the connector-construction gate (the create_app ai_chat route), so the posture is threaded + # explicitly; None (a direct/test construction) falls back to the unclamped escape — byte- + # identical to the pre-#329 bare read. + if scheme == "http" and not weakened_tls_escape_permitted(posture): raise AiBrokerError( "[ai].endpoint over cleartext http would expose the api_key; refused unless " f"{INSECURE_TLS_ESCAPE_ENV} is set (dev/trusted-network only) — use https" @@ -239,14 +246,20 @@ def _extract_text(self, body: str) -> str: return text -def ai_broker_from_settings(ai: AiSettings) -> AiBroker: +def ai_broker_from_settings(ai: AiSettings, *, posture: HopPosture | None = None) -> AiBroker: """Build the :class:`AiBroker` from the loaded ``[ai]`` settings, or raise :class:`AiBrokerError` when the engine broker is not fully configured. Settings arrive already ``env()``-resolved (the API - lifespan stashes the resolved :class:`AiSettings` on ``app.state.ai``).""" + lifespan stashes the resolved :class:`AiSettings` on ``app.state.ai``). + + ``posture`` (#329) is the derived instance hop posture, threaded from the create_app ai_chat route so + the broker's cleartext-http credential refusal is clamped on an enforcing-PHI instance (the escape + can no longer put the ``api_key`` on the wire there). ``None`` = the unclamped escape, byte-identical + to before.""" return AiBroker( endpoint=ai.endpoint or "", api_key=ai.api_key or "", allowed_endpoints=list(ai.allowed_endpoints), provider=ai.provider, model=ai.model, + posture=posture, ) diff --git a/messagefoundry/transports/remotefile.py b/messagefoundry/transports/remotefile.py index ae737648..bf68157a 100644 --- a/messagefoundry/transports/remotefile.py +++ b/messagefoundry/transports/remotefile.py @@ -53,7 +53,6 @@ from messagefoundry.config.models import ConnectorType, ContentType, Destination, Source from messagefoundry.config.settings import ( INSECURE_TLS_ESCAPE_ENV, - insecure_tls_allowed, weakened_tls_escape_permitted_here, ) from messagefoundry.config.tls_policy import ( @@ -370,9 +369,13 @@ def __init__(self, settings: dict[str, Any]) -> None: self._known_hosts = settings.get("known_hosts") self._timeout = float(settings.get("connect_timeout", 30.0)) # Fail fast at construction (build_check time): an unknown-host-key posture without the escape - # must never silently weaken to auto-accept. The accept-unknown policy is gated here so the - # connector refuses to build rather than trust-on-first-use a man-in-the-middle. - self._accept_unknown = insecure_tls_allowed() + # must never silently weaken to auto-accept. #329: read the escape through the ADR-0092 clamp + # (weakened_tls_escape_permitted_here consults the active construction posture, exactly like the + # FTPS tls_verify=false sibling at :176 that this class is built alongside), so under an + # enforcing-PHI posture the escape is INERT — the accept-unknown policy then stays RejectPolicy + # and an unknown host key is refused at connect (:392-394), as today. Off the construction gate + # (posture unstamped) the escape is unclamped, byte-identical to the pre-#329 bare read. + self._accept_unknown = weakened_tls_escape_permitted_here() if self._accept_unknown: logger.warning( "REMOTEFILE sftp %s accepts UNKNOWN host keys (AutoAddPolicy) because %s is set " diff --git a/tests/test_hop_refusal_329.py b/tests/test_hop_refusal_329.py new file mode 100644 index 00000000..4b653764 --- /dev/null +++ b/tests/test_hop_refusal_329.py @@ -0,0 +1,281 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""#329 — route the four remaining ``MEFOR_ALLOW_INSECURE_TLS`` cells through the ADR-0092 clamp. + +Before this lane four cells still consulted the raw ``insecure_tls_allowed()`` predicate, so on first +deployment each WOULD permit weakened TLS regardless of the posture clamp: the SFTP unknown-host-key +acceptance (in-gate), and the LDAPS ``ad_tls_verify=false`` bind, the webhook alert sink and the +AI-broker cleartext-http credential hop (all out-of-gate). This suite proves each cell now REFUSES an +enforcing-PHI hop even with the escape set, that a non-enforcing / unstamped posture still crosses +(byte-identical to before), that the secure path is untouched, and — the load-bearing guard against the +item's headline "green and inert" risk — that the out-of-gate factories/constructors actually THREAD a +non-``None`` posture through the real ``create_app`` route and ``AuthService``/notifier seams. +""" + +from __future__ import annotations + +import logging +from collections.abc import AsyncIterator +from pathlib import Path + +import httpx +import pytest + +from messagefoundry.api import create_app +from messagefoundry.auth.ldap import LdapAuthenticator, LdapError +from messagefoundry.auth.service import AuthService +from messagefoundry.config.ai_policy import AiMode +from messagefoundry.config.settings import ( + INSECURE_TLS_ESCAPE_ENV, + AiSettings, + AlertsSettings, + AuthSettings, +) +from messagefoundry.config.tls_policy import HopPosture, active_hop_posture +from messagefoundry.pipeline import Engine +from messagefoundry.pipeline.alert_sinks import WebhookTransport, notifier_from_settings +from messagefoundry.store.store import MessageStore +from messagefoundry.transports import ai_broker as ai_broker_mod +from messagefoundry.transports.ai_broker import AiBroker, AiBrokerError, ai_broker_from_settings +from messagefoundry.transports.remotefile import _SftpClient + +# Mirror tests/test_hop_refusal_serve_clamp.py so the two suites decide against the same postures. +PROD_PHI = HopPosture(is_phi=True, enforcing=True) +STAGING_PHI = HopPosture(is_phi=True, enforcing=False) +SYNTHETIC = HopPosture(is_phi=False, enforcing=False) # dev / synthetic instance (no PHI) + + +@pytest.fixture +async def store(tmp_path: Path) -> AsyncIterator[MessageStore]: + s = await MessageStore.open(tmp_path / "hop329.db") + yield s + await s.close() + + +# --- Cell (2): SFTP host key (IN-GATE, reads current_hop_posture via _here) --------------------- + + +def test_sftp_host_key_clamped_prod_phi_even_with_escape(monkeypatch: pytest.MonkeyPatch) -> None: + # In-gate cell: with the escape set, the unknown-host-key auto-accept is CLAMPED under an enforcing + # PHI posture (accept_unknown stays False → RejectPolicy), but a non-enforcing / synthetic posture + # still auto-accepts. Construction reads only the posture, so no paramiko fake is needed here. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + s = {"host": "h", "port": 22, "remote_dir": "/in"} + with active_hop_posture(PROD_PHI): + assert _SftpClient(s)._accept_unknown is False # clamped: enforcing PHI refuses the escape + with active_hop_posture(STAGING_PHI): + assert ( + _SftpClient(s)._accept_unknown is True + ) # non-enforcing PHI still crosses with the escape + with active_hop_posture(SYNTHETIC): + assert _SftpClient(s)._accept_unknown is True # synthetic instance still crosses + + +def test_sftp_host_key_fail_closed_without_escape(monkeypatch: pytest.MonkeyPatch) -> None: + # Escape-off negative control: no posture crosses (unchanged fail-closed). Asserting True here would + # red immediately — proof the assertion binds something. + monkeypatch.delenv(INSECURE_TLS_ESCAPE_ENV, raising=False) + s = {"host": "h", "port": 22, "remote_dir": "/in"} + with active_hop_posture(PROD_PHI): + assert _SftpClient(s)._accept_unknown is False + # Unstamped (no active_hop_posture) with the escape off is also fail-closed. + assert _SftpClient(s)._accept_unknown is False + + +# --- Cell (1): LDAPS ad_tls_verify=false (OUT-OF-GATE, explicit posture) ------------------------ + + +def _ldaps_verifyoff() -> AuthSettings: + return AuthSettings( + ad_enabled=True, + ad_server="ldaps://dc.example.com", + ad_user_search_base="DC=example,DC=com", + ad_bind_dn="CN=svc,DC=example,DC=com", + ad_bind_password="synthetic-bind-secret", + ad_tls_verify=False, + ) + + +def test_ldaps_verifyoff_clamped_prod_phi_even_with_escape( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + s = _ldaps_verifyoff() + # Enforcing PHI: refused even with the escape (the clamp bites). + with pytest.raises(LdapError, match="ad_tls_verify=false"): + LdapAuthenticator(s, posture=PROD_PHI) + # Non-enforcing PHI: crosses with the escape (warned loudly). + with caplog.at_level(logging.WARNING): + LdapAuthenticator(s, posture=STAGING_PHI) + assert any("DISABLED" in r.getMessage() for r in caplog.records) + # Unstamped posture (a direct/test construction) falls back to the unclamped escape — byte-identical. + LdapAuthenticator(s, posture=None) + + +async def test_ldaps_authservice_threads_posture( + store: MessageStore, monkeypatch: pytest.MonkeyPatch +) -> None: + # Seam: AuthService must thread hop_posture into the LdapAuthenticator it builds. With an enforcing + # PHI posture + the escape set, constructing the service raises LdapError — proof the posture reaches + # the cell. If the seam dropped it (posture=None), the escape would be unclamped and the build would + # succeed. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + with pytest.raises(LdapError, match="ad_tls_verify=false"): + AuthService(store, _ldaps_verifyoff(), hop_posture=PROD_PHI) + # A non-enforcing posture builds the service without raising (crosses with the escape). + AuthService(store, _ldaps_verifyoff(), hop_posture=STAGING_PHI) + + +def test_ldaps_verifying_secure_path_unaffected(monkeypatch: pytest.MonkeyPatch) -> None: + # Secure-path negative control: ad_tls_verify=True (verifying LDAPS) + enforcing PHI + escape UNSET + # builds with no error — the guard only fires on the verify-OFF path, so #329 leaves the secure path + # byte-identical. + monkeypatch.delenv(INSECURE_TLS_ESCAPE_ENV, raising=False) + s = AuthSettings( + ad_enabled=True, + ad_server="ldaps://dc.example.com", + ad_user_search_base="DC=example,DC=com", + ad_bind_dn="CN=svc,DC=example,DC=com", + ad_bind_password="synthetic-bind-secret", + ad_tls_verify=True, + ) + LdapAuthenticator(s, posture=PROD_PHI) # no raise: verifying LDAPS is unaffected by the change + + +# --- Cell (3): webhook alert sink cleartext http (OUT-OF-GATE, explicit posture) ---------------- + + +def test_webhook_cleartext_clamped_prod_phi_even_with_escape( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + with pytest.raises(ValueError, match="plaintext http"): + WebhookTransport("http://hooks.example/x", posture=PROD_PHI) + # Non-enforcing PHI and unstamped both cross with the escape (byte-identical to before). + WebhookTransport("http://hooks.example/x", posture=STAGING_PHI) + WebhookTransport("http://hooks.example/x", posture=None) + + +def test_webhook_notifier_threads_posture(monkeypatch: pytest.MonkeyPatch) -> None: + # Seam: notifier_from_settings must thread posture into the WebhookTransport it builds. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + with pytest.raises(ValueError, match="plaintext http"): + notifier_from_settings( + AlertsSettings(webhook_url="http://hooks.example/x"), posture=PROD_PHI + ) + # Non-enforcing posture builds the notifier (crosses with the escape). + notifier_from_settings( + AlertsSettings(webhook_url="http://hooks.example/x"), posture=STAGING_PHI + ) + + +def test_webhook_https_secure_path_unaffected(monkeypatch: pytest.MonkeyPatch) -> None: + # Negative control: an https webhook is never refused, whatever the posture. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + WebhookTransport("https://hooks.example/x", posture=PROD_PHI) + + +# --- Cell (4): AI-broker cleartext http credential hop (OUT-OF-GATE, explicit posture) ---------- + +_HTTP_ENDPOINT = "http://ai.internal/v1/messages" +_HTTPS_ENDPOINT = "https://ai.internal/v1/messages" + + +def _managed_ai(**over: object) -> AiSettings: + kw: dict[str, object] = { + "mode": AiMode.MANAGED_ENDPOINT, + "environment": "prod", + "endpoint": _HTTP_ENDPOINT, + "api_key": "sk-synthetic-key", + "allowed_endpoints": ["ai.internal"], + } + kw.update(over) + return AiSettings(**kw) # type: ignore[arg-type] + + +def test_ai_broker_cleartext_clamped_prod_phi_even_with_escape( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + + def _build(posture: HopPosture | None) -> AiBroker: + return AiBroker( + endpoint=_HTTP_ENDPOINT, + api_key="k", + allowed_endpoints=["ai.internal"], + posture=posture, + ) + + with pytest.raises(AiBrokerError, match="cleartext http"): + _build(PROD_PHI) + _build(STAGING_PHI) # crosses with the escape on non-enforcing PHI + _build(None) # unstamped falls back to the unclamped escape — byte-identical + + +def test_ai_broker_factory_threads_posture(monkeypatch: pytest.MonkeyPatch) -> None: + # Seam: ai_broker_from_settings must thread posture into the AiBroker it builds. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + with pytest.raises(AiBrokerError, match="cleartext http"): + ai_broker_from_settings(_managed_ai(), posture=PROD_PHI) + ai_broker_from_settings(_managed_ai(), posture=STAGING_PHI) # crosses on non-enforcing PHI + + +def test_ai_broker_https_secure_path_unaffected(monkeypatch: pytest.MonkeyPatch) -> None: + # Negative control: an https endpoint is never refused on the cleartext arm, whatever the posture. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + AiBroker( + endpoint=_HTTPS_ENDPOINT, api_key="k", allowed_endpoints=["ai.internal"], posture=PROD_PHI + ) + + +# --- Anti-"green-and-inert" WIRING test: the real create_app ai_chat route threads the posture ----- + + +@pytest.fixture +async def engine(tmp_path: Path) -> AsyncIterator[Engine]: + eng = await Engine.create(tmp_path / "ai_wiring.db", poll_interval=0.02) + yield eng + await eng.stop() + + +def _client(app: object) -> httpx.AsyncClient: + transport = httpx.ASGITransport(app=app) # type: ignore[arg-type] + return httpx.AsyncClient(transport=transport, base_url="http://t") + + +@pytest.fixture +def stub_chat(monkeypatch: pytest.MonkeyPatch) -> None: + # If (and only if) the broker BUILDS, a canned reply keeps the route off the network — so a route + # that fails to thread the posture returns 200, not a network 502. That makes the 503-vs-not signal + # crisp for the falsification below. + def fake_chat(self: AiBroker, prompt: str) -> str: + return "def handle(msg): return Send('OB', msg)" + + monkeypatch.setattr(ai_broker_mod.AiBroker, "chat", fake_chat) + + +async def test_ai_chat_route_refuses_cleartext_under_enforcing_phi( + engine: Engine, monkeypatch: pytest.MonkeyPatch, stub_chat: None +) -> None: + # The load-bearing wiring guard: through the REAL route, a prod (enforcing-PHI) instance with the + # escape set must REFUSE to build the broker over a cleartext-http endpoint → HTTP 503. This proves + # create_app actually threads a non-None posture into ai_broker_from_settings (the "green and inert" + # failure the item warns of). FALSIFY: drop `posture=_phi_read_posture` in the ai_chat route → the + # broker builds over http and the route returns 200 (chat stubbed) instead of 503. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + app = create_app(engine, ai_settings=_managed_ai(endpoint=_HTTP_ENDPOINT), allow_no_auth=True) + async with _client(app) as c: + r = await c.post("/ai/chat", json={"prompt": "hi"}) + assert r.status_code == 503 # broker refused by the clamp through the real route + + +async def test_ai_chat_route_allows_https_under_enforcing_phi( + engine: Engine, monkeypatch: pytest.MonkeyPatch, stub_chat: None +) -> None: + # Companion control: the SAME enforcing-PHI + escape setup over an HTTPS endpoint is NOT 503 — so the + # 503 above is the cleartext clamp, not a blanket refusal of the route. + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + app = create_app(engine, ai_settings=_managed_ai(endpoint=_HTTPS_ENDPOINT), allow_no_auth=True) + async with _client(app) as c: + r = await c.post("/ai/chat", json={"prompt": "hi"}) + assert r.status_code == 200 From bb8ee564bb982fd017355bbe4977602545bed5d4 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 11:10:45 -0500 Subject: [PATCH 10/14] docs(backlog): flip #329 to shipped -- four insecure-TLS cells clamped (BACKLOG #329) Banner line only (leaves the 2026-08-03 amendment note); census not recomputed. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 911cf3da..af89ca2a 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -2859,7 +2859,7 @@ This is **wider than the disclosure describes.** [`CONFIGURATION.md:718`](CONFIG ## 329. Five `MEFOR_ALLOW_INSECURE_TLS` cells bypass the ADR 0092 clamp -> 🔢 **Filed 2026-08-01 — not started.** Value **6/10** · Difficulty **4/10** · _quick win_. The LDAPS bind (`ssl.CERT_NONE` on the authentication substrate for every AD identity), the SFTP host key, the webhook sink and the `[ai].api_key` still cross an enforcing production-PHI posture on one env var, and converting them is what collapses five per-site facts into one repo-wide invariant the ASVS scorecard's regex mechanism can actually express — bounded because setting the variable needs Administrator, who can already do worse; the cheap in-gate half shipped with #323, so what remains is threading an explicit posture into `AuthService`/`create_app`'s three out-of-gate constructors, where `_here()` would otherwise ship green and inert. +> ✅ **SHIPPED 2026-08-06 — the four out-of-gate insecure-TLS cells now route through the ADR-0092 clamp.** Value **6/10** · Difficulty **4/10** · _quick win_. LDAPS (`auth/ldap.py`), the SFTP host key (`transports/remotefile.py`), the webhook sink (`pipeline/alert_sinks.py`) and the AI-broker (`transports/ai_broker.py`) now gate the `MEFOR_ALLOW_INSECURE_TLS` escape through `weakened_tls_escape_permitted[_here]` — the instance posture threaded into `AuthService` / `create_app`'s out-of-gate constructors — so on an enforcing production-PHI instance the escape is inert and an unverified/cleartext hop stays refused. The fifth cell the heading names (Direct SMTP) was already clamped in #323, so this converted the remaining four. > ⚠️ **AMENDED 2026-08-03 — the census is FOUR, not five: #323 landed and took the Direct SMTP cell.** The heading, the evidence table (*"Confirmed at HEAD"*) and Proposed §1 all still name `transports/direct.py:170` as an unclamped cell, but that file now holds **no call to the raw predicate at all** — it imports only `weakened_tls_escape_permitted_here` (`messagefoundry/transports/direct.py:63`) and gates both arms on it (`:197` cleartext SMTP, `:215` `tls_verify=false`), with the #323 rationale — including its own warning that this absence is scoped to that file and never repo-wide — at `:182-196`; `:170` is now unrelated cert-loading. The Scope note called this in future tense and the 2026-08-03 banner already enumerates only four cells while still calling them *"five per-site facts"*, so read the table as **at least four** sites still reading the unclamped `insecure_tls_allowed()`: the SFTP host key (`messagefoundry/transports/remotefile.py:375`, feeding `AutoAddPolicy`/`RejectPolicy` at `:392-394`), LDAPS (`messagefoundry/auth/ldap.py:113`), the webhook alert sink (`messagefoundry/pipeline/alert_sinks.py:291` — the item cites `:290`) and the AI broker (`messagefoundry/transports/ai_broker.py:140`). > From 4e5df592c642d28c68613aaa719792554da57726 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 13:59:32 -0500 Subject: [PATCH 11/14] fix(dev): anchor setup-leak-gate.ps1 to its own checkout, not the cwd (BACKLOG #1063) `$repo` came from `git rev-parse --show-toplevel`, which resolves against the CURRENT directory rather than the path the script was handed. Invoked by absolute `-File` path from another worktree -- the ordinary shape on a clone carrying dozens of them -- it armed the CALLER's checkout and printed CONFIGURED about that one, while the checkout the operator named kept no token source and went on failing closed. An absolute `-File` invocation is naming the checkout to act on; it must not then consult a different one. Now `Split-Path -Parent (Split-Path -Parent $PSScriptRoot)`, the form postgres.ps1:37 and sqlserver.ps1:56 in the same directory already use, plus an assert that the derived root actually carries scripts/security/ -- a wrong root should say so where it is derived rather than surface later as a confusing scanner failure. Tested by the DIVERGENCE, which is the only shape that can fail: two temp checkouts that both carry scripts/security/, the script invoked by absolute path while the shell stands in the other one. A test run from inside the target passes with the bug still in, because cwd and script root are then the same directory. Reverted to the old line, the same test reports "the named checkout was not armed" -- the negative control was run, not assumed. The fixture copies only the three files the script reaches for, never the whole of scripts/security/: a maintainer running this suite has the real token list sitting in that directory, and a copytree would sweep it into a temp dir. Also corrects this item's own prose. It called alloc.ps1:51 "byte-equivalent"; it is not -- alloc.ps1 carries --path-format=absolute and this script does not. The defect is identical, the bytes are not, and "byte-equivalent" is the kind of claim a later reader greps for and then trusts. --- docs/BACKLOG.md | 4 +- scripts/dev/setup-leak-gate.ps1 | 18 ++++- tests/test_script_root_anchoring.py | 113 ++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 tests/test_script_root_anchoring.py diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 8f5a5835..1cf1727f 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5613,7 +5613,7 @@ The second step's arithmetic is measured: `GetFullPath('.git', )` r ## 1063. `setup-leak-gate.ps1` picks the checkout from the current directory, so it can arm a worktree the operator did not name -> 🔢 **Filed 2026-08-06 — not started.** Value **3/10** · Difficulty **1/10** · _quick win_. `scripts/dev/setup-leak-gate.ps1:37` is `$repo = (& git rev-parse --show-toplevel 2>$null)` — no `-C`, no `-Repo` parameter, no `$PSScriptRoot` anchor. Invoked by absolute `-File` path from a different worktree, which is the ordinary shape on a clone with 40-plus of them, it installs the leak-gate token list into **the current directory's** checkout and prints `CONFIGURED` about that one, while the worktree the operator named keeps no token source. Its own directory siblings already do it correctly. +> ✅ **SHIPPED 2026-08-06 — the root is anchored to the script's own location, and the divergence is under test.** Value **3/10** · Difficulty **1/10** · _quick win_. `$repo` now comes from `Split-Path -Parent (Split-Path -Parent $PSScriptRoot)`, the form `postgres.ps1` and `sqlserver.ps1` in the same directory already use, plus an assert that the derived root actually carries `scripts/security/` — a wrong root should say so at the point of derivation rather than surface later as a confusing scanner failure. Tested by the **divergence**, not the happy path: two temp checkouts that both carry `scripts/security/`, the script invoked by absolute `-File` path while the shell stands in the other one, asserting the token list lands in the checkout holding the script and **not** in the caller's. The pre-fix behaviour was reproduced directly rather than inferred — reverted, the same test reports *"the named checkout was not armed"*. Only the three files the script reaches for are copied into the fixture, never the whole of `scripts/security/`, because a maintainer running the suite has the real token list sitting in that directory. Original filing follows. `scripts/dev/setup-leak-gate.ps1:37` was `$repo = (& git rev-parse --show-toplevel 2>$null)` — no `-C`, no `-Repo` parameter, no `$PSScriptRoot` anchor. Invoked by absolute `-File` path from a different worktree, which is the ordinary shape on a clone with 40-plus of them, it installs the leak-gate token list into **the current directory's** checkout and prints `CONFIGURED` about that one, while the worktree the operator named keeps no token source. Its own directory siblings already do it correctly. **Cluster:** Developer tooling / configuration anchoring. **Priority:** P4. **Verdict:** build (trivial). **Severity:** **low, and the low severity is load-bearing** — every failure direction here is loud or fail-closed, which is why this is filed at 3 rather than alongside its siblings. Nothing is silently ungated and no wrong authorisation is granted. @@ -5627,7 +5627,7 @@ $repo = Split-Path -Parent (Split-Path -Parent $PSScriptRoot) Anchoring on `$PSScriptRoot` binds the script to the checkout it lives in, which is what an absolute `-File` invocation is asking for. `-From` names the token **source**, not the checkout, so it does not already cover this. -**Same construct as #1060.** `alloc.ps1:51` is byte-equivalent (`git rev-parse --show-toplevel` with no anchor) and produces the same class of wrong answer — there, a misattributed ledger allocation; here, a token list installed into the wrong tree. Fixing them together is reasonable; filing them together was not, because their severities differ by two priority bands and folding this into #1060 would have inflated it. +**Same construct as #1060.** `alloc.ps1:51` is the same construct — `git rev-parse --show-toplevel` with no anchor — and produces the same class of wrong answer — there, a misattributed ledger allocation; here, a token list installed into the wrong tree. Fixing them together is reasonable; filing them together was not, because their severities differ by two priority bands and folding this into #1060 would have inflated it. (The filing said *byte-equivalent*. It is not: `alloc.ps1` carries `--path-format=absolute` and this script does not. The defect is identical; the bytes are not, and "byte-equivalent" is the kind of claim a later reader greps for and then trusts.) **How it was found, and why that matters more than the defect.** A repo-wide sweep for the cwd-as-identity shape assigned five surfaces and left `scripts/dev` and `scripts/service` in **no** surface at all — seven `.ps1` files in the seam. This was found only because the synthesising agent went outside its brief and swept the unassigned region. A measuring apparatus with a blind spot, hunting mechanisms with blind spots. Worth remembering when the next sweep is designed: **state the unassigned regions, or the result reads as completeness.** diff --git a/scripts/dev/setup-leak-gate.ps1 b/scripts/dev/setup-leak-gate.ps1 index 831eb833..9747f843 100644 --- a/scripts/dev/setup-leak-gate.ps1 +++ b/scripts/dev/setup-leak-gate.ps1 @@ -34,9 +34,23 @@ param( ) $ErrorActionPreference = 'Stop' -$repo = (& git rev-parse --show-toplevel 2>$null) -if (-not $repo) { throw 'Not inside a git checkout.' } + +# Repo root = parent of scripts\ = parent of this script's dir (scripts\dev). Same form postgres.ps1 and +# sqlserver.ps1 in this directory already use. +# +# NOT `git rev-parse --show-toplevel`: that resolves against the CURRENT DIRECTORY, not against the path +# this script was handed. Invoked by absolute -File path from another worktree -- the ordinary shape on a +# clone carrying dozens of them -- it armed the CALLER's checkout and printed CONFIGURED about that one, +# while the checkout the operator named kept no token source and went on failing closed (BACKLOG #1063). +# An absolute -File invocation is naming the checkout to act on; it must not then consult a different one. +# -From names the token SOURCE, not the checkout, so it never covered this. +$repo = Split-Path -Parent (Split-Path -Parent $PSScriptRoot) $secDir = Join-Path $repo 'scripts/security' +# ASSERT what the derivation only IMPLIES. Discovering a wrong root later, as a confusing scanner +# failure, is how this class stays invisible; new.ps1 takes the same posture after deriving its path. +if (-not (Test-Path -LiteralPath $secDir)) { + throw "Derived repo root has no scripts/security -- this script must live in /scripts/dev/: $repo" +} $local = Join-Path $secDir 'scan-tokens.local.txt' $example = Join-Path $secDir 'scan-tokens.local.txt.example' $scanner = Join-Path $secDir 'scan_forbidden.py' diff --git a/tests/test_script_root_anchoring.py b/tests/test_script_root_anchoring.py new file mode 100644 index 00000000..0b12e835 --- /dev/null +++ b/tests/test_script_root_anchoring.py @@ -0,0 +1,113 @@ +"""Scripts invoked by absolute ``-File`` path must act on the checkout they LIVE in, not the cwd. + +The regression home for the "cwd is not the caller" family (BACKLOG #1057, #1059, #1060, #1062, #1063). +Three mechanisms in this repo silently assumed that where a command runs is where the caller is, and all +three failed in the benign-looking direction -- a wrong owner recorded, a gate armed in the wrong tree, an +occupancy of zero for a tree in active use. None of them raised. + +**Every test here asserts the DIVERGENCE, never the happy path.** A script run from inside its own +checkout passes with the bug still in: cwd and script root are the same directory, so the two candidate +answers are indistinguishable. The case that can tell them apart is an absolute ``-File`` invocation whose +cwd is a DIFFERENT checkout that also carries the file the script writes -- which is the ordinary shape on +a clone carrying dozens of worktrees, and is measured at 29% of writes on this repo. Per BACKLOG #1000 a +control needs the case that can distinguish; a test run from inside the target proves nothing. + +The static spelling guards below are deliberately paired with a behavioural test each. On their own they +would be exactly the green-because-it-cannot-see control this project keeps filing. +""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[1] +_SETUP_LEAK_GATE = _ROOT / "scripts" / "dev" / "setup-leak-gate.ps1" +_SECURITY = _ROOT / "scripts" / "security" + +pytestmark = pytest.mark.skipif( + shutil.which("pwsh") is None, reason="pwsh (PowerShell 7) not on PATH" +) + + +def _git(*args: str, cwd: Path) -> None: + subprocess.run(["git", *args], cwd=str(cwd), check=True, capture_output=True) + + +def _leak_gate_checkout(path: Path) -> Path: + """A minimal checkout carrying setup-leak-gate.ps1 and the files it reaches for. + + Only the three files the script needs are copied, NOT the whole of scripts/security. A maintainer + running this suite has the real ``scan-tokens.local.txt`` sitting in that directory, and a copytree + would sweep the private token list into a temp dir -- the one mistake the script itself refuses to + make (it deletes what it wrote rather than risk committing the list). + """ + (path / "scripts" / "dev").mkdir(parents=True) + (path / "scripts" / "security").mkdir(parents=True) + shutil.copy2(_SETUP_LEAK_GATE, path / "scripts" / "dev" / "setup-leak-gate.ps1") + for name in ("scan_forbidden.py", "scan-allowlist.txt", "scan-tokens.local.txt.example"): + shutil.copy2(_SECURITY / name, path / "scripts" / "security" / name) + # check-ignore reads the working-tree .gitignore, so no commit is needed -- but the repo must exist, + # or the script's own not-git-ignored guard deletes the file it just wrote and throws. + (path / ".gitignore").write_text("scripts/security/scan-tokens.local.txt\n", encoding="utf-8") + _git("init", "-b", "main", ".", cwd=path) + return path + + +def test_setup_leak_gate_arms_the_checkout_it_lives_in_not_the_cwd(tmp_path: Path) -> None: + """BACKLOG #1063. Reproduced by the divergence, which is the only shape that can fail. + + Pre-fix, ``$repo`` came from ``git rev-parse --show-toplevel`` -- the CURRENT directory -- so an + absolute ``-File`` invocation from another worktree installed the token list into the caller's tree + and printed CONFIGURED about it, while the tree the operator named kept no token source. Both + checkouts here carry ``scripts/security/``, so "which tree got the file" is the whole question and + neither answer is available by accident. + + The verify step's exit code is deliberately NOT asserted: it shells out to ``python``, which is not + guaranteed on PATH on every CI leg, and the copy has already happened by then. Placement is the fact + under test. + """ + named = _leak_gate_checkout(tmp_path / "Named") + caller = _leak_gate_checkout(tmp_path / "Caller") + + subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(named / "scripts" / "dev" / "setup-leak-gate.ps1"), + "-Synthetic", + ], + cwd=str(caller), # THE POINT: the shell is standing somewhere else entirely + capture_output=True, + text=True, + timeout=180, + ) + + installed = named / "scripts" / "security" / "scan-tokens.local.txt" + stray = caller / "scripts" / "security" / "scan-tokens.local.txt" + assert installed.is_file(), "the named checkout was not armed -- it read the cwd instead" + assert not stray.exists(), f"armed the CALLER's checkout instead: {stray}" + + +def test_setup_leak_gate_does_not_reintroduce_an_unanchored_toplevel(tmp_path: Path) -> None: + """A spelling guard for the regression, paired with the behavioural test above. + + Alone this would be worthless -- it cannot see the defect, only its most likely spelling. It earns + its place by naming the exact construct so a reader who reaches for ``--show-toplevel`` again gets + told why not, at the moment they do it. + """ + text = _SETUP_LEAK_GATE.read_text(encoding="utf-8") + assert "$PSScriptRoot" in text, "the repo root must be anchored to the script's own location" + for line in text.splitlines(): + stripped = line.strip() + if stripped.startswith("#") or "--show-toplevel" not in stripped: + continue + assert "-C " in stripped, ( + "an unanchored `git rev-parse --show-toplevel` resolves against the CURRENT directory, " + f"which is BACKLOG #1063: {stripped}" + ) From e4bf77f5e211fb909f8af3ab2effb16243185b19 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 14:05:52 -0500 Subject: [PATCH 12/14] fix(coord): anchor alloc.ps1 and claim.ps1 to their own checkout (BACKLOG #1060) Both took `$repo` from an unanchored `git rev-parse --show-toplevel`, which resolves against the CURRENT directory rather than the path the script was handed. Invoked by absolute `-File` path from worktree A while intending to commit from worktree B, the claim was recorded to A; the ledger gate then refused B's commit -- correctly, it fails closed -- but far from the cause and with a message about the wrong thing, and it cost a number. `git -C $PSScriptRoot`, not `Split-Path`. The recorded `worktree` value has THREE readers: ledger_check.py:227, this script's own `-List`, and prune-merged.ps1's orphan-claim release, whose comment at :787 names the producing command -- "records `worktree = $repo` from `git rev-parse --path-format=absolute`" -- and matches on the full normalised path because a false positive there hands a live session's key to someone else. All three fold separators, so `Split-Path` would not have broken anything; it would have silently falsified that comment, in a destructive tool, for no gain. THE FILING NAMED ONE OF FOUR CWD-DERIVED READS, and the other three are measured in the negative control below: * the `branch` recorded with the claim was the CALLER's branch; * the floor's boundary was parsed from the CALLER's scripts/hooks/ledger_check.py; * the floor's WORKING-TREE term read the CALLER's docs/BACKLOG.md. The third is not friction and the item's severity paragraph is corrected in the same commit. That term exists to catch a number written but committed NOWHERE. Reading the caller's tree makes a number drafted in the target worktree invisible, so the allocator hands it out as free and two items share it -- both owned by that worktree, so owns() passes and the ledger gate never fires. The silent collision the docstring says this script exists to prevent, reached through the script. Narrow, since anything committed on any ref is still caught by the all-refs term, but a correctness hole rather than friction. claim.ps1:54 carried the same construct and was never filed -- found by inspection here, fixed in the same commit. Its enforcing hook, claim_check.py, reads the repo from cwd and is RIGHT to: a commit hook's cwd IS the committing worktree. Hook right, tool wrong, and only the tool can be invoked from somewhere else. Both scripts now print a NOTE when the shell is standing somewhere else. Anchoring is correct but surprising, and the item's other half -- showing the recorded worktree -- was already built (`claimed by:` / `by :`); what was missing is saying so when it diverges, instead of leaving it to surface as a refused commit later. Silent on the ordinary same-tree invocation, so it stays worth reading. THE FIX TURNED TWO SANDBOXED TEST FILES INTO WRITERS ON THE LIVE REGISTRY, which is worse than the red suite it also caused, and is the reason those fixtures changed here. test_coord_claim_{refresh,liveness}.py ran the REAL scripts/coord/claim.ps1 with cwd set to a temp repo -- scoped to a throwaway registry purely by ambient cwd, and one of them said so ("it scopes itself to the cwd's repo"). Once the script stopped consulting cwd, the passing half of the run wrote real claims into this clone's shared registry: two strays, `k` and a date-shaped key, were created and removed by hand. Both fixtures now stage and COMMIT a copy of the script inside the temp repo, so the sandbox is structural rather than ambient, and a linked worktree of the fixture carries its own copy -- which is how the peer-holds-the-key tests still produce a claim recorded against the peer. test_ledger_check.py already did exactly this for alloc.ps1, which is why it was the one that did not break. Tested by the DIVERGENCE, with -ShowFloor so no numbers are burned: allocation is a one-way door and a test that allocated would leave permanent holes in the shared registry for every worktree of this clone. Two temp checkouts draft different numbers and carry different PUBLIC_BACKLOG_FLOOR stubs; the caller's number is deliberately HIGHER, because the floor is a maximum and an equal or lower one would pass with the bug in. Reverted to the old lines, the same test reports floor 7777, boundary 1900 and a watermark under Caller/.git -- three independent signals, all pointing at the wrong tree. --- docs/BACKLOG.md | 4 +- scripts/coord/alloc.ps1 | 55 ++++++++++--- scripts/coord/claim.ps1 | 31 +++++-- tests/test_coord_claim_liveness.py | 16 +++- tests/test_coord_claim_refresh.py | 24 +++++- tests/test_script_root_anchoring.py | 120 ++++++++++++++++++++++++++++ 6 files changed, 229 insertions(+), 21 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 1cf1727f..41556cf3 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5537,10 +5537,12 @@ The literal spellings are all caught, including the newline form. Only the indir ## 1060. `alloc.ps1` records the owning worktree from the current directory, so an absolute-path invocation misattributes it -> 🔢 **Filed 2026-08-05 — not started.** Value **5/10** · Difficulty **2/10** · _quick win_. `scripts/coord/alloc.ps1:51` takes the owner from `git rev-parse --show-toplevel`, which resolves against the **current directory** rather than the script's location. Invoke it by absolute `-File` path from a different worktree — which is how a session with several worktrees naturally calls it — and the allocation is recorded to the caller's worktree while the commit comes from another. The ledger gate then refuses that commit correctly, but far away from the cause and with a message about the wrong thing. +> ✅ **SHIPPED 2026-08-06 — every `git` call in both allocators is anchored to the script's own checkout, and the defect was larger than filed.** Value **5/10** · Difficulty **2/10** · _quick win_. `$repo` now comes from `git -C $PSScriptRoot rev-parse --path-format=absolute --show-toplevel`, and every subsequent call takes `-C $repo`. **`git -C`, not `Split-Path`:** the recorded `worktree` value is *compared* — by `ledger_check.py:227` and by `-List` — so its string form is part of a contract, and `--path-format=absolute` keeps writing the forward-slash form every claim already on disk carries. **The filing named one of four cwd-derived reads.** Also wrong, and measured on the pre-fix code: the `$branch` recorded with the claim was the *caller's* branch; the floor's boundary was parsed from the **caller's** `scripts/hooks/ledger_check.py`; and the floor's working-tree term — the one whose job is to catch a number written but committed **nowhere** — read the **caller's** `docs/BACKLOG.md`. That last one is not friction: a number drafted in the target worktree was invisible to the sweep and free to re-issue, which is the collision this script exists to prevent. **`scripts/coord/claim.ps1:54` carried the same construct and was never filed** — found by inspection while fixing this, fixed in the same commit; its enforcing hook `claim_check.py` reads the repo from cwd and is *right* to, because a commit hook's cwd **is** the committing worktree. Hook right, tool wrong, and only the tool can be invoked from elsewhere. The item's cheap second half — printing the recorded worktree at allocation time — was **already built** (`claimed by:`); what was missing is a note when the shell is standing somewhere else, which is now printed. Tested by the **divergence** with `-ShowFloor`, so no numbers were burned: two temp checkouts drafting different numbers, the allocator invoked by absolute path from the other one. The negative control was run, not assumed — reverted, the same test reports `floor : 7777`, `boundary : 1900` and a watermark under `Caller/.git/`, all three signals pointing at the wrong tree. Original filing follows. `scripts/coord/alloc.ps1:51` took the owner from `git rev-parse --show-toplevel`, which resolves against the **current directory** rather than the script's location. Invoke it by absolute `-File` path from a different worktree — which is how a session with several worktrees naturally calls it — and the allocation is recorded to the caller's worktree while the commit comes from another. The ledger gate then refuses that commit correctly, but far away from the cause and with a message about the wrong thing. **Cluster:** Session coordination / ledger integrity. **Priority:** P3. **Verdict:** build (small). **Severity:** no data loss and no security effect — the ledger gate **fails closed**, which is why this is a friction defect and not a correctness one. Nothing invalid lands; a valid commit is refused. +**SEVERITY CORRECTION, 2026-08-06, made while fixing it.** The paragraph above is right about the *recorded owner* and wrong about the item as a whole, because the filing looked at one line and the defect was in four. The ledger gate does fail closed on a misattributed claim — but the floor's **working-tree term** was cwd-derived too, and that term has no gate behind it. Its job is to see a number written to `docs/BACKLOG.md` and committed **nowhere**; reading the caller's tree makes a number drafted in the *target* worktree invisible, so the allocator hands it out as free and two items end up sharing it in the same tree. Both are then owned by that worktree, so `owns()` passes and the ledger gate never fires. That is the exact silent collision the script's own docstring says it exists to prevent, arrived at through the script. The window is narrow — anything committed on any ref is still caught by the all-refs term — but it is a correctness hole, not friction, and it was reproduced in the negative control rather than reasoned about. + **Reproduced 2026-08-05, twice, by accident.** A session ran `pwsh -NoProfile -File /scripts/coord/alloc.ps1 -Kind backlog` from worktree A while intending to commit from worktree B. `alloc/backlog/1058.json` recorded `"worktree": "<...>/trusting-wu-c2e6d5"`. The commit from `MessageFoundry-gate-deferrals` was then refused: *"BACKLOG item #1058 was not allocated to this worktree"* — true, unhelpful, and pointing at the allocator rather than at the invocation. Re-running with the shell actually inside the target worktree produced `1059.json` with the right owner and the commit went through. **#1058 is an abandoned hole**, which is the sanctioned outcome (`alloc.ps1`'s own docstring: *holes are free, collisions are not*). **The fix is small and there are two defensible shapes.** Either derive the repo from `$PSScriptRoot` so the allocator is anchored to the checkout it lives in — matching what `new.ps1` and `remove.ps1` already do — or keep the cwd behaviour and **say so at the point of use**, printing the recorded worktree in the `ALLOCATED` output so the mismatch is visible immediately rather than at commit time. The second is weaker but nearly free, and the two compose. Prefer anchoring: an allocator invoked by absolute path is being told which checkout to act on, and it should not then consult a different one. diff --git a/scripts/coord/alloc.ps1 b/scripts/coord/alloc.ps1 index 0daca930..fd5b3fbb 100644 --- a/scripts/coord/alloc.ps1 +++ b/scripts/coord/alloc.ps1 @@ -48,9 +48,27 @@ param( $ErrorActionPreference = "Stop" -$repo = (& git rev-parse --path-format=absolute --show-toplevel).Trim() -if (-not $repo) { throw "Not inside a git repository." } -$common = (& git rev-parse --path-format=absolute --git-common-dir).Trim() +# ANCHOR ON THE SCRIPT, NOT ON THE CURRENT DIRECTORY (BACKLOG #1060). Unanchored, every `git` call below +# resolved against wherever the shell happened to be standing, so an absolute `-File` invocation from +# worktree A while intending to commit from worktree B recorded the claim to A. The ledger gate then +# refused B's commit -- correctly, fails closed, nothing invalid lands -- but far from the cause and with +# a message about the wrong thing. It also cost a number: holes are free, collisions are not. +# +# `git -C $PSScriptRoot` rather than `Split-Path`, which is what the sibling scripts in scripts/dev use. +# The recorded `worktree` value is COMPARED, by ledger_check.py:227 and by -List below, so its string form +# is part of a contract: `--path-format=absolute` returns the forward-slash absolute form every claim +# already on disk carries, and `Split-Path` would start writing backslashes into the same field. Both +# comparisons normalise separators today, so this is not a live break -- it is a field whose format +# nothing pins, and changing it for no reason is how the next reader's grep stops matching. +# +# Every `git` call in this file is anchored the same way for the same reason. The floor's working-tree +# term (below) is the one where an unanchored read is more than misattribution: it looks for numbers +# written but committed NOWHERE, so reading the caller's tree makes a number drafted in the TARGET tree +# invisible and free to re-issue -- the collision this whole script exists to prevent. +$repo = (& git -C $PSScriptRoot rev-parse --path-format=absolute --show-toplevel 2>$null) +if (-not $repo) { throw "scripts/coord/ is not inside a git repository: $PSScriptRoot" } +$repo = $repo.Trim() +$common = (& git -C $repo rev-parse --path-format=absolute --git-common-dir).Trim() $allocRoot = Join-Path $common "mefor-coord/alloc" $alloc = Join-Path $allocRoot $Kind New-Item -ItemType Directory -Force -Path $alloc | Out-Null @@ -72,8 +90,8 @@ if (-not $Title -and -not $ShowFloor) { throw "-Title is required (it is recorde # `git branch --show-current` prints NOTHING on a detached HEAD, so `& git ...` yields $null (not "") # -- calling .Trim() on it here threw *before* the detached-HEAD fallback below could run. Null-check first. -$branch = & git branch --show-current -if ([string]::IsNullOrWhiteSpace($branch)) { $branch = "detached@" + (& git rev-parse --short HEAD) } +$branch = & git -C $repo branch --show-current +if ([string]::IsNullOrWhiteSpace($branch)) { $branch = "detached@" + (& git -C $repo rev-parse --short HEAD) } $branch = $branch.Trim() # FLOOR = max over (origin/main) U (every local + remote ref) U (existing allocations). @@ -88,9 +106,9 @@ function Get-Floor { $seen.Add(0) if ($Kind -eq "adr") { - $refs = @("origin/main") + @(& git for-each-ref --format='%(refname)' refs/heads refs/remotes) + $refs = @("origin/main") + @(& git -C $repo for-each-ref --format='%(refname)' refs/heads refs/remotes) foreach ($ref in ($refs | Select-Object -Unique)) { - $names = & git ls-tree --name-only $ref docs/adr/ 2>$null + $names = & git -C $repo ls-tree --name-only $ref docs/adr/ 2>$null foreach ($n in $names) { if ($n -match 'docs/adr/(\d{4})-') { $seen.Add([int]$Matches[1]) } } @@ -117,13 +135,13 @@ function Get-Floor { # files means adding each one here; an archive file not listed here is not policed. $backlogPaths = @("docs/BACKLOG.md", "docs/archive/backlog/BACKLOG-CLOSED.md") - $refs = @("origin/main", "HEAD") + @(& git for-each-ref --format='%(refname)' refs/heads refs/remotes) + $refs = @("origin/main", "HEAD") + @(& git -C $repo for-each-ref --format='%(refname)' refs/heads refs/remotes) $specs = foreach ($r in ($refs | Select-Object -Unique)) { foreach ($p in $backlogPaths) { "${r}:${p}" } } $oids = [System.Collections.Generic.HashSet[string]]::new() - foreach ($line in ($specs -join "`n" | & git cat-file --batch-check='%(objectname) %(objecttype)' 2>$null)) { + foreach ($line in ($specs -join "`n" | & git -C $repo cat-file --batch-check='%(objectname) %(objecttype)' 2>$null)) { $p = "$line".Split(' ') if ($p.Count -ge 2 -and $p[1] -eq 'blob') { [void]$oids.Add($p[0]) } } @@ -138,7 +156,7 @@ function Get-Floor { # that had been committed somewhere -- i.e. every case except the one this term is for. $rx = [regex]::new('^#{2,3} (\d+)\.', [System.Text.RegularExpressions.RegexOptions]::Multiline) if ($oids.Count -gt 0) { - foreach ($line in (($oids -join "`n") | & git cat-file --batch 2>$null)) { + foreach ($line in (($oids -join "`n") | & git -C $repo cat-file --batch 2>$null)) { $m = $rx.Match("$line") if ($m.Success) { $seen.Add([int]$m.Groups[1].Value) } } @@ -410,6 +428,23 @@ for ($i = $start; $i -lt $start + 500; $i++) { Write-Host " file : docs/BACKLOG.md" } Write-Host " claimed by: $repo [$branch]" + + # SAY IT AT THE POINT OF USE when the shell is standing somewhere else (BACKLOG #1060). Anchoring is + # now correct, but it is also SURPRISING: a caller who runs this by absolute path from worktree A gets + # a claim recorded to worktree B, and the only other place that fact surfaces is the ledger gate + # refusing a commit later, elsewhere, with a message about the wrong thing. One line here turns a + # deferred, misdirected refusal into an immediate, accurate note. Silent on the ordinary same-tree + # invocation, so it stays worth reading. + $cwdTop = (& git rev-parse --path-format=absolute --show-toplevel 2>$null) + if ($cwdTop) { + $a = ($cwdTop.Trim() -replace '\\', '/').TrimEnd('/') + $b = ($repo -replace '\\', '/').TrimEnd('/') + if ($a -ine $b) { + Write-Host " NOTE: your shell is in $a, but this allocator lives in $b, so the claim is recorded" -ForegroundColor Yellow + Write-Host " to $b. COMMIT FROM THERE -- the ledger gate keys entitlement on the worktree" -ForegroundColor Yellow + Write-Host " named above and will refuse the commit anywhere else." -ForegroundColor Yellow + } + } exit 0 } diff --git a/scripts/coord/claim.ps1 b/scripts/coord/claim.ps1 index b7b86057..308a2172 100644 --- a/scripts/coord/claim.ps1 +++ b/scripts/coord/claim.ps1 @@ -51,9 +51,17 @@ param( $ErrorActionPreference = "Stop" -$repo = (& git rev-parse --path-format=absolute --show-toplevel).Trim() -if (-not $repo) { throw "Not inside a git repository." } -$common = (& git rev-parse --path-format=absolute --git-common-dir).Trim() +# Anchored on the SCRIPT, not the current directory -- the reasoning is written out once, at the head of +# alloc.ps1 (BACKLOG #1060). It applies here identically and was found here by inspection rather than by +# a second reproduction: this file recorded `worktree = $repo` from the same unanchored call, so an +# absolute `-File` invocation from another worktree took the claim in the caller's name. A claim is +# advisory for free-text keys and ENFORCED for numbered ones by scripts/hooks/claim_check.py, which reads +# the repo from cwd correctly because it runs as a commit hook -- cwd IS the committing worktree there. +# Hook right, tool wrong, and only the tool can be invoked from somewhere else. +$repo = (& git -C $PSScriptRoot rev-parse --path-format=absolute --show-toplevel 2>$null) +if (-not $repo) { throw "scripts/coord/ is not inside a git repository: $PSScriptRoot" } +$repo = $repo.Trim() +$common = (& git -C $repo rev-parse --path-format=absolute --git-common-dir).Trim() $claims = Join-Path $common "mefor-coord/claims" New-Item -ItemType Directory -Force -Path $claims | Out-Null @@ -202,8 +210,8 @@ if ($List) { Show-List; exit 0 } $safe = ConvertTo-KeyFile $Take $file = Join-Path $claims "$safe.json" -$branch = & git branch --show-current -if ([string]::IsNullOrWhiteSpace($branch)) { $branch = "detached@" + (& git rev-parse --short HEAD) } +$branch = & git -C $repo branch --show-current +if ([string]::IsNullOrWhiteSpace($branch)) { $branch = "detached@" + (& git -C $repo rev-parse --short HEAD) } $branch = $branch.Trim() try { @@ -345,4 +353,17 @@ Write-Host "CLAIMED '$Take'" -ForegroundColor Green Write-Host " by : $repo [$branch]" Write-Host " note : $(if ($Note) { $Note } else { '(no note)' })" Write-Host " release when done: pwsh -NoProfile -File scripts\coord\claim.ps1 -Release $Take" + +# Same note alloc.ps1 prints, for the same reason (BACKLOG #1060): anchoring is correct but surprising, +# and a claim recorded to a worktree the caller is not standing in otherwise surfaces only as a refused +# commit later. Silent on the ordinary same-tree invocation. +$cwdTop = (& git rev-parse --path-format=absolute --show-toplevel 2>$null) +if ($cwdTop) { + $a = ($cwdTop.Trim() -replace '\\', '/').TrimEnd('/') + $b = ($repo -replace '\\', '/').TrimEnd('/') + if ($a -ine $b) { + Write-Host " NOTE: your shell is in $a, but this script lives in $b, so the claim is" -ForegroundColor Yellow + Write-Host " recorded to $b. -Release must be run against that same worktree." -ForegroundColor Yellow + } +} exit 0 diff --git a/tests/test_coord_claim_liveness.py b/tests/test_coord_claim_liveness.py index 04d81ef2..9bb784c3 100644 --- a/tests/test_coord_claim_liveness.py +++ b/tests/test_coord_claim_liveness.py @@ -50,10 +50,14 @@ def git(repo: Path, *args: str) -> str: @pytest.fixture def repo(tmp_path: Path) -> Path: r = tmp_path / "repo" - r.mkdir() + (r / "scripts" / "coord").mkdir(parents=True) subprocess.run(["git", "init", "-q", "-b", "main", str(r)], check=True, capture_output=True) git(r, "config", "user.email", "t@example.invalid") git(r, "config", "user.name", "t") + # Staged and committed for the reason written out in test_coord_claim_refresh.py's fixture: + # claim.ps1 anchors on its own location now (BACKLOG #1060), so the copy IS the sandbox, and a + # linked worktree of this fixture carries its own -- which is what `peer_holding` relies on. + shutil.copy2(CLAIM, r / "scripts" / "coord" / "claim.ps1") (r / "f.txt").write_text("x", encoding="utf-8") git(r, "add", "-A") git(r, "commit", "-qm", "base") @@ -61,8 +65,16 @@ def repo(tmp_path: Path) -> Path: def claim(cwd: Path, *args: str) -> subprocess.CompletedProcess[str]: + """Run the checkout's OWN copy -- it scopes itself to where it LIVES, not to the cwd.""" return subprocess.run( - ["pwsh", "-NoProfile", "-NonInteractive", "-File", str(CLAIM), *args], + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(cwd / "scripts" / "coord" / "claim.ps1"), + *args, + ], cwd=str(cwd), capture_output=True, text=True, diff --git a/tests/test_coord_claim_refresh.py b/tests/test_coord_claim_refresh.py index 49ef8a1f..6f65cb76 100644 --- a/tests/test_coord_claim_refresh.py +++ b/tests/test_coord_claim_refresh.py @@ -48,10 +48,17 @@ def git(repo: Path, *args: str) -> str: @pytest.fixture def repo(tmp_path: Path) -> Path: r = tmp_path / "repo" - r.mkdir() + (r / "scripts" / "coord").mkdir(parents=True) subprocess.run(["git", "init", "-q", "-b", "main", str(r)], check=True, capture_output=True) git(r, "config", "user.email", "t@example.invalid") git(r, "config", "user.name", "t") + # STAGE THE SCRIPT INSIDE THE FIXTURE, and commit it, because claim.ps1 anchors on its own location + # rather than on the cwd (BACKLOG #1060). This is also what makes the sandbox STRUCTURAL: these + # tests used to be scoped to a temp registry only by ambient cwd, so the moment the script stopped + # consulting cwd they began writing REAL claims into this clone's shared registry -- measured, two + # strays landed there on the first run after the fix. Committing it means a linked worktree of this + # fixture carries its own copy, which is how `peer_holding` still gets a claim held by the peer. + shutil.copy2(CLAIM, r / "scripts" / "coord" / "claim.ps1") (r / "f.txt").write_text("x", encoding="utf-8") git(r, "add", "-A") git(r, "commit", "-qm", "base") @@ -59,9 +66,20 @@ def repo(tmp_path: Path) -> Path: def claim(cwd: Path, *args: str) -> subprocess.CompletedProcess[str]: - """Run the real script from ``cwd`` -- it scopes itself to the cwd's repo, and has no -Repo.""" + """Run the checkout's OWN copy -- the script scopes itself to where it LIVES, not to the cwd. + + ``cwd`` still names the worktree under test, but it does so by HOLDING that copy rather than by + being the ambient directory, which is what a session invoking `scripts\\coord\\claim.ps1` does. + """ return subprocess.run( - ["pwsh", "-NoProfile", "-NonInteractive", "-File", str(CLAIM), *args], + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(cwd / "scripts" / "coord" / "claim.ps1"), + *args, + ], cwd=str(cwd), capture_output=True, text=True, diff --git a/tests/test_script_root_anchoring.py b/tests/test_script_root_anchoring.py index 0b12e835..5d51f4c6 100644 --- a/tests/test_script_root_anchoring.py +++ b/tests/test_script_root_anchoring.py @@ -18,6 +18,7 @@ from __future__ import annotations +import json import shutil import subprocess from pathlib import Path @@ -27,6 +28,8 @@ _ROOT = Path(__file__).resolve().parents[1] _SETUP_LEAK_GATE = _ROOT / "scripts" / "dev" / "setup-leak-gate.ps1" _SECURITY = _ROOT / "scripts" / "security" +_ALLOC = _ROOT / "scripts" / "coord" / "alloc.ps1" +_CLAIM = _ROOT / "scripts" / "coord" / "claim.ps1" pytestmark = pytest.mark.skipif( shutil.which("pwsh") is None, reason="pwsh (PowerShell 7) not on PATH" @@ -94,6 +97,123 @@ def test_setup_leak_gate_arms_the_checkout_it_lives_in_not_the_cwd(tmp_path: Pat assert not stray.exists(), f"armed the CALLER's checkout instead: {stray}" +def _coord_checkout(path: Path, *, drafted: int, boundary: int) -> Path: + """A minimal checkout carrying scripts/coord/ and the two files the allocator reads from ``$repo``. + + ``drafted`` is a BACKLOG heading written to the working tree and committed NOWHERE -- the floor term + that exists to catch exactly that, and the one where reading the wrong tree is worse than + misattribution: a number invisible to the sweep is a number free to re-issue. + + ``boundary`` is the ``PUBLIC_BACKLOG_FLOOR`` the allocator parses out of ledger_check.py rather than + restating. A stub is faithful here because the allocator reads it with a regex over the raw text, and + it gives a second, independent signal for which checkout was consulted. + """ + (path / "scripts" / "coord").mkdir(parents=True) + (path / "scripts" / "hooks").mkdir(parents=True) + (path / "docs").mkdir(parents=True) + shutil.copy2(_ALLOC, path / "scripts" / "coord" / "alloc.ps1") + shutil.copy2(_CLAIM, path / "scripts" / "coord" / "claim.ps1") + (path / "scripts" / "hooks" / "ledger_check.py").write_text( + f"PUBLIC_BACKLOG_FLOOR = {boundary}\n", encoding="utf-8" + ) + (path / "docs" / "BACKLOG.md").write_text( + f"# Backlog\n\n## {drafted}. A number drafted here and committed nowhere\n", + encoding="utf-8", + ) + _git("init", "-b", "main", ".", cwd=path) + _git("config", "user.email", "t@e.com", cwd=path) + _git("config", "user.name", "t", cwd=path) + _git("add", "-A", cwd=path) + _git("commit", "-m", "fixture", "--no-verify", cwd=path) + return path + + +def test_alloc_reads_the_floor_from_its_own_checkout_not_the_cwd(tmp_path: Path) -> None: + """BACKLOG #1060, asserted by ``-ShowFloor`` so no numbers are burned. + + Allocation is a one-way door -- claims are never released, "holes are free, collisions are not" -- + so a test that allocated would leave permanent holes in the shared registry for every worktree of + this clone. ``-ShowFloor`` exists for exactly this and computes the same floor without advancing the + ratchet. + + The caller's drafted number is deliberately HIGHER than the target's. The floor is a maximum, so a + wrong read shows up as a floor that is too high; equal numbers, or a lower one in the caller, would + both pass with the bug still in. + """ + named = _coord_checkout(tmp_path / "Named", drafted=4242, boundary=1200) + caller = _coord_checkout(tmp_path / "Caller", drafted=7777, boundary=1900) + + proc = subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(named / "scripts" / "coord" / "alloc.ps1"), + "-ShowFloor", + "-Kind", + "backlog", + ], + cwd=str(caller), # THE POINT: the shell is standing in the other checkout + capture_output=True, + text=True, + timeout=180, + ) + assert proc.returncode == 0, proc.stderr or proc.stdout + out = proc.stdout + + assert "floor : 4242" in out, f"read the CALLER's working tree, not its own:\n{out}" + assert "7777" not in out, f"the caller's drafted number reached the floor:\n{out}" + # Independent second signal: the boundary comes from /scripts/hooks/ledger_check.py, so it + # answers "which checkout" without depending on the floor arithmetic at all. + assert "boundary : 1200" in out, f"parsed the CALLER's ledger_check.py:\n{out}" + # And the registry it would write to must live beside the named checkout's object store. + watermark = next(line for line in out.splitlines() if line.startswith("watermark:")) + assert str(named).replace("\\", "/").casefold() in watermark.replace("\\", "/").casefold(), ( + watermark + ) + + +def test_claim_records_the_checkout_it_lives_in_not_the_cwd(tmp_path: Path) -> None: + """The same construct in claim.ps1, which was NOT filed -- found by inspection while fixing #1060. + + A numbered claim is enforced by scripts/hooks/claim_check.py at commit time. That hook reads the repo + from cwd and is right to: cwd IS the committing worktree for a commit hook. Only the tool can be + invoked from somewhere else, so only the tool needed anchoring. + """ + named = _coord_checkout(tmp_path / "Named", drafted=4242, boundary=1200) + caller = _coord_checkout(tmp_path / "Caller", drafted=7777, boundary=1900) + + proc = subprocess.run( + [ + "pwsh", + "-NoProfile", + "-NonInteractive", + "-File", + str(named / "scripts" / "coord" / "claim.ps1"), + "-Take", + "anchoring-probe", + "-Note", + "divergence fixture", + ], + cwd=str(caller), + capture_output=True, + text=True, + timeout=180, + ) + assert proc.returncode == 0, proc.stderr or proc.stdout + + written = list((named / ".git" / "mefor-coord" / "claims").glob("*.json")) + assert written, f"no claim landed in the named checkout's registry:\n{proc.stdout}" + recorded = json.loads(written[0].read_text(encoding="utf-8"))["worktree"] + assert recorded.replace("\\", "/").casefold() == str(named).replace("\\", "/").casefold(), ( + f"claim recorded against the CALLER: {recorded}" + ) + assert not (caller / ".git" / "mefor-coord" / "claims").exists(), ( + "the caller's registry was written to" + ) + + def test_setup_leak_gate_does_not_reintroduce_an_unanchored_toplevel(tmp_path: Path) -> None: """A spelling guard for the regression, paired with the behavioural test above. From a042c0fe4556dc4b3cdf094dfc4af85af523cacc Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 14:16:52 -0500 Subject: [PATCH 13/14] docs(backlog): record the test-isolation trap #1060's fix walked into (BACKLOG #1060) A cwd-dependence that reads as a defect in the tool can be load-bearing ISOLATION in its tests. Both claim test files were scoped to a throwaway registry purely by ambient cwd -- one said so in a docstring -- so anchoring the script turned the passing half of the run into a writer on this clone's shared registry before the rest of it went red. Recorded in the item rather than only in the commit message, because #1057 and #1059 are the remaining instances of the same class and will hit the same trap: check what a test is isolated BY before changing what the code reads. --- docs/BACKLOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 41556cf3..8315d36b 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5543,6 +5543,8 @@ The literal spellings are all caught, including the newline form. Only the indir **SEVERITY CORRECTION, 2026-08-06, made while fixing it.** The paragraph above is right about the *recorded owner* and wrong about the item as a whole, because the filing looked at one line and the defect was in four. The ledger gate does fail closed on a misattributed claim — but the floor's **working-tree term** was cwd-derived too, and that term has no gate behind it. Its job is to see a number written to `docs/BACKLOG.md` and committed **nowhere**; reading the caller's tree makes a number drafted in the *target* worktree invisible, so the allocator hands it out as free and two items end up sharing it in the same tree. Both are then owned by that worktree, so `owns()` passes and the ledger gate never fires. That is the exact silent collision the script's own docstring says it exists to prevent, arrived at through the script. The window is narrow — anything committed on any ref is still caught by the all-refs term — but it is a correctness hole, not friction, and it was reproduced in the negative control rather than reasoned about. +**AND THE FIX CONVERTED TWO SANDBOXED TESTS INTO WRITERS ON THE LIVE REGISTRY, which is the part worth remembering.** `tests/test_coord_claim_{refresh,liveness}.py` ran the **real** `scripts/coord/claim.ps1` with `cwd` set to a temp repo — scoped to a throwaway registry *purely by ambient cwd*, and one of them said so in a docstring: *"it scopes itself to the cwd's repo"*. The moment the script stopped consulting cwd, the passing half of the run wrote real claims into this clone's shared registry (two strays, removed by hand). So a cwd-dependence that looks like a defect in the tool can be load-bearing **isolation** in its tests, and removing it is a change to both. `tests/test_ledger_check.py` already staged a copy of `alloc.ps1` inside its fixture, which is exactly why it was the one file that did not break — the pattern existed and the two claim files were the outliers. Both now stage and commit the script into the fixture, so the sandbox is structural rather than ambient. Anyone fixing the remaining instances of this class (#1057, #1059) should check what their tests are isolated *by* before changing what the code reads. + **Reproduced 2026-08-05, twice, by accident.** A session ran `pwsh -NoProfile -File /scripts/coord/alloc.ps1 -Kind backlog` from worktree A while intending to commit from worktree B. `alloc/backlog/1058.json` recorded `"worktree": "<...>/trusting-wu-c2e6d5"`. The commit from `MessageFoundry-gate-deferrals` was then refused: *"BACKLOG item #1058 was not allocated to this worktree"* — true, unhelpful, and pointing at the allocator rather than at the invocation. Re-running with the shell actually inside the target worktree produced `1059.json` with the right owner and the commit went through. **#1058 is an abandoned hole**, which is the sanctioned outcome (`alloc.ps1`'s own docstring: *holes are free, collisions are not*). **The fix is small and there are two defensible shapes.** Either derive the repo from `$PSScriptRoot` so the allocator is anchored to the checkout it lives in — matching what `new.ps1` and `remove.ps1` already do — or keep the cwd behaviour and **say so at the point of use**, printing the recorded worktree in the `ALLOCATED` output so the mismatch is visible immediately rather than at commit time. The second is weaker but nearly free, and the two compose. Prefer anchoring: an allocator invoked by absolute path is being told which checkout to act on, and it should not then consult a different one. From 84eec220683ec5ad8e8fad779f25ade2586ad791 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 7 Aug 2026 11:58:19 -0500 Subject: [PATCH 14/14] docs(CONNECTIONS): repoint the serial/ASTM decline at the archive; #27 is closed The connector-parity row for Serial (RS-232) / ASTM E1381/E1394/E1318 cited the decline as ([BACKLOG.md](BACKLOG.md) #27). Item 27 is closed and lives at docs/archive/backlog/BACKLOG-CLOSED.md:994; it is not in the live ledger. The pointer sent a reader to the wrong file. This is the second half of a designated two-marker pair. The archived item's own banner names both markers -- "marker landed in PR #411 (CLAUDE.md section 12 + docs/CONNECTIONS.md Serial row)" -- and commit 8a14602d repointed the CLAUDE.md half while logging this one as still carrying the decline, because that commit was scoped to section 12. The two halves disagreed about where #27 lives until now. Form: an anchored link, matching the sibling convention already used in this same directory for this same target (docs/AOAG-DEPLOYMENT.md:389 and :476). The cell already opens with "declined-by-design (v0.2+)", so the citation's only job is to resolve; restating "closed" in the cell would duplicate a fact the cell asserts two clauses earlier. Relative path: (archive/backlog/BACKLOG-CLOSED.md), NOT (docs/archive/...). The link is repo-relative from inside docs/. CLAUDE.md is at the repo root and correctly uses the docs/-prefixed form; copying that form here would resolve to docs/docs/archive/... and 404. Verified, not assumed: - The anchor slug was derived by a rule first replayed against three anchors already committed in the repo (#100, #101, #52) -- 3 of 3 exact -- then applied to #27's heading, then confirmed to match exactly one real "## " heading in the target file. A bogus anchor was run through the same check and found nothing, so the check can report a miss. - Item locations come from parse_items imported from scripts/docs/backlog_status_check.py, per CLAUDE.md section 11 -- not a hand-rolled scan of the banner alphabet. - backlog_status_check.py still reports 363 items, unchanged. - Read from origin/main throughout; the primary checkout runs behind. NOT changed, deliberately, with the reason: - docs/BACKLOG.md:574 -- bare number inside the section headed "Value & priority analysis (recorded 2026-06-19) - superseded". A superseded snapshot is a historical record; it has no path to rot. - docs/testing/FEATURE-COVERAGE-PLAN.md:41 -- names the features in prose and carries no number or path at all. Nothing to rot; adding a pointer would be new scope, not a repair. - docs/testing/master-test-plan/00-strategy-and-governance.md:699 -- bare #26/#27 that resolve to nothing rather than to wrong content. Repairing one link here would leave a single correct relative link among 28 broken root-relative ones in the same file; it belongs in the doc-set-wide sweep that class needs. - docs/BACKLOG.md:906 -- a real defect, but larger than a pointer repair and in a file several sessions are editing. Reported separately for a decision. A wider scan (127 path-bearing BACKLOG citations) found roughly 90 more naming the live ledger for an archived item. Not touched here: the staleness is currently uniform, and repointing a subset would assert by contrast that the untouched siblings are live. That class needs one pass, not a trickle. --- docs/CONNECTIONS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index cd4219f8..f45d3e31 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -2433,7 +2433,7 @@ Legend: ✅ native · ~ partial / via extension / via another transport · ❌ n | **IBM MQ / MSMQ** | ~ | ❌ | ✅ | ❌ | not on roadmap | | **Kafka / streaming** | ~ | ❌ | ✅ | ❌ | not on roadmap | | **DICOM** (imaging) | ✅ | ~ | ✅ | ✅ | `DICOM-IN` C-STORE SCP (Phase 1) + `DICOM-OUT` C-STORE SCU/C-ECHO + `DICOMWEB-OUT` STOW-RS all shipped (ADR 0025); DICOMweb send exceeds both incumbents | -| **Serial (RS‑232)** + X/Y‑Modem/Kermit + **ASTM E1381/E1394/E1318** | ~ | ❌ | ✅ | ❌ | **declined-by-design (v0.2+)** — legacy/niche lab-instrument connectivity, no feed demand ([BACKLOG.md](BACKLOG.md) #27) | +| **Serial (RS‑232)** + X/Y‑Modem/Kermit + **ASTM E1381/E1394/E1318** | ~ | ❌ | ✅ | ❌ | **declined-by-design (v0.2+)** — legacy/niche lab-instrument connectivity, no feed demand ([BACKLOG #27](archive/backlog/BACKLOG-CLOSED.md#27-serial-rs-232--astm-e1381e1394e1318--decision-decline-unless-lab-analyzer-demand-no-build)) | | **FHIR** endpoint/client | ✅ | ✅ | ✅ | ~ | `FHIR-OUT` shipped (`FHIR()`, ADR 0022) + SMART Backend Services client auth (ADR 0024); the inbound **server facade** is deferred (BACKLOG #20) | | **Internal channel‑to‑channel** | ✅ | ✅ | ✅ | ✅ | the routing graph (wired by name) — plus two first-class internal inbounds: `Loopback()` (a captured reply) and `PassThrough()` (1:N internal re-ingress), ADR 0013 | | Printer / command‑line / screen‑scrape | ~ | ❌ | ✅ | ❌ | not on roadmap (niche) |