fix: normalize task ID types in tasks.json writes#1670
fix: normalize task ID types in tasks.json writes#1670withsivram wants to merge 2 commits intoeyaltoledano:mainfrom
Conversation
…ixes eyaltoledano#1647) When moving tasks between tags, if a task's ID already exists in the target tag, the task is now automatically renumbered to the next available ID instead of throwing an error. Dependencies within the moved batch are updated to reflect the new IDs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…#1583) The file-storage adapter and format-handler were converting task IDs to strings via String(), while the CLI scripts (utils.js) normalized them to numbers via parseInt(). This caused inconsistent ID types in tasks.json depending on which command last updated a task. Changes: - file-storage.ts normalizeTaskIds: String(task.id) -> Number(task.id) - file-storage.ts updateTask: String(taskId) -> Number(taskId) - format-handler.ts normalizeTasks: String(task.id) -> Number(task.id) - Both: String(subtask.parentId) -> Number(subtask.parentId) - TypeScript types: Task.id, Subtask.parentId, PlaceholderTask.id, UpdateTask.id updated from string to number | string - task-fixtures.ts: String(overrides.id) -> Number(overrides.id) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughThis PR implements auto-renumbering for task IDs during cross-tag moves when conflicts occur. It normalizes task identifiers as numeric values in storage, updates type definitions to support both numeric and string IDs, and enhances the move-task logic to track renumbering via ID mapping while updating internal dependencies accordingly. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant MoveTask as Move Logic
participant Storage
participant FileSystem as File System
User->>CLI: Move task across tags (ID collision)
CLI->>MoveTask: executeMoveOperation()
MoveTask->>MoveTask: Check ID collision in target tag
alt ID Conflict
MoveTask->>MoveTask: getNextAvailableId() → newId
MoveTask->>MoveTask: Track in idRemapping: oldId → newId
MoveTask->>MoveTask: Update task.id to newId
end
MoveTask->>MoveTask: Update internal subtask dependencies<br/>(parent.subId refs)
MoveTask->>MoveTask: Remap numeric dependencies<br/>in moved task batch
MoveTask->>Storage: Persist moved tasks (with newId)
Storage->>FileSystem: writeJSON(target-tag tasks)
MoveTask->>MoveTask: Build success message<br/>with renumbering summary
MoveTask->>CLI: Return response + movedTasks<br/>(originalId, newId fields)
CLI->>User: Display success message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 make tasks.json writes consistent by normalizing task/subtask IDs to numeric form across file-storage code paths, and updates shared TypeScript types/fixtures accordingly. It also changes cross-tag move behavior to auto-renumber tasks on ID collisions and surfaces renumbering details in responses/messages.
Changes:
- Normalize
Task.idandSubtask.parentIdto numbers when writing via tm-core file-storage (file-storage.ts,format-handler.ts). - Broaden ID-related TypeScript types to
number | stringand update tm-core test fixtures to align with numeric file-storage IDs. - Update cross-tag move logic/tests to auto-renumber on target-tag ID conflicts and include renumbering details in messages/results.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/move-task-cross-tag.integration.test.js | Updates expectations to assert auto-renumbering behavior on ID collisions. |
| scripts/modules/task-manager/move-task.js | Adds auto-renumbering on cross-tag ID conflicts plus dependency remapping & messaging. |
| packages/tm-core/src/testing/task-fixtures.ts | Updates fixture task ID coercion to numeric. |
| packages/tm-core/src/modules/storage/adapters/file-storage/format-handler.ts | Normalizes task/subtask IDs to numbers for file-storage save formatting. |
| packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts | Normalizes task/subtask IDs to numbers on save and update operations. |
| packages/tm-core/src/common/types/index.ts | Broadens ID types to `number |
| mcp-server/src/core/direct-functions/move-task-cross-tag.js | Adjusts returned message to include renumber details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (movedTask && Array.isArray(movedTask.subtasks)) { | ||
| movedTask.subtasks.forEach((subtask) => { | ||
| if (Array.isArray(subtask.dependencies)) { | ||
| subtask.dependencies = subtask.dependencies.map((dep) => { | ||
| const normalizedDep = normalizeDependency(dep); | ||
| if ( | ||
| Number.isFinite(normalizedDep) && | ||
| idRemapping.has(normalizedDep) | ||
| ) { | ||
| return idRemapping.get(normalizedDep); | ||
| } | ||
| return dep; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The post-move remapping of subtask dependencies uses normalizeDependency(), which strips dotted refs like "5.2" down to the parent ID (5). If a moved/renumbered task’s subtasks depend on another moved task’s specific subtask (e.g., "1.2"), this code will rewrite that to a bare number (e.g., 2) and lose the subtask portion, corrupting the dependency reference. Update the remapping logic to preserve dotted dependencies by only rewriting the parent portion when the dependency is in "parent.subId" form.
| // Apply the new ID to the task | ||
| taskToMove.id = assignedId; | ||
|
|
||
| // Update internal subtask dependency references that pointed to the old parent ID | ||
| if (Array.isArray(taskToMove.subtasks)) { | ||
| taskToMove.subtasks.forEach((subtask) => { | ||
| if (Array.isArray(subtask.dependencies)) { | ||
| subtask.dependencies = subtask.dependencies.map((dep) => { | ||
| if (typeof dep === 'string' && dep.includes('.')) { | ||
| const [depParent, depSub] = dep.split('.'); | ||
| if (parseInt(depParent, 10) === normalizedTaskId) { | ||
| return `${assignedId}.${depSub}`; | ||
| } | ||
| } | ||
| return dep; | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
When an ID collision is auto-renumbered, the parent task’s id is updated but subtasks’ stored parent pointer (scripts use parentTaskId) is not updated. This can leave subtasks claiming they belong to the old parent ID after the move/renumber, which will break helpers that rely on parentTaskId. When assignedId differs from the original, update each subtask.parentTaskId (if present) to assignedId as part of the renumbering step.
| let assignedId = normalizedTaskId; | ||
|
|
||
| if (existingTaskIndex !== -1) { | ||
| throw new MoveTaskError( | ||
| MOVE_ERROR_CODES.TASK_ALREADY_EXISTS, | ||
| `Task ${taskId} already exists in target tag "${targetTag}"`, | ||
| { | ||
| conflictingId: normalizedTaskId, | ||
| targetTag, | ||
| suggestions: [ | ||
| 'Choose a different target tag without conflicting IDs', | ||
| 'Move a different set of IDs (avoid existing ones)', | ||
| 'If needed, move within-tag to a new ID first, then cross-tag move' | ||
| ] | ||
| } | ||
| // ID collision detected — auto-renumber to the next available ID | ||
| assignedId = getNextAvailableId(rawData, targetTag, idRemapping); | ||
| idRemapping.set(normalizedTaskId, assignedId); | ||
| log( | ||
| 'info', | ||
| `Task ${normalizedTaskId} conflicts with existing ID in "${targetTag}", renumbered to ${assignedId}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
This PR’s description is about normalizing task ID types when writing tasks.json, but this change also alters cross-tag move semantics: instead of throwing TASK_ALREADY_EXISTS on ID collisions, it silently auto-renumbers and proceeds. That’s a user-visible behavior change and should be called out explicitly in the PR description (or split into a separate PR) so reviewers/users understand the new contract.
| private normalizeTaskIds(tasks: Task[]): Task[] { | ||
| return tasks.map((task) => ({ | ||
| ...task, | ||
| id: String(task.id), // Task IDs are strings | ||
| 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: String(subtask.parentId) // Parent ID is string (Task ID) | ||
| parentId: Number(subtask.parentId) // Parent ID is number (Task ID) | ||
| })) || [] | ||
| })); |
There was a problem hiding this comment.
Coercing IDs with Number(...) can yield NaN for unexpected inputs (e.g., non-numeric string IDs). Since Task/Subtask IDs are now typed as number|string, this can silently write invalid NaN IDs into tasks.json. Consider validating that coerced IDs are finite (and ideally positive integers) and throwing a clear error if not, rather than persisting NaN.
| tasks[taskIndex] = { | ||
| ...existingTask, | ||
| ...updates, | ||
| ...(mergedSubtasks && { subtasks: mergedSubtasks }), | ||
| id: String(taskId) // Keep consistent with normalizeTaskIds | ||
| id: Number(taskId) // Keep consistent with normalizeTaskIds | ||
| }; |
There was a problem hiding this comment.
updateTask() now forces id: Number(taskId). If taskId is ever a non-numeric string, this will persist NaN as the task ID. Please add validation (finite / integer / >0) before assigning, and surface a helpful error when the ID cannot be represented as a numeric file-storage ID.
| private normalizeTasks(tasks: Task[]): Task[] { | ||
| return tasks.map((task) => ({ | ||
| ...task, | ||
| id: String(task.id), // Task IDs are strings | ||
| 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: String(subtask.parentId) // Parent ID is string (Task ID) | ||
| parentId: Number(subtask.parentId) // Parent ID is number (Task ID) | ||
| })) || [] | ||
| })); |
There was a problem hiding this comment.
Coercing IDs with Number(...) here can produce NaN for non-numeric string IDs, and convert "" to 0. Since Task/Subtask IDs are now number|string, it would be safer to validate (Number.isFinite, integer, >0) and throw (or otherwise handle) invalid IDs rather than writing invalid values to disk.
| export function createTask( | ||
| overrides: Partial<Omit<Task, 'id'>> & { id: number | string; title: string } | ||
| ): Task { | ||
| return { | ||
| id: String(overrides.id), | ||
| id: Number(overrides.id), | ||
| title: overrides.title, |
There was a problem hiding this comment.
The createTask fixture now coerces id with Number(...), but the doc comment immediately above still says the id is converted to a string. Update the fixture documentation to match the new numeric-ID behavior (and consider guarding against NaN in fixtures to fail fast in tests).
| // Check if any tasks were renumbered during the move | ||
| const renumberedTasks = (result.movedTasks || []).filter( | ||
| (t) => t.newId !== undefined | ||
| ); | ||
| let message = `Successfully moved ${sourceIds.length} task(s) from "${args.sourceTag}" to "${args.targetTag}"`; | ||
| if (renumberedTasks.length > 0) { | ||
| const renumberDetails = renumberedTasks | ||
| .map((t) => `${t.originalId} → ${t.newId}`) | ||
| .join(', '); | ||
| message += `. Renumbered to avoid ID collisions: ${renumberDetails}`; | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| data: { | ||
| ...result, | ||
| message: `Successfully moved ${sourceIds.length} task(s) from "${args.sourceTag}" to "${args.targetTag}"`, | ||
| message, | ||
| moveOptions, |
There was a problem hiding this comment.
The message count here is based on sourceIds.length, but moveTasksBetweenTags() can move more tasks than that when withDependencies=true (and it returns movedTasks). Use result.movedTasks.length (or fall back safely) so the message accurately reflects what happened.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tm-core/src/common/types/index.ts (1)
296-329:⚠️ Potential issue | 🟠 MajorType guards
isTaskandisSubtaskare now inconsistent with the widenedidtypes.The type definitions were updated to allow
number | stringfor task IDs, but the type guards still check for specific types:
isTask(line 301): checkstypeof task.id === 'string'— rejects numeric IDsisSubtask(lines 321-322): checkstypeof subtask.id === 'number'andtypeof subtask.parentId === 'string'— rejects string IDs and numeric parentIdsThese guards will now incorrectly return
falsefor valid objects, potentially breaking runtime validation.🐛 Proposed fix to support both types
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' && typeof task.description === 'string' && isTaskStatus(task.status) && isTaskPriority(task.priority) && Array.isArray(task.dependencies) && typeof task.details === 'string' && typeof task.testStrategy === 'string' && Array.isArray(task.subtasks) ); } export function isSubtask(obj: unknown): obj is Subtask { if (!obj || typeof obj !== 'object') return false; const subtask = obj as Record<string, unknown>; return ( - typeof subtask.id === 'number' && - typeof subtask.parentId === 'string' && + (typeof subtask.id === 'number' || typeof subtask.id === 'string') && + (typeof subtask.parentId === 'number' || typeof subtask.parentId === 'string') && typeof subtask.title === 'string' && typeof subtask.description === 'string' && isTaskStatus(subtask.status) && isTaskPriority(subtask.priority) && !('subtasks' in subtask) ); }🤖 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 - 329, The type guards isTask and isSubtask currently only accept a single primitive for id fields; update their checks to accept both string and number per the widened ID types: in isTask allow task.id to be typeof 'string' OR 'number' (and if any other id-like fields exist on Task, apply same change), and in isSubtask allow subtask.id and subtask.parentId to be typeof 'string' OR 'number' instead of forcing only 'number'/'string'; keep the rest of each guard (status/priority/subtasks checks) unchanged.
🧹 Nitpick comments (3)
packages/tm-core/src/testing/task-fixtures.ts (2)
44-44: Stale documentation: comment says "Converted to string" but code now converts to number.The comment at line 44 states
id: Converted to string if number is provided, but the implementation at line 57 now usesNumber(overrides.id). Update the comment to reflect the new behavior.📝 Suggested fix
* DEFAULTS: - * - id: Converted to string if number is provided + * - id: Converted to number if string is provided * - 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` at line 44, Update the stale JSDoc/comment in packages/tm-core/src/testing/task-fixtures.ts describing the id behavior: change the line that reads "id: Converted to string if number is provided" to reflect the current implementation which uses Number(overrides.id) — e.g., "id: Converted to number if string is provided (uses Number(...))" — so the comment matches the actual conversion performed by the task fixture.
122-134: Inconsistent ID normalization:createSubtaskdoesn't normalize IDs to numbers likecreateTask.
createTasknow coercesidtoNumber()(line 57), butcreateSubtaskkeeps the rawoverrides.idand derivesparentIdas a string. For consistent test fixtures matching the PR's goal of numeric IDs in file storage, consider normalizing both here as well.♻️ Suggested fix for consistency
export function createSubtask( overrides: Partial<Omit<Subtask, 'id' | 'parentId'>> & { id: number | string; title: string; - parentId?: string; + parentId?: number | string; } ): Subtask { const idStr = String(overrides.id); - const defaultParentId = idStr.includes('.') ? idStr.split('.')[0] : '1'; + const defaultParentId = idStr.includes('.') ? Number(idStr.split('.')[0]) : 1; return { - id: overrides.id, - parentId: overrides.parentId ?? defaultParentId, + id: Number(overrides.id), + parentId: Number(overrides.parentId ?? defaultParentId),Based on learnings: "Use numeric IDs for direct task references" and the PR objective to normalize task IDs to numbers everywhere.
🤖 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 122 - 134, createSubtask currently returns overrides.id and computes parentId as a string (using idStr), causing inconsistency with createTask which coerces ids to numbers; update createSubtask to normalize both the subtask id and parentId to numbers (use Number(overrides.id) for id and derive defaultParentId as a numeric value instead of a string), and ensure the returned Subtask properties use these numeric values so fixtures match the numeric-ID convention used by createTask.scripts/modules/task-manager/move-task.js (1)
819-834: Consider filtering non-finite IDs ingetNextAvailableId.If any existing task has a malformed ID that becomes
NaNafter normalization,Math.max(...allIds)will returnNaN, and the function will returnNaN + 1 = NaN.🛡️ Suggested defensive improvement
function getNextAvailableId(rawData, targetTag, idRemapping) { - const existingIds = rawData[targetTag].tasks.map((t) => t.id); + const existingIds = rawData[targetTag].tasks + .map((t) => t.id) + .filter((id) => Number.isFinite(id)); const remappedIds = Array.from(idRemapping.values()); - const allIds = [...existingIds, ...remappedIds]; + const allIds = [...existingIds, ...remappedIds].filter((id) => Number.isFinite(id)); if (allIds.length === 0) return 1; return Math.max(...allIds) + 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/modules/task-manager/move-task.js` around lines 819 - 834, The getNextAvailableId function should defensively filter and coerce IDs so malformed or NaN values don't poison Math.max; when building existingIds and remappedIds (from rawData[targetTag].tasks and idRemapping.values()), map/convert entries to Number and then filter with Number.isFinite (or isFinite) to produce valid numeric IDs in allIds, and if after filtering allIds is empty return 1, otherwise return Math.max(...allIds) + 1; update references to existingIds, remappedIds and allIds in getNextAvailableId accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/tm-core/src/modules/storage/adapters/file-storage/format-handler.ts`:
- Around line 222-234: normalizeTasks currently uses Number(...) directly which
can produce NaN for malformed ids; update normalizeTasks to validate numeric
conversion for task.id, subtask.id, and subtask.parentId using
Number.isFinite(Number(value)) (or equivalent) and handle failures explicitly:
either throw a descriptive error (including the task/subtask context) or set a
safe fallback (e.g., null) and log the issue so callers don't get silent NaNs.
Ensure you apply this check inside the normalizeTasks function for the Task and
subtask mappings and include clear context (task id or index and subtask id) in
any error/log to aid debugging.
---
Outside diff comments:
In `@packages/tm-core/src/common/types/index.ts`:
- Around line 296-329: The type guards isTask and isSubtask currently only
accept a single primitive for id fields; update their checks to accept both
string and number per the widened ID types: in isTask allow task.id to be typeof
'string' OR 'number' (and if any other id-like fields exist on Task, apply same
change), and in isSubtask allow subtask.id and subtask.parentId to be typeof
'string' OR 'number' instead of forcing only 'number'/'string'; keep the rest of
each guard (status/priority/subtasks checks) unchanged.
---
Nitpick comments:
In `@packages/tm-core/src/testing/task-fixtures.ts`:
- Line 44: Update the stale JSDoc/comment in
packages/tm-core/src/testing/task-fixtures.ts describing the id behavior: change
the line that reads "id: Converted to string if number is provided" to reflect
the current implementation which uses Number(overrides.id) — e.g., "id:
Converted to number if string is provided (uses Number(...))" — so the comment
matches the actual conversion performed by the task fixture.
- Around line 122-134: createSubtask currently returns overrides.id and computes
parentId as a string (using idStr), causing inconsistency with createTask which
coerces ids to numbers; update createSubtask to normalize both the subtask id
and parentId to numbers (use Number(overrides.id) for id and derive
defaultParentId as a numeric value instead of a string), and ensure the returned
Subtask properties use these numeric values so fixtures match the numeric-ID
convention used by createTask.
In `@scripts/modules/task-manager/move-task.js`:
- Around line 819-834: The getNextAvailableId function should defensively filter
and coerce IDs so malformed or NaN values don't poison Math.max; when building
existingIds and remappedIds (from rawData[targetTag].tasks and
idRemapping.values()), map/convert entries to Number and then filter with
Number.isFinite (or isFinite) to produce valid numeric IDs in allIds, and if
after filtering allIds is empty return 1, otherwise return Math.max(...allIds) +
1; update references to existingIds, remappedIds and allIds in
getNextAvailableId accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb2f455d-bc65-4a46-8559-fbe69a866f96
📒 Files selected for processing (7)
mcp-server/src/core/direct-functions/move-task-cross-tag.jspackages/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.tsscripts/modules/task-manager/move-task.jstests/integration/move-task-cross-tag.integration.test.js
| private normalizeTasks(tasks: Task[]): Task[] { | ||
| return tasks.map((task) => ({ | ||
| ...task, | ||
| id: String(task.id), // Task IDs are strings | ||
| 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: String(subtask.parentId) // Parent ID is string (Task ID) | ||
| parentId: Number(subtask.parentId) // Parent ID is number (Task ID) | ||
| })) || [] | ||
| })); | ||
| } |
There was a problem hiding this comment.
Consider validating numeric conversion to avoid silent NaN values.
Number() returns NaN for undefined, null, or non-numeric strings. If task data is malformed (e.g., task.id is undefined or "abc"), the normalized task will have id: NaN, which could cause subtle bugs in lookups and comparisons.
Consider adding validation or using a safer conversion:
🛡️ Suggested defensive approach
private normalizeTasks(tasks: Task[]): Task[] {
return tasks.map((task) => ({
...task,
- id: Number(task.id), // Task IDs are numbers
+ id: this.toNumericId(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)
+ id: this.toNumericId(subtask.id), // Subtask IDs are numbers
+ parentId: this.toNumericId(subtask.parentId) // Parent ID is number (Task ID)
})) || []
}));
}
+/**
+ * Safely convert a value to a numeric ID, throwing if invalid
+ */
+private toNumericId(value: unknown): number {
+ const num = Number(value);
+ if (!Number.isFinite(num)) {
+ throw new Error(`Invalid task ID: ${value}`);
+ }
+ return num;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private normalizeTasks(tasks: Task[]): Task[] { | |
| return tasks.map((task) => ({ | |
| ...task, | |
| id: String(task.id), // Task IDs are strings | |
| 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: String(subtask.parentId) // Parent ID is string (Task ID) | |
| parentId: Number(subtask.parentId) // Parent ID is number (Task ID) | |
| })) || [] | |
| })); | |
| } | |
| private normalizeTasks(tasks: Task[]): Task[] { | |
| return tasks.map((task) => ({ | |
| ...task, | |
| id: this.toNumericId(task.id), // Task IDs are numbers | |
| dependencies: task.dependencies?.map((dep) => String(dep)) || [], | |
| subtasks: | |
| task.subtasks?.map((subtask) => ({ | |
| ...subtask, | |
| id: this.toNumericId(subtask.id), // Subtask IDs are numbers | |
| parentId: this.toNumericId(subtask.parentId) // Parent ID is number (Task ID) | |
| })) || [] | |
| })); | |
| } | |
| /** | |
| * Safely convert a value to a numeric ID, throwing if invalid | |
| */ | |
| private toNumericId(value: unknown): number { | |
| const num = Number(value); | |
| if (!Number.isFinite(num)) { | |
| throw new Error(`Invalid task ID: ${value}`); | |
| } | |
| return num; | |
| } |
🤖 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/format-handler.ts`
around lines 222 - 234, normalizeTasks currently uses Number(...) directly which
can produce NaN for malformed ids; update normalizeTasks to validate numeric
conversion for task.id, subtask.id, and subtask.parentId using
Number.isFinite(Number(value)) (or equivalent) and handle failures explicitly:
either throw a descriptive error (including the task/subtask context) or set a
safe fallback (e.g., null) and log the issue so callers don't get silent NaNs.
Ensure you apply this check inside the normalizeTasks function for the Task and
subtask mappings and include clear context (task id or index and subtask id) in
any error/log to aid debugging.
Summary
tasks.jsonwhereset-task-status(CLI path) wrote numeric IDs whileupdate-task(core file-storage path) wrote string IDsfile-storage.tsandformat-handler.tsnormalization fromString()toNumber()for task IDs, subtask parent IDsTask.id,Subtask.parentId,PlaceholderTask.id,UpdateTask.id) fromstringtonumber | stringto support both file storage (numeric) and API storage (string display IDs)createTaskhelper to useNumber()instead ofString()for consistencyTest plan
npm run turbo:typecheck)npm run test -w packages/tm-core)set-task-statusthenupdate-taskon same task, verifytasks.jsonhas consistent numeric IDsFixes #1583
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes