refactor: extract a shared HITL/ask_user interaction engine (#342)
* chore: add pytest-asyncio in auto mode * test: migrate channel and stream tests to native async Convert run_async() wrapper tests to plain 'async def test_*' under pytest-asyncio auto mode. collect_events() in stream_v3_fakes becomes a coroutine awaited at every call site. * test: migrate command and model/middleware tests to native async Convert run_async() wrappers (import, alias, and fixture forms) to plain 'async def test_*'. Multi-call tests merge onto one loop as sequential awaits; none asserted on loop identity. * test: migrate TUI, notifier, gateway, and session tests to native async TUI/notifier/gateway files convert run_async wrappers to plain async tests. test_sessions.py's unittest.TestCase classes move to unittest.IsolatedAsyncioTestCase (pytest-asyncio does not await async methods on plain TestCase; converting blindly would have made ~70 tests silently vacuous). Its setUpClass keeps a one-shot asyncio.run() since IsolatedAsyncioTestCase has no async class-level hook. TestLoadingWidget in test_tui_widgets.py drops its TestCase base for the same reason. * test: replace direct asyncio.run() calls with native async tests Convert tests that called asyncio.run() (directly or via a local _run helper) to plain 'async def test_*'; delete the local helpers. * test: drop undeclared anyio markers and delete run_async helper The @pytest.mark.anyio tests relied on anyio being a transitive dep of httpx; auto-mode pytest-asyncio collects them natively. run_async() and its fixture are unreferenced after the migration, so remove them — pytest-asyncio's per-test loop teardown covers the pending-task cancellation the helper existed for (verified: full suite runs with no 'Event loop is closed' errors or destroyed-task warnings). * test: add autouse fixture for watcher cleanup * refactor: remove redundant hasattr calls * refactor: extract shared HITL/ask_user interaction grammar Extract prompt/question formatting, the reply grammar (approval letters, ask_user choice letters + the 'Other' sub-flow, stop-commands), the ApprovalPolicy (config auto-approve rule + session registry + session-key derivation), per-flow timeout constants, and the bilingual feedback strings into channels/interaction.py. Both drivers now point at the shared functions: this reverses cli/channel.py's imports of consumer privates and closes the /stop drift at the parsing layer (serve-mode ask_user now checks stop-commands before parsing an answer, matching the CLI path). * refactor: add interaction engine + registry; port InboundConsumer Introduce InteractionIO (transport adapter Protocol), PendingReplyRegistry (one asyncio-based reply router per process), and the engine coroutines resolve_ask_user / resolve_approval in channels/interaction.py. Port InboundConsumer onto them: a _ConsumerIO adapter over bus.publish_outbound + the registry, one ApprovalPolicy replacing the config/session auto-approve checks, and a single reply-interception point (registry.try_resolve) replacing the parallel ask_user/HITL pending dicts. _resolve_ask_user and the approval section of _stream_with_hitl are now thin engine calls. Behavior unification (serve mode): an unrecognized HITL reply now declines with the shared 'Unrecognized reply' notice instead of rejecting-and-refeeding as a fresh turn, and /stop mid-approval cancels cleanly — both via the shared parser. * refactor: port CLI channel bridge onto the interaction engine Replace the ~250-line parallel bodies of channel_ask_user_prompt / channel_hitl_prompt with thin bridges that run resolve_ask_user / resolve_approval on the bus loop via run_coroutine_threadsafe(...).result() (outer = engine per-flow timeout + slack, so the engine's own timeout fires first). The 15s send timeout moves into the _BridgeIO adapter. Delete the _pending_hitl / _hitl_lock / _hitl_auto_approve module globals and the _register_hitl_wait / _try_set_hitl_reply / _pop_hitl_reply helpers, absorbed by one bus-loop PendingReplyRegistry + one ApprovalPolicy. The bus consumer feeds the registry via try_resolve ahead of normal enqueue. * refactor: restore serve-mode refeed for unrecognized HITL replies Gate-review fix: the engine no longer decides transport policy for unparseable approval replies. resolve_approval now returns an ApprovalOutcome carrying unrecognized_reply (raw text) when parsing fails, sending no feedback itself; recognized reject keeps the sharedi rejection message. Consumer driver (serve mode) restores the pre-engine semantics: an unrecognized reply rejects the pending action, confirms with the rejection message, and the text is re-dispatched as a NEW agent turn — _stream_with_hitl returns the captured text and _handle_message starts the refeed turn only after the current one has released the chat lock (old fall-through ordering). CLI bridge keeps its old no-refeed path byte-for-byte: 'Unrecognized reply. Action rejected.' and decline. Tests: serve refeed pinned end-to-end (prompt → unrecognized text → rejection feedback → text reaches the stream path as a new turn), CLI no-refeed pinned (notice sent, nothing enqueued), engine test updated to assert the outcome struct with no engine-side feedback. * refactor: polish the interaction engine surface - English feedback strings (Approved / Rejected / auto-approving) - drop the consumer's backwards-compatible re-exports and both modules' private timeout aliases; callers use the canonical interaction names - replace byte-for-byte prompt goldens with structural format tests and assert feedback via the shared constants instead of string literals - strip audit/design shorthand (R1/R2/G3, stage numbers) from comments * fix: propagate pending reply task cancellation * fix: preserve reply context when refeeding HITL replies * fix: honor HITL session grants without bus loop * fix: bound bridge waits by send latency * fix: handle empty ask_user replies explicitly * chore: remove stale interaction helpers * fix: harden interaction engine reply edge cases Review follow-ups on the interaction engine: - normalize ask_user choices before .get(): the tool args come from model JSON and only presence is validated, so plain-string choices must render and parse instead of crashing the turn - treat only None as an approval timeout, so an empty/media-only reply flows through the unrecognized path and serve mode refeeds it with its preserved context - intercept prompt replies before _get_thread_id so a consumed reply cannot create an orphan graph thread or touch the sender-session LRU - close engine coroutines the bridge failed to schedule (no bus loop / scheduling error) to avoid never-awaited warnings - clear pending-response and channel-request state in the bridge test fixture --------- Co-authored-by: Xi Zhang <106144707+X-iZhang@users.noreply.github.com>
This commit is contained in:
dinos
@@ -0,0 +1,341 @@
|
||||
"""Tests the shared interaction grammar for ``channels.interaction``."""
|
||||
|
||||
from typing import ClassVar
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from EvoScientist.channels import interaction as I
|
||||
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
# Stop / cancel grammar
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
class TestStopCommand:
|
||||
@pytest.mark.parametrize(
|
||||
"text",
|
||||
["/stop", "/cancel", " /stop ", "/STOP", "/Cancel", "\t/stop\n"],
|
||||
)
|
||||
def test_stop_recognized(self, text):
|
||||
assert I.is_stop_command(text) is True
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"text",
|
||||
["stop", "cancel", "/stopp", "1", "", None, "please /stop"],
|
||||
)
|
||||
def test_stop_not_recognized(self, text):
|
||||
assert I.is_stop_command(text) is False
|
||||
|
||||
@pytest.mark.parametrize("text", ["cancel", "CANCEL", " Cancel ", "\tcancel"])
|
||||
def test_cancel_recognized(self, text):
|
||||
assert I.is_cancel_reply(text) is True
|
||||
|
||||
@pytest.mark.parametrize("text", ["/cancel", "cancelled", "c", "", None])
|
||||
def test_cancel_not_recognized(self, text):
|
||||
assert I.is_cancel_reply(text) is False
|
||||
|
||||
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
# Approval reply grammar
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
class TestParseApprovalReply:
|
||||
@pytest.mark.parametrize(
|
||||
("text", "expected"),
|
||||
[
|
||||
# approve
|
||||
("1", "approve"),
|
||||
("y", "approve"),
|
||||
("yes", "approve"),
|
||||
("approve", "approve"),
|
||||
("ok", "approve"),
|
||||
(" 1 ", "approve"),
|
||||
(" Y ", "approve"),
|
||||
("YES", "approve"),
|
||||
# reject
|
||||
("2", "reject"),
|
||||
("n", "reject"),
|
||||
("no", "reject"),
|
||||
("reject", "reject"),
|
||||
("REJECT", "reject"),
|
||||
# auto / approve-all
|
||||
("3", "auto"),
|
||||
("a", "auto"),
|
||||
("auto", "auto"),
|
||||
("approve all", "auto"),
|
||||
("APPROVE ALL", "auto"),
|
||||
# unrecognized
|
||||
("hello world", None),
|
||||
("", None),
|
||||
("maybe", None),
|
||||
("4", None),
|
||||
],
|
||||
)
|
||||
def test_parse(self, text, expected):
|
||||
assert I.parse_approval_reply(text) == expected
|
||||
|
||||
def test_button_values_normalize_to_decisions(self):
|
||||
# Feishu/QQ buttons deliver their `value` ("1"/"2"/"3") through
|
||||
# the same reply path, so the shared parser must map them
|
||||
# identically to a typed reply.
|
||||
buttons = I.approval_prompt_metadata(None, with_buttons=True)["buttons"]
|
||||
values = [b["value"] for b in buttons]
|
||||
assert values == ["1", "2", "3"]
|
||||
assert [I.parse_approval_reply(v) for v in values] == [
|
||||
"approve",
|
||||
"reject",
|
||||
"auto",
|
||||
]
|
||||
|
||||
def test_approve_decisions_length(self):
|
||||
assert I.approve_decisions([{"name": "a"}, {"name": "b"}]) == [
|
||||
{"type": "approve"},
|
||||
{"type": "approve"},
|
||||
]
|
||||
# empty request list still yields a single approve (Command shape)
|
||||
assert I.approve_decisions([]) == [{"type": "approve"}]
|
||||
|
||||
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
# ask_user choice grammar (letters + "Other")
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
class TestParseChoiceAnswer:
|
||||
CHOICES: ClassVar = [{"value": "CIFAR-10"}, {"value": "ImageNet"}]
|
||||
|
||||
def test_letter_selects_choice(self):
|
||||
assert I.parse_choice_answer("A", self.CHOICES) == ("answer", "CIFAR-10")
|
||||
assert I.parse_choice_answer("b", self.CHOICES) == ("answer", "ImageNet")
|
||||
|
||||
def test_other_letter(self):
|
||||
# Two choices -> "Other" is C.
|
||||
assert I.parse_choice_answer("C", self.CHOICES) == ("other", None)
|
||||
assert I.parse_choice_answer("c", self.CHOICES) == ("other", None)
|
||||
|
||||
def test_out_of_range_letter_is_literal(self):
|
||||
# Z is a single alpha char but past the choice range -> literal answer.
|
||||
assert I.parse_choice_answer("Z", self.CHOICES) == ("answer", "Z")
|
||||
|
||||
def test_multichar_reply_is_literal(self):
|
||||
assert I.parse_choice_answer("CIFAR-10", self.CHOICES) == (
|
||||
"answer",
|
||||
"CIFAR-10",
|
||||
)
|
||||
|
||||
def test_no_choices_other_is_a(self):
|
||||
assert I.parse_choice_answer("A", []) == ("other", None)
|
||||
|
||||
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
# ApprovalPolicy (config rule + session registry + session key)
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
class TestApprovalPolicy:
|
||||
def test_grant_and_is_granted(self):
|
||||
p = I.ApprovalPolicy()
|
||||
assert p.is_session_granted("tg:c1") is False
|
||||
p.grant_session("tg:c1")
|
||||
assert p.is_session_granted("tg:c1") is True
|
||||
p.clear_sessions()
|
||||
assert p.is_session_granted("tg:c1") is False
|
||||
|
||||
def test_auto_decision_session_granted(self):
|
||||
p = I.ApprovalPolicy()
|
||||
p.grant_session("tg:c1")
|
||||
reqs = [{"name": "execute", "args": {"command": "rm -rf /"}}]
|
||||
# Session grant short-circuits config entirely.
|
||||
assert p.auto_decision("tg:c1", reqs) == [{"type": "approve"}]
|
||||
|
||||
def test_auto_decision_config_true(self):
|
||||
p = I.ApprovalPolicy()
|
||||
cfg = MagicMock()
|
||||
cfg.auto_approve = True
|
||||
with patch("EvoScientist.config.settings.load_config", return_value=cfg):
|
||||
reqs = [{"name": "execute", "args": {"command": "rm -rf /"}}]
|
||||
assert p.auto_decision("tg:c1", reqs) == [{"type": "approve"}]
|
||||
|
||||
def test_auto_decision_needs_prompt(self):
|
||||
p = I.ApprovalPolicy()
|
||||
cfg = MagicMock()
|
||||
cfg.auto_approve = False
|
||||
cfg.shell_allow_list = ""
|
||||
with patch("EvoScientist.config.settings.load_config", return_value=cfg):
|
||||
reqs = [{"name": "execute", "args": {"command": "rm -rf /"}}]
|
||||
assert p.auto_decision("tg:c1", reqs) is None
|
||||
|
||||
|
||||
class TestConfigAutoApprove:
|
||||
def test_empty(self):
|
||||
assert I.config_auto_approve([]) is True
|
||||
|
||||
def test_non_execute(self):
|
||||
assert I.config_auto_approve([{"name": "write_file", "args": {}}]) is True
|
||||
|
||||
def test_execute_no_allowlist(self):
|
||||
cfg = MagicMock()
|
||||
cfg.auto_approve = False
|
||||
cfg.shell_allow_list = ""
|
||||
with patch("EvoScientist.config.settings.load_config", return_value=cfg):
|
||||
assert (
|
||||
I.config_auto_approve(
|
||||
[{"name": "execute", "args": {"command": "rm -rf /"}}]
|
||||
)
|
||||
is False
|
||||
)
|
||||
|
||||
def test_execute_allowlist_match(self):
|
||||
cfg = MagicMock()
|
||||
cfg.auto_approve = False
|
||||
cfg.shell_allow_list = "ls,python"
|
||||
with patch("EvoScientist.config.settings.load_config", return_value=cfg):
|
||||
assert (
|
||||
I.config_auto_approve(
|
||||
[{"name": "execute", "args": {"command": "ls -la"}}]
|
||||
)
|
||||
is True
|
||||
)
|
||||
|
||||
def test_run_in_background_not_allowlisted(self):
|
||||
cfg = MagicMock()
|
||||
cfg.auto_approve = False
|
||||
cfg.shell_allow_list = "ls,cat"
|
||||
with patch("EvoScientist.config.settings.load_config", return_value=cfg):
|
||||
assert (
|
||||
I.config_auto_approve(
|
||||
[{"name": "run_in_background", "args": {"command": "rm -rf /"}}]
|
||||
)
|
||||
is False
|
||||
)
|
||||
|
||||
def test_fail_closed_on_config_error(self):
|
||||
with patch(
|
||||
"EvoScientist.config.settings.load_config", side_effect=RuntimeError("boom")
|
||||
):
|
||||
assert (
|
||||
I.config_auto_approve([{"name": "execute", "args": {"command": "ls"}}])
|
||||
is False
|
||||
)
|
||||
|
||||
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
# Prompt-format checks.
|
||||
# ═══════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
class TestApprovalPromptFormat:
|
||||
def test_lists_each_action_with_reply_options(self):
|
||||
got = I.format_approval_prompt(
|
||||
[
|
||||
{"name": "execute", "args": {"command": "ls"}},
|
||||
{"name": "write_file", "args": {"path": "/out.txt"}},
|
||||
]
|
||||
)
|
||||
assert "execute: ls" in got
|
||||
assert "write_file: /out.txt" in got
|
||||
# The offered options must match what parse_approval_reply accepts.
|
||||
for option in ("1=Approve", "2=Reject", "3=Approve all"):
|
||||
assert option in got
|
||||
|
||||
def test_with_buttons_drops_text_instruction(self):
|
||||
got = I.format_approval_prompt(
|
||||
[{"name": "execute", "args": {"command": "ls -la"}}],
|
||||
with_buttons=True,
|
||||
)
|
||||
assert "execute: ls -la" in got
|
||||
assert "1=Approve" not in got # buttons replace the typed-reply hint
|
||||
|
||||
def test_no_command_falls_back_to_name(self):
|
||||
got = I.format_approval_prompt([{"name": "ask_user", "args": {}}])
|
||||
assert "ask_user" in got
|
||||
|
||||
def test_metadata_no_buttons(self):
|
||||
assert I.approval_prompt_metadata({"k": "v"}, with_buttons=False) == {"k": "v"}
|
||||
|
||||
def test_metadata_with_buttons(self):
|
||||
md = I.approval_prompt_metadata({"k": "v"}, with_buttons=True)
|
||||
assert md["k"] == "v"
|
||||
# Button values must be replies parse_approval_reply understands.
|
||||
assert [b["value"] for b in md["buttons"]] == ["1", "2", "3"]
|
||||
|
||||
|
||||
class TestQuestionPromptFormat:
|
||||
def test_single_question_offers_cancel(self):
|
||||
got = I.format_question_prompt(
|
||||
{"question": "What dataset?", "type": "text"}, 0, 1
|
||||
)
|
||||
assert "What dataset?" in got
|
||||
assert "cancel" in got
|
||||
|
||||
def test_optional_question_is_marked_and_skippable(self):
|
||||
got = I.format_question_prompt(
|
||||
{"question": "Notes?", "type": "text", "required": False}, 0, 1
|
||||
)
|
||||
assert "(optional)" in got
|
||||
assert "skip" in got.lower()
|
||||
|
||||
def test_multi_question_header_shows_position(self):
|
||||
got = I.format_question_prompt(
|
||||
{
|
||||
"question": "Which?",
|
||||
"type": "multiple_choice",
|
||||
"choices": [{"value": "A"}, {"value": "B"}],
|
||||
"required": False,
|
||||
},
|
||||
1,
|
||||
3,
|
||||
)
|
||||
assert "2/3" in got
|
||||
|
||||
def test_choices_get_letters_plus_other(self):
|
||||
got = I.format_question_prompt(
|
||||
{
|
||||
"question": "Pick one",
|
||||
"type": "multiple_choice",
|
||||
"choices": [{"value": "CIFAR-10"}, {"value": "ImageNet"}],
|
||||
},
|
||||
0,
|
||||
1,
|
||||
)
|
||||
# Displayed letters must match what the choice parser accepts, with
|
||||
# the "Other" free-form option appended after the real choices.
|
||||
assert "A. CIFAR-10" in got
|
||||
assert "B. ImageNet" in got
|
||||
assert "C. Other" in got
|
||||
|
||||
|
||||
class TestChoiceNormalization:
|
||||
"""Choices arrive from model tool args — plain strings must not crash."""
|
||||
|
||||
def test_prompt_renders_plain_string_choices(self):
|
||||
got = I.format_question_prompt(
|
||||
{
|
||||
"question": "Pick one",
|
||||
"type": "multiple_choice",
|
||||
"choices": ["CIFAR-10", "ImageNet"],
|
||||
},
|
||||
0,
|
||||
1,
|
||||
)
|
||||
assert "A. CIFAR-10" in got
|
||||
assert "B. ImageNet" in got
|
||||
assert "C. Other" in got
|
||||
|
||||
def test_parse_returns_plain_string_choice(self):
|
||||
kind, value = I.parse_choice_answer("b", ["CIFAR-10", "ImageNet"])
|
||||
assert (kind, value) == ("answer", "ImageNet")
|
||||
|
||||
def test_mixed_dict_and_string_choices(self):
|
||||
choices = [{"value": "CIFAR-10"}, "ImageNet"]
|
||||
got = I.format_question_prompt(
|
||||
{"question": "Pick", "type": "multiple_choice", "choices": choices},
|
||||
0,
|
||||
1,
|
||||
)
|
||||
assert "A. CIFAR-10" in got
|
||||
assert "B. ImageNet" in got
|
||||
kind, value = I.parse_choice_answer("a", choices)
|
||||
assert (kind, value) == ("answer", "CIFAR-10")
|
||||
Reference in New Issue
Block a user