diff --git a/src/crates/core/src/agentic/execution/round_executor.rs b/src/crates/core/src/agentic/execution/round_executor.rs index 8c0a9d80b..69713e287 100644 --- a/src/crates/core/src/agentic/execution/round_executor.rs +++ b/src/crates/core/src/agentic/execution/round_executor.rs @@ -4,15 +4,12 @@ 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::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::tools::ToolPathOperation; -use crate::agentic::MessageContent; use crate::infrastructure::ai::AIClient; use crate::service::config::GlobalConfigManager; use crate::util::elapsed_ms_u64; @@ -20,8 +17,7 @@ use crate::util::errors::{BitFunError, BitFunResult}; 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 log::{debug, error, warn}; use std::sync::Arc; use std::time::{Duration, Instant}; use tokio_util::sync::CancellationToken; @@ -551,23 +547,7 @@ impl RoundExecutor { return Err(BitFunError::Cancelled("Execution cancelled".to_string())); } - // ---- Write tool content generation ---- - // For Write tool calls without a "content" field, spawn a separate AI - // request with the full session history to generate the file content as - // plain text wrapped in tags. This avoids having the - // model emit large file contents inside JSON tool-call arguments, which - // is a major source of JSON parse failures. let tool_calls = stream_result.tool_calls.clone(); - let tool_calls = self - .generate_write_tool_contents( - ai_client.clone(), - &context, - &ai_messages, - tool_calls, - &cancel_token, - event_subagent_parent_info.clone(), - ) - .await?; // Execute tool calls debug!( @@ -825,262 +805,6 @@ impl RoundExecutor { } } - /// Generate file content for Write tool calls that lack a `content` field. - /// - /// When a Write tool call arrives without `content`, this method spawns a - /// separate AI request with the full session history and a directive to - /// output the file content as plain text inside `` tags. - /// The extracted content is then injected into the tool call arguments so - /// the downstream Write tool execution proceeds as normal. - async fn generate_write_tool_contents( - &self, - ai_client: Arc, - context: &RoundContext, - ai_messages: &[AIMessage], - mut tool_calls: Vec, - cancel_token: &CancellationToken, - subagent_parent_info: Option, - ) -> BitFunResult> { - // Find indices of Write tool calls that need content generation - let write_indices: Vec = tool_calls - .iter() - .enumerate() - .filter(|(_, tc)| { - tc.tool_name == "Write" - && tc.arguments.get("content").is_none() - && tc - .arguments - .get("file_path") - .and_then(|v| v.as_str()) - .is_some() - }) - .map(|(i, _)| i) - .collect(); - - if write_indices.is_empty() { - return Ok(tool_calls); - } - - info!( - "Generating content for {} Write tool call(s) via separate AI request", - write_indices.len() - ); - - for idx in &write_indices { - if cancel_token.is_cancelled() { - return Err(BitFunError::Cancelled("Execution cancelled".to_string())); - } - - let tc = &tool_calls[*idx]; - let file_path = tc - .arguments - .get("file_path") - .and_then(|v| v.as_str()) - .unwrap_or("") - .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 { - session_id: context.session_id.clone(), - turn_id: context.dialog_turn_id.clone(), - tool_event: ToolEventData::Started { - tool_id: tool_id.clone(), - tool_name: "Write".to_string(), - params: tc.arguments.clone(), - timeout_seconds: None, - }, - subagent_parent_info: subagent_parent_info.clone(), - }, - EventPriority::High, - ) - .await; - - // Build a content-generation prompt - let content_prompt = format!( - "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 - ); - - 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 { - Ok(response) => { - let mut text = String::new(); - let mut stream = response.stream; - use futures::StreamExt; - while let Some(chunk) = stream.next().await { - if cancel_token.is_cancelled() { - return Err(BitFunError::Cancelled("Execution cancelled".to_string())); - } - match chunk { - Ok(resp) => { - let chunk_text = resp.text.unwrap_or_default(); - if !chunk_text.is_empty() { - text.push_str(&chunk_text); - - // Emit streaming ParamsPartial so the UI - // shows a live content preview - let params = serde_json::json!({ - "file_path": &file_path, - "content": &text, - }); - self.emit_event( - AgenticEvent::ToolEvent { - session_id: context.session_id.clone(), - turn_id: context.dialog_turn_id.clone(), - tool_event: ToolEventData::ParamsPartial { - tool_id: tool_id.clone(), - tool_name: "Write".to_string(), - params: params.to_string(), - }, - subagent_parent_info: subagent_parent_info.clone(), - }, - EventPriority::Normal, - ) - .await; - } - } - Err(e) => { - error!("Error in Write content generation stream: {}", e); - break; - } - } - } - text - } - Err(e) => { - error!("Write content generation request failed: {}", e); - return Err(BitFunError::AIClient(format!( - "Write content generation failed for {}: {}", - file_path, e - ))); - } - }; - - let content = extract_bitfun_contents(&full_text); - if content.is_empty() { - warn!( - "Write content generation returned empty content for file_path={}", - file_path - ); - } - - // 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, - }); - self.emit_event( - AgenticEvent::ToolEvent { - session_id: context.session_id.clone(), - turn_id: context.dialog_turn_id.clone(), - tool_event: ToolEventData::ParamsPartial { - tool_id: tool_id.clone(), - tool_name: "Write".to_string(), - params: final_params.to_string(), - }, - subagent_parent_info: subagent_parent_info.clone(), - }, - EventPriority::Normal, - ) - .await; - - // Inject content into the tool call arguments - tool_calls[*idx] - .arguments - .as_object_mut() - .expect("Write tool arguments must be a JSON object") - .insert("content".to_string(), serde_json::Value::String(content)); - - debug!( - "Write content generated: file_path={}, content_len={}", - file_path, - tool_calls[*idx] - .arguments - .get("content") - .and_then(|v| v.as_str()) - .map(|s| s.len()) - .unwrap_or(0) - ); - } - - 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(), - unlocked_collapsed_tools: context.unlocked_collapsed_tools.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; @@ -1256,219 +980,9 @@ fn token_details_from_usage( (!details.is_empty()).then_some(serde_json::Value::Object(details)) } -/// Extract content from `...` tags. -/// -/// If the tags are present, returns the text between them (trimmed). -/// If the tags are not present, returns the full text trimmed (fallback for -/// models that ignore the tag instruction). -fn extract_bitfun_contents(text: &str) -> String { - const OPEN_TAG: &str = ""; - const CLOSE_TAG: &str = ""; - - 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) { - &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..]); - } - } - } - - 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()); - } } 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 0598059fc..5306ac3f0 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 @@ -113,8 +113,45 @@ mod tests { } } + #[test] + fn input_schema_requires_explicit_content() { + let tool = FileWriteTool::new(); + let schema = tool.input_schema(); + + assert!(schema["properties"].get("content").is_some()); + let required = schema["required"] + .as_array() + .expect("required fields should be an array"); + assert!(required.iter().any(|value| value == "file_path")); + assert!(required.iter().any(|value| value == "content")); + } + + #[tokio::test] + async fn validate_input_rejects_missing_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"); + + let tool = FileWriteTool::new(); + let validation = tool + .validate_input( + &json!({ "file_path": "new.md" }), + Some(&local_context(root.clone())), + ) + .await; + + let _ = std::fs::remove_dir_all(&root); + + assert!(!validation.result); + assert!( + validation + .message + .unwrap_or_default() + .contains("content is required") + ); + } + #[tokio::test] - async fn validate_input_rejects_existing_file_before_content_generation() { + async fn validate_input_rejects_existing_file() { 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"); @@ -123,7 +160,7 @@ mod tests { let tool = FileWriteTool::new(); let validation = tool .validate_input( - &json!({ "file_path": "existing.md" }), + &json!({ "file_path": "existing.md", "content": "new content" }), Some(&local_context(root.clone())), ) .await; @@ -212,7 +249,7 @@ Usage: - 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. - 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()) +- The content parameter must contain the exact full file content to write. Do not include markdown fences, explanations, tool transcripts, or placeholder comments unless they are literally part of the target file."#.to_string()) } fn short_description(&self) -> String { @@ -226,9 +263,13 @@ Usage: "file_path": { "type": "string", "description": "The file to write. Use a workspace-relative path, an absolute path inside the current workspace, or an exact bitfun://runtime URI returned by another tool." + }, + "content": { + "type": "string", + "description": "The exact full file content to write. Do not include markdown fences, explanations, tool transcripts, or omitted-code placeholders unless they are literally part of the target file." } }, - "required": ["file_path"], + "required": ["file_path", "content"], "additionalProperties": false }) } @@ -262,6 +303,18 @@ Usage: } }; + match input.get("content").and_then(|v| v.as_str()) { + Some(_) => {} + None => { + return ValidationResult { + result: false, + message: Some("content is required".to_string()), + error_code: Some(400), + meta: None, + }; + } + } + if let Some(ctx) = context { let resolved = match ctx.resolve_tool_path(file_path) { Ok(resolved) => resolved, @@ -284,21 +337,13 @@ Usage: }; } - // 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, - }; - } + if let Some(error) = Self::existing_file_error(ctx, &resolved).await { + return ValidationResult { + result: false, + message: Some(error), + error_code: Some(400), + meta: None, + }; } }