From ca6f9914964ccfcf6ffb8746aa6651d4fcd7c43f Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Sun, 9 Aug 2026 20:25:53 -0700 Subject: [PATCH] fix(repair): Pass 1.5 must match tool results on call_id, not just id `_repair_message_sequence` Pass 1.5 resolved a tool_call's answered budget against `tc.get("id")` ONLY, while Pass 1 correctly registers the `id`/`call_id` SUPERSET (#58168). An assistant turn whose tool_call carries only `call_id` (or a Codex-Responses call whose `id` and `call_id` differ) therefore read as entirely unanswered even when the following `tool` message answered it -- and the "none answered" branch DELETED the valid assistant turn. Repro on fork/main: [user, assistant(tool_calls=[{call_id: "call_XYZ"}]), tool(tool_call_id="call_XYZ"), user] repairs -> 1 (expected 0); roles -> ['user','tool','user'] That leaves a stray tool result with no preceding tool_calls -- the exact HTTP 400 shape this pass exists to prevent. Fix: iterate ("id", "call_id") when consuming the answered budget, mirroring Pass 1. Also removes the two `@pytest.mark.xfail(strict=False)` markers that were parked on the upstream tests pending this fix, so they become real gates. --- agent/agent_runtime_helpers.py | 24 +++++-- .../run_agent/test_message_sequence_repair.py | 69 ++++++++----------- 2 files changed, 48 insertions(+), 45 deletions(-) diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index ad22c0db31f23..a160a9625e5d7 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -824,12 +824,24 @@ def _is_verification_candidate(m: Dict) -> bool: _remaining = dict(answered_counts) answered_flags = [] # parallel to ``calls``: True = this entry answered for tc in calls: - _cid = tc.get("id") if isinstance(tc, dict) else None - if _cid and _remaining.get(_cid, 0) > 0: - _remaining[_cid] -= 1 - answered_flags.append(True) - else: - answered_flags.append(False) + # Match on the SUPERSET of ``id`` and ``call_id`` -- the same + # tolerance Pass 1 applies when registering the answer budget + # (#58168). Resolving against ``id`` ALONE made a tool_call + # carrying only ``call_id`` (or a Codex-Responses call whose + # ``id`` and ``call_id`` differ) read as unanswered, so the + # "none answered" branch below DELETED an assistant turn that + # was in fact answered by the following ``tool`` message -- + # leaving a stray tool result and the very 400 this pass + # exists to prevent. + _matched = False + if isinstance(tc, dict): + for _key in ("id", "call_id"): + _cid = tc.get(_key) + if _cid and _remaining.get(_cid, 0) > 0: + _remaining[_cid] -= 1 + _matched = True + break + answered_flags.append(_matched) unanswered_calls = [ tc for tc, ok in zip(calls, answered_flags) if not ok ] diff --git a/tests/run_agent/test_message_sequence_repair.py b/tests/run_agent/test_message_sequence_repair.py index 8aa7baf15c415..42059af0e204f 100644 --- a/tests/run_agent/test_message_sequence_repair.py +++ b/tests/run_agent/test_message_sequence_repair.py @@ -9,7 +9,6 @@ recovery every turn. """ -import pytest from run_agent import AIAgent @@ -108,25 +107,6 @@ def test_repair_drops_stray_tool_with_unknown_tool_call_id(): assert all(m.get("role") != "tool" for m in messages) -@pytest.mark.xfail( - reason=( - "INHERITED fork/main defect, NOT parity-merge damage -- see card t_08cca32f. " - "Pass 1.5 of repair_message_sequence (agent/agent_runtime_helpers.py, " - "fork-only code from PR #196 / aaa34311e0) resolves a tool_call's answered " - "budget against tc.get('id') ONLY, while Pass 1 correctly matches the " - "id||call_id superset per PR #58168. A tool_call carrying only call_id (or a " - "Codex-Responses call whose id and call_id differ) therefore reads as " - "unanswered, and the 'none answered' branch deletes an assistant turn that " - "WAS answered. Evidence: reproduced identically on a clean detached worktree " - "at fork/main ee2fce2876 (repairs=1, expected 0); the merge's diff over the " - "Pass 1.5 region is empty; 'Pass 1.5' appears 6x at fork/main HEAD and 0x at " - "both merge-base a7a696ba and upstream target 1e5b5074. These tests exist at " - "the merge base and upstream but were dropped from fork/main -- the merge " - "restored them, which is how the bug surfaced. strict=False so this xpasses " - "and flags itself for deletion once t_08cca32f lands." - ), - strict=False, -) 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 @@ -157,25 +137,6 @@ def test_repair_keeps_tool_matching_codex_call_id(): assert messages[2]["tool_call_id"] == "call_ABC" -@pytest.mark.xfail( - reason=( - "INHERITED fork/main defect, NOT parity-merge damage -- see card t_08cca32f. " - "Pass 1.5 of repair_message_sequence (agent/agent_runtime_helpers.py, " - "fork-only code from PR #196 / aaa34311e0) resolves a tool_call's answered " - "budget against tc.get('id') ONLY, while Pass 1 correctly matches the " - "id||call_id superset per PR #58168. A tool_call carrying only call_id (or a " - "Codex-Responses call whose id and call_id differ) therefore reads as " - "unanswered, and the 'none answered' branch deletes an assistant turn that " - "WAS answered. Evidence: reproduced identically on a clean detached worktree " - "at fork/main ee2fce2876 (repairs=1, expected 0); the merge's diff over the " - "Pass 1.5 region is empty; 'Pass 1.5' appears 6x at fork/main HEAD and 0x at " - "both merge-base a7a696ba and upstream target 1e5b5074. These tests exist at " - "the merge base and upstream but were dropped from fork/main -- the merge " - "restored them, which is how the bug surfaced. strict=False so this xpasses " - "and flags itself for deletion once t_08cca32f lands." - ), - strict=False, -) 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). @@ -196,6 +157,36 @@ def test_repair_keeps_tool_matching_only_call_id(): assert any(m.get("role") == "tool" for m in messages) +def test_repair_pass_1_5_keeps_call_id_only_assistant_turn(): + """Pass 1.5 must resolve a tool_call's answered budget on ``call_id`` OR + ``id``, matching Pass 1's ``id``/``call_id`` superset (#58168). + + Regression guard: Pass 1.5 previously read only ``tc.get("id")``, so an + assistant turn whose tool_call carried ONLY ``call_id`` read as entirely + unanswered even though the following ``tool`` message answered it. The + "none answered" branch then DELETED the valid assistant turn, producing + ``repairs=1`` and a history of ``[user, tool, user]`` -- a stray tool + message with no preceding tool_calls, i.e. the exact HTTP 400 shape the + repair exists to prevent. + """ + 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 [m["role"] for m in messages] == ["user", "assistant", "tool", "user"] + # the assistant turn survives INTACT -- its tool_calls are not stripped + assert messages[1]["tool_calls"][0]["call_id"] == "call_XYZ" + +