mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 11:01:16 +01:00
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 <!-- prospector:issue-fix v1 issue=16034 base=5cfa2bf26d3e report=1b07160d9492357f --> Co-authored-by: commitperclip[bot] <284812584+commitperclip[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
51d1d4ce07
commit
a00109aa35
3 files changed
+109
-1
No files matched your search
@@ -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
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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 });
|
||||
|
||||
Reference in new issue
Block a user