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."""