From 515dcae9654cf602a48ac9ea310fa772738c072a Mon Sep 17 00:00:00 2001 From: Eyal Toledano Date: Mon, 21 Apr 2025 22:25:04 -0400 Subject: [PATCH] refactor(config)!: Enforce .taskmasterconfig and remove env var overrides BREAKING CHANGE: Taskmaster now requires a `.taskmasterconfig` file for model/parameter settings. Environment variables (except API keys) are no longer used for overrides. - Throws an error if `.taskmasterconfig` is missing, guiding user to run `task-master models --setup`." -m "- Removed env var checks from config getters in `config-manager.js`." -m "- Updated `env.example` to remove obsolete variables." -m "- Refined missing config file error message in `commands.js`. --- .env.example | 2 +- assets/env.example | 13 +---- scripts/modules/commands.js | 59 +++++++++++++++++-- scripts/modules/config-manager.js | 94 ++++++++++++++++++++----------- 4 files changed, 118 insertions(+), 50 deletions(-) diff --git a/.env.example b/.env.example index 45284a3c..42bc5408 100644 --- a/.env.example +++ b/.env.example @@ -17,4 +17,4 @@ DEFAULT_SUBTASKS=5 # Default number of subtasks DEFAULT_PRIORITY=medium # Default priority for generated tasks (high, medium, low) # Project Metadata (Optional) - PROJECT_NAME=Your Project Name # Override default project name in tasks.json \ No newline at end of file +PROJECT_NAME=Your Project Name # Override default project name in tasks.json \ No newline at end of file diff --git a/assets/env.example b/assets/env.example index f2ce88d6..20e5b2ce 100644 --- a/assets/env.example +++ b/assets/env.example @@ -7,15 +7,4 @@ GROK_API_KEY=your_grok_api_key_here # Optional, for XAI Grok mod MISTRAL_API_KEY=your_mistral_key_here # Optional, for Mistral AI models. AZURE_OPENAI_API_KEY=your_azure_key_here # Optional, for Azure OpenAI models. AZURE_OPENAI_ENDPOINT=your_azure_endpoint_here # Optional, for Azure OpenAI. - -# Optional - defaults shown -MODEL=claude-3-7-sonnet-20250219 # Recommended models: claude-3-7-sonnet-20250219, claude-3-opus-20240229 (Required) -PERPLEXITY_MODEL=sonar-pro # Make sure you have access to sonar-pro otherwise you can use sonar regular (Optional) -MAX_TOKENS=64000 # Maximum tokens for model responses (Required) -TEMPERATURE=0.2 # Temperature for model responses (0.0-1.0) - lower = less creativity and follow your prompt closely (Required) -DEBUG=false # Enable debug logging (true/false) -LOG_LEVEL=info # Log level (debug, info, warn, error) -DEFAULT_SUBTASKS=5 # Default number of subtasks when expanding -DEFAULT_PRIORITY=medium # Default priority for generated tasks (high, medium, low) -PROJECT_NAME={{projectName}} # Project name for tasks.json metadata -OLLAMA_BASE_URL=http://localhost:11434/api # Base URL for local Ollama instance (Optional) \ No newline at end of file +OLLAMA_BASE_URL=http://localhost:11434/api # Base URL for local Ollama instance (Optional) \ No newline at end of file diff --git a/scripts/modules/commands.js b/scripts/modules/commands.js index 29a7b9ab..084ec171 100644 --- a/scripts/modules/commands.js +++ b/scripts/modules/commands.js @@ -53,7 +53,8 @@ import { getMcpApiKeyStatus, getDebugFlag, getConfig, - writeConfig + writeConfig, + ConfigurationError // Import the custom error } from './config-manager.js'; import { @@ -2377,6 +2378,8 @@ async function runCLI(argv = process.argv) { const updateCheckPromise = checkForUpdate(); // Setup and parse + // NOTE: getConfig() might be called during setupCLI->registerCommands if commands need config + // This means the ConfigurationError might be thrown here if .taskmasterconfig is missing. const programInstance = setupCLI(); await programInstance.parseAsync(argv); @@ -2389,10 +2392,56 @@ async function runCLI(argv = process.argv) { ); } } catch (error) { - console.error(chalk.red(`Error: ${error.message}`)); - - if (getDebugFlag()) { - console.error(error); + // ** Specific catch block for missing configuration file ** + if (error instanceof ConfigurationError) { + console.error( + boxen( + chalk.red.bold('Configuration Update Required!') + + '\n\n' + + chalk.white('Taskmaster now uses the ') + + chalk.yellow.bold('.taskmasterconfig') + + chalk.white( + ' file in your project root for AI model choices and settings.\n\n' + + 'This file appears to be ' + ) + + chalk.red.bold('missing') + + chalk.white('. No worries though.\n\n') + + chalk.cyan.bold('To create this file, run the interactive setup:') + + '\n' + + chalk.green(' task-master models --setup') + + '\n\n' + + chalk.white.bold('Key Points:') + + '\n' + + chalk.white('* ') + + chalk.yellow.bold('.taskmasterconfig') + + chalk.white( + ': Stores your AI model settings (do not manually edit)\n' + ) + + chalk.white('* ') + + chalk.yellow.bold('.env & .mcp.json') + + chalk.white(': Still used ') + + chalk.red.bold('only') + + chalk.white(' for your AI provider API keys.\n\n') + + chalk.cyan( + '`task-master models` to check your config & available models\n' + ) + + chalk.cyan( + '`task-master models --setup` to adjust the AI models used by Taskmaster' + ), + { + padding: 1, + margin: { top: 1 }, + borderColor: 'red', + borderStyle: 'round' + } + ) + ); + } else { + // Generic error handling for other errors + console.error(chalk.red(`Error: ${error.message}`)); + if (getDebugFlag()) { + console.error(error); + } } process.exit(1); diff --git a/scripts/modules/config-manager.js b/scripts/modules/config-manager.js index edb42d9d..0b2e27dd 100644 --- a/scripts/modules/config-manager.js +++ b/scripts/modules/config-manager.js @@ -75,10 +75,19 @@ const DEFAULTS = { // --- Internal Config Loading --- let loadedConfig = null; // Cache for loaded config +// Custom Error for configuration issues +class ConfigurationError extends Error { + constructor(message) { + super(message); + this.name = 'ConfigurationError'; + } +} + function _loadAndValidateConfig(explicitRoot = null) { // Determine the root path to use const rootToUse = explicitRoot || findProjectRoot(); const defaults = DEFAULTS; // Use the defined defaults + let configExists = false; if (!rootToUse) { console.warn( @@ -86,34 +95,37 @@ function _loadAndValidateConfig(explicitRoot = null) { 'Warning: Could not determine project root. Using default configuration.' ) ); - return defaults; + // 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 = path.join(rootToUse, CONFIG_FILE_NAME); + const configPath = rootToUse ? path.join(rootToUse, CONFIG_FILE_NAME) : null; - if (fs.existsSync(configPath)) { + let config = defaults; // Start with defaults + + if (configPath && fs.existsSync(configPath)) { + configExists = true; try { const rawData = fs.readFileSync(configPath, 'utf-8'); const parsedConfig = JSON.parse(rawData); - // Deep merge with defaults - const config = { + // Deep merge parsed config onto defaults + config = { models: { main: { ...defaults.models.main, ...parsedConfig?.models?.main }, research: { ...defaults.models.research, ...parsedConfig?.models?.research }, - // Fallback needs careful merging - only merge if provider/model exist fallback: parsedConfig?.models?.fallback?.provider && parsedConfig?.models?.fallback?.modelId ? { ...defaults.models.fallback, ...parsedConfig.models.fallback } - : { ...defaults.models.fallback } // Use default params even if provider/model missing + : { ...defaults.models.fallback } }, global: { ...defaults.global, ...parsedConfig?.global } }; - // --- Validation --- + // --- Validation (Only warn if file exists but content is invalid) --- // Validate main provider/model if (!validateProvider(config.models.main.provider)) { console.warn( @@ -123,7 +135,6 @@ function _loadAndValidateConfig(explicitRoot = null) { ); config.models.main = { ...defaults.models.main }; } - // Optional: Add warning for model combination if desired // Validate research provider/model if (!validateProvider(config.models.research.provider)) { @@ -134,7 +145,6 @@ function _loadAndValidateConfig(explicitRoot = null) { ); config.models.research = { ...defaults.models.research }; } - // Optional: Add warning for model combination if desired // Validate fallback provider if it exists if ( @@ -146,24 +156,27 @@ function _loadAndValidateConfig(explicitRoot = null) { `Warning: Invalid fallback provider "${config.models.fallback.provider}" in ${CONFIG_FILE_NAME}. Fallback model configuration will be ignored.` ) ); - // Clear invalid fallback provider/model, but keep default params if needed elsewhere config.models.fallback.provider = undefined; config.models.fallback.modelId = undefined; } - - return config; } catch (error) { console.error( chalk.red( `Error reading or parsing ${configPath}: ${error.message}. Using default configuration.` ) ); - return defaults; + config = defaults; // Reset to defaults on parse error + // Do not throw ConfigurationError here, allow fallback to defaults if file is corrupt } } else { - // Config file doesn't exist, use defaults - return defaults; + // 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'}).` + ); } + + return config; } /** @@ -218,8 +231,13 @@ function getModelConfigForRole(role, explicitRoot = null) { const config = getConfig(explicitRoot); const roleConfig = config?.models?.[role]; if (!roleConfig) { - log('warn', `No model configuration found for role: ${role}`); - return DEFAULTS.models[role] || {}; // Fallback to default for the role + // This shouldn't happen if _loadAndValidateConfig ensures defaults + // But as a safety net, log and return defaults + log( + 'warn', + `No model configuration found for role: ${role}. Returning default.` + ); + return DEFAULTS.models[role] || {}; } return roleConfig; } @@ -233,10 +251,12 @@ function getMainModelId(explicitRoot = null) { } function getMainMaxTokens(explicitRoot = null) { + // Directly return value from config (which includes defaults) return getModelConfigForRole('main', explicitRoot).maxTokens; } function getMainTemperature(explicitRoot = null) { + // Directly return value from config return getModelConfigForRole('main', explicitRoot).temperature; } @@ -249,30 +269,32 @@ function getResearchModelId(explicitRoot = null) { } function getResearchMaxTokens(explicitRoot = null) { + // Directly return value from config return getModelConfigForRole('research', explicitRoot).maxTokens; } function getResearchTemperature(explicitRoot = null) { + // Directly return value from config return getModelConfigForRole('research', explicitRoot).temperature; } function getFallbackProvider(explicitRoot = null) { - // Specifically check if provider is set, as fallback is optional - return getModelConfigForRole('fallback', explicitRoot).provider || undefined; + // Directly return value from config (will be undefined if not set) + return getModelConfigForRole('fallback', explicitRoot).provider; } function getFallbackModelId(explicitRoot = null) { - // Specifically check if modelId is set - return getModelConfigForRole('fallback', explicitRoot).modelId || undefined; + // Directly return value from config + return getModelConfigForRole('fallback', explicitRoot).modelId; } function getFallbackMaxTokens(explicitRoot = null) { - // Return fallback tokens even if provider/model isn't set, in case it's needed generically + // Directly return value from config return getModelConfigForRole('fallback', explicitRoot).maxTokens; } function getFallbackTemperature(explicitRoot = null) { - // Return fallback temp even if provider/model isn't set + // Directly return value from config return getModelConfigForRole('fallback', explicitRoot).temperature; } @@ -280,32 +302,39 @@ function getFallbackTemperature(explicitRoot = null) { function getGlobalConfig(explicitRoot = null) { const config = getConfig(explicitRoot); - return config?.global || DEFAULTS.global; + // Ensure global defaults are applied if global section is missing + return { ...DEFAULTS.global, ...(config?.global || {}) }; } function getLogLevel(explicitRoot = null) { - return getGlobalConfig(explicitRoot).logLevel; + // Directly return value from config + return getGlobalConfig(explicitRoot).logLevel.toLowerCase(); } function getDebugFlag(explicitRoot = null) { - // Ensure boolean type + // Directly return value from config, ensure boolean return getGlobalConfig(explicitRoot).debug === true; } function getDefaultSubtasks(explicitRoot = null) { - // Ensure integer type - return parseInt(getGlobalConfig(explicitRoot).defaultSubtasks, 10); + // Directly return value from config, ensure integer + const val = getGlobalConfig(explicitRoot).defaultSubtasks; + const parsedVal = parseInt(val, 10); + return isNaN(parsedVal) ? DEFAULTS.global.defaultSubtasks : parsedVal; } function getDefaultPriority(explicitRoot = null) { + // Directly return value from config return getGlobalConfig(explicitRoot).defaultPriority; } function getProjectName(explicitRoot = null) { + // Directly return value from config return getGlobalConfig(explicitRoot).projectName; } function getOllamaBaseUrl(explicitRoot = null) { + // Directly return value from config return getGlobalConfig(explicitRoot).ollamaBaseUrl; } @@ -500,8 +529,9 @@ function writeConfig(config, explicitRoot = null) { export { // Core config access - getConfig, // Might still be useful for getting the whole object + getConfig, writeConfig, + ConfigurationError, // Export custom error type // Validation validateProvider, @@ -510,7 +540,7 @@ export { MODEL_MAP, getAvailableModels, - // Role-specific getters + // Role-specific getters (No env var overrides) getMainProvider, getMainModelId, getMainMaxTokens, @@ -524,7 +554,7 @@ export { getFallbackMaxTokens, getFallbackTemperature, - // Global setting getters + // Global setting getters (No env var overrides) getLogLevel, getDebugFlag, getDefaultSubtasks,