From b36abf39d46f2457b8636b5b83726c2ea16d1f07 Mon Sep 17 00:00:00 2001 From: wolfy Date: Mon, 1 Jun 2026 12:44:57 -0500 Subject: [PATCH] fix: stop report_progress loop and reinforce review-mode output discipline --- agents/ollama.ts | 38 +++++++++++++++++++++++++++++++------- modes.ts | 19 +++++++++++++++++++ 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/agents/ollama.ts b/agents/ollama.ts index c20c316..2ee8159 100644 --- a/agents/ollama.ts +++ b/agents/ollama.ts @@ -148,6 +148,8 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { let pendingModeNudge = false; let calledOutputTool = false; let addedContinueNudge = false; + let reportProgressCount = 0; + let selectedModeName = ""; while (iterations < MAX_ITERATIONS) { iterations++; @@ -222,13 +224,16 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { "» model stopped before completing task — nudging to continue", ); addedContinueNudge = true; + const isReview = + selectedModeName === "Review" || selectedModeName === "IncrementalReview"; messages.push({ role: "user", content: "Your task is not complete yet. Continue executing the workflow — " + "call the next required tool to finish. " + - "Do not stop until you have submitted a review (create_pull_request_review) " + - "or called report_progress with a final summary.", + (isReview + ? "Do not stop until you have submitted a review via create_pull_request_review." + : "Do not stop until you have submitted a review (create_pull_request_review) or called report_progress with a final summary."), }); continue; } @@ -279,6 +284,17 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { role: "tool", content: result, }); + + // report_progress called more than once means the model is looping — + // it has no signal to stop since the tool always returns success. + if (toolName === "report_progress") { + reportProgressCount++; + if (reportProgressCount >= 2) { + log.info("» report_progress called multiple times — stopping loop"); + await unloadModel(ollama, model); + return { success: true }; + } + } } // After the FIRST select_mode call, nudge the model to act on the guidance. @@ -301,10 +317,18 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { // best-effort } - const firstStep = - selectedMode === "Review" || selectedMode === "IncrementalReview" - ? "Your first tool call must be checkout_pr with the PR number from the event context." - : "Call the first tool required by the workflow now."; + const isReviewMode = + selectedMode === "Review" || selectedMode === "IncrementalReview"; + + selectedModeName = selectedMode; + + const firstStep = isReviewMode + ? "Your first tool call must be checkout_pr with the PR number from the event context." + : "Call the first tool required by the workflow now."; + + const endCondition = isReviewMode + ? "Your ONLY valid final action is create_pull_request_review. Do NOT call report_progress — the mode workflow explicitly forbids it." + : "Execute the complete workflow step by step until you call create_pull_request_review or report_progress."; messages.push({ role: "user", @@ -312,7 +336,7 @@ async function runOllamaLoop(ctx: AgentRunContext): Promise { `Good. You have selected ${selectedMode || "a"} mode and received the workflow. ` + "Do NOT call select_mode again. " + `${firstStep} ` + - "Execute the complete workflow step by step until you call create_pull_request_review or report_progress.", + endCondition, }); } } diff --git a/modes.ts b/modes.ts index b523dd4..c15ddc0 100644 --- a/modes.ts +++ b/modes.ts @@ -64,6 +64,21 @@ state before acting on findings. Pull every metadata field from the \`checkout_pr\` tool's response — file count, commit count, base/head ref + SHA, the commit list. For \`IncrementalReview\` runs, populate \`Prior shockbot review\` with the prior review's commit_id (short SHA) and \`html_url\` from \`list_pull_request_reviews\`. +After the metadata comment, include a collapsible per-file summary table built from your file inventory. Place it in the \`body\` parameter of the structured submission, after the metadata comment and before any \`### \` issue sections: + +\`\`\` +
+Show a summary per file + +| File | Description | +| ---- | ----------- | +| \`path/to/file.ts\` | One-line description of what this file's change does | + +
+\`\`\` + +Cover every non-trivial file you inventoried. If you skipped any (lockfiles, generated files), note "(N files not reviewed)" in the \`\` tag. Do NOT put this table in the \`changes\` array — \`changes\` is for the high-level bullet list of substantive changes, not the per-file breakdown. + ## 2. Cross-cutting issue sections (zero or more) For each cross-cutting concern, one \`### \` section. Use this exact shape: @@ -300,6 +315,8 @@ 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 the file — the diff hunk is usually sufficient; read the full file when the hunk alone doesn't make the intent clear. for each file, record a one-line description: *what changed and why*. this inventory becomes the per-file table in the review body, and ensures a concrete understanding of each file before you form review opinions. read in batches to stay efficient — you do not need to read every line, just enough to understand each file's role in the PR. + 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. if the PR is **genuinely trivial**, skip the fan-out entirely and submit a \`No new issues found.\` review per step 7. @@ -456,6 +473,8 @@ ${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 the file or its diff hunk and record a one-line description: *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. 4. **prior feedback — read AND retire it**: fetch previous reviews via \`${t("list_pull_request_reviews")}\`, then call \`${t("get_review_comments")}\` on each prior shockbot review. Each thread renders as a section whose first line is a fenced tag \`comment author= id= review= thread=\`; section headers carry \`[RESOLVED]\` / \`[OUTDATED]\` when relevant. For every **open, Pullfrog-originated** thread, decide and act: