feat(pr-review): implement compact diff handling with on-demand file inspection

[tmseidel]

No description provided.

[max-california]

🤖 Agentic Code Review

Now I have a thorough understanding of the codebase. Let me compile my review.

---

Review Summary

This PR replaces the full-diff-in-prompt approach for the agentic PR review workflow with a compact file-list summary (via new DiffSummary) and a new pr-diff on-demand tool. The change is well-scoped, well-tested, and correctly handles backward compatibility for non-review code paths. The design reduces token usage significantly and prevents context-window overflow for large PRs. No blocking issues found.

---

Findings

1. Minor: rawBlock omits the diff --git header line (DiffSummary.java:92-95)

When the inner parsing loop builds rawBlock, it starts appending after advancing i past the diff --git line. The returned hunk therefore lacks the traditional diff --git a/… b/… header. It still contains ---/+++ and all hunk content, so the LLM gets the practical information it needs, but the format is slightly non-standard. Not a blocker — just worth noting that any future consumer expecting strict diff --git headers won't find them.

// i++ moves past the "diff --git" line before the block-building loop
i++;
while (i < lines.length && !lines[i].startsWith("diff --git ")) {
    String line = lines[i];
    blockBuilder.append(line).append("\n");
    ...
}

2. Nit: pr-diff returns success=true when file not found (AgentToolRouter.java:234-239)

When the requested file path isn't in the diff, the tool returns success=true with a message listing all changed files. This is arguably a design choice — the tool ran without system error, and the message helps the LLM retry — but it's semantically misleading since the tool didn't return any diff hunks. Consider using success=false to signal to the LLM that it needs to choose a different file; the changedFiles list in the error message would still guide it.

3. Nit: ToolExecutionService.executeContextTool() has no pr-diff branch (ToolExecutionService.java:90-103)

pr-diff is registered as ToolKind.CONTEXT for both CODING and WRITER roles. In executeWriter the router intercepts it explicitly (correct). But in executeCoding it would fall through to executeContextTool, which has no case "pr-diff" and would return "Repository tool 'pr-diff' is not implemented". In practice this is harmless because coding workflows don't call pr-diff, but it's a latent inconsistency. Consider either removing Role.CODING from the tool registration or adding a defensive handler.

4. Nit: DiffSummary.statLine() says "0 files" when empty (DiffSummary.java:141-142)

For an empty diff, the output is "0 files" rather than something like "0 files changed" or "no changes". The fileTable() correctly returns "(no changes)" for this case, so there's a minor inconsistency between the two methods. Very low impact — statLine for empty diffs is only used in log messages.

5. Observation: No fallback to embedded diff for tiny PRs

For a 1-file / 3-line PR, the compact approach adds an extra tool-call round compared to the old behavior. This is a conscious trade-off and correct for the overall goal — the consistency and token savings for large PRs outweigh the minor latency for tiny ones.

---

What looks good

• DiffSummary parsing: Correctly handles new files (/dev/null), deleted files, renames (rename to), binary files, and multi-file diffs. The path resolution logic prioritises +++ → --- → rename to → header fallback, which matches git's conventions.
• Thread safety: DiffSummary and FileEntry are immutable; AgentRunContext.diffSummary is set once before the review loop runs.
• Backward compatibility: The 5-arg ToolCallContext constructor preserves existing callers (CodingAgentStrategy, WriterAgentStrategy) with diffSummary=null.
• Error handling: Null/blank diffs, missing diffSummary, and unknown file paths all produce clear, actionable error messages.
• Database migrations: Both H2 and PostgreSQL V30 scripts are idempotent (NOT EXISTS guard), selective (only bots with agentic-review workflow), and follow the existing versioning convention.
• Tests: DiffSummaryTest covers empty/null input, single/multi-file, new/deleted/renamed files, fileTable, fileDiff extraction, and pluralisation.
• Documentation: The migration guide (MIGRATION_1.14_TO_1.15.md) is thorough with verification queries, manual intervention steps, rollback instructions, and behavior comparisons.

---

Verdict

The implementation is correct, well-structured, and delivers the intended token-efficiency gains. All findings above are non-blocking nits or observations.

---

Read-only agentic review by AI Git Bot