From a39839fd152c7877b1da0e3771856147a2bdfb2a Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Thu, 13 Aug 2026 20:41:23 +0300 Subject: [PATCH] fix(tool_calling): stop dropping a GLM tag beside a p-format tag `parse_tool_calls_with_pformat` walks ``-family tags, taking a p-format body where one parses and re-parsing the rest as JSON. Once ANY tag yields a p-format call the walk never falls back to the canonical parse, so each remaining tag is on its own - and the JSON path is all it has. A GLM body (`shell/command>ls -la`) is not JSON, so the call was silently dropped: the agent lost a tool invocation it had asked for, and nothing reported it. Adds a GLM fallback for a tag body the JSON path could not read. Ordering is load-bearing: the fallback runs ONLY when the JSON path found nothing. GLM's `name/key>value` shape can occur inside a JSON string value (`{"command": "cat a/b>c"}`), so an unconditional fallback would count that body once as JSON and again as GLM, and the agent would execute the same tool twice. Pinned by `a_json_body_is_not_double_counted_by_the_glm_fallback`. Routing the branch through `parse_tool_calls` instead was the obvious alternative and is wrong. That function forbids the `args`/`parameters`/ `input` argument-key aliases for a bare top-level object, deliberately, so a plain JSON answer cannot be misread as a tool call. But a `` tag IS an explicit marker, where the aliases DO apply - so routing through it would have traded this silent drop for a different one, on aliased tagged calls. Pinned by `a_tagged_body_still_honours_argument_key_aliases`. The bug is inherited, not introduced by the relocation: the probe fails identically against the pre-port OpenHuman code. It was landed as an `#[ignore]`d test in #102 precisely so it stayed visible; this un-ignores it. 1823 lib tests pass, no ignores. Clippy and fmt clean. Co-authored-by: Medulla --- src/harness/tool_calling/parse.rs | 27 ++++++++- src/harness/tool_calling/parse_test.rs | 79 +++++++++++++++++++++----- 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/src/harness/tool_calling/parse.rs b/src/harness/tool_calling/parse.rs index 9682e60..a74c8e0 100644 --- a/src/harness/tool_calling/parse.rs +++ b/src/harness/tool_calling/parse.rs @@ -1001,9 +1001,34 @@ pub fn parse_tool_calls_with_pformat( } else { // Re-parse this tag body with the canonical JSON logic so a body // holding several calls contributes all of them. + // + // Deliberately the *permissive* (alias-honouring) path rather than + // `parse_tool_calls`: a `` tag is an explicit tool-call + // marker, so the `args`/`parameters`/`input` aliases apply here. + // `parse_tool_calls` forbids them for a bare top-level object, so + // routing through it would silently drop an aliased tagged call. + let mut from_body: Vec = Vec::new(); for value in extract_json_values(body) { - combined.extend(parse_tool_calls_from_json_value(&value)); + from_body.extend(parse_tool_calls_from_json_value(&value)); } + + // A tag body need not be JSON at all. GLM emits its own grammar + // (`shell/command>ls -la`), and once ANY tag in the response yields + // a p-format call this walk never falls back to the canonical + // result — so without this the GLM call is dropped and nothing + // reports it. Tried only when the JSON path found nothing, so a + // well-formed JSON body can never be double-counted. + if from_body.is_empty() { + from_body.extend(parse_glm_style_tool_calls(body).into_iter().map( + |(name, arguments, _raw)| ParsedToolCall { + name, + arguments, + id: None, + }, + )); + } + + combined.extend(from_body); } remaining = &after_open[close_idx + close_tag.len()..]; diff --git a/src/harness/tool_calling/parse_test.rs b/src/harness/tool_calling/parse_test.rs index ccf3477..0d55132 100644 --- a/src/harness/tool_calling/parse_test.rs +++ b/src/harness/tool_calling/parse_test.rs @@ -399,21 +399,12 @@ use crate::harness::tool_calling::{PFormatRegistry, PFormatToolParams}; /// A p-format tag alongside a GLM-style sibling. /// -/// **Known pre-existing limitation, inherited from the code this was ported -/// from — `#[ignore]`d rather than deleted so it stays visible.** -/// /// Once any tag yields a p-format call, the walk stops falling back to the -/// canonical parse, and the remaining path handles JSON only. A GLM body -/// (`shell/command>ls`) is not JSON, so that call is silently dropped: the -/// agent loses a tool invocation it asked for and nothing reports it. -/// -/// Verified against the pre-port original, which fails this identically — so -/// it is not a regression from the relocation or from the tag-walk rewrite. -/// Fixing it means routing the non-p-format branch through the full grammar -/// set rather than `extract_json_values`, which is a behaviour change and -/// belongs in its own change with its own review. +/// canonical parse, so every remaining tag is on its own. A GLM body +/// (`shell/command>ls -la`) is not JSON, so before the GLM fallback existed +/// this call was silently dropped — the agent lost a tool invocation it had +/// asked for and nothing reported it. #[test] -#[ignore = "pre-existing: a GLM sibling tag is dropped once a p-format tag matches"] fn a_pformat_tag_does_not_suppress_a_sibling_glm_tag() { let mut reg = PFormatRegistry::new(); reg.insert( @@ -467,3 +458,65 @@ fn a_pformat_tag_does_not_suppress_a_sibling_fenced_json_tag() { "the fenced-JSON sibling was dropped — got {names:?}" ); } + +/// A JSON body that ALSO looks like GLM's `name/key>value` grammar must not +/// yield the call twice. +/// +/// The GLM fallback runs only when the JSON path found nothing, and this is +/// what pins that ordering. If it ever ran unconditionally, a body containing +/// a `/` and a `>` inside a string value would be counted once as JSON and +/// again as GLM — the agent would execute the same tool twice. +#[test] +fn a_json_body_is_not_double_counted_by_the_glm_fallback() { + let mut reg = PFormatRegistry::new(); + reg.insert( + "echo".to_string(), + PFormatToolParams::from_schema(&serde_json::json!({ + "type": "object", + "properties": { "value": { "type": "string" } } + })), + ); + + let response = concat!( + "echo[hello]\n", + "{\"name\": \"shell\", \"arguments\": {\"command\": \"cat a/b>c\"}}" + ); + let (_narrative, calls) = parse_tool_calls_with_pformat(response, ®); + let shell_calls = calls.iter().filter(|c| c.name == "shell").count(); + assert_eq!( + shell_calls, + 1, + "the JSON body was counted twice: {:?}", + calls.iter().map(|c| c.name.as_str()).collect::>() + ); +} + +/// A tagged body may use the argument-key aliases. +/// +/// A `` tag is an explicit tool-call marker, so `args` / +/// `parameters` / `input` are honoured inside it — unlike a bare top-level +/// object, where they are refused so a plain JSON answer cannot read as a +/// call. This pins that the tag path keeps the permissive behaviour: routing +/// it through `parse_tool_calls` instead would silently drop this call. +#[test] +fn a_tagged_body_still_honours_argument_key_aliases() { + let mut reg = PFormatRegistry::new(); + reg.insert( + "echo".to_string(), + PFormatToolParams::from_schema(&serde_json::json!({ + "type": "object", + "properties": { "value": { "type": "string" } } + })), + ); + + let response = concat!( + "echo[hello]\n", + "{\"name\": \"shell\", \"args\": {\"command\": \"ls\"}}" + ); + let (_narrative, calls) = parse_tool_calls_with_pformat(response, ®); + let shell = calls + .iter() + .find(|c| c.name == "shell") + .expect("the aliased tagged call must survive"); + assert_eq!(shell.arguments["command"], "ls"); +}