From 9f6d32dff55df377b53a1af8cc5ef329a6fd2def Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 7 Apr 2026 17:11:46 +0300 Subject: [PATCH 1/3] refactor: extract CLI command execution into core/cliExec module --- package.json | 5 +- src/commands.ts | 249 +++++-------------- src/core/cliCredentialManager.ts | 6 +- src/core/cliExec.ts | 240 ++++++++++++++++++ src/core/cliManager.ts | 7 +- src/core/cliUtils.ts | 63 ----- src/remote/remote.ts | 4 +- test/fixtures/scripts/bin.bash | 3 - test/fixtures/scripts/bin.old.bash | 8 - test/fixtures/scripts/echo-args.js | 3 - test/unit/core/cliCredentialManager.test.ts | 16 +- test/unit/core/cliExec.test.ts | 211 ++++++++++++++++ test/unit/core/cliManager.concurrent.test.ts | 12 +- test/unit/core/cliManager.test.ts | 13 +- test/unit/core/cliUtils.test.ts | 148 ----------- test/utils/platform.ts | 25 ++ 16 files changed, 571 insertions(+), 442 deletions(-) create mode 100644 src/core/cliExec.ts delete mode 100755 test/fixtures/scripts/bin.bash delete mode 100755 test/fixtures/scripts/bin.old.bash delete mode 100644 test/fixtures/scripts/echo-args.js create mode 100644 test/unit/core/cliExec.test.ts diff --git a/package.json b/package.json index 7e4c9962..89b4bb58 100644 --- a/package.json +++ b/package.json @@ -401,7 +401,7 @@ }, { "command": "coder.speedTest", - "when": "coder.workspace.connected" + "when": "coder.authenticated" }, { "command": "coder.navigateToWorkspace", @@ -531,6 +531,9 @@ "coder.diagnostics": [ { "command": "coder.pingWorkspace" + }, + { + "command": "coder.speedTest" } ], "statusBar/remoteIndicator": [ diff --git a/src/commands.ts b/src/commands.ts index 46737802..720523b2 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -2,7 +2,6 @@ import { type Workspace, type WorkspaceAgent, } from "coder/site/src/api/typesGenerated"; -import { spawn } from "node:child_process"; import * as fs from "node:fs/promises"; import * as path from "node:path"; import * as semver from "semver"; @@ -14,8 +13,8 @@ import { workspaceStatusLabel, } from "./api/api-helper"; import { type CoderApi } from "./api/coderApi"; +import * as cliExec from "./core/cliExec"; import { type CliManager } from "./core/cliManager"; -import * as cliUtils from "./core/cliUtils"; import { type ServiceContainer } from "./core/container"; import { type MementoManager } from "./core/mementoManager"; import { type PathResolver } from "./core/pathResolver"; @@ -32,12 +31,8 @@ import { RECOMMENDED_SSH_SETTINGS, applySettingOverrides, } from "./remote/sshOverrides"; -import { - getGlobalFlags, - getGlobalShellFlags, - resolveCliAuth, -} from "./settings/cli"; -import { escapeCommandArg, toRemoteAuthority, toSafeHost } from "./util"; +import { resolveCliAuth } from "./settings/cli"; +import { toRemoteAuthority, toSafeHost } from "./util"; import { vscodeProposed } from "./vscodeProposed"; import { AgentTreeItem, @@ -172,17 +167,17 @@ export class Commands { } /** - * Run a speed test against the currently connected workspace and display the - * results in a new editor document. + * Run a speed test against a workspace and display the results in a new + * editor document. Can be triggered from the sidebar or command palette. */ - public async speedTest(): Promise { - const workspace = this.workspace; - const client = this.remoteWorkspaceClient; - if (!workspace || !client) { - vscode.window.showInformationMessage("No workspace connected."); + public async speedTest(item?: OpenableTreeItem): Promise { + const resolved = await this.resolveClientAndWorkspace(item); + if (!resolved) { return; } + const { client, workspaceId } = resolved; + const duration = await vscode.window.showInputBox({ title: "Speed Test Duration", prompt: "Duration for the speed test (e.g., 5s, 10s, 1m)", @@ -194,24 +189,8 @@ export class Commands { const result = await withCancellableProgress( async ({ signal }) => { - const baseUrl = client.getAxiosInstance().defaults.baseURL; - if (!baseUrl) { - throw new Error("No deployment URL for the connected workspace"); - } - const safeHost = toSafeHost(baseUrl); - const binary = await this.cliManager.fetchBinary(client); - const version = semver.parse(await cliUtils.version(binary)); - const featureSet = featureSetForVersion(version); - const configDir = this.pathResolver.getGlobalConfigDir(safeHost); - const configs = vscode.workspace.getConfiguration(); - const auth = resolveCliAuth(configs, featureSet, baseUrl, configDir); - const globalFlags = getGlobalFlags(configs, auth); - const workspaceName = createWorkspaceIdentifier(workspace); - - return cliUtils.speedtest(binary, globalFlags, workspaceName, { - signal, - duration: duration.trim(), - }); + const env = await this.resolveCliEnv(client); + return cliExec.speedtest(env, workspaceId, duration.trim(), signal); }, { location: vscode.ProgressLocation.Notification, @@ -568,17 +547,8 @@ export class Commands { title: `Connecting to AI Agent...`, }, async () => { - const { binary, globalFlags } = await this.resolveCliEnv( - this.extensionClient, - ); - - const terminal = vscode.window.createTerminal(app.name); - terminal.sendText( - `${escapeCommandArg(binary)} ${globalFlags.join(" ")} ssh ${app.workspace_name}`, - ); - await new Promise((resolve) => setTimeout(resolve, 5000)); - terminal.sendText(app.command ?? ""); - terminal.show(false); + const env = await this.resolveCliEnv(this.extensionClient); + await cliExec.openAppStatusTerminal(env, app); }, ); } @@ -729,175 +699,72 @@ export class Commands { } public async pingWorkspace(item?: OpenableTreeItem): Promise { - let client: CoderApi; - let workspaceId: string; - - if (item) { - client = this.extensionClient; - workspaceId = createWorkspaceIdentifier(item.workspace); - } else if (this.workspace && this.remoteWorkspaceClient) { - client = this.remoteWorkspaceClient; - workspaceId = createWorkspaceIdentifier(this.workspace); - } else { - client = this.extensionClient; - const workspace = await this.pickWorkspace({ - title: "Ping a running workspace", - initialValue: "owner:me status:running ", - placeholder: "Search running workspaces...", - filter: (w) => w.latest_build.status === "running", - }); - if (!workspace) { - return; - } - workspaceId = createWorkspaceIdentifier(workspace); + const resolved = await this.resolveClientAndWorkspace(item); + if (!resolved) { + return; } - return this.spawnPing(client, workspaceId); - } - - private spawnPing(client: CoderApi, workspaceId: string): Thenable { + const { client, workspaceId } = resolved; return withProgress( { location: vscode.ProgressLocation.Notification, title: `Starting ping for ${workspaceId}...`, }, async () => { - const { binary, globalFlags } = await this.resolveCliEnv(client); - - const writeEmitter = new vscode.EventEmitter(); - const closeEmitter = new vscode.EventEmitter(); - - const args = [...globalFlags, "ping", escapeCommandArg(workspaceId)]; - const cmd = `${escapeCommandArg(binary)} ${args.join(" ")}`; - // On Unix, spawn in a new process group so we can signal the - // entire group (shell + coder binary) on Ctrl+C. On Windows, - // detached opens a visible console window and negative-PID kill - // is unsupported, so we fall back to proc.kill(). - const useProcessGroup = process.platform !== "win32"; - const proc = spawn(cmd, { - shell: true, - detached: useProcessGroup, - }); - - let closed = false; - let exited = false; - let forceKillTimer: ReturnType | undefined; - - const sendSignal = (sig: "SIGINT" | "SIGKILL") => { - try { - if (useProcessGroup && proc.pid) { - process.kill(-proc.pid, sig); - } else { - proc.kill(sig); - } - } catch { - // Process already exited. - } - }; - - const gracefulKill = () => { - sendSignal("SIGINT"); - // Escalate to SIGKILL if the process doesn't exit promptly. - forceKillTimer = setTimeout(() => sendSignal("SIGKILL"), 5000); - }; - - const terminal = vscode.window.createTerminal({ - name: `Coder Ping: ${workspaceId}`, - pty: { - onDidWrite: writeEmitter.event, - onDidClose: closeEmitter.event, - open: () => { - writeEmitter.fire("Press Ctrl+C (^C) to stop.\r\n"); - writeEmitter.fire("─".repeat(40) + "\r\n"); - }, - close: () => { - closed = true; - clearTimeout(forceKillTimer); - sendSignal("SIGKILL"); - writeEmitter.dispose(); - closeEmitter.dispose(); - }, - handleInput: (data: string) => { - if (exited) { - closeEmitter.fire(); - } else if (data === "\x03") { - if (forceKillTimer) { - // Second Ctrl+C: force kill immediately. - clearTimeout(forceKillTimer); - sendSignal("SIGKILL"); - } else { - if (!closed) { - writeEmitter.fire("\r\nStopping...\r\n"); - } - gracefulKill(); - } - } - }, - }, - }); - - const fireLines = (data: Buffer) => { - if (closed) { - return; - } - const lines = data - .toString() - .split(/\r*\n/) - .filter((line) => line !== ""); - for (const line of lines) { - writeEmitter.fire(line + "\r\n"); - } - }; - - proc.stdout?.on("data", fireLines); - proc.stderr?.on("data", fireLines); - proc.on("error", (err) => { - exited = true; - clearTimeout(forceKillTimer); - if (closed) { - return; - } - writeEmitter.fire(`\r\nFailed to start: ${err.message}\r\n`); - writeEmitter.fire("Press any key to close.\r\n"); - }); - proc.on("close", (code, signal) => { - exited = true; - clearTimeout(forceKillTimer); - if (closed) { - return; - } - let reason: string; - if (signal === "SIGKILL") { - reason = "Ping force killed (SIGKILL)"; - } else if (signal) { - reason = "Ping stopped"; - } else { - reason = `Process exited with code ${code}`; - } - writeEmitter.fire(`\r\n${reason}. Press any key to close.\r\n`); - }); - - terminal.show(false); + const env = await this.resolveCliEnv(client); + cliExec.ping(env, workspaceId); }, ); } - private async resolveCliEnv( - client: CoderApi, - ): Promise<{ binary: string; globalFlags: string[] }> { + /** + * Resolve the API client and workspace identifier from a sidebar item, + * the currently connected workspace, or by prompting the user to pick one. + * Returns undefined if the user cancels the picker. + */ + private async resolveClientAndWorkspace( + item?: OpenableTreeItem, + ): Promise<{ client: CoderApi; workspaceId: string } | undefined> { + if (item) { + return { + client: this.extensionClient, + workspaceId: createWorkspaceIdentifier(item.workspace), + }; + } + if (this.workspace && this.remoteWorkspaceClient) { + return { + client: this.remoteWorkspaceClient, + workspaceId: createWorkspaceIdentifier(this.workspace), + }; + } + const workspace = await this.pickWorkspace({ + title: "Select a running workspace", + initialValue: "owner:me status:running ", + placeholder: "Search running workspaces...", + filter: (w) => w.latest_build.status === "running", + }); + if (!workspace) { + return undefined; + } + return { + client: this.extensionClient, + workspaceId: createWorkspaceIdentifier(workspace), + }; + } + + private async resolveCliEnv(client: CoderApi): Promise { const baseUrl = client.getAxiosInstance().defaults.baseURL; if (!baseUrl) { throw new Error("You are not logged in"); } const safeHost = toSafeHost(baseUrl); const binary = await this.cliManager.fetchBinary(client); - const version = semver.parse(await cliUtils.version(binary)); + const version = semver.parse(await cliExec.version(binary)); const featureSet = featureSetForVersion(version); const configDir = this.pathResolver.getGlobalConfigDir(safeHost); const configs = vscode.workspace.getConfiguration(); const auth = resolveCliAuth(configs, featureSet, baseUrl, configDir); - const globalFlags = getGlobalShellFlags(configs, auth); - return { binary, globalFlags }; + return { binary, configs, auth }; } /** diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index d8a3e9a6..acf2d207 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -11,7 +11,7 @@ import { isKeyringEnabled } from "../settings/cli"; import { getHeaderArgs } from "../settings/headers"; import { renameWithRetry, tempFilePath, toSafeHost } from "../util"; -import * as cliUtils from "./cliUtils"; +import { version } from "./cliExec"; import type { WorkspaceConfiguration } from "vscode"; @@ -172,8 +172,8 @@ export class CliCredentialManager { return undefined; } const binPath = await this.resolveBinary(url); - const version = semver.parse(await cliUtils.version(binPath)); - return featureSetForVersion(version)[feature] ? binPath : undefined; + const cliVersion = semver.parse(await version(binPath)); + return featureSetForVersion(cliVersion)[feature] ? binPath : undefined; } /** diff --git a/src/core/cliExec.ts b/src/core/cliExec.ts new file mode 100644 index 00000000..41b3833b --- /dev/null +++ b/src/core/cliExec.ts @@ -0,0 +1,240 @@ +import { type ExecFileException, execFile, spawn } from "node:child_process"; +import { promisify } from "node:util"; +import * as vscode from "vscode"; + +import { + type CliAuth, + getGlobalFlags, + getGlobalShellFlags, +} from "../settings/cli"; +import { escapeCommandArg } from "../util"; + +/** Resolved CLI environment for invoking the coder binary. */ +export interface CliEnv { + binary: string; + auth: CliAuth; + configs: Pick; +} + +const execFileAsync = promisify(execFile); + +// util.promisify types are dynamic so there is no concrete type we can import +// and we have to make our own. +type ExecException = ExecFileException & { stdout?: string; stderr?: string }; + +/** + * Return the version from the binary. Throw if unable to execute the binary or + * find the version for any reason. + */ +export async function version(binPath: string): Promise { + let stdout: string; + try { + const result = await execFileAsync(binPath, [ + "version", + "--output", + "json", + ]); + stdout = result.stdout; + } catch (error) { + // It could be an old version without support for --output. + if ((error as ExecException)?.stderr?.includes("unknown flag: --output")) { + const result = await execFileAsync(binPath, ["version"]); + if (result.stdout?.startsWith("Coder")) { + const v = result.stdout.split(" ")[1]?.trim(); + if (!v) { + throw new Error(`No version found in output: ${result.stdout}`, { + cause: error, + }); + } + return v; + } + } + throw error; + } + + const json = JSON.parse(stdout) as { version?: string }; + if (!json.version) { + throw new Error(`No version found in output: ${stdout}`); + } + return json.version; +} + +/** + * Run `coder speedtest` and return the raw JSON output. + */ +export async function speedtest( + env: CliEnv, + workspaceName: string, + duration?: string, + signal?: AbortSignal, +): Promise { + const globalFlags = getGlobalFlags(env.configs, env.auth); + const args = [...globalFlags, "speedtest", workspaceName, "--output", "json"]; + if (duration) { + args.push("-t", duration); + } + const result = await execFileAsync(env.binary, args, { signal }); + return result.stdout; +} + +/** + * Run `coder ping` in a PTY terminal with Ctrl+C support. + */ +export function ping(env: CliEnv, workspaceName: string): vscode.Terminal { + const globalFlags = getGlobalShellFlags(env.configs, env.auth); + return spawnCliInTerminal({ + name: `Coder Ping: ${workspaceName}`, + binary: env.binary, + args: [...globalFlags, "ping", escapeCommandArg(workspaceName)], + banner: ["Press Ctrl+C (^C) to stop.", "─".repeat(40)], + }); +} + +/** + * Spawn a CLI command in a PTY terminal with process lifecycle management. + */ +function spawnCliInTerminal(options: { + name: string; + binary: string; + args: string[]; + banner?: string[]; +}): vscode.Terminal { + const writeEmitter = new vscode.EventEmitter(); + const closeEmitter = new vscode.EventEmitter(); + + const cmd = `${escapeCommandArg(options.binary)} ${options.args.join(" ")}`; + // On Unix, spawn in a new process group so we can signal the + // entire group (shell + coder binary) on Ctrl+C. On Windows, + // detached opens a visible console window and negative-PID kill + // is unsupported, so we fall back to proc.kill(). + const useProcessGroup = process.platform !== "win32"; + const proc = spawn(cmd, { + shell: true, + detached: useProcessGroup, + }); + + let closed = false; + let exited = false; + let forceKillTimer: ReturnType | undefined; + + const sendSignal = (sig: "SIGINT" | "SIGKILL") => { + try { + if (useProcessGroup && proc.pid) { + process.kill(-proc.pid, sig); + } else { + proc.kill(sig); + } + } catch { + // Process already exited. + } + }; + + const gracefulKill = () => { + sendSignal("SIGINT"); + // Escalate to SIGKILL if the process doesn't exit promptly. + forceKillTimer = setTimeout(() => sendSignal("SIGKILL"), 5000); + }; + + const terminal = vscode.window.createTerminal({ + name: options.name, + pty: { + onDidWrite: writeEmitter.event, + onDidClose: closeEmitter.event, + open: () => { + if (options.banner) { + for (const line of options.banner) { + writeEmitter.fire(line + "\r\n"); + } + } + }, + close: () => { + closed = true; + clearTimeout(forceKillTimer); + sendSignal("SIGKILL"); + writeEmitter.dispose(); + closeEmitter.dispose(); + }, + handleInput: (data: string) => { + if (exited) { + closeEmitter.fire(); + } else if (data === "\x03") { + if (forceKillTimer) { + // Second Ctrl+C: force kill immediately. + clearTimeout(forceKillTimer); + sendSignal("SIGKILL"); + } else { + if (!closed) { + writeEmitter.fire("\r\nStopping...\r\n"); + } + gracefulKill(); + } + } + }, + }, + }); + + const fireLines = (data: Buffer) => { + if (closed) { + return; + } + const lines = data + .toString() + .split(/\r*\n/) + .filter((line) => line !== ""); + for (const line of lines) { + writeEmitter.fire(line + "\r\n"); + } + }; + + proc.stdout?.on("data", fireLines); + proc.stderr?.on("data", fireLines); + proc.on("error", (err) => { + exited = true; + clearTimeout(forceKillTimer); + if (closed) { + return; + } + writeEmitter.fire(`\r\nFailed to start: ${err.message}\r\n`); + writeEmitter.fire("Press any key to close.\r\n"); + }); + proc.on("close", (code, signal) => { + exited = true; + clearTimeout(forceKillTimer); + if (closed) { + return; + } + let reason: string; + if (signal === "SIGKILL") { + reason = "Process force killed (SIGKILL)"; + } else if (signal) { + reason = "Process stopped"; + } else { + reason = `Process exited with code ${code}`; + } + writeEmitter.fire(`\r\n${reason}. Press any key to close.\r\n`); + }); + + terminal.show(false); + return terminal; +} + +/** + * SSH into a workspace and run a command via `terminal.sendText`. + */ +export async function openAppStatusTerminal( + env: CliEnv, + app: { + name?: string; + command?: string; + workspace_name: string; + }, +): Promise { + const globalFlags = getGlobalShellFlags(env.configs, env.auth); + const terminal = vscode.window.createTerminal(app.name); + terminal.sendText( + `${escapeCommandArg(env.binary)} ${globalFlags.join(" ")} ssh ${app.workspace_name}`, + ); + await new Promise((resolve) => setTimeout(resolve, 5000)); + terminal.sendText(app.command ?? ""); + terminal.show(false); +} diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index e1996c8b..913abd02 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -17,6 +17,7 @@ import { tempFilePath, toSafeHost } from "../util"; import { vscodeProposed } from "../vscodeProposed"; import { BinaryLock } from "./binaryLock"; +import { version as cliVersion } from "./cliExec"; import * as cliUtils from "./cliUtils"; import * as downloadProgress from "./downloadProgress"; @@ -107,7 +108,7 @@ export class CliManager { } else { this.output.debug("Existing binary size is", prettyBytes(stat.size)); try { - const version = await cliUtils.version(binPath); + const version = await cliVersion(binPath); this.output.debug("Existing binary version is", version); // If we have the right version we can avoid the request entirely. if (version === buildInfo.version) { @@ -212,7 +213,7 @@ export class CliManager { } try { - const version = await cliUtils.version(binPath); + const version = await cliVersion(binPath); return { version, matches: version === expectedVersion, @@ -274,7 +275,7 @@ export class CliManager { ); // Make sure we can execute this new binary. - const version = await cliUtils.version(binPath); + const version = await cliVersion(binPath); this.output.info("Downloaded binary version is", version); } diff --git a/src/core/cliUtils.ts b/src/core/cliUtils.ts index db0283a1..7302b43a 100644 --- a/src/core/cliUtils.ts +++ b/src/core/cliUtils.ts @@ -1,10 +1,8 @@ -import { execFile, type ExecFileException } from "node:child_process"; import * as crypto from "node:crypto"; import { createReadStream, type Stats } from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { promisify } from "node:util"; /** * Custom error thrown when a binary file is locked (typically on Windows). @@ -31,67 +29,6 @@ export async function stat(binPath: string): Promise { } } -// util.promisify types are dynamic so there is no concrete type we can import -// and we have to make our own. -type ExecException = ExecFileException & { stdout?: string; stderr?: string }; - -/** - * Return the version from the binary. Throw if unable to execute the binary or - * find the version for any reason. - */ -export async function version(binPath: string): Promise { - let stdout: string; - try { - const result = await promisify(execFile)(binPath, [ - "version", - "--output", - "json", - ]); - stdout = result.stdout; - } catch (error) { - // It could be an old version without support for --output. - if ((error as ExecException)?.stderr?.includes("unknown flag: --output")) { - const result = await promisify(execFile)(binPath, ["version"]); - if (result.stdout?.startsWith("Coder")) { - const v = result.stdout.split(" ")[1]?.trim(); - if (!v) { - throw new Error(`No version found in output: ${result.stdout}`, { - cause: error, - }); - } - return v; - } - } - throw error; - } - - const json = JSON.parse(stdout) as { version?: string }; - if (!json.version) { - throw new Error("No version found in output: ${stdout}"); - } - return json.version; -} - -/** - * Run a speed test against the specified workspace and return the raw output. - * Throw if unable to execute the binary. - */ -export async function speedtest( - binPath: string, - globalFlags: string[], - workspaceName: string, - options: { signal?: AbortSignal; duration?: string }, -): Promise { - const args = [...globalFlags, "speedtest", workspaceName, "--output", "json"]; - if (options.duration) { - args.push("-t", options.duration); - } - const result = await promisify(execFile)(binPath, args, { - signal: options.signal, - }); - return result.stdout; -} - export interface RemovalResult { fileName: string; error: unknown; diff --git a/src/remote/remote.ts b/src/remote/remote.ts index cdf71a0c..30d2df37 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -22,8 +22,8 @@ import { CoderApi } from "../api/coderApi"; import { needToken } from "../api/utils"; import { type Commands } from "../commands"; import { watchConfigurationChanges } from "../configWatcher"; +import { version as cliVersion } from "../core/cliExec"; import { type CliManager } from "../core/cliManager"; -import * as cliUtils from "../core/cliUtils"; import { type ServiceContainer } from "../core/container"; import { type ContextManager } from "../core/contextManager"; import { type PathResolver } from "../core/pathResolver"; @@ -263,7 +263,7 @@ export class Remote { let version: semver.SemVer | null = null; try { - version = semver.parse(await cliUtils.version(binaryPath)); + version = semver.parse(await cliVersion(binaryPath)); } catch { version = semver.parse(buildInfo.version); } diff --git a/test/fixtures/scripts/bin.bash b/test/fixtures/scripts/bin.bash deleted file mode 100755 index 3f8db7e6..00000000 --- a/test/fixtures/scripts/bin.bash +++ /dev/null @@ -1,3 +0,0 @@ -#!/usr/bin/env bash - -echo '$ECHO' diff --git a/test/fixtures/scripts/bin.old.bash b/test/fixtures/scripts/bin.old.bash deleted file mode 100755 index f45bb6de..00000000 --- a/test/fixtures/scripts/bin.old.bash +++ /dev/null @@ -1,8 +0,0 @@ -#!/usr/bin/env bash - -if [[ $* == *--output* ]] ; then - >&2 echo -n '$STDERR' - exit 1 -else - echo -n '$STDOUT' -fi diff --git a/test/fixtures/scripts/echo-args.js b/test/fixtures/scripts/echo-args.js deleted file mode 100644 index 328b7bda..00000000 --- a/test/fixtures/scripts/echo-args.js +++ /dev/null @@ -1,3 +0,0 @@ -/* eslint-env node */ -// Prints each argument on its own line, so tests can verify exact args. -process.argv.slice(2).forEach((arg) => console.log(arg)); diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index fdd3be85..ce3ba004 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -8,7 +8,7 @@ import { isKeyringSupported, type BinaryResolver, } from "@/core/cliCredentialManager"; -import * as cliUtils from "@/core/cliUtils"; +import * as cliExec from "@/core/cliExec"; import { PathResolver } from "@/core/pathResolver"; import { isKeyringEnabled } from "@/settings/cli"; @@ -26,9 +26,9 @@ vi.mock("@/settings/cli", () => ({ isKeyringEnabled: vi.fn().mockReturnValue(false), })); -vi.mock("@/core/cliUtils", async () => { +vi.mock("@/core/cliExec", async () => { const actual = - await vi.importActual("@/core/cliUtils"); + await vi.importActual("@/core/cliExec"); return { ...actual, version: vi.fn().mockResolvedValue("2.29.0"), @@ -165,7 +165,7 @@ describe("CliCredentialManager", () => { vi.clearAllMocks(); vol.reset(); vi.mocked(isKeyringEnabled).mockReturnValue(false); - vi.mocked(cliUtils.version).mockResolvedValue("2.31.0"); + vi.mocked(cliExec.version).mockResolvedValue("2.31.0"); }); describe("storeToken", () => { @@ -197,7 +197,7 @@ describe("CliCredentialManager", () => { it("falls back to files when CLI version too old", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(cliUtils.version).mockResolvedValueOnce("2.28.0"); + vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); const { manager } = setup(); await manager.storeToken(TEST_URL, "token", configs); @@ -280,7 +280,7 @@ describe("CliCredentialManager", () => { async ({ keyringEnabled }) => { vi.mocked(isKeyringEnabled).mockReturnValue(keyringEnabled); if (keyringEnabled) { - vi.mocked(cliUtils.version).mockResolvedValueOnce("2.28.0"); + vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); } const { manager } = setup(); @@ -360,7 +360,7 @@ describe("CliCredentialManager", () => { 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"); + vi.mocked(cliExec.version).mockResolvedValueOnce("2.30.0"); stubExecFile({ stdout: "my-token" }); const { manager } = setup(); @@ -462,7 +462,7 @@ describe("CliCredentialManager", () => { it("skips keyring when CLI version too old", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(cliUtils.version).mockResolvedValueOnce("2.28.0"); + vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); stubExecFile({ stdout: "" }); writeCredentialFiles(TEST_URL, "old-token"); const { manager } = setup(); diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts new file mode 100644 index 00000000..c7ca6e17 --- /dev/null +++ b/test/unit/core/cliExec.test.ts @@ -0,0 +1,211 @@ +import fs from "fs/promises"; +import os from "os"; +import path from "path"; +import { beforeAll, describe, expect, it } from "vitest"; + +import * as cliExec from "@/core/cliExec"; + +import { MockConfigurationProvider } from "../../mocks/testHelpers"; +import { isWindows, writeExecutable } from "../../utils/platform"; + +import type { CliEnv } from "@/core/cliExec"; + +describe("cliExec", () => { + const tmp = path.join(os.tmpdir(), "vscode-coder-tests-cliExec"); + + beforeAll(async () => { + await fs.rm(tmp, { recursive: true, force: true }); + await fs.mkdir(tmp, { recursive: true }); + }); + + /** JS code for a fake CLI that writes a fixed string to stdout. */ + function echoBin(output: string): string { + return `process.stdout.write(${JSON.stringify(output)});`; + } + + /** + * JS code for a fake old CLI that rejects --output with stderr, + * and otherwise writes to stdout. + */ + function oldCliBin(stderr: string, stdout: string): string { + return [ + `if (process.argv.includes("--output")) {`, + ` process.stderr.write(${JSON.stringify(stderr)});`, + ` process.exitCode = 1;`, + `} else {`, + ` process.stdout.write(${JSON.stringify(stdout)});`, + `}`, + ].join("\n"); + } + + describe("version", () => { + it("throws when binary does not exist", async () => { + const missing = path.join(tmp, "nonexistent"); + await expect(cliExec.version(missing)).rejects.toThrow("ENOENT"); + }); + + it.skipIf(isWindows())("throws when binary is not executable", async () => { + const noExec = path.join(tmp, "version-noperm"); + await fs.writeFile(noExec, "hello"); + await expect(cliExec.version(noExec)).rejects.toThrow("EACCES"); + }); + + it("throws on invalid JSON output", async () => { + const bin = await writeExecutable(tmp, "ver-bad-json", echoBin("hello")); + await expect(cliExec.version(bin)).rejects.toThrow("Unexpected token"); + }); + + it("throws when JSON has no version field", async () => { + const bin = await writeExecutable(tmp, "ver-no-field", echoBin("{}")); + await expect(cliExec.version(bin)).rejects.toThrow( + "No version found in output", + ); + }); + + it("parses version from JSON output", async () => { + const bin = await writeExecutable( + tmp, + "ver-ok", + echoBin(JSON.stringify({ version: "v0.0.0" })), + ); + expect(await cliExec.version(bin)).toBe("v0.0.0"); + }); + + it("re-throws non-output errors from old CLI", async () => { + const bin = await writeExecutable( + tmp, + "ver-old-err", + oldCliBin("foobar", "Coder v1.1.1"), + ); + await expect(cliExec.version(bin)).rejects.toThrow("foobar"); + }); + + it("falls back to plain version for old CLI", async () => { + const bin = await writeExecutable( + tmp, + "ver-old-ok", + oldCliBin("unknown flag: --output", "Coder v1.1.1"), + ); + expect(await cliExec.version(bin)).toBe("v1.1.1"); + }); + + it("trims trailing newlines from old CLI", async () => { + const bin = await writeExecutable( + tmp, + "ver-old-trim", + oldCliBin("unknown flag: --output\n", "Coder v1.1.1\n"), + ); + expect(await cliExec.version(bin)).toBe("v1.1.1"); + }); + + it("re-throws when old CLI output is not Coder", async () => { + const bin = await writeExecutable( + tmp, + "ver-old-unrelated", + oldCliBin("unknown flag: --output", "Unrelated"), + ); + await expect(cliExec.version(bin)).rejects.toThrow("unknown flag"); + }); + + it("throws when old CLI has no version after Coder prefix", async () => { + const bin = await writeExecutable( + tmp, + "ver-old-noversion", + oldCliBin("unknown flag: --output", "Coder"), + ); + await expect(cliExec.version(bin)).rejects.toThrow("No version found"); + }); + }); + + describe("speedtest", () => { + let echoArgsBin: string; + + beforeAll(async () => { + const code = `process.argv.slice(2).forEach(a => console.log(a));`; + echoArgsBin = await writeExecutable(tmp, "echo-args", code); + }); + + function setup(auth: CliEnv["auth"], binary = echoArgsBin) { + const configs = new MockConfigurationProvider(); + const env: CliEnv = { binary, auth, configs }; + return { configs, env }; + } + + it("passes global-config auth flags", async () => { + const { env } = setup({ + mode: "global-config", + configDir: "/tmp/test-config", + }); + const result = await cliExec.speedtest(env, "owner/workspace"); + const args = result.trim().split("\n"); + expect(args).toEqual([ + "--global-config", + "/tmp/test-config", + "speedtest", + "owner/workspace", + "--output", + "json", + ]); + }); + + it("passes url auth flags", async () => { + const { env } = setup({ + mode: "url", + url: "http://localhost:3000", + }); + const result = await cliExec.speedtest(env, "owner/workspace"); + const args = result.trim().split("\n"); + expect(args).toEqual([ + "--url", + "http://localhost:3000", + "speedtest", + "owner/workspace", + "--output", + "json", + ]); + }); + + it("passes duration flag", async () => { + const { env } = setup({ + mode: "url", + url: "http://localhost:3000", + }); + const result = await cliExec.speedtest(env, "owner/workspace", "10s"); + const args = result.trim().split("\n"); + expect(args).toEqual([ + "--url", + "http://localhost:3000", + "speedtest", + "owner/workspace", + "--output", + "json", + "-t", + "10s", + ]); + }); + + it("passes header command", async () => { + const { configs, env } = setup({ + mode: "url", + url: "http://localhost:3000", + }); + configs.set("coder.headerCommand", "my-header-cmd"); + const result = await cliExec.speedtest(env, "owner/workspace"); + const args = result.trim().split("\n"); + expect(args).toContain("--header-command"); + }); + + it("throws when binary does not exist", async () => { + const { env } = setup( + { + mode: "global-config", + configDir: "/tmp", + }, + "/nonexistent/binary", + ); + await expect(cliExec.speedtest(env, "owner/workspace")).rejects.toThrow( + "ENOENT", + ); + }); + }); +}); diff --git a/test/unit/core/cliManager.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts index 4c0c83ff..057dbc60 100644 --- a/test/unit/core/cliManager.concurrent.test.ts +++ b/test/unit/core/cliManager.concurrent.test.ts @@ -11,6 +11,7 @@ import * as os from "node:os"; import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import * as cliExec from "@/core/cliExec"; import { CliManager } from "@/core/cliManager"; import * as cliUtils from "@/core/cliUtils"; import { PathResolver } from "@/core/pathResolver"; @@ -33,6 +34,13 @@ vi.mock("@/core/cliUtils", async () => { goos: vi.fn(), goarch: vi.fn(), name: vi.fn(), + }; +}); + +vi.mock("@/core/cliExec", async () => { + const actual = await vi.importActual("@/core/cliExec"); + return { + ...actual, version: vi.fn(), }; }); @@ -41,7 +49,7 @@ function setupCliUtilsMocks(version: string) { vi.mocked(cliUtils.goos).mockReturnValue("linux"); vi.mocked(cliUtils.goarch).mockReturnValue("amd64"); vi.mocked(cliUtils.name).mockReturnValue("coder-linux-amd64"); - vi.mocked(cliUtils.version).mockResolvedValue(version); + vi.mocked(cliExec.version).mockResolvedValue(version); vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); } @@ -123,7 +131,7 @@ describe("CliManager Concurrent Downloads", () => { it("redownloads when version mismatch is detected concurrently", async () => { const manager = setupManager(testDir); setupCliUtilsMocks("1.2.3"); - vi.mocked(cliUtils.version).mockImplementation(async (binPath) => { + vi.mocked(cliExec.version).mockImplementation(async (binPath) => { const fileContent = await fs.readFile(binPath, { encoding: "utf-8", }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 7618f9d5..024f1ef0 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -9,8 +9,8 @@ import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; +import * as cliExec from "@/core/cliExec"; import { CliManager } from "@/core/cliManager"; -import * as cliUtils from "@/core/cliUtils"; import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { isKeyringEnabled } from "@/settings/cli"; @@ -59,12 +59,11 @@ vi.mock("proper-lockfile", () => ({ vi.mock("@/pgp"); -vi.mock("@/core/cliUtils", async () => { +vi.mock("@/core/cliExec", async () => { const actual = - await vi.importActual("@/core/cliUtils"); + await vi.importActual("@/core/cliExec"); return { ...actual, - // No need to test script execution here version: vi.fn(), }; }); @@ -708,7 +707,7 @@ describe("CliManager", () => { }); // Mock version to return the specified version - vi.mocked(cliUtils.version).mockResolvedValueOnce(version); + vi.mocked(cliExec.version).mockResolvedValueOnce(version); } function withCorruptedBinary() { @@ -718,7 +717,7 @@ describe("CliManager", () => { }); // Mock version to fail - vi.mocked(cliUtils.version).mockRejectedValueOnce(new Error("corrupted")); + vi.mocked(cliExec.version).mockRejectedValueOnce(new Error("corrupted")); } function withSuccessfulDownload(opts?: { @@ -732,7 +731,7 @@ describe("CliManager", () => { ); // Mock version to return TEST_VERSION after download - vi.mocked(cliUtils.version).mockResolvedValue(TEST_VERSION); + vi.mocked(cliExec.version).mockResolvedValue(TEST_VERSION); } function withSignatureResponses(statuses: number[]): void { diff --git a/test/unit/core/cliUtils.test.ts b/test/unit/core/cliUtils.test.ts index 2347b8b7..0699f591 100644 --- a/test/unit/core/cliUtils.test.ts +++ b/test/unit/core/cliUtils.test.ts @@ -5,9 +5,6 @@ import { beforeAll, describe, expect, it } from "vitest"; import * as cliUtils from "@/core/cliUtils"; -import { getFixturePath } from "../../utils/fixtures"; -import { isWindows } from "../../utils/platform"; - describe("CliUtils", () => { const tmp = path.join(os.tmpdir(), "vscode-coder-tests"); @@ -29,67 +26,6 @@ describe("CliUtils", () => { expect((await cliUtils.stat(binPath))?.size).toBe(4); }); - it.skipIf(isWindows())("version", async () => { - const binPath = path.join(tmp, "version"); - await expect(cliUtils.version(binPath)).rejects.toThrow("ENOENT"); - - const binTmpl = await fs.readFile( - getFixturePath("scripts", "bin.bash"), - "utf8", - ); - await fs.writeFile(binPath, binTmpl.replace("$ECHO", "hello")); - await expect(cliUtils.version(binPath)).rejects.toThrow("EACCES"); - - await fs.chmod(binPath, "755"); - await expect(cliUtils.version(binPath)).rejects.toThrow("Unexpected token"); - - await fs.writeFile(binPath, binTmpl.replace("$ECHO", "{}")); - await expect(cliUtils.version(binPath)).rejects.toThrow( - "No version found in output", - ); - - await fs.writeFile( - binPath, - binTmpl.replace( - "$ECHO", - JSON.stringify({ - version: "v0.0.0", - }), - ), - ); - expect(await cliUtils.version(binPath)).toBe("v0.0.0"); - - const oldTmpl = await fs.readFile( - getFixturePath("scripts", "bin.old.bash"), - "utf8", - ); - const old = (stderr: string, stdout: string): string => { - return oldTmpl.replace("$STDERR", stderr).replace("$STDOUT", stdout); - }; - - // Should fall back only if it says "unknown flag". - await fs.writeFile(binPath, old("foobar", "Coder v1.1.1")); - await expect(cliUtils.version(binPath)).rejects.toThrow("foobar"); - - await fs.writeFile(binPath, old("unknown flag: --output", "Coder v1.1.1")); - expect(await cliUtils.version(binPath)).toBe("v1.1.1"); - - // Should trim off the newline if necessary. - await fs.writeFile( - binPath, - old("unknown flag: --output\n", "Coder v1.1.1\n"), - ); - expect(await cliUtils.version(binPath)).toBe("v1.1.1"); - - // Error with original error if it does not begin with "Coder". - await fs.writeFile(binPath, old("unknown flag: --output", "Unrelated")); - await expect(cliUtils.version(binPath)).rejects.toThrow("unknown flag"); - - // Error if no version. - await fs.writeFile(binPath, old("unknown flag: --output", "Coder")); - await expect(cliUtils.version(binPath)).rejects.toThrow("No version found"); - }); - it("rmOld", async () => { const binDir = path.join(tmp, "bins"); expect(await cliUtils.rmOld(path.join(binDir, "bin1"))).toStrictEqual([]); @@ -142,90 +78,6 @@ describe("CliUtils", () => { ]); }); - describe("speedtest", () => { - const echoArgsBin = isWindows() - ? path.join(tmp, "echo-args.cmd") - : path.join(tmp, "echo-args"); - - beforeAll(async () => { - const scriptPath = getFixturePath("scripts", "echo-args.js"); - if (isWindows()) { - await fs.writeFile(echoArgsBin, `@node "${scriptPath}" %*\r\n`); - } else { - const content = await fs.readFile(scriptPath, "utf8"); - await fs.writeFile(echoArgsBin, `#!/usr/bin/env node\n${content}`); - await fs.chmod(echoArgsBin, "755"); - } - }); - - it("passes global flags", async () => { - const result = await cliUtils.speedtest( - echoArgsBin, - ["--global-config", "/tmp/test-config"], - "owner/workspace", - {}, - ); - const args = result.trim().split("\n"); - expect(args).toEqual([ - "--global-config", - "/tmp/test-config", - "speedtest", - "owner/workspace", - "--output", - "json", - ]); - }); - - it("passes url flags", async () => { - const result = await cliUtils.speedtest( - echoArgsBin, - ["--url", "http://localhost:3000"], - "owner/workspace", - {}, - ); - const args = result.trim().split("\n"); - expect(args).toEqual([ - "--url", - "http://localhost:3000", - "speedtest", - "owner/workspace", - "--output", - "json", - ]); - }); - - it("passes duration flag", async () => { - const result = await cliUtils.speedtest( - echoArgsBin, - ["--url", "http://localhost:3000"], - "owner/workspace", - { duration: "10s" }, - ); - const args = result.trim().split("\n"); - expect(args).toEqual([ - "--url", - "http://localhost:3000", - "speedtest", - "owner/workspace", - "--output", - "json", - "-t", - "10s", - ]); - }); - - it("throws when binary does not exist", async () => { - await expect( - cliUtils.speedtest( - "/nonexistent/binary", - ["--global-config", "/tmp"], - "owner/workspace", - {}, - ), - ).rejects.toThrow("ENOENT"); - }); - }); - it("ETag", async () => { const binPath = path.join(tmp, "hash"); diff --git a/test/utils/platform.ts b/test/utils/platform.ts index b0abc660..9c72caf7 100644 --- a/test/utils/platform.ts +++ b/test/utils/platform.ts @@ -1,3 +1,4 @@ +import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { expect } from "vitest"; @@ -37,6 +38,30 @@ export function printEnvCommand(key: string, varName: string): string { return `node -e "process.stdout.write('${key}=' + process.env.${varName})"`; } +/** + * Write a cross-platform executable that runs the given JS code. + * On Unix creates a shebang script; on Windows creates a .cmd wrapper. + * Returns the path to the executable. + */ +export async function writeExecutable( + dir: string, + name: string, + code: string, +): Promise { + if (isWindows()) { + const jsPath = path.join(dir, `${name}.js`); + const cmdPath = path.join(dir, `${name}.cmd`); + await fs.writeFile(jsPath, code); + await fs.writeFile(cmdPath, `@node "${jsPath}" %*\r\n`); + return cmdPath; + } + + const binPath = path.join(dir, name); + await fs.writeFile(binPath, `#!/usr/bin/env node\n${code}`); + await fs.chmod(binPath, "755"); + return binPath; +} + export function expectPathsEqual(actual: string, expected: string) { expect(normalizePath(actual)).toBe(normalizePath(expected)); } From f297fe39ceff327253aac56dc63ab60069ce8bf7 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 8 Apr 2026 12:15:55 +0300 Subject: [PATCH 2/3] fix: improve cliExec error handling and input validation --- package.json | 35 ++++++++++++++++++++-------------- src/commands.ts | 24 +++++++++++++++++++---- src/core/cliExec.ts | 30 +++++++++++++++++++++++++---- src/extension.ts | 16 ++++++++++++---- test/unit/core/cliExec.test.ts | 32 +++++++++++++++++++++++++++++++ 5 files changed, 111 insertions(+), 26 deletions(-) diff --git a/package.json b/package.json index 89b4bb58..abafdf0e 100644 --- a/package.json +++ b/package.json @@ -259,12 +259,6 @@ "when": "!coder.authenticated && coder.loaded" } ], - "submenus": [ - { - "id": "coder.diagnostics", - "label": "Diagnostics" - } - ], "commands": [ { "command": "coder.login", @@ -327,7 +321,7 @@ }, { "command": "coder.speedTest", - "title": "Run Speed Test", + "title": "Speed Test Workspace", "category": "Coder" }, { @@ -379,6 +373,14 @@ "command": "coder.pingWorkspace", "title": "Ping Workspace", "category": "Coder" + }, + { + "command": "coder.speedTest:views", + "title": "Speed Test" + }, + { + "command": "coder.pingWorkspace:views", + "title": "Ping" } ], "menus": { @@ -415,6 +417,14 @@ "command": "coder.pingWorkspace", "when": "coder.authenticated" }, + { + "command": "coder.speedTest:views", + "when": "false" + }, + { + "command": "coder.pingWorkspace:views", + "when": "false" + }, { "command": "coder.workspace.update", "when": "coder.workspace.updatable" @@ -523,17 +533,14 @@ "group": "inline" }, { - "submenu": "coder.diagnostics", + "command": "coder.pingWorkspace:views", "when": "coder.authenticated && viewItem =~ /\\+running/", "group": "navigation" - } - ], - "coder.diagnostics": [ - { - "command": "coder.pingWorkspace" }, { - "command": "coder.speedTest" + "command": "coder.speedTest:views", + "when": "coder.authenticated && viewItem =~ /\\+running/", + "group": "navigation" } ], "statusBar/remoteIndicator": [ diff --git a/src/commands.ts b/src/commands.ts index 720523b2..1824f3ac 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -180,21 +180,31 @@ export class Commands { const duration = await vscode.window.showInputBox({ title: "Speed Test Duration", - prompt: "Duration for the speed test (e.g., 5s, 10s, 1m)", + prompt: "Duration for the speed test", value: "5s", + validateInput: (value) => { + const v = value.trim(); + if (v && !cliExec.isGoDuration(v)) { + return "Invalid Go duration (e.g., 5s, 10s, 1m, 1m30s)"; + } + return undefined; + }, }); if (duration === undefined) { return; } + const trimmedDuration = duration.trim(); const result = await withCancellableProgress( async ({ signal }) => { const env = await this.resolveCliEnv(client); - return cliExec.speedtest(env, workspaceId, duration.trim(), signal); + return cliExec.speedtest(env, workspaceId, trimmedDuration, signal); }, { location: vscode.ProgressLocation.Notification, - title: `Running ${duration.trim()} speed test...`, + title: trimmedDuration + ? `Running speed test (${trimmedDuration})...` + : "Running speed test...", cancellable: true, }, ); @@ -752,13 +762,19 @@ export class Commands { }; } + /** Resolve a CliEnv, preferring a locally cached binary over a network fetch. */ private async resolveCliEnv(client: CoderApi): Promise { const baseUrl = client.getAxiosInstance().defaults.baseURL; if (!baseUrl) { throw new Error("You are not logged in"); } const safeHost = toSafeHost(baseUrl); - const binary = await this.cliManager.fetchBinary(client); + let binary: string; + try { + binary = await this.cliManager.locateBinary(baseUrl); + } catch { + binary = await this.cliManager.fetchBinary(client); + } const version = semver.parse(await cliExec.version(binary)); const featureSet = featureSetForVersion(version); const configDir = this.pathResolver.getGlobalConfigDir(safeHost); diff --git a/src/core/cliExec.ts b/src/core/cliExec.ts index 41b3833b..49ae9a7f 100644 --- a/src/core/cliExec.ts +++ b/src/core/cliExec.ts @@ -2,6 +2,7 @@ import { type ExecFileException, execFile, spawn } from "node:child_process"; import { promisify } from "node:util"; import * as vscode from "vscode"; +import { toError } from "../error/errorUtils"; import { type CliAuth, getGlobalFlags, @@ -22,6 +23,23 @@ const execFileAsync = promisify(execFile); // and we have to make our own. type ExecException = ExecFileException & { stdout?: string; stderr?: string }; +/** Prefer stderr over the default message which includes the full command line. */ +function cliError(error: unknown): Error { + const stderr = (error as ExecException)?.stderr?.trim(); + if (stderr) { + return new Error(stderr, { cause: error }); + } + return toError(error); +} + +/** Go duration regex: one or more {number}{unit} segments (e.g. 5s, 1h30m). */ +const GO_DURATION_RE = /^(\d+(\.\d+)?(ns|us|µs|ms|s|m|h))+$/; + +/** Returns true if the string is a valid Go duration. */ +export function isGoDuration(value: string): boolean { + return GO_DURATION_RE.test(value); +} + /** * Return the version from the binary. Throw if unable to execute the binary or * find the version for any reason. @@ -49,7 +67,7 @@ export async function version(binPath: string): Promise { return v; } } - throw error; + throw cliError(error); } const json = JSON.parse(stdout) as { version?: string }; @@ -73,8 +91,12 @@ export async function speedtest( if (duration) { args.push("-t", duration); } - const result = await execFileAsync(env.binary, args, { signal }); - return result.stdout; + try { + const result = await execFileAsync(env.binary, args, { signal }); + return result.stdout; + } catch (error) { + throw cliError(error); + } } /** @@ -232,7 +254,7 @@ export async function openAppStatusTerminal( const globalFlags = getGlobalShellFlags(env.configs, env.auth); const terminal = vscode.window.createTerminal(app.name); terminal.sendText( - `${escapeCommandArg(env.binary)} ${globalFlags.join(" ")} ssh ${app.workspace_name}`, + `${escapeCommandArg(env.binary)} ${globalFlags.join(" ")} ssh ${escapeCommandArg(app.workspace_name)}`, ); await new Promise((resolve) => setTimeout(resolve, 5000)); terminal.sendText(app.command ?? ""); diff --git a/src/extension.ts b/src/extension.ts index e1c9cc2e..6533cfad 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -289,10 +289,6 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { void myWorkspacesProvider.fetchAndRefresh(); void allWorkspacesProvider.fetchAndRefresh(); }), - vscode.commands.registerCommand( - "coder.speedTest", - commands.speedTest.bind(commands), - ), vscode.commands.registerCommand( "coder.viewLogs", commands.viewLogs.bind(commands), @@ -315,6 +311,18 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { "coder.pingWorkspace", commands.pingWorkspace.bind(commands), ), + vscode.commands.registerCommand( + "coder.pingWorkspace:views", + commands.pingWorkspace.bind(commands), + ), + vscode.commands.registerCommand( + "coder.speedTest", + commands.speedTest.bind(commands), + ), + vscode.commands.registerCommand( + "coder.speedTest:views", + commands.speedTest.bind(commands), + ), ); const remote = new Remote(serviceContainer, commands, ctx); diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index c7ca6e17..0adc9839 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -207,5 +207,37 @@ describe("cliExec", () => { "ENOENT", ); }); + + it("surfaces stderr instead of full command line on failure", async () => { + const code = [ + `process.stderr.write("invalid argument for -t flag\\n");`, + `process.exitCode = 1;`, + ].join("\n"); + const bin = await writeExecutable(tmp, "speedtest-err", code); + const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin); + await expect( + cliExec.speedtest(env, "owner/workspace", "bad"), + ).rejects.toThrow("invalid argument for -t flag"); + }); + }); + + describe("isGoDuration", () => { + it.each([ + "5s", + "10m", + "1h", + "1h30m", + "500ms", + "1.5s", + "2h45m10s", + "100ns", + "50us", + "50µs", + ])("accepts %s", (v) => expect(cliExec.isGoDuration(v)).toBe(true)); + + it.each(["", "bjbmn", "5", "s", "5x", "1h 30m", "-5s", "5S"])( + "rejects %s", + (v) => expect(cliExec.isGoDuration(v)).toBe(false), + ); }); }); From 9a0433629a8896b53821848d9e6dccd5772c2685 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 8 Apr 2026 18:28:31 +0300 Subject: [PATCH 3/3] Review comments --- src/core/cliExec.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/core/cliExec.ts b/src/core/cliExec.ts index 49ae9a7f..8067ce72 100644 --- a/src/core/cliExec.ts +++ b/src/core/cliExec.ts @@ -19,15 +19,17 @@ export interface CliEnv { const execFileAsync = promisify(execFile); -// util.promisify types are dynamic so there is no concrete type we can import -// and we have to make our own. -type ExecException = ExecFileException & { stdout?: string; stderr?: string }; +function isExecFileException(error: unknown): error is ExecFileException { + return error instanceof Error && "code" in error; +} /** Prefer stderr over the default message which includes the full command line. */ function cliError(error: unknown): Error { - const stderr = (error as ExecException)?.stderr?.trim(); - if (stderr) { - return new Error(stderr, { cause: error }); + if (isExecFileException(error)) { + const stderr = error.stderr?.trim(); + if (stderr) { + return new Error(stderr, { cause: error }); + } } return toError(error); } @@ -55,7 +57,10 @@ export async function version(binPath: string): Promise { stdout = result.stdout; } catch (error) { // It could be an old version without support for --output. - if ((error as ExecException)?.stderr?.includes("unknown flag: --output")) { + if ( + isExecFileException(error) && + error.stderr?.includes("unknown flag: --output") + ) { const result = await execFileAsync(binPath, ["version"]); if (result.stdout?.startsWith("Coder")) { const v = result.stdout.split(" ")[1]?.trim();