From 8783708e5e3389890a78fcf685d3da0580e73b3f Mon Sep 17 00:00:00 2001 From: Parthy <52548018+mm-parthy@users.noreply.github.com> Date: Thu, 28 Aug 2025 19:02:00 +0200 Subject: [PATCH] Improve cross-tag move UX and safety; add MCP suggestions and CLI tips (#1135) * docs: Auto-update and format models.md * docs(ui,cli): remove --force from cross-tag move guidance; recommend --with-dependencies/--ignore-dependencies - scripts/modules/ui.js: drop force tip in conflict resolution - scripts/modules/commands.js: remove force examples from move help - docs/cross-tag-task-movement.md: purge force mentions; add explicit with/ignore examples * test(move): update cross-tag move tests to drop --force; assert with/ignore deps behavior and current-tag fallback - CLI integration: remove force expectations, keep with/ignore, current-tag fallback - Integration: remove force-path test - Unit: add scoped traversal test, adjust fixtures to avoid id collision * fix(move): scope dependency traversal to source tag; tag-aware ignore-dependencies filtering - resolveDependencies: traverse only sourceTag tasks to avoid cross-tag contamination - filter dependent IDs to those present in source tag, numeric only - ignore-dependencies: drop deps pointing to tasks from sourceTag; keep targetTag deps * test(mcp): ensure cross-tag move passes only with/ignore options and returns conflict suggestions - new test: tests/unit/mcp/tools/move-task-cross-tag-options.test.js * feat(move): add advisory tips when ignoring cross-tag dependencies; add integration test case * feat(cli/move): improve ID collision UX for cross-tag moves\n\n- Print Next Steps tips when core returns them (e.g., after ignore-dependencies)\n- Add dedicated help block when an ID already exists in target tag * feat(move/mcp): improve ID collision UX and suggestions\n\n- Core: include suggestions on TASK_ALREADY_EXISTS errors\n- MCP: map ID collision to TASK_ALREADY_EXISTS with suggestions\n- Tests: add MCP unit test for ID collision suggestions * test(move/cli): print tips on ignore-dependencies results; print ID collision suggestions\n\n- CLI integration test: assert Next Steps tips printed when result.tips present\n- Integration test: assert TASK_ALREADY_EXISTS error includes suggestions payload * chore(changeset): add changeset for cross-tag move UX improvements (CLI/MCP/core/tests) * Add cross-tag task movement help and validation improvements - Introduced a detailed help command for cross-tag task movement, enhancing user guidance on usage and options. - Updated validation logic in `validateCrossTagMove` to include checks for indirect dependencies, improving accuracy in conflict detection. - Refactored tests to ensure comprehensive coverage of new validation scenarios and error handling. - Cleaned up documentation to reflect the latest changes in task movement functionality. * refactor(commands): remove redundant tips printing after move operation - Eliminated duplicate printing of tips for next steps after the move operation, streamlining the output for users. - This change enhances clarity by ensuring tips are only displayed when relevant, improving overall user experience. * docs(move): clarify "force move" options and improve examples - Updated documentation to replace the deprecated "force move" concept with clear alternatives: `--with-dependencies` and `--ignore-dependencies`. - Enhanced Scenario 3 with explicit options and improved inline comments for better readability. - Removed confusing commented code in favor of a straightforward note in the Force Move section. * chore: run formatter * Update .changeset/clarify-force-move-docs.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update docs/cross-tag-task-movement.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update tests/unit/scripts/modules/task-manager/move-task-cross-tag.test.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * test(move): add test for dependency traversal scoping with --with-dependencies option - Introduced a new test to ensure that the dependency traversal is limited to tasks from the source tag when using the --with-dependencies option, addressing potential ID collisions across tags. * test(move): enhance tips validation in cross-tag task movement integration test --------- Co-authored-by: github-actions[bot] Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- .changeset/clarify-force-move-docs.md | 5 + .changeset/strong-eagles-vanish.md | 12 + docs/cross-tag-task-movement.md | 19 +- .../direct-functions/move-task-cross-tag.js | 11 + scripts/modules/commands.js | 36 ++- scripts/modules/task-manager/move-task.js | 133 +++++++++-- scripts/modules/ui.js | 3 - tests/integration/cli/move-cross-tag.test.js | 210 +++++++----------- .../move-task-cross-tag.integration.test.js | 83 ++++--- .../tools/move-task-cross-tag-options.test.js | 135 +++++++++++ .../task-manager/move-task-cross-tag.test.js | 31 +++ 11 files changed, 480 insertions(+), 198 deletions(-) create mode 100644 .changeset/clarify-force-move-docs.md create mode 100644 .changeset/strong-eagles-vanish.md create mode 100644 tests/unit/mcp/tools/move-task-cross-tag-options.test.js diff --git a/.changeset/clarify-force-move-docs.md b/.changeset/clarify-force-move-docs.md new file mode 100644 index 00000000..2550f252 --- /dev/null +++ b/.changeset/clarify-force-move-docs.md @@ -0,0 +1,5 @@ +--- +"task-master-ai": patch +--- + +docs(move): clarify cross-tag move docs; deprecate "force"; add explicit --with-dependencies/--ignore-dependencies examples diff --git a/.changeset/strong-eagles-vanish.md b/.changeset/strong-eagles-vanish.md new file mode 100644 index 00000000..d69c0f5b --- /dev/null +++ b/.changeset/strong-eagles-vanish.md @@ -0,0 +1,12 @@ +--- +"task-master-ai": minor +--- + +feat(move): improve cross-tag move UX and safety + +- CLI: print "Next Steps" tips after cross-tag moves that used --ignore-dependencies (validate/fix guidance) +- CLI: show dedicated help block on ID collisions (destination tag already has the ID) +- Core: add structured suggestions to TASK_ALREADY_EXISTS errors +- MCP: map ID collision errors to TASK_ALREADY_EXISTS and include suggestions +- Tests: cover MCP options, error suggestions, CLI tips printing, and integration error payload suggestions +--- diff --git a/docs/cross-tag-task-movement.md b/docs/cross-tag-task-movement.md index 2dfbc0ac..d2ec45ad 100644 --- a/docs/cross-tag-task-movement.md +++ b/docs/cross-tag-task-movement.md @@ -65,11 +65,10 @@ This removes the dependency relationships and moves only the specified task. ### Force Move -Force the move even with dependency conflicts: +Note: Force moves are no longer supported. Instead, use one of these options: -```bash -task-master move --from=5 --from-tag=backlog --to-tag=in-progress --force -``` +- `--with-dependencies` — move dependents together +- `--ignore-dependencies` — break cross-tag dependencies ⚠️ **Warning**: This may break dependency relationships and should be used with caution. @@ -93,7 +92,7 @@ Resolution options: 2. Break dependencies: task-master move --from=5,6 --from-tag=backlog --to-tag=in-progress --ignore-dependencies 3. Validate and fix dependencies: task-master validate-dependencies && task-master fix-dependencies 4. Move dependencies first: task-master move --from=2,3 --from-tag=backlog --to-tag=in-progress - 5. Force move (may break dependencies): task-master move --from=5,6 --from-tag=backlog --to-tag=in-progress --force + 5. After deciding, re-run the move with either --with-dependencies or --ignore-dependencies ``` ### Subtask Movement Restrictions @@ -149,7 +148,6 @@ task-master fix-dependencies - **`--with-dependencies`**: When you want to maintain task relationships - **`--ignore-dependencies`**: When you want to break cross-tag dependencies -- **`--force`**: Only when you understand the consequences ### 3. Organize by Context @@ -265,9 +263,14 @@ task-master move --from=5 --from-tag=backlog --to-tag=done --ignore-dependencies ### Scenario 3: Force Move +Choose one of these options explicitly: + ```bash -# Force move despite conflicts -task-master move --from=5 --from-tag=backlog --to-tag=in-progress --force +# Move together with dependencies +task-master move --from=5 --from-tag=backlog --to-tag=in-progress --with-dependencies + +# Or break dependencies +task-master move --from=5 --from-tag=backlog --to-tag=in-progress --ignore-dependencies ``` ### Scenario 4: Moving Subtasks diff --git a/mcp-server/src/core/direct-functions/move-task-cross-tag.js b/mcp-server/src/core/direct-functions/move-task-cross-tag.js index 37f655b0..26cc5fe1 100644 --- a/mcp-server/src/core/direct-functions/move-task-cross-tag.js +++ b/mcp-server/src/core/direct-functions/move-task-cross-tag.js @@ -189,6 +189,17 @@ export async function moveTaskCrossTagDirect(args, log, context = {}) { 'Verify task IDs exist: task-master list', 'Check task details: task-master show ' ]; + } else if ( + error.code === 'TASK_ALREADY_EXISTS' || + error.message?.includes('already exists in target tag') + ) { + // Target tag has an ID collision + errorCode = 'TASK_ALREADY_EXISTS'; + suggestions = [ + 'Choose a different target tag without conflicting IDs', + 'Move a different set of IDs (avoid existing ones)', + 'If needed, move within-tag to a new ID first, then cross-tag move' + ]; } return { diff --git a/scripts/modules/commands.js b/scripts/modules/commands.js index bc6f82b6..d0fd8968 100644 --- a/scripts/modules/commands.js +++ b/scripts/modules/commands.js @@ -4103,12 +4103,7 @@ Examples: ' task-master move --from=5 --from-tag=backlog --to-tag=in-progress --ignore-dependencies' ) + '\n\n' + - chalk.white(' # Force move (may break dependencies)') + '\n' + - chalk.white( - ' task-master move --from=5 --from-tag=backlog --to-tag=in-progress --force' - ) + - '\n\n' + chalk.yellow.bold('Best Practices:') + '\n' + chalk.white( @@ -4119,10 +4114,6 @@ Examples: ' • Use --ignore-dependencies to break cross-tag dependencies' ) + '\n' + - chalk.white( - ' • Use --force only when you understand the consequences' - ) + - '\n' + chalk.white( ' • Check dependencies first: task-master validate-dependencies' ) + @@ -4184,6 +4175,12 @@ Examples: console.log(chalk.green(`✓ ${result.message}`)); + // Print any tips returned from the move operation (e.g., after ignoring dependencies) + if (Array.isArray(result.tips) && result.tips.length > 0) { + console.log('\n' + chalk.yellow.bold('Next Steps:')); + result.tips.forEach((t) => console.log(chalk.white(` • ${t}`))); + } + // Check if source tag still contains tasks before regenerating files const tasksData = readJSON( taskMaster.getTasksPath(), @@ -4450,6 +4447,27 @@ Examples: await handleWithinTagMove(moveContext); } } catch (error) { + const errMsg = String(error && (error.message || error)); + if (errMsg.includes('already exists in target tag')) { + console.error(chalk.red(`Error: ${errMsg}`)); + console.log( + '\n' + + chalk.yellow.bold('Conflict: ID already exists in target tag') + + '\n' + + chalk.white( + ' • Choose a different target tag without conflicting IDs' + ) + + '\n' + + chalk.white( + ' • Move a different set of IDs (avoid existing ones)' + ) + + '\n' + + chalk.white( + ' • If needed, move within-tag to a new ID first, then cross-tag move' + ) + ); + process.exit(1); + } handleMoveError(error, moveContext); } }); diff --git a/scripts/modules/task-manager/move-task.js b/scripts/modules/task-manager/move-task.js index 8b3213da..a9377503 100644 --- a/scripts/modules/task-manager/move-task.js +++ b/scripts/modules/task-manager/move-task.js @@ -61,6 +61,43 @@ const MOVE_ERROR_CODES = { INVALID_TARGET_TAG: 'INVALID_TARGET_TAG' }; +/** + * Normalize a dependency value to its numeric parent task ID. + * - Numbers are returned as-is (if finite) + * - Numeric strings are parsed ("5" -> 5) + * - Dotted strings return the parent portion ("5.2" -> 5) + * - Empty/invalid values return null + * - null/undefined are preserved + * @param {number|string|null|undefined} dep + * @returns {number|null|undefined} + */ +function normalizeDependency(dep) { + if (dep === null || dep === undefined) return dep; + if (typeof dep === 'number') return Number.isFinite(dep) ? dep : null; + if (typeof dep === 'string') { + const trimmed = dep.trim(); + if (trimmed === '') return null; + const parentPart = trimmed.includes('.') ? trimmed.split('.')[0] : trimmed; + const parsed = parseInt(parentPart, 10); + return Number.isFinite(parsed) ? parsed : null; + } + return null; +} + +/** + * Normalize an array of dependency values to numeric IDs. + * Preserves null/undefined input (returns as-is) and filters out invalid entries. + * @param {Array|null|undefined} deps + * @returns {Array|null|undefined} + */ +function normalizeDependencies(deps) { + if (deps === null || deps === undefined) return deps; + if (!Array.isArray(deps)) return deps; + return deps + .map((d) => normalizeDependency(d)) + .filter((n) => Number.isFinite(n)); +} + /** * Move one or more tasks/subtasks to new positions * @param {string} tasksPath - Path to tasks.json file @@ -674,15 +711,34 @@ async function resolveDependencies( ) { const { withDependencies = false, ignoreDependencies = false } = options; + // Scope allTasks to the source tag to avoid cross-tag contamination when + // computing dependency chains for --with-dependencies + const tasksInSourceTag = Array.isArray(allTasks) + ? allTasks.filter((t) => t && t.tag === sourceTag) + : []; + // Handle --with-dependencies flag first (regardless of cross-tag dependencies) if (withDependencies) { // Move dependent tasks along with main tasks - // Find ALL dependencies recursively within the same tag - const allDependentTaskIds = findAllDependenciesRecursively( + // Find ALL dependencies recursively, but only using tasks from the source tag + const allDependentTaskIdsRaw = findAllDependenciesRecursively( sourceTasks, - allTasks, + tasksInSourceTag, { maxDepth: 100, includeSelf: false } ); + + // Filter dependent IDs to those that actually exist in the source tag + const sourceTagIds = new Set( + tasksInSourceTag.map((t) => + typeof t.id === 'string' ? parseInt(t.id, 10) : t.id + ) + ); + const allDependentTaskIds = allDependentTaskIdsRaw.filter((depId) => { + // Only numeric task IDs are eligible to be moved (subtasks cannot be moved cross-tag) + const normalizedId = normalizeDependency(depId); + return Number.isFinite(normalizedId) && sourceTagIds.has(normalizedId); + }); + const allTaskIdsToMove = [...new Set([...taskIds, ...allDependentTaskIds])]; log( @@ -711,22 +767,31 @@ async function resolveDependencies( if (ignoreDependencies) { // Break cross-tag dependencies (edge case - shouldn't normally happen) sourceTasks.forEach((task) => { + const sourceTagTasks = tasksInSourceTag; + const targetTagTasks = Array.isArray(allTasks) + ? allTasks.filter((t) => t && t.tag === targetTag) + : []; task.dependencies = task.dependencies.filter((depId) => { - // Handle both task IDs and subtask IDs (e.g., "1.2") - let depTask = null; - if (typeof depId === 'string' && depId.includes('.')) { - // It's a subtask ID - extract parent task ID and find the parent task - const [parentId, subtaskId] = depId - .split('.') - .map((id) => parseInt(id, 10)); - depTask = allTasks.find((t) => t.id === parentId); - } else { - // It's a regular task ID - normalize to number for comparison - const normalizedDepId = - typeof depId === 'string' ? parseInt(depId, 10) : depId; - depTask = allTasks.find((t) => t.id === normalizedDepId); + const parentTaskId = normalizeDependency(depId); + + // If dependency resolves to a task in the source tag, drop it (would be cross-tag after move) + if ( + Number.isFinite(parentTaskId) && + sourceTagTasks.some((t) => t.id === parentTaskId) + ) { + return false; } - return !depTask || depTask.tag === targetTag; + + // If dependency resolves to a task in the target tag, keep it + if ( + Number.isFinite(parentTaskId) && + targetTagTasks.some((t) => t.id === parentTaskId) + ) { + return true; + } + + // Otherwise, keep as-is (unknown/unresolved dependency) + return true; }); }); @@ -810,7 +875,16 @@ async function executeMoveOperation( if (existingTaskIndex !== -1) { throw new MoveTaskError( MOVE_ERROR_CODES.TASK_ALREADY_EXISTS, - `Task ${taskId} already exists in target tag "${targetTag}"` + `Task ${taskId} already exists in target tag "${targetTag}"`, + { + conflictingId: normalizedTaskId, + targetTag, + suggestions: [ + 'Choose a different target tag without conflicting IDs', + 'Move a different set of IDs (avoid existing ones)', + 'If needed, move within-tag to a new ID first, then cross-tag move' + ] + } ); } @@ -851,7 +925,8 @@ async function finalizeMove( tasksPath, context, sourceTag, - targetTag + targetTag, + dependencyResolution ) { const { projectRoot } = context; const { rawData, movedTasks } = moveResult; @@ -859,10 +934,23 @@ async function finalizeMove( // Write the updated data writeJSON(tasksPath, rawData, projectRoot, null); - return { + const response = { message: `Successfully moved ${movedTasks.length} tasks from "${sourceTag}" to "${targetTag}"`, movedTasks }; + + // If we intentionally broke cross-tag dependencies, provide tips to validate & fix + if ( + dependencyResolution && + dependencyResolution.type === 'ignored-dependencies' + ) { + response.tips = [ + 'Run "task-master validate-dependencies" to check for dependency issues.', + 'Run "task-master fix-dependencies" to automatically repair dangling dependencies.' + ]; + } + + return response; } /** @@ -898,7 +986,7 @@ async function moveTasksBetweenTags( const { rawData, sourceTasks, allTasks } = await prepareTaskData(validation); // 3. Handle dependencies - const { tasksToMove } = await resolveDependencies( + const { tasksToMove, dependencyResolution } = await resolveDependencies( sourceTasks, allTasks, options, @@ -923,7 +1011,8 @@ async function moveTasksBetweenTags( tasksPath, context, sourceTag, - targetTag + targetTag, + dependencyResolution ); } diff --git a/scripts/modules/ui.js b/scripts/modules/ui.js index 17275132..65450d0b 100644 --- a/scripts/modules/ui.js +++ b/scripts/modules/ui.js @@ -2876,9 +2876,6 @@ export function displayCrossTagDependencyError( ` 4. Move dependencies first: task-master move --from=${conflicts.map((c) => c.dependencyId).join(',')} --from-tag=${conflicts[0].dependencyTag} --to-tag=${targetTag}` ); } - console.log( - ` 5. Force move (may break dependencies): task-master move --from=${sourceIds} --from-tag=${sourceTag} --to-tag=${targetTag} --force` - ); } /** diff --git a/tests/integration/cli/move-cross-tag.test.js b/tests/integration/cli/move-cross-tag.test.js index 8b904185..699d4223 100644 --- a/tests/integration/cli/move-cross-tag.test.js +++ b/tests/integration/cli/move-cross-tag.test.js @@ -158,20 +158,25 @@ describe('Cross-Tag Move CLI Integration', () => { ); try { - await moveTaskModule.moveTasksBetweenTags( + const result = await moveTaskModule.moveTasksBetweenTags( tasksPath, taskIds, sourceTag, toTag, { withDependencies, - ignoreDependencies, - force + ignoreDependencies } ); console.log(chalk.green('Successfully moved task(s) between tags')); + // Print advisory tips when present + if (result && Array.isArray(result.tips) && result.tips.length > 0) { + console.log('Next Steps:'); + result.tips.forEach((t) => console.log(` • ${t}`)); + } + // Generate task files for both tags await generateTaskFilesModule.default( tasksPath, @@ -185,6 +190,21 @@ describe('Cross-Tag Move CLI Integration', () => { ); } catch (error) { console.error(chalk.red(`Error: ${error.message}`)); + // Print ID collision guidance similar to CLI help block + if ( + typeof error?.message === 'string' && + error.message.includes('already exists in target tag') + ) { + console.log(''); + console.log('Conflict: ID already exists in target tag'); + console.log( + ' • Choose a different target tag without conflicting IDs' + ); + console.log(' • Move a different set of IDs (avoid existing ones)'); + console.log( + ' • If needed, move within-tag to a new ID first, then cross-tag move' + ); + } throw error; } } else { @@ -268,8 +288,7 @@ describe('Cross-Tag Move CLI Integration', () => { 'in-progress', { withDependencies: undefined, - ignoreDependencies: undefined, - force: undefined + ignoreDependencies: undefined } ); }); @@ -323,8 +342,7 @@ describe('Cross-Tag Move CLI Integration', () => { 'in-progress', { withDependencies: true, - ignoreDependencies: undefined, - force: undefined + ignoreDependencies: undefined } ); }); @@ -350,8 +368,7 @@ describe('Cross-Tag Move CLI Integration', () => { 'in-progress', { withDependencies: undefined, - ignoreDependencies: true, - force: undefined + ignoreDependencies: true } ); }); @@ -376,8 +393,7 @@ describe('Cross-Tag Move CLI Integration', () => { 'new-tag', { withDependencies: undefined, - ignoreDependencies: undefined, - force: undefined + ignoreDependencies: undefined } ); }); @@ -440,6 +456,57 @@ describe('Cross-Tag Move CLI Integration', () => { restore(); }); + it('should print advisory tips when result.tips are returned (ignore-dependencies)', async () => { + const { errorMessages, logMessages, restore } = captureConsoleAndExit(); + try { + // Arrange: mock move to return tips + mockMoveTasksBetweenTags.mockResolvedValue({ + message: 'ok', + tips: [ + 'Run "task-master validate-dependencies" to check for dependency issues.', + 'Run "task-master fix-dependencies" to automatically repair dangling dependencies.' + ] + }); + + await moveAction({ + from: '2', + fromTag: 'backlog', + toTag: 'in-progress', + ignoreDependencies: true + }); + + const joined = logMessages.join('\n'); + expect(joined).toContain('Next Steps'); + expect(joined).toContain('validate-dependencies'); + expect(joined).toContain('fix-dependencies'); + } finally { + restore(); + } + }); + + it('should print ID collision suggestions when target already has the ID', async () => { + const { errorMessages, logMessages, restore } = captureConsoleAndExit(); + try { + // Arrange: mock move to throw collision + const err = new Error( + 'Task 1 already exists in target tag "in-progress"' + ); + mockMoveTasksBetweenTags.mockRejectedValue(err); + + await expect( + moveAction({ from: '1', fromTag: 'backlog', toTag: 'in-progress' }) + ).rejects.toThrow('already exists in target tag'); + + const joined = logMessages.join('\n'); + expect(joined).toContain('Conflict: ID already exists in target tag'); + expect(joined).toContain('different target tag'); + expect(joined).toContain('different set of IDs'); + expect(joined).toContain('within-tag'); + } finally { + restore(); + } + }); + it('should handle same tag error correctly', async () => { const options = { from: '1', @@ -485,8 +552,7 @@ describe('Cross-Tag Move CLI Integration', () => { from: '1', toTag: 'in-progress', withDependencies: false, - ignoreDependencies: false, - force: false + ignoreDependencies: false // fromTag is intentionally not provided to test fallback }); @@ -498,8 +564,7 @@ describe('Cross-Tag Move CLI Integration', () => { 'in-progress', { withDependencies: false, - ignoreDependencies: false, - force: false + ignoreDependencies: false } ); @@ -536,8 +601,7 @@ describe('Cross-Tag Move CLI Integration', () => { 'in-progress', { withDependencies: undefined, - ignoreDependencies: undefined, - force: undefined + ignoreDependencies: undefined } ); @@ -555,32 +619,7 @@ describe('Cross-Tag Move CLI Integration', () => { ); }); - it('should handle --force flag correctly', async () => { - // Mock successful cross-tag move with force flag - mockMoveTasksBetweenTags.mockResolvedValue(undefined); - mockGenerateTaskFiles.mockResolvedValue(undefined); - - const options = { - from: '1', - fromTag: 'backlog', - toTag: 'in-progress', - force: true - }; - - await moveAction(options); - - expect(mockMoveTasksBetweenTags).toHaveBeenCalledWith( - expect.stringContaining('tasks.json'), - [1], - 'backlog', - 'in-progress', - { - withDependencies: undefined, - ignoreDependencies: undefined, - force: true // Force flag should be passed through - } - ); - }); + // Note: --force flag is no longer supported for cross-tag moves it('should fail when invalid task ID is provided', async () => { const options = { @@ -662,90 +701,11 @@ describe('Cross-Tag Move CLI Integration', () => { restore(); }); - it('should combine --with-dependencies and --force flags correctly', async () => { - // Mock successful cross-tag move with both flags - mockMoveTasksBetweenTags.mockResolvedValue(undefined); - mockGenerateTaskFiles.mockResolvedValue(undefined); + // Note: --force combinations removed - const options = { - from: '1,2', - fromTag: 'backlog', - toTag: 'in-progress', - withDependencies: true, - force: true - }; + // Note: --force combinations removed - await moveAction(options); - - expect(mockMoveTasksBetweenTags).toHaveBeenCalledWith( - expect.stringContaining('tasks.json'), - [1, 2], - 'backlog', - 'in-progress', - { - withDependencies: true, - ignoreDependencies: undefined, - force: true // Both flags should be passed - } - ); - }); - - it('should combine --ignore-dependencies and --force flags correctly', async () => { - // Mock successful cross-tag move with both flags - mockMoveTasksBetweenTags.mockResolvedValue(undefined); - mockGenerateTaskFiles.mockResolvedValue(undefined); - - const options = { - from: '1', - fromTag: 'backlog', - toTag: 'in-progress', - ignoreDependencies: true, - force: true - }; - - await moveAction(options); - - expect(mockMoveTasksBetweenTags).toHaveBeenCalledWith( - expect.stringContaining('tasks.json'), - [1], - 'backlog', - 'in-progress', - { - withDependencies: undefined, - ignoreDependencies: true, - force: true // Both flags should be passed - } - ); - }); - - it('should handle all three flags combined correctly', async () => { - // Mock successful cross-tag move with all flags - mockMoveTasksBetweenTags.mockResolvedValue(undefined); - mockGenerateTaskFiles.mockResolvedValue(undefined); - - const options = { - from: '1,2,3', - fromTag: 'backlog', - toTag: 'in-progress', - withDependencies: true, - ignoreDependencies: true, - force: true - }; - - await moveAction(options); - - expect(mockMoveTasksBetweenTags).toHaveBeenCalledWith( - expect.stringContaining('tasks.json'), - [1, 2, 3], - 'backlog', - 'in-progress', - { - withDependencies: true, - ignoreDependencies: true, - force: true // All three flags should be passed - } - ); - }); + // Note: --force combinations removed it('should handle whitespace in comma-separated task IDs', async () => { // Mock successful cross-tag move with whitespace diff --git a/tests/integration/move-task-cross-tag.integration.test.js b/tests/integration/move-task-cross-tag.integration.test.js index ac59b1a8..9a689787 100644 --- a/tests/integration/move-task-cross-tag.integration.test.js +++ b/tests/integration/move-task-cross-tag.integration.test.js @@ -426,6 +426,38 @@ describe('Cross-Tag Task Movement Integration Tests', () => { ); }); + it('should provide advisory tips when ignoreDependencies breaks deps', async () => { + // Move a task that has dependencies so cross-tag conflicts would be broken + const taskIds = [2]; // backlog:2 depends on 1 + const sourceTag = 'backlog'; + const targetTag = 'in-progress'; + + // Override cross-tag detection to simulate conflicts for this case + const depManager = await import( + '../../scripts/modules/dependency-manager.js' + ); + depManager.findCrossTagDependencies.mockReturnValueOnce([ + { taskId: 2, dependencyId: 1, dependencyTag: sourceTag } + ]); + + const result = await moveTasksBetweenTags( + testDataPath, + taskIds, + sourceTag, + targetTag, + { ignoreDependencies: true }, + { projectRoot: '/test/project' } + ); + + expect(Array.isArray(result.tips)).toBe(true); + const expectedTips = [ + 'Run "task-master validate-dependencies" to check for dependency issues.', + 'Run "task-master fix-dependencies" to automatically repair dangling dependencies.' + ]; + expect(result.tips).toHaveLength(expectedTips.length); + expect(result.tips).toEqual(expect.arrayContaining(expectedTips)); + }); + it('should move task without cross-tag dependency conflicts (since dependencies only exist within tags)', async () => { const taskIds = [2]; // Task 2 depends on Task 1 (both in same tag) const sourceTag = 'backlog'; @@ -564,6 +596,25 @@ describe('Cross-Tag Task Movement Integration Tests', () => { { projectRoot: '/test/project' } ) ).rejects.toThrow('Task 1 already exists in target tag "in-progress"'); + + // Validate suggestions on the error payload + try { + await moveTasksBetweenTags( + testDataPath, + taskIds, + sourceTag, + targetTag, + {}, + { projectRoot: '/test/project' } + ); + } catch (err) { + expect(err.code).toBe('TASK_ALREADY_EXISTS'); + expect(Array.isArray(err.data?.suggestions)).toBe(true); + const s = (err.data?.suggestions || []).join(' '); + expect(s).toContain('different target tag'); + expect(s).toContain('different set of IDs'); + expect(s).toContain('within-tag'); + } }); }); @@ -637,37 +688,7 @@ describe('Cross-Tag Task Movement Integration Tests', () => { ); }); - it('should handle force flag for dependency conflicts', async () => { - const taskIds = [2]; // Task 2 depends on Task 1 - const sourceTag = 'backlog'; - const targetTag = 'in-progress'; - - const result = await moveTasksBetweenTags( - testDataPath, - taskIds, - sourceTag, - targetTag, - { force: true }, - { projectRoot: '/test/project' } - ); - - // Verify task was moved despite dependency conflicts - expect(mockUtils.writeJSON).toHaveBeenCalledWith( - testDataPath, - expect.objectContaining({ - 'in-progress': expect.objectContaining({ - tasks: expect.arrayContaining([ - expect.objectContaining({ - id: 2, - tag: 'in-progress' - }) - ]) - }) - }), - '/test/project', - null - ); - }); + // Note: force flag deprecated for cross-tag moves; covered by with/ignore dependencies tests }); describe('Complex Scenarios', () => { diff --git a/tests/unit/mcp/tools/move-task-cross-tag-options.test.js b/tests/unit/mcp/tools/move-task-cross-tag-options.test.js new file mode 100644 index 00000000..91a6fab8 --- /dev/null +++ b/tests/unit/mcp/tools/move-task-cross-tag-options.test.js @@ -0,0 +1,135 @@ +import { jest } from '@jest/globals'; + +// Mocks +const mockFindTasksPath = jest + .fn() + .mockReturnValue('/test/path/.taskmaster/tasks/tasks.json'); +jest.unstable_mockModule( + '../../../../mcp-server/src/core/utils/path-utils.js', + () => ({ + findTasksPath: mockFindTasksPath + }) +); + +const mockEnableSilentMode = jest.fn(); +const mockDisableSilentMode = jest.fn(); +jest.unstable_mockModule('../../../../scripts/modules/utils.js', () => ({ + enableSilentMode: mockEnableSilentMode, + disableSilentMode: mockDisableSilentMode +})); + +// Spyable mock for moveTasksBetweenTags +const mockMoveTasksBetweenTags = jest.fn(); +jest.unstable_mockModule( + '../../../../scripts/modules/task-manager/move-task.js', + () => ({ + moveTasksBetweenTags: mockMoveTasksBetweenTags + }) +); + +// Import after mocks +const { moveTaskCrossTagDirect } = await import( + '../../../../mcp-server/src/core/direct-functions/move-task-cross-tag.js' +); + +describe('MCP Cross-Tag Move Direct Function - options & suggestions', () => { + const mockLog = { info: jest.fn(), warn: jest.fn(), error: jest.fn() }; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('passes only withDependencies/ignoreDependencies (no force) to core', async () => { + // Arrange: make core throw tag validation after call to capture params + mockMoveTasksBetweenTags.mockImplementation(() => { + const err = new Error('Source tag "invalid" not found or invalid'); + err.code = 'INVALID_SOURCE_TAG'; + throw err; + }); + + // Act + await moveTaskCrossTagDirect( + { + sourceIds: '1,2', + sourceTag: 'backlog', + targetTag: 'in-progress', + withDependencies: true, + projectRoot: '/test' + }, + mockLog + ); + + // Assert options argument (5th param) + expect(mockMoveTasksBetweenTags).toHaveBeenCalled(); + const args = mockMoveTasksBetweenTags.mock.calls[0]; + const moveOptions = args[4]; + expect(moveOptions).toEqual({ + withDependencies: true, + ignoreDependencies: false + }); + expect('force' in moveOptions).toBe(false); + }); + + it('returns conflict suggestions on cross-tag dependency conflicts', async () => { + // Arrange: core throws cross-tag dependency conflicts + mockMoveTasksBetweenTags.mockImplementation(() => { + const err = new Error( + 'Cannot move tasks: 2 cross-tag dependency conflicts found' + ); + err.code = 'CROSS_TAG_DEPENDENCY_CONFLICTS'; + throw err; + }); + + // Act + const result = await moveTaskCrossTagDirect( + { + sourceIds: '1', + sourceTag: 'backlog', + targetTag: 'in-progress', + projectRoot: '/test' + }, + mockLog + ); + + // Assert + expect(result.success).toBe(false); + expect(result.error.code).toBe('CROSS_TAG_DEPENDENCY_CONFLICT'); + expect(Array.isArray(result.error.suggestions)).toBe(true); + // Key suggestions + const s = result.error.suggestions.join(' '); + expect(s).toContain('--with-dependencies'); + expect(s).toContain('--ignore-dependencies'); + expect(s).toContain('validate-dependencies'); + expect(s).toContain('Move dependencies first'); + }); + + it('returns ID collision suggestions when target tag already has the ID', async () => { + // Arrange: core throws TASK_ALREADY_EXISTS structured error + mockMoveTasksBetweenTags.mockImplementation(() => { + const err = new Error( + 'Task 1 already exists in target tag "in-progress"' + ); + err.code = 'TASK_ALREADY_EXISTS'; + throw err; + }); + + // Act + const result = await moveTaskCrossTagDirect( + { + sourceIds: '1', + sourceTag: 'backlog', + targetTag: 'in-progress', + projectRoot: '/test' + }, + mockLog + ); + + // Assert + expect(result.success).toBe(false); + expect(result.error.code).toBe('TASK_ALREADY_EXISTS'); + const joined = (result.error.suggestions || []).join(' '); + expect(joined).toContain('different target tag'); + expect(joined).toContain('different set of IDs'); + expect(joined).toContain('within-tag'); + }); +}); diff --git a/tests/unit/scripts/modules/task-manager/move-task-cross-tag.test.js b/tests/unit/scripts/modules/task-manager/move-task-cross-tag.test.js index d6d98eb1..6126abb6 100644 --- a/tests/unit/scripts/modules/task-manager/move-task-cross-tag.test.js +++ b/tests/unit/scripts/modules/task-manager/move-task-cross-tag.test.js @@ -219,6 +219,37 @@ describe('Cross-Tag Task Movement', () => { }); }); + // New test: ensure with-dependencies only traverses tasks from the source tag + it('should scope dependency traversal to source tag when using --with-dependencies', async () => { + findCrossTagDependencies.mockReturnValue([]); + validateSubtaskMove.mockImplementation(() => {}); + + const result = await moveTasksBetweenTags( + mockTasksPath, + [1], // backlog:1 depends on backlog:2 + 'backlog', + 'in-progress', + { withDependencies: true }, + mockContext + ); + + // Write should include backlog:2 moved, and must NOT traverse or fetch dependencies from the target tag + expect(writeJSON).toHaveBeenCalledWith( + mockTasksPath, + expect.objectContaining({ + 'in-progress': expect.objectContaining({ + tasks: expect.arrayContaining([ + expect.objectContaining({ id: 1 }), + expect.objectContaining({ id: 2 }) // the backlog:2 now moved + // ensure existing in-progress:2 remains (by id) but we don't double-add or fetch deps from it + ]) + }) + }), + mockContext.projectRoot, + null + ); + }); + describe('moveTasksBetweenTags', () => { it('should move tasks without dependencies successfully', async () => { // Mock the dependency functions to return no conflicts