diff --git a/scripts/build_dist.py b/scripts/build_dist.py index 11a2b221d..09f08b178 100755 --- a/scripts/build_dist.py +++ b/scripts/build_dist.py @@ -27,6 +27,7 @@ from __future__ import annotations import argparse +import functools import hashlib import json import os @@ -169,20 +170,38 @@ def write_plugin_manifest(names): _MAX_INDENT = 3 -def _fence_step(line, marker, marker_len): - """spec/audit.py's fence reading, imported on first use rather than at module import. - - One reading of CommonMark across the fidelity checks, the review ledger, and this generator, - rather than a second one here that could disagree with them. Imported lazily because - scripts/skills_install.py imports this module on hosts whose Python predates what audit.py - needs, and installing never fills a region. +# Deliberately lru_cache(maxsize=None) rather than functools.cache, which needs 3.9, and the noqa below is what stops ruff rewriting it back under this repo's own py313 target. +# The module scripts/skills_install.py imports this one at module scope, and host-setup/*/install-skills.* and scripts/skills_install.sh each commit to Python 3.7+, because that path runs before the fleet's own toolchain exists to install a newer interpreter. +# So functools.cache here would raise AttributeError at import on 3.7 and 3.8, breaking the one path that has to survive an old host, and spec/host-tools.json's python3 entry records that 3.7 floor and why it sits below the fleet's own. +@functools.lru_cache(maxsize=None) # noqa: UP033 +def _audit_module(): + """spec/audit.py, imported on first use rather than at module import, then held. + + Imported lazily because scripts/skills_install.py imports this module on hosts whose Python + predates what audit.py needs, and installing never fills a region. Held because the caller + below reaches for it once per line, and re-resolving it there costs a sys.path scan and a + sys.modules lookup every time: one pass over the 24 authored skills, 4644 lines across the 40 + Markdown files they comprise, measured warm in one process, best of five, costs 17.5 ms + re-resolved against 0.9 ms held, where calling audit directly costs 0.7 ms. One --check run + makes 18708 of these calls, four times that corpus, because it scans the authored tree and + both generated trees, and it runs on every pull request. The cache sits here rather than on + _fence_step, whose arguments vary per line and would make one unbounded. """ spec = str(ROOT / "spec") if spec not in sys.path: sys.path.insert(0, spec) import audit - return audit._fence_step(line, marker, marker_len) + return audit + + +def _fence_step(line, marker, marker_len): + """spec/audit.py's fence reading. + + One reading of CommonMark across the fidelity checks, the review ledger, and this generator, + rather than a second one here that could disagree with them. + """ + return _audit_module()._fence_step(line, marker, marker_len) def _lf(text): diff --git a/scripts/tests/test_build_dist.py b/scripts/tests/test_build_dist.py index 7b97b967c..b5bc7ffb5 100755 --- a/scripts/tests/test_build_dist.py +++ b/scripts/tests/test_build_dist.py @@ -10,6 +10,7 @@ import io import json import os +import subprocess import sys import tempfile import unittest @@ -775,11 +776,55 @@ def test_a_body_leaving_a_fence_open_is_refused(self) -> None: build_dist.regenerate() def test_fence_step_adds_the_spec_directory_to_sys_path_once(self) -> None: + """The cache would otherwise hide this: held, the accessor's body runs once per process, so + the `not in sys.path` guard could be deleted and the path still grow by only one. Clearing + between calls makes the body run each time, which is what the guard is there for.""" before = len(sys.path) for _ in range(3): + build_dist._audit_module.cache_clear() build_dist._fence_step("plain", None, 0) self.assertLessEqual(len(sys.path) - before, 1) + def test_the_audit_module_is_resolved_once_however_many_lines_are_read(self) -> None: + """`_fence_step` reaches for it per line, so resolving it per line is what this stops.""" + build_dist._audit_module.cache_clear() + + for _ in range(50): + build_dist._fence_step("plain", None, 0) + + self.assertEqual(build_dist._audit_module.cache_info().misses, 1) + self.assertEqual(build_dist._audit_module.cache_info().hits, 49) + + def test_importing_this_module_does_not_import_audit(self) -> None: + """The laziness `scripts/skills_install.py` depends on, which caching must not give up. + + It imports this module on hosts whose Python predates what `audit.py` needs, and installing + never fills a region, so importing `audit` at module scope would break that path. Run in a + fresh interpreter because this test process has already imported `audit` by other means. + + The measurement is the delta across the import rather than the child's absolute state, since + a `sitecustomize` or `usercustomize` hook runs before `-c` does and could import a module of + that name itself. That would be this case failing to run rather than the import being wrong, + which is a boundary to report rather than a finding to raise, so it skips and says so. + """ + scripts_dir = str(Path(build_dist.__file__).parent) + source = ( + "import sys; before = 'audit' in sys.modules;" + f" sys.path.insert(0, {scripts_dir!r}); import build_dist;" + " print(before, 'audit' in sys.modules, end='')" + ) + + result = subprocess.run( + [sys.executable, "-c", source], capture_output=True, text=True, check=True + ) + + before, after = result.stdout.split() + if before == "True": + self.skipTest( + "this interpreter imports a module named audit at startup, so the import delta cannot be read here" + ) + self.assertEqual(after, "False", "importing build_dist pulled in audit") + if __name__ == "__main__": unittest.main()