From 9ceae2af58bebbb84d3da62ef4565574e95699b6 Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Sun, 23 Nov 2025 09:53:27 -0800 Subject: [PATCH] feat: cancel client-side tools (#113) --- src/agent/approval-execution.ts | 35 +++++++++++- src/cli/App.tsx | 76 +++++++++++++++++++------ src/tests/tools/bash.test.ts | 6 +- src/tests/tools/tool-truncation.test.ts | 2 +- src/tools/README.md | 19 +++++++ src/tools/impl/Bash.ts | 43 ++++++++++---- src/tools/manager.ts | 25 +++++++- 7 files changed, 170 insertions(+), 36 deletions(-) create mode 100644 src/tools/README.md diff --git a/src/agent/approval-execution.ts b/src/agent/approval-execution.ts index ec89a38..d01412f 100644 --- a/src/agent/approval-execution.ts +++ b/src/agent/approval-execution.ts @@ -33,10 +33,34 @@ export type ApprovalResult = ToolReturn | ApprovalCreate.ApprovalReturn; export async function executeApprovalBatch( decisions: ApprovalDecision[], onChunk?: (chunk: ToolReturnMessage) => void, + options?: { abortSignal?: AbortSignal }, ): Promise { const results: ApprovalResult[] = []; for (const decision of decisions) { + // If aborted before starting this decision, record an interrupted result + if (options?.abortSignal?.aborted) { + // Emit an interrupted chunk for visibility if callback provided + if (onChunk) { + onChunk({ + message_type: "tool_return_message", + id: "dummy", + date: new Date().toISOString(), + tool_call_id: decision.approval.toolCallId, + tool_return: "User interrupted tool execution", + status: "error", + }); + } + + results.push({ + type: "tool", + tool_call_id: decision.approval.toolCallId, + tool_return: "User interrupted tool execution", + status: "error", + }); + continue; + } + if (decision.type === "approve") { // Execute the approved tool try { @@ -48,6 +72,7 @@ export async function executeApprovalBatch( const toolResult = await executeTool( decision.approval.toolName, parsedArgs, + { signal: options?.abortSignal }, ); // Update UI if callback provided (interactive mode) @@ -73,9 +98,15 @@ export async function executeApprovalBatch( stderr: toolResult.stderr, }); } catch (e) { - // Still need to send error result to backend for this tool - const errorMessage = `Error executing tool: ${String(e)}`; + const isAbortError = + e instanceof Error && + (e.name === "AbortError" || + e.message === "The operation was aborted"); + const errorMessage = isAbortError + ? "User interrupted tool execution" + : `Error executing tool: ${String(e)}`; + // Still need to send error result to backend for this tool // Update UI if callback provided if (onChunk) { onChunk({ diff --git a/src/cli/App.tsx b/src/cli/App.tsx index 79d8259..8004cf8 100644 --- a/src/cli/App.tsx +++ b/src/cli/App.tsx @@ -176,6 +176,10 @@ export default function App({ > >([]); const [isExecutingTool, setIsExecutingTool] = useState(false); + const [queuedApprovalResults, setQueuedApprovalResults] = useState< + ApprovalResult[] | null + >(null); + const toolAbortControllerRef = useRef(null); // Track auto-handled results to combine with user decisions const [autoHandledResults, setAutoHandledResults] = useState< @@ -732,6 +736,14 @@ export default function App({ }, []); const handleInterrupt = useCallback(async () => { + // If we're executing client-side tools, abort them locally instead of hitting the backend + if (isExecutingTool && toolAbortControllerRef.current) { + toolAbortControllerRef.current.abort(); + setStreaming(false); + setIsExecutingTool(false); + return; + } + if (!streaming || interruptRequested) return; setInterruptRequested(true); @@ -751,7 +763,7 @@ export default function App({ appendError(`Failed to interrupt stream: ${String(e)}`); setInterruptRequested(false); } - }, [agentId, streaming, interruptRequested, appendError]); + }, [agentId, streaming, interruptRequested, appendError, isExecutingTool]); // Reset interrupt flag when streaming ends useEffect(() => { @@ -1304,8 +1316,9 @@ export default function App({ setStreaming(true); refreshDerived(); - // Check for pending approvals before sending message - if (CHECK_PENDING_APPROVALS_BEFORE_SEND) { + // Check for pending approvals before sending message (skip if we already have + // a queued approval response to send first). + if (CHECK_PENDING_APPROVALS_BEFORE_SEND && !queuedApprovalResults) { try { const client = await getClient(); // Fetch fresh agent state to check for pending approvals with accurate in-context messages @@ -1343,14 +1356,25 @@ export default function App({ } } - // Start the conversation loop - await processConversation([ - { - type: "message", - role: "user", - content: messageContent as unknown as MessageCreate["content"], - }, - ]); + // Start the conversation loop. If we have queued approval results from an interrupted + // client-side execution, send them first before the new user message. + const initialInput: Array = []; + + if (queuedApprovalResults) { + initialInput.push({ + type: "approval", + approvals: queuedApprovalResults, + }); + setQueuedApprovalResults(null); + } + + initialInput.push({ + type: "message", + role: "user", + content: messageContent as unknown as MessageCreate["content"], + }); + + await processConversation(initialInput); // Clean up placeholders after submission clearPlaceholdersInText(msg); @@ -1368,6 +1392,7 @@ export default function App({ columns, commitEligibleLines, isExecutingTool, + queuedApprovalResults, ], ); @@ -1395,6 +1420,9 @@ export default function App({ // Show "thinking" state and lock input while executing approved tools client-side setStreaming(true); + const approvalAbortController = new AbortController(); + toolAbortControllerRef.current = approvalAbortController; + // Combine all decisions using snapshots const allDecisions = [ ...approvalResultsSnapshot, @@ -1421,7 +1449,10 @@ export default function App({ appendError(chunk.tool_return); } } + // Flush UI so completed tools show up while the batch continues + refreshDerived(); }, + { abortSignal: approvalAbortController.signal }, ); // Combine with auto-handled and auto-denied results using snapshots @@ -1472,16 +1503,25 @@ export default function App({ setThinkingMessage(getRandomThinkingMessage()); refreshDerived(); - // Continue conversation with all results - await processConversation([ - { - type: "approval", - approvals: allResults as ApprovalResult[], - }, - ]); + const wasAborted = approvalAbortController.signal.aborted; + + if (wasAborted) { + // Queue results to send alongside the next user message + setQueuedApprovalResults(allResults as ApprovalResult[]); + setStreaming(false); + } else { + // Continue conversation with all results + await processConversation([ + { + type: "approval", + approvals: allResults as ApprovalResult[], + }, + ]); + } } finally { // Always release the execution guard, even if an error occurred setIsExecutingTool(false); + toolAbortControllerRef.current = null; } }, [ diff --git a/src/tests/tools/bash.test.ts b/src/tests/tools/bash.test.ts index 9812ad1..4701414 100644 --- a/src/tests/tools/bash.test.ts +++ b/src/tests/tools/bash.test.ts @@ -10,7 +10,7 @@ describe("Bash tool", () => { expect(result.content).toBeDefined(); expect(result.content[0]?.text).toContain("Hello, World!"); - expect(result.isError).toBeUndefined(); + expect(result.status).toBe("success"); }); test("captures stderr in output", async () => { @@ -28,7 +28,7 @@ describe("Bash tool", () => { description: "Test exit code", }); - expect(result.isError).toBe(true); + expect(result.status).toBe("error"); expect(result.content[0]?.text).toContain("Exit code"); }); @@ -39,7 +39,7 @@ describe("Bash tool", () => { timeout: 100, }); - expect(result.isError).toBe(true); + expect(result.status).toBe("error"); expect(result.content[0]?.text).toContain("timed out"); }, 2000); diff --git a/src/tests/tools/tool-truncation.test.ts b/src/tests/tools/tool-truncation.test.ts index af7f05a..ad34002 100644 --- a/src/tests/tools/tool-truncation.test.ts +++ b/src/tests/tools/tool-truncation.test.ts @@ -65,7 +65,7 @@ describe("tool truncation integration tests", () => { const output = result.content[0]?.text || ""; expect(output).toContain("[Output truncated after 30,000 characters"); - expect(result.isError).toBe(true); + expect(result.status).toBe("error"); }, ); }); diff --git a/src/tools/README.md b/src/tools/README.md new file mode 100644 index 0000000..96749d1 --- /dev/null +++ b/src/tools/README.md @@ -0,0 +1,19 @@ +# Client-side tool guidelines + +How to implement tools that run locally in Letta Code. + +## Contract +- Function signature: `(args, opts?) => Promise<{ toolReturn: string; status: "success" | "error"; stdout?: string[]; stderr?: string[] }>` +- Optional `opts.signal?: AbortSignal`. If you spawn a subprocess, wire this signal to it so Esc/abort can kill it cleanly. If you’re pure in-process, you can ignore it. + +## Subprocess tools (e.g., Bash) +- Pass the provided `AbortSignal` to `exec`/`spawn` so abort kills the child. Normalize abort errors to `toolReturn: "User interrupted tool execution", status: "error"`. +- Avoid running multiple subprocesses unless you also expose a cancel hook; we execute tools serially to avoid races. + +## In-process tools (read/write/edit) +- You can ignore the signal, but still return a clear `toolReturn` and `status`. +- Be deterministic and side-effect aware; the runner keeps tools serial to avoid file races. + +## Errors +- Return a concise error message in `toolReturn` and set `status: "error"`. +- Don’t `console.error` from tools; the UI surfaces the returned message. diff --git a/src/tools/impl/Bash.ts b/src/tools/impl/Bash.ts index b2a9903..40d269e 100644 --- a/src/tools/impl/Bash.ts +++ b/src/tools/impl/Bash.ts @@ -12,6 +12,7 @@ interface BashArgs { timeout?: number; description?: string; run_in_background?: boolean; + signal?: AbortSignal; } interface BashResult { @@ -19,7 +20,7 @@ interface BashResult { type: string; text: string; }>; - isError?: boolean; + status: "success" | "error"; } export async function bash(args: BashArgs): Promise { @@ -29,13 +30,17 @@ export async function bash(args: BashArgs): Promise { timeout = 120000, description: _description, run_in_background = false, + signal, } = args; const userCwd = process.env.USER_CWD || process.cwd(); if (command === "/bashes") { const processes = Array.from(backgroundProcesses.entries()); if (processes.length === 0) { - return { content: [{ type: "text", text: "(no content)" }] }; + return { + content: [{ type: "text", text: "(no content)" }], + status: "success", + }; } let output = ""; for (const [id, proc] of processes) { @@ -44,7 +49,10 @@ export async function bash(args: BashArgs): Promise { : "unknown"; output += `${id}: ${proc.command} (${proc.status}, runtime: ${runtime})\n`; } - return { content: [{ type: "text", text: output.trim() }] }; + return { + content: [{ type: "text", text: output.trim() }], + status: "success", + }; } if (run_in_background) { @@ -100,6 +108,7 @@ export async function bash(args: BashArgs): Promise { text: `Command running in background with ID: ${bashId}`, }, ], + status: "success", }; } @@ -110,6 +119,7 @@ export async function bash(args: BashArgs): Promise { maxBuffer: 10 * 1024 * 1024, cwd: userCwd, env: { ...process.env }, + signal, }; const { stdout, stderr } = await execAsync(command, options); const stdoutStr = typeof stdout === "string" ? stdout : stdout.toString(); @@ -126,6 +136,7 @@ export async function bash(args: BashArgs): Promise { return { content: [{ type: "text", text: truncatedOutput }], + status: "success", }; } catch (error) { const err = error as NodeJS.ErrnoException & { @@ -133,14 +144,26 @@ export async function bash(args: BashArgs): Promise { stderr?: string; killed?: boolean; signal?: string; + code?: string; + name?: string; }; + const isAbort = + signal?.aborted || + err.code === "ABORT_ERR" || + err.name === "AbortError" || + err.message === "The operation was aborted"; + let errorMessage = ""; - if (err.killed && err.signal === "SIGTERM") - errorMessage = `Command timed out after ${effectiveTimeout}ms\n`; - if (err.code) errorMessage += `Exit code: ${err.code}\n`; - if (err.stderr) errorMessage += err.stderr; - else if (err.message) errorMessage += err.message; - if (err.stdout) errorMessage = `${err.stdout}\n${errorMessage}`; + if (isAbort) { + errorMessage = "User interrupted tool execution"; + } else { + if (err.killed && err.signal === "SIGTERM") + errorMessage = `Command timed out after ${effectiveTimeout}ms\n`; + if (err.code) errorMessage += `Exit code: ${err.code}\n`; + if (err.stderr) errorMessage += err.stderr; + else if (err.message) errorMessage += err.message; + if (err.stdout) errorMessage = `${err.stdout}\n${errorMessage}`; + } // Apply character limit even to error messages const { content: truncatedError } = truncateByChars( @@ -151,7 +174,7 @@ export async function bash(args: BashArgs): Promise { return { content: [{ type: "text", text: truncatedError }], - isError: true, + status: "error", }; } } diff --git a/src/tools/manager.ts b/src/tools/manager.ts index 9122650..7349107 100644 --- a/src/tools/manager.ts +++ b/src/tools/manager.ts @@ -459,6 +459,7 @@ function flattenToolResponse(result: unknown): string { export async function executeTool( name: string, args: ToolArgs, + options?: { signal?: AbortSignal }, ): Promise { const tool = toolRegistry.get(name); @@ -470,7 +471,13 @@ export async function executeTool( } try { - const result = await tool.fn(args); + // Inject abort signal for tools that support it (currently Bash) without altering schemas + const argsWithSignal = + name === "Bash" && options?.signal + ? { ...args, signal: options.signal } + : args; + + const result = await tool.fn(argsWithSignal); // Extract stdout/stderr if present (for bash tools) const recordResult = isRecord(result) ? result : undefined; @@ -478,7 +485,6 @@ export async function executeTool( const stderrValue = recordResult?.stderr; const stdout = isStringArray(stdoutValue) ? stdoutValue : undefined; const stderr = isStringArray(stderrValue) ? stderrValue : undefined; - // Flatten the response to plain text const flattenedResponse = flattenToolResponse(result); @@ -490,6 +496,21 @@ export async function executeTool( ...(stderr && { stderr }), }; } catch (error) { + const isAbort = + error instanceof Error && + (error.name === "AbortError" || + error.message === "The operation was aborted" || + // node:child_process AbortError may include code/message variants + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (error as any).code === "ABORT_ERR"); + + if (isAbort) { + return { + toolReturn: "User interrupted tool execution", + status: "error", + }; + } + // Don't console.error here - it pollutes the TUI // The error message is already returned in toolReturn return {