Skip to content

Commit 15ceaa8

Browse files
committed
fix(agent-actions): close every over-cap sibling issue, not just the incoming one (#2493)
Same delivery-order gap as #2479, mirrored for issues: closing only "the incoming issue, if it's over cap" let an older sibling's stale verdict (computed before a newer sibling existed in the DB) stand forever, since nothing else ever re-evaluates an issue. Now closes every number in the over-cap set discovered by the current delivery. Unlike the PR path, issues have no live-head/CI staleness risk to guard against and no issue-side "regate" job type to reuse, so acting directly on the already-fetched snapshot is safe. Also fixes the codecov/patch gap the gate flagged on this PR: adds coverage for the label ?? "" fallback, the no-slash repoFullName guard, and an author-less (ghost) sibling issue.
1 parent 624a896 commit 15ceaa8

3 files changed

Lines changed: 110 additions & 12 deletions

File tree

src/queue/processors.ts

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3554,12 +3554,23 @@ async function loadOpenQueueCounts(
35543554
/**
35553555
* Per-contributor open-ISSUE cap (#2270, anti-abuse): the first `eventName === "issues"` actuation branch —
35563556
* issues have no other auto-close path today. Mirrors the PR-path cap in runAgentMaintenancePlanAndExecute:
3557-
* counts the author's OTHER currently-open issues on this repo plus this one, ranked by issue NUMBER (GitHub's
3558-
* own creation order, not webhook-arrival order), and only matches when THIS issue's number is among the ones
3559-
* over the cap. Reuses planAgentMaintenanceActions to build the SAME label+close plan the PR path uses
3560-
* (identical closeKind/label/close-comment construction): passing `conclusion: "skipped"` and no
3561-
* `blacklistMatch` guarantees the function returns at the contributor_cap short-circuit (the very next check in
3562-
* the planner) before ever touching any PR/CI-specific field, so building a plan for an issue this way is safe.
3557+
* counts the author's currently-open issues on this repo (including this one), ranked by issue NUMBER
3558+
* (GitHub's own creation order, not webhook-arrival order). Reuses planAgentMaintenanceActions to build the
3559+
* SAME label+close plan the PR path uses (identical closeKind/label/close-comment construction): passing
3560+
* `conclusion: "skipped"` and no `blacklistMatch` guarantees the function returns at the contributor_cap
3561+
* short-circuit (the very next check in the planner) before ever touching any PR/CI-specific field, so
3562+
* building a plan for an issue this way is safe.
3563+
*
3564+
* Webhook-delivery-order guard (#2479 gate finding, mirrored here for issues): delivery order is not
3565+
* guaranteed to match issue creation order, so an OLDER sibling's own webhook can process before a NEWER
3566+
* sibling exists in the DB and wrongly conclude the author is within the cap — closing only "the incoming
3567+
* issue, if it's over cap" would let that stale verdict stand forever, since nothing else ever re-evaluates
3568+
* it. Closes EVERY number in the over-cap set discovered by THIS delivery, not just the incoming issue, so
3569+
* whichever delivery happens to see the complete picture corrects any sibling a prior delivery missed. Unlike
3570+
* the PR path (which enqueues a `agent-regate-pr` wake job so each sibling gets its own live-head/CI-freshness
3571+
* re-check before acting), issues have no such staleness risk to guard against — a plain issue has no head SHA
3572+
* or CI to go stale, and there's no issue-side "regate" job type to reuse — so acting directly on the
3573+
* already-fetched snapshot here is safe.
35633574
*/
35643575
async function maybeCloseIssueOverContributorCap(
35653576
env: Env,
@@ -3584,7 +3595,7 @@ async function maybeCloseIssueOverContributorCap(
35843595
.concat(issue.number)
35853596
.sort((a, b) => a - b);
35863597
const overCapNumbers = new Set(authorOpenIssueNumbers.slice(cap));
3587-
if (!overCapNumbers.has(issue.number)) return;
3598+
if (overCapNumbers.size === 0) return;
35883599

35893600
const planned = planAgentMaintenanceActions({
35903601
conclusion: "skipped",
@@ -3602,11 +3613,13 @@ async function maybeCloseIssueOverContributorCap(
36023613
});
36033614
if (planned.length === 0) return;
36043615

3605-
await executeIssueMaintenanceActions(
3606-
env,
3607-
{ installationId, repoFullName, issueNumber: issue.number, autonomy: settings.autonomy, agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun },
3608-
planned,
3609-
);
3616+
for (const overCapNumber of overCapNumbers) {
3617+
await executeIssueMaintenanceActions(
3618+
env,
3619+
{ installationId, repoFullName, issueNumber: overCapNumber, autonomy: settings.autonomy, agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun },
3620+
planned,
3621+
);
3622+
}
36103623
}
36113624

36123625
async function processGitHubWebhook(

test/unit/agent-action-executor.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,13 @@ describe("executeIssueMaintenanceActions (#2270 issue-side actuation)", () => {
578578
expect(closeIssue).toHaveBeenCalledTimes(1);
579579
});
580580

581+
it("a label action with no label name falls back to an empty string (defensive — the contributor_cap planner always sets one)", async () => {
582+
const env = createTestEnv({});
583+
const { label: _label, ...labelWithoutName } = issueLabel;
584+
await executeIssueMaintenanceActions(env, issueCtx(), [labelWithoutName]);
585+
expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 42, "", { createMissingLabel: true });
586+
});
587+
581588
it("PAUSED (per-repo): mutates nothing and audits denied", async () => {
582589
const env = createTestEnv({});
583590
const outcomes = await executeIssueMaintenanceActions(env, issueCtx({ agentPaused: true }), [issueLabel, issueClose]);

test/unit/queue.test.ts

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6160,6 +6160,84 @@ describe("queue processors", () => {
61606160
expect(closeAudit?.n ?? 0).toBe(0);
61616161
});
61626162

6163+
it("contributor open-ISSUE cap (#2270): a slash-free repoFullName is safely planned (repoOwner computation guard) even though the GitHub call itself can never succeed against that name", async () => {
6164+
// A real webhook always carries "owner/repo"; this pins the DEFENSIVE repoFullName.includes("/") ? ... : ""
6165+
// fallback (mirroring the PR path's own such guard) against a malformed value WITHOUT crashing the cap
6166+
// computation. The actual close attempt legitimately errors — splitRepo() (shared by every GitHub-action
6167+
// primitive) rejects any repoFullName that isn't "owner/repo" — and that error is caught and audited, not
6168+
// thrown into the webhook handler; a successful close against a slash-free name is not physically possible
6169+
// via the real GitHub REST API, so asserting an audited error (not a crash) is the correct expectation.
6170+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
6171+
await upsertRepositoryFromGitHub(env, { name: "noslash", full_name: "noslash", private: false, owner: { login: "" } }, 123);
6172+
await upsertInstallation(env, {
6173+
installation: { id: 123, account: { login: "", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", issues: "write" }, events: ["issues"] },
6174+
repositories: [{ name: "noslash", full_name: "noslash", private: false, owner: { login: "" } }],
6175+
});
6176+
await upsertIssueFromGitHub(env, "noslash", { number: 60, title: "Farmer issue one", state: "open", user: { login: "farmer99" }, labels: [], body: "x" });
6177+
await upsertIssueFromGitHub(env, "noslash", { number: 61, title: "Farmer issue two", state: "open", user: { login: "farmer99" }, labels: [], body: "y" });
6178+
await upsertRepositorySettings(env, { repoFullName: "noslash", autonomy: { close: "auto", label: "auto" }, contributorOpenIssueCap: 2 });
6179+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
6180+
const url = input.toString();
6181+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
6182+
return Response.json({});
6183+
});
6184+
6185+
await expect(
6186+
processJob(env, {
6187+
type: "github-webhook",
6188+
deliveryId: "contributor-issue-cap-noslash",
6189+
eventName: "issues",
6190+
payload: {
6191+
action: "opened",
6192+
installation: { id: 123, account: { login: "", id: 1, type: "User" } },
6193+
repository: { name: "noslash", full_name: "noslash", private: false, owner: { login: "" } },
6194+
issue: { number: 62, title: "Farmer's 3rd issue", state: "open", user: { login: "farmer99" }, labels: [], body: "x" },
6195+
},
6196+
}),
6197+
).resolves.not.toThrow();
6198+
6199+
const closeAudit = await env.DB.prepare("select outcome, detail from audit_events where event_type = 'agent.action.close' order by created_at desc limit 1").first<{ outcome: string; detail: string }>();
6200+
expect(closeAudit?.outcome).toBe("error");
6201+
expect(closeAudit?.detail).toMatch(/Invalid repository full name/);
6202+
});
6203+
6204+
it("contributor open-ISSUE cap (#2270): an author-less (ghost) open issue among the repo's others is excluded from the count, not crashed on", async () => {
6205+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
6206+
await upsertInstallation(env, {
6207+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", issues: "write" }, events: ["issues"] },
6208+
repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }],
6209+
});
6210+
// A ghost issue with no `user` at all (authorLogin ends up null) — must not match farmer99's count nor throw.
6211+
await upsertIssueFromGitHub(env, "JSONbored/gittensory", { number: 59, title: "Ghost issue", state: "open", labels: [], body: "z" });
6212+
await upsertIssueFromGitHub(env, "JSONbored/gittensory", { number: 60, title: "Farmer issue one", state: "open", user: { login: "farmer99" }, labels: [], body: "x" });
6213+
await upsertIssueFromGitHub(env, "JSONbored/gittensory", { number: 61, title: "Farmer issue two", state: "open", user: { login: "farmer99" }, labels: [], body: "y" });
6214+
await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" }, contributorOpenIssueCap: 2 });
6215+
const seen = { closed: false };
6216+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
6217+
const url = input.toString();
6218+
const method = init?.method ?? "GET";
6219+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
6220+
if (url.endsWith("/issues/62") && method === "PATCH") { seen.closed = true; return Response.json({ state: "closed" }); }
6221+
return Response.json({});
6222+
});
6223+
6224+
await processJob(env, {
6225+
type: "github-webhook",
6226+
deliveryId: "contributor-issue-cap-ghost-author",
6227+
eventName: "issues",
6228+
payload: {
6229+
action: "opened",
6230+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } },
6231+
repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } },
6232+
issue: { number: 62, title: "Farmer's 3rd issue", state: "open", user: { login: "farmer99" }, labels: [], body: "x" },
6233+
},
6234+
});
6235+
6236+
// Ghost issue's null authorLogin never matches "farmer99" — the count is still exactly 3 (farmer99's own),
6237+
// so the cap-of-2 close fires; a broken nullish fallback would either crash or double-count the ghost.
6238+
expect(seen.closed).toBe(true);
6239+
});
6240+
61636241
// #1092: prReadyForReview rebases a BEHIND-base PR through the agent executor (gated by update_branch autonomy
61646242
// + pull_requests:write) before reviewing, then defers — the synchronize on the new head re-runs review.
61656243
async function seedBehindRepo(env: Env, over: { autonomy?: Record<string, string>; agentPaused?: boolean; perms?: Record<string, string>; noInstall?: boolean } = {}) {

0 commit comments

Comments
 (0)