Feat/review accuracy and repo context

[atoomic]

Overhauls /review accuracy and consistency so repeat reviews stop drifting, the yellow "Important"
tier stops over-blocking, and humans can close the loop. Two bodies of work on this branch:

Review accuracy groundwork (4605879, pre-existing on branch)

• Snippet validation — reconcile each finding's model-authored code_snippet against authoritative
  content at the reviewed SHA (resync stale quotes, drop invalid anchors) so a review never quotes
  pre-fix source under a HEAD permalink.
• Prior-finding reconciliation — suppress re-raised, already-fixed findings; add a "Resolved since
  last review" section.
• Repo-convention ingestion — feed the reviewed repo's own AGENTS.md/CLAUDE.md/docs/ bundle into the
  prompt so the reviewer stops raising convention-based false positives. Config-gated, fail-open, on by
  default.

Consistency-critical logic lives in focused, deterministic, unit-tested modules (review_identity,
review_reuse, review_reconcile, review_triage); qualitative judgment stays in shared prompt partials.
No review output-schema change. New config (all fail-open, backward-compatible): review_consistency,
review_discovery, review_dispositions (+ the pre-existing
review_snippet_validation/review_reconcile/review_convention_docs).

Testing

• make lint (ruff): clean project-wide.
• Full suite: 19,354 passed, 10 skipped, 1 xfailed, 0 failed — new unit tests for identity, reuse,
  reconcile/freeze, triage (pre-existing + deferred), config getters, and prompt-injection gating; no
  regressions.
• Offline eval scorer green (9 golden cases, dataset valid), incl. new borderline_not_blocking,
  pre_existing_downgrade, recall_all cases.
• ⚠️ Live eval not yet run — run KOAN_EVAL_LIVE=1 python -m app.skill_evals review --live (needs
  provider) before merge to confirm no recall/precision regression (SC-006/007).
• Deferred to a follow-up harness extension: run-twice repeat-stability (SC-007), recall-gain
  (SC-009), and comment-injection for a live disposition eval — the single-diff harness can't express
  these; they're covered by deterministic unit tests for now.

Declarations

• Architectural change — modifies the durable skill contract specs/skills/review.md. Rationale:
  adds the review consistency/reuse/freeze guarantee, the yellow-tier bar, [Pre-Existing Issue]
  labeling, the human-disposition authority (+ attribution/injection guardrails), and the opt-in
  comprehensive-discovery mode — landed contract-first (spec updated before/with the code).

[Koan-Bot]

Previous review — superseded by a newer review below.

[Koan-Bot]

Warning

Important issues found.

• Freeze can silently suppress real new findings when GitHub's compare files array is truncated

[Koan-Bot]

Previous review — superseded by a newer review below.

[Koan-Bot]

Warning

Important issues found.

• _compare_changed_files can flip the freeze from fail-open to fail-toward-suppression

[Koan-Bot]

Previous review — superseded by a newer review below.

[Koan-Bot]

Warning

Important issues found.

• End-to-end gate test leaves run_gh unmocked — freeze path fires a real gh api compare with retry sleeps

[Koan-Bot]

Previous review — superseded by a newer review below.

[Koan-Bot]

Tip

No blocking issues found — ready to merge.

[Koan-Bot]

Previous review — superseded by a newer review below.

[Koan-Bot]

Tip

No blocking issues found — ready to merge.

[Koan-Bot]

Code Review

⚠️ Partial review — 4 file(s) omitted due to diff size and NOT reviewed: specs/010-review-consistency-triage/spec.md, instance.example/config.yaml (partial), koan/skills/core/review/evals/cases/recall_all.json, specs/010-review-consistency-triage/contracts/review-sidecar.schema.json

PR Review — Feat/review accuracy and repo context

Solid, well-factored consistency work — merge-ready with non-blocking nits.

Strengths worth calling out:

