From f9570471785506e531469668fc945e7a2e26d904 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 2 Jun 2026 10:58:38 -0400 Subject: [PATCH] fix(acp): make session/close and auth errors spec-compliant 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. --- CHANGELOG.md | 5 +++++ docs/en/release-notes/changelog.md | 5 +++++ src/pythinker_code/acp/server.py | 18 ++++++++++++--- tests/acp/test_server_initialize.py | 27 +++++++++++++++++++---- tests/ui_and_conv/test_acp_server_auth.py | 27 +++++++++++++++++++++++ 5 files changed, 75 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3865ed0..2cdcdf80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,11 @@ GitHub Releases page; `0.8.0` is the new starting line. ## Unreleased +- **ACP session close and auth errors are spec-compliant.** `session/close` now cancels any + in-flight work and frees the session's runtime resources (MCP toolset, background refresh) + before dropping it, instead of leaking them, per the ACP `session/close` requirement. The + `authenticate` failure path now serializes its `authMethods` to plain dicts so the JSON-RPC + error response no longer raises `TypeError` on encode. - **Auto-mode destructive actions deliberate per turn and context.** Auto-deliberation now scopes destructive-command one-shots to the active execution context and LLM generation, so duplicate destructive calls in one response keep bouncing while later deliberate retries diff --git a/docs/en/release-notes/changelog.md b/docs/en/release-notes/changelog.md index f6b71103..3d5c2e44 100644 --- a/docs/en/release-notes/changelog.md +++ b/docs/en/release-notes/changelog.md @@ -17,6 +17,11 @@ GitHub Releases page; `0.8.0` is the new starting line. ## Unreleased +- **ACP session close and auth errors are spec-compliant.** `session/close` now cancels any + in-flight work and frees the session's runtime resources (MCP toolset, background refresh) + before dropping it, instead of leaking them, per the ACP `session/close` requirement. The + `authenticate` failure path now serializes its `authMethods` to plain dicts so the JSON-RPC + error response no longer raises `TypeError` on encode. - **Auto-mode destructive actions deliberate per turn and context.** Auto-deliberation now scopes destructive-command one-shots to the active execution context and LLM generation, so duplicate destructive calls in one response keep bouncing while later deliberate retries diff --git a/src/pythinker_code/acp/server.py b/src/pythinker_code/acp/server.py index 6b7e5f80..e98cea48 100644 --- a/src/pythinker_code/acp/server.py +++ b/src/pythinker_code/acp/server.py @@ -376,9 +376,18 @@ async def set_session_mode( async def close_session( self, session_id: str, **kwargs: Any ) -> acp.schema.CloseSessionResponse | None: - """Drop a session from the in-memory registry (ACP 0.10 session/close).""" + """Close a session: cancel in-flight work, then free its resources (ACP 0.10). + + The ``session/close`` spec requires the agent to cancel any ongoing work + (as if ``session/cancel`` was called) and then release the resources + associated with the session before dropping it from the registry. + """ logger.info("Closing session: {id}", id=session_id) - self.sessions.pop(session_id, None) + entry = self.sessions.pop(session_id, None) + if entry is not None: + acp_session, _ = entry + await acp_session.cancel() + await acp_session.cli.cleanup_runtime_resources() return None async def set_config_option( @@ -469,7 +478,10 @@ async def authenticate(self, method_id: str, **kwargs: Any) -> acp.AuthenticateR raise acp.RequestError.auth_required( { "message": "Please complete login in terminal first", - "authMethods": self._auth_methods, + "authMethods": [ + m.model_dump(by_alias=True, exclude_none=True) + for m in self._auth_methods + ], } ) diff --git a/tests/acp/test_server_initialize.py b/tests/acp/test_server_initialize.py index 3f8d80e3..7ed75ec5 100644 --- a/tests/acp/test_server_initialize.py +++ b/tests/acp/test_server_initialize.py @@ -2,6 +2,9 @@ from __future__ import annotations +from types import SimpleNamespace +from unittest.mock import AsyncMock + import acp.schema import pytest @@ -34,14 +37,30 @@ async def test_initialize_advertises_terminal_auth_method(): assert auth_method.env == {} -async def test_close_session_drops_session_from_registry(): - """ACP 0.10 session/close removes the session from the in-memory registry.""" +async def test_close_session_cancels_and_releases_resources(): + """ACP 0.10 session/close must cancel in-flight work and free per-session + resources before dropping the session. + + The spec mandates the agent "must cancel any ongoing work related to the + session (treat it as if ``session/cancel`` was called) and then free up any + resources associated with the session." + """ server = ACPServer() - server.sessions["sess-1"] = ("placeholder",) # type: ignore[assignment] + cli = SimpleNamespace(cleanup_runtime_resources=AsyncMock()) + acp_session = SimpleNamespace(cancel=AsyncMock(), cli=cli) + # Registry stores ``tuple[ACPSession, _ModelIDConv]``. + server.sessions["sess-1"] = (acp_session, object()) # type: ignore[assignment] result = await server.close_session("sess-1") assert result is None assert "sess-1" not in server.sessions - # Closing an unknown session is a no-op (must not raise). + acp_session.cancel.assert_awaited_once() + cli.cleanup_runtime_resources.assert_awaited_once() + + +async def test_close_session_unknown_is_noop(): + """Closing an unknown session must be a no-op (no raise, no cleanup).""" + server = ACPServer() + assert await server.close_session("missing") is None diff --git a/tests/ui_and_conv/test_acp_server_auth.py b/tests/ui_and_conv/test_acp_server_auth.py index 7d244245..2065dbe9 100644 --- a/tests/ui_and_conv/test_acp_server_auth.py +++ b/tests/ui_and_conv/test_acp_server_auth.py @@ -169,6 +169,33 @@ async def test_authenticate_rejects_expired_token_without_refresh(server: ACPSer assert exc_info.value.code == -32000 +@pytest.mark.asyncio +async def test_authenticate_auth_required_data_is_json_serializable(server: ACPServer) -> None: + """The AUTH_REQUIRED error from authenticate() must carry JSON-serializable + ``authMethods`` (plain dicts), not raw Pydantic models. + + The JSON-RPC connection layer encodes the error ``data`` with a plain + ``json.dumps`` (no Pydantic-aware default encoder), so raw models would raise + ``TypeError`` while building the error response — crashing the agent process. + """ + import json + + token = _make_token(expires_at=time.time() - 100, refresh_token="") + + with ( + patch("pythinker_code.acp.server.load_config", return_value=Config()), + patch("pythinker_code.acp.server.load_tokens", return_value=token), + pytest.raises(acp.RequestError) as exc_info, + ): + await server.authenticate(method_id="login") + + data = exc_info.value.data + assert isinstance(data, dict) + # Must not raise TypeError on encode (the bug: raw Pydantic models). + json.dumps(data) + assert all(isinstance(m, dict) for m in data["authMethods"]) + + @pytest.mark.asyncio async def test_authenticate_accepts_valid_token(server: ACPServer) -> None: """authenticate('login') should succeed for a valid, non-expired token."""