fix: merge analyze-complexity results across --from/--to runs#1668
fix: merge analyze-complexity results across --from/--to runs#1668withsivram 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>
…ltoledano#1644) The merge logic incorrectly filtered existing report entries against tasksData.tasks (the --from/--to filtered subset) via currentTagTaskIds, which dropped entries from earlier runs that analyzed different ranges. Since the report file is already tag-specific (resolved by resolveComplexityReportOutputPath), all entries in it belong to the current tag. The fix removes the currentTagTaskIds filter so that all existing entries not re-analyzed in the current run are preserved. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 29e3daf 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 |
📝 WalkthroughWalkthroughThis PR implements automatic task ID renumbering when moving tasks across tags that encounter ID collisions, fixes complexity report merging to preserve existing entries across multiple Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant moveTask as move-task.js<br/>(executeMoveOperation)
participant deps as Dependency<br/>Processor
participant storage as Task Storage
participant response as move-task-cross-tag.js<br/>(response builder)
User->>moveTask: Initiate move with collision risk
moveTask->>moveTask: Check ID collision in target
alt ID Collision Detected
moveTask->>moveTask: Compute next available ID
moveTask->>moveTask: Update task.id (renumber)
moveTask->>moveTask: Record idRemapping
end
moveTask->>deps: Update dependencies<br/>using idRemapping
deps->>deps: Rewrite dependency strings<br/>(old parent ID → new ID)
deps->>moveTask: Return updated task
moveTask->>storage: Write tasks to target tag
storage->>moveTask: Confirm write
moveTask->>response: Pass result with<br/>originalId/newId
response->>response: Build dynamic message<br/>including renumbering info
response->>User: Return success + renumbering details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 fixes incremental analyze-complexity runs so previously computed analyses are preserved when running multiple --from/--to ranges, and also introduces new cross-tag move behavior that auto-renumbers tasks when IDs collide.
Changes:
- Fix
analyze-complexityreport merging to preserve existing entries not re-analyzed in the current run. - Update cross-tag task moves to auto-renumber on ID collisions and return renumbering details in responses.
- Adjust integration tests and MCP direct-function messaging to match the new move behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/modules/task-manager/analyze-task-complexity.js |
Removes range-derived filtering during merge so reports accumulate across multiple --from/--to runs. |
scripts/modules/task-manager/move-task.js |
Auto-renumbers moved tasks on ID collisions and attempts to remap dependencies accordingly. |
mcp-server/src/core/direct-functions/move-task-cross-tag.js |
Surfaces renumbering details in the returned message for MCP callers. |
tests/integration/move-task-cross-tag.integration.test.js |
Updates ID-collision test to expect successful move with renumbering rather than an error. |
.changeset/fix-complexity-merge.md |
Adds a patch changeset for the analyze-complexity merge fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 here uses normalizeDependency(dep), which intentionally strips the subtask portion from dotted IDs (e.g. "12.3" -> 12). If a task dependency is a subtask reference string, this code will replace it with a bare number (the new parent ID), losing the ".3" suffix and breaking the dependency reference. Consider handling dotted-string dependencies explicitly by remapping only the parent portion while preserving the subtask suffix, and only using numeric replacement for pure task-ID dependencies.
| 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.
Subtask dependencies can be full task/subtask IDs like "5.2" (see dependency-manager validation). Because normalizeDependency() returns only the parent task ID for dotted strings, this remapping will convert "5.2" into a number when task 5 is renumbered, dropping the subtask ID and corrupting the dependency graph. Update this logic to preserve dotted-string format by rewriting just the parent ID (e.g. "5.2" -> ".2").
| // Keep all existing entries that weren't re-analyzed in this run. | ||
| // The report file is already tag-specific (resolved by resolveComplexityReportOutputPath), | ||
| // so all entries in the existing report belong to the current tag. | ||
| // Previous logic incorrectly filtered against tasksData.tasks (the --from/--to filtered | ||
| // subset), which dropped entries from earlier runs that analyzed different ranges. |
There was a problem hiding this comment.
The merge logic assumes the report file is always tag-specific, but resolveComplexityReportOutputPath() returns the explicit --output path unchanged. If a user reuses the same explicit output path across tags, this change will now preserve and merge entries from other tags instead of filtering them out. Consider either recording the tag in report meta and validating it on read, or scoping preservation to task IDs that exist in the current tag (but based on the full tag task list, not the --from/--to subset).
| --- | ||
| 'task-master-ai': patch | ||
| --- | ||
|
|
||
| Fix analyze-complexity to merge results with existing report across --from/--to runs instead of overwriting |
There was a problem hiding this comment.
This changeset only mentions the analyze-complexity merge fix, but this PR also changes cross-tag move behavior to auto-renumber on ID collisions (and updates MCP + integration tests accordingly). To avoid release-note/versioning surprises, consider updating the changeset (or adding a second one) to include the move-task behavior change, and/or align the PR description/title with the full scope.
![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
🧹 Nitpick comments (4)
scripts/modules/task-manager/analyze-task-complexity.js (1)
470-498: Fix looks correct—merge logic now properly preserves entries from prior runs.The removal of the
currentTagTaskIdsfilter is the right fix. Since the report path is already tag-specific (viaresolveComplexityReportOutputPath), all entries in the file belong to the correct tag and should be preserved.One optional enhancement: consider sorting
finalComplexityAnalysisbytaskIdbefore writing to make the report easier to navigate manually.,
🔧 Optional: Sort merged entries by taskId
// Combine: existing entries preserved from prior runs + new/updated analyses finalComplexityAnalysis = [ ...existingEntriesNotAnalyzed, ...complexityAnalysis - ]; + ].sort((a, b) => a.taskId - b.taskId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/modules/task-manager/analyze-task-complexity.js` around lines 470 - 498, The merge logic is correct but add a deterministic sort to finalComplexityAnalysis before writing so the report is easier to inspect: after building finalComplexityAnalysis (from existingEntriesNotAnalyzed and complexityAnalysis) perform a sort by taskId (ensure numeric comparison if taskId is numeric, otherwise lexicographic) so entries are ordered; this change touches finalComplexityAnalysis and should be done just before the write that uses the file path resolved by resolveComplexityReportOutputPath.mcp-server/src/core/direct-functions/move-task-cross-tag.js (2)
204-215: Consider removing or documenting defensive error handling forTASK_ALREADY_EXISTS.With the new auto-renumbering behavior,
moveTasksBetweenTagsno longer throwsTASK_ALREADY_EXISTSfor cross-tag ID collisions. This error handling block is now unreachable for this direct function.You could either:
- Remove this block as dead code
- Add a comment explaining it's defensive code for potential future behavior changes
🤖 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 error branch that checks for TASK_ALREADY_EXISTS in move-task-cross-tag.js (the else-if that tests error.code === 'TASK_ALREADY_EXISTS' || error.message?.includes('already exists in target tag')) is now unreachable because moveTasksBetweenTags no longer throws that error; remove that entire branch and its suggestions array to eliminate dead code, or if you prefer to keep defensive handling, replace it with a clear comment on why the check remains (referencing moveTasksBetweenTags and TASK_ALREADY_EXISTS) so future readers know it's intentionally retained for potential behavior changes.
120-140: Redundant message construction duplicates core function logic.The
moveTasksBetweenTagscore function already returns aresult.messagethat includes renumbering details (seefinalizeMoveinmove-task.jslines 1016-1022). Here, the same logic is duplicated to computemessage, which then overwritesresult.messagein the response.Consider using the message directly from the result to avoid duplication and keep the logic centralized:
♻️ Proposed simplification
- // 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, 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 - 140, The response builds a duplicate `message` from `result` (using `renumberedTasks`) which overwrites the core `moveTasksBetweenTags`/`result.message` produced by `finalizeMove` in `move-task.js`; remove the local reconstruction and instead use `result.message` (with a short fallback to the original success string if `result.message` is missing) when building the returned `data` object. In practice, delete the `renumberedTasks` computation and `message` assembly and set `data.message` to `result.message || \`Successfully moved ${sourceIds.length} task(s) from "${args.sourceTag}" to "${args.targetTag}"\`` while preserving `...result`, `moveOptions`, `sourceTag` and `targetTag`.tests/integration/move-task-cross-tag.integration.test.js (1)
553-610: Consider adding tests for dependency remapping during renumbering.The current test validates basic ID collision handling. Consider adding tests for:
- Moving multiple tasks where some need renumbering
- Tasks with dependencies on other moved tasks that get renumbered (verifying dependency IDs are updated)
This would help catch edge cases like subtask dependency references being incorrectly handled.
🤖 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 integration tests in tests/integration/move-task-cross-tag.integration.test.js around the moveTasksBetweenTags scenario to cover dependency remapping and multiple-renumbering: create mockUtils.readJSON data with multiple tasks moved from source to target where some target IDs collide and some moved tasks depend on other moved tasks, call moveTasksBetweenTags, then assert result.movedTasks contains correct originalId→newId mappings for each moved task and that each moved task's dependencies were updated to the new IDs; also verify mockUtils.writeJSON's writtenData for the target tag contains both original and renumbered tasks with dependency arrays referencing the updated IDs and the source tag is emptied as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/modules/task-manager/move-task.js`:
- Around line 955-965: movedTask.dependencies mapping currently normalizes
dependencies with normalizeDependency and blindly replaces the whole dependency
when idRemapping has the normalized value, which breaks dotted subtask refs like
"5.2" (becoming "6" and losing ".2"). Update the mapping in
movedTask.dependencies to detect dotted/string deps, split into baseId and subId
(preserve suffix after the first dot), normalize only the baseId using
normalizeDependency, if idRemapping.has(normalizedBaseId) replace the base with
idRemapping.get(normalizedBaseId) and reattach the original subId (e.g., newBase
+ "." + subId), otherwise leave the original dependency unchanged; keep this
logic inside the same movedTask.dependencies map callback so it uses the
existing normalizeDependency and idRemapping symbols.
- Around line 967-983: The current post-move update in movedTask.subtasks is
using normalizeDependency(dep) which strips subtask suffixes, so when
idRemapping maps a parent ID (normalizedDep) you end up returning the mapped
parent as a bare number and lose the original subtask portion (e.g., "5.2" ->
6). Update the mapping in the movedTask.subtasks loop to detect and preserve any
subtask suffix from the original dep: extract the suffix (e.g., the ".N" part)
from the original dep string when present, look up the remapped parent via
idRemapping.get(normalizedDep), and return the remapped parent combined with the
preserved suffix (as a string) instead of returning the raw mapped number; still
fall back to returning the original dep when no remap applies. Ensure this
change touches the block using movedTask.subtasks, normalizeDependency,
idRemapping, and subtask.dependencies.
---
Nitpick comments:
In `@mcp-server/src/core/direct-functions/move-task-cross-tag.js`:
- Around line 204-215: The error branch that checks for TASK_ALREADY_EXISTS in
move-task-cross-tag.js (the else-if that tests error.code ===
'TASK_ALREADY_EXISTS' || error.message?.includes('already exists in target
tag')) is now unreachable because moveTasksBetweenTags no longer throws that
error; remove that entire branch and its suggestions array to eliminate dead
code, or if you prefer to keep defensive handling, replace it with a clear
comment on why the check remains (referencing moveTasksBetweenTags and
TASK_ALREADY_EXISTS) so future readers know it's intentionally retained for
potential behavior changes.
- Around line 120-140: The response builds a duplicate `message` from `result`
(using `renumberedTasks`) which overwrites the core
`moveTasksBetweenTags`/`result.message` produced by `finalizeMove` in
`move-task.js`; remove the local reconstruction and instead use `result.message`
(with a short fallback to the original success string if `result.message` is
missing) when building the returned `data` object. In practice, delete the
`renumberedTasks` computation and `message` assembly and set `data.message` to
`result.message || \`Successfully moved ${sourceIds.length} task(s) from
"${args.sourceTag}" to "${args.targetTag}"\`` while preserving `...result`,
`moveOptions`, `sourceTag` and `targetTag`.
In `@scripts/modules/task-manager/analyze-task-complexity.js`:
- Around line 470-498: The merge logic is correct but add a deterministic sort
to finalComplexityAnalysis before writing so the report is easier to inspect:
after building finalComplexityAnalysis (from existingEntriesNotAnalyzed and
complexityAnalysis) perform a sort by taskId (ensure numeric comparison if
taskId is numeric, otherwise lexicographic) so entries are ordered; this change
touches finalComplexityAnalysis and should be done just before the write that
uses the file path resolved by resolveComplexityReportOutputPath.
In `@tests/integration/move-task-cross-tag.integration.test.js`:
- Around line 553-610: Add integration tests in
tests/integration/move-task-cross-tag.integration.test.js around the
moveTasksBetweenTags scenario to cover dependency remapping and
multiple-renumbering: create mockUtils.readJSON data with multiple tasks moved
from source to target where some target IDs collide and some moved tasks depend
on other moved tasks, call moveTasksBetweenTags, then assert result.movedTasks
contains correct originalId→newId mappings for each moved task and that each
moved task's dependencies were updated to the new IDs; also verify
mockUtils.writeJSON's writtenData for the target tag contains both original and
renumbered tasks with dependency arrays referencing the updated IDs and the
source tag is emptied as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2127e5c8-94f5-4a35-b5c5-70a95d652342
📒 Files selected for processing (5)
.changeset/fix-complexity-merge.mdmcp-server/src/core/direct-functions/move-task-cross-tag.jsscripts/modules/task-manager/analyze-task-complexity.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; | ||
| }); |
There was a problem hiding this comment.
Same subtask dependency issue affects top-level task dependencies.
The same issue exists here: if a top-level task has a dependency like "5.2" (referencing a subtask), and task 5 was renumbered to 6, this code would replace "5.2" with 6, losing the subtask reference.
🐛 Proposed fix to handle dotted dependencies
if (movedTask && Array.isArray(movedTask.dependencies)) {
movedTask.dependencies = movedTask.dependencies.map((dep) => {
+ // Handle dotted subtask references like "5.2"
+ if (typeof dep === 'string' && dep.includes('.')) {
+ const [depParent, depSub] = dep.split('.');
+ const parentId = parseInt(depParent, 10);
+ if (Number.isFinite(parentId) && idRemapping.has(parentId)) {
+ return `${idRemapping.get(parentId)}.${depSub}`;
+ }
+ return dep;
+ }
+ // Handle numeric dependencies
const normalizedDep = normalizeDependency(dep);
if (
Number.isFinite(normalizedDep) &&
idRemapping.has(normalizedDep)
) {
return idRemapping.get(normalizedDep);
}
return dep;
});
}🤖 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 - 965,
movedTask.dependencies mapping currently normalizes dependencies with
normalizeDependency and blindly replaces the whole dependency when idRemapping
has the normalized value, which breaks dotted subtask refs like "5.2" (becoming
"6" and losing ".2"). Update the mapping in movedTask.dependencies to detect
dotted/string deps, split into baseId and subId (preserve suffix after the first
dot), normalize only the baseId using normalizeDependency, if
idRemapping.has(normalizedBaseId) replace the base with
idRemapping.get(normalizedBaseId) and reattach the original subId (e.g., newBase
+ "." + subId), otherwise leave the original dependency unchanged; keep this
logic inside the same movedTask.dependencies map callback so it uses the
existing normalizeDependency and idRemapping symbols.
| // 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.
Subtask dependency references lose subtask portion when parent ID is remapped.
When updating subtask dependencies in the post-move pass, the code uses normalizeDependency which extracts only the parent ID. If a subtask dependency like "5.2" exists and task 5 was renumbered to 6, the code replaces "5.2" with 6 (a plain number), losing the .2 subtask portion.
This differs from the earlier logic at lines 899-913 which correctly preserves the subtask portion using string manipulation.
🐛 Proposed fix to preserve subtask portion during remapping
if (movedTask && Array.isArray(movedTask.subtasks)) {
movedTask.subtasks.forEach((subtask) => {
if (Array.isArray(subtask.dependencies)) {
subtask.dependencies = subtask.dependencies.map((dep) => {
+ // Handle dotted subtask references like "5.2"
+ if (typeof dep === 'string' && dep.includes('.')) {
+ const [depParent, depSub] = dep.split('.');
+ const parentId = parseInt(depParent, 10);
+ if (Number.isFinite(parentId) && idRemapping.has(parentId)) {
+ return `${idRemapping.get(parentId)}.${depSub}`;
+ }
+ return dep;
+ }
+ // Handle numeric dependencies
const normalizedDep = normalizeDependency(dep);
if (
Number.isFinite(normalizedDep) &&
idRemapping.has(normalizedDep)
) {
return idRemapping.get(normalizedDep);
}
return dep;
});
}
});
}🤖 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 967 - 983, The
current post-move update in movedTask.subtasks is using normalizeDependency(dep)
which strips subtask suffixes, so when idRemapping maps a parent ID
(normalizedDep) you end up returning the mapped parent as a bare number and lose
the original subtask portion (e.g., "5.2" -> 6). Update the mapping in the
movedTask.subtasks loop to detect and preserve any subtask suffix from the
original dep: extract the suffix (e.g., the ".N" part) from the original dep
string when present, look up the remapped parent via
idRemapping.get(normalizedDep), and return the remapped parent combined with the
preserved suffix (as a string) instead of returning the raw mapped number; still
fall back to returning the original dep when no remap applies. Ensure this
change touches the block using movedTask.subtasks, normalizeDependency,
idRemapping, and subtask.dependencies.
Summary
analyze-complexitythat incorrectly dropped existing report entries when running with--from/--torangescurrentTagTaskIds(derived fromtasksData.tasks, the filtered subset) instead of preserving all existing entries in the tag-specific report filecurrentTagTaskIdsfilter since the report file is already tag-specific (resolved byresolveComplexityReportOutputPath), so all entries in it belong to the correct tagFixes #1644
Test plan
analyze-complexity --from=1 --to=5, verify report contains entries for tasks 1-5analyze-complexity --from=6 --to=10, verify report now contains entries for tasks 1-10 (merged)analyze-complexity --from=3 --to=7, verify entries for 1-2 and 8-10 are preserved, entries for 3-7 are updatedanalyze-complexitywithout--from/--toto verify full analysis still works🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests