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

fix: Critical writeJSON Context Fixes - Prevent Tag Corruption (#910)

Browse Source 
* feat(tasks): Fix critical tag corruption bug in task management

- Fixed missing context parameters in writeJSON calls across add-task, remove-task, and add-subtask functions
- Added projectRoot and tag parameters to prevent data corruption in multi-tag environments
- Re-enabled generateTaskFiles calls to ensure markdown files are updated after operations
- Enhanced add_subtask MCP tool with tag parameter support
- Refactored addSubtaskDirect function to properly pass context to core logic
- Streamlined codebase by removing deprecated functionality

This resolves the critical bug where task operations in one tag context would corrupt or delete tasks from other tags in tasks.json.

* feat(task-manager): Enhance addSubtask with current tag support

- Added `getCurrentTag` utility to retrieve the current tag context for task operations.
- Updated `addSubtask` to use the current tag when reading and writing tasks, ensuring proper context handling.
- Refactored tests to accommodate changes in the `addSubtask` function, ensuring accurate mock implementations and expectations.
- Cleaned up test cases for better readability and maintainability.

This improves task management by preventing tag-related data corruption and enhances the overall functionality of the task manager.

* feat(remove-task): Add tag support for task removal and enhance error handling

- Introduced `tag` parameter in `removeTaskDirect` to specify context for task operations, improving multi-tag support.
- Updated logging to include tag context in messages for better traceability.
- Refactored task removal logic to streamline the process and improve error reporting.
- Added comprehensive unit tests to validate tag handling and ensure robust error management.

This enhancement prevents task data corruption across different tags and improves the overall reliability of the task management system.

* feat(add-task): Add projectRoot and tag parameters to addTask tests

- Updated `addTask` unit tests to include `projectRoot` and `tag` parameters for better context handling.
- Enhanced test cases to ensure accurate expectations and improve overall test coverage.

This change aligns with recent enhancements in task management, ensuring consistency across task operations.

* feat(set-task-status): Add tag parameter support and enhance task status handling

- Introduced `tag` parameter in `setTaskStatusDirect` and related functions to improve context management in multi-tag environments.
- Updated `writeJSON` calls to ensure task data integrity across different tags.
- Enhanced unit tests to validate tag preservation during task status updates, ensuring robust functionality.

This change aligns with recent improvements in task management, preventing data corruption and enhancing overall reliability.

* feat(tag-management): Enhance writeJSON calls to preserve tag context

- Updated `writeJSON` calls in `createTag`, `deleteTag`, `renameTag`, `copyTag`, and `enhanceTagsWithMetadata` to include `projectRoot` for better context management and to prevent tag corruption.
- Added comprehensive unit tests for tag management functions to ensure data integrity and proper tag handling during operations.

This change improves the reliability of tag management by ensuring that operations do not corrupt existing tags and maintains the overall structure of the task data.

* feat(expand-task): Update writeJSON to include projectRoot and tag context

- Modified `writeJSON` call in `expandTaskDirect` to pass `projectRoot` and `tag` parameters, ensuring proper context management when saving tasks.json.
- This change aligns with recent enhancements in task management, preventing potential data corruption and improving overall reliability.

* feat(fix-dependencies): Add projectRoot and tag parameters for enhanced context management

- Updated `fixDependenciesDirect` and `registerFixDependenciesTool` to include `projectRoot` and `tag` parameters, improving context handling during dependency fixes.
- Introduced a new unit test for `fixDependenciesCommand` to ensure proper preservation of projectRoot and tag data in JSON outputs.

This change enhances the reliability of dependency management by ensuring that context is maintained across operations, preventing potential data issues.

* fix(context): propagate projectRoot and tag through dependency, expansion, status-update and tag-management commands to prevent cross-tag data corruption

* test(fix-dependencies): Enhance unit tests for fixDependenciesCommand

- Refactored tests to use unstable mocks for utils, ui, and task-manager modules, improving isolation and reliability.
- Added checks for process.exit to ensure proper handling of invalid data scenarios.
- Updated test cases to verify writeJSON calls with projectRoot and tag parameters, ensuring accurate context preservation during dependency fixes.

This change strengthens the test suite for dependency management, ensuring robust functionality and preventing potential data issues.

* chore(plan): remove outdated fix plan for `writeJSON` context parameters
This commit is contained in:
Parthy
2025-07-02 21:45:10 +02:00
committed by GitHub
parent 43e0025f4c
commit 2852149a47
22 changed files with 1166 additions and 389 deletions
Download Patch File Download Diff File Expand all files Collapse all files

403
tests/unit/scripts/modules/task-manager/add-subtask.test.js
View File

@@ -2,308 +2,171 @@
* Tests for the addSubtask function
*/
import { jest } from '@jest/globals';
import path from 'path';
// Mock dependencies
const mockReadJSON = jest.fn();
const mockWriteJSON = jest.fn();
const mockGenerateTaskFiles = jest.fn();
const mockIsTaskDependentOn = jest.fn().mockReturnValue(false);
// Mock path module
jest.mock('path', () => ({
dirname: jest.fn()
}));
// Define test version of the addSubtask function
const testAddSubtask = (
tasksPath,
parentId,
existingTaskId,
newSubtaskData,
generateFiles = true
) => {
// Read the existing tasks
const data = mockReadJSON(tasksPath);
if (!data || !data.tasks) {
throw new Error(`Invalid or missing tasks file at ${tasksPath}`);
}
// Convert parent ID to number
const parentIdNum = parseInt(parentId, 10);
// Find the parent task
const parentTask = data.tasks.find((t) => t.id === parentIdNum);
if (!parentTask) {
throw new Error(`Parent task with ID ${parentIdNum} not found`);
}
// Initialize subtasks array if it doesn't exist
if (!parentTask.subtasks) {
parentTask.subtasks = [];
}
let newSubtask;
// Case 1: Convert an existing task to a subtask
if (existingTaskId !== null) {
const existingTaskIdNum = parseInt(existingTaskId, 10);
// Find the existing task
const existingTaskIndex = data.tasks.findIndex(
(t) => t.id === existingTaskIdNum
);
if (existingTaskIndex === -1) {
throw new Error(`Task with ID ${existingTaskIdNum} not found`);
}
const existingTask = data.tasks[existingTaskIndex];
// Check if task is already a subtask
if (existingTask.parentTaskId) {
throw new Error(
`Task ${existingTaskIdNum} is already a subtask of task ${existingTask.parentTaskId}`
);
}
// Check for circular dependency
if (existingTaskIdNum === parentIdNum) {
throw new Error(`Cannot make a task a subtask of itself`);
}
// Check for circular dependency using mockIsTaskDependentOn
if (mockIsTaskDependentOn()) {
throw new Error(
`Cannot create circular dependency: task ${parentIdNum} is already a subtask or dependent of task ${existingTaskIdNum}`
);
}
// Find the highest subtask ID to determine the next ID
const highestSubtaskId =
parentTask.subtasks.length > 0
? Math.max(...parentTask.subtasks.map((st) => st.id))
: 0;
const newSubtaskId = highestSubtaskId + 1;
// Clone the existing task to be converted to a subtask
newSubtask = {
...existingTask,
id: newSubtaskId,
parentTaskId: parentIdNum
};
// Add to parent's subtasks
parentTask.subtasks.push(newSubtask);
// Remove the task from the main tasks array
data.tasks.splice(existingTaskIndex, 1);
}
// Case 2: Create a new subtask
else if (newSubtaskData) {
// Find the highest subtask ID to determine the next ID
const highestSubtaskId =
parentTask.subtasks.length > 0
? Math.max(...parentTask.subtasks.map((st) => st.id))
: 0;
const newSubtaskId = highestSubtaskId + 1;
// Create the new subtask object
newSubtask = {
id: newSubtaskId,
title: newSubtaskData.title,
description: newSubtaskData.description || '',
details: newSubtaskData.details || '',
status: newSubtaskData.status || 'pending',
dependencies: newSubtaskData.dependencies || [],
parentTaskId: parentIdNum
};
// Add to parent's subtasks
parentTask.subtasks.push(newSubtask);
} else {
throw new Error('Either existingTaskId or newSubtaskData must be provided');
}
// Write the updated tasks back to the file
mockWriteJSON(tasksPath, data);
// Generate task files if requested
if (generateFiles) {
mockGenerateTaskFiles(tasksPath, path.dirname(tasksPath));
}
return newSubtask;
// Mock dependencies before importing the module
const mockUtils = {
readJSON: jest.fn(),
writeJSON: jest.fn(),
log: jest.fn(),
getCurrentTag: jest.fn()
};
const mockTaskManager = {
isTaskDependentOn: jest.fn()
};
const mockGenerateTaskFiles = jest.fn();
jest.unstable_mockModule(
'../../../../../scripts/modules/utils.js',
() => mockUtils
);
jest.unstable_mockModule(
'../../../../../scripts/modules/task-manager.js',
() => mockTaskManager
);
jest.unstable_mockModule(
'../../../../../scripts/modules/task-manager/generate-task-files.js',
() => ({
default: mockGenerateTaskFiles
})
);
const addSubtask = (
await import('../../../../../scripts/modules/task-manager/add-subtask.js')
).default;
describe('addSubtask function', () => {
// Reset mocks before each test
const multiTagData = {
master: {
tasks: [{ id: 1, title: 'Master Task', subtasks: [] }],
metadata: { description: 'Master tasks' }
},
'feature-branch': {
tasks: [{ id: 1, title: 'Feature Task', subtasks: [] }],
metadata: { description: 'Feature tasks' }
}
};
beforeEach(() => {
jest.clearAllMocks();
mockTaskManager.isTaskDependentOn.mockReturnValue(false);
});
// Default mock implementations
mockReadJSON.mockImplementation(() => ({
tasks: [
{
id: 1,
title: 'Parent Task',
description: 'This is a parent task',
status: 'pending',
dependencies: []
},
{
id: 2,
title: 'Existing Task',
description: 'This is an existing task',
status: 'pending',
dependencies: []
},
{
id: 3,
title: 'Another Task',
description: 'This is another task',
status: 'pending',
dependencies: [1]
}
]
}));
// Setup success write response
mockWriteJSON.mockImplementation((path, data) => {
return data;
test('should add a new subtask and preserve other tags', async () => {
const context = { projectRoot: '/fake/root', tag: 'feature-branch' };
const newSubtaskData = { title: 'My New Subtask' };
mockUtils.readJSON.mockReturnValueOnce({
tasks: [{ id: 1, title: 'Feature Task', subtasks: [] }],
metadata: { description: 'Feature tasks' }
});
// Set up default behavior for dependency check
mockIsTaskDependentOn.mockReturnValue(false);
await addSubtask('tasks.json', '1', null, newSubtaskData, true, context);
expect(mockUtils.writeJSON).toHaveBeenCalledWith(
'tasks.json',
expect.any(Object),
'/fake/root',
'feature-branch'
);
const writtenData = mockUtils.writeJSON.mock.calls[0][1];
const parentTask = writtenData.tasks.find((t) => t.id === 1);
expect(parentTask.subtasks).toHaveLength(1);
expect(parentTask.subtasks[0].title).toBe('My New Subtask');
});
test('should add a new subtask to a parent task', async () => {
// Create new subtask data
const newSubtaskData = {
title: 'New Subtask',
description: 'This is a new subtask',
details: 'Implementation details for the subtask',
status: 'pending',
dependencies: []
};
// Execute the test version of addSubtask
const newSubtask = testAddSubtask(
'tasks/tasks.json',
1,
mockUtils.readJSON.mockReturnValueOnce({
tasks: [{ id: 1, title: 'Parent Task', subtasks: [] }]
});
const context = {};
const newSubtask = await addSubtask(
'tasks.json',
'1',
null,
newSubtaskData,
true
{ title: 'New Subtask' },
true,
context
);
// Verify readJSON was called with the correct path
expect(mockReadJSON).toHaveBeenCalledWith('tasks/tasks.json');
// Verify writeJSON was called with the correct path
expect(mockWriteJSON).toHaveBeenCalledWith(
'tasks/tasks.json',
expect.any(Object)
);
// Verify the subtask was created with correct data
expect(newSubtask).toBeDefined();
expect(newSubtask.id).toBe(1);
expect(newSubtask.title).toBe('New Subtask');
expect(newSubtask.parentTaskId).toBe(1);
// Verify generateTaskFiles was called
expect(mockUtils.writeJSON).toHaveBeenCalled();
const writeCallArgs = mockUtils.writeJSON.mock.calls[0][1]; // data is the second arg now
const parentTask = writeCallArgs.tasks.find((t) => t.id === 1);
expect(parentTask.subtasks).toHaveLength(1);
expect(parentTask.subtasks[0].title).toBe('New Subtask');
expect(mockGenerateTaskFiles).toHaveBeenCalled();
});
test('should convert an existing task to a subtask', async () => {
// Execute the test version of addSubtask to convert task 2 to a subtask of task 1
const convertedSubtask = testAddSubtask(
'tasks/tasks.json',
1,
2,
mockUtils.readJSON.mockReturnValueOnce({
tasks: [
{ id: 1, title: 'Parent Task', subtasks: [] },
{ id: 2, title: 'Existing Task 2', subtasks: [] }
]
});
const context = {};
const convertedSubtask = await addSubtask(
'tasks.json',
'1',
'2',
null,
true
true,
context
);
// Verify readJSON was called with the correct path
expect(mockReadJSON).toHaveBeenCalledWith('tasks/tasks.json');
// Verify writeJSON was called
expect(mockWriteJSON).toHaveBeenCalled();
// Verify the subtask was created with correct data
expect(convertedSubtask).toBeDefined();
expect(convertedSubtask.id).toBe(1);
expect(convertedSubtask.title).toBe('Existing Task');
expect(convertedSubtask.parentTaskId).toBe(1);
// Verify generateTaskFiles was called
expect(mockGenerateTaskFiles).toHaveBeenCalled();
expect(convertedSubtask.title).toBe('Existing Task 2');
expect(mockUtils.writeJSON).toHaveBeenCalled();
const writeCallArgs = mockUtils.writeJSON.mock.calls[0][1];
const parentTask = writeCallArgs.tasks.find((t) => t.id === 1);
expect(parentTask.subtasks).toHaveLength(1);
expect(parentTask.subtasks[0].title).toBe('Existing Task 2');
});
test('should throw an error if parent task does not exist', async () => {
// Create new subtask data
const newSubtaskData = {
title: 'New Subtask',
description: 'This is a new subtask'
};
mockUtils.readJSON.mockReturnValueOnce({
tasks: [{ id: 1, title: 'Task 1', subtasks: [] }]
});
const context = {};
await expect(
addSubtask(
'tasks.json',
'99',
null,
{ title: 'New Subtask' },
true,
context
)
).rejects.toThrow('Parent task with ID 99 not found');
});
// Override mockReadJSON for this specific test case
mockReadJSON.mockImplementationOnce(() => ({
test('should throw an error if trying to convert a non-existent task', async () => {
mockUtils.readJSON.mockReturnValueOnce({
tasks: [{ id: 1, title: 'Parent Task', subtasks: [] }]
});
const context = {};
await expect(
addSubtask('tasks.json', '1', '99', null, true, context)
).rejects.toThrow('Task with ID 99 not found');
});
test('should throw an error for circular dependency', async () => {
mockUtils.readJSON.mockReturnValueOnce({
tasks: [
{
id: 1,
title: 'Task 1',
status: 'pending'
}
{ id: 1, title: 'Parent Task', subtasks: [] },
{ id: 2, title: 'Child Task', subtasks: [] }
]
}));
// Expect an error when trying to add a subtask to a non-existent parent
expect(() =>
testAddSubtask('tasks/tasks.json', 999, null, newSubtaskData)
).toThrow(/Parent task with ID 999 not found/);
// Verify writeJSON was not called
expect(mockWriteJSON).not.toHaveBeenCalled();
});
test('should throw an error if existing task does not exist', async () => {
// Expect an error when trying to convert a non-existent task
expect(() => testAddSubtask('tasks/tasks.json', 1, 999, null)).toThrow(
/Task with ID 999 not found/
});
mockTaskManager.isTaskDependentOn.mockImplementation(
(tasks, parentTask, existingTaskIdNum) => {
return parentTask.id === 1 && existingTaskIdNum === 2;
}
);
// Verify writeJSON was not called
expect(mockWriteJSON).not.toHaveBeenCalled();
});
test('should throw an error if trying to create a circular dependency', async () => {
// Force the isTaskDependentOn mock to return true for this test only
mockIsTaskDependentOn.mockReturnValueOnce(true);
// Expect an error when trying to create a circular dependency
expect(() => testAddSubtask('tasks/tasks.json', 3, 1, null)).toThrow(
/circular dependency/
const context = {};
await expect(
addSubtask('tasks.json', '1', '2', null, true, context)
).rejects.toThrow(
'Cannot create circular dependency: task 1 is already a subtask or dependent of task 2'
);
// Verify writeJSON was not called
expect(mockWriteJSON).not.toHaveBeenCalled();
});
test('should not regenerate task files if generateFiles is false', async () => {
// Create new subtask data
const newSubtaskData = {
title: 'New Subtask',
description: 'This is a new subtask'
};
// Execute the test version of addSubtask with generateFiles = false
testAddSubtask('tasks/tasks.json', 1, null, newSubtaskData, false);
// Verify writeJSON was called
expect(mockWriteJSON).toHaveBeenCalled();
// Verify task files were not regenerated
expect(mockGenerateTaskFiles).not.toHaveBeenCalled();
});
});