diff --git a/scripts/modules/commands.js b/scripts/modules/commands.js index 3bb00cda..4bb020c3 100644 --- a/scripts/modules/commands.js +++ b/scripts/modules/commands.js @@ -471,7 +471,12 @@ function registerCommands(programInstance) { process.exit(1); } - await addDependency(tasksPath, parseInt(taskId, 10), parseInt(dependencyId, 10)); + // Handle subtask IDs correctly by preserving the string format for IDs containing dots + // Only use parseInt for simple numeric IDs + const formattedTaskId = taskId.includes('.') ? taskId : parseInt(taskId, 10); + const formattedDependencyId = dependencyId.includes('.') ? dependencyId : parseInt(dependencyId, 10); + + await addDependency(tasksPath, formattedTaskId, formattedDependencyId); }); // remove-dependency command @@ -491,7 +496,12 @@ function registerCommands(programInstance) { process.exit(1); } - await removeDependency(tasksPath, parseInt(taskId, 10), parseInt(dependencyId, 10)); + // Handle subtask IDs correctly by preserving the string format for IDs containing dots + // Only use parseInt for simple numeric IDs + const formattedTaskId = taskId.includes('.') ? taskId : parseInt(taskId, 10); + const formattedDependencyId = dependencyId.includes('.') ? dependencyId : parseInt(dependencyId, 10); + + await removeDependency(tasksPath, formattedTaskId, formattedDependencyId); }); // validate-dependencies command diff --git a/scripts/modules/dependency-manager.js b/scripts/modules/dependency-manager.js index b01ace11..dc86fac9 100644 --- a/scripts/modules/dependency-manager.js +++ b/scripts/modules/dependency-manager.js @@ -104,12 +104,34 @@ async function addDependency(tasksPath, taskId, dependencyId) { return; } - // Check if the task is trying to depend on itself + // Check if the task is trying to depend on itself - compare full IDs (including subtask parts) if (String(formattedTaskId) === String(formattedDependencyId)) { log('error', `Task ${formattedTaskId} cannot depend on itself.`); process.exit(1); } + // For subtasks of the same parent, we need to make sure we're not treating it as a self-dependency + // Check if we're dealing with subtasks with the same parent task + let isSelfDependency = false; + + if (typeof formattedTaskId === 'string' && typeof formattedDependencyId === 'string' && + formattedTaskId.includes('.') && formattedDependencyId.includes('.')) { + const [taskParentId] = formattedTaskId.split('.'); + const [depParentId] = formattedDependencyId.split('.'); + + // Only treat it as a self-dependency if both the parent ID and subtask ID are identical + isSelfDependency = formattedTaskId === formattedDependencyId; + + // Log for debugging + log('debug', `Adding dependency between subtasks: ${formattedTaskId} depends on ${formattedDependencyId}`); + log('debug', `Parent IDs: ${taskParentId} and ${depParentId}, Self-dependency check: ${isSelfDependency}`); + } + + if (isSelfDependency) { + log('error', `Subtask ${formattedTaskId} cannot depend on itself.`); + process.exit(1); + } + // Check for circular dependencies let dependencyChain = [formattedTaskId]; if (!isCircularDependency(data.tasks, formattedDependencyId, dependencyChain)) { @@ -276,8 +298,22 @@ async function addDependency(tasksPath, taskId, dependencyId) { return true; } - // Find the task - const task = tasks.find(t => String(t.id) === taskIdStr); + // Find the task or subtask + let task = null; + + // Check if this is a subtask reference (e.g., "1.2") + if (taskIdStr.includes('.')) { + const [parentId, subtaskId] = taskIdStr.split('.').map(Number); + const parentTask = tasks.find(t => t.id === parentId); + + if (parentTask && parentTask.subtasks) { + task = parentTask.subtasks.find(st => st.id === subtaskId); + } + } else { + // Regular task + task = tasks.find(t => String(t.id) === taskIdStr); + } + if (!task) { return false; // Task doesn't exist, can't create circular dependency } @@ -336,6 +372,50 @@ async function addDependency(tasksPath, taskId, dependencyId) { message: `Task ${task.id} is part of a circular dependency chain` }); } + + // Check subtask dependencies if they exist + if (task.subtasks && task.subtasks.length > 0) { + task.subtasks.forEach(subtask => { + if (!subtask.dependencies) { + return; // No dependencies to validate + } + + // Create a full subtask ID for reference + const fullSubtaskId = `${task.id}.${subtask.id}`; + + subtask.dependencies.forEach(depId => { + // Check for self-dependencies in subtasks + if (String(depId) === String(fullSubtaskId) || + (typeof depId === 'number' && depId === subtask.id)) { + issues.push({ + type: 'self', + taskId: fullSubtaskId, + message: `Subtask ${fullSubtaskId} depends on itself` + }); + return; + } + + // Check if dependency exists + if (!taskExists(tasks, depId)) { + issues.push({ + type: 'missing', + taskId: fullSubtaskId, + dependencyId: depId, + message: `Subtask ${fullSubtaskId} depends on non-existent task/subtask ${depId}` + }); + } + }); + + // Check for circular dependencies in subtasks + if (isCircularDependency(tasks, fullSubtaskId)) { + issues.push({ + type: 'circular', + taskId: fullSubtaskId, + message: `Subtask ${fullSubtaskId} is part of a circular dependency chain` + }); + } + }); + } }); return { diff --git a/tasks/task_023.txt b/tasks/task_023.txt index 53a83793..eb4ad391 100644 --- a/tasks/task_023.txt +++ b/tasks/task_023.txt @@ -223,7 +223,7 @@ Testing approach: - Benchmark performance improvements from SDK integration. ## 8. Implement Direct Function Imports and Replace CLI-based Execution [in-progress] -### Dependencies: None +### Dependencies: 23.13 ### Description: Refactor the MCP server implementation to use direct Task Master function imports instead of the current CLI-based execution using child_process.spawnSync. This will improve performance, reliability, and enable better error handling. ### Details: 1. Create a new module to import and expose Task Master core functions directly @@ -247,7 +247,7 @@ Testing approach: 8. Create unit tests for context management and caching functionality ## 10. Enhance Tool Registration and Resource Management [in-progress] -### Dependencies: 23.1 +### Dependencies: 23.1, 23.8 ### Description: Refactor tool registration to follow FastMCP best practices, using decorators and improving the overall structure. Implement proper resource management for task templates and other shared resources. ### Details: 1. Update registerTaskMasterTools function to use FastMCP's decorator pattern @@ -272,7 +272,7 @@ Testing approach: 1. Design structured log format for consistent parsing\n2. Implement different log levels (debug, info, warn, error)\n3. Add request/response logging middleware\n4. Implement correlation IDs for request tracking\n5. Add performance metrics logging\n6. Configure log output destinations (console, file)\n7. Document logging patterns and usage ## 13. Create Testing Framework and Test Suite [pending] -### Dependencies: 23.1, 23.3, 23.8 +### Dependencies: 23.1, 23.3 ### Description: Implement a comprehensive testing framework for the MCP server, including unit tests, integration tests, and end-to-end tests. ### Details: 1. Set up Jest testing framework with proper configuration\n2. Create MCPTestClient for testing FastMCP server interaction\n3. Implement unit tests for individual tool functions\n4. Create integration tests for end-to-end request/response cycles\n5. Set up test fixtures and mock data\n6. Implement test coverage reporting\n7. Document testing guidelines and examples diff --git a/tasks/tasks.json b/tasks/tasks.json index 9f5eb1a0..e404d3b9 100644 --- a/tasks/tasks.json +++ b/tasks/tasks.json @@ -1396,7 +1396,9 @@ "id": 8, "title": "Implement Direct Function Imports and Replace CLI-based Execution", "description": "Refactor the MCP server implementation to use direct Task Master function imports instead of the current CLI-based execution using child_process.spawnSync. This will improve performance, reliability, and enable better error handling.", - "dependencies": [], + "dependencies": [ + "23.13" + ], "details": "1. Create a new module to import and expose Task Master core functions directly\n2. Modify tools/utils.js to remove executeTaskMasterCommand and replace with direct function calls\n3. Update each tool implementation (listTasks.js, showTask.js, etc.) to use the direct function imports\n4. Implement proper error handling with try/catch blocks and FastMCP's MCPError\n5. Add unit tests to verify the function imports work correctly\n6. Test performance improvements by comparing response times between CLI and function import approaches", "status": "in-progress", "parentTaskId": 23 @@ -1417,7 +1419,8 @@ "title": "Enhance Tool Registration and Resource Management", "description": "Refactor tool registration to follow FastMCP best practices, using decorators and improving the overall structure. Implement proper resource management for task templates and other shared resources.", "dependencies": [ - 1 + 1, + "23.8" ], "details": "1. Update registerTaskMasterTools function to use FastMCP's decorator pattern\n2. Implement @mcp.tool() decorators for all existing tools\n3. Add proper type annotations and documentation for all tools\n4. Create resource handlers for task templates using @mcp.resource()\n5. Implement resource templates for common task patterns\n6. Update the server initialization to properly register all tools and resources\n7. Add validation for tool inputs using FastMCP's built-in validation\n8. Create comprehensive tests for tool registration and resource access", "status": "in-progress", @@ -1455,8 +1458,7 @@ "status": "pending", "dependencies": [ "23.1", - "23.3", - "23.8" + "23.3" ], "parentTaskId": 23 }, diff --git a/tests/unit/dependency-manager.test.js b/tests/unit/dependency-manager.test.js index 27ebd881..cb38f592 100644 --- a/tests/unit/dependency-manager.test.js +++ b/tests/unit/dependency-manager.test.js @@ -175,6 +175,81 @@ describe('Dependency Manager Module', () => { const result = isCircularDependency(tasks, 1); expect(result).toBe(true); }); + + test('should handle subtask dependencies correctly', () => { + const tasks = [ + { + id: 1, + dependencies: [], + subtasks: [ + { id: 1, dependencies: ["1.2"] }, + { id: 2, dependencies: ["1.3"] }, + { id: 3, dependencies: ["1.1"] } + ] + } + ]; + + // This creates a circular dependency: 1.1 -> 1.2 -> 1.3 -> 1.1 + const result = isCircularDependency(tasks, "1.1", ["1.3", "1.2"]); + expect(result).toBe(true); + }); + + test('should allow non-circular subtask dependencies within same parent', () => { + const tasks = [ + { + id: 1, + dependencies: [], + subtasks: [ + { id: 1, dependencies: [] }, + { id: 2, dependencies: ["1.1"] }, + { id: 3, dependencies: ["1.2"] } + ] + } + ]; + + // This is a valid dependency chain: 1.3 -> 1.2 -> 1.1 + const result = isCircularDependency(tasks, "1.1", []); + expect(result).toBe(false); + }); + + test('should properly handle dependencies between subtasks of the same parent', () => { + const tasks = [ + { + id: 1, + dependencies: [], + subtasks: [ + { id: 1, dependencies: [] }, + { id: 2, dependencies: ["1.1"] }, + { id: 3, dependencies: [] } + ] + } + ]; + + // Check if adding a dependency from subtask 1.3 to 1.2 creates a circular dependency + // This should be false as 1.3 -> 1.2 -> 1.1 is a valid chain + mockTaskExists.mockImplementation(() => true); + const result = isCircularDependency(tasks, "1.3", ["1.2"]); + expect(result).toBe(false); + }); + + test('should correctly detect circular dependencies in subtasks of the same parent', () => { + const tasks = [ + { + id: 1, + dependencies: [], + subtasks: [ + { id: 1, dependencies: ["1.3"] }, + { id: 2, dependencies: ["1.1"] }, + { id: 3, dependencies: ["1.2"] } + ] + } + ]; + + // This creates a circular dependency: 1.1 -> 1.3 -> 1.2 -> 1.1 + mockTaskExists.mockImplementation(() => true); + const result = isCircularDependency(tasks, "1.2", ["1.1"]); + expect(result).toBe(true); + }); }); describe('validateTaskDependencies function', () => { @@ -242,6 +317,128 @@ describe('Dependency Manager Module', () => { // Should be valid since a missing dependencies property is interpreted as an empty array expect(result.valid).toBe(true); }); + + test('should handle subtask dependencies correctly', () => { + const tasks = [ + { + id: 1, + dependencies: [], + subtasks: [ + { id: 1, dependencies: [] }, + { id: 2, dependencies: ["1.1"] }, // Valid - depends on another subtask + { id: 3, dependencies: ["1.2"] } // Valid - depends on another subtask + ] + }, + { + id: 2, + dependencies: ["1.3"], // Valid - depends on a subtask from task 1 + subtasks: [] + } + ]; + + // Set up mock to handle subtask validation + mockTaskExists.mockImplementation((tasks, id) => { + if (typeof id === 'string' && id.includes('.')) { + const [taskId, subtaskId] = id.split('.').map(Number); + const task = tasks.find(t => t.id === taskId); + return task && task.subtasks && task.subtasks.some(st => st.id === subtaskId); + } + return tasks.some(task => task.id === parseInt(id, 10)); + }); + + const result = validateTaskDependencies(tasks); + + expect(result.valid).toBe(true); + expect(result.issues.length).toBe(0); + }); + + test('should detect missing subtask dependencies', () => { + const tasks = [ + { + id: 1, + dependencies: [], + subtasks: [ + { id: 1, dependencies: ["1.4"] }, // Invalid - subtask 4 doesn't exist + { id: 2, dependencies: ["2.1"] } // Invalid - task 2 has no subtasks + ] + }, + { + id: 2, + dependencies: [], + subtasks: [] + } + ]; + + // Mock taskExists to correctly identify missing subtasks + mockTaskExists.mockImplementation((taskArray, depId) => { + if (typeof depId === 'string' && depId === "1.4") { + return false; // Subtask 1.4 doesn't exist + } + if (typeof depId === 'string' && depId === "2.1") { + return false; // Subtask 2.1 doesn't exist + } + return true; // All other dependencies exist + }); + + const result = validateTaskDependencies(tasks); + + expect(result.valid).toBe(false); + expect(result.issues.length).toBeGreaterThan(0); + // Should detect missing subtask dependencies + expect(result.issues.some(issue => + issue.type === 'missing' && String(issue.taskId) === "1.1" && String(issue.dependencyId) === "1.4" + )).toBe(true); + }); + + test('should detect circular dependencies between subtasks', () => { + const tasks = [ + { + id: 1, + dependencies: [], + subtasks: [ + { id: 1, dependencies: ["1.2"] }, + { id: 2, dependencies: ["1.1"] } // Creates a circular dependency with 1.1 + ] + } + ]; + + // Mock isCircularDependency for subtasks + mockFindCycles.mockReturnValue(true); + + const result = validateTaskDependencies(tasks); + + expect(result.valid).toBe(false); + expect(result.issues.some(issue => issue.type === 'circular')).toBe(true); + }); + + test('should properly validate dependencies between subtasks of the same parent', () => { + const tasks = [ + { + id: 23, + dependencies: [], + subtasks: [ + { id: 8, dependencies: ["23.13"] }, + { id: 10, dependencies: ["23.8"] }, + { id: 13, dependencies: [] } + ] + } + ]; + + // Mock taskExists to validate the subtask dependencies + mockTaskExists.mockImplementation((taskArray, id) => { + if (typeof id === 'string') { + if (id === "23.8" || id === "23.10" || id === "23.13") { + return true; + } + } + return false; + }); + + const result = validateTaskDependencies(tasks); + + expect(result.valid).toBe(true); + expect(result.issues.length).toBe(0); + }); }); describe('removeDuplicateDependencies function', () => {