fix: use module-specific config.yaml paths in GitHub Copilot installer#1710
fix: use module-specific config.yaml paths in GitHub Copilot installer#1710Markus-Ende wants to merge 4 commits intobmad-code-org:mainfrom
Conversation
Replace hardcoded bmm/config.yaml references with dynamic module-based paths so custom modules load their own config.yaml instead of the non-existent bmm config. - createWorkflowPromptContent(): use entry.module from bmad-help.csv - createAgentActivatorPromptContent(): use artifact.module - createTechWriterPromptContent(): use entry.module for config and agent paths - generateCopilotInstructions(): dynamically list installed module paths Fixes bmad-code-org#1708
📝 WalkthroughWalkthroughThe PR modifies the GitHub Copilot IDE installer to replace hardcoded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/cli/installers/lib/ide/github-copilot.js (1)
334-340:⚠️ Potential issue | 🟡 MinorHardcoded
bmmfilenames should align with dynamic module handling.The
filevalues intechWriterCommands(bmad-bmm-write-document,bmad-bmm-update-standards, etc.) are hardcoded tobmmand used to generate prompt filenames, even though the method receivesentry.moduleand uses it dynamically for config and agent paths within the prompt content. This creates an inconsistency: the generated prompt files will always be named with thebmmsegment regardless of which module is being processed, potentially confusing for installations with custom or non-bmm modules.Either:
- Document these as stable identifiers intentionally decoupled from the module, or
- Make the filenames dynamic to reflect
entry.module(e.g.,bmad-${configModule}-write-document)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/github-copilot.js` around lines 334 - 340, The techWriterCommands object currently hardcodes filenames with the `bmm` segment which causes generated prompt filenames to be incorrect for other modules; update the code that constructs those filenames to use the actual module name (e.g., `entry.module` or the derived `configModule`) instead of the fixed `bmm`. Specifically, change the `file` values in `techWriterCommands` (or the logic that consumes them) to generate names like `bmad-${entry.module}-write-document` (and similar for update-standards, mermaid-generate, validate-document, explain-concept) so prompt filenames reflect the dynamic module being processed.
🧹 Nitpick comments (5)
tools/cli/installers/lib/ide/github-copilot.js (5)
405-406:generateCopilotInstructionsreads config vialoadModuleConfig(bmadDir)but doesn't passoptions— it can't resolve per-module config.
loadModuleConfig(line 406) receives onlybmadDirand has no visibility into which modules are selected. Meanwhile,options(which containsselectedModules) is available at this call site. IfloadModuleConfigis updated to be module-aware (per the earlier comment), it will needoptionsor the module list forwarded here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/github-copilot.js` around lines 405 - 406, generateCopilotInstructions currently calls loadModuleConfig(bmadDir) without passing options, so per-module config cannot be resolved; update the call in generateCopilotInstructions to forward the options (or at least options.selectedModules) to loadModuleConfig so that loadModuleConfig(bmadDir, options) (or loadModuleConfig(bmadDir, selectedModules)) can use the selected modules when resolving config—adjust the loadModuleConfig signature and any callers accordingly to accept and use the extra parameter.
488-494: "Core workflows" line (491) is now redundant whenworkflowDefsLinealready points atcore/workflows/.When no non-core modules are installed, line 458 sets
workflowDefsLinetocore/workflows/, and line 491 lists core workflows at the same path again. Users will see two bullets referencing the same directory. Consider either removing the static "Core workflows" bullet or adjustingworkflowDefsLineto exclude core in the else branch (replacing it with a no-op or omitting it).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/github-copilot.js` around lines 488 - 494, The static "Core workflows" bullet duplicates the path already output via workflowDefsLine when no non-core modules are installed; update the logic that sets workflowDefsLine (the else branch where workflowDefsLine is assigned) to avoid duplication by setting workflowDefsLine to an empty string or null when it would otherwise point to `${bmad}/core/workflows/`, and then ensure downstream rendering skips empty/null workflowDefsLine (or alternately remove the static "- **Core workflows**" bullet and rely solely on workflowDefsLine/agentDefsLine/moduleConfigLine); locate the assignment to workflowDefsLine and the static bullet text near the variables agentDefsLine, workflowDefsLine, moduleConfigLine and bmad to implement the change.
331-361:createTechWriterPromptContentis fragile — it hardcodes a fixed set of commands and assumes a rigid agent path structure.The command map (lines 334-340) is a static dictionary that must be manually synchronized with any changes in the tech-writer agent's menu. If a custom module adds or renames tech-writer commands, this function silently skips them (returns
null). There's no warning when an unrecognized tech-writer entry is encountered. At minimum, a log/warn for skipped entries would help diagnose silent failures during installation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/github-copilot.js` around lines 331 - 361, The createTechWriterPromptContent function currently uses a hardcoded techWriterCommands map and silently returns null when entry.name isn't found; update it to log a warning before returning null (e.g., emit this.logger.warn or console.warn including entry.name and configModule) so skipped/unknown tech-writer commands are visible, and optionally fall back to generating a generic prompt using entry.name when cmd is missing; modify the block around const cmd = techWriterCommands[entry.name]; if (!cmd) return null; to emit the warning (and/or create a generic cmd object) while still using escapeYamlSingleQuote, getToolsForFile, configModule and this.bmadFolderName downstream.
251-253: No validation onentry.module/artifact.modulevalues used in path construction.Module names sourced from CSV (
entry.module) and artifacts (artifact.module) are interpolated directly into path strings without any sanitization. While these typically come from controlled sources, a malformed or adversarial CSV entry (e.g.,module: ../../etc) would produce an invalid or misleading path in the generated prompt. This pattern repeats at lines 252, 349, and 383.A lightweight guard (e.g., stripping path separators or validating against a known module list) would harden this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/github-copilot.js` around lines 251 - 253, The code interpolates entry.module and artifact.module directly into paths (see configModule, configLine and other uses) which can produce unsafe/malformed paths; sanitize or validate these values before use by stripping path separators and traversal tokens (e.g., remove characters like ../, ./, path.sep) or, better, validate against a whitelist of known module names and fallback to 'core' if invalid, and apply the same guard to every occurrence that uses entry.module or artifact.module (including the other spots mentioned) so generated paths use only safe, verified module names.
446-467: Asymmetric handling of core paths across the three generated lines creates inconsistency and duplication.Three issues in this block:
agentDefsLine(line 448) includes core as a suffix (and \…/core/agents/` (core)), butworkflowDefsLine` (line 456) omits core entirely. Core workflows only appear later on line 491 under a separate "Core workflows" bullet. The two lines should follow the same pattern.When only core is installed,
workflowDefsLine(line 458) referencescore/workflows/under "Workflow definitions", and line 491 lists the same path under "Core workflows" — a duplication in the generated markdown.
moduleConfigLine(line 466) says"(no non-core modules installed)"which is odd wording for a user-facing document. Since line 494 already listscore/config.yamlunder "Core configuration", this line could simply be omitted when no non-core modules exist.Suggested approach
// Build workflow definitions line let workflowDefsLine; if (nonCoreModules.length > 0) { - workflowDefsLine = `- **Workflow definitions**: ${moduleWorkflowPaths} (organized by phase)`; + workflowDefsLine = `- **Workflow definitions**: ${moduleWorkflowPaths} and \`${bmad}/core/workflows/\` (core)`; } else { workflowDefsLine = `- **Workflow definitions**: \`${bmad}/core/workflows/\``; } // Build module configuration line let moduleConfigLine; if (nonCoreModules.length > 0) { moduleConfigLine = `- **Module configuration**: ${moduleConfigPaths}`; } else { - moduleConfigLine = `- **Module configuration**: (no non-core modules installed)`; + moduleConfigLine = ''; }Then conditionally include
moduleConfigLinein the template only if non-empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/github-copilot.js` around lines 446 - 467, The three generated lines are inconsistent and cause duplication; make them consistent by: (1) build workflowDefsLine exactly like agentDefsLine — when nonCoreModules.length > 0 set workflowDefsLine = `- **Workflow definitions**: ${moduleWorkflowPaths} and \`${bmad}/core/workflows/\` (core)`, otherwise set workflowDefsLine to an empty string (so core-only cases don't duplicate the core path elsewhere); (2) set moduleConfigLine to `- **Module configuration**: ${moduleConfigPaths}` when nonCoreModules.length > 0 and to an empty string when there are no non-core modules; and (3) update the template render to include agentDefsLine, workflowDefsLine, and moduleConfigLine only if they are non-empty — use the existing symbols agentDefsLine, workflowDefsLine, moduleConfigLine, nonCoreModules, moduleAgentPaths, moduleWorkflowPaths, moduleConfigPaths, and bmad to locate and change the logic.
🤖 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/cli/installers/lib/ide/github-copilot.js`:
- Line 470: Remove the dead declaration primaryModule and update related logic:
replace the hardcoded "bmm" in loadModuleConfig with the actual module variable
used when constructing module paths (use the module name passed to
loadModuleConfig), make workflowDefsLine include "core" consistently (not
exclude it when nonCoreModules exists) to match agentDefsLine, change
moduleConfigLine text so it does not misleadingly say "(no non-core modules
installed)" when core config is listed (show accurate wording or list only
non-core when present), and add validation checks for entry.module,
artifact.module, and items of selectedModules before using them to construct
paths (ensure they are non-empty strings and safe identifiers, and handle/skip
invalid names). Ensure references to loadModuleConfig, workflowDefsLine,
agentDefsLine, moduleConfigLine, entry.module, artifact.module, and
selectedModules are updated accordingly.
- Around line 348-357: The tech-writer agent path is hardcoded to bmm which
conflicts with module-agnostic use: update the code that builds the agent path
(currently using configModule and bmadFolderName) and the techWriterCommands
lookup so the tech-writer agent is resolved per-module (e.g., derive the agent
file path from entry.module/configModule or from a module agent
manifest/registry) instead of always using
"bmm/agents/tech-writer/tech-writer.md"; alternatively, if tech-writer is
intentionally BMM-only, make that explicit by constraining techWriterCommands to
BMM-only and document/enforce that path choice so other modules cannot register
tech-writer commands that would point to a non-existent file.
- Around line 408-412: selectedModules (from options.selectedModules) can
contain duplicates causing nonCoreModules to include duplicate paths;
deduplicate selectedModules before computing installedModules/nonCoreModules
(e.g., replace options.selectedModules usage with a deduplicated array via a Set
or similar) so installedModules and nonCoreModules only contain unique module
names; update the logic around selectedModules, installedModules, and
nonCoreModules to use the deduplicated list so generated markdown no longer
contains repeated path strings.
---
Outside diff comments:
In `@tools/cli/installers/lib/ide/github-copilot.js`:
- Around line 334-340: The techWriterCommands object currently hardcodes
filenames with the `bmm` segment which causes generated prompt filenames to be
incorrect for other modules; update the code that constructs those filenames to
use the actual module name (e.g., `entry.module` or the derived `configModule`)
instead of the fixed `bmm`. Specifically, change the `file` values in
`techWriterCommands` (or the logic that consumes them) to generate names like
`bmad-${entry.module}-write-document` (and similar for update-standards,
mermaid-generate, validate-document, explain-concept) so prompt filenames
reflect the dynamic module being processed.
---
Nitpick comments:
In `@tools/cli/installers/lib/ide/github-copilot.js`:
- Around line 405-406: generateCopilotInstructions currently calls
loadModuleConfig(bmadDir) without passing options, so per-module config cannot
be resolved; update the call in generateCopilotInstructions to forward the
options (or at least options.selectedModules) to loadModuleConfig so that
loadModuleConfig(bmadDir, options) (or loadModuleConfig(bmadDir,
selectedModules)) can use the selected modules when resolving config—adjust the
loadModuleConfig signature and any callers accordingly to accept and use the
extra parameter.
- Around line 488-494: The static "Core workflows" bullet duplicates the path
already output via workflowDefsLine when no non-core modules are installed;
update the logic that sets workflowDefsLine (the else branch where
workflowDefsLine is assigned) to avoid duplication by setting workflowDefsLine
to an empty string or null when it would otherwise point to
`${bmad}/core/workflows/`, and then ensure downstream rendering skips empty/null
workflowDefsLine (or alternately remove the static "- **Core workflows**" bullet
and rely solely on workflowDefsLine/agentDefsLine/moduleConfigLine); locate the
assignment to workflowDefsLine and the static bullet text near the variables
agentDefsLine, workflowDefsLine, moduleConfigLine and bmad to implement the
change.
- Around line 331-361: The createTechWriterPromptContent function currently uses
a hardcoded techWriterCommands map and silently returns null when entry.name
isn't found; update it to log a warning before returning null (e.g., emit
this.logger.warn or console.warn including entry.name and configModule) so
skipped/unknown tech-writer commands are visible, and optionally fall back to
generating a generic prompt using entry.name when cmd is missing; modify the
block around const cmd = techWriterCommands[entry.name]; if (!cmd) return null;
to emit the warning (and/or create a generic cmd object) while still using
escapeYamlSingleQuote, getToolsForFile, configModule and this.bmadFolderName
downstream.
- Around line 251-253: The code interpolates entry.module and artifact.module
directly into paths (see configModule, configLine and other uses) which can
produce unsafe/malformed paths; sanitize or validate these values before use by
stripping path separators and traversal tokens (e.g., remove characters like
../, ./, path.sep) or, better, validate against a whitelist of known module
names and fallback to 'core' if invalid, and apply the same guard to every
occurrence that uses entry.module or artifact.module (including the other spots
mentioned) so generated paths use only safe, verified module names.
- Around line 446-467: The three generated lines are inconsistent and cause
duplication; make them consistent by: (1) build workflowDefsLine exactly like
agentDefsLine — when nonCoreModules.length > 0 set workflowDefsLine = `-
**Workflow definitions**: ${moduleWorkflowPaths} and \`${bmad}/core/workflows/\`
(core)`, otherwise set workflowDefsLine to an empty string (so core-only cases
don't duplicate the core path elsewhere); (2) set moduleConfigLine to `-
**Module configuration**: ${moduleConfigPaths}` when nonCoreModules.length > 0
and to an empty string when there are no non-core modules; and (3) update the
template render to include agentDefsLine, workflowDefsLine, and moduleConfigLine
only if they are non-empty — use the existing symbols agentDefsLine,
workflowDefsLine, moduleConfigLine, nonCoreModules, moduleAgentPaths,
moduleWorkflowPaths, moduleConfigPaths, and bmad to locate and change the logic.
- Deduplicate selectedModules to prevent duplicate paths in markdown output - Remove unused primaryModule variable (dead code) - Refactor loadModuleConfig to accept installedModules param instead of hardcoded 'bmm' - Make tech-writer BMM-only check explicit (entry.module !== 'bmm' returns null) - Add test/test-github-copilot-installer.js with comprehensive unit tests - Add test:copilot script to package.json and include in main test command
2 participants
What
Replace hardcoded
bmm/config.yamlreferences with dynamic module-based paths in the GitHub Copilot installer so custom modules load their own config.yaml.Why
When installing with
--custom-content, ALL generated agent prompts incorrectly referencebmm/config.yamlregardless of which module the agent belongs to. This breaks custom module installations that don't include bmm.Fixes #1708
How
createWorkflowPromptContent(): useentry.modulefrom bmad-help.csvcreateAgentActivatorPromptContent(): useartifact.modulecreateTechWriterPromptContent(): useentry.modulefor config and agent pathsgenerateCopilotInstructions(): dynamically list installed module paths instead of hardcoding bmmTesting
--modules core --custom-contentand verified:bmm/config.yamlin generated promptscore/config.yamlcopilot-instructions.mddynamically lists custom module pathsnpm run validate:schemaspassesnpm run validate:refspasses (pre-existing broken refs unrelated)npx eslintpasses on modified file{{module}}