refactor(installer): restructure installer with clean separation of concerns#2129
Merged
refactor(installer): restructure installer with clean separation of concerns#2129
Conversation
…oncerns Move tools/cli/ to tools/installer/ with major structural cleanup: - InstallPaths async factory for path resolution and directory creation - Config value object (frozen) replaces mutable config bag - ExistingInstall value object replaces stateful Detector class - OfficialModules + CustomModules + ExternalModuleManager replace monolithic ModuleManager - install() is prompt-free; all user interaction in ui.js - Update state returned explicitly instead of mutating customConfig - Delete dead code: dependency-resolver, _base-ide, IdeConfigManager, platform-codes helpers, npx wrapper, xml-utils - Flatten directory structure: custom/handler → custom-handler, tools/cli/ → tools/installer/, lib/ directories removed - Update all path references in package.json, tests, CI, and docs
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis PR reorganizes the installer codebase from Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/installer/modules/official-modules.js (1)
1405-1414:⚠️ Potential issue | 🟠 Major
--yesskips function-backed defaults.
buildQuestion()turns same-module templated defaults into functions, but this branch only copies literal defaults. Headless installs/tests will therefore drop those fields instead of resolving them.🤖 Proposed fix
for (const question of questions) { const hasDefault = question.default !== undefined && question.default !== null && question.default !== ''; - if (hasDefault && typeof question.default !== 'function') { - allAnswers[question.name] = question.default; - } + if (!hasDefault) continue; + allAnswers[question.name] = + typeof question.default === 'function' ? question.default(allAnswers) : question.default; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/modules/official-modules.js` around lines 1405 - 1414, The skip-prompts branch currently only copies literal defaults and ignores function-backed defaults produced by buildQuestion, causing templated defaults to be dropped; update the loop in the this.skipPrompts branch that iterates over questions to check if question.default is a function and, if so, invoke it (awaiting the result if it returns a Promise) with the current answers/context (e.g., pass allAnswers or the same args buildQuestion expects) and assign the returned value to allAnswers[question.name]; keep the existing behavior for non-function defaults and preserve the hasDefault guard and logging using moduleDisplayName.
🧹 Nitpick comments (4)
tools/installer/ide/_config-driven.js (2)
44-57: Unify the BMAD-entry predicate across detection paths.
detect(),cleanupTarget(), andfindAncestorConflict()now each define “BMAD-owned entry” a little differently. Pull that into one helper sobmad-os-*handling and case-folding stay consistent across detect/cleanup/conflict flows.♻️ Suggested refactor
+ isManagedBmadEntry(entry) { + const name = typeof entry === 'string' ? entry.toLowerCase() : ''; + return name.startsWith('bmad') && !name.startsWith('bmad-os-'); + } + async detect(projectDir) { if (!this.configDir) return false; const dir = path.join(projectDir || process.cwd(), this.configDir); if (await fs.pathExists(dir)) { try { const entries = await fs.readdir(dir); - return entries.some((e) => typeof e === 'string' && e.startsWith('bmad')); + return entries.some((e) => this.isManagedBmadEntry(e)); } catch { return false; } } return false; }- if (entry.startsWith('bmad') && !entry.startsWith('bmad-os-')) { + if (this.isManagedBmadEntry(entry)) { const entryPath = path.join(targetPath, entry); try { await fs.remove(entryPath); removedCount++;- const hasBmad = entries.some( - (e) => typeof e === 'string' && e.toLowerCase().startsWith('bmad') && !e.toLowerCase().startsWith('bmad-os-'), - ); + const hasBmad = entries.some((e) => this.isManagedBmadEntry(e));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/ide/_config-driven.js` around lines 44 - 57, Create a single helper (e.g., isBmadEntry(name)) in this module that normalizes the entry name (toLowerCase()) and returns true for names that start with 'bmad' or the fuller prefix 'bmad-os-' (so both 'bmad*' and 'bmad-os-*' are handled consistently); then replace the ad-hoc predicates in detect(), cleanupTarget(), and findAncestorConflict() to call that helper so case-folding and the 'bmad-os-' rule are unified across all detection/cleanup/conflict flows.
48-53: Don't hide unreadable config dirs behindfalse.A permissions/I/O failure here is indistinguishable from “not installed”, so higher layers can take the wrong success path. Consider logging a warning or propagating a structured error instead of swallowing the exception.
As per coding guidelines,
tools/**: Build script/tooling. Check error handling and proper exit codes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/ide/_config-driven.js` around lines 48 - 53, The current check in the config-driven installer swallows all errors in the fs.readdir block (when checking dir via fs.pathExists and fs.readdir) and returns false, making permission/I/O failures indistinguishable from "not installed"; change the catch to either propagate a structured error (e.g., throw a new Error with context including dir and the original error) or at minimum log a warning with dir and error details (using the project's logger or console.warn) before rethrowing or returning a distinct error result so callers don't treat unreadable config dirs as absent.tools/installer/ide/platform-codes.js (1)
13-25: Consider wrappingyaml.parsein try/catch for better error messages.If
platform-codes.yamlcontains malformed YAML,yaml.parse()will throw a parsing error with a potentially cryptic message. Wrapping it in try/catch would allow a more informative error like "Failed to parse platform-codes.yaml: ".♻️ Optional improvement for clearer error messages
const content = await fs.readFile(PLATFORM_CODES_PATH, 'utf8'); - _cachedPlatformCodes = yaml.parse(content); + try { + _cachedPlatformCodes = yaml.parse(content); + } catch (error) { + throw new Error(`Failed to parse platform-codes.yaml: ${error.message}`); + } return _cachedPlatformCodes;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/ide/platform-codes.js` around lines 13 - 25, Wrap the yaml.parse call in loadPlatformCodes in a try/catch to surface a clearer error: catch parsing exceptions thrown by yaml.parse(content) and rethrow a new Error that prefixes a helpful message like "Failed to parse platform-codes.yaml at <PLATFORM_CODES_PATH>:" and include the original error message (or error.stack) to preserve details; ensure _cachedPlatformCodes is only set on successful parse and still returned from loadPlatformCodes.tools/installer/modules/external-manager.js (1)
251-292: Dependency installation logic is correct but has minor duplication.The npm install command and spinner handling are duplicated between the two conditional branches (Lines 255 and 281). While functional, this could be refactored to a helper function.
♻️ Optional: Extract npm install to helper
+ async _runNpmInstall(moduleCacheDir, moduleInfo, silent) { + const installSpinner = silent + ? { start() {}, stop() {}, error() {} } + : await prompts.spinner(); + installSpinner.start(`Installing dependencies for ${moduleInfo.name}...`); + try { + execSync('npm install --omit=dev --no-audit --no-fund --no-progress --legacy-peer-deps', { + cwd: moduleCacheDir, + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 120_000, + }); + installSpinner.stop(`Installed dependencies for ${moduleInfo.name}`); + } catch (error) { + installSpinner.error(`Failed to install dependencies for ${moduleInfo.name}`); + if (!silent) await prompts.log.warn(` ${error.message}`); + } + }Then call
await this._runNpmInstall(moduleCacheDir, moduleInfo, silent)in both branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/modules/external-manager.js` around lines 251 - 292, The npm install + spinner block is duplicated in the two branches; extract that logic into a single helper (e.g., _runNpmInstall or runNpmInstall) that accepts (moduleCacheDir, moduleInfo, silent) and performs: createSpinner(), start message using moduleInfo.name, execSync with the same args and timeout, stop on success, and error handling that calls installSpinner.error and, if !silent, awaits prompts.log.warn with error.message; then replace both duplicated blocks with a single await this._runNpmInstall(moduleCacheDir, moduleInfo, silent) call so all install behavior is centralized and consistent.
🤖 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/installer/core/installer.js`:
- Around line 816-859: The current approach splits yamlContent into lines and
repartitions them, which breaks multi-line scalars/objects; instead, operate on
the data structure before stringifying: build a new plain object by taking
finalConfig (or structuredClone(finalConfig)), iterate its top-level keys and
construct an ordered object where keys not in coreConfig (module-specific) are
added first and keys present in coreConfig are added afterward, then
yaml.stringify that ordered object into yamlContent and prepend coreSection as a
comment if needed; reference symbols: coreConfig, finalConfig, yamlContent,
coreSection, moduleName.
- Around line 1683-1685: findBmadDir currently always returns
path.join(projectDir, BMAD_FOLDER_NAME) which breaks detection if the BMAD
folder was renamed; update findBmadDir to first scan projectDir for any
subdirectory that contains the marker file "_config/manifest.yaml" and return
that directory as bmadDir if found (use async fs access/stat or readdir +
access), otherwise fall back to path.join(projectDir, BMAD_FOLDER_NAME); ensure
the return shape stays { bmadDir } so callers like quickUpdate, uninstall,
getStatus, and getOutputFolder continue to work.
- Around line 40-45: Move the custom module discovery before building official
modules and update OfficialModules.build to accept and forward custom module
paths into its configuration collection: call
this.customModules.discoverPaths(originalConfig, paths) before invoking
OfficialModules.build(config, paths), and change OfficialModules.build(...) (and
inside it, the call to collectAllConfigurations(...)) to accept a
customModulePaths argument (pass this.customModules.paths) and propagate that
into collectAllConfigurations so module.yaml schemas for selected custom modules
are available during headless config collection.
In `@tools/installer/modules/custom-modules.js`:
- Around line 92-98: The current checks using file.startsWith('sub-modules/')
and path.dirname(file).split('/').some(dir =>
dir.toLowerCase().endsWith('-sidecar')) break on Windows because they assume '/'
separators; normalize the path first (e.g., const normalized =
path.normalize(file)) and then use normalized.split(path.sep) to test the first
segment for 'sub-modules' instead of startsWith, and use
path.dirname(normalized).split(path.sep).some(dir =>
dir.toLowerCase().endsWith('-sidecar')) so both the initial sub-module check and
the sidecar detection handle platform-specific separators; replace uses of
file.startsWith and path.dirname(file).split('/') with these
normalized/path.sep-based checks.
In `@tools/installer/modules/official-modules.js`:
- Around line 483-489: The current catch block around reading/parsing moduleYaml
swallows errors and returns emptyResult, hiding invalid module.yaml; change the
catch to capture the error (e.g., catch (err)) and surface it instead of
silently returning: either log a warning/error including moduleYamlPath and err
via the module installer logger or throw an explicit Error so the CLI can fail
appropriately; update the code paths that reference emptyResult to handle the
thrown error or the logged failure so installs of broken module definitions are
not treated as successful.
- Around line 678-690: getFileList() returns platform-specific separators from
path.relative which breaks downstream POSIX-based filters in
copyModuleWithFiltering; normalize each pushed path to use forward slashes.
Update getFileList (the function named getFileList) so that after computing rel
= path.relative(baseDir, fullPath) you convert backslashes to '/', e.g. replace
all '\\' with '/' (or use path.posix equivalents) before pushing to files,
ensuring all returned entries are POSIX-style paths expected by
copyModuleWithFiltering and its sub-modules/ and agents/ filters.
---
Outside diff comments:
In `@tools/installer/modules/official-modules.js`:
- Around line 1405-1414: The skip-prompts branch currently only copies literal
defaults and ignores function-backed defaults produced by buildQuestion, causing
templated defaults to be dropped; update the loop in the this.skipPrompts branch
that iterates over questions to check if question.default is a function and, if
so, invoke it (awaiting the result if it returns a Promise) with the current
answers/context (e.g., pass allAnswers or the same args buildQuestion expects)
and assign the returned value to allAnswers[question.name]; keep the existing
behavior for non-function defaults and preserve the hasDefault guard and logging
using moduleDisplayName.
---
Nitpick comments:
In `@tools/installer/ide/_config-driven.js`:
- Around line 44-57: Create a single helper (e.g., isBmadEntry(name)) in this
module that normalizes the entry name (toLowerCase()) and returns true for names
that start with 'bmad' or the fuller prefix 'bmad-os-' (so both 'bmad*' and
'bmad-os-*' are handled consistently); then replace the ad-hoc predicates in
detect(), cleanupTarget(), and findAncestorConflict() to call that helper so
case-folding and the 'bmad-os-' rule are unified across all
detection/cleanup/conflict flows.
- Around line 48-53: The current check in the config-driven installer swallows
all errors in the fs.readdir block (when checking dir via fs.pathExists and
fs.readdir) and returns false, making permission/I/O failures indistinguishable
from "not installed"; change the catch to either propagate a structured error
(e.g., throw a new Error with context including dir and the original error) or
at minimum log a warning with dir and error details (using the project's logger
or console.warn) before rethrowing or returning a distinct error result so
callers don't treat unreadable config dirs as absent.
In `@tools/installer/ide/platform-codes.js`:
- Around line 13-25: Wrap the yaml.parse call in loadPlatformCodes in a
try/catch to surface a clearer error: catch parsing exceptions thrown by
yaml.parse(content) and rethrow a new Error that prefixes a helpful message like
"Failed to parse platform-codes.yaml at <PLATFORM_CODES_PATH>:" and include the
original error message (or error.stack) to preserve details; ensure
_cachedPlatformCodes is only set on successful parse and still returned from
loadPlatformCodes.
In `@tools/installer/modules/external-manager.js`:
- Around line 251-292: The npm install + spinner block is duplicated in the two
branches; extract that logic into a single helper (e.g., _runNpmInstall or
runNpmInstall) that accepts (moduleCacheDir, moduleInfo, silent) and performs:
createSpinner(), start message using moduleInfo.name, execSync with the same
args and timeout, stop on success, and error handling that calls
installSpinner.error and, if !silent, awaits prompts.log.warn with
error.message; then replace both duplicated blocks with a single await
this._runNpmInstall(moduleCacheDir, moduleInfo, silent) call so all install
behavior is centralized and consistent.
🪄 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: 8309a8e7-0533-4ae2-8c11-0f52d1d629f9
📒 Files selected for processing (85)
.github/workflows/publish.yamldocs/fr/how-to/non-interactive-installation.mddocs/how-to/non-interactive-installation.mddocs/zh-cn/how-to/non-interactive-installation.mdpackage.jsonsrc/core-skills/bmad-distillator/resources/distillate-format-reference.mdtest/test-install-to-bmad.jstest/test-installation-components.jstest/test-workflow-path-regex.jstools/bmad-npx-wrapper.jstools/cli/installers/lib/core/dependency-resolver.jstools/cli/installers/lib/core/detector.jstools/cli/installers/lib/core/ide-config-manager.jstools/cli/installers/lib/core/installer.jstools/cli/installers/lib/ide/_base-ide.jstools/cli/installers/lib/ide/platform-codes.jstools/cli/installers/lib/ide/platform-codes.yamltools/cli/installers/lib/ide/templates/combined/claude-workflow-yaml.mdtools/cli/installers/lib/modules/external-manager.jstools/cli/installers/lib/modules/manager.jstools/cli/lib/config.jstools/cli/lib/platform-codes.jstools/docs/_prompt-external-modules-page.mdtools/installer/README.mdtools/installer/bmad-cli.jstools/installer/cli-utils.jstools/installer/commands/install.jstools/installer/commands/status.jstools/installer/commands/uninstall.jstools/installer/core/config.jstools/installer/core/custom-module-cache.jstools/installer/core/existing-install.jstools/installer/core/install-paths.jstools/installer/core/installer.jstools/installer/core/manifest-generator.jstools/installer/core/manifest.jstools/installer/custom-handler.jstools/installer/external-official-modules.yamltools/installer/file-ops.jstools/installer/ide/_config-driven.jstools/installer/ide/manager.jstools/installer/ide/platform-codes.jstools/installer/ide/platform-codes.yamltools/installer/ide/shared/agent-command-generator.jstools/installer/ide/shared/bmad-artifacts.jstools/installer/ide/shared/module-injections.jstools/installer/ide/shared/path-utils.jstools/installer/ide/shared/skill-manifest.jstools/installer/ide/templates/agent-command-template.mdtools/installer/ide/templates/combined/antigravity.mdtools/installer/ide/templates/combined/claude-agent.mdtools/installer/ide/templates/combined/claude-workflow.mdtools/installer/ide/templates/combined/default-agent.mdtools/installer/ide/templates/combined/default-task.mdtools/installer/ide/templates/combined/default-tool.mdtools/installer/ide/templates/combined/default-workflow.mdtools/installer/ide/templates/combined/gemini-agent.tomltools/installer/ide/templates/combined/gemini-task.tomltools/installer/ide/templates/combined/gemini-tool.tomltools/installer/ide/templates/combined/gemini-workflow-yaml.tomltools/installer/ide/templates/combined/gemini-workflow.tomltools/installer/ide/templates/combined/kiro-agent.mdtools/installer/ide/templates/combined/kiro-task.mdtools/installer/ide/templates/combined/kiro-tool.mdtools/installer/ide/templates/combined/kiro-workflow.mdtools/installer/ide/templates/combined/opencode-agent.mdtools/installer/ide/templates/combined/opencode-task.mdtools/installer/ide/templates/combined/opencode-tool.mdtools/installer/ide/templates/combined/opencode-workflow-yaml.mdtools/installer/ide/templates/combined/opencode-workflow.mdtools/installer/ide/templates/combined/rovodev.mdtools/installer/ide/templates/combined/trae.mdtools/installer/ide/templates/combined/windsurf-workflow.mdtools/installer/ide/templates/split/.gitkeeptools/installer/install-messages.yamltools/installer/message-loader.jstools/installer/modules/custom-modules.jstools/installer/modules/external-manager.jstools/installer/modules/official-modules.jstools/installer/project-root.jstools/installer/prompts.jstools/installer/ui.jstools/installer/yaml-format.jstools/javascript-conventions.mdtools/lib/xml-utils.js
💤 Files with no reviewable changes (13)
- tools/lib/xml-utils.js
- tools/cli/installers/lib/core/ide-config-manager.js
- tools/cli/installers/lib/modules/external-manager.js
- tools/bmad-npx-wrapper.js
- tools/cli/installers/lib/ide/platform-codes.yaml
- tools/cli/lib/platform-codes.js
- tools/cli/installers/lib/modules/manager.js
- tools/cli/installers/lib/ide/platform-codes.js
- tools/cli/installers/lib/core/dependency-resolver.js
- tools/cli/installers/lib/core/detector.js
- tools/cli/lib/config.js
- tools/cli/installers/lib/ide/templates/combined/claude-workflow-yaml.md
- tools/cli/installers/lib/ide/_base-ide.js
… errors Guard ExistingInstall.version access with .installed check in uninstall.js, ui.js, and installer.js to prevent throwing on empty/partial _bmad dirs. Surface invalid module.yaml parse errors as warnings instead of silently returning empty results.
Collaborator
Author
Triage Summary — 11 findings reviewedFIX: 4 | DISMISS: 3 | DEFER: 4
Fixes applied in commit |
This was referenced Apr 8, 2026
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
This is a ground-up restructure of the BMAD installer — the largest single refactor in project history. The 3,000-line monolithic
Installerclass and its tangled dependency tree have been replaced with a clean, prompt-free orchestrator backed by focused value objects and module classes.Net result: 85 files changed, 3,703 additions, 7,715 deletions (−4,012 lines).
What changed
Directory restructure
tools/cli/→tools/installer/with a flattened hierarchy (installers/lib/core/→core/,installers/lib/custom/handler.js→custom-handler.js, etc.)New value objects replace stateful classes
Config— frozen, built once from user input viaConfig.build()InstallPaths— async factory for path resolution and directory creationExistingInstall— replaces the statefulDetectorclassModule system split
OfficialModules— source resolution, install, config collection (absorbs the oldConfigCollector)CustomModules— paths Map,discoverPaths(), installExternalModuleManager— git clone/fetch, npm installinstall() is now a prompt-free orchestrator
ui.js_prepareUpdateState()returns an explicit update state object instead of mutating a cloned config bag (customConfigeliminated entirely)_restoreUserFiles()returns results instead of writing them back onto a mutable objectDead code removed (~4,000 lines)
dependency-resolver.js(743 lines) — unused since module system redesign_base-ide.js(657 lines) —_config-driven.jsis the sole IDE installerIdeConfigManager(157 lines),Detector(223 lines),config.js(213 lines),platform-codes.jshelpers (216 lines)manager.js(928 lines) — monolithicModuleManagerreplaced by three focused classesinstaller.jsold (3,002 lines) → new (1,790 lines) —update(),updateCore(),promptUpdateAction(),createDirectoryStructure()all deleted as unreachablenpx-wrapper.js—binin package.json points directly tobmad-cli.jsxml-utils.js, deaddiscoverPathsbranch, orphaned JSDoc blocksclaude-workflow-yaml.mddangling symlinkBug fixes included
ui.jspackage.json path was 3 levels up instead of 2 (broke quick-update detection)bmbexternal modulemodule-definitionpath updated for skills/ restructure (fix: update bmb module-definition path #2126 port)Design principles
Configfor official modules, separate custom content handlingconfig.moduleslike any other, goes throughOfficialModules.install()What this enables next
quickUpdate()simplification into a thinner config-assembly wrapper_preserveModules/_existingModulespass-through fieldsTest plan
npm ci && npm run quality— all checks pass (lint, format, markdown, docs build, 205 component tests, file ref validation, skill validation)bmad-cli.js install --directory /tmp/test --modules bmm --tools claude-code --yes→ 43 skills--custom-content→ 46 skills (3 from custom modules)tools/clireferences in JS, JSON, YAML, or CI files