fix(cli): log suppressed facade dispatch exceptions in session create #10898
@@ -0,0 +1,150 @@
|
||||
"""Step definitions for tdd_session_create_suppress_exception.feature.
|
||||
|
||||
This test captures bug #10414: ``session create`` in
|
||||
``src/cleveragents/cli/commands/session.py`` used
|
||||
``contextlib.suppress(Exception)`` to silently discard ALL exceptions raised
|
||||
by ``_facade_dispatch()`` without any logging. Programming errors,
|
||||
unexpected failures, and infrastructure issues in the facade dispatch were
|
||||
completely invisible.
|
||||
|
||||
The fix replaces the suppress block with a ``try/except Exception`` that
|
||||
calls ``_log.warning(..., exc_info=True)`` so that the exception is
|
||||
recorded but the session creation remains non-fatal.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import datetime
|
||||
import logging
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from behave import given, then, when
|
||||
from behave.runner import Context
|
||||
from typer.testing import CliRunner
|
||||
|
||||
from cleveragents.cli.commands import session as session_mod
|
||||
from cleveragents.cli.commands.session import app as session_app
|
||||
from cleveragents.domain.models.core.session import Session
|
||||
|
||||
# A distinctive error message to confirm the right exception was raised.
|
||||
_DISPATCH_ERROR_MESSAGE = "facade dispatch failed for tdd_10414"
|
||||
|
||||
|
||||
def _make_mock_session() -> MagicMock:
|
||||
"""Build a minimal mock Session object sufficient for the create command."""
|
||||
|
|
||||
mock_session = MagicMock(spec=Session)
|
||||
|
HAL9001
commented
Suggestion: replace the Also consider moving Suggestion: replace the `__import__("datetime")` calls on lines 35-36 with a standard `import datetime` at the top of the file. Using `__import__()` directly is a SAST code smell (bandit B406) and is flagged as unusual by linters — it may be the cause of the CI lint failure. The fix is trivial:
```python
import datetime
...
mock_session.created_at = datetime.datetime(2026, 1, 1, 0, 0, 0)
mock_session.updated_at = datetime.datetime(2026, 1, 1, 0, 0, 0)
```
Also consider moving `_DISPATCH_ERROR_MESSAGE` into the step function as a local constant to improve test isolation. This is a minor style suggestion and not blocking.
|
||||
mock_session.session_id = "01JTEST000000000000000000000"
|
||||
mock_session.actor_name = None
|
||||
mock_session.namespace = "default"
|
||||
mock_session.message_count = 0
|
||||
mock_session.created_at = datetime(2026, 1, 1, 0, 0, 0)
|
||||
mock_session.updated_at = datetime(2026, 1, 1, 0, 0, 0)
|
||||
return mock_session
|
||||
|
||||
|
||||
@given("a session create command with a mocked session service")
|
||||
def step_given_mocked_session_service(context: Context) -> None:
|
||||
"""Set up a CLI runner with a mocked session service.
|
||||
|
||||
The mock service returns a valid session so that the create command
|
||||
proceeds past the service call and reaches the ``_facade_dispatch``
|
||||
try/except block.
|
||||
"""
|
||||
context.runner = CliRunner()
|
||||
|
||||
mock_service = MagicMock()
|
||||
mock_service.create.return_value = _make_mock_session()
|
||||
|
||||
# Patch the module-level service so the CLI uses our mock.
|
||||
context._orig_service = session_mod._service
|
||||
session_mod._service = mock_service
|
||||
|
||||
def _cleanup() -> None:
|
||||
session_mod._service = context._orig_service
|
||||
|
||||
context.add_cleanup(_cleanup)
|
||||
|
||||
|
||||
@given("the facade dispatch is patched to raise a RuntimeError")
|
||||
def step_given_facade_dispatch_raises(context: Context) -> None:
|
||||
"""Patch ``_facade_dispatch`` so it always raises a RuntimeError.
|
||||
|
||||
This simulates a real failure in the facade layer (e.g. the A2A
|
||||
bootstrap is unavailable) and exercises the ``try/except`` block in
|
||||
the ``create`` command.
|
||||
"""
|
||||
context._facade_patcher = patch(
|
||||
"cleveragents.cli.commands.session._facade_dispatch",
|
||||
side_effect=RuntimeError(_DISPATCH_ERROR_MESSAGE),
|
||||
)
|
||||
context._facade_patcher.start()
|
||||
|
||||
def _cleanup() -> None:
|
||||
context._facade_patcher.stop()
|
||||
|
||||
context.add_cleanup(_cleanup)
|
||||
|
||||
|
||||
@when("I invoke the session create command via the CLI runner")
|
||||
def step_when_invoke_create(context: Context) -> None:
|
||||
"""Invoke ``session create`` and capture log records during the call."""
|
||||
# Capture WARNING-level log records from the session module logger.
|
||||
session_logger = logging.getLogger("cleveragents.cli.commands.session")
|
||||
handler = _CapturingHandler()
|
||||
handler.setLevel(logging.WARNING)
|
||||
session_logger.addHandler(handler)
|
||||
session_logger.setLevel(logging.WARNING)
|
||||
|
||||
try:
|
||||
context.result = context.runner.invoke(session_app, ["create"])
|
||||
finally:
|
||||
session_logger.removeHandler(handler)
|
||||
|
||||
context.captured_log_records = handler.records
|
||||
|
||||
|
||||
class _CapturingHandler(logging.Handler):
|
||||
"""A logging handler that stores all emitted records in a list."""
|
||||
|
||||
def __init__(self) -> None:
|
||||
super().__init__()
|
||||
self.records: list[logging.LogRecord] = []
|
||||
|
||||
def emit(self, record: logging.LogRecord) -> None:
|
||||
self.records.append(record)
|
||||
|
||||
|
||||
@then("a WARNING log entry should have been emitted for the facade dispatch failure")
|
||||
def step_then_warning_logged(context: Context) -> None:
|
||||
"""Assert that a WARNING log entry was emitted when the facade dispatch raised.
|
||||
|
||||
The fix replaces ``contextlib.suppress(Exception)`` with a
|
||||
``try/except Exception`` block that calls ``_log.warning(..., exc_info=True)``.
|
||||
This assertion verifies the fix is in place.
|
||||
"""
|
||||
# The session create command should still succeed (non-fatal).
|
||||
assert context.result.exit_code == 0, (
|
||||
f"Expected session create to exit 0 even when facade dispatch fails, "
|
||||
f"but got exit code {context.result.exit_code}.\n"
|
||||
f"Output:\n{context.result.output}"
|
||||
)
|
||||
|
||||
# Assert that a WARNING log record was emitted.
|
||||
warning_records = [
|
||||
r for r in context.captured_log_records if r.levelno >= logging.WARNING
|
||||
]
|
||||
assert warning_records, (
|
||||
f"Bug #10414: No WARNING log record was emitted when _facade_dispatch() "
|
||||
f"raised a RuntimeError inside the try/except block. "
|
||||
f"Captured records: {context.captured_log_records}"
|
||||
)
|
||||
|
||||
# Assert the log record references the facade dispatch failure.
|
||||
all_messages = " ".join(r.getMessage() for r in warning_records)
|
||||
assert _DISPATCH_ERROR_MESSAGE in all_messages or any(
|
||||
r.exc_info is not None for r in warning_records
|
||||
), (
|
||||
f"Bug #10414: WARNING log record was found but did not contain the "
|
||||
f"expected error message {_DISPATCH_ERROR_MESSAGE!r} or exc_info. "
|
||||
f"Log messages: {all_messages!r}"
|
||||
)
|
||||
@@ -0,0 +1,25 @@
|
||||
@tdd_issue @tdd_issue_10414
|
||||
Feature: TDD Issue #10414 — session create silently suppresses facade dispatch exceptions without logging
|
||||
As a developer debugging a session creation failure
|
||||
I want exceptions from _facade_dispatch() to be logged at WARNING level
|
||||
So that I can diagnose why the facade layer failed without silent data loss
|
||||
|
||||
The ``session create`` command in
|
||||
``src/cleveragents/cli/commands/session.py`` previously used
|
||||
``contextlib.suppress(Exception)`` to silently discard ALL exceptions
|
||||
raised by ``_facade_dispatch()``. There was no logging call before or
|
||||
after the suppress block, making it impossible to diagnose failures in
|
||||
the facade layer.
|
||||
|
||||
The fix replaces the suppress block with a ``try/except Exception`` that
|
||||
calls ``_log.warning(..., exc_info=True)`` so that the exception is
|
||||
recorded but the session creation remains non-fatal.
|
||||
|
||||
See CONTRIBUTING.md > Bug Fix Workflow > TDD Issue Test Tags.
|
||||
|
||||
@tdd_issue @tdd_issue_10414
|
||||
Scenario: Bug #10414 — session create logs a warning when facade dispatch raises
|
||||
Given a session create command with a mocked session service
|
||||
And the facade dispatch is patched to raise a RuntimeError
|
||||
When I invoke the session create command via the CLI runner
|
||||
Then a WARNING log entry should have been emitted for the facade dispatch failure
|
||||
@@ -213,13 +213,17 @@ def create(
|
||||
# Notify the facade layer for A2A protocol bookkeeping.
|
||||
# Pass session_id so the facade handler acknowledges the already-
|
||||
# persisted session instead of creating a duplicate (#1141).
|
||||
import contextlib
|
||||
|
||||
with contextlib.suppress(Exception):
|
||||
try:
|
||||
_facade_dispatch(
|
||||
"session.create",
|
||||
{"actor_name": actor or "", "session_id": session.session_id},
|
||||
)
|
||||
except Exception as _exc:
|
||||
_log.warning(
|
||||
"session_create_facade_dispatch_failed",
|
||||
extra={"session_id": session.session_id, "error": str(_exc)},
|
||||
exc_info=True,
|
||||
)
|
||||
|
||||
data = _session_summary_dict(session)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user
Blocking: This file uses
__import__("datetime").datetime(...)to construct datetime values (lines 34-35). Per the project Python import rules (ALL imports at top of file,from X import Yformat), this is not permitted.Fix: Add
from datetime import datetimeat the top of the file (afterfrom __future__ import annotations) and replace both__import__("datetime").datetime(...)calls withdatetime(...).This causes the CI lint failure.