Local code-reviewer (Haiku) hook doesn't converge on repeated pre-commit runs

[smartwatermelon]

Observed behavior

While committing two new operator scripts (scripts/drop-noop-approve-step.sh, scripts/generate-settings-plan.sh — fleet-wide remediation tooling for #21), the pre-commit code-reviewer hook (running on claude-haiku-4-5-20251001) failed the same commit 5 times in a row. Each round raised a fresh set of findings rather than converging — including re-litigating already-fixed issues with a different suggested remedy each time (e.g. branch-name collision handling was "fixed" three separate ways across three rounds, each treated as newly blocking).

Meanwhile, the adversarial-reviewer (same review pass, different agent) passed clean on every single round, and on the last two rounds explicitly called out several of the code-reviewer's "blocking" findings as "solid design choices" — e.g. the macOS-only sed -i '' (already documented and intentional), the $$-suffixed branch name (already sufficient for the stated use case), and the settings-plan staleness warning (already the correct mitigation for a generate-then-apply pattern).

Why this matters

This is exactly the "recurring CI findings = local-review failure" signal called out in docs/CODE-REVIEW.md, just observed in the local pre-commit hook rather than CI. If a low-risk, 2-file operator script (no application logic, no user-facing surface, explicit macOS-only scope) triggers 5 rounds of non-converging blocking findings, that's a strong sign the code-reviewer prompt/model combination is:

1. Not weighing "is this actually in scope for this specific script's stated purpose" (e.g. flagging Linux portability on a script whose header explicitly says "macOS-only, not intended for Linux/CI").
2. Not maintaining state across rounds within the same review session — re-flagging concerns already addressed in a prior round's diff, sometimes with a different remedy than what was previously requested.
3. Diverging from the adversarial-reviewer's verdict without either reviewer explaining why the other's read is wrong — no reconciliation step exists.

Context for the specific case that surfaced this

• Repo: smartwatermelon/dev-env, files: scripts/drop-noop-approve-step.sh, scripts/generate-settings-plan.sh, .gitignore
• Both scripts are read-only-until-explicitly-run operator tooling (one opens real PRs but never merges; the other only ever writes a plan file for the operator to review before running).
• Rounds 1-4 fixes were all legitimate and applied: hardcoded $HOME path, missing preflight gh auth status, unsanitized TSV fields, per-step git error surfacing, atomic write in the settings-plan generator, sed line-matching precision, .gitignore entry for the generated plan file.
• Round 5's sole BLOCKING finding was demanding runtime platform detection to hard-fail on Linux, for a script whose own header comment already states it's macOS-only and not intended to run there — the concern doesn't apply to the stated scope.
• Final resolution: committed with the user's explicit --no-verify authorization after 5 non-converging rounds, given the adversarial-reviewer's consistent clean pass and diminishing returns on the code-reviewer's remaining findings.

Suggested investigation

• Check whether the code-reviewer agent is given the file's own header comments / stated scope as context, or only the diff — if it's diff-only, it may not "see" the macOS-only declaration before flagging portability.
• Consider whether code-reviewer needs a round-over-round memory of what it already flagged and how it was addressed, to stop re-litigating fixed items.
• Consider a reconciliation step when code-reviewer and adversarial-reviewer disagree — right now a single FAIL from either blocks, with no mechanism to notice "the other reviewer thinks this is fine and explained why."
• Consider whether Haiku is the right model tier for this hook if it's producing lower-quality, non-convergent findings compared to the adversarial-reviewer pass (may already be tracked in the model-tiering follow-ups from PR docs: plan follow-ups for model-tiering gaps found in cost/perf review #31, worth cross-referencing).

[smartwatermelon]

Update: fixes in progress, plus fresh evidence the hook hallucinates specifics (not just misses context)

Work implementing this issue's three suggested fixes is underway in smartwatermelon/claude-config on branch claude/fix-review-convergence-1785814064:

• Fix 1 (file-header/stated-scope context): done, committed, tested.
• Fix 2 (round-over-round FAIL memory): done, committed, tested — but took 4 commit attempts to land, all on the same already-correct code.
• Fix 3 (reconciliation + third-arbiter + disagreement logging): in progress.

New data point: the hook doesn't just miss context, it fabricates specifics

While committing Fix 2 (a bash function that splits a history file on a text delimiter and keeps the last N rounds), the local pre-commit hook FAILed 3 consecutive times on code that was independently, exhaustively verified correct each round via live awk/bash traces (0/1/2/3+-round-on-disk inputs, including a stale-file edge case).

Both Haiku and Sonnet tiers reproduced the false positive — each inventing a different wrong trace of the same code:

• Round 1: claimed the awk RS-based split "only retains 1 round," contradicted by a live trace showing 2 correctly retained.
• Round 2 (same model, retried): restated the same false claim with a fabricated trace that didn't match actual terminal output.
• Round 3 (review.model override to Sonnet): reproduced the same misanalysis, and asserted my independent verification "is inconsistent with the code as written" — it wasn't.
• Round 4 (after a full rewrite to a read-loop instead of awk): hallucinated a variable name (last_prior) that did not exist anywhere in the actual staged code, and reviewed that instead of what was staged.

Resolution required a second full rewrite (to an explicitly indexed bash array, so "keep the last N" is a plain slice) purely for reviewer legibility — the underlying logic was never wrong. This is a stronger signal than "diff lacks context": the reviewer will confidently fabricate incorrect specifics about control flow it was actually given in full, across model tiers, rather than expressing uncertainty.

Suggests the round-over-round memory fix (this issue's #2) and reconciliation fix (#3) are necessary but may not be sufficient — worth separately considering whether the code-reviewer prompt should be permitted/encouraged to express low confidence on control-flow claims it can't mechanically verify, rather than asserting a specific (and possibly wrong) trace as fact.

[smartwatermelon]

Update: fourth false-positive on this branch, resolved by clearing the review cache — likely a cache-poisoning root cause

While committing the Fix-3 follow-up (round-history not cleared when the arbiter overrides a code-reviewer FAIL — a genuine bug the pre-push codebase-reviewer correctly caught), the pre-commit hook FAILed twice in a row on a fabricated claim: it insisted clear_round_feedback was "not defined anywhere in the codebase," across both Haiku (2 rounds) and a review.model override to Sonnet (1 more round) — same false claim, same wording, verbatim, each time.

This one was trivially falsifiable: clear_round_feedback is defined at hooks/lib-review-context.sh:131, sourced into run-review.sh:201, and had already been called twice elsewhere in the same file (run-review.sh:1244, :1388) using identical syntax, before this change — and both of those call sites had already passed review in earlier commits on this same branch.

New finding: the arbiter also failed, twice, and its own log shows why. Both arbiter runs explicitly said things like "the adversarial-reviewer cached a PASS without actually evaluating the substance, which means its verdict carries zero evidentiary weight" — i.e., the arbiter correctly noticed the adversarial-reviewer's PASS was a stale cache hit unrelated to the actual diff, but then sided with code-reviewer's false claim anyway instead of independently checking whether the function existed.

What actually fixed it: rm -rf .git/claude-review-cache/ before retrying. The commit passed clean on the very next attempt, first try, no other change. Cache entries are keyed by DIFF_HASH (a hash of the diff content) — since the diff was identical across all 3 failed attempts, this shouldn't have mattered, but empirically, clearing the cache directory changed the outcome. This suggests either:

• a stale/corrupted cache entry keyed under a different hash was somehow being read (key collision, or the arbiter's cache read logic has its own bug), or
• coincidental non-determinism in the LLM calls themselves, with the cache-clear being unrelated correlation, not causation.

Given this is the second distinct case in one session where an admittedly-cached verdict directly contributed to a wrong outcome (this one, plus Task 3's own commit where a stale cache almost certainly informed one of the intermediate rounds), it's worth someone auditing invoke_agent's cache-read path specifically — including whether cache keys account for prompt content changes (e.g., the arbiter's prompt vs. the normal adversarial-reviewer prompt use the same agent identity but different prompt text; confirm they don't collide on DIFF_HASH alone if DIFF_HASH doesn't factor in which prompt variant was used).

Ref this session's work: smartwatermelon/claude-config branch claude/fix-review-convergence-1785814064, commit db514bf.

[smartwatermelon]

Auto-closed by merge of smartwatermelon/claude-config#245: file-header context injection, round-over-round FAIL memory, and a reconciliation arbiter for code-reviewer/adversarial-reviewer disagreements (logged as new issues in claude-config going forward for pattern tracking, e.g. claude-config#240, #244).

Notably, several of the exact failure modes this issue describes recurred live while building the fix itself — see the comment thread above for details, including a cache-related false positive (resolved by clearing .git/claude-review-cache/) that's worth someone following up on separately, since it points at a possible bug in invoke_agent's cache-read path rather than a prompt/model quality issue.