ros/claude-task-master
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: Correct handling of dependencies between subtasks

Browse Source 
This commit fixes an issue with the dependency management system where
dependencies between subtasks of the same parent task were not being
handled correctly. Previously, when trying to add a dependency between
subtasks (e.g., making 23.8 depend on 23.13), the system incorrectly
interpreted it as a circular dependency of the parent task depending
on itself.

Changes:
- Modified add-dependency and remove-dependency commands to preserve
  string format for subtask IDs containing dots instead of converting
  them to integers
- Updated dependency detection logic to properly handle string-based
  subtask IDs
- Added specific tests verifying both successful dependency chains
  and circular dependency detection between subtasks of the same parent

The fix ensures proper task ordering for development workflows where
related subtasks must be completed in sequence (like establishing tests
before implementing a feature). It maintains backward compatibility with
existing task structures while correctly enforcing dependency chains
within subtask groups.

Tests:
- Added tests for valid dependency chains between subtasks of the same parent
- Added tests for circular dependency detection in subtask relationships
- Added specific test for the 23.8->23.13->23.10 subtask dependency chain

Resolves the issue where the suggested task development workflow couldn't
be properly enforced through the dependency management system.
This commit is contained in:
Eyal Toledano
2025-03-29 18:19:39 -04:00
parent 57f655affd
commit e70f44b6fb
5 changed files with 301 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
scripts/modules/commands.js
View File

@@ -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

86
scripts/modules/dependency-manager.js
View File

@@ -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 {

6
tasks/task_023.txt
View File

@@ -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

10
tasks/tasks.json
View File

@@ -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
},

197
tests/unit/dependency-manager.test.js
View File

@@ -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', () => {