diff --git a/src/backend.rs b/src/backend.rs index 7cff656..b9a2a13 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -159,12 +159,15 @@ pub trait InferenceBackend: Send + Sync { sink: Option<&TokenSink>, ) -> Result; - /// Continue the turn by returning tool results. `tools` may be `None` on the - /// final round to force a text answer. `sink` streams that text when set. + /// Continue the turn by returning tool results. `allow_tool_calls` controls + /// whether `tools` are offered again; the complete catalog remains available + /// so a disabled round can recognize and suppress tool-shaped model output. + /// `sink` streams assistant text when set. async fn send_tool_results( &self, results: Vec, - tools: Option<&[ToolSpec]>, + tools: &[ToolSpec], + allow_tool_calls: bool, sink: Option<&TokenSink>, ) -> Result; @@ -257,7 +260,8 @@ impl InferenceBackend for LocalBackend { async fn send_tool_results( &self, results: Vec, - tools: Option<&[ToolSpec]>, + tools: &[ToolSpec], + allow_tool_calls: bool, sink: Option<&TokenSink>, ) -> Result { let onde_results: Vec = results @@ -268,10 +272,10 @@ impl InferenceBackend for LocalBackend { }) .collect(); - // The final round passes `tools = None` to force a text answer; that's - // the only round onde can stream, since no further tool calls are parsed. + // A forced-text round is the only round onde can stream, since no + // further tool calls are parsed. if let Some(sink) = sink - && tools.is_none() + && !allow_tool_calls { let rx = self .engine @@ -281,7 +285,7 @@ impl InferenceBackend for LocalBackend { return drain_onde_stream(rx, sink).await; } - let onde_tools = tools.map(to_onde_tools); + let onde_tools = allow_tool_calls.then(|| to_onde_tools(tools)); let result = self .engine .send_tool_results(onde_results, onde_tools.as_deref()) @@ -485,12 +489,14 @@ impl OpenAiBackend { .collect() } - /// POST the current history (plus `tools`) and apply the assistant reply to - /// history, returning the neutral turn result. Streams via SSE when `sink` - /// is set; otherwise reads a single JSON response. + /// POST the current history and apply the assistant reply to history. + /// `tools` is always the known catalog, while `allow_tool_calls` determines + /// whether it is advertised to the model and whether returned calls may run. + /// Streams via SSE when `sink` is set; otherwise reads a single JSON response. async fn complete( &self, - tools: Option<&[ToolSpec]>, + tools: &[ToolSpec], + allow_tool_calls: bool, sink: Option<&TokenSink>, ) -> Result { let url = format!("{}/chat/completions", self.base_url.trim_end_matches('/')); @@ -501,9 +507,7 @@ impl OpenAiBackend { "messages": *self.history.lock().await, "stream": streaming, }); - if let Some(tools) = tools - && !tools.is_empty() - { + if allow_tool_calls && !tools.is_empty() { body["tools"] = serde_json::Value::Array(Self::tools_json(tools)); // OpenAI specifies `auto` as the default when tools are present, // but not every OpenAI-compatible gateway implements that default. @@ -528,13 +532,11 @@ impl OpenAiBackend { return Err(describe_api_error(status, &body)); } - // The specs are needed downstream to type the arguments of any tool - // call the model emitted as text rather than as a structured call. - let tools = tools.unwrap_or(&[]); if let Some(sink) = sink { - self.consume_stream(response, sink, tools).await + self.consume_stream(response, sink, tools, allow_tool_calls) + .await } else { - self.consume_json(response, tools).await + self.consume_json(response, tools, allow_tool_calls).await } } @@ -543,6 +545,7 @@ impl OpenAiBackend { &self, response: reqwest::Response, tools: &[ToolSpec], + allow_tool_calls: bool, ) -> Result { let parsed: ChatCompletion = response .json() @@ -556,7 +559,7 @@ impl OpenAiBackend { .map(|choice| choice.message) .ok_or_else(|| "endpoint returned no choices".to_string())?; - let text = message.content.clone().unwrap_or_default(); + let mut text = message.content.clone().unwrap_or_default(); let tool_calls: Vec = message .tool_calls .iter() @@ -568,6 +571,31 @@ impl OpenAiBackend { }) .collect(); + if !allow_tool_calls { + let (cleaned, recovered) = crate::inline_tool_calls::extract(&text, tools); + text = cleaned; + let suppressed = tool_calls.len() + recovered.len(); + if suppressed > 0 { + let names = tool_calls + .iter() + .map(|call| call.name.as_str()) + .chain(recovered.iter().map(|call| call.name.as_str())) + .collect::>() + .join(", "); + log::warn!( + "suppressed {suppressed} tool call(s) from a forced-text response: {names}" + ); + } + self.history + .lock() + .await + .push(streamed_assistant_history(&text, &[])); + return Ok(TurnResult { + text, + tool_calls: Vec::new(), + }); + } + // Some models write a tool call out as literal `` text // instead of using the structured field (see `inline_tool_calls`). // Recover it, or the turn ends with the tag rendered as prose and @@ -617,6 +645,7 @@ impl OpenAiBackend { response: reqwest::Response, sink: &TokenSink, tools: &[ToolSpec], + allow_tool_calls: bool, ) -> Result { use futures::StreamExt; @@ -691,10 +720,12 @@ impl OpenAiBackend { } } crate::inline_tool_calls::ScanEvent::ToolCall(call) => { - log::warn!( - "recovered tool call '{}' the model emitted as text instead of a structured call", - call.name - ); + if allow_tool_calls { + log::warn!( + "recovered tool call '{}' the model emitted as text instead of a structured call", + call.name + ); + } recovered.push(ToolCall { id: format!("call_recovered_{}", recovered.len()), name: call.name, @@ -755,6 +786,19 @@ impl OpenAiBackend { .collect(); tool_calls.extend(recovered); + if !allow_tool_calls && !tool_calls.is_empty() { + let names = tool_calls + .iter() + .map(|call| call.name.as_str()) + .collect::>() + .join(", "); + log::warn!( + "suppressed {} tool call(s) from a forced-text response: {names}", + tool_calls.len() + ); + tool_calls.clear(); + } + // Record the assistant turn so later tool results have context. self.history .lock() @@ -813,13 +857,14 @@ impl InferenceBackend for OpenAiBackend { .lock() .await .push(serde_json::json!({ "role": "user", "content": text })); - self.complete(Some(tools), sink).await + self.complete(tools, true, sink).await } async fn send_tool_results( &self, results: Vec, - tools: Option<&[ToolSpec]>, + tools: &[ToolSpec], + allow_tool_calls: bool, sink: Option<&TokenSink>, ) -> Result { { @@ -832,7 +877,7 @@ impl InferenceBackend for OpenAiBackend { })); } } - self.complete(tools, sink).await + self.complete(tools, allow_tool_calls, sink).await } async fn record_cancelled_tool_results(&self, results: Vec) { @@ -887,7 +932,7 @@ impl InferenceBackend for OpenAiBackend { })); *self.history.lock().await = request; - let summary = match self.complete(None, None).await { + let summary = match self.complete(&[], false, None).await { Ok(result) => result.text, Err(error) => { // Roll back the summarization request; the turn never happened. diff --git a/src/chat.rs b/src/chat.rs index ce00294..ce8c1dd 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -3346,12 +3346,8 @@ mod tui { // on the last round, pass no tools so the model must produce text — // that's also the round we can stream on-device. - let next_tools = if round < MAX_TOOL_ROUNDS { - Some(tools.as_slice()) - } else { - None - }; - let sink = if next_tools.is_none() { + let allow_tool_calls = round < MAX_TOOL_ROUNDS; + let sink = if !allow_tool_calls { streamed = true; Some(&delta_tx) } else { @@ -3359,7 +3355,7 @@ mod tui { }; match backend - .send_tool_results(tool_results, next_tools, sink) + .send_tool_results(tool_results, &tools, allow_tool_calls, sink) .await { Ok(r) => result = r, diff --git a/src/headless.rs b/src/headless.rs index dc0c128..b5addbe 100644 --- a/src/headless.rs +++ b/src/headless.rs @@ -304,18 +304,14 @@ async fn run_prompt( }); } - let next_tools = if round < crate::MAX_TOOL_ROUNDS { - Some(tools.as_slice()) - } else { - None // last round: force text - }; + let allow_tool_calls = round < crate::MAX_TOOL_ROUNDS; // Whatever this round says starts a new paragraph rather than // continuing the sentence the tool calls interrupted. reply.interrupt(); result = drain_to_stdout( - backend.send_tool_results(tool_results, next_tools, sink_opt), + backend.send_tool_results(tool_results, &tools, allow_tool_calls, sink_opt), &mut sink_rx, &mut reply, ) diff --git a/src/inline_tool_calls.rs b/src/inline_tool_calls.rs index e61b125..4b8a2e2 100644 --- a/src/inline_tool_calls.rs +++ b/src/inline_tool_calls.rs @@ -83,16 +83,22 @@ pub fn extract(text: &str, tools: &[ToolSpec]) -> (String, Vec) { fn parse_xml_block(inner: &str, tools: &[ToolSpec]) -> Option { let key_idx = inner.find(""); let malformed_key_idx = inner.find(XML_OPEN_TAG); - let (name, mut rest, malformed_first_key) = match (key_idx, malformed_key_idx) { - (Some(key_idx), Some(malformed_key_idx)) if malformed_key_idx < key_idx => ( - inner[..malformed_key_idx].trim(), - &inner[malformed_key_idx..], - true, - ), - (Some(idx), _) => (inner[..idx].trim(), &inner[idx..], false), - (None, Some(idx)) => (inner[..idx].trim(), &inner[idx..], true), - (None, None) => (inner.trim(), "", false), - }; + let check_status_name = key_idx.and_then(|idx| parse_check_status_preamble(&inner[..idx])); + let (name, mut rest, malformed_first_key) = + if let (Some(idx), Some(name)) = (key_idx, check_status_name) { + (name, &inner[idx..], false) + } else { + match (key_idx, malformed_key_idx) { + (Some(key_idx), Some(malformed_key_idx)) if malformed_key_idx < key_idx => ( + inner[..malformed_key_idx].trim(), + &inner[malformed_key_idx..], + true, + ), + (Some(idx), _) => (inner[..idx].trim(), &inner[idx..], false), + (None, Some(idx)) => (inner[..idx].trim(), &inner[idx..], true), + (None, None) => (inner.trim(), "", false), + } + }; if name.is_empty() || name.contains(['<', '>']) || !offered_tool(tools, name) { return None; } @@ -132,6 +138,24 @@ fn parse_xml_block(inner: &str, tools: &[ToolSpec]) -> Option { }) } +/// Some GLM-family responses prefix the real arguments with generation-control +/// metadata shaped like `tool_name CheckStatus=value`. It is not a +/// tool argument: accept that exact bounded preamble and discard it, while +/// leaving every other malformed prefix untouched. +fn parse_check_status_preamble(prefix: &str) -> Option<&str> { + let (name, status) = prefix.trim().split_once(" CheckStatus=")?; + let status = status.strip_suffix("")?; + if name.is_empty() + || status.is_empty() + || !status + .chars() + .all(|character| character.is_ascii_alphanumeric() || matches!(character, '_' | '-')) + { + return None; + } + Some(name) +} + fn parse_k3_tools_block(inner: &str, tools: &[ToolSpec]) -> Option> { let mut calls = Vec::new(); let mut rest = inner; @@ -565,6 +589,47 @@ mod tests { assert_eq!(args["cwd"], "/tmp"); } + #[test] + fn recovers_glm_call_with_check_status_before_real_arguments() { + let tools = vec![command_output_spec()]; + let (text, calls) = extract( + "polling command_output CheckStatus=true_or_poll_again_with_different_paramstask_id1", + &tools, + ); + + assert_eq!(text, "polling "); + assert_eq!(calls.len(), 1); + assert_eq!(calls[0].name, "command_output"); + let args: serde_json::Value = serde_json::from_str(&calls[0].arguments).unwrap(); + assert_eq!(args["task_id"], 1); + assert!(args["task_id"].is_number()); + assert!(args.get("CheckStatus").is_none()); + } + + #[test] + fn malformed_check_status_preamble_is_left_as_text() { + let text = "command_output CheckStatus=keep goingtask_id1"; + let (out, calls) = extract(text, &[command_output_spec()]); + assert_eq!(out, text); + assert!(calls.is_empty()); + } + + #[test] + fn check_status_after_a_real_argument_is_left_as_text() { + let text = "command_outputtask_id1 CheckStatus=true_or_poll_again_with_different_params"; + let (out, calls) = extract(text, &[command_output_spec()]); + assert_eq!(out, text); + assert!(calls.is_empty()); + } + + #[test] + fn check_status_for_an_unknown_tool_is_left_as_text() { + let text = "not_offered CheckStatus=true_or_poll_again_with_different_paramstask_id1"; + let (out, calls) = extract(text, &[command_output_spec()]); + assert_eq!(out, text); + assert!(calls.is_empty()); + } + #[test] fn unknown_glm_tool_is_left_as_text() { let text = "not_offeredcommandpwd"; @@ -773,6 +838,35 @@ mod tests { assert_eq!(args["command"], "pwd"); } + #[test] + fn scanner_recovers_check_status_call_split_across_chunks() { + let tools = vec![command_output_spec()]; + let mut scanner = StreamScanner::new(&tools); + let mut text = String::new(); + let mut calls = Vec::new(); + + for chunk in [ + "before command_output CheckStatus=true_or_poll_", + "again_with_different_paramstask_id1 after", + ] { + for event in scanner.push(chunk) { + match event { + ScanEvent::Text(value) => text.push_str(&value), + ScanEvent::ToolCall(call) => calls.push(call), + } + } + } + if let Some(rest) = scanner.take_pending() { + text.push_str(&rest); + } + + assert_eq!(text, "before after"); + assert_eq!(calls.len(), 1); + assert_eq!(calls[0].name, "command_output"); + } + #[test] fn scanner_recovers_kimi_k3_tools_split_across_chunk_boundaries() { let tools = vec![run_command_spec()]; diff --git a/src/main.rs b/src/main.rs index 4df5f15..a891b1a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2044,11 +2044,7 @@ impl SiGitAgent { }); } - let next_tools = if round < MAX_TOOL_ROUNDS && !force_text { - Some(tools.as_slice()) - } else { - None // last round: force text - }; + let allow_tool_calls = round < MAX_TOOL_ROUNDS && !force_text; // Whatever this round says starts a new paragraph rather than // continuing the sentence the tool calls interrupted. @@ -2058,7 +2054,7 @@ impl SiGitAgent { .drain_turn( cx, &session_id, - backend.send_tool_results(tool_results, next_tools, Some(&sink)), + backend.send_tool_results(tool_results, &tools, allow_tool_calls, Some(&sink)), &mut sink_rx, &mut reply, ) @@ -4197,7 +4193,19 @@ mod tests { #[test] fn history_replay_redraws_the_conversation_for_the_client() { - let changelog_path = std::env::current_dir().unwrap().join("CHANGELOG.md"); + // A fixed path, not `std::env::current_dir()`: the cwd is process-global + // and a handful of other tests (skills/commands/subagents discovery, + // the subagent end-to-end test) briefly `set_current_dir` to a temp + // directory behind `ENV_TEST_LOCK`. This test asserts on the exact + // rendered title, so racing one of them mid-run doesn't just read a + // stale value — it can hand back a temp path long enough to trip the + // title's truncation, breaking an assertion that has nothing to do + // with the cwd. A literal path sidesteps the shared state entirely. + let changelog_path = if cfg!(windows) { + PathBuf::from(r"C:\repo\CHANGELOG.md") + } else { + PathBuf::from("/repo/CHANGELOG.md") + }; let changelog_path_text = changelog_path.to_string_lossy().into_owned(); let arguments = serde_json::json!({ "path": changelog_path_text }).to_string(); let history = vec![ diff --git a/src/tools.rs b/src/tools.rs index ab6bda8..428779c 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -939,13 +939,9 @@ async fn run_subagent(backend: &dyn InferenceBackend, prompt: &str, allowed: &[S } // On the last round, offer no tools so the model must produce text. - let next_tools = if round < SUBAGENT_MAX_TOOL_ROUNDS { - Some(specs.as_slice()) - } else { - None - }; + let allow_tool_calls = round < SUBAGENT_MAX_TOOL_ROUNDS; result = match backend - .send_tool_results(tool_results, next_tools, None) + .send_tool_results(tool_results, &specs, allow_tool_calls, None) .await { Ok(r) => r, diff --git a/tests/acp_permissions.rs b/tests/acp_permissions.rs index e6aed6c..911324a 100644 --- a/tests/acp_permissions.rs +++ b/tests/acp_permissions.rs @@ -943,6 +943,120 @@ fn a_tool_call_emitted_as_text_is_executed_rather_than_rendered() { let _ = std::fs::remove_dir_all(&scratch); } +/// Once the repetition guard removes tools from a request, a model may still +/// emit its learned tool syntax. Known calls must be removed from the visible +/// response without being executed or recorded as orphaned history entries. +#[test] +fn forced_text_suppresses_glm_check_status_tool_markup() { + let repeated_arguments = json!({"task_id": 1}).to_string(); + let endpoint = start_fake_endpoint(vec![ + sse_tool_call("call_1", "command_output", &repeated_arguments), + sse_tool_call("call_2", "command_output", &repeated_arguments), + sse_tool_call("call_3", "command_output", &repeated_arguments), + sse_body(&[ + json!({"choices": [{"delta": {"content": "Build is still running. command_output CheckStatus=true_or_poll_"}}]}), + json!({"choices": [{"delta": {"content": "again_with_different_paramstask_id1"}}]}), + // Also enforce the forced-text boundary for a structured call. + json!({"choices": [{"delta": {"tool_calls": [{ + "index": 0, + "id": "call_4", + "function": {"name": "command_output", "arguments": "{\"task_id\":999}"}, + }]}}]}), + ]), + sse_text("Next turn."), + ]); + + let scratch = + std::env::temp_dir().join(format!("sigit_acp_forced_text_{}", std::process::id())); + let config_dir = scratch.join("config"); + let cwd = scratch.join("cwd"); + std::fs::create_dir_all(&config_dir).unwrap(); + std::fs::create_dir_all(&cwd).unwrap(); + + let mut agent = spawn_agent(endpoint.port, &config_dir); + + let id = agent.request( + "initialize", + json!({"protocolVersion": 1, "clientCapabilities": {}}), + ); + agent.wait_for_response(id); + + let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []})); + let session_id = agent.wait_for_response(id)["result"]["sessionId"] + .as_str() + .expect("session id") + .to_string(); + + let prompt_id = agent.request( + "session/prompt", + json!({ + "sessionId": session_id, + "prompt": [{"type": "text", "text": "keep polling"}], + }), + ); + let (response, rendered) = agent.wait_for_prompt(prompt_id); + assert_eq!(response["result"]["stopReason"], "end_turn"); + assert!( + rendered.contains("Build is still running."), + "surrounding prose should remain visible: {rendered:?}" + ); + assert!( + !rendered.contains("") + && !rendered.contains("") + && !rendered.contains("CheckStatus"), + "forced tool markup reached the client: {rendered:?}" + ); + + { + let requests = endpoint.requests.lock().unwrap(); + assert_eq!( + requests.len(), + 4, + "a forced-text tool call must not start another inference round" + ); + assert!( + requests[3].get("tools").is_none(), + "the repetition guard must not advertise tools: {:?}", + requests[3] + ); + } + + // Start another turn so the prior assistant message is replayed and its + // sanitized history shape can be inspected in the recorded request. + let next_prompt_id = agent.request( + "session/prompt", + json!({ + "sessionId": session_id, + "prompt": [{"type": "text", "text": "what happened?"}], + }), + ); + agent.wait_for_prompt(next_prompt_id); + + let requests = endpoint.requests.lock().unwrap(); + let replayed_messages = requests[4]["messages"].as_array().expect("messages"); + let prior_assistant = replayed_messages + .iter() + .rev() + .find(|message| { + message["role"] == "assistant" + && message["content"] + .as_str() + .is_some_and(|content| content.contains("Build is still running.")) + }) + .expect("sanitized forced-text assistant response"); + assert!(prior_assistant.get("tool_calls").is_none()); + assert!( + !prior_assistant["content"] + .as_str() + .expect("assistant content") + .contains("") + ); + drop(requests); + + drop(agent); + let _ = std::fs::remove_dir_all(&scratch); +} + /// Kimi K3 may emit tool calls in XTML content blocks even when the endpoint is /// OpenAI-compatible. The backend should recover those before ACP sees them, /// unwrap visible response text, and hide private thinking text.