Quiet PR-level findings: threads over prose, confidence over volume

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<string> {
+  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?/, "");

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.

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[] = [];

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 =
+  "<sub>🛡️ This DiffSentry review has been superseded by a newer one. Its findings, if still present, appear there.</sub>";
+
+/** 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})`,
+    "",
+    "<sub>DiffSentry couldn't open a review thread on these files — they may no longer be part of the diff.</sub>",
+    "",
+    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<void> {
-    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<ReviewComment[]> {
+    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<void> {
+    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;