feat(manifest): unified skill scanner decoupled from legacy collectors#1859
Merged
alexeyv merged 2 commits intobmad-code-org:mainfrom Mar 8, 2026
Merged
feat(manifest): unified skill scanner decoupled from legacy collectors#1859alexeyv merged 2 commits intobmad-code-org:mainfrom
alexeyv merged 2 commits intobmad-code-org:mainfrom
Conversation
🤖 Augment PR SummarySummary: This PR introduces a unified, recursive skill discovery pass in the manifest generator so skills can be detected anywhere in a module tree (not just under legacy collector paths). Changes:
Technical Notes: Skill identification is now driven by manifest 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe changes introduce automatic skill discovery and categorization to the manifest generator. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/cli/installers/lib/core/manifest-generator.js (1)
469-475:⚠️ Potential issue | 🟠 Major
getAgentsFromDir()still processes a claimed root directory.The new guard only skips claimed child directories. If
dirPathitself is inskillClaimedDirs, this collector will still parse its.mdfiles as agents.getTasksFromDir()andgetToolsFromDir()already have the early-return pattern this method is missing.Possible fix
async getAgentsFromDir(dirPath, moduleName, relativePath = '') { + if (this.skillClaimedDirs && this.skillClaimedDirs.has(dirPath)) return []; const agents = []; const entries = await fs.readdir(dirPath, { withFileTypes: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 469 - 475, getAgentsFromDir is missing the early-return that avoids processing a claimed root directory: add the same guard used in getTasksFromDir/getToolsFromDir so that if this.skillClaimedDirs exists and contains the incoming dirPath (or fullPath representing the root being processed) the function immediately returns an empty array; update the check near the top of getAgentsFromDir to short-circuit before reading entries and before parsing any .md files to mirror the existing pattern in getTasksFromDir/getToolsFromDir.
🧹 Nitpick comments (7)
tools/cli/installers/lib/core/manifest-generator.js (2)
222-227: Stop descending once a directory is claimed as a skill.After a successful claim, the parent skill already owns that subtree. Continuing the walk below it can produce overlapping nested skill entries and burns extra traversal time for no upside.
Possible fix
if (debug) { console.log(`[DEBUG] collectSkills: claimed skill "${workflow.name}" as ${canonicalId} at ${dir}`); } - break; // Successfully claimed — skip remaining workflow filenames + return; // Claimed skill owns this subtreeAlso applies to: 255-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 222 - 227, The collectSkills traversal currently adds a claimed directory via this.skillClaimedDirs.add(dir) and breaks out of the filename loop but continues descending into the directory subtree; update collectSkills (and the analogous block around the other claim at lines ~255-259) to stop descending once a directory is claimed by returning early or marking the directory as claimed before recursing so the walker skips its children—ensure the change occurs where workflow.name and canonicalId are used to claim the dir so that after the console.debug/claim call the function does not traverse into that dir's subtree.
111-112: Collapse skill discovery and skill-only module detection into one traversal.
scanInstalledModules()now recursively walks each module tree, andcollectSkills()immediately walks the same trees again. InsidecollectSkills()you also probeworkflow.md/workflow.yamlwithpathExists()afterreaddir()already told you what is in the directory. On larger custom modules this doubles the hot-path I/O for manifest generation.Also applies to: 153-179, 1288-1335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 111 - 112, scanInstalledModules() and collectSkills() are performing duplicate recursive walks and redundant file checks; consolidate skill discovery into the single traversal inside scanInstalledModules() so you only walk each module tree once, populate skillClaimedDirs during that pass, and remove the separate collectSkills() recursion. While traversing, use the directory entries returned by readdir() to detect workflow.md / workflow.yaml instead of calling pathExists() again (i.e., check names in the readdir() result), and remove the extra scans referenced around collectSkills() and the duplicated ranges (lines ~153-179 and ~1288-1335). Ensure the logic that previously lived in collectSkills() (claiming skill directories and detecting skill-only modules) is invoked from scanInstalledModules() so callers of collectSkills() can be removed or routed to the new single-pass implementation.test/test-installation-components.js (5)
1668-1681: Make the skill-only-module case end-to-end, and add the negative path.
skill-only-modis created aftergenerateManifests(), so this only proves the helper returns its name. It never proves the module flows through a real manifest-generation run, and it does not protect against the orphan-manifest false positive (type: skillwith noworkflow.md/workflow.yaml).🤖 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 1668 - 1681, The test creates skill-only-mod after generateManifests so it never verifies an end-to-end manifest generation; move the creation of the skill-only-mod (and its bmad-skill-manifest.yaml + workflow.md) so it exists before calling generator29.generateManifests() or explicitly call generator29.generateManifests() after creating it, then assert scanInstalledModules(tempFixture29) and the generated manifests include 'skill-only-mod' (reference: generator29.generateManifests, generator29.scanInstalledModules, tempFixture29, skill-only-mod). Also add a negative-case directory (e.g., orphan-skill-mod) that contains only bmad-skill-manifest.yaml with type: skill but no workflow.md/workflow.yaml and assert it is NOT included by scanInstalledModules/generation to prevent the orphan-manifest false positive.
1637-1681: Read the generated CSVs too.The feature here is manifest output, but the suite only inspects
generator29.skills,workflows, andtasks. A regression inskill-manifest.csvorworkflow-manifest.csvserialization 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 1637 - 1681, The test currently only checks generator29.skills/workflows/tasks but misses validating the generated CSV outputs; update the test around ManifestGenerator/generator29 to also read and assert contents of the produced CSV files (e.g., skill-manifest.csv and workflow-manifest.csv in the temp fixture output directory) to ensure the same entries appear (presence of my-custom-skill, task-skill not appearing in tasks CSV, regular-wf only in workflow CSV, and nested-skill present), and fail the test if the CSVs are missing or their rows do not match the expected canonicalIds/names; use generator29 (and/or ManifestGenerator.generateManifests/scanInstalledModules) to locate the output CSVs and parse them (CSV parse or simple line-splitting) before asserting.
1605-1652: This does not cover the workflow-collector regression path.The “unusual path” skill lives under
core/custom-area, socollectWorkflows()would never have seen it before or after this PR. The assertion that it stays out ofworkflows[]will stay green even if the new claimed-dir skip ingetWorkflowsFromPath()is removed.🤖 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 1605 - 1652, The test misses exercising the workflow-collector path, so add a check that calls the collector path (either invoke getWorkflowsFromPath() or the collector method used by ManifestGenerator, e.g., collectWorkflows() / ManifestGenerator.generateManifests() code path that scans workflows) against the module root so claimed dirs are skipped; specifically, after creating core/custom-area/my-skill, call the workflow-collection function used internally (getWorkflowsFromPath or the collector helper used by ManifestGenerator) and assert that the resulting workflows array (generator.workflows or the collector return value) does not include the 'my-custom-skill' entry to guarantee the claimed-dir skip remains enforced.
1623-1659: The task-skill case is vacuous against the current collector.
getTasksFromDir()only inspects files directly undercore/tasks, socore/tasks/task-skill/workflow.mdwould not land intasks[]even without the newskillClaimedDirsguard. This fixture does not exercise the changed branch.🤖 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 1623 - 1659, The test is vacuous because getTasksFromDir only looks at files directly under core/tasks, so putting workflow.md at core/tasks/task-skill/workflow.md never reaches the tasks-collection logic and doesn't exercise the new skillClaimedDirs guard; update the fixture so the task-skill workflow file is placed where getTasksFromDir will see it (e.g., move workflow.md to core/tasks/task-skill.md or create core/tasks/task-skill/workflow.md and adjust collector to recurse) or else change the assertion to target skills[] behavior only; reference getTasksFromDir and skillClaimedDirs when making the change so the test actually exercises the guarded branch.
1593-1681: Agent/tool claimed-dir behavior still has no direct coverage.This suite only probes workflow and task classification. The new claimed-dir branches in
getAgentsFromDir()andgetToolsFromDir()can regress independently and these assertions would never notice.🤖 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 1593 - 1681, The test suite misses coverage for the new "claimed-dir" branches so regressions in getAgentsFromDir() and getToolsFromDir() won't be caught; add test steps in this suite to create modules that use the claimed-dir pattern (e.g., create a directory that should be treated as an agent or tool via the claimed-dir marker the generator expects), invoke ManifestGenerator.generateManifests() and scanInstalledModules(), then assert that entries returned by getAgentsFromDir()/getToolsFromDir() (accessible via generator.agents or generator.tools after generateManifests) include the claimed-dir items and are not misclassified as workflows/tasks/skills; reference generator instance, methods generateManifests(), scanInstalledModules(), getAgentsFromDir(), and getToolsFromDir() when adding the assertions.
🤖 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/core/manifest-generator.js`:
- Around line 211-219: The code must not synthesize canonicalId from the
directory name; instead validate the manifest-provided canonicalId returned by
getCanonicalId(manifest, workflowFile) and if it's missing or invalid emit a
warning and skip adding this skill to this.skills; update the logic around
getCanonicalId(...) and the skills.push(...) block (where
name/description/module/path/canonicalId/install_to_bmad are assembled) to log a
clear warning (e.g. via the class logger or console.warn) referencing the
workflowFile/dir and then continue without pushing the malformed manifest entry.
---
Outside diff comments:
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 469-475: getAgentsFromDir is missing the early-return that avoids
processing a claimed root directory: add the same guard used in
getTasksFromDir/getToolsFromDir so that if this.skillClaimedDirs exists and
contains the incoming dirPath (or fullPath representing the root being
processed) the function immediately returns an empty array; update the check
near the top of getAgentsFromDir to short-circuit before reading entries and
before parsing any .md files to mirror the existing pattern in
getTasksFromDir/getToolsFromDir.
---
Nitpick comments:
In `@test/test-installation-components.js`:
- Around line 1668-1681: The test creates skill-only-mod after generateManifests
so it never verifies an end-to-end manifest generation; move the creation of the
skill-only-mod (and its bmad-skill-manifest.yaml + workflow.md) so it exists
before calling generator29.generateManifests() or explicitly call
generator29.generateManifests() after creating it, then assert
scanInstalledModules(tempFixture29) and the generated manifests include
'skill-only-mod' (reference: generator29.generateManifests,
generator29.scanInstalledModules, tempFixture29, skill-only-mod). Also add a
negative-case directory (e.g., orphan-skill-mod) that contains only
bmad-skill-manifest.yaml with type: skill but no workflow.md/workflow.yaml and
assert it is NOT included by scanInstalledModules/generation to prevent the
orphan-manifest false positive.
- Around line 1637-1681: The test currently only checks
generator29.skills/workflows/tasks but misses validating the generated CSV
outputs; update the test around ManifestGenerator/generator29 to also read and
assert contents of the produced CSV files (e.g., skill-manifest.csv and
workflow-manifest.csv in the temp fixture output directory) to ensure the same
entries appear (presence of my-custom-skill, task-skill not appearing in tasks
CSV, regular-wf only in workflow CSV, and nested-skill present), and fail the
test if the CSVs are missing or their rows do not match the expected
canonicalIds/names; use generator29 (and/or
ManifestGenerator.generateManifests/scanInstalledModules) to locate the output
CSVs and parse them (CSV parse or simple line-splitting) before asserting.
- Around line 1605-1652: The test misses exercising the workflow-collector path,
so add a check that calls the collector path (either invoke
getWorkflowsFromPath() or the collector method used by ManifestGenerator, e.g.,
collectWorkflows() / ManifestGenerator.generateManifests() code path that scans
workflows) against the module root so claimed dirs are skipped; specifically,
after creating core/custom-area/my-skill, call the workflow-collection function
used internally (getWorkflowsFromPath or the collector helper used by
ManifestGenerator) and assert that the resulting workflows array
(generator.workflows or the collector return value) does not include the
'my-custom-skill' entry to guarantee the claimed-dir skip remains enforced.
- Around line 1623-1659: The test is vacuous because getTasksFromDir only looks
at files directly under core/tasks, so putting workflow.md at
core/tasks/task-skill/workflow.md never reaches the tasks-collection logic and
doesn't exercise the new skillClaimedDirs guard; update the fixture so the
task-skill workflow file is placed where getTasksFromDir will see it (e.g., move
workflow.md to core/tasks/task-skill.md or create
core/tasks/task-skill/workflow.md and adjust collector to recurse) or else
change the assertion to target skills[] behavior only; reference getTasksFromDir
and skillClaimedDirs when making the change so the test actually exercises the
guarded branch.
- Around line 1593-1681: The test suite misses coverage for the new
"claimed-dir" branches so regressions in getAgentsFromDir() and
getToolsFromDir() won't be caught; add test steps in this suite to create
modules that use the claimed-dir pattern (e.g., create a directory that should
be treated as an agent or tool via the claimed-dir marker the generator
expects), invoke ManifestGenerator.generateManifests() and
scanInstalledModules(), then assert that entries returned by
getAgentsFromDir()/getToolsFromDir() (accessible via generator.agents or
generator.tools after generateManifests) include the claimed-dir items and are
not misclassified as workflows/tasks/skills; reference generator instance,
methods generateManifests(), scanInstalledModules(), getAgentsFromDir(), and
getToolsFromDir() when adding the assertions.
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 222-227: The collectSkills traversal currently adds a claimed
directory via this.skillClaimedDirs.add(dir) and breaks out of the filename loop
but continues descending into the directory subtree; update collectSkills (and
the analogous block around the other claim at lines ~255-259) to stop descending
once a directory is claimed by returning early or marking the directory as
claimed before recursing so the walker skips its children—ensure the change
occurs where workflow.name and canonicalId are used to claim the dir so that
after the console.debug/claim call the function does not traverse into that
dir's subtree.
- Around line 111-112: scanInstalledModules() and collectSkills() are performing
duplicate recursive walks and redundant file checks; consolidate skill discovery
into the single traversal inside scanInstalledModules() so you only walk each
module tree once, populate skillClaimedDirs during that pass, and remove the
separate collectSkills() recursion. While traversing, use the directory entries
returned by readdir() to detect workflow.md / workflow.yaml instead of calling
pathExists() again (i.e., check names in the readdir() result), and remove the
extra scans referenced around collectSkills() and the duplicated ranges (lines
~153-179 and ~1288-1335). Ensure the logic that previously lived in
collectSkills() (claiming skill directories and detecting skill-only modules) is
invoked from scanInstalledModules() so callers of collectSkills() can be removed
or routed to the new single-pass implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8a7cce3-0110-4554-8d51-a3542dac3ce3
📒 Files selected for processing (2)
test/test-installation-components.jstools/cli/installers/lib/core/manifest-generator.js
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
alexeyv
force-pushed
the
unified-skill-scanner
branch
from
7427f03 to
149c404
Compare
This was referenced Mar 8, 2026
3 tasks
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
collectSkills()that recursively walks each module tree to discovertype: skilldirectories anywhere, replacing the band-aid detection insidecollectWorkflows()scanInstalledModules()recognizes skill-only modules (noagents//workflows//tasks//tools/subdirs needed)Test plan
--modules bmm --tools claude-codeand verifyskill-manifest.csvoutput matches expectations🤖 Generated with Claude Code