From d1cc47324d11efff047b99ed12dad9485ebb11f6 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 22 May 2026 21:26:36 -0400 Subject: [PATCH] fix(agent): use raw sandbox reads for edits Separate raw file reads from the line-numbered display API so apply_patch and edit_file operate on unformatted UTF-8 content. Keep read_file/read_many_files model-facing output numbered and cover regressions for prefix corruption. --- lib/crates/fabro-agent/src/apply_patch.rs | 106 +++++++++---- lib/crates/fabro-agent/src/tools.rs | 70 ++++++--- lib/crates/fabro-sandbox/src/daytona/mod.rs | 15 +- lib/crates/fabro-sandbox/src/docker.rs | 142 ++++++++---------- lib/crates/fabro-sandbox/src/local.rs | 15 +- lib/crates/fabro-sandbox/src/sandbox.rs | 22 ++- lib/crates/fabro-sandbox/src/test_support.rs | 112 ++++++-------- lib/crates/fabro-sandbox/src/worktree.rs | 5 + lib/crates/fabro-server/src/run_files.rs | 14 +- lib/crates/fabro-workflow/src/artifact.rs | 7 +- .../fabro-workflow/src/artifact_snapshot.rs | 7 +- .../fabro-workflow/src/devcontainer_bridge.rs | 9 +- .../fabro-workflow/src/handler/command.rs | 7 +- lib/crates/fabro-workflow/src/sandbox_git.rs | 7 +- .../fabro-workflow/tests/it/integration.rs | 7 +- 15 files changed, 269 insertions(+), 276 deletions(-) diff --git a/lib/crates/fabro-agent/src/apply_patch.rs b/lib/crates/fabro-agent/src/apply_patch.rs index fc0f5b959..fb7624160 100644 --- a/lib/crates/fabro-agent/src/apply_patch.rs +++ b/lib/crates/fabro-agent/src/apply_patch.rs @@ -268,7 +268,7 @@ pub async fn apply_patch_operations( new_path, hunks, } => { - let original = env.read_file(path, None, None).await.map_err(|e| { + let original = env.read_file_text(path).await.map_err(|e| { format!( "Failed to read file to update {path}: {}", e.display_with_causes() @@ -507,6 +507,7 @@ mod tests { use tokio_util::sync::CancellationToken; use super::*; + use crate::LocalSandbox; use crate::test_support::MutableMockSandbox; use crate::tool_registry::ToolContext; @@ -683,7 +684,7 @@ mod tests { let result = apply_patch_operations(&ops, &env).await.unwrap(); assert!(result.contains("M src/game.py")); - let content = env.read_file("src/game.py", None, None).await.unwrap(); + let content = env.read_file_text("src/game.py").await.unwrap(); assert!(content.contains("from src.cards import Card, Suit")); assert!(!content.contains("from src.cards import Suit\n")); assert!(content.contains("stock: list[Card]")); @@ -804,7 +805,7 @@ mod tests { let result = apply_patch_operations(&ops, &env).await.unwrap(); assert!(result.contains("M src/lib.rs")); - let content = env.read_file("src/lib.rs", None, None).await.unwrap(); + let content = env.read_file_text("src/lib.rs").await.unwrap(); assert_eq!(content, "fn unchanged() {\n new_line();\n}\n"); } @@ -843,7 +844,7 @@ mod tests { let result = apply_patch_operations(&ops, &env).await.unwrap(); assert!(result.contains("M src/lib.rs")); - let content = env.read_file("src/lib.rs", None, None).await.unwrap(); + let content = env.read_file_text("src/lib.rs").await.unwrap(); assert!(content.contains("new_setup()")); assert!(content.contains("new_teardown()")); assert!(!content.contains("old_setup()")); @@ -861,7 +862,7 @@ mod tests { let result = apply_patch_operations(&ops, &env).await.unwrap(); assert!(result.contains("A src/new.rs")); - let content = env.read_file("src/new.rs", None, None).await.unwrap(); + let content = env.read_file_text("src/new.rs").await.unwrap(); assert_eq!(content, "fn new() {}"); } @@ -890,11 +891,39 @@ mod tests { let result = apply_patch_operations(&ops, &env).await.unwrap(); assert!(result.contains("M src/lib.rs")); - let content = env.read_file("src/lib.rs", None, None).await.unwrap(); + let content = env.read_file_text("src/lib.rs").await.unwrap(); assert!(content.contains("println!(\"new\")")); assert!(!content.contains("println!(\"old\")")); } + #[tokio::test] + async fn apply_patch_updates_raw_local_file_without_line_number_prefixes() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("src/lib.rs"); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, "fn hello() {\n println!(\"old\");\n}\n").unwrap(); + let env = LocalSandbox::new(dir.path().to_path_buf()); + let patch = "\ +*** Begin Patch +*** Update File: src/lib.rs +@@ +- println!(\"old\"); ++ println!(\"new\"); +*** End Patch"; + + let ops = parse_apply_patch(patch).unwrap(); + let result = apply_patch_operations(&ops, &env).await.unwrap(); + + assert_eq!( + result, + "Success. Updated the following files:\nM src/lib.rs\n" + ); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "fn hello() {\n println!(\"new\");\n}\n" + ); + } + #[test] fn apply_patch_tool_definition_is_custom_freeform() { let tool = make_apply_patch_tool(); @@ -964,7 +993,7 @@ mod tests { "Success. Updated the following files:\nA duplicate.txt\n" ); assert_eq!( - env.read_file("duplicate.txt", None, None).await.unwrap(), + env.read_file_text("duplicate.txt").await.unwrap(), "new content\n" ); } @@ -1001,7 +1030,33 @@ mod tests { "Success. Updated the following files:\nM insert_only.txt\n" ); assert_eq!( - env.read_file("insert_only.txt", None, None).await.unwrap(), + env.read_file_text("insert_only.txt").await.unwrap(), + "alpha\nomega\ninserted\n" + ); + } + + #[tokio::test] + async fn pure_addition_update_hunk_uses_raw_local_file_text() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("insert_only.txt"); + std::fs::write(&path, "alpha\nomega\n").unwrap(); + let env = LocalSandbox::new(dir.path().to_path_buf()); + let patch = "\ +*** Begin Patch +*** Update File: insert_only.txt +@@ ++inserted +*** End Patch"; + + let ops = parse_apply_patch(patch).unwrap(); + let result = apply_patch_operations(&ops, &env).await.unwrap(); + + assert_eq!( + result, + "Success. Updated the following files:\nM insert_only.txt\n" + ); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), "alpha\nomega\ninserted\n" ); } @@ -1026,7 +1081,7 @@ mod tests { apply_patch_operations(&ops, &env).await.unwrap(); assert_eq!( - env.read_file("no_newline.txt", None, None).await.unwrap(), + env.read_file_text("no_newline.txt").await.unwrap(), "has newline now\n" ); } @@ -1278,11 +1333,11 @@ please apply this assert!(result.contains("M src/new.py")); // New path exists with updated content - let content = env.read_file("src/new.py", None, None).await.unwrap(); + let content = env.read_file_text("src/new.py").await.unwrap(); assert_eq!(content, "def hello():\n return 1\n"); // Old path is deleted - let old = env.read_file("src/old.py", None, None).await; + let old = env.read_file_text("src/old.py").await; assert!(old.is_err()); } @@ -1425,7 +1480,7 @@ class GameState: let ops = parse_apply_patch(patch).unwrap(); apply_patch_operations(&ops, &env).await.unwrap(); - let content = env.read_file("src/game.py", None, None).await.unwrap(); + let content = env.read_file_text("src/game.py").await.unwrap(); assert!(content.contains("from src.cards import Card, Suit")); assert!(content.contains("stock: list[Card]")); assert!(content.contains("waste: list[Card]")); @@ -1482,12 +1537,12 @@ def main(): assert!(result.contains("D src/old_util.py")); assert!(result.contains("M src/main.py")); - let new_util = env.read_file("src/new_util.py", None, None).await.unwrap(); + let new_util = env.read_file_text("src/new_util.py").await.unwrap(); assert_eq!(new_util, "def new_helper():\n return 42\n"); - assert!(env.read_file("src/old_util.py", None, None).await.is_err()); + assert!(env.read_file_text("src/old_util.py").await.is_err()); - let main = env.read_file("src/main.py", None, None).await.unwrap(); + let main = env.read_file_text("src/main.py").await.unwrap(); assert!(main.contains("from new_util import new_helper")); assert!(main.contains("result = new_helper()")); assert!(main.contains("print(\"done\")")); @@ -1542,17 +1597,10 @@ EOF"; assert!(result.contains("M src/models/account.py")); // Old path gone - assert!( - env.read_file("src/models/user.py", None, None) - .await - .is_err() - ); + assert!(env.read_file_text("src/models/user.py").await.is_err()); // New path has updated content - let content = env - .read_file("src/models/account.py", None, None) - .await - .unwrap(); + let content = env.read_file_text("src/models/account.py").await.unwrap(); assert!(content.contains("self.email = None")); assert!(content.contains("self.active = True")); assert!(content.contains("def greet(self):")); @@ -1600,7 +1648,7 @@ def gamma(): let ops = parse_apply_patch(patch).unwrap(); apply_patch_operations(&ops, &env).await.unwrap(); - let content = env.read_file("src/stubs.py", None, None).await.unwrap(); + let content = env.read_file_text("src/stubs.py").await.unwrap(); assert!(content.contains("return \"a\"")); assert!(content.contains("return \"b\"")); assert!(content.contains("return \"c\"")); @@ -1628,7 +1676,7 @@ def gamma(): let ops = parse_apply_patch(patch).unwrap(); apply_patch_operations(&ops, &env).await.unwrap(); - let content = env.read_file("src/lib.rs", None, None).await.unwrap(); + let content = env.read_file_text("src/lib.rs").await.unwrap(); assert!(content.contains("println!(\"world\")")); assert!(!content.contains("println!(\"hello\")")); } @@ -1718,15 +1766,15 @@ def farewell(name): .await .unwrap(); - let content = env.read_file("src/app.py", None, None).await.unwrap(); + let content = env.read_file_text("src/app.py").await.unwrap(); assert!(content.contains("Hello, {name}!")); assert!(content.contains("Goodbye, {name}!")); assert!(!content.contains("Hi, {name}")); assert!(!content.contains("Bye, {name}")); - let created = env.read_file("src/created.py", None, None).await.unwrap(); + let created = env.read_file_text("src/created.py").await.unwrap(); assert!(created.contains("def created():")); - assert!(env.read_file("src/obsolete.py", None, None).await.is_err()); + assert!(env.read_file_text("src/obsolete.py").await.is_err()); } #[tokio::test] diff --git a/lib/crates/fabro-agent/src/tools.rs b/lib/crates/fabro-agent/src/tools.rs index 502d3d96f..9efa86c41 100644 --- a/lib/crates/fabro-agent/src/tools.rs +++ b/lib/crates/fabro-agent/src/tools.rs @@ -170,19 +170,12 @@ pub fn make_edit_file_tool() -> RegisteredTool { .and_then(serde_json::Value::as_bool) .unwrap_or(false); - let numbered_content = ctx + let raw_content = ctx .env - .read_file(file_path, None, None) + .read_file_text(file_path) .await .map_err(|e| e.display_with_causes())?; - // Strip line numbers: each line looks like " 1 | content" or " 10 | content" - let raw_lines: Vec<&str> = numbered_content - .lines() - .map(|line| line.find(" | ").map_or(line, |idx| &line[idx + 3..])) - .collect(); - let raw_content = raw_lines.join("\n"); - let count = raw_content.matches(old_string).count(); if count == 0 { return Err("old_string not found in file".to_string()); @@ -699,10 +692,9 @@ mod tests { async fn read_file_returns_content() { let tool = make_read_file_tool(); let mut files = HashMap::new(); - files.insert("/test.txt".into(), " 1 | hello\n 2 | world".into()); + files.insert("/test.txt".into(), "hello\nworld".into()); let env: Arc = Arc::new(MockSandbox { files, - apply_read_offset_limit: true, ..Default::default() }); let result = (tool.executor)(serde_json::json!({"file_path": "/test.txt"}), ToolContext { @@ -715,20 +707,16 @@ mod tests { agent_event_emitter: None, }) .await; - assert_eq!(result.unwrap(), " 1 | hello\n 2 | world"); + assert_eq!(result.unwrap(), "1 | hello\n2 | world\n"); } #[tokio::test] async fn read_file_with_offset_and_limit() { let tool = make_read_file_tool(); let mut files = HashMap::new(); - files.insert( - "/test.txt".into(), - " 1 | line1\n 2 | line2\n 3 | line3\n 4 | line4".into(), - ); + files.insert("/test.txt".into(), "line1\nline2\nline3\nline4".into()); let env: Arc = Arc::new(MockSandbox { files, - apply_read_offset_limit: true, ..Default::default() }); let result = (tool.executor)( @@ -744,7 +732,7 @@ mod tests { }, ) .await; - assert_eq!(result.unwrap(), " 2 | line2\n 3 | line3"); + assert_eq!(result.unwrap(), "3 | line3\n4 | line4\n"); } #[tokio::test] @@ -776,7 +764,7 @@ mod tests { async fn edit_file_replaces_match() { let tool = make_edit_file_tool(); let mut files = HashMap::new(); - files.insert("/f.txt".into(), " 1 | hello world".into()); + files.insert("/f.txt".into(), "hello world".into()); let env = Arc::new(MockSandbox { files, ..Default::default() @@ -809,7 +797,7 @@ mod tests { async fn edit_file_not_found_error() { let tool = make_edit_file_tool(); let mut files = HashMap::new(); - files.insert("/f.txt".into(), " 1 | hello world".into()); + files.insert("/f.txt".into(), "hello world".into()); let env: Arc = Arc::new(MockSandbox { files, ..Default::default() @@ -838,7 +826,7 @@ mod tests { async fn edit_file_not_unique_error() { let tool = make_edit_file_tool(); let mut files = HashMap::new(); - files.insert("/f.txt".into(), " 1 | aa bb aa".into()); + files.insert("/f.txt".into(), "aa bb aa".into()); let env: Arc = Arc::new(MockSandbox { files, ..Default::default() @@ -869,7 +857,7 @@ mod tests { async fn edit_file_replace_all() { let tool = make_edit_file_tool(); let mut files = HashMap::new(); - files.insert("/f.txt".into(), " 1 | aa bb aa".into()); + files.insert("/f.txt".into(), "aa bb aa".into()); let env = Arc::new(MockSandbox { files, ..Default::default() @@ -899,6 +887,39 @@ mod tests { assert_eq!(written[0].1, "cc bb cc"); } + #[tokio::test] + async fn edit_file_preserves_literal_line_number_prefixes() { + let tool = make_edit_file_tool(); + let mut files = HashMap::new(); + files.insert("/f.txt".into(), "1 | keep this literal\nhello".into()); + let env = Arc::new(MockSandbox { + files, + ..Default::default() + }); + let env_clone: Arc = env.clone(); + let result = (tool.executor)( + serde_json::json!({ + "file_path": "/f.txt", + "old_string": "hello", + "new_string": "goodbye" + }), + ToolContext { + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, + session_id: None, + root_session_id: None, + tool_call_id: None, + agent_event_emitter: None, + }, + ) + .await; + assert_eq!(result.unwrap(), "Successfully edited /f.txt"); + let written = env.written_files.lock().unwrap(); + assert_eq!(written.len(), 1); + assert_eq!(written[0].1, "1 | keep this literal\ngoodbye"); + } + #[tokio::test] async fn shell_basic_command() { let tool = make_shell_tool(); @@ -1131,10 +1152,9 @@ mod tests { async fn read_file_does_not_resolve_failing_tool_env_provider() { let tool = make_read_file_tool(); let mut files = HashMap::new(); - files.insert("/test.txt".into(), " 1 | hello".into()); + files.insert("/test.txt".into(), "hello".into()); let env: Arc = Arc::new(MockSandbox { files, - apply_read_offset_limit: true, ..Default::default() }); @@ -1149,7 +1169,7 @@ mod tests { }) .await; - assert_eq!(result.unwrap(), " 1 | hello"); + assert_eq!(result.unwrap(), "1 | hello\n"); } #[tokio::test] diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index f91c0a7c0..9609c4000 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -29,8 +29,7 @@ use crate::redact::redact_auth_url; use crate::sandbox::{optional_timeout, resolve_path}; use crate::{ CommandOutputCallback, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, Sandbox, - SandboxEvent, SandboxEventCallback, StdioProcess, format_lines_numbered, managed_labels, - shell_quote, + SandboxEvent, SandboxEventCallback, StdioProcess, managed_labels, shell_quote, }; pub(crate) const WORKING_DIRECTORY: &str = "/home/daytona/workspace"; @@ -1271,12 +1270,7 @@ impl Sandbox for DaytonaSandbox { .map_err(|e| crate::Error::context("Failed to set autostop interval", e)) } - async fn read_file( - &self, - path: &str, - offset: Option, - limit: Option, - ) -> crate::Result { + async fn read_file_bytes(&self, path: &str) -> crate::Result> { let sandbox = self.sandbox()?; let resolved = self.resolve_path(path); @@ -1290,10 +1284,7 @@ impl Sandbox for DaytonaSandbox { .await .map_err(|e| crate::Error::context(format!("Failed to read file {resolved}"), e))?; - let content = String::from_utf8(bytes) - .map_err(|e| crate::Error::context("File is not valid UTF-8", e))?; - - Ok(format_lines_numbered(&content, offset, limit)) + Ok(bytes) } async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { diff --git a/lib/crates/fabro-sandbox/src/docker.rs b/lib/crates/fabro-sandbox/src/docker.rs index 2df8e039d..2e22e884d 100644 --- a/lib/crates/fabro-sandbox/src/docker.rs +++ b/lib/crates/fabro-sandbox/src/docker.rs @@ -32,7 +32,7 @@ use crate::sandbox::{StdioProcessControl, optional_timeout, resolve_path}; use crate::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, - StdioProcess, StdioProcessHandle, StdioProcessTermination, format_lines_numbered, shell_quote, + StdioProcess, StdioProcessHandle, StdioProcessTermination, shell_quote, }; pub(crate) const WORKING_DIRECTORY: &str = "/workspace"; @@ -205,6 +205,64 @@ impl DockerSandbox { resolve_path(path, self.working_directory()) } + async fn download_file_bytes(&self, remote_path: &str) -> crate::Result> { + let container_id = self.container_id()?; + let container_path = self.resolve_container_path(remote_path); + let opts = DownloadFromContainerOptions { + path: container_path.clone(), + }; + let mut stream = self + .docker + .download_from_container(container_id, Some(opts)); + let mut archive_bytes = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|e| { + crate::Error::context( + format!("Failed to download {container_path} from container"), + e, + ) + })?; + archive_bytes.extend_from_slice(&chunk); + } + + #[expect( + clippy::disallowed_types, + reason = "tar entries are synchronous in-memory readers; bytes are collected before any await" + )] + use std::io::Read as _; + + let mut archive = tar::Archive::new(Cursor::new(archive_bytes)); + let entries = archive.entries().map_err(|e| { + crate::Error::context( + format!("Failed to read Docker archive for {container_path}"), + e, + ) + })?; + for entry in entries { + let mut entry = entry.map_err(|e| { + crate::Error::context( + format!("Failed to read Docker archive entry for {container_path}"), + e, + ) + })?; + if !entry.header().entry_type().is_file() { + continue; + } + let mut bytes = Vec::new(); + entry.read_to_end(&mut bytes).map_err(|e| { + crate::Error::context( + format!("Failed to read Docker archive file for {container_path}"), + e, + ) + })?; + return Ok(bytes); + } + + Err(crate::Error::message(format!( + "Docker archive for {container_path} did not contain a file" + ))) + } + fn repo_cloned(&self) -> bool { self.repo_cloned.get().copied().unwrap_or(false) } @@ -1161,67 +1219,7 @@ impl Sandbox for DockerSandbox { remote_path: &str, local_path: &std::path::Path, ) -> crate::Result<()> { - let container_id = self.container_id()?; - let container_path = self.resolve_container_path(remote_path); - let opts = DownloadFromContainerOptions { - path: container_path.clone(), - }; - let mut stream = self - .docker - .download_from_container(container_id, Some(opts)); - let mut archive_bytes = Vec::new(); - while let Some(chunk) = stream.next().await { - let chunk = chunk.map_err(|e| { - crate::Error::context( - format!("Failed to download {container_path} from container"), - e, - ) - })?; - archive_bytes.extend_from_slice(&chunk); - } - - let bytes = { - #[expect( - clippy::disallowed_types, - reason = "tar entries are synchronous in-memory readers; bytes are collected before any await" - )] - use std::io::Read as _; - - let mut archive = tar::Archive::new(Cursor::new(archive_bytes)); - let entries = archive.entries().map_err(|e| { - crate::Error::context( - format!("Failed to read Docker archive for {container_path}"), - e, - ) - })?; - let mut file_bytes = None; - for entry in entries { - let mut entry = entry.map_err(|e| { - crate::Error::context( - format!("Failed to read Docker archive entry for {container_path}"), - e, - ) - })?; - if !entry.header().entry_type().is_file() { - continue; - } - let mut bytes = Vec::new(); - entry.read_to_end(&mut bytes).map_err(|e| { - crate::Error::context( - format!("Failed to read Docker archive file for {container_path}"), - e, - ) - })?; - file_bytes = Some(bytes); - break; - } - file_bytes.ok_or_else(|| { - crate::Error::message(format!( - "Docker archive for {container_path} did not contain a file" - )) - })? - }; - + let bytes = self.download_file_bytes(remote_path).await?; if let Some(parent) = local_path.parent() { fs::create_dir_all(parent) .await @@ -1655,24 +1653,8 @@ impl Sandbox for DockerSandbox { }) } - async fn read_file( - &self, - path: &str, - offset: Option, - limit: Option, - ) -> crate::Result { - let container_path = self.resolve_container_path(path); - let (stdout, stderr, exit_code) = self - .docker_exec(vec!["cat".to_string(), container_path.clone()], None, None) - .await?; - - if exit_code != 0 { - return Err(crate::Error::message(format!( - "Failed to read {container_path}: {stderr}" - ))); - } - - Ok(format_lines_numbered(&stdout, offset, limit)) + async fn read_file_bytes(&self, path: &str) -> crate::Result> { + self.download_file_bytes(path).await } async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index 48c799106..aae5ef9f4 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -16,7 +16,7 @@ use crate::sandbox::{StdioProcessControl, optional_timeout}; use crate::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, - StdioProcess, StdioProcessHandle, StdioProcessTermination, format_lines_numbered, + StdioProcess, StdioProcessHandle, StdioProcessTermination, }; pub struct LocalSandbox { @@ -244,18 +244,11 @@ impl StdioProcessControl for LocalStdioProcessControl { #[async_trait] impl Sandbox for LocalSandbox { - async fn read_file( - &self, - path: &str, - offset: Option, - limit: Option, - ) -> crate::Result { + async fn read_file_bytes(&self, path: &str) -> crate::Result> { let full_path = self.resolve_path(path); - let content = fs::read_to_string(&full_path).await.map_err(|e| { + fs::read(&full_path).await.map_err(|e| { crate::Error::context(format!("Failed to read {}", full_path.display()), e) - })?; - - Ok(format_lines_numbered(&content, offset, limit)) + }) } async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index 4679f972a..e5da8d796 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -61,7 +61,7 @@ pub enum GitSetupIntent { /// delegate_sandbox! { /// MyDecorator => inner { /// // Only provide methods with custom logic — the rest delegate automatically. -/// async fn read_file(&self, path: &str, offset: Option, limit: Option) -> $crate::Result { +/// async fn read_file_bytes(&self, path: &str) -> $crate::Result> { /// // custom logic... /// } /// } @@ -234,6 +234,10 @@ macro_rules! delegate_sandbox { self.$field.get_preview_url(port).await } + async fn read_file_bytes(&self, path: &str) -> $crate::Result> { + self.$field.read_file_bytes(path).await + } + async fn read_file( &self, path: &str, @@ -805,12 +809,26 @@ pub struct GrepOptions { #[async_trait] pub trait Sandbox: Send + Sync { + async fn read_file_bytes(&self, path: &str) -> crate::Result>; + + async fn read_file_text(&self, path: &str) -> crate::Result { + String::from_utf8(self.read_file_bytes(path).await?) + .map_err(|err| crate::Error::context("File is not valid UTF-8", err)) + } + async fn read_file( &self, path: &str, offset: Option, limit: Option, - ) -> crate::Result; + ) -> crate::Result { + Ok(format_lines_numbered( + &self.read_file_text(path).await?, + offset, + limit, + )) + } + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()>; async fn delete_file(&self, path: &str) -> crate::Result<()>; async fn file_exists(&self, path: &str) -> crate::Result; diff --git a/lib/crates/fabro-sandbox/src/test_support.rs b/lib/crates/fabro-sandbox/src/test_support.rs index 2553ae132..6158aab23 100644 --- a/lib/crates/fabro-sandbox/src/test_support.rs +++ b/lib/crates/fabro-sandbox/src/test_support.rs @@ -19,33 +19,31 @@ use crate::{ // --- MockSandbox --- pub struct MockSandbox { - pub files: HashMap, - pub exec_result: ExecResult, - pub grep_results: Vec, - pub glob_results: Vec, - pub working_dir: &'static str, - pub platform_str: &'static str, - pub os_version_str: String, - /// When true, `read_file` applies offset/limit by splitting on lines. - pub apply_read_offset_limit: bool, + pub files: HashMap, + pub exec_result: ExecResult, + pub grep_results: Vec, + pub glob_results: Vec, + pub working_dir: &'static str, + pub platform_str: &'static str, + pub os_version_str: String, /// Captures (path, content) pairs from `write_file` calls. - pub written_files: Mutex>, + pub written_files: Mutex>, /// Captures the `timeout_ms` argument from `exec_command` calls. - pub captured_timeout: Mutex>, + pub captured_timeout: Mutex>, /// Captures the `command` argument from `exec_command` calls (last only). - pub captured_command: Mutex>, + pub captured_command: Mutex>, /// Captures all `command` arguments from `exec_command` calls in order. - pub captured_commands: Mutex>, + pub captured_commands: Mutex>, /// Captures all `working_dir` arguments from `exec_command` calls in order. - pub captured_working_dirs: Mutex>>, + pub captured_working_dirs: Mutex>>, /// Captures the `env_vars` argument from `exec_command` calls. - pub captured_env_vars: Mutex>>, - pub start_calls: Mutex, - pub stop_calls: Mutex, - pub delete_calls: Mutex, - pub event_callback: Option, - pub stdio_process_error: Option, - pub stdio_process: Mutex>, + pub captured_env_vars: Mutex>>, + pub start_calls: Mutex, + pub stop_calls: Mutex, + pub delete_calls: Mutex, + pub event_callback: Option, + pub stdio_process_error: Option, + pub stdio_process: Mutex>, } impl MockSandbox { @@ -93,32 +91,31 @@ impl MockSandbox { impl Default for MockSandbox { fn default() -> Self { Self { - files: HashMap::new(), - exec_result: ExecResult { + files: HashMap::new(), + exec_result: ExecResult { stdout: "mock output".into(), stderr: String::new(), exit_code: Some(0), termination: CommandTermination::Exited, duration_ms: 10, }, - grep_results: vec![], - glob_results: vec![], - working_dir: "/work", - platform_str: "darwin", - os_version_str: "Darwin 24.0.0".into(), - apply_read_offset_limit: false, - written_files: Mutex::new(Vec::new()), - captured_timeout: Mutex::new(None), - captured_command: Mutex::new(None), - captured_commands: Mutex::new(Vec::new()), - captured_working_dirs: Mutex::new(Vec::new()), - captured_env_vars: Mutex::new(None), - start_calls: Mutex::new(0), - stop_calls: Mutex::new(0), - delete_calls: Mutex::new(0), - event_callback: None, - stdio_process_error: None, - stdio_process: Mutex::new(None), + grep_results: vec![], + glob_results: vec![], + working_dir: "/work", + platform_str: "darwin", + os_version_str: "Darwin 24.0.0".into(), + written_files: Mutex::new(Vec::new()), + captured_timeout: Mutex::new(None), + captured_command: Mutex::new(None), + captured_commands: Mutex::new(Vec::new()), + captured_working_dirs: Mutex::new(Vec::new()), + captured_env_vars: Mutex::new(None), + start_calls: Mutex::new(0), + stop_calls: Mutex::new(0), + delete_calls: Mutex::new(0), + event_callback: None, + stdio_process_error: None, + stdio_process: Mutex::new(None), } } } @@ -177,27 +174,11 @@ impl StdioProcessControl for MockStdioProcessControl { #[async_trait] impl Sandbox for MockSandbox { - async fn read_file( - &self, - path: &str, - offset: Option, - limit: Option, - ) -> crate::Result { - let content = self - .files + async fn read_file_bytes(&self, path: &str) -> crate::Result> { + self.files .get(path) - .cloned() - .ok_or_else(|| crate::Error::message(format!("File not found: {path}")))?; - - if self.apply_read_offset_limit { - let lines: Vec<&str> = content.lines().collect(); - let start = offset.unwrap_or(1).saturating_sub(1); - let count = limit.unwrap_or(2000); - let selected: Vec<&str> = lines.into_iter().skip(start).take(count).collect(); - Ok(selected.join("\n")) - } else { - Ok(content) - } + .map(|content| content.as_bytes().to_vec()) + .ok_or_else(|| crate::Error::message(format!("File not found: {path}"))) } async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { @@ -442,17 +423,12 @@ impl MutableMockSandbox { #[async_trait] impl Sandbox for MutableMockSandbox { - async fn read_file( - &self, - path: &str, - _offset: Option, - _limit: Option, - ) -> crate::Result { + async fn read_file_bytes(&self, path: &str) -> crate::Result> { self.files .lock() .expect("files lock poisoned") .get(path) - .cloned() + .map(|content| content.as_bytes().to_vec()) .ok_or_else(|| crate::Error::message(format!("File not found: {path}"))) } diff --git a/lib/crates/fabro-sandbox/src/worktree.rs b/lib/crates/fabro-sandbox/src/worktree.rs index 39ef168c7..fb0eacf0a 100644 --- a/lib/crates/fabro-sandbox/src/worktree.rs +++ b/lib/crates/fabro-sandbox/src/worktree.rs @@ -282,6 +282,11 @@ impl Sandbox for WorktreeSandbox { // --- Delegated methods --- + async fn read_file_bytes(&self, path: &str) -> crate::Result> { + let resolved = self.resolve_path(path); + self.inner.read_file_bytes(&resolved).await + } + async fn read_file( &self, path: &str, diff --git a/lib/crates/fabro-server/src/run_files.rs b/lib/crates/fabro-server/src/run_files.rs index 835b386c4..e8416bfc1 100644 --- a/lib/crates/fabro-server/src/run_files.rs +++ b/lib/crates/fabro-server/src/run_files.rs @@ -1785,12 +1785,7 @@ diff --git a/src/live.rs b/src/live.rs }) } - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> fabro_sandbox::Result { + async fn read_file_bytes(&self, _path: &str) -> fabro_sandbox::Result> { unimplemented!() } async fn write_file(&self, _: &str, _: &str) -> fabro_sandbox::Result<()> { @@ -2970,12 +2965,7 @@ rename to .env.production // Unused by fetch_blob_table — panic loudly if anything tries to // use this sandbox beyond cat-file. - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> SandboxResult { + async fn read_file_bytes(&self, _path: &str) -> SandboxResult> { unimplemented!() } async fn write_file(&self, _: &str, _: &str) -> SandboxResult<()> { diff --git a/lib/crates/fabro-workflow/src/artifact.rs b/lib/crates/fabro-workflow/src/artifact.rs index 510a37ab5..3f9f9eb7d 100644 --- a/lib/crates/fabro-workflow/src/artifact.rs +++ b/lib/crates/fabro-workflow/src/artifact.rs @@ -592,12 +592,7 @@ mod tests { #[async_trait::async_trait] impl Sandbox for TestSyncEnv { - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> fabro_sandbox::Result { + async fn read_file_bytes(&self, _path: &str) -> fabro_sandbox::Result> { Err("not implemented".into()) } diff --git a/lib/crates/fabro-workflow/src/artifact_snapshot.rs b/lib/crates/fabro-workflow/src/artifact_snapshot.rs index d35be8309..a837ca45a 100644 --- a/lib/crates/fabro-workflow/src/artifact_snapshot.rs +++ b/lib/crates/fabro-workflow/src/artifact_snapshot.rs @@ -395,12 +395,7 @@ mod tests { #[async_trait::async_trait] impl Sandbox for AssetMockSandbox { - async fn read_file( - &self, - _: &str, - _: Option, - _: Option, - ) -> fabro_sandbox::Result { + async fn read_file_bytes(&self, _: &str) -> fabro_sandbox::Result> { Err("not implemented".into()) } async fn write_file(&self, _: &str, _: &str) -> fabro_sandbox::Result<()> { diff --git a/lib/crates/fabro-workflow/src/devcontainer_bridge.rs b/lib/crates/fabro-workflow/src/devcontainer_bridge.rs index 5d9cc092d..b7ec92107 100644 --- a/lib/crates/fabro-workflow/src/devcontainer_bridge.rs +++ b/lib/crates/fabro-workflow/src/devcontainer_bridge.rs @@ -277,13 +277,8 @@ mod tests { #[async_trait] impl Sandbox for TestSandbox { - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> fabro_sandbox::Result { - Ok(String::new()) + async fn read_file_bytes(&self, _path: &str) -> fabro_sandbox::Result> { + Ok(Vec::new()) } async fn write_file(&self, _path: &str, _content: &str) -> fabro_sandbox::Result<()> { Ok(()) diff --git a/lib/crates/fabro-workflow/src/handler/command.rs b/lib/crates/fabro-workflow/src/handler/command.rs index 9c89f7f06..8e110e202 100644 --- a/lib/crates/fabro-workflow/src/handler/command.rs +++ b/lib/crates/fabro-workflow/src/handler/command.rs @@ -920,12 +920,7 @@ mod tests { #[async_trait::async_trait] impl fabro_agent::sandbox::Sandbox for SpySandbox { - async fn read_file( - &self, - _: &str, - _: Option, - _: Option, - ) -> fabro_sandbox::Result { + async fn read_file_bytes(&self, _: &str) -> fabro_sandbox::Result> { unimplemented!() } async fn write_file(&self, _: &str, _: &str) -> fabro_sandbox::Result<()> { diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index cd7dfe7c4..b746e2f89 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -915,12 +915,7 @@ mod tests { #[async_trait] impl Sandbox for ScriptedSandbox { - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> fabro_sandbox::Result { + async fn read_file_bytes(&self, _path: &str) -> fabro_sandbox::Result> { Err("read_file not implemented for ScriptedSandbox".into()) } diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 13c3c1900..86dab5cd7 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -9224,12 +9224,7 @@ impl RemoteMockEnv { #[async_trait::async_trait] impl fabro_agent::Sandbox for RemoteMockEnv { - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> fabro_sandbox::Result { + async fn read_file_bytes(&self, _path: &str) -> fabro_sandbox::Result> { Err("not implemented".into()) }