fix(tui): address CoreRasurae review — worker error handling and test gaps
Fix 1 (HIGH): _on_llm_done now checks worker.error via _format_worker_outcome() so UI never freezes silently when the worker thread fails unexpectedly. Fix 2 (MEDIUM): _run_llm_dispatch catches SessionNotFoundError and DatabaseError with user-friendly messages instead of falling through to generic [Error]. Fix 3 (MEDIUM): BDD step_facade_received_request now asserts against context._submitted_text (set in step_submit_text) instead of the hardcoded literal "hello there". Fix 4 (MEDIUM): Robot integration cmd_dispatch_message now verifies assistant_msg contains _STUB_TEXT, not just len > 0. Fix 5 (LOW): Extract _format_worker_outcome() as a testable module-level function; add 4 new BDD scenarios covering SessionNotFoundError, DatabaseError, worker error surfacing, and worker success pass-through (12 scenarios total). Refs: #11230
This commit is contained in:
@@ -17,7 +17,7 @@ from unittest.mock import MagicMock, patch
|
||||
|
||||
from behave import given, then, when
|
||||
|
||||
from cleveragents.tui.app import _run_llm_dispatch
|
||||
from cleveragents.tui.app import _format_worker_outcome, _run_llm_dispatch
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -57,9 +57,7 @@ def step_init_without_facade(context: Any) -> None:
|
||||
@given("the facade raises SessionActorNotConfiguredError")
|
||||
def step_facade_raises_no_actor(context: Any) -> None:
|
||||
"""Configure the mock facade to raise SessionActorNotConfiguredError."""
|
||||
from cleveragents.domain.models.core.session import (
|
||||
SessionActorNotConfiguredError,
|
||||
)
|
||||
from cleveragents.domain.models.core.session import SessionActorNotConfiguredError
|
||||
|
||||
facade = MagicMock()
|
||||
facade.dispatch.side_effect = SessionActorNotConfiguredError("no actor")
|
||||
@@ -67,6 +65,28 @@ def step_facade_raises_no_actor(context: Any) -> None:
|
||||
context._session_id = "default"
|
||||
|
||||
|
||||
@given("the facade raises SessionNotFoundError")
|
||||
def step_facade_raises_session_not_found(context: Any) -> None:
|
||||
"""Configure the mock facade to raise SessionNotFoundError."""
|
||||
from cleveragents.domain.models.core.session import SessionNotFoundError
|
||||
|
||||
facade = MagicMock()
|
||||
facade.dispatch.side_effect = SessionNotFoundError("session not found")
|
||||
context._facade = facade
|
||||
context._session_id = "default"
|
||||
|
||||
|
||||
@given("the facade raises DatabaseError")
|
||||
def step_facade_raises_database_error(context: Any) -> None:
|
||||
"""Configure the mock facade to raise DatabaseError."""
|
||||
from cleveragents.core.exceptions import DatabaseError
|
||||
|
||||
facade = MagicMock()
|
||||
facade.dispatch.side_effect = DatabaseError("db locked")
|
||||
context._facade = facade
|
||||
context._session_id = "default"
|
||||
|
||||
|
||||
@given("the facade returns an A2A error response")
|
||||
def step_facade_returns_error(context: Any) -> None:
|
||||
"""Configure the mock facade to return an A2A error response."""
|
||||
@@ -109,6 +129,7 @@ def step_session_service_raises(context: Any) -> None:
|
||||
@when('I submit the text "{text}"')
|
||||
def step_submit_text(context: Any, text: str) -> None:
|
||||
"""Invoke _run_llm_dispatch directly with the given text."""
|
||||
context._submitted_text = text # store for parameterized assertions
|
||||
if context._facade is None:
|
||||
# Preview fallback path — no dispatch, just echo
|
||||
context._result_text = text
|
||||
@@ -121,11 +142,26 @@ def step_submit_text(context: Any, text: str) -> None:
|
||||
@when('I dispatch "{text}" overriding actor with "{actor}"')
|
||||
def step_dispatch_text_with_actor(context: Any, text: str, actor: str) -> None:
|
||||
"""Invoke _run_llm_dispatch with an explicit actor_override."""
|
||||
context._submitted_text = text
|
||||
context._result_text = _run_llm_dispatch(
|
||||
context._facade, context._session_id, text, actor_override=actor
|
||||
)
|
||||
|
||||
|
||||
@when('the worker completes with error "{error_msg}"')
|
||||
def step_worker_completes_with_error(context: Any, error_msg: str) -> None:
|
||||
"""Simulate a Textual worker that failed with the given exception."""
|
||||
context._worker_outcome = _format_worker_outcome(
|
||||
result=None, error=RuntimeError(error_msg)
|
||||
)
|
||||
|
||||
|
||||
@when('the worker completes with result "{result}"')
|
||||
def step_worker_completes_with_result(context: Any, result: str) -> None:
|
||||
"""Simulate a Textual worker that succeeded with the given result."""
|
||||
context._worker_outcome = _format_worker_outcome(result=result, error=None)
|
||||
|
||||
|
||||
@given('a persona state with active actor "{actor}"')
|
||||
def step_persona_state_with_actor(context: Any, actor: str) -> None:
|
||||
"""Set up a mock PersonaState whose active persona has the given actor."""
|
||||
@@ -174,7 +210,10 @@ def step_facade_received_request(context: Any) -> None:
|
||||
f"Expected method='message/send', got {request.method!r}"
|
||||
)
|
||||
assert request.params["session_id"] == context._session_id
|
||||
assert request.params["message"] == "hello there"
|
||||
assert request.params["message"] == context._submitted_text, (
|
||||
f"Expected message={context._submitted_text!r}, "
|
||||
f"got {request.params['message']!r}"
|
||||
)
|
||||
|
||||
|
||||
@then("the conversation should show the assistant response")
|
||||
@@ -230,6 +269,39 @@ def step_facade_received_request_with_actor(context: Any, actor: str) -> None:
|
||||
)
|
||||
|
||||
|
||||
@then("the conversation should show a session-not-found error message")
|
||||
def step_conversation_shows_session_not_found(context: Any) -> None:
|
||||
"""Assert the session-not-found friendly message is returned."""
|
||||
assert "Session not found" in context._result_text, (
|
||||
f"Expected session-not-found message, got: {context._result_text!r}"
|
||||
)
|
||||
|
||||
|
||||
@then("the conversation should show a database error message")
|
||||
def step_conversation_shows_database_error(context: Any) -> None:
|
||||
"""Assert the database error message is returned."""
|
||||
assert "Database error" in context._result_text, (
|
||||
f"Expected database error message, got: {context._result_text!r}"
|
||||
)
|
||||
|
||||
|
||||
@then("the formatted outcome should show an error message")
|
||||
def step_worker_outcome_shows_error(context: Any) -> None:
|
||||
"""Assert _format_worker_outcome produced an [Error] string."""
|
||||
assert context._worker_outcome is not None, "Expected non-None outcome"
|
||||
assert "[Error]" in context._worker_outcome, (
|
||||
f"Expected [Error] in outcome, got: {context._worker_outcome!r}"
|
||||
)
|
||||
|
||||
|
||||
@then('the formatted outcome should be "{expected}"')
|
||||
def step_worker_outcome_equals(context: Any, expected: str) -> None:
|
||||
"""Assert _format_worker_outcome returned the expected string."""
|
||||
assert context._worker_outcome == expected, (
|
||||
f"Expected {expected!r}, got {context._worker_outcome!r}"
|
||||
)
|
||||
|
||||
|
||||
@then('the session should have been created with actor "{actor}"')
|
||||
def step_session_created_with_actor(context: Any, actor: str) -> None:
|
||||
"""Assert service.create() was called with the persona's actor."""
|
||||
|
||||
@@ -40,6 +40,24 @@ Feature: TUI normal text input dispatches to LLM via A2A facade
|
||||
When I call _create_tui_session with that service
|
||||
Then the returned session_id should be "default"
|
||||
|
||||
Scenario: LLM dispatch handles SessionNotFoundError gracefully
|
||||
Given the facade raises SessionNotFoundError
|
||||
When I submit the text "hello"
|
||||
Then the conversation should show a session-not-found error message
|
||||
|
||||
Scenario: LLM dispatch handles DatabaseError gracefully
|
||||
Given the facade raises DatabaseError
|
||||
When I submit the text "hello"
|
||||
Then the conversation should show a database error message
|
||||
|
||||
Scenario: Worker error is surfaced in the conversation widget
|
||||
When the worker completes with error "network timeout"
|
||||
Then the formatted outcome should show an error message
|
||||
|
||||
Scenario: Worker success result is passed through unchanged
|
||||
When the worker completes with result "You: hi\n\nAssistant: hello"
|
||||
Then the formatted outcome should be "You: hi\n\nAssistant: hello"
|
||||
|
||||
Scenario: _create_tui_session binds active persona actor at session creation
|
||||
Given a mock session service that returns session_id "abc-123"
|
||||
And a persona state with active actor "anthropic/claude-4-sonnet"
|
||||
|
||||
@@ -168,6 +168,9 @@ def cmd_dispatch_message() -> None:
|
||||
assert response.result is not None, "Expected result dict"
|
||||
assistant_msg: str = response.result.get("assistant_message", "")
|
||||
assert len(assistant_msg) > 0, "Expected non-empty assistant message"
|
||||
assert _STUB_TEXT in assistant_msg, (
|
||||
f"Expected stub text {_STUB_TEXT!r} in response, got: {assistant_msg!r}"
|
||||
)
|
||||
|
||||
messages = service.get_messages(session_id)
|
||||
# get_messages() returns SessionMessage objects — access .role attribute
|
||||
|
||||
@@ -57,11 +57,15 @@ def _run_llm_dispatch(
|
||||
|
||||
Returns a formatted string ready for the conversation widget:
|
||||
- On success: ``"You: <message>\\n\\nAssistant: <reply>"``
|
||||
- On no-actor: friendly guidance message
|
||||
- On no-actor / session-not-found / database error: friendly guidance
|
||||
- On any other error: ``"[Error] <description>"``
|
||||
"""
|
||||
from cleveragents.a2a.models import A2aRequest
|
||||
from cleveragents.domain.models.core.session import SessionActorNotConfiguredError
|
||||
from cleveragents.core.exceptions import DatabaseError
|
||||
from cleveragents.domain.models.core.session import (
|
||||
SessionActorNotConfiguredError,
|
||||
SessionNotFoundError,
|
||||
)
|
||||
|
||||
params: dict[str, Any] = {"session_id": session_id, "message": message}
|
||||
if actor_override:
|
||||
@@ -76,10 +80,38 @@ def _run_llm_dispatch(
|
||||
return f"You: {message}\n\nAssistant: {assistant_text}"
|
||||
except SessionActorNotConfiguredError:
|
||||
return "No actor configured. Use /persona set <name> or select an actor first."
|
||||
except SessionNotFoundError:
|
||||
return (
|
||||
"[Error] Session not found. Please restart the TUI to create a new session."
|
||||
)
|
||||
except DatabaseError as exc:
|
||||
return f"[Error] Database error: {exc}"
|
||||
except Exception as exc: # pragma: no cover
|
||||
return f"[Error] {exc}"
|
||||
|
||||
|
||||
def _format_worker_outcome(
|
||||
result: str | None,
|
||||
error: BaseException | None,
|
||||
) -> str | None:
|
||||
"""Translate a completed Textual worker's outcome into a display string.
|
||||
|
||||
This module-level helper is extracted from the ``_on_llm_done`` closure
|
||||
so it can be unit-tested independently of the Textual event loop.
|
||||
|
||||
Returns
|
||||
-------
|
||||
str | None
|
||||
A string to display in the conversation widget, or ``None`` if there
|
||||
is nothing to show (both *result* and *error* are ``None``).
|
||||
"""
|
||||
if result is not None:
|
||||
return result
|
||||
if error is not None:
|
||||
return f"[Error] {error}"
|
||||
return None # pragma: no cover
|
||||
|
||||
|
||||
_TEXTUAL_AVAILABLE = False
|
||||
_TextualApp: type[Any] = object
|
||||
_Vertical: type[Any] = object
|
||||
@@ -289,8 +321,9 @@ if _TEXTUAL_AVAILABLE:
|
||||
|
||||
def _on_llm_done(worker: Any) -> None:
|
||||
"""Update conversation widget once worker finishes."""
|
||||
if worker.result is not None:
|
||||
conversation.update(worker.result)
|
||||
text = _format_worker_outcome(worker.result, worker.error)
|
||||
if text is not None:
|
||||
conversation.update(text)
|
||||
|
||||
worker = self.run_worker(_dispatch_llm, thread=True, exclusive=False)
|
||||
worker.done_callback = _on_llm_done
|
||||
|
||||
Reference in New Issue
Block a user