mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-31 22:24:16 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The tool gateway gives agents access to connected services. Recovery controls what happens when a run stops. > - Generated tool names can exceed the provider limit after the MCP client adds its prefix. > - The same invalid definition can fail each automatic retry. A cancelled run can also hold saved messages without showing its cause. > - This pull request bounds tool names, stops configuration retries, and retains cancellation evidence. > - It shows the stopped run and admits saved input only after the existing safety checks pass. > - The benefit is a clear recovery path that preserves operator Stop and prevents duplicate message delivery. ## Linked Issues or Issue Description **What happened?** A long connected MCP tool name makes the provider reject the entire request. Automatic recovery repeats the invalid request. Separately, unexpected legacy cancellations can leave saved input behind a recovery hold. The notice does not identify the stopped run or its cause. **Expected behavior** Complete MCP names fit the provider limit. Tool-definition errors require configuration repair. Cancelled runs retain their source and reason. The recovery notice shows the cause and saved-message count. Verified unexpected cancellations can start a fresh turn through the existing admission checks. **Steps to reproduce** 1. Assign an App gallery connection with a long application key and tool name to a Claude agent. 2. Start a run. The provider rejects a name over 128 characters, including its MCP prefix. 3. For cancellation recovery, stop a legacy provider turn without an operator Stop request and send a user message while the recovery hold is active. 4. Inspect the recovery notice and the deferred message queue. **Paperclip version or commit** Rebased onto master at `cf8ad63c806685bfd7c48e3ed4a919d61a7c55f1`. **Deployment mode** Hosted or self-hosted server with legacy Claude or Codex execution. Related public work: - Refs #14017. That PR caps name segments. This PR preserves existing short names and uses stable hash aliases for long complete names. It also covers classification and recovery. - Refs #4510. That PR adds a cancellation-source column. This PR records bounded evidence in the existing run result, without a migration. - Refs #12552 and #4506. Those PRs suppress recovery after operator cancellation. This PR preserves operator intent and uses the existing continuation gates. ## What Changed - Bound gateway names with the full provider prefix in the 128-character budget. Retain the original upstream tool name for dispatch and permissions. - Classify invalid tool definitions as configuration failures before diagnostic redaction. Stop automatic retries and continuation attempts for that error code. - Persist cancellation source, expectedness, initiator, reason, and time. Preserve recorded Stop intent when adapter results arrive. Report unexpected started cancellations with closed diagnostic labels. - Show the run cause, saved-message count, and Inspect run link. Offer Continue for eligible unexpected cancellations. Require verified provider stop, empty tool inventory, ownership, and the existing pause, budget, approval, and dependency gates. Use the existing queue for single delivery. - Add regression coverage and update the execution, MCP gateway, and run-log documentation. ## Verification - `pnpm -r typecheck` and `pnpm build` passed. - `pnpm check:token-gates` passed. - Ran `pnpm test:run` and completed its workspace and serialized groups. Initial resource and timing failures passed on isolated reruns. All 149 serialized route suites passed. - Reran the changed server, adapter, and UI suites after the rebase. Coverage includes long-name upstream dispatch, configuration retry suppression, cancellation evidence retention, privacy labels, oversized run projection, and concurrent saved-message delivery. - `pnpm test:e2e tests/e2e/legacy-failure-continuation.spec.ts` passed all six browser scenarios. The recovery notice shows the run cause and inspection link, and each recovery entry point reaches one new response. - Added database-backed checks for active, removed, paused, unavailable, and disabled chat connections. The final continuation and recovery-notice suites passed 167 tests. Externally bound chats hide board Continue and show a usable next action. - All 55 GitHub checks passed on `42afbf1371dcaeb72646e3d8f65c19ff7cddf8de`. Two unrelated Storybook jobs were skipped by their normal conditions. Greptile reviewed that commit at 5/5 with no findings and no open review threads. ## Risks - Long tool names change to aliases. Existing short names stay compatible. The original connection and upstream name remain the dispatch authority. - Invalid tool definitions no longer get automatic retries. An operator must repair the configuration before a new attempt. - Continuation changes apply only to positively identified unexpected legacy cancellations with complete empty tool inventory. Operator Stop, unknown historical cancellations, outstanding tools, and unverified provider termination keep their holds. - No database migration. The added projection fields are optional. Cancellation reason and initiator IDs remain local run evidence; Sentry receives only closed source and initiator-type labels and expectedness. ## Model Used - OpenAI GPT-6 through Codex, with reasoning, repository editing, shell execution, and GitHub tool use. The runtime does not expose the exact model variant or context-window size. ## 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 - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
155 lines
7.1 KiB
TypeScript
155 lines
7.1 KiB
TypeScript
import { describe, expect, it } from "vitest";
|
|
import { classifyToolDefinitionFailure, formatTerminalSessionFailure, sanitizeTerminalSessionFailure } from "./terminal-session-failure.js";
|
|
|
|
describe("terminal session failure diagnostics", () => {
|
|
it.each([
|
|
"API Error: 400 tools.17.custom.name: String should have at most 128 characters",
|
|
"tools[0].input_schema: invalid schema",
|
|
"Invalid schema for function 'read-file': unsupported type",
|
|
])("rejects unchanged tool definitions: %s", (details) => {
|
|
const failure = { category: "service", details };
|
|
expect(classifyToolDefinitionFailure(failure)).toEqual({
|
|
errorCode: "provider_tool_definition_invalid", errorFamily: "configuration",
|
|
});
|
|
// Classification precedes arbitrary short-secret redaction.
|
|
expect(sanitizeTerminalSessionFailure(failure, { SECRET: "128" }).details).not.toContain("128");
|
|
});
|
|
it.each([
|
|
"HTTP 503 service unavailable",
|
|
"tool call arguments are invalid: name must be a string",
|
|
"prompt is too long",
|
|
"tools.0.arguments.name: String should have at most 128 characters",
|
|
])("keeps unrelated errors on their existing recovery path: %s", (details) => {
|
|
expect(classifyToolDefinitionFailure({ category: "service", details })).toBeNull();
|
|
});
|
|
it("keeps the provider category, message, request id and stack", () => {
|
|
const failure = {
|
|
category: "service",
|
|
title: "HTTP 529: overloaded_error",
|
|
details: 'request_id=req_diagnostic_123\n{"error":{"message":"Service unavailable"}}\n at prompt (agent.js:42:7)',
|
|
};
|
|
const diagnostic = sanitizeTerminalSessionFailure(failure, {});
|
|
expect(diagnostic).toEqual(failure);
|
|
expect(formatTerminalSessionFailure("ACP turn failed.", diagnostic)).toBe(
|
|
`ACP turn failed.\n${failure.title}\n${failure.details}`,
|
|
);
|
|
});
|
|
|
|
it("redacts known credentials and common secret forms without dropping useful context", () => {
|
|
const secret = 'opaque / credential "value"';
|
|
const runKey = "run-credential-canary";
|
|
const diagnostic = sanitizeTerminalSessionFailure({
|
|
category: "access",
|
|
title: `Request rejected: ${secret}`,
|
|
details: [
|
|
`request_id=req_123 ${runKey}`,
|
|
JSON.stringify({ message: secret }),
|
|
`https://example.test/?value=${encodeURIComponent(secret)}`,
|
|
'Authorization: Bearer bearer-canary',
|
|
'{"api_key":"json-canary"}',
|
|
'sk-ant-example-provider-key-canary',
|
|
'TOKEN=assignment-canary',
|
|
].join("\n"),
|
|
}, { PROVIDER_SECRET: secret }, runKey);
|
|
const serialized = JSON.stringify(diagnostic);
|
|
for (const value of ["opaque", runKey, "bearer-canary", "json-canary", "sk-ant-example", "assignment-canary"]) {
|
|
expect(serialized).not.toContain(value);
|
|
}
|
|
expect(diagnostic.details).toContain("request_id=req_123");
|
|
expect(diagnostic.title).toBe("Request rejected: ***REDACTED***");
|
|
});
|
|
|
|
it("redacts before truncation and reports every omitted field", () => {
|
|
const credential = "opaque-credential-crossing-the-limit";
|
|
const diagnostic = sanitizeTerminalSessionFailure({
|
|
category: "service",
|
|
title: "t".repeat(5000),
|
|
details: `${"d".repeat(24568)}${credential}${"x".repeat(1000)}`,
|
|
}, { API_KEY: credential });
|
|
expect(diagnostic.truncatedFields).toEqual(["title", "details"]);
|
|
expect(diagnostic.title).toContain("[truncated: 904 characters omitted]");
|
|
expect(diagnostic.details).not.toContain("opaque");
|
|
expect(diagnostic.details).toContain("[truncated:");
|
|
expect(diagnostic.details!.length).toBeLessThan(24700);
|
|
});
|
|
|
|
it("redacts configured values with arbitrary names and connection URL passwords", () => {
|
|
const databaseUrl = "postgres://user:database%20canary@db.test/database";
|
|
const opaque = "arbitrarily-named-secret";
|
|
const diagnostic = sanitizeTerminalSessionFailure({
|
|
category: "service",
|
|
details: `request_id=req_123 ${databaseUrl}\npassword echoed: database canary\n${opaque}`,
|
|
}, { DATABASE_URL: databaseUrl, PROVIDER_SETTING: opaque }, undefined, {
|
|
DATABASE_URL: databaseUrl, PROVIDER_SETTING: opaque,
|
|
});
|
|
expect(diagnostic.details).toBe(
|
|
"request_id=req_123 ***REDACTED***\npassword echoed: ***REDACTED***\n***REDACTED***",
|
|
);
|
|
});
|
|
|
|
it("redacts unknown inherited launch values while preserving public process context", () => {
|
|
const diagnostic = sanitizeTerminalSessionFailure({
|
|
category: "service",
|
|
details: "request_id=req_123 inherited-canary A /workspace",
|
|
}, { UNEXPECTED_VARIABLE: "inherited-canary", ANOTHER_VARIABLE: "A", HOME: "/workspace" });
|
|
expect(diagnostic.details).toBe("request_id=req_123 ***REDACTED*** ***REDACTED*** /workspace");
|
|
});
|
|
|
|
it("preserves known boolean settings, HTTP codes and request IDs", () => {
|
|
const details = "HTTP 401 request_id=req_123 req-1 /1/path retries=1";
|
|
const diagnostic = sanitizeTerminalSessionFailure({ category: "access", details }, {
|
|
OPENCODE_ALLOW_ALL_MODELS: "1",
|
|
}, undefined, { OPENCODE_ALLOW_ALL_MODELS: "1" });
|
|
expect(diagnostic.details).toBe(details);
|
|
});
|
|
|
|
it("redacts short credentials even inside words, paths and punctuated strings", () => {
|
|
const diagnostic = sanitizeTerminalSessionFailure({
|
|
category: "access", details: "upstream abc-def /abc/path xabcx abc.value abc_other",
|
|
}, { CUSTOM_SETTING: "abc" }, undefined, { CUSTOM_SETTING: "abc" });
|
|
expect(diagnostic.details).not.toContain("abc");
|
|
expect(diagnostic.details).toBe(
|
|
"upstream ***REDACTED***-def /***REDACTED***/path x***REDACTED***x ***REDACTED***.value ***REDACTED***_other",
|
|
);
|
|
});
|
|
|
|
it("fits the persisted transcript chunk limit even with escaped provider text", () => {
|
|
const diagnostic = sanitizeTerminalSessionFailure({
|
|
category: "service",
|
|
title: '"'.repeat(5000),
|
|
details: "\\".repeat(40000),
|
|
}, {});
|
|
const log = JSON.stringify({
|
|
type: "acpx.error", summary: "failed", stopReason: "adapter_failed",
|
|
message: formatTerminalSessionFailure("ACP agent reported a terminal service failure.", diagnostic),
|
|
});
|
|
expect(log.length).toBeLessThan(64 * 1024);
|
|
expect(JSON.parse(log).message).toContain("[truncated:");
|
|
});
|
|
|
|
it("handles empty, malformed and control-character fields", () => {
|
|
expect(sanitizeTerminalSessionFailure({
|
|
category: "untrusted-category",
|
|
title: " \n",
|
|
details: 42 as unknown as string,
|
|
}, {})).toEqual({ category: "unknown" });
|
|
expect(sanitizeTerminalSessionFailure({
|
|
category: "service",
|
|
title: "\x1b[31mFailure\x1b[0m\0",
|
|
details: "line 1\n\tline 2",
|
|
}, {})).toEqual({ category: "service", title: "Failure", details: "line 1\n\tline 2" });
|
|
expect(formatTerminalSessionFailure("original error", null)).toBe("original error");
|
|
});
|
|
|
|
it("keeps truncated Unicode valid for JSONB storage", () => {
|
|
const diagnostic = sanitizeTerminalSessionFailure({
|
|
category: "service",
|
|
title: `${"x".repeat(4095)}🚨failure`,
|
|
details: "malformed \ud800 detail",
|
|
}, {});
|
|
expect(diagnostic.title).not.toMatch(/[\ud800-\udfff]/u);
|
|
expect(diagnostic.details).not.toMatch(/[\ud800-\udfff]/u);
|
|
expect(diagnostic.truncatedFields).toEqual(["title"]);
|
|
});
|
|
});
|