diff --git a/src/ai/parse.ts b/src/ai/parse.ts index ee32f28..02e9541 100644 --- a/src/ai/parse.ts +++ b/src/ai/parse.ts @@ -142,6 +142,89 @@ 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. + * 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 }, + 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..d38bac2 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, ReviewResult } 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,12 +66,35 @@ 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 []; } } +/** + * 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/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..ddab554 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"; @@ -28,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"; @@ -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,17 @@ 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") { - reviewResult.approval = "COMMENT"; - } - log.info({ count: driftWarnings.length }, "Folded description-drift warnings into PR-level findings"); + 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", + ); } const risk = assessRisk({ files: context.files, @@ -1253,6 +1269,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 +1373,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..eba184b --- /dev/null +++ b/tests/unit/pr-level-noise.test.ts @@ -0,0 +1,414 @@ +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, applyDriftToApproval, type DriftFinding } 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("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"; + 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); + }); + + 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", () => { + 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); + }); +});