@sigitcodereview / sigit / commits / e1f3787

Fix ACP handling of malformed Chinese-model tool calls

Co-Authored-By: siGit Code <noreply@sigit.si>

Seto Elkahfi committed Sep 14, 2026 at 18:59 UTC e1f378736c73f6f856207abbbe593f53300e1e93

7 files changed +294 -69

src/backend.rs

+67 -34

@@ -159,12 +159,15 @@ pub trait InferenceBackend: Send + Sync {

159 sink: Option<&TokenSink>,

160 ) -> Result<TurnResult, BackendError>;

161

162 - /// Continue the turn by returning tool results. `tools` may be `None` on the

163 - /// final round to force a text answer. `sink` streams that text when set.

162 + /// Continue the turn by returning tool results. `allow_tool_calls` controls

163 + /// whether `tools` are offered again; the complete catalog remains available

164 + /// so a disabled round can recognize and suppress tool-shaped model output.

165 + /// `sink` streams assistant text when set.

166 async fn send_tool_results(

167 &self,

168 results: Vec<ToolResult>,

167 - tools: Option<&[ToolSpec]>,

169 + tools: &[ToolSpec],

170 + allow_tool_calls: bool,

171 sink: Option<&TokenSink>,

172 ) -> Result<TurnResult, BackendError>;

173

