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.
This commit is contained in:
Dov Benyomin Sohacheski 2026-06-10 05:57:43 +03:00
parent d4f377f12d
commit 09f043c51e
No known key found for this signature in database
2 changed files with 17 additions and 9 deletions

View File

@ -3278,8 +3278,9 @@ async function runTests() {
// ============================================================ // ============================================================
console.log(`${colors.yellow}Test Suite 45: cleanup prunes empty skill-group dirs${colors.reset}\n`); console.log(`${colors.yellow}Test Suite 45: cleanup prunes empty skill-group dirs${colors.reset}\n`);
let root45;
try { 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'); const bmadDir45 = path.join(root45, '_bmad');
await fs.ensureDir(path.join(bmadDir45, '_config')); 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', '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(path.join(bmadDir45, 'bmm', 'config.yaml')), 'module-level files are preserved');
assert(await fs.pathExists(bmadDir45), 'bmad root is never removed'); assert(await fs.pathExists(bmadDir45), 'bmad root is never removed');
await fs.remove(root45);
} catch (error) { } catch (error) {
console.log(`${colors.red}Test Suite 45 setup failed: ${error.message}${colors.reset}`); console.log(`${colors.red}Test Suite 45 setup failed: ${error.message}${colors.reset}`);
console.log(error.stack); console.log(error.stack);
failed++; failed++;
} finally {
if (root45) await fs.remove(root45).catch(() => {});
} }
console.log(''); console.log('');

View File

@ -426,17 +426,24 @@ class Installer {
/** /**
* Remove now-empty parent directories left behind after skill dir cleanup. * 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} dir - Directory to start walking up from
* @param {string} bmadDir - BMAD installation directory (boundary) * @param {string} bmadDir - BMAD installation directory (boundary)
*/ */
async _removeEmptyParents(dir, bmadDir) { async _removeEmptyParents(dir, bmadDir) {
let current = dir; let current = dir;
while (current.startsWith(bmadDir) && current !== bmadDir) { while (true) {
if (!(await fs.pathExists(current))) break; // Path-boundary check (not a string prefix, so siblings like _bmad2 don't match).
const entries = await fs.readdir(current); const rel = path.relative(bmadDir, current);
if (entries.length > 0) break; if (rel === '' || rel.startsWith('..') || path.isAbsolute(rel)) break;
await fs.rmdir(current); try {
const entries = await fs.readdir(current);
if (entries.length > 0) break;
await fs.rmdir(current);
} catch {
break;
}
current = path.dirname(current); current = path.dirname(current);
} }
} }