From bc0745feb1b3e751f8ebef6d2d1c4eb53832ec8e Mon Sep 17 00:00:00 2001 From: Amar K P Date: Wed, 23 Sep 2026 07:41:58 +0530 Subject: [PATCH 1/4] feat: add crash-safe prepare-commit-msg hooks --- .pre-commit-hooks.yaml | 7 ++ README.md | 44 +++++++- plan.md | 9 +- pyproject.toml | 1 + src/gitmate/cli.py | 28 +++++ src/gitmate/fallback.py | 2 + src/gitmate/hooks.py | 163 +++++++++++++++++++++++++++ tests/test_hooks.py | 240 ++++++++++++++++++++++++++++++++++++++++ 8 files changed, 488 insertions(+), 6 deletions(-) create mode 100644 .pre-commit-hooks.yaml create mode 100644 src/gitmate/hooks.py create mode 100644 tests/test_hooks.py diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml new file mode 100644 index 0000000..004b7ca --- /dev/null +++ b/.pre-commit-hooks.yaml @@ -0,0 +1,7 @@ +- id: gitmate-prepare-commit-msg + name: Generate a commit message with gitmate + description: Safely generate a commit message before the editor opens. + entry: gitmate-hook + language: python + stages: [prepare-commit-msg] + pass_filenames: true diff --git a/README.md b/README.md index 14b646a..02d6555 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ uv sync --extra dev uv run pytest ``` -## Usage (Phases 0–4) +## Usage ```bash uv run gitmate --help @@ -37,6 +37,12 @@ uv run gitmate config set cache_dir /path/to/cache # custom prompt cache d uv run gitmate debug-diff # staged diff: per-file table + cleaned patch uv run gitmate debug-diff --summary # per-file stats table only, no patch text uv run gitmate debug-diff --base main # branch-vs-main diff (PR path) +uv run gitmate commit # review, edit, accept, or cancel a message +uv run gitmate pr-summary --base main +uv run gitmate changelog --from v1.0.0 --to v1.1.0 +uv run gitmate stats +uv run gitmate install-hook # optional local prepare-commit-msg hook +uv run gitmate uninstall-hook # remove the local hook ``` Phase 4 introduces versioned prompt templates (`src/gitmate/templates/`: `commit_conventional.txt`, @@ -44,8 +50,40 @@ Phase 4 introduces versioned prompt templates (`src/gitmate/templates/`: `commit (`fallback.py`). When the LLM backend is offline, rate-limited, unauthenticated, or fails retries, `generate_commit_message` warns the user (`⚠ API unavailable, using template fallback`) and derives a clean commit message directly from diff metadata without making an LLM call. -`commit`, `pr-summary`, `changelog`, and `doc` remain stubbed until their phases -land — see `AGENTS.md` and the phased roadmap for details. +`doc` is an unscheduled stub; commit, PR summary, changelog, stats, and hook +commands are implemented. + +### Automatic commit message generation + +For a personal checkout, run `gitmate install-hook`. It installs a local +`prepare-commit-msg` hook; Git opens the usual editor with the generated message +so you can review or change it before saving. The hook never runs `git commit` +and has a four-second generation timeout. If generation fails or times out, it +leaves Git's existing message in place and exits successfully so the commit is +not blocked. It only generates when the source is `template` or absent; explicit +`git commit -m`/`-F` messages (`message`), merge messages (`merge`), squash +messages (`squash`), and other source values are left alone. Existing hooks are +refused rather than overwritten; `gitmate uninstall-hook` removes only the +gitmate-managed hook. Both commands work with Git's configured `core.hooksPath`. + +For teams using the `pre-commit` framework, add this to `.pre-commit-config.yaml` +and run `pre-commit install --hook-type prepare-commit-msg`: + +```yaml +repos: + - repo: https://github.com/getdownandcode/gitmate + rev: + hooks: + - id: gitmate-prepare-commit-msg +``` + +The repository's `.pre-commit-hooks.yaml` declares the `prepare-commit-msg` +stage and `language: python`, so pre-commit installs gitmate into its isolated +environment for each adopting checkout. `keyring` still uses the user's OS +credential store from that environment; the API key is not copied into the +repository or the pre-commit environment. The `gitmate install-hook` route is +local-only because `.git/hooks` is not version controlled; the pre-commit route +lets a team version the hook declaration for everyone who opts in. ## Diagrams diff --git a/plan.md b/plan.md index 2207065..54a6a14 100644 --- a/plan.md +++ b/plan.md @@ -142,10 +142,13 @@ the loop. Goal: something you actually run every day, not just a portfolio repo. **Goal:** stop having to remember to run the tool at all. -- Ship a `prepare-commit-msg` git hook script that calls `gitmate commit --hook-mode` (non-interactive generate, writes to the commit message file git already opens in your editor — so you review inside your normal `git commit` flow, no extra step). -- Also provide a `.pre-commit-hooks.yaml` so people using the `pre-commit` framework can add it with one config line. +- Ship an optional local `prepare-commit-msg` hook. It calls only the generation pipeline, writes the message for Git's normal editor, and never runs `git commit` or the interactive accept/edit/regenerate loop. +- Fail open: the launcher catches failures and exits 0; the generation worker has a four-second timeout and uses Phase 4 fallback when available. Message writes are atomic so a failed write leaves Git's original message intact. +- Generate only when the source is `template` or absent. Leave `message` (explicit `-m`/`-F`), `merge`, `squash`, and all other source values untouched. +- Refuse to overwrite an existing hook, support idempotent install and managed-hook uninstall, and honor Git's configured hooks path. +- Also provide `.pre-commit-hooks.yaml` with `language: python` and `stages: [prepare-commit-msg]` for team adoption. The local `.git/hooks` installer is for one developer; the versioned pre-commit declaration is for teams. Its isolated environment still uses the user's OS credential store through keyring. -**Deliverables:** `gitmate install-hook` command that symlinks/writes the hook into `.git/hooks/`, documented `pre-commit` config snippet. +**Deliverables:** `gitmate install-hook` and `gitmate uninstall-hook`, crash/timeout safe real-repository tests, and a documented pre-commit config snippet. **Tech:** `pre-commit` framework config format, plain shell/Python hook script. diff --git a/pyproject.toml b/pyproject.toml index 5efd28d..356f3d8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -32,6 +32,7 @@ dev = [ [project.scripts] gitmate = "gitmate.cli:app" +gitmate-hook = "gitmate.hooks:main" [tool.ruff] line-length = 100 diff --git a/src/gitmate/cli.py b/src/gitmate/cli.py index 038e8b4..e44aec9 100644 --- a/src/gitmate/cli.py +++ b/src/gitmate/cli.py @@ -42,6 +42,34 @@ def commit( raise typer.Exit(code) +@app.command("install-hook") +def install_hook() -> None: + """Install gitmate's local prepare-commit-msg hook in this repository.""" + from gitmate.hooks import HookError + from gitmate.hooks import install_hook as install + + try: + path = install() + except HookError as exc: + err_console.print(f"[red]error:[/red] {exc}") + raise typer.Exit(1) from None + console.print(f"[green]Installed prepare-commit-msg hook:[/green] {path}") + + +@app.command("uninstall-hook") +def uninstall_hook() -> None: + """Remove gitmate's local prepare-commit-msg hook.""" + from gitmate.hooks import HookError + from gitmate.hooks import uninstall_hook as uninstall + + try: + path = uninstall() + except HookError as exc: + err_console.print(f"[red]error:[/red] {exc}") + raise typer.Exit(1) from None + console.print(f"[green]Removed gitmate prepare-commit-msg hook:[/green] {path}") + + @app.command() def stats( days: int | None = typer.Option(None, "--days", "-d", help="Limit stats to the last N days."), diff --git a/src/gitmate/fallback.py b/src/gitmate/fallback.py index 374ef42..9be7b96 100644 --- a/src/gitmate/fallback.py +++ b/src/gitmate/fallback.py @@ -262,6 +262,7 @@ def generate_commit_message( console: Console | None = None, secret_store: config_mod.SecretStore | None = None, bypass_cache: bool = False, + command: str | None = None, ) -> GenerationResult: """Orchestrate commit message generation with graceful fallback on provider failure.""" return run_generation_pipeline( @@ -270,6 +271,7 @@ def generate_commit_message( template_name=cfg.commit_style, fallback_generator=lambda: generate_fallback_message(diffs, style=cfg.commit_style), empty_diff_message="No changes staged for commit.", + command=command, provider=provider, counter=counter, console=console, diff --git a/src/gitmate/hooks.py b/src/gitmate/hooks.py new file mode 100644 index 0000000..3d0bfd7 --- /dev/null +++ b/src/gitmate/hooks.py @@ -0,0 +1,163 @@ +"""Crash-safe prepare-commit-msg hook installation and execution.""" + +from __future__ import annotations + +import os +import stat +import subprocess +import sys +import tempfile +from pathlib import Path + +HOOK_TIMEOUT_SECONDS = 4 +HOOK_MARKER = "# Installed by gitmate; managed by `gitmate install-hook`." + +HOOK_SCRIPT = f'''#!{sys.executable} +# Installed by gitmate; managed by `gitmate install-hook`. +"""Crash-safe launcher for gitmate's prepare-commit-msg worker.""" +import subprocess +import sys + +try: + subprocess.run( + [sys.executable, "-m", "gitmate.hooks", "--worker", *sys.argv[1:]], + timeout={HOOK_TIMEOUT_SECONDS}, + check=False, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) +except BaseException: + pass +raise SystemExit(0) +''' + + +class HookError(Exception): + """Raised when a hook cannot be safely installed or removed.""" + + +def _hook_path(cwd: Path | None = None) -> Path: + """Resolve the repository's active prepare-commit-msg hook path.""" + repo = cwd if cwd is not None else Path.cwd() + try: + result = subprocess.run( + ["git", "rev-parse", "--git-path", "hooks/prepare-commit-msg"], + cwd=repo, + capture_output=True, + text=True, + timeout=2, + check=True, + ) + except (OSError, subprocess.SubprocessError) as exc: + raise HookError(f"cannot locate the prepare-commit-msg hook: {exc}") from exc + path = Path(result.stdout.strip()) + return path if path.is_absolute() else (repo / path).resolve() + + +def install_hook(cwd: Path | None = None) -> Path: + """Install the crash-safe hook, refusing to replace user-owned hooks.""" + path = _hook_path(cwd) + path.parent.mkdir(parents=True, exist_ok=True) + if path.exists(): + content = path.read_text(encoding="utf-8", errors="replace") + if HOOK_MARKER in content: + path.chmod(path.stat().st_mode | 0o111) + return path + raise HookError(f"{path} already exists; move or remove it before installing gitmate.") + path.write_text(HOOK_SCRIPT, encoding="utf-8") + path.chmod(path.stat().st_mode | 0o111) + return path + + +def uninstall_hook(cwd: Path | None = None) -> Path: + """Remove the gitmate hook while preserving hooks owned by other tools.""" + path = _hook_path(cwd) + if not path.exists(): + return path + content = path.read_text(encoding="utf-8", errors="replace") + if HOOK_MARKER not in content: + raise HookError(f"{path} is not a gitmate-managed hook; it was left untouched.") + path.unlink() + return path + + +def main() -> int: + """Run the hook worker with a hard timeout and unconditional success status.""" + args = sys.argv[1:] + if args and args[0] == "--worker": + try: + _run_worker(args[1:]) + except BaseException: # noqa: BLE001, S110 + pass + return 0 + + if args: + source = os.environ.get("PRE_COMMIT_COMMIT_MSG_SOURCE") + commit_hash = os.environ.get("PRE_COMMIT_COMMIT_OBJECT_NAME") + if len(args) == 1 and source: + args.append(source) + if len(args) == 2 and commit_hash: + args.append(commit_hash) + + try: + subprocess.run( + [sys.executable, "-m", "gitmate.hooks", "--worker", *args], + timeout=HOOK_TIMEOUT_SECONDS, + check=False, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + except BaseException: # noqa: BLE001, S110 + pass + return 0 + + +def _run_worker(args: list[str]) -> None: + """Generate a message for supported Git hook sources without committing.""" + if not args or len(args) > 3: + return + message_file = Path(args[0]) + source = args[1] if len(args) > 1 else "" + if source not in ("", "template") or not message_file.is_file(): + return + + from gitmate import config as config_mod + from gitmate.diff_extractor import DiffExtractor + from gitmate.fallback import generate_commit_message + + cfg = config_mod.load_config() + diffs = DiffExtractor(extra_ignores=cfg.ignore_globs).staged() + if not diffs: + return + result = generate_commit_message( + diffs=diffs, + cfg=cfg, + console=None, + command="commit-hook", + ) + _replace_message(message_file, result.text.rstrip() + "\n") + + +def _replace_message(message_file: Path, message: str) -> None: + """Atomically replace Git's message file so write failures preserve its contents.""" + mode = stat.S_IMODE(message_file.stat().st_mode) + temp_path: Path | None = None + try: + with tempfile.NamedTemporaryFile( + mode="w", + encoding="utf-8", + dir=message_file.parent, + prefix=f".{message_file.name}.", + delete=False, + ) as temp_file: + temp_path = Path(temp_file.name) + temp_file.write(message) + temp_path.chmod(mode) + os.replace(temp_path, message_file) + finally: + if temp_path is not None and temp_path.exists(): + temp_path.unlink() + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_hooks.py b/tests/test_hooks.py new file mode 100644 index 0000000..4334fe6 --- /dev/null +++ b/tests/test_hooks.py @@ -0,0 +1,240 @@ +"""Real-repository tests for crash-safe prepare-commit-msg integration.""" + +from __future__ import annotations + +import os +import subprocess +import sys +import time +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from gitmate import hooks as hooks_mod +from gitmate.cli import app +from gitmate.hooks import HOOK_MARKER, HookError, install_hook, uninstall_hook +from gitmate.metrics import get_invocations + + +def _git(repo: Path, *args: str, check: bool = True) -> subprocess.CompletedProcess[str]: + """Run git in a scratch repository.""" + return subprocess.run( + ["git", *args], + cwd=repo, + text=True, + capture_output=True, + check=check, + ) + + +def _init_repo(repo: Path) -> None: + """Create a repository configured for editor-free integration tests.""" + repo.mkdir() + _git(repo, "init", "-b", "main") + _git(repo, "config", "user.name", "Hook Test") + _git(repo, "config", "user.email", "hook@example.test") + _git(repo, "commit", "--allow-empty", "-m", "initial") + + +def _commit(repo: Path, *args: str, env: dict[str, str]) -> subprocess.CompletedProcess[str]: + """Run a commit with an inert editor and isolated process environment.""" + return subprocess.run( + ["git", "commit", *args], + cwd=repo, + text=True, + capture_output=True, + env=env, + check=False, + ) + + +@pytest.fixture +def hook_env(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> dict[str, str]: + """Isolate config, metrics, and cache for hook subprocesses.""" + env = os.environ.copy() + env.update( + { + "GITMATE_CONFIG_DIR": str(tmp_path / "config"), + "GITMATE_METRICS_DIR": str(tmp_path / "metrics"), + "GIT_EDITOR": "true", + } + ) + env.pop("GEMINI_API_KEY", None) + monkeypatch.setenv("GITMATE_CONFIG_DIR", env["GITMATE_CONFIG_DIR"]) + monkeypatch.setenv("GITMATE_METRICS_DIR", env["GITMATE_METRICS_DIR"]) + return env + + +def test_install_is_executable_idempotent_and_uninstallable(tmp_path: Path) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + + path = install_hook(repo) + original = path.read_text(encoding="utf-8") + assert path.stat().st_mode & 0o111 + assert original.startswith(f"#!{sys.executable}\n") + assert HOOK_MARKER in original + + assert install_hook(repo) == path + assert path.read_text(encoding="utf-8") == original + assert uninstall_hook(repo) == path + assert not path.exists() + + +def test_install_refuses_to_overwrite_existing_hook(tmp_path: Path) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + path = repo / ".git" / "hooks" / "prepare-commit-msg" + path.write_text("#!/bin/sh\necho user-hook\n", encoding="utf-8") + + with pytest.raises(HookError, match="already exists"): + install_hook(repo) + assert path.read_text(encoding="utf-8") == "#!/bin/sh\necho user-hook\n" + + +def test_uninstall_refuses_to_remove_user_hook(tmp_path: Path) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + path = repo / ".git" / "hooks" / "prepare-commit-msg" + path.write_text("#!/bin/sh\necho user-hook\n", encoding="utf-8") + + with pytest.raises(HookError, match="not a gitmate-managed"): + uninstall_hook(repo) + assert path.exists() + + +def test_cli_installs_and_uninstalls_hook(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + monkeypatch.chdir(repo) + runner = CliRunner() + + installed = runner.invoke(app, ["install-hook"]) + hook_path = repo / ".git" / "hooks" / "prepare-commit-msg" + assert installed.exit_code == 0, installed.output + assert hook_path.exists() + + removed = runner.invoke(app, ["uninstall-hook"]) + assert removed.exit_code == 0, removed.output + assert not hook_path.exists() + + +def test_pre_commit_source_environment_is_forwarded( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + message_file = tmp_path / "COMMIT_EDITMSG" + message_file.write_text("User message\n", encoding="utf-8") + commands: list[list[str]] = [] + + def fake_run(command: list[str], **kwargs: object) -> None: + commands.append(command) + + monkeypatch.setattr(sys, "argv", ["gitmate-hook", str(message_file)]) + monkeypatch.setenv("PRE_COMMIT_COMMIT_MSG_SOURCE", "message") + monkeypatch.setenv("PRE_COMMIT_COMMIT_OBJECT_NAME", "deadbeef") + monkeypatch.setattr(hooks_mod.subprocess, "run", fake_run) + + assert hooks_mod.main() == 0 + assert commands[0][-3:] == [str(message_file), "message", "deadbeef"] + + +def test_real_commit_gets_fallback_message_and_hook_metrics( + tmp_path: Path, hook_env: dict[str, str] +) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + install_hook(repo) + (repo / "feature.py").write_text("print('hello')\n", encoding="utf-8") + _git(repo, "add", "feature.py") + + result = _commit(repo, env=hook_env) + assert result.returncode == 0, result.stderr + subject = _git(repo, "log", "-1", "--format=%s").stdout.strip() + assert subject == "feat: add feature.py" + rows = get_invocations() + assert len(rows) == 1 + assert rows[0].command == "commit-hook" + assert rows[0].fallback_used + + +def test_message_source_is_untouched(tmp_path: Path, hook_env: dict[str, str]) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + install_hook(repo) + (repo / "feature.py").write_text("print('hello')\n", encoding="utf-8") + _git(repo, "add", "feature.py") + + result = _commit(repo, "-m", "user supplied message", env=hook_env) + assert result.returncode == 0, result.stderr + assert _git(repo, "log", "-1", "--format=%s").stdout.strip() == "user supplied message" + assert get_invocations() == [] + + +def test_real_merge_message_is_untouched(tmp_path: Path, hook_env: dict[str, str]) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + install_hook(repo) + _git(repo, "checkout", "-b", "side") + (repo / "side.txt").write_text("side\n", encoding="utf-8") + _git(repo, "add", "side.txt") + _git(repo, "commit", "-m", "side change") + _git(repo, "checkout", "main") + (repo / "main.txt").write_text("main\n", encoding="utf-8") + _git(repo, "add", "main.txt") + _git(repo, "commit", "-m", "main change") + + result = subprocess.run( + ["git", "merge", "--no-ff", "side"], + cwd=repo, + text=True, + capture_output=True, + env=hook_env, + check=False, + ) + assert result.returncode == 0, result.stderr + subject = _git(repo, "log", "-1", "--format=%s").stdout.strip() + assert subject.startswith("Merge branch 'side'") + assert get_invocations() == [] + + +def _install_fake_worker(tmp_path: Path, mode: str, env: dict[str, str]) -> None: + """Shadow only the worker module to simulate a crash or a hung provider.""" + package = tmp_path / "shadow" / "gitmate" + package.mkdir(parents=True) + (package / "__init__.py").write_text("", encoding="utf-8") + (package / "hooks.py").write_text( + "import os, time\n" + "mode = os.environ['GITMATE_TEST_WORKER']\n" + "if mode == 'crash': raise RuntimeError('simulated worker crash')\n" + "time.sleep(30)\n", + encoding="utf-8", + ) + env["PYTHONPATH"] = str(tmp_path / "shadow") + env["GITMATE_TEST_WORKER"] = mode + + +@pytest.mark.parametrize("mode", ["crash", "slow"]) +def test_worker_failure_and_timeout_do_not_block_real_commit( + tmp_path: Path, hook_env: dict[str, str], mode: str +) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + install_hook(repo) + template = tmp_path / "commit-template.txt" + template.write_text("Use the repository template\n", encoding="utf-8") + _git(repo, "config", "commit.template", str(template)) + editor = tmp_path / "save-message.sh" + editor.write_text("#!/bin/sh\nprintf '\\nReviewed by test.\\n' >> \"$1\"\n", encoding="utf-8") + editor.chmod(0o755) + hook_env["GIT_EDITOR"] = str(editor) + (repo / "file.txt").write_text("change\n", encoding="utf-8") + _git(repo, "add", "file.txt") + _install_fake_worker(tmp_path, mode, hook_env) + + started = time.monotonic() + result = _commit(repo, env=hook_env) + elapsed = time.monotonic() - started + assert result.returncode == 0, result.stderr + assert _git(repo, "log", "-1", "--format=%s").stdout.strip() == "Use the repository template" + assert elapsed < 8 From 3cbbf5c8704072f4eed04c8ed0b23dcddb1c1f85 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Wed, 23 Sep 2026 08:09:02 +0530 Subject: [PATCH 2/4] fix(hooks): resolve CI mypy error, wrap hook in posix shell, and preserve git status comments --- src/gitmate/cli.py | 17 +++++++++++++++ src/gitmate/hooks.py | 32 ++++++++++++++++++++-------- tests/test_hooks.py | 50 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 88 insertions(+), 11 deletions(-) diff --git a/src/gitmate/cli.py b/src/gitmate/cli.py index e44aec9..381fdd9 100644 --- a/src/gitmate/cli.py +++ b/src/gitmate/cli.py @@ -33,8 +33,25 @@ def _stub(name: str, milestone: str) -> None: @app.command() def commit( yes: bool = typer.Option(False, "--yes", "-y", help="Commit without interactive confirmation."), + hook_mode: bool = typer.Option( + False, + "--hook-mode", + help="Non-interactive hook mode (writes directly to commit message file).", + ), + commit_msg_file: Path | None = typer.Argument( # noqa: B008 + None, help="Path to the commit message file in hook mode." + ), ) -> None: """Generate a commit message for the staged diff and review before committing.""" + if hook_mode: + if commit_msg_file is None: + err_console.print("[red]error:[/red] --hook-mode requires a commit message file path.") + raise typer.Exit(1) + from gitmate.hooks import _run_worker + + _run_worker([str(commit_msg_file)]) + return + from gitmate.committer import commit_flow code = commit_flow(yes=yes, console=console, secret_store=secret_store) diff --git a/src/gitmate/hooks.py b/src/gitmate/hooks.py index 3d0bfd7..42636b8 100644 --- a/src/gitmate/hooks.py +++ b/src/gitmate/hooks.py @@ -12,12 +12,19 @@ HOOK_TIMEOUT_SECONDS = 4 HOOK_MARKER = "# Installed by gitmate; managed by `gitmate install-hook`." -HOOK_SCRIPT = f'''#!{sys.executable} -# Installed by gitmate; managed by `gitmate install-hook`. -"""Crash-safe launcher for gitmate's prepare-commit-msg worker.""" -import subprocess -import sys - +HOOK_SCRIPT = f"""#!/bin/sh +{HOOK_MARKER} +# Crash-safe launcher for gitmate's prepare-commit-msg worker. +PYTHON="{sys.executable}" +if [ ! -x "$PYTHON" ]; then + if command -v python3 >/dev/null 2>&1; then + PYTHON="python3" + else + exit 0 + fi +fi +exec "$PYTHON" -c ' +import subprocess, sys try: subprocess.run( [sys.executable, "-m", "gitmate.hooks", "--worker", *sys.argv[1:]], @@ -28,8 +35,9 @@ ) except BaseException: pass -raise SystemExit(0) -''' +sys.exit(0) +' "$@" +""" class HookError(Exception): @@ -135,7 +143,13 @@ def _run_worker(args: list[str]) -> None: console=None, command="commit-hook", ) - _replace_message(message_file, result.text.rstrip() + "\n") + existing = message_file.read_text(encoding="utf-8", errors="replace") + generated = result.text.rstrip() + if existing.strip(): + new_message = f"{generated}\n\n{existing.lstrip()}" + else: + new_message = f"{generated}\n" + _replace_message(message_file, new_message) def _replace_message(message_file: Path, message: str) -> None: diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 4334fe6..ff61e76 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -73,7 +73,8 @@ def test_install_is_executable_idempotent_and_uninstallable(tmp_path: Path) -> N path = install_hook(repo) original = path.read_text(encoding="utf-8") assert path.stat().st_mode & 0o111 - assert original.startswith(f"#!{sys.executable}\n") + assert original.startswith("#!/bin/sh\n") + assert sys.executable in original assert HOOK_MARKER in original assert install_hook(repo) == path @@ -133,7 +134,7 @@ def fake_run(command: list[str], **kwargs: object) -> None: monkeypatch.setattr(sys, "argv", ["gitmate-hook", str(message_file)]) monkeypatch.setenv("PRE_COMMIT_COMMIT_MSG_SOURCE", "message") monkeypatch.setenv("PRE_COMMIT_COMMIT_OBJECT_NAME", "deadbeef") - monkeypatch.setattr(hooks_mod.subprocess, "run", fake_run) + monkeypatch.setattr("gitmate.hooks.subprocess.run", fake_run) assert hooks_mod.main() == 0 assert commands[0][-3:] == [str(message_file), "message", "deadbeef"] @@ -238,3 +239,48 @@ def test_worker_failure_and_timeout_do_not_block_real_commit( assert result.returncode == 0, result.stderr assert _git(repo, "log", "-1", "--format=%s").stdout.strip() == "Use the repository template" assert elapsed < 8 + + +def test_commit_preserves_template_and_prepends_message( + tmp_path: Path, hook_env: dict[str, str] +) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + install_hook(repo) + template = tmp_path / "commit-template.txt" + template.write_text("Ticket: JIRA-123\nReviewed-by: Lead\n", encoding="utf-8") + _git(repo, "config", "commit.template", str(template)) + + (repo / "auth.py").write_text("def auth(): pass\n", encoding="utf-8") + _git(repo, "add", "auth.py") + + result = _commit(repo, env=hook_env) + assert result.returncode == 0, result.stderr + full_message = _git(repo, "log", "-1", "--format=%B").stdout + assert full_message.startswith("feat: add auth.py") + assert "Ticket: JIRA-123" in full_message + assert "Reviewed-by: Lead" in full_message + + +def test_cli_commit_hook_mode(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + monkeypatch.chdir(repo) + + (repo / "new_module.py").write_text("val = 100\n", encoding="utf-8") + _git(repo, "add", "new_module.py") + + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("# Please enter the commit message\n# On branch main\n", encoding="utf-8") + + runner = CliRunner() + res = runner.invoke(app, ["commit", "--hook-mode", str(msg_file)]) + assert res.exit_code == 0, res.output + content = msg_file.read_text(encoding="utf-8") + assert content.startswith("feat: add new_module.py") + assert "# Please enter the commit message" in content + + # Missing file argument returns error + res_err = runner.invoke(app, ["commit", "--hook-mode"]) + assert res_err.exit_code == 1 + assert "error" in res_err.output.lower() From c72ba194c09269c56c46ffa84098a34b7ebb96ac Mon Sep 17 00:00:00 2001 From: Amar K P Date: Wed, 23 Sep 2026 23:20:44 +0530 Subject: [PATCH 3/4] fix: address PR #13 review feedback on hook retries, budget-cap fallback, and free-tier metrics - Persist free_tier as a real metrics column; stop inferring it from zero-cost rows at read time so free-tier spend can't be inflated - Hook mode falls back to the Phase 4 template on BudgetCapExceededError instead of going silent; covered by a real-git-commit test - Bound hook-mode Gemini calls to 2 attempts, zero backoff, 900ms HTTP caps (worst case ~2.7s of the 4s hook budget); interactive commands keep full retry resilience - Remove --hook-mode/commit_msg_file from the user-facing commit command; hooks run only via the gitmate-hook entry point - Overwrite gitmate-marked hooks on reinstall so upgraded interpreter paths are refreshed - Prune stale .COMMIT_EDITMSG.* orphans left by SIGKILLed workers - Fall back to gemini*/claude* context-window family defaults for unrecognized models --- src/gitmate/cli.py | 17 ----- src/gitmate/config.py | 5 ++ src/gitmate/fallback.py | 2 + src/gitmate/hooks.py | 48 ++++++++++---- src/gitmate/metrics.py | 23 ++++--- src/gitmate/orchestrator.py | 13 +++- src/gitmate/providers/gemini.py | 39 +++++++++++- src/gitmate/token_budget.py | 12 +++- tests/test_config.py | 8 +++ tests/test_hooks.py | 108 ++++++++++++++++++++++++++------ tests/test_metrics.py | 44 +++++++++++++ tests/test_providers.py | 35 ++++++++++- 12 files changed, 292 insertions(+), 62 deletions(-) diff --git a/src/gitmate/cli.py b/src/gitmate/cli.py index 381fdd9..e44aec9 100644 --- a/src/gitmate/cli.py +++ b/src/gitmate/cli.py @@ -33,25 +33,8 @@ def _stub(name: str, milestone: str) -> None: @app.command() def commit( yes: bool = typer.Option(False, "--yes", "-y", help="Commit without interactive confirmation."), - hook_mode: bool = typer.Option( - False, - "--hook-mode", - help="Non-interactive hook mode (writes directly to commit message file).", - ), - commit_msg_file: Path | None = typer.Argument( # noqa: B008 - None, help="Path to the commit message file in hook mode." - ), ) -> None: """Generate a commit message for the staged diff and review before committing.""" - if hook_mode: - if commit_msg_file is None: - err_console.print("[red]error:[/red] --hook-mode requires a commit message file path.") - raise typer.Exit(1) - from gitmate.hooks import _run_worker - - _run_worker([str(commit_msg_file)]) - return - from gitmate.committer import commit_flow code = commit_flow(yes=yes, console=console, secret_store=secret_store) diff --git a/src/gitmate/config.py b/src/gitmate/config.py index b44fbe0..5120615 100644 --- a/src/gitmate/config.py +++ b/src/gitmate/config.py @@ -132,6 +132,11 @@ def get_context_window(cfg: GitmateConfig) -> int: return cfg.max_context_tokens if cfg.model in DEFAULT_MODEL_CONTEXT_WINDOWS: return DEFAULT_MODEL_CONTEXT_WINDOWS[cfg.model] + model_name = cfg.model.lower() + if model_name.startswith("gemini"): + return 1_048_576 + if model_name.startswith("claude"): + return 200_000 raise UnknownModelError( f"unknown model {cfg.model!r} with no context window configured. " "Set 'max_context_tokens' in config.toml or use a known model." diff --git a/src/gitmate/fallback.py b/src/gitmate/fallback.py index 9be7b96..28a855f 100644 --- a/src/gitmate/fallback.py +++ b/src/gitmate/fallback.py @@ -263,6 +263,7 @@ def generate_commit_message( secret_store: config_mod.SecretStore | None = None, bypass_cache: bool = False, command: str | None = None, + hook_mode: bool = False, ) -> GenerationResult: """Orchestrate commit message generation with graceful fallback on provider failure.""" return run_generation_pipeline( @@ -272,6 +273,7 @@ def generate_commit_message( fallback_generator=lambda: generate_fallback_message(diffs, style=cfg.commit_style), empty_diff_message="No changes staged for commit.", command=command, + hook_mode=hook_mode, provider=provider, counter=counter, console=console, diff --git a/src/gitmate/hooks.py b/src/gitmate/hooks.py index 42636b8..9b5979e 100644 --- a/src/gitmate/hooks.py +++ b/src/gitmate/hooks.py @@ -7,6 +7,7 @@ import subprocess import sys import tempfile +import time from pathlib import Path HOOK_TIMEOUT_SECONDS = 4 @@ -68,10 +69,8 @@ def install_hook(cwd: Path | None = None) -> Path: path.parent.mkdir(parents=True, exist_ok=True) if path.exists(): content = path.read_text(encoding="utf-8", errors="replace") - if HOOK_MARKER in content: - path.chmod(path.stat().st_mode | 0o111) - return path - raise HookError(f"{path} already exists; move or remove it before installing gitmate.") + if HOOK_MARKER not in content: + raise HookError(f"{path} already exists; move or remove it before installing gitmate.") path.write_text(HOOK_SCRIPT, encoding="utf-8") path.chmod(path.stat().st_mode | 0o111) return path @@ -131,20 +130,37 @@ def _run_worker(args: list[str]) -> None: from gitmate import config as config_mod from gitmate.diff_extractor import DiffExtractor - from gitmate.fallback import generate_commit_message + from gitmate.fallback import generate_commit_message, generate_fallback_message + from gitmate.orchestrator import BudgetCapExceededError, safe_record_telemetry cfg = config_mod.load_config() diffs = DiffExtractor(extra_ignores=cfg.ignore_globs).staged() if not diffs: return - result = generate_commit_message( - diffs=diffs, - cfg=cfg, - console=None, - command="commit-hook", - ) + started = time.perf_counter() + try: + result = generate_commit_message( + diffs=diffs, + cfg=cfg, + console=None, + command="commit-hook", + hook_mode=True, + ) + generated = result.text.rstrip() + except BudgetCapExceededError: + generated = generate_fallback_message(diffs, style=cfg.commit_style) + safe_record_telemetry( + command="commit-hook", + model=cfg.model, + tokens_in=None, + tokens_out=None, + cache_hit=False, + latency_ms=int((time.perf_counter() - started) * 1000), + fallback_used=True, + estimated_cost_usd=0.0, + free_tier=cfg.free_tier, + ) existing = message_file.read_text(encoding="utf-8", errors="replace") - generated = result.text.rstrip() if existing.strip(): new_message = f"{generated}\n\n{existing.lstrip()}" else: @@ -154,6 +170,14 @@ def _run_worker(args: list[str]) -> None: def _replace_message(message_file: Path, message: str) -> None: """Atomically replace Git's message file so write failures preserve its contents.""" + cutoff = time.time() - HOOK_TIMEOUT_SECONDS + for stale in message_file.parent.glob(f".{message_file.name}.*"): + try: + if stale.is_file() and stale.stat().st_mtime < cutoff: + stale.unlink() + except OSError: + continue + mode = stat.S_IMODE(message_file.stat().st_mode) temp_path: Path | None = None try: diff --git a/src/gitmate/metrics.py b/src/gitmate/metrics.py index 81b235c..87f195c 100644 --- a/src/gitmate/metrics.py +++ b/src/gitmate/metrics.py @@ -42,7 +42,8 @@ cache_hit INTEGER NOT NULL, latency_ms INTEGER, fallback_used INTEGER NOT NULL, - estimated_cost_usd REAL + estimated_cost_usd REAL, + free_tier INTEGER NOT NULL DEFAULT 0 ); CREATE INDEX IF NOT EXISTS idx_invocations_timestamp ON invocations(timestamp); CREATE INDEX IF NOT EXISTS idx_invocations_command ON invocations(command); @@ -63,6 +64,7 @@ class InvocationRecord: latency_ms: int | None fallback_used: bool estimated_cost_usd: float = 0.0 + free_tier: bool | None = None @dataclass(frozen=True) @@ -158,6 +160,9 @@ def _ensure_schema(conn: sqlite3.Connection) -> None: columns = {row[1] for row in cursor.fetchall()} if "estimated_cost_usd" not in columns: conn.execute("ALTER TABLE invocations ADD COLUMN estimated_cost_usd REAL") + if "free_tier" not in columns: + # Old rows are unknown: free-tier usage was not recorded, so don't guess. + conn.execute("ALTER TABLE invocations ADD COLUMN free_tier INTEGER") def init_db(db_path: Path | str | None = None) -> Path: @@ -213,8 +218,8 @@ def record_invocation( """ INSERT INTO invocations ( timestamp, command, model, tokens_in, tokens_out, - cache_hit, latency_ms, fallback_used, estimated_cost_usd - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + cache_hit, latency_ms, fallback_used, estimated_cost_usd, free_tier + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) """, ( ts, @@ -226,6 +231,7 @@ def record_invocation( latency_ms, 1 if fallback_used else 0, cost, + 1 if free_tier else 0, ), ) except Exception as exc: # noqa: BLE001 @@ -248,7 +254,7 @@ def get_invocations( query = ( "SELECT id, timestamp, command, model, tokens_in, tokens_out, " "cache_hit, latency_ms, fallback_used, " - "COALESCE(estimated_cost_usd, 0.0) as estimated_cost_usd " + "estimated_cost_usd, free_tier " "FROM invocations ORDER BY id ASC" ) if limit is not None: @@ -258,11 +264,13 @@ def get_invocations( rows = cursor.fetchall() records: list[InvocationRecord] = [] for row in rows: - raw_cost = float(row["estimated_cost_usd"]) - # If an older row had tokens but $0.00 was recorded (and not cache hit or fallback), - # recalculate it at read-time against current pricing table: + stored_cost = row["estimated_cost_usd"] + raw_cost = float(stored_cost or 0.0) + free_tier = None if row["free_tier"] is None else bool(row["free_tier"]) + # Reprice only known non-free rows, or rows predating cost persistence. if ( raw_cost == 0.0 + and (free_tier is False or (free_tier is None and stored_cost is None)) and not bool(row["cache_hit"]) and not bool(row["fallback_used"]) and ((row["tokens_in"] or 0) > 0 or (row["tokens_out"] or 0) > 0) @@ -285,6 +293,7 @@ def get_invocations( latency_ms=row["latency_ms"], fallback_used=bool(row["fallback_used"]), estimated_cost_usd=raw_cost, + free_tier=free_tier, ) ) return records diff --git a/src/gitmate/orchestrator.py b/src/gitmate/orchestrator.py index ab10816..ae475d3 100644 --- a/src/gitmate/orchestrator.py +++ b/src/gitmate/orchestrator.py @@ -118,6 +118,7 @@ def run_generation_pipeline( console: Console | None = None, secret_store: SecretStore | None = None, bypass_cache: bool = False, + hook_mode: bool = False, ) -> GenerationResult: """Execute unified generation: token budgeting, chunking, caching, LLM call, and fallback.""" if empty_diff_message is not None and not diffs: @@ -142,7 +143,7 @@ def run_generation_pipeline( else: active_provider = provider else: - base_provider = GeminiProvider(api_key=api_key) + base_provider = GeminiProvider(api_key=api_key, hook_mode=hook_mode) if bypass_cache: active_provider = base_provider else: @@ -158,7 +159,7 @@ def run_generation_pipeline( elif provider is not None: active_counter = HeuristicTokenCounter() else: - active_counter = GeminiTokenCounter(api_key=api_key) + active_counter = GeminiTokenCounter(api_key=api_key, hook_mode=hook_mode) t0 = time.perf_counter() gen_result: GenerationResult @@ -183,6 +184,14 @@ def run_generation_pipeline( chunk_cost = 0.0 if decision.strategy is BudgetStrategy.NEEDS_CHUNKING: + if hook_mode: + gen_result = GenerationResult( + text=fallback_generator(), + is_fallback=True, + model=cfg.model, + fallback_reason="Hook time budget does not allow chunked generation.", + ) + return gen_result if not decision.omitted_diffs: if console is not None: console.print( diff --git a/src/gitmate/providers/gemini.py b/src/gitmate/providers/gemini.py index 7ded756..8a23166 100644 --- a/src/gitmate/providers/gemini.py +++ b/src/gitmate/providers/gemini.py @@ -4,13 +4,32 @@ from typing import TYPE_CHECKING -from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_exponential +from tenacity import ( + retry, + retry_if_exception_type, + stop_after_attempt, + wait_exponential, + wait_fixed, +) from gitmate.providers.base import LLMResponse, ProviderUnavailable, RetryableProviderError if TYPE_CHECKING: from google import genai +HOOK_REQUEST_TIMEOUT_MS = 900 +HOOK_GENERATION_ATTEMPTS = 2 + + +def hook_http_options() -> object: + """Limit each Gemini HTTP request and disable SDK-level retries for hooks.""" + from google.genai import types + + return types.HttpOptions( + timeout=HOOK_REQUEST_TIMEOUT_MS, + retry_options=types.HttpRetryOptions(attempts=1), + ) + def _is_transient(exc: BaseException) -> bool: """True for failures worth retrying: 5xx, 429 rate limits, transport.""" @@ -28,9 +47,15 @@ def _is_transient(exc: BaseException) -> bool: class GeminiProvider: """LLMProvider backed by Google's Gemini models via google-genai.""" - def __init__(self, api_key: str | None = None, client: genai.Client | None = None) -> None: + def __init__( + self, + api_key: str | None = None, + client: genai.Client | None = None, + hook_mode: bool = False, + ) -> None: self._client = client self._api_key = api_key + self._hook_mode = hook_mode def _get_client(self) -> genai.Client: if self._client is not None: @@ -41,12 +66,20 @@ def _get_client(self) -> genai.Client: ) from google import genai - self._client = genai.Client(api_key=self._api_key) + if self._hook_mode: + self._client = genai.Client(api_key=self._api_key, http_options=hook_http_options()) + else: + self._client = genai.Client(api_key=self._api_key) return self._client def generate(self, prompt: str, model: str) -> LLMResponse: """Generate text for prompt, retrying transient failures then raising.""" try: + if self._hook_mode: + return self._generate_with_retry.retry_with( # type: ignore[attr-defined] + stop=stop_after_attempt(HOOK_GENERATION_ATTEMPTS), + wait=wait_fixed(0), + )(self._get_client(), prompt, model) return self._generate_with_retry(self._get_client(), prompt, model) except RetryableProviderError as exc: raise ProviderUnavailable(f"Gemini unavailable after retries: {exc}") from exc diff --git a/src/gitmate/token_budget.py b/src/gitmate/token_budget.py index a606f0c..9afa6ed 100644 --- a/src/gitmate/token_budget.py +++ b/src/gitmate/token_budget.py @@ -51,9 +51,11 @@ def __init__( self, api_key: str | None = None, client: genai.Client | None = None, + hook_mode: bool = False, ) -> None: self._client = client self._api_key = api_key + self._hook_mode = hook_mode def _get_client(self) -> genai.Client: if self._client is not None: @@ -64,7 +66,15 @@ def _get_client(self) -> genai.Client: ) from google import genai - self._client = genai.Client(api_key=self._api_key) + if self._hook_mode: + from gitmate.providers.gemini import hook_http_options + + self._client = genai.Client( + api_key=self._api_key, + http_options=hook_http_options(), + ) + else: + self._client = genai.Client(api_key=self._api_key) return self._client def count_tokens(self, text: str, model: str) -> int: diff --git a/tests/test_config.py b/tests/test_config.py index 29fc5fd..2bd2b79 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -142,6 +142,14 @@ def test_get_context_window_unknown_model_raises() -> None: assert issubclass(UnknownModelError, ConfigError) +@pytest.mark.parametrize( + ("model", "expected"), + [("gemini-future-model", 1_048_576), ("claude-future-model", 200_000)], +) +def test_get_context_window_uses_family_default(model: str, expected: int) -> None: + assert get_context_window(GitmateConfig(model=model)) == expected + + def test_get_context_window_unknown_model_with_override() -> None: cfg = GitmateConfig(model="unknown-model-xyz", max_context_tokens=32_000) assert get_context_window(cfg) == 32_000 diff --git a/tests/test_hooks.py b/tests/test_hooks.py index ff61e76..43d2316 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -13,6 +13,7 @@ from gitmate import hooks as hooks_mod from gitmate.cli import app +from gitmate.config import GitmateConfig, save_config from gitmate.hooks import HOOK_MARKER, HookError, install_hook, uninstall_hook from gitmate.metrics import get_invocations @@ -83,6 +84,36 @@ def test_install_is_executable_idempotent_and_uninstallable(tmp_path: Path) -> N assert not path.exists() +def test_reinstall_rewrites_gitmate_hook_to_current_script( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + path = install_hook(repo) + path.write_text(f"#!/bin/sh\n{HOOK_MARKER}\nold python path\n", encoding="utf-8") + current_script = f"#!/bin/sh\n{HOOK_MARKER}\ncurrent python path\n" + monkeypatch.setattr(hooks_mod, "HOOK_SCRIPT", current_script) + + install_hook(repo) + + assert path.read_text(encoding="utf-8") == current_script + assert path.stat().st_mode & 0o111 + + +def test_reinstall_updates_old_hook_script(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + path = install_hook(repo) + path.write_text(f"#!/bin/sh\n{HOOK_MARKER}\nold-interpreter\n", encoding="utf-8") + updated = f"#!/bin/sh\n{HOOK_MARKER}\nupdated-interpreter\n" + monkeypatch.setattr(hooks_mod, "HOOK_SCRIPT", updated) + + install_hook(repo) + + assert path.read_text(encoding="utf-8") == updated + assert path.stat().st_mode & 0o111 + + def test_install_refuses_to_overwrite_existing_hook(tmp_path: Path) -> None: repo = tmp_path / "repo" _init_repo(repo) @@ -159,6 +190,41 @@ def test_real_commit_gets_fallback_message_and_hook_metrics( assert rows[0].fallback_used +def test_budget_cap_uses_template_fallback_in_real_hook( + tmp_path: Path, hook_env: dict[str, str] +) -> None: + repo = tmp_path / "repo" + _init_repo(repo) + install_hook(repo) + save_config(GitmateConfig(budget_cap_usd=0)) + (repo / "feature.py").write_text("print('hello')\n", encoding="utf-8") + _git(repo, "add", "feature.py") + + result = _commit(repo, env=hook_env) + assert result.returncode == 0, result.stderr + assert _git(repo, "log", "-1", "--format=%s").stdout.strip() == "feat: add feature.py" + [record] = get_invocations() + assert record.command == "commit-hook" + assert record.fallback_used + + +def test_replace_message_removes_old_orphaned_temporary_files(tmp_path: Path) -> None: + message_file = tmp_path / "COMMIT_EDITMSG" + message_file.write_text("original\n", encoding="utf-8") + old_temp = tmp_path / ".COMMIT_EDITMSG.abandoned" + current_temp = tmp_path / ".COMMIT_EDITMSG.current" + old_temp.write_text("partial\n", encoding="utf-8") + current_temp.write_text("active\n", encoding="utf-8") + old = time.time() - hooks_mod.HOOK_TIMEOUT_SECONDS - 2 + os.utime(old_temp, (old, old)) + + hooks_mod._replace_message(message_file, "new message\n") + + assert not old_temp.exists() + assert current_temp.exists() + assert message_file.read_text(encoding="utf-8") == "new message\n" + + def test_message_source_is_untouched(tmp_path: Path, hook_env: dict[str, str]) -> None: repo = tmp_path / "repo" _init_repo(repo) @@ -262,25 +328,29 @@ def test_commit_preserves_template_and_prepends_message( assert "Reviewed-by: Lead" in full_message -def test_cli_commit_hook_mode(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - repo = tmp_path / "repo" - _init_repo(repo) - monkeypatch.chdir(repo) +def test_commit_command_has_no_hook_arguments() -> None: + runner = CliRunner() + help_result = runner.invoke(app, ["commit", "--help"]) + assert help_result.exit_code == 0 + assert "hook-mode" not in help_result.output + assert "commit_msg_file" not in help_result.output - (repo / "new_module.py").write_text("val = 100\n", encoding="utf-8") - _git(repo, "add", "new_module.py") + unexpected_arg = runner.invoke(app, ["commit", "some_file.py"]) + assert unexpected_arg.exit_code != 0 - msg_file = tmp_path / "COMMIT_EDITMSG" - msg_file.write_text("# Please enter the commit message\n# On branch main\n", encoding="utf-8") - runner = CliRunner() - res = runner.invoke(app, ["commit", "--hook-mode", str(msg_file)]) - assert res.exit_code == 0, res.output - content = msg_file.read_text(encoding="utf-8") - assert content.startswith("feat: add new_module.py") - assert "# Please enter the commit message" in content - - # Missing file argument returns error - res_err = runner.invoke(app, ["commit", "--hook-mode"]) - assert res_err.exit_code == 1 - assert "error" in res_err.output.lower() +def test_replace_message_prunes_stale_temp_files(tmp_path: Path) -> None: + message_file = tmp_path / "COMMIT_EDITMSG" + message_file.write_text("original\n", encoding="utf-8") + stale = tmp_path / ".COMMIT_EDITMSG.stale" + recent = tmp_path / ".COMMIT_EDITMSG.recent" + stale.write_text("orphaned\n", encoding="utf-8") + recent.write_text("active\n", encoding="utf-8") + old_time = time.time() - hooks_mod.HOOK_TIMEOUT_SECONDS - 10 + os.utime(stale, (old_time, old_time)) + + hooks_mod._replace_message(message_file, "generated\n") + + assert not stale.exists() + assert recent.exists() + assert message_file.read_text(encoding="utf-8") == "generated\n" diff --git a/tests/test_metrics.py b/tests/test_metrics.py index 4437b63..4486f1c 100644 --- a/tests/test_metrics.py +++ b/tests/test_metrics.py @@ -100,9 +100,27 @@ def test_record_invocation_and_retrieve_all_fields(metrics_dir: Path) -> None: assert rec.cache_hit is False assert rec.latency_ms == 450 assert rec.fallback_used is False + assert rec.free_tier is False assert "T" in rec.timestamp # ISO 8601 +def test_free_tier_is_persisted_and_not_repriced(metrics_dir: Path) -> None: + record_invocation( + command="commit", + model="gemini-3.5-flash-lite", + tokens_in=1_000_000, + tokens_out=1_000_000, + cache_hit=False, + latency_ms=200, + fallback_used=False, + free_tier=True, + ) + + [record] = get_invocations() + assert record.free_tier is True + assert record.estimated_cost_usd == 0.0 + + def test_record_invocation_boolean_mappings(metrics_dir: Path) -> None: # Cache hit record_invocation( @@ -333,6 +351,32 @@ def test_schema_migration_adds_estimated_cost_column(tmp_path: Path) -> None: assert records_after[1].estimated_cost_usd == 2.80 +def test_migration_does_not_guess_free_tier_for_existing_zero_cost_rows( + tmp_path: Path, +) -> None: + db_file = tmp_path / "existing_metrics.db" + conn = sqlite3.connect(db_file) + conn.executescript( + """ + CREATE TABLE invocations ( + id INTEGER PRIMARY KEY, timestamp TEXT NOT NULL, command TEXT NOT NULL, + model TEXT NOT NULL, tokens_in INTEGER, tokens_out INTEGER, + cache_hit INTEGER NOT NULL, latency_ms INTEGER, + fallback_used INTEGER NOT NULL, estimated_cost_usd REAL + ); + INSERT INTO invocations VALUES ( + 1, '2026-09-01T12:00:00Z', 'commit', 'gemini-3.5-flash-lite', + 1000000, 1000000, 0, 200, 0, 0.0 + ); + """ + ) + conn.close() + + [record] = get_invocations(db_file) + assert record.free_tier is None + assert record.estimated_cost_usd == 0.0 + + def test_get_monthly_spend(metrics_dir: Path) -> None: from gitmate.metrics import get_monthly_spend diff --git a/tests/test_providers.py b/tests/test_providers.py index 2053c94..83bb37b 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -3,6 +3,8 @@ from __future__ import annotations from pathlib import Path +from time import monotonic, sleep +from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock @@ -11,7 +13,8 @@ from gitmate.providers.base import LLMProvider, LLMResponse, ProviderUnavailable from gitmate.providers.cache import CachedProvider, cache_key -from gitmate.providers.gemini import GeminiProvider +from gitmate.providers.gemini import GeminiProvider, hook_http_options +from gitmate.hooks import HOOK_TIMEOUT_SECONDS @pytest.fixture(autouse=True) @@ -185,6 +188,36 @@ def test_gemini_provider_exhausts_retries() -> None: assert client.models.generate_content.call_count == 3 +def test_hook_provider_retry_fits_hook_timeout() -> None: + import httpx + + class SlowThenSuccessfulModels: + calls = 0 + + def generate_content(self, **kwargs: Any) -> MagicMock: + self.calls += 1 + sleep(1.1) + if self.calls == 1: + raise httpx.ConnectError("temporary connection failure") + return _resp("ok") + + models = SlowThenSuccessfulModels() + provider = GeminiProvider(client=SimpleNamespace(models=models), hook_mode=True) + started = monotonic() + result = provider.generate("prompt", "gemini-3.5-flash-lite") + elapsed = monotonic() - started + + assert result.text == "ok" + assert models.calls == 2 + assert elapsed < HOOK_TIMEOUT_SECONDS + + +def test_hook_http_options_disable_sdk_retries_and_bound_requests() -> None: + options = hook_http_options() + assert options.timeout == 900 + assert options.retry_options.attempts == 1 + + def test_gemini_provider_non_transient_raises_without_retry() -> None: from google.genai.errors import ClientError From 59e2bdeac1e4f7c25829124d309c378bf98e4fec Mon Sep 17 00:00:00 2001 From: Amar K P Date: Wed, 23 Sep 2026 23:28:30 +0530 Subject: [PATCH 4/4] fix: repair hook-mode Gemini retry wiring lost from c72ba19 The previous commit captured a stale pre-fix version of the provider and test files. Restore the verified implementation: - Split Gemini retries into _generate_with_retry (3 attempts, exponential backoff) and _generate_with_hook_retry (2 attempts, zero backoff) sharing _generate_once; retry_with on an instance method re-wraps the unbound function and misbinds arguments - Type hook_http_options() as types.HttpOptions (mypy --strict) - Rework the hook timeout test around MagicMock and add a test proving hook mode exhausts after 2 attempts, not 3 - Drop duplicated hook reinstall/prune tests - Assert free-tier rows contribute $0 to monthly spend --- src/gitmate/providers/gemini.py | 46 ++++++++++++++++++++++----------- tests/test_hooks.py | 33 ++--------------------- tests/test_metrics.py | 5 ++++ tests/test_providers.py | 44 ++++++++++++++++++++----------- 4 files changed, 67 insertions(+), 61 deletions(-) diff --git a/src/gitmate/providers/gemini.py b/src/gitmate/providers/gemini.py index 8a23166..8a61eb2 100644 --- a/src/gitmate/providers/gemini.py +++ b/src/gitmate/providers/gemini.py @@ -16,13 +16,19 @@ if TYPE_CHECKING: from google import genai + from google.genai import types +#: Hook-mode worst case must stay inside HOOK_TIMEOUT_SECONDS (hooks.py, 4s). +#: The worker makes one count_tokens request plus HOOK_GENERATION_ATTEMPTS +#: generation attempts, each capped at HOOK_REQUEST_TIMEOUT_MS with zero +#: backoff: 3 x 900ms = 2.7s of network time, leaving ~1.3s for interpreter +#: startup, diff extraction, and the fallback template. HOOK_REQUEST_TIMEOUT_MS = 900 HOOK_GENERATION_ATTEMPTS = 2 -def hook_http_options() -> object: - """Limit each Gemini HTTP request and disable SDK-level retries for hooks.""" +def hook_http_options() -> types.HttpOptions: + """Bound each Gemini HTTP request and disable SDK-level retries for hooks.""" from google.genai import types return types.HttpOptions( @@ -74,23 +80,13 @@ def _get_client(self) -> genai.Client: def generate(self, prompt: str, model: str) -> LLMResponse: """Generate text for prompt, retrying transient failures then raising.""" + generate = self._generate_with_hook_retry if self._hook_mode else self._generate_with_retry try: - if self._hook_mode: - return self._generate_with_retry.retry_with( # type: ignore[attr-defined] - stop=stop_after_attempt(HOOK_GENERATION_ATTEMPTS), - wait=wait_fixed(0), - )(self._get_client(), prompt, model) - return self._generate_with_retry(self._get_client(), prompt, model) + return generate(self._get_client(), prompt, model) except RetryableProviderError as exc: raise ProviderUnavailable(f"Gemini unavailable after retries: {exc}") from exc - @retry( - retry=retry_if_exception_type(RetryableProviderError), - stop=stop_after_attempt(3), - wait=wait_exponential(multiplier=1, min=1, max=8), - reraise=True, - ) - def _generate_with_retry(self, client: genai.Client, prompt: str, model: str) -> LLMResponse: + def _generate_once(self, client: genai.Client, prompt: str, model: str) -> LLMResponse: try: resp = client.models.generate_content(model=model, contents=prompt) except Exception as exc: @@ -115,3 +111,23 @@ def _generate_with_retry(self, client: genai.Client, prompt: str, model: str) -> input_tokens=getattr(resp.usage_metadata, "prompt_token_count", None), output_tokens=getattr(resp.usage_metadata, "candidates_token_count", None), ) + + @retry( + retry=retry_if_exception_type(RetryableProviderError), + stop=stop_after_attempt(3), + wait=wait_exponential(multiplier=1, min=1, max=8), + reraise=True, + ) + def _generate_with_retry(self, client: genai.Client, prompt: str, model: str) -> LLMResponse: + return self._generate_once(client, prompt, model) + + @retry( + retry=retry_if_exception_type(RetryableProviderError), + stop=stop_after_attempt(HOOK_GENERATION_ATTEMPTS), + wait=wait_fixed(0), + reraise=True, + ) + def _generate_with_hook_retry( + self, client: genai.Client, prompt: str, model: str + ) -> LLMResponse: + return self._generate_once(client, prompt, model) diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 43d2316..6023a24 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -87,6 +87,7 @@ def test_install_is_executable_idempotent_and_uninstallable(tmp_path: Path) -> N def test_reinstall_rewrites_gitmate_hook_to_current_script( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: + """Reinstall overwrites a gitmate-marked hook so upgrades fix stale interpreter paths.""" repo = tmp_path / "repo" _init_repo(repo) path = install_hook(repo) @@ -100,20 +101,6 @@ def test_reinstall_rewrites_gitmate_hook_to_current_script( assert path.stat().st_mode & 0o111 -def test_reinstall_updates_old_hook_script(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - repo = tmp_path / "repo" - _init_repo(repo) - path = install_hook(repo) - path.write_text(f"#!/bin/sh\n{HOOK_MARKER}\nold-interpreter\n", encoding="utf-8") - updated = f"#!/bin/sh\n{HOOK_MARKER}\nupdated-interpreter\n" - monkeypatch.setattr(hooks_mod, "HOOK_SCRIPT", updated) - - install_hook(repo) - - assert path.read_text(encoding="utf-8") == updated - assert path.stat().st_mode & 0o111 - - def test_install_refuses_to_overwrite_existing_hook(tmp_path: Path) -> None: repo = tmp_path / "repo" _init_repo(repo) @@ -209,6 +196,7 @@ def test_budget_cap_uses_template_fallback_in_real_hook( def test_replace_message_removes_old_orphaned_temporary_files(tmp_path: Path) -> None: + """A SIGKILLed worker leaves .COMMIT_EDITMSG.* orphans; the next write prunes them.""" message_file = tmp_path / "COMMIT_EDITMSG" message_file.write_text("original\n", encoding="utf-8") old_temp = tmp_path / ".COMMIT_EDITMSG.abandoned" @@ -337,20 +325,3 @@ def test_commit_command_has_no_hook_arguments() -> None: unexpected_arg = runner.invoke(app, ["commit", "some_file.py"]) assert unexpected_arg.exit_code != 0 - - -def test_replace_message_prunes_stale_temp_files(tmp_path: Path) -> None: - message_file = tmp_path / "COMMIT_EDITMSG" - message_file.write_text("original\n", encoding="utf-8") - stale = tmp_path / ".COMMIT_EDITMSG.stale" - recent = tmp_path / ".COMMIT_EDITMSG.recent" - stale.write_text("orphaned\n", encoding="utf-8") - recent.write_text("active\n", encoding="utf-8") - old_time = time.time() - hooks_mod.HOOK_TIMEOUT_SECONDS - 10 - os.utime(stale, (old_time, old_time)) - - hooks_mod._replace_message(message_file, "generated\n") - - assert not stale.exists() - assert recent.exists() - assert message_file.read_text(encoding="utf-8") == "generated\n" diff --git a/tests/test_metrics.py b/tests/test_metrics.py index 4486f1c..22e0ecd 100644 --- a/tests/test_metrics.py +++ b/tests/test_metrics.py @@ -105,6 +105,7 @@ def test_record_invocation_and_retrieve_all_fields(metrics_dir: Path) -> None: def test_free_tier_is_persisted_and_not_repriced(metrics_dir: Path) -> None: + """Free-tier rows must read back as $0 spend so they can never spuriously trip a budget cap.""" record_invocation( command="commit", model="gemini-3.5-flash-lite", @@ -120,6 +121,10 @@ def test_free_tier_is_persisted_and_not_repriced(metrics_dir: Path) -> None: assert record.free_tier is True assert record.estimated_cost_usd == 0.0 + from gitmate.metrics import get_monthly_spend + + assert get_monthly_spend() == 0.0 + def test_record_invocation_boolean_mappings(metrics_dir: Path) -> None: # Cache hit diff --git a/tests/test_providers.py b/tests/test_providers.py index 83bb37b..0d4fd7b 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -4,17 +4,16 @@ from pathlib import Path from time import monotonic, sleep -from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock import pytest from tenacity import wait_none +from gitmate.hooks import HOOK_TIMEOUT_SECONDS from gitmate.providers.base import LLMProvider, LLMResponse, ProviderUnavailable from gitmate.providers.cache import CachedProvider, cache_key from gitmate.providers.gemini import GeminiProvider, hook_http_options -from gitmate.hooks import HOOK_TIMEOUT_SECONDS @pytest.fixture(autouse=True) @@ -189,32 +188,47 @@ def test_gemini_provider_exhausts_retries() -> None: def test_hook_provider_retry_fits_hook_timeout() -> None: + """A slow request that fails once must retry and still finish inside the hook budget.""" import httpx - class SlowThenSuccessfulModels: - calls = 0 - - def generate_content(self, **kwargs: Any) -> MagicMock: - self.calls += 1 - sleep(1.1) - if self.calls == 1: - raise httpx.ConnectError("temporary connection failure") - return _resp("ok") - - models = SlowThenSuccessfulModels() - provider = GeminiProvider(client=SimpleNamespace(models=models), hook_mode=True) + client = MagicMock() + calls = 0 + + def slow_then_succeed(**kwargs: Any) -> MagicMock: + nonlocal calls + calls += 1 + sleep(1.1) + if calls == 1: + raise httpx.ConnectError("temporary connection failure") + return _resp("ok") + + client.models.generate_content.side_effect = slow_then_succeed + provider = GeminiProvider(client=client, hook_mode=True) started = monotonic() result = provider.generate("prompt", "gemini-3.5-flash-lite") elapsed = monotonic() - started assert result.text == "ok" - assert models.calls == 2 + assert client.models.generate_content.call_count == 2 assert elapsed < HOOK_TIMEOUT_SECONDS +def test_hook_provider_exhausts_after_two_attempts_not_three() -> None: + """Hook mode trades resilience for the time budget: 2 attempts, no backoff.""" + from google.genai.errors import ServerError + + client = MagicMock() + client.models.generate_content.side_effect = ServerError(500, None, None) + provider = GeminiProvider(client=client, hook_mode=True) + with pytest.raises(ProviderUnavailable, match="unavailable after retries"): + provider.generate("p", "m") + assert client.models.generate_content.call_count == 2 + + def test_hook_http_options_disable_sdk_retries_and_bound_requests() -> None: options = hook_http_options() assert options.timeout == 900 + assert options.retry_options is not None assert options.retry_options.attempts == 1