From 149c4049fdd0f3d9c8974f89f4b20542ef29826b Mon Sep 17 00:00:00 2001 From: Alex Verkhovsky Date: Sun, 8 Mar 2026 08:53:29 -0600 Subject: [PATCH] fix(manifest): address PR review findings from triage - Add missing skillClaimedDirs guard to getAgentsFromDir (F1) - Add skills to this.files[] in collectSkills (F2) - Add test for type:skill inside workflows/ dir (F5) - Warn on malformed workflow.md parse in skill dirs (F6) - Add skills count to generateManifests return value (F9) - Remove redundant \r? from regex after line normalization (F10) - Normalize path.relative to forward slashes for cross-platform (F12) - Enforce directory name as skill canonicalId, warn if manifest overrides (F13) Co-Authored-By: Claude Opus 4.6 --- test/test-installation-components.js | 26 ++++++++++++----- .../installers/lib/core/manifest-generator.js | 29 +++++++++++++++---- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 81feda2f1..cf075bd67 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -1605,7 +1605,7 @@ async function runTests() { // --- Skill at unusual path: core/custom-area/my-skill/ --- const skillDir29 = path.join(tempFixture29, 'core', 'custom-area', 'my-skill'); await fs.ensureDir(skillDir29); - await fs.writeFile(path.join(skillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\ncanonicalId: my-custom-skill\n'); + await fs.writeFile(path.join(skillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\n'); await fs.writeFile( path.join(skillDir29, 'workflow.md'), '---\nname: My Custom Skill\ndescription: A skill at an unusual path\n---\n\nSkill body content\n', @@ -1620,10 +1620,19 @@ async function runTests() { '---\nname: Regular Workflow\ndescription: A regular workflow not a skill\n---\n\nWorkflow body\n', ); + // --- Skill inside workflows/ dir: core/workflows/wf-skill/ (exercises findWorkflows skip logic) --- + const wfSkillDir29 = path.join(tempFixture29, 'core', 'workflows', 'wf-skill'); + await fs.ensureDir(wfSkillDir29); + await fs.writeFile(path.join(wfSkillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\n'); + await fs.writeFile( + path.join(wfSkillDir29, 'workflow.md'), + '---\nname: Workflow Skill\ndescription: A skill inside workflows dir\n---\n\nSkill in workflows\n', + ); + // --- Skill inside tasks/ dir: core/tasks/task-skill/ --- const taskSkillDir29 = path.join(tempFixture29, 'core', 'tasks', 'task-skill'); await fs.ensureDir(taskSkillDir29); - await fs.writeFile(path.join(taskSkillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\ncanonicalId: task-skill\n'); + await fs.writeFile(path.join(taskSkillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\n'); await fs.writeFile( path.join(taskSkillDir29, 'workflow.md'), '---\nname: Task Skill\ndescription: A skill inside tasks dir\n---\n\nSkill in tasks\n', @@ -1638,7 +1647,7 @@ async function runTests() { await generator29.generateManifests(tempFixture29, ['core'], [], { ides: [] }); // Skill at unusual path should be in skills - const skillEntry29 = generator29.skills.find((s) => s.canonicalId === 'my-custom-skill'); + const skillEntry29 = generator29.skills.find((s) => s.canonicalId === 'my-skill'); assert(skillEntry29 !== undefined, 'Skill at unusual path appears in skills[]'); assert(skillEntry29 && skillEntry29.name === 'My Custom Skill', 'Skill has correct name from frontmatter'); assert( @@ -1665,13 +1674,16 @@ async function runTests() { const regularInSkills29 = generator29.skills.find((s) => s.canonicalId === 'regular-wf'); assert(regularInSkills29 === undefined, 'Regular type:workflow does NOT appear in skills[]'); + // Skill inside workflows/ should be in skills[], NOT in workflows[] (exercises findWorkflows skip at lines 311/322) + const wfSkill29 = generator29.skills.find((s) => s.canonicalId === 'wf-skill'); + assert(wfSkill29 !== undefined, 'Skill in workflows/ dir appears in skills[]'); + const wfSkillInWorkflows29 = generator29.workflows.find((w) => w.name === 'Workflow Skill'); + assert(wfSkillInWorkflows29 === undefined, 'Skill in workflows/ dir does NOT appear in workflows[]'); + // Test scanInstalledModules recognizes skill-only modules const skillOnlyModDir29 = path.join(tempFixture29, 'skill-only-mod'); await fs.ensureDir(path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill')); - await fs.writeFile( - path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill', 'bmad-skill-manifest.yaml'), - 'type: skill\ncanonicalId: nested-skill\n', - ); + await fs.writeFile(path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill', 'bmad-skill-manifest.yaml'), 'type: skill\n'); await fs.writeFile( path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill', 'workflow.md'), '---\nname: Nested Skill\ndescription: desc\n---\nbody\n', diff --git a/tools/cli/installers/lib/core/manifest-generator.js b/tools/cli/installers/lib/core/manifest-generator.js index 711faba92..8562672b5 100644 --- a/tools/cli/installers/lib/core/manifest-generator.js +++ b/tools/cli/installers/lib/core/manifest-generator.js @@ -135,6 +135,7 @@ class ManifestGenerator { ]; return { + skills: this.skills.length, workflows: this.workflows.length, agents: this.agents.length, tasks: this.tasks.length, @@ -189,7 +190,7 @@ class ManifestGenerator { if (workflowFile === 'workflow.yaml') { workflow = yaml.parse(content); } else { - const frontmatterMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); + const frontmatterMatch = content.match(/^---\n([\s\S]*?)\n---/); if (!frontmatterMatch) { if (debug) console.log(`[DEBUG] collectSkills: skipped (no frontmatter): ${workflowPath}`); continue; @@ -203,12 +204,18 @@ class ManifestGenerator { } // Build path relative from module root - const relativePath = path.relative(modulePath, dir); + const relativePath = path.relative(modulePath, dir).split(path.sep).join('/'); const installPath = relativePath ? `${this.bmadFolderName}/${moduleName}/${relativePath}/${workflowFile}` : `${this.bmadFolderName}/${moduleName}/${workflowFile}`; - const canonicalId = this.getCanonicalId(manifest, workflowFile) || path.basename(dir); + // Skills derive canonicalId from directory name — never from manifest + if (manifest && manifest.__single && manifest.__single.canonicalId) { + console.warn( + `Warning: Skill manifest at ${dir}/bmad-skill-manifest.yaml contains canonicalId — this field is ignored for skills (directory name is the canonical ID)`, + ); + } + const canonicalId = path.basename(dir); this.skills.push({ name: workflow.name, @@ -219,6 +226,14 @@ class ManifestGenerator { install_to_bmad: this.getInstallToBmad(manifest, workflowFile), }); + // Add to files list + this.files.push({ + type: 'skill', + name: workflow.name, + module: moduleName, + path: installPath, + }); + this.skillClaimedDirs.add(dir); if (debug) { @@ -246,7 +261,9 @@ class ManifestGenerator { } if (hasSkillType && debug) { const hasWorkflow = workflowFilenames.some((f) => entries.some((e) => e.name === f)); - if (!hasWorkflow) { + if (hasWorkflow) { + console.log(`[DEBUG] collectSkills: dir has type:skill manifest but workflow file failed to parse: ${dir}`); + } else { console.log(`[DEBUG] collectSkills: dir has type:skill manifest but no workflow.md/workflow.yaml: ${dir}`); } } @@ -347,7 +364,7 @@ class ManifestGenerator { workflow = yaml.parse(content); } else { // Parse MD workflow with YAML frontmatter - const frontmatterMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); + const frontmatterMatch = content.match(/^---\n([\s\S]*?)\n---/); if (!frontmatterMatch) { if (debug) { console.log(`[DEBUG] Skipped (no frontmatter): ${fullPath}`); @@ -462,6 +479,8 @@ class ManifestGenerator { * Only includes compiled .md files (not .agent.yaml source files) */ async getAgentsFromDir(dirPath, moduleName, relativePath = '') { + // Skip directories claimed by collectSkills + if (this.skillClaimedDirs && this.skillClaimedDirs.has(dirPath)) return []; const agents = []; const entries = await fs.readdir(dirPath, { withFileTypes: true }); // Load skill manifest for this directory (if present)