Report tool call failures as UI errors#3113
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@RitwijParmar is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR introduces discriminated result types and structured error handling for tool calls. A new ChangesTool Error Result Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/src/tools/result.ts`:
- Around line 28-31: The function formatToolCallError currently returns
JSON.stringify(error) which yields "undefined" for undefined inputs; change
formatToolCallError to detect undefined/null or non-stringifiable errors and
return a stable constant like "Unknown tool error" instead of the string
"undefined" (use this constant when JSON.stringify(error) is falsy or equals
"undefined"); also update the related unit test to expect "Unknown tool error"
for missing/unknown tool errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a53bfce-1a69-40d4-aae1-9f812e540a1e
📒 Files selected for processing (6)
apps/web/client/src/app/project/[id]/_hooks/use-chat/index.tsxapps/web/client/src/components/tools/tools.tspackages/ai/src/tools/index.tspackages/ai/src/tools/result.tspackages/ai/test/stream/convert.test.tspackages/ai/test/tools/result.test.ts
| try { | ||
| return JSON.stringify(error) ?? String(error); | ||
| } catch { | ||
| return String(error); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant code in the reviewed file
sed -n '1,120p' packages/ai/src/tools/result.ts
# Find all call sites of formatToolCallError
rg -n "formatToolCallError\s*\(" -S packages/ai/src/tools/result.ts packages/ai/src packages | head -n 200Repository: onlook-dev/onlook
Length of output: 1959
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the unit tests that mention formatToolCallError(undefined)
sed -n '1,120p' packages/ai/test/tools/result.test.ts
# Find call sites of createToolErrorResult (and formatToolCallError if any outside this file)
rg -n "createToolErrorResult\s*\(" packages/ai/src packages/ai/test packages | head -n 200
rg -n "formatToolCallError\s*\(" packages/ai/src packages/ai/test packages | head -n 200Repository: onlook-dev/onlook
Length of output: 2588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for usages of errorText to see how it's displayed
rg -n "errorText" packages/ai/src packages | head -n 200
# Search for formatToolCallError usage elsewhere
rg -n "formatToolCallError\s*\(" packages | head -n 200
# Search for output-error state handling (likely UI/consumers)
rg -n "state:\s*'output-error'|output-error" packages | head -n 200Repository: onlook-dev/onlook
Length of output: 2567
Use a stable fallback message for missing/unknown tool errors
formatToolCallError(undefined) currently returns 'undefined' (via JSON.stringify(undefined) ?? String(undefined)), and UI renders truthy errorText directly—there’s also a unit test asserting this behavior. Return a constant (e.g., 'Unknown tool error') instead and update the test accordingly.
Suggested patch
export function formatToolCallError(error: unknown): string {
@@
try {
- return JSON.stringify(error) ?? String(error);
+ return JSON.stringify(error) ?? 'Unknown tool error';
} catch {
- return String(error);
+ return 'Unknown tool error';
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai/src/tools/result.ts` around lines 28 - 31, The function
formatToolCallError currently returns JSON.stringify(error) which yields
"undefined" for undefined inputs; change formatToolCallError to detect
undefined/null or non-stringifiable errors and return a stable constant like
"Unknown tool error" instead of the string "undefined" (use this constant when
JSON.stringify(error) is falsy or equals "undefined"); also update the related
unit test to expect "Unknown tool error" for missing/unknown tool errors.
1 participant
Summary
output-errortool results instead of normal output stringsaddToolResultfails by clearingisExecutingToolCallinfinallyoutput-errorstateWhy
Issue #2766 asks for tool results to carry an error string so the UI can render tool failures separately from successful results. The UI already passes
errorTextintoToolOutput, buthandleToolCallwas catching failures and sending"error handling tool call ..."as a normaloutput. That makes parse failures, unavailable tools, sandbox errors, and thrown client-tool failures look like successful tool output in the transcript.This change preserves the existing success path while making failures first-class UI errors, so the chat can show the red error block and the next agent turn can distinguish a failed tool execution from a successful result string.
Closes #2766
Verification
npm exec --package bun@1.3.1 -- bun test packages/ai/test/tools/result.test.tsnpm exec --package bun@1.3.1 -- bun test packages/ai/test/stream/convert.test.tsnpm exec --package bun@1.3.1 -- bun --filter @onlook/web-client typecheckgit diff --checkNote:
bun --filter @onlook/ai typecheckstill fails on existing repo alias/test typing issues unrelated to this change (for example unresolved@/trpc/clientimports from web-client files referenced by the AI package and pre-existing test typing errors).Summary by CodeRabbit
Bug Fixes
Tests