From c56af3296e5f427dd7e23f31a11e368d4ea6355e Mon Sep 17 00:00:00 2001 From: Dicky Moore Date: Sun, 8 Mar 2026 17:17:58 +0000 Subject: [PATCH] fix(installer): tighten lean shard-doc review follow-ups --- test/test-installation-components.js | 93 +++++++++++++------ .../cli/installers/lib/ide/_config-driven.js | 6 +- .../lib/ide/shared/skill-manifest.js | 37 ++------ 3 files changed, 77 insertions(+), 59 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 27d7cf5a1..2ffbcd3cb 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -17,6 +17,7 @@ const fs = require('fs-extra'); const { YamlXmlBuilder } = require('../tools/cli/lib/yaml-xml-builder'); const { ManifestGenerator } = require('../tools/cli/installers/lib/core/manifest-generator'); const { IdeManager } = require('../tools/cli/installers/lib/ide/manager'); +const { ConfigDrivenIdeSetup } = require('../tools/cli/installers/lib/ide/_config-driven'); const { clearCache, loadPlatformCodes } = require('../tools/cli/installers/lib/ide/platform-codes'); // ANSI colors @@ -120,6 +121,15 @@ async function createShardDocPrototypeFixture() { '', ].join('\n'), ); + await fs.writeFile( + path.join(fixtureDir, 'core', 'tasks', 'bmad-shard-doc-skill-prototype', 'bmad-skill-manifest.yaml'), + [ + 'canonicalId: bmad-shard-doc-skill-prototype', + 'type: task', + 'description: "Prototype native skill wrapper for shard-doc during installer transition"', + '', + ].join('\n'), + ); await fs.writeFile( path.join(fixtureDir, '_config', 'task-manifest.csv'), @@ -882,68 +892,97 @@ async function runTests() { console.log(''); // ============================================================ - // Test 11: Shard-doc Prototype Duplication (Skill-Format Only) + // Test 11: Shard-doc Prototype Duplication (Skill/Non-Skill Scope) // ============================================================ console.log(`${colors.yellow}Test Suite 11: Shard-doc Prototype Duplication${colors.reset}\n`); - let tempCodexProjectDir; - let tempGeminiProjectDir; + let tempSkillProjectDir; + let tempNonSkillProjectDir; let installedBmadDir; try { clearCache(); const platformCodes = await loadPlatformCodes(); - const codexInstaller = platformCodes.platforms.codex?.installer; - const geminiInstaller = platformCodes.platforms.gemini?.installer; + const skillFormatEntry = Object.entries(platformCodes.platforms || {}).find(([_, platform]) => { + const installer = platform?.installer; + if (!installer || installer.skill_format !== true || typeof installer.target_dir !== 'string') return false; + if (Array.isArray(installer.artifact_types) && !installer.artifact_types.includes('tasks')) return false; + return true; + }); - assert(codexInstaller?.skill_format === true, 'Codex installer uses skill_format output'); - assert(geminiInstaller?.skill_format === true, 'Gemini installer uses skill_format output'); + assert(Boolean(skillFormatEntry), 'Found a skill_format platform that installs task artifacts'); + if (!skillFormatEntry) throw new Error('No suitable skill_format platform found for shard-doc prototype test'); - tempCodexProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-codex-prototype-test-')); - tempGeminiProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-gemini-prototype-test-')); + const [skillFormatPlatformCode, skillFormatPlatform] = skillFormatEntry; + const skillInstaller = skillFormatPlatform.installer; + + tempSkillProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-skill-prototype-test-')); + tempNonSkillProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-nonskill-prototype-test-')); installedBmadDir = await createShardDocPrototypeFixture(); const ideManager = new IdeManager(); await ideManager.ensureInitialized(); - const codexResult = await ideManager.setup('codex', tempCodexProjectDir, installedBmadDir, { + const skillResult = await ideManager.setup(skillFormatPlatformCode, tempSkillProjectDir, installedBmadDir, { silent: true, selectedModules: ['bmm'], }); - assert(codexResult.success === true, 'Codex setup succeeds for shard-doc prototype fixture'); + assert(skillResult.success === true, `${skillFormatPlatformCode} setup succeeds for shard-doc prototype fixture`); - const codexCanonicalSkill = path.join(tempCodexProjectDir, '.agents', 'skills', 'bmad-shard-doc', 'SKILL.md'); - const codexPrototypeSkill = path.join(tempCodexProjectDir, '.agents', 'skills', 'bmad-shard-doc-skill-prototype', 'SKILL.md'); - assert(await fs.pathExists(codexCanonicalSkill), 'Codex install writes canonical shard-doc skill'); - assert(await fs.pathExists(codexPrototypeSkill), 'Codex install writes duplicated shard-doc prototype skill'); + const canonicalSkillPath = path.join(tempSkillProjectDir, skillInstaller.target_dir, 'bmad-shard-doc', 'SKILL.md'); + const prototypeSkillPath = path.join(tempSkillProjectDir, skillInstaller.target_dir, 'bmad-shard-doc-skill-prototype', 'SKILL.md'); + assert(await fs.pathExists(canonicalSkillPath), `${skillFormatPlatformCode} install writes canonical shard-doc skill`); + assert(await fs.pathExists(prototypeSkillPath), `${skillFormatPlatformCode} install writes duplicated shard-doc prototype skill`); - const codexCanonicalContent = await fs.readFile(codexCanonicalSkill, 'utf8'); - const codexPrototypeContent = await fs.readFile(codexPrototypeSkill, 'utf8'); - assert(codexCanonicalContent.includes('name: bmad-shard-doc'), 'Canonical shard-doc skill keeps canonical frontmatter name'); + const canonicalSkillContent = await fs.readFile(canonicalSkillPath, 'utf8'); + const prototypeSkillContent = await fs.readFile(prototypeSkillPath, 'utf8'); + + assert(canonicalSkillContent.includes('name: bmad-shard-doc'), 'Canonical shard-doc skill keeps canonical frontmatter name'); assert( - codexPrototypeContent.includes('name: bmad-shard-doc-skill-prototype'), + canonicalSkillContent.includes('Read the entire task file at: {project-root}/_bmad/core/tasks/shard-doc.xml'), + 'Canonical shard-doc skill points to shard-doc.xml', + ); + assert( + prototypeSkillContent.includes('name: bmad-shard-doc-skill-prototype'), 'Prototype shard-doc skill uses prototype frontmatter name', ); assert( - codexPrototypeContent.includes('Prototype marker: source-authored-skill'), + prototypeSkillContent.includes('Prototype marker: source-authored-skill'), 'Prototype shard-doc skill is copied from source SKILL.md', ); + assert( + prototypeSkillContent.includes('Read and execute from: {project-root}/_bmad/core/tasks/shard-doc.xml'), + 'Prototype shard-doc skill preserves source-authored shard-doc.xml reference', + ); - const geminiResult = await ideManager.setup('gemini', tempGeminiProjectDir, installedBmadDir, { + const nonSkillInstaller = { + ...skillInstaller, + target_dir: '.legacy/prototype-commands', + skill_format: false, + artifact_types: ['tasks'], + }; + const nonSkillHandler = new ConfigDrivenIdeSetup('prototype-nonskill-test', { + name: 'Prototype Non-Skill Test', + preferred: false, + installer: nonSkillInstaller, + }); + const nonSkillResult = await nonSkillHandler.setup(tempNonSkillProjectDir, installedBmadDir, { silent: true, selectedModules: ['bmm'], }); - assert(geminiResult.success === true, 'Gemini setup succeeds for shard-doc prototype fixture'); + assert(nonSkillResult.success === true, 'Synthetic non-skill-format setup succeeds for shard-doc prototype fixture'); - const geminiCanonicalSkill = path.join(tempGeminiProjectDir, '.gemini', 'skills', 'bmad-shard-doc', 'SKILL.md'); - const geminiPrototypeSkill = path.join(tempGeminiProjectDir, '.gemini', 'skills', 'bmad-shard-doc-skill-prototype', 'SKILL.md'); - assert(await fs.pathExists(geminiCanonicalSkill), 'Gemini install writes canonical shard-doc skill'); - assert(await fs.pathExists(geminiPrototypeSkill), 'Gemini install writes duplicated shard-doc prototype skill'); + const nonSkillTargetDir = path.join(tempNonSkillProjectDir, nonSkillInstaller.target_dir); + const nonSkillEntries = await fs.readdir(nonSkillTargetDir); + const hasCanonical = nonSkillEntries.some((entry) => /^bmad-shard-doc(\.|$)/.test(entry)); + const hasPrototype = nonSkillEntries.some((entry) => /^bmad-shard-doc-skill-prototype(\.|$)/.test(entry)); + assert(hasCanonical, 'Non-skill-format install writes canonical shard-doc artifact'); + assert(!hasPrototype, 'Non-skill-format install does not write duplicated shard-doc prototype artifact'); } catch (error) { assert(false, 'Shard-doc prototype duplication test succeeds', error.message); } finally { - await Promise.allSettled([tempCodexProjectDir, tempGeminiProjectDir, installedBmadDir].filter(Boolean).map((dir) => fs.remove(dir))); + await Promise.allSettled([tempSkillProjectDir, tempNonSkillProjectDir, installedBmadDir].filter(Boolean).map((dir) => fs.remove(dir))); } // Test 17: GitHub Copilot Native Skills Install diff --git a/tools/cli/installers/lib/ide/_config-driven.js b/tools/cli/installers/lib/ide/_config-driven.js index c21c0525d..980d65e51 100644 --- a/tools/cli/installers/lib/ide/_config-driven.js +++ b/tools/cli/installers/lib/ide/_config-driven.js @@ -558,6 +558,7 @@ LOAD and execute from: {project-root}/{{bmadFolderName}}/{{path}} resolveArtifactSourceRef(artifact, bmadDir) { if (artifact.type !== 'task' || !artifact.path) return null; const sourcePath = artifact.path; + const resolvedBmadDir = path.resolve(bmadDir); let normalized = sourcePath.replaceAll('\\', '/'); if (path.isAbsolute(normalized)) { @@ -578,7 +579,10 @@ LOAD and execute from: {project-root}/{{bmadFolderName}}/{{path}} if (!filename || filename === '.' || filename === '..') return null; const relativeDir = path.dirname(normalized); - const dirPath = relativeDir === '.' ? bmadDir : path.join(bmadDir, relativeDir); + const dirPath = relativeDir === '.' ? resolvedBmadDir : path.resolve(resolvedBmadDir, relativeDir); + const relativeToRoot = path.relative(resolvedBmadDir, dirPath); + if (relativeToRoot === '..' || relativeToRoot.startsWith(`..${path.sep}`) || path.isAbsolute(relativeToRoot)) return null; + return { dirPath, filename }; } diff --git a/tools/cli/installers/lib/ide/shared/skill-manifest.js b/tools/cli/installers/lib/ide/shared/skill-manifest.js index f80235ab0..c487891c6 100644 --- a/tools/cli/installers/lib/ide/shared/skill-manifest.js +++ b/tools/cli/installers/lib/ide/shared/skill-manifest.js @@ -37,7 +37,7 @@ function getCanonicalId(manifest, filename) { /** * Resolve a manifest entry for a source filename. - * Handles single-entry manifests and extension fallbacks. + * Strict by default: supports single-entry manifests and exact filename keys only. * @param {Object|null} manifest - Loaded manifest * @param {string} filename - Source filename * @returns {Object|null} Manifest entry object @@ -48,12 +48,6 @@ function resolveManifestEntry(manifest, filename) { if (manifest.__single) return manifest.__single; // Multi-entry: look up by filename directly if (manifest[filename]) return manifest[filename]; - // Fallback: try alternate extensions for compiled files - const baseName = filename.replace(/\.(md|xml)$/i, ''); - const agentKey = `${baseName}.agent.yaml`; - if (manifest[agentKey]) return manifest[agentKey]; - const xmlKey = `${baseName}.xml`; - if (manifest[xmlKey]) return manifest[xmlKey]; return null; } @@ -64,18 +58,8 @@ function resolveManifestEntry(manifest, filename) { * @returns {string|null} type or null */ function getArtifactType(manifest, filename) { - if (!manifest) return null; - // Single-entry manifest applies to all files in the directory - if (manifest.__single) return manifest.__single.type || null; - // Multi-entry: look up by filename directly - if (manifest[filename]) return manifest[filename].type || null; - // Fallback: try alternate extensions for compiled files - const baseName = filename.replace(/\.(md|xml)$/i, ''); - const agentKey = `${baseName}.agent.yaml`; - if (manifest[agentKey]) return manifest[agentKey].type || null; - const xmlKey = `${baseName}.xml`; - if (manifest[xmlKey]) return manifest[xmlKey].type || null; - return null; + const manifestEntry = resolveManifestEntry(manifest, filename); + return manifestEntry?.type || null; } /** @@ -85,18 +69,9 @@ function getArtifactType(manifest, filename) { * @returns {boolean} install_to_bmad value (defaults to true) */ function getInstallToBmad(manifest, filename) { - if (!manifest) return true; - // Single-entry manifest applies to all files in the directory - if (manifest.__single) return manifest.__single.install_to_bmad !== false; - // Multi-entry: look up by filename directly - if (manifest[filename]) return manifest[filename].install_to_bmad !== false; - // Fallback: try alternate extensions for compiled files - const baseName = filename.replace(/\.(md|xml)$/i, ''); - const agentKey = `${baseName}.agent.yaml`; - if (manifest[agentKey]) return manifest[agentKey].install_to_bmad !== false; - const xmlKey = `${baseName}.xml`; - if (manifest[xmlKey]) return manifest[xmlKey].install_to_bmad !== false; - return true; + const manifestEntry = resolveManifestEntry(manifest, filename); + if (!manifestEntry) return true; + return manifestEntry.install_to_bmad !== false; } module.exports = { loadSkillManifest, getCanonicalId, getArtifactType, getInstallToBmad };