From e3863b44a5bd9a4406c604a2d300c8b51bfc7aa7 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Fri, 11 Sep 2026 20:09:16 +0300 Subject: [PATCH 01/16] feat: Add update notification for outdated hook pins Hooks silently keep running old releases for months since nothing flags a newer pre-commit-terraform tag exists. Adds a rate-limited (weekly), non-blocking check: hooks self-introspect their own pinned git rev and compare against the latest upstream tag via one git ls-remote call - no config-file parsing needed, works identically under prek. Skippable via CI or PCT_SKIP_UPDATE_CHECK=true. Never fails the hook on network error. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- .flake8 | 13 + README.md | 19 + hooks/_common.sh | 104 +++ tests/pytest/tool_version_test.py | 7 + tests/pytest/update_notification_test.py | 826 +++++++++++++++++++++++ 5 files changed, 969 insertions(+) create mode 100644 tests/pytest/update_notification_test.py diff --git a/.flake8 b/.flake8 index 27d94a76d..6c638d076 100644 --- a/.flake8 +++ b/.flake8 @@ -85,6 +85,19 @@ per-file-ignores = WPS202, # WPS204: "Found overused expression" -- every test wires the same hermetic env/PATH sandbox and re-asserts "no download happened"; deduplicating that into helpers would hide what each test actually guarantees WPS204, + tests/pytest/update_notification_test.py: + # WPS226: "Forbid the overuse of string literals" -- same legitimate test-fixture rationale as tool_version_test.py above + WPS226, + # WPS202: "Found too many module members" -- same one-scenario-per-test rationale as tool_version_test.py above + WPS202, + # WPS204: "Found overused expression" -- same hermetic env/PATH sandbox wiring rationale as tool_version_test.py above + WPS204, + # WPS210: "Found too many local variables" -- black-box hook subprocess tests inherently wire many pieces (dispatcher, sandbox PATH, cache dir, cache file, hook_run/combined); splitting further would hide the full env each assertion depends on + WPS210, + # WPS218: "Found too many assert statements" -- each assertion pins a distinct, independently-meaningful guarantee (message content, cache-file state, exit code); merging them would make failures less pinpointed + WPS218, + # WPS402: "Found noqa comments overuse" -- same subprocess.run(# noqa: S603) pattern as tool_version_test.py's tmp_repo fixture, just with a couple more scenario-specific git calls (rev-parse/tag) on top + WPS402, # We will not spend time on fixing complexity in deprecated hook src/pre_commit_terraform/terraform_docs_replace.py: WPS232 diff --git a/README.md b/README.md index 5a71950ae..fafaa2b09 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,7 @@ If you want to support the development of `pre-commit-terraform` and [many other * [All hooks: Set env vars inside hook at runtime](#all-hooks-set-env-vars-inside-hook-at-runtime) * [All hooks: Disable color output](#all-hooks-disable-color-output) * [All hooks: Log levels](#all-hooks-log-levels) + * [All hooks: Check for a newer pre-commit-terraform release](#all-hooks-check-for-a-newer-pre-commit-terraform-release) * [Most hooks: Pin a specific tool version](#most-hooks-pin-a-specific-tool-version) * [Keeping pinned versions up-to-date using Renovate](#keeping-pinned-versions-up-to-date-using-renovate) * [Many hooks: Parallelism](#many-hooks-parallelism) @@ -437,6 +438,24 @@ PCT_LOG=trace pre-commit run -a Less verbose log levels will be implemented in [#562](https://github.com/antonbabenko/pre-commit-terraform/issues/562). +### All hooks: Check for a newer pre-commit-terraform release + +> All, except deprecated hooks: `checkov`, `terraform_docs_replace` + +1. Once a week hooks checks whether the `rev` pinned in your `.pre-commit-config.yaml`/`prek.toml` is behind the latest `pre-commit-terraform` release tag. +2. The check runs once per hook invocation (not once per changed directory), and is entirely local except for one, read-only `git ls-remote` call against this repo - no data about your code or repository is sent anywhere. +2. If you're behind, you'll see a one-line notice suggesting update. If you're already on the latest tag, nothing is printed. +3. Skip the check, in this order of precedence: + 1. Set `CI=true` (most CI systems already export this automatically). + 2. Set `PCT_SKIP_UPDATE_CHECK=true` to disable it everywhere, including locally. +4. The check never fails or meaningfully slows down your commit: the remote query is capped at 3 seconds, and if it can't reach GitHub (offline, firewalled CI runner, etc.) it prints a short notice and moves on - the hook's own exit code is unaffected either way. +5. The last-checked timestamp is cached at `.last_update_check` under the same cache root used for [pinned tool versions](#most-hooks-pin-a-specific-tool-version) (`PCT_TOOL_CACHE_DIR`, or `$XDG_CACHE_HOME`/`$HOME/.cache` + `pre-commit-terraform`) - see [Mount tools cache directory](#mount-tools-cache-directory) if you also want this to persist across Docker runs. + +```bash +# Skip the check for this run (or export it in CI) +PCT_SKIP_UPDATE_CHECK=true pre-commit run -a +``` + ### Most hooks: Pin a specific tool version > All hooks, which wrap a tool distributed as a downloadable release asset. Not supported for `checkov`/`terraform_checkov` (distributed via PyPi) and for deprecated `terraform_docs_replace` hook. diff --git a/hooks/_common.sh b/hooks/_common.sh index d7209731c..695c9f4bd 100644 --- a/hooks/_common.sh +++ b/hooks/_common.sh @@ -979,3 +979,107 @@ function common::terragrunt_version_ge_0.78 { return 1 fi } + +####################################################################### +# Check for newer pre-commit-terraform release and notify if outdated. +# Rate-limited to once per 7 days, skippable via env vars. +# Globals: +# CI (string) if set, skip entirely +# PCT_SKIP_UPDATE_CHECK (string) if set, skip entirely +# PCT_TOOL_CACHE_DIR (string) cache root location +# XDG_CACHE_HOME (string) fallback cache location +# HOME (string) fallback cache location +# Arguments: +# None +# Outputs: +# Prints a yellow notice if pinned revision is behind latest upstream tag, +# or if HEAD is untagged while a newer release exists. Prints nothing +# when already up-to-date or when check was skipped. +####################################################################### +function common::maybe_notify_new_version { + # Guard chain: skip entirely if CI is set + if [[ -n ${CI:-} ]]; then + return + fi + + # Guard chain: skip entirely if PCT_SKIP_UPDATE_CHECK is set + if [[ -n ${PCT_SKIP_UPDATE_CHECK:-} ]]; then + return + fi + + # Determine cache root (mirrors common::resolve_tool_path) + local -r cache_root="${PCT_TOOL_CACHE_DIR:-${XDG_CACHE_HOME:-$HOME/.cache}/pre-commit-terraform}" + local -r cache_file="$cache_root/.last_update_check" + + # Guard chain: skip if cache file exists and is younger than 7 days + if [[ -f $cache_file ]]; then + local -r now=$(date +%s) + local -r cached_time=$(< "$cache_file") + local -r age_seconds=$((now - cached_time)) + # 7 days = 604800 seconds + if [[ $age_seconds -lt 604800 ]]; then + return + fi + fi + + # Attempt the remote query with 3-second timeout + local remote_output + if remote_output=$(timeout 3 git ls-remote --tags --refs --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform 2>&1); then + # Parse latest tag (last line of sorted output) + local latest_tag + latest_tag=$(echo "$remote_output" | tail -n1 | awk '{print $2}' | sed 's|^refs/tags/||') + + # Get current HEAD sha + local current_sha + current_sha=$(git rev-parse HEAD) + + # Find which tag, if any, matches current HEAD + local current_tag="" + while IFS=$'\t' read -r sha tag; do + if [[ $sha == "$current_sha" ]]; then + current_tag=${tag#refs/tags/} + break + fi + done <<< "$remote_output" + + # Determine nag message + if [[ $latest_tag == "$current_tag" ]]; then + # Already up-to-date, silent + : + else + # Either outdated or untagged + if [[ -n $current_tag ]]; then + common::colorify "yellow" \ + "pre-commit-terraform ${current_tag} is outdated; latest is ${latest_tag}." \ + 'Run "pre-commit autoupdate --freeze" (or "prek update --freeze") to upgrade.' + else + common::colorify "yellow" \ + "pre-commit-terraform pinned to a non-release commit; latest release is ${latest_tag}." \ + 'Run "pre-commit autoupdate --freeze" (or "prek update --freeze") to upgrade.' + fi + fi + else + # Network failure + local exit_code=$? + if [[ $exit_code -eq 124 ]]; then + common::colorify "yellow" \ + "Update check timed out. Set CI=true or PCT_SKIP_UPDATE_CHECK=true to skip." + else + common::colorify "yellow" \ + "Update check failed (exit ${exit_code}). Set CI=true or PCT_SKIP_UPDATE_CHECK=true to skip." + fi + fi + + # Stamp cache file after every real attempt (success or failure) + mkdir -p "$cache_root" + date +%s > "$cache_file" +} + +# Run once per hook invocation, as early as possible: this top-level +# statement executes the moment ANY hook sources this file - before +# `main`, before `common::per_dir_hook`, before `run_hook_on_whole_repo`, +# before any hook-specific logic. Placed here (not inside +# `common::per_dir_hook`) so coverage is uniform across every bash-based +# hook, including ones that never call `common::per_dir_hook` at all +# (e.g. infracost_breakdown.sh, terraform_wrapper_module_for_each.sh). +common::maybe_notify_new_version diff --git a/tests/pytest/tool_version_test.py b/tests/pytest/tool_version_test.py index 6d814d0e5..61df34381 100644 --- a/tests/pytest/tool_version_test.py +++ b/tests/pytest/tool_version_test.py @@ -150,6 +150,7 @@ class _HookWiring(NamedTuple): 'sed', 'sort', 'tail', + 'timeout', 'tr', 'uname', 'wc', @@ -268,6 +269,12 @@ def _hook_env( # pragma: win32 no cover # Read directly by `tools/install/_common.sh`. Forwarded as an # empty string when absent, which that script treats as unset. 'GITHUB_TOKEN': os.environ.get('GITHUB_TOKEN', ''), + # This suite is about tool-version resolution, not the update + # notification: without this, every hook invocation below would + # also attempt a real `git ls-remote` against GitHub, adding + # network flakiness/latency here and polluting the `cache_dir` + # fixture with an unrelated `.last_update_check` file. + 'PCT_SKIP_UPDATE_CHECK': 'true', **cache_env, } diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py new file mode 100644 index 000000000..eafadfadb --- /dev/null +++ b/tests/pytest/update_notification_test.py @@ -0,0 +1,826 @@ +"""Black-box tests for the update notification feature. + +Tests the `common::maybe_notify_new_version` function added to +`hooks/_common.sh`. Every test invokes a real hook script as a subprocess +and asserts on its +output, exit code, and cache directory state - never on bash-internal +function names. + +NOTE: the module-level `pytestmark` skip leaves every function body below +unexecuted on Windows, and `covdefaults` gates coverage at 100%, so every +module-level `def` in this file needs a `# pragma: win32 no cover`. +""" + +from __future__ import annotations + +import os +import shutil +import stat +import subprocess +import sys +import tempfile +import time +from pathlib import Path + +import pytest + + +REPO_ROOT = Path(__file__).resolve().parents[2] +HOOKS_DIR = REPO_ROOT / 'hooks' + + +# Both are resolved against *this* process' `PATH` by `execvp`, not against +# the `PATH` handed to the subprocess - which is what keeps a sandboxed +# `PATH` (see `_sandbox_path_dir`) from breaking the interpreter itself +# while still hiding wrapped CLI tools from the hook under test. +GIT = shutil.which('git') or 'git' +BASH = shutil.which('bash') or 'bash' + +_SECONDS_PER_HOUR = 3600 +_SECONDS_PER_DAY = 86400 + +# Diagnostic messages emitted by `common::colorify` in `hooks/_common.sh`. +OUTDATED_MSG = 'is outdated; latest is' +UNTAGGED_MSG = 'pinned to a non-release commit; latest release is' +# Quote-character-agnostic on purpose: `common::colorify` messages have +# been observed with both `'single'` and `"double"` quoting around the +# command names depending on how the file was last (re)formatted, so +# these check the command text itself, never the surrounding punctuation. +AUTOUPDATE_MSG = 'pre-commit autoupdate --freeze' +PREK_UPDATE_MSG = 'prek update --freeze' +TIMEOUT_MSG = 'Update check timed out.' +FAILED_MSG = 'Update check failed' +SKIP_SUGGESTION_MSG = 'Set CI=true or PCT_SKIP_UPDATE_CHECK=true to skip.' + +HOOK_TIMEOUT_SECONDS = 30 + +pytestmark = pytest.mark.skipif( + sys.platform == 'win32', + reason=( + 'Hook-subprocess tests are skipped on Windows: this repository ' + 'does not fully support/guarantee Windows hook execution ' + '(see README.md / .github/CONTRIBUTING.md).' + ), +) + + +class _GitDispatcherStub: + """A `git` dispatcher stub that forwards all commands except `ls-remote`. + + When placed ahead of the real `git` on `$PATH`, this stub intercepts + `git ls-remote` calls and returns canned fixture data, while forwarding + all other subcommands (`rev-parse`, `ls-files`, etc.) to the real `git`. + This keeps the hook's genuine git usage real while making the remote + tag query fully deterministic and network-free. + """ + + def __init__(self, tmp_path: Path) -> None: + """Create a dispatcher stub directory. + + Args: + tmp_path: Base temporary directory to create the stub in. + """ + self.stub_dir = tmp_path / 'git-dispatcher' + self.stub_dir.mkdir() + self.stub_path = self.stub_dir / 'git' + + # Write the dispatcher script + self.stub_path.write_text( + '#!/usr/bin/env bash\n' + 'set -eo pipefail\n' + '\n' + 'if [[ "$1" == "ls-remote" ]]; then\n' + ' # Intercept ls-remote calls\n' + ' if [[ -f "${0}.ls-remote-output" ]]; then\n' + ' cat "${0}.ls-remote-output"\n' + ' if [[ -f "${0}.ls-remote-exitcode" ]]; then\n' + ' exit_code=$(cat "${0}.ls-remote-exitcode")\n' + ' exit "$exit_code"\n' + ' else\n' + ' exit 0\n' + ' fi\n' + ' else\n' + ' # Default: empty tag list\n' + ' exit 0\n' + ' fi\n' + 'else\n' + ' # Forward all other commands to the real git\n' + ' exec "$(dirname "$0")/real-git" "$@"\n' + 'fi\n', + encoding='utf-8', + ) + exec_bits = stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH + self.stub_path.chmod(self.stub_path.stat().st_mode | exec_bits) + + # Symlink to real git + real_git_dir = self.stub_dir / 'real-git' + real_git_dir.symlink_to(GIT) + + def set_ls_remote_output(self, output: str, exit_code: int = 0) -> None: + """Configure the canned `ls-remote` output and exit code. + + Args: + output: The exact stdout `git ls-remote` should return. + exit_code: Exit code (0 for success, non-zero for failure). + """ + stub_dir, stub_name = self.stub_path.parent, self.stub_path.name + (stub_dir / f'{stub_name}.ls-remote-output').write_text( + output, + encoding='utf-8', + ) + (stub_dir / f'{stub_name}.ls-remote-exitcode').write_text( + str(exit_code), + encoding='utf-8', + ) + + @property + def path_entry(self) -> str: + """The directory path to prepend to `PATH`.""" + return str(self.stub_dir) + + +def _write_stub(path: Path, marker: str) -> None: # pragma: win32 no cover + """Write a fake, executable binary that prints a marker and exits 0.""" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + f'#!/usr/bin/env bash\necho "{marker}"\nexit 0\n', + encoding='utf-8', + ) + exec_bits = stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH + path.chmod(path.stat().st_mode | exec_bits) + + +def _create_terraform_stub(dispatcher: _GitDispatcherStub) -> None: + """Create a terraform stub in the dispatcher directory. + + The stub will be found before any real terraform in PATH, + allowing hooks that require terraform/tofu to succeed. + """ + terraform_stub = dispatcher.stub_dir / 'terraform' + _write_stub(terraform_stub, 'TERRAFORM_STUB') + + +def _sandbox_path_dir(base: Path) -> Path: # pragma: win32 no cover + """Build a `PATH` dir with coreutils but no wrapped CLI tool. + + Reuses the same list of required/optional tools as `tool_version_test.py`. + + Returns: + Path to the constructed directory, usable as a `PATH` entry. + """ + # Same tool lists as tool_version_test.py + sandbox_required_tools = ( + 'awk', + 'basename', + 'bash', + 'cat', + 'cut', + 'dirname', + 'env', + 'grep', + 'head', + 'mkdir', + 'mktemp', + 'rm', + 'sed', + 'sort', + 'tail', + 'timeout', + 'tr', + 'uname', + 'wc', + 'git', + ) + sandbox_optional_tools = ( + 'chmod', + 'cp', + 'curl', + 'date', + 'find', + 'getopt', + 'id', + 'ln', + 'ls', + 'mv', + 'nproc', + 'printf', + 'readlink', + 'realpath', + 'seq', + 'stat', + 'sysctl', + 'tar', + 'tee', + 'touch', + 'uniq', + 'unzip', + 'xargs', + ) + + path_dir = base / 'sandbox-path' + path_dir.mkdir() + for tool in sandbox_required_tools: + found = shutil.which(tool) + assert found is not None, f'{tool!r} not found on PATH' + (path_dir / tool).symlink_to(found) + for optional_tool in sandbox_optional_tools: + optional_found = shutil.which(optional_tool) + if optional_found is not None: # pragma: no branch + (path_dir / optional_tool).symlink_to(optional_found) + return path_dir + + +def _hook_env( # pragma: win32 no cover + cache_env: dict[str, str], + path: str, + extra_env: dict[str, str] | None = None, +) -> dict[str, str]: + """Build a minimal, hermetic environment for a hook subprocess. + + Inheriting `os.environ` wholesale would let `PCT_TFPATH`, + `TERRAGRUNT_TFPATH`, `PRE_COMMIT_COLOR` or `TF_*` from the developer's + shell change what these tests resolve, so only an explicit allowlist + is forwarded. + + Args: + cache_env: Variables that decide the cache root - + `PCT_TOOL_CACHE_DIR`, or `XDG_CACHE_HOME`/`HOME` when + exercising the fallbacks. Merged last, so it can override + `HOME`. + path: Value for `PATH`. + extra_env: Additional environment variables to include. + + Returns: + The environment mapping to hand to `subprocess.run`. + """ + env = { + 'PATH': path, + 'HOME': os.environ.get('HOME', ''), + 'TMPDIR': os.environ.get('TMPDIR', tempfile.gettempdir()), + 'LC_ALL': 'C', + # `common::colorify` wraps every message in ANSI escapes unless + # this is set; plain text keeps substring assertions honest. + 'PRE_COMMIT_COLOR': 'never', + # Read directly by `tools/install/_common.sh`. Forwarded as an + # empty string when absent, which that script treats as unset. + 'GITHUB_TOKEN': os.environ.get('GITHUB_TOKEN', ''), + **(extra_env or {}), + **cache_env, + } + return env # noqa: RET504 - keep the built value visible for review + + +def _pct_cache_env( # pragma: win32 no cover + cache_dir: Path, +) -> dict[str, str]: + """Point the cache root straight at `cache_dir`. + + Returns: + A `PCT_TOOL_CACHE_DIR` mapping for `_hook_env`. + """ + return {'PCT_TOOL_CACHE_DIR': str(cache_dir)} + + +@pytest.fixture +def tmp_repo(tmp_path: Path) -> Path: # pragma: win32 no cover + """Create a minimal git repo with one tracked, provider-free `.tf` file. + + Returns: + Path to the created repo directory. + """ + repo = tmp_path / 'repo' + repo.mkdir() + # `--template=` disables Git's init templates: this project's own + # README tells users to set `init.templateDir` to a directory with + # pre-commit installed, which would otherwise install a real + # pre-commit hook into this throwaway repo and run it on commit. + subprocess.run( # noqa: S603 + (GIT, 'init', '--quiet', '--template=', '--initial-branch=main'), + cwd=repo, + check=True, + ) + subprocess.run( # noqa: S603 + (GIT, 'config', 'user.email', 't@t.com'), + cwd=repo, + check=True, + ) + subprocess.run( # noqa: S603 + (GIT, 'config', 'user.name', 't'), + cwd=repo, + check=True, + ) + (repo / 'a.tf').write_text( + 'variable "x" { default = 1 }\n', + encoding='utf-8', + ) + subprocess.run((GIT, 'add', 'a.tf'), cwd=repo, check=True) # noqa: S603 + subprocess.run( # noqa: S603 + (GIT, 'commit', '--quiet', '--no-verify', '-m', 'init'), + cwd=repo, + check=True, + ) + return repo + + +@pytest.fixture +def cache_dir(tmp_path: Path) -> Path: # pragma: win32 no cover + """Create a dedicated, empty cache root for one test. + + Returns: + Path to the created, empty cache root directory. + """ + cache = tmp_path / 'cache' + cache.mkdir() + return cache + + +def _run_hook( # pragma: win32 no cover + hook_name: str, + args: list[str], + *, + cwd: Path, + env: dict[str, str], +) -> subprocess.CompletedProcess[str]: + """Invoke a hook script on the single `a.tf` file of a temp repo. + + Returns: + The completed process, with stderr folded into `.stdout`. + """ + hook_path = HOOKS_DIR / hook_name + if not hook_path.is_file(): # pragma: no cover + pytest.fail(f'Hook script not found: {hook_path}') + # `common::colorify` writes every diagnostic to stderr while a wrapped + # tool's own output goes to stdout, so the two are merged at the OS + # level to give each caller one ready-to-grep string. + return subprocess.run( # noqa: S603 + (BASH, str(hook_path), *args, '--', 'a.tf'), + cwd=cwd, + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + check=False, + timeout=HOOK_TIMEOUT_SECONDS, + ) + + +# Sample git ls-remote output for testing +SAMPLE_LS_REMOTE_OUTPUT = """\ +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/tags/v1.0.0 +bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb refs/tags/v1.1.0 +cccccccccccccccccccccccccccccccccccccccc refs/tags/v1.2.0 +dddddddddddddddddddddddddddddddddddddddd refs/tags/v1.3.0 +eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee refs/tags/v1.4.0 +""" + + +def test_ci_set_skips_check( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check `$CI` set → no network attempt, cache file untouched.""" + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + _create_terraform_stub(dispatcher) + + # Create a sandboxed PATH with the dispatcher first + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env( + _pct_cache_env(cache_dir), + path_with_dispatcher, + {'CI': 'true'}, + ), + ) + + combined = hook_run.stdout + assert OUTDATED_MSG not in combined, combined + assert UNTAGGED_MSG not in combined, combined + assert AUTOUPDATE_MSG not in combined, combined + assert TIMEOUT_MSG not in combined, combined + assert FAILED_MSG not in combined, combined + + # Cache file should not exist (check was skipped without attempt) + cache_file = cache_dir / '.last_update_check' + assert not cache_file.exists(), f'Cache file missing check: {cache_file}' + + # Hook should still run its real work + assert hook_run.returncode == 0, combined + + +def test_pct_skip_update_check_set_skips_check( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check `$PCT_SKIP_UPDATE_CHECK` set → no attempt, cache untouched.""" + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + _create_terraform_stub(dispatcher) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env( + _pct_cache_env(cache_dir), + path_with_dispatcher, + {'PCT_SKIP_UPDATE_CHECK': 'true'}, + ), + ) + + combined = hook_run.stdout + assert OUTDATED_MSG not in combined, combined + assert UNTAGGED_MSG not in combined, combined + assert AUTOUPDATE_MSG not in combined, combined + assert TIMEOUT_MSG not in combined, combined + assert FAILED_MSG not in combined, combined + + cache_file = cache_dir / '.last_update_check' + assert not cache_file.exists(), f'Cache file missing check: {cache_file}' + assert hook_run.returncode == 0, combined + + +def test_fresh_cache_skips_check( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check fresh cache file (< 7 days old) → no network attempt.""" + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + _create_terraform_stub(dispatcher) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + # Create a fresh cache file (1 hour old) + cache_file = cache_dir / '.last_update_check' + cache_file.parent.mkdir(parents=True, exist_ok=True) + one_hour_ago = int(time.time()) - _SECONDS_PER_HOUR + cache_file.write_text(str(one_hour_ago), encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert OUTDATED_MSG not in combined, combined + assert UNTAGGED_MSG not in combined, combined + assert AUTOUPDATE_MSG not in combined, combined + assert TIMEOUT_MSG not in combined, combined + assert FAILED_MSG not in combined, combined + + # Cache file should still contain the original timestamp (not updated) + assert cache_file.read_text(encoding='utf-8') == str(one_hour_ago) + assert hook_run.returncode == 0, combined + + +def test_stale_cache_outdated_tag_nag( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check stale cache + pinned tag older than latest → nag printed.""" + dispatcher = _GitDispatcherStub(tmp_path) + _create_terraform_stub(dispatcher) + + # Create empty commit and get its SHA + subprocess.run( # noqa: S603 + (GIT, 'commit', '--allow-empty', '-m', 'bump', '--date=2000-01-01'), + cwd=tmp_repo, + check=True, + ) + head_rev_parse = subprocess.run( # noqa: S603 + (GIT, 'rev-parse', 'HEAD'), + cwd=tmp_repo, + capture_output=True, + text=True, + check=True, + ) + current_sha = head_rev_parse.stdout.strip() + + # Tag current HEAD as v1.3.0 + subprocess.run( # noqa: S603 + (GIT, 'tag', '-f', 'v1.3.0'), + cwd=tmp_repo, + check=True, + ) + + # Update ls-remote output to include current SHA as v1.3.0 + ls_remote_output = SAMPLE_LS_REMOTE_OUTPUT.replace( + 'dddddddddddddddddddddddddddddddddddddddd', + current_sha, + ) + dispatcher.set_ls_remote_output(ls_remote_output) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + # Create a stale cache file (8 days old) + cache_file = cache_dir / '.last_update_check' + cache_file.parent.mkdir(parents=True, exist_ok=True) + eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) + cache_file.write_text(str(eight_days_ago), encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert OUTDATED_MSG in combined, combined + assert AUTOUPDATE_MSG in combined, combined + assert PREK_UPDATE_MSG in combined, combined + assert 'v1.3.0' in combined, combined + assert 'v1.4.0' in combined, combined + + # Cache file should be updated to now (or very recent) + new_timestamp = int(cache_file.read_text(encoding='utf-8')) + assert new_timestamp > eight_days_ago + assert hook_run.returncode == 0, combined + + +def test_stale_cache_untagged_nag( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check stale/absent cache + HEAD matches no tag → nag printed.""" + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + _create_terraform_stub(dispatcher) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + # Create a stale cache file (8 days old) + cache_file = cache_dir / '.last_update_check' + cache_file.parent.mkdir(parents=True, exist_ok=True) + eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) + cache_file.write_text(str(eight_days_ago), encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert UNTAGGED_MSG in combined, combined + assert AUTOUPDATE_MSG in combined, combined + assert PREK_UPDATE_MSG in combined, combined + assert 'v1.4.0' in combined, combined # Latest tag should be mentioned + + # Cache file should be updated + new_timestamp = int(cache_file.read_text(encoding='utf-8')) + assert new_timestamp > eight_days_ago + assert hook_run.returncode == 0, combined + + +def test_stale_cache_up_to_date_silent( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check stale/absent cache + pinned tag equals latest → no output.""" + dispatcher = _GitDispatcherStub(tmp_path) + _create_terraform_stub(dispatcher) + + # Get current HEAD sha and tag it as v1.4.0 (latest) + current_sha = subprocess.run( # noqa: S603 + (GIT, 'rev-parse', 'HEAD'), + cwd=tmp_repo, + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + # Create ls-remote output with current HEAD as v1.4.0 + ls_remote_output = SAMPLE_LS_REMOTE_OUTPUT.replace( + 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', + current_sha, + ) + dispatcher.set_ls_remote_output(ls_remote_output) + + # Tag current HEAD as v1.4.0 + subprocess.run( # noqa: S603 + (GIT, 'tag', '-f', 'v1.4.0'), + cwd=tmp_repo, + check=True, + ) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + # Create a stale cache file (8 days old) + cache_file = cache_dir / '.last_update_check' + cache_file.parent.mkdir(parents=True, exist_ok=True) + eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) + cache_file.write_text(str(eight_days_ago), encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert OUTDATED_MSG not in combined, combined + assert UNTAGGED_MSG not in combined, combined + assert AUTOUPDATE_MSG not in combined, combined + assert TIMEOUT_MSG not in combined, combined + assert FAILED_MSG not in combined, combined + + # Cache file should still be updated (attempt was made, just silent) + new_timestamp = int(cache_file.read_text(encoding='utf-8')) + assert new_timestamp > eight_days_ago + assert hook_run.returncode == 0, combined + + +def test_network_failure_warning( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check dispatcher stub makes `ls-remote` fail → warn notice printed.""" + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output('', exit_code=1) # Non-zero exit + _create_terraform_stub(dispatcher) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + # Create a stale cache file + cache_file = cache_dir / '.last_update_check' + cache_file.parent.mkdir(parents=True, exist_ok=True) + eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) + cache_file.write_text(str(eight_days_ago), encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert FAILED_MSG in combined, combined + assert SKIP_SUGGESTION_MSG in combined, combined + assert 'exit 1' in combined, combined + + # Cache file should be updated (attempt was made, even though it failed) + new_timestamp = int(cache_file.read_text(encoding='utf-8')) + assert new_timestamp > eight_days_ago + assert hook_run.returncode == 0, combined # Hook's own work unaffected + + +def test_check_at_most_once_per_invocation( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check hook invoked across multiple dirs attempts check at most once.""" + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + _create_terraform_stub(dispatcher) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + # Create multiple .tf files in different directories + (tmp_repo / 'dir1').mkdir() + (tmp_repo / 'dir2').mkdir() + (tmp_repo / 'dir1' / 'a.tf').write_text('variable "x" { default = 1 }\n') + (tmp_repo / 'dir2' / 'b.tf').write_text('variable "y" { default = 2 }\n') + + subprocess.run( # noqa: S603 + (GIT, 'add', 'dir1/a.tf', 'dir2/b.tf'), + cwd=tmp_repo, + check=True, + ) + subprocess.run( # noqa: S603 + (GIT, 'commit', '--quiet', '--no-verify', '-m', 'add dirs'), + cwd=tmp_repo, + check=True, + ) + + # Run hook with both files (should trigger check once) + hook_path = HOOKS_DIR / 'terraform_fmt.sh' + multi_dir_run = subprocess.run( # noqa: S603 + (BASH, str(hook_path), '--', 'dir1/a.tf', 'dir2/b.tf'), + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + check=False, + timeout=HOOK_TIMEOUT_SECONDS, + ) + + combined = multi_dir_run.stdout + # Should see the nag (untagged since HEAD matches no tag in the fixture) + assert UNTAGGED_MSG in combined, combined + assert AUTOUPDATE_MSG in combined, combined + assert PREK_UPDATE_MSG in combined, combined + + # Count occurrences - should appear only once + nag_count = combined.count(UNTAGGED_MSG) + assert nag_count == 1, f'Nag appeared {nag_count} times, expected 1' + + # Cache file should exist (check was attempted) + cache_file = cache_dir / '.last_update_check' + assert cache_file.exists() + assert multi_dir_run.returncode == 0, combined + + +def test_check_fires_without_per_dir_hook( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check the notice fires for a hook that never calls `per_dir_hook`. + + `terraform_wrapper_module_for_each.sh` has its own whole-repo flow and + never calls `common::per_dir_hook` (see `hooks/_common.sh` - + `common::maybe_notify_new_version` must run as a top-level statement + in `_common.sh` itself, not from inside `common::per_dir_hook`, or + this exact hook silently loses coverage). The hook's own tool + (`hcledit`) is deliberately left unstubbed and its exit code is + deliberately not asserted: only the notice's presence, printed + before the hook ever reaches its own tool resolution, is under test. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + hook_run = _run_hook( + 'terraform_wrapper_module_for_each.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert UNTAGGED_MSG in combined, combined + assert AUTOUPDATE_MSG in combined, combined + + cache_file = cache_dir / '.last_update_check' + assert cache_file.exists() + + +@pytest.mark.network +def test_real_network_sanity_check( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Sanity test hitting the real hardcoded URL. + + Asserts only that the call succeeds and returns parseable + `sharefs/tags/...` lines - no assertion on a specific version number. + """ + # Create terraform stub for this test + # Need dispatcher for stub creation even though not used for network + dispatcher = _GitDispatcherStub(tmp_path) + _create_terraform_stub(dispatcher) + + # Use real PATH (no dispatcher) to hit real network, but include stub dir + path_with_stub = f'{dispatcher.path_entry}:{os.environ["PATH"]}' + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_stub), + ) + + combined = hook_run.stdout + + # Either the check succeeds (and we might see a nag if outdated/untagged) + # or it fails with timeout/network error (and we see failure message) + # or it's skipped due to fresh cache (first run creates cache) + + # Cache file should exist (attempt was made) + cache_file = cache_dir / '.last_update_check' + assert cache_file.exists() + + # Hook should complete successfully regardless + assert hook_run.returncode == 0, combined From 89d5bed23d0c34786d3e288e6958ab3a7e9ab3d5 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Fri, 11 Sep 2026 23:53:48 +0300 Subject: [PATCH 02/16] refactor: Split cache files, extract update-check to own file - Split `.last_update_check` into `_time`/`_tags` cache files so a failed network attempt can leave cached tags untouched without read-modify-write. - Extracted `_check_new_version_on_failure` out of `_common.sh` into its own file (dropping the `common::` prefix), sourced from `common::initialize`. - Renamed `$remote_output` to `$known_tags`. - Allow long test names via `.flake8` (WPS118) instead of renaming. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- .flake8 | 2 + README.md | 21 +- hooks/_check_new_version_on_failure.sh | 99 ++++++ hooks/_common.sh | 110 +----- tests/pytest/tool_version_test.py | 2 +- tests/pytest/update_notification_test.py | 435 +++++++++++++++++------ 6 files changed, 443 insertions(+), 226 deletions(-) create mode 100755 hooks/_check_new_version_on_failure.sh diff --git a/.flake8 b/.flake8 index 6c638d076..d358668f7 100644 --- a/.flake8 +++ b/.flake8 @@ -98,6 +98,8 @@ per-file-ignores = WPS218, # WPS402: "Found noqa comments overuse" -- same subprocess.run(# noqa: S603) pattern as tool_version_test.py's tmp_repo fixture, just with a couple more scenario-specific git calls (rev-parse/tag) on top WPS402, + # WPS118: "Found too long name" -- allow long test function names + WPS118, # We will not spend time on fixing complexity in deprecated hook src/pre_commit_terraform/terraform_docs_replace.py: WPS232 diff --git a/README.md b/README.md index fafaa2b09..98cdddd6f 100644 --- a/README.md +++ b/README.md @@ -442,14 +442,19 @@ Less verbose log levels will be implemented in [#562](https://github.com/antonba > All, except deprecated hooks: `checkov`, `terraform_docs_replace` -1. Once a week hooks checks whether the `rev` pinned in your `.pre-commit-config.yaml`/`prek.toml` is behind the latest `pre-commit-terraform` release tag. -2. The check runs once per hook invocation (not once per changed directory), and is entirely local except for one, read-only `git ls-remote` call against this repo - no data about your code or repository is sent anywhere. -2. If you're behind, you'll see a one-line notice suggesting update. If you're already on the latest tag, nothing is printed. -3. Skip the check, in this order of precedence: - 1. Set `CI=true` (most CI systems already export this automatically). - 2. Set `PCT_SKIP_UPDATE_CHECK=true` to disable it everywhere, including locally. -4. The check never fails or meaningfully slows down your commit: the remote query is capped at 3 seconds, and if it can't reach GitHub (offline, firewalled CI runner, etc.) it prints a short notice and moves on - the hook's own exit code is unaffected either way. -5. The last-checked timestamp is cached at `.last_update_check` under the same cache root used for [pinned tool versions](#most-hooks-pin-a-specific-tool-version) (`PCT_TOOL_CACHE_DIR`, or `$XDG_CACHE_HOME`/`$HOME/.cache` + `pre-commit-terraform`) - see [Mount tools cache directory](#mount-tools-cache-directory) if you also want this to persist across Docker runs. +To skip the check set one of: + +* `CI=true` (most CI systems already export this automatically). +* `PCT_SKIP_UPDATE_CHECK=true` to disable it everywhere, including locally. + +How it works: + +1. The check only runs when a hook is about to fail for its own reasons - a clean run stays completely silent, no matter how outdated your pin is. +2. On a failing run, it checks whether the `rev` pinned in your `.pre-commit-config.yaml`/`prek.toml` is behind the latest `pre-commit-terraform` release tag, at most once per invocation. +3. If you're behind, you'll see a one-line notice suggesting `pre-commit autoupdate --freeze` (or `prek update --freeze` if you use [prek](https://github.com/j178/prek)) +4. The remote query itself - one read-only `git ls-remote` against this repo, no data about your code or repository sent anywhere - is rate-limited to once per 7 days. Within that window, a still-outdated pin keeps nagging on every failing run from the cached result, at no extra network cost. +5. The check never fails or meaningfully slows down your commit: the remote query is capped at 3 seconds, and if it can't reach GitHub (offline, firewalled CI runner, etc.) it prints a short notice and moves on - the hook's own exit code is unaffected either way. +6. The last-checked timestamp and the upstream tag list from that check are cached as two files, `.last_update_check_time` and `.last_update_check_tags`, under the same cache root used for [pinned tool versions](#most-hooks-pin-a-specific-tool-version) (`PCT_TOOL_CACHE_DIR`, or `$XDG_CACHE_HOME`/`$HOME/.cache` + `pre-commit-terraform`) - see [Mount tools cache directory](#mount-tools-cache-directory) if you also want this to persist across Docker runs. ```bash # Skip the check for this run (or export it in CI) diff --git a/hooks/_check_new_version_on_failure.sh b/hooks/_check_new_version_on_failure.sh new file mode 100755 index 000000000..3a744e996 --- /dev/null +++ b/hooks/_check_new_version_on_failure.sh @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +set -eo pipefail + +####################################################################### +# Check for newer pre-commit-terraform release and notify if outdated. +# The remote query is rate-limited to once per 7 days; within that +# window, an already-known-outdated pin still gets renagged every +# failing run, using cached tag data instead of a fresh query. +# Globals: +# CI (string) if set, skip entirely +# PCT_SKIP_UPDATE_CHECK (string) if set, skip entirely +# PCT_TOOL_CACHE_DIR (string) cache root location +# XDG_CACHE_HOME (string) fallback cache location +# HOME (string) fallback cache location +# Arguments: +# None +# Outputs: +# Prints a yellow notice if pinned revision is behind latest upstream tag, +# or if HEAD is untagged while a newer release exists. Prints nothing +# when already up-to-date or when check was skipped. +####################################################################### +function _check_new_version_on_failure { + if [[ -n ${CI:-} ]] || [[ -n ${PCT_SKIP_UPDATE_CHECK:-} ]]; then + return + fi + + local -r cache_root="${PCT_TOOL_CACHE_DIR:-${XDG_CACHE_HOME:-$HOME/.cache}/pre-commit-terraform}" + # Holds only the last-checked timestamp + local -r time_cache_file="$cache_root/.last_update_check_time" + # Hold the raw `git ls-remote --tags` output, one "refs/tags/" line per tag + local -r tags_cache_file="$cache_root/.last_update_check_tags" + local -r current_sha=$(git rev-parse HEAD) + # + # Try to get tags from valid cache when possible + # + local known_tags="" + if [[ -f $time_cache_file ]]; then + local cached_time + cached_time=$(< "$time_cache_file") + local -r age_seconds=$(($(date +%s) - cached_time)) + if [[ $age_seconds -lt 604800 ]] && [[ -f $tags_cache_file ]]; then + known_tags=$(< "$tags_cache_file") + fi + fi + # + # No/stale cache, need to go to network + # + if [[ -z $known_tags ]]; then + local fresh_output + + if fresh_output=$(timeout 3 git ls-remote --tags --refs --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform 2>&1); then + known_tags=$fresh_output + mkdir -p "$cache_root" + date +%s > "$time_cache_file" + echo "$known_tags" > "$tags_cache_file" + else + local exit_code=$? + if [[ $exit_code -eq 124 ]]; then + common::colorify "yellow" "Update check timed out." + else + common::colorify "yellow" "Update check failed (exit ${exit_code})." + fi + common::colorify "yellow" "Will try again in a week." \ + "Set CI=true or PCT_SKIP_UPDATE_CHECK=true to never check for updates." + + # Skip update check for a week + mkdir -p "$cache_root" + date +%s > "$time_cache_file" + return + fi + fi + # + # Valid cache + # + local latest_tag_name + latest_tag_name=$(echo "$known_tags" | tail -n1 | awk '{print $2}' | sed 's|^refs/tags/||') + + # Find which tag, if any, matches current HEAD + local current_tag="" + while IFS=$'\t' read -r sha tag; do + if [[ $sha == "$current_sha" ]]; then + current_tag=${tag#refs/tags/} + break + fi + done <<< "$known_tags" + + if [[ $latest_tag_name == "$current_tag" ]]; then + # Already up-to-date, silent + return + elif [[ -n $current_tag ]]; then + common::colorify "yellow" "pre-commit-terraform ${current_tag} is outdated; latest is ${latest_tag_name}." + else + common::colorify "yellow" "pre-commit-terraform pinned to a non-release commit; latest release is ${latest_tag_name}." + fi + common::colorify "yellow" 'Run "pre-commit autoupdate --freeze" (or "prek update --freeze") to upgrade.' +} + +# Check for update only on hooks failure +trap '[[ $? -ne 0 ]] && _check_new_version_on_failure' EXIT diff --git a/hooks/_common.sh b/hooks/_common.sh index 695c9f4bd..8bd311c98 100644 --- a/hooks/_common.sh +++ b/hooks/_common.sh @@ -24,15 +24,17 @@ HOOK_ID=${0##*/} readonly HOOK_ID=${HOOK_ID%%.*} ####################################################################### -# Init arguments parser +# Initialize common functions and environment for hooks # Arguments: # script_dir - absolute path to hook dir location ####################################################################### function common::initialize { local -r script_dir=$1 - # source getopt function + # Init arguments parser (getopt function) # shellcheck source=../lib_getopt . "$script_dir/../lib_getopt" + # Initialize update check on failure + . "$script_dir/_check_new_version_on_failure.sh" } ####################################################################### @@ -979,107 +981,3 @@ function common::terragrunt_version_ge_0.78 { return 1 fi } - -####################################################################### -# Check for newer pre-commit-terraform release and notify if outdated. -# Rate-limited to once per 7 days, skippable via env vars. -# Globals: -# CI (string) if set, skip entirely -# PCT_SKIP_UPDATE_CHECK (string) if set, skip entirely -# PCT_TOOL_CACHE_DIR (string) cache root location -# XDG_CACHE_HOME (string) fallback cache location -# HOME (string) fallback cache location -# Arguments: -# None -# Outputs: -# Prints a yellow notice if pinned revision is behind latest upstream tag, -# or if HEAD is untagged while a newer release exists. Prints nothing -# when already up-to-date or when check was skipped. -####################################################################### -function common::maybe_notify_new_version { - # Guard chain: skip entirely if CI is set - if [[ -n ${CI:-} ]]; then - return - fi - - # Guard chain: skip entirely if PCT_SKIP_UPDATE_CHECK is set - if [[ -n ${PCT_SKIP_UPDATE_CHECK:-} ]]; then - return - fi - - # Determine cache root (mirrors common::resolve_tool_path) - local -r cache_root="${PCT_TOOL_CACHE_DIR:-${XDG_CACHE_HOME:-$HOME/.cache}/pre-commit-terraform}" - local -r cache_file="$cache_root/.last_update_check" - - # Guard chain: skip if cache file exists and is younger than 7 days - if [[ -f $cache_file ]]; then - local -r now=$(date +%s) - local -r cached_time=$(< "$cache_file") - local -r age_seconds=$((now - cached_time)) - # 7 days = 604800 seconds - if [[ $age_seconds -lt 604800 ]]; then - return - fi - fi - - # Attempt the remote query with 3-second timeout - local remote_output - if remote_output=$(timeout 3 git ls-remote --tags --refs --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform 2>&1); then - # Parse latest tag (last line of sorted output) - local latest_tag - latest_tag=$(echo "$remote_output" | tail -n1 | awk '{print $2}' | sed 's|^refs/tags/||') - - # Get current HEAD sha - local current_sha - current_sha=$(git rev-parse HEAD) - - # Find which tag, if any, matches current HEAD - local current_tag="" - while IFS=$'\t' read -r sha tag; do - if [[ $sha == "$current_sha" ]]; then - current_tag=${tag#refs/tags/} - break - fi - done <<< "$remote_output" - - # Determine nag message - if [[ $latest_tag == "$current_tag" ]]; then - # Already up-to-date, silent - : - else - # Either outdated or untagged - if [[ -n $current_tag ]]; then - common::colorify "yellow" \ - "pre-commit-terraform ${current_tag} is outdated; latest is ${latest_tag}." \ - 'Run "pre-commit autoupdate --freeze" (or "prek update --freeze") to upgrade.' - else - common::colorify "yellow" \ - "pre-commit-terraform pinned to a non-release commit; latest release is ${latest_tag}." \ - 'Run "pre-commit autoupdate --freeze" (or "prek update --freeze") to upgrade.' - fi - fi - else - # Network failure - local exit_code=$? - if [[ $exit_code -eq 124 ]]; then - common::colorify "yellow" \ - "Update check timed out. Set CI=true or PCT_SKIP_UPDATE_CHECK=true to skip." - else - common::colorify "yellow" \ - "Update check failed (exit ${exit_code}). Set CI=true or PCT_SKIP_UPDATE_CHECK=true to skip." - fi - fi - - # Stamp cache file after every real attempt (success or failure) - mkdir -p "$cache_root" - date +%s > "$cache_file" -} - -# Run once per hook invocation, as early as possible: this top-level -# statement executes the moment ANY hook sources this file - before -# `main`, before `common::per_dir_hook`, before `run_hook_on_whole_repo`, -# before any hook-specific logic. Placed here (not inside -# `common::per_dir_hook`) so coverage is uniform across every bash-based -# hook, including ones that never call `common::per_dir_hook` at all -# (e.g. infracost_breakdown.sh, terraform_wrapper_module_for_each.sh). -common::maybe_notify_new_version diff --git a/tests/pytest/tool_version_test.py b/tests/pytest/tool_version_test.py index 61df34381..4ca8c0a6a 100644 --- a/tests/pytest/tool_version_test.py +++ b/tests/pytest/tool_version_test.py @@ -273,7 +273,7 @@ def _hook_env( # pragma: win32 no cover # notification: without this, every hook invocation below would # also attempt a real `git ls-remote` against GitHub, adding # network flakiness/latency here and polluting the `cache_dir` - # fixture with an unrelated `.last_update_check` file. + # fixture with unrelated `.last_update_check_*` files. 'PCT_SKIP_UPDATE_CHECK': 'true', **cache_env, } diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index eafadfadb..16725e44b 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -1,8 +1,8 @@ """Black-box tests for the update notification feature. -Tests the `common::maybe_notify_new_version` function added to -`hooks/_common.sh`. Every test invokes a real hook script as a subprocess -and asserts on its +Tests the `_check_new_version_on_failure` function in +`hooks/_check_new_version_on_failure.sh`. Every test invokes a real hook +script as a subprocess and asserts on its output, exit code, and cache directory state - never on bash-internal function names. @@ -39,7 +39,8 @@ _SECONDS_PER_HOUR = 3600 _SECONDS_PER_DAY = 86400 -# Diagnostic messages emitted by `common::colorify` in `hooks/_common.sh`. +# Diagnostic messages emitted via `common::colorify` calls in +# `hooks/_check_new_version_on_failure.sh`. OUTDATED_MSG = 'is outdated; latest is' UNTAGGED_MSG = 'pinned to a non-release commit; latest release is' # Quote-character-agnostic on purpose: `common::colorify` messages have @@ -50,7 +51,9 @@ PREK_UPDATE_MSG = 'prek update --freeze' TIMEOUT_MSG = 'Update check timed out.' FAILED_MSG = 'Update check failed' -SKIP_SUGGESTION_MSG = 'Set CI=true or PCT_SKIP_UPDATE_CHECK=true to skip.' +SKIP_SUGGESTION_MSG = ( + 'Set CI=true or PCT_SKIP_UPDATE_CHECK=true to never check for updates.' +) HOOK_TIMEOUT_SECONDS = 30 @@ -74,7 +77,7 @@ class _GitDispatcherStub: tag query fully deterministic and network-free. """ - def __init__(self, tmp_path: Path) -> None: + def __init__(self, tmp_path: Path) -> None: # pragma: win32 no cover """Create a dispatcher stub directory. Args: @@ -116,7 +119,11 @@ def __init__(self, tmp_path: Path) -> None: real_git_dir = self.stub_dir / 'real-git' real_git_dir.symlink_to(GIT) - def set_ls_remote_output(self, output: str, exit_code: int = 0) -> None: + def set_ls_remote_output( # pragma: win32 no cover + self, + output: str, + exit_code: int = 0, + ) -> None: """Configure the canned `ls-remote` output and exit code. Args: @@ -134,7 +141,7 @@ def set_ls_remote_output(self, output: str, exit_code: int = 0) -> None: ) @property - def path_entry(self) -> str: + def path_entry(self) -> str: # pragma: win32 no cover """The directory path to prepend to `PATH`.""" return str(self.stub_dir) @@ -150,7 +157,9 @@ def _write_stub(path: Path, marker: str) -> None: # pragma: win32 no cover path.chmod(path.stat().st_mode | exec_bits) -def _create_terraform_stub(dispatcher: _GitDispatcherStub) -> None: +def _create_terraform_stub( # pragma: win32 no cover + dispatcher: _GitDispatcherStub, +) -> None: """Create a terraform stub in the dispatcher directory. The stub will be found before any real terraform in PATH, @@ -281,6 +290,17 @@ def _pct_cache_env( # pragma: win32 no cover return {'PCT_TOOL_CACHE_DIR': str(cache_dir)} +def _read_cache_timestamp( # pragma: win32 no cover + time_cache_file: Path, +) -> int: + """Read the cached timestamp from `.last_update_check_time`. + + Returns: + The cached timestamp as an int. + """ + return int(time_cache_file.read_text(encoding='utf-8').strip()) + + @pytest.fixture def tmp_repo(tmp_path: Path) -> Path: # pragma: win32 no cover """Create a minimal git repo with one tracked, provider-free `.tf` file. @@ -379,12 +399,17 @@ def test_ci_set_skips_check( # pragma: win32 no cover cache_dir: Path, tmp_path: Path, ) -> None: - """Check `$CI` set → no network attempt, cache file untouched.""" + """Check `$CI` set → no network attempt, cache file untouched. + + The check only runs at all once the hook itself is about to exit + non-zero (`trap ... EXIT` in `hooks/_common.sh`), so no terraform + stub is installed here - the hook fails on its own (missing + terraform/tofu), which is what arms the check in the first place; + `$CI` must then still suppress it from there. + """ dispatcher = _GitDispatcherStub(tmp_path) dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) - _create_terraform_stub(dispatcher) - # Create a sandboxed PATH with the dispatcher first sandbox_path_dir = _sandbox_path_dir(tmp_path) path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' @@ -406,12 +431,19 @@ def test_ci_set_skips_check( # pragma: win32 no cover assert TIMEOUT_MSG not in combined, combined assert FAILED_MSG not in combined, combined - # Cache file should not exist (check was skipped without attempt) - cache_file = cache_dir / '.last_update_check' - assert not cache_file.exists(), f'Cache file missing check: {cache_file}' + # Cache files should not exist (check was skipped without attempt) + time_cache_file = cache_dir / '.last_update_check_time' + tags_cache_file = cache_dir / '.last_update_check_tags' + assert not time_cache_file.exists(), ( + f'Cache file missing check: {time_cache_file}' + ) + assert not tags_cache_file.exists(), ( + f'Cache file missing check: {tags_cache_file}' + ) - # Hook should still run its real work - assert hook_run.returncode == 0, combined + # Hook fails on its own (no terraform/tofu) - that failure is what + # arms the trap; $CI must suppress the check regardless. + assert hook_run.returncode != 0, combined def test_pct_skip_update_check_set_skips_check( # pragma: win32 no cover @@ -419,10 +451,14 @@ def test_pct_skip_update_check_set_skips_check( # pragma: win32 no cover cache_dir: Path, tmp_path: Path, ) -> None: - """Check `$PCT_SKIP_UPDATE_CHECK` set → no attempt, cache untouched.""" + """Check `$PCT_SKIP_UPDATE_CHECK` set → no attempt, cache untouched. + + No terraform stub: the hook must fail on its own to arm the trap in + the first place, and `$PCT_SKIP_UPDATE_CHECK` must then still + suppress the check from there. + """ dispatcher = _GitDispatcherStub(tmp_path) dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) - _create_terraform_stub(dispatcher) sandbox_path_dir = _sandbox_path_dir(tmp_path) path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' @@ -445,47 +481,15 @@ def test_pct_skip_update_check_set_skips_check( # pragma: win32 no cover assert TIMEOUT_MSG not in combined, combined assert FAILED_MSG not in combined, combined - cache_file = cache_dir / '.last_update_check' - assert not cache_file.exists(), f'Cache file missing check: {cache_file}' - assert hook_run.returncode == 0, combined - - -def test_fresh_cache_skips_check( # pragma: win32 no cover - tmp_repo: Path, - cache_dir: Path, - tmp_path: Path, -) -> None: - """Check fresh cache file (< 7 days old) → no network attempt.""" - dispatcher = _GitDispatcherStub(tmp_path) - dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) - _create_terraform_stub(dispatcher) - - sandbox_path_dir = _sandbox_path_dir(tmp_path) - path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' - - # Create a fresh cache file (1 hour old) - cache_file = cache_dir / '.last_update_check' - cache_file.parent.mkdir(parents=True, exist_ok=True) - one_hour_ago = int(time.time()) - _SECONDS_PER_HOUR - cache_file.write_text(str(one_hour_ago), encoding='utf-8') - - hook_run = _run_hook( - 'terraform_fmt.sh', - [], - cwd=tmp_repo, - env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + time_cache_file = cache_dir / '.last_update_check_time' + tags_cache_file = cache_dir / '.last_update_check_tags' + assert not time_cache_file.exists(), ( + f'Cache file missing check: {time_cache_file}' ) - - combined = hook_run.stdout - assert OUTDATED_MSG not in combined, combined - assert UNTAGGED_MSG not in combined, combined - assert AUTOUPDATE_MSG not in combined, combined - assert TIMEOUT_MSG not in combined, combined - assert FAILED_MSG not in combined, combined - - # Cache file should still contain the original timestamp (not updated) - assert cache_file.read_text(encoding='utf-8') == str(one_hour_ago) - assert hook_run.returncode == 0, combined + assert not tags_cache_file.exists(), ( + f'Cache file missing check: {tags_cache_file}' + ) + assert hook_run.returncode != 0, combined def test_stale_cache_outdated_tag_nag( # pragma: win32 no cover @@ -493,9 +497,12 @@ def test_stale_cache_outdated_tag_nag( # pragma: win32 no cover cache_dir: Path, tmp_path: Path, ) -> None: - """Check stale cache + pinned tag older than latest → nag printed.""" + """Check stale cache + pinned tag older than latest → nag printed. + + No terraform stub: the check only runs once the hook is about to + exit non-zero, so the hook is left to fail on its own. + """ dispatcher = _GitDispatcherStub(tmp_path) - _create_terraform_stub(dispatcher) # Create empty commit and get its SHA subprocess.run( # noqa: S603 @@ -530,10 +537,10 @@ def test_stale_cache_outdated_tag_nag( # pragma: win32 no cover path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' # Create a stale cache file (8 days old) - cache_file = cache_dir / '.last_update_check' - cache_file.parent.mkdir(parents=True, exist_ok=True) + time_cache_file = cache_dir / '.last_update_check_time' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) - cache_file.write_text(str(eight_days_ago), encoding='utf-8') + time_cache_file.write_text(str(eight_days_ago), encoding='utf-8') hook_run = _run_hook( 'terraform_fmt.sh', @@ -550,9 +557,9 @@ def test_stale_cache_outdated_tag_nag( # pragma: win32 no cover assert 'v1.4.0' in combined, combined # Cache file should be updated to now (or very recent) - new_timestamp = int(cache_file.read_text(encoding='utf-8')) + new_timestamp = _read_cache_timestamp(time_cache_file) assert new_timestamp > eight_days_ago - assert hook_run.returncode == 0, combined + assert hook_run.returncode != 0, combined def test_stale_cache_untagged_nag( # pragma: win32 no cover @@ -560,19 +567,22 @@ def test_stale_cache_untagged_nag( # pragma: win32 no cover cache_dir: Path, tmp_path: Path, ) -> None: - """Check stale/absent cache + HEAD matches no tag → nag printed.""" + """Check stale/absent cache + HEAD matches no tag → nag printed. + + No terraform stub: the hook fails on its own, which is what arms + the check in the first place. + """ dispatcher = _GitDispatcherStub(tmp_path) dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) - _create_terraform_stub(dispatcher) sandbox_path_dir = _sandbox_path_dir(tmp_path) path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' # Create a stale cache file (8 days old) - cache_file = cache_dir / '.last_update_check' - cache_file.parent.mkdir(parents=True, exist_ok=True) + time_cache_file = cache_dir / '.last_update_check_time' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) - cache_file.write_text(str(eight_days_ago), encoding='utf-8') + time_cache_file.write_text(str(eight_days_ago), encoding='utf-8') hook_run = _run_hook( 'terraform_fmt.sh', @@ -588,9 +598,9 @@ def test_stale_cache_untagged_nag( # pragma: win32 no cover assert 'v1.4.0' in combined, combined # Latest tag should be mentioned # Cache file should be updated - new_timestamp = int(cache_file.read_text(encoding='utf-8')) + new_timestamp = _read_cache_timestamp(time_cache_file) assert new_timestamp > eight_days_ago - assert hook_run.returncode == 0, combined + assert hook_run.returncode != 0, combined def test_stale_cache_up_to_date_silent( # pragma: win32 no cover @@ -598,9 +608,13 @@ def test_stale_cache_up_to_date_silent( # pragma: win32 no cover cache_dir: Path, tmp_path: Path, ) -> None: - """Check stale/absent cache + pinned tag equals latest → no output.""" + """Check stale/absent cache + pinned tag equals latest → no output. + + No terraform stub: the hook still fails (for its own, unrelated + reason), which is exactly the point - even on a failing hook, an + already-up-to-date pin must stay silent. + """ dispatcher = _GitDispatcherStub(tmp_path) - _create_terraform_stub(dispatcher) # Get current HEAD sha and tag it as v1.4.0 (latest) current_sha = subprocess.run( # noqa: S603 @@ -629,10 +643,10 @@ def test_stale_cache_up_to_date_silent( # pragma: win32 no cover path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' # Create a stale cache file (8 days old) - cache_file = cache_dir / '.last_update_check' - cache_file.parent.mkdir(parents=True, exist_ok=True) + time_cache_file = cache_dir / '.last_update_check_time' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) - cache_file.write_text(str(eight_days_ago), encoding='utf-8') + time_cache_file.write_text(str(eight_days_ago), encoding='utf-8') hook_run = _run_hook( 'terraform_fmt.sh', @@ -649,9 +663,9 @@ def test_stale_cache_up_to_date_silent( # pragma: win32 no cover assert FAILED_MSG not in combined, combined # Cache file should still be updated (attempt was made, just silent) - new_timestamp = int(cache_file.read_text(encoding='utf-8')) + new_timestamp = _read_cache_timestamp(time_cache_file) assert new_timestamp > eight_days_ago - assert hook_run.returncode == 0, combined + assert hook_run.returncode != 0, combined def test_network_failure_warning( # pragma: win32 no cover @@ -659,19 +673,22 @@ def test_network_failure_warning( # pragma: win32 no cover cache_dir: Path, tmp_path: Path, ) -> None: - """Check dispatcher stub makes `ls-remote` fail → warn notice printed.""" + """Check dispatcher stub makes `ls-remote` fail → warn notice printed. + + No terraform stub: the hook fails on its own, arming the check, + which then separately fails again over the (simulated) network. + """ dispatcher = _GitDispatcherStub(tmp_path) dispatcher.set_ls_remote_output('', exit_code=1) # Non-zero exit - _create_terraform_stub(dispatcher) sandbox_path_dir = _sandbox_path_dir(tmp_path) path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' # Create a stale cache file - cache_file = cache_dir / '.last_update_check' - cache_file.parent.mkdir(parents=True, exist_ok=True) + time_cache_file = cache_dir / '.last_update_check_time' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) - cache_file.write_text(str(eight_days_ago), encoding='utf-8') + time_cache_file.write_text(str(eight_days_ago), encoding='utf-8') hook_run = _run_hook( 'terraform_fmt.sh', @@ -686,9 +703,146 @@ def test_network_failure_warning( # pragma: win32 no cover assert 'exit 1' in combined, combined # Cache file should be updated (attempt was made, even though it failed) - new_timestamp = int(cache_file.read_text(encoding='utf-8')) + new_timestamp = _read_cache_timestamp(time_cache_file) assert new_timestamp > eight_days_ago - assert hook_run.returncode == 0, combined # Hook's own work unaffected + # The hook's own (unrelated) failure is what armed the check; + # nothing about the check itself adds to or changes that exit code. + assert hook_run.returncode != 0, combined + + +def test_network_failure_preserves_cached_latest( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check a failed remote query keeps the previously cached tags. + + Only the timestamp (line 1) advances; the previously cached tag + lines must survive a failed attempt untouched, so the fast path + can keep using them once the cache goes fresh again. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output('', exit_code=1) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + time_cache_file = cache_dir / '.last_update_check_time' + tags_cache_file = cache_dir / '.last_update_check_tags' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) + eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) + previously_cached_tags = ( + 'cccccccccccccccccccccccccccccccccccccccc\trefs/tags/v1.2.0\n' + ) + time_cache_file.write_text(str(eight_days_ago), encoding='utf-8') + tags_cache_file.write_text(previously_cached_tags, encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + assert FAILED_MSG in hook_run.stdout, hook_run.stdout + assert _read_cache_timestamp(time_cache_file) > eight_days_ago + assert ( + tags_cache_file.read_text(encoding='utf-8') == previously_cached_tags + ) + + +def test_fresh_cache_still_nags_when_outdated( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check a fresh cache still nags every failing run if still outdated. + + Rate-limiting only throttles the *network query*, not the nag + itself: the dispatcher's configured `ls-remote` output claims + v9.9.9 is latest, but since the cache is fresh that must never be + queried - if this test observes "v9.9.9" anywhere, the fast path + incorrectly hit the network instead of using the cached v1.2.0. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output( + 'ffffffffffffffffffffffffffffffffffffffff\trefs/tags/v9.9.9\n', + ) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + time_cache_file = cache_dir / '.last_update_check_time' + tags_cache_file = cache_dir / '.last_update_check_tags' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) + one_hour_ago = int(time.time()) - _SECONDS_PER_HOUR + stale_sha = 'cccccccccccccccccccccccccccccccccccccccc' + stale_tags_line = f'{stale_sha}\trefs/tags/v1.2.0\n' + time_cache_file.write_text(str(one_hour_ago), encoding='utf-8') + tags_cache_file.write_text(stale_tags_line, encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert UNTAGGED_MSG in combined, combined + assert 'v1.2.0' in combined, combined + assert 'v9.9.9' not in combined, combined + + # Fast path never touches either cache file. + assert time_cache_file.read_text(encoding='utf-8') == str(one_hour_ago) + assert tags_cache_file.read_text(encoding='utf-8') == stale_tags_line + assert hook_run.returncode != 0, combined + + +def test_fresh_cache_stays_silent_when_matching( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check a fresh cache stays silent when HEAD matches cached latest.""" + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output( + 'ffffffffffffffffffffffffffffffffffffffff\trefs/tags/v9.9.9\n', + ) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + current_sha = subprocess.run( # noqa: S603 + (GIT, 'rev-parse', 'HEAD'), + cwd=tmp_repo, + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + time_cache_file = cache_dir / '.last_update_check_time' + tags_cache_file = cache_dir / '.last_update_check_tags' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) + one_hour_ago = int(time.time()) - _SECONDS_PER_HOUR + time_cache_file.write_text(str(one_hour_ago), encoding='utf-8') + tags_cache_file.write_text( + f'{current_sha}\trefs/tags/v1.4.0\n', + encoding='utf-8', + ) + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert OUTDATED_MSG not in combined, combined + assert UNTAGGED_MSG not in combined, combined + assert 'v9.9.9' not in combined, combined + assert hook_run.returncode != 0, combined def test_check_at_most_once_per_invocation( # pragma: win32 no cover @@ -696,10 +850,14 @@ def test_check_at_most_once_per_invocation( # pragma: win32 no cover cache_dir: Path, tmp_path: Path, ) -> None: - """Check hook invoked across multiple dirs attempts check at most once.""" + """Check hook invoked across multiple dirs attempts check at most once. + + No terraform stub: the hook fails on its own, which is what arms + the `trap ... EXIT` in `hooks/_common.sh` exactly once for the + whole invocation - not once per per-dir subshell. + """ dispatcher = _GitDispatcherStub(tmp_path) dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) - _create_terraform_stub(dispatcher) sandbox_path_dir = _sandbox_path_dir(tmp_path) path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' @@ -744,10 +902,10 @@ def test_check_at_most_once_per_invocation( # pragma: win32 no cover nag_count = combined.count(UNTAGGED_MSG) assert nag_count == 1, f'Nag appeared {nag_count} times, expected 1' - # Cache file should exist (check was attempted) - cache_file = cache_dir / '.last_update_check' - assert cache_file.exists() - assert multi_dir_run.returncode == 0, combined + # Cache files should exist (check was attempted) + assert (cache_dir / '.last_update_check_time').exists() + assert (cache_dir / '.last_update_check_tags').exists() + assert multi_dir_run.returncode != 0, combined def test_check_fires_without_per_dir_hook( # pragma: win32 no cover @@ -759,9 +917,10 @@ def test_check_fires_without_per_dir_hook( # pragma: win32 no cover `terraform_wrapper_module_for_each.sh` has its own whole-repo flow and never calls `common::per_dir_hook` (see `hooks/_common.sh` - - `common::maybe_notify_new_version` must run as a top-level statement - in `_common.sh` itself, not from inside `common::per_dir_hook`, or - this exact hook silently loses coverage). The hook's own tool + `_check_new_version_on_failure` must be wired up regardless, via + `common::initialize`, not from inside + `common::per_dir_hook`, or this exact hook silently loses + coverage). The hook's own tool (`hcledit`) is deliberately left unstubbed and its exit code is deliberately not asserted: only the notice's presence, printed before the hook ever reaches its own tool resolution, is under test. @@ -783,8 +942,8 @@ def test_check_fires_without_per_dir_hook( # pragma: win32 no cover assert UNTAGGED_MSG in combined, combined assert AUTOUPDATE_MSG in combined, combined - cache_file = cache_dir / '.last_update_check' - assert cache_file.exists() + assert (cache_dir / '.last_update_check_time').exists() + assert (cache_dir / '.last_update_check_tags').exists() @pytest.mark.network @@ -795,32 +954,86 @@ def test_real_network_sanity_check( # pragma: win32 no cover ) -> None: """Sanity test hitting the real hardcoded URL. + No dispatcher and no terraform stub, deliberately: a dispatcher + would intercept `ls-remote` with its own canned/empty response + (defeating the point of a *real*-network test) even if never + explicitly configured via `set_ls_remote_output`, and a terraform + stub would make the hook succeed - which would mean the check, + gated on hook failure, never runs at all. `_sandbox_path_dir` alone + already includes real `git` (hits real network) and no + terraform/tofu (hook fails, arming the check). + Asserts only that the call succeeds and returns parseable - `sharefs/tags/...` lines - no assertion on a specific version number. + `sharefs/tags/...` lines - no assertion on a specific version + number, which changes over time. """ - # Create terraform stub for this test - # Need dispatcher for stub creation even though not used for network - dispatcher = _GitDispatcherStub(tmp_path) - _create_terraform_stub(dispatcher) - - # Use real PATH (no dispatcher) to hit real network, but include stub dir - path_with_stub = f'{dispatcher.path_entry}:{os.environ["PATH"]}' + sandbox_path_dir = _sandbox_path_dir(tmp_path) hook_run = _run_hook( 'terraform_fmt.sh', [], cwd=tmp_repo, - env=_hook_env(_pct_cache_env(cache_dir), path_with_stub), + env=_hook_env(_pct_cache_env(cache_dir), str(sandbox_path_dir)), ) combined = hook_run.stdout # Either the check succeeds (and we might see a nag if outdated/untagged) # or it fails with timeout/network error (and we see failure message) - # or it's skipped due to fresh cache (first run creates cache) # Cache file should exist (attempt was made) - cache_file = cache_dir / '.last_update_check' - assert cache_file.exists() + assert (cache_dir / '.last_update_check_time').exists() + assert (cache_dir / '.last_update_check_tags').exists() + + # The hook fails on its own (no terraform/tofu) - that's what arms + # the check; nothing about the check itself changes this exit code. + assert hook_run.returncode != 0, combined + + +def test_hook_success_skips_check_even_when_outdated( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check a successful hook never runs the check, however outdated. + + Mirror image of every other stale-cache test above: same outdated + fixture and stale cache, but this time WITH a terraform stub so the + hook succeeds. `_check_new_version_on_failure` only + does anything when the hook's own exit code is non-zero, so a + successful hook must stay completely silent and leave the + cache file untouched - regardless of how outdated the + pin actually is. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + _create_terraform_stub(dispatcher) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + # Stale cache, so the only thing preventing an attempt is the + # hook's own success. + time_cache_file = cache_dir / '.last_update_check_time' + tags_cache_file = cache_dir / '.last_update_check_tags' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) + eight_days_ago = int(time.time()) - (8 * _SECONDS_PER_DAY) + time_cache_file.write_text(str(eight_days_ago), encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert OUTDATED_MSG not in combined, combined + assert UNTAGGED_MSG not in combined, combined + assert AUTOUPDATE_MSG not in combined, combined + assert TIMEOUT_MSG not in combined, combined + assert FAILED_MSG not in combined, combined - # Hook should complete successfully regardless + # Cache untouched: the check was never even attempted. + assert time_cache_file.read_text(encoding='utf-8') == str(eight_days_ago) + assert not tags_cache_file.exists() assert hook_run.returncode == 0, combined From 529458576f756570621f7db05ad8c857b32cc91e Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 00:16:51 +0300 Subject: [PATCH 03/16] fix: Resolve HEAD from hook checkout, not linted project `git rev-parse HEAD` ran in the process's CWD, which pre-commit sets to the linted project - never this hook's own checkout. The pinned rev comparison was always wrong: current_sha never matched the tag list, so a failing hook always nagged "non-release commit" whatever the actual pin was. Resolve HEAD via `-C "$hooks_dir"` instead. Also guard the `timeout` dependency - absent on stock macOS - so a missing binary doesn't get misreported as a network failure. Addresses CodeRabbit review comments on PR #1019. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- hooks/_check_new_version_on_failure.sh | 13 ++- tests/pytest/update_notification_test.py | 104 +++++++++-------------- 2 files changed, 50 insertions(+), 67 deletions(-) diff --git a/hooks/_check_new_version_on_failure.sh b/hooks/_check_new_version_on_failure.sh index 3a744e996..57489247d 100755 --- a/hooks/_check_new_version_on_failure.sh +++ b/hooks/_check_new_version_on_failure.sh @@ -29,7 +29,11 @@ function _check_new_version_on_failure { local -r time_cache_file="$cache_root/.last_update_check_time" # Hold the raw `git ls-remote --tags` output, one "refs/tags/" line per tag local -r tags_cache_file="$cache_root/.last_update_check_tags" - local -r current_sha=$(git rev-parse HEAD) + # HEAD of *this* hook's own checkout - the pinned `rev` - not of the + # user's project repo, which is this function's actual CWD. + local -r hooks_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" + local current_sha + current_sha=$(git -C "$hooks_dir" rev-parse HEAD 2> /dev/null) || current_sha="" # # Try to get tags from valid cache when possible # @@ -47,8 +51,13 @@ function _check_new_version_on_failure { # if [[ -z $known_tags ]]; then local fresh_output + # `timeout` isn't guaranteed on every platform (e.g. stock macOS + # without GNU coreutils) - skip wrapping with it when absent rather + # than failing with a misleading "exit 127" before git even runs. + local -a timeout_cmd=() + command -v timeout > /dev/null && timeout_cmd=(timeout 3) - if fresh_output=$(timeout 3 git ls-remote --tags --refs --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform 2>&1); then + if fresh_output=$("${timeout_cmd[@]}" git ls-remote --tags --refs --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform 2>&1); then known_tags=$fresh_output mkdir -p "$cache_root" date +%s > "$time_cache_file" diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index 16725e44b..76cf7d2c7 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -68,13 +68,15 @@ class _GitDispatcherStub: - """A `git` dispatcher stub that forwards all commands except `ls-remote`. + """A `git` dispatcher stub forwarding all commands except two. When placed ahead of the real `git` on `$PATH`, this stub intercepts - `git ls-remote` calls and returns canned fixture data, while forwarding - all other subcommands (`rev-parse`, `ls-files`, etc.) to the real `git`. - This keeps the hook's genuine git usage real while making the remote - tag query fully deterministic and network-free. + `git ls-remote` and the hook's own `git -C rev-parse HEAD` + lookup, returning canned fixture data for both, while forwarding all + other subcommands (`rev-parse HEAD` without `-C`, `ls-files`, etc.) to + the real `git`. This keeps the hook's genuine git usage real while + making both the remote tag query and the hook's own pinned-sha lookup + fully deterministic and network-free. """ def __init__(self, tmp_path: Path) -> None: # pragma: win32 no cover @@ -106,6 +108,13 @@ def __init__(self, tmp_path: Path) -> None: # pragma: win32 no cover ' # Default: empty tag list\n' ' exit 0\n' ' fi\n' + 'elif [[ "$1" == "-C" && "$3" == "rev-parse" && "$4" == "HEAD" ]]; then\n' # noqa: E501 + ' # Intercept the hook checkout HEAD lookup\n' + ' if [[ -f "${0}.current-sha" ]]; then\n' + ' cat "${0}.current-sha"\n' + ' else\n' + ' exec "$(dirname "$0")/real-git" "$@"\n' + ' fi\n' 'else\n' ' # Forward all other commands to the real git\n' ' exec "$(dirname "$0")/real-git" "$@"\n' @@ -140,6 +149,18 @@ def set_ls_remote_output( # pragma: win32 no cover encoding='utf-8', ) + def set_current_sha(self, sha: str) -> None: # pragma: win32 no cover + """Configure the canned sha for the hook checkout's own `HEAD`. + + Args: + sha: The sha the hook should resolve its own pinned `rev` to. + """ + stub_dir, stub_name = self.stub_path.parent, self.stub_path.name + (stub_dir / f'{stub_name}.current-sha').write_text( + sha, + encoding='utf-8', + ) + @property def path_entry(self) -> str: # pragma: win32 no cover """The directory path to prepend to `PATH`.""" @@ -503,35 +524,11 @@ def test_stale_cache_outdated_tag_nag( # pragma: win32 no cover exit non-zero, so the hook is left to fail on its own. """ dispatcher = _GitDispatcherStub(tmp_path) - - # Create empty commit and get its SHA - subprocess.run( # noqa: S603 - (GIT, 'commit', '--allow-empty', '-m', 'bump', '--date=2000-01-01'), - cwd=tmp_repo, - check=True, - ) - head_rev_parse = subprocess.run( # noqa: S603 - (GIT, 'rev-parse', 'HEAD'), - cwd=tmp_repo, - capture_output=True, - text=True, - check=True, - ) - current_sha = head_rev_parse.stdout.strip() - - # Tag current HEAD as v1.3.0 - subprocess.run( # noqa: S603 - (GIT, 'tag', '-f', 'v1.3.0'), - cwd=tmp_repo, - check=True, - ) - - # Update ls-remote output to include current SHA as v1.3.0 - ls_remote_output = SAMPLE_LS_REMOTE_OUTPUT.replace( - 'dddddddddddddddddddddddddddddddddddddddd', - current_sha, - ) - dispatcher.set_ls_remote_output(ls_remote_output) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + # Pin the hook's own checkout to the v1.3.0 fixture sha already in + # `SAMPLE_LS_REMOTE_OUTPUT` above - not a tag/commit on `tmp_repo`, + # which is the *linted project*, never the hook's own checkout. + dispatcher.set_current_sha('dddddddddddddddddddddddddddddddddddddddd') sandbox_path_dir = _sandbox_path_dir(tmp_path) path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' @@ -615,29 +612,10 @@ def test_stale_cache_up_to_date_silent( # pragma: win32 no cover already-up-to-date pin must stay silent. """ dispatcher = _GitDispatcherStub(tmp_path) - - # Get current HEAD sha and tag it as v1.4.0 (latest) - current_sha = subprocess.run( # noqa: S603 - (GIT, 'rev-parse', 'HEAD'), - cwd=tmp_repo, - capture_output=True, - text=True, - check=True, - ).stdout.strip() - - # Create ls-remote output with current HEAD as v1.4.0 - ls_remote_output = SAMPLE_LS_REMOTE_OUTPUT.replace( - 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', - current_sha, - ) - dispatcher.set_ls_remote_output(ls_remote_output) - - # Tag current HEAD as v1.4.0 - subprocess.run( # noqa: S603 - (GIT, 'tag', '-f', 'v1.4.0'), - cwd=tmp_repo, - check=True, - ) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + # Pin the hook's own checkout to the v1.4.0 fixture sha (latest) - + # not a tag on `tmp_repo`, which is only the linted project. + dispatcher.set_current_sha('eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee') sandbox_path_dir = _sandbox_path_dir(tmp_path) path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' @@ -809,18 +787,14 @@ def test_fresh_cache_stays_silent_when_matching( # pragma: win32 no cover dispatcher.set_ls_remote_output( 'ffffffffffffffffffffffffffffffffffffffff\trefs/tags/v9.9.9\n', ) + # Pin the hook's own checkout to the same sha cached below as + # latest - not a tag on `tmp_repo`, the linted project. + current_sha = 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' + dispatcher.set_current_sha(current_sha) sandbox_path_dir = _sandbox_path_dir(tmp_path) path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' - current_sha = subprocess.run( # noqa: S603 - (GIT, 'rev-parse', 'HEAD'), - cwd=tmp_repo, - capture_output=True, - text=True, - check=True, - ).stdout.strip() - time_cache_file = cache_dir / '.last_update_check_time' tags_cache_file = cache_dir / '.last_update_check_tags' time_cache_file.parent.mkdir(parents=True, exist_ok=True) From 02b039cd88152b4f55575fb0350d00fe4199b14f Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 00:24:27 +0300 Subject: [PATCH 04/16] test: Make sandbox timeout optional, fix flaky network test `timeout` isn't on stock macOS; hard-requiring it in both test files' sandbox PATH would break the whole suite there, even for tests that never invoke it. Moved to optional in both. `test_real_network_sanity_check` asserted `.last_update_check_tags` unconditionally, but the failure path only ever writes the timestamp - flaky whenever GitHub is briefly unreachable. Branch on the failure messages instead, and validate the tag file's content on the success path per the test's own docstring promise. Addresses Copilot review comments on PR #1019. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- tests/pytest/tool_version_test.py | 4 +++- tests/pytest/update_notification_test.py | 21 ++++++++++++++++----- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/tests/pytest/tool_version_test.py b/tests/pytest/tool_version_test.py index 4ca8c0a6a..28e429572 100644 --- a/tests/pytest/tool_version_test.py +++ b/tests/pytest/tool_version_test.py @@ -150,7 +150,6 @@ class _HookWiring(NamedTuple): 'sed', 'sort', 'tail', - 'timeout', 'tr', 'uname', 'wc', @@ -176,6 +175,9 @@ class _HookWiring(NamedTuple): 'sysctl', 'tar', 'tee', + # Not on stock macOS (needs GNU coreutils); these tests always set + # `PCT_SKIP_UPDATE_CHECK=true` so they never actually invoke it. + 'timeout', 'touch', 'uniq', 'unzip', diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index 76cf7d2c7..bb7d1b2b9 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -215,7 +215,6 @@ def _sandbox_path_dir(base: Path) -> Path: # pragma: win32 no cover 'sed', 'sort', 'tail', - 'timeout', 'tr', 'uname', 'wc', @@ -241,6 +240,10 @@ def _sandbox_path_dir(base: Path) -> Path: # pragma: win32 no cover 'sysctl', 'tar', 'tee', + # Not on stock macOS (needs GNU coreutils) - the hook itself + # already tolerates its absence (`command -v timeout` guard), + # so the sandbox must too, not hard-require it. + 'timeout', 'touch', 'uniq', 'unzip', @@ -952,11 +955,19 @@ def test_real_network_sanity_check( # pragma: win32 no cover combined = hook_run.stdout # Either the check succeeds (and we might see a nag if outdated/untagged) - # or it fails with timeout/network error (and we see failure message) - - # Cache file should exist (attempt was made) + # or it fails with timeout/network error (and we see failure message) - + # the timestamp advances on any real attempt either way, but the tag + # cache is only ever written on success (see design.md Decision 3), so + # asserting it unconditionally would flake whenever GitHub is briefly + # unreachable from CI. assert (cache_dir / '.last_update_check_time').exists() - assert (cache_dir / '.last_update_check_tags').exists() + if TIMEOUT_MSG not in combined and FAILED_MSG not in combined: + tags_cache_file = cache_dir / '.last_update_check_tags' + assert tags_cache_file.exists() + # Parseable `sharefs/tags/...` lines, per this test's own + # docstring promise. + tags_content = tags_cache_file.read_text(encoding='utf-8') + assert '\trefs/tags/' in tags_content, tags_content # The hook fails on its own (no terraform/tofu) - that's what arms # the check; nothing about the check itself changes this exit code. From 91ac5c38baf4b229808038c57101e41ff9786000 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 00:35:51 +0300 Subject: [PATCH 05/16] fix: Bound git ls-remote without depending on `timeout` `timeout` isn't on stock macOS, so absence of the binary meant the 3s bound was silently dropped, leaving a stalled DNS/network request able to hang the failed hook indefinitely. Replace it with a portable watchdog: run the query in the background, race it against a `sleep 3` that kills it if it overruns. Only needs bash + kill/sleep/ mktemp, already assumed available everywhere else in this file. Also mark the real-network test's success-branch `# pragma: no cover` - whether GitHub answers within 3s during a given test run is inherently non-deterministic, so gating the coverage threshold on it isn't viable. Addresses a CodeRabbit review comment on PR #1019. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- hooks/_check_new_version_on_failure.sh | 33 ++++++++++++++++++------ tests/pytest/update_notification_test.py | 4 ++- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/hooks/_check_new_version_on_failure.sh b/hooks/_check_new_version_on_failure.sh index 57489247d..1908d751f 100755 --- a/hooks/_check_new_version_on_failure.sh +++ b/hooks/_check_new_version_on_failure.sh @@ -50,21 +50,38 @@ function _check_new_version_on_failure { # No/stale cache, need to go to network # if [[ -z $known_tags ]]; then - local fresh_output # `timeout` isn't guaranteed on every platform (e.g. stock macOS - # without GNU coreutils) - skip wrapping with it when absent rather - # than failing with a misleading "exit 127" before git even runs. - local -a timeout_cmd=() - command -v timeout > /dev/null && timeout_cmd=(timeout 3) + # without GNU coreutils), so the 3s bound is enforced by hand: run + # `git ls-remote` in the background, race it against a `sleep 3` + # watchdog, and kill whichever loses. Output goes to a temp file + # since a backgrounded command can't be captured with `$(...)`. + local fresh_output + local tmp_output + tmp_output=$(mktemp) + git ls-remote --tags --refs --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform > "$tmp_output" 2>&1 & + local git_pid=$! + ( + sleep 3 + kill -9 "$git_pid" 2> /dev/null || true + ) & + local watchdog_pid=$! + + local exit_code=0 + wait "$git_pid" 2> /dev/null || exit_code=$? + kill "$watchdog_pid" 2> /dev/null || true + wait "$watchdog_pid" 2> /dev/null || true + + fresh_output=$(< "$tmp_output") + rm -f "$tmp_output" - if fresh_output=$("${timeout_cmd[@]}" git ls-remote --tags --refs --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform 2>&1); then + if [[ $exit_code -eq 0 ]]; then known_tags=$fresh_output mkdir -p "$cache_root" date +%s > "$time_cache_file" echo "$known_tags" > "$tags_cache_file" else - local exit_code=$? - if [[ $exit_code -eq 124 ]]; then + # 137 = 128 + SIGKILL(9) - the watchdog fired. + if [[ $exit_code -eq 137 ]]; then common::colorify "yellow" "Update check timed out." else common::colorify "yellow" "Update check failed (exit ${exit_code})." diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index bb7d1b2b9..5547ba08f 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -961,7 +961,9 @@ def test_real_network_sanity_check( # pragma: win32 no cover # asserting it unconditionally would flake whenever GitHub is briefly # unreachable from CI. assert (cache_dir / '.last_update_check_time').exists() - if TIMEOUT_MSG not in combined and FAILED_MSG not in combined: + if ( + TIMEOUT_MSG not in combined and FAILED_MSG not in combined + ): # pragma: no cover tags_cache_file = cache_dir / '.last_update_check_tags' assert tags_cache_file.exists() # Parseable `sharefs/tags/...` lines, per this test's own From 7fbf10b6f55218eca7069b76ea5f5481f263e973 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 01:16:54 +0300 Subject: [PATCH 06/16] fix issues mentioned by copilot --- hooks/_check_new_version_on_failure.sh | 19 ++++++-- tests/pytest/tool_version_test.py | 1 + tests/pytest/update_notification_test.py | 55 ++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/hooks/_check_new_version_on_failure.sh b/hooks/_check_new_version_on_failure.sh index 1908d751f..8fe6b1ea4 100755 --- a/hooks/_check_new_version_on_failure.sh +++ b/hooks/_check_new_version_on_failure.sh @@ -63,7 +63,14 @@ function _check_new_version_on_failure { ( sleep 3 kill -9 "$git_pid" 2> /dev/null || true - ) & + # Redirected above: if `sleep`'s own child process outlives the + # `kill` sent to this subshell below (SIGTERM to a foreground + # `sleep` orphans it rather than propagating), an inherited copy + # of the caller's stdout/stderr pipe would otherwise stay open - + # and callers reading that pipe until EOF (e.g. Python's + # `subprocess.communicate`) would block for the orphan's full + # remaining sleep, not just until `git` actually finishes. + ) > /dev/null 2>&1 & local watchdog_pid=$! local exit_code=0 @@ -121,5 +128,11 @@ function _check_new_version_on_failure { common::colorify "yellow" 'Run "pre-commit autoupdate --freeze" (or "prek update --freeze") to upgrade.' } -# Check for update only on hooks failure -trap '[[ $? -ne 0 ]] && _check_new_version_on_failure' EXIT +# Check for update only on hooks failure. `errexit` is disabled around +# the call and the original pending status is captured/re-exited +# explicitly - otherwise any unguarded failure inside the checker +# itself (e.g. an unwritable cache dir) would, under `set -e`, replace +# the hook's real exit code with the checker's own failure instead of +# just being a best-effort, non-fatal notice. +# shellcheck disable=SC2154 # False positive: assigned inside the trap string itself +trap '_pct_update_check_exit_code=$?; [[ $_pct_update_check_exit_code -ne 0 ]] && { set +e; _check_new_version_on_failure; set -e; }; exit $_pct_update_check_exit_code' EXIT diff --git a/tests/pytest/tool_version_test.py b/tests/pytest/tool_version_test.py index 28e429572..7684626f2 100644 --- a/tests/pytest/tool_version_test.py +++ b/tests/pytest/tool_version_test.py @@ -148,6 +148,7 @@ class _HookWiring(NamedTuple): 'mktemp', 'rm', 'sed', + 'sleep', 'sort', 'tail', 'tr', diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index 5547ba08f..6071df304 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -38,6 +38,11 @@ _SECONDS_PER_HOUR = 3600 _SECONDS_PER_DAY = 86400 +# Generous upper bound (vs. the ~3s the watchdog itself targets): catches +# a broken watchdog without flaking on a loaded CI box, while still being +# far short of the hung call's real 60s / the 30s subprocess timeout +# either would hit if the watchdog never fired at all. +_WATCHDOG_BOUND_SECONDS = 10 # Diagnostic messages emitted via `common::colorify` calls in # `hooks/_check_new_version_on_failure.sh`. @@ -96,6 +101,9 @@ def __init__(self, tmp_path: Path) -> None: # pragma: win32 no cover '\n' 'if [[ "$1" == "ls-remote" ]]; then\n' ' # Intercept ls-remote calls\n' + ' if [[ -f "${0}.ls-remote-hang" ]]; then\n' + ' sleep 60\n' + ' fi\n' ' if [[ -f "${0}.ls-remote-output" ]]; then\n' ' cat "${0}.ls-remote-output"\n' ' if [[ -f "${0}.ls-remote-exitcode" ]]; then\n' @@ -149,6 +157,15 @@ def set_ls_remote_output( # pragma: win32 no cover encoding='utf-8', ) + def set_ls_remote_hang(self) -> None: # pragma: win32 no cover + """Make the canned `ls-remote` call hang instead of returning. + + Used to prove the watchdog actually bounds a stalled query, + rather than a canned instant exit code that never exercises it. + """ + stub_dir, stub_name = self.stub_path.parent, self.stub_path.name + (stub_dir / f'{stub_name}.ls-remote-hang').touch() + def set_current_sha(self, sha: str) -> None: # pragma: win32 no cover """Configure the canned sha for the hook checkout's own `HEAD`. @@ -213,6 +230,7 @@ def _sandbox_path_dir(base: Path) -> Path: # pragma: win32 no cover 'mktemp', 'rm', 'sed', + 'sleep', 'sort', 'tail', 'tr', @@ -732,6 +750,43 @@ def test_network_failure_preserves_cached_latest( # pragma: win32 no cover ) +def test_network_query_bounded_by_watchdog( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check a hung `ls-remote` gets killed by the watchdog within ~3s. + + Proves the portable watchdog (no `timeout` dependency) actually + bounds a stalled query, rather than merely asserting the exit-code + branch it *would* take on a real timeout - a canned instant exit + code would never exercise the watchdog at all, exactly the gap that + let a missing `sleep` on the sandboxed `PATH` silently disable it. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_hang() + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + start = time.monotonic() + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + elapsed = time.monotonic() - start + + combined = hook_run.stdout + assert TIMEOUT_MSG in combined, combined + assert SKIP_SUGGESTION_MSG in combined, combined + assert elapsed < _WATCHDOG_BOUND_SECONDS, ( + f'took {elapsed:.1f}s, watchdog should bound to ~3s' + ) + assert hook_run.returncode != 0, combined + + def test_fresh_cache_still_nags_when_outdated( # pragma: win32 no cover tmp_repo: Path, cache_dir: Path, From 9eace7d92322b1f354ab8c0ce7f1d91305e05281 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 01:22:19 +0300 Subject: [PATCH 07/16] fix: Match annotated tags, honor throttle without a tag cache `git ls-remote --tags --refs` drops the peeled `^{}` record for annotated tags, leaving only the tag *object* OID against the bare ref - never a commit OID. Verified against the real upstream repo: `v1.50.0` is annotated there, and its tag-object OID differs from the commit it actually points to. Every annotated-tag pin was therefore reported as a non-release commit, forever. Drop `--refs`, collapse each peeled/object pair to one commit-sha-per-tag line before caching. Also: the fast-path guard required a tag cache to exist even when only checking timestamp freshness, so a persistently failing network re-queried on every single invocation instead of respecting the 7-day throttle. Decouple "is it time to retry" from "do we have tag data" - rate-limited with nothing cached now stays silent rather than re-querying or nagging off no data. Addresses CodeRabbit review comments on PR #1019. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- hooks/_check_new_version_on_failure.sh | 47 +++++++++++-- tests/pytest/update_notification_test.py | 88 ++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 6 deletions(-) diff --git a/hooks/_check_new_version_on_failure.sh b/hooks/_check_new_version_on_failure.sh index 8fe6b1ea4..26b2eaf20 100755 --- a/hooks/_check_new_version_on_failure.sh +++ b/hooks/_check_new_version_on_failure.sh @@ -27,7 +27,9 @@ function _check_new_version_on_failure { local -r cache_root="${PCT_TOOL_CACHE_DIR:-${XDG_CACHE_HOME:-$HOME/.cache}/pre-commit-terraform}" # Holds only the last-checked timestamp local -r time_cache_file="$cache_root/.last_update_check_time" - # Hold the raw `git ls-remote --tags` output, one "refs/tags/" line per tag + # Hold the tag list, one "refs/tags/" line per + # tag - normalized from the raw `git ls-remote --tags` output (see + # below), not that raw output verbatim local -r tags_cache_file="$cache_root/.last_update_check_tags" # HEAD of *this* hook's own checkout - the pinned `rev` - not of the # user's project repo, which is this function's actual CWD. @@ -38,18 +40,30 @@ function _check_new_version_on_failure { # Try to get tags from valid cache when possible # local known_tags="" + local cache_is_fresh=false if [[ -f $time_cache_file ]]; then local cached_time cached_time=$(< "$time_cache_file") local -r age_seconds=$(($(date +%s) - cached_time)) - if [[ $age_seconds -lt 604800 ]] && [[ -f $tags_cache_file ]]; then - known_tags=$(< "$tags_cache_file") + if [[ $age_seconds -lt 604800 ]]; then + cache_is_fresh=true + [[ -f $tags_cache_file ]] && known_tags=$(< "$tags_cache_file") fi fi + + if [[ $cache_is_fresh == true && -z $known_tags ]]; then + # Rate-limited, and no tag data has ever been cached (e.g. every + # attempt this week has failed) - nothing to compare against, so + # stay silent rather than nagging off no data. Only the timestamp, + # not the tag list, is what the 7-day window actually gates - a + # second failing run minutes after the first must not re-query + # just because no tags happen to exist yet. + return + fi # # No/stale cache, need to go to network # - if [[ -z $known_tags ]]; then + if [[ $cache_is_fresh == false ]]; then # `timeout` isn't guaranteed on every platform (e.g. stock macOS # without GNU coreutils), so the 3s bound is enforced by hand: run # `git ls-remote` in the background, race it against a `sleep 3` @@ -58,7 +72,7 @@ function _check_new_version_on_failure { local fresh_output local tmp_output tmp_output=$(mktemp) - git ls-remote --tags --refs --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform > "$tmp_output" 2>&1 & + git ls-remote --tags --sort=version:refname https://github.com/antonbabenko/pre-commit-terraform > "$tmp_output" 2>&1 & local git_pid=$! ( sleep 3 @@ -82,7 +96,28 @@ function _check_new_version_on_failure { rm -f "$tmp_output" if [[ $exit_code -eq 0 ]]; then - known_tags=$fresh_output + # Without `--refs`, `git ls-remote` yields *two* lines for an + # annotated tag: its own ref (sha = the tag *object*, never a + # commit) and a peeled "^{}" line (sha = the commit it + # actually points at). Only the peeled sha can ever match + # `current_sha`, so collapse each pair to one line, preferring + # the peeled sha whenever a tag has one; lightweight tags (single + # line, already a commit sha) pass through unchanged. + known_tags=$(awk -v OFS='\t' ' + { + if (prev_ref != "" && $2 == prev_ref "^{}") { + print $1, prev_ref + prev_ref = "" + next + } + if (prev_ref != "") print prev_sha, prev_ref + prev_sha = $1 + prev_ref = $2 + } + END { + if (prev_ref != "") print prev_sha, prev_ref + } + ' <<< "$fresh_output") mkdir -p "$cache_root" date +%s > "$time_cache_file" echo "$known_tags" > "$tags_cache_file" diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index 6071df304..d7a81f913 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -667,6 +667,48 @@ def test_stale_cache_up_to_date_silent( # pragma: win32 no cover assert hook_run.returncode != 0, combined +def test_annotated_tag_matches_via_peeled_commit( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check an annotated tag's peeled commit OID is what gets matched. + + `git ls-remote --tags` (no `--refs`) returns *two* lines for an + annotated tag: the tag *object* OID against the bare ref, and the + real commit OID against that same ref suffixed `^{}`. A checkout + pinned to the peeled/commit OID must be recognized as up-to-date - + matching only the (different) tag-object OID would report every + annotated-tag pin as a non-release commit, forever. Fixture values + are the real, verified OID pair for antonbabenko/pre-commit-terraform's + own `v1.50.0` tag (which is annotated upstream). + """ + dispatcher = _GitDispatcherStub(tmp_path) + tag_object_sha = 'd032af694c17201cfcbd4d5ac106dd37926d39f9' + peeled_commit_sha = '9b84f70efef7419e53c9526dff2e4a7d6bc9c78d' + dispatcher.set_ls_remote_output( + f'{tag_object_sha}\trefs/tags/v1.50.0\n' + f'{peeled_commit_sha}\trefs/tags/v1.50.0^{{}}\n', + ) + dispatcher.set_current_sha(peeled_commit_sha) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert OUTDATED_MSG not in combined, combined + assert UNTAGGED_MSG not in combined, combined + assert AUTOUPDATE_MSG not in combined, combined + assert hook_run.returncode != 0, combined + + def test_network_failure_warning( # pragma: win32 no cover tmp_repo: Path, cache_dir: Path, @@ -750,6 +792,52 @@ def test_network_failure_preserves_cached_latest( # pragma: win32 no cover ) +def test_second_failure_skips_network_without_tags( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check back-to-back failures honor the throttle without any tags. + + A first failing attempt (network down) writes only the timestamp - + no tag cache exists yet, since one is only ever written on success. + A second failing run minutes later must still skip the network + entirely: the 7-day throttle is gated on the timestamp alone, not + on whether a tag cache happens to already exist. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output('', exit_code=1) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + env = _hook_env(_pct_cache_env(cache_dir), path_with_dispatcher) + + first_run = _run_hook('terraform_fmt.sh', [], cwd=tmp_repo, env=env) + assert FAILED_MSG in first_run.stdout, first_run.stdout + + time_cache_file = cache_dir / '.last_update_check_time' + tags_cache_file = cache_dir / '.last_update_check_tags' + assert time_cache_file.exists() + assert not tags_cache_file.exists() + first_timestamp = _read_cache_timestamp(time_cache_file) + + # Detectably-different data: if the second run queries the network + # at all, this would show up in its output, proving the throttle + # was bypassed instead of actually skipping the attempt. + dispatcher.set_ls_remote_output( + 'ffffffffffffffffffffffffffffffffffffffff\trefs/tags/v9.9.9\n', + ) + + second_run = _run_hook('terraform_fmt.sh', [], cwd=tmp_repo, env=env) + combined = second_run.stdout + assert FAILED_MSG not in combined, combined + assert UNTAGGED_MSG not in combined, combined + assert 'v9.9.9' not in combined, combined + assert not tags_cache_file.exists() + # No real attempt was made this time, so the timestamp is untouched. + assert _read_cache_timestamp(time_cache_file) == first_timestamp + + def test_network_query_bounded_by_watchdog( # pragma: win32 no cover tmp_repo: Path, cache_dir: Path, From 422810814634cdcae48880b6e2be44ebfcc8289d Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 01:38:42 +0300 Subject: [PATCH 08/16] style: Format the EXIT trap as a multiline string Same trap body, same behavior (verified exit-code preservation across all three cases: checker fails internally, checker succeeds, hook succeeds so checker never runs) - newlines instead of semicolons for readability, no wrapper function. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- hooks/_check_new_version_on_failure.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/hooks/_check_new_version_on_failure.sh b/hooks/_check_new_version_on_failure.sh index 26b2eaf20..de9dcde59 100755 --- a/hooks/_check_new_version_on_failure.sh +++ b/hooks/_check_new_version_on_failure.sh @@ -170,4 +170,12 @@ function _check_new_version_on_failure { # the hook's real exit code with the checker's own failure instead of # just being a best-effort, non-fatal notice. # shellcheck disable=SC2154 # False positive: assigned inside the trap string itself -trap '_pct_update_check_exit_code=$?; [[ $_pct_update_check_exit_code -ne 0 ]] && { set +e; _check_new_version_on_failure; set -e; }; exit $_pct_update_check_exit_code' EXIT +trap ' + _pct_update_check_exit_code=$? + if [[ $_pct_update_check_exit_code -ne 0 ]]; then + set +e + _check_new_version_on_failure + set -e + fi + exit $_pct_update_check_exit_code +' EXIT From 5609afecd3b9384b65ea005ceada978a7106be11 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 01:57:58 +0300 Subject: [PATCH 09/16] fix: Validate cached timestamp before bash arithmetic No file locking (design.md) means a torn/partial write can leave `.last_update_check_time` holding garbage instead of a plain integer. Verified empirically: feeding that into bash arithmetic doesn't crash the process, but it does make the function return early, silently and permanently, since the failing statement sits before the cache ever gets rewritten - the check never recovers on its own. Validate as `[0-9]+` before arithmetic, so a corrupt cache is treated as absent (triggers a real attempt, which fixes the cache going forward) instead of wedging the check forever. Also reject negative age (a bogus future-dated timestamp), which previously satisfied "< 7 days" and got treated as fresh indefinitely. Addresses a Copilot review comment on PR #1019. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- hooks/_check_new_version_on_failure.sh | 13 ++-- tests/pytest/update_notification_test.py | 82 ++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 4 deletions(-) diff --git a/hooks/_check_new_version_on_failure.sh b/hooks/_check_new_version_on_failure.sh index de9dcde59..c2efbbef7 100755 --- a/hooks/_check_new_version_on_failure.sh +++ b/hooks/_check_new_version_on_failure.sh @@ -44,10 +44,15 @@ function _check_new_version_on_failure { if [[ -f $time_cache_file ]]; then local cached_time cached_time=$(< "$time_cache_file") - local -r age_seconds=$(($(date +%s) - cached_time)) - if [[ $age_seconds -lt 604800 ]]; then - cache_is_fresh=true - [[ -f $tags_cache_file ]] && known_tags=$(< "$tags_cache_file") + # A torn/partial write can leave a malformed or empty timestamp, so validate first + if [[ $cached_time =~ ^[0-9]+$ ]]; then + local -r age_seconds=$(($(date +%s) - cached_time)) + # Negative age = a bogus future-dated timestamp (e.g. clock skew + # or the same corruption above) - never treat that as fresh. + if [[ $age_seconds -ge 0 ]] && [[ $age_seconds -lt 604800 ]]; then + cache_is_fresh=true + [[ -f $tags_cache_file ]] && known_tags=$(< "$tags_cache_file") + fi fi fi diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index d7a81f913..3097062db 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -534,6 +534,88 @@ def test_pct_skip_update_check_set_skips_check( # pragma: win32 no cover assert hook_run.returncode != 0, combined +def test_corrupted_timestamp_treated_as_stale( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check a malformed cached timestamp is treated as absent, not fatal. + + No file locking (see design.md) means a torn/partial write can + leave `.last_update_check_time` holding garbage instead of a plain + integer. Feeding that straight into bash arithmetic either + silently becomes 0 or raises an expression error depending on + exactly what landed there - neither of which this cache should + ever trust. A real attempt must still happen (and correct the + cache going forward), not silently break the check forever. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + time_cache_file = cache_dir / '.last_update_check_time' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) + time_cache_file.write_text('12345corrupted', encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert UNTAGGED_MSG in combined, combined + assert AUTOUPDATE_MSG in combined, combined + + # The real attempt corrects the cache going forward - a clean, + # current timestamp, not the garbage that was there before. + new_timestamp = _read_cache_timestamp(time_cache_file) + assert new_timestamp > int(time.time()) - _SECONDS_PER_HOUR + assert hook_run.returncode != 0, combined + + +def test_future_timestamp_treated_as_stale( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check a bogus future-dated timestamp is treated as stale, not fresh. + + A well-formed but future timestamp (clock skew, or the same kind + of corruption as a malformed one) would otherwise compute a + negative age - satisfying `age < 7 days` and getting treated as + "fresh" forever, permanently suppressing real checks. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_output(SAMPLE_LS_REMOTE_OUTPUT) + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + time_cache_file = cache_dir / '.last_update_check_time' + time_cache_file.parent.mkdir(parents=True, exist_ok=True) + future_time = int(time.time()) + _SECONDS_PER_DAY + time_cache_file.write_text(str(future_time), encoding='utf-8') + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + combined = hook_run.stdout + assert UNTAGGED_MSG in combined, combined + assert AUTOUPDATE_MSG in combined, combined + + new_timestamp = _read_cache_timestamp(time_cache_file) + assert new_timestamp <= int(time.time()) + assert hook_run.returncode != 0, combined + + def test_stale_cache_outdated_tag_nag( # pragma: win32 no cover tmp_repo: Path, cache_dir: Path, From 5ee081fc79286cd88fbb57367f38f60397011147 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 02:11:22 +0300 Subject: [PATCH 10/16] fix: Kill git's remote-helper child, not just git itself `git ls-remote https://...` spawns a separate remote-helper process (`git remote-https`) to do the actual network I/O - confirmed against a real invocation with a clean git config. The watchdog only killed `git_pid`; the helper could survive `SIGKILL` and keep running after the hook returned. Considered `setsid` (not on macOS) and bash job-control process groups (`set -m` hung indefinitely in non-interactive testing - unsafe in a hook that always runs non-interactively). Settled on walking `pgrep -P` recursively and killing children before parent, using only tools already portable to macOS. Dispatcher stub now forks a real child on a hang, mimicking git's actual process shape, so `test_watchdog_kills_remote_helper_child_too` can assert the child dies too via `os.kill(pid, 0)`. Addresses a CodeRabbit review comment on PR #1019. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- hooks/_check_new_version_on_failure.sh | 21 ++++++++- tests/pytest/tool_version_test.py | 1 + tests/pytest/update_notification_test.py | 58 +++++++++++++++++++++++- 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/hooks/_check_new_version_on_failure.sh b/hooks/_check_new_version_on_failure.sh index c2efbbef7..9a3fa7f6e 100755 --- a/hooks/_check_new_version_on_failure.sh +++ b/hooks/_check_new_version_on_failure.sh @@ -1,6 +1,25 @@ #!/usr/bin/env bash set -eo pipefail +####################################################################### +# Kill a process and all of its descendants, children first, so none +# get orphaned mid-kill. `git ls-remote https://...` spawns a separate +# remote-helper child (`git remote-https`, confirmed via a real +# invocation) to do the actual network I/O - killing only the parent +# PID lets that helper survive and keep running after the hook itself +# returns. +# Arguments: +# pid (string) PID of the process (and its descendants) to kill +####################################################################### +function _pct_kill_process_tree { + local -r pid=$1 + local child + for child in $(pgrep -P "$pid" 2> /dev/null); do + _pct_kill_process_tree "$child" + done + kill -9 "$pid" 2> /dev/null || true +} + ####################################################################### # Check for newer pre-commit-terraform release and notify if outdated. # The remote query is rate-limited to once per 7 days; within that @@ -81,7 +100,7 @@ function _check_new_version_on_failure { local git_pid=$! ( sleep 3 - kill -9 "$git_pid" 2> /dev/null || true + _pct_kill_process_tree "$git_pid" # Redirected above: if `sleep`'s own child process outlives the # `kill` sent to this subshell below (SIGTERM to a foreground # `sleep` orphans it rather than propagating), an inherited copy diff --git a/tests/pytest/tool_version_test.py b/tests/pytest/tool_version_test.py index 7684626f2..de06523d0 100644 --- a/tests/pytest/tool_version_test.py +++ b/tests/pytest/tool_version_test.py @@ -146,6 +146,7 @@ class _HookWiring(NamedTuple): 'head', 'mkdir', 'mktemp', + 'pgrep', 'rm', 'sed', 'sleep', diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index 3097062db..dc04f9ea1 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -43,6 +43,9 @@ # far short of the hung call's real 60s / the 30s subprocess timeout # either would hit if the watchdog never fired at all. _WATCHDOG_BOUND_SECONDS = 10 +# Grace period for the OS to finish reaping a just-killed process +# before a liveness check (`os.kill(pid, 0)`) is expected to be honest. +_PROCESS_REAP_GRACE_SECONDS = 0.2 # Diagnostic messages emitted via `common::colorify` calls in # `hooks/_check_new_version_on_failure.sh`. @@ -102,7 +105,10 @@ def __init__(self, tmp_path: Path) -> None: # pragma: win32 no cover 'if [[ "$1" == "ls-remote" ]]; then\n' ' # Intercept ls-remote calls\n' ' if [[ -f "${0}.ls-remote-hang" ]]; then\n' - ' sleep 60\n' + ' # Forked child, mimicking the real remote-helper process\n' + ' sleep 60 &\n' + ' echo "$!" > "${0}.ls-remote-hang-child-pid"\n' + ' wait\n' ' fi\n' ' if [[ -f "${0}.ls-remote-output" ]]; then\n' ' cat "${0}.ls-remote-output"\n' @@ -162,10 +168,25 @@ def set_ls_remote_hang(self) -> None: # pragma: win32 no cover Used to prove the watchdog actually bounds a stalled query, rather than a canned instant exit code that never exercises it. + The stub forks a child for the hang (see `hung_helper_pid`), + mimicking `git`'s own separate remote-helper process. """ stub_dir, stub_name = self.stub_path.parent, self.stub_path.name (stub_dir / f'{stub_name}.ls-remote-hang').touch() + def hung_helper_pid(self) -> int: # pragma: win32 no cover + """Read back the remote-helper child PID a hung call recorded. + + Only valid after `set_ls_remote_hang()` and an actual hung + invocation - raises `FileNotFoundError` otherwise. + + Returns: + The child's PID. + """ + stub_dir, stub_name = self.stub_path.parent, self.stub_path.name + pid_file = stub_dir / f'{stub_name}.ls-remote-hang-child-pid' + return int(pid_file.read_text(encoding='utf-8').strip()) + def set_current_sha(self, sha: str) -> None: # pragma: win32 no cover """Configure the canned sha for the hook checkout's own `HEAD`. @@ -228,6 +249,7 @@ def _sandbox_path_dir(base: Path) -> Path: # pragma: win32 no cover 'head', 'mkdir', 'mktemp', + 'pgrep', 'rm', 'sed', 'sleep', @@ -957,6 +979,40 @@ def test_network_query_bounded_by_watchdog( # pragma: win32 no cover assert hook_run.returncode != 0, combined +def test_watchdog_kills_remote_helper_child_too( # pragma: win32 no cover + tmp_repo: Path, + cache_dir: Path, + tmp_path: Path, +) -> None: + """Check the watchdog kills git's remote-helper child, not just git. + + `git ls-remote https://...` spawns a separate remote-helper process + (`git remote-https`, confirmed against a real invocation) to do the + actual network I/O. Killing only the parent PID lets that helper + survive and keep running after the hook returns - the dispatcher + stub forks its own child on a hang to mimic this exact shape. + """ + dispatcher = _GitDispatcherStub(tmp_path) + dispatcher.set_ls_remote_hang() + + sandbox_path_dir = _sandbox_path_dir(tmp_path) + path_with_dispatcher = f'{dispatcher.path_entry}:{sandbox_path_dir}' + + hook_run = _run_hook( + 'terraform_fmt.sh', + [], + cwd=tmp_repo, + env=_hook_env(_pct_cache_env(cache_dir), path_with_dispatcher), + ) + + assert TIMEOUT_MSG in hook_run.stdout, hook_run.stdout + + helper_pid = dispatcher.hung_helper_pid() + time.sleep(_PROCESS_REAP_GRACE_SECONDS) + with pytest.raises(ProcessLookupError): + os.kill(helper_pid, 0) + + def test_fresh_cache_still_nags_when_outdated( # pragma: win32 no cover tmp_repo: Path, cache_dir: Path, From 7abb04b117bac5127e14de06a6b4f3c55d4ea204 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 02:14:37 +0300 Subject: [PATCH 11/16] Specify minimum required git version --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 44df7b78e..8f6e6d178 100644 --- a/README.md +++ b/README.md @@ -229,7 +229,7 @@ Full list of dependencies and where they are used: * [`pre-commit`](https://pre-commit.com/#install), [`terraform`](https://www.terraform.io/downloads.html) or [`opentofu`](https://opentofu.org/docs/intro/install/), - [`git`](https://git-scm.com/downloads), + [`git`](https://git-scm.com/downloads) 2.18+, [BASH `3.2.57` or newer](https://www.gnu.org/software/bash/#download), Internet connection (on first run), x86_64 or arm64 compatible operating system, From 0f0cb4a2b2d5c93b577d3eaaa105c33995796fe5 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 02:28:45 +0300 Subject: [PATCH 12/16] fix README --- README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 8f6e6d178..ef802a361 100644 --- a/README.md +++ b/README.md @@ -447,6 +447,11 @@ To skip the check set one of: * `CI=true` (most CI systems already export this automatically). * `PCT_SKIP_UPDATE_CHECK=true` to disable it everywhere, including locally. + ```bash + # Skip the check for this run (or export it in CI) + PCT_SKIP_UPDATE_CHECK=true pre-commit run -a + ``` + How it works: 1. The check only runs when a hook is about to fail for its own reasons - a clean run stays completely silent, no matter how outdated your pin is. @@ -456,11 +461,6 @@ How it works: 5. The check never fails or meaningfully slows down your commit: the remote query is capped at 3 seconds, and if it can't reach GitHub (offline, firewalled CI runner, etc.) it prints a short notice and moves on - the hook's own exit code is unaffected either way. 6. The last-checked timestamp and the upstream tag list from that check are cached as two files, `.last_update_check_time` and `.last_update_check_tags`, under the same cache root used for [pinned tool versions](#most-hooks-pin-a-specific-tool-version) (`PCT_TOOL_CACHE_DIR`, or `$XDG_CACHE_HOME`/`$HOME/.cache` + `pre-commit-terraform`) - see [Mount tools cache directory](#mount-tools-cache-directory) if you also want this to persist across Docker runs. -```bash -# Skip the check for this run (or export it in CI) -PCT_SKIP_UPDATE_CHECK=true pre-commit run -a -``` - ### Most hooks: Pin a specific tool version > All hooks, which wrap a tool distributed as a downloadable release asset. Not supported for `checkov`/`terraform_checkov` (distributed via PyPi) and for deprecated `terraform_docs_replace` hook. From 7cbf2828ba03cb5779cd94bfb5591bc6a95e4bc1 Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 02:44:38 +0300 Subject: [PATCH 13/16] test: trim unused sandbox tool allowlist entries update_notification_test.py's _sandbox_path_dir copied tool_version_test.py's full tool lists, but this file only drives terraform_fmt.sh/terraform_wrapper_module_for_each.sh as far as _check_new_version_on_failure.sh - the --tool-version download path and common::is_hook_run_on_whole_repo are never reached here. Drop entries neither this file's tests nor _check_new_version_on_failure.sh actually invoke. Verified via full pytest run (100% coverage) that nothing pruned is still needed. Assisted-by: Sisyphus:claude-sonnet-5 opencode --- tests/pytest/update_notification_test.py | 54 +++++++----------------- 1 file changed, 15 insertions(+), 39 deletions(-) diff --git a/tests/pytest/update_notification_test.py b/tests/pytest/update_notification_test.py index dc04f9ea1..3e5d9b288 100644 --- a/tests/pytest/update_notification_test.py +++ b/tests/pytest/update_notification_test.py @@ -231,63 +231,39 @@ def _create_terraform_stub( # pragma: win32 no cover def _sandbox_path_dir(base: Path) -> Path: # pragma: win32 no cover """Build a `PATH` dir with coreutils but no wrapped CLI tool. - Reuses the same list of required/optional tools as `tool_version_test.py`. + Narrower than `tool_version_test.py`'s own list: this file only ever + drives `terraform_fmt.sh`/`terraform_wrapper_module_for_each.sh` far + enough to hit `_check_new_version_on_failure.sh`, never the + `--tool-version` download path or `common::is_hook_run_on_whole_repo` + (neither hook defines `run_hook_on_whole_repo`), so this list omits + tools those unreached paths would need. Returns: Path to the constructed directory, usable as a `PATH` entry. """ - # Same tool lists as tool_version_test.py sandbox_required_tools = ( - 'awk', - 'basename', + 'awk', # _check_new_version_on_failure.sh: tag-pair parsing 'bash', - 'cat', - 'cut', + 'cat', # common::get_cpu_num cgroup-v1 fallback + 'cut', # common::get_cpu_num cgroup-v2 fallback 'dirname', - 'env', - 'grep', - 'head', 'mkdir', 'mktemp', - 'pgrep', + 'pgrep', # _pct_kill_process_tree 'rm', 'sed', 'sleep', - 'sort', + 'sort', # common::per_dir_hook: dir_paths_unique 'tail', - 'tr', - 'uname', - 'wc', 'git', ) sandbox_optional_tools = ( - 'chmod', - 'cp', - 'curl', - 'date', - 'find', - 'getopt', - 'id', - 'ln', - 'ls', - 'mv', + 'date', # _check_new_version_on_failure.sh cache timestamps + # common::get_cpu_num's cgroup-less host fallback - either or + # both may be missing depending on the platform, hence + # optional rather than required. 'nproc', - 'printf', - 'readlink', - 'realpath', - 'seq', - 'stat', 'sysctl', - 'tar', - 'tee', - # Not on stock macOS (needs GNU coreutils) - the hook itself - # already tolerates its absence (`command -v timeout` guard), - # so the sandbox must too, not hard-require it. - 'timeout', - 'touch', - 'uniq', - 'unzip', - 'xargs', ) path_dir = base / 'sandbox-path' From cc586e659d45cb7b10667b237887a623f6df8f5d Mon Sep 17 00:00:00 2001 From: Maksym Vlasov Date: Sat, 12 Sep 2026 02:46:03 +0300 Subject: [PATCH 14/16] Apply suggestion from @MaxymVlasov --- tests/pytest/tool_version_test.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/pytest/tool_version_test.py b/tests/pytest/tool_version_test.py index de06523d0..58f0d1d8e 100644 --- a/tests/pytest/tool_version_test.py +++ b/tests/pytest/tool_version_test.py @@ -177,9 +177,6 @@ class _HookWiring(NamedTuple): 'sysctl', 'tar', 'tee', - # Not on stock macOS (needs GNU coreutils); these tests always set - # `PCT_SKIP_UPDATE_CHECK=true` so they never actually invoke it. - 'timeout', 'touch', 'uniq', 'unzip', From f80864ee7e6e87499ae7aa1fa9034729667924ac Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 02:51:01 +0300 Subject: [PATCH 15/16] Try to dial wit constant issues with GH workflow --- .github/workflows/pre-commit.yaml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/pre-commit.yaml b/.github/workflows/pre-commit.yaml index cf556c29b..df08f7120 100644 --- a/.github/workflows/pre-commit.yaml +++ b/.github/workflows/pre-commit.yaml @@ -31,13 +31,14 @@ jobs: echo "files=$( echo "$DIFF" | xargs echo )" >> $GITHUB_OUTPUT - name: Install shfmt + env: + # renovate: datasource=github-releases depName=shfmt lookupName=mvdan/sh + SHFMT_VERSION: 3.14.0 run: >- - curl -L "$( - curl -s https://api.github.com/repos/mvdan/sh/releases/latest - | grep -o -E -m 1 "https://.+?linux_amd64" - )" + curl -L + https://github.com/mvdan/sh/releases/download/v${SHFMT_VERSION}/shfmt_v${SHFMT_VERSION}_linux_amd64 > shfmt - && chmod +x shfmt && sudo mv shfmt /usr/bin/ + && chmod +x shfmt && mv shfmt /usr/bin/ - name: Install shellcheck run: | From 5fcb4537b94a0dbce46c3e9abf7be315c3ac699a Mon Sep 17 00:00:00 2001 From: MaxymVlasov Date: Sat, 12 Sep 2026 02:52:38 +0300 Subject: [PATCH 16/16] f --- .github/workflows/pre-commit.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pre-commit.yaml b/.github/workflows/pre-commit.yaml index df08f7120..c628e2a46 100644 --- a/.github/workflows/pre-commit.yaml +++ b/.github/workflows/pre-commit.yaml @@ -38,7 +38,7 @@ jobs: curl -L https://github.com/mvdan/sh/releases/download/v${SHFMT_VERSION}/shfmt_v${SHFMT_VERSION}_linux_amd64 > shfmt - && chmod +x shfmt && mv shfmt /usr/bin/ + && chmod +x shfmt && sudo mv shfmt /usr/bin/ - name: Install shellcheck run: |