From 32eec2e7c309f79c118183fb2739ede1af4a69e2 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 9 Mar 2026 00:30:37 +0300 Subject: [PATCH 1/2] refactor: improve keyring support reliability and reduce duplication - Separate keyringTokenRead (>= 2.31) from keyringAuth (>= 2.29) feature flags - Combine duplicate resolveKeyringBinary/resolveReadTokenBinary with explicit feature param - Extract tempFilePath() utility with crypto.randomUUID, replacing 4 inline patterns - Add withProgress() wrapper, adopt in commands.ts and promptUtils.ts - Type-safe circular dependency guard in ServiceContainer via ref object - Fix stubExecFileAbortable to handle non-pre-aborted signals - Replace regex test matchers with structural assertions --- src/commands.ts | 4 +- src/core/cliCredentialManager.ts | 37 ++++++++++------- src/core/cliManager.ts | 10 ++--- src/core/container.ts | 18 +++++++-- src/featureSet.ts | 6 +++ src/progress.ts | 14 +++++++ src/promptUtils.ts | 10 ++--- src/remote/sshConfig.ts | 16 ++++---- src/util.ts | 9 +++++ test/unit/core/cliCredentialManager.test.ts | 22 +++++++++-- test/unit/featureSet.test.ts | 15 +++++++ test/unit/progress.test.ts | 44 ++++++++++++++++++++- test/unit/remote/sshConfig.test.ts | 35 ++++++++-------- test/unit/util.test.ts | 22 +++++++++++ 14 files changed, 199 insertions(+), 63 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index b350773d..d6b08e14 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -22,6 +22,7 @@ import { toError } from "./error/errorUtils"; import { featureSetForVersion } from "./featureSet"; import { type Logger } from "./logging/logger"; import { type LoginCoordinator } from "./login/loginCoordinator"; +import { withProgress } from "./progress"; import { maybeAskAgent, maybeAskUrl } from "./promptUtils"; import { escapeCommandArg, toRemoteAuthority, toSafeHost } from "./util"; import { vscodeProposed } from "./vscodeProposed"; @@ -446,11 +447,10 @@ export class Commands { }): Promise { // Launch and run command in terminal if command is provided if (app.command) { - return vscode.window.withProgress( + return withProgress( { location: vscode.ProgressLocation.Notification, title: `Connecting to AI Agent...`, - cancellable: false, }, async () => { const terminal = vscode.window.createTerminal(app.name); diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index a7722362..28808ead 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -5,9 +5,9 @@ import { promisify } from "node:util"; import * as semver from "semver"; import { isKeyringEnabled } from "../cliConfig"; -import { featureSetForVersion } from "../featureSet"; +import { type FeatureSet, featureSetForVersion } from "../featureSet"; import { getHeaderArgs } from "../headers"; -import { toSafeHost } from "../util"; +import { tempFilePath, toSafeHost } from "../util"; import * as cliUtils from "./cliUtils"; @@ -59,7 +59,11 @@ export class CliCredentialManager { configs: Pick, signal?: AbortSignal, ): Promise { - const binPath = await this.resolveKeyringBinary(url, configs); + const binPath = await this.resolveKeyringBinary( + url, + configs, + "keyringAuth", + ); if (!binPath) { await this.writeCredentialFiles(url, token); return; @@ -91,17 +95,20 @@ export class CliCredentialManager { url: string, configs: Pick, ): Promise { - if (!isKeyringEnabled(configs)) { - return undefined; - } - - let binPath: string; + let binPath: string | undefined; try { - binPath = await this.resolveBinary(url); + binPath = await this.resolveKeyringBinary( + url, + configs, + "keyringTokenRead", + ); } catch (error) { this.logger.warn("Could not resolve CLI binary for token read:", error); return undefined; } + if (!binPath) { + return undefined; + } const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; try { @@ -131,8 +138,8 @@ export class CliCredentialManager { /** * Resolve a CLI binary for keyring operations. Returns the binary path - * when keyring is enabled in settings and the CLI version supports it, - * or undefined to fall back to file-based storage. + * when keyring is enabled in settings and the CLI version supports the + * requested feature, or undefined to fall back to file-based storage. * * Throws on binary resolution or version-check failure (caller decides * whether to catch or propagate). @@ -140,13 +147,14 @@ export class CliCredentialManager { private async resolveKeyringBinary( url: string, configs: Pick, + feature: keyof Pick, ): Promise { if (!isKeyringEnabled(configs)) { return undefined; } const binPath = await this.resolveBinary(url); const version = semver.parse(await cliUtils.version(binPath)); - return featureSetForVersion(version).keyringAuth ? binPath : undefined; + return featureSetForVersion(version)[feature] ? binPath : undefined; } /** @@ -217,7 +225,7 @@ export class CliCredentialManager { ): Promise { let binPath: string | undefined; try { - binPath = await this.resolveKeyringBinary(url, configs); + binPath = await this.resolveKeyringBinary(url, configs, "keyringAuth"); } catch (error) { this.logger.warn("Could not resolve keyring binary for delete:", error); return; @@ -243,8 +251,7 @@ export class CliCredentialManager { content: string, ): Promise { await fs.mkdir(path.dirname(filePath), { recursive: true }); - const tempPath = - filePath + ".temp-" + Math.random().toString(36).substring(8); + const tempPath = tempFilePath(filePath, "temp"); try { await fs.writeFile(tempPath, content, { mode: 0o600 }); await fs.rename(tempPath, filePath); diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 0ffc5a42..282c28ab 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -12,7 +12,7 @@ import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; import * as pgp from "../pgp"; import { withCancellableProgress } from "../progress"; -import { toSafeHost } from "../util"; +import { tempFilePath, toSafeHost } from "../util"; import { vscodeProposed } from "../vscodeProposed"; import { BinaryLock } from "./binaryLock"; @@ -40,7 +40,7 @@ export class CliManager { /** * Return the path to a cached CLI binary for a deployment URL. - * Stat check only — no network, no subprocess. Throws if absent. + * Stat check only, no network, no subprocess. Throws if absent. */ public async locateBinary(url: string): Promise { const safeHostname = toSafeHost(url); @@ -244,8 +244,7 @@ export class CliManager { binPath: string, tempFile: string, ): Promise { - const oldBinPath = - binPath + ".old-" + Math.random().toString(36).substring(8); + const oldBinPath = tempFilePath(binPath, "old"); try { // Step 1: Move existing binary to backup (if it exists) @@ -336,8 +335,7 @@ export class CliManager { progressLogPath: string, ): Promise { const cfg = vscode.workspace.getConfiguration("coder"); - const tempFile = - binPath + ".temp-" + Math.random().toString(36).substring(8); + const tempFile = tempFilePath(binPath, "temp"); try { const removed = await cliUtils.rmOld(binPath); diff --git a/src/core/container.ts b/src/core/container.ts index d34df35d..d50fa533 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -37,16 +37,25 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); - // Circular ref: cliCredentialManager ↔ cliManager. Safe because - // the resolver is only called after construction. + // Circular ref: cliCredentialManager ↔ cliManager. The resolver + // closure captures `ref` which starts undefined, so it must only + // be called after construction completes. + const cliManagerRef: { current: CliManager | undefined } = { + current: undefined, + }; this.cliCredentialManager = new CliCredentialManager( this.logger, async (url) => { + if (!cliManagerRef.current) { + throw new Error( + "BinaryResolver called before CliManager was initialised", + ); + } try { - return await this.cliManager.locateBinary(url); + return await cliManagerRef.current.locateBinary(url); } catch { const client = CoderApi.create(url, "", this.logger); - return this.cliManager.fetchBinary(client); + return cliManagerRef.current.fetchBinary(client); } }, this.pathResolver, @@ -56,6 +65,7 @@ export class ServiceContainer implements vscode.Disposable { this.pathResolver, this.cliCredentialManager, ); + cliManagerRef.current = this.cliManager; this.contextManager = new ContextManager(context); this.loginCoordinator = new LoginCoordinator( this.secretsManager, diff --git a/src/featureSet.ts b/src/featureSet.ts index 7f66d846..6d9fdcfc 100644 --- a/src/featureSet.ts +++ b/src/featureSet.ts @@ -6,6 +6,7 @@ export interface FeatureSet { wildcardSSH: boolean; buildReason: boolean; keyringAuth: boolean; + keyringTokenRead: boolean; } /** @@ -41,5 +42,10 @@ export function featureSetForVersion( keyringAuth: (version?.compare("2.29.0") ?? 0) >= 0 || version?.prerelease[0] === "devel", + + // `coder login token` for reading tokens from the keyring was added in 2.31.0 + keyringTokenRead: + (version?.compare("2.31.0") ?? 0) >= 0 || + version?.prerelease[0] === "devel", }; } diff --git a/src/progress.ts b/src/progress.ts index d24f017f..9de6c600 100644 --- a/src/progress.ts +++ b/src/progress.ts @@ -39,3 +39,17 @@ export function withCancellableProgress( }, ); } + +/** + * Run a task inside a VS Code progress notification (no cancellation). + * A thin wrapper over `vscode.window.withProgress` that passes only the + * progress reporter, hiding the unused cancellation token. + */ +export function withProgress( + options: vscode.ProgressOptions, + fn: ( + progress: vscode.Progress<{ message?: string; increment?: number }>, + ) => Promise, +): Thenable { + return vscode.window.withProgress(options, (progress) => fn(progress)); +} diff --git a/src/promptUtils.ts b/src/promptUtils.ts index f5b1360e..049b249c 100644 --- a/src/promptUtils.ts +++ b/src/promptUtils.ts @@ -4,6 +4,7 @@ import * as vscode from "vscode"; import { type CoderApi } from "./api/coderApi"; import { type MementoManager } from "./core/mementoManager"; import { OAuthMetadataClient } from "./oauth/metadataClient"; +import { withProgress } from "./progress"; type AuthMethod = "oauth" | "legacy"; @@ -147,17 +148,12 @@ export async function maybeAskAuthMethod( } // Check if server supports OAuth with progress indication - const supportsOAuth = await vscode.window.withProgress( + const supportsOAuth = await withProgress( { location: vscode.ProgressLocation.Notification, title: "Checking authentication methods", - cancellable: false, - }, - async () => { - return await OAuthMetadataClient.checkOAuthSupport( - client.getAxiosInstance(), - ); }, + () => OAuthMetadataClient.checkOAuthSupport(client.getAxiosInstance()), ); if (supportsOAuth) { diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index e1c2e365..5994feeb 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -1,7 +1,7 @@ import { mkdir, readFile, rename, stat, writeFile } from "node:fs/promises"; import path from "node:path"; -import { countSubstring } from "../util"; +import { countSubstring, tempFilePath } from "../util"; class SSHConfigBadFormat extends Error {} @@ -308,28 +308,30 @@ export class SSHConfig { mode: 0o700, // only owner has rwx permission, not group or everyone. recursive: true, }); - const randSuffix = Math.random().toString(36).substring(8); const fileName = path.basename(this.filePath); const dirName = path.dirname(this.filePath); - const tempFilePath = `${dirName}/.${fileName}.vscode-coder-tmp.${randSuffix}`; + const tempPath = tempFilePath( + `${dirName}/.${fileName}`, + "vscode-coder-tmp", + ); try { - await this.fileSystem.writeFile(tempFilePath, this.getRaw(), { + await this.fileSystem.writeFile(tempPath, this.getRaw(), { mode: existingMode, encoding: "utf-8", }); } catch (err) { throw new Error( - `Failed to write temporary SSH config file at ${tempFilePath}: ${err instanceof Error ? err.message : String(err)}. ` + + `Failed to write temporary SSH config file at ${tempPath}: ${err instanceof Error ? err.message : String(err)}. ` + `Please check your disk space, permissions, and that the directory exists.`, { cause: err }, ); } try { - await this.fileSystem.rename(tempFilePath, this.filePath); + await this.fileSystem.rename(tempPath, this.filePath); } catch (err) { throw new Error( - `Failed to rename temporary SSH config file at ${tempFilePath} to ${this.filePath}: ${ + `Failed to rename temporary SSH config file at ${tempPath} to ${this.filePath}: ${ err instanceof Error ? err.message : String(err) }. Please check your disk space, permissions, and that the directory exists.`, { cause: err }, diff --git a/src/util.ts b/src/util.ts index 35492eea..01d2db16 100644 --- a/src/util.ts +++ b/src/util.ts @@ -154,3 +154,12 @@ export function escapeCommandArg(arg: string): string { const escapedString = arg.replaceAll('"', String.raw`\"`); return `"${escapedString}"`; } + +/** + * Generate a temporary file path by appending a suffix with a random component. + * The suffix describes the purpose of the temp file (e.g. "temp", "old"). + * Example: tempFilePath("/a/b", "temp") → "/a/b.temp-k7x3f9qw" + */ +export function tempFilePath(basePath: string, suffix: string): string { + return `${basePath}.${suffix}-${crypto.randomUUID().substring(0, 8)}`; +} diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 218fa1c5..ac0bc998 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -76,11 +76,14 @@ function stubExecFileAbortable() { _bin: string, _args: string[], opts: ExecFileOptions, + cb: ExecFileCallback, ) => { + const err = new Error("The operation was aborted"); + err.name = "AbortError"; if (opts.signal?.aborted) { - const err = new Error("The operation was aborted"); - err.name = "AbortError"; - throw err; + cb(err); + } else { + opts.signal?.addEventListener("abort", () => cb(err)); } }) as unknown as typeof execFile); } @@ -163,7 +166,7 @@ describe("CliCredentialManager", () => { vi.clearAllMocks(); vol.reset(); vi.mocked(isKeyringEnabled).mockReturnValue(false); - vi.mocked(cliUtils.version).mockResolvedValue("2.29.0"); + vi.mocked(cliUtils.version).mockResolvedValue("2.31.0"); }); describe("storeToken", () => { @@ -315,6 +318,17 @@ describe("CliCredentialManager", () => { expect(execFile).not.toHaveBeenCalled(); }); + it("returns undefined when CLI version too old for token read", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + // 2.30 supports keyringAuth but not keyringTokenRead (requires 2.31+) + vi.mocked(cliUtils.version).mockResolvedValueOnce("2.30.0"); + stubExecFile({ stdout: "my-token" }); + const { manager } = setup(); + + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(execFile).not.toHaveBeenCalled(); + }); + it("passes timeout to execFile", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ stdout: "token" }); diff --git a/test/unit/featureSet.test.ts b/test/unit/featureSet.test.ts index b8dfad08..e7cba29a 100644 --- a/test/unit/featureSet.test.ts +++ b/test/unit/featureSet.test.ts @@ -40,4 +40,19 @@ describe("check version support", () => { featureSetForVersion(semver.parse("0.0.0-devel+abc123")).keyringAuth, ).toBeTruthy(); }); + it("keyring token read", () => { + ["v2.30.0", "v2.29.0", "v2.28.0", "v1.0.0"].forEach((v: string) => { + expect( + featureSetForVersion(semver.parse(v)).keyringTokenRead, + ).toBeFalsy(); + }); + ["v2.31.0", "v2.31.1", "v2.32.0", "v3.0.0"].forEach((v: string) => { + expect( + featureSetForVersion(semver.parse(v)).keyringTokenRead, + ).toBeTruthy(); + }); + expect( + featureSetForVersion(semver.parse("0.0.0-devel+abc123")).keyringTokenRead, + ).toBeTruthy(); + }); }); diff --git a/test/unit/progress.test.ts b/test/unit/progress.test.ts index 79bc255c..fbe8b8dd 100644 --- a/test/unit/progress.test.ts +++ b/test/unit/progress.test.ts @@ -1,7 +1,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; -import { withCancellableProgress, type ProgressContext } from "@/progress"; +import { + withCancellableProgress, + withProgress, + type ProgressContext, +} from "@/progress"; function mockWithProgress(opts?: { cancelImmediately?: boolean }) { const dispose = vi.fn(); @@ -117,3 +121,41 @@ describe("withCancellableProgress", () => { ); }); }); + +describe("withProgress", () => { + beforeEach(() => { + mockWithProgress(); + }); + + it("returns the resolved value", async () => { + const result = await withProgress( + { location: vscode.ProgressLocation.Notification, title: "test" }, + () => Promise.resolve(42), + ); + + expect(result).toBe(42); + }); + + it("passes progress reporter to callback", async () => { + let hasReport = false; + + await withProgress( + { location: vscode.ProgressLocation.Notification, title: "test" }, + (progress) => { + hasReport = typeof progress.report === "function"; + return Promise.resolve(); + }, + ); + + expect(hasReport).toBe(true); + }); + + it("propagates errors to the caller", async () => { + await expect( + withProgress( + { location: vscode.ProgressLocation.Notification, title: "test" }, + () => Promise.reject(new Error("boom")), + ), + ).rejects.toThrow("boom"); + }); +}); diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index a8e7aef8..eab774dc 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -10,7 +10,8 @@ import { // setting it to a different path makes it easier to test // and makes mistakes abundantly clear. const sshFilePath = "/Path/To/UserHomeDir/.sshConfigDir/sshConfigFile"; -const sshTempFilePathExpr = `^/Path/To/UserHomeDir/\\.sshConfigDir/\\.sshConfigFile\\.vscode-coder-tmp\\.[a-z0-9]+$`; +const sshTempFilePrefix = + "/Path/To/UserHomeDir/.sshConfigDir/.sshConfigFile.vscode-coder-tmp-"; const mockFileSystem = { mkdir: vi.fn(), @@ -53,7 +54,7 @@ Host coder-vscode--* expect.anything(), ); expect(mockFileSystem.writeFile).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), expectedOutput, expect.objectContaining({ encoding: "utf-8", @@ -61,7 +62,7 @@ Host coder-vscode--* }), ); expect(mockFileSystem.rename).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), sshFilePath, ); }); @@ -95,7 +96,7 @@ Host coder-vscode.dev.coder.com--* expect.anything(), ); expect(mockFileSystem.writeFile).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), expectedOutput, expect.objectContaining({ encoding: "utf-8", @@ -103,7 +104,7 @@ Host coder-vscode.dev.coder.com--* }), ); expect(mockFileSystem.rename).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), sshFilePath, ); }); @@ -142,7 +143,7 @@ Host coder-vscode.dev.coder.com--* # --- END CODER VSCODE dev.coder.com ---`; expect(mockFileSystem.writeFile).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), expectedOutput, { encoding: "utf-8", @@ -150,7 +151,7 @@ Host coder-vscode.dev.coder.com--* }, ); expect(mockFileSystem.rename).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), sshFilePath, ); }); @@ -215,7 +216,7 @@ Host * SetEnv TEST=1`; expect(mockFileSystem.writeFile).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), expectedOutput, { encoding: "utf-8", @@ -223,7 +224,7 @@ Host * }, ); expect(mockFileSystem.rename).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), sshFilePath, ); }); @@ -269,7 +270,7 @@ Host coder-vscode.dev.coder.com--* # --- END CODER VSCODE dev.coder.com ---`; expect(mockFileSystem.writeFile).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), expectedOutput, { encoding: "utf-8", @@ -277,7 +278,7 @@ Host coder-vscode.dev.coder.com--* }, ); expect(mockFileSystem.rename).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), sshFilePath, ); }); @@ -312,7 +313,7 @@ Host coder-vscode.dev.coder.com--* # --- END CODER VSCODE dev.coder.com ---`; expect(mockFileSystem.writeFile).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), expectedOutput, { encoding: "utf-8", @@ -320,7 +321,7 @@ Host coder-vscode.dev.coder.com--* }, ); expect(mockFileSystem.rename).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), sshFilePath, ); }); @@ -595,7 +596,7 @@ Host afterconfig }); expect(mockFileSystem.writeFile).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), expectedOutput, { encoding: "utf-8", @@ -603,7 +604,7 @@ Host afterconfig }, ); expect(mockFileSystem.rename).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), sshFilePath, ); }); @@ -652,7 +653,7 @@ Host coder-vscode.dev.coder.com--* expect.anything(), ); expect(mockFileSystem.writeFile).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), expectedOutput, expect.objectContaining({ encoding: "utf-8", @@ -660,7 +661,7 @@ Host coder-vscode.dev.coder.com--* }), ); expect(mockFileSystem.rename).toBeCalledWith( - expect.stringMatching(sshTempFilePathExpr), + expect.stringContaining(sshTempFilePrefix), sshFilePath, ); }); diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index 441ecd8c..cf887ca5 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -7,6 +7,7 @@ import { expandPath, findPort, parseRemoteAuthority, + tempFilePath, toSafeHost, } from "@/util"; @@ -238,3 +239,24 @@ describe("findPort", () => { expect(findPort(log)).toBe(3333); }); }); + +describe("tempFilePath", () => { + it("prepends basePath and suffix before the random part", () => { + const result = tempFilePath("/a/b/file", "temp"); + const prefix = "/a/b/file.temp-"; + expect(result.startsWith(prefix)).toBe(true); + // prefix(15) + uuid(8) = 23 + expect(result).toHaveLength(prefix.length + 8); + }); + + it("generates different paths on each call", () => { + const a = tempFilePath("/x", "tmp"); + const b = tempFilePath("/x", "tmp"); + expect(a).not.toBe(b); + }); + + it("uses the provided suffix", () => { + const result = tempFilePath("/base", "old"); + expect(result.startsWith("/base.old-")).toBe(true); + }); +}); From f3acedff3265f4adf8a3e4197989f83fbceaf0e9 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 11 Mar 2026 14:45:37 +0300 Subject: [PATCH 2/2] Handle review comments --- src/core/cliCredentialManager.ts | 6 ++-- src/core/container.ts | 14 ++++------ src/featureSet.ts | 48 ++++++++++++++++---------------- test/unit/featureSet.test.ts | 14 ++++++---- 4 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 28808ead..5b723637 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -5,7 +5,7 @@ import { promisify } from "node:util"; import * as semver from "semver"; import { isKeyringEnabled } from "../cliConfig"; -import { type FeatureSet, featureSetForVersion } from "../featureSet"; +import { featureSetForVersion } from "../featureSet"; import { getHeaderArgs } from "../headers"; import { tempFilePath, toSafeHost } from "../util"; @@ -19,6 +19,8 @@ import type { PathResolver } from "./pathResolver"; const execFileAsync = promisify(execFile); +type KeyringFeature = "keyringAuth" | "keyringTokenRead"; + const EXEC_TIMEOUT_MS = 60_000; const EXEC_LOG_INTERVAL_MS = 5_000; @@ -147,7 +149,7 @@ export class CliCredentialManager { private async resolveKeyringBinary( url: string, configs: Pick, - feature: keyof Pick, + feature: KeyringFeature, ): Promise { if (!isKeyringEnabled(configs)) { return undefined; diff --git a/src/core/container.ts b/src/core/container.ts index d50fa533..ce8ca887 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -38,24 +38,21 @@ export class ServiceContainer implements vscode.Disposable { this.logger, ); // Circular ref: cliCredentialManager ↔ cliManager. The resolver - // closure captures `ref` which starts undefined, so it must only - // be called after construction completes. - const cliManagerRef: { current: CliManager | undefined } = { - current: undefined, - }; + // closure captures `this` by reference, so `this.cliManager` is + // available when the closure is called (after construction). this.cliCredentialManager = new CliCredentialManager( this.logger, async (url) => { - if (!cliManagerRef.current) { + if (!this.cliManager) { throw new Error( "BinaryResolver called before CliManager was initialised", ); } try { - return await cliManagerRef.current.locateBinary(url); + return await this.cliManager.locateBinary(url); } catch { const client = CoderApi.create(url, "", this.logger); - return cliManagerRef.current.fetchBinary(client); + return this.cliManager.fetchBinary(client); } }, this.pathResolver, @@ -65,7 +62,6 @@ export class ServiceContainer implements vscode.Disposable { this.pathResolver, this.cliCredentialManager, ); - cliManagerRef.current = this.cliManager; this.contextManager = new ContextManager(context); this.loginCoordinator = new LoginCoordinator( this.secretsManager, diff --git a/src/featureSet.ts b/src/featureSet.ts index 6d9fdcfc..76c596ce 100644 --- a/src/featureSet.ts +++ b/src/featureSet.ts @@ -9,6 +9,20 @@ export interface FeatureSet { keyringTokenRead: boolean; } +/** + * True when the CLI version is at least `minVersion`, or is a dev build. + * Returns false for null (unknown) versions. + */ +function versionAtLeast( + version: semver.SemVer | null, + minVersion: string, +): boolean { + if (!version) { + return false; + } + return version.compare(minVersion) >= 0 || version.prerelease[0] === "devel"; +} + /** * Builds and returns a FeatureSet object for a given coder version. */ @@ -23,29 +37,15 @@ export function featureSetForVersion( version?.prerelease.length === 0 ), - // CLI versions before 2.3.3 don't support the --log-dir flag! - // If this check didn't exist, VS Code connections would fail on - // older versions because of an unknown CLI argument. - proxyLogDirectory: - (version?.compare("2.3.3") ?? 0) > 0 || - version?.prerelease[0] === "devel", - wildcardSSH: - (version ? version.compare("2.19.0") : -1) >= 0 || - version?.prerelease[0] === "devel", - - // The --reason flag was added to `coder start` in 2.25.0 - buildReason: - (version?.compare("2.25.0") ?? 0) >= 0 || - version?.prerelease[0] === "devel", - - // Keyring-backed token storage was added in CLI 2.29.0 - keyringAuth: - (version?.compare("2.29.0") ?? 0) >= 0 || - version?.prerelease[0] === "devel", - - // `coder login token` for reading tokens from the keyring was added in 2.31.0 - keyringTokenRead: - (version?.compare("2.31.0") ?? 0) >= 0 || - version?.prerelease[0] === "devel", + // --log-dir flag for proxy logs; vscodessh fails if unsupported + proxyLogDirectory: versionAtLeast(version, "2.4.0"), + // Wildcard SSH host matching + wildcardSSH: versionAtLeast(version, "2.19.0"), + // --reason flag for `coder start` + buildReason: versionAtLeast(version, "2.25.0"), + // Keyring-backed token storage via `coder login` + keyringAuth: versionAtLeast(version, "2.29.0"), + // `coder login token` for reading tokens from the keyring + keyringTokenRead: versionAtLeast(version, "2.31.0"), }; } diff --git a/test/unit/featureSet.test.ts b/test/unit/featureSet.test.ts index e7cba29a..3beeac3b 100644 --- a/test/unit/featureSet.test.ts +++ b/test/unit/featureSet.test.ts @@ -5,12 +5,14 @@ import { featureSetForVersion } from "@/featureSet"; describe("check version support", () => { it("has logs", () => { - ["v1.3.3+e491217", "v2.3.3+e491217"].forEach((v: string) => { - expect( - featureSetForVersion(semver.parse(v)).proxyLogDirectory, - ).toBeFalsy(); - }); - ["v2.3.4+e491217", "v5.3.4+e491217", "v5.0.4+e491217"].forEach( + ["v1.3.3+e491217", "v2.3.3+e491217", "v2.3.9+e491217"].forEach( + (v: string) => { + expect( + featureSetForVersion(semver.parse(v)).proxyLogDirectory, + ).toBeFalsy(); + }, + ); + ["v2.4.0+e491217", "v5.3.4+e491217", "v5.0.4+e491217"].forEach( (v: string) => { expect( featureSetForVersion(semver.parse(v)).proxyLogDirectory,