feat(bmad-module): community/custom module manager skill + installer parity#2482
Open
pbean wants to merge 39 commits into
Open
feat(bmad-module): community/custom module manager skill + installer parity#2482pbean wants to merge 39 commits into
pbean wants to merge 39 commits into
Conversation
… removed when bmad itself is updated
…tall The skill is copied into _bmad/core/skills/bmad-module/ by the installer, which strips node_modules, ships no package.json under the skill, and never runs npm install there. Bare `import 'yaml'`/`import 'semver'` therefore crashed at module load (ERR_MODULE_NOT_FOUND, exit 1) before any structured exit code could fire. Every other installed script is zero-third-party-dep. - Vendor the real yaml@2.8.4 as a deterministic esbuild single-file bundle (scripts/lib/vendor/yaml.mjs), imported by relative path. Guarantees byte-identical manifest.yaml round-trips with BMAD core's writer, which uses the same library + options (tools/installer/core/manifest.js). - Drop semver for a node:-only semver-lite.mjs (valid/validRange), parity-tested against the real semver across 469 cases (400 fuzzed). - Fix a third bare yaml import that the original report missed (frontmatter.mjs). - Make bmad-module.mjs a zero-import launcher that maps any load failure to a new documented EXIT.TOOLING (5) with reinstall guidance instead of leaking a raw ESM stack trace; the verb dispatcher moves to cli.mjs. - Enforce vendor freshness so a yaml/esbuild bump can't ship a stale bundle: build-vendor.mjs --check is wired into npm test (pre-commit), npm run quality, and the quality.yaml CI validate job. Adds vendor:build / vendor:check scripts. - Ignore the generated vendor/ dir in eslint + prettier; document the rationale in SKILL.md, README.md, and vendor/README.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iting Replace the flat copy-list staging with a manifest-driven copy plan: - buildCopyPlan maps declared skills/agents/commands and string-typed Claude-Code surfaces into canonical install slots, copies conventional top-level metadata, and drops anything not covered (no more leaking of tools/, website/, .github/, etc.). - rewriteManifestPaths emits a plugin.json whose paths point at the canonical post-install locations, keeping the on-disk manifest self-consistent inside _bmad/<code>/. - stageCopyPlan stages the plan plus synthesized files (rewritten plugin.json) into the tmp dir for the atomic swap. install.mjs and update.mjs switch to the new plan/skillDestDirs flow and drop the now-unused copy-list helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The bmad-module skill installed but was absent from the core help
registry, so it never surfaced in the bmad-help menu (every other core
skill has a row). Add it as menu-code MM ("Manage Modules").
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The copy planner treated every skills/agents/commands entry as a directory and ran it through addDirRecursive, which lists files *under* the path. For a subagent declared as a file (e.g. `"agents": ["./agents/foo.md"]` — a standard Claude-Code shape) that listed nothing, so the agent was silently dropped from the install even though rewriteManifestPaths already remapped it to `./agents/foo.md`. Stat each entry and branch: directories recurse as before, files are queued directly (honoring the ignore matcher). Verified by the comprehensive fixture's changelog-archivist.md agent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The suite resolved its reference modules from `<repo>/examples`, which existed only in the sibling bmad-marketplace checkout — so after the skill moved into BMAD-METHOD core every install/list/remove assertion failed with "local source not a directory". Vendor the two reference modules (acme-md-lint, acme-devlog) under tests/fixtures/examples/ and point EXAMPLES there; drop the now-unused REPO_DIR. Also correct the comprehensive-install assertion: hooks are flattened to the canonical root slot (hooks.json), matching .mcp.json and rewriteManifestPaths — not a hooks/ subdir. Suite now passes 41/41. Note: these fixtures are copies of the bmad-marketplace examples and must be re-synced if those reference modules change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Formatting-only; no content changes. Brings both docs in line with the repo's prettier config (a leftover from the .js→.mjs migration). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-plugin # Conflicts: # .prettierignore # package.json
The bmad-module skill staged community modules under _bmad/<code>/ but never pushed their skills out to the coding assistants the user selected at `bmad install` time, so a freshly installed module was invisible to Claude Code / Cursor / Copilot / etc. until a full reinstall; remove left skills orphaned in the IDE dirs. install/update/remove now distribute (or prune) skills to every IDE listed in _bmad/_config/manifest.yaml and clean the redundant skill dirs from _bmad/, matching how official modules end up. Single engine, three callers — no fork: - New tools/installer/core/ide-sync.js (syncIdes) wraps the real IdeManager.setupBatch + platform-codes engine. The full installer (_setupIdes/_cleanupSkillDirs), the new `bmad ide-sync` command, and the skill all route through it, so new IDEs and engine changes propagate everywhere automatically. Local, dependency-free delivery — no npx/network at runtime: - build-ide-sync.mjs esbuild-bundles the engine into vendor/ide-sync.mjs (+ platform-codes.yaml), aliasing ../prompts and ../project-root to small shims so @Clack and the installer graph are dropped. The bundle ships inside the skill tree (like yaml.mjs); the skill execs it locally. It's generated-from-source and gated by vendor:check, refreshed on every install. update/remove pass --prune with the module's canonicalIds so skills dropped between versions (or on uninstall) are removed from IDE dirs + command pointers. Graceful degradation: if the bundle is unreachable, the verb still succeeds and points the user at `bmad ide-sync`. Tests: new test/test-ide-sync.js drift-guard (engine == bundle, incl. prune), integration.test.sh IDE-distribution section (offline), bundle self-check in the build. All gates green (vendor:check, lint, format, test:install 349/349). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ler CLI The bmad-module skill copied a module's files and distributed skills to IDEs, but skipped four post-copy steps the full `bmad install --custom-source` path performs, leaving modules incompletely installed: - Merge each module's module-help.csv into _bmad/_config/bmad-help.csv (the catalog bmad-help reads) — new lib/help-catalog.mjs - Generate [modules.<code>] / [agents.<code>] blocks in config.toml / config.user.toml from module.yaml (defaults + --set overrides), via a targeted merge that preserves [core] and sibling modules — new lib/config-gen.mjs - Create the working directories a module declares under `directories:` (with move-on-path-change and wds_folders) — new lib/module-dirs.mjs - Run `npm install --omit=dev` in place when a module ships package.json (opt out via bmad.install.skipNpm) — new lib/npm-deps.mjs All four run as a shared finishModuleInstall step wired into install, update, and remove; every step is non-fatal so a module already committed to _bmad/ isn't lost to a post-copy hiccup. Adds a repeatable --set <code>.<key>=<value> flag mirroring the installer. Also fixes two latent issues in the manifest-driven copy that the new steps depend on: - moduleDefinition / moduleHelpCsv are now flattened to the module root even when they live inside a declared skill dir (the setup-skill assets pattern); previously claimedSrc dedup skipped them and the rewritten plugin.json pointed at a non-existent ./module.yaml. - package.json / package-lock.json are now copied so npm deps can install. Tests: extends the integration suite with config/agent-roster, --set, directory-creation, help-catalog, removal-cleanup, and npm assertions (73/73 pass); adds a minimal-npm fixture and rewrites the comprehensive fixture's module-help.csv to the canonical schema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The skill validator walks all of src/ recursively, so the bmad-module skill's reference and negative example modules under tests/fixtures/ (intentionally non-bmad-* names and deliberately-malformed fixtures) were validated as production skills and tripped HIGH SKILL-04 name-format findings, failing `validate:skills --strict`. Exclude a `fixtures/` directory whose parent is `tests/` from discovery so the validator judges only real skills. No production skill lives under tests/fixtures/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Strategy 0 to PluginResolver: detect a .claude-plugin/plugin.json
carrying a bmad{} block at the module root and resolve+validate it via
the bmad-module skill's own libs (new bmad-module-lib bridge), so the
installer and runtime skill agree on what a module is. Every resolved
module now carries a format discriminator ('plugin-json' | 'legacy').
OfficialModules routes 'plugin-json' resolutions through a new
_installFromPluginJson path that reuses the skill's copy-plan/flatten/
rewrite/atomic-swap libs, producing an on-disk layout byte-identical to
a bmad-module install while leaving legacy installs untouched. Direct
mode in ui.js prefers a root plugin.json#bmad manifest over SKILL.md
scanning. Adds Test Suite 45 covering detection, legacy fall-through,
and end-to-end install.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Step 4 pointed at _bmad/core/skills/bmad-module/, but the new module system distributes skills verbatim into the selected IDE directories (.claude/skills/, .agents/skills/, etc.). Resolve the script relative to SKILL.md instead, and route the missing-script case to the existing exit-code-5 reinstall guidance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This work was developed in a separate bmad-marketplace repo and is now landing directly in BMAD-METHOD, so the migration framing is obsolete: - remove links to docs/spec.md (only existed in the temp repo) and the bmad-marketplace / 'upstream patch' / 'sibling checkout' references in README, the integration test, and the acme-md-lint fixture - drop dead 'spec §N' pointers in install.mjs, install-plan.mjs, and plugin-json.mjs (including a user-facing reserved-code error message) - reword the manifest-generator 'patch' note as in-repo behavior - correct the documented install path from _bmad/ to the IDE skills directories the installer now distributes skills to (.claude/skills/, etc.) No behavior change. Integration suite: 73 pass / 0 fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PluginResolver strategy 1 only checked the skills' common parent for module.yaml + module-help.csv. For the canonical BMad layout (module files at src/, skills under src/skills/<name>/) the common parent is src/skills, so the files were missed and the resolver fell through to strategy 5 — synthesizing a degenerate module named after the marketplace plugin (e.g. bmad-creative-intelligence-suite) and discarding the real `code` (cis) and `agents:` roster. That mismatch then made resolveInstalledModuleYaml fail, emitting the collectAgentsFromModuleYaml and writeCentralConfig "could not locate module.yaml" warnings. Strategy 1 now walks up from the skills' common parent to the repo root (bounded, deepest-first) and uses the first directory with both files, so src/module.yaml resolves correctly. Also match on `pluginName` in resolveInstalledModuleYaml's resolution-cache fallback so a module tracked under its marketplace plugin name still resolves. Adds a regression test mirroring the bmad-creative-intelligence-suite layout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the ancestor walk added in the previous commit finds more than one module.yaml + module-help.csv pair between the skills' common parent and the repo root, the deepest one was chosen silently. That could surprise a user whose repo legitimately carries module definitions at multiple levels. PluginResolver.resolve now accepts an optional chooseModuleDefinition callback. When >1 candidate is found it is invoked with each candidate enriched with its relativePath + module.yaml code/name/description, and its selection wins. Headless callers (the --custom-source CLI flow, tests, re-resolution lookups) omit the callback and keep the deterministic deepest-first default, so nothing blocks. The interactive custom-module flow supplies a prompt, pausing/resuming its spinner around the choice. Threads options through CustomModuleManager.resolvePlugin and wires the prompt into both resolve sites of _addCustomUrlModules. Adds tests for the deepest-first default, candidate enrichment, and chooser override. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Remove EXIT.FILE_OVERLAP (40): never thrown anywhere; its docs described code 70 (path traversal). Dropped from exit.mjs and the cli.mjs --help listing. - Document --channel on the update verb (already wired through the scripts for both install and update). - Trim the SKILL.md exit-code table to the codes that change what the agent tells the user (5, 10, 80, 90); defer the full list to the script's --help, removing the triplicated/drifting table. - Lightly tighten the intro and script-path prose loaded on every skill activation; no behavioral change. Integration suite 73 pass / 0 fail; validate:refs clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The standalone bmad-module skill only understood the new plugin.json#bmad
spec, so installing a legacy repo (marketplace.json + module.yaml, e.g.
bmad-code-org/bmad-module-game-dev-studio) failed with exit 20. Legacy
support existed only in the full installer's PluginResolver, which the
skill can't import (it ships self-contained under .claude/skills/).
Port that resolver into lib/legacy-resolver.mjs (strategies 1-5): when a
repo has no plugin.json#bmad but has a marketplace.json, resolve it into a
synthetic manifest of the same shape readAndValidateManifest produces, then
run it through the existing buildCopyPlan -> rewrite -> stage -> swap
pipeline unchanged. buildCopyPlan already copies marketplace.json verbatim,
flattens arbitrary skill paths to skills/<basename>, and flattens
moduleDefinition/moduleHelpCsv, so almost no downstream change is needed.
- plugin-json.mjs: extract validateManifestObject(m, {allowReserved}) and
add hasBmadPluginJson(dir). Legacy installs pass allowReserved:true so
first-party codes (gds, bmm, ...) install; new-spec authors still get
exit 21.
- install.mjs: detect new-spec -> legacy -> neither in §3; write
synthesized module.yaml/module-help.csv into the temp clone for the
strategy-5 fallback.
- cli.mjs: add --module <code> to disambiguate a multi-module marketplace
(otherwise exit 20 lists the available codes).
- help-catalog.mjs: export MODULE_HELP_CSV_HEADER for the synthesizer.
- tests: legacy fixtures (strategy-1, reserved code, synthesize fallback)
+ integration assertions. SKILL.md/README updated.
Verified: full install of the real game-dev-studio repo resolves gds and
lands 250 files under _bmad/gds/. Integration suite 97/0, installer
component tests 374/0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, and clone cache Close the remaining capability gaps where the skill's custom-module install was narrower than the canonical installer (tools/installer/modules/custom-module-manager.js + channel-resolver.js). The new-spec install path was already shared code and the legacy resolver a faithful port, so this targets only the independently-implemented source/channel/cache plumbing. - source.mjs parseSource: accept `owner/repo@ref`, and browser-style deep-path git URLs (tree/blob, GitLab `-/tree`, Gitea `src/branch`, `?path=`), extracting the embedded ref + repo subdirectory — so a module in a monorepo subfolder installs directly. URL-based parsing handles Azure DevOps `_git`, nested groups, and dotted repo names. - lib/cache.mjs: shared clone cache at ~/.bmad/cache/custom-modules/<host>/<owner>/<repo>/ with .bmad-source.json/.bmad-channel.json metadata, matching the installer (reuse on matching ref, fetch/refresh otherwise, keep stale copy on fetch failure). materializeSource copies the module root out of the cache into a throwaway temp tree so the cache is never mutated. - lib/channel-resolver.mjs: node:-only port of resolveChannel (stable/next/pinned); `stable` resolves the latest non-prerelease GitHub tag, falling back to next. semver-lite gains prerelease/compare/rcompare so it stays registry-free. - install.mjs/update.mjs: resolve channel+ref before clone; update re-resolves the channel the module was installed with. Tests + CI: - test/test-bmad-module-source.mjs: unit coverage for parseSource, semver-lite, and channel-resolver, at parity with the installer's test-parse-source-urls.js / test-installer-channels.js. - Wire the skill's unit test (test:skill-source) and its end-to-end integration test (test:skill) into npm test, npm run quality, and the quality.yaml CI job — the integration test was previously committed but only ever run by hand. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-plugin # Conflicts: # test/test-installation-components.js # tools/installer/core/installer.js
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it. |
The acme-devlog fixture's .mcp.json was swallowed by the global .gitignore '.mcp.json' rule, so it was never committed. The skill's install test asserts _bmad/devlog/.mcp.json exists, which passed locally (file on disk) but failed on CI's clean checkout. Add a negation for the test-fixtures path and commit the fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
augment review |
📝 WalkthroughWalkthroughAdds the ChangesCommunity module management stack
Sequence Diagram(s)sequenceDiagram
participant bmad-module as bmad-module CLI
participant install as runInstall
participant source as materializeSource
participant cache as ensureCachedRepo
participant plan as buildCopyPlan
participant fs as atomicSwapDir
participant manifest as manifest-ops
participant config as regenerateCentralConfig
participant ide as distributeToIdes
bmad-module as bmad-module CLI->>install: install/update args
install->>source: parse and materialize source
source->>cache: fetch or refresh repo
cache-->>source: repoDir, sha, ref
source-->>install: temp working tree
install->>plan: build copy plan
plan-->>install: staged file plan
install->>fs: stage and swap module dir
install->>manifest: update module, skill, and file indexes
install->>config: regenerate config blocks
install->>ide: distribute skills and prune stale copies
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🤖 Augment PR SummarySummary: Adds Changes:
Technical Notes: The skill ships zero npm dependencies — 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| throw new BmadModuleError(EXIT.USAGE, `subdirectory "${descriptor.subdir}" not found in ${descriptor.displayName}`); | ||
| } | ||
|
|
||
| await copyDir(moduleRoot, dir, STAGE_IGNORE); |
There was a problem hiding this comment.
If copyDir throws (e.g. I/O error during the copy from cache to the temp working tree), the temp directory created at line 203 leaks — cleanup() is not called on this error path. The same gap exists on line 213 for the local-source branch. The caller's try…finally never fires because materializeSource itself throws before returning materialized.
Other locations where this applies: src/core-skills/bmad-module/scripts/lib/source.mjs:213
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in 02806ba. Both terminal copyDir calls (local and git branches) now run cleanup() before rethrowing, so the temp working dir no longer leaks on a copy error.
| await lib.stageCopyPlan(sourceDir, stagedDir, plan, { | ||
| '.claude-plugin/plugin.json': rewrittenManifestJson, | ||
| }); | ||
| await lib.atomicSwapDir(stagedDir, targetPath); |
There was a problem hiding this comment.
The new _installFromPluginJson path uses atomicSwapDir which silently removes any pre-existing directory at targetPath. The legacy installFromResolution path (line 326) has an explicit existence check (if (await fs.pathExists(targetPath)) { await fs.remove(targetPath); }). While functionally similar, the legacy path's explicit check is visible and intentional, whereas here a pre-existing module directory from a different source (e.g. a source: 'built-in' module whose code collides) would be silently replaced without any guard. Consider adding an explicit existence/collision check before the swap, consistent with the legacy path.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Contributor
Author
There was a problem hiding this comment.
Leaving as-is. The legacy path's if (pathExists) await fs.remove(targetPath) is functionally identical to what atomicSwapDir already does internally (fs-safe.mjs removes targetDir before the final rename) — both unconditionally replace an existing dir, so an explicit check here would be cosmetic. A genuine cross-source collision guard (erroring when a different-source module occupies the code) would be a behavior change that could break legitimate reinstall/update flows, so it's out of scope for this PR.
| } | ||
|
|
||
| const explicitRef = opts.ref ?? descriptor.ref ?? null; | ||
| let channel = opts.channel || (explicitRef ? 'pinned' : 'next'); |
There was a problem hiding this comment.
Invalid --channel values (anything other than stable, pinned, or next) silently fall through all the if branches and are treated as next without any warning. A typo like --channel stabl would silently track the default branch instead of erroring out.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Contributor
Author
There was a problem hiding this comment.
Fixed in 02806ba. resolveCloneTarget now rejects any --channel outside {stable, pinned, next} with a usage error instead of silently falling through to next.
|
Summary: Adds Changes:
Technical Notes: The skill is zero-dependency at runtime (vendored yaml, hand-rolled semver-lite, bundled IDE engine). Central config generation, working-directory creation, and help-catalog merging mirror the full installer's behavior. 📊 Shared Engine Architectureflowchart TB
subgraph Install Paths
User["User request"]
Skill["bmad-module skill"]
CLI["bmad CLI installer"]
end
subgraph Shared Engine
IdeSync["core/ide-sync.js"]
PluginResolver["PluginResolver"]
ManifestGen["ManifestGenerator"]
end
subgraph Outputs
BmadDir["_bmad/code/"]
IdeDirs[".claude/skills/ etc."]
Manifests["manifest.yaml + CSVs"]
end
User --> Skill
User --> CLI
Skill -->|vendored bundle| IdeSync
CLI --> IdeSync
Skill --> PluginResolver
CLI --> PluginResolver
IdeSync --> IdeDirs
PluginResolver --> BmadDir
CLI --> ManifestGen
ManifestGen --> Manifests
Skill --> Manifests
✅ Reviewer Checklist
Was this description helpful? 👍 if useful, 👎 if not • 🚀 if the diagram was helpful • ❤️ if the checklist was useful |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
tools/installer/core/ide-sync.js-56-59 (1)
56-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not cleanup
_bmadskill sources when any IDE sync fails.
cleanupBmadSkillDirsruns even whensetupBatchreports failed IDEs. That makes retry impossible for failed targets because source skill dirs are already removed.💡 Suggested fix
- if (cleanup) await cleanupBmadSkillDirs(bmadDir); + const allSucceeded = results.every((r) => r && r.success); + if (cleanup && allSucceeded) await cleanupBmadSkillDirs(bmadDir);As per coding guidelines,
tools/**changes should “Check error handling and proper exit codes”; this failure path currently performs irreversible cleanup before a successful end state is reached.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/installer/core/ide-sync.js` around lines 56 - 59, The cleanupBmadSkillDirs function is being called unconditionally even when setupBatch reports failures, making it impossible to retry failed targets since the source directories are already removed. Modify the cleanup logic to only call cleanupBmadSkillDirs when setupBatch completes successfully without any failures. Capture the result or error status from setupBatch and guard the cleanup conditional so it only executes if the sync was successful, preventing irreversible cleanup before all targets have been successfully synced.Source: Coding guidelines
tools/installer/core/ide-sync.js-82-87 (1)
82-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstrain cleanup paths to
_bmad/before deletion.
record.pathis used to buildsourceDirand then removed without boundary validation. A malformed CSV row (absolute path or../) can escape_bmad/and delete arbitrary directories.💡 Suggested fix
for (const record of records) { if (!record.path) continue; const relativePath = record.path.startsWith(bmadPrefix) ? record.path.slice(bmadPrefix.length) : record.path; - const sourceDir = path.dirname(path.join(bmadDir, relativePath)); + const skillFilePath = path.resolve(bmadDir, relativePath); + const relToBmad = path.relative(bmadDir, skillFilePath); + if (relToBmad === '' || relToBmad.startsWith('..') || path.isAbsolute(relToBmad)) continue; + const sourceDir = path.dirname(skillFilePath); + if (sourceDir === bmadDir) continue; if (await fs.pathExists(sourceDir)) { await fs.remove(sourceDir); await removeEmptyParents(path.dirname(sourceDir), bmadDir); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/installer/core/ide-sync.js` around lines 82 - 87, The code removes directories based on record.path without validating that the resulting sourceDir path remains within the bmadDir boundary. Attackers could provide absolute paths or paths with ../ sequences in the CSV to escape _bmad/ and delete arbitrary directories. Before calling fs.remove on sourceDir and removeEmptyParents, validate that the resolved sourceDir path is contained within the resolved bmadDir path. Use path.resolve() to normalize both paths, then verify that sourceDir starts with bmadDir and is not attempting to escape using traversal patterns. Only proceed with deletion if this validation passes.tools/installer/modules/bmad-module-lib.js-75-88 (1)
75-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently downgrade malformed
plugin.jsonto “not a new-system module.”
readPluginManifest()currently swallows JSON parse failures and returnsnull, which makes malformed.claude-plugin/plugin.jsonlook identical to “file missing / no bmad block.” That can bypass the intended invalid-manifest surfacing path and fall through to legacy behavior.🔧 Proposed fix
async function readPluginManifest(dir) { const fs = require('../fs-native'); const manifestPath = path.join(dir, '.claude-plugin', 'plugin.json'); if (!(await fs.pathExists(manifestPath))) return null; try { const parsed = JSON.parse(await fs.readFile(manifestPath, 'utf8')); if (parsed && typeof parsed === 'object' && parsed.bmad && typeof parsed.bmad === 'object') { return parsed; } - } catch { - // Malformed JSON — treat as "not a new-system module" and let the legacy - // resolver (or validateDeclaredPaths at install time) surface the problem. + } catch (error) { + throw new Error(`Invalid JSON in ${manifestPath}: ${error.message}`); } return null; }As per coding guidelines, tooling changes under
tools/**should preserve proper error handling instead of suppressing actionable failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/installer/modules/bmad-module-lib.js` around lines 75 - 88, The readPluginManifest function is silently catching JSON parse errors and returning null, which makes malformed plugin.json files indistinguishable from missing files. Modify the error handling so that the function only returns null when the manifest file does not exist (the pathExists check on line 78 handles this case). When the file exists but contains malformed JSON in the try-catch block, the error should be re-thrown or propagated rather than silently swallowed, allowing the invalid manifest to be properly surfaced and not fall through to legacy behavior. Keep the null return for cases where the parsed object is valid but lacks the required bmad block structure.Source: Coding guidelines
tools/installer/project-root.js-174-180 (1)
174-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved
moduleYamlPathwhen matching bypluginName.The new
pluginNamematch can identify a specific resolved plugin inside a multi-module repo, butsearchRoot(mod.localPath)then returns the first conventionalmodule.yamlunder the whole root. That can bind config/agent generation to the wrong module definition. Prefer the cached resolution’s exactmoduleYamlPathbefore falling back to root scanning.🐛 Proposed fix
const matches = mod.code === moduleName || mod.name === moduleName || mod.pluginName === moduleName; -if (matches && mod.localPath) { +if (matches && mod.moduleYamlPath && (await fs.pathExists(mod.moduleYamlPath))) { + return mod.moduleYamlPath; +} +if (matches && mod.localPath) { const found = await searchRoot(mod.localPath); if (found) return found; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/installer/project-root.js` around lines 174 - 180, The current matching logic in the condition checking code/name/pluginName always falls back to calling searchRoot(mod.localPath) when a match is found, which can return an incorrect module.yaml in multi-module repos. Instead, when a match is found and mod.localPath exists, first check if a cached moduleYamlPath is available on the mod object and use that directly, only falling back to searchRoot(mod.localPath) if the moduleYamlPath is not available. This ensures that when a specific plugin is resolved by pluginName, its exact moduleYamlPath is used rather than scanning the root directory.tools/installer/modules/official-modules.js-451-453 (1)
451-453:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate directories from the flattened installed
module.yaml.
buildCopyPlan()flattens plugin-jsonmoduleDefinitionto_bmad/<code>/module.yaml, butcreateModuleDirectories()still resolvesmodule.yamlfrom the source root viafindModuleSource(). New-spec modules that keepmodule.yamlunder a declared skill asset path will install successfully but skip their declared working directories.🐛 Proposed fix
async createModuleDirectories(moduleName, bmadDir, options = {}) { const moduleConfig = options.moduleConfig || {}; const existingModuleConfig = options.existingModuleConfig || {}; const projectRoot = path.dirname(bmadDir); const emptyResult = { createdDirs: [], movedDirs: [], createdWdsFolders: [] }; + let moduleYamlPath = path.join(bmadDir, moduleName, 'module.yaml'); + if (!(await fs.pathExists(moduleYamlPath))) { // Special handling for core module - it's in src/core-skills not src/modules let sourcePath; if (moduleName === 'core') { sourcePath = getSourcePath('core-skills'); } else { sourcePath = await this.findModuleSource(moduleName, { silent: true }); if (!sourcePath) { return emptyResult; // No source found, skip } } // Read module.yaml to find the `directories` key - const moduleYamlPath = path.join(sourcePath, 'module.yaml'); - if (!(await fs.pathExists(moduleYamlPath))) { - return emptyResult; // No module.yaml, skip - } + moduleYamlPath = path.join(sourcePath, 'module.yaml'); + } + if (!(await fs.pathExists(moduleYamlPath))) { + return emptyResult; // No module.yaml, skip + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/installer/modules/official-modules.js` around lines 451 - 453, The createModuleDirectories() method is resolving module.yaml from the source root via findModuleSource(), but buildCopyPlan() has already flattened the moduleDefinition to the bmadDir location at _bmad/<code>/module.yaml. Update createModuleDirectories() to read the module.yaml directly from the bmadDir parameter instead of calling findModuleSource(), so it uses the flattened module.yaml that contains the correct directory declarations for new-spec modules.tools/installer/ui.js-1297-1302 (1)
1297-1302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail non-interactive installs when an explicit custom source cannot resolve.
This warning lets
--custom-sourcecontinue and eventually exit successfully with the requested module omitted. For CLI/CI use, treat direct-mode resolution failure as fatal, or collect failures and throw after processing all sources.🐛 Proposed fix
try { const resolved = await customMgr.resolvePlugin(sourceResult.rootDir, directPlugin, sourceResult.sourceUrl, localPath); allResolved.push(...resolved); } catch (resolveError) { - await prompts.log.warn(` Could not resolve ${source}: ${resolveError.message}`); + s2.error(`Failed to resolve ${source}`); + throw new Error(`Could not resolve ${source}: ${resolveError.message}`); }As per coding guidelines,
tools/**: “Build script/tooling. Check error handling and proper exit codes.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/installer/ui.js` around lines 1297 - 1302, The current error handling in the customMgr.resolvePlugin call only logs a warning and allows the process to continue, but for non-interactive CLI/CI environments this should fail fatally. After logging the warning in the resolveError catch block, check if the install is running in non-interactive mode (likely via a flag or configuration property). If in non-interactive mode, either throw the resolveError immediately to halt the installation, or collect the failure and throw an error after processing all sources to report all failures at once. This ensures that explicit custom sources that fail to resolve will not be silently omitted during automated installations.Source: Coding guidelines
src/core-skills/bmad-module/scripts/lib/cache.mjs-128-158 (1)
128-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid storing cache metadata inside the cloned repo root.
Lines 128-158 write
.bmad-source.jsonand.bmad-channel.jsondirectly intorepoDir. If a module repository legitimately contains those filenames, the cache layer overwrites module content and contaminates subsequent installs/updates.Store these sidecars outside the working tree (e.g., sibling metadata directory keyed by cacheKey) to keep cloned content immutable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/cache.mjs` around lines 128 - 158, The metadata files `.bmad-source.json` and `.bmad-channel.json` are currently being written directly into the cloned repository directory (repoDir), which can overwrite legitimate module files if they exist. Instead of writing these files to path.join(repoDir, '.bmad-source.json') and path.join(repoDir, '.bmad-channel.json'), create a separate metadata directory structure keyed by the cacheKey and write the metadata files there. This keeps the cloned repository content immutable and prevents the cache layer from contaminating module files. Update both fs.writeFile calls to use the new metadata directory path instead of repoDir.src/core-skills/bmad-module/scripts/lib/install-plan.mjs-313-320 (1)
313-320:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDirectory-valued surface paths conflict with fixed manifest rewrites.
Line 313 allows directory sources for
hooks/mcpServers/lspServers/settings, but Lines 357-360 always rewrite those keys to fixed file paths (./hooks.json,./.mcp.json, etc.). That can leave installed manifests pointing at files that were never copied.Also applies to: 357-360
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/install-plan.mjs` around lines 313 - 320, The code at lines 313-320 allows directory sources to be handled via the addDirRecursive function when stat.isDirectory() is true, but the manifest rewrite logic at lines 357-360 always converts surface paths to fixed file paths like ./hooks.json or ./.mcp.json. This creates a conflict where directories are copied to the install tree but the manifest points to fixed filenames that don't exist. Remove the directory handling block (the if statement checking stat.isDirectory() and calling addDirRecursive) and only keep the file handling logic in the else if (stat.isFile()) branch to ensure the manifest only references files that are actually copied.src/core-skills/bmad-module/scripts/lib/install-plan.mjs-369-380 (1)
369-380:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
customize.schemasrewrite is lossy for nested paths.Lines 375-380 assume the final two segments are
<skill>/<file>. Nested schema paths (e.g.,.../<skill>/schemas/.../x.yaml) are rewritten under the wrong skill directory, breaking schema resolution after install.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/install-plan.mjs` around lines 369 - 380, The heuristic in the schemas mapping that assumes the skill name is always at parts.at(-2) and filename at parts.at(-1) is incorrect for nested schema paths like `.../<skill>/schemas/subdir/x.yaml`. Instead of taking only the last two segments, identify the skill name in the path (potentially by finding a known pattern such as where a skill directory marker appears) and preserve all path segments after the skill name as the relative path within that skill directory. Update the reconstruction logic to maintain nested directory structures when rewriting paths with the skill name..github/workflows/quality.yaml-106-117 (1)
106-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
test:ide-syncgate to CI.Lines 109-117 run installer and skill tests but skip
npm run test:ide-sync, even though it is part of thequalityscript inpackage.json. This creates a CI gap where IDE sync regressions can merge undetected.Suggested patch
- name: Test agent compilation components run: npm run test:install + - name: Test IDE sync core behavior + run: npm run test:ide-sync + - name: Test bmad-module skill (source parsing + channels) run: npm run test:skill-source🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/quality.yaml around lines 106 - 117, Add the missing test:ide-sync gate to the CI workflow. In the .github/workflows/quality.yaml file, after the existing test steps (test:install, test:skill-source, test:skill) that run npm commands like npm run test:install and npm run test:skill, add a new step named "Test IDE sync" that runs npm run test:ide-sync. This ensures the IDE synchronization tests are included in the CI quality checks and prevent regressions from merging undetected.src/core-skills/bmad-module/scripts/lib/config-gen.mjs-253-255 (1)
253-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale
[agents.*]blocks are not fully cleaned during regenerate.Lines 253-255 only remove agent blocks whose codes are present in the current
module.yaml. Removed/renamed agents (or missingmodule.yaml) leave orphaned agent entries inconfig.toml.Suggested direction
- const kept = blocks.filter( - (b) => b.header !== `modules.${sectionKey}` && !(b.header.startsWith('agents.') && agentCodes.has(b.header.slice('agents.'.length))), - ); + const kept = blocks.filter((b) => { + if (b.header === `modules.${sectionKey}` || b.header === `modules.${code}`) return false; + if (!b.header.startsWith('agents.')) return true; + + const agentCode = b.header.slice('agents.'.length); + if (agentCodes.has(agentCode)) return false; + + // Fallback cleanup for removed agents: drop by module ownership. + const moduleLine = b.lines.find((l) => /^module\s*=/.test(l)); + return !moduleLine || parseTomlScalar(moduleLine.split('=').slice(1).join('=')) !== code; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/config-gen.mjs` around lines 253 - 255, The filter logic in the `kept` variable assignment is preserving stale agent blocks because it only removes agents whose codes exist in the current agentCodes set. To fix this, modify the filter condition to remove all blocks with headers starting with 'agents.' during the regeneration process, rather than trying to preserve some agents based on whether their codes are present. This ensures orphaned entries from removed or renamed agents are fully cleaned up from the config.toml file, regardless of what was previously there.src/core-skills/bmad-module/scripts/lib/channel-resolver.mjs-25-32 (1)
25-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle GitHub URLs ending with
.git/correctly.Current normalization strips
.gitbefore removing a trailing slash, sohttps://github.com/o/r.git/becomes repor.gitand stable tag lookup hits the wrong API path (then falls back tonext).Suggested fix
- const trimmed = url - .trim() - .replace(/\.git$/, '') - .replace(/\/$/, ''); + const trimmed = url + .trim() + .replace(/\/+$/, '') + .replace(/\.git$/, '');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/channel-resolver.mjs` around lines 25 - 32, The normalization of the URL in the trimmed variable uses an incorrect order for removing the .git suffix and trailing slash. When a URL ends with .git/ (like https://github.com/o/r.git/), the current regex for removing .git doesn't match because it looks for .git at the end but finds / instead, leaving .git in the repo name. Reverse the order of the two replace operations so the trailing slash is removed first (before attempting to remove .git), ensuring .git is always stripped correctly regardless of whether a trailing slash is present.src/core-skills/bmad-module/scripts/lib/ide-sync.mjs-22-26 (1)
22-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDifferentiate invalid manifest from “no IDEs configured”.
readManifestYamlreturnsnullfor both missing and parse-failed manifest content, but this branch reports{ skipped: true }. That can hide a real config corruption and silently skip distribution.Suggested fix
const manifest = await readManifestYaml(bmadDir); + if (!manifest) { + return { + ok: false, + hint: + 'Could not read _bmad/_config/manifest.yaml (missing or invalid). ' + + 'Run `bmad install` to repair BMAD config, then `bmad ide-sync`.', + }; + } const ides = Array.isArray(manifest?.ides) ? manifest.ides.filter((i) => i && typeof i === 'string') : [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/ide-sync.mjs` around lines 22 - 26, The code currently treats a null manifest (returned by readManifestYaml when the manifest file is missing or fails to parse) the same as having no IDEs configured, both returning { skipped: true }. This masks config corruption or parse failures that should be reported as errors. Modify the code to first check if the manifest is null and handle that case separately - either by throwing an error, logging a warning, or checking the actual reason for the null return (missing file vs parse failure). Only return { skipped: true } when the manifest exists but contains no valid IDEs in the ides array.src/core-skills/bmad-module/scripts/cli.mjs-27-47 (1)
27-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unknown flags instead of silently accepting them.
Any
--<key> <value>pair is currently accepted, even when unsupported. Typos then run with unintended defaults instead of failing fast.Suggested fix
const VERBS = new Set(['install', 'update', 'remove', 'list']); +const BOOLEAN_FLAGS = new Set(['dry-run', 'purge', 'all', 'json']); +const VALUE_FLAGS = new Set(['ref', 'channel', 'module', 'set', 'project-dir']); @@ if (a.startsWith('--')) { const key = a.slice(2); + if (!BOOLEAN_FLAGS.has(key) && !VALUE_FLAGS.has(key)) { + throw new BmadModuleError(EXIT.USAGE, `unknown flag --${key}`); + } // boolean flags - if (['dry-run', 'purge', 'all', 'json'].includes(key)) { + if (BOOLEAN_FLAGS.has(key)) { out.flags[key] = true; i++; continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/cli.mjs` around lines 27 - 47, The code currently accepts any unknown flag that starts with '--' and takes a value, without validating if that flag is actually supported. Add a whitelist of allowed value flags (similar to the existing boolean flags whitelist) and validate the key against this list before processing value flags. If the key is not in the whitelist of supported flags, throw a BmadModuleError with an appropriate message indicating the flag is unknown, so typos fail fast instead of silently using defaults.src/core-skills/bmad-module/scripts/install.mjs-215-244 (1)
215-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
--channelagainst allowed values before resolving.Unsupported channel values currently fall through to
next. That silently changes install/update behavior on input typos.Suggested fix
const explicitRef = opts.ref ?? descriptor.ref ?? null; let channel = opts.channel || (explicitRef ? 'pinned' : 'next'); + const allowedChannels = new Set(['stable', 'next', 'pinned']); + if (!allowedChannels.has(channel)) { + throw new BmadModuleError( + EXIT.USAGE, + `invalid --channel "${channel}" (expected stable|next|pinned)`, + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/install.mjs` around lines 215 - 244, The function currently accepts any channel value and silently falls through to 'next' if it doesn't match 'pinned' or 'stable', which masks input typos. Add validation logic early in the function (after the channel variable is set from opts.channel) to check if the channel value is one of the allowed values ('pinned', 'stable', or 'next'), and throw an error or write a warning to process.stderr if an invalid channel is provided instead of silently falling back to 'next'.src/core-skills/bmad-module/scripts/cli.mjs-149-188 (1)
149-188:⚠️ Potential issue | 🟠 MajorAdd documentation for the
bmad-moduleCLI tool.This PR introduces new user-facing CLI verbs (
install,update,remove,list) and flags (--channel,--set,--purge,--dry-run) for thebmad-modulecommand, but there is no corresponding documentation indocs/. Per coding guidelines,src/**changes should include documentation updates for new features, changed behavior, CLI flags, or configuration options. Create a reference or how-to guide documenting thebmad-modulecommand's usage, options, and examples.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/cli.mjs` around lines 149 - 188, The `printUsage()` function documents the `bmad-module` CLI tool with commands (install, update, remove, list) and various flags (--ref, --channel, --module, --set, --dry-run, --purge, --project-dir, --json), but there is no corresponding documentation file in the `docs/` directory. Create a comprehensive documentation guide in `docs/` that covers all the commands, flags, usage examples, and exit codes currently shown in the `printUsage()` function to make this information accessible to users outside of the CLI help output.Source: Coding guidelines
src/core-skills/bmad-module/scripts/lib/source.mjs-67-68 (1)
67-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
~/...expansion is currently broken.
body.slice(1)preserves the leading/for~/x, sopath.join(os.homedir(), '/x')resolves to/x, not inside the home directory.Suggested fix
- const expanded = body.startsWith('~') ? path.join(os.homedir(), body.slice(1)) : body; + const expanded = body === '~' ? os.homedir() : body.startsWith('~/') ? path.join(os.homedir(), body.slice(2)) : body;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/source.mjs` around lines 67 - 68, The tilde expansion in the path handling for body.startsWith('~') is incorrect because body.slice(1) preserves the leading forward slash from inputs like ~/x, resulting in /x being passed to path.join. This causes path.join(os.homedir(), '/x') to discard the home directory and resolve to just /x instead of the intended path inside the home directory. Fix this by removing the leading forward slash from the sliced body before passing it to path.join, so that the home directory path is properly preserved.src/core-skills/bmad-module/scripts/lib/fs-safe.mjs-111-115 (1)
111-115:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
atomicSwapDircan permanently delete a working target on late failure.
targetDiris deleted beforerename(sibling, targetDir). If that rename fails, the catch block removessiblingtoo, leaving neither old nor new install.Safer swap pattern (with rollback)
export async function atomicSwapDir(stagedDir, targetDir) { const parent = path.dirname(targetDir); await fsp.mkdir(parent, { recursive: true }); const sibling = path.join(parent, `.${path.basename(targetDir)}.bmad-tmp-${crypto.randomBytes(6).toString('hex')}`); + const backup = path.join(parent, `.${path.basename(targetDir)}.bmad-old-${crypto.randomBytes(6).toString('hex')}`); try { try { await fsp.rename(stagedDir, sibling); @@ - await fsp.rm(targetDir, { recursive: true, force: true }); - await fsp.rename(sibling, targetDir); + const hadTarget = await fsp.stat(targetDir).then(() => true).catch(() => false); + if (hadTarget) await fsp.rename(targetDir, backup); + try { + await fsp.rename(sibling, targetDir); + } catch (e) { + if (hadTarget) await fsp.rename(backup, targetDir).catch(() => {}); + throw e; + } + if (hadTarget) await fsp.rm(backup, { recursive: true, force: true }); } catch (e) { await fsp.rm(sibling, { recursive: true, force: true }); + await fsp.rm(backup, { recursive: true, force: true }); throw e; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/fs-safe.mjs` around lines 111 - 115, The atomicSwapDir function deletes targetDir before attempting to rename sibling to targetDir, creating a risk of data loss if the rename operation fails. Reorder the operations in the try block so that targetDir is deleted only after the fsp.rename(sibling, targetDir) call succeeds, ensuring that if the rename fails, the original targetDir remains intact and can be recovered, rather than leaving both directories deleted.src/core-skills/bmad-module/scripts/lib/module-dirs.mjs-109-114 (1)
109-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
wds_foldersentries before path join.
subis untrusted module content;path.join(fullPath, sub)currently allows traversal/absolute escapes.Suggested guard
if (configKey === 'design_artifacts' && wdsFolders.length) { for (const sub of wdsFolders) { - const subPath = path.join(fullPath, sub); + if (typeof sub !== 'string' || sub === '') continue; + const subPath = path.resolve(fullPath, sub); + if (subPath !== fullPath && !subPath.startsWith(fullPath + path.sep)) { + warn(`invalid wds_folders entry outside design_artifacts: ${sub}`); + continue; + } if (!(await pathExists(subPath))) { await fs.mkdir(subPath, { recursive: true }); createdWdsFolders.push(sub); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/module-dirs.mjs` around lines 109 - 114, The variable `sub` from the `wdsFolders` array is untrusted module content and should be validated before being used in `path.join(fullPath, sub)` to prevent path traversal attacks. Add validation logic before the path.join call in the loop that iterates over `wdsFolders` to ensure each `sub` entry is a safe relative path that doesn't contain traversal sequences like ".." or start with "/" or absolute path indicators. Only proceed with creating the subdirectory if the validation passes, otherwise skip that entry or throw an error to prevent malicious paths from being created.src/core-skills/bmad-module/scripts/list.mjs-7-43 (1)
7-43:⚠️ Potential issue | 🟠 MajorDocument the
bmad-moduleskill commands and flags indocs/.The bmad-module skill is user-facing but undocumented in the main
docs/directory. Add a how-to or reference page covering the four commands (install,update,remove,list) and their respective flags:--ref,--channel,--module,--dry-run(install);--ref,--channel,--all(update);--purge(remove);--json(list). The skill is documented insrc/core-skills/bmad-module/README.mdandSKILL.md; port the relevant information into user-facing docs to comply with the coding guideline forsrc/**changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/list.mjs` around lines 7 - 43, Add user-facing documentation for the bmad-module skill in the `docs/` directory. Create a how-to or reference page that documents all four commands (install, update, remove, list) and their respective flags. For the install command, document the --ref, --channel, --module, and --dry-run flags; for update, document --ref, --channel, and --all flags; for remove, document the --purge flag; and for list, document the --json flag. Port the relevant information from the existing documentation in `src/core-skills/bmad-module/README.md` and `SKILL.md` to ensure consistency and compliance with the coding guideline that `src/**` changes require user-facing documentation.Source: Coding guidelines
src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-agent-historian/customize.toml-23-28 (1)
23-28:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't require the devlog config before the setup fallback.
bmad-agent-historian/SKILL.mdloads persistent facts before it reaches the later "config missing" branch, so thisfile:fact can make first-run activation fail before the graceful/bmad-devlog-setuppath runs. Defer the config read until after the existence check, or drop it from always-on facts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-agent-historian/customize.toml` around lines 23 - 28, The persistent_facts array in customize.toml includes a file reference to the devlog config that must exist, but this file read happens before the existence check in the setup fallback logic, causing first-run activation to fail before the graceful /bmad-devlog-setup path can run. Remove the "file:{project-root}/_bmad/devlog/config.yaml" entry from the persistent_facts array and instead defer loading this config file conditionally after verifying the config exists, or restructure the skill loading flow in SKILL.md to check for config existence before initializing persistent_facts.
🟡 Minor comments (7)
src/core-skills/bmad-module/scripts/lib/legacy-resolver.mjs-308-311 (1)
308-311:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSupport CRLF frontmatter delimiters when parsing
SKILL.md.Frontmatter parsing currently assumes LF-only separators; CRLF files silently drop
name/descriptionin synthesized fallback metadata.Suggested fix
- const match = content.match(/^---\s*\n([\s\S]*?)\n---/); + const match = content.match(/^---\s*\r?\n([\s\S]*?)\r?\n---/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/legacy-resolver.mjs` around lines 308 - 311, The regex pattern in the match statement at line 308 currently only matches LF line endings (\n) but fails to account for CRLF line endings (\r\n), causing frontmatter parsing to fail on Windows-formatted files. Update the regex pattern used in the content.match() call to handle both LF and CRLF separators by making the line ending matching more flexible, so that frontmatter delimiters with either line ending format are correctly recognized and parsed.src/core-skills/bmad-module/scripts/lib/frontmatter.mjs-6-7 (1)
6-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFrontmatter parser rejects valid blocks at EOF.
The current regex requires a newline after the closing
---, so frontmatter that ends exactly at EOF won’t parse.Suggested fix
- const m = content.match(/^---\r?\n([\s\S]*?)\r?\n---\r?\n/); + const m = content.match(/^---\r?\n([\s\S]*?)\r?\n---(?:\r?\n|$)/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/scripts/lib/frontmatter.mjs` around lines 6 - 7, The regex pattern in the match call for parsing frontmatter blocks currently requires a newline character after the closing `---`, which causes it to fail when frontmatter ends at EOF without a trailing newline. Modify the regex pattern to make the trailing newline optional by adding a quantifier to the final newline sequence, so it can successfully parse frontmatter blocks that end at the file boundary without requiring a newline after the closing delimiter.src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/README.md-35-36 (1)
35-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the uninstall note with the actual custom-file layout.
_bmad/custom/devlog/doesn’t match the customization files documented indocs/index.md; those are flat TOML files such as_bmad/custom/bmad-agent-historian.tomland_bmad/custom/bmad-agent-historian.user.toml. The current note sends readers to a directory that never exists.♻️ Suggested wording
-bmad-module remove devlog # leaves _bmad/custom/devlog/ intact +bmad-module remove devlog # leaves `_bmad/custom/bmad-agent-historian*.toml` intact🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/README.md` around lines 35 - 36, The uninstall documentation in the README.md file references a non-existent directory structure `_bmad/custom/devlog/` that does not match the actual custom-file layout. Update the notes for the bmad-module remove commands to correctly reference the actual flat TOML files used for customizations (such as `_bmad/custom/bmad-agent-historian.toml` and `_bmad/custom/bmad-agent-historian.user.toml`) instead of the directory path, ensuring the documentation accurately reflects what files are left intact or removed when using the remove command with and without the --purge flag.src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/scripts/fetch-git-history.sh-25-29 (1)
25-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace the
lspipeline with a filename-safe lookup.Parsing
lsoutput here is brittle and can mis-handle non-alphanumeric devlog filenames; ShellCheck already flags this line. Afind-based lookup (or a shell array plus sorting) will be more robust.♻️ Safer lookup
-latest=$(ls -t "${devlog_path}"/*.md 2>/dev/null | head -n 1 || true) +latest=$( + find "$devlog_path" -maxdepth 1 -type f -name '*.md' -printf '%T@ %p\n' 2>/dev/null | + sort -nr | + head -n 1 | + cut -d' ' -f2- +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/scripts/fetch-git-history.sh` around lines 25 - 29, The assignment to the `latest` variable uses an `ls` pipeline which is fragile and can mis-handle filenames with special characters or spaces. Replace the `ls -t "${devlog_path}"/*.md 2>/dev/null | head -n 1` pipeline with a more robust `find`-based approach that sorts markdown files by modification time, or use a shell array with proper filename handling. This will ensure the script correctly identifies the most recent devlog file regardless of special characters in the filename, and will resolve the ShellCheck warning on this line.Source: Linters/SAST tools
src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-summarize/SKILL.md-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
invokewording for the setup handoff.The skill validator expects skill-to-skill calls to be phrased as an invocation, not a raw slash command. As per coding guidelines, invoking another skill must use “invoke” wording.
🔧 Suggested fix
- If missing, run `/bmad-devlog-setup` first. + If missing, invoke `/bmad-devlog-setup` first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-summarize/SKILL.md` at line 14, The instruction text in the SKILL.md file for the bmad-devlog-summarize skill uses "run" instead of "invoke" when referring to the `/bmad-devlog-setup` skill. Change the wording from "run `/bmad-devlog-setup` first" to use "invoke" language instead, following the coding guideline that skill-to-skill calls must be phrased as invocations rather than raw slash commands. Update the text to clearly indicate that the `/bmad-devlog-setup` skill should be invoked.Source: Coding guidelines
src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-write/SKILL.md-27-30 (1)
27-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the template contract in sync with weekly/monthly mode.
bmad-devlog-writesays to substitute{{week}}/{{month}}, but the bundled template only renders{{date}}. Weekly/monthly files will still open with a daily-style header unless this contract is tightened.🔧 Suggested fix
- copy `./assets/template.md` to the target path. Substitute `{{date}}`, `{{author}}` (from `user_name`), and `{{week}}`/`{{month}}` placeholders. + copy `./assets/template.md` to the target path. Substitute `{{date}}` and `{{author}}` (from `user_name`); for `weekly`/`monthly`, render the heading from the period value instead of reusing the daily date.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-write/SKILL.md` around lines 27 - 30, The Step 3 documentation in SKILL.md states that the template should substitute `{{week}}` and `{{month}}` placeholders, but the actual bundled template at `./assets/template.md` only contains the `{{date}}` placeholder. Update the Step 3 description to accurately document which placeholders are actually supported by the bundled template, ensuring the written contract matches what the template implementation actually provides. This ensures users have correct expectations about what substitutions will occur when initializing weekly or monthly devlog files.src/core-skills/bmad-module/tests/integration.test.sh-36-43 (1)
36-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a unique temp file for stderr capture in
run().The current fixed
/tmp/...$$path is collision-prone across overlapping runs and can read stale content on edge failures.Suggested fix
run() { + local stderr_file + stderr_file="$(mktemp)" set +e - STDOUT="$(node "${MODULE_JS}" "$@" 2>/tmp/bmad-module-stderr.$$)" + STDOUT="$(node "${MODULE_JS}" "$@" 2>"${stderr_file}")" EXIT=$? - STDERR="$(cat /tmp/bmad-module-stderr.$$)" - rm -f /tmp/bmad-module-stderr.$$ + STDERR="$(cat "${stderr_file}")" + rm -f "${stderr_file}" set -e }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/tests/integration.test.sh` around lines 36 - 43, The run() function uses a fixed temporary file path /tmp/bmad-module-stderr.$$ for stderr capture which can cause collisions across overlapping test runs and potentially read stale content. Replace the manually constructed temp file path with a call to mktemp to generate a truly unique temporary file name, store it in a variable, and use that variable consistently throughout the run() function where stderr redirection and cleanup occur.
🧹 Nitpick comments (2)
test/test-installation-components.js (1)
3736-3744: ⚡ Quick winAlways clear
CustomModuleManager._resolutionCachewithfinally.If
om.install(...)throws, the cache entry remains and can taint subsequent suites when this file grows.Suggested fix
const [resolved] = await new PluginResolver().resolve(acmeDevlog, { name: 'acme-devlog', source: '.', skills: [] }); resolved.localPath = acmeDevlog; CustomModuleManager._resolutionCache.set(resolved.code, resolved); const om = new OfficialModules(); const tracked = []; - const result = await om.install('devlog', bmadDir, (p) => tracked.push(p), { skipModuleInstaller: true, moduleConfig: {} }); - CustomModuleManager._resolutionCache.delete('devlog'); + let result; + try { + result = await om.install('devlog', bmadDir, (p) => tracked.push(p), { skipModuleInstaller: true, moduleConfig: {} }); + } finally { + CustomModuleManager._resolutionCache.delete('devlog'); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test-installation-components.js` around lines 3736 - 3744, The CustomModuleManager._resolutionCache.delete('devlog') call will not execute if the om.install() call throws an exception, leaving the cache entry in place and potentially tainting subsequent test suites. Wrap the om.install() call in a try-finally block to ensure that CustomModuleManager._resolutionCache.delete('devlog') always executes, regardless of whether the install call succeeds or throws an error.src/core-skills/bmad-module/tests/integration.test.sh (1)
118-119: Replace&& ... ||assertion chains with explicitifblocks.These assertion lines use the SC2015 pattern, which is brittle and can incorrectly execute the failure branch when the success branch command returns non-zero. This applies throughout the test file at: lines 118–119, 125–126, 144–145, 149–150, 193–194, 217, 227–228, 276–277, 279–282, 294–300, 333–334, and 357–359.
Example fix:
Representative rewrite
-[[ "${STDOUT}" == *"no modules installed"* ]] && ok "stdout reports empty" \ - || ko "expected 'no modules installed' in stdout: ${STDOUT}" +if [[ "${STDOUT}" == *"no modules installed"* ]]; then + ok "stdout reports empty" +else + ko "expected 'no modules installed' in stdout: ${STDOUT}" +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-module/tests/integration.test.sh` around lines 118 - 119, The test file uses the brittle SC2015 pattern with `&& ok ... || ko` assertion chains that can incorrectly execute the failure branch when the success command returns non-zero. Replace all instances of this pattern (appearing at lines 118–119, 125–126, 144–145, 149–150, 193–194, 217, 227–228, 276–277, 279–282, 294–300, 333–334, and 357–359) with explicit if/then/else/fi blocks. For each occurrence, convert the pattern from `[[ condition ]] && ok "message" || ko "message"` to `if [[ condition ]]; then ok "message"; else ko "message"; fi` to ensure the failure condition is only executed when the test condition actually fails.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 839b5aab-83a4-4b13-b1c4-f3bf54f9deea
⛔ Files ignored due to path filters (5)
eslint.config.mjsis excluded by!eslint.config.mjssrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-setup/assets/module-help.csvis excluded by!**/*.csvsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-mini-legacy/src/module-help.csvis excluded by!**/*.csvsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-reserved-legacy/src/module-help.csvis excluded by!**/*.csvsrc/core-skills/module-help.csvis excluded by!**/*.csv
📒 Files selected for processing (91)
.github/workflows/quality.yaml.gitignore.prettierignorepackage.jsonsrc/core-skills/bmad-module/README.mdsrc/core-skills/bmad-module/SKILL.mdsrc/core-skills/bmad-module/scripts/bmad-module.mjssrc/core-skills/bmad-module/scripts/cli.mjssrc/core-skills/bmad-module/scripts/install.mjssrc/core-skills/bmad-module/scripts/lib/bmad-dir.mjssrc/core-skills/bmad-module/scripts/lib/cache.mjssrc/core-skills/bmad-module/scripts/lib/channel-resolver.mjssrc/core-skills/bmad-module/scripts/lib/config-gen.mjssrc/core-skills/bmad-module/scripts/lib/exit.mjssrc/core-skills/bmad-module/scripts/lib/frontmatter.mjssrc/core-skills/bmad-module/scripts/lib/fs-safe.mjssrc/core-skills/bmad-module/scripts/lib/help-catalog.mjssrc/core-skills/bmad-module/scripts/lib/ide-sync.mjssrc/core-skills/bmad-module/scripts/lib/install-plan.mjssrc/core-skills/bmad-module/scripts/lib/legacy-resolver.mjssrc/core-skills/bmad-module/scripts/lib/manifest-ops.mjssrc/core-skills/bmad-module/scripts/lib/module-dirs.mjssrc/core-skills/bmad-module/scripts/lib/npm-deps.mjssrc/core-skills/bmad-module/scripts/lib/plugin-json.mjssrc/core-skills/bmad-module/scripts/lib/semver-lite.mjssrc/core-skills/bmad-module/scripts/lib/source.mjssrc/core-skills/bmad-module/scripts/lib/vendor/README.mdsrc/core-skills/bmad-module/scripts/lib/vendor/build-ide-sync.mjssrc/core-skills/bmad-module/scripts/lib/vendor/build-vendor.mjssrc/core-skills/bmad-module/scripts/lib/vendor/ide-sync.mjssrc/core-skills/bmad-module/scripts/lib/vendor/platform-codes.yamlsrc/core-skills/bmad-module/scripts/lib/vendor/shims/project-root.cjssrc/core-skills/bmad-module/scripts/lib/vendor/shims/prompts.cjssrc/core-skills/bmad-module/scripts/lib/vendor/yaml.mjssrc/core-skills/bmad-module/scripts/list.mjssrc/core-skills/bmad-module/scripts/remove.mjssrc/core-skills/bmad-module/scripts/update.mjssrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/.claude-plugin/plugin.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/.mcp.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/CHANGELOG.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/LICENSEsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/README.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/agents/changelog-archivist.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/docs/index.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/hooks/hooks.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/scripts/fetch-git-history.shsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-agent-historian/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-agent-historian/customize.tomlsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-setup/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-setup/assets/module.yamlsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-summarize/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-write/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-write/assets/template.mdsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-mini-legacy/.claude-plugin/marketplace.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-mini-legacy/docs/guide.mdsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-mini-legacy/src/agents/mlg-agent-one/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-mini-legacy/src/module.yamlsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-mini-legacy/src/workflows/mlg-flow/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-reserved-legacy/.claude-plugin/marketplace.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-reserved-legacy/src/agents/gds-agent-demo/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-reserved-legacy/src/module.yamlsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-synth-legacy/.claude-plugin/marketplace.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/legacy/bmad-synth-legacy/src/skills/synthlg-do-thing/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/minimal-npm/acme-npmtool/.claude-plugin/plugin.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/minimal-npm/acme-npmtool/package.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/minimal-npm/acme-npmtool/skills/acme-npmtool/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/minimal/acme-md-lint/.claude-plugin/plugin.jsonsrc/core-skills/bmad-module/tests/fixtures/examples/minimal/acme-md-lint/LICENSEsrc/core-skills/bmad-module/tests/fixtures/examples/minimal/acme-md-lint/README.mdsrc/core-skills/bmad-module/tests/fixtures/examples/minimal/acme-md-lint/skills/acme-md-lint/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/module-bad-missing-fields/.claude-plugin/plugin.jsonsrc/core-skills/bmad-module/tests/fixtures/module-bad-traversal/.claude-plugin/plugin.jsonsrc/core-skills/bmad-module/tests/fixtures/module-bad-traversal/skills/skill-a/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/module-reserved-code/.claude-plugin/plugin.jsonsrc/core-skills/bmad-module/tests/fixtures/module-reserved-code/skills/skill-a/SKILL.mdsrc/core-skills/bmad-module/tests/integration.test.shtest/test-bmad-module-source.mjstest/test-ide-sync.jstest/test-installation-components.jstools/installer/commands/ide-sync.jstools/installer/core/ide-sync.jstools/installer/core/installer.jstools/installer/core/manifest-generator.jstools/installer/ide/platform-codes.jstools/installer/modules/bmad-module-lib.jstools/installer/modules/custom-module-manager.jstools/installer/modules/official-modules.jstools/installer/modules/plugin-resolver.jstools/installer/project-root.jstools/installer/ui.jstools/validate-skills.js
Address automated review findings on PR #2482: - source.mjs: validate URL-derived subdir with safePathInsideRoot so a ../ subdir can't copy out of the shared clone cache; run cleanup() if the terminal copyDir throws so the temp working dir never leaks. - install.mjs: reject unknown --channel values (e.g. a 'stabl' typo) instead of silently treating them as the 'next' default. - remove.mjs / update.mjs: containment-check manifest/CLI-derived paths before destructive fs.rm / atomic swap, reusing safePathInsideRoot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ds cleanup paths - atomicSwapDir: back the existing target up to a sibling before swapping the staged dir in, and roll back on a failed rename so an interrupted update can no longer leave neither the old nor the new install. - ide-sync cleanup: only remove _bmad/ skill source dirs when EVERY IDE synced successfully (so failed targets remain retryable), and containment-check each CSV-derived path before fs.remove so a malformed row can't escape _bmad/. - module-dirs: validate untrusted wds_folders entries for traversal/absolute escape before mkdir. - Regenerate the vendored ide-sync bundle (vendor:check clean). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n, module resolution - channel-resolver: strip a trailing slash before `.git` so `…/r.git/` resolves to repo `r` (was `r.git`, which broke stable-tag lookup). - config-gen: drop orphaned [agents.*] blocks owned by this module (module= fallback) on regenerate, not only those still in the current module.yaml. - install-plan: only honor file sources for the fixed-file Claude surfaces (hooks/mcpServers/lspServers/settings) so the rewritten manifest never points at an uncopied file; anchor customize.schemas rewrites on the owning skill dir so nested schema paths survive. - cli: reject unknown flags with a usage error instead of running with defaults. - project-root: prefer a resolution's exact moduleYamlPath over the first module.yaml found under the repo root (multi-module repos). - official-modules: read the flattened _bmad/<code>/module.yaml first so new-spec modules honor their declared working directories. - Add channel-resolver `.git/` regression cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ce resolve - bmad-module-lib: keep the legacy-fallback null return for malformed .claude-plugin/plugin.json, but warn so corruption is no longer indistinguishable from a missing file. - ui: in the non-interactive custom-source path, collect resolution failures and throw after attempting every source (and when a source yields no module), so a CI/--custom-source install fails instead of silently omitting the module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test:ide-sync is part of the local `quality`/`test` scripts but was absent from the CI validate job, letting ide-sync regressions merge undetected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document the bmad-module command (install/update/remove/list), every flag, channel semantics, examples, and exit codes under docs/reference/. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- frontmatter: accept a block that ends at EOF (optional trailing newline). - legacy-resolver: accept CRLF frontmatter delimiters. - integration.test.sh: unique mktemp stderr capture, explicit if/then/else assertions (drop the SC2015 && ok || ko chains), plus unknown-flag and invalid-channel usage-error cases. - test-installation-components: clear the resolution cache in a finally. - acme-devlog fixtures: correct the uninstall note to the flat TOML layout, replace `ls -t | head` with a glob/-nt lookup, drop the always-on devlog config file: fact, "run" -> "invoke" wording, and reconcile the devlog-write template contract with the bundled template. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
CodeRabbit review follow-up — full passWent through the entire review (21 Major / 7 Minor / 2 Nitpick + the critical inline set), verifying each finding against current code before acting. Pushed in 6 logical commits ( FixedSecurity / data-loss (Major)
Correctness (Major)
Behavior (per maintainer decision)
CI / Docs / Minors / Nitpicks
Intentionally not changed (with rationale)
Validation
|
Contributor
Author
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core-skills/bmad-module/scripts/lib/fs-safe.mjs`:
- Around line 123-129: In the catch block that handles errors during the rename
operation (line 128-130), the code currently deletes both sibling and backup
directories. This causes data loss if rename fails and rollback also fails, as
backup becomes the only recoverable previous installation. Remove the fsp.rm
call that deletes the backup directory from this catch block, preserving backup
as a recovery point while still cleaning up the failed sibling directory.
🪄 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: 7f788be1-68ec-4919-8f2b-7c5873a11c67
📒 Files selected for processing (29)
.github/workflows/quality.yamldocs/reference/bmad-module-cli.mdsrc/core-skills/bmad-module/scripts/cli.mjssrc/core-skills/bmad-module/scripts/install.mjssrc/core-skills/bmad-module/scripts/lib/channel-resolver.mjssrc/core-skills/bmad-module/scripts/lib/config-gen.mjssrc/core-skills/bmad-module/scripts/lib/frontmatter.mjssrc/core-skills/bmad-module/scripts/lib/fs-safe.mjssrc/core-skills/bmad-module/scripts/lib/ide-sync.mjssrc/core-skills/bmad-module/scripts/lib/install-plan.mjssrc/core-skills/bmad-module/scripts/lib/legacy-resolver.mjssrc/core-skills/bmad-module/scripts/lib/module-dirs.mjssrc/core-skills/bmad-module/scripts/lib/source.mjssrc/core-skills/bmad-module/scripts/lib/vendor/ide-sync.mjssrc/core-skills/bmad-module/scripts/remove.mjssrc/core-skills/bmad-module/scripts/update.mjssrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/README.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/scripts/fetch-git-history.shsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-agent-historian/customize.tomlsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-summarize/SKILL.mdsrc/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-write/SKILL.mdsrc/core-skills/bmad-module/tests/integration.test.shtest/test-bmad-module-source.mjstest/test-installation-components.jstools/installer/core/ide-sync.jstools/installer/modules/bmad-module-lib.jstools/installer/modules/official-modules.jstools/installer/project-root.jstools/installer/ui.js
✅ Files skipped from review due to trivial changes (3)
- src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-agent-historian/customize.toml
- src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/README.md
- src/core-skills/bmad-module/scripts/install.mjs
🚧 Files skipped from review as they are similar to previous changes (20)
- src/core-skills/bmad-module/scripts/lib/frontmatter.mjs
- src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-write/SKILL.md
- src/core-skills/bmad-module/scripts/lib/ide-sync.mjs
- test/test-bmad-module-source.mjs
- src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/scripts/fetch-git-history.sh
- tools/installer/project-root.js
- tools/installer/modules/bmad-module-lib.js
- src/core-skills/bmad-module/scripts/cli.mjs
- src/core-skills/bmad-module/tests/fixtures/examples/comprehensive/acme-devlog/skills/bmad-devlog-summarize/SKILL.md
- src/core-skills/bmad-module/scripts/lib/module-dirs.mjs
- src/core-skills/bmad-module/scripts/lib/legacy-resolver.mjs
- src/core-skills/bmad-module/scripts/remove.mjs
- src/core-skills/bmad-module/scripts/lib/source.mjs
- src/core-skills/bmad-module/scripts/update.mjs
- test/test-installation-components.js
- src/core-skills/bmad-module/scripts/lib/config-gen.mjs
- tools/installer/core/ide-sync.js
- src/core-skills/bmad-module/scripts/lib/channel-resolver.mjs
- src/core-skills/bmad-module/scripts/lib/install-plan.mjs
- src/core-skills/bmad-module/tests/integration.test.sh
| return; | ||
| } | ||
|
|
||
| const manifest = await readAndValidateManifest(materialized.dir); |
There was a problem hiding this comment.
readAndValidateManifest requires a .claude-plugin/plugin.json with a bmad{} block. Legacy modules (installed from marketplace.json) don't have one, so this call throws BAD_MANIFEST (exit 20) — making every legacy-installed community module un-updatable. The install path handles both formats (lines 45–59 of install.mjs via hasBmadPluginJson / resolveLegacyModule), but update is missing the equivalent branch.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Contributor
Author
There was a problem hiding this comment.
Valid — fixed in ce314ba. updateOne now mirrors install's format-aware branch: hasBmadPluginJson → readAndValidateManifest, else resolveLegacyModule + validateManifestObject({allowReserved}) + synthesized module.yaml/module-help.csv staging before the copy plan. Legacy marketplace.json modules are now updatable.
| const argv = process.argv.slice(2); | ||
| if (argv.length === 0 || argv[0] === '--help' || argv[0] === '-h') { | ||
| printUsage(); | ||
| process.exit(EXIT.USAGE); |
There was a problem hiding this comment.
--help / -h exits with EXIT.USAGE (code 2), which CI and shell tooling interpret as an error. The conventional behavior is exit 0 for a successful help invocation; exit 2 should be reserved for actual usage mistakes (missing args, bad flags).
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Contributor
Author
There was a problem hiding this comment.
Valid — fixed in ce314ba. Explicit --help/-h now exits 0; a bare no-args invocation keeps EXIT.USAGE (2), since an incomplete call is still a usage error.
| if (s.startsWith('"') && s.endsWith('"')) { | ||
| return s | ||
| .slice(1, -1) | ||
| .replaceAll('\\n', '\n') |
There was a problem hiding this comment.
The replaceAll unescape chain processes \\n → \ + newline (wrong) instead of \ + n (correct), because \n is replaced before \\. A value containing a literal backslash followed by n (e.g. a Windows path segment \new) would be silently corrupted on round-trip. Consider processing escapes in a single left-to-right pass, or at minimum swap the \\ replacement to run before the escape-sequence replacements.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Contributor
Author
There was a problem hiding this comment.
Valid — fixed in ce314ba. Replaced the replaceAll chain with a single left-to-right pass that consumes each escape once (true inverse of formatTomlValue), so \new round-trips intact. Note: the suggested 'swap \ to run first' alternative is also incorrect — \\new → \new → then \n→newline still corrupts it; only the single pass is correct. Added round-trip regression tests in test/test-bmad-module-source.mjs.
… exit, TOML unescape - fs-safe: keep the backup dir on a failed swap+rollback (it's the only surviving copy of the previous install) and report its path instead of deleting it. - update: handle legacy marketplace.json modules the same way install does (hasBmadPluginJson branch + resolveLegacyModule + synthesized staging) so legacy-installed community modules are no longer un-updatable. - cli: explicit --help/-h exits 0; bare no-args invocation keeps exit 2. - config-gen: unescape TOML scalars in a single left-to-right pass so a literal backslash before n/r/t (e.g. a Windows path \new) round-trips intact; export parseTomlScalar and add round-trip regression tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Print committed-vs-fresh lengths and the first differing window on a vendor:check mismatch so a CI-only drift (passes locally, fails in CI) is actionable from the log. Temporary diagnostic to locate the divergence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merge of main brought the hermes-agent IDE target (#2489) into tools/installer/ide/platform-codes.yaml. Regenerate the vendored sidecar so vendor:check matches the engine in the PR's merge-with-main tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
main #2493 (astro 6 upgrade) added `overrides: {esbuild: ^0.28.1}` to pin esbuild for a security fix. The vendored yaml.mjs and ide-sync.mjs bundles are byte-compared in CI against a fresh esbuild rebuild, so the embedded esbuild version must match. The branch merge of main brought the override but left the bundles built with 0.25.12; regenerated both with 0.28.1 so vendor:check passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| const PHASE_INDEX = 7; | ||
|
|
||
| // Top-level _bmad children that are not modules and must not be scanned. | ||
| const NON_MODULE_DIRS = new Set(['_config', '_memory', 'memory', 'docs', 'scripts', 'custom']); |
There was a problem hiding this comment.
NON_MODULE_DIRS excludes memory, docs, and scripts from the help-catalog scan, but these names are NOT in RESERVED_CODES (in plugin-json.mjs). A community module using bmad.code: "docs", "scripts", or "memory" would pass install-time validation, land at _bmad/<code>/, but have its module-help.csv entries silently dropped from bmad-help.csv. Consider adding these three to RESERVED_CODES, or removing them from NON_MODULE_DIRS.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| * (source: 'community' in manifest.yaml). Empty set if no manifest or none. | ||
| * @returns {Promise<Set<string>>} | ||
| */ | ||
| async _loadCommunityModuleCodes() { |
There was a problem hiding this comment.
_loadCommunityModuleCodes filters for source === 'community', but writeMainManifest (which runs first in generateManifests) rewrites every module entry via getModuleVersionInfo(). For a community-installed module with no resolution-cache entry in the current installer run, getModuleVersionInfo falls through to the "Unknown module" branch and returns source: 'unknown' — overwriting the original source: 'community' before writeSkillManifest/writeFilesManifest read it. This would cause the CSV row preservation to silently stop working after the first bmad install regeneration cycle.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
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
2 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
Adds
bmad-module, a core skill to install, update, remove, and list community/custom BMAD modules, and brings the installer internals to parity so the skill and thebmadCLI share one code path.Why
Community modules (standalone GitHub repos of skills/agents/assets) had no first-class install path from inside a project. This lets users add them the same way the installer does, distributed to whichever IDEs they selected at
bmad install.How
src/core-skills/bmad-module/skill with a self-contained vendored runtime (install/update/remove/list).plugin.json#bmadspec and the legacymarketplace.json+module.yamlformat into one on-disk layout.tools/installer/core/ide-sync.jsengine distributes/prunes skills to the assistants inmanifest.yaml; best-effortnpm install, config-block generation, andbmad-help.csvrebuild.Testing
npm testpasses in full — installer, ide-sync, and skill suites plus eslint, markdownlint, and prettier; vendored bundle regenerated andvendor:checkclean.