-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: resolve newTaskRequireTodos setting not working correctly #7363
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
- Use dynamic Package.name instead of hardcoded namespace values - Show todos parameter as optional/required based on setting value - Remove hardcoded new_task example from shared tool use section - Update tests to use Package.name pattern The setting now works correctly for both regular and nightly builds without requiring hardcoded namespace values.
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.
Pull Request Overview
This PR fixes the newTaskRequireTodos VSCode setting that wasn't working correctly due to hardcoded configuration namespaces and improves the tool description clarity.
- Replaces hardcoded configuration namespaces with dynamic
Package.nameto work across different builds - Updates tool description to always show the
todosparameter with clear optional/required indicators - Removes confusing hardcoded example from shared tool use section
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/tools/newTaskTool.ts |
Uses dynamic Package.name for VSCode configuration access |
src/core/tools/__tests__/newTaskTool.spec.ts |
Adds Package module mock and updates test descriptions |
src/core/prompts/tools/new-task.ts |
Refactors tool description to always show todos parameter with appropriate labeling |
src/core/prompts/tools/__tests__/new-task.spec.ts |
Updates tests to verify new optional/required parameter behavior |
src/core/prompts/sections/tool-use.ts |
Removes hardcoded new_task example to eliminate confusion |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/core/prompts/tools/new-task.ts
Outdated
| : "" | ||
| } | ||
| </new_task> | ||
| ${ | ||
| !todosRequired | ||
| ? `Example with optional todos: |
Copilot
AI
Aug 23, 2025
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.
[nitpick] The conditional logic for the example message and todos section creates complex template literal nesting that's difficult to read and maintain. Consider extracting the example generation into separate helper functions or variables to improve readability.
src/core/prompts/tools/new-task.ts
Outdated
| : "" | ||
| }` |
Copilot
AI
Aug 23, 2025
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.
[nitpick] The additional conditional example section further complicates the template literal structure. This nested conditional logic makes the function harder to understand and maintain. Consider restructuring to use a more straightforward approach with separate string building or helper functions.
| : "" | |
| }` | |
| } | |
| const description = [ | |
| "## new_task", | |
| `Description: This will let you create a new task instance in the chosen mode using your provided message${todosRequired ? " and initial todo list" : ""}.`, | |
| "", | |
| "Parameters:", | |
| "- mode: (required) The slug of the mode to start the new task in (e.g., \"code\", \"debug\", \"architect\").", | |
| "- message: (required) The initial user message or instructions for this new task.", | |
| getTodosParamDescription(todosRequired), | |
| "", | |
| "Usage:", | |
| "<new_task>", | |
| "<mode>your-mode-slug-here</mode>", | |
| "<message>Your initial instructions here</message>" + getTodosUsageBlock(todosRequired), | |
| "</new_task>", | |
| "", | |
| "Example:", | |
| "<new_task>", | |
| "<mode>code</mode>", | |
| `<message>${todosRequired ? "Implement user authentication" : "Implement a new feature for the application"}</message>` + getTodosExampleBlock(todosRequired), | |
| "</new_task>", | |
| "", | |
| getOptionalTodosExampleBlock(todosRequired) | |
| ].join("\n"); | |
| return description; |
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.
Thank you for your contribution! I've reviewed the changes and the fix correctly addresses the newTaskRequireTodos setting issue. The implementation successfully resolves the namespace mismatch problem and improves the tool description clarity.
Review Findings
✅ What works well:
- The use of
Package.namefor dynamic namespace resolution is an excellent solution that handles different build variants - Tests are comprehensive and cover the main functionality
- The change to always show the todos parameter with clear optional/required indicators improves clarity
💡 Suggestions for improvement:
1. Code readability in src/core/prompts/tools/new-task.ts
I agree with Copilot's existing feedback - the nested template literals with multiple ternary operators (lines 18-61) create complex, hard-to-maintain code. Consider refactoring this into helper functions or building the string incrementally.
2. Potential race condition in src/core/tools/newTaskTool.ts (line 136)
The hardcoded 500ms delay after mode switching might not be sufficient in all cases. Consider implementing a more robust wait mechanism that checks for completion rather than using a fixed delay.
3. Documentation improvement for src/core/tools/newTaskTool.ts (line 61-62)
Consider adding a comment explaining why Package.name is used instead of hardcoded values to help future maintainers understand the dynamic namespace requirement for different build variants.
4. Test coverage enhancement in src/core/tools/__tests__/newTaskTool.spec.ts
Consider adding test cases with different Package.name values (e.g., 'roo-code-nightly') to ensure the dynamic namespace resolution works correctly for all build variants.
Overall, this is a solid fix that addresses the core issue effectively. The suggestions above would further improve code maintainability and robustness.
The snapshots needed updating because the hardcoded new_task example was removed from the shared tool use section
The setting name should not include the namespace prefix in package.json as VSCode automatically adds the extension's namespace. This was preventing the setting from appearing in the VSCode settings UI.
…ttings UI visibility
…est(newTaskTool): verify config uses Package.name variant (roo-code-nightly)
- Replace complex template literals with two complete prompt constants - Remove nested ternary operators for better readability - Hide todos parameter completely when disabled (not shown as optional) - Update tests to reflect new behavior - Reduce code from 105 to 66 lines for better maintainability
The todos parameter is now conditionally required based on the newTaskRequireTodos setting, so the snapshots needed to be updated to reflect the new tool documentation format.
Description
This PR fixes the
newTaskRequireTodosVSCode setting that wasn't working correctly, particularly for the nightly build.Problem
The
newTaskRequireTodossetting was not enforcing the requirement for thetodosparameter in thenew_tasktool when set totrue. This was due to:roo-cline.newTaskRequireTodosandroo-code-nightly.newTaskRequireTodos) but the actual namespace varies between buildstodosparameter was completely hidden when the setting was false, instead of being shown as optionalnew_taskexample that always includedtodos, causing confusionSolution
1. Dynamic Configuration Namespace
Package.namefromsrc/shared/package.tswhich dynamically gets the correct package name based on the build2. Improved Tool Description
todosparameter is now always visible in the tool description(optional)whennewTaskRequireTodosis false(required)whennewTaskRequireTodosis true3. Removed Hardcoded Example
new_taskexample from the shared tool use section to avoid confusionChanges Made
src/core/tools/newTaskTool.ts- Use dynamic Package.name for configurationsrc/core/prompts/tools/new-task.ts- Show todos as optional/required based on settingsrc/core/prompts/sections/tool-use.ts- Remove hardcoded new_task examplesrc/core/tools/__tests__/newTaskTool.spec.ts- Update tests to use Package.namesrc/core/prompts/tools/__tests__/new-task.spec.ts- Update tests for new behaviorTesting
Verification
To verify this fix:
newTaskRequireTodostotruein VSCode settingsfalseand the todos parameter becomes optionalImportant
Fixes
newTaskRequireTodossetting in VSCode by using dynamic configuration namespace and adjusting tool descriptions and examples.newTaskRequireTodossetting in VSCode to correctly enforcetodosparameter requirement innew_tasktool.Package.namefrompackage.tsfor dynamic configuration namespace, supporting both regular and nightly builds.todosparameter visibility innew_tasktool description based on setting.new_taskexample from shared tool use section.newTaskTool.ts: Implements dynamic namespace and checksnewTaskRequireTodossetting.new-task.ts: Updates prompt to showtodosas optional/required.tool-use.ts: Removes staticnew_taskexample.newTaskTool.spec.tsandnew-task.spec.tsto cover new behavior and dynamic namespace usage.This description was created by
for 7ffe015. You can customize this summary. It will automatically update as commits are pushed.