From 3cfdacfef743fb2f88374b03c26d50096ba3b0cc Mon Sep 17 00:00:00 2001 From: Luke Date: Tue, 14 Jul 2026 11:36:46 -0700 Subject: [PATCH 1/2] Quiet PR-level findings: threads over prose, confidence over volume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #76 gave findings with no anchorable diff line a home so a REQUEST_CHANGES review could never read "Actionable comments posted: 0" with nothing to act on. That invariant is right and stays. But the home it chose — prose in the review summary body — is the one DiffSentry channel a reader cannot resolve, reply to, or collapse, and four things funnelled into it: - Every warning-level drift finding was stamped `severity: "major", confidence: "medium"`, and isNitpick never consulted confidence, so all drift counted as actionable and claimed a Major slot at the top. - parse.ts demoted ANY un-anchorable major/critical inline finding there — findings that wanted a line and missed. - submitReview creates a new review per push, and the cross-review dedup keys on fingerprintFor(path, line, title). PR-level findings have line 0 and often no path, leaving the title as the whole key — and drift titles are free prose regenerated per run. One re-wording minted a fresh fingerprint and the finding reprinted in every review body. - The dismissal pass only handled CHANGES_REQUESTED, but drift bumps APPROVE→COMMENT and never blocks — so the exact review class #76 introduced was the one class never cleaned up. Route findings to a channel by what the channel costs the reader: - github: post file-scoped findings as real resolvable threads (subject_type: "file"); post them BEFORE the review so any GitHub rejects fold back into the body and a finding is never lost between channels. Retire our superseded reviews — dismiss CHANGES_REQUESTED, and stub COMMENTED bodies via updateReview since GitHub refuses to dismiss them and submitted reviews can't be deleted. Match on our own body marker, not `user.type === "Bot"`, which reached other bots. - review-body: split prLevel by whether a path survived (thread-able vs body-only); gate the body section on high confidence and collapse the rest; add isVisiblyActionable as the single source of truth for both the count and the REQUEST_CHANGES invariant, so the two can't disagree. - drift: ask for confidence and carry it through instead of hardcoding "medium"; default an omitted value to "medium", not the "high" ReviewComment assumes. severity stays "major" — severity is how bad if real, confidence is whether it's real, and conflating them caused this. Only high-confidence drift may withhold an APPROVE. - parse/state: titleSimilarity (Jaccard over content words, 0.6) catches re-worded PR-level repeats that fingerprinting misses, backed by a capped postedPrLevelKeys; same-path-only, so a real finding is never swallowed by a same-sounding one about different code. On the review that prompted this, "Actionable comments posted: 3" becomes 1 and both Major/Medium drift findings collapse — still present, no longer shouting. Adds tests/unit/pr-level-noise.test.ts (29 tests). 361 unit tests pass, typecheck and lint clean. The subject_type:"file" call and review retirement are covered by mocked-Octokit tests, not a live PR — worth watching on the first real review after this ships. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ai/parse.ts | 75 +++++++ src/ai/prompt.ts | 1 + src/drift.ts | 19 +- src/github.ts | 156 +++++++++++-- src/review-body.ts | 134 ++++++++++-- src/reviewer.ts | 70 +++++- src/types.ts | 15 +- src/walkthrough-state.ts | 5 + tests/unit/pr-level-noise.test.ts | 352 ++++++++++++++++++++++++++++++ 9 files changed, 767 insertions(+), 60 deletions(-) create mode 100644 tests/unit/pr-level-noise.test.ts diff --git a/src/ai/parse.ts b/src/ai/parse.ts index ee32f28..b610b1d 100644 --- a/src/ai/parse.ts +++ b/src/ai/parse.ts @@ -142,6 +142,81 @@ export function fingerprintFor(path: string, line: number, title: string): strin .slice(0, 12); } +/** Words too common to carry meaning in a finding title — including the negations + * and auxiliaries that flip freely between re-runs ("does not" ⇄ "doesn't"). */ +const TITLE_STOPWORDS = new Set([ + "the", "and", "for", "not", "but", "its", "it", "is", "are", "was", "were", "be", "been", + "does", "doesnt", "dont", "did", "didnt", "do", "has", "have", "had", "can", "cant", + "will", "wont", "this", "that", "these", "those", "with", "from", "into", "than", "then", + "when", "while", "which", "who", "whose", "what", "any", "all", "only", "still", "also", + "there", "their", "they", "you", "your", "our", "via", "per", "out", "off", "own", +]); + +/** Content tokens of a finding title, for similarity matching. */ +function titleTokens(title: string): Set { + return new Set( + normalizeForFingerprint(title) + .split(" ") + .filter((t) => t.length > 2 && !TITLE_STOPWORDS.has(t)), + ); +} + +/** + * Jaccard similarity (0..1) over the content words of two finding titles. + * + * Exists because fingerprintFor can't dedup PR-level findings across reviews. + * An inline finding is pinned by `path:line`, so its fingerprint is stable even + * when the model re-words the title. A PR-level finding has line 0 and often no + * path, leaving the title as effectively the whole key — and PR-level titles are + * free prose regenerated by the model on every run. One re-wording ("tk02 does + * not change the default" → "tk02 never sets the default") mints a fresh + * fingerprint, dedup misses, and the finding reprints in every review body. + * Comparing meaning-bearing tokens instead of hashing exact strings survives + * that. + */ +export function titleSimilarity(a: string, b: string): number { + const ta = titleTokens(a); + const tb = titleTokens(b); + if (ta.size === 0 || tb.size === 0) return 0; + let intersection = 0; + for (const t of ta) if (tb.has(t)) intersection++; + return intersection / (ta.size + tb.size - intersection); +} + +/** + * Tuned against the failure it exists to stop: the same drift finding re-worded + * between runs. Distinct PR-level findings on one PR name different files and + * symbols (the drift prompt demands specifics), so they overlap well below this; + * re-wordings of one finding keep their nouns and land well above it. Set + * deliberately short of aggressive — a missed repeat is a duplicate paragraph in + * a collapsed block, while an over-eager match silently swallows a real finding. + */ +const PR_LEVEL_REPEAT_THRESHOLD = 0.6; + +/** Serialized prior-finding key: path (may be empty) + title. */ +export function prLevelRepeatKey(path: string, title: string): string { + return `${path}\t${title}`; +} + +/** + * Whether a PR-level finding restates one already posted on a previous review. + * Only compares findings scoped to the same file (or both unscoped), so a real + * finding is never swallowed by a same-sounding one about different code. + */ +export function isRepeatPrLevelFinding( + candidate: { path: string; title?: string }, + priorKeys: string[], +): boolean { + const title = candidate.title?.trim(); + if (!title) return false; + return priorKeys.some((key) => { + const tab = key.indexOf("\t"); + if (tab === -1) return false; + if (key.slice(0, tab) !== candidate.path) return false; + return titleSimilarity(key.slice(tab + 1), title) >= PR_LEVEL_REPEAT_THRESHOLD; + }); +} + function stripFences(input: string): string { let s = input.trim(); s = s.replace(/^```(?:\w+)?\s*\n?/, ""); diff --git a/src/ai/prompt.ts b/src/ai/prompt.ts index 429de62..944b9ec 100644 --- a/src/ai/prompt.ts +++ b/src/ai/prompt.ts @@ -37,6 +37,7 @@ You MUST respond with valid JSON matching this schema: Rules for the JSON response: - "line" must be a line number that appears in the diff (from the + side of the patch). - "prLevelComments" is for findings that are NOT tied to one specific changed line and therefore cannot be an inline comment. Use it — do NOT invent a line number or bury the finding only in the summary — for: the diff contradicting the PR description (a claimed change is missing, or the code does something the description doesn't mention), issues spanning many files, or concerns about the change as a whole. Each entry has NO "path"/"line". Same title/body/type/severity/aiAgentPrompt/confidence fields as inline comments. Omit the field or use [] when there are none. +- Reserve "prLevelComments" for findings that genuinely have no home in the diff. Unlike an inline comment, a reader cannot resolve one, reply to it, or collapse it — it stays in the review body permanently — so a high-confidence entry here is the most expensive thing you can emit. If a finding CAN be pinned to a changed line, always prefer "comments". Rate "confidence" honestly: a "medium" entry still reaches the reviewer, just without claiming the top of the review. - A REQUEST_CHANGES verdict MUST be backed by at least one concrete finding — an inline "comments" entry or a "prLevelComments" entry. Never request changes while leaving both arrays empty and describing the problem only in "summary". - "path" must exactly match a filename from the changed files. - "title" is REQUIRED on every comment. One sentence, ends with a period, no markdown formatting. This becomes the bold headline. diff --git a/src/drift.ts b/src/drift.ts index 7d5d0bb..4511780 100644 --- a/src/drift.ts +++ b/src/drift.ts @@ -1,4 +1,4 @@ -import type { AIProvider, FileChange, LicenseHeaderConfig, PRContext } from "./types.js"; +import type { AIProvider, Confidence, FileChange, LicenseHeaderConfig, PRContext } from "./types.js"; import { minimatch } from "minimatch"; /** @@ -10,6 +10,12 @@ export type DriftFinding = { level: "warning" | "info"; summary: string; details: string; + /** How sure the model is that this is real drift rather than a description + * that's merely terse. Drift is diagnosed by comparing prose to code, so it + * is unusually prone to confident-sounding false positives — the reviewer + * fold in reviewer.ts carries this through to the finding, and the review + * body only gives high-confidence drift a prominent, uncollapsed section. */ + confidence: Confidence; }; export async function detectDescriptionDrift(opts: { @@ -28,6 +34,7 @@ export async function detectDescriptionDrift(opts: { details: "The PR description has less than 30 characters of meaningful content. " + "Consider expanding it so reviewers can see at a glance what changed and why.", + confidence: "high", }, ]; } @@ -38,11 +45,14 @@ Respond with ONLY a JSON array (no prose, no code fences). Each entry: { "level": "warning" | "info", "summary": "one-line summary", - "details": "1-3 sentences with specifics referencing files/symbols" + "details": "1-3 sentences with specifics referencing files/symbols", + "confidence": "high" | "medium" | "low" } Use "warning" only when the drift is meaningful: missing critical changes, contradictory claims, or unsupported features named. Use "info" for minor omissions. If the description matches the diff well, return an empty array []. +"confidence" is how sure you are the drift is REAL, judged separately from how bad it would be. Use "high" ONLY when the description makes a concrete claim the diff plainly contradicts, and you verified both sides. Use "medium" when the reading depends on intent you can't see — an author's shorthand, an omission that may be deliberate, a description that's incomplete rather than wrong. Use "low" for a hypothesis worth raising but not standing behind. A description being terse or informal is NOT drift. Prefer "medium" when torn: a wrong high-confidence finding costs the reviewer more than a hedged one. + Be specific — name files and identifiers, don't say "various changes".`; const raw = await opts.ai.chat(opts.context, ask); @@ -56,6 +66,11 @@ Be specific — name files and identifiers, don't say "various changes".`; level: f.level === "info" ? "info" : "warning", summary: String(f.summary).slice(0, 200), details: String(f.details ?? "").slice(0, 600), + // Default to "medium", NOT the "high" that ReviewComment defaults to: + // an omitted confidence here means the model didn't engage with the + // question, and drift is the finding class least entitled to the + // benefit of the doubt. Only an explicit "high" earns the prominent slot. + confidence: f.confidence === "high" || f.confidence === "low" ? f.confidence : "medium", })) as DriftFinding[]; } catch { return []; diff --git a/src/github.ts b/src/github.ts index 0be4189..0a46704 100644 --- a/src/github.ts +++ b/src/github.ts @@ -1,7 +1,33 @@ import { createAppAuth } from "@octokit/auth-app"; import { Octokit } from "@octokit/rest"; -import { Config, FileChange, PRContext, ReviewResult, IssueContext, IssueComment } from "./types.js"; +import { Config, FileChange, PRContext, ReviewComment, ReviewResult, IssueContext, IssueComment } from "./types.js"; import { logger } from "./logger.js"; +import { isFileLevelFinding, REVIEW_BODY_MARKER } from "./review-body.js"; + +type Logger = typeof logger; + +/** Body a superseded COMMENTED review is rewritten to. Keeps the timeline entry + * honest (the review did happen) without leaving its full text competing with + * the current one. Carries no REVIEW_BODY_MARKER, so a later run doesn't + * re-retire what's already retired. */ +const SUPERSEDED_REVIEW_STUB = + "🛡️ This DiffSentry review has been superseded by a newer one. Its findings, if still present, appear there."; + +/** Findings GitHub refused as file-scoped threads, rendered into the review body + * so their substance still reaches the reviewer. Rare — the file usually left + * the diff mid-review. */ +function renderFileLevelFallbackSection(comments: ReviewComment[]): string { + const blocks = comments + .map((c) => `**\`${c.path}\`**\n\n${c.body.trim()}`) + .join("\n\n---\n\n"); + return [ + `### 🔎 Findings that couldn't be attached to their file (${comments.length})`, + "", + "DiffSentry couldn't open a review thread on these files — they may no longer be part of the diff.", + "", + blocks, + ].join("\n"); +} // ─── Rate-limit / transient-error backoff ────────────────────────── // Every Octokit instance this module hands out (installation- and App-level) @@ -544,43 +570,127 @@ export class GitHubClient { }; } - async submitReview( - installationId: number, + /** + * Clear DiffSentry's own superseded reviews off the PR before adding another. + * + * Two states, two mechanisms, because GitHub only lets you dismiss a review + * that carries a verdict: + * - CHANGES_REQUESTED → dismissReview, which strikes it through and drops + * the block. + * - COMMENTED → NOT dismissable (the API rejects it), and submitted reviews + * can't be deleted. The body is editable though, so rewrite it to a + * one-line stub. Without this, every commented review — the state a + * drift-only review lands in — keeps its full text on the timeline + * forever, and each push adds another. + * + * Matching is on our own body marker, not `user.type === "Bot"`, which would + * have us dismissing other bots' reviews. + */ + private async retireSupersededReviews( + octokit: Octokit, context: PRContext, - result: ReviewResult, - signal?: AbortSignal + log: Logger, ): Promise { - const octokit = await this.getInstallationOctokit(installationId, signal); - const log = logger.child({ owner: context.owner, repo: context.repo, pr: context.pullNumber }); - - // Dismiss previous DiffSentry reviews so we don't pile up stale comments try { - const reviews = await octokit.pulls.listReviews({ + const reviews = await octokit.paginate(octokit.pulls.listReviews, { owner: context.owner, repo: context.repo, pull_number: context.pullNumber, }); - const botReviews = reviews.data.filter( - (r) => r.user?.type === "Bot" && r.state === "CHANGES_REQUESTED" - ); - for (const review of botReviews) { - await octokit.pulls.dismissReview({ + const ours = reviews.filter((r) => r.body?.includes(REVIEW_BODY_MARKER)); + + for (const review of ours) { + try { + if (review.state === "CHANGES_REQUESTED") { + await octokit.pulls.dismissReview({ + owner: context.owner, + repo: context.repo, + pull_number: context.pullNumber, + review_id: review.id, + message: "Superseded by a newer DiffSentry review.", + }); + } else if (review.state === "COMMENTED") { + await octokit.pulls.updateReview({ + owner: context.owner, + repo: context.repo, + pull_number: context.pullNumber, + review_id: review.id, + body: SUPERSEDED_REVIEW_STUB, + }); + } + } catch (err) { + // One un-retirable review must not block the new one from posting. + log.debug({ err, reviewId: review.id, state: review.state }, "Could not retire superseded review"); + } + } + } catch { + log.warn("Could not list previous reviews to retire (may lack permission)"); + } + } + + /** + * Post file-scoped findings as their own resolvable review threads. + * Returns the findings GitHub refused, for the caller to fold back into the + * review body — most often because the file left the diff since the review + * started. + */ + private async postFileLevelComments( + octokit: Octokit, + context: PRContext, + comments: ReviewComment[], + log: Logger, + ): Promise { + const unpostable: ReviewComment[] = []; + for (const c of comments) { + try { + await octokit.pulls.createReviewComment({ owner: context.owner, repo: context.repo, pull_number: context.pullNumber, - review_id: review.id, - message: "Superseded by new review.", + commit_id: context.headSha, + path: c.path, + body: c.body, + subject_type: "file", }); + } catch (err) { + log.warn({ err, path: c.path }, "File-level comment rejected; folding into review body"); + unpostable.push(c); } - } catch { - log.warn("Could not dismiss previous reviews (may lack permission)"); } + return unpostable; + } + + async submitReview( + installationId: number, + context: PRContext, + result: ReviewResult, + signal?: AbortSignal + ): Promise { + const octokit = await this.getInstallationOctokit(installationId, signal); + const log = logger.child({ owner: context.owner, repo: context.repo, pr: context.pullNumber }); + + await this.retireSupersededReviews(octokit, context, log); // Submit the new review const validComments = result.comments.filter((c) => c.line > 0 && c.path); + // File-scoped findings: the model named a file but no line we could anchor + // to the diff. GitHub hosts these as real review threads via + // `subject_type: "file"` — resolvable, repliable, and collapsible like any + // inline comment — so post them as threads instead of as permanent prose in + // the review body. They go FIRST: any that GitHub rejects are folded back + // into the body below, so a finding is never silently lost between the two + // channels (the failure #76 set out to fix). + const fileLevelComments = result.comments.filter(isFileLevelFinding); + const unpostable = await this.postFileLevelComments(octokit, context, fileLevelComments, log); + + let body = result.summary; + if (unpostable.length > 0) { + body += "\n\n" + renderFileLevelFallbackSection(unpostable); + } + // GitHub rejects reviews with empty body and no comments (422) - if (!result.summary && validComments.length === 0) { + if (!body && validComments.length === 0) { log.warn("Skipping review submission: no summary and no comments"); return; } @@ -592,7 +702,7 @@ export class GitHubClient { pull_number: context.pullNumber, commit_id: context.headSha, event: result.approval, - body: result.summary, + body, comments: validComments.map((c) => ({ path: c.path, line: c.line, @@ -601,7 +711,7 @@ export class GitHubClient { })), }); log.info( - { comments: validComments.length, event: result.approval }, + { comments: validComments.length, fileLevel: fileLevelComments.length - unpostable.length, event: result.approval }, "Review submitted" ); } catch (err: any) { @@ -618,7 +728,7 @@ export class GitHubClient { pull_number: context.pullNumber, commit_id: context.headSha, event: result.approval, - body: `${result.summary}\n\n---\n\n## Inline Comments\n\n${commentBlock}`, + body: `${body}\n\n---\n\n## Inline Comments\n\n${commentBlock}`, }); } else { throw err; diff --git a/src/review-body.ts b/src/review-body.ts index 306070c..d237fe8 100644 --- a/src/review-body.ts +++ b/src/review-body.ts @@ -1,5 +1,6 @@ import { randomUUID } from "node:crypto"; import type { + Confidence, PRContext, ReviewComment, ReviewResult, @@ -31,7 +32,7 @@ export type ReviewBodyMeta = { incrementalFromSha?: string; }; -const REVIEW_BODY_MARKER = ""; +export const REVIEW_BODY_MARKER = ""; /** * Honest banner for the parse-failure path. When the AI's response can't be @@ -67,18 +68,65 @@ export function isNitpick(c: ReviewComment): boolean { } /** - * A REQUEST_CHANGES verdict must be backed by at least one actionable finding - * (inline OR PR-level; nitpicks/suggestions don't count). When every backing - * finding was dropped by line-anchoring/verification — or the model requested - * changes while describing the problem only in the summary — return COMMENT so - * the verdict and the visible findings never contradict each other. Pure and - * one-directional: only ever relaxes a block, never creates one. + * The two flavours of `prLevel` finding, distinguished by whether a `path` + * survived. Both carry line 0 and are excluded from inline posting. + * + * - FILE-level (`path` set): the model located the finding in a specific file + * but on a line we couldn't anchor to the diff (see the demotion path in + * ai/parse.ts). GitHub can host these as real, resolvable file-scoped review + * threads (`subject_type: "file"`), so they are posted as threads rather than + * rendered into the review body. + * - BODY-level (`path` empty): no file to attach to at all — the diff versus the + * PR description, or a concern spanning the whole change. GitHub has nowhere + * to hang a thread, so these are the only findings that must live as prose in + * the review body. + */ +export function isFileLevelFinding(c: ReviewComment): boolean { + return c.prLevel === true && !!c.path && c.line === 0; +} + +export function isPrBodyFinding(c: ReviewComment): boolean { + return c.prLevel === true && !c.path; +} + +/** Confidence with the documented default applied (see ai/prompt.ts: an omitted + * confidence means the model was sure enough not to qualify the finding). */ +export function confidenceOf(c: ReviewComment): Confidence { + return c.confidence ?? "high"; +} + +/** + * Whether a finding is actionable AND lands somewhere the reader can act on it. + * + * Inline and file-level findings become resolvable threads, so being actionable + * is enough. Body-level findings are unresolvable prose in the review summary — + * the one place noise cannot be dismissed — so they must additionally be + * high-confidence to claim that space. A medium/low-confidence body finding + * still renders, but in a collapsed block and without inflating the count. + * + * Single source of truth for both the "Actionable comments posted" count and the + * REQUEST_CHANGES invariant, so the number in the header and the verdict can + * never disagree about what counts. + */ +export function isVisiblyActionable(c: ReviewComment): boolean { + if (isNitpick(c)) return false; + if (isPrBodyFinding(c)) return confidenceOf(c) === "high"; + return true; +} + +/** + * A REQUEST_CHANGES verdict must be backed by at least one finding the reader + * can actually see and act on. When every backing finding was dropped by + * line-anchoring/verification, demoted into a collapsed low-confidence block, or + * the model requested changes while describing the problem only in the summary, + * return COMMENT so the verdict and the visible findings never contradict each + * other. Pure and one-directional: only ever relaxes a block, never creates one. */ export function reconcileApproval( approval: ReviewResult["approval"], comments: ReviewComment[], ): ReviewResult["approval"] { - if (approval === "REQUEST_CHANGES" && !comments.some((c) => !isNitpick(c))) { + if (approval === "REQUEST_CHANGES" && !comments.some(isVisiblyActionable)) { return "COMMENT"; } return approval; @@ -130,13 +178,17 @@ function renderNitpickEntry(c: ReviewComment): string { } /** - * Findings not tied to a specific changed line (prLevel) — e.g. the diff - * contradicts the PR description, a claimed change is missing, or a cross-cutting - * concern. They can't be posted as GitHub inline comments, so without this - * section they'd be invisible: the review would read as "changes requested / 0 - * actionable comments" with the substance only hinted at in the summary. Each - * comment's `body` is already the fully-formatted standalone block (type/severity - * header, title, prose, agent prompt), so we render it directly. + * Body-level findings with nowhere to hang a thread — the diff contradicts the + * PR description, a claimed change is missing, a concern spans the whole change. + * Without this section they'd be invisible: the review would read as "changes + * requested / 0 actionable comments" with the substance only hinted at in the + * summary. Each comment's `body` is already the fully-formatted standalone block + * (type/severity header, title, prose, agent prompt), so we render it directly. + * + * Deliberately narrow. This is the only DiffSentry output a reader cannot + * resolve, reply to, or collapse — it reprints in full on every subsequent + * review — so entry is gated on high confidence by isVisiblyActionable. + * Everything else goes to renderUncertainPrLevelSection below. */ function renderPrLevelSection(prLevel: ReviewComment[]): string { if (prLevel.length === 0) return ""; @@ -150,6 +202,28 @@ function renderPrLevelSection(prLevel: ReviewComment[]): string { ].join("\n"); } +/** + * Body-level findings the model itself flagged as medium/low confidence. They're + * hypotheses that depend on intent the model can't see, so they're worth + * surfacing but not worth the top of the review: collapsed, and excluded from + * the actionable count. Same treatment the nitpick collapse gives uncertain + * inline findings. + */ +function renderUncertainPrLevelSection(prLevel: ReviewComment[]): string { + if (prLevel.length === 0) return ""; + const blocks = prLevel.map((c) => c.body.trim()).join("\n\n---\n\n"); + return [ + `
`, + `🤔 Lower-confidence observations about the change as a whole (${prLevel.length})
`, + "", + "DiffSentry is unsure about these — they depend on intent it can't verify from the diff. Worth a glance, not a blocker.", + "", + blocks, + "", + `
`, + ].join("\n"); +} + function renderNitpicksSection(nitpicks: ReviewComment[]): string { if (nitpicks.length === 0) return ""; @@ -342,16 +416,25 @@ export function formatReviewBody( result: ReviewResult, meta: ReviewBodyMeta, ): string { - // Three buckets. PR-level findings (no diff-line anchor) are split out first - // so they render in their own section and never leak into the inline - // nitpick/actionable collapses. The "posted" count spans inline actionable + - // actionable PR-level findings, so a blocking review whose only finding is - // PR-level no longer reads as "Actionable comments posted: 0". - const prLevel = result.comments.filter((c) => c.prLevel); + // Buckets, by where each finding ends up in the rendered PR. + // + // inline → resolvable line threads (posted by submitReview) + // fileLevel → resolvable file threads (posted by submitReview); NOT rendered + // here, or they'd say everything twice + // prBody → prose in this body; the only unresolvable channel, so it's + // split by confidence: high gets its own section, the rest + // collapses + // + // The "posted" count spans everything visibly actionable across all three, so + // a blocking review whose only finding is unanchored still reads honestly + // instead of "Actionable comments posted: 0". + const fileLevel = result.comments.filter(isFileLevelFinding); + const prBody = result.comments.filter(isPrBodyFinding); const inline = result.comments.filter((c) => !c.prLevel); - const actionable = inline.filter((c) => !isNitpick(c)); const nitpicks = inline.filter(isNitpick); - const actionableCount = actionable.length + prLevel.filter((c) => !isNitpick(c)).length; + const prBodyProminent = prBody.filter(isVisiblyActionable); + const prBodyUncertain = prBody.filter((c) => !isVisiblyActionable(c)); + const actionableCount = [...inline, ...fileLevel, ...prBody].filter(isVisiblyActionable).length; const runId = randomUUID(); const sections: string[] = []; @@ -375,12 +458,15 @@ export function formatReviewBody( sections.push(result.summary.trim()); } - const prLevelBlock = renderPrLevelSection(prLevel); + const prLevelBlock = renderPrLevelSection(prBodyProminent); if (prLevelBlock) sections.push(prLevelBlock); const nitpicksBlock = renderNitpicksSection(nitpicks); if (nitpicksBlock) sections.push(nitpicksBlock); + const uncertainBlock = renderUncertainPrLevelSection(prBodyUncertain); + if (uncertainBlock) sections.push(uncertainBlock); + // Only inline comments feed the bulk agent prompt — its entries are keyed by // `path`/`line`, which PR-level findings don't have (their agent prompt is // already shown inline in the PR-level section above). diff --git a/src/reviewer.ts b/src/reviewer.ts index 0c8f439..c9b643c 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -9,7 +9,12 @@ import { formatWalkthrough, formatWalkthroughInner, wrapWalkthroughCollapse, for import { parseCommand, formatHelpMessage, formatConfigMessage } from "./commands.js"; import { parseIssueCommand, formatIssueHelpMessage } from "./issue-commands.js"; import { buildIssueSummaryInstruction, buildIssuePlanInstruction } from "./ai/prompt.js"; -import { synthesizeReviewSummary, buildReviewComment } from "./ai/parse.js"; +import { + synthesizeReviewSummary, + buildReviewComment, + isRepeatPrLevelFinding, + prLevelRepeatKey, +} from "./ai/parse.js"; import { verifyFindings } from "./ai/verify.js"; import { LearningsStore, synthesizeLearning, extractFindingMeta, type FindingContext } from "./learnings.js"; import { loadGuidelines, getRelevantGuidelines, formatGuidelinesForPrompt } from "./guidelines.js"; @@ -591,6 +596,7 @@ export class Reviewer { getWalkthroughState(owner, repo, pullNumber) ?? (priorComment ? extractState(priorComment.body) : null); const priorFingerprints = new Set(priorState?.postedFingerprints ?? []); + const priorPrLevelKeys = priorState?.postedPrLevelKeys ?? []; // Classify each file against prior state const currentFileShas: Record = {}; @@ -1025,10 +1031,19 @@ export class Reviewer { // line to anchor to, so they belong in the PR-level channel (visible + // actionable) rather than buried as an info block in the walkthrough. This // also gives the REQUEST_CHANGES invariant below a deterministic backstop. - // Non-blocking on their own: they bump APPROVE→COMMENT but never force - // REQUEST_CHANGES. Info-level drift (incl. the "description too short" - // sentinel) stays in the walkthrough block. Deduped cross-review by - // fingerprint alongside every other finding. + // Non-blocking on their own: high-confidence drift bumps APPROVE→COMMENT, + // but none of it ever forces REQUEST_CHANGES. Info-level drift (incl. the + // "description too short" sentinel) stays in the walkthrough block. + // Deduped cross-review by fingerprint and by title similarity alongside + // every other finding. + // + // severity and confidence are orthogonal here and must not be conflated: + // "major" says drift matters IF real (the drift prompt only emits + // level: "warning" for meaningful discrepancies), while the model's own + // confidence says whether it IS real. Only confidence may decide whether a + // finding claims the prominent, unresolvable section of the review body; + // pinning it to a constant here hands every drift warning a Major slot at + // the top of the review regardless of how sure the model was. const driftWarnings = driftFindings.filter((f) => f.level === "warning"); if (driftWarnings.length > 0) { for (const f of driftWarnings) { @@ -1043,16 +1058,23 @@ export class Reviewer { type: "issue", severity: "major", aiAgentPrompt: `Reconcile the PR description with the actual diff. ${f.summary} ${f.details}`.trim(), - confidence: "medium", + confidence: f.confidence, }, { path: "", line: 0, prLevel: true }, ), ); } - if (reviewResult.approval === "APPROVE") { + // Withholding an approval is a claim about the change, so only drift we + // actually stand behind may do it. A hedged observation still renders + // (collapsed, uncounted) — but it rides along with the APPROVE rather + // than quietly turning every clean PR into a commented one. + if (reviewResult.approval === "APPROVE" && driftWarnings.some((f) => f.confidence === "high")) { reviewResult.approval = "COMMENT"; } - log.info({ count: driftWarnings.length }, "Folded description-drift warnings into PR-level findings"); + log.info( + { count: driftWarnings.length, highConfidence: driftWarnings.filter((f) => f.confidence === "high").length }, + "Folded description-drift warnings into PR-level findings", + ); } const risk = assessRisk({ files: context.files, @@ -1253,6 +1275,17 @@ export class Reviewer { ...reviewResult.comments.map((c) => c.fingerprint).filter((x): x is string => !!x), ]), ), + // Trailing window, most-recent last: a PR-level finding that stopped + // recurring 50 findings ago is not worth suppressing forever, and the + // list is compared token-wise (not hashed), so it has to stay bounded. + postedPrLevelKeys: Array.from( + new Set([ + ...(priorState?.postedPrLevelKeys ?? []), + ...reviewResult.comments + .filter((c) => c.prLevel && c.title) + .map((c) => prLevelRepeatKey(c.path, c.title!)), + ]), + ).slice(-50), filesProcessed: context.files.map((f) => f.filename), filesSkippedSimilar, filesSkippedTrivial, @@ -1346,6 +1379,27 @@ export class Reviewer { }); } + // Second dedup pass, PR-level only. The fingerprint pass above catches a + // repeat only when the model re-words nothing; a PR-level finding has no + // path:line to pin its fingerprint, so any re-phrasing between runs slips + // through and the finding reprints in every review body. Match on title + // similarity instead. Inline findings never reach here — their fingerprint + // is already stable, and near-miss matching would be a real dedup risk + // across the many findings a single file can carry. + if (priorPrLevelKeys.length > 0) { + const before = reviewResult.comments.length; + reviewResult.comments = reviewResult.comments.filter((c) => { + if (!c.prLevel) return true; + if (isRepeatPrLevelFinding(c, priorPrLevelKeys)) { + log.debug({ path: c.path, title: c.title }, "Dropping re-worded PR-level finding posted on an earlier review"); + return false; + } + return true; + }); + const dropped = before - reviewResult.comments.length; + if (dropped > 0) log.info({ dropped }, "Suppressed re-worded PR-level repeats"); + } + // Opt-in: suppress findings whose fingerprint a human has dismissed (or // currently snoozed) in the command center. Off by default — gated on // DIFFSENTRY_SUPPRESS_DISMISSED so triage data never silently changes diff --git a/src/types.ts b/src/types.ts index 9feb6b9..8e16bd3 100644 --- a/src/types.ts +++ b/src/types.ts @@ -301,9 +301,18 @@ export interface ReviewComment { /** A finding NOT tied to a specific changed line — e.g. the diff contradicts * the PR description, a claimed change is missing, or a cross-cutting concern * spans the whole PR. These carry title/body/severity but no meaningful - * `line` (conventionally 0), are never posted as GitHub inline comments - * (submitReview's `line > 0` filter excludes them), and are rendered in a - * dedicated "not tied to a specific line" section of the review body. */ + * `line` (conventionally 0) and are never posted as inline comments + * (submitReview's `line > 0` filter excludes them). + * + * `path` then decides where the finding surfaces, because it decides whether + * GitHub can host a thread for it (see isFileLevelFinding / isPrBodyFinding + * in review-body.ts): + * - path set → posted as a resolvable file-scoped review thread + * (`subject_type: "file"`). + * - path empty → no file to attach to, so it renders as prose in the review + * body — the one channel a reader can't resolve or collapse, + * which is why entry there additionally requires high + * `confidence`. */ prLevel?: boolean; /** Set by the pattern engine so callers can record the hit source without * re-sniffing the rendered body. "builtin" = shipped heuristic; "custom" = diff --git a/src/walkthrough-state.ts b/src/walkthrough-state.ts index fdef8c0..e9e90ad 100644 --- a/src/walkthrough-state.ts +++ b/src/walkthrough-state.ts @@ -26,6 +26,11 @@ export interface WalkthroughState { fileShas?: Record; /** Inline-comment fingerprints already posted (for dedup). */ postedFingerprints?: string[]; + /** Keys (`path \t title`) of PR-level findings already posted, for similarity + * dedup. Separate from postedFingerprints because a PR-level finding has no + * `path:line` to pin its fingerprint — see isRepeatPrLevelFinding in + * ai/parse.ts. Capped at the most recent 50; older entries fall off. */ + postedPrLevelKeys?: string[]; /** Files we processed (passed path filters, had reviewable changes). */ filesProcessed?: string[]; /** Files skipped because their content matches the previously reviewed snapshot. */ diff --git a/tests/unit/pr-level-noise.test.ts b/tests/unit/pr-level-noise.test.ts new file mode 100644 index 0000000..9ccd72c --- /dev/null +++ b/tests/unit/pr-level-noise.test.ts @@ -0,0 +1,352 @@ +import { describe, it, expect, vi } from "vitest"; +import { formatReviewBody, reconcileApproval, isVisiblyActionable } from "../../src/review-body.js"; +import { titleSimilarity, isRepeatPrLevelFinding, prLevelRepeatKey } from "../../src/ai/parse.js"; +import { detectDescriptionDrift } from "../../src/drift.js"; +import { GitHubClient } from "../../src/github.js"; +import type { Config, PRContext, ReviewComment, ReviewResult } from "../../src/types.js"; + +// Companion to pr-level-findings.test.ts (which covers PR #76's "a block must +// name a finding" invariant). This file covers the noise controls layered on +// top: PR-level findings only claim the unresolvable review body when they're +// high-confidence and file-scoped ones become real threads instead. + +const META = { + profile: "chill", + owner: "o", + repo: "r", + headSha: "deadbee", + baseBranch: "main", + headBranch: "feat", + filesProcessed: ["src/a.ts"], + botName: "diffsentry", +}; + +function result(over: Partial = {}): ReviewResult { + return { summary: "s", comments: [], approval: "COMMENT", ...over }; +} + +/** Body-level: no path — nowhere to hang a thread (drift, whole-PR concerns). */ +function bodyFinding(over: Partial = {}): ReviewComment { + return { + path: "", + line: 0, + side: "RIGHT", + body: "the description claims a default change the diff never makes", + type: "issue", + severity: "major", + title: "tk02 does not change the DB status column default to not_started", + prLevel: true, + ...over, + }; +} + +/** File-level: path survived, line didn't — postable as a file-scoped thread. */ +function fileFinding(over: Partial = {}): ReviewComment { + return { + path: "src/a.ts", + line: 0, + side: "RIGHT", + body: "edit-mode fields were removed from the lead detail view", + type: "issue", + severity: "major", + title: "Lead detail removes editable Qualification Long Form", + prLevel: true, + ...over, + }; +} + +describe("review-body: confidence gates the unresolvable section", () => { + it("gives a high-confidence body finding the prominent section and counts it", () => { + const body = formatReviewBody(result({ comments: [bodyFinding({ confidence: "high" })] }), META); + expect(body).toContain("Issues not tied to a specific line (1)"); + expect(body).toContain("**Actionable comments posted: 1**"); + expect(body).not.toContain("Lower-confidence observations"); + }); + + it("collapses a medium-confidence body finding and drops it from the count", () => { + // The exact shape of the noise being fixed: a Major/Medium drift finding + // that claimed a top-of-review slot on every single review. + const body = formatReviewBody(result({ comments: [bodyFinding({ confidence: "medium" })] }), META); + expect(body).toContain("Lower-confidence observations about the change as a whole (1)"); + expect(body).not.toContain("Issues not tied to a specific line"); + expect(body).toContain("**Actionable comments posted: 0**"); + // Still present — collapsed, not deleted. + expect(body).toContain("the description claims a default change the diff never makes"); + }); + + it("collapses a low-confidence body finding too", () => { + const body = formatReviewBody(result({ comments: [bodyFinding({ confidence: "low" })] }), META); + expect(body).toContain("Lower-confidence observations about the change as a whole (1)"); + expect(body).toContain("**Actionable comments posted: 0**"); + }); + + it("treats an absent confidence as high, matching the renderer's documented default", () => { + const body = formatReviewBody(result({ comments: [bodyFinding({ confidence: undefined })] }), META); + expect(body).toContain("Issues not tied to a specific line (1)"); + }); + + it("splits a mixed set across both sections", () => { + const body = formatReviewBody( + result({ + comments: [ + bodyFinding({ confidence: "high", title: "A." }), + bodyFinding({ confidence: "medium", title: "B." }), + bodyFinding({ confidence: "low", title: "C." }), + ], + }), + META, + ); + expect(body).toContain("Issues not tied to a specific line (1)"); + expect(body).toContain("Lower-confidence observations about the change as a whole (2)"); + expect(body).toContain("**Actionable comments posted: 1**"); + }); +}); + +describe("review-body: file-level findings are threads, not body prose", () => { + it("counts a file-level finding without printing it in the body", () => { + const body = formatReviewBody(result({ comments: [fileFinding()] }), META); + // It's posted as its own review thread by submitReview — printing it here + // too would say everything twice. + expect(body).not.toContain("edit-mode fields were removed"); + expect(body).not.toContain("Issues not tied to a specific line"); + expect(body).toContain("**Actionable comments posted: 1**"); + }); + + it("counts a medium-confidence file-level finding — a thread can be resolved", () => { + // Confidence only gates the body section. A thread carries no permanent + // cost, so it doesn't need to clear the same bar. + const body = formatReviewBody(result({ comments: [fileFinding({ confidence: "medium" })] }), META); + expect(body).toContain("**Actionable comments posted: 1**"); + }); + + it("never routes a file-level finding into the bulk agent prompt", () => { + const body = formatReviewBody( + result({ comments: [fileFinding({ aiAgentPrompt: "fix the lead detail" })] }), + META, + ); + expect(body).not.toContain("Line 0:"); + }); +}); + +describe("isVisiblyActionable", () => { + it("rejects nitpicks regardless of channel or confidence", () => { + expect(isVisiblyActionable(bodyFinding({ severity: "minor", confidence: "high" }))).toBe(false); + expect(isVisiblyActionable(fileFinding({ type: "nitpick" }))).toBe(false); + }); + + it("accepts an inline actionable finding", () => { + const inline: ReviewComment = { + path: "src/a.ts", line: 3, side: "RIGHT", body: "x", type: "issue", severity: "major", title: "T.", + }; + expect(isVisiblyActionable(inline)).toBe(true); + }); +}); + +describe("reconcileApproval with confidence-gated findings", () => { + it("downgrades a block backed only by a medium-confidence body finding", () => { + // Otherwise we'd re-introduce exactly what #76 fixed: "changes requested" + // with nothing prominent to act on, the finding buried in a collapse. + expect(reconcileApproval("REQUEST_CHANGES", [bodyFinding({ confidence: "medium" })])).toBe("COMMENT"); + }); + + it("keeps a block backed by a high-confidence body finding", () => { + expect(reconcileApproval("REQUEST_CHANGES", [bodyFinding({ confidence: "high" })])).toBe("REQUEST_CHANGES"); + }); + + it("keeps a block backed by a file-level finding at any confidence", () => { + expect(reconcileApproval("REQUEST_CHANGES", [fileFinding({ confidence: "low" })])).toBe("REQUEST_CHANGES"); + }); +}); + +describe("titleSimilarity / isRepeatPrLevelFinding", () => { + it("scores a re-worded restatement of one finding above the repeat threshold", () => { + const a = "Lead detail removes editable Qualification Long Form fields"; + const b = "Lead detail removes the editable Qualification Long Form accordion fields"; + expect(titleSimilarity(a, b)).toBeGreaterThanOrEqual(0.6); + }); + + it("scores two genuinely distinct findings well below it", () => { + const a = "Lead detail removes editable Qualification Long Form"; + const b = "tk02 never widens the status column server_default"; + expect(titleSimilarity(a, b)).toBeLessThan(0.6); + }); + + it("is 0 against an empty or stopword-only title", () => { + expect(titleSimilarity("", "anything at all here")).toBe(0); + expect(titleSimilarity("it is the that", "a real finding about routing")).toBe(0); + }); + + it("suppresses a re-worded repeat of a prior PR-level finding", () => { + const prior = [prLevelRepeatKey("", "Lead detail removes editable Qualification Long Form fields")]; + const candidate = { path: "", title: "Lead detail removes the editable Qualification Long Form accordion fields" }; + expect(isRepeatPrLevelFinding(candidate, prior)).toBe(true); + }); + + it("does not suppress a same-sounding finding scoped to a different file", () => { + const prior = [prLevelRepeatKey("src/a.ts", "Lead detail removes editable Qualification Long Form fields")]; + const candidate = { path: "src/b.ts", title: "Lead detail removes editable Qualification Long Form fields" }; + expect(isRepeatPrLevelFinding(candidate, prior)).toBe(false); + }); + + it("does not suppress a distinct finding", () => { + const prior = [prLevelRepeatKey("", "Lead detail removes editable Qualification Long Form")]; + const candidate = { path: "", title: "tk02 never widens the status column server_default" }; + expect(isRepeatPrLevelFinding(candidate, prior)).toBe(false); + }); + + it("suppresses nothing when there is no prior state or no title", () => { + expect(isRepeatPrLevelFinding({ path: "", title: "anything" }, [])).toBe(false); + expect(isRepeatPrLevelFinding({ path: "", title: undefined }, [prLevelRepeatKey("", "anything")])).toBe(false); + }); +}); + +describe("drift: confidence is carried, not assumed", () => { + function ctx(description: string): PRContext { + return { + owner: "o", repo: "r", pullNumber: 1, title: "t", + description, + baseBranch: "main", headBranch: "feat", headSha: "deadbee", + files: [{ filename: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n+x", additions: 1, deletions: 0 }], + }; + } + const LONG_DESC = "This PR does a number of things worth describing at length."; + + function aiReturning(raw: string) { + return { chat: vi.fn().mockResolvedValue(raw) } as any; + } + + it("carries an explicit high confidence through", async () => { + const ai = aiReturning(JSON.stringify([ + { level: "warning", summary: "s", details: "d", confidence: "high" }, + ])); + const out = await detectDescriptionDrift({ ai, context: ctx(LONG_DESC) }); + expect(out[0].confidence).toBe("high"); + }); + + it("defaults a missing confidence to medium, not high", async () => { + // Drift is diagnosed by comparing prose to code and is the finding class + // most prone to confident-sounding false positives, so silence here must + // not buy a prominent slot. + const ai = aiReturning(JSON.stringify([{ level: "warning", summary: "s", details: "d" }])); + const out = await detectDescriptionDrift({ ai, context: ctx(LONG_DESC) }); + expect(out[0].confidence).toBe("medium"); + }); + + it("coerces a garbage confidence to medium", async () => { + const ai = aiReturning(JSON.stringify([ + { level: "warning", summary: "s", details: "d", confidence: "very-sure" }, + ])); + const out = await detectDescriptionDrift({ ai, context: ctx(LONG_DESC) }); + expect(out[0].confidence).toBe("medium"); + }); + + it("keeps the short-description sentinel informational", async () => { + const ai = aiReturning("[]"); + const out = await detectDescriptionDrift({ ai, context: ctx("tiny") }); + expect(out[0].level).toBe("info"); + expect(ai.chat).not.toHaveBeenCalled(); + }); +}); + +describe("submitReview: file-level threads and superseded reviews", () => { + const MARKER = ""; + + function ctx(): PRContext { + return { + owner: "o", repo: "r", pullNumber: 7, title: "t", description: "d", + baseBranch: "main", headBranch: "feat", headSha: "deadbee", files: [], + }; + } + + function fakeOctokit(over: { reviews?: any[]; createReviewComment?: any } = {}) { + const calls = { + createReview: vi.fn().mockResolvedValue({}), + createReviewComment: over.createReviewComment ?? vi.fn().mockResolvedValue({}), + dismissReview: vi.fn().mockResolvedValue({}), + updateReview: vi.fn().mockResolvedValue({}), + listReviews: vi.fn(), + }; + const octokit: any = { + pulls: calls, + paginate: vi.fn().mockResolvedValue(over.reviews ?? []), + }; + return { octokit, calls }; + } + + function clientWith(octokit: any): GitHubClient { + const client = new GitHubClient({} as Config); + client.getInstallationOctokit = vi.fn().mockResolvedValue(octokit); + return client; + } + + it("posts a file-level finding as a resolvable file-scoped thread", async () => { + const { octokit, calls } = fakeOctokit(); + await clientWith(octokit).submitReview(1, ctx(), result({ comments: [fileFinding()] })); + + expect(calls.createReviewComment).toHaveBeenCalledTimes(1); + expect(calls.createReviewComment.mock.calls[0][0]).toMatchObject({ + path: "src/a.ts", + subject_type: "file", + commit_id: "deadbee", + }); + // Not duplicated into the review body. + expect(calls.createReview.mock.calls[0][0].body).not.toContain("couldn't be attached"); + }); + + it("folds a rejected file-level finding back into the review body", async () => { + // A finding must never vanish between the thread and body channels — the + // silent-loss failure #76 exists to prevent. + const { octokit, calls } = fakeOctokit({ + createReviewComment: vi.fn().mockRejectedValue(Object.assign(new Error("422"), { status: 422 })), + }); + await clientWith(octokit).submitReview(1, ctx(), result({ comments: [fileFinding()] })); + + const body = calls.createReview.mock.calls[0][0].body; + expect(body).toContain("couldn't be attached to their file (1)"); + expect(body).toContain("edit-mode fields were removed from the lead detail view"); + }); + + it("dismisses a superseded CHANGES_REQUESTED review and stubs a COMMENTED one", async () => { + // COMMENTED reviews can't be dismissed via the API and submitted reviews + // can't be deleted, so the body is rewritten instead. Without this, every + // drift-driven review keeps its full text on the timeline forever. + const { octokit, calls } = fakeOctokit({ + reviews: [ + { id: 1, state: "CHANGES_REQUESTED", body: `old block${MARKER}` }, + { id: 2, state: "COMMENTED", body: `old comment${MARKER}` }, + ], + }); + await clientWith(octokit).submitReview(1, ctx(), result()); + + expect(calls.dismissReview).toHaveBeenCalledTimes(1); + expect(calls.dismissReview.mock.calls[0][0]).toMatchObject({ review_id: 1 }); + expect(calls.updateReview).toHaveBeenCalledTimes(1); + expect(calls.updateReview.mock.calls[0][0]).toMatchObject({ review_id: 2 }); + expect(calls.updateReview.mock.calls[0][0].body).toContain("superseded"); + }); + + it("leaves other bots' and humans' reviews alone", async () => { + // The old filter was `user.type === "Bot"`, which reached other bots' + // reviews. Match on our own marker instead. + const { octokit, calls } = fakeOctokit({ + reviews: [ + { id: 1, state: "CHANGES_REQUESTED", body: "some other bot's review" }, + { id: 2, state: "COMMENTED", body: "a human's note" }, + { id: 3, state: "APPROVED", body: `ours, approved${MARKER}` }, + ], + }); + await clientWith(octokit).submitReview(1, ctx(), result()); + + expect(calls.dismissReview).not.toHaveBeenCalled(); + expect(calls.updateReview).not.toHaveBeenCalled(); + }); + + it("still posts the review when retiring a stale one fails", async () => { + const { octokit, calls } = fakeOctokit({ + reviews: [{ id: 1, state: "COMMENTED", body: `ours${MARKER}` }], + }); + calls.updateReview.mockRejectedValue(new Error("no permission")); + await clientWith(octokit).submitReview(1, ctx(), result()); + expect(calls.createReview).toHaveBeenCalledTimes(1); + }); +}); From bd43de4a467f8fda94ab71e655f7c5e855fb0c21 Mon Sep 17 00:00:00 2001 From: Luke Date: Tue, 14 Jul 2026 12:00:22 -0700 Subject: [PATCH 2/2] fix: address DiffSentry review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the drift approval gate into applyDriftToApproval (drift.ts) so the rule that hedged drift must not cost an approval is testable in isolation rather than buried inline in the reviewer's main method. No behaviour change — same condition, now pinned by tests. Adds 8 tests: the APPROVE + medium/low-drift path end to end through both verdict rules, and coverage proving isRepeatPrLevelFinding spans file-level findings (not just body-level ones) so re-worded file findings can't stack duplicate threads across pushes. Documents the path-scope contract on isRepeatPrLevelFinding: path is part of a finding's identity, comparison is same-scope only, and drift must keep emitting with path: "" to collapse against its own history. 369 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ai/parse.ts | 12 +++++- src/drift.ts | 20 +++++++++- src/reviewer.ts | 10 +---- tests/unit/pr-level-noise.test.ts | 64 ++++++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 12 deletions(-) diff --git a/src/ai/parse.ts b/src/ai/parse.ts index b610b1d..02e9541 100644 --- a/src/ai/parse.ts +++ b/src/ai/parse.ts @@ -200,8 +200,16 @@ export function prLevelRepeatKey(path: string, title: string): string { /** * Whether a PR-level finding restates one already posted on a previous review. - * Only compares findings scoped to the same file (or both unscoped), so a real - * finding is never swallowed by a same-sounding one about different code. + * Covers BOTH prLevel flavours — the caller filters on `prLevel`, so file-scoped + * findings dedup here too and re-wordings don't stack duplicate threads in the + * Files tab across pushes. + * + * `path` is part of a finding's identity, so comparison is same-scope only: a + * file-scoped finding matches only file-scoped priors on that same file, and an + * unscoped one only unscoped priors. Two findings that read alike about + * different code are different findings. This is why drift must keep emitting + * with `path: ""` (reviewer.ts) — scope it to a file and it stops collapsing + * against its own unscoped history. */ export function isRepeatPrLevelFinding( candidate: { path: string; title?: string }, diff --git a/src/drift.ts b/src/drift.ts index 4511780..d38bac2 100644 --- a/src/drift.ts +++ b/src/drift.ts @@ -1,4 +1,4 @@ -import type { AIProvider, Confidence, FileChange, LicenseHeaderConfig, PRContext } from "./types.js"; +import type { AIProvider, Confidence, FileChange, LicenseHeaderConfig, PRContext, ReviewResult } from "./types.js"; import { minimatch } from "minimatch"; /** @@ -77,6 +77,24 @@ Be specific — name files and identifiers, don't say "various changes".`; } } +/** + * How folded drift may move the verdict. Withholding an approval is itself a + * claim about the change, so only drift the model actually stands behind may do + * it — a hedged observation still renders (collapsed, uncounted) but rides along + * with the APPROVE rather than quietly turning every clean PR into a commented + * one. Pure and one-directional: drift may cost an approval, never grant one, + * and never escalates to a block. + */ +export function applyDriftToApproval( + approval: ReviewResult["approval"], + driftWarnings: DriftFinding[], +): ReviewResult["approval"] { + if (approval === "APPROVE" && driftWarnings.some((f) => f.confidence === "high")) { + return "COMMENT"; + } + return approval; +} + export function renderDriftBlock(findings: DriftFinding[]): string { if (findings.length === 0) return ""; const lines: string[] = []; diff --git a/src/reviewer.ts b/src/reviewer.ts index c9b643c..ddab554 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -33,7 +33,7 @@ import { runSafetyScanners } from "./safety-scanner.js"; import { runPatternChecks } from "./pattern-checks.js"; import { runStaticAnalysis, resolveCheckoutDir, dedupeStaticFindings } from "./static-analysis.js"; import { scanDependencyChanges, renderDepBlock } from "./dep-scanner.js"; -import { detectDescriptionDrift, renderDriftBlock, reviewCommitMessages, renderCommitCoachBlock, reviewPRTitle, renderTitleCoachBlock, scanLicenseHeaders, renderLicenseHeaderBlock } from "./drift.js"; +import { detectDescriptionDrift, applyDriftToApproval, renderDriftBlock, reviewCommitMessages, renderCommitCoachBlock, reviewPRTitle, renderTitleCoachBlock, scanLicenseHeaders, renderLicenseHeaderBlock } from "./drift.js"; import { createHash } from "node:crypto"; import { logger } from "./logger.js"; import { bus } from "./realtime/bus.js"; @@ -1064,13 +1064,7 @@ export class Reviewer { ), ); } - // Withholding an approval is a claim about the change, so only drift we - // actually stand behind may do it. A hedged observation still renders - // (collapsed, uncounted) — but it rides along with the APPROVE rather - // than quietly turning every clean PR into a commented one. - if (reviewResult.approval === "APPROVE" && driftWarnings.some((f) => f.confidence === "high")) { - reviewResult.approval = "COMMENT"; - } + reviewResult.approval = applyDriftToApproval(reviewResult.approval, driftWarnings); log.info( { count: driftWarnings.length, highConfidence: driftWarnings.filter((f) => f.confidence === "high").length }, "Folded description-drift warnings into PR-level findings", diff --git a/tests/unit/pr-level-noise.test.ts b/tests/unit/pr-level-noise.test.ts index 9ccd72c..eba184b 100644 --- a/tests/unit/pr-level-noise.test.ts +++ b/tests/unit/pr-level-noise.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi } from "vitest"; import { formatReviewBody, reconcileApproval, isVisiblyActionable } from "../../src/review-body.js"; import { titleSimilarity, isRepeatPrLevelFinding, prLevelRepeatKey } from "../../src/ai/parse.js"; -import { detectDescriptionDrift } from "../../src/drift.js"; +import { detectDescriptionDrift, applyDriftToApproval, type DriftFinding } from "../../src/drift.js"; import { GitHubClient } from "../../src/github.js"; import type { Config, PRContext, ReviewComment, ReviewResult } from "../../src/types.js"; @@ -158,6 +158,44 @@ describe("reconcileApproval with confidence-gated findings", () => { }); }); +describe("applyDriftToApproval: only drift we stand behind costs an approval", () => { + function drift(confidence: DriftFinding["confidence"]): DriftFinding { + return { level: "warning", summary: "s", details: "d", confidence }; + } + + it("keeps APPROVE when the only drift is medium-confidence", () => { + // The headline promise: a hedged diff-vs-description reading must not + // quietly turn a clean PR into a commented one. + expect(applyDriftToApproval("APPROVE", [drift("medium")])).toBe("APPROVE"); + }); + + it("keeps APPROVE when the only drift is low-confidence", () => { + expect(applyDriftToApproval("APPROVE", [drift("low")])).toBe("APPROVE"); + }); + + it("downgrades APPROVE to COMMENT for high-confidence drift", () => { + expect(applyDriftToApproval("APPROVE", [drift("high")])).toBe("COMMENT"); + }); + + it("downgrades when any drift in a mixed set is high-confidence", () => { + expect(applyDriftToApproval("APPROVE", [drift("medium"), drift("high")])).toBe("COMMENT"); + }); + + it("never escalates a block or relaxes one", () => { + expect(applyDriftToApproval("REQUEST_CHANGES", [drift("high")])).toBe("REQUEST_CHANGES"); + expect(applyDriftToApproval("COMMENT", [drift("high")])).toBe("COMMENT"); + expect(applyDriftToApproval("APPROVE", [])).toBe("APPROVE"); + }); + + it("cannot be turned into a downgrade by reconcileApproval afterwards", () => { + // reconcileApproval is the only other thing that touches the verdict, and it + // guards on REQUEST_CHANGES — so the APPROVE + medium-drift path stays + // APPROVE end to end, through both rules in the order reviewer.ts runs them. + const approval = applyDriftToApproval("APPROVE", [drift("medium")]); + expect(reconcileApproval(approval, [bodyFinding({ confidence: "medium" })])).toBe("APPROVE"); + }); +}); + describe("titleSimilarity / isRepeatPrLevelFinding", () => { it("scores a re-worded restatement of one finding above the repeat threshold", () => { const a = "Lead detail removes editable Qualification Long Form fields"; @@ -198,6 +236,30 @@ describe("titleSimilarity / isRepeatPrLevelFinding", () => { expect(isRepeatPrLevelFinding({ path: "", title: "anything" }, [])).toBe(false); expect(isRepeatPrLevelFinding({ path: "", title: undefined }, [prLevelRepeatKey("", "anything")])).toBe(false); }); + + it("covers FILE-level findings, not just body-level ones", () => { + // Guards the property that keeps re-worded file findings from stacking + // duplicate threads in the Files tab on every push: the reviewer's + // similarity pass filters on `c.prLevel`, which spans both flavours, and + // records keys with each finding's real path. A path-scoped repeat must + // collapse against its path-scoped prior exactly like an unscoped one does. + const prior = [prLevelRepeatKey("src/22-leads.js", "Lead detail removes editable Qualification Long Form fields")]; + const reworded = { + path: "src/22-leads.js", + title: "Lead detail removes the editable Qualification Long Form accordion fields", + }; + expect(isRepeatPrLevelFinding(reworded, prior)).toBe(true); + }); + + it("keeps file-level and body-level findings in separate identity scopes", () => { + // Drift is always emitted unscoped (path: ""), so an unscoped candidate must + // not collapse against a same-titled file-scoped prior — they are claims + // about different things and each deserves its own thread/section. + const fileScopedPrior = [prLevelRepeatKey("src/a.ts", "Lead detail removes editable Qualification Long Form")]; + expect( + isRepeatPrLevelFinding({ path: "", title: "Lead detail removes editable Qualification Long Form" }, fileScopedPrior), + ).toBe(false); + }); }); describe("drift: confidence is carried, not assumed", () => {