fix: return correct target tag in add_task response#1669
fix: return correct target tag in add_task response#1669withsivram wants to merge 1 commit intoeyaltoledano:mainfrom
Conversation
…no#1638) The add_task MCP tool was returning the previously active tag (from state.json) in the response instead of the resolved target tag that the task was actually added to. This caused agents to take incorrect corrective actions based on misleading tag information. The fix passes the already-resolved tag to handleApiResult, which has built-in support for an explicit tag parameter that takes precedence over reading from state.json. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughThe PR broadens task ID type definitions from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix incorrect tag reporting in MCP add_task responses by ensuring responses reflect the resolved target tag (not the active tag from .taskmaster/state.json), and adds regression tests around handleApiResult tag behavior. However, the diff also includes broad tm-core task ID typing/storage normalization changes that are not described in the PR metadata and appear unrelated to issue #1638.
Changes:
- Add Vitest regression tests for
handleApiResulttag field precedence behavior. - Widen core
Task/Subtask/UpdateTaskID types tonumber | string. - Change file-storage normalization to coerce task IDs and subtask parent IDs to numbers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/mcp/src/shared/utils.spec.ts | Adds unit tests asserting handleApiResult uses an explicit tag over the state.json active tag. |
| packages/tm-core/src/common/types/index.ts | Broadens task/subtask ID types to `number |
| packages/tm-core/src/testing/task-fixtures.ts | Changes test fixtures to coerce task IDs to Number(...). |
| packages/tm-core/src/modules/storage/adapters/file-storage/format-handler.ts | Normalizes stored task IDs / subtask parent IDs to numbers. |
| packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts | Normalizes IDs to numbers when saving/updating tasks in file storage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /** | ||
| * Normalize task IDs - keep Task IDs as strings, Subtask IDs as numbers | ||
| * Normalize task IDs - Task IDs and Subtask IDs are both numbers for file storage |
There was a problem hiding this comment.
normalizeTasks now coerces IDs to numbers, but convertToSaveFormat earlier in this file still has a comment saying “Normalize task IDs to strings”. Please update that nearby documentation to match the new numeric-ID normalization so the file doesn’t contain contradictory guidance.
| * Normalize task IDs - Task IDs and Subtask IDs are both numbers for file storage | |
| * Normalize task and subtask IDs to numbers for file storage (dependency IDs remain strings) |
| /** | ||
| * Normalize task IDs - keep Task IDs as strings, Subtask IDs as numbers | ||
| * Normalize task IDs - Task IDs and Subtask IDs are both numbers for file storage | ||
| * Note: Uses spread operator to preserve all task properties including user-defined metadata | ||
| */ | ||
| private normalizeTaskIds(tasks: Task[]): Task[] { |
There was a problem hiding this comment.
This PR is titled/described as a fix for the MCP add_task response tag field, but this file introduces a behavior change that normalizes stored task IDs/parentIds to numbers instead of strings. That’s a potentially breaking storage-format change and seems unrelated to issue #1638—please either split these ID-normalization changes into a separate PR (with migration/compat notes) or update the PR description to cover and justify this change.
| it('should use provided tag instead of reading from state.json', async () => { | ||
| const result = await handleApiResult({ | ||
| result: { | ||
| success: true, | ||
| data: { taskId: 5, message: 'Success' } | ||
| }, | ||
| projectRoot: '/mock/project', | ||
| tag: 'target-tag' | ||
| }); |
There was a problem hiding this comment.
The PR description/title indicates the add_task response tag bug is fixed by passing the resolved target tag into handleApiResult, but in the current codebase the add_task tool (mcp-server/src/tools/add-task.js) still calls handleApiResult without supplying tag: resolvedTag, so responses will continue to fall back to state.json. These tests only validate handleApiResult’s precedence rules, not that add_task actually passes the resolved tag—please add the call-site change (and ideally a tool-level regression test) so the reported bug is truly fixed.
| export interface Task extends TaskImplementationMetadata { | ||
| id: string; | ||
| id: number | string; | ||
| title: string; | ||
| description: string; | ||
| status: TaskStatus; |
There was a problem hiding this comment.
Task.id is changed to number | string, but the runtime type guard isTask further down in this same file still checks typeof task.id === 'string' (and isSubtask still assumes parentId is a string). With the new union types (and with file-storage/fixtures now coercing IDs to numbers), these guards will incorrectly reject valid tasks/subtasks. Please update the guards (and any other ID validators) to accept the new ID shapes, or keep IDs consistently as strings.
| export interface Subtask extends Omit<Task, 'id' | 'subtasks'> { | ||
| id: number | string; | ||
| parentId: string; | ||
| parentId: number | string; |
There was a problem hiding this comment.
Subtask.parentId is widened to number | string, but isSubtask later in this file still checks typeof subtask.parentId === 'string'. Please keep the type guard consistent with this new union (or revert this type change) to avoid runtime validation rejecting subtasks that were normalized to numeric parent IDs.
| parentId: number | string; | |
| parentId: string; |
| ): Task { | ||
| return { | ||
| id: String(overrides.id), | ||
| id: Number(overrides.id), |
There was a problem hiding this comment.
createTask now coerces overrides.id via Number(...) even though the input type allows string. If a caller passes a non-numeric ID (e.g., UUIDs for API-storage tests), the fixture will silently produce id: NaN and create invalid task objects. Either restrict this fixture to numeric IDs (update the parameter type + doc comment) or keep IDs as strings/leave them uncoerced so fixtures can represent both storage backends safely.
| id: Number(overrides.id), | |
| id: String(overrides.id), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tm-core/src/common/types/index.ts (2)
296-311:⚠️ Potential issue | 🟠 MajorType guard
isTaskis inconsistent with widenedTask.idtype.
Task.idwas widened tonumber | string(line 132), butisTaskstill validatestypeof task.id === 'string'(line 301). This means valid tasks with numeric IDs will fail the type guard check.🔧 Suggested fix
export function isTask(obj: unknown): obj is Task { if (!obj || typeof obj !== 'object') return false; const task = obj as Record<string, unknown>; return ( - typeof task.id === 'string' && + (typeof task.id === 'string' || typeof task.id === 'number') && typeof task.title === 'string' &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tm-core/src/common/types/index.ts` around lines 296 - 311, Update the isTask type guard to accept the widened Task.id type (number | string) instead of only strings; in the isTask function change the id check from typeof task.id === 'string' to a check that allows both typeof task.id === 'string' || typeof task.id === 'number' so numeric IDs pass the guard and the function still narrows to Task.
316-329:⚠️ Potential issue | 🟠 MajorFix
isSubtaskandisTaskguards to accept widened type unions.The type guards are overly restrictive and will reject valid task and subtask objects:
isSubtask(line 321): Only acceptsnumberforid, butSubtask.idisnumber | stringisSubtask(line 322): Only acceptsstringforparentId, butSubtask.parentIdisnumber | stringisTask(line 301): Only acceptsstringforid, butTask.idisnumber | stringValid objects created with API-provided string IDs or numeric parentIds will fail these guards.
Suggested fixes
For
isSubtask(lines 321-322):- typeof subtask.id === 'number' && - typeof subtask.parentId === 'string' && + (typeof subtask.id === 'number' || typeof subtask.id === 'string') && + (typeof subtask.parentId === 'number' || typeof subtask.parentId === 'string') &&For
isTask(line 301):- typeof task.id === 'string' && + (typeof task.id === 'number' || typeof task.id === 'string') &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tm-core/src/common/types/index.ts` around lines 316 - 329, The type guards are too narrow: update isSubtask and isTask to accept the union types used by the models (allow both 'number' and 'string' where ids can be number|string and parentId can be number|string) — specifically change the typeof checks in isSubtask (currently testing subtask.id === 'number' and subtask.parentId === 'string') to accept 'number' or 'string', and change the typeof check in isTask (currently testing id === 'string') to accept 'number' or 'string'; keep the other checks (title, description, status/priority validators, and the absence of 'subtasks' for subtask) intact so the guards still validate shape.
🧹 Nitpick comments (3)
packages/tm-core/src/testing/task-fixtures.ts (2)
44-57: Update documentation to reflect the new behavior.The comment on line 44 states "id: Converted to string if number is provided" but the implementation now converts to
Number()(line 57). This is misleading.📝 Suggested fix
/** * Creates a valid task with all required fields * * DEFAULTS: - * - id: Converted to string if number is provided + * - id: Converted to number (file storage uses numeric IDs) * - status: 'pending'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tm-core/src/testing/task-fixtures.ts` around lines 44 - 57, The doc comment for createTask is out of sync: it says "id: Converted to string if number is provided" while the implementation uses Number(overrides.id). Update the documentation block above the createTask function to state that id is converted to a number (e.g., "id: Converted to number if string is provided" or "id: Normalized to a Number") so the comment matches the createTask behavior.
132-134: Inconsistent ID coercion betweencreateTaskandcreateSubtask.
createTasknow coercesidtoNumber()(line 57), butcreateSubtaskstill assignsiddirectly without coercion (line 133). This creates inconsistency in test fixtures.For file storage, subtask IDs are also expected to be numbers (see
format-handler.tsline 230). Consider applying the same treatment for consistency.♻️ Suggested fix
return { - id: overrides.id, + id: Number(overrides.id), parentId: overrides.parentId ?? defaultParentId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tm-core/src/testing/task-fixtures.ts` around lines 132 - 134, createSubtask assigns id directly while createTask coerces ids with Number(), causing inconsistent types; update createSubtask so its returned id is coerced to a number (e.g., id: Number(overrides.id)) and likewise ensure parentId follows the same numeric coercion pattern (e.g., parentId: Number(overrides.parentId ?? defaultParentId)) to match createTask and format-handler expectations.packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts (1)
288-300: Duplicate normalization logic betweenFileStorageandFormatHandler.The
normalizeTaskIdsmethod duplicates the exact same logic found inFormatHandler.normalizeTasks(lines 222-234 offormat-handler.ts). This violates DRY and creates maintenance risk if one is updated without the other.Consider removing this method and delegating to
this.formatHandler.normalizeTasks()instead, or extracting a shared utility function.♻️ Suggested refactor
- private normalizeTaskIds(tasks: Task[]): Task[] { - return tasks.map((task) => ({ - ...task, - id: Number(task.id), // Task IDs are numbers - dependencies: task.dependencies?.map((dep) => String(dep)) || [], - subtasks: - task.subtasks?.map((subtask) => ({ - ...subtask, - id: Number(subtask.id), // Subtask IDs are numbers - parentId: Number(subtask.parentId) // Parent ID is number (Task ID) - })) || [] - })); - }Then update
saveTasksto use the format handler:- const normalizedTasks = this.normalizeTaskIds(tasks); + const normalizedTasks = this.formatHandler.normalizeTasks(tasks);Note: This requires making
normalizeTaskspublic inFormatHandler.Based on learnings: "Do not duplicate task ID formatting logic across modules - centralize formatting utilities"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts` around lines 288 - 300, The normalizeTaskIds method in FileStorage duplicates logic already implemented in FormatHandler.normalizeTasks; remove FileStorage.normalizeTaskIds and update any callers (e.g., FileStorage.saveTasks) to delegate to this.formatHandler.normalizeTasks(...) instead, making FormatHandler.normalizeTasks public if necessary; ensure that dependencies on return shape remain identical and adjust imports/types only if visibility changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/tm-core/src/common/types/index.ts`:
- Around line 296-311: Update the isTask type guard to accept the widened
Task.id type (number | string) instead of only strings; in the isTask function
change the id check from typeof task.id === 'string' to a check that allows both
typeof task.id === 'string' || typeof task.id === 'number' so numeric IDs pass
the guard and the function still narrows to Task.
- Around line 316-329: The type guards are too narrow: update isSubtask and
isTask to accept the union types used by the models (allow both 'number' and
'string' where ids can be number|string and parentId can be number|string) —
specifically change the typeof checks in isSubtask (currently testing subtask.id
=== 'number' and subtask.parentId === 'string') to accept 'number' or 'string',
and change the typeof check in isTask (currently testing id === 'string') to
accept 'number' or 'string'; keep the other checks (title, description,
status/priority validators, and the absence of 'subtasks' for subtask) intact so
the guards still validate shape.
---
Nitpick comments:
In `@packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts`:
- Around line 288-300: The normalizeTaskIds method in FileStorage duplicates
logic already implemented in FormatHandler.normalizeTasks; remove
FileStorage.normalizeTaskIds and update any callers (e.g.,
FileStorage.saveTasks) to delegate to this.formatHandler.normalizeTasks(...)
instead, making FormatHandler.normalizeTasks public if necessary; ensure that
dependencies on return shape remain identical and adjust imports/types only if
visibility changes are required.
In `@packages/tm-core/src/testing/task-fixtures.ts`:
- Around line 44-57: The doc comment for createTask is out of sync: it says "id:
Converted to string if number is provided" while the implementation uses
Number(overrides.id). Update the documentation block above the createTask
function to state that id is converted to a number (e.g., "id: Converted to
number if string is provided" or "id: Normalized to a Number") so the comment
matches the createTask behavior.
- Around line 132-134: createSubtask assigns id directly while createTask
coerces ids with Number(), causing inconsistent types; update createSubtask so
its returned id is coerced to a number (e.g., id: Number(overrides.id)) and
likewise ensure parentId follows the same numeric coercion pattern (e.g.,
parentId: Number(overrides.parentId ?? defaultParentId)) to match createTask and
format-handler expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca92efdc-3753-4950-b9be-e839b0469fb0
📒 Files selected for processing (5)
apps/mcp/src/shared/utils.spec.tspackages/tm-core/src/common/types/index.tspackages/tm-core/src/modules/storage/adapters/file-storage/file-storage.tspackages/tm-core/src/modules/storage/adapters/file-storage/format-handler.tspackages/tm-core/src/testing/task-fixtures.ts
Summary
add_taskMCP tool responsetagfield was showing the previously active tag (fromstate.json/use_tag()state) instead of the resolved target tag the task was actually added toresolvedTagtohandleApiResult, which has built-intagparameter support that takes precedence over reading fromstate.jsonhandleApiResulttag field behavior inapps/mcp/src/shared/utils.spec.tsFixes #1638
Test plan
use_tag({name: "tag-a"}), thenadd_task({title: "test", tag: "tag-b"})and confirm response shows"tag-b"🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests