Conversation
Signed-off-by: Richard Chien <stdrc@outlook.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for named sessions in the Kimi CLI, allowing users to create and reference sessions with custom identifiers instead of only using auto-generated UUIDs. The library now supports optional session IDs, and the CLI automatically creates a new session when a specified session ID doesn't exist, improving the user experience.
Key changes:
- Modified
Session.create()to accept an optionalsession_idparameter that defaults to UUID generation - Changed CLI behavior to automatically create sessions when a specified session ID is not found, rather than raising an error
- Added test coverage for the basic named session creation and retrieval functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kimi_cli/session.py | Added optional session_id parameter to Session.create() method, defaulting to UUID generation when not provided |
| src/kimi_cli/cli.py | Changed session not found behavior from raising an error to automatically creating a new session with the specified ID |
| tests/test_session.py | Added test case for creating and finding a named session |
| CHANGELOG.md | Documented the new features in the unreleased section |
| AGENTS.md | Added comprehensive development workflow documentation including commit message style, quality checks, and testing conventions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if session_id is None: | ||
| session_id = str(uuid.uuid4()) | ||
| session_dir = work_dir_meta.sessions_dir / session_id |
There was a problem hiding this comment.
The session_id parameter accepts arbitrary strings without validation, which creates a security vulnerability. A malicious user could pass session IDs like "../../../sensitive_data" or use absolute paths to create session directories outside the intended sessions_dir. This could lead to path traversal attacks where session data is written to unintended locations.
Consider adding validation to ensure the session_id:
- Does not contain path separators (/, )
- Does not contain parent directory references (..)
- Is not empty after stripping whitespace
- Does not start with a dot to avoid creating hidden directories unintentionally
The CLI already validates that session_id is not empty (cli.py:233-235), but path traversal validation is still missing.
| async def test_create_named_session(isolated_share_dir: Path, work_dir: KaosPath): | ||
| session_id = "my-named-session" | ||
| session = await Session.create(work_dir, session_id) | ||
| assert session.id == session_id | ||
| assert session.dir.name == session_id | ||
|
|
||
| # Verify we can find it | ||
| found = await Session.find(work_dir, session_id) | ||
| assert found is not None | ||
| assert found.id == session_id |
There was a problem hiding this comment.
The test coverage for the new named session feature is incomplete. While the test verifies that a named session can be created and found, it doesn't test important edge cases:
- Invalid session IDs with path traversal attempts (e.g., "../escape", "/absolute/path")
- Session IDs with special characters or path separators
- Empty or whitespace-only session IDs
- Very long session IDs
- Session IDs that conflict with filesystem limitations
Consider adding tests for these cases to ensure the feature is robust and secure.
Session.create(work_dir, session_id)