mirror of
https://github.com/block/goose.git
synced 2026-07-17 12:56:20 +02:00
fix rare marker collisions
This commit is contained in:
Generated
+1
@@ -3211,6 +3211,7 @@ dependencies = [
|
||||
"umya-spreadsheet",
|
||||
"url",
|
||||
"utoipa",
|
||||
"uuid",
|
||||
"webbrowser",
|
||||
"which 8.0.0",
|
||||
"xcap",
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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_<uuid>__ so we find the first occurrence.
|
||||
fn strip_env_markers(raw: &str) -> String {
|
||||
// The format is: <command output>MARKER<cwd>MARKER<env>MARKER<exit_code>
|
||||
// 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="));
|
||||
}
|
||||
|
||||
@@ -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<PathBuf>,
|
||||
HashMap<String, String>,
|
||||
HashSet<String>,
|
||||
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<PathBuf>,
|
||||
HashMap<String, String>,
|
||||
HashSet<String>,
|
||||
i32,
|
||||
) {
|
||||
) -> Result<ParsedOutput, ParseError> {
|
||||
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"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user