diff --git a/features/steps/tui_llm_dispatch_steps.py b/features/steps/tui_llm_dispatch_steps.py index af86e43d2..93fd68049 100644 --- a/features/steps/tui_llm_dispatch_steps.py +++ b/features/steps/tui_llm_dispatch_steps.py @@ -47,6 +47,13 @@ def step_init_with_facade(context: Any) -> None: context._session_id = "test-session-001" +@given("the TUI app is initialised with a facade that returns empty response") +def step_init_with_empty_response_facade(context: Any) -> None: + """Set up a mock facade that returns empty assistant_message.""" + context._facade = _make_mock_facade(response_text="") + context._session_id = "test-session-001" + + @given("the TUI app is initialised without a facade") def step_init_without_facade(context: Any) -> None: """Simulate facade=None (preview fallback path).""" @@ -148,6 +155,12 @@ def step_dispatch_text_with_actor(context: Any, text: str, actor: str) -> None: ) +@when("the worker completes with a KeyboardInterrupt error") +def step_worker_keyboard_interrupt(context: Any) -> None: + """Simulate a worker that completed with a KeyboardInterrupt.""" + context._worker_error = KeyboardInterrupt("user interrupt") + + @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.""" @@ -162,6 +175,13 @@ def step_worker_completes_with_result(context: Any, result: str) -> None: context._worker_outcome = _format_worker_outcome(result=result, error=None) +@given('a facade returning assistant text "{reply}"') +def step_facade_returning_specific_reply(context: Any, reply: str) -> None: + """Set up a mock facade with a specific assistant reply.""" + context._facade = _make_mock_facade(response_text=reply) + context._session_id = "test-session-001" + + @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.""" @@ -302,6 +322,51 @@ def step_worker_outcome_equals(context: Any, expected: str) -> None: ) +@then('the conversation should show "(no response)"') +def step_conversation_shows_no_response(context: Any) -> None: + """Assert the empty-response fallback text is present.""" + assert "(no response)" in context._result_text, ( + f"Expected '(no response)' in result, got: {context._result_text!r}" + ) + + +@then("the result text should contain escaped markup") +def step_result_text_escaped(context: Any) -> None: + """Assert that applying rich.markup.escape() to the result text escapes brackets. + + _run_llm_dispatch returns raw text; the TUI's on_input_submitted applies + rich.markup.escape() before passing to conversation.update(). This test + verifies the escape works correctly when applied to the dispatch result. + """ + from rich.markup import escape + + raw = context._result_text + escaped = escape(raw) + + # After escaping, no unescaped [tag] patterns should remain. + # Use negative lookbehind to distinguish \[tag] (escaped) from [tag] (raw). + import re + + assert not re.search(r"(? None: + """Assert _format_worker_outcome re-raises BaseException subclasses.""" + raised = False + try: + _format_worker_outcome(result=None, error=context._worker_error) + except KeyboardInterrupt: + raised = True + assert raised, "_format_worker_outcome must re-raise KeyboardInterrupt" + + @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.""" diff --git a/features/tui_llm_dispatch.feature b/features/tui_llm_dispatch.feature index b02bcad85..a3071389c 100644 --- a/features/tui_llm_dispatch.feature +++ b/features/tui_llm_dispatch.feature @@ -58,6 +58,20 @@ Feature: TUI normal text input dispatches to LLM via A2A facade When the worker completes with result "You: hi\n\nAssistant: hello" Then the formatted outcome should be "You: hi\n\nAssistant: hello" + Scenario: Empty assistant response shows fallback text + Given the TUI app is initialised with a facade that returns empty response + When I submit the text "hello" + Then the conversation should show "(no response)" + + Scenario: Rich markup in user input is escaped + Given a facade returning assistant text "ok" + When I dispatch "[bold]attack[/bold]" overriding actor with "openai/gpt-4o" + Then the result text should contain escaped markup + + Scenario: _format_worker_outcome re-raises BaseException subclasses + When the worker completes with a KeyboardInterrupt error + Then _format_worker_outcome should re-raise it + 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" diff --git a/src/cleveragents/tui/app.py b/src/cleveragents/tui/app.py index d5dd43b63..a84564b1a 100644 --- a/src/cleveragents/tui/app.py +++ b/src/cleveragents/tui/app.py @@ -7,6 +7,8 @@ import os from dataclasses import dataclass from typing import TYPE_CHECKING, Any, ClassVar, Protocol +import structlog + from cleveragents.tui.first_run import create_default_persona_for_actor, is_first_run from cleveragents.tui.input.modes import InputMode, InputModeRouter from cleveragents.tui.input.reference_parser import suggestions @@ -29,6 +31,14 @@ if TYPE_CHECKING: else: InputSubmittedEvent = Any +logger = structlog.get_logger(__name__) + +# Sentinel session ID used when the real DB session cannot be created. +_FALLBACK_SESSION_ID = "default" +# Session ID key used to look up the active persona before the real session +# exists (persona resolution happens before session creation in run_tui). +_PRE_SESSION_PERSONA_KEY = "default" + def _run_llm_dispatch( facade: A2aLocalFacade, @@ -74,19 +84,32 @@ def _run_llm_dispatch( try: response = facade.dispatch(A2aRequest(method="message/send", params=params)) if response.error is not None: + logger.warning( + "tui.llm_dispatch.a2a_error", + session_id=session_id, + error=response.error.message, + ) return f"[Error] {response.error.message}" result_data: dict[str, Any] = response.result or {} - assistant_text: str = result_data.get("assistant_message", "") + assistant_text: str = ( + result_data.get("assistant_message", "") or "(no response)" + ) return f"You: {message}\n\nAssistant: {assistant_text}" except SessionActorNotConfiguredError: + logger.info( + "tui.llm_dispatch.no_actor", session_id=session_id, actor=actor_override + ) return "No actor configured. Use /persona set or select an actor first." except SessionNotFoundError: + logger.warning("tui.llm_dispatch.session_not_found", session_id=session_id) return ( "[Error] Session not found. Please restart the TUI to create a new session." ) except DatabaseError as exc: + logger.exception("tui.llm_dispatch.database_error", session_id=session_id) return f"[Error] Database error: {exc}" except Exception as exc: # pragma: no cover + logger.exception("tui.llm_dispatch.unexpected_error", session_id=session_id) return f"[Error] {exc}" @@ -99,6 +122,10 @@ def _format_worker_outcome( This module-level helper is extracted from the ``_on_llm_done`` closure so it can be unit-tested independently of the Textual event loop. + ``BaseException`` subclasses that are not ``Exception`` (e.g. + ``KeyboardInterrupt``, ``SystemExit``) are re-raised rather than + stringified so they can propagate correctly to the Textual runtime. + Returns ------- str | None @@ -108,6 +135,9 @@ def _format_worker_outcome( if result is not None: return result if error is not None: + if not isinstance(error, Exception): + raise error # re-raise KeyboardInterrupt, SystemExit, etc. + logger.warning("tui.worker.error", error=str(error)) return f"[Error] {error}" return None # pragma: no cover @@ -162,7 +192,7 @@ class _FallbackCleverAgentsTuiApp: # pragma: no cover command_router: _CommandRouter, persona_state: PersonaState, facade: A2aLocalFacade | None = None, - session_id: str = "default", + session_id: str = _FALLBACK_SESSION_ID, ) -> None: del command_router, persona_state, facade, session_id @@ -192,7 +222,7 @@ if _TEXTUAL_AVAILABLE: command_router: _CommandRouter, persona_state: PersonaState, facade: A2aLocalFacade | None = None, - session_id: str = "default", + session_id: str = _FALLBACK_SESSION_ID, ) -> None: super().__init__() self._command_router = command_router @@ -261,6 +291,8 @@ if _TEXTUAL_AVAILABLE: def on_input_submitted(self, event: InputSubmittedEvent) -> None: del event + from rich.markup import escape as _escape + prompt = self.query_one("#prompt", PromptInput) payload = prompt.consume_text() text = payload.text.strip() @@ -291,7 +323,7 @@ if _TEXTUAL_AVAILABLE: output = ( shell.stdout.strip() or shell.stderr.strip() or "(empty output)" ) - conversation.update(f"$ {shell.command}\n{output}") + conversation.update(_escape(f"$ {shell.command}\n{output}")) return expanded = result.expanded_text @@ -303,13 +335,16 @@ if _TEXTUAL_AVAILABLE: if self._facade is None: # Facade not wired yet — preview only (graceful degradation) - conversation.update(expanded) + conversation.update(_escape(expanded)) return - conversation.update(f"You: {expanded}\n\n⏳ Thinking...") + conversation.update(_escape(f"You: {expanded}\n\n⏳ Thinking...")) # Run blocking LLM call off the main Textual thread so the # event loop stays responsive during the API round-trip. + # exclusive=True serialises dispatches: a new message cancels + # any in-flight worker, preventing concurrent writes to the + # same session and conversation widget thrashing. # Pass the active persona's actor as an override on every # request so persona cycling (tab) takes effect immediately. facade = self._facade @@ -321,11 +356,13 @@ if _TEXTUAL_AVAILABLE: def _on_llm_done(worker: Any) -> None: """Update conversation widget once worker finishes.""" - text = _format_worker_outcome(worker.result, worker.error) - if text is not None: - conversation.update(text) + outcome = _format_worker_outcome(worker.result, worker.error) + if outcome is not None: + conversation.update(_escape(outcome)) - worker = self.run_worker(_dispatch_llm, thread=True, exclusive=False) + worker = self.run_worker( + _dispatch_llm, thread=True, exclusive=True, name="llm-dispatch" + ) worker.done_callback = _on_llm_done _ResolvedTuiApp = _TextualCleverAgentsTuiApp diff --git a/src/cleveragents/tui/commands.py b/src/cleveragents/tui/commands.py index 7c9a064b6..ec5265970 100644 --- a/src/cleveragents/tui/commands.py +++ b/src/cleveragents/tui/commands.py @@ -10,6 +10,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Any from cleveragents.application.container import get_container +from cleveragents.tui.app import _FALLBACK_SESSION_ID, _PRE_SESSION_PERSONA_KEY if TYPE_CHECKING: from cleveragents.a2a.facade import A2aLocalFacade @@ -236,7 +237,6 @@ def _build_tui_facade() -> A2aLocalFacade | None: LLM stack. Any failure is suppressed so the TUI degrades gracefully to preview-only mode rather than crashing. """ - import contextlib try: from cleveragents.a2a.cli_bootstrap import get_facade @@ -246,11 +246,13 @@ def _build_tui_facade() -> A2aLocalFacade | None: container = get_container() session_svc = None - with contextlib.suppress(Exception): + try: session_svc = container.session_service() facade.register_service("session_service", session_svc) + except (RuntimeError, ValueError, ImportError, AttributeError, OSError): + pass - with contextlib.suppress(Exception): + try: provider_registry = None try: from cleveragents.providers.registry import get_provider_registry @@ -266,8 +268,10 @@ def _build_tui_facade() -> A2aLocalFacade | None: provider_registry=provider_registry, ), ) + except (RuntimeError, ValueError, ImportError, AttributeError, OSError): + pass return facade - except Exception: # pragma: no cover + except (ImportError, RuntimeError, AttributeError, OSError): # pragma: no cover return None @@ -278,22 +282,25 @@ def _create_tui_session(persona_state: PersonaState | None = None) -> str: the session at creation time so the first message dispatches to the LLM without hitting ``SessionActorNotConfiguredError``. - Falls back to the string ``"default"`` if the session service is + Falls back to ``_FALLBACK_SESSION_ID`` if the session service is unavailable so the TUI can still launch without a database. """ import contextlib actor_name: str | None = None if persona_state is not None: - with contextlib.suppress(Exception): - actor_name = persona_state.active_persona("default").actor or None + with contextlib.suppress(ValueError, AttributeError, KeyError): + actor_name = ( + persona_state.active_persona(_PRE_SESSION_PERSONA_KEY).actor or None + ) - with contextlib.suppress(Exception): + try: container = get_container() service = container.session_service() session = service.create(actor_name=actor_name) return session.session_id - return "default" # graceful fallback + except (RuntimeError, ValueError, ImportError, AttributeError, OSError): + return _FALLBACK_SESSION_ID def run_tui(*, headless: bool = False) -> int: @@ -309,8 +316,8 @@ def run_tui(*, headless: bool = False) -> int: "persona_count": len(registry.list_personas()) if registry.personas_dir.exists() else 0, - "default_persona": state.active_name("default"), - "help": router.handle("help", session_id="default"), + "default_persona": state.active_name(_PRE_SESSION_PERSONA_KEY), + "help": router.handle("help", session_id=_PRE_SESSION_PERSONA_KEY), } print(json.dumps(payload, indent=2)) return 0