diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 4a520ed331..83d3d82492 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -59,16 +59,21 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de }); return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "head_moved" }; } - // An unpinned staged approve (no expectedHeadSha) cannot be safety-verified against a force-push that - // happened during the queue wait: unlike merge's `sha` param (which GitHub 409s on mismatch), the reviews API's - // `commit_id` is purely advisory -- GitHub will happily post an APPROVE at any valid commit, current or not. - // The check above only fires when a pin EXISTS and disagrees with the live head; a row staged with no pin at - // all (e.g. by code predating this head-pinning fix, or a planning pass that ran against a transiently-null - // stored head SHA) would otherwise fall through to the executor's `ctx.headSha` fallback and silently approve - // whatever commit is live NOW, under the authority of a review that was never actually performed against it. + // An unpinned staged approve or merge (no expectedHeadSha) cannot be safety-verified against a force-push that + // happened during the queue wait. For a PINNED merge, GitHub's `sha` param 409s on mismatch -- a real backstop. + // But that backstop only exists because there's something to compare against; an UNPINNED merge falls back to + // performAction's `mergeSha = action.expectedHeadSha ?? ctx.headSha`, which by construction substitutes + // whatever head is live right now, so it trivially "matches" and no 409 is possible. The reviews API's + // `commit_id` has no server-side staleness rejection at all, pinned or not (#2377). Either way, the check above + // only fires when a pin EXISTS and disagrees with the live head; a row staged with no pin at all (e.g. by code + // predating this head-pinning fix, or a planning pass that ran against a transiently-null stored head SHA) + // would otherwise fall through to the executor's `ctx.headSha` fallback and silently ratify whatever commit is + // live NOW, under the authority of a review/merge that was never actually performed against it (#2422). // dismissStaleApproval is exempt: it RETRACTS the bot's existing approval rather than granting a new one at a // specific commit, so it carries no "ratify unreviewed code" risk and is safe to replay unpinned. - if (!stagedHead && pending.actionClass === "approve" && !pending.params.dismissStaleApproval) { + const isUnpinnedRatifyingAction = + !stagedHead && ((pending.actionClass === "approve" && !pending.params.dismissStaleApproval) || pending.actionClass === "merge"); + if (isUnpinnedRatifyingAction) { await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); await recordAuditEvent(env, { eventType: "agent.pending_action.superseded", diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 22e004d5fa..8052462bb5 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -129,7 +129,7 @@ describe("agent approval queue (#779)", () => { await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); await seedInstallation(env); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); - const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash" }, reason: "clean" }); + 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" }); const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); expect(result.status).toBe("accepted"); @@ -172,6 +172,27 @@ describe("agent approval queue (#779)", () => { expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); }); + it("REGRESSION (#2422): accept denies a merge staged with NO reviewed-head pin, rather than silently merging whatever commit is currently live", async () => { + // Unlike a PINNED merge, where GitHub's `sha` param 409s on mismatch (a real backstop), an UNPINNED merge + // falls back to performAction's `mergeSha = action.expectedHeadSha ?? ctx.headSha`, which by construction + // substitutes the current live head -- no mismatch is possible, so the 409 backstop never fires. This is the + // same class of gap #2377 closed for approve; the identical accept-flow gate now covers merge too. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h-UNREVIEWED" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("unpinned_legacy_action"); + expect(mergePullRequest).not.toHaveBeenCalled(); + expect((await getPendingAgentAction(env, action.id))?.status).toBe("rejected"); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("no reviewed-head pin"); + }); + it("accept executes a staged approve when the staged head still matches the live head, pinned to the reviewed SHA (#2262)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "auto_with_approval" } }); @@ -464,7 +485,7 @@ describe("agent approval queue (#779)", () => { await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" }, agentDryRun: true }); await seedInstallation(env); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); - const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash" }, reason: "clean" }); + 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" }); const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); expect(result.status).toBe("accepted"); @@ -477,7 +498,7 @@ describe("agent approval queue (#779)", () => { await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "auto" } }); await seedInstallation(env); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); - const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash" }, reason: "clean" }); + 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" }); const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); expect(result.status).toBe("accepted"); @@ -512,7 +533,7 @@ describe("agent approval queue (#779)", () => { it("accept records error when the staged action cannot execute (no write permission)", async () => { const env = createTestEnv({}); // No settings/installation seeded → autonomy is empty + no pull_requests:write → the merge is denied. - const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash" }, reason: "clean" }); + 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" }); const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); expect(result.status).toBe("accepted"); // the decision is recorded... expect(result.executionOutcome).toBe("denied"); // ...but the action could not run diff --git a/test/unit/mcp-automation-state.test.ts b/test/unit/mcp-automation-state.test.ts index 43d360dd10..c5669193f1 100644 --- a/test/unit/mcp-automation-state.test.ts +++ b/test/unit/mcp-automation-state.test.ts @@ -403,7 +403,7 @@ describe("MCP gittensory_decide_pending_action (#784)", () => { await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 5); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" }, agentDryRun: true }); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); - const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash" }, reason: "clean" }); + 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" }); const client = await connect(env); const result = await client.callTool({ name: "gittensory_decide_pending_action", arguments: { owner: "owner", repo: "repo", id: action.id, decision: "accept" } }); diff --git a/test/unit/routes-agent-approval.test.ts b/test/unit/routes-agent-approval.test.ts index 30337daab2..ff8813d8d3 100644 --- a/test/unit/routes-agent-approval.test.ts +++ b/test/unit/routes-agent-approval.test.ts @@ -50,7 +50,7 @@ async function seedPending(env: Env) { repositories: [{ name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }], }); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); - const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash" }, reason: "clean" }); + 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" }); return action; }