From 205a11e82c25cdee4a8616e75dbc76700061fc66 Mon Sep 17 00:00:00 2001 From: Eyal Toledano Date: Thu, 24 Apr 2025 13:34:51 -0400 Subject: [PATCH] fix(config): Improve config-manager.js for MCP server integration - Fixed MCP server initialization warnings by refactoring config-manager.js to handle missing project roots silently during startup - Added project root tracking (loadedConfigRoot) to improve config caching and prevent unnecessary reloads - Modified _loadAndValidateConfig to return defaults without warnings when no explicitRoot provided - Improved getConfig to only update cache when loading config with a specific project root - Ensured warning messages still appear when explicitly specified roots have missing/invalid configs - Prevented console output during MCP startup that was causing JSON parsing errors - Verified parse_prd and other MCP tools still work correctly with the new config loading approach. - Replaces test perplexity api key in mcp.json and rolls it. It's invalid now. --- .cursor/mcp.json | 2 +- .../src/core/direct-functions/parse-prd.js | 95 +++++-------------- scripts/modules/ai-services-unified.js | 2 +- scripts/modules/config-manager.js | 84 ++++++++-------- scripts/modules/task-manager/models.js | 6 +- tests/unit/ai-services-unified.test.js | 22 ++--- 6 files changed, 86 insertions(+), 125 deletions(-) diff --git a/.cursor/mcp.json b/.cursor/mcp.json index e0ce8ae3..e322a13b 100644 --- a/.cursor/mcp.json +++ b/.cursor/mcp.json @@ -5,7 +5,7 @@ "args": ["./mcp-server/server.js"], "env": { "ANTHROPIC_API_KEY": "sk-ant-apikeyhere", - "PERPLEXITY_API_KEY": "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", + "PERPLEXITY_API_KEY": "pplx-apikeyhere", "OPENAI_API_KEY": "sk-proj-1234567890", "GOOGLE_API_KEY": "AIzaSyB1234567890", "GROK_API_KEY": "gsk_1234567890", diff --git a/mcp-server/src/core/direct-functions/parse-prd.js b/mcp-server/src/core/direct-functions/parse-prd.js index c3220962..47e6973c 100644 --- a/mcp-server/src/core/direct-functions/parse-prd.js +++ b/mcp-server/src/core/direct-functions/parse-prd.js @@ -12,41 +12,21 @@ import { enableSilentMode, disableSilentMode } from '../../../../scripts/modules/utils.js'; -import { - getAnthropicClientForMCP, - getModelConfig -} from '../utils/ai-client-utils.js'; /** * Direct function wrapper for parsing PRD documents and generating tasks. * - * @param {Object} args - Command arguments containing input, numTasks or tasks, and output options. + * @param {Object} args - Command arguments containing projectRoot, input, output, numTasks options. * @param {Object} log - Logger object. * @param {Object} context - Context object containing session data. * @returns {Promise} - Result object with success status and data/error information. */ export async function parsePRDDirect(args, log, context = {}) { - const { session } = context; // Only extract session, not reportProgress + const { session } = context; // Only extract session try { log.info(`Parsing PRD document with args: ${JSON.stringify(args)}`); - // Initialize AI client for PRD parsing - let aiClient; - try { - aiClient = getAnthropicClientForMCP(session, log); - } catch (error) { - log.error(`Failed to initialize AI client: ${error.message}`); - return { - success: false, - error: { - code: 'AI_CLIENT_ERROR', - message: `Cannot initialize AI client: ${error.message}` - }, - fromCache: false - }; - } - // Validate required parameters if (!args.projectRoot) { const errorMessage = 'Project root is required for parsePRDDirect'; @@ -57,7 +37,6 @@ export async function parsePRDDirect(args, log, context = {}) { fromCache: false }; } - if (!args.input) { const errorMessage = 'Input file path is required for parsePRDDirect'; log.error(errorMessage); @@ -67,7 +46,6 @@ export async function parsePRDDirect(args, log, context = {}) { fromCache: false }; } - if (!args.output) { const errorMessage = 'Output file path is required for parsePRDDirect'; log.error(errorMessage); @@ -137,58 +115,35 @@ export async function parsePRDDirect(args, log, context = {}) { success: (message, ...args) => log.info(message, ...args) // Map success to info }; - // Get model config from session - const modelConfig = getModelConfig(session); - // Enable silent mode to prevent console logs from interfering with JSON response enableSilentMode(); try { - // Make sure the output directory exists - const outputDir = path.dirname(outputPath); - if (!fs.existsSync(outputDir)) { - log.info(`Creating output directory: ${outputDir}`); - fs.mkdirSync(outputDir, { recursive: true }); + // Execute core parsePRD function - It now handles AI internally + const tasksDataResult = await parsePRD(inputPath, outputPath, numTasks, { + mcpLog: logWrapper, + session + }); + + // Check the result from the core function (assuming it might return data or null/undefined) + if (!tasksDataResult || !tasksDataResult.tasks) { + throw new Error( + 'Core parsePRD function did not return valid task data.' + ); } - // Execute core parsePRD function with AI client - await parsePRD( - inputPath, - outputPath, - numTasks, - { - mcpLog: logWrapper, - session - }, - aiClient, - modelConfig + log.info( + `Successfully parsed PRD and generated ${tasksDataResult.tasks?.length || 0} tasks` ); - // Since parsePRD doesn't return a value but writes to a file, we'll read the result - // to return it to the caller - if (fs.existsSync(outputPath)) { - const tasksData = JSON.parse(fs.readFileSync(outputPath, 'utf8')); - log.info( - `Successfully parsed PRD and generated ${tasksData.tasks?.length || 0} tasks` - ); - - return { - success: true, - data: { - message: `Successfully generated ${tasksData.tasks?.length || 0} tasks from PRD`, - taskCount: tasksData.tasks?.length || 0, - outputPath - }, - fromCache: false // This operation always modifies state and should never be cached - }; - } else { - const errorMessage = `Tasks file was not created at ${outputPath}`; - log.error(errorMessage); - return { - success: false, - error: { code: 'OUTPUT_FILE_NOT_CREATED', message: errorMessage }, - fromCache: false - }; - } + return { + success: true, + data: { + message: `Successfully generated ${tasksDataResult.tasks?.length || 0} tasks from PRD`, + taskCount: tasksDataResult.tasks?.length || 0, + outputPath + }, + fromCache: false // This operation always modifies state + }; } finally { // Always restore normal logging disableSilentMode(); @@ -201,7 +156,7 @@ export async function parsePRDDirect(args, log, context = {}) { return { success: false, error: { - code: 'PARSE_PRD_ERROR', + code: error.code || 'PARSE_PRD_ERROR', // Use error code if available message: error.message || 'Unknown error parsing PRD' }, fromCache: false diff --git a/scripts/modules/ai-services-unified.js b/scripts/modules/ai-services-unified.js index 0e1c88af..baf089fe 100644 --- a/scripts/modules/ai-services-unified.js +++ b/scripts/modules/ai-services-unified.js @@ -209,7 +209,7 @@ async function _unifiedServiceRunner(serviceType, params) { let providerName, modelId, apiKey, roleParams, providerFnSet, providerApiFn; try { - log('info', `Attempting service call with role: ${currentRole}`); + log('info', `New AI service call with role: ${currentRole}`); // --- Corrected Config Fetching --- // 1. Get Config: Provider, Model, Parameters for the current role diff --git a/scripts/modules/config-manager.js b/scripts/modules/config-manager.js index f7b8a392..a3746702 100644 --- a/scripts/modules/config-manager.js +++ b/scripts/modules/config-manager.js @@ -73,7 +73,8 @@ const DEFAULTS = { }; // --- Internal Config Loading --- -let loadedConfig = null; // Cache for loaded config +let loadedConfig = null; +let loadedConfigRoot = null; // Track which root loaded the config // Custom Error for configuration issues class ConfigurationError extends Error { @@ -84,25 +85,20 @@ class ConfigurationError extends Error { } function _loadAndValidateConfig(explicitRoot = null) { - // Determine the root path to use - const rootToUse = explicitRoot || findProjectRoot(); const defaults = DEFAULTS; // Use the defined defaults + + // If no explicit root is provided (e.g., during initial server load), + // return defaults immediately and silently. + if (!explicitRoot) { + return defaults; + } + + // --- Proceed with loading from the provided explicitRoot --- + const configPath = path.join(explicitRoot, CONFIG_FILE_NAME); + let config = { ...defaults }; // Start with a deep copy of defaults let configExists = false; - if (!rootToUse) { - console.warn( - chalk.yellow( - 'Warning: Could not determine project root. Using default configuration.' - ) - ); - // Allow proceeding with defaults if root finding fails, but validation later might trigger error - // Or perhaps throw here? Let's throw later based on file existence check. - } - const configPath = rootToUse ? path.join(rootToUse, CONFIG_FILE_NAME) : null; - - let config = defaults; // Start with defaults - - if (configPath && fs.existsSync(configPath)) { + if (fs.existsSync(configPath)) { configExists = true; try { const rawData = fs.readFileSync(configPath, 'utf-8'); @@ -125,59 +121,55 @@ function _loadAndValidateConfig(explicitRoot = null) { global: { ...defaults.global, ...parsedConfig?.global } }; - // --- Validation (Only warn if file exists but content is invalid) --- - // Validate main provider/model + // --- Validation (Warn if file content is invalid) --- + // Only use console.warn here, as this part runs only when an explicitRoot *is* provided if (!validateProvider(config.models.main.provider)) { console.warn( chalk.yellow( - `Warning: Invalid main provider "${config.models.main.provider}" in ${CONFIG_FILE_NAME}. Falling back to default.` + `Warning: Invalid main provider "${config.models.main.provider}" in ${configPath}. Falling back to default.` ) ); config.models.main = { ...defaults.models.main }; } - - // Validate research provider/model if (!validateProvider(config.models.research.provider)) { console.warn( chalk.yellow( - `Warning: Invalid research provider "${config.models.research.provider}" in ${CONFIG_FILE_NAME}. Falling back to default.` + `Warning: Invalid research provider "${config.models.research.provider}" in ${configPath}. Falling back to default.` ) ); config.models.research = { ...defaults.models.research }; } - - // Validate fallback provider if it exists if ( config.models.fallback?.provider && !validateProvider(config.models.fallback.provider) ) { console.warn( chalk.yellow( - `Warning: Invalid fallback provider "${config.models.fallback.provider}" in ${CONFIG_FILE_NAME}. Fallback model configuration will be ignored.` + `Warning: Invalid fallback provider "${config.models.fallback.provider}" in ${configPath}. Fallback model configuration will be ignored.` ) ); config.models.fallback.provider = undefined; config.models.fallback.modelId = undefined; } } catch (error) { + // Use console.error for actual errors during parsing console.error( chalk.red( `Error reading or parsing ${configPath}: ${error.message}. Using default configuration.` ) ); - config = defaults; // Reset to defaults on parse error - // Do not throw ConfigurationError here, allow fallback to defaults if file is corrupt + config = { ...defaults }; // Reset to defaults on parse error } } else { - // Config file doesn't exist - // **Changed: Log warning instead of throwing error** + // Config file doesn't exist at the provided explicitRoot. + // Use console.warn because an explicit root *was* given. 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.` + `Warning: ${CONFIG_FILE_NAME} not found at provided project root (${explicitRoot}). Using default configuration. Run 'task-master models --setup' to configure.` ) ); - // Return defaults instead of throwing error - return defaults; + // Keep config as defaults + config = { ...defaults }; } return config; @@ -185,18 +177,32 @@ function _loadAndValidateConfig(explicitRoot = null) { /** * Gets the current configuration, loading it if necessary. + * Handles MCP initialization context gracefully. * @param {string|null} explicitRoot - Optional explicit path to the project root. * @param {boolean} forceReload - Force reloading the config file. * @returns {object} The loaded configuration object. */ function getConfig(explicitRoot = null, forceReload = false) { - if (!loadedConfig || forceReload) { - loadedConfig = _loadAndValidateConfig(explicitRoot); - } - // If an explicitRoot was provided for a one-off check, don't cache it permanently - if (explicitRoot && !forceReload) { - return _loadAndValidateConfig(explicitRoot); + // Determine if a reload is necessary + const needsLoad = + !loadedConfig || + forceReload || + (explicitRoot && explicitRoot !== loadedConfigRoot); + + if (needsLoad) { + const newConfig = _loadAndValidateConfig(explicitRoot); // _load handles null explicitRoot + + // Only update the global cache if loading was forced or if an explicit root + // was provided (meaning we attempted to load a specific project's config). + // We avoid caching the initial default load triggered without an explicitRoot. + if (forceReload || explicitRoot) { + loadedConfig = newConfig; + loadedConfigRoot = explicitRoot; // Store the root used for this loaded config + } + return newConfig; // Return the newly loaded/default config } + + // If no load was needed, return the cached config return loadedConfig; } diff --git a/scripts/modules/task-manager/models.js b/scripts/modules/task-manager/models.js index 40c5c5bd..fb88ba9a 100644 --- a/scripts/modules/task-manager/models.js +++ b/scripts/modules/task-manager/models.js @@ -79,14 +79,14 @@ async function getModelConfiguration(options = {}) { // Check API keys const mainCliKeyOk = isApiKeySet(mainProvider); - const mainMcpKeyOk = getMcpApiKeyStatus(mainProvider); + const mainMcpKeyOk = getMcpApiKeyStatus(mainProvider, projectRoot); const researchCliKeyOk = isApiKeySet(researchProvider); - const researchMcpKeyOk = getMcpApiKeyStatus(researchProvider); + const researchMcpKeyOk = getMcpApiKeyStatus(researchProvider, projectRoot); const fallbackCliKeyOk = fallbackProvider ? isApiKeySet(fallbackProvider) : true; const fallbackMcpKeyOk = fallbackProvider - ? getMcpApiKeyStatus(fallbackProvider) + ? getMcpApiKeyStatus(fallbackProvider, projectRoot) : true; // Get available models to find detailed info diff --git a/tests/unit/ai-services-unified.test.js b/tests/unit/ai-services-unified.test.js index 3d7a4351..ae0733cf 100644 --- a/tests/unit/ai-services-unified.test.js +++ b/tests/unit/ai-services-unified.test.js @@ -162,7 +162,7 @@ describe('Unified AI Services', () => { expect(mockLog).toHaveBeenNthCalledWith( 2, 'info', - 'Attempting service call with role: main' + 'New AI service call with role: main' ); expect(mockLog).toHaveBeenNthCalledWith( 3, @@ -229,7 +229,7 @@ describe('Unified AI Services', () => { ); expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: main' + 'New AI service call with role: main' ); expect(mockLog).toHaveBeenCalledWith('info', 'Retrieved AI client', { provider: mockClient.provider, @@ -277,7 +277,7 @@ describe('Unified AI Services', () => { // Check subsequent fallback attempts (which also fail) expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: fallback' + 'New AI service call with role: fallback' ); expect(mockLog).toHaveBeenCalledWith( 'error', @@ -285,7 +285,7 @@ describe('Unified AI Services', () => { ); expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: research' + 'New AI service call with role: research' ); expect(mockLog).toHaveBeenCalledWith( 'error', @@ -349,7 +349,7 @@ describe('Unified AI Services', () => { ); expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: fallback' + 'New AI service call with role: fallback' ); expect(mockLog).toHaveBeenCalledWith( 'info', @@ -431,7 +431,7 @@ describe('Unified AI Services', () => { ); expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: research' + 'New AI service call with role: research' ); expect(mockLog).toHaveBeenCalledWith( 'info', @@ -509,7 +509,7 @@ describe('Unified AI Services', () => { ); expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: research' + 'New AI service call with role: research' ); expect(mockLog).toHaveBeenCalledWith( 'info', @@ -554,7 +554,7 @@ describe('Unified AI Services', () => { // Check logs for sequence expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: fallback' + 'New AI service call with role: fallback' ); expect(mockLog).toHaveBeenCalledWith( 'error', @@ -568,7 +568,7 @@ describe('Unified AI Services', () => { ); expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: research' + 'New AI service call with role: research' ); expect(mockLog).toHaveBeenCalledWith( 'info', @@ -613,7 +613,7 @@ describe('Unified AI Services', () => { // Check logs for sequence expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: research' + 'New AI service call with role: research' ); expect(mockLog).toHaveBeenCalledWith( 'error', @@ -627,7 +627,7 @@ describe('Unified AI Services', () => { ); expect(mockLog).toHaveBeenCalledWith( 'info', - 'Attempting service call with role: fallback' + 'New AI service call with role: fallback' ); expect(mockLog).toHaveBeenCalledWith( 'info',