From 8a6b08227484591a30a7adea3840f7e96755ed81 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 14:03:08 -0600 Subject: [PATCH] fix(api): send call_api mutations once when responseText asks only for the first window (#20344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Native agents use the `call_api` fallback tool for API operations that have no dedicated tool. > - `responseText` is an optional `call_api` argument. It returns a bounded text window of a GET response. > - The server rejects every `responseText` on a mutation, also a first-window request that cannot repeat the mutation. > - A native agent attached `responseText` to a company rename PATCH. It got the same 400 on each retry, and no call succeeded. > - The same call also had no `body`. A PATCH without a body changes nothing, but the route returns 200. With the first rejection removed, the agent could read that 200 as a completed rename. > - This pull request sends a mutation once when `responseText` asks only for the first window, and it rejects a PATCH without a body before dispatch. > - The benefit is that a valid mutation goes through on the first call, a later-page request still cannot repeat a mutation, and an empty PATCH cannot look like a successful update. ## Linked Issues or Issue Description Refs #18105. That open pull request keeps the 400 for all mutation `responseText` values and corrects its message. This pull request changes the same check. It accepts a first-window request and keeps a corrected 400 for a later page. Whichever pull request merges second must resolve this line. **What happened** A chat agent (OpenCode, `deepseek-v4.1-flash`) tried to rename its company with `call_api`. We recovered the exact arguments from the payload digest in the run events: `operationId: "PATCH /api/companies/{companyId}"`, `pathParams.companyId`, `query: {}` and `responseText: { limitBytes: 8192 }`. There was no `body`. The OpenCode MCP bridge validates the arguments with Ajv without `useDefaults`, and `8192` is not a default in any schema. Thus the model, not the bridge, sent `responseText`. The server returned "responseText requires a GET operation". The agent sent the identical call 10 more times and one identical branding PATCH. No call succeeded. A second chat case in the same cohort sent `responseText` on `POST /api/issues/{id}/inbox-archive` three times before it recovered. **Expected behavior** `responseText` with no offset or offset 0 changes only how the response is shown. The mutation must run once and return its response on the normal inline or saved-artifact path. A later page (`offsetBytes > 0`) must still fail before dispatch, because it would repeat the mutation. A PATCH without a body must fail before dispatch with a message that names `body`, the allowed fields and the received argument keys. **Steps to reproduce** 1. Call `executeRunnerApi` with `PATCH /api/companies/{companyId}`, a body, and `responseText: { limitBytes: 8192 }`. 2. Before this change: a 400, and no HTTP request. After: one PATCH with the body, and the normal response. 3. Call it with no body. Before: the request is sent and the route returns 200 with no change. After: a 400 that names `body`, and no HTTP request. ## What Changed - `validateRunnerApiCall`: for a non-GET operation, drop a first-window `responseText` (no offset or offset 0) and continue. A later page still returns 400 with "This request was not sent" and the reason. - New `assertRunnerApiDispatchable`: reject a new PATCH with a required request body when `body` and `files` are both absent. The message lists up to 12 allowed fields and the received argument keys. `call_api` runs it inside its receipt reservation, after saved and inherited receipts are read and before it writes a pending receipt, so a rejected call leaves no receipt. `executeRunnerApi` also runs it for direct callers. Recovery and receipt replay use only `validateRunnerApiCall`, so a body-less PATCH that an older server completed still returns its saved result. - Tests: first-window `responseText` on a PATCH dispatches once with the body (three window shapes). A later page is rejected with no dispatch. A body-less PATCH is rejected with no dispatch, and `body: {}` is still accepted. The catalog-wide dispatch tests now send `body: {}` for PATCH operations with a required body. ## Verification - Red: the 5 new or changed cases failed on `origin/master` (`runner-api.test.ts`). - Green: `npx vitest run src/services/native-runtime/runner-api.test.ts src/services/native-runtime/runner-tool-results.test.ts` passed (2,087 tests). `tests/runner-e2e/api-response-reading.test.ts` passed (14 tests). - `tsc --noEmit -p server/tsconfig.json`: no new errors. The error count is the same before and after this change; the remaining errors come from packages that are not built in this worktree. - Upgrade safety and receipts: a new integration test seeds a completed receipt for a body-less PATCH. `recover()` and a same-callId `execute()` return the saved result. A new body-less PATCH is rejected twice with the same error, writes no receipt and sends no HTTP request, and the corrected PATCH with that callId then succeeds. The test failed with the rule in `validateRunnerApiCall`, and again with the rule after the reservation; it passes now. `runner-api.integration.test.ts`, `runner-api.test.ts` and `native-case-tools.integration.test.ts` pass (2,164 passed, 2 skipped). - One paid chat eval run (GitHub Actions run 42680925312) with the first commit cherry-picked onto the eval branch: `companies.rename-company` passed. The agent sent one correct PATCH with a body, attributed to the agent. That run did not send `responseText`, so it does not exercise the changed path; the unit tests cover it. ## Risks - Behavior change: a mutation with a first-window `responseText` now runs. Before, it failed. The response comes back on the normal path (inline up to 24 KiB, else a saved artifact), not as a text window. - Behavior change: a caller that sent a PATCH with no body to a route with a required body now gets a 400. Such a call changed nothing before. POST, PUT and DELETE are not changed, because some of them have useful empty bodies. - No route, permission or company check changes. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), with tool use and code execution in Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .../paperclip-runner-tool-authority.ts | 5 ++- .../native-runtime/runner-api-client.ts | 32 +++++++++++++++++-- .../runner-api.integration.test.ts | 32 +++++++++++++++++++ .../native-runtime/runner-api.test.ts | 26 +++++++++++++-- 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/server/src/services/native-runtime/paperclip-runner-tool-authority.ts b/server/src/services/native-runtime/paperclip-runner-tool-authority.ts index 832581b6030..ecf9cfdcf07 100644 --- a/server/src/services/native-runtime/paperclip-runner-tool-authority.ts +++ b/server/src/services/native-runtime/paperclip-runner-tool-authority.ts @@ -72,7 +72,7 @@ import { runnerApiSensitiveResponse } from "./runner-api-sensitive-response.js"; import { workspaceFileResourceService } from "../workspace-file-resources.js"; import { badRequest, forbidden, notFound, HttpError } from "../../errors.js"; import { runnerApiCatalog, searchRunnerApi } from "./runner-api-catalog.js"; -import { executeRunnerApi, validateRunnerApiCall, RUNNER_API_MAX_BYTES, type RunnerApiCall, type RunnerApiFile, type RunnerApiIo } from "./runner-api-client.js"; +import { assertRunnerApiDispatchable, executeRunnerApi, validateRunnerApiCall, RUNNER_API_MAX_BYTES, type RunnerApiCall, type RunnerApiFile, type RunnerApiIo } from "./runner-api-client.js"; import { acquireRunnerApiResponseSlot, runnerApiCompanyCaptureMaxBytes, RUNNER_API_RESPONSE_MAX_BYTES, RUNNER_API_RESPONSE_RUN_MAX_BYTES, RunnerApiResponseLimitError } from "./runner-api-response-limits.js"; import { and, desc, eq, inArray, isNull, notInArray, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; @@ -1171,6 +1171,9 @@ export class PaperclipRunnerToolAuthority { const result = await this.#apiReceiptResult(existing); return { result: existing.encryptedResult ? result : result ?? { ok: false, status: null, error: "api_outcome_unknown", outcome: "unknown", guidance: "Inspect state before issuing another mutation." } }; } + // A new call that would be rejected before dispatch must not reserve a + // pending receipt; that would make its callId report an unknown outcome. + assertRunnerApiDispatchable(operation, input, Object.keys(record(value))); if (Object.keys(receipts).length >= 512) throw badRequest("Run API mutation limit reached"); receipts[key] = { digest, operationId: operation.operationId, state: "pending" }; await tx.update(heartbeatRuns).set({ resultJson: { ...resultJson, apiToolReceipts: receipts } }).where(eq(heartbeatRuns.id, this.binding.runId)); diff --git a/server/src/services/native-runtime/runner-api-client.ts b/server/src/services/native-runtime/runner-api-client.ts index ecce7755c8f..256831b8f3f 100644 --- a/server/src/services/native-runtime/runner-api-client.ts +++ b/server/src/services/native-runtime/runner-api-client.ts @@ -52,10 +52,17 @@ export interface RunnerApiIo { export function validateRunnerApiCall(value: unknown, context: RunnerApiContext): { input: RunnerApiCall; operation: RunnerApiOperation } { const parsed = runnerApiCallSchema.safeParse(value); if (!parsed.success) throw badRequest("Invalid call_api arguments", { issues: parsed.error.issues }); - const input = parsed.data; + let input = parsed.data; const operation = runnerApiOperation(input.operationId); if (operation.transport !== "rest") throw unprocessable(`This endpoint requires its existing protocol client; call_api supports REST only and no other API route substitutes for it.${operation.description.trim() ? " Read its catalog description for details." : ""}`); - if (input.responseText && operation.method !== "GET") throw badRequest("responseText requires a GET operation; read the saved artifact instead of repeating a mutation"); + if (input.responseText && operation.method !== "GET") { + // A later page of a mutation response would need the mutation repeated. + // A first-window request changes only presentation, so the mutation runs + // once and its response is returned through the normal inline/artifact path. + if ((input.responseText.offsetBytes ?? 0) > 0) throw badRequest("This request was not sent: responseText.offsetBytes applies only to GET reads, and a later page of a mutation response would repeat the mutation. Remove responseText and send the mutation once; read any saved response artifact with GET /api/assets/{assetId}/content."); + const { responseText: _ignored, ...rest } = input; + input = rest; + } if (!operation.allowedModes.includes(context.workMode)) throw forbidden("call_api permits only reads in Ask and Plan modes; use the permitted dedicated tools"); if (input.pathParams?.companyId && input.pathParams.companyId !== context.companyId) throw forbidden("API call belongs to another company"); const contentType = (input.contentType ?? "application/json").split(";", 1)[0].trim().toLowerCase(); @@ -71,6 +78,26 @@ export function validateRunnerApiCall(value: unknown, context: RunnerApiContext) return { input, operation }; } +/** + * Rules for a new dispatch only. Recovery and saved receipts still validate an + * older accepted call with validateRunnerApiCall, so these rules stay out of it. + * Callers that reserve a mutation receipt must run this before the reservation. + */ +export function assertRunnerApiDispatchable(operation: RunnerApiOperation, input: RunnerApiCall, receivedKeys: readonly string[]): void { + if (operation.method === "PATCH" && input.body === undefined && !input.files?.length && operation.requestBody?.required) { + // A PATCH without a body changes nothing but still returns success, which + // reads as a completed update. Name the missing argument instead. + throw badRequest(`This request was not sent: this PATCH requires body, a JSON object with the fields to change${patchFieldHint(operation)}. Received arguments: ${[...receivedKeys].sort().join(", ")}.`); + } +} + +function patchFieldHint(operation: RunnerApiOperation): string { + const schema = operation.requestBody?.content?.["application/json"]?.schema as { properties?: Record } | undefined; + const fields = Object.keys(schema?.properties ?? {}); + if (!fields.length) return ""; + return ` (allowed fields: ${fields.slice(0, 12).join(", ")}${fields.length > 12 ? ", ..." : ""})`; +} + export function runnerApiUrl(operation: RunnerApiOperation, input: RunnerApiCall, context: RunnerApiContext, apiUrl: string): URL { const origin = new URL(apiUrl); if (!["http:", "https:"].includes(origin.protocol) || origin.username || origin.password || origin.search || origin.hash) throw new Error("Invalid configured Paperclip API origin"); @@ -152,6 +179,7 @@ export function runnerApiPagination(headers: Headers) { export async function executeRunnerApi(input: RunnerApiCall, context: RunnerApiContext, io: RunnerApiIo) { const inlineBytes = RUNNER_API_INLINE_BYTES; const { operation, input: validated } = validateRunnerApiCall(input, context); + assertRunnerApiDispatchable(operation, validated, Object.keys(input)); input = validated; const sensitiveResponse = runnerApiSensitiveResponse(operation.method, operation.path); const url = runnerApiUrl(operation, input, context, io.apiUrl); diff --git a/server/src/services/native-runtime/runner-api.integration.test.ts b/server/src/services/native-runtime/runner-api.integration.test.ts index 3093f5f8be2..22d4c31b504 100644 --- a/server/src/services/native-runtime/runner-api.integration.test.ts +++ b/server/src/services/native-runtime/runner-api.integration.test.ts @@ -554,6 +554,38 @@ else if(m.id!==undefined) send({id:m.id,result:{}}); expect(await effects()).toEqual(before); }); + it("recovers a body-less PATCH saved before the body rule while rejecting a new one before dispatch", async () => { + const fixture = await server.fixture(); + const canonical = (value: unknown): string => Array.isArray(value) ? `[${value.map(canonical).join(",")}]` + : value && typeof value === "object" ? `{${Object.keys(value).sort().map(key => `${JSON.stringify(key)}:${canonical((value as Record)[key])}`).join(",")}}` + : JSON.stringify(value) ?? "null"; + const sha = (value: string) => createHash("sha256").update(value).digest("hex"); + const bodyless = { operationId: "PATCH /api/projects/{id}", pathParams: { id: fixture.projectId } }; + const call = { tool: "call_api", callId: "saved-bodyless-patch", arguments: bodyless }; + // An older server accepted and completed this call; seed its receipt from a real completed PATCH. + const result = await fixture.authority.execute({ ...call, arguments: { ...bodyless, body: { name: "Saved before upgrade" } } }); + expect(result).toMatchObject({ ok: true, status: 200 }); + const [run] = await server.db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, fixture.runId)); + const receipts = structuredClone((run.resultJson as Record).apiToolReceipts); + receipts[sha(call.callId)].digest = sha(canonical(bodyless)); + await server.db.update(heartbeatRuns).set({ resultJson: { ...run.resultJson, apiToolReceipts: receipts } }).where(eq(heartbeatRuns.id, fixture.runId)); + const reopened = new PaperclipRunnerToolAuthority(server.db, fixture.authority.binding); + const network = vi.spyOn(globalThis, "fetch"); + try { + expect(await reopened.recover(call)).toEqual({ result }); + expect(await reopened.execute(call)).toEqual(result); + const rejected = { ...call, callId: "new-bodyless-patch" }; + await expect(reopened.execute(rejected)).rejects.toThrow(/not sent.*requires body/); + await expect(reopened.execute(rejected)).rejects.toThrow(/not sent.*requires body/); + expect(network).not.toHaveBeenCalled(); + } finally { network.mockRestore(); } + // The rejected call reserved no receipt, so its callId stays usable for the corrected call. + const [after] = await server.db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, fixture.runId)); + expect(Object.keys((after.resultJson as Record).apiToolReceipts)).not.toContain(sha("new-bodyless-patch")); + expect(await reopened.execute({ tool: "call_api", callId: "new-bodyless-patch", arguments: { ...bodyless, body: { name: "Corrected" } } })) + .toMatchObject({ ok: true, status: 200, data: { name: "Corrected" } }); + }); + it("returns and replays scoped secret values without plaintext receipts or response assets", async () => { const fixture = await server.fixture(); const value = "disposable-private-credential-".repeat(1500); diff --git a/server/src/services/native-runtime/runner-api.test.ts b/server/src/services/native-runtime/runner-api.test.ts index a3401d5c953..b08b92c611d 100644 --- a/server/src/services/native-runtime/runner-api.test.ts +++ b/server/src/services/native-runtime/runner-api.test.ts @@ -275,7 +275,8 @@ describe("runner API request boundary", () => { [name, name === "companyId" ? context.companyId : "fixture"]), ); const result = await executeRunnerApi( - { operationId: operation.operationId, pathParams }, context, io(request), + // A body-less PATCH is rejected before dispatch; send an empty update. + { operationId: operation.operationId, pathParams, ...(operation.method === "PATCH" && operation.requestBody?.required ? { body: {} } : {}) }, context, io(request), ); expect(operation.callPolicy).toBe("rest"); expect(result).toMatchObject({ ok: false, status: 403, data: { error: "Route permission required" } }); @@ -326,7 +327,7 @@ describe("runner API request boundary", () => { ); await expect( executeRunnerApi( - { operationId: operation.operationId, pathParams }, + { operationId: operation.operationId, pathParams, ...(operation.method === "PATCH" ? { body: { status: "resolved" } } : {}) }, context, io(request), ), @@ -759,7 +760,26 @@ describe("runner API request boundary", () => { }); it("does not page mutation responses by repeating the mutation", async () => { const request = vi.fn(); - await expect(executeRunnerApi({ operationId: createProject, body: { name: "Once" }, responseText: {} }, context, io(request))).rejects.toThrow("GET"); + await expect(executeRunnerApi({ operationId: createProject, body: { name: "Once" }, responseText: { offsetBytes: 4 } }, context, io(request))).rejects.toThrow("This request was not sent"); + expect(request).not.toHaveBeenCalled(); + }); + it.each([{}, { limitBytes: 8192 }, { offsetBytes: 0, limitBytes: 4096 }])("sends a mutation once when responseText asks only for the first window: %j", async responseText => { + const request = vi.fn(async () => Response.json({ id: "company-a", name: "Acorn Studio" })); + const result = await executeRunnerApi({ operationId: "PATCH /api/companies/{companyId}", pathParams: { companyId: "company-a" }, body: { name: "Acorn Studio" }, responseText }, context, io(request)); + expect(request).toHaveBeenCalledTimes(1); + const [, init] = request.mock.calls[0]!; + expect(init?.method).toBe("PATCH"); + expect(JSON.parse(String(init?.body))).toEqual({ name: "Acorn Studio" }); + expect(result).toMatchObject({ ok: true, status: 200, data: { name: "Acorn Studio" } }); + expect(result).not.toHaveProperty("responseText"); + }); + it("rejects a PATCH with no body instead of sending a no-op update", async () => { + const request = vi.fn(); + const call = { operationId: "PATCH /api/companies/{companyId}", pathParams: { companyId: "company-a" }, query: {}, responseText: { limitBytes: 8192 } }; + await expect(executeRunnerApi(call, context, io(request))).rejects.toThrow(/not sent.*requires body.*name.*Received arguments: operationId, pathParams, query, responseText/); + expect(() => validateRunnerApiCall({ operationId: "PATCH /api/companies/{companyId}", pathParams: { companyId: "company-a" }, body: {} }, context)).not.toThrow(); + // Recovery validates saved calls without dispatch: an older accepted body-less PATCH stays readable. + expect(() => validateRunnerApiCall({ operationId: "PATCH /api/companies/{companyId}", pathParams: { companyId: "company-a" } }, context)).not.toThrow(); expect(request).not.toHaveBeenCalled(); }); it("rejects binary, invalid UTF-8 and offsets that split text", async () => {