diff --git a/package.json b/package.json index abafdf0e..961be8c6 100644 --- a/package.json +++ b/package.json @@ -62,7 +62,7 @@ "default": "" }, "coder.binaryDestination": { - "markdownDescription": "The full path of the directory into which the Coder CLI will be downloaded. Defaults to the value of `CODER_BINARY_DESTINATION` if not set, otherwise the extension's global storage directory.", + "markdownDescription": "The path to the Coder CLI binary or the directory containing it. When set to a file path (e.g., `/usr/bin/coder`), the extension checks its version and downloads a replacement if it does not match the server (and downloads are enabled). When set to a directory, the extension looks for the CLI inside it (downloading if enabled). Defaults to the value of `CODER_BINARY_DESTINATION` if not set, otherwise the extension's global storage directory.", "type": "string", "default": "" }, diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 913abd02..9694a5e3 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -2,7 +2,7 @@ import globalAxios, { type AxiosInstance, type AxiosRequestConfig, } from "axios"; -import { createWriteStream, type WriteStream } from "node:fs"; +import { createWriteStream, type WriteStream, type Stats } from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; import prettyBytes from "pretty-bytes"; @@ -29,6 +29,10 @@ import type { Logger } from "../logging/logger"; import type { CliCredentialManager } from "./cliCredentialManager"; import type { PathResolver } from "./pathResolver"; +type ResolvedBinary = + | { binPath: string; stat: Stats; source: "file-path" | "directory" } + | { binPath: string; source: "not-found" }; + export class CliManager { private readonly binaryLock: BinaryLock; @@ -46,15 +50,51 @@ export class CliManager { */ public async locateBinary(url: string): Promise { const safeHostname = toSafeHost(url); - const binPath = path.join( - this.pathResolver.getBinaryCachePath(safeHostname), - cliUtils.name(), - ); - const stat = await cliUtils.stat(binPath); - if (!stat) { - throw new Error(`No CLI binary found at ${binPath}`); + const resolved = await this.resolveBinaryPath(safeHostname); + if (resolved.source === "not-found") { + throw new Error(`No CLI binary found at ${resolved.binPath}`); } - return binPath; + return resolved.binPath; + } + + /** + * Resolve the CLI binary path from the configured cache path. + * + * Returns "file-path" when the cache path is an existing file (checked for + * version match and updated if needed), "directory" when a binary was found + * inside the directory, or "not-found" with the platform-specific path for + * the caller to download into. + */ + private async resolveBinaryPath( + safeHostname: string, + ): Promise { + const cachePath = this.pathResolver.getBinaryCachePath(safeHostname); + const cacheStat = await cliUtils.stat(cachePath); + + if (cacheStat?.isFile()) { + return { binPath: cachePath, stat: cacheStat, source: "file-path" }; + } + + const fullNamePath = path.join(cachePath, cliUtils.fullName()); + + // Path does not exist yet; return the platform-specific path to download. + if (!cacheStat) { + return { binPath: fullNamePath, source: "not-found" }; + } + + // Directory exists; check platform-specific name, then simple name. + const fullStat = await cliUtils.stat(fullNamePath); + if (fullStat) { + return { binPath: fullNamePath, stat: fullStat, source: "directory" }; + } + + const simpleNamePath = path.join(cachePath, cliUtils.simpleName()); + const simpleStat = await cliUtils.stat(simpleNamePath); + if (simpleStat) { + return { binPath: simpleNamePath, stat: simpleStat, source: "directory" }; + } + + return { binPath: fullNamePath, source: "not-found" }; } /** @@ -94,43 +134,40 @@ export class CliManager { ); } - // Check if there is an existing binary and whether it looks valid. If it - // is valid and matches the server, or if it does not match the server but - // downloads are disabled, we can return early. - const binPath = path.join( - this.pathResolver.getBinaryCachePath(safeHostname), - cliUtils.name(), + const resolved = await this.resolveBinaryPath(safeHostname); + this.output.debug( + `Resolved binary: ${resolved.binPath} (${resolved.source})`, ); - this.output.debug("Using binary path", binPath); - const stat = await cliUtils.stat(binPath); - if (stat === undefined) { - this.output.info("No existing binary found, starting download"); - } else { - this.output.debug("Existing binary size is", prettyBytes(stat.size)); + + // Check existing binary version when one was found. + if (resolved.source !== "not-found") { + this.output.debug( + "Existing binary size is", + prettyBytes(resolved.stat.size), + ); try { - const version = await cliVersion(binPath); + const version = await cliVersion(resolved.binPath); this.output.debug("Existing binary version is", version); - // If we have the right version we can avoid the request entirely. if (version === buildInfo.version) { - this.output.debug( - "Using existing binary since it matches the server version", - ); - return binPath; + this.output.debug("Existing binary matches server version"); + return resolved.binPath; } else if (!enableDownloads) { this.output.info( - "Using existing binary even though it does not match the server version because downloads are disabled", + "Using existing binary despite version mismatch because downloads are disabled", ); - return binPath; + return resolved.binPath; } this.output.info( "Downloading since existing binary does not match the server version", ); } catch (error) { this.output.warn( - "Unable to get version of existing binary. Downloading new binary instead", + "Unable to get version of existing binary, downloading instead", error, ); } + } else { + this.output.info("No existing binary found, starting download"); } if (!enableDownloads) { @@ -138,9 +175,15 @@ export class CliManager { throw new Error("Unable to download CLI because downloads are disabled"); } + // Always download using the platform-specific name. + const downloadBinPath = path.join( + path.dirname(resolved.binPath), + cliUtils.fullName(), + ); + // Create the `bin` folder if it doesn't exist - await fs.mkdir(path.dirname(binPath), { recursive: true }); - const progressLogPath = binPath + ".progress.log"; + await fs.mkdir(path.dirname(downloadBinPath), { recursive: true }); + const progressLogPath = downloadBinPath + ".progress.log"; let lockResult: | { release: () => Promise; waited: boolean } @@ -148,50 +191,71 @@ export class CliManager { let latestVersion = parsedVersion; try { lockResult = await this.binaryLock.acquireLockOrWait( - binPath, + downloadBinPath, progressLogPath, ); this.output.debug("Acquired download lock"); // If we waited for another process, re-check if binary is now ready + let needsDownload = true; if (lockResult.waited) { const latestBuildInfo = await restClient.getBuildInfo(); this.output.debug("Got latest server version", latestBuildInfo.version); const recheckAfterWait = await this.checkBinaryVersion( - binPath, + downloadBinPath, latestBuildInfo.version, ); if (recheckAfterWait.matches) { this.output.debug( "Using existing binary since it matches the latest server version", ); - return binPath; + needsDownload = false; + } else { + const latestParsedVersion = semver.parse(latestBuildInfo.version); + if (!latestParsedVersion) { + throw new Error( + `Got invalid version from deployment: ${latestBuildInfo.version}`, + ); + } + latestVersion = latestParsedVersion; } + } - // Parse the latest version for download - const latestParsedVersion = semver.parse(latestBuildInfo.version); - if (!latestParsedVersion) { - throw new Error( - `Got invalid version from deployment: ${latestBuildInfo.version}`, - ); - } - latestVersion = latestParsedVersion; + if (needsDownload) { + await this.performBinaryDownload( + restClient, + latestVersion, + downloadBinPath, + progressLogPath, + ); } - return await this.performBinaryDownload( - restClient, - latestVersion, - binPath, - progressLogPath, - ); + // Rename to user-configured file path while we hold the lock. + if ( + resolved.source === "file-path" && + downloadBinPath !== resolved.binPath + ) { + this.output.info( + "Renaming downloaded binary to", + path.basename(resolved.binPath), + ); + await fs.rename(downloadBinPath, resolved.binPath); + return resolved.binPath; + } + return downloadBinPath; } catch (error) { - // Unified error handling - check for fallback binaries and prompt user - return await this.handleAnyBinaryFailure( + const fallback = await this.handleAnyBinaryFailure( error, - binPath, + downloadBinPath, buildInfo.version, + resolved.binPath !== downloadBinPath ? resolved.binPath : undefined, ); + // Move the fallback to the expected path if needed. + if (fallback !== resolved.binPath) { + await fs.rename(fallback, resolved.binPath); + } + return resolved.binPath; } finally { if (lockResult) { await lockResult.release(); @@ -280,54 +344,59 @@ export class CliManager { } /** - * Unified handler for any binary-related failure. - * Checks for existing or old binaries and prompts user once. + * Try fallback binaries after a download failure, prompting the user once + * if the best candidate is a version mismatch. */ private async handleAnyBinaryFailure( error: unknown, binPath: string, expectedVersion: string, + fallbackBinPath?: string, ): Promise { const message = error instanceof cliUtils.FileLockError ? "Unable to update the Coder CLI binary because it's in use" : "Failed to update CLI binary"; - // Try existing binary first - const existingCheck = await this.checkBinaryVersion( - binPath, - expectedVersion, - ); - if (existingCheck.version) { - // Perfect match - use without prompting - if (existingCheck.matches) { - return binPath; + // Returns the path if usable, undefined if not found. + // Throws the original error if the user declines a mismatch. + const tryCandidate = async ( + candidate: string, + ): Promise => { + const check = await this.checkBinaryVersion(candidate, expectedVersion); + if (!check.version) { + return undefined; + } + if ( + !check.matches && + !(await this.promptUseExistingBinary(check.version, message)) + ) { + throw error; } - // Version mismatch - prompt user - if (await this.promptUseExistingBinary(existingCheck.version, message)) { - return binPath; + return candidate; + }; + + const primary = await tryCandidate(binPath); + if (primary) { + return primary; + } + + if (fallbackBinPath) { + const fallback = await tryCandidate(fallbackBinPath); + if (fallback) { + return fallback; } - throw error; } - // Try .old-* binaries as fallback + // Last resort: try the most recent .old-* backup. const oldBinaries = await cliUtils.findOldBinaries(binPath); if (oldBinaries.length > 0) { - const oldCheck = await this.checkBinaryVersion( - oldBinaries[0], - expectedVersion, - ); - if ( - oldCheck.version && - (oldCheck.matches || - (await this.promptUseExistingBinary(oldCheck.version, message))) - ) { - await fs.rename(oldBinaries[0], binPath); - return binPath; + const old = await tryCandidate(oldBinaries[0]); + if (old) { + return old; } } - // No fallback available or user declined - re-throw original error throw error; } @@ -351,7 +420,7 @@ export class CliManager { } // Figure out where to get the binary. - const binName = cliUtils.name(); + const binName = cliUtils.fullName(); const configSource = cfg.get("binarySource"); const binSource = configSource?.trim() ? configSource : "/bin/" + binName; this.output.info("Downloading binary from", binSource); diff --git a/src/core/cliUtils.ts b/src/core/cliUtils.ts index 7302b43a..f324e9ec 100644 --- a/src/core/cliUtils.ts +++ b/src/core/cliUtils.ts @@ -41,16 +41,18 @@ export interface RemovalResult { */ export async function rmOld(binPath: string): Promise { const binDir = path.dirname(binPath); + const binBase = path.basename(binPath); try { const files = await fs.readdir(binDir); const results: RemovalResult[] = []; for (const file of files) { const fileName = path.basename(file); if ( - fileName.includes(".old-") || - fileName.includes(".temp-") || - fileName.endsWith(".asc") || - fileName.endsWith(".progress.log") + fileName.startsWith(binBase) && + (fileName.includes(".old-") || + fileName.includes(".temp-") || + fileName.endsWith(".asc") || + fileName.endsWith(".progress.log")) ) { try { await fs.rm(path.join(binDir, file), { force: true }); @@ -137,9 +139,9 @@ export async function eTag(binPath: string): Promise { } /** - * Return the binary name for the current platform. + * Return the platform-specific binary name (e.g. "coder-linux-amd64"). */ -export function name(): string { +export function fullName(): string { const os = goos(); const arch = goarch(); let binName = `coder-${os}-${arch}`; @@ -150,6 +152,15 @@ export function name(): string { return binName; } +/** + * Return the simple binary name ("coder" or "coder.exe" on Windows). + * This is the name used by package managers, as opposed to the + * platform-specific name returned by fullName(). + */ +export function simpleName(): string { + return goos() === "windows" ? "coder.exe" : "coder"; +} + /** * Returns the Go format for the current platform. * Coder binaries are created in Go, so we conform to that name structure. diff --git a/test/unit/core/cliManager.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts index 057dbc60..08a47091 100644 --- a/test/unit/core/cliManager.concurrent.test.ts +++ b/test/unit/core/cliManager.concurrent.test.ts @@ -33,7 +33,7 @@ vi.mock("@/core/cliUtils", async () => { ...actual, goos: vi.fn(), goarch: vi.fn(), - name: vi.fn(), + fullName: vi.fn(), }; }); @@ -48,7 +48,7 @@ vi.mock("@/core/cliExec", async () => { function setupCliUtilsMocks(version: string) { vi.mocked(cliUtils.goos).mockReturnValue("linux"); vi.mocked(cliUtils.goarch).mockReturnValue("amd64"); - vi.mocked(cliUtils.name).mockReturnValue("coder-linux-amd64"); + vi.mocked(cliUtils.fullName).mockReturnValue("coder-linux-amd64"); vi.mocked(cliExec.version).mockResolvedValue(version); vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); } @@ -122,7 +122,7 @@ describe("CliManager Concurrent Downloads", () => { expect(result).toBe(binaryPath); } - // Verify binary exists and lock/progress files are cleaned up + // Verify binary exists, and lock/progress files are cleaned up await expect(fs.access(binaryPath)).resolves.toBeUndefined(); await expect(fs.access(binaryPath + ".lock")).rejects.toThrow(); await expect(fs.access(binaryPath + ".progress.log")).rejects.toThrow(); @@ -161,7 +161,7 @@ describe("CliManager Concurrent Downloads", () => { expect(result).toBe(binaryPath); } - // Binary should be updated to 2.0.0, lock/progress files cleaned up + // Binary should be updated to 2.0.0, and lock/progress files are cleaned up await expect(fs.access(binaryPath)).resolves.toBeUndefined(); const finalContent = await fs.readFile(binaryPath, "utf8"); expect(finalContent).toContain("v2.0.0"); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 024f1ef0..ab05ea4b 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -107,6 +107,9 @@ describe("CliManager", () => { vi.mocked(os.platform).mockReturnValue(PLATFORM); vi.mocked(os.arch).mockReturnValue(ARCH); vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); + + // Most tests don't need signature verification. + mockConfig.set("coder.disableSignatureVerification", true); }); afterEach(async () => { @@ -200,6 +203,148 @@ describe("CliManager", () => { }); }); + describe("Binary Resolution", () => { + /** Simulate a download failure with a usable fallback binary. */ + function withFailedDownload(fallbackVersion: string) { + withStreamError("write", "disk full"); + vi.mocked(cliExec.version).mockResolvedValueOnce(fallbackVersion); + mockUI.setResponse( + `Failed to update CLI binary. Run version ${fallbackVersion} anyway?`, + "Run", + ); + } + + describe("file destination", () => { + const FILE_PATH = "/usr/local/bin/coder"; + const FILE_DIR = path.dirname(FILE_PATH); + const DOWNLOAD_PATH = path.join(FILE_DIR, BINARY_NAME); + + function withFileBinary(filePath: string, version: string) { + mockConfig.set("coder.binaryDestination", filePath); + vol.mkdirSync(path.dirname(filePath), { recursive: true }); + memfs.writeFileSync(filePath, mockBinaryContent(version), { + mode: 0o755, + }); + vi.mocked(cliExec.version).mockResolvedValueOnce(version); + } + + it("locateBinary returns file path directly", async () => { + withFileBinary(FILE_PATH, TEST_VERSION); + expectPathsEqual(await manager.locateBinary(TEST_URL), FILE_PATH); + }); + + it("locateBinary throws when file does not exist", async () => { + mockConfig.set("coder.binaryDestination", "/nonexistent/coder"); + await expect(manager.locateBinary(TEST_URL)).rejects.toThrow( + "No CLI binary found at", + ); + }); + + it("fetchBinary uses file when version matches", async () => { + withFileBinary(FILE_PATH, TEST_VERSION); + expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + expect(mockAxios.get).not.toHaveBeenCalled(); + }); + + it("fetchBinary downloads to platform-specific name then renames", async () => { + withFileBinary(FILE_PATH, "0.0.1"); + withSuccessfulDownload(); + + expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + expect(memfs.existsSync(DOWNLOAD_PATH)).toBe(false); + expect(memfs.readFileSync(FILE_PATH).toString()).toBe( + mockBinaryContent(TEST_VERSION), + ); + }); + + it("fetchBinary downloads in-place when file is already platform-specific", async () => { + // User configured a path that matches the platform-specific name. + withFileBinary(DOWNLOAD_PATH, "0.0.1"); + withSuccessfulDownload(); + + expectPathsEqual(await manager.fetchBinary(mockApi), DOWNLOAD_PATH); + expect(memfs.readFileSync(DOWNLOAD_PATH).toString()).toBe( + mockBinaryContent(TEST_VERSION), + ); + }); + + it("fetchBinary keeps mismatched file when downloads disabled", async () => { + mockConfig.set("coder.enableDownloads", false); + withFileBinary(FILE_PATH, "0.0.1"); + expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + expect(mockAxios.get).not.toHaveBeenCalled(); + }); + + it("fetchBinary falls back to configured path on download failure", async () => { + withFileBinary(FILE_PATH, "0.0.1"); + withFailedDownload("0.0.1"); + expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + }); + + it("fetchBinary renames fallback to configured path on download failure", async () => { + withFileBinary(FILE_PATH, "0.0.1"); + // A previous download left a binary at the platform-specific path. + memfs.writeFileSync(DOWNLOAD_PATH, mockBinaryContent("0.0.1"), { + mode: 0o755, + }); + withFailedDownload("0.0.1"); + + expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + expect(memfs.existsSync(DOWNLOAD_PATH)).toBe(false); + }); + }); + + describe("simple name fallback", () => { + const SIMPLE_PATH = `${BINARY_DIR}/coder`; + + function withSimpleBinary(version: string) { + vol.mkdirSync(BINARY_DIR, { recursive: true }); + memfs.writeFileSync(SIMPLE_PATH, mockBinaryContent(version), { + mode: 0o755, + }); + vi.mocked(cliExec.version).mockResolvedValueOnce(version); + } + + it("locateBinary falls back to simple name", async () => { + withSimpleBinary(TEST_VERSION); + expectPathsEqual(await manager.locateBinary(TEST_URL), SIMPLE_PATH); + }); + + it("locateBinary prefers platform-specific name", async () => { + withSimpleBinary(TEST_VERSION); + memfs.writeFileSync(BINARY_PATH, mockBinaryContent(TEST_VERSION), { + mode: 0o755, + }); + expectPathsEqual(await manager.locateBinary(TEST_URL), BINARY_PATH); + }); + + it("fetchBinary uses simple-named binary", async () => { + withSimpleBinary(TEST_VERSION); + expectPathsEqual(await manager.fetchBinary(mockApi), SIMPLE_PATH); + expect(mockAxios.get).not.toHaveBeenCalled(); + }); + + it("fetchBinary downloads to platform-specific name (not simple name)", async () => { + withSimpleBinary("0.0.1"); + withSuccessfulDownload(); + + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + expect(memfs.readFileSync(SIMPLE_PATH).toString()).toBe( + mockBinaryContent("0.0.1"), + ); + expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( + mockBinaryContent(TEST_VERSION), + ); + }); + + it("fetchBinary falls back to simple name on download failure", async () => { + withSimpleBinary("0.0.1"); + withFailedDownload("0.0.1"); + expectPathsEqual(await manager.fetchBinary(mockApi), SIMPLE_PATH); + }); + }); + }); + describe("Clear Credentials", () => { const CLEAR_URL = "https://dev.coder.com"; @@ -251,21 +396,14 @@ describe("CliManager", () => { it("accepts valid semver versions", async () => { withExistingBinary(TEST_VERSION); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); }); }); describe("Existing Binary Handling", () => { - beforeEach(() => { - // Disable signature verification for these tests - mockConfig.set("coder.disableSignatureVerification", true); - }); - it("reuses matching binary without downloading", async () => { withExistingBinary(TEST_VERSION); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(mockAxios.get).not.toHaveBeenCalled(); // Verify binary still exists expect(memfs.existsSync(BINARY_PATH)).toBe(true); @@ -274,11 +412,7 @@ describe("CliManager", () => { it("downloads when versions differ", async () => { withExistingBinary("1.0.0"); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); - expect(mockAxios.get).toHaveBeenCalled(); - // Verify new binary exists - expect(memfs.existsSync(BINARY_PATH)).toBe(true); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( mockBinaryContent(TEST_VERSION), ); @@ -287,11 +421,8 @@ describe("CliManager", () => { it("keeps mismatched binary when downloads disabled", async () => { mockConfig.set("coder.enableDownloads", false); withExistingBinary("1.0.0"); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(mockAxios.get).not.toHaveBeenCalled(); - // Should still have the old version - expect(memfs.existsSync(BINARY_PATH)).toBe(true); expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( mockBinaryContent("1.0.0"), ); @@ -300,27 +431,18 @@ describe("CliManager", () => { it("downloads fresh binary when corrupted", async () => { withCorruptedBinary(); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); - expect(mockAxios.get).toHaveBeenCalled(); - expect(memfs.existsSync(BINARY_PATH)).toBe(true); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( mockBinaryContent(TEST_VERSION), ); }); it("downloads when no binary exists", async () => { - // Ensure directory doesn't exist initially expect(memfs.existsSync(BINARY_DIR)).toBe(false); - withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); - expect(mockAxios.get).toHaveBeenCalled(); - // Verify directory was created and binary exists + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(memfs.existsSync(BINARY_DIR)).toBe(true); - expect(memfs.existsSync(BINARY_PATH)).toBe(true); expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( mockBinaryContent(TEST_VERSION), ); @@ -333,13 +455,34 @@ describe("CliManager", () => { ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); }); - }); - describe("Binary Download Behavior", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", true); + it("restores old backup when replace fails", async () => { + withExistingBinary("1.0.0"); + withSuccessfulDownload(); + + // Fail the temp → binPath rename to simulate a locked binary. + // The existing → .old-* rename (no .temp- in source) still succeeds. + const realRename = memfs.promises.rename.bind(memfs.promises); + const spy = vi + .spyOn(memfs.promises, "rename") + .mockImplementation(async (src, dest) => { + if (String(src).includes(".temp-")) { + const err = new Error("EBUSY") as NodeJS.ErrnoException; + err.code = "EBUSY"; + throw err; + } + return realRename(src, dest); + }); + + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + expect( + readdir(BINARY_DIR).find((f) => f.includes(".old-")), + ).toBeUndefined(); + spy.mockRestore(); }); + }); + describe("Binary Download Behavior", () => { it("downloads with correct headers", async () => { withSuccessfulDownload(); await manager.fetchBinary(mockApi); @@ -386,25 +529,40 @@ describe("CliManager", () => { }); it("cleans up old files before download", async () => { - // Create old temporary files and signature files + // Create old temporary files and signature files matching the binary name vol.mkdirSync(BINARY_DIR, { recursive: true }); - memfs.writeFileSync(path.join(BINARY_DIR, "coder.old-xyz"), "old"); - memfs.writeFileSync(path.join(BINARY_DIR, "coder.temp-abc"), "temp"); - memfs.writeFileSync(path.join(BINARY_DIR, "coder.asc"), "signature"); + memfs.writeFileSync( + path.join(BINARY_DIR, `${BINARY_NAME}.old-xyz`), + "old", + ); + memfs.writeFileSync( + path.join(BINARY_DIR, `${BINARY_NAME}.temp-abc`), + "temp", + ); + memfs.writeFileSync( + path.join(BINARY_DIR, `${BINARY_NAME}.asc`), + "signature", + ); + // Unrelated files should not be removed memfs.writeFileSync(path.join(BINARY_DIR, "keeper.txt"), "keep"); + memfs.writeFileSync(path.join(BINARY_DIR, "other.old-xyz"), "keep"); withSuccessfulDownload(); await manager.fetchBinary(mockApi); - // Verify old files were actually removed but other files kept - expect(memfs.existsSync(path.join(BINARY_DIR, "coder.old-xyz"))).toBe( - false, - ); - expect(memfs.existsSync(path.join(BINARY_DIR, "coder.temp-abc"))).toBe( - false, - ); - expect(memfs.existsSync(path.join(BINARY_DIR, "coder.asc"))).toBe(false); + expect( + memfs.existsSync(path.join(BINARY_DIR, `${BINARY_NAME}.old-xyz`)), + ).toBe(false); + expect( + memfs.existsSync(path.join(BINARY_DIR, `${BINARY_NAME}.temp-abc`)), + ).toBe(false); + expect( + memfs.existsSync(path.join(BINARY_DIR, `${BINARY_NAME}.asc`)), + ).toBe(false); expect(memfs.existsSync(path.join(BINARY_DIR, "keeper.txt"))).toBe(true); + expect(memfs.existsSync(path.join(BINARY_DIR, "other.old-xyz"))).toBe( + true, + ); }); it("moves existing binary to backup file before writing new version", async () => { @@ -423,16 +581,10 @@ describe("CliManager", () => { }); describe("Download HTTP Response Handling", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", true); - }); - it("handles 304 Not Modified", async () => { withExistingBinary("1.0.0"); withHttpResponse(304); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); - // No change + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( mockBinaryContent("1.0.0"), ); @@ -476,10 +628,6 @@ describe("CliManager", () => { }); describe("Download Stream Handling", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", true); - }); - it("handles write stream errors", async () => { withStreamError("write", "disk full"); await expect(manager.fetchBinary(mockApi)).rejects.toThrow( @@ -498,10 +646,7 @@ describe("CliManager", () => { it("handles missing content-length", async () => { withSuccessfulDownload({ headers: {} }); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); - expect(memfs.existsSync(BINARY_PATH)).toBe(true); - // Without any content-length header, increment should be undefined. + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); const reports = mockProgress.getProgressReports(); expect(reports).not.toHaveLength(0); for (const report of reports) { @@ -513,9 +658,7 @@ describe("CliManager", () => { "reports progress with %s header", async (header) => { withSuccessfulDownload({ headers: { [header]: "1024" } }); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); - expect(memfs.existsSync(BINARY_PATH)).toBe(true); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); const reports = mockProgress.getProgressReports(); expect(reports).not.toHaveLength(0); for (const report of reports) { @@ -526,10 +669,6 @@ describe("CliManager", () => { }); describe("Download Progress Tracking", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", true); - }); - it("shows download progress", async () => { withSuccessfulDownload(); await manager.fetchBinary(mockApi); @@ -552,25 +691,25 @@ describe("CliManager", () => { }); describe("Binary Signature Verification", () => { + beforeEach(() => { + mockConfig.set("coder.disableSignatureVerification", false); + }); + it("verifies valid signatures", async () => { withSuccessfulDownload(); withSignatureResponses([200]); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(pgp.verifySignature).toHaveBeenCalled(); - const sigFile = expectFileInDir(BINARY_DIR, ".asc"); - expect(sigFile).toBeDefined(); + expect(expectFileInDir(BINARY_DIR, ".asc")).toBeDefined(); }); it("tries fallback signature on 404", async () => { withSuccessfulDownload(); withSignatureResponses([404, 200]); mockUI.setResponse("Signature not found", "Download signature"); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(mockAxios.get).toHaveBeenCalledTimes(3); - const sigFile = expectFileInDir(BINARY_DIR, ".asc"); - expect(sigFile).toBeDefined(); + expect(expectFileInDir(BINARY_DIR, ".asc")).toBeDefined(); }); it("allows running despite invalid signature", async () => { @@ -580,9 +719,7 @@ describe("CliManager", () => { createVerificationError("Invalid signature"), ); mockUI.setResponse("Signature does not match", "Run anyway"); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); - expect(memfs.existsSync(BINARY_PATH)).toBe(true); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); }); it("aborts on signature rejection", async () => { @@ -600,11 +737,8 @@ describe("CliManager", () => { it("skips verification when disabled", async () => { mockConfig.set("coder.disableSignatureVerification", true); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(pgp.verifySignature).not.toHaveBeenCalled(); - const files = readdir(BINARY_DIR); - expect(files.find((file) => file.includes(".asc"))).toBeUndefined(); }); type SignatureErrorTestCase = [status: number, message: string]; @@ -615,8 +749,7 @@ describe("CliManager", () => { withSuccessfulDownload(); withHttpResponse(status); mockUI.setResponse(message, "Run without verification"); - const result = await manager.fetchBinary(mockApi); - expectPathsEqual(result, BINARY_PATH); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(pgp.verifySignature).not.toHaveBeenCalled(); }); @@ -637,17 +770,11 @@ describe("CliManager", () => { }); describe("File System Operations", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", true); - }); - it("creates binary directory", async () => { expect(memfs.existsSync(BINARY_DIR)).toBe(false); withSuccessfulDownload(); await manager.fetchBinary(mockApi); - expect(memfs.existsSync(BINARY_DIR)).toBe(true); - const stats = memfs.statSync(BINARY_DIR); - expect(stats.isDirectory()).toBe(true); + expect(memfs.statSync(BINARY_DIR).isDirectory()).toBe(true); }); it("validates downloaded binary version", async () => { @@ -661,29 +788,22 @@ describe("CliManager", () => { it("sets correct file permissions", async () => { withSuccessfulDownload(); await manager.fetchBinary(mockApi); - const stats = memfs.statSync(BINARY_PATH); - expect(stats.mode & 0o777).toBe(0o755); + expect(memfs.statSync(BINARY_PATH).mode & 0o777).toBe(0o755); }); }); describe("Path Pecularities", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", true); - }); - it("handles binary with spaces in path", async () => { const pathWithSpaces = "/path with spaces/bin"; - const resolver = new PathResolver(pathWithSpaces, "/log"); const manager = new CliManager( createMockLogger(), - resolver, + new PathResolver(pathWithSpaces, "/log"), createMockCliCredentialManager(), ); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi); expectPathsEqual( - result, + await manager.fetchBinary(mockApi), `${pathWithSpaces}/test.coder.com/bin/${BINARY_NAME}`, ); }); diff --git a/test/unit/core/cliUtils.test.ts b/test/unit/core/cliUtils.test.ts index 0699f591..c0aa1a07 100644 --- a/test/unit/core/cliUtils.test.ts +++ b/test/unit/core/cliUtils.test.ts @@ -14,8 +14,17 @@ describe("CliUtils", () => { await fs.mkdir(tmp, { recursive: true }); }); - it("name", () => { - expect(cliUtils.name().startsWith("coder-")).toBeTruthy(); + it("fullName", () => { + expect(cliUtils.fullName().startsWith("coder-")).toBeTruthy(); + }); + + it("simpleName", () => { + const simple = cliUtils.simpleName(); + if (process.platform === "win32") { + expect(simple).toBe("coder.exe"); + } else { + expect(simple).toBe("coder"); + } }); it("stat", async () => { @@ -31,50 +40,56 @@ describe("CliUtils", () => { expect(await cliUtils.rmOld(path.join(binDir, "bin1"))).toStrictEqual([]); await fs.mkdir(binDir, { recursive: true }); - await fs.writeFile(path.join(binDir, "bin.old-1"), "echo hello"); - await fs.writeFile(path.join(binDir, "bin.old-2"), "echo hello"); - await fs.writeFile(path.join(binDir, "bin.temp-1"), "echo hello"); - await fs.writeFile(path.join(binDir, "bin.temp-2"), "echo hello"); - await fs.writeFile(path.join(binDir, "bin1"), "echo hello"); - await fs.writeFile(path.join(binDir, "bin2"), "echo hello"); - await fs.writeFile(path.join(binDir, "bin.asc"), "echo hello"); - await fs.writeFile(path.join(binDir, "bin.old-1.asc"), "echo hello"); - await fs.writeFile(path.join(binDir, "bin.temp-2.asc"), "echo hello"); + await fs.writeFile(path.join(binDir, "coder.old-1"), "echo hello"); + await fs.writeFile(path.join(binDir, "coder.old-2"), "echo hello"); + await fs.writeFile(path.join(binDir, "coder.temp-1"), "echo hello"); + await fs.writeFile(path.join(binDir, "coder.temp-2"), "echo hello"); + await fs.writeFile(path.join(binDir, "coder"), "echo hello"); + await fs.writeFile(path.join(binDir, "other-bin"), "echo hello"); + await fs.writeFile(path.join(binDir, "coder.asc"), "echo hello"); + await fs.writeFile(path.join(binDir, "coder.old-1.asc"), "echo hello"); + await fs.writeFile(path.join(binDir, "coder.temp-2.asc"), "echo hello"); + // Unrelated files with matching patterns should not be removed. + await fs.writeFile(path.join(binDir, "unrelated.old-1"), "echo hello"); + await fs.writeFile(path.join(binDir, "unrelated.temp-1"), "echo hello"); - expect(await cliUtils.rmOld(path.join(binDir, "bin1"))).toStrictEqual([ + expect(await cliUtils.rmOld(path.join(binDir, "coder"))).toStrictEqual([ { - fileName: "bin.asc", + fileName: "coder.asc", error: undefined, }, { - fileName: "bin.old-1", + fileName: "coder.old-1", error: undefined, }, { - fileName: "bin.old-1.asc", + fileName: "coder.old-1.asc", error: undefined, }, { - fileName: "bin.old-2", + fileName: "coder.old-2", error: undefined, }, { - fileName: "bin.temp-1", + fileName: "coder.temp-1", error: undefined, }, { - fileName: "bin.temp-2", + fileName: "coder.temp-2", error: undefined, }, { - fileName: "bin.temp-2.asc", + fileName: "coder.temp-2.asc", error: undefined, }, ]); - expect(await fs.readdir(path.join(tmp, "bins"))).toStrictEqual([ - "bin1", - "bin2", + // Only the binary and unrelated files should remain. + expect((await fs.readdir(binDir)).sort()).toStrictEqual([ + "coder", + "other-bin", + "unrelated.old-1", + "unrelated.temp-1", ]); });