fix: ensure consistent task ID serialization as numbers (#1583) #1618
Conversation
…#1583) **Summary** Task IDs were inconsistently saved as strings or numbers in tasks.json depending on which command was used (e.g., set-status vs update-task), causing unnecessary git diffs and file instability. **Root Cause** The codebase has two storage layers writing to tasks.json: - Legacy CLI layer (scripts/modules/utils.js): converted IDs to numbers - New tm-core layer (packages/tm-core): converted IDs to strings The tm-core file storage adapter was incorrectly using string normalization intended for API storage (which supports prefixed IDs like "HAM-123"), even though file storage only needs simple numeric IDs. **Solution** Updated both storage layers to consistently use numbers for file storage: - Task IDs: saved as numbers (1, 2, 3) - Subtask IDs: saved as numbers (1, 2, 3) - Parent IDs: saved as numbers - Dependencies: subtask references (e.g., "1.1") kept as strings, task references converted to numbers **Implementation** - Updated file-storage.ts and format-handler.ts normalization logic - Enhanced scripts/modules/utils.js with comprehensive dependency and parentId handling - Updated TypeScript types to support number | string for IDs throughout the system - Fixed type compatibility in tasks-domain, executor services, and CLI commands - Added changeset documenting the user-facing fix
🦋 Changeset detectedLatest commit: 6bb6bfa 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR establishes consistent task ID serialization across the codebase by introducing centralized normalization utilities that ensure task IDs, subtask IDs, and dependencies are stored as strings in persisted storage. Refactors existing ID handling logic to use shared normalization functions and updates type guards and service interfaces to support both numeric and string ID types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts
Outdated
Show resolved
Hide resolved
![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-328:⚠️ Potential issue | 🟠 MajorType guards have mismatched constraints with their interface definitions.
The
Taskinterface (line 132) definesid: number | string, butisTask()on line 301 requirestypeof task.id === 'string', rejecting valid numeric IDs. Similarly, theSubtaskinterface (lines 173–174) allowsid: number | stringandparentId: number | string, butisSubtask()checks onlytypeof subtask.id === 'number'andtypeof subtask.parentId === 'string', rejecting valid string IDs and numeric parent IDs respectively.Update the guards to accept both types:
🛠️ Proposed fix
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) ); } @@ return ( - typeof subtask.id === 'number' && - typeof subtask.parentId === 'string' && + (typeof subtask.id === 'string' || typeof subtask.id === 'number') && + (typeof subtask.parentId === 'string' || typeof subtask.parentId === 'number') && typeof subtask.title === 'string' && typeof subtask.description === 'string' && isTaskStatus(subtask.status) && isTaskPriority(subtask.priority) && !('subtasks' in subtask) ); }
🤖 Fix all issues with AI agents
In `@packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts`:
- Around line 285-310: The normalizeTaskIds implementation in file-storage.ts
(and the similar normalizeTasks in format-handler.ts) uses bare Number(...)
coercions which can produce NaN and corrupt IDs; extract a shared utility (e.g.,
create a new function parseValidId(value) in a central utils module) that uses
parseInt(value, 10) with validation (!isNaN(parsed) && parsed > 0) and returns
either the parsed number or the original value, then replace all Number(...)
uses in normalizeTaskIds and normalizeTasks (including subtask id, parentId and
dependency parsing) to call parseValidId and preserve string dependency refs
when appropriate, removing the duplicated logic and centralizing validation.
🧹 Nitpick comments (1)
packages/tm-core/src/modules/storage/adapters/file-storage/format-handler.ts (1)
219-245: Consider centralizing task-ID normalization to avoid drift.
This logic mirrorsFileStorage.normalizeTaskIds; extracting a shared helper in tm-core would reduce duplication and keep behavior consistent across adapters.
As per coding guidelines: Do not duplicate task ID formatting logic across modules - centralize formatting utilities.
packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts
Outdated
Show resolved
Hide resolved
Summary: Fixed four critical issues with task ID handling in tm-core: type guard mismatches, strict equality comparison failures, duplicated normalization logic, and missing NaN validation. Root Cause: 1. Type guards (isTask/isSubtask) checked for specific ID types (string/number) but interfaces allowed number | string, causing valid normalized tasks to be rejected 2. checkInProgressConflicts compared numeric task.id with string targetTaskId using strict equality (===), which always failed (e.g., 1 === "1") 3. Normalization logic was duplicated in file-storage.ts and format-handler.ts without validation 4. Number() coercions could produce NaN, corrupting task IDs without detection Solution: - Updated type guards to accept both number and string ID types using new isValidTaskIdType() helper - Fixed all ID comparisons in checkInProgressConflicts to use String() coercion - Created shared task-id-normalizer.ts utility with proper parseInt and NaN validation - Refactored both file-storage and format-handler to use the shared utility Implementation: - packages/tm-core/src/common/types/index.ts: Added isValidTaskIdType(), updated isTask() and isSubtask() - packages/tm-core/src/modules/tasks/services/task-execution-service.ts: String coercion for ID comparisons - packages/tm-core/src/common/utils/task-id-normalizer.ts: New shared utility with validation (normalizeTaskId, normalizeSubtaskId, normalizeDependency, normalizeTaskIds) - packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts: Removed duplicate normalizeTaskIds method - packages/tm-core/src/modules/storage/adapters/file-storage/format-handler.ts: Removed duplicate normalizeTasks method - Added comprehensive test coverage: 54 tests (24 for normalizer, 30 for type guards)
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/modules/tasks/services/task-execution-service.ts (1)
149-189:⚠️ Potential issue | 🟠 MajorNormalize
targetTaskIdonce to avoid false conflicts.
Right now onlytask.idis coerced, so numerictargetTaskIdvalues won’t match and can be incorrectly treated as conflicts.🛠️ Proposed fix
- // If the target task is already in-progress, that's fine - // Use String() coercion to handle both numeric and string IDs - const targetTaskInProgress = inProgressTasks.find( - (task) => String(task.id) === targetTaskId - ); + // If the target task is already in-progress, that's fine + // Use String() coercion to handle both numeric and string IDs + const targetTaskIdStr = String(targetTaskId); + const targetTaskInProgress = inProgressTasks.find( + (task) => String(task.id) === targetTaskIdStr + ); if (targetTaskInProgress) { return { canProceed: true, conflictingTasks: [] }; } // Check if target is a subtask and its parent is in-progress - const isSubtask = targetTaskId.includes('.'); + const isSubtask = targetTaskIdStr.includes('.'); if (isSubtask) { - const parentTaskId = targetTaskId.split('.')[0]; + const parentTaskId = targetTaskIdStr.split('.')[0]; const parentInProgress = inProgressTasks.find( (task) => String(task.id) === parentTaskId ); if (parentInProgress) { return { canProceed: true, conflictingTasks: [] }; // Allow subtasks when parent is in-progress @@ - const conflictingTasks = inProgressTasks.filter((task) => { + const conflictingTasks = inProgressTasks.filter((task) => { const taskIdStr = String(task.id); - if (taskIdStr === targetTaskId) return false; + if (taskIdStr === targetTaskIdStr) return false; // If target is a subtask, exclude its parent from conflicts if (isSubtask) { - const parentTaskId = targetTaskId.split('.')[0]; + const parentTaskId = targetTaskIdStr.split('.')[0]; if (taskIdStr === parentTaskId) return false; } // If the in-progress task is a subtask of our target parent, exclude it if (taskIdStr.includes('.')) { const taskParentId = taskIdStr.split('.')[0]; - if (isSubtask && taskParentId === targetTaskId.split('.')[0]) { + if (isSubtask && taskParentId === targetTaskIdStr.split('.')[0]) { return false; } } return true; });
🤖 Fix all issues with AI agents
In `@packages/tm-core/src/common/utils/task-id-normalizer.ts`:
- Around line 167-177: The function normalizeTask currently coerces
invalid/non-numeric task.id values to 0; change the fallback so we preserve the
original ID instead of forcing 0: replace the assignment of taskIdNum in
normalizeTask to use normalizedId ?? task.id (or undefined if task.id is absent)
and ensure calls to normalizeSubtask(subtask, taskIdNum) and any other places
(similar logic around lines 146-157) accept and propagate the original ID type
rather than a forced 0; update any type expectations or validations around
normalizeTaskId/normalizeSubtask to explicitly handle string IDs or missing IDs
instead of silently converting to 0.
🧹 Nitpick comments (2)
packages/tm-core/src/common/utils/index.ts (1)
60-69: Consider trimming exports to only what’s consumed. If these helpers aren’t imported by other packages, keepingindex.tsminimal avoids expanding the public surface unnecessarily.Based on learnings: For the tm-core package in the eyaltoledano/claude-task-master repository, the team prefers a minimal, need-based export strategy in index files rather than exposing all internal utilities.
packages/tm-core/src/common/types/index.ts (1)
305-318: Consider validating dependency element types in the guard.
Right now any array passes; checking each entry avoids accepting invalid dependency values.♻️ Suggested refinement
return ( isValidTaskIdType(task.id) && typeof task.title === 'string' && typeof task.description === 'string' && isTaskStatus(task.status) && isTaskPriority(task.priority) && Array.isArray(task.dependencies) && + task.dependencies.every(isValidTaskIdType) && typeof task.details === 'string' && typeof task.testStrategy === 'string' && Array.isArray(task.subtasks) );
…ormalizers
**Summary**
Fixed three critical issues in task ID normalization that could corrupt task data:
1. Non-numeric string IDs (e.g., "HAM-123", "abc") were silently coerced to 0
2. Negative numeric IDs bypassed validation in both normalizeTask() and normalizeSubtask()
3. Duplicated dependency normalization logic in scripts/modules/utils.js
**Root Cause**
The fallback logic in normalizeTask() and normalizeSubtask() had a fundamental flaw:
- `normalizedId ?? (typeof task.id === 'number' ? task.id : 0)` would accept ANY numeric value including negatives
- Non-numeric strings would fall through to 0, corrupting API IDs like "HAM-123"
- The typeof check didn't validate that numbers were positive, finite integers
**Solution**
1. Added `isValidPositiveInteger()` helper that strictly validates:
- Must be a number type
- Must be finite (rejects ±Infinity and NaN)
- Must be an integer (rejects decimals)
- Must be positive (rejects zero and negatives)
2. Updated normalizeTask() and normalizeSubtask() with explicit validation cascade:
- Use normalized ID if successful (positive integer)
- Preserve non-empty string IDs (API IDs like "HAM-123")
- Only accept valid positive integers (reject negative/zero/Infinity)
- Fall back to '0' only for truly undefined/null cases
3. Extracted normalizeDependencyArray() helper in utils.js to DRY up duplicate code
**Implementation**
- packages/tm-core/src/common/utils/task-id-normalizer.ts
- Added isValidPositiveInteger() validation helper
- Refactored normalizeTask() with proper validation cascade
- Refactored normalizeSubtask() with proper validation cascade
- Changed parentTaskId parameter to accept number | string for API ID support
- IDs now stored as strings for consistency with file-storage patterns
- packages/tm-core/src/common/utils/task-id-normalizer.spec.ts
- Added 12 new test cases covering:
- Negative ID rejection for tasks and subtasks
- Infinity rejection for tasks and subtasks
- Non-numeric string ID preservation (API IDs)
- Zero, NaN, null, undefined, empty string handling
- Subtask parentId preservation with string parent IDs
- scripts/modules/utils.js
- Extracted normalizeDependencyArray() helper (lines 540-558)
- Updated normalizeTaskIds() to use helper in both locations (lines 578, 607)
- Eliminated 24 lines of duplicated code
**Tests**: 37/37 passing (100%)
**TypeCheck**: ✅ Pass
**Linting**: ✅ Pass
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/tm-core/src/common/utils/task-id-normalizer.ts`:
- Around line 246-266: The JSDoc example for normalizeTaskIds is incorrect
relative to the implementation: IDs, parentId and numeric dependency strings
should be returned as numbers when they represent integers. Update
normalizeTaskIds (and any helper like parseTaskId/normalizeId in
task-id-normalizer.ts) to coerce string IDs that match an integer pattern (e.g.,
/^\d+$/) into numbers for id and parentId and convert dependency entries
similarly while leaving non-integer strings (like "3.1") unchanged; then ensure
subtasks inherit parentId as the coerced type. Also update any internal places
that construct dependencies/subtasks to use the normalized numeric form so the
returned structure matches the documented example.
- Around line 210-223: The code assigns string values to taskIdValue when
normalizedId is non-null, when task.id is a numeric valid integer, or in
fallback, which contradicts the requirement that numeric IDs remain numbers;
update the logic in task-id-normalizer.ts (symbols: taskIdValue, normalizedId,
isValidPositiveInteger, and the branch that mirrors normalizeSubtask) so that
when an ID is a numeric positive integer you assign a number (e.g.,
Number(normalizedId) or +task.id) instead of String(...), preserve non-empty
string IDs as strings, and only use '0' (string) if the fallback must be a
string per storage contract or otherwise use 0 as a numeric fallback to match
the numeric ID requirement.
- Around line 156-170: The code currently converts numeric IDs to strings (using
String(...)) and the fallback uses '0', which causes numeric IDs to be stored as
JSON strings; change the assignments so numeric branches produce numbers
instead: in task-id-normalizer.ts update subtaskIdValue (referenced as
subtaskIdValue, normalizedId, isValidPositiveInteger, and subtask.id) to assign
numeric values for numeric cases (e.g., use Number(normalizedId) or +subtask.id)
and set the fallback to 0 (number) while keeping the branch that preserves
non-empty non-numeric string IDs as strings; this ensures numeric IDs are
written as numbers in tasks.json.
🧹 Nitpick comments (1)
scripts/modules/utils.js (1)
564-612: Consider aligning mutation behavior with tm-core implementation.This
normalizeTaskIdsmutates tasks in-place, while the tm-core TypeScript version (normalizeTaskIdsintask-id-normalizer.ts) returns new objects via.map(). This behavioral difference could cause subtle bugs if callers expect one behavior but get the other.For now this works correctly, but consider aligning the approaches when refactoring.
…sks.json
Summary:
Fixed inconsistent task ID serialization where numeric IDs were being converted to strings in tasks.json. The tm-core file storage adapter now correctly stores numeric IDs as numbers (e.g., {"id": 1} instead of {"id": "1"}), while preserving string IDs for API identifiers like "HAM-123".
Root Cause:
The `normalizeSubtask` and `normalizeTask` functions in task-id-normalizer.ts were explicitly converting all normalized numeric IDs to strings via `String(normalizedId)` and using string fallback values ('0'), contradicting the PR objective to store numeric IDs as numbers in file storage.
Solution:
- Modified `normalizeSubtask` to return numeric IDs as numbers instead of strings
- Modified `normalizeTask` to return numeric IDs as numbers instead of strings
- Updated fallback value from string '0' to numeric 0
- Fixed `file-storage.ts` updateTask method to use `normalizeTaskId()` instead of `String(taskId)`
- Updated all related test expectations to assert numeric types for numeric IDs
Implementation:
- packages/tm-core/src/common/utils/task-id-normalizer.ts: Changed ID assignment from String(normalizedId) to normalizedId, fallback from '0' to 0
- packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts: Added normalizeTaskId import and fixed updateTask to preserve numeric IDs
- packages/tm-core/src/common/utils/task-id-normalizer.spec.ts: Updated 18 test assertions from string expectations to numeric expectations
- packages/tm-core/tests/integration/storage/file-storage-metadata.test.ts: Fixed ID comparison to handle numeric IDs
Fixes inconsistent serialization identified in code review for PR eyaltoledano#1583.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/tm-core/tests/integration/storage/file-storage-metadata.test.ts`:
- Around line 234-242: The test currently stringifies IDs (String(t.id) === '1')
which allows string IDs to pass; update the assertions to compare numeric IDs
directly (e.g., use t.id === 1 and t.id === 2 or add an explicit type assertion)
so the test enforces the numeric-serialization contract for loadedTasks and
their metadata (reference: loadedTasks and t.id in the
file-storage-metadata.test).
| // Use String() for comparison since IDs are normalized to numbers after save/load | ||
| expect( | ||
| loadedTasks.find((t) => String(t.id) === '1')?.metadata | ||
| ).toEqual({ | ||
| existing: true | ||
| }); | ||
| expect(loadedTasks.find((t) => t.id === '2')?.metadata).toEqual({ | ||
| expect( | ||
| loadedTasks.find((t) => String(t.id) === '2')?.metadata | ||
| ).toEqual({ |
There was a problem hiding this comment.
Keep numeric ID assertions to enforce the new contract.
Line 234-242 stringifies IDs, so string IDs would still pass and the test no longer guards the numeric-serialization requirement. Prefer numeric comparisons (and optionally a type assertion) to lock the behavior.
🔧 Suggested change
- // Use String() for comparison since IDs are normalized to numbers after save/load
- expect(
- loadedTasks.find((t) => String(t.id) === '1')?.metadata
- ).toEqual({
+ const task1 = loadedTasks.find((t) => t.id === 1);
+ const task2 = loadedTasks.find((t) => t.id === 2);
+ expect(task1?.id).toBe(1);
+ expect(task2?.id).toBe(2);
+ expect(task1?.metadata).toEqual({
existing: true
});
- expect(
- loadedTasks.find((t) => String(t.id) === '2')?.metadata
- ).toEqual({
+ expect(task2?.metadata).toEqual({
newTask: true
});📝 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.
| // Use String() for comparison since IDs are normalized to numbers after save/load | |
| expect( | |
| loadedTasks.find((t) => String(t.id) === '1')?.metadata | |
| ).toEqual({ | |
| existing: true | |
| }); | |
| expect(loadedTasks.find((t) => t.id === '2')?.metadata).toEqual({ | |
| expect( | |
| loadedTasks.find((t) => String(t.id) === '2')?.metadata | |
| ).toEqual({ | |
| const task1 = loadedTasks.find((t) => t.id === 1); | |
| const task2 = loadedTasks.find((t) => t.id === 2); | |
| expect(task1?.id).toBe(1); | |
| expect(task2?.id).toBe(2); | |
| expect(task1?.metadata).toEqual({ | |
| existing: true | |
| }); | |
| expect(task2?.metadata).toEqual({ | |
| newTask: true | |
| }); |
🤖 Prompt for AI Agents
In `@packages/tm-core/tests/integration/storage/file-storage-metadata.test.ts`
around lines 234 - 242, The test currently stringifies IDs (String(t.id) ===
'1') which allows string IDs to pass; update the assertions to compare numeric
IDs directly (e.g., use t.id === 1 and t.id === 2 or add an explicit type
assertion) so the test enforces the numeric-serialization contract for
loadedTasks and their metadata (reference: loadedTasks and t.id in the
file-storage-metadata.test).
| // Convert subtask dependencies using helper function | ||
| if (Array.isArray(subtask.dependencies)) { | ||
| subtask.dependencies = normalizeDependencyArray(subtask.dependencies); | ||
| } |
There was a problem hiding this comment.
Duplicated normalization logic between TypeScript and JavaScript
Medium Severity
The new normalizeDependencyArray and updated normalizeTaskIds functions duplicate the logic from task-id-normalizer.ts. Since utils.js already imports from @tm/core (line 18: import { FileOperations } from '@tm/core';), it could import normalizeTaskIds and normalizeDependencies from @tm/core instead of maintaining a parallel implementation.
Crunchyman-ralph
left a comment
Crunchyman-ralph
left a comment
There was a problem hiding this comment.
this PR is great, one comment, I think we should pick either number or string, not either or.
So I guess lets stick to string, and enforce that everywhere in the codebase instead of trying to play nice on both, what do you think ?
Address reviewer feedback to pick a single type for task IDs (string) rather than supporting both number and string. Reverts type widening from the original PR and enforces string as the canonical ID type throughout the domain model, storage adapters, CLI, and legacy code. - Revert Task.id to string (not number | string) - Rewrite task-id-normalizer to output strings instead of numbers - Fix type guards (isTask, isSubtask) for string-canonical IDs - Remove unnecessary String() wrappers in api-storage - Apply immutable patterns in legacy utils.js - Replace || with ?? in task-execution-service, add proper logger - Remove redundant taskIdStr variables in executor files - Update all tests for string-canonical assertions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/tm-core/src/common/utils/task-id-normalizer.ts`:
- Around line 175-189: normalizeDependency currently allows zero and negative
numbers which creates inconsistent ID formatting with
normalizeTaskId/normalizeSubtaskId; update normalizeDependency to treat numeric
deps <= 0 as invalid and return '' (same as non-finite/NaN handling).
Specifically, inside normalizeDependency where typeof dep === 'number', add the
range check for dep <= 0 (and keep the existing !Number.isFinite/Number.isNaN
checks) so only positive finite numbers are converted to String(dep); otherwise
return ''. Ensure behavior mirrors normalizeTaskId/normalizeSubtaskId to avoid
producing "0" or negative dependency IDs.
🧹 Nitpick comments (6)
packages/tm-core/src/modules/tasks/services/task-execution-service.ts (1)
152-190: String coercion for ID comparisons looks correct and consistent.Using
String(task.id)throughoutcheckInProgressConflictsproperly handles the case whereTask.idcould be numeric whiletargetTaskIdis always a string parameter. CachingtaskIdStron line 174 avoids repeated coercion—good.One minor note: the
targetTaskId.split('.')[0]call appears in three places within this method (lines 163, 179, 186). Consider extracting it once at the top of the method alongside theisSubtaskcheck to reduce repetition and ensure consistency.♻️ Optional: extract parentTaskId once
// Check if target is a subtask and its parent is in-progress const isSubtask = targetTaskId.includes('.'); + const parentTaskId = isSubtask ? targetTaskId.split('.')[0] : undefined; if (isSubtask) { - const parentTaskId = targetTaskId.split('.')[0]; const parentInProgress = inProgressTasks.find( (task) => String(task.id) === parentTaskId ); @@ .. const conflictingTasks = inProgressTasks.filter((task) => { const taskIdStr = String(task.id); if (taskIdStr === targetTaskId) return false; // If target is a subtask, exclude its parent from conflicts if (isSubtask) { - const parentTaskId = targetTaskId.split('.')[0]; if (taskIdStr === parentTaskId) return false; } // If the in-progress task is a subtask of our target parent, exclude it if (taskIdStr.includes('.')) { const taskParentId = taskIdStr.split('.')[0]; - if (isSubtask && taskParentId === targetTaskId.split('.')[0]) { + if (isSubtask && taskParentId === parentTaskId) { return false; } }packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts (1)
390-391:String(taskId)is redundant —taskIdis already typed asstring.The parameter
taskId: stringon line 348 guarantees the value is already a string. The explicitString(taskId)wrap adds no runtime benefit and slightly obscures intent.Suggested simplification
- // Keep task ID as string for consistent serialization - id: String(taskId) + id: taskIdpackages/tm-core/src/common/utils/task-id-normalizer.spec.ts (1)
263-296: Consider adding a test for numeric subtask IDs being normalized withinnormalizeTask.The
normalizeTasktest at lines 264-296 uses string IDs for subtasks. Consider adding a case where subtask IDs are numeric (e.g.,id: 1 as any) to verify they are converted to strings within thenormalizeTaskflow, exercising the full normalization chain with numeric subtask inputs.packages/tm-core/src/common/utils/task-id-normalizer.ts (3)
228-233:console.warncalls violate the coding guideline requiring use of the central log utility.Lines 230-232 and 261-263 use
console.warndirectly. The coding guidelines state: "Do not add direct console.log calls outside the logging utility - use the central log function instead."Since this is a low-level utility in
tm-corethat may not have easy access to the CLI logging utility, consider either:
- Accepting a logger parameter (dependency injection), or
- Silently returning the fallback without logging (callers can validate if needed), or
- Importing and using the appropriate logging utility from
tm-core.As per coding guidelines: "Do not add direct console.log calls outside the logging utility - use the central log function instead."
Also applies to: 259-264
89-98: Comment on line 89 is misleading and the guard on line 96 is always true.
The comment says "Try to parse as integer for normalization (e.g., '05' -> '5')" but the condition on line 91 (
String(parsed) === idStr) prevents this —"05"would parse to5, butString(5) !== "05", so it falls through and returns"05"unchanged.The
if (idStr.length > 0)guard on line 96 is redundant — it's always true becauseidStr.length === 0was already checked and returned on line 72-74.Suggested fix
- // Try to parse as integer for normalization (e.g., "05" -> "5") + // If it's a pure integer string (e.g., "5"), return as-is const parsed = parseInt(idStr, 10); if (!isNaN(parsed) && parsed > 0 && String(parsed) === idStr) { return idStr; } - // Non-numeric string ID (e.g., "HAM-1") - return as-is - if (idStr.length > 0) { - return idStr; - } - - return null; + // Non-numeric string ID (e.g., "HAM-1", "05") - return as-is + return idStr;
222-222: Typenumber | stringonsubtaskIdValueis wider than needed — onlystringvalues are ever assigned.All branches assign string values:
normalizedId(string),subtask.id(string branch), or the template literal fallback. Thenumberpart of the union is dead.Suggested fix
- let subtaskIdValue: number | string; + let subtaskIdValue: string;
| export function normalizeDependency(dep: unknown): string { | ||
| if (dep === undefined || dep === null) { | ||
| return ''; | ||
| } | ||
|
|
||
| if (typeof dep === 'number') { | ||
| if (!Number.isFinite(dep) || Number.isNaN(dep)) { | ||
| return ''; | ||
| } | ||
| return String(dep); | ||
| } | ||
|
|
||
| const depStr = String(dep).trim(); | ||
| return depStr; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, examine the normalizer functions in the file
cat -n packages/tm-core/src/common/utils/task-id-normalizer.tsRepository: eyaltoledano/claude-task-master
Length of output: 11200
🏁 Script executed:
# Search for how normalizeDependency is used
rg -n 'normalizeDependency' packages/tm-core/src --type ts --type js -A 3 -B 1Repository: eyaltoledano/claude-task-master
Length of output: 5565
🏁 Script executed:
# Look for normalizeTaskId and normalizeSubtaskId to understand their rejection logic
rg -n 'export function normalize(Task|Subtask)Id' packages/tm-core/src --type ts -A 10Repository: eyaltoledano/claude-task-master
Length of output: 2526
🏁 Script executed:
# Check for zero or negative dependencies in test data and JSON files
rg -n 'dependencies.*\[.*(?:0|-[0-9]+)' packages/tm-core --type json --type ts --type js -g '!node_modules' -g '!dist' | head -30Repository: eyaltoledano/claude-task-master
Length of output: 57
🏁 Script executed:
# Search for dependency validation logic
rg -n 'dependencies|validateDependency|checkDependency' packages/tm-core/src --type ts --type js -A 2 -B 1 | grep -A 3 -B 3 'validate\|check\|throw\|error' | head -40Repository: eyaltoledano/claude-task-master
Length of output: 3028
🏁 Script executed:
# Look specifically at how dependencies are validated in dependency-manager or similar
fd -name '*dependency*' packages/tm-core/src --type fRepository: eyaltoledano/claude-task-master
Length of output: 305
🏁 Script executed:
# Check if normalizeDependency is used with any validation in dependency operations
rg -n 'normalizeDependencies\|normalizeDependency' packages/tm-core/src --type ts -A 5 | head -50Repository: eyaltoledano/claude-task-master
Length of output: 57
🏁 Script executed:
# Find dependency-related files
fd 'dependency' packages/tm-core/src --type fRepository: eyaltoledano/claude-task-master
Length of output: 153
🏁 Script executed:
# Look at the task-loader validation logic more carefully
cat -n packages/tm-core/src/modules/tasks/services/task-loader.service.ts | sed -n '200,250p'Repository: eyaltoledano/claude-task-master
Length of output: 1940
🏁 Script executed:
# Check task-filters for how it validates dependencies
cat -n packages/tm-core/src/modules/tasks/utils/task-filters.ts | sed -n '50,120p'Repository: eyaltoledano/claude-task-master
Length of output: 2966
🏁 Script executed:
# Check what happens when invalid deps are detected - how they're handled
cat -n packages/tm-core/src/modules/tasks/services/task-loader.service.ts | sed -n '226,280p'Repository: eyaltoledano/claude-task-master
Length of output: 1941
🏁 Script executed:
# Look at the dependency-fetcher to see if it uses normalizeDependency
cat -n packages/tm-core/src/modules/tasks/repositories/supabase/dependency-fetcher.ts | head -100Repository: eyaltoledano/claude-task-master
Length of output: 2392
🏁 Script executed:
# Check if task ID "0" is handled specially anywhere or creates issues
rg -n '"0"|\'0\'' packages/tm-core/src --type ts --type js -B 2 -A 2 | grep -i 'task\|fallback\|emergency' -A 3 -B 3 | head -50Repository: eyaltoledano/claude-task-master
Length of output: 143
🏁 Script executed:
# Look at the full normalizeTask function to understand the "0" fallback context
cat -n packages/tm-core/src/common/utils/task-id-normalizer.ts | sed -n '250,275p'Repository: eyaltoledano/claude-task-master
Length of output: 1058
normalizeDependency accepts zero and negative numbers, creating an asymmetry with normalizeTaskId/normalizeSubtaskId that violates consistent ID formatting requirements.
The test explicitly verifies this behavior (lines 115–118), confirming it is intentional. However, this permissive design contradicts the learnings stating "use numeric IDs for direct task references" and "format task and dependency IDs consistently." While downstream validation (task-loader, buildBlocksMap) does catch references to non-existent tasks, the normalizer should align with the stricter range validation in normalizeTaskId and normalizeSubtaskId to prevent silent creation of invalid IDs—especially since normalizeTask uses "0" as an emergency fallback (line 264), which could then be referenced as a valid dependency.
Recommend adding range validation to normalizeDependency: reject dep <= 0 and non-finite numbers, just as normalizeTaskId and normalizeSubtaskId do.
🤖 Prompt for AI Agents
In `@packages/tm-core/src/common/utils/task-id-normalizer.ts` around lines 175 -
189, normalizeDependency currently allows zero and negative numbers which
creates inconsistent ID formatting with normalizeTaskId/normalizeSubtaskId;
update normalizeDependency to treat numeric deps <= 0 as invalid and return ''
(same as non-finite/NaN handling). Specifically, inside normalizeDependency
where typeof dep === 'number', add the range check for dep <= 0 (and keep the
existing !Number.isFinite/Number.isNaN checks) so only positive finite numbers
are converted to String(dep); otherwise return ''. Ensure behavior mirrors
normalizeTaskId/normalizeSubtaskId to avoid producing "0" or negative dependency
IDs.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // Truly invalid ID - log warning and use parentId-based fallback | ||
| console.warn( | ||
| `[task-id-normalizer] Subtask has invalid ID: ${JSON.stringify(subtask.id)}, using fallback "${parentId}.0"` | ||
| ); |
There was a problem hiding this comment.
Inconsistent logging between utility modules
Low Severity
The task-id-normalizer.ts utility uses console.warn() directly for logging warnings about invalid IDs, while task-execution-service.ts in the same PR properly uses the centralized logger from getLogger(). This creates inconsistent logging practices where some modules log through the configurable logger system and others bypass it with direct console calls, making logs harder to control and filter in production.
Additional Locations (1)
| // Try to parse as integer for normalization (e.g., "05" -> "5") | ||
| const parsed = parseInt(idStr, 10); | ||
| if (!isNaN(parsed) && parsed > 0 && String(parsed) === idStr) { | ||
| return idStr; |
There was a problem hiding this comment.
Leading zeros not normalized despite comment
Low Severity
The comment claims normalizeTaskId normalizes leading zeros (e.g., "05" becomes "5"), but the implementation preserves them unchanged. The condition String(parsed) === idStr on line 91 fails for strings like "05" (since String(5) !== "05"), causing the function to return "05" as-is. This could allow IDs like "05" and "5" to coexist as distinct identifiers, potentially breaking ID-based comparisons and lookups throughout the system.
| `[task-id-normalizer] Subtask has invalid ID: ${JSON.stringify(subtask.id)}, using fallback "${parentId}.0"` | ||
| ); | ||
| subtaskIdValue = `${parentId}.0`; | ||
| } |
There was a problem hiding this comment.
Multiple invalid subtask IDs create duplicates
High Severity
When multiple subtasks have invalid IDs (null, undefined, 0, negative), they all receive the same fallback ID ${parentId}.0, creating duplicate subtask IDs within the same task. For example, a task with ID "5" containing three subtasks with IDs [null, 0, -1] would produce three subtasks all with ID "5.0". Duplicate IDs break task lookups, dependency resolution, and update operations that rely on unique identifiers.
| `[task-id-normalizer] Task has invalid ID: ${JSON.stringify(task.id)}, using fallback "0"` | ||
| ); | ||
| taskIdValue = '0'; | ||
| } |
There was a problem hiding this comment.
Multiple invalid task IDs create duplicates
High Severity
When multiple tasks have invalid IDs (null, undefined, empty string, 0, NaN), they all receive the same fallback ID "0", creating duplicate task IDs in the task list. For example, a task list with three tasks having IDs [null, undefined, ""] would produce three tasks all with ID "0". Duplicate task IDs break core functionality including task retrieval, updates, and dependency resolution that rely on IDs being unique identifiers.
| String(dep) | ||
| ) | ||
| })) | ||
| })); |
There was a problem hiding this comment.
Redundant String conversions after normalization
Low Severity
The code calls normalizeTaskIds(tasks) which already returns all IDs as strings, then immediately wraps every ID, dependency, and parentId in another String() call. This is redundant since normalizeTaskIds() guarantees string output for all ID fields. The extra transformations create unnecessary object allocations and add visual noise without providing any runtime benefit.
2 participants
Description
Fixes inconsistent task ID serialization in tasks.json. Task IDs are now consistently stored as numbers (without quotes) in both "set task" and "update task" operations.
Background: The codebase has two storage layers writing to tasks.json - the legacy CLI layer and the new tm-core layer. The tm-core file storage adapter was incorrectly using string normalization intended for API storage (which supports prefixed IDs like "HAM-123"), causing IDs to flip between strings and numbers depending on which command was used.
Changes:
Result: Task and subtask IDs are now consistently saved as numbers in tasks.json, eliminating unnecessary git diffs and file instability.
Type of Change
Testing
Changeset
Additional Notes
Closes #1583
Note
Medium Risk
Touches persistence/serialization paths in both tm-core file storage and the legacy CLI, so mistakes could rewrite
tasks.jsonor break ID lookups; risk is mitigated by substantial new unit/integration test coverage.Overview
Fixes inconsistent
tasks.jsonserialization by centralizing task/subtask/dependency ID normalization and using it in tm-core file storage save/format conversion paths (replacing the adapter-local normalizers).Adds a shared
task-id-normalizerutility (with extensive tests) and updates type guards/types to acceptSubtask.idasnumber | stringplus a newisValidTaskIdTypehelper; also expandsTaskStatusvalidation to includecompleted.Updates the legacy CLI
scripts/modules/utils.jsnormalization to be non-mutating and to consistently coerce task/subtask/dependency IDs (andparentId) to strings, and adjusts integration/unit tests and task execution conflict checks to compare IDs viaString()coercion.Written by Cursor Bugbot for commit 6bb6bfa. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
New Features
Tests