diff --git a/Cargo.lock b/Cargo.lock index 568c01d59a..24a1f799ee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3211,6 +3211,7 @@ dependencies = [ "umya-spreadsheet", "url", "utoipa", + "uuid", "webbrowser", "which 8.0.0", "xcap", diff --git a/crates/goose-mcp/Cargo.toml b/crates/goose-mcp/Cargo.toml index aa88ca9db4..0f15c0b9ca 100644 --- a/crates/goose-mcp/Cargo.toml +++ b/crates/goose-mcp/Cargo.toml @@ -73,6 +73,7 @@ libc = "0.2" mpatch = "=0.2.0" tokio-util = "0.7.16" clap = { version = "4", features = ["derive"] } +uuid = { version = "1.19.0", features = ["v4"] } [dev-dependencies] sysinfo = "0.32.1" diff --git a/crates/goose-mcp/src/develop/mod.rs b/crates/goose-mcp/src/develop/mod.rs index cfd05c9811..66024a21c1 100644 --- a/crates/goose-mcp/src/develop/mod.rs +++ b/crates/goose-mcp/src/develop/mod.rs @@ -66,9 +66,17 @@ impl DevelopServer { let map_tool = Arc::new(MapTool::new()); let image_tool = Arc::new(ImageTool::new()); + // TODO: The cwd/os info at the end breaks prompt caching since it changes per-session. + // Consider moving dynamic context to a separate mechanism (e.g., resources or a + // dedicated context tool) so the static instructions can be cached. + let cwd = std::env::current_dir() + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_else(|_| ".".to_string()); + let os = std::env::consts::OS; + let mut instructions = formatdoc! {r#" The develop extension provides tools for software development tasks. - + Capabilities: - **Process**: Execute shell commands with `shell`. Working directory and environment variables persist across calls. Long-running or large-output @@ -88,7 +96,13 @@ impl DevelopServer { is re-sent. It's often better to do something in one call rather than two or three, even if you read a bit more than strictly necessary. Reading extra context to save a round-trip is usually worth it. - "#}; + + operating system: {os} + current directory: {cwd} + "#, + os = os, + cwd = cwd, + }; // Add shell warning if using fallback if let Some(warning) = manager.shell_warning() { diff --git a/crates/goose-mcp/src/develop/process/manager.rs b/crates/goose-mcp/src/develop/process/manager.rs index 984646826c..f9bdc63c6c 100644 --- a/crates/goose-mcp/src/develop/process/manager.rs +++ b/crates/goose-mcp/src/develop/process/manager.rs @@ -26,18 +26,19 @@ const PREVIEW_HEAD_LINES: usize = 20; /// Lines to show at tail of preview. const PREVIEW_TAIL_LINES: usize = 30; -/// Marker delimiter used in wrapped commands. -const ENV_MARKER: &str = "__GOOSE_ENV_MARKER__"; +/// Marker prefix used in wrapped commands (followed by UUID). +const ENV_MARKER_PREFIX: &str = "__GOOSE_ENV_"; /// Strip environment markers from raw output, returning just the command output. +/// The marker format is __GOOSE_ENV___ so we find the first occurrence. fn strip_env_markers(raw: &str) -> String { // The format is: MARKERMARKERMARKER // We want just the command output (everything before the first marker) - raw.split(ENV_MARKER) - .next() - .unwrap_or(raw) - .trim_end() - .to_string() + if let Some(pos) = raw.find(ENV_MARKER_PREFIX) { + raw.get(..pos).unwrap_or(raw).trim_end().to_string() + } else { + raw.trim_end().to_string() + } } /// A managed process with its output buffer and metadata. @@ -98,7 +99,7 @@ impl ProcessManager { // Build the wrapped command with env setup and capture let env_setup = env_state.setup_commands(); - let (wrapped, _, delimiter) = EnvState::wrap_command(command); + let (wrapped, delimiter) = EnvState::wrap_command(command); let full_command = format!("{}{}", env_setup, wrapped); let cwd = env_state.cwd.clone(); @@ -164,7 +165,7 @@ impl ProcessManager { let mut env_state = self.env_state.lock().unwrap(); let (actual_output, new_cwd, changed, unset, parsed_exit) = - env_state.parse_wrapped_output(&raw_output, delimiter); + env_state.parse_wrapped_output(&raw_output, &delimiter)?; env_state.apply_changes(new_cwd, changed, unset); drop(env_state); @@ -512,7 +513,7 @@ mod tests { // Get output - should NOT contain markers let output = manager.output(&id.0, OutputQuery::default()).unwrap(); assert!( - !output.contains(ENV_MARKER), + !output.contains(ENV_MARKER_PREFIX), "Output should not contain markers: {}", output ); @@ -528,7 +529,7 @@ mod tests { SpawnResult::Completed { output, .. } => { // If it completed, output should still be clean assert!( - !output.contains(ENV_MARKER), + !output.contains(ENV_MARKER_PREFIX), "Output should not contain markers" ); } @@ -537,10 +538,11 @@ mod tests { #[test] fn test_strip_env_markers() { - let raw = "hello world\n__GOOSE_ENV_MARKER__\n/tmp\n__GOOSE_ENV_MARKER__\nPATH=/bin\n__GOOSE_ENV_MARKER__\n0"; + // Test with UUID-style marker + let raw = "hello world\n__GOOSE_ENV_abc123def456__\n/tmp\n__GOOSE_ENV_abc123def456__\nPATH=/bin\n__GOOSE_ENV_abc123def456__\n0"; let clean = strip_env_markers(raw); assert_eq!(clean, "hello world"); - assert!(!clean.contains("__GOOSE_ENV_MARKER__")); + assert!(!clean.contains("__GOOSE_ENV_")); assert!(!clean.contains("/tmp")); assert!(!clean.contains("PATH=")); } diff --git a/crates/goose-mcp/src/develop/process/shell.rs b/crates/goose-mcp/src/develop/process/shell.rs index cacafbc822..5b8e9169c9 100644 --- a/crates/goose-mcp/src/develop/process/shell.rs +++ b/crates/goose-mcp/src/develop/process/shell.rs @@ -2,8 +2,47 @@ use std::collections::{HashMap, HashSet}; use std::env; +use std::fmt; use std::path::PathBuf; +use uuid::Uuid; + +/// Error type for shell output parsing failures. +#[derive(Debug, Clone)] +pub enum ParseError { + /// The delimiter appeared an unexpected number of times in the output. + /// This likely means the command output contained the delimiter pattern. + DelimiterMismatch { expected: usize, found: usize }, +} + +impl fmt::Display for ParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ParseError::DelimiterMismatch { expected, found } => { + write!( + f, + "Command output parsing failed: expected {} delimiter occurrences but found {}. \ + This is unexpected since the delimiter uses a UUID. \ + The command output may be corrupted.", + expected, found + ) + } + } + } +} + +impl std::error::Error for ParseError {} + +/// Result of parsing wrapped shell output. +/// Contains: (command_output, new_cwd, changed_env_vars, unset_env_vars, exit_code) +pub type ParsedOutput = ( + String, + Option, + HashMap, + HashSet, + i32, +); + /// Environment variables to ignore when tracking changes. /// These change on every command or are internal shell state. const ENV_IGNORELIST: &[&str] = &[ @@ -100,9 +139,11 @@ impl EnvState { } /// Generate shell commands to capture environment before/after user command. - /// Returns (wrapper_prefix, wrapper_suffix, delimiter). - pub fn wrap_command(command: &str) -> (String, String, &'static str) { - let delimiter = "__GOOSE_ENV_MARKER__"; + /// Returns (wrapped_command, delimiter). + /// + /// The delimiter is a UUID to avoid collisions with command output. + pub fn wrap_command(command: &str) -> (String, String) { + let delimiter = format!("__GOOSE_ENV_{}__", Uuid::new_v4().as_simple()); let prefix = r#"__goose_env_before=$(env | sort) __goose_cwd_before=$(pwd) @@ -122,7 +163,7 @@ echo $__goose_exit_code"#, ); let wrapped = format!("{}{}{}", prefix, command, suffix); - (wrapped, String::new(), delimiter) + (wrapped, delimiter) } /// Parse the output from a wrapped command, extracting: @@ -130,28 +171,30 @@ echo $__goose_exit_code"#, /// - New cwd /// - Environment changes /// - Exit code + /// + /// Returns Err if the delimiter appears an unexpected number of times (should be exactly 3). pub fn parse_wrapped_output( &self, raw_output: &str, delimiter: &str, - ) -> ( - String, - Option, - HashMap, - HashSet, - i32, - ) { + ) -> Result { + let delimiter_count = raw_output.matches(delimiter).count(); + + if delimiter_count != 3 { + return Err(ParseError::DelimiterMismatch { + expected: 3, + found: delimiter_count, + }); + } + let parts: Vec<&str> = raw_output.split(delimiter).collect(); - if parts.len() < 4 { - // Parsing failed, return raw output with no state changes - return ( - raw_output.to_string(), - None, - HashMap::new(), - HashSet::new(), - 1, - ); + // With exactly 3 delimiters, we should have exactly 4 parts + if parts.len() != 4 { + return Err(ParseError::DelimiterMismatch { + expected: 3, + found: delimiter_count, + }); } let command_output = parts[0].trim_end().to_string(); @@ -205,7 +248,7 @@ echo $__goose_exit_code"#, } } - (command_output, new_cwd_path, changed, unset, exit_code) + Ok((command_output, new_cwd_path, changed, unset, exit_code)) } /// Apply changes from a command execution. @@ -294,4 +337,81 @@ mod tests { let setup = state.setup_commands(); assert!(setup.contains("export QUOTED='it'\\''s a test'")); } + + #[test] + fn test_wrap_command_generates_uuid_delimiter() { + let (wrapped1, delim1) = EnvState::wrap_command("echo hello"); + let (_wrapped2, delim2) = EnvState::wrap_command("echo hello"); + + // Each call should generate a unique delimiter + assert_ne!(delim1, delim2); + + // Delimiter should have the expected format + assert!(delim1.starts_with("__GOOSE_ENV_")); + assert!(delim1.ends_with("__")); + + // Wrapped command should contain the delimiter + assert!(wrapped1.contains(&delim1)); + } + + #[test] + fn test_parse_wrapped_output_success() { + let state = EnvState::new(); + let delimiter = "__GOOSE_ENV_test123__"; + let raw_output = format!( + "hello world\n{}\n/tmp\n{}\nFOO=bar\n{}\n0", + delimiter, delimiter, delimiter + ); + + let result = state.parse_wrapped_output(&raw_output, delimiter); + assert!(result.is_ok()); + + let (output, new_cwd, _changed, _unset, exit_code) = result.unwrap(); + assert_eq!(output, "hello world"); + assert_eq!(new_cwd, Some(PathBuf::from("/tmp"))); + assert_eq!(exit_code, 0); + } + + #[test] + fn test_parse_wrapped_output_delimiter_mismatch() { + let state = EnvState::new(); + let delimiter = "__GOOSE_ENV_test123__"; + + // Output with only 2 delimiters (missing one) + let raw_output = format!("hello world\n{}\n/tmp\n{}\n0", delimiter, delimiter); + + let result = state.parse_wrapped_output(&raw_output, delimiter); + assert!(result.is_err()); + + match result { + Err(ParseError::DelimiterMismatch { expected, found }) => { + assert_eq!(expected, 3); + assert_eq!(found, 2); + } + Ok(_) => panic!("Expected delimiter mismatch error"), + } + } + + #[test] + fn test_parse_wrapped_output_extra_delimiter_in_output() { + let state = EnvState::new(); + let delimiter = "__GOOSE_ENV_test123__"; + + // Output where the command itself printed the delimiter (4 total) + let raw_output = format!( + "hello {} world\n{}\n/tmp\n{}\nFOO=bar\n{}\n0", + delimiter, delimiter, delimiter, delimiter + ); + + let result = state.parse_wrapped_output(&raw_output, delimiter); + assert!(result.is_err()); + + match result { + Err(ParseError::DelimiterMismatch { expected, found }) => { + assert_eq!(expected, 3); + assert_eq!(found, 4); + } + Ok(_) => panic!("Expected delimiter mismatch error"), + } + } }