From ee7240c919a2867557fc72bfadbd00546ff8b1fa Mon Sep 17 00:00:00 2001 From: bobleer Date: Wed, 13 May 2026 08:37:26 +0800 Subject: [PATCH 1/4] feat(tools): make Write reject existing files and guide rewrite via Delete+Write The Write tool previously overwrote existing files unconditionally, which made it easy for models to clobber files with incomplete content when they should have used Edit. Refuse the write when the target file already exists and update the tool description to explain the intended workflow: use Edit to modify, or Delete + Write to fully rewrite. The error message returned to the model also points at both alternatives so it can self-correct on the next round. --- .../tools/implementations/file_write_tool.rs | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/crates/core/src/agentic/tools/implementations/file_write_tool.rs b/src/crates/core/src/agentic/tools/implementations/file_write_tool.rs index 0bf319b44..6b15318af 100644 --- a/src/crates/core/src/agentic/tools/implementations/file_write_tool.rs +++ b/src/crates/core/src/agentic/tools/implementations/file_write_tool.rs @@ -32,11 +32,11 @@ impl Tool for FileWriteTool { Ok(r#"Writes a file to the local filesystem. Usage: -- This tool will overwrite the existing file if there is one at the provided path. -- If this is an existing file, you MUST use the Read tool first to read the file's contents. This tool will fail if you did not read the file first. +- This tool is for creating NEW files only. Calling Write on a path that already exists will be REJECTED with an error. +- To MODIFY an existing file, use the Edit tool — it is the correct choice in almost every case. +- To FULLY REWRITE an existing file (e.g. regenerate a generated file, replace a template), first call the Delete tool on that path, then call Write to create the new version. Do not try to "overwrite" via Write directly. - The file_path parameter must be workspace-relative, an absolute path inside the current workspace, or an exact `bitfun://runtime/...` URI returned by another tool. - ALWAYS prefer editing existing files in the codebase. NEVER write new files unless explicitly required. -- For existing files, prefer Read + targeted Edit calls. For new files or rewrites, preserve correctness and provide the complete intended file content when this tool is appropriate. - NEVER proactively create documentation files (*.md) or README files. Only create documentation files if explicitly requested by the User. - Only use emojis if the user explicitly requests it. Avoid writing emojis to files unless asked. - Do NOT include the file content in the tool call arguments. Only provide file_path. The system will prompt you separately to output the file content as plain text."#.to_string()) @@ -148,6 +148,28 @@ Usage: ) .await; + // Guard: refuse to overwrite an existing file — the model should use + // Edit instead. This prevents accidental data loss from models that + // call Write repeatedly on the same file with incomplete content. + let file_already_exists = if resolved.uses_remote_workspace_backend() { + if let Some(ws_fs) = context.ws_fs() { + ws_fs.exists(&resolved.resolved_path).await.unwrap_or(false) + } else { + false + } + } else { + Path::new(&resolved.resolved_path).exists() + }; + + if file_already_exists { + return Err(BitFunError::tool(format!( + "File {} already exists. The Write tool is reserved for creating NEW files. \ + To modify the file, use the Edit tool. \ + To fully rewrite the file, first call the Delete tool on this path, then call Write again.", + resolved.logical_path + ))); + } + let content = input .get("content") .and_then(|v| v.as_str()) From 34c033f7edfc2f1338484044e08efba098d9a584 Mon Sep 17 00:00:00 2001 From: bobleer Date: Wed, 13 May 2026 08:37:34 +0800 Subject: [PATCH 2/4] feat(agentic): give the model recovery attempts before stopping on detected loops Previously, both the consecutive-signature and periodic-signature loop detectors terminated the round immediately on the first hit. In practice the model can often recover if it is told that it is stuck and asked to change strategy. Inject a system_reminder user message describing the detected loop and asking the model to switch approach, give it up to 3 such recovery attempts (shared across both detectors), and only then fall back to the existing terminate-with-loop_detected behavior. The reminders are also persisted via SessionManager so the recovery is visible in transcripts. This keeps the existing safety net (we still stop eventually) while making genuine transient stalls recoverable instead of fatal. --- .../src/agentic/execution/execution_engine.rs | 88 ++++++++++++++++--- 1 file changed, 74 insertions(+), 14 deletions(-) diff --git a/src/crates/core/src/agentic/execution/execution_engine.rs b/src/crates/core/src/agentic/execution/execution_engine.rs index 0bd48bfca..7b9b4ec6f 100644 --- a/src/crates/core/src/agentic/execution/execution_engine.rs +++ b/src/crates/core/src/agentic/execution/execution_engine.rs @@ -1527,6 +1527,8 @@ impl ExecutionEngine { // P0: Loop detection: track recent tool call signatures let mut recent_tool_signatures: Vec = Vec::new(); let mut loop_detected = false; + let mut loop_recovery_attempts: usize = 0; + const MAX_LOOP_RECOVERY_ATTEMPTS: usize = 3; let mut full_compression_count = 0usize; let mut compression_failure_count = 0u32; @@ -1937,13 +1939,42 @@ impl ExecutionEngine { if recent_tool_signatures.len() >= max_consec { let tail = &recent_tool_signatures[recent_tool_signatures.len() - max_consec..]; if tail.windows(2).all(|w| w[0] == w[1]) { - warn!( - "Loop detected: {} consecutive rounds with identical tool signatures, stopping", - max_consec - ); - loop_detected = true; - finalization_reason = Some("loop_detected"); - break; + if loop_recovery_attempts < MAX_LOOP_RECOVERY_ATTEMPTS { + loop_recovery_attempts += 1; + warn!( + "Loop detected: {} consecutive rounds with identical tool signatures, injecting recovery prompt #{}", + max_consec, loop_recovery_attempts + ); + let reminder = format!( + "Loop detected: you have repeated the same tool call with identical arguments {} times in a row. \ + This means the approach is not making progress. You MUST now change your strategy: \ + (1) if the tool keeps failing, try a completely different approach or tool; \ + (2) if you are stuck, step back and reason about the root cause before acting; \ + (3) if the task is genuinely impossible with the available tools, provide a clear explanation to the user. \ + Do NOT repeat the same tool call again.", + max_consec + ); + let user_msg = Message::user(reminder); + messages.push(user_msg.clone()); + if let Err(e) = self + .session_manager + .add_message(&context.session_id, user_msg) + .await + { + warn!("Failed to persist loop recovery reminder: {}", e); + } + // Clear the recent signatures so the detector resets after recovery. + recent_tool_signatures.clear(); + // Do NOT break — continue the loop so the model gets a chance to recover. + } else { + warn!( + "Loop detected: {} consecutive rounds with identical tool signatures, max recovery attempts ({}) exhausted, stopping", + max_consec, MAX_LOOP_RECOVERY_ATTEMPTS + ); + loop_detected = true; + finalization_reason = Some("loop_detected"); + break; + } } } @@ -1963,13 +1994,42 @@ impl ExecutionEngine { // no genuine new exploration and we treat it as a loop. if Self::is_periodic_tool_signature_loop(&recent_tool_signatures, max_consec) { let window_size = max_consec.max(1).saturating_mul(2); - warn!( - "Loop detected: last {} rounds form a periodic tool-call pattern (<= {} distinct signatures, each repeated), stopping", - window_size, max_consec - ); - loop_detected = true; - finalization_reason = Some("loop_detected"); - break; + if loop_recovery_attempts < MAX_LOOP_RECOVERY_ATTEMPTS { + loop_recovery_attempts += 1; + warn!( + "Loop detected: last {} rounds form a periodic tool-call pattern (<= {} distinct signatures, each repeated), injecting recovery prompt #{}", + window_size, max_consec, loop_recovery_attempts + ); + let reminder = format!( + "Loop detected: your last {} tool calls form a repeating pattern with no new progress. \ + You are cycling between the same actions without advancing the task. You MUST now change your strategy: \ + (1) try a completely different approach or tool; \ + (2) step back and reason about the root cause before acting; \ + (3) if the task is genuinely impossible with the available tools, provide a clear explanation to the user. \ + Do NOT repeat the same pattern of tool calls.", + window_size + ); + let user_msg = Message::user(reminder); + messages.push(user_msg.clone()); + if let Err(e) = self + .session_manager + .add_message(&context.session_id, user_msg) + .await + { + warn!("Failed to persist periodic loop recovery reminder: {}", e); + } + // Clear the recent signatures so the detector resets after recovery. + recent_tool_signatures.clear(); + // Do NOT break — continue the loop so the model gets a chance to recover. + } else { + warn!( + "Loop detected: last {} rounds form a periodic tool-call pattern, max recovery attempts ({}) exhausted, stopping", + window_size, MAX_LOOP_RECOVERY_ATTEMPTS + ); + loop_detected = true; + finalization_reason = Some("loop_detected"); + break; + } } // User-steering messages submitted while this turn is running: drain and inject From f8b48f96469cca61631c1f5a33f192797fdc2671 Mon Sep 17 00:00:00 2001 From: bobleer Date: Wed, 13 May 2026 08:37:50 +0800 Subject: [PATCH 3/4] feat(agentic): harden Write content generation prompt and sanitize model output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related improvements to the two-stage Write flow that asks the model to emit the full file body inside tags. 1. Prompt hardening - Spell out the rule against omission placeholders ("// rest of the code", "// existing code unchanged", etc.) and clarify that literal "..." is fine when it is genuine file content (XML/JSON/docs). - Forbid markdown fences and stray XML wrappers around the body. - Add an assistant prefill of "\n" to bias the model toward emitting raw content immediately. 2. Output sanitization in extract_bitfun_contents - Strip thinking-style XML blocks (, , , ) including the non-standard variants that some reasoning models emit. - Strip outer markdown code fences (```lang ... ```) when they wrap the entire body. - Add a conservative "omission marker" detector that warns when the generated body contains comment-style phrases such as "// ... rest of the code" or "". The detector is deliberately strict (only matches phrases that are essentially never legitimate in real source/data files) and only emits a warning — it never blocks the write, since Write must be able to produce any kind of file, including ones that legitimately discuss these phrases in prose. Adds unit tests covering thinking-block stripping, fence stripping, preservation of legitimate XML, and both positive and negative cases for the placeholder detector (including XML data and prose mentioning the phrases, which must not trigger). --- .../src/agentic/execution/round_executor.rs | 341 +++++++++++++++++- 1 file changed, 330 insertions(+), 11 deletions(-) diff --git a/src/crates/core/src/agentic/execution/round_executor.rs b/src/crates/core/src/agentic/execution/round_executor.rs index 5fc1e5cd4..7bf295b81 100644 --- a/src/crates/core/src/agentic/execution/round_executor.rs +++ b/src/crates/core/src/agentic/execution/round_executor.rs @@ -882,15 +882,28 @@ impl RoundExecutor { // Build a content-generation prompt let content_prompt = format!( - "Now output the complete file content for the file `{}`. \ - Output ONLY the raw file content wrapped in tags. \ - Do NOT include any other text, explanation, or commentary outside the tags.\n\ + "Now output the COMPLETE file content for the file `{file_path}`.\n\ + CRITICAL RULES — you MUST follow all of them:\n\ + 1. Output the ENTIRE file content — every single line, every character that should end up on disk.\n\ + 2. Do NOT abbreviate, summarize, or insert placeholder comments referring to omitted code, such as: \ + \"// ... rest of the code\", \"// rest omitted\", \"// implementation follows\", \"// existing code unchanged\", \ + \"// same as before\", \"# rest omitted\", \"# rest of file\", or any equivalent in any language. \ + If a section is unchanged, write it out in full anyway.\n\ + 3. Literal `...` is allowed only when it is genuinely part of the file content (e.g. inside a string, \ + inside XML/JSON/YAML data, inside docs). Never use it as a stand-in for omitted code.\n\ + 4. Wrap the content inside tags exactly as shown below.\n\ + 5. Do NOT output anything outside the tags — no explanations, no commentary, \ + no thinking blocks, no markdown fences (```), no extra XML wrapper tags.\n\ + 6. The text between the tags must be EXACTLY what gets written to disk — raw file content only.\n\ \n", - file_path + file_path = file_path ); let mut content_messages = ai_messages.to_vec(); + // Add an assistant prefill to prime the model to output content directly + // inside the tags, reducing the chance of preamble text. content_messages.push(AIMessage::user(content_prompt)); + content_messages.push(AIMessage::assistant("\n".to_string())); // Send the content-generation request (no tools, pure text output) let full_text = match ai_client.send_message_stream(content_messages, None).await { @@ -955,6 +968,19 @@ impl RoundExecutor { ); } + // Detect strong "omission marker" phrases that indicate the model + // wrote a summary instead of the full file content. This is a + // best-effort warning only — we do not block the write, because + // Write must remain general enough to produce any kind of file + // (including ones that legitimately discuss these phrases). + if let Some(marker) = detect_placeholder_patterns(&content) { + warn!( + "Write content for file_path={} contains an omission marker comment ({:?}); \ + the generated content may be an outline rather than the full file", + file_path, marker + ); + } + let final_params = serde_json::json!({ "file_path": &file_path, "content": &content, @@ -1180,18 +1206,205 @@ fn extract_bitfun_contents(text: &str) -> String { const OPEN_TAG: &str = ""; const CLOSE_TAG: &str = ""; - if let Some(start) = text.find(OPEN_TAG) { + let raw = if let Some(start) = text.find(OPEN_TAG) { let content_start = start + OPEN_TAG.len(); if let Some(end) = text[content_start..].find(CLOSE_TAG) { - return text[content_start..content_start + end].trim().to_string(); + &text[content_start..content_start + end] + } else { + // Opening tag found but no closing tag — take everything after the + // opening tag (the model may still be streaming or forgot to close). + &text[content_start..] + } + } else { + // No tags at all — return the full text as a fallback + text + }; + + sanitize_write_content(raw.trim()) +} + +/// Sanitize model-generated file content by stripping common artifacts that +/// some models emit despite being told not to. +fn sanitize_write_content(content: &str) -> String { + let mut s = content.to_string(); + + // Strip multi-line thinking/reasoning XML blocks (e.g. ..) + // These are very common with reasoning models. + s = strip_thinking_blocks(&s); + + // Strip leading/trailing markdown code fences (```lang ... ```) + // that some models wrap around file content. + s = strip_markdown_fences(&s); + + // Trim leading/trailing whitespace left after stripping blocks + s.trim().to_string() +} + +/// Strip thinking-style XML blocks from content. Handles multi-line blocks +/// like `content` and `content`. +/// Also handles non-standard formats like `` where +/// the opening tag may not have a closing `>`. +fn strip_thinking_blocks(content: &str) -> String { + let thinking_open_tags = ["' or newline + let after_open = &result[open_start..]; + let tag_end_offset = after_open + .find(|c: char| c == '>' || c == '\n') + .unwrap_or(after_open.len()); + + // Extract tag name from + let tag_inner = &result[open_start + 1..open_start + tag_end_offset]; + let tag_name = tag_inner.split_whitespace().next().unwrap_or(""); + + // Skip if tag_name is empty (shouldn't happen but guard) + if tag_name.is_empty() { + break; + } + + // Build the closing tag. Note: some models output `` with + // trailing space or `' or newline or end) + let close_end = result[abs_close_pos..] + .find(|c: char| c == '>' || c == '\n') + .map(|p| abs_close_pos + p + 1) + .unwrap_or(result.len()); + result = format!("{}{}", &result[..open_start], &result[close_end..]); + } else { + // No closing tag found — strip from open_start to end of opening + // tag line and continue + let line_end = after_open + .find('\n') + .map(|p| open_start + p + 1) + .unwrap_or(result.len()); + result = format!("{}{}", &result[..open_start], &result[line_end..]); + } } - // Opening tag found but no closing tag — take everything after the - // opening tag (the model may still be streaming or forgot to close). - return text[content_start..].trim().to_string(); } - // No tags at all — return the full text as a fallback - text.trim().to_string() + result +} + +/// Strip markdown code fences that wrap the entire content. +/// Handles ```lang\n...\n``` patterns at the outermost level. +fn strip_markdown_fences(content: &str) -> String { + let trimmed = content.trim(); + if !trimmed.starts_with("```") { + return content.to_string(); + } + + // Find the end of the opening fence line + let fence_end = trimmed.find('\n').unwrap_or(3); + // let _lang = &trimmed[3..fence_end].trim(); // language hint, ignored + + // Check if content ends with ``` + let inner = trimmed[fence_end + 1..].trim_end(); + if inner.ends_with("```") { + return inner[..inner.len() - 3].trim_end().to_string(); + } + + // No closing fence — strip opening fence only + trimmed[fence_end + 1..].to_string() +} + +/// Detect "omission marker" phrases that strongly indicate the model wrote a +/// summary/outline instead of the full file. Returns the matched marker on the +/// first hit, or `None` otherwise. +/// +/// Design notes: +/// - Only match phrases that are very unlikely to legitimately appear in real +/// source/data files. Plain `...`, `…`, `TODO:` and `FIXME:` are NOT included +/// because they show up in real code, docs, XML/JSON data, etc., and would +/// trigger false positives on legitimate Write usage (the tool can write any +/// kind of file). +/// - Patterns are matched in a comment-like context (after `//`, `#`, `/*`, `--`, +/// or `", + "", + "", + ]; + + // Comment lead-ins we look for. Empty string means "no comment marker + // required" — used for the strongest phrases that are unmistakable on + // their own (e.g. ``). + const COMMENT_LEADS: &[&str] = &["//", "#", "/*", "--", "\n\n"; + assert!(detect_placeholder_patterns(content).is_some()); + } + + #[test] + fn no_false_positive_on_normal_code() { + use super::detect_placeholder_patterns; + let content = "fn main() {\n println!(\"hello\");\n}\n\nstruct Foo {\n x: i32,\n}\n"; + assert!(detect_placeholder_patterns(content).is_none()); + } + + #[test] + fn no_false_positive_on_single_todo() { + use super::detect_placeholder_patterns; + // Plain TODO/FIXME comments must NOT trigger — they are common in real code. + let content = "fn main() {\n println!(\"hello\");\n}\n\nfn helper() {\n // TODO: refactor later\n // FIXME: handle errors\n 42\n}\n"; + assert!(detect_placeholder_patterns(content).is_none()); + } + + #[test] + fn no_false_positive_on_xml_with_ellipsis() { + use super::detect_placeholder_patterns; + // XML/data files that genuinely contain "..." or "rest of" as data must NOT trigger. + let content = "\n The rest of the story is told elsewhere.\n Three dots: ...\n\n"; + assert!(detect_placeholder_patterns(content).is_none()); + } + + #[test] + fn no_false_positive_on_prose_mentioning_omission_phrase() { + use super::detect_placeholder_patterns; + // A markdown/doc file that talks about the phrase but isn't a code comment must NOT trigger. + let content = "# Style guide\n\nDo not write \"rest omitted for brevity\" inside committed source files.\n"; + assert!(detect_placeholder_patterns(content).is_none()); + } + + #[test] + fn detect_placeholder_empty_content() { + use super::detect_placeholder_patterns; + assert!(detect_placeholder_patterns("").is_none()); + } } From 7c02df1e0c653922af8dfac8e5caa4aebcdf62ca Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Wed, 13 May 2026 10:06:27 +0800 Subject: [PATCH 4/4] fix(agentic): preflight Write targets and handle idempotent same-content retries - Skip Write content generation when path resolution/policy fails or the file exists - Reject existing targets in validate_input when content is absent to avoid wasted model calls - Treat identical Write content on an existing path as successful no-op (already_exists_same_content) - Add FileWriteTool unit tests and tighten assistant-facing success guidance --- .../src/agentic/execution/round_executor.rs | 71 ++++- .../tools/implementations/file_write_tool.rs | 254 +++++++++++++++--- 2 files changed, 285 insertions(+), 40 deletions(-) diff --git a/src/crates/core/src/agentic/execution/round_executor.rs b/src/crates/core/src/agentic/execution/round_executor.rs index 7bf295b81..5841a1e5f 100644 --- a/src/crates/core/src/agentic/execution/round_executor.rs +++ b/src/crates/core/src/agentic/execution/round_executor.rs @@ -4,12 +4,15 @@ use super::stream_processor::{StreamProcessOptions, StreamProcessor, StreamResult}; use super::types::{FinishReason, RoundContext, RoundResult}; +use crate::agentic::MessageContent; use crate::agentic::core::{Message, ToolCall}; use crate::agentic::events::{AgenticEvent, EventPriority, EventQueue, ToolEventData}; +use crate::agentic::tools::ToolPathOperation; use crate::agentic::tools::computer_use_host::ComputerUseHostRef; +use crate::agentic::tools::framework::ToolUseContext; +use crate::agentic::tools::implementations::file_write_tool::FileWriteTool; use crate::agentic::tools::pipeline::{ToolExecutionContext, ToolExecutionOptions, ToolPipeline}; use crate::agentic::tools::registry::get_global_tool_registry; -use crate::agentic::MessageContent; use crate::infrastructure::ai::AIClient; use crate::service::config::GlobalConfigManager; use crate::util::elapsed_ms_u64; @@ -18,6 +21,7 @@ use crate::util::types::Message as AIMessage; use crate::util::types::ToolDefinition; use dashmap::DashMap; use log::{debug, error, info, warn}; +use std::collections::HashMap; use std::sync::Arc; use std::time::{Duration, Instant}; use tokio_util::sync::CancellationToken; @@ -214,7 +218,11 @@ impl RoundExecutor { attempt_index + 1, max_attempts, delay_ms, - result.tool_calls.iter().filter(|tool_call| !tool_call.is_valid()).count(), + result + .tool_calls + .iter() + .filter(|tool_call| !tool_call.is_valid()) + .count(), err_msg ); tokio::time::sleep(Duration::from_millis(delay_ms)).await; @@ -227,7 +235,11 @@ impl RoundExecutor { "Dropping invalid partial tool calls from interrupted stream; preserving already-streamed assistant text: session_id={}, round_id={}, invalid_tool_calls={}, error={}", context.session_id, round_id, - result.tool_calls.iter().filter(|tool_call| !tool_call.is_valid()).count(), + result + .tool_calls + .iter() + .filter(|tool_call| !tool_call.is_valid()) + .count(), err_msg ); self.emit_failed_partial_tool_calls( @@ -337,7 +349,10 @@ impl RoundExecutor { round_id, attempt_index + 1, max_attempts, - result.partial_recovery_reason.as_deref().unwrap_or("unknown") + result + .partial_recovery_reason + .as_deref() + .unwrap_or("unknown") ); } @@ -863,6 +878,14 @@ impl RoundExecutor { .to_string(); let tool_id = tc.tool_id.clone(); + if let Some(error) = Self::write_content_preflight_error(context, &file_path).await { + debug!( + "Skipping Write content generation after preflight failure: file_path={}, error={}", + file_path, error + ); + continue; + } + // Emit Started event so the UI can show the tool card self.emit_event( AgenticEvent::ToolEvent { @@ -1022,6 +1045,39 @@ impl RoundExecutor { Ok(tool_calls) } + async fn write_content_preflight_error( + context: &RoundContext, + file_path: &str, + ) -> Option { + let tool_context = Self::build_write_preflight_context(context); + let resolved = match tool_context.resolve_tool_path(file_path) { + Ok(resolved) => resolved, + Err(error) => return Some(error.to_string()), + }; + + if let Err(error) = tool_context.enforce_path_operation(ToolPathOperation::Write, &resolved) + { + return Some(error.to_string()); + } + + FileWriteTool::existing_file_error(&tool_context, &resolved).await + } + + fn build_write_preflight_context(context: &RoundContext) -> ToolUseContext { + ToolUseContext { + tool_call_id: None, + agent_type: Some(context.agent_type.clone()), + session_id: Some(context.session_id.clone()), + dialog_turn_id: Some(context.dialog_turn_id.clone()), + workspace: context.workspace.clone(), + custom_data: HashMap::new(), + computer_use_host: None, + cancellation_token: None, + runtime_tool_restrictions: context.runtime_tool_restrictions.clone(), + workspace_services: context.workspace_services.clone(), + } + } + /// Emit event async fn emit_event(&self, event: AgenticEvent, priority: EventPriority) { let _ = self.event_queue.enqueue(event, Some(priority)).await; @@ -1409,7 +1465,7 @@ fn detect_placeholder_patterns(content: &str) -> Option<&'static str> { #[cfg(test)] mod tests { - use super::{extract_bitfun_contents, RoundExecutor, StreamProcessor}; + use super::{RoundExecutor, StreamProcessor, extract_bitfun_contents}; use crate::agentic::events::{EventQueue, EventQueueConfig}; use dashmap::DashMap; use std::sync::Arc; @@ -1623,7 +1679,10 @@ mod tests { fn sanitization_preserves_xml_in_file_content() { // Real XML that should be part of the file let text = "\ntest\n"; - assert_eq!(extract_bitfun_contents(text), "test"); + assert_eq!( + extract_bitfun_contents(text), + "test" + ); } // --- Placeholder detection tests --- diff --git a/src/crates/core/src/agentic/tools/implementations/file_write_tool.rs b/src/crates/core/src/agentic/tools/implementations/file_write_tool.rs index 6b15318af..e9b19e43e 100644 --- a/src/crates/core/src/agentic/tools/implementations/file_write_tool.rs +++ b/src/crates/core/src/agentic/tools/implementations/file_write_tool.rs @@ -1,10 +1,10 @@ +use crate::agentic::tools::ToolPathOperation; use crate::agentic::tools::framework::{ - Tool, ToolRenderOptions, ToolResult, ToolUseContext, ValidationResult, + Tool, ToolPathResolution, ToolRenderOptions, ToolResult, ToolUseContext, ValidationResult, }; -use crate::agentic::tools::ToolPathOperation; use crate::util::errors::{BitFunError, BitFunResult}; use async_trait::async_trait; -use serde_json::{json, Value}; +use serde_json::{Value, json}; use std::path::Path; use tokio::fs; @@ -20,6 +20,177 @@ impl FileWriteTool { pub fn new() -> Self { Self } + + pub(crate) async fn existing_file_error( + context: &ToolUseContext, + resolved: &ToolPathResolution, + ) -> Option { + let file_already_exists = Self::file_exists(context, resolved).await; + + file_already_exists.then(|| { + format!( + "File {} already exists. The Write tool is reserved for creating NEW files. \ + To modify the file, use the Edit tool. \ + To fully rewrite the file, first call the Delete tool on this path, then call Write again.", + resolved.logical_path + ) + }) + } + + async fn file_exists(context: &ToolUseContext, resolved: &ToolPathResolution) -> bool { + if resolved.uses_remote_workspace_backend() { + if let Some(ws_fs) = context.ws_fs() { + ws_fs.exists(&resolved.resolved_path).await.unwrap_or(false) + } else { + false + } + } else { + Path::new(&resolved.resolved_path).exists() + } + } + + async fn existing_file_matches_content( + context: &ToolUseContext, + resolved: &ToolPathResolution, + content: &str, + ) -> Option { + let existing = if resolved.uses_remote_workspace_backend() { + context + .ws_fs()? + .read_file(&resolved.resolved_path) + .await + .ok()? + } else { + fs::read(&resolved.resolved_path).await.ok()? + }; + + Some(existing == content.as_bytes()) + } + + fn write_success_result( + logical_path: &str, + bytes_written: usize, + status: &str, + assistant_message: String, + ) -> ToolResult { + ToolResult::Result { + data: json!({ + "file_path": logical_path, + "bytes_written": bytes_written, + "success": true, + "status": status, + "message": assistant_message, + }), + result_for_assistant: Some(assistant_message), + image_attachments: None, + } + } +} + +#[cfg(test)] +mod tests { + use super::FileWriteTool; + use crate::agentic::WorkspaceBinding; + use crate::agentic::tools::ToolRuntimeRestrictions; + use crate::agentic::tools::framework::{Tool, ToolResult, ToolUseContext}; + use serde_json::json; + use std::collections::HashMap; + use std::path::PathBuf; + + fn local_context(root: PathBuf) -> ToolUseContext { + ToolUseContext { + tool_call_id: None, + agent_type: None, + session_id: None, + dialog_turn_id: None, + workspace: Some(WorkspaceBinding::new(None, root)), + custom_data: HashMap::new(), + computer_use_host: None, + cancellation_token: None, + runtime_tool_restrictions: ToolRuntimeRestrictions::default(), + workspace_services: None, + } + } + + #[tokio::test] + async fn validate_input_rejects_existing_file_before_content_generation() { + let root = std::env::temp_dir().join(format!("bitfun-write-test-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&root).expect("create temp workspace"); + let existing_file = root.join("existing.md"); + std::fs::write(&existing_file, "already here").expect("create existing file"); + + let tool = FileWriteTool::new(); + let validation = tool + .validate_input( + &json!({ "file_path": "existing.md" }), + Some(&local_context(root.clone())), + ) + .await; + + let _ = std::fs::remove_dir_all(&root); + + assert!(!validation.result); + let message = validation.message.unwrap_or_default(); + assert!(message.contains("already exists")); + assert!(message.contains("Edit tool")); + } + + #[tokio::test] + async fn call_impl_treats_identical_existing_content_as_success() { + let root = std::env::temp_dir().join(format!("bitfun-write-test-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&root).expect("create temp workspace"); + std::fs::write(root.join("existing.md"), "same content").expect("create existing file"); + + let tool = FileWriteTool::new(); + let results = tool + .call( + &json!({ "file_path": "existing.md", "content": "same content" }), + &local_context(root.clone()), + ) + .await + .expect("identical retry should be idempotent"); + + let _ = std::fs::remove_dir_all(&root); + + let ToolResult::Result { + data, + result_for_assistant, + .. + } = &results[0] + else { + panic!("expected result"); + }; + assert_eq!(data["success"], true); + assert_eq!(data["bytes_written"], 0); + assert_eq!(data["status"], "already_exists_same_content"); + assert!( + result_for_assistant + .as_deref() + .unwrap_or_default() + .contains("do not call Write for this path again") + ); + } + + #[tokio::test] + async fn call_impl_rejects_different_existing_content() { + let root = std::env::temp_dir().join(format!("bitfun-write-test-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&root).expect("create temp workspace"); + std::fs::write(root.join("existing.md"), "old content").expect("create existing file"); + + let tool = FileWriteTool::new(); + let error = tool + .call( + &json!({ "file_path": "existing.md", "content": "new content" }), + &local_context(root.clone()), + ) + .await + .expect_err("different content must not overwrite existing files"); + + let _ = std::fs::remove_dir_all(&root); + + assert!(error.to_string().contains("already exists")); + assert!(error.to_string().contains("Edit tool")); + } } #[async_trait] @@ -35,6 +206,7 @@ Usage: - This tool is for creating NEW files only. Calling Write on a path that already exists will be REJECTED with an error. - To MODIFY an existing file, use the Edit tool — it is the correct choice in almost every case. - To FULLY REWRITE an existing file (e.g. regenerate a generated file, replace a template), first call the Delete tool on that path, then call Write to create the new version. Do not try to "overwrite" via Write directly. +- After Write succeeds for a path, do not call Write for that path again in later rounds. Use Edit for any additional changes. - The file_path parameter must be workspace-relative, an absolute path inside the current workspace, or an exact `bitfun://runtime/...` URI returned by another tool. - ALWAYS prefer editing existing files in the codebase. NEVER write new files unless explicitly required. - NEVER proactively create documentation files (*.md) or README files. Only create documentation files if explicitly requested by the User. @@ -106,6 +278,23 @@ Usage: meta: None, }; } + + // If content is absent, RoundExecutor would otherwise launch a + // second model request to generate the full file. Reject existing + // targets here so we do not spend tokens producing content that + // Write must reject anyway. If a model already supplied content + // despite the public schema, defer to call_impl so identical + // retries can be treated as idempotent success. + if input.get("content").is_none() { + if let Some(error) = Self::existing_file_error(ctx, &resolved).await { + return ValidationResult { + result: false, + message: Some(error), + error_code: Some(400), + meta: None, + }; + } + } } ValidationResult::default() @@ -148,33 +337,29 @@ Usage: ) .await; - // Guard: refuse to overwrite an existing file — the model should use - // Edit instead. This prevents accidental data loss from models that - // call Write repeatedly on the same file with incomplete content. - let file_already_exists = if resolved.uses_remote_workspace_backend() { - if let Some(ws_fs) = context.ws_fs() { - ws_fs.exists(&resolved.resolved_path).await.unwrap_or(false) - } else { - false - } - } else { - Path::new(&resolved.resolved_path).exists() - }; - - if file_already_exists { - return Err(BitFunError::tool(format!( - "File {} already exists. The Write tool is reserved for creating NEW files. \ - To modify the file, use the Edit tool. \ - To fully rewrite the file, first call the Delete tool on this path, then call Write again.", - resolved.logical_path - ))); - } - let content = input .get("content") .and_then(|v| v.as_str()) .ok_or_else(|| BitFunError::tool("content is required".to_string()))?; + if let Some(error) = Self::existing_file_error(context, &resolved).await { + if Self::existing_file_matches_content(context, &resolved, content).await == Some(true) + { + let result = Self::write_success_result( + &resolved.logical_path, + 0, + "already_exists_same_content", + format!( + "Write skipped because {} already exists with identical content. Treat this file as successfully created and do not call Write for this path again. Use Edit for any further changes.", + resolved.logical_path + ), + ); + return Ok(vec![result]); + } + + return Err(BitFunError::tool(error)); + } + if resolved.uses_remote_workspace_backend() { let ws_fs = context.ws_fs().ok_or_else(|| { BitFunError::tool("Remote workspace file system is unavailable".to_string()) @@ -199,15 +384,16 @@ Usage: })?; } - let result = ToolResult::Result { - data: json!({ - "file_path": resolved.logical_path, - "bytes_written": content.len(), - "success": true - }), - result_for_assistant: Some(format!("Successfully wrote to {}", resolved.logical_path)), - image_attachments: None, - }; + let result = Self::write_success_result( + &resolved.logical_path, + content.len(), + "created", + format!( + "Successfully created {} ({} bytes). The file now exists; do not call Write for this path again. Use Edit for any further changes.", + resolved.logical_path, + content.len() + ), + ); Ok(vec![result]) }