Fix "requested changes" reviews that name no actionable fix

src/ai/parse.ts

@@ -321,7 +321,7 @@ export function synthesizeReviewSummary(
 
 /** Shape of one comment as it arrives from the model: untyped JSON, so every
  *  field is optional and validated at runtime in parseReviewResponse. */
-interface RawComment {
+export interface RawComment {
   path?: string;
   line?: number;
   body?: string;
@@ -334,6 +334,56 @@ interface RawComment {
   confidence?: string;
 }
 
+/**
+ * Build a validated ReviewComment from an untrusted raw model comment plus the
+ * already-resolved anchor. Shared by the inline path (a real diff line), the
+ * un-anchorable-demotion path, and the PR-level path (line 0, prLevel: true) so
+ * all three produce identical body/fingerprint formatting. Assumes `c.body` is
+ * present (the caller validated it).
+ */
+export function buildReviewComment(
+  c: RawComment,
+  anchor: { path: string; line: number; prLevel: boolean },
+): ReviewComment {
+  const type = VALID_TYPES.includes(c.type as CommentType) ? (c.type as CommentType) : undefined;
+  const severity = VALID_SEVERITIES.includes(c.severity as CommentSeverity) ? (c.severity as CommentSeverity) : undefined;
+  const title = typeof c.title === "string" && c.title.trim() ? c.title.trim() : undefined;
+  const suggestion = typeof c.suggestion === "string" && c.suggestion.trim() ? c.suggestion : undefined;
+  const suggestionLanguage: "diff" | "suggestion" =
+    c.suggestionLanguage === "diff" ? "diff" : "suggestion";
+  const aiAgentPrompt = typeof c.aiAgentPrompt === "string" && c.aiAgentPrompt.trim()
+    ? c.aiAgentPrompt
+    : undefined;
+  const confidence = VALID_CONFIDENCE.includes(c.confidence as Confidence) ? (c.confidence as Confidence) : "high";
+  const fingerprint = fingerprintFor(anchor.path, anchor.line, title || c.body!.slice(0, 80));
+
+  return {
+    path: anchor.path,
+    line: anchor.line,
+    side: "RIGHT" as const,
+    body: formatCommentBody({
+      title,
+      body: c.body!,
+      type,
+      severity,
+      suggestion,
+      suggestionLanguage,
+      aiAgentPrompt,
+      fingerprint,
+      confidence,
+    }),
+    type,
+    severity,
+    title,
+    suggestion,
+    suggestionLanguage,
+    aiAgentPrompt,
+    fingerprint,
+    confidence,
+    ...(anchor.prLevel ? { prLevel: true } : {}),
+  };
+}
+
 export function parseReviewResponse(raw: string, context: PRContext): ReviewResult {
   const log = logger.child({ step: "parse" });
 
@@ -360,10 +410,11 @@ export function parseReviewResponse(raw: string, context: PRContext): ReviewResu
   }
 
   // Track how many findings we couldn't anchor (dropped) vs. snapped to a
-  // nearby valid line (remapped) so the loss is visible in logs instead of
-  // silent. See nearestAnchor for the remap rationale.
+  // nearby valid line (remapped) vs. demoted to PR-level so the loss/rescue is
+  // visible in logs instead of silent. See nearestAnchor for the remap rationale.
   let droppedCount = 0;
   let remappedCount = 0;
+  let demotedCount = 0;
   const comments: ReviewComment[] = [];
 
   // Model output is untrusted JSON, so we model an incoming comment as a loose
@@ -384,66 +435,61 @@ export function parseReviewResponse(raw: string, context: PRContext): ReviewResu
       continue;
     }
 
+    const severity = VALID_SEVERITIES.includes(c.severity as CommentSeverity) ? (c.severity as CommentSeverity) : undefined;
+
     // Anchor the finding to a real diff line: keep it as-is when it already
-    // lands on one, otherwise snap to the nearest changed line. Only when no
-    // line is close enough do we discard it.
+    // lands on one, otherwise snap to the nearest changed line. When no line is
+    // close enough we can't post it inline — but rather than losing a real
+    // blocking finding (a critical/major issue the model located imprecisely,
+    // which is exactly how diff-vs-description discrepancies present), we DEMOTE
+    // it to a PR-level finding so its substance still reaches the reviewer.
+    // Minor/trivial un-anchorable findings are still dropped: not worth the
+    // noise once they've slipped their line.
     let line = c.line;
     if (!info.valid.has(line)) {
       const anchor = nearestAnchor(line, info);
       if (anchor === null) {
-        log.warn(
-          { path: c.path, line },
-          "Comment references line not in diff with no nearby anchor, dropping",
-        );
-        droppedCount++;
+        if (severity === "critical" || severity === "major") {
+          comments.push(buildReviewComment(c, { path: c.path, line: 0, prLevel: true }));
+          demotedCount++;
+          log.info(
+            { path: c.path, line, severity },
+            "Un-anchorable blocking finding demoted to PR-level (kept, not posted inline)",
+          );
+        } else {
+          log.warn(
+            { path: c.path, line },
+            "Comment references line not in diff with no nearby anchor, dropping",
+          );
+          droppedCount++;
+        }
         continue;
       }
       log.info({ path: c.path, from: line, to: anchor }, "Remapped finding to nearest valid diff line");
       line = anchor;
       remappedCount++;
     }
 
-    const type = VALID_TYPES.includes(c.type as CommentType) ? (c.type as CommentType) : undefined;
-    const severity = VALID_SEVERITIES.includes(c.severity as CommentSeverity) ? (c.severity as CommentSeverity) : undefined;
-    const title = typeof c.title === "string" && c.title.trim() ? c.title.trim() : undefined;
-    const suggestion = typeof c.suggestion === "string" && c.suggestion.trim() ? c.suggestion : undefined;
-    const suggestionLanguage: "diff" | "suggestion" =
-      c.suggestionLanguage === "diff" ? "diff" : "suggestion";
-    const aiAgentPrompt = typeof c.aiAgentPrompt === "string" && c.aiAgentPrompt.trim()
-      ? c.aiAgentPrompt
-      : undefined;
-    const confidence = VALID_CONFIDENCE.includes(c.confidence as Confidence) ? (c.confidence as Confidence) : "high";
-    const fingerprint = fingerprintFor(c.path, line, title || c.body.slice(0, 80));
-
-    comments.push({
-      path: c.path,
-      line,
-      side: "RIGHT" as const,
-      body: formatCommentBody({
-        title,
-        body: c.body,
-        type,
-        severity,
-        suggestion,
-        suggestionLanguage,
-        aiAgentPrompt,
-        fingerprint,
-        confidence,
-      }),
-      type,
-      severity,
-      title,
-      suggestion,
-      suggestionLanguage,
-      aiAgentPrompt,
-      fingerprint,
-      confidence,
-    });
+    comments.push(buildReviewComment(c, { path: c.path, line, prLevel: false }));
+  }
+
+  // PR-level findings: the model's dedicated channel for issues not tied to a
+  // single changed line (diff contradicts the PR description, a claimed change
+  // is missing, cross-cutting concerns). No anchoring and — per the schema in
+  // ai/prompt.ts, which gives these entries no "path"/"line" — always built with
+  // path: "" so their title-based fingerprint stays stable across reviews.
+  // Non-array degrades to none.
+  const rawPrLevel: RawComment[] = Array.isArray(parsed.prLevelComments) ? parsed.prLevelComments : [];
+  let prLevelKept = 0;
+  for (const c of rawPrLevel) {
+    if (!c.body || !(typeof c.title === "string" && c.title.trim())) continue;
+    comments.push(buildReviewComment(c, { path: "", line: 0, prLevel: true }));
+    prLevelKept++;
   }
 
-  if (droppedCount > 0 || remappedCount > 0) {
+  if (droppedCount > 0 || remappedCount > 0 || demotedCount > 0 || prLevelKept > 0) {
     log.info(
-      { dropped: droppedCount, remapped: remappedCount, kept: comments.length },
+      { dropped: droppedCount, remapped: remappedCount, demoted: demotedCount, prLevel: prLevelKept, kept: comments.length },
       "Finding line validation complete",
     );
   }

src/ai/prompt.ts

@@ -21,11 +21,23 @@ You MUST respond with valid JSON matching this schema:
       "confidence": "high | medium | low"
     }
   ],
