From 04a8d8126419b7540a34eeed88aec2fd570fade7 Mon Sep 17 00:00:00 2001 From: melbinjp Date: Wed, 19 Aug 2026 20:47:57 +0530 Subject: [PATCH] A directory that was emptied for good is not a directory that might refill `paths.py` skips a trailing-slash directory claim when the parent is tracked, because git stores files and not directories, so an empty one is indistinguishable from an absent one. The reason is real: posthog-python's RELEASING.md says changesets live in `.sampo/changesets/`, and every release empties that directory as the bot consumes them. The guard's own comment claimed the tracked-parent requirement "keeps the case this must still catch: a documented directory whose entire tree really did go". It keeps none of them. A removed SUBdirectory of a live tree has a tracked parent too - toolhive documents `pkg/container/verifier/` for Sigstore verification, `pkg/container` is alive, and `verifier` went in 7095e8e1 "Remove /verifier in favour of one coming from toolhive-core". Measured over the 47 clones of batches 10 and 11, every --show-skips run captured to disk: 38 directory claims silenced, 31 never tracked at all and the guard right about every one, 7 populated and wholly emptied across toolhive, hive, onyx and openmed. So the skip keeps its reason and gains a receipt, which is the soundness rule the rest of the verifier already runs on. `Git.emptied_and_stayed` replays the paths OLDEST FIRST and counts how many times the live set falls to empty; once means it stayed empty, more than once is a lifecycle. The forwards replay is not incidental. The first version of the measurement walked newest-first and called it a refill at the first add after a delete - but an add seen later in a newest-first walk is older, so every directory created before it was deleted scored as churn. That run reported ZERO emptied directories and nothing about the output looked wrong. Caught only because one case had been verified by hand first. The posthog test moved to tests/test_emptied_directory.py and now builds a real fill-consume-fill-consume history, because the old fixture committed one file and deleted it once, which is not what a changeset directory does. Also checked against the live repository: emptied_and_stayed returns None there. 215 tests. Verified on the real toolhive clone: 13 findings to 15, both new ones naming the commit that removed the subsystem. --- src/docproof/vcs.py | 87 +++++++++++++++++++ src/docproof/verifiers/paths.py | 13 ++- tests/test_emptied_directory.py | 149 ++++++++++++++++++++++++++++++++ tests/test_paths.py | 37 ++++++-- 4 files changed, 279 insertions(+), 7 deletions(-) create mode 100644 tests/test_emptied_directory.py diff --git a/src/docproof/vcs.py b/src/docproof/vcs.py index 61a3fde..3249377 100644 --- a/src/docproof/vcs.py +++ b/src/docproof/vcs.py @@ -383,6 +383,93 @@ def _copied_from(self, destination: str, source: str) -> str | None: return commit return None + def emptied_and_stayed(self, directory: str) -> str | None: + """The commit that took the last file out of a directory, if it never refilled. + + **A guard whose stated intent and behaviour disagreed.** `paths.py` skips a claim + written with a trailing slash when the directory's parent is tracked, because git + cannot represent an empty directory - `PostHog/posthog-python`'s RELEASING.md says + changesets live in `.sampo/changesets/`, that directory is emptied by every release + as the bot consumes them, and reporting it argues with a lifecycle. Right. + + Its comment then claims the tracked-parent requirement "keeps the case this must + still catch: a documented directory whose entire tree really did go". It keeps none + of them, because a wholly removed subdirectory of a live tree has a tracked parent + too. `stacklok/toolhive` documents `pkg/container/verifier/` for Sigstore + verification; `pkg/container` is alive and `verifier` went in `7095e8e1` + *"Remove /verifier in favour of one coming from toolhive-core"*. + + **Measured across the 47 clones of sweep batches 10 and 11**, every `--show-skips` + run captured to disk: the guard silences **38 directory claims, of which 31 were + never tracked at all** - promises, or places the reader creates, and the guard is + right about every one - **and 7 were populated and wholly emptied**, in four + repositories. None of the 38 is the churning case in this corpus, which is the shape + the guard was written for and which `composio/.changeset` confirms is real. + + So the skip keeps its reason and gains a receipt, which is the soundness rule the + rest of this verifier already runs on: a path is only called broken when the + repository can be shown to have HAD it and dropped it. + + THE REPLAY RUNS FORWARDS, and the first version of the measurement did not. Walking + newest-first and calling it a refill on the first add after a delete marks every + directory that was created before it was deleted, which is all of them; that run + reported zero emptied directories and put toolhive's verifier in the churn column. + Nothing about the output looked wrong. So: replay oldest-first, count how many times + the live set falls to empty, and only once means it stayed empty. + """ + code, out, _ = _run( + [ + "git", + "log", + "--reverse", + "-M", + "--name-status", + "--format=commit %h", + "--", + directory, + ], + self.root, + ) + if code != 0 or not out.strip(): + return None + + wanted = directory.strip("/") + prefix = wanted + "/" + + def inside(path: str) -> bool: + return path == wanted or path.startswith(prefix) + + live: set[str] = set() + commit: str | None = None + emptyings: list[str] = [] + refilled = False + for line in out.splitlines(): + if line.startswith("commit "): + commit = line[len("commit ") :].strip() + continue + parts = line.split("\t") + if len(parts) < 2 or commit is None: + continue + was_empty = not live + status = parts[0][:1] + if status == "D": + live.discard(parts[1]) + elif status in {"R", "C"} and len(parts) == 3: + if inside(parts[1]): + live.discard(parts[1]) + if inside(parts[2]): + live.add(parts[2]) + elif inside(parts[1]): + live.add(parts[1]) + if live and was_empty and emptyings: + refilled = True + elif not live and not was_empty: + emptyings.append(commit) + + if refilled or not emptyings: + return None + return emptyings[-1] + def claim_introduced_after(self, doc: str, subject: str, commit: str) -> bool | None: """Whether this document first mentioned `subject` *after* `commit` removed it. diff --git a/src/docproof/verifiers/paths.py b/src/docproof/verifiers/paths.py index c1aa0de..0f0f95a 100644 --- a/src/docproof/verifiers/paths.py +++ b/src/docproof/verifiers/paths.py @@ -550,7 +550,18 @@ def _judge(self, project: Project, claim: Claim, git: Git) -> Finding: f"instruction rather than a claim that this repository has it", ) - if claim.subject.rstrip().endswith("/") and git.tracks(str(PurePosixPath(subject).parent)): + # **UNLESS git can show it was populated and wholly emptied.** The comment above says + # the tracked-parent requirement keeps the case of a directory whose entire tree + # really did go; it keeps none of them, because a removed subdirectory of a live tree + # has a tracked parent too. Measured over 47 clones: this guard silences 38 directory + # claims, 31 never tracked at all and 7 populated and emptied for good. See + # `Git.emptied_and_stayed` - the skip keeps its reason and gains the same receipt + # every other verdict in this file runs on. + if ( + claim.subject.rstrip().endswith("/") + and git.tracks(str(PurePosixPath(subject).parent)) + and not git.emptied_and_stayed(subject) + ): return self.skip( claim, f"`{subject}` is written as a directory and its parent is tracked. Git " diff --git a/tests/test_emptied_directory.py b/tests/test_emptied_directory.py new file mode 100644 index 0000000..b53f331 --- /dev/null +++ b/tests/test_emptied_directory.py @@ -0,0 +1,149 @@ +"""An empty directory and a removed one look the same to git, until you replay the history. + +`paths.py` skips a claim written with a trailing slash when the directory's parent is +tracked. The reason is real: git stores files, not directories, so a directory with no files +is indistinguishable from one that never existed - and `PostHog/posthog-python`'s +RELEASING.md says changesets live in `.sampo/changesets/`, a directory every release empties +as the bot consumes them. The sentence says where `sampo add` PUTS files. Reporting it argues +with a lifecycle. + +The guard's own comment then claimed the tracked-parent requirement "keeps the case this must +still catch: a documented directory whose entire tree really did go". It keeps none of them. +A removed SUBdirectory of a live tree has a tracked parent too: + + stacklok/toolhive docs/arch/06-registry-system.md:864 `pkg/container/verifier/` + 7095e8e1 "Remove /verifier in favour of one coming from toolhive-core" + +`pkg/container` is alive. `verifier` is gone, and the tool said nothing. + +MEASURED over the 47 clones of sweep batches 10 and 11, every `--show-skips` run captured to +disk rather than sampled: the guard silences **38 directory claims, 31 of them never tracked +at all** - promises, and places the reader creates, and the guard is right about all 31 - +**and 7 populated and wholly emptied**, across toolhive, hive, onyx and openmed. + +The test for the posthog case builds its own history because the shape under test is a +directory that goes empty and REFILLS, and a fixture that commits once and deletes once +cannot express it. That distinction is the whole rule, and the previous test asserted the +posthog verdict over a history posthog does not have. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +from docproof.vcs import Git + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + ["git", *args], + cwd=repo, + capture_output=True, + check=True, + timeout=60, + env={ + "GIT_AUTHOR_NAME": "docproof tests", + "GIT_AUTHOR_EMAIL": "test@example.invalid", + "GIT_COMMITTER_NAME": "docproof tests", + "GIT_COMMITTER_EMAIL": "test@example.invalid", + "GIT_AUTHOR_DATE": "2026-01-01T00:00:00", + "GIT_COMMITTER_DATE": "2026-01-01T00:00:00", + "PATH": os.environ.get("PATH", ""), + "SYSTEMROOT": os.environ.get("SYSTEMROOT", ""), + }, + ) + + +def _add(repo: Path, path: str, message: str) -> None: + target = repo / path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("contents\n", encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", message, "--no-gpg-sign") + + +def _remove(repo: Path, path: str, message: str) -> None: + _git(repo, "rm", "-q", path) + _git(repo, "commit", "-qm", message, "--no-gpg-sign") + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + root = tmp_path / "tree" + root.mkdir() + _git(root, "init", "-q") + _add(root, "keep/anchor.txt", "something so the repository is not empty") + return root + + +def test_the_posthog_case_a_directory_that_refills(repo: Path) -> None: + """Filled, consumed, filled, consumed. This is what a changeset directory does. + + Verified against the real repository rather than only the fixture: at the clone on disk + `PostHog/posthog-python` has a file under `.sampo/changesets/` again, and + `emptied_and_stayed` returns None for it. + """ + _add(repo, ".sampo/changesets/gallant-prince.md", "add a changeset") + _remove(repo, ".sampo/changesets/gallant-prince.md", "release v7.39.0, consume changesets") + _add(repo, ".sampo/changesets/brave-otter.md", "add another changeset") + _remove(repo, ".sampo/changesets/brave-otter.md", "release v7.39.1, consume changesets") + + assert Git(root=repo).emptied_and_stayed(".sampo/changesets") is None + + +def test_the_toolhive_case_a_subsystem_removed_under_a_live_parent(repo: Path) -> None: + """The case the guard's comment claimed to keep and did not. + + Three files added over three commits, then all of them removed at once, and the parent + `pkg/container` still holds other code. That is a subsystem leaving, and every one of + the seven found in the corpus has this shape. + """ + _add(repo, "pkg/container/runtime.go", "the container package") + _add(repo, "pkg/container/verifier/sigstore.go", "add sigstore verification") + _add(repo, "pkg/container/verifier/cosign.go", "add cosign verification") + _git(repo, "rm", "-q", "-r", "pkg/container/verifier") + _git( + repo, + "commit", + "-qm", + "Remove /verifier in favour of one coming from toolhive-core", + "--no-gpg-sign", + ) + + git = Git(root=repo) + # The premise: the parent is alive, which is exactly why the old guard fired. + assert git.tracks("pkg/container") + assert git.emptied_and_stayed("pkg/container/verifier") is not None + + +def test_a_directory_that_never_existed_names_nothing(repo: Path) -> None: + """31 of the 38 silenced claims are this, and the guard is right about every one of them. + + A documented directory git has never tracked a file under is a promise, or a place the + reader is told to create. There is no receipt and there must be no finding. + """ + assert Git(root=repo).emptied_and_stayed("docs/changesets") is None + + +def test_a_directory_that_still_has_files_names_nothing(repo: Path) -> None: + _add(repo, "pkg/live/thing.go", "a package that is still here") + assert Git(root=repo).emptied_and_stayed("pkg/live") is None + + +def test_emptied_then_refilled_then_emptied_is_still_churn(repo: Path) -> None: + """The boundary, because a single trailing emptying is what the rule keys on. + + A directory that has been empty before and filled again is a lifecycle whatever state it + happens to be in at HEAD, and reporting it on the down-swing would be reporting the same + directory differently depending on when the run happened. + """ + _add(repo, "fragments/one.md", "first fragment") + _remove(repo, "fragments/one.md", "consume it") + _add(repo, "fragments/two.md", "second fragment") + _remove(repo, "fragments/two.md", "consume that one too") + + assert Git(root=repo).emptied_and_stayed("fragments") is None diff --git a/tests/test_paths.py b/tests/test_paths.py index ca67ff3..61ca0eb 100644 --- a/tests/test_paths.py +++ b/tests/test_paths.py @@ -544,17 +544,22 @@ def test_a_file_that_moved_under_src_did_not_vanish(make_repo: Callable[..., Pat assert "moved it under" in detail -def test_the_posthog_case_an_empty_directory_is_not_a_deleted_one(make_repo: Callable[..., Path]): +def test_a_documented_directory_git_never_tracked_is_not_drift(make_repo: Callable[..., Path]): """Git stores files, not directories, so it cannot tell empty from absent. - `PostHog/posthog-python`'s RELEASING.md says changesets must live in - `.sampo/changesets/`. The last file under it was deleted in `0fc7ec6` — the release bot - consuming a changeset on the v7.39.1 release, which is the directory's normal lifecycle, - not its removal. The sentence says where `sampo add` PUTS files and is still true. + **This used to be the posthog case and the fixture could not express it.** It built one + file and deleted it, which is not what `.sampo/changesets/` does — that directory is + filled and consumed on every release, and a fixture that commits once and removes once + has no lifecycle in it at all. The real shape now lives in + `tests/test_emptied_directory.py`, which builds the refill, and it was checked against + the live repository as well. + + What stays here is the case that needs no history: a directory the project documents and + git has never tracked a file under. **31 of the 38 claims this guard silences across the + 47-clone corpus are exactly this**, and there is no receipt to report. """ repo = make_repo( {".sampo/config.toml": "", "src/a.py": ""}, - deleted={".sampo/changesets/gallant-prince-ukko.md": ""}, documented_before={"README.md": "Changesets must live in `.sampo/changesets/`.\n"}, ) verdict, detail = run(repo)[".sampo/changesets/"] @@ -562,6 +567,26 @@ def test_the_posthog_case_an_empty_directory_is_not_a_deleted_one(make_repo: Cal assert "empty one is indistinguishable" in detail +def test_a_subsystem_removed_under_a_live_parent_is_broken(make_repo: Callable[..., Path]): + """The case the guard's own comment claimed to keep, and kept none of. + + `test_a_directory_whose_whole_tree_went_is_still_broken` below covers the parent going + too. `stacklok/toolhive` is the other shape and the commoner one: `pkg/container` is + alive and documented, and `pkg/container/verifier/` went in `7095e8e1` *"Remove + /verifier in favour of one coming from toolhive-core"*. Tracked parent, so the guard + fired, so the tool said nothing about a Sigstore verification subsystem its own + documentation still names two lines under a finding it DID report. + """ + repo = make_repo( + {"pkg/container/runtime.go": "", "src/a.py": ""}, + deleted={"pkg/container/verifier/sigstore.go": ""}, + documented_before={"README.md": "Verification lives in `pkg/container/verifier/`.\n"}, + ) + verdict, detail = run(repo)["pkg/container/verifier/"] + assert verdict is Verdict.BROKEN + assert "deleted in" in detail + + def test_a_directory_whose_whole_tree_went_is_still_broken(make_repo: Callable[..., Path]): """The recall this must not cost: requiring the PARENT to be tracked is what keeps a genuinely removed directory tree reportable.