fix(hooks): handle todowrite todos passed as string instead of array#625
Closed
SJY0917032 wants to merge 1 commit intocode-yeongyu:devfrom
Closed
fix(hooks): handle todowrite todos passed as string instead of array#625SJY0917032 wants to merge 1 commit intocode-yeongyu:devfrom
SJY0917032 wants to merge 1 commit intocode-yeongyu:devfrom
Conversation
Contributor
|
All contributors have signed the CLA. Thank you! ✅ |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Author
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
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
3 tasks
Collaborator
kdcokenny
pushed a commit
that referenced
this pull request
Jan 13, 2026
sssgun
pushed a commit
to sssgun/oh-my-opencode
that referenced
this pull request
Jan 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.