From c24c5dd050d002471eb17d988768ce79aa41aeb1 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 23 Aug 2026 12:38:20 -0700 Subject: [PATCH 1/2] Never Trust a Failed git ls-files Call's stdout, Even Non-Empty Real, HIGH-severity finding from qodo-code-review on PR #959 (the develop -> main promotion PR carrying #958/#960's exclude-globs work). `tracked()` printed git's own stderr on a nonzero exit but still parsed and returned `result.stdout` regardless. `main()` only checks `if not files:`, so a failed `git ls-files` call that happened to emit any stdout before failing would be read as a successful, complete scan, letting every check run against a silently incomplete file list. ## The fix `tracked()` now returns `[]` unconditionally on a nonzero exit, after printing stderr, never falling through to parse stdout on that path. ## Verified Added `test_a_failed_call_is_never_trusted_even_with_nonempty_stdout` (a mocked nonzero exit carrying non-empty stdout, asserting `tracked()` still returns `[]`). Full test suite (807 tests), ruff check and format, mypy, `repo_gate.py` against this checkout, and `prose_lint.py --diff origin/develop` all pass clean. --- .github/actions/repo-gate/repo_gate.py | 10 ++++++---- scripts/tests/test_repo_gate.py | 7 +++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/actions/repo-gate/repo_gate.py b/.github/actions/repo-gate/repo_gate.py index 46508234..7e700aab 100755 --- a/.github/actions/repo-gate/repo_gate.py +++ b/.github/actions/repo-gate/repo_gate.py @@ -127,10 +127,12 @@ def tracked(root: Path, exclude: list[str] | None = None) -> list[str]: if exclude: args += ["--", *(f":!{pattern}" for pattern in exclude)] result = subprocess.run(args, capture_output=True, text=True, check=False) - # A failed command and a valid empty result would otherwise look the same to the caller. - # The command's own stderr (a bad pathspec, an unreadable root) is surfaced rather than dropped. - if result.returncode != 0 and result.stderr.strip(): - print(f"git ls-files failed: {result.stderr.strip()}", file=sys.stderr) + if result.returncode != 0: + # A failed command's stdout is never trusted, even where it is non-empty. + # A partial listing read as complete is a scan that missed files and said nothing. + if result.stderr.strip(): + print(f"git ls-files failed: {result.stderr.strip()}", file=sys.stderr) + return [] return [l for l in result.stdout.split("\n") if l] diff --git a/scripts/tests/test_repo_gate.py b/scripts/tests/test_repo_gate.py index ec9649cb..c2137429 100755 --- a/scripts/tests/test_repo_gate.py +++ b/scripts/tests/test_repo_gate.py @@ -617,6 +617,13 @@ def test_a_failed_ls_files_call_prints_gits_own_error(self) -> None: self.assertIn("git ls-files failed", err.getvalue()) self.assertIn("Invalid pathspec magic", err.getvalue()) + def test_a_failed_call_is_never_trusted_even_with_nonempty_stdout(self) -> None: + """A partial listing read as a complete one is a scan that missed files silently.""" + proc = subprocess.CompletedProcess([], 128, "kept.py\n", "fatal: something went wrong\n") + with mock.patch.object(repo_gate.subprocess, "run", return_value=proc): + files = repo_gate.tracked(self.tmp, ["vendor/**"]) + self.assertEqual([], files) + class TestHarness(unittest.TestCase): def test_this_module_collects_a_plausible_number_of_cases(self) -> None: From 0739f66b6e1f61990087efc705fa6fbe55d4df21 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 23 Aug 2026 12:45:59 -0700 Subject: [PATCH 2/2] Print a Reason Even When git ls-files Fails With Empty stderr Real finding from qodo-code-review on PR #961's own round-2 review, reproduced before the fix. Also fixes a test-hygiene finding from the same round: the new stdout-distrust test left tracked()'s mocked stderr print unredirected, cluttering test output. `tracked()` only printed a failure reason when `result.stderr` was non-empty, so a `git ls-files` call that exits non-zero with empty stderr (rare, but not excludable) now returns `[]` with no explanation at all, contradicting main()'s own comment that assumes a reason was already printed. ## The fix `tracked()` always prints a reason on a nonzero exit: git's own stderr when present, otherwise `exit , no stderr`. ## Verified Added `test_a_failure_with_no_stderr_still_prints_a_reason` (a mocked nonzero exit with empty stdout and stderr, asserting the exit code appears in the printed reason). Wrapped the existing `test_a_failed_call_is_never_trusted_even_with_nonempty_stdout` in `contextlib.redirect_stderr` to stop it leaking to test output. Full test suite (808 tests), ruff check and format, mypy, `repo_gate.py` against this checkout, and `prose_lint.py --diff origin/develop` all pass clean. --- .github/actions/repo-gate/repo_gate.py | 4 ++-- scripts/tests/test_repo_gate.py | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/.github/actions/repo-gate/repo_gate.py b/.github/actions/repo-gate/repo_gate.py index 7e700aab..375a78df 100755 --- a/.github/actions/repo-gate/repo_gate.py +++ b/.github/actions/repo-gate/repo_gate.py @@ -130,8 +130,8 @@ def tracked(root: Path, exclude: list[str] | None = None) -> list[str]: if result.returncode != 0: # A failed command's stdout is never trusted, even where it is non-empty. # A partial listing read as complete is a scan that missed files and said nothing. - if result.stderr.strip(): - print(f"git ls-files failed: {result.stderr.strip()}", file=sys.stderr) + reason = result.stderr.strip() or f"exit {result.returncode}, no stderr" + print(f"git ls-files failed: {reason}", file=sys.stderr) return [] return [l for l in result.stdout.split("\n") if l] diff --git a/scripts/tests/test_repo_gate.py b/scripts/tests/test_repo_gate.py index c2137429..0ce17d69 100755 --- a/scripts/tests/test_repo_gate.py +++ b/scripts/tests/test_repo_gate.py @@ -620,9 +620,24 @@ def test_a_failed_ls_files_call_prints_gits_own_error(self) -> None: def test_a_failed_call_is_never_trusted_even_with_nonempty_stdout(self) -> None: """A partial listing read as a complete one is a scan that missed files silently.""" proc = subprocess.CompletedProcess([], 128, "kept.py\n", "fatal: something went wrong\n") - with mock.patch.object(repo_gate.subprocess, "run", return_value=proc): + with ( + mock.patch.object(repo_gate.subprocess, "run", return_value=proc), + contextlib.redirect_stderr(io.StringIO()), + ): + files = repo_gate.tracked(self.tmp, ["vendor/**"]) + self.assertEqual([], files) + + def test_a_failure_with_no_stderr_still_prints_a_reason(self) -> None: + """Empty stderr on a nonzero exit must not read as a silent, unexplained empty scan.""" + proc = subprocess.CompletedProcess([], 1, "", "") + with ( + mock.patch.object(repo_gate.subprocess, "run", return_value=proc), + contextlib.redirect_stderr(io.StringIO()) as err, + ): files = repo_gate.tracked(self.tmp, ["vendor/**"]) self.assertEqual([], files) + self.assertIn("git ls-files failed", err.getvalue()) + self.assertIn("exit 1", err.getvalue()) class TestHarness(unittest.TestCase):