Proactively fetch file context during review

[iamnbutler]

Summary

• Adds a unified diff parser (crates/orchestrator/src/diff.rs) that extracts changed file paths and line numbers from PR diffs
• During pass 1 evaluation, proactively fetches surrounding code context (±50 lines) for the top 3 most-changed files
• Includes this context in the evaluation prompt so the reviewer can see how changes fit into the broader codebase
• Skips non-code files (binaries, lock files, images) to avoid wasting tokens

How it works

1. Diff parsing: parse_diff_files() extracts file paths, changed line numbers, and change counts from unified diffs, sorted by most changed
2. Context windowing: extract_context_window() fetches ±50 lines around each changed line, collapsing overlapping windows and inserting ... separators for gaps
3. Prompt enhancement: build_evaluation_prompt_with_context() appends an "auto-fetched file context" section to the pass 1 prompt
4. Smart limits: Max 3 files, 300 lines per file, non-code files filtered out

Benefits

• Better reviews: Reviewer sees surrounding code, not just isolated diff hunks
• Fewer false negatives: Can catch issues like "function changed but callers weren't updated"
• Reduced pass-2 needs: More context upfront means fewer deep review requests

Test plan

• All 63 orchestrator tests pass (7 new tests added)
• Full workspace builds cleanly
• Integration test with real PR evaluation (requires API keys)

Closes #668

🤖 Generated with Claude Code

[github-actions]

PR adds a diff module with a unified diff parser and context windowing, then wires it into pass 1 evaluation in ClaudeOrchestrator. The structure is clean, error handling is defensive (fetch failures are logged and skipped gracefully), and the 7 new tests cover the core parsing and windowing paths.

One correctness issue worth addressing before merge: take(MAX_CONTEXT_FILES) is applied to the sorted file list before the is_likely_non_code filter, so a PR that touches Cargo.lock + bun.lockb + a third lock file (all sorted to the top by change count) will exhaust all 3 slots on non-code files and return nothing — exactly the scenario this filter is meant to handle.

Full Review

Findings

Important

take(MAX_CONTEXT_FILES) before the non-code filter (claude.rs:231) — see inline comment. Lock file bumps are one of the most common PR types in this repo and the file list is sorted by change count descending, so lock files will frequently occupy the top 3 slots. The fix is a one-liner: move the filter before the take.

Suggestions

• pub mod diff vs pub(crate) mod diff (lib.rs:11) — the module is only consumed internally; publishing it widens the crate's API surface without benefit.
• max_lines doc comment is slightly inaccurate (diff.rs:135) — ... gap markers are appended outside the take(max_lines) budget. Minor, but the doc says "limits total output to max_lines".

What looks good

• Diff parser correctly handles the +++ b/ override for reliable path extraction (more robust than diff --git line splitting for paths containing b/).
• extract_context_window correctly uses a HashSet to deduplicate overlapping windows before sorting, avoiding repeated lines.
• Fetch failures are downgraded to warn! and skipped — no impact on pass 1 when context is unavailable.
• build_evaluation_prompt_with_context delegates to the existing build_evaluation_prompt and appends, keeping backward compatibility clean.
• Test coverage for empty diff, multiple files, gap separators, and empty changed lines.

---

References:

• §23808815227

Reviewed by PR / Review

[github-actions]

[IMPORTANT]

Priority: Correctness

take(MAX_CONTEXT_FILES) is applied before the is_likely_non_code filter. If the top 3 most-changed files are all non-code (e.g., a PR that bumps Cargo.lock + bun.lockb + package-lock.yaml and also touches Rust source files), the take(3) exhausts the slots on the lock files and the actual code files at positions 4+ never get context. The feature silently produces nothing useful in this common case.

Fix: filter before capping.

for file in changed_files
    .iter()
    .filter(|f| !is_likely_non_code(&f.path))
    .take(MAX_CONTEXT_FILES)
{
    // ...
}

[github-actions]

[SUGGESTION]

Priority: Architecture

diff is a pure internal module — only used by claude.rs — but it's declared pub mod diff, which exposes DiffFile, parse_diff_files, extract_context_window, and the constants as part of the crate's public API. Unless there's an intent for other crates to reuse the diff parser, pub(crate) mod diff would be more appropriate.

[github-actions]

[SUGGESTION]

Priority: Correctness (minor)

sorted_lines.iter().take(max_lines) limits the number of source lines emitted, but the "..." gap separators are appended on top of that count. With MAX_CONTEXT_LINES_PER_FILE = 300 and many disjoint hunks, the actual output can exceed 300 lines by the number of gap markers. Not a big deal given the LLM-context framing, but the doc comment says "limits total output to max_lines" which is slightly inaccurate.