diff --git a/.changeset/plain-falcons-serve.md b/.changeset/plain-falcons-serve.md new file mode 100644 index 00000000..11bf91ac --- /dev/null +++ b/.changeset/plain-falcons-serve.md @@ -0,0 +1,7 @@ +--- +"task-master-ai": patch +--- + +Fix cross-level task dependencies not being saved + +Fixes an issue where adding dependencies between subtasks and top-level tasks (e.g., `task-master add-dependency --id=2.2 --depends-on=11`) would report success but fail to persist the changes. Dependencies can now be created in both directions between any task levels. diff --git a/scripts/modules/dependency-manager.js b/scripts/modules/dependency-manager.js index 9e65b7e0..e7c618a2 100644 --- a/scripts/modules/dependency-manager.js +++ b/scripts/modules/dependency-manager.js @@ -312,18 +312,23 @@ async function removeDependency(tasksPath, taskId, dependencyId, context = {}) { // Check if the dependency exists by comparing string representations const dependencyIndex = targetTask.dependencies.findIndex((dep) => { - // Convert both to strings for comparison - let depStr = String(dep); - - // Special handling for numeric IDs that might be subtask references - if (typeof dep === 'number' && dep < 100 && isSubtask) { - // It's likely a reference to another subtask in the same parent task - // Convert to full format for comparison (e.g., 2 -> "1.2" for a subtask in task 1) - const [parentId] = formattedTaskId.split('.'); - depStr = `${parentId}.${dep}`; + // Direct string comparison (handles both numeric IDs and dot notation) + const depStr = String(dep); + if (depStr === normalizedDependencyId) { + return true; } - return depStr === normalizedDependencyId; + // For subtasks: handle numeric dependencies that might be references to other subtasks + // in the same parent (e.g., subtask 1.2 depending on subtask 1.1 stored as just "1") + if (typeof dep === 'number' && dep < 100 && isSubtask) { + const [parentId] = formattedTaskId.split('.'); + const fullSubtaskRef = `${parentId}.${dep}`; + if (fullSubtaskRef === normalizedDependencyId) { + return true; + } + } + + return false; }); if (dependencyIndex === -1) { @@ -396,8 +401,9 @@ function isCircularDependency(tasks, taskId, chain = []) { task = parentTask.subtasks.find((st) => st.id === subtaskId); } } else { - // Regular task - task = tasks.find((t) => String(t.id) === taskIdStr); + // Regular task - handle both string and numeric task IDs + const taskIdNum = parseInt(taskIdStr, 10); + task = tasks.find((t) => t.id === taskIdNum || String(t.id) === taskIdStr); } if (!task) { diff --git a/tests/fixture/test-tasks.json b/tests/fixture/test-tasks.json deleted file mode 100644 index 6b99c177..00000000 --- a/tests/fixture/test-tasks.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "tasks": [ - { - "id": 1, - "dependencies": [], - "subtasks": [ - { - "id": 1, - "dependencies": [] - } - ] - } - ] -} \ No newline at end of file diff --git a/tests/fixtures/sample-tasks.js b/tests/fixtures/sample-tasks.js index e1fb53c3..c4ad2b1e 100644 --- a/tests/fixtures/sample-tasks.js +++ b/tests/fixtures/sample-tasks.js @@ -88,3 +88,38 @@ export const emptySampleTasks = { }, tasks: [] }; + +export const crossLevelDependencyTasks = { + tasks: [ + { + id: 2, + title: 'Task 2 with subtasks', + description: 'Parent task', + status: 'pending', + dependencies: [], + subtasks: [ + { + id: 1, + title: 'Subtask 2.1', + description: 'First subtask', + status: 'pending', + dependencies: [] + }, + { + id: 2, + title: 'Subtask 2.2', + description: 'Second subtask that should depend on Task 11', + status: 'pending', + dependencies: [] + } + ] + }, + { + id: 11, + title: 'Task 11', + description: 'Top-level task that 2.2 should depend on', + status: 'done', + dependencies: [] + } + ] +}; diff --git a/tests/unit/dependency-manager.test.js b/tests/unit/dependency-manager.test.js index b1b430ef..deda7cf4 100644 --- a/tests/unit/dependency-manager.test.js +++ b/tests/unit/dependency-manager.test.js @@ -4,18 +4,47 @@ import { jest } from '@jest/globals'; import { - validateTaskDependencies, - isCircularDependency, - removeDuplicateDependencies, - cleanupSubtaskDependencies, - ensureAtLeastOneIndependentSubtask, - validateAndFixDependencies, - canMoveWithDependencies -} from '../../scripts/modules/dependency-manager.js'; -import * as utils from '../../scripts/modules/utils.js'; -import { sampleTasks } from '../fixtures/sample-tasks.js'; + sampleTasks, + crossLevelDependencyTasks +} from '../fixtures/sample-tasks.js'; + +// Create mock functions that we can control in tests +const mockTaskExists = jest.fn(); +const mockFormatTaskId = jest.fn(); +const mockFindCycles = jest.fn(); +const mockLog = jest.fn(); +const mockReadJSON = jest.fn(); +const mockWriteJSON = jest.fn(); + +// Mock the utils module using the same pattern as move-task-cross-tag.test.js +jest.mock('../../scripts/modules/utils.js', () => ({ + log: mockLog, + readJSON: mockReadJSON, + writeJSON: mockWriteJSON, + taskExists: mockTaskExists, + formatTaskId: mockFormatTaskId, + findCycles: mockFindCycles, + traverseDependencies: jest.fn(() => []), + isSilentMode: jest.fn(() => true), + findProjectRoot: jest.fn(() => '/test'), + resolveEnvVariable: jest.fn(() => undefined), + isEmpty: jest.fn((v) => + v == null + ? true + : Array.isArray(v) + ? v.length === 0 + : typeof v === 'object' + ? Object.keys(v).length === 0 + : false + ), + // Common extras + enableSilentMode: jest.fn(), + disableSilentMode: jest.fn(), + getTaskManager: jest.fn(async () => ({})), + getTagAwareFilePath: jest.fn((basePath, _tag, projectRoot = '.') => basePath), + readComplexityReport: jest.fn(() => null) +})); -// Mock dependencies jest.mock('path'); jest.mock('chalk', () => ({ green: jest.fn((text) => `${text}`), @@ -27,22 +56,16 @@ jest.mock('chalk', () => ({ jest.mock('boxen', () => jest.fn((text) => `[boxed: ${text}]`)); -// Mock utils module -const mockTaskExists = jest.fn(); -const mockFormatTaskId = jest.fn(); -const mockFindCycles = jest.fn(); -const mockLog = jest.fn(); -const mockReadJSON = jest.fn(); -const mockWriteJSON = jest.fn(); - -jest.mock('../../scripts/modules/utils.js', () => ({ - log: mockLog, - readJSON: mockReadJSON, - writeJSON: mockWriteJSON, - taskExists: mockTaskExists, - formatTaskId: mockFormatTaskId, - findCycles: mockFindCycles -})); +// Now import SUT after mocks are in place +import { + validateTaskDependencies, + isCircularDependency, + removeDuplicateDependencies, + cleanupSubtaskDependencies, + ensureAtLeastOneIndependentSubtask, + validateAndFixDependencies, + canMoveWithDependencies +} from '../../scripts/modules/dependency-manager.js'; jest.mock('../../scripts/modules/ui.js', () => ({ displayBanner: jest.fn() @@ -52,8 +75,8 @@ jest.mock('../../scripts/modules/task-manager.js', () => ({ generateTaskFiles: jest.fn() })); -// Create a path for test files -const TEST_TASKS_PATH = 'tests/fixture/test-tasks.json'; +// Use a temporary path for test files - Jest will clean up the temp directory +const TEST_TASKS_PATH = '/tmp/jest-test-tasks.json'; describe('Dependency Manager Module', () => { beforeEach(() => { @@ -684,6 +707,8 @@ describe('Dependency Manager Module', () => { // IMPORTANT: Verify no calls to writeJSON with actual tasks.json expect(mockWriteJSON).not.toHaveBeenCalledWith( 'tasks/tasks.json', + expect.anything(), + expect.anything(), expect.anything() ); }); @@ -737,6 +762,8 @@ describe('Dependency Manager Module', () => { // IMPORTANT: Verify no calls to writeJSON with actual tasks.json expect(mockWriteJSON).not.toHaveBeenCalledWith( 'tasks/tasks.json', + expect.anything(), + expect.anything(), expect.anything() ); }); @@ -750,6 +777,8 @@ describe('Dependency Manager Module', () => { // IMPORTANT: Verify no calls to writeJSON with actual tasks.json expect(mockWriteJSON).not.toHaveBeenCalledWith( 'tasks/tasks.json', + expect.anything(), + expect.anything(), expect.anything() ); }); @@ -803,6 +832,8 @@ describe('Dependency Manager Module', () => { // IMPORTANT: Verify no calls to writeJSON with actual tasks.json expect(mockWriteJSON).not.toHaveBeenCalledWith( 'tasks/tasks.json', + expect.anything(), + expect.anything(), expect.anything() ); }); @@ -916,4 +947,297 @@ describe('Dependency Manager Module', () => { expect(result.conflicts).toEqual([]); }); }); + + describe('Cross-level dependency tests (Issue #542)', () => { + let originalExit; + + beforeEach(async () => { + // Ensure a fresh module instance so ESM mocks apply to dynamic imports + jest.resetModules(); + originalExit = process.exit; + process.exit = jest.fn(); + + // For ESM dynamic imports, use the same pattern + await jest.unstable_mockModule('../../scripts/modules/utils.js', () => ({ + log: mockLog, + readJSON: mockReadJSON, + writeJSON: mockWriteJSON, + taskExists: mockTaskExists, + formatTaskId: mockFormatTaskId, + findCycles: mockFindCycles, + traverseDependencies: jest.fn(() => []), + isSilentMode: jest.fn(() => true), + findProjectRoot: jest.fn(() => '/test'), + resolveEnvVariable: jest.fn(() => undefined), + isEmpty: jest.fn((v) => + v == null + ? true + : Array.isArray(v) + ? v.length === 0 + : typeof v === 'object' + ? Object.keys(v).length === 0 + : false + ), + enableSilentMode: jest.fn(), + disableSilentMode: jest.fn(), + getTaskManager: jest.fn(async () => ({})), + getTagAwareFilePath: jest.fn( + (basePath, _tag, projectRoot = '.') => basePath + ), + readComplexityReport: jest.fn(() => null) + })); + + // Also mock transitive imports to keep dependency surface minimal + await jest.unstable_mockModule('../../scripts/modules/ui.js', () => ({ + displayBanner: jest.fn() + })); + await jest.unstable_mockModule( + '../../scripts/modules/task-manager/generate-task-files.js', + () => ({ default: jest.fn() }) + ); + // Set up test data that matches the issue report + // Clone fixture data before each test to prevent mutation issues + mockReadJSON.mockImplementation(() => + structuredClone(crossLevelDependencyTasks) + ); + + // Configure mockTaskExists to properly validate cross-level dependencies + mockTaskExists.mockImplementation((tasks, taskId) => { + if (typeof taskId === 'string' && taskId.includes('.')) { + const [parentId, subtaskId] = taskId.split('.').map(Number); + const task = tasks.find((t) => t.id === parentId); + return ( + task && + task.subtasks && + task.subtasks.some((st) => st.id === subtaskId) + ); + } + + const numericId = + typeof taskId === 'string' ? parseInt(taskId, 10) : taskId; + return tasks.some((task) => task.id === numericId); + }); + + mockFormatTaskId.mockImplementation((id) => { + if (typeof id === 'string' && id.includes('.')) return id; // keep dot notation + return parseInt(id, 10); // normalize top-level task IDs to number + }); + }); + + afterEach(() => { + process.exit = originalExit; + }); + + test('should allow subtask to depend on top-level task', async () => { + const { addDependency } = await import( + '../../scripts/modules/dependency-manager.js' + ); + + // Test the specific scenario from Issue #542: subtask 2.2 depending on task 11 + await addDependency(TEST_TASKS_PATH, '2.2', 11, { projectRoot: '/test' }); + + // Verify we wrote to the test path (and not the real tasks.json) + expect(mockWriteJSON).toHaveBeenCalledWith( + TEST_TASKS_PATH, + expect.anything(), + '/test', + undefined + ); + expect(mockWriteJSON).not.toHaveBeenCalledWith( + 'tasks/tasks.json', + expect.anything(), + expect.anything(), + expect.anything() + ); + // Get the specific write call for TEST_TASKS_PATH + const writeCall = mockWriteJSON.mock.calls.find( + ([p]) => p === TEST_TASKS_PATH + ); + expect(writeCall).toBeDefined(); + const savedData = writeCall[1]; + const parent2 = savedData.tasks.find((t) => t.id === 2); + const subtask22 = parent2.subtasks.find((st) => st.id === 2); + + // Verify the dependency was actually added to subtask 2.2 + expect(subtask22.dependencies).toContain(11); + // Also verify a success log was emitted + const successCall = mockLog.mock.calls.find( + ([level]) => level === 'success' + ); + expect(successCall).toBeDefined(); + expect(successCall[1]).toContain('2.2'); + expect(successCall[1]).toContain('11'); + }); + + test('should allow top-level task to depend on subtask', async () => { + const { addDependency } = await import( + '../../scripts/modules/dependency-manager.js' + ); + + // Test reverse scenario: task 11 depending on subtask 2.1 + await addDependency(TEST_TASKS_PATH, 11, '2.1', { projectRoot: '/test' }); + + // Stronger assertions for writeJSON call and locating the correct task + expect(mockWriteJSON).toHaveBeenCalledWith( + TEST_TASKS_PATH, + expect.anything(), + '/test', + undefined + ); + expect(mockWriteJSON).not.toHaveBeenCalledWith( + 'tasks/tasks.json', + expect.anything(), + expect.anything(), + expect.anything() + ); + const writeCall = mockWriteJSON.mock.calls.find( + ([p]) => p === TEST_TASKS_PATH + ); + expect(writeCall).toBeDefined(); + const savedData = writeCall[1]; + const task11 = savedData.tasks.find((t) => t.id === 11); + + // Verify the dependency was actually added to task 11 + expect(task11.dependencies).toContain('2.1'); + // Verify a success log was emitted mentioning both task 11 and subtask 2.1 + const successCall = mockLog.mock.calls.find( + ([level]) => level === 'success' + ); + expect(successCall).toBeDefined(); + expect(successCall[1]).toContain('11'); + expect(successCall[1]).toContain('2.1'); + }); + + test('should properly validate cross-level dependencies exist', async () => { + // Test that validation correctly identifies when a cross-level dependency target doesn't exist + mockTaskExists.mockImplementation((tasks, taskId) => { + // Simulate task 99 not existing + if (taskId === '99' || taskId === 99) { + return false; + } + + if (typeof taskId === 'string' && taskId.includes('.')) { + const [parentId, subtaskId] = taskId.split('.').map(Number); + const task = tasks.find((t) => t.id === parentId); + return ( + task && + task.subtasks && + task.subtasks.some((st) => st.id === subtaskId) + ); + } + + const numericId = + typeof taskId === 'string' ? parseInt(taskId, 10) : taskId; + return tasks.some((task) => task.id === numericId); + }); + + const { addDependency } = await import( + '../../scripts/modules/dependency-manager.js' + ); + + const exitError = new Error('process.exit invoked'); + process.exit.mockImplementation(() => { + throw exitError; + }); + + await expect( + addDependency(TEST_TASKS_PATH, '2.2', 99, { projectRoot: '/test' }) + ).rejects.toBe(exitError); + + expect(process.exit).toHaveBeenCalledWith(1); + expect(mockWriteJSON).not.toHaveBeenCalled(); + // Verify that an error was reported to the user + expect(mockLog).toHaveBeenCalled(); + }); + + test('should remove top-level task dependency from a subtask', async () => { + const { addDependency, removeDependency } = await import( + '../../scripts/modules/dependency-manager.js' + ); + + // Start with cloned data and add 11 to 2.2 + await addDependency(TEST_TASKS_PATH, '2.2', 11, { projectRoot: '/test' }); + + // Get the saved data from the add operation + const addWriteCall = mockWriteJSON.mock.calls.find( + ([p]) => p === TEST_TASKS_PATH + ); + expect(addWriteCall).toBeDefined(); + const dataWithDep = addWriteCall[1]; + + // Verify the dependency was added + const subtask22AfterAdd = dataWithDep.tasks + .find((t) => t.id === 2) + .subtasks.find((st) => st.id === 2); + expect(subtask22AfterAdd.dependencies).toContain(11); + + // Clear mocks and re-setup mockReadJSON with the modified data + jest.clearAllMocks(); + mockReadJSON.mockImplementation(() => structuredClone(dataWithDep)); + + await removeDependency(TEST_TASKS_PATH, '2.2', 11, { + projectRoot: '/test' + }); + + const writeCall = mockWriteJSON.mock.calls.find( + ([p]) => p === TEST_TASKS_PATH + ); + expect(writeCall).toBeDefined(); + const saved = writeCall[1]; + const subtask22 = saved.tasks + .find((t) => t.id === 2) + .subtasks.find((st) => st.id === 2); + expect(subtask22.dependencies).not.toContain(11); + // Verify success log was emitted + const successCall = mockLog.mock.calls.find( + ([level]) => level === 'success' + ); + expect(successCall).toBeDefined(); + expect(successCall[1]).toContain('2.2'); + expect(successCall[1]).toContain('11'); + }); + + test('should remove subtask dependency from a top-level task', async () => { + const { addDependency, removeDependency } = await import( + '../../scripts/modules/dependency-manager.js' + ); + + // Add subtask dependency to task 11 + await addDependency(TEST_TASKS_PATH, 11, '2.1', { projectRoot: '/test' }); + + // Get the saved data from the add operation + const addWriteCall = mockWriteJSON.mock.calls.find( + ([p]) => p === TEST_TASKS_PATH + ); + expect(addWriteCall).toBeDefined(); + const dataWithDep = addWriteCall[1]; + + // Verify the dependency was added + const task11AfterAdd = dataWithDep.tasks.find((t) => t.id === 11); + expect(task11AfterAdd.dependencies).toContain('2.1'); + + // Clear mocks and re-setup mockReadJSON with the modified data + jest.clearAllMocks(); + mockReadJSON.mockImplementation(() => structuredClone(dataWithDep)); + + await removeDependency(TEST_TASKS_PATH, 11, '2.1', { + projectRoot: '/test' + }); + + const writeCall = mockWriteJSON.mock.calls.find( + ([p]) => p === TEST_TASKS_PATH + ); + expect(writeCall).toBeDefined(); + const saved = writeCall[1]; + const task11 = saved.tasks.find((t) => t.id === 11); + expect(task11.dependencies).not.toContain('2.1'); + // Verify success log was emitted + const successCall = mockLog.mock.calls.find( + ([level]) => level === 'success' + ); + expect(successCall).toBeDefined(); + expect(successCall[1]).toContain('11'); + expect(successCall[1]).toContain('2.1'); + }); + }); });