-
Notifications
You must be signed in to change notification settings - Fork 911
fix(hooks): handle todowrite todos passed as string instead of array #625
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: dev
Are you sure you want to change the base?
fix(hooks): handle todowrite todos passed as string instead of array #625
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
I have read the CLA Document and I hereby sign the CLA |
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.
1 issue found across 1 file
Confidence score: 3/5
src/hooks/claude-code-hooks/index.tsassumes the parsed LLM response is always an array, so a valid JSON scalar like'null'or'{}'would slip through and likely break downstream logic.- Because this is a concrete parsing/validation gap, there’s a moderate chance of user-facing errors if an LLM returns unexpected JSON.
- Pay close attention to
src/hooks/claude-code-hooks/index.ts- add post-parse validation to ensure the value is actually an array.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/claude-code-hooks/index.ts">
<violation number="1" location="src/hooks/claude-code-hooks/index.ts:207">
P2: After `JSON.parse`, the result should be validated as an array. If an LLM sends a non-array JSON string (e.g., `'null'`, `'{}'`), parsing succeeds but produces a non-array value. The log message "parsed todos string to array" would be misleading, and it's inconsistent with the pattern in `todo.ts` which uses `Array.isArray()` after parsing.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ea08b5a to
f105ceb
Compare
Summary
todowritetool failing with "expected array, received string" error when LLMs pass todos as a JSON string instead of an arraytool.execute.beforehook to parse string input before Zod validationProblem
When invoking the
todowritetool, some LLM tool-calling interfaces pass thetodosparameter as a JSON string instead of an array. This caused failures like:The root cause: OpenCode's
TodoWriteTooluses Zod schemaz.array(...)for validation, but the LLM sometimes sends the array as a stringified JSON.Solution
Add a pre-validation check in
tool.execute.beforehook that detects whentodosis a string and parses it to an array before the tool execution.This is similar to the fix in #532 which handled
skill_mcparguments passed as object instead of string (opposite direction).Test Plan
bun run typecheckpassesbun run buildsucceedsSummary by cubic
Fixes todowrite failures when LLMs send todos as a JSON string. The before-execute hook now parses string todos to an array (with success/error logs) before Zod validation so the tool runs correctly.
Written for commit f105ceb. Summary will update on new commits.