From b6fc7326e7c3e998195006bf885ce39990320e3c Mon Sep 17 00:00:00 2001 From: kevin9327 Date: Mon, 31 Aug 2026 19:26:38 +0900 Subject: [PATCH] fix(provider): keep OpenAI Chat tool calls that finish with reason "stop" `map_finish` matched `"stop" | "content_filter"` before the `has_tools` fallback, so the fallback only ever applied to finish reasons the adapter did not recognise. When an OpenAI-compatible server streams tool calls and then reports `finish_reason: "stop"` the adapter yielded `Done(Stop)` even though `ToolCallStart` / `ToolCallEnd` had already been emitted. `run/model_cycle.rs` rejects that combination: let has_tool_calls = !calls.is_empty(); if matches!(finish_reason, FinishReason::ToolUse) != has_tool_calls { return Err(failure(RunFailure::Protocol( "finish reason and tool calls disagree".into()), ...)); } so the whole turn fails with a protocol error and the tool never runs. Servers that report `"stop"` alongside `tool_calls` are common in BYOK setups (llama.cpp, Ollama's OpenAI shim, several proxies), which makes those models unusable for anything agentic. Move the `has_tools` arm ahead of `"stop" | "content_filter"` so an observed tool call outranks the label the provider attached to the stop. This matches the two sibling adapters (`anthropic.rs` checks `_ if saw_tool` before every reason except `Length`, `openai_responses.rs` derives the reason from `saw_tool` alone) and this adapter's own `[DONE]` fallback, which already infers `ToolUse` from `!tools.is_empty()`. `"length"` still wins so a truncated response is still reported as truncated, and behaviour with no tool calls is byte-for-byte unchanged. Before (with the arm restored): cargo test -p cursor-server --lib provider::openai_chat -> observed_tool_calls_outrank_a_stop_finish_reason FAILED assertion `left == right` failed: left: Stop, right: ToolUse After: cargo test -p cursor-server --lib provider::openai_chat -> 3 passed --- server/src/provider/openai_chat.rs | 31 +++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/server/src/provider/openai_chat.rs b/server/src/provider/openai_chat.rs index 32835a4..f2d962d 100644 --- a/server/src/provider/openai_chat.rs +++ b/server/src/provider/openai_chat.rs @@ -368,7 +368,9 @@ fn map_finish(value: &str, has_tools: bool) -> FinishReason { match value { "tool_calls" | "function_call" => FinishReason::ToolUse, "length" => FinishReason::Length, - "stop" | "content_filter" => FinishReason::Stop, + // Observed tool calls outrank whatever the provider labelled the stop: + // OpenAI-compatible servers routinely report "stop" while streaming tool + // calls, and the run rejects a finish reason that contradicts them. _ if has_tools => FinishReason::ToolUse, _ => FinishReason::Stop, } @@ -434,3 +436,30 @@ pub(crate) fn openai_usage(value: &Value) -> Usage { .and_then(Value::as_u64), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn observed_tool_calls_outrank_a_stop_finish_reason() { + assert_eq!(map_finish("stop", true), FinishReason::ToolUse); + assert_eq!(map_finish("content_filter", true), FinishReason::ToolUse); + assert_eq!(map_finish("", true), FinishReason::ToolUse); + } + + #[test] + fn finish_reason_mapping_without_tool_calls_is_unchanged() { + assert_eq!(map_finish("stop", false), FinishReason::Stop); + assert_eq!(map_finish("content_filter", false), FinishReason::Stop); + assert_eq!(map_finish("", false), FinishReason::Stop); + assert_eq!(map_finish("tool_calls", false), FinishReason::ToolUse); + assert_eq!(map_finish("function_call", false), FinishReason::ToolUse); + } + + #[test] + fn a_truncated_response_stays_truncated_even_with_tool_calls() { + assert_eq!(map_finish("length", true), FinishReason::Length); + assert_eq!(map_finish("length", false), FinishReason::Length); + } +}