diff --git a/src/review/linked-issue-hard-rules.ts b/src/review/linked-issue-hard-rules.ts index 5750f859e8..71745f0b54 100644 --- a/src/review/linked-issue-hard-rules.ts +++ b/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 { 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); diff --git a/src/review/unlinked-issue-match.ts b/src/review/unlinked-issue-match.ts index fd8a640536..86254394bb 100644 --- a/src/review/unlinked-issue-match.ts +++ b/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)"}`, diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index ef3a25aa11..c757b4c4e6 100644 --- a/src/settings/agent-actions.ts +++ b/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(); 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", diff --git a/src/signals/unlinked-issue-candidates.ts b/src/signals/unlinked-issue-candidates.ts index cee5e69b6c..bbc193219e 100644 --- a/src/signals/unlinked-issue-candidates.ts +++ b/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 { /** 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}`)); }); } diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 4c0653273f..b284595ce3 100644 --- a/test/unit/agent-actions.test.ts +++ b/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)", () => { diff --git a/test/unit/linked-issue-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index e234132f86..48f3be9da2 100644 --- a/test/unit/linked-issue-hard-rules.test.ts +++ b/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 }), diff --git a/test/unit/unlinked-issue-candidates.test.ts b/test/unit/unlinked-issue-candidates.test.ts index 06c7ec14af..5b8381c2fe 100644 --- a/test/unit/unlinked-issue-candidates.test.ts +++ b/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", diff --git a/test/unit/unlinked-issue-match.test.ts b/test/unit/unlinked-issue-match.test.ts index 97587d3ccb..ef2634f06b 100644 --- a/test/unit/unlinked-issue-match.test.ts +++ b/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); }); });