From a2b6f6a187f3e19e20f7c6f48220bd3d9be49fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Fern=C3=A1ndez?= Date: Mon, 4 May 2026 21:42:34 +0200 Subject: [PATCH] fix(agent): recover gemma4 tool_call.arguments corrupted by leaked chat-template markers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When using Gemma 4 served via vLLM (Nous Builders, NaN Builders, others), the harmony parser intermittently structures the tool call cleanly (so ``tool_calls`` is non-empty and ``finish_reason='tool_calls'`` from the upstream) but lets the model's chat-template quote markers (``<|"|>`` in its full and partial forms) escape into ``tool_call.arguments`` as literal characters. Hermes' downstream ``json.loads(arguments)`` then fails. The retry sees the same deterministic corruption, fails again, and the run is surfaced to the user as ``Response truncated (finish_reason='length') — model hit max output tokens`` even though no token cap was hit and the upstream returned successfully. This is on top of the existing content-side recovery path (the structured channel was assumed safe). Add a conservative sanitizer that strips the leaked markers from structured ``tool_call.arguments``. To avoid masking unrelated bugs the sanitizer only rewrites ``arguments`` when both: * the ORIGINAL is unparseable as JSON, and * the CLEANED variant IS parseable as JSON. Already-valid args are never touched, even if they cosmetically contain ``<|`` substrings (legitimate user content can do this). Validated against captured failing payloads from a gemma4 + NaN Builders session (``kanban_complete`` with nested ``metadata.findings``): 12 already-valid tool_calls untouched, 2 broken tool_calls recovered, 0 collateral damage. --- agent/transports/chat_completions.py | 42 ++++++++ .../agent/transports/test_chat_completions.py | 102 ++++++++++++++++++ 2 files changed, 144 insertions(+) diff --git a/agent/transports/chat_completions.py b/agent/transports/chat_completions.py index 9a115e4547316..c612c5ed3f2ad 100644 --- a/agent/transports/chat_completions.py +++ b/agent/transports/chat_completions.py @@ -462,6 +462,48 @@ def normalize_response(self, response: Any, **kwargs) -> NormalizedResponse: provider_data=tc_provider_data or None, )) + # Recover from a Gemma 4 chat-template leak: vLLM's harmony parser + # for Gemma 4 sometimes structures the tool call (so ``tool_calls`` + # is populated) but lets the model's quote markers (``<|"|>`` in + # full and partial forms) escape into ``tool_call.arguments`` as + # literal characters. Hermes' downstream ``json.loads(arguments)`` + # then fails and the run gets surfaced as + # ``Response truncated (finish_reason='length')`` even though the + # upstream actually returned ``finish_reason: 'tool_calls'`` + # cleanly with no token cap hit. Strip the leaked markers so the + # JSON parses again, but only when the original was unparseable + # AND the cleaned version IS parseable — never silently mutate + # already-valid arguments. + if tool_calls: + import json as _json_for_validation + + def _strip_gemma4_quote_markers(s: str) -> str: + if not s or "<|" not in s: + return s + return (s + .replace('"<|\\"|"', '"') # full open: "<|\"|" -> " + .replace('<|\\"', '"') # full close: <|\" -> " + .replace('"<|', '"') # bare open: "<| -> " + .replace('<|"', '"')) # bare close: <|" -> " + + for tc in tool_calls: + args_str = tc.arguments + if not args_str or "<|" not in args_str: + continue + try: + _json_for_validation.loads(args_str) + continue # already valid + except (ValueError, TypeError): + pass + cleaned = _strip_gemma4_quote_markers(args_str) + if cleaned == args_str: + continue + try: + _json_for_validation.loads(cleaned) + except (ValueError, TypeError): + continue # cleaned still broken, leave original + tc.arguments = cleaned + usage = None if hasattr(response, "usage") and response.usage: u = response.usage diff --git a/tests/agent/transports/test_chat_completions.py b/tests/agent/transports/test_chat_completions.py index b8fdced8aa6cc..fc6ac26132694 100644 --- a/tests/agent/transports/test_chat_completions.py +++ b/tests/agent/transports/test_chat_completions.py @@ -656,6 +656,108 @@ def test_reasoning_content_preserved_from_model_extra(self, transport): assert nr.provider_data == {"reasoning_content": "model-extra scratchpad"} +class TestChatCompletionsGemma4ArgsSanitization: + """Recovery from Gemma 4 chat-template markers leaking into structured + tool_call.arguments (the upstream emits ``finish_reason='tool_calls'`` + cleanly, but the JSON inside ``arguments`` is corrupted by ``<|"|>`` + and partial variants). Hermes' downstream ``json.loads`` would otherwise + fail and the run would be reported as length-truncated. + + The sanitizer is conservative: it only rewrites ``arguments`` when the + original is unparseable AND the cleaned variant IS parseable. + """ + + def _resp(self, args: str): + tc = SimpleNamespace( + id="call_x", + function=SimpleNamespace(name="terminal", arguments=args), + ) + return SimpleNamespace( + choices=[SimpleNamespace( + message=SimpleNamespace(content=None, tool_calls=[tc], reasoning_content=None), + finish_reason="tool_calls", + )], + usage=None, + ) + + def test_full_open_and_close_markers_stripped(self, transport): + # Real-world payload shape (kanban_complete with metadata.findings). + bad = ( + '{"metadata": {"findings": [{"date": "<|\\"|"2023-09-18<|\\", ' + '"identifier": "<|\\"|"BOE-A-2023-19616<|\\"}]}, ' + '"task_id": "t_8345a55c"}' + ) + nr = transport.normalize_response(self._resp(bad)) + import json + parsed = json.loads(nr.tool_calls[0].arguments) + assert parsed["task_id"] == "t_8345a55c" + assert parsed["metadata"]["findings"][0]["date"] == "2023-09-18" + assert parsed["metadata"]["findings"][0]["identifier"] == "BOE-A-2023-19616" + + def test_bare_marker_only_args_left_intact(self, transport): + # Bare ``<|`` markers without inner escaped quote do NOT break JSON + # parsing — the value is just an unusual literal string. The strict + # sanitizer must NOT touch already-valid args, even if they look + # cosmetically off; downstream callers may legitimately use ``<|``. + ok = '{"command": "<|grep -l Justicia<|"}' + import json + json.loads(ok) # confirm it parses fine before we hand it in + nr = transport.normalize_response(self._resp(ok)) + assert nr.tool_calls[0].arguments == ok + + def test_full_marker_with_bare_close_recovered(self, transport): + # Real-world variant: full ``<|"|>`` open marker (which DOES break + # JSON because the inner escaped quote terminates the string early) + # paired with a bare ``<|"`` close on the same value. + bad = '{"y": "<|\\"|"hello<|\\"}' + nr = transport.normalize_response(self._resp(bad)) + import json + parsed = json.loads(nr.tool_calls[0].arguments) + assert parsed["y"] == "hello" + + def test_already_valid_args_not_mutated(self, transport): + good = '{"command": "ls -la"}' + nr = transport.normalize_response(self._resp(good)) + # Identity preserved — no spurious rewrite of clean payloads. + assert nr.tool_calls[0].arguments == good + + def test_args_without_markers_not_inspected(self, transport): + # Does not contain "<|" — sanitizer skips entirely. Even if the JSON + # were broken for unrelated reasons, we leave it alone so downstream + # surfaces the real error. + broken_unrelated = '{"command": "ls",,}' + nr = transport.normalize_response(self._resp(broken_unrelated)) + assert nr.tool_calls[0].arguments == broken_unrelated + + def test_unrecoverable_args_left_for_downstream(self, transport): + # Marker present but the cleaned version is still not valid JSON. + # Sanitizer must NOT half-mutate — leave the original so the real + # downstream error is surfaced rather than masked. + bad = '{"command": "<|"}, "extra": <|>}' + nr = transport.normalize_response(self._resp(bad)) + assert nr.tool_calls[0].arguments == bad + + def test_multiple_tool_calls_each_handled_independently(self, transport): + clean_args = '{"x": 1}' + # Full marker open — actually breaks JSON parse, so sanitizer fires. + dirty_args = '{"y": "<|\\"|"hello<|\\"}' + tcs = [ + SimpleNamespace(id="a", function=SimpleNamespace(name="t1", arguments=clean_args)), + SimpleNamespace(id="b", function=SimpleNamespace(name="t2", arguments=dirty_args)), + ] + r = SimpleNamespace( + choices=[SimpleNamespace( + message=SimpleNamespace(content=None, tool_calls=tcs, reasoning_content=None), + finish_reason="tool_calls", + )], + usage=None, + ) + nr = transport.normalize_response(r) + import json + assert nr.tool_calls[0].arguments == clean_args + assert json.loads(nr.tool_calls[1].arguments) == {"y": "hello"} + + class TestChatCompletionsCacheStats: def test_no_usage(self, transport):