From ed5c86fcfc390e371109d761ec0461b42304a020 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 15:15:37 -0700 Subject: [PATCH] tui: fuzz slash command roundtrips Add registry-driven proptest coverage for slash command serialization in tui and tui_app_server. The new property suite surfaced a remainder quoting bug, so the shared slash protocol now shell-quotes non-roundtrippable remainder text before replay parsing. Co-authored-by: Codex --- codex-rs/Cargo.lock | 47 ++- codex-rs/Cargo.toml | 1 + codex-rs/tui/Cargo.toml | 1 + codex-rs/tui/src/slash_command.rs | 383 ++++++++++++++++++ codex-rs/tui/src/slash_command_protocol.rs | 85 +++- codex-rs/tui_app_server/Cargo.toml | 1 + codex-rs/tui_app_server/src/slash_command.rs | 381 +++++++++++++++++ .../src/slash_command_protocol.rs | 85 +++- 8 files changed, 968 insertions(+), 16 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b3f8d88027..24e026f057 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -965,7 +965,16 @@ version = "0.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" dependencies = [ - "bit-vec", + "bit-vec 0.6.3", +] + +[[package]] +name = "bit-set" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3" +dependencies = [ + "bit-vec 0.8.0", ] [[package]] @@ -974,6 +983,12 @@ version = "0.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" +[[package]] +name = "bit-vec" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" + [[package]] name = "bitflags" version = "1.3.2" @@ -2624,6 +2639,7 @@ dependencies = [ "libc", "pathdiff", "pretty_assertions", + "proptest", "pulldown-cmark", "rand 0.9.2", "ratatui", @@ -2716,6 +2732,7 @@ dependencies = [ "libc", "pathdiff", "pretty_assertions", + "proptest", "pulldown-cmark", "rand 0.9.2", "ratatui", @@ -5305,7 +5322,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "525e9ff3e1a4be2fbea1fdf0e98686a6d98b4d8f937e1bf7402245af1909e8c3" dependencies = [ "byteorder-lite", - "quick-error", + "quick-error 2.0.1", ] [[package]] @@ -5661,7 +5678,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0a1cbf952127589f2851ab2046af368fd20645491bb4b376f04b7f94d7a9837b" dependencies = [ "ascii-canvas", - "bit-set", + "bit-set 0.5.3", "diff", "ena", "is-terminal", @@ -7345,12 +7362,16 @@ version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bee689443a2bd0a16ab0348b52ee43e3b2d1b1f931c8aa5c9f8de4c86fbe8c40" dependencies = [ + "bit-set 0.8.0", + "bit-vec 0.8.0", "bitflags 2.10.0", "num-traits", "rand 0.9.2", "rand_chacha 0.9.0", "rand_xorshift", "regex-syntax 0.8.8", + "rusty-fork", + "tempfile", "unarray", ] @@ -7420,6 +7441,12 @@ dependencies = [ "num-traits", ] +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + [[package]] name = "quick-error" version = "2.0.1" @@ -8384,6 +8411,18 @@ version = "1.0.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" +[[package]] +name = "rusty-fork" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc6bf79ff24e648f6da1f8d1f011e9cac26491b619e6b9280f2b47f1774e6ee2" +dependencies = [ + "fnv", + "quick-error 1.2.3", + "tempfile", + "wait-timeout", +] + [[package]] name = "rustyline" version = "14.0.0" @@ -9963,7 +10002,7 @@ dependencies = [ "fax", "flate2", "half", - "quick-error", + "quick-error 2.0.1", "weezl", "zune-jpeg 0.4.21", ] diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 6d768d6963..9b36717e3f 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -237,6 +237,7 @@ pathdiff = "0.2" portable-pty = "0.9.0" predicates = "3" pretty_assertions = "1.4.1" +proptest = "1.7.0" pulldown-cmark = "0.10" quick-xml = "0.38.4" rand = "0.9" diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 8013b1325e..5f11d3c128 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -142,6 +142,7 @@ assert_matches = { workspace = true } chrono = { workspace = true, features = ["serde"] } insta = { workspace = true } pretty_assertions = { workspace = true } +proptest = { workspace = true } rand = { workspace = true } serial_test = { workspace = true } vt100 = { workspace = true } diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index d0320c3b2c..3a9369d148 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -1091,11 +1091,394 @@ pub fn built_in_slash_commands() -> Vec<(&'static str, SlashCommand)> { #[cfg(test)] mod tests { + use codex_protocol::user_input::ByteRange; + use codex_protocol::user_input::TextElement; use pretty_assertions::assert_eq; + use proptest::prelude::*; + use proptest::sample::select; + use proptest::test_runner::Config as ProptestConfig; + use proptest::test_runner::TestCaseError; + use proptest::test_runner::TestRunner; use std::str::FromStr; use super::*; + fn placeholder_text_arg(placeholder: &str) -> SlashTextArg { + SlashTextArg::new( + placeholder.to_string(), + vec![TextElement::new( + (0..placeholder.len()).into(), + Some(placeholder.to_string()), + )], + ) + } + + fn shift_text_element_left(element: &TextElement, offset: usize) -> Option { + if element.byte_range.end <= offset { + return None; + } + let start = element.byte_range.start.saturating_sub(offset); + let end = element.byte_range.end.saturating_sub(offset); + (start < end).then(|| element.map_range(|_| ByteRange { start, end })) + } + + fn serialized_args( + invocation: &SlashCommandInvocation, + ) -> (String, Vec) { + let serialized = invocation.serialize(); + let prefix = format!("/{}", invocation.command().command()); + let args = serialized + .text + .strip_prefix(&prefix) + .expect("serialized invocation should start with command prefix"); + let Some(args) = args.strip_prefix(' ') else { + return (String::new(), Vec::new()); + }; + let offset = prefix.len() + 1; + let elements = serialized + .text_elements + .iter() + .filter_map(|element| shift_text_element_left(element, offset)) + .collect(); + (args.to_string(), elements) + } + + fn token_text_strategy() -> BoxedStrategy { + prop_oneof![ + proptest::string::string_regex("[A-Za-z0-9._/-]{1,16}").unwrap(), + ( + proptest::string::string_regex("[A-Za-z0-9._/-]{1,8}").unwrap(), + proptest::string::string_regex("[A-Za-z0-9._/-]{1,8}").unwrap(), + ) + .prop_map(|(lhs, rhs)| format!("{lhs} {rhs}")), + proptest::string::string_regex("[A-Za-z0-9._/-]{1,8}[\"'][A-Za-z0-9._/-]{1,8}") + .unwrap(), + ] + .boxed() + } + + fn plain_text_arg_strategy() -> BoxedStrategy { + proptest::string::string_regex( + "[A-Za-z0-9][A-Za-z0-9._/'\"-]{0,10}( [A-Za-z0-9][A-Za-z0-9._/'\"-]{0,10}){0,3}", + ) + .unwrap() + .prop_map(|text| SlashTextArg::new(text, Vec::new())) + .boxed() + } + + fn placeholder_text_arg_strategy() -> BoxedStrategy { + ( + proptest::string::string_regex("[A-Za-z]{0,6}").unwrap(), + select(vec!["[Image #1]".to_string(), "[Image #12]".to_string()]), + proptest::string::string_regex("[A-Za-z]{0,6}").unwrap(), + ) + .prop_map(|(prefix, placeholder, suffix)| { + let mut text = String::new(); + if !prefix.is_empty() { + text.push_str(&prefix); + text.push(' '); + } + let start = text.len(); + text.push_str(&placeholder); + let end = text.len(); + if !suffix.is_empty() { + text.push(' '); + text.push_str(&suffix); + } + SlashTextArg::new( + text, + vec![TextElement::new((start..end).into(), Some(placeholder))], + ) + }) + .boxed() + } + + fn text_arg_strategy() -> BoxedStrategy { + prop_oneof![plain_text_arg_strategy(), placeholder_text_arg_strategy(),].boxed() + } + + fn token_arg_strategy() -> BoxedStrategy { + token_text_strategy() + .prop_map(|text| SlashTokenArg::new(text, Vec::new())) + .boxed() + } + + impl SlashCommand { + fn roundtrip_test_invocations(self) -> Vec { + let bare = SlashCommandInvocation::Bare(self); + match self { + SlashCommand::Fast => vec![ + bare, + SlashCommandInvocation::Fast(FastArgs { + mode: FastSlashCommandArgs::On, + }), + SlashCommandInvocation::Fast(FastArgs { + mode: FastSlashCommandArgs::Off, + }), + SlashCommandInvocation::Fast(FastArgs { + mode: FastSlashCommandArgs::Status, + }), + ], + SlashCommand::SandboxReadRoot => vec![ + bare, + SlashCommandInvocation::SandboxReadRoot(SandboxReadRootArgs { + path: SlashTokenArg::new("/tmp/test-dir".to_string(), Vec::new()), + }), + ], + SlashCommand::Review => vec![ + bare, + SlashCommandInvocation::Review(ReviewArgs { + instructions: placeholder_text_arg("[Image #1]"), + }), + ], + SlashCommand::Rename => vec![ + bare, + SlashCommandInvocation::Rename(RenameArgs { + title: SlashTextArg::new("ship it".to_string(), Vec::new()), + }), + ], + SlashCommand::Plan => vec![ + bare, + SlashCommandInvocation::Plan(PlanArgs { + prompt: SlashTextArg::new("investigate flaky test".to_string(), Vec::new()), + }), + ], + SlashCommand::Statusline => vec![ + bare, + SlashCommandInvocation::Statusline(StatuslineArgs { + items: vec![StatusLineItem::ModelName, StatusLineItem::CurrentDir], + }), + ], + SlashCommand::Feedback => vec![ + bare, + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::BadResult, + }), + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::GoodResult, + }), + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::Bug, + }), + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::SafetyCheck, + }), + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::Other, + }), + ], + SlashCommand::Model + | SlashCommand::Approvals + | SlashCommand::Permissions + | SlashCommand::ElevateSandbox + | SlashCommand::Experimental + | SlashCommand::Skills + | SlashCommand::New + | SlashCommand::Resume + | SlashCommand::Fork + | SlashCommand::Init + | SlashCommand::Compact + | SlashCommand::Collab + | SlashCommand::Agent + | SlashCommand::Diff + | SlashCommand::Copy + | SlashCommand::Mention + | SlashCommand::Status + | SlashCommand::DebugConfig + | SlashCommand::Title + | SlashCommand::Theme + | SlashCommand::Mcp + | SlashCommand::Apps + | SlashCommand::Plugins + | SlashCommand::Logout + | SlashCommand::Quit + | SlashCommand::Exit + | SlashCommand::Rollout + | SlashCommand::Ps + | SlashCommand::Stop + | SlashCommand::Clear + | SlashCommand::Personality + | SlashCommand::Realtime + | SlashCommand::Settings + | SlashCommand::TestApproval + | SlashCommand::MultiAgents + | SlashCommand::MemoryDrop + | SlashCommand::MemoryUpdate => vec![bare], + } + } + + fn roundtrip_strategy(self) -> BoxedStrategy { + let bare = Just(SlashCommandInvocation::Bare(self)); + match self { + SlashCommand::Fast => prop_oneof![ + bare, + select(vec![ + FastSlashCommandArgs::On, + FastSlashCommandArgs::Off, + FastSlashCommandArgs::Status, + ]) + .prop_map(|mode| SlashCommandInvocation::Fast(FastArgs { mode })), + ] + .boxed(), + SlashCommand::SandboxReadRoot => prop_oneof![ + bare, + token_arg_strategy().prop_map(|path| { + SlashCommandInvocation::SandboxReadRoot(SandboxReadRootArgs { path }) + }), + ] + .boxed(), + SlashCommand::Review => prop_oneof![ + bare, + text_arg_strategy().prop_map(|instructions| { + SlashCommandInvocation::Review(ReviewArgs { instructions }) + }), + ] + .boxed(), + SlashCommand::Rename => prop_oneof![ + bare, + plain_text_arg_strategy() + .prop_map(|title| { SlashCommandInvocation::Rename(RenameArgs { title }) }), + ] + .boxed(), + SlashCommand::Plan => prop_oneof![ + bare, + text_arg_strategy() + .prop_map(|prompt| { SlashCommandInvocation::Plan(PlanArgs { prompt }) }), + ] + .boxed(), + SlashCommand::Statusline => { + let items = vec![ + StatusLineItem::ModelName, + StatusLineItem::ModelWithReasoning, + StatusLineItem::CurrentDir, + StatusLineItem::ProjectRoot, + StatusLineItem::GitBranch, + StatusLineItem::ContextRemaining, + StatusLineItem::ContextUsed, + StatusLineItem::FiveHourLimit, + StatusLineItem::WeeklyLimit, + StatusLineItem::CodexVersion, + StatusLineItem::ContextWindowSize, + StatusLineItem::UsedTokens, + StatusLineItem::TotalInputTokens, + StatusLineItem::TotalOutputTokens, + StatusLineItem::SessionId, + StatusLineItem::FastMode, + ]; + prop_oneof![ + bare, + proptest::collection::vec(select(items), 1..5).prop_map(|items| { + SlashCommandInvocation::Statusline(StatuslineArgs { items }) + }), + ] + .boxed() + } + SlashCommand::Feedback => prop_oneof![ + bare, + select(vec![ + FeedbackCategory::BadResult, + FeedbackCategory::GoodResult, + FeedbackCategory::Bug, + FeedbackCategory::SafetyCheck, + FeedbackCategory::Other, + ]) + .prop_map(|category| { + SlashCommandInvocation::Feedback(FeedbackArgs { category }) + }), + ] + .boxed(), + SlashCommand::Model + | SlashCommand::Approvals + | SlashCommand::Permissions + | SlashCommand::ElevateSandbox + | SlashCommand::Experimental + | SlashCommand::Skills + | SlashCommand::New + | SlashCommand::Resume + | SlashCommand::Fork + | SlashCommand::Init + | SlashCommand::Compact + | SlashCommand::Collab + | SlashCommand::Agent + | SlashCommand::Diff + | SlashCommand::Copy + | SlashCommand::Mention + | SlashCommand::Status + | SlashCommand::DebugConfig + | SlashCommand::Title + | SlashCommand::Theme + | SlashCommand::Mcp + | SlashCommand::Apps + | SlashCommand::Plugins + | SlashCommand::Logout + | SlashCommand::Quit + | SlashCommand::Exit + | SlashCommand::Rollout + | SlashCommand::Ps + | SlashCommand::Stop + | SlashCommand::Clear + | SlashCommand::Personality + | SlashCommand::Realtime + | SlashCommand::Settings + | SlashCommand::TestApproval + | SlashCommand::MultiAgents + | SlashCommand::MemoryDrop + | SlashCommand::MemoryUpdate => bare.boxed(), + } + } + } + + #[test] + fn all_registered_commands_roundtrip_from_serialized_text() { + for spec in SLASH_COMMAND_SPECS { + for invocation in spec.command.roundtrip_test_invocations() { + let (args, text_elements) = serialized_args(&invocation); + assert_eq!( + spec.command.parse_invocation(&args, &text_elements), + Ok(invocation.clone()), + "roundtrip failed for /{} with serialized {:?}", + spec.command.command(), + invocation.serialize().text + ); + } + } + } + + #[test] + fn all_registered_commands_proptest_roundtrip_from_serialized_text() { + for spec in SLASH_COMMAND_SPECS { + let command = spec.command; + let mut runner = TestRunner::new(ProptestConfig { + cases: 24, + failure_persistence: None, + ..ProptestConfig::default() + }); + runner + .run(&command.roundtrip_strategy(), |invocation| { + let serialized = invocation.serialize(); + let (args, text_elements) = serialized_args(&invocation); + let reparsed = + command + .parse_invocation(&args, &text_elements) + .map_err(|err| { + TestCaseError::fail(format!( + "roundtrip parse failed for /{} from {:?}: {err:?}", + command.command(), + serialized.text + )) + })?; + prop_assert_eq!(reparsed, invocation); + Ok(()) + }) + .unwrap_or_else(|err| { + panic!( + "property roundtrip failed for /{}: {err}", + command.command() + ) + }); + } + } + #[test] fn approvals_alias_is_hidden_from_command_popup() { assert!(SlashCommand::Approvals.hide_in_command_popup()); diff --git a/codex-rs/tui/src/slash_command_protocol.rs b/codex-rs/tui/src/slash_command_protocol.rs index b97e0a92ec..de7939502d 100644 --- a/codex-rs/tui/src/slash_command_protocol.rs +++ b/codex-rs/tui/src/slash_command_protocol.rs @@ -210,7 +210,7 @@ impl<'a> SlashArgsParser<'a> { } pub(crate) fn remainder(&self) -> Option { - trim_text_arg(self.input.args, self.input.text_elements) + parse_remainder_text_arg(self.input.args, self.input.text_elements) } pub(crate) fn required_remainder(&self) -> Result { @@ -265,10 +265,17 @@ impl SlashArgsSerializer { } pub(crate) fn remainder(&mut self, value: &SlashTextArg) { - self.fragments.push(SlashSerializedText { - text: value.text.clone(), - text_elements: value.text_elements.clone(), - }); + if remainder_can_roundtrip_raw(value) { + self.fragments.push(SlashSerializedText { + text: value.text.clone(), + text_elements: value.text_elements.clone(), + }); + } else { + self.fragments.push(serialize_token(&SlashTokenArg::new( + value.text.clone(), + value.text_elements.clone(), + ))); + } } pub(crate) fn finish(self) -> SlashSerializedText { @@ -299,6 +306,30 @@ fn trim_text_arg(text: &str, text_elements: &[TextElement]) -> Option Option { + let trimmed = trim_text_arg(text, text_elements)?; + match tokenize_with_elements(&trimmed.text, &trimmed.text_elements) { + Ok(tokens) => match tokens.as_slice() { + [token] => Some(SlashTextArg::new( + token.text.clone(), + token.text_elements.clone(), + )), + _ => Some(trimmed), + }, + _ => Some(trimmed), + } +} + +fn remainder_can_roundtrip_raw(value: &SlashTextArg) -> bool { + match tokenize_with_elements(&value.text, &value.text_elements) { + Ok(tokens) if tokens.len() == 1 => { + tokens[0] == SlashTokenArg::new(value.text.clone(), value.text_elements.clone()) + } + Ok(_) => true, + Err(_) => false, + } +} + fn split_named_arg(token: &SlashTokenArg) -> Option<(String, SlashTokenArg)> { let rest = token.text.strip_prefix("--")?; let (key, value) = rest.split_once('=')?; @@ -345,10 +376,28 @@ fn serialize_token(token: &SlashTokenArg) -> SlashSerializedText { let (token_for_shlex, replacements) = replace_text_elements_with_sentinels(&token.text, &token.text_elements); - let quoted = try_join([token_for_shlex.as_str()]).unwrap_or_else(|_| token_for_shlex.clone()); + let quoted = try_join([token_for_shlex.as_str()]) + .unwrap_or_else(|_| shell_quote_token(&token_for_shlex)); restore_sentinels_in_fragment(quoted, &replacements) } +fn shell_quote_token(token: &str) -> String { + if token.is_empty() { + return "''".to_string(); + } + + let mut quoted = String::from("'"); + for ch in token.chars() { + if ch == '\'' { + quoted.push_str("'\"'\"'"); + } else { + quoted.push(ch); + } + } + quoted.push('\''); + quoted +} + fn join_serialized_fragments(fragments: Vec) -> SlashSerializedText { let mut text = String::new(); let mut text_elements = Vec::new(); @@ -590,4 +639,28 @@ mod tests { } ); } + + #[test] + fn remainder_quotes_shell_sensitive_text_when_needed() { + let prompt = SlashTextArg::new("a\"\" a\"".to_string(), Vec::new()); + let mut serializer = SlashArgsSerializer::default(); + serializer.remainder(&prompt); + + assert_eq!( + serializer.finish(), + SlashSerializedText { + text: "'a\"\" a\"'".to_string(), + text_elements: Vec::new(), + } + ); + assert_eq!( + SlashArgsParser::new(SlashCommandParseInput { + args: "'a\"\" a\"'", + text_elements: &[], + }) + .unwrap() + .required_remainder(), + Ok(prompt) + ); + } } diff --git a/codex-rs/tui_app_server/Cargo.toml b/codex-rs/tui_app_server/Cargo.toml index 8866042051..f800cce85f 100644 --- a/codex-rs/tui_app_server/Cargo.toml +++ b/codex-rs/tui_app_server/Cargo.toml @@ -145,6 +145,7 @@ assert_matches = { workspace = true } chrono = { workspace = true, features = ["serde"] } insta = { workspace = true } pretty_assertions = { workspace = true } +proptest = { workspace = true } rand = { workspace = true } serial_test = { workspace = true } vt100 = { workspace = true } diff --git a/codex-rs/tui_app_server/src/slash_command.rs b/codex-rs/tui_app_server/src/slash_command.rs index cd9487efb8..d70011bebf 100644 --- a/codex-rs/tui_app_server/src/slash_command.rs +++ b/codex-rs/tui_app_server/src/slash_command.rs @@ -1077,11 +1077,392 @@ pub fn built_in_slash_commands() -> Vec<(&'static str, SlashCommand)> { #[cfg(test)] mod tests { + use codex_protocol::user_input::ByteRange; + use codex_protocol::user_input::TextElement; use pretty_assertions::assert_eq; + use proptest::prelude::*; + use proptest::sample::select; + use proptest::test_runner::Config as ProptestConfig; + use proptest::test_runner::TestCaseError; + use proptest::test_runner::TestRunner; use std::str::FromStr; use super::*; + fn placeholder_text_arg(placeholder: &str) -> SlashTextArg { + SlashTextArg::new( + placeholder.to_string(), + vec![TextElement::new( + (0..placeholder.len()).into(), + Some(placeholder.to_string()), + )], + ) + } + + fn shift_text_element_left(element: &TextElement, offset: usize) -> Option { + if element.byte_range.end <= offset { + return None; + } + let start = element.byte_range.start.saturating_sub(offset); + let end = element.byte_range.end.saturating_sub(offset); + (start < end).then(|| element.map_range(|_| ByteRange { start, end })) + } + + fn serialized_args( + invocation: &SlashCommandInvocation, + ) -> (String, Vec) { + let serialized = invocation.serialize(); + let prefix = format!("/{}", invocation.command().command()); + let args = serialized + .text + .strip_prefix(&prefix) + .expect("serialized invocation should start with command prefix"); + let Some(args) = args.strip_prefix(' ') else { + return (String::new(), Vec::new()); + }; + let offset = prefix.len() + 1; + let elements = serialized + .text_elements + .iter() + .filter_map(|element| shift_text_element_left(element, offset)) + .collect(); + (args.to_string(), elements) + } + + fn token_text_strategy() -> BoxedStrategy { + prop_oneof![ + proptest::string::string_regex("[A-Za-z0-9._/-]{1,16}").unwrap(), + ( + proptest::string::string_regex("[A-Za-z0-9._/-]{1,8}").unwrap(), + proptest::string::string_regex("[A-Za-z0-9._/-]{1,8}").unwrap(), + ) + .prop_map(|(lhs, rhs)| format!("{lhs} {rhs}")), + proptest::string::string_regex("[A-Za-z0-9._/-]{1,8}[\"'][A-Za-z0-9._/-]{1,8}") + .unwrap(), + ] + .boxed() + } + + fn plain_text_arg_strategy() -> BoxedStrategy { + proptest::string::string_regex( + "[A-Za-z0-9][A-Za-z0-9._/'\"-]{0,10}( [A-Za-z0-9][A-Za-z0-9._/'\"-]{0,10}){0,3}", + ) + .unwrap() + .prop_map(|text| SlashTextArg::new(text, Vec::new())) + .boxed() + } + + fn placeholder_text_arg_strategy() -> BoxedStrategy { + ( + proptest::string::string_regex("[A-Za-z]{0,6}").unwrap(), + select(vec!["[Image #1]".to_string(), "[Image #12]".to_string()]), + proptest::string::string_regex("[A-Za-z]{0,6}").unwrap(), + ) + .prop_map(|(prefix, placeholder, suffix)| { + let mut text = String::new(); + if !prefix.is_empty() { + text.push_str(&prefix); + text.push(' '); + } + let start = text.len(); + text.push_str(&placeholder); + let end = text.len(); + if !suffix.is_empty() { + text.push(' '); + text.push_str(&suffix); + } + SlashTextArg::new( + text, + vec![TextElement::new((start..end).into(), Some(placeholder))], + ) + }) + .boxed() + } + + fn text_arg_strategy() -> BoxedStrategy { + prop_oneof![plain_text_arg_strategy(), placeholder_text_arg_strategy(),].boxed() + } + + fn token_arg_strategy() -> BoxedStrategy { + token_text_strategy() + .prop_map(|text| SlashTokenArg::new(text, Vec::new())) + .boxed() + } + + impl SlashCommand { + fn roundtrip_test_invocations(self) -> Vec { + let bare = SlashCommandInvocation::Bare(self); + match self { + SlashCommand::Fast => vec![ + bare, + SlashCommandInvocation::Fast(FastArgs { + mode: FastSlashCommandArgs::On, + }), + SlashCommandInvocation::Fast(FastArgs { + mode: FastSlashCommandArgs::Off, + }), + SlashCommandInvocation::Fast(FastArgs { + mode: FastSlashCommandArgs::Status, + }), + ], + SlashCommand::SandboxReadRoot => vec![ + bare, + SlashCommandInvocation::SandboxReadRoot(SandboxReadRootArgs { + path: SlashTokenArg::new("/tmp/test-dir".to_string(), Vec::new()), + }), + ], + SlashCommand::Review => vec![ + bare, + SlashCommandInvocation::Review(ReviewArgs { + instructions: placeholder_text_arg("[Image #1]"), + }), + ], + SlashCommand::Rename => vec![ + bare, + SlashCommandInvocation::Rename(RenameArgs { + title: SlashTextArg::new("ship it".to_string(), Vec::new()), + }), + ], + SlashCommand::Plan => vec![ + bare, + SlashCommandInvocation::Plan(PlanArgs { + prompt: SlashTextArg::new("investigate flaky test".to_string(), Vec::new()), + }), + ], + SlashCommand::Statusline => vec![ + bare, + SlashCommandInvocation::Statusline(StatuslineArgs { + items: vec![StatusLineItem::ModelName, StatusLineItem::CurrentDir], + }), + ], + SlashCommand::Feedback => vec![ + bare, + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::BadResult, + }), + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::GoodResult, + }), + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::Bug, + }), + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::SafetyCheck, + }), + SlashCommandInvocation::Feedback(FeedbackArgs { + category: FeedbackCategory::Other, + }), + ], + SlashCommand::Model + | SlashCommand::Approvals + | SlashCommand::Permissions + | SlashCommand::ElevateSandbox + | SlashCommand::Experimental + | SlashCommand::Skills + | SlashCommand::New + | SlashCommand::Resume + | SlashCommand::Fork + | SlashCommand::Init + | SlashCommand::Compact + | SlashCommand::Collab + | SlashCommand::Agent + | SlashCommand::Diff + | SlashCommand::Copy + | SlashCommand::Mention + | SlashCommand::Status + | SlashCommand::DebugConfig + | SlashCommand::Theme + | SlashCommand::Mcp + | SlashCommand::Apps + | SlashCommand::Plugins + | SlashCommand::Logout + | SlashCommand::Quit + | SlashCommand::Exit + | SlashCommand::Rollout + | SlashCommand::Ps + | SlashCommand::Stop + | SlashCommand::Clear + | SlashCommand::Personality + | SlashCommand::Realtime + | SlashCommand::Settings + | SlashCommand::TestApproval + | SlashCommand::MultiAgents + | SlashCommand::MemoryDrop + | SlashCommand::MemoryUpdate => vec![bare], + } + } + + fn roundtrip_strategy(self) -> BoxedStrategy { + let bare = Just(SlashCommandInvocation::Bare(self)); + match self { + SlashCommand::Fast => prop_oneof![ + bare, + select(vec![ + FastSlashCommandArgs::On, + FastSlashCommandArgs::Off, + FastSlashCommandArgs::Status, + ]) + .prop_map(|mode| SlashCommandInvocation::Fast(FastArgs { mode })), + ] + .boxed(), + SlashCommand::SandboxReadRoot => prop_oneof![ + bare, + token_arg_strategy().prop_map(|path| { + SlashCommandInvocation::SandboxReadRoot(SandboxReadRootArgs { path }) + }), + ] + .boxed(), + SlashCommand::Review => prop_oneof![ + bare, + text_arg_strategy().prop_map(|instructions| { + SlashCommandInvocation::Review(ReviewArgs { instructions }) + }), + ] + .boxed(), + SlashCommand::Rename => prop_oneof![ + bare, + plain_text_arg_strategy() + .prop_map(|title| { SlashCommandInvocation::Rename(RenameArgs { title }) }), + ] + .boxed(), + SlashCommand::Plan => prop_oneof![ + bare, + text_arg_strategy() + .prop_map(|prompt| { SlashCommandInvocation::Plan(PlanArgs { prompt }) }), + ] + .boxed(), + SlashCommand::Statusline => { + let items = vec![ + StatusLineItem::ModelName, + StatusLineItem::ModelWithReasoning, + StatusLineItem::CurrentDir, + StatusLineItem::ProjectRoot, + StatusLineItem::GitBranch, + StatusLineItem::ContextRemaining, + StatusLineItem::ContextUsed, + StatusLineItem::FiveHourLimit, + StatusLineItem::WeeklyLimit, + StatusLineItem::CodexVersion, + StatusLineItem::ContextWindowSize, + StatusLineItem::UsedTokens, + StatusLineItem::TotalInputTokens, + StatusLineItem::TotalOutputTokens, + StatusLineItem::SessionId, + StatusLineItem::FastMode, + ]; + prop_oneof![ + bare, + proptest::collection::vec(select(items), 1..5).prop_map(|items| { + SlashCommandInvocation::Statusline(StatuslineArgs { items }) + }), + ] + .boxed() + } + SlashCommand::Feedback => prop_oneof![ + bare, + select(vec![ + FeedbackCategory::BadResult, + FeedbackCategory::GoodResult, + FeedbackCategory::Bug, + FeedbackCategory::SafetyCheck, + FeedbackCategory::Other, + ]) + .prop_map(|category| { + SlashCommandInvocation::Feedback(FeedbackArgs { category }) + }), + ] + .boxed(), + SlashCommand::Model + | SlashCommand::Approvals + | SlashCommand::Permissions + | SlashCommand::ElevateSandbox + | SlashCommand::Experimental + | SlashCommand::Skills + | SlashCommand::New + | SlashCommand::Resume + | SlashCommand::Fork + | SlashCommand::Init + | SlashCommand::Compact + | SlashCommand::Collab + | SlashCommand::Agent + | SlashCommand::Diff + | SlashCommand::Copy + | SlashCommand::Mention + | SlashCommand::Status + | SlashCommand::DebugConfig + | SlashCommand::Theme + | SlashCommand::Mcp + | SlashCommand::Apps + | SlashCommand::Plugins + | SlashCommand::Logout + | SlashCommand::Quit + | SlashCommand::Exit + | SlashCommand::Rollout + | SlashCommand::Ps + | SlashCommand::Stop + | SlashCommand::Clear + | SlashCommand::Personality + | SlashCommand::Realtime + | SlashCommand::Settings + | SlashCommand::TestApproval + | SlashCommand::MultiAgents + | SlashCommand::MemoryDrop + | SlashCommand::MemoryUpdate => bare.boxed(), + } + } + } + + #[test] + fn all_registered_commands_proptest_roundtrip_from_serialized_text() { + for spec in SLASH_COMMAND_SPECS { + let command = spec.command; + let mut runner = TestRunner::new(ProptestConfig { + cases: 24, + failure_persistence: None, + ..ProptestConfig::default() + }); + runner + .run(&command.roundtrip_strategy(), |invocation| { + let serialized = invocation.serialize(); + let (args, text_elements) = serialized_args(&invocation); + let reparsed = + command + .parse_invocation(&args, &text_elements) + .map_err(|err| { + TestCaseError::fail(format!( + "roundtrip parse failed for /{} from {:?}: {err:?}", + command.command(), + serialized.text + )) + })?; + prop_assert_eq!(reparsed, invocation); + Ok(()) + }) + .unwrap_or_else(|err| { + panic!( + "property roundtrip failed for /{}: {err}", + command.command() + ) + }); + } + } + + #[test] + fn all_registered_commands_roundtrip_from_serialized_text() { + for spec in SLASH_COMMAND_SPECS { + for invocation in spec.command.roundtrip_test_invocations() { + let (args, text_elements) = serialized_args(&invocation); + assert_eq!( + spec.command.parse_invocation(&args, &text_elements), + Ok(invocation.clone()), + "roundtrip failed for /{} with serialized {:?}", + spec.command.command(), + invocation.serialize().text + ); + } + } + } + #[test] fn approvals_alias_is_hidden_from_command_popup() { assert!(SlashCommand::Approvals.hide_in_command_popup()); diff --git a/codex-rs/tui_app_server/src/slash_command_protocol.rs b/codex-rs/tui_app_server/src/slash_command_protocol.rs index b97e0a92ec..de7939502d 100644 --- a/codex-rs/tui_app_server/src/slash_command_protocol.rs +++ b/codex-rs/tui_app_server/src/slash_command_protocol.rs @@ -210,7 +210,7 @@ impl<'a> SlashArgsParser<'a> { } pub(crate) fn remainder(&self) -> Option { - trim_text_arg(self.input.args, self.input.text_elements) + parse_remainder_text_arg(self.input.args, self.input.text_elements) } pub(crate) fn required_remainder(&self) -> Result { @@ -265,10 +265,17 @@ impl SlashArgsSerializer { } pub(crate) fn remainder(&mut self, value: &SlashTextArg) { - self.fragments.push(SlashSerializedText { - text: value.text.clone(), - text_elements: value.text_elements.clone(), - }); + if remainder_can_roundtrip_raw(value) { + self.fragments.push(SlashSerializedText { + text: value.text.clone(), + text_elements: value.text_elements.clone(), + }); + } else { + self.fragments.push(serialize_token(&SlashTokenArg::new( + value.text.clone(), + value.text_elements.clone(), + ))); + } } pub(crate) fn finish(self) -> SlashSerializedText { @@ -299,6 +306,30 @@ fn trim_text_arg(text: &str, text_elements: &[TextElement]) -> Option Option { + let trimmed = trim_text_arg(text, text_elements)?; + match tokenize_with_elements(&trimmed.text, &trimmed.text_elements) { + Ok(tokens) => match tokens.as_slice() { + [token] => Some(SlashTextArg::new( + token.text.clone(), + token.text_elements.clone(), + )), + _ => Some(trimmed), + }, + _ => Some(trimmed), + } +} + +fn remainder_can_roundtrip_raw(value: &SlashTextArg) -> bool { + match tokenize_with_elements(&value.text, &value.text_elements) { + Ok(tokens) if tokens.len() == 1 => { + tokens[0] == SlashTokenArg::new(value.text.clone(), value.text_elements.clone()) + } + Ok(_) => true, + Err(_) => false, + } +} + fn split_named_arg(token: &SlashTokenArg) -> Option<(String, SlashTokenArg)> { let rest = token.text.strip_prefix("--")?; let (key, value) = rest.split_once('=')?; @@ -345,10 +376,28 @@ fn serialize_token(token: &SlashTokenArg) -> SlashSerializedText { let (token_for_shlex, replacements) = replace_text_elements_with_sentinels(&token.text, &token.text_elements); - let quoted = try_join([token_for_shlex.as_str()]).unwrap_or_else(|_| token_for_shlex.clone()); + let quoted = try_join([token_for_shlex.as_str()]) + .unwrap_or_else(|_| shell_quote_token(&token_for_shlex)); restore_sentinels_in_fragment(quoted, &replacements) } +fn shell_quote_token(token: &str) -> String { + if token.is_empty() { + return "''".to_string(); + } + + let mut quoted = String::from("'"); + for ch in token.chars() { + if ch == '\'' { + quoted.push_str("'\"'\"'"); + } else { + quoted.push(ch); + } + } + quoted.push('\''); + quoted +} + fn join_serialized_fragments(fragments: Vec) -> SlashSerializedText { let mut text = String::new(); let mut text_elements = Vec::new(); @@ -590,4 +639,28 @@ mod tests { } ); } + + #[test] + fn remainder_quotes_shell_sensitive_text_when_needed() { + let prompt = SlashTextArg::new("a\"\" a\"".to_string(), Vec::new()); + let mut serializer = SlashArgsSerializer::default(); + serializer.remainder(&prompt); + + assert_eq!( + serializer.finish(), + SlashSerializedText { + text: "'a\"\" a\"'".to_string(), + text_elements: Vec::new(), + } + ); + assert_eq!( + SlashArgsParser::new(SlashCommandParseInput { + args: "'a\"\" a\"'", + text_elements: &[], + }) + .unwrap() + .required_remainder(), + Ok(prompt) + ); + } }