mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-11 22:53:00 +00:00
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`
This commit is contained in:
Bryan Helmkamp
GitHub
parent
2e39dfc70e
commit
91d11eb04d
6 changed files with 206 additions and 27 deletions
|
|
@ -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<dyn ProviderAdapter> = Arc::new(
|
||||
OpenAiCompatibleAdapter::new(twin.api_key.clone(), twin.base_url.clone())
|
||||
.with_name(provider_name.clone()),
|
||||
);
|
||||
let mut providers: HashMap<String, Arc<dyn ProviderAdapter>> = 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<dyn AgentProfile> =
|
||||
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);
|
||||
|
|
|
|||
|
|
@ -392,6 +392,31 @@ fn translate_tool_choice(choice: &ToolChoice) -> serde_json::Value {
|
|||
}
|
||||
}
|
||||
|
||||
fn custom_tool_names(request: &Request) -> Vec<String> {
|
||||
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<String>,
|
||||
/// True after `finish_events()` has been called (guards against
|
||||
/// duplicates).
|
||||
finished: bool,
|
||||
|
|
@ -744,6 +776,7 @@ impl StreamState {
|
|||
model: String,
|
||||
rate_limit: Option<RateLimitInfo>,
|
||||
stream_read_timeout: Option<std::time::Duration>,
|
||||
custom_tool_names: Vec<String>,
|
||||
) -> 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();
|
||||
|
|
|
|||
|
|
@ -2155,6 +2155,20 @@ impl TwinScenario {
|
|||
}
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn chat_completions(model: impl Into<String>) -> 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<String>) -> 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<String>,
|
||||
}
|
||||
|
||||
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<String>,
|
||||
arguments: Value,
|
||||
raw_arguments: impl Into<String>,
|
||||
) -> 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<String>) -> 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
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<String>,
|
||||
}
|
||||
|
||||
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::<Vec<_>>(),
|
||||
}
|
||||
|
|
|
|||
|
|
@ -56,9 +56,11 @@ pub enum ScenarioScript {
|
|||
|
||||
#[derive(Clone, Debug, Deserialize, Serialize)]
|
||||
pub struct ToolCallTemplate {
|
||||
pub id: Option<String>,
|
||||
pub name: String,
|
||||
pub arguments: Value,
|
||||
pub id: Option<String>,
|
||||
pub name: String,
|
||||
pub arguments: Value,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub raw_arguments: Option<String>,
|
||||
}
|
||||
|
||||
#[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(),
|
||||
|
|
|
|||
|
|
@ -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::<Vec<_>>()
|
||||
},
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue