fix(review): address non-blocking review nits across the credibility-gate-farming series

src/review/linked-issue-hard-rules.ts

@@ -1,7 +1,7 @@
 import { fetchLinkedIssueFacts, type LinkedIssueFactsFetch } from "../github/backfill";
 import { githubRateLimitAdmissionKeyForToken } from "../github/client";
 import { createInstallationToken } from "../github/app";
-import { extractLinkedIssueNumbersWithOverflow } from "../db/repositories";
+import { extractLinkedIssueNumbersWithOverflow, MAX_LINKED_ISSUE_NUMBERS } from "../db/repositories";
 import { resolveRepositorySettings } from "../settings/repository-settings";
 import { DEFAULT_LINKED_ISSUE_HARD_RULES } from "./linked-issue-hard-rules-config";
 import type { LinkedIssueHardRulesConfig } from "../types";
@@ -225,6 +225,12 @@ export async function resolveLinkedIssueHasOpenReference(args: {
   installationId?: number | null | undefined;
 }): Promise<boolean> {
   if (args.linkedIssues.length === 0) return true;
+  // Fail open (mirrors hasVerifiableOpenLinkedIssueReference's own ambiguity philosophy above) rather than
+  // firing an unbounded per-issue fan-out for a body citing more references than can be safely verified in
+  // one pass -- the same cap resolveLinkedIssueHardRule's own extractLinkedIssueNumbersWithOverflow enforces
+  // on the sibling gate, reused here instead of a second bound so a noisy body can't create surprise API
+  // pressure on this path.
+  if (args.linkedIssues.length > MAX_LINKED_ISSUE_NUMBERS) return true;
   const ciToken = args.installationId ? await createInstallationToken(args.env, args.installationId).catch(() => undefined) : undefined;
   const token = ciToken ?? args.env.GITHUB_PUBLIC_TOKEN;
   const admissionKey = githubRateLimitAdmissionKeyForToken(args.env, token, args.installationId);

src/review/unlinked-issue-match.ts

@@ -37,7 +37,7 @@ function buildSystemPrompt(): string {
 }
 
 function buildUserPrompt(input: { prTitle: string; prBody: string | null | undefined; diff: string; candidate: CandidateOpenIssue }): string {
-  const diff = input.diff.length > DIFF_CHAR_BUDGET ? `${input.diff.slice(0, DIFF_CHAR_BUDGET)}\n… (diff truncated)` : input.diff;
+  const diff = input.diff.length > DIFF_CHAR_BUDGET ? `${input.diff.slice(0, DIFF_CHAR_BUDGET)}\n... (diff truncated)` : input.diff;
   return [
     `PULL REQUEST TITLE: ${input.prTitle}`,
     `PULL REQUEST BODY: ${input.prBody?.trim() || "(empty)"}`,

src/settings/agent-actions.ts

@@ -917,9 +917,17 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
     // independently wants manualReview and may re-add it later in this SAME pass) — removing it here would
     // race against that later add.
     const dispositionLabelSiblings = [labels.readyToMerge, labels.manualReview, labels.migrationCollision, labels.changesRequested];
+    const livePrLabels = new Set(input.pr.labels.map((l) => l.toLowerCase()));
+    // Dedupe defensively: if a repo ever misconfigures two of the four settings to the identical label
+    // string, only clear it once (still correct — the label either belongs here or it doesn't — just
+    // avoids a redundant duplicate remove action for the same name).
+    const alreadyHandled = new Set<string>();
     for (const stale of dispositionLabelSiblings) {
-      if (stale === null || stale === label || !hasLabel(input.pr.labels, stale)) continue;
+      if (stale === null || stale === label) continue;
+      const staleLower = stale.toLowerCase();
+      if (alreadyHandled.has(staleLower) || !livePrLabels.has(staleLower)) continue;
       if (stale === labels.manualReview && manualHoldReason !== null) continue;
+      alreadyHandled.add(staleLower);
       actions.push({
         actionClass: "label",
         autonomyClass: "review_state_label",

src/signals/unlinked-issue-candidates.ts

@@ -45,7 +45,7 @@ const MIN_TOKEN_LENGTH = 4;
 // the token-overlap score and swamp genuinely distinctive words.
 const STOPWORDS = new Set([
   "this", "that", "with", "from", "have", "when", "where", "which", "there", "their",
-  "issue", "issues", "should", "would", "could", "about", "would", "into", "your", "were",
+  "issue", "issues", "should", "would", "could", "about", "into", "your", "were",
   "then", "than", "will", "does", "doesn", "cannot", "currently", "instead", "because",
   "these", "those", "being", "only", "also", "still", "even", "some", "each", "such",
 ]);
@@ -60,14 +60,21 @@ function tokenize(text: string): Set<string> {
 
 /** True when an issue's body names one of the PR's changed files — either the full repo-relative path or
  *  just its basename (issues commonly reference "the X.ts file" without the full path). Basenames shorter
- *  than {@link MIN_TOKEN_LENGTH} are skipped as too generic (e.g. `db.ts`, `index.ts` collide across repos). */
+ *  than {@link MIN_TOKEN_LENGTH} are skipped as too generic (e.g. `db.ts`, `index.ts` collide across repos).
+ *  The full-path check stays a plain substring match (a repo-relative path is already distinctive enough
+ *  that a coincidental false positive is not realistic). The basename check instead matches against
+ *  path-like TOKENS extracted from the body, requiring an exact token match (or a longer path token ending
+ *  in `/basename`) rather than raw substring containment — a naive `.includes()` would let a basename like
+ *  `reader.ts` match inside an unrelated, longer filename such as `csv-reader.ts`. */
 function issueMentionsChangedPath(issueBody: string, changedPaths: string[]): boolean {
   const lowerBody = issueBody.toLowerCase();
+  const bodyPathTokens = lowerBody.match(/[a-z0-9_\-./]+/g) ?? [];
   return changedPaths.some((path) => {
     const lowerPath = path.toLowerCase();
     if (lowerBody.includes(lowerPath)) return true;
     const basename = lowerPath.slice(lowerPath.lastIndexOf("/") + 1);
-    return basename.length >= MIN_TOKEN_LENGTH && lowerBody.includes(basename);
+    if (basename.length < MIN_TOKEN_LENGTH) return false;
+    return bodyPathTokens.some((token) => token === basename || token.endsWith(`/${basename}`));
   });
 }
 

test/unit/agent-actions.test.ts

@@ -178,6 +178,11 @@ describe("planAgentMaintenanceActions (#778)", () => {
       // the new sibling-cleanup loop (which does not include pendingClosure in its sibling set at all).
       expect(plan.filter((a) => a.label === AGENT_LABEL_PENDING_CLOSURE && a.labelOp === "remove")).toHaveLength(1);
     });
+
+    it("dedupes when two disposition-label settings are misconfigured to the identical string", () => {
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, manualReviewLabel: "shared-label", migrationCollisionLabel: "shared-label", pr: { labels: ["shared-label"] } }));
+      expect(plan.filter((a) => a.actionClass === "label" && a.label === "shared-label" && a.labelOp === "remove")).toHaveLength(1);
+    });
   });
 
   it("approves a passing verdict and never re-approves; a failing one closes (never approves, never requests changes)", () => {

test/unit/linked-issue-hard-rules.test.ts

@@ -1,6 +1,7 @@
 import { afterEach, describe, expect, it, vi } from "vitest";
 import { createTestEnv } from "../helpers/d1";
 import * as backfillModule from "../../src/github/backfill";
+import { MAX_LINKED_ISSUE_NUMBERS } from "../../src/db/repositories";
 import {
   DEFAULT_LINKED_ISSUE_HARD_RULES,
   evaluateLinkedIssueHardRules,
@@ -543,6 +544,24 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup
     expect(fetchSpy).not.toHaveBeenCalled();
   });
 
+  it("fails open (true) and fetches nothing when the linked-issue count exceeds the safe-verification cap (#bounded-fanout)", async () => {
+    const fetchSpy = vi.fn();
+    vi.stubGlobal("fetch", fetchSpy);
+    const tooMany = Array.from({ length: MAX_LINKED_ISSUE_NUMBERS + 1 }, (_, i) => i + 1);
+    const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: tooMany });
+    expect(result).toBe(true);
+    expect(fetchSpy).not.toHaveBeenCalled();
+  });
+
+  it("still fans out normally at exactly the cap", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) =>
+      input.toString().includes("/issues/") ? Response.json({ number: 1, state: "open", labels: [], assignees: [] }) : new Response("missing", { status: 404 }),
+    );
+    const atCap = Array.from({ length: MAX_LINKED_ISSUE_NUMBERS }, (_, i) => i + 1);
+    const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: atCap });
+    expect(result).toBe(true);
+  });
+
   it("returns true when the linked issue is confirmed open", async () => {
     vi.stubGlobal("fetch", async (input: RequestInfo | URL) =>
       input.toString().includes("/issues/") ? Response.json({ number: 7, state: "open", labels: [], assignees: [] }) : new Response("missing", { status: 404 }),

test/unit/unlinked-issue-candidates.test.ts

@@ -57,6 +57,28 @@ describe("findUnlinkedIssueCandidates", () => {
     expect(result[0]?.pathMentioned).toBe(true);
   });
 
+  it("does NOT false-positive when a basename is a substring of a longer, unrelated filename (#boundary-matching)", () => {
+    // "reader.ts" must not match merely because it is a substring of "csv-reader.ts" -- a different file.
+    const result = findUnlinkedIssueCandidates({
+      prTitle: "xyz",
+      prBody: "abc",
+      changedPaths: ["src/utils/reader.ts"],
+      openIssues: [issue({ number: 7, title: "bug", body: "something is wrong in csv-reader.ts specifically" })],
+    });
+    expect(result).toEqual([]);
+  });
+
+  it("still matches a basename embedded in a longer PATH token (same file, fuller path mentioned)", () => {
+    const result = findUnlinkedIssueCandidates({
+      prTitle: "xyz",
+      prBody: "abc",
+      changedPaths: ["other/reader.ts"],
+      openIssues: [issue({ number: 8, title: "bug", body: "reproduced after editing src/utils/reader.ts" })],
+    });
+    expect(result).toHaveLength(1);
+    expect(result[0]?.pathMentioned).toBe(true);
+  });
+
   it("does not match on a too-short basename, regardless of body content", () => {
     // basename "db" is only 2 chars (< MIN_TOKEN_LENGTH), so the length check short-circuits the match
     // before ever scanning the body for it — too generic a fragment to trust as evidence.
@@ -69,6 +91,18 @@ describe("findUnlinkedIssueCandidates", () => {
     expect(result).toEqual([]);
   });
 
+  it("does not path-match (and does not crash) when a non-empty body has no path-like characters at all", () => {
+    // A body of pure punctuation/whitespace never matches the path-token regex at all, so the `?? []`
+    // fallback is exercised instead of the usual non-empty match array.
+    const result = findUnlinkedIssueCandidates({
+      prTitle: "xyz",
+      prBody: "abc",
+      changedPaths: ["src/queue/processors.ts"],
+      openIssues: [issue({ number: 9, title: "bug", body: "??? !!!" })],
+    });
+    expect(result).toEqual([]);
+  });
+
   it("does not path-match when the issue body is empty (null body)", () => {
     const result = findUnlinkedIssueCandidates({
       prTitle: "xyz",

test/unit/unlinked-issue-match.test.ts

@@ -121,7 +121,7 @@ describe("buildUserPrompt", () => {
   it("truncates a diff over the char budget", () => {
     const bigDiff = "x".repeat(7_000);
     const prompt = buildUserPrompt({ prTitle: "t", prBody: null, diff: bigDiff, candidate: { number: 1, title: "i", body: null, labels: [] } });
-    expect(prompt).toContain("… (diff truncated)");
+    expect(prompt).toContain("... (diff truncated)");
     expect(prompt.length).toBeLessThan(bigDiff.length + 500);
   });
 });