• Consistency-critical logic (identity/reuse/reconcile/triage) lives in small, pure, genuinely behavior-tested sibling modules rather than growing the ~4k-line runner; the accuracy-gate order (reconcile → snippet-resync → freeze → pre-existing → deferred) is deliberate and documented, and the freeze/label precedence ("freeze wins") is coherent.
• Prior blocking issues are resolved: _compare_changed_files fails open toward skipping the freeze (None on blank response and at the 300-file cap), and the e2e gate test mocks _compare_changed_files so it no longer shells out to real gh.
• Two carried-over concerns are now fixed in this revision: _read_file_at_sha's API fallback logs on failure, and the stale REPRODUCTION_NOTE dead constant is gone.
• Untrusted repo-convention and PR-comment content is fenced with explicit injection guardrails; the durable-contract change to specs/skills/review.md is declared architectural and the docs are in sync.

Remaining items are all suggestions:

• _has_skipped_disposition keys the push/no-push decision off one exact output format — ordinary model-output variance can reject a legitimate rebase (fail-safe, but annoying).
• on_mismatch="off" keeps out-of-range findings, contradicting the config docstring; no test pins that combination.
• The run_review reuse short-circuit (Step 1g) is new branching logic with no integration test driving the early return.
• Prompt-block builders catch bare Exception, so a coding error could silently strip the {DISPOSITIONS} injection guardrail (they do log, so not fully silent).

---

🟢 Suggestions

1. feedback_no_disposition gate hinges on a brittle exact-format regex
koan/app/rebase_pr.py:2195-2196

_has_skipped_disposition requires the model's no-change output to match ^SKIPPED:\n- <text> exactly (multiline, SKIPPED: at line start, a newline, then a dash bullet). When the feedback leg runs, the agent makes no commit, and it phrases the no-op differently, _run_rebase_impl returns [feedback_no_disposition] and does not push the otherwise-successful rebase.

