refactor(skills): add SKILL.md entrypoint to skill directories#1868
Merged
alexeyv merged 5 commits intobmad-code-org:mainfrom Mar 9, 2026
Merged
refactor(skills): add SKILL.md entrypoint to skill directories#1868alexeyv merged 5 commits intobmad-code-org:mainfrom
alexeyv merged 5 commits intobmad-code-org:mainfrom
Conversation
Align skill source format with Open Skills standard: each skill directory now contains a SKILL.md with name/description frontmatter where name must match the directory name exactly. The installer copies skill directories verbatim instead of generating SKILL.md. - Add SKILL.md to both tracer bullet skill directories - Strip name/description from workflow.md frontmatter (SKILL.md owns it) - Installer reads metadata from SKILL.md, validates name matches dirname - Install path in manifest CSV now points to SKILL.md - Copy filter excludes OS/editor artifacts (.DS_Store, backups, dotfiles) - Debug-guard validation messages, keep name-mismatch as hard error - Add typeof guard for malformed YAML frontmatter - Add negative test cases for parseSkillMd validation (Suite 30)
🤖 Augment PR SummarySummary: This PR standardizes skills around a dedicated Changes:
Technical Notes: Skills continue to derive 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis PR migrates the skill detection and installation system from using Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test-installation-components.js (1)
1609-1697:⚠️ Potential issue | 🟠 MajorThese fixtures still stop short of the code path that changed.
Suite 29 only inspects
generator29.skillsin memory. The risky part of this PR is the serializedskill-manifest.csvrow plus_config-driven.installVerbatimSkills()copying/filtering the directory tree, so a badSKILL.mdpath or broken nested-artifact filter would still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-installation-components.js` around lines 1609 - 1697, The test only checks in-memory generator29.skills and doesn't exercise the serialization and installation code paths that were changed (skill-manifest.csv and _config-driven.installVerbatimSkills), so extend the fixture and assertions to write/read the serialized manifest and call the install routine: after generator29.generateManifests(tempFixture29,...), read the generated skill-manifest.csv (or equivalent manifest CSV produced by ManifestGenerator.generateManifests) and assert it contains the correct SKILL.md relative path for 'my-skill', then call _config-driven.installVerbatimSkills (or the public wrapper that triggers it) against the generated manifest and the temp install dir and assert the copied tree includes the SKILL.md at the expected nested path and that nested artifacts (like workflow.md inside skill-only modules) are filtered according to the new nested-artifact rules; use the existing generator29 and scannedModules29 variables (and functions generateManifests, scanInstalledModules, and installVerbatimSkills) to locate code paths to exercise.
🧹 Nitpick comments (1)
tools/cli/installers/lib/ide/_config-driven.js (1)
665-680: Don't delete the working skill before the replacement copy succeeds.Lines 667-680 make any mid-copy failure destructive: the old skill is already gone, and the new one is only partially written. Stage into a temp directory and rename on success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/_config-driven.js` around lines 665 - 680, Current logic removes skillDir before copying, which can leave you with a partially written skill on failure; instead create a temporary target (e.g., skillTmpDir = path.join(targetPath, `${canonicalId}.tmp-${Date.now()}`) or use fs.mkdtemp), ensure and copy all files from sourceDir into skillTmpDir using the same filtering logic (entries loop and fs.copy), then on successful completion move/rename skillTmpDir to skillDir (or fs.move) replacing the old directory atomically; only remove the original skillDir after a successful copy or use fs.move to overwrite atomically so failures don’t delete the working skill.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test-installation-components.js`:
- Around line 1713-1714: Replace the timestamped directory creation for
tempFixture30 with fs.mkdtemp to avoid collisions: instead of building a path
with path.join(os.tmpdir(), `bmad-test-30-${Date.now()}`) and calling
fs.ensureDir, call fs.mkdtemp with a prefix like path.join(os.tmpdir(),
'bmad-test-30-') and assign its returned path to tempFixture30 (no separate
ensureDir call needed); update any usages of tempFixture30 accordingly.
- Around line 1686-1697: The test creates the "skill-only-mod" fixture but calls
generateManifests() before it, so it only verifies scanInstalledModules() in
isolation; move the creation of skill-only-mod (the fs.ensureDir and
fs.writeFile calls creating deep/nested/my-skill with bmad-skill-manifest.yaml,
SKILL.md, and workflow.md) to occur before invoking
generator29.generateManifests(), or re-run generateManifests() after creating
the skill-only-mod so that generateManifests() (and downstream collectSkills())
can discover and include that module in the emitted manifests; verify the
generated manifests include "skill-only-mod" (or the expected skill identifier)
rather than only asserting scanInstalledModules().
- Around line 1716-1765: Add tests that create SKILL.md frontmatter where
description is a non-string structured value (e.g., description: [ "a", "b" ]
and description: { text: "x" }) and ensure these fixtures are passed through the
full manifest generation path by invoking
ManifestGenerator.generateManifests(...) (in addition to parseSkillMd),
asserting the generator either skips/returns null for those skills or safely
normalizes them; reference the existing ManifestGenerator, parseSkillMd, and
generateManifests symbols when adding the new cases so the test exercises the
CSV/text normalization codepath that currently fails on structured descriptions.
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 670-680: The current artifact filtering only checks top-level
entries and then blindly copies directory subtrees, so nested dotfiles/temp
files slip through; replace the flat copy with a recursive copy that applies the
same checks for every Dirent: implement a helper (e.g., copyFiltered or
recursiveCopy) that reads each directory with fs.readdir(..., { withFileTypes:
true }), skips entries matching skipPatterns, skipSuffixes, or leading-dot rules
(but preserves '.gitkeep'), and for directories recurses into them creating the
destination dir, while for files performs fs.copy; update the call sites that
used fs.copy(sourceDir, skillDir) to use copyFiltered(sourceDir, skillDir) so
the artifact filter is applied recursively.
- Around line 656-660: The code uses new RegExp(`^${bmadFolderName}/`) to strip
a prefix from record.path which fails if bmadFolderName contains regex
metacharacters; update the logic where relativePath is computed (and the same
other occurrence later) to build a literal prefix string (prefix =
bmadFolderName + '/') and use record.path.startsWith(prefix) ?
record.path.slice(prefix.length) : record.path to derive relativePath, then
continue to join with bmadDir to produce sourceFile; apply the identical change
to the second occurrence mentioned so both prefix-strip sites use
startsWith/slice.
---
Outside diff comments:
In `@test/test-installation-components.js`:
- Around line 1609-1697: The test only checks in-memory generator29.skills and
doesn't exercise the serialization and installation code paths that were changed
(skill-manifest.csv and _config-driven.installVerbatimSkills), so extend the
fixture and assertions to write/read the serialized manifest and call the
install routine: after generator29.generateManifests(tempFixture29,...), read
the generated skill-manifest.csv (or equivalent manifest CSV produced by
ManifestGenerator.generateManifests) and assert it contains the correct SKILL.md
relative path for 'my-skill', then call _config-driven.installVerbatimSkills (or
the public wrapper that triggers it) against the generated manifest and the temp
install dir and assert the copied tree includes the SKILL.md at the expected
nested path and that nested artifacts (like workflow.md inside skill-only
modules) are filtered according to the new nested-artifact rules; use the
existing generator29 and scannedModules29 variables (and functions
generateManifests, scanInstalledModules, and installVerbatimSkills) to locate
code paths to exercise.
---
Nitpick comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 665-680: Current logic removes skillDir before copying, which can
leave you with a partially written skill on failure; instead create a temporary
target (e.g., skillTmpDir = path.join(targetPath,
`${canonicalId}.tmp-${Date.now()}`) or use fs.mkdtemp), ensure and copy all
files from sourceDir into skillTmpDir using the same filtering logic (entries
loop and fs.copy), then on successful completion move/rename skillTmpDir to
skillDir (or fs.move) replacing the old directory atomically; only remove the
original skillDir after a successful copy or use fs.move to overwrite atomically
so failures don’t delete the working skill.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e8fda07-9c00-4858-a5ea-20a156032dd5
📒 Files selected for processing (7)
src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/SKILL.mdsrc/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/workflow.mdsrc/core/tasks/bmad-review-adversarial-general/SKILL.mdsrc/core/tasks/bmad-review-adversarial-general/workflow.mdtest/test-installation-components.jstools/cli/installers/lib/core/manifest-generator.jstools/cli/installers/lib/ide/_config-driven.js
💤 Files with no reviewable changes (2)
- src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/workflow.md
- src/core/tasks/bmad-review-adversarial-general/workflow.md
Add trigger context so LLMs know when to invoke the skill, matching the "Use when..." pattern used by other skills.
…killMd Prevents cleanForCSV() crash when YAML parses name or description as a non-string type (number, object, boolean).
…e filter) - Replace Date.now() temp dir with fs.mkdtemp() in Suite 30 tests (F5) - Replace unescaped RegExp with startsWith/slice for path prefix stripping (F7) - Apply artifact filter recursively via fs.copy filter option (F8) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 9, 2026
This was referenced Mar 13, 2026
[BUG] Gemini reports skill conflict when BMAD skills are installed in both .gemini and .agents
#2028
Open
This was referenced Mar 28, 2026
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
SKILL.mdwith name/description frontmatter to both tracer bullet skill directories, aligning with the Open Skills standardSKILL.mdinstead ofworkflow.md, validates name matches directory name exactlySKILL.mdSKILL.md(the permanent entrypoint)bmad-skill-manifest.yamlfrom copied skillstypeofguard for malformed YAML frontmatterparseSkillMdvalidation (Suite 30)Test plan