From f836299b86c9d919e30c441ada61832839a0a0b2 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 16 Sep 2026 14:51:51 +0530 Subject: [PATCH 1/2] Report pins a branch drops and modules it unseals, without refusing either bridge#416's second decision: report, and let a person decide. Rebuilt on master after #434's squash, and after two reviews. scripts/surface_coverage_report.py runs after every successful reseal.sh (not --verify) and never fails it. Against the merge-base with origin/master it lists a dropped pin, and a module declared directly by a pinned module and newly left unpinned -- a new file, a newly attached file, or one pinned at the base. What it does not check is stated where it will be read: in its all-clear line, its docstring, docs/release-process.md and the reseal.sh comment. Review showed the first wording ("code moved between existing files") named one blind spot and implied it was the only one. It also misses a new module under an unpinned parent even when carved out of a pinned file, deeper descendants, a pinned file that stops being compiled, a test-only or feature-gated module becoming production, and a new crate root. Module resolution follows rustc, walked from crate roots: a bare `mod` resolves beside a file that owns its directory (crate root, mod.rs, or declaring file. Review compared file sets, not just counts, with rustc's dep-info for every crate here and found them identical. The docstring lists the forms it does not handle, none of which occurs in the repo. Relative --surface paths now resolve against the caller's directory, and paths are compared as real paths: on macOS /tmp and /var are symlinks, and the first version reported a surface under a symlinked root as outside the repository. 32 tests, run against 34 mutations -- the 19 from the first version, and 15 more including every survivor review found -- and all 34 fail a test. Six needed the tests strengthened first: #[path] in an ordinary file (the shape agent.rs uses for nearly every agent_*.rs), inheritance of test-only and feature gates through a file, restricted visibility with a cfg, the new-file and pinned-at-base report clauses, a relative surface from a subdirectory, and an escaped quote in a char literal. docs/module-decomposition.md now points at the report and says what it does not catch. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 1 + docs/module-decomposition.md | 6 +- docs/release-process.md | 23 + .../compatibility/compatibility-matrix.json | 2 +- .../compatibility/compatibility-surface.json | 4 +- scripts/reseal.sh | 5 + scripts/surface_coverage_report.py | 439 ++++++++++++++++++ scripts/surface_coverage_report.test.py | 381 +++++++++++++++ 8 files changed, 856 insertions(+), 5 deletions(-) create mode 100644 scripts/surface_coverage_report.py create mode 100644 scripts/surface_coverage_report.test.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5aecc211..ef4cf94b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -148,6 +148,7 @@ jobs: - run: python3 scripts/bank_statement_import.test.py - run: python3 scripts/sanitise-bbox-capture.test.py - run: python3 scripts/merge-gate.test.py + - run: python3 scripts/surface_coverage_report.test.py tally-portable: name: Tally portable core diff --git a/docs/module-decomposition.md b/docs/module-decomposition.md index 20d5cd9a..82c4eca3 100644 --- a/docs/module-decomposition.md +++ b/docs/module-decomposition.md @@ -57,8 +57,10 @@ reconcile to the pinned `agent_desktop_journal.rs`. **The gate will not tell you when a split leaves code unsealed.** A moved module's parent changes hash, the reseal succeeds, and the extracted file is -outside the seal with every check green. Check the new files against the rule -above yourself. +outside the seal with every check green. `scripts/reseal.sh` prints a report +that catches the common case -- a new module declared directly by a pinned one +-- but not a new module under an unpinned parent or deeper descendants, so check +the new files against the rule above yourself. **Capacity is not free, and that is deliberate.** `MAX_SURFACE_FILES` is set to the exact pin count, so any branch adding a pin raises diff --git a/docs/release-process.md b/docs/release-process.md index 7ba8aa86..6c5eccdc 100644 --- a/docs/release-process.md +++ b/docs/release-process.md @@ -112,6 +112,29 @@ new digest was computed from disk, and the `rehash-surface` that follows re-reads every pin, including the new one, before the second seal. Never stop after that first seal. +#### What the reseal reports after it succeeds + +A successful `scripts/reseal.sh` (not `--verify`) prints a surface coverage +report from `scripts/surface_coverage_report.py`. It never fails the reseal. +Against the merge-base with `origin/master` (override with +`SURFACE_REPORT_BASE`) it lists two things the gate cannot see: a pin that was +dropped, and a module declared directly by a pinned module and newly left +unpinned. Modules left unpinned before the branch are not reprinted, test-only +modules are only counted, and feature-gated ones are labelled. + +**A clean report is not evidence that nothing left the seal.** It does not see +code moved between files that already existed; a new module declared by an +*unpinned* module, even one carved out of a pinned file (a new file under an +unpinned `db/mod.rs`, say); deeper descendants of a pinned module; a pinned +file that stops being compiled; a test-only or feature-gated module becoming +production; or a new crate root. The script's docstring keeps the full list. +The merge driver (`scripts/reseal-merge-driver.mjs`) calls the tool directly and +does not print the report; run `scripts/reseal.sh` after resolving. + +Read it, then pin each listed file that decides what Bridge posts or lets leave +the machine, and leave the rest; see the comment on `MAX_SURFACE_FILES` for the +rule and bridge#416 for the reasoning. + #### When the surface itself conflicts in a merge or rebase The surface and the matrix are **generated artifacts**. Never hand-merge them. diff --git a/docs/tally/compatibility/compatibility-matrix.json b/docs/tally/compatibility/compatibility-matrix.json index ffc61449..b56fe096 100644 --- a/docs/tally/compatibility/compatibility-matrix.json +++ b/docs/tally/compatibility/compatibility-matrix.json @@ -1,7 +1,7 @@ { "schema_version": 1, "bridge_commit_sha": "be1c20cc3fd66fa1ece196505c69f26e555e4b8e", - "compatibility_surface_sha256": "17dcd4722234ab278f55d130b00151b71ae797f6a1a7cf18246f08ab82f21915", + "compatibility_surface_sha256": "d8735ad48170a115c0504ec053ca1669e72c539c621aadfe196228c3921435b4", "claims": [ { "claim_id": "erp9-6-6-3-windows-education-xml-one-company", diff --git a/docs/tally/compatibility/compatibility-surface.json b/docs/tally/compatibility/compatibility-surface.json index 088f022f..ba4791bb 100644 --- a/docs/tally/compatibility/compatibility-surface.json +++ b/docs/tally/compatibility/compatibility-surface.json @@ -3,7 +3,7 @@ "files": [ { "path": ".github/workflows/ci.yml", - "sha256": "c986297046d65dc1d218583410e693c9d017e1d72e3891d667012c8474db2ed1" + "sha256": "4a116d6db823d8e01ae1f7232e9ddbb845aeb4a0f581cb463983724f6d5c017b" }, { "path": ".github/workflows/dependency-security-scheduled.yml", @@ -954,5 +954,5 @@ "sha256": "a8ac2714fecf51947f2822c8c46d7ce2e8602c732780ff60566a7771f0836f9a" } ], - "manifest_sha256": "17dcd4722234ab278f55d130b00151b71ae797f6a1a7cf18246f08ab82f21915" + "manifest_sha256": "d8735ad48170a115c0504ec053ca1669e72c539c621aadfe196228c3921435b4" } \ No newline at end of file diff --git a/scripts/reseal.sh b/scripts/reseal.sh index dba517eb..9fed152d 100755 --- a/scripts/reseal.sh +++ b/scripts/reseal.sh @@ -198,3 +198,8 @@ pins_changed="no" [ "$MODE" = "pins-changed" ] && pins_changed="yes" reseal_into "$SURFACE" "$MATRIX" "$pins_changed" echo "reseal.sh: compatibility surface and matrix resealed" + +# Report-only, never fails the reseal: pins this branch dropped, and modules +# newly left unpinned directly under a pinned one, since origin/master -- two +# things the gate cannot see. Much else is not checked; see the script (#416). +python3 "$SCRIPT_DIR/surface_coverage_report.py" --root "$PIN_ROOT" --surface "$SURFACE" || true diff --git a/scripts/surface_coverage_report.py b/scripts/surface_coverage_report.py new file mode 100644 index 00000000..76673843 --- /dev/null +++ b/scripts/surface_coverage_report.py @@ -0,0 +1,439 @@ +#!/usr/bin/env python3 +"""Report two ways code leaves the sealed compatibility surface. Never fails. + +Checked (bridge#416), comparing the working tree with the merge-base against a +base ref (default `origin/master`): + +1. A dropped pin. `rehash-surface` never adds paths, so a conflict resolved by + taking the base side of `compatibility-surface.json` loses entries a branch + added, and the gate still passes. +2. A module declared *directly* by a pinned module and left unpinned, where it + is a new file, a file newly attached to that pinned module, or a file that + was pinned at the base. + +Not checked -- a clean report is not evidence that nothing left the seal: + +- code moved between files that already existed, in any direction; +- a module declared by an unpinned module, even when its code came out of a + pinned file (e.g. a new file under an unpinned `db/mod.rs`), and any deeper + descendant of a pinned module; +- a pinned file that stops being compiled because its declaration was removed; +- a module that was test-only or feature-gated at the base becoming production; +- a new crate root. + +Only what is new on the branch is printed, so modules left unpinned before it +are not reprinted on every reseal. It decides nothing: a person reads it and +either pins the file or leaves it. + +The module graph follows rustc's rules for out-of-line modules, walked from +each crate root: a bare `mod name;` resolves beside a file that owns its +directory -- a crate root, a `mod.rs`, or a file loaded through `#[path]` -- and +under `/` otherwise; `#[path]` resolves beside the declaring file. When +checked for bridge#436 it reached exactly the file sets rustc compiled for each +crate in this repository, across the build profiles compared. It does +not handle `#[cfg_attr(..., path = ...)]`, `mod r#name;`, `include!`, +macro-generated modules, `#[path]` on an inline module, raw-string `#[path]` +values or inner `#![cfg]`; none occurs here today. A `cfg` it cannot evaluate +leaves the module classified as production, which can add noise but does not +hide a module. +""" +from __future__ import annotations + +import argparse +import json +import os +import re +import subprocess +import sys +from pathlib import PurePosixPath +from typing import NamedTuple + +IDENT = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +MOD_ITEM = re.compile( + r"(?:pub(?:\s*\([^)]*\))?\s+)?mod\s+(?P[A-Za-z_][A-Za-z0-9_]*)\s*(?P[;{])" +) +CRATE_TARGET_DIRS = {"tests", "benches", "examples"} +RAW_STRING = re.compile(r'b?r(#*)"') +CHAR_LITERAL = re.compile(r"b?'(?:\\(?:x[0-9A-Fa-f]{2}|u\{[0-9A-Fa-f]+\}|.)|[^\\'\n])'") + + +def git(root: str, *args: str) -> str: + return subprocess.run( + ["git", "-C", root, *args], check=True, capture_output=True, text=True + ).stdout + + +class Tree: + """File access over the working tree, or over one git revision.""" + + def __init__(self, root: str, rev: str | None = None): + self.root, self.rev = root, rev + if rev is None: + listed = git(root, "ls-files", "--cached", "--others", "--exclude-standard") + self.paths = {p for p in listed.splitlines() if os.path.isfile(os.path.join(root, p))} + else: + self.paths = set(git(root, "ls-tree", "-r", "--name-only", rev).splitlines()) + + def exists(self, path: str) -> bool: + return path in self.paths + + def read(self, path: str) -> str: + if self.rev is None: + with open(os.path.join(self.root, path), encoding="utf-8", errors="replace") as handle: + return handle.read() + return git(self.root, "show", f"{self.rev}:{path}") + + +# --- lexing --------------------------------------------------------------- + + +def skip_string(text: str, i: int) -> int: + """Index just past a string or char literal starting at `i`, else `i`.""" + raw = RAW_STRING.match(text, i) + if raw: + closing = '"' + raw.group(1) + end = text.find(closing, raw.end()) + return len(text) if end < 0 else end + len(closing) + if text.startswith('"', i) or text.startswith('b"', i): + j = i + (2 if text[i] == "b" else 1) + while j < len(text): + if text[j] == "\\": + j += 2 + elif text[j] == '"': + return j + 1 + else: + j += 1 + return len(text) + char = CHAR_LITERAL.match(text, i) + if char: + return char.end() + return i + + +def strip_comments(source: str) -> str: + """Blank out comments, leaving string literals intact and positions stable.""" + out, i = [], 0 + while i < len(source): + end = skip_string(source, i) + if end != i: + out.append(source[i:end]) + i = end + elif source.startswith("//", i): + end = source.find("\n", i) + end = len(source) if end < 0 else end + out.append(" " * (end - i)) + i = end + elif source.startswith("/*", i): + depth, j = 1, i + 2 + while j < len(source) and depth: + if source.startswith("/*", j): + depth, j = depth + 1, j + 2 + elif source.startswith("*/", j): + depth, j = depth - 1, j + 2 + else: + j += 1 + out.append(re.sub(r"[^\n]", " ", source[i:j])) + i = j + else: + out.append(source[i]) + i += 1 + return "".join(out) + + +def read_balanced(text: str, i: int, open_: str, close: str) -> int: + """Index just past the bracket group opened at `text[i] == open_`.""" + depth, j = 0, i + while j < len(text): + end = skip_string(text, j) + if end != j: + j = end + continue + if text[j] == open_: + depth += 1 + elif text[j] == close: + depth -= 1 + if depth == 0: + return j + 1 + j += 1 + return len(text) + + +class Declaration(NamedTuple): + name: str + attrs: tuple[str, ...] + inline: tuple[str, ...] + inline_attrs: tuple[str, ...] + + +def declarations(source: str) -> list[Declaration]: + """Out-of-line `mod name;` items, with their outer attributes and the inline + modules they sit inside.""" + text = strip_comments(source) + found, attrs, inline, depth = [], [], [], 0 + i = 0 + while i < len(text): + ch = text[i] + if ch.isspace(): + i += 1 + continue + end = skip_string(text, i) + if end != i: + attrs, i = [], end + continue + if text.startswith("#![", i): + i = read_balanced(text, i + 2, "[", "]") + continue + if text.startswith("#[", i): + close = read_balanced(text, i + 1, "[", "]") + attrs.append(text[i + 2 : close - 1]) + i = close + continue + item = MOD_ITEM.match(text, i) + if item and (i == 0 or not (text[i - 1].isalnum() or text[i - 1] == "_")): + if item.group("end") == ";": + found.append(Declaration( + item.group("name"), tuple(attrs), + tuple(n for n, _, _ in inline), tuple(a for _, _, xs in inline for a in xs))) + else: + depth += 1 + inline.append((item.group("name"), depth, tuple(attrs))) + attrs, i = [], item.end() + continue + if ch == "{": + depth += 1 + elif ch == "}": + while inline and inline[-1][1] == depth: + inline.pop() + depth -= 1 + word = IDENT.match(text, i) + attrs = [] + i = word.end() if word else i + 1 + return found + + +# --- cfg ------------------------------------------------------------------ + + +def parse_cfg(text: str): + """`cfg(...)` body -> nested tuples: ('name', x) | ('kv', k) | (op, [..]).""" + pos = 0 + + def ws(): + nonlocal pos + while pos < len(text) and text[pos].isspace(): + pos += 1 + + def predicate(): + nonlocal pos + ws() + word = IDENT.match(text, pos) + if not word: + raise ValueError(text) + name, pos = word.group(0), word.end() + ws() + if pos < len(text) and text[pos] == "(": + pos += 1 + items = [] + ws() + while pos < len(text) and text[pos] != ")": + items.append(predicate()) + ws() + if pos < len(text) and text[pos] == ",": + pos += 1 + ws() + pos += 1 + return (name, items) + if pos < len(text) and text[pos] == "=": + pos += 1 + ws() + pos = skip_string(text, pos) + return ("kv", name) + return ("name", name) + + return predicate() + + +def implies(pred, test) -> bool: + kind, value = pred + if kind == "all": + return any(implies(p, test) for p in value) + if kind == "any": + return bool(value) and all(implies(p, test) for p in value) + if kind in ("not",) or not isinstance(value, str): + return False + return test(kind, value) + + +def cfg_predicates(attrs) -> list: + preds = [] + for attr in attrs: + match = re.match(r"\s*cfg\s*\(", attr) + if match: + try: + preds.append(parse_cfg(attr[match.end() : attr.rindex(")")])) + except ValueError: + pass + return preds + + +def path_attr(attrs) -> str | None: + for attr in attrs: + match = re.match(r'\s*path\s*=\s*"([^"]*)"\s*$', attr) + if match: + return match.group(1) + return None + + +# --- module graph --------------------------------------------------------- + + +class Module(NamedTuple): + path: str + parent: str | None + test_only: bool + feature_gated: bool + + +def crate_roots(tree: Tree) -> list[str]: + """Files cargo compiles as crate roots, which own their directory.""" + roots = set() + for manifest in sorted(p for p in tree.paths if PurePosixPath(p).name == "Cargo.toml"): + crate = PurePosixPath(manifest).parent + prefix = "" if str(crate) == "." else f"{crate}/" + for default in ("src/lib.rs", "src/main.rs", "build.rs"): + if tree.exists(prefix + default): + roots.add(prefix + default) + for path in tree.paths: + if not (path.startswith(prefix) and path.endswith(".rs")): + continue + rel = PurePosixPath(path[len(prefix):]).parts + if rel[:2] == ("src", "bin") and (len(rel) == 3 or (len(rel) == 4 and rel[3] == "main.rs")): + roots.add(path) + elif rel[0] in CRATE_TARGET_DIRS and (len(rel) == 2 or (len(rel) == 3 and rel[2] == "main.rs")): + roots.add(path) + for value in re.findall(r'^\s*path\s*=\s*"([^"]+\.rs)"', tree.read(manifest), re.M): + candidate = os.path.normpath(prefix + value) + if tree.exists(candidate): + roots.add(candidate) + return sorted(roots) + + +def module_graph(tree: Tree) -> dict[str, Module]: + """Every module file reachable from a crate root, keyed by path.""" + modules: dict[str, Module] = {} + stack = [(root, None, True, False, False) for root in crate_roots(tree)] + while stack: + path, parent, owns_dir, test_only, gated = stack.pop() + seen = modules.get(path) + if not tree.exists(path) or (seen and (seen.test_only <= test_only)): + continue + modules[path] = Module(path, parent, test_only, gated) + here = PurePosixPath(path).parent + base = here if owns_dir else here / PurePosixPath(path).stem + for decl in declarations(tree.read(path)): + preds = cfg_predicates(decl.inline_attrs + decl.attrs) + child_test = test_only or any(implies(p, lambda k, v: k == "name" and v == "test") for p in preds) + child_gated = gated or any(implies(p, lambda k, v: k == "kv" and v == "feature") for p in preds) + explicit = path_attr(decl.attrs) + if explicit is not None: + start = here if not decl.inline else base.joinpath(*decl.inline) + candidates, child_owns = [start / explicit], True + else: + nested = base.joinpath(*decl.inline) + candidates = [nested / f"{decl.name}.rs", nested / decl.name / "mod.rs"] + child_owns = None + for candidate in candidates: + child = os.path.normpath(str(candidate)) + if tree.exists(child): + owns = child_owns if child_owns is not None else PurePosixPath(child).name == "mod.rs" + stack.append((child, path, owns, child_test, child_gated)) + break + return modules + + +def surface_paths(tree: Tree, surface: str) -> set[str]: + return {entry["path"] for entry in json.loads(tree.read(surface))["files"]} + + +def unpinned_under_pinned(tree: Tree, surface: str) -> dict[str, Module]: + pinned = surface_paths(tree, surface) + return { + path: module + for path, module in module_graph(tree).items() + if module.parent in pinned and path not in pinned + } + + +# --- report --------------------------------------------------------------- + + +def report(root: str, surface: str, base_ref: str) -> list[str]: + try: + base = git(root, "merge-base", "HEAD", base_ref).strip() + except (subprocess.CalledProcessError, FileNotFoundError): + return [f"surface coverage: no merge-base with {base_ref}; nothing compared"] + now, then = Tree(root), Tree(root, base) + if not then.exists(surface): + return [f"surface coverage: {surface} absent at merge-base; nothing compared"] + + pinned_then = surface_paths(then, surface) + before = unpinned_under_pinned(then, surface) + lines = [f" pin removed: {p}" for p in sorted(pinned_then - surface_paths(now, surface))] + # Newly unsealed under a pinned module: not already there at the base, and + # either its parent was pinned at the base (so attaching it moved something + # out), or it is a new file, or it was itself pinned at the base. An + # existing unpinned file whose parent this branch pinned did not move -- the + # seal grew around it. + added = { + path: module + for path, module in unpinned_under_pinned(now, surface).items() + if path not in before + and (module.parent in pinned_then or not then.exists(path) or path in pinned_then) + } + for path, module in sorted(added.items()): + if not module.test_only: + gated = ", behind a cargo feature" if module.feature_gated else "" + lines.append(f" newly unsealed module: {path} (declared by pinned {module.parent}{gated})") + tests = sum(1 for module in added.values() if module.test_only) + if tests: + lines.append(f" newly unsealed test-only modules: {tests} (not listed; unpinned tests are the norm)") + if not lines: + return [ + f"surface coverage: no dropped pins and no newly unsealed direct children of" + f" pinned modules since {base_ref} (not a proof nothing left the seal; see" + " scripts/surface_coverage_report.py)" + ] + return [ + f"surface coverage: since {base_ref}, this branch dropped pins or unsealed modules.", + " Nothing is refused. Pin each file that decides what Bridge posts or lets leave", + " the machine; leave the rest. See bridge#416 and the comment on MAX_SURFACE_FILES.", + *lines, + ] + + +def main(argv: list[str]) -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--root", default=os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + parser.add_argument("--surface", default="docs/tally/compatibility/compatibility-surface.json") + parser.add_argument("--base", default=os.environ.get("SURFACE_REPORT_BASE", "origin/master")) + args = parser.parse_args(argv) + try: + surface = args.surface + if not os.path.isabs(surface) and os.path.exists(surface): + surface = os.path.abspath(surface) # relative to the caller's directory + if os.path.isabs(surface): + # Compare real paths: on macOS /tmp and /var are symlinks, so an + # absolute path built from the working directory and a root given + # through the symlink would otherwise look unrelated. + surface = os.path.relpath(os.path.realpath(surface), os.path.realpath(args.root)) + if surface.startswith(".."): + print("surface coverage: surface is outside the repository; nothing compared") + else: + print("\n".join(report(args.root, surface, args.base))) + except Exception as error: # a report must never block a reseal + print(f"surface coverage: report unavailable ({type(error).__name__}: {error})") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/surface_coverage_report.test.py b/scripts/surface_coverage_report.test.py new file mode 100644 index 00000000..74191bd9 --- /dev/null +++ b/scripts/surface_coverage_report.test.py @@ -0,0 +1,381 @@ +#!/usr/bin/env python3 +"""Offline controls for scripts/surface_coverage_report.py. + +Each test builds a throwaway git repository where `main` holds the base, the +change is committed on a `work` branch (and sometimes left uncommitted on top), +and the report is run against `main`. Committing the change -- rather than +leaving HEAD at the base -- is what lets these tests tell the merge-base from +HEAD or from the tip of `main`. + +The module graph itself was checked against rustc rather than asserted: on +src-tauri's lib, bridge-tally-protocol and bridge-tally-core it reproduces the +file counts rustc compiled under default, all-features, test and +test-plus-all-features builds (bridge#416). +""" +from __future__ import annotations + +import contextlib +import importlib.util +import io +import json +import subprocess +import tempfile +import unittest +from pathlib import Path + +SCRIPT = Path(__file__).resolve().parent / "surface_coverage_report.py" +spec = importlib.util.spec_from_file_location("surface_coverage_report", SCRIPT) +report_module = importlib.util.module_from_spec(spec) +spec.loader.exec_module(report_module) + +CLEAN = ("surface coverage: no dropped pins and no newly unsealed direct children of" + " pinned modules since main (not a proof nothing left the seal; see" + " scripts/surface_coverage_report.py)") +CARGO = '[package]\nname = "fixture"\nversion = "0.0.0"\n' + + +class Repo: + def __init__(self, root: str): + self.root = root + self.git("init", "-q", "-b", "main") + self.git("config", "user.email", "test@example.invalid") + self.git("config", "user.name", "test") + + def git(self, *args: str) -> str: + return subprocess.run(["git", "-C", self.root, *args], check=True, + capture_output=True, text=True).stdout + + def write(self, files: dict[str, str | None]) -> None: + for path, content in files.items(): + target = Path(self.root, path) + if content is None: + target.unlink() + else: + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + + def pin(self, *paths: str) -> None: + entries = [{"path": p, "sha256": "0" * 64} for p in sorted(paths)] + self.write({"surface.json": json.dumps({"files": entries})}) + + def commit(self, message: str) -> None: + self.git("add", "-A") + self.git("commit", "-q", "--allow-empty", "-m", message) + + def report(self) -> list[str]: + return report_module.report(self.root, "surface.json", "main") + + def newly_unsealed(self) -> set[str]: + return {line.split()[3] for line in self.report() if "newly unsealed module:" in line} + + +class SurfaceCoverageReport(unittest.TestCase): + def setUp(self): + self.directory = tempfile.TemporaryDirectory() + self.repo = Repo(self.directory.name) + + def tearDown(self): + self.directory.cleanup() + + def base(self, files: dict[str, str], pinned: tuple[str, ...]) -> None: + self.repo.write({"Cargo.toml": CARGO, **files}) + self.repo.pin(*pinned) + self.repo.commit("base") + self.repo.git("checkout", "-q", "-b", "work") + + def change(self, files: dict[str, str | None], pinned: tuple[str, ...] | None = None) -> None: + self.repo.write(files) + if pinned is not None: + self.repo.pin(*pinned) + self.repo.commit("change") + + # --- what it reports ------------------------------------------------- + + def test_a_committed_extraction_from_a_pinned_file_is_reported(self): + self.base({"src/lib.rs": "pub fn admit() {}\n"}, ("src/lib.rs",)) + self.change({"src/lib.rs": "mod admit;\n", "src/admit.rs": "pub fn admit() {}\n"}) + self.assertIn( + " newly unsealed module: src/admit.rs (declared by pinned src/lib.rs)", + self.repo.report()) + + def test_an_uncommitted_extraction_is_reported_too(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.repo.write({"src/lib.rs": "mod admit;\n", "src/admit.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/admit.rs"}) + + def test_a_dropped_pin_is_reported(self): + self.base({"src/lib.rs": "", "src/gate.rs": ""}, ("src/gate.rs", "src/lib.rs")) + self.change({}, pinned=("src/lib.rs",)) + self.assertIn(" pin removed: src/gate.rs", self.repo.report()) + + def test_a_module_unpinned_on_this_branch_is_reported_as_both(self): + self.base({"src/lib.rs": "mod gate;\n", "src/gate.rs": ""}, ("src/gate.rs", "src/lib.rs")) + self.change({}, pinned=("src/lib.rs",)) + lines = self.repo.report() + self.assertIn(" pin removed: src/gate.rs", lines) + self.assertEqual(self.repo.newly_unsealed(), {"src/gate.rs"}) + + def test_an_existing_file_newly_attached_to_a_pinned_module_is_reported(self): + # The file existed at the base but no pinned module declared it. + self.base({"src/lib.rs": "", "src/orphan.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": "mod orphan;\n"}) + self.assertEqual(self.repo.newly_unsealed(), {"src/orphan.rs"}) + + def test_test_only_modules_are_counted_not_listed(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({ + "src/lib.rs": '#[cfg(test)]\n#[path = "lib_tests.rs"]\nmod tests;\n' + '#[cfg(all(feature = "x", test))]\nmod gated_tests;\n', + "src/lib_tests.rs": "", "src/gated_tests.rs": "", + }) + lines = self.repo.report() + self.assertIn(" newly unsealed test-only modules: 2 (not listed; unpinned tests are the norm)", lines) + self.assertEqual(self.repo.newly_unsealed(), set()) + + def test_a_module_inside_a_test_only_module_is_test_only(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({ + "src/lib.rs": '#[cfg(test)]\nmod tests {\n #[path = "../fixtures/helper.rs"]\n mod helper;\n}\n', + # rustc: relative to src/tests/, the inline module's directory. + "src/fixtures/helper.rs": "", + }) + self.assertEqual(self.repo.newly_unsealed(), set()) + self.assertIn(" newly unsealed test-only modules: 1 (not listed; unpinned tests are the norm)", + self.repo.report()) + + def test_not_test_and_any_test_are_production(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": '#[cfg(not(test))]\nmod real;\n' + '#[cfg(any(test, feature = "x"))]\nmod either;\n', + "src/real.rs": "", "src/either.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/real.rs", "src/either.rs"}) + + def test_a_module_inside_a_test_only_file_is_test_only(self): + # Inheritance through a file, not just an inline block. + self.base({"src/lib.rs": "#[cfg(test)]\nmod tests;\n", "src/tests.rs": ""}, + ("src/lib.rs", "src/tests.rs")) + self.change({"src/tests.rs": "mod helper;\n", "src/tests/helper.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), set()) + self.assertIn(" newly unsealed test-only modules: 1 (not listed; unpinned tests are the norm)", + self.repo.report()) + + def test_restricted_visibility_keeps_its_attributes(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": "#[cfg(test)]\npub(crate) mod tests;\n" + "pub(in crate) mod scoped;\npub(super) mod parent_scoped;\n", + "src/tests.rs": "", "src/scoped.rs": "", "src/parent_scoped.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/scoped.rs", "src/parent_scoped.rs"}) + self.assertIn(" newly unsealed test-only modules: 1 (not listed; unpinned tests are the norm)", + self.repo.report()) + + def test_a_feature_gated_module_is_labelled(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": '#[cfg(feature = "lab")]\nmod lab;\n' + '#[cfg(target_os = "macos")]\nmod mac;\n', + "src/lab.rs": "", "src/mac.rs": ""}) + lines = self.repo.report() + self.assertIn( + " newly unsealed module: src/lab.rs (declared by pinned src/lib.rs, behind a cargo feature)", + lines) + # A non-feature key = value is not a feature gate. + self.assertIn(" newly unsealed module: src/mac.rs (declared by pinned src/lib.rs)", lines) + + def test_a_feature_gate_is_inherited_through_a_file(self): + self.base({"src/lib.rs": '#[cfg(feature = "lab")]\nmod lab;\n', "src/lab.rs": ""}, + ("src/lab.rs", "src/lib.rs")) + self.change({"src/lab.rs": "mod inner;\n", "src/lab/inner.rs": ""}) + self.assertIn( + " newly unsealed module: src/lab/inner.rs (declared by pinned src/lab.rs, behind a cargo feature)", + self.repo.report()) + + def test_a_new_file_under_a_parent_pinned_on_this_branch_is_reported(self): + # The parent was not pinned at the base; the child is a new file. + self.base({"src/lib.rs": "mod post;\n", "src/post.rs": ""}, ("src/lib.rs",)) + self.change({"src/post.rs": "mod guard;\n", "src/post/guard.rs": ""}, + pinned=("src/lib.rs", "src/post.rs")) + self.assertEqual(self.repo.newly_unsealed(), {"src/post/guard.rs"}) + + def test_a_file_pinned_at_the_base_and_unpinned_under_a_newly_pinned_parent(self): + self.base({"src/lib.rs": "mod post;\n", "src/post.rs": "mod guard;\n", "src/post/guard.rs": ""}, + ("src/lib.rs", "src/post/guard.rs")) + self.change({}, pinned=("src/lib.rs", "src/post.rs")) + lines = self.repo.report() + self.assertIn(" pin removed: src/post/guard.rs", lines) + self.assertEqual(self.repo.newly_unsealed(), {"src/post/guard.rs"}) + + # --- what it deliberately does not report ---------------------------- + + def test_pinning_a_parent_does_not_report_its_existing_children(self): + # The false positive the first real run produced: pinning thirteen files + # made their existing test modules look like new departures. + self.base({"src/lib.rs": "mod post;\n", "src/post.rs": "#[cfg(test)]\nmod tests;\nmod rules;\n", + "src/post/tests.rs": "", "src/post/rules.rs": ""}, ("src/lib.rs",)) + self.change({}, pinned=("src/lib.rs", "src/post.rs")) + self.assertEqual(self.repo.report(), [CLEAN]) + + def test_a_module_left_unpinned_before_the_branch_is_not_reprinted(self): + self.base({"src/lib.rs": "mod labels;\n", "src/labels.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": "mod labels;\npub fn changed() {}\n"}) + self.assertEqual(self.repo.report(), [CLEAN]) + + def test_a_declared_module_with_no_file_is_not_reported(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": "mod ghost;\n"}) + self.assertEqual(self.repo.report(), [CLEAN]) + + def test_commented_out_modules_are_ignored(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": "// mod ghost;\n/* mod phantom; /* nested */ mod still_comment; */\n", + "src/ghost.rs": "", "src/phantom.rs": "", "src/still_comment.rs": ""}) + self.assertEqual(self.repo.report(), [CLEAN]) + + def test_it_compares_with_the_merge_base_not_the_tip_of_main(self): + # main moves on after the branch point and pins a new file. Compared with + # main's tip, this branch would appear to drop that pin. + self.base({"src/lib.rs": "", "src/later.rs": ""}, ("src/lib.rs",)) + self.change({"src/unrelated.txt": "x"}) + self.repo.git("checkout", "-q", "main") + self.repo.pin("src/later.rs", "src/lib.rs") + self.repo.commit("main pins later.rs") + self.repo.git("checkout", "-q", "work") + self.assertEqual(self.repo.report(), [CLEAN]) + + # --- module resolution, per rustc ------------------------------------- + + def test_a_bare_mod_in_a_path_loaded_file_resolves_beside_it(self): + # The case review found missing: rustc treats a #[path]-loaded file as + # owning its directory, so `mod guard;` there is `src/guard.rs`. + self.base({"src/lib.rs": '#[path = "post_impl.rs"]\nmod post;\n', "src/post_impl.rs": ""}, + ("src/lib.rs", "src/post_impl.rs")) + self.change({"src/post_impl.rs": "mod guard;\n", "src/guard.rs": "", "src/post_impl/guard.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/guard.rs"}) + + def test_a_path_attribute_in_an_ordinary_file_resolves_beside_it(self): + # The shape agent.rs uses for nearly every agent_*.rs file. rustc + # resolves `#[path]` from the declaring file's own directory, never from + # `/`, even though a bare `mod` in the same file would. + self.base({"src/lib.rs": "mod agent;\n", "src/agent.rs": ""}, ("src/agent.rs", "src/lib.rs")) + self.change({"src/agent.rs": '#[path = "agent_post.rs"]\nmod post;\n', + "src/agent_post.rs": "", "src/agent/agent_post.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/agent_post.rs"}) + + def test_a_bare_mod_in_an_ordinary_file_resolves_under_its_stem(self): + self.base({"src/lib.rs": "mod foo;\n", "src/foo.rs": ""}, ("src/foo.rs", "src/lib.rs")) + self.change({"src/foo.rs": "mod bar;\n", "src/foo/bar.rs": "", "src/bar.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/foo/bar.rs"}) + + def test_a_mod_rs_directory_module_is_found(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": "mod db;\n", "src/db/mod.rs": "mod store;\n", "src/db/store.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/db/mod.rs"}) + + def test_a_path_attribute_inside_an_inline_module(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": 'mod outer {\n #[path = "inner_impl.rs"]\n mod inner;\n}\n', + "src/outer/inner_impl.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/outer/inner_impl.rs"}) + + def test_crate_roots_come_from_cargo_conventions_and_manifest_paths(self): + self.base({"src/bin/tool.rs": "", "tests/it.rs": "", "src/daemon.rs": "", "build.rs": "", + "src/bin/multi/main.rs": "", "sub/Cargo.toml": CARGO, "sub/src/main.rs": ""}, + ("build.rs", "src/bin/multi/main.rs", "src/bin/tool.rs", "src/daemon.rs", + "sub/src/main.rs", "tests/it.rs")) + self.repo.write({"Cargo.toml": CARGO + '[[bin]]\nname = "d"\npath = "./src/daemon.rs"\n'}) + self.change({"src/bin/tool.rs": "mod a;\n", "src/bin/a.rs": "", + "tests/it.rs": "mod b;\n", "tests/b.rs": "", + "src/daemon.rs": "mod c;\n", "src/c.rs": "", + "build.rs": "mod d;\n", "d.rs": "", + "src/bin/multi/main.rs": "mod e;\n", "src/bin/multi/e.rs": "", + "sub/src/main.rs": "mod f;\n", "sub/src/f.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), + {"src/bin/a.rs", "tests/b.rs", "src/c.rs", "d.rs", + "src/bin/multi/e.rs", "sub/src/f.rs"}) + + def test_a_file_under_a_nested_tests_directory_is_an_ordinary_module(self): + # Only `/tests/*.rs` is a crate root. `src/tests/case.rs`, reached + # through `mod tests;`, is an ordinary file, so rustc resolves its + # `mod stray;` under its stem -- and a report that took it for a root + # would neither look there nor name it as a child. + # `src/tests.rs` rather than `src/tests/mod.rs`: it sorts ahead of the + # nested files, so a resolver that wrongly took them for roots would + # visit them first. With `mod.rs` the correct parent happens to win the + # visit order and the mistake goes unseen -- measured. + self.base({"src/lib.rs": "mod tests;\n", "src/tests.rs": "mod case;\n", + "src/tests/case.rs": ""}, + ("src/lib.rs", "src/tests.rs", "src/tests/case.rs")) + self.change({"src/tests/case.rs": "mod stray;\n", + "src/tests/case/stray.rs": "", "src/tests/stray.rs": ""}) + self.assertEqual(self.repo.newly_unsealed(), {"src/tests/case/stray.rs"}) + + # --- lexing ------------------------------------------------------------ + + def test_comment_markers_inside_strings_do_not_hide_a_module(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({ + "src/lib.rs": 'const A: &str = "src/*"; mod first;\n' + 'const B: &str = "http://x"; mod second;\n' + 'const C: &str = r#"*/ //"#; mod third;\n' + "const D: char = '\"'; mod fourth;\n" + 'const E: &str = r#"a"b /* "#; mod fifth;\n' + "const F: char = '\\''; mod sixth; // */\n" + # An escaped quote in a char literal must not open a string + # that runs to the end of the file. + "const G: char = '\\\"'; mod seventh;\n", + "src/first.rs": "", "src/second.rs": "", "src/third.rs": "", "src/fourth.rs": "", + "src/fifth.rs": "", "src/sixth.rs": "", "src/seventh.rs": "", + }) + self.assertEqual(self.repo.newly_unsealed(), + {"src/first.rs", "src/second.rs", "src/third.rs", "src/fourth.rs", + "src/fifth.rs", "src/sixth.rs", "src/seventh.rs"}) + + def test_an_attribute_containing_a_bracket_still_classifies_the_module(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.change({"src/lib.rs": '#[doc = "a]"]\n#[cfg(test)]\nmod tests;\n', "src/tests.rs": ""}) + # Found, and classified: a parser that loses the module entirely would + # also report no production module, so assert the test-only count. + self.assertEqual(self.repo.newly_unsealed(), set()) + self.assertIn(" newly unsealed test-only modules: 1 (not listed; unpinned tests are the norm)", + self.repo.report()) + + # --- it never blocks --------------------------------------------------- + + def test_no_merge_base_is_reported_not_raised(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.assertEqual(report_module.report(self.repo.root, "surface.json", "no-such-ref"), + ["surface coverage: no merge-base with no-such-ref; nothing compared"]) + + def test_a_surface_absent_at_the_merge_base_is_reported_not_raised(self): + self.repo.write({"Cargo.toml": CARGO, "src/lib.rs": ""}) + self.repo.commit("base without a surface") + self.repo.git("checkout", "-q", "-b", "work") + self.repo.pin("src/lib.rs") + self.repo.commit("surface") + self.assertEqual(self.repo.report(), + ["surface coverage: surface.json absent at merge-base; nothing compared"]) + + def test_the_command_accepts_the_surface_path_the_hook_passes(self): + # reseal.sh passes an absolute path; a person may pass one relative to + # where they stand. Both must reach the same comparison. + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + # Run from a subdirectory, so a relative path only works if it is taken + # relative to the caller rather than to --root. + for surface in (str(Path(self.repo.root, "surface.json")), "../surface.json"): + out = io.StringIO() + with contextlib.redirect_stdout(out), contextlib.chdir(Path(self.repo.root, "src")): + code = report_module.main(["--root", self.repo.root, "--surface", surface, + "--base", "main"]) + self.assertEqual((code, out.getvalue().strip()), (0, CLEAN), surface) + + def test_the_command_exits_zero_even_when_the_report_breaks(self): + self.base({"src/lib.rs": ""}, ("src/lib.rs",)) + self.repo.write({"surface.json": "not json"}) + out = io.StringIO() + with contextlib.redirect_stdout(out): + code = report_module.main(["--root", self.repo.root, "--surface", "surface.json", + "--base", "main"]) + self.assertEqual(code, 0) + self.assertIn("surface coverage: report unavailable", out.getvalue()) + + +if __name__ == "__main__": + unittest.main(verbosity=1) From ab5bde0fbe67fa2411181c607ef44d4797f8c370 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 16 Sep 2026 14:57:56 +0530 Subject: [PATCH 2/2] Scope two docstring claims to what was checked Inner #![cfg] does occur here, in five test files; none of them declares a module, which is the case the resolver does not handle. And the test file now says file sets, matching what the review compared, not counts. Co-Authored-By: Claude Opus 5 --- scripts/surface_coverage_report.py | 3 ++- scripts/surface_coverage_report.test.py | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/surface_coverage_report.py b/scripts/surface_coverage_report.py index 76673843..e58a773a 100644 --- a/scripts/surface_coverage_report.py +++ b/scripts/surface_coverage_report.py @@ -33,7 +33,8 @@ crate in this repository, across the build profiles compared. It does not handle `#[cfg_attr(..., path = ...)]`, `mod r#name;`, `include!`, macro-generated modules, `#[path]` on an inline module, raw-string `#[path]` -values or inner `#![cfg]`; none occurs here today. A `cfg` it cannot evaluate +values, or inner `#![cfg]` on a file that declares modules; none of these +occurs here today. A `cfg` it cannot evaluate leaves the module classified as production, which can add noise but does not hide a module. """ diff --git a/scripts/surface_coverage_report.test.py b/scripts/surface_coverage_report.test.py index 74191bd9..acc20a09 100644 --- a/scripts/surface_coverage_report.test.py +++ b/scripts/surface_coverage_report.test.py @@ -8,9 +8,8 @@ HEAD or from the tip of `main`. The module graph itself was checked against rustc rather than asserted: on -src-tauri's lib, bridge-tally-protocol and bridge-tally-core it reproduces the -file counts rustc compiled under default, all-features, test and -test-plus-all-features builds (bridge#416). +every crate in this repository it reached exactly the file sets rustc compiled, +across the build profiles compared in bridge#436's review. """ from __future__ import annotations