refactor(installer): remove custom content installation feature#2227
Merged
refactor(installer): remove custom content installation feature#2227
Conversation
Remove the entire local filesystem custom content feature from the installer to make way for marketplace-based plugin installation. Deleted: custom-handler.js, custom-module-cache.js, custom-modules.js Removed: --custom-content CLI flag, interactive custom content prompts, custom module caching, manifest tracking, missing-source resolution, and related test suites. Updated docs across all translations.
🤖 Augment PR SummarySummary: This PR removes the installer’s local filesystem “custom content” feature to make room for marketplace-based plugin/module installation. Changes:
Technical Notes: The installer still preserves user-modified files via 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| - Has a `code` field in the `module.yaml` | ||
|
|
||
| :::note[Still stuck?] | ||
| ::: note[Still stuck?] |
There was a problem hiding this comment.
::: note[...] looks like a syntax change from the prior :::note[...] admonition form; if this site uses Docusaurus-style admonitions, the space can prevent the note block from rendering.
Severity: medium
Other Locations
docs/fr/how-to/non-interactive-installation.md:163docs/cs/how-to/non-interactive-installation.md:151docs/zh-cn/how-to/non-interactive-installation.md:150
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| return { modules, customModules }; | ||
| return { modules }; |
There was a problem hiding this comment.
listAvailable() now returns only { modules }, but the JSDoc above still says it returns customModules as well, which is now misleading for callers.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Fix admonition syntax (remove accidental space in :::note) across 4 translated docs files, and update stale JSDoc on listAvailable().
📝 WalkthroughWalkthroughThe PR removes the 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 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/installer/modules/official-modules.js (1)
1022-1033:⚠️ Potential issue | 🟠 MajorPreserve existing module config when
module.yamlis missing.Lines 1022-1030 already keep
this._existingConfig[moduleName]in the quick path, but Line 1309 returns without doing the same in the full path. That leavesthis.collectedConfig[moduleName]unset even though the previous values were already loaded, so full config collection can discard persisted module settings just because the source schema is unavailable.As per coding guidelines, `tools/**`: Build script/tooling. Check error handling and proper exit codes.💡 Proposed fix
let configPath = null; if (await fs.pathExists(moduleConfigPath)) { configPath = moduleConfigPath; } else { + if (this._existingConfig?.[moduleName]) { + this.collectedConfig[moduleName] = { ...this._existingConfig[moduleName] }; + } // No config for this module return; }Also applies to: 1294-1311
🤖 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 1022 - 1033, When moduleConfigPath is missing the code currently preserves existing settings in the early/quick path but not in the full config-collection path, causing this.collectedConfig[moduleName] to be left unset; update the full-path branch (the code that returns false around where moduleConfigPath is checked) to check this._existingConfig[moduleName] and, if present, set this.collectedConfig[moduleName] = { ...this._existingConfig[moduleName] } (creating the object first if needed) before returning false; apply the same change to the analogous block around the other occurrence (the 1294-1311 area) so both quick and full collection paths preserve persisted module settings.tools/installer/core/installer.js (1)
1148-1215:⚠️ Potential issue | 🔴 CriticalDon't let quick update delete skipped modules.
modulesToUpdatenow excludes installed modules with no current source, butinstall()still treats anything missing fromconfig.modulesas deselected and removes it before_prepareUpdateState()backs files up. A quick update on an install that still contains a legacy/custom module—or an external module that is temporarily unavailable—will silently delete that module directory even though this branch labels it as "skipped".🐛 Suggested fix
async install(originalConfig) { let updateState = null; try { const config = Config.build(originalConfig); const paths = await InstallPaths.create(config); const officialModules = await OfficialModules.build(config, paths); const existingInstall = await ExistingInstall.detect(paths.bmadDir); if (existingInstall.installed) { - await this._removeDeselectedModules(existingInstall, config, paths); + await this._removeDeselectedModules( + existingInstall, + config, + paths, + new Set(originalConfig._preserveModules || []), + ); updateState = await this._prepareUpdateState(paths, config, existingInstall, officialModules); await this._removeDeselectedIdes(existingInstall, config, paths); }- async _removeDeselectedModules(existingInstall, config, paths) { + async _removeDeselectedModules(existingInstall, config, paths, preservedModules = new Set()) { const previouslyInstalled = new Set(existingInstall.moduleIds); const newlySelected = new Set(config.modules || []); + for (const moduleId of preservedModules) { + newlySelected.add(moduleId); + } const toRemove = [...previouslyInstalled].filter((m) => !newlySelected.has(m) && m !== 'core');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/core/installer.js` around lines 1148 - 1215, The quick-update flow builds installConfig with modules: modulesToUpdate but that causes install() / _prepareUpdateState() to treat skipped/legacy modules as deselected and delete them; fix by ensuring skipped modules are preserved: either (preferred) update install()/_prepareUpdateState() to honor installConfig._preserveModules (skip deletion/backups for any id in _preserveModules) or (alternative) merge skippedModules into installConfig.modules (modulesToUpdate ∪ skippedModules) while still setting _preserveModules to skippedModules so they are not re-installed; update references in install(), _prepareUpdateState(), and where installConfig is consumed to use installConfig._preserveModules to prevent removal of those module directories.
🧹 Nitpick comments (1)
tools/installer/modules/official-modules.js (1)
98-102: Update thelistAvailable()return contract comment.
listAvailable()now returns only{ modules }, but the JSDoc still advertises acustomModulesarray. Leaving that stale makes this API change easy to miss in the next edit.Also applies to: 124-124
🤖 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 98 - 102, Update the JSDoc for the listAvailable() function to reflect its current return contract: replace the outdated description that says it returns "{ modules array and customModules array }" with the correct return type "{ modules }" (i.e., an object containing only the modules array). Modify the comment block above listAvailable() (and the duplicate JSDoc occurrence referenced) to remove any mention of customModules and adjust the `@returns` line to accurately state it returns {Object} with property modules (or { modules: Array }). Ensure the function name listAvailable is unchanged and only the documentation text is updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tools/installer/core/installer.js`:
- Around line 1148-1215: The quick-update flow builds installConfig with
modules: modulesToUpdate but that causes install() / _prepareUpdateState() to
treat skipped/legacy modules as deselected and delete them; fix by ensuring
skipped modules are preserved: either (preferred) update
install()/_prepareUpdateState() to honor installConfig._preserveModules (skip
deletion/backups for any id in _preserveModules) or (alternative) merge
skippedModules into installConfig.modules (modulesToUpdate ∪ skippedModules)
while still setting _preserveModules to skippedModules so they are not
re-installed; update references in install(), _prepareUpdateState(), and where
installConfig is consumed to use installConfig._preserveModules to prevent
removal of those module directories.
In `@tools/installer/modules/official-modules.js`:
- Around line 1022-1033: When moduleConfigPath is missing the code currently
preserves existing settings in the early/quick path but not in the full
config-collection path, causing this.collectedConfig[moduleName] to be left
unset; update the full-path branch (the code that returns false around where
moduleConfigPath is checked) to check this._existingConfig[moduleName] and, if
present, set this.collectedConfig[moduleName] = {
...this._existingConfig[moduleName] } (creating the object first if needed)
before returning false; apply the same change to the analogous block around the
other occurrence (the 1294-1311 area) so both quick and full collection paths
preserve persisted module settings.
---
Nitpick comments:
In `@tools/installer/modules/official-modules.js`:
- Around line 98-102: Update the JSDoc for the listAvailable() function to
reflect its current return contract: replace the outdated description that says
it returns "{ modules array and customModules array }" with the correct return
type "{ modules }" (i.e., an object containing only the modules array). Modify
the comment block above listAvailable() (and the duplicate JSDoc occurrence
referenced) to remove any mention of customModules and adjust the `@returns` line
to accurately state it returns {Object} with property modules (or { modules:
Array }). Ensure the function name listAvailable is unchanged and only the
documentation text is updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50cccfab-ae71-419c-b86f-982cdddbdb8c
📒 Files selected for processing (21)
docs/cs/how-to/non-interactive-installation.mddocs/fr/how-to/install-bmad.mddocs/fr/how-to/non-interactive-installation.mddocs/how-to/install-bmad.mddocs/how-to/non-interactive-installation.mddocs/vi-vn/how-to/install-bmad.mddocs/vi-vn/how-to/non-interactive-installation.mddocs/zh-cn/how-to/install-bmad.mddocs/zh-cn/how-to/non-interactive-installation.mdtest/test-installation-components.jstools/installer/commands/install.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/modules/custom-modules.jstools/installer/modules/official-modules.jstools/installer/ui.js
💤 Files with no reviewable changes (7)
- tools/installer/commands/install.js
- tools/installer/core/install-paths.js
- tools/installer/core/manifest-generator.js
- test/test-installation-components.js
- tools/installer/core/custom-module-cache.js
- tools/installer/modules/custom-modules.js
- tools/installer/custom-handler.js
4 tasks
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
custom-handler.js,custom-module-cache.js,custom-modules.js(combined ~675 lines)--custom-contentCLI flag, interactive prompts, module caching, manifest tracking, and missing-source resolutionNet: -2,094 lines across 21 files
What is NOT touched
detectCustomFiles()in installer.js (preserves user-modified files during updates; different concept, coincidental naming)Test plan
npm testpasses (196 tests, lint, format)_config/custom/dirnpx bmad-method install --helpconfirms--custom-contentis gone