fix: prompt for update parameters before CLI update

[EhabY]

Summary

• Collects newly required workspace update parameters with VS Code InputBox/QuickPick prompts before running coder update.
• Runs the CLI update without invoking a shell — args go straight to spawn(binary, args, {}), so values reach coder exactly as typed and there's no quote-escaping surface on Windows. The REST API fallback is preserved for older CLIs.
• Carries the fresh workspace through the connection state machine, falls back to starting the existing workspace if the update fails or the user cancels, and hides the update status bar while the workspace is still building.

Fixes #939.

Notes for reviewers

Parameter prompting + validation lives in a new src/api/updateParameters.ts module so src/api/workspace.ts stays focused on CLI orchestration. User cancellation throws a dedicated WorkspaceUpdateCancelledError so the state machine can skip the failure warning toast and log it as info instead.

Hardening picked up during review:

• runCliCommand no longer spawns through a shell, removing the Windows cmd.exe injection surface and the need for cross-platform escape rules.
• proc.stdin.end() is called immediately after spawn so any future CLI prompt the extension didn't pre-collect EOFs and errors out instead of hanging.
• Client-side regex testing is dropped (server validates with RE2; JS's backtracking engine would freeze the extension host on hostile patterns). Number-range and required-field checks still run client-side for inline feedback.
• ${env:VAR} references in coder.globalFlags are now substituted (replacement for the lost shell expansion); documented in package.json.

Tests

• New unit suite at test/unit/api/updateParameters.test.ts covering parameter collection, prompt validation, cancellation, multi-select serialization, and a ReDoS regression guard.
• escapeShellArg covered in test/unit/util.test.ts for both platforms.
• ${env:VAR} substitution covered in test/unit/cliConfig.test.ts.
• State machine has dedicated tests for the cancellation vs failure branches in maybeUpdate.

Supporting context

The original issue was caused by coder update prompting for parameter values while the extension streams CLI output to a read-only VS Code OutputChannel.

This PR keeps the CLI update path but avoids interactive terminal prompts during remote setup by pre-collecting only parameters that need new values:

• required active-template parameters without defaults;
• excluding parameters that already have values on the current build;
• using quick picks for booleans/options/multi-selects and input boxes for free-form values;
• applying number-range validation client-side and deferring regex/monotonic constraints to the server.

The collected values are passed as --parameter name=value argv entries to coder update. If the CLI is older than --parameter-aware coder update, the existing REST API fallback handles the version bump.

[EhabY]

/coder-agents-review

[coder-agents-review]

First-pass review (Netero only). One P0: the new test file has a broken import that prevents the entire updateWorkspace/startWorkspace test suite from loading. The 424 new test lines are dead code until this is fixed.

The panel has not yet reviewed this PR. Fixing this P0 first avoids spending parallel review time on code that may shift once tests actually run.

Netero's take: "The test file is a ghost town. 462 lines of coverage, zero executions. One import stands between this PR and meaningful test feedback."

1 P0, 0 lower.

🤖 This review was automatically generated with Coder Agents.

[EhabY]

/coder-agents-review

[coder-agents-review]

Clean feature design. The parameter collection flow, state machine fallback with upfront mode downgrade, and workspace propagation through getWorkspace() are well-structured. The test infrastructure (controlSpawn, quickInputMock, table-driven parameter tests) is solid for async CLI + UI interactions. DEREM-1 (broken import) fixed in b59cadd.

One P1 blocks merge: escapeCommandArg only escapes " inside double quotes, but spawn uses shell: true. Template parameter names and option values come from the server with no character restriction, so a malicious template admin can achieve code execution on the user's local machine. Six of fourteen reviewers independently flagged this. Fix: either single-quote the parameter arg (matching getHeaderArgs in settings/headers.ts), or refactor runCliCommand to use spawn(binary, argsArray) without shell: true.

The PR description explains what the PR does but not why the CLI's own prompts can't be used. The doc comment on updateWorkspace has the answer ("the resolver phase can't render an interactive terminal"), but a reviewer reading the description first would wonder why coder update can't prompt the user directly.

Pariston: "The fix doesn't create a problem worse than the original."

1 P1, 6 P3, 4 Nit.

---

src/api/workspace.ts:131

Nit [DEREM-14] "canceled" vs "cancelled" inconsistency. Codebase convention for user-facing messages is "cancelled" (17 instances across progress.ts, workspaceStateMachine.ts, cliManager.ts, remote.ts). This line uses American spelling "canceled" to mirror the server API status string, but it's a user-facing error message, not an API enum. (Leorio)

🤖

src/api/workspace.ts:275

P3 [DEREM-3] makeValidator has 6 branching paths and formatConstraint has 6 paths, none with direct test coverage. Server-side validation is the safety net, so incorrect client validation produces confusing inline messages but does not pass bad values to the CLI. The functions total ~35 lines of branching logic. (Netero)

🤖

🤖 This review was automatically generated with Coder Agents.

[EhabY]

Addressing the two notes embedded in the review body (DEREM-3 and DEREM-14):

DEREM-3 (validator/constraint coverage): added a parameter prompt validation describe block in test/unit/api/updateParameters.test.ts that drives makeValidator and formatConstraint through collectUpdateParameters. Coverage spans the empty-required block, number range, non-numeric input, regex mismatch (both default and substituted message), {value} and {min}/{max} substitution, JSON-null bound handling, and placeholder rendering for each constraint shape. No new exports required.

DEREM-14 (canceled vs cancelled): fixed. Error message switched to "cancelled". The server enum string ("canceled") on the comparison side is unchanged.

All other inline threads replied to individually.

[EhabY]

/coder-agents-review

[coder-agents-review]

R2 findings thoroughly addressed. 12 of 13 open findings fixed in 892c77c with corresponding tests. The escapeShellArg extraction and deduplication with getHeaderArgs is a trust signal: the author generalized from a pointed correction. The WorkspaceUpdateCancelledError class cleanly separates user cancellation from failures. DEREM-7 (empty required field) contested; panel closed 6/6, accepting the author's defense that the stepped quick-input flow provides sufficient context.

Two new issues surfaced during fix verification. The DEREM-4 fix is correct on Unix (single quotes block all expansion) but incomplete on Windows: cmd.exe does not treat \" as an escape, and &, |, <, >, ^ are metacharacters even inside double quotes (DEREM-17, P1). Separately, the parameter collection filter is narrower than the CLI's prompting conditions; three scenarios exist where the CLI reaches an interactive prompt the extension didn't pre-collect, causing an unrecoverable hang because stdin is never closed (DEREM-18, P2).

Hisoka on the running-case status guard: "Dormant today because coder update blocks until the workspace is running. Breaks when CLI semantics change."

1 P1, 1 P2, 5 P3.

🤖 This review was automatically generated with Coder Agents.

[EhabY]

/coder-agents-review

[coder-agents-review]

R3 findings comprehensively addressed. The structural decision to drop shell: true from runCliCommand (DEREM-17) eliminates the entire class of shell injection bugs. proc.stdin.end() (DEREM-18) converts future unmatched CLI prompts into catchable errors. Dropping client-side regex (DEREM-22) removes the ReDoS surface. The ${env:VAR} substitution is a clean replacement for the lost shell expansion. All R3 fixes verified by Netero and the panel.

Two P2 findings from the shell: true removal. First, spawn without a shell changes the failure channel for a missing binary: Node emits an error event instead of a shell-reported non-zero exit. There is no error handler, so the promise never settles and the progress dialog hangs (DEREM-23, 3 reviewers). One-line fix: proc.on("error", reject). Second, getGlobalFlags includes getHeaderArgs output which applies escapeShellArg. Without a shell, those baked-in quotes become literal characters, breaking any multi-word --header-command value for update and start operations (DEREM-24).

Hisoka on the spawn error: "The DEREM-17 fix correctly removed shell: true but changed the failure channel. With a shell, a missing binary produces stderr and a non-zero close code. Without a shell, Node emits an error event on the ChildProcess."

2 P2, 2 P3.

---

src/api/workspace.ts:83

P3 [DEREM-25] Close handler drops the signal parameter, producing "exited with code null" when the process is killed.

proc.on("close", (code: number) => { ... }) annotates code as number and ignores the second parameter. Node's signature is (code: number | null, signal: NodeJS.Signals | null). When killed by signal (OOM, SIGTERM), code is null. An operator sees "exited with code null" with no indication of what killed the process.

Fix: accept (code: number | null, signal: NodeJS.Signals | null) and include signal in the message when code is null.

(Chopper P3, Hisoka P3)

🤖

🤖 This review was automatically generated with Coder Agents.

[EhabY]

Addressing DEREM-25 from the R4 review body (no inline thread for it):

Fixed in 83c36a3. The close handler now accepts (code: number | null, signal: NodeJS.Signals | null) and formats the error message based on which one is set:

• Non-zero exit: "..." exited with code N (unchanged path)
• Signal kill: "..." exited with signal SIGTERM (or signal unknown if both null)

Stderr is still appended when available. Added a regression test (reports the terminating signal when the process is killed) that closes the mocked process with (null, "SIGTERM") and asserts the message contains signal SIGTERM.

All other R4 inline threads replied to individually.