From c206bab9b79533b9f9492ebe0bfaa1333fa965c1 Mon Sep 17 00:00:00 2001 From: damengrandom Date: Mon, 8 Jun 2026 18:25:25 +1000 Subject: [PATCH] feat(T23): update code review code logics --- packages/pr-review/src/constants.ts | 16 +++++++-- packages/pr-review/src/pr-review.agent.ts | 40 +++++++++++++++------ packages/pr-review/src/pr-review.tool.ts | 42 +++++++++++++++++------ packages/pr-review/src/prompt.ts | 7 +++- 4 files changed, 80 insertions(+), 25 deletions(-) diff --git a/packages/pr-review/src/constants.ts b/packages/pr-review/src/constants.ts index 29933ba..56ee950 100644 --- a/packages/pr-review/src/constants.ts +++ b/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 diff --git a/packages/pr-review/src/pr-review.agent.ts b/packages/pr-review/src/pr-review.agent.ts index c372d35..e9c67c8 100644 --- a/packages/pr-review/src/pr-review.agent.ts +++ b/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 { 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) diff --git a/packages/pr-review/src/pr-review.tool.ts b/packages/pr-review/src/pr-review.tool.ts index 55ec827..5f512fb 100644 --- a/packages/pr-review/src/pr-review.tool.ts +++ b/packages/pr-review/src/pr-review.tool.ts @@ -6,9 +6,19 @@ import { readFile } from 'node:fs/promises' 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 @@ export const getPrDiffTool = new DynamicStructuredTool({ 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 getPrDiffTool = new DynamicStructuredTool({ 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 @@ export const readFileTool = new DynamicStructuredTool({ 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 @@ export const searchCodeTool = new DynamicStructuredTool({ 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)`) diff --git a/packages/pr-review/src/prompt.ts b/packages/pr-review/src/prompt.ts index cce20c1..76e0cc9 100644 --- a/packages/pr-review/src/prompt.ts +++ b/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: