diff --git a/docs/public/agents/permissions.mdx b/docs/public/agents/permissions.mdx index b27ad589f..c12503c56 100644 --- a/docs/public/agents/permissions.mdx +++ b/docs/public/agents/permissions.mdx @@ -80,15 +80,6 @@ The `shell` tool and web tools (`web_search`, `web_fetch`) are only auto-approve Sub-agent management tools (`spawn_agent`, `send_input`, `wait`, `close_agent`) are auto-approved at all permission levels. Sub-agents inherit the parent's permission level, so a `read-only` parent spawns `read-only` children. -## Read-before-write guardrail - -Independent of the permission system, Fabro enforces a **read-before-write guardrail** that applies to all permission levels. Even with `full` permissions, an agent must read an existing file before modifying or deleting it. See [Tools: Read-before-write guardrail](/agents/tools#read-before-write-guardrail) for details. - -This guardrail operates at the sandbox layer and works alongside (not instead of) the permission system. A tool call must pass **both** checks: - -1. The tool's category must be allowed by the current permission level -2. If writing to an existing file, the file must have been previously read - ## Permission flow The following sequence shows how Fabro decides whether to execute a tool call: diff --git a/docs/public/agents/tools.mdx b/docs/public/agents/tools.mdx index 693d0e5e5..373971d9b 100644 --- a/docs/public/agents/tools.mdx +++ b/docs/public/agents/tools.mdx @@ -76,7 +76,7 @@ Creates or overwrites a file. | `content` | string | yes | Content to write | -The [read-before-write guardrail](#read-before-write-guardrail) prevents writing to existing files that haven't been read first. Writing to new files is always allowed. +`write_file` replaces the entire file. For changes to an existing file, prefer `edit_file`, which only replaces the string you name. ### edit_file @@ -92,7 +92,7 @@ Replaces a string in an existing file. Available for Anthropic and Gemini provid If `old_string` is not found, the tool returns an error. If multiple occurrences exist and `replace_all` is false, the tool returns an error asking for more context or to set `replace_all`. -The tool reads the file internally before writing, so it satisfies the read-before-write guardrail automatically. +Because `old_string` must match the file exactly, an edit built from a stale or remembered version of the file fails rather than silently applying somewhere unintended. ### grep @@ -106,7 +106,7 @@ Searches file contents with a regex pattern, powered by ripgrep in the sandbox. | `case_insensitive` | boolean | no | Case-insensitive search (default: false) | | `max_results` | integer | no | Maximum number of results | -Results are returned as `file:line:content` lines. Files that appear in grep results are marked as "read" for the [read-before-write guardrail](#read-before-write-guardrail). +Results are returned as `file:line:content` lines. ### glob @@ -121,10 +121,6 @@ Returns matching file paths, one per line, sorted lexicographically by their pat Patterns are case-sensitive and relative to `path`: `*` and `?` stay within one path segment, bracket expressions such as `[abc]` match one character, and `**` crosses directories when used as a complete segment. Leading dots are matched normally. For example, `*.rs` searches only the root of `path`, and `**/*.rs` searches recursively. Patterns must use `/`, be relative, and cannot contain a backslash or `..` segment. - -Unlike `grep`, `glob` does **not** mark files as read for the read-before-write guardrail. To modify a file found via glob, the agent must read it first. - - ### web_search Searches the web using the Brave Search API. @@ -202,23 +198,6 @@ Like `update_plan`, task changes are persisted as `todo.created`, `todo.updated` When an Anthropic session has not used `TaskCreate` or `TaskUpdate` for ten assistant turns, Fabro may inject a system reminder asking the agent to keep task state current. The reminder is only added when both tools are available and resets after the agent uses either tool. -## Read-before-write guardrail - -Fabro wraps every sandbox in a `ReadBeforeWriteSandbox` decorator that tracks which files the agent has seen. The rules are: - -1. **Writing to a new file** (one that doesn't exist yet) is always allowed -2. **Writing to an existing file** requires that the agent has previously read it via `read_file` or seen it in `grep` results -3. **Deleting an existing file** follows the same rule as writing - -If an agent attempts to write to an existing file it hasn't read, the tool returns an error: - -``` -Cannot write to 'src/main.rs': file exists but has not been read. -Use read_file to read the file before writing to it. -``` - -This prevents agents from blindly overwriting files they haven't inspected. The guardrail normalizes paths so that reading `src/main.rs` (relative) and `/workspace/src/main.rs` (absolute) both satisfy the check. - ## Tool execution ### Parallel execution @@ -256,7 +235,7 @@ Common error cases: | Argument validation failure | Arguments don't match the tool's JSON Schema | | File not found | The target file doesn't exist | | Command timeout | A shell command exceeded its timeout | -| Read-before-write | The agent tried to write to a file it hasn't read (see [guardrail](#read-before-write-guardrail)) | +| `old_string` not found | An `edit_file` anchor didn't match the file's current contents | ### Timeouts diff --git a/docs/public/reference/sdk.mdx b/docs/public/reference/sdk.mdx index 84f8e72b5..651d67bcc 100644 --- a/docs/public/reference/sdk.mdx +++ b/docs/public/reference/sdk.mdx @@ -161,7 +161,6 @@ pub trait Sandbox: Send + Sync { |---|---| | `LocalSandbox` | Executes directly on the local filesystem. | | `DockerSandbox` | Runs inside a Docker container (feature-gated: `docker`). | -| `ReadBeforeWriteSandbox` | Decorator that blocks writes to files the agent hasn't read. Wraps any `Arc`. | The `DaytonaSandbox` implementation (feature-gated: `daytona`) runs inside a Daytona cloud sandbox. diff --git a/lib/components/fabro-agent/src/cli.rs b/lib/components/fabro-agent/src/cli.rs index 48dabe440..666235737 100644 --- a/lib/components/fabro-agent/src/cli.rs +++ b/lib/components/fabro-agent/src/cli.rs @@ -556,9 +556,7 @@ pub async fn run_with_args_and_client_and_catalog( // 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(crate::ReadBeforeWriteSandbox::new(Arc::new( - LocalSandbox::new(cwd), - ))); + let env: Arc = Arc::new(LocalSandbox::new(cwd)); // Build tool approval callback let permissions = args.permissions.unwrap_or(PermissionLevel::ReadWrite); diff --git a/lib/components/fabro-agent/src/lib.rs b/lib/components/fabro-agent/src/lib.rs index e8c8b01dc..c66c4bfeb 100644 --- a/lib/components/fabro-agent/src/lib.rs +++ b/lib/components/fabro-agent/src/lib.rs @@ -18,7 +18,6 @@ pub mod memory; pub mod native_tool; pub mod profiles; pub mod question_tools; -pub mod read_before_write_sandbox; pub mod sandbox; pub mod session; pub mod skills; @@ -57,7 +56,6 @@ pub use question_tools::{ AgentQuestionAnswerStatus, AgentQuestionRuntime, AgentToolRuntime, OPENAI_REQUEST_USER_INPUT_TOOL, register_question_tools, }; -pub use read_before_write_sandbox::ReadBeforeWriteSandbox; pub use sandbox::{ CommandOutputCallback, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, RefreshOutcome, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, StdioProcess, StdioProcessHandle, diff --git a/lib/components/fabro-agent/src/profiles/kimi.rs b/lib/components/fabro-agent/src/profiles/kimi.rs index f5f8f05fa..32a277671 100644 --- a/lib/components/fabro-agent/src/profiles/kimi.rs +++ b/lib/components/fabro-agent/src/profiles/kimi.rs @@ -16,16 +16,16 @@ use crate::tools::{WebFetchSummarizer, register_discovery_and_web_tools}; const CORE_PROMPT: &str = include_str!("prompts/kimi.md.j2"); -/// Kimi models repeatedly fail the workspace's read-before-write guard: across -/// two observed K3 implementation stages, 32 of 35 tool failures were writes to -/// files the model had not read, or `old_string` values reconstructed from -/// memory. Kimi Code's own tool descriptions drill this rule directly, so the -/// Kimi profile restates it where the model is most likely to act on it — in -/// the description of the tool being called — rather than relying only on the -/// system prompt. +/// Kimi models repeatedly reconstruct `old_string` from memory rather than from +/// a fresh read: across two observed K3 implementation stages, 32 of 35 tool +/// failures were edits against a file the model had not read, or `old_string` +/// values recalled from an earlier version. Kimi Code's own tool descriptions +/// drill this rule directly, so the Kimi profile restates it where the model is +/// most likely to act on it — in the description of the tool being called — +/// rather than relying only on the system prompt. const EDIT_FILE_DESCRIPTION: &str = "Edit a file by replacing an exact string. \ -Read the file with Read before EVERY edit — this workspace refuses writes to files that \ -have not been read, and the call will fail. Take old_string verbatim from the Read output; \ +Read the file with Read before EVERY edit: old_string is matched byte for byte, and only the \ +Read output tells you what it is. Take old_string verbatim from that output; \ never reconstruct it from memory or from an earlier version of the file. old_string must be an \ exact match and unique unless replace_all is true. If the edit fails with 'old_string not found', \ re-read the file and take the exact text from the fresh output rather than guessing again. \ @@ -298,7 +298,7 @@ mod tests { } #[test] - fn edit_and_write_descriptions_drill_read_before_write() { + fn edit_and_write_descriptions_drill_reading_first() { let profile = KimiProfile::new("kimi-k3"); let describe = |name: &str| { profile @@ -313,11 +313,12 @@ mod tests { for name in ["Edit", "Write"] { let text = describe(name); assert!( - text.contains("have not been read") || text.contains("has not been read"), - "{name} should warn about the read-before-write guard" + text.contains("Read"), + "{name} should steer the model to read the file first" ); } assert!(describe("Edit").contains("never reconstruct it from memory")); + assert!(describe("Write").contains("everything you do not restate")); // Bash steers shell usage toward the dedicated tools, under the names // this profile actually exposes. @@ -342,8 +343,8 @@ mod tests { // Fabro has no background shell; promising one would be a lie. assert!(!bash.contains("run_in_background"), "{bash}"); - // Read explains that reading is what clears a file for writing. - assert!(describe("Read").contains("refuse a file that has not been read")); + // Read explains why reading precedes editing. + assert!(describe("Read").contains("matches `old_string` byte for byte")); // Grep must not promise ripgrep syntax: fabro falls back to POSIX grep. let grep = describe("Grep"); assert!(grep.contains("POSIX"), "{grep}"); diff --git a/lib/components/fabro-agent/src/profiles/kimi_tools.rs b/lib/components/fabro-agent/src/profiles/kimi_tools.rs index 30c9e48e3..fb49eff6c 100644 --- a/lib/components/fabro-agent/src/profiles/kimi_tools.rs +++ b/lib/components/fabro-agent/src/profiles/kimi_tools.rs @@ -15,7 +15,7 @@ //! //! Everything these tools do reaches the environment through the same //! [`Sandbox`](crate::sandbox::Sandbox) methods the built-ins use, so sandbox -//! behavior, path policy, and the read-before-write guard are unchanged. +//! behavior and path policy are unchanged. use std::collections::{HashMap, HashSet}; use std::fmt::Write as _; @@ -159,8 +159,8 @@ pub fn make_kimi_read_tool() -> RegisteredTool { NativeTool::ReadFile, "Read a text file from the workspace. -Reading a file is also what clears it for writing: Edit and Write refuse a file that has not been \ -read in this session. +Read a file before editing it: Edit matches `old_string` byte for byte, and only the Read output \ +tells you what that string is. - If you have a concrete path, call Read directly. Do not Glob or `ls` first to check that it \ exists — a missing path returns an error you can handle. @@ -238,7 +238,6 @@ returns the last 100 lines. } .map_err(|e| e.display_with_causes())?; - ctx.env.mark_agent_read(path); Ok(content) }) }), @@ -262,8 +261,8 @@ pub fn make_kimi_write_tool() -> RegisteredTool { NativeTool::WriteFile, "Create, append to, or replace a file. -Read an existing file with Read before writing to it — this workspace refuses writes to files \ -that have not been read, and the call will fail. +Read an existing file with Read before writing to it: overwrite replaces everything you do not \ +restate, so anything you have not seen is what you stand to lose. - `mode` defaults to `overwrite`, which replaces the whole file. `append` requires an existing file \ and adds to its end without inserting a newline. @@ -328,7 +327,7 @@ overwrite replaces everything you did not restate. /// Kimi Code's `Edit` schema names the target `path`; fabro's shared edit /// executor calls it `file_path`. Translate only that adapter field and reuse -/// the exact-match/read-before-write implementation. +/// the exact-match implementation. #[must_use] pub fn make_kimi_edit_tool(description: &str) -> RegisteredTool { let shared = make_edit_file_tool(); @@ -513,7 +512,6 @@ mod tests { #[tokio::test] async fn edit_translates_kimi_path_to_the_shared_executor() { let env = sandbox_with("/f.txt", "before"); - env.mark_agent_read("/f.txt"); let tool = make_kimi_edit_tool("Edit"); (tool.executor)( diff --git a/lib/components/fabro-agent/src/profiles/mod.rs b/lib/components/fabro-agent/src/profiles/mod.rs index 9defbf54f..ee0da68c1 100644 --- a/lib/components/fabro-agent/src/profiles/mod.rs +++ b/lib/components/fabro-agent/src/profiles/mod.rs @@ -538,8 +538,8 @@ mod tests { // The specific Kimi-only phrasing must not appear elsewhere. assert!( - !anthropic_text.contains("has not been read"), - "read-before-write drilling leaked into {tool} for other profiles" + !anthropic_text.contains("never reconstruct it from memory"), + "Kimi read-before-edit drilling leaked into {tool} for other profiles" ); } } diff --git a/lib/components/fabro-agent/src/profiles/prompts/kimi.md.j2 b/lib/components/fabro-agent/src/profiles/prompts/kimi.md.j2 index 0c5c0f041..1e017430a 100644 --- a/lib/components/fabro-agent/src/profiles/prompts/kimi.md.j2 +++ b/lib/components/fabro-agent/src/profiles/prompts/kimi.md.j2 @@ -26,13 +26,13 @@ When a tool call fails, diagnose why before acting again: read the error, check # Reading Before Writing -This workspace refuses writes to files you have not read. `Edit` and `Write` both fail with "file exists but has not been read" when you target an existing file without reading it first. That failure costs a full turn and teaches you nothing you could not have known. +`Edit` replaces an exact string, so an `old_string` that does not match the file byte for byte fails. Reading first is how you get a string that matches. - Call `Read` on the target before every `Edit` or `Write` against a file that already exists. No exceptions, including small or "obvious" edits. - Take `old_string` and `new_string` from what `Read` actually returned. Never construct `old_string` from memory, from an earlier version of the file, or from what you expect the file to contain. - If an `Edit` fails with "old_string not found", do not guess a different string. Re-read the file and take the exact text from the fresh output. - After you edit a file, its contents have changed. Re-read before your next edit to the same file rather than assuming your own edit landed as written. -- `Write` fully replaces a file. Use it only for new files or a deliberate complete rewrite; for every incremental change use `Edit`. +- `Write` fully replaces a file, and everything you do not restate is lost. Use it only for new files or a deliberate complete rewrite; for every incremental change use `Edit`. # Tracking Multi-Step Work diff --git a/lib/components/fabro-agent/src/read_before_write_sandbox.rs b/lib/components/fabro-agent/src/read_before_write_sandbox.rs deleted file mode 100644 index 11fd2d523..000000000 --- a/lib/components/fabro-agent/src/read_before_write_sandbox.rs +++ /dev/null @@ -1 +0,0 @@ -pub use fabro_sandbox::read_guard::ReadBeforeWriteSandbox; diff --git a/lib/components/fabro-agent/src/tool_execution.rs b/lib/components/fabro-agent/src/tool_execution.rs index 259d04530..2a2c78b1c 100644 --- a/lib/components/fabro-agent/src/tool_execution.rs +++ b/lib/components/fabro-agent/src/tool_execution.rs @@ -570,13 +570,9 @@ mod tests { AgentQuestion, AgentQuestionAnswer, AgentQuestionAnswerStatus, AgentQuestionRuntime, AgentToolRuntime, register_question_tools, }; - use crate::read_before_write_sandbox::ReadBeforeWriteSandbox; - use crate::test_support::{MockSandbox, MutableMockSandbox}; + use crate::test_support::MockSandbox; use crate::tool_registry::{RegisteredTool, ToolContext, ToolRegistry, ToolSource}; - use crate::tools::{ - make_edit_file_tool, make_grep_tool, make_read_file_tool, make_shell_tool, - make_write_file_tool, - }; + use crate::tools::make_shell_tool; use crate::types::SessionEvent; struct NamedPolicy { @@ -1091,213 +1087,6 @@ mod tests { assert_eq!(*executions.lock().unwrap(), 0); } - // --- ReadBeforeWriteSandbox e2e tests --- - - fn make_guarded_sandbox(files: HashMap) -> Arc { - Arc::new(ReadBeforeWriteSandbox::new(Arc::new( - MutableMockSandbox::new(files), - ))) - } - - #[tokio::test] - async fn write_to_unread_file_blocked() { - let mut registry = ToolRegistry::new(); - registry.register(make_write_file_tool()); - - let sandbox = make_guarded_sandbox(HashMap::from([("a.ts".into(), "content".into())])); - let tc = make_tool_call( - "write_file", - "call_1", - serde_json::json!({"file_path": "a.ts", "content": "new"}), - ); - let emitter = Emitter::new(); - let config = SessionOptions::default(); - - let result = execute_and_emit_one_tool( - &tc, - ®istry, - sandbox, - None, - CancellationToken::new(), - &config, - &emitter, - "test-session", - "test-session", - None, - ) - .await; - - assert!(result.is_error); - assert!(result.content.to_string().contains("has not been read")); - } - - #[tokio::test] - async fn read_then_write_succeeds() { - let mut registry = ToolRegistry::new(); - registry.register(make_read_file_tool()); - registry.register(make_write_file_tool()); - - let sandbox = make_guarded_sandbox(HashMap::from([("a.ts".into(), "content".into())])); - let emitter = Emitter::new(); - let config = SessionOptions::default(); - - // First read the file - let read_tc = make_tool_call( - "read_file", - "call_1", - serde_json::json!({"file_path": "a.ts"}), - ); - let read_result = execute_and_emit_one_tool( - &read_tc, - ®istry, - sandbox.clone(), - None, - CancellationToken::new(), - &config, - &emitter, - "test-session", - "test-session", - None, - ) - .await; - assert!(!read_result.is_error); - - // Then write should succeed - let write_tc = make_tool_call( - "write_file", - "call_2", - serde_json::json!({"file_path": "a.ts", "content": "new"}), - ); - let write_result = execute_and_emit_one_tool( - &write_tc, - ®istry, - sandbox, - None, - CancellationToken::new(), - &config, - &emitter, - "test-session", - "test-session", - None, - ) - .await; - - assert!(!write_result.is_error); - } - - #[tokio::test] - async fn grep_then_write_succeeds() { - let mut registry = ToolRegistry::new(); - registry.register(make_grep_tool()); - registry.register(make_write_file_tool()); - - let sandbox = make_guarded_sandbox(HashMap::from([("a.ts".into(), "content".into())])); - let emitter = Emitter::new(); - let config = SessionOptions::default(); - - // Grep matching a.ts - let grep_tc = make_tool_call("grep", "call_1", serde_json::json!({"pattern": "content"})); - let grep_result = execute_and_emit_one_tool( - &grep_tc, - ®istry, - sandbox.clone(), - None, - CancellationToken::new(), - &config, - &emitter, - "test-session", - "test-session", - None, - ) - .await; - assert!(!grep_result.is_error); - - // Then write should succeed - let write_tc = make_tool_call( - "write_file", - "call_2", - serde_json::json!({"file_path": "a.ts", "content": "new"}), - ); - let write_result = execute_and_emit_one_tool( - &write_tc, - ®istry, - sandbox, - None, - CancellationToken::new(), - &config, - &emitter, - "test-session", - "test-session", - None, - ) - .await; - - assert!(!write_result.is_error); - } - - #[tokio::test] - async fn edit_unread_file_blocked() { - let mut registry = ToolRegistry::new(); - registry.register(make_edit_file_tool()); - - let sandbox = make_guarded_sandbox(HashMap::from([("a.ts".into(), "content".into())])); - let tc = make_tool_call( - "edit_file", - "call_1", - serde_json::json!({"file_path": "a.ts", "old_string": "content", "new_string": "updated"}), - ); - let emitter = Emitter::new(); - let config = SessionOptions::default(); - - let result = execute_and_emit_one_tool( - &tc, - ®istry, - sandbox, - None, - CancellationToken::new(), - &config, - &emitter, - "test-session", - "test-session", - None, - ) - .await; - - assert!(result.is_error); - assert!(result.content.to_string().contains("has not been read")); - } - - #[tokio::test] - async fn write_new_file_succeeds() { - let mut registry = ToolRegistry::new(); - registry.register(make_write_file_tool()); - - let sandbox = make_guarded_sandbox(HashMap::new()); - let tc = make_tool_call( - "write_file", - "call_1", - serde_json::json!({"file_path": "new.ts", "content": "hello"}), - ); - let emitter = Emitter::new(); - let config = SessionOptions::default(); - - let result = execute_and_emit_one_tool( - &tc, - ®istry, - sandbox, - None, - CancellationToken::new(), - &config, - &emitter, - "test-session", - "test-session", - None, - ) - .await; - - assert!(!result.is_error); - } - fn shell_sandbox(result: fabro_sandbox::ExecResult) -> Arc { Arc::new(MockSandbox { exec_result: result, diff --git a/lib/components/fabro-agent/src/tools.rs b/lib/components/fabro-agent/src/tools.rs index 82deb68e7..b187a8f87 100644 --- a/lib/components/fabro-agent/src/tools.rs +++ b/lib/components/fabro-agent/src/tools.rs @@ -138,7 +138,6 @@ pub fn make_read_file_tool() -> RegisteredTool { .read_file(file_path, offset_usize, limit_usize) .await .map_err(|e| e.display_with_causes())?; - ctx.env.mark_agent_read(file_path); Ok(content) }) }), @@ -441,27 +440,20 @@ pub fn make_grep_tool() -> RegisteredTool { } } -/// Run a content search and mark every returned file as observed by the -/// agent's read-before-write guard. +/// Run a content search, rendering sandbox failures as tool-result strings. +/// +/// Shared by the canonical `grep` tool and the Kimi profile's `Grep`, which +/// group the same result lines differently. pub(crate) async fn execute_grep( ctx: &ToolContext, pattern: &str, path: &str, options: &GrepOptions, ) -> Result, String> { - let results = ctx - .env + ctx.env .grep(pattern, path, options) .await - .map_err(|e| e.display_with_causes())?; - let mut seen_files = std::collections::HashSet::new(); - for line in &results { - let file_path = grep_result_path(line, path); - if !file_path.is_empty() && seen_files.insert(file_path) { - ctx.env.mark_agent_read(file_path); - } - } - Ok(results) + .map_err(|e| e.display_with_causes()) } /// Extract the file path from `::` grep output. @@ -563,7 +555,6 @@ pub(crate) fn make_read_many_files_tool() -> RegisteredTool { for (path, result) in results { match result { Ok(content) => { - ctx.env.mark_agent_read(&path); let _ = write!(output, "=== {path} ===\n{content}\n\n"); } Err(err) => { @@ -2286,69 +2277,4 @@ 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_provider: None, - session_id: None, - root_session_id: None, - tool_call_id: None, - agent_event_emitter: 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_provider: None, - session_id: None, - root_session_id: None, - tool_call_id: None, - agent_event_emitter: 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/lib/components/fabro-sandbox/src/lib.rs b/lib/components/fabro-sandbox/src/lib.rs index 9d85e8ded..edf567ada 100644 --- a/lib/components/fabro-sandbox/src/lib.rs +++ b/lib/components/fabro-sandbox/src/lib.rs @@ -12,8 +12,6 @@ mod clone_source; #[cfg(any(feature = "docker", feature = "daytona", test))] mod managed_labels; -pub mod read_guard; - #[cfg(any(feature = "docker", feature = "daytona", test))] pub mod redact; @@ -48,7 +46,6 @@ pub use provider::{ LocalSandboxProvider, SandboxCreateSpec, SandboxLookupError, SandboxProvider, SandboxProviderRegistry, }; -pub use read_guard::ReadBeforeWriteSandbox; pub use reconnect::{reconnect, reconnect_for_run, reconnect_for_run_with_callback}; pub use sandbox::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, diff --git a/lib/components/fabro-sandbox/src/read_guard.rs b/lib/components/fabro-sandbox/src/read_guard.rs deleted file mode 100644 index 588bd960d..000000000 --- a/lib/components/fabro-sandbox/src/read_guard.rs +++ /dev/null @@ -1,298 +0,0 @@ -use std::collections::HashSet; -use std::path::{Component, PathBuf}; -use std::sync::{Arc, Mutex}; - -use tracing::{debug, warn}; - -use crate::Sandbox; - -/// Decorator that prevents writing to files the agent hasn't read first. -/// -/// 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>, -} - -impl ReadBeforeWriteSandbox { - pub fn new(inner: Arc) -> Self { - Self { - inner, - read_set: Mutex::new(HashSet::new()), - } - } - - fn normalize_path(&self, path: &str) -> String { - let full = if path.starts_with('/') { - PathBuf::from(path) - } else { - PathBuf::from(self.inner.working_directory()).join(path) - }; - - let mut parts: Vec = Vec::new(); - for component in full.components() { - match component { - Component::Normal(s) => parts.push(s.to_string_lossy().into_owned()), - Component::ParentDir => { - parts.pop(); - } - Component::RootDir | Component::CurDir | Component::Prefix(_) => {} - } - } - - format!("/{}", parts.join("/")) - } - - fn mark_read(&self, path: &str) { - let normalized = self.normalize_path(path); - self.read_set - .lock() - .expect("read_set lock poisoned") - .insert(normalized); - } - - fn has_read(&self, path: &str) -> bool { - let normalized = self.normalize_path(path); - self.read_set - .lock() - .expect("read_set lock poisoned") - .contains(&normalized) - } - - async fn guard_write(&self, path: &str) -> crate::Result<()> { - let normalized = self.normalize_path(path); - if normalized.starts_with("/tmp/") { - return Ok(()); - } - 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(crate::Error::message(format!( - "Cannot write to '{path}': file exists but has not been read. \ - Use read_file to read the file before writing to it." - ))) - } else { - Ok(()) - } - } -} - -crate::delegate_sandbox! { - ReadBeforeWriteSandbox => inner { - async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { - self.guard_write(path).await?; - self.inner.write_file(path, content).await - } - - async fn delete_file(&self, path: &str) -> crate::Result<()> { - self.guard_write(path).await?; - self.inner.delete_file(path).await - } - - fn mark_agent_read(&self, path: &str) { - debug!(path = %path, "File marked as agent-read"); - self.mark_read(path); - } - } -} - -#[cfg(test)] -mod tests { - use std::collections::HashMap; - - use super::*; - use crate::GrepOptions; - use crate::test_support::MockSandbox; - - fn mock_with_files(files: HashMap) -> MockSandbox { - MockSandbox { - files, - working_dir: "/work", - ..Default::default() - } - } - - // Cycle 1: write to existing unread file → error - #[tokio::test] - async fn write_to_existing_unread_file_returns_error() { - let mock = mock_with_files(HashMap::from([("a.ts".into(), "content".into())])); - let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - - let result = env.write_file("a.ts", "new content").await; - - assert!(result.is_err()); - let err = result.unwrap_err().to_string(); - assert!(err.contains("a.ts")); - assert!(err.contains("read")); - } - - // Cycle 2: write to non-existent file → success - #[tokio::test] - async fn write_to_nonexistent_file_succeeds() { - let mock = mock_with_files(HashMap::new()); - let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - - let result = env.write_file("new.ts", "content").await; - - assert!(result.is_ok()); - } - - // Cycle 3: mark_agent_read then write → success - #[tokio::test] - async fn read_then_write_succeeds() { - let mock = mock_with_files(HashMap::from([("a.ts".into(), "content".into())])); - 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 = mock_with_files(HashMap::from([("a.ts".into(), "content".into())])); - 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_err()); - } - - // Cycle 5: grep alone does NOT populate read set - #[tokio::test] - 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()], - working_dir: "/work", - ..Default::default() - }; - let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - - env.grep("pattern", ".", &GrepOptions::default()) - .await - .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()], - working_dir: "/work", - ..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 7: glob does NOT populate read set - #[tokio::test] - async fn glob_does_not_populate_read_set() { - let mock = MockSandbox { - files: HashMap::from([("c.ts".into(), "content".into())]), - glob_results: vec!["c.ts".into()], - working_dir: "/work", - ..Default::default() - }; - let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - - env.glob("*.ts", None).await.unwrap(); - let result = env.write_file("c.ts", "new").await; - - assert!(result.is_err()); - } - - // Cycle 8: path normalization — relative vs absolute via mark_agent_read - #[tokio::test] - async fn path_normalization_relative_and_absolute() { - let mock = MockSandbox { - files: HashMap::from([ - ("a.ts".into(), "content".into()), - ("/work/a.ts".into(), "content".into()), - ]), - working_dir: "/work", - ..Default::default() - }; - let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - - env.mark_agent_read("a.ts"); - let result = env.write_file("/work/a.ts", "new content").await; - - assert!(result.is_ok()); - } - - // Cycle 9: delete unread file → error - #[tokio::test] - async fn delete_unread_file_returns_error() { - let mock = mock_with_files(HashMap::from([("d.ts".into(), "content".into())])); - let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - - let result = env.delete_file("d.ts").await; - - assert!(result.is_err()); - } - - // Cycle 10: error message is actionable - #[tokio::test] - async fn error_message_is_actionable() { - let mock = mock_with_files(HashMap::from([("main.rs".into(), "fn main() {}".into())])); - let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - - let err = env - .write_file("main.rs", "new") - .await - .unwrap_err() - .to_string(); - - assert!(err.contains("main.rs")); - assert!(err.contains("read_file")); - } - - // Cycle 11: write to /tmp bypasses guard - #[tokio::test] - async fn write_to_tmp_bypasses_guard() { - let mock = MockSandbox { - files: HashMap::from([("/tmp/fabro-commit-msg".into(), "old".into())]), - working_dir: "/work", - ..Default::default() - }; - let env = ReadBeforeWriteSandbox::new(Arc::new(mock)); - - let result = env.write_file("/tmp/fabro-commit-msg", "new").await; - - assert!(result.is_ok()); - } - - #[tokio::test] - async fn stdio_process_forwards_to_inner_sandbox() { - let mock = Arc::new(MockSandbox::linux()); - let env = ReadBeforeWriteSandbox::new(mock.clone()); - - env.spawn_stdio_process("python fake_agent.py", Some("/work/sub"), None, None) - .await - .unwrap(); - - assert_eq!( - *mock.captured_command.lock().unwrap(), - Some("python fake_agent.py".to_string()) - ); - assert_eq!(*mock.captured_working_dirs.lock().unwrap(), vec![Some( - "/work/sub".to_string() - )]); - } -} diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index 1b6d6625a..fee0c6c02 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -1211,12 +1211,6 @@ pub trait Sandbox: Send + Sync { ) -> crate::Result)>> { Ok(None) } - - /// 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) {} } /// Resolve a path: relative paths are prepended with the working directory. diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index 60df59c4f..149a9ee59 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -12,8 +12,7 @@ use fabro_graphviz::graph; use fabro_hooks::{HookContext, HookDecision, HookEvent, HookExecutionContext, HookRunner}; use fabro_model::Catalog; use fabro_sandbox::{ - GitSetupIntent, ReadBeforeWriteSandbox, SandboxEventCallback, SandboxSpec, - reconnect_for_run_with_callback, shell_quote, + GitSetupIntent, SandboxEventCallback, SandboxSpec, reconnect_for_run_with_callback, shell_quote, }; use fabro_static::EnvVars; use fabro_vault::Vault; @@ -374,15 +373,13 @@ pub async fn initialize( .await .map_err(|err| Error::engine_with_anyhow("Failed to reconnect sandbox for resume", err))?; sandbox_initialized = false; - Arc::new(ReadBeforeWriteSandbox::new(Arc::from(sandbox))) + Arc::from(sandbox) } else { - Arc::new(ReadBeforeWriteSandbox::new( - options - .sandbox - .build(Some(Arc::clone(&sandbox_event_callback))) - .await - .map_err(|e| Error::engine_with_anyhow("Failed to build sandbox", e))?, - )) + options + .sandbox + .build(Some(Arc::clone(&sandbox_event_callback))) + .await + .map_err(|e| Error::engine_with_anyhow("Failed to build sandbox", e))? }; let cleanup_guard = (!attach_existing).then(|| { scopeguard::guard(Arc::clone(&sandbox), |sandbox| {