From ccc7970a2f63938ab50e262ae17903ff19c61baa Mon Sep 17 00:00:00 2001 From: zaebee Date: Mon, 17 Aug 2026 10:48:55 +0000 Subject: [PATCH 1/4] feat(ci): a floor under the size of the test suite itself (#405) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 2026-08-17 a stale-tree push over #339 deleted 62 files — 5 modules under src/cgis and 24 test files — and Python Verification passed. 1922 tests became 1264 and nothing reported it, because the tests that would have failed were deleted by the same commit. To CI, 'the module and its tests are gone' and 'both still pass' are one observation. Every floor the repository had (>= 72 in test_recordings_from_corpus, >= 16 in test_backfill_calibration_fingerprint) was written against this exact silence and could not help: they live inside test files, and the push that trips them also removes them. So this is a workflow step, not a test. The floor is read from the base branch and the ceiling from the branch's own copy, which is the load-bearing half. A stale tree carries a stale baseline: the tree in the incident was six days old and held roughly the count it had then, so a check against its own file would have compared 1264 to 1264 and passed. Against origin/main's 1932 it fails — verified end to end on a worktree with 24 test files removed: 628 gone missing, exit 1, and exit 0 on the intact tree. Four rules: the suite may not fall below the base branch's number; the baseline may only rise; it may not claim more tests than exist; and it may not fall more than 150 behind, or it stops bounding anything. Rules 3 and 4 are suppressed while rule 1 is broken, because 'lower the baseline' is the wrong advice to print beside '628 tests have gone missing'. A base ref with no baseline yet returns None rather than a number, and the step says on stderr that the anti-deletion rule did not apply. That covers the bootstrap commit and branches cut before it; it cannot be reached by deleting the file later, because such a branch fails on the working-tree read first. Co-Authored-By: Claude Opus 5 --- .github/test-count-baseline | 1 + .github/workflows/ci.yml | 19 ++ scripts/check_test_count.py | 267 ++++++++++++++++++++++++++++ tests/unit/test_check_test_count.py | 207 +++++++++++++++++++++ 4 files changed, 494 insertions(+) create mode 100644 .github/test-count-baseline create mode 100644 scripts/check_test_count.py create mode 100644 tests/unit/test_check_test_count.py diff --git a/.github/test-count-baseline b/.github/test-count-baseline new file mode 100644 index 00000000..b22803ae --- /dev/null +++ b/.github/test-count-baseline @@ -0,0 +1 @@ +1957 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1f12acfc..6fba7e15 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,6 +67,25 @@ jobs: uv run --frozen pytest --cov=cgis --cov=scripts --cov-report=term-missing --cov-report=xml + - name: Test-count floor + # Deliberately a workflow step and not a test (#405). On 2026-08-17 a + # stale-tree push deleted 24 test files and 5 modules under src/cgis, + # and the step above passed: the tests that would have failed went with + # them. Every floor the repository had lived inside a test file, so the + # push that tripped them also removed them. + # + # The floor is read from the *base branch*, not from this checkout — a + # stale tree carries a stale baseline, and comparing the branch against + # its own copy passes the exact incident above. `fetch-depth: 0` on the + # checkout is what makes origin/ readable here. + # + # `github.base_ref` is set on pull_request only; on a push to the trunk + # it is empty and ref_name is the branch being pushed, which is the + # right comparison there too. + run: > + uv run --frozen python scripts/check_test_count.py + --base-ref "origin/${{ github.base_ref || github.ref_name }}" + - name: Project version for Sonar id: version # Without this every analysis records the version as the literal string diff --git a/scripts/check_test_count.py b/scripts/check_test_count.py new file mode 100644 index 00000000..711dffb0 --- /dev/null +++ b/scripts/check_test_count.py @@ -0,0 +1,267 @@ +"""A floor under the size of the test suite itself (#405). + +On 2026-08-17 a bot pushed a stale worktree over a pull request. It committed a +whole tree rather than a diff, so a week of `main` read as deletion: 62 files, +5 modules under `src/cgis` and 24 test files. **Python Verification passed** — +because the tests that would have failed were deleted by the same commit. To +CI, "the module and its tests are gone" and "both still pass" are one +observation. + +The corpus floors already in the repository (`>= 72` in +`test_recordings_from_corpus.py`, `>= 16` in +`test_backfill_calibration_fingerprint.py`) were written against exactly this +shape of silence and could not help: they live inside test files, and this +failure deletes the file. A guard shipped inside the thing it guards is removed +by the event it exists to catch. So this runs from `ci.yml`, not from pytest. + +**The baseline is read twice, from two places, and that is the load-bearing +part.** The floor comes from the *base branch*; only the ceiling consults the +copy in the pull request. A stale tree carries a stale baseline — in the +incident, a tree from six days earlier held roughly the count it had then, so a +check against the branch's own file would have compared 1264 against ~1264 and +passed. The base branch held 1932. + +The point generalises past this repository: a reference value stored inside the +subject cannot bound the subject. +""" + +import argparse +import re +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +#: One line, one integer. Under `.github/` rather than `tests/` so that deleting +#: the test tree does not also delete the record of how large it was. +BASELINE_PATH = Path(".github/test-count-baseline") + +#: How far the real count may run ahead of the recorded baseline before the +#: baseline must be raised. Without a ceiling the floor rots: a suite that grows +#: to 3000 against a baseline of 1932 would let a thousand tests be deleted +#: silently, which is the failure this file exists to prevent, arriving late. +MAX_DRIFT = 150 + +#: `pytest --collect-only -q` ends in one of three shapes, measured on this +#: repository rather than assumed: +#: +#: 1932 tests collected in 1.83s +#: 137/1932 tests collected (1785 deselected) in 1.95s +#: no tests collected in 0.00s +#: +#: The third yields no number at all. It must raise rather than fall back to a +#: default, because every plausible default (0, or "skip the check") turns a +#: broken command into a passing one — and a floor that cannot fail is worse +#: than no floor, since it reads as protection. +_COLLECTED = re.compile(r"^(?:(\d+)/)?(\d+) tests? collected", re.M) + + +class CannotCountError(RuntimeError): + """Raised when the number of tests cannot be established. + + Its own type so a caller cannot confuse "the suite shrank" with "we never + found out how big it is". The first is a finding; the second is a broken + check, and reporting one as the other is the whole defect class here. + """ + + +def parse_collected(output: str) -> int: + """The total pytest collected, from the summary line. + + Returns the *total*, not the selected count: under `-k` the line reads + `137/1932`, and 137 is how many matched a filter rather than how many exist. + A floor fed the filtered number would fail on every filtered run and, worse, + would pass a real deletion whenever the filter happened to be narrow. + """ + match = _COLLECTED.search(output) + if match is None: + _msg = ( + "Could not find a collected-test count in pytest's output. The last line is " + "normally ' tests collected'; 'no tests collected' means collection failed " + "and is not a count of zero.\n" + f"--- output ---\n{output.strip()[-2000:]}" + ) + raise CannotCountError(_msg) + return int(match.group(2)) + + +def collect_count(repo_root: Path) -> int: + """How many tests pytest can collect right now.""" + result = subprocess.run( + ["uv", "run", "--frozen", "pytest", "--collect-only", "-q"], + cwd=repo_root, + capture_output=True, + text=True, + check=False, + ) + # Not `check=True`: a non-zero exit with a usable summary line is possible, + # and the exit code is not the question being asked. The parse decides, and + # it refuses loudly when there is nothing to parse — so the output is passed + # through either way rather than being thrown away with an exception whose + # message would not contain it. + return parse_collected(result.stdout + result.stderr) + + +def baseline_on(ref: str, repo_root: Path) -> int | None: + """The baseline as recorded on `ref`, or None when `ref` predates the file. + + None is not a fallback and not a silent pass. It says there is no prior + count to compare against, and a shrink is undetectable without a prior — the + honest report of that is "this rule cannot fire", not a number. It happens + for exactly two reasons, both self-limiting: the commit that introduces the + baseline, and branches cut before it. Removing the file later cannot get + here, because a branch that deletes it fails on `baseline_here` first. + + A ref that does not resolve at all is a different thing entirely — a broken + workflow, not a young repository — and raises. The two are told apart by + resolving the ref first rather than by matching git's error text, which + would not distinguish "unknown ref" from "path not in this tree". + """ + resolved = subprocess.run( + ["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"], + cwd=repo_root, + capture_output=True, + check=False, + ) + if resolved.returncode: + _msg = ( + f"{ref} does not resolve. The floor has to come from the base branch, so a " + f"missing base ref is a broken workflow rather than a young repository — " + f"falling back to this branch's own copy is the evasion this check closes." + ) + raise CannotCountError(_msg) + result = subprocess.run( + ["git", "show", f"{ref}:{BASELINE_PATH.as_posix()}"], + cwd=repo_root, + capture_output=True, + text=True, + check=False, + ) + if result.returncode: + return None + return _as_count(result.stdout, f"{ref}:{BASELINE_PATH}") + + +def baseline_here(repo_root: Path) -> int: + """The baseline as it stands in the working tree — the copy a branch may raise.""" + path = repo_root / BASELINE_PATH + if not path.is_file(): + _msg = f"{path} is missing. It records how large the suite is expected to be." + raise CannotCountError(_msg) + return _as_count(path.read_text(encoding="utf-8"), str(path)) + + +def _as_count(raw: str, source: str) -> int: + """One integer, or a refusal naming where the unusable value came from.""" + text = raw.strip() + if not text.isdigit(): + _msg = f"{source} does not hold a plain integer: {text[:80]!r}" + raise CannotCountError(_msg) + return int(text) + + +def problems(actual: int, floor: int | None, here: int) -> list[str]: + """Every rule broken, in the order a reader wants them. + + Four rules, each closing a different route: + + 1. `actual >= floor` — the suite may not shrink below what the base branch + recorded. This is the anti-deletion rule, and it uses the base branch's + number so that editing the file in the branch cannot defeat it. + 2. `here >= floor` — the recorded baseline may only go up. Otherwise a + branch lowers it, merges, and every later branch inherits a weaker floor. + 3. `here <= actual` — the baseline may not claim more tests than exist, or + the next branch starts red through no fault of its own. + 4. `actual - here <= MAX_DRIFT` — the baseline has to be kept current, or it + stops bounding anything. + + Rules 3 and 4 are about maintaining the file and are suppressed while rule 1 + is broken. When tests really have gone missing, "the baseline claims more + than exists" is a consequence of the deletion, not a second problem — and + its remedy reads as *lower the baseline*, which is precisely the wrong move + to put beside "668 tests have gone missing". + + A `floor` of None disables rules 1 and 2, and nothing else. There is no + prior count on the base branch, so a shrink is not merely undetected but + undefined — the caller says so on stderr rather than letting a green step + imply a floor was applied. + """ + found: list[str] = [] + if floor is not None: + if actual < floor: + found.append( + f"The suite collects {actual} tests; {floor} were recorded on the base " + f"branch. {floor - actual} have gone missing. Deleting a module together " + f"with its tests leaves CI green, which is why this is checked separately " + f"from whether the tests pass." + ) + if here < floor: + found.append( + f"{BASELINE_PATH} was lowered from {floor} to {here}. It may only rise: a " + f"lowered floor is inherited by every branch cut afterwards." + ) + if actual < floor: + return found + if here > actual: + found.append( + f"{BASELINE_PATH} records {here} but only {actual} tests are collected, so the " + f"next branch would start red. Lower it to {actual} or restore the tests." + ) + elif actual - here > MAX_DRIFT: + found.append( + f"The suite has grown to {actual}, more than {MAX_DRIFT} past the recorded " + f"{here}. Raise {BASELINE_PATH} to {actual} so the floor keeps bounding " + f"something." + ) + return found + + +def check(base_ref: str, repo_root: Path) -> tuple[list[str], bool]: + """Problems to report, and whether the anti-deletion floor was actually in force. + + The flag travels with the result rather than being inferred by the caller. + A clean run with no floor and a clean run with a floor are different + outcomes, and only one of them means the suite did not shrink. + """ + floor = baseline_on(base_ref, repo_root) + found = problems(collect_count(repo_root), floor, baseline_here(repo_root)) + return found, floor is not None + + +def main() -> int: + """Print every problem and return a shell exit code.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--base-ref", + default="origin/main", + help="where the floor is read from; must not be the branch under test", + ) + parser.add_argument("--repo-root", type=Path, default=REPO_ROOT) + args = parser.parse_args() + try: + found, floor_applied = check(args.base_ref, args.repo_root) + except CannotCountError as exc: + # Reported as a failure, never as a pass. "We could not tell" and "the + # suite is intact" are different answers, and only one of them is this + # check's job to give. + print(f"Test-count floor could not run: {exc}", file=sys.stderr) + return 2 + if not floor_applied: + # Said out loud on every such run. A step that goes green while the + # anti-deletion rule never ran looks identical in the log to one where + # it ran and passed, and that is the confusion this whole file exists + # to remove. + print( + f"NOTE: {args.base_ref} carries no {BASELINE_PATH}, so there is no prior count " + f"and the anti-deletion floor did not apply. Expected only while the baseline " + f"is being introduced, or on a branch cut before it.", + file=sys.stderr, + ) + for problem in found: + print(problem, file=sys.stderr) + return 1 if found else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/unit/test_check_test_count.py b/tests/unit/test_check_test_count.py new file mode 100644 index 00000000..22939d9d --- /dev/null +++ b/tests/unit/test_check_test_count.py @@ -0,0 +1,207 @@ +"""The floor under the suite's own size (#405). + +These tests cover the script; they are not the guard. The guard is the step in +`ci.yml`, because a check that ships inside the test tree is deleted by the push +it exists to catch — which is what happened on 2026-08-17, when 24 test files +and 5 modules went missing and Python Verification passed. +""" + +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent / "scripts")) + +from check_test_count import ( + BASELINE_PATH, + MAX_DRIFT, + CannotCountError, + _as_count, + baseline_here, + baseline_on, + parse_collected, + problems, +) + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent + + +def _commit_repo(root: Path, baseline: str | None = None) -> None: + """A one-commit git repository, with or without the baseline file in it.""" + subprocess.run(["git", "init", "-q"], cwd=root, check=True) + subprocess.run(["git", "config", "user.email", "t@t"], cwd=root, check=True) + subprocess.run(["git", "config", "user.name", "t"], cwd=root, check=True) + (root / "seed.txt").write_text("x", encoding="utf-8") + if baseline is not None: + (root / BASELINE_PATH.parent).mkdir(parents=True, exist_ok=True) + (root / BASELINE_PATH).write_text(baseline, encoding="utf-8") + subprocess.run(["git", "add", "-A"], cwd=root, check=True) + subprocess.run(["git", "commit", "-qm", "seed"], cwd=root, check=True) + + +class TestParsingPytestsSummary: + """Three shapes, measured on this repository rather than assumed.""" + + def test_the_ordinary_line(self) -> None: + assert parse_collected("1932 tests collected in 1.83s") == 1932 + + def test_a_filtered_run_reports_the_total_not_the_selection(self) -> None: + """`137/1932` under `-k`: 137 matched a filter, 1932 exist. + + Reading the left number would fail every filtered run, and — worse — + would pass a real deletion whenever the filter happened to be narrow + enough that the shrunken suite still cleared the floor. + """ + assert parse_collected("137/1932 tests collected (1785 deselected) in 1.95s") == 1932 + + def test_a_single_test_still_parses(self) -> None: + """pytest writes 'test' rather than 'tests' at one, so the plural is optional.""" + assert parse_collected("1 test collected in 0.01s") == 1 + + def test_no_tests_collected_refuses_rather_than_returning_zero(self) -> None: + """The line that carries no number at all. + + Zero would be a count, and a count of zero compared against a floor + fails loudly — which sounds safe until the caller catches it. The real + danger is the other plausible default, "skip the check", so this refuses + with its own exception type instead of returning anything. + """ + with pytest.raises(CannotCountError, match="not a count of zero"): + parse_collected("no tests collected in 0.00s") + + def test_the_refusal_carries_the_output_it_could_not_parse(self) -> None: + """A "could not run" with no output is a bug report nobody can action.""" + with pytest.raises(CannotCountError, match="ImportError: cannot import name"): + parse_collected("ImportError: cannot import name 'x' from 'y'") + + +class TestReadingTheBaselines: + """Where each number comes from, and what happens when it cannot be had.""" + + def test_the_working_tree_copy_is_read(self, tmp_path: Path) -> None: + (tmp_path / BASELINE_PATH.parent).mkdir(parents=True) + (tmp_path / BASELINE_PATH).write_text("1932\n", encoding="utf-8") + assert baseline_here(tmp_path) == 1932 + + def test_a_missing_file_refuses(self, tmp_path: Path) -> None: + with pytest.raises(CannotCountError, match="is missing"): + baseline_here(tmp_path) + + @pytest.mark.parametrize("body", ["", " ", "about 1932", "1932 tests", "-5"]) + def test_anything_but_a_plain_integer_refuses(self, body: str) -> None: + """Including the near-misses, which are the ones that would parse wrong.""" + with pytest.raises(CannotCountError, match="does not hold a plain integer"): + _as_count(body, "somewhere") + + def test_a_base_ref_that_does_not_resolve_refuses(self, tmp_path: Path) -> None: + """A broken workflow, not a young repository. + + No falling back to the branch's own copy: a stale tree carries a stale + baseline, so comparing a branch against itself passes the exact incident + this check was written for. + """ + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + with pytest.raises(CannotCountError, match="does not resolve"): + baseline_on("origin/main", tmp_path) + + def test_a_ref_that_predates_the_baseline_returns_none_rather_than_raising( + self, tmp_path: Path + ) -> None: + """The bootstrap commit, and every branch cut before it. + + Told apart from the unresolvable ref above by resolving the ref first, + not by matching git's error text — which would not distinguish "unknown + ref" from "path not in this tree", the same defect #375 found in a + `git show` reader. + """ + _commit_repo(tmp_path) + assert baseline_on("HEAD", tmp_path) is None + + def test_a_ref_that_carries_the_baseline_returns_it(self, tmp_path: Path) -> None: + _commit_repo(tmp_path, baseline="1932\n") + assert baseline_on("HEAD", tmp_path) == 1932 + + +class TestTheFourRules: + """Each rule closes a different route, so each is asserted on its own.""" + + def test_a_suite_at_its_floor_is_clean(self) -> None: + assert problems(actual=1932, floor=1932, here=1932) == [] + + def test_growth_within_the_drift_allowance_is_clean(self) -> None: + assert problems(actual=1932 + MAX_DRIFT, floor=1932, here=1932) == [] + + def test_a_shrunken_suite_is_reported_with_the_delta(self) -> None: + """The incident, in miniature: 1932 recorded, 1264 collected.""" + found = problems(actual=1264, floor=1932, here=1932) + assert len(found) == 1 + assert "668 have gone missing" in found[0] + + def test_the_shrink_is_caught_even_when_the_branch_lowered_its_own_baseline(self) -> None: + """The load-bearing case, and the reason the floor is read from the base branch. + + A stale tree carries a stale baseline. Checked against its own copy this + is 1264 against 1264 and passes; against the base branch's 1932 it + fails. Both rules fire here — the deletion and the lowering. + """ + found = problems(actual=1264, floor=1932, here=1264) + assert len(found) == 2 + assert any("gone missing" in p for p in found) + assert any("may only rise" in p for p in found) + + def test_lowering_the_baseline_alone_is_refused(self) -> None: + """Tests intact, floor quietly weakened for every branch cut afterwards.""" + found = problems(actual=1932, floor=1932, here=1900) + assert len(found) == 1 + assert "may only rise" in found[0] + + def test_a_baseline_above_the_real_count_is_refused(self) -> None: + """Otherwise the next branch starts red through no fault of its own.""" + found = problems(actual=1932, floor=1900, here=1950) + assert len(found) == 1 + assert "would start red" in found[0] + + def test_a_baseline_left_far_behind_is_refused(self) -> None: + """The anti-rot rule: a floor 1000 below reality bounds nothing.""" + found = problems(actual=1932 + MAX_DRIFT + 1, floor=1932, here=1932) + assert len(found) == 1 + assert "keeps bounding something" in found[0] + + def test_no_prior_disables_the_first_two_rules_and_nothing_else(self) -> None: + """`floor=None` is "undefined", not "zero" and not "fine". + + A shrink cannot be detected without a prior count, so rules 1 and 2 go + quiet — but the file must still be honest about the suite in front of + it, or the bootstrap commit could record any number at all and the floor + would start life wrong. + """ + assert problems(actual=1932, floor=None, here=1932) == [] + assert problems(actual=10, floor=None, here=99) != [] + assert problems(actual=1932 + MAX_DRIFT + 1, floor=None, here=1932) != [] + + def test_drift_is_not_reported_when_the_baseline_is_already_too_high(self) -> None: + """The two are mutually exclusive by construction, and both name a fix. + + Reported together they would tell the operator to raise and lower the + same number in one message. + """ + found = problems(actual=100, floor=50, here=400) + assert len(found) == 1 + + +def test_the_repository_currently_satisfies_its_own_floor() -> None: + """The recorded baseline is not above what this checkout actually collects. + + Deliberately not a call to `collect_count`: that shells out to a second full + collection, and the number is already known to the run collecting this test. + What can be checked cheaply is the invariant that breaks first — a baseline + committed ahead of reality, which would redden the next branch. + """ + recorded = baseline_here(REPO_ROOT) + assert recorded > 0 + assert recorded >= 1932, ( + f"{BASELINE_PATH} records {recorded}; it may only rise, and 1932 was the count when " + f"the floor was introduced." + ) From 97ab2b4c905e22a993e06f183963db25fd3c5391 Mon Sep 17 00:00:00 2001 From: zaebee Date: Mon, 17 Aug 2026 10:55:04 +0000 Subject: [PATCH 2/4] test(ci): cover the branches the floor exists for (#405) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud put new-code coverage at 73.3%, and the uncovered lines were collect_count, check and main — the refusals and the exit codes, which is the whole contract. The same shape #399 hit: a refusal nobody executes is a comment with a raise in it. Covers both streams being joined before parsing (uv writes on stderr), a broken collection refusing rather than returning a count, the flag saying whether the floor was in force, and the three exit codes — 0, 1, and 2 for 'could not tell', which must not read as 'the suite is intact'. 99% now; the remaining line is the __main__ guard. Co-Authored-By: Claude Opus 5 --- tests/unit/test_check_test_count.py | 120 ++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/tests/unit/test_check_test_count.py b/tests/unit/test_check_test_count.py index 22939d9d..34992b0b 100644 --- a/tests/unit/test_check_test_count.py +++ b/tests/unit/test_check_test_count.py @@ -14,6 +14,7 @@ sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent / "scripts")) +import check_test_count from check_test_count import ( BASELINE_PATH, MAX_DRIFT, @@ -21,6 +22,9 @@ _as_count, baseline_here, baseline_on, + check, + collect_count, + main, parse_collected, problems, ) @@ -191,6 +195,122 @@ def test_drift_is_not_reported_when_the_baseline_is_already_too_high(self) -> No assert len(found) == 1 +class _Ran: + """A stand-in for `subprocess.run`'s result, carrying only what is read.""" + + def __init__(self, stdout: str = "", stderr: str = "", returncode: int = 0) -> None: + self.stdout = stdout + self.stderr = stderr + self.returncode = returncode + + +class TestCollectingTheCount: + """The one place that shells out to pytest.""" + + def test_the_summary_is_taken_from_either_stream(self, monkeypatch: pytest.MonkeyPatch) -> None: + """stdout and stderr are joined before parsing. + + pytest writes the summary to stdout, but `uv run` prepends its own + chatter on stderr and a plugin can move things about. Reading only one + stream would turn a working checkout into "could not count". + """ + monkeypatch.setattr( + check_test_count.subprocess, "run", lambda *_a, **_k: _Ran(stderr="7 tests collected") + ) + assert collect_count(Path()) == 7 + + def test_a_broken_collection_refuses_and_shows_the_output( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Exit code is not the question; whether a number came back is. + + A non-zero exit with a usable summary is possible, and a zero exit with + none is too — so the parse decides, and the refusal carries the text so + the failure is actionable from the CI log alone. + """ + monkeypatch.setattr( + check_test_count.subprocess, + "run", + lambda *_a, **_k: _Ran(stdout="no tests collected in 0.00s", returncode=4), + ) + root = Path() + with pytest.raises(CannotCountError, match="not a count of zero"): + collect_count(root) + + +class TestTheCliContract: + """Exit codes, and the line that says whether the floor was in force.""" + + def _wire( + self, + monkeypatch: pytest.MonkeyPatch, + *, + actual: int, + floor: int | None, + here: int, + ) -> None: + monkeypatch.setattr(check_test_count, "collect_count", lambda _r: actual) + monkeypatch.setattr(check_test_count, "baseline_on", lambda _ref, _r: floor) + monkeypatch.setattr(check_test_count, "baseline_here", lambda _r: here) + monkeypatch.setattr(sys, "argv", ["check_test_count.py"]) + + def test_check_reports_whether_the_floor_applied(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The flag travels with the result instead of being re-derived. + + A clean run with no floor and a clean run with a floor are different + outcomes, and both produce an empty problem list. + """ + self._wire(monkeypatch, actual=1957, floor=1932, here=1932) + assert check("origin/main", Path()) == ([], True) + self._wire(monkeypatch, actual=1957, floor=None, here=1932) + assert check("origin/main", Path()) == ([], False) + + def test_a_healthy_suite_exits_zero_and_says_nothing( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + self._wire(monkeypatch, actual=1957, floor=1932, here=1932) + assert main() == 0 + assert capsys.readouterr().err == "" + + def test_a_shrunken_suite_exits_one_and_prints_the_delta( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + self._wire(monkeypatch, actual=1264, floor=1932, here=1932) + assert main() == 1 + assert "668 have gone missing" in capsys.readouterr().err + + def test_a_missing_prior_is_announced_on_an_otherwise_green_run( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + """Green because nothing is wrong, and loud because nothing was checked. + + Without the notice this run is indistinguishable in the log from one + where the anti-deletion rule ran and passed — which is the confusion the + whole file exists to remove. + """ + self._wire(monkeypatch, actual=1957, floor=None, here=1957) + assert main() == 0 + assert "the anti-deletion floor did not apply" in capsys.readouterr().err + + def test_being_unable_to_count_exits_two_rather_than_zero( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + """ "We could not tell" is not "the suite is intact". + + Its own exit code so a caller cannot read the failure as a pass, and so + it is distinguishable from a genuine shrink at exit 1. + """ + + def _boom(_r: Path) -> int: + _msg = "pytest exploded" + raise CannotCountError(_msg) + + self._wire(monkeypatch, actual=0, floor=1932, here=1932) + monkeypatch.setattr(check_test_count, "collect_count", _boom) + assert main() == 2 + assert "could not run" in capsys.readouterr().err + + def test_the_repository_currently_satisfies_its_own_floor() -> None: """The recorded baseline is not above what this checkout actually collects. From 81ddfed0d88510c7dccbd7a94060cfec994f7ad7 Mon Sep 17 00:00:00 2001 From: zaebee Date: Mon, 17 Aug 2026 10:59:46 +0000 Subject: [PATCH 3/4] fix(ci): validate the base ref before it reaches git (pythonsecurity:S8705) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud put Security Rating at C on this PR: both subprocess calls in baseline_on take the ref straight from argv. Fixed rather than exempted — scripts/ already carries exemptions for S8707 and S2083, and adding a third would be silencing the one finding here with substance. Nothing is passed through a shell, so the metacharacter cases are not exploitable. The leading dash is: '--upload-pack=...' reaches git as an option, not a revision — the same argument-injection shape evidence.py guards against on changed-file paths, and the reason that guard exists there is that a value which never looked like its type has no business reaching a subprocess, exploitable or not. Refused rather than sanitised. Stripping a bad ref into something git accepts would compare against a revision nobody asked for, which is this file's own failure mode wearing a different hat. Co-Authored-By: Claude Opus 5 --- scripts/check_test_count.py | 31 +++++++++++++++++++++++++++++ tests/unit/test_check_test_count.py | 27 +++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/scripts/check_test_count.py b/scripts/check_test_count.py index 711dffb0..70da2dfb 100644 --- a/scripts/check_test_count.py +++ b/scripts/check_test_count.py @@ -103,6 +103,36 @@ def collect_count(repo_root: Path) -> int: return parse_collected(result.stdout + result.stderr) +#: What may be handed to git as a revision. Deliberately narrower than git's own +#: rules — it has to admit `main`, `origin/main`, `HEAD`, a sha and +#: `release/1.2`, and nothing else needs to work. +#: +#: The first character is the point. A value beginning with `-` reaches git as a +#: *flag* rather than a revision, which is the same argument-injection shape +#: `evidence.py` guards against on changed-file paths. Nothing here is passed +#: through a shell, so this is not about quoting; it is that a string which +#: never looked like a ref has no business reaching a subprocess, exploitable or +#: not (pythonsecurity:S8705). +_REF = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._/-]*$") + + +def _validated_ref(ref: str) -> str: + """`ref` unchanged, or a refusal — never a sanitised version of it. + + Rejected rather than stripped: silently rewriting `--upload-pack=x` into + something git accepts would compare against a ref nobody asked for, and this + whole file exists to stop a check reporting on the wrong subject. + """ + if not _REF.match(ref): + _msg = ( + f"{ref!r} is not a usable git revision. It must start with a letter or digit and " + f"hold only letters, digits, and `._/-` — a value beginning with `-` reaches git " + f"as an option rather than a revision." + ) + raise CannotCountError(_msg) + return ref + + def baseline_on(ref: str, repo_root: Path) -> int | None: """The baseline as recorded on `ref`, or None when `ref` predates the file. @@ -118,6 +148,7 @@ def baseline_on(ref: str, repo_root: Path) -> int | None: resolving the ref first rather than by matching git's error text, which would not distinguish "unknown ref" from "path not in this tree". """ + ref = _validated_ref(ref) resolved = subprocess.run( ["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"], cwd=repo_root, diff --git a/tests/unit/test_check_test_count.py b/tests/unit/test_check_test_count.py index 34992b0b..f382ae88 100644 --- a/tests/unit/test_check_test_count.py +++ b/tests/unit/test_check_test_count.py @@ -20,6 +20,7 @@ MAX_DRIFT, CannotCountError, _as_count, + _validated_ref, baseline_here, baseline_on, check, @@ -127,6 +128,32 @@ def test_a_ref_that_carries_the_baseline_returns_it(self, tmp_path: Path) -> Non _commit_repo(tmp_path, baseline="1932\n") assert baseline_on("HEAD", tmp_path) == 1932 + @pytest.mark.parametrize( + "ref", + [ + pytest.param("--upload-pack=touch /tmp/x", id="reads-as-an-option"), + pytest.param("-main", id="leading-dash"), + pytest.param("main; rm -rf /", id="shell-metacharacters"), + pytest.param("main$(id)", id="substitution"), + pytest.param("", id="empty"), + ], + ) + def test_a_ref_that_is_not_a_ref_never_reaches_git(self, ref: str) -> None: + """Refused before the subprocess, and refused rather than sanitised. + + Nothing here goes through a shell, so the metacharacter cases are not + exploitable — but the leading-dash case is real: git reads it as an + option. Stripping it instead would compare against a revision nobody + asked for, which is this file's own failure mode. + """ + with pytest.raises(CannotCountError, match="not a usable git revision"): + baseline_on(ref, REPO_ROOT) + + @pytest.mark.parametrize("ref", ["main", "origin/main", "HEAD", "release/1.2", "98380bc"]) + def test_the_refs_actually_used_are_accepted(self, ref: str) -> None: + """The guard must not be so tight that it rejects the real inputs.""" + assert _validated_ref(ref) == ref + class TestTheFourRules: """Each rule closes a different route, so each is asserted on its own.""" From a12ef0e12db1ba52c55a606e14e223e63f87561f Mon Sep 17 00:00:00 2001 From: zaebee Date: Mon, 17 Aug 2026 11:10:39 +0000 Subject: [PATCH 4/4] fix(ci): take gemini's five on the test-count floor (#405) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The best of them: an absent uv or git raised FileNotFoundError, Python exited 1, and 1 is the code for "the suite shrank" — so a machine without the toolchain would have reported as a deletion. Telling "could not find out" from "tests are missing" is this file's whole job, and it was not doing it in its own plumbing. Both call sites now go through _run and refuse at exit 2. Last match rather than first: pytest's summary is the final line, so a plugin printing something count-shaped earlier is not the answer. Taken with a change, because as suggested it inverts on the case the code was written for — stderr is appended after stdout, so "last match" over the joined text would let uv's chatter outrank the real summary. stdout is read first and the joined form only as a fallback, which is also what the refusal reports. Relative revisions admitted: HEAD~1, HEAD^, origin/main~2. CI passes origin/, but those are what a person debugging locally types, and none of them introduces an option or a shell. Writing that test showed the guard encoded the wrong rule. Requiring the first character to be alphanumeric rejected "@", a legal revision meaning HEAD, while the property that matters is only that a ref must not begin with a dash. The pattern now says that directly. Co-Authored-By: Claude Opus 5 --- scripts/check_test_count.py | 99 +++++++++++++++++------------ tests/unit/test_check_test_count.py | 75 +++++++++++++++++++++- 2 files changed, 133 insertions(+), 41 deletions(-) diff --git a/scripts/check_test_count.py b/scripts/check_test_count.py index 70da2dfb..0836a93c 100644 --- a/scripts/check_test_count.py +++ b/scripts/check_test_count.py @@ -73,9 +73,14 @@ def parse_collected(output: str) -> int: `137/1932`, and 137 is how many matched a filter rather than how many exist. A floor fed the filtered number would fail on every filtered run and, worse, would pass a real deletion whenever the filter happened to be narrow. + + The **last** match wins. pytest's summary is the final line, so anything + else matching came earlier and is not the answer — a plugin or a warning + printing something count-shaped would otherwise be read as the count. + Raised in review of #409. """ - match = _COLLECTED.search(output) - if match is None: + matches = list(_COLLECTED.finditer(output)) + if not matches: _msg = ( "Could not find a collected-test count in pytest's output. The last line is " "normally ' tests collected'; 'no tests collected' means collection failed " @@ -83,37 +88,62 @@ def parse_collected(output: str) -> int: f"--- output ---\n{output.strip()[-2000:]}" ) raise CannotCountError(_msg) - return int(match.group(2)) + return int(matches[-1].group(2)) + + +def _run(command: list[str], repo_root: Path) -> subprocess.CompletedProcess[str]: + """Run a command, turning a missing executable into this module's refusal. + + Without this, an absent `uv` or `git` raises `FileNotFoundError`, Python + exits 1, and **1 is the code for "the suite shrank"** — so a machine without + the toolchain would report as a deletion. Telling "we could not find out" + apart from "tests are missing" is the entire job of this file, and it cannot + make that distinction everywhere except in its own plumbing. Raised in + review of #409. + """ + try: + return subprocess.run(command, cwd=repo_root, capture_output=True, text=True, check=False) + except FileNotFoundError as exc: + _msg = f"Could not run {command[0]!r}: {exc.strerror}. It must be on PATH." + raise CannotCountError(_msg) from exc def collect_count(repo_root: Path) -> int: """How many tests pytest can collect right now.""" - result = subprocess.run( - ["uv", "run", "--frozen", "pytest", "--collect-only", "-q"], - cwd=repo_root, - capture_output=True, - text=True, - check=False, - ) # Not `check=True`: a non-zero exit with a usable summary line is possible, # and the exit code is not the question being asked. The parse decides, and - # it refuses loudly when there is nothing to parse — so the output is passed - # through either way rather than being thrown away with an exception whose - # message would not contain it. - return parse_collected(result.stdout + result.stderr) + # it refuses loudly when there is nothing to parse. + result = _run(["uv", "run", "--frozen", "pytest", "--collect-only", "-q"], repo_root) + # stdout first, and only then the two joined. pytest writes its summary to + # stdout while `uv` writes its own chatter to stderr, and stderr is appended + # *after* — so "take the last match" is right within a stream and wrong + # across them. The joined form is still what a failure reports, so the + # refusal carries both. + try: + return parse_collected(result.stdout) + except CannotCountError: + return parse_collected(result.stdout + result.stderr) -#: What may be handed to git as a revision. Deliberately narrower than git's own -#: rules — it has to admit `main`, `origin/main`, `HEAD`, a sha and -#: `release/1.2`, and nothing else needs to work. +#: What may be handed to git as a revision. Narrower than git's own rules, but +#: wide enough for the forms people actually type: `main`, `origin/main`, `HEAD`, +#: a sha, `release/1.2`, and the relative ones — `HEAD~1`, `HEAD^`, `main@{u}` +#: minus the braces. The relative characters were added in review of #409; +#: `origin/` is what CI passes, but `HEAD~1` is what a person debugging +#: this locally reaches for, and `~ ^ @` introduce no option and no shell. +#: +#: The leading dash is the point, and the pattern now says so directly. An +#: earlier version required the first character to be alphanumeric, which is a +#: stricter rule wearing the same clothes — it also rejected `@`, a legal +#: revision meaning HEAD, as its own test discovered. Git validates the rest; +#: what git cannot do is tell that an argument was meant as a revision, because +#: a value beginning with `-` reaches it as a *flag*. Same argument-injection +#: shape `evidence.py` guards against on changed-file paths. #: -#: The first character is the point. A value beginning with `-` reaches git as a -#: *flag* rather than a revision, which is the same argument-injection shape -#: `evidence.py` guards against on changed-file paths. Nothing here is passed -#: through a shell, so this is not about quoting; it is that a string which -#: never looked like a ref has no business reaching a subprocess, exploitable or -#: not (pythonsecurity:S8705). -_REF = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._/-]*$") +#: Nothing here is passed through a shell, so this is not about quoting; it is +#: that a string which never looked like a ref has no business reaching a +#: subprocess, exploitable or not (pythonsecurity:S8705). +_REF = re.compile(r"^(?!-)[A-Za-z0-9._/~^@-]+$") def _validated_ref(ref: str) -> str: @@ -125,9 +155,9 @@ def _validated_ref(ref: str) -> str: """ if not _REF.match(ref): _msg = ( - f"{ref!r} is not a usable git revision. It must start with a letter or digit and " - f"hold only letters, digits, and `._/-` — a value beginning with `-` reaches git " - f"as an option rather than a revision." + f"{ref!r} is not a usable git revision. It must not begin with `-`, and may hold " + f"only letters, digits, and `._/~^@-` — a value beginning with `-` reaches git as " + f"an option rather than a revision." ) raise CannotCountError(_msg) return ref @@ -149,12 +179,7 @@ def baseline_on(ref: str, repo_root: Path) -> int | None: would not distinguish "unknown ref" from "path not in this tree". """ ref = _validated_ref(ref) - resolved = subprocess.run( - ["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"], - cwd=repo_root, - capture_output=True, - check=False, - ) + resolved = _run(["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"], repo_root) if resolved.returncode: _msg = ( f"{ref} does not resolve. The floor has to come from the base branch, so a " @@ -162,13 +187,7 @@ def baseline_on(ref: str, repo_root: Path) -> int | None: f"falling back to this branch's own copy is the evasion this check closes." ) raise CannotCountError(_msg) - result = subprocess.run( - ["git", "show", f"{ref}:{BASELINE_PATH.as_posix()}"], - cwd=repo_root, - capture_output=True, - text=True, - check=False, - ) + result = _run(["git", "show", f"{ref}:{BASELINE_PATH.as_posix()}"], repo_root) if result.returncode: return None return _as_count(result.stdout, f"{ref}:{BASELINE_PATH}") diff --git a/tests/unit/test_check_test_count.py b/tests/unit/test_check_test_count.py index f382ae88..29e94d25 100644 --- a/tests/unit/test_check_test_count.py +++ b/tests/unit/test_check_test_count.py @@ -76,6 +76,17 @@ def test_no_tests_collected_refuses_rather_than_returning_zero(self) -> None: with pytest.raises(CannotCountError, match="not a count of zero"): parse_collected("no tests collected in 0.00s") + def test_the_last_match_wins(self) -> None: + """pytest's summary is the final line; anything earlier is not the answer. + + Raised in review of #409. A plugin or a warning printing something + count-shaped would otherwise be read as the count, and the floor would + then bound the wrong number in whichever direction that number happened + to fall. + """ + output = "10 tests collected\nsome plugin chatter\n1932 tests collected in 1.83s" + assert parse_collected(output) == 1932 + def test_the_refusal_carries_the_output_it_could_not_parse(self) -> None: """A "could not run" with no output is a bug report nobody can action.""" with pytest.raises(CannotCountError, match="ImportError: cannot import name"): @@ -149,7 +160,23 @@ def test_a_ref_that_is_not_a_ref_never_reaches_git(self, ref: str) -> None: with pytest.raises(CannotCountError, match="not a usable git revision"): baseline_on(ref, REPO_ROOT) - @pytest.mark.parametrize("ref", ["main", "origin/main", "HEAD", "release/1.2", "98380bc"]) + @pytest.mark.parametrize( + "ref", + [ + "main", + "origin/main", + "HEAD", + "release/1.2", + "98380bc", + # Relative forms, admitted in review of #409. CI passes + # `origin/`, but these are what a person debugging locally + # types, and none of them introduces an option or a shell. + "HEAD~1", + "HEAD^", + "origin/main~2", + "@", + ], + ) def test_the_refs_actually_used_are_accepted(self, ref: str) -> None: """The guard must not be so tight that it rejects the real inputs.""" assert _validated_ref(ref) == ref @@ -231,6 +258,16 @@ def __init__(self, stdout: str = "", stderr: str = "", returncode: int = 0) -> N self.returncode = returncode +class _stub_subprocess: # noqa: N801 — stands in for a module, so it is named like one + """A `subprocess` module whose `run` always returns the same result.""" + + def __init__(self, result: _Ran) -> None: + self._result = result + + def run(self, *_args: object, **_kwargs: object) -> _Ran: + return self._result + + class TestCollectingTheCount: """The one place that shells out to pytest.""" @@ -246,6 +283,42 @@ def test_the_summary_is_taken_from_either_stream(self, monkeypatch: pytest.Monke ) assert collect_count(Path()) == 7 + def test_stdout_outranks_stderr_when_both_carry_a_count( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Why "take the last match" is right within a stream and wrong across two. + + pytest writes its summary to stdout and `uv` writes its chatter to + stderr, and the two are joined with stderr last — so a naive last-match + over the joined text would let stderr outrank the real summary. Reading + stdout first removes the question. + """ + monkeypatch.setattr( + check_test_count, + "subprocess", + _stub_subprocess(_Ran(stdout="1932 tests collected", stderr="7 tests collected")), + ) + assert collect_count(Path()) == 1932 + + def test_a_missing_executable_is_a_could_not_tell_not_a_shrink( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Absent `uv` or `git` must not exit 1, which is the code for a deletion. + + Raised in review of #409, and the sharpest of the six: an uncaught + FileNotFoundError exits 1, and 1 means "tests have gone missing". A + machine without the toolchain would have reported as a deletion — the + one confusion this module exists to prevent, in its own plumbing. + """ + + def _absent(*_a: object, **_k: object) -> None: + raise FileNotFoundError(2, "No such file or directory") + + monkeypatch.setattr(check_test_count.subprocess, "run", _absent) + root = Path() + with pytest.raises(CannotCountError, match="must be on PATH"): + collect_count(root) + def test_a_broken_collection_refuses_and_shows_the_output( self, monkeypatch: pytest.MonkeyPatch ) -> None: