diff --git a/src/github/pr-actions.ts b/src/github/pr-actions.ts index a08918c146..2c8fa3d2dd 100644 --- a/src/github/pr-actions.ts +++ b/src/github/pr-actions.ts @@ -24,7 +24,10 @@ 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, @@ -32,6 +35,7 @@ export async function createPullRequestReview( 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 }; } diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index b33abdd18b..6ed6fb72a2 100644 --- a/src/services/agent-action-executor.ts +++ b/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 diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index d6cce6b7fa..cb99da4c27 100644 --- a/src/services/agent-approval-queue.ts +++ b/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 diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 70d3d6b9c7..a500672f2f 100644 --- a/src/settings/agent-actions.ts +++ b/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 diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index dfc2feb4ce..da43c22d9c 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/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" }; diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 929f157401..696d20444a 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/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" } }); diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 9d297adcb9..a7bb40ca8d 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/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) => {