From a925275778bba97b24090fd07f0ccf362ae94341 Mon Sep 17 00:00:00 2001 From: Release Repro Date: Sat, 25 Jul 2026 14:53:38 -0400 Subject: [PATCH 1/3] 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) --- docs/public/agents/permissions.mdx | 9 - docs/public/agents/tools.mdx | 29 +- docs/public/reference/sdk.mdx | 1 - lib/components/fabro-agent/src/cli.rs | 4 +- lib/components/fabro-agent/src/lib.rs | 2 - .../fabro-agent/src/profiles/kimi.rs | 29 +- .../fabro-agent/src/profiles/kimi_tools.rs | 14 +- .../fabro-agent/src/profiles/mod.rs | 4 +- .../src/profiles/prompts/kimi.md.j2 | 4 +- .../src/read_before_write_sandbox.rs | 1 - .../fabro-agent/src/tool_execution.rs | 215 +------------ lib/components/fabro-agent/src/tools.rs | 86 +---- lib/components/fabro-sandbox/src/lib.rs | 3 - .../fabro-sandbox/src/read_guard.rs | 298 ------------------ lib/components/fabro-sandbox/src/sandbox.rs | 6 - .../fabro-workflow/src/pipeline/initialize.rs | 17 +- 16 files changed, 45 insertions(+), 677 deletions(-) delete mode 100644 lib/components/fabro-agent/src/read_before_write_sandbox.rs delete mode 100644 lib/components/fabro-sandbox/src/read_guard.rs 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| { From 454f07d56037b02b4578971f87b1fdf2584d6459 Mon Sep 17 00:00:00 2001 From: Release Repro Date: Sat, 25 Jul 2026 15:07:19 -0400 Subject: [PATCH 2/3] refactor(kimi): match Codex and Kimi Code on where read guidance lives Neither harness puts read-before-edit mechanics in the system prompt. Kimi Code's `system.md` has no such section; the rules live in `edit.md`, `write.md`, and `read.md`. Codex's prompts say nothing about reading before an edit at all, and its editing guidance is attached to `apply_patch`. Drop the `# Reading Before Writing` section from the Kimi prompt and carry its content in the Edit, Write, and Read descriptions, worded as Kimi Code words it. Nothing is lost: every bullet in the removed section was already covered by a tool description. Two behaviors change to match upstream. Edit now says not to issue consecutive edits against the same file, since the first invalidates the second's `old_string` -- Kimi Code's stated reason. Read now says not to re-read solely to confirm a write landed, which both harnesses call out as waste; the previous prompt asked for exactly that re-read. The gpt56 profile already followed the Codex split and is unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .../fabro-agent/src/profiles/kimi.rs | 50 +++++++++++++------ .../fabro-agent/src/profiles/kimi_tools.rs | 21 ++++---- .../src/profiles/prompts/kimi.md.j2 | 10 ---- 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/lib/components/fabro-agent/src/profiles/kimi.rs b/lib/components/fabro-agent/src/profiles/kimi.rs index 32a277671..02d56f8ce 100644 --- a/lib/components/fabro-agent/src/profiles/kimi.rs +++ b/lib/components/fabro-agent/src/profiles/kimi.rs @@ -19,17 +19,25 @@ const CORE_PROMPT: &str = include_str!("prompts/kimi.md.j2"); /// 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: 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. \ -Preserve existing indentation."; +/// values recalled from an earlier version. Kimi Code carries this guidance in +/// its tool descriptions and nowhere in its system prompt, so this profile does +/// the same — the rule lands in the description of the tool being called. +const EDIT_FILE_DESCRIPTION: &str = "Perform exact replacements in existing files. + +- Edit is mandatory for every incremental change, especially small edits. DO NOT use Write or \ +Bash `sed`. +- Read the target file before every Edit. DO NOT call Edit from memory, stale context, or a \ +guessed `old_string`. +- Take `old_string` and `new_string` from the Read output view, dropping the line-number prefix \ +and separator; match only file content. +- `old_string` must be unique unless `replace_all` is set. If it is ambiguous, add surrounding \ +context. Use `replace_all` only when every occurrence should change — for example, renaming a \ +symbol throughout the file. +- DO NOT issue consecutive Edit calls on the same file. A previous Edit can invalidate a later \ +Edit's `old_string`, causing `old_string not found`. Read the file again before the next Edit. +- If an Edit fails with `old_string not found`, re-read the file and take the exact text from the \ +fresh output rather than guessing again. +- Preserve existing indentation."; const GLOB_DESCRIPTION: &str = "Find files by search-root-relative path using a glob pattern. \ Results are sorted lexicographically by relative path. @@ -310,6 +318,8 @@ mod tests { .clone() }; + // Kimi Code carries read-before-edit guidance in the tool descriptions + // and nowhere in its system prompt, so this is where it must land. for name in ["Edit", "Write"] { let text = describe(name); assert!( @@ -317,8 +327,13 @@ mod tests { "{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")); + assert!( + describe("Edit").contains("DO NOT call Edit from memory, stale context, or a guessed") + ); + assert!(describe("Edit").contains("DO NOT issue consecutive Edit calls on the same file")); + assert!(describe("Write").contains("Read before overwriting an existing file")); + // Re-reading only to confirm a write landed is waste, not diligence. + assert!(describe("Read").contains("do not re-read solely to prove the write landed")); // Bash steers shell usage toward the dedicated tools, under the names // this profile actually exposes. @@ -343,8 +358,8 @@ mod tests { // Fabro has no background shell; promising one would be a lie. assert!(!bash.contains("run_in_background"), "{bash}"); - // Read explains why reading precedes editing. - assert!(describe("Read").contains("matches `old_string` byte for byte")); + // Read tells the model how to turn its output into an Edit old_string. + assert!(describe("Read").contains("Drop the number and separator")); // Grep must not promise ripgrep syntax: fabro falls back to POSIX grep. let grep = describe("Grep"); assert!(grep.contains("POSIX"), "{grep}"); @@ -381,7 +396,10 @@ mod tests { let env = MockSandbox::linux(); let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None, &[]); assert!(prompt.contains("You are Kimi")); - assert!(prompt.contains("# Reading Before Writing")); + assert!(prompt.contains("# Tracking Multi-Step Work")); assert!(prompt.contains("")); + // Kimi Code keeps read-before-edit mechanics out of its system prompt + // and in the tool descriptions; the profile follows that split. + assert!(!prompt.contains("Reading Before Writing")); } } diff --git a/lib/components/fabro-agent/src/profiles/kimi_tools.rs b/lib/components/fabro-agent/src/profiles/kimi_tools.rs index fb49eff6c..a379569a8 100644 --- a/lib/components/fabro-agent/src/profiles/kimi_tools.rs +++ b/lib/components/fabro-agent/src/profiles/kimi_tools.rs @@ -159,9 +159,6 @@ pub fn make_kimi_read_tool() -> RegisteredTool { NativeTool::ReadFile, "Read a text file from the workspace. -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. - When you need several files, emit multiple Read calls in one response rather than one per turn. @@ -170,7 +167,9 @@ an Edit `old_string`. - `line_offset` is the 1-based first line to read. A NEGATIVE value reads from the end, so -100 \ returns the last 100 lines. - `n_lines` defaults to 2000 lines. -- Use Bash or an MCP tool for binary formats; this tool reads text.", +- Use Bash or an MCP tool for binary formats; this tool reads text. +- After a successful Edit or Write, do not re-read solely to prove the write landed. When the task \ +depends on an exact file, API, or output shape, inspect the final result before finishing.", serde_json::json!({ "type": "object", "properties": { @@ -259,16 +258,16 @@ pub fn make_kimi_write_tool() -> RegisteredTool { RegisteredTool { definition: definition( NativeTool::WriteFile, - "Create, append to, or replace a file. - -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. + "Create, append to, or replace a file entirely. - `mode` defaults to `overwrite`, which replaces the whole file. `append` requires an existing file \ and adds to its end without inserting a newline. -- Write is NOT for incremental changes to an existing file, however small. Use Edit instead: \ -overwrite replaces everything you did not restate. -- Use `overwrite` when the file does not exist, or when you intend a complete replacement. +- Write is NOT ALLOWED for incremental changes to existing files, including trivial, one-line, \ +quick, or cosmetic edits. Use Edit instead. +- Use Write only when the file does not exist, you intend a complete replacement, or the new \ +contents have little continuity with the old contents. +- Read before overwriting an existing file. +- Write ignores the Read/Edit line-number view. NEVER include line prefixes. - Do not create documentation files that were not asked for.", serde_json::json!({ "type": "object", 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 1e017430a..8804b1378 100644 --- a/lib/components/fabro-agent/src/profiles/prompts/kimi.md.j2 +++ b/lib/components/fabro-agent/src/profiles/prompts/kimi.md.j2 @@ -24,16 +24,6 @@ Tool calls run behind the user's permission settings. A rejected or denied call When a tool call fails, diagnose why before acting again: read the error, check your assumptions, and make a focused adjustment. Do not retry the identical call blindly, but do not abandon a viable approach after a single failure either — if you are still stuck after investigating, ask the user. -# Reading Before Writing - -`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, 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 Use `TodoList` for work that spans several steps, and keep it current as you go. From 6d61c6b5e43e4ac33de8f02679ed571e3e83f843 Mon Sep 17 00:00:00 2001 From: Release Repro Date: Sat, 25 Jul 2026 15:13:41 -0400 Subject: [PATCH 3/3] fix(agent): stop the Kimi leak assertion from passing vacuously The check tested for `never reconstruct it from memory`, a phrase the Kimi edit description no longer contains after it was reworded to match Kimi Code. A one-sided `!contains` against a literal cannot tell "the phrase is absent because nothing leaked" from "the phrase is absent everywhere", so it silently stopped protecting anything. Assert the marker is present in Kimi's own description and absent from the stock one. Removing the marker from the description now fails the test instead of quietly disarming it, verified by doing exactly that. Also correct the grep docs: all three sandbox implementations probe for `rg` and fall back to POSIX `grep`, so the page should not imply a single engine. Pre-existing, adjacent to the lines this branch touched. Reported by Copilot review on #646. Co-Authored-By: Claude Opus 5 (1M context) --- docs/public/agents/tools.mdx | 2 +- .../fabro-agent/src/profiles/mod.rs | 20 ++++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/docs/public/agents/tools.mdx b/docs/public/agents/tools.mdx index 373971d9b..62d4e9f98 100644 --- a/docs/public/agents/tools.mdx +++ b/docs/public/agents/tools.mdx @@ -96,7 +96,7 @@ Because `old_string` must match the file exactly, an edit built from a stale or ### grep -Searches file contents with a regex pattern, powered by ripgrep in the sandbox. +Searches file contents with a regex pattern. The sandbox uses ripgrep when `rg` is on the path and falls back to POSIX `grep` otherwise, detected once and cached per sandbox. Keep patterns portable across both rather than relying on ripgrep-only syntax. | Parameter | Type | Required | Description | |---|---|---|---| diff --git a/lib/components/fabro-agent/src/profiles/mod.rs b/lib/components/fabro-agent/src/profiles/mod.rs index ee0da68c1..b6d60e204 100644 --- a/lib/components/fabro-agent/src/profiles/mod.rs +++ b/lib/components/fabro-agent/src/profiles/mod.rs @@ -536,11 +536,21 @@ mod tests { ); } - // The specific Kimi-only phrasing must not appear elsewhere. - assert!( - !anthropic_text.contains("never reconstruct it from memory"), - "Kimi read-before-edit drilling leaked into {tool} for other profiles" - ); + // The Kimi-only phrasing must not appear elsewhere. Assert it is + // present in Kimi's own description too: a one-sided check against + // a literal silently goes vacuous the next time that wording is + // rewritten, which is exactly how it last stopped testing anything. + if tool == NativeTool::EditFile { + const KIMI_EDIT_MARKER: &str = "DO NOT call Edit from memory"; + assert!( + kimi_text.contains(KIMI_EDIT_MARKER), + "Kimi's {tool} description should drill reading before an edit: {kimi_text}" + ); + assert!( + !anthropic_text.contains(KIMI_EDIT_MARKER), + "Kimi read-before-edit drilling leaked into {tool} for other profiles" + ); + } } }