fix(task): preserve inner quotes for cmd /c tasks and hooks on windows#10301
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds Windows-specific support for preserving inner double quotes when executing inline scripts and hooks via ChangesWindows cmd.exe Quote Preservation
Sequence DiagramsequenceDiagram
participant TaskExecutor
participant get_cmd_program_and_args
participant exec_program
participant CmdLineRunner
participant Command
TaskExecutor->>get_cmd_program_and_args: script, shell=/c
get_cmd_program_and_args->>get_cmd_program_and_args: is_cmd_shell_program?
get_cmd_program_and_args->>get_cmd_program_and_args: cmd_verbatim_args(flags, script, forwarded_args)
get_cmd_program_and_args-->>TaskExecutor: (program, args, cmd_verbatim=true)
TaskExecutor->>exec_program: program, args, cmd_verbatim=true
exec_program->>CmdLineRunner: new(program)
alt Windows & cmd_verbatim
CmdLineRunner->>CmdLineRunner: raw_arg(args[i]) for each arg
CmdLineRunner->>Command: raw args passed unchanged
else Normal path
CmdLineRunner->>CmdLineRunner: args(args)
CmdLineRunner->>Command: stdlib/MSVCRT quoting applied
end
Command-->>CmdLineRunner: spawned
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
Greptile SummaryFixes inner-double-quote mangling in Windows
Confidence Score: 5/5Safe to merge; all changes are Windows-only, the quoting algorithm is correct, and non-Windows paths are untouched. The fix is narrowly scoped to Windows cmd.exe inline tasks and hooks. The backslash-doubling algorithm in quote_arg_for_cmd_body matches the MSVCRT spec exactly, the cmd /s /c outer-pair-stripping approach is a well-known cmd idiom, and the three code paths (task executor, hooks, cmd runner) are each consistent with their existing patterns. Unit tests cover the key quoting edge cases; the e2e test covers the end-to-end regression scenario on CI. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(task): preserve inner quotes for cmd..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e-win/task_quoting.Tests.ps1`:
- Around line 11-30: The AfterAll currently deletes the environment variable set
in BeforeAll; instead capture the original value of
$env:MISE_TRUSTED_CONFIG_PATHS in BeforeAll (e.g. $originalMISETrusted =
$env:MISE_TRUSTED_CONFIG_PATHS) and in AfterAll restore it (set
$env:MISE_TRUSTED_CONFIG_PATHS = $originalMISETrusted) or remove it only if it
did not exist originally, modifying the BeforeAll/AfterAll blocks and references
to $env:MISE_TRUSTED_CONFIG_PATHS and $originalPath accordingly.
In `@src/hooks.rs`:
- Around line 611-621: The current change pipes the child's stdout and spawns a
copier thread (variables c, child, copier) which can deadlock or reorder output
compared to the previous stdout_to_stderr() behavior; restore the old semantics
by not piping stdout for the child but redirecting the child's stdout to stderr
before spawn (or call the original stdout_to_stderr() helper) so background
children inherit the parent's descriptors and we avoid a blocking join on copier
and EOF waits; replace c.stdout(std::process::Stdio::piped()) + copier logic
with the prior redirection approach (e.g., use the stdout_to_stderr() path or
set c.stdout(...) to inherit / dup the fd to stderr) and remove the
join/wait-on-copier logic so child.wait() cannot block forever.
In `@src/path.rs`:
- Around line 205-208: quote_arg_for_cmd_body currently only quotes args with
whitespace or double quotes, leaving Windows cmd metacharacters (e.g. & | < > ^
% () etc.) unquoted so they get interpreted by cmd.exe; update
quote_arg_for_cmd_body to also detect any of the cmd metacharacters and treat
them like whitespace (i.e. wrap the argument in quotes) and ensure any internal
" characters are properly escaped/handled for the cmd /s /c "... " form; locate
the function quote_arg_for_cmd_body and change the initial if check to include a
test for these metacharacters and then return a safely quoted/escaped string
instead of the bare arg.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5188fc94-fd5c-4c03-b62d-8aef69ca3445
📒 Files selected for processing (5)
e2e-win/task_quoting.Tests.ps1src/cmd.rssrc/hooks.rssrc/path.rssrc/task/task_executor.rs
af8d7b7 to
02bf972
Compare
On Windows, inner double quotes in a task's `run` string (and in `[hooks]` commands) were mangled when spawned through the default `cmd /c` shell. mise passed the script as an ordinary argument to std::process::Command, which serializes argv into lpCommandLine using MSVCRT-style quoting (escaping inner `"` as `\"`). cmd.exe does not understand that escaping, so a command like `uv run python -c "import x"` reached the child as `\"import` and failed with a syntax error. `[hooks]` were hit identically, with no usage/shebang/file-based workaround available. Fix: when the inline/hook shell is cmd.exe, build the command line verbatim via raw command-line args. The whole command is wrapped in a single outer quote pair and run with `cmd /s /c "..."`, so cmd strips exactly that pair and leaves the user's inner quotes intact. Forwarded args are MSVCRT-quoted inside the outer pair so the spaces-in-args fix from jdx#6744 is preserved. `[hooks]` spawn through duct, which can't emit raw args, so the cmd case now spawns a std Command directly and forwards stdout to stderr to match the previous `stdout_to_stderr()` behavior. Adds `is_cmd_shell_program` / `cmd_verbatim_args` (+ unit tests) and an e2e-win test asserting inner quotes survive. Refs jdx#9355 and jdx#6119. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
02bf972 to
757123f
Compare
…es on windows jdx#10301 fixed inner-quote mangling for inline tasks and [hooks] on Windows by passing the command to cmd verbatim (`cmd /s /c "<body>"`). The same root cause affects the other call sites that spawn `cmd /c <command>`: `mise exec -c`, the tera `exec()` template function, [[watch_files]] hooks, tool postinstall hooks, [deps] commands, and credential commands. Add two shared helpers that reuse jdx#10301's `cmd_verbatim_args`: - `path::cmd_verbatim_command(program, flags, body) -> Option<Command>` for the duct / raw-Command sites (returns None for non-cmd shells so callers fall through to their existing path unchanged). - `CmdLineRunner::cmd_body_args(flags, body)` for the CmdLineRunner sites (identical to `args(flags).arg(body)` on Unix / non-cmd shells). Wire them into exec.rs (windows variant), tera.rs, watch_files.rs, backend/mod.rs, deps/engine.rs, and tokens.rs. Windows-only behavior change; non-cmd shells and all Unix paths are byte-for-byte unchanged. Adds unit gate tests for both helpers plus Unix and Windows e2e coverage (e2e/env/test_env_template, e2e-win/exec_inner_quotes.Tests.ps1). Refs jdx#9355. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…es on windows jdx#10301 fixed inner-quote mangling for inline tasks and [hooks] on Windows by passing the command to cmd verbatim (`cmd /s /c "<body>"`). The same root cause affects the other call sites that spawn `cmd /c <command>`: `mise exec -c`, the tera `exec()` template function, [[watch_files]] hooks, tool postinstall hooks, [deps] commands, and credential commands. Add two shared helpers that reuse jdx#10301's `cmd_verbatim_args`: - `path::cmd_verbatim_command(program, flags, body) -> Option<Command>` for the duct / raw-Command sites (returns None for non-cmd shells so callers fall through to their existing path unchanged). - `CmdLineRunner::cmd_body_args(flags, body)` for the CmdLineRunner sites (identical to `args(flags).arg(body)` on Unix / non-cmd shells). Wire them into exec.rs (windows variant), tera.rs, watch_files.rs, backend/mod.rs, deps/engine.rs, and tokens.rs. Windows-only behavior change; non-cmd shells and all Unix paths are byte-for-byte unchanged. Adds unit gate tests for both helpers plus Unix and Windows e2e coverage (e2e/env/test_env_template, e2e-win/exec_inner_quotes.Tests.ps1). Refs jdx#9355. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…es on windows jdx#10301 fixed inner-quote mangling for inline tasks and [hooks] on Windows by passing the command to cmd verbatim (`cmd /s /c "<body>"`). The same root cause affects the other call sites that spawn `cmd /c <command>`: `mise exec -c`, the tera `exec()` template function, [[watch_files]] hooks, tool postinstall hooks, [deps] commands, and credential commands. Add two shared helpers that reuse jdx#10301's `cmd_verbatim_args`: - `path::cmd_verbatim_command(program, flags, body) -> Option<Command>` for the duct / raw-Command sites (returns None for non-cmd shells so callers fall through to their existing path unchanged). - `CmdLineRunner::cmd_body_args(flags, body)` for the CmdLineRunner sites (identical to `args(flags).arg(body)` on Unix / non-cmd shells). Wire them into exec.rs (windows variant), tera.rs, watch_files.rs, backend/mod.rs, deps/engine.rs, and tokens.rs. Windows-only behavior change; non-cmd shells and all Unix paths are byte-for-byte unchanged. Adds unit gate tests for both helpers plus Unix and Windows e2e coverage (e2e/env/test_env_template, e2e-win/exec_inner_quotes.Tests.ps1). Refs jdx#9355. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…es on windows jdx#10301 fixed inner-quote mangling for inline tasks and [hooks] on Windows by passing the command to cmd verbatim (`cmd /s /c "<body>"`). The same root cause affects the other call sites that spawn `cmd /c <command>`: `mise exec -c`, the tera `exec()` template function, [[watch_files]] hooks, tool postinstall hooks, [deps] commands, and credential commands. Add two shared helpers that reuse jdx#10301's `cmd_verbatim_args`: - `path::cmd_verbatim_command(program, flags, body) -> Option<Command>` for the duct / raw-Command sites (returns None for non-cmd shells so callers fall through to their existing path unchanged). - `CmdLineRunner::cmd_body_args(flags, body)` for the CmdLineRunner sites (identical to `args(flags).arg(body)` on Unix / non-cmd shells). Wire them into exec.rs (windows variant), tera.rs, watch_files.rs, backend/mod.rs, deps/engine.rs, and tokens.rs. Windows-only behavior change; non-cmd shells and all Unix paths are byte-for-byte unchanged. Adds unit gate tests for both helpers plus Unix and Windows e2e coverage (e2e/env/test_env_template, e2e-win/exec_inner_quotes.Tests.ps1). Refs jdx#9355. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Problem
On Windows, inner double quotes in a task's
runstring — and in[hooks]commands — are mangled when spawned through the defaultcmd /cshell. Reported in #9355 (originally #6119).The debug line looks correct, but Python receives just
"import. Likewiserun = 'echo "x"'prints\"x\".[hooks](postinstall,enter,cd, …) are hit identically, and unlike tasks they have nousage/shebang/file-based escape hatch.Root cause
mise builds the argv
["cmd", "/c", <script>, …]and passes the script as an ordinarystd::process::Commandargument. On Windows, std serializes argv intolpCommandLineusing MSVCRT/CommandLineToArgvW-style quoting (it wraps the script in quotes and escapes inner"as\").cmd.exedoes not parse that convention — it treats\"literally — so the child receives mangled arguments. Noraw_argwas used anywhere in the tree.Fix
When the inline/hook shell is
cmd.exe, build the command line verbatim via raw command-line args:cmd /s /c "<body>"./sstrips exactly that one outer pair and leaves the remainder — including the user's inner quotes — untouched.mise run type ".\test dir\file.txt").[hooks]spawn through duct, which can't emit raw args, so the cmd case now spawns astd::process::Commanddirectly and forwards stdout to stderr to match the previousstdout_to_stderr()behavior.pwsh -Command,bash -c, …) are gated out viais_cmd_shell_programand keep the existing behavior.New helpers live in
src/path.rs(is_cmd_shell_program,cmd_verbatim_args,quote_arg_for_cmd_body) so the other call sites can reuse them later.Scope / follow-ups
[hooks](the two paths reported in mise tasks on Windows arguments with spaces not being escaped? #9355).watch_files,deps,{{ exec() }},mise exec -c, and backend scripts. Those can reusecmd_verbatim_argsin a follow-up.shell = "bash -c"$0-shift issue noted in the discussion is left for a follow-up.Testing
is_cmd_shell_program,cmd_verbatim_args, andquote_arg_for_cmd_body(covering the MSVCRT quoting edge cases and thetype ".\test dir\file.txt"case frome2e-win/task_args.Tests.ps1). I verified this pure string logic locally.e2e-win/task_quoting.Tests.ps1: asserts inner quotes survive (no\mangling) and that an inner-quoted-cargument reaches the program as a single argument.#[cfg(windows)]branches at all, so I'm relying on CI for the Windows build, clippy, and thee2e-winrun.Refs #9355, #6119.
This pull request was prepared with the help of an AI coding assistant.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests