diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 7f32dfc58b..23245a77b5 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -1635,6 +1635,12 @@ async function maybeRunAgentMaintenance( const authorIsOwner = authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase(); + // Fleet-operator identity (#2133): the same ADMIN_GITHUB_LOGINS allowlist already honored by the + // reopen-reclose path's hasMaintainerPermission, folded into the primary close-eligibility computation so an + // admin login (not the literal repo owner) gets the identical never-auto-closed exemption everywhere. + const authorIsAdmin = + authorLogin.length > 0 && + parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(authorLogin.toLowerCase()); const authorIsAutomationBot = isProtectedAutomationAuthor(pr.authorLogin); // Linked-issue HARD-RULE close (#linked-issue-hard-rules): when the repo enabled any rule, a body that links @@ -1677,6 +1683,7 @@ async function maybeRunAgentMaintenance( changedPaths, hardGuardrailGlobs, authorIsOwner, + authorIsAdmin, authorIsAutomationBot, closeOwnerAuthors: settings.closeOwnerAuthors, ciState: ciAggregate.ciState, @@ -3360,14 +3367,21 @@ async function processGitHubWebhook( const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")).toLowerCase() : ""; + const draftDodgeAuthorLogin = (pr.authorLogin ?? "").toLowerCase(); const authorIsOwner = - (pr.authorLogin ?? "").toLowerCase() === repoOwner && - repoOwner.length > 0; + draftDodgeAuthorLogin === repoOwner && repoOwner.length > 0; + // Fleet-operator identity (#2133): same ADMIN_GITHUB_LOGINS exemption as the primary close-eligibility + // computation above and hasMaintainerPermission below — an admin login must never be auto-closed here + // either, matching every other actuation path's trusted-operator definition. + const authorIsAdmin = + draftDodgeAuthorLogin.length > 0 && + parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(draftDodgeAuthorLogin); if ( block && block.headSha === pr.headSha && !block.overridden && - !authorIsOwner + !authorIsOwner && + !authorIsAdmin ) { // Respect the agent action mode (#killswitch-gap): the outer guard already excludes a per-repo pause, // but this close path must also honor the global freeze and dry-run — so a freeze is a COMPLETE stop diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 93dfecb636..40e92f3a73 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -86,13 +86,20 @@ export type AgentActionPlanInput = { // True when the PR author is the repo owner (e.g. JSONbored). Standing rule: owner PRs are NEVER // auto-closed. They may still auto-merge when clean + passing. authorIsOwner: boolean; + // True when the PR author is a fleet-operator login (env ADMIN_GITHUB_LOGINS) that is NOT the literal repo + // owner (#2133). This is the same trusted-operator identity already honored by the reopen-reclose path's + // hasMaintainerPermission — folded in here so it isn't a second, drifting definition of "maintainer". Treated + // identically to authorIsOwner throughout this planner (never auto-closed by default; auto-close only when + // closeOwnerAuthors is on). + authorIsAdmin: boolean; // True when the PR author is a maintainer-managed automation account (e.g. github-actions[bot] opening an // accumulator like automation/readme-refresh, or dependabot/renovate). These are NEVER auto-closed — a noise // heuristic (duplicate/slop) must not kill a recurring maintainer-managed PR. They may still auto-merge. authorIsAutomationBot: boolean; - // Per-repo toggle (#configurable-owner-close): when TRUE, the repo OWNER's own PRs are eligible for auto-close - // like a contributor's (still gated by the `close` autonomy class + adverse-signal conditions). Default/undefined - // ⇒ owner PRs are exempt (merge or manual-hold only). Automation-bot PRs stay exempt regardless. + // Per-repo toggle (#configurable-owner-close): when TRUE, the repo OWNER's own PRs (and admin-authored PRs, + // #2133) are eligible for auto-close like a contributor's (still gated by the `close` autonomy class + + // adverse-signal conditions). Default/undefined ⇒ owner/admin PRs are exempt (merge or manual-hold only). + // Automation-bot PRs stay exempt regardless. closeOwnerAuthors?: boolean | undefined; // Live CI aggregate over ALL of the PR's checks — required OR not, including non-required ones like // codecov/patch and every commit-status (reviewbot parity). "passed" = every check completed and none @@ -256,12 +263,13 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // Contributor blacklist (#1425): a banned author's PR is a DETERMINISTIC short-circuit — it SHORT-CIRCUITS to a // label + close AHEAD of all merit/CI/gate/AI analysis (this returns before any of it), so a blocked account is - // never merit-reviewed or auto-merged. Fires for a CONTRIBUTOR only (owner/automation bots are NEVER auto-closed, - // the standing rule). Zero-hallucination, so its close is `closeKind: "blacklist"`, separate from heuristic + // never merit-reviewed or auto-merged. Fires for a CONTRIBUTOR only (owner/admin/automation bots are NEVER + // auto-closed, the standing rule — #2133 folds the fleet-operator admin allowlist into the same exemption). + // Zero-hallucination, so its close is `closeKind: "blacklist"`, separate from heuristic // closes. The `acting`/`approval` gates here + the executor's pause/dry-run/ // kill-switch gate make it dry-run-able and approval-gated exactly like every other action. The close comment is // static by construction so private maintainer metadata from the blacklist entry cannot leak. - const blacklistContributor = !input.authorIsOwner && !input.authorIsAutomationBot; + const blacklistContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot; if (input.blacklistMatch?.matched === true && blacklistContributor) { const label = input.blacklistLabel ?? DEFAULT_BLACKLIST_LABEL; if (acting("label")) actions.push({ actionClass: "label", requiresApproval: approval("label"), reason: "blacklisted contributor", label, labelOp: "add" }); @@ -312,12 +320,14 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // would-approve/would-merge dispositions into a manual hold. const ciUnverified = input.ciState === "unverified"; const reviewGood = gatePassing && ciPassed; - const isContributor = !input.authorIsOwner && !input.authorIsAutomationBot; + const isContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot; // The owner-close exemption is PER-REPO CONFIGURABLE (#configurable-owner-close): by default the repo owner's // own PRs are exempt from auto-close (closeOwnerAuthors !== true ⇒ merge or manual-hold only), but a maintainer - // can opt in to closing them like a contributor's. Automation bots stay exempt regardless (a noise heuristic - // must not kill a recurring maintainer-managed accumulator). - const closeEligible = isContributor || (input.authorIsOwner && input.closeOwnerAuthors === true); + // can opt in to closing them like a contributor's. #2133 folds the fleet-operator admin allowlist into the same + // trusted-identity exemption (a login honored as a maintainer everywhere else in the codebase must not be + // treated as an ordinary contributor here). Automation bots stay exempt regardless (a noise heuristic must not + // kill a recurring maintainer-managed accumulator). + const closeEligible = isContributor || ((input.authorIsOwner || input.authorIsAdmin) && input.closeOwnerAuthors === true); const mergeableClean = input.pr.mergeableState === "clean"; const isConflict = input.pr.mergeableState === "dirty"; // conflicts with base — can't merge as-is // RC3: a prior merge attempt failed terminally for THIS exact head SHA (403/405/409/conflict) → never re-plan diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 50bcb42369..08fbfa7668 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -12,6 +12,7 @@ function input(overrides: Partial & { conclusion: GateChec changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, + authorIsAdmin: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [] }, @@ -135,12 +136,12 @@ describe("planAgentMaintenanceActions (#778)", () => { it("applies conservative defaults when autoMaintain / slopGateMinScore are omitted", () => { // no autoMaintain → requireApprovals defaults to 1 → a clean passing PR without APPROVED does NOT merge - expect(classes(planAgentMaintenanceActions({ conclusion: "success", blockerTitles: [], autonomy: { merge: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], mergeableState: "clean" } }))).not.toContain("merge"); + expect(classes(planAgentMaintenanceActions({ conclusion: "success", blockerTitles: [], autonomy: { merge: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAdmin: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], mergeableState: "clean" } }))).not.toContain("merge"); // no slopGateMinScore → defaults to 60 → slopRisk 70 counts as noise and closes - expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], slopRisk: 70 } }))).toContain("close"); + expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAdmin: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], slopRisk: 70 } }))).toContain("close"); // ...and slopRisk 50 (below the slop default) STILL closes — a failing-gate contributor PR is closed one-shot // regardless of slop; the slop score only adds a close reason (minimize-manual: merge-or-close). - expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], slopRisk: 50 } }))).toContain("close"); + expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAdmin: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], slopRisk: 50 } }))).toContain("close"); }); it("closes any non-passing contributor PR (citing noise when present), and never closes a passing PR", () => { @@ -371,6 +372,28 @@ describe("planAgentMaintenanceActions (#778)", () => { }); }); + describe("admin-login guard: ADMIN_GITHUB_LOGINS gets the same never-auto-close exemption as the owner (#2133)", () => { + it("does NOT auto-close a noisy failing PR authored by a fleet-operator admin login", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], authorIsAdmin: true, pr: { labels: [], slopRisk: 95 } }))); + expect(plan).not.toContain("close"); + }); + + it("still auto-merges a clean+approved admin-authored PR (the guard blocks only close, never merge)", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, authorIsAdmin: true, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(plan).toContain("merge"); + }); + + it("DOES auto-close a failing admin PR when closeOwnerAuthors is enabled (the same per-repo opt-in covers admins)", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], authorIsAdmin: true, closeOwnerAuthors: true, ciState: "passed", pr: { labels: [], slopRisk: 95 } }))); + expect(plan).toContain("close"); + }); + + it("does NOT auto-close a red-CI PR authored by an admin login (mirrors the CI-policy owner exemption)", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { close: "auto", request_changes: "auto", label: "auto" }, ciState: "failed", failingCheckNames: ["codecov/patch"], authorIsAdmin: true, pr: { labels: [] } }))); + expect(plan).not.toContain("close"); + }); + }); + describe("automation-bot guard: never auto-close maintainer-managed accumulator/dependency PRs", () => { it("does NOT auto-close a noisy failing PR authored by an automation bot (e.g. the readme-refresh accumulator)", () => { const plan = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], authorIsAutomationBot: true, pr: { labels: [], slopRisk: 95, linkedDuplicateCount: 3 } }))); @@ -714,8 +737,11 @@ describe("contributor blacklist short-circuit (#1425)", () => { expect(classes(planAgentMaintenanceActions(blacklisted({ ciState: "pending" })))).toEqual(["label", "close"]); }); - it("NEVER fires for the owner or an automation bot (standing rule) — the PR falls through to normal disposition", () => { + it("NEVER fires for the owner, an admin login, or an automation bot (standing rule) — the PR falls through to normal disposition", () => { expect(classes(planAgentMaintenanceActions(blacklisted({ authorIsOwner: true })))).not.toContain("close"); + // #2133: a fleet-operator ADMIN_GITHUB_LOGINS author gets the identical exemption — the blacklist + // short-circuit must not treat a trusted admin as an ordinary contributor. + expect(classes(planAgentMaintenanceActions(blacklisted({ authorIsAdmin: true })))).not.toContain("close"); expect(classes(planAgentMaintenanceActions(blacklisted({ authorIsAutomationBot: true })))).not.toContain("close"); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index ec69054c93..e564ef5371 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -9573,6 +9573,25 @@ describe("converted_to_draft gate-close (draft-dodge prevention)", () => { expect(calls.some((c) => c.includes("PATCH") && c.includes("/pulls/42"))).toBe(false); }); + it("no-ops when the PR author is an ADMIN_GITHUB_LOGINS fleet-operator, not just the literal repo owner (#2133)", async () => { + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push(`${init?.method ?? "GET"} ${url}`); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory", ADMIN_GITHUB_LOGINS: "admin-user" }); + await setupRepo(env); + await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + + // Author = "admin-user" ≠ repo owner "JSONbored", but IS in ADMIN_GITHUB_LOGINS → no close. + await processJob(env, { type: "github-webhook", deliveryId: "draft-admin", eventName: "pull_request", payload: draftPayload("admin-user") }); + + expect(calls.some((c) => c.includes("PATCH") && c.includes("/pulls/42"))).toBe(false); + }); + it("no-ops when the agent is paused (agentPaused=true)", async () => { const calls: string[] = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {