From bcf86f23308f569c29cd38ed96d3d6c412e7c142 Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Sat, 25 Apr 2026 21:05:22 -0500 Subject: [PATCH] fix(installer): setupBatch must not claim a shared target_dir on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the first platform's setup throws or returns success: false, the dedup map previously still recorded the claim with skillCount: 0, causing every peer sharing the target_dir to skip its install — leaving the dir empty/broken behind a cascade of misleading "shares with X" rows. Now the claim is only recorded when the install succeeded and wrote skills. On failure, the next peer becomes the new first writer and recovers. Adds Suite 40b regression test that monkey-patches cursor.setup to throw and verifies gemini still populates the shared dir. --- test/test-installation-components.js | 48 ++++++++++++++++++++++++++++ tools/installer/ide/manager.js | 8 ++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 21a467200..4827afcbf 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -2683,6 +2683,54 @@ async function runTests() { console.log(''); + // ============================================================ + // Test Suite 40b: setupBatch — failed first writer does not poison peers + // ============================================================ + console.log(`${colors.yellow}Test Suite 40b: setupBatch resilience to first-writer failure${colors.reset}\n`); + + try { + const tempProjectDir40b = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-batch-fail-')); + const installedBmadDir40b = await createTestBmadFixture(); + + const ideManager40b = new IdeManager(); + await ideManager40b.ensureInitialized(); + + // Force cursor's setup() to fail. With the bug, gemini would see the + // claimed target and skip — leaving .agents/skills/ empty. + const cursorHandler40b = ideManager40b.handlers.get('cursor'); + const originalSetup = cursorHandler40b.setup.bind(cursorHandler40b); + cursorHandler40b.setup = async () => { + throw new Error('Simulated cursor failure'); + }; + + const batchResults40b = await ideManager40b.setupBatch(['cursor', 'gemini'], tempProjectDir40b, installedBmadDir40b, { + silent: true, + selectedModules: ['core'], + }); + + // Restore so other tests aren't affected. + cursorHandler40b.setup = originalSetup; + + assert(batchResults40b[0].success === false, 'Cursor reports failure'); + assert(batchResults40b[1].success === true, 'Gemini still succeeds despite cursor failure'); + assert( + batchResults40b[1].handlerResult?.results?.sharedTargetHandledByPeer !== true, + 'Gemini does NOT skip its own write — it becomes the new first writer', + ); + + const sharedDir40b = path.join(tempProjectDir40b, '.agents', 'skills'); + const entries40b = await fs.readdir(sharedDir40b); + assert(entries40b.includes('bmad-master'), 'Shared dir is populated by gemini after cursor failure'); + + await fs.remove(tempProjectDir40b).catch(() => {}); + await fs.remove(path.dirname(installedBmadDir40b)).catch(() => {}); + } catch (error) { + console.log(`${colors.red}Test Suite 40b setup failed: ${error.message}${colors.reset}`); + failed++; + } + + console.log(''); + // ============================================================ // Test Suite 41: Custom-module skill ownership (non-bmad prefix) // ============================================================ diff --git a/tools/installer/ide/manager.js b/tools/installer/ide/manager.js index 0143319d8..6370e4f41 100644 --- a/tools/installer/ide/manager.js +++ b/tools/installer/ide/manager.js @@ -219,7 +219,13 @@ class IdeManager { if (target && !claim) { const writtenCount = result.handlerResult?.results?.skillDirectories || result.handlerResult?.results?.skills || 0; - claimedTargets.set(target, { firstIde: ideName, skillCount: writtenCount }); + // Only claim the target when the install actually succeeded and wrote skills. + // If the first platform fails (ancestor conflict, exception, etc.), leave the + // dir unclaimed so the next peer becomes the new first writer instead of + // silently skipping into a broken/empty target_dir. + if (result.success && writtenCount > 0) { + claimedTargets.set(target, { firstIde: ideName, skillCount: writtenCount }); + } } results.push(result); }