fix(tool_calling): land the GLM sibling-drop fix, and make the module usable by a host

Two halves, both discovered by actually wiring OpenHuman onto this module.

1. The GLM sibling drop (re-lands #104 against main)

#104 merged into #102's branch, but #102 had already merged to main - so the
fix never reached main. `main` today carries `tool_calling` with the
`#[ignore]`d test and no fix.

`parse_tool_calls_with_pformat` walks `<tool_call>`-family tags. Once ANY
tag yields a p-format call the walk never falls back to the canonical
parse, so every remaining tag has only the JSON path. A GLM body
(`shell/command>ls -la`) is not JSON, so the call was silently dropped.

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"}`), and an unconditional fallback would count
that body twice, making the agent run the same tool twice. Routing through
`parse_tool_calls` instead is also wrong - it forbids the argument-key
aliases for a bare top-level object, but a `<tool_call>` tag IS an explicit
marker where they apply, so that would trade one silent drop for another.
Both pinned by tests.

2. Host-consumability fixes

The first real consumer could not compile against this module:

- `extract_json_values` was unreachable, and it is not a test helper:
  pulling the first JSON object out of model prose is how a host checks a
  required-output contract, which has nothing to do with tool calls. Now
  exported, along with `parse_arguments_value`,
  `parse_glm_style_tool_calls`, `parse_tool_call_value` and
  `parse_tool_calls_from_json_value`, all of which host tests exercise
  directly.
- `parse_tool_call_value` was `#[cfg(test)]`, inherited from a host where
  it was test-only. It is a reasonable primitive - parse one JSON value as
  a tool call - so it is un-gated and documented rather than duplicated
  host-side.
- `build_registry` took `&Value`. A host tool trait that RETURNS a schema
  by value - the common shape - then has to collect into a temporary just
  to hand out references. It takes `Borrow<Value>` now, so both forms work.

Without these a host either shadows the code it just stopped owning, or
loses the tests that covered it. OpenHuman keeps all 62 of its parser
tests working against the crate.

1823 lib tests pass, no ignores. Clippy and fmt clean.

Co-authored-by: Medulla <medulla@tinyhumans.ai>

Author: senamakel

src/harness/tool_calling/mod.rs

@@ -41,7 +41,15 @@
 pub(crate) mod parse;
 pub(crate) mod pformat;
 
-pub use parse::{ParsedToolCall, parse_tool_calls, parse_tool_calls_with_pformat};
+// The two entry points, plus the building blocks a host legitimately reaches
+// for on its own. `extract_json_values` in particular is not a test helper:
+// pulling the first JSON object out of model prose is how a host checks a
+// required-output contract, which has nothing to do with tool calls.
+pub use parse::{
+    ParsedToolCall, extract_json_values, parse_arguments_value, parse_glm_style_tool_calls,
+    parse_tool_call_value, parse_tool_calls, parse_tool_calls_from_json_value,
+    parse_tool_calls_with_pformat,
+};
 pub use pformat::{
     PFormatParamType, PFormatRegistry, PFormatToolParams, build_registry, parse_call,
     render_signature, render_signature_from_schema,

src/harness/tool_calling/parse.rs

@@ -42,7 +42,13 @@ fn first_args_by_keys(obj: &serde_json::Value) -> serde_json::Value {
     parse_arguments_value(None)
 }
 
-#[cfg(test)]
+/// Parse a single JSON value as a tool call, honouring the argument-key
+/// aliases.
+///
+/// The permissive entry point: callers reach a value through an explicit
+/// tool-call marker (a `tool_calls` array, a `<tool_call>` tag, a fenced
+/// block), which is what licenses the aliases. Do not use it on arbitrary
+/// model output — see the module docs.
 pub fn parse_tool_call_value(value: &serde_json::Value) -> Option<ParsedToolCall> {
     // Default to the permissive (tagged) behaviour: callers that reach a
     // value through an explicit tool-call marker (`tool_calls` array,
@@ -1001,9 +1007,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 `<tool_call>` 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<ParsedToolCall> = 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()..];

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!(
+        "<tool_call>echo[hello]</tool_call>\n",
+        "<tool_call>{\"name\": \"shell\", \"arguments\": {\"command\": \"cat a/b>c\"}}</tool_call>"
+    );
+    let (_narrative, calls) = parse_tool_calls_with_pformat(response, &reg);
+    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::<Vec<_>>()
+    );
+}
+
+/// A tagged body may use the argument-key aliases.
+///
+/// A `<tool_call>` 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!(
+        "<tool_call>echo[hello]</tool_call>\n",
+        "<tool_call>{\"name\": \"shell\", \"args\": {\"command\": \"ls\"}}</tool_call>"
+    );
+    let (_narrative, calls) = parse_tool_calls_with_pformat(response, &reg);
+    let shell = calls
+        .iter()
+        .find(|c| c.name == "shell")
+        .expect("the aliased tagged call must survive");
+    assert_eq!(shell.arguments["command"], "ls");
+}

src/harness/tool_calling/pformat.rs

@@ -150,14 +150,19 @@ pub type PFormatRegistry = HashMap<String, PFormatToolParams>;
 /// type is its own vocabulary, and requiring it here would make this module
 /// depend on the very thing it exists to stay independent of. Hosts keep a
 /// one-line adapter over their own tool slice.
-pub fn build_registry<'a, I, N>(tools: I) -> PFormatRegistry
+///
+/// The schema is `Borrow<Value>` rather than `&Value` so a host whose tool
+/// trait *returns* a schema by value — the common shape — can map straight
+/// into this without collecting into a temporary first.
+pub fn build_registry<I, N, S>(tools: I) -> PFormatRegistry
 where
-    I: IntoIterator<Item = (N, &'a Value)>,
+    I: IntoIterator<Item = (N, S)>,
     N: Into<String>,
+    S: std::borrow::Borrow<Value>,
 {
     tools
         .into_iter()
-        .map(|(name, schema)| (name.into(), PFormatToolParams::from_schema(schema)))
+        .map(|(name, schema)| (name.into(), PFormatToolParams::from_schema(schema.borrow())))
         .collect()
 }