diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index 08c0052bc59..648b73df1be 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -306,7 +306,13 @@ def sanitize_tool_call_arguments( try: json.loads(arguments) except json.JSONDecodeError: - tool_call_id = tool_call.get("id") + # Use the canonical ``call_id || id`` precedence so both the + # scan for an existing tool result and any inserted stub key + # on the same id the rest of the pipeline uses. Keying on bare + # ``id`` here would fail to find a result built with ``call_id`` + # (Codex Responses format) and insert a duplicate stub that + # itself becomes an orphan (#58168). + tool_call_id = _ra().AIAgent._get_tool_call_id_static(tool_call) or None function_name = function.get("name", "?") preview = arguments[:80] log.warning( @@ -464,6 +470,21 @@ def repair_message_sequence(agent, messages: List[Dict]) -> int: # Pass 1: drop stray tool messages that don't follow a known # assistant tool_call_id. Uses a rolling set of known ids refreshed # on each assistant message. + # + # Both ``id`` AND ``call_id`` are registered for every assistant + # tool_call. In the Codex Responses API format the two differ + # (``id`` = ``fc_...`` response-item id, ``call_id`` = ``call_...`` + # the function-call id), and a tool result's ``tool_call_id`` may be + # matched against *either* depending on which code path built it + # (the OpenAI-compatible path stores ``tc.id``; codex paths store + # ``call_id``). Registering only ``id`` — as this pass did before — + # made a valid tool result look orphaned whenever the assistant + # tool_call carried a distinct ``call_id`` (or only ``call_id``); the + # pass then dropped it, leaving the assistant tool_call unanswered and + # producing an HTTP 400 on strict providers (DeepSeek, Kimi). Matching + # on the *superset* of both keys achieves the same tolerance as + # ``_get_tool_call_id_static``'s ``call_id || id`` — a match set must + # accept every legitimate reference, not just the canonical one (#58168). known_tool_ids: set = set() filtered: List[Dict] = [] for msg in collapsed: @@ -474,9 +495,12 @@ def repair_message_sequence(agent, messages: List[Dict]) -> int: if role == "assistant": known_tool_ids = set() for tc in (msg.get("tool_calls") or []): - tc_id = tc.get("id") if isinstance(tc, dict) else None - if tc_id: - known_tool_ids.add(tc_id) + if not isinstance(tc, dict): + continue + for key in ("id", "call_id"): + tc_id = tc.get(key) + if tc_id: + known_tool_ids.add(tc_id) filtered.append(msg) elif role == "tool": tc_id = msg.get("tool_call_id") diff --git a/tests/run_agent/test_message_sequence_repair.py b/tests/run_agent/test_message_sequence_repair.py index 93e65193756..ddfe1ba4d9b 100644 --- a/tests/run_agent/test_message_sequence_repair.py +++ b/tests/run_agent/test_message_sequence_repair.py @@ -143,6 +143,103 @@ def test_repair_drops_stray_tool_with_unknown_tool_call_id(): assert all(m.get("role") != "tool" for m in messages) +def test_repair_keeps_tool_matching_codex_call_id(): + """A valid tool result must survive when the assistant tool_call carries a + Codex-format ``call_id`` distinct from ``id`` and the result matches on + ``call_id`` (#58168). + + Before the fix, Pass 1 registered only ``tc.get("id")`` (``fc_...``) in the + known-id set, so a result keyed on ``call_id`` (``call_...``) looked + orphaned and was dropped -- leaving the assistant tool_call unanswered and + triggering an HTTP 400 on strict providers (DeepSeek, Kimi): + "Messages with role 'tool' must be a response to a preceding message with + 'tool_calls'". + """ + agent = _bare_agent() + messages = [ + {"role": "user", "content": "do it"}, + {"role": "assistant", "content": "", + "tool_calls": [{"id": "fc_123", "call_id": "call_ABC", + "type": "function", + "function": {"name": "x", "arguments": "{}"}}]}, + {"role": "tool", "tool_call_id": "call_ABC", "content": "result"}, + {"role": "user", "content": "next"}, + ] + + repairs = AIAgent._repair_message_sequence(agent, messages) + + assert repairs == 0 + assert [m["role"] for m in messages] == ["user", "assistant", "tool", "user"] + assert messages[2]["tool_call_id"] == "call_ABC" + + +def test_repair_keeps_tool_matching_only_call_id(): + """Same as above but the assistant tool_call carries ONLY ``call_id`` (no + ``id``). The result keyed on ``call_id`` must still be recognized (#58168). + """ + agent = _bare_agent() + messages = [ + {"role": "user", "content": "do it"}, + {"role": "assistant", "content": "", + "tool_calls": [{"call_id": "call_XYZ", "type": "function", + "function": {"name": "x", "arguments": "{}"}}]}, + {"role": "tool", "tool_call_id": "call_XYZ", "content": "result"}, + {"role": "user", "content": "next"}, + ] + + repairs = AIAgent._repair_message_sequence(agent, messages) + + assert repairs == 0 + assert any(m.get("role") == "tool" for m in messages) + + +def test_repair_keeps_tool_matching_id_when_call_id_also_present(): + """When the assistant tool_call carries both ``id`` and ``call_id`` and the + result matches on ``id`` (OpenAI-compatible builder path), it must be kept + (#58168 -- both keys are registered, so either matches). + """ + agent = _bare_agent() + messages = [ + {"role": "user", "content": "do it"}, + {"role": "assistant", "content": "", + "tool_calls": [{"id": "fc_9", "call_id": "call_9", + "type": "function", + "function": {"name": "x", "arguments": "{}"}}]}, + {"role": "tool", "tool_call_id": "fc_9", "content": "result"}, + {"role": "user", "content": "next"}, + ] + + repairs = AIAgent._repair_message_sequence(agent, messages) + + assert repairs == 0 + assert any(m.get("role") == "tool" for m in messages) + + +def test_repair_still_drops_genuine_orphan_alongside_codex_pair(): + """Negative control for #58168: registering both id and call_id must NOT + over-relax orphan detection. A genuinely orphaned tool result (matching + neither the id nor the call_id of any assistant tool_call) is still + dropped, while the valid codex-format pair in the same window survives. + """ + agent = _bare_agent() + messages = [ + {"role": "user", "content": "go"}, + {"role": "assistant", "content": "", + "tool_calls": [{"id": "fc_1", "call_id": "call_1", + "type": "function", + "function": {"name": "x", "arguments": "{}"}}]}, + {"role": "tool", "tool_call_id": "call_1", "content": "valid"}, + {"role": "tool", "tool_call_id": "call_ORPHAN", "content": "stray"}, + {"role": "user", "content": "next"}, + ] + + repairs = AIAgent._repair_message_sequence(agent, messages) + + assert repairs == 1 + tool_ids = [m["tool_call_id"] for m in messages if m.get("role") == "tool"] + assert tool_ids == ["call_1"] + + def test_repair_leaves_valid_conversation_unchanged(): agent = _bare_agent() messages = [