fix(agent-actions): distinguish an executor error from a clean accept

src/mcp/server.ts

@@ -404,7 +404,7 @@ const automationStateOutputSchema = {
 const listPendingActionsShape = {
   owner: z.string().min(1),
   repo: z.string().min(1),
-  status: z.enum(["pending", "accepted", "rejected"]).optional(),
+  status: z.enum(["pending", "accepted", "rejected", "errored"]).optional(),
 };
 
 const pendingActionEntrySchema = z.object({
@@ -2530,9 +2530,11 @@ export class GittensoryMcp {
       summary:
         result.status === "accepted"
           ? `Accepted ${pending.actionClass} on ${fullName}#${pending.pullNumber} (execution: ${result.executionOutcome}).`
-          : result.status === "rejected"
-            ? `Rejected ${pending.actionClass} on ${fullName}#${pending.pullNumber}.`
-            : `Action ${input.id} was already decided.`,
+          : result.status === "errored"
+            ? `Accepted ${pending.actionClass} on ${fullName}#${pending.pullNumber}, but execution errored: ${result.executionOutcome}.`
+            : result.status === "rejected"
+              ? `Rejected ${pending.actionClass} on ${fullName}#${pending.pullNumber}.`
+              : `Action ${input.id} was already decided.`,
       data: {
         status: result.status,
         ...(result.executionOutcome !== undefined ? { executionOutcome: result.executionOutcome } : {}),

src/services/agent-approval-queue.ts

@@ -10,7 +10,7 @@ import type { AgentPendingActionParams, AgentPendingActionRecord } from "../type
 export type ApprovalDecision = "accept" | "reject";
 
 export type ApprovalDecisionResult = {
-  status: "accepted" | "rejected" | "already_decided" | "not_found";
+  status: "accepted" | "errored" | "rejected" | "already_decided" | "not_found";
   action?: AgentPendingActionRecord;
   // For an accept, the executor outcome of running the staged action (completed / denied / error / dry_run).
   executionOutcome?: string;
@@ -159,7 +159,14 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de
   );
   /* v8 ignore next -- the executor returns one outcome per planned action, so the fallback is defensive. */
   const execOutcome = outcomes[0]?.outcome ?? "no_outcome";
-  await setPendingAgentActionStatus(env, pending.id, { status: "accepted", decidedBy: input.decidedBy });
+  // "error" means performAction threw a real exception (a GitHub-call failure) -- persist "errored" so a
+  // maintainer scanning the queue can see the mutation itself failed, not just that a decision was recorded.
+  // Every OTHER outcome ("completed", "denied", "dry_run", "queued") is a clean result of the executor's own
+  // gates running to a normal conclusion -- "denied" in particular is an intentional policy decision (autonomy no
+  // longer authorizes, dry-run active, a live pre-condition failed cleanly), not a failure, so it correctly stays
+  // "accepted": the maintainer's accept WAS honored, the executor just chose not to act on it (#2423).
+  const finalStatus = execOutcome === "error" ? "errored" : "accepted";
+  await setPendingAgentActionStatus(env, pending.id, { status: finalStatus, decidedBy: input.decidedBy });
   await recordAuditEvent(env, {
     eventType: "agent.pending_action.accepted",
     actor: input.decidedBy,
@@ -168,5 +175,5 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de
     detail: `accepted ${pending.actionClass} → ${execOutcome}`,
     metadata: { ...baseMetadata, executionOutcome: execOutcome },
   });
-  return { status: "accepted", action: { ...pending, status: "accepted", decidedBy: input.decidedBy }, executionOutcome: execOutcome };
+  return { status: finalStatus, action: { ...pending, status: finalStatus, decidedBy: input.decidedBy }, executionOutcome: execOutcome };
 }

src/types.ts

@@ -696,7 +696,11 @@ export type AgentPendingActionParams = {
   dismissStaleApproval?: boolean;
 };
 
-export type AgentPendingActionStatus = "pending" | "accepted" | "rejected";
+// "errored" is distinct from "accepted": the maintainer's accept decision ran the staged action through the
+// executor, but the mutation itself threw (a real GitHub-call failure), as opposed to a clean "accepted" outcome
+// where the executor's own gates (autonomy/dry-run/freshness) declined to act -- that's an intentional policy
+// result, not a failure, and stays "accepted" (#2423).
+export type AgentPendingActionStatus = "pending" | "accepted" | "rejected" | "errored";
 
 /** Approval-queue row (#779): an `auto_with_approval` action the write-actions layer staged for a one-tap
  *  maintainer accept (→ execute) or reject (→ cancel). */

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

@@ -520,6 +520,26 @@ describe("agent approval queue (#779)", () => {
     expect((await env.DB.prepare("select outcome from audit_events where event_type = ?").bind("agent.pending_action.accepted").first<{ outcome: string }>())?.outcome).toBe("error");
   });
 
+  it("REGRESSION (#2423): accept persists status=errored, not accepted, when the executor's mutation call throws", async () => {
+    // Distinct from the "no write permission" test above: there, the executor's own gates cleanly DENY before
+    // ever attempting a mutation -- a legitimate, intentional non-action, correctly recorded as "accepted". Here
+    // every gate passes and the executor genuinely ATTEMPTS the GitHub call, which fails -- that failure must not
+    // read the same as a quiet, uneventful success in the approval-queue listing.
+    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" });
+    vi.mocked(mergePullRequest).mockRejectedValueOnce(new Error("GitHub 500"));
+
+    const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" });
+    expect(result.status).toBe("errored");
+    expect(result.executionOutcome).toBe("error");
+    expect((await getPendingAgentAction(env, action.id))?.status).toBe("errored");
+    const audit = await env.DB.prepare("select outcome from audit_events where event_type = ?").bind("agent.pending_action.accepted").first<{ outcome: string }>();
+    expect(audit?.outcome).toBe("error");
+  });
+
   it("actionParams extracts only the field for the action class", () => {
     expect(actionParams({ actionClass: "label", requiresApproval: false, reason: "x", label: "L" })).toEqual({ label: "L" });
     expect(actionParams({ actionClass: "request_changes", requiresApproval: false, reason: "x", reviewBody: "B" })).toEqual({ reviewBody: "B" });

test/unit/mcp-automation-state.test.ts

@@ -3,6 +3,7 @@ import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js";
 import { beforeEach, describe, expect, it, vi } from "vitest";
 import { GittensoryMcp } from "../../src/mcp/server";
 import { getRepositoryCollaboratorPermission } from "../../src/github/app";
+import { mergePullRequest } from "../../src/github/pr-actions";
 import { createPendingAgentActionIfAbsent, getPendingAgentAction, listPendingAgentActions, recordAuditEvent, upsertInstallation, upsertOfficialMinerDetection, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories";
 import type { AuthIdentity } from "../../src/auth/security";
 import { createTestEnv } from "../helpers/d1";
@@ -12,6 +13,30 @@ vi.mock("../../src/github/app", async (importOriginal) => ({
   getRepositoryCollaboratorPermission: vi.fn(),
   createInstallationToken: vi.fn(async () => "test-installation-token"),
 }));
+// No test in this file exercises a genuinely successful live mutation (every accept path here is dry-run,
+// rejected, or gate-denied before reaching performAction) except the dedicated #2423 "errored" test below, which
+// needs a controllable throw. Mocked the same shape as agent-approval-queue.test.ts's dedicated unit coverage.
+vi.mock("../../src/github/pr-actions", () => ({
+  createPullRequestReview: vi.fn(async () => ({ id: 1 })),
+  mergePullRequest: vi.fn(async () => ({ merged: true, sha: "merged-sha" })),
+  closePullRequest: vi.fn(async () => ({ state: "closed" })),
+  createIssueComment: vi.fn(async () => ({ id: 2 })),
+}));
+// The executor's step-5 freshness guard otherwise calls the REAL fetchPullRequestFreshness, which needs a live
+// GitHub token/API — unreachable in this test's offline env, so it fails "unavailable" and denies BEFORE the
+// #2423 test below ever reaches performAction. Only that one test needs this; every other accept path here is
+// denied/rejected/dry-run before step 5 is consulted, so defaulting to "current" is inert for them.
+vi.mock("../../src/github/pr-freshness", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("../../src/github/pr-freshness")>();
+  return {
+    ...actual,
+    fetchPullRequestFreshness: vi.fn(async (_env: Env, args: { expectedHeadSha?: string | null }) => ({
+      status: "current" as const,
+      liveHeadSha: args.expectedHeadSha ?? null,
+      liveState: "open",
+    })),
+  };
+});
 // decidePendingAgentAction's accept-time live re-check (#2126) needs these off-network, deterministic here — the
 // dedicated staleness-supersede test coverage lives in agent-approval-queue.test.ts, not this MCP-surface file.
 vi.mock("../../src/github/backfill", async (importOriginal) => ({
@@ -388,6 +413,27 @@ describe("MCP gittensory_decide_pending_action (#784)", () => {
     expect((await getPendingAgentAction(env, action.id))?.status).toBe("accepted");
   });
 
+  it("REGRESSION (#2423): reports status=errored (not accepted) and a distinct summary when the live mutation throws", async () => {
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" });
+    await upsertInstallation(env, {
+      installation: { id: 5, account: { login: "owner", id: 1, type: "User" }, repository_selection: "selected", permissions: { metadata: "read", pull_requests: "write" }, events: ["pull_request"] },
+      repositories: [{ name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }],
+    });
+    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" } });
+    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" });
+    vi.mocked(mergePullRequest).mockRejectedValueOnce(new Error("GitHub 500"));
+
+    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" } });
+    const data = result.structuredContent as { status: string; executionOutcome: string };
+    expect(data.status).toBe("errored");
+    expect(data.executionOutcome).toBe("error");
+    expect(JSON.stringify(result)).toMatch(/execution errored/);
+    expect((await getPendingAgentAction(env, action.id))?.status).toBe("errored");
+  });
+
   it("denies a static MCP-token caller from deciding a pending action when the repo is not allowlisted (#2253)", async () => {
     // "" overrides createTestEnv's own MCP_ACTUATION_REPO_ALLOWLIST: "*" default back to unset.
     const env = createTestEnv({ MCP_ACTUATION_REPO_ALLOWLIST: "" });