Why it matters: the gate is a good addition (it stops a /fix on a PR from silently reporting success when feedback wasn't applied), but the push/no-push decision now depends on one exact output shape. A valid no-op phrased SKIPPED: already addressed (inline reason), **SKIPPED:**, or as prose will fail the regex and discard a good rebase, forcing a manual retry.

It fails safe (never pushes bad code, always recoverable), so this is non-blocking — but consider loosening the matcher to accept SKIPPED: with an inline or bulleted reason, case-insensitively, so ordinary model-output variance doesn't reject a legitimate rebase.

def _has_skipped_disposition(summary: str) -> bool:
    return bool(re.search(r"(?m)^SKIPPED:\s*\n\s*-\s+\S", summary))

2. on_mismatch="off" keeps out-of-range findings, contradicting the config docstring
koan/app/review_runner.py:1419-1421

get_review_snippet_validation_config's docstring states "A finding whose anchor line is past end-of-file at HEAD is dropped unless on_mismatch is 'off'" — but read against the review-runner comments elsewhere, the intent is that a location that no longer exists should not be posted under a permalink. With on_mismatch: "off", _validate_finding_snippets skips the out-of-range drop entirely, so a finding can still be posted with a file:line permalink pointing past EOF at HEAD — the exact stale-anchor case the gate exists to prevent.

Non-default and fail-safe in direction, but the code and its contract should agree, and no test pins the off + out-of-range combination. Either drop out-of-range findings unconditionally, or state the off carve-out explicitly in the config docstring so the behavior is intentional and covered.

        if line_start > line_count:
            if on_mismatch != "off":
                drop_indices.add(idx)
            continue

3. Reuse short-circuit (Step 1g) has no test driving run_review into the early return
koan/app/review_runner.py:3985-3992

The new reuse block assembles _focus_flags, computes _merge_base_sha, builds _request_signature, calls should_reuse(...), and on a match returns early from run_review (return True, msg, None), skipping the provider call entirely.

The deterministic helpers are well covered (test_review_reuse.py, TestReviewSidecarReuseFields, TestDiscoverySignature), but no test appears to exercise run_review actually taking this branch — the focus-flag assembly, the if _base_sha: guard, the 3-tuple early return, and the notify_fn call are untested. This is new branching logic on a path that can bypass the entire review; a regression that wrongly reused (or wrongly skipped) a review wouldn't be caught.

Add an integration test that mocks the prior record + SHAs so should_reuse returns True, then asserts run_review returns the reproduction tuple without invoking the provider.

            if should_reuse(_prior_record, _head_sha, _base_sha, _request_signature):
                msg = (...)
                log("review", msg)
                notify_fn(msg)
                return True, msg, None

4. Prompt-block builders catch bare Exception, so a coding error silently strips the injection guardrail
koan/app/review_runner.py:725-727

_build_dispositions_block, _build_repo_conventions_block, and _build_discovery_block each wrap their body in except Exception and return "" (with a log line). Fail-open is the right posture for a prompt-enrichment slot, and — to their credit — these do log, so it is not fully silent.

But catching bare Exception means a genuine programming error (renamed config key, missing {PLACEHOLDER}, a load_prompt typo) would be indistinguishable from a benign "feature off / no docs" and would drop the whole {DISPOSITIONS} block — including the FR-038 prompt-injection guardrail — from every review, leaving only a log line no one is watching.

Consider narrowing the caught types to the expected I/O/config errors (OSError, KeyError, ValueError) so a real defect surfaces loudly instead of quietly removing a guardrail block.

    except Exception as e:
        log("review", f"dispositions guidance injection skipped: {e}")
        return ""

---

Checklist

• New deterministic helpers have behavior tests (identity/reuse/reconcile/triage)
• New tests avoid real external boundaries (gh/provider); mock at run_gh
• New branching logic has accompanying tests — suggestion ＃1, suggestion ＃3
• Config behavior matches its documented contract — suggestion ＃2
• Boundary failures are observable (logged), not silently swallowed — suggestion ＃4
• Untrusted external content (repo docs, PR comments) is fenced/injection-guarded
• Durable-contract spec change declared as architectural

---

Silent Failure Analysis

🟡 **MEDIUM** — swallowed exception (silent local fallback)
koan/app/review_runner.py:1282-1291

Risk: The local git show branch suppresses all errors (including subprocess.TimeoutExpired, a SubprocessError) with no log, so a systematically broken or slow local clone silently falls through to a second network call each time — the exact 'silently disables/degrades' failure the sibling API branch's comment says to guard against, yet the git branch has no breadcrumb.

if project_path:
    with contextlib.suppress(OSError, ValueError, subprocess.SubprocessError):
        r = subprocess.run(
            ["git", "show", f"{sha}:{path}"],
            cwd=project_path, ..., timeout=timeout,
        )

Fix: Log at debug/info when the git show path raises (distinguishing an expected missing-object from an unexpected OSError/timeout) so a systematic local-repo failure is observable, mirroring the logging already added to the API fallback.

---

Automated review by Skuggi HEAD=39f7941 3 min 53s

[Koan-Bot]

Tip

No blocking issues found — ready to merge.

[Koan-Bot]

💡 PR Explanation

Explaining the PR: "Review accuracy and repo context"

1. The Problem (What was wrong?)

/review behaved like a slot machine. The findings came straight from the model, so nothing pinned them down between runs.

Concrete example of the pain:

• You run /review on PR feat: smarter mission classification with /chat escape hatch #42. It reports 3 issues.
• You fix all 3, push the fix, and run /review again.
• Instead of saying "all clear," it now reports 2 brand-new issues on files you never touched.

Those new issues were always there. They either mattered (and should have been caught the first time) or they didn't (and shouldn't be raised at all). Surfacing them after a fix-and-push is pure whiplash — it feels like the reviewer moved the goalposts.

Three more concrete failure modes:

