From becad08acbb0f4ee5419129ea9937dbaba74e751 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 7 May 2026 18:41:20 +0000 Subject: [PATCH 1/7] fix: update workspaces via API defaults --- src/api/workspace.ts | 172 +++++++- src/remote/remote.ts | 2 +- src/remote/workspaceStateMachine.ts | 30 +- test/unit/api/workspace.test.ts | 387 +++++++++++++++++- .../unit/remote/workspaceStateMachine.test.ts | 13 + 5 files changed, 586 insertions(+), 18 deletions(-) diff --git a/src/api/workspace.ts b/src/api/workspace.ts index 3c23e28f..5f15458f 100644 --- a/src/api/workspace.ts +++ b/src/api/workspace.ts @@ -1,8 +1,11 @@ import { type Api } from "coder/site/src/api/api"; import { - type WorkspaceAgentLog, + type CreateWorkspaceBuildRequest, type ProvisionerJobLog, + type TemplateVersionParameter, type Workspace, + type WorkspaceAgentLog, + type WorkspaceBuildParameter, } from "coder/site/src/api/typesGenerated"; import { spawn } from "node:child_process"; import * as vscode from "vscode"; @@ -113,21 +116,40 @@ export async function startWorkspace(ctx: CliContext): Promise { } /** - * Update a workspace to the latest template version. + * Update a workspace to the latest template version using the API. * - * Uses `coder update` when the CLI supports it (>= 2.24). - * Falls back to the REST API: stop, wait, then updateWorkspaceVersion. + * Parameter prompts cannot be answered from the read-only output channel, so + * update builds are created with existing mutable values where valid and + * template defaults for values that would otherwise require prompting. */ export async function updateWorkspace(ctx: CliContext): Promise { - if (ctx.featureSet.cliUpdate) { - await runCliCommand(ctx, ["update"]); - return ctx.restClient.getWorkspace(ctx.workspace.id); + const workspace = await ctx.restClient.getWorkspace(ctx.workspace.id); + if (!workspace.outdated) { + ctx.write("Workspace is up-to-date.\r\n"); + return workspace; } - // REST API fallback for older CLIs. - if (ctx.workspace.latest_build.status === "running") { + const targetVersionId = workspace.template_active_version_id; + const oldBuildParameters = await ctx.restClient.getWorkspaceBuildParameters( + workspace.latest_build.id, + ); + const templateParameters = await getUpdateTemplateParameters( + ctx, + workspace, + oldBuildParameters, + targetVersionId, + ); + const buildParameters = resolveUpdateParametersWithDefaults( + oldBuildParameters, + templateParameters, + ); + + if (workspace.latest_build.transition === "start") { ctx.write("Stopping workspace for update...\r\n"); - const stopBuild = await ctx.restClient.stopWorkspace(ctx.workspace.id); + const stopBuild = await ctx.restClient.postWorkspaceBuild( + workspace.id, + withBuildReason(ctx, { transition: "stop" }), + ); const stoppedJob = await ctx.restClient.waitForBuild(stopBuild); if (stoppedJob?.status === "canceled") { throw new Error("Workspace update canceled during stop"); @@ -135,8 +157,134 @@ export async function updateWorkspace(ctx: CliContext): Promise { } ctx.write("Starting workspace with updated template...\r\n"); - await ctx.restClient.updateWorkspaceVersion(ctx.workspace); - return ctx.restClient.getWorkspace(ctx.workspace.id); + const startBuild = await ctx.restClient.postWorkspaceBuild( + workspace.id, + withBuildReason(ctx, { + transition: "start", + template_version_id: targetVersionId, + rich_parameter_values: buildParameters, + }), + ); + const startedJob = await ctx.restClient.waitForBuild(startBuild); + if (startedJob?.status === "canceled") { + throw new Error("Workspace update canceled during start"); + } + + return ctx.restClient.getWorkspace(workspace.id); +} + +async function getUpdateTemplateParameters( + ctx: CliContext, + workspace: Workspace, + oldBuildParameters: WorkspaceBuildParameter[], + targetVersionId: string, +): Promise { + if (workspace.template_use_classic_parameter_flow) { + return ctx.restClient.getTemplateVersionRichParameters(targetVersionId); + } + + return ctx.restClient.getDynamicParameters( + targetVersionId, + workspace.owner_id, + oldBuildParameters, + ); +} + +function withBuildReason( + ctx: CliContext, + request: CreateWorkspaceBuildRequest, +): CreateWorkspaceBuildRequest { + if (!ctx.featureSet.buildReason) { + return request; + } + return { ...request, reason: "vscode_connection" }; +} + +function resolveUpdateParametersWithDefaults( + oldBuildParameters: WorkspaceBuildParameter[], + templateParameters: TemplateVersionParameter[], +): WorkspaceBuildParameter[] { + const oldBuildParametersByName = new Map( + oldBuildParameters.map((parameter) => [parameter.name, parameter]), + ); + const resolvedParameters: WorkspaceBuildParameter[] = []; + + for (const parameter of templateParameters) { + if (parameter.ephemeral) { + continue; + } + + const oldParameter = oldBuildParametersByName.get(parameter.name); + const oldParameterIsValid = oldParameter + ? isValidParameterValue(oldParameter.value, parameter) + : false; + + if (oldParameter && parameter.mutable && oldParameterIsValid) { + resolvedParameters.push(oldParameter); + continue; + } + + if (!oldParameter || !oldParameterIsValid || parameter.mutable) { + if (!hasUsableDefault(parameter)) { + throw new Error(missingDefaultMessage(parameter)); + } + resolvedParameters.push({ + name: parameter.name, + value: parameter.default_value, + }); + } + } + + return resolvedParameters; +} + +function hasUsableDefault(parameter: TemplateVersionParameter): boolean { + return !parameter.required; +} + +function missingDefaultMessage(parameter: TemplateVersionParameter): string { + const name = parameter.display_name + ? `${parameter.display_name} (${parameter.name})` + : parameter.name; + return `Workspace update requires a value for parameter "${name}", but no default is available. Open Coder in your browser to set this parameter, then try updating again.`; +} + +function isValidParameterValue( + value: string, + parameter: TemplateVersionParameter, +): boolean { + if (parameter.options.length === 0) { + return true; + } + + const allowedValues = new Set( + parameter.options.map((option) => option.value), + ); + if (parameter.type === "list(string)") { + return isValidListParameterValue(value, allowedValues); + } + + return allowedValues.has(value); +} + +function isValidListParameterValue( + value: string, + allowedValues: ReadonlySet, +): boolean { + let values: unknown; + try { + values = JSON.parse(value); + } catch { + return false; + } + + return ( + Array.isArray(values) && + values.every( + (parameterValue) => + typeof parameterValue === "string" && allowedValues.has(parameterValue), + ) + ); } /** diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 29077169..7dadf70d 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -562,7 +562,7 @@ export class Remote { const isReady = await stateMachine.processWorkspace(w, progress); if (isReady) { subscription.dispose(); - resolve(w); + resolve(stateMachine.getWorkspace() ?? w); return; } } catch (error: unknown) { diff --git a/src/remote/workspaceStateMachine.ts b/src/remote/workspaceStateMachine.ts index b533ffb8..24653981 100644 --- a/src/remote/workspaceStateMachine.ts +++ b/src/remote/workspaceStateMachine.ts @@ -35,6 +35,7 @@ export class WorkspaceStateMachine implements vscode.Disposable { private readonly agentLogStream = new LazyStream(); private agent: { id: string; name: string } | undefined; + private workspace: Workspace | undefined; constructor( private readonly parts: AuthorityParts, @@ -56,13 +57,19 @@ export class WorkspaceStateMachine implements vscode.Disposable { workspace: Workspace, progress: vscode.Progress<{ message?: string }>, ): Promise { + this.workspace = workspace; const workspaceName = createWorkspaceIdentifier(workspace); switch (workspace.latest_build.status) { case "running": this.buildLogStream.close(); if (this.startupMode === "update") { - await this.triggerUpdate(workspace, workspaceName, progress); + workspace = await this.triggerUpdate( + workspace, + workspaceName, + progress, + ); + this.workspace = workspace; // Agent IDs may have changed after an update. this.agent = undefined; } @@ -84,7 +91,15 @@ export class WorkspaceStateMachine implements vscode.Disposable { } if (this.startupMode === "update") { - await this.triggerUpdate(workspace, workspaceName, progress); + workspace = await this.triggerUpdate( + workspace, + workspaceName, + progress, + ); + this.workspace = workspace; + if (workspace.latest_build.status === "running") { + break; + } } else { await this.triggerStart(workspace, workspaceName, progress); } @@ -252,16 +267,19 @@ export class WorkspaceStateMachine implements vscode.Disposable { workspace: Workspace, workspaceName: string, progress: vscode.Progress<{ message?: string }>, - ): Promise { + ): Promise { progress.report({ message: `updating ${workspaceName}...` }); this.logger.info(`Updating ${workspaceName}`, { mode: this.startupMode, status: workspace.latest_build.status, }); - await updateWorkspace(this.buildCliContext(workspace)); + const updatedWorkspace = await updateWorkspace( + this.buildCliContext(workspace), + ); // Downgrade so subsequent transitions don't re-trigger the update. this.startupMode = "start"; this.logger.info(`${workspaceName} update initiated`); + return updatedWorkspace; } private async confirmStartOrUpdate( @@ -286,6 +304,10 @@ export class WorkspaceStateMachine implements vscode.Disposable { return this.agent?.id; } + public getWorkspace(): Workspace | undefined { + return this.workspace; + } + dispose(): void { this.buildLogStream.close(); this.agentLogStream.close(); diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 182ee390..be8d85c8 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -1,8 +1,54 @@ import { describe, expect, it, vi } from "vitest"; -import { LazyStream } from "@/api/workspace"; +import { LazyStream, updateWorkspace } from "@/api/workspace"; +import { type FeatureSet } from "@/featureSet"; import { type UnidirectionalStream } from "@/websocket/eventStreamConnection"; +import { workspace as createWorkspace } from "../../mocks/workspace"; + +import type { Api } from "coder/site/src/api/api"; +import type { + CreateWorkspaceBuildRequest, + ProvisionerJob, + TemplateVersionParameter, + TemplateVersionParameterOption, + Workspace, + WorkspaceBuild, + WorkspaceBuildParameter, +} from "coder/site/src/api/typesGenerated"; + +type UpdateWorkspaceContext = Parameters[0]; +interface UpdateRestClient { + getWorkspace: (workspaceId: string) => Promise; + getWorkspaceBuildParameters: ( + workspaceBuildId: string, + ) => Promise; + getTemplateVersionRichParameters: ( + versionId: string, + ) => Promise; + getDynamicParameters: ( + templateVersionId: string, + ownerId: string, + oldBuildParameters: WorkspaceBuildParameter[], + ) => Promise; + postWorkspaceBuild: ( + workspaceId: string, + data: CreateWorkspaceBuildRequest, + ) => Promise; + waitForBuild: (build: WorkspaceBuild) => Promise; +} + +const featureSet: FeatureSet = { + vscodessh: true, + proxyLogDirectory: true, + wildcardSSH: true, + buildReason: true, + cliUpdate: true, + keyringAuth: true, + keyringTokenRead: true, + supportBundle: true, +}; + function mockStream(): UnidirectionalStream { return { url: "ws://test", @@ -28,6 +74,132 @@ function deferredFactory() { }; } +function templateParameter( + overrides: Partial = {}, +): TemplateVersionParameter { + return { + name: "parameter", + description: "", + description_plaintext: "", + type: "string", + form_type: "", + mutable: true, + default_value: "default", + icon: "", + options: [], + required: false, + ephemeral: false, + ...overrides, + }; +} + +function parameterOption( + value: string, + overrides: Partial = {}, +): TemplateVersionParameterOption { + return { + name: value, + description: "", + value, + icon: "", + ...overrides, + }; +} + +function createBuild( + workspace: Workspace, + overrides: Partial = {}, +): WorkspaceBuild { + return { + ...workspace.latest_build, + ...overrides, + }; +} + +function succeededJob(workspace: Workspace): ProvisionerJob { + return { ...workspace.latest_build.job, status: "succeeded" }; +} + +function setupUpdateWorkspace({ + workspace = createWorkspace({ + outdated: true, + template_use_classic_parameter_flow: true, + latest_build: { status: "stopped", transition: "stop" }, + }), + finalWorkspace = createWorkspace({ + outdated: false, + template_use_classic_parameter_flow: true, + latest_build: { status: "running" }, + }), + oldBuildParameters = [], + templateParameters = [], + featureSetOverrides = {}, +}: { + workspace?: Workspace; + finalWorkspace?: Workspace; + oldBuildParameters?: WorkspaceBuildParameter[]; + templateParameters?: TemplateVersionParameter[]; + featureSetOverrides?: Partial; +} = {}) { + const stopBuild = createBuild(workspace, { + id: "stop-build", + build_number: 2, + transition: "stop", + status: "stopped", + }); + const startBuild = createBuild(workspace, { + id: "start-build", + build_number: 3, + transition: "start", + status: "running", + }); + const postWorkspaceBuild = vi + .fn< + ( + workspaceId: string, + data: CreateWorkspaceBuildRequest, + ) => Promise + >() + .mockResolvedValueOnce(stopBuild) + .mockResolvedValue(startBuild); + const waitForBuild = vi + .fn<(build: WorkspaceBuild) => Promise>() + .mockResolvedValue(succeededJob(workspace)); + const restClient: UpdateRestClient = { + getWorkspace: vi + .fn<(workspaceId: string) => Promise>() + .mockResolvedValueOnce(workspace) + .mockResolvedValue(finalWorkspace), + getWorkspaceBuildParameters: vi + .fn<(workspaceBuildId: string) => Promise>() + .mockResolvedValue(oldBuildParameters), + getTemplateVersionRichParameters: vi + .fn<(versionId: string) => Promise>() + .mockResolvedValue(templateParameters), + getDynamicParameters: vi + .fn< + ( + templateVersionId: string, + ownerId: string, + oldBuildParameters: WorkspaceBuildParameter[], + ) => Promise + >() + .mockResolvedValue(templateParameters), + postWorkspaceBuild, + waitForBuild, + }; + const write = vi.fn<(data: string) => void>(); + const ctx: UpdateWorkspaceContext = { + restClient: restClient as Api, + auth: { mode: "url", url: "https://test.coder.com" }, + binPath: "/usr/bin/coder", + workspace, + write, + featureSet: { ...featureSet, ...featureSetOverrides }, + }; + return { ctx, restClient, startBuild, stopBuild, write }; +} + describe("LazyStream", () => { it("opens once and ignores subsequent calls", async () => { const factory: StreamFactory = vi.fn().mockResolvedValue(mockStream()); @@ -86,3 +258,216 @@ describe("LazyStream", () => { expect(factory2).toHaveBeenCalledOnce(); }); }); + +describe("updateWorkspace", () => { + it("returns the fresh workspace without building when already up to date", async () => { + const workspace = createWorkspace({ outdated: false }); + const { ctx, restClient, write } = setupUpdateWorkspace({ workspace }); + + await expect(updateWorkspace(ctx)).resolves.toBe(workspace); + + expect(write).toHaveBeenCalledWith("Workspace is up-to-date.\r\n"); + expect(restClient.getWorkspaceBuildParameters).not.toHaveBeenCalled(); + expect(restClient.postWorkspaceBuild).not.toHaveBeenCalled(); + expect(restClient.waitForBuild).not.toHaveBeenCalled(); + }); + + it("stops a started workspace before starting the update build", async () => { + const workspace = createWorkspace({ + outdated: true, + template_active_version_id: "active-version", + template_use_classic_parameter_flow: true, + latest_build: { status: "running", transition: "start" }, + }); + const finalWorkspace = createWorkspace({ + outdated: false, + template_active_version_id: "active-version", + latest_build: { + status: "running", + template_version_id: "active-version", + }, + }); + const oldBuildParameters = [{ name: "region", value: "us" }]; + const templateParameters = [ + templateParameter({ name: "region", default_value: "eu" }), + ]; + const { ctx, restClient, startBuild, stopBuild } = setupUpdateWorkspace({ + workspace, + finalWorkspace, + oldBuildParameters, + templateParameters, + }); + + await expect(updateWorkspace(ctx)).resolves.toBe(finalWorkspace); + + expect(restClient.getWorkspace).toHaveBeenNthCalledWith(1, workspace.id); + expect(restClient.getWorkspaceBuildParameters).toHaveBeenCalledWith( + workspace.latest_build.id, + ); + expect(restClient.getTemplateVersionRichParameters).toHaveBeenCalledWith( + "active-version", + ); + expect(restClient.postWorkspaceBuild).toHaveBeenNthCalledWith( + 1, + workspace.id, + { transition: "stop", reason: "vscode_connection" }, + ); + expect(restClient.postWorkspaceBuild).toHaveBeenNthCalledWith( + 2, + workspace.id, + { + transition: "start", + template_version_id: "active-version", + rich_parameter_values: oldBuildParameters, + reason: "vscode_connection", + }, + ); + expect(restClient.waitForBuild).toHaveBeenNthCalledWith(1, stopBuild); + expect(restClient.waitForBuild).toHaveBeenNthCalledWith(2, startBuild); + }); + + it("evaluates dynamic parameters when the template uses dynamic parameter flow", async () => { + const workspace = createWorkspace({ + outdated: true, + template_active_version_id: "active-version", + template_use_classic_parameter_flow: false, + latest_build: { status: "stopped", transition: "stop" }, + }); + const oldBuildParameters = [{ name: "region", value: "us" }]; + const { ctx, restClient } = setupUpdateWorkspace({ + workspace, + oldBuildParameters, + templateParameters: [templateParameter({ name: "region" })], + }); + + await updateWorkspace(ctx); + + expect(restClient.getDynamicParameters).toHaveBeenCalledWith( + "active-version", + workspace.owner_id, + oldBuildParameters, + ); + expect(restClient.getTemplateVersionRichParameters).not.toHaveBeenCalled(); + }); + + it("uses template defaults for missing optional parameters", async () => { + const { ctx, restClient } = setupUpdateWorkspace({ + templateParameters: [ + templateParameter({ name: "editor", default_value: "vim" }), + ], + }); + + await updateWorkspace(ctx); + + expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( + ctx.workspace.id, + expect.objectContaining({ + rich_parameter_values: [{ name: "editor", value: "vim" }], + }), + ); + }); + + it("falls back to the template default when an old option value is invalid", async () => { + const { ctx, restClient } = setupUpdateWorkspace({ + oldBuildParameters: [{ name: "color", value: "blue" }], + templateParameters: [ + templateParameter({ + name: "color", + default_value: "red", + options: [parameterOption("red"), parameterOption("green")], + }), + ], + }); + + await updateWorkspace(ctx); + + expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( + ctx.workspace.id, + expect.objectContaining({ + rich_parameter_values: [{ name: "color", value: "red" }], + }), + ); + }); + + it("preserves valid multi-select values", async () => { + const oldBuildParameters = [ + { name: "tools", value: JSON.stringify(["vim", "emacs"]) }, + ]; + const { ctx, restClient } = setupUpdateWorkspace({ + oldBuildParameters, + templateParameters: [ + templateParameter({ + name: "tools", + type: "list(string)", + default_value: JSON.stringify(["vim"]), + options: [parameterOption("vim"), parameterOption("emacs")], + }), + ], + }); + + await updateWorkspace(ctx); + + expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( + ctx.workspace.id, + expect.objectContaining({ rich_parameter_values: oldBuildParameters }), + ); + }); + + it("omits ephemeral and previously-set immutable parameters", async () => { + const { ctx, restClient } = setupUpdateWorkspace({ + oldBuildParameters: [ + { name: "token", value: "secret" }, + { name: "size", value: "large" }, + ], + templateParameters: [ + templateParameter({ name: "token", ephemeral: true }), + templateParameter({ + name: "size", + mutable: false, + default_value: "small", + }), + ], + }); + + await updateWorkspace(ctx); + + expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( + ctx.workspace.id, + expect.objectContaining({ rich_parameter_values: [] }), + ); + }); + + it("throws before stopping when a required parameter has no default", async () => { + const workspace = createWorkspace({ + outdated: true, + template_use_classic_parameter_flow: true, + latest_build: { status: "running", transition: "start" }, + }); + const { ctx, restClient } = setupUpdateWorkspace({ + workspace, + templateParameters: [ + templateParameter({ + name: "project", + required: true, + default_value: "", + }), + ], + }); + + await expect(updateWorkspace(ctx)).rejects.toThrow('parameter "project"'); + expect(restClient.postWorkspaceBuild).not.toHaveBeenCalled(); + expect(restClient.waitForBuild).not.toHaveBeenCalled(); + }); + + it("omits build reasons when unsupported by the server", async () => { + const { ctx, restClient } = setupUpdateWorkspace({ + templateParameters: [templateParameter({ name: "region" })], + featureSetOverrides: { buildReason: false }, + }); + + await updateWorkspace(ctx); + + const request = vi.mocked(restClient.postWorkspaceBuild).mock.calls[0][1]; + expect(request).not.toHaveProperty("reason"); + }); +}); diff --git a/test/unit/remote/workspaceStateMachine.test.ts b/test/unit/remote/workspaceStateMachine.test.ts index 1b62a217..a0681cca 100644 --- a/test/unit/remote/workspaceStateMachine.test.ts +++ b/test/unit/remote/workspaceStateMachine.test.ts @@ -94,6 +94,9 @@ describe("WorkspaceStateMachine", () => { beforeEach(() => { vi.clearAllMocks(); MockTerminalOutputChannel.lastInstance = undefined; + vi.mocked(updateWorkspace).mockImplementation((ctx) => + Promise.resolve(ctx.workspace), + ); vi.mocked(maybeAskAgent).mockImplementation((agents) => Promise.resolve(agents.length > 0 ? agents[0] : undefined), ); @@ -182,6 +185,16 @@ describe("WorkspaceStateMachine", () => { expect(updateWorkspace).toHaveBeenCalledOnce(); }); + it("falls through to the agent check after an update completes", async () => { + vi.mocked(updateWorkspace).mockResolvedValueOnce(runningWorkspace()); + const { sm, progress } = setup("update"); + const ws = createWorkspace({ latest_build: { status: "stopped" } }); + + expect(await sm.processWorkspace(ws, progress)).toBe(true); + expect(updateWorkspace).toHaveBeenCalledOnce(); + expect(sm.getWorkspace()?.latest_build.status).toBe("running"); + }); + it("prompts user when mode is 'none' and user picks 'Start'", async () => { const { sm, progress, userInteraction } = setup("none"); userInteraction.setResponse(CONFIRM_MESSAGE, "Start"); From 5b123d6be1fba06bccf7562df5c85c873df883a5 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 7 May 2026 19:04:56 +0000 Subject: [PATCH 2/7] fix: prompt for workspace update parameters --- src/api/workspace.ts | 279 +++++++++++++++++++++++++------- test/unit/api/workspace.test.ts | 114 +++++++++++-- 2 files changed, 321 insertions(+), 72 deletions(-) diff --git a/src/api/workspace.ts b/src/api/workspace.ts index 5f15458f..5637a346 100644 --- a/src/api/workspace.ts +++ b/src/api/workspace.ts @@ -118,9 +118,8 @@ export async function startWorkspace(ctx: CliContext): Promise { /** * Update a workspace to the latest template version using the API. * - * Parameter prompts cannot be answered from the read-only output channel, so - * update builds are created with existing mutable values where valid and - * template defaults for values that would otherwise require prompting. + * Parameter values that need user input are collected with VS Code prompts + * before the workspace is stopped. */ export async function updateWorkspace(ctx: CliContext): Promise { const workspace = await ctx.restClient.getWorkspace(ctx.workspace.id); @@ -139,7 +138,7 @@ export async function updateWorkspace(ctx: CliContext): Promise { oldBuildParameters, targetVersionId, ); - const buildParameters = resolveUpdateParametersWithDefaults( + const buildParameters = await promptForUpdateParameters( oldBuildParameters, templateParameters, ); @@ -200,93 +199,261 @@ function withBuildReason( return { ...request, reason: "vscode_connection" }; } -function resolveUpdateParametersWithDefaults( +async function promptForUpdateParameters( oldBuildParameters: WorkspaceBuildParameter[], templateParameters: TemplateVersionParameter[], -): WorkspaceBuildParameter[] { - const oldBuildParametersByName = new Map( - oldBuildParameters.map((parameter) => [parameter.name, parameter]), +): Promise { + const missingParameters = getMissingParameters( + oldBuildParameters, + [], + templateParameters, + ); + const buildParameters: WorkspaceBuildParameter[] = []; + + for (const parameter of missingParameters) { + const value = await promptForParameter(parameter); + if (value === undefined) { + throw new Error("Workspace update canceled while configuring parameters"); + } + buildParameters.push({ name: parameter.name, value }); + } + + return buildParameters; +} + +function getMissingParameters( + oldBuildParameters: WorkspaceBuildParameter[], + buildParameters: WorkspaceBuildParameter[], + templateParameters: TemplateVersionParameter[], +): TemplateVersionParameter[] { + const missingParameters: TemplateVersionParameter[] = []; + const requiredParameters = templateParameters.filter( + (parameter) => + (parameter.mutable && parameter.required) || !parameter.mutable, ); - const resolvedParameters: WorkspaceBuildParameter[] = []; + + for (const parameter of requiredParameters) { + const buildParameter = findBuildParameter( + parameter, + oldBuildParameters, + buildParameters, + ); + if (!buildParameter) { + missingParameters.push(parameter); + } + } for (const parameter of templateParameters) { - if (parameter.ephemeral) { + if ( + parameter.options.length === 0 || + parameter.form_type === "multi-select" + ) { continue; } - const oldParameter = oldBuildParametersByName.get(parameter.name); - const oldParameterIsValid = oldParameter - ? isValidParameterValue(oldParameter.value, parameter) - : false; - - if (oldParameter && parameter.mutable && oldParameterIsValid) { - resolvedParameters.push(oldParameter); + const buildParameter = findBuildParameter( + parameter, + oldBuildParameters, + buildParameters, + ); + if (!buildParameter) { continue; } - if (!oldParameter || !oldParameterIsValid || parameter.mutable) { - if (!hasUsableDefault(parameter)) { - throw new Error(missingDefaultMessage(parameter)); - } - resolvedParameters.push({ - name: parameter.name, - value: parameter.default_value, - }); + const matchingOption = parameter.options.find( + (option) => option.value === buildParameter.value, + ); + if (!matchingOption && !missingParameters.includes(parameter)) { + missingParameters.push(parameter); } } - return resolvedParameters; + return missingParameters; } -function hasUsableDefault(parameter: TemplateVersionParameter): boolean { - return !parameter.required; +function findBuildParameter( + parameter: TemplateVersionParameter, + oldBuildParameters: WorkspaceBuildParameter[], + buildParameters: WorkspaceBuildParameter[], +): WorkspaceBuildParameter | undefined { + return ( + buildParameters.find((p) => p.name === parameter.name) ?? + oldBuildParameters.find((p) => p.name === parameter.name) + ); } -function missingDefaultMessage(parameter: TemplateVersionParameter): string { - const name = parameter.display_name - ? `${parameter.display_name} (${parameter.name})` - : parameter.name; - return `Workspace update requires a value for parameter "${name}", but no default is available. Open Coder in your browser to set this parameter, then try updating again.`; +async function promptForParameter( + parameter: TemplateVersionParameter, +): Promise { + if (parameter.options.length > 0) { + if ( + parameter.form_type === "multi-select" || + parameter.type === "list(string)" + ) { + return promptForMultiSelectParameter(parameter); + } + return promptForSelectParameter(parameter); + } + + if (parameter.type === "bool" || parameter.form_type === "checkbox") { + return promptForBooleanParameter(parameter); + } + + return promptForTextParameter(parameter); } -function isValidParameterValue( - value: string, +type ParameterQuickPickItem = vscode.QuickPickItem & { + value: string; +}; + +async function promptForSelectParameter( parameter: TemplateVersionParameter, -): boolean { - if (parameter.options.length === 0) { - return true; - } +): Promise { + const items = parameter.options.map((option): ParameterQuickPickItem => { + const details = [option.description]; + if (option.value === parameter.default_value) { + details.push("Default"); + } + return { + label: option.name || option.value, + description: option.value, + detail: details.filter(Boolean).join(" • "), + value: option.value, + }; + }); + const choice = await vscode.window.showQuickPick(items, { + title: parameterTitle(parameter), + placeHolder: parameterPlaceHolder(parameter), + ignoreFocusOut: true, + }); + return choice?.value; +} - const allowedValues = new Set( - parameter.options.map((option) => option.value), +async function promptForMultiSelectParameter( + parameter: TemplateVersionParameter, +): Promise { + const defaultValues = parseListParameterValue(parameter.default_value); + const items = parameter.options.map( + (option): ParameterQuickPickItem => ({ + label: option.name || option.value, + description: option.value, + detail: option.description, + picked: defaultValues.includes(option.value), + value: option.value, + }), ); - if (parameter.type === "list(string)") { - return isValidListParameterValue(value, allowedValues); - } + const choices = await vscode.window.showQuickPick(items, { + title: parameterTitle(parameter), + placeHolder: parameterPlaceHolder(parameter), + canPickMany: true, + ignoreFocusOut: true, + }); + return choices + ? JSON.stringify(choices.map((choice) => choice.value)) + : undefined; +} - return allowedValues.has(value); +async function promptForBooleanParameter( + parameter: TemplateVersionParameter, +): Promise { + const items: ParameterQuickPickItem[] = [ + { label: "Yes", value: "true" }, + { label: "No", value: "false" }, + ].map((item) => ({ + ...item, + detail: item.value === parameter.default_value ? "Default" : undefined, + })); + const choice = await vscode.window.showQuickPick(items, { + title: parameterTitle(parameter), + placeHolder: parameterPlaceHolder(parameter), + ignoreFocusOut: true, + }); + return choice?.value; } -function isValidListParameterValue( +async function promptForTextParameter( + parameter: TemplateVersionParameter, +): Promise { + return vscode.window.showInputBox({ + title: parameterTitle(parameter), + prompt: parameterPlaceHolder(parameter), + value: parameter.default_value, + password: parameter.form_type === "password", + ignoreFocusOut: true, + validateInput: (value) => validateParameterInput(parameter, value), + }); +} + +function validateParameterInput( + parameter: TemplateVersionParameter, value: string, - allowedValues: ReadonlySet, -): boolean { - let values: unknown; +): string | undefined { + if (parameter.required && value === "") { + return "A value is required."; + } + + if (parameter.type === "number") { + const numberValue = Number(value); + if (!Number.isFinite(numberValue)) { + return "Enter a number."; + } + if ( + parameter.validation_min !== undefined && + numberValue < parameter.validation_min + ) { + return `Enter a number greater than or equal to ${parameter.validation_min}.`; + } + if ( + parameter.validation_max !== undefined && + numberValue > parameter.validation_max + ) { + return `Enter a number less than or equal to ${parameter.validation_max}.`; + } + } + + if (parameter.type === "list(string)" && parameter.options.length === 0) { + const values = parseListParameterValue(value); + if (values.length === 0 && value !== "[]") { + return "Enter a JSON array of strings."; + } + } + + if (parameter.validation_regex) { + const regex = new RegExp(parameter.validation_regex); + if (!regex.test(value)) { + return parameter.validation_error || "Enter a valid value."; + } + } + + return undefined; +} + +function parseListParameterValue(value: string): string[] { try { - values = JSON.parse(value); + const parsed: unknown = JSON.parse(value); + return Array.isArray(parsed) && + parsed.every((item): item is string => typeof item === "string") + ? parsed + : []; } catch { - return false; + return []; } +} +function parameterTitle(parameter: TemplateVersionParameter): string { + return `Workspace parameter: ${parameterDisplayName(parameter)}`; +} + +function parameterPlaceHolder(parameter: TemplateVersionParameter): string { return ( - Array.isArray(values) && - values.every( - (parameterValue) => - typeof parameterValue === "string" && allowedValues.has(parameterValue), - ) + parameter.description_plaintext || parameter.description || parameter.name ); } +function parameterDisplayName(parameter: TemplateVersionParameter): string { + return parameter.display_name || parameter.name; +} + /** * Streams build logs in real-time via a callback. * Returns the websocket for lifecycle management. diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index be8d85c8..f7eea842 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; import { LazyStream, updateWorkspace } from "@/api/workspace"; import { type FeatureSet } from "@/featureSet"; @@ -106,6 +107,13 @@ function parameterOption( }; } +function chooseQuickPickValue(value: string): void { + vi.mocked(vscode.window.showQuickPick).mockResolvedValueOnce({ + label: value, + value, + } as never); +} + function createBuild( workspace: Workspace, overrides: Partial = {}, @@ -141,6 +149,9 @@ function setupUpdateWorkspace({ templateParameters?: TemplateVersionParameter[]; featureSetOverrides?: Partial; } = {}) { + vi.mocked(vscode.window.showInputBox).mockReset(); + vi.mocked(vscode.window.showQuickPick).mockReset(); + const stopBuild = createBuild(workspace, { id: "stop-build", build_number: 2, @@ -160,8 +171,9 @@ function setupUpdateWorkspace({ data: CreateWorkspaceBuildRequest, ) => Promise >() - .mockResolvedValueOnce(stopBuild) - .mockResolvedValue(startBuild); + .mockImplementation((_workspaceId, data) => + Promise.resolve(data.transition === "stop" ? stopBuild : startBuild), + ); const waitForBuild = vi .fn<(build: WorkspaceBuild) => Promise>() .mockResolvedValue(succeededJob(workspace)); @@ -318,7 +330,7 @@ describe("updateWorkspace", () => { { transition: "start", template_version_id: "active-version", - rich_parameter_values: oldBuildParameters, + rich_parameter_values: [], reason: "vscode_connection", }, ); @@ -350,7 +362,7 @@ describe("updateWorkspace", () => { expect(restClient.getTemplateVersionRichParameters).not.toHaveBeenCalled(); }); - it("uses template defaults for missing optional parameters", async () => { + it("omits missing optional mutable parameters", async () => { const { ctx, restClient } = setupUpdateWorkspace({ templateParameters: [ templateParameter({ name: "editor", default_value: "vim" }), @@ -359,15 +371,74 @@ describe("updateWorkspace", () => { await updateWorkspace(ctx); + expect(vscode.window.showInputBox).not.toHaveBeenCalled(); + expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( + ctx.workspace.id, + expect.objectContaining({ rich_parameter_values: [] }), + ); + }); + + it("prompts for missing required parameters before stopping", async () => { + const workspace = createWorkspace({ + outdated: true, + template_use_classic_parameter_flow: true, + latest_build: { status: "running", transition: "start" }, + }); + const { ctx, restClient } = setupUpdateWorkspace({ + workspace, + templateParameters: [ + templateParameter({ + name: "project", + required: true, + default_value: "", + }), + ], + }); + vi.mocked(vscode.window.showInputBox).mockResolvedValueOnce("project-a"); + + await updateWorkspace(ctx); + + expect(vscode.window.showInputBox).toHaveBeenCalledWith( + expect.objectContaining({ title: "Workspace parameter: project" }), + ); + expect(restClient.postWorkspaceBuild).toHaveBeenNthCalledWith( + 2, + workspace.id, + expect.objectContaining({ + rich_parameter_values: [{ name: "project", value: "project-a" }], + }), + ); + expect( + vi.mocked(vscode.window.showInputBox).mock.invocationCallOrder[0], + ).toBeLessThan( + vi.mocked(restClient.postWorkspaceBuild).mock.invocationCallOrder[0], + ); + }); + + it("prompts for first-time immutable parameters", async () => { + const { ctx, restClient } = setupUpdateWorkspace({ + templateParameters: [ + templateParameter({ + name: "image", + mutable: false, + required: false, + default_value: "ubuntu", + }), + ], + }); + vi.mocked(vscode.window.showInputBox).mockResolvedValueOnce("debian"); + + await updateWorkspace(ctx); + expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( ctx.workspace.id, expect.objectContaining({ - rich_parameter_values: [{ name: "editor", value: "vim" }], + rich_parameter_values: [{ name: "image", value: "debian" }], }), ); }); - it("falls back to the template default when an old option value is invalid", async () => { + it("prompts when an old scalar option value is invalid", async () => { const { ctx, restClient } = setupUpdateWorkspace({ oldBuildParameters: [{ name: "color", value: "blue" }], templateParameters: [ @@ -378,38 +449,47 @@ describe("updateWorkspace", () => { }), ], }); + chooseQuickPickValue("green"); await updateWorkspace(ctx); + expect(vscode.window.showQuickPick).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ label: "red", value: "red" }), + expect.objectContaining({ label: "green", value: "green" }), + ]), + expect.objectContaining({ title: "Workspace parameter: color" }), + ); expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( ctx.workspace.id, expect.objectContaining({ - rich_parameter_values: [{ name: "color", value: "red" }], + rich_parameter_values: [{ name: "color", value: "green" }], }), ); }); - it("preserves valid multi-select values", async () => { - const oldBuildParameters = [ - { name: "tools", value: JSON.stringify(["vim", "emacs"]) }, - ]; + it("does not prompt for invalid multi-select values", async () => { const { ctx, restClient } = setupUpdateWorkspace({ - oldBuildParameters, + oldBuildParameters: [ + { name: "tools", value: JSON.stringify(["vim", "emacs"]) }, + ], templateParameters: [ templateParameter({ name: "tools", type: "list(string)", + form_type: "multi-select", default_value: JSON.stringify(["vim"]), - options: [parameterOption("vim"), parameterOption("emacs")], + options: [parameterOption("vim")], }), ], }); await updateWorkspace(ctx); + expect(vscode.window.showQuickPick).not.toHaveBeenCalled(); expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( ctx.workspace.id, - expect.objectContaining({ rich_parameter_values: oldBuildParameters }), + expect.objectContaining({ rich_parameter_values: [] }), ); }); @@ -437,7 +517,7 @@ describe("updateWorkspace", () => { ); }); - it("throws before stopping when a required parameter has no default", async () => { + it("cancels before stopping when a parameter prompt is dismissed", async () => { const workspace = createWorkspace({ outdated: true, template_use_classic_parameter_flow: true, @@ -454,7 +534,9 @@ describe("updateWorkspace", () => { ], }); - await expect(updateWorkspace(ctx)).rejects.toThrow('parameter "project"'); + await expect(updateWorkspace(ctx)).rejects.toThrow( + "Workspace update canceled while configuring parameters", + ); expect(restClient.postWorkspaceBuild).not.toHaveBeenCalled(); expect(restClient.waitForBuild).not.toHaveBeenCalled(); }); From e719c0e36c942c95b8a23c45990f1df5e55fe6f4 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 11 May 2026 21:42:21 +0300 Subject: [PATCH 3/7] fix: run workspace start/update non-interactively with VS Code prompts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `coder start` and `coder update` can need to prompt for template parameters, but the VS Code workbench's terminal renderer isn't initialized during `resolveAuthority` — so a PTY terminal created in that phase silently drops all output and can't be used to surface prompts. See microsoft/vscode#12000 and #144729: extension-Pseudoterminal writes before `open()` fires are ignored, and `open()` doesn't fire until the workbench has mounted the terminal panel at least once. Run the CLI via plain `spawn` again and stream output through the existing write callback (output channel). Before invoking `coder update`, fetch the new template version's rich parameters and the workspace's current build parameters; for any required parameter that has no default and no existing value, prompt the user with `vscode.window.showInputBox` (with regex/range validation) or `showQuickPick` (when the parameter declares options). Pass the collected values as `--parameter name=value` flags so the CLI never has to prompt. Also propagate the post-update workspace through the state machine so the agent check uses the fresh build's resources. --- src/api/workspace.ts | 474 ++++-------- src/remote/workspaceStateMachine.ts | 76 +- src/workspace/workspaceMonitor.ts | 5 +- test/mocks/vscode.runtime.ts | 2 + test/unit/api/workspace.test.ts | 713 ++++++++---------- .../unit/remote/workspaceStateMachine.test.ts | 12 + test/unit/workspace/workspaceMonitor.test.ts | 13 + 7 files changed, 573 insertions(+), 722 deletions(-) diff --git a/src/api/workspace.ts b/src/api/workspace.ts index 5637a346..df8223ce 100644 --- a/src/api/workspace.ts +++ b/src/api/workspace.ts @@ -1,11 +1,9 @@ import { type Api } from "coder/site/src/api/api"; import { - type CreateWorkspaceBuildRequest, type ProvisionerJobLog, type TemplateVersionParameter, type Workspace, type WorkspaceAgentLog, - type WorkspaceBuildParameter, } from "coder/site/src/api/typesGenerated"; import { spawn } from "node:child_process"; import * as vscode from "vscode"; @@ -57,10 +55,6 @@ interface CliContext { featureSet: FeatureSet; } -/** - * Spawn a Coder CLI subcommand and stream its output. - * Resolves when the process exits successfully; rejects on non-zero exit. - */ function runCliCommand(ctx: CliContext, args: string[]): Promise { return new Promise((resolve, reject) => { const fullArgs = [ @@ -68,18 +62,17 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise { ...args, createWorkspaceIdentifier(ctx.workspace), ]; - const cmd = `${escapeCommandArg(ctx.binPath)} ${fullArgs.join(" ")}`; const proc = spawn(cmd, { shell: true }); proc.stdout.on("data", (data: Buffer) => { - ctx.write(data.toString().replace(/\r?\n/g, "\r\n")); + ctx.write(data.toString()); }); let capturedStderr = ""; proc.stderr.on("data", (data: Buffer) => { const text = data.toString(); - ctx.write(text.replace(/\r?\n/g, "\r\n")); + ctx.write(text); capturedStderr += text; }); @@ -87,11 +80,9 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise { if (code === 0) { resolve(); } else { - let errorText = `"${fullArgs.join(" ")}" exited with code ${code}`; - if (capturedStderr !== "") { - errorText += `: ${capturedStderr}`; - } - reject(new Error(errorText)); + let msg = `"${fullArgs.join(" ")}" exited with code ${code}`; + if (capturedStderr) msg += `: ${capturedStderr}`; + reject(new Error(msg)); } }); }); @@ -116,39 +107,25 @@ export async function startWorkspace(ctx: CliContext): Promise { } /** - * Update a workspace to the latest template version using the API. - * - * Parameter values that need user input are collected with VS Code prompts - * before the workspace is stopped. + * Update a workspace to the latest template version. Collects any newly- + * required parameters via VS Code prompts and passes them to the CLI as flags + * (the resolver phase can't render an interactive terminal). Falls back to + * the REST API for CLIs older than 2.24. */ export async function updateWorkspace(ctx: CliContext): Promise { - const workspace = await ctx.restClient.getWorkspace(ctx.workspace.id); - if (!workspace.outdated) { - ctx.write("Workspace is up-to-date.\r\n"); - return workspace; + if (!ctx.featureSet.cliUpdate) { + return updateWorkspaceVersion(ctx); } - const targetVersionId = workspace.template_active_version_id; - const oldBuildParameters = await ctx.restClient.getWorkspaceBuildParameters( - workspace.latest_build.id, - ); - const templateParameters = await getUpdateTemplateParameters( - ctx, - workspace, - oldBuildParameters, - targetVersionId, - ); - const buildParameters = await promptForUpdateParameters( - oldBuildParameters, - templateParameters, - ); + const paramArgs = await collectUpdateParameters(ctx); + await runCliCommand(ctx, ["update", ...paramArgs]); + return ctx.restClient.getWorkspace(ctx.workspace.id); +} - if (workspace.latest_build.transition === "start") { +async function updateWorkspaceVersion(ctx: CliContext): Promise { + if (ctx.workspace.latest_build.status === "running") { ctx.write("Stopping workspace for update...\r\n"); - const stopBuild = await ctx.restClient.postWorkspaceBuild( - workspace.id, - withBuildReason(ctx, { transition: "stop" }), - ); + const stopBuild = await ctx.restClient.stopWorkspace(ctx.workspace.id); const stoppedJob = await ctx.restClient.waitForBuild(stopBuild); if (stoppedJob?.status === "canceled") { throw new Error("Workspace update canceled during stop"); @@ -156,302 +133,181 @@ export async function updateWorkspace(ctx: CliContext): Promise { } ctx.write("Starting workspace with updated template...\r\n"); - const startBuild = await ctx.restClient.postWorkspaceBuild( - workspace.id, - withBuildReason(ctx, { - transition: "start", - template_version_id: targetVersionId, - rich_parameter_values: buildParameters, - }), - ); - const startedJob = await ctx.restClient.waitForBuild(startBuild); - if (startedJob?.status === "canceled") { - throw new Error("Workspace update canceled during start"); - } - - return ctx.restClient.getWorkspace(workspace.id); + await ctx.restClient.updateWorkspaceVersion(ctx.workspace); + return ctx.restClient.getWorkspace(ctx.workspace.id); } -async function getUpdateTemplateParameters( - ctx: CliContext, - workspace: Workspace, - oldBuildParameters: WorkspaceBuildParameter[], - targetVersionId: string, -): Promise { - if (workspace.template_use_classic_parameter_flow) { - return ctx.restClient.getTemplateVersionRichParameters(targetVersionId); - } - - return ctx.restClient.getDynamicParameters( - targetVersionId, - workspace.owner_id, - oldBuildParameters, +async function collectUpdateParameters(ctx: CliContext): Promise { + const newParams = await ctx.restClient.getTemplateVersionRichParameters( + ctx.workspace.template_active_version_id, ); -} - -function withBuildReason( - ctx: CliContext, - request: CreateWorkspaceBuildRequest, -): CreateWorkspaceBuildRequest { - if (!ctx.featureSet.buildReason) { - return request; - } - return { ...request, reason: "vscode_connection" }; -} + const candidates = newParams.filter((p) => p.required && !p.default_value); + if (candidates.length === 0) return []; -async function promptForUpdateParameters( - oldBuildParameters: WorkspaceBuildParameter[], - templateParameters: TemplateVersionParameter[], -): Promise { - const missingParameters = getMissingParameters( - oldBuildParameters, - [], - templateParameters, + const currentValues = await ctx.restClient.getWorkspaceBuildParameters( + ctx.workspace.latest_build.id, ); - const buildParameters: WorkspaceBuildParameter[] = []; + const existing = new Set(currentValues.map((p) => p.name)); + const toPrompt = candidates.filter((p) => !existing.has(p.name)); - for (const parameter of missingParameters) { - const value = await promptForParameter(parameter); + const args: string[] = []; + for (let i = 0; i < toPrompt.length; i++) { + const value = await promptForParameter(toPrompt[i], i + 1, toPrompt.length); if (value === undefined) { - throw new Error("Workspace update canceled while configuring parameters"); + throw new Error("Workspace update cancelled"); } - buildParameters.push({ name: parameter.name, value }); + args.push("--parameter", escapeCommandArg(`${toPrompt[i].name}=${value}`)); } - - return buildParameters; + return args; } -function getMissingParameters( - oldBuildParameters: WorkspaceBuildParameter[], - buildParameters: WorkspaceBuildParameter[], - templateParameters: TemplateVersionParameter[], -): TemplateVersionParameter[] { - const missingParameters: TemplateVersionParameter[] = []; - const requiredParameters = templateParameters.filter( - (parameter) => - (parameter.mutable && parameter.required) || !parameter.mutable, - ); - - for (const parameter of requiredParameters) { - const buildParameter = findBuildParameter( - parameter, - oldBuildParameters, - buildParameters, - ); - if (!buildParameter) { - missingParameters.push(parameter); - } - } - - for (const parameter of templateParameters) { - if ( - parameter.options.length === 0 || - parameter.form_type === "multi-select" - ) { - continue; - } - - const buildParameter = findBuildParameter( - parameter, - oldBuildParameters, - buildParameters, - ); - if (!buildParameter) { - continue; - } - - const matchingOption = parameter.options.find( - (option) => option.value === buildParameter.value, - ); - if (!matchingOption && !missingParameters.includes(parameter)) { - missingParameters.push(parameter); - } - } - - return missingParameters; -} - -function findBuildParameter( - parameter: TemplateVersionParameter, - oldBuildParameters: WorkspaceBuildParameter[], - buildParameters: WorkspaceBuildParameter[], -): WorkspaceBuildParameter | undefined { - return ( - buildParameters.find((p) => p.name === parameter.name) ?? - oldBuildParameters.find((p) => p.name === parameter.name) - ); -} - -async function promptForParameter( - parameter: TemplateVersionParameter, +function promptForParameter( + param: TemplateVersionParameter, + step: number, + totalSteps: number, ): Promise { - if (parameter.options.length > 0) { - if ( - parameter.form_type === "multi-select" || - parameter.type === "list(string)" - ) { - return promptForMultiSelectParameter(parameter); - } - return promptForSelectParameter(parameter); - } - - if (parameter.type === "bool" || parameter.form_type === "checkbox") { - return promptForBooleanParameter(parameter); + const title = param.display_name || param.name; + const items = quickPickItems(param); + + if (items) { + const multi = param.form_type === "multi-select"; + const qp = vscode.window.createQuickPick<(typeof items)[number]>(); + qp.title = title; + qp.step = step; + qp.totalSteps = totalSteps; + qp.placeholder = param.description_plaintext; + qp.items = items; + qp.canSelectMany = multi; + qp.ignoreFocusOut = true; + return untilHidden(qp, () => { + if (multi) { + return qp.selectedItems.length > 0 + ? JSON.stringify(qp.selectedItems.map((i) => i.value)) + : undefined; + } + return qp.selectedItems[0]?.value; + }); } - return promptForTextParameter(parameter); -} - -type ParameterQuickPickItem = vscode.QuickPickItem & { - value: string; -}; - -async function promptForSelectParameter( - parameter: TemplateVersionParameter, -): Promise { - const items = parameter.options.map((option): ParameterQuickPickItem => { - const details = [option.description]; - if (option.value === parameter.default_value) { - details.push("Default"); - } - return { - label: option.name || option.value, - description: option.value, - detail: details.filter(Boolean).join(" • "), - value: option.value, - }; - }); - const choice = await vscode.window.showQuickPick(items, { - title: parameterTitle(parameter), - placeHolder: parameterPlaceHolder(parameter), - ignoreFocusOut: true, - }); - return choice?.value; -} - -async function promptForMultiSelectParameter( - parameter: TemplateVersionParameter, -): Promise { - const defaultValues = parseListParameterValue(parameter.default_value); - const items = parameter.options.map( - (option): ParameterQuickPickItem => ({ - label: option.name || option.value, - description: option.value, - detail: option.description, - picked: defaultValues.includes(option.value), - value: option.value, - }), + const input = vscode.window.createInputBox(); + input.title = title; + input.step = step; + input.totalSteps = totalSteps; + input.prompt = param.description_plaintext; + input.placeholder = formatConstraint(param); + input.value = param.default_value; + input.ignoreFocusOut = true; + const validate = makeValidator(param); + const refresh = () => { + input.validationMessage = validate(input.value).message ?? ""; + }; + refresh(); + input.onDidChangeValue(refresh); + return untilHidden(input, () => + validate(input.value).ok ? input.value : undefined, ); - const choices = await vscode.window.showQuickPick(items, { - title: parameterTitle(parameter), - placeHolder: parameterPlaceHolder(parameter), - canPickMany: true, - ignoreFocusOut: true, - }); - return choices - ? JSON.stringify(choices.map((choice) => choice.value)) - : undefined; -} - -async function promptForBooleanParameter( - parameter: TemplateVersionParameter, -): Promise { - const items: ParameterQuickPickItem[] = [ - { label: "Yes", value: "true" }, - { label: "No", value: "false" }, - ].map((item) => ({ - ...item, - detail: item.value === parameter.default_value ? "Default" : undefined, - })); - const choice = await vscode.window.showQuickPick(items, { - title: parameterTitle(parameter), - placeHolder: parameterPlaceHolder(parameter), - ignoreFocusOut: true, - }); - return choice?.value; } -async function promptForTextParameter( - parameter: TemplateVersionParameter, -): Promise { - return vscode.window.showInputBox({ - title: parameterTitle(parameter), - prompt: parameterPlaceHolder(parameter), - value: parameter.default_value, - password: parameter.form_type === "password", - ignoreFocusOut: true, - validateInput: (value) => validateParameterInput(parameter, value), +function untilHidden( + qi: vscode.InputBox | vscode.QuickPick, + onAccept: () => T | undefined, +): Promise { + return new Promise((resolve) => { + let done = false; + const finish = (value: T | undefined) => { + if (done) return; + done = true; + resolve(value); + qi.dispose(); + }; + qi.onDidAccept(() => { + const value = onAccept(); + if (value !== undefined) finish(value); + }); + qi.onDidHide(() => finish(undefined)); + qi.show(); }); } -function validateParameterInput( - parameter: TemplateVersionParameter, - value: string, -): string | undefined { - if (parameter.required && value === "") { - return "A value is required."; - } - - if (parameter.type === "number") { - const numberValue = Number(value); - if (!Number.isFinite(numberValue)) { - return "Enter a number."; - } - if ( - parameter.validation_min !== undefined && - numberValue < parameter.validation_min - ) { - return `Enter a number greater than or equal to ${parameter.validation_min}.`; - } - if ( - parameter.validation_max !== undefined && - numberValue > parameter.validation_max - ) { - return `Enter a number less than or equal to ${parameter.validation_max}.`; - } - } - - if (parameter.type === "list(string)" && parameter.options.length === 0) { - const values = parseListParameterValue(value); - if (values.length === 0 && value !== "[]") { - return "Enter a JSON array of strings."; - } +/** + * Returns picker items if the param needs a chooser, otherwise undefined. + * Anything that falls through gets a free-form text input. + */ +function quickPickItems( + param: TemplateVersionParameter, +): Array | undefined { + if (param.type === "bool") { + return [ + { label: "True", value: "true" }, + { label: "False", value: "false" }, + ]; } - - if (parameter.validation_regex) { - const regex = new RegExp(parameter.validation_regex); - if (!regex.test(value)) { - return parameter.validation_error || "Enter a valid value."; - } + if (param.options.length > 0) { + return param.options.map((o) => ({ + label: o.name, + description: o.description, + value: o.value, + })); } - return undefined; } -function parseListParameterValue(value: string): string[] { - try { - const parsed: unknown = JSON.parse(value); - return Array.isArray(parsed) && - parsed.every((item): item is string => typeof item === "string") - ? parsed - : []; - } catch { - return []; +function formatConstraint(param: TemplateVersionParameter): string { + if (param.type === "number") { + const lo = param.validation_min; + const hi = param.validation_max; + if (lo !== undefined && hi !== undefined) return `between ${lo} and ${hi}`; + if (lo !== undefined) return `at least ${lo}`; + if (hi !== undefined) return `at most ${hi}`; + return "a number"; } + if (param.validation_regex) { + return param.validation_error || `must match ${param.validation_regex}`; + } + return ""; } -function parameterTitle(parameter: TemplateVersionParameter): string { - return `Workspace parameter: ${parameterDisplayName(parameter)}`; -} - -function parameterPlaceHolder(parameter: TemplateVersionParameter): string { - return ( - parameter.description_plaintext || parameter.description || parameter.name - ); -} - -function parameterDisplayName(parameter: TemplateVersionParameter): string { - return parameter.display_name || parameter.name; +/** + * Returns `{ ok, message }`: `ok` gates submission, `message` (if any) is + * shown inline. Empty input on a required param blocks submit silently. + * Coder regexes are RE2; on parse failure we defer to server-side validation. + */ +function makeValidator( + param: TemplateVersionParameter, +): (input: string) => { ok: boolean; message?: string } { + let re: RegExp | undefined; + if (param.validation_regex) { + try { + re = new RegExp(param.validation_regex); + } catch { + re = undefined; + } + } + return (input) => { + if (!input) return { ok: !param.required }; + if (param.type === "number") { + const n = Number(input); + if (!Number.isFinite(n)) { + return { ok: false, message: "Must be a number" }; + } + if (param.validation_min !== undefined && n < param.validation_min) { + return { + ok: false, + message: `Must be at least ${param.validation_min}`, + }; + } + if (param.validation_max !== undefined && n > param.validation_max) { + return { + ok: false, + message: `Must be at most ${param.validation_max}`, + }; + } + } + if (re && !re.test(input)) { + return { ok: false, message: param.validation_error || "Invalid format" }; + } + return { ok: true }; + }; } /** diff --git a/src/remote/workspaceStateMachine.ts b/src/remote/workspaceStateMachine.ts index 24653981..c24dc422 100644 --- a/src/remote/workspaceStateMachine.ts +++ b/src/remote/workspaceStateMachine.ts @@ -1,4 +1,10 @@ -import { createWorkspaceIdentifier, extractAgents } from "../api/api-helper"; +import * as vscode from "vscode"; + +import { + createWorkspaceIdentifier, + errToStr, + extractAgents, +} from "../api/api-helper"; import { LazyStream, startWorkspace, @@ -16,7 +22,6 @@ import type { Workspace, WorkspaceAgentLog, } from "coder/site/src/api/typesGenerated"; -import type * as vscode from "vscode"; import type { CoderApi } from "../api/coderApi"; import type { StartupMode } from "../core/mementoManager"; @@ -61,19 +66,20 @@ export class WorkspaceStateMachine implements vscode.Disposable { const workspaceName = createWorkspaceIdentifier(workspace); switch (workspace.latest_build.status) { - case "running": + case "running": { this.buildLogStream.close(); - if (this.startupMode === "update") { - workspace = await this.triggerUpdate( - workspace, - workspaceName, - progress, - ); - this.workspace = workspace; + const updated = await this.maybeUpdate( + workspace, + workspaceName, + progress, + ); + if (updated) { + workspace = updated; // Agent IDs may have changed after an update. this.agent = undefined; } break; + } case "stopped": case "failed": { @@ -90,19 +96,18 @@ export class WorkspaceStateMachine implements vscode.Disposable { this.startupMode = choice; } - if (this.startupMode === "update") { - workspace = await this.triggerUpdate( - workspace, - workspaceName, - progress, - ); - this.workspace = workspace; - if (workspace.latest_build.status === "running") { - break; - } - } else { - await this.triggerStart(workspace, workspaceName, progress); + const updated = await this.maybeUpdate( + workspace, + workspaceName, + progress, + ); + if (updated) { + workspace = updated; + if (workspace.latest_build.status === "running") break; + return false; } + // Either we weren't in update mode, or the update failed: start. + await this.triggerStart(workspace, workspaceName, progress); return false; } @@ -263,23 +268,30 @@ export class WorkspaceStateMachine implements vscode.Disposable { this.logger.info(`${workspaceName} start initiated`); } - private async triggerUpdate( + /** No-op if not in update mode. Falls through to start on failure. */ + private async maybeUpdate( workspace: Workspace, workspaceName: string, progress: vscode.Progress<{ message?: string }>, - ): Promise { + ): Promise { + if (this.startupMode !== "update") return undefined; + // Downgrade up-front so monitor events don't retry the update. + this.startupMode = "start"; progress.report({ message: `updating ${workspaceName}...` }); this.logger.info(`Updating ${workspaceName}`, { - mode: this.startupMode, status: workspace.latest_build.status, }); - const updatedWorkspace = await updateWorkspace( - this.buildCliContext(workspace), - ); - // Downgrade so subsequent transitions don't re-trigger the update. - this.startupMode = "start"; - this.logger.info(`${workspaceName} update initiated`); - return updatedWorkspace; + try { + this.workspace = await updateWorkspace(this.buildCliContext(workspace)); + return this.workspace; + } catch (error) { + const reason = errToStr(error); + this.logger.warn(`Update failed for ${workspaceName}: ${reason}`); + vscode.window.showWarningMessage( + `Workspace update failed: ${reason}. Continuing with the existing version.`, + ); + return undefined; + } } private async confirmStartOrUpdate( diff --git a/src/workspace/workspaceMonitor.ts b/src/workspace/workspaceMonitor.ts index 7f37b07a..4fca7473 100644 --- a/src/workspace/workspaceMonitor.ts +++ b/src/workspace/workspaceMonitor.ts @@ -269,7 +269,10 @@ export class WorkspaceMonitor implements vscode.Disposable { } private updateStatusBar(workspace: Workspace) { - if (workspace.outdated) { + const status = workspace.latest_build.status; + const settled = + status === "running" || status === "stopped" || status === "failed"; + if (workspace.outdated && settled) { this.statusBarItem.show(); } else { this.statusBarItem.hide(); diff --git a/test/mocks/vscode.runtime.ts b/test/mocks/vscode.runtime.ts index 8def001e..b22a86f0 100644 --- a/test/mocks/vscode.runtime.ts +++ b/test/mocks/vscode.runtime.ts @@ -138,6 +138,8 @@ export const window = { dispose: vi.fn(), clear: vi.fn(), })), + createInputBox: vi.fn(), + createQuickPick: vi.fn(), createStatusBarItem: vi.fn(), createWebviewPanel: vi.fn(), registerUriHandler: vi.fn(() => ({ dispose: vi.fn() })), diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index f7eea842..fa78483e 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -1,43 +1,25 @@ -import { describe, expect, it, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; -import { LazyStream, updateWorkspace } from "@/api/workspace"; +import { LazyStream, startWorkspace, updateWorkspace } from "@/api/workspace"; import { type FeatureSet } from "@/featureSet"; import { type UnidirectionalStream } from "@/websocket/eventStreamConnection"; -import { workspace as createWorkspace } from "../../mocks/workspace"; +import { workspace as createWorkspace } from "@repo/mocks"; import type { Api } from "coder/site/src/api/api"; import type { - CreateWorkspaceBuildRequest, - ProvisionerJob, TemplateVersionParameter, - TemplateVersionParameterOption, Workspace, WorkspaceBuild, - WorkspaceBuildParameter, } from "coder/site/src/api/typesGenerated"; -type UpdateWorkspaceContext = Parameters[0]; -interface UpdateRestClient { - getWorkspace: (workspaceId: string) => Promise; - getWorkspaceBuildParameters: ( - workspaceBuildId: string, - ) => Promise; - getTemplateVersionRichParameters: ( - versionId: string, - ) => Promise; - getDynamicParameters: ( - templateVersionId: string, - ownerId: string, - oldBuildParameters: WorkspaceBuildParameter[], - ) => Promise; - postWorkspaceBuild: ( - workspaceId: string, - data: CreateWorkspaceBuildRequest, - ) => Promise; - waitForBuild: (build: WorkspaceBuild) => Promise; -} +vi.mock(import("node:child_process"), async (importOriginal) => ({ + ...(await importOriginal()), + spawn: vi.fn(), +})); +const { spawn } = await import("node:child_process"); const featureSet: FeatureSet = { vscodessh: true, @@ -75,141 +57,159 @@ function deferredFactory() { }; } -function templateParameter( - overrides: Partial = {}, -): TemplateVersionParameter { - return { - name: "parameter", - description: "", - description_plaintext: "", - type: "string", - form_type: "", - mutable: true, - default_value: "default", - icon: "", - options: [], - required: false, - ephemeral: false, - ...overrides, +function createUpdateCtx( + overrides: { + workspace?: Omit, "latest_build"> & { + latest_build?: Partial; + }; + featureSet?: Partial; + } = {}, +) { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn((_key: string, defaultValue: unknown) => defaultValue), + } as never); + const workspace = createWorkspace({ + outdated: true, + latest_build: { status: "running", transition: "start" }, + ...overrides.workspace, + }); + const finalWorkspace = createWorkspace({ + outdated: false, + latest_build: { status: "running" }, + }); + const restClient = { + getWorkspace: vi.fn().mockResolvedValue(finalWorkspace), + stopWorkspace: vi + .fn() + .mockResolvedValue({ ...workspace.latest_build, status: "stopped" }), + updateWorkspaceVersion: vi.fn().mockResolvedValue(workspace.latest_build), + waitForBuild: vi.fn().mockResolvedValue({ + ...workspace.latest_build.job, + status: "succeeded", + }), + getTemplateVersionRichParameters: vi.fn().mockResolvedValue([]), + getWorkspaceBuildParameters: vi.fn().mockResolvedValue([]), }; + const ctx = { + restClient: restClient as unknown as Api, + auth: { mode: "url" as const, url: "https://test.coder.com" }, + binPath: "/usr/bin/coder", + workspace, + write: vi.fn<(data: string) => void>(), + featureSet: { ...featureSet, ...overrides.featureSet }, + }; + return { ctx, restClient, finalWorkspace }; } -function parameterOption( - value: string, - overrides: Partial = {}, -): TemplateVersionParameterOption { +/** Drives mocked spawn() so tests can fire stdout/stderr + close at will. */ +function controlSpawn() { + const proc = new EventEmitter() as EventEmitter & { + stdout: EventEmitter; + stderr: EventEmitter; + }; + proc.stdout = new EventEmitter(); + proc.stderr = new EventEmitter(); + let resolveSpawned!: () => void; + const spawned = new Promise((resolve) => { + resolveSpawned = resolve; + }); + vi.mocked(spawn).mockImplementation(() => { + resolveSpawned(); + return proc as never; + }); return { - name: value, - description: "", - value, - icon: "", - ...overrides, + spawned, + stderr(data: string) { + proc.stderr.emit("data", Buffer.from(data)); + }, + async close(exitCode: number) { + await spawned; + proc.emit("close", exitCode); + }, }; } -function chooseQuickPickValue(value: string): void { - vi.mocked(vscode.window.showQuickPick).mockResolvedValueOnce({ - label: value, - value, - } as never); +interface QuickInputMock { + mock: Record & { + show: ReturnType; + dispose: ReturnType; + }; + accept: (overrides?: Record) => void; + hide: () => void; } -function createBuild( - workspace: Workspace, - overrides: Partial = {}, -): WorkspaceBuild { +function quickInputMock(): QuickInputMock { + let acceptCb: () => void = () => {}; + let hideCb: () => void = () => {}; + let changeCb: (v: string) => void = () => {}; + const mock = { + title: "", + step: 0, + totalSteps: 0, + prompt: "", + placeholder: "", + value: "", + validationMessage: "", + ignoreFocusOut: false, + items: [] as readonly unknown[], + selectedItems: [] as readonly unknown[], + onDidAccept: vi.fn((cb: () => void) => { + acceptCb = cb; + return { dispose: vi.fn() }; + }), + onDidHide: vi.fn((cb: () => void) => { + hideCb = cb; + return { dispose: vi.fn() }; + }), + onDidChangeValue: vi.fn((cb: (v: string) => void) => { + changeCb = cb; + return { dispose: vi.fn() }; + }), + show: vi.fn(), + dispose: vi.fn(), + }; return { - ...workspace.latest_build, - ...overrides, + mock, + accept(overrides) { + Object.assign(mock, overrides ?? {}); + if (overrides && "value" in overrides) changeCb(mock.value); + acceptCb(); + }, + hide() { + hideCb(); + }, }; } -function succeededJob(workspace: Workspace): ProvisionerJob { - return { ...workspace.latest_build.job, status: "succeeded" }; +function mockCreateInputBox() { + const qi = quickInputMock(); + vi.mocked(vscode.window.createInputBox).mockReturnValue( + qi.mock as unknown as vscode.InputBox, + ); + return qi; } -function setupUpdateWorkspace({ - workspace = createWorkspace({ - outdated: true, - template_use_classic_parameter_flow: true, - latest_build: { status: "stopped", transition: "stop" }, - }), - finalWorkspace = createWorkspace({ - outdated: false, - template_use_classic_parameter_flow: true, - latest_build: { status: "running" }, - }), - oldBuildParameters = [], - templateParameters = [], - featureSetOverrides = {}, -}: { - workspace?: Workspace; - finalWorkspace?: Workspace; - oldBuildParameters?: WorkspaceBuildParameter[]; - templateParameters?: TemplateVersionParameter[]; - featureSetOverrides?: Partial; -} = {}) { - vi.mocked(vscode.window.showInputBox).mockReset(); - vi.mocked(vscode.window.showQuickPick).mockReset(); - - const stopBuild = createBuild(workspace, { - id: "stop-build", - build_number: 2, - transition: "stop", - status: "stopped", - }); - const startBuild = createBuild(workspace, { - id: "start-build", - build_number: 3, - transition: "start", - status: "running", - }); - const postWorkspaceBuild = vi - .fn< - ( - workspaceId: string, - data: CreateWorkspaceBuildRequest, - ) => Promise - >() - .mockImplementation((_workspaceId, data) => - Promise.resolve(data.transition === "stop" ? stopBuild : startBuild), - ); - const waitForBuild = vi - .fn<(build: WorkspaceBuild) => Promise>() - .mockResolvedValue(succeededJob(workspace)); - const restClient: UpdateRestClient = { - getWorkspace: vi - .fn<(workspaceId: string) => Promise>() - .mockResolvedValueOnce(workspace) - .mockResolvedValue(finalWorkspace), - getWorkspaceBuildParameters: vi - .fn<(workspaceBuildId: string) => Promise>() - .mockResolvedValue(oldBuildParameters), - getTemplateVersionRichParameters: vi - .fn<(versionId: string) => Promise>() - .mockResolvedValue(templateParameters), - getDynamicParameters: vi - .fn< - ( - templateVersionId: string, - ownerId: string, - oldBuildParameters: WorkspaceBuildParameter[], - ) => Promise - >() - .mockResolvedValue(templateParameters), - postWorkspaceBuild, - waitForBuild, - }; - const write = vi.fn<(data: string) => void>(); - const ctx: UpdateWorkspaceContext = { - restClient: restClient as Api, - auth: { mode: "url", url: "https://test.coder.com" }, - binPath: "/usr/bin/coder", - workspace, - write, - featureSet: { ...featureSet, ...featureSetOverrides }, - }; - return { ctx, restClient, startBuild, stopBuild, write }; +function mockCreateQuickPick() { + const qi = quickInputMock(); + vi.mocked(vscode.window.createQuickPick).mockReturnValue( + qi.mock as unknown as vscode.QuickPick, + ); + return qi; +} + +async function flushMicrotasks(times = 4) { + for (let i = 0; i < times; i++) await Promise.resolve(); +} + +function setupUpdate( + params: Array> = [], + opts: Parameters[0] = {}, +) { + const ctxBundle = createUpdateCtx(opts); + ctxBundle.restClient.getTemplateVersionRichParameters.mockResolvedValue( + params.map(param), + ); + return { ...ctxBundle, sp: controlSpawn() }; } describe("LazyStream", () => { @@ -271,285 +271,238 @@ describe("LazyStream", () => { }); }); -describe("updateWorkspace", () => { - it("returns the fresh workspace without building when already up to date", async () => { - const workspace = createWorkspace({ outdated: false }); - const { ctx, restClient, write } = setupUpdateWorkspace({ workspace }); - - await expect(updateWorkspace(ctx)).resolves.toBe(workspace); +function param(overrides: Partial = {}) { + return { + name: "environment", + display_name: "Environment", + description: "", + description_plaintext: "", + type: "string", + form_type: "input", + mutable: true, + default_value: "", + icon: "", + options: [], + required: true, + ephemeral: false, + ...overrides, + } as TemplateVersionParameter; +} - expect(write).toHaveBeenCalledWith("Workspace is up-to-date.\r\n"); - expect(restClient.getWorkspaceBuildParameters).not.toHaveBeenCalled(); - expect(restClient.postWorkspaceBuild).not.toHaveBeenCalled(); - expect(restClient.waitForBuild).not.toHaveBeenCalled(); +describe("updateWorkspace", () => { + beforeEach(() => { + vi.clearAllMocks(); }); - it("stops a started workspace before starting the update build", async () => { - const workspace = createWorkspace({ - outdated: true, - template_active_version_id: "active-version", - template_use_classic_parameter_flow: true, - latest_build: { status: "running", transition: "start" }, - }); - const finalWorkspace = createWorkspace({ - outdated: false, - template_active_version_id: "active-version", - latest_build: { - status: "running", - template_version_id: "active-version", + it.each([ + { + kind: "text input", + param: { name: "environment" } as Partial, + mock: mockCreateInputBox, + accept: { value: "dev" }, + expected: '--parameter "environment=dev"', + }, + { + kind: "bool quick pick", + param: { name: "enabled", type: "bool" }, + mock: mockCreateQuickPick, + accept: { selectedItems: [{ label: "True", value: "true" }] }, + expected: '--parameter "enabled=true"', + }, + { + kind: "options quick pick", + param: { + name: "size", + options: [ + { name: "Small", description: "", value: "s", icon: "" }, + { name: "Large", description: "", value: "l", icon: "" }, + ], }, - }); - const oldBuildParameters = [{ name: "region", value: "us" }]; - const templateParameters = [ - templateParameter({ name: "region", default_value: "eu" }), - ]; - const { ctx, restClient, startBuild, stopBuild } = setupUpdateWorkspace({ - workspace, - finalWorkspace, - oldBuildParameters, - templateParameters, - }); - - await expect(updateWorkspace(ctx)).resolves.toBe(finalWorkspace); + mock: mockCreateQuickPick, + accept: { selectedItems: [{ value: "l" }] }, + expected: '--parameter "size=l"', + }, + { + kind: "fallback text input for unknown types", + param: { name: "x", type: "list(string)" }, + mock: mockCreateInputBox, + accept: { value: "[]" }, + expected: '--parameter "x=[]"', + }, + ])( + "collects the value via $kind", + async ({ param: p, mock, accept, expected }) => { + const { ctx, sp, finalWorkspace } = setupUpdate([p]); + const qi = mock(); + + const result = updateWorkspace(ctx); + await flushMicrotasks(); + qi.accept(accept); + await sp.close(0); + + await expect(result).resolves.toBe(finalWorkspace); + expect(spawn).toHaveBeenCalledWith( + expect.stringContaining(expected), + expect.objectContaining({ shell: true }), + ); + }, + ); - expect(restClient.getWorkspace).toHaveBeenNthCalledWith(1, workspace.id); - expect(restClient.getWorkspaceBuildParameters).toHaveBeenCalledWith( - workspace.latest_build.id, - ); - expect(restClient.getTemplateVersionRichParameters).toHaveBeenCalledWith( - "active-version", + it("skips parameters that already have a value or default", async () => { + const { ctx, restClient, sp } = setupUpdate([ + { name: "existing" }, + { name: "with_default", default_value: "foo" }, + { name: "optional", required: false }, + ]); + restClient.getWorkspaceBuildParameters.mockResolvedValue([ + { name: "existing", value: "kept" }, + ]); + + const result = updateWorkspace(ctx); + await sp.close(0); + await result; + + expect(vscode.window.createInputBox).not.toHaveBeenCalled(); + expect(spawn).toHaveBeenCalledWith( + expect.not.stringContaining("--parameter"), + expect.anything(), ); - expect(restClient.postWorkspaceBuild).toHaveBeenNthCalledWith( - 1, - workspace.id, - { transition: "stop", reason: "vscode_connection" }, - ); - expect(restClient.postWorkspaceBuild).toHaveBeenNthCalledWith( - 2, - workspace.id, - { - transition: "start", - template_version_id: "active-version", - rich_parameter_values: [], - reason: "vscode_connection", - }, - ); - expect(restClient.waitForBuild).toHaveBeenNthCalledWith(1, stopBuild); - expect(restClient.waitForBuild).toHaveBeenNthCalledWith(2, startBuild); }); - it("evaluates dynamic parameters when the template uses dynamic parameter flow", async () => { - const workspace = createWorkspace({ - outdated: true, - template_active_version_id: "active-version", - template_use_classic_parameter_flow: false, - latest_build: { status: "stopped", transition: "stop" }, - }); - const oldBuildParameters = [{ name: "region", value: "us" }]; - const { ctx, restClient } = setupUpdateWorkspace({ - workspace, - oldBuildParameters, - templateParameters: [templateParameter({ name: "region" })], - }); + it("throws when the user cancels a parameter prompt", async () => { + const { ctx } = setupUpdate([{}]); + const qi = mockCreateInputBox(); - await updateWorkspace(ctx); + const result = updateWorkspace(ctx); + await flushMicrotasks(); + qi.hide(); - expect(restClient.getDynamicParameters).toHaveBeenCalledWith( - "active-version", - workspace.owner_id, - oldBuildParameters, - ); - expect(restClient.getTemplateVersionRichParameters).not.toHaveBeenCalled(); + await expect(result).rejects.toThrow("Workspace update cancelled"); + expect(spawn).not.toHaveBeenCalled(); }); - it("omits missing optional mutable parameters", async () => { - const { ctx, restClient } = setupUpdateWorkspace({ - templateParameters: [ - templateParameter({ name: "editor", default_value: "vim" }), - ], - }); - - await updateWorkspace(ctx); - - expect(vscode.window.showInputBox).not.toHaveBeenCalled(); - expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( - ctx.workspace.id, - expect.objectContaining({ rich_parameter_values: [] }), - ); + it("steps the input title across multiple required params", async () => { + const { ctx, sp } = setupUpdate([{ name: "a" }, { name: "b" }]); + const inputs = [quickInputMock(), quickInputMock()]; + vi.mocked(vscode.window.createInputBox) + .mockReturnValueOnce(inputs[0].mock as unknown as vscode.InputBox) + .mockReturnValueOnce(inputs[1].mock as unknown as vscode.InputBox); + + const result = updateWorkspace(ctx); + await flushMicrotasks(); + inputs[0].accept({ value: "first" }); + await flushMicrotasks(); + inputs[1].accept({ value: "second" }); + await sp.close(0); + await result; + + expect(inputs.map((i) => [i.mock.step, i.mock.totalSteps])).toEqual([ + [1, 2], + [2, 2], + ]); }); - it("prompts for missing required parameters before stopping", async () => { - const workspace = createWorkspace({ - outdated: true, - template_use_classic_parameter_flow: true, - latest_build: { status: "running", transition: "start" }, - }); - const { ctx, restClient } = setupUpdateWorkspace({ - workspace, - templateParameters: [ - templateParameter({ - name: "project", - required: true, - default_value: "", - }), - ], - }); - vi.mocked(vscode.window.showInputBox).mockResolvedValueOnce("project-a"); + it("rejects when the process exits non-zero", async () => { + const { ctx, restClient } = createUpdateCtx(); + const sp = controlSpawn(); - await updateWorkspace(ctx); + const result = updateWorkspace(ctx); + await sp.spawned; + sp.stderr("auth failed"); + await sp.close(1); - expect(vscode.window.showInputBox).toHaveBeenCalledWith( - expect.objectContaining({ title: "Workspace parameter: project" }), - ); - expect(restClient.postWorkspaceBuild).toHaveBeenNthCalledWith( - 2, - workspace.id, - expect.objectContaining({ - rich_parameter_values: [{ name: "project", value: "project-a" }], - }), - ); - expect( - vi.mocked(vscode.window.showInputBox).mock.invocationCallOrder[0], - ).toBeLessThan( - vi.mocked(restClient.postWorkspaceBuild).mock.invocationCallOrder[0], - ); + await expect(result).rejects.toThrow(/exited with code 1.*auth failed/); + expect(restClient.getWorkspace).not.toHaveBeenCalled(); }); - it("prompts for first-time immutable parameters", async () => { - const { ctx, restClient } = setupUpdateWorkspace({ - templateParameters: [ - templateParameter({ - name: "image", - mutable: false, - required: false, - default_value: "ubuntu", - }), - ], + it("falls back to the API update path when coder update is unsupported", async () => { + const { ctx, restClient, finalWorkspace } = createUpdateCtx({ + featureSet: { cliUpdate: false }, }); - vi.mocked(vscode.window.showInputBox).mockResolvedValueOnce("debian"); - await updateWorkspace(ctx); + await expect(updateWorkspace(ctx)).resolves.toBe(finalWorkspace); - expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( - ctx.workspace.id, - expect.objectContaining({ - rich_parameter_values: [{ name: "image", value: "debian" }], - }), + expect(spawn).not.toHaveBeenCalled(); + expect(restClient.getTemplateVersionRichParameters).not.toHaveBeenCalled(); + expect(restClient.stopWorkspace).toHaveBeenCalledWith(ctx.workspace.id); + expect(restClient.updateWorkspaceVersion).toHaveBeenCalledWith( + ctx.workspace, ); }); - it("prompts when an old scalar option value is invalid", async () => { - const { ctx, restClient } = setupUpdateWorkspace({ - oldBuildParameters: [{ name: "color", value: "blue" }], - templateParameters: [ - templateParameter({ - name: "color", - default_value: "red", - options: [parameterOption("red"), parameterOption("green")], - }), - ], + it("does not stop before API fallback update when the workspace is not running", async () => { + const { ctx, restClient } = createUpdateCtx({ + workspace: { latest_build: { status: "stopped", transition: "stop" } }, + featureSet: { cliUpdate: false }, }); - chooseQuickPickValue("green"); await updateWorkspace(ctx); - expect(vscode.window.showQuickPick).toHaveBeenCalledWith( - expect.arrayContaining([ - expect.objectContaining({ label: "red", value: "red" }), - expect.objectContaining({ label: "green", value: "green" }), - ]), - expect.objectContaining({ title: "Workspace parameter: color" }), - ); - expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( - ctx.workspace.id, - expect.objectContaining({ - rich_parameter_values: [{ name: "color", value: "green" }], - }), + expect(restClient.stopWorkspace).not.toHaveBeenCalled(); + expect(restClient.updateWorkspaceVersion).toHaveBeenCalledWith( + ctx.workspace, ); }); - it("does not prompt for invalid multi-select values", async () => { - const { ctx, restClient } = setupUpdateWorkspace({ - oldBuildParameters: [ - { name: "tools", value: JSON.stringify(["vim", "emacs"]) }, - ], - templateParameters: [ - templateParameter({ - name: "tools", - type: "list(string)", - form_type: "multi-select", - default_value: JSON.stringify(["vim"]), - options: [parameterOption("vim")], - }), - ], + it("throws before update when the API fallback stop is canceled", async () => { + const { ctx, restClient } = createUpdateCtx({ + featureSet: { cliUpdate: false }, + }); + restClient.waitForBuild.mockResolvedValueOnce({ + ...ctx.workspace.latest_build.job, + status: "canceled", }); - await updateWorkspace(ctx); - - expect(vscode.window.showQuickPick).not.toHaveBeenCalled(); - expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( - ctx.workspace.id, - expect.objectContaining({ rich_parameter_values: [] }), + await expect(updateWorkspace(ctx)).rejects.toThrow( + "Workspace update canceled during stop", ); + expect(restClient.updateWorkspaceVersion).not.toHaveBeenCalled(); }); +}); - it("omits ephemeral and previously-set immutable parameters", async () => { - const { ctx, restClient } = setupUpdateWorkspace({ - oldBuildParameters: [ - { name: "token", value: "secret" }, - { name: "size", value: "large" }, - ], - templateParameters: [ - templateParameter({ name: "token", ephemeral: true }), - templateParameter({ - name: "size", - mutable: false, - default_value: "small", - }), - ], +describe("startWorkspace", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("runs coder start when the workspace is stopped", async () => { + const { ctx, restClient, finalWorkspace } = createUpdateCtx({ + workspace: { latest_build: { status: "stopped", transition: "stop" } }, }); + const sp = controlSpawn(); - await updateWorkspace(ctx); + const result = startWorkspace(ctx); + await sp.close(0); - expect(restClient.postWorkspaceBuild).toHaveBeenCalledWith( - ctx.workspace.id, - expect.objectContaining({ rich_parameter_values: [] }), + await expect(result).resolves.toBe(finalWorkspace); + expect(spawn).toHaveBeenCalledWith( + '"/usr/bin/coder" --url "https://test.coder.com" start --yes --reason vscode_connection testuser/test-workspace', + expect.objectContaining({ shell: true }), ); + expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id); }); - it("cancels before stopping when a parameter prompt is dismissed", async () => { - const workspace = createWorkspace({ - outdated: true, - template_use_classic_parameter_flow: true, - latest_build: { status: "running", transition: "start" }, - }); - const { ctx, restClient } = setupUpdateWorkspace({ - workspace, - templateParameters: [ - templateParameter({ - name: "project", - required: true, - default_value: "", - }), - ], - }); - - await expect(updateWorkspace(ctx)).rejects.toThrow( - "Workspace update canceled while configuring parameters", - ); - expect(restClient.postWorkspaceBuild).not.toHaveBeenCalled(); - expect(restClient.waitForBuild).not.toHaveBeenCalled(); + it("no-ops when the workspace is already running", async () => { + const { ctx, restClient } = createUpdateCtx(); + await expect(startWorkspace(ctx)).resolves.toBe(ctx.workspace); + expect(spawn).not.toHaveBeenCalled(); + expect(restClient.getWorkspace).not.toHaveBeenCalled(); }); - it("omits build reasons when unsupported by the server", async () => { - const { ctx, restClient } = setupUpdateWorkspace({ - templateParameters: [templateParameter({ name: "region" })], - featureSetOverrides: { buildReason: false }, + it("omits --reason when buildReason feature is unavailable", async () => { + const { ctx } = createUpdateCtx({ + workspace: { latest_build: { status: "stopped", transition: "stop" } }, + featureSet: { buildReason: false }, }); + const sp = controlSpawn(); - await updateWorkspace(ctx); + const result = startWorkspace(ctx); + await sp.close(0); + await result; - const request = vi.mocked(restClient.postWorkspaceBuild).mock.calls[0][1]; - expect(request).not.toHaveProperty("reason"); + expect(spawn).toHaveBeenCalledWith( + '"/usr/bin/coder" --url "https://test.coder.com" start --yes testuser/test-workspace', + expect.objectContaining({ shell: true }), + ); }); }); diff --git a/test/unit/remote/workspaceStateMachine.test.ts b/test/unit/remote/workspaceStateMachine.test.ts index a0681cca..46aae11f 100644 --- a/test/unit/remote/workspaceStateMachine.test.ts +++ b/test/unit/remote/workspaceStateMachine.test.ts @@ -195,6 +195,18 @@ describe("WorkspaceStateMachine", () => { expect(sm.getWorkspace()?.latest_build.status).toBe("running"); }); + it("falls back to start when the update fails", async () => { + vi.mocked(updateWorkspace).mockRejectedValueOnce( + new Error("Workspace update cancelled"), + ); + const { sm, progress } = setup("update"); + const ws = createWorkspace({ latest_build: { status: "stopped" } }); + + expect(await sm.processWorkspace(ws, progress)).toBe(false); + expect(updateWorkspace).toHaveBeenCalledOnce(); + expect(startWorkspace).toHaveBeenCalledOnce(); + }); + it("prompts user when mode is 'none' and user picks 'Start'", async () => { const { sm, progress, userInteraction } = setup("none"); userInteraction.setResponse(CONFIRM_MESSAGE, "Start"); diff --git a/test/unit/workspace/workspaceMonitor.test.ts b/test/unit/workspace/workspaceMonitor.test.ts index 5ceedcad..fb2b9cc7 100644 --- a/test/unit/workspace/workspaceMonitor.test.ts +++ b/test/unit/workspace/workspaceMonitor.test.ts @@ -106,6 +106,19 @@ describe("WorkspaceMonitor", () => { stream.pushMessage(workspaceEvent({ outdated: false })); expect(statusBar.hide).toHaveBeenCalled(); }); + + it("hides status bar while an outdated workspace is building", async () => { + const { stream, statusBar } = await setup(); + + stream.pushMessage( + workspaceEvent({ + outdated: true, + latest_build: { status: "starting" }, + }), + ); + expect(statusBar.show).not.toHaveBeenCalled(); + expect(statusBar.hide).toHaveBeenCalled(); + }); }); describe("notifications", () => { From 1908f823553951dd926121e81ee97e5333589800 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 12 May 2026 14:28:43 +0300 Subject: [PATCH 4/7] refactor: extract update parameter handling and apply review feedback - Move parameter prompts and validation to src/api/updateParameters.ts. - Escape --parameter values via escapeShellArg so server-controlled names and values cannot inject shell metacharacters under shell: true. - Clear stale agent id after a stopped/failed to running update. - Treat JSON null on validation_min/max as unset. - Substitute {min}/{max}/{value} in validation_error. - Drop the warning toast when the user cancels a parameter prompt. - Rename untilHidden to collectInput; canceled to cancelled. - Modernize tests: Promise.withResolvers, vi.waitFor, multi-select coverage. --- src/api/updateParameters.ts | 229 ++++++++++++++ src/api/workspace.ts | 204 ++----------- src/remote/workspaceStateMachine.ts | 9 + src/settings/headers.ts | 15 +- src/util.ts | 13 +- test/unit/api/updateParameters.test.ts | 396 +++++++++++++++++++++++++ test/unit/api/workspace.test.ts | 225 +------------- test/unit/util.test.ts | 40 +++ test/utils/platform.test.ts | 50 +++- test/utils/platform.ts | 11 + 10 files changed, 774 insertions(+), 418 deletions(-) create mode 100644 src/api/updateParameters.ts create mode 100644 test/unit/api/updateParameters.test.ts diff --git a/src/api/updateParameters.ts b/src/api/updateParameters.ts new file mode 100644 index 00000000..35a8fdcb --- /dev/null +++ b/src/api/updateParameters.ts @@ -0,0 +1,229 @@ +import * as vscode from "vscode"; + +import { escapeShellArg } from "../util"; + +import type { Api } from "coder/site/src/api/api"; +import type { + TemplateVersionParameter, + Workspace, +} from "coder/site/src/api/typesGenerated"; + +/** Thrown when the user dismisses a parameter prompt. */ +export class WorkspaceUpdateCancelledError extends Error { + constructor() { + super("Workspace update cancelled"); + this.name = "WorkspaceUpdateCancelledError"; + } +} + +/** + * Prompts the user for any newly-required template parameters and returns + * `--parameter name=value` args suitable for `coder update`. Throws + * `WorkspaceUpdateCancelledError` if the user dismisses a prompt. + */ +export async function collectUpdateParameters( + restClient: Api, + workspace: Workspace, +): Promise { + const newParams = await restClient.getTemplateVersionRichParameters( + workspace.template_active_version_id, + ); + const candidates = newParams.filter((p) => p.required && !p.default_value); + if (candidates.length === 0) return []; + + const currentValues = await restClient.getWorkspaceBuildParameters( + workspace.latest_build.id, + ); + const existing = new Set(currentValues.map((p) => p.name)); + const toPrompt = candidates.filter((p) => !existing.has(p.name)); + + const args: string[] = []; + for (let i = 0; i < toPrompt.length; i++) { + const param = toPrompt[i]; + const value = await promptForParameter(param, i + 1, toPrompt.length); + if (value === undefined) { + throw new WorkspaceUpdateCancelledError(); + } + // Server-controlled values; block shell expansion under `shell: true`. + args.push("--parameter", escapeShellArg(`${param.name}=${value}`)); + } + return args; +} + +function promptForParameter( + param: TemplateVersionParameter, + step: number, + totalSteps: number, +): Promise { + const title = param.display_name || param.name; + const items = quickPickItems(param); + + if (items) { + const multi = param.form_type === "multi-select"; + const qp = vscode.window.createQuickPick<(typeof items)[number]>(); + qp.title = title; + qp.step = step; + qp.totalSteps = totalSteps; + qp.placeholder = param.description_plaintext; + qp.items = items; + qp.canSelectMany = multi; + qp.ignoreFocusOut = true; + return collectInput(qp, () => { + if (multi) { + return qp.selectedItems.length > 0 + ? JSON.stringify(qp.selectedItems.map((i) => i.value)) + : undefined; + } + return qp.selectedItems[0]?.value; + }); + } + + const input = vscode.window.createInputBox(); + input.title = title; + input.step = step; + input.totalSteps = totalSteps; + input.prompt = param.description_plaintext; + input.placeholder = formatConstraint(param); + input.value = param.default_value; + input.ignoreFocusOut = true; + const validate = makeValidator(param); + const refresh = () => { + input.validationMessage = validate(input.value).message ?? ""; + }; + refresh(); + input.onDidChangeValue(refresh); + return collectInput(input, () => + validate(input.value).ok ? input.value : undefined, + ); +} + +/** Resolves with `onAccept()` on accept, or `undefined` when hidden. */ +function collectInput( + qi: vscode.InputBox | vscode.QuickPick, + onAccept: () => T | undefined, +): Promise { + return new Promise((resolve) => { + let done = false; + const finish = (value: T | undefined) => { + if (done) return; + done = true; + resolve(value); + qi.dispose(); + }; + qi.onDidAccept(() => { + const value = onAccept(); + if (value !== undefined) finish(value); + }); + qi.onDidHide(() => finish(undefined)); + qi.show(); + }); +} + +/** + * Returns picker items if the param needs a chooser, otherwise undefined. + * Anything that falls through gets a free-form text input. + */ +function quickPickItems( + param: TemplateVersionParameter, +): Array | undefined { + if (param.type === "bool") { + return [ + { label: "True", value: "true" }, + { label: "False", value: "false" }, + ]; + } + if (param.options.length > 0) { + return param.options.map((o) => ({ + label: o.name, + description: o.description, + value: o.value, + })); + } + return undefined; +} + +function isSet(value: T | null | undefined): value is T { + return value !== null && value !== undefined; +} + +function formatConstraint(param: TemplateVersionParameter): string { + if (param.type === "number") { + const lo = param.validation_min; + const hi = param.validation_max; + if (isSet(lo) && isSet(hi)) return `between ${lo} and ${hi}`; + if (isSet(lo)) return `at least ${lo}`; + if (isSet(hi)) return `at most ${hi}`; + return "a number"; + } + if (param.validation_regex) { + return ( + substituteTemplate(param.validation_error, param) || + `must match ${param.validation_regex}` + ); + } + return ""; +} + +/** Substitutes `{min}`, `{max}`, `{value}` placeholders in validation_error. */ +function substituteTemplate( + template: string | undefined, + param: TemplateVersionParameter, + value?: string, +): string | undefined { + if (!template) return template; + return template + .replace(/{min}/g, String(param.validation_min ?? "")) + .replace(/{max}/g, String(param.validation_max ?? "")) + .replace(/{value}/g, value ?? ""); +} + +/** + * Returns `{ ok, message }`; invalid RE2 regexes fall through to server-side + * validation. + */ +function makeValidator( + param: TemplateVersionParameter, +): (input: string) => { ok: boolean; message?: string } { + let re: RegExp | undefined; + if (param.validation_regex) { + try { + re = new RegExp(param.validation_regex); + } catch { + re = undefined; + } + } + return (input) => { + if (!input) return { ok: !param.required }; + if (param.type === "number") { + const n = Number(input); + if (!Number.isFinite(n)) { + return { ok: false, message: "Must be a number" }; + } + if (isSet(param.validation_min) && n < param.validation_min) { + return { + ok: false, + message: + substituteTemplate(param.validation_error, param, input) || + `Must be at least ${param.validation_min}`, + }; + } + if (isSet(param.validation_max) && n > param.validation_max) { + return { + ok: false, + message: + substituteTemplate(param.validation_error, param, input) || + `Must be at most ${param.validation_max}`, + }; + } + } + if (re && !re.test(input)) { + return { + ok: false, + message: + substituteTemplate(param.validation_error, param, input) || + "Invalid format", + }; + } + return { ok: true }; + }; +} diff --git a/src/api/workspace.ts b/src/api/workspace.ts index df8223ce..9afe5781 100644 --- a/src/api/workspace.ts +++ b/src/api/workspace.ts @@ -1,20 +1,23 @@ -import { type Api } from "coder/site/src/api/api"; -import { - type ProvisionerJobLog, - type TemplateVersionParameter, - type Workspace, - type WorkspaceAgentLog, -} from "coder/site/src/api/typesGenerated"; import { spawn } from "node:child_process"; import * as vscode from "vscode"; -import { type FeatureSet } from "../featureSet"; import { getGlobalShellFlags, type CliAuth } from "../settings/cli"; import { escapeCommandArg } from "../util"; -import { type UnidirectionalStream } from "../websocket/eventStreamConnection"; import { errToStr, createWorkspaceIdentifier } from "./api-helper"; -import { type CoderApi } from "./coderApi"; +import { collectUpdateParameters } from "./updateParameters"; + +import type { Api } from "coder/site/src/api/api"; +import type { + ProvisionerJobLog, + Workspace, + WorkspaceAgentLog, +} from "coder/site/src/api/typesGenerated"; + +import type { FeatureSet } from "../featureSet"; +import type { UnidirectionalStream } from "../websocket/eventStreamConnection"; + +import type { CoderApi } from "./coderApi"; /** Opens a stream once; subsequent open() calls are no-ops until closed. */ export class LazyStream { @@ -55,6 +58,7 @@ interface CliContext { featureSet: FeatureSet; } +/** Streams CLI output via `ctx.write`; rejects with stderr on non-zero exit. */ function runCliCommand(ctx: CliContext, args: string[]): Promise { return new Promise((resolve, reject) => { const fullArgs = [ @@ -117,7 +121,10 @@ export async function updateWorkspace(ctx: CliContext): Promise { return updateWorkspaceVersion(ctx); } - const paramArgs = await collectUpdateParameters(ctx); + const paramArgs = await collectUpdateParameters( + ctx.restClient, + ctx.workspace, + ); await runCliCommand(ctx, ["update", ...paramArgs]); return ctx.restClient.getWorkspace(ctx.workspace.id); } @@ -128,7 +135,7 @@ async function updateWorkspaceVersion(ctx: CliContext): Promise { const stopBuild = await ctx.restClient.stopWorkspace(ctx.workspace.id); const stoppedJob = await ctx.restClient.waitForBuild(stopBuild); if (stoppedJob?.status === "canceled") { - throw new Error("Workspace update canceled during stop"); + throw new Error("Workspace update cancelled during stop"); } } @@ -137,179 +144,6 @@ async function updateWorkspaceVersion(ctx: CliContext): Promise { return ctx.restClient.getWorkspace(ctx.workspace.id); } -async function collectUpdateParameters(ctx: CliContext): Promise { - const newParams = await ctx.restClient.getTemplateVersionRichParameters( - ctx.workspace.template_active_version_id, - ); - const candidates = newParams.filter((p) => p.required && !p.default_value); - if (candidates.length === 0) return []; - - const currentValues = await ctx.restClient.getWorkspaceBuildParameters( - ctx.workspace.latest_build.id, - ); - const existing = new Set(currentValues.map((p) => p.name)); - const toPrompt = candidates.filter((p) => !existing.has(p.name)); - - const args: string[] = []; - for (let i = 0; i < toPrompt.length; i++) { - const value = await promptForParameter(toPrompt[i], i + 1, toPrompt.length); - if (value === undefined) { - throw new Error("Workspace update cancelled"); - } - args.push("--parameter", escapeCommandArg(`${toPrompt[i].name}=${value}`)); - } - return args; -} - -function promptForParameter( - param: TemplateVersionParameter, - step: number, - totalSteps: number, -): Promise { - const title = param.display_name || param.name; - const items = quickPickItems(param); - - if (items) { - const multi = param.form_type === "multi-select"; - const qp = vscode.window.createQuickPick<(typeof items)[number]>(); - qp.title = title; - qp.step = step; - qp.totalSteps = totalSteps; - qp.placeholder = param.description_plaintext; - qp.items = items; - qp.canSelectMany = multi; - qp.ignoreFocusOut = true; - return untilHidden(qp, () => { - if (multi) { - return qp.selectedItems.length > 0 - ? JSON.stringify(qp.selectedItems.map((i) => i.value)) - : undefined; - } - return qp.selectedItems[0]?.value; - }); - } - - const input = vscode.window.createInputBox(); - input.title = title; - input.step = step; - input.totalSteps = totalSteps; - input.prompt = param.description_plaintext; - input.placeholder = formatConstraint(param); - input.value = param.default_value; - input.ignoreFocusOut = true; - const validate = makeValidator(param); - const refresh = () => { - input.validationMessage = validate(input.value).message ?? ""; - }; - refresh(); - input.onDidChangeValue(refresh); - return untilHidden(input, () => - validate(input.value).ok ? input.value : undefined, - ); -} - -function untilHidden( - qi: vscode.InputBox | vscode.QuickPick, - onAccept: () => T | undefined, -): Promise { - return new Promise((resolve) => { - let done = false; - const finish = (value: T | undefined) => { - if (done) return; - done = true; - resolve(value); - qi.dispose(); - }; - qi.onDidAccept(() => { - const value = onAccept(); - if (value !== undefined) finish(value); - }); - qi.onDidHide(() => finish(undefined)); - qi.show(); - }); -} - -/** - * Returns picker items if the param needs a chooser, otherwise undefined. - * Anything that falls through gets a free-form text input. - */ -function quickPickItems( - param: TemplateVersionParameter, -): Array | undefined { - if (param.type === "bool") { - return [ - { label: "True", value: "true" }, - { label: "False", value: "false" }, - ]; - } - if (param.options.length > 0) { - return param.options.map((o) => ({ - label: o.name, - description: o.description, - value: o.value, - })); - } - return undefined; -} - -function formatConstraint(param: TemplateVersionParameter): string { - if (param.type === "number") { - const lo = param.validation_min; - const hi = param.validation_max; - if (lo !== undefined && hi !== undefined) return `between ${lo} and ${hi}`; - if (lo !== undefined) return `at least ${lo}`; - if (hi !== undefined) return `at most ${hi}`; - return "a number"; - } - if (param.validation_regex) { - return param.validation_error || `must match ${param.validation_regex}`; - } - return ""; -} - -/** - * Returns `{ ok, message }`: `ok` gates submission, `message` (if any) is - * shown inline. Empty input on a required param blocks submit silently. - * Coder regexes are RE2; on parse failure we defer to server-side validation. - */ -function makeValidator( - param: TemplateVersionParameter, -): (input: string) => { ok: boolean; message?: string } { - let re: RegExp | undefined; - if (param.validation_regex) { - try { - re = new RegExp(param.validation_regex); - } catch { - re = undefined; - } - } - return (input) => { - if (!input) return { ok: !param.required }; - if (param.type === "number") { - const n = Number(input); - if (!Number.isFinite(n)) { - return { ok: false, message: "Must be a number" }; - } - if (param.validation_min !== undefined && n < param.validation_min) { - return { - ok: false, - message: `Must be at least ${param.validation_min}`, - }; - } - if (param.validation_max !== undefined && n > param.validation_max) { - return { - ok: false, - message: `Must be at most ${param.validation_max}`, - }; - } - } - if (re && !re.test(input)) { - return { ok: false, message: param.validation_error || "Invalid format" }; - } - return { ok: true }; - }; -} - /** * Streams build logs in real-time via a callback. * Returns the websocket for lifecycle management. diff --git a/src/remote/workspaceStateMachine.ts b/src/remote/workspaceStateMachine.ts index c24dc422..105c0ac8 100644 --- a/src/remote/workspaceStateMachine.ts +++ b/src/remote/workspaceStateMachine.ts @@ -5,6 +5,7 @@ import { errToStr, extractAgents, } from "../api/api-helper"; +import { WorkspaceUpdateCancelledError } from "../api/updateParameters"; import { LazyStream, startWorkspace, @@ -103,6 +104,8 @@ export class WorkspaceStateMachine implements vscode.Disposable { ); if (updated) { workspace = updated; + // Agent IDs may have changed after an update. + this.agent = undefined; if (workspace.latest_build.status === "running") break; return false; } @@ -285,6 +288,12 @@ export class WorkspaceStateMachine implements vscode.Disposable { this.workspace = await updateWorkspace(this.buildCliContext(workspace)); return this.workspace; } catch (error) { + if (error instanceof WorkspaceUpdateCancelledError) { + this.logger.info( + `Update cancelled for ${workspaceName}; continuing with the existing version.`, + ); + return undefined; + } const reason = errToStr(error); this.logger.warn(`Update failed for ${workspaceName}: ${reason}`); vscode.window.showWarningMessage( diff --git a/src/settings/headers.ts b/src/settings/headers.ts index bb1c4594..2381b27e 100644 --- a/src/settings/headers.ts +++ b/src/settings/headers.ts @@ -1,6 +1,4 @@ -import * as os from "node:os"; - -import { escapeCommandArg } from "../util"; +import { escapeShellArg } from "../util"; import type { WorkspaceConfiguration } from "vscode"; @@ -19,18 +17,9 @@ export function getHeaderCommand( export function getHeaderArgs( config: Pick, ): string[] { - // Escape a command line to be executed by the Coder binary, so ssh doesn't substitute variables. - const escapeSubcommand: (str: string) => string = - os.platform() === "win32" - ? // On Windows variables are %VAR%, and we need to use double quotes. - (str) => escapeCommandArg(str).replace(/%/g, "%%") - : // On *nix we can use single quotes to escape $VARS. - // Note single quotes cannot be escaped inside single quotes. - (str) => `'${str.replace(/'/g, "'\\''")}'`; - const command = getHeaderCommand(config); if (!command) { return []; } - return ["--header-command", escapeSubcommand(command)]; + return ["--header-command", escapeShellArg(command)]; } diff --git a/src/util.ts b/src/util.ts index 9b3ec7ff..e00bbd24 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1,4 +1,4 @@ -import * as os from "node:os"; +import os from "node:os"; import url from "node:url"; export interface AuthorityParts { @@ -207,6 +207,17 @@ export function escapeCommandArg(arg: string): string { return `"${escapedString}"`; } +/** + * Cross-platform shell quoting that blocks variable expansion. Use for + * values from outside the user's local settings (e.g. server-controlled). + */ +export function escapeShellArg(arg: string): string { + if (os.platform() === "win32") { + return escapeCommandArg(arg).replace(/%/g, "%%"); + } + return `'${arg.replace(/'/g, "'\\''")}'`; +} + /** * Generate a temporary file path by appending a suffix with a random component. * The suffix describes the purpose of the temp file (e.g. "temp", "old"). diff --git a/test/unit/api/updateParameters.test.ts b/test/unit/api/updateParameters.test.ts new file mode 100644 index 00000000..277361d3 --- /dev/null +++ b/test/unit/api/updateParameters.test.ts @@ -0,0 +1,396 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; + +import { + collectUpdateParameters, + WorkspaceUpdateCancelledError, +} from "@/api/updateParameters"; + +import { workspace as createWorkspace } from "@repo/mocks"; + +import { shellQuote } from "../../utils/platform"; + +import type { Api } from "coder/site/src/api/api"; +import type { TemplateVersionParameter } from "coder/site/src/api/typesGenerated"; + +function param(overrides: Partial = {}) { + return { + name: "environment", + display_name: "Environment", + description: "", + description_plaintext: "", + type: "string", + form_type: "input", + mutable: true, + default_value: "", + icon: "", + options: [], + required: true, + ephemeral: false, + ...overrides, + } as TemplateVersionParameter; +} + +function createCollectCtx( + params: Array> = [], + previousValues: Array<{ name: string; value: string }> = [], +) { + const workspace = createWorkspace(); + const restClient = { + getTemplateVersionRichParameters: vi + .fn() + .mockResolvedValue(params.map(param)), + getWorkspaceBuildParameters: vi.fn().mockResolvedValue(previousValues), + }; + return { + workspace, + restClient: restClient as unknown as Api, + mocks: restClient, + }; +} + +interface QuickInputMock { + mock: Record & { + show: ReturnType; + dispose: ReturnType; + }; + accept: (overrides?: Record) => void; + change: (value: string) => void; + hide: () => void; +} + +function quickInputMock(): QuickInputMock { + let acceptCb: () => void = () => {}; + let hideCb: () => void = () => {}; + let changeCb: (v: string) => void = () => {}; + const mock = { + title: "", + step: 0, + totalSteps: 0, + prompt: "", + placeholder: "", + value: "", + validationMessage: "", + ignoreFocusOut: false, + items: [] as readonly unknown[], + selectedItems: [] as readonly unknown[], + onDidAccept: vi.fn((cb: () => void) => { + acceptCb = cb; + return { dispose: vi.fn() }; + }), + onDidHide: vi.fn((cb: () => void) => { + hideCb = cb; + return { dispose: vi.fn() }; + }), + onDidChangeValue: vi.fn((cb: (v: string) => void) => { + changeCb = cb; + return { dispose: vi.fn() }; + }), + show: vi.fn(), + dispose: vi.fn(), + }; + return { + mock, + accept(overrides) { + Object.assign(mock, overrides ?? {}); + if (overrides && "value" in overrides) changeCb(mock.value); + acceptCb(); + }, + change(value) { + mock.value = value; + changeCb(value); + }, + hide() { + hideCb(); + }, + }; +} + +function mockCreateInputBox() { + const qi = quickInputMock(); + vi.mocked(vscode.window.createInputBox).mockReturnValue( + qi.mock as unknown as vscode.InputBox, + ); + return qi; +} + +function mockCreateQuickPick() { + const qi = quickInputMock(); + vi.mocked(vscode.window.createQuickPick).mockReturnValue( + qi.mock as unknown as vscode.QuickPick, + ); + return qi; +} + +async function waitShown(qi: QuickInputMock): Promise { + await vi.waitFor(() => expect(qi.mock.show).toHaveBeenCalled()); +} + +describe("collectUpdateParameters", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + interface CollectCase { + kind: string; + param: Partial; + mock: () => QuickInputMock; + accept: Record; + expected: string[]; + } + + it.each([ + { + kind: "text input", + param: { name: "environment" }, + mock: mockCreateInputBox, + accept: { value: "dev" }, + expected: ["--parameter", shellQuote("environment=dev")], + }, + { + kind: "bool quick pick", + param: { name: "enabled", type: "bool" }, + mock: mockCreateQuickPick, + accept: { selectedItems: [{ value: "true" }] }, + expected: ["--parameter", shellQuote("enabled=true")], + }, + { + kind: "options quick pick", + param: { + name: "size", + options: [ + { name: "Small", description: "", value: "s", icon: "" }, + { name: "Large", description: "", value: "l", icon: "" }, + ], + }, + mock: mockCreateQuickPick, + accept: { selectedItems: [{ value: "l" }] }, + expected: ["--parameter", shellQuote("size=l")], + }, + { + kind: "multi-select quick pick (JSON array)", + param: { + name: "regions", + form_type: "multi-select", + options: [ + { name: "US", description: "", value: "us", icon: "" }, + { name: "EU", description: "", value: "eu", icon: "" }, + ], + }, + mock: mockCreateQuickPick, + accept: { selectedItems: [{ value: "us" }, { value: "eu" }] }, + expected: ["--parameter", shellQuote('regions=["us","eu"]')], + }, + ])( + "collects the value via $kind", + async ({ param: p, mock, accept, expected }) => { + const { restClient, workspace } = createCollectCtx([p]); + const qi = mock(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept(accept); + + await expect(result).resolves.toEqual(expected); + }, + ); + + it("escapes shell metacharacters in server-controlled values", async () => { + const { restClient, workspace } = createCollectCtx([{ name: "evil" }]); + const qi = mockCreateInputBox(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ value: "$(rm -rf /)" }); + + await expect(result).resolves.toEqual([ + "--parameter", + shellQuote("evil=$(rm -rf /)"), + ]); + }); + + it("skips parameters that already have a value or default", async () => { + const { restClient, workspace } = createCollectCtx( + [ + { name: "existing" }, + { name: "with_default", default_value: "foo" }, + { name: "optional", required: false }, + ], + [{ name: "existing", value: "kept" }], + ); + + await expect( + collectUpdateParameters(restClient, workspace), + ).resolves.toEqual([]); + expect(vscode.window.createInputBox).not.toHaveBeenCalled(); + }); + + it("throws WorkspaceUpdateCancelledError when the prompt is dismissed", async () => { + const { restClient, workspace } = createCollectCtx([{}]); + const qi = mockCreateInputBox(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.hide(); + + await expect(result).rejects.toBeInstanceOf(WorkspaceUpdateCancelledError); + }); + + it("steps the input title across multiple required params", async () => { + const { restClient, workspace } = createCollectCtx([ + { name: "a" }, + { name: "b" }, + ]); + const inputs = [quickInputMock(), quickInputMock()]; + vi.mocked(vscode.window.createInputBox) + .mockReturnValueOnce(inputs[0].mock as unknown as vscode.InputBox) + .mockReturnValueOnce(inputs[1].mock as unknown as vscode.InputBox); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(inputs[0]); + inputs[0].accept({ value: "first" }); + await waitShown(inputs[1]); + inputs[1].accept({ value: "second" }); + await result; + + expect(inputs.map((i) => [i.mock.step, i.mock.totalSteps])).toEqual([ + [1, 2], + [2, 2], + ]); + }); +}); + +describe("parameter prompt validation", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + async function withInputBox( + p: Partial, + fn: (qi: QuickInputMock) => void, + ) { + const { restClient, workspace } = createCollectCtx([p]); + const qi = mockCreateInputBox(); + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + fn(qi); + qi.hide(); + await result.catch(() => {}); + } + + it("silently blocks accept on empty required input", async () => { + await withInputBox({ name: "x" }, (qi) => { + qi.accept({ value: "" }); + expect(qi.mock.validationMessage).toBe(""); + expect(qi.mock.dispose).not.toHaveBeenCalled(); + }); + }); + + interface ValidationCase { + kind: string; + param: Partial; + input: string; + expected: string; + } + + it.each([ + { + kind: "number below validation_min", + param: { type: "number", validation_min: 5 }, + input: "1", + expected: "Must be at least 5", + }, + { + kind: "number above validation_max", + param: { type: "number", validation_max: 9 }, + input: "10", + expected: "Must be at most 9", + }, + { + kind: "non-numeric input on number param", + param: { type: "number" }, + input: "abc", + expected: "Must be a number", + }, + { + kind: "regex mismatch with default message", + param: { validation_regex: "^x" }, + input: "y", + expected: "Invalid format", + }, + { + kind: "regex mismatch with {value} substitution", + param: { + validation_regex: "^x", + validation_error: "Value {value} is not allowed", + }, + input: "y", + expected: "Value y is not allowed", + }, + { + kind: "number out-of-range with {min}/{max} substitution", + param: { + type: "number", + validation_min: 1, + validation_max: 5, + validation_error: "Pick {value} between {min} and {max}", + }, + input: "7", + expected: "Pick 7 between 1 and 5", + }, + ])("surfaces $kind", async ({ param: p, input, expected }) => { + await withInputBox({ name: "x", ...p }, (qi) => { + qi.change(input); + expect(qi.mock.validationMessage).toBe(expected); + }); + }); + + it("treats JSON null on validation_min/max as unset", async () => { + await withInputBox( + { + name: "n", + type: "number", + validation_min: null as unknown as number, + validation_max: null as unknown as number, + }, + (qi) => { + qi.change("42"); + expect(qi.mock.validationMessage).toBe(""); + expect(qi.mock.placeholder).toBe("a number"); + }, + ); + }); + + interface PlaceholderCase { + kind: string; + param: Partial; + expected: string; + } + + it.each([ + { + kind: "between bounds", + param: { type: "number", validation_min: 1, validation_max: 10 }, + expected: "between 1 and 10", + }, + { + kind: "lower bound only", + param: { type: "number", validation_min: 1 }, + expected: "at least 1", + }, + { + kind: "upper bound only", + param: { type: "number", validation_max: 10 }, + expected: "at most 10", + }, + { + kind: "regex with custom error", + param: { validation_regex: "^x", validation_error: "must start with x" }, + expected: "must start with x", + }, + ])("renders placeholder for $kind", async ({ param: p, expected }) => { + await withInputBox({ name: "x", ...p }, (qi) => { + expect(qi.mock.placeholder).toBe(expected); + }); + }); +}); diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index fa78483e..d574f93d 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -3,18 +3,18 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; import { LazyStream, startWorkspace, updateWorkspace } from "@/api/workspace"; -import { type FeatureSet } from "@/featureSet"; -import { type UnidirectionalStream } from "@/websocket/eventStreamConnection"; import { workspace as createWorkspace } from "@repo/mocks"; import type { Api } from "coder/site/src/api/api"; import type { - TemplateVersionParameter, Workspace, WorkspaceBuild, } from "coder/site/src/api/typesGenerated"; +import type { FeatureSet } from "@/featureSet"; +import type { UnidirectionalStream } from "@/websocket/eventStreamConnection"; + vi.mock(import("node:child_process"), async (importOriginal) => ({ ...(await importOriginal()), spawn: vi.fn(), @@ -109,10 +109,8 @@ function controlSpawn() { }; proc.stdout = new EventEmitter(); proc.stderr = new EventEmitter(); - let resolveSpawned!: () => void; - const spawned = new Promise((resolve) => { - resolveSpawned = resolve; - }); + const { promise: spawned, resolve: resolveSpawned } = + Promise.withResolvers(); vi.mocked(spawn).mockImplementation(() => { resolveSpawned(); return proc as never; @@ -129,89 +127,6 @@ function controlSpawn() { }; } -interface QuickInputMock { - mock: Record & { - show: ReturnType; - dispose: ReturnType; - }; - accept: (overrides?: Record) => void; - hide: () => void; -} - -function quickInputMock(): QuickInputMock { - let acceptCb: () => void = () => {}; - let hideCb: () => void = () => {}; - let changeCb: (v: string) => void = () => {}; - const mock = { - title: "", - step: 0, - totalSteps: 0, - prompt: "", - placeholder: "", - value: "", - validationMessage: "", - ignoreFocusOut: false, - items: [] as readonly unknown[], - selectedItems: [] as readonly unknown[], - onDidAccept: vi.fn((cb: () => void) => { - acceptCb = cb; - return { dispose: vi.fn() }; - }), - onDidHide: vi.fn((cb: () => void) => { - hideCb = cb; - return { dispose: vi.fn() }; - }), - onDidChangeValue: vi.fn((cb: (v: string) => void) => { - changeCb = cb; - return { dispose: vi.fn() }; - }), - show: vi.fn(), - dispose: vi.fn(), - }; - return { - mock, - accept(overrides) { - Object.assign(mock, overrides ?? {}); - if (overrides && "value" in overrides) changeCb(mock.value); - acceptCb(); - }, - hide() { - hideCb(); - }, - }; -} - -function mockCreateInputBox() { - const qi = quickInputMock(); - vi.mocked(vscode.window.createInputBox).mockReturnValue( - qi.mock as unknown as vscode.InputBox, - ); - return qi; -} - -function mockCreateQuickPick() { - const qi = quickInputMock(); - vi.mocked(vscode.window.createQuickPick).mockReturnValue( - qi.mock as unknown as vscode.QuickPick, - ); - return qi; -} - -async function flushMicrotasks(times = 4) { - for (let i = 0; i < times; i++) await Promise.resolve(); -} - -function setupUpdate( - params: Array> = [], - opts: Parameters[0] = {}, -) { - const ctxBundle = createUpdateCtx(opts); - ctxBundle.restClient.getTemplateVersionRichParameters.mockResolvedValue( - params.map(param), - ); - return { ...ctxBundle, sp: controlSpawn() }; -} - describe("LazyStream", () => { it("opens once and ignores subsequent calls", async () => { const factory: StreamFactory = vi.fn().mockResolvedValue(mockStream()); @@ -271,137 +186,11 @@ describe("LazyStream", () => { }); }); -function param(overrides: Partial = {}) { - return { - name: "environment", - display_name: "Environment", - description: "", - description_plaintext: "", - type: "string", - form_type: "input", - mutable: true, - default_value: "", - icon: "", - options: [], - required: true, - ephemeral: false, - ...overrides, - } as TemplateVersionParameter; -} - describe("updateWorkspace", () => { beforeEach(() => { vi.clearAllMocks(); }); - it.each([ - { - kind: "text input", - param: { name: "environment" } as Partial, - mock: mockCreateInputBox, - accept: { value: "dev" }, - expected: '--parameter "environment=dev"', - }, - { - kind: "bool quick pick", - param: { name: "enabled", type: "bool" }, - mock: mockCreateQuickPick, - accept: { selectedItems: [{ label: "True", value: "true" }] }, - expected: '--parameter "enabled=true"', - }, - { - kind: "options quick pick", - param: { - name: "size", - options: [ - { name: "Small", description: "", value: "s", icon: "" }, - { name: "Large", description: "", value: "l", icon: "" }, - ], - }, - mock: mockCreateQuickPick, - accept: { selectedItems: [{ value: "l" }] }, - expected: '--parameter "size=l"', - }, - { - kind: "fallback text input for unknown types", - param: { name: "x", type: "list(string)" }, - mock: mockCreateInputBox, - accept: { value: "[]" }, - expected: '--parameter "x=[]"', - }, - ])( - "collects the value via $kind", - async ({ param: p, mock, accept, expected }) => { - const { ctx, sp, finalWorkspace } = setupUpdate([p]); - const qi = mock(); - - const result = updateWorkspace(ctx); - await flushMicrotasks(); - qi.accept(accept); - await sp.close(0); - - await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith( - expect.stringContaining(expected), - expect.objectContaining({ shell: true }), - ); - }, - ); - - it("skips parameters that already have a value or default", async () => { - const { ctx, restClient, sp } = setupUpdate([ - { name: "existing" }, - { name: "with_default", default_value: "foo" }, - { name: "optional", required: false }, - ]); - restClient.getWorkspaceBuildParameters.mockResolvedValue([ - { name: "existing", value: "kept" }, - ]); - - const result = updateWorkspace(ctx); - await sp.close(0); - await result; - - expect(vscode.window.createInputBox).not.toHaveBeenCalled(); - expect(spawn).toHaveBeenCalledWith( - expect.not.stringContaining("--parameter"), - expect.anything(), - ); - }); - - it("throws when the user cancels a parameter prompt", async () => { - const { ctx } = setupUpdate([{}]); - const qi = mockCreateInputBox(); - - const result = updateWorkspace(ctx); - await flushMicrotasks(); - qi.hide(); - - await expect(result).rejects.toThrow("Workspace update cancelled"); - expect(spawn).not.toHaveBeenCalled(); - }); - - it("steps the input title across multiple required params", async () => { - const { ctx, sp } = setupUpdate([{ name: "a" }, { name: "b" }]); - const inputs = [quickInputMock(), quickInputMock()]; - vi.mocked(vscode.window.createInputBox) - .mockReturnValueOnce(inputs[0].mock as unknown as vscode.InputBox) - .mockReturnValueOnce(inputs[1].mock as unknown as vscode.InputBox); - - const result = updateWorkspace(ctx); - await flushMicrotasks(); - inputs[0].accept({ value: "first" }); - await flushMicrotasks(); - inputs[1].accept({ value: "second" }); - await sp.close(0); - await result; - - expect(inputs.map((i) => [i.mock.step, i.mock.totalSteps])).toEqual([ - [1, 2], - [2, 2], - ]); - }); - it("rejects when the process exits non-zero", async () => { const { ctx, restClient } = createUpdateCtx(); const sp = controlSpawn(); @@ -444,7 +233,7 @@ describe("updateWorkspace", () => { ); }); - it("throws before update when the API fallback stop is canceled", async () => { + it("throws before update when the API fallback stop is cancelled", async () => { const { ctx, restClient } = createUpdateCtx({ featureSet: { cliUpdate: false }, }); @@ -454,7 +243,7 @@ describe("updateWorkspace", () => { }); await expect(updateWorkspace(ctx)).rejects.toThrow( - "Workspace update canceled during stop", + "Workspace update cancelled during stop", ); expect(restClient.updateWorkspaceVersion).not.toHaveBeenCalled(); }); diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index e8ef1f0f..df401bb9 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -5,6 +5,7 @@ import { type AuthorityParts, countSubstring, escapeCommandArg, + escapeShellArg, expandPath, findPort, parseRemoteAuthority, @@ -212,6 +213,45 @@ describe("escapeCommandArg", () => { }); }); +describe("escapeShellArg", () => { + const platformSpy = vi.spyOn(os, "platform"); + afterEach(() => platformSpy.mockReset()); + + describe("on Unix", () => { + beforeEach(() => platformSpy.mockReturnValue("linux")); + + it("wraps in single quotes", () => { + expect(escapeShellArg("env=dev")).toBe("'env=dev'"); + }); + + it("escapes single quotes via the '\\'' sequence", () => { + expect(escapeShellArg("it's fine")).toBe("'it'\\''s fine'"); + }); + + it("leaves $VAR, $(...), and backticks literal inside the quotes", () => { + expect(escapeShellArg("$(echo pwned)")).toBe("'$(echo pwned)'"); + }); + }); + + describe("on Windows", () => { + beforeEach(() => platformSpy.mockReturnValue("win32")); + + it("wraps in double quotes", () => { + expect(escapeShellArg("env=dev")).toBe('"env=dev"'); + }); + + it("escapes embedded double quotes", () => { + expect(escapeShellArg('regions=["us","eu"]')).toBe( + String.raw`"regions=[\"us\",\"eu\"]"`, + ); + }); + + it("doubles percent signs to block %VAR% expansion", () => { + expect(escapeShellArg("%PATH%")).toBe('"%%PATH%%"'); + }); + }); +}); + describe("expandPath", () => { const home = os.homedir(); diff --git a/test/utils/platform.test.ts b/test/utils/platform.test.ts index 35bbc866..5d37ddd0 100644 --- a/test/utils/platform.test.ts +++ b/test/utils/platform.test.ts @@ -3,7 +3,15 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; -import { beforeAll, describe, expect, it } from "vitest"; +import { + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; import { expectPathsEqual, @@ -11,6 +19,7 @@ import { isWindows, printCommand, printEnvCommand, + shellQuote, shimExecFile, writeExecutable, writeStdoutJs, @@ -159,4 +168,43 @@ describe("platform utils", () => { expect(mod.spawn).toBe(cp.spawn); }); }); + + describe("shellQuote", () => { + const platformSpy = vi.spyOn(os, "platform"); + afterEach(() => platformSpy.mockReset()); + + describe("on Unix", () => { + beforeEach(() => platformSpy.mockReturnValue("linux")); + + it("wraps in single quotes", () => { + expect(shellQuote("env=dev")).toBe("'env=dev'"); + }); + + it("escapes single quotes via the '\\'' sequence", () => { + expect(shellQuote("it's fine")).toBe("'it'\\''s fine'"); + }); + + it("keeps $VAR, $(...), and backticks literal inside the quotes", () => { + expect(shellQuote("$(echo pwned)")).toBe("'$(echo pwned)'"); + }); + }); + + describe("on Windows", () => { + beforeEach(() => platformSpy.mockReturnValue("win32")); + + it("wraps in double quotes", () => { + expect(shellQuote("env=dev")).toBe('"env=dev"'); + }); + + it("escapes embedded double quotes", () => { + expect(shellQuote('regions=["us","eu"]')).toBe( + String.raw`"regions=[\"us\",\"eu\"]"`, + ); + }); + + it("doubles percent signs to block %VAR% expansion", () => { + expect(shellQuote("%PATH%")).toBe('"%%PATH%%"'); + }); + }); + }); }); diff --git a/test/utils/platform.ts b/test/utils/platform.ts index 5edb8587..01e7c1cf 100644 --- a/test/utils/platform.ts +++ b/test/utils/platform.ts @@ -124,6 +124,17 @@ export function quoteCommand(value: string): string { return `${quote}${value}${quote}`; } +/** + * Test-side mirror of `escapeShellArg` from src/util.ts. Wraps in single + * quotes on Unix and double quotes (with `"` and `%` escaped) on Windows. + */ +export function shellQuote(value: string): string { + if (isWindows()) { + return `"${value.replace(/"/g, '\\"').replace(/%/g, "%%")}"`; + } + return `'${value.replace(/'/g, "'\\''")}'`; +} + export function expectPathsEqual(actual: string, expected: string) { expect(normalizePath(actual)).toBe(normalizePath(expected)); } From 9f10c2c9a96a65a19b98d6f3a3e176c5a4368f9f Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 12 May 2026 15:43:30 +0300 Subject: [PATCH 5/7] fix: address R3 review feedback - Drop `shell: true` from `runCliCommand`; pass args as an array via `getGlobalFlags` (raw). Eliminates Windows cmd.exe quoting concerns entirely (DEREM-17) and lets `--parameter` values pass through verbatim. - Close child stdin after spawn so unexpected interactive CLI prompts see EOF and error out instead of hanging (DEREM-18). - Guard the running case of the state machine against post-update builds that aren't running yet (DEREM-19). - Drop client-side regex validation (server validates with RE2, ReDoS-safe) to prevent extension-host freezes on hostile patterns (DEREM-22). - Expand `${env:VAR}` in `coder.globalFlags` so users can reference environment variables now that there's no outer shell expansion. - Document the new globalFlags expansion semantics in package.json. --- package.json | 2 +- src/api/updateParameters.ts | 37 ++++---------- src/api/workspace.ts | 10 ++-- src/remote/remote.ts | 4 +- src/remote/workspaceStateMachine.ts | 5 +- src/settings/cli.ts | 30 +++++++---- src/util.ts | 3 +- test/unit/api/updateParameters.test.ts | 39 +++++---------- test/unit/api/workspace.test.ts | 43 +++++++++++++--- test/unit/cliConfig.test.ts | 31 ++++++++++-- .../unit/remote/workspaceStateMachine.test.ts | 22 +++++++- test/unit/util.test.ts | 12 +++-- test/utils/platform.test.ts | 50 +------------------ test/utils/platform.ts | 11 ---- 14 files changed, 146 insertions(+), 153 deletions(-) diff --git a/package.json b/package.json index 1e652166..83887c87 100644 --- a/package.json +++ b/package.json @@ -160,7 +160,7 @@ ] }, "coder.globalFlags": { - "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.", + "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nValues are passed directly to the CLI without a shell, so `$VAR` and `%VAR%` are **not** expanded. Use `${env:VAR}` to reference environment variables (e.g. `--cfg=${env:HOME}/cfg`); missing variables resolve to an empty string.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.", "type": "array", "items": { "type": "string" diff --git a/src/api/updateParameters.ts b/src/api/updateParameters.ts index 35a8fdcb..f6cde17f 100644 --- a/src/api/updateParameters.ts +++ b/src/api/updateParameters.ts @@ -1,7 +1,5 @@ import * as vscode from "vscode"; -import { escapeShellArg } from "../util"; - import type { Api } from "coder/site/src/api/api"; import type { TemplateVersionParameter, @@ -25,15 +23,15 @@ export async function collectUpdateParameters( restClient: Api, workspace: Workspace, ): Promise { - const newParams = await restClient.getTemplateVersionRichParameters( - workspace.template_active_version_id, - ); + const [newParams, currentValues] = await Promise.all([ + restClient.getTemplateVersionRichParameters( + workspace.template_active_version_id, + ), + restClient.getWorkspaceBuildParameters(workspace.latest_build.id), + ]); const candidates = newParams.filter((p) => p.required && !p.default_value); if (candidates.length === 0) return []; - const currentValues = await restClient.getWorkspaceBuildParameters( - workspace.latest_build.id, - ); const existing = new Set(currentValues.map((p) => p.name)); const toPrompt = candidates.filter((p) => !existing.has(p.name)); @@ -44,8 +42,7 @@ export async function collectUpdateParameters( if (value === undefined) { throw new WorkspaceUpdateCancelledError(); } - // Server-controlled values; block shell expansion under `shell: true`. - args.push("--parameter", escapeShellArg(`${param.name}=${value}`)); + args.push("--parameter", `${param.name}=${value}`); } return args; } @@ -178,20 +175,12 @@ function substituteTemplate( } /** - * Returns `{ ok, message }`; invalid RE2 regexes fall through to server-side - * validation. + * Returns `{ ok, message }`. Regex constraints are intentionally not tested + * client-side; server validates with RE2 (linear-time, ReDoS-safe). */ function makeValidator( param: TemplateVersionParameter, ): (input: string) => { ok: boolean; message?: string } { - let re: RegExp | undefined; - if (param.validation_regex) { - try { - re = new RegExp(param.validation_regex); - } catch { - re = undefined; - } - } return (input) => { if (!input) return { ok: !param.required }; if (param.type === "number") { @@ -216,14 +205,6 @@ function makeValidator( }; } } - if (re && !re.test(input)) { - return { - ok: false, - message: - substituteTemplate(param.validation_error, param, input) || - "Invalid format", - }; - } return { ok: true }; }; } diff --git a/src/api/workspace.ts b/src/api/workspace.ts index 9afe5781..449895b5 100644 --- a/src/api/workspace.ts +++ b/src/api/workspace.ts @@ -1,8 +1,7 @@ import { spawn } from "node:child_process"; import * as vscode from "vscode"; -import { getGlobalShellFlags, type CliAuth } from "../settings/cli"; -import { escapeCommandArg } from "../util"; +import { getGlobalFlags, type CliAuth } from "../settings/cli"; import { errToStr, createWorkspaceIdentifier } from "./api-helper"; import { collectUpdateParameters } from "./updateParameters"; @@ -62,12 +61,13 @@ interface CliContext { function runCliCommand(ctx: CliContext, args: string[]): Promise { return new Promise((resolve, reject) => { const fullArgs = [ - ...getGlobalShellFlags(vscode.workspace.getConfiguration(), ctx.auth), + ...getGlobalFlags(vscode.workspace.getConfiguration(), ctx.auth), ...args, createWorkspaceIdentifier(ctx.workspace), ]; - const cmd = `${escapeCommandArg(ctx.binPath)} ${fullArgs.join(" ")}`; - const proc = spawn(cmd, { shell: true }); + const proc = spawn(ctx.binPath, fullArgs); + // Unexpected prompts EOF instead of hanging forever. + proc.stdin.end(); proc.stdout.on("data", (data: Buffer) => { ctx.write(data.toString()); diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 7dadf70d..045ba8de 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -41,9 +41,9 @@ import { type LoginCoordinator } from "../login/loginCoordinator"; import { OAuthSessionManager } from "../oauth/sessionManager"; import { type CliAuth, - getGlobalFlagsRaw, getGlobalShellFlags, getSshFlags, + getUserGlobalFlags, resolveCliAuth, } from "../settings/cli"; import { getHeaderCommand } from "../settings/headers"; @@ -437,7 +437,7 @@ export class Remote { setting: "coder.globalFlags", title: "Global Flags", getValue: () => - getGlobalFlagsRaw(vscode.workspace.getConfiguration()), + getUserGlobalFlags(vscode.workspace.getConfiguration()), }, { setting: "coder.headerCommand", diff --git a/src/remote/workspaceStateMachine.ts b/src/remote/workspaceStateMachine.ts index 105c0ac8..0b9a299b 100644 --- a/src/remote/workspaceStateMachine.ts +++ b/src/remote/workspaceStateMachine.ts @@ -78,6 +78,7 @@ export class WorkspaceStateMachine implements vscode.Disposable { workspace = updated; // Agent IDs may have changed after an update. this.agent = undefined; + if (workspace.latest_build.status !== "running") return false; } break; } @@ -106,8 +107,8 @@ export class WorkspaceStateMachine implements vscode.Disposable { workspace = updated; // Agent IDs may have changed after an update. this.agent = undefined; - if (workspace.latest_build.status === "running") break; - return false; + if (workspace.latest_build.status !== "running") return false; + break; } // Either we weren't in update mode, or the update failed: start. await this.triggerStart(workspace, workspaceName, progress); diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 23185139..d33a37de 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -12,12 +12,21 @@ export type CliAuth = | { mode: "url"; url: string }; /** - * Returns the raw global flags from user configuration. + * Returns the user's `coder.globalFlags` with `${env:VAR}` references + * substituted from `process.env`. Missing variables resolve to an empty + * string, matching VS Code's behaviour for built-in `${env:VAR}` sites. */ -export function getGlobalFlagsRaw( +export function getUserGlobalFlags( configs: Pick, ): string[] { - return configs.get("coder.globalFlags", []); + return configs + .get("coder.globalFlags", []) + .map((flag) => + flag.replace( + /\$\{env:([A-Za-z_][A-Za-z0-9_]*)\}/g, + (_, name: string) => process.env[name] ?? "", + ), + ); } /** @@ -52,26 +61,25 @@ function buildGlobalFlags( ? ["--url", esc(auth.url)] : ["--global-config", esc(auth.configDir)]; - const raw = getGlobalFlagsRaw(configs); - const filtered = stripManagedFlags(raw); + const filtered = stripManagedFlags(getUserGlobalFlags(configs)); return [...filtered, ...authFlags, ...getHeaderArgs(configs)]; } -function stripManagedFlags(rawFlags: string[]): string[] { +function stripManagedFlags(flags: string[]): string[] { const filtered: string[] = []; - for (let i = 0; i < rawFlags.length; i++) { - if (isFlag(rawFlags[i], "--use-keyring")) { + for (let i = 0; i < flags.length; i++) { + if (isFlag(flags[i], "--use-keyring")) { continue; } - if (isFlag(rawFlags[i], "--global-config")) { + if (isFlag(flags[i], "--global-config")) { // Skip the next item too when the value is a separate entry. - if (rawFlags[i] === "--global-config") { + if (flags[i] === "--global-config") { i++; } continue; } - filtered.push(rawFlags[i]); + filtered.push(flags[i]); } return filtered; } diff --git a/src/util.ts b/src/util.ts index e00bbd24..22a63800 100644 --- a/src/util.ts +++ b/src/util.ts @@ -213,7 +213,8 @@ export function escapeCommandArg(arg: string): string { */ export function escapeShellArg(arg: string): string { if (os.platform() === "win32") { - return escapeCommandArg(arg).replace(/%/g, "%%"); + const escaped = arg.replace(/"/g, '""').replace(/%/g, "%%"); + return `"${escaped}"`; } return `'${arg.replace(/'/g, "'\\''")}'`; } diff --git a/test/unit/api/updateParameters.test.ts b/test/unit/api/updateParameters.test.ts index 277361d3..f8d83f23 100644 --- a/test/unit/api/updateParameters.test.ts +++ b/test/unit/api/updateParameters.test.ts @@ -8,8 +8,6 @@ import { import { workspace as createWorkspace } from "@repo/mocks"; -import { shellQuote } from "../../utils/platform"; - import type { Api } from "coder/site/src/api/api"; import type { TemplateVersionParameter } from "coder/site/src/api/typesGenerated"; @@ -145,14 +143,14 @@ describe("collectUpdateParameters", () => { param: { name: "environment" }, mock: mockCreateInputBox, accept: { value: "dev" }, - expected: ["--parameter", shellQuote("environment=dev")], + expected: ["--parameter", "environment=dev"], }, { kind: "bool quick pick", param: { name: "enabled", type: "bool" }, mock: mockCreateQuickPick, accept: { selectedItems: [{ value: "true" }] }, - expected: ["--parameter", shellQuote("enabled=true")], + expected: ["--parameter", "enabled=true"], }, { kind: "options quick pick", @@ -165,7 +163,7 @@ describe("collectUpdateParameters", () => { }, mock: mockCreateQuickPick, accept: { selectedItems: [{ value: "l" }] }, - expected: ["--parameter", shellQuote("size=l")], + expected: ["--parameter", "size=l"], }, { kind: "multi-select quick pick (JSON array)", @@ -179,7 +177,7 @@ describe("collectUpdateParameters", () => { }, mock: mockCreateQuickPick, accept: { selectedItems: [{ value: "us" }, { value: "eu" }] }, - expected: ["--parameter", shellQuote('regions=["us","eu"]')], + expected: ["--parameter", 'regions=["us","eu"]'], }, ])( "collects the value via $kind", @@ -195,7 +193,7 @@ describe("collectUpdateParameters", () => { }, ); - it("escapes shell metacharacters in server-controlled values", async () => { + it("passes server-controlled values through verbatim (no shell expansion path)", async () => { const { restClient, workspace } = createCollectCtx([{ name: "evil" }]); const qi = mockCreateInputBox(); @@ -203,10 +201,7 @@ describe("collectUpdateParameters", () => { await waitShown(qi); qi.accept({ value: "$(rm -rf /)" }); - await expect(result).resolves.toEqual([ - "--parameter", - shellQuote("evil=$(rm -rf /)"), - ]); + await expect(result).resolves.toEqual(["--parameter", "evil=$(rm -rf /)"]); }); it("skips parameters that already have a value or default", async () => { @@ -312,21 +307,6 @@ describe("parameter prompt validation", () => { input: "abc", expected: "Must be a number", }, - { - kind: "regex mismatch with default message", - param: { validation_regex: "^x" }, - input: "y", - expected: "Invalid format", - }, - { - kind: "regex mismatch with {value} substitution", - param: { - validation_regex: "^x", - validation_error: "Value {value} is not allowed", - }, - input: "y", - expected: "Value y is not allowed", - }, { kind: "number out-of-range with {min}/{max} substitution", param: { @@ -345,6 +325,13 @@ describe("parameter prompt validation", () => { }); }); + it("does not evaluate validation_regex client-side (ReDoS guard)", async () => { + await withInputBox({ name: "r", validation_regex: "^(a+)+$" }, (qi) => { + qi.change("aaaaaaaaaaaaaaaaaaaaaaaaaaa!"); + expect(qi.mock.validationMessage).toBe(""); + }); + }); + it("treats JSON null on validation_min/max as unset", async () => { await withInputBox( { diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index d574f93d..48b395f6 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -106,9 +106,11 @@ function controlSpawn() { const proc = new EventEmitter() as EventEmitter & { stdout: EventEmitter; stderr: EventEmitter; + stdin: { end: ReturnType }; }; proc.stdout = new EventEmitter(); proc.stderr = new EventEmitter(); + proc.stdin = { end: vi.fn() }; const { promise: spawned, resolve: resolveSpawned } = Promise.withResolvers(); vi.mocked(spawn).mockImplementation(() => { @@ -191,6 +193,23 @@ describe("updateWorkspace", () => { vi.clearAllMocks(); }); + it("runs coder update and resolves with the refreshed workspace", async () => { + const { ctx, restClient, finalWorkspace } = createUpdateCtx(); + const sp = controlSpawn(); + + const result = updateWorkspace(ctx); + await sp.close(0); + + await expect(result).resolves.toBe(finalWorkspace); + expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + "--url", + "https://test.coder.com", + "update", + "testuser/test-workspace", + ]); + expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id); + }); + it("rejects when the process exits non-zero", async () => { const { ctx, restClient } = createUpdateCtx(); const sp = controlSpawn(); @@ -264,10 +283,15 @@ describe("startWorkspace", () => { await sp.close(0); await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith( - '"/usr/bin/coder" --url "https://test.coder.com" start --yes --reason vscode_connection testuser/test-workspace', - expect.objectContaining({ shell: true }), - ); + expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + "--url", + "https://test.coder.com", + "start", + "--yes", + "--reason", + "vscode_connection", + "testuser/test-workspace", + ]); expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id); }); @@ -289,9 +313,12 @@ describe("startWorkspace", () => { await sp.close(0); await result; - expect(spawn).toHaveBeenCalledWith( - '"/usr/bin/coder" --url "https://test.coder.com" start --yes testuser/test-workspace', - expect.objectContaining({ shell: true }), - ); + expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + "--url", + "https://test.coder.com", + "start", + "--yes", + "testuser/test-workspace", + ]); }); }); diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 61030f8a..54dbbfcf 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -6,9 +6,9 @@ import { featureSetForVersion } from "@/featureSet"; import { type CliAuth, getGlobalFlags, - getGlobalFlagsRaw, getGlobalShellFlags, getSshFlags, + getUserGlobalFlags, isKeyringEnabled, resolveCliAuth, } from "@/settings/cli"; @@ -212,11 +212,11 @@ describe("cliConfig", () => { }); }); - describe("getGlobalFlagsRaw", () => { + describe("getUserGlobalFlags", () => { it("returns empty array when no global flags configured", () => { const config = new MockConfigurationProvider(); - expect(getGlobalFlagsRaw(config)).toStrictEqual([]); + expect(getUserGlobalFlags(config)).toStrictEqual([]); }); it("returns global flags from config", () => { @@ -226,11 +226,34 @@ describe("cliConfig", () => { "--disable-direct-connections", ]); - expect(getGlobalFlagsRaw(config)).toStrictEqual([ + expect(getUserGlobalFlags(config)).toStrictEqual([ "--verbose", "--disable-direct-connections", ]); }); + + it("substitutes ${env:VAR} from process.env", () => { + const restore = process.env.CODER_TEST_VAR; + process.env.CODER_TEST_VAR = "from-env"; + try { + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", [ + "--prefix=${env:CODER_TEST_VAR}", + "${env:CODER_MISSING_VAR}-suffix", + ]); + + expect(getUserGlobalFlags(config)).toStrictEqual([ + "--prefix=from-env", + "-suffix", + ]); + } finally { + if (restore === undefined) { + delete process.env.CODER_TEST_VAR; + } else { + process.env.CODER_TEST_VAR = restore; + } + } + }); }); describe("getSshFlags", () => { diff --git a/test/unit/remote/workspaceStateMachine.test.ts b/test/unit/remote/workspaceStateMachine.test.ts index 46aae11f..fb82c42b 100644 --- a/test/unit/remote/workspaceStateMachine.test.ts +++ b/test/unit/remote/workspaceStateMachine.test.ts @@ -1,5 +1,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; +import { WorkspaceUpdateCancelledError } from "@/api/updateParameters"; import { startWorkspace, updateWorkspace, @@ -195,9 +197,9 @@ describe("WorkspaceStateMachine", () => { expect(sm.getWorkspace()?.latest_build.status).toBe("running"); }); - it("falls back to start when the update fails", async () => { + it("falls back to start and warns the user when the update fails", async () => { vi.mocked(updateWorkspace).mockRejectedValueOnce( - new Error("Workspace update cancelled"), + new Error("template not found"), ); const { sm, progress } = setup("update"); const ws = createWorkspace({ latest_build: { status: "stopped" } }); @@ -205,6 +207,22 @@ describe("WorkspaceStateMachine", () => { expect(await sm.processWorkspace(ws, progress)).toBe(false); expect(updateWorkspace).toHaveBeenCalledOnce(); expect(startWorkspace).toHaveBeenCalledOnce(); + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith( + expect.stringMatching(/Workspace update failed:.*template not found/), + ); + }); + + it("falls back to start silently when the user cancels the update", async () => { + vi.mocked(updateWorkspace).mockRejectedValueOnce( + new WorkspaceUpdateCancelledError(), + ); + const { sm, progress } = setup("update"); + const ws = createWorkspace({ latest_build: { status: "stopped" } }); + + expect(await sm.processWorkspace(ws, progress)).toBe(false); + expect(updateWorkspace).toHaveBeenCalledOnce(); + expect(startWorkspace).toHaveBeenCalledOnce(); + expect(vscode.window.showWarningMessage).not.toHaveBeenCalled(); }); it("prompts user when mode is 'none' and user picks 'Start'", async () => { diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index df401bb9..745d7e97 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -240,15 +240,21 @@ describe("escapeShellArg", () => { expect(escapeShellArg("env=dev")).toBe('"env=dev"'); }); - it("escapes embedded double quotes", () => { + it('doubles embedded `"`', () => { expect(escapeShellArg('regions=["us","eu"]')).toBe( - String.raw`"regions=[\"us\",\"eu\"]"`, + '"regions=[""us"",""eu""]"', ); }); - it("doubles percent signs to block %VAR% expansion", () => { + it("doubles `%` to block %VAR% expansion", () => { expect(escapeShellArg("%PATH%")).toBe('"%%PATH%%"'); }); + + it("keeps cmd metachars inside the quoted region", () => { + expect(escapeShellArg('foo" & calc.exe & "x')).toBe( + '"foo"" & calc.exe & ""x"', + ); + }); }); }); diff --git a/test/utils/platform.test.ts b/test/utils/platform.test.ts index 5d37ddd0..35bbc866 100644 --- a/test/utils/platform.test.ts +++ b/test/utils/platform.test.ts @@ -3,15 +3,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; -import { - afterEach, - beforeAll, - beforeEach, - describe, - expect, - it, - vi, -} from "vitest"; +import { beforeAll, describe, expect, it } from "vitest"; import { expectPathsEqual, @@ -19,7 +11,6 @@ import { isWindows, printCommand, printEnvCommand, - shellQuote, shimExecFile, writeExecutable, writeStdoutJs, @@ -168,43 +159,4 @@ describe("platform utils", () => { expect(mod.spawn).toBe(cp.spawn); }); }); - - describe("shellQuote", () => { - const platformSpy = vi.spyOn(os, "platform"); - afterEach(() => platformSpy.mockReset()); - - describe("on Unix", () => { - beforeEach(() => platformSpy.mockReturnValue("linux")); - - it("wraps in single quotes", () => { - expect(shellQuote("env=dev")).toBe("'env=dev'"); - }); - - it("escapes single quotes via the '\\'' sequence", () => { - expect(shellQuote("it's fine")).toBe("'it'\\''s fine'"); - }); - - it("keeps $VAR, $(...), and backticks literal inside the quotes", () => { - expect(shellQuote("$(echo pwned)")).toBe("'$(echo pwned)'"); - }); - }); - - describe("on Windows", () => { - beforeEach(() => platformSpy.mockReturnValue("win32")); - - it("wraps in double quotes", () => { - expect(shellQuote("env=dev")).toBe('"env=dev"'); - }); - - it("escapes embedded double quotes", () => { - expect(shellQuote('regions=["us","eu"]')).toBe( - String.raw`"regions=[\"us\",\"eu\"]"`, - ); - }); - - it("doubles percent signs to block %VAR% expansion", () => { - expect(shellQuote("%PATH%")).toBe('"%%PATH%%"'); - }); - }); - }); }); diff --git a/test/utils/platform.ts b/test/utils/platform.ts index 01e7c1cf..5edb8587 100644 --- a/test/utils/platform.ts +++ b/test/utils/platform.ts @@ -124,17 +124,6 @@ export function quoteCommand(value: string): string { return `${quote}${value}${quote}`; } -/** - * Test-side mirror of `escapeShellArg` from src/util.ts. Wraps in single - * quotes on Unix and double quotes (with `"` and `%` escaped) on Windows. - */ -export function shellQuote(value: string): string { - if (isWindows()) { - return `"${value.replace(/"/g, '\\"').replace(/%/g, "%%")}"`; - } - return `'${value.replace(/'/g, "'\\''")}'`; -} - export function expectPathsEqual(actual: string, expected: string) { expect(normalizePath(actual)).toBe(normalizePath(expected)); } From fea14f0d28445cb79dd9880ba61db2c430400dcb Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 12 May 2026 17:33:43 +0300 Subject: [PATCH 6/7] fix: address R4 review feedback - Handle spawn 'error' events (ENOENT, EACCES) in runCliCommand so missing or unexecutable binaries reject the promise instead of hanging the progress dialog (DEREM-23). - Thread the shell-escape choice through to getHeaderArgs so the value is only quoted in shell:true contexts. Multi-word coder.headerCommand values now reach the CLI verbatim in execFile/no-shell paths (DEREM-24). - Report the terminating signal in the error message when the process is killed (DEREM-25). - Expose stdinEnd from the spawn test harness and assert it in the happy-path test so the DEREM-18 EOF fix is locked in (DEREM-26). Also added regression tests for spawn 'error' and signal-kill paths, and updated cliExec/cliConfig tests for the corrected no-shell header value. --- src/api/workspace.ts | 15 ++++++++++----- src/settings/cli.ts | 27 ++++++++++++--------------- src/settings/headers.ts | 7 +++---- test/unit/api/workspace.test.ts | 31 +++++++++++++++++++++++++++++-- test/unit/cliConfig.test.ts | 4 ++-- test/unit/core/cliExec.test.ts | 5 ++--- 6 files changed, 58 insertions(+), 31 deletions(-) diff --git a/src/api/workspace.ts b/src/api/workspace.ts index 449895b5..3ff82711 100644 --- a/src/api/workspace.ts +++ b/src/api/workspace.ts @@ -80,14 +80,19 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise { capturedStderr += text; }); - proc.on("close", (code: number) => { + // Settle on ENOENT/EACCES; later `close` rejects are then no-ops. + proc.on("error", reject); + + proc.on("close", (code: number | null, signal: NodeJS.Signals | null) => { if (code === 0) { resolve(); - } else { - let msg = `"${fullArgs.join(" ")}" exited with code ${code}`; - if (capturedStderr) msg += `: ${capturedStderr}`; - reject(new Error(msg)); + return; } + const exit = + code !== null ? `code ${code}` : `signal ${signal ?? "unknown"}`; + let msg = `"${fullArgs.join(" ")}" exited with ${exit}`; + if (capturedStderr) msg += `: ${capturedStderr}`; + reject(new Error(msg)); }); }); } diff --git a/src/settings/cli.ts b/src/settings/cli.ts index d33a37de..d240402e 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -1,5 +1,5 @@ import { isKeyringSupported } from "../core/cliCredentialManager"; -import { escapeCommandArg } from "../util"; +import { escapeCommandArg, escapeShellArg } from "../util"; import { getHeaderArgs } from "./headers"; @@ -29,41 +29,38 @@ export function getUserGlobalFlags( ); } -/** - * Returns global configuration flags for Coder CLI commands with auth values - * escaped for shell use (e.g., `terminal.sendText`, `spawn({ shell: true })`). - */ +/** Flags for shell contexts (`terminal.sendText`, `spawn({ shell: true })`). */ export function getGlobalShellFlags( configs: Pick, auth: CliAuth, ): string[] { - return buildGlobalFlags(configs, auth, escapeCommandArg); + return buildGlobalFlags(configs, auth, escapeCommandArg, escapeShellArg); } -/** - * Returns global configuration flags for Coder CLI commands with raw auth - * values suitable for `execFile` (no shell escaping). - */ +/** Raw flags for `execFile` or `spawn` without a shell. */ export function getGlobalFlags( configs: Pick, auth: CliAuth, ): string[] { - return buildGlobalFlags(configs, auth, (s) => s); + return buildGlobalFlags(configs, auth, identity, identity); } +const identity = (s: string) => s; + function buildGlobalFlags( configs: Pick, auth: CliAuth, - esc: (s: string) => string, + escAuth: (s: string) => string, + escHeader: (s: string) => string, ): string[] { const authFlags = auth.mode === "url" - ? ["--url", esc(auth.url)] - : ["--global-config", esc(auth.configDir)]; + ? ["--url", escAuth(auth.url)] + : ["--global-config", escAuth(auth.configDir)]; const filtered = stripManagedFlags(getUserGlobalFlags(configs)); - return [...filtered, ...authFlags, ...getHeaderArgs(configs)]; + return [...filtered, ...authFlags, ...getHeaderArgs(configs, escHeader)]; } function stripManagedFlags(flags: string[]): string[] { diff --git a/src/settings/headers.ts b/src/settings/headers.ts index 2381b27e..7abbdd10 100644 --- a/src/settings/headers.ts +++ b/src/settings/headers.ts @@ -1,5 +1,3 @@ -import { escapeShellArg } from "../util"; - import type { WorkspaceConfiguration } from "vscode"; /** Returns the header command from settings or the CODER_HEADER_COMMAND env var. */ @@ -13,13 +11,14 @@ export function getHeaderCommand( return cmd || undefined; } -/** Returns `--header-command` CLI args, escaped for the current platform. */ +/** Returns `--header-command` args; shell callers pass `escapeShellArg` as `esc`. */ export function getHeaderArgs( config: Pick, + esc: (s: string) => string = (s) => s, ): string[] { const command = getHeaderCommand(config); if (!command) { return []; } - return ["--header-command", escapeShellArg(command)]; + return ["--header-command", esc(command)]; } diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 48b395f6..2a1efb96 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -119,12 +119,17 @@ function controlSpawn() { }); return { spawned, + stdinEnd: proc.stdin.end, stderr(data: string) { proc.stderr.emit("data", Buffer.from(data)); }, - async close(exitCode: number) { + async close(exitCode: number | null, signal?: NodeJS.Signals) { await spawned; - proc.emit("close", exitCode); + proc.emit("close", exitCode, signal ?? null); + }, + async error(err: Error) { + await spawned; + proc.emit("error", err); }, }; } @@ -207,6 +212,7 @@ describe("updateWorkspace", () => { "update", "testuser/test-workspace", ]); + expect(sp.stdinEnd).toHaveBeenCalled(); expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id); }); @@ -223,6 +229,27 @@ describe("updateWorkspace", () => { expect(restClient.getWorkspace).not.toHaveBeenCalled(); }); + it("rejects when spawn emits an error (e.g. missing binary)", async () => { + const { ctx, restClient } = createUpdateCtx(); + const sp = controlSpawn(); + + const result = updateWorkspace(ctx); + await sp.error(new Error("spawn /usr/bin/coder ENOENT")); + + await expect(result).rejects.toThrow(/ENOENT/); + expect(restClient.getWorkspace).not.toHaveBeenCalled(); + }); + + it("reports the terminating signal when the process is killed", async () => { + const { ctx } = createUpdateCtx(); + const sp = controlSpawn(); + + const result = updateWorkspace(ctx); + await sp.close(null, "SIGTERM"); + + await expect(result).rejects.toThrow(/signal SIGTERM/); + }); + it("falls back to the API update path when coder update is unsupported", async () => { const { ctx, restClient, finalWorkspace } = createUpdateCtx({ featureSet: { cliUpdate: false }, diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 54dbbfcf..3f65d98d 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -190,14 +190,14 @@ describe("cliConfig", () => { ]); }); - it("should still escape header-command flags", () => { + it("passes header-command value through verbatim (no shell)", () => { const config = new MockConfigurationProvider(); config.set("coder.headerCommand", "echo test"); expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ "--global-config", "/config/dir", "--header-command", - quoteCommand("echo test"), + "echo test", ]); }); diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index 7d1fcbd8..bec8767a 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -6,7 +6,6 @@ import { beforeAll, describe, expect, it, vi } from "vitest"; import { MockConfigurationProvider } from "../../mocks/testHelpers"; import { isWindows, - quoteCommand, writeExecutable, writeStderrJs, writeStdoutJs, @@ -154,7 +153,7 @@ describe("cliExec", () => { "--url", "http://localhost:3000", "--header-command", - quoteCommand("my-header-cmd"), + "my-header-cmd", "speedtest", "owner/workspace", "--output", @@ -211,7 +210,7 @@ describe("cliExec", () => { "--url", "http://localhost:3000", "--header-command", - quoteCommand("my-header-cmd"), + "my-header-cmd", "support", "bundle", "owner/workspace", From f2fbba5724e3f89513c3cb0875a926469dc772b4 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 12 May 2026 23:04:33 +0300 Subject: [PATCH 7/7] revert: keep shell:true in runCliCommand on this branch Splits the shell-handling change out of this PR so it can land (and be reverted) on its own. Reinstates: - `runCliCommand` spawns through `shell: true` again, with the workspace identifier wrapped in `escapeShellArg` (so server-controlled values still flow through the cmd.exe-safe `""`/`%%` escape introduced in R2). - `coder update` parameter values go through `escapeShellArg` in updateParameters.ts before being passed as `--parameter name=value`. - `getUserGlobalFlags` returns `coder.globalFlags` verbatim again; the `${env:VAR}` substitution was tied to the no-shell path and moves to the follow-up PR. - package.json `coder.globalFlags` description drops the env-expansion note for now. Kept (orthogonal hardening, no shell dependence): - `proc.stdin.end()` to EOF unexpected CLI prompts (DEREM-18). - State-machine running-case guard (DEREM-19). - Server-side regex only, no client-side regex eval (DEREM-22). - `getHeaderArgs(esc)` still threads the escape choice so execFile callers (speedtest/supportBundle) don't get quoted header values (DEREM-24). - Signal-aware close handler reporting `signal SIGTERM` etc. (DEREM-25). - `stdinEnd` assertion in the happy-path test (DEREM-26). Dropped the `proc.on("error", reject)` handler since with `shell: true` a missing binary surfaces as a non-zero close from the shell, not a spawn error event; the corresponding ENOENT test is removed. Restores the test-side `shellQuote` mirror of `escapeShellArg` in test/utils/platform.ts (and its tests) for asserting shell-escaped values cross-platform. --- package.json | 2 +- src/api/updateParameters.ts | 4 +- src/api/workspace.ts | 13 +++---- src/settings/cli.ts | 15 +------- test/unit/api/updateParameters.test.ts | 17 ++++++--- test/unit/api/workspace.test.ts | 51 +++++++------------------- test/unit/cliConfig.test.ts | 23 ------------ test/utils/platform.test.ts | 50 ++++++++++++++++++++++++- test/utils/platform.ts | 12 ++++++ 9 files changed, 98 insertions(+), 89 deletions(-) diff --git a/package.json b/package.json index 83887c87..1e652166 100644 --- a/package.json +++ b/package.json @@ -160,7 +160,7 @@ ] }, "coder.globalFlags": { - "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nValues are passed directly to the CLI without a shell, so `$VAR` and `%VAR%` are **not** expanded. Use `${env:VAR}` to reference environment variables (e.g. `--cfg=${env:HOME}/cfg`); missing variables resolve to an empty string.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.", + "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.", "type": "array", "items": { "type": "string" diff --git a/src/api/updateParameters.ts b/src/api/updateParameters.ts index f6cde17f..34dc39b1 100644 --- a/src/api/updateParameters.ts +++ b/src/api/updateParameters.ts @@ -1,5 +1,7 @@ import * as vscode from "vscode"; +import { escapeShellArg } from "../util"; + import type { Api } from "coder/site/src/api/api"; import type { TemplateVersionParameter, @@ -42,7 +44,7 @@ export async function collectUpdateParameters( if (value === undefined) { throw new WorkspaceUpdateCancelledError(); } - args.push("--parameter", `${param.name}=${value}`); + args.push("--parameter", escapeShellArg(`${param.name}=${value}`)); } return args; } diff --git a/src/api/workspace.ts b/src/api/workspace.ts index 3ff82711..a0e24dd6 100644 --- a/src/api/workspace.ts +++ b/src/api/workspace.ts @@ -1,7 +1,8 @@ import { spawn } from "node:child_process"; import * as vscode from "vscode"; -import { getGlobalFlags, type CliAuth } from "../settings/cli"; +import { getGlobalShellFlags, type CliAuth } from "../settings/cli"; +import { escapeCommandArg, escapeShellArg } from "../util"; import { errToStr, createWorkspaceIdentifier } from "./api-helper"; import { collectUpdateParameters } from "./updateParameters"; @@ -61,11 +62,12 @@ interface CliContext { function runCliCommand(ctx: CliContext, args: string[]): Promise { return new Promise((resolve, reject) => { const fullArgs = [ - ...getGlobalFlags(vscode.workspace.getConfiguration(), ctx.auth), + ...getGlobalShellFlags(vscode.workspace.getConfiguration(), ctx.auth), ...args, - createWorkspaceIdentifier(ctx.workspace), + escapeShellArg(createWorkspaceIdentifier(ctx.workspace)), ]; - const proc = spawn(ctx.binPath, fullArgs); + const cmd = `${escapeCommandArg(ctx.binPath)} ${fullArgs.join(" ")}`; + const proc = spawn(cmd, { shell: true }); // Unexpected prompts EOF instead of hanging forever. proc.stdin.end(); @@ -80,9 +82,6 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise { capturedStderr += text; }); - // Settle on ENOENT/EACCES; later `close` rejects are then no-ops. - proc.on("error", reject); - proc.on("close", (code: number | null, signal: NodeJS.Signals | null) => { if (code === 0) { resolve(); diff --git a/src/settings/cli.ts b/src/settings/cli.ts index d240402e..2fe70826 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -11,22 +11,11 @@ export type CliAuth = | { mode: "global-config"; configDir: string } | { mode: "url"; url: string }; -/** - * Returns the user's `coder.globalFlags` with `${env:VAR}` references - * substituted from `process.env`. Missing variables resolve to an empty - * string, matching VS Code's behaviour for built-in `${env:VAR}` sites. - */ +/** Returns the user's `coder.globalFlags` as configured, with no expansion. */ export function getUserGlobalFlags( configs: Pick, ): string[] { - return configs - .get("coder.globalFlags", []) - .map((flag) => - flag.replace( - /\$\{env:([A-Za-z_][A-Za-z0-9_]*)\}/g, - (_, name: string) => process.env[name] ?? "", - ), - ); + return configs.get("coder.globalFlags", []); } /** Flags for shell contexts (`terminal.sendText`, `spawn({ shell: true })`). */ diff --git a/test/unit/api/updateParameters.test.ts b/test/unit/api/updateParameters.test.ts index f8d83f23..2b67b2fa 100644 --- a/test/unit/api/updateParameters.test.ts +++ b/test/unit/api/updateParameters.test.ts @@ -8,6 +8,8 @@ import { import { workspace as createWorkspace } from "@repo/mocks"; +import { shellQuote } from "../../utils/platform"; + import type { Api } from "coder/site/src/api/api"; import type { TemplateVersionParameter } from "coder/site/src/api/typesGenerated"; @@ -143,14 +145,14 @@ describe("collectUpdateParameters", () => { param: { name: "environment" }, mock: mockCreateInputBox, accept: { value: "dev" }, - expected: ["--parameter", "environment=dev"], + expected: ["--parameter", shellQuote("environment=dev")], }, { kind: "bool quick pick", param: { name: "enabled", type: "bool" }, mock: mockCreateQuickPick, accept: { selectedItems: [{ value: "true" }] }, - expected: ["--parameter", "enabled=true"], + expected: ["--parameter", shellQuote("enabled=true")], }, { kind: "options quick pick", @@ -163,7 +165,7 @@ describe("collectUpdateParameters", () => { }, mock: mockCreateQuickPick, accept: { selectedItems: [{ value: "l" }] }, - expected: ["--parameter", "size=l"], + expected: ["--parameter", shellQuote("size=l")], }, { kind: "multi-select quick pick (JSON array)", @@ -177,7 +179,7 @@ describe("collectUpdateParameters", () => { }, mock: mockCreateQuickPick, accept: { selectedItems: [{ value: "us" }, { value: "eu" }] }, - expected: ["--parameter", 'regions=["us","eu"]'], + expected: ["--parameter", shellQuote('regions=["us","eu"]')], }, ])( "collects the value via $kind", @@ -193,7 +195,7 @@ describe("collectUpdateParameters", () => { }, ); - it("passes server-controlled values through verbatim (no shell expansion path)", async () => { + it("escapes shell metacharacters in server-controlled values", async () => { const { restClient, workspace } = createCollectCtx([{ name: "evil" }]); const qi = mockCreateInputBox(); @@ -201,7 +203,10 @@ describe("collectUpdateParameters", () => { await waitShown(qi); qi.accept({ value: "$(rm -rf /)" }); - await expect(result).resolves.toEqual(["--parameter", "evil=$(rm -rf /)"]); + await expect(result).resolves.toEqual([ + "--parameter", + shellQuote("evil=$(rm -rf /)"), + ]); }); it("skips parameters that already have a value or default", async () => { diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 2a1efb96..4bc4eff0 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -6,6 +6,8 @@ import { LazyStream, startWorkspace, updateWorkspace } from "@/api/workspace"; import { workspace as createWorkspace } from "@repo/mocks"; +import { shellQuote } from "../../utils/platform"; + import type { Api } from "coder/site/src/api/api"; import type { Workspace, @@ -127,10 +129,6 @@ function controlSpawn() { await spawned; proc.emit("close", exitCode, signal ?? null); }, - async error(err: Error) { - await spawned; - proc.emit("error", err); - }, }; } @@ -206,12 +204,10 @@ describe("updateWorkspace", () => { await sp.close(0); await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", - "update", - "testuser/test-workspace", - ]); + expect(spawn).toHaveBeenCalledWith( + `"/usr/bin/coder" --url "https://test.coder.com" update ${shellQuote("testuser/test-workspace")}`, + { shell: true }, + ); expect(sp.stdinEnd).toHaveBeenCalled(); expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id); }); @@ -229,17 +225,6 @@ describe("updateWorkspace", () => { expect(restClient.getWorkspace).not.toHaveBeenCalled(); }); - it("rejects when spawn emits an error (e.g. missing binary)", async () => { - const { ctx, restClient } = createUpdateCtx(); - const sp = controlSpawn(); - - const result = updateWorkspace(ctx); - await sp.error(new Error("spawn /usr/bin/coder ENOENT")); - - await expect(result).rejects.toThrow(/ENOENT/); - expect(restClient.getWorkspace).not.toHaveBeenCalled(); - }); - it("reports the terminating signal when the process is killed", async () => { const { ctx } = createUpdateCtx(); const sp = controlSpawn(); @@ -310,15 +295,10 @@ describe("startWorkspace", () => { await sp.close(0); await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", - "start", - "--yes", - "--reason", - "vscode_connection", - "testuser/test-workspace", - ]); + expect(spawn).toHaveBeenCalledWith( + `"/usr/bin/coder" --url "https://test.coder.com" start --yes --reason vscode_connection ${shellQuote("testuser/test-workspace")}`, + { shell: true }, + ); expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id); }); @@ -340,12 +320,9 @@ describe("startWorkspace", () => { await sp.close(0); await result; - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", - "start", - "--yes", - "testuser/test-workspace", - ]); + expect(spawn).toHaveBeenCalledWith( + `"/usr/bin/coder" --url "https://test.coder.com" start --yes ${shellQuote("testuser/test-workspace")}`, + { shell: true }, + ); }); }); diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 3f65d98d..9481827c 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -231,29 +231,6 @@ describe("cliConfig", () => { "--disable-direct-connections", ]); }); - - it("substitutes ${env:VAR} from process.env", () => { - const restore = process.env.CODER_TEST_VAR; - process.env.CODER_TEST_VAR = "from-env"; - try { - const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", [ - "--prefix=${env:CODER_TEST_VAR}", - "${env:CODER_MISSING_VAR}-suffix", - ]); - - expect(getUserGlobalFlags(config)).toStrictEqual([ - "--prefix=from-env", - "-suffix", - ]); - } finally { - if (restore === undefined) { - delete process.env.CODER_TEST_VAR; - } else { - process.env.CODER_TEST_VAR = restore; - } - } - }); }); describe("getSshFlags", () => { diff --git a/test/utils/platform.test.ts b/test/utils/platform.test.ts index 35bbc866..9d9d5307 100644 --- a/test/utils/platform.test.ts +++ b/test/utils/platform.test.ts @@ -3,7 +3,15 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; -import { beforeAll, describe, expect, it } from "vitest"; +import { + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; import { expectPathsEqual, @@ -11,6 +19,7 @@ import { isWindows, printCommand, printEnvCommand, + shellQuote, shimExecFile, writeExecutable, writeStdoutJs, @@ -159,4 +168,43 @@ describe("platform utils", () => { expect(mod.spawn).toBe(cp.spawn); }); }); + + describe("shellQuote", () => { + const platformSpy = vi.spyOn(os, "platform"); + afterEach(() => platformSpy.mockReset()); + + describe("on Unix", () => { + beforeEach(() => platformSpy.mockReturnValue("linux")); + + it("wraps in single quotes", () => { + expect(shellQuote("env=dev")).toBe("'env=dev'"); + }); + + it("escapes single quotes via the '\\'' sequence", () => { + expect(shellQuote("it's fine")).toBe("'it'\\''s fine'"); + }); + + it("keeps $VAR, $(...), and backticks literal inside the quotes", () => { + expect(shellQuote("$(echo pwned)")).toBe("'$(echo pwned)'"); + }); + }); + + describe("on Windows", () => { + beforeEach(() => platformSpy.mockReturnValue("win32")); + + it("wraps in double quotes", () => { + expect(shellQuote("env=dev")).toBe('"env=dev"'); + }); + + it("doubles embedded double quotes", () => { + expect(shellQuote('regions=["us","eu"]')).toBe( + '"regions=[""us"",""eu""]"', + ); + }); + + it("doubles percent signs to block %VAR% expansion", () => { + expect(shellQuote("%PATH%")).toBe('"%%PATH%%"'); + }); + }); + }); }); diff --git a/test/utils/platform.ts b/test/utils/platform.ts index 5edb8587..d4827335 100644 --- a/test/utils/platform.ts +++ b/test/utils/platform.ts @@ -124,6 +124,18 @@ export function quoteCommand(value: string): string { return `${quote}${value}${quote}`; } +/** + * Test-side mirror of `escapeShellArg` from src/util.ts. Wraps in single + * quotes on Unix and double quotes (with `"` doubled and `%` doubled) on + * Windows. + */ +export function shellQuote(value: string): string { + if (isWindows()) { + return `"${value.replace(/"/g, '""').replace(/%/g, "%%")}"`; + } + return `'${value.replace(/'/g, "'\\''")}'`; +} + export function expectPathsEqual(actual: string, expected: string) { expect(normalizePath(actual)).toBe(normalizePath(expected)); }