From 78a53767961a98338d1949259e997788b5627ebe Mon Sep 17 00:00:00 2001 From: Eyal Toledano Date: Tue, 22 Apr 2025 16:09:33 -0400 Subject: [PATCH] fix(mcp): prevents the mcp from failing due to the newly introduced ConfigurationError object thrown if .taskmasterconfig is not present. I'll need to implement MCP tools for model to manage models from MCP and be able to create it. --- mcp-server/src/core/utils/async-manager.js | 251 --------------------- mcp-server/src/index.js | 7 - mcp-server/src/tools/index.js | 6 +- scripts/modules/config-manager.js | 25 +- scripts/modules/task-manager/list-tasks.js | 1 + tasks/task_061.txt | 27 +++ tasks/tasks.json | 3 +- test-config-manager.js | 48 ++++ 8 files changed, 102 insertions(+), 266 deletions(-) delete mode 100644 mcp-server/src/core/utils/async-manager.js create mode 100644 test-config-manager.js diff --git a/mcp-server/src/core/utils/async-manager.js b/mcp-server/src/core/utils/async-manager.js deleted file mode 100644 index cf75c8b4..00000000 --- a/mcp-server/src/core/utils/async-manager.js +++ /dev/null @@ -1,251 +0,0 @@ -import { v4 as uuidv4 } from 'uuid'; - -class AsyncOperationManager { - constructor() { - this.operations = new Map(); // Stores active operation state - this.completedOperations = new Map(); // Stores completed operations - this.maxCompletedOperations = 100; // Maximum number of completed operations to store - this.listeners = new Map(); // For potential future notifications - } - - /** - * Adds an operation to be executed asynchronously. - * @param {Function} operationFn - The async function to execute (e.g., a Direct function). - * @param {Object} args - Arguments to pass to the operationFn. - * @param {Object} context - The MCP tool context { log, reportProgress, session }. - * @returns {string} The unique ID assigned to this operation. - */ - addOperation(operationFn, args, context) { - const operationId = `op-${uuidv4()}`; - const operation = { - id: operationId, - status: 'pending', - startTime: Date.now(), - endTime: null, - result: null, - error: null, - // Store necessary parts of context, especially log for background execution - log: context.log, - reportProgress: context.reportProgress, // Pass reportProgress through - session: context.session // Pass session through if needed by the operationFn - }; - this.operations.set(operationId, operation); - this.log(operationId, 'info', `Operation added.`); - - // Start execution in the background (don't await here) - this._runOperation(operationId, operationFn, args, context).catch((err) => { - // Catch unexpected errors during the async execution setup itself - this.log( - operationId, - 'error', - `Critical error starting operation: ${err.message}`, - { stack: err.stack } - ); - operation.status = 'failed'; - operation.error = { - code: 'MANAGER_EXECUTION_ERROR', - message: err.message - }; - operation.endTime = Date.now(); - - // Move to completed operations - this._moveToCompleted(operationId); - }); - - return operationId; - } - - /** - * Internal function to execute the operation. - * @param {string} operationId - The ID of the operation. - * @param {Function} operationFn - The async function to execute. - * @param {Object} args - Arguments for the function. - * @param {Object} context - The original MCP tool context. - */ - async _runOperation(operationId, operationFn, args, context) { - const operation = this.operations.get(operationId); - if (!operation) return; // Should not happen - - operation.status = 'running'; - this.log(operationId, 'info', `Operation running.`); - this.emit('statusChanged', { operationId, status: 'running' }); - - try { - // Pass the necessary context parts to the direct function - // The direct function needs to be adapted if it needs reportProgress - // We pass the original context's log, plus our wrapped reportProgress - const result = await operationFn(args, operation.log, { - reportProgress: (progress) => - this._handleProgress(operationId, progress), - mcpLog: operation.log, // Pass log as mcpLog if direct fn expects it - session: operation.session - }); - - operation.status = result.success ? 'completed' : 'failed'; - operation.result = result.success ? result.data : null; - operation.error = result.success ? null : result.error; - this.log( - operationId, - 'info', - `Operation finished with status: ${operation.status}` - ); - } catch (error) { - this.log( - operationId, - 'error', - `Operation failed with error: ${error.message}`, - { stack: error.stack } - ); - operation.status = 'failed'; - operation.error = { - code: 'OPERATION_EXECUTION_ERROR', - message: error.message - }; - } finally { - operation.endTime = Date.now(); - this.emit('statusChanged', { - operationId, - status: operation.status, - result: operation.result, - error: operation.error - }); - - // Move to completed operations if done or failed - if (operation.status === 'completed' || operation.status === 'failed') { - this._moveToCompleted(operationId); - } - } - } - - /** - * Move an operation from active operations to completed operations history. - * @param {string} operationId - The ID of the operation to move. - * @private - */ - _moveToCompleted(operationId) { - const operation = this.operations.get(operationId); - if (!operation) return; - - // Store only the necessary data in completed operations - const completedData = { - id: operation.id, - status: operation.status, - startTime: operation.startTime, - endTime: operation.endTime, - result: operation.result, - error: operation.error - }; - - this.completedOperations.set(operationId, completedData); - this.operations.delete(operationId); - - // Trim completed operations if exceeding maximum - if (this.completedOperations.size > this.maxCompletedOperations) { - // Get the oldest operation (sorted by endTime) - const oldest = [...this.completedOperations.entries()].sort( - (a, b) => a[1].endTime - b[1].endTime - )[0]; - - if (oldest) { - this.completedOperations.delete(oldest[0]); - } - } - } - - /** - * Handles progress updates from the running operation and forwards them. - * @param {string} operationId - The ID of the operation reporting progress. - * @param {Object} progress - The progress object { progress, total? }. - */ - _handleProgress(operationId, progress) { - const operation = this.operations.get(operationId); - if (operation && operation.reportProgress) { - try { - // Use the reportProgress function captured from the original context - operation.reportProgress(progress); - this.log( - operationId, - 'debug', - `Reported progress: ${JSON.stringify(progress)}` - ); - } catch (err) { - this.log( - operationId, - 'warn', - `Failed to report progress: ${err.message}` - ); - // Don't stop the operation, just log the reporting failure - } - } - } - - /** - * Retrieves the status and result/error of an operation. - * @param {string} operationId - The ID of the operation. - * @returns {Object | null} The operation details or null if not found. - */ - getStatus(operationId) { - // First check active operations - const operation = this.operations.get(operationId); - if (operation) { - return { - id: operation.id, - status: operation.status, - startTime: operation.startTime, - endTime: operation.endTime, - result: operation.result, - error: operation.error - }; - } - - // Then check completed operations - const completedOperation = this.completedOperations.get(operationId); - if (completedOperation) { - return completedOperation; - } - - // Operation not found in either active or completed - return { - error: { - code: 'OPERATION_NOT_FOUND', - message: `Operation ID ${operationId} not found. It may have been completed and removed from history, or the ID may be invalid.` - }, - status: 'not_found' - }; - } - - /** - * Internal logging helper to prefix logs with the operation ID. - * @param {string} operationId - The ID of the operation. - * @param {'info'|'warn'|'error'|'debug'} level - Log level. - * @param {string} message - Log message. - * @param {Object} [meta] - Additional metadata. - */ - log(operationId, level, message, meta = {}) { - const operation = this.operations.get(operationId); - // Use the logger instance associated with the operation if available, otherwise console - const logger = operation?.log || console; - const logFn = logger[level] || logger.log || console.log; // Fallback - logFn(`[AsyncOp ${operationId}] ${message}`, meta); - } - - // --- Basic Event Emitter --- - on(eventName, listener) { - if (!this.listeners.has(eventName)) { - this.listeners.set(eventName, []); - } - this.listeners.get(eventName).push(listener); - } - - emit(eventName, data) { - if (this.listeners.has(eventName)) { - this.listeners.get(eventName).forEach((listener) => listener(data)); - } - } -} - -// Export a singleton instance -const asyncOperationManager = new AsyncOperationManager(); - -// Export the manager and potentially the class if needed elsewhere -export { asyncOperationManager, AsyncOperationManager }; diff --git a/mcp-server/src/index.js b/mcp-server/src/index.js index a3fe5bd0..2ea14842 100644 --- a/mcp-server/src/index.js +++ b/mcp-server/src/index.js @@ -5,7 +5,6 @@ import { fileURLToPath } from 'url'; import fs from 'fs'; import logger from './logger.js'; import { registerTaskMasterTools } from './tools/index.js'; -import { asyncOperationManager } from './core/utils/async-manager.js'; // Load environment variables dotenv.config(); @@ -35,9 +34,6 @@ class TaskMasterMCPServer { this.server.addResourceTemplate({}); - // Make the manager accessible (e.g., pass it to tool registration) - this.asyncManager = asyncOperationManager; - // Bind methods this.init = this.init.bind(this); this.start = this.start.bind(this); @@ -88,7 +84,4 @@ class TaskMasterMCPServer { } } -// Export the manager from here as well, if needed elsewhere -export { asyncOperationManager }; - export default TaskMasterMCPServer; diff --git a/mcp-server/src/tools/index.js b/mcp-server/src/tools/index.js index 0ed3f22f..2fe97cb6 100644 --- a/mcp-server/src/tools/index.js +++ b/mcp-server/src/tools/index.js @@ -27,14 +27,12 @@ import { registerComplexityReportTool } from './complexity-report.js'; import { registerAddDependencyTool } from './add-dependency.js'; import { registerRemoveTaskTool } from './remove-task.js'; import { registerInitializeProjectTool } from './initialize-project.js'; -import { asyncOperationManager } from '../core/utils/async-manager.js'; /** * Register all Task Master tools with the MCP server * @param {Object} server - FastMCP server instance - * @param {asyncOperationManager} asyncManager - The async operation manager instance */ -export function registerTaskMasterTools(server, asyncManager) { +export function registerTaskMasterTools(server) { try { // Register each tool registerListTasksTool(server); @@ -47,7 +45,7 @@ export function registerTaskMasterTools(server, asyncManager) { registerShowTaskTool(server); registerNextTaskTool(server); registerExpandTaskTool(server); - registerAddTaskTool(server, asyncManager); + registerAddTaskTool(server); registerAddSubtaskTool(server); registerRemoveSubtaskTool(server); registerAnalyzeTool(server); diff --git a/scripts/modules/config-manager.js b/scripts/modules/config-manager.js index 2973af0d..f7b8a392 100644 --- a/scripts/modules/config-manager.js +++ b/scripts/modules/config-manager.js @@ -170,10 +170,14 @@ function _loadAndValidateConfig(explicitRoot = null) { } } else { // Config file doesn't exist - // **Strict Check**: Throw error if config file is missing - throw new ConfigurationError( - `${CONFIG_FILE_NAME} not found at project root (${rootToUse || 'unknown'}).` + // **Changed: Log warning instead of throwing error** + console.warn( + chalk.yellow( + `Warning: ${CONFIG_FILE_NAME} not found at project root (${rootToUse || 'unknown'}). Using default configuration. Run 'task-master models --setup' to configure.` + ) ); + // Return defaults instead of throwing error + return defaults; } return config; @@ -541,11 +545,26 @@ function writeConfig(config, explicitRoot = null) { } } +/** + * Checks if the .taskmasterconfig file exists at the project root + * @param {string|null} explicitRoot - Optional explicit path to the project root + * @returns {boolean} True if the file exists, false otherwise + */ +function isConfigFilePresent(explicitRoot = null) { + const rootPath = explicitRoot || findProjectRoot(); + if (!rootPath) { + return false; + } + const configPath = path.join(rootPath, CONFIG_FILE_NAME); + return fs.existsSync(configPath); +} + export { // Core config access getConfig, writeConfig, ConfigurationError, // Export custom error type + isConfigFilePresent, // Add the new function export // Validation validateProvider, diff --git a/scripts/modules/task-manager/list-tasks.js b/scripts/modules/task-manager/list-tasks.js index e63445c0..983d08ed 100644 --- a/scripts/modules/task-manager/list-tasks.js +++ b/scripts/modules/task-manager/list-tasks.js @@ -3,6 +3,7 @@ import boxen from 'boxen'; import Table from 'cli-table3'; import { log, readJSON, truncate } from '../utils.js'; +import findNextTask from './find-next-task.js'; import { displayBanner, diff --git a/tasks/task_061.txt b/tasks/task_061.txt index b2692940..79948668 100644 --- a/tasks/task_061.txt +++ b/tasks/task_061.txt @@ -1678,6 +1678,33 @@ The new AI architecture introduces a clear separation between sensitive credenti ### Details: + +I'll provide additional technical information to enhance the "Cleanup Old AI Service Files" subtask: + +## Implementation Details + +**Pre-Cleanup Verification Steps:** +- Run a comprehensive codebase search for any remaining imports or references to `ai-services.js` and `ai-client-factory.js` using grep or your IDE's search functionality[1][4] +- Check for any dynamic imports that might not be caught by static analysis tools +- Verify that all dependent modules have been properly migrated to the new AI service architecture + +**Cleanup Process:** +- Create a backup of the files before deletion in case rollback is needed +- Document the file removal in the migration changelog with timestamps and specific file paths[5] +- Update any build configuration files that might reference these files (webpack configs, etc.) +- Run a full test suite after removal to ensure no runtime errors occur[2] + +**Post-Cleanup Validation:** +- Implement automated tests to verify the application functions correctly without the removed files +- Monitor application logs and error reporting systems for 48-72 hours after deployment to catch any missed dependencies[3] +- Perform a final code review to ensure clean architecture principles are maintained in the new implementation + +**Technical Considerations:** +- Check for any circular dependencies that might have been created during the migration process +- Ensure proper garbage collection by removing any cached instances of the old services +- Verify that performance metrics remain stable after the removal of legacy code + + ## 34. Audit and Standardize Env Variable Access [done] ### Dependencies: None ### Description: Audit the entire codebase (core modules, provider modules, utilities) to ensure all accesses to environment variables (API keys, configuration flags) consistently use a standardized resolution function (like `resolveEnvVariable` or a new utility) that checks `process.env` first and then `session.env` if available. Refactor any direct `process.env` access where `session.env` should also be considered. diff --git a/tasks/tasks.json b/tasks/tasks.json index 8b4f9858..c3d98c5f 100644 --- a/tasks/tasks.json +++ b/tasks/tasks.json @@ -3095,9 +3095,10 @@ "id": 33, "title": "Cleanup Old AI Service Files", "description": "After all other migration subtasks (refactoring, provider implementation, testing, documentation) are complete and verified, remove the old `ai-services.js` and `ai-client-factory.js` files from the `scripts/modules/` directory. Ensure no code still references them.", - "details": "", + "details": "\n\n\nI'll provide additional technical information to enhance the \"Cleanup Old AI Service Files\" subtask:\n\n## Implementation Details\n\n**Pre-Cleanup Verification Steps:**\n- Run a comprehensive codebase search for any remaining imports or references to `ai-services.js` and `ai-client-factory.js` using grep or your IDE's search functionality[1][4]\n- Check for any dynamic imports that might not be caught by static analysis tools\n- Verify that all dependent modules have been properly migrated to the new AI service architecture\n\n**Cleanup Process:**\n- Create a backup of the files before deletion in case rollback is needed\n- Document the file removal in the migration changelog with timestamps and specific file paths[5]\n- Update any build configuration files that might reference these files (webpack configs, etc.)\n- Run a full test suite after removal to ensure no runtime errors occur[2]\n\n**Post-Cleanup Validation:**\n- Implement automated tests to verify the application functions correctly without the removed files\n- Monitor application logs and error reporting systems for 48-72 hours after deployment to catch any missed dependencies[3]\n- Perform a final code review to ensure clean architecture principles are maintained in the new implementation\n\n**Technical Considerations:**\n- Check for any circular dependencies that might have been created during the migration process\n- Ensure proper garbage collection by removing any cached instances of the old services\n- Verify that performance metrics remain stable after the removal of legacy code\n", "status": "pending", "dependencies": [ + "61.31", "61.32" ], "parentTaskId": 61 diff --git a/test-config-manager.js b/test-config-manager.js new file mode 100644 index 00000000..cf8b72f7 --- /dev/null +++ b/test-config-manager.js @@ -0,0 +1,48 @@ +// test-config-manager.js +console.log('=== ENVIRONMENT TEST ==='); +console.log('Working directory:', process.cwd()); +console.log('NODE_PATH:', process.env.NODE_PATH); + +// Test basic imports +try { + console.log('Importing config-manager'); + // Use dynamic import for ESM + const configManagerModule = await import( + './scripts/modules/config-manager.js' + ); + const configManager = configManagerModule.default || configManagerModule; + console.log('Config manager loaded successfully'); + + console.log('Loading supported models'); + // Add after line 14 (after "Config manager loaded successfully") + console.log('Config manager exports:', Object.keys(configManager)); +} catch (error) { + console.error('Import error:', error.message); + console.error(error.stack); +} + +// Test file access +try { + console.log('Checking for .taskmasterconfig'); + // Use dynamic import for ESM + const { readFileSync, existsSync } = await import('fs'); + const { resolve } = await import('path'); + + const configExists = existsSync('./.taskmasterconfig'); + console.log('.taskmasterconfig exists:', configExists); + + if (configExists) { + const config = JSON.parse(readFileSync('./.taskmasterconfig', 'utf-8')); + console.log('Config keys:', Object.keys(config)); + } + + console.log('Checking for supported-models.json'); + const modelsPath = resolve('./scripts/modules/supported-models.json'); + console.log('Models path:', modelsPath); + const modelsExists = existsSync(modelsPath); + console.log('supported-models.json exists:', modelsExists); +} catch (error) { + console.error('File access error:', error.message); +} + +console.log('=== TEST COMPLETE ===');