From bbdefb4269b218c2cfdd331702167517daf8b023 Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Mon, 27 Apr 2026 22:51:15 -0500 Subject: [PATCH] =?UTF-8?q?fix(installer):=20address=20review=20for=20#232?= =?UTF-8?q?6=20=E2=80=94=20single=20source=20of=20truth,=20drop=20dead=20c?= =?UTF-8?q?ode,=20add=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Refactor formatPlatformList to use IdeManager so --list-tools and --tools validation see the same set of platforms. Eliminates the drift where suspended platforms appeared in --list-tools but were rejected at validation. - Drop unused getValidPlatformIds export. - Flatten redundant block scope around the throw in the --yes-without-tools branch (refactor leftover). - Drop dead String() defensive cast (Commander always passes a string). - Add Test Suite 42: 8 unit tests covering _parseToolsFlag empty/whitespace/ unknown/typo cases plus an integration check that --list-tools output and --tools validation agree on the ID set. --- test/test-installation-components.js | 88 +++++++++++++++++++++++++++ tools/installer/ide/platform-codes.js | 43 +++++-------- tools/installer/ui.js | 28 ++++----- 3 files changed, 116 insertions(+), 43 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 4827afcbf..f63f1b446 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -2773,6 +2773,94 @@ async function runTests() { console.log(''); + // ============================================================ + // Test Suite 42: --tools flag parsing & validation (#2326) + // ============================================================ + console.log(`${colors.yellow}Test Suite 42: --tools flag parsing & validation${colors.reset}\n`); + try { + const { UI } = require('../tools/installer/ui'); + const ui = new UI(); + const known = new Set(['claude-code', 'cursor', 'windsurf']); + + assert( + JSON.stringify(ui._parseToolsFlag('claude-code', known)) === JSON.stringify(['claude-code']), + 'parseToolsFlag returns single ID', + ); + + assert( + JSON.stringify(ui._parseToolsFlag('claude-code,cursor', known)) === JSON.stringify(['claude-code', 'cursor']), + 'parseToolsFlag returns multiple IDs', + ); + + assert( + JSON.stringify(ui._parseToolsFlag(' claude-code , cursor ', known)) === JSON.stringify(['claude-code', 'cursor']), + 'parseToolsFlag trims whitespace', + ); + + let emptyErr; + try { + ui._parseToolsFlag('', known); + } catch (error) { + emptyErr = error; + } + assert( + emptyErr && emptyErr.expected === true && /empty/i.test(emptyErr.message), + 'parseToolsFlag rejects empty string with expected=true', + ); + + let commasOnlyErr; + try { + ui._parseToolsFlag(' , , ', known); + } catch (error) { + commasOnlyErr = error; + } + assert(commasOnlyErr && commasOnlyErr.expected === true, 'parseToolsFlag rejects whitespace/comma-only input'); + + let noneErr; + try { + ui._parseToolsFlag('none', known); + } catch (error) { + noneErr = error; + } + assert(noneErr && noneErr.expected === true && /Unknown tool ID/.test(noneErr.message), 'parseToolsFlag rejects "none" as unknown ID'); + + let typoErr; + try { + ui._parseToolsFlag('claude-code,claude-cdoe', known); + } catch (error) { + typoErr = error; + } + const typoHeader = typoErr ? typoErr.message.split('\n')[0] : ''; + assert( + typoErr && typoErr.expected === true && /claude-cdoe/.test(typoHeader) && !/claude-code/.test(typoHeader), + 'parseToolsFlag reports only the unknown ID in error header (valid ones not listed as unknown)', + ); + + // --list-tools and --tools validation must agree on what counts as a valid ID. + const { formatPlatformList } = require('../tools/installer/ide/platform-codes'); + const { IdeManager } = require('../tools/installer/ide/manager'); + const ideManager42 = new IdeManager(); + await ideManager42.ensureInitialized(); + const validIds = new Set(ideManager42.getAvailableIdes().map((i) => i.value)); + const listed = await formatPlatformList(); + // Each entry line starts with ' *' (preferred) or ' ' (other), followed by the ID, then padding. + const entryLines = listed.split('\n').filter((l) => /^( \*| {2})[a-z]/.test(l)); + const listedIds = entryLines.map((l) => l.trim().replace(/^\*/, '').split(/\s+/)[0]); + const missingFromList = [...validIds].filter((id) => !listedIds.includes(id)); + const extraInList = listedIds.filter((id) => !validIds.has(id)); + assert( + missingFromList.length === 0 && extraInList.length === 0, + '--list-tools output matches the IDs that --tools accepts', + `Missing from list: ${missingFromList.join(',') || '(none)'}; Extra in list: ${extraInList.join(',') || '(none)'}`, + ); + } catch (error) { + console.log(`${colors.red}Test Suite 42 setup failed: ${error.message}${colors.reset}`); + console.log(error.stack); + failed++; + } + + console.log(''); + // ============================================================ // Summary // ============================================================ diff --git a/tools/installer/ide/platform-codes.js b/tools/installer/ide/platform-codes.js index 82560d8bf..6d1aa9180 100644 --- a/tools/installer/ide/platform-codes.js +++ b/tools/installer/ide/platform-codes.js @@ -32,24 +32,24 @@ function clearCache() { } /** - * Format the platform list for human-readable output (used by --list-tools). + * Format the installable platform list for human-readable output (used by --list-tools). + * Sourced from IdeManager so this view matches what --tools accepts at install time + * (suspended platforms excluded). * @returns {Promise} Formatted multi-line string with id, name, target_dir, preferred flag. */ async function formatPlatformList() { - const config = await loadPlatformCodes(); - const platforms = config.platforms || {}; + const { IdeManager } = require('./manager'); + const ideManager = new IdeManager(); + await ideManager.ensureInitialized(); - const entries = Object.entries(platforms).map(([id, p]) => ({ - id, - name: p.name || id, - targetDir: p.installer?.target_dir || '', - preferred: p.preferred === true, - suspended: typeof p.suspended === 'string' ? p.suspended : null, - })); - - entries.sort((a, b) => { - if (a.preferred !== b.preferred) return a.preferred ? -1 : 1; - return a.id.localeCompare(b.id); + const entries = ideManager.getAvailableIdes().map((ide) => { + const handler = ideManager.handlers.get(ide.value); + return { + id: ide.value, + name: ide.name, + targetDir: handler?.installerConfig?.target_dir || '', + preferred: ide.preferred, + }; }); const idWidth = Math.max(...entries.map((e) => e.id.length), 'ID'.length); @@ -65,8 +65,7 @@ async function formatPlatformList() { for (const e of entries) { const star = e.preferred ? ' *' : ' '; - const suffix = e.suspended ? ` [suspended: ${e.suspended}]` : ''; - lines.push(`${star}${pad(e.id, idWidth)} ${pad(e.name, nameWidth)} ${e.targetDir}${suffix}`); + lines.push(`${star}${pad(e.id, idWidth)} ${pad(e.name, nameWidth)} ${e.targetDir}`); } lines.push('', '* = recommended / preferred', '', 'Example: bmad-method install --modules bmm --tools claude-code'); @@ -74,20 +73,8 @@ async function formatPlatformList() { return lines.join('\n'); } -/** - * @returns {Promise} List of valid platform IDs (suspended ones excluded). - */ -async function getValidPlatformIds() { - const config = await loadPlatformCodes(); - const platforms = config.platforms || {}; - return Object.entries(platforms) - .filter(([, p]) => !p.suspended) - .map(([id]) => id); -} - module.exports = { loadPlatformCodes, clearCache, formatPlatformList, - getValidPlatformIds, }; diff --git a/tools/installer/ui.js b/tools/installer/ui.js index 15b18f9a1..904fe7e51 100644 --- a/tools/installer/ui.js +++ b/tools/installer/ui.js @@ -405,7 +405,7 @@ class UI { * @returns {Object} Tool configuration */ _parseToolsFlag(toolsArg, allKnownValues) { - const selectedIdes = String(toolsArg) + const selectedIdes = toolsArg .split(',') .map((t) => t.trim()) .filter(Boolean); @@ -559,20 +559,18 @@ class UI { await this.displaySelectedTools(configuredIdes, preferredIdes, allTools); return { ides: configuredIdes, skipIde: false }; } else { - { - const err = new Error( - [ - '--tools is required for non-interactive install (--yes / -y) when no tools are previously configured.', - '', - 'Common: claude-code, cursor, copilot, windsurf, cline', - 'See all supported tools: bmad-method install --list-tools', - '', - 'Example: bmad-method install --modules bmm --tools claude-code -y', - ].join('\n'), - ); - err.expected = true; - throw err; - } + const err = new Error( + [ + '--tools is required for non-interactive install (--yes / -y) when no tools are previously configured.', + '', + 'Common: claude-code, cursor, copilot, windsurf, cline', + 'See all supported tools: bmad-method install --list-tools', + '', + 'Example: bmad-method install --modules bmm --tools claude-code -y', + ].join('\n'), + ); + err.expected = true; + throw err; } }