Fix PowerShell quoting for runInTerminal environment values - #331753
Fix PowerShell quoting for runInTerminal environment values#331753Zain Nadeem (zainnadeem786) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes PowerShell quoting for runInTerminal environment values.
Changes:
- Reuses PowerShell argument quoting for environment values.
- Adds regression coverage for embedded single quotes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/vs/workbench/contrib/debug/node/terminals.ts |
Updates PowerShell environment assignment quoting. |
src/vs/workbench/contrib/debug/test/node/terminals.test.ts |
Tests common and quoted environment values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/vs/workbench/contrib/debug/node/terminals.ts:106
- The implementation contradicts the PR description and linked issue, which explicitly say this assignment now reuses
quote(value). It instead creates a second PowerShell escaping path; this distinction is material becausequote()also special-cases trailing backslashes while the new tests expect environment values not to. Please either share/factor the literal-escaping logic while preserving the intended trailing-backslash behavior, or update the PR rationale to explain whyquote()cannot be reused.
command += `\${env:${key}}='${value.replace(/'/g, '\'\'')}'; `;
Connor Peet (connor4312)
left a comment
There was a problem hiding this comment.
The ASCII-apostrophe fix and trailing-backslash handling look correct in both Windows PowerShell 5.1 and PowerShell 7. One remaining gap: PowerShell also treats U+2018/U+2019 smart single quotes as quote characters, but this only doubles ASCII '. Please escape those as well and add regression coverage.
Separately, ${env:X}='' removes X in PowerShell 5.1 and 7.0–7.4, while 7.5+ preserves an empty value. That is pre-existing and need not block this focused fix, but the snapshot test does not establish equivalent runtime behavior across versions.
Thanks for checking this across Windows PowerShell 5.1 and PowerShell 7. Good catch on the U+2018/U+2019 smart quotes. I'll update the escaping and regression coverage for those cases while keeping the empty-value behavior out of scope for this focused fix. |
|
Thanks for the review Connor Peet (@connor4312) . Confirmed the U+2018/U+2019 behavior in both Windows PowerShell 5.1 and PowerShell 7, and updated the escaping to double all three PowerShell single-quote characters while preserving trailing backslashes. I also added regression coverage for both smart-quote cases. The focused I’ve kept the version-dependent empty environment value behavior out of scope as suggested. |
Summary
This PR fixes PowerShell environment value quoting when constructing commands for Debug Adapter Protocol
runInTerminalrequests.Environment values were previously interpolated directly inside PowerShell single-quoted strings:
This works for ordinary values, but values containing embedded single quotes are not escaped correctly.
For example, an environment value such as:
was represented as:
instead of the correctly escaped PowerShell representation:
Root cause
PowerShell single-quoted strings represent a literal single quote by doubling it (
'').The existing environment assignment wrapped values in single quotes without escaping embedded single quotes, so values containing
'were not represented correctly.The existing PowerShell
quote()helper used for command arguments cannot be reused directly for environment values because it also special-cases values ending in a backslash. That behavior is appropriate for argument construction, but PowerShell single-quoted strings treat backslashes literally, so applying it to environment values would change values such as:into a value with an additional trailing backslash.
Change
Environment values now escape embedded single quotes directly while preserving literal backslashes:
This applies the escaping required for PowerShell single-quoted strings without applying argument-specific trailing-backslash handling to environment values.
Existing handling of
nullenvironment values remains unchanged.Regression coverage
Added a focused
prepareCommandregression test covering PowerShell environment values with:For example:
is represented as:
A value ending in a backslash:
is preserved as:
And a value containing both an embedded quote and backslashes is represented without altering the backslashes:
becomes:
Validation
The focused regression test passes:
The existing
prepareCommandtest suite also passes:Client transpilation completes successfully:
git diff --checkalso completes with exit code0.The resulting PowerShell single-quoted representation was additionally verified directly in PowerShell.
Scope
The change is intentionally limited to PowerShell environment value construction in
prepareCommand().No behavior for other shells is changed, and existing handling of
nullenvironment values remains unchanged.Close #331751