-
Notifications
You must be signed in to change notification settings - Fork 120
fix: add comprehensive parameter validation and error handling for all tools #542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix: add comprehensive parameter validation and error handling for all tools #542
Conversation
…l tools - Add unified parameter validation system using Zod schemas - Implement defensive programming in all tool getDescription methods - Fix potential crashes from type coercion issues (e.g., .trim() on non-strings) - Add sanitization for common parameter type mismatches - Enhance AskUserQuestion tool schema validation for array parameters - Add comprehensive error messages for LLM auto-retry mechanism This fixes issues where LLM might return incorrect parameter types, causing crashes like "A.trim is not a function" and "Q.sort is not a function". The fix ensures graceful error handling with clear feedback for automatic retry. Affected tools: - write, edit, read, ls (add cwd type checking + try-catch) - glob, grep, fetch (add try-catch protection) - bash, bash_output, kill_bash (fix .trim() call + add protection) - askUserQuestion (enhance array validation) Changes: - src/tool.ts: Add unified parameter validation before tool execution - src/utils/validateToolParams.ts: New validation and sanitization utilities - src/tools/*.ts: Add defensive checks in getDescription functions
217cd7b to
e2d4732
Compare
src/tools/bash.ts
Outdated
| } | ||
| return `Read output from task: ${params.task_id}`; | ||
| try { | ||
| return `Read output from task: ${String(params.task_id)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为啥需要 try
src/tools/fetch.ts
Outdated
| } | ||
| return params.url; | ||
| try { | ||
| return String(params.url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果校验的话 感觉直接 zod 校验就行了 之前加上了 zod 校验 但是模型需要重试比较多次 才能解决问题 后续的想法是 在 toolUse 对常见的幻觉进行兜底
|
|
||
| let result; | ||
| try { | ||
| result = schema.safeParse(params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个方案之前合入了 但是如果模型固执的不调整 容易死循环
Based on code review feedback: 1. Enhanced error messages in validateToolParams: - Add detailed type mismatch information (expected vs received) - Include specific conversion suggestions for common cases - Provide clear guidance for LLM to correct parameters - Support validation with allowed values list 2. Remove unnecessary try-catch blocks: - Remove try-catch around String() conversions (never throws) - Affected files: bash.ts, fetch.ts, glob.ts, grep.ts - Keep necessary try-catch for path.relative() calls 3. Add TODO comment for future retry limit: - Document current behavior: retries up to maxTurns (50) - Suggest future improvement: detect consecutive identical errors - Recommend implementing in loop.ts with 3-attempt limit Changes address reviewer concerns about: - Unnecessary defensive programming in getDescription - Need for better error messages to help LLM self-correct - Awareness of potential infinite retry loops (to be addressed later)
ef8a161 to
19a0d01
Compare
|
感谢审核,已经更新了代码。 关于 try-catch:确实不需要,String() 转换不会抛异常。已经移除了 bash.ts、fetch.ts、glob.ts、grep.ts 中多余的 try-catch,只保留了 path.relative() 这种可能抛异常的。 关于验证策略:"兜底幻觉"这部分,我看了下 Claude Code 和 OpenCode 的做法,现在调整为:
关于死循环:这个问题很重要。当前确实可能重试最多 50 次,不过加强了错误返回,帮助LLM更好的返回正确参数,重试这部分,参考 Claude Code 的做法,建议检测连续 3 次相同参数的失败就快速 fail(而不是等 50 轮)。但这需要改 loop.ts,超出当前 PR 范围了。 我在 tool.ts 也加了 TODO 注释说明这个改进点,看后面是否需要按这个逻辑处理。 |

Problem
When LLM returns incorrect parameter types, the tool execution crashes with errors like:
A.trim is not a function(when non-string passed to path.relative)Q.sort is not a function(when non-array passed to array methods)These crashes interrupt the user session and require manual intervention.
Solution
Implemented a comprehensive 3-layer defense system:
Layer 1: Unified Parameter Validation (src/tool.ts)
isError: trueflagLayer 2: Defensive Programming (all 11 tools)
getDescriptionmethods to prevent crashes during UI renderingLayer 3: Robust Validation System (src/utils/validateToolParams.ts)
Affected Tools
Impact
Testing
Tested with various parameter type mismatches:
All scenarios now handle gracefully with automatic retry or safe degradation.
Files Changed
src/tool.ts: Unified validation layersrc/utils/validateToolParams.ts: New validation utilities (with tests)src/tools/*.ts: Defensive checks in 11 tools