From 71952b709fe305865ec47992e585f9dad405a637 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Sun, 12 Jul 2026 17:23:36 -0700 Subject: [PATCH] fix(tools): thrown tool errors match original wire+display format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Read of a directory printed the raw wire markup in the transcript: Error: path is not a file: /ws/obama-blog/src The original never shows those bytes, on two independent grounds this port had missed: 1. Wire parity — thrown call() errors are sent RAW. TS formatError (utils/toolErrors.ts:5) returns the message (+ stderr/stdout parts, 40KB middle truncation) and is_error:true carries the error-ness (toolExecution.ts:1633+1674). tags are reserved for pre-execution failures (no-such-tool, InputValidationError, validateInput), which keep them — identical to TS. 2. Display parity — the transcript renderer strips error markup anyway. Port of FallbackToolUseErrorMessage.tsx (extractTag tool_use_error → drop sandbox_violations → strip tags → InputValidationError collapses to "Invalid tool parameters") plus each tool's renderToolUseErrorMessage collapse (File not found / Error reading file / File must be read first / …) with extractTag ported verbatim from utils/messages.ts:635. Supporting changes: - OpenAI-wire converters (chat-completions, Responses, Gemini) now prefix "Error: " on is_error tool results, mirroring TS convertToolResultContent (openaiShim.ts:309) — role=tool has no is_error field, so with the tags gone the prefix is the only failure signal those models get. - Read on a directory raises the exact TS message (readFileInRange.ts:89): EISDIR: illegal operation on a directory, read '' — models know this string from original transcripts and recover by listing. Other non-files (FIFOs/sockets) keep the generic message: TS streams those, we refuse (pre-existing deliberate divergence). - Overflow hint pluralizes ("+1 line") like the original. Suites: pytest 8529 passed / 0 failed (15 new); vitest +8 passed, pre-existing failures byte-identical to main; tsc clean. Co-Authored-By: Claude Opus 4.8 --- src/providers/gemini_provider.py | 6 + src/providers/openai_compatible.py | 11 + src/providers/openai_responses.py | 6 + src/services/tool_execution/tool_execution.py | 49 ++- src/tool_system/tools/read.py | 8 + tests/test_tool_error_format_parity.py | 305 ++++++++++++++++++ ui-tui/src/__tests__/toolTranscript.test.ts | 64 ++++ ui-tui/src/gatewayClient.ts | 75 ++++- ui-tui/src/lib/messages.test.ts | 31 +- ui-tui/src/lib/messages.ts | 65 ++++ 10 files changed, 610 insertions(+), 10 deletions(-) create mode 100644 tests/test_tool_error_format_parity.py diff --git a/src/providers/gemini_provider.py b/src/providers/gemini_provider.py index bb4b7b898..217a6f10a 100644 --- a/src/providers/gemini_provider.py +++ b/src/providers/gemini_provider.py @@ -211,6 +211,12 @@ def _convert_messages( ) else: raw_text = str(raw) + # FunctionResponse has no error field — prefix the + # text like the OpenAI-wire converters (mirrors TS + # convertToolResultContent, openaiShim.ts:309) so + # failed tools keep a failure signal on the wire. + if block.get("is_error"): + raw_text = f"Error: {raw_text}" # Gemini FunctionResponse needs a name; tool_use_id is # an Anthropic-only correlation id. Use it as the name # if no better signal is available. diff --git a/src/providers/openai_compatible.py b/src/providers/openai_compatible.py index 3bf922d71..72b85eb12 100644 --- a/src/providers/openai_compatible.py +++ b/src/providers/openai_compatible.py @@ -416,6 +416,17 @@ def _convert_anthropic_messages_to_openai( # requirement is honoured. flat_content = "[empty tool result]" + # OpenAI's ``role=tool`` message has no ``is_error`` field, + # so error-ness must ride the text: prefix ``Error: `` like + # TS convertToolResultContent (openaiShim.ts:309/:349/:356 + # — every text-shaped emission applies it). Load-bearing + # since thrown tool errors are no longer wrapped in + # ```` tags (_format_error parity port): + # without the prefix an OpenAI-wire model would see a bare + # message with no failure signal at all. + if tr.get("is_error"): + flat_content = f"Error: {flat_content}" + result.append({ "role": "tool", "tool_call_id": tool_use_id, diff --git a/src/providers/openai_responses.py b/src/providers/openai_responses.py index 2e8f917f7..b5556448f 100644 --- a/src/providers/openai_responses.py +++ b/src/providers/openai_responses.py @@ -349,6 +349,12 @@ def convert_messages_to_responses_input( }) if not flat: flat = "[empty tool result]" + # ``function_call_output`` is text-only with no error + # field — prefix ``Error: `` on is_error, same policy + # as the Chat Completions converter (which mirrors TS + # convertToolResultContent, openaiShim.ts:309). + if block.get("is_error"): + flat = f"Error: {flat}" input_items.append({ "type": "function_call_output", "call_id": call_id, diff --git a/src/services/tool_execution/tool_execution.py b/src/services/tool_execution/tool_execution.py index a0ff7064b..d18d62f94 100644 --- a/src/services/tool_execution/tool_execution.py +++ b/src/services/tool_execution/tool_execution.py @@ -16,6 +16,7 @@ from src.types.messages import ( CANCEL_MESSAGE, + INTERRUPT_MESSAGE_FOR_TOOL_USE, AssistantMessage, Message, create_progress_message, @@ -667,13 +668,51 @@ def _create_tool_result_stop(tool_use_id: str) -> dict[str, Any]: } +# TS formatError caps thrown-error content at 40KB with middle truncation +# (toolErrors.ts:16) — "enough for most command error logs". +_FORMAT_ERROR_MAX_LENGTH = 40000 + + def _format_error(error: Exception) -> str: + """Format a thrown tool error — port of formatError (utils/toolErrors.ts:5). + + Thrown ``call()`` errors go on the wire RAW: ``is_error: True`` on the + tool_result carries the error-ness (TS toolExecution.ts:1633+1674 sends + ``content: formatError(error)`` with no ```` wrapping). + The tags are reserved for pre-execution failures — no-such-tool, + InputValidationError, validate_input — which TS also tags. Wrapping + thrown errors too (the old behavior here) leaked the tags into the TUI + transcript (`Error: path is not a file: …`) and diverged + the model-facing bytes from the original. + + getErrorParts (toolErrors.ts:26) appends ``stderr``/``stdout`` string + attributes when present (subprocess.CalledProcessError carries both). + The TS ShellError branch has no Python parallel — our bash tool formats + shell failures into the result body itself, so only the generic branch + is ported. + """ if isinstance(error, AbortError): - return CANCEL_MESSAGE - msg = str(error) - if not msg: - msg = type(error).__name__ - return f"{msg}" + # TS: error.message || INTERRUPT_MESSAGE_FOR_TOOL_USE. Effectively + # dead here — AbortError is caught by the dedicated except at the + # call-tool layer before Step 14 — kept as a belt for direct callers. + return str(error) or INTERRUPT_MESSAGE_FOR_TOOL_USE + parts = [str(error)] + for attr in ("stderr", "stdout"): + value = getattr(error, attr, None) + if isinstance(value, str): + parts.append(value) + full_message = "\n".join(p for p in parts if p).strip() + if not full_message: + full_message = "Command failed with no output" + if len(full_message) <= _FORMAT_ERROR_MAX_LENGTH: + return full_message + half_length = _FORMAT_ERROR_MAX_LENGTH // 2 + start = full_message[:half_length] + end = full_message[-half_length:] + return ( + f"{start}\n\n... [{len(full_message) - _FORMAT_ERROR_MAX_LENGTH} " + f"characters truncated] ...\n\n{end}" + ) def classify_tool_error(error: Exception) -> str: diff --git a/src/tool_system/tools/read.py b/src/tool_system/tools/read.py index 1e7db84ed..dce411e5c 100644 --- a/src/tool_system/tools/read.py +++ b/src/tool_system/tools/read.py @@ -318,6 +318,14 @@ def _read_call(tool_input: dict[str, Any], context: ToolContext) -> ToolResult: raise ToolInputError(message) if not path.is_file(): + # Directory: exact TS message (readFileInRange.ts:89-92) — models + # know EISDIR from original-CC transcripts and recover by listing. + # Other non-files (sockets, FIFOs, devices) keep the generic message: + # TS streams those; we refuse (deliberate pre-existing divergence). + if path.is_dir(): + raise ToolInputError( + f"EISDIR: illegal operation on a directory, read '{path}'" + ) raise ToolInputError(f"path is not a file: {path}") offset = tool_input.get("offset") diff --git a/tests/test_tool_error_format_parity.py b/tests/test_tool_error_format_parity.py new file mode 100644 index 000000000..e5cfffc0d --- /dev/null +++ b/tests/test_tool_error_format_parity.py @@ -0,0 +1,305 @@ +"""Thrown-tool-error wire format parity — port of TS formatError. + +The original sends thrown ``call()`` errors RAW: ``content: +formatError(error)`` with ``is_error: true`` and NO ```` +wrapping (toolExecution.ts:1633+1674). The tags are reserved for +pre-execution failures (no-such-tool :391, InputValidationError :726, +validate_input :783), which we tag identically. The old ``_format_error`` +wrapped thrown errors too, which (a) diverged the model-facing bytes and +(b) leaked ``Error: path is not a file: …`` +into the TUI transcript. + +Because ``role=tool`` messages have no ``is_error`` field on the OpenAI +wire, converters must carry the error signal in text: ``Error: `` prefix, +mirroring TS convertToolResultContent (openaiShim.ts:309/:349/:356). Without +it, de-tagging would have erased the failure signal for OpenAI-wire models. + +Also pins the Read-on-directory message: EISDIR wording, byte-identical to +TS readFileInRange.ts:89-92. +""" +from __future__ import annotations + +import asyncio +import os +import tempfile +import unittest +from pathlib import Path +from typing import Any + +from src.providers.openai_compatible import _convert_anthropic_messages_to_openai +from src.providers.openai_responses import convert_messages_to_responses_input +from src.services.tool_execution.tool_execution import _format_error, run_tool_use +from src.tool_system.build_tool import build_tool +from src.tool_system.context import ToolContext, ToolUseOptions +from src.tool_system.errors import ToolInputError +from src.tool_system.tools.read import _read_call +from src.types.content_blocks import ToolResultBlock, ToolUseBlock +from src.types.messages import INTERRUPT_MESSAGE_FOR_TOOL_USE, AssistantMessage +from src.utils.abort_controller import AbortController, AbortError + + +# --------------------------------------------------------------------------- +# _format_error — unit (toolErrors.ts:5-43) +# --------------------------------------------------------------------------- + +class TestFormatError(unittest.TestCase): + def test_plain_exception_is_raw_untagged(self) -> None: + out = _format_error(ValueError("path is not a file: /x/src")) + self.assertEqual(out, "path is not a file: /x/src") + self.assertNotIn("", out) + + def test_stderr_stdout_string_attrs_join_as_parts(self) -> None: + # getErrorParts (toolErrors.ts:26): message, stderr, stdout — + # subprocess.CalledProcessError carries both attributes. + err = RuntimeError("exit 1") + err.stderr = "boom to stderr" + err.stdout = "partial stdout" + self.assertEqual( + _format_error(err), "exit 1\nboom to stderr\npartial stdout" + ) + + def test_non_string_stderr_excluded(self) -> None: + err = RuntimeError("exit 1") + err.stderr = b"bytes ignored" # TS: typeof === 'string' gate + self.assertEqual(_format_error(err), "exit 1") + + def test_empty_message_falls_back(self) -> None: + self.assertEqual(_format_error(ValueError()), "Command failed with no output") + + def test_over_40kb_middle_truncated(self) -> None: + msg = "x" * 50_000 + out = _format_error(ValueError(msg)) + self.assertIn("... [10000 characters truncated] ...", out) + self.assertTrue(out.startswith("x" * 100)) + self.assertTrue(out.endswith("x" * 100)) + # 20_000 head + 20_000 tail + the marker line + self.assertLess(len(out), 41_000) + + def test_abort_error_uses_message_or_interrupt_constant(self) -> None: + self.assertEqual(_format_error(AbortError("stop reason")), "stop reason") + # Python AbortError defaults reason="aborted", so the TS + # `error.message || INTERRUPT_MESSAGE_FOR_TOOL_USE` fallback only + # fires for an explicitly empty reason. + self.assertEqual( + _format_error(AbortError("")), INTERRUPT_MESSAGE_FOR_TOOL_USE + ) + + +# --------------------------------------------------------------------------- +# Thrown tool error through the pipeline — content untagged on the wire +# --------------------------------------------------------------------------- + +def _make_ctx(workspace: Path, tools: list | None = None) -> ToolContext: + ctx = ToolContext( + workspace_root=workspace, + options=ToolUseOptions(tools=tools or []), + ) + ctx.abort_controller = AbortController() + ctx.permission_context.mode = "bypassPermissions" + return ctx + + +def _allow(*_a: Any, **_k: Any) -> dict[str, Any]: + return {"behavior": "allow"} + + +def _stub(name: str, call): + return build_tool( + name=name, + input_schema={ + "type": "object", "properties": {}, "additionalProperties": True, + }, + call=call, + prompt=name, + description=name, + ) + + +def _run_tool(ctx: ToolContext, name: str, tid: str) -> list: + block = ToolUseBlock(id=tid, name=name, input={}) + + async def drive(): + updates = [] + async for u in run_tool_use( + block, AssistantMessage(content="t"), _allow, ctx + ): + updates.append(u) + return updates + + return asyncio.run(drive()) + + +def _sole_tool_result(updates: list) -> tuple[Any, Any]: + """(tool_result block, carrying message) — exactly one expected.""" + found = [] + for u in updates: + m = getattr(u, "message", u) + content = getattr(m, "content", None) + if not isinstance(content, list): + continue + for b in content: + if isinstance(b, ToolResultBlock): + found.append((b, m)) + elif isinstance(b, dict) and b.get("type") == "tool_result": + found.append(( + ToolResultBlock( + tool_use_id=b.get("tool_use_id", ""), + content=b.get("content", ""), + is_error=bool(b.get("is_error")), + ), + m, + )) + assert len(found) == 1, f"expected exactly one tool_result, got {found}" + return found[0] + + +class TestThrownToolErrorWireFormat(unittest.TestCase): + def test_thrown_error_content_is_raw_with_is_error(self) -> None: + def boom(_input, _ctx): + raise RuntimeError("kaboom with details") + + with tempfile.TemporaryDirectory() as tmp: + tool = _stub("Boom", boom) + ctx = _make_ctx(Path(tmp), tools=[tool]) + block, msg = _sole_tool_result(_run_tool(ctx, "Boom", "t1")) + + self.assertTrue(block.is_error) + self.assertEqual(block.content, "kaboom with details") + # toolUseResult mirrors TS toolExecution.ts:1674: `Error: ${content}`. + self.assertEqual( + getattr(msg, "toolUseResult", None), "Error: kaboom with details" + ) + + def test_validation_failure_keeps_tool_use_error_tags(self) -> None: + """Pre-execution failures stay tagged (TS toolExecution.ts:726).""" + def ok(_input, _ctx): # pragma: no cover - never reached + return "unreachable" + + tool = build_tool( + name="Strict", + input_schema={ + "type": "object", + "properties": {"x": {"type": "string"}}, + "required": ["x"], + "additionalProperties": False, + }, + call=ok, + prompt="Strict", + description="Strict", + ) + with tempfile.TemporaryDirectory() as tmp: + ctx = _make_ctx(Path(tmp), tools=[tool]) + block, _ = _sole_tool_result(_run_tool(ctx, "Strict", "t2")) + + self.assertTrue(block.is_error) + self.assertTrue(block.content.startswith("InputValidationError: ")) + self.assertTrue(block.content.endswith("")) + + +# --------------------------------------------------------------------------- +# Read on a directory — EISDIR parity (readFileInRange.ts:89-92) +# --------------------------------------------------------------------------- + +class TestReadDirectoryEisdir(unittest.TestCase): + def test_directory_raises_eisdir_wording(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + ctx = ToolContext(workspace_root=Path(tmp)) + # ensure_readable_path resolves symlinks (macOS /var → /private/var), + # so the message names the resolved path. + target = (Path(tmp) / "src").resolve() + target.mkdir() + with self.assertRaises(ToolInputError) as caught: + _read_call({"file_path": str(target)}, ctx) + self.assertEqual( + str(caught.exception), + f"EISDIR: illegal operation on a directory, read '{target}'", + ) + + @unittest.skipUnless(hasattr(os, "mkfifo"), "mkfifo not available") + def test_non_directory_non_file_keeps_generic_message(self) -> None: + # TS streams FIFOs/devices; we refuse — deliberate divergence, and + # those must NOT masquerade as EISDIR. + with tempfile.TemporaryDirectory() as tmp: + ctx = ToolContext(workspace_root=Path(tmp)) + fifo = (Path(tmp) / "pipe").resolve() + os.mkfifo(fifo) + with self.assertRaises(ToolInputError) as caught: + _read_call({"file_path": str(fifo)}, ctx) + self.assertEqual(str(caught.exception), f"path is not a file: {fifo}") + + +# --------------------------------------------------------------------------- +# Converters — is_error rides the text as an ``Error: `` prefix +# --------------------------------------------------------------------------- + +class TestOpenAICompatErrorPrefix(unittest.TestCase): + def _convert(self, content: str, is_error: bool) -> list: + messages = [ + {"role": "assistant", "content": [ + {"type": "tool_use", "id": "call_a", "name": "Read", "input": {}}, + ]}, + {"role": "user", "content": [ + { + "type": "tool_result", + "tool_use_id": "call_a", + "content": content, + "is_error": is_error, + }, + ]}, + ] + return _convert_anthropic_messages_to_openai(messages) + + def test_error_result_prefixed(self) -> None: + out = self._convert( + "EISDIR: illegal operation on a directory, read '/x/src'", True + ) + self.assertEqual(out[1]["role"], "tool") + self.assertEqual( + out[1]["content"], + "Error: EISDIR: illegal operation on a directory, read '/x/src'", + ) + + def test_success_result_unprefixed(self) -> None: + out = self._convert("1\thello\n", False) + self.assertEqual(out[1]["content"], "1\thello\n") + + def test_tagged_validation_error_prefixed_unconditionally(self) -> None: + # TS applies the prefix to every is_error emission — even content + # that already carries the tag envelope (openaiShim.ts:309). + out = self._convert("InputValidationError: bad", True) + self.assertEqual( + out[1]["content"], + "Error: InputValidationError: bad", + ) + + +class TestResponsesErrorPrefix(unittest.TestCase): + def _convert(self, content: str, is_error: bool) -> list: + messages = [ + {"role": "assistant", "content": [ + {"type": "tool_use", "id": "call_a", "name": "Read", "input": {}}, + ]}, + {"role": "user", "content": [ + { + "type": "tool_result", + "tool_use_id": "call_a", + "content": content, + "is_error": is_error, + }, + ]}, + ] + items, _instructions = convert_messages_to_responses_input(messages) + return [i for i in items if i.get("type") == "function_call_output"] + + def test_error_output_prefixed(self) -> None: + outputs = self._convert("boom", True) + self.assertEqual(len(outputs), 1) + self.assertEqual(outputs[0]["output"], "Error: boom") + + def test_success_output_unprefixed(self) -> None: + outputs = self._convert("fine", False) + self.assertEqual(outputs[0]["output"], "fine") + + +if __name__ == "__main__": + unittest.main() diff --git a/ui-tui/src/__tests__/toolTranscript.test.ts b/ui-tui/src/__tests__/toolTranscript.test.ts index 66d65448e..b03adc1b3 100644 --- a/ui-tui/src/__tests__/toolTranscript.test.ts +++ b/ui-tui/src/__tests__/toolTranscript.test.ts @@ -100,6 +100,70 @@ describe('formatToolResult', () => { expect(lines[0]).toBe('Error: e0') expect(lines[10]).toBe('… +4 lines (ctrl+o to see all)') }) + + // FallbackToolUseErrorMessage.tsx:35-47 — the wire-format markup never + // reaches the transcript (the bug: `Error: path is not a + // file: …` printed raw). + it('strips tool_use_error / error / sandbox_violations markup from errors', () => { + expect(formatToolResult('Bash', 'permission denied', true)).toBe( + 'Error: permission denied' + ) + expect(formatToolResult('Bash', 'Command was aborted', true)).toBe( + 'Error: Command was aborted' + ) + expect( + formatToolResult( + 'Bash', + 'boomwrite /etc/hosts\nconnect 1.2.3.4', + true + ) + ).toBe('Error: boom') + // Cancelled: survives unprefixed after unwrapping. + expect(formatToolResult('Bash', 'Cancelled: parallel tool call errored', true)).toBe( + 'Cancelled: parallel tool call errored' + ) + }) + + it('collapses InputValidationError to the original one-liner', () => { + expect( + formatToolResult( + 'Grep', + 'InputValidationError: Grep failed due to the following issue:\nAn unexpected parameter `n` was provided', + true + ) + ).toBe('Error searching files') + // Non-search tools take the fallback collapse (FallbackToolUseErrorMessage.tsx:39-40). + expect( + formatToolResult('TodoWrite', 'InputValidationError: bad', true) + ).toBe('Invalid tool parameters') + }) + + // Per-tool renderToolUseErrorMessage ports (tools/*/UI.tsx). + it('collapses tagged per-tool errors like the original transcript', () => { + expect(formatToolResult('Read', 'boom', true)).toBe('Error reading file') + expect( + formatToolResult('Read', "File does not exist: /x. Note: your current working directory is /ws.", true) + ).toBe('File not found') + expect( + formatToolResult('Glob', 'File does not exist. Note: your current working directory is /ws.', true) + ).toBe('File not found') + expect(formatToolResult('Edit', 'File has not been read yet. Read it first before writing to it.', true)).toBe( + 'File must be read first' + ) + expect(formatToolResult('Edit', 'boom', true)).toBe('Error editing file') + expect(formatToolResult('Write', 'boom', true)).toBe('Error writing file') + expect(formatToolResult('NotebookEdit', 'boom', true)).toBe('Error editing notebook') + // Untagged thrown errors (formatError parity: raw on the wire) show in full. + expect(formatToolResult('Read', "EISDIR: illegal operation on a directory, read '/ws/src'", true)).toBe( + "Error: EISDIR: illegal operation on a directory, read '/ws/src'" + ) + }) + + it('pluralizes the overflow hint for a single hidden line', () => { + const big = Array.from({ length: 11 }, (_, i) => `e${i}`).join('\n') + + expect(formatToolResult('Bash', big, true).split('\n')[10]).toBe('… +1 line (ctrl+o to see all)') + }) }) // ── virtualHeights: multi-line tool entries count rendered rows ────────────── diff --git a/ui-tui/src/gatewayClient.ts b/ui-tui/src/gatewayClient.ts index 850c762ee..567af1304 100644 --- a/ui-tui/src/gatewayClient.ts +++ b/ui-tui/src/gatewayClient.ts @@ -30,6 +30,7 @@ import type { StructuredDiffPayload } from './gatewayTypes.js' import { formatTotalCost, setLastCostSnapshot } from './lib/costSummary.js' +import { extractTag } from './lib/messages.js' import type { SessionInfo } from './types.js' const STARTUP_TIMEOUT_MS = 30_000 @@ -185,6 +186,50 @@ function webSearchSummary(result: string, webSearch?: WebSearchDisplay): string return s === undefined ? line : `${line} in ${s >= 1 ? `${Math.round(s)}s` : `${Math.round(s * 1000)}ms`}` } +// Shared with the backend Read tool (read.py FILE_NOT_FOUND_CWD_NOTE) and the +// original (utils/file.ts:213) — the marker the per-tool error renderers key +// "File not found" on. +const FILE_NOT_FOUND_CWD_NOTE = 'Note: your current working directory is' + +/** Collapsed one-liners for known error shapes, port of each tool's + * renderToolUseErrorMessage (tools/{FileReadTool,GrepTool,GlobTool, + * FileEditTool,FileWriteTool,NotebookEditTool}/UI.tsx). These fire in the + * original's non-verbose transcript — our trail line — while the raw text + * stays reachable behind ctrl+o (result_raw), the verbose analog. Only + * ``-tagged results collapse (pre-execution failures: + * validation, permissions); thrown call() errors arrive untagged and fall + * through to the full message. Exception: Read's file-not-found check is on + * the raw string — Read throws that error, so it's never tagged + * (FileReadTool/UI.tsx:150-156). */ +function toolSpecificErrorSummary(name: string | undefined, result: string): string | undefined { + if (name === 'Read' && result.includes(FILE_NOT_FOUND_CWD_NOTE)) { + return 'File not found' + } + + const tagged = extractTag(result, 'tool_use_error') + + if (!tagged) {return undefined} + + switch (name) { + case 'Read': + return 'Error reading file' + case 'Grep': + case 'Glob': + return tagged.includes(FILE_NOT_FOUND_CWD_NOTE) ? 'File not found' : 'Error searching files' + case 'Edit': + // "Show a less scary message for intended behavior" (FileEditTool/UI.tsx:138). + if (tagged.includes('File has not been read yet')) {return 'File must be read first'} + + return tagged.includes(FILE_NOT_FOUND_CWD_NOTE) ? 'File not found' : 'Error editing file' + case 'Write': + return 'Error writing file' + case 'NotebookEdit': + return 'Error editing notebook' + default: + return undefined + } +} + export function formatToolResult( name: string | undefined, result: string, @@ -192,18 +237,40 @@ export function formatToolResult( webSearch?: WebSearchDisplay ): string { if (isError) { - let msg = (result ?? '').trim() || 'Tool execution failed' + const summary = toolSpecificErrorSummary(name, result ?? '') - if (!/^(Error|Cancelled): /.test(msg)) { - msg = `Error: ${msg}` + if (summary) {return summary} + + // Port of FallbackToolUseErrorMessage.tsx:30-55 — unwrap the wire-format + // error markup before display: the `` envelope, sandbox + // violation blocks, and bare `` tags are model-facing bytes, not + // something the transcript should print. + const extracted = extractTag(result ?? '', 'tool_use_error') ?? (result ?? '') + const trimmed = extracted + .replace(/[\s\S]*?<\/sandbox_violations>/g, '') + .replace(/<\/?error>/g, '') + .trim() + + let msg: string + + if (!trimmed) { + msg = 'Tool execution failed' + } else if (trimmed.includes('InputValidationError: ')) { + msg = 'Invalid tool parameters' + } else if (/^(Error|Cancelled): /.test(trimmed)) { + msg = trimmed + } else { + msg = `Error: ${trimmed}` } const lines = msg.split('\n') if (lines.length > ERROR_RESULT_MAX_LINES) { + const plusLines = lines.length - ERROR_RESULT_MAX_LINES + return [ ...lines.slice(0, ERROR_RESULT_MAX_LINES), - `… +${lines.length - ERROR_RESULT_MAX_LINES} lines (ctrl+o to see all)` + `… +${plusLines} ${plusLines === 1 ? 'line' : 'lines'} (ctrl+o to see all)` ].join('\n') } diff --git a/ui-tui/src/lib/messages.test.ts b/ui-tui/src/lib/messages.test.ts index 422ddb1af..a815e34ed 100644 --- a/ui-tui/src/lib/messages.test.ts +++ b/ui-tui/src/lib/messages.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' -import { appendTranscriptMessage } from './messages.js' +import { appendTranscriptMessage, extractTag } from './messages.js' describe('appendTranscriptMessage', () => { it('merges adjacent tool-only shelves into one transcript row', () => { @@ -27,3 +27,32 @@ describe('appendTranscriptMessage', () => { ]) }) }) + +// ── extractTag: exact port of the original (utils/messages.ts:635) ────────── + +describe('extractTag', () => { + it('extracts simple multiline tag content', () => { + expect(extractTag('path is not a file: /x/src', 'tool_use_error')).toBe( + 'path is not a file: /x/src' + ) + expect(extractTag('line1\nline2', 'e')).toBe('line1\nline2') + }) + + it('returns null when the tag is absent, empty, or unclosed', () => { + expect(extractTag('no tags here', 'tool_use_error')).toBeNull() + expect(extractTag('', 'tool_use_error')).toBeNull() + expect(extractTag('unclosed', 'tool_use_error')).toBeNull() + expect(extractTag('', 'tool_use_error')).toBeNull() + }) + + it('handles attributes and case-insensitive matches', () => { + expect(extractTag('boom', 'err')).toBe('boom') + expect(extractTag('boom', 'err')).toBe('boom') + }) + + it('only returns content at nesting depth zero', () => { + // Outer match wins; the regex is non-greedy so the first top-level + // closing tag ends the match (original behavior, preserved verbatim). + expect(extractTag('outer inner', 't')).toBe('outer inner') + }) +}) diff --git a/ui-tui/src/lib/messages.ts b/ui-tui/src/lib/messages.ts index b8e89421e..d3fd63904 100644 --- a/ui-tui/src/lib/messages.ts +++ b/ui-tui/src/lib/messages.ts @@ -6,3 +6,68 @@ export const appendTranscriptMessage = (prev: Msg[], msg: Msg): Msg[] => appendT export const upsert = (prev: Msg[], role: Role, text: string): Msg[] => prev.at(-1)?.role === role ? [...prev.slice(0, -1), { role, text }] : [...prev, { role, text }] + +// Exact port of the original's escapeRegExp (utils/stringUtils.ts:9). +function escapeRegExp(str: string): string { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') +} + +/** Extract the content of the first top-level `…` block. + * Exact port of the original's extractTag (utils/messages.ts:635-689) — + * the error renderer uses it to unwrap `` from tool_result + * content before display (FallbackToolUseErrorMessage.tsx:35). */ +export function extractTag(html: string, tagName: string): string | null { + if (!html.trim() || !tagName.trim()) { + return null + } + + const escapedTag = escapeRegExp(tagName) + + // Create regex pattern that handles: + // 1. Self-closing tags + // 2. Tags with attributes + // 3. Nested tags of the same type + // 4. Multiline content + const pattern = new RegExp( + `<${escapedTag}(?:\\s+[^>]*)?>` + // Opening tag with optional attributes + '([\\s\\S]*?)' + // Content (non-greedy match) + `<\\/${escapedTag}>`, // Closing tag + 'gi' + ) + + let match + let depth = 0 + let lastIndex = 0 + const openingTag = new RegExp(`<${escapedTag}(?:\\s+[^>]*?)?>`, 'gi') + const closingTag = new RegExp(`<\\/${escapedTag}>`, 'gi') + + while ((match = pattern.exec(html)) !== null) { + // Check for nested tags + const content = match[1] + const beforeMatch = html.slice(lastIndex, match.index) + + // Reset depth counter + depth = 0 + + // Count opening tags before this match + openingTag.lastIndex = 0 + while (openingTag.exec(beforeMatch) !== null) { + depth++ + } + + // Count closing tags before this match + closingTag.lastIndex = 0 + while (closingTag.exec(beforeMatch) !== null) { + depth-- + } + + // Only include content if we're at the correct nesting level + if (depth === 0 && content) { + return content + } + + lastIndex = match.index + match[0].length + } + + return null +}