diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3bc8dc42a4..483831fd1e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3138,9 +3138,9 @@ async function runAgentMaintenancePlanAndExecute( moderationBannedLabel: settings.moderationBannedLabel, }, // #selfhost-ci-verification: the executor's own final pre-mutation live-CI re-check (immediately before a - // merge or a CI-driven close) must honor the same configured expectedCiContexts this plan was evaluated - // against, or the two can disagree on ciState. - expectedCiContexts: settings.expectedCiContexts, + // merge or a CI-driven close) must honor the same effective branch-protection-plus-expected contexts this + // plan was evaluated against, or the two can disagree on ciState. + requiredCiContexts: requiredContexts, // #3472 split-brain: the executor's own live manual-review hold guard (immediately before approve/merge) // must check the SAME configured label the planner itself resolves labels.manualReview from. manualReviewLabel: settings.manualReviewLabel, diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 338fc5a37c..f1330080d7 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -16,7 +16,7 @@ import { isAuthorBlacklisted } from "../settings/contributor-blacklist"; import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; import { cancelInFlightWorkflowRunsForHeadSha, createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, mergeRequiredCiContexts, refreshInstallationHealthForInstallation } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, refreshInstallationHealthForInstallation } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { ensurePullRequestAssignee } from "../github/assignees"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; @@ -168,16 +168,13 @@ export type AgentActionExecutionContext = { // executor via getGlobalModerationConfig -- a single extra DB read only on the rare path where a // moderation-tracked close actually completed, not threaded through every caller. moderationSettings?: ModerationContextSettings | undefined; - // settings.expectedCiContexts (#selfhost-ci-verification), resolved by the CALLER (same "the executor has no - // settings access" shape as the fields above): the final pre-mutation live-CI re-verification (step 8 below) - // must honor the SAME configured-required-contexts view the planning pass already evaluated against, or a - // maintainer-configured expectedCiContexts repo could see the plan and its own execution-time re-check - // disagree on ciState (e.g. a still-in-progress NON-required check reading "pending" here when the plan's - // required-only view was already clean). Absent/undefined ⇒ fold-all mode, unchanged from before this field - // existed. - expectedCiContexts?: ReadonlyArray | null | undefined; + // Effective required CI contexts (#selfhost-ci-verification), resolved by the CALLER (same "the executor has + // no settings access" shape as the fields above): the final pre-mutation live-CI re-verification (step 8 below) + // must honor the SAME branch-protection-plus-expected required-contexts view the planning pass already + // evaluated against. Absent/undefined ⇒ fold-all mode, unchanged from before this field existed. + requiredCiContexts?: ReadonlySet | null | undefined; // settings.manualReviewLabel (#3472 split-brain), resolved by the CALLER (same "the executor has no settings - // access" shape as expectedCiContexts above): the approve/merge live label guard (step 7b below) needs the + // access" shape as requiredCiContexts above): the approve/merge live label guard (step 7b below) needs the // SAME configured label name the planner itself resolves labels.manualReview from (agent-actions.ts), so a // custom label name is honored instead of only ever checking the literal default. `null` explicitly disables // the manual-review label (and this guard with it); absent/undefined uses the default AGENT_LABEL_NEEDS_REVIEW. @@ -401,12 +398,9 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE if (requiresLiveCiRecheck || requiresLiveMergeableRecheck || requiresLiveThreadRecheck || requiresLiveDuplicateRecheck) { const ciToken = await createInstallationToken(env, ctx.installationId).catch(() => undefined); const admissionKey = githubRateLimitAdmissionKeyForToken(env, ciToken, ctx.installationId); - // mergeRequiredCiContexts(null, ...) -- no live branch-protection re-fetch here, just the maintainer's own - // configured expectedCiContexts (or null/fold-all when unset), matching the "no branch protection" arm of - // the planning pass's own merge (mergeRequiredCiContexts is pure and already exported for that call site). const [liveCi, liveMergeableState, liveThreadBlockers, liveWinnerState] = await Promise.all([ requiresLiveCiRecheck - ? fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, mergeRequiredCiContexts(null, ctx.expectedCiContexts), admissionKey) + ? fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, ctx.requiredCiContexts ?? null, admissionKey) : Promise.resolve(undefined), requiresLiveMergeableRecheck ? fetchLivePullRequestMergeState(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined), requiresLiveThreadRecheck ? fetchLiveReviewThreadBlockers(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined), diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 0fb52bbb07..e87b5a8f16 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -6,7 +6,7 @@ import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent- import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; import { findBlacklistEntry } from "../settings/contributor-blacklist"; import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, mergeRequiredCiContexts } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts, mergeRequiredCiContexts } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; @@ -228,13 +228,13 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // The CI/mergeable/review calls are no-ops (Promise.resolve(undefined)) when shouldRecheckLiveDisposition is // false (block entered ONLY for a duplicate-only recheck); the thread/duplicate calls are independently // gated on their own specific flags, mirroring the executor's own same-pattern conditional-Promise.all in - // agent-action-executor.ts. + // agent-action-executor.ts. The branch-protection fetch is nested inside this same ternary (rather than a + // standalone variable) because pr is only known-defined when shouldRecheckLiveDisposition is true. const [ciResult, mergeableResult, reviewResult, threadResult, duplicateWinnerResult] = await Promise.allSettled([ shouldRecheckLiveDisposition - ? // mergeRequiredCiContexts(null, ...) -- no live branch-protection re-fetch here, just the maintainer's own - // configured expectedCiContexts (or null/fold-all when unset), so this accept-time re-check honors the - // same required-contexts view the original plan was evaluated against (#selfhost-ci-verification). - fetchLiveCiAggregate(env, pending.repoFullName, pr!.headSha, token, mergeRequiredCiContexts(null, settings.expectedCiContexts), admissionKey) + ? fetchRequiredStatusContexts(env, pending.repoFullName, pr!.baseRef, token, admissionKey) + .then((branchProtectionContexts) => mergeRequiredCiContexts(branchProtectionContexts, settings.expectedCiContexts)) + .then((requiredContexts) => fetchLiveCiAggregate(env, pending.repoFullName, pr!.headSha, token, requiredContexts, admissionKey)) : Promise.resolve(undefined), shouldRecheckLiveDisposition ? fetchLivePullRequestMergeState(env, pending.repoFullName, pending.pullNumber, token, admissionKey) : Promise.resolve(undefined), shouldRecheckLiveDisposition ? fetchLivePullRequestReviewDecision(env, pending.repoFullName, pending.pullNumber, token, admissionKey) : Promise.resolve(undefined), @@ -390,6 +390,12 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de } } + const executionCiToken = await createInstallationToken(env, pending.installationId).catch(() => undefined); + const executionAdmissionKey = githubRateLimitAdmissionKeyForToken(env, executionCiToken, pending.installationId); + const executionRequiredContexts = await fetchRequiredStatusContexts(env, pending.repoFullName, pr?.baseRef, executionCiToken, executionAdmissionKey) + .then((branchProtectionContexts) => mergeRequiredCiContexts(branchProtectionContexts, settings.expectedCiContexts)) + .catch(() => mergeRequiredCiContexts(null, settings.expectedCiContexts)); + const outcomes = await executeAgentMaintenanceActions( env, { @@ -406,9 +412,9 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // same way the live webhook path does (src/queue/processors.ts) for the cancel hook to fire here too. contributorCapCancelCi: settings.contributorCapCancelCi ?? env.CONTRIBUTOR_CAP_CANCEL_CI_DEFAULT === "true", // #selfhost-ci-verification: the executor's OWN final pre-mutation live-CI re-check (step 8 of - // executeAgentMaintenanceActions) needs the same configured expectedCiContexts this accept-time - // re-check (above) and the original plan were both evaluated against. - expectedCiContexts: settings.expectedCiContexts, + // executeAgentMaintenanceActions) needs the same effective required contexts this accept-time re-check + // (above) evaluated against. Re-fetch so branch-protection changes remain authoritative at accept time. + requiredCiContexts: executionRequiredContexts, // #3472 split-brain: a staged approve/merge can sit queued long enough for a SIBLING pass to publish a // manual-review hold on this same PR/head before the maintainer accepts — the executor's own live guard // (step 7b of executeAgentMaintenanceActions) needs the configured label to check for. diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index a94dcce14b..9ec557efff 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -782,14 +782,14 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { // REGRESSION (gate-flagged gap, #selfhost-ci-verification): the planning pass evaluates settings.expectedCiContexts, // but this final pre-mutation re-check used to always pass `undefined` for requiredContexts (fold-all mode) -- // a maintainer-configured repo could see the plan and its own execution-time re-check disagree on ciState. - it("threads ctx.expectedCiContexts into the live CI re-check's requiredContexts argument", async () => { + it("threads ctx.requiredCiContexts into the live CI re-check's requiredContexts argument", async () => { const env = createTestEnv({}); - const outcomes = await executeAgentMaintenanceActions(env, ctx({ expectedCiContexts: ["build", "test"] }), [merge]); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ requiredCiContexts: new Set(["build", "test"]) }), [merge]); expect(outcomes[0]?.outcome).toBe("completed"); expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "sha7", expect.any(String), new Set(["build", "test"]), expect.any(String)); }); - it("passes null (fold-all) requiredContexts when ctx.expectedCiContexts is unset — unchanged pre-existing behavior", async () => { + it("passes null (fold-all) requiredContexts when ctx.requiredCiContexts is unset — unchanged pre-existing behavior", async () => { const env = createTestEnv({}); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); expect(outcomes[0]?.outcome).toBe("completed"); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 2fff622452..1718b4229c 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -32,6 +32,7 @@ vi.mock("../../src/github/app", async (importOriginal) => ({ vi.mock("../../src/github/backfill", async (importOriginal) => ({ ...(await importOriginal()), fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null })), + fetchRequiredStatusContexts: vi.fn(async () => null), fetchLivePullRequestMergeState: vi.fn(async () => "clean"), fetchLivePullRequestReviewDecision: vi.fn(async () => undefined), // Defaults to "no live blockers left" so the existing accept tests stay deterministic; individual tests below @@ -55,7 +56,7 @@ vi.mock("../../src/review/linked-issue-hard-rules", async (importOriginal) => { import { createPullRequestReview, mergePullRequest } from "../../src/github/pr-actions"; import { ensurePullRequestLabel } from "../../src/github/labels"; import { createInstallationToken } from "../../src/github/app"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers } from "../../src/github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts } from "../../src/github/backfill"; import { resolveLinkedIssueHardRule } from "../../src/review/linked-issue-hard-rules"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { actionParams, executeAgentMaintenanceActions, pendingActionToPlanned, type AgentActionExecutionContext } from "../../src/services/agent-action-executor"; @@ -504,7 +505,7 @@ describe("agent approval queue (#779)", () => { // REGRESSION (gate-flagged gap, #selfhost-ci-verification): this accept-time re-check used to always pass // `undefined` for requiredContexts (fold-all mode), even for a repo with settings.expectedCiContexts // configured -- so this re-check could disagree with the plan it is meant to validate. - it("threads the repo's settings.expectedCiContexts into the accept-time live CI re-check", async () => { + it("threads the repo's expectedCiContexts into the accept-time live CI re-check", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); // expectedCiContexts (#selfhost-ci-verification) is config-as-code only, resolved from the repo's focus @@ -517,9 +518,41 @@ describe("agent approval queue (#779)", () => { const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); expect(result.status).toBe("accepted"); + expect(fetchRequiredStatusContexts).toHaveBeenCalledWith(env, "owner/repo", null, expect.any(String), expect.any(String)); expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["build", "test"]), expect.any(String)); }); + it("unions branch-protection contexts into the accept-time live CI re-check", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await upsertRepoFocusManifest(env, "owner/repo", { gate: { expectedCiContexts: ["build"] } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, base: { ref: "main" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(fetchRequiredStatusContexts).mockResolvedValueOnce(new Set(["branch-required"])).mockResolvedValueOnce(new Set(["branch-required"])); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + expect(fetchRequiredStatusContexts).toHaveBeenCalledWith(env, "owner/repo", "main", expect.any(String), expect.any(String)); + expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["branch-required", "build"]), expect.any(String)); + }); + + it("falls back to expectedCiContexts when the accept-time branch-protection read fails", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await upsertRepoFocusManifest(env, "owner/repo", { gate: { expectedCiContexts: ["build"] } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, base: { ref: "main" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(fetchRequiredStatusContexts).mockRejectedValueOnce(new Error("branch protection unavailable")).mockRejectedValueOnce(new Error("branch protection unavailable")); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + expect(fetchLiveCiAggregate).toHaveBeenCalledWith(env, "owner/repo", "h7", expect.any(String), new Set(["build"]), expect.any(String)); + }); + it("accept supersedes a staged merge when live CI has since turned pending, not just failed (#2126)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } });