fix: quarantine invalid tool results
This commit is contained in:
@@ -232,8 +232,9 @@ class ContextGovernor:
|
|||||||
def drop_orphan_tool_results(
|
def drop_orphan_tool_results(
|
||||||
messages: list[dict[str, Any]],
|
messages: list[dict[str, Any]],
|
||||||
) -> 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()
|
declared: set[str] = set()
|
||||||
|
fulfilled: set[str] = set()
|
||||||
updated: list[dict[str, Any]] | None = None
|
updated: list[dict[str, Any]] | None = None
|
||||||
for idx, msg in enumerate(messages):
|
for idx, msg in enumerate(messages):
|
||||||
role = msg.get("role")
|
role = msg.get("role")
|
||||||
@@ -243,10 +244,12 @@ class ContextGovernor:
|
|||||||
declared.add(str(tc["id"]))
|
declared.add(str(tc["id"]))
|
||||||
if role == "tool":
|
if role == "tool":
|
||||||
tid = msg.get("tool_call_id")
|
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:
|
if updated is None:
|
||||||
updated = [dict(m) for m in messages[:idx]]
|
updated = [dict(m) for m in messages[:idx]]
|
||||||
continue
|
continue
|
||||||
|
fulfilled.add(tid_str)
|
||||||
if updated is not None:
|
if updated is not None:
|
||||||
updated.append(dict(msg))
|
updated.append(dict(msg))
|
||||||
|
|
||||||
|
|||||||
+14
-3
@@ -1753,6 +1753,11 @@ class AgentLoop:
|
|||||||
for tc in m.get("tool_calls") or []
|
for tc in m.get("tool_calls") or []
|
||||||
if isinstance(tc, dict) and tc.get("id")
|
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
|
last_assistant_idx: int | None = None
|
||||||
for m in messages[skip:]:
|
for m in messages[skip:]:
|
||||||
entry = dict(m)
|
entry = dict(m)
|
||||||
@@ -1767,14 +1772,20 @@ class AgentLoop:
|
|||||||
continue # skip empty assistant messages — they poison session context
|
continue # skip empty assistant messages — they poison session context
|
||||||
if role == "tool":
|
if role == "tool":
|
||||||
tool_call_id = entry.get("tool_call_id")
|
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.
|
# Undeclared tool results corrupt future provider requests.
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"Dropping orphaned tool result {} from session {} during persistence",
|
"Dropping invalid tool result {} from session {} during persistence",
|
||||||
tool_call_id or "(missing id)",
|
tool_call_id_str or "(missing id)",
|
||||||
session.key,
|
session.key,
|
||||||
)
|
)
|
||||||
continue
|
continue
|
||||||
|
fulfilled_tool_call_ids.add(tool_call_id_str)
|
||||||
if isinstance(content, str) and len(content) > self.max_tool_result_chars:
|
if isinstance(content, str) and len(content) > self.max_tool_result_chars:
|
||||||
entry["content"] = truncate_text_fn(content, self.max_tool_result_chars)
|
entry["content"] = truncate_text_fn(content, self.max_tool_result_chars)
|
||||||
elif isinstance(content, list):
|
elif isinstance(content, list):
|
||||||
|
|||||||
@@ -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"
|
||||||
@@ -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"]
|
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"
|
||||||
|
|||||||
Reference in New Issue
Block a user