fix(core): quote arguments passed to external commands on Windows

[cataggar]

Overview

Preserve shell-expanded arguments when launching external commands on Windows, including programs built on the MSYS2 or Cygwin runtime.

This change was made by GitHub Copilot at my direction.

Details

Windows passes a child process one command-line string rather than an argv array. Rust's standard library quotes arguments when required for native Windows parsing, but otherwise leaves them unquoted. MSYS2 and Cygwin programs can then apply their own glob, brace, tilde, and quote processing to those arguments, corrupting values that the shell has already expanded.

Examples include:

grep -oE '[0-9]{3}'   -> [0-9]3
grep -oE 'MAC\(x'     -> MAC(x
find . -name '*.txt'  -> every matching file in the directory
printf '%s' '~'       -> the home directory

This change:

• routes command argument setup through a platform-specific sys::commands::set_args;
• leaves Unix and stub targets on the existing Command::args behavior;
• adds a Windows implementation that explicitly quotes every argument using CommandLineToArgvW and Microsoft C runtime rules;
• correctly escapes embedded quotes and trailing backslashes;
• preserves the standard library path for cmd.exe, .bat, and .cmd files because they require different escaping and injection protections; and
• keeps command tracing based on the shell-expanded arguments rather than their Windows command-line encoding.

The Windows module includes focused tests for ordinary arguments, MSYS metacharacters, embedded quotes, trailing backslashes, cmd.exe detection, batch-file behavior, and an optional end-to-end check using Git for Windows' MSYS2 printf.exe.

Testing

• cargo test --package brush-core
• cargo check --package brush-core --all-features --all-targets --target x86_64-pc-windows-gnu
• cargo clippy --package brush-core --all-features --all-targets --target x86_64-pc-windows-gnu
• cargo xtask check fmt

[github-actions]

Public API changes for crate: brush-core

Added items

+pub fn brush_core::sys::commands::set_args<S: core::convert::AsRef<std::ffi::os_str::OsStr>>(&mut std::process::Command, &[S])

Performance Benchmark Report

Benchmark name	Baseline (μs)	Test/PR (μs)	Delta (μs)	Delta %
clone_shell_object	17.70 μs	17.38 μs	-0.32 μs	🟢 -1.82%
eval_arithmetic	0.16 μs	0.17 μs	0.00 μs	⚪ Unchanged
expand_one_string	1.73 μs	1.79 μs	0.06 μs	⚪ Unchanged
for_loop	32.52 μs	33.08 μs	0.56 μs	⚪ Unchanged
full_peg_complex	58.05 μs	59.91 μs	1.86 μs	🟠 +3.20%
full_peg_for_loop	6.21 μs	6.52 μs	0.30 μs	🟠 +4.88%
full_peg_nested_expansions	16.40 μs	16.59 μs	0.19 μs	⚪ Unchanged
full_peg_pipeline	4.34 μs	4.55 μs	0.21 μs	🟠 +4.79%
full_peg_simple	1.86 μs	1.84 μs	-0.02 μs	⚪ Unchanged
function_call	4.04 μs	3.98 μs	-0.06 μs	⚪ Unchanged
instantiate_shell	56.32 μs	56.64 μs	0.32 μs	⚪ Unchanged
instantiate_shell_with_init_scripts	28138.78 μs	31371.75 μs	3232.97 μs	🟠 +11.49%
parse_peg_bash_completion	2122.43 μs	2120.20 μs	-2.24 μs	⚪ Unchanged
parse_peg_complex	19.76 μs	18.88 μs	-0.88 μs	🟢 -4.44%
parse_peg_for_loop	1.96 μs	1.97 μs	0.02 μs	⚪ Unchanged
parse_peg_pipeline	2.06 μs	2.14 μs	0.08 μs	⚪ Unchanged
parse_peg_simple	1.08 μs	1.10 μs	0.02 μs	⚪ Unchanged
run_echo_builtin_command	16.54 μs	16.91 μs	0.37 μs	⚪ Unchanged
tokenize_sample_script	3.57 μs	3.64 μs	0.06 μs	⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
brush-builtins/src/command.rs	🟢 98.78%	🟢 100%	🟢 1.22%
brush-builtins/src/declare.rs	🟢 87.4%	🟢 90.35%	🟢 2.95%
brush-builtins/src/export.rs	🟠 75%	🟢 93.48%	🟢 18.48%
brush-core/src/commands.rs	🟢 91.59%	🟢 94.22%	🟢 2.63%
brush-core/src/env.rs	🟢 87.16%	🟢 88.38%	🟢 1.22%
brush-core/src/sys/unix/commands.rs	🔴 38.64%	🔴 42.55%	🟢 3.91%
brush-test-harness/src/comparison.rs	🟢 83.97%	🟢 76.92%	🔴 -7.05%
brush-test-harness/src/execution.rs	🟢 83.17%	🟢 80.2%	🔴 -2.97%
brush-test-harness/src/reporting.rs	🔴 46.48%	🔴 11.62%	🔴 -34.86%
brush-test-harness/src/runner.rs	🟢 84.58%	🟢 83.98%	🔴 -0.6%
brush-test-harness/src/util.rs	🟠 71.64%	🔴 47.76%	🔴 -23.88%
Overall Coverage	🟢 75.84%	🟢 75.46%	🔴 -0.38%

Minimum allowed coverage is 70%, this run produced 75.46%
Maximum allowed coverage difference is -5%, this run produced -0.38%

Test Summary: bash-completion test suite

Outcome	Count	Percentage
✅ Pass	1583	75.06
❗️ Error	17	0.81
❌ Fail	155	7.35
⏩ Skip	339	16.07
❎ Expected Fail	13	0.62
✔️ Unexpected Pass	2	0.09
📊 Total	2109	100.00

[reubeno]

Thanks for highlighting the issue that you're seeing in this scenario. May main concern is whether this is addressing the problem at the right layer of the stack, and whether this kind of logic could cause problems for other called programs.

Let me do a bit more research, think about it a bit more, and respond back with some other suggestions for consideration.

[reubeno]

This makes it sound like the issue is not actually a brush-specific issue, but a challenging assumption made by msys2/cygwin tools. Perhaps I'm missing something, but wouldn't this be a problem for any Rust application that might use std::process::Command to invoke a program on Windows that might behave like this?

Do you know if there are precedents for how any other such apps have handled this?

[reubeno]

This kind of callee-specific logic, to me, begs the question of how many other applications may similarly have challenges and what risk there is in encoding such specific logic in our general execution paths. I understand there are real limitations that this PR is trying to mitigate but I worry that this is a slippery slope toward accumulated heuristics for how to interact with specific (Windows) invoked programs, and that's something that I would expect a general-purpose shell avoids.

[reubeno]

@cataggar -- some additional things I found or thought about while doing a bit more research:

• There's an unstable feature for enabling force_quotes behavior on commands in Rust. Looks like you found it as well, as I see you referenced it.

• There's a noglob option that can be set via env vars (CYGWIN or MSYS) when launching cygwin/msys processes -- and that looks like it inhibits the default (re)expansion. I think it's worth considering whether that would mitigate the problem. (I don't understand offhand whether there are negative ramifications of this or if it would leak past the directly invoked child.) I'm not sure how I'd feel about brush default setting this, but if it were set in a brush-specific rc file, I wonder if it would be sufficient to cover the case here.

• As best as I can tell, the core Python library doesn't handle this in its subprocess module -- but I do suppose callers could always work around that. I'm really curious to know if any other cross-platform runtimes specifically try to handle this in some way.

• If we convince ourselves we do need to support some kind of force-quoting behavior, I think it should be something that's configurable and not unconditionally hard-wired behavior.