diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 43bda63e8c..4818f9652e 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -56,6 +56,7 @@ from openhands.sdk.llm import LLM, Message, MessageToolCall, TextContent from openhands.sdk.logger import get_logger from openhands.sdk.observability.laminar import maybe_init_laminar, observe +from openhands.sdk.secret import SecretSource from openhands.sdk.tool import Tool # noqa: TC002 from openhands.sdk.tool.builtins.finish import FinishAction, FinishObservation @@ -885,6 +886,20 @@ def _start_acp_server(self, state: ConversationState) -> None: env = default_environment() env.update(os.environ) env.update(self.acp_env) + # Inject secrets from agent_context. acp_env entries take precedence + # (already set above), so we only fill keys not already present. + # SecretSource.get_value() is synchronous; calling it here is safe + # because _start_acp_server is a regular (non-async) method. + if self.agent_context and self.agent_context.secrets: + for name, secret in self.agent_context.secrets.items(): + if name not in env: + value = ( + secret.get_value() + if isinstance(secret, SecretSource) + else str(secret) + ) + if value: + env[name] = value # Strip CLAUDECODE so nested Claude Code instances don't refuse to start env.pop("CLAUDECODE", None) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 6da4f65c95..967513b43a 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -95,7 +95,7 @@ class AgentContext(BaseModel): "Values can be either strings or SecretSource instances " "(str | SecretSource)." ), - json_schema_extra={"acp_compatible": False}, + json_schema_extra={"acp_compatible": True}, ) current_datetime: datetime | str | None = Field( default_factory=datetime.now, @@ -316,20 +316,24 @@ def to_acp_prompt_context(self) -> str | None: ACP servers own their tools, MCP servers, hooks, and execution model, so this adapter only emits prompt-only context. Unsupported AgentContext - semantics (e.g. secrets) are rejected by - :meth:`validate_acp_compatibility`. + fields are rejected by :meth:`validate_acp_compatibility`. The rendering reuses :meth:`get_system_message_suffix` with the same ``system_message_suffix.j2`` template so that ACP agents receive the - identical prompt layout as the general agent (minus secrets). + identical prompt layout as the regular agent. This includes the + ```` block when secrets are present, informing the ACP + subprocess which environment variables are available. The actual secret + values are injected into the subprocess environment by + ``ACPAgent._start_acp_server``; the prompt block only advertises their + names so the agent knows to use them. ``user_message_suffix`` is a compatible field but is not emitted here because ``LocalConversation`` already applies it through ``event.to_llm_message()``; including it would duplicate it. """ self.validate_acp_compatibility() - # ACP doesn't support secrets (enforced above) and has no model-specific - # skill filtering, so we delegate to the shared renderer with no extras. + # No model-specific skill filtering for ACP — delegate to the shared + # renderer which also renders the block from secrets. return self.get_system_message_suffix() def get_user_message_suffix( diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index d63170bd78..c9d4d27be5 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -211,13 +211,15 @@ def test_allows_agent_context_for_prompt_extensions(self, tmp_path): self._init_with_patches(agent, tmp_path) - def test_rejects_unsupported_agent_context_at_init(self, tmp_path): + def test_allows_agent_context_with_secrets(self, tmp_path): + """Secrets are now ACP-compatible: they are injected into the subprocess + env by _start_acp_server and advertised in the prompt via .""" agent = ACPAgent( acp_command=["echo"], agent_context=AgentContext(secrets={"GITHUB_TOKEN": "ghp_secret"}), ) - with pytest.raises(NotImplementedError, match="secrets"): - self._init_with_patches(agent, tmp_path) + # Should not raise + self._init_with_patches(agent, tmp_path) def test_agent_context_to_acp_prompt_context(self): context = AgentContext( @@ -261,11 +263,31 @@ def test_agent_context_to_acp_prompt_context_emits_datetime_by_default(self): assert prompt is not None assert "" in prompt - def test_agent_context_to_acp_prompt_context_rejects_secrets(self): - context = AgentContext(secrets={"GITHUB_TOKEN": "ghp_secret"}) + def test_agent_context_to_acp_prompt_context_includes_secrets(self): + """Secrets appear in the ACP prompt as a block so the + ACP subprocess knows which environment variables are available.""" + from pydantic import SecretStr + + from openhands.sdk.secret import StaticSecret + + context = AgentContext( + secrets={ + "GITHUB_TOKEN": StaticSecret( + value=SecretStr("ghp_secret"), + description="GitHub authentication token", + ), + "MY_API_KEY": StaticSecret(value=SecretStr("key123")), + }, + current_datetime=None, + ) - with pytest.raises(NotImplementedError, match="secrets"): - context.to_acp_prompt_context() + prompt = context.to_acp_prompt_context() + + assert prompt is not None + assert "" in prompt + assert "$GITHUB_TOKEN" in prompt + assert "GitHub authentication token" in prompt + assert "$MY_API_KEY" in prompt def test_agent_context_to_acp_prompt_context_includes_legacy_repo_skills(self): context = AgentContext( @@ -3291,3 +3313,144 @@ def test_roundtrip_via_conversation_state_persistence(self, tmp_path): assert kwargs["cwd"] == str(workspace) conn2.new_session.assert_not_awaited() assert agent2._session_id == "roundtrip-sess" + + +class TestACPSecretsEnvInjection: + """Tests for secret injection into the ACP subprocess environment. + + Secrets passed via ``agent_context.secrets`` must land in the subprocess + env so the ACP server (Claude Code, Codex CLI, etc.) can use them. + ``acp_env`` entries take precedence over agent_context secrets. + """ + + @staticmethod + def _make_conn(): + conn = MagicMock() + init_response = MagicMock() + init_response.agent_info = MagicMock() + init_response.agent_info.name = "claude-agent-acp" + init_response.agent_info.version = "1.0" + init_response.auth_methods = [] + conn.initialize = AsyncMock(return_value=init_response) + new_response = MagicMock() + new_response.session_id = "sess-1" + conn.new_session = AsyncMock(return_value=new_response) + conn.load_session = AsyncMock(return_value=MagicMock()) + conn.set_session_mode = AsyncMock() + conn.set_session_model = AsyncMock() + conn.authenticate = AsyncMock() + conn.close = AsyncMock() + return conn + + @staticmethod + def _run_start_capturing_env(agent, tmp_path) -> dict: + """Run _start_acp_server and return the env dict passed to the subprocess.""" + from contextlib import ExitStack + + from openhands.sdk.utils.async_executor import AsyncExecutor + + captured: dict = {} + conn = TestACPSecretsEnvInjection._make_conn() + + mock_process = MagicMock() + mock_process.stdin = MagicMock() + mock_process.stdout = MagicMock() + + async def _fake_create_subprocess_exec(*_args, env=None, **_kwargs): + captured.update(env or {}) + return mock_process + + async def _fake_filter(_src, _dst): + return None + + state = _make_state(tmp_path) + agent._executor = AsyncExecutor() + + with ExitStack() as stack: + stack.enter_context( + patch( + "openhands.sdk.agent.acp_agent.asyncio.create_subprocess_exec", + new=_fake_create_subprocess_exec, + ) + ) + stack.enter_context( + patch( + "openhands.sdk.agent.acp_agent.ClientSideConnection", + return_value=conn, + ) + ) + stack.enter_context( + patch( + "openhands.sdk.agent.acp_agent._filter_jsonrpc_lines", + new=_fake_filter, + ) + ) + stack.enter_context( + patch( + "openhands.sdk.agent.acp_agent.asyncio.StreamReader", + return_value=MagicMock(), + ) + ) + agent._start_acp_server(state) + + return captured + + def test_static_secret_injected_into_subprocess_env(self, tmp_path): + """A StaticSecret in agent_context.secrets lands in the subprocess env.""" + from pydantic import SecretStr + + from openhands.sdk.secret import StaticSecret + + agent = _make_agent( + agent_context=AgentContext( + secrets={ + "GITHUB_TOKEN": StaticSecret( + value=SecretStr("ghp_test123"), + description="GitHub token", + ) + } + ) + ) + env = self._run_start_capturing_env(agent, tmp_path) + assert env.get("GITHUB_TOKEN") == "ghp_test123" + + def test_acp_env_takes_precedence_over_agent_context_secret(self, tmp_path): + """An explicit acp_env entry wins over the same key in agent_context.secrets.""" + from pydantic import SecretStr + + from openhands.sdk.secret import StaticSecret + + agent = _make_agent( + acp_env={"MY_TOKEN": "acp-env-wins"}, + agent_context=AgentContext( + secrets={"MY_TOKEN": StaticSecret(value=SecretStr("secret-panel"))} + ), + ) + env = self._run_start_capturing_env(agent, tmp_path) + assert env.get("MY_TOKEN") == "acp-env-wins" + + def test_none_value_secret_not_injected(self, tmp_path): + """A StaticSecret with value=None is not added to the subprocess env.""" + from openhands.sdk.secret import StaticSecret + + agent = _make_agent( + agent_context=AgentContext( + secrets={"ABSENT_SECRET": StaticSecret(value=None)} + ) + ) + env = self._run_start_capturing_env(agent, tmp_path) + assert "ABSENT_SECRET" not in env + + def test_empty_string_secret_not_injected(self, tmp_path): + """Empty string secrets are not injected into the subprocess env.""" + from pydantic import SecretStr + + from openhands.sdk.secret import StaticSecret + + agent = _make_agent( + agent_context=AgentContext( + secrets={"EMPTY_SECRET": StaticSecret(value=SecretStr(""))} + ) + ) + env = self._run_start_capturing_env(agent, tmp_path) + assert "EMPTY_SECRET" not in env