feat(T23): update code review code logics

packages/pr-review/src/constants.ts

@@ -1,5 +1,15 @@
-// Model used for the review reasoning. Sonnet balances code-reasoning quality and cost;
+// Model used for the review reasoning. Haiku 4.5 is the most cost-efficient model
+// ($1 / $5 per 1M tokens); we deliberately trade some review depth for cost here.
 export const REVIEW_LLM = 'claude-haiku-4-5'
 
-// Caps how many times the agent may read/search context before it must produce a verdict.
-export const MAX_REVIEW_TOOL_CALLS = 30
+// Caps how many times the agent may read/search context before it must produce a
+// verdict. This is the primary bound on worst-case token spend: the agent re-sends
+// the whole message history on every turn, so the last calls are the most expensive
+// ones — keep this tight.
+export const MAX_REVIEW_TOOL_CALLS = 18
+
+// Bounds on individual tool outputs. Tool results are re-sent on every subsequent
+// model turn, so capping their size compounds across the whole review.
+export const MAX_READ_LINES = 400 // max lines returned by read_file in one call
+export const MAX_PATCH_LINES = 300 // max patch lines per file in get_pr_diff
+export const MAX_SEARCH_MATCHES = 30 // max matches returned by search_code

packages/pr-review/src/pr-review.agent.ts

@@ -1,13 +1,20 @@
 import { createAgent, toolCallLimitMiddleware } from 'langchain'
 import { ChatAnthropic } from '@langchain/anthropic'
-import { AIMessage } from '@langchain/core/messages'
+import { AIMessage, SystemMessage } from '@langchain/core/messages'
 import { getPrDiffTool, readFileTool, searchCodeTool } from './pr-review.tool.js'
 import { PR_REVIEW_PROMPT } from './prompt.js'
 import { REVIEW_LLM, MAX_REVIEW_TOOL_CALLS } from './constants.js'
 import { ReviewResultSchema, type ReviewResult } from './schemas.js'
 import { parsePrUrl } from './pr-url.js'
 
-const llm = new ChatAnthropic({ model: REVIEW_LLM, temperature: 0 })
+// `cache_control` is a per-call option that makes ChatAnthropic place a prompt-cache breakpoint
+// on the last block of every request. In an agent loop that re-sends the full message history
+// each turn, this caches the growing conversation prefix incrementally — later turns read the
+// prior diff and file contents at ~0.1x input price instead of reprocessing them at full rate.
+// We attach it via withConfig (the @langchain/core 1.x replacement for bind); createAgent still
+// binds the tools onto the underlying model and merges this option through (see langchain's
+// _simpleBindTools, which preserves the RunnableBinding's kwargs/config when binding tools).
+const llm = new ChatAnthropic({ model: REVIEW_LLM, temperature: 0 }).withConfig({ cache_control: { type: 'ephemeral' } })
 
 // The reviewer loops: read the diff, pull in context with read_file/search_code, then judge.
 // The tool-call limit caps that loop.
@@ -17,10 +24,15 @@ const llm = new ChatAnthropic({ model: REVIEW_LLM, temperature: 0 })
 // concurrent writes to the internal `jumpTo` channel (INVALID_CONCURRENT_GRAPH_UPDATE).
 // Instead the agent returns a plain-text review, and we convert it to the typed
 // ReviewResult in a separate withStructuredOutput call below.
+//
+// The systemPrompt is passed as a SystemMessage with a cache_control breakpoint so the
+// (stable) system prompt + tool definitions are cached once and reused on every turn.
 export const prReviewAgent = createAgent({
   model: llm,
   tools: [getPrDiffTool, readFileTool, searchCodeTool],
-  systemPrompt: PR_REVIEW_PROMPT,
+  systemPrompt: new SystemMessage({
+    content: [{ type: 'text', text: PR_REVIEW_PROMPT, cache_control: { type: 'ephemeral' } }],
+  }),
   middleware: [toolCallLimitMiddleware({ runLimit: MAX_REVIEW_TOOL_CALLS, exitBehavior: 'end' })],
 })
 
@@ -48,14 +60,20 @@ export async function reviewPullRequest(prUrl: string): Promise<ReviewResult> {
 
   console.log(`🔍 Reviewing ${owner}/${repo} PR #${number} ...\n`)
 
-  const result = await prReviewAgent.invoke({
-    messages: [
-      {
-        role: 'user',
-        content: `Review this pull request. owner="${owner}", repo="${repo}", prNumber=${number}. Start by calling get_pr_diff, then investigate the surrounding code before reporting any issues.`,
-      },
-    ],
-  })
+  // LangGraph's default recursionLimit (25 super-steps ≈ 12 tool round-trips) is lower than
+  // our tool-call budget, so without this the graph throws GRAPH_RECURSION_LIMIT before the
+  // toolCallLimitMiddleware can gracefully end the run. 2 * N + 1 leaves room for N tool calls.
+  const result = await prReviewAgent.invoke(
+    {
+      messages: [
+        {
+          role: 'user',
+          content: `Review this pull request. owner="${owner}", repo="${repo}", prNumber=${number}. Start by calling get_pr_diff, then investigate the surrounding code before reporting any issues.`,
+        },
+      ],
+    },
+    { recursionLimit: 2 * MAX_REVIEW_TOOL_CALLS + 1 }
+  )
 
   const reviewText = finalMessageText(result.messages)
 

packages/pr-review/src/pr-review.tool.ts

@@ -6,9 +6,19 @@
 import { realpathSync } from 'node:fs'
 import path from 'node:path'
 import { z } from 'zod'
+import { MAX_READ_LINES, MAX_PATCH_LINES, MAX_SEARCH_MATCHES } from './constants.js'
 
 const execFileAsync = promisify(execFile)
 
+// Trim an oversized diff patch. Patches ride along in every subsequent model turn, so a huge
+// generated-file diff would be re-sent dozens of times. The agent can read_file for full context.
+function truncatePatch(patch: string): string {
+  const lines = patch.split('\n')
+  if (lines.length <= MAX_PATCH_LINES) return patch
+  const omitted = lines.length - MAX_PATCH_LINES
+  return `${lines.slice(0, MAX_PATCH_LINES).join('\n')}\n… (patch truncated: ${omitted} more line(s) — read the file directly if you need the rest)`
+}
+
 const octokit = new Octokit({ auth: process.env.GITHUB_TOKEN })
 
 // Resolve the local clone root once. Every file/search tool is sandboxed to this directory.
@@ -62,7 +72,7 @@
         status: f.status,
         additions: f.additions,
         deletions: f.deletions,
-        patch: f.patch ?? '(no textual diff — binary or too large)',
+        patch: f.patch ? truncatePatch(f.patch) : '(no textual diff — binary or too large)',
       })),
     }
 
