chore: address review feedback

Author: EhabY

CONTRIBUTING.md

@@ -34,21 +34,25 @@ the current editor, and if so we delay activation to:
 3. Download the matching server binary to the client.
 4. Configure the binary with the URL and token, asking the user for them if they
    are missing. Each domain gets its own config directory.
-5. Write an entry for `coder-<editor>.<domain>--*` to `ssh-config` in the
-   extension's global storage directory.
-6. Add a per-editor `Include` block at the top of the user's SSH config.
+5. Write an entry for `coder-<editor>.<domain>--*` to a per-editor,
+   per-deployment file in a data directory shared by every editor, such as
+   `~/.local/share/coder.coder-remote/ssh/cursor--dev.coder.com.conf`.
+6. Keep a shared `Include` block at the top of the user's SSH config that
+   globs the whole directory. Every editor writes the identical block, so
+   concurrent writers converge on the same content. The `CODER INCLUDE <id>`
+   marker convention lets other Coder integrations recognize the block, since
+   Coder-managed includes route disjoint hosts and are order-independent.
 
 ```text
-# --- START CODER cursor ---
-Include "~/.config/Cursor/User/globalStorage/coder.coder-remote/ssh-config"
-# --- END CODER cursor ---
-
-# --- START CODER vscode ---
-Include "~/.config/Code/User/globalStorage/coder.coder-remote/ssh-config"
-# --- END CODER vscode ---
+# --- START CODER INCLUDE CODER-REMOTE ---
+# Managed by each editor's Coder extension (coder.coder-remote).
+# Moves back to the top on connect; override options via coder.sshConfig.
+Include "~/.local/share/coder.coder-remote/ssh/*.conf"
+# --- END CODER INCLUDE CODER-REMOTE ---
 ```
 
