From 9086546e2fdc15543784c74fe7dcd82b7a704616 Mon Sep 17 00:00:00 2001 From: jheyworth <8269695+jheyworth@users.noreply.github.com> Date: Sun, 26 Apr 2026 20:40:30 +0100 Subject: [PATCH 1/5] fix(installer): generate OpenCode / slash commands Adds .opencode/commands/.md pointer files for each installed skill so users can invoke skills directly (e.g. /bmad-quick-dev) instead of going through the /skills menu. - platform-codes.yaml: add commands_target_dir field for opencode - _config-driven.js: installCommandPointers() with skip-if-exists default, reserved-name collision guard, YAML-safe description quoting - _config-driven.js: cleanupCommandPointers() for symmetric uninstall - test-installation-components.js: extend OpenCode suite with assertions covering pointer creation, content, and idempotency OpenCode-only and opt-in via the new yaml field; other adapters unchanged. Refs #2267 Co-Authored-By: Claude Opus 4.7 (1M context) --- test/test-installation-components.js | 22 ++++ tools/installer/ide/_config-driven.js | 154 ++++++++++++++++++++++++ tools/installer/ide/platform-codes.yaml | 1 + 3 files changed, 177 insertions(+) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 4827afcbf..6a7909ca3 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -285,6 +285,10 @@ async function runTests() { const opencodeInstaller = platformCodes.platforms.opencode?.installer; assert(opencodeInstaller?.target_dir === '.agents/skills', 'OpenCode target_dir uses native skills path'); + assert( + opencodeInstaller?.commands_target_dir === '.opencode/commands', + 'OpenCode commands_target_dir is configured for / slash commands', + ); const tempProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-opencode-test-')); const installedBmadDir = await createTestBmadFixture(); @@ -301,6 +305,24 @@ async function runTests() { const skillFile = path.join(tempProjectDir, '.agents', 'skills', 'bmad-master', 'SKILL.md'); assert(await fs.pathExists(skillFile), 'OpenCode install writes SKILL.md directory output'); + // Command pointer assertions: a / slash command should exist + // for each installed skill so users can invoke skills directly without + // going through the /skills menu. + const commandFile = path.join(tempProjectDir, '.opencode', 'commands', 'bmad-master.md'); + assert(await fs.pathExists(commandFile), 'OpenCode install writes per-skill command pointer file'); + + const commandContent = await fs.readFile(commandFile, 'utf8'); + assert(commandContent.includes('@skills/bmad-master'), 'Command pointer body references the skill via @skills/'); + assert(commandContent.includes('description:'), 'Command pointer carries a description in YAML frontmatter'); + + // Idempotency: re-running install must not duplicate or rewrite pointers. + const result2 = await ideManager.setup('opencode', tempProjectDir, installedBmadDir, { + silent: true, + selectedModules: ['bmm'], + }); + assert(result2.success === true, 'Second OpenCode install succeeds (idempotent)'); + assert(await fs.pathExists(commandFile), 'Command pointer survives a second install pass'); + await fs.remove(tempProjectDir); await fs.remove(path.dirname(installedBmadDir)); } catch (error) { diff --git a/tools/installer/ide/_config-driven.js b/tools/installer/ide/_config-driven.js index 737e10862..0d12983e5 100644 --- a/tools/installer/ide/_config-driven.js +++ b/tools/installer/ide/_config-driven.js @@ -6,6 +6,43 @@ const csv = require('csv-parse/sync'); const { BMAD_FOLDER_NAME } = require('./shared/path-utils'); const { getInstalledCanonicalIds, isBmadOwnedEntry } = require('./shared/installed-skills'); +// Reserved OpenCode slash commands. A skill whose canonicalId collides with +// one of these is skipped during command-pointer generation so it doesn't +// shadow a built-in. +const RESERVED_OPENCODE_COMMANDS = new Set([ + 'review', + 'commit', + 'init', + 'help', + 'skills', + 'fast', + 'compact', + 'clear', + 'undo', + 'redo', + 'edit', + 'editor', + 'exit', + 'quit', + 'theme', + 'config', + 'model', + 'session', +]); + +// Wrap a description for safe insertion into single-line YAML frontmatter. +// Leaves plain values untouched; double-quotes (and escapes) anything that +// could break YAML parsing or span multiple lines. +function yamlSafeSingleLine(value) { + const collapsed = String(value) + .replaceAll(/[\r\n]+/g, ' ') + .trim(); + const needsQuoting = /[:#'"\\]/.test(collapsed) || /^[!&*?|>%@`]/.test(collapsed); + if (!needsQuoting) return collapsed; + const escaped = collapsed.replaceAll('\\', '\\\\').replaceAll('"', String.raw`\"`); + return `"${escaped}"`; +} + /** * Config-driven IDE setup handler * @@ -128,11 +165,76 @@ class ConfigDrivenIdeSetup { results.skills = await this.installVerbatimSkills(projectDir, bmadDir, targetPath, config); results.skillDirectories = this.skillWriteTracker.size; + if (config.commands_target_dir) { + results.commands = await this.installCommandPointers(projectDir, bmadDir, config, options); + } + await this.printSummary(results, target_dir, options); this.skillWriteTracker = null; return { success: true, results }; } + /** + * Generate per-skill command pointer files for IDEs that surface commands + * separately from skills (e.g. OpenCode's `.opencode/commands/.md`). + * + * Each pointer is a tiny markdown file whose body is `@skills/` + * so invoking `/` routes the user straight to the skill instead + * of forcing them through a `/skills` menu. + * + * Skips: + * - Names that collide with reserved built-in slash commands. + * - Existing files (treated as hand-tuned) unless options.forceCommands. + * + * @param {string} projectDir + * @param {string} bmadDir + * @param {Object} config - Installer config; reads commands_target_dir. + * @param {Object} options - Setup options. forceCommands overwrites existing files. + * @returns {Promise} { created, skippedExisting, skippedCollision, fallbackDescription } + */ + async installCommandPointers(projectDir, bmadDir, config, options = {}) { + const result = { created: 0, skippedExisting: 0, skippedCollision: 0, fallbackDescription: 0 }; + + const csvPath = path.join(bmadDir, '_config', 'skill-manifest.csv'); + if (!(await fs.pathExists(csvPath))) return result; + + const commandsPath = path.join(projectDir, config.commands_target_dir); + await fs.ensureDir(commandsPath); + + const csvContent = await fs.readFile(csvPath, 'utf8'); + const records = csv.parse(csvContent, { columns: true, skip_empty_lines: true }); + + for (const record of records) { + const canonicalId = record.canonicalId; + if (!canonicalId) continue; + + if (RESERVED_OPENCODE_COMMANDS.has(canonicalId)) { + result.skippedCollision++; + continue; + } + + const commandFile = path.join(commandsPath, `${canonicalId}.md`); + + if ((await fs.pathExists(commandFile)) && !options.forceCommands) { + result.skippedExisting++; + continue; + } + + let description = (record.description || '').trim(); + if (!description) { + description = `Run the ${canonicalId} skill`; + result.fallbackDescription++; + } + + const body = `---\ndescription: ${yamlSafeSingleLine(description)}\n---\n\n@skills/${canonicalId}\n`; + + await fs.writeFile(commandFile, body, 'utf8'); + result.created++; + } + + return result; + } + /** * Install verbatim native SKILL.md directories from skill-manifest.csv. * Copies the entire source directory as-is into the IDE skill directory. @@ -256,6 +358,13 @@ class ConfigDrivenIdeSetup { if (this.installerConfig?.target_dir) { await this.cleanupTarget(projectDir, this.installerConfig.target_dir, options, removalSet); } + + // Clean generated command pointer files in commands_target_dir. + // Mirrors target_dir cleanup so uninstalls and skill removals don't + // leave dangling / commands pointing at missing skills. + if (this.installerConfig?.commands_target_dir) { + await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet); + } } /** @@ -346,6 +455,51 @@ class ConfigDrivenIdeSetup { } } + /** + * Cleanup generated command pointer files for entries in removalSet. + * Symmetric counterpart to installCommandPointers — removes .md + * files whose canonicalId is in the set. Removes the commands directory + * entirely if it ends up empty. + * @param {string} projectDir + * @param {string} commandsTargetDir - Relative dir (e.g. .opencode/commands) + * @param {Object} options + * @param {Set} removalSet - canonicalIds whose pointer files to remove + */ + async cleanupCommandPointers(projectDir, commandsTargetDir, options = {}, removalSet = new Set()) { + if (!removalSet || removalSet.size === 0) return; + + const commandsPath = path.join(projectDir, commandsTargetDir); + if (!(await fs.pathExists(commandsPath))) return; + + let entries; + try { + entries = await fs.readdir(commandsPath); + } catch { + return; + } + + for (const entry of entries) { + if (typeof entry !== 'string' || !entry.endsWith('.md')) continue; + const canonicalId = entry.slice(0, -3); + if (!removalSet.has(canonicalId)) continue; + try { + await fs.remove(path.join(commandsPath, entry)); + } catch { + // Skip files we can't remove. + } + } + + // Remove the commands directory if we emptied it. + try { + const remaining = await fs.readdir(commandsPath); + if (remaining.length === 0) { + await fs.remove(commandsPath); + } + } catch { + // Directory may already be gone. + } + } + /** * Cleanup a specific target directory. * When removalSet is provided, only removes entries in that set. diff --git a/tools/installer/ide/platform-codes.yaml b/tools/installer/ide/platform-codes.yaml index 0f49a7fbe..78ca1d271 100644 --- a/tools/installer/ide/platform-codes.yaml +++ b/tools/installer/ide/platform-codes.yaml @@ -222,6 +222,7 @@ platforms: installer: target_dir: .agents/skills global_target_dir: ~/.agents/skills + commands_target_dir: .opencode/commands openhands: name: "OpenHands" From 81badb92cddc9cd1cf73ec107b923d187f754239 Mon Sep 17 00:00:00 2001 From: jheyworth <8269695+jheyworth@users.noreply.github.com> Date: Sun, 26 Apr 2026 21:45:45 +0100 Subject: [PATCH 2/5] fix(installer): address PR #2324 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six fixes from CodeRabbit + Augment review on the OpenCode command pointer generation: - skipTarget no longer suppresses installCommandPointers in multi-IDE shared-target_dir batches. Pointers live in a per-IDE directory and are not deduped across peers, so OpenCode must still generate them even when a peer (e.g. openhands) won the .agents/skills write race. - skipTarget no longer suppresses cleanupCommandPointers either, so partial uninstalls leave no stale pointers when a peer remains. - canonicalId is validated as a safe basename before being interpolated into a file path (defense in depth against a malformed manifest entry writing outside commands_target_dir). - yamlSafeSingleLine now quotes descriptions starting with `[` or `{` so YAML doesn't parse them as a sequence/map. - Per-record fs.writeFile failures are caught and counted (writeFailures) rather than aborting the whole IDE install — pointer files are a non-essential adjunct to the skill copy. - Generator-shaped pointer files are refreshed when the manifest description changes; hand-modified files (body diverges from the generator pattern) are still preserved unless forceCommands is set. Tests: extends Suite 8 with description-update propagation; adds new Suite 40c covering OpenCode + openhands batches in both orderings plus partial-IDE uninstall pointer cleanup. 308 tests pass (was 296). Co-Authored-By: Claude Opus 4.7 (1M context) --- test/test-installation-components.js | 123 +++++++++++++++++++++ tools/installer/ide/_config-driven.js | 153 ++++++++++++++++++++++---- 2 files changed, 253 insertions(+), 23 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 6a7909ca3..17379a20b 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -323,6 +323,22 @@ async function runTests() { assert(result2.success === true, 'Second OpenCode install succeeds (idempotent)'); assert(await fs.pathExists(commandFile), 'Command pointer survives a second install pass'); + // Description-update propagation: when the manifest description changes + // and the on-disk pointer still matches the generator pattern, refresh + // the file so users see the updated description. + const csvPath = path.join(installedBmadDir, '_config', 'skill-manifest.csv'); + const updatedCsv = + 'canonicalId,name,description,module,path\n' + + '"bmad-master","bmad-master","UPDATED description for the test agent","core","_bmad/core/bmad-master/SKILL.md"\n'; + await fs.writeFile(csvPath, updatedCsv); + const result3 = await ideManager.setup('opencode', tempProjectDir, installedBmadDir, { + silent: true, + selectedModules: ['bmm'], + }); + assert(result3.success === true, 'Third OpenCode install succeeds after description update'); + const refreshed = await fs.readFile(commandFile, 'utf8'); + assert(refreshed.includes('UPDATED description'), 'Generator-shaped pointer is refreshed when manifest description changes'); + await fs.remove(tempProjectDir); await fs.remove(path.dirname(installedBmadDir)); } catch (error) { @@ -2753,6 +2769,113 @@ async function runTests() { console.log(''); + // ============================================================ + // Test Suite 40c: OpenCode command pointers in multi-IDE batches + // ============================================================ + // Regression: when OpenCode is the *peer* in a setupBatch sharing + // .agents/skills (e.g. with openhands), the skill write is dedup-skipped + // but the per-IDE .opencode/commands/ pointers must still be generated. + // Symmetrically, partial uninstall while a peer remains must still clean + // up OpenCode's own command pointers. + console.log(`${colors.yellow}Test Suite 40c: OpenCode command pointers in shared-target batches${colors.reset}\n`); + + try { + clearCache(); + const platformCodes40c = await loadPlatformCodes(); + const opencodeTarget40c = platformCodes40c.platforms.opencode?.installer?.target_dir; + const openhandsTarget40c = platformCodes40c.platforms.openhands?.installer?.target_dir; + assert( + opencodeTarget40c === '.agents/skills' && openhandsTarget40c === '.agents/skills', + 'OpenCode and OpenHands share .agents/skills target_dir', + ); + + // Order A: opencode first → opencode is the writer. + const projA = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-opencode-batch-a-')); + const bmadA = await createTestBmadFixture(); + const mgrA = new IdeManager(); + await mgrA.ensureInitialized(); + const resultsA = await mgrA.setupBatch(['opencode', 'openhands'], projA, bmadA, { + silent: true, + selectedModules: ['core'], + }); + const cmdA = path.join(projA, '.opencode', 'commands', 'bmad-master.md'); + assert( + resultsA.every((r) => r.success === true), + 'opencode-first batch: all platforms succeed', + ); + assert(await fs.pathExists(cmdA), 'opencode-first batch: command pointer is created'); + + // Order B: openhands first → opencode is the peer (skipTarget=true). + // Without the fix, the early-return would bypass installCommandPointers. + const projB = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-opencode-batch-b-')); + const bmadB = await createTestBmadFixture(); + const mgrB = new IdeManager(); + await mgrB.ensureInitialized(); + const resultsB = await mgrB.setupBatch(['openhands', 'opencode'], projB, bmadB, { + silent: true, + selectedModules: ['core'], + }); + const cmdB = path.join(projB, '.opencode', 'commands', 'bmad-master.md'); + const opencodeResultB = resultsB.find((r) => r.ide === 'opencode'); + assert( + resultsB.every((r) => r.success === true), + 'openhands-first batch: all platforms succeed', + ); + assert( + opencodeResultB?.handlerResult?.results?.sharedTargetHandledByPeer === true, + 'openhands-first batch: opencode is marked sharedTargetHandledByPeer (skill write deduped)', + ); + assert(await fs.pathExists(cmdB), 'openhands-first batch: command pointer is generated even when skill write is deduped'); + + // Cleanup symmetry: uninstall opencode while openhands remains. + // Uses an in-project bmadDir so the cleanup path can compute removalSet + // from the manifest (the production layout). The cross-temp-dir fixture + // above can't exercise this — same constraint Test Suite 40 documents. + const projC = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-opencode-batch-c-')); + const bmadC = path.join(projC, '_bmad'); + await fs.ensureDir(path.join(bmadC, '_config')); + await fs.writeFile( + path.join(bmadC, '_config', 'skill-manifest.csv'), + 'canonicalId,name,description,module,path\n' + + '"bmad-master","bmad-master","Minimal test agent fixture","core","_bmad/core/bmad-master/SKILL.md"\n', + ); + const skillC = path.join(bmadC, 'core', 'bmad-master'); + await fs.ensureDir(skillC); + await fs.writeFile( + path.join(skillC, 'SKILL.md'), + ['---', 'name: bmad-master', 'description: Minimal test agent fixture', '---', '', 'You are a test agent.'].join('\n'), + ); + + const mgrC = new IdeManager(); + await mgrC.ensureInitialized(); + await mgrC.setupBatch(['openhands', 'opencode'], projC, bmadC, { + silent: true, + selectedModules: ['core'], + }); + const cmdC = path.join(projC, '.opencode', 'commands', 'bmad-master.md'); + assert(await fs.pathExists(cmdC), 'in-project fixture: pointer is generated for opencode peer'); + + const cleanupResultsC = await mgrC.cleanupByList(projC, ['opencode'], { + silent: true, + remainingIdes: ['openhands'], + }); + assert(cleanupResultsC[0].success !== false, 'opencode partial-uninstall reports success'); + const sharedSurvivesC = await fs.pathExists(path.join(projC, '.agents', 'skills', 'bmad-master', 'SKILL.md')); + assert(sharedSurvivesC, 'shared .agents/skills/ survives partial uninstall (peer still uses it)'); + assert(!(await fs.pathExists(cmdC)), 'opencode command pointer is removed on partial uninstall even when peer remains'); + + await fs.remove(projA).catch(() => {}); + await fs.remove(path.dirname(bmadA)).catch(() => {}); + await fs.remove(projB).catch(() => {}); + await fs.remove(path.dirname(bmadB)).catch(() => {}); + await fs.remove(projC).catch(() => {}); + } catch (error) { + console.log(`${colors.red}Test Suite 40c setup failed: ${error.message}${colors.reset}`); + failed++; + } + + console.log(''); + // ============================================================ // Test Suite 41: Custom-module skill ownership (non-bmad prefix) // ============================================================ diff --git a/tools/installer/ide/_config-driven.js b/tools/installer/ide/_config-driven.js index 0d12983e5..0ec652aae 100644 --- a/tools/installer/ide/_config-driven.js +++ b/tools/installer/ide/_config-driven.js @@ -37,12 +37,47 @@ function yamlSafeSingleLine(value) { const collapsed = String(value) .replaceAll(/[\r\n]+/g, ' ') .trim(); - const needsQuoting = /[:#'"\\]/.test(collapsed) || /^[!&*?|>%@`]/.test(collapsed); + const needsQuoting = /[:#'"\\]/.test(collapsed) || /^[!&*?|>%@`[{]/.test(collapsed); if (!needsQuoting) return collapsed; const escaped = collapsed.replaceAll('\\', '\\\\').replaceAll('"', String.raw`\"`); return `"${escaped}"`; } +// Validate that a canonicalId is a safe basename — no path separators, no +// parent-dir traversal, no leading dots, only the character set we expect. +// Defense-in-depth: the manifest is trusted today, but the value flows +// directly into a file path and a malformed entry should not write outside +// the commands directory. +function isSafeCanonicalId(value) { + return typeof value === 'string' && /^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/.test(value) && !value.includes('..'); +} + +// The exact body the installer would generate for a given description and +// canonicalId. Centralised so both the write and the freshness-check paths +// agree on the canonical form. +function buildCommandPointerBody(description, canonicalId) { + return `---\ndescription: ${yamlSafeSingleLine(description)}\n---\n\n@skills/${canonicalId}\n`; +} + +// Heuristic: does an existing pointer file look like our generator's output +// (and therefore safe to refresh) versus a user-modified file (which we +// preserve)? We check the body shape rather than full equality so that +// description-only edits in the manifest can propagate without trampling +// hand edits to the body. +function looksLikeGeneratorOutput(content, canonicalId) { + if (typeof content !== 'string') return false; + const trimmed = content.trim(); + // Must end with the exact reference line our generator writes. + if (!trimmed.endsWith(`@skills/${canonicalId}`)) return false; + // Must start with frontmatter containing exactly one description: line. + const fmMatch = trimmed.match(/^---\n([\S\s]*?)\n---\n/); + if (!fmMatch) return false; + const fmLines = fmMatch[1].split('\n').filter((l) => l.length > 0); + if (fmLines.length !== 1) return false; + if (!fmLines[0].startsWith('description:')) return false; + return true; +} + /** * Config-driven IDE setup handler * @@ -134,9 +169,15 @@ class ConfigDrivenIdeSetup { } // When a peer platform in the same install batch owns this target_dir, - // skip the skill write — the peer has already populated it. + // skip the skill write — the peer has already populated it. Command + // pointers, however, write to a separate per-IDE directory and must + // still be generated for this IDE; they are not deduped across peers. if (options.skipTarget) { - return { success: true, results: { skills: 0, sharedTargetHandledByPeer: true } }; + const results = { skills: 0, sharedTargetHandledByPeer: true }; + if (this.installerConfig.commands_target_dir) { + results.commands = await this.installCommandPointers(projectDir, bmadDir, this.installerConfig, options); + } + return { success: true, results }; } if (this.installerConfig.target_dir) { @@ -184,16 +225,33 @@ class ConfigDrivenIdeSetup { * * Skips: * - Names that collide with reserved built-in slash commands. - * - Existing files (treated as hand-tuned) unless options.forceCommands. + * - canonicalIds that aren't safe basename-only identifiers (defense + * against path traversal even though the manifest is currently trusted). + * - Existing files whose body looks user-modified (preserves hand edits); + * pointer files matching the generator pattern get overwritten so that + * description changes in skill-manifest.csv propagate on re-install. + * + * Per-file write failures are recorded and reported but do not abort the + * rest of the install — pointer files are a non-essential adjunct to the + * skill copy that already succeeded. * * @param {string} projectDir * @param {string} bmadDir * @param {Object} config - Installer config; reads commands_target_dir. - * @param {Object} options - Setup options. forceCommands overwrites existing files. - * @returns {Promise} { created, skippedExisting, skippedCollision, fallbackDescription } + * @param {Object} options - Setup options. forceCommands overwrites existing + * files unconditionally (including hand-modified ones). + * @returns {Promise} { created, updated, skippedExisting, skippedCollision, skippedInvalidId, writeFailures, fallbackDescription } */ async installCommandPointers(projectDir, bmadDir, config, options = {}) { - const result = { created: 0, skippedExisting: 0, skippedCollision: 0, fallbackDescription: 0 }; + const result = { + created: 0, + updated: 0, + skippedExisting: 0, + skippedCollision: 0, + skippedInvalidId: 0, + writeFailures: 0, + fallbackDescription: 0, + }; const csvPath = path.join(bmadDir, '_config', 'skill-manifest.csv'); if (!(await fs.pathExists(csvPath))) return result; @@ -208,15 +266,16 @@ class ConfigDrivenIdeSetup { const canonicalId = record.canonicalId; if (!canonicalId) continue; - if (RESERVED_OPENCODE_COMMANDS.has(canonicalId)) { - result.skippedCollision++; + // Defensive basename validation. canonicalId comes from a trusted + // manifest today, but the value flows directly into a file path — + // reject anything that could escape commands_target_dir. + if (!isSafeCanonicalId(canonicalId)) { + result.skippedInvalidId++; continue; } - const commandFile = path.join(commandsPath, `${canonicalId}.md`); - - if ((await fs.pathExists(commandFile)) && !options.forceCommands) { - result.skippedExisting++; + if (RESERVED_OPENCODE_COMMANDS.has(canonicalId)) { + result.skippedCollision++; continue; } @@ -226,10 +285,54 @@ class ConfigDrivenIdeSetup { result.fallbackDescription++; } - const body = `---\ndescription: ${yamlSafeSingleLine(description)}\n---\n\n@skills/${canonicalId}\n`; + const body = buildCommandPointerBody(description, canonicalId); + const commandFile = path.join(commandsPath, `${canonicalId}.md`); - await fs.writeFile(commandFile, body, 'utf8'); - result.created++; + // If a pointer file already exists, decide whether to overwrite based + // on whether it looks like generator output (description-only diff) or + // a user-modified file. forceCommands overrides this protection. + if (!options.forceCommands && (await fs.pathExists(commandFile))) { + let existing; + try { + existing = await fs.readFile(commandFile, 'utf8'); + } catch { + // Treat unreadable as user-owned and skip — safer than overwriting. + result.skippedExisting++; + continue; + } + + if (existing === body) { + // No-op idempotent re-run. + result.skippedExisting++; + continue; + } + if (looksLikeGeneratorOutput(existing, canonicalId)) { + // Description (or other generated bit) has changed; refresh in place. + try { + await fs.writeFile(commandFile, body, 'utf8'); + result.updated++; + } catch (error) { + result.writeFailures++; + if (!options.silent) { + await prompts.log.warn(`Failed to update command pointer ${canonicalId}.md: ${error.message}`); + } + } + continue; + } + // Hand-modified pointer — preserve it. + result.skippedExisting++; + continue; + } + + try { + await fs.writeFile(commandFile, body, 'utf8'); + result.created++; + } catch (error) { + result.writeFailures++; + if (!options.silent) { + await prompts.log.warn(`Failed to write command pointer ${canonicalId}.md: ${error.message}`); + } + } } return result; @@ -349,6 +452,17 @@ class ConfigDrivenIdeSetup { await this.cleanupRovoDevPrompts(projectDir, options); } + // Clean generated command pointer files in commands_target_dir. + // Mirrors target_dir cleanup so uninstalls and skill removals don't + // leave dangling / commands pointing at missing skills. + // Runs regardless of skipTarget — command pointers live in a per-IDE + // directory and are not deduped across peers, so a peer-owned shared + // skills directory does not protect this IDE's command pointers from + // cleanup. + if (this.installerConfig?.commands_target_dir) { + await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet); + } + // Skip target_dir cleanup when a peer platform owns this directory // (set during dedup'd install or when uninstalling one of several // platforms that share the same target_dir). @@ -358,13 +472,6 @@ class ConfigDrivenIdeSetup { if (this.installerConfig?.target_dir) { await this.cleanupTarget(projectDir, this.installerConfig.target_dir, options, removalSet); } - - // Clean generated command pointer files in commands_target_dir. - // Mirrors target_dir cleanup so uninstalls and skill removals don't - // leave dangling / commands pointing at missing skills. - if (this.installerConfig?.commands_target_dir) { - await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet); - } } /** From 46a3d854f360343f18ede8b3519749d3b6732f38 Mon Sep 17 00:00:00 2001 From: jheyworth <8269695+jheyworth@users.noreply.github.com> Date: Sun, 26 Apr 2026 22:02:59 +0100 Subject: [PATCH 3/5] fix(installer): address PR #2324 follow-up nitpicks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four nitpicks from CodeRabbit's original review that were missed in the first triage pass: - Hand-edited pointers now survive the production install flow. cleanupCommandPointers spares pointers for canonicalIds that are still in the new manifest when called from the install/update flow (signal: options.previousSkillIds is set). Uninstall and partial-IDE removal flows still wipe pointers as before. The previous behavior wiped every pointer in removalSet before installCommandPointers could run, so its skip-if-exists guard never fired and hand edits were lost on every reinstall — contradicting the docstring's preservation claim. - RESERVED_OPENCODE_COMMANDS is now gated on this.name === 'opencode' so future adapters opting into commands_target_dir don't silently inherit OpenCode's reserved-name set. - printSummary now surfaces results.commands so users see how many pointers were created/refreshed/skipped per install, plus a warning for any per-file write failures. - Dropped a dead `typeof entry !== 'string'` check; fs.readdir without withFileTypes always yields strings. Tests: extends Suite 8 with a hand-edit-preservation regression that calls setup with previousSkillIds (the production shape) and asserts a sentinel byte sequence in the pointer body survives. 310 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/test-installation-components.js | 15 ++++++ tools/installer/ide/_config-driven.js | 71 +++++++++++++++++++++++++-- 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 17379a20b..a687def21 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -339,6 +339,21 @@ async function runTests() { const refreshed = await fs.readFile(commandFile, 'utf8'); assert(refreshed.includes('UPDATED description'), 'Generator-shaped pointer is refreshed when manifest description changes'); + // Hand-edit preservation across the production install flow. The + // installer passes previousSkillIds — without the cleanup-side spare, + // hand edits would be wiped here. + const SENTINEL = 'HAND_EDITED_BY_USER_SHOULD_SURVIVE'; + const handEditedBody = `---\ndescription: my custom description\n---\n\n${SENTINEL}\n`; + await fs.writeFile(commandFile, handEditedBody); + const result4 = await ideManager.setup('opencode', tempProjectDir, installedBmadDir, { + silent: true, + selectedModules: ['bmm'], + previousSkillIds: new Set(['bmad-master']), + }); + assert(result4.success === true, 'Fourth OpenCode install succeeds with hand-edited pointer present'); + const afterReinstall = await fs.readFile(commandFile, 'utf8'); + assert(afterReinstall.includes(SENTINEL), 'Hand-edited pointer survives a routine reinstall (cleanup spares active-manifest IDs)'); + await fs.remove(tempProjectDir); await fs.remove(path.dirname(installedBmadDir)); } catch (error) { diff --git a/tools/installer/ide/_config-driven.js b/tools/installer/ide/_config-driven.js index 0ec652aae..ad3fafd7a 100644 --- a/tools/installer/ide/_config-driven.js +++ b/tools/installer/ide/_config-driven.js @@ -274,7 +274,10 @@ class ConfigDrivenIdeSetup { continue; } - if (RESERVED_OPENCODE_COMMANDS.has(canonicalId)) { + // Reserved-name guard is OpenCode-specific. Other adapters that opt + // into commands_target_dir later should declare their own reserved + // set rather than inheriting OpenCode's. + if (this.name === 'opencode' && RESERVED_OPENCODE_COMMANDS.has(canonicalId)) { result.skippedCollision++; continue; } @@ -412,6 +415,18 @@ class ConfigDrivenIdeSetup { if (count > 0) { await prompts.log.success(`${this.name} configured: ${count} skills → ${targetDir}`); } + const cmd = results.commands; + if (cmd && (cmd.created > 0 || cmd.updated > 0) && this.installerConfig?.commands_target_dir) { + const total = cmd.created + cmd.updated; + const detail = cmd.updated > 0 ? `${cmd.created} new, ${cmd.updated} refreshed` : `${total}`; + await prompts.log.success(`${this.name} commands: ${detail} → ${this.installerConfig.commands_target_dir}`); + if (cmd.skippedCollision > 0) { + await prompts.log.message(` (${cmd.skippedCollision} skipped — name collides with reserved slash command)`); + } + if (cmd.writeFailures > 0) { + await prompts.log.warn(` (${cmd.writeFailures} pointer writes failed — see warnings above)`); + } + } } /** @@ -458,9 +473,20 @@ class ConfigDrivenIdeSetup { // Runs regardless of skipTarget — command pointers live in a per-IDE // directory and are not deduped across peers, so a peer-owned shared // skills directory does not protect this IDE's command pointers from - // cleanup. + // cleanup. The "currently active" set is passed so install-flow cleanup + // (where removalSet contains skills that will be re-added moments later) + // doesn't trample hand-edited pointers; install-flow cleanup will only + // delete pointers for skills that are not in the new manifest. if (this.installerConfig?.commands_target_dir) { - await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet); + // In the install/update flow (signal: previousSkillIds was passed), + // spare pointers whose canonicalId is still in the manifest so hand + // edits survive a routine reinstall. In the uninstall flow (no + // previousSkillIds — full uninstall or per-IDE removal via + // cleanupByList), don't spare anything; the IDE itself is going away, + // so its pointers should go with it. + const isInstallFlow = options.previousSkillIds && options.previousSkillIds.size > 0; + const activeSkillIds = isInstallFlow ? await this._readActiveSkillIds(resolvedBmadDir) : new Set(); + await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet, activeSkillIds); } // Skip target_dir cleanup when a peer platform owns this directory @@ -571,8 +597,14 @@ class ConfigDrivenIdeSetup { * @param {string} commandsTargetDir - Relative dir (e.g. .opencode/commands) * @param {Object} options * @param {Set} removalSet - canonicalIds whose pointer files to remove + * @param {Set} [activeSkillIds] - canonicalIds present in the + * current manifest. Pointers for IDs in this set are spared so an + * install-flow cleanup (where removalSet === previousSkillIds and the + * same skills are about to be re-installed) doesn't wipe hand-edited + * pointer files. Pass an empty set or omit to delete every match in + * removalSet (uninstall flow). */ - async cleanupCommandPointers(projectDir, commandsTargetDir, options = {}, removalSet = new Set()) { + async cleanupCommandPointers(projectDir, commandsTargetDir, options = {}, removalSet = new Set(), activeSkillIds = new Set()) { if (!removalSet || removalSet.size === 0) return; const commandsPath = path.join(projectDir, commandsTargetDir); @@ -586,9 +618,13 @@ class ConfigDrivenIdeSetup { } for (const entry of entries) { - if (typeof entry !== 'string' || !entry.endsWith('.md')) continue; + if (!entry.endsWith('.md')) continue; const canonicalId = entry.slice(0, -3); if (!removalSet.has(canonicalId)) continue; + // Spare pointers for skills that are still in the manifest; the + // install pass will refresh them in place if their content has gone + // stale, while preserving hand edits. + if (activeSkillIds.has(canonicalId)) continue; try { await fs.remove(path.join(commandsPath, entry)); } catch { @@ -607,6 +643,31 @@ class ConfigDrivenIdeSetup { } } + /** + * Read the canonicalIds currently present in the skill-manifest.csv. + * Used by cleanup to distinguish "re-install of an existing skill" + * (preserve pointer) from "skill truly being removed" (delete pointer). + * @param {string|null} bmadDir + * @returns {Promise>} + */ + async _readActiveSkillIds(bmadDir) { + const ids = new Set(); + if (!bmadDir) return ids; + const csvPath = path.join(bmadDir, '_config', 'skill-manifest.csv'); + if (!(await fs.pathExists(csvPath))) return ids; + try { + const content = await fs.readFile(csvPath, 'utf8'); + const records = csv.parse(content, { columns: true, skip_empty_lines: true }); + for (const record of records) { + if (record.canonicalId) ids.add(record.canonicalId); + } + } catch { + // Manifest unreadable — return an empty set so cleanup falls back to + // the conservative "delete what removalSet says" behavior. + } + return ids; + } + /** * Cleanup a specific target directory. * When removalSet is provided, only removes entries in that set. From 56a3f267f008b60bab85aa248b933c8fda4bf8bf Mon Sep 17 00:00:00 2001 From: jheyworth <8269695+jheyworth@users.noreply.github.com> Date: Tue, 28 Apr 2026 12:03:26 +0100 Subject: [PATCH 4/5] fix(installer): extend command-pointer generation to Copilot Custom Agents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-scopes #2324 to cover the second user-facing pain: GitHub Copilot's Custom Agents picker, where installed BMAD skills currently don't show up even though slash commands work natively. Generalizes the per-platform pointer-file mechanism so the same installCommandPointers / cleanupCommandPointers code path serves both OpenCode (slash commands palette) and Copilot (Custom Agents picker), with all platform-specific shape pushed into platform-codes.yaml as data: - commands_target_dir — where pointer files live (existing) - commands_extension — file extension (default '.md'; Copilot uses '.agent.md' per VS Code Custom Agents docs) - commands_body_template — pointer body, supports {canonicalId} and {target_dir} placeholders. Default matches OpenCode's `@skills/` resolver. Copilot has no such resolver, so its template uses the {project-root}///SKILL.md LOAD pattern (consistent with PR #1769). OpenCode behavior is unchanged. Copilot users now get a per-skill .github/agents/.agent.md file that surfaces the skill in the Custom Agents picker — addressing the "agents being gone" complaint flagged by enterprise users. Tests: extends Suite 17 with assertions for Copilot agent pointer creation, body content (LOAD pattern with {project-root}-rooted path), and idempotency. 318 tests pass (was 310). Refs #2267 Co-Authored-By: Claude Opus 4.7 (1M context) --- test/test-installation-components.js | 35 +++++++++++ tools/installer/ide/_config-driven.js | 78 +++++++++++++++++++------ tools/installer/ide/platform-codes.yaml | 3 + 3 files changed, 97 insertions(+), 19 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index a687def21..0e5f12ad4 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -557,6 +557,15 @@ async function runTests() { const copilotInstaller = platformCodes17.platforms['github-copilot']?.installer; assert(copilotInstaller?.target_dir === '.agents/skills', 'GitHub Copilot target_dir uses native skills path'); + assert( + copilotInstaller?.commands_target_dir === '.github/agents', + 'GitHub Copilot commands_target_dir is configured for the Custom Agents picker', + ); + assert(copilotInstaller?.commands_extension === '.agent.md', 'GitHub Copilot uses .agent.md extension for Custom Agents files'); + assert( + typeof copilotInstaller?.commands_body_template === 'string' && copilotInstaller.commands_body_template.includes('{canonicalId}'), + 'GitHub Copilot defines a commands_body_template with {canonicalId} placeholder', + ); const tempProjectDir17 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-copilot-test-')); const installedBmadDir17 = await createTestBmadFixture(); @@ -596,6 +605,32 @@ async function runTests() { 'GitHub Copilot setup preserves user content in copilot-instructions.md', ); + // Custom Agents picker integration: a per-skill .agent.md file should be + // generated under .github/agents/ so the skill appears in Copilot's + // Custom Agents picker. Body uses the LOAD-{project-root}/... pattern + // (Copilot's body has no @skills/ resolver, so the agent file + // instructs the model to load the SKILL.md directly). + const agentFile17 = path.join(tempProjectDir17, '.github', 'agents', 'bmad-master.agent.md'); + assert(await fs.pathExists(agentFile17), 'GitHub Copilot install writes per-skill .agent.md pointer file'); + const agentContent17 = await fs.readFile(agentFile17, 'utf8'); + assert( + agentContent17.includes('description:'), + 'Copilot agent pointer carries a description in YAML frontmatter (drives the agents picker label)', + ); + assert( + agentContent17.includes('{project-root}/.agents/skills/bmad-master/SKILL.md'), + 'Copilot agent pointer body resolves to the skill via LOAD {project-root}///SKILL.md', + ); + + // Idempotency: re-running setup must not duplicate or rewrite the agent + // pointer when the source manifest is unchanged. + const result17b = await ideManager17.setup('github-copilot', tempProjectDir17, installedBmadDir17, { + silent: true, + selectedModules: ['bmm'], + }); + assert(result17b.success === true, 'Second GitHub Copilot install succeeds (idempotent)'); + assert(await fs.pathExists(agentFile17), 'Copilot agent pointer survives a second install pass'); + await fs.remove(tempProjectDir17); await fs.remove(path.dirname(installedBmadDir17)); } catch (error) { diff --git a/tools/installer/ide/_config-driven.js b/tools/installer/ide/_config-driven.js index ad3fafd7a..1644aecb3 100644 --- a/tools/installer/ide/_config-driven.js +++ b/tools/installer/ide/_config-driven.js @@ -52,11 +52,26 @@ function isSafeCanonicalId(value) { return typeof value === 'string' && /^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/.test(value) && !value.includes('..'); } +// Default body template for command pointer files. Used when a platform's +// installer config doesn't override `commands_body_template`. Matches +// OpenCode's native `@skills/` skill-reference syntax. +const DEFAULT_COMMANDS_BODY_TEMPLATE = '@skills/{canonicalId}'; + +// Resolve placeholders in a body template. Supported placeholders: +// {canonicalId} — the skill's canonical id +// {target_dir} — the platform's skill install directory (e.g. .agents/skills) +// {project-root} — left as a literal placeholder for the model/tool to expand +// at runtime; consistent with PR #1769's templates. +function expandBodyTemplate(template, { canonicalId, targetDir }) { + return template.replaceAll('{canonicalId}', canonicalId).replaceAll('{target_dir}', targetDir); +} + // The exact body the installer would generate for a given description and -// canonicalId. Centralised so both the write and the freshness-check paths -// agree on the canonical form. -function buildCommandPointerBody(description, canonicalId) { - return `---\ndescription: ${yamlSafeSingleLine(description)}\n---\n\n@skills/${canonicalId}\n`; +// canonicalId, given the platform's body template. Centralised so both the +// write and the freshness-check paths agree on the canonical form. +function buildCommandPointerBody(description, canonicalId, { template, targetDir }) { + const bodyText = expandBodyTemplate(template, { canonicalId, targetDir }); + return `---\ndescription: ${yamlSafeSingleLine(description)}\n---\n\n${bodyText}\n`; } // Heuristic: does an existing pointer file look like our generator's output @@ -64,11 +79,12 @@ function buildCommandPointerBody(description, canonicalId) { // preserve)? We check the body shape rather than full equality so that // description-only edits in the manifest can propagate without trampling // hand edits to the body. -function looksLikeGeneratorOutput(content, canonicalId) { +function looksLikeGeneratorOutput(content, canonicalId, { template, targetDir }) { if (typeof content !== 'string') return false; const trimmed = content.trim(); - // Must end with the exact reference line our generator writes. - if (!trimmed.endsWith(`@skills/${canonicalId}`)) return false; + const expectedTail = expandBodyTemplate(template, { canonicalId, targetDir }).trim(); + // Must end with the exact body our generator writes (post-expansion). + if (!trimmed.endsWith(expectedTail)) return false; // Must start with frontmatter containing exactly one description: line. const fmMatch = trimmed.match(/^---\n([\S\s]*?)\n---\n/); if (!fmMatch) return false; @@ -259,6 +275,11 @@ class ConfigDrivenIdeSetup { const commandsPath = path.join(projectDir, config.commands_target_dir); await fs.ensureDir(commandsPath); + // Per-platform pointer-file shape, all overrideable in platform-codes.yaml. + const extension = config.commands_extension || '.md'; + const template = config.commands_body_template || DEFAULT_COMMANDS_BODY_TEMPLATE; + const targetDir = config.target_dir; + const csvContent = await fs.readFile(csvPath, 'utf8'); const records = csv.parse(csvContent, { columns: true, skip_empty_lines: true }); @@ -288,8 +309,8 @@ class ConfigDrivenIdeSetup { result.fallbackDescription++; } - const body = buildCommandPointerBody(description, canonicalId); - const commandFile = path.join(commandsPath, `${canonicalId}.md`); + const body = buildCommandPointerBody(description, canonicalId, { template, targetDir }); + const commandFile = path.join(commandsPath, `${canonicalId}${extension}`); // If a pointer file already exists, decide whether to overwrite based // on whether it looks like generator output (description-only diff) or @@ -309,7 +330,7 @@ class ConfigDrivenIdeSetup { result.skippedExisting++; continue; } - if (looksLikeGeneratorOutput(existing, canonicalId)) { + if (looksLikeGeneratorOutput(existing, canonicalId, { template, targetDir })) { // Description (or other generated bit) has changed; refresh in place. try { await fs.writeFile(commandFile, body, 'utf8'); @@ -317,7 +338,7 @@ class ConfigDrivenIdeSetup { } catch (error) { result.writeFailures++; if (!options.silent) { - await prompts.log.warn(`Failed to update command pointer ${canonicalId}.md: ${error.message}`); + await prompts.log.warn(`Failed to update command pointer ${canonicalId}${extension}: ${error.message}`); } } continue; @@ -333,7 +354,7 @@ class ConfigDrivenIdeSetup { } catch (error) { result.writeFailures++; if (!options.silent) { - await prompts.log.warn(`Failed to write command pointer ${canonicalId}.md: ${error.message}`); + await prompts.log.warn(`Failed to write command pointer ${canonicalId}${extension}: ${error.message}`); } } } @@ -486,7 +507,15 @@ class ConfigDrivenIdeSetup { // so its pointers should go with it. const isInstallFlow = options.previousSkillIds && options.previousSkillIds.size > 0; const activeSkillIds = isInstallFlow ? await this._readActiveSkillIds(resolvedBmadDir) : new Set(); - await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet, activeSkillIds); + const extension = this.installerConfig.commands_extension || '.md'; + await this.cleanupCommandPointers( + projectDir, + this.installerConfig.commands_target_dir, + options, + removalSet, + activeSkillIds, + extension, + ); } // Skip target_dir cleanup when a peer platform owns this directory @@ -590,9 +619,9 @@ class ConfigDrivenIdeSetup { /** * Cleanup generated command pointer files for entries in removalSet. - * Symmetric counterpart to installCommandPointers — removes .md - * files whose canonicalId is in the set. Removes the commands directory - * entirely if it ends up empty. + * Symmetric counterpart to installCommandPointers — removes + * `` files whose canonicalId is in the set. Removes + * the commands directory entirely if it ends up empty. * @param {string} projectDir * @param {string} commandsTargetDir - Relative dir (e.g. .opencode/commands) * @param {Object} options @@ -603,8 +632,19 @@ class ConfigDrivenIdeSetup { * same skills are about to be re-installed) doesn't wipe hand-edited * pointer files. Pass an empty set or omit to delete every match in * removalSet (uninstall flow). + * @param {string} [extension] - Pointer file extension (default '.md'); + * matches the platform's commands_extension config value so cleanup + * correctly identifies pointer files for IDEs whose convention isn't .md + * (e.g. Copilot's `.agent.md`). */ - async cleanupCommandPointers(projectDir, commandsTargetDir, options = {}, removalSet = new Set(), activeSkillIds = new Set()) { + async cleanupCommandPointers( + projectDir, + commandsTargetDir, + options = {}, + removalSet = new Set(), + activeSkillIds = new Set(), + extension = '.md', + ) { if (!removalSet || removalSet.size === 0) return; const commandsPath = path.join(projectDir, commandsTargetDir); @@ -618,8 +658,8 @@ class ConfigDrivenIdeSetup { } for (const entry of entries) { - if (!entry.endsWith('.md')) continue; - const canonicalId = entry.slice(0, -3); + if (!entry.endsWith(extension)) continue; + const canonicalId = entry.slice(0, -extension.length); if (!removalSet.has(canonicalId)) continue; // Spare pointers for skills that are still in the manifest; the // install pass will refresh them in place if their content has gone diff --git a/tools/installer/ide/platform-codes.yaml b/tools/installer/ide/platform-codes.yaml index 78ca1d271..c80996107 100644 --- a/tools/installer/ide/platform-codes.yaml +++ b/tools/installer/ide/platform-codes.yaml @@ -132,6 +132,9 @@ platforms: installer: target_dir: .agents/skills global_target_dir: ~/.agents/skills + commands_target_dir: .github/agents + commands_extension: .agent.md + commands_body_template: "LOAD the FULL {project-root}/{target_dir}/{canonicalId}/SKILL.md, READ its entire contents and follow its directions exactly!" goose: name: "Block Goose" From 5e0a8ea2f39ffac157599daf17027d8fba632ebd Mon Sep 17 00:00:00 2001 From: jheyworth <8269695+jheyworth@users.noreply.github.com> Date: Tue, 28 Apr 2026 12:55:26 +0100 Subject: [PATCH 5/5] fix(installer): filter Copilot Custom Agents picker to persona agents only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Earlier commit naively wrote a `.github/agents/.agent.md` for every installed skill, which would clutter the Custom Agents picker with 90+ workflow/tool entries that don't belong there. Adds an `agents-only` filter that gates the per-skill emission on whether the canonical id signals a persona agent: - Primary rule: id contains `-agent-` (e.g. `bmad-agent-pm`, `gds-agent-game-dev`, `wds-agent-freya-ux`, `bmad-cis-agent-storyteller`). - Allowlist: `bmad-tea` — TEA's Murat persona uses the bare module code rather than the `-agent-` convention. Listed explicitly so the rule still surfaces it. Verified against the full installed manifest (114 skills): catches all 20 description-confirmed personas across BMM, CIS, GDS, WDS, TEA; excludes all 94 workflows/tools. Wired through a new yaml field on github-copilot: commands_filter: agents-only OpenCode is unaffected — it has no `commands_filter` set, so the loop behaves as before (every skill becomes a slash command). Tests: extends Suite 17 with a multi-skill manifest fixture covering persona/agent + bmad-tea + workflow cases; asserts persona agents and bmad-tea get .agent.md files while workflows do not. 322 tests pass. Refs #2267 Co-Authored-By: Claude Opus 4.7 (1M context) --- test/test-installation-components.js | 56 +++++++++++++++++++------ tools/installer/ide/_config-driven.js | 29 +++++++++++++ tools/installer/ide/platform-codes.yaml | 8 ++++ 3 files changed, 81 insertions(+), 12 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 0e5f12ad4..8da88958c 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -566,10 +566,30 @@ async function runTests() { typeof copilotInstaller?.commands_body_template === 'string' && copilotInstaller.commands_body_template.includes('{canonicalId}'), 'GitHub Copilot defines a commands_body_template with {canonicalId} placeholder', ); + assert( + copilotInstaller?.commands_filter === 'agents-only', + 'GitHub Copilot filters Custom Agents picker to persona agents only (agents-only)', + ); const tempProjectDir17 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-copilot-test-')); const installedBmadDir17 = await createTestBmadFixture(); + // Extend the fixture: add a persona-style agent skill (`-agent-` in id) + // and the `bmad-tea` non-conventional agent so we can verify that the + // agents-only filter routes them to .github/agents/ while leaving the + // workflow-style `bmad-master` out. + const fixtureCsvPath17 = path.join(installedBmadDir17, '_config', 'skill-manifest.csv'); + await fs.writeFile( + fixtureCsvPath17, + [ + 'canonicalId,name,description,module,path', + '"bmad-master","bmad-master","Workflow-style fixture (no -agent- in id, should NOT appear in Copilot agents picker)","core","_bmad/core/bmad-master/SKILL.md"', + '"bmad-agent-fixture","bmad-agent-fixture","Persona agent fixture (-agent- in id, SHOULD appear in Copilot agents picker)","core","_bmad/core/bmad-agent-fixture/SKILL.md"', + '"bmad-tea","bmad-tea","Non-conventional persona agent fixture (Murat-style, SHOULD appear despite no -agent- segment)","core","_bmad/core/bmad-tea/SKILL.md"', + '', + ].join('\n'), + ); + const copilotInstructionsPath17 = path.join(tempProjectDir17, '.github', 'copilot-instructions.md'); await fs.ensureDir(path.dirname(copilotInstructionsPath17)); await fs.writeFile( @@ -605,31 +625,43 @@ async function runTests() { 'GitHub Copilot setup preserves user content in copilot-instructions.md', ); - // Custom Agents picker integration: a per-skill .agent.md file should be - // generated under .github/agents/ so the skill appears in Copilot's - // Custom Agents picker. Body uses the LOAD-{project-root}/... pattern - // (Copilot's body has no @skills/ resolver, so the agent file - // instructs the model to load the SKILL.md directly). - const agentFile17 = path.join(tempProjectDir17, '.github', 'agents', 'bmad-master.agent.md'); - assert(await fs.pathExists(agentFile17), 'GitHub Copilot install writes per-skill .agent.md pointer file'); - const agentContent17 = await fs.readFile(agentFile17, 'utf8'); + // Custom Agents picker integration: persona agents (and bmad-tea) get + // .agent.md files in .github/agents/. Workflows do NOT — the + // agents-only filter keeps the picker uncluttered. + const agentsDir17 = path.join(tempProjectDir17, '.github', 'agents'); + const agentFileForPersona17 = path.join(agentsDir17, 'bmad-agent-fixture.agent.md'); + const agentFileForTea17 = path.join(agentsDir17, 'bmad-tea.agent.md'); + const agentFileForWorkflow17 = path.join(agentsDir17, 'bmad-master.agent.md'); + + assert(await fs.pathExists(agentFileForPersona17), 'Persona agent (-agent- in id) gets a .agent.md file in .github/agents/'); + assert(await fs.pathExists(agentFileForTea17), 'bmad-tea persona (non-conventional id) is allowlisted into .github/agents/'); assert( - agentContent17.includes('description:'), + !(await fs.pathExists(agentFileForWorkflow17)), + 'Workflow skill (no -agent- in id, not in allowlist) is FILTERED OUT of .github/agents/', + ); + + // Body content of the persona agent file: frontmatter description + + // LOAD pattern referencing the skill's SKILL.md path under target_dir. + const personaAgentContent17 = await fs.readFile(agentFileForPersona17, 'utf8'); + assert( + personaAgentContent17.includes('description:'), 'Copilot agent pointer carries a description in YAML frontmatter (drives the agents picker label)', ); assert( - agentContent17.includes('{project-root}/.agents/skills/bmad-master/SKILL.md'), + personaAgentContent17.includes('{project-root}/.agents/skills/bmad-agent-fixture/SKILL.md'), 'Copilot agent pointer body resolves to the skill via LOAD {project-root}///SKILL.md', ); // Idempotency: re-running setup must not duplicate or rewrite the agent - // pointer when the source manifest is unchanged. + // pointer when the source manifest is unchanged, AND must not start + // emitting workflow-skill agent files. const result17b = await ideManager17.setup('github-copilot', tempProjectDir17, installedBmadDir17, { silent: true, selectedModules: ['bmm'], }); assert(result17b.success === true, 'Second GitHub Copilot install succeeds (idempotent)'); - assert(await fs.pathExists(agentFile17), 'Copilot agent pointer survives a second install pass'); + assert(await fs.pathExists(agentFileForPersona17), 'Persona agent pointer survives a second install pass'); + assert(!(await fs.pathExists(agentFileForWorkflow17)), 'Workflow skill remains filtered out of agents picker on second install'); await fs.remove(tempProjectDir17); await fs.remove(path.dirname(installedBmadDir17)); diff --git a/tools/installer/ide/_config-driven.js b/tools/installer/ide/_config-driven.js index 1644aecb3..b725aaa77 100644 --- a/tools/installer/ide/_config-driven.js +++ b/tools/installer/ide/_config-driven.js @@ -57,6 +57,24 @@ function isSafeCanonicalId(value) { // OpenCode's native `@skills/` skill-reference syntax. const DEFAULT_COMMANDS_BODY_TEMPLATE = '@skills/{canonicalId}'; +// Persona-agent id outside the `-agent-` naming convention. +// TEA's Murat is the only persona whose canonical id is the bare module code +// rather than `-agent-`. Listed here so platforms that filter +// for "agents only" (e.g. GitHub Copilot's Custom Agents picker) include it. +const NON_CONVENTIONAL_AGENT_IDS = new Set(['bmad-tea']); + +// Is this skill a persona agent (vs. a workflow/tool/standalone skill)? +// Used by platforms that surface only persona agents (e.g. Copilot's Custom +// Agents picker). Rule: canonical id contains `-agent-` OR is in the +// known non-conventional allowlist. Tested against the full installed +// manifest — catches all 20 description-confirmed personas across BMM, +// CIS, GDS, WDS, TEA without false positives. +function isAgentSkill(canonicalId) { + if (typeof canonicalId !== 'string') return false; + if (NON_CONVENTIONAL_AGENT_IDS.has(canonicalId)) return true; + return canonicalId.includes('-agent-'); +} + // Resolve placeholders in a body template. Supported placeholders: // {canonicalId} — the skill's canonical id // {target_dir} — the platform's skill install directory (e.g. .agents/skills) @@ -265,6 +283,7 @@ class ConfigDrivenIdeSetup { skippedExisting: 0, skippedCollision: 0, skippedInvalidId: 0, + skippedFiltered: 0, writeFailures: 0, fallbackDescription: 0, }; @@ -279,6 +298,7 @@ class ConfigDrivenIdeSetup { const extension = config.commands_extension || '.md'; const template = config.commands_body_template || DEFAULT_COMMANDS_BODY_TEMPLATE; const targetDir = config.target_dir; + const filter = config.commands_filter || null; const csvContent = await fs.readFile(csvPath, 'utf8'); const records = csv.parse(csvContent, { columns: true, skip_empty_lines: true }); @@ -295,6 +315,15 @@ class ConfigDrivenIdeSetup { continue; } + // Optional per-platform filter: surfaces that should only show + // persona agents (e.g. Copilot's Custom Agents picker) skip + // workflow/tool skills here so the picker isn't cluttered with + // 90+ unrelated entries. + if (filter === 'agents-only' && !isAgentSkill(canonicalId)) { + result.skippedFiltered++; + continue; + } + // Reserved-name guard is OpenCode-specific. Other adapters that opt // into commands_target_dir later should declare their own reserved // set rather than inheriting OpenCode's. diff --git a/tools/installer/ide/platform-codes.yaml b/tools/installer/ide/platform-codes.yaml index c80996107..0e84c4f02 100644 --- a/tools/installer/ide/platform-codes.yaml +++ b/tools/installer/ide/platform-codes.yaml @@ -135,6 +135,14 @@ platforms: commands_target_dir: .github/agents commands_extension: .agent.md commands_body_template: "LOAD the FULL {project-root}/{target_dir}/{canonicalId}/SKILL.md, READ its entire contents and follow its directions exactly!" + # The Custom Agents picker should only show persona agents (not + # workflows/tools). BMAD's persona agents are conventionally named + # with an `-agent-` segment in their canonical id (e.g. + # `bmad-agent-pm`, `gds-agent-game-dev`, `wds-agent-freya-ux`, + # `bmad-cis-agent-storyteller`). The one exception is `bmad-tea` — + # TEA's Murat persona uses the module code as its id rather than the + # `-agent-` convention. This filter keeps the picker uncluttered. + commands_filter: agents-only goose: name: "Block Goose"