@sigitcodereview / sigit / commits / b24742b

Hide tool-call markup that can't be parsed, and retry it

Some models write tool calls into the reply as text. Well-formed blocks were already recovered and run, but a block that failed to parse, or never closed, was printed to the editor as-is. It also stayed in history, so the model later read back a call it had "made" with no result, and began writing invented <function_results> of its own. The scanner now reports those blocks as malformed. The backend drops them from the reply and from history. When a reply's only tool call was malformed, the model gets one retry with a short note saying the call didn't run and should go through the tool-calling interface. The well-formed blocks from #97 are covered by new tests and already recovered on development. Fixes #97 Fixes #105

Seto Elkahfi committed Sep 16, 2026 at 22:44 UTC b24742b722cbe4207fb35d853631c13ef7cfdf53
4 files changed +473 -105
.agents/AGENTS.md
+4
@@ -127,6 +127,10 @@ feeds results back. Neither the loop nor ACP/TUI surfaces depend on a concrete b
127 endpoint can also fail *after* the response is open, reporting it as a `data:` frame holding
128 the same envelope; that frame has no `choices`, so `consume_stream` has to check for it
129 explicitly or it parses as an empty chunk and the turn ends looking like an empty answer.
130 + Some models write tool calls into content as text; `src/inline_tool_calls.rs` recovers the
131 + well-formed ones. A block that doesn't parse (or never closes) is dropped from both the reply
132 + and history, and `OpenAiBackend::complete` retries once with a note telling the model the call
133 + didn't run. Leaving the raw block in history makes the model invent `<function_results>` later.
134 - **`src/provider.rs`** — decides *which* backend serves inference. Resolution order, first match
135 wins: (1) override via `OPENAI_BASE_URL`+`OPENAI_API_KEY` or active profile in
136 `~/.config/sigit/providers.toml`; (2) siGit Code Cloud when logged in; (3) on-device.
src/backend.rs
+139 -23
@@ -529,12 +529,52 @@ impl OpenAiBackend {
529 /// `tools` is always the known catalog, while `allow_tool_calls` determines
530 /// whether it is advertised to the model and whether returned calls may run.
531 /// Streams via SSE when `sink` is set; otherwise reads a single JSON response.
532 + ///
533 + /// A reply whose only attempt at a tool call was a block that couldn't be
534 + /// parsed gets one retry: the model is told the call didn't run and asked
535 + /// to make it again through the structured interface. Without that the
536 + /// turn ends on whatever prose preceded the broken block, and the call is
537 + /// silently lost.
538 async fn complete(
539 &self,
540 tools: &[ToolSpec],
541 allow_tool_calls: bool,
542 sink: Option<&TokenSink>,
543 ) -> Result<TurnResult, BackendError> {
544 + let (result, malformed) = self.request(tools, allow_tool_calls, sink).await?;
545 + if malformed == 0 || !allow_tool_calls || !result.tool_calls.is_empty() {
546 + return Ok(result);
547 + }
548 +
549 + log::warn!(
550 + "dropped {malformed} unparseable inline tool call(s); asking the model to retry"
551 + );
552 + self.history.lock().await.push(serde_json::json!({
553 + "role": "user",
554 + "content": MALFORMED_TOOL_CALL_RETRY,
555 + }));
556 + if let Some(sink) = sink
557 + && !result.text.trim().is_empty()
558 + {
559 + // The retry's text follows what already streamed; keep it from
560 + // running on into the previous sentence.
561 + let _ = sink.send("\n\n".to_string());
562 + }
563 + let (retry, _) = self.request(tools, allow_tool_calls, sink).await?;
564 + Ok(TurnResult {
565 + text: join_reply_text(&result.text, &retry.text),
566 + tool_calls: retry.tool_calls,
567 + })
568 + }
569 +
570 + /// One chat-completion round trip. Returns the turn plus how many inline
571 + /// tool-call blocks were dropped because they couldn't be parsed.
572 + async fn request(
573 + &self,
574 + tools: &[ToolSpec],
575 + allow_tool_calls: bool,
576 + sink: Option<&TokenSink>,
577 + ) -> Result<(TurnResult, usize), BackendError> {
578 let url = format!("{}/chat/completions", self.base_url.trim_end_matches('/'));
579 let streaming = sink.is_some();
580
@@ -582,7 +622,7 @@ impl OpenAiBackend {
622 response: reqwest::Response,
623 tools: &[ToolSpec],
624 allow_tool_calls: bool,
585 - ) -> Result<TurnResult, BackendError> {
625 + ) -> Result<(TurnResult, usize), BackendError> {
626 let parsed: ChatCompletion = response
627 .json()
628 .await
@@ -607,9 +647,12 @@ impl OpenAiBackend {
647 })
648 .collect();
649
650 + let extracted = crate::inline_tool_calls::extract(&text, tools);
651 + let malformed = extracted.malformed;
652 +
653 if !allow_tool_calls {
611 - let (cleaned, recovered) = crate::inline_tool_calls::extract(&text, tools);
612 - text = cleaned;
654 + let recovered = extracted.calls;
655 + text = extracted.text;
656 let suppressed = tool_calls.len() + recovered.len();
657 if suppressed > 0 {
658 let names = tool_calls
@@ -626,10 +669,13 @@ impl OpenAiBackend {
669 .lock()
670 .await
671 .push(streamed_assistant_history(&text, &[]));
629 - return Ok(TurnResult {
630 - text,
631 - tool_calls: Vec::new(),
632 - });
672 + return Ok((
673 + TurnResult {
674 + text,
675 + tool_calls: Vec::new(),
676 + },
677 + malformed,
678 + ));
679 }
680
681 // Some models write a tool call out as literal `<tool_call>` text
@@ -637,7 +683,8 @@ impl OpenAiBackend {
683 // Recover it, or the turn ends with the tag rendered as prose and
684 // whatever the model meant to do is dropped.
685 if tool_calls.is_empty() {
640 - let (cleaned, recovered) = crate::inline_tool_calls::extract(&text, tools);
686 + let cleaned = extracted.text;
687 + let recovered = extracted.calls;
688 if !recovered.is_empty() {
689 log::warn!(
690 "recovered {} tool call(s) the model emitted as text instead of a structured call",
@@ -660,17 +707,44 @@ impl OpenAiBackend {
707 .lock()
708 .await
709 .push(streamed_assistant_history(&cleaned, &tool_calls));
663 - return Ok(TurnResult {
664 - text: cleaned,
665 - tool_calls,
666 - });
710 + return Ok((
711 + TurnResult {
712 + text: cleaned,
713 + tool_calls,
714 + },
715 + malformed,
716 + ));
717 + }
718 + if malformed > 0 {
719 + // Keep the broken block out of history as well as the reply;
720 + // see `inline_tool_calls` for why it poisons later turns.
721 + self.push_reply_without_calls(&cleaned).await;
722 + return Ok((
723 + TurnResult {
724 + text: cleaned,
725 + tool_calls,
726 + },
727 + malformed,
728 + ));
729 }
730 }
731
732 // Record the assistant turn so later tool results have context.
733 self.history.lock().await.push(message.into_history_value());
734
673 - Ok(TurnResult { text, tool_calls })
735 + Ok((TurnResult { text, tool_calls }, 0))
736 + }
737 +
738 + /// Record a text-only assistant reply. An empty one is skipped: strict
739 + /// endpoints reject an assistant message with neither content nor tool
740 + /// calls, and there is nothing in it worth replaying.
741 + async fn push_reply_without_calls(&self, text: &str) {
742 + if !text.is_empty() {
743 + self.history
744 + .lock()
745 + .await
746 + .push(streamed_assistant_history(text, &[]));
747 + }
748 }
749
750 /// Consume an OpenAI Server-Sent Events stream, forwarding content deltas to
@@ -682,7 +756,7 @@ impl OpenAiBackend {
756 sink: &TokenSink,
757 tools: &[ToolSpec],
758 allow_tool_calls: bool,
685 - ) -> Result<TurnResult, BackendError> {
759 + ) -> Result<(TurnResult, usize), BackendError> {
760 use futures::StreamExt;
761
762 let mut stream = response.bytes_stream();
@@ -699,6 +773,7 @@ impl OpenAiBackend {
773 // already been rendered. See `inline_tool_calls`.
774 let mut scanner = crate::inline_tool_calls::StreamScanner::new(tools);
775 let mut recovered: Vec<ToolCall> = Vec::new();
776 + let mut malformed = 0usize;
777
778 while let Some(item) = stream.next().await {
779 let bytes = item.map_err(|error| format!("stream read error: {error}"))?;
@@ -768,6 +843,10 @@ impl OpenAiBackend {
843 arguments: call.arguments,
844 });
845 }
846 + crate::inline_tool_calls::ScanEvent::Malformed(block) => {
847 + log_malformed_block(&block);
848 + malformed += 1;
849 + }
850 }
851 }
852 if cancelled {
@@ -800,10 +879,19 @@ impl OpenAiBackend {
879 }
880 }
881
803 - // Text held back waiting on a tag that never closed is just text.
804 - if let Some(leftover) = scanner.take_pending() {
805 - text.push_str(&leftover);
806 - let _ = sink.send(leftover);
882 + // Flush what was held back. A partial marker is just text; a
883 + // tool-call block that never closed is dropped like any other
884 + // unparseable one.
885 + match scanner.finish() {
886 + Some(crate::inline_tool_calls::ScanEvent::Text(leftover)) => {
887 + text.push_str(&leftover);
888 + let _ = sink.send(leftover);
889 + }
890 + Some(crate::inline_tool_calls::ScanEvent::Malformed(block)) => {
891 + log_malformed_block(&block);
892 + malformed += 1;
893 + }
894 + Some(crate::inline_tool_calls::ScanEvent::ToolCall(_)) | None => {}
895 }
896
897 let mut tool_calls: Vec<ToolCall> = tool_accum
@@ -836,12 +924,40 @@ impl OpenAiBackend {
924 }
925
926 // Record the assistant turn so later tool results have context.
839 - self.history
840 - .lock()
841 - .await
842 - .push(streamed_assistant_history(&text, &tool_calls));
927 + if malformed > 0 && tool_calls.is_empty() {
928 + self.push_reply_without_calls(&text).await;
929 + } else {
930 + self.history
931 + .lock()
932 + .await
933 + .push(streamed_assistant_history(&text, &tool_calls));
934 + }
935 +
936 + Ok((TurnResult { text, tool_calls }, malformed))
937 + }
938 +}
939 +
940 +/// Sent as a user turn after a reply whose tool call couldn't be parsed.
941 +const MALFORMED_TOOL_CALL_RETRY: &str = "[siGit Code] Your last reply tried to call a tool by \
942 + writing the call out as text, and it could not be parsed, so nothing ran and the user did \
943 + not see it. Do not write tool calls or tool results as text. Make the call again using the \
944 + tool-calling interface, or answer in plain prose if no tool is needed.";
945 +
946 +fn log_malformed_block(block: &str) {
947 + log::warn!(
948 + "dropped an unparseable inline tool call ({} chars): {}",
949 + block.len(),
950 + block.chars().take(200).collect::<String>()
951 + );
952 +}
953
844 - Ok(TurnResult { text, tool_calls })
954 +/// Join the visible text of a reply and its retry the way the stream showed
955 +/// them: as separate paragraphs.
956 +fn join_reply_text(first: &str, retry: &str) -> String {
957 + match (first.trim().is_empty(), retry.trim().is_empty()) {
958 + (true, _) => retry.to_string(),
959 + (false, true) => first.to_string(),
960 + (false, false) => format!("{first}\n\n{retry}"),
961 }
962 }
963
src/inline_tool_calls.rs
+240 -82
@@ -18,11 +18,15 @@
18 //! OpenAI-compatible base URL from `providers.toml` or `OPENAI_BASE_URL`, and
19 //! on-device models. A fix in any single upstream leaves the others exposed.
20 //!
21 -//! Only well-formed shapes are recovered. A block that doesn't match is left
22 -//! in the text untouched, deliberately. Reissuing a `run_command` is cheap,
23 -//! but guessing wrong at a half-parsed `edit_file` would write the wrong
24 -//! change to a file, so a malformed block stays visible rather than being
25 -//! silently misinterpreted.
21 +//! Only well-formed shapes are recovered. A block that doesn't match is never
22 +//! guessed at: reissuing a `run_command` is cheap, but guessing wrong at a
23 +//! half-parsed `edit_file` would write the wrong change to a file. It isn't
24 +//! shown either. A tool-call block that fails to parse, or that is still open
25 +//! when the reply ends, is reported as [`ScanEvent::Malformed`] and kept out of
26 +//! both the UI and the conversation history. Left in the history, the model
27 +//! reads back a call it "made" with no result attached, and later turns start
28 +//! inventing `<function_results>` blocks of their own. The backend tells the
29 +//! model the call didn't run and asks for it again instead.
30
31 use std::collections::HashMap;
32
@@ -53,26 +57,36 @@ pub struct Recovered {
57 pub arguments: String,
58 }
59
56 -/// Scan a complete text blob for inline tool-call blocks. Returns the text
57 -/// with recovered blocks removed, plus the calls recovered from them. Blocks
58 -/// that don't parse cleanly, and unterminated trailing tags, are left in the
59 -/// returned text untouched.
60 -pub fn extract(text: &str, tools: &[ToolSpec]) -> (String, Vec<Recovered>) {
60 +/// What [`extract`] found in a complete text blob.
61 +#[derive(Debug, Default, PartialEq, Eq)]
62 +pub struct Extracted {
63 + /// The text with every tool-call block removed, recovered or not.
64 + pub text: String,
65 + /// Calls recovered from well-formed blocks, in order.
66 + pub calls: Vec<Recovered>,
67 + /// Tool-call blocks that were dropped because they couldn't be parsed.
68 + pub malformed: usize,
69 +}
70 +
71 +/// Scan a complete text blob for inline tool-call blocks. Recovered blocks
72 +/// become calls; blocks that don't parse cleanly, including an unterminated
73 +/// trailing one, are dropped and counted in [`Extracted::malformed`].
74 +pub fn extract(text: &str, tools: &[ToolSpec]) -> Extracted {
75 let mut scanner = StreamScanner::new(tools);
62 - let mut out = String::with_capacity(text.len());
63 - let mut calls = Vec::new();
76 + let mut extracted = Extracted {
77 + text: String::with_capacity(text.len()),
78 + ..Extracted::default()
79 + };
80
65 - for event in scanner.push(text) {
81 + for event in scanner.push(text).into_iter().chain(scanner.finish()) {
82 match event {
67 - ScanEvent::Text(text) => out.push_str(&text),
68 - ScanEvent::ToolCall(call) => calls.push(call),
83 + ScanEvent::Text(text) => extracted.text.push_str(&text),
84 + ScanEvent::ToolCall(call) => extracted.calls.push(call),
85 + ScanEvent::Malformed(_) => extracted.malformed += 1,
86 }
87 }
71 - if let Some(rest) = scanner.take_pending() {
72 - out.push_str(&rest);
73 - }
88
75 - (out, calls)
89 + extracted
90 }
91
92 /// Parse one legacy XML block's inner text: an offered tool name, then zero or
@@ -314,6 +328,9 @@ pub enum ScanEvent {
328 Text(String),
329 /// A tool call recovered from a completed block.
330 ToolCall(Recovered),
331 + /// A tool-call block that couldn't be parsed, or never closed. Carries the
332 + /// raw block for logging; it must not be shown or executed.
333 + Malformed(String),
334 }
335
336 /// Incremental scanner for a live delta stream.
@@ -342,15 +359,23 @@ impl<'a> StreamScanner<'a> {
359 self.drain()
360 }
361
345 - /// Take whatever is still held back, e.g. a `<tool_call` or Kimi marker
346 - /// prefix that never closed. Text we never saw the end of can't be
347 - /// interpreted safely, so the honest thing is to hand it back as-is.
348 - pub fn take_pending(&mut self) -> Option<String> {
362 + /// Call once the stream has ended to flush whatever is still held back.
363 + /// A tool-call block that opened but never closed is [`ScanEvent::Malformed`]:
364 + /// it can't be executed safely, and it isn't prose either. Anything else,
365 + /// such as a partial marker prefix or an unclosed Kimi response wrapper, is
366 + /// handed back as text.
367 + pub fn finish(&mut self) -> Option<ScanEvent> {
368 if self.pending.is_empty() {
350 - None
351 - } else {
352 - Some(std::mem::take(&mut self.pending))
369 + return None;
370 }
371 + let rest = std::mem::take(&mut self.pending);
372 + Some(
373 + if rest.starts_with(XML_OPEN_TAG) || rest.starts_with(K3_TOOLS_OPEN) {
374 + ScanEvent::Malformed(rest)
375 + } else {
376 + ScanEvent::Text(rest)
377 + },
378 + )
379 }
380
381 fn drain(&mut self) -> Vec<ScanEvent> {
@@ -416,7 +441,7 @@ impl<'a> StreamScanner<'a> {
441 let inner = &block[XML_OPEN_TAG.len()..block.len() - XML_CLOSE_TAG.len()];
442 Some(match parse_xml_block(inner, self.tools) {
443 Some(call) => ScanEvent::ToolCall(call),
419 - None => ScanEvent::Text(block),
444 + None => ScanEvent::Malformed(block),
445 })
446 }
447
@@ -431,7 +456,7 @@ impl<'a> StreamScanner<'a> {
456
457 Some(match parse_k3_tools_block(inner, self.tools) {
458 Some(calls) => calls.into_iter().map(ScanEvent::ToolCall).collect(),
434 - None => vec![ScanEvent::Text(block)],
459 + None => vec![ScanEvent::Malformed(block)],
460 })
461 }
462
@@ -473,9 +498,7 @@ impl<'a> StreamScanner<'a> {
498 fn scan_complete_text(text: &str, tools: &[ToolSpec]) -> Vec<ScanEvent> {
499 let mut scanner = StreamScanner::new(tools);
500 let mut events = scanner.push(text);
476 - if let Some(rest) = scanner.take_pending() {
477 - events.push(ScanEvent::Text(rest));
478 - }
501 + events.extend(scanner.finish());
502 events
503 }
504
@@ -547,7 +570,7 @@ mod tests {
570 #[test]
571 fn an_integer_argument_is_recovered_as_a_json_number() {
572 let tools = vec![command_output_spec()];
550 - let (text, calls) = extract(
573 + let Extracted { text, calls, .. } = extract(
574 "Checking.<tool_call>command_output<arg_key>task_id</arg_key><arg_value>2</arg_value></tool_call>",
575 &tools,
576 );
@@ -562,7 +585,7 @@ mod tests {
585 #[test]
586 fn recovers_multiple_string_arguments_in_order() {
587 let tools = vec![run_command_spec()];
565 - let (text, calls) = extract(
588 + let Extracted { text, calls, .. } = extract(
589 "<tool_call>run_command<arg_key>command</arg_key><arg_value>cargo test --locked 2>&1 | tail -20</arg_value><arg_key>cwd</arg_key><arg_value>/Users/x/sigit</arg_value><arg_key>run_in_background</arg_key><arg_value>true</arg_value></tool_call>",
590 &tools,
591 );
@@ -576,7 +599,7 @@ mod tests {
599 #[test]
600 fn recovers_glm_call_with_a_malformed_first_argument_marker() {
601 let tools = vec![run_command_spec()];
579 - let (text, calls) = extract(
602 + let Extracted { text, calls, .. } = extract(
603 "<tool_call>run_command<tool_call>command</arg_key><arg_value>pwd</arg_value><arg_key>cwd</arg_key><arg_value>/tmp</arg_value></tool_call>",
604 &tools,
605 );
@@ -592,7 +615,7 @@ mod tests {
615 #[test]
616 fn recovers_glm_call_with_check_status_before_real_arguments() {
617 let tools = vec![command_output_spec()];
595 - let (text, calls) = extract(
618 + let Extracted { text, calls, .. } = extract(
619 "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>",
620 &tools,
621 );
@@ -607,64 +630,101 @@ mod tests {
630 }
631
632 #[test]
610 - fn malformed_check_status_preamble_is_left_as_text() {
633 + fn malformed_check_status_preamble_is_dropped() {
634 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);
635 + let Extracted {
636 + text: out,
637 + calls,
638 + malformed,
639 + } = extract(text, &[command_output_spec()]);
640 + assert_eq!(out, "");
641 + assert_eq!(malformed, 1);
642 assert!(calls.is_empty());
643 }
644
645 #[test]
618 - fn check_status_after_a_real_argument_is_left_as_text() {
646 + fn check_status_after_a_real_argument_is_dropped() {
647 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);
648 + let Extracted {
649 + text: out,
650 + calls,
651 + malformed,
652 + } = extract(text, &[command_output_spec()]);
653 + assert_eq!(out, "");
654 + assert_eq!(malformed, 1);
655 assert!(calls.is_empty());
656 }
657
658 #[test]
626 - fn check_status_for_an_unknown_tool_is_left_as_text() {
659 + fn check_status_for_an_unknown_tool_is_dropped() {
660 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);
661 + let Extracted {
662 + text: out,
663 + calls,
664 + malformed,
665 + } = extract(text, &[command_output_spec()]);
666 + assert_eq!(out, "");
667 + assert_eq!(malformed, 1);
668 assert!(calls.is_empty());
669 }
670
671 #[test]
634 - fn unknown_glm_tool_is_left_as_text() {
672 + fn unknown_glm_tool_is_dropped() {
673 let text = "<tool_call>not_offered<tool_call>command</arg_key><arg_value>pwd</arg_value></tool_call>";
636 - let (out, calls) = extract(text, &[run_command_spec()]);
637 - assert_eq!(out, text);
674 + let Extracted {
675 + text: out,
676 + calls,
677 + malformed,
678 + } = extract(text, &[run_command_spec()]);
679 + assert_eq!(out, "");
680 + assert_eq!(malformed, 1);
681 assert!(calls.is_empty());
682 }
683
684 #[test]
685 fn malformed_glm_marker_is_accepted_only_for_the_first_argument() {
686 let text = "<tool_call>run_command<arg_key>command</arg_key><arg_value>pwd</arg_value><tool_call>cwd</arg_key><arg_value>/tmp</arg_value></tool_call>";
644 - let (out, calls) = extract(text, &[run_command_spec()]);
645 - assert_eq!(out, text);
687 + let Extracted {
688 + text: out,
689 + calls,
690 + malformed,
691 + } = extract(text, &[run_command_spec()]);
692 + assert_eq!(out, "");
693 + assert_eq!(malformed, 1);
694 assert!(calls.is_empty());
695 }
696
697 #[test]
650 - fn a_malformed_block_is_left_alone_rather_than_guessed_at() {
698 + fn a_malformed_block_is_dropped_rather_than_guessed_at() {
699 let text = "<tool_call>edit_file<arg_value>new_text</arg_key><arg_value>def x; end</arg_value></tool_call>";
652 - let (out, calls) = extract(text, &[run_command_spec()]);
653 - assert_eq!(out, text);
700 + let Extracted {
701 + text: out,
702 + calls,
703 + malformed,
704 + } = extract(text, &[run_command_spec()]);
705 + assert_eq!(out, "");
706 + assert_eq!(malformed, 1);
707 assert!(calls.is_empty());
708 }
709
710 #[test]
658 - fn an_unterminated_tag_is_left_alone() {
711 + fn an_unterminated_tag_is_dropped() {
712 let text = "working on it <tool_call>run_command<arg_key>command</arg_key>";
660 - let (out, calls) = extract(text, &[run_command_spec()]);
661 - assert_eq!(out, text);
713 + let Extracted {
714 + text: out,
715 + calls,
716 + malformed,
717 + } = extract(text, &[run_command_spec()]);
718 + assert_eq!(out, "working on it ");
719 + assert_eq!(malformed, 1);
720 assert!(calls.is_empty());
721 }
722
723 #[test]
724 fn ordinary_text_passes_through_untouched() {
667 - let (out, calls) = extract("if x < y then a<b, nothing to recover", &[]);
725 + let Extracted {
726 + text: out, calls, ..
727 + } = extract("if x < y then a<b, nothing to recover", &[]);
728 assert_eq!(out, "if x < y then a<b, nothing to recover");
729 assert!(calls.is_empty());
730 }
@@ -672,7 +732,7 @@ mod tests {
732 #[test]
733 fn kimi_k3_run_command_block_is_recovered_and_removed() {
734 let tools = vec![run_command_spec()];
675 - let (text, calls) = extract(
735 + let Extracted { text, calls, .. } = extract(
736 "Working <|open|>tools<|sep|><|open|>call tool=\"run_command\" index=\"1\"<|sep|><|open|>argument key=\"command\" type=\"string\"<|sep|>sleep 180; echo waited<|close|>argument<|sep|><|open|>argument key=\"cwd\" type=\"string\"<|sep|>/Users/setoelkahfi/Repositories/onde-ed<|close|>argument<|sep|><|close|>call<|sep|><|close|>tools<|sep|>",
737 &tools,
738 );
@@ -687,7 +747,7 @@ mod tests {
747 #[test]
748 fn kimi_k3_non_string_arguments_are_json_decoded() {
749 let tools = vec![command_output_spec()];
690 - let (_, calls) = extract(
750 + let Extracted { calls, .. } = extract(
751 "<|open|>tools<|sep|><|open|>call tool=\"command_output\" index=\"1\"<|sep|><|open|>argument key=\"task_id\" type=\"integer\"<|sep|>2<|close|>argument<|sep|><|close|>call<|sep|><|close|>tools<|sep|>",
752 &tools,
753 );
@@ -699,7 +759,7 @@ mod tests {
759 #[test]
760 fn kimi_k3_missing_argument_type_uses_the_tool_schema() {
761 let tools = vec![command_output_spec()];
702 - let (_, calls) = extract(
762 + let Extracted { calls, .. } = extract(
763 "<|open|>tools<|sep|><|open|>call tool=\"command_output\" index=\"1\"<|sep|><|open|>argument key=\"task_id\"<|sep|>2<|close|>argument<|sep|><|close|>call<|sep|><|close|>tools<|sep|>",
764 &tools,
765 );
@@ -711,7 +771,7 @@ mod tests {
771 #[test]
772 fn kimi_k3_multiple_calls_are_recovered_in_order() {
773 let tools = vec![command_output_spec()];
714 - let (_, calls) = extract(
774 + let Extracted { calls, .. } = extract(
775 "<|open|>tools<|sep|><|open|>call tool=\"command_output\" index=\"1\"<|sep|><|open|>argument key=\"task_id\" type=\"integer\"<|sep|>1<|close|>argument<|sep|><|close|>call<|sep|><|open|>call tool=\"command_output\" index=\"2\"<|sep|><|open|>argument key=\"task_id\" type=\"integer\"<|sep|>2<|close|>argument<|sep|><|close|>call<|sep|><|close|>tools<|sep|>",
776 &tools,
777 );
@@ -724,7 +784,7 @@ mod tests {
784
785 #[test]
786 fn kimi_k3_response_is_unwrapped_and_think_is_hidden() {
727 - let (text, calls) = extract(
787 + let Extracted { text, calls, .. } = extract(
788 "a<|open|>think<|sep|>private<|close|>think<|sep|>b<|open|>response<|sep|>visible<|close|>response<|sep|>c",
789 &[],
790 );
@@ -735,7 +795,7 @@ mod tests {
795 #[test]
796 fn kimi_k3_response_content_is_scanned_for_nested_tool_calls() {
797 let tools = vec![command_output_spec()];
738 - let (text, calls) = extract(
798 + let Extracted { text, calls, .. } = extract(
799 "<|open|>response<|sep|>before <|open|>think<|sep|>private<|close|>think<|sep|><|open|>tools<|sep|><|open|>call tool=\"command_output\" index=\"1\"<|sep|><|open|>argument key=\"task_id\" type=\"integer\"<|sep|>2<|close|>argument<|sep|><|close|>call<|sep|><|close|>tools<|sep|> after<|close|>response<|sep|>",
800 &tools,
801 );
@@ -753,26 +813,41 @@ mod tests {
813 }
814
815 #[test]
756 - fn malformed_kimi_k3_tools_block_is_left_alone() {
816 + fn unterminated_kimi_k3_tools_block_is_dropped() {
817 let text = "<|open|>tools<|sep|><|open|>call tool=\"run_command\" index=\"1\"<|sep|><|open|>argument key=\"command\" type=\"string\"<|sep|>echo hi<|close|>argument<|sep|>";
758 - let (out, calls) = extract(text, &[run_command_spec()]);
759 - assert_eq!(out, text);
818 + let Extracted {
819 + text: out,
820 + calls,
821 + malformed,
822 + } = extract(text, &[run_command_spec()]);
823 + assert_eq!(out, "");
824 + assert_eq!(malformed, 1);
825 assert!(calls.is_empty());
826 }
827
828 #[test]
764 - fn unknown_kimi_k3_tool_block_is_left_alone() {
829 + fn unknown_kimi_k3_tool_block_is_dropped() {
830 let text = "<|open|>tools<|sep|><|open|>call tool=\"delete_everything\" index=\"1\"<|sep|><|close|>call<|sep|><|close|>tools<|sep|>";
766 - let (out, calls) = extract(text, &[run_command_spec()]);
767 - assert_eq!(out, text);
831 + let Extracted {
832 + text: out,
833 + calls,
834 + malformed,
835 + } = extract(text, &[run_command_spec()]);
836 + assert_eq!(out, "");
837 + assert_eq!(malformed, 1);
838 assert!(calls.is_empty());
839 }
840
841 #[test]
772 - fn invalid_kimi_k3_non_string_argument_is_left_alone() {
842 + fn invalid_kimi_k3_non_string_argument_is_dropped() {
843 let text = "<|open|>tools<|sep|><|open|>call tool=\"command_output\" index=\"1\"<|sep|><|open|>argument key=\"task_id\" type=\"integer\"<|sep|>not-json<|close|>argument<|sep|><|close|>call<|sep|><|close|>tools<|sep|>";
774 - let (out, calls) = extract(text, &[command_output_spec()]);
775 - assert_eq!(out, text);
844 + let Extracted {
845 + text: out,
846 + calls,
847 + malformed,
848 + } = extract(text, &[command_output_spec()]);
849 + assert_eq!(out, "");
850 + assert_eq!(malformed, 1);
851 assert!(calls.is_empty());
852 }
853
@@ -795,10 +870,11 @@ mod tests {
870 match event {
871 ScanEvent::Text(t) => text.push_str(&t),
872 ScanEvent::ToolCall(c) => calls.push(c),
873 + ScanEvent::Malformed(block) => panic!("unexpected malformed block: {block}"),
874 }
875 }
876 }
801 - if let Some(rest) = scanner.take_pending() {
877 + if let Some(ScanEvent::Text(rest)) = scanner.finish() {
878 text.push_str(&rest);
879 }
880
@@ -824,10 +900,11 @@ mod tests {
900 match event {
901 ScanEvent::Text(value) => text.push_str(&value),
902 ScanEvent::ToolCall(call) => calls.push(call),
903 + ScanEvent::Malformed(block) => panic!("unexpected malformed block: {block}"),
904 }
905 }
906 }
830 - if let Some(rest) = scanner.take_pending() {
907 + if let Some(ScanEvent::Text(rest)) = scanner.finish() {
908 text.push_str(&rest);
909 }
910
@@ -855,10 +932,11 @@ mod tests {
932 match event {
933 ScanEvent::Text(value) => text.push_str(&value),
934 ScanEvent::ToolCall(call) => calls.push(call),
935 + ScanEvent::Malformed(block) => panic!("unexpected malformed block: {block}"),
936 }
937 }
938 }
861 - if let Some(rest) = scanner.take_pending() {
939 + if let Some(ScanEvent::Text(rest)) = scanner.finish() {
940 text.push_str(&rest);
941 }
942
@@ -886,10 +964,11 @@ mod tests {
964 match event {
965 ScanEvent::Text(t) => text.push_str(&t),
966 ScanEvent::ToolCall(c) => calls.push(c),
967 + ScanEvent::Malformed(block) => panic!("unexpected malformed block: {block}"),
968 }
969 }
970 }
892 - if let Some(rest) = scanner.take_pending() {
971 + if let Some(ScanEvent::Text(rest)) = scanner.finish() {
972 text.push_str(&rest);
973 }
974
@@ -913,10 +992,11 @@ mod tests {
992 match event {
993 ScanEvent::Text(chunk) => text.push_str(&chunk),
994 ScanEvent::ToolCall(call) => calls.push(call),
995 + ScanEvent::Malformed(block) => panic!("unexpected malformed block: {block}"),
996 }
997 }
998 }
919 - if let Some(rest) = scanner.take_pending() {
999 + if let Some(ScanEvent::Text(rest)) = scanner.finish() {
1000 text.push_str(&rest);
1001 }
1002
@@ -928,7 +1008,7 @@ mod tests {
1008 #[test]
1009 fn scanner_recovers_tools_after_an_unclosed_k3_think_wrapper() {
1010 let tools = vec![command_output_spec()];
931 - let (text, calls) = extract(
1011 + let Extracted { text, calls, .. } = extract(
1012 "<|open|>think<|sep|>broken <|open|>tools<|sep|><|open|>call tool=\"command_output\" index=\"1\"<|sep|><|open|>argument key=\"task_id\" type=\"integer\"<|sep|>2<|close|>argument<|sep|><|close|>call<|sep|><|close|>tools<|sep|>",
1013 &tools,
1014 );
@@ -945,7 +1025,7 @@ mod tests {
1025 events,
1026 vec![ScanEvent::Text("a normal streamed answer".to_string())]
1027 );
948 - assert!(scanner.take_pending().is_none());
1028 + assert!(scanner.finish().is_none());
1029 }
1030
1031 #[test]
@@ -959,18 +1039,96 @@ mod tests {
1039 scanner.push("box for details"),
1040 vec![ScanEvent::Text("<toolbox for details".to_string())]
1041 );
962 - assert!(scanner.take_pending().is_none());
1042 + assert!(scanner.finish().is_none());
1043 }
1044
1045 #[test]
966 - fn scanner_hands_back_an_unclosed_tag_at_stream_end() {
1046 + fn scanner_reports_an_unclosed_tag_at_stream_end_as_malformed() {
1047 let tools = vec![run_command_spec()];
1048 let mut scanner = StreamScanner::new(&tools);
1049 let events = scanner.push("partial <tool_call>run_command<arg_key>cmd");
1050 assert_eq!(events, vec![ScanEvent::Text("partial ".to_string())]);
1051 assert_eq!(
972 - scanner.take_pending().as_deref(),
973 - Some("<tool_call>run_command<arg_key>cmd")
1052 + scanner.finish(),
1053 + Some(ScanEvent::Malformed(
1054 + "<tool_call>run_command<arg_key>cmd".to_string()
1055 + ))
1056 + );
1057 + }
1058 +
1059 + #[test]
1060 + fn scanner_hands_back_a_partial_marker_prefix_at_stream_end_as_text() {
1061 + let mut scanner = StreamScanner::new(&[]);
1062 + assert_eq!(
1063 + scanner.push("see <tool_"),
1064 + vec![ScanEvent::Text("see ".to_string())]
1065 + );
1066 + assert_eq!(
1067 + scanner.finish(),
1068 + Some(ScanEvent::Text("<tool_".to_string()))
1069 );
1070 }
1071 +
1072 + /// Issue #105: a GLM-family reply that opened a call, then wrote an
1073 + /// invented invoke/result transcript instead of closing it. None of that
1074 + /// may reach the editor, and it must not count as a recovered call.
1075 + #[test]
1076 + fn a_hallucinated_invoke_transcript_is_dropped() {
1077 + let tools = vec![command_output_spec()];
1078 + let reply = "Checking.<tool_call>command_output\n\n<invokeID>reply_A</invokeID>\n\
1079 + <parameter_name>task_id</parameter_name>\n<parameter>6</parameter>\n\
1080 + </invoke>\n\n<function_results>PyPI and NuGet are queued.\n\
1081 + <tool_call>command_output<arg_key>task_id</arg_key><arg_value>5</task_id>\n\
1082 + </invoke>\n</function_results>";
1083 + let Extracted {
1084 + text,
1085 + calls,
1086 + malformed,
1087 + } = extract(reply, &tools);
1088 + assert_eq!(text, "Checking.");
1089 + assert!(calls.is_empty());
1090 + assert_eq!(malformed, 1);
1091 + }
1092 +
1093 + /// Issue #97: the well-formed blocks from that thread are recovered as
1094 + /// real calls, multi-line argument values included.
1095 + #[test]
1096 + fn well_formed_blocks_from_issue_97_are_recovered() {
1097 + let tools = vec![
1098 + run_command_spec(),
1099 + spec(
1100 + "edit_file",
1101 + serde_json::json!({
1102 + "type": "object",
1103 + "properties": {
1104 + "path": { "type": "string" },
1105 + "old_text": { "type": "string" },
1106 + "new_text": { "type": "string" }
1107 + }
1108 + }),
1109 + ),
1110 + ];
1111 + let reply = "Now wire `body_path` into the step:<tool_call>edit_file<arg_key>new_text</arg_key><arg_value> - name: Release\n with:\n body_path: release_notes.md</arg_value><arg_key>old_text</arg_key><arg_value> - name: Release\n with:</arg_value><arg_key>path</arg_key><arg_value>/repo/.github/workflows/release-github.yml</arg_value></tool_call>";
1112 + let Extracted {
1113 + text,
1114 + calls,
1115 + malformed,
1116 + } = extract(reply, &tools);
1117 + assert_eq!(text, "Now wire `body_path` into the step:");
1118 + assert_eq!(malformed, 0);
1119 + assert_eq!(calls.len(), 1);
1120 + let args: serde_json::Value = serde_json::from_str(&calls[0].arguments).unwrap();
1121 + assert_eq!(
1122 + args["new_text"],
1123 + " - name: Release\n with:\n body_path: release_notes.md"
1124 + );
1125 + assert_eq!(args["path"], "/repo/.github/workflows/release-github.yml");
1126 +
1127 + let Extracted { calls, .. } = extract(
1128 + "<tool_call>run_command<arg_key>command</arg_key><arg_value>sleep 90</arg_value><arg_key>cwd</arg_key><arg_value>/repo</arg_value></tool_call>",
1129 + &tools,
1130 + );
1131 + assert_eq!(calls.len(), 1);
1132 + assert_eq!(calls[0].name, "run_command");
1133 + }
1134 }
tests/acp_permissions.rs
+90
@@ -943,6 +943,96 @@ fn a_tool_call_emitted_as_text_is_executed_rather_than_rendered() {
943 let _ = std::fs::remove_dir_all(&scratch);
944 }
945
946 +/// A tool-call block that can't be parsed must not reach the editor, and must
947 +/// not stay in history where the model reads it back as a call it made and
948 +/// starts inventing results (getsigit/sigit#97, #105). The model gets one
949 +/// retry with a note that the call didn't run.
950 +#[test]
951 +fn an_unparseable_tool_call_is_hidden_and_retried() {
952 + let endpoint = start_fake_endpoint(vec![
953 + sse_body(&[
954 + json!({"choices": [{"delta": {"content": "Checking. <tool_call>command_output\n\n<invokeID>reply_A"}}]}),
955 + json!({"choices": [{"delta": {"content": "</invokeID>\n<parameter>6</parameter>\n</invoke>\n<function_results>all green"}}]}),
956 + ]),
957 + sse_tool_call(
958 + "call_1",
959 + "command_output",
960 + &json!({"task_id": 6}).to_string(),
961 + ),
962 + sse_text("All done."),
963 + ]);
964 +
965 + let scratch = std::env::temp_dir().join(format!("sigit_acp_malformed_{}", std::process::id()));
966 + let config_dir = scratch.join("config");
967 + let cwd = scratch.join("cwd");
968 + std::fs::create_dir_all(&config_dir).unwrap();
969 + std::fs::create_dir_all(&cwd).unwrap();
970 +
971 + let mut agent = spawn_agent(endpoint.port, &config_dir);
972 +
973 + let id = agent.request(
974 + "initialize",
975 + json!({"protocolVersion": 1, "clientCapabilities": {}}),
976 + );
977 + agent.wait_for_response(id);
978 +
979 + let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []}));
980 + let session_id = agent.wait_for_response(id)["result"]["sessionId"]
981 + .as_str()
982 + .expect("session id")
983 + .to_string();
984 +
985 + let prompt_id = agent.request(
986 + "session/prompt",
987 + json!({
988 + "sessionId": session_id,
989 + "prompt": [{"type": "text", "text": "is the build done?"}],
990 + }),
991 + );
992 + let (response, rendered) = agent.wait_for_prompt(prompt_id);
993 + assert_eq!(response["result"]["stopReason"], "end_turn");
994 + for markup in [
995 + "<tool_call>",
996 + "<invokeID>",
997 + "<function_results>",
998 + "all green",
999 + ] {
1000 + assert!(
1001 + !rendered.contains(markup),
1002 + "{markup} reached the client: {rendered:?}"
1003 + );
1004 + }
1005 + assert!(rendered.contains("Checking."), "{rendered:?}");
1006 + assert!(rendered.contains("All done."), "{rendered:?}");
1007 +
1008 + let requests = endpoint.requests.lock().unwrap();
1009 + assert_eq!(
1010 + requests.len(),
1011 + 3,
1012 + "expected the retry, then the round answering the retried call"
1013 + );
1014 + let retry_messages = requests[1]["messages"].as_array().expect("messages");
1015 + let last = retry_messages.last().expect("retry note");
1016 + assert_eq!(last["role"], "user");
1017 + assert!(
1018 + last["content"]
1019 + .as_str()
1020 + .is_some_and(|content| content.starts_with("[siGit Code]")),
1021 + "{last:?}"
1022 + );
1023 + for message in requests[2]["messages"].as_array().expect("messages") {
1024 + let content = message["content"].as_str().unwrap_or_default();
1025 + assert!(
1026 + !content.contains("<tool_call>") && !content.contains("<invokeID>"),
1027 + "broken markup was kept in history: {message:?}"
1028 + );
1029 + }
1030 + drop(requests);
1031 +
1032 + drop(agent);
1033 + let _ = std::fs::remove_dir_all(&scratch);
1034 +}
1035 +
1036 /// Once the repetition guard removes tools from a request, a model may still
1037 /// emit its learned tool syntax. Known calls must be removed from the visible
1038 /// response without being executed or recorded as orphaned history entries.