From a00109aa3593b683e5b6e19bf174a6e34bb2d99d Mon Sep 17 00:00:00 2001 From: "commitperclip[bot]" <284812584+commitperclip[bot]@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:43:24 -0800 Subject: [PATCH] fix: Preserve sanitized ACP JSON-RPC error details and numeric codes in run errors, summaries, and acpx.error records (#18087) > [!NOTE] > Opened by Prospector's automated issue-fix pipeline. A maintainer reviews and decides; nothing here merges on its own. > [!WARNING] > This change crosses a trust boundary. See Risks. ## Thinking Path - Issue #16034 reports a defect in paperclipai/paperclip. - Root cause, as the fixing agent read it: emitAcpxFailure in packages/adapter-utils/src/acpx-engine/execute.ts built the run error, summary, and acpx.error message from Error.message alone. It ignored the JSON-RPC error's 'data' explanation and numeric 'code', so every ACP phase failure (create runtime, ensure session, configure session, turn) showed only 'Internal error'. - This pull request: Preserve sanitized ACP JSON-RPC error details and numeric codes in run errors, summaries, and acpx.error records ## What Changed - `packages/adapter-utils/src/acpx-engine/execute.ts`: This change is kept from the earlier attempt. The shared failure handler reads the explanation from a string 'data', 'data.details', or 'data.message'. It appends that text to the message unless the message already contains it, and runs the result through sanitizeTerminalSessionFailure. A numeric JSON-RPC code is recorded as errorMeta.rpcCode. Errors without these fields keep their old message. - `packages/adapter-utils/src/acpx-engine/execute.test.ts`: The existing harness keeps the earlier it.each case, which covers all four phases, all three detail shapes, redaction, and rpcCode in the acpx.error record. A new case in the same harness covers the checks that only the separate file had: detail text already in the message is not repeated, and a plain Error stays unchanged with no rpcCode. - `doc/run-log-events.md`: Documents the change to the acpx.error record: which detail formats are read and in what order, the rule against repeating detail text, the redaction and 4,096-character title limit applied through the existing sanitizer, and the optional integer 'rpcCode' field. ## Verification - Reproducing test(s), added in this PR: `packages/adapter-utils/src/acpx-engine/execute.test.ts` - They fail on the base commit and pass with the fix, each twice in a fresh sandbox. - The repository's compile command passes with the fix applied. - 9 existing test file(s) near the change passed. - The full test suite passes except for the 11 file(s) that already fail on the base commit. ## Risks - Written by an automated pipeline, not a person: review it as you would an outside contribution. - Risk tier of the touched paths: 2. - The scope reviewer found it also changes, beyond the report: Plain Error, non-object throw, string or Node errors with string codes (ENOENT), and data that is empty, missing, non-string or malformed - The scope reviewer found it also changes, beyond the report: messageOverride callers (turn timeout, handshake guard) - It crosses a trust boundary. Check each crossing against what the issue asked for, and against who asked: - data-exposure (packages/adapter-utils/src/acpx-engine/execute.ts:3626): Text the ACP child process controls (JSON-RPC error 'data', 'data.details' or 'data.message') is now added to the run error message and summary that users see; it passes through sanitizeTerminalSessionFailure redaction first. - logging (packages/adapter-utils/src/acpx-engine/execute.ts:3651): The 'acpx.error' run-log record now carries the provider's explanation in 'message' and a new integer 'rpcCode' field, both supplied by the child, on every failure route that calls emitAcpxFailure without a messageOverride, including the handshake-guard route that suppresses stderr. - data-exposure (packages/adapter-utils/src/acpx-engine/execute.ts:3639): The numeric JSON-RPC error code from the child is now copied into 'classified.errorMeta.rpcCode', which reaches the adapter result's errorMeta and the places that store or show it. ## Model Used - opus (Anthropic Claude, via Claude Code), writing and checking the change in an isolated sandbox. - opus (Anthropic Claude, via Claude Code), reviewing it independently. ## Linked Issues Fixes #16034 A search of paperclipai/paperclip's open and closed pull requests found no other pull request that names #16034. ## 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) - [ ] 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) - [ ] 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 - [ ] 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 - [ ] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: commitperclip[bot] <284812584+commitperclip[bot]@users.noreply.github.com> --- doc/run-log-events.md | 10 +++ .../src/acpx-engine/execute.test.ts | 85 +++++++++++++++++++ .../adapter-utils/src/acpx-engine/execute.ts | 15 +++- 3 files changed, 109 insertions(+), 1 deletion(-) diff --git a/doc/run-log-events.md b/doc/run-log-events.md index 33648bb8ef0..12bbc26e666 100644 --- a/doc/run-log-events.md +++ b/doc/run-log-events.md @@ -485,6 +485,16 @@ byte budgets, including truncation markers. `retrievalTruncated` directs callers to the full adapter-bounded text in the run error or transcript. Other provider metadata and action payloads are not copied. +When an ACP request fails with a JSON-RPC error, the engine also keeps the +provider explanation. It reads a string `data`, or the string `data.details`, or +the string `data.message`, in that order. It adds the explanation on a new line +after the error message unless the message already contains it. The combined +text passes through the same redaction and the 4,096-character title bound +before it becomes the run error, the summary, and the `acpx.error` `message`. +A numeric JSON-RPC error code appears as the optional integer `rpcCode` in +`errorMeta` and in the `acpx.error` record. Errors without these fields keep +their existing message and omit `rpcCode`. + Recovery still uses the typed failure category and the adapter's existing classifier. Provider warnings do not become failures, and timeouts or lost control channels keep their authoritative failure messages. These diagnostics diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index d11ca17b494..c480b9fa002 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -2537,6 +2537,91 @@ describe("shared ACPX engine runtime behavior", () => { ).toBe("second"); }); + it.each(["create_runtime", "ensure_session", "configure_session", "turn"])( + "preserves sanitized JSON-RPC diagnostics when %s fails", + async (phase) => { + for (const shape of ["string", "details", "message"]) { + const root = await makeTempRoot(); + const detail = "Invalid session configuration: model_context_window must be a number; private-setting-value"; + const data = shape === "string" ? detail : { [shape]: detail }; + const err = Object.assign(new Error("Internal error"), { code: -32603, data }); + const fail = () => { throw err; }; + const logs: string[] = []; + const execute = createAcpxEngineExecutor({ + createRuntime: () => { + if (phase === "create_runtime") fail(); + return { + ...buildRuntime(), + ...(phase === "ensure_session" ? { ensureSession: fail } : {}), + ...(phase === "configure_session" ? { setConfigOption: fail } : {}), + ...(phase === "turn" ? { startTurn: () => ({ + events: (async function* () { fail(); })(), + result: Promise.resolve({ status: "completed", stopReason: "end_turn" }), + cancel: async () => {}, + }) } : {}), + } as never; + }, + }); + const result = await execute({ + runId: "run-1", + agent: { id: "agent-1", companyId: "company-1" }, + runtime: {}, + config: { + agent: "custom", + agentCommand: "node ./fake-acp.js", + stateDir: path.join(root, "state"), + thinkingEffort: "high", + env: { PRIVATE_SETTING: "private-setting-value" }, + }, + context: {}, + onLog: async (_stream: string, text: string) => { logs.push(text); }, + onMeta: async () => {}, + } as never); + expect(result.exitCode).toBe(1); + expect(result.errorMessage).toContain("Internal error\nInvalid session configuration: model_context_window must be a number;"); + expect(result.errorMessage).not.toContain("private-setting-value"); + expect(result.summary).toBe(result.errorMessage); + expect(result.errorMeta).toMatchObject({ phase, rpcCode: -32603 }); + const payload = JSON.parse(logs.find((text) => text.includes('"type":"acpx.error"'))!.trim()); + expect(payload.message).toBe(result.errorMessage); + expect(payload.rpcCode).toBe(-32603); + } + }, + ); + + it("does not repeat JSON-RPC details already in the message and leaves plain errors unchanged", async () => { + const run = async (err: Error) => { + const root = await makeTempRoot(); + const execute = createAcpxEngineExecutor({ + createRuntime: () => ({ + ...buildRuntime(), + ensureSession: async () => { throw err; }, + }) as never, + }); + return execute({ + runId: "run-1", + agent: { id: "agent-1", companyId: "company-1" }, + runtime: {}, + config: { agent: "custom", agentCommand: "node ./fake-acp.js", stateDir: path.join(root, "state") }, + context: {}, + onLog: async () => {}, + onMeta: async () => {}, + } as never); + }; + + const repeated = await run(Object.assign( + new Error("Internal error: Invalid session configuration"), + { code: -32603, data: { details: "Invalid session configuration" } }, + )); + expect(repeated.errorMessage).toBe("Internal error: Invalid session configuration"); + expect(repeated.errorMeta).toMatchObject({ rpcCode: -32603 }); + + const plain = await run(new Error("session/new failed")); + expect(plain.errorMessage).toBe("session/new failed"); + expect(plain.summary).toBe("session/new failed"); + expect(plain.errorMeta?.rpcCode).toBeUndefined(); + }); + it("enriches acpx.error diagnostics and child stderr when ensureSession rejects", async () => { const root = await makeTempRoot(); const stateDir = path.join(root, "state"); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index b48c1414ca1..9fe1f8184a8 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -3623,9 +3623,22 @@ async function emitAcpxFailure(input: { childStderrTail: string | null; }> { const { ctx, prepared, err, phase, messageOverride, suppressChildStderrTail } = input; + const requestError = parseObject(err); + const data = parseObject(requestError.data); + const detail = typeof requestError.data === "string" + ? requestError.data + : asString(data.details, asString(data.message, "")); const rawMessage = err instanceof Error ? err.message : String(err); - const message = messageOverride ?? rawMessage; + const message = messageOverride ?? (detail + ? sanitizeTerminalSessionFailure({ + category: "unknown", + title: rawMessage.includes(detail) ? rawMessage : `${rawMessage}\n${detail}`, + }, prepared.env, ctx.authToken, parseObject(ctx.config.env)).title ?? "ACP request failed." + : rawMessage); const classified = classifyError(err, phase); + if (typeof requestError.code === "number" && Number.isSafeInteger(requestError.code)) { + classified.errorMeta = { ...classified.errorMeta, rpcCode: requestError.code }; + } const childStderrTail = suppressChildStderrTail ? null : await readChildStderrTail({ logPath: prepared.childStderrLogPath });