Fix: --custom-content flag and workflow config.yaml copying#1651
Fix: --custom-content flag and workflow config.yaml copying#1651bmadcode merged 7 commits intobmad-code-org:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tools/cli/lib/ui.js (3)
330-342:⚠️ Potential issue | 🟡 MinorNo guard against duplicate module codes across custom content paths.
If a user passes
--custom-content pathA,pathBand both contain amodule.yamlwith the samecodevalue, the module ID is pushed twice intoselectedModuleIdsandsources. This could cause duplicate installation attempts or silent conflicts in the installer. The interactive flow (promptCustomContentSource) has no such guard either, but the CLI path is more susceptible since users may pass overlapping directories.Consider deduplicating by
moduleMeta.code:🛡️ Proposed guard
+ if (selectedModuleIds.includes(moduleMeta.code)) { + await prompts.log.warn(`Skipping duplicate custom module: ${moduleMeta.code} from ${customPath}`); + continue; + } + customPaths.push(expandedPath); selectedModuleIds.push(moduleMeta.code);
318-333:⚠️ Potential issue | 🟡 Minor
yaml.parse()can returnnullfor empty YAML, causing an uncaughtTypeErroron line 330.If
module.yamlis empty or contains only---,yaml.parse()returnsnull. The subsequentmoduleMeta.codeaccess (line 330) sits outside the try/catch block and will throwTypeError: Cannot read properties of null. This is a pre-existing gap but lives squarely in the code path you're touching.🛡️ Proposed guard
moduleMeta = yaml.parse(moduleYaml); } catch (error) { await prompts.log.warn(`Skipping custom content path: ${customPath} - failed to read module.yaml: ${error.message}`); continue; } - if (!moduleMeta.code) { + if (!moduleMeta || !moduleMeta.code) { await prompts.log.warn(`Skipping custom content path: ${customPath} - module.yaml missing 'code' field`); continue; }Apply the same fix in the new-install block (line 480).
344-356:⚠️ Potential issue | 🟡 MinorRemove the redundant
pathsfield from the custom content config.The
pathsfield included at lines 345-356 is never referenced anywhere in the codebase. The installer only consumessources,selectedFiles,selected, andhasCustomContent— all already present in the config. Additionally, the interactive flow (promptCustomContentSource) does not include apathsfield, creating shape inconsistency between the CLI and interactive code paths. SinceselectedFilesalready contains the full file paths to eachmodule.yaml, thepathsfield is redundant and should be removed.
🧹 Nitpick comments (2)
tools/cli/lib/ui.js (2)
297-356: Massive code duplication between modify-flow and new-install flow.Lines 297–356 (modify/update path) and lines 447–503 (new-install path) are near-identical copy-paste blocks for building
customContentConfigfromoptions.customContent. This violates DRY and means any future bug fix or shape change must be applied in two places — exactly the kind of drift that caused this bug in the first place.Extract a shared helper, e.g.
buildCustomContentConfigFromPaths(paths), that returns{ customPaths, selectedModuleIds, sources, customContentConfig }.♻️ Sketch of extracted helper
// Add as a method on the UI class: async _buildCustomContentConfig(rawPaths) { const customPaths = []; const selectedModuleIds = []; const sources = []; for (const customPath of rawPaths) { const expandedPath = this.expandUserPath(customPath); const validation = this.validateCustomContentPathSync(expandedPath); if (validation) { await prompts.log.warn(`Skipping invalid custom content path: ${customPath} - ${validation}`); continue; } let moduleMeta; try { const moduleYamlPath = path.join(expandedPath, 'module.yaml'); const moduleYaml = await fs.readFile(moduleYamlPath, 'utf-8'); const yaml = require('yaml'); moduleMeta = yaml.parse(moduleYaml); } catch (error) { await prompts.log.warn(`Skipping custom content path: ${customPath} - failed to read module.yaml: ${error.message}`); continue; } if (!moduleMeta.code) { await prompts.log.warn(`Skipping custom content path: ${customPath} - module.yaml missing 'code' field`); continue; } customPaths.push(expandedPath); selectedModuleIds.push(moduleMeta.code); sources.push({ path: expandedPath, id: moduleMeta.code, name: moduleMeta.name || moduleMeta.code, }); } if (customPaths.length === 0) { return { selectedCustomModules: [], customContentConfig: { hasCustomContent: false } }; } return { selectedCustomModules: selectedModuleIds, customContentConfig: { hasCustomContent: true, paths: customPaths, selectedModuleIds, sources, selected: true, selectedFiles: customPaths.map((p) => path.join(p, 'module.yaml')), }, }; }Then both call sites collapse to:
const result = await this._buildCustomContentConfig(paths); // use result.customContentConfig / result.selectedCustomModulesAlso applies to: 447-503
337-353: No automated test coverage for the CLI custom content config shape.This bug existed because the non-interactive config shape diverged from the interactive one, and no test caught it. The fix is correct, but without a test asserting the expected config shape from
--custom-content, this can silently regress again. Consider adding a unit test that exercises theoptions.customContentbranch and asserts the presence ofsources,selected, andselectedFilesin the returned config.
|
Is this duplicate of #1624 ? |
|
quite possibly a duplicate. |
|
Better one PR too many than one too few. |
cidemaxio
changed the title
…warn) and removed paths from the config to match promptCustomContentSource()
…h, add PR#1624 improvements to allow update installs to work using non-interactive mode
|
I included the changes from the referenced PR. I also made fixes so that update installs work.
|
3 participants
What
When using --custom-content, the installer now populates sources, selected, and selectedFiles in the custom content config so the installer can resolve custom module sources.
Why
The installer expects config.customContent.sources to build customModulePaths, but the --custom-content path only set paths and selectedModuleIds. That left customModulePaths empty and caused findModuleSource() to fail with "Source for module 'X' is not available."
Fixes #1623
How
Added a sources array with { path, id, name } for each custom module when building config from --custom-content
Set selected: true and selectedFiles (paths to each module.yaml) on the custom content config
Applied the same logic in both the modify flow and the new-install flow in tools/cli/lib/ui.js
Incorporated fixes from PR #1624: empty module.yaml handling (skip + warn) and removal of paths from the config to match promptCustomContentSource()
Fixed quick-update with --custom-content: pass through for re-caching, add defensive pathExists checks for cache entries, and fix workflow-level config.yaml copying in manager.js
Testing
Ran npx bmad-method install --directory . --modules bmm,bmb --custom-content /path/to/bmad-story-orchestration --tools claude-code,cursor --yes and confirmed the install completes and custom modules are installed. Re-running the same command (quick-update) also succeeds.