From 59e0ed8a1839bbb15828f5af1dfc26bb58730ae8 Mon Sep 17 00:00:00 2001 From: Luke Date: Wed, 8 Jul 2026 08:11:24 -0700 Subject: [PATCH 1/2] Fix "requested changes" reviews that name no actionable fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A REQUEST_CHANGES review could be submitted with "Actionable comments posted: 0" and only prose describing the problem — most often a diff-vs-PR description discrepancy, which has no diff line to anchor an inline comment to. The review schema forced every finding onto a diff line, so the model raised these in the summary + approval only, they were dropped by the line-anchoring gate (or never emitted), and nothing reconciled the verdict with the surviving comment set. Give non-line findings a first-class home and enforce the invariant that a block must be backed by a concrete finding: - types: add `prLevel` flag on ReviewComment (no diff-line anchor; excluded from inline posting by the existing line > 0 filter). - prompt: add a `prLevelComments` channel + rule that REQUEST_CHANGES must be backed by a finding, never summary-only. - parse: parse prLevelComments (no anchoring); demote un-anchorable major/critical inline findings to PR-level instead of dropping them. - review-body: render an "Issues not tied to a specific line" section and count PR-level findings toward "Actionable comments posted"; add the pure reconcileApproval helper. - reviewer: downgrade REQUEST_CHANGES -> COMMENT when no actionable finding survives; fold warning-level description drift into PR-level findings (bumps APPROVE -> COMMENT, never forces a block). - drift: reclassify the "description too short" sentinel as info so it isn't folded into an actionable finding. Adds tests/unit/pr-level-findings.test.ts. All 304 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ai/parse.ts | 140 ++++++++++------ src/ai/prompt.ts | 12 ++ src/drift.ts | 5 +- src/review-body.ts | 63 ++++++- src/reviewer.ts | 72 +++++++- src/types.ts | 7 + tests/unit/finding-verification.test.ts | 9 +- tests/unit/pr-level-findings.test.ts | 214 ++++++++++++++++++++++++ 8 files changed, 464 insertions(+), 58 deletions(-) create mode 100644 tests/unit/pr-level-findings.test.ts diff --git a/src/ai/parse.ts b/src/ai/parse.ts index 1c5e488..358b2bb 100644 --- a/src/ai/parse.ts +++ b/src/ai/parse.ts @@ -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). + */ +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,18 +435,34 @@ 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"); @@ -403,47 +470,26 @@ export function parseReviewResponse(raw: string, context: PRContext): ReviewResu 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 — line 0, path kept for + // context/fingerprint stability if the model supplied one. See the schema in + // ai/prompt.ts. 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; + const path = typeof c.path === "string" ? c.path : ""; + 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", ); } diff --git a/src/ai/prompt.ts b/src/ai/prompt.ts index c7e3365..429de62 100644 --- a/src/ai/prompt.ts +++ b/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. diff --git a/src/drift.ts b/src/drift.ts index bc1cbed..7d5d0bb 100644 --- a/src/drift.ts +++ b/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. " + diff --git a/src/review-body.ts b/src/review-body.ts index c39e709..306070c 100644 --- a/src/review-body.ts +++ b/src/review-body.ts @@ -58,7 +58,7 @@ function parseFailureBanner(botName: string): string { * 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; } @@ -66,6 +66,24 @@ function isNitpick(c: ReviewComment): boolean { 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 `
\n${path} (${count})
\n`; } @@ -111,6 +129,27 @@ function renderNitpickEntry(c: ReviewComment): string { 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})`, + "", + "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.", + "", + blocks, + ].join("\n"); +} + function renderNitpicksSection(nitpicks: ReviewComment[]): string { if (nitpicks.length === 0) return ""; @@ -303,8 +342,16 @@ export function formatReviewBody( 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 @@ export function formatReviewBody( 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 @@ export function formatReviewBody( 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) { diff --git a/src/reviewer.ts b/src/reviewer.ts index e84ca8d..46bc2f7 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -10,14 +10,14 @@ import { formatWalkthrough, formatWalkthroughInner, wrapWalkthroughCollapse, for import { parseCommand, formatHelpMessage, formatConfigMessage } from "./commands.js"; import { parseIssueCommand, formatIssueHelpMessage } from "./issue-commands.js"; import { buildIssueSummaryInstruction, buildIssuePlanInstruction } from "./ai/prompt.js"; -import { synthesizeReviewSummary } from "./ai/parse.js"; +import { synthesizeReviewSummary, fingerprintFor, renderInlineCommentBody } from "./ai/parse.js"; import { verifyFindings } from "./ai/verify.js"; import { LearningsStore, synthesizeLearning, extractFindingMeta, type FindingContext } from "./learnings.js"; import { loadGuidelines, getRelevantGuidelines, formatGuidelinesForPrompt } from "./guidelines.js"; import { parseIssueReferences, fetchLinkedIssues, formatIssuesForPrompt, formatIssuesForWalkthrough } from "./issues.js"; import { runPreMergeChecks, formatCheckResults, getOverallStatus } from "./pre-merge.js"; import { generateDocstrings, generateTests, simplifyCode, autofix } from "./finishing-touches.js"; -import { formatReviewBody } from "./review-body.js"; +import { formatReviewBody, reconcileApproval } from "./review-body.js"; import { encodeState, encodeStateRef, extractState, isTrivialPatch, WalkthroughState } from "./walkthrough-state.js"; import { assessRisk, renderRiskBlock, assessCoverage, renderCoverageBlock, shouldSuggestSplit, renderSplitSuggestion, renderConfidenceAggregate, computeReviewerDeltas, renderReviewerDeltaBlock, calibrateSeverities, resolveSeverityCalibration, renderSeverityCalibrationBlock, type CalibrationResult } from "./insights.js"; import { suggestReviewersFromBlame, renderSuggestedReviewers, combineReviewers, renderCombinedReviewers } from "./blame-reviewers.js"; @@ -935,6 +935,50 @@ export class Reviewer { } catch (err) { log.debug({ err }, "Drift detection failed"); } + + // Fold WARNING-level description drift into first-class PR-level findings. + // Diff-vs-description discrepancies are the exact class that has no diff + // line to anchor to, so they belong in the PR-level channel (visible + + // actionable) rather than buried as an info block in the walkthrough. This + // also gives the REQUEST_CHANGES invariant below a deterministic backstop. + // Non-blocking on their own: they bump APPROVE→COMMENT but never force + // REQUEST_CHANGES. Info-level drift (incl. the "description too short" + // sentinel) stays in the walkthrough block. Deduped cross-review by + // fingerprint alongside every other finding. + const driftWarnings = driftFindings.filter((f) => f.level === "warning"); + if (driftWarnings.length > 0) { + for (const f of driftWarnings) { + const title = f.summary; + const fingerprint = fingerprintFor("", 0, title); + const aiAgentPrompt = + `Reconcile the PR description with the actual diff. ${f.summary} ${f.details}`.trim(); + reviewResult.comments.push({ + path: "", + line: 0, + side: "RIGHT", + body: renderInlineCommentBody({ + title, + body: f.details || f.summary, + type: "issue", + severity: "major", + aiAgentPrompt, + fingerprint, + confidence: "medium", + }), + type: "issue", + severity: "major", + title, + aiAgentPrompt, + fingerprint, + confidence: "medium", + prLevel: true, + }); + } + if (reviewResult.approval === "APPROVE") { + reviewResult.approval = "COMMENT"; + } + log.info({ count: driftWarnings.length }, "Folded description-drift warnings into PR-level findings"); + } const risk = assessRisk({ files: context.files, review: reviewResult, @@ -1048,7 +1092,10 @@ export class Reviewer { if (calBlock) inner += "\n\n" + calBlock; const depBlock = renderDepBlock(depDeltas); if (depBlock) inner += "\n\n" + depBlock; - const driftBlock = renderDriftBlock(driftFindings); + // Warning-level drift is now surfaced as PR-level findings on the review + // (see the fold above); render only the remaining info-level drift here + // so it isn't reported twice. + const driftBlock = renderDriftBlock(driftFindings.filter((f) => f.level !== "warning")); if (driftBlock) inner += "\n\n" + driftBlock; const coachBlock = renderCommitCoachBlock(commitFindings); if (coachBlock) inner += "\n\n" + coachBlock; @@ -1246,6 +1293,25 @@ export class Reviewer { } } + // Invariant: a REQUEST_CHANGES verdict must be backed by at least one + // actionable finding (inline OR PR-level). When every backing finding was + // dropped by line-anchoring/verification — or the model blocked while + // describing the problem only in prose — the review would otherwise read + // as "changes requested / 0 actionable comments" with nothing to act on + // (the exact failure this addresses). Downgrade to COMMENT so the verdict + // and the visible findings never contradict each other. Runs AFTER every + // producer (AI, safety, pattern, static, calibration, drift) has merged + // and after cross-review dedup/suppression, so it sees the final set. + // One-directional: only ever relaxes a block, never creates one. + const reconciledApproval = reconcileApproval(reviewResult.approval, reviewResult.comments); + if (reconciledApproval !== reviewResult.approval) { + log.info( + { from: reviewResult.approval, to: reconciledApproval, totalComments: reviewResult.comments.length }, + "Downgrading REQUEST_CHANGES → COMMENT: no actionable finding survived to back the block", + ); + reviewResult.approval = reconciledApproval; + } + // If the AI didn't supply a usable summary, regenerate from the // final merged comment set so the user sees something meaningful // instead of "Review complete (no structured response from AI).". diff --git a/src/types.ts b/src/types.ts index 8792565..09173cb 100644 --- a/src/types.ts +++ b/src/types.ts @@ -277,6 +277,13 @@ export interface ReviewComment { fingerprint?: string; /** AI's self-rated confidence in this finding (default high). */ confidence?: Confidence; + /** A finding NOT tied to a specific changed line — e.g. the diff contradicts + * the PR description, a claimed change is missing, or a cross-cutting concern + * spans the whole PR. These carry title/body/severity but no meaningful + * `line` (conventionally 0), are never posted as GitHub inline comments + * (submitReview's `line > 0` filter excludes them), and are rendered in a + * dedicated "not tied to a specific line" section of the review body. */ + prLevel?: boolean; /** Set by the pattern engine so callers can record the hit source without * re-sniffing the rendered body. "builtin" = shipped heuristic; "custom" = * a `.diffsentry.yaml` anti-pattern or an admin-authored command-center rule. */ diff --git a/tests/unit/finding-verification.test.ts b/tests/unit/finding-verification.test.ts index 671244d..5e54abf 100644 --- a/tests/unit/finding-verification.test.ts +++ b/tests/unit/finding-verification.test.ts @@ -101,9 +101,14 @@ describe("parse: line remapping", () => { expect(res.comments[0].line).toBe(3); }); - it("drops a finding whose line is far from any diff line (no valid anchor)", () => { + it("demotes an un-anchorable major finding to PR-level instead of dropping it", () => { + // reviewJson emits severity "major"; a line far from any anchor can't be an + // inline comment, but a blocking finding is kept as a PR-level finding + // rather than lost. (See pr-level-findings.test.ts for the minor→drop case.) const res = parseReviewResponse(reviewJson([{ path: "src/a.ts", line: 500, title: "Hallucinated location." }]), ctx()); - expect(res.comments).toHaveLength(0); + expect(res.comments).toHaveLength(1); + expect(res.comments[0].prLevel).toBe(true); + expect(res.comments[0].line).toBe(0); }); it("drops a finding referencing an unknown file", () => { diff --git a/tests/unit/pr-level-findings.test.ts b/tests/unit/pr-level-findings.test.ts new file mode 100644 index 0000000..e6eeb90 --- /dev/null +++ b/tests/unit/pr-level-findings.test.ts @@ -0,0 +1,214 @@ +import { describe, it, expect } from "vitest"; +import { parseReviewResponse } from "../../src/ai/parse.js"; +import { formatReviewBody, reconcileApproval } from "../../src/review-body.js"; +import type { PRContext, ReviewComment, ReviewResult } from "../../src/types.js"; + +// Right-side lines: 1 (context), 2 (+), 3 (+), 4 (context), 5 (context). +// valid = {1,2,3,4,5}, changed (+) = [2,3]. +const PATCH = [ + "@@ -1,3 +1,5 @@", + " context line one", + "+added line two", + "+added line three", + " context line four", + " context line five", +].join("\n"); + +function ctx(): PRContext { + return { + owner: "o", + repo: "r", + pullNumber: 1, + title: "t", + description: "", + baseBranch: "main", + headBranch: "feat", + headSha: "deadbee", + files: [ + { filename: "src/a.ts", status: "modified", patch: PATCH, additions: 2, deletions: 0 }, + ], + }; +} + +const META = { + profile: "chill", + owner: "o", + repo: "r", + headSha: "deadbee", + baseBranch: "main", + headBranch: "feat", + filesProcessed: ["src/a.ts"], + botName: "diffsentry", +}; + +function prLevelComment(over: Partial = {}): ReviewComment { + return { + path: "", + line: 0, + side: "RIGHT", + body: "the report and review paths are swapped versus the backend URLs", + type: "issue", + severity: "major", + title: "Route mapping swaps report and review paths.", + prLevel: true, + ...over, + }; +} + +describe("parse: PR-level findings channel", () => { + it("parses prLevelComments into unanchored (line 0) prLevel findings", () => { + const raw = JSON.stringify({ + summary: "One production-breaking issue.", + approval: "REQUEST_CHANGES", + comments: [], + prLevelComments: [ + { + title: "Route mapping swaps report and review paths.", + body: "The new route map swaps the report and review paths versus the backend-generated URLs.", + type: "issue", + severity: "major", + aiAgentPrompt: "In the public routes, swap the report/review path mapping back.", + }, + ], + }); + const res = parseReviewResponse(raw, ctx()); + expect(res.comments).toHaveLength(1); + expect(res.comments[0].prLevel).toBe(true); + expect(res.comments[0].line).toBe(0); + expect(res.comments[0].title).toBe("Route mapping swaps report and review paths."); + // A prLevel finding is never posted inline (submitReview filters line > 0). + expect(res.comments[0].line).not.toBeGreaterThan(0); + }); + + it("ignores prLevelComments entries missing a title or body", () => { + const raw = JSON.stringify({ + summary: "", + approval: "COMMENT", + comments: [], + prLevelComments: [ + { body: "no title" }, + { title: "no body." }, + { title: "Good one.", body: "has both" }, + ], + }); + const res = parseReviewResponse(raw, ctx()); + expect(res.comments).toHaveLength(1); + expect(res.comments[0].title).toBe("Good one."); + }); +}); + +describe("parse: un-anchorable finding demotion", () => { + function inlineJson(severity: string, line: number): string { + return JSON.stringify({ + summary: "", + approval: "REQUEST_CHANGES", + comments: [ + { + path: "src/a.ts", + line, + title: "Finding far from the diff.", + body: "body", + type: "issue", + severity, + aiAgentPrompt: "fix it", + }, + ], + }); + } + + it("demotes an un-anchorable major finding to PR-level instead of dropping it", () => { + // line 900 is > MAX_REMAP_DISTANCE (25) from the nearest changed line (3). + const res = parseReviewResponse(inlineJson("major", 900), ctx()); + expect(res.comments).toHaveLength(1); + expect(res.comments[0].prLevel).toBe(true); + expect(res.comments[0].line).toBe(0); + }); + + it("demotes an un-anchorable critical finding to PR-level", () => { + const res = parseReviewResponse(inlineJson("critical", 900), ctx()); + expect(res.comments).toHaveLength(1); + expect(res.comments[0].prLevel).toBe(true); + }); + + it("still drops an un-anchorable minor finding (not worth the noise)", () => { + const res = parseReviewResponse(inlineJson("minor", 900), ctx()); + expect(res.comments).toHaveLength(0); + }); + + it("does not demote when the finding anchors normally", () => { + const res = parseReviewResponse(inlineJson("major", 3), ctx()); + expect(res.comments).toHaveLength(1); + expect(res.comments[0].prLevel).toBeUndefined(); + expect(res.comments[0].line).toBe(3); + }); +}); + +describe("review-body: PR-level rendering", () => { + function result(over: Partial = {}): ReviewResult { + return { summary: "One production-breaking issue.", comments: [], approval: "COMMENT", ...over }; + } + + it("renders a dedicated section and counts prLevel findings as actionable", () => { + const body = formatReviewBody(result({ comments: [prLevelComment()], approval: "REQUEST_CHANGES" }), META); + expect(body).toContain("Issues not tied to a specific line (1)"); + // renderPrLevelSection emits each finding's body verbatim. + expect(body).toContain("the report and review paths are swapped versus the backend URLs"); + // The count is no longer 0 when the only finding is PR-level — the exact + // regression this fixes. + expect(body).toContain("**Actionable comments posted: 1**"); + // PR-level findings must not leak a malformed entry (no path/line) into the + // bulk agent-prompt block. + expect(body).not.toContain("Line 0:"); + expect(body).not.toContain("In ``:"); + }); + + it("combines inline + PR-level actionable findings in the count", () => { + const inline: ReviewComment = { + path: "src/a.ts", + line: 3, + side: "RIGHT", + body: "inline issue", + type: "issue", + severity: "major", + title: "Inline problem.", + }; + const body = formatReviewBody(result({ comments: [inline, prLevelComment()] }), META); + expect(body).toContain("**Actionable comments posted: 2**"); + }); + + it("does not render the PR-level section when there are none", () => { + const body = formatReviewBody(result(), META); + expect(body).not.toContain("Issues not tied to a specific line"); + expect(body).toContain("**Actionable comments posted: 0**"); + }); +}); + +describe("reconcileApproval invariant", () => { + const issue: ReviewComment = { + path: "src/a.ts", line: 3, side: "RIGHT", body: "x", type: "issue", severity: "major", title: "T.", + }; + const nitpick: ReviewComment = { + path: "src/a.ts", line: 3, side: "RIGHT", body: "x", type: "nitpick", severity: "minor", title: "N.", + }; + + it("downgrades REQUEST_CHANGES to COMMENT when no finding survives", () => { + expect(reconcileApproval("REQUEST_CHANGES", [])).toBe("COMMENT"); + }); + + it("downgrades REQUEST_CHANGES when only nitpicks remain", () => { + expect(reconcileApproval("REQUEST_CHANGES", [nitpick])).toBe("COMMENT"); + }); + + it("keeps REQUEST_CHANGES when an inline actionable finding backs it", () => { + expect(reconcileApproval("REQUEST_CHANGES", [issue])).toBe("REQUEST_CHANGES"); + }); + + it("keeps REQUEST_CHANGES when a PR-level actionable finding backs it", () => { + expect(reconcileApproval("REQUEST_CHANGES", [prLevelComment()])).toBe("REQUEST_CHANGES"); + }); + + it("never touches APPROVE or COMMENT", () => { + expect(reconcileApproval("APPROVE", [])).toBe("APPROVE"); + expect(reconcileApproval("COMMENT", [])).toBe("COMMENT"); + }); +}); From 1bc3bcfc92ea48140300972444bf45ec7c808ded Mon Sep 17 00:00:00 2001 From: Luke Date: Wed, 8 Jul 2026 12:02:37 -0700 Subject: [PATCH 2/2] Address DiffSentry review: PR-level parsing + shared comment builder - parse.ts: prLevelComments now always build with path: "" per the ai/prompt.ts schema (which gives these entries no path/line), keeping their title-based fingerprint stable instead of reading c.path. - parse.ts: export buildReviewComment + RawComment. - reviewer.ts: drift-warning PR-level findings now go through the shared buildReviewComment instead of a hand-built ReviewComment, so body formatting and fingerprinting stay consistent with model-emitted findings. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ai/parse.ts | 14 +++++++------- src/reviewer.ts | 43 +++++++++++++++++-------------------------- 2 files changed, 24 insertions(+), 33 deletions(-) diff --git a/src/ai/parse.ts b/src/ai/parse.ts index 358b2bb..ee32f28 100644 --- a/src/ai/parse.ts +++ b/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; @@ -341,7 +341,7 @@ interface RawComment { * all three produce identical body/fingerprint formatting. Assumes `c.body` is * present (the caller validated it). */ -function buildReviewComment( +export function buildReviewComment( c: RawComment, anchor: { path: string; line: number; prLevel: boolean }, ): ReviewComment { @@ -475,15 +475,15 @@ export function parseReviewResponse(raw: string, context: PRContext): ReviewResu // 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 — line 0, path kept for - // context/fingerprint stability if the model supplied one. See the schema in - // ai/prompt.ts. Non-array degrades to none. + // 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; - const path = typeof c.path === "string" ? c.path : ""; - comments.push(buildReviewComment(c, { path, line: 0, prLevel: true })); + comments.push(buildReviewComment(c, { path: "", line: 0, prLevel: true })); prLevelKept++; } diff --git a/src/reviewer.ts b/src/reviewer.ts index 46bc2f7..f3ffa6e 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -10,7 +10,7 @@ import { formatWalkthrough, formatWalkthroughInner, wrapWalkthroughCollapse, for import { parseCommand, formatHelpMessage, formatConfigMessage } from "./commands.js"; import { parseIssueCommand, formatIssueHelpMessage } from "./issue-commands.js"; import { buildIssueSummaryInstruction, buildIssuePlanInstruction } from "./ai/prompt.js"; -import { synthesizeReviewSummary, fingerprintFor, renderInlineCommentBody } from "./ai/parse.js"; +import { synthesizeReviewSummary, buildReviewComment } from "./ai/parse.js"; import { verifyFindings } from "./ai/verify.js"; import { LearningsStore, synthesizeLearning, extractFindingMeta, type FindingContext } from "./learnings.js"; import { loadGuidelines, getRelevantGuidelines, formatGuidelinesForPrompt } from "./guidelines.js"; @@ -948,31 +948,22 @@ export class Reviewer { const driftWarnings = driftFindings.filter((f) => f.level === "warning"); if (driftWarnings.length > 0) { for (const f of driftWarnings) { - const title = f.summary; - const fingerprint = fingerprintFor("", 0, title); - const aiAgentPrompt = - `Reconcile the PR description with the actual diff. ${f.summary} ${f.details}`.trim(); - reviewResult.comments.push({ - path: "", - line: 0, - side: "RIGHT", - body: renderInlineCommentBody({ - title, - body: f.details || f.summary, - type: "issue", - severity: "major", - aiAgentPrompt, - fingerprint, - confidence: "medium", - }), - type: "issue", - severity: "major", - title, - aiAgentPrompt, - fingerprint, - confidence: "medium", - prLevel: true, - }); + // Build through the same normalizer parseReviewResponse uses so + // PR-level body formatting and title-based fingerprinting stay + // identical to model-emitted findings. + reviewResult.comments.push( + buildReviewComment( + { + title: f.summary, + body: f.details || f.summary, + type: "issue", + severity: "major", + aiAgentPrompt: `Reconcile the PR description with the actual diff. ${f.summary} ${f.details}`.trim(), + confidence: "medium", + }, + { path: "", line: 0, prLevel: true }, + ), + ); } if (reviewResult.approval === "APPROVE") { reviewResult.approval = "COMMENT";