Handle review comments

Author: EhabY

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<WorkspaceConfiguration, "get">,
-		feature: keyof Pick<FeatureSet, "keyringAuth" | "keyringTokenRead">,
+		feature: KeyringFeature,
 	): Promise<string | undefined> {
 		if (!isKeyringEnabled(configs)) {
 			return undefined;

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,

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

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,