fix(cli): log suppressed facade dispatch exceptions in session create #10898

Merged
HAL9000 merged 3 commits from bugfix/m6-session-create-suppress-exception-logging into master 2026-05-07 19:37:18 +00:00
3 changed files with 182 additions and 3 deletions
@@ -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."""
Outdated
Review

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 Y format), this is not permitted.

Fix: Add from datetime import datetime at the top of the file (after from __future__ import annotations) and replace both __import__("datetime").datetime(...) calls with datetime(...).

This causes the CI lint failure.

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 Y` format), this is not permitted. Fix: Add `from datetime import datetime` at the top of the file (after `from __future__ import annotations`) and replace both `__import__("datetime").datetime(...)` calls with `datetime(...)`. This causes the CI lint failure.
mock_session = MagicMock(spec=Session)
Outdated
Review

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:

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.

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
+7 -3
View File
1
@@ -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)