From 88c927374f15e881f7485bf18858eec189376060 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 22 Jul 2026 19:28:07 -0400 Subject: [PATCH 1/7] feat: feed prior bot comments to model for conceptual dedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-run dedup uses file:category:line keys with a ±5 line proximity window — this misses conceptual duplicates at distant lines (same premise, 100+ lines apart). Feed existing bot comment bodies to the model via a new prior_bot_comments prompt section so it self-suppresses known issues at the semantic level. - Refactor fetchInlineAnchors → fetchInlineCommentState to preserve raw comment bodies alongside anchors - Extend fetchIssueCommentState to return finding bodies (status comment excluded) - Strip dedup anchor HTML before including in prompt - Cap at 30 most recent comments to bound token usage - Add priorBotComments to ReviewContext, flows through to buildUserPrompt - 7 new tests: body passthrough, anchor stripping, status exclusion, cap enforcement, first-run empty case, prompt rendering + ordering Co-Authored-By: Claude Opus 4.6 (1M context) --- src/__tests__/orchestrate.test.ts | 78 +++++++++++++++++++++++++++++ src/orchestrate.ts | 69 ++++++++++++++++++------- src/review/__tests__/prompt.test.ts | 43 ++++++++++++++++ src/review/prompt.ts | 9 ++++ 4 files changed, 180 insertions(+), 19 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 9ddb650..117e8c1 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -1648,6 +1648,81 @@ describe("orchestrate", () => { data: { error: "[Error]: API rate limit" }, }) }) + + const priorBotCommentsFrom = (stubs: RecordingStubs): string[] => { + const context = stubs.generateFindingsCalls[0] + if (!context) { + throw new Error("expected at least one generateFindings call") + } + return context.priorBotComments + } + + it("passes prior bot comment bodies (anchor-stripped) to generateFindings", async () => { + const commentBody = + "**[high/correctness]** Fix null check\n\nDescription.\n\n" + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchBotReviewComments: async () => [ + existingComment(commentBody, { line: 99 }), + ], + fetchBotIssueComments: async () => [ + statusComment, + issueFinding("src/other.ts:security:50"), + ], + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(priorBotCommentsFrom(stubs)).toEqual([ + "**[high/correctness]** Fix null check\n\nDescription.", + "finding text", + ]) + }) + + it("excludes the status comment from prior bot comments", async () => { + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchBotIssueComments: async () => [statusComment], + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(priorBotCommentsFrom(stubs)).toEqual([]) + }) + + it("caps prior bot comments at 30", async () => { + const comments = Array.from({ length: 40 }, (_, index) => + existingComment( + `finding ${index}\n\n`, + { line: index + 1 }, + ), + ) + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchBotReviewComments: async () => comments, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + const priorComments = priorBotCommentsFrom(stubs) + expect(priorComments).toHaveLength(30) + expect(priorComments[29]).toBe("finding 39") + }) + + it("passes empty prior bot comments on a first run", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(priorBotCommentsFrom(stubs)).toEqual([]) + }) }) }) @@ -1690,6 +1765,7 @@ describe("createPromptedGenerateFindings", () => { relatedDocs: [], annotatedDiff: annotateDiff(files), priorFindings: [], + priorBotComments: [], }) expect(requestReviewCalls).toHaveLength(1) @@ -1736,6 +1812,7 @@ describe("createPromptedGenerateFindings", () => { relatedDocs: [], annotatedDiff: annotated, priorFindings: [], + priorBotComments: [], }) const call = first(requestReviewCalls) @@ -1777,6 +1854,7 @@ describe("createPromptedGenerateFindings", () => { relatedDocs: [], annotatedDiff: annotated, priorFindings: [], + priorBotComments: [], } await generate(context) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 473a9c3..907e33e 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -48,6 +48,7 @@ export type ReviewContext = { relatedDocs: PromptFile[] annotatedDiff: string priorFindings: Finding[] + priorBotComments: string[] } export type GenerateFindings = ( @@ -72,6 +73,13 @@ export type OrchestrateDeps = { generateFindings: GenerateFindings } +const PRIOR_COMMENT_CAP = 30 + +/** Strips the trailing dedup anchor from a comment body so the model + * doesn't see the dedup infrastructure in the prior-comments section. */ +const stripAnchorComment = (body: string): string => + body.replace(/\n*\s*$/, "") + const buildSkipBody = (reason: string): string => `**umm-actually** — review skipped\n\n${reason}\n\n---\n*umm-actually*` @@ -81,29 +89,38 @@ const describeError = (error: unknown): string => { : String(error) } -/** Anchors from the bot's inline review comments (live positions). Empty on +type InlineCommentState = { + anchors: AnchorEntry[] + commentBodies: string[] +} + +/** Anchors and raw bodies from the bot's inline review comments. Empty on * fetch failure — findings then post as new; duplicates beat losing them. */ -const fetchInlineAnchors = async ( +const fetchInlineCommentState = async ( { githubClient, prNumber }: { githubClient: GithubClient; prNumber: number }, logger: Logger, -): Promise => { +): Promise => { try { const existingComments = await githubClient.fetchBotReviewComments({ prNumber, }) - return extractAnchors(existingComments) + return { + anchors: extractAnchors(existingComments), + commentBodies: existingComments.map((comment) => comment.body), + } } catch (fetchError) { logger.warn( "failed to fetch inline comments — treating their findings as new", { error: describeError(fetchError) }, ) - return [] + return { anchors: [], commentBodies: [] } } } type IssueCommentState = { statusCommentExists: boolean anchors: AnchorEntry[] + findingBodies: string[] } /** The bot's issue comments carry the rest of the cross-run state: the @@ -116,25 +133,27 @@ const fetchIssueCommentState = async ( ): Promise => { try { const comments = await githubClient.fetchBotIssueComments({ prNumber }) + const findingComments = comments.filter( + (comment) => !comment.body.startsWith(STATUS_ANCHOR), + ) return { // startsWith, not includes: a finding comment's model-generated text // could quote the marker mid-body and misclassify the run as a re-run. - statusCommentExists: comments.some((comment) => - comment.body.startsWith(STATUS_ANCHOR), - ), + statusCommentExists: comments.length > findingComments.length, anchors: extractAnchors( - comments.map((comment) => ({ + findingComments.map((comment) => ({ body: comment.body, line: null, originalLine: null, })), ), + findingBodies: findingComments.map((comment) => comment.body), } } catch (fetchError) { logger.warn("failed to fetch issue comments — treating as a first run", { error: describeError(fetchError), }) - return { statusCommentExists: false, anchors: [] } + return { statusCommentExists: false, anchors: [], findingBodies: [] } } } @@ -396,6 +415,24 @@ export const orchestrate = async ( ) } + // Step 9.5: fetch prior bot comments — needed both for the prompt (the + // model sees what's already posted and self-suppresses conceptual dupes) + // and for positional dedup after generation. + const inlineState = await fetchInlineCommentState( + { githubClient, prNumber: prContext.prNumber }, + logger, + ) + const issueState = await fetchIssueCommentState( + { githubClient, prNumber: prContext.prNumber }, + logger, + ) + const priorBotComments = [ + ...inlineState.commentBodies, + ...issueState.findingBodies, + ] + .map(stripAnchorComment) + .slice(-PRIOR_COMMENT_CAP) + // Step 10–11: generate findings (V1: single combined phase) const phase = phases[0] if (!phase) { @@ -410,6 +447,7 @@ export const orchestrate = async ( relatedDocs, annotatedDiff, priorFindings: [], + priorBotComments, }) const { modelUsed, attempts } = structuredResult @@ -428,15 +466,7 @@ export const orchestrate = async ( // is just the case where no bot comments exist yet. Sources: the bot's // inline comments (live positions) and its beyond-diff issue comments // (anchor lines). Runs before the cap so duplicates don't consume slots. - const inlineAnchors = await fetchInlineAnchors( - { githubClient, prNumber: prContext.prNumber }, - logger, - ) - const issueState = await fetchIssueCommentState( - { githubClient, prNumber: prContext.prNumber }, - logger, - ) - const existingAnchors = [...inlineAnchors, ...issueState.anchors] + const existingAnchors = [...inlineState.anchors, ...issueState.anchors] const newFindings = realFindings.filter( (finding) => !isDuplicateFinding(finding, existingAnchors), ) @@ -444,6 +474,7 @@ export const orchestrate = async ( logger.info("cross-run dedup against prior bot comments", { statusCommentFound: issueState.statusCommentExists, existingAnchorCount: existingAnchors.length, + priorBotCommentCount: priorBotComments.length, findingsAfterFilter: realFindings.length, findingsSurvivedDedup: newFindings.length, }) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index 743f504..d8d23ee 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -45,6 +45,7 @@ const makeUserPromptParts = () => ({ annotatedDiff: "=== src/greeter.ts ===\n@@ -1,1 +1,1 @@\n 1 + export const greet", priorFindings: [], + priorBotComments: [], delimiterNonce: "abc123def456", }) @@ -288,6 +289,48 @@ describe("buildUserPrompt", () => { expect(userPrompt).toContain(priorFinding.title) }) + it("omits the prior bot comments section when empty", () => { + const userPrompt = buildUserPrompt(makeUserPromptParts()) + + expect(userPrompt).not.toContain("prior_bot_comments") + }) + + it("includes prior bot comments with a do-not-re-report instruction when present", () => { + const userPrompt = buildUserPrompt({ + ...makeUserPromptParts(), + priorBotComments: [ + "**[high/correctness]** Fix null check\n\nDescription here.", + "**[medium/security]** Sanitize input\n\nAnother finding.", + ], + }) + + expect(userPrompt).toContain( + '', + ) + expect(userPrompt).toContain("Fix null check") + expect(userPrompt).toContain("Sanitize input") + expect(userPrompt).toContain("\n\n---\n\n") + }) + + it("places prior bot comments after the diff and before prior findings", () => { + const priorFinding = makeFinding() + + const userPrompt = buildUserPrompt({ + ...makeUserPromptParts(), + priorFindings: [priorFinding], + priorBotComments: ["**[high/correctness]** Some prior finding"], + }) + + const diffIndex = userPrompt.indexOf("diff-abc123def456") + const botCommentsIndex = userPrompt.indexOf( + "prior_bot_comments-abc123def456", + ) + const priorFindingsIndex = userPrompt.indexOf("prior_findings-abc123def456") + + expect(diffIndex).toBeLessThan(botCommentsIndex) + expect(botCommentsIndex).toBeLessThan(priorFindingsIndex) + }) + it("labels related files with their inclusion reason", () => { const userPrompt = buildUserPrompt(makeUserPromptParts()) diff --git a/src/review/prompt.ts b/src/review/prompt.ts index cc7c158..cf87acf 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -137,6 +137,7 @@ export const buildUserPrompt = ({ relatedDocs, annotatedDiff, priorFindings, + priorBotComments, delimiterNonce, }: { prContext: PrContext @@ -146,6 +147,7 @@ export const buildUserPrompt = ({ relatedDocs: PromptFile[] annotatedDiff: string priorFindings: Finding[] + priorBotComments: string[] /** Per-run random tag suffix — see generateDelimiterNonce. */ delimiterNonce: string }): string => { @@ -153,6 +155,7 @@ export const buildUserPrompt = ({ const conventionsTag = `conventions-${delimiterNonce}` const diffTag = `diff-${delimiterNonce}` const priorFindingsTag = `prior_findings-${delimiterNonce}` + const priorBotCommentsTag = `prior_bot_comments-${delimiterNonce}` // Title, description, and branch names are PR-author-controlled — wrapped // like every other untrusted section so they can't sit in instruction position @@ -191,6 +194,11 @@ export const buildUserPrompt = ({ ? "" : `<${priorFindingsTag} note="already reported by earlier phases — do not re-report">\n${JSON.stringify(priorFindings, null, 2)}\n` + const priorBotCommentsSection = + priorBotComments.length === 0 + ? "" + : `<${priorBotCommentsTag} note="findings already posted on this PR — do not re-report the same issues, even at different locations">\n${priorBotComments.join("\n\n---\n\n")}\n` + const sections = [ metadataSection, conventionsSection, @@ -198,6 +206,7 @@ export const buildUserPrompt = ({ relatedFilesSection, relatedDocsSection, `<${diffTag} note="line numbers shown are new-file line numbers">\n${annotatedDiff}\n`, + priorBotCommentsSection, priorFindingsSection, ] From 4fd9170d8af2bddf12ad004c64207fbb79debeb4 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 22 Jul 2026 19:53:25 -0400 Subject: [PATCH 2/7] fix(review): document conceptual dedup in README pipeline and shipped sections Co-Authored-By: Claude Opus 4.6 (1M context) --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4876d07..4f8f2da 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm 1. Resolves the PR from the triggering event (supports `pull_request`, `pull_request_target`, and `issue_comment` events) 2. Fetches the unified diff via the GitHub API — PRs that exceed the API's diff size limit are skipped 3. Reads the conventions file and changed source files (token-budgeted), traces imports to find related code files, and scans doc files (`.md`, `.json`) for mentions of changed paths -4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense) and sends it to OpenRouter +4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense); on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends it to OpenRouter 5. Validates the response against a strict Zod schema, retrying with a fallback model if the primary fails 6. Drops non-findings (see [Non-finding filter](#non-finding-filter)), then on re-runs deduplicates against previously posted inline comments (by hidden HTML anchor) 7. Filters remaining findings by severity threshold, deduplicates overlapping findings within the run, and caps if configured @@ -137,7 +137,7 @@ umm-actually is in early development — the core review pipeline works but ther - Skip-path handling with posted reasons (oversized diff, empty diff, API limits) - Cost transparency (per-run model/token/USD report in workflow summary) - `@umm review` comment trigger for on-demand re-reviews -- Cross-run finding dedup — re-runs detect previously posted inline findings via hidden HTML anchors and post only new ones, with an updatable summary comment tracking totals +- Cross-run finding dedup — re-runs detect previously posted inline findings via hidden HTML anchors and post only new ones; prior bot comment bodies also feed into the prompt for conceptual dedup (the model self-suppresses even when positional anchors differ). An updatable summary comment tracks totals - Non-finding filter — deterministic drop of findings that amount to "no bug here" (`N/A` prefixes, "no action needed" suggestions, "…is correct" titles) before threshold and cap **In progress** From 17c997fdf1f25e87942b3ebd7c86fc5466b273aa Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 22 Jul 2026 19:59:01 -0400 Subject: [PATCH 3/7] style: rename pathList to renderPaths, add summary to review/ description Co-Authored-By: Claude Opus 4.6 (1M context) --- AGENTS.md | 2 +- src/review/review-summary.ts | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1404261..c6d9fef 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,7 @@ src/ openrouter/ # OpenRouter I/O: @openrouter/sdk wrapper, structured-output retry ladder, cost summary diff/ # pure transforms over parse-diff output context/ # workspace I/O: conventions file, changed files, import-trace scan, doc-mention scan, priority docs - review/ # pure review logic: finding schema, phases, prompt, selection, comment mapping + review/ # pure review logic: finding schema, phases, prompt, selection, comment mapping, summary orchestrate.ts # pipeline + createPromptedGenerateFindings — fully testable with stub clients ``` diff --git a/src/review/review-summary.ts b/src/review/review-summary.ts index 75bc30c..046da0b 100644 --- a/src/review/review-summary.ts +++ b/src/review/review-summary.ts @@ -20,7 +20,7 @@ export type ReviewSummaryStats = { /** Formats paths for a markdown table cell — em-dash when empty so cells * are never blank. Pipes are escaped so paths can't break the table. */ -const pathList = (paths: string[]): string => +const renderPaths = (paths: string[]): string => paths.length === 0 ? "—" : paths.map((path) => path.replaceAll("|", "\\|")).join(", ") @@ -42,12 +42,12 @@ export const renderReviewSummary = (stats: ReviewSummaryStats): string => { "", "| type | count | paths |", "| --- | --- | --- |", - `| Changed files | ${stats.changedFilePaths.length} | ${pathList(stats.changedFilePaths)} |`, - `| Related files | ${stats.relatedFilePaths.length} | ${pathList(stats.relatedFilePaths)} |`, - `| Priority docs | ${stats.priorityDocPaths.length} | ${pathList(stats.priorityDocPaths)} |`, - `| Mention-matched docs | ${stats.mentionMatchedDocPaths.length} | ${pathList(stats.mentionMatchedDocPaths)} |`, - `| Excluded (related files cap) | ${stats.relatedFilesExcludedPaths.length} | ${pathList(stats.relatedFilesExcludedPaths)} |`, - `| Excluded (docs cap) | ${stats.docsExcludedPaths.length} | ${pathList(stats.docsExcludedPaths)} |`, + `| Changed files | ${stats.changedFilePaths.length} | ${renderPaths(stats.changedFilePaths)} |`, + `| Related files | ${stats.relatedFilePaths.length} | ${renderPaths(stats.relatedFilePaths)} |`, + `| Priority docs | ${stats.priorityDocPaths.length} | ${renderPaths(stats.priorityDocPaths)} |`, + `| Mention-matched docs | ${stats.mentionMatchedDocPaths.length} | ${renderPaths(stats.mentionMatchedDocPaths)} |`, + `| Excluded (related files cap) | ${stats.relatedFilesExcludedPaths.length} | ${renderPaths(stats.relatedFilesExcludedPaths)} |`, + `| Excluded (docs cap) | ${stats.docsExcludedPaths.length} | ${renderPaths(stats.docsExcludedPaths)} |`, "", "#### Findings pipeline", "", From 534aa708453a324f79f86366e7dba96607a3aec4 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:08:01 -0400 Subject: [PATCH 4/7] test: strengthen assertions + add renderPaths pipe-escaping coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full-result toEqual on all selectFindings tests (was decomposed property picks that missed droppedAsOverlapping/droppedByCap fields). Collapse 4 fragment toContain calls in the prior-bot-comments prompt test into one full-section assertion. Add pipe-escaping coverage for renderPaths — the only production behavior with no test. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/review/__tests__/prompt.test.ts | 13 ++- src/review/__tests__/review-summary.test.ts | 10 +++ src/review/__tests__/select-findings.test.ts | 85 +++++++++++++++----- 3 files changed, 82 insertions(+), 26 deletions(-) diff --git a/src/review/__tests__/prompt.test.ts b/src/review/__tests__/prompt.test.ts index d8d23ee..58848c1 100644 --- a/src/review/__tests__/prompt.test.ts +++ b/src/review/__tests__/prompt.test.ts @@ -305,11 +305,16 @@ describe("buildUserPrompt", () => { }) expect(userPrompt).toContain( - '', + [ + '', + "**[high/correctness]** Fix null check\n\nDescription here.", + "", + "---", + "", + "**[medium/security]** Sanitize input\n\nAnother finding.", + "", + ].join("\n"), ) - expect(userPrompt).toContain("Fix null check") - expect(userPrompt).toContain("Sanitize input") - expect(userPrompt).toContain("\n\n---\n\n") }) it("places prior bot comments after the diff and before prior findings", () => { diff --git a/src/review/__tests__/review-summary.test.ts b/src/review/__tests__/review-summary.test.ts index 38d912b..321d399 100644 --- a/src/review/__tests__/review-summary.test.ts +++ b/src/review/__tests__/review-summary.test.ts @@ -174,4 +174,14 @@ describe("renderReviewSummary", () => { expect(summary).toContain("`abc123d`") expect(summary).not.toContain(baseStats.prContext.headSha) }) + + it("escapes pipe characters in paths so they cannot break the markdown table", () => { + const summary = renderReviewSummary({ + ...baseStats, + changedFilePaths: ["src/a|b.ts"], + }) + + expect(summary).toContain("| Changed files | 1 | src/a\\|b.ts |") + expect(summary).not.toContain("| src/a|b.ts |") + }) }) diff --git a/src/review/__tests__/select-findings.test.ts b/src/review/__tests__/select-findings.test.ts index b750399..177e4cf 100644 --- a/src/review/__tests__/select-findings.test.ts +++ b/src/review/__tests__/select-findings.test.ts @@ -14,8 +14,12 @@ describe("selectFindings", () => { maxFindings: undefined, }) - expect(result.selected).toEqual([highFinding, mediumFinding]) - expect(result.droppedBelowThreshold).toBe(1) + expect(result).toEqual({ + selected: [highFinding, mediumFinding], + droppedBelowThreshold: 1, + droppedAsOverlapping: 0, + droppedByCap: [], + }) }) it("keeps the higher-severity finding when same-category findings overlap in a file", () => { @@ -28,8 +32,12 @@ describe("selectFindings", () => { maxFindings: undefined, }) - expect(result.selected).toEqual([highOriginal]) - expect(result.droppedAsOverlapping).toBe(1) + expect(result).toEqual({ + selected: [highOriginal], + droppedBelowThreshold: 0, + droppedAsOverlapping: 1, + droppedByCap: [], + }) }) it("deduplicates overlapping line ranges of the same category", () => { @@ -50,8 +58,12 @@ describe("selectFindings", () => { maxFindings: undefined, }) - expect(result.selected).toEqual([rangeFinding]) - expect(result.droppedAsOverlapping).toBe(1) + expect(result).toEqual({ + selected: [rangeFinding], + droppedBelowThreshold: 0, + droppedAsOverlapping: 1, + droppedByCap: [], + }) }) it("deduplicates against an inverted range (end_line before line)", () => { @@ -72,7 +84,12 @@ describe("selectFindings", () => { maxFindings: undefined, }) - expect(result.selected).toEqual([invertedRangeFinding]) + expect(result).toEqual({ + selected: [invertedRangeFinding], + droppedBelowThreshold: 0, + droppedAsOverlapping: 1, + droppedByCap: [], + }) }) it("does not deduplicate same-category overlapping findings in different files", () => { @@ -85,7 +102,12 @@ describe("selectFindings", () => { maxFindings: undefined, }) - expect(result.selected).toEqual([firstFileFinding, secondFileFinding]) + expect(result).toEqual({ + selected: [firstFileFinding, secondFileFinding], + droppedBelowThreshold: 0, + droppedAsOverlapping: 0, + droppedByCap: [], + }) }) it("does not deduplicate overlapping findings of different categories", () => { @@ -101,7 +123,12 @@ describe("selectFindings", () => { maxFindings: undefined, }) - expect(result.selected).toHaveLength(2) + expect(result).toEqual({ + selected: [correctnessFinding, testsFinding], + droppedBelowThreshold: 0, + droppedAsOverlapping: 0, + droppedByCap: [], + }) }) it("sorts by severity descending, then file, then line", () => { @@ -128,12 +155,12 @@ describe("selectFindings", () => { maxFindings: undefined, }) - expect(result.selected).toEqual([ - criticalLate, - mediumOtherFile, - mediumSameFileLater, - lowEarly, - ]) + expect(result).toEqual({ + selected: [criticalLate, mediumOtherFile, mediumSameFileLater, lowEarly], + droppedBelowThreshold: 0, + droppedAsOverlapping: 0, + droppedByCap: [], + }) }) it("posts every finding when no cap is provided", () => { @@ -147,8 +174,12 @@ describe("selectFindings", () => { maxFindings: undefined, }) - expect(result.selected).toHaveLength(25) - expect(result.droppedByCap).toEqual([]) + expect(result).toEqual({ + selected: manyFindings, + droppedBelowThreshold: 0, + droppedAsOverlapping: 0, + droppedByCap: [], + }) }) it("caps at max_findings keeping the most severe, and reports the dropped ones", () => { @@ -162,18 +193,28 @@ describe("selectFindings", () => { maxFindings: 2, }) - expect(result.selected).toEqual([criticalFinding, highFinding]) - expect(result.droppedByCap).toEqual([lowFinding]) + expect(result).toEqual({ + selected: [criticalFinding, highFinding], + droppedBelowThreshold: 0, + droppedAsOverlapping: 0, + droppedByCap: [lowFinding], + }) }) it("does not report a cap drop when the cap exceeds the finding count", () => { + const finding = makeFinding() + const result = selectFindings({ - findings: [makeFinding()], + findings: [finding], severityThreshold: "low", maxFindings: 10, }) - expect(result.selected).toHaveLength(1) - expect(result.droppedByCap).toEqual([]) + expect(result).toEqual({ + selected: [finding], + droppedBelowThreshold: 0, + droppedAsOverlapping: 0, + droppedByCap: [], + }) }) }) From f09d764eca6f772792eaf745707ec2f9236deb6f Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:17:37 -0400 Subject: [PATCH 5/7] fix: description-vs-implementation gaps in structure tree, prompt JSDoc, outputs table - AGENTS.md: review/ structure tree now includes filter-non-findings.ts (listed as "non-finding filter") - prompt.ts: buildUserPrompt JSDoc now enumerates the prior-bot-comments and prior-findings sections that follow the diff - README.md: findings_count output description now mentions the non-finding filter stage Co-Authored-By: Claude Opus 4.6 (1M context) --- AGENTS.md | 2 +- README.md | 12 ++++++------ src/review/prompt.ts | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c6d9fef..c20cb84 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,7 @@ src/ openrouter/ # OpenRouter I/O: @openrouter/sdk wrapper, structured-output retry ladder, cost summary diff/ # pure transforms over parse-diff output context/ # workspace I/O: conventions file, changed files, import-trace scan, doc-mention scan, priority docs - review/ # pure review logic: finding schema, phases, prompt, selection, comment mapping, summary + review/ # pure review logic: finding schema, phases, prompt, non-finding filter, selection, comment mapping, summary orchestrate.ts # pipeline + createPromptedGenerateFindings — fully testable with stub clients ``` diff --git a/README.md b/README.md index 4f8f2da..39bb59b 100644 --- a/README.md +++ b/README.md @@ -93,12 +93,12 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm ## Outputs -| Output | Description | -| ---------------- | ------------------------------------------------------------------------- | -| `findings_count` | Number of new findings posted (after threshold, cap, and cross-run dedup) | -| `review_url` | URL of the submitted review; empty when no review was posted | -| `model_used` | Model that produced the accepted response | -| `skipped_reason` | Non-empty when the review was skipped (e.g. diff too large) | +| Output | Description | +| ---------------- | --------------------------------------------------------------------------------------------- | +| `findings_count` | Number of new findings posted (after non-finding filter, threshold, cap, and cross-run dedup) | +| `review_url` | URL of the submitted review; empty when no review was posted | +| `model_used` | Model that produced the accepted response | +| `skipped_reason` | Non-empty when the review was skipped (e.g. diff too large) | ## How it works diff --git a/src/review/prompt.ts b/src/review/prompt.ts index cf87acf..91f2c07 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -126,7 +126,8 @@ const renderFileBlock = (file: PromptFile, delimiterNonce: string): string => { /** * Assembles the user message. Section order is stable → volatile so prefix * caching can help on providers that support it: metadata, conventions, file - * contents, related files, related docs, then the diff. The diff is never + * contents, related files, related docs, diff, then prior bot comments and + * prior findings (most volatile — change between runs). The diff is never * truncated here — oversized PRs are skipped upstream rather than reviewed badly. */ export const buildUserPrompt = ({ From 0dc4d67aca029a074ddc06b86521dfc5851d89bf Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:21:02 -0400 Subject: [PATCH 6/7] test: assert complete capped prior-comments array Co-Authored-By: Claude Opus 4.6 (1M context) --- src/__tests__/orchestrate.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 117e8c1..0fab8fe 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -1710,9 +1710,9 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) - const priorComments = priorBotCommentsFrom(stubs) - expect(priorComments).toHaveLength(30) - expect(priorComments[29]).toBe("finding 39") + expect(priorBotCommentsFrom(stubs)).toEqual( + Array.from({ length: 30 }, (_, index) => `finding ${index + 10}`), + ) }) it("passes empty prior bot comments on a first run", async () => { From a23ed309638c8e27f4a0583f492c79843897ce98 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:16:39 -0400 Subject: [PATCH 7/7] docs: add JSDoc to PRIOR_COMMENT_CAP constant Co-Authored-By: Claude Opus 4.6 (1M context) --- src/orchestrate.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 907e33e..380cad1 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -73,6 +73,7 @@ export type OrchestrateDeps = { generateFindings: GenerateFindings } +/** Bounds token cost of prior bot comments in the prompt (~200 tokens each). */ const PRIOR_COMMENT_CAP = 30 /** Strips the trailing dedup anchor from a comment body so the model