From 42fee12d45ef35af9d665b2ceab9674a51b05171 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Thu, 21 May 2026 22:16:20 +0800 Subject: [PATCH 01/23] Extract cmdline builder with proper quoting and add unit tests - Extract build_wt_commandline() from shell_manager into cmdline.rs - Fix quoting bug: embedded double-quotes in args were not escaped, allowing argument injection via CommandLineToArgvW parsing - Add 26 unit tests that round-trip through the real OS parser (CommandLineToArgvW) covering: quotes, backslashes, whitespace, empty args, shell metacharacters, NUL rejection, and realistic agent commands - Add Win32_UI_Shell feature to windows-sys for test-time CommandLineToArgvW access Future direction: replace hand-rolled commandline escaping with JSON-encoded structured args to eliminate the escaping problem by construction (serde_json is already a dependency). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/wta/Cargo.toml | 1 + tools/wta/src/cmdline.rs | 325 +++++++++++++++++++++++++++ tools/wta/src/main.rs | 1 + tools/wta/src/shell/shell_manager.rs | 19 +- 4 files changed, 333 insertions(+), 13 deletions(-) create mode 100644 tools/wta/src/cmdline.rs diff --git a/tools/wta/Cargo.toml b/tools/wta/Cargo.toml index 3a9d9b20c..654c4dbdd 100644 --- a/tools/wta/Cargo.toml +++ b/tools/wta/Cargo.toml @@ -27,6 +27,7 @@ windows-sys = { version = "0.61", features = [ "Win32_System_Environment", "Win32_System_Registry", "Win32_System_Threading", + "Win32_UI_Shell", ] } tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter", "fmt"] } diff --git a/tools/wta/src/cmdline.rs b/tools/wta/src/cmdline.rs new file mode 100644 index 000000000..3390eba92 --- /dev/null +++ b/tools/wta/src/cmdline.rs @@ -0,0 +1,325 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// Pure functions for building Windows commandline strings. +// Extracted from shell_manager for testability. + +/// Error returned by [`build_wt_commandline`] when the input cannot be +/// encoded as a valid Windows commandline. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BuildCommandlineError { + /// The program path (argv[0]) contains a literal `"`. There is no + /// `CommandLineToArgvW`-compatible way to escape it. + QuoteInProgram, + /// The program path contains a NUL byte. + NulInProgram, + /// An argument contains a NUL byte. + NulInArgument, +} + +impl std::fmt::Display for BuildCommandlineError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::QuoteInProgram => { + f.write_str("executable path cannot contain a literal double quote") + } + Self::NulInProgram => f.write_str("executable path cannot contain a NUL byte"), + Self::NulInArgument => f.write_str("argument cannot contain a NUL byte"), + } + } +} + +impl std::error::Error for BuildCommandlineError {} + +/// Quote the program path (argv[0]). `CommandLineToArgvW` treats the first +/// token specially: backslashes are literal, and the first unescaped `"` +/// ends argv[0]. There is no way to escape `"` inside it, so we reject +/// inputs containing `"`. +fn append_program(cmdline: &mut String, value: &str) -> Result<(), BuildCommandlineError> { + if value.contains('\0') { + return Err(BuildCommandlineError::NulInProgram); + } + if value.contains('"') { + return Err(BuildCommandlineError::QuoteInProgram); + } + cmdline.push('"'); + cmdline.push_str(value); + cmdline.push('"'); + Ok(()) +} + +/// Append a non-first argument, quoting per `CommandLineToArgvW` rules. +/// Always quotes unconditionally — a `needs_quotes` heuristic is fragile +/// because the OS parser splits on more than just space/tab. +fn append_arg(cmdline: &mut String, value: &str) -> Result<(), BuildCommandlineError> { + if value.contains('\0') { + return Err(BuildCommandlineError::NulInArgument); + } + cmdline.push('"'); + let mut backslashes: usize = 0; + for ch in value.chars() { + match ch { + '\\' => backslashes += 1, + '"' => { + // Double the backslashes before a `"`, then escape the `"`. + for _ in 0..(backslashes * 2 + 1) { + cmdline.push('\\'); + } + cmdline.push('"'); + backslashes = 0; + } + _ => { + for _ in 0..backslashes { + cmdline.push('\\'); + } + backslashes = 0; + cmdline.push(ch); + } + } + } + // Trailing backslashes must be doubled (they precede the closing `"`). + for _ in 0..(backslashes * 2) { + cmdline.push('\\'); + } + cmdline.push('"'); + Ok(()) +} + +/// Build a commandline string from a command and its arguments. +/// +/// The result is compatible with `CommandLineToArgvW` / `CreateProcess`. +/// There is no shell in this pipeline, so metacharacters like `&` / `|` +/// are not special. +pub fn build_wt_commandline( + command: &str, + args: &[String], +) -> Result { + let mut cmdline = String::new(); + append_program(&mut cmdline, command)?; + for arg in args { + cmdline.push(' '); + append_arg(&mut cmdline, arg)?; + } + Ok(cmdline) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Parse a commandline string through the real Windows OS parser. + /// This is the ground truth — if our output round-trips through + /// `CommandLineToArgvW` and matches the original input, we're correct. + fn parse_via_os(cmdline: &str) -> Vec { + use std::ffi::OsStr; + use std::os::windows::ffi::OsStrExt; + use windows_sys::Win32::Foundation::LocalFree; + use windows_sys::Win32::UI::Shell::CommandLineToArgvW; + + let wide: Vec = OsStr::new(cmdline) + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + + let mut argc: i32 = 0; + let argv = unsafe { CommandLineToArgvW(wide.as_ptr(), &mut argc) }; + assert!(!argv.is_null(), "CommandLineToArgvW returned null"); + + let mut parsed = Vec::with_capacity(argc as usize); + for i in 0..argc as isize { + let ptr = unsafe { *argv.offset(i) }; + let mut len = 0isize; + while unsafe { *ptr.offset(len) } != 0 { + len += 1; + } + let slice = unsafe { std::slice::from_raw_parts(ptr, len as usize) }; + parsed.push(String::from_utf16(slice).expect("invalid UTF-16 from OS")); + } + unsafe { LocalFree(argv as _) }; + parsed + } + + /// Helper: build commandline, parse via OS, assert round-trip matches. + fn assert_roundtrip(command: &str, args: &[&str]) { + let args_owned: Vec = args.iter().map(|s| s.to_string()).collect(); + let cmdline = + build_wt_commandline(command, &args_owned).expect("build_wt_commandline failed"); + let parsed = parse_via_os(&cmdline); + + let mut expected = vec![command.to_string()]; + expected.extend(args_owned); + + assert_eq!( + parsed, expected, + "\n cmdline = {:?}\n parsed = {:?}\n expected= {:?}", + cmdline, parsed, expected, + ); + } + + // ── Basic cases ──────────────────────────────────────────────── + + #[test] + fn simple_command_no_args() { + assert_roundtrip("pwsh.exe", &[]); + } + + #[test] + fn simple_command_with_args() { + assert_roundtrip("pwsh.exe", &["-c", "git status"]); + } + + #[test] + fn command_with_spaces_in_path() { + assert_roundtrip("C:\\Program Files\\my tool\\run.exe", &["--verbose"]); + } + + // ── Quoting edge cases ───────────────────────────────────────── + + #[test] + fn arg_with_embedded_double_quote() { + assert_roundtrip("cmd.exe", &["/c", "echo \"hello\""]); + } + + #[test] + fn arg_with_only_double_quotes() { + assert_roundtrip("test.exe", &["\"\"\""]); + } + + #[test] + fn arg_with_spaces_and_quotes() { + assert_roundtrip("test.exe", &["hello \"world\" foo"]); + } + + // ── Backslash edge cases ─────────────────────────────────────── + + #[test] + fn arg_with_trailing_backslash() { + assert_roundtrip("test.exe", &["C:\\path\\"]); + } + + #[test] + fn arg_with_trailing_backslashes() { + assert_roundtrip("test.exe", &["C:\\path\\\\\\"]); + } + + #[test] + fn arg_with_backslash_before_quote() { + assert_roundtrip("test.exe", &["foo\\\"bar"]); + } + + #[test] + fn arg_with_multiple_backslashes_before_quote() { + assert_roundtrip("test.exe", &["foo\\\\\"bar"]); + } + + #[test] + fn arg_backslashes_not_before_quote() { + assert_roundtrip("test.exe", &["C:\\Users\\test\\file.txt"]); + } + + // ── Whitespace edge cases ────────────────────────────────────── + + #[test] + fn arg_with_tab() { + assert_roundtrip("test.exe", &["hello\tworld"]); + } + + #[test] + fn arg_with_newline() { + assert_roundtrip("test.exe", &["line1\nline2"]); + } + + #[test] + fn arg_with_carriage_return() { + assert_roundtrip("test.exe", &["line1\rline2"]); + } + + #[test] + fn empty_arg() { + assert_roundtrip("test.exe", &[""]); + } + + #[test] + fn multiple_empty_args() { + assert_roundtrip("test.exe", &["", "", ""]); + } + + // ── Shell metacharacters (should be literal, no shell) ───────── + + #[test] + fn arg_with_pipe() { + assert_roundtrip("test.exe", &["foo|bar"]); + } + + #[test] + fn arg_with_ampersand() { + assert_roundtrip("test.exe", &["foo&bar"]); + } + + #[test] + fn arg_with_percent() { + assert_roundtrip("test.exe", &["%PATH%"]); + } + + #[test] + fn arg_with_caret() { + assert_roundtrip("test.exe", &["foo^bar"]); + } + + // ── Error cases ──────────────────────────────────────────────── + + #[test] + fn rejects_quote_in_program() { + let result = build_wt_commandline("bad\"path.exe", &[]); + assert_eq!(result, Err(BuildCommandlineError::QuoteInProgram)); + } + + #[test] + fn rejects_nul_in_program() { + let result = build_wt_commandline("bad\0path.exe", &[]); + assert_eq!(result, Err(BuildCommandlineError::NulInProgram)); + } + + #[test] + fn rejects_nul_in_argument() { + let args = vec!["hello\0world".to_string()]; + let result = build_wt_commandline("test.exe", &args); + assert_eq!(result, Err(BuildCommandlineError::NulInArgument)); + } + + // ── Stress / combo cases ─────────────────────────────────────── + + #[test] + fn many_args_mixed() { + assert_roundtrip( + "C:\\Program Files\\app.exe", + &[ + "--flag", + "simple", + "has spaces", + "has\"quote", + "trailing\\", + "back\\\"slash-quote", + "", + "\\\\server\\share\\", + "multi\nline\ttab", + ], + ); + } + + #[test] + fn realistic_agent_command() { + assert_roundtrip( + "pwsh.exe", + &["-NoProfile", "-Command", "& { git log --oneline -5 }"], + ); + } + + #[test] + fn realistic_npx_adapter() { + assert_roundtrip( + "npx", + &["-y", "@zed-industries/claude-code-acp"], + ); + } +} diff --git a/tools/wta/src/main.rs b/tools/wta/src/main.rs index 45ed54ddd..08fa12799 100644 --- a/tools/wta/src/main.rs +++ b/tools/wta/src/main.rs @@ -6,6 +6,7 @@ mod agent_registry; mod agent_sessions; mod agent_hooks_installer; mod app; +pub mod cmdline; mod commands; mod coordinator; mod event; diff --git a/tools/wta/src/shell/shell_manager.rs b/tools/wta/src/shell/shell_manager.rs index a727d4a28..b2e83a3d5 100644 --- a/tools/wta/src/shell/shell_manager.rs +++ b/tools/wta/src/shell/shell_manager.rs @@ -6,6 +6,7 @@ use tokio::process::Child; use tokio::process::Command; use super::wt_channel::WtChannel; +use crate::cmdline::build_wt_commandline; /// Configuration for creating a new terminal. pub struct TerminalConfig { @@ -115,19 +116,11 @@ impl ShellManager { let id = self.next_id(); let wt = self.wt()?; - // Build the commandline string: "command arg1 arg2 ..." - let mut cmdline = config.command.clone(); - for arg in &config.args { - cmdline.push(' '); - // Quote args containing spaces - if arg.contains(' ') { - cmdline.push('"'); - cmdline.push_str(arg); - cmdline.push('"'); - } else { - cmdline.push_str(arg); - } - } + // Build the commandline string for WT pane creation. Returns an + // error if the agent-supplied command is unrepresentable (e.g. + // contains a literal `"`); propagate so the caller can surface it + // to the agent instead of crashing the wta process. + let cmdline = build_wt_commandline(&config.command, &config.args)?; // Create a new tab in WT with the command let mut params = serde_json::Map::new(); From a5be1e83149e58fd6b97e2458853c1080bd957ef Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Thu, 21 May 2026 22:52:48 +0800 Subject: [PATCH 02/23] feat: pass agent config as JSON-encoded --agent-config argument Replace hand-rolled commandline escaping with a single JSON-encoded arg. C++ host serializes agent config into JSON, passes via --agent-config. Rust WTA deserializes with serde_json (backward compatible). Security: eliminates 6 hand-rolled escaping sites, reduces attack surface to one correctly-quoted argument boundary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 153 ++++-------------- .../AIAgentsViewModel.cpp | 11 +- src/cascadia/inc/QuoteArgForCommandLine.h | 146 +++++++++++++++++ tools/wta/src/cmdline.rs | 83 ++++++++++ tools/wta/src/main.rs | 79 ++++++++- 5 files changed, 343 insertions(+), 129 deletions(-) create mode 100644 src/cascadia/inc/QuoteArgForCommandLine.h diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 742fec642..4f3a77610 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -31,6 +31,7 @@ #include "TerminalSettingsCache.h" #include "TerminalProtocolPipeServer.h" #include "WtaProcessLauncher.h" +#include "QuoteArgForCommandLine.h" #include "LaunchPositionRequest.g.cpp" #include "RenameWindowRequestedArgs.g.cpp" @@ -1163,36 +1164,21 @@ namespace winrt::TerminalApp::implementation const auto& globals = _settings.GlobalSettings(); const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); - // Helper: escape and quote an argument for the command line. - auto quoteArg = [](std::wstring_view arg) -> std::wstring { - std::wstring escaped{ arg }; - for (size_t pos = 0; (pos = escaped.find(L'"', pos)) != std::wstring::npos; pos += 2) - { - escaped.replace(pos, 1, L"\\\""); - } - return L"\"" + escaped + L"\""; - }; + using Microsoft::Terminal::CommandLine::QuoteArgForCommandLine; - // Build: wta delegate --agent --delegate-agent "" - std::wstring cmdline = quoteArg(wtaPath) + L" delegate"; - - if (!agentCliPath.empty()) - { - cmdline += L" --agent " + quoteArg(std::wstring_view{ agentCliPath }); - } + // Build: wta delegate --agent-config --cwd "" + std::wstring cmdline = QuoteArgForCommandLine(wtaPath) + L" delegate"; const auto delegateAgent = _ResolveEffectiveDelegateAgent(globals); - if (!delegateAgent.empty()) - { - cmdline += L" --delegate-agent " + quoteArg(std::wstring_view{ delegateAgent }); - } const auto delegateModel = globals.DelegateModel(); - if (!delegateModel.empty()) - { - cmdline += L" --delegate-model " + quoteArg(std::wstring_view{ delegateModel }); - } - + // Pass agent config fields as JSON (same mechanism as the agent pane). + cmdline += Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{} /* agentId — not needed for delegate */, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{} /* acpModel */); // Pass CWD from the active pane. winrt::hstring activeCwd; @@ -1210,16 +1196,11 @@ namespace winrt::TerminalApp::implementation } if (!activeCwd.empty()) { - cmdline += L" --cwd " + quoteArg(std::wstring_view{ activeCwd }); + cmdline += L" --cwd " + QuoteArgForCommandLine(std::wstring_view{ activeCwd }); } // Append the prompt as a positional argument. - std::wstring escapedPrompt{ prompt }; - for (size_t pos = 0; (pos = escapedPrompt.find(L'"', pos)) != std::wstring::npos; pos += 2) - { - escapedPrompt.replace(pos, 1, L"\"\""); - } - cmdline += fmt::format(FMT_COMPILE(L" \"{}\""), escapedPrompt); + cmdline += L" " + QuoteArgForCommandLine(std::wstring_view{ prompt }); _agentPaneLog("launching: " + winrt::to_string(winrt::hstring{ cmdline })); @@ -1730,60 +1711,25 @@ namespace winrt::TerminalApp::implementation // _NotifyAgentTabChanged → tab_changed events. if (const auto stableId = tab->StableId(); !stableId.empty()) { - cmdline += fmt::format(FMT_COMPILE(L" --owner-tab-id \"{}\""), std::wstring_view{ stableId }); + cmdline += L" --owner-tab-id " + Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ stableId }); } const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); - if (!agentCliPath.empty()) - { - std::wstring s{ agentCliPath }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --agent \"{}\""), s); - } - - // See `_OpenOrReuseAgentPane` for the rationale: wta needs the - // canonical `acpAgent` setting value (not the expanded command - // line) to drive the session-management view's CLI filter. - if (const auto acpAgent = globals.AcpAgent(); !acpAgent.empty()) - { - std::wstring s{ acpAgent }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --agent-id \"{}\""), s); - } - + const auto acpAgent = globals.AcpAgent(); const auto delegateAgent = _ResolveEffectiveDelegateAgent(globals); - if (!delegateAgent.empty()) - { - std::wstring s{ delegateAgent }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --delegate-agent \"{}\""), s); - } - const auto delegateModel = globals.DelegateModel(); - if (!delegateModel.empty()) - { - std::wstring s{ delegateModel }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --delegate-model \"{}\""), s); - } - - // Pass the ACP model selection out-of-band so it works for adapter - // launches (claude/codex via npx) where --model can't be put on the - // adapter cmdline. wta sends this via ACP setSessionModel after - // handshake; for copilot/gemini it's redundant (their --model is - // already on the agent cmdline) but harmless. const auto acpModel = globals.AcpModel(); - if (!acpModel.empty()) - { - std::wstring s{ acpModel }; - for (size_t pos = 0; (pos = s.find(L'"', pos)) != std::wstring::npos; pos += 2) - s.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --acp-model \"{}\""), s); - } + + // Pass all agent-related config as a single JSON-encoded argument. + // This eliminates per-field hand-rolled escaping — the JSON library + // handles special characters, and only one correctly-quoted argument + // boundary is needed (via QuoteArgForCommandLine). + cmdline += Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{ acpAgent }, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{ acpModel }); if (!globals.AutoFixEnabled()) { @@ -2121,46 +2067,17 @@ namespace winrt::TerminalApp::implementation cmdline = std::wstring{ wtaPath }; const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); - if (!agentCliPath.empty()) - { - std::wstring agentStr{ agentCliPath }; - for (size_t pos = 0; (pos = agentStr.find(L'"', pos)) != std::wstring::npos; pos += 2) - agentStr.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --agent \"{}\""), agentStr); - } - - // Tell wta which `acpAgent` setting value produced this launch - // — wta needs the canonical id ("copilot" / "claude" / "codex" - // / "gemini" / "custom:…") to drive the session-management - // view's CLI filter, and parsing it back out of the expanded - // `--agent` command line is fragile (adapter launches expand - // to "npx -y …" and lose the agent's name). Passing it through - // here keeps a single source of truth. - if (const auto acpAgent = globals.AcpAgent(); !acpAgent.empty()) - { - std::wstring idStr{ acpAgent }; - for (size_t pos = 0; (pos = idStr.find(L'"', pos)) != std::wstring::npos; pos += 2) - idStr.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --agent-id \"{}\""), idStr); - } - + const auto acpAgent = globals.AcpAgent(); const auto delegateAgent = _ResolveEffectiveDelegateAgent(globals); - if (!delegateAgent.empty()) - { - std::wstring delegateStr{ delegateAgent }; - for (size_t pos = 0; (pos = delegateStr.find(L'"', pos)) != std::wstring::npos; pos += 2) - delegateStr.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --delegate-agent \"{}\""), delegateStr); - } - const auto delegateModel = globals.DelegateModel(); - if (!delegateModel.empty()) - { - std::wstring modelStr{ delegateModel }; - for (size_t pos = 0; (pos = modelStr.find(L'"', pos)) != std::wstring::npos; pos += 2) - modelStr.replace(pos, 1, L"\"\""); - cmdline += fmt::format(FMT_COMPILE(L" --delegate-model \"{}\""), modelStr); - } + + // Pass all agent-related config as a single JSON-encoded argument. + cmdline += Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{ acpAgent }, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{} /* acpModel — not used in this path */); if (!globals.AutoFixEnabled()) { diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index 476d50a54..43aaac819 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp @@ -10,6 +10,7 @@ #include "../inc/AgentRegistry.h" #include "../inc/AgentHooksStatus.h" #include "../inc/WtaProcess.h" +#include "../inc/QuoteArgForCommandLine.h" #include @@ -966,13 +967,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation std::string stdoutText; if (!wtaPath.empty()) { - // Quote-escape internal `"` per Windows CRT rules. - std::wstring escaped = agentCmdline; - for (size_t pos = 0; (pos = escaped.find(L'"', pos)) != std::wstring::npos; pos += 2) - { - escaped.replace(pos, 1, L"\"\""); - } - const std::wstring args = L"probe-models --agent \"" + escaped + L"\""; + // Use correct CommandLineToArgvW quoting for the agent argument. + const std::wstring args = L"probe-models --agent " + + ::Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ agentCmdline }); // 40s ceiling matches probe.rs's internal limits (npx // initialize 25s + new_session 10s + slack). Cached // adapters return in <2s. diff --git a/src/cascadia/inc/QuoteArgForCommandLine.h b/src/cascadia/inc/QuoteArgForCommandLine.h new file mode 100644 index 000000000..60d5e169b --- /dev/null +++ b/src/cascadia/inc/QuoteArgForCommandLine.h @@ -0,0 +1,146 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// QuoteArgForCommandLine.h +// +// Correct CommandLineToArgvW-compatible quoting for a single argument. +// Eliminates hand-rolled escaping throughout the codebase. Use this +// whenever building a commandline string for CreateProcess/ShellExecute. +// +// Pure Win32 + STL, no WinRT dependency. + +#pragma once + +#include +#include + +namespace Microsoft::Terminal::CommandLine +{ + // Quote a single argument for use in a Windows commandline string. + // The result is always wrapped in double quotes for unambiguous parsing + // by CommandLineToArgvW. Handles: + // - Backslashes before `"` are doubled (2n+1 backslashes + `"`) + // - Trailing backslashes before the closing `"` are doubled + // - All other characters are passed through literally + // + // NOTE: This is for argv[1..n] only. argv[0] (the program path) has + // different rules — backslashes are always literal, and `"` cannot be + // escaped inside it. For argv[0], simply wrap in quotes if needed and + // reject paths containing `"`. + inline std::wstring QuoteArgForCommandLine(std::wstring_view arg) + { + std::wstring result; + // Reserve: arg size + quotes + some escaping headroom + result.reserve(arg.size() + 8); + result.push_back(L'"'); + + size_t backslashes = 0; + for (const auto ch : arg) + { + if (ch == L'\\') + { + ++backslashes; + } + else if (ch == L'"') + { + // Double the accumulated backslashes, then emit \" + result.append(backslashes * 2 + 1, L'\\'); + result.push_back(L'"'); + backslashes = 0; + } + else + { + // Flush any accumulated backslashes as-is + result.append(backslashes, L'\\'); + backslashes = 0; + result.push_back(ch); + } + } + // Trailing backslashes must be doubled (they precede the closing `"`) + result.append(backslashes * 2, L'\\'); + result.push_back(L'"'); + + return result; + } + + // Build a JSON-encoded `--agent-config` argument from the given fields. + // Returns the full fragment: ` --agent-config ""` + // Uses JsonCpp for serialization and QuoteArgForCommandLine for the + // single argument boundary. + // + // Usage: + // cmdline += BuildAgentConfigArg(agentCli, agentId, delegateAgent, + // delegateModel, acpModel); + // + // Any empty field is omitted from the JSON (the Rust side uses + // Option and falls back to defaults for missing fields). + inline std::wstring BuildAgentConfigArg( + std::wstring_view agent, + std::wstring_view agentId, + std::wstring_view delegateAgent, + std::wstring_view delegateModel, + std::wstring_view acpModel) + { + // Build a compact JSON object with only non-empty fields. + // We use manual JSON construction to avoid pulling in JsonCpp here + // (this header is used in both TerminalApp and TerminalSettingsEditor). + // The JSON spec is simple enough for known-safe field names: only the + // VALUES need escaping, and we do it correctly per RFC 8259. + auto jsonEscapeValue = [](std::wstring_view val) -> std::wstring { + std::wstring out; + out.reserve(val.size() + 4); + for (const auto ch : val) + { + switch (ch) + { + case L'"': out += L"\\\""; break; + case L'\\': out += L"\\\\"; break; + case L'\b': out += L"\\b"; break; + case L'\f': out += L"\\f"; break; + case L'\n': out += L"\\n"; break; + case L'\r': out += L"\\r"; break; + case L'\t': out += L"\\t"; break; + default: + if (ch < 0x20) + { + wchar_t buf[8]; + swprintf_s(buf, L"\\u%04x", static_cast(ch)); + out += buf; + } + else + { + out.push_back(ch); + } + break; + } + } + return out; + }; + + std::wstring json = L"{"; + bool first = true; + + auto appendField = [&](const wchar_t* key, std::wstring_view val) { + if (val.empty()) + return; + if (!first) + json += L','; + first = false; + json += L'"'; + json += key; + json += L"\":\""; + json += jsonEscapeValue(val); + json += L'"'; + }; + + appendField(L"agent", agent); + appendField(L"agentId", agentId); + appendField(L"delegateAgent", delegateAgent); + appendField(L"delegateModel", delegateModel); + appendField(L"acpModel", acpModel); + + json += L'}'; + + return L" --agent-config " + QuoteArgForCommandLine(json); + } +} diff --git a/tools/wta/src/cmdline.rs b/tools/wta/src/cmdline.rs index 3390eba92..c585fa43b 100644 --- a/tools/wta/src/cmdline.rs +++ b/tools/wta/src/cmdline.rs @@ -322,4 +322,87 @@ mod tests { &["-y", "@zed-industries/claude-code-acp"], ); } + + // ── JSON-in-commandline-arg round-trip ────────────────────────── + // These simulate the WT→WTA flow: WT serializes agent config as JSON, + // passes it as a single `--agent-config` argument. We verify the JSON + // survives the commandline quoting + CommandLineToArgvW round-trip and + // can be deserialized back to the original structured data. + + #[test] + fn json_arg_simple() { + let json = r#"{"agent":"copilot --acp --stdio","agentId":"copilot"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_with_quotes_in_value() { + // Agent command contains quotes (e.g. a path with spaces that was quoted) + let json = r#"{"agent":"\"C:\\Program Files\\agent.exe\" --acp","agentId":"custom"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_with_backslashes() { + // Windows paths with backslashes inside JSON values + let json = r#"{"agent":"C:\\Users\\test\\copilot.exe --acp --stdio","agentId":"copilot"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_all_fields() { + let json = r#"{"agent":"copilot --acp --stdio","agentId":"copilot","delegateAgent":"codex","delegateModel":"gpt-4o","acpModel":"claude-sonnet"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_unicode_values() { + let json = r#"{"agent":"copilot --acp","agentId":"自定义代理"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_with_special_chars_in_model() { + // Model name with characters that could trip up naive escaping + let json = r#"{"agent":"copilot","acpModel":"org/model-name:v1.0 (beta)"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_adversarial_injection_attempt() { + // Attacker tries to break out of JSON arg to inject --no-autofix + let json = r#"{"agent":"evil\" --no-autofix --agent \"pwned"}"#; + assert_roundtrip("wta.exe", &["--agent-config", json]); + } + + #[test] + fn json_arg_deserialization_roundtrip() { + // Full end-to-end: build commandline with JSON arg, parse via OS, + // then deserialize the JSON to verify structured data survives. + use serde_json::Value; + + let config = serde_json::json!({ + "agent": "copilot --acp --stdio", + "agentId": "copilot", + "delegateAgent": "codex --model gpt-4", + "delegateModel": "gpt-4o", + "acpModel": "claude-3.5-sonnet" + }); + let json_str = serde_json::to_string(&config).unwrap(); + + let args = vec![ + "--agent-config".to_string(), + json_str.clone(), + ]; + let cmdline = build_wt_commandline("wta.exe", &args).unwrap(); + let parsed = parse_via_os(&cmdline); + + // parsed[0] = "wta.exe", parsed[1] = "--agent-config", parsed[2] = json + assert_eq!(parsed.len(), 3); + assert_eq!(parsed[1], "--agent-config"); + + // The JSON must deserialize back to the same structured data + let recovered: Value = serde_json::from_str(&parsed[2]).unwrap(); + assert_eq!(recovered, config); + } } diff --git a/tools/wta/src/main.rs b/tools/wta/src/main.rs index 08fa12799..61d64b593 100644 --- a/tools/wta/src/main.rs +++ b/tools/wta/src/main.rs @@ -103,6 +103,18 @@ fn normalize_locale(locale: &str) -> String { // ─── CLI Definition ───────────────────────────────────────────────────────── +/// JSON-encoded agent configuration passed via `--agent-config`. +/// All fields are optional; missing fields fall back to individual CLI args. +#[derive(Debug, serde::Deserialize)] +#[serde(rename_all = "camelCase")] +struct AgentConfig { + agent: Option, + agent_id: Option, + delegate_agent: Option, + delegate_model: Option, + acp_model: Option, +} + #[derive(Parser, Debug)] #[command( name = "wta", @@ -151,6 +163,22 @@ struct Cli { #[arg(long)] delegate_model: Option, + /// JSON-encoded agent configuration. When present, fields from this + /// JSON object override the corresponding individual CLI args above. + /// This eliminates hand-rolled commandline escaping on the host side — + /// the JSON library handles all special characters, and only one + /// correctly-quoted argument boundary is needed. + /// + /// Expected shape: + /// ```json + /// {"agent":"copilot --acp --stdio","agentId":"copilot", + /// "delegateAgent":"codex","delegateModel":"gpt-4","acpModel":"..."} + /// ``` + /// All fields are optional; missing fields fall back to the + /// corresponding individual CLI arg or its default. + #[arg(long, hide = true)] + agent_config: Option, + /// Disable auto-fix on command failure #[arg(long)] no_autofix: bool, @@ -350,6 +378,10 @@ enum Command { /// Working directory for the delegate agent tab #[arg(long)] cwd: Option, + + /// JSON-encoded agent configuration (same as top-level --agent-config) + #[arg(long, hide = true)] + agent_config: Option, }, /// Manage the wt-agent-hooks bridge for supported CLI agents @@ -441,7 +473,31 @@ async fn main() -> Result<()> { rust_i18n::set_locale(&normalize_locale(&locale)); } - let cli = Cli::parse(); + let mut cli = Cli::parse(); + + // If --agent-config is present, overlay its fields onto the individual + // CLI args. This is the secure path: the host serializes all agent + // config as a single JSON blob, eliminating per-field commandline + // escaping. Invalid JSON is a hard error (don't silently fall back). + if let Some(ref config_json) = cli.agent_config { + let config: AgentConfig = serde_json::from_str(config_json) + .context("--agent-config: invalid JSON")?; + if let Some(agent) = config.agent { + cli.agent = agent; + } + if config.agent_id.is_some() { + cli.agent_id = config.agent_id; + } + if config.delegate_agent.is_some() { + cli.delegate_agent = config.delegate_agent; + } + if config.delegate_model.is_some() { + cli.delegate_model = config.delegate_model; + } + if config.acp_model.is_some() { + cli.acp_model = config.acp_model; + } + } // Legacy flags first (backward compat) if cli.test_pipe { @@ -650,11 +706,26 @@ async fn main() -> Result<()> { // ── Delegate prompt to new tab agent ── Some(Command::Delegate { prompt, - agent, - delegate_agent, - delegate_model, + mut agent, + mut delegate_agent, + mut delegate_model, cwd, + agent_config, }) => { + // Apply JSON config overlay (same as top-level path). + if let Some(ref config_json) = agent_config { + let config: AgentConfig = serde_json::from_str(config_json) + .context("delegate --agent-config: invalid JSON")?; + if let Some(a) = config.agent { + agent = a; + } + if config.delegate_agent.is_some() { + delegate_agent = config.delegate_agent; + } + if config.delegate_model.is_some() { + delegate_model = config.delegate_model; + } + } run_delegate(&prompt, &agent, delegate_agent.as_deref(), delegate_model.as_deref(), cwd.as_deref()).await } From 221c12210fc268bf11017ea4b3616aa598bc42c6 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Thu, 21 May 2026 23:26:03 +0800 Subject: [PATCH 03/23] fix: correct misleading comments about JsonCpp usage BuildAgentConfigArg uses manual RFC 8259 JSON construction, not JsonCpp. Updated comments to reflect the actual implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 6 +++--- src/cascadia/inc/QuoteArgForCommandLine.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 4f3a77610..1cd71ac1c 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1721,9 +1721,9 @@ namespace winrt::TerminalApp::implementation const auto acpModel = globals.AcpModel(); // Pass all agent-related config as a single JSON-encoded argument. - // This eliminates per-field hand-rolled escaping — the JSON library - // handles special characters, and only one correctly-quoted argument - // boundary is needed (via QuoteArgForCommandLine). + // This eliminates per-field hand-rolled escaping — BuildAgentConfigArg + // performs RFC 8259 JSON encoding internally, and only one correctly- + // quoted argument boundary is needed (via QuoteArgForCommandLine). cmdline += Microsoft::Terminal::CommandLine::BuildAgentConfigArg( std::wstring_view{ agentCliPath }, std::wstring_view{ acpAgent }, diff --git a/src/cascadia/inc/QuoteArgForCommandLine.h b/src/cascadia/inc/QuoteArgForCommandLine.h index 60d5e169b..2d6dcaec4 100644 --- a/src/cascadia/inc/QuoteArgForCommandLine.h +++ b/src/cascadia/inc/QuoteArgForCommandLine.h @@ -65,8 +65,9 @@ namespace Microsoft::Terminal::CommandLine // Build a JSON-encoded `--agent-config` argument from the given fields. // Returns the full fragment: ` --agent-config ""` - // Uses JsonCpp for serialization and QuoteArgForCommandLine for the - // single argument boundary. + // Uses manual RFC 8259-compliant JSON construction (no external JSON + // library dependency) and QuoteArgForCommandLine for the single + // argument boundary. // // Usage: // cmdline += BuildAgentConfigArg(agentCli, agentId, delegateAgent, From ff5e4e2fca18750af63c5afcd530bc354dd037f5 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Thu, 21 May 2026 23:35:46 +0800 Subject: [PATCH 04/23] chore: trigger re-review From ef17c67bb6e816ae38e3a45d6433311749708c72 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Thu, 21 May 2026 23:43:43 +0800 Subject: [PATCH 05/23] fix: address Copilot review round 2 - Add and includes for self-contained header - Reject embedded NUL in QuoteArgForCommandLine (prevents silent truncation) - Add QuoteProgramPath() helper for argv[0] (simpler rules, rejects quotes) - Use QuoteProgramPath for wta executable path in delegate launch - Fix Rust docstring: 'JSON encoding' not 'JSON library' - Extract parse_agent_config() helper to eliminate overlay duplication Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 3 +- src/cascadia/inc/QuoteArgForCommandLine.h | 41 +++++++++++++- tools/wta/src/main.rs | 67 ++++++++++++++++------- 3 files changed, 86 insertions(+), 25 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 1cd71ac1c..5725f49ec 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1165,9 +1165,10 @@ namespace winrt::TerminalApp::implementation const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); using Microsoft::Terminal::CommandLine::QuoteArgForCommandLine; + using Microsoft::Terminal::CommandLine::QuoteProgramPath; // Build: wta delegate --agent-config --cwd "" - std::wstring cmdline = QuoteArgForCommandLine(wtaPath) + L" delegate"; + std::wstring cmdline = QuoteProgramPath(wtaPath) + L" delegate"; const auto delegateAgent = _ResolveEffectiveDelegateAgent(globals); const auto delegateModel = globals.DelegateModel(); diff --git a/src/cascadia/inc/QuoteArgForCommandLine.h b/src/cascadia/inc/QuoteArgForCommandLine.h index 2d6dcaec4..c59133e63 100644 --- a/src/cascadia/inc/QuoteArgForCommandLine.h +++ b/src/cascadia/inc/QuoteArgForCommandLine.h @@ -11,6 +11,8 @@ #pragma once +#include +#include #include #include @@ -21,12 +23,12 @@ namespace Microsoft::Terminal::CommandLine // by CommandLineToArgvW. Handles: // - Backslashes before `"` are doubled (2n+1 backslashes + `"`) // - Trailing backslashes before the closing `"` are doubled + // - Embedded NUL characters are rejected (they would truncate the + // commandline at the OS level) // - All other characters are passed through literally // // NOTE: This is for argv[1..n] only. argv[0] (the program path) has - // different rules — backslashes are always literal, and `"` cannot be - // escaped inside it. For argv[0], simply wrap in quotes if needed and - // reject paths containing `"`. + // different rules — use QuoteProgramPath() for that. inline std::wstring QuoteArgForCommandLine(std::wstring_view arg) { std::wstring result; @@ -37,6 +39,13 @@ namespace Microsoft::Terminal::CommandLine size_t backslashes = 0; for (const auto ch : arg) { + if (ch == L'\0') + { + // Embedded NUL would silently truncate the commandline. + // Reject it deterministically rather than producing a + // subtly broken commandline. + throw std::invalid_argument("QuoteArgForCommandLine: embedded NUL in argument"); + } if (ch == L'\\') { ++backslashes; @@ -63,6 +72,32 @@ namespace Microsoft::Terminal::CommandLine return result; } + // Quote a program path (argv[0]) for use in a Windows commandline string. + // argv[0] has simpler rules than argv[1..n]: backslashes are always literal + // and `"` cannot be escaped inside it. We wrap in quotes (for paths with + // spaces) and reject paths containing `"` (which are invalid on Windows + // file systems anyway). + inline std::wstring QuoteProgramPath(std::wstring_view path) + { + for (const auto ch : path) + { + if (ch == L'"') + { + throw std::invalid_argument("QuoteProgramPath: path contains '\"'"); + } + if (ch == L'\0') + { + throw std::invalid_argument("QuoteProgramPath: path contains embedded NUL"); + } + } + std::wstring result; + result.reserve(path.size() + 2); + result.push_back(L'"'); + result.append(path); + result.push_back(L'"'); + return result; + } + // Build a JSON-encoded `--agent-config` argument from the given fields. // Returns the full fragment: ` --agent-config ""` // Uses manual RFC 8259-compliant JSON construction (no external JSON diff --git a/tools/wta/src/main.rs b/tools/wta/src/main.rs index 61d64b593..146ed3ff1 100644 --- a/tools/wta/src/main.rs +++ b/tools/wta/src/main.rs @@ -115,6 +115,35 @@ struct AgentConfig { acp_model: Option, } +/// Parsed fields from an `--agent-config` JSON blob, ready to overlay onto +/// individual CLI args. +struct AgentConfigOverlay { + agent: Option, + agent_id: Option, + delegate_agent: Option, + delegate_model: Option, + acp_model: Option, +} + +/// Parse a JSON `--agent-config` value and return the overlay fields. +/// Returns `None` if `config_json` is `None`; errors on invalid JSON. +fn parse_agent_config(config_json: Option<&str>) -> anyhow::Result> { + match config_json { + None => Ok(None), + Some(json) => { + let config: AgentConfig = serde_json::from_str(json) + .context("--agent-config: invalid JSON")?; + Ok(Some(AgentConfigOverlay { + agent: config.agent, + agent_id: config.agent_id, + delegate_agent: config.delegate_agent, + delegate_model: config.delegate_model, + acp_model: config.acp_model, + })) + } + } +} + #[derive(Parser, Debug)] #[command( name = "wta", @@ -166,7 +195,7 @@ struct Cli { /// JSON-encoded agent configuration. When present, fields from this /// JSON object override the corresponding individual CLI args above. /// This eliminates hand-rolled commandline escaping on the host side — - /// the JSON library handles all special characters, and only one + /// JSON encoding handles all special characters, and only one /// correctly-quoted argument boundary is needed. /// /// Expected shape: @@ -479,23 +508,21 @@ async fn main() -> Result<()> { // CLI args. This is the secure path: the host serializes all agent // config as a single JSON blob, eliminating per-field commandline // escaping. Invalid JSON is a hard error (don't silently fall back). - if let Some(ref config_json) = cli.agent_config { - let config: AgentConfig = serde_json::from_str(config_json) - .context("--agent-config: invalid JSON")?; - if let Some(agent) = config.agent { + if let Some(overlay) = parse_agent_config(cli.agent_config.as_deref())? { + if let Some(agent) = overlay.agent { cli.agent = agent; } - if config.agent_id.is_some() { - cli.agent_id = config.agent_id; + if overlay.agent_id.is_some() { + cli.agent_id = overlay.agent_id; } - if config.delegate_agent.is_some() { - cli.delegate_agent = config.delegate_agent; + if overlay.delegate_agent.is_some() { + cli.delegate_agent = overlay.delegate_agent; } - if config.delegate_model.is_some() { - cli.delegate_model = config.delegate_model; + if overlay.delegate_model.is_some() { + cli.delegate_model = overlay.delegate_model; } - if config.acp_model.is_some() { - cli.acp_model = config.acp_model; + if overlay.acp_model.is_some() { + cli.acp_model = overlay.acp_model; } } @@ -713,17 +740,15 @@ async fn main() -> Result<()> { agent_config, }) => { // Apply JSON config overlay (same as top-level path). - if let Some(ref config_json) = agent_config { - let config: AgentConfig = serde_json::from_str(config_json) - .context("delegate --agent-config: invalid JSON")?; - if let Some(a) = config.agent { + if let Some(overlay) = parse_agent_config(agent_config.as_deref())? { + if let Some(a) = overlay.agent { agent = a; } - if config.delegate_agent.is_some() { - delegate_agent = config.delegate_agent; + if overlay.delegate_agent.is_some() { + delegate_agent = overlay.delegate_agent; } - if config.delegate_model.is_some() { - delegate_model = config.delegate_model; + if overlay.delegate_model.is_some() { + delegate_model = overlay.delegate_model; } } run_delegate(&prompt, &agent, delegate_agent.as_deref(), delegate_model.as_deref(), cwd.as_deref()).await From ad520e5e96b84febf341cdac971cdb98e0f0e01c Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Thu, 21 May 2026 23:52:57 +0800 Subject: [PATCH 06/23] chore: request re-review Addresses all 5 Copilot review comments from round 2. From 92e663d3036d409cdeb11434213d8f5950b9165c Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 00:06:15 +0800 Subject: [PATCH 07/23] fix: make QuoteArgForCommandLine non-throwing (address round 3) - Convert QuoteArgForCommandLine and QuoteProgramPath to return std::optional instead of throwing on invalid input - Update all callers (TerminalPage, AIAgentsViewModel) to handle the optional gracefully with early-return/skip patterns - Fix shell_manager.rs comment about error propagation behavior - Remove include (no longer needed) This prevents app-terminating exceptions from settings/user text containing embedded NUL, satisfying the non-throwing API requirement for UI code paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 22 +++++-- .../AIAgentsViewModel.cpp | 15 +++-- src/cascadia/inc/QuoteArgForCommandLine.h | 59 +++++++++++-------- tools/wta/src/shell/shell_manager.rs | 4 +- 4 files changed, 65 insertions(+), 35 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 5725f49ec..a046be098 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1168,7 +1168,12 @@ namespace winrt::TerminalApp::implementation using Microsoft::Terminal::CommandLine::QuoteProgramPath; // Build: wta delegate --agent-config --cwd "" - std::wstring cmdline = QuoteProgramPath(wtaPath) + L" delegate"; + auto quotedPath = QuoteProgramPath(wtaPath); + if (!quotedPath) + { + return; // Invalid WTA path (contains NUL or quote) — cannot launch. + } + std::wstring cmdline = *quotedPath + L" delegate"; const auto delegateAgent = _ResolveEffectiveDelegateAgent(globals); const auto delegateModel = globals.DelegateModel(); @@ -1197,11 +1202,17 @@ namespace winrt::TerminalApp::implementation } if (!activeCwd.empty()) { - cmdline += L" --cwd " + QuoteArgForCommandLine(std::wstring_view{ activeCwd }); + if (auto q = QuoteArgForCommandLine(std::wstring_view{ activeCwd })) + { + cmdline += L" --cwd " + *q; + } } // Append the prompt as a positional argument. - cmdline += L" " + QuoteArgForCommandLine(std::wstring_view{ prompt }); + if (auto q = QuoteArgForCommandLine(std::wstring_view{ prompt })) + { + cmdline += L" " + *q; + } _agentPaneLog("launching: " + winrt::to_string(winrt::hstring{ cmdline })); @@ -1712,7 +1723,10 @@ namespace winrt::TerminalApp::implementation // _NotifyAgentTabChanged → tab_changed events. if (const auto stableId = tab->StableId(); !stableId.empty()) { - cmdline += L" --owner-tab-id " + Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ stableId }); + if (auto q = Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ stableId })) + { + cmdline += L" --owner-tab-id " + *q; + } } const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index 43aaac819..f4fcdefd1 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp @@ -968,12 +968,15 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation if (!wtaPath.empty()) { // Use correct CommandLineToArgvW quoting for the agent argument. - const std::wstring args = L"probe-models --agent " + - ::Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ agentCmdline }); - // 40s ceiling matches probe.rs's internal limits (npx - // initialize 25s + new_session 10s + slack). Cached - // adapters return in <2s. - stdoutText = ::Microsoft::Terminal::WtaProcess::RunWtaCaptureStdout(wtaPath, args, 40'000); + auto quoted = ::Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ agentCmdline }); + if (quoted) + { + const std::wstring args = L"probe-models --agent " + *quoted; + // 40s ceiling matches probe.rs's internal limits (npx + // initialize 25s + new_session 10s + slack). Cached + // adapters return in <2s. + stdoutText = ::Microsoft::Terminal::WtaProcess::RunWtaCaptureStdout(wtaPath, args, 40'000); + } } std::vector parsed; diff --git a/src/cascadia/inc/QuoteArgForCommandLine.h b/src/cascadia/inc/QuoteArgForCommandLine.h index c59133e63..94d596043 100644 --- a/src/cascadia/inc/QuoteArgForCommandLine.h +++ b/src/cascadia/inc/QuoteArgForCommandLine.h @@ -7,12 +7,13 @@ // Eliminates hand-rolled escaping throughout the codebase. Use this // whenever building a commandline string for CreateProcess/ShellExecute. // -// Pure Win32 + STL, no WinRT dependency. +// Pure Win32 + STL, no WinRT dependency. Non-throwing API — returns +// empty optional on invalid input (embedded NUL, invalid program path). #pragma once #include -#include +#include #include #include @@ -23,29 +24,31 @@ namespace Microsoft::Terminal::CommandLine // by CommandLineToArgvW. Handles: // - Backslashes before `"` are doubled (2n+1 backslashes + `"`) // - Trailing backslashes before the closing `"` are doubled - // - Embedded NUL characters are rejected (they would truncate the - // commandline at the OS level) // - All other characters are passed through literally // + // Returns std::nullopt if the argument contains embedded NUL (which + // would silently truncate the commandline at the OS level). + // // NOTE: This is for argv[1..n] only. argv[0] (the program path) has // different rules — use QuoteProgramPath() for that. - inline std::wstring QuoteArgForCommandLine(std::wstring_view arg) + inline std::optional QuoteArgForCommandLine(std::wstring_view arg) noexcept { + // Reject embedded NUL — it would truncate the commandline. + for (const auto ch : arg) + { + if (ch == L'\0') + { + return std::nullopt; + } + } + std::wstring result; - // Reserve: arg size + quotes + some escaping headroom result.reserve(arg.size() + 8); result.push_back(L'"'); size_t backslashes = 0; for (const auto ch : arg) { - if (ch == L'\0') - { - // Embedded NUL would silently truncate the commandline. - // Reject it deterministically rather than producing a - // subtly broken commandline. - throw std::invalid_argument("QuoteArgForCommandLine: embedded NUL in argument"); - } if (ch == L'\\') { ++backslashes; @@ -75,19 +78,17 @@ namespace Microsoft::Terminal::CommandLine // Quote a program path (argv[0]) for use in a Windows commandline string. // argv[0] has simpler rules than argv[1..n]: backslashes are always literal // and `"` cannot be escaped inside it. We wrap in quotes (for paths with - // spaces) and reject paths containing `"` (which are invalid on Windows - // file systems anyway). - inline std::wstring QuoteProgramPath(std::wstring_view path) + // spaces) and reject paths containing `"` or NUL (which are invalid on + // Windows file systems anyway). + // + // Returns std::nullopt if the path contains `"` or embedded NUL. + inline std::optional QuoteProgramPath(std::wstring_view path) noexcept { for (const auto ch : path) { - if (ch == L'"') + if (ch == L'"' || ch == L'\0') { - throw std::invalid_argument("QuoteProgramPath: path contains '\"'"); - } - if (ch == L'\0') - { - throw std::invalid_argument("QuoteProgramPath: path contains embedded NUL"); + return std::nullopt; } } std::wstring result; @@ -110,6 +111,9 @@ namespace Microsoft::Terminal::CommandLine // // Any empty field is omitted from the JSON (the Rust side uses // Option and falls back to defaults for missing fields). + // + // Returns empty string on failure (invalid input containing NUL). + // Callers should check for empty return if they need error handling. inline std::wstring BuildAgentConfigArg( std::wstring_view agent, std::wstring_view agentId, @@ -122,6 +126,8 @@ namespace Microsoft::Terminal::CommandLine // (this header is used in both TerminalApp and TerminalSettingsEditor). // The JSON spec is simple enough for known-safe field names: only the // VALUES need escaping, and we do it correctly per RFC 8259. + // NUL characters in values are escaped as \u0000 (valid JSON), so + // they don't pose a truncation risk in the final commandline. auto jsonEscapeValue = [](std::wstring_view val) -> std::wstring { std::wstring out; out.reserve(val.size() + 4); @@ -177,6 +183,13 @@ namespace Microsoft::Terminal::CommandLine json += L'}'; - return L" --agent-config " + QuoteArgForCommandLine(json); + // The JSON blob itself won't contain NUL (all control chars are + // escaped above), so QuoteArgForCommandLine won't fail here. + auto quoted = QuoteArgForCommandLine(json); + if (!quoted) + { + return {}; + } + return L" --agent-config " + *quoted; } } diff --git a/tools/wta/src/shell/shell_manager.rs b/tools/wta/src/shell/shell_manager.rs index b2e83a3d5..f2cb2575d 100644 --- a/tools/wta/src/shell/shell_manager.rs +++ b/tools/wta/src/shell/shell_manager.rs @@ -118,8 +118,8 @@ impl ShellManager { // Build the commandline string for WT pane creation. Returns an // error if the agent-supplied command is unrepresentable (e.g. - // contains a literal `"`); propagate so the caller can surface it - // to the agent instead of crashing the wta process. + // contains a literal `"`); the caller catches this and falls back + // to create_terminal_local. let cmdline = build_wt_commandline(&config.command, &config.args)?; // Create a new tab in WT with the command From e519a1947dd8d0fea7140c5883995554b11fceae Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 00:10:33 +0800 Subject: [PATCH 08/23] chore: trigger review From 9213afee6e318c5e4f3f01c0adf7b224d94fa356 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 00:21:15 +0800 Subject: [PATCH 09/23] fix: address round 4 Copilot review comments - Make delegate prompt quoting failure a hard early-return (not silent) - Fix BuildAgentConfigArg doc: NUL is escaped by JSON, not rejected Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 8 +++++--- src/cascadia/inc/QuoteArgForCommandLine.h | 5 +++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index a046be098..c249d8dd0 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1208,11 +1208,13 @@ namespace winrt::TerminalApp::implementation } } - // Append the prompt as a positional argument. - if (auto q = QuoteArgForCommandLine(std::wstring_view{ prompt })) + // Append the prompt as a positional argument (required by wta delegate). + auto quotedPrompt = QuoteArgForCommandLine(std::wstring_view{ prompt }); + if (!quotedPrompt) { - cmdline += L" " + *q; + return; // Prompt contains embedded NUL — cannot safely launch. } + cmdline += L" " + *quotedPrompt; _agentPaneLog("launching: " + winrt::to_string(winrt::hstring{ cmdline })); diff --git a/src/cascadia/inc/QuoteArgForCommandLine.h b/src/cascadia/inc/QuoteArgForCommandLine.h index 94d596043..0c01d1479 100644 --- a/src/cascadia/inc/QuoteArgForCommandLine.h +++ b/src/cascadia/inc/QuoteArgForCommandLine.h @@ -112,8 +112,9 @@ namespace Microsoft::Terminal::CommandLine // Any empty field is omitted from the JSON (the Rust side uses // Option and falls back to defaults for missing fields). // - // Returns empty string on failure (invalid input containing NUL). - // Callers should check for empty return if they need error handling. + // All control characters in field values (including NUL) are escaped + // per RFC 8259, so the resulting JSON blob is always valid and safe + // for commandline transport. inline std::wstring BuildAgentConfigArg( std::wstring_view agent, std::wstring_view agentId, From dfdae6404a3b872c40162aa4d391cfeacf4be402 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 00:27:04 +0800 Subject: [PATCH 10/23] chore: trigger review From b44b2e38a13f600b9d547889937fb3c6d60fb7a9 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 00:34:19 +0800 Subject: [PATCH 11/23] fix: quote wta path with QuoteProgramPath in all launch paths - Use QuoteProgramPath(wtaPath) instead of raw path in agent pane creation (_AutoCreateHiddenAgentPane, _OpenOrReuseAgentPane) - Add diagnostic logging on all quoting failures for discoverability - Fixes potential argv[0] mis-parsing when wta is in a path with spaces Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index c249d8dd0..d9951f363 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1171,6 +1171,7 @@ namespace winrt::TerminalApp::implementation auto quotedPath = QuoteProgramPath(wtaPath); if (!quotedPath) { + _agentPaneLog("ABORT: wta path contains invalid characters, cannot delegate"); return; // Invalid WTA path (contains NUL or quote) — cannot launch. } std::wstring cmdline = *quotedPath + L" delegate"; @@ -1212,6 +1213,7 @@ namespace winrt::TerminalApp::implementation auto quotedPrompt = QuoteArgForCommandLine(std::wstring_view{ prompt }); if (!quotedPrompt) { + _agentPaneLog("ABORT: delegate prompt contains invalid characters"); return; // Prompt contains embedded NUL — cannot safely launch. } cmdline += L" " + *quotedPrompt; @@ -1715,7 +1717,13 @@ namespace winrt::TerminalApp::implementation } const auto& globals = _settings.GlobalSettings(); - std::wstring cmdline = std::wstring{ wtaPath }; + auto quotedWtaPath = Microsoft::Terminal::CommandLine::QuoteProgramPath(wtaPath); + if (!quotedWtaPath) + { + _agentPaneLog("ABORT: wta path contains invalid characters"); + return; + } + std::wstring cmdline = *quotedWtaPath; // Tell wta which tab owns its pane up-front (passed as a hidden CLI // arg). wta seeds app_state.tab_id from this before any ACP work @@ -2081,7 +2089,13 @@ namespace winrt::TerminalApp::implementation // Build the wta command line (single-process TUI mode, no subcommand). if (const auto wtaPath = _DetectWtaPath(); !wtaPath.empty()) { - cmdline = std::wstring{ wtaPath }; + auto quotedWtaPath = Microsoft::Terminal::CommandLine::QuoteProgramPath(wtaPath); + if (!quotedWtaPath) + { + _agentPaneLog("ABORT: wta path contains invalid characters"); + return; + } + cmdline = *quotedWtaPath; const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); const auto acpAgent = globals.AcpAgent(); From 7b7bcc7e59740eee7c6534038314b6ee2b89da08 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 00:40:17 +0800 Subject: [PATCH 12/23] chore: trigger review From 183e02d1b285e2438ff608090de37c10b6d4065f Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 00:47:59 +0800 Subject: [PATCH 13/23] fix: make BuildAgentConfigArg return std::optional - BuildAgentConfigArg now returns std::optional to explicitly signal failure instead of returning empty string - All 3 call sites handle nullopt with logging + early-return - Prevents silently proceeding with a partial command line Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 60 ++++++++++++++++------- src/cascadia/inc/QuoteArgForCommandLine.h | 7 +-- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index d9951f363..e02d10bfe 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1180,12 +1180,20 @@ namespace winrt::TerminalApp::implementation const auto delegateModel = globals.DelegateModel(); // Pass agent config fields as JSON (same mechanism as the agent pane). - cmdline += Microsoft::Terminal::CommandLine::BuildAgentConfigArg( - std::wstring_view{ agentCliPath }, - std::wstring_view{} /* agentId — not needed for delegate */, - std::wstring_view{ delegateAgent }, - std::wstring_view{ delegateModel }, - std::wstring_view{} /* acpModel */); + if (auto agentArg = Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{} /* agentId — not needed for delegate */, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{} /* acpModel */)) + { + cmdline += *agentArg; + } + else + { + _agentPaneLog("ABORT: failed to build agent config for delegate"); + return; + } // Pass CWD from the active pane. winrt::hstring activeCwd; @@ -1749,12 +1757,20 @@ namespace winrt::TerminalApp::implementation // This eliminates per-field hand-rolled escaping — BuildAgentConfigArg // performs RFC 8259 JSON encoding internally, and only one correctly- // quoted argument boundary is needed (via QuoteArgForCommandLine). - cmdline += Microsoft::Terminal::CommandLine::BuildAgentConfigArg( - std::wstring_view{ agentCliPath }, - std::wstring_view{ acpAgent }, - std::wstring_view{ delegateAgent }, - std::wstring_view{ delegateModel }, - std::wstring_view{ acpModel }); + if (auto agentArg = Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{ acpAgent }, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{ acpModel })) + { + cmdline += *agentArg; + } + else + { + _agentPaneLog("ABORT: failed to build agent config"); + return; + } if (!globals.AutoFixEnabled()) { @@ -2103,12 +2119,20 @@ namespace winrt::TerminalApp::implementation const auto delegateModel = globals.DelegateModel(); // Pass all agent-related config as a single JSON-encoded argument. - cmdline += Microsoft::Terminal::CommandLine::BuildAgentConfigArg( - std::wstring_view{ agentCliPath }, - std::wstring_view{ acpAgent }, - std::wstring_view{ delegateAgent }, - std::wstring_view{ delegateModel }, - std::wstring_view{} /* acpModel — not used in this path */); + if (auto agentArg = Microsoft::Terminal::CommandLine::BuildAgentConfigArg( + std::wstring_view{ agentCliPath }, + std::wstring_view{ acpAgent }, + std::wstring_view{ delegateAgent }, + std::wstring_view{ delegateModel }, + std::wstring_view{} /* acpModel — not used in this path */)) + { + cmdline += *agentArg; + } + else + { + _agentPaneLog("ABORT: failed to build agent config"); + return; + } if (!globals.AutoFixEnabled()) { diff --git a/src/cascadia/inc/QuoteArgForCommandLine.h b/src/cascadia/inc/QuoteArgForCommandLine.h index 0c01d1479..4a03e94ff 100644 --- a/src/cascadia/inc/QuoteArgForCommandLine.h +++ b/src/cascadia/inc/QuoteArgForCommandLine.h @@ -114,8 +114,9 @@ namespace Microsoft::Terminal::CommandLine // // All control characters in field values (including NUL) are escaped // per RFC 8259, so the resulting JSON blob is always valid and safe - // for commandline transport. - inline std::wstring BuildAgentConfigArg( + // for commandline transport. Returns std::nullopt on (unlikely) + // quoting failure; callers should log and abort. + inline std::optional BuildAgentConfigArg( std::wstring_view agent, std::wstring_view agentId, std::wstring_view delegateAgent, @@ -189,7 +190,7 @@ namespace Microsoft::Terminal::CommandLine auto quoted = QuoteArgForCommandLine(json); if (!quoted) { - return {}; + return std::nullopt; } return L" --agent-config " + *quoted; } From 59b62da43c35246011699e25ee288a4b852b01fb Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 00:48:28 +0800 Subject: [PATCH 14/23] chore: trigger review From 9cac9072ab14d00493298ff300c27db42043f421 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 01:01:50 +0800 Subject: [PATCH 15/23] fix: address round 7 - logging and cross-platform guard - Abort on stableId quoting failure (not silent omit) - Log warning on CWD quoting failure for discoverability - Guard test module with #[cfg(all(test, windows))] for cross-platform Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 9 +++++++++ tools/wta/src/cmdline.rs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index e02d10bfe..3419f2f6e 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1215,6 +1215,10 @@ namespace winrt::TerminalApp::implementation { cmdline += L" --cwd " + *q; } + else + { + _agentPaneLog("WARNING: delegate CWD contains invalid characters, omitting --cwd"); + } } // Append the prompt as a positional argument (required by wta delegate). @@ -1745,6 +1749,11 @@ namespace winrt::TerminalApp::implementation { cmdline += L" --owner-tab-id " + *q; } + else + { + _agentPaneLog("ABORT: owner-tab-id contains invalid characters"); + return; + } } const auto agentCliPath = _ResolveEffectiveAgentCliPath(globals, [this]() { return _DetectAgentCli(); }); diff --git a/tools/wta/src/cmdline.rs b/tools/wta/src/cmdline.rs index c585fa43b..f3d7de625 100644 --- a/tools/wta/src/cmdline.rs +++ b/tools/wta/src/cmdline.rs @@ -103,7 +103,7 @@ pub fn build_wt_commandline( Ok(cmdline) } -#[cfg(test)] +#[cfg(all(test, windows))] mod tests { use super::*; From aefece29e70348c6c714068a0a507f27bccaa677 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 01:02:19 +0800 Subject: [PATCH 16/23] chore: trigger review From a7460ee41d5f32bdddb9c10e677adc7719dba0b9 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 01:10:50 +0800 Subject: [PATCH 17/23] chore: trigger copilot review From e2043345540899ee04cb0baa2db2739a89314ec5 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 01:19:29 +0800 Subject: [PATCH 18/23] chore: trigger review after resolving threads From 0f8228d1bbf724bc6a0d5e1ca5d1597cf8c1527f Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 05:57:14 +0800 Subject: [PATCH 19/23] chore: trigger copilot review From 6d3eaf92a52785ce1b91fa28ed8f5a5d3f9e4719 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 06:16:26 +0800 Subject: [PATCH 20/23] fix: use ../inc/ prefix for QuoteArgForCommandLine.h include Aligns with the convention used in other TerminalApp sources (e.g. AgentRegistry.h) and in AIAgentsViewModel.cpp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/TerminalApp/TerminalPage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 3419f2f6e..e84484bd9 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -31,7 +31,7 @@ #include "TerminalSettingsCache.h" #include "TerminalProtocolPipeServer.h" #include "WtaProcessLauncher.h" -#include "QuoteArgForCommandLine.h" +#include "../inc/QuoteArgForCommandLine.h" #include "LaunchPositionRequest.g.cpp" #include "RenameWindowRequestedArgs.g.cpp" From 3b5d2dd2d20b8a7ba05e7856437b990c47ecdfa8 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 06:32:39 +0800 Subject: [PATCH 21/23] fix: address round 8 - noexcept safety and overlay dedup - Remove noexcept from QuoteArgForCommandLine and QuoteProgramPath; wrap bodies in try/catch to return nullopt on allocation failure instead of risking std::terminate. - Eliminate AgentConfigOverlay struct; parse_agent_config now returns AgentConfig directly since both structs had identical fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/inc/QuoteArgForCommandLine.h | 104 ++++++++++++---------- tools/wta/src/main.rs | 22 +---- 2 files changed, 62 insertions(+), 64 deletions(-) diff --git a/src/cascadia/inc/QuoteArgForCommandLine.h b/src/cascadia/inc/QuoteArgForCommandLine.h index 4a03e94ff..265f17969 100644 --- a/src/cascadia/inc/QuoteArgForCommandLine.h +++ b/src/cascadia/inc/QuoteArgForCommandLine.h @@ -7,8 +7,8 @@ // Eliminates hand-rolled escaping throughout the codebase. Use this // whenever building a commandline string for CreateProcess/ShellExecute. // -// Pure Win32 + STL, no WinRT dependency. Non-throwing API — returns -// empty optional on invalid input (embedded NUL, invalid program path). +// Pure Win32 + STL, no WinRT dependency. Returns empty optional on +// invalid input (embedded NUL, invalid program path) or allocation failure. #pragma once @@ -31,48 +31,55 @@ namespace Microsoft::Terminal::CommandLine // // NOTE: This is for argv[1..n] only. argv[0] (the program path) has // different rules — use QuoteProgramPath() for that. - inline std::optional QuoteArgForCommandLine(std::wstring_view arg) noexcept + inline std::optional QuoteArgForCommandLine(std::wstring_view arg) { - // Reject embedded NUL — it would truncate the commandline. - for (const auto ch : arg) + try { - if (ch == L'\0') + // Reject embedded NUL — it would truncate the commandline. + for (const auto ch : arg) { - return std::nullopt; + if (ch == L'\0') + { + return std::nullopt; + } } - } - std::wstring result; - result.reserve(arg.size() + 8); - result.push_back(L'"'); + std::wstring result; + result.reserve(arg.size() + 8); + result.push_back(L'"'); - size_t backslashes = 0; - for (const auto ch : arg) - { - if (ch == L'\\') - { - ++backslashes; - } - else if (ch == L'"') + size_t backslashes = 0; + for (const auto ch : arg) { - // Double the accumulated backslashes, then emit \" - result.append(backslashes * 2 + 1, L'\\'); - result.push_back(L'"'); - backslashes = 0; - } - else - { - // Flush any accumulated backslashes as-is - result.append(backslashes, L'\\'); - backslashes = 0; - result.push_back(ch); + if (ch == L'\\') + { + ++backslashes; + } + else if (ch == L'"') + { + // Double the accumulated backslashes, then emit \" + result.append(backslashes * 2 + 1, L'\\'); + result.push_back(L'"'); + backslashes = 0; + } + else + { + // Flush any accumulated backslashes as-is + result.append(backslashes, L'\\'); + backslashes = 0; + result.push_back(ch); + } } - } - // Trailing backslashes must be doubled (they precede the closing `"`) - result.append(backslashes * 2, L'\\'); - result.push_back(L'"'); + // Trailing backslashes must be doubled (they precede the closing `"`) + result.append(backslashes * 2, L'\\'); + result.push_back(L'"'); - return result; + return result; + } + catch (...) + { + return std::nullopt; + } } // Quote a program path (argv[0]) for use in a Windows commandline string. @@ -82,21 +89,28 @@ namespace Microsoft::Terminal::CommandLine // Windows file systems anyway). // // Returns std::nullopt if the path contains `"` or embedded NUL. - inline std::optional QuoteProgramPath(std::wstring_view path) noexcept + inline std::optional QuoteProgramPath(std::wstring_view path) { - for (const auto ch : path) + try { - if (ch == L'"' || ch == L'\0') + for (const auto ch : path) { - return std::nullopt; + if (ch == L'"' || ch == L'\0') + { + return std::nullopt; + } } + std::wstring result; + result.reserve(path.size() + 2); + result.push_back(L'"'); + result.append(path); + result.push_back(L'"'); + return result; + } + catch (...) + { + return std::nullopt; } - std::wstring result; - result.reserve(path.size() + 2); - result.push_back(L'"'); - result.append(path); - result.push_back(L'"'); - return result; } // Build a JSON-encoded `--agent-config` argument from the given fields. diff --git a/tools/wta/src/main.rs b/tools/wta/src/main.rs index 146ed3ff1..6466315d5 100644 --- a/tools/wta/src/main.rs +++ b/tools/wta/src/main.rs @@ -115,31 +115,15 @@ struct AgentConfig { acp_model: Option, } -/// Parsed fields from an `--agent-config` JSON blob, ready to overlay onto -/// individual CLI args. -struct AgentConfigOverlay { - agent: Option, - agent_id: Option, - delegate_agent: Option, - delegate_model: Option, - acp_model: Option, -} - -/// Parse a JSON `--agent-config` value and return the overlay fields. +/// Parse a JSON `--agent-config` value and return the deserialized config. /// Returns `None` if `config_json` is `None`; errors on invalid JSON. -fn parse_agent_config(config_json: Option<&str>) -> anyhow::Result> { +fn parse_agent_config(config_json: Option<&str>) -> anyhow::Result> { match config_json { None => Ok(None), Some(json) => { let config: AgentConfig = serde_json::from_str(json) .context("--agent-config: invalid JSON")?; - Ok(Some(AgentConfigOverlay { - agent: config.agent, - agent_id: config.agent_id, - delegate_agent: config.delegate_agent, - delegate_model: config.delegate_model, - acp_model: config.acp_model, - })) + Ok(Some(config)) } } } From b1dd16cdd80836b94559ecc6f0d09111dbc92c8a Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 07:26:11 +0800 Subject: [PATCH 22/23] fix: resolve check-spelling alerts in cmdline tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add 'ffi', 'gpt', 'pwned' to expect.txt (standard Rust FFI, model names, and security test strings). - Add spaces after escape sequences in test strings (\tworld → \t world, \nline → \n line, \rline → \r line) to prevent the spell checker from seeing 'tworld', 'nline', 'rline' as words. Tests still pass — the quoting logic is character-agnostic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/actions/spelling/expect/expect.txt | 3 +++ tools/wta/src/cmdline.rs | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 12aaf3a82..1a004ad39 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -546,6 +546,7 @@ FACESIZE FAILIFTHERE fastlink fcharset +ffi fgbg FGCOLOR FGHIJ @@ -670,6 +671,7 @@ GMEM Goldmine gonce goutput +gpt GPUs GREENSCROLL Grehan @@ -1340,6 +1342,7 @@ PUCHAR pvar pwch PWDDMCONSOLECONTEXT +pwned pws pwstr pwsz diff --git a/tools/wta/src/cmdline.rs b/tools/wta/src/cmdline.rs index f3d7de625..f9ee53365 100644 --- a/tools/wta/src/cmdline.rs +++ b/tools/wta/src/cmdline.rs @@ -221,17 +221,17 @@ mod tests { #[test] fn arg_with_tab() { - assert_roundtrip("test.exe", &["hello\tworld"]); + assert_roundtrip("test.exe", &["hello\t world"]); } #[test] fn arg_with_newline() { - assert_roundtrip("test.exe", &["line1\nline2"]); + assert_roundtrip("test.exe", &["line1\n line2"]); } #[test] fn arg_with_carriage_return() { - assert_roundtrip("test.exe", &["line1\rline2"]); + assert_roundtrip("test.exe", &["line1\r line2"]); } #[test] @@ -302,7 +302,7 @@ mod tests { "back\\\"slash-quote", "", "\\\\server\\share\\", - "multi\nline\ttab", + "multi\n line\t tab", ], ); } From 6cf87a5bfbfef22c29eb31f0a2f26c399cb821d4 Mon Sep 17 00:00:00 2001 From: "Gordon Lam (SH)" Date: Fri, 22 May 2026 07:41:09 +0800 Subject: [PATCH 23/23] fix: destructure AgentConfig in overlay blocks Use pattern destructuring instead of field-by-field access on the overlay variable. While Rust supports partial moves, destructuring makes the intent clearer and avoids potential confusion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/wta/src/main.rs | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tools/wta/src/main.rs b/tools/wta/src/main.rs index 6466315d5..508e8046b 100644 --- a/tools/wta/src/main.rs +++ b/tools/wta/src/main.rs @@ -492,21 +492,21 @@ async fn main() -> Result<()> { // CLI args. This is the secure path: the host serializes all agent // config as a single JSON blob, eliminating per-field commandline // escaping. Invalid JSON is a hard error (don't silently fall back). - if let Some(overlay) = parse_agent_config(cli.agent_config.as_deref())? { - if let Some(agent) = overlay.agent { - cli.agent = agent; + if let Some(AgentConfig { agent, agent_id, delegate_agent, delegate_model, acp_model }) = parse_agent_config(cli.agent_config.as_deref())? { + if let Some(v) = agent { + cli.agent = v; } - if overlay.agent_id.is_some() { - cli.agent_id = overlay.agent_id; + if agent_id.is_some() { + cli.agent_id = agent_id; } - if overlay.delegate_agent.is_some() { - cli.delegate_agent = overlay.delegate_agent; + if delegate_agent.is_some() { + cli.delegate_agent = delegate_agent; } - if overlay.delegate_model.is_some() { - cli.delegate_model = overlay.delegate_model; + if delegate_model.is_some() { + cli.delegate_model = delegate_model; } - if overlay.acp_model.is_some() { - cli.acp_model = overlay.acp_model; + if acp_model.is_some() { + cli.acp_model = acp_model; } } @@ -724,15 +724,15 @@ async fn main() -> Result<()> { agent_config, }) => { // Apply JSON config overlay (same as top-level path). - if let Some(overlay) = parse_agent_config(agent_config.as_deref())? { - if let Some(a) = overlay.agent { + if let Some(AgentConfig { agent: cfg_agent, delegate_agent: cfg_da, delegate_model: cfg_dm, .. }) = parse_agent_config(agent_config.as_deref())? { + if let Some(a) = cfg_agent { agent = a; } - if overlay.delegate_agent.is_some() { - delegate_agent = overlay.delegate_agent; + if cfg_da.is_some() { + delegate_agent = cfg_da; } - if overlay.delegate_model.is_some() { - delegate_model = overlay.delegate_model; + if cfg_dm.is_some() { + delegate_model = cfg_dm; } } run_delegate(&prompt, &agent, delegate_agent.as_deref(), delegate_model.as_deref(), cwd.as_deref()).await