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>
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 was referenced May 25, 2026
This was referenced Jun 2, 2026
Closed
This was referenced Jun 10, 2026
This was referenced Jun 18, 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.