fix: improve review inline/body discipline and agent reliability

mcp/readFile.ts

@@ -4,7 +4,7 @@ import type { ToolContext } from "./server.ts";
 import { execute, tool } from "./shared.ts";
 
 /** Hard cap on returned content to avoid flooding the model's context window. */
-const MAX_CHARS = 6000;
+const MAX_CHARS = 20000;
 
 export const ReadFileParams = type({
   path: type.string.describe("absolute path to the file to read"),
@@ -17,9 +17,10 @@ export function ReadFileTool(_ctx: ToolContext) {
     name: "read_file",
     description:
       "Read lines from a file. Use this to read sections of the PR diff returned by checkout_pr. " +
-      `Returns at most ${MAX_CHARS} characters — use narrow line ranges to stay within budget. ` +
+      `Returns at most ${MAX_CHARS} characters. ` +
-      "Read selectively: only the files most relevant to the review, not the entire diff at once. " +
+      "Prefer fewer, wider reads: read large contiguous ranges rather than many small ones. " +
-      "Example: `read_file({ path: diffPath, start_line: 5, end_line: 42 })`.",
+      "If the TOC lists 10 files, read 3-4 wide ranges that cover them rather than 10 separate calls. " +
+      "Example: `read_file({ path: diffPath, start_line: 5, end_line: 200 })`.",
     parameters: ReadFileParams,
     execute: execute(async ({ path, start_line, end_line }) => {
       let content: string;

mcp/review.ts

@@ -236,6 +236,11 @@ function postProcessBody(
   if (validComments.length === 0) return body;
 
   const commentedPaths = new Set(validComments.map((c) => c.path));
+  // Also match short filenames (e.g. "data-settings.tsx") since the body often
+  // uses basenames while inline comments store full repo-relative paths.
+  const commentedBasenames = new Set(
+    validComments.map((c) => c.path.split("/").pop() ?? c.path),
+  );
 
   const sectionRegex = /^### .+$/gm;
   const matches: Array<{ index: number; heading: string }> = [];
@@ -254,13 +259,19 @@ function postProcessBody(
   const toRemove: Array<{ start: number; end: number }> = [];
   for (const section of sections) {
     const content = body.slice(section.start, section.end);
+    let matched: string | undefined;
     for (const path of commentedPaths) {
-      if (content.includes(path)) {
+      if (content.includes(path)) { matched = path; break; }
-        toRemove.push({ start: section.start, end: section.end });
+    }
-        log.info(`stripped duplicate body section "${section.heading.slice(0, 80)}" — already covered by inline comment on ${path}`);
+    if (!matched) {
-        break;
+      for (const basename of commentedBasenames) {
+        if (content.includes(basename)) { matched = basename; break; }
       }
     }
+    if (matched) {
+      toRemove.push({ start: section.start, end: section.end });
+      log.info(`stripped duplicate body section "${section.heading.slice(0, 80)}" — already covered by inline comment on ${matched}`);
+    }
   }
 
   if (toRemove.length === 0) return body;

modes.ts

@@ -143,7 +143,8 @@ Inline comments are plain, no-frills anchors on the affected line:
 - **Lead with a 1-2 sentence problem statement.** The reader is looking at the line in question, so don't restate what the line says — describe what's wrong with it.
 - **Optional \`<details><summary>Technical details</summary>...</details>\` collapsible** for findings whose technical context (longer file:line references, related-code snippets, suggested approach, regression-risk notes) would overwhelm the human-readable lead-in. Same plain-markdown bold-header shape as the body technical-details block — see *Inline technical details* above. Encouraged whenever the depth helps a downstream fix-agent; don't force one when the inline lead-in already says everything.
 - **Visible portion ≤ 2-3 sentences.** If you find yourself writing more, that's the cue to split the depth into the \`Technical details\` collapsible.
-- **Multi-site findings go inline too, as ONE comment.** A finding that spans multiple files or multiple lines is still a single inline comment — anchor it to the PRIMARY causal site (the place a developer would fix first), and list the other affected sites in the \`**Affected sites:**\` section of the technical-details block. "Spans multiple files" is NOT a reason to put a finding in the body. **Never post two separate inline comments for the same logical issue** — one finding = one comment, always.
+- **Multi-site findings go inline too, as ONE comment.** A finding that spans multiple files or multiple lines is still a single inline comment — anchor it to the PRIMARY causal site (the place a developer would fix first), and list the other affected sites in the \`**Affected sites:**\` section of the technical-details block. "Spans multiple files" is NOT a reason to put a finding in the body. **Never post two separate inline comments for the same logical issue** — one finding = one comment, always. If the same root cause (e.g. the same lock key used in two methods, the same missing check in two places) shows up in two locations, pick the most important location and list the other in \`**Affected sites:**\`.
+- **No non-actionable comments.** Do not post inline comments that conclude "this is fine" or "this is acceptable" or "worth noting but OK". If something is not a finding, don't post it. Every inline comment must identify a problem the author should address.
 - **Anchor to the exact problem line.** Use the \`| newLine |\` column to find the specific line where the problematic symbol is **defined or first assigned** — not a nearby related line. If the symbol is \`isAnyPending\`, anchor to the line that defines \`isAnyPending\`, not a line that uses a different variable nearby.
 
 ## Body-wide rules