diff --git a/.github/workflows/branch-leak-scan.yml b/.github/workflows/branch-leak-scan.yml index 68228981..ba94449b 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 47865d71..c8485c60 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -677,7 +677,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." diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index a0fd6715..e807cf97 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -13914,7 +13914,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. @@ -16453,6 +16464,8 @@ declared serialised write order on that file. Building this alongside those with first risks the exact same-file collision this project's own collision-detection conventions exist to catch. +--- + ## 1357. The connscale FD probe cannot see a multi-process engine: PID-set re-resolution never runs > 🔢 **Filed 2026-08-25 - not started.** ***A single probe tick costs LONGER THAN THE ENTIRE MEASUREMENT WINDOW, so the PID set is resolved once per sweep step and never re-resolved -- and a multi-process engine's worker children are invisible to the FD gauge.*** **Measured: one tick costs 1419-1848 ms against `hold_seconds = 1.5`.** So `_RESOLVE_EVERY_TICKS = 8` ([`probe.py:60`](../harness/load/connscale/probe.py)) is **unreachable by construction** -- not a badly-chosen constant, a constant the window cannot afford at any cadence. Its own comment reasons at "the runner's poll cadence", which this profile does not have. @@ -16490,6 +16503,42 @@ 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. + +--- + +--- + ## 1363. the retirement flag is preempted by the key-set guard, so a full-list retirement cannot be expressed > 🔢 **Filed 2026-08-26 (lander) - not started. Successor to [#1307](#1307-the-safe-asvs-writer-has-no-path-to-retire-an-anchor), whose banner is amended rather than reopened.** Value **5/10** · Difficulty **3/10** · _fill-in_. #1307 shipped `--allow-retirement` and the flag genuinely works for a PARTIAL retirement. It is unreachable for a FULL-LIST one, and that is the shape both authorised retirements take -- so the sanctioned outcome #1307 exists to make expressible still cannot be expressed for the case that prompted it. diff --git a/harness/load/multishard.py b/harness/load/multishard.py index 71ae0fed..59d60035 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/scripts/coord/mail.ps1 b/scripts/coord/mail.ps1 index 8a1bccd7..0d48ee55 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/_dead_pid.py b/tests/_dead_pid.py new file mode 100644 index 00000000..a19e43e9 --- /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 83a1f08f..c3c87e74 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_multishard_smoke.py b/tests/test_multishard_smoke.py index ad81b082..3ba44893 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 diff --git a/tests/test_scan_forbidden_loaded_set.py b/tests/test_scan_forbidden_loaded_set.py new file mode 100644 index 00000000..eb0ed79b --- /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" + ) diff --git a/tests/test_session_mail.py b/tests/test_session_mail.py index ed0cdce6..6e302ccb 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. diff --git a/tests/test_session_registry.py b/tests/test_session_registry.py index e21587e9..f962c3f6 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 1ceb63a2..75637128 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) # diff --git a/tests/tooling_manifest.txt b/tests/tooling_manifest.txt index 433d8cd6..9ebb5fd2 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