diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 4a93268..72524e6 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -20,6 +20,7 @@ import { mapFindingsToReview, type ReviewComment, } from "../review/comment-mapping.js" +import { filterNonFindings } from "../review/filter-non-findings.js" import { selectFindings } from "../review/select-findings.js" import { renderCostSummary } from "../openrouter/cost-summary.js" import { @@ -29,6 +30,7 @@ import { type GenerateFindings, type ReviewContext, } from "../orchestrate.js" +import { makeFinding } from "../review/__tests__/make-finding.js" import { createTestLogger } from "./test-logger.js" const sampleDiff = readFileSync( @@ -652,6 +654,79 @@ describe("orchestrate", () => { expect(stubs.fetchPullRequestCalls).toHaveLength(0) }) + + it("filters non-findings from LLM output", async () => { + const nonFinding = makeFinding({ + line: 3, + failure_scenario: "N/A — this is correct behavior.", + }) + const realFinding = makeFinding({ + line: 145, + failure_scenario: + 'register(" ", "value") succeeds and the entry is orphaned.', + }) + const mixedResponse: ReviewResponse = { + analysis: "checked", + findings: [nonFinding, realFinding], + } + + const { findings: mixedFiltered } = filterNonFindings( + mixedResponse.findings, + ) + const mixedSelection = selectFindings({ + findings: mixedFiltered, + severityThreshold: "low", + maxFindings: undefined, + }) + const mixedMapped = mapFindingsToReview({ + findings: mixedSelection.selected, + commentableByPath: fixtureCommentableByPath, + }) + const mixedBody = buildReviewBody({ + bodyFindings: mixedMapped.bodyFindings, + droppedByCap: mixedSelection.droppedByCap, + model: "test/model", + inlineCommentCount: mixedMapped.comments.length, + }) + const mixedFallbackBody = buildReviewBody({ + bodyFindings: mixedSelection.selected, + droppedByCap: mixedSelection.droppedByCap, + model: "test/model", + bodyFindingsHeading: "Findings", + bodyFindingsDescription: + "Inline comments were unavailable; all findings are listed here:", + }) + + const stubs = makeOrchestrateDeps({ + fixtureResult: { review: mixedResponse }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result).toEqual({ + findingsCount: 1, + reviewUrl: "https://github.com/test/review/1", + modelUsed: "test/model", + skippedReason: "", + costSummaryMarkdown: expectedCostSummary, + }) + + expect(stubs.submitReviewCalls).toHaveLength(1) + expect(first(stubs.submitReviewCalls)).toEqual({ + prNumber: fixturePrContext.prNumber, + commitId: fixturePrContext.headSha, + body: mixedBody, + comments: mixedMapped.comments, + fallbackBody: mixedFallbackBody, + }) + + expect(logger.messages).toContainEqual({ + level: "info", + message: "filtered non-findings", + data: { droppedAsNonFinding: 1 }, + }) + }) }) }) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index f2b25e7..e501819 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -30,6 +30,7 @@ import { generateDelimiterNonce, type PromptFile, } from "./review/prompt.js" +import { filterNonFindings } from "./review/filter-non-findings.js" import { selectFindings } from "./review/select-findings.js" export type ReviewContext = { @@ -245,9 +246,16 @@ export const orchestrate = async ( const { modelUsed, attempts } = structuredResult - // Step 12: select findings + // Step 12: filter non-findings before selection so cap slots aren't wasted + const { findings: realFindings, droppedAsNonFinding } = filterNonFindings( + structuredResult.review.findings, + ) + if (droppedAsNonFinding > 0) { + logger.info("filtered non-findings", { droppedAsNonFinding }) + } + const { selected, droppedByCap } = selectFindings({ - findings: structuredResult.review.findings, + findings: realFindings, severityThreshold, maxFindings: config.maxFindings, }) diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index b3ce9ad..86ab70b 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -215,11 +215,23 @@ The guard rejects only the exact empty string. }) expect(mapped.comments[0]?.body).toContain( - "```diff\n-old line\n+new line\n```", + "
\nSuggested fix\n\n```diff\n-old line\n+new line\n```\n\n
", ) expect(mapped.comments[1]?.body).not.toContain("```diff") }) + it("omits the suggestion block for an empty-string suggestion", () => { + const finding = makeFinding({ line: 145, suggestion: "" }) + + const mapped = mapFindingsToReview({ + findings: [finding], + commentableByPath: makeCommentableByPath(), + }) + + expect(mapped.comments[0]?.body).not.toContain("
") + expect(mapped.comments[0]?.body).not.toContain("```diff") + }) + it("sizes the suggestion fence beyond any backtick run inside the suggestion", () => { const fencedSuggestion = "```md\n-old fence\n+new fence\n```" const finding = makeFinding({ line: 145, suggestion: fencedSuggestion }) @@ -230,7 +242,7 @@ The guard rejects only the exact empty string. }) expect(mapped.comments[0]?.body).toContain( - `\`\`\`\`diff\n${fencedSuggestion}\n\`\`\`\``, + `
\nSuggested fix\n\n\`\`\`\`diff\n${fencedSuggestion}\n\`\`\`\`\n\n
`, ) }) }) @@ -282,7 +294,26 @@ _1 lower-severity finding(s) omitted by the max_findings cap: \`src/greeter.ts:5 model: "anthropic/claude-sonnet-4-6", }) - expect(body).toContain("```diff\n-old line\n+new line\n```") + expect(body).toBe(`### Findings beyond the diff + +These are in code the changes touch or depend on, outside the diff's line ranges: + +- **[medium/correctness]** Whitespace-only keys pass the empty-key guard — \`src/untouched.ts:30\` _(confidence: high)_ + The guard rejects only the exact empty string. + **Failure scenario:** register(" ", "value") succeeds and the entry is orphaned. + +
+Suggested fix + +\`\`\`diff +-old line ++new line +\`\`\` + +
+ +--- +*umm-actually · anthropic/claude-sonnet-4-6*`) }) it("renders only attribution when there is nothing beyond the diff and no cap drops", () => { diff --git a/src/review/__tests__/filter-non-findings.test.ts b/src/review/__tests__/filter-non-findings.test.ts new file mode 100644 index 0000000..2581280 --- /dev/null +++ b/src/review/__tests__/filter-non-findings.test.ts @@ -0,0 +1,183 @@ +import { describe, expect, it } from "vitest" +import { filterNonFindings } from "../filter-non-findings.js" +import { makeFinding } from "./make-finding.js" + +describe("filterNonFindings", () => { + // --- Prefix patterns that should be dropped --- + + it("drops a finding whose failure_scenario starts with 'N/A'", () => { + const finding = makeFinding({ + failure_scenario: "N/A — tests are valid.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario starts with 'n/a' (case-insensitive)", () => { + const finding = makeFinding({ + failure_scenario: "n/a — Zod validation catches malformed responses.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario starts with 'NA' (no slash)", () => { + const finding = makeFinding({ + failure_scenario: "NA — the code handles this edge case.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario starts with 'Not applicable'", () => { + const finding = makeFinding({ + failure_scenario: "Not applicable — the code handles this edge case.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario starts with 'Placeholder'", () => { + const finding = makeFinding({ + failure_scenario: "Placeholder — re-evaluating.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + // --- Passthrough: real findings must not be dropped --- + + it("keeps a finding with a concrete failure scenario", () => { + const finding = makeFinding({ + failure_scenario: + "A CI step that gates deployment reads the env var, but the value is never set in the matrix.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [finding], droppedAsNonFinding: 0 }) + }) + + it("keeps a finding that mentions 'by design' as a qualifier", () => { + const finding = makeFinding({ + failure_scenario: + "The function returns null by design, but the caller never null-checks.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [finding], droppedAsNonFinding: 0 }) + }) + + it("keeps a finding that mentions 'this is correct' as a qualifier", () => { + const finding = makeFinding({ + failure_scenario: + "This is correct for ASCII but breaks on multi-byte UTF-8.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [finding], droppedAsNonFinding: 0 }) + }) + + it("keeps a finding starting with 'None of'", () => { + const finding = makeFinding({ + failure_scenario: "None of the guards catch this input.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [finding], droppedAsNonFinding: 0 }) + }) + + it("keeps all findings when none are non-findings", () => { + const findings = [ + makeFinding({ + line: 1, + failure_scenario: + 'register(" ", "value") succeeds and the entry is orphaned.', + }), + makeFinding({ + line: 10, + failure_scenario: + "A user configures the threshold to 'extreme' and the action crashes.", + }), + ] + + const result = filterNonFindings(findings) + + expect(result).toEqual({ findings, droppedAsNonFinding: 0 }) + }) + + // --- Mixed and edge cases --- + + it("returns empty findings when all findings are non-findings", () => { + const findings = [ + makeFinding({ line: 1, failure_scenario: "N/A — tests are valid." }), + makeFinding({ + line: 2, + failure_scenario: "Not applicable — already handled.", + }), + ] + + const result = filterNonFindings(findings) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 2 }) + }) + + it("filters selectively in a mixed set of findings", () => { + const realFinding = makeFinding({ + line: 1, + failure_scenario: + 'register(" ", "value") succeeds and the entry is orphaned.', + }) + const nonFinding = makeFinding({ + line: 2, + failure_scenario: "N/A — this is correct behavior.", + }) + + const result = filterNonFindings([realFinding, nonFinding]) + + expect(result).toEqual({ + findings: [realFinding], + droppedAsNonFinding: 1, + }) + }) + + it("does not drop a finding that contains 'None' mid-sentence", () => { + const finding = makeFinding({ + failure_scenario: + "Returns None instead of an empty list when the input is empty.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [finding], droppedAsNonFinding: 0 }) + }) + + it("drops a finding with leading whitespace before the prefix", () => { + const finding = makeFinding({ + failure_scenario: " N/A — tests are valid.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("handles an empty findings array", () => { + const result = filterNonFindings([]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 0 }) + }) +}) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 7adf0b4..9fb70b5 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -67,10 +67,32 @@ describe("buildSystemPrompt", () => { expect(systemPrompt).toContain("For CI/workflow files") expect(systemPrompt).toContain('fill the "analysis" field') expect(systemPrompt).toContain("Severity rubric:") + expect(systemPrompt).toContain("OUTPUT DISCIPLINE") + expect(systemPrompt).toContain( + "imperative fix statement, under 80 characters", + ) + expect(systemPrompt).toContain( + "1–3 sentences stating the defect and its impact", + ) + expect(systemPrompt).toContain( + 'starting with "N/A", "None",\n "Not applicable", or "Placeholder"', + ) + expect(systemPrompt).toContain("HARD PROHIBITION") expect(systemPrompt).toContain( "new-file line numbers printed in\nthe annotated diff", ) }) + + it("places OUTPUT DISCIPLINE between SEVERITY_RUBRIC and ANCHORING_CONTRACT", () => { + const systemPrompt = buildSystemPrompt({ phase: combinedPhase }) + + const severityIndex = systemPrompt.indexOf("Severity rubric:") + const outputDisciplineIndex = systemPrompt.indexOf("OUTPUT DISCIPLINE") + const anchoringIndex = systemPrompt.indexOf("Line anchoring:") + + expect(outputDisciplineIndex).toBeGreaterThan(severityIndex) + expect(anchoringIndex).toBeGreaterThan(outputDisciplineIndex) + }) }) describe("buildUserPrompt", () => { diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index 1e43acf..0e3ec6b 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -28,7 +28,7 @@ const findingTag = (finding: Finding): string => `**[${finding.severity}/${finding.category}]** ${finding.title}` const suggestionBlock = (finding: Finding): string => { - if (finding.suggestion === null) return "" + if (!finding.suggestion) return "" // CommonMark: a fence longer than any backtick run inside the content // cannot be closed early — sizes the fence to LLM-generated suggestions // that themselves contain fenced blocks @@ -38,7 +38,7 @@ const suggestionBlock = (finding: Finding): string => { 0, ) const fence = "`".repeat(Math.max(3, longestBacktickRun + 1)) - return `\n\n${fence}diff\n${finding.suggestion}\n${fence}` + return `\n\n
\nSuggested fix\n\n${fence}diff\n${finding.suggestion}\n${fence}\n\n
` } const renderCommentBody = ( diff --git a/src/review/filter-non-findings.ts b/src/review/filter-non-findings.ts new file mode 100644 index 0000000..8b99ee7 --- /dev/null +++ b/src/review/filter-non-findings.ts @@ -0,0 +1,20 @@ +import type { Finding } from "./finding.js" + +export type FilterResult = { + findings: Finding[] + droppedAsNonFinding: number +} + +/** Matches failure_scenario prefixes that signal "no real finding" — start-anchored, case-insensitive, word-bounded. */ +const NON_FINDING_PREFIX = /^(?:n\/?a|not applicable|placeholder)\b/i + +const isNonFinding = (finding: Finding): boolean => + NON_FINDING_PREFIX.test(finding.failure_scenario.trim()) + +export const filterNonFindings = (findings: Finding[]): FilterResult => { + const realFindings = findings.filter((finding) => !isNonFinding(finding)) + return { + findings: realFindings, + droppedAsNonFinding: findings.length - realFindings.length, + } +} diff --git a/src/review/prompt.ts b/src/review/prompt.ts index 76da20a..94c34ab 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -49,6 +49,23 @@ Every finding must include failure_scenario: the concrete input or state that triggers the problem and what goes wrong. If you cannot state a concrete failure scenario, do not report the finding.` +const OUTPUT_DISCIPLINE = `OUTPUT DISCIPLINE — field constraints: +- "title": imperative fix statement, under 80 characters (e.g. "Trim keys + before inserting into the registry"). Do not start with "Issue:" or + "Bug:". +- "description": 1–3 sentences stating the defect and its impact. No code + tracing, no call-chain walk-through, no quoting of source lines. Put + traces and evidence in "analysis", not here. +- "failure_scenario": a concrete input or state that triggers the problem + and what goes wrong. A failure_scenario starting with "N/A", "None", + "Not applicable", or "Placeholder" means there is no real finding — do + not emit the finding at all. + +HARD PROHIBITION — do not report a finding whose conclusion is "no bug", +"this is correct", "working as designed", "correct behavior", or any +equivalent. If your analysis concludes the code is correct, record that +conclusion in "analysis" and move on — do not emit a finding for it.` + const ANCHORING_CONTRACT = `Line anchoring: "line" and "end_line" use the new-file line numbers printed in the annotated diff. For inline placement, reference only numbers that appear there, and keep end_line in the same hunk as line. Findings in code outside @@ -62,6 +79,7 @@ export const buildSystemPrompt = ({ phase }: { phase: ReviewPhase }): string => ...phase.instructionSections, PROOF_OF_WORK, SEVERITY_RUBRIC, + OUTPUT_DISCIPLINE, ANCHORING_CONTRACT, ].join("\n\n")