fix(session): drop empty sessions and show current first#486
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves session management by automatically cleaning up empty sessions and ensuring the current session appears first in session listings. Empty sessions are now filtered from the list view and automatically deleted on exit, helping maintain a cleaner session directory for users.
Key Changes:
- Added
is_empty()anddelete()methods to the Session class for detecting and removing empty sessions - Modified
Session.list()to filter out sessions with empty context files - Enhanced
/sessionscommand to always show the current session first, even when other sessions exist - Implemented automatic cleanup of empty sessions on CLI exit with proper metadata updates
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kimi_cli/session.py | Added is_empty() method to check for zero-byte context files, delete() method for session cleanup, and filtering logic in list() to exclude empty sessions |
| src/kimi_cli/cli.py | Added session cleanup logic on exit that deletes empty sessions and updates last_session_id metadata accordingly |
| src/kimi_cli/ui/shell/metacmd.py | Modified list_sessions to always show current session first by filtering it from Session.list() results and re-inserting at index 0 |
| tests/test_session.py | Added _write_context_message() helper and test_list_ignores_empty_sessions() test to verify empty session filtering, plus context message writes to existing tests |
| CHANGELOG.md | Documented the new session cleanup and filtering functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if session.is_empty(): | ||
| logger.info( | ||
| "Session {session_id} has empty context, removing it", | ||
| session_id=session.id, | ||
| ) | ||
| await session.delete() | ||
| if work_dir_meta.last_session_id == session.id: | ||
| work_dir_meta.last_session_id = None | ||
| else: | ||
| work_dir_meta.last_session_id = session.id |
There was a problem hiding this comment.
The session cleanup logic on exit (lines 380-389) is not covered by any tests. This is critical functionality that determines whether sessions are persisted or deleted, and whether last_session_id metadata is updated. Tests should verify the behavior for both empty and non-empty sessions, and ensure the metadata is correctly updated in both cases.
| current_session = app.soul._runtime.session | ||
| current_session_id = current_session.id | ||
| sessions = [ | ||
| session for session in await Session.list(work_dir) if session.id != current_session_id | ||
| ] | ||
|
|
||
| if not sessions: | ||
| console.print("[yellow]No sessions found.[/yellow]") | ||
| return | ||
| await current_session.refresh() | ||
| sessions.insert(0, current_session) |
There was a problem hiding this comment.
The list_sessions function now always includes the current session in the choices list (line 280), even if it's empty. While this may be intentional to show users their current session, it creates an inconsistency with the Session.list() behavior which filters out empty sessions. Consider adding a comment to clarify this intentional deviation, or add test coverage to document this behavior.
| async def delete(self) -> None: | ||
| """Delete the session directory.""" | ||
| session_dir = self.work_dir_meta.sessions_dir / self.id | ||
| if not session_dir.exists(): | ||
| return | ||
| await asyncio.to_thread(shutil.rmtree, session_dir, True) |
There was a problem hiding this comment.
The new delete() method added to the Session class is not covered by any tests. Given that deletion is a critical operation that permanently removes data, this functionality should have dedicated test coverage to ensure it correctly deletes the session directory, handles non-existent directories gracefully, and doesn't delete unintended data.
No description provided.