feat(installer): lean shard-doc skill prototype intermediary-state PoC#1844
Closed
dickymoore wants to merge 11 commits intobmad-code-org:mainfrom
Closed
feat(installer): lean shard-doc skill prototype intermediary-state PoC#1844dickymoore wants to merge 11 commits intobmad-code-org:mainfrom
dickymoore wants to merge 11 commits intobmad-code-org:mainfrom
Conversation
🤖 Augment PR SummarySummary: Adds a transitional “prototype skill” wrapper for Changes:
Technical Notes: Prototype copying is intentionally narrow-scoped (hard-coded to 🤖 Was this summary useful? React with 👍 or 👎 |
📝 WalkthroughWalkthroughThe change extends the config-driven installer to support shard-doc prototype SKILL duplication via a new helper method, updates method signatures to pass bmadDir parameters, creates a prototype skill directory, and adds tests verifying installation behavior across skill-format and non-skill paths. Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant IdeManager
participant ConfigDrivenIdeSetup
participant writeTaskToolArtifacts
participant writeSkillFile
participant writeShardDocPrototypeSkill
participant FileSystem
Test->>IdeManager: initiate installation with bmadDir
IdeManager->>ConfigDrivenIdeSetup: execute installation flow
ConfigDrivenIdeSetup->>writeTaskToolArtifacts: write task tool artifacts (bmadDir passed)
writeTaskToolArtifacts->>writeSkillFile: delegate skill writing (bmadDir passed)
writeSkillFile->>FileSystem: write base SKILL.md
writeSkillFile->>writeShardDocPrototypeSkill: copy prototype (bmadDir, skillName)
alt bmadDir present AND skillName is 'bmad-shard-doc'
writeShardDocPrototypeSkill->>FileSystem: copy prototype SKILL.md from BMAD source
else
writeShardDocPrototypeSkill->>writeShardDocPrototypeSkill: skip duplication
end
writeSkillFile-->>ConfigDrivenIdeSetup: skill installation complete
ConfigDrivenIdeSetup-->>Test: installation artifacts provisioned
Test->>FileSystem: validate canonical and prototype artifacts
Test->>Test: verify frontmatter/content as expected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 6
🧹 Nitpick comments (2)
test/test-installation-components.js (1)
576-617: Move the temp-directory cleanup tofinally.Any exception after these directories are created leaks them, because the three
fs.remove()calls only run on the happy path.One way to make the cleanup reliable
- try { + let tempCodexProjectDir; + let tempGeminiProjectDir; + let installedBmadDir; + try { clearCache(); const platformCodes = await loadPlatformCodes(); const codexInstaller = platformCodes.platforms.codex?.installer; const geminiInstaller = platformCodes.platforms.gemini?.installer; @@ - const tempCodexProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-codex-prototype-test-')); - const tempGeminiProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-gemini-prototype-test-')); - const installedBmadDir = await createShardDocPrototypeFixture(); + tempCodexProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-codex-prototype-test-')); + tempGeminiProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-gemini-prototype-test-')); + installedBmadDir = await createShardDocPrototypeFixture(); @@ - await fs.remove(tempCodexProjectDir); - await fs.remove(tempGeminiProjectDir); - await fs.remove(installedBmadDir); } catch (error) { assert(false, 'Shard-doc prototype duplication test succeeds', error.message); + } finally { + await Promise.allSettled( + [tempCodexProjectDir, tempGeminiProjectDir, installedBmadDir] + .filter(Boolean) + .map((dir) => fs.remove(dir)), + ); }🤖 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 576 - 617, The temp directories (tempCodexProjectDir, tempGeminiProjectDir, installedBmadDir) are only removed on the happy path; move the three await fs.remove(...) calls into a finally block so they always run; declare those variables in the outer scope before the try so the finally can reference them, and in the finally guard each removal (e.g., if (variable) await fs.remove(variable)) to avoid errors when creation failed; update the test block containing IdeManager.setup and assertions accordingly so cleanup is performed regardless of success or failure.tools/cli/installers/lib/ide/_config-driven.js (1)
520-525: Do not reread the same manifest for every artifact.Skill-format installs now add one
readFile+yaml.parseper artifact even when many artifacts share a directory. Cache bydirPathfor the duration of a setup run.🤖 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 520 - 525, getPrototypeSkillIdsForArtifact currently calls loadSkillManifest for every artifact even when multiple artifacts share the same source dir; add a short-lived in-memory cache keyed by sourceRef.dirPath (e.g., this._manifestCache or similar) and change getPrototypeSkillIdsForArtifact to first resolveArtifactSourceRef, then check the cache for sourceRef.dirPath, call loadSkillManifest only if missing, store the parsed manifest in the cache, and finally call getPrototypeIds(manifest, sourceRef.filename); ensure the cache is scoped to the installer/run (instance property) so manifests are reused for the duration of the setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/native-skills-lean-shard-doc-prototype/phase-2-prd.md`:
- Around line 1-4: Add the required YAML frontmatter block (--- title,
description, sidebar.order, ---) before the H1 heading in each phase doc (e.g.,
phase-1-analysis.md, phase-2-prd.md, phase-3-design.md,
phase-4-implementation.md) so the files conform to the Astro/Starlight
collection schema; specifically, insert a frontmatter block like `---\ntitle:
"Phase X: ..."\ndescription: "Brief desc"\nsidebar:\n order: N\n---`
immediately above the existing `# Phase ...` H1 heading in each file.
In `@docs/native-skills-lean-shard-doc-prototype/phase-3-architecture.md`:
- Around line 1-4: This doc (starting with the heading "Phase 3 Architecture:
Native Skills Lean PoC") and the other phase files (phase-1, phase-2, phase-4)
are missing required YAML frontmatter; add a top-of-file YAML block containing
at least title and description fields (e.g. title: "Phase 3 Architecture: Native
Skills Lean PoC" and a short description summarizing the doc) so the file
follows the docs/ frontmatter convention, ensuring the frontmatter sits before
any markdown content and is present in each of the four phase files.
In `@docs/native-skills-lean-shard-doc-prototype/phase-4-implementation.md`:
- Around line 20-31: Update the Verification Plan to include the docs
content-validation step that CI enforces: in addition to running node
test/test-installation-components.js, add running the repository’s
docs/content-validation job (the CI "content-validation" step) and verify that
it passes; keep the existing checks that no edits were made to legacy
shard-doc.xml and src/core/module-help.csv and state that the PR must pass the
docs validation before marking Done Criteria complete.
- Around line 1-4: The Markdown files for "Phase 4 Implementation: Native Skills
Lean PoC" and the corresponding Phase 1 file are missing the required YAML
frontmatter that Astro Starlight's docsSchema() expects; add a YAML frontmatter
block at the very top of each file containing at minimum a title field (e.g.,
title matching the document heading) so the schema validates.
In `@test/test-installation-components.js`:
- Around line 93-101: The fixture manifest written to bmad-skill-manifest.yaml
is missing the shard-doc.xml fields present in production; update the
shard-doc.xml entry in the test fixture to include the same type and description
keys as the real manifest (restore the missing "type" and "description"
properties for the shard-doc.xml entry so the test shape matches production),
editing the write in test-installation-components.js that creates
bmad-skill-manifest.yaml to include those fields and values used in the real
src/core/tasks/bmad-skill-manifest.yaml.
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 561-562: Remove the now-dead eslint suppression that precedes the
regex replace on the normalized variable; specifically delete the comment "//
eslint-disable-next-line unicorn/prefer-string-replace-all -- regex replacement
is intentional" above the line "normalized = normalized.replace(/^\/+/, '');" so
the linter can report appropriately and the code remains unchanged.
---
Nitpick comments:
In `@test/test-installation-components.js`:
- Around line 576-617: The temp directories (tempCodexProjectDir,
tempGeminiProjectDir, installedBmadDir) are only removed on the happy path; move
the three await fs.remove(...) calls into a finally block so they always run;
declare those variables in the outer scope before the try so the finally can
reference them, and in the finally guard each removal (e.g., if (variable) await
fs.remove(variable)) to avoid errors when creation failed; update the test block
containing IdeManager.setup and assertions accordingly so cleanup is performed
regardless of success or failure.
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 520-525: getPrototypeSkillIdsForArtifact currently calls
loadSkillManifest for every artifact even when multiple artifacts share the same
source dir; add a short-lived in-memory cache keyed by sourceRef.dirPath (e.g.,
this._manifestCache or similar) and change getPrototypeSkillIdsForArtifact to
first resolveArtifactSourceRef, then check the cache for sourceRef.dirPath, call
loadSkillManifest only if missing, store the parsed manifest in the cache, and
finally call getPrototypeIds(manifest, sourceRef.filename); ensure the cache is
scoped to the installer/run (instance property) so manifests are reused for the
duration of the setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ede490f-b11f-4753-9ebd-3dbe93d7f1eb
📒 Files selected for processing (8)
docs/native-skills-lean-shard-doc-prototype/phase-1-analysis.mddocs/native-skills-lean-shard-doc-prototype/phase-2-prd.mddocs/native-skills-lean-shard-doc-prototype/phase-3-architecture.mddocs/native-skills-lean-shard-doc-prototype/phase-4-implementation.mdsrc/core/tasks/bmad-skill-manifest.yamltest/test-installation-components.jstools/cli/installers/lib/ide/_config-driven.jstools/cli/installers/lib/ide/shared/skill-manifest.js
docs/native-skills-lean-shard-doc-prototype/phase-3-architecture.md
Outdated
Show resolved
Hide resolved
docs/native-skills-lean-shard-doc-prototype/phase-4-implementation.md
Outdated
Show resolved
Hide resolved
docs/native-skills-lean-shard-doc-prototype/phase-4-implementation.md
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Applied a scope-tightening update and resolved all open review threads.
PR remains in draft for manual review. |
Contributor
Author
|
Thread-alignment update pushed:
Validation: node test/test-installation-components.js passed. |
alexeyv
requested changes
Mar 7, 2026
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
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/ide/shared/skill-manifest.js (1)
12-24:⚠️ Potential issue | 🟠 MajorBroken manifests still degrade to “no manifest”.
Returning
nullon parse failure makes a malformed skill manifest indistinguishable from an absent one. In this PR that means prototype installs can silently disappear while setup still reports success. That should fail loudly, or at least propagate a structured warning the caller can treat as an installation problem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/shared/skill-manifest.js` around lines 12 - 24, loadSkillManifest currently swallows parse errors and returns null, making malformed manifests indistinguishable from missing ones; change it so parsing failures (and cases where parsed is falsy/not an object) return a structured error object instead of null (for example { __invalidManifest: { message: error.message, path: manifestPath } }) and likewise replace the catch that logs and returns null with a return of that structured object; keep the existing success shapes (returning { __single: parsed } or parsed) so callers can detect and handle the { __invalidManifest } sentinel and treat it as an install failure rather than a missing manifest.
🧹 Nitpick comments (1)
tools/cli/installers/lib/ide/_config-driven.js (1)
523-528: This hot path re-parses the same manifest over and over.Every artifact write does a fresh
loadSkillManifest(sourceRef.dirPath). On a real install that is repeated YAML I/O for the same directory, exactly where the new prototype logic sits in the inner write loop. Memoize bydirPathfor the lifetime ofsetup()instead of rereading it on every artifact.🤖 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 523 - 528, The getPrototypeSkillIdsForArtifact function is repeatedly calling loadSkillManifest(sourceRef.dirPath) for the same dirPath, causing duplicate YAML I/O; add a short-lived cache keyed by dirPath that lives for the duration of setup() and use it inside getPrototypeSkillIdsForArtifact (and any sibling callers) to return a cached manifest if present instead of reloading; reference the functions getPrototypeSkillIdsForArtifact, resolveArtifactSourceRef, and loadSkillManifest and ensure the cache is populated on first load and read thereafter, scoped to the installer/setup lifecycle so it resets after setup completes.
🤖 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 887-949: The test never verifies the non-skill_format (flat-file)
path, so add an assertion that the flat-file installer is not skill_format and
run the same fixture through IdeManager.setup for the flat-file platform (use
the same createShardDocPrototypeFixture and ideManager.setup calls), then assert
the canonical SKILL.md exists but the prototype duplicate does NOT (i.e., verify
only the canonical file is written for the flat-file install); reference
platformCodes.platforms.flatfile?.installer, ideManager.setup('flatfile', ...),
createShardDocPrototypeFixture, and the canonical/prototype SKILL.md path checks
to implement this additional negative test.
- Around line 106-136: Add a prototype sidecar manifest next to the SKILL.md
file so the fixture mirrors production sidecar discovery: create a manifest file
inside the 'core/tasks/bmad-shard-doc-skill-prototype' directory (using the same
path construction as the SKILL.md write: path.join(fixtureDir, 'core', 'tasks',
'bmad-shard-doc-skill-prototype', ...)) and populate it with the minimal fields
the sidecar parser expects (e.g., name/canonicalId/displayName or the same keys
used in '_config/task-manifest.csv') so tests exercise sidecar discovery for the
prototype directory the same way production does.
- Around line 917-944: The current tests only assert file existence; enhance
assertions to verify content contracts: read codexCanonicalSkill and
codexPrototypeSkill (codexCanonicalContent, codexPrototypeContent) and assert
codexCanonicalContent contains the canonical shard-doc reference (e.g.,
"shard-doc.xml") and does NOT contain the prototype marker or prototype
frontmatter, while codexPrototypeContent includes the exact "Prototype marker:
source-authored-skill" and the prototype frontmatter name "name:
bmad-shard-doc-skill-prototype"; after ideManager.setup('gemini', ...) also read
geminiCanonicalSkill and geminiPrototypeSkill and assert geminiCanonicalContent
references "shard-doc.xml" and geminiPrototypeContent exactly matches the
source-authored prototype content (or equals codexPrototypeContent) to ensure
Gemini preserved the prototype without rewriting it.
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 511-513: The prototype-copy path currently writes the source
SKILL.md without enforcing the name/frontmatter invariant; update the code that
calls readPrototypeSourceSkillContent(...) and writeFile(...) to parse the
returned SKILL.md frontmatter, verify that the frontmatter.name equals the
destination prototypeId (after normalizing with transformToSkillFormat if
appropriate), and throw or return a descriptive error instead of writing if they
disagree; apply the same validation for the other prototype-copy site (the
second read/write block mentioned) so no prototype install can produce a
SKILL.md whose name mismatches the destination id.
- Around line 504-513: The install currently skips prototypes when
readPrototypeSourceSkillContent(sourceRef, prototypeId) returns falsy, leading
to silent partial installs; change the logic in the loop inside the function
that calls resolveArtifactSourceRef and getPrototypeSkillIdsForArtifact so that
if readPrototypeSourceSkillContent(...) returns null/undefined or the SKILL.md
is unreadable, you surface an installation error (throw or return an
install-failure) referencing the specific prototypeId and sourceRef instead of
silently continuing; update the same guard used around the second occurrence
(the block around lines 538-542) so both places fail fast with a clear error
tied to prototypeId/SKILL.md rather than skipping.
- Around line 504-513: The code uses manifest-derived prototypeId directly as a
filesystem name and path (in
resolveArtifactSourceRef/getPrototypeSkillIdsForArtifact loop) which can allow
traversal or invalid names; validate and normalize prototypeId to a single safe
basename before using it for targetPath, ensureDir,
readPrototypeSourceSkillContent and writeFile (same validation/normalization
used for canonical artifacts), reject or skip any prototypeId that is empty,
contains path separators, dot-segments, or platform-invalid characters, and only
proceed with the sanitized basename when creating prototypeDir and
reading/writing SKILL.md.
- Around line 551-575: The containment check in resolveArtifactSourceRef is
insufficient for embedded traversal; after computing relativeDir/dirPath,
resolve the final directory to an absolute path (e.g. via path.resolve with
bmadDir and relativeDir) and normalize it, then verify the resolvedDir is
contained in the resolved bmadDir (compare with path.resolve(bmadDir) and ensure
the resolvedDir starts with that + path.sep or is equal); if it is outside,
return null. Keep existing filename checks but perform this secure containment
check before returning { dirPath, filename } and use the resolved absolute
dirPath for any subsequent file operations.
In `@tools/cli/installers/lib/ide/shared/skill-manifest.js`:
- Around line 60-72: resolveManifestEntry is too permissive and task-specific:
it falls back from .md to .xml and to .agent.yaml causing unrelated artifacts
(e.g., help.md matching help.xml); update resolveManifestEntry so it only
returns exact filename matches (and the __single override), and remove the
hardcoded alternate-extension fallbacks; if shared fallback behavior is required
for tasks only, add a boolean flag or a separate function (e.g.,
resolveManifestEntryForTasks) and call that from getTasksFromDir so other
callers keep strict matching; ensure references in getTasksFromDir use the
task-specific resolver and other modules continue to call the strict
resolveManifestEntry.
- Around line 80-91: normalizeIdList currently drops malformed prototypeIds
silently; change it to validate and warn instead of silently filtering: update
normalizeIdList to accept an optional logger (or import the module logger) and,
before returning, detect the malformed shapes—if ids is neither string nor
array, call logger.warn with the field name "prototypeIds" and the provided
value; if ids is an array and contains non-string entries or items that trim to
empty, call logger.warn listing the offending items and their indexes; keep the
existing dedupe/trim behavior for valid entries but ensure the warnings include
enough context so downstream installers know prototypeIds was malformed.
---
Outside diff comments:
In `@tools/cli/installers/lib/ide/shared/skill-manifest.js`:
- Around line 12-24: loadSkillManifest currently swallows parse errors and
returns null, making malformed manifests indistinguishable from missing ones;
change it so parsing failures (and cases where parsed is falsy/not an object)
return a structured error object instead of null (for example {
__invalidManifest: { message: error.message, path: manifestPath } }) and
likewise replace the catch that logs and returns null with a return of that
structured object; keep the existing success shapes (returning { __single:
parsed } or parsed) so callers can detect and handle the { __invalidManifest }
sentinel and treat it as an install failure rather than a missing manifest.
---
Nitpick comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 523-528: The getPrototypeSkillIdsForArtifact function is
repeatedly calling loadSkillManifest(sourceRef.dirPath) for the same dirPath,
causing duplicate YAML I/O; add a short-lived cache keyed by dirPath that lives
for the duration of setup() and use it inside getPrototypeSkillIdsForArtifact
(and any sibling callers) to return a cached manifest if present instead of
reloading; reference the functions getPrototypeSkillIdsForArtifact,
resolveArtifactSourceRef, and loadSkillManifest and ensure the cache is
populated on first load and read thereafter, scoped to the installer/setup
lifecycle so it resets after setup completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24f667a2-5df0-41d1-a489-41e442589887
📒 Files selected for processing (5)
src/core/tasks/bmad-shard-doc-skill-prototype/SKILL.mdsrc/core/tasks/bmad-shard-doc-skill-prototype/bmad-skill-manifest.yamltest/test-installation-components.jstools/cli/installers/lib/ide/_config-driven.jstools/cli/installers/lib/ide/shared/skill-manifest.js
✅ Files skipped from review due to trivial changes (2)
- src/core/tasks/bmad-shard-doc-skill-prototype/SKILL.md
- src/core/tasks/bmad-shard-doc-skill-prototype/bmad-skill-manifest.yaml
dickymoore
force-pushed
the
feature/native-skills-lean-shard-doc-prototype
branch
from
07d7322 to
3755e72
Compare
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| try { | ||
| clearCache(); | ||
| const platformCodes = await loadPlatformCodes(); | ||
| const skillFormatEntry = Object.entries(platformCodes.platforms || {}).find(([_, platform]) => { |
There was a problem hiding this comment.
The test picks the first skill_format platform it finds, but task templates differ across platforms (e.g. default-task.md vs kiro-task.md/opencode-task.md), so the hard-coded string assertions below can become order-dependent/flaky. Consider selecting a specific known platform code for this test (or relaxing assertions to match the chosen platform’s template contract).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
test/test-installation-components.js (3)
958-972:⚠️ Potential issue | 🟠 MajorThis negative-path test is not representative of production.
You mutate a real skill-format installer into
skill_format: falseand instantiateConfigDrivenIdeSetupdirectly, bypassing the same handler construction path production uses (IdeManager.loadConfigDrivenHandlers()plussetBmadFolderName()). A pass here does not prove existing flat-file behavior stayed intact. Use a real flat-file platform throughIdeManager, or initialize the synthetic handler exactly the same way the manager does.🤖 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 958 - 972, The test mutates a real skill-format installer into non-skill mode and constructs ConfigDrivenIdeSetup directly, which bypasses the production construction path; instead obtain a real flat-file platform handler via IdeManager.loadConfigDrivenHandlers() and configure it with setBmadFolderName() (or replicate the exact initialization sequence IdeManager uses) so the test uses the same handler creation as production; replace the direct new ConfigDrivenIdeSetup(...) and mutated nonSkillInstaller usage with loading the handler from IdeManager and selecting the matching flat-file/platform entry before calling setup (keep references to nonSkillInstaller, nonSkillHandler, ConfigDrivenIdeSetup, IdeManager.loadConfigDrivenHandlers, and setBmadFolderName to guide the change).
976-981:⚠️ Potential issue | 🟠 MajorThe flat-file assertions are still too shallow to catch the real regression.
Checking directory entries only proves that something named
bmad-shard-docwas written. It would still pass if the canonical flat file accidentally contained the prototype wrapper content. Read the canonical output and assert it still targetsshard-doc.xmland does not includePrototype marker: source-authored-skill.🤖 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 976 - 981, The current test only checks filenames; extend it to open the canonical artifact file found in nonSkillTargetDir (the entry matched by hasCanonical / /^bmad-shard-doc(\.|$)/), read its contents and assert that the canonical output targets "shard-doc.xml" (e.g., contains the expected target string) and that it does NOT include the prototype marker "Prototype marker: source-authored-skill"; keep the existing hasPrototype assertion but add these content assertions after locating the canonical file using nonSkillEntries/nonSkillTargetDir.
905-930:⚠️ Potential issue | 🟠 MajorOne skill-format platform is not enough coverage for this feature.
The change is supposed to hold across skill-format installers, but this only exercises one randomly selected platform. That leaves whole template families uncovered, especially Gemini’s non-default path. Run the scenario against at least one Markdown-based platform and Gemini so a regression in either family is caught.
🤖 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 905 - 930, The test only exercises one randomly selected skill-format platform (skillFormatEntry/skillFormatPlatformCode) — extend it to run the same setup/assertion against two additional platforms: one Markdown-based platform and the Gemini platform. Concretely, collect all matching skill-format platforms (replace skillFormatEntry with skillFormatPlatforms array), locate a Markdown template platform by checking installer fields that indicate Markdown templating (e.g., installer.template_engine/template_type containing "md" or "markdown" or platform code including "markdown"/"md"), and locate the Gemini platform by platform code "gemini" or installer name containing "Gemini"; for each found platform call ideManager.setup (same options as used for skillFormatPlatformCode) and assert success (similar to skillResult assertion). Ensure the new variables (e.g., markdownPlatformCode, geminiPlatformCode and their results) reuse the same temp dirs and installedBmadDir setup, and skip/test-guard if either Markdown or Gemini platform is not present.tools/cli/installers/lib/ide/_config-driven.js (2)
529-534:⚠️ Potential issue | 🟠 MajorValidate the copied prototype frontmatter before writing it.
This path copies the source
SKILL.mdverbatim without checking that itsname:still matchesbmad-shard-doc-skill-prototype. If that source file drifts, you install a directory whose metadata points at a different skill id, which strict skill platforms tend to reject.🤖 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 529 - 534, Before writing SKILL.md, parse and validate the frontmatter in sourceSkillContent to ensure the "name" field matches prototypeId (the expected prototype skill id); if the parsed frontmatter name is missing or not equal to prototypeId, abort (throw or log and return) instead of writing the file. Use the existing variables sourceSkillContent, prototypeId, prototypeDir and the SKILL.md write path to locate the check, and perform the validation between reading the file and calling this.writeFile to avoid installing mismatched prototype metadata.
527-530:⚠️ Potential issue | 🟠 MajorMissing or empty prototype source still reports a successful install.
Returning on
pathExists === falseor empty content turns a broken source tree into a partial install with no signal beyond a missing duplicate skill. This should fail fast, or at minimum emit a warning tied tobmad-shard-doc-skill-prototype.🤖 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 527 - 530, The installer currently silently returns when a prototype skill file is missing or empty (checks on sourceSkillPath and sourceSkillContent), which leads to a partial/hidden success; update the logic in the code handling sourceSkillPath/sourceSkillContent so that if fs.pathExists(sourceSkillPath) is false or the file read yields empty content you either (a) throw an Error to fail fast or (b) emit a clear error/warning tied to the bmad-shard-doc-skill-prototype identifier (use the existing logging mechanism used elsewhere in this module) and abort the install flow rather than returning silently; locate the checks around sourceSkillPath and sourceSkillContent to implement this change.
🧹 Nitpick comments (1)
tools/cli/installers/lib/ide/_config-driven.js (1)
269-297: Keep the shard-doc transition out of the generic skill writer.Plumbing
bmadDirthroughwriteTaskToolArtifacts()intowriteSkillFile()makes the shared “write one skill” primitive carry a task-specific filesystem side effect. That coupling will get worse the moment a second prototype is added. Trigger the duplicate install from the task branch, or drive it from artifact metadata before callingwriteSkillFile().Also applies to: 492-513
🤖 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 269 - 297, The writeTaskToolArtifacts function is passing bmadDir into the generic writeSkillFile, coupling a task-specific duplicate-install side effect into the shared skill writer; remove plumbing of bmadDir into writeSkillFile and instead handle the shard/duplicate install from the task branch (inside writeTaskToolArtifacts) before calling writeSkillFile when artifact.type === 'task' (or when artifact metadata indicates a duplicate-install is required), or drive that duplicate write from artifact.metadata; update calls to writeSkillFile to omit bmadDir and perform any extra filesystem writes (the bmadDir copy) explicitly in writeTaskToolArtifacts (and the analogous spot at lines ~492-513) so the generic writeSkillFile remains task-agnostic.
🤖 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 94-103: Update the test fixture that writes
bmad-skill-manifest.yaml in test/test-installation-components.js to include the
production prototype linkage: inside the array passed to fs.writeFile for the
manifest (the entry with 'shard-doc.xml:' and 'canonicalId: bmad-shard-doc') add
a 'prototypeIds:' YAML block with the same prototype id(s) used in production
(e.g. a nested list line like ' prototypeIds:' followed by ' -
<production-prototype-id>'). This ensures the installer test exercises the
prototype relationship rather than relying on a hardcoded path.
- Around line 905-944: The test is fragile because it picks the first
skill_format platform via skillFormatEntry and asserts an exact template
sentence; change it to either (A) pin explicit platform codes (use a small
allowed list and find the first matching code, e.g. check
skillFormatPlatformCode against that list before running the rest) or (B) relax
the content assertions: keep selecting a valid skill installer via
skillFormatEntry/skillInstaller but replace the exact sentence checks on
canonicalSkillContent/prototypeSkillContent with stable checks such as the
presence of "shard-doc.xml" reference and a distinct prototype marker (e.g.
presence/absence of a known prototype filename or frontmatter flag) rather than
the full written sentence; update the assertions that reference
canonicalSkillContent and prototypeSkillContent accordingly.
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 512-526: The writeShardDocPrototypeSkill function currently
hardcodes prototypeId and sourceSkillPath, ignoring manifest-declared
prototypeIds and sidecar loading; change it to read the artifact/manifest for
the given skill (use the same manifest/sidecar loading code used elsewhere),
fetch prototypeIds from that manifest, and iterate over those prototypeIds to
copy each prototype SKILL.md from bmadDir (or skip if not present) instead of
always using the literal 'bmad-shard-doc-skill-prototype'; ensure you update
references to prototypeId and sourceSkillPath in this function to use the
manifest-driven values.
---
Duplicate comments:
In `@test/test-installation-components.js`:
- Around line 958-972: The test mutates a real skill-format installer into
non-skill mode and constructs ConfigDrivenIdeSetup directly, which bypasses the
production construction path; instead obtain a real flat-file platform handler
via IdeManager.loadConfigDrivenHandlers() and configure it with
setBmadFolderName() (or replicate the exact initialization sequence IdeManager
uses) so the test uses the same handler creation as production; replace the
direct new ConfigDrivenIdeSetup(...) and mutated nonSkillInstaller usage with
loading the handler from IdeManager and selecting the matching
flat-file/platform entry before calling setup (keep references to
nonSkillInstaller, nonSkillHandler, ConfigDrivenIdeSetup,
IdeManager.loadConfigDrivenHandlers, and setBmadFolderName to guide the change).
- Around line 976-981: The current test only checks filenames; extend it to open
the canonical artifact file found in nonSkillTargetDir (the entry matched by
hasCanonical / /^bmad-shard-doc(\.|$)/), read its contents and assert that the
canonical output targets "shard-doc.xml" (e.g., contains the expected target
string) and that it does NOT include the prototype marker "Prototype marker:
source-authored-skill"; keep the existing hasPrototype assertion but add these
content assertions after locating the canonical file using
nonSkillEntries/nonSkillTargetDir.
- Around line 905-930: The test only exercises one randomly selected
skill-format platform (skillFormatEntry/skillFormatPlatformCode) — extend it to
run the same setup/assertion against two additional platforms: one
Markdown-based platform and the Gemini platform. Concretely, collect all
matching skill-format platforms (replace skillFormatEntry with
skillFormatPlatforms array), locate a Markdown template platform by checking
installer fields that indicate Markdown templating (e.g.,
installer.template_engine/template_type containing "md" or "markdown" or
platform code including "markdown"/"md"), and locate the Gemini platform by
platform code "gemini" or installer name containing "Gemini"; for each found
platform call ideManager.setup (same options as used for
skillFormatPlatformCode) and assert success (similar to skillResult assertion).
Ensure the new variables (e.g., markdownPlatformCode, geminiPlatformCode and
their results) reuse the same temp dirs and installedBmadDir setup, and
skip/test-guard if either Markdown or Gemini platform is not present.
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 529-534: Before writing SKILL.md, parse and validate the
frontmatter in sourceSkillContent to ensure the "name" field matches prototypeId
(the expected prototype skill id); if the parsed frontmatter name is missing or
not equal to prototypeId, abort (throw or log and return) instead of writing the
file. Use the existing variables sourceSkillContent, prototypeId, prototypeDir
and the SKILL.md write path to locate the check, and perform the validation
between reading the file and calling this.writeFile to avoid installing
mismatched prototype metadata.
- Around line 527-530: The installer currently silently returns when a prototype
skill file is missing or empty (checks on sourceSkillPath and
sourceSkillContent), which leads to a partial/hidden success; update the logic
in the code handling sourceSkillPath/sourceSkillContent so that if
fs.pathExists(sourceSkillPath) is false or the file read yields empty content
you either (a) throw an Error to fail fast or (b) emit a clear error/warning
tied to the bmad-shard-doc-skill-prototype identifier (use the existing logging
mechanism used elsewhere in this module) and abort the install flow rather than
returning silently; locate the checks around sourceSkillPath and
sourceSkillContent to implement this change.
---
Nitpick comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 269-297: The writeTaskToolArtifacts function is passing bmadDir
into the generic writeSkillFile, coupling a task-specific duplicate-install side
effect into the shared skill writer; remove plumbing of bmadDir into
writeSkillFile and instead handle the shard/duplicate install from the task
branch (inside writeTaskToolArtifacts) before calling writeSkillFile when
artifact.type === 'task' (or when artifact metadata indicates a
duplicate-install is required), or drive that duplicate write from
artifact.metadata; update calls to writeSkillFile to omit bmadDir and perform
any extra filesystem writes (the bmadDir copy) explicitly in
writeTaskToolArtifacts (and the analogous spot at lines ~492-513) so the generic
writeSkillFile remains task-agnostic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5af717a-3eb4-4c59-9d6c-542084b25623
📒 Files selected for processing (4)
src/core/tasks/bmad-shard-doc-skill-prototype/SKILL.mdsrc/core/tasks/bmad-shard-doc-skill-prototype/bmad-skill-manifest.yamltest/test-installation-components.jstools/cli/installers/lib/ide/_config-driven.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/tasks/bmad-shard-doc-skill-prototype/SKILL.md
- src/core/tasks/bmad-shard-doc-skill-prototype/bmad-skill-manifest.yaml
Comment on lines
+94
to
+103
| await fs.writeFile( | ||
| path.join(fixtureDir, 'core', 'tasks', 'bmad-skill-manifest.yaml'), | ||
| [ | ||
| 'shard-doc.xml:', | ||
| ' canonicalId: bmad-shard-doc', | ||
| ' type: task', | ||
| ' description: "Splits large markdown documents into smaller, organized files based on sections"', | ||
| '', | ||
| ].join('\n'), | ||
| ); |
There was a problem hiding this comment.
The fixture still omits the new prototypeIds linkage.
Right now this test passes even if the installer ignores manifest-declared prototype relationships entirely, because the shard-doc fixture only declares canonicalId, type, and description. Add the prototypeIds entry that production uses, otherwise this scenario is proving the hardcoded path, not the new contract.
🤖 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 94 - 103, Update the test
fixture that writes bmad-skill-manifest.yaml in
test/test-installation-components.js to include the production prototype
linkage: inside the array passed to fs.writeFile for the manifest (the entry
with 'shard-doc.xml:' and 'canonicalId: bmad-shard-doc') add a 'prototypeIds:'
YAML block with the same prototype id(s) used in production (e.g. a nested list
line like ' prototypeIds:' followed by ' - <production-prototype-id>'). This
ensures the installer test exercises the prototype relationship rather than
relying on a hardcoded path.
Comment on lines
+905
to
+944
| const skillFormatEntry = Object.entries(platformCodes.platforms || {}).find(([_, platform]) => { | ||
| const installer = platform?.installer; | ||
| if (!installer || installer.skill_format !== true || typeof installer.target_dir !== 'string') return false; | ||
| if (Array.isArray(installer.artifact_types) && !installer.artifact_types.includes('tasks')) return false; | ||
| return true; | ||
| }); | ||
|
|
||
| assert(Boolean(skillFormatEntry), 'Found a skill_format platform that installs task artifacts'); | ||
| if (!skillFormatEntry) throw new Error('No suitable skill_format platform found for shard-doc prototype test'); | ||
|
|
||
| const [skillFormatPlatformCode, skillFormatPlatform] = skillFormatEntry; | ||
| const skillInstaller = skillFormatPlatform.installer; | ||
|
|
||
| tempSkillProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-skill-prototype-test-')); | ||
| tempNonSkillProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-nonskill-prototype-test-')); | ||
| installedBmadDir = await createShardDocPrototypeFixture(); | ||
|
|
||
| const ideManager = new IdeManager(); | ||
| await ideManager.ensureInitialized(); | ||
|
|
||
| const skillResult = await ideManager.setup(skillFormatPlatformCode, tempSkillProjectDir, installedBmadDir, { | ||
| silent: true, | ||
| selectedModules: ['bmm'], | ||
| }); | ||
|
|
||
| assert(skillResult.success === true, `${skillFormatPlatformCode} setup succeeds for shard-doc prototype fixture`); | ||
|
|
||
| const canonicalSkillPath = path.join(tempSkillProjectDir, skillInstaller.target_dir, 'bmad-shard-doc', 'SKILL.md'); | ||
| const prototypeSkillPath = path.join(tempSkillProjectDir, skillInstaller.target_dir, 'bmad-shard-doc-skill-prototype', 'SKILL.md'); | ||
| assert(await fs.pathExists(canonicalSkillPath), `${skillFormatPlatformCode} install writes canonical shard-doc skill`); | ||
| assert(await fs.pathExists(prototypeSkillPath), `${skillFormatPlatformCode} install writes duplicated shard-doc prototype skill`); | ||
|
|
||
| const canonicalSkillContent = await fs.readFile(canonicalSkillPath, 'utf8'); | ||
| const prototypeSkillContent = await fs.readFile(prototypeSkillPath, 'utf8'); | ||
|
|
||
| assert(canonicalSkillContent.includes('name: bmad-shard-doc'), 'Canonical shard-doc skill keeps canonical frontmatter name'); | ||
| assert( | ||
| canonicalSkillContent.includes('Read the entire task file at: {project-root}/_bmad/core/tasks/shard-doc.xml'), | ||
| 'Canonical shard-doc skill points to shard-doc.xml', | ||
| ); |
There was a problem hiding this comment.
This test is order-dependent and template-dependent.
It picks the first skill_format platform returned by Object.entries() and then asserts a very specific task-template sentence. Both are fragile: platform order can change, and not every valid skill-format template is going to use that exact wording. Pin explicit platforms or assert only the stable contract, e.g. a shard-doc.xml reference plus the absence/presence of the prototype marker.
🤖 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 905 - 944, The test is
fragile because it picks the first skill_format platform via skillFormatEntry
and asserts an exact template sentence; change it to either (A) pin explicit
platform codes (use a small allowed list and find the first matching code, e.g.
check skillFormatPlatformCode against that list before running the rest) or (B)
relax the content assertions: keep selecting a valid skill installer via
skillFormatEntry/skillInstaller but replace the exact sentence checks on
canonicalSkillContent/prototypeSkillContent with stable checks such as the
presence of "shard-doc.xml" reference and a distinct prototype marker (e.g.
presence/absence of a known prototype filename or frontmatter flag) rather than
the full written sentence; update the assertions that reference
canonicalSkillContent and prototypeSkillContent accordingly.
Comment on lines
+512
to
+526
| await this.writeShardDocPrototypeSkill(targetPath, bmadDir, skillName); | ||
| } | ||
|
|
||
| /** | ||
| * Copy shard-doc prototype skill during transition when installing skill-format targets. | ||
| * Keeps scope literal for the first PoC without introducing generalized prototype linkage. | ||
| * @param {string} targetPath - Base skills directory | ||
| * @param {string|null} bmadDir - Installed bmad directory | ||
| * @param {string} skillName - Canonical skill name being written | ||
| */ | ||
| async writeShardDocPrototypeSkill(targetPath, bmadDir, skillName) { | ||
| if (!bmadDir || skillName !== 'bmad-shard-doc') return; | ||
|
|
||
| const prototypeId = 'bmad-shard-doc-skill-prototype'; | ||
| const sourceSkillPath = path.join(bmadDir, 'core', 'tasks', prototypeId, 'SKILL.md'); |
There was a problem hiding this comment.
The new manifest linkage is ignored here.
writeShardDocPrototypeSkill() hardcodes both the canonical source skill and the prototype id, so the newly added prototypeIds manifest data and sidecar loading logic never participate in the install decision on this path. If the manifest changes, this code keeps copying whatever the literals say. Plumb the declared prototype ids onto the artifact and iterate those instead.
🤖 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 512 - 526, The
writeShardDocPrototypeSkill function currently hardcodes prototypeId and
sourceSkillPath, ignoring manifest-declared prototypeIds and sidecar loading;
change it to read the artifact/manifest for the given skill (use the same
manifest/sidecar loading code used elsewhere), fetch prototypeIds from that
manifest, and iterate over those prototypeIds to copy each prototype SKILL.md
from bmadDir (or skip if not present) instead of always using the literal
'bmad-shard-doc-skill-prototype'; ensure you update references to prototypeId
and sourceSkillPath in this function to use the manifest-driven values.
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
3 participants
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.
What
This PR delivers the first lean native-skills installer PoC requested in the Native Skills Transition thread.
It keeps the scope literal and narrow:
shard-doc.xmlin placesrc/core/tasks/bmad-shard-doc-skill-prototype/SKILL.mdskill_formattargets, install that prototype skill alongside the canonical installedbmad-shard-docskillWhy
The requested first step was a small, reviewable intermediary-state proof of concept:
shard-doc.xmlduring the transitionThis PR is intentionally low-abstraction. It is not proposing final architecture for native skills.
Scope
Included
src/core/tasks/bmad-shard-doc-skill-prototype/SKILL.mdsrc/core/tasks/bmad-shard-doc-skill-prototype/bmad-skill-manifest.yamltools/cli/installers/lib/ide/_config-driven.jsbmadDirthrough the skill-writing pathskill_formattargets, copiesbmad-shard-doc-skill-prototype/SKILL.mdfrom source into the platform skills directorytest/test-installation-components.jsskill_formatinstall writes both:bmad-shard-docbmad-shard-doc-skill-prototypeSKILL.mdExcluded
Notes
shard-doc.xml._config-driven.jsas a tradeoff to keep the first PoC narrow and reviewable.Validation
node test/test-installation-components.js✅Supersedes