diff --git a/nanobot/agent/context_governance.py b/nanobot/agent/context_governance.py index b2e1249f..838982b2 100644 --- a/nanobot/agent/context_governance.py +++ b/nanobot/agent/context_governance.py @@ -232,8 +232,9 @@ class ContextGovernor: def drop_orphan_tool_results( messages: list[dict[str, Any]], ) -> list[dict[str, Any]]: - """Drop tool results that have no matching assistant tool_call earlier in history.""" + """Drop invalid tool results before history is sent back to providers.""" declared: set[str] = set() + fulfilled: set[str] = set() updated: list[dict[str, Any]] | None = None for idx, msg in enumerate(messages): role = msg.get("role") @@ -243,10 +244,12 @@ class ContextGovernor: declared.add(str(tc["id"])) if role == "tool": tid = msg.get("tool_call_id") - if tid and str(tid) not in declared: + tid_str = str(tid) if tid else "" + if not tid_str or tid_str not in declared or tid_str in fulfilled: if updated is None: updated = [dict(m) for m in messages[:idx]] continue + fulfilled.add(tid_str) if updated is not None: updated.append(dict(msg)) diff --git a/nanobot/agent/loop.py b/nanobot/agent/loop.py index 9df71670..b265422e 100644 --- a/nanobot/agent/loop.py +++ b/nanobot/agent/loop.py @@ -1753,6 +1753,11 @@ class AgentLoop: for tc in m.get("tool_calls") or [] if isinstance(tc, dict) and tc.get("id") } + fulfilled_tool_call_ids = { + str(m["tool_call_id"]) + for m in session.messages + if m.get("role") == "tool" and m.get("tool_call_id") + } last_assistant_idx: int | None = None for m in messages[skip:]: entry = dict(m) @@ -1767,14 +1772,20 @@ class AgentLoop: continue # skip empty assistant messages — they poison session context if role == "tool": tool_call_id = entry.get("tool_call_id") - if not tool_call_id or str(tool_call_id) not in declared_tool_call_ids: + tool_call_id_str = str(tool_call_id) if tool_call_id else "" + if ( + not tool_call_id_str + or tool_call_id_str not in declared_tool_call_ids + or tool_call_id_str in fulfilled_tool_call_ids + ): # Undeclared tool results corrupt future provider requests. logger.warning( - "Dropping orphaned tool result {} from session {} during persistence", - tool_call_id or "(missing id)", + "Dropping invalid tool result {} from session {} during persistence", + tool_call_id_str or "(missing id)", session.key, ) continue + fulfilled_tool_call_ids.add(tool_call_id_str) if isinstance(content, str) and len(content) > self.max_tool_result_chars: entry["content"] = truncate_text_fn(content, self.max_tool_result_chars) elif isinstance(content, list): diff --git a/tests/agent/test_context_governance.py b/tests/agent/test_context_governance.py new file mode 100644 index 00000000..14daef17 --- /dev/null +++ b/tests/agent/test_context_governance.py @@ -0,0 +1,39 @@ +from nanobot.agent.context_governance import ContextGovernor + + +def _assistant_tool_call(call_id: str) -> dict: + return { + "role": "assistant", + "content": "", + "tool_calls": [{ + "id": call_id, + "type": "function", + "function": {"name": "exec", "arguments": "{}"}, + }], + } + + +def test_drop_orphan_tool_results_drops_missing_tool_call_id() -> None: + messages = [ + _assistant_tool_call("call_1"), + {"role": "tool", "name": "exec", "content": "missing id"}, + {"role": "tool", "tool_call_id": "call_1", "name": "exec", "content": "ok"}, + ] + + result = ContextGovernor.drop_orphan_tool_results(messages) + + assert [m.get("tool_call_id") for m in result if m.get("role") == "tool"] == ["call_1"] + + +def test_drop_orphan_tool_results_drops_duplicate_tool_result() -> None: + messages = [ + _assistant_tool_call("call_1"), + {"role": "tool", "tool_call_id": "call_1", "name": "exec", "content": "first"}, + {"role": "tool", "tool_call_id": "call_1", "name": "exec", "content": "duplicate"}, + ] + + result = ContextGovernor.drop_orphan_tool_results(messages) + + tool_results = [m for m in result if m.get("role") == "tool"] + assert len(tool_results) == 1 + assert tool_results[0]["content"] == "first" diff --git a/tests/agent/test_loop_save_turn.py b/tests/agent/test_loop_save_turn.py index 39e7ff2c..b09d8324 100644 --- a/tests/agent/test_loop_save_turn.py +++ b/tests/agent/test_loop_save_turn.py @@ -1870,3 +1870,58 @@ def test_save_turn_keeps_tool_results_declared_in_prior_history() -> None: ) assert [m["role"] for m in session.messages] == ["assistant", "tool"] + + +def test_save_turn_drops_tool_result_already_fulfilled_in_history() -> None: + loop = _mk_loop() + session = Session(key="test:prior-fulfilled") + session.add_message( + "assistant", + "", + tool_calls=[{ + "id": "call_prior", + "type": "function", + "function": {"name": "exec", "arguments": "{}"}, + }], + ) + session.add_message( + "tool", + "first", + tool_call_id="call_prior", + name="exec", + ) + + loop._save_turn( + session, + [{"role": "tool", "tool_call_id": "call_prior", "name": "exec", "content": "duplicate"}], + skip=0, + ) + + assert [m["role"] for m in session.messages] == ["assistant", "tool"] + assert session.messages[1]["content"] == "first" + + +def test_save_turn_drops_duplicate_tool_result_ids() -> None: + loop = _mk_loop() + session = Session(key="test:duplicate-tool-result") + + loop._save_turn( + session, + [ + { + "role": "assistant", + "content": "", + "tool_calls": [{ + "id": "call_dupe", + "type": "function", + "function": {"name": "exec", "arguments": "{}"}, + }], + }, + {"role": "tool", "tool_call_id": "call_dupe", "name": "exec", "content": "first"}, + {"role": "tool", "tool_call_id": "call_dupe", "name": "exec", "content": "second"}, + ], + skip=0, + ) + + assert [m["role"] for m in session.messages] == ["assistant", "tool"] + assert session.messages[1]["content"] == "first"