fix(agent-actions): live-verify + installation-scope the global contributor cap - #2689
Closed
JSONbored wants to merge 1 commit into
Closed
fix(agent-actions): live-verify + installation-scope the global contributor cap#2689JSONbored wants to merge 1 commit into
JSONbored wants to merge 1 commit into
Conversation
…ibutor cap Fixes two confirmed bugs in the already-merged #2678 (issue #2562): the install-wide open-item count trusted the raw D1 cache with no live verification before an irreversible close (the sibling per-repo issue cap already avoids this exact failure mode), and itemKind was hardcoded to a single kind even though the count sums PRs + issues, producing a factually wrong close-comment noun for a mixed-kind contributor. Also scopes the aggregate query to the calling installation's own repo set -- an unscoped cross-database query could wrongly count a contributor's activity on a totally unrelated installation's repos toward a close here. Addresses gate-review findings on #2678.
| 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) => |
Contributor
There was a problem hiding this comment.
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>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2689 +/- ##
==========================================
- Coverage 96.13% 96.13% -0.01%
==========================================
Files 239 239
Lines 26777 26817 +40
Branches 9719 9727 +8
==========================================
+ Hits 25743 25781 +38
Misses 424 424
- Partials 610 612 +2
🚀 New features to boost your workflow:
|
Owner
Author
|
Superseded by a fresh PR from current main — this branch predates an installation-scoping fix that landed concurrently on main and conflicts here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adversarial review of the already-merged #2678 (issue #2562) found two confirmed, empirically-verified bugs, and a third surfaced while porting the fix:
countOpenItemsForAuthorAcrossRepostrusted the raw D1 cache directly. The sibling per-repo issue-cap path (maybeCloseIssueOverContributorCap) already live-verifies each sibling viafetchLiveIssueStatebefore trusting it toward a close, specifically because the stored cache can lag GitHub. The install-wide path skipped this entirely.contributorCapMatch.itemKindwas hardcoded to"pull requests"(PR call site) or"issues"(issue call site) even though the count sums PRs + issues together — a mixed-kind contributor got a close message with a wrong noun/count.installationIdfilter — it summed across the ENTIRE D1 database regardless of which GitHub App installation each repo belongs to. A D1 instance serving more than one installation could wrongly count a contributor's unrelated-installation activity toward a close here.Replaced
countOpenItemsForAuthorAcrossRepos(a count) withlistOpenItemsForAuthorAcrossInstall(rows, installation-scoped), addedverifiedGlobalOpenItemCountwhich live-confirms every OTHER counted item (bounded worker-pool fan-out, concurrency 10, to avoid an unbounded burst against the installation's rate limit) before trusting it toward the cap, and wideneditemKindto include"pull requests and issues"for the install-wide case.Addresses gate-review findings on #2678.
Test plan
npm run typechecknpx vitest run test/unit/global-contributor-cap.test.ts test/unit/queue.test.ts— 395/395 pass, including all 9 existing install-wide cap webhook-flow tests (updated to register repos under the calling installation, matching the new installation-scoping requirement) and 3 new/updatedlistOpenItemsForAuthorAcrossInstalltests covering cross-installation isolation and list-truncation observability