[Python] update CopilotSession to handle workspace_path as os.PathLike and ensure proper initialization#901
[Python] update CopilotSession to handle workspace_path as os.PathLike and ensure proper initialization#901brettcannon wants to merge 1 commit intogithub:mainfrom
Conversation
…d ensure proper initialization Also have the workspace_path property return a cached pathlib.Path instance.
There was a problem hiding this comment.
Pull request overview
This PR updates the Python SDK’s CopilotSession workspace path handling to accept os.PathLike inputs and to expose workspace_path as a pathlib.Path (with caching), aligning the session API more closely with idiomatic Python path handling.
Changes:
- Extend
CopilotSession.__init__to acceptos.PathLike[str] | str | Noneand normalize viaos.fsdecode. - Change
workspace_pathto returnpathlib.Path | Noneand implement it as a cached property. - Update
CopilotClient.create_session/resume_sessionto passworkspace_pathas a keyword argument.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
python/copilot/session.py |
Accepts PathLike workspace paths and returns a pathlib.Path via a cached accessor. |
python/copilot/client.py |
Updates session construction call sites to use the new workspace_path parameter form. |
| @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 |
There was a problem hiding this comment.
workspace_path is a cached_property, but _workspace_path is set after session construction (e.g. in CopilotClient.create_session/resume_session). If workspace_path is accessed before the RPC response populates _workspace_path (for example inside an early on_event callback), the cached value will be None and will never update even after _workspace_path is later set. Consider using a regular @property (compute Path on each access) or implementing explicit cache invalidation whenever _workspace_path changes (e.g., a setter/helper that deletes the cached attribute from __dict__).
| # 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) |
There was a problem hiding this comment.
CopilotSession.workspace_path is now a cached_property, but this method sets session._workspace_path only after the session.create RPC returns. If any early event handler accesses session.workspace_path before _workspace_path is populated, it will cache None permanently unless the cache is invalidated after assigning _workspace_path. Consider explicitly clearing the cached attribute after setting _workspace_path (or switching workspace_path back to a non-cached @property).
| # 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) |
There was a problem hiding this comment.
Same issue as create_session: CopilotSession.workspace_path is a cached_property while _workspace_path is populated only after the session.resume RPC completes. If any early event handler reads session.workspace_path before _workspace_path is set, the cached value may remain None unless the cache is invalidated after assigning _workspace_path.
Also have the workspace_path property return a cached pathlib.Path instance.