From b7f968127d15463e89c27f6e770703a04242c715 Mon Sep 17 00:00:00 2001 From: melbinjp Date: Mon, 31 Aug 2026 02:13:44 +0530 Subject: [PATCH] Pin the action to a commit, and prove the read-only claim @fschutt security-reviewed this action before adding it to azul's CI and pointed out that `melbinjp/docproof@v0.2.3` is a tag, so it can be repointed after someone has read the code behind it. The README now shows the commit form and says why. His workflow also asserts that a docproof run leaves the tree clean, on the grounds that the tool "is documented as read-only". It was not documented as read-only, and nothing checked it. It was simply true, which is the shape of defect this project exists to find, found here by a stranger reading it before he trusted it. Both halves fixed: the guarantee is written down, and tests/test_read_only.py runs a full check over a repository with real drift and fails if one tracked byte changed or one untracked file appeared. Verified by injecting a write into main() and confirming both tests go red. --- README.md | 20 ++++++++++ tests/test_read_only.py | 83 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 tests/test_read_only.py diff --git a/README.md b/README.md index c6a1c16..725f4b5 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,26 @@ a full clone reports `1 broken` and exits 1, and the same repository at `--depth permanently green gate that had judged nothing, so it fails with the one-line fix in the message instead. [Requirements](#requirements) has the rest. +**Pin it to a commit rather than to the tag.** `melbinjp/docproof@v0.2.3` is a tag, and a +tag can be repointed after you have read the code behind it. Anyone able to push here, +including anyone who takes this account, could move `v0.2.3` onto something else and your +CI would run it without a diff for you to review: + +```yaml +- uses: melbinjp/docproof@a3b2fbc0513d20836a8a1f273009b16ac97e00d9 # v0.2.3 +``` + +Raised by [@fschutt](https://github.com/fschutt) while security-reviewing this action +before putting it into `azul`'s CI, which is the right order to do those two things in. + +**It only reads.** A run shells out to `git` for history and opens documents for reading, +and that is the whole of its contact with your tree: nothing written, nothing moved, +nothing created, including inside the project being checked. So it is safe ahead of any +step that expects a clean checkout. That is not a promise in prose. `tests/test_read_only.py` +runs a full check over a real repository with real drift and fails if one tracked byte +changed, or one untracked file appeared, afterwards. The guarantee is there because +@fschutt asserted it in `azul`'s CI before this repository had documented or tested it. + Adopting on a project that already has drift, which is most of them: `fail-on-findings: false` prints every finding and leaves the run green while you work through them. [What that suppresses, and the one thing it diff --git a/tests/test_read_only.py b/tests/test_read_only.py new file mode 100644 index 0000000..a80f4bf --- /dev/null +++ b/tests/test_read_only.py @@ -0,0 +1,83 @@ +"""Running the checker must not change the project it is checking. + +@fschutt asserted this in `azul`'s CI before adding docproof to it: the workflow fails the +build if a run leaves a single tracked file modified, on the grounds that the tool "is +documented as read-only". It was not documented as read-only. It was true, nobody had +written it down, and nothing checked it, which is the exact shape of defect this project +exists to find, found in this project by somebody reading it before they trusted it. + +A guarantee a stranger is willing to assert in their own CI is one worth owning here. +""" + +from __future__ import annotations + +import subprocess +from collections.abc import Callable +from pathlib import Path + +from docproof.cli import main + + +def _tracked_bytes(repo: Path) -> dict[str, bytes]: + """Every tracked file's exact contents, asked of git rather than of a walk.""" + listing = subprocess.run( + ["git", "ls-files", "-z"], + cwd=repo, + check=True, + capture_output=True, + timeout=60, + ) + names = [chunk.decode() for chunk in listing.stdout.split(b"\0") if chunk] + return {name: (repo / name).read_bytes() for name in names} + + +def _porcelain(repo: Path) -> str: + """Modified tracked files AND new untracked ones, which `git status` reports as `??`.""" + return subprocess.run( + ["git", "status", "--porcelain"], + cwd=repo, + check=True, + capture_output=True, + text=True, + timeout=60, + ).stdout + + +def test_a_run_that_finds_drift_still_leaves_every_tracked_byte_alone( + make_repo: Callable[..., Path], +) -> None: + """The repository has real drift, so the run does real work before it is measured. + + Asserting exit 1 is not decoration. A read-only check that judged nothing would pass + this test while proving nothing, and a vacuous version of this assertion is worth less + than no assertion at all. + """ + repo = make_repo( + {"pyproject.toml": '[project]\nname = "prog"\nversion = "0"\n'}, + deleted={"tools/gone.py": ""}, + documented_before={"README.md": "Run `tools/gone.py` first."}, + ) + before = _tracked_bytes(repo) + assert _porcelain(repo) == "", "the fixture itself must start clean" + + assert main([str(repo)]) == 1, "the run has to have found the drift, or this proves nothing" + + assert _porcelain(repo) == "", "a docproof run left the working tree dirty" + assert _tracked_bytes(repo) == before, "a docproof run rewrote a tracked file" + + +def test_a_clean_run_creates_no_files_either(make_repo: Callable[..., Path]) -> None: + """The quiet path writes nothing too, including no cache, log or report dropped into + the project. `git status --porcelain` lists untracked files, so this catches those.""" + repo = make_repo( + { + "pyproject.toml": '[project]\nname = "prog"\nversion = "0"\n', + "README.md": "The entry point is `src/app.py`.\n", + "src/app.py": "", + } + ) + before = _tracked_bytes(repo) + assert main([str(repo)]) == 0 + + assert _porcelain(repo) == "", "a clean docproof run still touched the tree" + assert _tracked_bytes(repo) == before