From 0159db521454fc5eab1cd268f8bf3780d44bb084 Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:28:38 -0700 Subject: [PATCH 1/3] Give the census guard test teeth, and stop it corrupting the census MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #96's fix for the vacuous guard test moved the defect rather than removing it. `PrefixListTests.setUp` still imported prefix_census in-process, and that is the import under test: with the guard removed the scan ran in setUp, clobbering the census *before* the test read `before`, after which the subprocess re-ran the same deterministic scan and produced byte-identical output. The assertion compared a corrupted file against itself and passed (#98). My mutation test missed this because it used the real corpus, where the scan exceeds the 60-second timeout. Against a ten-file corpus the old test reports OK in 0.057s while the census md5 changes underneath it. Only the timeout ever had teeth, and only because the corpus happens to be slow. Worse, the child computes its paths from `__file__`, so a regression would have an unguarded interpreter write the real tracked census and then take SIGKILL at sixty seconds, possibly mid-`json.dump`. The test written to prevent #95 could cause it. The guard test now lives in its own TestCase with no setUp importing the module, so the snapshot is genuinely taken first, and the child runs with MECHS_ROOT pointing at an empty directory. An unguarded import then reaches roots.mech_root, which exits rather than counting an empty corpus, so the child fails in milliseconds: deterministic, independent of machine speed, and nothing writes to a tracked file. Against the ten-file corpus that defeated the old version it now fails in 0.048s with the census untouched. stderr is captured rather than discarded, so a failure reports the traceback. `literal()` also only saw `ast.Assign`, so four ways to rebind a list after its literal survived it (#99) — `VOC += [...]`, `VOC.append(...)`, `VOC[0] = ...` and a rebinding nested in an `if`. It now walks the tree for Assign, AugAssign and AnnAssign, and separately fails on subscript assignment or a mutating method call. All five shapes are killed; the commit message in 39122af claiming "a later reassignment cannot hide" was true only of the plain `=` form. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_fleet_page.py | 66 +++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 18 deletions(-) diff --git a/tests/test_fleet_page.py b/tests/test_fleet_page.py index 93a1a3d..7165b92 100644 --- a/tests/test_fleet_page.py +++ b/tests/test_fleet_page.py @@ -288,9 +288,28 @@ def literal(self, script, name): whose value is a plain literal rules both out. """ source = (ROOT / "scripts/fleet" / script).read_text() - bindings = [node for node in ast.parse(source).body - if isinstance(node, ast.Assign) - and any(getattr(target, "id", None) == name for target in node.targets)] + tree = ast.parse(source) + # ast.Assign alone misses `VOC += [...]`, `VOC.append(...)`, `VOC[0] = ...` + # and a rebinding nested in an `if` — each leaves this reading a list the + # module no longer uses (#99). Walk the whole tree for every shape. + bindings, mutations = [], [] + for node in ast.walk(tree): + if isinstance(node, ast.Assign): + for target in node.targets: + if getattr(target, "id", None) == name: + bindings.append(node) + elif (isinstance(target, ast.Subscript) + and getattr(target.value, "id", None) == name): + mutations.append("subscript assignment") + elif isinstance(node, (ast.AugAssign, ast.AnnAssign)): + if getattr(node.target, "id", None) == name: + bindings.append(node) + elif (isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and getattr(node.func.value, "id", None) == name): + mutations.append(f"{name}.{node.func.attr}()") + self.assertEqual(mutations, [], f"{name} is mutated in {script} after it is bound: " + f"{mutations}; this test reads a single literal") self.assertEqual(len(bindings), 1, f"{name} is bound {len(bindings)} times in {script}; " "this test reads a single literal binding") @@ -316,25 +335,36 @@ def test_columns_and_clickable_cells_describe_the_same_vocabularies(self): self.assertEqual(sorted(columns - cells), [], "heatmap column with no cells behind it") self.assertEqual(sorted(cells - columns), [], "record lists built for a vocabulary no column shows") + + + +class CensusImportGuardTests(unittest.TestCase): + """That importing prefix_census does no work (#95). + + Deliberately its own class with no setUp: PrefixListTests imports the + module in setUp, and that import is the thing under test here. Sharing it + meant the scan ran before the snapshot was taken, so the assertion compared + a clobbered file against itself and passed (#98). + """ + def test_importing_the_census_module_does_not_scan_or_write(self): - # Reading P used to cost a four-minute scan and clobber the committed - # census, which is why none of the above could be tested (#95). - # - # This has to import in a FRESH interpreter. Importing here would be a - # no-op — setUp already put the module in sys.modules — and reloading - # would re-run the scan after the snapshot was taken, so the comparison - # would hold even with the guard removed. The first version of this test - # did exactly that and passed against the unguarded module. census = ROOT / "_fleet/data/prefix_census.json" before = census.read_bytes() - environment = dict(os.environ, PYTHONPATH=str(ROOT / "scripts/fleet")) - # The timeout is the teeth: a real scan takes minutes, so an unguarded - # module fails here long before it finishes writing. - subprocess.run([sys.executable, "-c", "import prefix_census"], - env=environment, check=True, timeout=60, - stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + # An empty MECHS_ROOT is what gives this teeth. An unguarded import + # reaches roots.mech_root, which exits rather than counting an empty + # corpus, so the child fails in milliseconds and check=True raises — + # no dependence on the real corpus being slow enough to hit a timeout, + # and no unguarded interpreter left writing over a tracked file. + with tempfile.TemporaryDirectory() as empty: + environment = dict(os.environ, + PYTHONPATH=str(ROOT / "scripts/fleet"), + MECHS_ROOT=empty) + done = subprocess.run([sys.executable, "-c", "import prefix_census"], + env=environment, timeout=60, + capture_output=True, text=True) + self.assertEqual(done.returncode, 0, + f"importing prefix_census did work:\n{done.stderr}") self.assertEqual(census.read_bytes(), before) - if __name__ == '__main__': unittest.main() From 2aa2ba971f91e100f2798549765914a18510751e Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:19:32 -0700 Subject: [PATCH 2/3] Stop the suite importing the census in-process at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splitting the guard test into its own class fixed its detection but closed only half of #98. `PrefixListTests.setUp` still did an in-process `import prefix_census`, so against a regressed module the suite went on overwriting the tracked census on every run — the guard test would now fail, correctly, but the damage had already happened in setUp. Measured on a copy with the guard removed and a ten-file corpus: the suite reported the failure and the census md5 changed anyway. setUp now fetches `P` and `norm` from a separate interpreter, the same way the guard test checks the import, with `MECHS_ROOT` pointing at an empty directory. A regressed module dies there in milliseconds and setUp raises with the child's stderr, so the failure is loud and nothing is written. Same corpus, same mutation, now: five failures and errors, and the census byte-identical. Reading the constants out of a subprocess rather than parsing them from source keeps the values the pipeline actually uses, which is what makes the accepted set exactly the 53 keys the census can emit rather than an approximation of it. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_fleet_page.py | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/tests/test_fleet_page.py b/tests/test_fleet_page.py index 7165b92..bcda4ac 100644 --- a/tests/test_fleet_page.py +++ b/tests/test_fleet_page.py @@ -265,7 +265,13 @@ class PrefixListTests(unittest.TestCase): """ def setUp(self): - import prefix_census + # Read P and norm out of a SEPARATE interpreter, never this one. An + # in-process import here is what made the old guard test toothless, and + # splitting that test out fixed its detection without closing this hole: + # against a regressed module, setUp itself still ran the scan and + # overwrote the tracked census every time the suite ran (#98). The empty + # MECHS_ROOT means a regressed module dies here in milliseconds instead. + constants = self.module_constants() # What the census can actually EMIT: every alternative in P after norm # is applied. Taking P plus norm's values instead would also accept the # 15 raw spellings norm exists to fold away — UniProtKB, IPR, mesh, @@ -273,11 +279,29 @@ def setUp(self): # so a column named one of them would pass while rendering as zeros. def literal_prefix(p): return p.replace("\\.", ".") # the regex escapes dots - self.census = {prefix_census.norm.get(literal_prefix(p), literal_prefix(p)) - for p in prefix_census.P.split("|")} + norm = constants["norm"] + self.census = {norm.get(literal_prefix(p), literal_prefix(p)) + for p in constants["P"].split("|")} self.voc = self.literal("build_data.py", "VOC") self.pref = self.literal("build_subsets.py", "PREF") + @staticmethod + def module_constants(): + """prefix_census's P and norm, fetched without importing it here.""" + with tempfile.TemporaryDirectory() as empty: + environment = dict(os.environ, + PYTHONPATH=str(ROOT / "scripts/fleet"), + MECHS_ROOT=empty) + done = subprocess.run( + [sys.executable, "-c", + "import json, prefix_census as p; print(json.dumps({'P': p.P, 'norm': p.norm}))"], + env=environment, timeout=60, capture_output=True, text=True) + if done.returncode != 0: + raise AssertionError( + "could not read prefix_census's constants; it does work at import " + f"(#95):\n{done.stderr}") + return json.loads(done.stdout) + def literal(self, script, name): """Read a list literal without importing — both scripts scan on import. From 2532f9b74a26f640de920cafccac4c52baa9aa8d Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:25:50 -0700 Subject: [PATCH 3/3] Sandbox the child, so the guard cannot rely on evaluation order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The child derived its own REPO from `__file__`, so its DATA was the real tracked `_fleet/data`. Nothing was written there only because json.dump(census(), open(DATA + "/prefix_census.json", "w"), indent=1) evaluates `census()` — which exits under an empty MECHS_ROOT — before `open()` truncates. That is an accident of argument order, and the expression emits a ResourceWarning on every real run, which actively invites the idiomatic with open(DATA + "/prefix_census.json", "w") as fh: that reverses it. With both that refactor and the guard regression applied, the suite's own children truncated the tracked census to zero bytes. The child now runs against a copy of scripts/fleet in a temp tree with its own _fleet/data, so REPO and DATA resolve inside the sandbox whatever the module does. Same mutation, now: four failures and an error, and the tracked census byte-identical at 2,778. That also allows a better assertion. Comparing the real file's bytes is vacuous once the child cannot reach it; the guard now lists what the child wrote in the sandbox and requires it to be empty — positive evidence, and independent of the exit code. The byte comparison stays as a backstop. `PrefixListTests.setUp` shares the same sandbox rather than duplicating the launch. Also: a comment claimed the child's failure surfaced through `check=True`, which this hunk had deliberately removed in favour of asserting on returncode so the child's stderr reaches the failure message. In a test whose whole value is explaining why it has teeth, that was the sentence a reader would act on. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_fleet_page.py | 77 ++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 19 deletions(-) diff --git a/tests/test_fleet_page.py b/tests/test_fleet_page.py index bcda4ac..22a1e6e 100644 --- a/tests/test_fleet_page.py +++ b/tests/test_fleet_page.py @@ -1,10 +1,12 @@ """Regression coverage for fleet admission, capability drift and generated output.""" from copy import deepcopy import ast +import contextlib import json import os from pathlib import Path import re +import shutil import subprocess import sys import tempfile @@ -253,6 +255,35 @@ def test_every_excluded_mech_is_one_the_census_actually_reads(self): self.assertLessEqual(set(roots.EXCLUDE_DIRS), set(roots.RECORD_GLOBS)) + +@contextlib.contextmanager +def census_sandbox(): + """A throwaway repo root for importing prefix_census in a child. + + The child derives its own REPO from `__file__`, so pointing it at the real + scripts/ would make its DATA the tracked _fleet/data. Nothing is written + there today only because `json.dump(census(), open(PATH, "w"))` evaluates + census() — which exits — before open() truncates. Rewrite that as the + idiomatic `with open(PATH, "w") as fh:` and the order reverses, leaving the + tracked census at zero bytes every time the suite runs. Copying the scripts + into a temp tree means the guard cannot depend on that accident (#102). + """ + with tempfile.TemporaryDirectory() as box: + root = Path(box) + shutil.copytree(ROOT / "scripts/fleet", root / "scripts/fleet") + data = root / "_fleet/data" + data.mkdir(parents=True) + (root / "empty").mkdir() + yield root, data, dict(os.environ, + PYTHONPATH=str(root / "scripts/fleet"), + MECHS_ROOT=str(root / "empty")) + +# Methods that change a list in place. A read like VOC.index(v) must not be +# mistaken for one. +LIST_MUTATORS = {"append", "extend", "insert", "remove", "pop", "clear", + "sort", "reverse", "__setitem__", "__delitem__"} + + class PrefixListTests(unittest.TestCase): """The pipeline carries three hand-maintained prefix lists that must agree. @@ -288,10 +319,7 @@ def literal_prefix(p): @staticmethod def module_constants(): """prefix_census's P and norm, fetched without importing it here.""" - with tempfile.TemporaryDirectory() as empty: - environment = dict(os.environ, - PYTHONPATH=str(ROOT / "scripts/fleet"), - MECHS_ROOT=empty) + with census_sandbox() as (root, data, environment): done = subprocess.run( [sys.executable, "-c", "import json, prefix_census as p; print(json.dumps({'P': p.P, 'norm': p.norm}))"], @@ -315,7 +343,14 @@ def literal(self, script, name): tree = ast.parse(source) # ast.Assign alone misses `VOC += [...]`, `VOC.append(...)`, `VOC[0] = ...` # and a rebinding nested in an `if` — each leaves this reading a list the - # module no longer uses (#99). Walk the whole tree for every shape. + # module no longer uses (#99). Walking the tree covers those four. + # It does NOT cover tuple-unpack rebinding, `del VOC[0]`, `VOC[0] += x` + # or a walrus; those are filed rather than chased (#101). + # + # Only mutating methods count. Flagging every attribute call would fail + # on `VOC.index(v)` — a read, and an idiomatic one in a file that + # already sorts VOC — with a message asserting a mutation that never + # happened, which is a worse trap than the hole it closes. bindings, mutations = [], [] for node in ast.walk(tree): if isinstance(node, ast.Assign): @@ -330,12 +365,14 @@ def literal(self, script, name): bindings.append(node) elif (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) - and getattr(node.func.value, "id", None) == name): + and getattr(node.func.value, "id", None) == name + and node.func.attr in LIST_MUTATORS): mutations.append(f"{name}.{node.func.attr}()") self.assertEqual(mutations, [], f"{name} is mutated in {script} after it is bound: " f"{mutations}; this test reads a single literal") self.assertEqual(len(bindings), 1, - f"{name} is bound {len(bindings)} times in {script}; " + f"{name} is bound or shadowed {len(bindings)} times in {script} " + f"(lines {[node.lineno for node in bindings]}); " "this test reads a single literal binding") # literal_eval refuses anything that is not a literal, so a computed # value fails here rather than being silently half-read. @@ -372,23 +409,25 @@ class CensusImportGuardTests(unittest.TestCase): """ def test_importing_the_census_module_does_not_scan_or_write(self): - census = ROOT / "_fleet/data/prefix_census.json" - before = census.read_bytes() - # An empty MECHS_ROOT is what gives this teeth. An unguarded import - # reaches roots.mech_root, which exits rather than counting an empty - # corpus, so the child fails in milliseconds and check=True raises — - # no dependence on the real corpus being slow enough to hit a timeout, - # and no unguarded interpreter left writing over a tracked file. - with tempfile.TemporaryDirectory() as empty: - environment = dict(os.environ, - PYTHONPATH=str(ROOT / "scripts/fleet"), - MECHS_ROOT=empty) + tracked = ROOT / "_fleet/data/prefix_census.json" + before = tracked.read_bytes() + # The empty MECHS_ROOT gives this teeth: an unguarded import reaches + # roots.mech_root, which exits rather than counting an empty corpus, so + # the child dies in milliseconds and the returncode assertion reports + # its stderr. No dependence on the real corpus being slow enough to trip + # a timeout. The sandbox means the child cannot touch the real tree + # whatever it does. + with census_sandbox() as (root, data, environment): done = subprocess.run([sys.executable, "-c", "import prefix_census"], env=environment, timeout=60, capture_output=True, text=True) + # Positive evidence, and independent of the exit code: a scan that + # ran would have left its census here. + written = sorted(path.name for path in data.iterdir()) self.assertEqual(done.returncode, 0, f"importing prefix_census did work:\n{done.stderr}") - self.assertEqual(census.read_bytes(), before) + self.assertEqual(written, [], f"importing prefix_census wrote {written}") + self.assertEqual(tracked.read_bytes(), before) if __name__ == '__main__': unittest.main()