From f0e5f5b6414a895d49aee30f08de2ed795823a0a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 22:52:51 -0700 Subject: [PATCH 1/2] fix(agent-actions): scope the global open-item cap query by installation countOpenItemsForAuthorAcrossRepos (merged via #2678, closing #2562) counted an author's open PRs/issues across the ENTIRE D1 database with no installation scoping at all -- on a database shared by multiple installations (the hosted product's normal shape, and possible on self-host too), a contributor's activity on one installation could wrongly trigger GLOBAL_CONTRIBUTOR_OPEN_ITEM_CAP closes on a completely unrelated installation that never gated them. Scope the query through repositories.installationId first (matching the existing markRepositoriesRemovedFromInstallation precedent), then inArray(...) against the resulting repoFullNames -- this codebase has no Drizzle joins to lean on instead. Also fixes 5 AiReviewCacheInput test fixtures in queue.test.ts left broken by an unrelated already-merged PR (#2675, security-focused review profile) that added a required securityFocus field without updating these fixtures -- main's typecheck was red without this, which this PR's own CI would otherwise have inherited. --- src/db/repositories.ts | 32 +++++++++++++++++----- src/queue/processors.ts | 4 +-- test/unit/global-contributor-cap.test.ts | 34 +++++++++++++++++++----- test/unit/queue.test.ts | 9 +++++++ 4 files changed, 64 insertions(+), 15 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 8354d771d4..0317482ae1 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3104,20 +3104,38 @@ export async function countOpenPullRequests(env: Env, fullName: string): Promise return Number(row?.count ?? 0); } +/** 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). */ +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(20_000); + 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..1a82d81476 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,37 @@ 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); }); }); 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. From 0a741564f6d2fca0d13d5ac0da0d37a35b6f53d3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 23:16:29 -0700 Subject: [PATCH 2/2] fix(agent-actions): audit a truncated installation repo list, don't drop it silently listRepoFullNamesForInstallation's .limit(20_000) meant an installation with more tracked repos than that would silently undercount toward GLOBAL_CONTRIBUTOR_OPEN_ITEM_CAP with no signal anything was dropped. Records an audit event on the rare install where the limit is still hit, mirroring the same observability pattern already used for the per-author item-count truncation in this file. Addresses a gate review finding on #2687. --- src/db/repositories.ts | 20 ++++++++++++++++-- test/unit/global-contributor-cap.test.ts | 26 ++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 0317482ae1..6ff5aa0b65 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3104,14 +3104,30 @@ 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). */ + * (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(20_000); + 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); } diff --git a/test/unit/global-contributor-cap.test.ts b/test/unit/global-contributor-cap.test.ts index 1a82d81476..71f1ec7853 100644 --- a/test/unit/global-contributor-cap.test.ts +++ b/test/unit/global-contributor-cap.test.ts @@ -70,4 +70,30 @@ describe("countOpenItemsForAuthorAcrossRepos (#2562)", () => { 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); + }); });