From ef0c911722bf6edc9800be4d7eb71acaf77dd583 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Mon, 9 Feb 2026 09:09:59 +0100 Subject: [PATCH] fix: address code review findings from PR #1612 - Restore saved IDE configurations when removal is cancelled or skipped in non-interactive mode, preventing silent config downgrade (e.g., Codex 'project' install reverting to 'global') - Short-circuit IDE removal block when skipPrompts is true to eliminate unnecessary warning/cancelled log output in --yes mode - Add null guard on config.ides before pushing re-added IDEs - Return empty result object from createModuleDirectories early exits instead of undefined for a uniform return contract Co-Authored-By: Claude Opus 4.6 --- tools/cli/installers/lib/core/installer.js | 88 ++++++++++++--------- tools/cli/installers/lib/modules/manager.js | 11 +-- 2 files changed, 57 insertions(+), 42 deletions(-) diff --git a/tools/cli/installers/lib/core/installer.js b/tools/cli/installers/lib/core/installer.js index 5cf8f4427..f3bb027af 100644 --- a/tools/cli/installers/lib/core/installer.js +++ b/tools/cli/installers/lib/core/installer.js @@ -710,47 +710,61 @@ class Installer { const idesToRemove = [...previouslyInstalledIdes].filter((ide) => !newlySelectedIdes.has(ide)); if (idesToRemove.length > 0) { - if (spinner.isSpinning) { - spinner.stop('Reviewing IDE changes'); - } - - await prompts.log.warn('IDEs to be removed:'); - for (const ide of idesToRemove) { - await prompts.log.error(` - ${ide}`); - } - - // In non-interactive mode, preserve existing configs (matches prompt default: false) - const confirmRemoval = config.skipPrompts - ? false - : await prompts.confirm({ - message: `Remove BMAD configuration for ${idesToRemove.length} IDE(s)?`, - default: false, - }); - - if (confirmRemoval) { - await this.ideManager.ensureInitialized(); - for (const ide of idesToRemove) { - try { - const handler = this.ideManager.handlers.get(ide); - if (handler) { - await handler.cleanup(projectDir); - } - await this.ideConfigManager.deleteIdeConfig(bmadDir, ide); - await prompts.log.message(` Removed: ${ide}`); - } catch (error) { - await prompts.log.warn(` Warning: Failed to remove ${ide}: ${error.message}`); - } - } - await prompts.log.success(` Removed ${idesToRemove.length} IDE(s)`); - } else { - await prompts.log.message(' IDE removal cancelled'); - // Add IDEs back to selection since user cancelled removal + if (config.skipPrompts) { + // Non-interactive mode: silently preserve existing IDE configs + if (!config.ides) config.ides = []; + const savedIdeConfigs = await this.ideConfigManager.loadAllIdeConfigs(bmadDir); for (const ide of idesToRemove) { config.ides.push(ide); + if (savedIdeConfigs[ide] && !ideConfigurations[ide]) { + ideConfigurations[ide] = savedIdeConfigs[ide]; + } + } + } else { + if (spinner.isSpinning) { + spinner.stop('Reviewing IDE changes'); } - } - spinner.start('Preparing installation...'); + await prompts.log.warn('IDEs to be removed:'); + for (const ide of idesToRemove) { + await prompts.log.error(` - ${ide}`); + } + + const confirmRemoval = await prompts.confirm({ + message: `Remove BMAD configuration for ${idesToRemove.length} IDE(s)?`, + default: false, + }); + + if (confirmRemoval) { + await this.ideManager.ensureInitialized(); + for (const ide of idesToRemove) { + try { + const handler = this.ideManager.handlers.get(ide); + if (handler) { + await handler.cleanup(projectDir); + } + await this.ideConfigManager.deleteIdeConfig(bmadDir, ide); + await prompts.log.message(` Removed: ${ide}`); + } catch (error) { + await prompts.log.warn(` Warning: Failed to remove ${ide}: ${error.message}`); + } + } + await prompts.log.success(` Removed ${idesToRemove.length} IDE(s)`); + } else { + await prompts.log.message(' IDE removal cancelled'); + // Add IDEs back to selection and restore their saved configurations + if (!config.ides) config.ides = []; + const savedIdeConfigs = await this.ideConfigManager.loadAllIdeConfigs(bmadDir); + for (const ide of idesToRemove) { + config.ides.push(ide); + if (savedIdeConfigs[ide] && !ideConfigurations[ide]) { + ideConfigurations[ide] = savedIdeConfigs[ide]; + } + } + } + + spinner.start('Preparing installation...'); + } } } diff --git a/tools/cli/installers/lib/modules/manager.js b/tools/cli/installers/lib/modules/manager.js index 72f72c56e..6e5526fbd 100644 --- a/tools/cli/installers/lib/modules/manager.js +++ b/tools/cli/installers/lib/modules/manager.js @@ -1252,11 +1252,12 @@ class ModuleManager { * @param {Object} options - Installation options * @param {Object} options.moduleConfig - Module configuration from config collector * @param {Object} options.coreConfig - Core configuration - * @returns {Promise<{createdDirs: string[], createdWdsFolders: string[]} | undefined>} Created directories info + * @returns {Promise<{createdDirs: string[], createdWdsFolders: string[]}>} Created directories info */ async createModuleDirectories(moduleName, bmadDir, options = {}) { const moduleConfig = options.moduleConfig || {}; const projectRoot = path.dirname(bmadDir); + const emptyResult = { createdDirs: [], createdWdsFolders: [] }; // Special handling for core module - it's in src/core not src/modules let sourcePath; @@ -1265,14 +1266,14 @@ class ModuleManager { } else { sourcePath = await this.findModuleSource(moduleName, { silent: true }); if (!sourcePath) { - return; // No source found, skip + return emptyResult; // No source found, skip } } // Read module.yaml to find the `directories` key const moduleYamlPath = path.join(sourcePath, 'module.yaml'); if (!(await fs.pathExists(moduleYamlPath))) { - return; // No module.yaml, skip + return emptyResult; // No module.yaml, skip } let moduleYaml; @@ -1280,11 +1281,11 @@ class ModuleManager { const yamlContent = await fs.readFile(moduleYamlPath, 'utf8'); moduleYaml = yaml.parse(yamlContent); } catch { - return; // Invalid YAML, skip + return emptyResult; // Invalid YAML, skip } if (!moduleYaml || !moduleYaml.directories) { - return; // No directories declared, skip + return emptyResult; // No directories declared, skip } const directories = moduleYaml.directories;