From d4f377f12d31d3d6c00c52b37fd90a424adac661 Mon Sep 17 00:00:00 2001 From: Dov Benyomin Sohacheski Date: Wed, 10 Jun 2026 05:14:54 +0300 Subject: [PATCH 1/2] fix: remove empty skill-group dirs left in _bmad after install Skill cleanup removed each skill's own directory but never pruned the now-empty grouping folders above it (e.g. _bmad/bmm/1-analysis), leaving empty dirs behind after every install. Walk up from each removed skill dir and drop empty parents, stopping at the bmad root. --- test/test-installation-components.js | 44 ++++++++++++++++++++++++++++ tools/installer/core/installer.js | 18 ++++++++++++ 2 files changed, 62 insertions(+) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index d5d1a6e62..177fca166 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -3273,6 +3273,50 @@ async function runTests() { console.log(''); + // ============================================================ + // Test Suite 45: _cleanupSkillDirs prunes empty parent dirs (#empty-bmm-folders) + // ============================================================ + console.log(`${colors.yellow}Test Suite 45: cleanup prunes empty skill-group dirs${colors.reset}\n`); + + try { + const root45 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-cleanup-test-')); + const bmadDir45 = path.join(root45, '_bmad'); + await fs.ensureDir(path.join(bmadDir45, '_config')); + + // Two skills nested under the same grouping dir (1-analysis), plus a + // module-level file that must survive the cleanup. + await fs.writeFile( + path.join(bmadDir45, '_config', 'skill-manifest.csv'), + [ + 'canonicalId,name,description,module,path', + '"bmad-agent-analyst","bmad-agent-analyst","fixture","bmm","_bmad/bmm/1-analysis/bmad-agent-analyst/SKILL.md"', + '"bmad-research","bmad-research","fixture","bmm","_bmad/bmm/1-analysis/research/bmad-research/SKILL.md"', + '', + ].join('\n'), + ); + await fs.ensureDir(path.join(bmadDir45, 'bmm', '1-analysis', 'bmad-agent-analyst')); + await fs.writeFile(path.join(bmadDir45, 'bmm', '1-analysis', 'bmad-agent-analyst', 'SKILL.md'), 'x'); + await fs.ensureDir(path.join(bmadDir45, 'bmm', '1-analysis', 'research', 'bmad-research')); + await fs.writeFile(path.join(bmadDir45, 'bmm', '1-analysis', 'research', 'bmad-research', 'SKILL.md'), 'x'); + await fs.writeFile(path.join(bmadDir45, 'bmm', 'config.yaml'), 'module: bmm\n'); + + const installer45 = new Installer(); + await installer45._cleanupSkillDirs(bmadDir45); + + assert(!(await fs.pathExists(path.join(bmadDir45, 'bmm', '1-analysis'))), 'empty skill-group dir is pruned after cleanup'); + assert(!(await fs.pathExists(path.join(bmadDir45, 'bmm', '1-analysis', 'research'))), 'empty nested skill-group dir is pruned'); + assert(await fs.pathExists(path.join(bmadDir45, 'bmm', 'config.yaml')), 'module-level files are preserved'); + assert(await fs.pathExists(bmadDir45), 'bmad root is never removed'); + + await fs.remove(root45); + } catch (error) { + console.log(`${colors.red}Test Suite 45 setup failed: ${error.message}${colors.reset}`); + console.log(error.stack); + failed++; + } + + console.log(''); + // ============================================================ // Summary // ============================================================ diff --git a/tools/installer/core/installer.js b/tools/installer/core/installer.js index 9347e1f0b..4898873b6 100644 --- a/tools/installer/core/installer.js +++ b/tools/installer/core/installer.js @@ -419,10 +419,28 @@ class Installer { const sourceDir = path.dirname(path.join(bmadDir, relativePath)); if (await fs.pathExists(sourceDir)) { await fs.remove(sourceDir); + await this._removeEmptyParents(path.dirname(sourceDir), bmadDir); } } } + /** + * Remove now-empty parent directories left behind after skill dir cleanup. + * Walks up from dir, stopping at (and never removing) bmadDir. + * @param {string} dir - Directory to start walking up from + * @param {string} bmadDir - BMAD installation directory (boundary) + */ + async _removeEmptyParents(dir, bmadDir) { + let current = dir; + while (current.startsWith(bmadDir) && current !== bmadDir) { + if (!(await fs.pathExists(current))) break; + const entries = await fs.readdir(current); + if (entries.length > 0) break; + await fs.rmdir(current); + current = path.dirname(current); + } + } + async _readSkillManifestRows(bmadDir) { const csvPath = path.join(bmadDir, '_config', 'skill-manifest.csv'); if (!(await fs.pathExists(csvPath))) return []; From 09f043c51e022ad152885b845430d45abc270183 Mon Sep 17 00:00:00 2001 From: Dov Benyomin Sohacheski Date: Wed, 10 Jun 2026 05:57:43 +0300 Subject: [PATCH 2/2] fix: harden empty-parent pruning boundary and cleanup Use a path-boundary check instead of a string prefix so sibling dirs (e.g. _bmad2) can't match the bmad root, and make the walk best-effort so a dir that vanishes or fills in mid-walk never aborts the install. Move the test fixture cleanup into finally so failures don't leak temp dirs. --- test/test-installation-components.js | 7 ++++--- tools/installer/core/installer.js | 19 +++++++++++++------ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 177fca166..6e015322a 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -3278,8 +3278,9 @@ async function runTests() { // ============================================================ console.log(`${colors.yellow}Test Suite 45: cleanup prunes empty skill-group dirs${colors.reset}\n`); + let root45; try { - const root45 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-cleanup-test-')); + root45 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-cleanup-test-')); const bmadDir45 = path.join(root45, '_bmad'); await fs.ensureDir(path.join(bmadDir45, '_config')); @@ -3307,12 +3308,12 @@ async function runTests() { assert(!(await fs.pathExists(path.join(bmadDir45, 'bmm', '1-analysis', 'research'))), 'empty nested skill-group dir is pruned'); assert(await fs.pathExists(path.join(bmadDir45, 'bmm', 'config.yaml')), 'module-level files are preserved'); assert(await fs.pathExists(bmadDir45), 'bmad root is never removed'); - - await fs.remove(root45); } catch (error) { console.log(`${colors.red}Test Suite 45 setup failed: ${error.message}${colors.reset}`); console.log(error.stack); failed++; + } finally { + if (root45) await fs.remove(root45).catch(() => {}); } console.log(''); diff --git a/tools/installer/core/installer.js b/tools/installer/core/installer.js index 4898873b6..df9955f40 100644 --- a/tools/installer/core/installer.js +++ b/tools/installer/core/installer.js @@ -426,17 +426,24 @@ class Installer { /** * Remove now-empty parent directories left behind after skill dir cleanup. - * Walks up from dir, stopping at (and never removing) bmadDir. + * Walks up from dir, stopping at (and never removing) bmadDir. Best-effort: + * a directory that vanishes or fills in mid-walk just ends the walk. * @param {string} dir - Directory to start walking up from * @param {string} bmadDir - BMAD installation directory (boundary) */ async _removeEmptyParents(dir, bmadDir) { let current = dir; - while (current.startsWith(bmadDir) && current !== bmadDir) { - if (!(await fs.pathExists(current))) break; - const entries = await fs.readdir(current); - if (entries.length > 0) break; - await fs.rmdir(current); + while (true) { + // Path-boundary check (not a string prefix, so siblings like _bmad2 don't match). + const rel = path.relative(bmadDir, current); + if (rel === '' || rel.startsWith('..') || path.isAbsolute(rel)) break; + try { + const entries = await fs.readdir(current); + if (entries.length > 0) break; + await fs.rmdir(current); + } catch { + break; + } current = path.dirname(current); } }