@@ -257,7 +260,8 @@ impl InferenceBackend for LocalBackend {

260 async fn send_tool_results(

261 &self,

262 results: Vec<ToolResult>,

260 - tools: Option<&[ToolSpec]>,

263 + tools: &[ToolSpec],

264 + allow_tool_calls: bool,

265 sink: Option<&TokenSink>,

266 ) -> Result<TurnResult, BackendError> {

267 let onde_results: Vec<onde::inference::ToolResult> = results

@@ -268,10 +272,10 @@ impl InferenceBackend for LocalBackend {

272 })

273 .collect();

274

271 - // The final round passes `tools = None` to force a text answer; that's

272 - // the only round onde can stream, since no further tool calls are parsed.

275 + // A forced-text round is the only round onde can stream, since no

276 + // further tool calls are parsed.

277 if let Some(sink) = sink

274 - && tools.is_none()

278 + && !allow_tool_calls

279 {

280 let rx = self

281 .engine

@@ -281,7 +285,7 @@ impl InferenceBackend for LocalBackend {

285 return drain_onde_stream(rx, sink).await;

286 }

287

284 - let onde_tools = tools.map(to_onde_tools);

288 + let onde_tools = allow_tool_calls.then(|| to_onde_tools(tools));

289 let result = self

290 .engine

291 .send_tool_results(onde_results, onde_tools.as_deref())

@@ -485,12 +489,14 @@ impl OpenAiBackend {

489 .collect()

490 }

491

488 - /// POST the current history (plus `tools`) and apply the assistant reply to

489 - /// history, returning the neutral turn result. Streams via SSE when `sink`

490 - /// is set; otherwise reads a single JSON response.

492 + /// POST the current history and apply the assistant reply to history.

493 + /// `tools` is always the known catalog, while `allow_tool_calls` determines

494 + /// whether it is advertised to the model and whether returned calls may run.

495 + /// Streams via SSE when `sink` is set; otherwise reads a single JSON response.

496 async fn complete(

497 &self,

493 - tools: Option<&[ToolSpec]>,

498 + tools: &[ToolSpec],

499 + allow_tool_calls: bool,

500 sink: Option<&TokenSink>,

501 ) -> Result<TurnResult, BackendError> {

502 let url = format!("{}/chat/completions", self.base_url.trim_end_matches('/'));

@@ -501,9 +507,7 @@ impl OpenAiBackend {

507 "messages": *self.history.lock().await,

508 "stream": streaming,

509 });

504 - if let Some(tools) = tools

505 - && !tools.is_empty()

506 - {

510 + if allow_tool_calls && !tools.is_empty() {

511 body["tools"] = serde_json::Value::Array(Self::tools_json(tools));

512 // OpenAI specifies `auto` as the default when tools are present,

513 // but not every OpenAI-compatible gateway implements that default.

@@ -528,13 +532,11 @@ impl OpenAiBackend {

532 return Err(describe_api_error(status, &body));

533 }

534

531 - // The specs are needed downstream to type the arguments of any tool

532 - // call the model emitted as text rather than as a structured call.

533 - let tools = tools.unwrap_or(&[]);

535 if let Some(sink) = sink {

535 - self.consume_stream(response, sink, tools).await

536 + self.consume_stream(response, sink, tools, allow_tool_calls)

537 + .await

538 } else {

537 - self.consume_json(response, tools).await

539 + self.consume_json(response, tools, allow_tool_calls).await

540 }

541 }

542

@@ -543,6 +545,7 @@ impl OpenAiBackend {

545 &self,

546 response: reqwest::Response,

547 tools: &[ToolSpec],

548 + allow_tool_calls: bool,

549 ) -> Result<TurnResult, BackendError> {

550 let parsed: ChatCompletion = response

551 .json()

@@ -556,7 +559,7 @@ impl OpenAiBackend {

559 .map(|choice| choice.message)

560 .ok_or_else(|| "endpoint returned no choices".to_string())?;

561

559 - let text = message.content.clone().unwrap_or_default();

562 + let mut text = message.content.clone().unwrap_or_default();

563 let tool_calls: Vec<ToolCall> = message

564 .tool_calls

565 .iter()

@@ -568,6 +571,23 @@ impl OpenAiBackend {

571 })

572 .collect();

573

574 + if !allow_tool_calls {

575 + let (cleaned, recovered) = crate::inline_tool_calls::extract(&text, tools);

576 + text = cleaned;

577 + let suppressed = tool_calls.len() + recovered.len();

578 + if suppressed > 0 {

579 + log::warn!("suppressed {suppressed} tool call(s) from a forced-text response");

580 + }

581 + self.history

582 + .lock()

583 + .await

584 + .push(streamed_assistant_history(&text, &[]));

585 + return Ok(TurnResult {

586 + text,

587 + tool_calls: Vec::new(),

588 + });

589 + }

590 +

591 // Some models write a tool call out as literal `<tool_call>` text

592 // instead of using the structured field (see `inline_tool_calls`).

593 // Recover it, or the turn ends with the tag rendered as prose and

@@ -617,6 +637,7 @@ impl OpenAiBackend {

637 response: reqwest::Response,

638 sink: &TokenSink,

639 tools: &[ToolSpec],

640 + allow_tool_calls: bool,

641 ) -> Result<TurnResult, BackendError> {

642 use futures::StreamExt;

643

@@ -691,15 +712,22 @@ impl OpenAiBackend {

712 }

713 }

714 crate::inline_tool_calls::ScanEvent::ToolCall(call) => {

694 - log::warn!(

695 - "recovered tool call '{}' the model emitted as text instead of a structured call",

696 - call.name

697 - );

698 - recovered.push(ToolCall {

699 - id: format!("call_recovered_{}", recovered.len()),

700 - name: call.name,

701 - arguments: call.arguments,

702 - });

715 + if allow_tool_calls {

716 + log::warn!(

717 + "recovered tool call '{}' the model emitted as text instead of a structured call",

718 + call.name

719 + );

720 + recovered.push(ToolCall {

721 + id: format!("call_recovered_{}", recovered.len()),

722 + name: call.name,

723 + arguments: call.arguments,

724 + });

725 + } else {

726 + log::warn!(

727 + "suppressed inline tool call '{}' from a forced-text response",

728 + call.name

729 + );

730 + }

731 }

732 }

733 }

@@ -709,6 +737,10 @@ impl OpenAiBackend {

737 }

738 }

739 for delta in choice.delta.tool_calls.into_iter().flatten() {

740 + if !allow_tool_calls {

741 + log::warn!("suppressed a structured tool call from a forced-text response");

742 + continue;

743 + }

744 let index = delta.index.unwrap_or(0) as usize;

745 if tool_accum.len() <= index {

746 tool_accum.resize_with(index + 1, StreamingToolCall::default);

@@ -813,13 +845,14 @@ impl InferenceBackend for OpenAiBackend {

845 .lock()

846 .await

847 .push(serde_json::json!({ "role": "user", "content": text }));

816 - self.complete(Some(tools), sink).await

848 + self.complete(tools, true, sink).await

849 }

850

851 async fn send_tool_results(

852 &self,

853 results: Vec<ToolResult>,

822 - tools: Option<&[ToolSpec]>,

854 + tools: &[ToolSpec],

855 + allow_tool_calls: bool,

856 sink: Option<&TokenSink>,

857 ) -> Result<TurnResult, BackendError> {

858 {

@@ -832,7 +865,7 @@ impl InferenceBackend for OpenAiBackend {

865 }));

866 }

867 }

835 - self.complete(tools, sink).await

868 + self.complete(tools, allow_tool_calls, sink).await

869 }

