From 57dab8bf74e1f3d6fa6fdb7dc383ab82128bb85b Mon Sep 17 00:00:00 2001 From: LeSingh1 Date: Sat, 8 Aug 2026 18:49:23 -0700 Subject: [PATCH] fix(toolshed): keep the pre-commit-installed check advisory, not fatal The module docstring says this check should "Warn (without failing)", and main() returns 0 on both of its branches. Three paths contradict that: 1. `_git_hooks_dir` runs `git rev-parse --git-path hooks` with `check=True`. Outside a work tree git exits 128, so the advisory hook raises CalledProcessError and fails the whole `pre-commit run`. 2. If git is not on PATH, `subprocess.run` raises FileNotFoundError. That is not the FileNotFoundError the code catches -- that one guards `open()` -- so it escapes as a traceback. 3. The `open()` guard catches only FileNotFoundError, so any other reason the hooks path is unreadable (a directory, a dangling symlink, restrictive permissions) also escapes. All three were reproduced by calling `main()` directly: outside a git repo -> CalledProcessError: ... exit status 128 git missing on PATH -> FileNotFoundError: [Errno 2] ... 'git' hooks/pre-commit is a directory -> IsADirectoryError: [Errno 21] The hook is configured with `always_run: true`, so it fires on every local `pre-commit run`. Any of these turns a nudge to run `pre-commit install` into a hard failure of an unrelated commit. Treat "cannot determine whether the hook is installed" as "not installed": that is the situation the warning already describes, and it is the only outcome consistent with a check that cannot block. Adds tests under toolshed/tests/ covering the installed, missing, foreign-hook, no-work-tree, no-git, and unreadable-path cases. --- .github/workflows/ci-nightly.yml | 2 +- toolshed/check_precommit_installed.py | 42 ++++++--- .../tests/test_check_precommit_installed.py | 86 +++++++++++++++++++ 3 files changed, 115 insertions(+), 15 deletions(-) create mode 100644 toolshed/tests/test_check_precommit_installed.py diff --git a/.github/workflows/ci-nightly.yml b/.github/workflows/ci-nightly.yml index 0188ebf2524..bf9d42e9f56 100644 --- a/.github/workflows/ci-nightly.yml +++ b/.github/workflows/ci-nightly.yml @@ -46,7 +46,7 @@ jobs: run: | python -m pip install pytest # Standalone CI tool tests; skip repo-root conftest.py (imports cuda.pathfinder). - python -m pytest -v --noconftest ci/tools/tests + python -m pytest -v --noconftest ci/tools/tests toolshed/tests find-wheels: runs-on: ubuntu-latest diff --git a/toolshed/check_precommit_installed.py b/toolshed/check_precommit_installed.py index 19d7cfc20b6..f6145ca58df 100644 --- a/toolshed/check_precommit_installed.py +++ b/toolshed/check_precommit_installed.py @@ -20,25 +20,39 @@ MARKER = b"File generated by pre-commit" -def _git_hooks_dir() -> str: - result = subprocess.run( - ["git", "rev-parse", "--git-path", "hooks"], # noqa: S607 - capture_output=True, - check=True, - text=True, - ) - return result.stdout.strip() +def _git_hooks_dir() -> str | None: + """Resolve the git hooks directory, or None if git cannot tell us. + + Anything that stops git from answering -- not inside a work tree, git + missing from PATH, a broken repository -- leaves us unable to confirm the + hook is installed, which is the same outcome as "not installed". This + check is advisory (see the module docstring), so it must never turn a + `pre-commit run` into a failure. + """ + try: + result = subprocess.run( + ["git", "rev-parse", "--git-path", "hooks"], # noqa: S607 + capture_output=True, + check=True, + text=True, + ) + except (OSError, subprocess.SubprocessError): + return None + return result.stdout.strip() or None def main() -> int: hooks_dir = _git_hooks_dir() - hook_path = f"{hooks_dir}/pre-commit" - try: - with open(hook_path, "rb") as f: - installed = MARKER in f.read() - except FileNotFoundError: - installed = False + installed = False + if hooks_dir is not None: + try: + with open(f"{hooks_dir}/pre-commit", "rb") as f: + installed = MARKER in f.read() + except OSError: + # Unreadable, a directory, a dangling symlink: all mean we cannot + # confirm an installed hook, and none of them justify failing. + installed = False if not installed: print( diff --git a/toolshed/tests/test_check_precommit_installed.py b/toolshed/tests/test_check_precommit_installed.py new file mode 100644 index 00000000000..d204ec7a2d4 --- /dev/null +++ b/toolshed/tests/test_check_precommit_installed.py @@ -0,0 +1,86 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +from __future__ import annotations + +import os +import subprocess +import sys + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +import check_precommit_installed as mod + +WARNING_TEXT = "pre-commit git hook is not installed" + + +def _write_hook(hooks_dir, body=mod.MARKER): + hooks_dir.mkdir(parents=True, exist_ok=True) + (hooks_dir / "pre-commit").write_bytes(body) + + +@pytest.mark.agent_authored(model="claude-opus-5") +def test_installed_hook_is_quiet(tmp_path, monkeypatch, capsys): + hooks = tmp_path / "hooks" + _write_hook(hooks, b"#!/bin/sh\n# File generated by pre-commit\n") + monkeypatch.setattr(mod, "_git_hooks_dir", lambda: str(hooks)) + + assert mod.main() == 0 + assert WARNING_TEXT not in capsys.readouterr().err + + +@pytest.mark.agent_authored(model="claude-opus-5") +def test_missing_hook_warns_and_succeeds(tmp_path, monkeypatch, capsys): + monkeypatch.setattr(mod, "_git_hooks_dir", lambda: str(tmp_path / "hooks")) + + assert mod.main() == 0 + assert WARNING_TEXT in capsys.readouterr().err + + +@pytest.mark.agent_authored(model="claude-opus-5") +def test_foreign_hook_warns_and_succeeds(tmp_path, monkeypatch, capsys): + """A hook installed by something other than pre-commit lacks the marker.""" + hooks = tmp_path / "hooks" + _write_hook(hooks, b"#!/bin/sh\necho hand-written hook\n") + monkeypatch.setattr(mod, "_git_hooks_dir", lambda: str(hooks)) + + assert mod.main() == 0 + assert WARNING_TEXT in capsys.readouterr().err + + +@pytest.mark.agent_authored(model="claude-opus-5") +def test_outside_a_git_work_tree_warns_and_succeeds(tmp_path, monkeypatch, capsys): + """`git rev-parse` exits 128 outside a work tree. + + check=True turned that into a CalledProcessError traceback and a non-zero + exit, even though this check is advisory and main() returns 0 on both of + its own branches. + """ + monkeypatch.chdir(tmp_path) + + assert mod.main() == 0 + assert WARNING_TEXT in capsys.readouterr().err + + +@pytest.mark.agent_authored(model="claude-opus-5") +def test_git_missing_from_path_warns_and_succeeds(monkeypatch, capsys): + def no_git(*args, **kwargs): + raise FileNotFoundError(2, "No such file or directory: 'git'") + + monkeypatch.setattr(subprocess, "run", no_git) + + assert mod.main() == 0 + assert WARNING_TEXT in capsys.readouterr().err + + +@pytest.mark.agent_authored(model="claude-opus-5") +def test_unreadable_hook_path_warns_and_succeeds(tmp_path, monkeypatch, capsys): + """The open() guard only caught FileNotFoundError, so anything else that + makes the path unreadable escaped as a traceback.""" + hooks = tmp_path / "hooks" + (hooks / "pre-commit").mkdir(parents=True) # a directory, not a file + monkeypatch.setattr(mod, "_git_hooks_dir", lambda: str(hooks)) + + assert mod.main() == 0 + assert WARNING_TEXT in capsys.readouterr().err