• Stale code quotes. A finding quotes a code_snippet, but the quote is model-authored. After a fix, a re-review could show the old, pre-fix source under a permalink that actually points at the already-fixed code. Confusing and wrong.
• Over-blocking yellow tier. warning ("🟡 Important") is a blocking severity. Borderline "should-fix" nits landed there, so merge-ready PRs got turned into request-changes over trivia.
• Convention false positives. The reviewer didn't know the reviewed repo's own rules (its CLAUDE.md, AGENTS.md, docs), so it flagged things like a documented link style as "broken."

There was also no way for a human to say "this finding isn't a problem" and have the reviewer honor it — which violates the project's core principle: the agent proposes, the human decides.

---

2. The Solution (How does this PR fix it?)

The strategy is: move the consistency-critical decisions out of the model and into deterministic Python, while leaving qualitative judgment (what is a bug) in the prompt.

Four new focused, unit-tested modules do the mechanical work:

• review_identity.py — computes a stable identity for each finding, so the same issue is recognized across runs (needed for dedup and reconciliation).
• review_reuse.py — the reuse decision. If PR head and base are unchanged and you asked for the same kind of review, reproduce the prior review instead of re-analyzing. No drift because there's no re-analysis.
• review_reconcile.py — the re-review "freeze." When code did change, re-derive findings but suppress first-time non-critical findings on files that didn't change since the last review. This is the direct fix for fix-and-push whiplash. A genuine critical still breaks through, labeled [Pre-Existing Issue].
• review_triage.py — enforces the mechanical rules: verdict follows severity, and [Deferred] / [Pre-Existing Issue] findings get demoted to non-blocking. enforce_deferred is real Python, not a prompt hope.

Supporting changes:

• Snippet validation — each finding's quoted code is reconciled against the actual file content at the reviewed SHA. Stale quotes get resynced; quotes for lines that no longer exist get dropped. No more pre-fix source under a HEAD permalink.
• Repo-convention ingestion (review-repo-conventions.md, review_convention_docs config) — feeds the reviewed repo's own AGENTS.md/CLAUDE.md/CONTRIBUTING.md and (if present) its docs/ OKF bundle into the review prompt. Auto-detecting, degrades to a no-op for repos that ship none of these — safe to leave on.
• Prompt-fixed yellow bar — the "Important" bar and the [Pre-Existing Issue] label live in the review-severity-rubric prompt partial. They're constants, not config knobs, because the Python label must match the prompt exactly.
• Human dispositions (review-dispositions.md, review_dispositions config) — a human PR comment can dismiss a finding ("not a problem" → not re-raised as a blocker) or defer it ("fix later" → non-blocking [Deferred]). Every such change is attributed to the commenter with their quoted rationale.
• Comprehensive discovery mode (review-comprehensive-discovery.md, review_discovery, default off) — an opt-in multi-perspective pass for high-stakes PRs.

Before / after:

• Before: fix 3 issues, push, re-review → 2 new complaints on untouched files.
• After: fix 3 issues, push, re-review → clean, unless a real critical exists on pre-existing code (which is shown, but labeled [Pre-Existing Issue] and doesn't block for code you didn't write).

Every new switch is fail-open and backward-compatible — if a piece breaks or is disabled, /review behaves as it did before.

---

3. How It Works (The mechanism)

End-to-end scenario — re-running /review on PR #42 after a fix-and-push:

1. Reuse check (review_reuse): Compare current head SHA + base merge-base SHA + request type against the prior review's recorded values. If all unchanged → reproduce the prior review verbatim. Done. No model call for fresh findings.
2. Changed → re-derive. Head moved, so the model re-analyzes. The prompt now also carries the repo's own conventions and any human dispositions.
3. Snippet validation. Each finding's quote is checked against live source at the reviewed SHA. Stale → resynced; gone → dropped.
4. Reconcile + freeze (review_reconcile): Prior findings already fixed at this SHA are detected via the review sidecar and listed under "Resolved since last review." A current finding that merely re-raises a resolved prior — and whose own quote no longer matches live source — is suppressed (conservative: still-broken code is never hidden). First-time non-critical findings on unchanged files are frozen out; a critical survives, labeled [Pre-Existing Issue].
5. Triage (review_triage): Severities are enforced, [Deferred] findings demoted to non-blocking, verdict computed from the surviving severities.

