diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 8354d771d4..6ff5aa0b65 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3104,20 +3104,54 @@ export async function countOpenPullRequests(env: Env, fullName: string): Promise return Number(row?.count ?? 0); } +const INSTALLATION_REPO_LIST_LIMIT = 20_000; + +/** List every repo's fullName tracked under one installation (regression fix, #2562): pullRequests/issues have + * no installationId column of their own (only repoFullName, a plain string, matched by convention against + * repositories.fullName -- this codebase has no Drizzle joins to lean on instead), so scoping a cross-repo + * aggregate to one install means resolving its repo set FIRST, mirroring markRepositoriesRemovedFromInstallation + * (same file). + * + * INSTALLATION_REPO_LIST_LIMIT (gate finding): raised far above any realistic install size so truncation should + * never occur in practice, but a silently truncated repo set would understate countOpenItemsForAuthorAcrossRepos + * for that installation with no signal anything was dropped -- record an audit event on the rare install where + * the limit is still hit, rather than pretending completeness this query can't actually guarantee unbounded. */ +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 recordAuditEvent(env, { + eventType: "agent.global_open_item_cap.repo_list_truncated", + actor: "gittensory", + targetKey: `installation:${installationId}`, + outcome: "error", + detail: `installation has >= ${INSTALLATION_REPO_LIST_LIMIT} repos; the global contributor-cap check may undercount repos not included here`, + }).catch(() => undefined); + } + 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 + * issues across every repo THIS INSTALLATION tracks. 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). + * + * Installation-scoped (regression fix): pullRequests/issues rows carry no installationId of their own, only + * repoFullName. The original version of this query filtered by authorLogin alone with no installation scoping + * at all, so on a D1 database shared by MULTIPLE installations (the hosted product's normal shape, and possible + * on self-host too -- the same App installed against more than one org/account) a contributor's open items on + * a DIFFERENT, unrelated installation would count toward (and could wrongly close a PR on) an install that + * never gated them on -- the exact cross-tenant leak install-scoped helpers elsewhere in this codebase (e.g. + * markRepositoriesRemovedFromInstallation) exist to avoid. */ -export async function countOpenItemsForAuthorAcrossRepos(env: Env, authorLogin: string): Promise { +export async function countOpenItemsForAuthorAcrossRepos(env: Env, installationId: number, authorLogin: string): Promise { + const repoNames = await listRepoFullNamesForInstallation(env, installationId); + if (repoNames.length === 0) return 0; 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))), + db.select({ count: sql`count(*)` }).from(pullRequests).where(and(eq(pullRequests.state, "open"), loginMatches(pullRequests.authorLogin, authorLogin), inArray(pullRequests.repoFullName, repoNames))), + db.select({ count: sql`count(*)` }).from(issues).where(and(eq(issues.state, "open"), loginMatches(issues.authorLogin, authorLogin), inArray(issues.repoFullName, repoNames))), ]); /* v8 ignore next -- SQL aggregate count always returns one row; fallback protects D1 driver anomalies. */ const prCount = Number(prRow?.count ?? 0); diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3159e97d43..c57f70e186 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2074,7 +2074,7 @@ 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); + const installOpenCount = await countOpenItemsForAuthorAcrossRepos(env, installationId, pr.authorLogin); if (installOpenCount > globalCap) { contributorCapMatch = { matched: true, authorLogin: pr.authorLogin, openCount: installOpenCount, cap: globalCap, itemKind: "pull requests", scope: "install" }; } @@ -3981,7 +3981,7 @@ 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); + const installOpenCount = await countOpenItemsForAuthorAcrossRepos(env, installationId, authorLogin); if (installOpenCount > globalCap) { const planned = planAgentMaintenanceActions({ conclusion: "skipped", diff --git a/test/unit/global-contributor-cap.test.ts b/test/unit/global-contributor-cap.test.ts index 70777b79ed..71f1ec7853 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 { countOpenItemsForAuthorAcrossRepos, upsertIssueFromGitHub, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; describe("resolveGlobalContributorOpenItemCap (#2562)", () => { @@ -25,8 +25,11 @@ 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 () => { + it("sums open PRs + open issues for one author across EVERY repo THIS INSTALLATION tracks, not just one", async () => { const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org/repo-a", owner: { login: "org" } }, 123); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "org/repo-b", owner: { login: "org" } }, 123); + await upsertRepositoryFromGitHub(env, { name: "repo-c", full_name: "org/repo-c", owner: { login: "org" } }, 123); 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" } }); @@ -34,18 +37,63 @@ describe("countOpenItemsForAuthorAcrossRepos (#2562)", () => { 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); + expect(await countOpenItemsForAuthorAcrossRepos(env, 123, "farmer99")).toBe(3); }); it("is case-insensitive on the author login (mirrors loginMatches/findBlacklistEntry elsewhere)", async () => { const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org/repo-a", owner: { login: "org" } }, 123); 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); + expect(await countOpenItemsForAuthorAcrossRepos(env, 123, "farmer99")).toBe(1); + expect(await countOpenItemsForAuthorAcrossRepos(env, 123, "FARMER99")).toBe(1); }); it("returns 0 for an author with no open items anywhere", async () => { const env = createTestEnv(); - expect(await countOpenItemsForAuthorAcrossRepos(env, "nobody")).toBe(0); + expect(await countOpenItemsForAuthorAcrossRepos(env, 123, "nobody")).toBe(0); + }); + + it("returns 0 for an installation that tracks no repos yet (regression fix, gate finding: no repoNames means no rows to scope against, not an unscoped fetch-everything)", async () => { + const env = createTestEnv(); + expect(await countOpenItemsForAuthorAcrossRepos(env, 999, "farmer99")).toBe(0); + }); + + it("REGRESSION (cross-tenant leak fix): an author's open items on a DIFFERENT installation do not count toward this installation's cap, even though both installations share the same D1 database", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org-a/repo-a", owner: { login: "org-a" } }, 123); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "org-b/repo-b", owner: { login: "org-b" } }, 456); + // farmer99 has open items on BOTH installations -- only installation 123's own item may count toward 123's cap. + await upsertPullRequestFromGitHub(env, "org-a/repo-a", { number: 1, title: "a1", state: "open", user: { login: "farmer99" } }); + await upsertPullRequestFromGitHub(env, "org-b/repo-b", { number: 2, title: "b1", state: "open", user: { login: "farmer99" } }); + await upsertIssueFromGitHub(env, "org-b/repo-b", { number: 3, title: "b2", state: "open", user: { login: "farmer99" } }); + + expect(await countOpenItemsForAuthorAcrossRepos(env, 123, "farmer99")).toBe(1); + expect(await countOpenItemsForAuthorAcrossRepos(env, 456, "farmer99")).toBe(2); + }); + + it("audits (never silently drops) when an installation's own repo set hits the list limit (gate finding)", async () => { + const env = createTestEnv(); + const LIMIT = 20_000; + const now = new Date().toISOString(); + const values = Array.from({ length: LIMIT }, (_, i) => `('org/repo-${i}', 'org', '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(); + await upsertPullRequestFromGitHub(env, "org/repo-0", { number: 1, title: "a1", state: "open", user: { login: "farmer99" } }); + + expect(await countOpenItemsForAuthorAcrossRepos(env, 123, "farmer99")).toBe(1); + + 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); + }); + + it("does NOT audit a repo-list truncation when the installation's repo count is well under the limit", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo-a", full_name: "org/repo-a", owner: { login: "org" } }, 123); + + expect(await countOpenItemsForAuthorAcrossRepos(env, 123, "farmer99")).toBe(0); + + const audit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.global_open_item_cap.repo_list_truncated'").first<{ n: number }>(); + expect(audit?.n ?? 0).toBe(0); }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 9f40620299..cde9932b28 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" } }, ], }); + // upsertInstallation's own `repositories:` array is NOT itself persisted to the `repositories` table (only + // the `installations` row) -- the real webhook pipeline registers a repo's installationId as a side effect + // of processing an event FOR that repo, which never happens here for repo-b (the non-webhook-triggered repo). + // Register it explicitly so countOpenItemsForAuthorAcrossRepos's installation-scoped lookup can find its rows. + 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, { @@ -7466,6 +7471,7 @@ describe("queue processors", () => { { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, ], }); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, 123); 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, { repoFullName: "JSONbored/repo-a", @@ -7522,6 +7528,7 @@ describe("queue processors", () => { { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, ], }); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, 123); 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, { repoFullName: "JSONbored/repo-a", @@ -7581,6 +7588,7 @@ describe("queue processors", () => { { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, ], }); + await upsertRepositoryFromGitHub(env, { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, 123); 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, { repoFullName: "JSONbored/repo-a", @@ -8458,6 +8466,7 @@ describe("queue processors", () => { { name: "repo-b", full_name: "JSONbored/repo-b", private: false, owner: { login: "JSONbored" } }, ], }); + 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.