From 6714fe165ca9fdcd54428b7e69055ae3ce4218e8 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 9 Apr 2026 17:23:55 +0300 Subject: [PATCH 1/2] feat: allow binaryDestination to accept a full path to the CLI binary The coder.binaryDestination setting (and CODER_BINARY_DESTINATION env var) now accepts either a file path or a directory path: - File path (e.g. /usr/bin/coder): if the file exists, use it directly. Version is checked and reported. If the version mismatches and downloads are enabled, the binary is re-downloaded to the same path. If downloads are disabled, the existing binary is used as-is with a warning. - Directory path (existing behavior): look for the platform-specific name (e.g. coder-linux-amd64) first, then fall back to the simple name (coder / coder.exe). This supports package-manager-installed CLIs that use the simple name. Adds cliUtils.simpleName() returning "coder" (or "coder.exe" on Windows) and a unified resolveBinaryPath() method in CliManager used by both locateBinary() and fetchBinary(). Resolves #861 --- package.json | 2 +- src/core/cliManager.ts | 117 ++++++++++++++++++++---------- src/core/cliUtils.ts | 11 ++- test/unit/core/cliManager.test.ts | 88 ++++++++++++++++++++++ test/unit/core/cliUtils.test.ts | 9 +++ 5 files changed, 187 insertions(+), 40 deletions(-) diff --git a/package.json b/package.json index abafdf0e..2dd0111c 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 uses it directly without downloading. 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..f14ed43c 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; kind: "file" | "dir"; stat: Stats } + | { binPath: string; kind: "not-found" }; + export class CliManager { private readonly binaryLock: BinaryLock; @@ -46,15 +50,50 @@ 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.kind === "not-found") { + throw new Error(`No CLI binary found at ${resolved.binPath}`); + } + return resolved.binPath; + } + + /** + * Resolve the CLI binary path from the configured cache path. + * + * Returns "file" when the cache path is an existing file (use as-is), + * "dir" 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, kind: "file", stat: cacheStat }; + } + + const fullNamePath = path.join(cachePath, cliUtils.name()); + + // Path does not exist yet; return the platform-specific path to download. + if (!cacheStat) { + return { binPath: fullNamePath, kind: "not-found" }; + } + + // Directory exists; check platform-specific name, then simple name. + const fullStat = await cliUtils.stat(fullNamePath); + if (fullStat) { + return { binPath: fullNamePath, kind: "dir", stat: fullStat }; } - return binPath; + + const simpleNamePath = path.join(cachePath, cliUtils.simpleName()); + const simpleStat = await cliUtils.stat(simpleNamePath); + if (simpleStat) { + return { binPath: simpleNamePath, kind: "dir", stat: simpleStat }; + } + + return { binPath: fullNamePath, kind: "not-found" }; } /** @@ -94,43 +133,38 @@ 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(), - ); - 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)); + const resolved = await this.resolveBinaryPath(safeHostname); + this.output.debug("Resolved binary path", resolved.binPath, resolved.kind); + + // Check existing binary version when one was found. + if (resolved.kind !== "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"); } if (!enableDownloads) { @@ -138,9 +172,16 @@ export class CliManager { throw new Error("Unable to download CLI because downloads are disabled"); } + // File destinations download to the same path. Directory destinations + // always use the platform-specific binary name. + const downloadBinPath = + resolved.kind === "file" + ? resolved.binPath + : path.join(path.dirname(resolved.binPath), cliUtils.name()); + // 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,7 +189,7 @@ export class CliManager { let latestVersion = parsedVersion; try { lockResult = await this.binaryLock.acquireLockOrWait( - binPath, + downloadBinPath, progressLogPath, ); this.output.debug("Acquired download lock"); @@ -159,14 +200,14 @@ export class CliManager { 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; + return downloadBinPath; } // Parse the latest version for download @@ -182,14 +223,14 @@ export class CliManager { return await this.performBinaryDownload( restClient, latestVersion, - binPath, + downloadBinPath, progressLogPath, ); } catch (error) { // Unified error handling - check for fallback binaries and prompt user return await this.handleAnyBinaryFailure( error, - binPath, + downloadBinPath, buildInfo.version, ); } finally { diff --git a/src/core/cliUtils.ts b/src/core/cliUtils.ts index 7302b43a..edf0b636 100644 --- a/src/core/cliUtils.ts +++ b/src/core/cliUtils.ts @@ -137,7 +137,7 @@ 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 { const os = goos(); @@ -150,6 +150,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 name(). + */ +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.test.ts b/test/unit/core/cliManager.test.ts index 024f1ef0..592605ff 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -200,6 +200,94 @@ describe("CliManager", () => { }); }); + describe("File Destination", () => { + const FILE_PATH = "/usr/local/bin/coder"; + + beforeEach(() => { + mockConfig.set("coder.disableSignatureVerification", true); + }); + + 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 same path when version mismatches", async () => { + withFileBinary(FILE_PATH, "0.0.1"); + withSuccessfulDownload(); + expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + expect(mockAxios.get).toHaveBeenCalled(); + }); + + 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(); + }); + }); + + 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 when simple name has wrong version", async () => { + mockConfig.set("coder.disableSignatureVerification", true); + withSimpleBinary("0.0.1"); + withSuccessfulDownload(); + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + expect(mockAxios.get).toHaveBeenCalled(); + }); + }); + describe("Clear Credentials", () => { const CLEAR_URL = "https://dev.coder.com"; diff --git a/test/unit/core/cliUtils.test.ts b/test/unit/core/cliUtils.test.ts index 0699f591..fed02b27 100644 --- a/test/unit/core/cliUtils.test.ts +++ b/test/unit/core/cliUtils.test.ts @@ -18,6 +18,15 @@ describe("CliUtils", () => { expect(cliUtils.name().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 () => { const binPath = path.join(tmp, "stat"); expect(await cliUtils.stat(binPath)).toBeUndefined(); From 59f545c8dee89b9ac7dec9a34ed5031192eace7e Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 9 Apr 2026 19:08:44 +0300 Subject: [PATCH 2/2] fix: use platform-specific name for temp/cleanup in shared directories Always download to the platform-specific binary name (coder-linux-amd64) for temp files, old backups, signatures, and lock files. After a successful download, rename the result to the user's configured file name while still holding the lock to avoid races with other windows. Scoped rmOld to only clean files prefixed with the binary basename so it does not accidentally remove unrelated files when the target directory is shared (e.g. /usr/bin). Additional improvements: - Rename cliUtils.name() to fullName() to pair with simpleName() - Replace ResolvedBinary kind "file"/"dir" with source "file-path"/"directory" to clarify that stat always describes the binary, not the directory - Fix binaryDestination description: file paths are version-checked and updated on mismatch, not used without downloading - Update resolveBinaryPath JSDoc to remove misleading "as-is" language - Add fallback path to error handler so file-path destinations check the user's original binary when the download path has no usable binary - Simplify handleAnyBinaryFailure with tryCandidate closure and deferred findOldBinaries to avoid unnecessary I/O when earlier candidates match - On error recovery, rename fallback to resolved.binPath so the binary always ends up where the user expects - Add "starting download" back to the no-binary-found log message - Improve test coverage: file destination error fallback, simple name error fallback, platform-specific file destination, old backup restore - Unify test structure: shared disableSignatureVerification in top-level beforeEach, shared withFailedDownload helper, Binary Resolution group - Remove flaky lock/progress file assertions from concurrent tests --- package.json | 2 +- src/core/cliManager.ts | 162 ++++---- src/core/cliUtils.ts | 14 +- test/unit/core/cliManager.concurrent.test.ts | 8 +- test/unit/core/cliManager.test.ts | 370 ++++++++++--------- test/unit/core/cliUtils.test.ts | 50 +-- 6 files changed, 337 insertions(+), 269 deletions(-) diff --git a/package.json b/package.json index 2dd0111c..961be8c6 100644 --- a/package.json +++ b/package.json @@ -62,7 +62,7 @@ "default": "" }, "coder.binaryDestination": { - "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 uses it directly without downloading. 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.", + "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 f14ed43c..9694a5e3 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -30,8 +30,8 @@ import type { CliCredentialManager } from "./cliCredentialManager"; import type { PathResolver } from "./pathResolver"; type ResolvedBinary = - | { binPath: string; kind: "file" | "dir"; stat: Stats } - | { binPath: string; kind: "not-found" }; + | { binPath: string; stat: Stats; source: "file-path" | "directory" } + | { binPath: string; source: "not-found" }; export class CliManager { private readonly binaryLock: BinaryLock; @@ -51,7 +51,7 @@ export class CliManager { public async locateBinary(url: string): Promise { const safeHostname = toSafeHost(url); const resolved = await this.resolveBinaryPath(safeHostname); - if (resolved.kind === "not-found") { + if (resolved.source === "not-found") { throw new Error(`No CLI binary found at ${resolved.binPath}`); } return resolved.binPath; @@ -60,9 +60,10 @@ export class CliManager { /** * Resolve the CLI binary path from the configured cache path. * - * Returns "file" when the cache path is an existing file (use as-is), - * "dir" when a binary was found inside the directory, or "not-found" - * with the platform-specific path for the caller to download into. + * 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, @@ -71,29 +72,29 @@ export class CliManager { const cacheStat = await cliUtils.stat(cachePath); if (cacheStat?.isFile()) { - return { binPath: cachePath, kind: "file", stat: cacheStat }; + return { binPath: cachePath, stat: cacheStat, source: "file-path" }; } - const fullNamePath = path.join(cachePath, cliUtils.name()); + const fullNamePath = path.join(cachePath, cliUtils.fullName()); // Path does not exist yet; return the platform-specific path to download. if (!cacheStat) { - return { binPath: fullNamePath, kind: "not-found" }; + 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, kind: "dir", stat: 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, kind: "dir", stat: simpleStat }; + return { binPath: simpleNamePath, stat: simpleStat, source: "directory" }; } - return { binPath: fullNamePath, kind: "not-found" }; + return { binPath: fullNamePath, source: "not-found" }; } /** @@ -134,10 +135,12 @@ export class CliManager { } const resolved = await this.resolveBinaryPath(safeHostname); - this.output.debug("Resolved binary path", resolved.binPath, resolved.kind); + this.output.debug( + `Resolved binary: ${resolved.binPath} (${resolved.source})`, + ); // Check existing binary version when one was found. - if (resolved.kind !== "not-found") { + if (resolved.source !== "not-found") { this.output.debug( "Existing binary size is", prettyBytes(resolved.stat.size), @@ -164,7 +167,7 @@ export class CliManager { ); } } else { - this.output.info("No existing binary found"); + this.output.info("No existing binary found, starting download"); } if (!enableDownloads) { @@ -172,12 +175,11 @@ export class CliManager { throw new Error("Unable to download CLI because downloads are disabled"); } - // File destinations download to the same path. Directory destinations - // always use the platform-specific binary name. - const downloadBinPath = - resolved.kind === "file" - ? resolved.binPath - : path.join(path.dirname(resolved.binPath), cliUtils.name()); + // 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(downloadBinPath), { recursive: true }); @@ -195,6 +197,7 @@ export class CliManager { 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); @@ -207,32 +210,52 @@ export class CliManager { this.output.debug( "Using existing binary since it matches the latest server version", ); - return downloadBinPath; + 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, - downloadBinPath, - 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, 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(); @@ -321,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; } - // Version mismatch - prompt user - if (await this.promptUseExistingBinary(existingCheck.version, message)) { - return binPath; + if ( + !check.matches && + !(await this.promptUseExistingBinary(check.version, message)) + ) { + throw error; + } + 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; } @@ -392,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 edf0b636..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 }); @@ -139,7 +141,7 @@ export async function eTag(binPath: string): Promise { /** * 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}`; @@ -153,7 +155,7 @@ export function name(): string { /** * 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 name(). + * platform-specific name returned by fullName(). */ export function simpleName(): string { return goos() === "windows" ? "coder.exe" : "coder"; 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 592605ff..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,91 +203,145 @@ describe("CliManager", () => { }); }); - describe("File Destination", () => { - const FILE_PATH = "/usr/local/bin/coder"; + 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", + ); + } - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", true); - }); + 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); + } - 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, + it("locateBinary returns file path directly", async () => { + withFileBinary(FILE_PATH, TEST_VERSION); + expectPathsEqual(await manager.locateBinary(TEST_URL), FILE_PATH); }); - 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("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 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(); - it("fetchBinary downloads to same path when version mismatches", async () => { - withFileBinary(FILE_PATH, "0.0.1"); - withSuccessfulDownload(); - expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); - expect(mockAxios.get).toHaveBeenCalled(); - }); + 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 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 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(); - describe("Simple Name Fallback", () => { - const SIMPLE_PATH = `${BINARY_DIR}/coder`; + expectPathsEqual(await manager.fetchBinary(mockApi), DOWNLOAD_PATH); + expect(memfs.readFileSync(DOWNLOAD_PATH).toString()).toBe( + mockBinaryContent(TEST_VERSION), + ); + }); - function withSimpleBinary(version: string) { - vol.mkdirSync(BINARY_DIR, { recursive: true }); - memfs.writeFileSync(SIMPLE_PATH, mockBinaryContent(version), { - mode: 0o755, + 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(); }); - 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("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("locateBinary prefers platform-specific name", async () => { - withSimpleBinary(TEST_VERSION); - memfs.writeFileSync(BINARY_PATH, mockBinaryContent(TEST_VERSION), { - mode: 0o755, + 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); }); - 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(); - }); + describe("simple name fallback", () => { + const SIMPLE_PATH = `${BINARY_DIR}/coder`; - it("fetchBinary downloads when simple name has wrong version", async () => { - mockConfig.set("coder.disableSignatureVerification", true); - withSimpleBinary("0.0.1"); - withSuccessfulDownload(); - expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); - expect(mockAxios.get).toHaveBeenCalled(); + 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); + }); }); }); @@ -339,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); @@ -362,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), ); @@ -375,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"), ); @@ -388,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), ); @@ -421,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); @@ -474,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 () => { @@ -511,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"), ); @@ -564,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( @@ -586,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) { @@ -601,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) { @@ -614,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); @@ -640,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 () => { @@ -668,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 () => { @@ -688,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]; @@ -703,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(); }); @@ -725,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 () => { @@ -749,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 fed02b27..c0aa1a07 100644 --- a/test/unit/core/cliUtils.test.ts +++ b/test/unit/core/cliUtils.test.ts @@ -14,8 +14,8 @@ 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", () => { @@ -40,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", ]); });