From 3e09110fb9f08d4fb35e708ac6c6760ffbe95136 Mon Sep 17 00:00:00 2001 From: CTO Date: Sat, 15 Aug 2026 19:38:04 +0000 Subject: [PATCH 1/2] feat(mcp): expose requester-scoped approval withdraw on paperclipApprovalDecision (BLO-27534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /approvals/:id/withdraw` has been requester-scoped since f31c54513 (BLO-19079), but `paperclipApprovalDecision` only ever offered approve / reject / requestRevision / resubmit. Two of those are board-only, so an agent's whole observable experience of the tool was `403 Board access required` — and the reasonable, wrong inference is "agents cannot terminate approvals". The one action an agent is authorized to take was the only one the tool did not expose. The cost is not hypothetical: approval ce343617 sat pending in a human's queue for 3.3 days carrying three separate retraction comments, two from the CEO and one from me, each asking a human to close a card either of us could have withdrawn. - `action: "withdraw"` routes to the withdraw endpoint. - A `reason` field is added; `decisionNote` is still read as a fallback so existing callers keep working. An empty reason is refused at the tool boundary rather than sent on as a bare 400 — the audit trail relies on it to tell a moot request apart from an abandoned one. - The description now says who may use it, mirroring the wording already carried by `paperclipWithdrawInteraction`. - `skills/paperclip/SKILL.md` gains a "Withdrawing an approval you filed" note: the false belief propagated through the docs as much as the tool. Scoping is unchanged and stays server-side — the route refuses another agent's card (403) and the service refuses an already-decided one (409). The added tests pin that the tool widens neither, and that it sends `reason` and nothing else. One correction to the issue's framing: it describes all four existing actions as board-gated. `resubmit` is not — approvals.ts:~497 scopes it to the requester exactly as withdraw is. Only approve / reject / request-revision call `assertBoard`, so the tool description names those three. Refs BLO-27534 Co-Authored-By: Claude --- packages/mcp-server/src/tools.test.ts | 80 +++++++++++++++++++++++++++ packages/mcp-server/src/tools.ts | 28 +++++++++- skills/paperclip/SKILL.md | 15 +++++ 3 files changed, 120 insertions(+), 3 deletions(-) diff --git a/packages/mcp-server/src/tools.test.ts b/packages/mcp-server/src/tools.test.ts index 01773dbe33b3..d1b191e7f176 100644 --- a/packages/mcp-server/src/tools.test.ts +++ b/packages/mcp-server/src/tools.test.ts @@ -546,6 +546,86 @@ 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("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..19a108f2a1b0 100644 --- a/packages/mcp-server/src/tools.ts +++ b/packages/mcp-server/src/tools.ts @@ -194,8 +194,13 @@ 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(), + // Withdrawal is the only action here an agent is authorized to take, and its + // route requires a non-empty reason. Accept a dedicated `reason` so callers do + // not have to learn that `decisionNote` is overloaded, but keep reading + // `decisionNote` as a fallback for callers that already do. + reason: z.string().optional(), payloadJson: z.string().optional(), }); @@ -729,9 +734,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` is the action an agent has: the requesting agent may rescind its own ask, so a card that went moot is yours to clear rather than something to ask a human to close. You can only withdraw cards you filed, and only while they are still pending; withdrawing another agent's card is refused (403), as is withdrawing 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.", 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` diff --git a/skills/paperclip/SKILL.md b/skills/paperclip/SKILL.md index 6683f02334e4..0bfee3c2b12a 100644 --- a/skills/paperclip/SKILL.md +++ b/skills/paperclip/SKILL.md @@ -251,6 +251,21 @@ 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. + +The other actions on that tool (`approve`, `reject`, `requestRevision`) are board-only and return `403 Board access required` for agents. That 403 is about those actions, not about approvals generally — it does not mean you cannot retract 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. From 557064675f8d999dfe7248c3c391c31239de5377 Mon Sep 17 00:00:00 2001 From: CTO Date: Sun, 16 Aug 2026 23:28:36 +0000 Subject: [PATCH 2/2] fix(mcp): correct resubmit scoping claim and document hire_agent withdraw side effect (BLO-27534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both Important findings from Ally's review of #1376. 1. The tool description and its code comment claimed withdraw is "the only action here an agent is authorized to take". That is false: `resubmit` is scoped identically (server/src/routes/approvals.ts:518-527, no assertBoard). Only approve/reject/requestRevision call assertBoard (:443,:467,:494). The wrong copy re-created this PR's own bug class one action over — an agent whose card came back `revision_requested` would read "withdraw is the action an agent has" and ask a human to resubmit. Both surfaces now say withdraw and resubmit are requester-scoped; SKILL.md's list of board-only actions no longer reads as exhaustive while omitting resubmit. 2. Withdrawing a `hire_agent` approval also terminates the bound pending agent (server/src/services/approvals.ts:615-617). The server behavior is correct, and requester-scoping means an agent can only terminate a hire it filed itself — but it is a destructive, non-obvious consequence of an action this PR newly promotes as safe self-service. Documented on both surfaces. Also the two suggestions: - `reason` was advertised on the shared schema but consumed only on the withdraw branch, so `reason` on approve/reject was silently elided by JSON.stringify and the server received {}. Now folds back as `decisionNote ?? reason`. - Pinned `reason`-wins precedence when both fields are supplied, and pinned the fold-back. Both verified non-vacuous by reverting the source and re-running. 71 tests pass across 6 files; tsc --noEmit clean. --- packages/mcp-server/src/tools.test.ts | 44 +++++++++++++++++++++++++-- packages/mcp-server/src/tools.ts | 14 +++++---- skills/paperclip/SKILL.md | 4 ++- 3 files changed, 53 insertions(+), 9 deletions(-) diff --git a/packages/mcp-server/src/tools.test.ts b/packages/mcp-server/src/tools.test.ts index d1b191e7f176..2e03b5914680 100644 --- a/packages/mcp-server/src/tools.test.ts +++ b/packages/mcp-server/src/tools.test.ts @@ -585,8 +585,48 @@ describe("paperclip MCP tools", () => { expect(JSON.parse(String(init.body))).toEqual({ reason: "Question answered itself." }); }); - it("refuses a withdraw with a blank reason instead of dropping it", async () => { - const fetchMock = vi.fn(); + 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"); diff --git a/packages/mcp-server/src/tools.ts b/packages/mcp-server/src/tools.ts index 19a108f2a1b0..c14f398945f3 100644 --- a/packages/mcp-server/src/tools.ts +++ b/packages/mcp-server/src/tools.ts @@ -196,10 +196,12 @@ const approvalDecisionSchema = z.object({ approvalId: approvalIdSchema, action: z.enum(["approve", "reject", "requestRevision", "resubmit", "withdraw"]), decisionNote: z.string().optional(), - // Withdrawal is the only action here an agent is authorized to take, and its - // route requires a non-empty reason. Accept a dedicated `reason` so callers do - // not have to learn that `decisionNote` is overloaded, but keep reading - // `decisionNote` as a fallback for callers that already do. + // `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(), }); @@ -734,7 +736,7 @@ export function createToolDefinitions(client: PaperclipApiClient): ToolDefinitio ), makeTool( "paperclipApprovalDecision", - "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` is the action an agent has: the requesting agent may rescind its own ask, so a card that went moot is yours to clear rather than something to ask a human to close. You can only withdraw cards you filed, and only while they are still pending; withdrawing another agent's card is refused (403), as is withdrawing 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.", + "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, reason, payloadJson }) => { if (action === "withdraw") { @@ -769,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 0bfee3c2b12a..5eab47a347e7 100644 --- a/skills/paperclip/SKILL.md +++ b/skills/paperclip/SKILL.md @@ -264,7 +264,9 @@ 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. -The other actions on that tool (`approve`, `reject`, `requestRevision`) are board-only and return `403 Board access required` for agents. That 403 is about those actions, not about approvals generally — it does not mean you cannot retract your own ask. +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