fix: ensure --yes flag assigns defaults for all config fields#1907
Open
nikolasdehor wants to merge 1 commit intobmad-code-org:mainfrom
Open
fix: ensure --yes flag assigns defaults for all config fields#1907nikolasdehor wants to merge 1 commit intobmad-code-org:mainfrom
nikolasdehor wants to merge 1 commit intobmad-code-org:mainfrom
Conversation
When skipPrompts is true, config fields without a default value were silently skipped, leaving them undefined in the collected config. This caused downstream code to prompt the user or fail when encountering missing config values. Now assigns empty string as fallback for fields without defaults when --yes flag is active, ensuring fully non-interactive install. Fixes bmad-code-org#1803
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tools/cli/installers/lib/core/config-collector.js (1)
736-745: Pull this into a shared default resolver before the branches drift further.Lines 736-745 and 767-785 now contain two copies of “accept defaults” behavior with different edge-case handling. A small helper would keep
--yes, Express Setup, and per-module default acceptance consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/config-collector.js` around lines 736 - 745, Extract the duplicated “accept defaults” logic into a shared helper (e.g., resolveDefaultAnswers or collectDefaultAnswers) and replace both branches that currently repeat the code ("if (this.skipPrompts) { ... }" and the later similar block) to call this helper; the helper should accept (questions, allAnswers, prompts/logging context) and implement the same rules: skip questions where question.default is a function, treat hasDefault as question.default !== undefined && question.default !== null && question.default !== '', and assign question.default when present or '' when not, while preserving the prompts.log.info message (e.g., "Using default configuration for ${moduleDisplayName}") so --yes/Express Setup/per-module paths use identical behavior.
🤖 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/installers/lib/core/config-collector.js`:
- Around line 743-744: The current headless fallback always serializes an empty
string into allAnswers[question.name] which breaks non-input prompts and
bypasses validation; update the headless branch that uses question.default to
instead use a type-aware fallback based on question.type (e.g., for 'confirm'
set false, for 'checkbox' set [], for 'number' set null or a numeric sentinel,
for 'list' use null/empty array as appropriate) and preserve question.default
when present; additionally, when in headless mode and a prompt is required
(question.required or validation present) but has no explicit default, throw a
clear error (fail fast) rather than inserting a generic '' so validations still
run correctly — change the logic around
question.default/allAnswers[question.name] to inspect question.type and
question.required and either assign the type-appropriate value or raise an
error.
- Around line 743-744: The code writes an empty string for unanswered questions
which downstream truthiness checks treat as "missing", allowing placeholder
leaks; in the block using allAnswers[question.name] and hasDefault, change the
fallback from '' to undefined (or omit the key) so that missing answers are
represented as undefined/null rather than an empty string; update the assignment
in the config-collector logic (the hasDefault check and the
allAnswers[question.name] assignment) to use question.default when present and
otherwise leave the property undefined or delete it instead of setting ''.
- Around line 740-741: The code currently skips questions whose default is a
function by executing "continue" when it sees typeof question.default ===
'function', which prevents computed defaults from being applied in
non-interactive (--yes) mode; instead, call the default function with the
current answers and assign its return value into the answers map (the same way
done earlier where question.default(answers) is used), and only skip if the
computed default is explicitly undefined; update the branch around
question.default to compute const value = await question.default(answers)
(handle Promise) and set answers[question.name] = value when applicable so
computed defaults are preserved in --yes mode.
---
Nitpick comments:
In `@tools/cli/installers/lib/core/config-collector.js`:
- Around line 736-745: Extract the duplicated “accept defaults” logic into a
shared helper (e.g., resolveDefaultAnswers or collectDefaultAnswers) and replace
both branches that currently repeat the code ("if (this.skipPrompts) { ... }"
and the later similar block) to call this helper; the helper should accept
(questions, allAnswers, prompts/logging context) and implement the same rules:
skip questions where question.default is a function, treat hasDefault as
question.default !== undefined && question.default !== null && question.default
!== '', and assign question.default when present or '' when not, while
preserving the prompts.log.info message (e.g., "Using default configuration for
${moduleDisplayName}") so --yes/Express Setup/per-module paths use identical
behavior.
🪄 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: 6e691077-dfe6-446b-9d85-18e254417ab9
📒 Files selected for processing (1)
tools/cli/installers/lib/core/config-collector.js
Comment on lines
+740
to
+741
| if (typeof question.default === 'function') { | ||
| continue; |
There was a problem hiding this comment.
Evaluate computed defaults here instead of dropping them.
Line 1151 already turns same-module defaults into question.default(answers). continue here skips those fields entirely, so --yes can still leave computed defaults unset.
Suggested fix
for (const question of questions) {
- if (typeof question.default === 'function') {
- continue;
- }
- const hasDefault = question.default !== undefined && question.default !== null && question.default !== '';
- allAnswers[question.name] = hasDefault ? question.default : '';
+ const resolvedDefault =
+ typeof question.default === 'function'
+ ? question.default(allAnswers)
+ : question.default;
+ const hasDefault =
+ resolvedDefault !== undefined &&
+ resolvedDefault !== null &&
+ resolvedDefault !== '';
+ allAnswers[question.name] = hasDefault ? resolvedDefault : '';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/cli/installers/lib/core/config-collector.js` around lines 740 - 741,
The code currently skips questions whose default is a function by executing
"continue" when it sees typeof question.default === 'function', which prevents
computed defaults from being applied in non-interactive (--yes) mode; instead,
call the default function with the current answers and assign its return value
into the answers map (the same way done earlier where question.default(answers)
is used), and only skip if the computed default is explicitly undefined; update
the branch around question.default to compute const value = await
question.default(answers) (handle Promise) and set answers[question.name] =
value when applicable so computed defaults are preserved in --yes mode.
Comment on lines
+743
to
+744
| const hasDefault = question.default !== undefined && question.default !== null && question.default !== ''; | ||
| allAnswers[question.name] = hasDefault ? question.default : ''; |
There was a problem hiding this comment.
Don't serialize '' for every prompt that lacks an explicit default.
This bypasses the prompt's validation rules and produces the wrong shape for non-input questions like confirm and checkbox. In headless mode, either use a type-aware fallback or fail fast when a required prompt has no explicit default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/cli/installers/lib/core/config-collector.js` around lines 743 - 744,
The current headless fallback always serializes an empty string into
allAnswers[question.name] which breaks non-input prompts and bypasses
validation; update the headless branch that uses question.default to instead use
a type-aware fallback based on question.type (e.g., for 'confirm' set false, for
'checkbox' set [], for 'number' set null or a numeric sentinel, for 'list' use
null/empty array as appropriate) and preserve question.default when present;
additionally, when in headless mode and a prompt is required (question.required
or validation present) but has no explicit default, throw a clear error (fail
fast) rather than inserting a generic '' so validations still run correctly —
change the logic around question.default/allAnswers[question.name] to inspect
question.type and question.required and either assign the type-appropriate value
or raise an error.
'' is not a real fix if downstream lookup still treats it as “missing”.
Lines 596-618 and 958-980 resolve placeholders with truthiness checks. A blank written here will be ignored by that code, so dependent {field} references can still leak into rendered config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/cli/installers/lib/core/config-collector.js` around lines 743 - 744,
The code writes an empty string for unanswered questions which downstream
truthiness checks treat as "missing", allowing placeholder leaks; in the block
using allAnswers[question.name] and hasDefault, change the fallback from '' to
undefined (or omit the key) so that missing answers are represented as
undefined/null rather than an empty string; update the assignment in the
config-collector logic (the hasDefault check and the allAnswers[question.name]
assignment) to use question.default when present and otherwise leave the
property undefined or delete it instead of setting ''.
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
1 participant
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.
Summary
Fixes #1803
When running
bmad install --yes, the config collector skips interactive prompts but only assigns values for fields that have explicit defaults. Fields without defaults are silently skipped, leavingundefinedvalues that can cause downstream template rendering failures (e.g.,{{name}}placeholders left unresolved).This change ensures that in
skipPromptsmode, fields without defaults are assigned an empty string fallback, preventing undefined values from propagating through the installer pipeline.Changes
tools/cli/installers/lib/core/config-collector.js: UpdatedskipPromptsbranch to assign empty string for fields without explicit defaultsTest plan
npx bmad-method install --yesand verify noundefinedvalues appear in generated filesnpx bmad-method install --yes --directory ./test-outputand inspect output confignpx bmad-method install) still works as before