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 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/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/scripts/docs/dangling_citation_check.py b/scripts/docs/dangling_citation_check.py new file mode 100644 index 00000000..1aee0144 --- /dev/null +++ b/scripts/docs/dangling_citation_check.py @@ -0,0 +1,289 @@ +# 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 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 + + +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( + "--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) + + # 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." + ) + # 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__": + sys.exit(main()) 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_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") diff --git a/tests/test_cp1252_console_safety.py b/tests/test_cp1252_console_safety.py new file mode 100644 index 00000000..589fab7e --- /dev/null +++ b/tests/test_cp1252_console_safety.py @@ -0,0 +1,201 @@ +# 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. + +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 + 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) diff --git a/tests/test_dangling_citation_check.py b/tests/test_dangling_citation_check.py new file mode 100644 index 00000000..42d00b26 --- /dev/null +++ b/tests/test_dangling_citation_check.py @@ -0,0 +1,226 @@ +# 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 + + +# --- 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 + ) 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"} 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"