diff --git a/docs/superpowers/evidence/file-length-test-venv-blindspot-2026-07-25.json b/docs/superpowers/evidence/file-length-test-venv-blindspot-2026-07-25.json new file mode 100644 index 00000000..ef3f3072 --- /dev/null +++ b/docs/superpowers/evidence/file-length-test-venv-blindspot-2026-07-25.json @@ -0,0 +1,35 @@ +{ + "intent": "Make the 500-line file-length test see the repo's real Python sources. It walked the whole working tree and skipped vendored trees by hard-coded directory NAME, so the venv that the pre-push CI mirror creates in-repo (.ci-mirror-venv, not the listed .venv) was walked — and the test died with a UnicodeDecodeError on a joblib fixture that is deliberately not UTF-8, hiding every real violation behind a crash.", + "scope": [ + "python_pkg/tests/test_file_length.py - the walk, the line count, and two new regression tests", + "Non-goal: changing the 500-line limit itself, or which non-venv directories count as vendored beyond adding third_party/" + ], + "changes": [ + "Virtual environments are detected by their pyvenv.cfg marker instead of by name, so any venv anyone drops in the tree is pruned whatever it is called. Naming them one at a time is precisely what failed here.", + "The walk uses os.walk and prunes in place, so a venv's thousands of third-party files are never visited at all rather than visited and filtered.", + "Lines are counted from bytes (read_bytes().splitlines()), so a line count can never depend on a file's encoding — that is what turned a non-UTF-8 vendored fixture into a test error.", + "third_party/ joins the vendored skip set: it holds vendored upstream MCP servers (blender-mcp's 2813-line addon.py, wander-agent's 1835-line server.py, ...) which the repo's own 500-line rule does not govern.", + "Two regression tests added: a fake venv under an unlisted name containing an oversized, undecodable module is pruned; and _line_count handles a big5-encoded file." + ], + "verification": [ + { + "command": "python -m pytest python_pkg/tests/test_file_length.py -q", + "result": "pass", + "evidence": "3 passed. Before the fix: UnicodeDecodeError on .ci-mirror-venv/.../test_func_inspect_special_encoding.py. Immediately after the walk was fixed the test reported 8 real violations (6 vendored third_party/ files plus session_autopsy's detectors.py 559 and parse.py 511); third_party/ is now skipped as vendored and the two session_autopsy files were split under 500 lines, so the run is green on merit rather than by exemption." + }, + { + "command": "python -m pytest python_pkg -q (full suite with coverage gate)", + "result": "pass", + "evidence": "876 passed; TOTAL coverage 100% (3874 statements, 1124 branches), fail_under=100 satisfied. Test files are coverage-omitted, so the new helpers add no coverage debt." + } + ], + "risks": [ + "A source file placed inside a directory that happens to contain a pyvenv.cfg is no longer checked. That is the intended trade: such a directory IS a virtualenv.", + "third_party/ is now unchecked wholesale. Vendored upstream code was never meant to satisfy this repo's line limit, but a first-party file misfiled under third_party/ would slip past.", + "Counting lines from bytes treats a lone CR as a line break where the old text read did too (str.splitlines and bytes.splitlines split on the same set for ASCII control characters), so counts are unchanged for real source files." + ], + "rollback": [ + "git revert the commit; the test returns to name-based skipping and will again crash on any in-repo venv.", + "After rollback, re-run python -m pytest python_pkg/tests/test_file_length.py and confirm it errors rather than reporting violations - that is the old behaviour." + ] +} diff --git a/python_pkg/tests/test_file_length.py b/python_pkg/tests/test_file_length.py index 1abab19f..ddff33e0 100644 --- a/python_pkg/tests/test_file_length.py +++ b/python_pkg/tests/test_file_length.py @@ -2,20 +2,27 @@ from __future__ import annotations +import os from pathlib import Path MAX_LINES = 500 -# Directories to skip (vendored / generated / virtual-envs) +# Directories to skip (vendored / generated). +# +# Virtual environments are NOT listed here by name: they are detected by their +# pyvenv.cfg marker instead, so any venv anyone drops in the tree is skipped +# whatever it is called. Naming them one by one is what broke this test before — +# `.venv` was listed but `.ci-mirror-venv` (created by the pre-push CI mirror) +# was not, and the walk then tripped over a third-party test fixture inside it. _SKIP_DIRS = frozenset( { - ".venv", "__pycache__", "build", "dist", ".eggs", "node_modules", "sonic_pi", + "third_party", # vendored upstream MCP servers, skills and agents ".git", } ) @@ -23,21 +30,62 @@ _SKIP_DIRS = frozenset( _ROOT = Path(__file__).resolve().parents[2] -def _python_files() -> list[Path]: - """Collect every *.py file under the repo root, skipping vendored dirs.""" +def _is_venv(path: Path) -> bool: + """Report whether a directory is a Python virtual environment. + + Args: + path: Directory to inspect. + + Returns: + True when the directory holds a ``pyvenv.cfg``, which every venv does. + """ + return (path / "pyvenv.cfg").is_file() + + +def _python_files(root: Path) -> list[Path]: + """Collect the project's own *.py files, pruning vendored trees and venvs. + + Args: + root: Directory to walk. + + Returns: + Every Python file under ``root`` that belongs to the project, sorted. + """ files: list[Path] = [] - for path in _ROOT.rglob("*.py"): - if any(part in _SKIP_DIRS for part in path.parts): - continue - files.append(path) + for dirpath, dirnames, filenames in os.walk(root): + current = Path(dirpath) + # Mutating dirnames in place prunes the walk, so a venv's thousands of + # third-party files are never visited at all. + dirnames[:] = [ + name + for name in dirnames + if name not in _SKIP_DIRS and not _is_venv(current / name) + ] + files.extend(current / name for name in filenames if name.endswith(".py")) return sorted(files) +def _line_count(path: Path) -> int: + """Count a file's lines without decoding it. + + Reading bytes keeps a line count from depending on the file's encoding: the + repo vendors third-party fixtures that are deliberately not UTF-8, and + decoding one of those used to fail this test with a UnicodeDecodeError. + + Args: + path: File to measure. + + Returns: + The number of lines in the file. + """ + return len(path.read_bytes().splitlines()) + + def test_all_python_files_are_at_most_500_lines() -> None: """Every Python source file must be at most 500 lines.""" violations: list[str] = [] - for path in _python_files(): - line_count = len(path.read_text(encoding="utf-8").splitlines()) + for path in _python_files(_ROOT): + line_count = _line_count(path) if line_count > MAX_LINES: rel = path.relative_to(_ROOT) violations.append(f" {rel}: {line_count} lines") @@ -45,3 +93,27 @@ def test_all_python_files_are_at_most_500_lines() -> None: assert not violations, ( f"The following files exceed {MAX_LINES} lines:\n" + "\n".join(violations) ) + + +def test_walk_skips_virtualenvs_whatever_they_are_named(tmp_path: Path) -> None: + """A venv is pruned by its marker file, not by matching a known name.""" + package = tmp_path / "python_pkg" + package.mkdir() + own_source = package / "real_module.py" + own_source.write_text("x = 1\n", encoding="utf-8") + + venv = tmp_path / "some-unlisted-venv" + (venv / "lib").mkdir(parents=True) + (venv / "pyvenv.cfg").write_text("home = /usr\n", encoding="utf-8") + # Both of the traps the old walk fell into: far over the line limit, and not + # decodable as UTF-8. + (venv / "lib" / "vendored.py").write_bytes(b"# \xa4@\xa8\xc7\n" + b"pass\n" * 900) + + assert _python_files(tmp_path) == [own_source] + + +def test_line_count_survives_undecodable_bytes(tmp_path: Path) -> None: + """Line counting works on a file that is not valid UTF-8.""" + path = tmp_path / "big5.py" + path.write_bytes(b"# -*- coding: big5 -*-\n# \xa4@\xa8\xc7\npass\n") + assert _line_count(path) == 3