Python SDK API review fixes#1376
Merged
Merged
Conversation
…ten PermissionRequestResult Brings Python in line with TS/Rust/.NET/Go which all emit per-variant types for �nyOf-of-\ discriminated unions in the schemas (PermissionRequest, PermissionDecision, AuthInfo, SendAttachment, ToolExecutionCompleteContent, TaskInfo, SystemNotification, etc.). Previously the Python codegen merged each one into a single flat dataclass with every variant's fields collapsed to Optional — see `scripts/codegen/python.ts:897-901` for the old remap table that fronted the merged blob as `PermissionRequest`. Codegen changes (`scripts/codegen/python.ts`) * `tryEmitPyRefBasedDiscriminatedUnion` in the hand-written session-events pipeline emits each variant as its own `@dataclass`, plus a `Name = VariantA | VariantB | ...` union alias and a `_load_Name(obj)` dispatcher that switches on the discriminator (matches `findPyDiscriminator`). * `postProcessRefBasedDiscriminatedUnionsForPython` does the equivalent on the quicktype-emitted RPC types: detects each `\`-based discriminated union, deletes the merged flat class quicktype produced, emits the union alias and dispatcher, and rewrites every `Name.from_dict(x)` / `to_class(Name, x)` call (including in RPC method wrappers generated later) to route through the dispatcher. * Acronym resolution table (`Api → API`, `Mcp → MCP`, `Cli → CLI`, etc.) to map schema names to the actual class names quicktype emits. * `postProcessDiscriminatorDefaultsForPython` replaces the dataclass-level `kind` field on each variant with a class-level `ClassVar[str]` constant. Users construct variants with no discriminator arg required: `PermissionDecisionApproveOnce()` instead of `PermissionDecisionApproveOnce(kind=PermissionDecisionApproveOnceKind.APPROVE_ONCE)`. `from_dict` / `to_dict` are rewritten in lock-step. Hand-written SDK changes * `copilot.session.PermissionRequestResult` becomes a type alias for `PermissionDecision | PermissionNoResult` (mirrors TS `nodejs/src/types.ts:883`). The hand-written `PermissionRequestResult` dataclass and the `_decision_from_result` mapper are gone — handlers now return the generated variant directly. `PermissionNoResult` is a tiny hand-written sentinel for the v1-protocol case. * `PermissionHandler.approve_all` returns `PermissionDecisionApproveOnce()`. * `CopilotSession._execute_permission_and_respond` passes the returned decision straight through to the RPC; v2 servers reject `PermissionNoResult` with a clear `ValueError`. * `CopilotClient._handle_permission_request_v2` uses `_load_PermissionRequest` and serialises the variant result with `.to_dict()`. Tests and scenarios updated in lock-step. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- on_exit_plan_mode -> on_exit_plan_mode_request (handler kwarg + TypedDict field) - on_auto_mode_switch -> on_auto_mode_switch_request (handler kwarg + TypedDict field) - CopilotSession.get_messages() -> get_events() (returns SessionEvent[], not messages) - ProviderConfig.max_input_tokens -> max_prompt_tokens (matches wire key maxPromptTokens) Mirrors PR #1357 Phase A (TypeScript SDK API review). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors PR #1343 Phase 9 (.NET) and PR #1357 Phase I (TypeScript). Removes the flat `SubprocessConfig` / `ExternalServerConfig` split in favour of a `RuntimeConnection` discriminated hierarchy, with process-management options moved to a new `CopilotClientOptions` dataclass. Public API: * `RuntimeConnection` abstract base with static factories `stdio()`, `tcp()`, `uri()`. * `ChildProcessRuntimeConnection` intermediate base carrying `path` and `args` shared by Stdio + Tcp. * `StdioRuntimeConnection`, `TcpRuntimeConnection`, `UriRuntimeConnection` concrete subclasses; pattern-match / `isinstance` on the class to branch on the transport. * `CopilotClientOptions(connection=..., working_directory=..., log_level=..., env=..., github_token=..., base_directory=..., use_logged_in_user=..., telemetry=..., session_fs=..., session_idle_timeout_seconds=..., enable_remote_sessions=...)`. * `CopilotClient(options=CopilotClientOptions(...) | None, *, auto_start=..., on_list_models=...)`. Renames: * `copilot_home` → `base_directory` * `remote` → `enable_remote_sessions` * `CopilotClient.actual_port` → `CopilotClient.runtime_port` * `CopilotClient.on(...)` → `CopilotClient.on_lifecycle(...)` * `tcp_connection_token` moves off `CopilotClientOptions` and onto the variant: `TcpRuntimeConnection.connection_token` / `UriRuntimeConnection.connection_token` * `use_stdio` bool is gone — the variant class IS the discriminator. Migration: * All hand-written code that previously branched on `isinstance(config, ExternalServerConfig)` now branches on the runtime-connection variant via `isinstance(connection, UriRuntimeConnection)` / `StdioRuntimeConnection` etc., giving proper type narrowing in pyright/mypy without relying on Literal-based tagged-union narrowing. * Samples, scenarios, README, and tests updated in lock-step. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Test collection broke on CI because e2e/test_telemetry_e2e.py imports it from the top-level package; only the dataclass option types had been re-exported. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Two issues surfaced by Phase B CI:
1. The committed Python generated files were stale relative to the rest
of the cross-language generators. Re-running `npm run generate` syncs
them. No semantic change beyond round-tripping each variant dataclass
through `from_dict(obj: Any) -> "Name":` quoting (matches what
codegen has produced for a while; the on-disk file just hadn't been
resynced).
2. `test/scenarios/**/python/main.py` were calling `client.create_session({...})`
and `client.resume_session(session_id, {...})` with positional dicts.
`CopilotClient.create_session` and `CopilotClient.resume_session` are
kwargs-only — these always TypeError'd at runtime. Converted all 18
scenarios to explicit kwargs (`create_session(model="...", ...)`).
Caught by github-code-quality CodeQL on PR #1376.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codegen-check CI complained because the local fmt I ran when re-running codegen used stable rustfmt; the verification workflow uses the nightly toolchain plus the nightly-only .rustfmt.nightly.toml config (see .github/workflows/rust-sdk-tests.yml). Re-run nightly fmt to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1376 · ● 6.3M
| cli_path: str | None = None | ||
| """Path to the Copilot CLI executable. ``None`` uses the bundled binary.""" | ||
| @staticmethod | ||
| def stdio( |
Contributor
There was a problem hiding this comment.
Cross-SDK consistency note: The factory method names here differ from the other SDKs. C# uses RuntimeConnection.ForStdio() / ForTcp() / ForUri(), and TypeScript uses RuntimeConnection.forStdio() / forTcp() / forUri() — both include a for prefix.
The Python snake_case equivalent would be RuntimeConnection.for_stdio(), RuntimeConnection.for_tcp(), RuntimeConnection.for_uri().
Is the for_ prefix intentionally dropped in Python, or should these be renamed to match the pattern in C# and TypeScript?
…enum
Six classes of failures surfaced by the Python e2e CI:
1. `_make_subprocess_client(ctx, use_stdio=False)` in test_pending_work_resume_e2e.py
and test_suspend_e2e.py was still creating an stdio `RuntimeConnection` regardless
of the `use_stdio` flag (leftover from the bulk migration). The downstream
`f"localhost:{server.runtime_port}"` then formatted `None` because stdio doesn't
listen on a port, triggering `ValueError: Invalid port in cli_url: localhost:None`.
Now branches on `use_stdio` and constructs `RuntimeConnection.tcp(...)` when False.
2. `.kind.value` / `.type.value` usages in e2e tests assumed the discriminator
field was an `Enum`. After the Phase L codegen change it's a `ClassVar[str]`,
so direct equality (`r.kind == "write"`, `attachment.type == "file"`) works
and no `.value` is needed. Updated test_multi_client_e2e.py, test_permissions_e2e.py,
test_tools_e2e.py, test_session_e2e.py.
3. `test_rpc_tasks_and_handlers_e2e.py` was constructing the now-deleted
`PermissionDecision`/`PermissionDecisionApproveForIonApproval` merged
blob types. Rewrote each call to use the proper per-variant constructors
(`PermissionDecisionReject(feedback=...)`,
`PermissionDecisionApprovePermanently(domain=...)`,
`PermissionDecisionApproveForSession(approval=PermissionDecisionApproveForSessionApprovalCustomTool(...))`,
`PermissionDecisionApproveForLocation(location_key=..., approval=PermissionDecisionApproveForLocationApprovalCustomTool(...))`),
and `found_task.type == TaskInfoType.AGENT` to `isinstance(found_task, TaskAgentInfo)`.
4. Codegen: quicktype merges structurally-identical types and synthesizes a
fuzzy class name (`PermissionDecisionApproveForIonApproval` is the merge
of `PermissionDecisionApproveFor{Session,Location}Approval`). Extended
`acronymCandidates` to try the `Session→Ion` / `Location→Ion` substitutions
so the post-pass can resolve the fuzzy name, and added an explicit cleanup
step that deletes the now-orphan blob after the union rewrites complete.
Updated generated rpc.py reflects this.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors PR #1357 Phase C (TypeScript SDK API review). Changes: * `SessionConfig.streaming` and `ResumeSessionConfig.streaming` now document `Defaults to False` (matches TS PR #1357 4.2). * `ToolBinaryResult.type` tightened from `str` to `Literal["image", "resource"]` with `"image"` default (matches TS PR #1357 2.6). * `scripts/codegen/python.ts` now sets `inferenceFlags: { combineClasses: false }` when invoking quicktype. Previously quicktype's default structural-equality merging produced fuzzy synthesized names (e.g. `PermissionDecisionApproveForIonApproval` for the merge of `PermissionDecisionApproveFor{Session,Location}Approval`), which are unstable: any future divergence between the schema variants would silently change the generated class name. We rely on the schema's named definitions and resolve structural unions explicitly via `postProcessRefBasedDiscriminatedUnionsForPython`, so the merging is also redundant. Removed the `Session→Ion` / `Location→Ion` acronym hack we needed when the merging was on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors PR #1357 Phases D + E (TypeScript SDK API review). Phase D — Session lifecycle event polymorphic hierarchy: * Split the flat `SessionLifecycleEvent` dataclass into a `SessionLifecycleEventBase` + five concrete variant dataclasses (`SessionCreatedEvent`, `SessionDeletedEvent`, `SessionUpdatedEvent`, `SessionForegroundEvent`, `SessionBackgroundEvent`). `SessionLifecycleEvent` is now a `Union` type alias over the five variants; pattern-match on the variant class to branch on the event kind. Each variant exposes `type: ClassVar[Literal["..."]]`, so existing `event.type == "session.created"` consumer code continues to work. * `SessionLifecycleEvent.from_dict` becomes a module-private `_session_lifecycle_event_from_dict` factory that switches on the wire `type` and constructs the matching variant. Phase D / E — Timestamps as `datetime`: * `SessionMetadata.startTime` / `modifiedTime` now `datetime` (previously ISO-8601 `str`). `to_dict` round-trips back to ISO strings. * `SessionLifecycleEventMetadata.startTime` / `modifiedTime` likewise. * All `Pre/PostToolUseHookInput`, `PreMcpToolCallHookInput`, `UserPromptSubmittedHookInput`, `SessionStartHookInput`, `SessionEndHookInput`, `ErrorOccurredHookInput` declare `timestamp: datetime`. `CopilotSession._handle_hooks_invoke` converts the wire epoch-milliseconds integer into a timezone-aware `datetime` at the dispatch boundary (same place the existing `cwd → workingDirectory` remap lives). Tests updated in lock-step: `test_session_e2e.py` asserts the new `datetime` types on `SessionMetadata`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors PR #1357 Phase K + 4.1 (TypeScript SDK API review). Cleanup: * Remove the deprecated `CopilotSession.destroy()` method. Only `disconnect()` remains (matches TS PR #1357 4.1). * Rename `NO_RESULT_PERMISSION_V2_ERROR` → `_NO_RESULT_PERMISSION_V2_ERROR` and `MIN_PROTOCOL_VERSION` → `_MIN_PROTOCOL_VERSION` (module-private constants; Python equivalent of TS `@internal` + `stripInternal`). Public API audit vs C# / TypeScript: * `copilot/__init__.py` now re-exports the full set of types that match the TS public surface from `nodejs/src/index.ts`. Notable additions: - Lifecycle event hierarchy: `SessionCreatedEvent`, `SessionDeletedEvent`, `SessionUpdatedEvent`, `SessionForegroundEvent`, `SessionBackgroundEvent`, `SessionLifecycleEvent`, `SessionLifecycleEventBase`, `SessionLifecycleEventMetadata`, `SessionLifecycleEventType`, `SessionLifecycleHandler` - Session metadata / context: `SessionContext`, `SessionListFilter`, `SessionMetadata` - Generated `PermissionRequest`, `SessionEvent`, `SessionEventType` ( consumers shouldn't have to reach into `copilot.generated.*`) - `PermissionHandler`, `PermissionRequestResult`, `PermissionNoResult`, `UserInputHandler`, `UserInputRequest`, `UserInputResponse` - MCP server configs: `MCPHTTPServerConfig`, `MCPServerConfig`, `MCPStdioServerConfig` - Session config: `SessionConfig`, `ResumeSessionConfig`, `SessionHooks`, `SystemMessageConfig`, `InfiniteSessionConfig` - All hook handler / input / output TypedDicts: `PreToolUseHandler` & friends across the six hook lifecycle points - Model* completion (added `ModelLimits`, `ModelSupports`, `ModelVisionLimits`, `ModelBilling`, `ModelPolicy`, `ModelCapabilities`) - `ConnectionState`, `LogLevel`, `PingResponse`, `GetStatusResponse`, `GetAuthStatusResponse`, `StopError` - `ToolResultType`, `SessionEventHandler` `__all__` is now alphabetically sorted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors PR #1357 phase B + api_review_python.md item B.9 (TypeScript SDK API review). Public Python dataclasses no longer expose camelCase attribute names; the wire JSON keys are unchanged. Renames (Python attribute → wire JSON key): * `PingResponse.protocolVersion` → `protocol_version` (wire: `protocolVersion`) * `SessionContext.gitRoot` → `git_root` (wire: `gitRoot`) * `SessionListFilter.gitRoot` → `git_root` (wire: `gitRoot`) * `SessionMetadata.sessionId` → `session_id` (wire: `sessionId`) * `SessionMetadata.startTime` → `start_time` (wire: `startTime`) * `SessionMetadata.modifiedTime` → `modified_time` (wire: `modifiedTime`) * `SessionMetadata.isRemote` → `is_remote` (wire: `isRemote`) * `SessionLifecycleEventMetadata.startTime` → `start_time` * `SessionLifecycleEventMetadata.modifiedTime` → `modified_time` * `SessionLifecycleEventBase.sessionId` → `session_id` Hook input TypedDicts (`PreToolUseHookInput`, `PreMcpToolCallHookInput`, etc.) intentionally keep camelCase keys — those are wire-format dicts delivered straight to handlers, not Python attributes. Tests + docs updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors PR #1357 Phase G (TypeScript SDK API review) and addresses api_review_python.md item B.5. Hoists the ~30 fields shared between `SessionConfig` and `ResumeSessionConfig` into a new `SessionConfigBase(TypedDict, total=False)`. `SessionConfig` now only adds `session_id` (create-only) and `ResumeSessionConfig` only adds `disable_resume` + `continue_pending_work` (resume-only). Both inherit `SessionConfigBase`. `SessionConfigBase` is re-exported from `copilot.__init__` to match the TS public API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1376 · ● 6.6M
| cli_path: str | None = None | ||
| """Path to the Copilot CLI executable. ``None`` uses the bundled binary.""" | ||
| @staticmethod | ||
| def stdio( |
Contributor
There was a problem hiding this comment.
Cross-SDK consistency note: The factory method names here differ from the other SDKs:
- TypeScript:
RuntimeConnection.forStdio(),RuntimeConnection.forTcp(),RuntimeConnection.forUri() - .NET:
RuntimeConnection.ForStdio(),RuntimeConnection.ForTcp(),RuntimeConnection.ForUri() - Python (this PR):
RuntimeConnection.stdio(),RuntimeConnection.tcp(),RuntimeConnection.uri()
Following Python snake_case conventions and aligning with the semantic for* naming pattern used in TypeScript and .NET, these should likely be RuntimeConnection.for_stdio(), RuntimeConnection.for_tcp(), and RuntimeConnection.for_uri().
The for_ prefix signals that these are factory/constructor methods rather than instance methods or properties—which is useful information for API consumers. Worth aligning if this isn't intentional.
Mirrors PR #1357 Phase K (TypeScript SDK API review): every Python code snippet under \docs/\ and the \python/README.md\ now uses the post-Phase-B/F/G/H public API: * `CopilotClient()` / `CopilotClient(CopilotClientOptions(connection=RuntimeConnection.uri/stdio/tcp(...), ...))` in place of `SubprocessConfig` / `ExternalServerConfig` * `PermissionDecisionApproveOnce()`, `PermissionDecisionReject(feedback=...)`, `PermissionDecisionUserNotAvailable()`, `PermissionNoResult()` in place of `PermissionRequestResult(kind=...)` * `base_directory` (was `copilot_home`) * `enable_remote_sessions` (was `remote`) * `on_exit_plan_mode_request` / `on_auto_mode_switch_request` * `session.get_events()` (was `get_messages`) * `client.on_lifecycle(...)` (was `client.on`) * `max_prompt_tokens` (was `max_input_tokens`) * snake_case dataclass attribute accesses (\session_id\, \start_time\, \modified_time\, \is_remote\, \git_root\, \protocol_version\) TypeScript / Go / C# / Rust / Java snippets in the same files are untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1376 · ● 5.2M
| cli_path: str | None = None | ||
| """Path to the Copilot CLI executable. ``None`` uses the bundled binary.""" | ||
| @staticmethod | ||
| def stdio( |
Contributor
There was a problem hiding this comment.
Cross-SDK naming consistency: The factory method names stdio(), tcp(), and uri() differ from the equivalent names in the TypeScript and C# SDKs:
- TypeScript:
RuntimeConnection.forStdio(...),RuntimeConnection.forTcp(...),RuntimeConnection.forUri(...) - C#:
RuntimeConnection.ForStdio(...),RuntimeConnection.ForTcp(...),RuntimeConnection.ForUri(...) - Python (this PR):
RuntimeConnection.stdio(...),RuntimeConnection.tcp(...),RuntimeConnection.uri(...)
The consistent Python equivalents (following snake_case conventions) would be for_stdio(), for_tcp(), and for_uri(). Dropping the for_ prefix is a semantic shift — RuntimeConnection.uri(...) in particular could be misread as a property or as constructing a URI object rather than a connection-from-URI factory.
Would it make sense to use for_stdio() / for_tcp() / for_uri() here to keep the factory method semantics visible and parallel with TS/C#?
Two new ty errors surfaced by Phase F/G changes: 1. `client.py:2660` accessed `ping_result.protocolVersion` — the field was renamed to `protocol_version` in Phase G but this call site was missed. 2. `session.py:_handle_hooks_invoke` lost the static link between the normalized hook input dict and the per-hook-type TypedDict variant after the Phase E wire-key remap + timestamp conversion, breaking the typed Union dispatch. Cast the normalized dict back to `Any` so the call-site picks the correct Union variant at type-check time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1376 · ● 6.5M
| cli_path: str | None = None | ||
| """Path to the Copilot CLI executable. ``None`` uses the bundled binary.""" | ||
| @staticmethod | ||
| def stdio( |
Contributor
There was a problem hiding this comment.
Cross-SDK naming consistency note: The factory method names in Python (stdio, tcp, uri) differ from the equivalent names in C# and TypeScript:
| SDK | stdio | tcp | uri |
|---|---|---|---|
| TypeScript | RuntimeConnection.forStdio(...) |
RuntimeConnection.forTcp(...) |
RuntimeConnection.forUri(...) |
| C# | RuntimeConnection.ForStdio(...) |
RuntimeConnection.ForTcp(...) |
RuntimeConnection.ForUri(...) |
| Python (this PR) | RuntimeConnection.stdio(...) |
RuntimeConnection.tcp(...) |
RuntimeConnection.uri(...) |
For cross-SDK consistency, the Python equivalents would be RuntimeConnection.for_stdio(...), RuntimeConnection.for_tcp(...), and RuntimeConnection.for_uri(...) (snake_case of forStdio/ForStdio).
That said, shorter names like stdio() / tcp() / uri() are arguably more idiomatic when the class name already provides context — so this may be a deliberate Python-convention choice. Just flagging it since it's one of the more visible parts of the public API surface and the other SDKs are consistent with each other on this.
Fix six E2E test failures surfaced by the macOS Python 3.11 CI job: 1. test_should_get_null_last_session_id_before_any_sessions_exist: the runtime tracks 'last session id' as a persistent value; if a prior test in the class created any session, the assertion `result is None` fails. Clear leftover sessions before reading the value (matches the .NET fix in commit 5d55127). 2. test_should_get_status_with_version_and_protocol_info: the assertion was checking `hasattr(status, "protocolVersion")` (camelCase) but asserting against `status.protocol_version` (snake_case). Phase G renamed the field to snake_case; update the hasattr check to match. 3. test_should_emit_session_lifecycle_events: `getattr(e, "sessionId", ...)` still used camelCase after Phase G's rename to `session_id`. 4. test_should_propagate_process_options_to_spawned_cli: my earlier rename sweep mistakenly changed `COPILOT_HOME` env-var references in the testharness and fake CLI script to `base_directory` (the new option *name*). `COPILOT_HOME` is a wire-format env var owned by the CLI and must remain unchanged — only the SDK option name was renamed. Restore `COPILOT_HOME` everywhere it's used as an env var. 5. test_should_set_meta_via_premcptoolcall_hook: assertion compared the hook input timestamp against `0` (int), which now raises `TypeError: '>' not supported between instances of 'datetime.datetime' and 'int'` after Phase E converted hook timestamps to `datetime`. Replace with `isinstance(..., datetime)`. 6. test_should_list_sessions: `hasattr(session_data, "sessionId")` etc. still used camelCase after Phase G renamed `SessionMetadata` fields to snake_case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Match the naming used by the TypeScript and C# SDKs (`forStdio` / `forTcp` / `forUri` and `ForStdio` / `ForTcp` / `ForUri` respectively). The plain names `stdio` / `tcp` / `uri` read like properties or constructors of those concepts; the `for_` prefix makes the factory-method semantics explicit and keeps the API parallel across languages. Renames `RuntimeConnection.stdio` → `for_stdio`, `RuntimeConnection.tcp` → `for_tcp`, `RuntimeConnection.uri` → `for_uri` along with every call-site across the SDK, unit tests, E2E tests, scenarios, README, and docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- `CopilotClient.on_lifecycle` `@overload` stubs were using `...` as the body. CodeQL flags this as `Statement has no effect`; the rest of the codebase (`tools.py:define_tool`) already uses `pass` for overload bodies. Match the existing convention. - `_session_lifecycle_event_from_dict` used a `match` statement with a `case _:` default that returned in every arm. CodeQL still flagged it as `Explicit returns mixed with implicit (fall through) returns` (false positive on `match` analysis). Restructure as an if/return chain with a tail return, which CodeQL handles correctly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Phase G missed this hand-written response dataclass (it lives in `client.py` alongside the other RPC response types). The E2E test `test_should_get_status_with_version_and_protocol_info` exposed this: the test was updated to assert `hasattr(status, 'protocol_version')` but the attribute itself still used camelCase, so the assertion failed in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1376 · ● 8.4M
| options: CopilotClientOptions | None = None, | ||
| *, | ||
| auto_start: bool = True, | ||
| on_list_models: Callable[[], list[ModelInfo] | Awaitable[list[ModelInfo]]] | None = None, |
Contributor
There was a problem hiding this comment.
Cross-SDK consistency note: In TypeScript (onListModels in CopilotClientOptions) and C# (OnListModels in CopilotClientOptions), the custom model-listing handler is a field on the options object — not a separate constructor parameter.
Python's on_list_models living as a standalone __init__ parameter rather than inside CopilotClientOptions creates an asymmetry: callers can't store/pass a pre-built options object that includes this handler, and the pattern diverges from the other two SDKs.
Suggest moving on_list_models into CopilotClientOptions (perhaps in Phase F / public API audit) so all three SDKs have the same shape. Happy to defer if there's a reason to keep it separate for now.
Align CopilotClient constructor with the TypeScript and .NET SDKs: 1. **Move `on_list_models` into `CopilotClientOptions`** — both TS (`onListModels`) and .NET (`OnListModels`) place this handler inside the options object. The Python equivalent now lives on the options dataclass instead of as a `CopilotClient.__init__` kwarg. 2. **Remove the `auto_start` option** — .NET and TypeScript do not expose an opt-out for autostart. Autostart behaviour is preserved (calling `start()` is still optional), but consumers can no longer disable it. The deleted test `test_autostart_false_requires_explicit_start` covered an option that no longer exists. 3. **Remove `CopilotClient.get_state()` and the public `ConnectionState` type** — .NET removed these in commit b00fd8c (Phase 4d/4e). Python now matches. The internal `_state` field is preserved (used by `start()`/`stop()` to gate behavior), but it is no longer exposed via a public accessor or type alias. TypeScript still has `getState()` — tracked as a cross-SDK follow-up to remove there too. `CopilotClient` is now just `CopilotClient()` or `CopilotClient(options)` with no kwargs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Make the Python public API idiomatically Pythonic, matching the conventions used by ``openai``, ``anthropic``, ``httpx``, etc. **CopilotClient: flat kw-only ctor params** The ``CopilotClientOptions`` dataclass was a port of the TS / .NET ``CopilotClientOptions`` interface and is not how a Python API client is typically configured. Python users expect ``CopilotClient(connection=..., log_level=..., github_token=...)`` rather than ``CopilotClient(CopilotClientOptions(connection=..., log_level=..., github_token=...))``. Changes: - All 12 options previously on ``CopilotClientOptions`` are now kw-only parameters on ``CopilotClient.__init__``: ``connection``, ``working_directory``, ``log_level``, ``env``, ``github_token``, ``base_directory``, ``use_logged_in_user``, ``telemetry``, ``session_fs``, ``session_idle_timeout_seconds``, ``enable_remote_sessions``, ``on_list_models``. - ``CopilotClientOptions`` is renamed to ``_CopilotClientOptions`` and is now an implementation detail used only internally to carry the resolved options around. The public ``__all__`` no longer exports it. - IDE autocomplete on ``CopilotClient(`` now lists every option directly with its type, default, and docstring. Pyright / ty catch unknown kwargs at the call site. **Drop vestigial SessionConfig / ResumeSessionConfig / SessionConfigBase** These TypedDicts mirrored the TS / .NET ``SessionConfig`` interfaces but were never used anywhere in Python — ``create_session()`` and ``resume_session()`` already take flat kw-only parameters (the idiomatic form). The TypedDicts were just unreferenced documentation noise. Updates: - README, all docs Python snippets, all 25 ``test/scenarios/**/python/main.py`` fixtures, every E2E test, every unit test now use the new flat ctor. - The test class ``TestSubprocessOptions`` (E2E) and the unit-style ``test_telemetry_config_in_subprocess_config`` test exercised the ``CopilotClientOptions`` dataclass shape itself; they're deleted since Python's type system already enforces ctor-kwarg correctness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
The multi-line _make_options(...) call in test_should_propagate_process_options_to_spawned_cli was missed by my earlier sed pass that converted CopilotClient(_make_options(...)) to CopilotClient(**_make_options(...)) — the regex only matched single-line forms. Add the missing ** here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review (vs C# and TypeScript)Per the PR notes, I've reviewed only against C# and TypeScript. ✅ Consistent changes
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the Python SDK and examples to a new connection/configuration model (RuntimeConnection + kw-only CopilotClient options), modernizes generated Python types to use proper discriminated unions (with dispatcher loaders), and aligns tests/docs with these API changes.
Changes:
- Replaced
SubprocessConfig/ExternalServerConfigusage withRuntimeConnectionfactories and kw-onlyCopilotClient(...)options across scenarios, tests, and docs. - Switched session history API from
get_messages()toget_events()and updated related E2E assertions/types (timestamps asdatetime, snake_case fields, etc.). - Enhanced Python codegen post-processing to emit/refine
$ref-based discriminated unions and discriminator defaults for improved ergonomics.
Show a summary per file
| File | Description |
|---|---|
| test/scenarios/transport/tcp/python/main.py | Update TCP scenario to use RuntimeConnection.for_uri and kw-only session config. |
| test/scenarios/transport/stdio/python/main.py | Update stdio scenario to kw-only CopilotClient/session calls. |
| test/scenarios/transport/reconnect/python/main.py | Update reconnect scenario to RuntimeConnection and kw-only session config. |
| test/scenarios/tools/virtual-filesystem/python/main.py | Update permission decisions/types and client construction. |
| test/scenarios/tools/tool-overrides/python/main.py | Update client construction and formatting; keep tool override example. |
| test/scenarios/tools/tool-filtering/python/main.py | Convert dict-based session config to kw-only arguments. |
| test/scenarios/tools/skills/python/main.py | Update permission decision type and client construction. |
| test/scenarios/tools/no-tools/python/main.py | Convert session config dict to kw-only arguments and update client creation. |
| test/scenarios/tools/mcp-servers/python/main.py | Update client creation and minor formatting improvements. |
| test/scenarios/tools/custom-agents/python/main.py | Update client creation to kw-only options. |
| test/scenarios/sessions/streaming/python/main.py | Convert session config dict to kw-only args and modernize calls. |
| test/scenarios/sessions/session-resume/python/main.py | Convert session config dict to kw-only args; modernize calls. |
| test/scenarios/sessions/infinite-sessions/python/main.py | Convert nested config dict to kw-only args and keep infinite session config. |
| test/scenarios/sessions/concurrent-sessions/python/main.py | Convert session creation and message sends to kw-only calls. |
| test/scenarios/prompts/system-message/python/main.py | Convert session config dict to kw-only args. |
| test/scenarios/prompts/reasoning-effort/python/main.py | Convert session config dict to kw-only args and separate send call. |
| test/scenarios/prompts/attachments/python/main.py | Convert session config dict to kw-only args; keep attachment flow. |
| test/scenarios/modes/minimal/python/main.py | Convert session config dict to kw-only args; reformat prompt send. |
| test/scenarios/modes/default/python/main.py | Convert session creation to kw-only args; reformat prompt send. |
| test/scenarios/callbacks/user-input/python/main.py | Update permission decision type and convert session config to kw-only args. |
| test/scenarios/callbacks/permissions/python/main.py | Update permission decision type and convert session config to kw-only args. |
| test/scenarios/callbacks/hooks/python/main.py | Update permission decision type and convert session config to kw-only args. |
| test/scenarios/bundling/fully-bundled/python/main.py | Convert client/session creation to kw-only args. |
| test/scenarios/bundling/container-proxy/python/main.py | Update external connection to RuntimeConnection.for_uri. |
| test/scenarios/bundling/container-proxy/proxy.py | Reorder HTTP server imports (no functional change). |
| test/scenarios/bundling/app-direct-server/python/main.py | Update external connection to RuntimeConnection.for_uri. |
| test/scenarios/bundling/app-backend-to-server/python/main.py | Update external connection API and minor import formatting. |
| test/scenarios/auth/gh-app/python/main.py | Remove SubprocessConfig usage and switch to kw-only CopilotClient(github_token=...). |
| test/scenarios/auth/byok-openai/python/main.py | Shift to default client creation and kw-only provider config. |
| test/scenarios/auth/byok-ollama/python/main.py | Shift to default client creation and kw-only provider config. |
| test/scenarios/auth/byok-azure/python/main.py | Shift to default client creation and kw-only provider config. |
| test/scenarios/auth/byok-anthropic/python/main.py | Shift to default client creation and kw-only provider config. |
| scripts/codegen/python.ts | Add/refine Python codegen post-processing for $ref-based discriminated unions and discriminator defaults; disable quicktype class merging. |
| python/test_telemetry.py | Remove SubprocessConfig telemetry test and keep TelemetryConfig coverage. |
| python/test_rpc_generated.py | Update assertions to reflect discriminated-union variants (isinstance(...)). |
| python/test_event_forward_compatibility.py | Update parsing/round-trip tests to use union dispatch loaders (_load_...). |
| python/test_commands_and_elicitation.py | Switch tests to RuntimeConnection.for_stdio and renamed mode-handler params. |
| python/test_client.py | Switch to RuntimeConnection and update permission no-result handling and provider token fields. |
| python/e2e/testharness/helper.py | Switch history retrieval from get_messages() to get_events(). |
| python/e2e/testharness/context.py | Update harness client creation to RuntimeConnection.for_stdio and kw-only options. |
| python/e2e/test_ui_elicitation_multi_client_e2e.py | Update multi-client TCP setup to RuntimeConnection and runtime port naming. |
| python/e2e/test_tools_e2e.py | Update permission decision/result handling to new union classes and kind representation. |
| python/e2e/test_telemetry_e2e.py | Update telemetry client setup to kw-only options and remove SubprocessConfig-specific tests. |
| python/e2e/test_suspend_e2e.py | Update TCP/URI connection creation and permission decision variants. |
| python/e2e/test_subagent_hooks_e2e.py | Update client creation and adjust hook input field naming comment. |
| python/e2e/test_streaming_fidelity_e2e.py | Update to get_events() and kw-only client options. |
| python/e2e/test_session_fs_sqlite_e2e.py | Update client creation to RuntimeConnection.for_stdio. |
| python/e2e/test_session_fs_e2e.py | Update TCP/URI client creation and get_events() usage. |
| python/e2e/test_session_e2e.py | Update to get_events(), snake_case fields, and timestamp parsing to datetime. |
| python/e2e/test_session_config_e2e.py | Update history retrieval to get_events(). |
| python/e2e/test_rpc_tasks_and_handlers_e2e.py | Update permission decision unions and task-info checking to variant classes. |
| python/e2e/test_rpc_shell_and_fleet_e2e.py | Switch polling helper from get_messages() to get_events(). |
| python/e2e/test_rpc_session_state_e2e.py | Switch fork/state assertions to get_events(). |
| python/e2e/test_rpc_server_e2e.py | Update harness env access path and RuntimeConnection.for_stdio. |
| python/e2e/test_rpc_event_side_effects_e2e.py | Switch to get_events() for truncate/snapshot assertions. |
| python/e2e/test_rpc_e2e.py | Switch to RuntimeConnection.for_stdio across RPC E2E tests. |
| python/e2e/test_pre_mcp_tool_call_hook_e2e.py | Update timestamp assertion to datetime type. |
| python/e2e/test_permissions_e2e.py | Update permission decision/result types and kind representation. |
| python/e2e/test_per_session_auth_e2e.py | Update harness env access path and RuntimeConnection.for_stdio. |
| python/e2e/test_pending_work_resume_e2e.py | Update TCP/URI clients and permission decision variant usage. |
| python/e2e/test_multi_client_e2e.py | Update multi-client TCP + permission decision variants and runtime port naming. |
| python/e2e/test_mode_handlers_e2e.py | Update proxy env path; rename handler params to *_request. |
| python/e2e/test_event_fidelity_e2e.py | Switch to get_events() in ordering assertions. |
| python/e2e/test_error_resilience_e2e.py | Switch to get_events() in disconnect error test. |
| python/e2e/test_connection_token.py | Update token scenarios to RuntimeConnection and runtime port naming. |
| python/e2e/test_commands_e2e.py | Update multi-client TCP setup to RuntimeConnection and runtime port naming. |
| python/e2e/test_client_options_e2e.py | Refactor options builder to kw-only client args + RuntimeConnection; adjust fake CLI stub to snake_case payload. |
| python/e2e/test_client_lifecycle_e2e.py | Rename lifecycle subscription API to on_lifecycle and update event fields. |
| python/e2e/test_client_e2e.py | Switch to RuntimeConnection and snake_case response fields (e.g., protocol version). |
| python/e2e/test_client_api_e2e.py | Add cleanup to ensure get_last_session_id() test runs against empty state. |
| python/e2e/test_agent_and_compact_rpc_e2e.py | Switch to RuntimeConnection.for_stdio for agent RPC tests. |
| python/copilot/tools.py | Tighten ToolBinaryResult.type typing to Literal["image","resource"]. |
| python/copilot/session.py | Replace permission result model with decision union + PermissionNoResult; normalize hook timestamps to datetime; rename get_messages() → get_events(). |
| python/copilot/client.py | Introduce RuntimeConnection and kw-only client options; restructure lifecycle event types; align parsing/serialization field names and permission handling. |
| python/copilot/init.py | Re-export new public API surface (RuntimeConnection, lifecycle event variants, etc.) and session event types. |
| python/README.md | Update docs for RuntimeConnection, get_events(), and new permission decision types. |
| docs/troubleshooting/debugging.md | Update Python example to kw-only CopilotClient(log_level="debug") and new URI connection wording. |
| docs/setup/backend-services.md | Update Python backend-services example to RuntimeConnection.for_uri(...). |
| docs/observability/opentelemetry.md | Update Python telemetry example to kw-only CopilotClient(telemetry=...). |
| docs/getting-started.md | Update permission example to new decision variant; update URI connection example (see suggestions). |
| docs/features/steering-and-queueing.md | Update permission examples to new decision variant class. |
| docs/features/skills.md | Update permission examples to new decision variant class. |
| docs/features/remote-sessions.md | Update remote sessions example to enable_remote_sessions=True. |
| docs/features/image-input.md | Update permission examples to new decision variant class. |
| docs/features/hooks.md | Update permission examples/imports to new decision variant class. |
| docs/features/custom-agents.md | Update permission examples to new decision variant class. |
| docs/auth/byok.md | Update custom model listing example to kw-only CopilotClient(on_list_models=...). |
Copilot's findings
Comments suppressed due to low confidence (4)
python/e2e/test_ui_elicitation_multi_client_e2e.py:1
- These comments/docstrings contain mojibake (
—) where an em dash (—) was likely intended. Replace—with—(and consider running a repo-wide search/replace for this sequence to fix other occurrences).
python/e2e/test_ui_elicitation_multi_client_e2e.py:1 - These comments/docstrings contain mojibake (
—) where an em dash (—) was likely intended. Replace—with—(and consider running a repo-wide search/replace for this sequence to fix other occurrences).
python/e2e/test_ui_elicitation_multi_client_e2e.py:1 - These comments/docstrings contain mojibake (
—) where an em dash (—) was likely intended. Replace—with—(and consider running a repo-wide search/replace for this sequence to fix other occurrences).
python/e2e/test_ui_elicitation_multi_client_e2e.py:1 - These comments/docstrings contain mojibake (
—) where an em dash (—) was likely intended. Replace—with—(and consider running a repo-wide search/replace for this sequence to fix other occurrences).
- Files reviewed: 87/89 changed files
- Comments generated: 5
Comment on lines
+1970
to
+1975
| from copilot import CopilotClient, CopilotClientOptions, RuntimeConnection | ||
| from copilot.session import PermissionHandler | ||
|
|
||
| client = CopilotClient({ | ||
| "cli_url": "localhost:4321" | ||
| }) | ||
| client = CopilotClient(CopilotClientOptions( | ||
| connection=RuntimeConnection.for_uri("localhost:4321"), | ||
| )) |
Comment on lines
+577
to
+583
| from copilot import ( | ||
| PermissionDecisionApproveOnce, | ||
| PermissionDecisionReject, | ||
| PermissionRequest, | ||
| PermissionRequestResult, | ||
| PermissionRequestShell, | ||
| ) |
Comment on lines
+159
to
+163
| class RuntimeConnection: | ||
| """Discriminated config describing how to reach the Copilot runtime. | ||
|
|
||
| Construct via the static factories :meth:`stdio`, :meth:`tcp`, or | ||
| :meth:`uri`. Each factory returns the matching subclass; pattern-match |
Comment on lines
+48
to
+52
| # Other tests in this class create sessions, and pytest doesn't | ||
| # guarantee test execution order. Clear any leftover sessions so this | ||
| # test sees a genuinely empty state regardless of order. | ||
| for existing in await ctx.client.list_sessions(): | ||
| await ctx.client.delete_session(existing.session_id) |
Comment on lines
+250
to
+255
| # The decision returned by a permission handler. Identical shape to the wire | ||
| # ``PermissionDecision`` discriminated union, plus a :class:`PermissionNoResult` | ||
| # sentinel for v1 servers. Construct via the generated variant classes: | ||
| # ``PermissionDecisionApproveOnce(kind=...)``, ``PermissionDecisionReject(kind=..., | ||
| # feedback=...)``, etc. | ||
| PermissionRequestResult = PermissionDecision | PermissionNoResult |
SteveSandersonMS
added a commit
that referenced
this pull request
May 22, 2026
.NET removed `CopilotClient.State` and `ConnectionState` in commit b00fd8c (Phase 4d/4e). Python matched this in PR #1376. TypeScript was the last SDK still exposing both — drop them for cross-SDK consistency. - `CopilotClient.getState(): ConnectionState` removed. - Public `ConnectionState` type alias removed from `types.ts` and the index re-exports. - The private `state` field on `CopilotClient` stays (still used by start/stop to gate behavior); its type is now an inline union declaration instead of the exported alias. - Unit test for unexpected child-process death now reads the private `state` field via `(client as any).state` since the behaviour being tested (state transition on async disconnect) is genuinely internal. - E2E `client.getState() === "..."` assertions are dropped — they were pure tautologies once `start()` returned without throwing. - `nodejs/README.md` no longer documents `getState()`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
added a commit
that referenced
this pull request
May 22, 2026
1. docs/getting-started.md: Python snippet still used the removed `CopilotClientOptions` wrapper. Switch to the flat-kwargs form (`CopilotClient(connection=RuntimeConnection.for_uri(...))`). 2. python/README.md custom-permission-handler example imported per-kind variants (`PermissionRequestShell`, `PermissionDecisionApproveOnce`, `PermissionDecisionReject`) from `copilot` — but only `PermissionRequest` and `PermissionRequestResult` are re-exported at the top level. Fix the example to import the variant classes from `copilot.generated.session_events`. 3. python/copilot/client.py `RuntimeConnection` docstring referenced the old factory names `stdio`/`tcp`/`uri`. Update to `for_stdio`/`for_tcp`/`for_uri` to match the renamed methods. 4. python/copilot/session.py comment above `PermissionRequestResult` told users to construct variants with `kind=...` arguments — but Phase L baked the discriminator into a `ClassVar` default, so the generated variants reject `kind=` at the call site. Update the comment to reflect the new ergonomics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
added a commit
that referenced
this pull request
May 22, 2026
…(.NET, Go) Python (#1376) drove out its own hand-written ``PermissionRequestResult`` wrapper in favour of the generated discriminated ``PermissionDecision`` union plus a small ``PermissionNoResult`` sentinel. This commit lands the same refactor for the .NET and Go SDKs. **.NET** The old ``PermissionRequestResult`` struct (just ``Kind`` + optional ``Feedback``) collapsed every decision variant into a flat string-tagged DTO, losing the rich per-variant payloads — ``Feedback`` on rejection, per-kind ``Approval`` lists on ``ApproveForSession`` / ``ApproveForLocation``, ``Domain`` on ``ApprovePermanently``, etc. The generated ``PermissionDecision`` (``Rpc.cs:4760``) is already a proper polymorphic hierarchy with ``[JsonDerivedType]`` wired up for every variant. Switch ``OnPermissionRequest`` to return ``Task<PermissionDecision>`` and route the variant straight onto the wire — StreamJsonRpc handles the discriminator via the existing attributes. Additions: - ``PermissionDecisionNoResult`` — hand-written subclass of ``PermissionDecision`` used when the handler declines to respond so another connected client can answer. The SDK suppresses the wire response when it sees this variant. - Static factories on ``PermissionDecision`` for discoverability: ``ApproveOnce()``, ``Reject(feedback)``, ``UserNotAvailable()``, ``NoResult()``. For richer decisions that need an ``Approval`` payload, instantiate the variant class directly. Deletions: - ``PermissionRequestResult`` class (``Types.cs``) - ``PermissionRequestResultKind`` struct + obsolete enum-like wrappers - ``PermissionRequestResultKindTests.cs`` - ``PermissionRequestResult`` JSON serialization metadata **Go** Same shape: drop ``PermissionRequestResult`` + ``PermissionRequestResultKind`` in favour of ``rpc.PermissionDecision`` (already a sealed interface implemented by every variant). Added ``rpc.PermissionDecisionNoResult`` as a hand-written variant that satisfies the unexported ``permissionDecision()`` method — kept inside the ``rpc`` package since the sealing method is unexported. Handler signature changes from ``(PermissionRequestResult, error)`` to ``(rpc.PermissionDecision, error)``. ``PermissionHandler.ApproveAll`` now returns ``&rpc.PermissionDecisionApproveOnce{}``. Removed the ``rpcPermissionDecisionFromKind`` helper used to convert the flat kind back to a variant — no longer needed when the handler already returns the variant directly. **Tests / scenarios** All E2E tests and scenarios across .NET and Go updated to construct ``rpc.PermissionDecision*`` variants directly. The Go interface return type means explicit ``Task.FromResult<PermissionDecision>(...)`` casts are needed in C# where lambdas previously inferred the wrapper type. **Doc style** Per repo convention, public docs do not reference protocol v1/v2 or internal transport details. The README copy for each SDK describes the behavioural semantics ("decline to respond so another client can answer") rather than the wire mechanism. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
added a commit
that referenced
this pull request
May 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applies the equivalent of PR #1343 (C#) and PR #1357 (TypeScript) to the Python SDK, plus the Python-specific items from �pi_review_python.md.
Will be pushed phase-by-phase. Tracking plan: session-local plan.md.
Phases
Cross-language follow-up
Phase L additionally drops the hand-written PermissionRequestResult dataclass in Python and uses the generated PermissionDecision union directly (matching TS). We should investigate whether .NET (and Go / Java) should adopt the same pattern — tracked in plan.md but NOT blocking this PR.
Notes for cross-language parity reviewer
This is part of a refactoring series. We've already done C# and TypeScript. So, do NOT comment on inconsistencies with Rust/Go/Java at this point — only assess consistency with C# and TS.