diff --git a/openhands-sdk/openhands/sdk/agent/base.py b/openhands-sdk/openhands/sdk/agent/base.py index dbd580e2d1..e97da7fb4a 100644 --- a/openhands-sdk/openhands/sdk/agent/base.py +++ b/openhands-sdk/openhands/sdk/agent/base.py @@ -304,6 +304,47 @@ def _decrypt_mcp_config(cls, data: Any, info: ValidationInfo) -> Any: return data + @model_validator(mode="after") + def _validate_unique_llm_usage_ids(self) -> AgentBase: + """Validate that LLMs with the same usage_id have the same configuration. + + When multiple LLM references share a usage_id, they must have identical + settings. This commonly happens when: + 1. An LLM object is intentionally shared (e.g., agent and condenser use + the same LLM) - this is fine, they're the same object + 2. An agent is deserialized from JSON, creating separate LLM objects from + what was originally a shared reference - this is fine if configs match + 3. Different LLMs are created with the same usage_id but different settings + (e.g., different models) - this is an error + + Only case 3 raises an error, as silently using one LLM config while + ignoring another would be confusing. + """ + usage_id_to_llms: dict[str, list[LLM]] = {} + for llm in self.get_all_llms(): + usage_id_to_llms.setdefault(llm.usage_id, []).append(llm) + + # Check for conflicting LLM configurations with the same usage_id + for usage_id, llms in usage_id_to_llms.items(): + if len(llms) <= 1: + continue + + # Compare LLM configs (excluding usage_id itself) + # Use model_dump with redaction to compare functional equivalence + first_config = llms[0].model_dump(exclude={"usage_id"}) + for other_llm in llms[1:]: + other_config = other_llm.model_dump(exclude={"usage_id"}) + if first_config != other_config: + raise ValueError( + f"Multiple LLMs share usage_id='{usage_id}' but have " + f"different configurations. This would cause one LLM's " + f"settings to be silently ignored. Set distinct usage_id " + f"values for LLMs with different configs, e.g.: " + f"LLM(model='gpt-4o', usage_id='agent'), " + f"LLM(model='gpt-4o-mini', usage_id='condenser')" + ) + return self + @model_serializer(mode="wrap") def _serialize_with_mcp_handling(self, handler, info: SerializationInfo): """Serialize the agent, handling mcp_config encryption/redaction. diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index a6bb1ec60e..948160847b 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -633,8 +633,18 @@ def _ensure_agent_ready(self) -> None: self.llm_registry.subscribe(self._state.stats.register_llm) registered = set(self.llm_registry.list_usage_ids()) for llm in list(self.agent.get_all_llms()): - if llm.usage_id not in registered: - self.llm_registry.add(llm) + if llm.usage_id in registered: + # Skip duplicates but warn - this shouldn't happen if agent + # was validated properly, but can occur with deserialized + # agents that have the same usage_id on multiple LLMs + logger.warning( + f"Skipping duplicate LLM registration for usage_id " + f"'{llm.usage_id}' (model={llm.model}). Consider using " + f"distinct usage_id values for each LLM." + ) + continue + self.llm_registry.add(llm) + registered.add(llm.usage_id) self._agent_ready = True diff --git a/tests/sdk/llm/test_llm_registry.py b/tests/sdk/llm/test_llm_registry.py index 9933a835f9..1f04e1c110 100644 --- a/tests/sdk/llm/test_llm_registry.py +++ b/tests/sdk/llm/test_llm_registry.py @@ -306,3 +306,160 @@ def test_llm_registry_does_not_reset_metrics_for_independent_llms(): # llm2 should have its own independent metrics assert llm2.metrics is not llm1.metrics assert llm2.metrics.accumulated_cost == 0.0 + + +def test_agent_rejects_conflicting_usage_id_llms(): + """Test agent validation rejects LLMs with same usage_id but different configs. + + When an agent has multiple LLMs with the same usage_id but different + configurations (e.g., different models), validation should fail with a clear + error message guiding the user to set distinct usage_id values. + + This catches the dangerous case where silently using one LLM config while + ignoring another would lead to unexpected behavior. + """ + import pytest + from pydantic import SecretStr + + from openhands.sdk.agent import Agent + from openhands.sdk.context.condenser import LLMSummarizingCondenser + + # Create two separate LLM objects with the same usage_id but different models + agent_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + # usage_id defaults to "default" + ) + condenser_llm = LLM( + model="gpt-4o-mini", # Different model! + api_key=SecretStr("test-key"), + # usage_id defaults to "default" - same as agent_llm + ) + + # Verify they are different objects with the same usage_id + assert id(agent_llm) != id(condenser_llm) + assert agent_llm.usage_id == condenser_llm.usage_id == "default" + + # Create condenser with its own LLM + condenser = LLMSummarizingCondenser(llm=condenser_llm, max_size=100) + + # Agent construction should fail with a clear error message + with pytest.raises(ValueError) as exc_info: + Agent(llm=agent_llm, condenser=condenser, tools=[]) + + error_msg = str(exc_info.value) + assert "usage_id='default'" in error_msg + assert "different configurations" in error_msg + + +def test_agent_accepts_shared_llm_with_same_usage_id(): + """Test agent accepts when the same LLM is shared (e.g., with condenser). + + When an agent shares the same LLM object between itself and its condenser, + this should work fine since they're the same object. + """ + from pydantic import SecretStr + + from openhands.sdk.agent import Agent + from openhands.sdk.context.condenser import LLMSummarizingCondenser + + # Create a single LLM object shared between agent and condenser + shared_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + ) + + # Create condenser sharing the agent's LLM + condenser = LLMSummarizingCondenser(llm=shared_llm, max_size=100) + + # Agent construction should succeed + agent = Agent(llm=shared_llm, condenser=condenser, tools=[]) + + # Verify only one LLM is discoverable (deduped by object identity) + llms = list(agent.get_all_llms()) + assert len(llms) == 1 + assert llms[0] is shared_llm + + +def test_agent_accepts_duplicate_llms_with_identical_config(): + """Test agent accepts multiple LLMs with same usage_id if configs match. + + When an agent is deserialized from JSON, Pydantic creates separate LLM + objects even if they were originally the same object. As long as their + configurations match, this should not raise an error. + """ + from pydantic import SecretStr + + from openhands.sdk.agent import Agent + from openhands.sdk.context.condenser import LLMSummarizingCondenser + + # Simulate what happens during deserialization: two separate objects + # with identical configs + agent_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + usage_id="shared", + ) + condenser_llm = LLM( + model="gpt-4o", # Same model + api_key=SecretStr("test-key"), # Same key + usage_id="shared", # Same usage_id + ) + + # Verify they are different objects + assert id(agent_llm) != id(condenser_llm) + + # Create condenser with its own LLM + condenser = LLMSummarizingCondenser(llm=condenser_llm, max_size=100) + + # Agent construction should succeed since configs match + agent = Agent(llm=agent_llm, condenser=condenser, tools=[]) + + # Verify both LLMs are discoverable (not deduped since different objects) + llms = list(agent.get_all_llms()) + assert len(llms) == 2 + + +def test_agent_accepts_distinct_usage_id_llms(): + """Test agent accepts LLMs with distinct usage_ids.""" + import tempfile + + from pydantic import SecretStr + + from openhands.sdk.agent import Agent + from openhands.sdk.context.condenser import LLMSummarizingCondenser + from openhands.sdk.conversation import Conversation + + # Create LLMs with distinct usage_ids + agent_llm = LLM( + model="gpt-4o", + api_key=SecretStr("test-key"), + usage_id="agent", + ) + condenser_llm = LLM( + model="gpt-4o-mini", + api_key=SecretStr("test-key"), + usage_id="condenser", + ) + + # Create condenser with its own LLM + condenser = LLMSummarizingCondenser(llm=condenser_llm, max_size=100) + + # Agent construction should succeed + agent = Agent(llm=agent_llm, condenser=condenser, tools=[]) + + # Verify both LLMs are discoverable + llms = list(agent.get_all_llms()) + assert len(llms) == 2 + usage_ids = {llm.usage_id for llm in llms} + assert usage_ids == {"agent", "condenser"} + + with tempfile.TemporaryDirectory() as tmpdir: + # Create conversation - this should work fine + convo = Conversation(agent=agent, workspace=tmpdir, persistence_dir=tmpdir) + convo.send_message("Hello") + + # Verify both LLMs were registered + assert "agent" in convo.llm_registry.list_usage_ids() + assert "condenser" in convo.llm_registry.list_usage_ids() + assert len(convo.llm_registry.list_usage_ids()) == 2