Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 17 additions & 3 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1677,6 +1683,7 @@ async function maybeRunAgentMaintenance(
changedPaths,
hardGuardrailGlobs,
authorIsOwner,
authorIsAdmin,
authorIsAutomationBot,
closeOwnerAuthors: settings.closeOwnerAuthors,
ciState: ciAggregate.ciState,
Expand Down Expand Up @@ -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
Expand Down
30 changes: 20 additions & 10 deletions src/settings/agent-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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" });
Expand Down Expand Up @@ -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
Expand Down
34 changes: 30 additions & 4 deletions test/unit/agent-actions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ function input(overrides: Partial<AgentActionPlanInput> & { conclusion: GateChec
changedPaths: [],
hardGuardrailGlobs: [],
authorIsOwner: false,
authorIsAdmin: false,
authorIsAutomationBot: false,
ciState: "passed",
pr: { labels: [] },
Expand Down Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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 } })));
Expand Down Expand Up @@ -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");
});

Expand Down
19 changes: 19 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
Loading