From fb32b978575c1c18d8a827e7c4ec53f8c5a8bb57 Mon Sep 17 00:00:00 2001 From: wolfy Date: Mon, 1 Jun 2026 14:05:43 -0500 Subject: [PATCH] fix: accumulate all diff reads, intercept report_progress in review mode --- agents/ollama.ts | 91 +++++++++++++++++++++++++++++------------------- modes.ts | 4 +-- 2 files changed, 57 insertions(+), 38 deletions(-) diff --git a/agents/ollama.ts b/agents/ollama.ts index 61fa1c9..c6cdeb9 100644 --- a/agents/ollama.ts +++ b/agents/ollama.ts @@ -150,11 +150,12 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { let addedContinueNudge = false; let reportProgressCount = 0; let selectedModeName = ""; - // Accumulates content previews from read_file calls so that when tool - // messages are pruned under context pressure, the model retains a compact - // summary of what it already read. User messages survive pruning. - const readFileLog = new Map(); // path → content excerpt + // Accumulates content from each read_file call (all ranges) so that when + // tool messages are pruned, the model retains the diff content it read. + // Keyed by "path:start:end" so multiple ranges of the same file are kept. + const readFileLog: string[] = []; let injectedFileReadSummary = false; + let editCommentCount = 0; while (iterations < MAX_ITERATIONS) { iterations++; @@ -210,21 +211,22 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { ); if (promptTokens > numCtx * 0.77) { messages = pruneToolMessages(messages); - // Inject a compact summary of all files read so far as a user message - // so the model retains the context even after tool messages are pruned. + // Inject accumulated diff content as a user message (survives pruning). // Only inject once — subsequent prune events leave this message intact. - if (readFileLog.size > 0 && !injectedFileReadSummary) { + if (readFileLog.length > 0 && !injectedFileReadSummary) { injectedFileReadSummary = true; - const entries = [...readFileLog.entries()] - .map(([p, c]) => `### ${p}\n${c}`) - .join("\n\n"); + const combined = readFileLog.join("\n---\n").slice(0, 10000); + const isReview = + selectedModeName === "Review" || selectedModeName === "IncrementalReview"; messages.push({ role: "user", content: - `The following files were read earlier but their content was pruned from context. ` + - `Use these excerpts as your understanding of each file when writing the review:\n\n${entries}`, + `Diff content read earlier (preserved after context pruning):\n\n${combined}\n\n` + + (isReview + ? "Now call create_pull_request_review with your review findings. Do NOT call report_progress." + : "Continue the workflow using the content above."), }); - log.info(`» injected file-read summary: ${readFileLog.size} file(s)`); + log.info(`» injected file-read summary: ${readFileLog.length} read(s)`); } } } @@ -306,23 +308,18 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { content: result, }); - // Track read_file calls so we can reconstruct context after pruning. - // read_file returns { content: "..." } JSON — parse out the actual content. - // Promote args logging to info so we can diagnose the path key format. + // Track every read_file call — accumulate all ranges (same path, different + // start/end lines) since the model reads the diff file in multiple chunks. + // read_file returns { content: "..." } JSON — extract the actual content. if (toolName === "read_file") { - const args = toolArgs as Record; - log.info(` read_file args keys: ${Object.keys(args).join(", ")} | path type: ${typeof args.path} | path: ${String(args.path).slice(0, 120)}`); - const path = typeof args.path === "string" ? args.path : undefined; - if (path && !readFileLog.has(path)) { - let excerpt: string; - try { - const parsed = JSON.parse(result) as { content?: string }; - excerpt = (parsed.content ?? result).slice(0, 500); - } catch { - excerpt = result.slice(0, 500); - } - readFileLog.set(path, excerpt); + let excerpt: string; + try { + const parsed = JSON.parse(result) as { content?: string }; + excerpt = (parsed.content ?? result).slice(0, 500); + } catch { + excerpt = result.slice(0, 500); } + readFileLog.push(excerpt); } // After checkout_pr returns, extract and pin the diffPath as a user @@ -338,10 +335,10 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { role: "user", content: `The diff file is at: ${dp}\n\n` + - `Read each file's diff by calling read_file on this path with the line range from the TOC.\n` + - `Example — for "device.ts (163 lines, 1146-1308)": read_file(path="${dp}", offset=1146, limit=163)\n` + + `Read each file's diff by calling read_file on this path with start_line/end_line from the TOC.\n` + + `Example — for "device.ts (163 lines, 1146-1308)": read_file(path="${dp}", start_line=1146, end_line=1308)\n` + `IMPORTANT: Do NOT call read_file on source files (apps/..., packages/...). ` + - `Use read_file on the diff file path above with the TOC ranges.`, + `Use read_file on the diff file path above with the TOC line ranges.`, }); log.info(`» pinned diffPath to context: ${dp}`); } @@ -350,12 +347,34 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { } } - // report_progress called more than once means the model is looping — - // it has no signal to stop since the tool always returns success. + // report_progress is forbidden in Review/IncrementalReview — intercept it. + // In other modes, stop after 2+ calls (loop detection). if (toolName === "report_progress") { - reportProgressCount++; - if (reportProgressCount >= 2) { - log.info("» report_progress called multiple times — stopping loop"); + const isReview = + selectedModeName === "Review" || selectedModeName === "IncrementalReview"; + if (isReview) { + log.info("» report_progress intercepted in Review mode — redirecting"); + messages.push({ + role: "user", + content: + "report_progress is not allowed in Review mode. " + + "Call create_pull_request_review now with your findings.", + }); + } else { + reportProgressCount++; + if (reportProgressCount >= 2) { + log.info("» report_progress loop — stopping"); + await unloadModel(ollama, model); + return { success: true }; + } + } + } + + // edit_issue_comment called repeatedly means the model is stuck looping. + if (toolName === "edit_issue_comment") { + editCommentCount++; + if (editCommentCount >= 2) { + log.info("» edit_issue_comment loop — stopping"); await unloadModel(ollama, model); return { success: true }; } diff --git a/modes.ts b/modes.ts index ddab217..ccb03df 100644 --- a/modes.ts +++ b/modes.ts @@ -315,7 +315,7 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`, 2. **checkout**: call \`${t("checkout_pr")}\` — this returns PR metadata and a \`diffPath\`. read the diff TOC end-to-end and treat its file line ranges as your coverage checklist. - **file inventory**: for every non-trivial file in the diff TOC (skip lockfiles, \`*.lock\`, auto-generated migration checksums, formatter/linter configs with no logic changes), read its diff hunk by calling \`read_file\` on \`diffPath\` using the TOC line range (e.g. if the TOC shows \`foo.ts (lines 42-100)\`, call \`read_file\` on \`diffPath\` with offset 41, limit 59). **Do NOT read full source files one-by-one — that exhausts the context window on large PRs.** Only call \`read_file\` on the actual source file when the diff hunk is insufficient to understand broader context. For each file record a one-line description: *what changed and why*. This inventory becomes the per-file table in the review body. + **file inventory**: for every non-trivial file in the diff TOC (skip lockfiles, \`*.lock\`, auto-generated migration checksums, formatter/linter configs with no logic changes), read its diff hunk by calling \`read_file\` on \`diffPath\` with \`start_line\`/\`end_line\` from the TOC (e.g. if the TOC shows \`foo.ts (lines 42-100)\`, call \`read_file(path=diffPath, start_line=42, end_line=100)\`). **Do NOT read full source files one-by-one — that exhausts the context window on large PRs.** Only call \`read_file\` on the actual source file when the diff hunk is insufficient to understand broader context. For each file record a one-line description: *what changed and why*. This inventory becomes the per-file table in the review body. 3. **triage**: orient yourself on the PR — identify *what kind of thing this is* (domain it touches, seams it crosses, external contracts it depends on, user-facing surfaces it changes). pull as much context as you need to render a confident, well-grounded review: read related files, grep for callers of changed symbols, check tests that exercise the touched paths, fetch related GitHub state. **you are the synthesizer** — never delegate understanding to subagents. @@ -473,7 +473,7 @@ ${PR_SUMMARY_FORMAT}`, 2. **checkout**: call \`${t("checkout_pr")}\` — this returns PR metadata, \`diffPath\` (full diff), and \`incrementalDiffPath\` (changes since last reviewed version, if available). read the diff TOC first and use its line ranges as your coverage checklist. - **file inventory**: for every non-trivial file in the incremental diff TOC (skip lockfiles, \`*.lock\`, auto-generated migration checksums, formatter/linter configs with no logic changes), read its diff hunk by calling \`read_file\` on \`diffPath\` (or \`incrementalDiffPath\` if present) using the TOC line range. **Do NOT read full source files one-by-one — that exhausts the context window.** Only read the actual source file when the diff hunk is insufficient to understand broader context. Record a one-line description per file: *what changed and why*. This inventory becomes the per-file table in the review body. + **file inventory**: for every non-trivial file in the incremental diff TOC (skip lockfiles, \`*.lock\`, auto-generated migration checksums, formatter/linter configs with no logic changes), read its diff hunk by calling \`read_file\` on \`diffPath\` (or \`incrementalDiffPath\` if present) with \`start_line\`/\`end_line\` from the TOC. **Do NOT read full source files one-by-one — that exhausts the context window.** Only read the actual source file when the diff hunk is insufficient to understand broader context. Record a one-line description per file: *what changed and why*. This inventory becomes the per-file table in the review body. 3. **incremental scope**: if \`incrementalDiffPath\` is present, read it to see what changed since the last review. this is a range-diff that isolates the net changes, filtering out base branch noise. if not present, fall back to reviewing the full PR diff and determine what changed since Pullfrog's most recent review.