fix: resolve deprecated transitive dependencies#1671
fix: resolve deprecated transitive dependencies#1671withsivram 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>
) Add npm overrides for `gaxios` (>=7.1.4) and `rimraf` (>=6.0.0) to eliminate deprecated transitive dependencies that produce warnings during installation: - glob@10.5.0: removed by upgrading gaxios to 7.1.4 (drops rimraf dep) - rimraf@5.0.10: no longer pulled in transitively - koa-router@14.0.0 and keygrip@1.1.0: already absent in current mcp-proxy@5.x (no pipenet dependency) - node-domexception@1.0.0: already resolved by existing node-fetch@2 override Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughThe PR implements automatic ID collision resolution during cross-tag task moves. When a task's ID conflicts with an existing task in the target tag, the system now renumbers the moved task to the next available ID instead of throwing an error, updates all dependency references accordingly, and reports the changes in the response message. Package overrides for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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 eliminate deprecated transitive dependency warnings during npm install by adding npm overrides, and it also changes cross-tag move behavior to auto-renumber tasks on ID collisions (with corresponding test/MCP response updates).
Changes:
- Add npm
overridesforgaxiosandrimrafinpackage.json. - Auto-renumber tasks during cross-tag moves when the target tag already contains the requested ID, and attempt to remap dependencies accordingly.
- Update integration test expectations for ID-collision behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
package.json |
Adds dependency overrides intended to remove deprecated transitive packages from the install tree. |
scripts/modules/task-manager/move-task.js |
Implements auto-renumbering on ID collisions and post-move dependency remapping. |
mcp-server/src/core/direct-functions/move-task-cross-tag.js |
Adjusts MCP direct-function response message to include renumbering details. |
tests/integration/move-task-cross-tag.integration.test.js |
Updates integration test to expect successful move with renumbering instead of an error. |
💡 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); | ||
| } |
There was a problem hiding this comment.
When updating dependencies after renumbering, this mapping loses information for dotted subtask references. For example, a dependency like "5.2" normalizes to parent 5, matches idRemapping, and then gets replaced with the numeric new ID only—dropping the ".2" suffix and changing the dependency meaning. The remap logic should preserve dotted dependencies by rewriting only the parent portion (and ideally preserve original string/number format).
| subtask.dependencies = subtask.dependencies.map((dep) => { | ||
| const normalizedDep = normalizeDependency(dep); | ||
| if ( | ||
| Number.isFinite(normalizedDep) && | ||
| idRemapping.has(normalizedDep) | ||
| ) { | ||
| return idRemapping.get(normalizedDep); | ||
| } |
There was a problem hiding this comment.
Subtask dependency remapping has the same issue as task-level dependencies: normalizeDependency("5.2") returns 5, so a remap will replace the entire string with the numeric new ID and drop the subtask portion. This can corrupt subtask-to-subtask or task-to-subtask dependency references after renumbering; update logic should rewrite the parent task ID while keeping any subtask suffix intact (and keep string deps as strings where applicable).
| // 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}`; |
There was a problem hiding this comment.
This success message uses sourceIds.length, which will be incorrect when --with-dependencies expands the move set (core can move more tasks than were explicitly requested). Consider using result.movedTasks.length (or just result.message from core) so the MCP response stays accurate.
| "overrides": { | ||
| "node-fetch": "^2.6.12", | ||
| "whatwg-url": "^11.0.0" | ||
| "whatwg-url": "^11.0.0", | ||
| "gaxios": "^7.1.4", | ||
| "rimraf": "^6.0.0" |
There was a problem hiding this comment.
The PR title/description focus on dependency override cleanup, but this change set also introduces functional behavior changes (auto-renumbering tasks on cross-tag ID collisions) and updates related tests/MCP responses. Please either update the PR description/title to cover the move-task behavior change (and its user-facing impact) or split the functional change into a separate PR to keep scope clear.
| "node-fetch": "^2.6.12", | ||
| "whatwg-url": "^11.0.0" | ||
| "whatwg-url": "^11.0.0", | ||
| "gaxios": "^7.1.4", |
There was a problem hiding this comment.
Adding a top-level override for gaxios forces version selection even for transitive dependents that declare an incompatible major range (e.g., package-lock shows gcp-metadata declares "gaxios": "^6.1.1"). This can cause subtle runtime breakage despite a clean install. Prefer a more targeted override (scoped to the specific dependency chain) or bump the direct dependency that brings in gaxios, and ensure the lockfile is regenerated/committed so CI/users resolve the same tree.
| "gaxios": "^7.1.4", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
mcp-server/src/core/direct-functions/move-task-cross-tag.js (2)
204-215: Error handler forTASK_ALREADY_EXISTSis now unreachable for cross-tag moves.With the auto-renumbering behavior, cross-tag moves no longer throw
TASK_ALREADY_EXISTS. This error handling block (lines 204-215) is effectively dead code for themoveTaskCrossTagDirectfunction.Consider removing or updating these suggestions since the primary advice ("Choose a different target tag without conflicting IDs") no longer applies — conflicts are now resolved automatically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/src/core/direct-functions/move-task-cross-tag.js` around lines 204 - 215, The TASK_ALREADY_EXISTS error branch in moveTaskCrossTagDirect is now dead due to auto-renumbering; remove or update the branch that checks error.code === 'TASK_ALREADY_EXISTS' || error.message?.includes('already exists in target tag') and stop setting errorCode = 'TASK_ALREADY_EXISTS' and the suggestions array there; either delete the entire conditional block or replace it with a note/suggestion reflecting that conflicts are auto-resolved (e.g., inform users about auto-renumbering behavior), and update any callers that rely on errorCode or suggestions from this branch accordingly.
120-136: Duplicated message construction may diverge from core logic.The direct function rebuilds the success message (lines 120-130) instead of using
result.messagereturned bymoveTasksBetweenTags. The core function atscripts/modules/task-manager/move-task.js:1016-1022already constructs an identical message with renumbering details.Consider using
result.messagedirectly to avoid duplication and ensure consistency:- // 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, + // result.message already includes renumbering details from core function moveOptions, sourceTag: args.sourceTag, targetTag: args.targetTag } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/src/core/direct-functions/move-task-cross-tag.js` around lines 120 - 136, The success message is being reconstructed in move-task-cross-tag.js instead of using the canonical message from moveTasksBetweenTags; replace the manual message build (the renumberedTasks mapping and concatenation that sets `message`) and instead use `result.message` returned by `moveTasksBetweenTags` (i.e., ensure the returned payload sets data.message = result.message or spreads result including its message) so the direct function relies on the core `result.message` for consistency with the logic in move-task.js.tests/integration/move-task-cross-tag.integration.test.js (1)
553-610: Consider adding test for batch collision with dependency remapping.The current test covers a single task collision. To validate the batch dependency update logic (lines 943-985 in move-task.js), consider adding a test where:
- Task A (id: 1) depends on Task B (id: 2)
- Both collide with existing IDs in the target tag
- Both get renumbered (e.g., 1→3, 2→4)
- A's dependency is updated from 2 to 4
This would ensure the cross-reference update path is exercised.
Would you like me to generate this additional test case?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/move-task-cross-tag.integration.test.js` around lines 553 - 610, Add a new integration test for moveTasksBetweenTags that simulates a batch collision with dependency remapping: create source tag tasks A(id:1, dependsOn:[2]) and B(id:2) and a target tag already containing ids 1 and 2 so both A and B must be renumbered (e.g., 1→3, 2→4); mockUtils.readJSON should return this dataset, call moveTasksBetweenTags with both IDs, then assert result.movedTasks contains the originalId→newId mappings for both tasks, assert the moved task A's dependency was updated from 2 to 4 in the saved payload (inspect mockUtils.writeJSON mock.calls[0][1]), assert the target tag now has both original and renumbered tasks with correct ids/titles/deps and the source tag is empty; reference moveTasksBetweenTags and the dependency remapping path in move-task.js to locate logic to test.scripts/modules/task-manager/move-task.js (1)
899-913: Subtask dependency string handling is unreachable for schema-validated data.The
SubtaskSchemaenforcesdependencies: z.array(z.number().int().positive())— numeric only. The code at lines 903–907 checks for string dependencies with dot notation (e.g.,"1.2"), which cannot appear in subtask dependency arrays that pass schema validation.This is defensive code that won't cause problems, but it's not reachable. If validation is strictly enforced during task deserialization, consider removing this unreachable branch.
🤖 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 899 - 913, The string-dot dependency handling inside the taskToMove.subtasks mapping is unreachable because SubtaskSchema enforces numeric-only dependencies; remove the typeof dep === 'string' && dep.includes('.') branch and replace it with numeric-only handling: inside the subtask.dependencies = subtask.dependencies.map(...) check for typeof dep === 'number' and if dep === normalizedTaskId return assignedId (otherwise return dep). Update the mapping logic around taskToMove.subtasks and variables normalizedTaskId and assignedId accordingly and delete the unreachable string-splitting code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcp-server/src/core/direct-functions/move-task-cross-tag.js`:
- Around line 204-215: The TASK_ALREADY_EXISTS error branch in
moveTaskCrossTagDirect is now dead due to auto-renumbering; remove or update the
branch that checks error.code === 'TASK_ALREADY_EXISTS' ||
error.message?.includes('already exists in target tag') and stop setting
errorCode = 'TASK_ALREADY_EXISTS' and the suggestions array there; either delete
the entire conditional block or replace it with a note/suggestion reflecting
that conflicts are auto-resolved (e.g., inform users about auto-renumbering
behavior), and update any callers that rely on errorCode or suggestions from
this branch accordingly.
- Around line 120-136: The success message is being reconstructed in
move-task-cross-tag.js instead of using the canonical message from
moveTasksBetweenTags; replace the manual message build (the renumberedTasks
mapping and concatenation that sets `message`) and instead use `result.message`
returned by `moveTasksBetweenTags` (i.e., ensure the returned payload sets
data.message = result.message or spreads result including its message) so the
direct function relies on the core `result.message` for consistency with the
logic in move-task.js.
In `@scripts/modules/task-manager/move-task.js`:
- Around line 899-913: The string-dot dependency handling inside the
taskToMove.subtasks mapping is unreachable because SubtaskSchema enforces
numeric-only dependencies; remove the typeof dep === 'string' &&
dep.includes('.') branch and replace it with numeric-only handling: inside the
subtask.dependencies = subtask.dependencies.map(...) check for typeof dep ===
'number' and if dep === normalizedTaskId return assignedId (otherwise return
dep). Update the mapping logic around taskToMove.subtasks and variables
normalizedTaskId and assignedId accordingly and delete the unreachable
string-splitting code.
In `@tests/integration/move-task-cross-tag.integration.test.js`:
- Around line 553-610: Add a new integration test for moveTasksBetweenTags that
simulates a batch collision with dependency remapping: create source tag tasks
A(id:1, dependsOn:[2]) and B(id:2) and a target tag already containing ids 1 and
2 so both A and B must be renumbered (e.g., 1→3, 2→4); mockUtils.readJSON should
return this dataset, call moveTasksBetweenTags with both IDs, then assert
result.movedTasks contains the originalId→newId mappings for both tasks, assert
the moved task A's dependency was updated from 2 to 4 in the saved payload
(inspect mockUtils.writeJSON mock.calls[0][1]), assert the target tag now has
both original and renumbered tasks with correct ids/titles/deps and the source
tag is empty; reference moveTasksBetweenTags and the dependency remapping path
in move-task.js to locate logic to test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d6d9b13-c8d7-4965-a890-8525cae48669
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
mcp-server/src/core/direct-functions/move-task-cross-tag.jspackage.jsonscripts/modules/task-manager/move-task.jstests/integration/move-task-cross-tag.integration.test.js
Summary
gaxios(^7.1.4) andrimraf(^6.0.0) to eliminate deprecated transitive dependency warnings duringnpm installgaxios@7.1.4dropsrimraffrom its dependencies, which removes therimraf@5 -> glob@10.5.0chainkoa-router,keygrip, andnode-domexceptionwere already absent in the current dependency tree (mcp-proxy@5 has no pipenet dependency, and the existing node-fetch@2 override prevents node-domexception)Fixes #1624
Test plan
npm lsshows nokoa-router,keygrip,node-domexception, orrimrafin the dependency treeglob@10.5.0is no longer present in production dependencies (only remains in dev-only packages like jest, mocha, @vscode/test-cli)gaxiosresolves to7.1.4(which has norimrafdependency)npm run build)npm install -g task-master-aiproduces no deprecated warnings for the four packages listed in the issue🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores