fix(acp): make session/close and auth errors spec-compliant - #58
Merged
Conversation
session/close now cancels in-flight work and releases per-session runtime
resources (MCP toolset, background refresh) before dropping the session, as the
ACP session/close spec requires ("the agent must cancel any ongoing work ...
and then free up any resources associated with the session"). Previously it
only popped the registry entry, leaking the MCP clients and background-refresh
task of every closed session.
The authenticate() failure path passed its authMethods as raw Pydantic models
inside the error data. The JSON-RPC connection layer encodes error data with a
plain json.dumps (no Pydantic-aware encoder), so that path raised TypeError
while building the response. Serialize to plain dicts via
model_dump(by_alias=True, exclude_none=True), matching the _check_auth path.
Adds regression tests for both; updates the close_session test to assert the
cancel + cleanup contract instead of a synthetic placeholder tuple.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR fixes two ACP spec-compliance bugs: ChangesACP Session and Auth Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two ACP 0.10 fixes surfaced by a deep-code-scan of the dependabot dep-bump, re-validated against the live code, the installed
agent-client-protocol==0.10.1, and the current ACP spec.session/closeresource leak + spec violationclose_sessionpreviously only didself.sessions.pop(...). The ACPsession/closespec mandates the agent must "cancel any ongoing work related to the session (treat it as ifsession/cancelwas called) and then free up any resources associated with the session." The fix cancels the in-flight turn and callsPythinkerCLI.cleanup_runtime_resources()(the same teardown used by the shell-reload / shutdown paths), which tears down the per-session MCP toolset and background-refresh task, before dropping the registry entry.authenticateerror path raisedTypeErroron encodeThe
authenticate("login")failure path put raw PydanticauthMethodsmodels in the JSON-RPC errordata. The connection layer encodes error data with a plainjson.dumps(no Pydantic-aware encoder), so that path raisedTypeError: Object of type TerminalAuthMethod is not JSON serializablewhile building the response. Now serialized to plain dicts viamodel_dump(by_alias=True, exclude_none=True), matching the existing_check_authpath.Tests
test_authenticate_auth_required_data_is_json_serializable— asserts the errordataisjson.dumps-able andauthMethodsare dicts (fails onmainwith the exactTypeError).test_close_session_cancels_and_releases_resources— assertscancel()andcleanup_runtime_resources()are awaited (replaces the prior synthetic-tuple test).test_close_session_unknown_is_noop— closing an unknown session stays a no-op.Verification
tests/acp+tests/ui_and_conv→ 1472 passed.ruff check,ruff format --check,pyright→ clean on changed files.Notes
session/closeis routed (the agent runs withuse_unstable_protocol=True) butinitialize()does not advertisesessionCapabilities.close, so spec-compliant clients won't call it today — this hardens the path against non-compliant callers and makes it correct for when the capability is advertised. Whether to advertisecloseis left as a separate decision.Summary by CodeRabbit
Bug Fixes