From 222f0e44cb9f3922621b29c0e3b2ec0fc6c22121 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 24 Jun 2026 04:05:27 -0700 Subject: [PATCH] fix(agent): pin staged auto_with_approval merge to the reviewed head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A staged `auto_with_approval` merge stored no head SHA and no gate/CI snapshot. On accept the executor re-read the current head fresh and merged it, so a contributor could get a clean PR staged, force-push a backdoor, and a maintainer accepting the stale "gate passed, CI green" queue entry would merge the malicious commit with no re-validation. The planner now pins the merge to the exact reviewed head SHA (expectedHeadSha, persisted via actionParams into the pending row). On accept, the approval queue refuses and supersedes the staged action when the live head no longer matches the reviewed head, and the executor pins the GitHub merge `sha` to the reviewed commit as a backstop — a moved head fails safe with a 409 (terminal hold) instead of merging un-reviewed code. A live sweep plans expectedHeadSha == the current head, so its behavior is unchanged. --- src/services/agent-action-executor.ts | 10 ++++-- src/services/agent-approval-queue.ts | 19 +++++++++++ src/settings/agent-actions.ts | 5 +++ test/unit/agent-action-executor.test.ts | 9 +++++ test/unit/agent-actions.test.ts | 5 +++ test/unit/agent-approval-queue.test.ts | 45 +++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 6ca87c7d91..aa22f0de20 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -180,9 +180,15 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action: case "approve": await createPullRequestReview(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, "APPROVE", action.reviewBody ?? ""); return; - case "merge": - await mergePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, { mergeMethod: action.mergeMethod ?? "squash", ...(ctx.headSha ? { sha: ctx.headSha } : {}) }); + case "merge": { + // Pin the merge to the REVIEWED head (action.expectedHeadSha) when present — for an approval-queue replay + // this is the commit the maintainer reviewed, not necessarily the current head, so a force-push after + // staging fails safe with a 409 (→ terminal hold) instead of merging un-reviewed code. A live sweep plans + // expectedHeadSha == ctx.headSha, so its behavior is unchanged; the fallback covers any unpinned plan. + const mergeSha = action.expectedHeadSha ?? ctx.headSha; + await mergePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, { mergeMethod: action.mergeMethod ?? "squash", ...(mergeSha ? { sha: mergeSha } : {}) }); return; + } case "close": if (action.closeComment) await createIssueComment(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.closeComment); await closePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber); diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 97dede42e0..a391b5c106 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -36,6 +36,25 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de getPullRequest(env, pending.repoFullName, pending.pullNumber), getInstallation(env, pending.installationId), ]); + + // Re-validate the staged action against the LIVE head before executing. A staged merge records the reviewed + // head (expectedHeadSha); if the contributor force-pushed after staging, the live head has moved and replaying + // the action would act on un-reviewed code. Refuse, supersede the sticky row, and record it. This is the + // application-level fail-safe; the executor additionally pins the GitHub merge to the reviewed SHA as a backstop. + const stagedHead = pending.params.expectedHeadSha; + if (stagedHead && pr?.headSha && stagedHead !== pr.headSha) { + await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); + await recordAuditEvent(env, { + eventType: "agent.pending_action.superseded", + actor: input.decidedBy, + targetKey, + outcome: "denied", + detail: `superseded ${pending.actionClass}: staged head ${stagedHead.slice(0, 12)} no longer matches live head ${pr.headSha.slice(0, 12)} (force-push after staging)`, + metadata: { ...baseMetadata, stagedHeadSha: stagedHead, liveHeadSha: pr.headSha }, + }); + return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "head_moved" }; + } + const outcomes = await executeAgentMaintenanceActions( env, { diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 3e97420d1e..76755c325c 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -414,6 +414,11 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne requiresApproval: approval("merge"), reason: `gate passed, CI green, mergeable, ${autoMaintain.requireApprovals} approval(s) satisfied`, mergeMethod: autoMaintain.mergeMethod, + // Pin the merge to the EXACT reviewed head. For an `auto_with_approval` stage this travels into the pending + // row (actionParams persists expectedHeadSha), so a force-push after staging can never be merged: the + // executor pins GitHub's merge `sha` to this commit → a moved head yields a 409 (terminal hold) instead of + // merging un-reviewed code. A live sweep sets this == ctx.headSha, so its behavior is unchanged. + ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}), }); } else if (willClose) { // Contributor PR that is NOT review-good (gate blockers / red / unverified CI) OR conflicts with base → diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index f9be0686d5..6a3eea3157 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -69,6 +69,15 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect((await auditFor(env, "merge"))?.outcome).toBe("completed"); }); + it("LIVE merge pins the GitHub merge to the action's reviewed head (expectedHeadSha) over the context head", async () => { + const env = createTestEnv({}); + // A staged merge replayed on accept carries the REVIEWED head. Even when ctx.headSha is a newer live head, + // the merge must pin to the reviewed commit so a force-pushed (un-reviewed) head can never be merged. + const pinnedMerge: PlannedAgentAction = { actionClass: "merge", requiresApproval: false, reason: "clean", mergeMethod: "squash", expectedHeadSha: "reviewed-sha" }; + await executeAgentMaintenanceActions(env, ctx({ headSha: "live-sha" }), [pinnedMerge]); + expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "reviewed-sha" }); + }); + it("LIVE label with labelOp=add + comment: adds the label AND posts the comment", async () => { const env = createTestEnv({}); const flag: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "flag", label: "gittensory:pending-closure", labelOp: "add", comment: "⚠️ flagged" }; diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 808bf9e73b..e30a32f957 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -116,6 +116,11 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(plan.find((a) => a.actionClass === "merge")).toMatchObject({ mergeMethod: "rebase" }); }); + it("pins the planned merge to the PR's reviewed head SHA so a staged merge cannot replay against a moved head", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, autoMaintain: { requireApprovals: 0, mergeMethod: "squash" }, pr: { labels: [], mergeableState: "clean", headSha: "reviewed-abc" } })); + expect(plan.find((a) => a.actionClass === "merge")).toMatchObject({ mergeMethod: "squash", expectedHeadSha: "reviewed-abc" }); + }); + 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"); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 01e2bef1ad..e80d3243f4 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -113,6 +113,51 @@ describe("agent approval queue (#779)", () => { expect(audit).toMatchObject({ outcome: "completed", actor: "owner" }); }); + it("accept supersedes a staged merge when the live head moved after staging (force-push fail-safe)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + // The PR head is now h-NEW: the contributor force-pushed after the merge was staged against h-OLD. + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h-NEW" }, 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: "h-OLD" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("head_moved"); + 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("force-push after staging"); + }); + + it("accept executes a staged merge when the staged head still matches the live head (pinned to the reviewed SHA)", async () => { + 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: "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", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + // Pinned to the REVIEWED head from the staged params — not merely whatever the current head happens to be. + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + }); + + it("accept does not supersede when the PR record is absent (no live head to compare) — proceeds to the executor", async () => { + const env = createTestEnv({}); + // No PR seeded → getPullRequest returns null → pr?.headSha is undefined, so the staleness guard is skipped + // even though the staged action carries an expectedHeadSha. No settings/install → the merge denies downstream. + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h-OLD" }, reason: "clean" }); + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("denied"); + expect(mergePullRequest).not.toHaveBeenCalled(); + const superseded = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ n: number }>(); + expect(superseded?.n).toBe(0); + }); + it("accept honors current dry-run setting instead of forcing a live mutation", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" }, agentDryRun: true });