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.
2025-04-24 13:34:51 -04:00
parent be3f68e777
commit 205a11e82c
6 changed files with 86 additions and 125 deletions
--- 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",
--- 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<Object>} - 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
--- 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
--- 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;
 }

--- 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
--- 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',