From 22460b6e74e1296777d62e3076e4dfaaa0207169 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 20 Aug 2026 13:43:37 -0500 Subject: [PATCH 1/7] fix(test): a zero-read engine now reports the engine's OWN reason instead of a bare assert 0 > 0 tests/test_multishard_smoke.py has been red on main with `assert e.reads > 0` firing as `assert 0 > 0`, and the failure could not be attributed. This does not change what the test ASSERTS. It changes what the failure SAYS, which is the part that was unusable. I HANDED THIS INVESTIGATION A HYPOTHESIS AND IT WAS WRONG. I reasoned that because the two assertions above it PASS -- `inbound_rows == _COUNT_PER_ENGINE` and `foreign_rows == 0` -- the rows had ARRIVED and only the counter was wrong, so this was a counter defect. That is refuted. Both of those counters are CONFIG-derived, not traffic-derived: the `/connections` builder appends a source row for EVERY registry inbound unconditionally and sets `read` to an int rather than None, so both pass unchanged on an engine that received NOTHING -- including one whose listeners never bound. The test's own docstring already conceded it: the isolation proof "is config-derived so it holds regardless of the write lock". `reads` is the ONLY traffic-derived counter of the three, and `reads == 0` means the engine genuinely received nothing. The pair I called discriminating carries zero traffic information. THE ENGINE ALREADY KNOWS WHY, AND THE HARNESS WAS THROWING IT AWAY. A lane that failed to start is reported as not-listening with a reason (ADR 0031, surfaced as `/connections`.error). The harness fetched that response, read `name` and `read` off each row, and discarded `error`. So the one artifact that could attribute the failure was fetched and dropped on every run. `EngineAttribution` now carries `failed_lanes`, the engine's verbatim reasons; the assertion prints them and distinguishes the two cases -- lanes reported as not listening (the engine never received traffic) versus no failed lanes at all (they bound and the traffic did not arrive or did not commit, a different cause this assertion cannot narrow further and now says so rather than implying it can). The JSON artifact carries the field too, so a CI reader with only the uploaded file can attribute it without re-running anything, which is the whole point. Collected for EVERY inbound row rather than only this engine's own: a lane failing under a peer's tag is equally diagnostic, and filtering by tag here would drop the cross-engine case the isolation assertion above exists to catch. READ DIRECTLY AS `row.error`, NOT `getattr(row, "error", None)`. `EngineClient.connections()` is typed `list[ConnectionRow]` and that model declares the field, so a default could only ever mask a RENAME -- after which this would report "no failed lanes" forever, silently, on precisely the runs it exists to explain. A diagnostic field that fails closed to "nothing to report" is worse than no field. VERIFIED with both controls, because a green run has no failed lanes and therefore proves nothing about the new path: failed-lane case -> inbound_rows=2, foreign_rows=0, reads=0 (the exact CI triple) AND both reasons collected into failed_lanes clean case -> reads=8, failed_lanes=() -- silent on success, so the field is not always-on noise tests/test_multishard_smoke.py: 2 passed. ruff 0.15.22 clean. No cp1252-unsafe character introduced. WHAT THIS DOES NOT DO, explicitly: it does not fix the CI red and does not claim to. Which of the conditions fired on CI is NOT ESTABLISHED -- the engine's own stdout is written to a temp file that `EngineNode` discards on stop unless `MEFOR_BENCH_KEEP_NODE_LOGS` names a directory, and the CI leg leaves it unset, so the bind-failure warning is thrown away on every run. Capturing that is the next step and is a ci.yml change I have not made. This commit makes the NEXT occurrence self-attributing, which is what four sessions lacked when a neighbouring red was mis-attributed three times today. --- harness/load/multishard.py | 34 +++++++++++++++++++++++++++++++++- tests/test_multishard_smoke.py | 28 +++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/harness/load/multishard.py b/harness/load/multishard.py index 71ae0fed1..59d60035d 100644 --- a/harness/load/multishard.py +++ b/harness/load/multishard.py @@ -91,6 +91,22 @@ class EngineAttribution: inbound_rows: int # number of inbound (source) connection rows this engine reports foreign_rows: int # inbound rows whose name does NOT carry this engine's tag (a steal ⇒ > 0) reads: int # Σ inbound read across this engine's own rows + #: The engine's OWN reason for each lane it reports as not-listening, verbatim from `/connections` + #: (`error`, which the API sets from `connection_failed()` per ADR 0031). Empty on a clean run. + #: + #: CARRIED BECAUSE `reads == 0` CANNOT DIAGNOSE ITSELF WITHOUT IT, and the engine already knows. + #: `inbound_rows` and `foreign_rows` are CONFIG-derived, not traffic-derived -- the API appends a + #: source row for every registry inbound unconditionally and `read` is always an int, never None + #: (`api/app.py`, the `/connections` builder), so both pass unchanged on an engine that received + #: NOTHING, including one whose listeners never bound. The test's own docstring says as much: the + #: isolation proof "is config-derived so it holds regardless of the write lock". So when `reads` + #: reads 0 the other two counters say nothing about why, and the failure message was left naming a + #: number with no cause attached. + #: + #: Measured: occupying one engine's inbound ports with a listen-never-accept squatter reproduces + #: the exact CI triple -- inbound_rows PASS, foreign_rows PASS, reads 0 FAIL -- and the engine + #: reported `status: "failed"` on both lanes throughout. The information was present and discarded. + failed_lanes: tuple[str, ...] = () @dataclass(frozen=True) @@ -195,6 +211,9 @@ def to_json_dict(self) -> dict[str, object]: "inbound_rows": e.inbound_rows, "foreign_rows": e.foreign_rows, "reads": e.reads, + # In the artifact as well as the assertion: a CI reader who has only the uploaded + # JSON must be able to attribute a zero-read engine without re-running anything. + "failed_lanes": list(e.failed_lanes), } for e in self.per_engine ], @@ -693,6 +712,7 @@ def _attribute_engines_sync( # empty attribution — the smoke asserts positive rows, so it won't silently pass. out.append(EngineAttribution(node.node_id, tag, 0, 0, 0)) continue + failed: list[str] = [] for row in rows: if row.read is None: # inbound (source) rows carry a read counter; skip outbound rows continue @@ -701,7 +721,19 @@ def _attribute_engines_sync( reads += row.read else: foreign_rows += 1 - out.append(EngineAttribution(node.node_id, tag, inbound_rows, foreign_rows, reads)) + # Collected for EVERY inbound row, not only this engine's own: a lane that failed to bind + # under a peer's tag is exactly as diagnostic, and filtering by tag here would drop the + # cross-engine case the isolation assertion above exists to catch. + # DIRECT ATTRIBUTE ACCESS, NOT `getattr(row, "error", None)`. `EngineClient.connections()` + # is typed `list[ConnectionRow]` and that model declares `error`, so the field is + # guaranteed and a default would only ever mask a RENAME -- after which this would report + # "no failed lanes" forever, silently, on exactly the runs it exists to explain. A + # diagnostic field that fails closed to "nothing to report" is worse than no field. + if row.error: + failed.append(f"{row.name}: {row.error}") + out.append( + EngineAttribution(node.node_id, tag, inbound_rows, foreign_rows, reads, tuple(failed)) + ) return out diff --git a/tests/test_multishard_smoke.py b/tests/test_multishard_smoke.py index ad81b082a..3ba44893c 100644 --- a/tests/test_multishard_smoke.py +++ b/tests/test_multishard_smoke.py @@ -141,7 +141,33 @@ async def test_multishard_two_engines_shared_sqlite() -> None: for e in rec.per_engine: assert e.inbound_rows == _COUNT_PER_ENGINE, e # exactly its own C lanes, no more assert e.foreign_rows == 0, e # none of a peer's lanes bled in - assert e.reads > 0, e # this engine independently received traffic on its own lanes + # This engine independently received traffic on its own lanes. + # + # THE MESSAGE CARRIES THE ENGINE'S OWN REASON, because the two assertions above CANNOT supply + # one. `inbound_rows` and `foreign_rows` are CONFIG-derived, not traffic-derived -- the API + # emits a source row for every registry inbound and `read` is always an int -- so both pass + # unchanged on an engine that received NOTHING, including one whose listeners never bound. + # The docstring above already concedes this ("config-derived so it holds regardless of the + # write lock"); the consequence for THIS line is that a bare `assert 0 > 0` names a number and + # no cause, which is what made the CI red undiagnosable. + # + # The engine knows: it reports the lane as not-listening with a reason (ADR 0031, surfaced as + # `/connections`.error), and the harness was fetching that response and discarding the field. + # Reproduced deterministically with a control -- a listen-never-accept squatter on one engine's + # inbound ports gives exactly inbound_rows PASS / foreign_rows PASS / reads 0 FAIL, with the + # engine reporting failed lanes throughout. + assert e.reads > 0, ( + f"engine {e.name_tag} read 0 messages on its own lanes. " + + ( + f"IT REPORTS THESE LANES AS NOT LISTENING: {e.failed_lanes}. That is the cause -- the " + f"engine never received traffic, rather than receiving it and miscounting." + if e.failed_lanes + else "It reports NO failed lanes, so the listeners bound and the traffic did not " + "arrive or did not commit -- a different cause from a bind failure, and one this " + "assertion cannot narrow further on its own." + ) + + f" Full attribution: {e}" + ) # (c) Zero-loss end-to-end is NOT required on shared SQLite (the single writer can strand a row on a # SQLITE_LOCKED delivery commit — the server-DB bench is the real zero-loss gate). A clean drain is a From 0c242c9941dcebb5b7e5fde8c5b6a656d512427e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 20 Aug 2026 23:54:33 -0500 Subject: [PATCH 2/7] test(321): per-class coverage that exercises the LOADED token set instead of a monkeypatched one BACKLOG #321, BUILD HALF ONLY. Owner-directed. The floor raise in security.yml is NOT here -- see the bottom of this message. THE DEFECT. Every per-class test in tests/test_scan_forbidden.py runs behind the `sf` fixture, which monkeypatches synthetic values over FORBIDDEN / ESTATE_TOKENS / SITE_CODE_RE / _SITE_CODE_FILE. They prove the machinery matches a pattern someone handed it, and never touch the load path. With no prefix loaded both site detectors fall back to `_NEVER` (scan_forbidden.py:194), an empty negative lookahead that matches nothing anywhere -- so a blind scanner and a clean tree are the same green tick, and the suite that looks like per-class coverage CANNOT FAIL when the real token set is wrong. That is the shape of the original defect, reproduced inside its own test suite. TWO ARMS, split by what each is allowed to touch. BEHAVIOURAL: pins the source to the committed synthetic example and drives the REAL pipeline -- MEFOR_FORBIDDEN_TOKENS -> _resolve_token_text -> _parse_tokens -> compilation -> scan_file. Nothing is monkeypatched onto the globals, and probes are DERIVED from what loaded, so a set that loads blind never reaches an assertion: the derivation fails first. REAL SET: runs only where a real source is configured, and asserts STRUCTURE ONLY -- present, not the sentinel, every class counted. It never reads, builds with, or reports a real token. Proving the scanner catches a real token would require putting one in this file, which is exactly the disclosure the scanner exists to prevent (CLAUDE.md sec. 9); the test would become the leak. A HOLE I FOUND IN MY OWN FIRST CUT AND CLOSED. A configured-but-MANGLED source leaves TOKENS_PRESENT false, identically to having no source at all -- so skipping on that alone turned the documented cutover-mangling case (headers lost, comments only, a BOM before the first section) into a green tick. Only the ABSENCE of a source is now a skip; a source that exists and parsed to nothing FAILS. AND ONE THE RED-FIRST PASS FOUND IN MY OWN TEST, recorded because it is this item's exact subject. With FORBIDDEN forced empty, the [names] arm still PASSED -- green against the very class it names. The sets OVERLAP BY DESIGN (a customer name is typically in [names] AND [estate]), so the probe word drawn from [names] was also an estate token and the estate detector produced the hit. The probe is now filtered to a candidate no other detector can explain, and the arm goes red as it should. An over-determined assertion is not coverage, which is the whole reason this item exists. ASSERTED DELIBERATELY, AND NOT: - the estate arm asserts the scan_file PATH, not a count. [estate_body_only] tokens are held out of _ESTATE_FILE_RES and never enter scan_file, while raising the `estate` count identically -- so a count cannot tell a token the file scanner sees from one it does not. - never reason TEXT. The scanner substitutes a generic reason when a reason would itself match a detector, so asserting wording reads the substituted value rather than the finding. - a negative control per class, so an arm cannot be satisfied by a detector that flags everything, and a negative control on the blind state itself, so the guard cannot be asserting something vacuously true. RED-FIRST, ALL FIVE, each broken in scan_forbidden.py and watched to fail, then restored: site prefixes forced to the sentinel; estate held out of the file scan; names loaded empty; the site detector widened to any six-digit run; the blind fallback made unreachable. NOT DONE, AND NOT MINE TO DO: MEFOR_MIN_DETECTORS in .github/workflows/security.yml stays at names=7,estate=13,site_prefixes=1. Raising it to 8/14/2 hard-fails a required check until the owner updates BOTH the Actions and the Dependabot secrets -- both, or every Dependabot PR fails. I will ask rather than infer that from any message. VERIFIED: ruff check + format clean; 102 tests across the three scan_forbidden suites; glyph scan 0 over the new file against a 736-hit positive control. No token value appears in this file, in any assertion message, or in this commit. Co-Authored-By: Claude Opus 5 --- tests/test_scan_forbidden_loaded_set.py | 289 ++++++++++++++++++++++++ 1 file changed, 289 insertions(+) create mode 100644 tests/test_scan_forbidden_loaded_set.py diff --git a/tests/test_scan_forbidden_loaded_set.py b/tests/test_scan_forbidden_loaded_set.py new file mode 100644 index 000000000..eb0ed79b4 --- /dev/null +++ b/tests/test_scan_forbidden_loaded_set.py @@ -0,0 +1,289 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""BACKLOG #321, build half -- per-class coverage that exercises the LOADED token set. + +WHY THIS FILE EXISTS, AND WHY THE EXISTING PER-CLASS TESTS DO NOT COVER IT. Every per-class test in +``test_scan_forbidden.py`` runs behind the ``sf`` fixture, which monkeypatches SYNTHETIC values over +``FORBIDDEN`` / ``ESTATE_TOKENS`` / ``SITE_CODE_RE`` / ``_SITE_CODE_FILE``. Those tests prove the +MACHINERY matches a pattern someone handed it. They never touch the load path, so they say nothing +about whether a real token set arrives compiled and able to match. + +THE FAILURE MODE THAT MAKES THAT A DEFECT RATHER THAN A GAP. With no prefix loaded, ``SITE_CODE_RE`` +and ``_SITE_CODE_FILE`` both fall back to ``_NEVER`` (``scan_forbidden.py:194``), an empty negative +lookahead that matches NOTHING ANYWHERE. A blind scanner and a scanner with nothing to report are +the same green tick. So the suite that looks like per-class coverage cannot fail when the real set is +wrong -- which is the shape of the original defect, reproduced inside its own tests. + +THE TWO ARMS, and the split is about what each is allowed to touch: + +* THE BEHAVIOURAL ARM pins the source to the COMMITTED SYNTHETIC EXAMPLE and drives the REAL + pipeline -- ``MEFOR_FORBIDDEN_TOKENS`` -> ``_resolve_token_text`` -> ``_parse_tokens`` -> + compilation -> ``scan_file``. Nothing is monkeypatched onto the globals. Probes are DERIVED from + what actually loaded, so a set that loads blind cannot reach the assertion: the derivation itself + fails first. +* THE REAL-SET ARM runs only where a real (non-synthetic) source is configured, and asserts + STRUCTURE ONLY -- present, not ``_NEVER``, counted. It never reads, builds with, or reports a real + token, because this repository's forbidden-content guard exists to keep exactly those values out of + files like this one (CLAUDE.md sec. 9). A test that embedded one to prove the scanner catches it + would be the leak it is testing for. + +TWO THINGS THIS FILE DELIBERATELY DOES NOT ASSERT: + +* NOT REASON TEXT. The scanner substitutes a generic reason when a reason string would itself match a + detector, so an assertion on reason wording reads the substituted value rather than the finding. + These tests assert that a hit OCCURRED, on a probe line constructed so nothing else could have + produced it. +* NOT A BARE DETECTOR COUNT. A token added under ``[estate_body_only]`` raises the ``estate`` count + identically to one under ``[estate]`` while never entering ``scan_file``. Counting alone therefore + cannot tell a token that is scanned from one that is merely listed, so the estate arm asserts the + ``scan_file`` path itself. +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from typing import Any + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "scripts" / "security")) + +import scan_forbidden as sfm # noqa: E402 + +pytestmark = pytest.mark.tooling + +EXAMPLE = Path(sfm.__file__).parent / "scan-tokens.local.txt.example" + + +def _load(monkeypatch: pytest.MonkeyPatch, source: str | None) -> Any: + """Drive the REAL load path and hand back the module. + + Deliberately NOT a monkeypatch of the globals: the point of this file is that everything from + ``_resolve_token_text`` through pattern compilation actually runs. + """ + if source is None: + monkeypatch.setenv("MEFOR_FORBIDDEN_TOKENS", "") + else: + monkeypatch.setenv("MEFOR_FORBIDDEN_TOKENS", source) + sfm.reload_tokens() + return sfm + + +@pytest.fixture +def example(monkeypatch: pytest.MonkeyPatch) -> Any: + """The committed synthetic example, loaded through the real pipeline. + + Restored afterwards by reloading from the ambient environment, so a real local token file is not + left displaced for the rest of the session. + """ + mod = _load(monkeypatch, str(EXAMPLE)) + yield mod + monkeypatch.undo() + sfm.reload_tokens() + + +def _word_probes_for_names(text: str) -> list[str]: + """Words a ``\\b``-anchored ``[names]`` entry is expected to match, RECOVERED FROM THE SOURCE. + + A ``[names]`` entry is ``regex | reason | flags``, so the loaded table holds compiled patterns and + the literal is gone -- and a regex cannot be inverted into a matching string in general. Rather + than type a probe here (which would drift silently from the file it is meant to exercise), this + recovers the plain-word entries, which are the shape the class is overwhelmingly made of, and + ignores the rest. Recovering NOTHING is treated as a failure by the caller, so a source whose + shape changed cannot quietly turn this into a no-op. + """ + import re as _re + + probes: list[str] = [] + in_names = False + for raw in text.splitlines(): + line = raw.strip() + if line.startswith("["): + in_names = line.lower().startswith("[names]") + continue + if not in_names or not line or line.startswith("#"): + continue + pattern = line.split("|")[0].strip() + if m := _re.fullmatch(r"\\b([A-Za-z][A-Za-z0-9]*)\\b", pattern): + probes.append(m.group(1)) + return probes + + +# --- the anti-vacuity guards: these run FIRST because every assertion below is worthless without --- + + +def test_the_example_load_produces_non_blind_detectors(example: Any) -> None: + """THE GUARD THE REST OF THIS FILE STANDS ON. + + ``_NEVER`` matches nothing anywhere, so a scanner that loaded nothing reports a clean tree and a + scanner with nothing to find reports a clean tree. Asserting the detectors are not the sentinel is + what makes every "no hit" result below mean something. + """ + assert example.TOKENS_PRESENT, "the example did not load as a usable token source" + assert example.SITE_CODE_RE is not example._NEVER + assert example._SITE_CODE_FILE is not example._NEVER + + counts = example.loaded_token_counts() + for section in ("names", "estate", "site_prefixes"): + assert counts[section] > 0, f"class {section!r} loaded ZERO detectors from the example" + # estate_file_scanned is the subset that scan_file can actually reach. Zero here with a non-zero + # estate count means every token is body-only, which no file-scan assertion could detect. + assert counts["estate_file_scanned"] > 0 + + +def test_a_source_that_loads_nothing_is_reported_blind_rather_than_clean( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """THE NEGATIVE CONTROL FOR THE GUARD ABOVE -- it must be able to observe the blind state. + + Without this, the guard could be asserting a condition that is simply always true, and would pass + just as happily if ``_NEVER`` were unreachable. This pins that the blind state EXISTS and is + distinguishable, which is the whole premise of the file. + """ + mod = _load(monkeypatch, None) + try: + assert not mod.TOKENS_PRESENT + assert mod.SITE_CODE_RE is mod._NEVER + assert mod._SITE_CODE_FILE is mod._NEVER + counts = mod.loaded_token_counts() + assert counts["names"] == 0 and counts["estate"] == 0 and counts["site_prefixes"] == 0 + finally: + monkeypatch.undo() + sfm.reload_tokens() + + +# --- per class, over what actually loaded --------------------------------------------------------- + + +def test_the_loaded_names_class_matches_a_token_it_loaded(example: Any, tmp_path: Path) -> None: + """Class [names], driven end-to-end rather than through a handed-in pattern. + + The probe is recovered FROM THE SOURCE that loaded rather than typed here, so it cannot drift away + from the file it is meant to exercise, and a source that failed to load reaches no assertion at + all: there is nothing to recover a probe from. + """ + probes = _word_probes_for_names(EXAMPLE.read_text(encoding="utf-8")) + assert probes, "recovered no plain-word [names] entry to probe with -- the source shape changed" + + # THE PROBE MUST BE ATTRIBUTABLE TO THIS CLASS ALONE, and the first candidate is not: the sets + # OVERLAP BY DESIGN (a customer name is typically in [names] AND [estate]), so a word drawn from + # [names] is often an estate token too, and the estate detector then produces the hit. Measured: + # with `FORBIDDEN` forced empty this test still PASSED on the first candidate -- green against the + # very class it names. Discard any candidate another detector can explain. + def _line(word: str) -> str: + return f"contact {word} about the interface" + + estate_pats = [pat for _token, pat in example._ESTATE_FILE_RES] + usable = [ + w + for w in probes + if not any(p.search(_line(w)) for p in estate_pats) + and not example._SITE_CODE_FILE.search(_line(w)) + ] + assert usable, ( + "every recovered [names] probe is also matched by another class, so no hit here could be " + "attributed to [names] -- this arm cannot be made to mean anything against this source" + ) + + probe = tmp_path / "note.md" + probe.write_text(_line(usable[0]) + "\n", encoding="utf-8") + hits = example.scan_file(probe) + + assert hits, "a loaded [names] token in a file body produced no hit" + + +def test_the_loaded_estate_class_is_caught_on_the_FILE_SCAN_path( + example: Any, tmp_path: Path +) -> None: + """Class [estate], asserted through ``scan_file`` -- NOT through a count and NOT through scan_text. + + THE DISTINCTION IS THE POINT. ``[estate_body_only]`` tokens are excluded from ``_ESTATE_FILE_RES`` + and therefore never enter ``scan_file`` at all, while still raising the ``estate`` detector count + exactly as a scanned token does. So a test that watched the count could not tell a token the file + scanner can see from one it cannot, and the leak this class exists for is a token sitting in a + tracked file. + + The probe butts the token against identifier characters on an otherwise-unremarkable line: estate + patterns run LAST in ``scan_file`` and only on a line no other detector flagged, so this shape is + both the case only estate can reach and the one that keeps the hit attributable. + """ + scanned = [token for token, _pat in example._ESTATE_FILE_RES] + assert scanned, "no estate token is file-scanned, so this path cannot be exercised" + + probe = tmp_path / "config.txt" + probe.write_text(f"OB_{scanned[0]}_ORU\n", encoding="utf-8") + hits = example.scan_file(probe) + + assert hits, "a file-scanned estate token butted against identifier characters produced no hit" + + +def test_the_loaded_site_prefix_class_matches_a_prefix_it_loaded( + example: Any, tmp_path: Path +) -> None: + """Class [site_prefix], built from the prefix that actually loaded plus a four-digit run. + + ``SITE_CODE_RE`` is the detector that falls back to ``_NEVER``, so this is the class where a + silent load failure is indistinguishable from a clean tree. + """ + assert example._SITE_PREFIXES, "no site prefix loaded, so this class cannot be exercised" + code = f"{example._SITE_PREFIXES[0]}0000" + + probe = tmp_path / "note.md" + probe.write_text(f"the record was filed under {code} last week\n", encoding="utf-8") + hits = example.scan_file(probe) + + assert hits, "a site code built from a loaded prefix produced no hit" + + +def test_a_digit_run_with_no_loaded_prefix_is_not_flagged(example: Any, tmp_path: Path) -> None: + """The per-class negative control: the site detector must not match any six-digit run. + + Without this the class arm above is satisfied by a detector that flags everything, which is the + other way an instrument stops discriminating. + """ + probe = tmp_path / "note.md" + probe.write_text("order 4815162342 shipped\n", encoding="utf-8") + + assert example.scan_file(probe) == [] + + +# --- the real set, when one is configured --------------------------------------------------------- + + +def test_a_configured_REAL_token_set_is_loaded_and_not_blind() -> None: + """The arm that covers the environment the gate actually protects. + + STRUCTURE ONLY, AND THAT IS NOT A SHORTCUT. Asserting a real token matches would require putting + one in this file, which is precisely the disclosure the scanner exists to prevent (CLAUDE.md + sec. 9) -- the test would become the leak. What is checkable without handling a value is that the + set LOADED, that no class fell back to the sentinel, and that every class is populated. That is + the blind-set failure, which is the one this item is about. + + Skips where no real set is configured, and says which state it saw: a silent skip here would be + indistinguishable from a pass, and this is the arm most likely to be silently absent in CI. + """ + sfm.reload_tokens() + # A SOURCE THAT EXISTS BUT PARSED TO NOTHING IS A FAILURE, NOT A SKIP -- and separating the two is + # the point. Both states leave TOKENS_PRESENT false, so skipping on that alone would turn the + # documented mangling case (headers lost, comments only, a BOM ahead of the first section -- the + # cutover runbook has the owner paste a whole file into a secret box) into a green tick, which is + # the vacuous pass this item exists to remove. Only the ABSENCE of any source is a legitimate skip. + configured = sfm._resolve_token_text() is not None + if not configured: + pytest.skip("no token source configured in this environment") + assert sfm.TOKENS_PRESENT, ( + "a token source IS configured but parsed to zero detectors -- the source is present and " + "unusable, which reports identically to having none" + ) + if sfm.is_synthetic_token_set(): + pytest.skip("token source is the shipped synthetic example, not a real set") + + counts = sfm.loaded_token_counts() + assert sfm.SITE_CODE_RE is not sfm._NEVER + assert sfm._SITE_CODE_FILE is not sfm._NEVER + for section in ("names", "estate", "site_prefixes"): + assert counts[section] > 0, f"real token set loaded ZERO detectors for class {section!r}" + assert counts["estate_file_scanned"] > 0, ( + "every real estate token is body-only, so scan_file covers none of them" + ) From ce693091accd7a67198c9d495cd4d3a24b5bd7a0 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 05:17:44 -0500 Subject: [PATCH 3/7] fix(test): a pid that is FREE is not a pid that stays free (BACKLOG #1360) Three test files each spawned `cmd /c exit`, waited for it to EXIT, slept, and returned its pid as "free". The pid is free at the moment it returns and NOTHING KEEPS IT FREE: between that return and the moment the tool under test reads the record, the OS may hand it to a new process. Replaced all three with tests/_dead_pid.py, returning 2147483647 (Int32.MaxValue): within the [int] cast the fence performs, non-zero so it takes the liveness path, and structurally unassignable on either platform -- dead by construction rather than by timing. Lifted from stranded PR 531 (per Cleaner's grading) onto a fresh branch. The number changed from #1303 to #1360: #1303 was legitimately allocated on 2026-08-21 but bound to a worktree confirmed dead (fleet.ps1: freshest record INTERRUPTED at 119+ hours; absent from git worktree list), and the ledger gate's allocation registry has no rebind mechanism for either kind (verified by reading scripts/coord/alloc.ps1 in full -- it always searches forward for a free slot, never re-touches an existing one). Renumbered to the freshly allocated #1360 rather than hand-edit the protected registry file, which a PreToolUse hook explicitly and correctly refuses. #1303 stays a permanent hole, which this project's own tooling treats as free ("holes are free, collisions are not"). The ledger banner is Lander-authored per ADR 0165 (the code fix is the builder's own verified work, landed unmodified) -- verified independently against the landed diff before writing it: tests/_dead_pid.py exists, defines NEVER_LIVE_PID: Final = 2147483647, and all three test files import it. Co-Authored-By: Claude Opus 5 --- docs/BACKLOG.md | 37 ++++++++++++++++ tests/_dead_pid.py | 66 +++++++++++++++++++++++++++++ tests/test_coord_presence.py | 14 ++---- tests/test_session_registry.py | 15 ++----- tests/test_worktree_prune_merged.py | 13 ++---- 5 files changed, 113 insertions(+), 32 deletions(-) create mode 100644 tests/_dead_pid.py diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 83bef1c4d..51ef5698b 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -16262,3 +16262,40 @@ apart correctly. declared serialised write order on that file. Building this alongside those without agreeing an order first risks the exact same-file collision this project's own collision-detection conventions exist to catch. + +--- + +## 1360. three test files pick a free pid and rely on it staying free, so a loaded runner reuses it and a DEAD record reads as a veto + +> ✅ **SHIPPED 2026-08-26 (lander, authored per ADR 0165 -- the underlying fix is the builder's own +> verified work, landed unmodified; this banner is mine).** Three copies of a `_find_free_pid` helper +> (`test_worktree_prune_merged.py`, `test_coord_presence.py`, `test_session_registry.py`) each spawned +> `cmd /c exit`, waited for it to exit, slept, and returned its pid as "free" -- free at the moment it +> returns, and nothing keeps it free afterward. Replaced by one `tests/_dead_pid.py`, returning +> `2147483647` (`Int32.MaxValue`): within the `[int]` cast `Test-RecordLiveness` performs, non-zero so +> it takes the liveness path rather than the `UNREADABLE` shortcut, and structurally unassignable on +> either platform (Linux caps pids at ~2^22, Windows pids are multiples of 4 far below 2^31) -- dead by +> construction rather than by timing. Verified independently against the landed diff before writing +> this banner: `tests/_dead_pid.py` exists, defines `NEVER_LIVE_PID: Final = 2147483647`, and all +> three test files import it. +> +> **Diagnosed from a real failure**, not from inspection alone: `windows-2025` run `32268545492`, +> `tests/test_worktree_prune_merged.py:753`, `assert d["Occupants"] == []`, an ordinary exit-1 +> assertion beside an unrelated crash in the same run. The comment on the original helper -- +> `time.sleep(0.3) # let the OS reap it before we claim the pid is gone` -- states the intended +> guard and the mechanism does the opposite: reaping RELEASES a pid for reuse rather than reserving +> it, so the sleep widens the window it appears to close. +> +> **The fix does not stub, mock, or force the liveness verdict** -- the real `Get-Process` call still +> runs and still returns "not running" on its own; only the pid handed to it is now unassignable. The +> constraint that shaped this over the obvious alternative: the tests assert both `Occupants == []` +> and `Decision == "SKIP"`, so any remedy that short-circuited the verdict would pass while testing +> nothing. + +**Cluster:** CI reliability / test determinism. **Priority:** P2. **Verdict:** build. +**Severity:** no product effect, no PHI effect, no deployment axis (sec. 0) -- test-suite determinism +only. The cost was a required context redding on a race whose failure looked like a real occupancy +veto. + +--- + diff --git a/tests/_dead_pid.py b/tests/_dead_pid.py new file mode 100644 index 000000000..a19e43e9f --- /dev/null +++ b/tests/_dead_pid.py @@ -0,0 +1,66 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""A pid whose DEADNESS HOLDS -- BACKLOG #1360. + +THE DEFECT THIS REPLACES. Three test files each carried their own copy of:: + + proc = subprocess.Popen(["cmd", "/c", "exit"], ...) + proc.wait(timeout=30) + time.sleep(0.3) # let the OS reap it before we claim the pid is gone + return proc.pid + +It spawns a process, waits for it to EXIT, and returns its pid as "free". The pid is free at the +moment it returns and **nothing keeps it free**: between that return and the moment the tool under +test reads the record, the OS may hand the pid to a new process. + +**THE COMMENT STATES THE INTENT AND THE MECHANISM DOES THE OPPOSITE.** Reaping does not RESERVE a pid, +it RELEASES it for reuse -- so the sleep widens the window it appears to guard. The hazard was +reasoned about and the direction was inverted, which is why this is a defect rather than an oversight. + +HOW IT SURFACED. `Test-RecordLiveness` (`scripts/coord/session-registry.ps1:181`) reads +`Get-Process -Id `: not running is **DEAD**, which vetoes nothing. But a REUSED pid IS running, +and a test record carries no ``startedAt`` for the reuse fence to check, so the verdict becomes +**UNVERIFIED** -- and UNVERIFIED *does* veto. The occupant list then comes back non-empty and an +assertion that a dead record is "not a veto" fails. Observed on `windows-2025`, run `32268545492`:: + + assert d["Occupants"] == [] + AssertionError: assert [{'Short': 'e...-clean', ...}] == [] + +`cmd /c exit` is Windows-only, which matches where it was seen. Pid reuse needs pid churn, and that +tier spawns pwsh/git children constantly, on a runner whose pid space recycles far faster than a +developer box -- which is why it reproduces there and not locally. + +WHY THIS VALUE. ``2147483647`` is ``Int32.MaxValue``. It is: + +* **within ``[int]``**, which `Test-RecordLiveness` casts to (``$procId = [int]$Record.pid``); +* **non-zero**, so it takes the liveness path rather than the ``UNREADABLE`` shortcut that a falsy + pid triggers -- a record with no pid is deliberately NOT dead there; +* **structurally unassignable**: Linux caps pids at ``/proc/sys/kernel/pid_max`` (default 4194304, + ceiling ~2^22) and Windows pids are multiples of 4 far below 2^31. + +So it is dead **by construction rather than by timing**, and it stays dead however loaded the host is. + +WHAT THIS DELIBERATELY DOES *NOT* DO, and it is the point. It does not stub, mock or force the +liveness verdict. The real `Get-Process` call still runs and still returns "not running" on its own. +A remedy that short-circuited the verdict would make every caller pass while testing NOTHING -- and +the assertions this feeds exist to prove that a dead record is *neither* a veto *nor* a permission +(``Occupants == []`` AND ``Decision == "SKIP"``). Converting a loud false-failure into a quiet +always-pass would be worse than the flake it replaces. +""" + +from __future__ import annotations + +from typing import Final + +#: See the module docstring for why this specific value, and why it is not merely "a big number". +NEVER_LIVE_PID: Final = 2147483647 + + +def never_live_pid() -> int: + """A pid no process can hold, so a record written with it reads DEAD at any later moment. + + Callable rather than a bare constant so call sites read as an intent ("give me a pid that cannot + be alive") rather than as a magic literal, and so a future platform that needs a different value + has one place to change. + """ + return NEVER_LIVE_PID diff --git a/tests/test_coord_presence.py b/tests/test_coord_presence.py index 83a1f08f4..c3c87e740 100644 --- a/tests/test_coord_presence.py +++ b/tests/test_coord_presence.py @@ -29,6 +29,8 @@ import pytest +from tests._dead_pid import never_live_pid + PRESENCE = Path(__file__).resolve().parents[1] / "scripts" / "coord" / "presence.ps1" pytestmark = pytest.mark.skipif( @@ -159,7 +161,7 @@ def test_pid_reuse_is_not_reported_live(repo: Path, config_root: Path) -> None: def test_dead_pid_is_excluded_by_default_and_shown_with_all(repo: Path, config_root: Path) -> None: - dead = _find_free_pid() + dead = never_live_pid() write_session(config_root, pid=dead, cwd=repo, session_id="dddddddd-4444") assert run_presence(repo, config_root) == [] @@ -273,16 +275,6 @@ def test_the_human_table_names_its_columns_so_the_id_cannot_read_as_a_sha( ) -def _find_free_pid() -> int: - """A pid that is not currently running -- start a process, note its pid, wait for it to exit.""" - proc = subprocess.Popen( - ["cmd", "/c", "exit"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL - ) - proc.wait(timeout=30) - time.sleep(0.3) # let the OS reap it before we claim the pid is gone - return proc.pid - - def test_outside_a_repo_the_json_roster_carries_an_unavailable_receipt(tmp_path: Path) -> None: """ "I could not look" must not render as "nobody is live" on the machine-readable channel. diff --git a/tests/test_session_registry.py b/tests/test_session_registry.py index e21587e96..f962c3f65 100644 --- a/tests/test_session_registry.py +++ b/tests/test_session_registry.py @@ -30,6 +30,8 @@ import pytest +from tests._dead_pid import never_live_pid + REGISTRY = Path(__file__).resolve().parents[1] / "scripts" / "coord" / "session-registry.ps1" pytestmark = pytest.mark.skipif( @@ -108,7 +110,7 @@ def test_recycled_pid_is_not_live(config_root: Path) -> None: def test_dead_pid_is_dead(config_root: Path) -> None: - write_session(config_root, pid=_find_free_pid(), session_id="cccccccc-3333") + write_session(config_root, pid=never_live_pid(), session_id="cccccccc-3333") assert liveness(config_root, "cccccccc")["State"] == "DEAD" @@ -154,15 +156,6 @@ def test_malformed_record_does_not_break_the_lookup(config_root: Path) -> None: def test_prefix_match_reports_the_most_alive_candidate(config_root: Path) -> None: """Deciding whether it is safe to disturb something: an ambiguous prefix must not resolve to the dead one and green-light the move.""" - write_session(config_root, pid=_find_free_pid(), session_id="7777abcd-8888") + write_session(config_root, pid=never_live_pid(), session_id="7777abcd-8888") write_session(config_root, pid=os.getpid(), session_id="7777efgh-9999") assert liveness(config_root, "7777")["State"] == "LIVE" - - -def _find_free_pid() -> int: - proc = subprocess.Popen( - ["cmd", "/c", "exit"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL - ) - proc.wait(timeout=30) - time.sleep(0.3) - return proc.pid diff --git a/tests/test_worktree_prune_merged.py b/tests/test_worktree_prune_merged.py index 1ceb63a2e..756371281 100644 --- a/tests/test_worktree_prune_merged.py +++ b/tests/test_worktree_prune_merged.py @@ -48,6 +48,8 @@ import pytest +from tests._dead_pid import never_live_pid + SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "worktree" / "prune-merged.ps1" # TWO marks, and the timeout one is load-bearing on CI. Every subprocess wait in this file outlives @@ -761,7 +763,7 @@ def test_a_record_with_no_cwd_is_unplaceable_and_refuses_too(fx: Fixture, sleepe def test_dead_record_is_not_a_veto_and_not_a_permission(fx: Fixture, sleeper: int) -> None: """Liveness may only VETO. A DEAD verdict must not authorise the removal by itself.""" - dead = _find_free_pid() + dead = never_live_pid() fx.write_session(pid=dead, cwd=fx.sibling("clean"), session_id="eeeeeeee-5555") live_record(fx, sleeper, fx.primary) # keeps the fence available @@ -1635,15 +1637,6 @@ def test_a_name_that_matches_nothing_does_not_exit_green(fx: Fixture, sleeper: i ) -def _find_free_pid() -> int: - proc = subprocess.Popen( - ["cmd", "/c", "exit"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL - ) - proc.wait(timeout=30) - time.sleep(0.3) - return proc.pid - - # -------------------------------------------------------------------------------------------------- # Coordination claims stranded by a removal (BACKLOG #345) # From 5151ee1ff8f8931a0f396d4f181fda1b5421b017 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 05:36:30 -0500 Subject: [PATCH 4/7] fix(coord): refuse a wrong-namespace -ToSessionId at SEND, not at the inbox (BACKLOG #1302) An id-addressed message could strand silently. `mail-drain.ps1:852` compares the recorded `to.sessionId` against the reading session's harness id with `-ne`; an id from another namespace never matches, so the message sat in the inbox until it was swept to expired/ -- with the send path printing `Queued 1 message(s)` the whole time. THE ASYMMETRY WAS THE DEFECT, AND IT IS WHY THE GUARD IS ON THE SEND SIDE. The drain already reported its half ("N message(s) are addressed to a different session id and were left in the inbox"), so the RECIPIENT was told. The SENDER was told nothing -- and the sender is the only party who can correct the id. I did not touch the drain's filter: it is CORRECT. A worktree outlives its occupant, so an id-addressed note must not reach a stranger, and loosening the match to "fix" delivery would trade a silent non-delivery for a silent MIS-delivery, which is worse. THE SHAPE. A harness session id is a bare UUID (the drain reads `$hook.session_id`). The MCP namespace prefixes its own as `local_`. Two id spaces for one session, compared literally. MEASURED, AND I WAS THE ONE WHO CAUSED IT. Six of my own messages to the dispatcher stranded for a whole session -- a level report, a CI mechanism diagnosis, two unprompted self-retractions and a request to pull two never-started items. The recipient read my lane as silent and wrote "level unreported" three times. They were found only by opening the box by hand, and were due to expire with neither end told. PARTIAL CONTROL, AND THE ITEM SAYS SO RATHER THAN LEAVING IT TO BE DISCOVERED. This catches a wrong-NAMESPACE id. It does NOT catch a correctly-shaped but STALE one -- an id belonging to a session that has ended fails identically and just as silently. A pass at send is not a promise of delivery. THE MUST-NOT-TRIP ARM IS IN THE SAME TEST AS THE REFUSAL, deliberately: a guard that rejected everything would otherwise pass by satisfying one half. No `-ToSessionId` at all is the ordinary broadcast and still sends; a genuine bare-UUID id still sends. RED-FIRST IN BOTH DIRECTIONS, each broken then restored -- disabling the guard reddens the refusal test, widening it to reject everything reddens the must-not-trip test. The refusal names the REMEDY, not just the rejection: the sender's next move is to drop the flag and address by worktree path, and a message that only said "invalid" would leave them hunting an id. VERIFIED: ruff clean; 103 tests across the two mail suites; red-first both ways as above. The #1302 banner was flipped by lifting the closed-alphabet character from an already-closed item and asserting its membership before use -- never typed (sec. 11). Co-Authored-By: Claude Opus 5 --- docs/BACKLOG.md | 13 +++++++++- scripts/coord/mail.ps1 | 34 +++++++++++++++++++++++++ tests/test_session_mail.py | 51 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 51ef5698b..2dbf203f4 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -13726,7 +13726,18 @@ point, which are the parts that must survive it.** ## 1302. mail.ps1 accepts an MCP-namespace session id in -ToSessionId and the message becomes silently undeliverable, expiring with neither sender nor recipient told -> 🔢 **Re-scored 2026-08-20 -> P2.** Value **5/10** · Difficulty **2/10** · _fill-in_. The validation gap stands -- mail.ps1:106 takes -ToSessionId unvalidated, :240 stores it, and mail-drain.ps1:852 string-compares it against the harness id read at :490, so a wrong-namespace id is filtered on every pass -- but the item's reporting claim does not: mail-drain.ps1:825-836 writes an 'expired-unshown' receipt on the sweep, mail.ps1:484-487 already tells the sender to read exactly that file and what the disposition means, and mail-drain.ps1:1014 reports the filtered count to the recipient on every drain. Value 5 rather than the filed 7 or the scorer's 6 because the item's value argument was built on those instruments being absent and all three ship, leaving a detection DELAY and an ambiguous no-receipt reading rather than a silent loss, on developer coordination with no product or PHI axis. Difficulty 2 and arguably generous: mail.ps1:153 already dot-sources mail-claim.ps1, whose Test-SessionId at :209 is the exact UUID shape check the fix needs, so the remainder is a post-binding refusal plus a must-not-trip arm for the no-sessionId broadcast case. _(was 7/10 · 2/10.)_ +> ✅ **SHIPPED 2026-08-21 -- `mail.ps1` now REFUSES a `-ToSessionId` that is not a harness session id, at SEND, before any message is written. The sender is the only party who can correct the id, and was the only party never told.** The drain already reported its half (*"N message(s) are addressed to a different session id and were left in the inbox"*); the send path printed `Queued 1 message(s)` and nothing else. **That asymmetry was the defect, and it is why the guard sits on the send side rather than in the drain -- the drain's filter is CORRECT: a worktree outlives its occupant, so an id-addressed note must not reach a stranger.** + +> **THE SHAPE.** A harness session id is a bare UUID (the drain reads `$hook.session_id`); the MCP namespace prefixes its own as `local_`. Two id spaces for one session, compared with `-ne` at [`mail-drain.ps1:852`](../scripts/hooks/mail-drain.ps1), so a wrong-namespace id never matches and the message waits in the inbox until it is swept to `expired/` -- silently in both directions. + +> **MEASURED, NOT HYPOTHETICAL.** Six messages from one seat -- a level report, a CI mechanism diagnosis, two unprompted self-retractions and a request to pull two never-started items -- stranded for a whole session while the recipient read that lane as silent and wrote "level unreported" three times. They were found only by opening the box by hand. + +> **PARTIAL CONTROL, RECORDED RATHER THAN DISCOVERED LATER.** This catches a wrong-NAMESPACE id. It does **not** catch a correctly-shaped but **STALE** one -- an id belonging to a session that has ended fails identically and just as silently. A pass at send is not a promise of delivery, and nothing here should be read as one. + +> **THE MUST-NOT-TRIP ARM IS ASSERTED IN THE SAME TEST AS THE REFUSAL**, so a guard that rejected everything could not pass by satisfying one half: a message with NO `-ToSessionId` is the ordinary broadcast and still sends, and a genuine bare-UUID id still sends. **RED-FIRST IN BOTH DIRECTIONS:** disabling the guard reddens the refusal test, and widening it to reject everything reddens the must-not-trip test. + +> **Re-scored 2026-08-20 -> P2.** Value **5/10** · Difficulty **2/10** · _fill-in_. The validation gap stands -- mail.ps1:106 takes -ToSessionId unvalidated, :240 stores it, and mail-drain.ps1:852 string-compares it against the harness id read at :490, so a wrong-namespace id is filtered on every pass -- but the item's reporting claim does not: mail-drain.ps1:825-836 writes an 'expired-unshown' receipt on the sweep, mail.ps1:484-487 already tells the sender to read exactly that file and what the disposition means, and mail-drain.ps1:1014 reports the filtered count to the recipient on every drain. Value 5 rather than the filed 7 or the scorer's 6 because the item's value argument was built on those instruments being absent and all three ship, leaving a detection DELAY and an ambiguous no-receipt reading rather than a silent loss, on developer coordination with no product or PHI axis. Difficulty 2 and arguably generous: mail.ps1:153 already dot-sources mail-claim.ps1, whose Test-SessionId at :209 is the exact UUID shape check the fix needs, so the remainder is a post-binding refusal plus a must-not-trip arm for the no-sessionId broadcast case. _(was 7/10 · 2/10.)_ + > > **Filed 2026-08-21 -- not started. The one place in this transport where a message is genuinely LOST rather than late, and both ends read it as delivered.** `scripts/coord/mail.ps1:106` declares `-ToSessionId` as a bare `[string]` with **no validation**, and `:240` writes it into the message as `sessionId`. The drain then compares that value against the **harness** session id -- `scripts/hooks/mail-drain.ps1:852`, against `$sessionId` sourced at `:490` from `$hook.session_id`. **Three id namespaces exist for one session -- registry, MCP and harness -- and only one of them can ever match.** > **THE MECHANISM, verified in code rather than inferred from the symptom.** An MCP id is `local_`-prefixed; a harness id is a bare UUID. The comparison at `:852` is a string inequality, so a `local_` id can never equal the value it is tested against, on any drain, ever. The message is skipped every pass, stays in `inbox/`, and expires. **Nothing errors at send time, nothing errors at drain time, and nothing reports the expiry to either party.** `docs/WORKTREES.md` already records that a registry id and an MCP id for one session **shared no characters** -- so the namespaces are known to be disjoint, and nothing acts on that knowledge at the point where it matters. diff --git a/scripts/coord/mail.ps1 b/scripts/coord/mail.ps1 index 8a1bccd7d..0d48ee555 100644 --- a/scripts/coord/mail.ps1 +++ b/scripts/coord/mail.ps1 @@ -440,6 +440,40 @@ if ($Send) { } } + # BACKLOG #1302 -- FAIL THE SENDER, WHO CAN FIX IT, RATHER THAN THE RECIPIENT, WHO CANNOT. + # + # A `-ToSessionId` from the wrong namespace is compared literally against the reading session's + # harness id (`mail-drain.ps1`: `[string]$m.to.sessionId -ne $sessionId`), never matches, and the + # message sits in the inbox until it is swept to expired/. MEASURED: six messages from one seat -- + # a level report, a CI mechanism diagnosis, two unprompted self-retractions and a request to pull + # two items -- stranded for a whole session while the recipient read that lane as silent. The send + # path printed `Queued 1 message(s)` for every one of them. + # + # THE ASYMMETRY IS THE DEFECT, and it is why this check goes HERE. The drain ALREADY reports its + # side ("N message(s) are addressed to a different session id and were left in the inbox"), so the + # recipient is told. The SENDER is told nothing, and the sender is the only party who can correct + # the id. + # + # THE SHAPE: a harness session id is a bare UUID (the drain reads `$hook.session_id`). The MCP + # namespace prefixes its own (`local_`), and that is exactly the shape that stranded them -- + # two id spaces for one session, compared with `-ne`. + # + # PARTIAL CONTROL, AND RECORDING THAT IS PART OF THE FIX. This catches a wrong-NAMESPACE id. It + # does NOT catch a correctly-shaped but STALE one -- an id belonging to a session that has since + # ended fails identically and just as silently. A pass here is not a promise of delivery. + # + # MUST NOT TRIP ON THE ORDINARY CASE: no `-ToSessionId` at all is the normal broadcast, and it has + # to keep delivering untouched. The guard is scoped to a value the caller actually supplied. + if ($ToSessionId -and $ToSessionId -notmatch '^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$') { + throw ( + "-ToSessionId '$ToSessionId' is not a harness session id, so the drain would compare it " + + "against the reading session's id, never match, and leave the message in the inbox until " + + "it expired -- with neither end told. A harness session id is a bare UUID; an id carrying " + + "a namespace prefix such as 'local_' belongs to a different id space. Send with -To " + + " and omit -ToSessionId unless you have the harness id." + ) + } + # PER-TARGET, NOT ALL-OR-NOTHING. Now that a publish can genuinely fail -- the verify is real, so a # move that did not happen is reported instead of assumed -- a broadcast that aborted on target 1 # would hide targets 2..N, and one that swallowed the failure would put the defect back at the diff --git a/tests/test_session_mail.py b/tests/test_session_mail.py index ed0cdce66..6e302ccb1 100644 --- a/tests/test_session_mail.py +++ b/tests/test_session_mail.py @@ -437,6 +437,57 @@ def _send(repo: Path, body: str) -> subprocess.CompletedProcess[str]: ) # fmt: skip +def _send_addressed(repo: Path, session_id: str | None) -> subprocess.CompletedProcess[str]: + """Send with an optional ``-ToSessionId``. ``None`` omits the flag entirely (the ordinary case).""" + args = [ + "pwsh", "-NoProfile", "-NonInteractive", "-File", str(MAIL), + "-Send", "-MailRoot", str(mail_root(repo)), "-To", str(repo), "-Body", "probe", + ] # fmt: skip + if session_id is not None: + args += ["-ToSessionId", session_id] + return subprocess.run( + args, cwd=str(repo), capture_output=True, text=True, timeout=TIMEOUT, check=False + ) + + +def test_a_wrong_namespace_session_id_is_refused_at_SEND(repo: Path) -> None: + """BACKLOG #1302 -- the sender is the only party who can fix the id, so the sender is told. + + An id from the wrong namespace is compared literally against the reading session's harness id + (`mail-drain.ps1`), never matches, and the message sits in the inbox until it is swept to + `expired/`. MEASURED: six messages from one seat stranded for a whole session while the send path + printed `Queued 1 message(s)` for every one of them, and the recipient read that lane as silent. + + The drain already reports ITS side. This asserts the half that was missing. + """ + bad = _send_addressed(repo, "local_2b3b416c-1d0c-4e81-987c-bb19e590045d") + + assert bad.returncode != 0, f"a wrong-namespace id must be refused at send: {bad.stdout}" + # The refusal has to name the REMEDY, not merely the rejection -- the sender's next move is to drop + # the flag, and a message that only says "invalid" leaves them guessing at which id to hunt for. + assert "-ToSessionId" in bad.stderr + assert "omit -ToSessionId" in bad.stderr + + +def test_the_ordinary_broadcast_and_a_real_harness_id_both_still_send(repo: Path) -> None: + """THE MUST-NOT-TRIP ARM, and the reason the guard is scoped to a supplied value. + + Two ways this fix could have been worse than the defect. Refusing a message with NO + ``-ToSessionId`` would break the ORDINARY broadcast, which is most traffic on this channel. And + refusing a genuine harness id would make the flag unusable for the case it exists to serve -- mail + that is only meaningful to one session, where a worktree outliving its occupant would otherwise + hand a note to a stranger. + + Asserted in the SAME test as a pair, so a guard that accidentally rejected everything cannot pass + by satisfying one half. + """ + broadcast = _send_addressed(repo, None) + assert broadcast.returncode == 0, f"no -ToSessionId is the ordinary case: {broadcast.stderr}" + + real = _send_addressed(repo, "177a513c-60f4-49af-8cd4-465ff4f9118d") + assert real.returncode == 0, f"a bare-UUID harness id must send: {real.stderr}" + + def test_the_send_line_arm_refuses_at_the_boundary_and_passes_one_below(repo: Path) -> None: """The adjacent pair, not a 300-char probe. From 170f986fc4f5b010c672407d7d45d003295a8945 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 07:50:39 -0500 Subject: [PATCH 5/7] ci(security): raise the leak-gate detector floor to 8/14/2, in BOTH workflows (BACKLOG #321) The owner refreshed MEFOR_FORBIDDEN_TOKENS in both secret stores, so the floor can now assert the larger set. VERIFIED BY ME rather than taken from a relay -- names and dates only, no values, which is all `gh secret list` exposes: Actions MEFOR_FORBIDDEN_TOKENS 2026-08-21T12:45:15Z Dependabot MEFOR_FORBIDDEN_TOKENS 2026-08-21T12:45:23Z EIGHT SECONDS APART, so the half-done state this change was held for did not occur. If Actions had been updated and Dependabot had not, every Dependabot PR would hard-fail a required check. My stated constraint was "only after BOTH, and I will ask rather than infer" -- both are updated and the measurement is mine. RAISED IN TWO PLACES, NOT ONE. The release named `security.yml`. `branch-leak-scan.yml:88` carried the SAME literal and nobody named it. Raising only one would have left a second gate passing on the old floor -- a partial raise that reads as done. There are now zero occurrences of the old triple under .github/workflows/. PRE-FLIGHT BEFORE RAISING A FLOOR THAT HARD-FAILS A REQUIRED CHECK, counts only: names 8 estate 14 estate_file_scanned 13 site_prefixes 2 synthetic=False The real set satisfies 8/14/2 exactly, and `estate_file_scanned` at 13 matches the documented 12->13 move -- so the added token is FILE-SCANNED rather than body-only, which is the half that matters. NOT INERT, AND THAT IS CHECKED RATHER THAN ASSUMED. `token_floor_failure` passes at 8/14/2 and FAILS at 9/15/3 naming each short section ("names 8<9, estate 14<15, site_prefixes 2<3"). A floor that cannot fail is not a floor. EXPECT COLLATERAL HITS ON THE FIRST FULL SWEEP AND DO NOT READ THEM AS FINDINGS. The added site prefix is two digits, so it matches any delimited six-digit run in a 10000-wide band -- synthetic MRNs, sentinel ids, clamp ceilings. Triage noise, anticipated before the value was written. VERIFIED: 146 tests across the scanner + token-source + CI-pinning suites; 120 more across the workflow-lint and lockstep suites. No token value appears in this change, in any test, or in this message. Co-Authored-By: Claude Opus 5 --- .github/workflows/branch-leak-scan.yml | 2 +- .github/workflows/security.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/branch-leak-scan.yml b/.github/workflows/branch-leak-scan.yml index 68228981c..ba94449b8 100644 --- a/.github/workflows/branch-leak-scan.yml +++ b/.github/workflows/branch-leak-scan.yml @@ -85,7 +85,7 @@ jobs: run: | if [ -n "$MEFOR_FORBIDDEN_TOKENS" ]; then export MEFOR_REQUIRE_TOKENS=1 - export MEFOR_MIN_DETECTORS=names=7,estate=13,site_prefixes=1 + export MEFOR_MIN_DETECTORS=names=8,estate=14,site_prefixes=2 echo "token list loaded from the MEFOR_FORBIDDEN_TOKENS secret (fail-closed, per-section floor)." else # A push event cannot come from a fork, so unlike security.yml there is no legitimate diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 3dfab8390..826450da7 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -671,7 +671,7 @@ jobs: # the env var first, so that file would never be read -- it would only drop the full real # token list into the job workspace for every later step to see. export MEFOR_REQUIRE_TOKENS=1 - export MEFOR_MIN_DETECTORS=names=7,estate=13,site_prefixes=1 + export MEFOR_MIN_DETECTORS=names=8,estate=14,site_prefixes=2 echo "token list loaded from the MEFOR_FORBIDDEN_TOKENS secret (fail-closed, per-section floor)." elif [ "$IS_FORK_PR" = "true" ]; then echo "fork PR -- the secret is unavailable BY DESIGN; structural-only scan." From 468a66184ffbc719809ed47f3c2dd8156e356912 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 26 Aug 2026 12:25:14 -0500 Subject: [PATCH 6/7] fix(test): classify test_scan_forbidden_loaded_set.py as engine-subject without importing test_every_non_engine_test_is_classified caught this on CI (all 3 platforms, identical failure): PR 615's new test file imports scan_forbidden directly (a scripts/security/ module), not messagefoundry/harness/tee, so the partition guard correctly flagged it as unclassified rather than silently letting it run on every engine leg forever or drop off them entirely. Added to _STAYS_WITHOUT_IMPORTING alongside its sibling test_scan_forbidden.py, for the identical reason already documented there: it is a scanner test whose SUBJECT is engine-adjacent security tooling, read off disk rather than imported. Verified: tests/test_tooling_partition.py now 9/9 passing locally. Co-Authored-By: Claude Opus 5 --- tests/test_tooling_partition.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_tooling_partition.py b/tests/test_tooling_partition.py index cc5bb01bf..66101d49f 100644 --- a/tests/test_tooling_partition.py +++ b/tests/test_tooling_partition.py @@ -98,6 +98,7 @@ "test_release_pipeline.py", "test_sandbox_worker_logging.py", "test_scan_forbidden.py", + "test_scan_forbidden_loaded_set.py", "test_scan_tokens_source.py", "test_seam_discovery.py", "test_security_static.py", From 140dcb3bc648f08d63b6f0e9f64f027688faf0bf Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 28 Aug 2026 13:50:44 -0500 Subject: [PATCH 7/7] fix(test): classify test_scan_forbidden_loaded_set as TOOLING, not engine-subject The branch put tests/test_scan_forbidden_loaded_set.py in _STAYS_WITHOUT_IMPORTING. That list's own stated rule is narrower than the entry: it names tests whose SUBJECT is engine source, "they read messagefoundry/** off disk and assert something about it". This file does not. It carries zero messagefoundry references, its subject is scripts/security/scan_forbidden.py, and it self-marks pytestmark = pytest.mark.tooling -- which no other one of the 26 stay-list entries does. The consequence is not cosmetic and it is not visible on this PR. ci.yml's tooling path gate greps the changed set against tooling_manifest.txt itself, so the classification decides whether the tooling job runs at all. Simulated against both real manifests with the gate's own expression, changed set = this one file: the stay-list form gives tooling=false and the manifest form gives tooling=true. Combined with the file's own tooling mark, which the engine legs deselect via -m 'not tooling', the stay-list form would run this test in ZERO jobs on any future PR that touches only it. That is the regression shape ci.yml's own comment in that block names. Not reachable on this PR, because this PR also touches .github/ and scripts/, so tooling=true here regardless and both repo-harness legs are green. The hole is prospective. The sibling entry is correct and stays: test_scan_forbidden.py does reference messagefoundry/ paths, so its subject genuinely is engine source. Both arms proven locally before this commit. With the fix, the gate is 9 passed. Planting the name in BOTH lists reds test_the_two_lists_are_disjoint alone, and its message states the mechanism: "the manifest would win silently and the file would leave the engine legs". Restored, 9 passed again. Credit: the correct classification is builder-2's, carried on PR 655. This adopts it so main does not gain the wrong one, and so 655's rebase does not have to edit a line that just landed. --- tests/test_tooling_partition.py | 1 - tests/tooling_manifest.txt | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_tooling_partition.py b/tests/test_tooling_partition.py index 66101d49f..cc5bb01bf 100644 --- a/tests/test_tooling_partition.py +++ b/tests/test_tooling_partition.py @@ -98,7 +98,6 @@ "test_release_pipeline.py", "test_sandbox_worker_logging.py", "test_scan_forbidden.py", - "test_scan_forbidden_loaded_set.py", "test_scan_tokens_source.py", "test_seam_discovery.py", "test_security_static.py", diff --git a/tests/tooling_manifest.txt b/tests/tooling_manifest.txt index 433d8cd67..9ebb5fd2e 100644 --- a/tests/tooling_manifest.txt +++ b/tests/tooling_manifest.txt @@ -103,6 +103,7 @@ tests/test_quality_advisory_invariants.py tests/test_quality_record_scope_claims.py tests/test_required_contexts.py tests/test_required_workflow_state.py +tests/test_scan_forbidden_loaded_set.py tests/test_sbom_finalize.py tests/test_script_root_anchoring.py tests/test_security_posture.py