refactor: improve keyring support reliability and reduce duplication

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

@@ -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<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 +95,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 +138,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: keyof Pick<FeatureSet, "keyringAuth" | "keyringTokenRead">,
 	): 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 +225,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 +251,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,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,

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

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

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