-
Notifications
You must be signed in to change notification settings - Fork 1k
[Python] update CopilotSession to handle workspace_path as os.PathLike and ensure proper initialization #901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1252,7 +1252,7 @@ async def create_session( | |
|
|
||
| # Create and register the session before issuing the RPC so that | ||
| # events emitted by the CLI (e.g. session.start) are not dropped. | ||
| session = CopilotSession(actual_session_id, self._client, None) | ||
| session = CopilotSession(actual_session_id, self._client, workspace_path=None) | ||
| session._register_tools(tools) | ||
| session._register_permission_handler(on_permission_request) | ||
| if on_user_input_request: | ||
|
|
@@ -1456,7 +1456,7 @@ async def resume_session( | |
|
|
||
| # Create and register the session before issuing the RPC so that | ||
| # events emitted by the CLI (e.g. session.start) are not dropped. | ||
| session = CopilotSession(session_id, self._client, None) | ||
| session = CopilotSession(session_id, self._client, workspace_path=None) | ||
| session._register_tools(tools) | ||
|
Comment on lines
1457
to
1460
|
||
| session._register_permission_handler(on_permission_request) | ||
| if on_user_input_request: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,10 @@ | |
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import functools | ||
| import inspect | ||
| import os | ||
| import pathlib | ||
| import threading | ||
| from collections.abc import Awaitable, Callable | ||
| from dataclasses import dataclass | ||
|
|
@@ -639,7 +642,9 @@ class CopilotSession: | |
| ... unsubscribe() | ||
| """ | ||
|
|
||
| def __init__(self, session_id: str, client: Any, workspace_path: str | None = None): | ||
| def __init__( | ||
| self, session_id: str, client: Any, workspace_path: os.PathLike[str] | str | None = None | ||
| ): | ||
| """ | ||
| Initialize a new CopilotSession. | ||
|
|
||
|
|
@@ -655,7 +660,7 @@ def __init__(self, session_id: str, client: Any, workspace_path: str | None = No | |
| """ | ||
| self.session_id = session_id | ||
| self._client = client | ||
| self._workspace_path = workspace_path | ||
| self._workspace_path = os.fsdecode(workspace_path) if workspace_path is not None else None | ||
| self._event_handlers: set[Callable[[SessionEvent], None]] = set() | ||
| self._event_handlers_lock = threading.Lock() | ||
| self._tool_handlers: dict[str, ToolHandler] = {} | ||
|
|
@@ -677,15 +682,19 @@ def rpc(self) -> SessionRpc: | |
| self._rpc = SessionRpc(self._client, self.session_id) | ||
| return self._rpc | ||
|
|
||
| @property | ||
| def workspace_path(self) -> str | None: | ||
| @functools.cached_property | ||
| def workspace_path(self) -> pathlib.Path | None: | ||
| """ | ||
| Path to the session workspace directory when infinite sessions are enabled. | ||
|
|
||
| Contains checkpoints/, plan.md, and files/ subdirectories. | ||
| None if infinite sessions are disabled. | ||
| """ | ||
| return self._workspace_path | ||
| # Done as a property as self._workspace_path is directly set from a server | ||
| # response post-init. So it was either make sure all places directly setting | ||
| # the attribute handle the None case appropriately, use a setter for the | ||
| # attribute to do the conversion, or just do the conversion lazily via a getter. | ||
| return pathlib.Path(self._workspace_path) if self._workspace_path else None | ||
|
Comment on lines
+685
to
+697
|
||
|
|
||
| async def send( | ||
| self, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CopilotSession.workspace_pathis now acached_property, but this method setssession._workspace_pathonly after thesession.createRPC returns. If any early event handler accessessession.workspace_pathbefore_workspace_pathis populated, it will cacheNonepermanently unless the cache is invalidated after assigning_workspace_path. Consider explicitly clearing the cached attribute after setting_workspace_path(or switchingworkspace_pathback to a non-cached@property).