diff --git a/acp_adapter/events.py b/acp_adapter/events.py index b06a798c65..374d3fc50c 100644 --- a/acp_adapter/events.py +++ b/acp_adapter/events.py @@ -277,9 +277,13 @@ def make_step_cb( continue tc_id = queue.popleft() meta = tool_call_meta.pop(tc_id, {}) + # ``prev_tools`` carries the wire ``arguments`` JSON *string*; the content + # builders index it as a dict, so an uncoerced string raised inside this + # (swallowed) callback and the bubble never closed. _send_update(conn, session_id, loop, build_tool_complete( tc_id, tool_name, result=str(result) if result is not None else None, - function_args=function_args or meta.get("args"), snapshot=meta.get("snapshot"), + function_args=coerce_tool_args(function_args) if function_args else meta.get("args"), + snapshot=meta.get("snapshot"), )) if not queue: tool_call_ids.pop(tool_name, None) diff --git a/tests/acp_adapter/test_events.py b/tests/acp_adapter/test_events.py index 406b45668d..2df6ae1673 100644 --- a/tests/acp_adapter/test_events.py +++ b/tests/acp_adapter/test_events.py @@ -315,79 +315,45 @@ class TestAssistantMessageIds: class TestToolCallsAlwaysReachATerminalStatus: """A tool call left ``in_progress`` makes a finished turn look like it ran nothing. - Paseo read four Hermes tool calls as never-run on 2026-09-17: the step - callback only fires on the *next* step, so a turn's last tools stayed open, - and a denied edit projects no ``tool.completed`` at all.""" + Two invariants: every call is closed exactly once from its own ``tool.completed`` + (the ``prev_tools`` step closer stands down once completions arrive), and whatever + is still open at turn end is failed with BOTH per-turn dicts drained together.""" def _patch(self): return patch("acp_adapter.events.asyncio.run_coroutine_threadsafe") - def test_tool_completed_closes_the_call_without_waiting_for_another_step(self, mock_conn, event_loop_fixture): + def test_tool_completed_closes_the_call_once_and_the_step_closer_stands_down(self, mock_conn, event_loop_fixture): from collections import deque - ids = {"read": deque(["tc-1"])} - cb = make_tool_progress_cb(mock_conn, "s", event_loop_fixture, ids, {"tc-1": {"args": {"path": "a"}}}) + ids, meta, turn_state = {"read": deque(["tc-1", "tc-2"])}, {"tc-1": {"args": {"path": "a"}}}, {} + progress = make_tool_progress_cb(mock_conn, "s", event_loop_fixture, ids, meta, turn_state=turn_state) + step = make_step_cb(mock_conn, "s", event_loop_fixture, ids, meta, turn_state) with self._patch() as rcts, patch("acp_adapter.events.build_tool_complete") as btc: rcts.return_value = MagicMock(spec=Future) - cb("tool.completed", "read", None, None, result="file body") + progress("tool.completed", "read", None, None, result="file body") + step(2, [{"name": "read", "result": "file body", "arguments": '{"path": "a"}'}]) btc.assert_called_once_with("tc-1", "read", result="file body", function_args={"path": "a"}, snapshot=None) - assert "read" not in ids + assert list(ids["read"]) == ["tc-2"] and "tc-1" not in meta - def test_step_callback_does_not_close_a_second_call_after_tool_completed(self, mock_conn, event_loop_fixture): - """Both closers pop the same FIFO, so the fallback must stand down once completions arrive.""" - from collections import deque - - ids, turn_state = {"read": deque(["tc-1", "tc-2"])}, {} - progress = make_tool_progress_cb(mock_conn, "s", event_loop_fixture, ids, {}, turn_state=turn_state) - step = make_step_cb(mock_conn, "s", event_loop_fixture, ids, {}, turn_state) - with self._patch() as rcts, patch("acp_adapter.events.build_tool_complete") as btc: - rcts.return_value = MagicMock(spec=Future) - progress("tool.completed", "read", None, None, result="first") - step(1, [{"name": "read", "result": "first"}]) - assert [c.args[0] for c in btc.call_args_list] == ["tc-1"] - assert list(ids["read"]) == ["tc-2"] - - def test_step_callback_still_closes_when_no_completion_is_ever_projected(self, mock_conn, event_loop_fixture): - from collections import deque - - ids = {"read": deque(["tc-1"])} - step = make_step_cb(mock_conn, "s", event_loop_fixture, ids, {}, {}) - with self._patch() as rcts, patch("acp_adapter.events.build_tool_complete") as btc: - rcts.return_value = MagicMock(spec=Future) - step(1, [{"name": "read", "result": "body"}]) - btc.assert_called_once_with("tc-1", "read", result="body", function_args=None, snapshot=None) - - def test_a_todo_plan_update_survives_the_completion_path(self, mock_conn, event_loop_fixture): - """The plan panel is fed by the step callback, not by the close it now skips.""" - from collections import deque - - ids, turn_state = {"todo": deque(["tc-1"])}, {} - progress = make_tool_progress_cb(mock_conn, "s", event_loop_fixture, ids, {}, turn_state=turn_state) - step = make_step_cb(mock_conn, "s", event_loop_fixture, ids, {}, turn_state) - todos = '{"todos": [{"content": "ship it", "status": "in_progress"}]}' - with self._patch() as rcts, patch("acp_adapter.events.build_tool_complete"): - rcts.return_value = MagicMock(spec=Future) - progress("tool.completed", "todo", None, None, result=todos) - step(1, [{"name": "todo", "result": todos}]) - sent = [c.args[1] for c in mock_conn.session_update.call_args_list] - assert any(isinstance(u, AgentPlanUpdate) for u in sent) - - def test_flush_fails_a_call_the_turn_never_reported(self, mock_conn, event_loop_fixture): - """A denied edit projects no completion; ``failed`` is the honest end state, not ``completed``.""" + def test_step_fallback_coerces_wire_arguments_and_turn_end_flush_fails_what_is_still_open( + self, mock_conn, event_loop_fixture, + ): + """No completion projected: the step closer must survive the JSON-string ``arguments`` + the wire carries (a real ``write_file`` close raised on ``.get`` and was swallowed); + a denied edit is then failed at turn end, draining ``tool_call_ids`` AND ``tool_call_meta``.""" from collections import deque from acp_adapter.events import flush_open_tool_calls - ids, meta = {"edit": deque(["tc-denied"])}, {"tc-denied": {"args": {}}} + ids = {"write_file": deque(["tc-1"]), "edit": deque(["tc-denied"])} + meta = {"tc-1": {"args": {"path": "a"}, "snapshot": None}, "tc-denied": {"args": {}}} + step = make_step_cb(mock_conn, "s", event_loop_fixture, ids, meta, {}) with self._patch() as rcts: rcts.return_value = MagicMock(spec=Future) + step(2, [{"name": "write_file", "result": "ok", "arguments": '{"path": "a", "content": "x"}'}]) + assert [c.args[1].status for c in mock_conn.session_update.call_args_list] == ["completed"] assert flush_open_tool_calls(mock_conn, "s", event_loop_fixture, ids, meta) == 1 - update = mock_conn.session_update.call_args_list[0].args[1] - assert update.status == "failed" + assert flush_open_tool_calls(mock_conn, "s", event_loop_fixture, ids, meta) == 0 + statuses = [c.args[1].status for c in mock_conn.session_update.call_args_list] + assert statuses == ["completed", "failed"] assert ids == {} and meta == {} - - def test_flush_is_a_no_op_when_every_call_is_closed(self, mock_conn, event_loop_fixture): - from acp_adapter.events import flush_open_tool_calls - - assert flush_open_tool_calls(mock_conn, "s", event_loop_fixture, {}, {}) == 0 - mock_conn.session_update.assert_not_called()