feat: support acp session listing and loading#456
Conversation
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
|
This change is part of the following stack: Change managed by git-spice. |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for ACP session continuation by implementing session listing and loading capabilities. It introduces a new multi-session ACP server alongside the existing single-session implementation, enabling clients to discover and resume previous sessions.
Key Changes:
- Adds
Session.find()andSession.list()methods to enable session discovery and retrieval - Creates a new
src/kimi_cli/acp/package with a multi-sessionACPServerthat supports session listing and loading - Refactors the existing ACP implementation into
ACPServerSingleSessionfor backward compatibility - Extends the
Sessiondataclass withtitleandupdated_atfields to support session metadata in ACP responses
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/conftest.py | Updates session fixture to include new required fields (title, updated_at) |
| src/kimi_cli/session.py | Adds find() and list() static methods for session discovery; adds title and updated_at fields to Session dataclass; adds context file existence check in continue_() |
| src/kimi_cli/cli.py | Converts main command to callback to support subcommands; adds new 'acp' subcommand for multi-session server |
| src/kimi_cli/acp/init.py | New entry point for multi-session ACP server with unstable protocol support |
| src/kimi_cli/acp/types.py | Defines type aliases for ACP content blocks and MCP server types |
| src/kimi_cli/acp/session.py | Implements ACPSession class extracted from original ACP implementation; handles wire message streaming and conversion |
| src/kimi_cli/acp/server.py | Implements ACPServer with session management, listing, and loading capabilities |
| src/kimi_cli/acp/convert.py | Provides conversion utilities between ACP content blocks and internal content parts |
| src/kimi_cli/ui/acp/init.py | Refactors original ACP class to ACPServerSingleSession; maintains backward compatibility for single-session mode |
| pyrightconfig.json | Adds new acp package to strict type checking |
| CHANGELOG.md | Documents the new session continuation feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/kimi_cli/acp/convert.py
Outdated
| case acp.schema.TextContentBlock(): | ||
| content.append(TextPart(text=block.text)) | ||
| case acp.schema.ImageContentBlock(): | ||
| content.append(ImageURLPart(image_url=ImageURLPart.ImageURL(url=""))) # TODO |
There was a problem hiding this comment.
The ImageContentBlock case creates an ImageURLPart with an empty URL string, which could cause issues downstream when the image is processed. The acp.schema.ImageContentBlock likely contains image data (either as a URL or data) that should be properly extracted and used. Additionally, according to the initialize method in init.py line 55, image capability is set to True, meaning this functionality is advertised as supported but not properly implemented.
| content.append(ImageURLPart(image_url=ImageURLPart.ImageURL(url=""))) # TODO | |
| # Attempt to extract image URL from the block | |
| image_url = getattr(block, "url", None) | |
| if image_url and isinstance(image_url, str) and image_url.strip(): | |
| content.append(ImageURLPart(image_url=ImageURLPart.ImageURL(url=image_url))) | |
| else: | |
| logger.warning("ImageContentBlock missing valid URL: {block}", block=block) |
|
|
||
| @cli.command() | ||
| def acp(): | ||
| """Run Kimi CLI ACP server.""" |
There was a problem hiding this comment.
The docstring for the acp subcommand is vague. It should clarify that this runs the full multi-session ACP server (as opposed to the single-session --acp mode available in the main command). Consider updating to something like "Run Kimi CLI multi-session ACP server with session listing and loading support."
| """Run Kimi CLI ACP server.""" | |
| """Run Kimi CLI multi-session ACP server with session listing and loading support.""" |
src/kimi_cli/acp/session.py
Outdated
| logger.warning("Max steps reached: {n_steps}", n_steps=e.n_steps) | ||
| return acp.PromptResponse(stop_reason="max_turn_requests") | ||
| except RunCancelled: | ||
| logger.error("Prompt cancelled by user") |
There was a problem hiding this comment.
The logging for RunCancelled error is inconsistent with other exception handlers. Similar to LLMNotSet, this should use logger.info() instead of logger.error() since cancellation is a normal user action, not an error condition. This is consistent with the pattern in ui/print/init.py:104 which logs "Interrupted by user" at the info level.
| logger.error("Prompt cancelled by user") | |
| logger.info("Prompt cancelled by user") |
| async def find(work_dir: KaosPath, session_id: str) -> Session | None: | ||
| """Find a session by work directory and session ID.""" | ||
| work_dir = work_dir.canonical() | ||
| logger.debug( | ||
| "Finding session for work directory: {work_dir}, session ID: {session_id}", | ||
| work_dir=work_dir, | ||
| session_id=session_id, | ||
| ) | ||
|
|
||
| metadata = load_metadata() | ||
| work_dir_meta = metadata.get_work_dir_meta(work_dir) | ||
| if work_dir_meta is None: | ||
| logger.debug("Work directory never been used") | ||
| return None | ||
|
|
||
| _migrate_session_context_file(work_dir_meta, session_id) | ||
|
|
||
| session_dir = work_dir_meta.sessions_dir / session_id | ||
| if not session_dir.is_dir(): | ||
| logger.debug("Session directory not found: {session_dir}", session_dir=session_dir) | ||
| return None | ||
|
|
||
| context_file = session_dir / "context.jsonl" | ||
| if not context_file.exists(): | ||
| logger.debug( | ||
| "Session context file not found: {context_file}", context_file=context_file | ||
| ) | ||
| return None | ||
|
|
||
| return Session( | ||
| id=session_id, | ||
| work_dir=work_dir, | ||
| work_dir_meta=work_dir_meta, | ||
| context_file=context_file, | ||
| title=session_id, # TODO: readable session titles | ||
| updated_at=context_file.stat().st_mtime, | ||
| ) |
There was a problem hiding this comment.
The new Session.find() method lacks test coverage. Given that tests/test_session.py exists with test infrastructure and other session functionality would benefit from automated testing, this new method should have tests to verify it correctly handles cases like: finding an existing session, handling missing session directories, handling missing context files, and proper migration of old session files.
| async def list(work_dir: KaosPath) -> list[Session]: | ||
| """List all sessions for a work directory.""" | ||
| work_dir = work_dir.canonical() | ||
| logger.debug("Listing sessions for work directory: {work_dir}", work_dir=work_dir) | ||
|
|
||
| metadata = load_metadata() | ||
| work_dir_meta = metadata.get_work_dir_meta(work_dir) | ||
| if work_dir_meta is None: | ||
| logger.debug("Work directory never been used") | ||
| return [] | ||
|
|
||
| session_ids = { | ||
| path.name if path.is_dir() else path.stem | ||
| for path in work_dir_meta.sessions_dir.iterdir() | ||
| if path.is_dir() or path.suffix == ".jsonl" | ||
| } | ||
|
|
||
| sessions: list[Session] = [] | ||
| for session_id in sorted(session_ids): | ||
| _migrate_session_context_file(work_dir_meta, session_id) | ||
| session_dir = work_dir_meta.sessions_dir / session_id | ||
| if not session_dir.is_dir(): | ||
| logger.debug("Session directory not found: {session_dir}", session_dir=session_dir) | ||
| continue | ||
| context_file = session_dir / "context.jsonl" | ||
| if not context_file.exists(): | ||
| logger.debug( | ||
| "Session context file not found: {context_file}", context_file=context_file | ||
| ) | ||
| continue | ||
| sessions.append( | ||
| Session( | ||
| id=session_id, | ||
| work_dir=work_dir, | ||
| work_dir_meta=work_dir_meta, | ||
| context_file=context_file, | ||
| title=session_id, # TODO: readable session titles | ||
| updated_at=context_file.stat().st_mtime, | ||
| ) | ||
| ) | ||
| return sessions |
There was a problem hiding this comment.
The new Session.list() method lacks test coverage. This method has complex logic including session discovery, filtering by file extensions, and migration handling. Tests should verify it correctly: lists all sessions in a directory, handles empty directories, properly migrates old session files, and correctly extracts session metadata including timestamps.
src/kimi_cli/session.py
Outdated
| work_dir_meta=work_dir_meta, | ||
| context_file=context_file, | ||
| title=session_id, # TODO: readable session titles | ||
| updated_at=datetime.now().timestamp(), |
There was a problem hiding this comment.
The updated_at timestamp should be set consistently with how it's retrieved in other methods (find, list, continue_). Those methods use context_file.stat().st_mtime, but here in create() it uses datetime.now().timestamp(). While functionally similar, this inconsistency could cause confusion. Consider creating the context_file first (via touch()) and then using its st_mtime for consistency, or document why the difference is acceptable.
| updated_at=datetime.now().timestamp(), | |
| updated_at=context_file.stat().st_mtime, |
Signed-off-by: Richard Chien <stdrc@outlook.com>
Signed-off-by: Richard Chien <stdrc@outlook.com>
Related Issue
Resolve #(issue_number)
Description