diff --git a/packages/mcp-server/src/tools.test.ts b/packages/mcp-server/src/tools.test.ts index 01773dbe33b3..2e03b5914680 100644 --- a/packages/mcp-server/src/tools.test.ts +++ b/packages/mcp-server/src/tools.test.ts @@ -546,6 +546,126 @@ describe("paperclip MCP tools", () => { expect(JSON.parse(String(init.body))).toEqual({}); }); + it("routes approval withdraw to the requester-scoped withdraw endpoint", async () => { + const fetchMock = vi.fn().mockResolvedValue( + mockJsonResponse({ id: "approval-1", status: "withdrawn" }), + ); + vi.stubGlobal("fetch", fetchMock); + + const tool = getTool("paperclipApprovalDecision"); + await tool.execute({ + approvalId: "55555555-5555-5555-5555-555555555555", + action: "withdraw", + reason: "Superseded by PR #1190.", + }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const [url, init] = fetchMock.mock.calls[0] as [string, RequestInit]; + expect(String(url)).toBe( + "http://localhost:3100/api/approvals/55555555-5555-5555-5555-555555555555/withdraw", + ); + expect(init.method).toBe("POST"); + expect(JSON.parse(String(init.body))).toEqual({ reason: "Superseded by PR #1190." }); + }); + + it("falls back to decisionNote as the withdraw reason", async () => { + const fetchMock = vi.fn().mockResolvedValue( + mockJsonResponse({ id: "approval-1", status: "withdrawn" }), + ); + vi.stubGlobal("fetch", fetchMock); + + const tool = getTool("paperclipApprovalDecision"); + await tool.execute({ + approvalId: "55555555-5555-5555-5555-555555555555", + action: "withdraw", + decisionNote: "Question answered itself.", + }); + + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit]; + expect(JSON.parse(String(init.body))).toEqual({ reason: "Question answered itself." }); + }); + + it("prefers reason over decisionNote when a withdraw supplies both", async () => { + const fetchMock = vi.fn().mockResolvedValue( + mockJsonResponse({ id: "approval-1", status: "withdrawn" }), + ); + vi.stubGlobal("fetch", fetchMock); + + const tool = getTool("paperclipApprovalDecision"); + await tool.execute({ + approvalId: "55555555-5555-5555-5555-555555555555", + action: "withdraw", + reason: "Superseded by PR #1190.", + decisionNote: "stale note from an earlier draft", + }); + + // `reason` is the withdraw-specific field, so it wins; `decisionNote` is only + // a fallback. Pinning this keeps the precedence from silently inverting. + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit]; + expect(JSON.parse(String(init.body))).toEqual({ reason: "Superseded by PR #1190." }); + }); + + it("folds reason into decisionNote on non-withdraw actions so it is never dropped", async () => { + const fetchMock = vi.fn().mockResolvedValue(mockJsonResponse({ id: "approval-1" })); + vi.stubGlobal("fetch", fetchMock); + + const tool = getTool("paperclipApprovalDecision"); + await tool.execute({ + approvalId: "55555555-5555-5555-5555-555555555555", + action: "approve", + reason: "Looks good.", + }); + + // `reason` is advertised on the shared schema, so a board caller can reach for + // it on any action. Without the fold-back the note is silently elided by + // JSON.stringify and the server receives {}. + const [url, init] = fetchMock.mock.calls[0] as [string, RequestInit]; + expect(String(url)).toBe( + "http://localhost:3100/api/approvals/55555555-5555-5555-5555-555555555555/approve", + ); + expect(JSON.parse(String(init.body))).toEqual({ decisionNote: "Looks good." }); + }); + + it("refuses a withdraw with a blank reason instead of dropping it", async () => { const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + + const tool = getTool("paperclipApprovalDecision"); + const response = await tool.execute({ + approvalId: "55555555-5555-5555-5555-555555555555", + action: "withdraw", + reason: " ", + }); + + // Fails loudly and locally: the audit trail relies on the reason to tell a + // moot request from an abandoned one, so this must never reach the server + // as a withdrawal with no note. + expect(response.isError).toBe(true); + expect(response.content[0]?.text).toContain("withdraw requires a non-empty reason"); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("leaves requester and pending scoping to the server for withdraw", async () => { + // The route rejects withdrawing another agent's card (403) and the service + // rejects withdrawing an already-decided one (409). Pin that the tool sends + // no actor or status override that could widen either check. + const fetchMock = vi.fn().mockResolvedValue( + mockJsonResponse({ error: "Only requesting agent can withdraw this approval" }, 403), + ); + vi.stubGlobal("fetch", fetchMock); + + const tool = getTool("paperclipApprovalDecision"); + const response = await tool.execute({ + approvalId: "55555555-5555-5555-5555-555555555555", + action: "withdraw", + reason: "Not mine to withdraw.", + }); + + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit]; + expect(Object.keys(JSON.parse(String(init.body)))).toEqual(["reason"]); + expect(response.isError).toBe(true); + expect(response.content[0]?.text).toContain("Only requesting agent can withdraw this approval"); + }); + it("rejects invalid generic request paths", async () => { vi.stubGlobal("fetch", vi.fn()); diff --git a/packages/mcp-server/src/tools.ts b/packages/mcp-server/src/tools.ts index 5b532d630bdb..c14f398945f3 100644 --- a/packages/mcp-server/src/tools.ts +++ b/packages/mcp-server/src/tools.ts @@ -194,8 +194,15 @@ const createRequestCheckboxConfirmationToolSchema = z.object({ const approvalDecisionSchema = z.object({ approvalId: approvalIdSchema, - action: z.enum(["approve", "reject", "requestRevision", "resubmit"]), + action: z.enum(["approve", "reject", "requestRevision", "resubmit", "withdraw"]), decisionNote: z.string().optional(), + // `withdraw` and `resubmit` are both requester-scoped; only approve/reject/ + // requestRevision call assertBoard. The withdraw route additionally requires a + // non-empty reason, so accept a dedicated `reason` rather than making callers + // learn that `decisionNote` is overloaded. `decisionNote` is still read as a + // fallback, and on the non-withdraw path `reason` folds back into + // `decisionNote` so a note can never be silently dropped either way. + reason: z.string().optional(), payloadJson: z.string().optional(), }); @@ -729,9 +736,26 @@ export function createToolDefinitions(client: PaperclipApiClient): ToolDefinitio ), makeTool( "paperclipApprovalDecision", - "Approve, reject, request revision, or resubmit an approval", + "Approve, reject, request revision, resubmit, or withdraw an approval. `approve`, `reject`, and `requestRevision` are board-only — an agent calling them gets `403 Board access required`. `withdraw` and `resubmit` are **both requester-scoped**: the requesting agent may rescind its own ask or resubmit it, so a card that went moot is yours to clear rather than something to ask a human to close, and a card the board sent back as `revision_requested` is yours to resubmit. You can only act on cards you filed, and only while they are still pending; acting on another agent's card is refused (403), as is acting on one already decided (409). `withdraw` requires a non-empty `reason` (or `decisionNote`) — the audit trail relies on it to tell a moot request apart from an abandoned one. Note one destructive side effect: withdrawing a `hire_agent` approval also terminates the pending agent it would have created (it would otherwise be stranded frozen with no approval left to decide it).", approvalDecisionSchema, - async ({ approvalId, action, decisionNote, payloadJson }) => { + async ({ approvalId, action, decisionNote, reason, payloadJson }) => { + if (action === "withdraw") { + // Refuse here rather than letting an empty reason reach the server as a + // bare 400: the caller learns which field to fill, and a withdrawal can + // never silently lose the note the audit trail depends on. + const withdrawReason = (reason ?? decisionNote ?? "").trim(); + if (!withdrawReason) { + throw new Error( + "withdraw requires a non-empty reason: pass `reason` (or `decisionNote`) saying why the request became moot", + ); + } + return client.requestJson( + "POST", + `/approvals/${encodeURIComponent(approvalId)}/withdraw`, + { body: { reason: withdrawReason } }, + ); + } + const path = action === "approve" ? `/approvals/${encodeURIComponent(approvalId)}/approve` @@ -747,7 +771,7 @@ export function createToolDefinitions(client: PaperclipApiClient): ToolDefinitio ? replacementPayload === undefined ? {} : { payload: replacementPayload } - : { decisionNote }; + : { decisionNote: decisionNote ?? reason }; return client.requestJson("POST", path, { body }); }, diff --git a/skills/paperclip/SKILL.md b/skills/paperclip/SKILL.md index 6683f02334e4..5eab47a347e7 100644 --- a/skills/paperclip/SKILL.md +++ b/skills/paperclip/SKILL.md @@ -251,6 +251,23 @@ POST /api/companies/{companyId}/approvals `issueIds` links the approval into the issue thread. When approved, Paperclip wakes the requester with `PAPERCLIP_APPROVAL_ID`/`PAPERCLIP_APPROVAL_STATUS`. Keep the payload concise and decision-ready. +### Withdrawing an approval you filed + +If a card you filed goes moot — the work landed another way, the question answered itself, the ask was wrong — **withdraw it yourself**. Do not comment asking the board to close it: a pending approval sits in a human's queue until someone acts on it, and the retraction comment costs them a read on top of the card. + +```json +POST /api/approvals/{approvalId}/withdraw +{ "reason": "Superseded by PR #1190, which landed the same patch on 08-09." } +``` + +Or via MCP: `paperclipApprovalDecision` with `action: "withdraw"` and a `reason`. + +Scope: **the requesting agent, on its own still-pending card.** Withdrawing another agent's card is refused (403); withdrawing one the board already decided is refused (409). `reason` is required and must be non-empty — the audit trail uses it to tell a moot request apart from an abandoned one. + +One destructive side effect to know before you reach for this: withdrawing a `hire_agent` approval **also terminates the pending agent it would have created**. That is deliberate — the agent is parked in `pending_approval` and would otherwise be stranded frozen with no approval left to decide it — but it is not obvious from the word "withdraw". Requester-scoping means you can only ever terminate a hire you filed yourself. + +`resubmit` is scoped the same way: if the board sends your card back as `revision_requested`, **you resubmit it yourself** — that is not a board-only action either. Only `approve`, `reject`, and `requestRevision` are board-only and return `403 Board access required` for agents. That 403 is about those three actions, not about approvals generally — it does not mean you cannot retract or resubmit your own ask. + ## Issue-Thread Interactions Issue-thread interactions are first-class cards that render in the issue thread and capture a typed board/user response. Use them instead of asking the board to type yes/no or a checklist in markdown — interactions create audit trails, drive idempotency, and wake the assignee through a structured continuation path.