Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 26, 2026

As titled

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

@friggeri friggeri changed the title Add TypedDict for ping() and stop() return types Consistently use Dataclasses in Python SDK Jan 27, 2026
@github-actions
Copy link

✅ Cross-SDK Consistency Review: APPROVED

I've reviewed PR #216 for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). This PR successfully addresses the Python SDK's inconsistency issue and brings it into alignment with the other SDKs.

Summary

What Changed: The Python SDK was returning a mix of raw dictionaries (via TypedDict) and dataclass objects. This PR converts all public API response types to use dataclasses consistently.

Consistency Status:Excellent cross-SDK alignment


Detailed Cross-SDK Comparison

Method Node.js/TypeScript Python (After PR) Go .NET
ping() Typed object PingResponse dataclass *PingResponse struct PingResponse class
getStatus() GetStatusResponse interface GetStatusResponse dataclass *GetStatusResponse struct GetStatusResponse class
getAuthStatus() GetAuthStatusResponse interface GetAuthStatusResponse dataclass *GetAuthStatusResponse struct GetAuthStatusResponse class
listModels() ModelInfo[] list[ModelInfo] []ModelInfo List(ModelInfo)
listSessions() SessionMetadata[] list[SessionMetadata] ⚠️ Missing method List(SessionMetadata)
stop() Error[] list[StopError] []error Task (void)

Key Observations

Consistent approach: All SDKs now use strongly-typed response objects:

  • TypeScript: Interfaces with TypeScript's type system
  • Python: Dataclasses with from_dict() / to_dict() methods
  • Go: Structs with JSON tags
  • C#: Classes with JSON property attributes

API naming consistency: Method names follow language conventions while maintaining semantic parity:

  • TypeScript: getStatus(), getAuthStatus(), listModels()
  • Python: get_status(), get_auth_status(), list_models()
  • Go: GetStatus(), GetAuthStatus(), ListModels()
  • .NET: GetStatusAsync(), GetAuthStatusAsync(), ListModelsAsync()

Field naming consistency: All SDKs use the same JSON field names:

  • protocolVersion, isAuthenticated, sessionId, etc.
  • Python properly preserves camelCase field names in dataclasses despite Python's snake_case convention

Minor Inconsistency Found (Pre-existing)

⚠️ Go SDK is missing ListSessions() method

The Go SDK doesn't implement the ListSessions() method that exists in all other SDKs. This appears to be a pre-existing gap unrelated to this PR.

Recommendation: Consider adding ListSessions() to the Go SDK in a follow-up issue for complete feature parity.


Conclusion

This PR successfully achieves its goal of making the Python SDK consistent internally and aligned with the other SDK implementations. The changes are well-executed:

  1. ✅ All response types converted from TypedDict to dataclasses
  2. ✅ Proper serialization/deserialization with from_dict() and to_dict()
  3. ✅ Documentation updated to reflect object attribute access (.field) instead of dict access (['field'])
  4. ✅ Tests updated to use attribute access
  5. ✅ Type checking passes

No cross-SDK consistency issues introduced by this PR. Great work! 🎉

AI generated by SDK Consistency Review Agent

@friggeri friggeri marked this pull request as ready for review January 27, 2026 21:54
@friggeri friggeri requested a review from a team as a code owner January 27, 2026 21:54
Copilot AI review requested due to automatic review settings January 27, 2026 21:54
Copy link
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 pull request converts several TypedDict response types to dataclasses in the Python SDK to improve type consistency and enable structured attribute access. The changes affect core response types returned by the Copilot CLI API.

Changes:

  • Converted response types (PingResponse, StopError, GetStatusResponse, GetAuthStatusResponse, ModelInfo and related classes, SessionMetadata) from TypedDict to dataclass with from_dict/to_dict methods
  • Updated all dictionary-style access patterns to attribute access throughout test files
  • Removed unnecessary type: ignore comment and cast() call that are no longer needed
  • Updated API documentation to reflect dataclass return types

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
python/copilot/types.py Converted 11 TypedDict classes to dataclasses with serialization methods
python/copilot/client.py Updated return types and added from_dict conversions for all API methods
python/copilot/init.py Added PingResponse and StopError to public exports
python/e2e/test_session.py Updated tests to use attribute access instead of dictionary access
python/e2e/test_client.py Updated tests to use attribute access instead of dictionary access
python/copilot/tools.py Removed unnecessary type: ignore comment
Comments suppressed due to low confidence (1)

python/copilot/types.py:658

  • The from_dict method should convert the optional summary field to str type when it is not None, similar to how the required string fields are converted. Currently, summary is passed through without type conversion, which could cause type inconsistencies if the server returns a non-string value.
            summary=summary,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

✅ Cross-SDK Consistency Review

This PR improves cross-SDK consistency by converting the Python SDK from TypedDict to dataclasses for response types.

Consistency Analysis

After reviewing all four SDK implementations, I can confirm this change aligns Python with the patterns used in other languages:

SDK Response Type Pattern
Node.js/TypeScript TypeScript interfaces (typed objects) ✅
Go Structs (typed objects) ✅
.NET Classes (typed objects) ✅
Python (before) TypedDict (dictionary types) ⚠️
Python (after) Dataclasses (typed objects) ✅

Key Improvements

  1. Type Safety: Dataclasses provide better runtime type checking and IDE support compared to TypedDict
  2. Consistent API Surface: All SDKs now use proper object types with properties/attributes (e.g., response.message instead of response["message"])
  3. Serialization Support: The added from_dict() and to_dict() methods parallel the serialization patterns in Go and .NET
  4. Error Handling: The new StopError dataclass provides structured error reporting consistent with other SDKs

Affected Methods

The following methods now return typed dataclass instances instead of dictionaries:

  • ping()PingResponse
  • get_status()GetStatusResponse
  • get_auth_status()GetAuthStatusResponse
  • list_models()list[ModelInfo]
  • stop()list[StopError]

No consistency issues found. This PR successfully brings the Python SDK into alignment with the type safety patterns used across all other SDK implementations.

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.

3 participants