fix(cli): resolve non-interactive install defaults for directory and output_folder#1963
Open
jschulte wants to merge 8 commits intobmad-code-org:mainfrom
Open
fix(cli): resolve non-interactive install defaults for directory and output_folder#1963jschulte wants to merge 8 commits intobmad-code-org:mainfrom
jschulte wants to merge 8 commits intobmad-code-org:mainfrom
Conversation
…output_folder
When using --yes with partial CLI options (e.g. --user-name without
--output-folder), the installer had two bugs:
1. Still prompted for directory instead of defaulting to cwd
2. Left {output_folder} unresolved in module configs, creating a literal
"{output_folder}" directory instead of "_bmad-output"
Extract getDefaultCoreConfig() that reads defaults from
src/core/module.yaml (single source of truth) and use it to backfill
missing fields when --yes skips interactive prompts.
📝 WalkthroughWalkthroughThe PR adds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes non-interactive (--yes) install behavior so that missing CLI options don’t trigger prompts and core config placeholders (notably {output_folder}) get sane defaults, preventing accidental literal {output_folder} directories.
Changes:
- Default install directory to
process.cwd()when--yesis used without--directory. - Add default “backfill” for missing core config values when
--yesis used with partial CLI core options. - Extract
getDefaultCoreConfig()helper to read core defaults fromsrc/core/module.yaml.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tools/cli/lib/ui.js
Outdated
Comment on lines
+849
to
+859
| * 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() { |
tools/cli/lib/ui.js
Outdated
| // 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]) { |
Comment on lines
938
to
947
| @@ -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)'); | |||
| } | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/cli/lib/ui.js (2)
938-947:⚠️ Potential issue | 🟠 Major
--yespath fails to repair partially populated existing core config.This branch only applies defaults when the object is fully empty. Existing installs missing one required key remain broken.
Proposed fix
- // If no existing config, use defaults - if (Object.keys(existingConfig).length === 0) { - configCollector.collectedConfig.core = this.getDefaultCoreConfig(); - await prompts.log.info('Using default configuration (--yes flag)'); - } + const defaults = this.getDefaultCoreConfig(); + const merged = { ...defaults, ...existingConfig }; + configCollector.collectedConfig.core = merged; + if (Object.keys(existingConfig).length === 0) { + await prompts.log.info('Using default configuration (--yes flag)'); + }🤖 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 938 - 947, The --yes branch currently only replaces core config when collectedConfig.core is completely empty, leaving partially populated core configs with missing required keys; after calling configCollector.loadExistingConfig(directory) you should merge defaults from this.getDefaultCoreConfig() into configCollector.collectedConfig.core by filling in any missing keys (rather than only when Object.keys(...) === 0), and call prompts.log.info('Using default configuration (--yes flag)') when any defaults are applied; update the logic around options.yes, configCollector.collectedConfig.core, and this.getDefaultCoreConfig() to perform a shallow/defaults merge so partially populated configs are repaired.
938-947:⚠️ Potential issue | 🟠 MajorExisting-config
--yesbranch also needs unresolved-token cleanup.Even with non-empty existing config, placeholder literals from prior bad installs (for example
"{output_folder}") should be normalized to defaults before returning.🤖 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 938 - 947, The --yes branch currently skips cleanup when an existing core config is non-empty, allowing placeholder literals like "{output_folder}" to persist; modify the branch after loading existingConfig (the variable populated by configCollector.loadExistingConfig) to iterate core config keys and replace any unresolved-token placeholders (e.g. values matching /^\{.*\}$/) with the corresponding values from this.getDefaultCoreConfig(), updating configCollector.collectedConfig.core accordingly (or add a helper like normalizeUnresolvedTokens(coreConfig, defaultCoreConfig) and call it), and optionally log that placeholders were normalized before returning.
🧹 Nitpick comments (2)
tools/cli/lib/ui.js (2)
859-890: Memoize core defaults instead of reparsing YAML repeatedly.
getDefaultCoreConfig()does sync disk I/O every call. Cache once per process to avoid repeated file reads in multi-step install/update flows.🤖 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 859 - 890, getDefaultCoreConfig currently re-reads and parses core/module.yaml on every call; add a module-scoped cache (e.g., let cachedCoreConfig = null) and have getDefaultCoreConfig return the cached object when present. On first call construct the config exactly as now (using getModulePath, moduleYamlPath, yaml.parse, defaultUsername logic) and assign it to the cache; keep the existing try/catch fallback behavior so errors still produce the same defaults before caching. Ensure subsequent calls simply return cachedCoreConfig without re-reading the file.
859-890: Default centralization is incomplete across the install/render pipeline.You centralized defaults here, but
tools/cli/installers/lib/custom/handler.jsstill hardcodes fallback values ('User','English','docs') at Line 256-260 and Line 332-336, so drift risk remains.🤖 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 859 - 890, Replace the hardcoded fallback defaults in the custom installer handler with the centralized defaults from getDefaultCoreConfig: import/require getDefaultCoreConfig from the module that defines it (the UI helper) and call it where the handler currently sets 'User', 'English', 'docs' (the fallback user_name, communication_language, document_output_language/output_folder values). Use the returned object values (user_name, communication_language, document_output_language, output_folder) instead of the literal strings so the installer uses the same single-source-of-truth defaults as getDefaultCoreConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/cli/lib/ui.js`:
- Around line 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.
- Around line 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.
- Around line 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.
- Around line 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.
- Around line 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.
- Around line 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.
---
Outside diff comments:
In `@tools/cli/lib/ui.js`:
- Around line 938-947: The --yes branch currently only replaces core config when
collectedConfig.core is completely empty, leaving partially populated core
configs with missing required keys; after calling
configCollector.loadExistingConfig(directory) you should merge defaults from
this.getDefaultCoreConfig() into configCollector.collectedConfig.core by filling
in any missing keys (rather than only when Object.keys(...) === 0), and call
prompts.log.info('Using default configuration (--yes flag)') when any defaults
are applied; update the logic around options.yes,
configCollector.collectedConfig.core, and this.getDefaultCoreConfig() to perform
a shallow/defaults merge so partially populated configs are repaired.
- Around line 938-947: The --yes branch currently skips cleanup when an existing
core config is non-empty, allowing placeholder literals like "{output_folder}"
to persist; modify the branch after loading existingConfig (the variable
populated by configCollector.loadExistingConfig) to iterate core config keys and
replace any unresolved-token placeholders (e.g. values matching /^\{.*\}$/) with
the corresponding values from this.getDefaultCoreConfig(), updating
configCollector.collectedConfig.core accordingly (or add a helper like
normalizeUnresolvedTokens(coreConfig, defaultCoreConfig) and call it), and
optionally log that placeholders were normalized before returning.
---
Nitpick comments:
In `@tools/cli/lib/ui.js`:
- Around line 859-890: getDefaultCoreConfig currently re-reads and parses
core/module.yaml on every call; add a module-scoped cache (e.g., let
cachedCoreConfig = null) and have getDefaultCoreConfig return the cached object
when present. On first call construct the config exactly as now (using
getModulePath, moduleYamlPath, yaml.parse, defaultUsername logic) and assign it
to the cache; keep the existing try/catch fallback behavior so errors still
produce the same defaults before caching. Ensure subsequent calls simply return
cachedCoreConfig without re-reading the file.
- Around line 859-890: Replace the hardcoded fallback defaults in the custom
installer handler with the centralized defaults from getDefaultCoreConfig:
import/require getDefaultCoreConfig from the module that defines it (the UI
helper) and call it where the handler currently sets 'User', 'English', 'docs'
(the fallback user_name, communication_language,
document_output_language/output_folder values). Use the returned object values
(user_name, communication_language, document_output_language, output_folder)
instead of the literal strings so the installer uses the same
single-source-of-truth defaults as getDefaultCoreConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: edee3e8a-87dc-4ad7-b8a2-70d43202f999
📒 Files selected for processing (1)
tools/cli/lib/ui.js
Comment on lines
+50
to
+53
| } 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}`); |
There was a problem hiding this comment.
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.
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.
tools/cli/lib/ui.js
Outdated
Comment on lines
+855
to
+880
| /** | ||
| * 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', | ||
| }; |
There was a problem hiding this comment.
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.
tools/cli/lib/ui.js
Outdated
Comment on lines
+873
to
+880
| 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', | ||
| }; |
There was a problem hiding this comment.
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.
tools/cli/lib/ui.js
Outdated
Comment on lines
+881
to
+889
| } catch { | ||
| // Fallback if module.yaml is unreadable | ||
| return { | ||
| user_name: defaultUsername, | ||
| communication_language: 'English', | ||
| document_output_language: 'English', | ||
| output_folder: '_bmad-output', | ||
| }; | ||
| } |
There was a problem hiding this comment.
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.
Comment on lines
+929
to
+936
| } 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
…-install-defaults
…hulte/BMAD-METHOD into fix/non-interactive-install-defaults
…ults
- 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
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes non-interactive (--yes) install behavior in the CLI so partial option sets don’t trigger prompts and core config placeholders (notably {output_folder}) are correctly defaulted.
Changes:
- Default install directory to
process.cwd()when--yesis used without--directory. - Backfill missing/unresolved core config values when
--yesskips interactive prompts. - Add
getDefaultCoreConfig()to derive core defaults fromsrc/core/module.yamlas the source of truth.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tools/cli/lib/ui.js
Outdated
| output_folder: norm(moduleConfig.output_folder?.default, '_bmad-output'), | ||
| }; | ||
| } catch (error) { | ||
| console.warn(`Failed to load module.yaml, falling back to defaults: ${error.message}`); |
tools/cli/lib/ui.js
Outdated
Comment on lines
+937
to
+940
| // 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(); |
Collaborator
|
@jschulte Thanks for this - there are quite a few CR comments still. |
… logic - Extract getDefaultCoreConfig, applyDefaultCoreConfig, and isMissingOrUnresolvedCoreConfigValue into core-config-defaults.js - Memoize YAML parsing with clearable cache for testing - Use prompts.log.warn instead of console.warn for error logging - Guard process.cwd() with try/catch in --yes path - Centralize handler.js placeholder replacement via shared defaults - Add tests for backfill logic and custom handler shared defaults
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fix two bugs in non-interactive (
--yes) install when partial CLI options are provided.Why
When using
--yeswith--user-namebut without--output-folder, the installer (1) still prompted for the directory and (2) left{output_folder}unresolved, creating a literal{output_folder}directory.Fixes #1962
How
else if (options.yes)branch to default directory toprocess.cwd()when no--directoryis given--yesskips interactive prompts but partial CLI options are providedgetDefaultCoreConfig()helper that reads defaults fromsrc/core/module.yaml(single source of truth), eliminating hardcoded duplicatesTesting
npm test— schemas, refs, install components, lint, format)node tools/cli/bmad-cli.js install --modules bmm --tools claude-code,codex --user-name "Jonah" --yes— installs to cwd without prompting, creates_bmad-outputcorrectly