From bc1de8f7b76d790688a85ab2334b2c00413dfbbb Mon Sep 17 00:00:00 2001 From: Jer Miller Date: Fri, 31 Jul 2026 23:26:32 -0600 Subject: [PATCH] fix(checks): retirement is a repository fact, not a disk fact A completed conversion wave declares the Python roots it removed. The check asked the filesystem whether those directories still existed, so a checkout that deleted the sources but left ignored build output behind reported the wave as unfinished. The failure named the wave rather than the working tree that produced it, sending the reader to the wrong place entirely. Ask git instead: a declared root is retired once the repository holds nothing under it. Tracked and untracked-but-unignored files both still count, so a genuine leftover fails exactly as before, and the message now names an offending file. When git cannot answer, fall back to the on-disk test rather than passing a retirement we cannot prove. --- scripts/check_conversion_retirements.py | 47 +++++++++++- tests/test_conversion_retirements.py | 99 +++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 2 deletions(-) create mode 100644 tests/test_conversion_retirements.py diff --git a/scripts/check_conversion_retirements.py b/scripts/check_conversion_retirements.py index 416e96aa2..9e4e07c9e 100644 --- a/scripts/check_conversion_retirements.py +++ b/scripts/check_conversion_retirements.py @@ -29,6 +29,38 @@ _TEST_ONLY_GROUPS = frozenset( ) +def _repository_paths_under(root: Path, relative: str) -> list[str] | None: + """Return repository-visible paths under ``relative``, or None if git cannot say. + + Tracked files and untracked-but-unignored files both count. Ignored build + output does not: a checkout that removed a directory's sources leaves its + ``__pycache__`` behind, and that is a property of one operator's disk rather + than of the repository. Reporting it as an unfinished retirement sends the + reader to the wave author instead of to their own working tree. + """ + try: + result = subprocess.run( + [ + "git", + "ls-files", + "--cached", + "--others", + "--exclude-standard", + "--", + relative, + ], + cwd=root, + capture_output=True, + text=True, + check=False, + ) + except OSError: + return None + if result.returncode != 0: + return None + return [line for line in result.stdout.splitlines() if line.strip()] + + class CheckResult(NamedTuple): ok: bool checked_waves: tuple[str, ...] @@ -314,9 +346,20 @@ def check_repository( checked.append(wave_id) for python_root in python_roots: - if (root / python_root).exists(): + survivors = _repository_paths_under(root, python_root) + if survivors is None: + # git could not answer; fall back to the on-disk test rather than + # reporting a retirement we cannot prove. + if (root / python_root).exists(): + violations.append( + f"{wave_id}: declared Python root still exists: {python_root}" + ) + continue + if survivors: violations.append( - f"{wave_id}: declared Python root still exists: {python_root}" + f"{wave_id}: declared Python root still exists: {python_root} " + f"({len(survivors)} file(s) in the repository, e.g. " + f"{survivors[0]})" ) normalized_distribution = _normalize_distribution(distribution) diff --git a/tests/test_conversion_retirements.py b/tests/test_conversion_retirements.py new file mode 100644 index 000000000..e3ed6078f --- /dev/null +++ b/tests/test_conversion_retirements.py @@ -0,0 +1,99 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright (c) 2026 sol pbc + +"""The retirement check must describe the repository, not one operator's disk.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import scripts.check_conversion_retirements as checker + +MANIFEST = """\ +schema_version = 1 +dependency_files = ["pyproject.toml"] +content_roots = ["src"] +content_exclusions = [] + +[[waves]] +id = "sample-wave" +status = "done" +distribution = "retired-dist" +python_roots = ["src/legacy"] +import_roots = ["retired_dist"] +test_only_dependency_locations = [] +""" + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + ["git", "-c", "user.email=t@e", "-c", "user.name=t", *args], + cwd=repo, + check=True, + ) + + +def _repo(base: Path) -> Path: + base.mkdir(parents=True, exist_ok=True) + _git(base, "init", "-q") + (base / "pyproject.toml").write_text('[project]\nname = "x"\n', encoding="utf-8") + (base / ".gitignore").write_text("__pycache__/\n", encoding="utf-8") + (base / "src").mkdir() + (base / "src" / "keep.py").write_text("x = 1\n", encoding="utf-8") + _git(base, "add", "-A") + _git(base, "commit", "-qm", "base") + return base + + +def _run(repo: Path, tmp_path: Path): + manifest = tmp_path / "conversion-retirements.toml" + manifest.write_text(MANIFEST, encoding="utf-8") + return checker.check_repository(repo, manifest) + + +def test_retired_root_absent_passes(tmp_path): + repo = _repo(tmp_path / "r") + + result = _run(repo, tmp_path) + + assert result.ok, result.violations + + +def test_retired_root_holding_only_ignored_build_output_passes(tmp_path): + """A checkout that removed the sources leaves __pycache__; the repo did not.""" + repo = _repo(tmp_path / "r") + cache = repo / "src" / "legacy" / "__pycache__" + cache.mkdir(parents=True) + (cache / "mod.cpython-313.pyc").write_bytes(b"\x00") + + result = _run(repo, tmp_path) + + assert result.ok, result.violations + + +def test_retired_root_with_tracked_source_still_fails(tmp_path): + repo = _repo(tmp_path / "r") + legacy = repo / "src" / "legacy" + legacy.mkdir(parents=True) + (legacy / "mod.py").write_text("x = 1\n", encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "legacy") + + result = _run(repo, tmp_path) + + assert not result.ok + assert any("src/legacy" in item for item in result.violations), result.violations + + +def test_retired_root_with_untracked_unignored_source_still_fails(tmp_path): + """Not a weakening: only ignored content is forgiven.""" + repo = _repo(tmp_path / "r") + legacy = repo / "src" / "legacy" + legacy.mkdir(parents=True) + (legacy / "stray.py").write_text("x = 1\n", encoding="utf-8") + + result = _run(repo, tmp_path) + + assert not result.ok + assert any("src/legacy" in item for item in result.violations), result.violations -- 2.51.2