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 <noreply@anthropic.com>

* Fix clippy warnings after merging main

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: arc <arc@local>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

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<dyn crate::Sandbox> = Arc::new(LocalSandbox::new(cwd));
+    let env: Arc<dyn crate::Sandbox> = Arc::new(crate::ReadBeforeWriteSandbox::new(Arc::new(
+        LocalSandbox::new(cwd),
+    )));
 
     // Build tool approval callback
     let permissions = args.permissions.unwrap_or(PermissionLevel::ReadWrite);

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<dyn Sandbox>,
     read_set: Mutex<HashSet<String>>,
@@ -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<usize>,
-            limit: Option<usize>,
-        ) -> Result<String, String> {
-            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<Vec<String>, 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 {

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<usize>,
+                limit: Option<usize>,
+            ) -> Result<String, String> {
+                self.$field.read_file(path, offset, limit).await
+            }
+
+            async fn grep(
+                &self,
+                pattern: &str,
+                path: &str,
+                options: &$crate::sandbox::GrepOptions,
+            ) -> Result<Vec<String>, 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)]

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<dyn Sandbox> = 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<dyn Sandbox> = 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"
+        );
+    }
 }

crates/arc-api/src/server.rs

@@ -558,7 +558,9 @@ async fn execute_run(state: Arc<AppState>, run_id: String) {
 
     let registry = (state.registry_factory)(Arc::clone(&interviewer) as Arc<dyn Interviewer>);
     let cwd = std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from("."));
-    let sandbox: Arc<dyn arc_agent::Sandbox> = Arc::new(LocalSandbox::new(cwd));
+    let sandbox: Arc<dyn arc_agent::Sandbox> = 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]

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]

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");
     }
 

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<dyn Sandbox> = Arc::new(arc_agent::ReadBeforeWriteSandbox::new(sandbox));
+
     // Initialize sandbox (creates sandbox/container once for the whole run)
     sandbox
         .initialize()

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,
         }
     }

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"));

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()
     );