mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-07 03:00:29 +00:00
fix(agent): remove the read-before-write guard
`ReadBeforeWriteSandbox` blocked writes to any existing file the agent had not read, tracked by a session read set populated only by `read_file`, `grep`, `read_many_files`, and the Kimi `Read`. The gpt56 profile has none of those. It mirrors Codex's tool contract -- `shell_command`, `apply_patch`/`edit_file`, `update_plan`, `web_search` -- and reads through the shell, so its read set stayed permanently empty and every edit to an existing file failed. In run 01KYD4360GN6SED4BYEVGYP4XT all 28 `edit_file` calls failed, 25 of them on the guard. The agent read `package.json` with `sed` and `cat`, hex-dumped it trying to diagnose the rejections, then routed around the guard with `sed -i`, which the guard never covered. It prevented no blind write; it converted content-anchored edits into an unreviewed in-place shell rewrite. Neither Codex nor Kimi Code enforces read-before-write at runtime. Codex's `apply_patch` `Add File` overwrites an existing path silently; Kimi Code's `Write` has no check at all. Both rely on the exact-match requirement in their edit tools, which is stronger proof of inspection than a read set, plus per-write approval. Tool descriptions and the Kimi prompt keep telling the model to read before editing -- that guidance matches Kimi Code's own `edit.md` and still prevents `old_string not found` -- but no longer claim the workspace refuses unread writes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Release Repro
parent
e510dac98d
commit
a925275778
16 changed files with 45 additions and 677 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -76,7 +76,7 @@ Creates or overwrites a file.
|
|||
| `content` | string | yes | Content to write |
|
||||
|
||||
<Note>
|
||||
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.
|
||||
</Note>
|
||||
|
||||
### 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.
|
||||
|
||||
<Note>
|
||||
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.
|
||||
</Note>
|
||||
|
||||
### 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
|
||||
|
||||
|
|
|
|||
|
|
@ -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<dyn Sandbox>`. |
|
||||
|
||||
The `DaytonaSandbox` implementation (feature-gated: `daytona`) runs inside a Daytona cloud sandbox.
|
||||
|
||||
|
|
|
|||
|
|
@ -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<dyn Sandbox> = Arc::new(crate::ReadBeforeWriteSandbox::new(Arc::new(
|
||||
LocalSandbox::new(cwd),
|
||||
)));
|
||||
let env: Arc<dyn Sandbox> = Arc::new(LocalSandbox::new(cwd));
|
||||
|
||||
// Build tool approval callback
|
||||
let permissions = args.permissions.unwrap_or(PermissionLevel::ReadWrite);
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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}");
|
||||
|
|
|
|||
|
|
@ -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)(
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -1 +0,0 @@
|
|||
pub use fabro_sandbox::read_guard::ReadBeforeWriteSandbox;
|
||||
|
|
@ -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<String, String>) -> Arc<dyn Sandbox> {
|
||||
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<dyn Sandbox> {
|
||||
Arc::new(MockSandbox {
|
||||
exec_result: result,
|
||||
|
|
|
|||
|
|
@ -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<Vec<String>, 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 `<path>:<line>:<content>` 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<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_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<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_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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<dyn Sandbox>,
|
||||
read_set: Mutex<HashSet<String>>,
|
||||
}
|
||||
|
||||
impl ReadBeforeWriteSandbox {
|
||||
pub fn new(inner: Arc<dyn Sandbox>) -> 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<String> = 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<String, String>) -> 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()
|
||||
)]);
|
||||
}
|
||||
}
|
||||
|
|
@ -1211,12 +1211,6 @@ pub trait Sandbox: Send + Sync {
|
|||
) -> crate::Result<Option<(String, HashMap<String, String>)>> {
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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| {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue