refactor(installer): discover skills by SKILL.md instead of manifest YAML#2081
Closed
refactor(installer): discover skills by SKILL.md instead of manifest YAML#2081
Conversation
alexeyv
force-pushed
the
remove-skill-manifest-yaml
branch
from
c533ccf to
cf22a7d
Compare
🤖 Augment PR SummarySummary: This PR refactors the installer’s “native skill” discovery to use Changes:
Technical Notes: Manifest YAML is still loaded opportunistically when present (e.g., for 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| // Copy agents directory | ||
| const agentsDir = path.join(customPath, 'agents'); | ||
| if (await fs.pathExists(agentsDir)) { | ||
| await this.copyDirectory(agentsDir, bmadAgentsDir, results, fileTrackingCallback, config); |
There was a problem hiding this comment.
copyDirectory() increments results.workflowsInstalled for every copied .md; when it’s used for the agents/ directory, installs that include only custom agents (no workflows/) can end up reporting non-zero workflowsInstalled in the results summary.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
📝 WalkthroughWalkthroughThis PR removes legacy agent compilation infrastructure and shifts skill discovery from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/cli/installers/lib/core/manifest-generator.js (1)
1342-1381:⚠️ Potential issue | 🟡 MinorUse SKILL validation in
scanInstalledModules()too.
_hasSkillMdRecursive()now treats any file namedSKILL.mdas enough to classify a skill-only module. That includes the same invalid casesparseSkillMd()skips, so a module with a stub or malformedSKILL.mdstill lands ininstalledModuleseven though no skill will ever be collected from it. ReuseparseSkillMd()here so module detection and discovery stay in sync.♻️ Keep module scanning aligned with the real SKILL contract
- hasSkills = await this._hasSkillMdRecursive(modulePath); + hasSkills = await this._hasValidSkillMdRecursive(modulePath); ... - async _hasSkillMdRecursive(dir) { + async _hasValidSkillMdRecursive(dir) { let entries; try { entries = await fs.readdir(dir, { withFileTypes: true }); } catch { return false; } - // Check for SKILL.md in this directory - if (entries.some((e) => !e.isDirectory() && e.name === 'SKILL.md')) return true; + if (entries.some((e) => !e.isDirectory() && e.name === 'SKILL.md')) { + const dirName = path.basename(dir); + if (await this.parseSkillMd(path.join(dir, 'SKILL.md'), dir, dirName)) { + 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._hasSkillMdRecursive(path.join(dir, entry.name))) return true; + if (await this._hasValidSkillMdRecursive(path.join(dir, entry.name))) return true; }tools/cli/installers/lib/modules/manager.js (1)
422-466:⚠️ Potential issue | 🟠 MajorFail fast on legacy
.agent.yamlmodules now that compilation is gone.The compile/process phase has been removed from
install()andupdate(), butcopyModuleWithFiltering()still skips.agent.yamlandsyncModule()never regenerates their.mdoutputs. Fresh installs and force updates will drop those agents entirely; selective updates can copy the raw source files without updating the usable artifacts. Any custom/external module still on the old format now ends up partially installed while the command still reports success.🛡️ Add an explicit guard for unsupported legacy agent sources
+ async assertNoLegacyAgentYaml(sourcePath, moduleName) { + const legacyAgentFiles = (await this.getFileList(sourcePath)).filter((file) => file.endsWith('.agent.yaml')); + if (legacyAgentFiles.length > 0) { + throw new Error( + `Module "${moduleName}" still ships legacy .agent.yaml agents, but agent compilation has been removed. ` + + `Migrate them to prebuilt Markdown/SKILL.md before installing or updating.` + ); + } + } ... async install(moduleName, bmadDir, fileTrackingCallback = null, options = {}) { const sourcePath = await this.findModuleSource(moduleName, { silent: options.silent }); const targetPath = path.join(bmadDir, moduleName); // Check if source module exists if (!sourcePath) { throw new Error( `Source for module '${moduleName}' is not available. It will be retained but cannot be updated without its source files.`, ); } + + await this.assertNoLegacyAgentYaml(sourcePath, moduleName); ... async update(moduleName, bmadDir, force = false, options = {}) { const sourcePath = await this.findModuleSource(moduleName); const targetPath = path.join(bmadDir, moduleName); // Check if source module exists if (!sourcePath) { throw new Error(`Module '${moduleName}' not found in any source location`); } + + await this.assertNoLegacyAgentYaml(sourcePath, moduleName);As per coding guidelines,
tools/**: Build script/tooling. Check error handling and proper exit codes.Also applies to: 498-519, 599-636, 985-1006
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/modules/manager.js` around lines 422 - 466, The installer currently allows legacy modules that contain .agent.yaml files to be copied (via copyModuleWithFiltering) even though compilation/processing was removed; add a fail-fast guard that detects legacy agent source files and throws a clear error instead of partially installing them. Specifically, in install() (and mirror the same check in update() and any force-update paths) after resolving sourcePath (before vendorCrossModuleWorkflows and copyModuleWithFiltering) scan sourcePath for any "*.agent.yaml" or legacy "agent.yaml" files and if any are present throw an Error explaining legacy-agent format is unsupported and must be migrated; also add the same validation to syncModule() (and any callers that assume processed artifacts) so legacy sources never produce partial installs. Use the existing symbols install(), update(), syncModule(), copyModuleWithFiltering(), vendorCrossModuleWorkflows(), and findModuleSource() to locate where to add the check and error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/cli/installers/lib/custom/handler.js`:
- Around line 144-150: The installer currently copies the entire agentsDir
(variable agentsDir -> bmadAgentsDir) but only counts Markdown agents via
findFilesRecursively, which lets legacy *.agent.yaml packs succeed with
agentsInstalled === 0; update the handler in this block to detect legacy agent
YAML files before copying (e.g., scan agentsDir for files matching *.agent.yaml
or *.agent.yml) and either (A) perform a compatibility conversion into the
supported .md artifact format or (B) throw a clear error and abort the install
with a non-zero exit code; the change should reference the existing
copyDirectory, findFilesRecursively usage and ensure any error path logs a
descriptive message about unsupported legacy custom agent YAML and prevents
copying to bmadAgentsDir.
---
Outside diff comments:
In `@tools/cli/installers/lib/modules/manager.js`:
- Around line 422-466: The installer currently allows legacy modules that
contain .agent.yaml files to be copied (via copyModuleWithFiltering) even though
compilation/processing was removed; add a fail-fast guard that detects legacy
agent source files and throws a clear error instead of partially installing
them. Specifically, in install() (and mirror the same check in update() and any
force-update paths) after resolving sourcePath (before
vendorCrossModuleWorkflows and copyModuleWithFiltering) scan sourcePath for any
"*.agent.yaml" or legacy "agent.yaml" files and if any are present throw an
Error explaining legacy-agent format is unsupported and must be migrated; also
add the same validation to syncModule() (and any callers that assume processed
artifacts) so legacy sources never produce partial installs. Use the existing
symbols install(), update(), syncModule(), copyModuleWithFiltering(),
vendorCrossModuleWorkflows(), and findModuleSource() to locate where to add the
check and error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a59a9248-b747-4bcd-88d3-73b3fac782e9
📒 Files selected for processing (51)
src/bmm-skills/1-analysis/bmad-document-project/bmad-skill-manifest.yamlsrc/bmm-skills/1-analysis/bmad-product-brief/bmad-skill-manifest.yamlsrc/bmm-skills/1-analysis/research/bmad-domain-research/bmad-skill-manifest.yamlsrc/bmm-skills/1-analysis/research/bmad-market-research/bmad-skill-manifest.yamlsrc/bmm-skills/1-analysis/research/bmad-technical-research/bmad-skill-manifest.yamlsrc/bmm-skills/2-plan-workflows/bmad-create-prd/bmad-skill-manifest.yamlsrc/bmm-skills/2-plan-workflows/bmad-create-ux-design/bmad-skill-manifest.yamlsrc/bmm-skills/2-plan-workflows/bmad-edit-prd/bmad-skill-manifest.yamlsrc/bmm-skills/2-plan-workflows/bmad-validate-prd/bmad-skill-manifest.yamlsrc/bmm-skills/3-solutioning/bmad-check-implementation-readiness/bmad-skill-manifest.yamlsrc/bmm-skills/3-solutioning/bmad-create-architecture/bmad-skill-manifest.yamlsrc/bmm-skills/3-solutioning/bmad-create-epics-and-stories/bmad-skill-manifest.yamlsrc/bmm-skills/3-solutioning/bmad-generate-project-context/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-code-review/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-correct-course/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-create-story/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-dev-story/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-qa-generate-e2e-tests/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-quick-dev/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-retrospective/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-sprint-planning/bmad-skill-manifest.yamlsrc/bmm-skills/4-implementation/bmad-sprint-status/bmad-skill-manifest.yamlsrc/core-skills/bmad-advanced-elicitation/bmad-skill-manifest.yamlsrc/core-skills/bmad-brainstorming/bmad-skill-manifest.yamlsrc/core-skills/bmad-distillator/bmad-skill-manifest.yamlsrc/core-skills/bmad-editorial-review-prose/bmad-skill-manifest.yamlsrc/core-skills/bmad-editorial-review-structure/bmad-skill-manifest.yamlsrc/core-skills/bmad-help/bmad-skill-manifest.yamlsrc/core-skills/bmad-index-docs/bmad-skill-manifest.yamlsrc/core-skills/bmad-init/bmad-skill-manifest.yamlsrc/core-skills/bmad-party-mode/bmad-skill-manifest.yamlsrc/core-skills/bmad-review-adversarial-general/bmad-skill-manifest.yamlsrc/core-skills/bmad-review-edge-case-hunter/bmad-skill-manifest.yamlsrc/core-skills/bmad-shard-doc/bmad-skill-manifest.yamltest/test-installation-components.jstools/cli/commands/install.jstools/cli/installers/lib/core/installer.jstools/cli/installers/lib/core/manifest-generator.jstools/cli/installers/lib/custom/handler.jstools/cli/installers/lib/ide/_base-ide.jstools/cli/installers/lib/modules/manager.jstools/cli/lib/activation-builder.jstools/cli/lib/agent-analyzer.jstools/cli/lib/agent-party-generator.jstools/cli/lib/agent/compiler.jstools/cli/lib/agent/installer.jstools/cli/lib/agent/template-engine.jstools/cli/lib/ui.jstools/cli/lib/xml-handler.jstools/cli/lib/xml-to-markdown.jstools/cli/lib/yaml-xml-builder.js
💤 Files with no reviewable changes (45)
- src/bmm-skills/4-implementation/bmad-create-story/bmad-skill-manifest.yaml
- src/bmm-skills/1-analysis/research/bmad-domain-research/bmad-skill-manifest.yaml
- src/bmm-skills/1-analysis/bmad-document-project/bmad-skill-manifest.yaml
- src/core-skills/bmad-review-adversarial-general/bmad-skill-manifest.yaml
- src/bmm-skills/4-implementation/bmad-code-review/bmad-skill-manifest.yaml
- src/bmm-skills/4-implementation/bmad-sprint-planning/bmad-skill-manifest.yaml
- src/bmm-skills/4-implementation/bmad-dev-story/bmad-skill-manifest.yaml
- src/bmm-skills/3-solutioning/bmad-create-epics-and-stories/bmad-skill-manifest.yaml
- src/bmm-skills/4-implementation/bmad-retrospective/bmad-skill-manifest.yaml
- src/core-skills/bmad-editorial-review-structure/bmad-skill-manifest.yaml
- src/bmm-skills/4-implementation/bmad-correct-course/bmad-skill-manifest.yaml
- src/bmm-skills/1-analysis/research/bmad-market-research/bmad-skill-manifest.yaml
- src/bmm-skills/1-analysis/research/bmad-technical-research/bmad-skill-manifest.yaml
- src/core-skills/bmad-index-docs/bmad-skill-manifest.yaml
- src/core-skills/bmad-review-edge-case-hunter/bmad-skill-manifest.yaml
- src/bmm-skills/4-implementation/bmad-qa-generate-e2e-tests/bmad-skill-manifest.yaml
- src/bmm-skills/2-plan-workflows/bmad-validate-prd/bmad-skill-manifest.yaml
- src/core-skills/bmad-init/bmad-skill-manifest.yaml
- src/bmm-skills/2-plan-workflows/bmad-edit-prd/bmad-skill-manifest.yaml
- src/bmm-skills/4-implementation/bmad-sprint-status/bmad-skill-manifest.yaml
- src/core-skills/bmad-editorial-review-prose/bmad-skill-manifest.yaml
- src/core-skills/bmad-party-mode/bmad-skill-manifest.yaml
- src/bmm-skills/3-solutioning/bmad-generate-project-context/bmad-skill-manifest.yaml
- src/core-skills/bmad-advanced-elicitation/bmad-skill-manifest.yaml
- src/bmm-skills/2-plan-workflows/bmad-create-ux-design/bmad-skill-manifest.yaml
- src/core-skills/bmad-shard-doc/bmad-skill-manifest.yaml
- src/bmm-skills/3-solutioning/bmad-create-architecture/bmad-skill-manifest.yaml
- tools/cli/lib/agent-analyzer.js
- src/core-skills/bmad-brainstorming/bmad-skill-manifest.yaml
- tools/cli/installers/lib/ide/_base-ide.js
- src/core-skills/bmad-help/bmad-skill-manifest.yaml
- tools/cli/lib/xml-handler.js
- tools/cli/lib/ui.js
- src/core-skills/bmad-distillator/bmad-skill-manifest.yaml
- src/bmm-skills/1-analysis/bmad-product-brief/bmad-skill-manifest.yaml
- src/bmm-skills/3-solutioning/bmad-check-implementation-readiness/bmad-skill-manifest.yaml
- tools/cli/lib/xml-to-markdown.js
- src/bmm-skills/4-implementation/bmad-quick-dev/bmad-skill-manifest.yaml
- tools/cli/lib/agent/compiler.js
- tools/cli/lib/agent/template-engine.js
- src/bmm-skills/2-plan-workflows/bmad-create-prd/bmad-skill-manifest.yaml
- tools/cli/lib/activation-builder.js
- tools/cli/lib/yaml-xml-builder.js
- tools/cli/lib/agent-party-generator.js
- tools/cli/lib/agent/installer.js
Comment on lines
+144
to
+150
| // Copy agents directory | ||
| const agentsDir = path.join(customPath, 'agents'); | ||
| if (await fs.pathExists(agentsDir)) { | ||
| await this.copyDirectory(agentsDir, bmadAgentsDir, results, fileTrackingCallback, config); | ||
|
|
||
| // Count agent files | ||
| const agentFiles = await this.findFilesRecursively(agentsDir, ['.md']); |
There was a problem hiding this comment.
Reject legacy custom agent YAML instead of silently copying it.
This path now copies agents/ verbatim, but only .md files are treated as installed agents. A custom pack that still ships *.agent.yaml will now return success with agentsInstalled === 0 and no usable agent artifact. Please either preserve a compatibility conversion here or throw a clear error before copying unsupported agent sources.
🛠️ Fail fast on unsupported legacy custom agents
const agentsDir = path.join(customPath, 'agents');
if (await fs.pathExists(agentsDir)) {
+ const legacyAgentFiles = await this.findFilesRecursively(agentsDir, ['.agent.yaml']);
+ if (legacyAgentFiles.length > 0) {
+ throw new Error(
+ `Legacy custom agent sources are no longer supported: ${legacyAgentFiles
+ .map((file) => path.relative(customPath, file))
+ .join(', ')}`
+ );
+ }
+
await this.copyDirectory(agentsDir, bmadAgentsDir, results, fileTrackingCallback, config);
// Count agent files
const agentFiles = await this.findFilesRecursively(agentsDir, ['.md']);As per coding guidelines, tools/**: Build script/tooling. Check error handling and proper exit codes.
Also applies to: 249-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/cli/installers/lib/custom/handler.js` around lines 144 - 150, The
installer currently copies the entire agentsDir (variable agentsDir ->
bmadAgentsDir) but only counts Markdown agents via findFilesRecursively, which
lets legacy *.agent.yaml packs succeed with agentsInstalled === 0; update the
handler in this block to detect legacy agent YAML files before copying (e.g.,
scan agentsDir for files matching *.agent.yaml or *.agent.yml) and either (A)
perform a compatibility conversion into the supported .md artifact format or (B)
throw a clear error and abort the install with a non-zero exit code; the change
should reference the existing copyDirectory, findFilesRecursively usage and
ensure any error path logs a descriptive message about unsupported legacy custom
agent YAML and prevents copying to bmadAgentsDir.
cf22a7d to
c28206d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bmad-skill-manifest.yamlwithtype: skillto detecting any directory with a validSKILL.md(frontmatter name + description, name matches directory name)type: skill(including distillator's legacycapabilitiesblock)isNativeSkillDirType()andhasNativeSkillManifest(), rename_hasSkillManifestRecursive()→_hasSkillMdRecursive()agent-manifest.csvTest plan
npm run qualitypasses (lint, format, markdownlint, docs build, 231 tests, ref validation, skill validation)test-install-to-bmad.jspasses unchanged — sharedskill-manifest.jsmodule untouchedvalidate:skillsfinds all 43 skills (no regressions from manifest removal)Follows
remove-agent-compilation-pipeline(parent branch).