Skip to content

Generate dedicated Python session event types#1063

Open
stephentoub wants to merge 1 commit intomainfrom
stephentoub/align-python-event-generation
Open

Generate dedicated Python session event types#1063
stephentoub wants to merge 1 commit intomainfrom
stephentoub/align-python-event-generation

Conversation

@stephentoub
Copy link
Copy Markdown
Collaborator

Summary

Python session events still used quicktype and collapsed payload members into one merged Data model, unlike the newer Go implementation. That made the language bindings inconsistent and increased the chance of naming conflicts as the runtime schema grows.

This change switches Python session-event generation to dedicated per-event payload dataclasses, matching the newer Go-style approach. SessionEvent.from_dict() now dispatches to typed payload classes, unknown events still round-trip through RawSessionEventData, and the Python runtime/tests were updated to handle the typed payload union.

The one non-obvious part is compatibility: the generator also restores legacy top-level helper exports and keeps arbitrary nested mappings in the Data shim as plain dicts so older callers do not break while the generated model becomes more structured.

Validation

  • python -m pytest test_event_forward_compatibility.py test_commands_and_elicitation.py
  • python -m pytest --ignore=e2e

Align Python session event generation with the newer Go-style dedicated per-event payload model instead of the old merged quicktype Data shape. This updates the runtime/tests for typed payloads while preserving compatibility aliases and legacy Data behavior for existing callers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@stephentoub stephentoub requested a review from a team as a code owner April 11, 2026 18:51
Copilot AI review requested due to automatic review settings April 11, 2026 18:51
const lines: string[] = [];
if (description) {
lines.push(`class ${enumName}(Enum):`);
lines.push(` """${description.replace(/"/g, '\\"')}"""`);
lines.push(`@dataclass`);
lines.push(`class ${typeName}:`);
if (description || schema.description) {
lines.push(` """${(description || schema.description || "").replace(/"/g, '\\"')}"""`);
lines.push(`@dataclass`);
lines.push(`class ${typeName}:`);
if (description) {
lines.push(` """${description.replace(/"/g, '\\"')}"""`);
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Python SDK’s session-events generation to emit dedicated per-event payload dataclasses (instead of a single merged Data payload), and updates the Python runtime/tests to consume these typed payloads while retaining compatibility shims.

Changes:

  • Replaced Python session-events quicktype generation with a custom generator that emits per-event payload dataclasses and dispatches in SessionEvent.from_dict().
  • Updated Python session handling and tests to use typed payload classes (e.g., CommandExecuteData, ElicitationRequestedData) and validate legacy top-level exports + Data shim behavior.
  • Preserved forward compatibility for unknown event types via SessionEventType.UNKNOWN + RawSessionEventData, plus added several legacy helper aliases.
Show a summary per file
File Description
scripts/codegen/python.ts Implements custom Python session-events codegen emitting per-event payload dataclasses, compatibility shims, and typed dispatch.
python/test_event_forward_compatibility.py Extends forward-compat tests to cover legacy helper exports and Data shim dict-preservation behavior.
python/test_commands_and_elicitation.py Updates tests to construct typed event payloads instead of using the legacy Data container.
python/copilot/session.py Updates broadcast event handling to cast event.data to the appropriate generated payload dataclass before accessing fields.

Copilot's findings

  • Files reviewed: 4/5 changed files
  • Comments generated: 2

Comment on lines +902 to +905
out.push(` result: list[str] = []`);
out.push(` for index, char in enumerate(name.replace(".", "_")):`);
out.push(
` if char.isupper() and index > 0 and (not name[index - 1].isupper() or (index + 1 < len(name) and name[index + 1].islower())):`
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the generated _compat_to_python_key, the loop iterates over name.replace(".", "_") but the underscore-insertion condition still indexes into the original name. For keys containing a dot followed by an uppercase letter (e.g. "foo.Bar"), this will insert an extra underscore and produce "foo__bar" instead of "foo_bar". Consider using a local normalized = name.replace(".", "_") and use normalized[...] for the neighbor checks to keep the logic consistent with the iterated string.

Suggested change
out.push(` result: list[str] = []`);
out.push(` for index, char in enumerate(name.replace(".", "_")):`);
out.push(
` if char.isupper() and index > 0 and (not name[index - 1].isupper() or (index + 1 < len(name) and name[index + 1].islower())):`
out.push(` normalized = name.replace(".", "_")`);
out.push(` result: list[str] = []`);
out.push(` for index, char in enumerate(normalized):`);
out.push(
` if char.isupper() and index > 0 and (not normalized[index - 1].isupper() or (index + 1 < len(normalized) and normalized[index + 1].islower())):`

Copilot uses AI. Check for mistakes.
}
for (const value of values) {
lines.push(` ${toEnumMemberName(value)} = ${JSON.stringify(value)}`);
}
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getOrCreatePyEnum generates plain Enum classes with only the known schema values. Any newly introduced enum values from the server/schema will raise ValueError in parse_enum(...), which is at odds with the forward-compat approach used for SessionEventType (UNKNOWN + _missing_). To preserve forward compatibility (especially for discriminator enums created by emitPyFlatDiscriminatedUnion), consider emitting an UNKNOWN member plus a _missing_ handler (and optionally preserving the raw string) for these generated enums as well, or avoid enums for forward-compatible discriminator fields.

Suggested change
}
}
lines.push(` UNKNOWN = "__unknown__"`);
lines.push(``);
lines.push(` @classmethod`);
lines.push(` def _missing_(cls, value: object) -> "${enumName}":`);
lines.push(` return cls.UNKNOWN`);

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR improves cross-SDK consistency rather than introducing new inconsistencies.

Summary

The PR aligns Python session-event generation with the Go (and .NET) approach by replacing quicktype's flattened Data model with dedicated per-event payload dataclasses. After this change:

SDK Session Event Approach
Python (after this PR) @dataclass class XxxData per event, typed union SessionEventData
Go type XxxData struct per event, SessionEventData interface
.NET public partial class XxxData per event
TypeScript Anonymous discriminated union types (intentionally different — idiomatic TS)

Key consistency checks

  • Class naming: Python ExternalToolRequestedData, PermissionRequestedData, etc. match Go and .NET exactly ✅
  • Convenience aliases: Python preserves PermissionRequest = PermissionRequestedDataPermissionRequest (and PermissionRequestKind, PermissionRequestCommand, PossibleURL) — matching Go's alias set ✅
  • Backward compat: Python adds additional legacy aliases (e.g. RepositoryClass, Selection, etc.) not present in Go, but these are needed only because Python is migrating away from quicktype-generated names; they don't represent an API gap in other SDKs
  • TypeScript: Continues to use anonymous discriminated-union types — this is intentional per the SDK design and isn't affected by this PR ✅

Internal usage in session.py

The session.py changes are internal only: replacing untyped event.data.field accesses with explicit cast(XxxData, event.data) before field access. No public API changes.

No cross-SDK consistency issues found. This PR closes an existing inconsistency between Python and the other SDKs.

Generated by SDK Consistency Review Agent for issue #1063 · ● 791.9K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants