-
Notifications
You must be signed in to change notification settings - Fork 7
feat: major Python-style refactoring underway, preparing to release version 0.7.x. #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* feat: change field names to snake case Signed-off-by: Frost Ming <me@frostming.com> * fix lint errors Signed-off-by: Frost Ming <me@frostming.com> --------- Signed-off-by: Frost Ming <me@frostming.com>
* feat: Auto-generate agent/client methods based on the schema Signed-off-by: Frost Ming <me@frostming.com> * fix: overrides in gen_schema.py Signed-off-by: Frost Ming <me@frostming.com> --------- Signed-off-by: Frost Ming <me@frostming.com>
…compatible (#37) * feat: Expose new APIs for running agent and connect with client Signed-off-by: Frost Ming <me@frostming.com> * fix: allow camelCase for accessing Signed-off-by: Frost Ming <me@frostming.com> * fix: make the docs clearer Signed-off-by: Frost Ming <me@frostming.com> * doc: add a migration guide Signed-off-by: Frost Ming <me@frostming.com> --------- Signed-off-by: Frost Ming <me@frostming.com>
…ection (#38) * fix: backward compatibility for the default behavior of AgentSideConnection Signed-off-by: Frost Ming <me@frostming.com> * fix: add 0.7 Migration Guide to navigation Signed-off-by: Frost Ming <me@frostming.com> --------- Signed-off-by: Frost Ming <me@frostming.com>
* fix: use classmethod for error factory methods Signed-off-by: Frost Ming <me@frostming.com> * fix: update return type hints for RequestError factory methods Signed-off-by: Frost Ming <me@frostming.com> --------- Signed-off-by: Frost Ming <me@frostming.com>
|
I’m considering whether we need to add a helper function that allows the agent to serve on a TCP host port pair and also make client connect to it. Is there a real use case for it? AFAIK it seems Zed only supports stdio streams. |
I believe this is useful. Having observed several instances of acp being utilised on SaaS platforms, I support it. Additionally, we may need to consider how to address the changes introduced by this PR agentclientprotocol/agent-client-protocol#252 |
Hmm, the schema generation failed on the new json schema. Not sure whether it is an issue with the schema or generator. |
Just need to add a small guard in scripts/gen_schema.py that rewrites bare booleans under anyOf/allOf/oneOf to {}/{"not":{}} so datamodel-code-generator doesn’t choke. And I only checked the main schema so far — for the "unstable schema" parts, probably need to add a few small handlers as well. |
|
@PsiACE I found a transform on my end that can rewrite these bools. will push it up |
Thanks, @benbrandt . That will make things a lot smoother for the Python generator. Also, if it makes sense, I’d like to suggest adding @frostming to the Python team in the org . He’s done an exceptional amount of work — essentially a full rewrite of the SDK — and would be a great fit for long-term maintenance and reviews. |
* feat: update to unstable schema and metadata Signed-off-by: Frost Ming <me@frostming.com> * feat: adapt to the 0.7.0 schema Signed-off-by: Frost Ming <me@frostming.com> --------- Signed-off-by: Frost Ming <me@frostming.com>
Signed-off-by: Frost Ming <me@frostming.com>
- LInt and fix examples/* - Switch to pdm.backend - Exclude .uv-cache and .github from sdist build Signed-off-by: Frost Ming <me@frostming.com>
|
We've been on this branch for a while, and agentclientprotocol is now at version 0.9. Given the relatively short interval, I'd prefer to skip the intermediate versions and release a 0.9.x. |
|
Don't feel your sdk needs to match the Json schema version We don't for any other SDK |
|
I am aiming to get the JSON schema representation to a 1.0, but this already doesn't match the protocol version, and it is likely you'll want to have a breaking change at some point to make things nicer for your user that won't be aligned with a release of a breaking change in the schema representation or the protocol itself |
|
Tldr; feel free to release as 0.7 which is likely more intuitive to your users. The schema representation changed but the wire format has remained uncuanged |
That sounds good too, thank you for pointing that out. In fact, others have suggested this approach as well. So the Python SDK will establish its own repair and release schedule. |
* chore: make check happy Signed-off-by: Chojan Shang <chojan.shang@vesoft.com> * chore: gen schema with 0.9.1 Signed-off-by: Chojan Shang <chojan.shang@vesoft.com> * chore: bump to 0.7.0 Signed-off-by: Chojan Shang <chojan.shang@vesoft.com> --------- Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>
There was a problem hiding this 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 introduces significant Pythonic restructuring in preparation for the 0.7.x release, upgrading the protocol schema from v0.6.3 to v0.9.1. The changes focus on improving code quality, maintainability, and developer experience while maintaining backward compatibility.
Key Changes:
- Migrated to snake_case naming convention throughout the API with a comprehensive compatibility layer
- Introduced new high-level
run_agent()andconnect_to_agent()helpers to simplify common workflows - Refactored the router system for better maintainability and reduced boilerplate
Reviewed changes
Copilot reviewed 44 out of 47 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/acp/schema.py |
Updated schema to v0.9.1 with snake_case field names and new models (ListSessionsResponse, SessionCapabilities, etc.) |
src/acp/router.py |
Simplified router with integrated legacy API support and unstable protocol warnings |
src/acp/utils.py |
Added param_model decorator, to_camel_case converter, and compatibility wrapper functions |
src/acp/interfaces.py |
Updated Protocol definitions with snake_case methods and @param_model decorators |
src/acp/core.py |
Added run_agent() and connect_to_agent() convenience functions |
src/acp/client/connection.py |
Migrated to snake_case API with @compatible_class decorator for backward compatibility |
src/acp/agent/connection.py |
Similar migration with added listen() method for manual receive loop control |
tests/conftest.py |
Centralized test fixtures with TestClient and TestAgent implementations |
tests/test_compatibility.py |
New comprehensive backward compatibility tests |
examples/*.py |
Updated all examples to use new snake_case API |
scripts/gen_*.py |
Enhanced code generation with signature generation and improved schema processing |
The PR successfully balances modernization with backward compatibility, providing deprecation warnings to guide users toward the new API patterns.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| server.server_reader, | ||
| ) | ||
| agent_conn = ClientSideConnection(client, server._client_writer, server._client_reader) # type: ignore[arg-type] | ||
| _agent_conn = AgentSideConnection( |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable _agent_conn is not used.
| assert resp.session_id == "test-session-123" | ||
|
|
||
| with pytest.warns(DeprecationWarning) as record: | ||
| resp = await agent_conn.new_session(cwd="/home/tmp", mcp_servers=[]) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to 'resp' is unnecessary as it is redefined before this value is used.
| for offset, line in enumerate(to_insert.splitlines(), 1): | ||
| lines.insert(insert_idx + offset, line) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nested for statement uses loop variable 'line' of enclosing for statement.
| for offset, line in enumerate(to_insert.splitlines(), 1): | |
| lines.insert(insert_idx + offset, line) | |
| for offset, insert_line in enumerate(to_insert.splitlines(), 1): | |
| lines.insert(insert_idx + offset, insert_line) |
Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>
|
The major changes appear to be complete. I will merge this PR and release version 0.7.0 to gather some feedback. |
Summary
Bring in the
0.7.xchanges, which resolve most points raised in #30.Includes major refactoring and compatibility-preserving improvements — with special thanks to @frostming for the exceptional contributions.
Related issues
Closes #30
Testing
make checkmake test0.7.xChecklist
chore:for DNM aggregation work)0.7.xrefactor tests)make gen-all) not applicable at this stage