@@ -76,11 +86,14 @@
 export const readFileTool = new DynamicStructuredTool({
   name: 'read_file',
   description:
-    'Reads a file from the local checkout of the repository so you can inspect the surrounding code (callers, definitions, types) before deciding whether a change is a real issue. Path is relative to the repo root.',
+    'Reads a file from the local checkout so you can inspect surrounding code (callers, definitions, types) before deciding whether a change is a real issue. Path is relative to the repo root. Files are returned at most ' +
+    `${MAX_READ_LINES} lines at a time — for larger files, read a targeted window with offset/limit (the diff already tells you which lines changed) instead of pulling the whole file.`,
   schema: z.object({
     path: z.string().describe('Repo-relative path of the file to read, e.g. "src/utils/parser.ts"'),
+    offset: z.number().int().min(1).optional().describe('1-based line number to start reading from. Use to jump to a specific region.'),
+    limit: z.number().int().min(1).optional().describe(`Max number of lines to return (default and hard cap: ${MAX_READ_LINES}).`),
   }),
-  func: async ({ path: relativePath }) => {
+  func: async ({ path: relativePath, offset, limit }) => {
     const absolutePath = resolveInsideRepo(relativePath)
 
     let contents: string
@@ -91,15 +104,24 @@
       return JSON.stringify({ path: relativePath, error: 'File not found or unreadable in the local clone.' })
     }
 
-    // Prefix line numbers so the agent can cite accurate file:line references.
-    const numbered = contents
-      .split('\n')
-      .map((line, i) => `${i + 1}\t${line}`)
+    const allLines = contents.split('\n')
+    const totalLines = allLines.length
+    const start = offset && offset > 0 ? offset : 1
+    const count = Math.min(limit ?? MAX_READ_LINES, MAX_READ_LINES)
+    const end = Math.min(start - 1 + count, totalLines)
+
+    // Prefix absolute line numbers so the agent can cite accurate file:line references.
+    const numbered = allLines
+      .slice(start - 1, end)
+      .map((line, i) => `${start + i}\t${line}`)
       .join('\n')
 
-    console.log(`✅ read_file: ${relativePath} (${contents.split('\n').length} lines)`)
+    // Tell the agent when there's more to read, so it can request another window if needed.
+    const note = end < totalLines || start > 1 ? `Showing lines ${start}-${end} of ${totalLines}. Use offset/limit to read another range.` : undefined
+
+    console.log(`✅ read_file: ${relativePath} (lines ${start}-${end} of ${totalLines})`)
 
-    return JSON.stringify({ path: relativePath, contents: numbered })
+    return JSON.stringify({ path: relativePath, totalLines, startLine: start, endLine: end, contents: numbered, ...(note ? { note } : {}) })
   },
 })
 
@@ -126,7 +148,7 @@
       const matches = stdout
         .split('\n')
         .filter(Boolean)
-        .slice(0, 100)
+        .slice(0, MAX_SEARCH_MATCHES)
         .map((line) => line.replace(`${root}${path.sep}`, ''))
 
       console.log(`✅ search_code: "${query}" → ${matches.length} match(es)`)

packages/pr-review/src/prompt.ts

@@ -2,9 +2,14 @@ export const PR_REVIEW_PROMPT = `You are a senior code reviewer with deep expert
 
 You have three tools:
 - get_pr_diff — fetch the PR's changed files and diffs. Call this FIRST.
-- read_file — read any file from the local checkout (callers, definitions, types, config).
+- read_file — read a file (or a targeted line range via offset/limit) from the local checkout (callers, definitions, types, config).
 - search_code — find where a symbol is defined or used across the codebase.
 
+Be economical with tools — each result is re-sent on every later step, so wasted calls are expensive:
+- Read targeted line ranges (offset/limit) instead of whole files; the diff already tells you which lines changed.
+- Never re-read a file or re-run a search you have already done.
+- Search for precise symbols, not broad terms, and only investigate changes substantial enough to plausibly hide a bug.
+
 How to review — investigate before judging:
 1. Call get_pr_diff to see exactly what changed.
 2. For each meaningful change, DO NOT judge from the diff alone. Pull in context first: