Skip to content

fix(cli): resolve non-interactive install defaults for directory and output_folder #1963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jschulte wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix(cli): resolve non-interactive install defaults for directory and output_folder #1963

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1cdc7f4
fix(cli): resolve non-interactive install defaults for directory and …
jschulte Mar 13, 2026
fe2cbe9
Merge branch 'main' into fix/non-interactive-install-defaults
jschulte Mar 13, 2026
ac04378
Merge remote-tracking branch 'upstream/main' into fix/non-interactive…
jschulte Mar 13, 2026
d2e7158
Merge branch 'fix/non-interactive-install-defaults' of github.com:jsc…
jschulte Mar 13, 2026
7422d3a
fix(cli): address PR review feedback for non-interactive install defa…
jschulte Mar 13, 2026
5c0dfd8
Merge branch 'main' into fix/non-interactive-install-defaults
jschulte Mar 14, 2026
1571a3c
Merge branch 'main' into fix/non-interactive-install-defaults
jschulte Mar 25, 2026
8927e20
fix(cli): extract core-config-defaults module and centralize fallback…
jschulte Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 50 additions & 13 deletions tools/cli/lib/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ class UI {
}
confirmedDirectory = expandedDir;
await prompts.log.info(`Using directory from command-line: ${confirmedDirectory}`);
} else if (options.yes) {
// Default to current directory when --yes flag is set
confirmedDirectory = process.cwd();
await prompts.log.info(`Using current directory (--yes flag): ${confirmedDirectory}`);
Comment on lines +51 to +65
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard process.cwd() in non-interactive mode.

If the working directory was removed between process start and this call, process.cwd() can throw and crash this path without installer-level context.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/cli/lib/ui.js` around lines 50 - 53, The call to process.cwd() in the
branch handling options.yes can throw if the current working directory was
removed; wrap the process.cwd() access in a try/catch around the
confirmedDirectory assignment in the same block (the code that sets
confirmedDirectory when options.yes is true), and on error handle it gracefully
by logging an error via prompts.log.error (or returning a clear failure) and
using a safe fallback (e.g., fallback to '.' or abort the installer) instead of
letting the exception crash the process; ensure the log message includes the
caught error details.

⚠️ Potential issue | 🟠 Major

Validate the --yes default directory before proceeding.

This branch skips validateDirectorySync, so non-writable or invalid cwd fails later with less actionable errors.

Proposed fix 
-    } else if (options.yes) {
-      // Default to current directory when --yes flag is set
-      confirmedDirectory = process.cwd();
-      await prompts.log.info(`Using current directory (--yes flag): ${confirmedDirectory}`);
+    } else if (options.yes) {
+      // Default to current directory when --yes flag is set
+      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}`);
     } else {
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/cli/lib/ui.js` around lines 50 - 53, The branch handling options.yes
should validate the chosen default directory before proceeding: after setting
confirmedDirectory = process.cwd() (in the options.yes branch of the CLI flow),
call validateDirectorySync(confirmedDirectory) and handle a validation failure
the same way other branches do (log the error via prompts.log.error or
processLogger and abort/exit). Ensure you reuse the same validation function
(validateDirectorySync) and error handling path so non-writable or invalid cwd
is detected early, and keep the existing informational log
prompts.log.info(`Using current directory (--yes flag): ${confirmedDirectory}`)
only after validation succeeds.

} else {
confirmedDirectory = await this.getConfirmedDirectory();
}
Expand Down Expand Up @@ -848,6 +852,43 @@ class UI {
* @param {Object} options - Command-line options
* @returns {Object} Core configuration
*/
/**
* 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
*/
getDefaultCoreConfig() {
const { getModulePath } = require('./project-root');
const yaml = require('yaml');

let safeUsername;
try {
safeUsername = os.userInfo().username;
} catch {
safeUsername = process.env.USER || process.env.USERNAME || 'User';
}
const defaultUsername = safeUsername.charAt(0).toUpperCase() + safeUsername.slice(1);

// Read defaults from core module.yaml (single source of truth)
try {
const moduleYamlPath = path.join(getModulePath('core'), 'module.yaml');
const moduleConfig = yaml.parse(fs.readFileSync(moduleYamlPath, 'utf8'));
return {
user_name: defaultUsername,
communication_language: moduleConfig.communication_language?.default || 'English',
document_output_language: moduleConfig.document_output_language?.default || 'English',
output_folder: moduleConfig.output_folder?.default || '_bmad-output',
};
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

getDefaultCoreConfig() is not fully module.yaml-driven for user_name.

The method claims module.yaml-backed defaults, but user_name is hardcoded from OS/env. That reintroduces drift against src/core/module.yaml.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/cli/lib/ui.js` around lines 855 - 880, getDefaultCoreConfig currently
hardcodes user_name from the OS/env instead of using the core module.yaml;
update the function to read moduleConfig.user_name?.default (from the same
moduleYamlPath/moduleConfig used for communication_language,
document_output_language, output_folder) and only fall back to the existing
safeUsername/capitalization logic when that YAML default is missing or empty,
ensuring the returned object sets user_name from moduleConfig.user_name?.default
with the same capitalization behavior as before.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Harden parsed defaults before writing them into config.

Values are accepted from YAML without type/empty checks. Non-string or blank defaults can leak into core config and break path/template replacement downstream.

Proposed fix 
-      return {
-        user_name: defaultUsername,
-        communication_language: moduleConfig.communication_language?.default || 'English',
-        document_output_language: moduleConfig.document_output_language?.default || 'English',
-        output_folder: moduleConfig.output_folder?.default || '_bmad-output',
-      };
+      const norm = (value, fallback) =>
+        typeof value === 'string' && value.trim() !== '' ? value.trim() : fallback;
+      return {
+        user_name: defaultUsername,
+        communication_language: norm(moduleConfig.communication_language?.default, 'English'),
+        document_output_language: norm(moduleConfig.document_output_language?.default, 'English'),
+        output_folder: norm(moduleConfig.output_folder?.default, '_bmad-output'),
+      };
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/cli/lib/ui.js` around lines 873 - 880, The parsed YAML defaults
(moduleConfig.communication_language?.default,
moduleConfig.document_output_language?.default,
moduleConfig.output_folder?.default) are used without type or empty-string
checks and can inject invalid values; update the return construction in ui.js to
validate and coerce each default to a non-empty string before using it (e.g.,
read moduleConfig values, if value is not a string or trims to empty, fall back
to the literal defaults 'English' / '_bmad-output' or defaultUsername for
user_name), reference moduleYamlPath/getModulePath and the moduleConfig
properties to find the code, and ensure the final returned properties are safe
strings suitable for downstream path/template replacement.

} catch {
// Fallback if module.yaml is unreadable
return {
user_name: defaultUsername,
communication_language: 'English',
document_output_language: 'English',
output_folder: '_bmad-output',
};
}
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Do not silently swallow default-loading failures.

A blanket catch {} masks packaging/path/yaml regressions and quietly changes behavior. Emit a warning with the caught error so broken builds are diagnosable.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/cli/lib/ui.js` around lines 881 - 889, The empty catch around the
module.yaml fallback hides real errors; change the anonymous catch in
tools/cli/lib/ui.js to capture the exception (e.g., catch (err)) and emit a
warning that includes the error before returning the defaults (referencing the
same defaultUsername and the existing return object). Use the project's logging
facility if available (e.g., logger.warn or processLogger.warn), otherwise
console.warn, and include context like "Failed to load module.yaml, falling back
to defaults" plus the error details so packaging/path/yaml regressions are
visible.

}

async collectCoreConfig(directory, options = {}) {
const { ConfigCollector } = require('../installers/lib/core/config-collector');
const configCollector = new ConfigCollector();
Expand Down Expand Up @@ -885,6 +926,14 @@ class UI {
(!options.userName || !options.communicationLanguage || !options.documentOutputLanguage || !options.outputFolder)
) {
await configCollector.collectModuleConfig('core', directory, false, true);
} else if (options.yes) {
// Fill in defaults for any fields not provided via command-line or existing config
const defaults = this.getDefaultCoreConfig();
for (const [key, value] of Object.entries(defaults)) {
if (!configCollector.collectedConfig.core[key]) {
configCollector.collectedConfig.core[key] = value;
}
}
Comment on lines +893 to +899
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Backfill predicate still lets unresolved placeholders survive.

if (!configCollector.collectedConfig.core[key]) treats "{output_folder}" as valid (truthy), preserving broken placeholder values in --yes runs.

Proposed fix 
-        for (const [key, value] of Object.entries(defaults)) {
-          if (!configCollector.collectedConfig.core[key]) {
-            configCollector.collectedConfig.core[key] = value;
-          }
-        }
+        const isMissingOrUnresolved = (v) =>
+          v == null ||
+          (typeof v === 'string' && (v.trim() === '' || /^\{[^}]+\}$/.test(v.trim())));
+
+        for (const [key, value] of Object.entries(defaults)) {
+          if (isMissingOrUnresolved(configCollector.collectedConfig.core[key])) {
+            configCollector.collectedConfig.core[key] = value;
+          }
+        }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/cli/lib/ui.js` around lines 929 - 936, The backfill loop inside the
options.yes branch (using getDefaultCoreConfig and
configCollector.collectedConfig.core) treats placeholder strings like
"{output_folder}" as present; change the condition so it also considers
unresolved placeholders as missing — e.g., detect placeholder patterns (strings
matching /^\{.*\}$/ or containing un-replaced tokens) or add an isPlaceholder
helper and use it in the check (if value is falsy OR isPlaceholder(value))
before assigning the default for that key.

}
} else if (options.yes) {
// Use all defaults when --yes flag is set
Expand All @@ -893,19 +942,7 @@ class UI {

// If no existing config, use defaults
if (Object.keys(existingConfig).length === 0) {
let safeUsername;
try {
safeUsername = os.userInfo().username;
} catch {
safeUsername = process.env.USER || process.env.USERNAME || 'User';
}
const defaultUsername = safeUsername.charAt(0).toUpperCase() + safeUsername.slice(1);
configCollector.collectedConfig.core = {
user_name: defaultUsername,
communication_language: 'English',
document_output_language: 'English',
output_folder: '_bmad-output',
};
configCollector.collectedConfig.core = this.getDefaultCoreConfig();
await prompts.log.info('Using default configuration (--yes flag)');
}
} else {
Expand Down