From becc1fd0f8b36dc92d5ea7430007a0f8ed900ae1 Mon Sep 17 00:00:00 2001 From: Nick Eddy Date: Fri, 15 May 2026 16:34:15 -0700 Subject: [PATCH] fix: shell-escape generator-produced suggestion replacements Suggestions produced by `Generator::script` (Rust generators registered via the command-signatures crate) and by v2 script generators flowed through `From<...::Suggestion> for Suggestion` impls that copied the raw value into `replacement` with no shell-escaping. Once the upstream command-signatures fix for `git add ` lands, this gap becomes user-visible: selecting a suggestion for a path containing spaces inserts an unescaped string into the buffer, so the shell parses it as multiple tokens (`git add new file test.csv` instead of `git add new\ file\ test.csv`). Apply `ShellFamily::escape` after the `From` conversion in both the legacy (`engine/argument/legacy.rs`) and v2 (`engine/argument/v2.rs`) generator pipelines so generator-backed suggestions match the escaping behavior the path completer (`engine/path.rs`) already provides for filesystem-backed completions. Includes a failing-then-passing unit test for the legacy code path. A parallel v2 test is deferred because it would require introducing shared test infrastructure (TEST_GENERATOR_3 + JS callback wiring). Refs warpdotdev/warp#11072, warpdotdev/command-signatures#271. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/completer/engine/argument/legacy.rs | 9 ++ .../src/completer/engine/argument/v2.rs | 10 +++ .../src/completer/suggest/test.rs | 88 +++++++++++++++++++ 3 files changed, 107 insertions(+) diff --git a/crates/warp_completer/src/completer/engine/argument/legacy.rs b/crates/warp_completer/src/completer/engine/argument/legacy.rs index 62b3803561..f98b860100 100644 --- a/crates/warp_completer/src/completer/engine/argument/legacy.rs +++ b/crates/warp_completer/src/completer/engine/argument/legacy.rs @@ -726,10 +726,19 @@ async fn generate_suggestions_for_argument_type( let results = generator.on_complete(&output_string); + // Generators emit raw values; shell-escape so the buffer-insertion path + // produces a single shell token (e.g. `new file test.csv` becomes + // `new\ file\ test.csv` for POSIX shells). + let shell_family = ctx.shell_family().unwrap_or(ShellFamily::Posix); let internal_suggestions = results .suggestions .into_iter() .map(Into::into) + .map(|mut suggestion: Suggestion| { + suggestion.replacement = + shell_family.escape(&suggestion.replacement).into(); + suggestion + }) .filter_map(|suggestion: Suggestion| { let match_type = matcher .get_match_type(parsed_token.as_str(), suggestion.display.as_str()); diff --git a/crates/warp_completer/src/completer/engine/argument/v2.rs b/crates/warp_completer/src/completer/engine/argument/v2.rs index f4af428134..b832d6d629 100644 --- a/crates/warp_completer/src/completer/engine/argument/v2.rs +++ b/crates/warp_completer/src/completer/engine/argument/v2.rs @@ -13,6 +13,7 @@ use std::collections::HashMap; use itertools::Itertools; use smol_str::SmolStr; +use warp_util::path::ShellFamily; use super::add_extra_positional; use crate::completer::context::call_js_function; @@ -695,10 +696,19 @@ async fn generate_suggestions_for_argument_value( } } }; + // Generators emit raw values; shell-escape so the buffer-insertion path + // produces a single shell token (e.g. `new file test.csv` becomes + // `new\ file\ test.csv` for POSIX shells). Mirrors the legacy code path. + let shell_family = ctx.shell_family().unwrap_or(ShellFamily::Posix); let internal_suggestions = results .suggestions .into_iter() .map(Into::into) + .map(|mut suggestion: Suggestion| { + suggestion.replacement = + shell_family.escape(&suggestion.replacement).into(); + suggestion + }) .filter_map(|suggestion: Suggestion| { let match_type = matcher .get_match_type(parsed_token.as_str(), suggestion.display.as_str()); diff --git a/crates/warp_completer/src/completer/suggest/test.rs b/crates/warp_completer/src/completer/suggest/test.rs index 7dfb3db51a..1a78f8fbef 100644 --- a/crates/warp_completer/src/completer/suggest/test.rs +++ b/crates/warp_completer/src/completer/suggest/test.rs @@ -2297,3 +2297,91 @@ fn test_powershell_parser_directives_for_case_insensitivity() { vec!["-Force"] ); } + +/// Suggestions produced by a Rust generator (`Generator::script` registered via the +/// command-signatures crate) flow through `legacy.rs:From +/// for Suggestion`, which today copies `exact_string` into `replacement` verbatim. Real-world +/// trigger: `git add ` after the command-signatures repo is fixed to return raw filenames +/// — the generator produces `new file test.csv`, but the buffer-insertion path needs +/// `new\ file\ test.csv` (POSIX) for the shell to parse it as one token. Without escaping at +/// this boundary, picking the suggestion produces three shell tokens and `git add` fails. +#[cfg(not(feature = "v2"))] +#[test] +fn test_generator_suggestion_replacement_is_shell_escaped() { + use std::collections::HashMap; + use warp_command_signatures::{ + Argument, ArgumentType, CommandBuilder, CommandSignatureGenerators, Generator, + GeneratorName, GeneratorResults, IsArgumentOptional, Signature, + Suggestion as MetadataSuggestion, + }; + + use crate::signatures::CommandRegistry; + + const FILE_NAME: &str = "new file test.csv"; + const SHELL_CMD: &str = "echo file_with_spaces"; + + let generators = CommandSignatureGenerators::new("stage").add_generator( + "files", + Generator::script( + CommandBuilder::single_command(SHELL_CMD), + // Output is irrelevant to this test; the callback returns a fixed + // suggestion mirroring what the fixed command-signatures generator + // emits for `git status --short -z`. + |_| GeneratorResults { + suggestions: vec![MetadataSuggestion::new(FILE_NAME)], + is_ordered: false, + }, + ), + ); + let generators_map = HashMap::from([generators.into()]); + + let signature = Signature { + name: "stage".to_owned(), + alias_generator: None, + description: None, + arguments: Some(vec![Argument { + display_name: Some("file".to_owned()), + description: None, + is_variadic: false, + is_command: false, + argument_types: vec![ArgumentType::Generator(GeneratorName("files".to_owned()))], + optional: IsArgumentOptional::Required, + skip_generator_validation: false, + }]), + subcommands: None, + options: None, + priority: warp_command_signatures::Priority::default(), + parser_directives: Default::default(), + }; + + let registry = CommandRegistry::new_for_test([signature], generators_map); + let generator_ctx = MockGeneratorContext::new().with_expected_command(SHELL_CMD, ""); + let ctx = FakeCompletionContext::new(registry).with_generator_context(generator_ctx); + + let results = suggestions_for_test( + "stage ", + "stage ".len(), + CompleterOptions { + match_strategy: MatchStrategy::CaseInsensitive, + fallback_strategy: CompletionsFallbackStrategy::FilePaths, + suggest_file_path_completions_only: false, + parse_quotes_as_literals: false, + }, + &ctx, + ) + .expect("expected suggestion results from generator"); + + let matched = results + .suggestions + .iter() + .find(|s| s.display() == FILE_NAME) + .expect("expected the generator's suggestion to surface"); + + assert_eq!( + matched.replacement(), + r"new\ file\ test.csv", + "Generator-produced replacement must be shell-escaped before insertion. \ + Today it equals the unescaped `{FILE_NAME}`, which the shell would parse \ + as three separate tokens." + ); +}