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
6 changes: 3 additions & 3 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
22 changes: 8 additions & 14 deletions src/services/agent-action-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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<string> | 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<string> | 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.
Expand Down Expand Up @@ -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),
Expand Down
24 changes: 15 additions & 9 deletions src/services/agent-approval-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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,
{
Expand All @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions test/unit/agent-action-executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
37 changes: 35 additions & 2 deletions test/unit/agent-approval-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ vi.mock("../../src/github/app", async (importOriginal) => ({
vi.mock("../../src/github/backfill", async (importOriginal) => ({
...(await importOriginal<typeof import("../../src/github/backfill")>()),
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
Expand All @@ -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";
Expand Down Expand Up @@ -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
Expand All @@ -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" } });
Expand Down