feat: copilot installer frontmatter improvements#1800
Open
skittlz444 wants to merge 3 commits intobmad-code-org:mainfrom
Open
feat: copilot installer frontmatter improvements#1800skittlz444 wants to merge 3 commits intobmad-code-org:mainfrom
skittlz444 wants to merge 3 commits intobmad-code-org:mainfrom
Conversation
…installer Remove tools declaration from agent and prompt frontmatter since users manage their own tooling. Add name field to agents for cleaner @mention names and to prompts for cleaner /command display. Set agent field in prompts to the actual agent displayName for context continuity instead of resetting to default. Omit agent from prompts with no assigned agent. Remove now-unused getToolsForFile() and collectExistingToolPermissions() methods and related tool-permission preservation code from setup(). Fixes bmad-code-org#1794 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use raw agent names (e.g., 'dev', 'pm') for the name field in .agent.md frontmatter and all agent references in prompt frontmatter, instead of persona display names. This provides clean @mention and /command names. Resolve Create Story / Validate Story filename collision where both entries shared command 'bmad-bmm-create-story'. When multiple entries share a command, derive unique filenames from the entry name slug. Also pass workflow options (Create Mode, Validate Mode) to the prompt body so each prompt invokes the correct workflow mode. Fixes bmad-code-org#1794 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the GitHub Copilot installer by addressing four issues: removing forced tool declarations from frontmatter, using clean identifiers for agent/prompt names, fixing filename collisions when multiple CSV entries share the same command slug, and adding agent assignments to prompt frontmatter.
Changes:
- Removed
toolsfrom all agent and prompt frontmatter, and deleted thecollectExistingToolPermissions()andgetToolsForFile()helper methods - Added
namefield to agent.agent.mdand prompt.prompt.mdfrontmatter using the raw agent name (e.g.,"dev","pm") instead of persona display names - Added duplicate command detection in
generatePromptFiles()to derive unique filenames from entry name slugs, and surfacedoptions(e.g., "Create Mode") as a numbered step in the prompt body
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughRefactored GitHub Copilot installer to remove tool declarations from agent and prompt generation, add explicit agent names to YAML front matter, enable agent field assignment in prompts, and introduce safer handling for duplicate workflow commands and copilot-instructions section replacement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tools/cli/installers/lib/ide/github-copilot.js (3)
449-456:⚠️ Potential issue | 🟡 MinorMarkdown table values not sanitized—pipe characters break layout.
Values from the manifest (displayName, title, capabilities) are interpolated directly into the Markdown table. If any field contains
|or newlines, the table structure is corrupted.🛡️ Proposed sanitization
+ /** + * Sanitize value for Markdown table cell + */ + sanitizeTableCell(value) { + return (value || '').replace(/\|/g, '\\|').replace(/[\r\n]+/g, ' '); + } + // In generateCopilotInstructions: for (const agentName of agentOrder) { const meta = agentManifest.get(agentName); if (meta) { - const capabilities = meta.capabilities || 'agent capabilities'; - const cleanTitle = (meta.title || '').replaceAll('""', '"'); - agentsTable += `| ${agentName} | ${meta.displayName} | ${cleanTitle} | ${capabilities} |\n`; + const capabilities = this.sanitizeTableCell(meta.capabilities || 'agent capabilities'); + const cleanTitle = this.sanitizeTableCell((meta.title || '').replaceAll('""', '"')); + const displayName = this.sanitizeTableCell(meta.displayName); + agentsTable += `| ${agentName} | ${displayName} | ${cleanTitle} | ${capabilities} |\n`; } }🤖 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 449 - 456, Table cells are not sanitized: values from agentManifest (meta.displayName, meta.title, meta.capabilities) may contain pipe characters or newlines and will break the Markdown table; update the loop that builds agentsTable (iterating agentOrder, using agentManifest and cleanTitle) to sanitize each interpolated field by replacing pipe characters with an escaped "\|" and collapsing newlines into spaces (or trimming them), apply this to meta.displayName, the computed cleanTitle, and capabilities before concatenating into agentsTable so the Markdown row remains valid.
586-591:⚠️ Potential issue | 🟠 MajorCleanup pattern is overly broad—may delete user files.
The pattern
file.startsWith('bmad')matches any file beginning with "bmad", not just BMAD-generated agents. A user file likebmad-project-notes.mdorbmadness.mdin the agents directory would be incorrectly deleted.🛡️ Proposed tighter matching
for (const file of files) { - if (file.startsWith('bmad') && (file.endsWith('.agent.md') || file.endsWith('.md'))) { + // Match only BMAD-generated patterns: bmad-*.agent.md or bmm-*.agent.md + if ((file.startsWith('bmad-') || file.startsWith('bmm-')) && file.endsWith('.agent.md')) { await fs.remove(path.join(agentsDir, file)); removed++; }🤖 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 586 - 591, The deletion loop is using file.startsWith('bmad') which is too broad and can remove user files; update the check in the loop that iterates "for (const file of files)" to only match BMAD-generated agent filenames (e.g. require the "bmad-" prefix and the allowed extensions) by replacing file.startsWith('bmad') with a stricter test such as a regex like /^bmad-[^/]+(\.agent\.md|\.md)$/ or at minimum file.startsWith('bmad-') && (file.endsWith('.agent.md') || file.endsWith('.md')), so the removal (fs.remove(path.join(agentsDir, file)); and increment of removed) only runs for legitimately named BMAD agent files.
520-526:⚠️ Potential issue | 🟡 MinorBackup logic has race conditions and overwrites existing backups.
- If
.bakalready exists from a previous run, it's silently overwritten—the user loses that backup.- No error handling around
fs.copy; if backup fails, the original is still overwritten.- Race condition: another process could modify the file between
fs.copyandwriteFile.🛡️ Proposed safer backup handling
} else { // Existing file without markers — back it up before overwriting const backupPath = `${instructionsPath}.bak`; + // Avoid overwriting existing backup + if (await fs.pathExists(backupPath)) { + const timestamp = Date.now(); + backupPath = `${instructionsPath}.${timestamp}.bak`; + } - await fs.copy(instructionsPath, backupPath); + try { + await fs.copy(instructionsPath, backupPath); + } catch (err) { + if (!options.silent) await prompts.log.warn(` Failed to backup copilot-instructions.md: ${err.message}`); + return; // Don't overwrite if backup failed + } if (!options.silent) await prompts.log.warn(` Backed up copilot-instructions.md → .bak`);🤖 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 520 - 526, The current backup path `${instructionsPath}.bak` is overwritten and backup/copy has no error handling or atomic replace; change the backup logic in the block that references instructionsPath, backupPath, fs.copy and this.writeFile so it: 1) checks for an existing backup and chooses a unique backup name (e.g., append a timestamp or increment like .bak.1) instead of silently overwriting; 2) wrap the backup operation (fs.copy/fs.copyFile) in try/catch and if the copy fails log the error via prompts.log.warn or process the error and abort without calling this.writeFile; and 3) avoid the race by writing the new content to a temp file and then atomically replacing the original (fs.rename or platform-appropriate atomic replace) so there’s no window between backup and write where another process can cause corruption.
🧹 Nitpick comments (5)
tools/cli/installers/lib/ide/github-copilot.js (5)
165-168: Inconsistent YAML escaping and potential multiline value issue.Line 167 uses inline
replaceAll("'", "''")while other places use theescapeYamlSingleQuotehelper. More critically, thedescriptionbuilt from manifest fields (line 154) could contain newlines or YAML special characters that would break the single-quoted YAML value.♻️ Proposed fix for consistent escaping
+ // Sanitize description: remove newlines and use consistent escaping + const sanitizedDescription = description.replace(/[\r\n]+/g, ' ').trim(); + const safeDescription = this.escapeYamlSingleQuote(sanitizedDescription); + return `--- name: '${safeName}' -description: '${description.replaceAll("'", "''")}' +description: '${safeDescription}' ---🤖 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 165 - 168, The YAML generation currently uses inline replaceAll on description instead of the existing escape helper and doesn't handle multiline or special YAML characters; change the template to use the escapeYamlSingleQuote helper for both safeName and description (e.g., call escapeYamlSingleQuote(safeName) and escapeYamlSingleQuote(description)) and ensure the helper properly converts single quotes and folds or preserves newlines (or replaces newlines with '\n') so multi-line manifest-derived descriptions won't break the single-quoted YAML value in the function that returns the template string.
666-673: Inconsistent file write method and unhandled backup removal errors.Line 668 uses
fs.writeFiledirectly while the rest of the class usesthis.writeFile. This bypasses any tracking or consistent behavior the base class method might provide. Additionally,fs.removeat line 672 has no error handling—if removal fails (permissions, file lock), it's silently ignored.♻️ Proposed consistency fix
} else { // Write cleaned content back (preserve original whitespace) - await fs.writeFile(instructionsPath, cleaned, 'utf8'); + await this.writeFile(instructionsPath, cleaned); // If backup exists, it's stale now — remove it if (await fs.pathExists(backupPath)) { - await fs.remove(backupPath); + try { + await fs.remove(backupPath); + } catch { + // Non-critical: stale backup remains + } } }🤖 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 666 - 673, Replace the direct fs.writeFile call with the class's this.writeFile(instructionsPath, cleaned, 'utf8') to preserve any instrumentation/behavior provided by the base class (references: instructionsPath, cleaned, this.writeFile). Also wrap the backup removal in a try/catch and call await fs.remove(backupPath) inside the try, logging or handling the error in the catch (references: backupPath, fs.remove) so failures (permissions/locks) aren't silently ignored; ensure the catch uses the class logger or rethrows as appropriate for existing error handling conventions.
379-382: Hardcoded tech-writer agent path is fragile.The path
bmm/agents/tech-writer/tech-writer.mdis hardcoded while other parts of the installer derive agent paths dynamically from artifacts. If the tech-writer agent is relocated or renamed, this will silently produce broken prompts.Consider passing the tech-writer artifact or deriving the path from the agentArtifacts array to maintain consistency with how other agent paths are resolved.
🤖 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 379 - 382, The code hardcodes the tech-writer agent path ("bmm/agents/tech-writer/tech-writer.md"), which is brittle; update the logic that loads the tech-writer file to derive the path from the existing agentArtifacts array (or accept the tech-writer artifact as an argument) instead of the literal string: locate where the string appears and replace it with a resolved path using this.bmadFolderName + artifact.relativePath (or use agentArtifacts.find(a => a.name === 'tech-writer') to get the correct artifact) so the loader uses the same dynamic resolution as other agents (refer to symbols agentArtifacts, this.bmadFolderName, and the code that executes entry.name / cmd.code).
108-110: Silent error swallowing hides CSV parsing failures.When
agent-manifest.csvis malformed, the error is silently caught and the function returns an empty Map. Users have no visibility into why agent metadata is missing. Consider logging a warning so users can diagnose CSV format issues.♻️ Proposed improvement
} catch { - // Gracefully degrade if manifest is unreadable/malformed + // Gracefully degrade if manifest is unreadable/malformed, but warn + if (this.options?.verbose) { + console.warn(`Warning: Could not parse agent-manifest.csv`); + } }The same applies to
loadBmadHelpat line 133.🤖 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 108 - 110, The catch blocks that silently swallow errors when parsing agent-manifest.csv (the try/catch around the CSV read/parse) and in loadBmadHelp should be changed to capture the error (catch (err)) and log a warning including the error message and context instead of doing nothing; locate the CSV-reading function and the loadBmadHelp function in github-copilot.js, replace empty catch blocks with a logger (e.g., console.warn or the module logger) that emits a clear message like "Failed to parse agent-manifest.csv" or "Failed to load BMAD help" plus the error stack/message so users can diagnose malformed CSV issues.
542-551: Config loading silently falls back on parse errors—may mask issues.If
bmm/config.yamlexists but contains invalid YAML, the error is silently caught andcore/config.yamlis used instead. The user has no indication their BMM config was ignored due to a syntax error.♻️ Proposed improvement
for (const configPath of [bmmConfigPath, coreConfigPath]) { if (await fs.pathExists(configPath)) { try { const content = await fs.readFile(configPath, 'utf8'); return yaml.parse(content) || {}; - } catch { - // Fall through to next config + } catch (err) { + // Only fall through if file doesn't exist or is empty, not on parse errors + if (err.name === 'YAMLParseError') { + console.warn(`Warning: Invalid YAML in ${configPath}: ${err.message}`); + } + // Fall through to next config } } }🤖 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 542 - 551, The current loop swallows YAML parse errors and silently falls back to the next config; update the catch block in the loop over bmmConfigPath/coreConfigPath to surface parse failures by logging a warning or error that includes the failing configPath and the caught error (e.g., include configPath and error.message from the yaml.parse failure) before continuing to the next file so users know when a config was ignored; reference the variables configPath, bmmConfigPath, coreConfigPath and the calls to fs.readFile and yaml.parse when making the change.
🤖 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`:
- Around line 291-300: The optionsInstruction uses a hardcoded "4" which
misnumbers steps when body has 2 or 3 steps; compute the next step number from
the generated body instead of using 4. Inspect the produced body (variable body)
to count existing numbered steps (e.g., count occurrences like /^\d+\./m or
detect presence of "3.") then set a stepNumber = existingCount + 1 and replace
the hardcoded "4" with that dynamic stepNumber when composing optionsInstruction
(variables: options, optionsInstruction, body, promptName).
- Around line 214-220: The slug generation can produce empty or colliding
filenames: guard by computing slug from entry.name then if slug === '' use a
safe fallback like sanitized command or 'untitled', and track generated slugs in
a Set to detect collisions; when a collision occurs (same slug already exists)
append a numeric suffix (e.g., -1, -2) to slug before building promptFileName so
promptFileName (used where commandCounts, entry.name, slug, promptFileName
appear) is always non-empty and unique.
- Around line 148-149: Validate artifact.name before assigning it to name:
ensure artifact.name is a non-empty string (trimmed) and either throw a clear
error or provide a safe fallback (e.g., use artifact.id or "unknown-agent") if
it's missing; update the assignment where const name = artifact.name is set to
perform this check and sanitize whitespace so downstream frontmatter and
`@-mentions` never receive an empty value.
- Around line 408-416: The returned prompt hardcodes "bmm/config.yaml" which
breaks non-BMM agents; inside createAgentActivatorPromptContent use
artifact.module to build the module path instead of the literal "bmm". Update
the string that reads "Load
{project-root}/${this.bmadFolderName}/bmm/config.yaml" to reference the
artifact.module (e.g.,
{project-root}/${this.bmadFolderName}/${artifact.module}/config.yaml) so agents
discovered by getAgentsFromBmad/agentArtifacts correctly load their module's
config.
---
Outside diff comments:
In `@tools/cli/installers/lib/ide/github-copilot.js`:
- Around line 449-456: Table cells are not sanitized: values from agentManifest
(meta.displayName, meta.title, meta.capabilities) may contain pipe characters or
newlines and will break the Markdown table; update the loop that builds
agentsTable (iterating agentOrder, using agentManifest and cleanTitle) to
sanitize each interpolated field by replacing pipe characters with an escaped
"\|" and collapsing newlines into spaces (or trimming them), apply this to
meta.displayName, the computed cleanTitle, and capabilities before concatenating
into agentsTable so the Markdown row remains valid.
- Around line 586-591: The deletion loop is using file.startsWith('bmad') which
is too broad and can remove user files; update the check in the loop that
iterates "for (const file of files)" to only match BMAD-generated agent
filenames (e.g. require the "bmad-" prefix and the allowed extensions) by
replacing file.startsWith('bmad') with a stricter test such as a regex like
/^bmad-[^/]+(\.agent\.md|\.md)$/ or at minimum file.startsWith('bmad-') &&
(file.endsWith('.agent.md') || file.endsWith('.md')), so the removal
(fs.remove(path.join(agentsDir, file)); and increment of removed) only runs for
legitimately named BMAD agent files.
- Around line 520-526: The current backup path `${instructionsPath}.bak` is
overwritten and backup/copy has no error handling or atomic replace; change the
backup logic in the block that references instructionsPath, backupPath, fs.copy
and this.writeFile so it: 1) checks for an existing backup and chooses a unique
backup name (e.g., append a timestamp or increment like .bak.1) instead of
silently overwriting; 2) wrap the backup operation (fs.copy/fs.copyFile) in
try/catch and if the copy fails log the error via prompts.log.warn or process
the error and abort without calling this.writeFile; and 3) avoid the race by
writing the new content to a temp file and then atomically replacing the
original (fs.rename or platform-appropriate atomic replace) so there’s no window
between backup and write where another process can cause corruption.
---
Nitpick comments:
In `@tools/cli/installers/lib/ide/github-copilot.js`:
- Around line 165-168: The YAML generation currently uses inline replaceAll on
description instead of the existing escape helper and doesn't handle multiline
or special YAML characters; change the template to use the escapeYamlSingleQuote
helper for both safeName and description (e.g., call
escapeYamlSingleQuote(safeName) and escapeYamlSingleQuote(description)) and
ensure the helper properly converts single quotes and folds or preserves
newlines (or replaces newlines with '\n') so multi-line manifest-derived
descriptions won't break the single-quoted YAML value in the function that
returns the template string.
- Around line 666-673: Replace the direct fs.writeFile call with the class's
this.writeFile(instructionsPath, cleaned, 'utf8') to preserve any
instrumentation/behavior provided by the base class (references:
instructionsPath, cleaned, this.writeFile). Also wrap the backup removal in a
try/catch and call await fs.remove(backupPath) inside the try, logging or
handling the error in the catch (references: backupPath, fs.remove) so failures
(permissions/locks) aren't silently ignored; ensure the catch uses the class
logger or rethrows as appropriate for existing error handling conventions.
- Around line 379-382: The code hardcodes the tech-writer agent path
("bmm/agents/tech-writer/tech-writer.md"), which is brittle; update the logic
that loads the tech-writer file to derive the path from the existing
agentArtifacts array (or accept the tech-writer artifact as an argument) instead
of the literal string: locate where the string appears and replace it with a
resolved path using this.bmadFolderName + artifact.relativePath (or use
agentArtifacts.find(a => a.name === 'tech-writer') to get the correct artifact)
so the loader uses the same dynamic resolution as other agents (refer to symbols
agentArtifacts, this.bmadFolderName, and the code that executes entry.name /
cmd.code).
- Around line 108-110: The catch blocks that silently swallow errors when
parsing agent-manifest.csv (the try/catch around the CSV read/parse) and in
loadBmadHelp should be changed to capture the error (catch (err)) and log a
warning including the error message and context instead of doing nothing; locate
the CSV-reading function and the loadBmadHelp function in github-copilot.js,
replace empty catch blocks with a logger (e.g., console.warn or the module
logger) that emits a clear message like "Failed to parse agent-manifest.csv" or
"Failed to load BMAD help" plus the error stack/message so users can diagnose
malformed CSV issues.
- Around line 542-551: The current loop swallows YAML parse errors and silently
falls back to the next config; update the catch block in the loop over
bmmConfigPath/coreConfigPath to surface parse failures by logging a warning or
error that includes the failing configPath and the caught error (e.g., include
configPath and error.message from the yaml.parse failure) before continuing to
the next file so users know when a config was ignored; reference the variables
configPath, bmmConfigPath, coreConfigPath and the calls to fs.readFile and
yaml.parse when making the change.
- Dynamically compute step number for options instruction instead of hardcoding 4 (fixes skipped step 3 for .md and .xml patterns) - Add validation guard for empty/undefined artifact.name in createAgentContent - Trim leading/trailing dashes from slugs, guard against empty and colliding slugs in duplicate command handling - Use artifact.module for config.yaml path in agent activator prompts instead of hardcoding bmm (core agents now reference core/config.yaml) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
What
Updated the GitHub Copilot installer to remove tool declarations from frontmatter, add clean agent names, resolve prompt filename collisions, and set correct agent assignments in prompt files.
Why
Tools in frontmatter forced users to manually edit every agent/prompt file to customize tooling, and broke on update. Agent names used persona display names (e.g., "Mary") instead of clean identifiers (e.g., "dev"), making @mentions and /commands
hard to read. Create Story and Validate Story shared the same command slug, causing one to silently overwrite the other. Fixes #1794
How
Testing
Ran the full test suite (test:schemas, test:refs, test:install, validate:schemas) — all 71 tests pass. Verified ESLint and Prettier both report clean on the modified file.
fixes #1794