From c845e78aabfb7d041d5964806d4a16c00bb2be9d Mon Sep 17 00:00:00 2001 From: pbean Date: Sat, 20 Jun 2026 15:58:13 -0700 Subject: [PATCH] =?UTF-8?q?fix(bmad-module):=20correctness=20=E2=80=94=20c?= =?UTF-8?q?hannel/.git,=20config-gen,=20install-plan,=20module=20resolutio?= =?UTF-8?q?n?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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//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 --- src/core-skills/bmad-module/scripts/cli.mjs | 9 ++++- .../scripts/lib/channel-resolver.mjs | 4 +- .../bmad-module/scripts/lib/config-gen.mjs | 16 ++++++-- .../bmad-module/scripts/lib/ide-sync.mjs | 13 +++++++ .../bmad-module/scripts/lib/install-plan.mjs | 37 +++++++++++-------- test/test-bmad-module-source.mjs | 2 + tools/installer/modules/official-modules.js | 36 +++++++++++------- tools/installer/project-root.js | 9 ++++- 8 files changed, 89 insertions(+), 37 deletions(-) diff --git a/src/core-skills/bmad-module/scripts/cli.mjs b/src/core-skills/bmad-module/scripts/cli.mjs index 293d7f49b..eeac451ce 100644 --- a/src/core-skills/bmad-module/scripts/cli.mjs +++ b/src/core-skills/bmad-module/scripts/cli.mjs @@ -18,6 +18,8 @@ import { runList } from './list.mjs'; import { EXIT, BmadModuleError } from './lib/exit.mjs'; 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']); function parseArgs(argv) { const out = { _: [], flags: {} }; @@ -26,8 +28,13 @@ function parseArgs(argv) { const a = argv[i]; if (a.startsWith('--')) { const key = a.slice(2); + // Reject unknown flags so typos fail fast instead of silently running + // with defaults. + 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; diff --git a/src/core-skills/bmad-module/scripts/lib/channel-resolver.mjs b/src/core-skills/bmad-module/scripts/lib/channel-resolver.mjs index ee37f0631..d89d37573 100644 --- a/src/core-skills/bmad-module/scripts/lib/channel-resolver.mjs +++ b/src/core-skills/bmad-module/scripts/lib/channel-resolver.mjs @@ -24,8 +24,8 @@ export function parseGitHubRepo(url) { if (!url || typeof url !== 'string') return null; const trimmed = url .trim() - .replace(/\.git$/, '') - .replace(/\/$/, ''); + .replace(/\/+$/, '') + .replace(/\.git$/, ''); const httpsMatch = trimmed.match(/^https?:\/\/github\.com\/([^/]+)\/([^/]+)(?:\/.*)?$/i); if (httpsMatch) return { owner: httpsMatch[1], repo: httpsMatch[2] }; const sshMatch = trimmed.match(/^git@github\.com:([^/]+)\/([^/]+)$/i); diff --git a/src/core-skills/bmad-module/scripts/lib/config-gen.mjs b/src/core-skills/bmad-module/scripts/lib/config-gen.mjs index a735976d8..de52f3756 100644 --- a/src/core-skills/bmad-module/scripts/lib/config-gen.mjs +++ b/src/core-skills/bmad-module/scripts/lib/config-gen.mjs @@ -249,10 +249,18 @@ export async function regenerateCentralConfig(bmadDir, code, opts = {}) { { const base = teamContent || TEAM_HEADER.join('\n') + '\n'; const { preamble, blocks } = splitBlocks(base); - // Drop this module's prior [modules.] and its [agents.*] (by code). - const kept = blocks.filter( - (b) => b.header !== `modules.${sectionKey}` && !(b.header.startsWith('agents.') && agentCodes.has(b.header.slice('agents.'.length))), - ); + // Drop this module's prior [modules.] and its [agents.*] blocks. Match + // current agent codes AND, as a fallback for removed/renamed agents (or a + // missing module.yaml that leaves agentCodes empty), any [agents.*] block + // whose `module = ""` line marks it as owned by this module. + const kept = blocks.filter((b) => { + if (b.header === `modules.${sectionKey}` || b.header === `modules.${code}`) return false; + if (b.header.startsWith('agents.')) { + if (agentCodes.has(b.header.slice('agents.'.length))) return false; + if (b.lines.some((l) => /^module\s*=/.test(l) && parseTomlScalar(l.split('=').slice(1).join('=')) === code)) return false; + } + return true; + }); if (Object.keys(teamKv).length) kept.push(renderModuleBlock(sectionKey, teamKv)); for (const a of agents) kept.push(renderAgentBlock(a)); await fs.writeFile(teamPath, joinFile(preamble, kept), 'utf8'); diff --git a/src/core-skills/bmad-module/scripts/lib/ide-sync.mjs b/src/core-skills/bmad-module/scripts/lib/ide-sync.mjs index b7832b613..9df4f192d 100644 --- a/src/core-skills/bmad-module/scripts/lib/ide-sync.mjs +++ b/src/core-skills/bmad-module/scripts/lib/ide-sync.mjs @@ -1,5 +1,6 @@ import { spawn } from 'node:child_process'; import { existsSync } from 'node:fs'; +import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { readManifestYaml } from './manifest-ops.mjs'; @@ -20,6 +21,18 @@ import { readManifestYaml } from './manifest-ops.mjs'; // (the _bmad/ write already succeeded). export async function distributeToIdes({ projectDir, bmadDir, prune = [] }) { const manifest = await readManifestYaml(bmadDir); + // readManifestYaml returns null for BOTH a missing manifest and a parse + // failure. A present-but-unreadable manifest is real config corruption — don't + // silently skip distribution; surface a repair hint. A genuinely absent + // manifest falls through to the "nothing configured" skip below. + if (manifest === null && existsSync(path.join(bmadDir, '_config', 'manifest.yaml'))) { + return { + ok: false, + hint: + 'Could not read _bmad/_config/manifest.yaml (invalid YAML). Run `bmad install` to repair BMAD config, ' + + 'then `bmad ide-sync` to push skills to your coding assistants.', + }; + } const ides = Array.isArray(manifest?.ides) ? manifest.ides.filter((i) => i && typeof i === 'string') : []; if (ides.length === 0) { return { skipped: true }; diff --git a/src/core-skills/bmad-module/scripts/lib/install-plan.mjs b/src/core-skills/bmad-module/scripts/lib/install-plan.mjs index 8627e132d..27c4fd46d 100644 --- a/src/core-skills/bmad-module/scripts/lib/install-plan.mjs +++ b/src/core-skills/bmad-module/scripts/lib/install-plan.mjs @@ -310,14 +310,14 @@ export async function buildCopyPlan(sourceDir, manifest, ignoreMatch) { if (typeof v !== 'string') continue; const srcRel = stripDotSlash(v); if (!srcRel) continue; - // If the declared path is a directory, copy it under its basename. + // These surfaces are single JSON files installed at a fixed canonical name + // (rewriteManifestPaths always points the manifest at `./hooks.json`, + // `./.mcp.json`, etc.). A directory source would be copied under its + // basename yet leave the manifest pointing at a file that was never written, + // so only file sources are honored here. try { const stat = await fs.stat(path.join(sourceDir, srcRel)); - if (stat.isDirectory()) { - await addDirRecursive(srcRel, path.posix.basename(srcRel)); - } else if (stat.isFile()) { - addFile(srcRel, destName); - } + if (stat.isFile()) addFile(srcRel, destName); } catch { /* missing — skip */ } @@ -363,21 +363,28 @@ export function rewriteManifestPaths(manifest) { if (typeof out.bmad.moduleDefinition === 'string') out.bmad.moduleDefinition = './module.yaml'; if (typeof out.bmad.moduleHelpCsv === 'string') out.bmad.moduleHelpCsv = './module-help.csv'; - // customize.schemas — each entry lives inside its skill dir; the skill dir - // itself is remapped to `skills/`, so the schema's new path is - // `./skills//`. + // customize.schemas — each entry lives inside a declared skill dir, which is + // remapped to `skills/`. Anchor on the owning skill dir and keep + // every segment after it, so nested schemas (e.g. `/schemas/x.yaml`) + // land under the right skill instead of being collapsed to the last two + // segments. + const skillDirs = Array.isArray(manifest.skills) + ? manifest.skills.filter((s) => typeof s === 'string').map((s) => stripDotSlash(s)) + : []; const schemas = out.bmad.customize?.schemas; if (Array.isArray(schemas)) { out.bmad.customize.schemas = schemas.map((entry) => { if (typeof entry !== 'string') return entry; const srcRel = stripDotSlash(entry); - const parts = srcRel.split('/'); - // Heuristic: last two segments are `/`. - if (parts.length >= 2) { - const file = parts.at(-1); - const skill = parts.at(-2); - return `./skills/${skill}/${file}`; + const owner = skillDirs.find((sd) => sd && (srcRel === sd || srcRel.startsWith(sd + '/'))); + if (owner) { + const remainder = srcRel.slice(owner.length + 1); + return `./skills/${path.posix.basename(owner)}/${remainder}`; } + // Fallback when no declared skill owns the path: last two segments are + // assumed to be `/`. + const parts = srcRel.split('/'); + if (parts.length >= 2) return `./skills/${parts.at(-2)}/${parts.at(-1)}`; return `./${srcRel}`; }); } diff --git a/test/test-bmad-module-source.mjs b/test/test-bmad-module-source.mjs index 9c0cdf16e..018f5c99e 100644 --- a/test/test-bmad-module-source.mjs +++ b/test/test-bmad-module-source.mjs @@ -144,6 +144,8 @@ console.log(`\n${colors.cyan}channel-resolver${colors.reset}\n`); eq(parseGitHubRepo('https://github.com/o/r/tree/main'), { owner: 'o', repo: 'r' }, 'parseGitHubRepo from deep URL'); eq(parseGitHubRepo('git@github.com:o/r'), { owner: 'o', repo: 'r' }, 'parseGitHubRepo from SSH'); eq(parseGitHubRepo('https://gitlab.com/o/r'), null, 'parseGitHubRepo null for non-GitHub'); +eq(parseGitHubRepo('https://github.com/o/r.git'), { owner: 'o', repo: 'r' }, 'parseGitHubRepo strips .git'); +eq(parseGitHubRepo('https://github.com/o/r.git/'), { owner: 'o', repo: 'r' }, 'parseGitHubRepo strips .git before trailing slash'); eq( [normalizeStableTag('v1.7.0'), normalizeStableTag('1.0.0-rc.1'), normalizeStableTag('nope')], ['1.7.0', null, null], diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js index 3f71796de..2d701879d 100644 --- a/tools/installer/modules/official-modules.js +++ b/tools/installer/modules/official-modules.js @@ -694,21 +694,29 @@ class OfficialModules { const projectRoot = path.dirname(bmadDir); const emptyResult = { createdDirs: [], movedDirs: [], createdWdsFolders: [] }; - // 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'); + // Prefer the flattened installed module.yaml. buildCopyPlan() copies a + // new-spec module's moduleDefinition to _bmad//module.yaml, where it + // carries the canonical `directories` declarations — but its source tree may + // keep module.yaml under a skill asset path that findModuleSource() can't + // locate, which would otherwise skip the declared working dirs. + let moduleYamlPath = path.join(bmadDir, moduleName, 'module.yaml'); if (!(await fs.pathExists(moduleYamlPath))) { - return emptyResult; // No module.yaml, skip + // 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 + moduleYamlPath = path.join(sourcePath, 'module.yaml'); + if (!(await fs.pathExists(moduleYamlPath))) { + return emptyResult; // No module.yaml, skip + } } let moduleYaml; diff --git a/tools/installer/project-root.js b/tools/installer/project-root.js index ae12a3f13..f101608bc 100644 --- a/tools/installer/project-root.js +++ b/tools/installer/project-root.js @@ -176,7 +176,14 @@ async function resolveInstalledModuleYaml(moduleName) { // from its marketplace plugin name (e.g. bmad-creative-intelligence-suite) // can be tracked downstream under any of the three — match all of them. const matches = mod.code === moduleName || mod.name === moduleName || mod.pluginName === moduleName; - if (matches && mod.localPath) { + if (!matches) continue; + // Prefer the resolution's exact module.yaml — searchRoot(localPath) returns + // the FIRST module.yaml under the root, which can be the wrong one in a + // multi-module/multi-plugin repo resolved by pluginName. + if (mod.moduleYamlPath && (await fs.pathExists(mod.moduleYamlPath))) { + return mod.moduleYamlPath; + } + if (mod.localPath) { const found = await searchRoot(mod.localPath); if (found) return found; }