fix(agent-actions): pin a staged approve to its reviewed head SHA

src/github/pr-actions.ts

@@ -24,14 +24,18 @@ function splitRepo(repoFullName: string): { owner: string; repo: string } {
 
 export type PullRequestReviewEvent = "REQUEST_CHANGES" | "APPROVE" | "COMMENT";
 
-/** Post a pull-request review (request-changes / approve / comment). `body` is required for REQUEST_CHANGES. */
+/** Post a pull-request review (request-changes / approve / comment). `body` is required for REQUEST_CHANGES.
+ *  `commitId`, when given, pins the review to that exact commit (GitHub's `commit_id`) instead of defaulting to
+ *  the PR's CURRENT head — so a review staged/reviewed against one commit can never silently land on a
+ *  force-pushed, unreviewed later commit (#2262). */
 export async function createPullRequestReview(
   env: Env,
   installationId: number,
   repoFullName: string,
   pullNumber: number,
   event: PullRequestReviewEvent,
   body: string,
+  commitId?: string,
 ): Promise<{ id: number }> {
   const { owner, repo } = splitRepo(repoFullName);
   const token = await createInstallationToken(env, installationId);
@@ -42,6 +46,7 @@ export async function createPullRequestReview(
     pull_number: pullNumber,
     event,
     body,
+    ...(commitId ? { commit_id: commitId } : {}),
   });
   return { id: (response.data as { id: number }).id };
 }

src/services/agent-action-executor.ts

@@ -235,13 +235,23 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action:
     case "request_changes":
       await createPullRequestReview(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, "REQUEST_CHANGES", action.reviewBody ?? "");
       return;
-    case "approve":
+    case "approve": {
       if (action.dismissStaleApproval) {
         await dismissLatestBotApproval(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, "Gittensory retracted this approval — a newer commit no longer qualifies.");
-      } else {
-        await createPullRequestReview(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, "APPROVE", action.reviewBody ?? "");
+        return;
       }
+      // Pin the approve to the REVIEWED head (#2262), mirroring the merge case's identical pattern immediately
+      // below: for an approval-queue replay this is the commit the maintainer reviewed, not necessarily the
+      // current head, so GitHub's own commit_id targeting keeps a force-push after staging from silently
+      // landing on the new, unreviewed commit. A live sweep plans expectedHeadSha == ctx.headSha, so its
+      // behavior is unchanged; the fallback covers any unpinned plan.
+      const approveSha = 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 approveSha (the same expression) is always a
+       * truthy string here; the ?? undefined only satisfies createPullRequestReview's string|undefined type. */
+      await createPullRequestReview(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, "APPROVE", action.reviewBody ?? "", approveSha ?? undefined);
       return;
+    }
     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

src/services/agent-approval-queue.ts

