Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 26, 2026

Verification: Type Checking Status ✅

Current Status

The type checking with uv run ty check copilot is now passing successfully.

What Was Already Fixed

The previous commit (342039c) "Fix ty type checker errors after dataclass conversion" already resolved all the type checking issues:

  1. ✅ Fixed PingResponse.get() error - changed to attribute access .protocolVersion
  2. ✅ Removed redundant cast(bytes, ...) call
  3. ✅ Removed unused # type: ignore comment
  4. ✅ Removed unused cast import

Verification Results

$ uv run ty check copilot
Checking ------------------------------------------------------------ 1/7 files
Checking ------------------------------------------------------------ 2/7 files
Checking ------------------------------------------------------------ 3/7 files
Checking ------------------------------------------------------------ 4/7 files
Checking ------------------------------------------------------------ 5/7 files
Checking ------------------------------------------------------------ 6/7 files
Checking ------------------------------------------------------------ 7/7 files
All checks passed!

All type checks are passing - no further action needed.

Original prompt

This section details on the original issue you should resolve

<issue_title>Public python API inconsistency: some methods return models, others return raw dicts</issue_title>
<issue_description>The Python Copilot SDK currently exposes a mixed public API surface: some higher-level methods return dataclass-based models (generated from NodeJS definitions), while others return raw JSON-RPC dicts typed only via TypedDict (e.g. get_status(), list_sessions()).

I understand that the underlying JsonRpcClient intentionally returns dicts at the transport layer, but at the SDK boundary this leads to an inconsistent abstraction and unclear guidance for users (object attributes vs dict access).

Some responses are represented as dataclasses generated from NodeJS definitions which are generated, with explicit serialization helpers:

copilot/generated/session_events.py

@dataclass
class ContextClass:
    ...
    @staticmethod
    def from_dict(...)

These expose a clear, object-oriented API to SDK users.

Moreover Sample: 

>>> sess = await client.create_session()
>>> sess
<copilot.session.CopilotSession object at 0x000002292C6F7E00>

Other public methods return raw JSON-RPC responses directly:

async def get_status(self) -> GetStatusResponse:
    return await self._client.request("status.get", {})
>>> await client.get_status()
{'version': '0.0.394', 'protocolVersion': 2}

This should return GetStatusResponse ?

Could you clarify whether the intended direction is to consistently expose object-based models at the public API level, with dicts treated as an internal detail? @friggeri

Happy to help refactor or align missing response types if this matches the design intent.</issue_description>

<agent_instructions>This user is providing very valuable feedback. Audit all the responses that are exposed to the consumer of the SDK (essentially all public methods meant to be used by developers) and if they aren't wrapped in data classes fix that. You should update all tests wherever needed to reflect those changes.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix public API inconsistency in Python SDK Add TypedDict for ping() and stop() return types Jan 26, 2026
Copilot AI requested a review from friggeri January 26, 2026 23:44
Copilot AI and others added 2 commits January 26, 2026 23:52
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
@github-actions
Copy link

Cross-SDK Consistency Review

I've reviewed this PR for consistency with the other SDK implementations (Node.js/TypeScript, Go, and .NET). The Python SDK is converting response types from TypedDict to dataclasses, which is great for consistency within Python. However, I've identified one cross-SDK inconsistency to consider:

❌ Inconsistency Found: StopError dataclass

Python SDK (this PR):

async def stop(self) -> list[StopError]:
    # Returns list of StopError dataclasses with 'message' field
    errors: list[StopError] = []
    errors.append(StopError(message=f"Failed to destroy session..."))
    return errors

Node.js/TypeScript SDK:

async stop(): Promise(Error[]) {
    // Returns native Error objects
    const errors: Error[] = [];
    errors.push(new Error(`Failed to destroy session...`));
    return errors;
}

Go SDK:

func (c *Client) Stop() []error {
    // Returns native error slice
    var errors []error
    errors = append(errors, fmt.Errorf("failed to destroy session..."))
    return errors
}

.NET SDK:

public async Task StopAsync() {
    // Throws AggregateException instead of returning errors
    var errors = new List(Exception)();
    errors.Add(new Exception($"Failed to destroy session..."));
    ThrowErrors(errors); // Throws instead of returning
}

Recommendation

Consider these options for consistency:

  1. Option A (Preferred): Return native Python Exception objects in stop() to match Node.js and Go:

    async def stop(self) -> list[Exception]:
        errors: list[Exception] = []
        errors.append(Exception(f"Failed to destroy session {session.session_id}: {e}"))
        return errors
  2. Option B: Keep StopError dataclass but update other SDKs to use similar wrapper types (more invasive)

  3. Option C: Keep current implementation but document the cross-language difference

All Other Response Types: ✅ Consistent

The dataclass conversions for PingResponse, GetStatusResponse, GetAuthStatusResponse, ModelInfo, and SessionMetadata are consistent with other SDKs:

  • Node.js/TypeScript uses interfaces
  • Go uses structs
  • .NET uses classes
  • Python now uses dataclasses

All expose object-oriented APIs with attribute/property access, which aligns perfectly.


Note: This is a suggestion for maintaining cross-SDK consistency, not a blocking issue. The current implementation is functional and improves Python's internal consistency.

AI generated by SDK Consistency Review Agent

Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
@github-actions
Copy link

Cross-SDK Consistency Review ✅

I've reviewed this PR for consistency across all four SDK implementations (Python, TypeScript, Go, and .NET).

Summary

This PR successfully aligns the Python SDK with the other SDKs by converting return types from raw dicts (TypedDict) to dataclasses. This is excellent for API consistency!

Current State After This PR

Method Python TypeScript Go .NET
ping() PingResponse Inline object ⚠️ *PingResponse PingResponse
stop() list[StopError] Error[] []error Throws AggregateException ⚠️
get_status() GetStatusResponse GetStatusResponse *GetStatusResponse GetStatusResponse
get_auth_status() GetAuthStatusResponse GetAuthStatusResponse *GetAuthStatusResponse GetAuthStatusResponse
list_models() list[ModelInfo] ModelInfo[] []ModelInfo ModelInfo[]
list_sessions() list[SessionMetadata] (not checked) (not checked) (not checked)

Minor Inconsistency Noted (Not a blocker for this PR)

The TypeScript SDK's ping() method returns an inline anonymous object type:

Promise<{ message: string; timestamp: number; protocolVersion?: number }>

While all other SDKs (including Python after this PR) use a named type like PingResponse. This is a minor inconsistency that could be addressed in a future TypeScript PR to define a PingResponse interface.

Recommendation

This PR maintains good cross-SDK consistency. The Python SDK now properly returns typed objects instead of raw dicts, matching the pattern used in Go and .NET.

No changes needed to this PR. Great work on improving the Python SDK's type consistency!

AI generated by SDK Consistency Review Agent

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.

Public python API inconsistency: some methods return models, others return raw dicts

3 participants