hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 28 Packages Projects Releases Wiki Activity

fix(acp): step-closer fallback coerces wire arguments; trim to two invariant tests

Browse Source 
Follow-up to the salvaged #114442 (@hteo1337), which closes each ACP tool call
from its own ``tool.completed`` (the mechanism PR #50741 by @liuhao1024 filed in
June, and PR #27854 by @godlin-gh filed first in May via ``tool_complete_callback``) and fails whatever is still open at turn end, draining
``tool_call_ids`` and ``tool_call_meta`` together.

Live repro of the fallback path exposed a third mechanism the issue did not
name: ``make_step_cb`` passed ``prev_tools[i]["arguments"]`` — the wire JSON
*string* — as ``function_args`` into ``build_tool_complete``, whose content
builders index it as a dict. The ``.get`` on a str raised inside the swallowed
step callback, so a ``write_file`` close never reached the client even when a
next step existed. Coerce with ``coerce_tool_args`` (meta args when absent).

Tests: the contributor's six tests folded into two invariants — a call is
closed exactly once from ``tool.completed`` with the step closer standing down;
the step fallback survives JSON-string arguments and the turn-end flush fails
the remaining call while emptying BOTH per-turn dicts.

Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
Co-authored-by: godlin <ganlinbupt@gmail.com>
This commit is contained in:
teknium1
2026-09-18 00:53:15 -07:00
committed by Teknium
parent 12d7b4ab9c
commit ac6fc98c0f
2 changed files with 29 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
acp_adapter/events.py
View File

@@ -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)

82
tests/acp_adapter/test_events.py
View File

@@ -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()