refactor: improve keyring support reliability and reduce duplication (#831)

- Separate keyringTokenRead (>= 2.31) from keyringAuth (>= 2.29) feature
flags
- 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
- Fix stubExecFileAbortable to handle non-pre-aborted signals
- Replace regex test matchers with structural assertions

Author: EhabY

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<void> {
 		// 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);

src/core/cliCredentialManager.ts

@@ -7,7 +7,7 @@ import * as semver from "semver";
 import { isKeyringEnabled } from "../cliConfig";
 import { featureSetForVersion } from "../featureSet";
 import { getHeaderArgs } from "../headers";
-import { toSafeHost } from "../util";
+import { tempFilePath, toSafeHost } from "../util";
 
 import * as cliUtils from "./cliUtils";
 
@@ -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;
 
@@ -59,7 +61,11 @@ export class CliCredentialManager {
 		configs: Pick<WorkspaceConfiguration, "get">,
 		signal?: AbortSignal,
 	): Promise<void> {
-		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 +97,20 @@ export class CliCredentialManager {
 		url: string,
 		configs: Pick<WorkspaceConfiguration, "get">,
 	): Promise<string | undefined> {
-		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,22 +140,23 @@ 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).
 	 */
 	private async resolveKeyringBinary(
 		url: string,
 		configs: Pick<WorkspaceConfiguration, "get">,
+		feature: KeyringFeature,
 	): Promise<string | undefined> {
 		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 +227,7 @@ export class CliCredentialManager {
 	): Promise<void> {
 		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 +253,7 @@ export class CliCredentialManager {
 		content: string,
 	): Promise<void> {
 		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);

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<string> {
 		const safeHostname = toSafeHost(url);
@@ -244,8 +244,7 @@ export class CliManager {
 		binPath: string,
 		tempFile: string,
 	): Promise<void> {
-		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<string> {
 		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);

src/core/container.ts

@@ -37,11 +37,17 @@ 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 `this` by reference, so `this.cliManager` is
+		// available when the closure is called (after construction).
 		this.cliCredentialManager = new CliCredentialManager(
 			this.logger,
 			async (url) => {
+				if (!this.cliManager) {
+					throw new Error(
+						"BinaryResolver called before CliManager was initialised",
+					);
+				}
 				try {
 					return await this.cliManager.locateBinary(url);
 				} catch {

src/featureSet.ts

@@ -6,6 +6,21 @@ export interface FeatureSet {
 	wildcardSSH: boolean;
 	buildReason: boolean;
 	keyringAuth: boolean;
+	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";
 }
 
 /**
@@ -22,24 +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",
+		// --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"),
 	};
 }

src/progress.ts

@@ -39,3 +39,17 @@ export function withCancellableProgress<T>(
 		},
 	);
 }
+
+/**
+ * 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<T>(
+	options: vscode.ProgressOptions,
+	fn: (
+		progress: vscode.Progress<{ message?: string; increment?: number }>,
+	) => Promise<T>,
+): Thenable<T> {
+	return vscode.window.withProgress(options, (progress) => fn(progress));
+}

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) {

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 },

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)}`;
+}

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" });