diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 8354d771d4..8b0c6b47d2 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3104,26 +3104,91 @@ export async function countOpenPullRequests(env: Env, fullName: string): Promise return Number(row?.count ?? 0); } +export type OpenItemAcrossInstallRow = { repoFullName: string; number: number; kind: "pull_request" | "issue" }; + +/** Repo full names belonging to ONE installation (#2562 gate finding): `pullRequests`/`issues` carry no + * installation column of their own (only `repoFullName`, matched against `repositories.fullName` by + * convention, no FK) -- so scoping a cross-repo query to one install means resolving its repo set FIRST, + * mirroring the existing installation-scoped filter in markRepositoriesRemovedFromInstallation (same file) + * rather than a SQL join, which this codebase doesn't otherwise use. */ +// #regate-review (gate finding): a fixed LIMIT that's quietly hit degrades the install-wide contributor cap from +// "every repo in the install is counted" to a silent undercount -- an install (or an author's open items, below) +// at or beyond the limit could bypass GLOBAL_CONTRIBUTOR_OPEN_ITEM_CAP with no signal anything was dropped. These +// are raised far above any realistic install size / per-author open-item count so truncation should never occur +// in practice; the audit event below makes it OBSERVABLE (not silent) on the rare install where it still does, +// rather than pretending completeness the contract promises but the query can't actually guarantee at an +// unbounded size. +const INSTALLATION_REPO_LIST_LIMIT = 20_000; +const AUTHOR_OPEN_ITEM_LIST_LIMIT = 20_000; + +async function auditListTruncated(env: Env, eventType: string, targetKey: string, detail: string): Promise { + /* v8 ignore next -- defensive: recordAuditEvent is a same-module direct call (not interceptable via + * vi.spyOn on the module's exports the way a cross-module import site is), so a genuine write failure here + * would require corrupting the shared test D1 handle itself; the truncation detection above must never be + * allowed to throw and mask the (already-truncated) result this function's caller still needs to return. */ + await recordAuditEvent(env, { eventType, outcome: "error", targetKey, detail }).catch(() => undefined); +} + +async function listRepoFullNamesForInstallation(env: Env, installationId: number): Promise { + const db = getDb(env.DB); + const rows = await db.select({ fullName: repositories.fullName }).from(repositories).where(eq(repositories.installationId, installationId)).limit(INSTALLATION_REPO_LIST_LIMIT); + if (rows.length === INSTALLATION_REPO_LIST_LIMIT) + await auditListTruncated( + env, + "agent.global_open_item_cap.repo_list_truncated", + `installation:${installationId}`, + `installation has >= ${INSTALLATION_REPO_LIST_LIMIT} repos; the global contributor-cap check may undercount repos not included here`, + ); + return rows.map((row) => row.fullName); +} + /** - * Install-wide open-item count for one author (#2562, anti-abuse): SUM of this author's open PRs + open - * issues across EVERY repo tracked in this install's database -- deliberately NOT scoped by repoFullName, - * unlike countOpenPullRequests/countOpenIssues above. This is what makes the globalContributorOpenItemCap - * catch an actor spreading low-volume spam across several gated repos in the same self-hosted install: no - * single repo's own cap trips, but the aggregate does. Same-database aggregate only -- no cross-instance - * networking, mirroring the install-scoped singleton shape of global_contributor_blacklist. Case-insensitive - * login match (mirrors loginMatches/findBlacklistEntry elsewhere in this file). + * Install-wide open-item ROWS for one author (#2562, anti-abuse): every open PR + open issue by this author + * across EVERY repo THIS INSTALLATION gates in the SAME D1 database -- no cross-instance networking. Gate + * finding: one D1 database can serve MORE than one installation (the hosted product, or a self-host operator + * running more than one App install), so this MUST scope to `installationId`'s own repo set rather than + * querying the whole database, or a contributor's activity on a completely unrelated installation's repos + * could wrongly count toward -- and close -- a PR/issue here. This is what makes the globalContributorOpenItemCap + * catch an actor spreading low-volume spam across several gated repos in the SAME install: no single repo's own + * cap trips, but the aggregate does. Returns the actual rows (not just a count) so the caller can live-verify + * each one before trusting the aggregate toward an irreversible close -- gate finding: the stored DB cache can + * lag GitHub for a repo OTHER than the one the current webhook is for, and an inflated stale count must never + * itself trigger a close (mirrors the existing per-repo issue-cap's own sibling live-verification, #2479). The + * existing per-repo countOpenPullRequests/countOpenIssues stay scoped to one repo and are unaffected by this + * addition. Case-insensitive login match (mirrors loginMatches/findBlacklistEntry elsewhere in this file). */ -export async function countOpenItemsForAuthorAcrossRepos(env: Env, authorLogin: string): Promise { +export async function listOpenItemsForAuthorAcrossInstall(env: Env, installationId: number, authorLogin: string): Promise { + const repoNames = await listRepoFullNamesForInstallation(env, installationId); + if (repoNames.length === 0) return []; const db = getDb(env.DB); - const [[prRow], [issueRow]] = await Promise.all([ - db.select({ count: sql`count(*)` }).from(pullRequests).where(and(eq(pullRequests.state, "open"), loginMatches(pullRequests.authorLogin, authorLogin))), - db.select({ count: sql`count(*)` }).from(issues).where(and(eq(issues.state, "open"), loginMatches(issues.authorLogin, authorLogin))), - ]); - /* v8 ignore next -- SQL aggregate count always returns one row; fallback protects D1 driver anomalies. */ - const prCount = Number(prRow?.count ?? 0); - /* v8 ignore next -- SQL aggregate count always returns one row; fallback protects D1 driver anomalies. */ - const issueCount = Number(issueRow?.count ?? 0); - return prCount + issueCount; + const prRows = await db + .select({ repoFullName: pullRequests.repoFullName, number: pullRequests.number }) + .from(pullRequests) + .where(and(eq(pullRequests.state, "open"), loginMatches(pullRequests.authorLogin, authorLogin), inArray(pullRequests.repoFullName, repoNames))) + .limit(AUTHOR_OPEN_ITEM_LIST_LIMIT); + if (prRows.length === AUTHOR_OPEN_ITEM_LIST_LIMIT) + await auditListTruncated( + env, + "agent.global_open_item_cap.author_items_truncated", + `${authorLogin}@installation:${installationId}`, + `author has >= ${AUTHOR_OPEN_ITEM_LIST_LIMIT} open pull requests across the install; the global contributor-cap check may undercount`, + ); + const issueRows = await db + .select({ repoFullName: issues.repoFullName, number: issues.number }) + .from(issues) + .where(and(eq(issues.state, "open"), loginMatches(issues.authorLogin, authorLogin), inArray(issues.repoFullName, repoNames))) + .limit(AUTHOR_OPEN_ITEM_LIST_LIMIT); + if (issueRows.length === AUTHOR_OPEN_ITEM_LIST_LIMIT) + await auditListTruncated( + env, + "agent.global_open_item_cap.author_items_truncated", + `${authorLogin}@installation:${installationId}`, + `author has >= ${AUTHOR_OPEN_ITEM_LIST_LIMIT} open issues across the install; the global contributor-cap check may undercount`, + ); + return [ + ...prRows.map((row) => ({ repoFullName: row.repoFullName, number: row.number, kind: "pull_request" as const })), + ...issueRows.map((row) => ({ repoFullName: row.repoFullName, number: row.number, kind: "issue" as const })), + ]; } // Anti-farming (#anti-gaming-flood): how many PRs this author has SUBMITTED to this repo since `sinceIso` (ANY diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3159e97d43..af1574f40a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -1,7 +1,8 @@ import { countOpenIssues, - countOpenItemsForAuthorAcrossRepos, countOpenPullRequests, + listOpenItemsForAuthorAcrossInstall, + type OpenItemAcrossInstallRow, getAgentCommandAnswer, getInstallation, getLatestRepoGithubTotalsSnapshot, @@ -2036,7 +2037,7 @@ async function runAgentMaintenancePlanAndExecute( // default) ⇒ this block is a no-op. A below-account-age-threshold author (#2561) gets a TIGHTER effective // cap (half, rounded up, minimum 1) — visibility/friction, still never a close on account age by itself // (the close, if any, is still tagged/reasoned as the ordinary contributor-cap close). - let contributorCapMatch: { matched: boolean; authorLogin: string; openCount: number; cap: number; itemKind: "pull requests" | "issues"; scope?: "repository" | "install" | undefined } | undefined; + let contributorCapMatch: { matched: boolean; authorLogin: string; openCount: number; cap: number; itemKind: "pull requests" | "issues" | "pull requests and issues"; scope?: "repository" | "install" | undefined } | undefined; const contributorOpenPrCap = isNewAccount && typeof settings.contributorOpenPrCap === "number" ? Math.max(1, Math.ceil(settings.contributorOpenPrCap / 2)) @@ -2074,9 +2075,16 @@ async function runAgentMaintenancePlanAndExecute( if (contributorCapMatch === undefined && pr.authorLogin && !isAutoCloseExempt(pr.authorLogin, settings.autoCloseExemptLogins)) { const globalCap = resolveGlobalContributorOpenItemCap(env); if (globalCap !== null) { - const installOpenCount = await countOpenItemsForAuthorAcrossRepos(env, pr.authorLogin); - if (installOpenCount > globalCap) { - contributorCapMatch = { matched: true, authorLogin: pr.authorLogin, openCount: installOpenCount, cap: globalCap, itemKind: "pull requests", scope: "install" }; + const globalOpenCount = await verifiedGlobalOpenItemCount(env, installationId, pr.authorLogin, { + repoFullName, + number: pr.number, + kind: "pull_request", + }); + if (globalOpenCount > globalCap) { + // verifiedGlobalOpenItemCount sums BOTH open PRs and open issues (gate finding) -- reporting this + // as "pull requests" when the author's over-cap total may include issues would be a factually wrong + // close message. "pull requests and issues" is accurate regardless of the actual split. + contributorCapMatch = { matched: true, authorLogin: pr.authorLogin, openCount: globalOpenCount, cap: globalCap, itemKind: "pull requests and issues", scope: "install" }; } } } @@ -3931,6 +3939,78 @@ async function loadOpenQueueCounts( }; } +/** + * True when one row from listOpenItemsForAuthorAcrossInstall is CONFIRMED still open on GitHub right now + * (#2562 gate finding): the stored DB cache can lag GitHub for a repo OTHER than the one this webhook is for + * (closed manually, by another automation, or by a webhook this instance hasn't processed yet) -- an inflated + * stale count must never itself trigger an irreversible close. Fail SAFE, not fail-open: a row this call + * cannot POSITIVELY confirm is still open is excluded from the count (mirrors the existing per-repo issue-cap's + * own sibling live-verification, #2479). + */ +async function isOpenItemRowStillLiveOpen( + env: Env, + row: OpenItemAcrossInstallRow, + liveToken: string | undefined, + admissionKey: GitHubRateLimitAdmissionKey | undefined, +): Promise { + if (row.kind === "issue") { + const liveState = await fetchLiveIssueState(env, row.repoFullName, row.number, liveToken, admissionKey).catch(() => undefined); + return liveState === "open"; + } + const livePr = await fetchLivePullRequest(env, row.repoFullName, row.number, liveToken, admissionKey).catch(() => undefined); + return livePr?.state === "open"; +} + +// A contributor can have thousands of open rows across a large install (listOpenItemsForAuthorAcrossInstall +// caps at 20,000 PRs + 20,000 issues) -- an unbounded Promise.all over every one of them would fire that many +// concurrent GitHub API calls from a single webhook, exhausting the installation's rate limit for every OTHER +// repo it gates. Bounded worker-pool fan-out, mirroring the same fixed-concurrency shape already used +// elsewhere in this codebase for GitHub fan-out (e.g. src/github/backfill.ts's mapWithConcurrency). +const GLOBAL_OPEN_ITEM_LIVE_CHECK_CONCURRENCY = 10; + +async function mapWithConcurrency(items: T[], concurrency: number, mapper: (item: T) => Promise): Promise { + const results: R[] = new Array(items.length); + let nextIndex = 0; + const workerCount = Math.max(1, Math.min(concurrency, items.length || 1)); + await Promise.all( + Array.from({ length: workerCount }, async () => { + while (nextIndex < items.length) { + const index = nextIndex; + nextIndex += 1; + results[index] = await mapper(items[index] as T); + } + }), + ); + return results; +} + +/** + * Install-wide contributor open-item count, LIVE-VERIFIED and INSTALLATION-SCOPED (#2562, gate findings): the + * aggregate is scoped to `installationId`'s own repo set (a D1 database can serve MORE than one installation, + * so an unscoped aggregate could wrongly count a contributor's activity on a totally unrelated installation's + * repos toward a close here), and every OTHER counted item is live-verified before trusting it toward the cap + * (mirrors the existing per-repo issue-cap's own sibling live-verification, #2479). `currentItem` (the one THIS + * webhook just delivered) is trusted unverified, same as every other cap check in this file. + */ +async function verifiedGlobalOpenItemCount( + env: Env, + installationId: number, + authorLogin: string, + currentItem: { repoFullName: string; number: number; kind: "pull_request" | "issue" }, +): Promise { + const rows = await listOpenItemsForAuthorAcrossInstall(env, installationId, authorLogin); + const otherRows = rows.filter( + (row) => !(row.repoFullName === currentItem.repoFullName && row.number === currentItem.number && row.kind === currentItem.kind), + ); + const token = await createInstallationToken(env, installationId).catch(() => undefined); + const liveToken = token ?? env.GITHUB_PUBLIC_TOKEN; + const admissionKey = githubAdmissionKeyForToken(env, installationId, liveToken); + const confirmedOpen = await mapWithConcurrency(otherRows, GLOBAL_OPEN_ITEM_LIVE_CHECK_CONCURRENCY, (row) => + isOpenItemRowStillLiveOpen(env, row, liveToken, admissionKey), + ); + return confirmedOpen.filter(Boolean).length + 1; +} + /** * Per-contributor open-ISSUE cap (#2270, anti-abuse): the first `eventName === "issues"` actuation branch — * issues have no other auto-close path today. Mirrors the PR-path cap in runAgentMaintenancePlanAndExecute: @@ -3981,8 +4061,12 @@ async function maybeCloseIssueOverContributorCap( // match here closes THIS issue directly (unlike the per-repo cap below, there is no cross-repo sibling set to // union/live-verify -- the aggregate count already covers every repo, so a single over-cap read is enough). if (globalCap !== null && !isAutoCloseExempt(authorLogin, settings.autoCloseExemptLogins)) { - const installOpenCount = await countOpenItemsForAuthorAcrossRepos(env, authorLogin); - if (installOpenCount > globalCap) { + const globalOpenCount = await verifiedGlobalOpenItemCount(env, installationId, authorLogin, { + repoFullName, + number: issue.number, + kind: "issue", + }); + if (globalOpenCount > globalCap) { const planned = planAgentMaintenanceActions({ conclusion: "skipped", blockerTitles: [], @@ -3993,7 +4077,9 @@ async function maybeCloseIssueOverContributorCap( authorIsAdmin, authorIsAutomationBot, ciState: "unverified", - contributorCapMatch: { matched: true, authorLogin, openCount: installOpenCount, cap: globalCap, itemKind: "issues", scope: "install" }, + // verifiedGlobalOpenItemCount sums BOTH open PRs and open issues (gate finding); "pull requests and + // issues" is accurate regardless of the actual split, unlike a hardcoded single kind. + contributorCapMatch: { matched: true, authorLogin, openCount: globalOpenCount, cap: globalCap, itemKind: "pull requests and issues", scope: "install" }, contributorCapLabel: settings.contributorCapLabel, pr: { labels: [] }, }); diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index e5630a645f..294e352ed6 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -169,11 +169,13 @@ export type AgentActionPlanInput = { // so — unlike the blacklist's private-reason close — they ARE interpolated into the public close comment. // `itemKind` selects the close-comment noun ("pull requests" for the PR-path caller, "issues" for the // issue-path caller, #2270) — REQUIRED (not defaulted) so a caller can't silently mislabel the other kind. + // "pull requests and issues" (#2562) is for the install-wide globalContributorOpenItemCap, whose count sums + // BOTH kinds across the install — a single-kind label there would misstate a mixed-kind contributor's count. // `scope` (#2562) selects the close-comment's cap description: "repository" (default when absent, back-compat // for every existing per-repo caller) says "this repository's configured limit"; "install" says "across every // repository this install gates, combined" for the install-wide globalContributorOpenItemCap. Same closeKind // ("contributor_cap") and label either way — this is a description-only distinction, not a new disposition. - contributorCapMatch?: { matched: boolean; authorLogin: string; openCount: number; cap: number; itemKind: "pull requests" | "issues"; scope?: "repository" | "install" | undefined } | undefined; + contributorCapMatch?: { matched: boolean; authorLogin: string; openCount: number; cap: number; itemKind: "pull requests" | "issues" | "pull requests and issues"; scope?: "repository" | "install" | undefined } | undefined; // The repo-configured label applied to an over-cap author's PR/issue (#2270), resolved from `.gittensory.yml`. // Absent ⇒ the default (`DEFAULT_CONTRIBUTOR_CAP_LABEL` = "over-contributor-limit"). contributorCapLabel?: string | undefined; @@ -315,7 +317,7 @@ function blacklistCloseMessage(): string { // (#2562) picks the cap description: "repository" (default, back-compat for every existing per-repo caller) vs. // "install" for the install-wide globalContributorOpenItemCap — same message shape, closeKind, and label either // way, just an accurate noun phrase for where the count was aggregated. -function contributorCapCloseMessage(authorLogin: string, openCount: number, cap: number, itemNoun: "pull requests" | "issues", scope?: "repository" | "install" | undefined): string { +function contributorCapCloseMessage(authorLogin: string, openCount: number, cap: number, itemNoun: "pull requests" | "issues" | "pull requests and issues", scope?: "repository" | "install" | undefined): string { const scopeDescription = scope === "install" ? "this install's configured limit (across every repository it gates, combined)" : "this repository's configured limit"; return `Gittensory closed this because @${authorLogin} has ${openCount} open ${itemNoun}, above ${scopeDescription} of ${cap}. Close or merge an existing one to open a new one. This is an automated maintenance action.`; } diff --git a/test/unit/global-contributor-cap.test.ts b/test/unit/global-contributor-cap.test.ts index 70777b79ed..da94e0665b 100644 --- a/test/unit/global-contributor-cap.test.ts +++ b/test/unit/global-contributor-cap.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { resolveGlobalContributorOpenItemCap } from "../../src/settings/global-contributor-cap"; -import { countOpenItemsForAuthorAcrossRepos, upsertIssueFromGitHub, upsertPullRequestFromGitHub } from "../../src/db/repositories"; +import { listOpenItemsForAuthorAcrossInstall, upsertIssueFromGitHub, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; describe("resolveGlobalContributorOpenItemCap (#2562)", () => { @@ -24,28 +24,84 @@ describe("resolveGlobalContributorOpenItemCap (#2562)", () => { }); }); -describe("countOpenItemsForAuthorAcrossRepos (#2562)", () => { - it("sums open PRs + open issues for one author across EVERY repo in the database, not just one", async () => { +describe("listOpenItemsForAuthorAcrossInstall (#2562)", () => { + it("lists one author's open PRs + open issues across every repo THIS INSTALLATION tracks, excludes closed items/other authors/other installations, and matches case-insensitively", async () => { const env = createTestEnv(); - await upsertPullRequestFromGitHub(env, "org/repo-a", { number: 1, title: "a1", state: "open", user: { login: "farmer99" } }); - await upsertPullRequestFromGitHub(env, "org/repo-b", { number: 2, title: "b1", state: "open", user: { login: "farmer99" } }); - await upsertIssueFromGitHub(env, "org/repo-c", { number: 3, title: "c1", state: "open", user: { login: "farmer99" } }); - // A closed item and a different author's item must NOT count toward the total. - await upsertPullRequestFromGitHub(env, "org/repo-a", { number: 4, title: "a2 (closed)", state: "closed", user: { login: "farmer99" } }); - await upsertPullRequestFromGitHub(env, "org/repo-a", { number: 5, title: "a3 (other author)", state: "open", user: { login: "someone-else" } }); - - expect(await countOpenItemsForAuthorAcrossRepos(env, "farmer99")).toBe(3); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "owner/repo-a", owner: { login: "owner" } }, 123); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "owner/repo-b", owner: { login: "owner" } }, 123); + // farmer99's open items, spread across TWO different repos this install (123) gates. + await upsertPullRequestFromGitHub(env, "owner/repo-a", { number: 1, title: "PR one", state: "open", user: { login: "farmer99" }, labels: [], body: "x" }); + await upsertPullRequestFromGitHub(env, "owner/repo-b", { number: 2, title: "PR two", state: "open", user: { login: "farmer99" }, labels: [], body: "y" }); + await upsertIssueFromGitHub(env, "owner/repo-a", { number: 3, title: "Issue one", state: "open", user: { login: "farmer99" }, labels: [], body: "z" }); + // A CLOSED item from farmer99 — must be excluded. + await upsertPullRequestFromGitHub(env, "owner/repo-b", { number: 4, title: "PR three (closed)", state: "closed", user: { login: "farmer99" }, labels: [], body: "w" }); + // An OPEN item from a DIFFERENT author — must be excluded. + await upsertIssueFromGitHub(env, "owner/repo-a", { number: 5, title: "Someone else's issue", state: "open", user: { login: "other-author" }, labels: [], body: "v" }); + // Gate finding (#2562): an open item from farmer99 on a repo belonging to a DIFFERENT installation, in the + // SAME D1 database, must be excluded -- this is exactly the cross-installation boundary the fix enforces. + await upsertRepositoryFromGitHub(env, { name: "other-install-repo", full_name: "other-owner/other-install-repo", owner: { login: "other-owner" } }, 456); + await upsertPullRequestFromGitHub(env, "other-owner/other-install-repo", { number: 6, title: "Farmer PR on a different installation", state: "open", user: { login: "farmer99" }, labels: [], body: "u" }); + + // 2 open PRs + 1 open issue for farmer99, across repo-a and repo-b combined -- NOT the 4th, cross-install one. + const rows = await listOpenItemsForAuthorAcrossInstall(env, 123, "farmer99"); + expect(rows).toHaveLength(3); + expect(rows).toEqual( + expect.arrayContaining([ + { repoFullName: "owner/repo-a", number: 1, kind: "pull_request" }, + { repoFullName: "owner/repo-b", number: 2, kind: "pull_request" }, + { repoFullName: "owner/repo-a", number: 3, kind: "issue" }, + ]), + ); + expect(rows.some((row) => row.repoFullName === "other-owner/other-install-repo")).toBe(false); + // The OTHER installation sees only its own repo's item. + expect(await listOpenItemsForAuthorAcrossInstall(env, 456, "farmer99")).toEqual([{ repoFullName: "other-owner/other-install-repo", number: 6, kind: "pull_request" }]); + // Case-insensitive: a differently-cased login still matches the same rows. + expect(await listOpenItemsForAuthorAcrossInstall(env, 123, "FARMER99")).toHaveLength(3); + // An author with no open items anywhere lists nothing. + expect(await listOpenItemsForAuthorAcrossInstall(env, 123, "nobody")).toEqual([]); + // An installation with no repos registered at all lists nothing (never throws, never scans the whole DB). + expect(await listOpenItemsForAuthorAcrossInstall(env, 999, "farmer99")).toEqual([]); }); - it("is case-insensitive on the author login (mirrors loginMatches/findBlacklistEntry elsewhere)", async () => { + it("audits (never silently drops) when an author's open items across the install hit the list limit (#regate-review)", async () => { const env = createTestEnv(); - await upsertPullRequestFromGitHub(env, "org/repo-a", { number: 1, title: "a1", state: "open", user: { login: "Farmer99" } }); - expect(await countOpenItemsForAuthorAcrossRepos(env, "farmer99")).toBe(1); - expect(await countOpenItemsForAuthorAcrossRepos(env, "FARMER99")).toBe(1); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "owner/repo-a", owner: { login: "owner" } }, 123); + const LIMIT = 20_000; + const now = new Date().toISOString(); + const prValues = Array.from({ length: LIMIT }, (_, i) => `('pr-${i}', 'owner/repo-a', ${i + 1}, 'PR ${i}', 'open', 'farmer99', '[]', '${now}', '${now}')`).join(","); + await env.DB.prepare( + `INSERT INTO pull_requests (id, repo_full_name, number, title, state, author_login, labels_json, created_at, updated_at) VALUES ${prValues}`, + ).run(); + // Issue numbers also hit the limit — both the PR side AND the issue side of the truncation check must fire. + const issueValues = Array.from({ length: LIMIT }, (_, i) => `('issue-${i}', 'owner/repo-a', ${i + 1}, 'Issue ${i}', 'open', 'farmer99', '[]', '${now}', '${now}')`).join(","); + await env.DB.prepare( + `INSERT INTO issues (id, repo_full_name, number, title, state, author_login, labels_json, created_at, updated_at) VALUES ${issueValues}`, + ).run(); + + const rows = await listOpenItemsForAuthorAcrossInstall(env, 123, "farmer99"); + expect(rows).toHaveLength(LIMIT * 2); // both PR and issue results truncated at the limit each, not silently fewer + + const audit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("agent.global_open_item_cap.author_items_truncated", "farmer99@installation:123") + .first<{ n: number }>(); + expect(audit?.n).toBe(2); // one row for the PR truncation, one for the issue truncation }); - it("returns 0 for an author with no open items anywhere", async () => { + it("audits when an installation's own repo set hits the list limit (#regate-review)", async () => { const env = createTestEnv(); - expect(await countOpenItemsForAuthorAcrossRepos(env, "nobody")).toBe(0); + const LIMIT = 20_000; + const now = new Date().toISOString(); + const values = Array.from({ length: LIMIT }, (_, i) => `('owner/repo-${i}', 'owner', 'repo-${i}', 123, '${now}', '${now}')`).join(","); + await env.DB.prepare( + `INSERT INTO repositories (full_name, owner, name, installation_id, created_at, updated_at) VALUES ${values}`, + ).run(); + + const rows = await listOpenItemsForAuthorAcrossInstall(env, 123, "nobody-in-particular"); + expect(rows).toEqual([]); // no open items for this author, but the repo-list truncation must still be audited + + const audit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("agent.global_open_item_cap.repo_list_truncated", "installation:123") + .first<{ n: number }>(); + expect(audit?.n).toBe(1); }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 9f40620299..a55ee0ad93 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -7406,6 +7406,11 @@ describe("queue processors", () => { { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, ], }); + // The global cap scopes its cross-repo query to THIS installation's own repo set (#2562 gate finding) -- + // upsertInstallation's own `repositories` payload field is NOT persisted, so each repo must be registered + // explicitly against installation 123 or listOpenItemsForAuthorAcrossInstall would see zero repos. + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "JSONbored/repo-a", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, 123); await upsertPullRequestFromGitHub(env, "JSONbored/repo-a", { number: 20, title: "Farmer PR on repo-a", state: "open", user: { login: "farmer99" }, head: { sha: "fa20" }, labels: [], body: "x" }); await upsertPullRequestFromGitHub(env, "JSONbored/repo-b", { number: 10, title: "Farmer PR on repo-b", state: "open", user: { login: "farmer99" }, head: { sha: "fb10" }, labels: [], body: "y" }); await upsertRepositorySettings(env, { @@ -7435,6 +7440,10 @@ describe("queue processors", () => { if (url.includes("/issues/55/labels") && method === "POST") { seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); return Response.json([]); } if (url.includes("/issues/55/comments") && method === "POST") { seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); return Response.json({ id: 1 }, { status: 201 }); } if (url.includes("/issues/55/comments")) return Response.json([]); + // Live-verification of the OTHER install-wide siblings (#2562, gate finding): the global cap + // live-confirms every OTHER counted item before trusting it toward the cap. + if (url.endsWith("/repos/JSONbored/repo-a/pulls/20")) return Response.json({ number: 20, state: "open" }); + if (url.endsWith("/repos/JSONbored/repo-b/pulls/10")) return Response.json({ number: 10, state: "open" }); return Response.json({}); }); @@ -8458,6 +8467,8 @@ describe("queue processors", () => { { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, ], }); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "JSONbored/repo-a", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, 123); await upsertIssueFromGitHub(env, "JSONbored/repo-a", { number: 20, title: "Farmer issue on repo-a", state: "open", user: { login: "farmer99" }, labels: [], body: "x" }); await upsertIssueFromGitHub(env, "JSONbored/repo-b", { number: 10, title: "Farmer issue on repo-b", state: "open", user: { login: "farmer99" }, labels: [], body: "y" }); // No contributorOpenIssueCap set — only the install-wide env cap should catch this. @@ -8471,6 +8482,11 @@ describe("queue processors", () => { if (url.includes("/issues/62/labels") && method === "GET") return Response.json([]); if (url.includes("/issues/62/labels") && method === "POST") { seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); return Response.json([]); } if (url.includes("/issues/62/comments") && method === "POST") { seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); return Response.json({ id: 1 }, { status: 201 }); } + // Live-verification of the OTHER install-wide siblings (#2562, gate finding): the global cap + // live-confirms every OTHER counted item before trusting it toward the cap. Issues use the same + // GET /issues/{n} endpoint the issue-state fetcher reads (open PRs are also reachable there). + if (url.endsWith("/repos/JSONbored/repo-a/issues/20")) return Response.json({ number: 20, state: "open" }); + if (url.endsWith("/repos/JSONbored/repo-b/issues/10")) return Response.json({ number: 10, state: "open" }); return Response.json({}); }); @@ -8488,7 +8504,7 @@ describe("queue processors", () => { expect(seen.closed).toBe(true); expect(seen.labels).toContain("over-contributor-limit"); - expect(seen.comments.some((c) => c.includes("@farmer99") && c.includes("3 open issues") && c.includes("across every repository it gates"))).toBe(true); + expect(seen.comments.some((c) => c.includes("@farmer99") && c.includes("3 open pull requests and issues") && c.includes("across every repository it gates"))).toBe(true); }); it("install-wide contributor open-item cap (#2562): off by default (env var unset) — an issue author spread across repos is never closed", async () => { @@ -8500,6 +8516,8 @@ describe("queue processors", () => { { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, ], }); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "JSONbored/repo-a", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, 123); await upsertIssueFromGitHub(env, "JSONbored/repo-a", { number: 20, title: "Farmer issue on repo-a", state: "open", user: { login: "farmer99" }, labels: [], body: "x" }); await upsertIssueFromGitHub(env, "JSONbored/repo-b", { number: 10, title: "Farmer issue on repo-b", state: "open", user: { login: "farmer99" }, labels: [], body: "y" }); await upsertRepositorySettings(env, { repoFullName: "JSONbored/repo-a", autonomy: { close: "auto", label: "auto" } }); @@ -8577,6 +8595,8 @@ describe("queue processors", () => { { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, ], }); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "JSONbored/repo-a", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, 123); await upsertIssueFromGitHub(env, "JSONbored/repo-a", { number: 20, title: "Farmer issue on repo-a", state: "open", user: { login: "farmer99" }, labels: [], body: "x" }); await upsertIssueFromGitHub(env, "JSONbored/repo-b", { number: 10, title: "Farmer issue on repo-b", state: "open", user: { login: "farmer99" }, labels: [], body: "y" }); // autonomy: {} (no acting classes granted) — the plan builds empty, so `planned.length > 0` is false.