From 8248d075dbc3f4f9482fc424f0ddb3d2efefad54 Mon Sep 17 00:00:00 2001 From: wangjunwei Date: Sun, 28 Jun 2026 11:36:32 +0800 Subject: [PATCH] fix(agent): harden tool-call handling against malformed upstream relays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Combine malformed tool-call handling with placeholder filtering and a no-tools fallback so a relay that returns tool_use blocks with null id/name/input can no longer crash a turn or permanently wedge a session. Adapted to the ContextGovernor architecture (context governance now lives in nanobot/agent/context_governance.py, not runner.py): - ToolCallRequest.has_valid_name(): single source of truth for "usable name" (non-empty string). - tool_hints.format_tool_hints(): skip tool calls with a non-string/empty name instead of raising AttributeError on the whole turn. - ContextGovernor.strip_placeholder_assistant_messages() and strip_malformed_tool_calls() (plus the _tool_call_name_is_valid helper): history-cleaning staticmethods invoked at the START of prepare_for_model() — strip_placeholder, then strip_malformed, then the existing drop_orphan/backfill chain. Both only repair the model-facing copy and leave persisted history untouched (return a copy, or the same list when nothing changes). Also wired into runner's minimal-repair path. - AgentRunner._drop_malformed_tool_calls(): returns (dropped, all_dropped, original_finish_reason); clears finish_reason to "stop" when all calls are dropped. - AgentRunner._malformed_tool_call_retry_messages() + _request_model malformed_retry flag: when an all-dropped tool_calls response comes back, retry once with a corrective note; if the retry STILL comes back all-dropped, fall back to _request_no_tools for graceful text degradation. Tests for the history-cleaning methods live with ContextGovernor in tests/agent/test_runner_governance.py; response-layer and tool-hint tests stay on AgentRunner / tool_hints. Co-Authored-By: Claude Opus 4.8 --- nanobot/agent/context_governance.py | 114 +++++++++++++++++- nanobot/agent/runner.py | 98 +++++++++++++++- nanobot/providers/base.py | 12 ++ nanobot/utils/tool_hints.py | 9 +- tests/agent/test_runner_governance.py | 161 +++++++++++++++++++++++++- tests/agent/test_tool_hint.py | 19 +++ 6 files changed, 408 insertions(+), 5 deletions(-) diff --git a/nanobot/agent/context_governance.py b/nanobot/agent/context_governance.py index 148b40e6..3622f73d 100644 --- a/nanobot/agent/context_governance.py +++ b/nanobot/agent/context_governance.py @@ -36,6 +36,23 @@ COMPACTABLE_TOOLS = frozenset({ # read_file is the recovery path for persisted results; exempting it prevents persist->read->persist loops. TOOL_RESULT_OFFLOAD_EXEMPT_TOOLS = frozenset({"read_file"}) BACKFILL_CONTENT = "[Tool result unavailable — call was interrupted or lost]" +PLACEHOLDER_TEXTS = frozenset({ + "[Previous assistant message omitted.]", +}) + + +def _tool_call_name_is_valid(tool_call: Any) -> bool: + """Whether a persisted OpenAI-style tool_call carries a usable name. + + Mirrors ``ToolCallRequest.has_valid_name`` for the dict shape stored in + message history: a degenerate call with ``name=None`` / ``""`` cannot be + executed and is rejected by upstream APIs if replayed. + """ + if not isinstance(tool_call, dict): + return False + fn = tool_call.get("function") + name = fn.get("name") if isinstance(fn, dict) else tool_call.get("name") + return isinstance(name, str) and bool(name) @dataclass(slots=True) @@ -61,7 +78,9 @@ class ContextGovernor: messages: list[dict[str, Any]], compacted_tool_call_ids: set[str], ) -> list[dict[str, Any]]: - updated = self.drop_orphan_tool_results(messages) + updated = self.strip_placeholder_assistant_messages(messages) + updated = self.strip_malformed_tool_calls(updated) + updated = self.drop_orphan_tool_results(updated) updated = self.backfill_missing_tool_results(updated) updated = self.apply_tool_result_budget(config, updated) updated = self.compact_inflight_overflow(config, updated, compacted_tool_call_ids) @@ -116,6 +135,99 @@ class ContextGovernor: return truncate_text(content, config.max_tool_result_chars) return content + @staticmethod + def strip_placeholder_assistant_messages( + messages: list[dict[str, Any]], + ) -> list[dict[str, Any]]: + """Remove assistant messages that are compaction placeholders. + + Messages like ``[Previous assistant message omitted.]`` carry no useful + context for the model and can cause it to repeatedly attempt tool calls + that previously failed, producing malformed responses in a loop. + Consecutive same-role messages that result from removal are handled + downstream by the provider's merge-consecutive logic. Only the + model-facing copy is repaired; the persisted transcript is untouched + (a copy is returned, or the same list object when nothing changes). + """ + updated: list[dict[str, Any]] | None = None + for idx, msg in enumerate(messages): + if msg.get("role") != "assistant": + if updated is not None: + updated.append(msg) + continue + content = msg.get("content", "") + text = content if isinstance(content, str) else "" + is_placeholder = text.strip() in PLACEHOLDER_TEXTS + has_tool_calls = bool(msg.get("tool_calls")) + if is_placeholder and not has_tool_calls: + if updated is None: + updated = list(messages[:idx]) + logger.debug( + "Stripping placeholder assistant message from history: {!r}", + text[:60], + ) + continue + if updated is not None: + updated.append(msg) + if updated is None: + return messages + return updated + + @staticmethod + def strip_malformed_tool_calls( + messages: list[dict[str, Any]], + ) -> list[dict[str, Any]]: + """Drop persisted assistant tool_calls whose name is missing/non-string. + + A degenerate tool call (``name=None`` or ``""``) that slipped into the + saved history before this guard existed gets replayed on every turn and + makes upstream APIs reject the whole request + (``messages.content.N.tool_use.name: Input should be a valid string``), + permanently wedging the session. Removing the bad call here lets the + existing orphan-result cleanup drop its now-dangling tool result, so a + polluted session self-heals on its next turn. The persisted transcript + is left untouched; only the model-facing copy is repaired (a copy is + returned, or the same list object when nothing changes). + """ + updated: list[dict[str, Any]] | None = None + for idx, msg in enumerate(messages): + if msg.get("role") != "assistant": + if updated is not None: + updated.append(msg) + continue + calls = msg.get("tool_calls") + if not calls: + if updated is not None: + updated.append(msg) + continue + kept = [tc for tc in calls if _tool_call_name_is_valid(tc)] + if len(kept) == len(calls): + if updated is not None: + updated.append(msg) + continue + if updated is None: + updated = [dict(m) for m in messages[:idx]] + logger.warning( + "Stripping {} malformed tool_call(s) with missing/non-string " + "name from assistant history before request", + len(calls) - len(kept), + ) + repaired = dict(msg) + if kept: + repaired["tool_calls"] = kept + else: + repaired.pop("tool_calls", None) + # An assistant turn with neither content nor any valid tool call is + # itself invalid upstream; drop it entirely in that case. + has_content = bool(repaired.get("content")) + if not kept and not has_content: + continue + updated.append(repaired) + + if updated is None: + return messages + return updated + @staticmethod def drop_orphan_tool_results( messages: list[dict[str, Any]], diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index 436a1a12..70740b0c 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -389,7 +389,15 @@ class AgentRunner: spec.session_key or "default", ) try: - messages_for_model = ContextGovernor.drop_orphan_tool_results(messages) + messages_for_model = ContextGovernor.strip_placeholder_assistant_messages( + messages + ) + messages_for_model = ContextGovernor.strip_malformed_tool_calls( + messages_for_model + ) + messages_for_model = ContextGovernor.drop_orphan_tool_results( + messages_for_model + ) messages_for_model = ContextGovernor.backfill_missing_tool_results( messages_for_model ) @@ -725,6 +733,8 @@ class AgentRunner: messages: list[dict[str, Any]], hook: AgentHook, context: AgentHookContext, + *, + malformed_retry: bool = False, ): timeout_s: float | None = spec.llm_timeout_s if timeout_s is None: @@ -867,8 +877,94 @@ class AgentRunner: ) if progress_state and progress_state.get("reasoning_open"): await hook.emit_reasoning_end() + dropped, all_dropped, original_finish_reason = ( + self._drop_malformed_tool_calls(response) + ) + if ( + all_dropped + and original_finish_reason in ("tool_calls", "function_call") + and not malformed_retry + ): + logger.warning( + "Retrying LLM request after all {} malformed tool call(s) were dropped", + dropped, + ) + retry_messages = self._malformed_tool_call_retry_messages( + messages, response.content, + ) + return await self._request_model( + spec, retry_messages, hook, context, + malformed_retry=True, + ) + if ( + all_dropped + and original_finish_reason in ("tool_calls", "function_call") + and malformed_retry + ): + logger.warning( + "Malformed tool calls persisted after retry; falling back to no-tools request", + ) + fallback_messages = self._malformed_tool_call_retry_messages( + messages, response.content, + ) + return await self._request_no_tools(spec, fallback_messages) return response + @staticmethod + def _drop_malformed_tool_calls( + response: LLMResponse, + ) -> tuple[int, bool, str | None]: + """Strip tool calls whose name is missing/non-string from the response. + + Returns (dropped_count, all_dropped, original_finish_reason). + + A degenerate call (name=None or "") cannot be executed, and if it were + persisted into the assistant message it would be replayed on every + subsequent turn, causing upstream validation errors + (``tool_use.name: Input should be a valid string``) that permanently + wedge the session. Dropping it here keeps it out of execution, the + assistant message, and the saved history in one place. + """ + calls = getattr(response, "tool_calls", None) + if not calls: + return (0, False, getattr(response, "finish_reason", None)) + valid = [tc for tc in calls if tc.has_valid_name()] + if len(valid) == len(calls): + return (0, False, getattr(response, "finish_reason", None)) + dropped = len(calls) - len(valid) + original_finish_reason = getattr(response, "finish_reason", None) + logger.warning( + "Dropped {} malformed tool call(s) with missing/non-string name " + "from LLM response (finish_reason={!r})", + dropped, + original_finish_reason, + ) + response.tool_calls = valid + if not valid: + response.finish_reason = "stop" + return (dropped, not valid, original_finish_reason) + + @staticmethod + def _malformed_tool_call_retry_messages( + messages: list[dict[str, Any]], + assistant_text: str | None, + ) -> list[dict[str, Any]]: + retry_messages = list(messages) + note = ( + "The previous model response attempted to call tools, but every tool call " + "was malformed: the tool_use blocks had missing/non-string name, id, or input. " + "Do not answer with a promise to use tools. Either call the required tools again " + "using valid tool names from the provided tool list and JSON object inputs, or give " + "a final answer only if no tool is required." + ) + if assistant_text: + note += ( + f"\n\nPrevious assistant text before the malformed calls:\n" + f"{assistant_text}" + ) + retry_messages.append({"role": "user", "content": note}) + return retry_messages + async def _request_finalization_retry( self, spec: AgentRunSpec, diff --git a/nanobot/providers/base.py b/nanobot/providers/base.py index b2a93ded..f7f6a6df 100644 --- a/nanobot/providers/base.py +++ b/nanobot/providers/base.py @@ -54,6 +54,18 @@ class ToolCallRequest: provider_specific_fields: dict[str, Any] | None = None function_provider_specific_fields: dict[str, Any] | None = None + def has_valid_name(self) -> bool: + """Whether this call carries a usable (non-empty string) tool name. + + ToolCallRequest.name is typed ``str`` but not enforced at runtime: a + model/gateway can emit a degenerate call with ``name=None`` or ``""``. + Such a call cannot be executed and, if persisted and replayed, makes + upstream APIs reject the whole request (e.g. Anthropic-style + ``messages.content.N.tool_use.name: Input should be a valid string``), + which permanently wedges the session. + """ + return isinstance(self.name, str) and bool(self.name) + def to_openai_tool_call(self) -> dict[str, Any]: """Serialize to an OpenAI-style tool_call payload.""" arguments = ( diff --git a/nanobot/utils/tool_hints.py b/nanobot/utils/tool_hints.py index 3a646070..ca4101ba 100644 --- a/nanobot/utils/tool_hints.py +++ b/nanobot/utils/tool_hints.py @@ -35,10 +35,15 @@ def format_tool_hints(tool_calls: list, max_length: int = 40) -> str: formatted = [] for tc in tool_calls: - fmt = _TOOL_FORMATS.get(tc.name) + name = getattr(tc, "name", None) + if not isinstance(name, str) or not name: + # Degenerate/malformed tool call (e.g. a model emits name=None); + # skip it instead of raising AttributeError on the whole turn. + continue + fmt = _TOOL_FORMATS.get(name) if fmt: formatted.append(_fmt_known(tc, fmt, max_length)) - elif tc.name.startswith("mcp_"): + elif name.startswith("mcp_"): formatted.append(_fmt_mcp(tc, max_length)) else: formatted.append(_fmt_fallback(tc, max_length)) diff --git a/tests/agent/test_runner_governance.py b/tests/agent/test_runner_governance.py index 01f1c210..a8f88d15 100644 --- a/tests/agent/test_runner_governance.py +++ b/tests/agent/test_runner_governance.py @@ -15,7 +15,7 @@ from nanobot.agent.context_governance import ( ) from nanobot.agent.runner import AgentRunSpec from nanobot.config.schema import AgentDefaults -from nanobot.providers.base import LLMResponse +from nanobot.providers.base import LLMResponse, ToolCallRequest _MAX_TOOL_RESULT_CHARS = AgentDefaults().max_tool_result_chars @@ -877,3 +877,162 @@ def test_snip_history_no_user_at_all_falls_back_gracefully(monkeypatch): assert non_system[0]["role"] in ("user", "tool"), ( f"Safety net should ensure first non-system is user/tool, got {non_system[0]['role']}" ) + + +# --------------------------------------------------------------------------- +# Malformed tool_call name guard (missing/non-string name wedges the session +# upstream: messages.content.N.tool_use.name: Input should be a valid string) +# --------------------------------------------------------------------------- + + +def test_drop_malformed_tool_calls_trims_response(): + """LLM response tool_calls with a missing/empty name are dropped in place.""" + from nanobot.agent.runner import AgentRunner + + response = LLMResponse( + content=None, + tool_calls=[ + ToolCallRequest(id="1", name=None, arguments={}), + ToolCallRequest(id="2", name="", arguments={}), + ToolCallRequest(id="3", name="read_file", arguments={}), + ], + finish_reason="tool_calls", + ) + dropped, all_dropped, orig = AgentRunner._drop_malformed_tool_calls(response) + assert [tc.name for tc in response.tool_calls] == ["read_file"] + assert response.finish_reason == "tool_calls" + assert response.should_execute_tools is True + assert dropped == 2 + assert all_dropped is False + assert orig == "tool_calls" + + +def test_drop_malformed_tool_calls_all_bad_disables_execution(): + """If every tool call is malformed, execution is disabled (no empty exec).""" + from nanobot.agent.runner import AgentRunner + + response = LLMResponse( + content="some text", + tool_calls=[ToolCallRequest(id="1", name=None, arguments={})], + finish_reason="tool_calls", + ) + dropped, all_dropped, orig = AgentRunner._drop_malformed_tool_calls(response) + assert response.tool_calls == [] + assert response.finish_reason == "stop" + assert response.should_execute_tools is False + assert dropped == 1 + assert all_dropped is True + assert orig == "tool_calls" + + +def test_drop_malformed_returns_tuple_no_calls(): + """No tool calls returns (0, False, current_finish_reason).""" + from nanobot.agent.runner import AgentRunner + + response = LLMResponse(content="hi", finish_reason="stop") + dropped, all_dropped, orig = AgentRunner._drop_malformed_tool_calls(response) + assert dropped == 0 + assert all_dropped is False + assert orig == "stop" + + +def test_strip_malformed_tool_calls_keeps_valid_calls_in_history(): + """A mixed assistant turn keeps only its valid tool_calls.""" + messages = [ + {"role": "user", "content": "hi"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + {"id": "bad", "type": "function", "function": {"name": None, "arguments": "{}"}}, + {"id": "ok", "type": "function", "function": {"name": "exec", "arguments": "{}"}}, + ], + }, + {"role": "tool", "tool_call_id": "ok", "name": "exec", "content": "done"}, + ] + result = ContextGovernor.strip_malformed_tool_calls(messages) + assert result is not messages # copied, original untouched + assert len(messages[1]["tool_calls"]) == 2 # original preserved + kept = result[1]["tool_calls"] + assert [tc["function"]["name"] for tc in kept] == ["exec"] + + +def test_strip_malformed_tool_calls_drops_empty_assistant_turn(): + """An assistant turn that is only a malformed call is removed entirely; + the existing orphan-result cleanup then drops its dangling tool result, + so a polluted session self-heals.""" + messages = [ + {"role": "user", "content": "hi"}, + { + "role": "assistant", + "content": None, + "tool_calls": [ + {"id": "bad", "type": "function", "function": {"name": None, "arguments": "{}"}}, + ], + }, + {"role": "tool", "tool_call_id": "bad", "name": "", "content": "r"}, + ] + stripped = ContextGovernor.strip_malformed_tool_calls(messages) + assert [m["role"] for m in stripped] == ["user", "tool"] + healed = ContextGovernor.drop_orphan_tool_results(stripped) + assert [m["role"] for m in healed] == ["user"] + + +def test_strip_malformed_tool_calls_noop_when_clean(): + """Clean history is returned unchanged (same object).""" + messages = [ + {"role": "user", "content": "hi"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + {"id": "ok", "type": "function", "function": {"name": "exec", "arguments": "{}"}}, + ], + }, + {"role": "tool", "tool_call_id": "ok", "name": "exec", "content": "done"}, + ] + assert ContextGovernor.strip_malformed_tool_calls(messages) is messages + + +def test_strip_placeholder_assistant_messages_removes_omitted(): + """Placeholder assistant messages are removed; real messages kept.""" + messages = [ + {"role": "user", "content": "hi"}, + {"role": "assistant", "content": "real response"}, + {"role": "user", "content": "ok"}, + {"role": "assistant", "content": "[Previous assistant message omitted.]"}, + {"role": "user", "content": "?"}, + {"role": "assistant", "content": "[Previous assistant message omitted.]"}, + {"role": "user", "content": "hello"}, + ] + result = ContextGovernor.strip_placeholder_assistant_messages(messages) + assert [m["role"] for m in result] == [ + "user", "assistant", "user", "user", "user", + ] + assert result[1]["content"] == "real response" + + +def test_strip_placeholder_noop_when_clean(): + """Clean history is returned unchanged (same object).""" + messages = [ + {"role": "user", "content": "hi"}, + {"role": "assistant", "content": "hello back"}, + ] + assert ContextGovernor.strip_placeholder_assistant_messages(messages) is messages + + +def test_strip_placeholder_keeps_assistant_with_tool_calls(): + """A placeholder assistant that also carries tool_calls is kept.""" + messages = [ + {"role": "user", "content": "hi"}, + { + "role": "assistant", + "content": "[Previous assistant message omitted.]", + "tool_calls": [ + {"id": "1", "type": "function", "function": {"name": "exec", "arguments": "{}"}}, + ], + }, + {"role": "tool", "tool_call_id": "1", "name": "exec", "content": "done"}, + ] + result = ContextGovernor.strip_placeholder_assistant_messages(messages) + assert result is messages diff --git a/tests/agent/test_tool_hint.py b/tests/agent/test_tool_hint.py index 6e3bdb03..6aeed400 100644 --- a/tests/agent/test_tool_hint.py +++ b/tests/agent/test_tool_hint.py @@ -306,3 +306,22 @@ class TestToolHintMaxLength: short = _hint([_tc("list_dir", {"path": long_path})], max_length=40) long = _hint([_tc("list_dir", {"path": long_path})], max_length=120) assert len(long) > len(short) + + +class TestToolHintMalformedCalls: + """Malformed tool calls must not crash hint formatting (see HKUDS/nanobot).""" + + def test_none_name_is_skipped(self): + """A tool call with name=None should be skipped, not raise AttributeError.""" + result = _hint([_tc(None, None)]) + assert result == "" + + def test_empty_name_is_skipped(self): + """A tool call with an empty name should be skipped.""" + result = _hint([_tc("", {"path": "foo.txt"})]) + assert result == "" + + def test_none_name_mixed_with_valid_call(self): + """A degenerate call must not suppress hints for the valid calls beside it.""" + result = _hint([_tc(None, None), _tc("read_file", {"path": "foo.txt"})]) + assert result == "read foo.txt"