From 9210fbce33aa536476ad6d75612a5430fa0aa415 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 5 Apr 2026 09:38:43 +0000 Subject: [PATCH] refactor(cli): deduplicate session list summary logic in list_sessions Refactor list_sessions to reuse the summary dict already computed by _session_list_dict instead of recalculating total_msgs, sorted_sessions, most_recent, and oldest independently. The rich table summary panel now reads directly from data["summary"], ensuring JSON and rich table output are always consistent and the summary logic lives in exactly one place (_session_list_dict). Add Behave feature (session_list_summary_dedup.feature) with 6 scenarios verifying that JSON and rich table summary values are consistent across all required fields: total, most_recent, oldest, total_messages, storage. ISSUES CLOSED: #3046 --- features/session_list_summary_dedup.feature | 44 +++ .../steps/session_list_summary_dedup_steps.py | 251 ++++++++++++++++++ src/cleveragents/cli/commands/session.py | 19 +- 3 files changed, 303 insertions(+), 11 deletions(-) create mode 100644 features/session_list_summary_dedup.feature create mode 100644 features/steps/session_list_summary_dedup_steps.py diff --git a/features/session_list_summary_dedup.feature b/features/session_list_summary_dedup.feature new file mode 100644 index 000000000..286fbecdd --- /dev/null +++ b/features/session_list_summary_dedup.feature @@ -0,0 +1,44 @@ +@tdd_issue @tdd_issue_3046 +Feature: Session list summary consistency between JSON and rich table output + As a developer + I want the session list summary values to be consistent between JSON and rich table output + So that the deduplicated summary logic in _session_list_dict is the single source of truth + + Background: + Given a session-list-summary mock service with two sessions + + Scenario: JSON output summary matches rich table summary for total count + When I invoke session list with JSON format + And I invoke session list with rich format + Then the JSON summary total should match the rich table total + + Scenario: JSON output summary matches rich table summary for total messages + When I invoke session list with JSON format + And I invoke session list with rich format + Then the JSON summary total_messages should match the rich table total messages + + Scenario: JSON output summary matches rich table summary for most recent session + When I invoke session list with JSON format + And I invoke session list with rich format + Then the JSON summary most_recent should appear in the rich table summary panel + + Scenario: JSON output summary matches rich table summary for oldest session + When I invoke session list with JSON format + And I invoke session list with rich format + Then the JSON summary oldest should appear in the rich table summary panel + + Scenario: Rich table summary panel contains all required fields + When I invoke session list with rich format + Then the rich table output contains "Total:" + And the rich table output contains "Most Recent:" + And the rich table output contains "Oldest:" + And the rich table output contains "Total Messages:" + And the rich table output contains "Storage:" + + Scenario: JSON output summary contains all required fields + When I invoke session list with JSON format + Then the JSON output has a summary key with total + And the JSON output has a summary key with most_recent + And the JSON output has a summary key with oldest + And the JSON output has a summary key with total_messages + And the JSON output has a summary key with storage diff --git a/features/steps/session_list_summary_dedup_steps.py b/features/steps/session_list_summary_dedup_steps.py new file mode 100644 index 000000000..1be2bf796 --- /dev/null +++ b/features/steps/session_list_summary_dedup_steps.py @@ -0,0 +1,251 @@ +"""Step definitions for session_list_summary_dedup.feature (issue #3046). + +Verifies that the summary values in the JSON output and the rich table output +are consistent — i.e. that the deduplicated ``_session_list_dict`` helper is +the single source of truth for both rendering paths. +""" + +from __future__ import annotations + +import json +from datetime import UTC, datetime +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.session import app as session_app +from cleveragents.domain.models.core.session import ( + MessageRole, + Session, + SessionMessage, + SessionService, + SessionTokenUsage, +) + +_runner = CliRunner() + +# Stable ULIDs for deterministic test output +_ULID_NEWER = "01SCVBST000000000000000001" +_ULID_OLDER = "01SCVBST000000000000000002" + +# Newer session updated more recently +_DT_NEWER = datetime(2025, 8, 1, 12, 0, 0, tzinfo=UTC) +_DT_OLDER = datetime(2025, 7, 1, 10, 0, 0, tzinfo=UTC) + + +def _make_session( + session_id: str, + actor_name: str | None, + updated_at: datetime, + message_count: int = 0, + name: str | None = None, +) -> Session: + messages = [ + SessionMessage( + message_id=f"01SCVBST0000000000000000{i + 1:02d}", + role=MessageRole.USER, + content=f"msg {i}", + sequence=i, + timestamp=updated_at, + ) + for i in range(message_count) + ] + return Session( + session_id=session_id, + actor_name=actor_name, + namespace="local", + messages=messages, + linked_plan_ids=[], + token_usage=SessionTokenUsage(), + created_at=updated_at, + updated_at=updated_at, + metadata={}, + cost_budget=None, + name=name, + ) + + +def _mock_service_with_sessions() -> MagicMock: + svc = MagicMock(spec=SessionService) + sessions = [ + _make_session( + session_id=_ULID_NEWER, + actor_name="openai/gpt-4", + updated_at=_DT_NEWER, + message_count=3, + name="newer-session", + ), + _make_session( + session_id=_ULID_OLDER, + actor_name=None, + updated_at=_DT_OLDER, + message_count=2, + name=None, + ), + ] + svc.list.return_value = sessions + return svc + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a session-list-summary mock service with two sessions") +def step_setup_mock_service(context: Context) -> None: + context.sls_svc = _mock_service_with_sessions() + context.sls_json_result = None + context.sls_rich_result = None + + +# --------------------------------------------------------------------------- +# When +# --------------------------------------------------------------------------- + + +@when("I invoke session list with JSON format") +def step_invoke_json(context: Context) -> None: + with patch( + "cleveragents.cli.commands.session._get_session_service", + return_value=context.sls_svc, + ): + context.sls_json_result = _runner.invoke( + session_app, ["list", "--format", "json"] + ) + + +@when("I invoke session list with rich format") +def step_invoke_rich(context: Context) -> None: + with patch( + "cleveragents.cli.commands.session._get_session_service", + return_value=context.sls_svc, + ): + context.sls_rich_result = _runner.invoke( + session_app, ["list", "--format", "rich"] + ) + + +# --------------------------------------------------------------------------- +# Then — cross-format consistency +# --------------------------------------------------------------------------- + + +def _get_json_summary(context: Context) -> dict: + result = context.sls_json_result + assert result is not None, "JSON result not set — invoke JSON format first" + assert result.exit_code == 0, ( + f"JSON command failed (exit {result.exit_code}):\n{result.output}" + ) + data = json.loads(result.output) + assert "summary" in data, f"No 'summary' key in JSON output: {data}" + return data["summary"] + + +def _get_rich_output(context: Context) -> str: + result = context.sls_rich_result + assert result is not None, "Rich result not set — invoke rich format first" + assert result.exit_code == 0, ( + f"Rich command failed (exit {result.exit_code}):\n{result.output}" + ) + return result.output + + +@then("the JSON summary total should match the rich table total") +def step_total_matches(context: Context) -> None: + summary = _get_json_summary(context) + rich_output = _get_rich_output(context) + expected_total = str(summary["total"]) + assert "Total:" in rich_output, f"'Total:' not found in rich output:\n{rich_output}" + assert expected_total in rich_output, ( + f"Expected total '{expected_total}' not found in rich output:\n{rich_output}" + ) + + +@then("the JSON summary total_messages should match the rich table total messages") +def step_total_messages_matches(context: Context) -> None: + summary = _get_json_summary(context) + rich_output = _get_rich_output(context) + expected = str(summary["total_messages"]) + assert "Total Messages:" in rich_output, ( + f"'Total Messages:' not found in rich output:\n{rich_output}" + ) + assert expected in rich_output, ( + f"Expected total_messages '{expected}' not found in rich output:\n{rich_output}" + ) + + +@then("the JSON summary most_recent should appear in the rich table summary panel") +def step_most_recent_matches(context: Context) -> None: + summary = _get_json_summary(context) + rich_output = _get_rich_output(context) + most_recent = summary["most_recent"] + assert most_recent is not None, "most_recent is None in JSON summary" + assert most_recent in rich_output, ( + f"Expected most_recent '{most_recent}' not found in rich output:\n{rich_output}" + ) + + +@then("the JSON summary oldest should appear in the rich table summary panel") +def step_oldest_matches(context: Context) -> None: + summary = _get_json_summary(context) + rich_output = _get_rich_output(context) + oldest = summary["oldest"] + assert oldest is not None, "oldest is None in JSON summary" + assert oldest in rich_output, ( + f"Expected oldest '{oldest}' not found in rich output:\n{rich_output}" + ) + + +# --------------------------------------------------------------------------- +# Then — rich table field presence +# --------------------------------------------------------------------------- + + +@then('the rich table output contains "{text}"') +def step_rich_contains(context: Context, text: str) -> None: + rich_output = _get_rich_output(context) + assert text in rich_output, f"Expected '{text}' in rich output:\n{rich_output}" + + +# --------------------------------------------------------------------------- +# Then — JSON summary field presence +# --------------------------------------------------------------------------- + + +@then("the JSON output has a summary key with total") +def step_json_has_total(context: Context) -> None: + summary = _get_json_summary(context) + assert "total" in summary, f"'total' not in summary: {summary}" + assert isinstance(summary["total"], int), ( + f"'total' should be int, got {type(summary['total'])}" + ) + + +@then("the JSON output has a summary key with most_recent") +def step_json_has_most_recent(context: Context) -> None: + summary = _get_json_summary(context) + assert "most_recent" in summary, f"'most_recent' not in summary: {summary}" + + +@then("the JSON output has a summary key with oldest") +def step_json_has_oldest(context: Context) -> None: + summary = _get_json_summary(context) + assert "oldest" in summary, f"'oldest' not in summary: {summary}" + + +@then("the JSON output has a summary key with total_messages") +def step_json_has_total_messages(context: Context) -> None: + summary = _get_json_summary(context) + assert "total_messages" in summary, f"'total_messages' not in summary: {summary}" + assert isinstance(summary["total_messages"], int), ( + f"'total_messages' should be int, got {type(summary['total_messages'])}" + ) + + +@then("the JSON output has a summary key with storage") +def step_json_has_storage(context: Context) -> None: + summary = _get_json_summary(context) + assert "storage" in summary, f"'storage' not in summary: {summary}" diff --git a/src/cleveragents/cli/commands/session.py b/src/cleveragents/cli/commands/session.py index 4c00643f2..2ec14af28 100644 --- a/src/cleveragents/cli/commands/session.py +++ b/src/cleveragents/cli/commands/session.py @@ -324,21 +324,18 @@ def list_sessions( console.print(table) console.print() - # Summary panel with all required fields - total_msgs = sum(s.message_count for s in sessions) - # Find most recent and oldest sessions - sorted_sessions = sorted(sessions, key=lambda x: x.updated_at, reverse=True) - most_recent = sorted_sessions[0].name or sorted_sessions[0].session_id[:8] - oldest = sorted_sessions[-1].name or sorted_sessions[-1].session_id[:8] + # Reuse the summary dict already computed by _session_list_dict to avoid + # duplicating the total_msgs / sorted_sessions / most_recent / oldest logic. + summary = data["summary"] summary_table = Table.grid(padding=(0, 1)) summary_table.add_column(style="cyan bold", justify="left") summary_table.add_column(style="white", justify="left") - summary_table.add_row("Total:", str(len(sessions))) - summary_table.add_row("Most Recent:", most_recent) - summary_table.add_row("Oldest:", oldest) - summary_table.add_row("Total Messages:", str(total_msgs)) - summary_table.add_row("Storage:", "0 KB") # Placeholder + summary_table.add_row("Total:", str(summary["total"])) + summary_table.add_row("Most Recent:", summary["most_recent"] or "") + summary_table.add_row("Oldest:", summary["oldest"] or "") + summary_table.add_row("Total Messages:", str(summary["total_messages"])) + summary_table.add_row("Storage:", summary["storage"]) console.print(Panel(summary_table, title="Summary", border_style="blue")) console.print()