+  "prLevelComments": [
+    {
+      "title": "Single-sentence headline describing the finding (ends with a period).",
+      "body": "1-3 paragraph prose explanation. Reference identifiers/files in backticks.",
+      "type": "issue | suggestion | nitpick | documentation | security",
+      "severity": "critical | major | minor | trivial",
+      "aiAgentPrompt": "Imperative instruction to a coding agent. Name the files/symbols and say WHAT to change.",
+      "confidence": "high | medium | low"
+    }
+  ],
   "approval": "APPROVE" | "REQUEST_CHANGES" | "COMMENT"
 }
 
 Rules for the JSON response:
 - "line" must be a line number that appears in the diff (from the + side of the patch).
+- "prLevelComments" is for findings that are NOT tied to one specific changed line and therefore cannot be an inline comment. Use it — do NOT invent a line number or bury the finding only in the summary — for: the diff contradicting the PR description (a claimed change is missing, or the code does something the description doesn't mention), issues spanning many files, or concerns about the change as a whole. Each entry has NO "path"/"line". Same title/body/type/severity/aiAgentPrompt/confidence fields as inline comments. Omit the field or use [] when there are none.
+- A REQUEST_CHANGES verdict MUST be backed by at least one concrete finding — an inline "comments" entry or a "prLevelComments" entry. Never request changes while leaving both arrays empty and describing the problem only in "summary".
 - "path" must exactly match a filename from the changed files.
 - "title" is REQUIRED on every comment. One sentence, ends with a period, no markdown formatting. This becomes the bold headline.
 - "body" is the explanation BELOW the title. Do NOT repeat the title in the body.

src/drift.ts

@@ -20,7 +20,10 @@ export async function detectDescriptionDrift(opts: {
   if (desc.length < 30) {
     return [
       {
-        level: "warning",
+        // Informational, not a warning: a thin description is worth noting in
+        // the walkthrough but shouldn't be folded into an actionable PR-level
+        // finding (that folding keys off level === "warning" in reviewer.ts).
+        level: "info",
         summary: "PR description is too short to drift-check.",
         details:
           "The PR description has less than 30 characters of meaningful content. " +

src/review-body.ts

@@ -1,9 +1,9 @@
 import { randomUUID } from "node:crypto";
 import type {
   PRContext,
   ReviewComment,
   ReviewResult,
   RepoConfig,
 } from "./types.js";
 
 export type ReviewBodyMeta = {
@@ -58,14 +58,32 @@
  * documentation hints, minor issues) goes into the Nitpicks collapse.
  * Mirrors CodeRabbit's "Actionable comments posted" semantics.
  */
-function isNitpick(c: ReviewComment): boolean {
+export function isNitpick(c: ReviewComment): boolean {
   if (c.type === "nitpick" || c.type === "suggestion" || c.type === "documentation") {
     return true;
   }
   if (c.severity === "trivial" || c.severity === "minor") return true;
   return false;
 }
 
+/**
+ * A REQUEST_CHANGES verdict must be backed by at least one actionable finding
+ * (inline OR PR-level; nitpicks/suggestions don't count). When every backing
+ * finding was dropped by line-anchoring/verification — or the model requested
+ * changes while describing the problem only in the summary — return COMMENT so
+ * the verdict and the visible findings never contradict each other. Pure and
+ * one-directional: only ever relaxes a block, never creates one.
+ */
+export function reconcileApproval(
+  approval: ReviewResult["approval"],
+  comments: ReviewComment[],
+): ReviewResult["approval"] {
+  if (approval === "REQUEST_CHANGES" && !comments.some((c) => !isNitpick(c))) {
+    return "COMMENT";
+  }
+  return approval;
+}
+
 function fileHeading(path: string, count: number): string {
   return `<details>\n<summary>${path} (${count})</summary><blockquote>\n`;
 }
@@ -111,6 +129,27 @@
   return lines.join("\n");
 }
 
+/**
+ * Findings not tied to a specific changed line (prLevel) — e.g. the diff
+ * contradicts the PR description, a claimed change is missing, or a cross-cutting
+ * concern. They can't be posted as GitHub inline comments, so without this
+ * section they'd be invisible: the review would read as "changes requested / 0
+ * actionable comments" with the substance only hinted at in the summary. Each
+ * comment's `body` is already the fully-formatted standalone block (type/severity
+ * header, title, prose, agent prompt), so we render it directly.
+ */
+function renderPrLevelSection(prLevel: ReviewComment[]): string {
+  if (prLevel.length === 0) return "";
+  const blocks = prLevel.map((c) => c.body.trim()).join("\n\n---\n\n");
+  return [
+    `### 🔎 Issues not tied to a specific line (${prLevel.length})`,
+    "",
+    "<sub>These findings concern the change as a whole (for example, the diff versus the PR description) and can't be attached to a single changed line.</sub>",
+    "",
+    blocks,
+  ].join("\n");
+}
+
 function renderNitpicksSection(nitpicks: ReviewComment[]): string {
   if (nitpicks.length === 0) return "";
 
@@ -303,8 +342,16 @@
   result: ReviewResult,
   meta: ReviewBodyMeta,
 ): string {
-  const actionable = result.comments.filter((c) => !isNitpick(c));
-  const nitpicks = result.comments.filter(isNitpick);
+  // Three buckets. PR-level findings (no diff-line anchor) are split out first
+  // so they render in their own section and never leak into the inline
+  // nitpick/actionable collapses. The "posted" count spans inline actionable +
+  // actionable PR-level findings, so a blocking review whose only finding is
+  // PR-level no longer reads as "Actionable comments posted: 0".
+  const prLevel = result.comments.filter((c) => c.prLevel);
+  const inline = result.comments.filter((c) => !c.prLevel);
+  const actionable = inline.filter((c) => !isNitpick(c));
+  const nitpicks = inline.filter(isNitpick);
+  const actionableCount = actionable.length + prLevel.filter((c) => !isNitpick(c)).length;
   const runId = randomUUID();
 
   const sections: string[] = [];
@@ -316,7 +363,7 @@
     sections.push(parseFailureBanner(meta.botName));
   }
 
-  sections.push(`**Actionable comments posted: ${actionable.length}**`);
+  sections.push(`**Actionable comments posted: ${actionableCount}**`);
 
   // Show the AI/synthesized summary — but suppress it on the parse-failure path
   // when there are no findings at all, because the synthesized text there reads
@@ -328,10 +375,16 @@
     sections.push(result.summary.trim());
   }
 
+  const prLevelBlock = renderPrLevelSection(prLevel);
+  if (prLevelBlock) sections.push(prLevelBlock);
+
   const nitpicksBlock = renderNitpicksSection(nitpicks);
   if (nitpicksBlock) sections.push(nitpicksBlock);
 
-  const bulkPrompt = renderBulkAiPrompt(result.comments);
+  // Only inline comments feed the bulk agent prompt — its entries are keyed by
+  // `path`/`line`, which PR-level findings don't have (their agent prompt is
+  // already shown inline in the PR-level section above).
+  const bulkPrompt = renderBulkAiPrompt(inline);
   if (bulkPrompt) sections.push(bulkPrompt);
 
   if (result.comments.length > 0) {