Skip to content

Commit 3cf66ef

Browse files
committed
fix(agent-actions): wake sibling PRs a prior delivery missed over the cap (#2479)
Gate finding on this PR: contributorCapMatch was computed only from otherOpenPullRequests, so webhook delivery order (not guaranteed to match PR creation order) could let a sibling's own webhook process before this PR existed in the DB — that sibling would wrongly conclude it's within the cap, and since nothing else ever re-evaluates it, the cap would be permanently bypassed for that PR. Once a delivery has the complete picture, it now wakes any other still- open sibling that's also in the over-cap set via the existing agent-regate-pr sweep-unit job — the same "wake and fully re-evaluate" entry point the linked-issue-wake feature (#2259) uses for an identical class of problem, so the sibling gets its own live-head/CI-freshness re-check rather than acting on a shortcut. Coalesced per sibling PR to avoid an O(N^2) job storm from a burst of over-cap siblings.
1 parent 1d080a7 commit 3cf66ef

2 files changed

Lines changed: 259 additions & 1 deletion

File tree

src/queue/processors.ts

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1738,7 +1738,7 @@ async function runAgentMaintenancePlanAndExecute(
17381738
liveFacts: LiveGithubFacts;
17391739
},
17401740
): Promise<void> {
1741-
const { installationId, repoFullName, pr, settings, otherOpenPullRequests, gate } = args;
1741+
const { installationId, repoFullName, pr, settings, otherOpenPullRequests, deliveryId, gate } = args;
17421742

17431743
// Convergence safety: feed the planner the PR's changed paths + the repo's hard-guardrail globs so guarded
17441744
// paths force manual review, and flag owner-authored PRs so they are never auto-closed (standing rule).
@@ -1882,6 +1882,17 @@ async function runAgentMaintenancePlanAndExecute(
18821882
if (overCapNumbers.has(pr.number)) {
18831883
contributorCapMatch = { matched: true, authorLogin: pr.authorLogin, openCount: authorOpenPrNumbers.length, cap: contributorOpenPrCap };
18841884
}
1885+
// Webhook-delivery-order guard (#2479 gate finding): delivery order is not guaranteed to match PR creation
1886+
// order, so a sibling PR's own webhook can process before THIS PR exists in the DB and wrongly conclude the
1887+
// author is within the cap — nothing else would ever re-evaluate it, permanently bypassing the cap for that
1888+
// sibling. Now that THIS delivery has the complete picture, wake any OTHER still-open sibling that's also
1889+
// in the over-cap set so its own next pass re-evaluates against the complete set and self-corrects.
1890+
const otherOverCapSiblingNumbers = otherOpenPullRequests
1891+
.filter((other) => (other.authorLogin ?? "").toLowerCase() === authorLoginLower && overCapNumbers.has(other.number))
1892+
.map((other) => other.number);
1893+
if (otherOverCapSiblingNumbers.length > 0) {
1894+
await wakeOverCapSiblingPullRequests(env, deliveryId, installationId, repoFullName, otherOverCapSiblingNumbers);
1895+
}
18851896
}
18861897

18871898
const planned = planAgentMaintenanceActions({
@@ -2515,6 +2526,51 @@ async function scheduleTrailingIssueLinkedReReview(
25152526
await putTransientKey(env, key, "1", CI_COALESCE_WINDOW_SECONDS);
25162527
}
25172528

2529+
/** Best-effort wake for sibling PRs discovered to be over the per-contributor cap by a LATER delivery (#2270,
2530+
* #2479 gate finding): webhook delivery order isn't guaranteed to match PR creation order, so a sibling's own
2531+
* webhook can fire before this one exists in the DB and wrongly conclude the author is within the cap — with
2532+
* nothing else to ever re-evaluate it, that verdict would otherwise stand forever. Reuses the existing
2533+
* agent-regate-pr sweep-unit job (already rate-limit-aware and retried) — the SAME "wake and fully
2534+
* re-evaluate" entry point the linked-issue-wake feature (#2259) uses for an identical class of problem, so
2535+
* the sibling gets its own live-head/CI-freshness re-check before anything acts on it, not a shortcut based
2536+
* on this delivery's now-possibly-stale snapshot. Coalesced per sibling PR (mirrors
2537+
* scheduleTrailingIssueLinkedReReview's check-then-claim-after-success shape) so a burst of N over-cap
2538+
* siblings each discovering the same others doesn't fan out into an O(N^2) job storm. */
2539+
async function wakeOverCapSiblingPullRequests(
2540+
env: Env,
2541+
deliveryId: string,
2542+
installationId: number,
2543+
repoFullName: string,
2544+
siblingPrNumbers: number[],
2545+
): Promise<void> {
2546+
await Promise.all(
2547+
siblingPrNumbers.map(async (prNumber) => {
2548+
const key = `contributor-cap-wake:${repoFullName.toLowerCase()}#${prNumber}`;
2549+
if (await getTransientKey(env, key)) return;
2550+
try {
2551+
await env.JOBS.send({
2552+
type: "agent-regate-pr",
2553+
deliveryId,
2554+
repoFullName,
2555+
prNumber,
2556+
installationId,
2557+
});
2558+
} catch (error) {
2559+
console.log(
2560+
JSON.stringify({
2561+
ev: "contributor_cap_wake_enqueue_failed",
2562+
repoFullName,
2563+
pull: prNumber,
2564+
message: errorMessage(error).slice(0, 120),
2565+
}),
2566+
);
2567+
return; // do NOT claim — a later discovery should retry the enqueue
2568+
}
2569+
await putTransientKey(env, key, "1", CI_COALESCE_WINDOW_SECONDS);
2570+
}),
2571+
);
2572+
}
2573+
25182574
async function ciHeadShaResolutionCoalesced(
25192575
env: Env,
25202576
repoFullName: string,

test/unit/queue.test.ts

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5773,6 +5773,208 @@ describe("queue processors", () => {
57735773
expect(seen.closed).toBe(false);
57745774
});
57755775

5776+
it("contributor open-PR cap (#2270): an author-less (ghost) open PR among the repo's others is excluded from the count and the sibling-wake scan, not crashed on", async () => {
5777+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
5778+
await upsertInstallation(env, {
5779+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] },
5780+
repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }],
5781+
});
5782+
// A ghost PR with no `user` at all (authorLogin ends up null) — must not match farmer99's count, and must
5783+
// not crash the sibling-wake scan, which runs the identical (authorLogin ?? "") fallback.
5784+
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 50, title: "Ghost PR", state: "open", head: { sha: "ghost50" }, labels: [], body: "z" });
5785+
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 53, title: "Farmer PR one", state: "open", user: { login: "farmer99" }, head: { sha: "f53" }, labels: [], body: "x" });
5786+
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 54, title: "Farmer PR two", state: "open", user: { login: "farmer99" }, head: { sha: "f54" }, labels: [], body: "y" });
5787+
await upsertRepositorySettings(env, {
5788+
repoFullName: "JSONbored/gittensory",
5789+
commentMode: "all_prs",
5790+
publicSurface: "comment_only",
5791+
checkRunMode: "off",
5792+
gateCheckMode: "enabled",
5793+
aiReviewMode: "advisory",
5794+
autonomy: { close: "auto", label: "auto" },
5795+
contributorOpenPrCap: 2,
5796+
});
5797+
const seen = { closed: false };
5798+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
5799+
const url = input.toString();
5800+
const method = init?.method ?? "GET";
5801+
if (url === "https://api.gittensor.io/miners") return Response.json([]);
5802+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
5803+
if (url.includes("/pulls/55/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+const ok = true;" }]);
5804+
if (url.includes("/pulls/55/reviews")) return Response.json([]);
5805+
if (url.includes("/pulls/55/commits")) return Response.json([]);
5806+
if (url.endsWith("/pulls/55") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 55, state: "closed" }); }
5807+
if (url.endsWith("/pulls/55")) return Response.json({ number: 55, state: "open", user: { login: "farmer99" }, head: { sha: "f55" }, mergeable_state: "clean" });
5808+
if (url.includes("/commits/f55/check-runs")) return Response.json({ total_count: 0, check_runs: [] });
5809+
if (url.includes("/commits/f55/status")) return Response.json({ state: "success", statuses: [] });
5810+
if (url.includes("/issues/55/labels") && method === "GET") return Response.json([]);
5811+
if (url.includes("/issues/55/labels") && method === "POST") return Response.json([]);
5812+
if (url.includes("/issues/55/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 });
5813+
if (url.includes("/issues/55/comments")) return Response.json([]);
5814+
return Response.json({});
5815+
});
5816+
5817+
await processJob(env, {
5818+
type: "github-webhook",
5819+
deliveryId: "contributor-cap-ghost-author",
5820+
eventName: "pull_request",
5821+
payload: {
5822+
action: "opened",
5823+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } },
5824+
repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } },
5825+
pull_request: { number: 55, title: "Farmer's 3rd PR", state: "open", user: { login: "farmer99" }, head: { sha: "f55" }, labels: [], body: "x", mergeable_state: "clean", reviewDecision: "APPROVED" },
5826+
},
5827+
});
5828+
5829+
// Ghost PR's null authorLogin never matches "farmer99" — the count is still exactly 3 (farmer99's own).
5830+
expect(seen.closed).toBe(true);
5831+
});
5832+
5833+
it("contributor open-PR cap (#2270): out-of-order webhook delivery wakes and self-corrects the missed sibling (regression, gate finding on #2479)", async () => {
5834+
// PR56 (the NEWER PR) is delivered BEFORE PR55 exists in the DB — a real possibility under concurrent/
5835+
// retried webhook delivery. At that moment PR56 only sees {54, 56} (2 total, AT the cap of 2, not over),
5836+
// so it correctly stays open — but a naive "only ever check myself" implementation would leave it open
5837+
// FOREVER, since nothing else ever re-evaluates PR56 again. This pins the fix: once PR55's delivery later
5838+
// sees the COMPLETE set {54, 55, 56}, it must wake PR56 (not just decide for itself) so PR56 gets a fresh,
5839+
// fully-gated re-evaluation and self-corrects.
5840+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
5841+
await upsertInstallation(env, {
5842+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] },
5843+
repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }],
5844+
});
5845+
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 54, title: "Farmer PR zero", state: "open", user: { login: "farmer99" }, head: { sha: "f54" }, labels: [], body: "w" });
5846+
await upsertRepositorySettings(env, {
5847+
repoFullName: "JSONbored/gittensory",
5848+
commentMode: "all_prs",
5849+
publicSurface: "comment_only",
5850+
checkRunMode: "off",
5851+
gateCheckMode: "enabled",
5852+
aiReviewMode: "advisory",
5853+
autonomy: { close: "auto", label: "auto" },
5854+
contributorOpenPrCap: 2,
5855+
});
5856+
const closedNumbers = new Set<number>();
5857+
const fanned: import("../../src/types").JobMessage[] = [];
5858+
const realSend = env.JOBS.send.bind(env.JOBS);
5859+
env.JOBS.send = (async (message: import("../../src/types").JobMessage, options?: QueueSendOptions) => {
5860+
if (message.type === "agent-regate-pr") fanned.push(message);
5861+
return realSend(message, options);
5862+
}) as typeof env.JOBS.send;
5863+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
5864+
const url = input.toString();
5865+
const method = init?.method ?? "GET";
5866+
if (url === "https://api.gittensor.io/miners") return Response.json([]);
5867+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
5868+
for (const [n, sha] of [[55, "f55"], [56, "f56"]] as const) {
5869+
if (url.includes(`/pulls/${n}/files`)) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+const ok = true;" }]);
5870+
if (url.includes(`/pulls/${n}/reviews`)) return Response.json([]);
5871+
if (url.includes(`/pulls/${n}/commits`)) return Response.json([]);
5872+
if (url.endsWith(`/pulls/${n}`) && method === "PATCH") { closedNumbers.add(n); return Response.json({ number: n, state: "closed" }); }
5873+
if (url.endsWith(`/pulls/${n}`)) return Response.json({ number: n, state: closedNumbers.has(n) ? "closed" : "open", user: { login: "farmer99" }, head: { sha }, mergeable_state: "clean" });
5874+
if (url.includes(`/commits/${sha}/check-runs`)) return Response.json({ total_count: 0, check_runs: [] });
5875+
if (url.includes(`/commits/${sha}/status`)) return Response.json({ state: "success", statuses: [] });
5876+
if (url.includes(`/issues/${n}/labels`) && method === "GET") return Response.json([]);
5877+
if (url.includes(`/issues/${n}/labels`) && method === "POST") return Response.json([]);
5878+
if (url.includes(`/issues/${n}/comments`) && method === "POST") return Response.json({ id: 1 }, { status: 201 });
5879+
if (url.includes(`/issues/${n}/comments`)) return Response.json([]);
5880+
}
5881+
return Response.json({});
5882+
});
5883+
5884+
// PR56 arrives FIRST — PR55 does not exist yet, so PR56 sees only {54, 56}: at the cap, not over.
5885+
await processJob(env, {
5886+
type: "github-webhook",
5887+
deliveryId: "burst-pr56-first",
5888+
eventName: "pull_request",
5889+
payload: {
5890+
action: "opened",
5891+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } },
5892+
repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } },
5893+
pull_request: { number: 56, title: "Farmer PR two (out of order)", state: "open", user: { login: "farmer99" }, head: { sha: "f56" }, labels: [], body: "y", mergeable_state: "clean", reviewDecision: "APPROVED" },
5894+
},
5895+
});
5896+
expect(closedNumbers.has(56)).toBe(false); // correctly not closed YET — the set looked complete at the time
5897+
5898+
// PR55 arrives SECOND — now the complete set {54, 55, 56} is visible. PR55 itself ranks within the cap
5899+
// (oldest 2 of 3), so it stays open — but PR56 is now discoverably over-cap and must be woken.
5900+
await processJob(env, {
5901+
type: "github-webhook",
5902+
deliveryId: "burst-pr55-second",
5903+
eventName: "pull_request",
5904+
payload: {
5905+
action: "opened",
5906+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } },
5907+
repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } },
5908+
pull_request: { number: 55, title: "Farmer PR one", state: "open", user: { login: "farmer99" }, head: { sha: "f55" }, labels: [], body: "x", mergeable_state: "clean", reviewDecision: "APPROVED" },
5909+
},
5910+
});
5911+
expect(closedNumbers.has(55)).toBe(false); // PR55 itself is within the cap
5912+
expect(fanned.some((job) => job.type === "agent-regate-pr" && job.prNumber === 56)).toBe(true); // sibling woken
5913+
5914+
// Drain the woken job — PR56's OWN fresh re-evaluation now sees the complete set and self-corrects.
5915+
env.JOBS.send = realSend;
5916+
for (const job of fanned) await processJob(env, job);
5917+
expect(closedNumbers.has(56)).toBe(true);
5918+
});
5919+
5920+
it("contributor open-PR cap (#2270): a re-delivered sibling-wake is coalesced — the second discovery does not re-enqueue", async () => {
5921+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
5922+
await upsertInstallation(env, {
5923+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] },
5924+
repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }],
5925+
});
5926+
// Pre-seed the coalescing key for PR56 exactly as wakeOverCapSiblingPullRequests itself would after a
5927+
// first, already-successful enqueue — proving the SECOND discovery within the window skips re-enqueueing.
5928+
await env.SELFHOST_TRANSIENT_CACHE?.set("contributor-cap-wake:jsonbored/gittensory#56", "1", 60);
5929+
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 54, title: "Farmer PR zero", state: "open", user: { login: "farmer99" }, head: { sha: "f54" }, labels: [], body: "w" });
5930+
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 56, title: "Farmer PR two (already over cap)", state: "open", user: { login: "farmer99" }, head: { sha: "f56" }, labels: [], body: "y" });
5931+
await upsertRepositorySettings(env, {
5932+
repoFullName: "JSONbored/gittensory",
5933+
commentMode: "all_prs",
5934+
publicSurface: "comment_only",
5935+
checkRunMode: "off",
5936+
gateCheckMode: "enabled",
5937+
aiReviewMode: "advisory",
5938+
autonomy: { close: "auto", label: "auto" },
5939+
contributorOpenPrCap: 2,
5940+
});
5941+
const fanned: import("../../src/types").JobMessage[] = [];
5942+
const realSend = env.JOBS.send.bind(env.JOBS);
5943+
env.JOBS.send = (async (message: import("../../src/types").JobMessage, options?: QueueSendOptions) => {
5944+
if (message.type === "agent-regate-pr") fanned.push(message);
5945+
return realSend(message, options);
5946+
}) as typeof env.JOBS.send;
5947+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
5948+
const url = input.toString();
5949+
if (url === "https://api.gittensor.io/miners") return Response.json([]);
5950+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
5951+
if (url.includes("/pulls/55/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+const ok = true;" }]);
5952+
if (url.includes("/pulls/55/reviews")) return Response.json([]);
5953+
if (url.includes("/pulls/55/commits")) return Response.json([]);
5954+
if (url.endsWith("/pulls/55")) return Response.json({ number: 55, state: "open", user: { login: "farmer99" }, head: { sha: "f55" }, mergeable_state: "clean" });
5955+
if (url.includes("/commits/f55/check-runs")) return Response.json({ total_count: 0, check_runs: [] });
5956+
if (url.includes("/commits/f55/status")) return Response.json({ state: "success", statuses: [] });
5957+
if (url.includes("/issues/55/labels")) return Response.json([]);
5958+
if (url.includes("/issues/55/comments")) return Response.json([]);
5959+
return Response.json({});
5960+
});
5961+
5962+
// PR55 arrives and independently discovers PR56 is over cap — but the wake was already claimed.
5963+
await processJob(env, {
5964+
type: "github-webhook",
5965+
deliveryId: "wake-coalesce-second",
5966+
eventName: "pull_request",
5967+
payload: {
5968+
action: "opened",
5969+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } },
5970+
repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } },
5971+
pull_request: { number: 55, title: "Farmer PR one", state: "open", user: { login: "farmer99" }, head: { sha: "f55" }, labels: [], body: "x", mergeable_state: "clean", reviewDecision: "APPROVED" },
5972+
},
5973+
});
5974+
5975+
expect(fanned).toEqual([]); // coalesced — no duplicate wake enqueued
5976+
});
5977+
57765978
// #1092: prReadyForReview rebases a BEHIND-base PR through the agent executor (gated by update_branch autonomy
57775979
// + pull_requests:write) before reviewing, then defers — the synchronize on the new head re-runs review.
57785980
async function seedBehindRepo(env: Env, over: { autonomy?: Record<string, string>; agentPaused?: boolean; perms?: Record<string, string>; noInstall?: boolean } = {}) {

0 commit comments

Comments
 (0)