Conversation
The "None - Skip module installation" option was unnecessary since core is always locked/selected, satisfying the required constraint. Users can simply press Enter with only core selected to skip modules. Also removes dead code: selectModules(), getExternalModuleChoices(), and selectExternalModules() methods that were never called. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Module installer loading now handles three cases: - .cjs files loaded via require() (always CommonJS regardless of package type) - .js files loaded via dynamic import() (works for both CJS and ESM) - CJS default export unwrapped automatically for consistent API This fixes errors when external modules set "type":"module" in their package.json. Those modules must still rename installer.js to installer.cjs if it uses require() internally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🤖 Augment PR SummarySummary: Improves the CLI installer’s module selection UX and makes external module post-installers compatible with both CommonJS and ESM module packaging. Changes:
Technical Notes: This addresses external modules with 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughRefactors installer loading to prefer CommonJS ( Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(52,152,219,0.5)
participant CLI as CLI UI
end
rect rgba(46,204,113,0.5)
participant Manager as Installer Manager
end
rect rgba(241,196,15,0.5)
participant FS as Filesystem
end
rect rgba(155,89,182,0.5)
participant Module as Installer Module
end
CLI->>Manager: request runModuleInstaller(module)
Manager->>FS: check for installer.cjs and installer.js
FS-->>Manager: existence info
alt installer.cjs exists
Manager->>Module: require(installer.cjs)
else installer.js exists
Manager->>Module: import(pathToFileURL(installer.js))
else none
Manager-->>CLI: skip custom installer
end
Module-->>Manager: installer object (CJS or normalized ESM)
Manager->>Module: call installer.install(...) if present
Manager-->>CLI: continue post-install flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/cli/lib/ui.js (1)
268-277:⚠️ Potential issue | 🟡 MinorThe update path does not filter
'core'from CLI-provided--modules.In the new-install path (line 467),
selectedModulesis filtered to remove'core'. In the update path here,options.modulesparsing (line 270-274) can include'core', but it's passed directly to the return on line 373 without filtering. SinceinstallCore: trueis always set (line 372), having'core'in the modules list too could cause double-installation or unexpected behavior.Pre-existing, but the inconsistency is now more visible after this PR's filter changes.
Proposed fix
selectedModules = await this.selectAllModules(installedModuleIds); } + // Filter out core - it's always installed via installCore flag + selectedModules = selectedModules.filter((m) => m !== 'core'); + // After module selection, ask about custom modules
🤖 Fix all issues with AI agents
In `@tools/cli/installers/lib/modules/manager.js`:
- Around line 1280-1304: The dynamic-import branch incorrectly unwraps default
exports only when typeof imported.default === 'object', which misses
function/class defaults; change the unwrap logic in the else branch where
imported is obtained via import(pathToFileURL(installerPath).href) so that
moduleInstaller is set to imported.default when imported.default is defined
(e.g., imported.default !== undefined && imported.default !== null) and
otherwise set to imported; update references around moduleInstaller, imported
and installerPath accordingly so .install checks work for default-exported
functions/classes as well as objects.
🧹 Nitpick comments (1)
tools/cli/installers/lib/modules/manager.js (1)
1293-1298:require()result is not checked for the.cjspath — unlike the.jsbranch, there's no default-export unwrapping.If a
.cjsinstaller usesmodule.exports = { default: { install } }(e.g., transpiled from ESM),moduleInstaller.installwon't exist — you'd needmoduleInstaller.default.install. The.jsbranch handles this unwrapping, but the.cjsbranch doesn't. This is likely fine for hand-written CJS modules, but could bite if someone transpiles ESM to CJS.Low risk given the audience, but worth a defensive note or a shared unwrap step.
- Filter 'core' from CLI --modules in update path for consistency - Update selectAllModules() JSDoc to reflect core exclusion - Fix ESM default-export unwrap to handle function/class exports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change error display from log.error to log.warn and explain that the module was installed successfully — only the optional post-install script could not run. Prevents users from thinking the module installation itself failed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Post-install scripts fail due to CJS/ESM incompatibility but module files are already copied successfully. Silently catch the error instead of showing a warning that alarms users into thinking installation failed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tools/cli/installers/lib/modules/manager.js`:
- Around line 1330-1336: The empty catch block after calling
moduleInstaller.install() swallows all errors; change it to catch the error
(e.g., catch (err)) and log the failure in verbose/debug mode including the
error and context (module name and that it was a post-install step) so CJS/ESM
incompatibility remains silent by default but real runtime/IO errors are
diagnosable; keep the flow that post-install scripts are optional (do not
re-throw by default), but ensure the catch uses the existing verbose/logger API
(or console.debug if no logger available) to record err and moduleInstaller
information.
🧹 Nitpick comments (2)
tools/cli/lib/ui.js (1)
364-366: Redundantcorefilter —selectAllModulesalready strips it at Line 987.Lines 365, 470, and 987 all filter
m !== 'core'. The one at 987 insideselectAllModulesis the canonical location. The filters at 365 and 470 only matter for theoptions.modules(CLI) andoptions.yescode paths, which bypassselectAllModules. So they are necessary for those paths but redundant whenselectAllModulesis the source.Consider extracting a small helper (e.g.,
filterCoreModule(modules)) to centralize this, reducing the chance that a future code path forgets the filter.tools/cli/installers/lib/modules/manager.js (1)
1292-1304: Loading and execution share one try/catch — a load failure is indistinguishable from an install failure.Lines 1292–1328 load the module and call
.install()inside the sametry. If loading fails (e.g., syntax error in the installer file), the catch silently discards it. If.install()fails, same silent discard. Separating these would let you surface load errors (which likely indicate a packaging bug) while still tolerating install-script failures.Sketch
let moduleInstaller; + try { if (hasCjs) { moduleInstaller = require(installerPath); } else { const { pathToFileURL } = require('node:url'); const imported = await import(pathToFileURL(installerPath).href); moduleInstaller = imported.default == null ? imported : imported.default; } + } catch (loadError) { + if (process.env.BMAD_VERBOSE_INSTALL === 'true') { + await prompts.log.warn(` Could not load installer for ${moduleName}: ${loadError.message}`); + } + return; + } + try { if (typeof moduleInstaller.install === 'function') { // ... existing call ... } - } catch { + } catch (runError) { + if (process.env.BMAD_VERBOSE_INSTALL === 'true') { + await prompts.log.warn(` Post-install script for ${moduleName} failed: ${runError.message}`); + } }
The checkmark list already shows each installed module and IDE tool. Keep only the install path and file-warning lines in the summary footer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I saw the error described in #1592 and came to report it but found it's already fixed. Pulled this branch to a git worktree to try it out. Installed all the things for a few tools with no installer errors. QA Passed :) |
|
Thanks @wsmoak! Details are described at CIS: bmad-code-org/bmad-module-creative-intelligence-suite#4 |
3 participants
Summary
selectModules(),getExternalModuleChoices(), andselectExternalModules()methods that were never called.cjssupport toModuleManager.runModuleInstaller()so external modules with"type": "module"in theirpackage.jsoncan provide installers in either formatDetails
"None" option removal (
ui.js)The
autocompleteMultiselectprompt haslockedValues: ['core']andrequired: true, meaning core is always selected and cannot be unchecked. On fresh install, no other modules are pre-selected, so pressing Enter with only core selected already results in no modules being installed — making the "None" option redundant and potentially confusing.The
__NONE__sentinel value handling and override logic have been removed. Core filtering is now done insideselectAllModules()itself rather than at call sites.Dead code removal (
ui.js)Three methods were identified as unused (zero call sites):
selectModules()— superseded byselectAllModules()getExternalModuleChoices()— helper for the aboveselectExternalModules()— superseded byselectAllModules()ESM/CJS module installer support (
manager.js)runModuleInstaller()previously usedrequire()to load module installers. This fails when an external module'spackage.jsonsets"type": "module"(e.g., CIS), because Node.js treats.jsfiles as ESM whererequireis not defined.The loader now:
installer.cjsfirst (always loaded viarequire(), safe regardless of package type)installer.jsloaded via dynamicimport()(handles both CJS and ESM)Test plan
bmad install, select no extra modules (only core locked) → verify install completes with no modulesbmad installwith other module selected after renamesinstaller.js→installer.cjs→ verify CIS installer runs successfully