From 6100565578fae681f839c81de07600a0da9a7bf6 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 12 May 2026 04:51:57 +0000 Subject: [PATCH] fix: regression test now actually exercises --staged in temp repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Devin Review on PR #11 ("Staged regression test passes vacuously — git diff runs in real repo, not temp repo"). Root cause: scripts/math-first/require_math_evidence.py hardcoded cwd=REPO_ROOT on the git subprocess. REPO_ROOT is computed from __file__, which always resolves to the REAL repo path -- even when the test invokes the script with cwd=tmp_path. So 'git diff --cached --name-only' queried the real (clean) repo, returned an empty list, and the script exited 0 via the noop short-circuit. The regression test then printed OK while never actually exercising the --staged classification logic. Fix: Introduce _git_toplevel() in require_math_evidence.py that runs 'git rev-parse --show-toplevel' from the *inherited* cwd. Both _files_from_staged() and _files_from_git_diff() now use that toplevel as their subprocess cwd. Pre-commit and CI both invoke the script from inside the repo root anyway, so behaviour in production is unchanged; only the test (and any other caller that runs the script from a non-Research-Stack directory) now works correctly. Test hardening: test_require_math_evidence.py's staged regression now drives the script via plain subprocess with cwd=tmp_path (no runpy wrapper, no os.chdir trickery) and covers three sub-cases against the same temp repo: (a) math-track-only staged -> expect exit 1 (negative) (b) math-track + receipt staged -> expect exit 0 (the bug) (c) math-track + claims.yaml -> expect exit 0 (registry) Sub-case (a) is the key addition: with the pre-fix script, this case wrongly returns 0 (real repo is clean -> noop), so the test fails loudly. Verified locally by temporarily reverting the cwd fix -- the test correctly reports 'FAIL staged_regression_negative: expected exit 1, got 0'. Docs: docs/math-first-tooling.md notes that receipt-required-for-math-content is a no-op in the CI pre-commit job (pre-commit's --from-ref/--to-ref mode does not touch the index, so --staged returns an empty list and the hook exits 0). PR-scope enforcement of the same rule lives in the dedicated require-evidence CI job. This addresses the Devin Review info comment on .pre-commit-config.yaml. Co-Authored-By: Allaun Silverfox --- docs/math-first-tooling.md | 6 +- scripts/math-first/require_math_evidence.py | 48 ++++++- .../math-first/test_require_math_evidence.py | 122 +++++++++++------- 3 files changed, 126 insertions(+), 50 deletions(-) diff --git a/docs/math-first-tooling.md b/docs/math-first-tooling.md index 5dfe93cd..2806927e 100644 --- a/docs/math-first-tooling.md +++ b/docs/math-first-tooling.md @@ -184,7 +184,11 @@ math-first surface. It has three jobs: 3. **`pre-commit`** — runs every pre-commit hook against the PR's range of changed files (`pre-commit run --from-ref --to-ref HEAD`), so the guardrails are honoured even when a contributor has not installed the - hooks locally. + hooks locally. Note: `receipt-required-for-math-content` is a no-op in + this CI job because pre-commit's `--from-ref / --to-ref` mode does not + stage anything in the index, so the script's `--staged` lookup returns + an empty file list and exits 0. PR-scope enforcement of the same rule + lives in the dedicated `require-evidence` job above. The pre-existing `wolfram-verification.yml` workflow continues to police mathematical formulas inside Lean source for missing Wolfram Alpha diff --git a/scripts/math-first/require_math_evidence.py b/scripts/math-first/require_math_evidence.py index d551eb20..b0115a09 100755 --- a/scripts/math-first/require_math_evidence.py +++ b/scripts/math-first/require_math_evidence.py @@ -40,8 +40,42 @@ import subprocess import sys from pathlib import Path +# Only used for logging/help -- *not* as the subprocess ``cwd`` for the git +# calls below. Hardcoding the subprocess cwd to this path caused the +# regression test in ``test_require_math_evidence.py`` to pass vacuously +# because the test stages files in a *temp* repo and expects ``git diff +# --cached`` to query that temp repo, not the real one. Pre-commit and CI +# both happen to invoke the script from inside the repo root, so letting +# the git subprocess inherit cwd is correct in every real-world case. REPO_ROOT = Path(__file__).resolve().parents[2] + +def _git_toplevel(cwd: Path | None = None) -> Path: + """Return the absolute path of the git working tree containing ``cwd``. + + Defaults to the current working directory. The script never assumes the + git repo lives at ``REPO_ROOT`` because that would be wrong when the + script is run from a different working tree -- notably, the temp repo + set up by ``test_require_math_evidence.py``. + """ + cmd = ["git", "rev-parse", "--show-toplevel"] + try: + result = subprocess.run( + cmd, + capture_output=True, + text=True, + check=True, + cwd=str(cwd) if cwd is not None else None, + ) + except subprocess.CalledProcessError as exc: + print( + f"error: `{' '.join(cmd)}` failed: {exc.stderr.strip()}", + file=sys.stderr, + ) + raise SystemExit(2) + return Path(result.stdout.strip()) + + MATH_TRACK_PREFIXES: tuple[str, ...] = ( "0-Core-Formalism/lean/Semantics/", "6-Documentation/docs/distilled/", @@ -82,9 +116,12 @@ def _is_evidence(path: str) -> bool: def _files_from_git_diff(base_ref: str) -> list[str]: + cwd = _git_toplevel() cmd = ["git", "diff", "--name-only", f"{base_ref}...HEAD"] try: - result = subprocess.run(cmd, capture_output=True, text=True, check=True, cwd=REPO_ROOT) + result = subprocess.run( + cmd, capture_output=True, text=True, check=True, cwd=str(cwd) + ) except subprocess.CalledProcessError as exc: print(f"error: `{' '.join(cmd)}` failed: {exc.stderr.strip()}", file=sys.stderr) raise SystemExit(2) @@ -99,10 +136,17 @@ def _files_from_staged() -> list[str]: ``files`` filter. This is important because pre-commit otherwise strips receipts and ``claims.yaml`` from the argv before the script ever sees them, which would cause the evidence check to falsely fail. + + The subprocess cwd is the actual git toplevel containing the *current* + working directory, not a hardcoded path -- so the script works + correctly inside the regression test's temp repo too. """ + cwd = _git_toplevel() cmd = ["git", "diff", "--cached", "--name-only"] try: - result = subprocess.run(cmd, capture_output=True, text=True, check=True, cwd=REPO_ROOT) + result = subprocess.run( + cmd, capture_output=True, text=True, check=True, cwd=str(cwd) + ) except subprocess.CalledProcessError as exc: print(f"error: `{' '.join(cmd)}` failed: {exc.stderr.strip()}", file=sys.stderr) raise SystemExit(2) diff --git a/scripts/math-first/test_require_math_evidence.py b/scripts/math-first/test_require_math_evidence.py index f51e397d..38776de3 100755 --- a/scripts/math-first/test_require_math_evidence.py +++ b/scripts/math-first/test_require_math_evidence.py @@ -123,71 +123,99 @@ def _run_mutex_check() -> int: def _run_staged_regression() -> int: - """Regression test: ``--staged`` sees the entire index, not just argv. + """Regression for the pre-commit ``files``-filter bug. - Reproduces the pre-commit ``files`` filter bug. Without ``--staged`` - invoking the script with only the math-track filename (as pre-commit - would, after filtering) would falsely fail. With ``--staged`` the - script reads the full index and passes. + Builds a throwaway git repo so this test is self-contained -- the + test does not depend on the state of the real repo's index. The + script is run with ``cwd=tmp_path`` (NOT via ``runpy``) so that + ``git rev-parse --show-toplevel`` inside the script resolves to the + temp repo, and ``git diff --cached`` queries the temp repo's index. + + Three sub-cases: + (a) math-track-only staged -> expect exit 1 + (proves the script actually evaluates classification logic; + without (a) the next sub-case could pass vacuously by + short-circuiting on an empty diff) + (b) math-track + receipt staged -> expect exit 0 + (the original pre-commit ``files``-filter bug) + (c) math-track + claims.yaml staged -> expect exit 0 + (verifies the registry-update path) """ failures = 0 with tempfile.TemporaryDirectory() as tmp: - tmp_path = Path(tmp) + tmp_path = Path(tmp).resolve() _git("init", "-q", cwd=tmp_path) - # Lay out a minimal mirror of the math-track and evidence surfaces. math_track = tmp_path / "6-Documentation" / "docs" / "distilled" / "Spec.md" math_track.parent.mkdir(parents=True, exist_ok=True) math_track.write_text("# math claim\n") receipt = ( - tmp_path - / "shared-data" - / "artifacts" - / "deepseek_review" - / "x.receipt.json" + tmp_path / "shared-data" / "artifacts" / "deepseek_review" / "x.receipt.json" ) receipt.parent.mkdir(parents=True, exist_ok=True) receipt.write_text("{}\n") - # Stage both files. (The script does not parse the receipts here -- - # it only classifies by path.) - _git("add", str(math_track.relative_to(tmp_path)), cwd=tmp_path) - _git("add", str(receipt.relative_to(tmp_path)), cwd=tmp_path) + claims = tmp_path / "claims.yaml" + claims.write_text("claims: []\n") - # Run with --staged from inside the temp repo. The script will - # rebase REPO_ROOT off its own location (the real repo), but the - # ``git diff --cached`` call uses subprocess cwd, which we override - # below by chdir-ing. - # We invoke the script with a small wrapper script so we control cwd. - wrapper = tmp_path / "run_wrapper.py" - wrapper.write_text( - "import os, runpy, sys\n" - f"os.chdir({str(tmp_path)!r})\n" - "sys.argv = ['require_math_evidence.py', '--staged']\n" - f"runpy.run_path({str(SCRIPT)!r}, run_name='__main__')\n" + def _stage_only(*relpaths: str) -> None: + # Reset the index to a clean state, then stage exactly the + # supplied paths. ``git reset`` is safe here -- the temp repo + # has no commits, so there is no "HEAD" to reset against. We + # instead remove everything currently in the index. + _git("rm", "--cached", "-rf", "--ignore-unmatch", ".", cwd=tmp_path) + for relpath in relpaths: + _git("add", "--", relpath, cwd=tmp_path) + + def _invoke() -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(SCRIPT), "--staged"], + capture_output=True, + text=True, + cwd=str(tmp_path), + ) + + def _assert(label: str, expected_exit: int, *staged: str) -> int: + _stage_only(*staged) + indexed = _git("diff", "--cached", "--name-only", cwd=tmp_path).stdout.splitlines() + indexed = [line for line in indexed if line.strip()] + if sorted(indexed) != sorted(staged): + print(f"FAIL {label}: index does not match expected staging") + print(f" expected: {sorted(staged)}") + print(f" actual: {sorted(indexed)}") + return 1 + result = _invoke() + if result.returncode != expected_exit: + print( + f"FAIL {label}: expected exit {expected_exit}, " + f"got {result.returncode}" + ) + if result.stdout.strip(): + print(f" stdout: {result.stdout.strip()}") + if result.stderr.strip(): + print(f" stderr: {result.stderr.strip()}") + return 1 + print(f"OK {label} (exit {expected_exit})") + return 0 + + failures += _assert( + "staged_regression_negative (math-only -> FAIL)", + 1, + "6-Documentation/docs/distilled/Spec.md", ) - result = subprocess.run( - [sys.executable, str(wrapper)], - capture_output=True, - text=True, - cwd=str(tmp_path), + failures += _assert( + "staged_regression_positive_receipt (math + receipt -> OK)", + 0, + "6-Documentation/docs/distilled/Spec.md", + "shared-data/artifacts/deepseek_review/x.receipt.json", + ) + failures += _assert( + "staged_regression_positive_claims (math + claims.yaml -> OK)", + 0, + "6-Documentation/docs/distilled/Spec.md", + "claims.yaml", ) - # The script's REPO_ROOT is computed from its own __file__ (the real - # repo), so it would join 6-Documentation/... onto the real repo - # path. That's fine -- ``_is_math_track`` and ``_is_evidence`` - # operate on path *strings*, not on disk existence. The relevant - # invariant is that the staged file list (computed via - # ``git diff --cached`` in tmp_path) contains BOTH files. - if result.returncode != 0: - failures += 1 - print("FAIL staged_regression: expected exit 0, got", result.returncode) - if result.stdout.strip(): - print(f" stdout: {result.stdout.strip()}") - if result.stderr.strip(): - print(f" stderr: {result.stderr.strip()}") - else: - print("OK staged_regression (math-track + receipt both staged -> exit 0)") return failures