refactor: derive safeHostname internally from url instead of passing both

Make url the single source of truth for deployment identity. fetchBinary,
configure, and clearCredentials now derive safeHostname via toSafeHost()
internally, eliminating redundant parameters and preventing inconsistency.
BinaryResolver and CliCredentialManager methods take url string instead of
Deployment object.

Author: EhabY

src/commands.ts

@@ -214,13 +214,12 @@ export class Commands {
 		this.logger.debug("Logging out");
 
 		const deployment = this.deploymentManager.getCurrentDeployment();
-		const safeHostname = deployment?.safeHostname;
 
 		await this.deploymentManager.clearDeployment();
 
-		if (safeHostname) {
-			await this.cliManager.clearCredentials(safeHostname);
-			await this.secretsManager.clearAllAuthData(safeHostname);
+		if (deployment) {
+			await this.cliManager.clearCredentials(deployment.url);
+			await this.secretsManager.clearAllAuthData(deployment.safeHostname);
 		}
 
 		vscode.window
@@ -287,7 +286,10 @@ export class Commands {
 
 			if (selected.hostnames.length === 1) {
 				const selectedHostname = selected.hostnames[0];
-				await this.cliManager.clearCredentials(selectedHostname);
+				const auth = await this.secretsManager.getSessionAuth(selectedHostname);
+				if (auth?.url) {
+					await this.cliManager.clearCredentials(auth.url);
+				}
 				await this.secretsManager.clearAllAuthData(selectedHostname);
 				this.logger.info("Removed credentials for", selectedHostname);
 				vscode.window.showInformationMessage(
@@ -306,7 +308,10 @@ export class Commands {
 				if (confirm === "Remove All") {
 					await Promise.all(
 						selected.hostnames.map(async (h) => {
-							await this.cliManager.clearCredentials(h);
+							const auth = await this.secretsManager.getSessionAuth(h);
+							if (auth?.url) {
+								await this.cliManager.clearCredentials(auth.url);
+							}
 							await this.secretsManager.clearAllAuthData(h);
 						}),
 					);
@@ -455,7 +460,6 @@ export class Commands {
 					const safeHost = toSafeHost(baseUrl);
 					const binary = await this.cliManager.fetchBinary(
 						this.extensionClient,
-						safeHost,
 					);
 
 					const version = semver.parse(await cliUtils.version(binary));

src/core/cliCredentialManager.ts

@@ -9,6 +9,12 @@ import type { Logger } from "../logging/logger";
 
 const execFileAsync = promisify(execFile);
 
+/**
+ * Resolves a CLI binary path for a given deployment URL, fetching/downloading
+ * if needed. Returns the path or throws if unavailable.
+ */
+export type BinaryResolver = (url: string) => Promise<string>;
+
 /**
  * Returns true on platforms where the OS keyring is supported (macOS, Windows).
  */
@@ -18,9 +24,15 @@ export function isKeyringSupported(): boolean {
 
 /**
  * Delegates credential storage to the Coder CLI to keep the credentials in sync.
+ *
+ * For operations that don't have a binary path available (readToken, deleteToken),
+ * uses the injected BinaryResolver to fetch/locate the CLI binary.
  */
 export class CliCredentialManager {
-	constructor(private readonly logger: Logger) {}
+	constructor(
+		private readonly logger: Logger,
+		private readonly resolveBinary: BinaryResolver,
+	) {}
 
 	/**
 	 * Store a token by running:
@@ -29,7 +41,7 @@ export class CliCredentialManager {
 	 * The token is passed via environment variable so it never appears in
 	 * process argument lists.
 	 */
-	async storeToken(
+	public async storeToken(
 		binPath: string,
 		url: string,
 		token: string,
@@ -56,13 +68,21 @@ export class CliCredentialManager {
 	 * Read a token by running:
 	 *   <bin> login token --url <url>
 	 *
-	 * Returns trimmed stdout, or undefined on any failure.
+	 * Resolves the CLI binary automatically. Returns trimmed stdout,
+	 * or undefined if the binary can't be resolved or the CLI returns no token.
 	 */
-	async readToken(
-		binPath: string,
+	public async readToken(
 		url: string,
 		configs: Pick<WorkspaceConfiguration, "get">,
 	): Promise<string | undefined> {
+		let binPath: string;
+		try {
+			binPath = await this.resolveBinary(url);
+		} catch (error) {
+			this.logger.warn("Could not resolve CLI binary for token read:", error);
+			return undefined;
+		}
+
 		const args = [...getHeaderArgs(configs), "login", "token", "--url", url];
 		try {
 			const { stdout } = await execFileAsync(binPath, args);
@@ -73,4 +93,32 @@ export class CliCredentialManager {
 			return undefined;
 		}
 	}
+
+	/**
+	 * Delete a token by running:
+	 *   <bin> logout --url <url>
+	 *
+	 * Resolves the CLI binary automatically. Best-effort: logs warnings
+	 * on failure but never throws.
+	 */
+	async deleteToken(
+		url: string,
+		configs: Pick<WorkspaceConfiguration, "get">,
+	): Promise<void> {
+		let binPath: string;
+		try {
+			binPath = await this.resolveBinary(url);
+		} catch (error) {
+			this.logger.warn("Could not resolve CLI binary for token delete:", error);
+			return;
+		}
+
+		const args = [...getHeaderArgs(configs), "logout", "--url", url, "--yes"];
+		try {
+			await execFileAsync(binPath, args);
+			this.logger.info("Deleted token via CLI for", url);
+		} catch (error) {
+			this.logger.warn("Failed to delete token via CLI:", error);
+		}
+	}
 }

src/core/cliManager.ts

@@ -10,8 +10,9 @@ import * as semver from "semver";
 import * as vscode from "vscode";
 
 import { errToStr } from "../api/api-helper";
-import { shouldUseKeyring } from "../cliConfig";
+import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig";
 import * as pgp from "../pgp";
+import { toSafeHost } from "../util";
 import { vscodeProposed } from "../vscodeProposed";
 
 import { BinaryLock } from "./binaryLock";
@@ -49,10 +50,12 @@ export class CliManager {
 	 * unable to download a working binary, whether because of network issues or
 	 * downloads being disabled.
 	 */
-	public async fetchBinary(
-		restClient: Api,
-		safeHostname: string,
-	): Promise<string> {
+	public async fetchBinary(restClient: Api): Promise<string> {
+		const baseUrl = restClient.getAxiosInstance().defaults.baseURL;
+		if (!baseUrl) {
+			throw new Error("REST client has no base URL configured");
+		}
+		const safeHostname = toSafeHost(baseUrl);
 		const cfg = vscode.workspace.getConfiguration("coder");
 		// Settings can be undefined when set to their defaults (true in this case),
 		// so explicitly check against false.
@@ -719,7 +722,6 @@ export class CliManager {
 	 * authentication) but the URL must be a non-empty string.
 	 */
 	public async configure(
-		safeHostname: string,
 		url: string,
 		token: string,
 		featureSet: FeatureSet,
@@ -728,6 +730,7 @@ export class CliManager {
 		if (!url) {
 			throw new Error("URL is required to configure the CLI");
 		}
+		const safeHostname = toSafeHost(url);
 
 		const configs = vscode.workspace.getConfiguration();
 		if (shouldUseKeyring(configs, featureSet)) {
@@ -765,12 +768,17 @@ export class CliManager {
 	}
 
 	/**
-	 * Remove file-based credentials for a deployment. Keyring entries are not
-	 * removed here because deleting requires the CLI binary, which may not be
-	 * available at logout time. This is fine: stale keyring entries are harmless
-	 * since the CLI overwrites them on next `coder login`.
+	 * Remove credentials for a deployment. Clears both file-based credentials
+	 * and keyring entries (via `coder logout`). Keyring deletion is best-effort:
+	 * if it fails, file cleanup still runs.
 	 */
-	public async clearCredentials(safeHostname: string): Promise<void> {
+	public async clearCredentials(url: string): Promise<void> {
+		const safeHostname = toSafeHost(url);
+		const configs = vscode.workspace.getConfiguration();
+		if (isKeyringEnabled(configs)) {
+			await this.cliCredentialManager.deleteToken(url, configs);
+		}
+
 		const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname);
 		const urlPath = this.pathResolver.getUrlPath(safeHostname);
 		await Promise.all([

src/core/container.ts

@@ -1,5 +1,6 @@
 import * as vscode from "vscode";
 
+import { CoderApi } from "../api/coderApi";
 import { type Logger } from "../logging/logger";
 import { LoginCoordinator } from "../login/loginCoordinator";
 
@@ -36,7 +37,13 @@ export class ServiceContainer implements vscode.Disposable {
 			context.globalState,
 			this.logger,
 		);
-		this.cliCredentialManager = new CliCredentialManager(this.logger);
+		this.cliCredentialManager = new CliCredentialManager(
+			this.logger,
+			async (url) => {
+				const client = CoderApi.create(url, "", this.logger);
+				return this.cliManager.fetchBinary(client);
+			},
+		);
 		this.cliManager = new CliManager(
 			this.logger,
 			this.pathResolver,
@@ -48,7 +55,6 @@ export class ServiceContainer implements vscode.Disposable {
 			this.mementoManager,
 			this.logger,
 			this.cliCredentialManager,
-			this.cliManager,
 			context.extension.id,
 		);
 	}

src/login/loginCoordinator.ts

@@ -4,7 +4,6 @@ import * as vscode from "vscode";
 
 import { CoderApi } from "../api/coderApi";
 import { needToken } from "../api/utils";
-import { isKeyringEnabled } from "../cliConfig";
 import { CertificateError } from "../error/certificateError";
 import { OAuthAuthorizer } from "../oauth/authorizer";
 import { buildOAuthTokenData } from "../oauth/utils";
@@ -14,7 +13,6 @@ import { vscodeProposed } from "../vscodeProposed";
 import type { User } from "coder/site/src/api/typesGenerated";
 
 import type { CliCredentialManager } from "../core/cliCredentialManager";
-import type { CliManager } from "../core/cliManager";
 import type { MementoManager } from "../core/mementoManager";
 import type { OAuthTokenData, SecretsManager } from "../core/secretsManager";
 import type { Deployment } from "../deployment/types";
@@ -43,7 +41,6 @@ export class LoginCoordinator implements vscode.Disposable {
 		private readonly mementoManager: MementoManager,
 		private readonly logger: Logger,
 		private readonly cliCredentialManager: CliCredentialManager,
-		private readonly cliManager: CliManager,
 		extensionId: string,
 	) {
 		this.oauthAuthorizer = new OAuthAuthorizer(
@@ -306,52 +303,15 @@ export class LoginCoordinator implements vscode.Disposable {
 	}
 
 	/**
-	 * Read a token from the CLI keyring. Fetches the CLI binary first (using
-	 * an unauthenticated client) so the binary is available for keyring reads.
-	 * Returns undefined if the keyring is disabled, the binary can't be fetched,
-	 * or the CLI returns no token.
+	 * Read a token from the CLI keyring.
 	 */
 	private async getCliKeyringToken(
 		deployment: Deployment,
 	): Promise<string | undefined> {
-		if (!isKeyringEnabled(vscode.workspace.getConfiguration())) {
-			return undefined;
-		}
-		try {
-			const binPath = await this.ensureBinaryForKeyring(
-				deployment.url,
-				deployment.safeHostname,
-			);
-			if (!binPath) {
-				return undefined;
-			}
-			return await this.cliCredentialManager.readToken(
-				binPath,
-				deployment.url,
-				vscode.workspace.getConfiguration(),
-			);
-		} catch (error) {
-			this.logger.warn("Failed to read token from CLI keyring", error);
-			return undefined;
-		}
-	}
-
-	/**
-	 * Fetch or locate a CLI binary for the given deployment. Uses an
-	 * unauthenticated client since getBuildInfo and binary downloads
-	 * don't require auth.
-	 */
-	private async ensureBinaryForKeyring(
-		url: string,
-		safeHostname: string,
-	): Promise<string | undefined> {
-		try {
-			const client = CoderApi.create(url, "", this.logger);
-			return await this.cliManager.fetchBinary(client, safeHostname);
-		} catch (error) {
-			this.logger.warn("Could not fetch CLI binary for keyring read:", error);
-			return undefined;
-		}
+		return this.cliCredentialManager.readToken(
+			deployment.url,
+			vscode.workspace.getConfiguration(),
+		);
 	}
 
 	/**

src/remote/remote.ts

@@ -215,21 +215,15 @@ export class Remote {
 			if (
 				this.extensionContext.extensionMode === vscode.ExtensionMode.Production
 			) {
-				binaryPath = await this.cliManager.fetchBinary(
-					workspaceClient,
-					parts.safeHostname,
-				);
+				binaryPath = await this.cliManager.fetchBinary(workspaceClient);
 			} else {
 				try {
 					// In development, try to use `/tmp/coder` as the binary path.
 					// This is useful for debugging with a custom bin!
 					binaryPath = path.join(os.tmpdir(), "coder");
 					await fs.stat(binaryPath);
 				} catch {
-					binaryPath = await this.cliManager.fetchBinary(
-						workspaceClient,
-						parts.safeHostname,
-					);
+					binaryPath = await this.cliManager.fetchBinary(workspaceClient);
 				}
 			}
 
@@ -248,7 +242,6 @@ export class Remote {
 			// Write token to keyring or file (after CLI version is known)
 			if (baseUrlRaw && token !== undefined) {
 				await this.cliManager.configure(
-					parts.safeHostname,
 					baseUrlRaw,
 					token,
 					featureSet,
@@ -265,7 +258,6 @@ export class Remote {
 						if (auth?.url) {
 							try {
 								await this.cliManager.configure(
-									parts.safeHostname,
 									auth.url,
 									auth.token,
 									featureSet,

test/mocks/testHelpers.ts

@@ -380,6 +380,7 @@ export function createMockCliCredentialManager(): CliCredentialManager {
 	return {
 		storeToken: vi.fn().mockResolvedValue(undefined),
 		readToken: vi.fn().mockResolvedValue(undefined),
+		deleteToken: vi.fn().mockResolvedValue(undefined),
 	} as unknown as CliCredentialManager;
 }