-Each included file contains only that editor's host entries:
+Each generated file contains only its own editor's host entries for one
+deployment:
 
 ```text
 Host coder-cursor.dev.coder.com--*

src/remote/remote.ts

@@ -40,10 +40,9 @@ import { getHeaderCommand } from "../settings/headers";
 import { escapeCommandArg, expandPath } from "../util";
 import {
 	type AuthorityParts,
-	classifyRemoteAuthority,
+	classifySshHost,
 	parseRemoteAuthority,
 	retargetRemoteAuthority,
-	toCurrentAuthorityHostPrefix,
 } from "../util/authority";
 import { createStatusBarItem } from "../util/statusBar";
 import { vscodeProposed } from "../vscodeProposed";
@@ -54,7 +53,6 @@ import { migrateAuthToSecretsStorage } from "./migration";
 import {
 	SshConfig,
 	type SshValues,
-	cleanupStaleSshConfigs,
 	mergeSshConfigValues,
 	parseCoderSshOptions,
 	parseSshConfig,
@@ -164,9 +162,11 @@ export class Remote {
 
 		// parseRemoteAuthority returned null for foreign hosts, so this is
 		// either the current editor's authority or a migratable legacy one.
-		if (classifyRemoteAuthority(parts) === "legacy") {
-			await this.migrateLegacyAuthority(remoteAuthority, startupMode);
-			return;
+		if (classifySshHost(parts.sshHost) === "legacy") {
+			if (await this.migrateLegacyAuthority(remoteAuthority, startupMode)) {
+				return;
+			}
+			// Not reopened: keep going so the legacy host still connects.
 		}
 
 		this.logger.info("Setting up remote connection", {
@@ -676,8 +676,7 @@ export class Remote {
 			this.logger.info("Updating SSH config...");
 			return await this.updateSSHConfig(
 				workspaceClient,
-				context.parts.safeHostname,
-				context.parts.sshHost,
+				context.parts,
 				binaryPath,
 				logDir,
 				featureSet,
@@ -727,39 +726,42 @@ export class Remote {
 		return undefined;
 	}
 
+	/**
+	 * Reopen the window on this editor's own authority. Returns false when the
+	 * workspace cannot be reopened losslessly, so the caller connects over the
+	 * legacy host instead.
+	 */
 	private async migrateLegacyAuthority(
 		remoteAuthority: string,
 		startupMode: StartupMode,
-	): Promise<void> {
+	): Promise<boolean> {
 		const migratedAuthority = retargetRemoteAuthority(remoteAuthority);
 		const workspaceFile = vscode.workspace.workspaceFile;
 		const workspaceFolders = vscode.workspace.workspaceFolders ?? [];
 		const savedWorkspaceFile =
 			workspaceFile?.scheme === "untitled" ? undefined : workspaceFile;
 		if (!savedWorkspaceFile && workspaceFolders.length > 1) {
 			this.logger.warn(
-				"Cannot migrate an unsaved multi-root workspace",
+				"Cannot migrate an unsaved multi-root workspace; connecting over the legacy host",
 				remoteAuthority,
 			);
-			const choice = await vscodeProposed.window.showWarningMessage(
-				"Opening the remote over the old coder-vscode SSH host",
-				{
-					modal: true,
-					useCustom: true,
-					detail:
-						"This editor now uses its own SSH hosts, but switching an unsaved multi-root workspace would drop its folders. " +
-						"To switch, save the workspace, then reload the window.",
-				},
-				"Learn More",
-			);
-			if (choice === "Learn More") {
-				await vscode.env.openExternal(
-					vscode.Uri.parse(
-						"https://code.visualstudio.com/docs/editing/workspaces/multi-root-workspaces",
-					),
-				);
-			}
-			return;
+			// Fire-and-forget: the connection proceeds either way.
+			void vscode.window
+				.showWarningMessage(
+					"This workspace still opens over the old coder-vscode SSH host. " +
+						"To switch it to this editor's own host, save the workspace, then reload the window.",
+					"Learn More",
+				)
+				.then(async (choice) => {
+					if (choice === "Learn More") {
+						await vscode.env.openExternal(
+							vscode.Uri.parse(
+								"https://code.visualstudio.com/docs/editing/workspaces/multi-root-workspaces",
+							),
+						);
+					}
+				});
+			return false;
 		}
 
 		await this.serviceContainer
@@ -779,12 +781,13 @@ export class Remote {
 				}),
 				false,
 			);
-			return;
+			return true;
 		}
 		await vscode.commands.executeCommand("vscode.newWindow", {
 			remoteAuthority: migratedAuthority,
 			reuseWindow: true,
 		});
+		return true;
 	}
 
 	private async resolveRemoteBinary(workspaceClient: Api): Promise<string> {
@@ -959,13 +962,14 @@ export class Remote {
 	// all Coder entries.
 	private async updateSSHConfig(
 		restClient: Api,
-		safeHostname: string,
-		hostName: string,
+		parts: AuthorityParts,
 		binaryPath: string,
 		logDir: string,
 		featureSet: FeatureSet,
 		cliAuth: CliAuth,
 	): Promise<SshProperties> {
+		// Taken from the authority, so an unmigrated legacy host keeps working.
+		const { hostPrefix, safeHostname } = parts;
 		// One file per (editor, deployment); the user's config gains one shared include.
 		const sshConfig = new SshConfig(this.getMainSshConfigPath(), this.logger);
 		await sshConfig.load();
@@ -1017,8 +1021,6 @@ export class Remote {
 			userConfig,
 		);
 
-		const hostPrefix = toCurrentAuthorityHostPrefix(safeHostname);
-
 		const proxyCommand = await this.buildProxyCommand(
 			binaryPath,
 			safeHostname,
@@ -1046,15 +1048,14 @@ export class Remote {
 
 		// Write our file before including it, so the include never dangles.
 		await coderConfig.update(sshValues, sshConfigOverrides);
-		const sharedSshConfigDir = this.pathResolver.getSshConfigDir();
-		await sshConfig.updateInclude(sharedSshConfigDir, safeHostname);
-		// Our file was just written, so only other unused deployments are swept.
-		// Never throws, and the connection does not depend on it.
-		void cleanupStaleSshConfigs(sharedSshConfigDir, this.logger);
+		await sshConfig.updateInclude(
+			this.pathResolver.getSshConfigDir(),
+			safeHostname,
+		);
 
 		// Mirror SSH's parse order; RemoteCommand can come from the user's config.
 		return computeSshProperties(
-			hostName,
+			parts.sshHost,
 			`${coderConfig.getRaw()}\n${sshConfig.getRaw()}`,
 		);
 	}

src/remote/sshConfig.ts

@@ -11,7 +11,6 @@ import path from "node:path";
 
 import { SSH_CONFIG_EXT } from "../core/pathResolver";
 import { countSubstring, lowercase } from "../util";
-import { cleanupFiles } from "../util/fileCleanup";
 import { renameWithRetry, tempFilePath } from "../util/fs";
 
 import type { Logger } from "../logging/logger";
@@ -79,32 +78,20 @@ function legacyDeploymentMarkers(safeHostname: string): BlockMarkers {
 	};
 }
 
-/** Shared include block; identical bytes from every editor, so writers converge. */
+/**
+ * Shared include block; identical bytes from every editor, so writers
+ * converge. "CODER INCLUDE <id>" is the convention for Coder-managed include
+ * blocks with disjoint hosts, so integrations can recognize each other's.
+ */
 const INCLUDE_MARKERS: BlockMarkers = {
-	start: "# --- START CODER ---",
-	end: "# --- END CODER ---",
+	start: "# --- START CODER INCLUDE CODER-REMOTE ---",
+	end: "# --- END CODER INCLUDE CODER-REMOTE ---",
 };
 
 /** Header of the generated per-deployment file. */
 const CODER_SSH_CONFIG_HEADER = `# Coder workspace hosts. Do not edit; the Coder extension rewrites this file
 # on every connection. Override options with the "coder.sshConfig" setting.`;
 
