fix(cli): address PR review feedback for non-interactive install defaults

- Validate cwd with validateDirectorySync in --yes path
- Fix JSDoc placement: move collectCoreConfig docs above its method
- Read user_name default from module.yaml, fall back to OS username
- Harden YAML defaults with type/empty-string normalization
- Log warning on module.yaml load failure instead of silent catch
- Fix backfill predicate to detect unresolved placeholders like {output_folder}
- Always merge defaults into existing config in --yes mode
This commit is contained in:
Jonah Schulte 2026-03-13 16:53:45 -04:00
parent d2e7158c72
commit 7422d3a0a3
1 changed files with 36 additions and 19 deletions

View File

@ -49,7 +49,12 @@ class UI {
await prompts.log.info(`Using directory from command-line: ${confirmedDirectory}`); await prompts.log.info(`Using directory from command-line: ${confirmedDirectory}`);
} else if (options.yes) { } else if (options.yes) {
// Default to current directory when --yes flag is set // Default to current directory when --yes flag is set
confirmedDirectory = process.cwd(); const cwd = process.cwd();
const validation = this.validateDirectorySync(cwd);
if (validation) {
throw new Error(`Invalid current directory: ${validation}`);
}
confirmedDirectory = cwd;
await prompts.log.info(`Using current directory (--yes flag): ${confirmedDirectory}`); await prompts.log.info(`Using current directory (--yes flag): ${confirmedDirectory}`);
} else { } else {
confirmedDirectory = await this.getConfirmedDirectory(); confirmedDirectory = await this.getConfirmedDirectory();
@ -846,12 +851,6 @@ class UI {
return { existingInstall, installedModuleIds, bmadDir }; return { existingInstall, installedModuleIds, bmadDir };
} }
/**
* Collect core configuration
* @param {string} directory - Installation directory
* @param {Object} options - Command-line options
* @returns {Object} Core configuration
*/
/** /**
* Get default core config values by reading from src/core/module.yaml * Get default core config values by reading from src/core/module.yaml
* @returns {Object} Default core config with user_name, communication_language, document_output_language, output_folder * @returns {Object} Default core config with user_name, communication_language, document_output_language, output_folder
@ -866,22 +865,24 @@ class UI {
} catch { } catch {
safeUsername = process.env.USER || process.env.USERNAME || 'User'; safeUsername = process.env.USER || process.env.USERNAME || 'User';
} }
const defaultUsername = safeUsername.charAt(0).toUpperCase() + safeUsername.slice(1); const osUsername = safeUsername.charAt(0).toUpperCase() + safeUsername.slice(1);
const norm = (value, fallback) => (typeof value === 'string' && value.trim() !== '' ? value.trim() : fallback);
// Read defaults from core module.yaml (single source of truth) // Read defaults from core module.yaml (single source of truth)
try { try {
const moduleYamlPath = path.join(getModulePath('core'), 'module.yaml'); const moduleYamlPath = path.join(getModulePath('core'), 'module.yaml');
const moduleConfig = yaml.parse(fs.readFileSync(moduleYamlPath, 'utf8')); const moduleConfig = yaml.parse(fs.readFileSync(moduleYamlPath, 'utf8'));
return { return {
user_name: defaultUsername, user_name: norm(moduleConfig.user_name?.default, osUsername),
communication_language: moduleConfig.communication_language?.default || 'English', communication_language: norm(moduleConfig.communication_language?.default, 'English'),
document_output_language: moduleConfig.document_output_language?.default || 'English', document_output_language: norm(moduleConfig.document_output_language?.default, 'English'),
output_folder: moduleConfig.output_folder?.default || '_bmad-output', output_folder: norm(moduleConfig.output_folder?.default, '_bmad-output'),
}; };
} catch { } catch (error) {
// Fallback if module.yaml is unreadable console.warn(`Failed to load module.yaml, falling back to defaults: ${error.message}`);
return { return {
user_name: defaultUsername, user_name: osUsername,
communication_language: 'English', communication_language: 'English',
document_output_language: 'English', document_output_language: 'English',
output_folder: '_bmad-output', output_folder: '_bmad-output',
@ -889,6 +890,12 @@ class UI {
} }
} }
/**
* Collect core configuration
* @param {string} directory - Installation directory
* @param {Object} options - Command-line options
* @returns {Object} Core configuration
*/
async collectCoreConfig(directory, options = {}) { async collectCoreConfig(directory, options = {}) {
const { ConfigCollector } = require('../installers/lib/core/config-collector'); const { ConfigCollector } = require('../installers/lib/core/config-collector');
const configCollector = new ConfigCollector(); const configCollector = new ConfigCollector();
@ -928,21 +935,31 @@ class UI {
await configCollector.collectModuleConfig('core', directory, false, true); await configCollector.collectModuleConfig('core', directory, false, true);
} else if (options.yes) { } else if (options.yes) {
// Fill in defaults for any fields not provided via command-line or existing config // Fill in defaults for any fields not provided via command-line or existing config
const isMissingOrUnresolved = (v) => v == null || (typeof v === 'string' && (v.trim() === '' || /^\{[^}]+\}$/.test(v.trim())));
const defaults = this.getDefaultCoreConfig(); const defaults = this.getDefaultCoreConfig();
for (const [key, value] of Object.entries(defaults)) { for (const [key, value] of Object.entries(defaults)) {
if (!configCollector.collectedConfig.core[key]) { if (isMissingOrUnresolved(configCollector.collectedConfig.core[key])) {
configCollector.collectedConfig.core[key] = value; configCollector.collectedConfig.core[key] = value;
} }
} }
} }
} else if (options.yes) { } else if (options.yes) {
// Use all defaults when --yes flag is set // Use all defaults when --yes flag is set, merging with any existing config
await configCollector.loadExistingConfig(directory); await configCollector.loadExistingConfig(directory);
const existingConfig = configCollector.collectedConfig.core || {}; const existingConfig = configCollector.collectedConfig.core || {};
const defaults = this.getDefaultCoreConfig();
configCollector.collectedConfig.core = { ...defaults, ...existingConfig };
// Clean up any unresolved placeholder tokens from existing config
const isMissingOrUnresolved = (v) => v == null || (typeof v === 'string' && (v.trim() === '' || /^\{[^}]+\}$/.test(v.trim())));
for (const [key, value] of Object.entries(configCollector.collectedConfig.core)) {
if (isMissingOrUnresolved(value)) {
configCollector.collectedConfig.core[key] = defaults[key];
}
}
// If no existing config, use defaults
if (Object.keys(existingConfig).length === 0) { if (Object.keys(existingConfig).length === 0) {
configCollector.collectedConfig.core = this.getDefaultCoreConfig();
await prompts.log.info('Using default configuration (--yes flag)'); await prompts.log.info('Using default configuration (--yes flag)');
} }
} else { } else {