Address review comments (zach)

Author: EhabY

src/cliConfig.ts

@@ -1,10 +1,11 @@
-import * as vscode from "vscode";
-
-import { type FeatureSet } from "./featureSet";
 import { getHeaderArgs } from "./headers";
 import { isKeyringSupported } from "./keyringStore";
 import { escapeCommandArg } from "./util";
 
+import type { WorkspaceConfiguration } from "vscode";
+
+import type { FeatureSet } from "./featureSet";
+
 export type CliAuth =
 	| { mode: "global-config"; configDir: string }
 	| { mode: "url"; url: string };
@@ -13,7 +14,7 @@ export type CliAuth =
  * Returns the raw global flags from user configuration.
  */
 export function getGlobalFlagsRaw(
-	configs: Pick<vscode.WorkspaceConfiguration, "get">,
+	configs: Pick<WorkspaceConfiguration, "get">,
 ): string[] {
 	return configs.get<string[]>("coder.globalFlags", []);
 }
@@ -23,7 +24,7 @@ export function getGlobalFlagsRaw(
  * Includes either `--global-config` or `--url` depending on the auth mode.
  */
 export function getGlobalFlags(
-	configs: Pick<vscode.WorkspaceConfiguration, "get">,
+	configs: Pick<WorkspaceConfiguration, "get">,
 	auth: CliAuth,
 ): string[] {
 	const authFlags =
@@ -39,28 +40,37 @@ export function getGlobalFlags(
 	];
 }
 
+/**
+ * Returns true when the user has keyring enabled and the platform supports it.
+ */
+export function isKeyringEnabled(
+	configs: Pick<WorkspaceConfiguration, "get">,
+): boolean {
+	return isKeyringSupported() && configs.get<boolean>("coder.useKeyring", true);
+}
+
 /**
  * Single source of truth: should the extension use the OS keyring for this session?
  * Requires CLI >= 2.29.0, macOS or Windows, and the coder.useKeyring setting enabled.
  */
-export function shouldUseKeyring(featureSet: FeatureSet): boolean {
-	return (
-		featureSet.keyringAuth &&
-		isKeyringSupported() &&
-		vscode.workspace.getConfiguration().get<boolean>("coder.useKeyring", true)
-	);
+export function shouldUseKeyring(
+	configs: Pick<WorkspaceConfiguration, "get">,
+	featureSet: FeatureSet,
+): boolean {
+	return isKeyringEnabled(configs) && featureSet.keyringAuth;
 }
 
 /**
  * Resolves how the CLI should authenticate: via the keyring (`--url`) or via
  * the global config directory (`--global-config`).
  */
 export function resolveCliAuth(
+	configs: Pick<WorkspaceConfiguration, "get">,
 	featureSet: FeatureSet,
 	deploymentUrl: string | undefined,
 	configDir: string,
 ): CliAuth {
-	if (shouldUseKeyring(featureSet) && deploymentUrl) {
+	if (shouldUseKeyring(configs, featureSet)) {
 		return { mode: "url", url: deploymentUrl };
 	}
 	return { mode: "global-config", configDir };
@@ -70,7 +80,7 @@ export function resolveCliAuth(
  * Returns SSH flags for the `coder ssh` command from user configuration.
  */
 export function getSshFlags(
-	configs: Pick<vscode.WorkspaceConfiguration, "get">,
+	configs: Pick<WorkspaceConfiguration, "get">,
 ): string[] {
 	// Make sure to match this default with the one in the package.json
 	return configs.get<string[]>("coder.sshFlags", ["--disable-autostart"]);

src/commands.ts

@@ -461,11 +461,9 @@ export class Commands {
 					const version = semver.parse(await cliUtils.version(binary));
 					const featureSet = featureSetForVersion(version);
 					const configDir = this.pathResolver.getGlobalConfigDir(safeHost);
-					const auth = resolveCliAuth(featureSet, baseUrl, configDir);
-					const globalFlags = getGlobalFlags(
-						vscode.workspace.getConfiguration(),
-						auth,
-					);
+					const configs = vscode.workspace.getConfiguration();
+					const auth = resolveCliAuth(configs, featureSet, baseUrl, configDir);
+					const globalFlags = getGlobalFlags(configs, auth);
 					terminal.sendText(
 						`${escapeCommandArg(binary)} ${globalFlags.join(" ")} ssh ${app.workspace_name}`,
 					);

src/core/cliManager.ts

@@ -10,8 +10,7 @@ import * as semver from "semver";
 import * as vscode from "vscode";
 
 import { errToStr } from "../api/api-helper";
-import { shouldUseKeyring } from "../cliConfig";
-import { isKeyringSupported, type KeyringStore } from "../keyringStore";
+import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig";
 import * as pgp from "../pgp";
 import { vscodeProposed } from "../vscodeProposed";
 
@@ -23,6 +22,7 @@ import type { Api } from "coder/site/src/api/api";
 import type { IncomingMessage } from "node:http";
 
 import type { FeatureSet } from "../featureSet";
+import type { KeyringStore } from "../keyringStore";
 import type { Logger } from "../logging/logger";
 
 import type { PathResolver } from "./pathResolver";
@@ -717,16 +717,29 @@ export class CliManager {
 		token: string | null,
 		featureSet: FeatureSet,
 	) {
-		if (shouldUseKeyring(featureSet) && url && token !== null) {
+		const configs = vscode.workspace.getConfiguration();
+		if (shouldUseKeyring(configs, featureSet) && url && token !== null) {
 			try {
 				this.keyringStore.setToken(url, token);
 				this.output.info("Stored token in OS keyring for", url);
 				return;
 			} catch (error) {
-				this.output.warn(
-					"Keyring write failed, falling back to file storage",
-					error,
-				);
+				this.output.error("Failed to store token in OS keyring:", error);
+				vscode.window
+					.showErrorMessage(
+						`Failed to store session token in OS keyring: ${errToStr(error)}. ` +
+							"Disable keyring storage in settings to use plaintext files instead.",
+						"Open Settings",
+					)
+					.then((action) => {
+						if (action === "Open Settings") {
+							vscode.commands.executeCommand(
+								"workbench.action.openSettings",
+								"coder.useKeyring",
+							);
+						}
+					});
+				throw error;
 			}
 		}
 		await Promise.all([
@@ -739,7 +752,7 @@ export class CliManager {
 	 * Remove credentials for a deployment from both keyring and file storage.
 	 */
 	public async clearCredentials(safeHostname: string): Promise<void> {
-		if (isKeyringSupported()) {
+		if (isKeyringEnabled(vscode.workspace.getConfiguration())) {
 			try {
 				this.keyringStore.deleteToken(safeHostname);
 				this.output.info("Removed keyring token for", safeHostname);

src/keyringStore.ts

@@ -1,4 +1,4 @@
-import { type Logger } from "./logging/logger";
+import type { Logger } from "./logging/logger";
 
 /** A single entry in the CLI's keyring credential map. */
 interface CredentialEntry {
@@ -90,13 +90,20 @@ export function isKeyringSupported(): boolean {
  * Encoding (must match CLI):
  *   macOS: base64-encoded JSON via setPassword/getPassword
  *   Windows: raw UTF-8 JSON bytes via setSecret/getSecret
+ *
+ * Concurrency: setToken does read-modify-write on a shared entry, so concurrent
+ * writes can clobber each other. Callers recover by re-writing on reconnection.
  */
 export class KeyringStore {
 	public constructor(
 		private readonly logger: Logger,
 		private readonly entryFactory: () => KeyringEntry = createDefaultEntry,
 	) {}
 
+	/**
+	 * Store a token under the host extracted from deploymentUrl (includes port).
+	 * The CLI stores map keys as host+port, so we must write in the same format.
+	 */
 	public setToken(deploymentUrl: string, token: string): void {
 		this.assertSupported();
 		const entry = this.entryFactory();
@@ -106,6 +113,10 @@ export class KeyringStore {
 		this.writeMap(entry, map);
 	}
 
+	/**
+	 * Look up a token by safeHostname (hostname without port). VS Code identifies
+	 * deployments by safeHostname, so findMapKey bridges to the CLI's host+port keys.
+	 */
 	public getToken(safeHostname: string): string | undefined {
 		this.assertSupported();
 		const entry = this.entryFactory();
@@ -114,6 +125,7 @@ export class KeyringStore {
 		return key !== undefined ? map[key].api_token : undefined;
 	}
 
+	/** Remove a token by safeHostname, matching the same way as getToken. */
 	public deleteToken(safeHostname: string): void {
 		this.assertSupported();
 		const entry = this.entryFactory();

src/login/loginCoordinator.ts

@@ -4,6 +4,7 @@ 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";
@@ -243,12 +244,7 @@ export class LoginCoordinator implements vscode.Disposable {
 		}
 
 		// Try keyring token (picks up tokens written by `coder login` in the terminal)
-		let keyringToken: string | undefined;
-		try {
-			keyringToken = this.keyringStore.getToken(deployment.safeHostname);
-		} catch (error) {
-			this.logger.warn("Failed to read token from keyring", error);
-		}
+		const keyringToken = this.getKeyringToken(deployment.safeHostname);
 		if (
 			keyringToken &&
 			keyringToken !== providedToken &&
@@ -307,6 +303,18 @@ export class LoginCoordinator implements vscode.Disposable {
 		}
 	}
 
+	private getKeyringToken(safeHostname: string): string | undefined {
+		if (!isKeyringEnabled(vscode.workspace.getConfiguration())) {
+			return undefined;
+		}
+		try {
+			return this.keyringStore.getToken(safeHostname);
+		} catch (error) {
+			this.logger.warn("Failed to read token from keyring", error);
+			return undefined;
+		}
+	}
+
 	/**
 	 * Shows auth error via dialog or logs it for autoLogin.
 	 */

src/remote/remote.ts

@@ -262,15 +262,22 @@ export class Remote {
 					async (auth) => {
 						workspaceClient.setCredentials(auth?.url, auth?.token);
 						if (auth?.url) {
-							await this.cliManager.configure(
-								parts.safeHostname,
-								auth.url,
-								auth.token,
-								featureSet,
-							);
-							this.logger.info(
-								"Updated CLI config with new token for remote deployment",
-							);
+							try {
+								await this.cliManager.configure(
+									parts.safeHostname,
+									auth.url,
+									auth.token,
+									featureSet,
+								);
+								this.logger.info(
+									"Updated CLI config with new token for remote deployment",
+								);
+							} catch (error) {
+								this.logger.error(
+									"Failed to update CLI config for remote deployment",
+									error,
+								);
+							}
 						}
 					},
 				),
@@ -279,7 +286,12 @@ export class Remote {
 			const configDir = this.pathResolver.getGlobalConfigDir(
 				parts.safeHostname,
 			);
-			const cliAuth = resolveCliAuth(featureSet, baseUrlRaw, configDir);
+			const cliAuth = resolveCliAuth(
+				vscode.workspace.getConfiguration(),
+				featureSet,
+				baseUrlRaw,
+				configDir,
+			);
 
 			// Server versions before v0.14.1 don't support the vscodessh command!
 			if (!featureSet.vscodessh) {