diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 6ed6fb72a2..3d0defde8b 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -265,9 +265,23 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action: if (action.closeComment) await createIssueComment(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.closeComment); await closePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber); return; - case "update_branch": - await updatePullRequestBranch(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.expectedHeadSha); + case "update_branch": { + // update_branch does NOT need the accept-flow-level "unpinned → deny" gate that #2377/#2422 added for + // approve/merge: it only merges the current BASE into the head (never contributor-controlled content), so + // it cannot itself ratify unreviewed code the way an approval or a merge does -- the worst case is a + // premature rebase that fires a fresh synchronize and gets re-reviewed on the next pass (#2424). It's also + // already covered by the generic guards that run before ANY action class reaches this switch: step 5's + // freshness check (`expectedHeadSha ?? ctx.headSha`) denies on a moved head, and the approval-queue + // accept-flow's supersede check (agent-approval-queue.ts) is actionClass-agnostic. The `?? ctx.headSha` + // fallback below is pure parity/defense-in-depth for the tiny window between that freshness read and this + // call, matching the same pattern used by approve/merge immediately above. + const updateSha = action.expectedHeadSha ?? ctx.headSha; + /* v8 ignore next -- the step-5 freshness guard above already denies the action when + * action.expectedHeadSha ?? ctx.headSha is falsy, so updateSha (the same expression) is always a + * truthy string here; the ?? undefined only satisfies updatePullRequestBranch's string|undefined type. */ + await updatePullRequestBranch(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, updateSha ?? undefined); return; + } } } diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 4a7023f037..43c31ec9cb 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -103,6 +103,18 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect((await auditFor(env, "merge"))?.outcome).toBe("completed"); }); + it("REGRESSION (#2424): LIVE update_branch falls back to ctx.headSha when the action carries no expectedHeadSha of its own", async () => { + // The `updateBranch` fixture above is pre-pinned (expectedHeadSha: "sha7"), so the big LIVE test never + // exercises the `?? ctx.headSha` fallback -- it's parity with approve/merge for the tiny window between + // step 5's freshness read and this call, matching a live sweep's construction (processors.ts:2196-2202 + // always sets expectedHeadSha, but the fallback exists for any future/legacy caller that omits it). + const env = createTestEnv({}); + const unpinnedUpdateBranch: PlannedAgentAction = { actionClass: "update_branch", requiresApproval: false, reason: "behind base" }; + const outcomes = await executeAgentMaintenanceActions(env, ctx({ headSha: "sha7" }), [unpinnedUpdateBranch]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(updatePullRequestBranch).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "sha7"); + }); + it("LIVE approve with dismissStaleApproval retracts the stale review instead of posting a new one (#2254)", async () => { const env = createTestEnv({}); const dismiss: PlannedAgentAction = { actionClass: "approve", requiresApproval: false, reason: "stale approval retracted", dismissStaleApproval: true };