From ff053e1e4ac019600e6f956e21cea01e7d85c117 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Sun, 8 Mar 2026 16:19:22 -0400 Subject: [PATCH] Fix ReadBeforeWriteSandbox (#1) * arc(01KK60MHXX1G7Y2H0YN6HAWWZH): start (success) Arc-Run: 01KK60MHXX1G7Y2H0YN6HAWWZH Arc-Completed: 1 Arc-Checkpoint: be0355a55571c147ac954b15ac0d5605d63539e8 * arc(01KK60MHXX1G7Y2H0YN6HAWWZH): implement (success) Arc-Run: 01KK60MHXX1G7Y2H0YN6HAWWZH Arc-Completed: 2 Arc-Checkpoint: b67ee066bf67ce3c36bbdf31833e4303065de311 * arc(01KK60MHXX1G7Y2H0YN6HAWWZH): simplify (success) Arc-Run: 01KK60MHXX1G7Y2H0YN6HAWWZH Arc-Completed: 3 Arc-Checkpoint: 69fbd818f537771b9d7f8b43339e31fe4fa63321 * Fix formatting in cli.rs and tools.rs Co-Authored-By: Claude Opus 4.6 * Fix clippy warnings after merging main Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: arc Co-authored-by: Claude Opus 4.6 --- crates/arc-agent/src/cli.rs | 4 +- .../src/read_before_write_sandbox.rs | 87 +++++++++++-------- crates/arc-agent/src/sandbox.rs | 23 +++++ crates/arc-agent/src/tools.rs | 73 ++++++++++++++++ crates/arc-api/src/server.rs | 8 +- crates/arc-api/src/server_config.rs | 4 +- crates/arc-api/src/sessions.rs | 2 +- crates/arc-workflows/src/cli/run.rs | 3 + crates/arc-workflows/src/hook/runner.rs | 2 +- crates/arc-workflows/src/preamble.rs | 10 +-- crates/arc-workflows/tests/integration.rs | 2 +- 11 files changed, 166 insertions(+), 52 deletions(-) diff --git a/crates/arc-agent/src/cli.rs b/crates/arc-agent/src/cli.rs index 09446c31b..bb192c229 100644 --- a/crates/arc-agent/src/cli.rs +++ b/crates/arc-agent/src/cli.rs @@ -394,7 +394,9 @@ pub async fn run_with_args(args: AgentArgs) -> anyhow::Result<()> { // Build sandbox let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); let cwd_str = cwd.to_string_lossy().to_string(); - let env: Arc = Arc::new(LocalSandbox::new(cwd)); + let env: Arc = Arc::new(crate::ReadBeforeWriteSandbox::new(Arc::new( + LocalSandbox::new(cwd), + ))); // Build tool approval callback let permissions = args.permissions.unwrap_or(PermissionLevel::ReadWrite); diff --git a/crates/arc-agent/src/read_before_write_sandbox.rs b/crates/arc-agent/src/read_before_write_sandbox.rs index 0b2e928c4..5da4a648f 100644 --- a/crates/arc-agent/src/read_before_write_sandbox.rs +++ b/crates/arc-agent/src/read_before_write_sandbox.rs @@ -2,12 +2,14 @@ use crate::sandbox::*; use std::collections::HashSet; use std::path::{Component, PathBuf}; use std::sync::{Arc, Mutex}; +use tracing::{debug, warn}; /// Decorator that prevents writing to files the agent hasn't read first. /// -/// Tracks which file paths the agent has seen (via `read_file` or `grep`) and -/// returns an error when `write_file` or `delete_file` targets an existing file -/// that hasn't been read. Writing to new (non-existent) files is always allowed. +/// Tracks which file paths the agent has seen (via `mark_agent_read`, called by +/// tool executors after agent-visible reads) and returns an error when `write_file` +/// or `delete_file` targets an existing file that hasn't been read. +/// Writing to new (non-existent) files is always allowed. pub struct ReadBeforeWriteSandbox { inner: Arc, read_set: Mutex>, @@ -61,6 +63,7 @@ impl ReadBeforeWriteSandbox { async fn guard_write(&self, path: &str) -> Result<(), String> { let exists = self.inner.file_exists(path).await?; if exists && !self.has_read(path) { + warn!(path = %path, "Write blocked: file not read by agent"); Err(format!( "Cannot write to '{path}': file exists but has not been read. \ Use read_file to read the file before writing to it." @@ -73,17 +76,6 @@ impl ReadBeforeWriteSandbox { crate::delegate_sandbox! { ReadBeforeWriteSandbox => inner { - async fn read_file( - &self, - path: &str, - offset: Option, - limit: Option, - ) -> Result { - let result = self.inner.read_file(path, offset, limit).await?; - self.mark_read(path); - Ok(result) - } - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { self.guard_write(path).await?; self.inner.write_file(path, content).await @@ -94,21 +86,9 @@ crate::delegate_sandbox! { self.inner.delete_file(path).await } - async fn grep( - &self, - pattern: &str, - path: &str, - options: &GrepOptions, - ) -> Result, String> { - let results = self.inner.grep(pattern, path, options).await?; - for line in &results { - if let Some(file_path) = line.split(':').next() { - if !file_path.is_empty() { - self.mark_read(file_path); - } - } - } - Ok(results) + fn mark_agent_read(&self, path: &str) { + debug!(path = %path, "File marked as agent-read"); + self.mark_read(path); } } } @@ -147,7 +127,7 @@ mod tests { assert!(result.is_ok()); } - // Cycle 3: read then write → success + // Cycle 3: mark_agent_read then write → success #[tokio::test] async fn read_then_write_succeeds() { let mock = MockSandbox { @@ -156,15 +136,30 @@ mod tests { }; let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); + env.mark_agent_read("a.ts"); + let result = env.write_file("a.ts", "new content").await; + + assert!(result.is_ok()); + } + + // Cycle 4: read_file alone does NOT satisfy guard + #[tokio::test] + async fn read_file_alone_does_not_satisfy_guard() { + let mock = MockSandbox { + files: HashMap::from([("a.ts".into(), "content".into())]), + ..Default::default() + }; + let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); + env.read_file("a.ts", None, None).await.unwrap(); let result = env.write_file("a.ts", "new content").await; - assert!(result.is_ok()); + assert!(result.is_err()); } - // Cycle 4: grep results populate read set + // Cycle 5: grep alone does NOT populate read set #[tokio::test] - async fn grep_populates_read_set() { + async fn grep_does_not_populate_read_set() { let mock = MockSandbox { files: HashMap::from([("b.ts".into(), "content".into())]), grep_results: vec!["b.ts:1:content".into()], @@ -177,10 +172,26 @@ mod tests { .unwrap(); let result = env.write_file("b.ts", "new").await; + assert!(result.is_err()); + } + + // Cycle 6: mark_agent_read from grep results then write → success + #[tokio::test] + async fn mark_agent_read_from_grep_then_write_succeeds() { + let mock = MockSandbox { + files: HashMap::from([("b.ts".into(), "content".into())]), + grep_results: vec!["b.ts:1:content".into()], + ..Default::default() + }; + let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); + + env.mark_agent_read("b.ts"); + let result = env.write_file("b.ts", "new").await; + assert!(result.is_ok()); } - // Cycle 5: glob does NOT populate read set + // Cycle 7: glob does NOT populate read set #[tokio::test] async fn glob_does_not_populate_read_set() { let mock = MockSandbox { @@ -196,7 +207,7 @@ mod tests { assert!(result.is_err()); } - // Cycle 6: path normalization — relative vs absolute + // Cycle 8: path normalization — relative vs absolute via mark_agent_read #[tokio::test] async fn path_normalization_relative_and_absolute() { let mock = MockSandbox { @@ -209,13 +220,13 @@ mod tests { }; let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - env.read_file("a.ts", None, None).await.unwrap(); + env.mark_agent_read("a.ts"); let result = env.write_file("/work/a.ts", "new content").await; assert!(result.is_ok()); } - // Cycle 7: delete unread file → error + // Cycle 9: delete unread file → error #[tokio::test] async fn delete_unread_file_returns_error() { let mock = MockSandbox { @@ -229,7 +240,7 @@ mod tests { assert!(result.is_err()); } - // Cycle 8: error message is actionable + // Cycle 10: error message is actionable #[tokio::test] async fn error_message_is_actionable() { let mock = MockSandbox { diff --git a/crates/arc-agent/src/sandbox.rs b/crates/arc-agent/src/sandbox.rs index 8167d510b..21516a479 100644 --- a/crates/arc-agent/src/sandbox.rs +++ b/crates/arc-agent/src/sandbox.rs @@ -100,6 +100,24 @@ macro_rules! delegate_sandbox { async fn set_autostop_interval(&self, minutes: i32) -> Result<(), String> { self.$field.set_autostop_interval(minutes).await } + + async fn read_file( + &self, + path: &str, + offset: Option, + limit: Option, + ) -> Result { + self.$field.read_file(path, offset, limit).await + } + + async fn grep( + &self, + pattern: &str, + path: &str, + options: &$crate::sandbox::GrepOptions, + ) -> Result, String> { + self.$field.grep(pattern, path, options).await + } } }; } @@ -349,6 +367,11 @@ pub trait Sandbox: Send + Sync { async fn set_autostop_interval(&self, _minutes: i32) -> Result<(), String> { Ok(()) } + + /// Record that the agent has explicitly read (seen) the given file path. + /// Called by tool executors after agent-visible reads (e.g. `read_file`, `grep`). + /// Default is a no-op; `ReadBeforeWriteSandbox` overrides to populate its read set. + fn mark_agent_read(&self, _path: &str) {} } #[cfg(test)] diff --git a/crates/arc-agent/src/tools.rs b/crates/arc-agent/src/tools.rs index 9ab91a671..4e7369875 100644 --- a/crates/arc-agent/src/tools.rs +++ b/crates/arc-agent/src/tools.rs @@ -95,6 +95,7 @@ pub fn make_read_file_tool() -> RegisteredTool { .env .read_file(file_path, offset_usize, limit_usize) .await?; + ctx.env.mark_agent_read(file_path); Ok(content) }) }), @@ -291,6 +292,14 @@ pub fn make_grep_tool() -> RegisteredTool { }; let results = ctx.env.grep(pattern, path, &options).await?; + let mut seen_files = std::collections::HashSet::new(); + for line in &results { + if let Some(file_path) = line.split(':').next() { + if !file_path.is_empty() && seen_files.insert(file_path) { + ctx.env.mark_agent_read(file_path); + } + } + } Ok(results.join("\n")) }) }), @@ -355,6 +364,7 @@ pub(crate) fn make_read_many_files_tool() -> RegisteredTool { .ok_or_else(|| "each path must be a string".to_string())?; match ctx.env.read_file(path, None, None).await { Ok(content) => { + ctx.env.mark_agent_read(path); let _ = write!(output, "=== {path} ===\n{content}\n\n"); } Err(err) => { @@ -1412,4 +1422,67 @@ mod tests { "results should mention rust, got: {output}" ); } + + #[tokio::test] + async fn read_file_tool_marks_agent_read() { + use crate::read_before_write_sandbox::ReadBeforeWriteSandbox; + + let mock = MockSandbox { + files: HashMap::from([("a.ts".into(), "content".into())]), + ..Default::default() + }; + let env: Arc = Arc::new(ReadBeforeWriteSandbox::new(Arc::new(mock))); + + // read_file tool should mark the file as agent-read + let tool = make_read_file_tool(); + (tool.executor)( + serde_json::json!({"file_path": "a.ts"}), + ToolContext { + env: Arc::clone(&env), + cancel: CancellationToken::new(), + tool_env: None, + }, + ) + .await + .unwrap(); + + // write_file should succeed because read_file tool marked it + let result = env.write_file("a.ts", "new content").await; + assert!( + result.is_ok(), + "write should succeed after read_file tool marks agent-read" + ); + } + + #[tokio::test] + async fn grep_tool_marks_agent_read() { + use crate::read_before_write_sandbox::ReadBeforeWriteSandbox; + + let mock = MockSandbox { + files: HashMap::from([("b.ts".into(), "content".into())]), + grep_results: vec!["b.ts:1:content".into()], + ..Default::default() + }; + let env: Arc = Arc::new(ReadBeforeWriteSandbox::new(Arc::new(mock))); + + // grep tool should mark matched files as agent-read + let tool = make_grep_tool(); + (tool.executor)( + serde_json::json!({"pattern": "content"}), + ToolContext { + env: Arc::clone(&env), + cancel: CancellationToken::new(), + tool_env: None, + }, + ) + .await + .unwrap(); + + // write_file should succeed because grep tool marked it + let result = env.write_file("b.ts", "new content").await; + assert!( + result.is_ok(), + "write should succeed after grep tool marks agent-read" + ); + } } diff --git a/crates/arc-api/src/server.rs b/crates/arc-api/src/server.rs index 25a52bf15..42860b39f 100644 --- a/crates/arc-api/src/server.rs +++ b/crates/arc-api/src/server.rs @@ -558,7 +558,9 @@ async fn execute_run(state: Arc, run_id: String) { let registry = (state.registry_factory)(Arc::clone(&interviewer) as Arc); let cwd = std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")); - let sandbox: Arc = Arc::new(LocalSandbox::new(cwd)); + let sandbox: Arc = Arc::new(arc_agent::ReadBeforeWriteSandbox::new( + Arc::new(LocalSandbox::new(cwd)), + )); let mut engine = WorkflowRunEngine::with_interviewer( registry, Arc::new(emitter), @@ -1964,7 +1966,7 @@ mod tests { assert_eq!(response.status(), StatusCode::OK); let body = body_json(response.into_body()).await; assert_eq!(body["data"].as_array().unwrap().len(), 0); - assert_eq!(body["meta"]["has_more"].as_bool().unwrap(), false); + assert!(!body["meta"]["has_more"].as_bool().unwrap()); // Start a run let req = Request::builder() @@ -1994,7 +1996,7 @@ mod tests { assert_eq!(items.len(), 1); assert_eq!(items[0]["id"].as_str().unwrap(), run_id); assert!(items[0]["status"].as_str().is_some()); - assert_eq!(body["meta"]["has_more"].as_bool().unwrap(), false); + assert!(!body["meta"]["has_more"].as_bool().unwrap()); } #[tokio::test] diff --git a/crates/arc-api/src/server_config.rs b/crates/arc-api/src/server_config.rs index 65cab07b9..a5d6af0e9 100644 --- a/crates/arc-api/src/server_config.rs +++ b/crates/arc-api/src/server_config.rs @@ -485,14 +485,14 @@ matcher = "agent_loop" fn parse_feature_flags() { let toml = "[feature_flags]\nsession_sandboxes = true"; let config: ServerConfig = toml::from_str(toml).unwrap(); - assert_eq!(config.feature_flags.session_sandboxes, true); + assert!(config.feature_flags.session_sandboxes); } #[test] fn parse_feature_flags_defaults() { let toml = ""; let config: ServerConfig = toml::from_str(toml).unwrap(); - assert_eq!(config.feature_flags.session_sandboxes, false); + assert!(!config.feature_flags.session_sandboxes); } #[test] diff --git a/crates/arc-api/src/sessions.rs b/crates/arc-api/src/sessions.rs index e3e9fa589..a64a1fc81 100644 --- a/crates/arc-api/src/sessions.rs +++ b/crates/arc-api/src/sessions.rs @@ -515,7 +515,7 @@ mod tests { assert_eq!(body["id"].as_str().unwrap(), session_id); assert!(body["turns"].is_array()); // Should have at least the initial user turn - assert!(body["turns"].as_array().unwrap().len() >= 1); + assert!(!body["turns"].as_array().unwrap().is_empty()); assert_eq!(body["turns"][0]["kind"], "user"); } diff --git a/crates/arc-workflows/src/cli/run.rs b/crates/arc-workflows/src/cli/run.rs index 297794c4e..e5a0f6bda 100644 --- a/crates/arc-workflows/src/cli/run.rs +++ b/crates/arc-workflows/src/cli/run.rs @@ -517,6 +517,9 @@ pub async fn run_command( } }; + // Wrap with ReadBeforeWriteSandbox to enforce read-before-write guard + let sandbox: Arc = Arc::new(arc_agent::ReadBeforeWriteSandbox::new(sandbox)); + // Initialize sandbox (creates sandbox/container once for the whole run) sandbox .initialize() diff --git a/crates/arc-workflows/src/hook/runner.rs b/crates/arc-workflows/src/hook/runner.rs index 1351b034a..0aa24f5e7 100644 --- a/crates/arc-workflows/src/hook/runner.rs +++ b/crates/arc-workflows/src/hook/runner.rs @@ -33,7 +33,7 @@ impl HookRunner { let compiled_matchers = Self::compile_matchers(&config); Self { config, - executor: executor, + executor, compiled_matchers, } } diff --git a/crates/arc-workflows/src/preamble.rs b/crates/arc-workflows/src/preamble.rs index d56bbac66..7ad8796ad 100644 --- a/crates/arc-workflows/src/preamble.rs +++ b/crates/arc-workflows/src/preamble.rs @@ -728,15 +728,15 @@ mod tests { let graph = Graph::new("test"); let context = Context::new(); context.set(keys::INTERNAL_FIDELITY, serde_json::json!("compact")); - context.set(&keys::retry_count_key("plan"), serde_json::json!(1)); + context.set(keys::retry_count_key("plan"), serde_json::json!(1)); context.set(keys::CURRENT_NODE, serde_json::json!("work")); context.set( - &keys::graph_attr_key("default_fidelity"), + keys::graph_attr_key("default_fidelity"), serde_json::json!("compact"), ); context.set("thread.main.current_node", serde_json::json!("work")); context.set( - &keys::response_key("plan"), + keys::response_key("plan"), serde_json::json!("some response"), ); context.set(keys::LAST_STAGE, serde_json::json!("plan")); @@ -925,12 +925,12 @@ mod tests { let graph = Graph::new("test"); let context = Context::new(); context.set( - &keys::graph_attr_key("default_fidelity"), + keys::graph_attr_key("default_fidelity"), serde_json::json!("compact"), ); context.set("thread.main.current_node", serde_json::json!("work")); context.set( - &keys::response_key("plan"), + keys::response_key("plan"), serde_json::json!("some LLM response"), ); context.set(keys::LAST_STAGE, serde_json::json!("plan")); diff --git a/crates/arc-workflows/tests/integration.rs b/crates/arc-workflows/tests/integration.rs index 8f11f209f..469324ffe 100644 --- a/crates/arc-workflows/tests/integration.rs +++ b/crates/arc-workflows/tests/integration.rs @@ -10208,7 +10208,7 @@ async fn git_checkpoint_host_emits_events_and_diff_patch() { .collect(); // work node gets a checkpoint commit (start is skipped, exit is terminal) assert!( - git_events.len() >= 1, + !git_events.is_empty(), "expected at least 1 GitCheckpoint event, got {}", git_events.len() );