[python] Change CopilotClient.__init__ to take config objects#793
[python] Change CopilotClient.__init__ to take config objects#793brettcannon wants to merge 2 commits intogithub:mainfrom
CopilotClient.__init__ to take config objects#793Conversation
|
This is an alternative to #788 |
There was a problem hiding this comment.
Pull request overview
Updates the Python SDK to replace the dict-based CopilotClient constructor with explicit connection config objects, and migrates tests/docs/scenarios to the new API.
Changes:
- Introduce
SubprocessConfigandExternalServerConfigdataclasses and updateCopilotClient.__init__to accept them (plusauto_start/on_list_modelskwargs). - Update Python unit/E2E tests and scenario scripts to use the new constructor/config objects.
- Refresh Python README examples and constructor documentation to reflect the new API.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/scenarios/transport/tcp/python/main.py | Use ExternalServerConfig when connecting to an external TCP server. |
| test/scenarios/transport/stdio/python/main.py | Use SubprocessConfig for stdio/subprocess configuration. |
| test/scenarios/transport/reconnect/python/main.py | Migrate reconnect scenario to ExternalServerConfig. |
| test/scenarios/tools/virtual-filesystem/python/main.py | Migrate tool scenario to SubprocessConfig. |
| test/scenarios/tools/tool-overrides/python/main.py | Migrate tool override scenario to SubprocessConfig. |
| test/scenarios/tools/tool-filtering/python/main.py | Migrate tool filtering scenario to SubprocessConfig. |
| test/scenarios/tools/skills/python/main.py | Migrate skills scenario to SubprocessConfig. |
| test/scenarios/tools/no-tools/python/main.py | Migrate no-tools scenario to SubprocessConfig. |
| test/scenarios/tools/mcp-servers/python/main.py | Migrate MCP servers scenario to SubprocessConfig. |
| test/scenarios/tools/custom-agents/python/main.py | Migrate custom agents scenario to SubprocessConfig. |
| test/scenarios/sessions/streaming/python/main.py | Migrate streaming scenario to SubprocessConfig. |
| test/scenarios/sessions/session-resume/python/main.py | Migrate session resume scenario to SubprocessConfig. |
| test/scenarios/sessions/infinite-sessions/python/main.py | Migrate infinite sessions scenario to SubprocessConfig. |
| test/scenarios/sessions/concurrent-sessions/python/main.py | Migrate concurrent sessions scenario to SubprocessConfig. |
| test/scenarios/prompts/system-message/python/main.py | Migrate system message prompt scenario to SubprocessConfig. |
| test/scenarios/prompts/reasoning-effort/python/main.py | Migrate reasoning effort scenario to SubprocessConfig. |
| test/scenarios/prompts/attachments/python/main.py | Migrate attachments scenario to SubprocessConfig. |
| test/scenarios/modes/minimal/python/main.py | Migrate minimal mode scenario to SubprocessConfig. |
| test/scenarios/modes/default/python/main.py | Migrate default mode scenario to SubprocessConfig. |
| test/scenarios/callbacks/user-input/python/main.py | Migrate user input callback scenario to SubprocessConfig. |
| test/scenarios/callbacks/permissions/python/main.py | Migrate permissions callback scenario to SubprocessConfig. |
| test/scenarios/callbacks/hooks/python/main.py | Migrate hooks callback scenario to SubprocessConfig. |
| test/scenarios/bundling/fully-bundled/python/main.py | Migrate fully bundled scenario to SubprocessConfig. |
| test/scenarios/bundling/container-proxy/python/main.py | Migrate container proxy scenario to ExternalServerConfig. |
| test/scenarios/bundling/app-direct-server/python/main.py | Migrate direct server scenario to ExternalServerConfig. |
| test/scenarios/bundling/app-backend-to-server/python/main.py | Update backend-to-server sample to use ExternalServerConfig. |
| test/scenarios/auth/gh-app/python/main.py | Migrate GitHub App auth scenario to SubprocessConfig. |
| test/scenarios/auth/byok-openai/python/main.py | Migrate BYOK OpenAI scenario to SubprocessConfig. |
| test/scenarios/auth/byok-ollama/python/main.py | Migrate BYOK Ollama scenario to SubprocessConfig. |
| test/scenarios/auth/byok-azure/python/main.py | Migrate BYOK Azure scenario to SubprocessConfig. |
| test/scenarios/auth/byok-anthropic/python/main.py | Migrate BYOK Anthropic scenario to SubprocessConfig. |
| python/test_client.py | Update unit tests to instantiate clients via config objects and new kwargs. |
| python/e2e/testharness/context.py | Update E2E harness context to pass SubprocessConfig. |
| python/e2e/test_streaming_fidelity.py | Update E2E streaming fidelity test client construction. |
| python/e2e/test_session.py | Update E2E session test client construction. |
| python/e2e/test_rpc.py | Update E2E RPC tests to use SubprocessConfig. |
| python/e2e/test_multi_client.py | Update multi-client E2E to use SubprocessConfig/ExternalServerConfig. |
| python/e2e/test_client.py | Update E2E client tests to use SubprocessConfig. |
| python/e2e/test_agent_and_compact_rpc.py | Update agent/compact E2E tests to use SubprocessConfig. |
| python/copilot/types.py | Add SubprocessConfig/ExternalServerConfig types to replace CopilotClientOptions. |
| python/copilot/client.py | Refactor CopilotClient constructor and startup logic around the new config objects. |
| python/copilot/init.py | Export the new config types from the package entrypoint. |
| python/README.md | Update documentation/examples for the new constructor/config objects. |
| auto_restart: bool = True | ||
| """Auto-restart the CLI server if it crashes.""" | ||
|
|
There was a problem hiding this comment.
SubprocessConfig.auto_restart is documented as restarting the CLI on crash, but it isn't referenced anywhere in the client implementation (no restart/watch logic). Either implement crash detection + restart behavior using this flag, or remove the field/docs to avoid a silently ignored setting.
| auto_restart: bool = True | |
| """Auto-restart the CLI server if it crashes.""" |
| - `port` (int): Server port for TCP mode (default: 0 for random) | ||
| - `log_level` (str): Log level (default: "info") | ||
| - `auto_start` (bool): Auto-start server on first use (default: True) | ||
| - `auto_restart` (bool): Auto-restart on crash (default: True) |
There was a problem hiding this comment.
Docs list SubprocessConfig.auto_restart as supported, but the current CopilotClient implementation does not use this setting (no restart behavior). Either document it as not implemented or remove it until it's supported, otherwise readers will assume crash-restart works.
| - `auto_restart` (bool): Auto-restart on crash (default: True) | |
| - `auto_restart` (bool): Reserved for future use — not yet implemented; currently has no effect (default: True) |
No description provided.