refactor: Complete @clack/prompts Migration & Installer Output Consolidation #1586
Conversation
Full migration of BMAD CLI installer from legacy terminal libraries
(chalk, ora, boxen, figlet, wrap-ansi, cli-table3, readline) to unified
@clack/prompts v1.0.0 visual system.
Foundation (prompts.js + cli-utils.js):
- Extended prompts.js wrapper with box, spinner, progress, taskLog,
path, autocomplete, selectKey, stream, color re-export
- Refactored cli-utils.js: displayLogo uses box(), sections use note(),
steps use log.step(), removed boxen/figlet/wrap-ansi/cli-table3
UI orchestration (ui.js):
- Replaced ~100 console.log+chalk calls with log.*, note(), box()
- Replaced ora spinner with @Clack spinner
- Module selection: autocompleteMultiselect with locked core module,
bulleted post-selection display, maxItems for no-scroll
Spinner migration (installer.js):
- Replaced 40+ ora spinner calls with @Clack spinner
- All spinner.stop() calls include meaningful messages
- Failure paths use spinner.error() (red cross) instead of stop()
Readline migration (agent/installer.js + config-collector.js):
- Migrated readline prompts to @Clack text/confirm/select
- Fixed chalk.dim bug (chalk was never imported)
- Removed chalk from config-collector.js
IDE handlers + modules (7 files):
- Replaced chalk+ora across all IDE handlers and module manager
- Fixed options.installer undefined bug in manager.js update()
Cleanup:
- Removed ora, boxen, figlet, wrap-ansi, cli-table3 from dependencies
- chalk stays (used outside tools/cli/ scope)
- Replaced hand-drawn Unicode update box in bmad-cli.js with box()
- Added process.stdin.setMaxListeners(25) for sequential prompts
Spinner wrapper adds isSpinning state tracking (not native to @Clack).
Removed dead groupMultiselect and sortKey sort calls.
Ref: tech-spec-installer-clack-migration-ui-enhancement.md
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace ~40 lines of output from 15+ spinner start/stop cycles with a single animated spinner during installation and a final note() summary block showing checkmarks per step. Key changes: - Add results collector pattern in install() method - Replace spinner.stop/start pairs with addResult + spinner.message - Add renderInstallSummary() using prompts.note() with colored output - Propagate silent flag through IDE handlers and module manager - Add spinner race condition guards (start while spinning, stop while stopped) - Add no-op spinner pattern for silent external module cloning - Fix stdin listener limit to be defensive with Math.max - Add GIT_TERMINAL_PROMPT=0 for non-interactive git operations - Merge locked values into initialValue for autocomplete prompts Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@bmadcode this requires more detailed testing due to the large volume of changes, so I'm not sure if I would push it for the v6 release if the current setup is stable enough. On the other hand, this is one of the options in the discussion regarding the final solution for the installer. |
🤖 Augment PR SummarySummary: This PR completes a broad CLI UX refactor by standardizing interactive output on Changes:
Technical Notes: The PR relies on async display helpers throughout the CLI, adds stdin listener-limit protection, and centralizes color output via the prompts color utilities (picocolors). 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughRemoved five npm dependencies (boxen, cli-table3, figlet, ora, wrap-ansi) and refactored the CLI to use a centralized prompts-based logging system instead of chalk, ora, boxen, and direct console output. Updated ~25 files across installers, IDE setup, and CLI utilities to route messages through async prompts APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
tools/cli/installers/lib/ide/shared/workflow-command-generator.js (2)
234-250:⚠️ Potential issue | 🟠 Major
transformWorkflowPathnever handles/src/core/paths and returnsundefinedfor non-bmm inputs.The
else if (workflowPath.includes('/src/core/'))on Line 241 is nested inside theif (workflowPath.includes('/src/bmm/'))block. When the path contains/src/bmm/, the regex on Line 238 will always match, making theelse ifdead code. When the path does not contain/src/bmm/, the entire block is skipped and the function returnsundefined.This means any workflow path referencing
/src/core/is silently dropped, and callers receiveundefinedinstead of a transformed path—which will propagate as the string"undefined"wherever it's interpolated into template content (e.g., Line 213 inbuildLauncherContent).Proposed fix
transformWorkflowPath(workflowPath) { let transformed = workflowPath; if (workflowPath.includes('/src/bmm/')) { const match = workflowPath.match(/\/src\/bmm\/(.+)/); if (match) { transformed = `{project-root}/${this.bmadFolderName}/bmm/${match[1]}`; } - } else if (workflowPath.includes('/src/core/')) { - const match = workflowPath.match(/\/src\/core\/(.+)/); + } else if (workflowPath.includes('/src/core/')) { + const match = workflowPath.match(/\/src\/core\/(.+)/); if (match) { transformed = `{project-root}/${this.bmadFolderName}/core/${match[1]}`; } - - return transformed; } + + return transformed; }
155-161:⚠️ Potential issue | 🟠 MajorLine 161 is a no-op and should be removed or corrected.
Line 161 replaces
'_bmad'with'_bmad'—the search and replacement strings are identical (verified at byte level: 0x5f 0x62 0x6d 0x61 0x64). This has no effect after Line 160 already replaces'_bmad'withthis.bmadFolderName. Either this line should be deleted, or the search/replacement strings differ in a way not visible in the code (e.g., different unicode characters that should be preserved).The parallel code in
tools/cli/installers/lib/modules/manager.jslines 792–793 shows the same pattern but with reversed order, and includes a comment "IMPORTANT: Replace escape sequence and placeholder" that contradicts the identical strings shown in the code. This suggests a systematic issue where escape sequences may not be preserved in the codebase.tools/cli/installers/lib/custom/handler.js (1)
275-278:⚠️ Potential issue | 🟡 MinorPre-existing:
workflowsInstalledis incremented for preserved files too.This counter runs unconditionally for every
.mdfile, regardless of whether the file was preserved (Line 247-249) or newly copied (Line 250-268). This means the count inflates on reinstall/update. Not introduced by this PR, but it's in the changed file surface.tools/cli/commands/install.js (1)
87-100:⚠️ Potential issue | 🟠 MajorIncomplete migration:
console.erroron Line 90, and fragileawaitchains in catch block.Two issues here:
Line 90 still uses
console.error(error.fullMessage)while every other logging call in this file migrated toprompts.log.*. If the intent was to keep raw output for pre-formatted messages, that should be documented. Otherwise this is an oversight.Lines 92, 96–97
awaitasync prompts calls inside acatchblock. Ifprompts.log.*itself throws (e.g.,@clack/promptsfails to lazy-load), the original error context is silently lost and replaced by the prompts failure. A defensive try/catch or a fallback toconsole.errorwould be safer here.Proposed fix
} catch (error) { - // Check if error has a complete formatted message - if (error.fullMessage) { - console.error(error.fullMessage); - if (error.stack) { - await prompts.log.message(error.stack); - } - } else { - // Generic error handling for all other errors - await prompts.log.error(`Installation failed: ${error.message}`); - await prompts.log.message(error.stack); + try { + if (error.fullMessage) { + await prompts.log.error(error.fullMessage); + if (error.stack) { + await prompts.log.message(error.stack); + } + } else { + await prompts.log.error(`Installation failed: ${error.message}`); + await prompts.log.message(error.stack); + } + } catch { + // Fallback if prompts logging itself fails + console.error(error.fullMessage || `Installation failed: ${error.message}`); + if (error.stack) console.error(error.stack); } process.exit(1); }tools/cli/installers/lib/modules/manager.js (2)
786-793:⚠️ Potential issue | 🔴 CriticalRemove the no-op
replaceAllcall and fix ordering inconsistency with workflow-command-generator.js.Lines 792–793 contain a problematic pattern: Line 792
replaceAll('_bmad', '_bmad')is a literal no-op that serves no purpose. More critically, the ordering of operations differs between this file andworkflow-command-generator.js(lines 160–161): here the no-op comes first followed by the actual replacement, but in the other file it's reversed (actual replacement first, no-op second). One of these orderings is wrong. Remove the no-op call on line 792 and align the replacement sequence withworkflow-command-generator.js.
288-291:⚠️ Potential issue | 🟡 MinorMigrate remaining
console.warncalls toprompts.log.warnfor consistency.The file has migrated nearly all logging to
prompts.log.*but threeconsole.warncalls remain in error handlers at lines 290, 697, and 1326. Replace them withprompts.log.warnto maintain consistent logging throughout the module.tools/cli/bmad-cli.js (1)
17-19:⚠️ Potential issue | 🟡 MinorFire-and-forget async update check can interleave output with command results.
checkForUpdate()is invoked at module-load time and forgotten. Becauseprompts.box()is now async and writes to stdout, the update banner can render in the middle of (or after) any command's output — especially fast commands likestatus. The oldconsole.warnapproach had the same theoretical race, but the newawait prompts.box(...)internally awaits rendering, meaning the promise chain takes longer and the interleaving window is wider.Consider capturing the update message and printing it at a deterministic point (e.g., after
program.parseAsync()or via a Commander hook) rather than fire-and-forget.Also applies to: 38-48
tools/cli/installers/lib/ide/_config-driven.js (1)
494-510:⚠️ Potential issue | 🟠 MajorUnhandled
fs.staterror on broken symlinks will crash cleanup loop.If an entry starting with
bmadis a broken symlink,fs.stat(entryPath)throwsENOENTand the entirecleanupTargetaborts, leaving remaining files uncleaned. Wrap the per-entry block in a try/catch to make cleanup resilient.Also, the
isFile()/isDirectory()branch is redundant — both paths callfs.remove(entryPath). Simplify.Proposed fix
for (const entry of entries) { if (!entry || typeof entry !== 'string') { continue; } if (entry.startsWith('bmad')) { - const entryPath = path.join(targetPath, entry); - const stat = await fs.stat(entryPath); - if (stat.isFile()) { - await fs.remove(entryPath); - removedCount++; - } else if (stat.isDirectory()) { + try { + const entryPath = path.join(targetPath, entry); await fs.remove(entryPath); removedCount++; + } catch { + // Broken symlink or permission error — skip and continue } } }tools/cli/lib/ui.js (2)
828-828:⚠️ Potential issue | 🟡 MinorDead variable
hasCustomContentItems.
hasCustomContentItemsis declared asfalseon line 828 and never read or reassigned anywhere in the method. Remove it.
558-564:⚠️ Potential issue | 🟠 MajorRecursive
promptToolSelectioncall drops theoptionsparameter.When the user declines "no tools selected" and the method recurses,
optionsis not forwarded. This means command-line flags (e.g.--tools) are lost on retry, which could cause unexpected behavior (e.g., re-prompting interactively when CLI args were supplied).Same issue on line 642.
Proposed fix
if (!confirmNoTools) { - return this.promptToolSelection(projectDir); + return this.promptToolSelection(projectDir, options); }And similarly at line 642:
if (!confirmNoTools) { - return this.promptToolSelection(projectDir); + return this.promptToolSelection(projectDir, options); }tools/cli/installers/lib/core/config-collector.js (1)
740-759:⚠️ Potential issue | 🟠 Major"Accept Defaults?" prompt result is captured but never used.
For modules with zero configuration keys (line 738:
hasNoConfig === true), the code prompts the user with "Accept Defaults (no to customize)?" on line 749, destructures the result intocustomize, and then unconditionally displays the subheader/message regardless of the answer. The user's choice has no effect — there's nothing to customize.Either remove the prompt entirely (since there's nothing to configure) or wire the
customizeresponse to a meaningful branch.tools/cli/installers/lib/core/dependency-resolver.js (1)
80-96:⚠️ Potential issue | 🟡 Minor
moduleDirisundefinedwhen the module is neither'core'nor'bmm', producing a misleading warning.If
selectedModulescontains a module other than'core'or'bmm'(and it isn't filtered byisExternalModule), themoduleDirvariable is never assigned. Line 93 then callsfs.pathExists(undefined)which returnsfalse, and line 94 logs"Module directory not found: undefined". This is confusing and a latent bug.While pre-existing, the new
prompts.log.warnmakes this more user-visible. Consider adding an explicit guard:🛡️ Proposed guard
+ if (!moduleDir) { + if (options.verbose) { + await prompts.log.warn(`No source mapping for module: ${module}`); + } + continue; + } + if (!(await fs.pathExists(moduleDir))) { await prompts.log.warn('Module directory not found: ' + moduleDir); continue; }tools/cli/installers/lib/ide/codex.js (1)
258-298:⚠️ Potential issue | 🟡 MinorInconsistent
silentgating withinclearOldBmadFiles.The directory-read error at line 268 is gated by
!options.silent, but the per-entry error at line 295 always logs viaprompts.log.message. During a silent installation (spinner running), the per-entry message could interleave with the spinner output.🛡️ Proposed fix — gate per-entry message
} catch (error) { // Skip files that can't be processed - await prompts.log.message(` Skipping ${entry}: ${error.message}`); + if (!options.silent) await prompts.log.message(` Skipping ${entry}: ${error.message}`); }tools/cli/installers/lib/ide/manager.js (1)
170-213:⚠️ Potential issue | 🟡 MinorInconsistent error return shape:
reasonvserror.Line 176 returns
{ success: false, reason: 'unsupported' }but line 212 returns{ success: false, ide: ideName, error: error.message }. The consumer ininstaller.js(lines 1049–1053) readssetupResult.error, so the unsupported-IDE case silently falls through to the'failed'default. Consider unifying:🛡️ Proposed fix
if (!handler) { await prompts.log.warn(`IDE '${ideName}' is not yet supported`); await prompts.log.message(`Supported IDEs: ${[...this.handlers.keys()].join(', ')}`); - return { success: false, reason: 'unsupported' }; + return { success: false, ide: ideName, error: 'unsupported IDE' }; }tools/cli/installers/lib/core/installer.js (1)
273-276:⚠️ Potential issue | 🔴 Critical
bmadDiris not in scope at line 275 and will cause a ReferenceError.
bmadDiris declared at line 381 inside thetryblock, but line 275 references it in theelseblock that precedes thetry. If this code path executes during a non-quick-update modify flow where a custom module'ssourcePathstarts with'_config', it will throw aReferenceError.Fix by declaring
bmadDirbefore theelseblock:const bmadDir = path.join(projectDir, BMAD_FOLDER_NAME);
🤖 Fix all issues with AI agents
In `@tools/cli/installers/lib/core/config-collector.js`:
- Line 347: Remove the stray spacing console.log() call (the bare expression
console.log()) in config-collector.js and either delete it entirely or replace
it with the project's prompts-based spacing/output convention (e.g., use the
prompts library's separator/notice helper used elsewhere in this module). Locate
the exact console.log() expression and update it to match surrounding prompt
usage so output remains consistent with other prompt-driven messages.
In `@tools/cli/installers/lib/core/installer.js`:
- Around line 1017-1062: The code temporarily replaces console.log when
!config.verbose before iterating validIdes and awaiting this.ideManager.setup,
which leaves console.log broken if an awaited call throws; remove the global
monkey-patch entirely and rely on the existing silent/verbose flags (pass
silent: ideHasConfig and verbose: config.verbose to this.ideManager.setup and
fix any handlers that directly call console.log), or at minimum wrap the
suppression/restoration around the loop in a try/finally so console.log =
originalLog is always executed even if awaits inside the validIdes loop throw;
update references around validIdes, ideConfigurations, spinner, and
this.ideManager.setup accordingly.
In `@tools/cli/installers/lib/modules/manager.js`:
- Around line 626-630: In update(), the call to compileModuleAgents(sourcePath,
targetPath, moduleName, bmadDir) omits the fifth installer parameter, so sidecar
file tracking never gets populated; modify the call inside update() to pass the
installer object (the same value passed from install(), e.g. options.installer
or the local installer variable) so compileModuleAgents(...) receives the
installer and installer.installedFiles is populated during updates (ensure the
identifier matches the installer used elsewhere in this file).
- Around line 366-371: The silent-mode spinner stub inside createSpinner must
match the real spinner API from prompts.spinner: add no-op cancel() and clear()
methods and define isSpinning and isCancelled getters (returning false) so
callers won't throw or get undefined; update the returned object in
createSpinner (the stub that currently has start/stop/error/message) to include
cancel, clear, and the two getters while keeping existing no-op implementations
for start/stop/error/message to preserve behavior.
In `@tools/cli/lib/agent/installer.js`:
- Line 167: The defaultValue for prompts.text currently uses the expression
q.default || '' which incorrectly replaces legitimate falsy defaults like 0 or
"0"; update the code that constructs the prompts.text options (the block that
sets defaultValue for each question, referencing q.default and prompts.text) to
use the nullish coalescing operator (q.default ?? '') so only null or undefined
are replaced, preserving numeric 0 and string "0" as valid defaults.
- Line 169: The assignment answers[q.var] = response || q.default || ''
incorrectly treats intentional empty-string responses as missing; change the
fallback logic in the installer code that sets answers[q.var] (where response
and q.default are used) to use nullish coalescing so that only null/undefined
trigger the fallback (i.e., use response ?? q.default ?? ''), preserving
deliberate empty-string input.
- Around line 173-177: The boolean prompt call using prompts.confirm should pass
the wrapper's expected option name: replace the incorrect option key
"initialValue" with "default" so the call to prompts.confirm({ message:
q.prompt, default: q.default }) forwards q.default correctly; update the call
around prompts.confirm in installer.js (the block that assigns answers[q.var] =
response) to use "default" instead of "initialValue".
In `@tools/cli/lib/cli-utils.js`:
- Around line 61-76: The border color mapping in displayBox silently maps only
'green','red','yellow' to color functions and defaults everything else to cyan;
change formatBorder resolution in the displayBox function to use a lookup or
dynamic property access on the color object (from prompts.getColor())—e.g.,
check if color[borderColor] is a function and use it, otherwise fall back to
color.cyan—to support arbitrary valid color names (and keep existing behavior
for undefined borderStyle and options.title when calling prompts.box).
In `@tools/cli/lib/prompts.js`:
- Line 115: The message passthrough currently calls s.message(msg)
unconditionally (line with "message: (msg) => s.message(msg)"), which can run
when the spinner isn't active; guard this call with the same spinning flag used
by start/stop/error/cancel/clear so it only forwards to s.message when spinning
is true (otherwise no-op or return safely). Update the "message" property to
check the spinning variable before invoking s.message to avoid calling the
underlying `@clack/prompts` spinner when it's not active.
- Around line 648-673: The stream methods (stream.info, stream.success,
stream.step, stream.warn, stream.error, stream.message) call clack.stream.*
without awaiting or returning the resulting promise; update each async method in
the stream object to either return or await the inner call (e.g., return
clack.stream.info(generator) or await clack.stream.info(generator)) so callers
awaiting prompts.stream.info(...) properly wait for the underlying
clack.stream.* promise resolved; use getClack() as currently done and apply the
same change to all listed methods including message(generator, options).
In `@tools/cli/lib/ui.js`:
- Around line 964-966: Remove the unused variable availableNames: delete the
const availableNames = externalModuleChoices.map((c) => c.name).join(', '); line
from the module selection code so only the used message variable remains; ensure
no other code references availableNames (look for externalModuleChoices and
message in the same block) and run linter to confirm no unused-vars remain.
🧹 Nitpick comments (20)
tools/cli/installers/lib/custom/handler.js (1)
88-93: Logging migration looks correct, but error context is lost.The
awaitonprompts.log.warn()is correct. However, the old code likely included chalk-formatted path info. The new string concatenation approach works, but consider using template literals for consistency with the rest of the codebase (e.g., Line 352 uses template literals for a similar pattern).Use template literal for consistency
- await prompts.log.warn('YAML parse error in ' + configPath + ': ' + parseError.message); + await prompts.log.warn(`YAML parse error in ${configPath}: ${parseError.message}`);tools/cli/installers/lib/modules/manager.js (1)
1282-1288: Default logger still uses rawconsole.*methods, inconsistent with migration.The fallback logger at Lines 1284–1288 uses
console.log,console.error, andconsole.warn. While this is a fallback for when no logger is provided, it means any module installer that doesn't receive a logger will bypass the prompts-based UI entirely. Consider providing a prompts-based default logger, or at minimum documenting why raw console is acceptable here.tools/cli/lib/cli-utils.js (2)
22-45:getColor()is called on every invocation ofdisplayLogo,displayBox,displayComplete, anddisplayError.Each display method independently
awaitsprompts.getColor(). WhilegetPicocolorscaches the import, each call still traverses the async wrapper chain. If these methods are called in quick succession (e.g., during install flow), this is wasteful. Consider caching the color object at module scope after first resolution.Example: cache color at module level
+let _color = null; +async function getColor() { + if (!_color) _color = await prompts.getColor(); + return _color; +} + const CLIUtils = { // ... async displayLogo(_clearScreen = true) { const version = this.getVersion(); - const color = await prompts.getColor(); + const color = await getColor();
148-153:clearLinesuses rawprocess.stdoutmethods that may not exist in all environments.
process.stdout.moveCursorandprocess.stdout.clearLineare only available when stdout is a TTY. In piped/redirected output or CI environments, these will throw. This is pre-existing, but now that the rest of the module migrates to prompts-based output (which handles non-TTY gracefully), this method stands out as inconsistent.Add TTY guard
clearLines(lines) { + if (!process.stdout.isTTY) return; for (let i = 0; i < lines; i++) { process.stdout.moveCursor(0, -1); process.stdout.clearLine(1); } },tools/cli/lib/prompts.js (2)
362-369: Coupling to private_isActionKeymethod of@clack/core.Line 363 accesses
prompt._isActionKey, which is a private implementation detail ofAutocompletePrompt. This will break silently or throw if@clack/corerenames or removes this method in a future version. The comment acknowledges this is a fix, but it should be defensive.Add a guard
- const originalIsActionKey = prompt._isActionKey.bind(prompt); - prompt._isActionKey = function (char, key) { - if (key && key.name === 'space') { - return true; - } - return originalIsActionKey(char, key); - }; + if (typeof prompt._isActionKey === 'function') { + const originalIsActionKey = prompt._isActionKey.bind(prompt); + prompt._isActionKey = function (char, key) { + if (key && key.name === 'space') { + return true; + } + return originalIsActionKey(char, key); + }; + }
531-556:logmethods also don'tawait/returninner calls—but these are likely synchronous.For consistency, and to future-proof against
@clack/promptsmaking these async, considerreturn-ing the inner call, same as the stream methods issue. Currently callersawait prompts.log.info(msg)which resolves toundefinedfrom theawait getClack()rather than from the actual log call.tools/cli/installers/lib/message-loader.js (1)
80-80: Class field declaration placed after all method definitions.The
messages = nullfield initializer works, but placing it after all methods is unconventional and easy to miss. Moving it to the top of the class (or into the constructor) would improve discoverability.tools/cli/commands/status.js (2)
32-32: Calling private methodmanifest._readRaw()from a command module.
_readRawis conventionally private (underscore prefix). This couplesstatus.jsto an internal implementation detail ofManifest. If the manifest API already exposes a public method for reading data, prefer that; otherwise, promote_readRawto a public method (e.g.,readRaworread).
22-22: Inlinerequire('fs-extra')inside action handler.
fs-extrais already a top-level dependency in other files across this codebase. Moving it to the top of the file is cleaner and avoids repeated resolution on every invocation.tools/cli/installers/lib/core/config-collector.js (1)
594-597: Emptyifbranch for core module.The
if (moduleName === 'core')block contains only a comment. This is a no-op that adds noise. Invert the condition to remove the empty branch.Proposed fix
- if (moduleName === 'core') { - // Core module: no confirm prompt, continues directly - } else { + if (moduleName !== 'core') { // Non-core modules: show "Accept Defaults?" confirm prompttools/cli/installers/lib/ide/kilo.js (1)
160-161: Inconsistent file-system access: inlinerequire('fs-extra')vs. base-class helpers.
clearBmadWorkflows(line 160) andcleanup(line 175) each do an inlinerequire('fs-extra'), while the rest of the class usesthis.pathExists,this.readFile,this.writeFile, andthis.ensureDirfromBaseIdeSetup. This inconsistency makes it harder to mock/test and breaks the abstraction the base class provides.Prefer using the base-class helpers throughout, or import
fs-extraonce at the top of the file.Also applies to: 175-175
tools/cli/installers/lib/ide/kiro-cli.js (1)
17-21: JSDoc missing@paramforoptions.The
cleanupsignature was updated to acceptoptions = {}, but the JSDoc block (lines 17–20) doesn't document it. Other methods likesetup(line 40) do document@param {Object} options.📝 Proposed doc fix
/** * Cleanup old BMAD installation before reinstalling * `@param` {string} projectDir - Project directory + * `@param` {Object} [options] - Cleanup options + * `@param` {boolean} [options.silent] - Suppress log output */ async cleanup(projectDir, options = {}) {tools/cli/installers/lib/core/dependency-resolver.js (1)
553-554: Dead variable:contentis read but never used.Line 553 reads the file into
content, but the actual parsing happens insidethis.parseDependencies(...)at line 555, which re-reads the file independently. Thecontentvariable is unused.🧹 Remove dead read
if ((depPath.endsWith('.md') || depPath.endsWith('.yaml') || depPath.endsWith('.yml')) && (await fs.pathExists(depPath))) { - const content = await fs.readFile(depPath, 'utf8'); const subDeps = await this.parseDependencies([tools/cli/installers/lib/core/installer.js (3)
438-442: Shadowedpromptsvariable — redundant re-import.Line 439 re-declares
const prompts = require(...)inside a block scope, shadowing the module-levelpromptsimported at line 17. Since Node caches modules, this works identically, but it's dead weight and a readability hazard. This looks like a leftover from before the module-level import was added.🧹 Remove redundant import
if (modulesToRemove.length > 0) { - const prompts = require('../../../lib/prompts'); if (spinner.isSpinning) { spinner.stop('Reviewing module changes'); }
376-378:projectDiris re-declared insidetry, shadowing the identical definition at Line 239.Line 239:
const projectDir = path.resolve(config.directory);
Line 378:const projectDir = path.resolve(config.directory);(insidetry)Both resolve to the same value. This shadowing means code before the
try(lines 240–372) and code insidetryreference differentconstbindings that happen to hold the same value. It's confusing and creates a maintenance trap — if one changes, the other won't.♻️ Remove duplicate declaration
// Resolve target directory (path.resolve handles platform differences) - const projectDir = path.resolve(config.directory); + // projectDir already declared at line 239
1070-1075:moduleLoggerstill uses rawconsole.log/console.error/console.warn— inconsistent with prompts migration.The module logger at lines 1071–1075 creates a wrapper that calls
console.log,console.error, andconsole.warndirectly. Given that lines 1018-1021 suppressconsole.log, and the broader migration replaces console calls withprompts.log.*, this logger won't output anything in non-verbose mode (becauseconsole.logis overridden to a no-op). If the console.log suppression is removed (as recommended), this logger will start emitting raw console output that may interleave with the spinner.Consider routing this through prompts as well, or noting that
silent: trueon line 1086/1101 should make the logger moot.tools/cli/installers/lib/ide/codex.js (1)
286-293: BothisFile()andisDirectory()branches perform identicalfs.remove— could be simplified.Pre-existing, but since you're touching this method: the conditional is redundant because
fs.removehandles both files and directories.♻️ Simplify
try { - const stat = await fs.stat(entryPath); - if (stat.isFile()) { - await fs.remove(entryPath); - } else if (stat.isDirectory()) { - await fs.remove(entryPath); - } + await fs.remove(entryPath); } catch (error) {tools/cli/installers/lib/ide/manager.js (1)
183-208: Triplicated detail-building logic — consider extracting a helper.The identical "collect non-zero counts into parts and join" pattern is repeated for three handler return shapes. A small helper would reduce duplication and make adding new handler types easier.
♻️ Proposed extraction
+ /** + * Build a human-readable detail string from count fields + */ + _buildDetailString(counts) { + const labels = { agents: 'agents', modes: 'agents', workflows: 'workflows', tasks: 'tasks', tools: 'tools' }; + const parts = []; + for (const [key, label] of Object.entries(labels)) { + if (counts[key] > 0) parts.push(`${counts[key]} ${label}`); + } + return parts.join(', '); + } // Then in setup(): - if (handlerResult && handlerResult.results) { - const r = handlerResult.results; - const parts = []; - if (r.agents > 0) parts.push(`${r.agents} agents`); - // ... etc - detail = parts.join(', '); - } else if (handlerResult && handlerResult.counts) { - // ... etc - } else if (handlerResult && handlerResult.modes !== undefined) { - // ... etc - } + const rawCounts = handlerResult?.results || handlerResult?.counts || handlerResult || {}; + detail = this._buildDetailString(rawCounts);tools/cli/lib/agent/installer.js (2)
147-149:presetAnswersvalues are never validated or type-checked.
presetAnswersis spread directly intoanswersand can silently override defaults with wrong types (e.g., string where boolean is expected). There's no guard or schema validation. Consumers ofanswersdownstream may break on unexpected types.
461-461: Redundantrequire('yaml')— already imported at the top of the file (Line 8).
yamlis required at the module level on Line 8 asyaml, butsaveAgentSource(Line 461),createIdeSlashCommands(Line 542), andupdateManifestYaml(Line 571) all do a localconst yamlLib = require('yaml'). This is harmless (Node caches modules) but unnecessarily noisy — just use the existingyamlbinding.
Address 31 issues across 14 CLI files found during PR bmad-code-org#1586 review (Augment Code + CodeRabbit): - Fix bmadDir ReferenceError by hoisting declaration before try block - Wrap console.log monkey-patch in try/finally for safe restoration - Fix transformWorkflowPath dead code and undefined return path - Fix broken symlink crash in _config-driven.js and codex.js cleanup - Pass installer instance through update() for agent recompilation - Fix @clack/prompts API: defaultValue→default, initialValue→default - Use nullish coalescing (??) instead of logical OR for falsy values - Forward options in recursive promptToolSelection calls - Remove no-op replaceAll('_bmad','_bmad') in manager and generator - Remove unused confirm prompt in config-collector hasNoConfig branch - Guard spinner.message() when spinner is not running - Add missing methods to silent spinner stub (cancel, clear, isSpinning) - Wrap install.js error handler with inner try/catch + console fallback - Gate codex per-entry error log with silent flag - Add return statements to all stream wrapper methods - Remove dead variables (availableNames, hasCustomContentItems) - Filter core module from update flow selection - Replace borderColor ternary chain with object map - Fix Kilo "agents" label to "modes" in IDE manager - Normalize error return shape for unsupported IDEs - Fix spinner message timing before dependency resolution - Guard undefined moduleDir in dependency-resolver - Fix workflowsInstalled counter inflation in custom handler - Migrate console.warn calls to prompts.log.warn - Replace console.log() with prompts.log.message('') - Fix legacyBmadPath hardcoded to .bmad instead of entry.name - Fix focusedValue never assigned breaking SPACE toggle and hints Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
|
amazing thank you buddy @dracic ! |
2 participants
Summary
ora,chalk+console.log,boxen,readline, andfigletto a unified@clack/promptsvisual system across all 24 modified filesora,boxen,figlet,wrap-ansi,cli-table3), saving ~870 lines from the lockfileWhat changed
prompts.jswrapper -- new @Clack v1.0.0 featuresExtended with
intro(),outro(),cancel(),note(),box(),spinner(),progress(),tasks(),taskLog(),path(),autocomplete(),selectKey(),stream.*,colorre-export, and a fulllog.*object. Added spinner race condition guards (start-while-spinning becomesmessage(), stop-while-stopped is a no-op) and merged locked values intoinitialValuefor autocomplete prompts.cli-utils.js-- display primitives refactoredReplaced
boxen-based boxes withprompts.box(), chalk section headers withprompts.note(), and chalk step indicators withprompts.log.step(). Removed deadfigletimport, unuseddisplayTable()function, andboxen/wrapAnsi/cli-table3requires.ui.js-- main UI orchestratorReplaced ~100
console.log(chalk.*)calls withprompts.log.*,prompts.note(), andprompts.box(). Replaced the singleoraspinner with @Clack spinner. Replaced legacy installation warning blocks and version displays withnote(). Removedchalkandoraimports entirely. All display functions made async.installer.js-- ora spinner replacement + output consolidationReplaced 40+
oraspinner operations with @Clack spinner equivalents. Converted allspinner.textassignments tos.message(), allspinner.succeed()tos.stop(), andspinner.fail()tos.error(). Replaced ~30console.log(chalk.*)calls withprompts.log.*andprompts.note().Refactored
install()to use a results collector pattern: onespinner.start()at the beginning,spinner.message()for phase updates,addResult(step, status, detail)to accumulate results, and a singlespinner.stop()at the end followed byrenderInstallSummary()which renders aprompts.note()block with colored checkmarks per step.agent/installer.js-- readline removalFully migrated from Node.js
readlineto @Clack prompts (text(),confirm(),select()). Fixed a pre-existing bug wherechalk.dim()was called but chalk was never imported.config-collector.js+dependency-resolver.js-- chalk removalReplaced all chalk console.log output with
prompts.log.*. Preserved verbose gating.IDE handlers -- silent flag + chalk removal
Migrated all chalk output in
codex.js,kilo.js,kiro-cli.js,_config-driven.js,_base-ide.js, andmanager.js(IDE) toprompts.log.*. Addedsilentflag propagation through all handlers to suppress per-handler log output while the main installer spinner is active. Config-driven IDEs that need no prompting are marked with{ _noConfigNeeded: true }so the spinner runs uninterrupted. IDEs requiring interactive configuration (e.g., Codex install location) stop the spinner, collect input, then restart it.modules/manager.js-- ora replacement + silent modeReplaced ora spinners with @Clack spinner. Added no-op spinner pattern for silent mode. Added
GIT_TERMINAL_PROMPT=0andstdio: ['ignore', 'pipe', 'pipe']to gitexecSynccalls to prevent hangs.Entry points + minor files
Replaced hand-drawn Unicode update box in
bmad-cli.jswithprompts.box(). Madestdin.setMaxListenersdefensive withMath.max(currentLimit, 50). Migrated chalk output ininstall.js,status.js,message-loader.js, andcustom/handler.js.Dependency cleanup
Removed 5 dependencies from
package.json.chalkstays (used in files outside CLI scope).[email protected]@clack/promptsspinner[email protected]@clack/promptsbox()[email protected][email protected][email protected]Files changed (24)
tools/cli/lib/prompts.jstools/cli/lib/cli-utils.jstools/cli/lib/ui.jstools/cli/installers/lib/core/installer.jstools/cli/installers/lib/core/config-collector.jstools/cli/installers/lib/core/dependency-resolver.jstools/cli/installers/lib/modules/manager.jstools/cli/installers/lib/ide/_config-driven.jstools/cli/installers/lib/ide/_base-ide.jstools/cli/installers/lib/ide/codex.jstools/cli/installers/lib/ide/kilo.jstools/cli/installers/lib/ide/kiro-cli.jstools/cli/installers/lib/ide/manager.jstools/cli/lib/agent/installer.jstools/cli/bmad-cli.jstools/cli/commands/install.jstools/cli/commands/status.jstools/cli/installers/lib/custom/handler.jstools/cli/installers/lib/message-loader.jstools/cli/installers/lib/ide/shared/agent-command-generator.jstools/cli/installers/lib/ide/shared/task-tool-command-generator.jstools/cli/installers/lib/ide/shared/workflow-command-generator.jspackage.jsonpackage-lock.jsonTest plan
npm test-- all 52 schema tests, 12 install tests, 10 agent validations passnpx bmad installin clean directory -- verify single spinner + summary notenpx bmad install --yes-- verify non-interactive mode works