diff --git a/.agents/skills/pydantic-acp/SKILL.md b/.agents/skills/pydantic-acp/SKILL.md index 8fda35d..160ee12 100644 --- a/.agents/skills/pydantic-acp/SKILL.md +++ b/.agents/skills/pydantic-acp/SKILL.md @@ -364,6 +364,20 @@ agent = Agent(model) Do not pass arbitrary CLI commands here; the process must be an ACP server. Each command-backed model owns one child ACP process and should be reused for repeated subagent calls. +`create_acp_model(...)` and `AcpProvider(...)` recover from an ACP +`auth_required` response by authenticating with the first advertised +`AuthMethodAgent` and retrying `session/new` once. Use `auth_method_id=...` only +when a specific method has already been prepared. `EnvVarAuthMethod` and +`TerminalAuthMethod` require host-owned credential injection or terminal +execution and must not be selected automatically. + +Use `AcpProvider.ensure_session()` to initialize and create a session without a +prompt. Use `AcpProvider.set_session_mode(...)` to bootstrap that session and +select a mode before the first model turn. Set `raise_on_empty_turn=True` on the +provider or factory when a silent text turn must raise +`UnexpectedModelBehavior`; the backward-compatible default returns an empty +response. + Use `AcpProvider(acp_agent=...)` directly only when provider ownership, host delegation, or lower level lifecycle control matters. @@ -464,6 +478,11 @@ Stay in this skill when the main issue is: `plan`; the runtime must fall back to full updates otherwise. - Accept and persist `AcpMcpServer` session payloads, but do not advertise ACP MCP transport capability until the SDK exposes a public router. +- Automatically authenticate only with `AuthMethodAgent`. Environment-variable + and terminal auth methods require client-owned setup before `authenticate` + can be called. +- Unwrap only single-child groups whose message identifies an anyio TaskGroup; + preserve unrelated exception groups and cancellation. - Do not describe `pydantic-acp` as transport. - Do not promise ACP state the active `pydantic_ai.Agent` cannot honor. diff --git a/CHANGELOG.md b/CHANGELOG.md index 4dcd38f..2e1af82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,32 @@ ACP Kit uses synchronized versions for `acpkit`, `pydantic-acp`, `langchain-acp` ## [Unreleased] +## [1.5.1] - 2026-07-24 + +### Added + +- `pydantic-acp`'s `AcpProvider` recovers from an `auth_required` (`-32000`) + rejection of `session/new` by running the ACP `authenticate` flow — using an + advertised agent-managed method or an explicit `auth_method_id=` — and + retrying session creation once. Environment-variable and terminal methods + remain client-owned because they require credential injection or an + interactive process. +- Public `AcpProvider.ensure_session()` and `AcpProvider.set_session_mode()` + bootstrap a session and select a session mode without sending a prompt turn, + so callers no longer reach into the private `_ensure_session`. +- Opt-in `AcpProvider(raise_on_empty_turn=True)` raises + `UnexpectedModelBehavior` with an ACP-specific diagnostic when a text-output + turn yields no visible text. The default remains `False`, preserving the + empty-response contract. + +### Fixed + +- `pydantic-acp`'s `request_prompt` propagates the ACP agent's real error + (rate limit, auth rejection, upstream API failure) by unwrapping single-child + anyio TaskGroup exception wrappers and dropping matching TaskGroup + `__context__` noise, while preserving unrelated aggregate errors and + cancellation. + ## [1.5.0] - 2026-07-24 ### Changed diff --git a/COVERAGE b/COVERAGE index 721f3c3..ad42586 100644 --- a/COVERAGE +++ b/COVERAGE @@ -1,2 +1,2 @@ -Line coverage: 100.00% (10329 / 10329) -Branch coverage: 100.00% (3434 / 3434) +Line coverage: 100.00% (10398 / 10398) +Branch coverage: 100.00% (3456 / 3456) diff --git a/README.md b/README.md index e1ace70..2211dbf 100644 --- a/README.md +++ b/README.md @@ -303,10 +303,17 @@ model = create_acp_model( acp_command=("npx", "@zed-industries/codex-acp"), cwd="/workspace", stderr_mode="inherit", + raise_on_empty_turn=True, ) agent = Agent(model) ``` +When `session/new` reports `auth_required`, the provider authenticates with the +first agent-managed method advertised by the ACP agent and retries once. Pass +`auth_method_id="..."` to `create_acp_model(...)` or `AcpProvider(...)` to +select a specific prepared method. Environment-variable and terminal methods +still require their client-side credential or terminal setup. + For lower-level ownership, construct the provider directly: ```python @@ -315,6 +322,8 @@ from pydantic_acp import AcpProvider # `remote_acp_agent` can be any object implementing the ACP Agent interface. provider = AcpProvider(acp_agent=remote_acp_agent, cwd="/workspace") +session_id = await provider.ensure_session() +await provider.set_session_mode("review") model = provider.model() agent = Agent(model) diff --git a/VERSION b/VERSION index bc80560..26ca594 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.5.0 +1.5.1 diff --git a/docs/compatibility-matrix-template.md b/docs/compatibility-matrix-template.md index 9c43825..409f123 100644 --- a/docs/compatibility-matrix-template.md +++ b/docs/compatibility-matrix-template.md @@ -64,8 +64,10 @@ manifest = CompatibilityManifest( rationale='The runtime keeps the hook seam but does not expose it to ACP clients.', ), 'authenticate': SurfaceSupport( - status='planned', - rationale='No auth handshake has been added yet.', + status='implemented', + owner='provider', + mapping='AcpProvider auth_required recovery', + rationale='Agent-managed methods are automatic; env and terminal setup stays client-owned.', ), }, ) diff --git a/docs/pydantic-acp.md b/docs/pydantic-acp.md index 66be201..4ee12cc 100644 --- a/docs/pydantic-acp.md +++ b/docs/pydantic-acp.md @@ -102,6 +102,7 @@ model = create_acp_model( acp_command=("npx", "@zed-industries/codex-acp"), cwd="/workspace", stderr_mode="inherit", + raise_on_empty_turn=True, ) agent = Agent(model) ``` @@ -112,6 +113,37 @@ ACP agent exposes a selectable `"model"` `session/set_config_option` option. `AcpProvider` and `AcpModel` remain available when lower-level provider ownership is needed. +For command-backed and in-process ACP agents, session setup is explicit and +does not require a dummy prompt: + +```python +from acp.interfaces import Agent as AcpAgent +from pydantic_acp import AcpProvider + + +async def prepare_provider(acp_agent: AcpAgent) -> AcpProvider: + provider = AcpProvider( + acp_agent=acp_agent, + cwd="/workspace", + raise_on_empty_turn=True, + ) + await provider.ensure_session() + await provider.set_session_mode("review") + return provider +``` + +If `session/new` fails with ACP `auth_required`, `AcpProvider` calls +`authenticate` with the first advertised `AuthMethodAgent` and retries session +creation once. `auth_method_id="..."` selects a specific method when the host +has prepared it. `EnvVarAuthMethod` and `TerminalAuthMethod` require +client-owned credential injection or terminal execution, so the provider does +not select them automatically. + +Agent errors are propagated without single-child anyio TaskGroup wrapping. +`raise_on_empty_turn=True` additionally turns a text request that produces no +visible agent text into `UnexpectedModelBehavior`; the default remains `False` +for backward compatibility. + The bridge keeps ownership explicit: - Pydantic AI owns the outer run, output validation, and normal provider diff --git a/packages/adapters/langchain-acp/VERSION b/packages/adapters/langchain-acp/VERSION index bc80560..26ca594 100644 --- a/packages/adapters/langchain-acp/VERSION +++ b/packages/adapters/langchain-acp/VERSION @@ -1 +1 @@ -1.5.0 +1.5.1 diff --git a/packages/adapters/langchain-acp/src/langchain_acp/_version.py b/packages/adapters/langchain-acp/src/langchain_acp/_version.py index dbcf630..f8a095a 100644 --- a/packages/adapters/langchain-acp/src/langchain_acp/_version.py +++ b/packages/adapters/langchain-acp/src/langchain_acp/_version.py @@ -2,4 +2,4 @@ __all__ = ("__version__",) -__version__ = "1.5.0" +__version__ = "1.5.1" diff --git a/packages/adapters/pydantic-acp/README.md b/packages/adapters/pydantic-acp/README.md index cc1df87..4a91ca8 100644 --- a/packages/adapters/pydantic-acp/README.md +++ b/packages/adapters/pydantic-acp/README.md @@ -121,10 +121,19 @@ model = create_acp_model( acp_command=("npx", "@zed-industries/codex-acp"), cwd="/workspace", stderr_mode="inherit", + raise_on_empty_turn=True, ) agent = Agent(model) ``` +`raise_on_empty_turn=True` converts a silent text turn into an ACP-specific +`UnexpectedModelBehavior` instead of returning an empty response. When +`session/new` reports `auth_required`, the provider calls `authenticate` with +the first advertised agent-managed method and retries session creation once. +Use `auth_method_id="..."` to select a specific method that has already been +prepared by the host. Environment-variable and terminal auth methods require +client-side credential or terminal setup and are not selected automatically. + For lower-level ownership, construct the provider directly: ```python @@ -133,6 +142,8 @@ from pydantic_acp import AcpProvider # `remote_acp_agent` can be any object implementing the ACP Agent interface. provider = AcpProvider(acp_agent=remote_acp_agent, cwd="/workspace") +session_id = await provider.ensure_session() +await provider.set_session_mode("review") model = provider.model() agent = Agent(model) @@ -144,6 +155,7 @@ This keeps ownership boundaries explicit: - Pydantic AI owns the outer agent run, output validation, and normal model/provider lifecycle. - ACP owns the delegated agent session, ACP-visible updates, and any editor or host capabilities requested by that agent. +- `ensure_session()` initializes and creates the ACP session without consuming a prompt turn; `set_session_mode(...)` uses that same session. - `create_acp_model(...)` and `provider.model()` leave ACP model selection to the wrapped agent's session default; pass `model_name="zed-agent"` or `provider.model("zed-agent")` only when the ACP agent exposes a selectable `"model"` `session/set_config_option` option. - `AcpHostBridge` records ACP `session_update` messages and can delegate filesystem, terminal, approval, and extension callbacks to a real ACP host client when one is supplied. - Pydantic AI function tools are intentionally not executed directly by `AcpModel`; register tools on the ACP agent or expose host capabilities through ACP. diff --git a/packages/adapters/pydantic-acp/VERSION b/packages/adapters/pydantic-acp/VERSION index bc80560..26ca594 100644 --- a/packages/adapters/pydantic-acp/VERSION +++ b/packages/adapters/pydantic-acp/VERSION @@ -1 +1 @@ -1.5.0 +1.5.1 diff --git a/packages/adapters/pydantic-acp/src/pydantic_acp/_version.py b/packages/adapters/pydantic-acp/src/pydantic_acp/_version.py index dbcf630..f8a095a 100644 --- a/packages/adapters/pydantic-acp/src/pydantic_acp/_version.py +++ b/packages/adapters/pydantic-acp/src/pydantic_acp/_version.py @@ -2,4 +2,4 @@ __all__ = ("__version__",) -__version__ = "1.5.0" +__version__ = "1.5.1" diff --git a/packages/adapters/pydantic-acp/src/pydantic_acp/client.py b/packages/adapters/pydantic-acp/src/pydantic_acp/client.py index ad5eeef..ed1ab91 100644 --- a/packages/adapters/pydantic-acp/src/pydantic_acp/client.py +++ b/packages/adapters/pydantic-acp/src/pydantic_acp/client.py @@ -10,32 +10,37 @@ from uuid import uuid4 from acp import PROTOCOL_VERSION +from acp.exceptions import RequestError from acp.helpers import text_block from acp.interfaces import Agent as AcpAgent from acp.interfaces import Client as AcpClient from acp.schema import ( AgentMessageChunk, + AuthMethodAgent, ClientCapabilities, ClientSessionCapabilities, CreateElicitationResponse, CreateTerminalResponse, ElicitationMode, + EnvVarAuthMethod, EnvVariable, Implementation, KillTerminalResponse, + NewSessionResponse, PermissionOption, ReadTextFileResponse, ReleaseTerminalResponse, RequestPermissionResponse, SessionConfigOptionsCapabilities, SessionConfigOptionSelect, + TerminalAuthMethod, TerminalOutputResponse, ToolCallUpdate, UsageUpdate, WaitForTerminalExitResponse, WriteTextFileResponse, ) -from pydantic_ai.exceptions import UserError +from pydantic_ai.exceptions import UnexpectedModelBehavior, UserError from pydantic_ai.messages import ( AudioUrl, BinaryContent, @@ -63,6 +68,7 @@ from pydantic_ai.providers import Provider from pydantic_ai.settings import ModelSettings from pydantic_ai.usage import RequestUsage +from typing_extensions import TypeIs from ._meta_protocol import ( MISSING_STRUCTURED_OUTPUT, @@ -74,7 +80,12 @@ from .types import AgentPromptBlock HistoryMode: TypeAlias = Literal["latest_user", "full"] +AuthMethod: TypeAlias = EnvVarAuthMethod | TerminalAuthMethod | AuthMethodAgent _DEFAULT_MODEL_NAME = "agent" +_TASK_GROUP_ERROR_MESSAGE = "unhandled errors in a TaskGroup" +_AUTH_REQUIRED_DIAGNOSTIC = ( + "The ACP agent requires authentication (session/new returned auth_required / -32000)" +) AcpPromptRenderer: TypeAlias = Callable[ [Sequence[ModelMessage], ModelRequestParameters], @@ -407,6 +418,36 @@ def _default_client_capabilities() -> ClientCapabilities: ) +def _is_task_group_error(exc: BaseException) -> TypeIs[ExceptionGroup[Exception]]: + return isinstance(exc, ExceptionGroup) and exc.message == _TASK_GROUP_ERROR_MESSAGE + + +def _unwrap_acp_error(exc: Exception) -> Exception: + """Return the ACP agent's real error, free of anyio TaskGroup wrapping. + + ACP calls run over a stdio connection whose background reader lives in an + anyio task group. An agent/protocol error (rate limit, auth rejection, + upstream API failure) can therefore reach the caller wrapped as a + single-child :class:`BaseExceptionGroup` ("unhandled errors in a + TaskGroup"), or as the real error carrying that group as its + ``__context__``. Both bury the actual cause. This peels single-child groups + down to the leaf and drops a TaskGroup ``__context__`` so the meaningful + error propagates on its own instead of as an opaque group. Other exception + groups are preserved because they may carry meaningful aggregate failures. + """ + leaf: Exception = exc + while ( + _is_task_group_error(leaf) + and len(leaf.exceptions) == 1 + and isinstance(leaf.exceptions[0], Exception) + ): + leaf = leaf.exceptions[0] + if leaf.__context__ is not None and _is_task_group_error(leaf.__context__): + leaf.__context__ = None + leaf.__suppress_context__ = True + return leaf + + # --------------------------------------------------------------------------- # AcpProvider # --------------------------------------------------------------------------- @@ -445,6 +486,8 @@ def __init__( prompt_renderer: AcpPromptRenderer | None = None, history_mode: HistoryMode = "latest_user", enable_pydantic_acp_meta: bool | None = None, + auth_method_id: str | None = None, + raise_on_empty_turn: bool = False, ) -> None: """Create a new ACP provider. @@ -477,6 +520,17 @@ def __init__( output. ``None`` auto-enables it only for ACP agents produced by this package; arbitrary ACP agents must not be trusted to implement this private contract. + auth_method_id: ACP ``authenticate`` method id to use when the + agent rejects ``session/new`` with an ``auth_required`` + (``-32000``) error. When ``None`` the provider falls back to + the first authentication method advertised by the agent's + ``initialize`` response. + raise_on_empty_turn: When ``True``, a prompt turn that produces no + visible text for a text-output request raises + :class:`~pydantic_ai.exceptions.UnexpectedModelBehavior` with an + ACP-specific diagnostic instead of returning an empty response. + Defaults to ``False`` to preserve the standard contract where an + empty ACP turn yields a response with no parts. """ self._client = acp_agent @@ -499,10 +553,14 @@ def __init__( if enable_pydantic_acp_meta is None else enable_pydantic_acp_meta ) + self._auth_method_id = auth_method_id + self._raise_on_empty_turn = raise_on_empty_turn self._initialized = False self._session_id: str | None = None self._current_model_name: str | None = None self._model_config_option_available: bool | None = None + self._auth_methods: list[AuthMethod] = [] + self._authenticated = False self._session_lock: asyncio.Lock | None = None self._session_lock_loop: asyncio.AbstractEventLoop | None = None @@ -549,6 +607,11 @@ def enable_pydantic_acp_meta(self) -> bool: """Whether this provider opts into private ``pydantic_acp`` ACP metadata.""" return self._enable_pydantic_acp_meta + @property + def raise_on_empty_turn(self) -> bool: + """Whether an empty ACP turn raises instead of returning empty parts.""" + return self._raise_on_empty_turn + async def __aexit__( self, exc_type: type[BaseException] | None, @@ -618,42 +681,56 @@ async def request_prompt( prompt: Sequence[AgentPromptBlock], model_request_parameters: ModelRequestParameters, ) -> _AcpPromptResult: - """Send one prompt turn to the ACP agent and collect its visible text.""" - session_id = await self._ensure_session(model_name=model_name) - start_index = self._host.snapshot_index() - request_meta = None - if self._enable_pydantic_acp_meta: - request_meta = build_structured_output_request_meta(model_request_parameters) - prompt_kwargs: dict[str, Any] = { - "prompt": list(prompt), - "session_id": session_id, - "message_id": uuid4().hex, - } - if request_meta is not None: - prompt_kwargs["_meta"] = request_meta - prompt_response = await self._client.prompt(**prompt_kwargs) - text = await self._agent_message_text_after_prompt( - start_index, - session_id=session_id, - prompt_response=prompt_response, - ) + """Send one prompt turn to the ACP agent and collect its visible text. - response_meta = extract_field_meta(prompt_response) - usage = _usage_from_acp(getattr(prompt_response, "usage", None)) - if not usage.has_values(): - usage = self._host.usage_update_since(start_index, session_id=session_id) - stop_reason = getattr(prompt_response, "stop_reason", None) or getattr( - prompt_response, - "stopReason", - None, - ) - return _AcpPromptResult( - text=text, - usage=usage, - stop_reason=stop_reason, - session_id=session_id, - structured_output=extract_structured_output(response_meta), - ) + Errors raised by the ACP agent (rate limits, auth rejection, upstream + API failures) are propagated with anyio TaskGroup wrapping stripped so + the caller sees the agent's real error rather than an opaque + ``ExceptionGroup: unhandled errors in a TaskGroup``. + """ + try: + session_id = await self._ensure_session(model_name=model_name) + start_index = self._host.snapshot_index() + request_meta = None + if self._enable_pydantic_acp_meta: + request_meta = build_structured_output_request_meta(model_request_parameters) + prompt_kwargs: dict[str, Any] = { + "prompt": list(prompt), + "session_id": session_id, + "message_id": uuid4().hex, + } + if request_meta is not None: + prompt_kwargs["_meta"] = request_meta + prompt_response = await self._client.prompt(**prompt_kwargs) + text = await self._agent_message_text_after_prompt( + start_index, + session_id=session_id, + prompt_response=prompt_response, + ) + + response_meta = extract_field_meta(prompt_response) + usage = _usage_from_acp(getattr(prompt_response, "usage", None)) + if not usage.has_values(): + usage = self._host.usage_update_since(start_index, session_id=session_id) + stop_reason = getattr(prompt_response, "stop_reason", None) or getattr( + prompt_response, + "stopReason", + None, + ) + return _AcpPromptResult( + text=text, + usage=usage, + stop_reason=stop_reason, + session_id=session_id, + structured_output=extract_structured_output(response_meta), + ) + except asyncio.CancelledError: + raise + except Exception as exc: + cleaned = _unwrap_acp_error(exc) + if cleaned is exc: + raise + raise cleaned from None async def _agent_message_text_after_prompt( self, @@ -680,18 +757,16 @@ async def _agent_message_text_after_prompt( async def _ensure_session(self, *, model_name: str | None) -> str: async with self._get_session_lock(): if not self._initialized: - await self._client.initialize( + init_response = await self._client.initialize( protocol_version=self._protocol_version, client_capabilities=self._client_capabilities or _default_client_capabilities(), client_info=self._client_info, ) + self._auth_methods = list(getattr(init_response, "auth_methods", None) or []) self._initialized = True if self._session_id is None: - session = await self._client.new_session( - cwd=self._cwd, - mcp_servers=list(self._mcp_servers), - ) + session = await self._new_session_with_auth() self._session_id = session.session_id self._model_config_option_available = any( isinstance(option, SessionConfigOptionSelect) and option.id == "model" @@ -719,6 +794,86 @@ async def _ensure_session(self, *, model_name: str | None) -> str: return self._session_id + async def _new_session_with_auth(self) -> NewSessionResponse: + """Create an ACP session, authenticating once if the agent demands it. + + ACP agents may reject ``session/new`` with an ``auth_required`` + (``-32000``) error until the client has authenticated. The base + ``initialize``/``session/new`` handshake never calls ``authenticate``, + so such agents are otherwise unrecoverable. This retries session + creation exactly once after a successful ``authenticate`` call. + """ + try: + return await self._call_new_session() + except RequestError as exc: + if exc.code != RequestError.auth_required().code or self._authenticated: + raise + await self._authenticate() + return await self._call_new_session() + + async def _call_new_session(self) -> NewSessionResponse: + return await self._client.new_session( + cwd=self._cwd, + mcp_servers=list(self._mcp_servers), + ) + + async def _authenticate(self) -> None: + """Run the ACP ``authenticate`` flow using an advertised auth method.""" + authenticate = getattr(self._client, "authenticate", None) + if authenticate is None: + raise UserError( + f"{_AUTH_REQUIRED_DIAGNOSTIC} but the wrapped agent does not expose an " + "'authenticate' method, so the session cannot be established." + ) + agent_method = next( + (method for method in self._auth_methods if isinstance(method, AuthMethodAgent)), + None, + ) + method_id = self._auth_method_id or (agent_method.id if agent_method is not None else None) + if method_id is None: + advertised = ", ".join(method.id for method in self._auth_methods) + raise UserError( + f"{_AUTH_REQUIRED_DIAGNOSTIC} but advertised no agent-managed authentication " + f"method to satisfy it{f' (advertised: {advertised})' if advertised else ''}. " + "Environment-variable and terminal methods require client-side setup. Complete " + "that setup out of band, or pass auth_method_id=... after preparing the agent." + ) + result = authenticate(method_id=method_id) + if inspect.isawaitable(result): + await result + self._authenticated = True + + async def ensure_session(self, *, model_name: str | None = None) -> str: + """Initialize the ACP connection and return an active session id. + + This is the public entry point for bootstrapping a session without + sending a prompt turn: it performs ``initialize`` (authenticating if the + agent demands it), creates the session, and optionally selects a model. + Callers that need to configure a session up front (for example to set a + session mode) should use this instead of reaching into the private + ``_ensure_session``. + """ + return await self._ensure_session(model_name=model_name) + + async def set_session_mode(self, mode_id: str) -> None: + """Set the ACP session mode, bootstrapping the session if needed. + + ACP session modes (for example the permission mode) are distinct from + the ``model`` session config option and are set via ``session/set_mode``. + This ensures a session exists, then delegates to the wrapped agent's + ``set_session_mode`` when available. + """ + session_id = await self._ensure_session(model_name=None) + set_mode = getattr(self._client, "set_session_mode", None) + if set_mode is None: + raise UserError( + "The ACP agent does not expose 'set_session_mode', so the session " + f"mode {mode_id!r} cannot be selected." + ) + result = set_mode(session_id=session_id, mode_id=mode_id) + if inspect.isawaitable(result): + await result + def _get_session_lock(self) -> asyncio.Lock: loop = asyncio.get_running_loop() if self._session_lock is None or self._session_lock_loop is not loop: @@ -877,7 +1032,18 @@ def _response_parts( "ACP agent did not return pydantic_acp structured output metadata for " "this structured-output request.", ) - return [TextPart(acp_result.text, provider_name=self.system)] if acp_result.text else [] + if acp_result.text: + return [TextPart(acp_result.text, provider_name=self.system)] + if cast(AcpProvider, self._provider).raise_on_empty_turn: + stop_reason = acp_result.stop_reason + raise UnexpectedModelBehavior( + "The ACP agent ended its turn without producing any text output" + f"{f' (stop reason: {stop_reason})' if stop_reason else ''}. " + "This usually means the ACP agent is unauthenticated, its ACP mode is " + "not functioning, or it silently declined the request. Check that the " + "underlying agent is logged in and that its ACP integration is working." + ) + return [] # --------------------------------------------------------------------------- @@ -1088,7 +1254,7 @@ def _agent_supports_pydantic_acp_meta(acp_agent: AcpAgent) -> bool: return getattr(acp_agent, "_pydantic_acp_meta_supported", False) is True -_TEXT_FIELD_NAMES = ("text", "delta", "message", "output_text", "response", "data") +_TEXT_FIELD_NAMES: tuple[str, ...] = ("text", "delta", "message", "output_text", "response", "data") def _extract_text(value: Any) -> str: diff --git a/packages/adapters/pydantic-acp/src/pydantic_acp/factory.py b/packages/adapters/pydantic-acp/src/pydantic_acp/factory.py index 1eee11b..77a0e5a 100644 --- a/packages/adapters/pydantic-acp/src/pydantic_acp/factory.py +++ b/packages/adapters/pydantic-acp/src/pydantic_acp/factory.py @@ -27,6 +27,8 @@ def create_acp_model( history_mode: HistoryMode = "latest_user", delegate_client: AcpClient | None = None, enable_pydantic_acp_meta: bool | None = None, + auth_method_id: str | None = None, + raise_on_empty_turn: bool = False, settings: ModelSettings | None = None, profile: ModelProfileSpec | None = None, ) -> AcpModel: @@ -35,6 +37,10 @@ def create_acp_model( Exactly one of ``acp_agent`` or ``acp_command`` must be provided. Passing ``model_name=None`` leaves ACP model selection to the remote agent's session default and does not send a ``session/set_config_option`` request for ``"model"``. + ``auth_method_id`` selects an explicit ACP authentication method when + ``session/new`` reports ``auth_required``. Set ``raise_on_empty_turn=True`` + when a silent ACP turn should fail as ``UnexpectedModelBehavior`` instead + of producing an empty Pydantic AI response. """ command = _normalize_command(acp_command) @@ -63,6 +69,8 @@ def create_acp_model( prompt_renderer=prompt_renderer, history_mode=history_mode, enable_pydantic_acp_meta=enable_pydantic_acp_meta, + auth_method_id=auth_method_id, + raise_on_empty_turn=raise_on_empty_turn, ) return provider.model( model_name, diff --git a/packages/helpers/codex-auth-helper/VERSION b/packages/helpers/codex-auth-helper/VERSION index bc80560..26ca594 100644 --- a/packages/helpers/codex-auth-helper/VERSION +++ b/packages/helpers/codex-auth-helper/VERSION @@ -1 +1 @@ -1.5.0 +1.5.1 diff --git a/packages/helpers/codex-auth-helper/src/codex_auth_helper/_version.py b/packages/helpers/codex-auth-helper/src/codex_auth_helper/_version.py index dbcf630..f8a095a 100644 --- a/packages/helpers/codex-auth-helper/src/codex_auth_helper/_version.py +++ b/packages/helpers/codex-auth-helper/src/codex_auth_helper/_version.py @@ -2,4 +2,4 @@ __all__ = ("__version__",) -__version__ = "1.5.0" +__version__ = "1.5.1" diff --git a/packages/transports/acpremote/VERSION b/packages/transports/acpremote/VERSION index bc80560..26ca594 100644 --- a/packages/transports/acpremote/VERSION +++ b/packages/transports/acpremote/VERSION @@ -1 +1 @@ -1.5.0 +1.5.1 diff --git a/packages/transports/acpremote/src/acpremote/_version.py b/packages/transports/acpremote/src/acpremote/_version.py index dbcf630..f8a095a 100644 --- a/packages/transports/acpremote/src/acpremote/_version.py +++ b/packages/transports/acpremote/src/acpremote/_version.py @@ -2,4 +2,4 @@ __all__ = ("__version__",) -__version__ = "1.5.0" +__version__ = "1.5.1" diff --git a/pyproject.toml b/pyproject.toml index f209105..2db0b3d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -23,12 +23,12 @@ dependencies = [ ] [project.optional-dependencies] -codex = ["codex-auth-helper>=1.5.0,<2.0.0"] -deepagents = ["langchain-acp[deepagents]>=1.5.0,<2.0.0"] -langchain = ["langchain-acp>=1.5.0,<2.0.0"] +codex = ["codex-auth-helper>=1.5.1,<2.0.0"] +deepagents = ["langchain-acp[deepagents]>=1.5.1,<2.0.0"] +langchain = ["langchain-acp>=1.5.1,<2.0.0"] launch = ["uv>=0.8.3"] -pydantic = ["pydantic-acp>=1.5.0,<2.0.0"] -remote = ["acpremote>=1.5.0,<2.0.0"] +pydantic = ["pydantic-acp>=1.5.1,<2.0.0"] +remote = ["acpremote>=1.5.1,<2.0.0"] dev = [ "acpremote", diff --git a/src/acpkit/_version.py b/src/acpkit/_version.py index dbcf630..f8a095a 100644 --- a/src/acpkit/_version.py +++ b/src/acpkit/_version.py @@ -2,4 +2,4 @@ __all__ = ("__version__",) -__version__ = "1.5.0" +__version__ = "1.5.1" diff --git a/tests/pydantic/test_acp_client_provider.py b/tests/pydantic/test_acp_client_provider.py index 99de6c7..aa3ab41 100644 --- a/tests/pydantic/test_acp_client_provider.py +++ b/tests/pydantic/test_acp_client_provider.py @@ -6,7 +6,7 @@ import tomllib from pathlib import Path from types import SimpleNamespace -from typing import Any, Literal, cast +from typing import Any, Literal, TypeAlias, cast from unittest.mock import AsyncMock import pydantic_acp @@ -22,10 +22,13 @@ AgentCapabilities, AgentMessageChunk, AllowedOutcome, + AuthEnvVar, + AuthMethodAgent, ClientCapabilities, ElicitationFormSessionMode, ElicitationMode, ElicitationSchema, + EnvVarAuthMethod, Implementation, InitializeResponse, NewSessionResponse, @@ -34,6 +37,7 @@ SessionConfigOptionSelect, SessionConfigSelectOption, SetSessionConfigOptionResponse, + TerminalAuthMethod, ToolCallUpdate, Usage, UsageUpdate, @@ -51,7 +55,7 @@ has_structured_output_request, ) from pydantic_ai import Agent -from pydantic_ai.exceptions import UserError +from pydantic_ai.exceptions import UnexpectedModelBehavior, UserError from pydantic_ai.messages import ( BinaryContent, ImageUrl, @@ -76,6 +80,8 @@ from .support import HostRecordingClient, RecordingClient +TestAuthMethod: TypeAlias = EnvVarAuthMethod | TerminalAuthMethod | AuthMethodAgent + class EchoACPAgent: # type: ignore[misc] def __init__( @@ -415,6 +421,33 @@ async def test_create_acp_model_wraps_in_process_acp_agent() -> None: assert acp_agent.session_models == [("session-1", "zed-agent")] +async def test_create_acp_model_forwards_auth_and_empty_turn_options() -> None: + auth_agent = AuthRequiredACPAgent( + auth_methods=[AuthMethodAgent(id="login", name="Login")], + ) + auth_model = create_acp_model( + acp_agent=cast(AcpAgent, auth_agent), + cwd="/workspace", + auth_method_id="custom", + ) + + await cast(AcpProvider, auth_model.provider).ensure_session() + + assert auth_agent.authenticated_with == ["custom"] + + empty_model = create_acp_model( + acp_agent=cast(AcpAgent, NoHandshakeACPAgent()), + cwd="/workspace", + raise_on_empty_turn=True, + ) + with pytest.raises(UnexpectedModelBehavior, match="ACP agent ended its turn"): + await empty_model.request( + [ModelRequest(parts=[UserPromptPart("hello")])], + None, + ModelRequestParameters(), + ) + + async def test_acp_provider_default_model_leaves_remote_model_selection_to_agent() -> None: acp_agent = EchoACPAgent() set_session_model = AsyncMock( @@ -2162,3 +2195,514 @@ def test_acp_model_profile_disables_tool_and_structured_output_support() -> None assert profile["supports_json_object_output"] is False assert profile["supports_image_output"] is False assert profile["supported_native_tools"] == frozenset() + + +# --------------------------------------------------------------------------- +# Authentication recovery, explicit session bootstrap, and empty-turn handling +# --------------------------------------------------------------------------- + + +class AuthRequiredACPAgent(EchoACPAgent): + """An ACP agent that rejects ``session/new`` until ``authenticate`` runs.""" + + def __init__(self, *, auth_methods: list[TestAuthMethod], fail_times: int = 1) -> None: + super().__init__() + self._auth_methods = auth_methods + self._fail_times = fail_times + self.authenticated_with: list[str] = [] + self.new_session_calls = 0 + + async def initialize( + self, + protocol_version: int, + client_capabilities: ClientCapabilities | None = None, + client_info: Implementation | None = None, + **kwargs: Any, + ) -> InitializeResponse: + del client_capabilities, client_info, kwargs + self.initialized_protocols.append(protocol_version) + return InitializeResponse( + protocol_version=protocol_version, + agent_info=Implementation(name="auth-acp-agent", version="test"), + agent_capabilities=AgentCapabilities(), + auth_methods=self._auth_methods, + ) + + async def new_session( + self, + cwd: str, + mcp_servers: list[Any] | None = None, + **kwargs: Any, + ) -> NewSessionResponse: + self.new_session_calls += 1 + if self.new_session_calls <= self._fail_times: + raise RequestError.auth_required() + return await super().new_session(cwd, mcp_servers, **kwargs) + + async def authenticate(self, method_id: str, **kwargs: Any) -> None: + del kwargs + self.authenticated_with.append(method_id) + + +async def test_acp_provider_authenticates_and_retries_session_new_on_auth_required() -> None: + agent = AuthRequiredACPAgent( + auth_methods=[AuthMethodAgent(id="login", name="Login", description="Log in")], + ) + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + session_id = await provider.ensure_session() + + assert session_id == "session-1" + assert agent.new_session_calls == 2 + assert agent.authenticated_with == ["login"] + + +async def test_acp_provider_uses_explicit_auth_method_id_when_configured() -> None: + agent = AuthRequiredACPAgent( + auth_methods=[AuthMethodAgent(id="login", name="Login", description="Log in")], + ) + provider = AcpProvider( + acp_agent=cast(AcpAgent, agent), cwd="/workspace", auth_method_id="custom" + ) + + await provider.ensure_session() + + assert agent.authenticated_with == ["custom"] + + +async def test_acp_provider_auth_required_without_any_method_raises_userror() -> None: + agent = AuthRequiredACPAgent(auth_methods=[]) + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + with pytest.raises(UserError, match="authentication"): + await provider.ensure_session() + + +async def test_acp_provider_auth_required_without_authenticate_support_raises_userror() -> None: + class NoAuthenticateACPAgent(EchoACPAgent): + async def new_session( + self, + cwd: str, + mcp_servers: list[Any] | None = None, + **kwargs: Any, + ) -> NewSessionResponse: + del cwd, mcp_servers, kwargs + raise RequestError.auth_required() + + provider = AcpProvider(acp_agent=cast(AcpAgent, NoAuthenticateACPAgent()), cwd="/workspace") + + with pytest.raises(UserError, match="does not expose an 'authenticate' method"): + await provider.ensure_session() + + +async def test_acp_provider_ensure_session_bootstraps_without_a_prompt() -> None: + agent = EchoACPAgent() + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + session_id = await provider.ensure_session() + + assert session_id == "session-1" + assert provider.session_id == "session-1" + assert agent.initialized_protocols == [PROTOCOL_VERSION] + assert agent.prompts == [] + + +async def test_acp_provider_set_session_mode_delegates_to_agent() -> None: + mode_calls: list[tuple[str, str]] = [] + + class ModeACPAgent(EchoACPAgent): + async def set_session_mode( + self, + session_id: str, + mode_id: str, + **kwargs: Any, + ) -> None: + del kwargs + mode_calls.append((session_id, mode_id)) + + provider = AcpProvider(acp_agent=cast(AcpAgent, ModeACPAgent()), cwd="/workspace") + + await provider.set_session_mode("default") + + assert mode_calls == [("session-1", "default")] + + +async def test_acp_provider_set_session_mode_without_support_raises_userror() -> None: + provider = AcpProvider(acp_agent=cast(AcpAgent, EchoACPAgent()), cwd="/workspace") + + with pytest.raises(UserError, match="set_session_mode"): + await provider.set_session_mode("default") + + +async def test_empty_turn_returns_empty_parts_by_default() -> None: + provider = AcpProvider(acp_agent=cast(AcpAgent, NoHandshakeACPAgent()), cwd="/workspace") + model = provider.model() + + response = await model.request( + [ModelRequest(parts=[UserPromptPart("hello")])], + None, + ModelRequestParameters(), + ) + + assert response.parts == [] + + +async def test_empty_turn_raises_acp_specific_error_when_opted_in() -> None: + provider = AcpProvider( + acp_agent=cast(AcpAgent, NoHandshakeACPAgent()), + cwd="/workspace", + raise_on_empty_turn=True, + ) + model = provider.model() + + with pytest.raises(UnexpectedModelBehavior, match="ACP agent ended its turn"): + await model.request( + [ModelRequest(parts=[UserPromptPart("hello")])], + None, + ModelRequestParameters(), + ) + + +# --------------------------------------------------------------------------- +# Additional coverage: auth edge cases, session bootstrap, and empty turns +# --------------------------------------------------------------------------- + + +async def test_acp_provider_does_not_retry_authenticate_after_it_already_ran() -> None: + """A second ``auth_required`` after authenticating must propagate, not loop.""" + agent = AuthRequiredACPAgent( + auth_methods=[AuthMethodAgent(id="login", name="Login", description="Log in")], + fail_times=2, + ) + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + with pytest.raises(RequestError) as exc_info: + await provider.ensure_session() + + assert exc_info.value.code == RequestError.auth_required().code + assert agent.new_session_calls == 2 + assert agent.authenticated_with == ["login"] + + +async def test_acp_provider_non_auth_request_error_propagates_without_authenticating() -> None: + class OtherErrorACPAgent(EchoACPAgent): + async def new_session( + self, + cwd: str, + mcp_servers: list[Any] | None = None, + **kwargs: Any, + ) -> NewSessionResponse: + del cwd, mcp_servers, kwargs + raise RequestError.internal_error() + + provider = AcpProvider(acp_agent=cast(AcpAgent, OtherErrorACPAgent()), cwd="/workspace") + + with pytest.raises(RequestError) as exc_info: + await provider.ensure_session() + + assert exc_info.value.code == RequestError.internal_error().code + + +async def test_acp_provider_uses_first_advertised_auth_method_when_multiple_exist() -> None: + agent = AuthRequiredACPAgent( + auth_methods=[ + AuthMethodAgent(id="oauth", name="OAuth", description="OAuth login"), + AuthMethodAgent(id="apikey", name="API Key", description="API key login"), + ], + ) + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + await provider.ensure_session() + + assert agent.authenticated_with == ["oauth"] + + +async def test_acp_provider_automatically_uses_only_agent_managed_auth_methods() -> None: + agent = AuthRequiredACPAgent( + auth_methods=[ + EnvVarAuthMethod( + id="api-key", + name="API key", + type="env_var", + vars=[AuthEnvVar(name="API_KEY")], + ), + AuthMethodAgent(id="oauth", name="OAuth", description="Browser login"), + ], + ) + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + await provider.ensure_session() + + assert agent.authenticated_with == ["oauth"] + + +async def test_acp_provider_supports_synchronous_authenticate() -> None: + # A *synchronous* authenticate() exercises the non-awaitable branch of + # AcpProvider._authenticate. It subclasses EchoACPAgent (which has no + # authenticate) rather than AuthRequiredACPAgent, so the sync method is a + # fresh definition, not an invalid override of an async one. + class SyncAuthenticateACPAgent(EchoACPAgent): + def __init__(self) -> None: + super().__init__() + self.authenticated_with: list[str] = [] + self.new_session_calls = 0 + + async def initialize( + self, + protocol_version: int, + client_capabilities: ClientCapabilities | None = None, + client_info: Implementation | None = None, + **kwargs: Any, + ) -> InitializeResponse: + del client_capabilities, client_info, kwargs + self.initialized_protocols.append(protocol_version) + return InitializeResponse( + protocol_version=protocol_version, + agent_info=Implementation(name="sync-auth-agent", version="test"), + agent_capabilities=AgentCapabilities(), + auth_methods=[AuthMethodAgent(id="login", name="Login", description="Log in")], + ) + + async def new_session( + self, + cwd: str, + mcp_servers: list[Any] | None = None, + **kwargs: Any, + ) -> NewSessionResponse: + self.new_session_calls += 1 + if self.new_session_calls == 1: + raise RequestError.auth_required() + return await super().new_session(cwd, mcp_servers, **kwargs) + + def authenticate(self, method_id: str, **kwargs: Any) -> None: + del kwargs + self.authenticated_with.append(method_id) + + agent = SyncAuthenticateACPAgent() + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + session_id = await provider.ensure_session() + + assert session_id == "session-1" + assert agent.new_session_calls == 2 + assert agent.authenticated_with == ["login"] + + +async def test_acp_provider_auth_required_without_any_method_error_omits_advertised_clause() -> ( + None +): + agent = AuthRequiredACPAgent(auth_methods=[]) + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + with pytest.raises(UserError) as exc_info: + await provider.ensure_session() + + assert "(advertised:" not in str(exc_info.value) + + +async def test_acp_provider_requires_client_setup_for_env_var_auth_method() -> None: + agent = AuthRequiredACPAgent( + auth_methods=[ + EnvVarAuthMethod( + id="api-key", + name="API key", + type="env_var", + vars=[AuthEnvVar(name="API_KEY")], + ) + ], + ) + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + with pytest.raises(UserError, match="require client-side setup"): + await provider.ensure_session() + + assert agent.authenticated_with == [] + + +async def test_acp_provider_ensure_session_forwards_model_name() -> None: + agent = EchoACPAgent() + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + + await provider.ensure_session(model_name="agent") + + assert agent.session_models == [("session-1", "agent")] + + +async def test_acp_provider_set_session_mode_supports_synchronous_agent_method() -> None: + mode_calls: list[tuple[str, str]] = [] + + class SyncModeACPAgent(EchoACPAgent): + def set_session_mode(self, session_id: str, mode_id: str, **kwargs: Any) -> None: + del kwargs + mode_calls.append((session_id, mode_id)) + + provider = AcpProvider(acp_agent=cast(AcpAgent, SyncModeACPAgent()), cwd="/workspace") + + await provider.set_session_mode("default") + + assert mode_calls == [("session-1", "default")] + + +async def test_acp_provider_set_session_mode_reuses_session_created_by_prior_prompt() -> None: + class ModeACPAgent(EchoACPAgent): + def __init__(self) -> None: + super().__init__() + self.mode_calls: list[tuple[str, str]] = [] + + async def set_session_mode( + self, + session_id: str, + mode_id: str, + **kwargs: Any, + ) -> None: + del kwargs + self.mode_calls.append((session_id, mode_id)) + + agent = ModeACPAgent() + provider = AcpProvider(acp_agent=cast(AcpAgent, agent), cwd="/workspace") + model = provider.model() + + await model.request( + [ModelRequest(parts=[UserPromptPart("hello")])], + None, + ModelRequestParameters(), + ) + await provider.set_session_mode("default") + + assert agent.mode_calls == [(provider.session_id, "default")] + assert agent.session_cwds == ["/workspace"] + + +def test_acp_provider_raise_on_empty_turn_property_reflects_constructor_arg() -> None: + default_provider = AcpProvider(acp_agent=cast(AcpAgent, EchoACPAgent()), cwd="/workspace") + assert default_provider.raise_on_empty_turn is False + + opted_in_provider = AcpProvider( + acp_agent=cast(AcpAgent, EchoACPAgent()), + cwd="/workspace", + raise_on_empty_turn=True, + ) + assert opted_in_provider.raise_on_empty_turn is True + + +async def test_empty_turn_raises_with_stop_reason_in_message_when_available() -> None: + class RefusalACPAgent(NoHandshakeACPAgent): + async def prompt( + self, + prompt: list[Any], + session_id: str, + message_id: str | None = None, + **kwargs: Any, + ) -> PromptResponse: + del prompt, session_id, message_id, kwargs + return PromptResponse(stop_reason="refusal") + + provider = AcpProvider( + acp_agent=cast(AcpAgent, RefusalACPAgent()), + cwd="/workspace", + raise_on_empty_turn=True, + ) + model = provider.model() + + with pytest.raises(UnexpectedModelBehavior, match=r"stop reason: refusal"): + await model.request( + [ModelRequest(parts=[UserPromptPart("hello")])], + None, + ModelRequestParameters(), + ) + + +# --------------------------------------------------------------------------- +# Agent error propagation (anyio TaskGroup unwrapping) +# --------------------------------------------------------------------------- + + +def test_unwrap_acp_error_peels_single_child_group() -> None: + inner = RequestError(-32000, "Rate limited") + group = ExceptionGroup("unhandled errors in a TaskGroup", [inner]) + assert client_module._unwrap_acp_error(group) is inner + + +def test_unwrap_acp_error_peels_nested_single_child_groups() -> None: + inner = RequestError(-32000, "Rate limited") + nested = ExceptionGroup( + "unhandled errors in a TaskGroup", + [ExceptionGroup("unhandled errors in a TaskGroup", [inner])], + ) + assert client_module._unwrap_acp_error(nested) is inner + + +def test_unwrap_acp_error_drops_taskgroup_context_noise() -> None: + err = RuntimeError("boom") + err.__context__ = ExceptionGroup("unhandled errors in a TaskGroup", [ValueError("x")]) + cleaned = client_module._unwrap_acp_error(err) + assert cleaned is err + assert cleaned.__context__ is None + assert cleaned.__suppress_context__ is True + + +def test_unwrap_acp_error_leaves_plain_exception_untouched() -> None: + plain = ValueError("z") + assert client_module._unwrap_acp_error(plain) is plain + + +def test_unwrap_acp_error_keeps_multi_child_group() -> None: + group = ExceptionGroup("many", [ValueError("a"), RequestError(-32000, "b")]) + assert client_module._unwrap_acp_error(group) is group + + +def test_unwrap_acp_error_preserves_unrelated_single_child_group_and_context() -> None: + context = ExceptionGroup("validation failures", [ValueError("context")]) + inner = RuntimeError("boom") + inner.__context__ = context + group = ExceptionGroup("validation failures", [inner]) + + assert client_module._unwrap_acp_error(group) is group + assert inner.__context__ is context + + +async def test_request_prompt_unwraps_taskgroup_error_from_the_agent() -> None: + class GroupErrorACPAgent(EchoACPAgent): + async def prompt( + self, + prompt: list[Any], + session_id: str, + message_id: str | None = None, + **kwargs: Any, + ) -> PromptResponse: + del prompt, session_id, message_id, kwargs + raise ExceptionGroup( + "unhandled errors in a TaskGroup", + [RequestError(-32000, "Rate limited")], + ) + + provider = AcpProvider(acp_agent=cast(AcpAgent, GroupErrorACPAgent()), cwd="/workspace") + + with pytest.raises(RequestError, match="Rate limited"): + await provider.request_prompt( + model_name=None, + prompt=[text_block("hi")], + model_request_parameters=ModelRequestParameters(), + ) + + +async def test_request_prompt_reraises_cancellederror_untouched() -> None: + class CancellingACPAgent(EchoACPAgent): + async def prompt( + self, + prompt: list[Any], + session_id: str, + message_id: str | None = None, + **kwargs: Any, + ) -> PromptResponse: + del prompt, session_id, message_id, kwargs + raise asyncio.CancelledError + + provider = AcpProvider(acp_agent=cast(AcpAgent, CancellingACPAgent()), cwd="/workspace") + + with pytest.raises(asyncio.CancelledError): + await provider.request_prompt( + model_name=None, + prompt=[text_block("hi")], + model_request_parameters=ModelRequestParameters(), + )