Skip to content

fix(agent-actions): live-verify + installation-scope the global contributor cap #2689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JSONbored wants to merge 1 commit into from
Closed

fix(agent-actions): live-verify + installation-scope the global contributor cap #2689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
99 changes: 82 additions & 17 deletions src/db/repositories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
/* 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<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 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<number> {
export async function listOpenItemsForAuthorAcrossInstall(env: Env, installationId: number, authorLogin: string): Promise<OpenItemAcrossInstallRow[]> {
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<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))),
]);
/* 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
Expand Down
102 changes: 94 additions & 8 deletions src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {
countOpenIssues,
countOpenItemsForAuthorAcrossRepos,
countOpenPullRequests,
listOpenItemsForAuthorAcrossInstall,
type OpenItemAcrossInstallRow,
getAgentCommandAnswer,
getInstallation,
getLatestRepoGithubTotalsSnapshot,
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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" };
}
}
}
Expand Down Expand Up @@ -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<boolean> {
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<T, R>(items: T[], concurrency: number, mapper: (item: T) => Promise<R>): Promise<R[]> {
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<number> {
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) =>

@superagent-security superagent-security Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 verifiedGlobalOpenItemCount can exhaust GitHub rate limits via unbounded live verification fan-out

A single webhook can trigger up to 40,000 GitHub API calls, exhausting the installation's rate limit.

Short-circuit live verification once globalCap confirmed items are reached.

AI prompt 
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="src/queue/processors.ts">
<violation number="1" location="src/queue/processors.ts:4008">
<priority>P2</priority>
<title>verifiedGlobalOpenItemCount can exhaust GitHub rate limits via unbounded live verification fan-out</title>
<evidence>verifiedGlobalOpenItemCount lists all open items for an author across an installation (up to 20,000 PRs + 20,000 issues from listOpenItemsForAuthorAcrossInstall) and then calls mapWithConcurrency(otherRows, GLOBAL_OPEN_ITEM_LIVE_CHECK_CONCURRENCY, ...) to live-verify every single item via GitHub API, even though only globalCap items need to be confirmed. With concurrency 10 and up to 39,999 other rows, a single webhook can consume thousands of API calls and occupy the worker for minutes, exhausting the installation's rate limit and denying service to other repos.</evidence>
<recommendation>Short-circuit the live verification once globalCap confirmed-open items have been found, instead of checking every row. Alternatively, cap the total number of live checks to a small multiple of globalCap, or cache live-verification results for a short TTL.</recommendation>
</violation>
</file>

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:
Expand Down Expand Up @@ -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: [],
Expand All @@ -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: [] },
});
Expand Down
6 changes: 4 additions & 2 deletions src/settings/agent-actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.`;
}
Expand Down
Loading
Loading