dinos
5c829942d7
* refactor(cli): replace channel module globals with ChannelRuntime Removes _cli_agent / _cli_thread_id from EvoScientist/cli/channel.py and threads a ChannelRuntime via CommandContext.channel_runtime so /model and /channel rebind without poking module-level state. * fix(cli): address coderabbit review - _auto_start_channel: bind ChannelRuntime only after _start_channels_bus_mode succeeds, so a startup failure no longer leaves a stale binding pointing at channels that never started. - _sync_tui_command_completion (TUI) and the Rich CLI command-completion paths: rebind the runtime on thread rotation, not just agent swap, so /new and /resume keep ChannelRuntime in sync with the running thread (matches the serve-mode hook contract). - test_hook_syncs_channel_runtime: pin ctx.thread_id explicitly so a bare MagicMock attribute can't silently mutate runtime.thread_id. - New regression test covering the rebind-on-thread-rotation contract.
348 lines
11 KiB
Python
348 lines
11 KiB
Python
"""Tests for the serve-mode ``on_cmd_completed`` hook factory.
|
|
|
|
Regression coverage for the follow-up to issue #181 — `/model` invoked
|
|
over a channel in ``EvoSci serve`` must swap the running agent for
|
|
subsequent messages, not silently keep the stale one the while-loop
|
|
captured at startup.
|
|
"""
|
|
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
from EvoScientist.cli.channel import (
|
|
ChannelMessage,
|
|
_register_channel_request,
|
|
)
|
|
from EvoScientist.cli.commands import (
|
|
_make_serve_cmd_completed_hook,
|
|
_make_serve_start_new_session_cb,
|
|
_serve_process_message,
|
|
)
|
|
from EvoScientist.commands.base import ChannelRuntime
|
|
from tests.conftest import run_async as _run
|
|
|
|
|
|
def test_hook_updates_holder_on_agent_swap():
|
|
"""``/model`` mutates ``ctx.agent`` to a new handle — the hook must
|
|
push that handle into the shared holder so the outer poll loop sees
|
|
it on the next message."""
|
|
holder = {"agent": "original-agent"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "new-agent"
|
|
cmd = MagicMock()
|
|
cmd.name = "/model"
|
|
|
|
_run(hook(ctx, "original-agent", cmd))
|
|
|
|
assert holder["agent"] == "new-agent"
|
|
|
|
|
|
def test_hook_syncs_channel_runtime():
|
|
"""Other readers (the bus) look at ``ChannelRuntime.agent``; the
|
|
hook keeps the runtime in sync with the holder update."""
|
|
holder = {"agent": "original-agent", "thread_id": "t"}
|
|
runtime = ChannelRuntime(agent="original-agent", thread_id="t")
|
|
hook = _make_serve_cmd_completed_hook(holder, runtime)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "new-agent"
|
|
# Pin ctx.thread_id explicitly — a bare MagicMock would let the
|
|
# hook's getattr fall through to a fresh MagicMock attribute and
|
|
# silently mutate runtime.thread_id, hiding regressions.
|
|
ctx.thread_id = "t"
|
|
cmd = MagicMock()
|
|
cmd.name = "/model"
|
|
|
|
_run(hook(ctx, "original-agent", cmd))
|
|
|
|
assert runtime.agent == "new-agent"
|
|
assert runtime.thread_id == "t"
|
|
|
|
|
|
def test_hook_noop_when_agent_unchanged():
|
|
"""Commands like ``/evoskills`` don't touch ``ctx.agent`` — the
|
|
holder must stay put."""
|
|
holder = {"agent": "original-agent"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "original-agent" # no swap
|
|
cmd = MagicMock()
|
|
cmd.name = "/evoskills"
|
|
|
|
_run(hook(ctx, "original-agent", cmd))
|
|
|
|
assert holder["agent"] == "original-agent"
|
|
|
|
|
|
def test_hook_noop_when_ctx_agent_is_none():
|
|
"""Guard against commands that reset ``ctx.agent`` to ``None`` —
|
|
we never want to write ``None`` into the holder."""
|
|
holder = {"agent": "original-agent"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = None
|
|
cmd = MagicMock()
|
|
cmd.name = "/whatever"
|
|
|
|
_run(hook(ctx, "original-agent", cmd))
|
|
|
|
assert holder["agent"] == "original-agent"
|
|
|
|
|
|
def test_hook_updates_thread_id_on_resume():
|
|
"""``/resume`` mutates ``ctx.thread_id`` — the hook must push the
|
|
new id into the holder so the outer poll loop runs subsequent
|
|
messages on the resumed thread."""
|
|
holder = {"agent": "a", "thread_id": "original-tid"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "a" # no agent swap
|
|
ctx.thread_id = "new-tid"
|
|
cmd = MagicMock()
|
|
cmd.name = "/resume"
|
|
|
|
_run(hook(ctx, "a", cmd))
|
|
|
|
assert holder["thread_id"] == "new-tid"
|
|
|
|
|
|
def test_hook_updates_workspace_dir_on_resume():
|
|
"""`/resume` can restore a different workspace; serve must adopt it."""
|
|
holder = {"agent": "a", "thread_id": "original-tid", "workspace_dir": "/old-ws"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "a"
|
|
ctx.thread_id = "new-tid"
|
|
ctx.workspace_dir = "/restored-ws"
|
|
cmd = MagicMock()
|
|
cmd.name = "/resume"
|
|
|
|
_run(hook(ctx, "a", cmd))
|
|
|
|
assert holder["workspace_dir"] == "/restored-ws"
|
|
|
|
|
|
def test_hook_syncs_channel_runtime_thread_id():
|
|
"""The bus reads ``ChannelRuntime.thread_id``; hook must sync it
|
|
alongside the holder update."""
|
|
holder = {"agent": "a", "thread_id": "original-tid"}
|
|
runtime = ChannelRuntime(agent="a", thread_id="original-tid")
|
|
hook = _make_serve_cmd_completed_hook(holder, runtime)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "a"
|
|
ctx.thread_id = "new-tid"
|
|
cmd = MagicMock()
|
|
cmd.name = "/resume"
|
|
|
|
_run(hook(ctx, "a", cmd))
|
|
|
|
assert runtime.thread_id == "new-tid"
|
|
|
|
|
|
def test_hook_noop_when_thread_id_unchanged():
|
|
"""Most commands don't touch thread_id — holder stays put."""
|
|
holder = {"agent": "a", "thread_id": "same-tid"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "a"
|
|
ctx.thread_id = "same-tid"
|
|
cmd = MagicMock()
|
|
cmd.name = "/evoskills"
|
|
|
|
_run(hook(ctx, "a", cmd))
|
|
|
|
assert holder["thread_id"] == "same-tid"
|
|
|
|
|
|
def test_hook_skips_resume_warning_when_thread_unchanged():
|
|
"""Bare ``/resume`` with no argument prints usage but leaves
|
|
``ctx.thread_id`` unchanged — the in-memory-state warning must NOT
|
|
fire because no resume actually happened."""
|
|
holder = {"agent": "a", "thread_id": "original-tid"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "a"
|
|
ctx.thread_id = "original-tid" # unchanged — bare /resume case
|
|
cmd = MagicMock()
|
|
cmd.name = "/resume"
|
|
|
|
_run(hook(ctx, "a", cmd))
|
|
|
|
ctx.ui.append_system.assert_not_called()
|
|
ctx.ui.flush.assert_not_called()
|
|
|
|
|
|
def test_hook_emits_resume_warning_when_thread_changed():
|
|
"""``/resume <tid>`` that actually changes thread_id must surface
|
|
the in-memory-state warning via ``ctx.ui``."""
|
|
holder = {"agent": "a", "thread_id": "original-tid"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
# Mock out async flush so the test can synchronously run the hook.
|
|
ctx.ui.flush = AsyncMock()
|
|
ctx.agent = "a"
|
|
ctx.thread_id = "abc12345-resumed-tid"
|
|
cmd = MagicMock()
|
|
cmd.name = "/resume"
|
|
|
|
_run(hook(ctx, "a", cmd))
|
|
|
|
ctx.ui.append_system.assert_called_once()
|
|
warn_text, warn_kwargs = (
|
|
ctx.ui.append_system.call_args.args,
|
|
ctx.ui.append_system.call_args.kwargs,
|
|
)
|
|
assert "in-memory state" in warn_text[0]
|
|
assert "abc12345" in warn_text[0]
|
|
assert warn_kwargs.get("style") == "yellow"
|
|
ctx.ui.flush.assert_awaited_once()
|
|
|
|
|
|
def test_start_new_session_cb_rotates_thread_id():
|
|
"""``/new`` via channel calls this callback — must generate a new
|
|
thread id, push into holder, and sync the channel runtime."""
|
|
holder = {"agent": "a", "thread_id": "old-tid"}
|
|
runtime = ChannelRuntime(agent="a", thread_id="old-tid")
|
|
|
|
with patch(
|
|
"EvoScientist.sessions.generate_thread_id",
|
|
return_value="freshly-generated-tid",
|
|
):
|
|
cb = _make_serve_start_new_session_cb(holder, runtime)
|
|
cb()
|
|
|
|
assert holder["thread_id"] == "freshly-generated-tid"
|
|
assert runtime.thread_id == "freshly-generated-tid"
|
|
|
|
|
|
def test_start_new_session_cb_leaves_agent_alone():
|
|
"""``/new`` rotates thread only — agent handle must stay put
|
|
(serve's agent is a single pre-loaded instance, not per-thread)."""
|
|
holder = {"agent": "a", "thread_id": "old-tid"}
|
|
|
|
with patch(
|
|
"EvoScientist.sessions.generate_thread_id",
|
|
return_value="new-tid",
|
|
):
|
|
cb = _make_serve_start_new_session_cb(holder)
|
|
cb()
|
|
|
|
assert holder["agent"] == "a"
|
|
|
|
|
|
def test_hook_handles_both_agent_and_thread_swap():
|
|
"""Edge case: a command that changes both (hypothetical). Both
|
|
updates must land in the holder."""
|
|
holder = {"agent": "old-agent", "thread_id": "old-tid"}
|
|
hook = _make_serve_cmd_completed_hook(holder)
|
|
|
|
ctx = MagicMock()
|
|
ctx.agent = "new-agent"
|
|
ctx.thread_id = "new-tid"
|
|
cmd = MagicMock()
|
|
|
|
_run(hook(ctx, "old-agent", cmd))
|
|
|
|
assert holder["agent"] == "new-agent"
|
|
assert holder["thread_id"] == "new-tid"
|
|
|
|
|
|
def test_serve_process_message_reports_slash_dispatch_error_without_fallback():
|
|
"""Defensive: if ``dispatch_channel_slash_command`` ever leaks an
|
|
exception past its own wrapper, ``_serve_process_message`` must set
|
|
one error response and not fall through to ``run_streaming``.
|
|
"""
|
|
msg = ChannelMessage(
|
|
msg_id="msg-1",
|
|
content="/evoskills core",
|
|
sender="channel-user",
|
|
channel_type="imessage",
|
|
metadata={},
|
|
channel_ref=None,
|
|
bus_ref=None,
|
|
chat_id="channel-user",
|
|
message_id="ts-1",
|
|
)
|
|
holder = {"agent": "agent", "thread_id": "tid"}
|
|
|
|
with (
|
|
patch(
|
|
"EvoScientist.cli.commands.dispatch_channel_slash_command",
|
|
new=AsyncMock(side_effect=RuntimeError("slash broke")),
|
|
),
|
|
patch("EvoScientist.cli.commands._set_channel_response") as mock_set_resp,
|
|
patch("EvoScientist.cli.tui_runtime.run_streaming") as mock_run_streaming,
|
|
):
|
|
_register_channel_request(msg)
|
|
_serve_process_message(
|
|
msg,
|
|
agent_holder=holder,
|
|
model="model",
|
|
workspace_dir="/tmp",
|
|
show_thinking=False,
|
|
)
|
|
|
|
mock_set_resp.assert_called_once_with("msg-1", "Command error: slash broke")
|
|
mock_run_streaming.assert_not_called()
|
|
|
|
|
|
def test_serve_process_message_uses_runtime_workspace_from_holder():
|
|
"""After `/resume`, serve should use the adopted workspace, not startup ws."""
|
|
msg = ChannelMessage(
|
|
msg_id="msg-2",
|
|
content="hello",
|
|
sender="channel-user",
|
|
channel_type="imessage",
|
|
metadata={},
|
|
channel_ref=None,
|
|
bus_ref=None,
|
|
chat_id="channel-user",
|
|
message_id="ts-2",
|
|
)
|
|
holder = {
|
|
"agent": "agent",
|
|
"thread_id": "tid",
|
|
"workspace_dir": "/restored-workspace",
|
|
}
|
|
captured: dict[str, str] = {}
|
|
|
|
async def _fake_dispatch(*args, **kwargs):
|
|
captured["slash_workspace"] = kwargs["workspace_dir"]
|
|
return False
|
|
|
|
def _fake_build_metadata(workspace_dir: str, _model: str | None):
|
|
captured["meta_workspace"] = workspace_dir
|
|
return {}
|
|
|
|
with (
|
|
patch(
|
|
"EvoScientist.cli.commands.dispatch_channel_slash_command",
|
|
new=AsyncMock(side_effect=_fake_dispatch),
|
|
),
|
|
patch(
|
|
"EvoScientist.cli.commands.build_metadata",
|
|
side_effect=_fake_build_metadata,
|
|
),
|
|
patch("EvoScientist.cli.tui_runtime.run_streaming", return_value="ok"),
|
|
):
|
|
_register_channel_request(msg)
|
|
_serve_process_message(
|
|
msg,
|
|
agent_holder=holder,
|
|
model="model",
|
|
workspace_dir="/startup-workspace",
|
|
show_thinking=False,
|
|
)
|
|
|
|
assert captured["slash_workspace"] == "/restored-workspace"
|
|
assert captured["meta_workspace"] == "/restored-workspace"
|