diff --git a/src/mcp/server.ts b/src/mcp/server.ts index eda5c352ca..265390021b 100644 --- a/src/mcp/server.ts +++ b/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 } : {}), diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index cb99da4c27..4a520ed331 100644 --- a/src/services/agent-approval-queue.ts +++ b/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 }; } diff --git a/src/types.ts b/src/types.ts index 1fc400e92e..07b3dcbd54 100644 --- a/src/types.ts +++ b/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). */ diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index f4af9901af..22e004d5fa 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/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" }); diff --git a/test/unit/mcp-automation-state.test.ts b/test/unit/mcp-automation-state.test.ts index 1ff8fd4660..43d360dd10 100644 --- a/test/unit/mcp-automation-state.test.ts +++ 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(); + 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: "" });