Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 41 additions & 7 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string[]> {
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<number> {
export async function countOpenItemsForAuthorAcrossRepos(env: Env, installationId: number, authorLogin: string): Promise<number> {
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<number>`count(*)` }).from(pullRequests).where(and(eq(pullRequests.state, "open"), loginMatches(pullRequests.authorLogin, authorLogin))),
db.select({ count: sql<number>`count(*)` }).from(issues).where(and(eq(issues.state, "open"), loginMatches(issues.authorLogin, authorLogin))),
db.select({ count: sql<number>`count(*)` }).from(pullRequests).where(and(eq(pullRequests.state, "open"), loginMatches(pullRequests.authorLogin, authorLogin), inArray(pullRequests.repoFullName, repoNames))),
db.select({ count: sql<number>`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);
Expand Down
4 changes: 2 additions & 2 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" };
}
Expand Down Expand Up @@ -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",
Expand Down
60 changes: 54 additions & 6 deletions test/unit/global-contributor-cap.test.ts
Original file line number Diff line number Diff line change
@@ -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)", () => {
Expand All @@ -25,27 +25,75 @@ 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" } });
// 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);
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);
});
});
9 changes: 9 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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.
Expand Down
Loading