diff --git a/lib/crates/fabro-agent/src/profiles/openai.rs b/lib/crates/fabro-agent/src/profiles/openai.rs index d860a5db3..ff0215d5c 100644 --- a/lib/crates/fabro-agent/src/profiles/openai.rs +++ b/lib/crates/fabro-agent/src/profiles/openai.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use fabro_model::{AgentProfileKind, Catalog, ProviderId}; +use fabro_model::{AgentProfileKind, Catalog, CodecKind, ProviderId}; use super::EnvContext; use crate::agent_profile::AgentProfile; @@ -12,10 +12,27 @@ use crate::skills::Skill; use crate::todo_runtime::TodoRuntime; use crate::todo_tools::make_update_plan_tool; use crate::tool_registry::ToolRegistry; -use crate::tools::{WebFetchSummarizer, register_core_tools}; +use crate::tools::{self, WebFetchSummarizer, register_core_tools}; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum FileEditToolKind { + ApplyPatch, + EditFile, +} + +impl FileEditToolKind { + fn for_codec(codec: CodecKind) -> Self { + if codec == CodecKind::OpenAiResponses { + Self::ApplyPatch + } else { + Self::EditFile + } + } +} pub struct OpenAiProfile { - base: BaseProfile, + base: BaseProfile, + file_edit_tool: FileEditToolKind, } impl OpenAiProfile { @@ -39,13 +56,14 @@ impl OpenAiProfile { registry.register(make_update_plan_tool(todo_runtime)); Self { - base: BaseProfile { + base: BaseProfile { profile_kind: AgentProfileKind::OpenAi, provider_id: ProviderId::openai(), model: model.into(), catalog: None, registry, }, + file_edit_tool: FileEditToolKind::ApplyPatch, } } @@ -53,15 +71,43 @@ impl OpenAiProfile { #[must_use] pub fn with_provider_id(mut self, provider_id: ProviderId) -> Self { self.base.provider_id = provider_id; + self.configure_file_edit_tool(); self } #[must_use] pub fn with_catalog(mut self, catalog: Arc) -> Self { self.base.catalog = Some(catalog); + self.configure_file_edit_tool(); self } + fn configure_file_edit_tool(&mut self) { + let Some(codec) = self.base.catalog.as_ref().and_then(|catalog| { + catalog.effective_codec(&self.base.provider_id, Some(&self.base.model)) + }) else { + return; + }; + let desired = FileEditToolKind::for_codec(codec); + if desired == self.file_edit_tool { + return; + } + + match desired { + FileEditToolKind::ApplyPatch => { + self.base.registry.unregister("edit_file"); + self.base + .registry + .register(apply_patch::make_apply_patch_tool()); + } + FileEditToolKind::EditFile => { + self.base.registry.unregister("apply_patch"); + self.base.registry.register(tools::make_edit_file_tool()); + } + } + self.file_edit_tool = desired; + } + fn provider_display_name(&self) -> String { self.base .catalog @@ -108,13 +154,47 @@ impl AgentProfile for OpenAiProfile { skills: &[Skill], ) -> String { let provider_name = self.provider_display_name(); + let (file_edit_tool_name, file_edit_failure_guidance, file_edit_tool_guidance) = + match self.file_edit_tool { + FileEditToolKind::ApplyPatch => ( + "apply_patch", + "- When apply_patch fails, use the error text to construct a corrected patch. \ +Re-read the target file if you need fresh context.", + "## apply_patch +Use the `apply_patch` tool for all file modifications. This is a freeform tool: pass the raw \ +patch text directly, never wrap it in JSON. The format uses `*** Begin Patch` / \ +`*** End Patch` delimiters with `*** Add File:`, `*** Delete File:`, `*** Update File:` \ +operations. Use `-` for removals, `+` for additions, and space-prefix for unchanged context \ +lines. Show 3 lines of context around each change. NEVER use `applypatch` or `apply-patch`, \ +only `apply_patch`. + +Example: +``` +*** Begin Patch +*** Update File: src/main.py +@@ def hello(): +- print(\"old\") ++ print(\"new\") +*** End Patch +```", + ), + FileEditToolKind::EditFile => ( + "edit_file", + "- When edit_file fails, use the error text to construct a corrected exact \ +replacement. Re-read the target file if you need fresh context.", + "## edit_file +Use `edit_file` to modify an existing file by replacing an exact string. Read the file first. \ +The `old_string` must match exactly and be unique unless `replace_all` is true; include enough \ +surrounding context to make the match unique and preserve the existing indentation.", + ), + }; let core_prompt = format!("\ You are a coding agent powered by {provider_name}, running in a terminal-based agentic coding assistant. \ You are expected to be precise, safe, and helpful. You can receive user prompts and context such as files in the workspace, communicate with the \ user by streaming thinking and responses, and emit function calls to run terminal commands and \ -apply patches. +edit files. # Personality @@ -146,8 +226,7 @@ If completing the task requires writing or modifying files: and focused on the task. - Use `git log` and `git blame` to search the history of the codebase if additional context is needed. - NEVER add copyright or license headers unless specifically requested. -- When apply_patch fails, use the error text to construct a corrected patch. Re-read the target \ -file if you need fresh context. +{file_edit_failure_guidance} - Do not `git commit` your changes or create new git branches unless explicitly requested. # Validating Your Work @@ -163,26 +242,10 @@ Use the provided tools to interact with the codebase and environment. ## read_file Read files to understand code before modifying. Use offset/limit for large files. -## apply_patch -Use the `apply_patch` tool for all file modifications. This is a freeform tool: pass the raw \ -patch text directly, never wrap it in JSON. The format uses `*** Begin Patch` / \ -`*** End Patch` delimiters with `*** Add File:`, `*** Delete File:`, `*** Update File:` \ -operations. Use `-` for removals, `+` for additions, and space-prefix for unchanged context \ -lines. Show 3 lines of context around each change. NEVER use `applypatch` or `apply-patch`, \ -only `apply_patch`. - -Example: -``` -*** Begin Patch -*** Update File: src/main.py -@@ def hello(): -- print(\"old\") -+ print(\"new\") -*** End Patch -``` +{file_edit_tool_guidance} ## write_file -Use for creating new files. For modifications, prefer apply_patch. +Use for creating new files. For modifications, prefer {file_edit_tool_name}. ## shell Execute shell commands. Default timeout is 10 seconds. Use timeout_ms parameter for \ @@ -350,6 +413,44 @@ mod tests { assert!(!prompt.contains("powered by OpenAI")); } + #[test] + fn openai_compatible_profile_uses_json_schema_edit_tool() { + let profile = OpenAiProfile::new("kimi-k2.5") + .with_provider_id(ProviderId::new("kimi")) + .with_catalog(test_catalog()); + + let names = profile.tool_registry().names(); + assert!(names.contains(&"edit_file".to_string())); + assert!(!names.contains(&"apply_patch".to_string())); + + let edit_file = profile.tool_registry().get("edit_file").unwrap(); + assert!(!edit_file.definition.is_custom()); + assert_eq!(edit_file.definition.parameters["type"], "object"); + for definition in profile.tool_registry().definitions() { + assert_eq!( + definition.parameters["type"], "object", + "tool '{}' must use an object parameter schema", + definition.name + ); + } + + let env = MockSandbox::linux(); + let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None, &[]); + assert!(prompt.contains("## edit_file")); + assert!(!prompt.contains("## apply_patch")); + assert!(!prompt.contains("freeform tool")); + } + + #[test] + fn file_edit_tool_selection_is_builder_order_independent() { + let profile = OpenAiProfile::new("kimi-k2.5") + .with_catalog(test_catalog()) + .with_provider_id(ProviderId::new("kimi")); + + assert!(profile.tool_registry().get("edit_file").is_some()); + assert!(profile.tool_registry().get("apply_patch").is_none()); + } + #[test] fn zai_provider_prompt_uses_catalog_display_name() { let profile = OpenAiProfile::new("glm-4.7") diff --git a/lib/crates/fabro-agent/tests/it/parity_matrix.rs b/lib/crates/fabro-agent/tests/it/parity_matrix.rs index 3c720ec73..808d8b71d 100644 --- a/lib/crates/fabro-agent/tests/it/parity_matrix.rs +++ b/lib/crates/fabro-agent/tests/it/parity_matrix.rs @@ -56,11 +56,16 @@ fn build_summarizer(provider: &Provider, client: &Client) -> WebFetchSummarizer fn build_profile(provider: &Provider, model: &str, client: &Client) -> Box { let summarizer = Some(build_summarizer(provider, client)); + let catalog = Arc::new(Catalog::from_builtin().expect("default catalog should build")); match provider.as_str() { ProviderId::ANTHROPIC => Box::new(AnthropicProfile::with_summarizer(model, summarizer)), - ProviderId::OPENAI => Box::new(OpenAiProfile::with_summarizer(model, summarizer)), + ProviderId::OPENAI => Box::new( + OpenAiProfile::with_summarizer(model, summarizer).with_catalog(Arc::clone(&catalog)), + ), "kimi" | "zai" | "minimax" | "inception" => Box::new( - OpenAiProfile::with_summarizer(model, summarizer).with_provider_id(provider.clone()), + OpenAiProfile::with_summarizer(model, summarizer) + .with_provider_id(provider.clone()) + .with_catalog(Arc::clone(&catalog)), ), ProviderId::GEMINI => Box::new(GeminiProfile::with_summarizer(model, summarizer)), other => panic!("unexpected provider {other}"), @@ -85,6 +90,7 @@ async fn make_session( let factory_cwd = cwd.to_path_buf(); let factory_provider = provider.clone(); let factory: SessionFactory = Arc::new(move || { + let catalog = Arc::new(Catalog::from_builtin().expect("default catalog should build")); let sub_profile: Arc = { let summarizer = Some(build_summarizer(&factory_provider, &factory_client)); match factory_provider.as_str() { @@ -92,12 +98,14 @@ async fn make_session( &factory_model, summarizer, )), - ProviderId::OPENAI => { - Arc::new(OpenAiProfile::with_summarizer(&factory_model, summarizer)) - } + ProviderId::OPENAI => Arc::new( + OpenAiProfile::with_summarizer(&factory_model, summarizer) + .with_catalog(Arc::clone(&catalog)), + ), "kimi" | "zai" | "minimax" | "inception" => Arc::new( OpenAiProfile::with_summarizer(&factory_model, summarizer) - .with_provider_id(factory_provider.clone()), + .with_provider_id(factory_provider.clone()) + .with_catalog(Arc::clone(&catalog)), ), ProviderId::GEMINI => { Arc::new(GeminiProfile::with_summarizer(&factory_model, summarizer)) @@ -176,8 +184,12 @@ fn make_openai_compatible_twin_session( twin: &OpenAiTwinOptions, ) -> Session { let client = make_openai_compatible_twin_client(&provider, twin); - let profile: Arc = - Arc::new(OpenAiProfile::new(model).with_provider_id(provider)); + let catalog = Arc::new(Catalog::from_builtin().expect("default catalog should build")); + let profile: Arc = Arc::new( + OpenAiProfile::new(model) + .with_provider_id(provider) + .with_catalog(catalog), + ); let env = Arc::new(LocalSandbox::new(cwd.to_path_buf())); Session::new(client, profile, env, config, None) } @@ -271,27 +283,25 @@ macro_rules! provider_tests { } #[fabro_macros::e2e_test(twin)] -async fn openai_compatible_twin_preserves_raw_apply_patch_arguments() { +async fn openai_compatible_twin_uses_json_edit_file_tool() { let tmp = tempfile::tempdir().expect("failed to create tempdir"); let file_path = tmp.path().join("data.txt"); std::fs::write(&file_path, "old\n").expect("failed to write data.txt"); let (base_url, api_key) = fabro_test::e2e_openai!(); let twin = OpenAiTwinOptions { base_url, api_key }; - let patch = "\ -*** Begin Patch -*** Update File: data.txt -@@ --old -+new -*** End Patch -"; - TwinScenarios::new(twin.api_key.clone()) .scenario( TwinScenario::chat_completions("gpt-5.4-mini") .input_contains("Replace old with new") - .tool_call(TwinToolCall::apply_patch_raw_arguments(patch)), + .tool_call(TwinToolCall::new( + "edit_file", + serde_json::json!({ + "file_path": "data.txt", + "old_string": "old", + "new_string": "new" + }), + )), ) .load(twin_openai().await) .await; @@ -311,7 +321,7 @@ async fn openai_compatible_twin_preserves_raw_apply_patch_arguments() { let mut rx = session.subscribe(); session - .process_input("Replace old with new in data.txt using apply_patch") + .process_input("Replace old with new in data.txt using edit_file") .await .expect("process_input failed"); diff --git a/lib/crates/fabro-llm/src/codec/openai_compatible/mod.rs b/lib/crates/fabro-llm/src/codec/openai_compatible/mod.rs index ef45023d5..42a2fde09 100644 --- a/lib/crates/fabro-llm/src/codec/openai_compatible/mod.rs +++ b/lib/crates/fabro-llm/src/codec/openai_compatible/mod.rs @@ -21,7 +21,7 @@ pub(crate) struct OpenAiCompatible; impl Codec for OpenAiCompatible { fn encode(&self, ctx: &CodecCtx<'_>, stream: bool) -> Result { - Ok(request::encode(ctx, stream)) + request::encode(ctx, stream) } fn decode_response( diff --git a/lib/crates/fabro-llm/src/codec/openai_compatible/request.rs b/lib/crates/fabro-llm/src/codec/openai_compatible/request.rs index 7fcb81abb..2b8abf6b9 100644 --- a/lib/crates/fabro-llm/src/codec/openai_compatible/request.rs +++ b/lib/crates/fabro-llm/src/codec/openai_compatible/request.rs @@ -3,20 +3,22 @@ use super::translate; use super::wire::ApiRequest; use crate::codec::{CodecCtx, EncodedRequest, merge_named_provider_options}; +use crate::error::Error; /// Build the Chat Completions request for `ctx.request`. `stream` toggles the /// `stream` body field. The body is assembled as a `serde_json::Value` so /// `provider_options.` fields can be merged in before sending. /// -/// Infallible for this dialect — the `Codec::encode` `Result` is wrapped by the -/// trait impl. -pub(super) fn encode(ctx: &CodecCtx<'_>, stream: bool) -> EncodedRequest { +/// Returns an error when the request contains a custom tool definition, which +/// the Chat Completions tool envelope cannot represent. +pub(super) fn encode(ctx: &CodecCtx<'_>, stream: bool) -> Result { let request = ctx.request; let chat_messages = translate::translate_messages(&request.messages); let tools = request .tools .as_ref() - .map(|t| translate::translate_tools(t)); + .map(|t| translate::translate_tools(t)) + .transpose()?; let tool_choice = request .tool_choice .as_ref() @@ -46,11 +48,11 @@ pub(super) fn encode(ctx: &CodecCtx<'_>, stream: bool) -> EncodedRequest { ctx.provider_name, ); - EncodedRequest { + Ok(EncodedRequest { body, endpoint: "/chat/completions".to_string(), headers: Vec::new(), - } + }) } /// Merge `provider_options.` fields into the serialized API @@ -71,7 +73,7 @@ mod tests { use super::super::wire::ApiRequest; use super::*; use crate::codec::CodecParams; - use crate::types::{Message, Request}; + use crate::types::{Message, Request, ToolDefinition}; fn minimal_request() -> Request { Request { @@ -104,7 +106,7 @@ mod tests { model: None, params: ¶ms, }; - encode(&ctx, stream).body + encode(&ctx, stream).unwrap().body } #[test] @@ -152,10 +154,38 @@ mod tests { model: None, params: ¶ms, }; - let body = encode(&ctx, false).body; + let body = encode(&ctx, false).unwrap().body; assert_eq!(body["model"], "acme/model-large"); } + #[test] + fn encode_rejects_custom_tool_definitions() { + let mut request = minimal_request(); + request.tools = Some(vec![ToolDefinition::custom( + "apply_patch", + "Apply a patch", + serde_json::json!({"type": "grammar"}), + )]); + let params = CodecParams::default(); + let deployment_id = request.model.clone(); + let ctx = CodecCtx { + request: &request, + provider_name: "kimi", + deployment_id: &deployment_id, + model: None, + params: ¶ms, + }; + + let Err(error) = encode(&ctx, false) else { + panic!("custom tool definition should be rejected"); + }; + assert!(matches!( + error, + Error::Configuration { message, source: None } + if message.contains("custom tool definition 'apply_patch'") + )); + } + #[test] fn provider_options_none_produces_standard_body() { let request = minimal_request(); diff --git a/lib/crates/fabro-llm/src/codec/openai_compatible/translate.rs b/lib/crates/fabro-llm/src/codec/openai_compatible/translate.rs index 99c751a22..127af26a2 100644 --- a/lib/crates/fabro-llm/src/codec/openai_compatible/translate.rs +++ b/lib/crates/fabro-llm/src/codec/openai_compatible/translate.rs @@ -1,6 +1,7 @@ //! Pure mapping between canonical types and the Chat Completions wire shapes. use super::wire::{ChatFunction, ChatMessage, ChatToolCall}; +use crate::error::Error; use crate::types::{ ContentPart, CostSource, FinishReason, Message, Request, ResponseFormat, ResponseFormatType, Role, ToolChoice, ToolDefinition, @@ -146,18 +147,28 @@ pub(super) fn translate_messages(messages: &[Message]) -> Vec { .collect() } -pub(super) fn translate_tools(tools: &[ToolDefinition]) -> Vec { +pub(super) fn translate_tools(tools: &[ToolDefinition]) -> Result, Error> { tools .iter() .map(|t| { - serde_json::json!({ + if t.is_custom() { + return Err(Error::Configuration { + message: format!( + "openai_compatible codec does not support custom tool definition '{}'", + t.name + ), + source: None, + }); + } + + Ok(serde_json::json!({ "type": "function", "function": { "name": t.name, "description": t.description, "parameters": t.parameters, } - }) + })) }) .collect() }