fix: prevent task number collisions when moving tasks between tags#1665
fix: prevent task number collisions when moving tasks between tags#1665withsivram 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>
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAutomatic renumbering was implemented for tasks moved between tags when destination ID collisions occur. The move operation now assigns new IDs, tracks original→new mappings, updates parent/subtask dependencies accordingly, and returns success messages that include renumbering details. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as Move-Task API
participant MoveOp as executeMoveOperation
participant IdMgr as getNextAvailableId
participant DepMgr as Dependency Updater
participant Storage as Persistence (writeJSON)
User->>API: Request move tasks between tags
API->>MoveOp: start move batch
MoveOp->>MoveOp: detect ID collision(s)
MoveOp->>IdMgr: request next available ID(s)
IdMgr-->>MoveOp: return newId(s)
MoveOp->>MoveOp: record idRemapping (originalId → newId)
MoveOp->>DepMgr: rewrite dependencies and subtask parent IDs
DepMgr-->>MoveOp: updated tasks with remapped references
MoveOp->>Storage: persist updated source/target tag data
Storage-->>API: persist success
API-->>User: respond with success message and renumbering details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
There was a problem hiding this comment.
Pull request overview
This PR updates cross-tag task moves to automatically renumber tasks when the destination tag already contains the same task ID, and surfaces renumbering details in both CLI/core responses and the MCP direct-function output.
Changes:
- Auto-renumber tasks on cross-tag ID collisions instead of throwing
TASK_ALREADY_EXISTS. - Attempt to remap dependencies within the moved batch to reflect any new IDs (including subtasks).
- Update integration coverage for the ID-collision scenario and enrich responses with renumbering details + a dependency validation tip.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/integration/move-task-cross-tag.integration.test.js | Updates the cross-tag collision test to expect successful move + renumber metadata. |
| scripts/modules/task-manager/move-task.js | Implements auto-renumbering during cross-tag moves, adds renumber info to responses, and adds dependency remapping logic. |
| mcp-server/src/core/direct-functions/move-task-cross-tag.js | Adds renumbering details to the MCP direct-function success message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (movedTask && Array.isArray(movedTask.dependencies)) { | ||
| movedTask.dependencies = movedTask.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 dependency remapping logic uses normalizeDependency(dep) and, when a remap exists, replaces the original dependency value with the new parent task ID. For dotted dependencies like "1.2" this drops the subtask portion, and for string dependencies like "2" it changes the type to number. This will corrupt subtask dependency references and can break expectations that string deps remain strings. Update the remap to preserve dotted suffixes (e.g., "oldParent.sub" -> "newParent.sub") and preserve the original value type where possible (string in -> string out).
| 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.
This same remapping pattern is applied to subtask.dependencies and has the same issue: dotted dependency strings (e.g., "2.1") will be converted to just the remapped parent ID, losing the ".1" portion, and string deps can become numbers. Adjust this block to preserve dotted subtask references and the original value type when rewriting dependencies.
| // 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}`; |
There was a problem hiding this comment.
When a task is renumbered (taskToMove.id = assignedId), any nested subtasks that carry a parentTaskId field (created by add-subtask/convert flows) will still point at the old parent ID. That makes the data internally inconsistent and can break logic that relies on parentTaskId (e.g., isTaskDependentOn). If subtasks have parentTaskId, update it to assignedId when it matches the old normalizedTaskId.
| 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}`; | ||
| } |
There was a problem hiding this comment.
The success message is built using sourceIds.length, but moveTasksBetweenTags can move additional tasks when withDependencies is true (and result.message / result.movedTasks reflect the actual moved count). Consider basing this message on result.movedTasks.length (or reusing result.message) so MCP output stays accurate for dependency-inclusive moves.
| // Task should be moved successfully with a new ID | ||
| expect(result.message).toContain('Successfully moved 1 tasks'); | ||
| expect(result.message).toContain('Renumbered'); | ||
| expect(result.movedTasks).toHaveLength(1); | ||
| expect(result.movedTasks[0].originalId).toBe(1); | ||
| expect(result.movedTasks[0].newId).toBe(2); // Next available ID after existing ID 1 | ||
|
|
||
| // Verify the target tag now has both tasks | ||
| expect(mockUtils.writeJSON).toHaveBeenCalled(); | ||
| const writtenData = mockUtils.writeJSON.mock.calls[0][1]; | ||
| expect(writtenData['in-progress'].tasks).toHaveLength(2); | ||
| expect(writtenData['in-progress'].tasks[0].id).toBe(1); // Original task | ||
| expect(writtenData['in-progress'].tasks[1].id).toBe(2); // Renumbered moved task | ||
| expect(writtenData['in-progress'].tasks[1].title).toBe('Backlog Task'); | ||
|
|
There was a problem hiding this comment.
This integration test validates auto-renumbering, but it doesn't exercise the new behavior of updating task/subtask dependencies within the moved batch when IDs are remapped. To prevent regressions (especially for dotted dependencies like "1.2"), consider extending the fixture to include at least one dependency pointing at a task that gets renumbered and asserting that the writtenData dependencies are updated accordingly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp-server/src/core/direct-functions/move-task-cross-tag.js`:
- Around line 120-136: The wrapper builds a success message using
sourceIds.length which can undercount when withDependencies expands the moved
batch; change the wrapper to use the actual moved batch information
(result.movedTasks) or reuse result.message instead of sourceIds.length so the
reported count and text match the core formatter. Locate the block that
constructs message (references: sourceIds, args.sourceTag, args.targetTag,
result.movedTasks, result.message) and replace the manual construction that uses
sourceIds.length with either using result.message directly or computing the
count from result.movedTasks.length and include renumberDetails from
result.movedTasks as currently done.
In `@scripts/modules/task-manager/move-task.js`:
- Around line 955-980: The current remapping normalizes dotted subtask refs into
plain numbers and corrupts sibling subtask refs; change the remapping logic so
it preserves the original dependency "shape": for movedTask.dependencies keep
existing behavior (map numeric IDs and leave dotted strings alone or map their
parent component if desired), but for movedTask.subtasks[].dependencies only
remap the parent ID inside a dotted-string (e.g. "1.2" → map the "1" via
idRemapping and reassemble as "5.2") and do NOT remap pure numeric deps (they
may be local sibling-subtask refs). Update the mapping code around movedTask,
normalizeDependency, idRemapping and subtask.dependencies to detect
dotted-string deps, split and remap only the parent segment, then reassemble,
leaving non-dotted numeric deps unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eab859b7-cbfb-4a2e-b6f6-6406683ac4da
📒 Files selected for processing (3)
mcp-server/src/core/direct-functions/move-task-cross-tag.jsscripts/modules/task-manager/move-task.jstests/integration/move-task-cross-tag.integration.test.js
| if (movedTask && Array.isArray(movedTask.dependencies)) { | ||
| movedTask.dependencies = movedTask.dependencies.map((dep) => { | ||
| const normalizedDep = normalizeDependency(dep); | ||
| if ( | ||
| Number.isFinite(normalizedDep) && | ||
| idRemapping.has(normalizedDep) | ||
| ) { | ||
| return idRemapping.get(normalizedDep); | ||
| } | ||
| return dep; | ||
| }); | ||
| } | ||
| // Also update subtask dependencies that reference remapped IDs | ||
| 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.
Preserve subtask reference shape when remapping dependencies.
Lines 956-980 normalize every dependency down to its parent task ID. That turns a dotted reference like 1.2 into 5, and inside subtask.dependencies it can also rewrite a local sibling-subtask reference like 1 as if it were a task ID. A renumbered batch move will therefore corrupt both task-level dotted refs and subtask-level dependency refs.
🛠️ Suggested direction
+function remapDependencyReference(dep, idRemapping, { remapPlainTaskIds = true } = {}) {
+ if (typeof dep === 'string' && dep.includes('.')) {
+ const [parentId, subtaskId] = dep.split('.');
+ const remappedParentId = idRemapping.get(parseInt(parentId, 10));
+ return remappedParentId ? `${remappedParentId}.${subtaskId}` : dep;
+ }
+
+ const normalizedDep = normalizeDependency(dep);
+ if (
+ remapPlainTaskIds &&
+ Number.isFinite(normalizedDep) &&
+ idRemapping.has(normalizedDep)
+ ) {
+ return idRemapping.get(normalizedDep);
+ }
+
+ return dep;
+}
...
- movedTask.dependencies = movedTask.dependencies.map((dep) => {
- const normalizedDep = normalizeDependency(dep);
- if (
- Number.isFinite(normalizedDep) &&
- idRemapping.has(normalizedDep)
- ) {
- return idRemapping.get(normalizedDep);
- }
- return dep;
- });
+ movedTask.dependencies = movedTask.dependencies.map((dep) =>
+ remapDependencyReference(dep, idRemapping)
+ );
...
- subtask.dependencies = subtask.dependencies.map((dep) => {
- const normalizedDep = normalizeDependency(dep);
- if (
- Number.isFinite(normalizedDep) &&
- idRemapping.has(normalizedDep)
- ) {
- return idRemapping.get(normalizedDep);
- }
- return dep;
- });
+ subtask.dependencies = subtask.dependencies.map((dep) =>
+ remapDependencyReference(dep, idRemapping, {
+ remapPlainTaskIds: false
+ })
+ );🤖 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 955 - 980, The
current remapping normalizes dotted subtask refs into plain numbers and corrupts
sibling subtask refs; change the remapping logic so it preserves the original
dependency "shape": for movedTask.dependencies keep existing behavior (map
numeric IDs and leave dotted strings alone or map their parent component if
desired), but for movedTask.subtasks[].dependencies only remap the parent ID
inside a dotted-string (e.g. "1.2" → map the "1" via idRemapping and reassemble
as "5.2") and do NOT remap pure numeric deps (they may be local sibling-subtask
refs). Update the mapping code around movedTask, normalizeDependency,
idRemapping and subtask.dependencies to detect dotted-string deps, split and
remap only the parent segment, then reassemble, leaving non-dotted numeric deps
unchanged.
…ate, message count - Preserve dotted subtask suffix when remapping dependencies during ID collision (e.g., "1.2" becomes "5.2" when task 1 remaps to 5, not just 5) - Update subtask parentTaskId when parent task is renumbered - Use actual moved task count instead of sourceIds.length in MCP message - Add integration test covering dotted dependency remapping scenario Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
TASK_ALREADY_EXISTSerror1 → 5)task-master validate-dependencieswhen renumbering occursTest plan
Fixes #1647
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests