From 835fc0faedb18be1415d4a539935c56afc634c05 Mon Sep 17 00:00:00 2001 From: Shimada666 <649940882@qq.com> Date: Fri, 7 Aug 2026 18:33:32 +0800 Subject: [PATCH 1/4] fix(subagent): decrypt encrypted model profiles Co-authored-by: openhands --- .../agent_server/conversation_service.py | 5 +++- .../conversation/impl/local_conversation.py | 4 ++- .../openhands/sdk/subagent/registry.py | 26 ++++++++++++++++--- tests/sdk/subagent/test_subagent_registry.py | 23 ++++++++++++++++ 4 files changed, 52 insertions(+), 6 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index 4f9abac659..b68b395bb9 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -514,6 +514,7 @@ def _register_agent_definitions( agent_defs: list["AgentDefinition"], *, context: str, + cipher: Cipher | None, ) -> None: """Register agent definitions into the subagent registry. @@ -528,7 +529,7 @@ def _register_agent_definitions( registered = 0 for agent_def in agent_defs: try: - factory = agent_definition_to_factory(agent_def) + factory = agent_definition_to_factory(agent_def, cipher=cipher) register_agent_if_absent( name=agent_def.name, factory_func=factory, @@ -1054,6 +1055,7 @@ def _prepare_persisted_runtime(self, stored: StoredConversation) -> None: _register_agent_definitions( stored.agent_definitions, context=f"resuming conversation {stored.id}", + cipher=self.cipher, ) def _get_conversation_lock(self, conversation_id: UUID) -> asyncio.Lock: @@ -1615,6 +1617,7 @@ async def _start_conversation( _register_agent_definitions( request.agent_definitions, context=f"conversation {conversation_id}", + cipher=self.cipher, ) # Plugin loading is now handled lazily by LocalConversation. diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index c5146c19d3..d4ea272a98 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -1214,6 +1214,7 @@ def _ensure_plugins_loaded(self) -> None: register_plugin_agents( agents=all_plugin_agents, work_dir=self.workspace.working_dir, + cipher=self._cipher, ) # Combine explicit hook_config with plugin hooks @@ -1459,6 +1460,7 @@ def load_plugin(self, plugin_ref: str) -> None: register_plugin_agents( agents=plugin.agents, work_dir=self.workspace.working_dir, + cipher=self._cipher, ) if plugin.hooks and not plugin.hooks.is_empty(): self._merge_runtime_plugin_hooks(plugin.hooks) @@ -1497,7 +1499,7 @@ def _register_file_based_agents(self) -> None: then `~/.openhands/agents/*.md`) """ # register project-level and then user-level file-based agents - register_file_agents(self.workspace.working_dir) + register_file_agents(self.workspace.working_dir, cipher=self._cipher) def _ensure_agent_ready(self) -> None: """Ensure the agent is fully initialized with plugins and agents loaded. diff --git a/openhands-sdk/openhands/sdk/subagent/registry.py b/openhands-sdk/openhands/sdk/subagent/registry.py index 6350877352..2d3f12ae93 100644 --- a/openhands-sdk/openhands/sdk/subagent/registry.py +++ b/openhands-sdk/openhands/sdk/subagent/registry.py @@ -42,6 +42,7 @@ def create_security_expert(llm): if TYPE_CHECKING: from openhands.sdk.agent.agent import Agent from openhands.sdk.llm.llm import LLM + from openhands.sdk.utils.cipher import Cipher logger = get_logger(__name__) @@ -160,6 +161,8 @@ def _get_profile_store(profile_store_dir: str | None) -> LLMProfileStore: def agent_definition_to_factory( agent_def: AgentDefinition, work_dir: str | Path | None = None, + *, + cipher: "Cipher | None" = None, ) -> Callable[["LLM"], "Agent"]: """Create an agent factory closure from an `AgentDefinition`. @@ -181,6 +184,7 @@ def agent_definition_to_factory( agent_def: The agent definition to convert. work_dir: Project directory for resolving skill names. If None, only user-level skills are searched. + cipher: Cipher for decrypting secrets in the selected LLM profile. Raises: ValueError: If a tool or skill is not found. @@ -225,7 +229,7 @@ def _factory(llm: "LLM") -> "Agent": f"Available profiles: {available_profiles}" ) - llm = store.load(profile_name) + llm = store.load(profile_name, cipher=cipher) # the system prompt of the subagent is added as a suffix of the # main system prompt @@ -285,7 +289,11 @@ def _factory(llm: "LLM") -> "Agent": return _factory -def register_file_agents(work_dir: str | Path) -> list[str]: +def register_file_agents( + work_dir: str | Path, + *, + cipher: "Cipher | None" = None, +) -> list[str]: """Load and register file-based agents from project-level `.agents/agents` and `.openhands/agents`, and user-level `~/.agents/agents` and `~/.openhands/agents` directories. @@ -317,7 +325,11 @@ def register_file_agents(work_dir: str | Path) -> list[str]: registered: list[str] = [] for agent_def in deduplicated: - factory = agent_definition_to_factory(agent_def, work_dir=work_dir) + factory = agent_definition_to_factory( + agent_def, + work_dir=work_dir, + cipher=cipher, + ) was_registered = register_agent_if_absent( name=agent_def.name, factory_func=factory, @@ -336,6 +348,8 @@ def register_file_agents(work_dir: str | Path) -> list[str]: def register_plugin_agents( agents: list[AgentDefinition], work_dir: str | Path | None = None, + *, + cipher: "Cipher | None" = None, ) -> list[str]: """Register plugin-provided agent definitions into the delegate registry. @@ -354,7 +368,11 @@ def register_plugin_agents( """ registered: list[str] = [] for agent_def in agents: - factory = agent_definition_to_factory(agent_def, work_dir=work_dir) + factory = agent_definition_to_factory( + agent_def, + work_dir=work_dir, + cipher=cipher, + ) was_registered = register_agent_if_absent( name=agent_def.name, factory_func=factory, diff --git a/tests/sdk/subagent/test_subagent_registry.py b/tests/sdk/subagent/test_subagent_registry.py index 8c4402bfac..51f770b308 100644 --- a/tests/sdk/subagent/test_subagent_registry.py +++ b/tests/sdk/subagent/test_subagent_registry.py @@ -21,6 +21,7 @@ register_plugin_agents, ) from openhands.sdk.subagent.schema import AgentDefinition +from openhands.sdk.utils.cipher import Cipher def setup_function() -> None: @@ -591,6 +592,28 @@ def test_agent_definition_to_factory_model_profile_custom_store(tmp_path: Path) assert agent.llm.metrics is not parent_llm.metrics +def test_agent_definition_to_factory_decrypts_model_profile(tmp_path: Path) -> None: + """Encrypted model profiles are decrypted for file-based sub-agents.""" + cipher = Cipher("test-secret") + store = LLMProfileStore(base_dir=tmp_path) + profile_llm = LLM( + model="gpt-4o-mini", + api_key=SecretStr("profile-key"), + usage_id="profile-llm", + ) + store.save("encrypted-profile", profile_llm, include_secrets=True, cipher=cipher) + agent_def = AgentDefinition( + name="encrypted-profile-agent", + model="encrypted-profile", + profile_store_dir=str(tmp_path), + ) + + agent = agent_definition_to_factory(agent_def, cipher=cipher)(_make_test_llm()) + + assert isinstance(agent.llm.api_key, SecretStr) + assert agent.llm.api_key.get_secret_value() == "profile-key" + + def test_agent_definition_to_factory_profile_store_dir(tmp_path: Path) -> None: """profile_store_dir on AgentDefinition is used by the factory.""" store = LLMProfileStore(base_dir=tmp_path) From ad61ede39f3f4d96252c63dab22f889115dfc851 Mon Sep 17 00:00:00 2001 From: Shimada666 <649940882@qq.com> Date: Fri, 28 Aug 2026 16:43:44 +0800 Subject: [PATCH 2/4] fix(subagent): propagate profile cipher through child runtimes Co-authored-by: openhands --- .../conversation/impl/local_conversation.py | 6 ++- .../openhands/sdk/llm/fallback_strategy.py | 11 +++- .../openhands/sdk/llm/llm_profile_store.py | 33 ++++++++++++ .../openhands/sdk/subagent/registry.py | 7 ++- .../openhands/tools/delegate/impl.py | 1 + .../openhands/tools/task/manager.py | 2 + tests/sdk/conversation/local/test_fork.py | 15 ++++++ tests/sdk/llm/test_llm_fallback.py | 32 +++++++++++ tests/sdk/llm/test_llm_profile_store.py | 36 +++++++++++++ tests/sdk/subagent/test_subagent_registry.py | 53 +++++++++++++++++++ tests/tools/delegate/test_delegation.py | 4 ++ tests/tools/task/test_task_manager.py | 19 +++++++ tests/tools/test_builtin_agents.py | 6 +++ 13 files changed, 221 insertions(+), 4 deletions(-) diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index d4ea272a98..be8282373e 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -406,6 +406,7 @@ def __init__( if recovered_specs: register_client_tools(recovered_specs) self.agent = agent + self._cipher = cipher self._bind_conversation_context(self.agent.llm) @@ -486,8 +487,6 @@ def _default_callback(e): # This ensures plugins are loaded before agent initialization self.llm_registry = LLMRegistry() self._profile_store = LLMProfileStore(profile_store_dir) - self._cipher = cipher - # Seed agent_context.secrets into the registry for every agent (regular # and ACP), covering callers that skip create_request() — canvas / # TypeScript, or the server-side agent_settings -> create_agent fold. @@ -835,6 +834,7 @@ def fork( stuck_detection=self._stuck_detector is not None, visualizer=type(self._visualizer) if self._visualizer else None, delete_on_close=self.delete_on_close, + cipher=self._cipher, tags=tags, ) @@ -1605,6 +1605,8 @@ def _bind_conversation_context(self, llm: LLM) -> None: See #3443 for background. """ llm._call_context = self.get_llm_call_context() + if llm.fallback_strategy is not None: + llm.fallback_strategy._bind_cipher(self._cipher) def _condenser_for_switched_llm( self, diff --git a/openhands-sdk/openhands/sdk/llm/fallback_strategy.py b/openhands-sdk/openhands/sdk/llm/fallback_strategy.py index e1793e767a..3c0806943b 100644 --- a/openhands-sdk/openhands/sdk/llm/fallback_strategy.py +++ b/openhands-sdk/openhands/sdk/llm/fallback_strategy.py @@ -22,6 +22,7 @@ if TYPE_CHECKING: from openhands.sdk.llm.llm_response import LLMResponse from openhands.sdk.llm.utils.metrics import Metrics + from openhands.sdk.utils.cipher import Cipher logger = get_logger(__name__) @@ -55,6 +56,11 @@ class FallbackStrategy(BaseModel): # Private: lazily resolved LLM instances _resolved: list[Any] | None = PrivateAttr(default=None) + _cipher: Cipher | None = PrivateAttr(default=None) + + def _bind_cipher(self, cipher: Cipher | None) -> None: + """Bind the conversation cipher used for lazy profile resolution.""" + self._cipher = cipher def should_fallback(self, error: Exception) -> bool: """Whether this error type is eligible for fallback.""" @@ -139,7 +145,10 @@ def _iter_fallbacks(self) -> Generator[Any]: remaining_names = self.fallback_llms[len(self._resolved) :] for name in remaining_names: try: - fb = self._profile_store.load(name) + fb = self._profile_store._load_for_execution( + name, + cipher=self._cipher, + ) self._resolved.append(fb) yield fb except (FileNotFoundError, ValueError) as exc: diff --git a/openhands-sdk/openhands/sdk/llm/llm_profile_store.py b/openhands-sdk/openhands/sdk/llm/llm_profile_store.py index 5001e4b9c4..3fc3b28edf 100644 --- a/openhands-sdk/openhands/sdk/llm/llm_profile_store.py +++ b/openhands-sdk/openhands/sdk/llm/llm_profile_store.py @@ -11,11 +11,13 @@ from typing import TYPE_CHECKING, Any, Final, Protocol, runtime_checkable from filelock import FileLock, Timeout +from pydantic import SecretStr from openhands.sdk.llm.utils.openhands_provider import ( canonicalize_openhands_llm_payload, ) from openhands.sdk.logger import get_logger +from openhands.sdk.utils.cipher import FERNET_TOKEN_PREFIX from openhands.sdk.utils.pydantic_secrets import REDACTED_SECRET_VALUE @@ -358,6 +360,37 @@ def _resolve_provider_connection( updates["api_key"] = SecretStr(api_key) return llm.model_copy(update=updates) + def _load_for_execution( + self, + name: str, + *, + cipher: Cipher | None = None, + ) -> LLM: + """Load a profile and reject an unavailable encrypted API key.""" + llm = self.load(name, cipher=cipher) + profile_name = name.removesuffix(".json") + stored_api_key = next( + ( + summary["api_key_set"] + for summary in self.list_summaries() + if summary["name"] == profile_name + ), + False, + ) + loaded_api_key = ( + llm.api_key.get_secret_value() + if isinstance(llm.api_key, SecretStr) + else llm.api_key + ) + if stored_api_key and ( + loaded_api_key is None or loaded_api_key.startswith(FERNET_TOKEN_PREFIX) + ): + raise ValueError( + f"Could not decrypt API key for profile '{profile_name}'. " + "Use the cipher that encrypted the profile." + ) + return llm + def delete(self, name: str) -> None: """Delete an existing profile. diff --git a/openhands-sdk/openhands/sdk/subagent/registry.py b/openhands-sdk/openhands/sdk/subagent/registry.py index 2d3f12ae93..bb65a21294 100644 --- a/openhands-sdk/openhands/sdk/subagent/registry.py +++ b/openhands-sdk/openhands/sdk/subagent/registry.py @@ -229,7 +229,7 @@ def _factory(llm: "LLM") -> "Agent": f"Available profiles: {available_profiles}" ) - llm = store.load(profile_name, cipher=cipher) + llm = store._load_for_execution(profile_name, cipher=cipher) # the system prompt of the subagent is added as a suffix of the # main system prompt @@ -303,6 +303,10 @@ def register_file_agents( Does not overwrite agents already registered programmatically or by plugins. + Args: + work_dir: Project directory used to discover agent definitions. + cipher: Cipher for decrypting secrets in selected LLM profiles. + Returns: List of agent names that were actually registered. """ @@ -362,6 +366,7 @@ def register_plugin_agents( agents: Agent definitions collected from loaded plugins. work_dir: Project directory for resolving skill names in agent definitions. If None, only user-level skills are searched. + cipher: Cipher for decrypting secrets in selected LLM profiles. Returns: List of agent names that were actually registered. diff --git a/openhands-tools/openhands/tools/delegate/impl.py b/openhands-tools/openhands/tools/delegate/impl.py index c72a77862f..db630d892c 100644 --- a/openhands-tools/openhands/tools/delegate/impl.py +++ b/openhands-tools/openhands/tools/delegate/impl.py @@ -220,6 +220,7 @@ def _spawn_agents(self, action: "DelegateAction") -> DelegateObservation: "visualizer": sub_visualizer, "hook_config": factory.definition.hooks, "persistence_dir": subagents_persistence_dir, + "cipher": parent_conversation._cipher, } if factory.definition.max_iteration_per_run is not None: diff --git a/openhands-tools/openhands/tools/task/manager.py b/openhands-tools/openhands/tools/task/manager.py index a8b52a1d60..697084af6f 100644 --- a/openhands-tools/openhands/tools/task/manager.py +++ b/openhands-tools/openhands/tools/task/manager.py @@ -220,6 +220,7 @@ def _resume_task(self, resume: str, subagent_type: str) -> Task: conversation_id=conversation_id, hook_config=factory.definition.hooks, delete_on_close=True, + cipher=self.parent_conversation._cipher, observability_metadata=self._delegate_observability_metadata( task_id=resume, subagent_type=subagent_type, link=link ), @@ -322,6 +323,7 @@ def _get_conversation( max_budget_per_run=max_budget_per_run, hook_config=hook_config, delete_on_close=True, + cipher=parent._cipher, prompt_cache_key=str(parent.state.id), observability_metadata=self._delegate_observability_metadata( task_id=task_id, subagent_type=subagent_type, link=link diff --git a/tests/sdk/conversation/local/test_fork.py b/tests/sdk/conversation/local/test_fork.py index 3057831077..a6016cdb2f 100644 --- a/tests/sdk/conversation/local/test_fork.py +++ b/tests/sdk/conversation/local/test_fork.py @@ -16,6 +16,7 @@ from openhands.sdk.event.llm_convertible import MessageEvent, SystemPromptEvent from openhands.sdk.llm import LLM, Message, TextContent from openhands.sdk.tool import Action, Observation, ToolDefinition, ToolExecutor +from openhands.sdk.utils.cipher import Cipher def _agent() -> Agent: @@ -80,6 +81,20 @@ def test_fork_creates_new_id(): assert isinstance(fork.id, uuid.UUID) +def test_fork_inherits_cipher(): + cipher = Cipher("test-secret") + with tempfile.TemporaryDirectory() as tmpdir: + src = LocalConversation( + agent=_agent(), + persistence_dir=tmpdir, + workspace=tmpdir, + cipher=cipher, + ) + fork = src.fork() + + assert fork._cipher is cipher + + def test_fork_with_explicit_id(): """Explicit conversation_id is honoured.""" custom_id = uuid.uuid4() diff --git a/tests/sdk/llm/test_llm_fallback.py b/tests/sdk/llm/test_llm_fallback.py index f48fb4b732..f142e37b4c 100644 --- a/tests/sdk/llm/test_llm_fallback.py +++ b/tests/sdk/llm/test_llm_fallback.py @@ -15,12 +15,16 @@ ) from pydantic import SecretStr +from openhands.sdk import Agent +from openhands.sdk.conversation.impl.local_conversation import LocalConversation from openhands.sdk.llm import LLM, FallbackStrategy, Message, TextContent from openhands.sdk.llm.exceptions import ( LLMContextWindowExceedError, LLMServiceUnavailableError, ) from openhands.sdk.llm.llm import LLMCallContext +from openhands.sdk.llm.llm_profile_store import LLMProfileStore +from openhands.sdk.utils.cipher import Cipher def _get_mock_response(content: str = "ok", model: str = "gpt-4o") -> ModelResponse: @@ -313,6 +317,34 @@ def side_effect(**kwargs): assert content.text == "from store" +def test_fallback_profile_uses_bound_cipher(tmp_path): + cipher = Cipher("test-secret") + store = LLMProfileStore(base_dir=tmp_path) + store.save( + "encrypted-fallback", + _get_llm("fallback-model"), + include_secrets=True, + cipher=cipher, + ) + strategy = FallbackStrategy( + fallback_llms=["encrypted-fallback"], + profile_store_dir=tmp_path, + ) + + conversation = LocalConversation( + agent=Agent(llm=_get_llm("primary", fallback_strategy=strategy), tools=[]), + workspace=tmp_path, + cipher=cipher, + visualizer=None, + ) + bound_strategy = conversation.agent.llm.fallback_strategy + assert bound_strategy is not None + [fallback] = list(bound_strategy._iter_fallbacks()) + + assert isinstance(fallback.api_key, SecretStr) + assert fallback.api_key.get_secret_value() == "k" + + # ========================================================================= # Async error-handling parity tests (acompletion / aresponses) # ========================================================================= diff --git a/tests/sdk/llm/test_llm_profile_store.py b/tests/sdk/llm/test_llm_profile_store.py index 6c429c165a..67dde898ad 100644 --- a/tests/sdk/llm/test_llm_profile_store.py +++ b/tests/sdk/llm/test_llm_profile_store.py @@ -12,6 +12,7 @@ LLMProfileStore, ProfileLimitExceeded, ) +from openhands.sdk.utils.cipher import Cipher @pytest.fixture @@ -334,6 +335,41 @@ def test_load_nonexistent_profile(profile_store: LLMProfileStore) -> None: assert "not found" in str(exc_info.value) +@pytest.mark.parametrize("cipher", [None, Cipher("wrong-secret")]) +def test_load_for_execution_rejects_undecrypted_api_key( + profile_store: LLMProfileStore, + sample_llm_with_secrets: LLM, + cipher: Cipher | None, +) -> None: + profile_store.save( + "encrypted-profile", + sample_llm_with_secrets, + include_secrets=True, + cipher=Cipher("right-secret"), + ) + + with pytest.raises(ValueError, match="encrypted-profile"): + profile_store._load_for_execution("encrypted-profile", cipher=cipher) + + +def test_load_for_execution_decrypts_api_key( + profile_store: LLMProfileStore, + sample_llm_with_secrets: LLM, +) -> None: + cipher = Cipher("right-secret") + profile_store.save( + "encrypted-profile", + sample_llm_with_secrets, + include_secrets=True, + cipher=cipher, + ) + + loaded = profile_store._load_for_execution("encrypted-profile", cipher=cipher) + + assert isinstance(loaded.api_key, SecretStr) + assert loaded.api_key.get_secret_value() == "secret-api-key-12345" + + def test_load_nonexistent_shows_available( profile_store: LLMProfileStore, sample_llm: LLM ) -> None: diff --git a/tests/sdk/subagent/test_subagent_registry.py b/tests/sdk/subagent/test_subagent_registry.py index 51f770b308..ae95339329 100644 --- a/tests/sdk/subagent/test_subagent_registry.py +++ b/tests/sdk/subagent/test_subagent_registry.py @@ -614,6 +614,59 @@ def test_agent_definition_to_factory_decrypts_model_profile(tmp_path: Path) -> N assert agent.llm.api_key.get_secret_value() == "profile-key" +def test_register_file_agents_forwards_cipher_to_factory(tmp_path: Path) -> None: + cipher = Cipher("test-secret") + profile_dir = tmp_path / "profiles" + store = LLMProfileStore(base_dir=profile_dir) + store.save( + "encrypted-profile", + LLM(model="gpt-4o-mini", api_key=SecretStr("profile-key")), + include_secrets=True, + cipher=cipher, + ) + agents_dir = tmp_path / ".agents" / "agents" + agents_dir.mkdir(parents=True) + (agents_dir / "encrypted-agent.md").write_text( + "---\n" + "name: encrypted-agent\n" + "model: encrypted-profile\n" + f"profile_store_dir: {profile_dir}\n" + "---\n" + ) + + with patch( + "openhands.sdk.subagent.load.Path.home", return_value=tmp_path / "no_user" + ): + register_file_agents(tmp_path, cipher=cipher) + + agent = get_agent_factory("encrypted-agent").factory_func(_make_test_llm()) + + assert isinstance(agent.llm.api_key, SecretStr) + assert agent.llm.api_key.get_secret_value() == "profile-key" + + +@pytest.mark.parametrize("cipher", [None, Cipher("wrong-secret")]) +def test_agent_definition_to_factory_rejects_undecrypted_profile( + tmp_path: Path, + cipher: Cipher | None, +) -> None: + store = LLMProfileStore(base_dir=tmp_path) + store.save( + "encrypted-profile", + LLM(model="gpt-4o-mini", api_key=SecretStr("profile-key")), + include_secrets=True, + cipher=Cipher("right-secret"), + ) + agent_def = AgentDefinition( + name="encrypted-profile-agent", + model="encrypted-profile", + profile_store_dir=str(tmp_path), + ) + + with pytest.raises(ValueError, match="encrypted-profile"): + agent_definition_to_factory(agent_def, cipher=cipher)(_make_test_llm()) + + def test_agent_definition_to_factory_profile_store_dir(tmp_path: Path) -> None: """profile_store_dir on AgentDefinition is used by the factory.""" store = LLMProfileStore(base_dir=tmp_path) diff --git a/tests/tools/delegate/test_delegation.py b/tests/tools/delegate/test_delegation.py index f8f10af6ca..184fca56f0 100644 --- a/tests/tools/delegate/test_delegation.py +++ b/tests/tools/delegate/test_delegation.py @@ -17,6 +17,7 @@ register_agent, ) from openhands.sdk.subagent.schema import AgentDefinition +from openhands.sdk.utils.cipher import Cipher from openhands.tools.delegate import ( DelegateExecutor, DelegateObservation, @@ -264,6 +265,8 @@ def test_spawn_disables_streaming_for_sub_agents(): parent_conversation.state.workspace.working_dir = "/tmp" parent_conversation.state.persistence_dir = None parent_conversation._visualizer = None + cipher = Cipher("test-secret") + parent_conversation._cipher = cipher executor = DelegateExecutor() @@ -279,6 +282,7 @@ def test_spawn_disables_streaming_for_sub_agents(): sub_conversation = executor._sub_agents["test_agent"] sub_llm = sub_conversation.agent.llm assert sub_llm.stream is False, "Sub-agent LLM should have streaming disabled" + assert sub_conversation._cipher is cipher # Verify parent LLM still has streaming enabled (wasn't mutated) assert parent_llm.stream is True, "Parent LLM should still have streaming enabled" diff --git a/tests/tools/task/test_task_manager.py b/tests/tools/task/test_task_manager.py index e459f42ebe..8bf68bce6e 100644 --- a/tests/tools/task/test_task_manager.py +++ b/tests/tools/task/test_task_manager.py @@ -15,6 +15,7 @@ register_agent, ) from openhands.sdk.subagent.schema import AgentDefinition +from openhands.sdk.utils.cipher import Cipher from openhands.tools.preset import register_builtins_agents from openhands.tools.task.manager import ( Task, @@ -34,6 +35,7 @@ def _make_llm() -> LLM: def _make_parent_conversation( tmp_path: Path, persistence_dir: str | Path | None = None, + cipher: Cipher | None = None, ) -> LocalConversation: """Create a real (minimal) parent conversation for the manager.""" llm = _make_llm() @@ -44,6 +46,7 @@ def _make_parent_conversation( visualizer=None, delete_on_close=False, persistence_dir=persistence_dir, + cipher=cipher, ) @@ -191,6 +194,22 @@ def test_registers_uuid(self, tmp_path): assert task.id in manager._tasks assert isinstance(manager._tasks[task.id].conversation_id, uuid.UUID) + def test_create_and_resume_inherit_parent_cipher(self, tmp_path): + cipher = Cipher("test-secret") + parent = _make_parent_conversation(tmp_path, cipher=cipher) + manager = TaskManager() + manager._ensure_parent(parent) + register_builtins_agents() + + task = manager._create_task(subagent_type="general-purpose", description=None) + assert task.conversation is not None + assert task.conversation._cipher is cipher + + manager._evict_task(task) + resumed = manager._resume_task(resume=task.id, subagent_type="general-purpose") + assert resumed.conversation is not None + assert resumed.conversation._cipher is cipher + def test_create_task_uses_parent_max_iteration_when_factory_is_none(self, tmp_path): """Fallback to parent's max_iteration_per_run when factory has none.""" register_builtins_agents() diff --git a/tests/tools/test_builtin_agents.py b/tests/tools/test_builtin_agents.py index 80ab9c16ac..dde728d0e5 100644 --- a/tests/tools/test_builtin_agents.py +++ b/tests/tools/test_builtin_agents.py @@ -53,6 +53,12 @@ def test_load_all_builtins() -> None: }.issubset(names) +def test_builtins_only_inherit_parent_model() -> None: + agents = load_agents_from_dir(SUBAGENTS_DIR) + + assert all(agent.model == "inherit" for agent in agents) + + @pytest.mark.parametrize( "enable_browser, expected_agents", [ From 6525221754fb708e911cb344474520b4c1bae487 Mon Sep 17 00:00:00 2001 From: Shimada666 <649940882@qq.com> Date: Fri, 28 Aug 2026 16:57:25 +0800 Subject: [PATCH 3/4] test(subagent): tighten encrypted profile coverage --- .../openhands/sdk/llm/fallback_strategy.py | 2 +- tests/sdk/conversation/local/test_fork.py | 11 ++---- tests/sdk/llm/test_llm_fallback.py | 7 ++++ tests/sdk/llm/test_llm_profile_store.py | 36 ------------------- tests/sdk/subagent/test_subagent_registry.py | 22 ------------ tests/tools/test_builtin_agents.py | 6 ---- 6 files changed, 10 insertions(+), 74 deletions(-) diff --git a/openhands-sdk/openhands/sdk/llm/fallback_strategy.py b/openhands-sdk/openhands/sdk/llm/fallback_strategy.py index 3c0806943b..b3392902ec 100644 --- a/openhands-sdk/openhands/sdk/llm/fallback_strategy.py +++ b/openhands-sdk/openhands/sdk/llm/fallback_strategy.py @@ -151,7 +151,7 @@ def _iter_fallbacks(self) -> Generator[Any]: ) self._resolved.append(fb) yield fb - except (FileNotFoundError, ValueError) as exc: + except FileNotFoundError as exc: logger.error( "[Fallback Strategy] Failed to load " f"fallback profile '{name}': {exc}" diff --git a/tests/sdk/conversation/local/test_fork.py b/tests/sdk/conversation/local/test_fork.py index a6016cdb2f..e7e58e24a8 100644 --- a/tests/sdk/conversation/local/test_fork.py +++ b/tests/sdk/conversation/local/test_fork.py @@ -73,15 +73,6 @@ def create(cls, *args, **kwargs) -> Sequence[Self]: def test_fork_creates_new_id(): """Forked conversation must have a distinct ID.""" - with tempfile.TemporaryDirectory() as tmpdir: - src = Conversation(agent=_agent(), persistence_dir=tmpdir, workspace=tmpdir) - fork = src.fork() - - assert fork.id != src.id - assert isinstance(fork.id, uuid.UUID) - - -def test_fork_inherits_cipher(): cipher = Cipher("test-secret") with tempfile.TemporaryDirectory() as tmpdir: src = LocalConversation( @@ -92,6 +83,8 @@ def test_fork_inherits_cipher(): ) fork = src.fork() + assert fork.id != src.id + assert isinstance(fork.id, uuid.UUID) assert fork._cipher is cipher diff --git a/tests/sdk/llm/test_llm_fallback.py b/tests/sdk/llm/test_llm_fallback.py index f142e37b4c..d72add926f 100644 --- a/tests/sdk/llm/test_llm_fallback.py +++ b/tests/sdk/llm/test_llm_fallback.py @@ -344,6 +344,13 @@ def test_fallback_profile_uses_bound_cipher(tmp_path): assert isinstance(fallback.api_key, SecretStr) assert fallback.api_key.get_secret_value() == "k" + unbound_strategy = FallbackStrategy( + fallback_llms=["encrypted-fallback"], + profile_store_dir=tmp_path, + ) + with pytest.raises(ValueError, match="encrypted-fallback"): + list(unbound_strategy._iter_fallbacks()) + # ========================================================================= # Async error-handling parity tests (acompletion / aresponses) diff --git a/tests/sdk/llm/test_llm_profile_store.py b/tests/sdk/llm/test_llm_profile_store.py index 67dde898ad..6c429c165a 100644 --- a/tests/sdk/llm/test_llm_profile_store.py +++ b/tests/sdk/llm/test_llm_profile_store.py @@ -12,7 +12,6 @@ LLMProfileStore, ProfileLimitExceeded, ) -from openhands.sdk.utils.cipher import Cipher @pytest.fixture @@ -335,41 +334,6 @@ def test_load_nonexistent_profile(profile_store: LLMProfileStore) -> None: assert "not found" in str(exc_info.value) -@pytest.mark.parametrize("cipher", [None, Cipher("wrong-secret")]) -def test_load_for_execution_rejects_undecrypted_api_key( - profile_store: LLMProfileStore, - sample_llm_with_secrets: LLM, - cipher: Cipher | None, -) -> None: - profile_store.save( - "encrypted-profile", - sample_llm_with_secrets, - include_secrets=True, - cipher=Cipher("right-secret"), - ) - - with pytest.raises(ValueError, match="encrypted-profile"): - profile_store._load_for_execution("encrypted-profile", cipher=cipher) - - -def test_load_for_execution_decrypts_api_key( - profile_store: LLMProfileStore, - sample_llm_with_secrets: LLM, -) -> None: - cipher = Cipher("right-secret") - profile_store.save( - "encrypted-profile", - sample_llm_with_secrets, - include_secrets=True, - cipher=cipher, - ) - - loaded = profile_store._load_for_execution("encrypted-profile", cipher=cipher) - - assert isinstance(loaded.api_key, SecretStr) - assert loaded.api_key.get_secret_value() == "secret-api-key-12345" - - def test_load_nonexistent_shows_available( profile_store: LLMProfileStore, sample_llm: LLM ) -> None: diff --git a/tests/sdk/subagent/test_subagent_registry.py b/tests/sdk/subagent/test_subagent_registry.py index ae95339329..b98d2a93dd 100644 --- a/tests/sdk/subagent/test_subagent_registry.py +++ b/tests/sdk/subagent/test_subagent_registry.py @@ -592,28 +592,6 @@ def test_agent_definition_to_factory_model_profile_custom_store(tmp_path: Path) assert agent.llm.metrics is not parent_llm.metrics -def test_agent_definition_to_factory_decrypts_model_profile(tmp_path: Path) -> None: - """Encrypted model profiles are decrypted for file-based sub-agents.""" - cipher = Cipher("test-secret") - store = LLMProfileStore(base_dir=tmp_path) - profile_llm = LLM( - model="gpt-4o-mini", - api_key=SecretStr("profile-key"), - usage_id="profile-llm", - ) - store.save("encrypted-profile", profile_llm, include_secrets=True, cipher=cipher) - agent_def = AgentDefinition( - name="encrypted-profile-agent", - model="encrypted-profile", - profile_store_dir=str(tmp_path), - ) - - agent = agent_definition_to_factory(agent_def, cipher=cipher)(_make_test_llm()) - - assert isinstance(agent.llm.api_key, SecretStr) - assert agent.llm.api_key.get_secret_value() == "profile-key" - - def test_register_file_agents_forwards_cipher_to_factory(tmp_path: Path) -> None: cipher = Cipher("test-secret") profile_dir = tmp_path / "profiles" diff --git a/tests/tools/test_builtin_agents.py b/tests/tools/test_builtin_agents.py index dde728d0e5..80ab9c16ac 100644 --- a/tests/tools/test_builtin_agents.py +++ b/tests/tools/test_builtin_agents.py @@ -53,12 +53,6 @@ def test_load_all_builtins() -> None: }.issubset(names) -def test_builtins_only_inherit_parent_model() -> None: - agents = load_agents_from_dir(SUBAGENTS_DIR) - - assert all(agent.model == "inherit" for agent in agents) - - @pytest.mark.parametrize( "enable_browser, expected_agents", [ From 6496f0385bc1b269af8d4f994916e72858a25adf Mon Sep 17 00:00:00 2001 From: Shimada666 <649940882@qq.com> Date: Fri, 28 Aug 2026 21:16:46 +0800 Subject: [PATCH 4/4] fix(llm): harden encrypted profile execution Co-authored-by: openhands --- .../openhands/sdk/llm/fallback_strategy.py | 9 +++- .../openhands/sdk/llm/llm_profile_store.py | 49 ++++++++++--------- .../openhands/sdk/subagent/registry.py | 4 +- tests/sdk/llm/test_llm_fallback.py | 24 ++++++++- tests/sdk/llm/test_llm_profile_store.py | 20 ++++++++ tests/sdk/subagent/test_subagent_registry.py | 21 ++++++-- tests/tools/test_builtin_agents.py | 1 + 7 files changed, 97 insertions(+), 31 deletions(-) diff --git a/openhands-sdk/openhands/sdk/llm/fallback_strategy.py b/openhands-sdk/openhands/sdk/llm/fallback_strategy.py index b3392902ec..e35256cffb 100644 --- a/openhands-sdk/openhands/sdk/llm/fallback_strategy.py +++ b/openhands-sdk/openhands/sdk/llm/fallback_strategy.py @@ -15,7 +15,10 @@ from pydantic import BaseModel, Field, PrivateAttr from openhands.sdk.llm.exceptions import LLMNoResponseError -from openhands.sdk.llm.llm_profile_store import LLMProfileStore +from openhands.sdk.llm.llm_profile_store import ( + LLMProfileStore, + ProfileDecryptionError, +) from openhands.sdk.logger import get_logger @@ -151,7 +154,9 @@ def _iter_fallbacks(self) -> Generator[Any]: ) self._resolved.append(fb) yield fb - except FileNotFoundError as exc: + except ProfileDecryptionError: + raise + except (FileNotFoundError, ValueError) as exc: logger.error( "[Fallback Strategy] Failed to load " f"fallback profile '{name}': {exc}" diff --git a/openhands-sdk/openhands/sdk/llm/llm_profile_store.py b/openhands-sdk/openhands/sdk/llm/llm_profile_store.py index 3fc3b28edf..4542bc7290 100644 --- a/openhands-sdk/openhands/sdk/llm/llm_profile_store.py +++ b/openhands-sdk/openhands/sdk/llm/llm_profile_store.py @@ -11,7 +11,6 @@ from typing import TYPE_CHECKING, Any, Final, Protocol, runtime_checkable from filelock import FileLock, Timeout -from pydantic import SecretStr from openhands.sdk.llm.utils.openhands_provider import ( canonicalize_openhands_llm_payload, @@ -42,6 +41,10 @@ class ProfileLimitExceeded(Exception): """Raised when saving would exceed the configured profile limit.""" +class ProfileDecryptionError(ValueError): + """Raised when an encrypted profile secret cannot be decrypted.""" + + def _api_key_present(llm: LLM) -> bool: """True when ``llm`` carries a non-empty, non-redacted API key.""" from pydantic import SecretStr @@ -366,29 +369,31 @@ def _load_for_execution( *, cipher: Cipher | None = None, ) -> LLM: - """Load a profile and reject an unavailable encrypted API key.""" + """Load a profile and reject any secret that remained encrypted.""" + from openhands.sdk.llm.llm import LLM_SECRET_FIELDS + + profile_path = self._get_profile_path(name) llm = self.load(name, cipher=cipher) profile_name = name.removesuffix(".json") - stored_api_key = next( - ( - summary["api_key_set"] - for summary in self.list_summaries() - if summary["name"] == profile_name - ), - False, - ) - loaded_api_key = ( - llm.api_key.get_secret_value() - if isinstance(llm.api_key, SecretStr) - else llm.api_key - ) - if stored_api_key and ( - loaded_api_key is None or loaded_api_key.startswith(FERNET_TOKEN_PREFIX) - ): - raise ValueError( - f"Could not decrypt API key for profile '{profile_name}'. " - "Use the cipher that encrypted the profile." - ) + with self._acquire_lock(): + stored_profile = json.loads(profile_path.read_text()) + + loaded_profile = llm.model_dump(mode="json", context={"expose_secrets": True}) + for field in LLM_SECRET_FIELDS: + stored_value = stored_profile.get(field) + loaded_value = loaded_profile[field] + if ( + isinstance(loaded_value, str) + and loaded_value.startswith(FERNET_TOKEN_PREFIX) + ) or ( + isinstance(stored_value, str) + and stored_value.startswith(FERNET_TOKEN_PREFIX) + and loaded_value is None + ): + raise ProfileDecryptionError( + f"Could not decrypt secret '{field}' for profile " + f"'{profile_name}'. Use the cipher that encrypted the profile." + ) return llm def delete(self, name: str) -> None: diff --git a/openhands-sdk/openhands/sdk/subagent/registry.py b/openhands-sdk/openhands/sdk/subagent/registry.py index bb65a21294..d08227db7d 100644 --- a/openhands-sdk/openhands/sdk/subagent/registry.py +++ b/openhands-sdk/openhands/sdk/subagent/registry.py @@ -133,7 +133,9 @@ def register_agent_if_absent( no-ops when an agent with *name* is already registered, instead of raising `ValueError`. This is used by file-based and plugin-based agent loading to gracefully skip conflicts with programmatically - registered agents. + registered agents. Because the first registration wins, values captured + by ``factory_func`` (including a profile cipher) are pinned by the first + caller for that name. See `register_agent` for full parameter documentation. diff --git a/tests/sdk/llm/test_llm_fallback.py b/tests/sdk/llm/test_llm_fallback.py index d72add926f..b3bd549ec8 100644 --- a/tests/sdk/llm/test_llm_fallback.py +++ b/tests/sdk/llm/test_llm_fallback.py @@ -23,7 +23,10 @@ LLMServiceUnavailableError, ) from openhands.sdk.llm.llm import LLMCallContext -from openhands.sdk.llm.llm_profile_store import LLMProfileStore +from openhands.sdk.llm.llm_profile_store import ( + LLMProfileStore, + ProfileDecryptionError, +) from openhands.sdk.utils.cipher import Cipher @@ -348,10 +351,27 @@ def test_fallback_profile_uses_bound_cipher(tmp_path): fallback_llms=["encrypted-fallback"], profile_store_dir=tmp_path, ) - with pytest.raises(ValueError, match="encrypted-fallback"): + with pytest.raises(ProfileDecryptionError, match="encrypted-fallback"): list(unbound_strategy._iter_fallbacks()) +def test_fallback_skips_profile_with_missing_provider_connection(tmp_path): + store = LLMProfileStore(base_dir=tmp_path) + store.save( + "missing-connection", + LLM(model="gpt-4o-mini", provider_connection_id="deleted"), + ) + store.save("working", _get_llm("fallback-model"), include_secrets=True) + strategy = FallbackStrategy( + fallback_llms=["missing-connection", "working"], + profile_store_dir=tmp_path, + ) + + [fallback] = list(strategy._iter_fallbacks()) + + assert fallback.model == "fallback-model" + + # ========================================================================= # Async error-handling parity tests (acompletion / aresponses) # ========================================================================= diff --git a/tests/sdk/llm/test_llm_profile_store.py b/tests/sdk/llm/test_llm_profile_store.py index 6c429c165a..fc80c83615 100644 --- a/tests/sdk/llm/test_llm_profile_store.py +++ b/tests/sdk/llm/test_llm_profile_store.py @@ -10,8 +10,10 @@ from openhands.sdk.llm import LLM, LLM_PROFILE_SCHEMA_VERSION from openhands.sdk.llm.llm_profile_store import ( LLMProfileStore, + ProfileDecryptionError, ProfileLimitExceeded, ) +from openhands.sdk.utils.cipher import Cipher @pytest.fixture @@ -310,6 +312,24 @@ def test_save_with_secrets( assert "secret-api-key-12345" in content +def test_execution_load_checks_actual_case_variant_path( + profile_store: LLMProfileStore, + sample_llm_with_secrets: LLM, + monkeypatch: pytest.MonkeyPatch, +) -> None: + profile_store.save( + "Prod", + sample_llm_with_secrets, + include_secrets=True, + cipher=Cipher("right-secret"), + ) + stored_path = profile_store.base_dir / "Prod.json" + monkeypatch.setattr(profile_store, "_get_profile_path", lambda _name: stored_path) + + with pytest.raises(ProfileDecryptionError, match="api_key.*prod"): + profile_store._load_for_execution("prod") + + @pytest.mark.parametrize("name", ["my_profile", "my_profile.json"]) def test_load_existing_profile( name: str, profile_store: LLMProfileStore, sample_llm: LLM diff --git a/tests/sdk/subagent/test_subagent_registry.py b/tests/sdk/subagent/test_subagent_registry.py index b98d2a93dd..a6f0ceaebe 100644 --- a/tests/sdk/subagent/test_subagent_registry.py +++ b/tests/sdk/subagent/test_subagent_registry.py @@ -8,7 +8,10 @@ from openhands.sdk import LLM, Agent from openhands.sdk.context.condenser import LLMSummarizingCondenser, NoOpCondenser from openhands.sdk.hooks.config import HookConfig, HookDefinition, HookMatcher -from openhands.sdk.llm.llm_profile_store import LLMProfileStore +from openhands.sdk.llm.llm_profile_store import ( + LLMProfileStore, + ProfileDecryptionError, +) from openhands.sdk.mcp.config import MCPServer, dump_mcp_config from openhands.sdk.subagent.registry import ( _reset_registry_for_tests, @@ -624,14 +627,22 @@ def test_register_file_agents_forwards_cipher_to_factory(tmp_path: Path) -> None @pytest.mark.parametrize("cipher", [None, Cipher("wrong-secret")]) -def test_agent_definition_to_factory_rejects_undecrypted_profile( +@pytest.mark.parametrize( + "secret_field", + ["api_key", "aws_access_key_id", "aws_secret_access_key", "aws_session_token"], +) +def test_agent_definition_to_factory_rejects_undecrypted_secret( tmp_path: Path, cipher: Cipher | None, + secret_field: str, ) -> None: store = LLMProfileStore(base_dir=tmp_path) + profile = LLM.model_validate( + {"model": "gpt-4o-mini", secret_field: "profile-secret"} + ) store.save( "encrypted-profile", - LLM(model="gpt-4o-mini", api_key=SecretStr("profile-key")), + profile, include_secrets=True, cipher=Cipher("right-secret"), ) @@ -641,7 +652,9 @@ def test_agent_definition_to_factory_rejects_undecrypted_profile( profile_store_dir=str(tmp_path), ) - with pytest.raises(ValueError, match="encrypted-profile"): + with pytest.raises( + ProfileDecryptionError, match=rf"{secret_field}.*encrypted-profile" + ): agent_definition_to_factory(agent_def, cipher=cipher)(_make_test_llm()) diff --git a/tests/tools/test_builtin_agents.py b/tests/tools/test_builtin_agents.py index 80ab9c16ac..40b2f56539 100644 --- a/tests/tools/test_builtin_agents.py +++ b/tests/tools/test_builtin_agents.py @@ -77,6 +77,7 @@ def test_register_builtins_agents_registers_expected_factories( factory = get_agent_factory(name) agent = factory.factory_func(llm) assert isinstance(agent, Agent) + assert agent.llm is llm agent_tool_names[name] = [t.name for t in agent.tools] assert len(agent_tool_names) == len(expected_agents)