870

871 async fn record_cancelled_tool_results(&self, results: Vec<ToolResult>) {

@@ -887,7 +920,7 @@ impl InferenceBackend for OpenAiBackend {

920 }));

921 *self.history.lock().await = request;

922

890 - let summary = match self.complete(None, None).await {

923 + let summary = match self.complete(&[], false, None).await {

924 Ok(result) => result.text,

925 Err(error) => {

926 // Roll back the summarization request; the turn never happened.

src/chat.rs

+3 -7

@@ -3346,12 +3346,8 @@ mod tui {

3346

3347 // on the last round, pass no tools so the model must produce text —

3348 // that's also the round we can stream on-device.

3349 - let next_tools = if round < MAX_TOOL_ROUNDS {

3350 - Some(tools.as_slice())

3351 - } else {

3352 - None

3353 - };

3354 - let sink = if next_tools.is_none() {

3349 + let allow_tool_calls = round < MAX_TOOL_ROUNDS;

3350 + let sink = if !allow_tool_calls {

3351 streamed = true;

3352 Some(&delta_tx)

3353 } else {

@@ -3359,7 +3355,7 @@ mod tui {

3355 };

3356

3357 match backend

3362 - .send_tool_results(tool_results, next_tools, sink)

3358 + .send_tool_results(tool_results, &tools, allow_tool_calls, sink)

3359 .await

3360 {

3361 Ok(r) => result = r,

src/headless.rs

+2 -6

@@ -304,18 +304,14 @@ async fn run_prompt(

304 });

305 }

306

307 - let next_tools = if round < crate::MAX_TOOL_ROUNDS {

308 - Some(tools.as_slice())

309 - } else {

310 - None // last round: force text

311 - };

307 + let allow_tool_calls = round < crate::MAX_TOOL_ROUNDS;

308

309 // Whatever this round says starts a new paragraph rather than

310 // continuing the sentence the tool calls interrupted.

311 reply.interrupt();

312

313 result = drain_to_stdout(

318 - backend.send_tool_results(tool_results, next_tools, sink_opt),

314 + backend.send_tool_results(tool_results, &tools, allow_tool_calls, sink_opt),

315 &mut sink_rx,

316 &mut reply,

317 )

src/inline_tool_calls.rs

+104 -10

@@ -83,16 +83,22 @@ pub fn extract(text: &str, tools: &[ToolSpec]) -> (String, Vec<Recovered>) {

83 fn parse_xml_block(inner: &str, tools: &[ToolSpec]) -> Option<Recovered> {

84 let key_idx = inner.find("<arg_key>");

85 let malformed_key_idx = inner.find(XML_OPEN_TAG);

86 - let (name, mut rest, malformed_first_key) = match (key_idx, malformed_key_idx) {

87 - (Some(key_idx), Some(malformed_key_idx)) if malformed_key_idx < key_idx => (

88 - inner[..malformed_key_idx].trim(),

89 - &inner[malformed_key_idx..],

90 - true,

91 - ),

92 - (Some(idx), _) => (inner[..idx].trim(), &inner[idx..], false),

93 - (None, Some(idx)) => (inner[..idx].trim(), &inner[idx..], true),

94 - (None, None) => (inner.trim(), "", false),

95 - };

86 + let check_status_name = key_idx.and_then(|idx| parse_check_status_preamble(&inner[..idx]));

87 + let (name, mut rest, malformed_first_key) =

88 + if let (Some(idx), Some(name)) = (key_idx, check_status_name) {

89 + (name, &inner[idx..], false)

90 + } else {

91 + match (key_idx, malformed_key_idx) {

92 + (Some(key_idx), Some(malformed_key_idx)) if malformed_key_idx < key_idx => (

93 + inner[..malformed_key_idx].trim(),

94 + &inner[malformed_key_idx..],

95 + true,

96 + ),

97 + (Some(idx), _) => (inner[..idx].trim(), &inner[idx..], false),

98 + (None, Some(idx)) => (inner[..idx].trim(), &inner[idx..], true),

99 + (None, None) => (inner.trim(), "", false),

100 + }

101 + };

102 if name.is_empty() || name.contains(['<', '>']) || !offered_tool(tools, name) {

103 return None;

104 }

@@ -132,6 +138,24 @@ fn parse_xml_block(inner: &str, tools: &[ToolSpec]) -> Option<Recovered> {

138 })

139 }

140

141 +/// Some GLM-family responses prefix the real arguments with generation-control

142 +/// metadata shaped like `tool_name CheckStatus=value</arg_value>`. It is not a

143 +/// tool argument: accept that exact bounded preamble and discard it, while

144 +/// leaving every other malformed prefix untouched.

145 +fn parse_check_status_preamble(prefix: &str) -> Option<&str> {

146 + let (name, status) = prefix.trim().split_once(" CheckStatus=")?;

147 + let status = status.strip_suffix("</arg_value>")?;

148 + if name.is_empty()

149 + || status.is_empty()

150 + || !status

151 + .chars()

152 + .all(|character| character.is_ascii_alphanumeric() || matches!(character, '_' | '-'))

153 + {

154 + return None;

155 + }

156 + Some(name)

157 +}

158 +

159 fn parse_k3_tools_block(inner: &str, tools: &[ToolSpec]) -> Option<Vec<Recovered>> {

160 let mut calls = Vec::new();

161 let mut rest = inner;

@@ -565,6 +589,47 @@ mod tests {

589 assert_eq!(args["cwd"], "/tmp");

590 }

591

592 + #[test]

593 + fn recovers_glm_call_with_check_status_before_real_arguments() {

594 + let tools = vec![command_output_spec()];

595 + let (text, calls) = extract(

596 + "polling <tool_call>command_output CheckStatus=true_or_poll_again_with_different_params</arg_value><arg_key>task_id</arg_key><arg_value>1</arg_value></tool_call>",

597 + &tools,

598 + );

599 +

600 + assert_eq!(text, "polling ");

601 + assert_eq!(calls.len(), 1);

602 + assert_eq!(calls[0].name, "command_output");

603 + let args: serde_json::Value = serde_json::from_str(&calls[0].arguments).unwrap();

604 + assert_eq!(args["task_id"], 1);

605 + assert!(args["task_id"].is_number());

606 + assert!(args.get("CheckStatus").is_none());

607 + }

608 +

609 + #[test]

610 + fn malformed_check_status_preamble_is_left_as_text() {

611 + let text = "<tool_call>command_output CheckStatus=keep going</arg_value><arg_key>task_id</arg_key><arg_value>1</arg_value></tool_call>";

612 + let (out, calls) = extract(text, &[command_output_spec()]);

613 + assert_eq!(out, text);

614 + assert!(calls.is_empty());

615 + }

616 +

617 + #[test]

618 + fn check_status_after_a_real_argument_is_left_as_text() {

619 + let text = "<tool_call>command_output<arg_key>task_id</arg_key><arg_value>1</arg_value> CheckStatus=true_or_poll_again_with_different_params</arg_value></tool_call>";

620 + let (out, calls) = extract(text, &[command_output_spec()]);

621 + assert_eq!(out, text);

622 + assert!(calls.is_empty());

623 + }

624 +

625 + #[test]

626 + fn check_status_for_an_unknown_tool_is_left_as_text() {

627 + let text = "<tool_call>not_offered CheckStatus=true_or_poll_again_with_different_params</arg_value><arg_key>task_id</arg_key><arg_value>1</arg_value></tool_call>";

628 + let (out, calls) = extract(text, &[command_output_spec()]);

629 + assert_eq!(out, text);

630 + assert!(calls.is_empty());

631 + }

632 +

633 #[test]

634 fn unknown_glm_tool_is_left_as_text() {

635 let text = "<tool_call>not_offered<tool_call>command</arg_key><arg_value>pwd</arg_value></tool_call>";

@@ -773,6 +838,35 @@ mod tests {

838 assert_eq!(args["command"], "pwd");

839 }

840

841 + #[test]

842 + fn scanner_recovers_check_status_call_split_across_chunks() {

843 + let tools = vec![command_output_spec()];

844 + let mut scanner = StreamScanner::new(&tools);

845 + let mut text = String::new();

846 + let mut calls = Vec::new();

847 +

848 + for chunk in [

849 + "before <tool_call>command_output CheckStatus=true_or_poll_",

850 + "again_with_different_params</arg_",

851 + "value><arg_key>task_id</arg_key><arg_value>1</arg_value></tool_",

852 + "call> after",

853 + ] {

854 + for event in scanner.push(chunk) {

855 + match event {

856 + ScanEvent::Text(value) => text.push_str(&value),

857 + ScanEvent::ToolCall(call) => calls.push(call),

858 + }

859 + }

860 + }

861 + if let Some(rest) = scanner.take_pending() {

862 + text.push_str(&rest);

863 + }

864 +

865 + assert_eq!(text, "before after");

866 + assert_eq!(calls.len(), 1);

867 + assert_eq!(calls[0].name, "command_output");

868 + }

869 +

870 #[test]

871 fn scanner_recovers_kimi_k3_tools_split_across_chunk_boundaries() {

872 let tools = vec![run_command_spec()];

src/main.rs

+2 -6

@@ -2044,11 +2044,7 @@ impl SiGitAgent {

2044 });

2045 }

2046

2047 - let next_tools = if round < MAX_TOOL_ROUNDS && !force_text {

2048 - Some(tools.as_slice())

2049 - } else {

2050 - None // last round: force text

2051 - };

2047 + let allow_tool_calls = round < MAX_TOOL_ROUNDS && !force_text;

2048

2049 // Whatever this round says starts a new paragraph rather than

2050 // continuing the sentence the tool calls interrupted.

@@ -2058,7 +2054,7 @@ impl SiGitAgent {

2054 .drain_turn(

2055 cx,

2056 &session_id,

2061 - backend.send_tool_results(tool_results, next_tools, Some(&sink)),

2057 + backend.send_tool_results(tool_results, &tools, allow_tool_calls, Some(&sink)),

2058 &mut sink_rx,

2059 &mut reply,

2060 )

src/tools.rs

+2 -6

@@ -939,13 +939,9 @@ async fn run_subagent(backend: &dyn InferenceBackend, prompt: &str, allowed: &[S

939 }

940

941 // On the last round, offer no tools so the model must produce text.

942 - let next_tools = if round < SUBAGENT_MAX_TOOL_ROUNDS {

943 - Some(specs.as_slice())

944 - } else {

945 - None

946 - };

942 + let allow_tool_calls = round < SUBAGENT_MAX_TOOL_ROUNDS;

943 result = match backend

948 - .send_tool_results(tool_results, next_tools, None)

944 + .send_tool_results(tool_results, &specs, allow_tool_calls, None)

945 .await

946 {

947 Ok(r) => r,

tests/acp_permissions.rs

+114

@@ -943,6 +943,120 @@ fn a_tool_call_emitted_as_text_is_executed_rather_than_rendered() {

943 let _ = std::fs::remove_dir_all(&scratch);

944 }

945

946 +/// Once the repetition guard removes tools from a request, a model may still

947 +/// emit its learned tool syntax. Known calls must be removed from the visible

948 +/// response without being executed or recorded as orphaned history entries.

949 +#[test]

950 +fn forced_text_suppresses_glm_check_status_tool_markup() {

951 + let repeated_arguments = json!({"task_id": 1}).to_string();

952 + let endpoint = start_fake_endpoint(vec![

953 + sse_tool_call("call_1", "command_output", &repeated_arguments),

954 + sse_tool_call("call_2", "command_output", &repeated_arguments),

955 + sse_tool_call("call_3", "command_output", &repeated_arguments),

956 + sse_body(&[

957 + json!({"choices": [{"delta": {"content": "Build is still running. <tool_call>command_output CheckStatus=true_or_poll_"}}]}),

958 + json!({"choices": [{"delta": {"content": "again_with_different_params</arg_value><arg_key>task_id</arg_key><arg_value>1</arg_value></tool_call>"}}]}),

959 + // Also enforce the forced-text boundary for a structured call.

960 + json!({"choices": [{"delta": {"tool_calls": [{

961 + "index": 0,

962 + "id": "call_4",

963 + "function": {"name": "command_output", "arguments": "{\"task_id\":999}"},

964 + }]}}]}),

965 + ]),

966 + sse_text("Next turn."),

967 + ]);

968 +

969 + let scratch =

970 + std::env::temp_dir().join(format!("sigit_acp_forced_text_{}", std::process::id()));

971 + let config_dir = scratch.join("config");

972 + let cwd = scratch.join("cwd");

973 + std::fs::create_dir_all(&config_dir).unwrap();

974 + std::fs::create_dir_all(&cwd).unwrap();

975 +

976 + let mut agent = spawn_agent(endpoint.port, &config_dir);

977 +

978 + let id = agent.request(

979 + "initialize",

980 + json!({"protocolVersion": 1, "clientCapabilities": {}}),

981 + );

982 + agent.wait_for_response(id);

983 +

984 + let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []}));

985 + let session_id = agent.wait_for_response(id)["result"]["sessionId"]

986 + .as_str()

987 + .expect("session id")

988 + .to_string();

989 +

990 + let prompt_id = agent.request(

991 + "session/prompt",

992 + json!({

993 + "sessionId": session_id,

994 + "prompt": [{"type": "text", "text": "keep polling"}],

995 + }),

996 + );

997 + let (response, rendered) = agent.wait_for_prompt(prompt_id);

998 + assert_eq!(response["result"]["stopReason"], "end_turn");

999 + assert!(

1000 + rendered.contains("Build is still running."),

1001 + "surrounding prose should remain visible: {rendered:?}"

1002 + );

1003 + assert!(

1004 + !rendered.contains("<tool_call>")

1005 + && !rendered.contains("<arg_key>")

1006 + && !rendered.contains("CheckStatus"),

1007 + "forced tool markup reached the client: {rendered:?}"

1008 + );

1009 +

1010 + {

1011 + let requests = endpoint.requests.lock().unwrap();

1012 + assert_eq!(

1013 + requests.len(),

1014 + 4,

1015 + "a forced-text tool call must not start another inference round"

1016 + );

1017 + assert!(

1018 + requests[3].get("tools").is_none(),

1019 + "the repetition guard must not advertise tools: {:?}",

1020 + requests[3]

1021 + );

1022 + }

1023 +

1024 + // Start another turn so the prior assistant message is replayed and its

1025 + // sanitized history shape can be inspected in the recorded request.

1026 + let next_prompt_id = agent.request(

1027 + "session/prompt",

1028 + json!({

1029 + "sessionId": session_id,

1030 + "prompt": [{"type": "text", "text": "what happened?"}],

1031 + }),

1032 + );

1033 + agent.wait_for_prompt(next_prompt_id);

1034 +

1035 + let requests = endpoint.requests.lock().unwrap();

1036 + let replayed_messages = requests[4]["messages"].as_array().expect("messages");

1037 + let prior_assistant = replayed_messages

1038 + .iter()

1039 + .rev()

1040 + .find(|message| {

1041 + message["role"] == "assistant"

1042 + && message["content"]

1043 + .as_str()

1044 + .is_some_and(|content| content.contains("Build is still running."))

1045 + })

1046 + .expect("sanitized forced-text assistant response");

1047 + assert!(prior_assistant.get("tool_calls").is_none());

1048 + assert!(

1049 + !prior_assistant["content"]

1050 + .as_str()

1051 + .expect("assistant content")

1052 + .contains("<tool_call>")

1053 + );

1054 + drop(requests);

1055 +

1056 + drop(agent);

1057 + let _ = std::fs::remove_dir_all(&scratch);

1058 +}

1059 +

1060 /// Kimi K3 may emit tool calls in XTML content blocks even when the endpoint is

1061 /// OpenAI-compatible. The backend should recover those before ACP sees them,

1062 /// unwrap visible response text, and hide private thinking text.