fix: use default variant for update-task/update-subtask with --research flag#1692
Conversation
…ch flag (fixes eyaltoledano#1689) The update-task and update-subtask commands were requesting a non-existent 'research' prompt variant. When useResearch is true they now use the 'default' variant, which already contains {{#if useResearch}} conditionals to render research-specific content, avoiding a crash: "Cannot read properties of undefined (reading 'replace')". Also adds a regression test to prevent recurrence.
🦋 Changeset detectedLatest commit: 9b24e71 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis pull request fixes a bug where Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/modules/task-manager/update-subtask-by-id.js (1)
305-310: Fix is correct; consider adding a symmetric regression test forupdate-subtask.Forcing
variantKey = 'default'resolves the crash, and theupdate-subtask.jsondefaulttemplate already handlesuseResearchvia{{#ifuseResearch}}conditionals, so research-backed output is preserved. The AI service role on line 312 correctly remains tied touseResearch.Only
update-task-by-id.test.jsgained a regression test — it would be worth mirroring it forupdate-subtask-by-id.jsso a future refactor doesn't silently reintroduce a missing-variant lookup here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/modules/task-manager/update-subtask-by-id.js` around lines 305 - 310, Add a symmetric regression test for update-subtask to mirror update-task-by-id.test.js: create a new test (e.g., update-subtask-by-id.test.js) that simulates the missing-variant lookup on promptManager.loadPrompt for update-subtask and asserts the handler in update-subtask-by-id.js completes without throwing and preserves research-backed output (exercise the {{`#if` useResearch}} path); stub/mock promptManager.loadPrompt and AI service role to reproduce the original crash case and verify variantKey falls back to 'default' and behavior matches the existing update-task test expectations.tests/unit/scripts/modules/task-manager/update-task-by-id.test.js (1)
688-741: Good regression test; consider also assertinguseResearchflows intopromptParams.The test correctly locks in the
'default'variant key foruseResearch: true. As a low-cost hardening, also assert thatpromptParams.useResearch === trueis passed — that would catch a future regression where the variant is still'default'butuseResearchis accidentally dropped from the params (which would break the template's{{#ifuseResearch}}branches and silently lose research behavior).♻️ Optional hardening
const promptManagerInstance = getPromptManager.mock.results[0].value; const loadPromptCall = promptManagerInstance.loadPrompt.mock.calls[0]; expect(loadPromptCall[2]).toBe('default'); + // Guard against useResearch being dropped from promptParams, which would + // silently disable the {{`#if` useResearch}} branches inside the 'default' template. + expect(loadPromptCall[1]).toEqual( + expect.objectContaining({ useResearch: true }) + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/scripts/modules/task-manager/update-task-by-id.test.js` around lines 688 - 741, Add an assertion that the prompt parameters include useResearch=true so the template sees the flag: after retrieving promptManagerInstance and loadPromptCall (as in the existing test where loadPromptCall[2] is the variant), check that loadPromptCall[3].useResearch === true (or the correct argument index for promptParams in loadPrompt calls) to ensure updateTaskById passes useResearch into promptParams when calling getPromptManager().loadPrompt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/modules/task-manager/update-subtask-by-id.js`:
- Around line 305-310: Add a symmetric regression test for update-subtask to
mirror update-task-by-id.test.js: create a new test (e.g.,
update-subtask-by-id.test.js) that simulates the missing-variant lookup on
promptManager.loadPrompt for update-subtask and asserts the handler in
update-subtask-by-id.js completes without throwing and preserves research-backed
output (exercise the {{`#if` useResearch}} path); stub/mock
promptManager.loadPrompt and AI service role to reproduce the original crash
case and verify variantKey falls back to 'default' and behavior matches the
existing update-task test expectations.
In `@tests/unit/scripts/modules/task-manager/update-task-by-id.test.js`:
- Around line 688-741: Add an assertion that the prompt parameters include
useResearch=true so the template sees the flag: after retrieving
promptManagerInstance and loadPromptCall (as in the existing test where
loadPromptCall[2] is the variant), check that loadPromptCall[3].useResearch ===
true (or the correct argument index for promptParams in loadPrompt calls) to
ensure updateTaskById passes useResearch into promptParams when calling
getPromptManager().loadPrompt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a553e35-3f4b-4b57-b2d2-36ac390e3967
📒 Files selected for processing (4)
.changeset/fix-update-task-research-variant.mdscripts/modules/task-manager/update-subtask-by-id.jsscripts/modules/task-manager/update-task-by-id.jstests/unit/scripts/modules/task-manager/update-task-by-id.test.js
1 participant
Fixes #1689
Problem
When running
task-master update-task --id=<n> --research --prompt="...", the command crashes immediately with:The same error also affects
update-subtask --research.Root cause: Both
update-task-by-id.jsandupdate-subtask-by-id.jscomputedvariantKey = 'research'whenuseResearchistrue. However, neitherupdate-task.jsonnorupdate-subtask.jsondefine a'research'variant (onlyexpand-task.jsondoes). Accessingtemplate.prompts['research']returnsundefined, so spreading it produces{}with nosystemoruserkeys — causingrenderTemplate(undefined, …)to crash.Solution
Use the
'default'variant in both files whenuseResearchistrue. Thedefaultvariant in both templates already contains{{#if useResearch}}/{{#if (not useResearch)}}Handlebars conditionals that produce the appropriate research-aware prompt. No changes to the prompt templates are needed.update-task-by-id.js: simplifiedvariantKeyselection toappendMode ? 'append' : 'default'update-subtask-by-id.js: simplified to always'default'Testing
update-task-by-id.test.jsthat verifiesloadPromptis called with'default'(not'research') whenuseResearchistrue.npm test -- --testPathPattern=update-task-by-id: 13/13 passing).npm run format-checkpasses (biome reports no issues).Note
Low Risk
Low risk: this is a small change to prompt variant selection to avoid loading a non-existent template variant, plus a regression test to lock the behavior in.
Overview
Fixes
update-taskandupdate-subtaskcrashing when run with--researchby stopping use of a non-existentresearchprompt variant and always loading thedefaultvariant (while still using the research AI role when requested). Adds a regression unit test to assertupdate-taskcallsloadPromptwithdefaultwhenuseResearchistrue, and includes a patch changeset documenting the fix.Reviewed by Cursor Bugbot for commit 9b24e71. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests