Skip to content

Commit 5a5a768

Browse files
committed
docs: address review feedback on comments
Per @vicb's review: - helpers.ts: rewrite quoteShellMeta docstring so it describes both the Windows and POSIX branches with equal weight, rather than treating POSIX as a footnote. The function works on both platforms. - helpers.ts: simplify the inner POSIX comment to "POSIX shell quoting" per the suggestion. - run-wrangler.ts: hoist the shell:false / DEP0190 explanation to the top-level comment block so it covers both branches, then describe the POSIX path and the Windows path in turn. Drop the now-redundant inner comment in the POSIX spawn call. - run-wrangler.ts: restore the stdio explanatory comment that was lost when stdio was hoisted to a const in an earlier refactor. No behavioral changes.
1 parent 229c45f commit 5a5a768

2 files changed

Lines changed: 29 additions & 26 deletions

File tree

‎packages/cloudflare/src/cli/commands/utils/helpers.ts‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -73,32 +73,32 @@ export async function getEnvFromPlatformProxy(options: GetPlatformProxyOptions,
7373
}
7474

7575
/**
76-
* Escape a single argument so it survives the `cmd.exe /d /s /c "<command line>"` invocation
77-
* used by `runWrangler` on Windows (paired with `windowsVerbatimArguments: true`).
76+
* Escape a single argument for shell-style invocation. Cross-platform.
7877
*
79-
* The algorithm matches `cross-spawn`'s `escapeArgument`:
78+
* On Windows, `runWrangler` invokes `cmd.exe /d /s /c "<command line>"` with
79+
* `windowsVerbatimArguments: true`. The algorithm matches `cross-spawn`'s `escapeArgument`:
8080
* 1. Wrap the value in `"..."`, doubling backslash sequences that precede a quote per the
8181
* Windows command-line argument rules.
8282
* 2. Caret-escape every cmd.exe metacharacter, including the wrapping quotes themselves.
83+
* The wrapping carets are needed for the OUTER `cmd /c "..."` parse: cmd.exe consumes them
84+
* and the receiving program sees a normally-quoted argument. Crucially, `^` must NOT be
85+
* applied inside an un-caret-escaped quoted region, because cmd.exe treats `^` as literal
86+
* inside double quotes (this was the bug an earlier shell-quote-style implementation hit).
8387
*
84-
* The carets are needed for the OUTER `cmd /c "..."` parse (where the inner quotes would
85-
* otherwise terminate the command-line string); cmd.exe consumes them and the receiving
86-
* program sees a normally-quoted argument. Crucially, `^` must NOT be applied inside an
87-
* un-caret-escaped quoted region: `cmd.exe` treats `^` as literal inside double quotes,
88-
* which is the bug an earlier shell-quote-style implementation suffered from.
88+
* On POSIX, the value is quoted using standard POSIX shell rules (single quotes by default,
89+
* double quotes when the value contains a single quote, backslash-escapes for bare values).
90+
* `runWrangler` itself does not invoke a shell on POSIX, but this branch is preserved so the
91+
* function behaves usefully for any direct caller.
8992
*
90-
* Not used on POSIX, where `runWrangler` spawns the package manager directly with
91-
* `shell: false` and each argv entry is passed verbatim.
92-
*
93-
* Reference: https://github.com/moxystudio/node-cross-spawn/blob/master/lib/util/escape.js
94-
* https://qntm.org/cmd
93+
* References: https://github.com/moxystudio/node-cross-spawn/blob/master/lib/util/escape.js
94+
* https://qntm.org/cmd
9595
*
9696
* @param arg
97-
* @returns escaped arg suitable for embedding in a `cmd /c "..."` command line
97+
* @returns escaped arg
9898
*/
9999
export function quoteShellMeta(arg: string) {
100100
if (process.platform !== "win32") {
101-
// POSIX fallback (preserved for any external callers): standard shell quoting.
101+
// POSIX shell quoting.
102102
if (/["\s]/.test(arg) && !/'/.test(arg)) {
103103
return `'${arg.replace(/(['\\])/g, "\\$1")}'`;
104104
}

‎packages/cloudflare/src/cli/commands/utils/run-wrangler.ts‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,9 @@ export function runWrangler(
112112
]),
113113
];
114114

115+
// Always pipe stderr so that we can capture it for inspection.
116+
// Keep stdin and stdout as "inherit" when not piping logs to maintain TTY detection
117+
// (wrangler checks `process.stdin.isTTY && process.stdout.isTTY` for interactive mode).
115118
const stdio: ("inherit" | "ignore" | "pipe")[] =
116119
shouldPipeLogs || noLogs ? ["ignore", "pipe", "pipe"] : ["inherit", "inherit", "pipe"];
117120

@@ -124,16 +127,18 @@ export function runWrangler(
124127
CLOUDFLARE_LOAD_DEV_VARS_FROM_DOT_ENV: "false",
125128
};
126129

127-
// On Windows the package manager binary is typically a `.cmd` shim. Per Node.js docs
128-
// https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows the
129-
// recommended way to invoke a `.cmd`/`.bat` is to spawn `cmd.exe` directly with `/c`,
130-
// rather than relying on `shell: true` (which emits DEP0190 and would re-tokenize values).
131-
// We pre-escape every token with `quoteShellMeta` and pass the joined command line as a
132-
// single quoted string with `windowsVerbatimArguments: true` so Node does not re-escape it.
133-
// This mirrors how `cross-spawn` handles non-`.exe` commands on Windows.
130+
// We use `shell: false` everywhere to avoid DEP0190.
131+
// See https://nodejs.org/api/deprecations.html#dep0190-passing-args-to-nodechild-process-execfilespawn-with-shell-option
134132
//
135-
// On POSIX we spawn the package manager directly with `shell: false`, which avoids DEP0190
136-
// and lets each value be passed verbatim as its own argv entry.
133+
// On POSIX we can spawn the package manager binary directly: each value is passed verbatim
134+
// as its own argv entry.
135+
//
136+
// On Windows the package manager is typically a `.cmd` shim. Per Node.js docs
137+
// (https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows) the
138+
// recommended way to invoke a `.cmd`/`.bat` without `shell: true` is to spawn `cmd.exe`
139+
// directly with `/c`. We pre-escape every token with `quoteShellMeta` and pass the joined
140+
// command line as a single quoted string with `windowsVerbatimArguments: true` so Node
141+
// does not re-escape it. This mirrors how `cross-spawn` handles non-`.exe` commands.
137142
const result = isWin
138143
? spawnSync(
139144
"cmd.exe",
@@ -151,8 +156,6 @@ export function runWrangler(
151156
}
152157
)
153158
: spawnSync(options.packager, packagerArgs, {
154-
// Setting `shell` to `false` to avoid DEP0190.
155-
// See https://nodejs.org/api/deprecations.html#dep0190-passing-args-to-nodechild-process-execfilespawn-with-shell-option
156159
shell: false,
157160
stdio,
158161
env,

0 commit comments

Comments
 (0)