@@ -59,6 +59,27 @@ 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.
+  // 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) {
+    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 with no reviewed-head pin, so freshness cannot be verified — re-stage from a fresh sweep`,
+      metadata: baseMetadata,
+    });
+    return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "unpinned_legacy_action" };
+  }
 
   // Re-derive live justification for a staged MERGE at accept time. auto_with_approval rows have no expiry, so
   // CI can flip red, the base can go dirty, or a reviewer can request changes while the row just sits waiting for

src/settings/agent-actions.ts

@@ -441,6 +441,12 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
       requiresApproval: approval("approve"),
       reason: "gate passed, CI green",
       reviewBody: "Gittensory approves — the gate is satisfied and CI is green.",
+      // Pin the approve to the EXACT reviewed head (#2262), matching the merge action's existing pin. For an
+      // auto_with_approval stage this travels into the pending row (actionParams persists expectedHeadSha), so
+      // the accept-time supersede check — which only fires when expectedHeadSha is truthy — actually engages: a
+      // force-push after staging is detected and denied instead of the accept silently approving the NEW,
+      // unreviewed commit. The executor also pins createPullRequestReview's commit_id to this SHA.
+      ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}),
     });
   } else if (
     // A prior bot approval is now STALE: a later commit landed (approvedHeadSha !== the current head) and this

test/unit/agent-action-executor.test.ts

@@ -92,7 +92,9 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
     expect(outcomes.map((o) => o.outcome)).toEqual(["completed", "completed", "completed", "completed", "completed", "completed"]);
     expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "gittensory:ready-to-merge", { createMissingLabel: true });
     expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "REQUEST_CHANGES", "please fix");
-    expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "APPROVE", "lgtm");
+    // Falls back to ctx.headSha ("sha7") as the pinned commit_id when the action carries no expectedHeadSha of
+    // its own — a live sweep's approve plans no explicit pin, so this is the unpinned/live-sweep case (#2262).
+    expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "APPROVE", "lgtm", "sha7");
     expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" });
     expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "closing");
     expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7);
@@ -156,7 +158,9 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
     const bareApprove: PlannedAgentAction = { actionClass: "approve", requiresApproval: false, reason: "passed" };
     await executeAgentMaintenanceActions(env, ctx(), [bareRequestChanges, bareApprove]);
     expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "REQUEST_CHANGES", "");
-    expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "APPROVE", "");
+    // The approve still falls back to ctx.headSha ("sha7") as the pinned commit_id, same as the "LIVE: executes
+    // each action class" test above — request_changes has no head-pinning of its own (unaffected).
+    expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "APPROVE", "", "sha7");
   });
 
   it("LIVE merge pins the GitHub merge to the action's reviewed head (expectedHeadSha) over the context head", async () => {
@@ -169,6 +173,15 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
     expect(fetchPullRequestFreshness).toHaveBeenCalledWith(env, expect.objectContaining({ expectedHeadSha: "reviewed-sha" }));
   });
 
+  it("LIVE approve pins the review to the action's reviewed head (expectedHeadSha) over the context head, falling back to an empty body (#2262)", async () => {
+    const env = createTestEnv({});
+    // A staged approve replayed on accept carries the REVIEWED head — same pin as merge already has — and this
+    // one also has no reviewBody set, exercising the empty-string fallback.
+    const pinnedApprove: PlannedAgentAction = { actionClass: "approve", requiresApproval: false, reason: "gate passed", expectedHeadSha: "reviewed-sha" };
+    await executeAgentMaintenanceActions(env, ctx({ headSha: "live-sha" }), [pinnedApprove]);
+    expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "APPROVE", "", "reviewed-sha");
+  });
+
   it("LIVE heuristic close is denied when live CI has since turned green (#2128)", async () => {
     const env = createTestEnv({});
     const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic" };

test/unit/agent-approval-queue.test.ts

@@ -5,6 +5,7 @@ vi.mock("../../src/github/pr-actions", () => ({
   mergePullRequest: vi.fn(async () => ({ merged: true, sha: "merged-sha" })),
   closePullRequest: vi.fn(async () => ({ state: "closed" })),
   createIssueComment: vi.fn(async () => ({ id: 2 })),
+  dismissLatestBotApproval: vi.fn(async () => ({ dismissed: true })),
 }));
 vi.mock("../../src/github/labels", () => ({
   ensurePullRequestLabel: vi.fn(async () => ({ applied: true, created: false })),
@@ -34,7 +35,7 @@ vi.mock("../../src/github/backfill", async (importOriginal) => ({
   fetchLivePullRequestReviewDecision: vi.fn(async () => undefined),
 }));
 
-import { mergePullRequest } from "../../src/github/pr-actions";
+import { createPullRequestReview, mergePullRequest } from "../../src/github/pr-actions";
 import { ensurePullRequestLabel } from "../../src/github/labels";
 import { createInstallationToken } from "../../src/github/app";
 import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision } from "../../src/github/backfill";
@@ -171,6 +172,75 @@ describe("agent approval queue (#779)", () => {
     expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" });
   });
 
+  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" } });
+    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: "approve", autonomyLevel: "auto_with_approval", params: { reviewBody: "lgtm", expectedHeadSha: "h7" }, reason: "gate passed" });
+
+    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 via commit_id — not merely whatever the current head happens to be.
+    expect(createPullRequestReview).toHaveBeenCalledWith(env, 5, "owner/repo", 7, "APPROVE", "lgtm", "h7");
+  });
+
+  it("REGRESSION (#2377): accept denies an approve staged with NO reviewed-head pin, rather than silently approving whatever commit is currently live", async () => {
+    // A row with no expectedHeadSha (e.g. staged by code predating the head-pin fix, or a planning pass that ran
+    // against a transiently-null stored head SHA) carries no record of what was actually reviewed. Unlike merge's
+    // `sha` param, GitHub's review API has no server-side staleness rejection for `commit_id` — the ONLY
+    // protection is this application-level check, so an unpinned row must be refused, not silently approved
+    // against whatever the live head happens to be at accept time.
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" });
+    await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "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: "approve", autonomyLevel: "auto_with_approval", params: { reviewBody: "lgtm" }, reason: "gate passed" });
+
+    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(createPullRequestReview).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 does NOT deny an unpinned dismissStaleApproval retraction — retracting an approval carries no ratify-unreviewed-code risk (#2377)", async () => {
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" });
+    await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "auto_with_approval" } });
+    await seedInstallation(env);
+    await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h-CURRENT" }, labels: [], body: "x" });
+    const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "approve", autonomyLevel: "auto_with_approval", params: { dismissStaleApproval: true }, reason: "stale approval retracted" });
+
+    const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" });
+    expect(result.status).toBe("accepted");
+    expect(result.executionOutcome).toBe("completed");
+    expect(createPullRequestReview).not.toHaveBeenCalled(); // dismiss retracts, never posts a new APPROVE
+  });
+
+  it("accept supersedes a staged approve when the live head moved after staging (force-push fail-safe, #2262)", async () => {
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" });
+    await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "auto_with_approval" } });
+    await seedInstallation(env);
+    // The PR head is now h-NEW: the contributor force-pushed after the approve was staged against h-OLD. Before
+    // #2262, expectedHeadSha was never set on a planned approve, so this staleness guard was a silent no-op and
+    // the accept would have approved the new, unreviewed commit.
+    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: "approve", autonomyLevel: "auto_with_approval", params: { reviewBody: "lgtm", expectedHeadSha: "h-OLD" }, reason: "gate passed" });
+
+    const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" });
+    expect(result.status).toBe("rejected");
+    expect(result.executionOutcome).toBe("head_moved");
+    expect(createPullRequestReview).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 supersedes a staged merge when live CI has since turned failed (no head move) (#2126)", async () => {
     const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" });
     await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } });

test/unit/github-pr-actions.test.ts

@@ -28,9 +28,24 @@ describe("GitHub PR action primitives (#778)", () => {
     const result = await createPullRequestReview(envWithKey(), 123, "owner/repo", 7, "REQUEST_CHANGES", "please fix");
     expect(result).toEqual({ id: 99 });
     expect(calls[0]).toMatchObject({ method: "POST", body: { event: "REQUEST_CHANGES", body: "please fix" } });
+    expect(calls[0]?.body).not.toHaveProperty("commit_id"); // no commitId passed → no commit_id sent
     expect(calls[0]?.url).toMatch(/\/repos\/owner\/repo\/pulls\/7\/reviews$/);
   });
 
+  it("pins an approve review to the reviewed commit via commit_id when provided (#2262)", async () => {
+    const calls: Array<{ method: string; url: string; body: unknown }> = [];
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
+      const url = input.toString();
+      if (url.includes("/access_tokens")) return Response.json({ token: "t" });
+      calls.push({ method: init?.method ?? "GET", url, body: init?.body ? JSON.parse(String(init.body)) : null });
+      if (url.endsWith("/pulls/7/reviews")) return Response.json({ id: 100 });
+      return new Response("unexpected", { status: 500 });
+    });
+    const result = await createPullRequestReview(envWithKey(), 123, "owner/repo", 7, "APPROVE", "lgtm", "reviewed-sha");
+    expect(result).toEqual({ id: 100 });
+    expect(calls[0]).toMatchObject({ method: "POST", body: { event: "APPROVE", body: "lgtm", commit_id: "reviewed-sha" } });
+  });
+
   it("posts a quiet COMMENT review with inline comments anchored to the head SHA (#inline-comments)", async () => {
     const calls: Array<{ method: string; url: string; body: unknown }> = [];
     vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {