refactor(cli): deduplicate session list summary logic in list_sessions #3320
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Blocks
#3046 BUG-HUNT: [consistency] Duplicated session list summary logic
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!3320
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/session-list-summary-dedup"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Closes #3046.
The
list_sessionscommand was recalculatingtotal_msgs,sorted_sessions,most_recent, andoldestindependently, duplicating logic already present in_session_list_dict. This meant the summary values shown in the rich table could theoretically diverge from the JSON output if either code path was updated without updating the other.Changes
src/cleveragents/cli/commands/session.pytotal_msgs,sorted_sessions,most_recent, andoldestcalculations fromlist_sessions(lines ~327–341 in the original).summary = data["summary"]lookup —datais already the return value of_session_list_dict(sessions), which is called earlier in the same function for non-rich formats.summary["total"],summary["most_recent"],summary["oldest"],summary["total_messages"], andsummary["storage"]directly from the centralised dict.features/session_list_summary_dedup.feature(new)Six Behave scenarios verifying:
totalmatches the value shown in the rich table.total_messagesmatches the rich table.most_recentappears in the rich table summary panel.oldestappears in the rich table summary panel.Total:,Most Recent:,Oldest:,Total Messages:,Storage:).summarydict contains all required keys with correct types.features/steps/session_list_summary_dedup_steps.py(new)Step definitions for the above feature. Uses a mock
SessionServicewith two deterministic sessions (one named, one unnamed; differentupdated_attimestamps) to produce predictable summary values for cross-format comparison.Quality Gates
nox -s lint✅nox -s typecheck✅nox -s format✅nox -s unit_tests -- features/session_list_summary_dedup.feature✅ (6/6 scenarios pass)nox -s unit_tests -- features/session_list_error.feature✅ (10/10 scenarios pass — no regression)nox -s unit_tests -- features/session_cli_coverage_boost.feature✅ (35/35 scenarios pass — no regression)Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker
🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-3320-1775375100]
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-3320-1775373400]
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
Review Summary
Reviewed PR #3320 with focus on resource-management, memory-leaks, and cleanup-patterns.
This is a clean, well-scoped refactoring that eliminates duplicated summary logic in
list_sessions. The rich table summary panel now reads from the samedata["summary"]dict already computed by_session_list_dict(), ensuring JSON and rich output can never diverge.Reviewer verdict: APPROVED ✅ (posted as COMMENT due to Forgejo self-review restriction)
✅ Specification Compliance
The change correctly centralises the summary computation in
_session_list_dict(), which is the single source of truth for session list output. The rich table still renders all required fields (Total, Most Recent, Oldest, Total Messages, Storage) per spec.✅ Resource Management (Deep Dive — Focus Area)
Given special attention to resource management, memory leaks, and cleanup patterns:
sorted()call and temporary list allocation that existed in the oldlist_sessionscode. The summary is now computed once (in_session_list_dict) rather than twice. This eliminates an O(n log n) sort and associated temporary memory.summary = data["summary"]is a dict reference, not a copy. No new resources are created.patch()as a context manager, ensuring mocks are properly restored after each step. No leaked state between scenarios.CliRunner(): Lightweight, stateless — appropriate for test infrastructure. No resource concern.✅ Code Correctness
or ""fallback onsummary["most_recent"]andsummary["oldest"]is defensive but harmless — these values cannot beNonewhensessionsis non-empty (the empty case returns early at line ~280).✅ Commit & PR Metadata
refactor(cli): deduplicate session list summary logic in list_sessions✅ISSUES CLOSED: #3046✅Closes #3046✅Type/Refactorlabel present ✅✅ Test Quality
Six well-structured BDD scenarios covering:
total,total_messages,most_recent,oldestTests use deterministic fixtures (fixed ULIDs, fixed timestamps) ensuring reproducible results. The mock pattern (inline
patch+MagicMock(spec=SessionService)) follows the established codebase convention.Minor Suggestions (Non-blocking)
Missing milestone: The linked issue #3046 is assigned to milestone
v3.7.0, but this PR has no milestone set. Per CONTRIBUTING.md, PRs should be assigned to the same milestone as their linked issue. The implementation worker should set this before merge.Pre-existing: duplicate print in
showcommand (not introduced by this PR): Theshowcommand has two identicalconsole.print("[green bold]✓ OK[/green bold] Session details loaded")lines at the end. Consider filing a separate issue to clean this up.Pre-existing:
contextlib.suppress(Exception)increatecommand (not introduced by this PR): Thecreatecommand silently swallows all exceptions from the facade dispatch call. This conflicts with the project's "errors must not be suppressed" principle. Consider filing a separate issue.Decision: APPROVED ✅
The code change is correct, well-tested, and improves resource efficiency by eliminating redundant computation. The missing milestone is a metadata issue that should be fixed before merge but does not warrant blocking the review.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Review Summary (Independent Review — Error Handling & Edge Cases Focus)
Reviewed PR #3320 with focus on error-handling-patterns, edge-cases, and boundary-conditions.
Reviewer verdict: APPROVED ✅ (posted as COMMENT due to Forgejo self-review restriction)
This is a well-scoped refactoring that eliminates duplicated summary computation in
list_sessions. The rich table summary panel now reads from the centraliseddata["summary"]dict already computed by_session_list_dict(), ensuring JSON and rich output can never diverge.✅ Specification Compliance
The change correctly centralises the summary computation in
_session_list_dict(), which becomes the single source of truth for session list output. The rich table still renders all five required fields (Total, Most Recent, Oldest, Total Messages, Storage) per spec.✅ Error Handling Patterns (Deep Dive — Focus Area)
Given special attention to error handling:
data["summary"]. Since_session_list_dict()always returns a dict with a"summary"key containing all required sub-keys (total,most_recent,oldest,total_messages,storage), there is noKeyErrorrisk.DatabaseErrorandSessionNotFoundErrorcatch blocks inlist_sessionsare untouched. The early return for empty sessions (if not sessions: ... return) still guards against the None-valued summary fields.or ""fallback is safe. Thesummary["most_recent"] or ""andsummary["oldest"] or ""expressions are defensive — whensessionsis non-empty (the only case that reaches this code),_session_list_dictalways produces non-None string values for these keys. The fallback can never trigger in practice, but it's harmless and guards against future changes to_session_list_dict.✅ Edge Cases & Boundary Conditions (Deep Dive — Focus Area)
Examined the following edge cases:
len(sessions) == 1,sorted_sessions[0]andsorted_sessions[-1]are the same object. Bothmost_recentandoldestresolve to the same session name/ID. This is correct and unchanged by the refactoring.name=None: Falls back tosession_id[:8]in_session_list_dict. The test fixture includes one named session and one unnamed session (name=None), which exercises this fallback path for theoldestfield. Good coverage.updated_at: Python'ssorted()is stable, so ordering is deterministic. Unchanged.if not sessions:guard before_session_list_dictis called. The empty-list JSON output ({"sessions": [], "total": 0}) is preserved. Note: this output structure differs from_session_list_dict's return shape (which uses"summary"instead of"total"), but this is pre-existing behavior, not introduced by this PR.message_countcomputation:_session_list_dictsumss.message_countacross all sessions. The test fixture uses sessions with 3 and 2 messages respectively, sototal_messagesshould be 5. This is verified by the cross-format consistency test.✅ Code Correctness
summary = data["summary"]is a dict reference (not a copy), so no unnecessary allocations.sorted()call and temporary list from the old code are eliminated, providing a minor performance improvement.✅ Commit & PR Metadata
refactor(cli): deduplicate session list summary logic in list_sessions✅ISSUES CLOSED: #3046✅Closes #3046✅Type/Refactorlabel ✅# type: ignoresuppressions ✅✅ Test Quality
Six well-structured BDD scenarios covering:
total,total_messages,most_recent,oldest(4 scenarios)Tests use deterministic fixtures (fixed ULIDs, fixed timestamps
2025-08-01and2025-07-01) ensuring reproducible results. The mock pattern (patch()context manager +MagicMock(spec=SessionService)) follows established codebase conventions.Minor Suggestions (Non-blocking)
Missing edge-case test for empty sessions list: While the empty case is handled by the early return and isn't affected by this refactoring, a scenario verifying that
list_sessionswith zero sessions produces the correct empty-list output would strengthen the test suite. Consider adding in a follow-up.Pre-existing: duplicate "✓ OK" print in
showcommand (not introduced by this PR): Theshowcommand on the branch has two identicalconsole.print("[green bold]✓ OK[/green bold] Session details loaded")lines. This appears to be a pre-existing issue — consider filing a separate bug.Pre-existing:
contextlib.suppress(Exception)increatecommand (not introduced by this PR): Thecreatecommand silently swallows all exceptions from the facade dispatch call. This conflicts with the project's fail-fast error handling principle. Consider filing a separate issue.Decision: APPROVED ✅
The refactoring is correct, well-tested, and achieves its stated goal of eliminating duplicated summary logic. Error handling is preserved, edge cases are properly handled, and the test coverage is meaningful.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer