-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: add type check before calling .match() on diffItem.content (#6905) #6906
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
Conversation
Fixes #6905 - Error during diff application "v.content.match is not a function" - Added type check to ensure diffItem.content is a string before calling .match() - Added comprehensive tests for handling non-string content values - Prevents runtime errors when content is null, undefined, or other non-string types
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
src/core/tools/multiApplyDiffTool.ts
Outdated
| const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length | ||
| totalSearchBlocks += searchBlocks | ||
| // Ensure content is a string before calling match | ||
| if (typeof diffItem.content === "string") { |
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.
Good defensive programming here! Though I wonder - should we also add a similar type check in the legacy applyDiffTool.ts at line 234? While it's currently protected by earlier validation, adding the same defensive check would ensure consistency and future-proof the code.
Also, is there a reason we're not checking for the same issue in the earlier loop around line 420 where we process diff items?
daniel-lxs
left a comment
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.
LGTM
It would be good to find out why this tool is returning content as something other than a string.
- Move type check to where content is first extracted from XML - Remove redundant check since content is now guaranteed to be string - Add test for early content validation
* main: (70 commits) fix: use native Ollama API instead of OpenAI compatibility layer (RooCodeInc#7137) feat: add support for OpenAI gpt-5-chat-latest model (RooCodeInc#7058) Make enhance with task history default to true (RooCodeInc#7140) Bump cloud version to 0.16.0 (RooCodeInc#7135) Release: v1.51.0 (RooCodeInc#7130) Add an API for resuming tasks by ID (RooCodeInc#7122) Add support for task page event population (RooCodeInc#7117) fix: add type check before calling .match() on diffItem.content (RooCodeInc#6905) (RooCodeInc#6906) Fix: Enable save button for provider dropdown and checkbox changes (RooCodeInc#7113) fix: Use cline.cwd as primary source for workspace path in codebaseSearchTool (RooCodeInc#6902) Hotfix multiple folder workspace checkpoint (RooCodeInc#6903) fix: prevent XML entity decoding in diff tools (RooCodeInc#7107) (RooCodeInc#7108) Refactor task execution system: improve call stack management (RooCodeInc#7035) Changeset version bump (RooCodeInc#7104) feat(web): fill missing SEO-related values (RooCodeInc#7096) Update contributors list (RooCodeInc#6883) Release v3.25.15 (RooCodeInc#7103) fix: add /evals page to sitemap generation (RooCodeInc#7102) feat: implement sitemap generation in TypeScript and remove XML file (RooCodeInc#6206) fix: reset condensing state when switching tasks (RooCodeInc#6922) ...
Fixes #6905
Problem
The application was throwing a runtime error
"v.content.match is not a function"when attempting to apply diffs. This occurred whendiffItem.contentwas not a string (could be null, undefined, or another type), but the code was calling.match()on it without type checking.Solution
Added a type check to ensure
diffItem.contentis a string before calling.match()method on it. This prevents the runtime error and allows the diff application to handle non-string content gracefully.Changes
typeof diffItem.content === 'string'check insrc/core/tools/multiApplyDiffTool.tsbefore calling.match()src/core/tools/__tests__/multiApplyDiffTool.spec.tsto verify the fix handles:Testing
Important
Adds type check in
applyDiffTool()to prevent runtime errors whendiffItem.contentis not a string, with comprehensive tests for various content types.typeof diffItem.content === 'string'check inapplyDiffTool()inmultiApplyDiffTool.tsto prevent runtime errors whendiffItem.contentis not a string.diffItem.contentgracefully by skipping invalid entries.multiApplyDiffTool.spec.tsto verify handling of undefined, null, numeric, and mixed content types.This description was created by
for 55f0ab1. You can customize this summary. It will automatically update as commits are pushed.