stop hook: don't silently skip gates, don't block on own artifacts

[evgunter]

Stop-hook failures that made the hook either skip work it should have done or block on work nobody could do — plus the test suite that pins them down.

Silently skipped gates

Step 5 decided "docs-only" from git diff --name-only "origin/$BASE_BRANCH"...HEAD 2>/dev/null || echo "". When origin/main did not resolve — no remote, fetch_and_merge off, or a base branch that simply is not called main — the diff errored, the fallback made CHANGED_FILES empty, and an empty file list is indistinguishable from docs-only. Every review and PR gate was skipped, silently, on a branch full of code.

The base ref is now resolved explicitly (origin/<base>, else the local branch), and the diff no longer swallows its own errors. If neither ref exists, the hook says so and exits 1 — non-blocking, because only a human can fix a misconfigured base branch, and blocking the agent on a demand it cannot satisfy just wedges the session.

Blocking on its own artifacts

The uncommitted-changes gate blocks on any untracked file, and the hook's own .reviewer/logs/ and .reviewer/outputs/ are untracked — so it blocked on its own exhaust. ensure_reviewer_gitignore now writes .reviewer/.gitignore before the first log write (which is what creates the directory). The rules cover .gitignore itself so the fix does not become the same kind of noise; settings.json and the category files stay trackable.

Miscounted blocks

The EXIT trap recorded a consecutive block on any non-zero exit, so non-blocking exit-1 paths inflated the counter and dragged the stuck-hatch closer for reasons the agent was never asked to fix. It now counts only exit 2, the blocking code.

A typo disabled everything

Found while writing the tests, in code this branch had not touched. read_json_config returns nothing for every key in an unparseable file, and Step 1 cannot tell that apart from "enabled_when was never set" — so it exited 0. One stray comma in .reviewer/settings.json silently switched off every gate, with 2>/dev/null on the jq call ensuring no diagnostic anywhere. Demonstrated on one repo, changing nothing but the comma:

valid config  -> exit 2   gates fire
+ one comma   -> exit 0   hook does nothing, says nothing

Both config files are now checked before the first read, reported on stderr, and exit 1 — non-blocking, same reasoning as the unresolvable base above. The 2>/dev/null is gone as well, so a read that aborts on a typo carries jq's parse error out with it instead of dying mute. get_config.sh surfaces the same fault differently, exiting 5 with the parse error on stderr and nothing on stdout, which is what the skills consume; the up-front check is what actually protects the gates.

Also here

• _ensure_pr creates .reviewer/outputs up front, since its early-return paths write there too. _poll_ci writes pr_status to the same directory and never created it either; standalone, green CI came back as exit 1 with a raw bash redirect error. Masked today only because the orchestrator always runs ensure-pr first.
• The autofix and verify-architecture skills resolved the base branch with echo "${GIT_BASE_BRANCH:-main}", which reads a different env var than the hook's own resolution and silently disagreed with it about which branch is the base. Both now go through scripts/get_config.sh, a small CLI wrapper over the existing read_json_config, so the skills and the hook always agree.

Tests

uv run pytest — 116 tests, ~23s, or ~5s with -n auto. No network.

pytest rather than bash, even though the scripts are shell: every test is a black-box subprocess call asserting on an exit code, an output stream, or a file on disk, and none reaches inside a script. The harness language is therefore a free choice, and the shipped plugin is unaffected — it stays bash + jq + gh. pytest is a contributor dependency only, next to the uv and python that .pre-commit-config.yaml already requires.

The suite covers the scripts this branch does not change, not just the new behavior, so it documents the existing contract: config precedence, the common helpers, the gate marker logic, PR/CI with a gh stub that refuses unrecognized calls, and orchestrator Steps 0–3 plus the EXIT trap. Pointed at main's scripts it fails 30 tests, every one of them on a fix this branch makes.

Hermeticity is structural here rather than conventional, which is most of why it is not bash:

• Each test drives its own repo under tmp_path through an explicit subprocess cwd. The test process never changes directory, so no helper can operate on the real checkout — a bash harness that ran cd inside a command substitution once committed test fixtures to the working branch.
• The subprocess environment drops CODE_GUARDIAN_*, which outrank every config file and which the mngr integration exports five of into each session, and points GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM at /dev/null. A personal global .reviewer/ ignore rule would otherwise mask exactly what the gitignore tests check, and the suite would pass for that developer and fail for everyone else.
• Every repo gets a gh stub that refuses any call, from a stub directory outside the work tree. Reaching the real API takes deliberately overwriting that stub, rather than merely forgetting to write one.

Verification

uv run pytest passes all 116 tests here and fails 30 against main's scripts, every one of them on a fix this branch makes. The scenarios run end to end through stop_hook_orchestrator.sh: the unresolvable base exiting 1, the local base fallback reaching the gates, docs-only and empty diffs still skipping cleanly, the block tracker gaining an entry on exit 2 and none on exit 1, and .reviewer/.gitignore being written at all.

The silent skip needs no misconfiguration to reach. Any repo without a remote — or with fetch_and_merge off, which is how the mngr integration runs — hits it.

The two failures also mask each other, which is most of why the skip went unnoticed. On a clean checkout main never reaches the base branch logic at all: Step 3 blocks on the hook's own untracked .reviewer/logs first, so every scenario exits 2 and the base is never consulted. Ignore .reviewer/ in your global gitignore — as anyone keeping their reviewer config out of shared repos does — and the self-block disappears, Step 5 is reached, and main silently skips every gate instead. Which of the two you see depends on your gitignore, and each is invisible while the other holds.

🤖 Generated with Claude Code