-/** Connects rewrite the file, so anything older is unused and safe to sweep. */
-const STALE_CONFIG_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000;
-
-/** Delete generated configs (from any editor) not connected to recently. */
-export async function cleanupStaleSshConfigs(
-	dir: string,
-	logger: Logger,
-): Promise<void> {
-	await cleanupFiles(dir, logger, {
-		label: "generated SSH config",
-		filter: (name) => name.endsWith(SSH_CONFIG_EXT),
-		select: (files, now) =>
-			files.filter((file) => now - file.mtime > STALE_CONFIG_MAX_AGE_MS),
-	});
-}
-
 /**
  * SSH options a deployment may not set, mirroring the server's validation of
  * --ssh-config-options (codersdk.ValidateSSHConfigOption).
@@ -410,6 +397,7 @@ export class SshConfig {
 	private renderIncludeBlock(includeDir: string): string {
 		return [
 			INCLUDE_MARKERS.start,
+			"# Managed by each editor's Coder extension (coder.coder-remote).",
 			"# Moves back to the top on connect; override options via coder.sshConfig.",
 			`Include "${this.escapeIncludePath(includeDir)}/*${SSH_CONFIG_EXT}"`,
 			INCLUDE_MARKERS.end,
@@ -509,13 +497,15 @@ export class SshConfig {
 	}
 
 	private async discardTemp(tempPath: string): Promise<void> {
-		await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => {
+		try {
+			await this.fileSystem.unlink(tempPath);
+		} catch (unlinkErr) {
 			this.logger.warn(
 				"Failed to clean up temp SSH config file",
 				tempPath,
 				unlinkErr,
 			);
-		});
+		}
 	}
 
 	private async read(): Promise<string> {

src/util/authority.ts

@@ -6,6 +6,8 @@ export const LegacyAuthorityPrefix = "coder-vscode";
 
 export interface AuthorityParts {
 	agent: string | undefined;
+	/** The `coder-<editor>.<safeHostname>--` prefix of the parsed host. */
+	hostPrefix: string;
 	sshHost: string;
 	safeHostname: string;
 	username: string;
@@ -46,7 +48,7 @@ function getSshHostStart(authority: string): number | undefined {
 	return undefined;
 }
 
-function classifySshHost(sshHost: string): AuthorityClassification {
+export function classifySshHost(sshHost: string): AuthorityClassification {
 	const currentPrefix = currentAuthorityPrefix();
 	if (sshHost.startsWith(`${currentPrefix}.`)) {
 		return "current";
@@ -120,19 +122,14 @@ export function parseRemoteAuthority(authority: string): AuthorityParts | null {
 
 	return {
 		agent,
+		hostPrefix: `${prefix}${safeHostname}--`,
 		sshHost,
 		safeHostname,
 		username,
 		workspace,
 	};
 }
 
-export function classifyRemoteAuthority(
-	parts: AuthorityParts,
-): AuthorityClassification {
-	return classifySshHost(parts.sshHost);
-}
-
 export function toRemoteAuthority(
 	baseUrl: string,
 	workspaceOwner: string,
@@ -146,10 +143,6 @@ export function toRemoteAuthority(
 	return remoteAuthority;
 }
 
-export function toCurrentAuthorityHostPrefix(safeHostname: string): string {
-	return `${currentAuthorityPrefix()}.${safeHostname}--`;
-}
-
 export function retargetRemoteAuthority(authority: string): string {
 	const sshHostStart = getSshHostStart(authority);
 	if (sshHostStart === undefined) {

test/unit/remote/remote.test.ts

@@ -209,10 +209,15 @@ describe("Remote", () => {
 			docsUrls: [expect.stringContaining("multi-root-workspaces")],
 		},
 	])(
-		"keeps an untitled multi-root workspace on the old host (choice: $choice)",
+		"sets up an untitled multi-root workspace on the old host (choice: $choice)",
 		async ({ choice, docsUrls }) => {
 			mockEnv.uriScheme = "cursor";
-			const { remote, mementoManager, userInteraction } = createRemote();
+			const {
+				remote,
+				ensureLoggedInWithDialog,
+				mementoManager,
+				userInteraction,
+			} = createRemote();
 			setWorkspace(
 				[createUri("/first-folder"), createUri("/second-folder")],
 				createUri("/Untitled-1.code-workspace", {
@@ -232,7 +237,17 @@ describe("Remote", () => {
 			expect(warning?.message).toContain("coder-vscode SSH host");
 			expect(warning?.items).toEqual(["Learn More"]);
 			expect(userInteraction.getExternalUrls()).toEqual(docsUrls);
-			expect(vscode.commands.executeCommand).not.toHaveBeenCalled();
+			// Setup continues over the legacy host instead of reopening the window.
+			expect(ensureLoggedInWithDialog).toHaveBeenCalledOnce();
+			expect(vscode.commands.executeCommand).not.toHaveBeenCalledWith(
+				"vscode.openFolder",
+				expect.anything(),
+				expect.anything(),
+			);
+			expect(vscode.commands.executeCommand).not.toHaveBeenCalledWith(
+				"vscode.newWindow",
+				expect.anything(),
+			);
 			expect(await mementoManager.getAndClearStartupMode()).toBe("none");
 		},
 	);