Address PR review feedback

- Rename _make_sentinel_llm → _make_dummy_llm
- Move inline acp imports to top-level
- Add GitHub issue link to TODO comment
- Simplify _filter_jsonrpc_lines exception handling
- Add debug logging for on_token and stats callback failures
- Remove chatty Example from class docstring
- Remove critic validation check (ACP server manages its own evaluation)
- Add comment explaining lazy import in __init__.py (avoids circular dependency)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: simonrosenberg

openhands-sdk/openhands/sdk/agent/__init__.py

@@ -1,25 +1,12 @@
-from __future__ import annotations
-
-from typing import TYPE_CHECKING
-
-from openhands.sdk.agent.agent import Agent
+# Import order matters: Agent must be imported before ACPAgent to avoid
+# a circular import through tool/builtins/finish.py.
+from openhands.sdk.agent.agent import Agent  # noqa: I001
 from openhands.sdk.agent.base import AgentBase
-
-
-if TYPE_CHECKING:
-    from openhands.sdk.agent.acp_agent import ACPAgent
-
-
-def __getattr__(name: str):
-    if name == "ACPAgent":
-        from openhands.sdk.agent.acp_agent import ACPAgent
-
-        return ACPAgent
-    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
+from openhands.sdk.agent.acp_agent import ACPAgent
 
 
 __all__ = [
+    "ACPAgent",
     "Agent",
     "AgentBase",
-    "ACPAgent",
 ]

openhands-sdk/openhands/sdk/agent/acp_agent.py

@@ -10,10 +10,24 @@
 
 from __future__ import annotations
 
+import asyncio
 import os
 from collections.abc import Generator
 from typing import TYPE_CHECKING, Any
 
+from acp.client.connection import ClientSideConnection
+from acp.helpers import text_block
+from acp.schema import (
+    AgentMessageChunk,
+    AgentThoughtChunk,
+    AllowedOutcome,
+    RequestPermissionResponse,
+    TextContentBlock,
+    ToolCallProgress,
+    ToolCallStart,
+    UsageUpdate,
+)
+from acp.transports import default_environment
 from pydantic import Field, PrivateAttr
 
 from openhands.sdk.agent.base import AgentBase
@@ -41,15 +55,16 @@
 # so we yield to the event loop and then sleep briefly to allow in-flight
 # handlers to finish.  Override via ACP_NOTIFICATION_DRAIN_DELAY for slow or
 # remote servers.
-# TODO: Replace with protocol-level synchronization once ACP supports a
-#       "turn complete" sentinel notification.
+# TODO(https://github.com/agentclientprotocol/agent-client-protocol/issues/554):
+#       Replace with protocol-level synchronization once ACP supports a
+#       "turn complete" notification.
 _NOTIFICATION_DRAIN_DELAY: float = float(
     os.environ.get("ACP_NOTIFICATION_DRAIN_DELAY", "0.1")
 )
 
 
-def _make_sentinel_llm() -> LLM:
-    """Create a sentinel LLM that should never be called."""
+def _make_dummy_llm() -> LLM:
+    """Create a dummy LLM that should never be called directly."""
     return LLM(model="acp-managed")
 
 
@@ -78,15 +93,12 @@ async def _filter_jsonrpc_lines(source: Any, dest: Any) -> None:
             if stripped.startswith(b"{") and b'"jsonrpc"' in line:
                 dest.feed_data(line)
             else:
-                # Log non-JSON lines at debug level
-                try:
-                    logger.debug(
-                        "ACP stdout (non-JSON): %s",
-                        line.decode(errors="replace").rstrip(),
-                    )
-                except Exception:
-                    pass
+                logger.debug(
+                    "ACP stdout (non-JSON): %s",
+                    line.decode(errors="replace").rstrip(),
+                )
     except Exception:
+        logger.debug("_filter_jsonrpc_lines stopped", exc_info=True)
         dest.feed_eof()
 
 
@@ -120,15 +132,6 @@ async def session_update(
         update: Any,
         **kwargs: Any,  # noqa: ARG002
     ) -> None:
-        from acp.schema import (
-            AgentMessageChunk,
-            AgentThoughtChunk,
-            TextContentBlock,
-            ToolCallProgress,
-            ToolCallStart,
-            UsageUpdate,
-        )
-
         if isinstance(update, AgentMessageChunk):
             if isinstance(update.content, TextContentBlock):
                 text = update.content.text
@@ -137,7 +140,7 @@ async def session_update(
                     try:
                         self.on_token(text)
                     except Exception:
-                        pass
+                        logger.debug("on_token callback failed", exc_info=True)
         elif isinstance(update, AgentThoughtChunk):
             if isinstance(update.content, TextContentBlock):
                 self.accumulated_thoughts.append(update.content.text)
@@ -163,11 +166,6 @@ async def request_permission(
         **kwargs: Any,  # noqa: ARG002
     ) -> Any:
         """Auto-approve all permission requests from the ACP server."""
-        from acp.schema import (
-            AllowedOutcome,
-            RequestPermissionResponse,
-        )
-
         # Pick the first option (usually "allow once")
         option_id = options[0].option_id if options else "allow_once"
         logger.info(
@@ -260,7 +258,7 @@ class ACPAgent(AgentBase):
     """
 
     # Override required fields with ACP-appropriate defaults
-    llm: LLM = Field(default_factory=_make_sentinel_llm)
+    llm: LLM = Field(default_factory=_make_dummy_llm)
     tools: list[Tool] = Field(default_factory=list)
     include_default_tools: list[str] = Field(default_factory=list)
 
@@ -322,10 +320,6 @@ def init_state(
                 "ACPAgent does not support condenser; "
                 "the ACP server manages its own context"
             )
-        if self.critic is not None:
-            raise NotImplementedError(
-                "ACPAgent does not support critic; "
-            )
         if self.agent_context is not None:
             raise NotImplementedError(
                 "ACPAgent does not support agent_context; "
@@ -354,15 +348,6 @@ def init_state(
 
     def _start_acp_server(self, state: ConversationState) -> None:
         """Start the ACP subprocess and initialize the session."""
-        import asyncio
-
-        from acp.client.connection import (
-            ClientSideConnection,
-        )
-        from acp.transports import (
-            default_environment,
-        )
-
         client = _OpenHandsACPClient()
         client._llm_ref = self.llm
         self._client = client
@@ -449,9 +434,6 @@ def step(
         self._client.on_token = on_token
 
         try:
-            import asyncio
-
-            from acp.helpers import text_block
 
             async def _prompt() -> Any:
                 response = await self._conn.prompt(
@@ -490,7 +472,7 @@ async def _prompt() -> Any:
                 try:
                     self.llm.telemetry._stats_update_callback()
                 except Exception:
-                    pass
+                    logger.debug("Stats update callback failed", exc_info=True)
 
             # Build response message
             response_text = "".join(self._client.accumulated_text)