fix(acp): make session/close and auth errors spec-compliant (#58)

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.

Author: elkaix

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

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

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
+                        ],
                     }
                 )
 

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

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