fix(installer): address review for #2326 — single source of truth, drop dead code, add tests

- 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.
This commit is contained in:
Brian Madison 2026-04-27 22:51:15 -05:00
parent c5486f6aa5
commit bbdefb4269
3 changed files with 116 additions and 43 deletions

View File

@ -2773,6 +2773,94 @@ async function runTests() {
console.log(''); 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 // Summary
// ============================================================ // ============================================================

View File

@ -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<string>} Formatted multi-line string with id, name, target_dir, preferred flag. * @returns {Promise<string>} Formatted multi-line string with id, name, target_dir, preferred flag.
*/ */
async function formatPlatformList() { async function formatPlatformList() {
const config = await loadPlatformCodes(); const { IdeManager } = require('./manager');
const platforms = config.platforms || {}; const ideManager = new IdeManager();
await ideManager.ensureInitialized();
const entries = Object.entries(platforms).map(([id, p]) => ({ const entries = ideManager.getAvailableIdes().map((ide) => {
id, const handler = ideManager.handlers.get(ide.value);
name: p.name || id, return {
targetDir: p.installer?.target_dir || '', id: ide.value,
preferred: p.preferred === true, name: ide.name,
suspended: typeof p.suspended === 'string' ? p.suspended : null, targetDir: handler?.installerConfig?.target_dir || '',
})); preferred: ide.preferred,
};
entries.sort((a, b) => {
if (a.preferred !== b.preferred) return a.preferred ? -1 : 1;
return a.id.localeCompare(b.id);
}); });
const idWidth = Math.max(...entries.map((e) => e.id.length), 'ID'.length); const idWidth = Math.max(...entries.map((e) => e.id.length), 'ID'.length);
@ -65,8 +65,7 @@ async function formatPlatformList() {
for (const e of entries) { for (const e of entries) {
const star = e.preferred ? ' *' : ' '; const star = e.preferred ? ' *' : ' ';
const suffix = e.suspended ? ` [suspended: ${e.suspended}]` : ''; lines.push(`${star}${pad(e.id, idWidth)} ${pad(e.name, nameWidth)} ${e.targetDir}`);
lines.push(`${star}${pad(e.id, idWidth)} ${pad(e.name, nameWidth)} ${e.targetDir}${suffix}`);
} }
lines.push('', '* = recommended / preferred', '', 'Example: bmad-method install --modules bmm --tools claude-code'); lines.push('', '* = recommended / preferred', '', 'Example: bmad-method install --modules bmm --tools claude-code');
@ -74,20 +73,8 @@ async function formatPlatformList() {
return lines.join('\n'); return lines.join('\n');
} }
/**
* @returns {Promise<string[]>} 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 = { module.exports = {
loadPlatformCodes, loadPlatformCodes,
clearCache, clearCache,
formatPlatformList, formatPlatformList,
getValidPlatformIds,
}; };

View File

@ -405,7 +405,7 @@ class UI {
* @returns {Object} Tool configuration * @returns {Object} Tool configuration
*/ */
_parseToolsFlag(toolsArg, allKnownValues) { _parseToolsFlag(toolsArg, allKnownValues) {
const selectedIdes = String(toolsArg) const selectedIdes = toolsArg
.split(',') .split(',')
.map((t) => t.trim()) .map((t) => t.trim())
.filter(Boolean); .filter(Boolean);
@ -559,7 +559,6 @@ class UI {
await this.displaySelectedTools(configuredIdes, preferredIdes, allTools); await this.displaySelectedTools(configuredIdes, preferredIdes, allTools);
return { ides: configuredIdes, skipIde: false }; return { ides: configuredIdes, skipIde: false };
} else { } else {
{
const err = new Error( const err = new Error(
[ [
'--tools is required for non-interactive install (--yes / -y) when no tools are previously configured.', '--tools is required for non-interactive install (--yes / -y) when no tools are previously configured.',
@ -574,7 +573,6 @@ class UI {
throw err; throw err;
} }
} }
}
// Interactive mode // Interactive mode
const interactiveSelectedIdes = await prompts.autocompleteMultiselect({ const interactiveSelectedIdes = await prompts.autocompleteMultiselect({