From 60529a87517fa418fbb9701d2ae946d65587e0e4 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 20 May 2026 12:23:15 +0300 Subject: [PATCH 1/2] fix(workspace-update): prompt for option drift and immutable params Aligns parameter collection with the dashboard's getMissingParameters in coder/site/src/api/api.ts. Previously the filter only checked `required && !default_value` against `name in storedValues`, so: - Required params whose stored option value was no longer in the new version's option list (option-value drift) were carried forward and the build was rejected server-side with no useful UI. - Immutable params without a stored value were never prompted, even though they can never be set after the first build. Add `needsPrompt(param, storedValue)` covering both cases. Beyond the dashboard, also handle multi-select drift by parsing the JSON-encoded value (dashboard explicitly skips multi-select). The `default_value` short-circuit is preserved for the unset case so the server can fall back to the template default during auto-update without a prompt. --- CHANGELOG.md | 5 + src/api/updateParameters.ts | 49 +++++++-- test/unit/api/updateParameters.test.ts | 139 +++++++++++++++++++++++++ 3 files changed, 185 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3087b55b9..a00167f12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,11 @@ `coder update` non-interactively) now passes newly-required template parameters into the REST-API fallback build, instead of silently omitting them and letting the server reject the build. +- Updating a workspace now re-prompts for an option or multi-select + parameter whose stored value is no longer one of the new template + version's options, instead of carrying a stale value forward and + failing the build. Immutable parameters without a stored value are + also surfaced, matching the dashboard's behaviour. - Updating a workspace from VS Code no longer hangs when the new template version requires parameters. The extension now prompts for any missing required values through VS Code input boxes and passes them to diff --git a/src/api/updateParameters.ts b/src/api/updateParameters.ts index 8e24ba736..3d00b8aa0 100644 --- a/src/api/updateParameters.ts +++ b/src/api/updateParameters.ts @@ -16,9 +16,9 @@ export class WorkspaceUpdateCancelledError extends Error { } /** - * Prompts the user for any newly-required template parameters and returns the - * collected `{ name, value }` pairs. Throws `WorkspaceUpdateCancelledError` if - * the user dismisses a prompt. + * Prompts the user for any template parameters that the new version needs + * answered, and returns the collected `{ name, value }` pairs. Throws + * `WorkspaceUpdateCancelledError` if the user dismisses a prompt. */ export async function collectUpdateParameters( restClient: Api, @@ -30,11 +30,8 @@ export async function collectUpdateParameters( ), restClient.getWorkspaceBuildParameters(workspace.latest_build.id), ]); - const candidates = newParams.filter((p) => p.required && !p.default_value); - if (candidates.length === 0) return []; - - const existing = new Set(currentValues.map((p) => p.name)); - const toPrompt = candidates.filter((p) => !existing.has(p.name)); + const stored = new Map(currentValues.map((p) => [p.name, p.value])); + const toPrompt = newParams.filter((p) => needsPrompt(p, stored.get(p.name))); const collected: WorkspaceBuildParameter[] = []; for (let i = 0; i < toPrompt.length; i++) { @@ -48,6 +45,42 @@ export async function collectUpdateParameters( return collected; } +/** Mirrors the dashboard's `getMissingParameters` (coder/site/src/api/api.ts). */ +function needsPrompt( + param: TemplateVersionParameter, + storedValue: string | undefined, +): boolean { + if (storedValue === undefined) { + const mustBeSet = (param.mutable && param.required) || !param.mutable; + // Deviation: skip when a template default exists; server falls back to it. + return mustBeSet && !param.default_value; + } + if (param.options.length === 0) return false; + + const validValues = new Set(param.options.map((o) => o.value)); + if (param.form_type === "multi-select") { + const picks = parseMultiSelectValue(storedValue); + return picks === null || picks.some((v) => !validValues.has(v)); + } + return !validValues.has(storedValue); +} + +/** Multi-select values are stored as a JSON-encoded string array. */ +function parseMultiSelectValue(raw: string): string[] | null { + try { + const parsed: unknown = JSON.parse(raw); + if ( + Array.isArray(parsed) && + parsed.every((v): v is string => typeof v === "string") + ) { + return parsed; + } + } catch { + // invalid JSON + } + return null; +} + function promptForParameter( param: TemplateVersionParameter, step: number, diff --git a/test/unit/api/updateParameters.test.ts b/test/unit/api/updateParameters.test.ts index c00c5a33c..9c8701ab9 100644 --- a/test/unit/api/updateParameters.test.ts +++ b/test/unit/api/updateParameters.test.ts @@ -222,6 +222,145 @@ describe("collectUpdateParameters", () => { expect(vscode.window.createInputBox).not.toHaveBeenCalled(); }); + it("prompts an immutable param that has no stored value (even if not required)", async () => { + const { restClient, workspace } = createCollectCtx([ + { name: "zone", required: false, mutable: false }, + ]); + const qi = mockCreateInputBox(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ value: "us-east" }); + + await expect(result).resolves.toEqual([{ name: "zone", value: "us-east" }]); + }); + + it("skips an option param whose stored value is still in the new options", async () => { + const optionParam: Partial = { + name: "size", + options: [ + { name: "Small", description: "", value: "s", icon: "" }, + { name: "Large", description: "", value: "l", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [optionParam], + [{ name: "size", value: "l" }], + ); + + await expect( + collectUpdateParameters(restClient, workspace), + ).resolves.toEqual([]); + expect(vscode.window.createQuickPick).not.toHaveBeenCalled(); + }); + + it("re-prompts when a stored option value drifted out of the new options", async () => { + const optionParam: Partial = { + name: "size", + options: [ + { name: "Small", description: "", value: "s", icon: "" }, + { name: "Large", description: "", value: "l", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [optionParam], + [{ name: "size", value: "xl-retired" }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "l" }] }); + + await expect(result).resolves.toEqual([{ name: "size", value: "l" }]); + }); + + it("re-prompts on drift even when the template parameter has a default value", async () => { + const optionParam: Partial = { + name: "size", + default_value: "s", + options: [ + { name: "Small", description: "", value: "s", icon: "" }, + { name: "Large", description: "", value: "l", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [optionParam], + [{ name: "size", value: "xl-retired" }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "l" }] }); + + await expect(result).resolves.toEqual([{ name: "size", value: "l" }]); + }); + + it("re-prompts on drift for an immutable param too", async () => { + const optionParam: Partial = { + name: "zone", + mutable: false, + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [optionParam], + [{ name: "zone", value: "eu-retired" }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "us" }] }); + + await expect(result).resolves.toEqual([{ name: "zone", value: "us" }]); + }); + + it("re-prompts a multi-select when any stored pick drifted out", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [ + { name: "US", description: "", value: "us", icon: "" }, + { name: "EU", description: "", value: "eu", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["us","apac-retired"]' }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "us" }] }); + + await expect(result).resolves.toEqual([ + { name: "regions", value: '["us"]' }, + ]); + }); + + it("re-prompts a multi-select when the stored value isn't a JSON string array", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: "not-json" }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "us" }] }); + + await expect(result).resolves.toEqual([ + { name: "regions", value: '["us"]' }, + ]); + }); + it("throws WorkspaceUpdateCancelledError when the prompt is dismissed", async () => { const { restClient, workspace } = createCollectCtx([{}]); const qi = mockCreateInputBox(); From bd42d05b336dfb75b2d9b58e41838da9b7a26e93 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 20 May 2026 15:25:48 +0300 Subject: [PATCH 2/2] review: address coder-agents-review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - needsPrompt: prompt immutable + default (no user override possible later — permanent lock-in to server default otherwise). Per DEREM-3. - promptForParameter: accept empty selection on non-required multi-select re-prompts; the previous logic kept the QuickPick open with no way to express "none." Per DEREM-5. - promptForParameter: pre-select still-valid picks on multi-select drift re-prompts so the user doesn't lose what was already correct. Per DEREM-7. - Inline notes on both deviations from the dashboard's getMissingParameters (default short-circuit + multi-select drift) so future syncs catch them. - Drop manual `(v): v is string =>` predicate (TS 5.5+ inference). - Tests: multi-select happy path, '[]' empty array, non-required empty submission, preselect verification, immutable + default prompt. - CHANGELOG: user-facing wording ("closing a gap with the web dashboard"). --- CHANGELOG.md | 2 +- src/api/updateParameters.ts | 46 ++++++---- test/unit/api/updateParameters.test.ts | 121 +++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a00167f12..c631dfb1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,7 +57,7 @@ parameter whose stored value is no longer one of the new template version's options, instead of carrying a stale value forward and failing the build. Immutable parameters without a stored value are - also surfaced, matching the dashboard's behaviour. + now prompted as well, closing a gap with the web dashboard. - Updating a workspace from VS Code no longer hangs when the new template version requires parameters. The extension now prompts for any missing required values through VS Code input boxes and passes them to diff --git a/src/api/updateParameters.ts b/src/api/updateParameters.ts index 3d00b8aa0..eabfe0593 100644 --- a/src/api/updateParameters.ts +++ b/src/api/updateParameters.ts @@ -36,7 +36,12 @@ export async function collectUpdateParameters( const collected: WorkspaceBuildParameter[] = []; for (let i = 0; i < toPrompt.length; i++) { const param = toPrompt[i]; - const value = await promptForParameter(param, i + 1, toPrompt.length); + const value = await promptForParameter( + param, + i + 1, + toPrompt.length, + stored.get(param.name), + ); if (value === undefined) { throw new WorkspaceUpdateCancelledError(); } @@ -45,20 +50,24 @@ export async function collectUpdateParameters( return collected; } -/** Mirrors the dashboard's `getMissingParameters` (coder/site/src/api/api.ts). */ +/** Based on the dashboard's `getMissingParameters` (coder/site/src/api/api.ts). */ function needsPrompt( param: TemplateVersionParameter, storedValue: string | undefined, ): boolean { if (storedValue === undefined) { - const mustBeSet = (param.mutable && param.required) || !param.mutable; - // Deviation: skip when a template default exists; server falls back to it. - return mustBeSet && !param.default_value; + // Accepting the default locks it in. + if (!param.mutable) return true; + // Deviation: dashboard prompts mutable+required even when a default + // exists; we skip so the server can apply the default during + // auto-update without surfacing a modal. + return param.required && !param.default_value; } if (param.options.length === 0) return false; const validValues = new Set(param.options.map((o) => o.value)); if (param.form_type === "multi-select") { + // Beyond dashboard: detect multi-select drift too. const picks = parseMultiSelectValue(storedValue); return picks === null || picks.some((v) => !validValues.has(v)); } @@ -67,24 +76,22 @@ function needsPrompt( /** Multi-select values are stored as a JSON-encoded string array. */ function parseMultiSelectValue(raw: string): string[] | null { + let parsed: unknown; try { - const parsed: unknown = JSON.parse(raw); - if ( - Array.isArray(parsed) && - parsed.every((v): v is string => typeof v === "string") - ) { - return parsed; - } + parsed = JSON.parse(raw); } catch { - // invalid JSON + return null; } - return null; + return Array.isArray(parsed) && parsed.every((v) => typeof v === "string") + ? parsed + : null; } function promptForParameter( param: TemplateVersionParameter, step: number, totalSteps: number, + storedValue: string | undefined, ): Promise { const title = param.display_name || param.name; const items = quickPickItems(param); @@ -99,11 +106,16 @@ function promptForParameter( qp.items = items; qp.canSelectMany = multi; qp.ignoreFocusOut = true; + if (multi && storedValue !== undefined) { + const previous = new Set(parseMultiSelectValue(storedValue) ?? []); + qp.selectedItems = items.filter((item) => previous.has(item.value)); + } return collectInput(qp, () => { if (multi) { - return qp.selectedItems.length > 0 - ? JSON.stringify(qp.selectedItems.map((i) => i.value)) - : undefined; + if (qp.selectedItems.length === 0) { + return param.required ? undefined : "[]"; + } + return JSON.stringify(qp.selectedItems.map((i) => i.value)); } return qp.selectedItems[0]?.value; }); diff --git a/test/unit/api/updateParameters.test.ts b/test/unit/api/updateParameters.test.ts index 9c8701ab9..a11b697d2 100644 --- a/test/unit/api/updateParameters.test.ts +++ b/test/unit/api/updateParameters.test.ts @@ -235,6 +235,19 @@ describe("collectUpdateParameters", () => { await expect(result).resolves.toEqual([{ name: "zone", value: "us-east" }]); }); + it("prompts an immutable param even when a template default exists", async () => { + const { restClient, workspace } = createCollectCtx([ + { name: "zone", mutable: false, default_value: "us-east" }, + ]); + const qi = mockCreateInputBox(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ value: "us-west" }); + + await expect(result).resolves.toEqual([{ name: "zone", value: "us-west" }]); + }); + it("skips an option param whose stored value is still in the new options", async () => { const optionParam: Partial = { name: "size", @@ -361,6 +374,114 @@ describe("collectUpdateParameters", () => { ]); }); + it("skips a multi-select when every stored pick is still in the new options", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [ + { name: "US", description: "", value: "us", icon: "" }, + { name: "EU", description: "", value: "eu", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["us","eu"]' }], + ); + + await expect( + collectUpdateParameters(restClient, workspace), + ).resolves.toEqual([]); + expect(vscode.window.createQuickPick).not.toHaveBeenCalled(); + }); + + it("skips a multi-select when the stored value is an empty JSON array", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: "[]" }], + ); + + await expect( + collectUpdateParameters(restClient, workspace), + ).resolves.toEqual([]); + expect(vscode.window.createQuickPick).not.toHaveBeenCalled(); + }); + + it("pre-selects still-valid picks on a multi-select drift re-prompt", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [ + { name: "US", description: "", value: "us", icon: "" }, + { name: "EU", description: "", value: "eu", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["us","apac-retired"]' }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + expect(qi.mock.selectedItems).toEqual([ + { label: "US", description: "", value: "us" }, + ]); + qi.accept({ selectedItems: [{ value: "us" }, { value: "eu" }] }); + + await expect(result).resolves.toEqual([ + { name: "regions", value: '["us","eu"]' }, + ]); + }); + + it("keeps the prompt open on an empty submission for a required multi-select", async () => { + const multiParam: Partial = { + name: "regions", + required: true, + form_type: "multi-select", + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["apac-retired"]' }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [] }); + expect(qi.mock.dispose).not.toHaveBeenCalled(); + qi.accept({ selectedItems: [{ value: "us" }] }); + + await expect(result).resolves.toEqual([ + { name: "regions", value: '["us"]' }, + ]); + }); + + it("accepts an empty selection on a non-required multi-select re-prompt", async () => { + const multiParam: Partial = { + name: "regions", + required: false, + form_type: "multi-select", + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["apac-retired"]' }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [] }); + + await expect(result).resolves.toEqual([{ name: "regions", value: "[]" }]); + }); + it("throws WorkspaceUpdateCancelledError when the prompt is dismissed", async () => { const { restClient, workspace } = createCollectCtx([{}]); const qi = mockCreateInputBox();