From 140ae57f2a575736cba3eed2f8472d361fb1b2e2 Mon Sep 17 00:00:00 2001 From: Alex Verkhovsky Date: Sun, 8 Mar 2026 09:02:06 -0600 Subject: [PATCH] feat(manifest): unified skill scanner decoupled from legacy collectors (#1859) * feat(manifest): unified skill scanner decoupled from legacy collectors Add collectSkills() that recursively walks module trees to discover type:skill directories anywhere, replacing the band-aid detection inside collectWorkflows(). Legacy collectors now skip claimed dirs. scanInstalledModules recognizes skill-only modules. Co-Authored-By: Claude Opus 4.6 * 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 --------- Co-authored-by: Claude Opus 4.6 --- test/test-installation-components.js | 109 +++++++++ .../installers/lib/core/manifest-generator.js | 220 ++++++++++++++++-- 2 files changed, 306 insertions(+), 23 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 2eff6e5ee..cf075bd67 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -1590,6 +1590,115 @@ async function runTests() { console.log(''); + // ============================================================ + // Suite 29: Unified Skill Scanner — collectSkills + // ============================================================ + console.log(`${colors.yellow}Test Suite 29: Unified Skill Scanner${colors.reset}\n`); + + let tempFixture29; + try { + tempFixture29 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-skill-scanner-')); + + // Create _config dir (required by manifest generator) + await fs.ensureDir(path.join(tempFixture29, '_config')); + + // --- 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\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', + ); + + // --- Regular workflow dir: core/workflows/regular-wf/ (type: workflow) --- + const wfDir29 = path.join(tempFixture29, 'core', 'workflows', 'regular-wf'); + await fs.ensureDir(wfDir29); + await fs.writeFile(path.join(wfDir29, 'bmad-skill-manifest.yaml'), 'type: workflow\ncanonicalId: regular-wf\n'); + await fs.writeFile( + path.join(wfDir29, 'workflow.md'), + '---\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\n'); + await fs.writeFile( + path.join(taskSkillDir29, 'workflow.md'), + '---\nname: Task Skill\ndescription: A skill inside tasks dir\n---\n\nSkill in tasks\n', + ); + + // Minimal agent so core module is detected + await fs.ensureDir(path.join(tempFixture29, 'core', 'agents')); + const minimalAgent29 = 'p'; + await fs.writeFile(path.join(tempFixture29, 'core', 'agents', 'test.md'), minimalAgent29); + + const generator29 = new ManifestGenerator(); + await generator29.generateManifests(tempFixture29, ['core'], [], { ides: [] }); + + // Skill at unusual path should be in skills + 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( + skillEntry29 && skillEntry29.path.includes('custom-area/my-skill/workflow.md'), + 'Skill path includes relative path from module root', + ); + + // Skill should NOT be in workflows + const inWorkflows29 = generator29.workflows.find((w) => w.name === 'My Custom Skill'); + assert(inWorkflows29 === undefined, 'Skill at unusual path does NOT appear in workflows[]'); + + // Skill in tasks/ dir should be in skills + const taskSkillEntry29 = generator29.skills.find((s) => s.canonicalId === 'task-skill'); + assert(taskSkillEntry29 !== undefined, 'Skill in tasks/ dir appears in skills[]'); + + // Skill in tasks/ should NOT appear in tasks[] + const inTasks29 = generator29.tasks.find((t) => t.name === 'Task Skill'); + assert(inTasks29 === undefined, 'Skill in tasks/ dir does NOT appear in tasks[]'); + + // Regular workflow should be in workflows, NOT in skills + const regularWf29 = generator29.workflows.find((w) => w.name === 'Regular Workflow'); + assert(regularWf29 !== undefined, 'Regular type:workflow appears in workflows[]'); + + 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\n'); + await fs.writeFile( + path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill', 'workflow.md'), + '---\nname: Nested Skill\ndescription: desc\n---\nbody\n', + ); + + const scannedModules29 = await generator29.scanInstalledModules(tempFixture29); + assert(scannedModules29.includes('skill-only-mod'), 'scanInstalledModules recognizes skill-only module'); + } catch (error) { + assert(false, 'Unified skill scanner test succeeds', error.message); + } finally { + if (tempFixture29) await fs.remove(tempFixture29).catch(() => {}); + } + + console.log(''); + // ============================================================ // Summary // ============================================================ diff --git a/tools/cli/installers/lib/core/manifest-generator.js b/tools/cli/installers/lib/core/manifest-generator.js index db5fef7e6..8562672b5 100644 --- a/tools/cli/installers/lib/core/manifest-generator.js +++ b/tools/cli/installers/lib/core/manifest-generator.js @@ -108,6 +108,9 @@ class ManifestGenerator { // Reset files list (defensive: prevent stale data if instance is reused) this.files = []; + // Collect skills first (populates skillClaimedDirs before legacy collectors run) + await this.collectSkills(); + // Collect workflow data await this.collectWorkflows(selectedModules); @@ -132,6 +135,7 @@ class ManifestGenerator { ]; return { + skills: this.skills.length, workflows: this.workflows.length, agents: this.agents.length, tasks: this.tasks.length, @@ -141,13 +145,152 @@ class ManifestGenerator { }; } + /** + * Recursively walk a module directory tree, collecting skill directories. + * A skill directory is one that contains both a bmad-skill-manifest.yaml with + * type: skill AND a workflow.md (or workflow.yaml) file. + * Populates this.skills[] and this.skillClaimedDirs (Set of absolute paths). + */ + async collectSkills() { + this.skills = []; + this.skillClaimedDirs = new Set(); + const debug = process.env.BMAD_DEBUG_MANIFEST === 'true'; + + for (const moduleName of this.updatedModules) { + const modulePath = path.join(this.bmadDir, moduleName); + if (!(await fs.pathExists(modulePath))) continue; + + // Recursive walk skipping . and _ prefixed dirs + const walk = async (dir) => { + let entries; + try { + entries = await fs.readdir(dir, { withFileTypes: true }); + } catch { + return; + } + + // Check this directory for skill manifest + workflow file + const manifest = await this.loadSkillManifest(dir); + + // Try both workflow.md and workflow.yaml + const workflowFilenames = ['workflow.md', 'workflow.yaml']; + for (const workflowFile of workflowFilenames) { + const workflowPath = path.join(dir, workflowFile); + if (!(await fs.pathExists(workflowPath))) continue; + + const artifactType = this.getArtifactType(manifest, workflowFile); + if (artifactType !== 'skill') continue; + + // Read and parse the workflow file + try { + const rawContent = await fs.readFile(workflowPath, 'utf8'); + const content = rawContent.replaceAll('\r\n', '\n').replaceAll('\r', '\n'); + + let workflow; + if (workflowFile === 'workflow.yaml') { + workflow = yaml.parse(content); + } else { + const frontmatterMatch = content.match(/^---\n([\s\S]*?)\n---/); + if (!frontmatterMatch) { + if (debug) console.log(`[DEBUG] collectSkills: skipped (no frontmatter): ${workflowPath}`); + continue; + } + workflow = yaml.parse(frontmatterMatch[1]); + } + + if (!workflow || !workflow.name || !workflow.description) { + if (debug) console.log(`[DEBUG] collectSkills: skipped (missing name/description): ${workflowPath}`); + continue; + } + + // Build path relative from module root + const relativePath = path.relative(modulePath, dir).split(path.sep).join('/'); + const installPath = relativePath + ? `${this.bmadFolderName}/${moduleName}/${relativePath}/${workflowFile}` + : `${this.bmadFolderName}/${moduleName}/${workflowFile}`; + + // 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, + description: this.cleanForCSV(workflow.description), + module: moduleName, + path: installPath, + canonicalId, + 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) { + console.log(`[DEBUG] collectSkills: claimed skill "${workflow.name}" as ${canonicalId} at ${dir}`); + } + break; // Successfully claimed — skip remaining workflow filenames + } catch (error) { + if (debug) console.log(`[DEBUG] collectSkills: failed to parse ${workflowPath}: ${error.message}`); + } + } + + // Warn if manifest says type:skill but no workflow file found + if (manifest && !this.skillClaimedDirs.has(dir)) { + // Check if any entry in the manifest is type:skill + let hasSkillType = false; + if (manifest.__single) { + hasSkillType = manifest.__single.type === 'skill'; + } else { + for (const key of Object.keys(manifest)) { + if (manifest[key]?.type === 'skill') { + hasSkillType = true; + break; + } + } + } + if (hasSkillType && debug) { + const hasWorkflow = workflowFilenames.some((f) => entries.some((e) => e.name === f)); + 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}`); + } + } + } + + // Recurse into subdirectories + for (const entry of entries) { + if (!entry.isDirectory()) continue; + if (entry.name.startsWith('.') || entry.name.startsWith('_')) continue; + await walk(path.join(dir, entry.name)); + } + }; + + await walk(modulePath); + } + + if (debug) { + console.log(`[DEBUG] collectSkills: total skills found: ${this.skills.length}, claimed dirs: ${this.skillClaimedDirs.size}`); + } + } + /** * Collect all workflows from core and selected modules * Scans the INSTALLED bmad directory, not the source */ async collectWorkflows(selectedModules) { this.workflows = []; - this.skills = []; // Use updatedModules which already includes deduplicated 'core' + selectedModules for (const moduleName of this.updatedModules) { @@ -185,6 +328,9 @@ class ManifestGenerator { // Recursively find workflow.yaml files const findWorkflows = async (dir, relativePath = '') => { + // Skip directories already claimed as skills + if (this.skillClaimedDirs && this.skillClaimedDirs.has(dir)) return; + const entries = await fs.readdir(dir, { withFileTypes: true }); // Load skill manifest for this directory (if present) const skillManifest = await this.loadSkillManifest(dir); @@ -193,6 +339,8 @@ class ManifestGenerator { const fullPath = path.join(dir, entry.name); if (entry.isDirectory()) { + // Skip directories claimed by collectSkills + if (this.skillClaimedDirs && this.skillClaimedDirs.has(fullPath)) continue; // Recurse into subdirectories const newRelativePath = relativePath ? `${relativePath}/${entry.name}` : entry.name; await findWorkflows(fullPath, newRelativePath); @@ -216,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}`); @@ -253,25 +401,6 @@ class ManifestGenerator { ? `${this.bmadFolderName}/core/${subDir}/${relativePath}/${entry.name}` : `${this.bmadFolderName}/${moduleName}/${subDir}/${relativePath}/${entry.name}`; - // Check if this is a type:skill entry — collect separately, skip workflow CSV - const artifactType = this.getArtifactType(skillManifest, entry.name); - if (artifactType === 'skill') { - const canonicalId = path.basename(dir); - this.skills.push({ - name: workflow.name, - description: this.cleanForCSV(workflow.description), - module: moduleName, - path: installPath, - canonicalId, - install_to_bmad: this.getInstallToBmad(skillManifest, entry.name), - }); - - if (debug) { - console.log(`[DEBUG] ✓ Added skill (skipped workflow CSV): ${workflow.name} as ${canonicalId}`); - } - continue; - } - // Workflows with standalone: false are filtered out above workflows.push({ name: workflow.name, @@ -350,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) @@ -359,6 +490,8 @@ class ManifestGenerator { const fullPath = path.join(dirPath, entry.name); if (entry.isDirectory()) { + // Skip directories claimed by collectSkills + if (this.skillClaimedDirs && this.skillClaimedDirs.has(fullPath)) continue; // Recurse into subdirectories const newRelativePath = relativePath ? `${relativePath}/${entry.name}` : entry.name; const subDirAgents = await this.getAgentsFromDir(fullPath, moduleName, newRelativePath); @@ -447,6 +580,8 @@ class ManifestGenerator { * Get tasks from a directory */ async getTasksFromDir(dirPath, moduleName) { + // Skip directories claimed by collectSkills + if (this.skillClaimedDirs && this.skillClaimedDirs.has(dirPath)) return []; const tasks = []; const files = await fs.readdir(dirPath); // Load skill manifest for this directory (if present) @@ -548,6 +683,8 @@ class ManifestGenerator { * Get tools from a directory */ async getToolsFromDir(dirPath, moduleName) { + // Skip directories claimed by collectSkills + if (this.skillClaimedDirs && this.skillClaimedDirs.has(dirPath)) return []; const tools = []; const files = await fs.readdir(dirPath); // Load skill manifest for this directory (if present) @@ -1171,8 +1308,14 @@ class ManifestGenerator { const hasTasks = await fs.pathExists(path.join(modulePath, 'tasks')); const hasTools = await fs.pathExists(path.join(modulePath, 'tools')); - // If it has any of these directories, it's likely a module - if (hasAgents || hasWorkflows || hasTasks || hasTools) { + // Check for skill-only modules: recursive scan for bmad-skill-manifest.yaml with type: skill + let hasSkills = false; + if (!hasAgents && !hasWorkflows && !hasTasks && !hasTools) { + hasSkills = await this._hasSkillManifestRecursive(modulePath); + } + + // If it has any of these directories or skill manifests, it's likely a module + if (hasAgents || hasWorkflows || hasTasks || hasTools || hasSkills) { modules.push(entry.name); } } @@ -1182,6 +1325,37 @@ class ManifestGenerator { return modules; } + + /** + * Recursively check if a directory tree contains a bmad-skill-manifest.yaml with type: skill. + * Skips directories starting with . or _. + * @param {string} dir - Directory to search + * @returns {boolean} True if a skill manifest is found + */ + async _hasSkillManifestRecursive(dir) { + let entries; + try { + entries = await fs.readdir(dir, { withFileTypes: true }); + } catch { + return false; + } + + // Check for manifest in this directory + const manifest = await this.loadSkillManifest(dir); + if (manifest) { + const type = this.getArtifactType(manifest, 'workflow.md') || this.getArtifactType(manifest, 'workflow.yaml'); + if (type === 'skill') return true; + } + + // Recurse into subdirectories + for (const entry of entries) { + if (!entry.isDirectory()) continue; + if (entry.name.startsWith('.') || entry.name.startsWith('_')) continue; + if (await this._hasSkillManifestRecursive(path.join(dir, entry.name))) return true; + } + + return false; + } } module.exports = { ManifestGenerator };