Skip to content

Conversation

@PsiACE
Copy link
Member

@PsiACE PsiACE commented Dec 19, 2025

Summary

Temporary fix:

  • Add a terminal_id alias on TerminalHandle and widen Client.create_terminal typing to
    match runtime.
  • Add a regression test for on_connect + create_terminal behavior.

Related issues

Related to #49

Testing

Docs & screenshots

Checklist

  • Conventional Commit title (e.g. feat:, fix:).
  • Tests cover the change or are not required (explain above).
  • Docs/examples updated when behaviour is user-facing.
  • Schema regenerations (make gen-all) are called out if applicable.

Copy link

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 addresses a type mismatch between the Client.create_terminal protocol signature and its runtime behavior. The fix provides a temporary solution by widening the return type and adding a backward-compatible property alias.

Key Changes:

  • Added a terminal_id property to TerminalHandle that aliases the existing id attribute, ensuring naming consistency with CreateTerminalResponse.terminal_id
  • Widened the Client.create_terminal return type from CreateTerminalResponse to CreateTerminalResponse | TerminalHandle to accommodate both test implementations (which return the response directly) and runtime behavior (which wraps responses in TerminalHandle)
  • Added a regression test validating that agents can call create_terminal through on_connect connections and access terminal_id on the result

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/acp/terminal.py Adds terminal_id property as an alias to the id attribute for API consistency
src/acp/interfaces.py Updates Client.create_terminal return type to union of CreateTerminalResponse | TerminalHandle
tests/test_rpc.py Adds regression test validating on_connect + create_terminal interaction and terminal_id accessibility

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

output_byte_limit: int | None = None,
**kwargs: Any,
) -> CreateTerminalResponse: ...
) -> CreateTerminalResponse | TerminalHandle: ...
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The union return type CreateTerminalResponse | TerminalHandle creates ambiguity for API consumers. Callers cannot determine at compile-time which type will be returned, requiring runtime type checking. Consider documenting which implementations return which type, or ideally standardizing all implementations to return the same type (TerminalHandle) for consistency with the runtime behavior shown in agent/connection.py.

Suggested change
) -> CreateTerminalResponse | TerminalHandle: ...
) -> TerminalHandle: ...

Copilot uses AI. Check for mistakes.
Signed-off-by: Chojan Shang <[email protected]>
@PsiACE PsiACE merged commit 769f582 into main Dec 22, 2025
8 checks passed
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.

2 participants