-
Notifications
You must be signed in to change notification settings - Fork 889
feat: add async context manager support for CopilotClient and Copilot… #475
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
4926b48
f982ac2
4ca3ee8
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 |
|---|---|---|
|
|
@@ -14,13 +14,15 @@ | |
|
|
||
| import asyncio | ||
| import inspect | ||
| import logging | ||
| import os | ||
| import re | ||
| import subprocess | ||
| import sys | ||
| import threading | ||
| from dataclasses import asdict, is_dataclass | ||
| from pathlib import Path | ||
| from types import TracebackType | ||
| from typing import Any, Callable, Optional, cast | ||
|
|
||
| from .generated.rpc import ServerRpc | ||
|
|
@@ -206,6 +208,56 @@ def __init__(self, options: Optional[CopilotClientOptions] = None): | |
| self._lifecycle_handlers_lock = threading.Lock() | ||
| self._rpc: Optional[ServerRpc] = None | ||
|
|
||
| async def __aenter__(self) -> "CopilotClient": | ||
| """ | ||
| Enter the async context manager. | ||
|
|
||
| Automatically starts the CLI server and establishes a connection if not | ||
| already connected. | ||
|
|
||
| Returns: | ||
| The CopilotClient instance. | ||
|
|
||
| Example: | ||
| >>> async with CopilotClient() as client: | ||
| ... session = await client.create_session() | ||
| ... await session.send({"prompt": "Hello!"}) | ||
| """ | ||
| await self.start() | ||
| return self | ||
|
|
||
| async def __aexit__( | ||
| self, | ||
| exc_type: Optional[type[BaseException]], | ||
| exc_val: Optional[BaseException], | ||
| exc_tb: Optional[TracebackType], | ||
| ) -> bool: | ||
| """ | ||
| Exit the async context manager. | ||
|
|
||
| Performs graceful cleanup by destroying all active sessions and stopping | ||
| the CLI server. If cleanup errors occur, they are logged but do not | ||
| prevent the context from exiting. | ||
|
|
||
| Args: | ||
| exc_type: The type of exception that occurred, if any. | ||
| exc_val: The exception instance that occurred, if any. | ||
| exc_tb: The traceback of the exception that occurred, if any. | ||
|
|
||
| Returns: | ||
| False to propagate any exception that occurred in the context. | ||
| """ | ||
| try: | ||
| stop_errors = await self.stop() | ||
| # Log any session destruction errors | ||
| if stop_errors: | ||
| for error in stop_errors: | ||
| logging.warning("Error during session cleanup in CopilotClient: %s", error) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit concerned about this as not everyone wants 'logging' in their code as they may use a different logger. Plus this is too broad regardless as there isn't a way to filter out the logging for the library (i.e. this call uses the root logger). I think more thought has to go into whether there's a desire for logging, and if there is then how to handle it. |
||
| except Exception: | ||
| # Log the error but don't raise - we want cleanup to always complete | ||
| logging.warning("Error during CopilotClient cleanup", exc_info=True) | ||
| return False | ||
|
Comment on lines
+256
to
+259
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is incorrect if you're trying to suppress the exception; you would want to return |
||
|
|
||
| @property | ||
| def rpc(self) -> ServerRpc: | ||
| """Typed server-scoped RPC methods.""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,9 @@ | |
|
|
||
| import asyncio | ||
| import inspect | ||
| import logging | ||
| import threading | ||
| from types import TracebackType | ||
| from typing import Any, Callable, Optional | ||
|
|
||
| from .generated.rpc import SessionRpc | ||
|
|
@@ -70,6 +72,7 @@ def __init__(self, session_id: str, client: Any, workspace_path: Optional[str] = | |
| self.session_id = session_id | ||
| self._client = client | ||
| self._workspace_path = workspace_path | ||
| self._destroyed = False | ||
| self._event_handlers: set[Callable[[SessionEvent], None]] = set() | ||
| self._event_handlers_lock = threading.Lock() | ||
| self._tool_handlers: dict[str, ToolHandler] = {} | ||
|
|
@@ -82,6 +85,50 @@ def __init__(self, session_id: str, client: Any, workspace_path: Optional[str] = | |
| self._hooks_lock = threading.Lock() | ||
| self._rpc: Optional[SessionRpc] = None | ||
|
|
||
| async def __aenter__(self) -> "CopilotSession": | ||
| """ | ||
| Enter the async context manager. | ||
|
|
||
| Returns the session instance, ready for use. The session must already be | ||
| created (via CopilotClient.create_session or resume_session). | ||
|
|
||
| Returns: | ||
| The CopilotSession instance. | ||
|
|
||
| Example: | ||
| >>> async with await client.create_session() as session: | ||
| ... await session.send({"prompt": "Hello!"}) | ||
| """ | ||
| return self | ||
|
|
||
| async def __aexit__( | ||
| self, | ||
| exc_type: Optional[type[BaseException]], | ||
| exc_val: Optional[BaseException], | ||
| exc_tb: Optional[TracebackType], | ||
| ) -> bool: | ||
| """ | ||
| Exit the async context manager. | ||
|
|
||
| Automatically destroys the session and releases all associated resources. | ||
| If an error occurs during cleanup, it is logged but does not prevent the | ||
| context from exiting. | ||
|
|
||
| Args: | ||
| exc_type: The type of exception that occurred, if any. | ||
| exc_val: The exception instance that occurred, if any. | ||
| exc_tb: The traceback of the exception that occurred, if any. | ||
|
|
||
| Returns: | ||
| False to propagate any exception that occurred in the context. | ||
| """ | ||
| try: | ||
| await self.destroy() | ||
| except Exception: | ||
| # Log the error but don't raise - we want cleanup to always complete | ||
| logging.warning("Error during CopilotSession cleanup", exc_info=True) | ||
| return False | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the exception that was raised should be suppressed without at least a configuration option. |
||
|
|
||
| @property | ||
| def rpc(self) -> SessionRpc: | ||
| """Typed session-scoped RPC methods.""" | ||
|
|
@@ -479,20 +526,33 @@ async def destroy(self) -> None: | |
| handlers and tool handlers are cleared. To continue the conversation, | ||
| use :meth:`CopilotClient.resume_session` with the session ID. | ||
|
|
||
| This method is idempotent—calling it multiple times is safe and will | ||
| not raise an error if the session is already destroyed. | ||
|
|
||
| Raises: | ||
| Exception: If the connection fails. | ||
| Exception: If the connection fails (on first destroy call). | ||
|
|
||
| Example: | ||
| >>> # Clean up when done | ||
| >>> await session.destroy() | ||
| """ | ||
| await self._client.request("session.destroy", {"sessionId": self.session_id}) | ||
| # Ensure that the check and update of _destroyed are atomic so that | ||
| # only the first caller proceeds to send the destroy RPC. | ||
| with self._event_handlers_lock: | ||
| self._event_handlers.clear() | ||
| with self._tool_handlers_lock: | ||
| self._tool_handlers.clear() | ||
| with self._permission_handler_lock: | ||
| self._permission_handler = None | ||
| if self._destroyed: | ||
| return | ||
| self._destroyed = True | ||
|
|
||
| try: | ||
| await self._client.request("session.destroy", {"sessionId": self.session_id}) | ||
| finally: | ||
| # Clear handlers even if the request fails | ||
| with self._event_handlers_lock: | ||
| self._event_handlers.clear() | ||
| with self._tool_handlers_lock: | ||
| self._tool_handlers.clear() | ||
| with self._permission_handler_lock: | ||
| self._permission_handler = None | ||
|
|
||
| async def abort(self) -> None: | ||
| """ | ||
|
|
||
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.
If
StopErrorwas an exception then you would want to useExceptionGroup, but since that's a Python 3.11 thing and this library supports 3.9 it would need a shim for 3.9 and 3.10.