diff --git a/src/commands/bar/install-subcommand.ts b/src/commands/bar/install-subcommand.ts index cc3cd068..af5b2670 100644 --- a/src/commands/bar/install-subcommand.ts +++ b/src/commands/bar/install-subcommand.ts @@ -513,10 +513,58 @@ export async function handleBarInstall( const { downloadUrl } = releaseInfo; console.log(`[i] Installing CCS Bar from ${BAR_RELEASE_TAG}...`); - // 1b. Stale reinstall guard: remove the existing bundle BEFORE extraction so a - // malformed archive that lacks the expected bundle cannot silently leave the - // old version in place. The removal window is intentionally small — release - // info is already resolved so we are committed to downloading. + // 1b. Stage-then-swap: extract into a hidden staging dir on the same filesystem + // so the rename to appPath is atomic-ish. The old bundle is only removed + // AFTER the new one is verified in staging — a transient download/extraction + // failure during reinstall therefore leaves the previous working install intact. + fs.mkdirSync(appsDir, { recursive: true }); + let stagingDir: string; + try { + stagingDir = fs.mkdtempSync(path.join(appsDir, '.ccs-bar-staging-')); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`[X] Could not create staging directory in ${appsDir}: ${msg}`); + return; + } + + // Helper: remove staging dir, ignoring errors (best-effort cleanup). + function cleanupStaging(): void { + try { + fs.rmSync(stagingDir, { recursive: true, force: true }); + } catch { + /* ignore */ + } + } + + // 2. Download and extract into staging, NOT appsDir. + try { + await downloadAndExtract(downloadUrl, stagingDir); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`[X] Download or extraction failed: ${msg}`); + cleanupStaging(); + return; + } + + // 3. Finding #12: assert that the expected .app bundle was actually extracted + // into the staging dir before touching the old install. + const stagedApp = path.join(stagingDir, BAR_APP_NAME); + if (!fs.existsSync(stagedApp)) { + // Report what was actually extracted to help diagnose archive issues. + let extracted: string[] = []; + try { + extracted = fs.readdirSync(stagingDir); + } catch { + /* ignore */ + } + const found = extracted.length > 0 ? extracted.join(', ') : '(none)'; + console.error(`[X] Extraction succeeded but "${BAR_APP_NAME}" was not found in staging.`); + console.error(`[i] Files found in staging: ${found}`); + cleanupStaging(); + return; + } + + // 3b. New bundle verified in staging — now safe to remove the old install. if (fs.existsSync(appPath)) { try { removeExistingApp(appPath); @@ -524,34 +572,23 @@ export async function handleBarInstall( const msg = err instanceof Error ? err.message : String(err); console.error(`[X] Could not remove the existing CCS Bar.app: ${msg}`); console.error('[i] Close any running instance and try again.'); + cleanupStaging(); return; } } - // 2. Download and extract into ~/Applications. + // 3c. Atomic-ish rename: move staged bundle into the final location. try { - await downloadAndExtract(downloadUrl, appsDir); + fs.renameSync(stagedApp, appPath); } catch (err) { const msg = err instanceof Error ? err.message : String(err); - console.error(`[X] Download or extraction failed: ${msg}`); + console.error(`[X] Could not move staged app into place: ${msg}`); + cleanupStaging(); return; } - // 3. Finding #12: assert that the expected .app bundle was actually extracted - // before reporting success. - if (!fs.existsSync(appPath)) { - // Report what was actually extracted to help diagnose archive issues. - let extracted: string[] = []; - try { - extracted = fs.readdirSync(appsDir); - } catch { - /* ignore */ - } - const found = extracted.length > 0 ? extracted.join(', ') : '(none)'; - console.error(`[X] Extraction succeeded but "${BAR_APP_NAME}" was not found in ${appsDir}.`); - console.error(`[i] Files found in ${appsDir}: ${found}`); - return; - } + // Staging dir is now empty (bundle was moved out); remove it. + cleanupStaging(); // 4. Read the real version from the extracted bundle's Info.plist. const installedVersion = readAppBundleVersion(appPath); diff --git a/tests/unit/commands/bar-command.test.ts b/tests/unit/commands/bar-command.test.ts index bb27967a..4ad796f7 100644 --- a/tests/unit/commands/bar-command.test.ts +++ b/tests/unit/commands/bar-command.test.ts @@ -2767,10 +2767,10 @@ describe('bar install: whitespace-only CFBundleShortVersionString yields null (F }); // --------------------------------------------------------------------------- -// Review finding — stale reinstall: old bundle removed before extraction +// Data Loss finding — stage-then-swap: old bundle preserved on failure // --------------------------------------------------------------------------- -describe('bar install: stale reinstall guard — removeExistingApp (review finding)', () => { +describe('bar install: stage-then-swap safety (Data Loss finding)', () => { const FAKE_DOWNLOAD_URL = 'https://github.com/kaitranntt/ccs/releases/download/ccs-bar-latest/CCS-Bar.app.zip'; @@ -2792,36 +2792,134 @@ describe('bar install: stale reinstall guard — removeExistingApp (review findi }; } - it('removes old bundle before calling downloadAndExtract on reinstall', async () => { - // Pre-seed a fake CCS Bar.app so the already-installed path is exercised. + it('downloadAndExtract receives a staging dir inside appsDir, NOT appsDir itself', async () => { + // The new contract: downloadAndExtract dest is a hidden staging dir under appsDir, + // not appsDir directly. This keeps the old install untouched during download. const appsDir = path.join(tempHome, 'Applications'); - const appPath = path.join(appsDir, 'CCS Bar.app'); - fs.mkdirSync(appPath, { recursive: true }); - - let appExistedAtDownload = true; // pessimistic default + const destDirs: string[] = []; const { handleBarInstall } = await loadInstallSubcommand(); await handleBarInstall([], { ...baseDeps(appsDir), - // Default removeExistingApp (fs.rmSync) is used — it will remove the dir. downloadAndExtract: async (_url: string, dest: string) => { - // Assert: old bundle must be gone when extraction is invoked. - appExistedAtDownload = fs.existsSync(appPath); - // Place a fresh bundle so the post-extract assertion passes. + destDirs.push(dest); + // Create the expected bundle inside staging so the verify step passes. fs.mkdirSync(path.join(dest, 'CCS Bar.app'), { recursive: true }); }, }); - expect(appExistedAtDownload).toBe(false); + expect(destDirs).toHaveLength(1); + const dest = destDirs[0]; + // Must NOT equal appsDir + expect(dest).not.toBe(appsDir); + // Must be a child of appsDir (same filesystem for atomic rename) + expect(dest.startsWith(appsDir + path.sep)).toBe(true); + // Dotted prefix to minimise Finder noise + expect(path.basename(dest)).toMatch(/^\..+/); }); - it('aborts install and does NOT call downloadAndExtract when removeExistingApp throws', async () => { - // Pre-seed the old bundle so the removal path is triggered. + it('downloadAndExtract throws during reinstall: old bundle still present, removeExistingApp NOT called', async () => { + // Core data-loss guard: if download/extraction fails, the old working install + // must remain intact — removeExistingApp must never have been called. + const appsDir = path.join(tempHome, 'Applications'); + const appPath = path.join(appsDir, 'CCS Bar.app'); + // Pre-seed the existing install. + fs.mkdirSync(appPath, { recursive: true }); + fs.writeFileSync(path.join(appPath, 'sentinel'), 'old-install'); + + let removeCalled = false; + const { handleBarInstall } = await loadInstallSubcommand(); + + await handleBarInstall([], { + ...baseDeps(appsDir), + removeExistingApp: (_path: unknown) => { + removeCalled = true; + fs.rmSync(String(_path), { recursive: true, force: true }); + }, + downloadAndExtract: async (_url: string, _dest: string) => { + throw new Error('network timeout'); + }, + }); + + // removeExistingApp must NOT have been called — download failed before the swap. + expect(removeCalled).toBe(false); + // Old bundle must still be present. + expect(fs.existsSync(appPath)).toBe(true); + expect(fs.existsSync(path.join(appPath, 'sentinel'))).toBe(true); + + const allOutput = consoleOutput.join('\n'); + expect(allOutput).toMatch(/\[X\].*Download or extraction failed/i); + }); + + it('extracted archive missing the bundle: old bundle still present, install aborts with diagnostics', async () => { + // If the archive extracts successfully but does not contain CCS Bar.app, + // the old install must be left untouched and [X] with file listing printed. + const appsDir = path.join(tempHome, 'Applications'); + const appPath = path.join(appsDir, 'CCS Bar.app'); + fs.mkdirSync(appPath, { recursive: true }); + fs.writeFileSync(path.join(appPath, 'sentinel'), 'old-install'); + + let removeCalled = false; + const { handleBarInstall } = await loadInstallSubcommand(); + + await handleBarInstall([], { + ...baseDeps(appsDir), + removeExistingApp: (_path: unknown) => { + removeCalled = true; + fs.rmSync(String(_path), { recursive: true, force: true }); + }, + downloadAndExtract: async (_url: string, dest: string) => { + // Extraction produces a wrong-named artifact — CCS Bar.app absent. + fs.mkdirSync(dest, { recursive: true }); + fs.writeFileSync(path.join(dest, 'WrongName.app'), 'dummy'); + }, + }); + + // removeExistingApp must NOT have been called. + expect(removeCalled).toBe(false); + // Old bundle still present. + expect(fs.existsSync(appPath)).toBe(true); + + const allOutput = consoleOutput.join('\n'); + expect(allOutput).toMatch(/\[X\]/); + expect(allOutput).toMatch(/CCS Bar\.app/); + // Diagnostics: should list staging contents + expect(allOutput).toMatch(/WrongName\.app/); + }); + + it('success path: staging dir removed, app present at appPath', async () => { + const appsDir = path.join(tempHome, 'Applications'); + const appPath = path.join(appsDir, 'CCS Bar.app'); + let stagingDirUsed = ''; + const { handleBarInstall } = await loadInstallSubcommand(); + + await handleBarInstall([], { + ...baseDeps(appsDir), + downloadAndExtract: async (_url: string, dest: string) => { + stagingDirUsed = dest; + fs.mkdirSync(path.join(dest, 'CCS Bar.app'), { recursive: true }); + }, + }); + + // App at the final location. + expect(fs.existsSync(appPath)).toBe(true); + // Staging dir cleaned up. + if (stagingDirUsed) { + expect(fs.existsSync(stagingDirUsed)).toBe(false); + } + + const allOutput = consoleOutput.join('\n'); + expect(allOutput).toMatch(/\[OK\].*CCS Bar/); + expect(allOutput).not.toMatch(/\[X\]/); + }); + + it('aborts install and prints [X] when removeExistingApp throws (old app present = no swap)', async () => { + // removeExistingApp now runs AFTER staging verification. + // If it throws, the new bundle is still in staging (not yet moved); old app untouched. const appsDir = path.join(tempHome, 'Applications'); const appPath = path.join(appsDir, 'CCS Bar.app'); fs.mkdirSync(appPath, { recursive: true }); - let downloadCalled = false; const { handleBarInstall } = await loadInstallSubcommand(); await handleBarInstall([], { @@ -2829,13 +2927,11 @@ describe('bar install: stale reinstall guard — removeExistingApp (review findi removeExistingApp: (_path: unknown) => { throw new Error('Permission denied'); }, - downloadAndExtract: async (_url: string, _dest: string) => { - downloadCalled = true; + downloadAndExtract: async (_url: string, dest: string) => { + fs.mkdirSync(path.join(dest, 'CCS Bar.app'), { recursive: true }); }, }); - expect(downloadCalled).toBe(false); - const allOutput = consoleOutput.join('\n'); expect(allOutput).toMatch(/\[X\].*Could not remove.*CCS Bar\.app/); });