Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm
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); on re-runs, prior bot comment bodies are included so the model can self-suppress conceptual duplicates. Sends one request per review phase to OpenRouter (see the `phases` input for dispatch modes)
5. Validates each response against a strict Zod schema, retrying with a fallback model if the primary fails. A phase that fails after its retry ladder is named on the status comment and the check run while the other phases' findings still post; the run fails only when no phase completes
6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on the same lines, then on re-runs deduplicates against previously posted bot comments (two-tier: positional match by hidden HTML anchor, or content match by title similarity within 50 lines)
6. Drops non-findings (see [Non-finding filter](#non-finding-filter)) and findings on files the model was never given (see [Unknown-file filter](#unknown-file-filter)), collapses findings that two phases reported on the same lines, then on re-runs deduplicates against previously posted bot comments (three-tier: positional match by hidden HTML anchor, content match by title similarity within 50 lines in the same file, or title-only match by high title similarity across any file)
7. Filters remaining findings by severity threshold, deduplicates overlapping findings within the run, and caps if configured
8. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback
9. Posts one review with inline comments (invisible body); beyond-diff findings post as standalone PR comments; every run upserts a status comment with cross-run totals
Expand Down
1 change: 1 addition & 0 deletions src/__tests__/orchestrate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2447,6 +2447,7 @@ describe("orchestrate", () => {
findingsSurvivedDedup: findings.length - 1,
droppedByPositional: 0,
droppedByContent: 1,
droppedByTitle: 0,
},
})
})
Expand Down
25 changes: 15 additions & 10 deletions src/orchestrate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,13 +418,14 @@ const describePhaseOutcome = (outcome: PhaseOutcome): PhaseStatus => {
/** A failed phase's billed attempts ride on the client's error; any other
* failure reached no provider and billed nothing. */
const phaseAttempts = (outcome: PhaseOutcome): PhaseAttempt[] => {
const attempts =
outcome.status === "completed"
? outcome.result.attempts
: outcome.error instanceof ReviewRequestError
? outcome.error.attempts
: []
return attempts.map((attempt) => ({ ...attempt, phase: outcome.phase.id }))
const tag = (attempts: StructuredReviewResult["attempts"]) => {
return attempts.map((attempt) => ({ ...attempt, phase: outcome.phase.id }))
}

if (outcome.status === "completed") return tag(outcome.result.attempts)
if (outcome.error instanceof ReviewRequestError)
return tag(outcome.error.attempts)
return []
}

type FilteredPhaseFindings = {
Expand Down Expand Up @@ -580,6 +581,7 @@ const runReviewPipeline = async (
? `${annotateDiff(reviewableFiles)}\n\n${excludedFilesNote}`
: annotateDiff(reviewableFiles)
const diffTokens = estimateTokens(annotatedDiff)
// The diff gets half the budget; the other half is for context files.
const budgetHalf = Math.floor(config.contextBudgetTokens / 2)
if (diffTokens > budgetHalf) {
return postSkipReview({
Expand Down Expand Up @@ -883,13 +885,15 @@ const runReviewPipeline = async (
// (anchor lines). Runs before the cap so duplicates don't consume slots.
const existingAnchors = [...inlineState.anchors, ...issueState.anchors]
const newFindings: Finding[] = []
const dedupCounts = { positional: 0, content: 0 }
const dedupCounts = { positional: 0, content: 0, title: 0 }
for (const finding of realFindings) {
const tier = classifyDuplicate(finding, existingAnchors)
if (tier) {
dedupCounts[tier]++
if (tier === "content") {
logger.info("content-tier dedup suppressed finding", {
// Positional is the common case and would be noisy — log only the
// higher tiers, which need the title evidence for diagnosis.
if (tier === "content" || tier === "title") {
logger.info(`${tier}-tier dedup suppressed finding`, {
file: finding.file,
line: finding.line,
category: finding.category,
Expand All @@ -909,6 +913,7 @@ const runReviewPipeline = async (
findingsSurvivedDedup: newFindings.length,
droppedByPositional: dedupCounts.positional,
droppedByContent: dedupCounts.content,
droppedByTitle: dedupCounts.title,
})

const {
Expand Down
251 changes: 245 additions & 6 deletions src/review/__tests__/comment-mapping.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"
import type { CommentableFile } from "../../diff/commentable-lines.js"
import {
buildStatusComment,
classifyDuplicate,
coalesceAnchors,
computeAnchorKey,
extractAnchors,
Expand Down Expand Up @@ -885,7 +886,7 @@ describe("isDuplicateFinding", () => {
).toBe(false)
})

it("rejects content match when line distance exceeds 50", () => {
it("content tier rejects >50 lines apart but title tier catches it", () => {
const anchors = [
{
file: "src/a.ts",
Expand All @@ -896,7 +897,7 @@ describe("isDuplicateFinding", () => {
]

expect(
isDuplicateFinding(
classifyDuplicate(
{
file: "src/a.ts",
category: "correctness",
Expand All @@ -905,7 +906,7 @@ describe("isDuplicateFinding", () => {
},
anchors,
),
).toBe(false)
).toBe("title")
})

it("skips content match when the finding has no title", () => {
Expand Down Expand Up @@ -988,7 +989,7 @@ describe("isDuplicateFinding", () => {
).toBe(true)
})

it("rejects content match when file differs", () => {
it("rejects content match when file differs but title tier catches it", () => {
const anchors = [
{
file: "src/a.ts",
Expand All @@ -999,7 +1000,7 @@ describe("isDuplicateFinding", () => {
]

expect(
isDuplicateFinding(
classifyDuplicate(
{
file: "src/b.ts",
category: "correctness",
Expand All @@ -1008,7 +1009,245 @@ describe("isDuplicateFinding", () => {
},
anchors,
),
).toBe(false)
).toBe("title")
})

// --- Title dedup tier ---

it("catches cross-file drift with identical title via title tier", () => {
const anchors = [
{
file: "src/orchestrate.ts",
category: "correctness",
line: 50,
title: "Unguarded RRule construction outside the try catch",
},
]

expect(
classifyDuplicate(
{
file: "src/config.ts",
category: "subtle_bugs",
line: 312,
title: "Unguarded RRule construction outside the try catch",
},
anchors,
),
).toBe("title")
})

it("catches same-file drift beyond 50 lines via title tier", () => {
const anchors = [
{
file: "src/a.ts",
category: "correctness",
line: 44,
title: "Missing validation before database insert",
},
]

expect(
classifyDuplicate(
{
file: "src/a.ts",
category: "correctness",
line: 118,
title: "Missing validation before database insert",
},
anchors,
),
).toBe("title")
})

it("rejects title tier when titles are dissimilar", () => {
const anchors = [
{
file: "src/a.ts",
category: "correctness",
line: 50,
title: "Missing null check on user.email",
},
]

expect(
classifyDuplicate(
{
file: "src/b.ts",
category: "correctness",
line: 50,
title: "Race condition in async handler teardown",
},
anchors,
),
).toBeNull()
})

it("rejects title tier at Jaccard 0.846, catches at 0.857", () => {
// 12 content words, 1 swap → intersection 11, union 13 → 0.846 < 0.85
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const anchors = [
{
file: "src/a.ts",
category: "correctness",
line: 50,
title:
"alpha bravo charlie delta echo foxtrot golf hotel india juliet kilo lima",
},
]

expect(
classifyDuplicate(
{
file: "src/b.ts",
category: "correctness",
line: 200,
title:
"alpha bravo charlie delta echo foxtrot golf hotel india juliet kilo mike",
},
anchors,
),
).toBeNull()

// 13 content words each, 1 swap → intersection 12, union 14 → 0.857 > 0.85
const swapAnchors = [
{
file: "src/a.ts",
category: "correctness",
line: 50,
title:
"alpha bravo charlie delta echo foxtrot golf hotel india juliet kilo lima november",
},
]
expect(
classifyDuplicate(
{
file: "src/b.ts",
category: "correctness",
line: 200,
title:
"alpha bravo charlie delta echo foxtrot golf hotel india juliet kilo lima mike",
},
swapAnchors,
),
).toBe("title")
})

it("catches title tier at exact 0.85 inclusive boundary", () => {
// 18 anchor words, 19 finding words, 17 shared → union 20, 17/20 = 0.85
const anchors = [
{
file: "src/a.ts",
category: "correctness",
line: 50,
title:
"alpha bravo charlie delta echo foxtrot golf hotel india juliet kilo lima mike november oscar papa quebec romeo",
},
]

expect(
classifyDuplicate(
{
file: "src/b.ts",
category: "correctness",
line: 201,
title:
"alpha bravo charlie delta echo foxtrot golf hotel india juliet kilo lima mike november oscar papa quebec victor whiskey",
},
anchors,
),
).toBe("title")
})

it("skips title tier when both titles have fewer than 3 content words", () => {
const anchors = [
{
file: "src/a.ts",
category: "correctness",
line: 50,
title: "Race condition",
},
]

expect(
classifyDuplicate(
{
file: "src/b.ts",
category: "correctness",
line: 50,
title: "Race condition",
},
anchors,
),
).toBeNull()
})

it("skips title tier when only the anchor title has fewer than 3 content words", () => {
const anchors = [
{
file: "src/a.ts",
category: "correctness",
line: 50,
title: "Race condition",
},
]

expect(
classifyDuplicate(
{
file: "src/b.ts",
category: "correctness",
line: 50,
title: "Race condition found during async handler teardown",
},
anchors,
),
).toBeNull()
})

it("positional tier wins when all three tiers would match", () => {
const anchors = [
{
file: "src/a.ts",
category: "correctness",
line: 50,
title: "Missing null check on user.email before access",
},
]

expect(
classifyDuplicate(
{
file: "src/a.ts",
category: "correctness",
line: 52,
title: "Missing null check on user.email before access",
},
anchors,
),
).toBe("positional")
})

it("content tier wins over title tier for same-file near-line match", () => {
const anchors = [
{
file: "src/a.ts",
category: "correctness",
line: 50,
title: "Missing null check on user.email before access",
},
]

expect(
classifyDuplicate(
{
file: "src/a.ts",
category: "subtle_bugs",
line: 70,
title: "Null check missing on user.email before access",
},
anchors,
),
).toBe("content")
})
})

Expand Down
Loading