From 91d11eb04d1bc49c91f2320fa69b73ccec4aabc5 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Thu, 28 May 2026 11:52:36 -0400 Subject: [PATCH] fix(llm): preserve raw compatible tool arguments (#448) ## Summary Fixes #435. Preserve raw non-JSON tool-call arguments for custom/freeform tools when using the OpenAI-compatible Chat Completions adapter. This keeps `apply_patch` receiving the raw patch text instead of `{}` when LiteLLM/openai-compatible providers emit Codex-style freeform patch calls. Also extends the OpenAI twin so black-box tests can exercise the Chat Completions path with raw tool-call arguments. ## Test Plan - `cargo +nightly-2026-04-14 fmt --check --all` - `cargo nextest run -p fabro-agent --test it openai_compatible_twin_preserves_raw_apply_patch_arguments --run-ignored only` - `cargo nextest run -p fabro-llm` - `cargo nextest run -p fabro-test` --- .../fabro-agent/tests/it/parity_matrix.rs | 93 ++++++++++++++++++- .../src/providers/openai_compatible.rs | 50 +++++++++- lib/crates/fabro-test/src/lib.rs | 46 ++++++++- test/twin/openai/src/engine/plan.rs | 18 +++- test/twin/openai/src/engine/scenario.rs | 15 +-- test/twin/openai/src/sse.rs | 11 ++- 6 files changed, 206 insertions(+), 27 deletions(-) diff --git a/lib/crates/fabro-agent/tests/it/parity_matrix.rs b/lib/crates/fabro-agent/tests/it/parity_matrix.rs index ff86a26ce..3c720ec73 100644 --- a/lib/crates/fabro-agent/tests/it/parity_matrix.rs +++ b/lib/crates/fabro-agent/tests/it/parity_matrix.rs @@ -10,13 +10,13 @@ use std::sync::Arc; use fabro_agent::subagent::SessionFactory; use fabro_agent::{ - AgentProfile, AnthropicProfile, GeminiProfile, LocalSandbox, OpenAiProfile, Session, - SessionOptions, SubAgentManager, WebFetchSummarizer, + AgentEvent, AgentProfile, AnthropicProfile, GeminiProfile, LocalSandbox, OpenAiProfile, + Session, SessionOptions, SubAgentManager, WebFetchSummarizer, }; use fabro_auth::EnvCredentialSource; use fabro_llm::client::Client; use fabro_llm::provider::ProviderAdapter; -use fabro_llm::providers::OpenAiAdapter; +use fabro_llm::providers::{OpenAiAdapter, OpenAiCompatibleAdapter}; use fabro_model::{Catalog, ModelHandle, ProviderId}; use fabro_test::{TwinScenario, TwinScenarios, TwinToolCall, twin_openai}; use tokio::sync::Mutex as AsyncMutex; @@ -157,6 +157,31 @@ fn make_twin_client(twin: &OpenAiTwinOptions) -> Client { Client::new(providers, Some("openai".to_string()), Vec::new()) } +fn make_openai_compatible_twin_client(provider: &Provider, twin: &OpenAiTwinOptions) -> Client { + let provider_name = provider.to_string(); + let adapter: Arc = Arc::new( + OpenAiCompatibleAdapter::new(twin.api_key.clone(), twin.base_url.clone()) + .with_name(provider_name.clone()), + ); + let mut providers: HashMap> = HashMap::new(); + providers.insert(provider_name.clone(), adapter); + Client::new(providers, Some(provider_name), Vec::new()) +} + +fn make_openai_compatible_twin_session( + provider: Provider, + model: &str, + cwd: &Path, + config: SessionOptions, + 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 env = Arc::new(LocalSandbox::new(cwd.to_path_buf())); + Session::new(client, profile, env, config, None) +} + macro_rules! provider_test { ($scenario:ident, $provider:expr, $model:expr, $prefix:ident, keys = [$($key:expr),+ $(,)?]) => { paste::paste! { @@ -245,6 +270,68 @@ macro_rules! provider_tests { }; } +#[fabro_macros::e2e_test(twin)] +async fn openai_compatible_twin_preserves_raw_apply_patch_arguments() { + 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)), + ) + .load(twin_openai().await) + .await; + + let config = SessionOptions { + max_turns: 2, + ..SessionOptions::default() + }; + let mut session = make_openai_compatible_twin_session( + ProviderId::new("litellm"), + "gpt-5.4-mini", + tmp.path(), + config, + &twin, + ); + session.initialize().await.unwrap(); + let mut rx = session.subscribe(); + + session + .process_input("Replace old with new in data.txt using apply_patch") + .await + .expect("process_input failed"); + + let mut tool_results = Vec::new(); + while let Ok(event) = rx.try_recv() { + if let AgentEvent::ToolCallCompleted { + tool_name, + output, + is_error, + .. + } = event.event + { + tool_results.push(format!("{tool_name}: is_error={is_error} output={output}")); + } + } + + let content = std::fs::read_to_string(file_path).expect("failed to read data.txt"); + assert_eq!(content, "new\n", "tool results: {tool_results:#?}"); +} + provider_tests!(simple_file_creation); openai_twin_provider_test!(simple_file_creation); provider_tests!(read_and_edit_file); diff --git a/lib/crates/fabro-llm/src/providers/openai_compatible.rs b/lib/crates/fabro-llm/src/providers/openai_compatible.rs index b08977b41..3b784dc04 100644 --- a/lib/crates/fabro-llm/src/providers/openai_compatible.rs +++ b/lib/crates/fabro-llm/src/providers/openai_compatible.rs @@ -392,6 +392,31 @@ fn translate_tool_choice(choice: &ToolChoice) -> serde_json::Value { } } +fn custom_tool_names(request: &Request) -> Vec { + request + .tools + .as_deref() + .unwrap_or_default() + .iter() + .filter(|tool| tool.is_custom()) + .map(|tool| tool.name.clone()) + .collect() +} + +fn parse_tool_arguments( + tool_name: &str, + raw_arguments: &str, + custom_tool_names: &[String], +) -> serde_json::Value { + match serde_json::from_str(raw_arguments) { + Ok(arguments) => arguments, + Err(_) if custom_tool_names.iter().any(|name| name == tool_name) => { + serde_json::Value::String(raw_arguments.to_string()) + } + Err(_) => serde_json::json!({}), + } +} + /// Translate unified `ResponseFormat` to Chat Completions `response_format`. fn translate_response_format(format: &ResponseFormat) -> serde_json::Value { match format.kind { @@ -541,9 +566,13 @@ impl ProviderAdapter for Adapter { } } if let Some(tool_calls) = &choice.message.tool_calls { + let custom_tool_names = custom_tool_names(request); for tc in tool_calls { - let arguments = serde_json::from_str(&tc.function.arguments) - .unwrap_or_else(|_| serde_json::json!({})); + let arguments = parse_tool_arguments( + &tc.function.name, + &tc.function.arguments, + &custom_tool_names, + ); let mut tool_call = ToolCall::new(&tc.id, &tc.function.name, arguments); tool_call.raw_arguments = Some(tc.function.arguments.clone()); content_parts.push(ContentPart::ToolCall(tool_call)); @@ -618,6 +647,7 @@ impl ProviderAdapter for Adapter { let model = request.model.clone(); let rate_limit = parse_rate_limit_headers(http_resp.headers()); let stream_read_timeout = self.http.stream_read_timeout; + let custom_tool_names = custom_tool_names(request); let stream = stream::unfold( StreamState::new( @@ -626,6 +656,7 @@ impl ProviderAdapter for Adapter { model, rate_limit, stream_read_timeout, + custom_tool_names, ), |mut state| async move { loop { @@ -731,6 +762,7 @@ struct StreamState { finish_reason: FinishReason, text_started: bool, done: bool, + custom_tool_names: Vec, /// True after `finish_events()` has been called (guards against /// duplicates). finished: bool, @@ -744,6 +776,7 @@ impl StreamState { model: String, rate_limit: Option, stream_read_timeout: Option, + custom_tool_names: Vec, ) -> Self { Self { line_reader: super::common::LineReader::new(response, stream_read_timeout), @@ -758,6 +791,7 @@ impl StreamState { finish_reason: FinishReason::Stop, text_started: false, done: false, + custom_tool_names, finished: false, rate_limit, } @@ -910,8 +944,11 @@ impl StreamState { } for accumulated in &self.tool_calls { - let arguments = serde_json::from_str(&accumulated.arguments) - .unwrap_or_else(|_| serde_json::json!({})); + let arguments = parse_tool_arguments( + &accumulated.name, + &accumulated.arguments, + &self.custom_tool_names, + ); let mut tool_call = ToolCall::new(&accumulated.id, &accumulated.name, arguments); tool_call.raw_arguments = Some(accumulated.arguments.clone()); @@ -1029,6 +1066,7 @@ mod tests { "model".into(), None, Some(std::time::Duration::from_secs(30)), + Vec::new(), ); // First text chunk should emit TextStart + TextDelta. @@ -1061,6 +1099,7 @@ mod tests { "model".into(), None, Some(std::time::Duration::from_secs(30)), + Vec::new(), ); // First tool call chunk (has id and name) -> ToolCallStart. @@ -1092,6 +1131,7 @@ mod tests { "test-model".into(), None, Some(std::time::Duration::from_secs(30)), + Vec::new(), ); state.response_id = "resp-1".into(); state.response_model = "gpt-4".into(); @@ -1135,6 +1175,7 @@ mod tests { "model".into(), None, Some(std::time::Duration::from_secs(30)), + Vec::new(), ); state.response_id = "resp-1".into(); state.tool_calls.push(AccumulatedToolCall { @@ -1180,6 +1221,7 @@ mod tests { "fallback-model".into(), None, Some(std::time::Duration::from_secs(30)), + Vec::new(), ); // response_model is empty, so finish_events should use the request model. let events = state.finish_events(); diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 77512bfe6..8db94500c 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -2155,6 +2155,20 @@ impl TwinScenario { } } + #[must_use] + pub fn chat_completions(model: impl Into) -> Self { + Self { + matcher: Map::from_iter([ + ( + "endpoint".to_string(), + Value::String("chat.completions".to_string()), + ), + ("model".to_string(), Value::String(model.into())), + ]), + script: json!({ "kind": "success" }), + } + } + #[must_use] pub fn text(mut self, text: impl Into) -> Self { self.assert_script_kind("success", "text"); @@ -2255,8 +2269,9 @@ impl TwinScenario { #[derive(Debug, Clone)] pub struct TwinToolCall { - name: String, - arguments: Value, + name: String, + arguments: Value, + raw_arguments: Option, } impl TwinToolCall { @@ -2265,6 +2280,20 @@ impl TwinToolCall { Self { name: name.into(), arguments, + raw_arguments: None, + } + } + + #[must_use] + pub fn new_raw_arguments( + name: impl Into, + arguments: Value, + raw_arguments: impl Into, + ) -> Self { + Self { + name: name.into(), + arguments, + raw_arguments: Some(raw_arguments.into()), } } @@ -2315,11 +2344,20 @@ impl TwinToolCall { Self::new("apply_patch", json!({ "patch": patch.into() })) } + #[must_use] + pub fn apply_patch_raw_arguments(patch: impl Into) -> Self { + Self::new_raw_arguments("apply_patch", Value::Null, patch.into()) + } + fn into_json(self) -> Value { - json!({ + let mut value = json!({ "name": self.name, "arguments": self.arguments, - }) + }); + if let Some(raw_arguments) = self.raw_arguments { + value["raw_arguments"] = Value::String(raw_arguments); + } + value } } diff --git a/test/twin/openai/src/engine/plan.rs b/test/twin/openai/src/engine/plan.rs index 6440d28e1..f750bb39b 100644 --- a/test/twin/openai/src/engine/plan.rs +++ b/test/twin/openai/src/engine/plan.rs @@ -60,19 +60,27 @@ pub struct ResponsePlan { #[derive(Clone, Debug)] pub struct ToolCallPlan { - pub id: String, - pub name: String, - pub arguments: Value, + pub id: String, + pub name: String, + pub arguments: Value, + pub raw_arguments: Option, } impl ResponsePlan { + pub fn tool_call_arguments_text(tool_call: &ToolCallPlan) -> String { + tool_call + .raw_arguments + .clone() + .unwrap_or_else(|| tool_call.arguments.to_string()) + } + fn responses_tool_call_item(tool_call: &ToolCallPlan) -> Value { json!({ "id": format!("fc_{}", tool_call.id), "type": "function_call", "call_id": tool_call.id, "name": tool_call.name, - "arguments": tool_call.arguments.to_string(), + "arguments": Self::tool_call_arguments_text(tool_call), }) } @@ -144,7 +152,7 @@ impl ResponsePlan { "type": "function", "function": { "name": tool_call.name, - "arguments": tool_call.arguments.to_string(), + "arguments": Self::tool_call_arguments_text(tool_call), } })).collect::>(), } diff --git a/test/twin/openai/src/engine/scenario.rs b/test/twin/openai/src/engine/scenario.rs index e88715808..b30be7eb0 100644 --- a/test/twin/openai/src/engine/scenario.rs +++ b/test/twin/openai/src/engine/scenario.rs @@ -56,9 +56,11 @@ pub enum ScenarioScript { #[derive(Clone, Debug, Deserialize, Serialize)] pub struct ToolCallTemplate { - pub id: Option, - pub name: String, - pub arguments: Value, + pub id: Option, + pub name: String, + pub arguments: Value, + #[serde(skip_serializing_if = "Option::is_none")] + pub raw_arguments: Option, } #[derive(Clone, Debug)] @@ -256,11 +258,12 @@ fn build_plan_from_script( .into_iter() .enumerate() .map(|(index, tool_call)| ToolCallPlan { - id: tool_call + id: tool_call .id .unwrap_or_else(|| format!("call_{response_number}_{index}")), - name: tool_call.name, - arguments: tool_call.arguments, + name: tool_call.name, + arguments: tool_call.arguments, + raw_arguments: tool_call.raw_arguments, }) .collect(), usage: usage.unwrap_or_default(), diff --git a/test/twin/openai/src/sse.rs b/test/twin/openai/src/sse.rs index 728fde5ac..ddfc43d93 100644 --- a/test/twin/openai/src/sse.rs +++ b/test/twin/openai/src/sse.rs @@ -186,7 +186,7 @@ pub fn responses_sse_response(plan: &ResponsePlan, transport: TransportOptions) &json!({ "type": "response.function_call_arguments.delta", "item_id": item_id, - "delta": tool_call.arguments.to_string(), + "delta": ResponsePlan::tool_call_arguments_text(tool_call), "output_index": next_output_index, }), )); @@ -195,7 +195,7 @@ pub fn responses_sse_response(plan: &ResponsePlan, transport: TransportOptions) &json!({ "type": "response.function_call_arguments.done", "item_id": item_id, - "arguments": tool_call.arguments.to_string(), + "arguments": ResponsePlan::tool_call_arguments_text(tool_call), "output_index": next_output_index, }), )); @@ -208,7 +208,7 @@ pub fn responses_sse_response(plan: &ResponsePlan, transport: TransportOptions) "type": "function_call", "call_id": tool_call.id, "name": tool_call.name, - "arguments": tool_call.arguments.to_string(), + "arguments": ResponsePlan::tool_call_arguments_text(tool_call), }, "output_index": next_output_index, }), @@ -287,12 +287,13 @@ pub fn chat_sse_response(plan: &ResponsePlan, transport: TransportOptions) -> Re "choices": [{ "index": 0, "delta": { - "tool_calls": plan.tool_calls.iter().map(|tool_call| json!({ + "tool_calls": plan.tool_calls.iter().enumerate().map(|(index, tool_call)| json!({ + "index": index, "id": tool_call.id, "type": "function", "function": { "name": tool_call.name, - "arguments": tool_call.arguments.to_string(), + "arguments": ResponsePlan::tool_call_arguments_text(tool_call), } })).collect::>() },