From 8b468080d2cfbd2a0983b69bf62ad1f7a050cb99 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:00:36 -0400 Subject: [PATCH 1/5] feat: filter non-findings and constrain comment verbosity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reduce review noise via three changes: 1. Prompt hardening — OUTPUT DISCIPLINE section constraining field content (imperative titles, 1-3 sentence descriptions, concrete failure scenarios) and prohibiting "no bug" conclusions from appearing as findings. 2. Post-LLM filter — deterministic regex filter on failure_scenario catches non-findings the prompt didn't prevent (N/A, None, "this is correct", etc). Integrated as Step 12.5 in orchestrate between selection and posting. 3. Collapsible suggestions — diff blocks wrapped in
to keep the main comment body focused on the problem and failure scenario. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/__tests__/orchestrate.test.ts | 69 ++++++ src/orchestrate.ts | 10 +- src/review/__tests__/comment-mapping.test.ts | 8 +- .../__tests__/filter-non-findings.test.ts | 212 ++++++++++++++++++ src/review/__tests__/prompt.test.ts | 13 ++ src/review/comment-mapping.ts | 2 +- src/review/filter-non-findings.ts | 24 ++ src/review/prompt.ts | 18 ++ 8 files changed, 351 insertions(+), 5 deletions(-) create mode 100644 src/review/__tests__/filter-non-findings.test.ts create mode 100644 src/review/filter-non-findings.ts diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 4a93268..395464e 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,73 @@ 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 mixedSelection = selectFindings({ + findings: mixedResponse.findings, + severityThreshold: "low", + maxFindings: undefined, + }) + const { findings: mixedFiltered } = filterNonFindings( + mixedSelection.selected, + ) + const mixedMapped = mapFindingsToReview({ + findings: mixedFiltered, + commentableByPath: fixtureCommentableByPath, + }) + const mixedBody = buildReviewBody({ + bodyFindings: mixedMapped.bodyFindings, + droppedByCap: mixedSelection.droppedByCap, + model: "test/model", + inlineCommentCount: mixedMapped.comments.length, + }) + const mixedFallbackBody = buildReviewBody({ + bodyFindings: mixedFiltered, + 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, + }) + }) }) }) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index f2b25e7..67e4ca9 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 = { @@ -246,12 +247,19 @@ export const orchestrate = async ( const { modelUsed, attempts } = structuredResult // Step 12: select findings - const { selected, droppedByCap } = selectFindings({ + const { selected: selectedByThreshold, droppedByCap } = selectFindings({ findings: structuredResult.review.findings, severityThreshold, maxFindings: config.maxFindings, }) + // Step 12.5: filter non-findings + const { findings: selected, droppedAsNonFinding } = + filterNonFindings(selectedByThreshold) + if (droppedAsNonFinding > 0) { + logger.info("filtered non-findings", { droppedAsNonFinding }) + } + // Step 13: post review const hasFindings = selected.length > 0 diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index b3ce9ad..608c1da 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -215,7 +215,7 @@ 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") }) @@ -230,7 +230,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 +282,9 @@ _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).toContain( + "
\nSuggested fix\n\n```diff\n-old line\n+new line\n```\n\n
", + ) }) 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..69aeebc --- /dev/null +++ b/src/review/__tests__/filter-non-findings.test.ts @@ -0,0 +1,212 @@ +import { describe, expect, it } from "vitest" +import { filterNonFindings } from "../filter-non-findings.js" +import { makeFinding } from "./make-finding.js" + +describe("filterNonFindings", () => { + // --- Primary signal: prefix patterns --- + + 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 — this is correct behavior.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario starts with 'None'", () => { + const finding = makeFinding({ + failure_scenario: "None — logic appears correct.", + }) + + 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 }) + }) + + // --- Secondary signal: body content --- + + it("drops a finding whose failure_scenario contains 'no actual bug'", () => { + const finding = makeFinding({ + failure_scenario: "After analysis, no actual bug was found in this path.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario contains 'this is correct'", () => { + const finding = makeFinding({ + failure_scenario: + "The behavior triggered when input is empty, but this is correct per the spec.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario contains 'working as designed'", () => { + const finding = makeFinding({ + failure_scenario: + "The function returns null for missing keys, working as designed.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario contains 'correct behavior'", () => { + const finding = makeFinding({ + failure_scenario: + "The validation rejects empty strings — correct behavior, not a bug.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario contains 'by design'", () => { + const finding = makeFinding({ + failure_scenario: + "The guard rejects the empty string by design; this is not a bug.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + it("drops a finding whose failure_scenario contains 'no bug found'", () => { + const finding = makeFinding({ + failure_scenario: "Reviewed the logic — no bug found after re-analysis.", + }) + + const result = filterNonFindings([finding]) + + expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + }) + + // --- Passthrough cases --- + + 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 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 }) + }) + + 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: "None — logic is correct." }), + ] + + 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("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..f900a0f 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -67,10 +67,23 @@ 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("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..a0dee5f 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -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..152feff --- /dev/null +++ b/src/review/filter-non-findings.ts @@ -0,0 +1,24 @@ +import type { Finding } from "./finding.js" + +export type FilterResult = { + findings: Finding[] + droppedAsNonFinding: number +} + +const NON_FINDING_PREFIX = /^(?:n\/?a|none|not applicable|placeholder)\b/i + +const NON_FINDING_BODY = + /\bno actual bug\b|\bthis is correct\b|\bworking as designed\b|\bby design\b|\bcorrect behavior\b|\bno bug found\b/i + +const isNonFinding = (finding: Finding): boolean => { + const scenario = finding.failure_scenario + return NON_FINDING_PREFIX.test(scenario) || NON_FINDING_BODY.test(scenario) +} + +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") From 7204628010ccd6bc095775d7e42e25e1e632f50c Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:33:13 -0400 Subject: [PATCH 2/5] fix: narrow non-finding filter to prefix-only matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the unanchored body regex (NON_FINDING_BODY) and the "none" prefix pattern — both produce false positives on real findings: - "by design" matches "returns null by design, but the caller never null-checks" — a real bug with a qualifying clause - "this is correct" matches "correct for ASCII but breaks on UTF-8" - "None" matches "None of the guards catch this input" The filter now only matches unambiguous non-finding prefixes: N/A, NA, Not applicable, Placeholder. The prompt hardening steers models toward these prefixes when there is no real finding. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../__tests__/filter-non-findings.test.ts | 77 +++++-------------- src/review/filter-non-findings.ts | 11 +-- 2 files changed, 22 insertions(+), 66 deletions(-) diff --git a/src/review/__tests__/filter-non-findings.test.ts b/src/review/__tests__/filter-non-findings.test.ts index 69aeebc..e055f8c 100644 --- a/src/review/__tests__/filter-non-findings.test.ts +++ b/src/review/__tests__/filter-non-findings.test.ts @@ -3,7 +3,7 @@ import { filterNonFindings } from "../filter-non-findings.js" import { makeFinding } from "./make-finding.js" describe("filterNonFindings", () => { - // --- Primary signal: prefix patterns --- + // --- Prefix patterns that should be dropped --- it("drops a finding whose failure_scenario starts with 'N/A'", () => { const finding = makeFinding({ @@ -35,16 +35,6 @@ describe("filterNonFindings", () => { expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) }) - it("drops a finding whose failure_scenario starts with 'None'", () => { - const finding = makeFinding({ - failure_scenario: "None — logic appears correct.", - }) - - 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.", @@ -65,78 +55,44 @@ describe("filterNonFindings", () => { expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) }) - // --- Secondary signal: body content --- - - it("drops a finding whose failure_scenario contains 'no actual bug'", () => { - const finding = makeFinding({ - failure_scenario: "After analysis, no actual bug was found in this path.", - }) - - const result = filterNonFindings([finding]) - - expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) - }) - - it("drops a finding whose failure_scenario contains 'this is correct'", () => { - const finding = makeFinding({ - failure_scenario: - "The behavior triggered when input is empty, but this is correct per the spec.", - }) - - const result = filterNonFindings([finding]) - - expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) - }) + // --- Passthrough: real findings must not be dropped --- - it("drops a finding whose failure_scenario contains 'working as designed'", () => { + it("keeps a finding with a concrete failure scenario", () => { const finding = makeFinding({ failure_scenario: - "The function returns null for missing keys, working as designed.", + "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: [], droppedAsNonFinding: 1 }) + expect(result).toEqual({ findings: [finding], droppedAsNonFinding: 0 }) }) - it("drops a finding whose failure_scenario contains 'correct behavior'", () => { + it("keeps a finding that mentions 'by design' as a qualifier", () => { const finding = makeFinding({ failure_scenario: - "The validation rejects empty strings — correct behavior, not a bug.", + "The function returns null by design, but the caller never null-checks.", }) const result = filterNonFindings([finding]) - expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + expect(result).toEqual({ findings: [finding], droppedAsNonFinding: 0 }) }) - it("drops a finding whose failure_scenario contains 'by design'", () => { + it("keeps a finding that mentions 'this is correct' as a qualifier", () => { const finding = makeFinding({ failure_scenario: - "The guard rejects the empty string by design; this is not a bug.", - }) - - const result = filterNonFindings([finding]) - - expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) - }) - - it("drops a finding whose failure_scenario contains 'no bug found'", () => { - const finding = makeFinding({ - failure_scenario: "Reviewed the logic — no bug found after re-analysis.", + "This is correct for ASCII but breaks on multi-byte UTF-8.", }) const result = filterNonFindings([finding]) - expect(result).toEqual({ findings: [], droppedAsNonFinding: 1 }) + expect(result).toEqual({ findings: [finding], droppedAsNonFinding: 0 }) }) - // --- Passthrough cases --- - - it("keeps a finding with a concrete failure scenario", () => { + it("keeps a finding starting with 'None of'", () => { const finding = makeFinding({ - failure_scenario: - "A CI step that gates deployment reads the env var, but the value is never set in the matrix.", + failure_scenario: "None of the guards catch this input.", }) const result = filterNonFindings([finding]) @@ -163,10 +119,15 @@ describe("filterNonFindings", () => { 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: "None — logic is correct." }), + makeFinding({ + line: 2, + failure_scenario: "Not applicable — already handled.", + }), ] const result = filterNonFindings(findings) diff --git a/src/review/filter-non-findings.ts b/src/review/filter-non-findings.ts index 152feff..4c5cd04 100644 --- a/src/review/filter-non-findings.ts +++ b/src/review/filter-non-findings.ts @@ -5,15 +5,10 @@ export type FilterResult = { droppedAsNonFinding: number } -const NON_FINDING_PREFIX = /^(?:n\/?a|none|not applicable|placeholder)\b/i +const NON_FINDING_PREFIX = /^(?:n\/?a|not applicable|placeholder)\b/i -const NON_FINDING_BODY = - /\bno actual bug\b|\bthis is correct\b|\bworking as designed\b|\bby design\b|\bcorrect behavior\b|\bno bug found\b/i - -const isNonFinding = (finding: Finding): boolean => { - const scenario = finding.failure_scenario - return NON_FINDING_PREFIX.test(scenario) || NON_FINDING_BODY.test(scenario) -} +const isNonFinding = (finding: Finding): boolean => + NON_FINDING_PREFIX.test(finding.failure_scenario) export const filterNonFindings = (findings: Finding[]): FilterResult => { const realFindings = findings.filter((finding) => !isNonFinding(finding)) From 792cb8c6859f4218fa0d8cd0b00a818c12fa36e5 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:40:40 -0400 Subject: [PATCH 3/5] =?UTF-8?q?fix:=20address=20bot=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20doc=20comments,=20test=20assertions,=20fixture=20cl?= =?UTF-8?q?arity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add doc comment to NON_FINDING_PREFIX regex constant (AGENTS.md convention) - Clean up NA test fixture text to avoid ambiguity with removed body patterns - Assert logger.messages for the "filtered non-findings" log in orchestrate test - Assert key OUTPUT_DISCIPLINE prohibitions in prompt test (not just headers) - Upgrade comment-mapping suggestion test from toContain to full toBe assertion Co-Authored-By: Claude Opus 4.6 (1M context) --- src/__tests__/orchestrate.test.ts | 6 +++++ src/review/__tests__/comment-mapping.test.ts | 23 ++++++++++++++++--- .../__tests__/filter-non-findings.test.ts | 2 +- src/review/__tests__/prompt.test.ts | 9 ++++++++ src/review/filter-non-findings.ts | 1 + 5 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 395464e..614fdaa 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -720,6 +720,12 @@ describe("orchestrate", () => { comments: mixedMapped.comments, fallbackBody: mixedFallbackBody, }) + + expect(logger.messages).toContainEqual({ + level: "info", + message: "filtered non-findings", + data: { droppedAsNonFinding: 1 }, + }) }) }) }) diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index 608c1da..90b6a3e 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -282,9 +282,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( - "
\nSuggested fix\n\n```diff\n-old line\n+new line\n```\n\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 index e055f8c..5823369 100644 --- a/src/review/__tests__/filter-non-findings.test.ts +++ b/src/review/__tests__/filter-non-findings.test.ts @@ -27,7 +27,7 @@ describe("filterNonFindings", () => { it("drops a finding whose failure_scenario starts with 'NA' (no slash)", () => { const finding = makeFinding({ - failure_scenario: "NA — this is correct behavior.", + failure_scenario: "NA — the code handles this edge case.", }) const result = filterNonFindings([finding]) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index f900a0f..9fb70b5 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -68,6 +68,15 @@ describe("buildSystemPrompt", () => { 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", diff --git a/src/review/filter-non-findings.ts b/src/review/filter-non-findings.ts index 4c5cd04..f5f6b9d 100644 --- a/src/review/filter-non-findings.ts +++ b/src/review/filter-non-findings.ts @@ -5,6 +5,7 @@ export type FilterResult = { 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 => From 14bcf6c7bde682226748d0bb486e8d8595c7ff69 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:54:24 -0400 Subject: [PATCH 4/5] fix: filter before cap, trim whitespace, guard empty suggestion - Move filterNonFindings before selectFindings so cap slots aren't wasted on non-findings that would be dropped anyway - Trim failure_scenario before regex test to handle leading whitespace - Guard against empty-string suggestion rendering a confusing empty
block Co-Authored-By: Claude Opus 4.6 (1M context) --- src/__tests__/orchestrate.test.ts | 12 +++++------ src/orchestrate.ts | 20 +++++++++---------- src/review/__tests__/comment-mapping.test.ts | 12 +++++++++++ .../__tests__/filter-non-findings.test.ts | 10 ++++++++++ src/review/comment-mapping.ts | 2 +- src/review/filter-non-findings.ts | 2 +- 6 files changed, 40 insertions(+), 18 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 614fdaa..72524e6 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -670,16 +670,16 @@ describe("orchestrate", () => { findings: [nonFinding, realFinding], } + const { findings: mixedFiltered } = filterNonFindings( + mixedResponse.findings, + ) const mixedSelection = selectFindings({ - findings: mixedResponse.findings, + findings: mixedFiltered, severityThreshold: "low", maxFindings: undefined, }) - const { findings: mixedFiltered } = filterNonFindings( - mixedSelection.selected, - ) const mixedMapped = mapFindingsToReview({ - findings: mixedFiltered, + findings: mixedSelection.selected, commentableByPath: fixtureCommentableByPath, }) const mixedBody = buildReviewBody({ @@ -689,7 +689,7 @@ describe("orchestrate", () => { inlineCommentCount: mixedMapped.comments.length, }) const mixedFallbackBody = buildReviewBody({ - bodyFindings: mixedFiltered, + bodyFindings: mixedSelection.selected, droppedByCap: mixedSelection.droppedByCap, model: "test/model", bodyFindingsHeading: "Findings", diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 67e4ca9..e501819 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -246,20 +246,20 @@ export const orchestrate = async ( const { modelUsed, attempts } = structuredResult - // Step 12: select findings - const { selected: selectedByThreshold, droppedByCap } = selectFindings({ - findings: structuredResult.review.findings, - severityThreshold, - maxFindings: config.maxFindings, - }) - - // Step 12.5: filter non-findings - const { findings: selected, droppedAsNonFinding } = - filterNonFindings(selectedByThreshold) + // 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: realFindings, + severityThreshold, + maxFindings: config.maxFindings, + }) + // Step 13: post review const hasFindings = selected.length > 0 diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index 90b6a3e..86ab70b 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -220,6 +220,18 @@ The guard rejects only the exact empty string. 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 }) diff --git a/src/review/__tests__/filter-non-findings.test.ts b/src/review/__tests__/filter-non-findings.test.ts index 5823369..2581280 100644 --- a/src/review/__tests__/filter-non-findings.test.ts +++ b/src/review/__tests__/filter-non-findings.test.ts @@ -165,6 +165,16 @@ describe("filterNonFindings", () => { 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([]) diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index a0dee5f..5cec7ee 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 === null || 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 diff --git a/src/review/filter-non-findings.ts b/src/review/filter-non-findings.ts index f5f6b9d..8b99ee7 100644 --- a/src/review/filter-non-findings.ts +++ b/src/review/filter-non-findings.ts @@ -9,7 +9,7 @@ export type FilterResult = { const NON_FINDING_PREFIX = /^(?:n\/?a|not applicable|placeholder)\b/i const isNonFinding = (finding: Finding): boolean => - NON_FINDING_PREFIX.test(finding.failure_scenario) + NON_FINDING_PREFIX.test(finding.failure_scenario.trim()) export const filterNonFindings = (findings: Finding[]): FilterResult => { const realFindings = findings.filter((finding) => !isNonFinding(finding)) From 7d2fdfa8e3d6da84f974e90935f2dc6f2e670f12 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 21:23:45 -0400 Subject: [PATCH 5/5] refactor: simplify suggestion null/empty guard to falsy check Co-Authored-By: Claude Opus 4.6 (1M context) --- src/review/comment-mapping.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index 5cec7ee..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 || finding.suggestion === "") 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