Edge cases handled:

• Human dismissed a critical — honored (the posture is deliberately open: any non-bot commenter, all severities), but always attributed and quoted so it's auditable.
• Injection guardrail (FR-038) — a comment can only dispose of one specific finding; it can't rewrite the severity rubric or verdict rules. Instruction-like comment text is treated as untrusted.
• Kill switches everywhere — review_consistency.reuse_enabled, freeze_enabled, review_dispositions.enabled, etc., all fail-open.

There's also a smaller, semi-independent change riding along: /rebase conflict resolution now has a per-round 600s budget (rebase_conflict_timeout) before falling back to recreate, and the failure summary names the agent timeout when that was the cause (claude_step.py now surfaces detail over a bare exception string).

---

4. Could It Be Simpler? (Critical analysis)

The split of concerns is the right call, and it's the codebase's established pattern — see the memory note about "Determinism where it counts." Consistency guarantees (identity, reuse, freeze, verdict-follows-severity) genuinely cannot live in the model, because the model is the source of the drift. Putting them in four small, unit-tested modules is the honest way to make the guarantee real. I wouldn't collapse those.

Where it's worth questioning:

• Four separate modules vs. fewer. review_identity, review_reuse, review_reconcile, review_triage could arguably be one review_consistency.py. But they have distinct responsibilities and each is independently tested — the separation buys testability, which is worth more than a smaller file count here.

• The disposition posture is a real, acknowledged tradeoff, not a simplification miss. Accepting dispositions from any non-bot commenter on all severities (including a dismissed critical) widens a prompt-injection surface. The PR mitigates rather than eliminates it (attribution + injection guardrail + kill-switch). A stricter default (bot-author only, non-critical only) would be simpler to defend but weaker on the "human decides" principle. The choice is deliberate and documented in docs/design/review-consistency.md — reasonable, but reviewers should weigh it consciously.

• The reviewer bot flagged genuine simplicity/safety gaps that should land before merge:

  • _compare_changed_files can flip the freeze from fail-open to fail-toward-suppression when GitHub's compare files array is truncated — the freeze could then silently hide real new findings. That's the most important open issue; it undercuts the whole "never suppress still-broken code" promise.
  • The prompt-block builders (_build_dispositions_block etc.) catch bare Exception and return "". A real coding bug (renamed config key, typo) would be indistinguishable from "feature off" and would silently strip the injection guardrail from every review. Narrowing to OSError/KeyError/ValueError would make a real defect fail loudly.
  • The local git show snippet-fetch branch swallows all errors with no log, so a broken local clone silently degrades to an extra network call each time — mirror the logging the API branch already has.

Net: the architecture is close to the simplest that can actually deliver the guarantee. The remaining work is not "make it simpler" — it's closing the fail-open holes (truncated compare, bare-Exception guardrail strip) so the deterministic promises hold under adverse inputs. Those are exactly what the open CHANGES_REQUESTED review is pointing at.

One thing unclear from the truncated diff: 49 files are omitted, including the core review_runner.py and all the new modules' bodies — I'm reasoning about their behavior from the config comments, docs, spec, and review commentary, not the implementation itself. The freeze's exact fail-open logic (the flagged truncation bug) can't be verified from what's shown.

[Koan-Bot]

Simple rebase

Branch feat/review-accuracy-and-repo-context was rebased onto main — no additional changes were needed.

Stats

56 files changed, 5326 insertions(+), 83 deletions(-)

Actions performed

• Already-solved check: negative (confidence=high, reasoning=No commit on main implements spec 010's review consistency/triage/disposition features (snippet vali)
• Rebased feat/review-accuracy-and-repo-context onto upstream/main
• Pre-push CI check: previous run passed
• Force-pushed feat/review-accuracy-and-repo-context to upstream
• Private review gate passed
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

---

Automated by Skuggi

[sukria]

@sukria-koan0 explain