From 94ad263a94738a3fd5eabcf5f831eecd5c71f7fc Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 21:51:30 -0500 Subject: [PATCH 1/9] docs(security): the ingest-plane rate limit cannot be enabled -- say so (BACKLOG #1249) The control table listed `max_messages_per_second` and `message_burst` with state "off", which every reader takes to mean "set it to on", and the cell went on to describe what happens "when set". No documented configuration can set them. MEASURED, with a positive control that proves the check discriminates: MLLP() parameters: 26, **kwargs present: False max_messages_per_second NOT A PARAMETER message_burst NOT A PARAMETER max_connections ACCEPTED <- positive control mllp.py:1378 mps = s.get("max_messages_per_second", DEFAULT_...) pacer reads it mllp.py:116 DEFAULT_MAX_MESSAGES_PER_SECOND = None ships off `connections.toml` routes through the SAME factory, so neither authoring surface can express the keys. The pacer is real, reads its settings, and is unreachable. A SECURITY DOCUMENT DESCRIBING A CONTROL, ITS BEHAVIOUR WHEN ENABLED, AND ITS COVERAGE LIMITS -- FOR SOMETHING NOTHING CAN ENABLE -- is a compensating control resting on a false premise, which SDS-3.7 forbids by name. TWO THINGS DELIBERATELY KEPT, because the correction must not overshoot: Not "unbounded intake". That would be FALSE and it runs against the engine. Several bounds ship ON -- max_connections 256, receive_timeout 60s, max_frame_bytes 16 MiB, per-connection max_message_bytes, source_ip_allowlist. They bound SIZE and CONCURRENCY, not RATE. The cell now says which is which. The off default was RULED, not accidental -- a rate on a clinical interface is only safe at a number from a real feed profile. That reasoning stands. What the old text conflated is that a ruled default and an unreachable setting are different things, and only the first was intended. NOT FIXED HERE, and not mine to decide: adding the two keys to the MLLP() signature would change a shipped public factory, and whether the posture should be reachable at all is a product question -- the item notes the default is ruled off for a reason. Handed to the dispatcher; the doc half does not wait on it. Verified: table structure intact (10 cells in the edited row, same as its neighbour; 360 pipe-rows unchanged); link resolution and banner hygiene 30 passed; cp1252 scan over the ADDED line -- 7 non-ASCII seen, all safe punctuation, 0 unsafe. Coordinated: the collision gate refused this edit while another session held the file with an unresolved merge conflict. I held rather than overriding, and they released it. Their commits touch :56-77, :1164 and :1834 -- nowhere near this row. No ledger edit; banner flip withheld, disposition routes to the dispatcher. --- docs/SECURITY.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index b9e43ee3..96091f32 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -1674,7 +1674,7 @@ multi-host deployment must additionally front the API with a proxy/WAF limiter a | Request body | `[store].max_upload_bytes` (the `/uploads` routes only) | 1 MiB elsewhere | per request | no | no | no | **stateless** — every route, in ASGI middleware | **413** over the cap, **400** on ambiguous CL+TE framing or an invalid `Content-Length`, **411** on a chunked body | | OIDC pending flows | `[auth].oidc_flow_cache_max` (global), `DEFAULT_PER_IP_CAP` (per-IP, no knob), `oidc_flow_ttl_seconds` | 512 / 16 / 300 s | 300 s TTL | no | **yes** (512) | **yes** (16) | **in-process** — `GET /ui/oidc/start` — reject-when-full, never evict | 303 → `/ui/login?e=rate_limited`, WARNING-logged, **never** audited | | WebAuthn pending ceremonies | `GLOBAL_PENDING_CAP`, `PER_USER_PENDING_CAP`, `CHALLENGE_TTL_SECONDS` (module constants, no knobs) | 4096 / 16 / 120 s | 120 s TTL | **yes** (16) | **yes** (4096) | no | **in-process** — every passkey registration + assertion ceremony | per-user: evicts that user's **own** oldest pending ceremony (silent); global: `ChallengeCacheFullError` naming the cause + the `admin_reset_mfa` recovery path | -| **Ingest plane** | `max_messages_per_second`, `message_burst` (MLLP inbound) | **off** | per message | no | no | no | **in-process** — one bucket per MLLP connection, so it neither coordinates across engine shards nor aggregates per peer | **exists but ships OFF, so unset there is still no volume bound.** When set, the listener **pauses reading** over budget so TCP back-pressures the sender: no message is dropped, refused, NAK'd or reordered (the count-and-log invariant forbids accept-and-drop, so a discarding limiter was never available). Bounded by the bucket deficit. The off default is **ruled, not accidental** — a rate on a clinical interface is only safe at a number from a real feed profile. **Not covered:** the raw-TCP inbound, and any per-peer bound (MLLP peers are unauthenticated, so the only key would be source IP, which NAT collapses). Other inbound caps are resource-only — `max_connections` (256), `receive_timeout` (60.0 s), `max_frame_bytes` (16 MiB), per-connection `max_message_bytes`, `source_ip_allowlist` | +| **Ingest plane** | `max_messages_per_second`, `message_burst` (MLLP inbound) | **NOT REACHABLE — read the note** | per message | no | no | no | **in-process** — one bucket per MLLP connection, so it neither coordinates across engine shards nor aggregates per peer | **THE PACER IS BUILT AND NO DOCUMENTED CONFIGURATION CAN TURN IT ON.** Read that before the rest of this cell. Neither key is a parameter of the `MLLP()` factory, and `connections.toml` routes through that same factory, so **neither the code-first nor the TOML surface can express them**. This row previously read state **"off"**, which every reader takes to mean "set it to on". **So there is currently NO message-RATE bound anywhere on the ingest plane.** — **This is NOT "unbounded intake", and that correction runs in the engine's favour:** several bounds ship **ON** and are listed at the end of this cell. They bound **SIZE and CONCURRENCY, not RATE**. — *What the pacer would do if it were reachable:* the listener **pauses reading** over budget so TCP back-pressures the sender; no message is dropped, refused, NAK'd or reordered (the count-and-log invariant forbids accept-and-drop, so a discarding limiter was never available). The off default was **ruled, not accidental** — a rate on a clinical interface is only safe at a number from a real feed profile — but **a ruled default and an unreachable setting are different things, and only the first was intended.** **Not covered even if reachable:** the raw-TCP inbound, and any per-peer bound (MLLP peers are unauthenticated, so the only key would be source IP, which NAT collapses). **Resource bounds that DO ship on** — `max_connections` (256), `receive_timeout` (60.0 s), `max_frame_bytes` (16 MiB), per-connection `max_message_bytes`, `source_ip_allowlist` | **What these limits defend, and what they do not.** The full inventory of resource-demanding functionality — including the surfaces that remain **unbounded** at this release — is From e92b7ecfb70cbec92be144c54cfa49fb23ac657f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 14 Aug 2026 00:06:08 -0500 Subject: [PATCH 2/9] test(security-doc): the ingest-plane guard tests the DOC against the CODE (BACKLOG #1249) 94ad263a rewrote the SECURITY.md ingest-plane row from "ships off" to "NOT REACHABLE" and did NOT update the guard that pins that wording, so the commit as handed over failed test_ingest_plane_rate_limit_is_documented_as_existing_but_off. Found by running the full suite; entirely mine. A docs-only diff is not test-neutral -- a test reads that file from disk, so a documentation commit moves a test result. The guard now reads reachability from inspect.signature(wiring.MLLP) and requires the row to AGREE with it, instead of asserting that unreachability is the correct state. #1249 IS OPEN AND THIS COMMIT DOES NOT CLOSE IT. The owner has not chosen between exposing max_messages_per_second / message_burst and rewording the row. A guard that pinned "NOT REACHABLE" as correct would settle that product question by build -- the same shape as settling one by omission. This guard flips on its own: expose the keys and it demands the row stop saying unreachable; leave them out and it demands the row say so. A green run here is not the ruling. Verified: MLLP() (config/wiring.py:1007) accepts neither pacing key, and connections.toml desugars through that same factory. Both branches are exercised -- simulating accepted keys fires the opposite assertion against the current doc. Unrelated: seven failures elsewhere in the full suite remain unlocated. They are not named here because I have not run them individually, and two positional estimates of their location were both wrong. --- tests/test_security_doc_rate_limits.py | 49 ++++++++++++++++++++------ 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/tests/test_security_doc_rate_limits.py b/tests/test_security_doc_rate_limits.py index 572f428b..08f4f486 100644 --- a/tests/test_security_doc_rate_limits.py +++ b/tests/test_security_doc_rate_limits.py @@ -532,23 +532,50 @@ def test_scope_guard_detects_a_planted_rescoping() -> None: ) -def test_ingest_plane_rate_limit_is_documented_as_existing_but_off() -> None: - """A silent omission reads as coverage, and so does an overstatement in the other direction. - - Until 2026-08-11 this guard asserted the doc said "no message-rate or volume limit exists", which - was true and load-bearing. The MLLP pacing build (ASVS 2.4.1 / 15.2.2) falsified it, so the guard - had to move with the code and the doc rather than be deleted -- the row must now state BOTH that - a control exists and that it ships OFF, because either half alone misleads: "exists" implies the - shipped default is bounded, and "none" is now simply false. +def test_ingest_plane_rate_limit_row_matches_the_code() -> None: + """The ingest row must describe the pacer's REACHABILITY as the code actually has it. + + This guard has moved twice. Until 2026-08-11 it asserted the doc said "no message-rate or volume + limit exists"; the MLLP pacing build (ASVS 2.4.1 / 15.2.2) falsified that, so it moved to + requiring both that a control exists and that it "ships OFF". BACKLOG #1249 falsified that + wording in turn: the pacer is built, but neither key is a parameter of the ``MLLP()`` factory and + ``connections.toml`` desugars through that SAME factory, so no documented surface can set either. + "Off" is the more dangerous half-truth, because a reader takes it to mean "then set it to on". + + WHAT THIS ASSERTS IS CONSISTENCY, NOT A PREFERENCE, AND THE DISTINCTION IS THE WHOLE POINT. + **#1249 is OPEN.** The owner has not chosen between exposing the two keys and rewording the row, + and a guard that pinned "NOT REACHABLE" as the correct state would settle that product question + BY BUILD -- the same shape as settling one by omission. So reachability is READ FROM THE + SIGNATURE and the doc is required to agree with whatever it says. Expose the keys and this test + demands the row stop saying unreachable; leave them out and it demands the row say so. Either + ruling stays cheap, and neither is pre-empted by a green run here. """ + import inspect + + from messagefoundry.config import wiring + + pacing_keys = {"max_messages_per_second", "message_burst"} + accepted = pacing_keys & set(inspect.signature(wiring.MLLP).parameters) + block = _section(_H_LIMITS) assert "Ingest plane" in block assert "max_messages_per_second" in block, ( "the ingest row must name the control that now exists" ) - assert "ships OFF" in block, "and must say it is not on by default, or 'exists' overstates it" - # The honest gaps stay named: an off default bounds nothing, and the raw-TCP intake never got it. - assert "still no volume bound" in block + if accepted: + assert "NOT REACHABLE" not in block, ( + f"MLLP() now accepts {sorted(accepted)}, so the row may no longer call the pacer " + "unreachable -- it must state the shipped default instead" + ) + else: + assert "NOT REACHABLE" in block, ( + "MLLP() accepts neither pacing key, so the row must say the pacer cannot be turned on " + "rather than that it is merely off by default" + ) + assert "NO message-RATE bound anywhere on the ingest plane" in block, ( + "and must name the resulting gap outright, not leave it inferred" + ) + # True under either ruling: the raw-TCP intake never got the pacer at all. assert "raw-TCP inbound" in block for cap in ("max_connections", "receive_timeout", "max_frame_bytes", "max_message_bytes"): assert cap in block, f"the ingest row must name the {cap} resource cap it DOES have" From bbd2d86cf70780ab37cc8f30c17b1a26332523ee Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 14 Aug 2026 00:06:36 -0500 Subject: [PATCH 3/9] test(collection): an incomplete local run now STATES what it could not collect (BACKLOG #1230) The LOUD-OMISSION half only, per the owner's 2026-08-12 scope ruling. The worktree venv is deliberately NOT changed here. scripts/worktree/new.ps1:232 builds a worktree venv with .[dev,harness]; CI installs .[dev,harness,fhir,dicom,x12,xml,webauthn] (.github/workflows/ci.yml:273). Every module gated on that five-extra gap removes ITSELF at collection time via a module-level pytest.importorskip, so those tests never become test items: a block of coverage collapses into a short skip tally and the run still prints as green. THE DEFECT IS THE SILENCE, NOT THE ABSENCE -- skipping an uninstalled optional extra is correct; rendering an incomplete run as a complete one is not. A report-header line and a terminal-summary block now name the absent extras and the exact install command. NO PINNED TEST COUNT: a hard-coded figure is right the day it is written and silently wrong after, and re-running reproduces it, so it reads as verified. Proved in BOTH directions. The silent arm (nothing missing means nothing printed) is the one this venv cannot show on its own, so it is staged with monkeypatch rather than assumed. THE PROBE LIVES IN ITS OWN MODULE, AND THAT IS THE INTERESTING PART. It began inside conftest.py and the tests reached it with a bare `import conftest`. That passes when only tests/ is collected and FAILS IN A FULL RUN: pyproject has two testpaths, and packaging/messagefoundry-webconsole/ tests/conftest.py claims the same top-level module name, so the import bound to the webconsole's conftest and every attribute lookup failed. Seven tests passed alone and failed together -- the worst way for a test to be wrong, because isolation says it is fine. Fixed by moving the probe to tests/_extras_probe.py (no module-level side effects) and importing it package-qualified, the same idiom as tests._workflow_contexts. Importing conftest BY PATH was rejected: its module body claims a per-process test slot and registers an atexit unlink, so a second execution burns a slot for nothing. Verified against the failure mode, not just in isolation: with both testpaths collected together the suite is green, and restoring the bare import reproduces exactly those seven failures. The negative control is what makes the green meaningful. Known limit, stated rather than implied: these hooks load for any run that collects tests/, which includes every full-suite run -- the only kind that can earn "the full suite is green". A run scoped entirely to the webconsole testpath prints no banner. Measured both ways. Left rather than fixed with a repo-root conftest.py, which would change collection globally and is outside this scope. --- tests/_extras_probe.py | 91 +++++++++++++++++++++++++++++ tests/conftest.py | 42 +++++++++++++ tests/test_incomplete_run_banner.py | 88 ++++++++++++++++++++++++++++ 3 files changed, 221 insertions(+) create mode 100644 tests/_extras_probe.py create mode 100644 tests/test_incomplete_run_banner.py diff --git a/tests/_extras_probe.py b/tests/_extras_probe.py new file mode 100644 index 00000000..7bffd20a --- /dev/null +++ b/tests/_extras_probe.py @@ -0,0 +1,91 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Which optional extras are absent from this interpreter (BACKLOG #1230, loud-omission half). + +WHY THIS IS A SIBLING MODULE AND NOT PART OF ``conftest.py``, which is where it started and where it +does not belong. The tests reached it with a bare ``import conftest``. That passes when only +``tests/`` is collected and FAILS IN A FULL RUN, because ``pyproject.toml`` has two testpaths and +``packaging/messagefoundry-webconsole/tests/conftest.py`` claims the same top-level module name: + + AttributeError: + has no attribute '_OPTIONAL_EXTRAS' + +Seven tests passed alone and failed together, which is the worst way for a test to be wrong -- the +name resolved to the neighbouring module and nothing said so. Importing ``conftest`` BY PATH instead +would re-execute it, and its module body claims a per-process test slot and registers an ``atexit`` +unlink, so a second execution burns a slot for nothing. + +So the probe lives here: no module-level side effects, imported package-qualified as +``tests._extras_probe`` (the same idiom as ``tests._workflow_contexts``), and ``conftest`` keeps only +the thin pytest hooks that render what these functions compute. +""" + +from __future__ import annotations + +import importlib.util +from typing import Protocol + +#: extra name -> the import sentinels its tests need. Mirrors pyproject.toml's +#: [project.optional-dependencies]; every distribution an extra pulls in is listed, so a +#: half-installed extra reads as absent rather than present. +OPTIONAL_EXTRAS: dict[str, tuple[str, ...]] = { + "fhir": ("fhir.resources", "fhirpathpy"), + "dicom": ("pydicom", "pynetdicom"), + "x12": ("pyx12",), + "xml": ("lxml", "xmlschema", "signxml"), + "webauthn": ("webauthn",), +} + + +class SummaryWriter(Protocol): + """The two ``TerminalReporter`` methods the summary uses, and nothing else.""" + + def write_line(self, line: str) -> None: ... + + def write_sep(self, sep: str, title: str = "", **kwargs: object) -> None: ... + + +def extra_is_installed(sentinels: tuple[str, ...]) -> bool: + """True only when EVERY sentinel resolves. + + ``find_spec`` rather than an import: it answers the question without paying the import cost and + without leaving a partially-imported module behind on failure. A sentinel whose PARENT package is + absent (``fhir.resources`` with no ``fhir``) RAISES instead of returning ``None`` -- measured, not + assumed -- so both arms have to mean "absent" or the fhir extra would read as present. + """ + for name in sentinels: + try: + if importlib.util.find_spec(name) is None: + return False + except (ImportError, ValueError): + return False + return True + + +def missing_extras() -> list[str]: + """Extras whose tests cannot be collected in this interpreter, in declaration order.""" + return [name for name, probes in OPTIONAL_EXTRAS.items() if not extra_is_installed(probes)] + + +def report_header_lines() -> list[str]: + """State the omission up front -- a summary line is easy to scroll past on a long run.""" + missing = missing_extras() + if not missing: + return [] + return [f"INCOMPLETE RUN: optional extras absent -- {', '.join(missing)}"] + + +def write_incomplete_run_summary(reporter: SummaryWriter) -> None: + """Print the omission where the verdict is rendered, so green cannot be read as complete.""" + missing = missing_extras() + if not missing: + return + write = reporter.write_line + reporter.write_sep("=", "INCOMPLETE RUN -- coverage was NOT collected", red=True, bold=True) + write(f"Optional extras absent from this interpreter: {', '.join(missing)}") + write("Every test module gated on them removed itself at COLLECTION time, so its tests are not") + write("in the counts above -- passed, failed and skipped alike. This result does NOT establish") + write("that the full suite is green, and must not be reported as if it did.") + write("") + write("To collect them, install what CI installs (.github/workflows/ci.yml):") + write(f' pip install --constraint constraints.lock -e ".[dev,harness,{",".join(missing)}]"') diff --git a/tests/conftest.py b/tests/conftest.py index 6bb7a79e..f5a27c1a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -25,6 +25,7 @@ import pytest from messagefoundry.config.settings import INSECURE_CONFIG_SOURCE_ESCAPE_ENV +from tests._extras_probe import report_header_lines, write_incomplete_run_summary # --------------------------------------------------------------------------------------------------- # Per-PROCESS test slot. @@ -327,3 +328,44 @@ def _tolerate_logging_on_closed_capture_streams() -> Iterator[None]: yield finally: logging.raiseExceptions = prior_raise + + +# --------------------------------------------------------------------------------------------------- +# LOUD OMISSION: an incomplete run must SAY SO (BACKLOG #1230 — the loud-omission half only, per the +# owner's 2026-08-12 scope ruling; the venv is deliberately NOT changed here). +# +# scripts/worktree/new.ps1:232 creates a worktree venv with `.[dev,harness]`. CI installs +# `.[dev,harness,fhir,dicom,x12,xml,webauthn]` (.github/workflows/ci.yml:273). Every module gated on +# that five-extra gap removes ITSELF at collection time via a module-level `pytest.importorskip`, so +# those tests never become test items at all: a large block of coverage collapses into a short skip +# tally and the run still prints as green. +# +# THE DEFECT IS THE SILENCE, NOT THE ABSENCE. Skipping an uninstalled optional extra is correct and +# intended. Rendering an incomplete run as a complete one is not — "the full suite is green" is the +# sentence the next session bases its own scope on, and a local run cannot currently earn it. +# +# Deliberately NOT a pinned test count. A hard-coded figure would be right the day it was written and +# silently wrong after, and a stale number in a measurement surface is worse than none: re-running +# reproduces it, so it reads as verified. What prints is what is re-derived every run — which extras +# are absent, and the command that installs them. +# +# This never fails a run and never skips anything itself. It only makes an already-incomplete run +# legible, so the omission has to be read rather than inferred from a skip count. +# +# SCOPE, STATED RATHER THAN IMPLIED. These hooks live in tests/conftest.py, so they load for any run +# that collects tests/ — which includes every full-suite run, the only kind that can earn the phrase +# above. A run scoped ENTIRELY to the other testpath (packaging/messagefoundry-webconsole/tests) +# does not load this file and prints no banner. Measured both ways, with a control to rule out the +# hooks simply being inert: webconsole-only collected 365 tests silently; the same flags over a +# tests/ path printed the banner. That gap is left rather than fixed with a repo-root conftest.py, +# which would change collection globally and is outside this item's ruled scope — a deliberately +# scoped run already reads as partial; a full-suite run is the one that must not. +# --------------------------------------------------------------------------------------------------- + + +def pytest_report_header() -> list[str]: + return report_header_lines() + + +def pytest_terminal_summary(terminalreporter: pytest.TerminalReporter) -> None: + write_incomplete_run_summary(terminalreporter) diff --git a/tests/test_incomplete_run_banner.py b/tests/test_incomplete_run_banner.py new file mode 100644 index 00000000..9d1de192 --- /dev/null +++ b/tests/test_incomplete_run_banner.py @@ -0,0 +1,88 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The incomplete-run banner (BACKLOG #1230, loud-omission half). + +Proved in BOTH directions, because a banner that always prints and a banner that never prints are +both indistinguishable from a working one if you only ever observe the state you happen to be in. +This worktree's venv is missing all five extras, so the "fires" direction is the ambient case and +the SILENT direction is the one that needs staging. +""" + +from __future__ import annotations + +from typing import Any + +from tests import _extras_probe as probe + + +class _FakeReporter: + """Captures what the summary hook writes. Mirrors only the two methods the hook uses.""" + + def __init__(self) -> None: + self.lines: list[str] = [] + self.seps: list[str] = [] + + def write_line(self, line: str, **_: Any) -> None: + self.lines.append(line) + + def write_sep(self, _sep: str, title: str = "", **_kw: Any) -> None: + self.seps.append(title) + + +def test_a_resolvable_sentinel_reads_as_installed() -> None: + """Positive control: the probe must be capable of returning True at all, or every other + assertion here would pass against a predicate that can only ever say 'absent'.""" + assert probe.extra_is_installed(("json",)) is True + assert probe.extra_is_installed(("json", "pathlib")) is True + + +def test_an_absent_sentinel_reads_as_missing() -> None: + assert probe.extra_is_installed(("mefor_no_such_module_1230",)) is False + + +def test_one_absent_sentinel_condemns_the_whole_extra() -> None: + """A half-installed extra must read as absent -- its tests still cannot be collected.""" + assert probe.extra_is_installed(("json", "mefor_no_such_module_1230")) is False + + +def test_a_missing_parent_package_reads_as_missing_not_as_present() -> None: + """``find_spec`` RAISES rather than returning None when the parent package is absent. If that + arm were not caught, the fhir extra (``fhir.resources``) would report as installed.""" + assert probe.extra_is_installed(("mefor_no_such_parent_1230.child",)) is False + + +def test_silent_when_every_extra_resolves(monkeypatch: Any) -> None: + """The direction this venv cannot show on its own: nothing missing means nothing printed.""" + monkeypatch.setattr(probe, "OPTIONAL_EXTRAS", {"stdlib": ("json",)}) + assert probe.missing_extras() == [] + assert probe.report_header_lines() == [] + reporter = _FakeReporter() + probe.write_incomplete_run_summary(reporter) + assert reporter.lines == [] + assert reporter.seps == [] + + +def test_loud_when_an_extra_is_absent(monkeypatch: Any) -> None: + monkeypatch.setattr( + probe, "OPTIONAL_EXTRAS", {"stdlib": ("json",), "ghost": ("mefor_no_such_module_1230",)} + ) + assert probe.missing_extras() == ["ghost"] + header = probe.report_header_lines() + assert len(header) == 1 + assert "ghost" in header[0] + + reporter = _FakeReporter() + probe.write_incomplete_run_summary(reporter) + body = "\n".join(reporter.lines) + assert "ghost" in body + assert "stdlib" not in body # only what is actually absent is named + assert any("INCOMPLETE RUN" in title for title in reporter.seps) + # The remedy has to be present and has to name the absent extra, or the banner states a problem + # without a way out and gets ignored. + assert 'pip install --constraint constraints.lock -e ".[dev,harness,ghost]"' in body + + +def test_the_real_extra_table_matches_pyproject() -> None: + """The table is hand-maintained beside pyproject.toml; assert it still names the five extras CI + installs, so a renamed or dropped extra fails here instead of silently never being reported.""" + assert set(probe.OPTIONAL_EXTRAS) == {"fhir", "dicom", "x12", "xml", "webauthn"} From 8e7ee1505aee88dbe95616230f235df7d8cd16fd Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 14 Aug 2026 00:07:03 -0500 Subject: [PATCH 4/9] tooling(docs): detect a backlog citation that names no item at all (BACKLOG #1235) The RULE half. A citation to a number that names nothing resolves to nothing -- honest, and it advertises its own brokenness. If that number is later issued, the citation silently starts naming unrelated work, which the ledger's own erratum calls the worse outcome. THE DISTINCTION THAT DECIDES WHETHER A CITATION CAN EVER ARM, and it is not the obvious one. alloc.ps1 starts its search at $observed + 1 (config at :392, or the public-floor clamp at :389) and scans UPWARD, never filling a hole: "numbers are never reclaimed ... holes are free, collisions are not". So a number at or below the high-water mark is unreachable FOREVER; only a number above it can be issued. A check that asked only "does this resolve in the ledger" rates the two identically and raises 26 false alarms on this repository alone. Classification is by the FLOOR, deliberately not by the allocation registry. An earlier draft read the records under the git common dir. It gave the same answers and rested on the wrong thing: those records are machine-local, uncommittable and losable, so garbage-collecting a directory would appear to change a conclusion that never depended on it. The unreachability is structural. The floor is built from the ledgers only, so it can only UNDERSTATE the allocator's true floor (which also spans refs and allocations). Understating means over-warning, never a missed trap. TWO DEFECTS IN THE TOOL ITSELF, both found by RUNNING it over docs/ and both now pinned as tests. It crashed mid-scan on a character a cp1252 console cannot encode -- a scanner that dies partway prints a partial list that reads as a complete one. And it matched CSS/Mermaid hex colours: #1565c0 read as a citation of #1565, 8 of 40 hits spurious. An inflated count in a tool whose whole output is a bound is the one failure that makes it useless. Output is an UPPER BOUND ON CITATIONS, never a defect count, and says so in those words. Every hit prints with its file, line and source line so a human judges it; PR/issue/foreign-repo shapes are ANNOTATED, never trimmed, because a trimmed scan silently understates. Measured over docs/: 32 tokens, 26 below the floor, 6 above -- and all 6 are foreign references (pyodbc#1459, a Mirth forum #4849, code-server #6256). ZERO genuine live traps in this repository. NOT backlog_citation_check.py, which sits beside it and answers a different question (#1095: does the cited FILE contain the item, live ledger vs archive). Neither subsumes the other. The near- identical names are why that distinction is the first paragraph of the new module. The item's own premise needs amending -- its two named live traps are inert by construction -- but a builder may not author ledger content, so that is filed with the dispatcher, not written here. --- scripts/docs/dangling_citation_check.py | 257 ++++++++++++++++++++++++ tests/test_dangling_citation_check.py | 176 ++++++++++++++++ 2 files changed, 433 insertions(+) create mode 100644 scripts/docs/dangling_citation_check.py create mode 100644 tests/test_dangling_citation_check.py diff --git a/scripts/docs/dangling_citation_check.py b/scripts/docs/dangling_citation_check.py new file mode 100644 index 00000000..73e8d51d --- /dev/null +++ b/scripts/docs/dangling_citation_check.py @@ -0,0 +1,257 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Report a `#N` backlog citation that names NO ITEM AT ALL (BACKLOG #1235). + +NOT `backlog_citation_check.py`, WHICH SITS BESIDE THIS FILE AND ANSWERS A DIFFERENT QUESTION. +That gate (BACKLOG #1095) resolves a citation against the ledger FILE its item lives in, so a +reference naming the live `docs/BACKLOG.md` for an item that has been archived is caught -- the +number is real, the path is stale. This one asks whether the number names anything whatsoever. +An item that exists in either ledger is invisible here and is that gate's business; a number that +exists in neither is invisible there and is this one's. Neither subsumes the other, and the near- +identical names are the reason this paragraph is the first thing in the file. + +WHAT THE DEFECT IS. While a number names nothing, a citation to it resolves to NOTHING -- honest and +harmless, and it advertises its own brokenness. If that number is later issued, the citation starts +resolving to unrelated work and NOTHING anywhere reports a problem. The ledger's own erratum names +that as the worse outcome: a wrongly-resolving reference "reads as a working cross-reference +forever". This tool catches the citation while it is still in the honest state. + +THE DISTINCTION THAT DECIDES WHETHER A CITATION IS A TRAP, and it is not the obvious one. Two very +different states both look like "resolves to nothing", and only one of them can ever arm: + + * BELOW THE HIGH-WATER MARK. `scripts/coord/alloc.ps1` starts its search at ``$observed + 1`` and + scans UPWARD, never filling a hole -- "numbers are never reclaimed ... holes are free, + collisions are not". A number at or below the mark is therefore unreachable FOREVER, and the + citation is harmless permanently rather than by luck. Measured here: 26 of 32 hits. + * ABOVE IT. That number will be issued in the normal course, and on the day it is, the citation + silently starts naming unrelated work. This is the only shape that can arm. + +A tool reporting only "resolves in the ledger or not" rates those identically and raises 26 false +alarms on this repository alone. + +WHY THE FLOOR AND NOT THE ALLOCATION REGISTRY. An earlier version of this tool classified by looking +for an allocation RECORD under the git common dir. It produced the same answers and rested on the +wrong thing: those records are machine-local, uncommittable and losable, so garbage-collecting a +directory would appear to change a conclusion that never depended on it. The unreachability is a +structural property of how the allocator picks a number. Reasoning from the registry makes a sound +property look fragile and invites a guard nothing needs. + +WHY A TOOL AND NOT VIGILANCE. There is no gate on either side. `alloc.ps1` answers "is this number +free"; nothing asks "is anything already pointing at it". The two halves are each individually +correct, and the gap between them is the defect. + +WHAT THIS DELIBERATELY DOES NOT DO. + + * It does NOT claim a defect count. Its output is an UPPER BOUND ON CITATIONS -- some hits are + near-certainly not backlog references at all (a four-digit token in a research evaluation, one + inside a diagram). Reporting the raw number as a defect count would be a completeness claim the + scan cannot support, so the summary says so in those words rather than leaving it to be + inferred. Every hit is printed with its file, line and full source line so a human judges it. + * It does NOT sweep or edit. Fixing existing instances is a separate, owner-present task. + * It does NOT see the private companion repository, where the known instances live. A citation in + another repository is invisible to every check this one runs -- that limitation is inherent, is + the reason the item exists, and is printed rather than left implicit. + +The allocated set is read through `backlog_status_check.parse_items`, never a hand-rolled scan: +that module DEFINES what an item is, and a second definition here would drift from it silently. +""" + +from __future__ import annotations + +import argparse +import importlib.util +import re +import sys +from pathlib import Path +from typing import NamedTuple + +_HERE = Path(__file__).resolve().parent + +# The window the item pinned. Numbers below 1000 are the pre-partition internal sequence and the +# repository's PR/issue numbers, which share the `#N` spelling and are NOT backlog citations. +_LOW = 1000 +_HIGH = 9000 + +#: A citation is `#` immediately followed by digits AND ENDING THERE. A Markdown heading +#: (`## 1235.`) puts a SPACE between, so it cannot match -- which matters, because every item +#: heading in the ledger would otherwise report as a citation of itself. +#: +#: The trailing boundary is not cosmetic. Without it, a CSS/Mermaid hex colour matches its own digit +#: prefix: `#1565c0` reads as a citation of #1565 and `#06302b` as one of #6302. Measured on the +#: first real run over docs/ -- those two alone produced 8 of 40 hits, all spurious, across +#: ARCHITECTURE.md and architecture-diagram.md. An inflated count in a tool whose entire output is a +#: bound is the one failure that makes it useless. +_CITATION = re.compile(r"#(\d+)(?![0-9A-Za-z])") + +#: A `#N` introduced by one of these is a PULL REQUEST, issue or forum reference, not a backlog +#: citation. This repository's PR numbers ALREADY exceed 1000 (`PR #995`, `PR #1001` are both cited +#: in docs/adr/), so the [1000,9000) window does not separate the two namespaces on its own -- +#: measured, not assumed. Matching hits are still REPORTED and still COUNTED; they are only +#: annotated, because this item's discipline is to DISCLOSE a false positive rather than trim it. +_PR_CONTEXT = re.compile(r"(?i)\b(?:PR|pull request|commit|issue|discussion)\s+$") + +#: `pyodbc#1459`, `coder/code-server#6256` -- a `#N` glued to an identifier is ANOTHER PROJECT's +#: issue number. Both shapes are live in docs/ today. +_FOREIGN_REPO = re.compile(r"[A-Za-z0-9_./-]$") + + +class Hit(NamedTuple): + path: Path + lineno: int + number: int + line: str + pr_shaped: bool + + +def _load_backlog_module() -> object: + """Import backlog_status_check by path -- it is a script directory, not an installed package.""" + spec = importlib.util.spec_from_file_location( + "_backlog_status_check", _HERE / "backlog_status_check.py" + ) + if spec is None or spec.loader is None: # pragma: no cover - a packaging accident, not a state + raise RuntimeError("cannot load backlog_status_check.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def allocation_floor(sources: list[Path] | None = None) -> int: + """The highest item number the ledgers know about. Nothing at or below it can ever be issued. + + THIS IS A STRUCTURAL PROPERTY OF THE ALLOCATOR, NOT A FACT ABOUT ANY REGISTRY FILE. + `scripts/coord/alloc.ps1` starts its search at ``$observed + 1`` (or + ``[Math]::Max($observed, $PublicBacklogFloor - 1) + 1`` under the public clamp) and scans + UPWARD. It never fills a hole: "numbers are never reclaimed ... holes are free, collisions are + not". So a number at or below the high-water mark is unreachable **forever**, and a number above + it will be issued in the normal course. That is the whole classification. + + An earlier version of this tool read the allocation records under the git common dir instead. + That gave the same answers here and was the wrong basis: those records are machine-local, + uncommittable and losable, so garbage-collecting a directory would have "changed" a conclusion + that does not actually depend on it. Reasoning from the registry makes a sound property look + fragile and invites a guard nothing needs. + + Deliberately conservative: the true floor is the max over origin/main, every ref AND every + allocation, so this ledger-only figure can only ever be LOWER. A number between the two is + reported as reachable when it is not -- an over-warning, never a missed trap. + """ + numbers = allocated_numbers(sources) + return max(numbers) if numbers else 0 + + +def allocated_numbers(sources: list[Path] | None = None) -> set[int]: + """Every item number that EXISTS in the ledgers, open or closed. + + Closed items are included deliberately: a citation to a closed item resolves correctly and is + not this defect. Only a number that names nothing at all is the trap. + """ + module = _load_backlog_module() + paths = sources if sources is not None else [Path(p) for p in module.DEFAULT_SOURCES] # type: ignore[attr-defined] + numbers: set[int] = set() + for path in paths: + if not path.exists(): + continue + for item in module.parse_items(path.read_text(encoding="utf-8")): # type: ignore[attr-defined] + numbers.add(item.num) + return numbers + + +def citations_in(text: str) -> list[tuple[int, int, str, bool]]: + """(lineno, number, line, pr_shaped) for every in-window `#N` token. 1-indexed lines.""" + found: list[tuple[int, int, str, bool]] = [] + for lineno, line in enumerate(text.splitlines(), start=1): + for match in _CITATION.finditer(line): + number = int(match.group(1)) + if _LOW <= number < _HIGH: + before = line[: match.start()] + pr_shaped = ( + _PR_CONTEXT.search(before) is not None + or _FOREIGN_REPO.search(before) is not None + ) + found.append((lineno, number, line.strip(), pr_shaped)) + return found + + +def unresolved_citations(paths: list[Path], allocated: set[int]) -> list[Hit]: + hits: list[Hit] = [] + for path in paths: + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + for lineno, number, line, pr_shaped in citations_in(text): + if number not in allocated: + hits.append(Hit(path, lineno, number, line, pr_shaped)) + return hits + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter + ) + parser.add_argument("paths", nargs="*", type=Path, help="files to scan (default: docs/**/*.md)") + parser.add_argument( + "--fail", action="store_true", help="exit non-zero when any citation is unresolved" + ) + args = parser.parse_args(argv) + + # A scanner that CRASHES partway prints a partial list that reads as a complete one. Source lines + # legitimately carry characters a stock Windows cp1252 console cannot encode, and this tool hit + # exactly that on its first real run (U+2194, inside an ADR). Force UTF-8 and KEEP errors=replace: + # the defect is the wrong codec, and replacement is what stops one exotic glyph truncating a + # measurement. + if hasattr(sys.stdout, "reconfigure"): + sys.stdout.reconfigure(encoding="utf-8", errors="replace") + + paths = args.paths or sorted(Path("docs").rglob("*.md")) + allocated = allocated_numbers() + hits = unresolved_citations(list(paths), allocated) + + if not hits: + print(f"No unresolved backlog citation in {len(paths)} file(s).") + print(f"Resolved against {len(allocated)} allocated item numbers (open and closed).") + return 0 + + floor = allocation_floor() + + for hit in hits: + note = ( + " [PR/issue/foreign-repo shaped -- very likely NOT a backlog citation]" + if hit.pr_shaped + else "" + ) + print(f"{hit.path}:{hit.lineno}: #{hit.number} resolves to no filed item{note}") + print(f" {hit.line}") + state = ( + f"BELOW THE FLOOR ({floor}) -- the allocator only ever issues above its high-water " + "mark, so this number can NEVER be issued and the citation is permanently harmless." + if hit.number <= floor + else f"ABOVE THE FLOOR ({floor}) -- this number CAN still be issued to unrelated work. " + "This is the live shape." + ) + print(f" -> {state}") + + distinct = sorted({hit.number for hit in hits}) + pr_shaped = sum(1 for hit in hits if hit.pr_shaped) + print() + print( + f"{len(hits)} token(s) across {len({h.path for h in hits})} file(s); " + f"{len(distinct)} distinct number(s): {', '.join(f'#{n}' for n in distinct)}" + ) + if pr_shaped: + print( + f"{pr_shaped} of those {len(hits)} TOKENS (not of the {len(distinct)} numbers) are " + f"PR/issue/foreign-repo shaped, annotated above." + ) + print( + "This is an UPPER BOUND ON CITATIONS, NOT A COUNT OF DEFECTS -- some hits are very likely" + ) + print("not backlog references at all. Each is printed above with its source line; judge them.") + print( + "Not scanned: the private companion repository, where a citation is invisible to this repo." + ) + return 1 if args.fail else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_dangling_citation_check.py b/tests/test_dangling_citation_check.py new file mode 100644 index 00000000..06022e16 --- /dev/null +++ b/tests/test_dangling_citation_check.py @@ -0,0 +1,176 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The unresolved-backlog-citation detector (BACKLOG #1235). + +Every false-positive shape asserted here was found by RUNNING the tool over docs/, not predicted +before it. Two of them were defects in the detector itself on its first real run: a hex colour +matching its own digit prefix, and a crash on a character a cp1252 console cannot encode. They are +pinned as tests because both re-appear the moment the pattern is loosened. +""" + +from __future__ import annotations + +import importlib.util +import pathlib +from pathlib import Path + +import pytest + +_SPEC = importlib.util.spec_from_file_location( + "_dangling_citation_check", + Path(__file__).resolve().parents[1] / "scripts" / "docs" / "dangling_citation_check.py", +) +assert _SPEC is not None and _SPEC.loader is not None +cc = importlib.util.module_from_spec(_SPEC) +_SPEC.loader.exec_module(cc) + + +def _numbers(text: str) -> list[int]: + return [number for _lineno, number, _line, _pr in cc.citations_in(text)] + + +# --- what a citation IS --------------------------------------------------------------------------- + + +def test_a_plain_citation_is_found() -> None: + assert _numbers("see BACKLOG #1235 for the rule") == [1235] + + +def test_line_numbers_are_one_indexed() -> None: + lineno, number, _line, _pr = cc.citations_in("first\nsecond #1235\n")[0] + assert (lineno, number) == (2, 1235) + + +@pytest.mark.parametrize("trailing", [".", ",", ")", "'s", " and", "]", ";"]) +def test_ordinary_punctuation_still_closes_a_citation(trailing: str) -> None: + assert _numbers(f"see #1235{trailing}") == [1235] + + +# --- what a citation IS NOT ----------------------------------------------------------------------- + + +def test_a_markdown_heading_is_not_a_citation_of_itself() -> None: + """Every item heading in the ledger would otherwise report as citing its own number.""" + assert _numbers("## 1235. a citation to an unallocated number") == [] + + +@pytest.mark.parametrize("colour", ["#1565c0", "#06302b", "#1234ab"]) +def test_a_hex_colour_does_not_match_its_own_digit_prefix(colour: str) -> None: + """Measured, not predicted: on the first real run over docs/, `#1565c0` and `#06302b` alone + produced 8 of 40 hits. An inflated count in a tool whose whole output is a bound is fatal.""" + assert _numbers(f"classDef x fill:#e3f2fd,stroke:{colour};") == [] + + +def test_numbers_outside_the_window_are_ignored() -> None: + assert _numbers("PR #995 and issue #42 and #9000 and #12345") == [] + + +def test_the_window_is_half_open_at_both_ends() -> None: + assert _numbers("#999") == [] + assert _numbers("#1000") == [1000] + assert _numbers("#8999") == [8999] + assert _numbers("#9000") == [] + + +# --- annotation, which discloses rather than trims ------------------------------------------------- + + +@pytest.mark.parametrize( + "line", + [ + "shipped in PR #1001", + "see pull request #1001", + "fixed by commit #1001", + "upstream issue #1001", + "code-server discussion #6256", + ], +) +def test_a_pr_or_forum_reference_is_annotated(line: str) -> None: + hits = cc.citations_in(line) + assert len(hits) == 1 + assert hits[0][3] is True, "should be flagged as very likely not a backlog citation" + + +@pytest.mark.parametrize("line", ["upstream pyodbc#1459", "coder/code-server#6256"]) +def test_a_foreign_repo_reference_is_annotated(line: str) -> None: + hits = cc.citations_in(line) + assert len(hits) == 1 + assert hits[0][3] is True + + +def test_a_genuine_citation_is_not_annotated() -> None: + hits = cc.citations_in("this supersedes #1084") + assert len(hits) == 1 + assert hits[0][3] is False + + +def test_an_annotated_hit_is_still_reported_and_still_counted() -> None: + """The item's discipline is to DISCLOSE a false positive, never to trim it -- a trimmed scan + silently understates, which is the failure this whole item is about.""" + assert len(cc.citations_in("upstream pyodbc#1459")) == 1 + + +# --- resolution against the ledger ------------------------------------------------------------------ + + +def test_only_unallocated_numbers_are_reported(tmp_path: Path) -> None: + doc = tmp_path / "d.md" + doc.write_text("real #1230, unreal #8999\n", encoding="utf-8") + hits = cc.unresolved_citations([doc], allocated={1230}) + assert [h.number for h in hits] == [8999] + + +def test_a_closed_item_still_resolves(tmp_path: Path) -> None: + """A citation to a CLOSED item points at something real. Only a number naming nothing is the trap.""" + doc = tmp_path / "d.md" + doc.write_text("see #1230\n", encoding="utf-8") + assert cc.unresolved_citations([doc], allocated={1230}) == [] + + +def test_an_unreadable_file_is_skipped_not_fatal(tmp_path: Path) -> None: + missing = tmp_path / "gone.md" + assert cc.unresolved_citations([missing], allocated=set()) == [] + + +def test_the_real_ledger_yields_a_plausible_allocated_set() -> None: + """Positive control on the ledger read: if parse_items ever returns nothing, every citation in + the repository would report as unresolved and the tool would look catastrophically alarming.""" + allocated = cc.allocated_numbers() + assert len(allocated) > 100 + assert 1235 in allocated, "the item that defines this tool must resolve" + + +# --- the floor, which decides whether a citation can ever arm --------------------------------------- + + +def test_the_floor_is_found_and_is_plausible() -> None: + """Positive control. If the ledger read ever silently returned nothing, the floor would be 0 and + EVERY citation would classify as live -- 26 manufactured alarms on this repository.""" + assert cc.allocation_floor() > 1000 + + +def test_a_number_below_the_floor_can_never_be_issued() -> None: + """#1203 and #1231 are the two instances BACKLOG #1235 names as live traps. Both sit BELOW the + high-water mark, and alloc.ps1 starts at `$observed + 1` and never fills a hole, so neither can + ever be issued. The citations are inert BY CONSTRUCTION, not by luck. + + Note what this does NOT depend on: any allocation record under .git. Those are machine-local and + losable; the unreachability is structural and survives losing them.""" + floor = cc.allocation_floor() + assert floor >= 1203 + assert floor >= 1231 + filed = cc.allocated_numbers() + assert 1203 not in filed, "if #1203 gets filed this test has served its purpose -- update it" + assert 1231 not in filed + + +def test_a_number_above_the_floor_is_the_live_shape() -> None: + assert cc.allocation_floor() < 8999 + + +def test_the_floor_is_conservative_never_optimistic(tmp_path: pathlib.Path) -> None: + """Built from the ledgers only, so it can only ever UNDERSTATE the allocator's true floor (which + also spans refs and allocations). Understating means over-warning, never a missed trap.""" + ledger = tmp_path / "L.md" + ledger.write_text("## 1500. an item\n\n> open\n", encoding="utf-8") + assert cc.allocation_floor([ledger]) == 1500 From 8387c88db4cf9e43354dac97aa5e5aabf4f29926 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 14 Aug 2026 00:16:06 -0500 Subject: [PATCH 5/9] docs(tooling): record WHY the floor's conservatism is the design (BACKLOG #1235) Docstring only; no behaviour change. The dispatcher and I converged on the allocator's mechanics through four positions between us, and the durable part is not in the code yet. Two additions: WHY THE LEDGER-ONLY FLOOR CAN ONLY UNDERSTATE, as a structural argument rather than an assertion: the allocator's observed set is a SUPERSET of the ledger headings -- it also reads refs, the working tree, claim files and a persisted high-water mark (alloc.ps1:97). Independently reproduced: ledger floor 1250 on origin/main against the allocator's own 1254. WHY AN ABOVE-FLOOR REPORT ON A FRESHLY-ALLOCATED NUMBER IS CORRECT AND NOT A FALSE POSITIVE. The ratchet persists the max of the OBSERVED set, not the number just issued (:205, :214, :215), so after issuing N the watermark holds N-1 and the only durable record of N is an untracked, never-pushed registry file. Lose it and N is re-issued. The flag then clears itself once the heading lands and the floor rises past it -- which is precisely when the number becomes permanently reserved. Recorded because both are things a future reader would otherwise "fix": by treating a live report on a just-allocated number as noise, or by adding a near-the-floor heuristic for an edge this floor already covers. #1253, #1254 and #1255 are in exactly that state today. --- scripts/docs/dangling_citation_check.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/scripts/docs/dangling_citation_check.py b/scripts/docs/dangling_citation_check.py index 73e8d51d..067bd888 100644 --- a/scripts/docs/dangling_citation_check.py +++ b/scripts/docs/dangling_citation_check.py @@ -131,9 +131,22 @@ def allocation_floor(sources: list[Path] | None = None) -> int: that does not actually depend on it. Reasoning from the registry makes a sound property look fragile and invites a guard nothing needs. - Deliberately conservative: the true floor is the max over origin/main, every ref AND every - allocation, so this ledger-only figure can only ever be LOWER. A number between the two is - reported as reachable when it is not -- an over-warning, never a missed trap. + Deliberately conservative: the allocator's observed set is a SUPERSET of the ledger headings -- + it also reads refs, the working tree, claim files and a persisted high-water mark (``:97``) -- so + this ledger-only figure can only ever sit at or BELOW the true floor. A number between the two is + reported as reachable when it is not: an over-warning, never a missed trap. + + THAT GAP IS THE POINT, NOT A ROUNDING ERROR, AND THE FLAGS IT PRODUCES ARE NOT FALSE POSITIVES. + A number that has been ALLOCATED but whose heading is not yet committed sits above this floor and + is reported live -- correctly, because the allocator's ratchet persists the max of the observed + set rather than the number just issued, so until that heading lands the only durable record of it + is an untracked, never-pushed registry file. Lose that file and the number is re-issued. + + The flag then clears ITSELF: once the heading is committed the floor rises past it and it stops + being reported, which is exactly when it becomes permanently reserved. Do not "fix" a live report + on a freshly-allocated number, and do not add a near-the-floor rule to catch this case -- this + floor already covers it, and a second rule aimed at an edge the first one covers is how a tool + acquires a guard nothing needs. """ numbers = allocated_numbers(sources) return max(numbers) if numbers else 0 From 4c93bb89fff696a011488cdfe94ed3e358b321e7 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 14 Aug 2026 03:52:18 -0500 Subject: [PATCH 6/9] test(tooling): gate the whole scripts/ tree against a cp1252 console abort (BACKLOG #1030) Enforcement was per-file and hand-placed: test_cli.py asserts one STRING is encodable, test_announce_hook.py asserts one FILE is ASCII, test_session_mail.py names five mail scripts in a literal list. None generalises, so a glyph reaching print() from any other script was caught only by a human reading the diff -- and the class recurred at least three times, the third TONIGHT, when a scanner I had just written died mid-scan on U+2194 and printed a partial list that read as complete. THE GATE IS ON REACHING AN UNGUARDED STREAM, NOT ON BARE ENCODABILITY, and that is the whole design decision the item flagged as its real difficulty. sys.stdout carries errors='surrogateescape', which round-trips only lone surrogates in DC80-DCFF; every other unencodable codepoint still raises. sys.stderr carries backslashreplace and never raises. That asymmetry, not a strict/non-strict split, is why the same text survives on stderr and aborts on stdout. So a file may carry non-cp1252 characters IF IT HARDENS ITS OWN STDOUT. That is not an exemption list: it is a property of the file, checked mechanically, and it is the REMEDY rather than a promise about one. scripts/docs/backlog_status_check.py is why the distinction is load-bearing -- its argparse description quotes the machine-parsed banner alphabet CLAUDE.md section 11 protects, and remediation text that cannot show an author the character it wants added is not actionable. A gate that could not express that would fire on correct code and be switched off. That script is therefore FIXED rather than scrubbed: a stdout reconfigure at the CLI entry point, scoped so importers are untouched. Measured before: `--help` raised UnicodeEncodeError on U+2705. After: exit 0, the gate still reports 517 items, and parse_items still imports clean. THE THREE PROPERTIES THE ITEM NAMES, each kept and each tested: * PRINTS AND PINS WHAT IT SCANNED -- a walk that collapses to zero files would otherwise report clean forever. Asserted, not merely emitted. * READS THE WHOLE FILE, never line by line. splitlines() CONSUMES U+2028/U+2029, so a line-oriented scan is structurally blind to them; a test demonstrates that mechanism rather than asserting it. * NEVER SILENTLY DROPS A FILE -- an undecodable file is a FAILURE, not a skip. MEASURED, NOT INHERITED: the item counted 43 characters across four scripts/ files on 2026-08-05; today it is ONE file, and it is the one that must keep them. Three were cleaned in between. The control set is built from characters with recorded failures behind them, via chr() so this test stays cp1252-clean itself -- verified: 0 non-cp1252 characters in it. Scope stated in the file: scripts/**/*.py only. The engine already hardens both streams at __main__.py; .ps1 has no equivalent reconfigure so generalising there is a different decision; docs/ is out because BACKLOG.md is a sanctioned holdout for that same alphabet. --- scripts/docs/backlog_status_check.py | 13 ++ tests/test_cp1252_console_safety.py | 175 +++++++++++++++++++++++++++ 2 files changed, 188 insertions(+) create mode 100644 tests/test_cp1252_console_safety.py diff --git a/scripts/docs/backlog_status_check.py b/scripts/docs/backlog_status_check.py index be5f4218..3164826b 100644 --- a/scripts/docs/backlog_status_check.py +++ b/scripts/docs/backlog_status_check.py @@ -196,6 +196,19 @@ def scan( def main(argv: list[str] | None = None) -> int: + # THIS MODULE MUST CARRY NON-cp1252 CHARACTERS, so it hardens the stream instead of losing them + # (BACKLOG #1030). The docstring below is argparse's description and quotes the machine-parsed + # banner alphabet CLAUDE.md section 11 protects; remediation text that cannot show an author the + # character it wants added is not actionable. On a stock Windows cp1252 console `--help` therefore + # raised UnicodeEncodeError on U+2705 before this line -- measured, not theorised. + # + # `errors="replace"` is deliberate and is NOT a way of tolerating mangled text: the codec is what + # was wrong, and it is fixed here to UTF-8. Replacement is the backstop for a stream that cannot + # be reconfigured at all, so one exotic codepoint can never again truncate a gate's output + # mid-sentence. Scoped to the CLI entry point: importers get their own stdout untouched. + if hasattr(sys.stdout, "reconfigure"): + sys.stdout.reconfigure(encoding="utf-8", errors="replace") + root = Path(__file__).resolve().parents[2] ap = argparse.ArgumentParser( description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter diff --git a/tests/test_cp1252_console_safety.py b/tests/test_cp1252_console_safety.py new file mode 100644 index 00000000..dd522e06 --- /dev/null +++ b/tests/test_cp1252_console_safety.py @@ -0,0 +1,175 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""No script under ``scripts/`` can abort on a stock Windows console (BACKLOG #1030). + +THE DEFECT THIS REPLACES. Enforcement was per-file and hand-placed: ``tests/test_cli.py`` asserts one +STRING is cp1252-encodable, ``tests/test_announce_hook.py`` asserts one FILE is ASCII, and +``tests/test_session_mail.py`` names five mail scripts in a literal list. None generalises, so a glyph +reaching ``print()`` from any other script was caught only by a human reading the diff -- and the +class recurred at least three times. + +WHAT IS GATED, AND WHY IT IS NOT BARE ENCODABILITY. The failure is a character reaching a stream that +can RAISE, not a character existing. ``sys.stdout`` carries ``errors='surrogateescape'``, which +round-trips only lone surrogates in DC80-DCFF; every other unencodable codepoint still raises. +``sys.stderr`` carries ``backslashreplace`` and never raises -- that asymmetry, not a strict/non-strict +split, is why the same text survives on stderr and aborts on stdout. + +So a file may carry non-cp1252 characters IF IT HARDENS ITS OWN STDOUT. That is not an exemption list: +it is a property of the file, checked mechanically, and it is the actual remedy rather than a promise +about one. ``scripts/docs/backlog_status_check.py`` is exactly why the distinction is load-bearing -- +its argparse description quotes the machine-parsed banner alphabet CLAUDE.md section 11 protects, and +remediation text that cannot show an author the character it wants added is not actionable. A gate +that could not express that would fire on correct code and be switched off. + +THREE PROPERTIES THIS KEEPS, each of which the item names: + + * IT PRINTS WHAT IT SCANNED. A filtered scan that skips a file type reads as clean when it never + looked. The inventory is asserted, not merely emitted, so a collapse to zero files fails here + instead of passing silently. + * IT READS THE WHOLE FILE, never line by line. ``str.splitlines()`` splits on U+2028 and U+2029 and + consumes them, so a line-oriented scan is structurally blind to the two separators most likely to + break a terminal. + * IT NEVER SILENTLY DROPS A FILE. A file that will not decode as UTF-8 is a FAILURE, not a skip. + +SCOPE, STATED RATHER THAN IMPLIED. ``scripts/**/*.py`` only. The engine already hardens both streams +at ``messagefoundry/__main__.py``, and the ``.ps1`` surface has no equivalent reconfigure, so +generalising to PowerShell is a different decision and is left to the existing per-file gates. +``docs/`` is deliberately out: ``docs/BACKLOG.md`` is a sanctioned holdout for that same alphabet. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[1] +_SCRIPTS = _ROOT / "scripts" + +#: The remedy, detected as a property of the file. A call that rebinds stdout's codec is the only +#: thing that actually stops the abort, which is why it -- and not a promissory comment -- is the +#: signal. Matched loosely on the call itself so a keyword reordering does not silently un-exempt. +_HARDENS_STDOUT = re.compile(r"stdout\s*\.\s*reconfigure\s*\(", re.MULTILINE) + + +def _python_scripts() -> list[Path]: + return sorted(p for p in _SCRIPTS.rglob("*.py") if "__pycache__" not in p.parts) + + +def _unencodable(text: str) -> list[str]: + """Distinct characters cp1252 cannot represent, in codepoint order. + + Whole-string, deliberately: see the module docstring on ``splitlines`` eating U+2028/U+2029. + """ + bad: set[str] = set() + for ch in set(text): + try: + ch.encode("cp1252") + except UnicodeEncodeError: + bad.add(ch) + return sorted(bad) + + +def test_the_scan_actually_covers_something() -> None: + """PRINT AND PIN WHAT WAS SCANNED. A scan whose file list collapses to nothing reports a clean + result forever; this is the positive control that stops that being indistinguishable from green. + """ + found = _python_scripts() + print(f"scanned {len(found)} python files under scripts/") + assert len(found) >= 25, ( + f"only {len(found)} files under scripts/ -- the walk is not finding them" + ) + assert (_SCRIPTS / "docs" / "backlog_status_check.py") in found + + +def test_every_script_file_decodes_as_utf8() -> None: + """A file that will not decode is a FAILURE, never a silent skip -- an undecodable file is the + one most likely to carry the bytes this gate exists to find.""" + undecodable: list[str] = [] + for path in _python_scripts(): + try: + path.read_text(encoding="utf-8") + except UnicodeDecodeError as exc: + undecodable.append(f"{path.relative_to(_ROOT)}: {exc}") + assert not undecodable, "not decodable as UTF-8:\n " + "\n ".join(undecodable) + + +def test_no_script_can_abort_a_cp1252_console() -> None: + """The gate itself: a script may carry non-cp1252 characters only if it hardens its own stdout.""" + offenders: list[str] = [] + exempted: list[str] = [] + for path in _python_scripts(): + text = path.read_text(encoding="utf-8") + bad = _unencodable(text) + if not bad: + continue + rel = path.relative_to(_ROOT) + shown = " ".join(f"U+{ord(c):04X}" for c in bad[:6]) + if _HARDENS_STDOUT.search(text): + exempted.append(f"{rel} ({len(bad)} distinct: {shown})") + continue + offenders.append( + f"{rel} carries {len(bad)} non-cp1252 character(s) [{shown}] and does NOT " + f"reconfigure sys.stdout -- printing any of them aborts on a stock Windows console" + ) + print(f"carrying non-cp1252 characters, hardened and therefore allowed: {exempted or 'none'}") + assert not offenders, "\n ".join(["scripts that can abort a cp1252 console:", *offenders]) + + +# --- the detector's own controls, so a green above is evidence rather than a pattern that quietly +# --- stopped matching ---------------------------------------------------------------------------- + +#: Built with chr(), never literals. This file must stay cp1252-clean itself -- a gate whose own +#: test could abort the console it defends would be the joke version of this item -- and chr() also +#: keeps it inside CLAUDE.md section 11, naming a character without adopting one. Every entry has a +#: recorded failure behind it; none is hypothetical. +_BROKE_SOMETHING = [ + chr(0x2192), # broke `messagefoundry --help` (the adr-analyze arrow) + chr(0x2705), # banner alphabet; broke this repo's own backlog gate --help + chr(0x26D4), # banner alphabet + chr(0x1F522), # banner alphabet; the documented cp1252 console crash + chr(0x2194), # crashed a scanner mid-scan this session, TRUNCATING its output + chr(0x2028), # line separator: invisible to any splitlines()-based scan + chr(0x2029), # paragraph separator: same +] + + +@pytest.mark.parametrize("ch", _BROKE_SOMETHING, ids=lambda c: f"U+{ord(c):04X}") +def test_the_detector_sees_every_character_that_has_actually_broken_something(ch: str) -> None: + assert _unencodable(f"x{ch}y") == [ch] + + +def test_the_detector_does_not_fire_on_representable_text() -> None: + """U+2014 and U+00A3 ARE cp1252-representable and must not be flagged. The item calls this out: + a gate that fires on an em dash gets switched off within a day.""" + text = "plain ASCII, an em dash " + chr(0x2014) + ", and a pound sign " + chr(0x00A3) + assert _unencodable(text) == [] + + +def test_the_line_oriented_blindness_is_real_and_this_scan_avoids_it() -> None: + """Demonstrates the mechanism instead of asserting it: ``splitlines()`` CONSUMES U+2028, so a + line-oriented scan is structurally unable to see it. The whole-string scan does.""" + sep = chr(0x2028) + text = "before" + sep + "after" + assert sep not in "".join(text.splitlines()), "splitlines would have hidden it" + assert _unencodable(text) == [sep] + + +def test_the_hardening_signal_is_detected_and_is_not_vacuous() -> None: + """The exemption must be the REMEDY itself, not a promise about one.""" + assert _HARDENS_STDOUT.search('sys.stdout.reconfigure(encoding="utf-8", errors="replace")') + assert _HARDENS_STDOUT.search("sys . stdout . reconfigure ( encoding='utf-8' )") + assert not _HARDENS_STDOUT.search("# we should probably reconfigure stdout one day") + assert not _HARDENS_STDOUT.search("sys.stderr.reconfigure(encoding='utf-8')") + + +def test_a_synthetic_offender_is_caught_and_a_hardened_one_is_not() -> None: + """The gate proved in BOTH directions, on files it has never seen.""" + glyph = chr(0x2705) + bare = f'print("{glyph} done")' + hardened = 'import sys; sys.stdout.reconfigure(encoding="utf-8"); ' + bare + assert _unencodable(bare) == [glyph] + assert not _HARDENS_STDOUT.search(bare) + assert _unencodable(hardened) == [glyph] + assert _HARDENS_STDOUT.search(hardened) From 2a1f30cc8cdbf99462c7c527eccede9e81901625 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 14 Aug 2026 04:26:45 -0500 Subject: [PATCH 7/9] docs(tests): record what a scrubbing gate would destroy, with no pinned count (BACKLOG #1030) Docstring only; no behaviour change. 14 tests unchanged and passing. The gate's design -- harden the stream, never scrub the file -- was argued from principle. This records the concrete case that lives in this repository, because the next reader to see a lone variation selector in a source file will think it is lint. backlog_status_check.py carries one non-cp1252 character beyond the banner alphabet: a lone U+FE0F inside the banner regex as []\uFE0F?\s. That is an OPTIONAL VS-16, letting a banner be written with or without the selector -- the handling CLAUDE.md section 11 mandates for any regex touching that alphabet. Invisible at the point of use. Delete it and the `?` binds to the CHARACTER CLASS. The pattern STILL COMPILES, so nothing at author time objects. It matches an indented continuation line, group("emoji") returns None, and the dispatch evaluates `None in _CLOSED` where _CLOSED is a str -- TypeError on any run touching the real ledger. Verified by rebuilding the module with the character stripped and running parse_items against docs/BACKLOG.md. Two failure modes, and the second is the dangerous one: LOUDLY today, since every gate calling parse_items dies; SILENTLY later, when the first banner authored WITH a selector stops matching -- and no banner carries one today, so nothing would catch that regression on the day it arrives. NO COUNT IS PINNED, DELIBERATELY, and the omission is the point. An earlier draft of this note said the scrub "silently inverts the parser while every test stays green". That was FALSE -- it crashes -- and it was falsifiable in one command, which would have discredited a correct surrounding argument. The qualifying-line count is also ref-relative and grows with every filed item: measured 74 on origin/main and 78 on an unpushed branch in the same hour. A figure would be stale the moment it was written, and re-reading it would reproduce it, which reads as verification. Same hazard the conftest banner refuses for the same reason. --- tests/test_cp1252_console_safety.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/test_cp1252_console_safety.py b/tests/test_cp1252_console_safety.py index dd522e06..589fab7e 100644 --- a/tests/test_cp1252_console_safety.py +++ b/tests/test_cp1252_console_safety.py @@ -21,6 +21,32 @@ class recurred at least three times. remediation text that cannot show an author the character it wants added is not actionable. A gate that could not express that would fire on correct code and be switched off. +WHAT A SCRUBBING GATE WOULD DESTROY IN THIS REPOSITORY, MEASURED RATHER THAN IMAGINED. Besides the +banner alphabet, ``backlog_status_check.py`` carries one further non-cp1252 character: a lone U+FE0F +inside the banner regex, as ``[]\\uFE0F?\\s``. That is an OPTIONAL VS-16, letting a banner be +written with or without the selector -- exactly the handling CLAUDE.md section 11 mandates for any +regex touching that alphabet. It is invisible at the point of use and looks like lint. + +Delete it and the ``?`` binds to the CHARACTER CLASS instead. The pattern STILL COMPILES, so nothing +at author time objects. It then matches an indented continuation line (``^>\\s\\s``, which the ledger +is full of), ``b.group("emoji")`` returns ``None``, and the dispatch below it evaluates +``None in _CLOSED`` where ``_CLOSED`` is a ``str`` -- ``TypeError``, on any run that touches the real +ledger. Two failure modes, and the second is the dangerous one: + + * LOUDLY, TODAY -- every gate that calls ``parse_items`` dies, which is most of them. + * SILENTLY, LATER -- the first banner authored WITH a selector stops matching. No banner carries + one today, so nothing would catch that regression on the day it arrives. + +That is the case for hardening the stream rather than scrubbing the file, and it is why the exemption +had to be expressible: one invisible character, removed by a well-meaning gate, takes out the reader +every ledger gate depends on. + +NO COUNT IS PINNED IN THAT ARGUMENT, DELIBERATELY. The number of qualifying lines is ref-relative and +grows with every filed item, so a figure would be stale the moment it was written -- and re-reading +it would reproduce it, which reads as verification. That is the same hazard the banner in +``conftest.py`` refuses for the same reason. The mechanism above needs no number and is re-derivable +in one command on any ref. + THREE PROPERTIES THIS KEEPS, each of which the item names: * IT PRINTS WHAT IT SCANNED. A filtered scan that skips a file type reads as clean when it never From 023eb094bca00ff59586c3e6c8e98b7496852dc3 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 14 Aug 2026 07:09:39 -0500 Subject: [PATCH 8/9] refactor(text): one C0/DEL predicate instead of seven copies (BACKLOG #1253) `ord(ch) < 0x20 or ord(ch) == 0x7F` was written out seven times across six files. Every copy agreed, so nothing was mis-screened; the cost was future-tense and is the one #1239 named -- a later hardening applied to one copy silently does not apply to the rest, and nothing reports the omission. THIS SHARES THE PREDICATE, NOT THE ACTION, and that is the design rather than an implementation detail. #1239 explicitly ruled out "collapsing the call sites into one helper with a flag", because the differing wrappers are appropriate: a raise suits a path context, a bool suits a filter, and the exception differs by layer (WiringError in config, a PHI-safe NegativeAckError in FHIR). So every call site keeps its own refusal and its own message; only the TEST moved. A flag parameter would have re-created the coupling this item exists to remove, one indirection further away. TWO ACTIONS ARE PRESERVED, and one must never be "simplified" into the other. Six sites REJECT. transports/rest.py STRIPS, on a message-derived header VALUE, and that is defensible rather than a second instance of the mutation pattern the owner ruled against in #1238: that ruling turns on basename() converting a path into a valid-but-DIFFERENT target, handing an attacker a real file. A header value has no such property -- removing CR/LF cannot redirect a request anywhere -- and rest.py already REJECTS a header NAME failing its RFC 7230 token check. Name-rejected, value-stripped. parsing/sniff.py is deliberately NOT folded in: it tests the same code points but is byte-wise rather than character-wise and subtracts an allowlist, because a text sniffer must tolerate tab, CR and LF. Folding it in would change its behaviour. TESTS. #1239 asked for proof that "the two predicates agree across a shared character corpus". With one predicate that obligation becomes a CHARACTERISATION test: the caught set is pinned over U+0000-U+02FF against an independently-written definition, the boundaries are pinned at each edge (0x1F in, 0x20 out, 0x7E out, 0x7F in, 0x80 out), and C1 plus the Unicode separators are pinned as deliberately NOT caught -- widening this is now a behaviour change at seven sites at once, which is the leverage and also the risk. 47 new tests; 256 existing tests across the six touched modules pass unchanged. BASE VERIFIED BEFORE BUILDING, because #1242's limb 4 was refused on exactly this ground: all six files are byte-identical between this branch and origin/main, and the three commits this branch is behind touch BACKLOG.md, scripts/asvs/apply.py and .claude/settings.json only. That is the discriminator -- apply.py differs here, these do not. --- messagefoundry/config/codeset_edit.py | 3 +- messagefoundry/config/impact.py | 3 +- messagefoundry/controlchars.py | 56 ++++++++++++++++ messagefoundry/transports/dicomweb.py | 3 +- messagefoundry/transports/fhir.py | 5 +- messagefoundry/transports/remotefile.py | 3 +- messagefoundry/transports/rest.py | 3 +- tests/test_controlchars.py | 87 +++++++++++++++++++++++++ 8 files changed, 156 insertions(+), 7 deletions(-) create mode 100644 messagefoundry/controlchars.py create mode 100644 tests/test_controlchars.py diff --git a/messagefoundry/config/codeset_edit.py b/messagefoundry/config/codeset_edit.py index 9a0826b7..a61c3b49 100644 --- a/messagefoundry/config/codeset_edit.py +++ b/messagefoundry/config/codeset_edit.py @@ -40,6 +40,7 @@ load_code_sets, ) from messagefoundry.config.wiring import WiringError +from messagefoundry.controlchars import has_control_char from messagefoundry.spreadsheet import SPREADSHEET_FORMULA_TRIGGERS, spreadsheet_safe #: Extensions the loader recognises (the writer only ever *writes* ``.csv``; both are read). @@ -302,7 +303,7 @@ def _validate_name(codesets_dir: Path, name: str) -> None: # Reject control characters (NUL, tab, newline, DEL, …) before they reach a filesystem call: an # embedded NUL makes Path.resolve() raise a bare ValueError the CLI's except clause can't catch # (crashing with no JSON on stdout), and none belong in a bare file stem regardless. - if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in name): + if has_control_char(name): raise WiringError(f"code set name {name!r} must not contain control characters") if "/" in name or "\\" in name: raise WiringError(f"code set name {name!r} must not contain a path separator") diff --git a/messagefoundry/config/impact.py b/messagefoundry/config/impact.py index c11089e1..6293b837 100644 --- a/messagefoundry/config/impact.py +++ b/messagefoundry/config/impact.py @@ -44,6 +44,7 @@ build_reference_index, ) from messagefoundry.config.wiring import Registry, WiringError +from messagefoundry.controlchars import has_control_char __all__ = [ "LiteralEdit", @@ -628,7 +629,7 @@ def _validate_new_name(config_dir: Path, target_kind: str, new: str) -> None: # A name is embedded verbatim into a Python/TOML string literal; a quote, backslash, or control # char would break out of the literal (or the value). Names are simple identifiers-in-practice — # refuse anything that could corrupt the rewrite. - if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in new): + if has_control_char(new): raise WiringError(f"the new name {new!r} must not contain control characters") if any(ch in new for ch in ("'", '"', "\\")): raise WiringError(f"the new name {new!r} must not contain a quote or backslash") diff --git a/messagefoundry/controlchars.py b/messagefoundry/controlchars.py new file mode 100644 index 00000000..f4308797 --- /dev/null +++ b/messagefoundry/controlchars.py @@ -0,0 +1,56 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The C0/DEL control-character test, written once (BACKLOG #1253). + +WHAT THIS REPLACES. ``ord(ch) < 0x20 or ord(ch) == 0x7F`` was written out seven times across six +files -- two in ``transports/fhir.py`` and one each in ``config/codeset_edit.py``, +``config/impact.py``, ``transports/dicomweb.py``, ``transports/remotefile.py`` and +``transports/rest.py``. Every copy agreed, so nothing was mis-screened. The cost was future-tense +and is the one #1239 named: a later hardening applied to one copy silently does not apply to the +rest, and nothing reports the omission. + +THIS SHARES THE PREDICATE, NOT THE ACTION, AND THAT DISTINCTION IS THE DESIGN. #1239 ruled out +"collapsing the call sites into one helper with a flag" because the differing wrappers are +appropriate: a raise suits a path context, a bool suits a filter, and the exceptions differ by layer +(``WiringError`` in config, a PHI-safe negative ACK in FHIR). So each call site keeps its own +refusal and its own message; only the TEST moves here. A flag parameter would have re-created the +coupling the item exists to remove, one indirection further away. + +TWO ACTIONS ARE PRESERVED ON PURPOSE, and one of them must never be "simplified" into the other: + + * REJECT -- six sites. A control character in a value that reaches a URL path, a header, a + filename or a config field is refused outright. + * STRIP -- ``transports/rest.py`` only, on a message-derived header VALUE. That is defensible + rather than a second instance of the mutation pattern the owner ruled against in #1238: that + ruling turns on ``basename()`` converting a path into a valid-but-DIFFERENT target, handing an + attacker a real file. A header value has no such property -- removing CR/LF cannot redirect a + request anywhere -- and ``rest.py`` already REJECTS a header NAME failing its RFC 7230 token + check. Name-rejected, value-stripped, which is principled. + +DELIBERATELY NOT FOLDED IN. ``parsing/sniff.py`` tests the same code points but is a genuinely +different predicate: it is byte-wise rather than character-wise and subtracts an allowlist, because +a text sniffer must tolerate tab, CR and LF. Folding it in would change its behaviour. + +THE POINT IS THE COPYING PRACTICE, not the seven known lines. If you need this test, import it. +""" + +from __future__ import annotations + + +#: C0 controls (U+0000-U+001F) plus DEL (U+007F). NOT a general "is this printable" test: it is +#: deliberately blind to C1 (U+0080-U+009F) and to Unicode separators, because every call site +#: screens values destined for byte-oriented sinks -- a request line, a header, a path -- where C0 +#: and DEL are the injection alphabet. Widening it is a behaviour change at seven call sites at +#: once, which is exactly the leverage this module exists to provide; make it deliberately. +def has_control_char(text: str) -> bool: + """True if ``text`` contains any C0 control character or DEL.""" + return any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in text) + + +def strip_control_chars(text: str) -> str: + """``text`` with every C0 control and DEL removed. + + The strip arm, used where a value must be neutralised rather than refused. See the module + docstring: this is NOT the general remedy and must not be substituted for a rejection. + """ + return "".join(ch for ch in text if not (ord(ch) < 0x20 or ord(ch) == 0x7F)) diff --git a/messagefoundry/transports/dicomweb.py b/messagefoundry/transports/dicomweb.py index bd7f71b6..5013e448 100644 --- a/messagefoundry/transports/dicomweb.py +++ b/messagefoundry/transports/dicomweb.py @@ -45,6 +45,7 @@ from typing import Any from messagefoundry.config.models import ConnectorType, Destination +from messagefoundry.controlchars import has_control_char from messagefoundry.transports.base import ( DeliveryError, DeliveryResponse, @@ -93,7 +94,7 @@ def _reject_url_control_chars(value: str, field: str) -> None: CR/LF in ``study_uid`` would let it split the request line; urllib would reject it with a bare ``ValueError`` at send. Surface it as a clear construction-time ``ValueError`` (caught at ``check``/dry-run as a ``WiringError``) — PHI-safe (names the field, never the value).""" - if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in value): + if has_control_char(value): raise ValueError(f"DICOMweb {field} contains an illegal control character") diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index 7293e95c..ac9b0cdd 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -47,6 +47,7 @@ from typing import Any from messagefoundry.config.models import ConnectorType, Destination +from messagefoundry.controlchars import has_control_char from messagefoundry.parsing.fhir import FhirPeek, FhirPeekError from messagefoundry.transports.base import ( DeliveryError, @@ -175,7 +176,7 @@ def _reject_control_chars(value: str, field: str) -> str: ``ValueError`` that would otherwise escape ``send()`` as an 'internal error'. Surface it as a permanent ``NegativeAckError`` (a retry re-sends the same body) with a PHI-safe message (the field name only, never the value).""" - if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in value): + if has_control_char(value): raise NegativeAckError( f"FHIR {field} contains an illegal control character", code="bad-request-value", @@ -198,7 +199,7 @@ def _reject_config_control_chars(value: str, setting: str, where: str = "destina and the read executor -- and #1241's subject is precisely the asymmetry of screening one and not its sibling. """ - if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in value): + if has_control_char(value): raise ValueError(f"FHIR {where} {setting!r} contains an illegal control character") return value diff --git a/messagefoundry/transports/remotefile.py b/messagefoundry/transports/remotefile.py index d1b2e9cd..ca9ced8b 100644 --- a/messagefoundry/transports/remotefile.py +++ b/messagefoundry/transports/remotefile.py @@ -64,6 +64,7 @@ relax_verify_expiry, resolve_trust_anchor, ) +from messagefoundry.controlchars import has_control_char from messagefoundry.transports.base import ( DeliveryError, DestinationConnector, @@ -118,7 +119,7 @@ def _is_contained_name(name: object) -> bool: return False if "/" in name or "\\" in name: return False - if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in name): + if has_control_char(name): return False # A drive-relative path ("C:x.hl7") resolves against the drive's CWD on Windows and contains no # separator, so the checks above cannot see it. Two chars, ASCII letter, then a colon. diff --git a/messagefoundry/transports/rest.py b/messagefoundry/transports/rest.py index ea1f8dd9..d6fbf7ad 100644 --- a/messagefoundry/transports/rest.py +++ b/messagefoundry/transports/rest.py @@ -54,6 +54,7 @@ is_loopback_hop_host, relax_verify_expiry, ) +from messagefoundry.controlchars import strip_control_chars from messagefoundry.transports.base import ( DeliveryError, DeliveryResponse, @@ -108,7 +109,7 @@ def _strip_header_control_chars(value: str) -> str: """Neutralize a message-derived header VALUE (header-injection safety, #68): strip every C0 control (< 0x20 — incl. CR/LF) and DEL (0x7F) so the value can never split the request line or inject an extra header. Returns the value with those bytes removed (a single, safe header value).""" - return "".join(ch for ch in value if not (ord(ch) < 0x20 or ord(ch) == 0x7F)) + return strip_control_chars(value) # --- captured HTTP response headers (BACKLOG #154, ADR 0013 amendment) ----------------------------- diff --git a/tests/test_controlchars.py b/tests/test_controlchars.py new file mode 100644 index 00000000..99428e4c --- /dev/null +++ b/tests/test_controlchars.py @@ -0,0 +1,87 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The shared C0/DEL predicate (BACKLOG #1253). + +#1239 asked for a test that "the two predicates agree across a shared character corpus, so a future +widening of one without the other fails". There is now ONE predicate, so that obligation becomes a +CHARACTERISATION test: pin the exact code-point set, over the whole of Latin-1 plus the neighbours +that tempt a widener, so a change to the shared definition has to be deliberate and cannot ride in +as a tidy-up. Seven call sites move together now -- that is the leverage and also the risk. +""" + +from __future__ import annotations + +import pytest + +from messagefoundry.controlchars import has_control_char, strip_control_chars + +#: The set the predicate is defined to catch. Written independently of the implementation, so this +#: is a second opinion rather than a restatement of the same expression. +_CONTROL = frozenset(chr(c) for c in range(0x00, 0x20)) | {chr(0x7F)} + + +@pytest.mark.parametrize("code", sorted(ord(c) for c in _CONTROL)) +def test_every_c0_control_and_del_is_caught(code: int) -> None: + assert has_control_char(f"a{chr(code)}b") is True + + +def test_the_predicate_matches_its_definition_across_all_of_latin1_and_beyond() -> None: + """The characterisation. Any divergence here is a deliberate widening or a mistake, and either + way it must not pass silently -- seven call sites share this now.""" + caught = {chr(c) for c in range(0x0000, 0x0300) if has_control_char(chr(c))} + assert caught == set(_CONTROL) + + +def test_ordinary_text_is_not_flagged() -> None: + assert has_control_char("") is False + assert has_control_char("a normal value") is False + assert has_control_char("punctuation!@#$%^&*()-_=+[]{};:'\",.<>/?\\|`~") is False + + +def test_the_boundaries_are_where_they_are_documented() -> None: + """0x1F in, 0x20 out; 0x7E out, 0x7F in, 0x80 out. The off-by-one at each edge.""" + assert has_control_char(chr(0x1F)) is True + assert has_control_char(chr(0x20)) is False # space + assert has_control_char(chr(0x7E)) is False # tilde + assert has_control_char(chr(0x7F)) is True # DEL + assert has_control_char(chr(0x80)) is False # C1 starts here and is NOT covered + + +@pytest.mark.parametrize("code", [0x85, 0x9B, 0x2028, 0x2029, 0x200B, 0xFEFF]) +def test_c1_and_unicode_separators_are_deliberately_NOT_caught(code: int) -> None: + """Documented as deliberate, and pinned so nobody "fixes" it by accident. Every call site + screens values bound for byte-oriented sinks where C0 and DEL are the injection alphabet. + Widening this is a behaviour change at seven sites at once and must be made on purpose.""" + assert has_control_char(chr(code)) is False + + +# --- the two actions stay two actions --------------------------------------------------------- + + +def test_strip_removes_exactly_what_the_predicate_catches() -> None: + noisy = "".join(sorted(_CONTROL)) + "keep me" + assert strip_control_chars(noisy) == "keep me" + assert has_control_char(strip_control_chars(noisy)) is False + + +def test_strip_is_a_no_op_on_clean_text() -> None: + assert strip_control_chars("nothing to remove") == "nothing to remove" + + +def test_strip_preserves_order_and_the_rest_of_the_value() -> None: + assert strip_control_chars("a\rb\nc\td") == "abcd" + + +def test_the_two_actions_disagree_on_purpose() -> None: + """A regression that turned the strip into a reject (or vice versa) would show up here. #1253 + requires both arms to survive: six sites refuse, rest.py's header-VALUE path neutralises.""" + hostile = "value\r\nX-Injected: 1" + assert has_control_char(hostile) is True + assert strip_control_chars(hostile) == "valueX-Injected: 1" + assert has_control_char(strip_control_chars(hostile)) is False + + +def test_the_strip_defeats_header_injection_which_is_why_it_exists() -> None: + """CRLF is the whole point: a stripped value can no longer split a request line.""" + assert "\r" not in strip_control_chars("a\rb") + assert "\n" not in strip_control_chars("a\nb") From 15e6039eac86372e4a2b7fd95735ef940cc78415 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 14 Aug 2026 08:30:54 -0500 Subject: [PATCH 9/9] fix(tooling): the dangling-citation detector can now fail, and the suite runs it (BACKLOG #1235) #1235 was correctly ruled PARTIAL after PR #385: the detector shipped, but it was wired into nothing and could not fail. Two of that ruling's three measurements are addressed here. The third -- the written RULE -- is a convention change and is not mine to make. IT COULD NOT FAIL, AND THAT IS THE DEFECT THIS TOOL EXISTS TO CATCH ELSEWHERE. `--fail` was opt-in and nothing passed it, so a planted dangling citation was reported correctly AND the process exited 0. A checker that cannot fail is not a check. The default is now fail-closed with `--advisory` as the escape; flipping it cost nothing, because repo-wide the script was referenced by exactly two lines, both inside its own unit test. THE EXIT CODE KEYS ON THE LIVE SHAPE, NOT THE HIT COUNT. A number at or below the allocator's floor can never be issued, and a PR/issue/foreign-repo reference is not a backlog citation at all. Both are still REPORTED for a human to read, and neither reds the tree: failing on them would red it today for hits that are correct, and a gate that cries wolf gets switched off. THE SUITE IS NOW THE CALLER. A test walks the real docs/ tree and asserts no citation names a still-issuable number. Measured on origin/main before writing it: 312 markdown files, 6 above-floor hits, ALL foreign-repo shaped, ZERO live-shape -- so the gate passes today on merit rather than by being lenient. PROVED IT CAN FAIL, which is the whole point of the item. A live-shape citation was planted in docs/, the test ran, and it failed naming the file, line and number; the plant was removed in a finally block and the run without it passes. A gate whose failing arm has never been observed is the state this item is about. The population walk is pinned too (>200 files asserted, not merely printed): a walk that collapses to nothing would otherwise report clean forever. --- scripts/docs/dangling_citation_check.py | 23 +++++++++++- tests/test_dangling_citation_check.py | 50 +++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/scripts/docs/dangling_citation_check.py b/scripts/docs/dangling_citation_check.py index 067bd888..1aee0144 100644 --- a/scripts/docs/dangling_citation_check.py +++ b/scripts/docs/dangling_citation_check.py @@ -204,7 +204,9 @@ def main(argv: list[str] | None = None) -> int: ) parser.add_argument("paths", nargs="*", type=Path, help="files to scan (default: docs/**/*.md)") parser.add_argument( - "--fail", action="store_true", help="exit non-zero when any citation is unresolved" + "--advisory", + action="store_true", + help="report and exit 0 even when a live-shape citation is found (default: exit 1)", ) args = parser.parse_args(argv) @@ -263,7 +265,24 @@ def main(argv: list[str] | None = None) -> int: print( "Not scanned: the private companion repository, where a citation is invisible to this repo." ) - return 1 if args.fail else 0 + # FAIL CLOSED, ON THE LIVE SHAPE ONLY (BACKLOG #1235). The first version of this took `--fail` + # as opt-in and nothing passed it, so a planted dangling citation was reported correctly AND the + # process still exited 0 -- a checker that cannot fail is not a check, which is the defect this + # tool exists to catch in other people's gates. Flipping the default cost nothing: repo-wide the + # script was referenced by two lines, both inside its own unit test. + # + # The exit code keys on the LIVE shape, not on the hit count. A number at or below the floor can + # never be issued, and a PR/issue/foreign-repo reference is not a backlog citation at all; both + # are reported for a human to read and neither is a defect. Failing on them would red the tree + # today for hits that are correct, and a gate that cries wolf gets switched off. + live = [h for h in hits if h.number > floor and not h.pr_shaped] + if live: + print() + print(f"LIVE SHAPE: {len(live)} citation(s) name a number that can still be issued.") + for h in live: + print(f" {h.path}:{h.lineno}: #{h.number}") + print("Allocate the number before citing it, or write the reference so it CANNOT resolve.") + return 1 if (live and not args.advisory) else 0 if __name__ == "__main__": diff --git a/tests/test_dangling_citation_check.py b/tests/test_dangling_citation_check.py index 06022e16..42d00b26 100644 --- a/tests/test_dangling_citation_check.py +++ b/tests/test_dangling_citation_check.py @@ -174,3 +174,53 @@ def test_the_floor_is_conservative_never_optimistic(tmp_path: pathlib.Path) -> N ledger = tmp_path / "L.md" ledger.write_text("## 1500. an item\n\n> open\n", encoding="utf-8") assert cc.allocation_floor([ledger]) == 1500 + + +# --- THE GATE ITSELF, run over the real tree (BACKLOG #1235) --------------------------------------- +# +# The detector shipped in PR #385 wired into NOTHING: repo-wide it was referenced by two lines, both +# inside this file, and its CLI exited 0 even when it reported a hit. A detector nobody invokes and +# that cannot fail is not a gate. These give it the failing arm and make the suite the caller. + + +def _repo_root() -> Path: + return Path(__file__).resolve().parents[1] + + +def _live_shape_citations() -> list[tuple[str, int, int]]: + """Citations naming a number ABOVE the floor that are not PR/foreign-repo shaped. + + Below the floor is unreachable forever, and a foreign reference is not a backlog citation at + all; both are reported by the tool for a human to read and neither is a defect. + """ + root = _repo_root() + floor = cc.allocation_floor() + filed = cc.allocated_numbers() + out: list[tuple[str, int, int]] = [] + for path in sorted((root / "docs").rglob("*.md")): + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + for lineno, number, _line, pr_shaped in cc.citations_in(text): + if number in filed or number <= floor or pr_shaped: + continue + out.append((str(path.relative_to(root)), lineno, number)) + return out + + +def test_the_docs_scan_actually_covers_something() -> None: + """PRINT AND PIN THE POPULATION. A walk that collapses to nothing reports clean forever, which + is the exact failure this whole item is about.""" + found = sorted((_repo_root() / "docs").rglob("*.md")) + print(f"scanned {len(found)} markdown files under docs/") + assert len(found) > 200, f"only {len(found)} docs found -- the walk is not finding them" + + +def test_no_docs_citation_names_a_number_that_can_still_be_issued() -> None: + """THE GATE. A citation to an unissued number is harmless until someone files that number, at + which point it silently starts naming unrelated work. Catch it while it is still honest.""" + live = _live_shape_citations() + assert not live, "citations naming a still-issuable number:\n " + "\n ".join( + f"{p}:{n} #{num}" for p, n, num in live + )