diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index 00e96dc396..5093ef771c 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -671,6 +671,16 @@ def _load_persisted_state_sync( base_state_file.read_text(), context=context ) + def _agent_from_base_state(self, conversation_id: UUID) -> AgentBase | None: + """Return the persisted agent from ``base_state.json`` (its single source + of truth), or ``None`` if there is no persisted state yet. + + Used by cold-path checks (e.g. codex-agent detection) that used to read + the agent from ``meta.json`` before the agent was removed from it. + """ + state = self._load_persisted_state_sync(conversation_id) + return state.agent if state is not None else None + def _children_index(self) -> dict[UUID, list[UUID]]: """Reverse map parent_id -> child ids; rebuilt per call because the catalog is mutated from several places and a cache could go stale.""" @@ -756,11 +766,17 @@ async def _has_local_codex_credential(self) -> bool: async def _resolve_credential_bindings( self, stored: StoredConversation, + agent: AgentBase | None = None, ) -> dict[str, VersionedCredentialBinding]: + # The agent no longer lives on ``stored`` (meta.json). Callers pass the + # agent explicitly (the new-conversation request agent, or the live + # agent); otherwise fall back to the persisted base_state.json agent. + if agent is None: + agent = self._agent_from_base_state(stored.id) bindings = self._credential_bindings.pop(stored.id, {}) if ( CODEX_AUTH_SECRET_NAME not in bindings - and self._is_codex_agent(stored.agent) + and self._is_codex_agent(agent) and await self._has_local_codex_credential() ): assert self.secrets_store is not None @@ -1208,8 +1224,13 @@ async def _start_conversation( existing_event_service is not None and existing_event_service.is_open() ): + existing_agent = ( + existing_event_service._conversation.agent + if existing_event_service._conversation is not None + else None + ) if ( - self._is_codex_agent(existing_event_service.stored.agent) + self._is_codex_agent(existing_agent) and CODEX_AUTH_SECRET_NAME not in existing_event_service.credential_bindings ): @@ -1261,7 +1282,7 @@ async def _start_conversation( f"Persisted conversation {conversation_id} has no record" ) managed_codex_credential = self._is_codex_agent( - existing_record.stored.agent + self._agent_from_base_state(conversation_id) ) and ( CODEX_AUTH_SECRET_NAME in self._credential_bindings.get(conversation_id, {}) @@ -1452,6 +1473,10 @@ async def _start_conversation( exclude={"agent_profile_id", "agent_launch_additions"}, ) + # The agent is persisted to base_state.json (not meta.json), so it is + # passed to _start_event_service separately. Default to request.agent. + new_agent: AgentBase = request.agent + # If secrets_encrypted=True, the agent's secrets (e.g., LLM api_key) are # cipher-encrypted and need decryption during model validation. Pass the # cipher in the validation context so validate_secret() can decrypt them. @@ -1473,6 +1498,13 @@ async def _start_conversation( }, context={"cipher": self.cipher}, ) + # Decrypt the agent's secrets too (it no longer rides on `stored`). + # Re-validate the serialized agent with the cipher context so + # validate_secret() decrypts LLM api_key, MCP env, etc. + agent_cls = type(request.agent) + new_agent = agent_cls.model_validate( + request_data["agent"], context={"cipher": self.cipher} + ) else: stored = StoredConversation( id=conversation_id, @@ -1480,8 +1512,12 @@ async def _start_conversation( **request_data, ) async with self._lifecycle_lock: + # New conversation: the agent is written to base_state.json (its + # single source of truth), not to meta.json. Pass it explicitly. + # ``new_agent`` is ``request.agent`` (decrypted when the request was + # secrets_encrypted). event_service = await self._start_event_service( - stored, is_new_conversation=True + stored, is_new_conversation=True, agent=new_agent ) initial_message = request.initial_message if initial_message: @@ -1735,9 +1771,12 @@ async def fork_conversation( # fork-specific fields. Without this, e.g. a fork of a client-tool # conversation would lose ``client_tools`` in meta.json and be unable # to re-register its tools after a server restart. + # Note: the agent is NOT stored in meta.json (StoredConversation) — the + # fork's agent is already persisted to the fork's base_state.json by + # ``source_conversation.fork`` above. It is passed to + # ``_start_event_service`` via ``agent=`` for the new-conversation path. fork_overrides: dict[str, Any] = { "id": fork_conv_id, - "agent": fork_agent, "workspace": fork_workspace, "title": title, "created_at": utc_now(), @@ -1756,7 +1795,7 @@ async def fork_conversation( try: async with self._lifecycle_lock: fork_event_service = await self._start_event_service( - fork_stored, is_new_conversation=True + fork_stored, is_new_conversation=True, agent=fork_agent ) except Exception: safe_rmtree(fork_dir) @@ -2023,16 +2062,26 @@ def get_instance(cls, config: Config) -> "ConversationService": ) async def _start_event_service( - self, stored: StoredConversation, *, is_new_conversation: bool = False + self, + stored: StoredConversation, + *, + is_new_conversation: bool = False, + agent: AgentBase | None = None, ) -> EventService: event_services = self._event_services if event_services is None: raise ValueError("inactive_service") - credential_bindings = await self._resolve_credential_bindings(stored) + # ``agent`` is supplied for a NEW conversation (meta.json no longer + # carries it). On resume it is ``None`` and both the credential check + # and EventService read the agent from base_state.json. + credential_bindings = await self._resolve_credential_bindings( + stored, agent=agent + ) event_service = EventService( stored=stored, conversations_dir=self.conversations_dir, + agent=agent, cipher=self.cipher, mcp_tool_provider=self.mcp_tool_provider, credential_bindings=credential_bindings, @@ -2138,11 +2187,15 @@ async def _maybe_subscribe_telemetry( if factory is None: return + live_conversation = getattr(event_service, "_conversation", None) + live_agent = ( + live_conversation.agent if live_conversation is not None else None + ) subscriber = TelemetrySubscriber( conversation_id=stored.id, sink=sink, factory=factory, - context=_build_telemetry_context(stored, factory), + context=_build_telemetry_context(stored, factory, agent=live_agent), ) await event_service.subscribe_to_events(subscriber) if is_new_conversation: @@ -2152,14 +2205,21 @@ async def _maybe_subscribe_telemetry( def _build_telemetry_context( - stored: StoredConversation, factory: DiagnosticEventFactory + stored: StoredConversation, + factory: DiagnosticEventFactory, + agent: AgentBase | None = None, ) -> ConversationTelemetryContext: """Reduce a stored conversation to its sanitized telemetry facts. Every read is defensive: a shape change upstream should degrade a property to ``unknown``, never raise into conversation startup. + + The agent is no longer stored on meta.json; callers pass the live/persisted + agent explicitly. ``getattr(stored, "agent", None)`` remains as a defensive + fallback for older callers. """ - agent = getattr(stored, "agent", None) + if agent is None: + agent = getattr(stored, "agent", None) llm = getattr(agent, "llm", None) workspace = getattr(stored, "workspace", None) diff --git a/openhands-agent-server/openhands/agent_server/event_service.py b/openhands-agent-server/openhands/agent_server/event_service.py index 141bf318f9..0c8770afc7 100644 --- a/openhands-agent-server/openhands/agent_server/event_service.py +++ b/openhands-agent-server/openhands/agent_server/event_service.py @@ -115,6 +115,11 @@ class EventService: stored: StoredConversation conversations_dir: Path + # Agent for a NEW conversation. meta.json (``stored``) no longer carries the + # agent — base_state.json is its single source of truth — so the creating + # caller passes it here. On resume this is ``None`` and the agent is loaded + # from base_state.json. + agent: AgentBase | None = None cipher: Cipher | None = None mcp_tool_provider: MCPToolProvider | None = None credential_bindings: dict[str, VersionedCredentialBinding] = field( @@ -179,17 +184,13 @@ async def save_meta(self): ) def _without_stored_secret(self, secret_name: str) -> StoredConversation: + # meta.json (StoredConversation) no longer carries the agent, so there is + # no agent_context secret to scrub here — only the stored secrets map. + # The agent's own secret scrub happens on base_state.json (see + # _scrub_persisted_credentials). secrets = dict(self.stored.secrets) secrets.pop(secret_name, None) - return self.stored.model_copy( - update={ - "secrets": secrets, - "agent": _without_agent_context_secret( - self.stored.agent, - secret_name, - ), - } - ) + return self.stored.model_copy(update={"secrets": secrets}) async def _scrub_persisted_credentials( self, @@ -972,10 +973,24 @@ async def start(self): working_dir = Path(workspace.working_dir) working_dir.mkdir(parents=True, exist_ok=True) self._ensure_workspace_is_git_repo(working_dir) - agent_cls = type(self.stored.agent) - agent = agent_cls.model_validate( - self.stored.agent.model_dump(context={"expose_secrets": True}), - ) + # base_state.json is the single source of truth for the agent. On resume + # (base_state exists) pass ``agent=None`` so LocalConversation keeps the + # persisted agent. On a new conversation the creating caller supplied the + # agent via ``self.agent``; deep-copy it (expose_secrets) so the running + # agent is independent of the caller's object. + base_state_exists = (self.conversation_dir / BASE_STATE).exists() + if base_state_exists: + agent: AgentBase | None = None + else: + if self.agent is None: + raise ValueError( + "Cannot start a new conversation without an agent: no " + "base_state.json to resume and no agent was provided." + ) + agent_cls = type(self.agent) + agent = agent_cls.model_validate( + self.agent.model_dump(context={"expose_secrets": True}), + ) # Create LocalConversation with plugins and hook_config. # Plugins are loaded lazily on first run()/send_message() call. @@ -987,16 +1002,18 @@ async def start(self): self._pub_sub, loop=asyncio.get_running_loop() ) - # Only wire token streaming for agents that can actually emit token - # callbacks. SDK LLM agents need stream=True, while ACP agents emit - # AgentMessageChunk text through their bridge without exposing an LLM. - streaming_enabled = isinstance(agent, ACPAgent) or any( - llm.stream for llm in agent.get_all_llms() - ) - logger.debug( - "Token streaming: %s", - "enabled" if streaming_enabled else "disabled (no LLM has stream=True)", - ) + # Token streaming is wired only for agents that can actually emit token + # callbacks (SDK LLM agents with stream=True, or ACP agents). For a NEW + # conversation the agent is known here, so decide now. On RESUME the + # agent is loaded from base_state.json during construction, so defer the + # decision until after (see the post-construction block below). + def _agent_can_stream(a: AgentBase) -> bool: + return isinstance(a, ACPAgent) or any( + llm.stream for llm in a.get_all_llms() + ) + + streaming_enabled = _agent_can_stream(agent) if agent is not None else True + streaming_decided = agent is not None def _publish_stream_delta( content: str | None = None, @@ -1059,6 +1076,17 @@ def _token_streaming_callback(chunk: LLMStreamChunk | str) -> None: conversation.set_confirmation_policy(self.stored.confirmation_policy) conversation.set_security_analyzer(self.stored.security_analyzer) + # On resume the agent was unknown at construction time (loaded from + # base_state.json), so decide token streaming now and disable it when the + # resolved agent can't emit token callbacks. + if not streaming_decided: + streaming_enabled = _agent_can_stream(conversation.agent) + logger.debug( + "Token streaming: %s", + "enabled" if streaming_enabled else "disabled (no LLM has stream=True)", + ) + if not streaming_enabled: + conversation._on_token = None self._conversation = conversation if isinstance(conversation.agent, ACPAgent): for secret_name, binding in self.credential_bindings.items(): @@ -1621,13 +1649,13 @@ async def switch_acp_model(self, model: str) -> None: For a conversation that has already started, runs the (blocking) protocol-level ``session/set_model`` round-trip in a worker thread; for - one not yet run, the SDK defers the switch (persist-only). Either way it - mirrors the new model into ``meta.json`` so the switch survives an - agent-server restart: ``start()`` rebuilds the agent from - ``self.stored.agent`` and ``ConversationState.create()`` copies that over - the persisted base_state.json on resume. Only ``acp_model`` needs - updating — ``model_post_init`` re-derives the sentinel ``llm.model`` on - reload. + one not yet run, the SDK defers the switch (persist-only). Either way the + switched model is persisted as the authoritative value in + ``base_state.json``: ``LocalConversation.switch_acp_model`` sets + ``state.agent`` to an agent copy carrying the new ``acp_model``, which the + autosave path writes to base_state. On resume the agent is rebuilt from + base_state (the single source of truth), so no ``meta.json`` mirror is + needed. """ if self._conversation is None: # Match the inactive-service convention of the other event-service @@ -1637,10 +1665,6 @@ async def switch_acp_model(self, model: str) -> None: raise ValueError("inactive_service") loop = asyncio.get_running_loop() await loop.run_in_executor(None, self._conversation.switch_acp_model, model) - self.stored = self.stored.model_copy( - update={"agent": self.stored.agent.model_copy(update={"acp_model": model})} - ) - await self.save_meta() async def close(self): self._closing = True diff --git a/openhands-agent-server/openhands/agent_server/models.py b/openhands-agent-server/openhands/agent_server/models.py index c710772b20..0182ccf1aa 100644 --- a/openhands-agent-server/openhands/agent_server/models.py +++ b/openhands-agent-server/openhands/agent_server/models.py @@ -13,6 +13,7 @@ from openhands.sdk.conversation.conversation_stats import ConversationStats from openhands.sdk.conversation.request import ( # re-export for backward compat ACPEnabledAgent as ACPEnabledAgent, + ConversationConfig as ConversationConfig, SendMessageRequest as SendMessageRequest, StartConversationRequest as StartConversationRequest, ) @@ -73,15 +74,17 @@ class EventSortOrder(StrEnum): TIMESTAMP_DESC = "TIMESTAMP_DESC" -class StoredConversation(StartConversationRequest): +class StoredConversation(ConversationConfig): """Stored details about a conversation. - Extends StartConversationRequest with server-assigned fields. + Extends :class:`ConversationConfig` (the agent-less shared config) with + server-assigned fields. It deliberately does NOT carry the ``agent``: the + single source of truth for the agent / runtime state is + ``ConversationState`` persisted to ``base_state.json``. Because + ``StoredConversation`` is not a ``StartConversationRequest``, the agent + cannot silently re-appear in ``meta.json``. """ - # agent_profile_id is resolved into launched_agent_profile at creation; exclude from - # the persistence payload so it does not re-appear in meta.json. - agent_profile_id: UUID | None = Field(default=None, exclude=True) required_runtime_credential_bindings: set[str] = Field(default_factory=set) id: OpenHandsUUID diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index e90cb8f308..ea87918cf7 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -198,7 +198,7 @@ class LocalConversation(BaseConversation): def __init__( self, - agent: AgentBase, + agent: AgentBase | None, workspace: str | Path | LocalWorkspace, plugins: list[PluginSource] | None = None, persistence_dir: str | Path | None = None, @@ -318,12 +318,20 @@ def __init__( # or, when resuming a persisted conversation without re-supplying them, # from the persisted agent's tool specs — mirroring the server resume # path so a fresh process can re-register the dynamic tools. + # Client tools are injected into the caller-supplied agent. When + # ``agent`` is None the agent is resumed from base_state.json (which + # already carries its persisted tool specs), so there is nothing to + # inject here. resolved_client_tools = list(client_tools or []) - if not resolved_client_tools and persistence_dir is not None: + if ( + agent is not None + and not resolved_client_tools + and persistence_dir is not None + ): resolved_client_tools = self._recover_persisted_client_tools( persistence_dir, desired_id ) - if resolved_client_tools: + if agent is not None and resolved_client_tools: from openhands.sdk.tool.client_tool import register_client_tools client_tool_specs = register_client_tools(resolved_client_tools) @@ -333,8 +341,6 @@ def __init__( ] if new_tools: agent = agent.model_copy(update={"tools": [*agent.tools, *new_tools]}) - - self.agent = agent if isinstance(workspace, (str, Path)): # LocalWorkspace accepts both str and Path via BeforeValidator workspace = LocalWorkspace(working_dir=workspace) @@ -358,6 +364,10 @@ def __init__( cipher=cipher, tags=tags, ) + # base_state.json is the source of truth for the agent. On resume with + # ``agent=None`` the state holds the persisted agent; adopt it here so + # ``self.agent`` and ``self._state.agent`` are the same object. + self.agent = self._state.agent self._bind_conversation_context(self.agent.llm) diff --git a/openhands-sdk/openhands/sdk/conversation/request.py b/openhands-sdk/openhands/sdk/conversation/request.py index 56f91bbbc9..b99722b176 100644 --- a/openhands-sdk/openhands/sdk/conversation/request.py +++ b/openhands-sdk/openhands/sdk/conversation/request.py @@ -90,14 +90,16 @@ class AgentLaunchAdditions(BaseModel): ) -class StartConversationRequest(BaseModel): - """Payload to create a new conversation. +class ConversationConfig(BaseModel): + """Shared conversation configuration — everything except the agent. - Supports any concrete :class:`AgentBase` implementation, including regular - OpenHands agents and ACP agents. Clients may provide either a concrete - ``agent`` payload or an ``agent_settings`` payload; when ``agent_settings`` - is provided without ``agent``, the settings are validated with the - ``agent_kind`` discriminator and converted to the appropriate agent type. + This is the common base for :class:`StartConversationRequest` (which adds the + ``agent``/``agent_settings``/``agent_profile_id`` family) and the + agent-server's ``StoredConversation`` (which does NOT persist the agent — + the agent's single source of truth is ``ConversationState`` / + ``base_state.json``). Keeping the agent off this base is what makes the + duplication structurally impossible rather than something a reviewer has to + remember to exclude. """ workspace: LocalWorkspace = Field( @@ -276,6 +278,23 @@ class StartConversationRequest(BaseModel): ), ) + +class StartConversationRequest(ConversationConfig): + """Payload to create a new conversation. + + Extends :class:`ConversationConfig` with the agent specification. Supports + any concrete :class:`AgentBase` implementation, including regular OpenHands + agents and ACP agents. Clients may provide either a concrete ``agent`` + payload or an ``agent_settings`` payload; when ``agent_settings`` is provided + without ``agent``, the settings are validated with the ``agent_kind`` + discriminator and converted to the appropriate agent type. + + Note: the agent lives here on the *request*, deliberately not on + ``ConversationConfig``. The persisted record (``StoredConversation``) does + not carry the agent — its single source of truth is ``ConversationState`` / + ``base_state.json``. + """ + agent_settings: dict[str, Any] | None = Field( default=None, exclude=True, diff --git a/openhands-sdk/openhands/sdk/conversation/state.py b/openhands-sdk/openhands/sdk/conversation/state.py index 596113aa82..9254a0d829 100644 --- a/openhands-sdk/openhands/sdk/conversation/state.py +++ b/openhands-sdk/openhands/sdk/conversation/state.py @@ -446,7 +446,7 @@ def _save_base_state(self, fs: FileStore) -> None: def create( cls: type["ConversationState"], id: ConversationID, - agent: AgentBase, + agent: AgentBase | None, workspace: BaseWorkspace, persistence_dir: str | None = None, max_iterations: int = 500, @@ -537,12 +537,18 @@ def create( # version or be corrupted. state.rebuild_view() - # Verify compatibility (agent class + tools) - agent.verify(state.agent, events=state._events) - # Commit runtime-provided values (may autosave) state._autosave_enabled = True - state.agent = agent + # Agent: base_state.json is the single source of truth. When the + # caller does not supply an agent (``agent is None``), keep the + # persisted one untouched — this is what lets a persisted + # switch_llm survive an idle-eviction reload. When a caller *does* + # supply an agent (legacy behavior), verify tool compatibility and + # let it override, so existing callers that reconfigure on resume + # keep working. + if agent is not None: + agent.verify(state.agent, events=state._events) + state.agent = agent state.workspace = workspace state.max_iterations = max_iterations diff --git a/tests/agent_server/telemetry/test_telemetry_disabled_by_default.py b/tests/agent_server/telemetry/test_telemetry_disabled_by_default.py index 56da9c9852..59e6662bae 100644 --- a/tests/agent_server/telemetry/test_telemetry_disabled_by_default.py +++ b/tests/agent_server/telemetry/test_telemetry_disabled_by_default.py @@ -118,7 +118,6 @@ async def test_conversation_service_reads_the_live_sink_not_a_captured_one( DiagnosticEventFactory, build_runtime_properties, ) - from openhands.sdk import LLM, Agent from openhands.sdk.security.confirmation_policy import NeverConfirm from openhands.sdk.workspace import LocalWorkspace @@ -164,7 +163,6 @@ async def subscribe_to_events(self, subscriber): event_service = _FakeEventService() stored = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test"), tools=[]), workspace=LocalWorkspace(working_dir=str(tmp_path)), confirmation_policy=NeverConfirm(), initial_message=None, diff --git a/tests/agent_server/telemetry/test_telemetry_subscriber.py b/tests/agent_server/telemetry/test_telemetry_subscriber.py index c76d106fe8..bdd792aca1 100644 --- a/tests/agent_server/telemetry/test_telemetry_subscriber.py +++ b/tests/agent_server/telemetry/test_telemetry_subscriber.py @@ -576,9 +576,9 @@ def test_confirmation_policy_is_read_from_the_field_that_exists(): assert "confirmation_mode" not in StoredConversation.model_fields assert "confirmation_policy" in StoredConversation.model_fields + agent = Agent(llm=LLM(model="anthropic/claude-sonnet-5", usage_id="t"), tools=[]) stored = StoredConversation( id=_uuid.uuid4(), - agent=Agent(llm=LLM(model="anthropic/claude-sonnet-5", usage_id="t"), tools=[]), workspace=LocalWorkspace(working_dir="/Users/alice/secret-project"), confirmation_policy=AlwaysConfirm(), user_id="canvas-user-42", @@ -589,6 +589,7 @@ def test_confirmation_policy_is_read_from_the_field_that_exists(): runtime=build_runtime_properties(deferred_init=False), salt="s", ), + agent=agent, ) from dataclasses import asdict diff --git a/tests/agent_server/test_agent_launch_additions.py b/tests/agent_server/test_agent_launch_additions.py index 800e3d0852..5fed84ca6b 100644 --- a/tests/agent_server/test_agent_launch_additions.py +++ b/tests/agent_server/test_agent_launch_additions.py @@ -126,8 +126,9 @@ async def test_launch_additions_apply_after_agent_resolution(profile_launch, tmp service = ConversationService(conversations_dir=tmp_path) service._event_services = {} - async def capture_start(stored, **_kwargs): + async def capture_start(stored, **kwargs): captured["stored"] = stored + captured["agent"] = kwargs.get("agent") return _mock_event_service(state) with ( @@ -145,10 +146,11 @@ async def capture_start(stored, **_kwargs): await service.start_conversation(request) stored = captured["stored"] - assert stored.agent.agent_context is not None - suffix = stored.agent.agent_context.system_message_suffix + agent = captured["agent"] + assert agent.agent_context is not None + suffix = agent.agent_context.system_message_suffix assert suffix == f"PROFILE_BASELINE\n\n{_RUNTIME_SERVICES}" - assert [tool.name for tool in stored.agent.tools] == ["canvas_ui_client"] + assert [tool.name for tool in agent.tools] == ["canvas_ui_client"] assert stored.agent_launch_additions is None assert stored.client_tools == [_CANVAS_UI] assert stored.tool_module_qualnames == {} @@ -157,10 +159,11 @@ async def capture_start(stored, **_kwargs): else: resolve_profile.assert_not_called() - restored = StoredConversation.model_validate(stored.model_dump(mode="json")) - assert restored.agent.agent_context is not None - restored_suffix = restored.agent.agent_context.system_message_suffix + restored_agent = type(agent).model_validate(agent.model_dump(mode="json")) + assert restored_agent.agent_context is not None + restored_suffix = restored_agent.agent_context.system_message_suffix assert restored_suffix is not None assert restored_suffix.count("") == 1 - assert [tool.name for tool in restored.agent.tools] == ["canvas_ui_client"] + assert [tool.name for tool in restored_agent.tools] == ["canvas_ui_client"] + restored = StoredConversation.model_validate(stored.model_dump(mode="json")) assert restored.client_tools == [_CANVAS_UI] diff --git a/tests/agent_server/test_agent_profile_conv_start.py b/tests/agent_server/test_agent_profile_conv_start.py index f46f60c4a6..d079bd4a32 100644 --- a/tests/agent_server/test_agent_profile_conv_start.py +++ b/tests/agent_server/test_agent_profile_conv_start.py @@ -536,8 +536,8 @@ async def _start_from_profile( profile: OpenHandsAgentProfile | ACPAgentProfile, resolved_settings: OpenHandsAgentSettings | ACPAgentSettings, persisted_settings: PersistedSettings, -) -> StoredConversation: - """Launch from ``profile`` and return the captured ``StoredConversation``. +) -> tuple[StoredConversation, Any]: + """Launch from ``profile`` and return the captured ``(StoredConversation, agent)``. Only the stores are stubbed, so ``_resolve_agent_from_profile`` and the settings read that feeds it both run for real — this is the path a client @@ -549,12 +549,14 @@ async def _start_from_profile( ) captured: dict[str, Any] = {} - async def capture_start(stored, **_kwargs): + async def capture_start(stored, **kwargs): + agent = kwargs["agent"] captured["stored"] = stored + captured["agent"] = agent event_service = AsyncMock(spec=EventService) event_service.get_state.return_value = ConversationState( id=uuid4(), - agent=stored.agent, + agent=agent, workspace=request.workspace, execution_status=ConversationExecutionStatus.IDLE, ) @@ -598,7 +600,7 @@ async def capture_start(stored, **_kwargs): MockStore.return_value.load.return_value = profile await service.start_conversation(request) - return captured["stored"] + return captured["stored"], captured["agent"] class TestConversationServiceStartFromProfile: @@ -649,8 +651,9 @@ async def test_start_from_profile_stamps_launched_agent_profile_on_stored( parent_conversation_id=None, ) - async def capture_start(stored, **_kwargs): + async def capture_start(stored, **kwargs): captured["stored"] = stored + captured["agent"] = kwargs.get("agent") return mock_es mock_ses.side_effect = capture_start @@ -662,8 +665,9 @@ async def capture_start(stored, **_kwargs): assert stored.launched_agent_profile is not None assert stored.launched_agent_profile.agent_profile_id == profile_id assert stored.launched_agent_profile.revision == 5 - # The resolved agent (not None) must be present - assert stored.agent is not None + # The resolved agent (not None) must be passed to _start_event_service + # (it is persisted to base_state.json, not meta.json). + assert captured["agent"] is not None @pytest.mark.asyncio async def test_profile_not_found_propagates(self, tmp_path): @@ -718,12 +722,12 @@ async def test_profile_launch_inherits_the_stored_memory_preference( ) ) - stored = await _start_from_profile( + stored, agent = await _start_from_profile( tmp_path, profile, resolved_settings, persisted ) - assert stored.agent.agent_context is not None - assert stored.agent.agent_context.load_memory is True + assert agent.agent_context is not None + assert agent.agent_context.load_memory is True @pytest.mark.parametrize( "persisted_settings", @@ -749,12 +753,12 @@ async def test_profile_launch_leaves_memory_off_without_the_preference( ): profile, resolved_settings = _resolved_settings_for("openhands") - stored = await _start_from_profile( + stored, agent = await _start_from_profile( tmp_path, profile, resolved_settings, persisted_settings ) - assert stored.agent.agent_context is not None - assert stored.agent.agent_context.load_memory is False + assert agent.agent_context is not None + assert agent.agent_context.load_memory is False # --------------------------------------------------------------------------- @@ -816,7 +820,6 @@ def test_launched_agent_profile_survives_stored_conversation_round_trip(self): lp = LaunchedAgentProfile(agent_profile_id=profile_id, revision=7) stored = StoredConversation( id=uuid4(), - agent=_make_agent(), workspace=LocalWorkspace(working_dir="/tmp"), launched_agent_profile=lp, ) @@ -834,7 +837,6 @@ def test_launched_agent_profile_survives_stored_conversation_round_trip(self): def test_stored_conversation_without_profile_has_none(self): stored = StoredConversation( id=uuid4(), - agent=_make_agent(), workspace=LocalWorkspace(working_dir="/tmp"), ) assert stored.launched_agent_profile is None @@ -898,7 +900,6 @@ def test_launched_agent_profile_survives_json_serialization(self, tmp_path): lp = LaunchedAgentProfile(agent_profile_id=profile_id, revision=5) stored = StoredConversation( id=uuid4(), - agent=_make_agent(), workspace=LocalWorkspace(working_dir=str(tmp_path)), launched_agent_profile=lp, ) diff --git a/tests/agent_server/test_auto_title_span_metadata.py b/tests/agent_server/test_auto_title_span_metadata.py index b734745b53..3598cbb125 100644 --- a/tests/agent_server/test_auto_title_span_metadata.py +++ b/tests/agent_server/test_auto_title_span_metadata.py @@ -72,7 +72,6 @@ def mocked_completion(**kwargs: Any): stored = StoredConversation( id=uuid4(), - agent=agent, workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, diff --git a/tests/agent_server/test_conversation_info_model.py b/tests/agent_server/test_conversation_info_model.py index 89ae9ed4ec..9cfbc5cd0b 100644 --- a/tests/agent_server/test_conversation_info_model.py +++ b/tests/agent_server/test_conversation_info_model.py @@ -56,7 +56,6 @@ def _make_stored(state: ConversationState) -> StoredConversation: workspace = LocalWorkspace(working_dir=state.workspace.working_dir) return StoredConversation( id=state.id, - agent=state.agent, workspace=workspace, title="Test", metrics=None, diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index 79b383e3cb..5c8f7ea1ec 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -6,6 +6,7 @@ import time from datetime import UTC, datetime from pathlib import Path +from typing import Any, cast from unittest.mock import AsyncMock, MagicMock, patch from uuid import UUID, uuid4 @@ -33,7 +34,7 @@ UpdateConversationRequest, ) from openhands.agent_server.utils import safe_rmtree as _safe_rmtree -from openhands.sdk import LLM, Agent, Message +from openhands.sdk import LLM, Agent, AgentBase, Message from openhands.sdk.agent.acp_agent import ACPAgent from openhands.sdk.conversation.state import ( ConversationExecutionStatus, @@ -62,12 +63,18 @@ def mock_event_service(): return service +# The agent is no longer stored on meta.json (StoredConversation); it lives in +# base_state.json (ConversationState). Tests that need an agent to build a state +# use this helper instead of reading it back off StoredConversation. +def _sample_agent() -> Agent: + return Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]) + + @pytest.fixture def sample_stored_conversation(): """Create a sample StoredConversation for testing.""" return StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -77,6 +84,40 @@ def sample_stored_conversation(): ) +@pytest.mark.asyncio +async def test_meta_json_has_no_agent_and_reload_uses_base_state(tmp_path): + """End-to-end single-source-of-truth guarantee. + + A newly started conversation must persist its agent to base_state.json and + NOT to meta.json. A fresh ConversationService (simulating a server restart) + must reload the agent from base_state.json. + """ + conversations_dir = tmp_path / "conversations" + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + request = StartConversationRequest( + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), + workspace=LocalWorkspace(working_dir=str(workspace_dir)), + confirmation_policy=NeverConfirm(), + ) + async with ConversationService(conversations_dir=conversations_dir) as service: + info, _ = await service.start_conversation(request) + conv_id = info.id + + conv_dir = conversations_dir / conv_id.hex + meta = json.loads((conv_dir / "meta.json").read_text()) + base_state = json.loads((conv_dir / "base_state.json").read_text()) + # meta.json is agent-free; base_state.json owns the agent. + assert "agent" not in meta + assert base_state["agent"]["llm"]["model"] == "gpt-4o" + + # A fresh service (restart) reloads the agent from base_state.json. + async with ConversationService(conversations_dir=conversations_dir) as service2: + reloaded = await service2.get_conversation(conv_id) + assert reloaded is not None + assert reloaded.agent.llm.model == "gpt-4o" + + def _create_running_terminal_action(tool_call_id: str = "call_1") -> ActionEvent: tool_call = MessageToolCall.from_chat_tool_call( ChatCompletionMessageToolCall( @@ -186,15 +227,17 @@ async def test_start_conversation_registers_and_injects_client_tools( ], ) - captured: dict[str, StoredConversation] = {} + captured: dict[str, Any] = {} - async def fake_start_event_service(stored: StoredConversation, **_kwargs): + async def fake_start_event_service(stored: StoredConversation, **kwargs): + agent = cast(AgentBase, kwargs.get("agent")) captured["stored"] = stored + captured["agent"] = agent service = AsyncMock(spec=EventService) service.stored = stored service.get_state.return_value = ConversationState( id=stored.id, - agent=stored.agent, + agent=agent, workspace=stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored.confirmation_policy, @@ -209,8 +252,9 @@ async def fake_start_event_service(stored: StoredConversation, **_kwargs): await conversation_service.start_conversation(request) stored = captured["stored"] + agent = captured["agent"] # Injected into the agent's tool specs so _initialize() can resolve it - assert "srv_show_dialog" in {t.name for t in stored.agent.tools} + assert "srv_show_dialog" in {t.name for t in agent.tools} # Persisted so forks / restarts can re-register the dynamic action type assert [s.name for s in stored.client_tools] == ["srv_show_dialog"] # The class is registered in the global tool registry @@ -262,15 +306,17 @@ async def test_start_conversation_decrypts_encrypted_agent_settings_mcp_env( == encrypted_mcp_token ) - captured: dict[str, StoredConversation] = {} + captured: dict[str, Any] = {} - async def fake_start_event_service(stored: StoredConversation, **_kwargs): + async def fake_start_event_service(stored: StoredConversation, **kwargs): + agent = cast(AgentBase, kwargs.get("agent")) captured["stored"] = stored + captured["agent"] = agent service = AsyncMock(spec=EventService) service.stored = stored service.get_state.return_value = ConversationState( id=stored.id, - agent=stored.agent, + agent=agent, workspace=stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored.confirmation_policy, @@ -284,11 +330,11 @@ async def fake_start_event_service(stored: StoredConversation, **_kwargs): ): await conversation_service.start_conversation(request) - stored = captured["stored"] - assert isinstance(stored.agent.llm.api_key, SecretStr) - assert stored.agent.llm.api_key.get_secret_value() == "sk-plaintext" + agent = captured["agent"] + assert isinstance(agent.llm.api_key, SecretStr) + assert agent.llm.api_key.get_secret_value() == "sk-plaintext" assert ( - dump_mcp_config(stored.agent.mcp_config)["github"]["env"][ + dump_mcp_config(agent.mcp_config)["github"]["env"][ "GITHUB_PERSONAL_ACCESS_TOKEN" ] == "ghp-plaintext" @@ -782,7 +828,7 @@ async def test_waiting_hydration_cannot_restore_deleted_conversation( record = service._conversation_records[conversation_id] state = ConversationState( id=conversation_id, - agent=record.stored.agent, + agent=_sample_agent(), workspace=record.stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=record.stored.confirmation_policy, @@ -845,7 +891,7 @@ async def test_shutdown_closes_runtime_from_in_flight_hydration( record = service._conversation_records[conversation_id] state = ConversationState( id=conversation_id, - agent=record.stored.agent, + agent=_sample_agent(), workspace=record.stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=record.stored.confirmation_policy, @@ -947,7 +993,7 @@ async def test_search_conversations_basic( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -980,7 +1026,6 @@ async def test_search_conversations_with_critic_redacts_api_key( ) stored_conv = StoredConversation( id=uuid4(), - agent=agent, workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -993,7 +1038,7 @@ async def test_search_conversations_with_critic_redacts_api_key( mock_service.stored = stored_conv mock_service.get_state.return_value = ConversationState( id=stored_conv.id, - agent=stored_conv.agent, + agent=agent, workspace=stored_conv.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored_conv.confirmation_policy, @@ -1027,7 +1072,6 @@ async def test_search_conversations_status_filter(self, conversation_service): ): stored_conv = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1040,7 +1084,7 @@ async def test_search_conversations_status_filter(self, conversation_service): mock_service.stored = stored_conv mock_state = ConversationState( id=stored_conv.id, - agent=stored_conv.agent, + agent=_sample_agent(), workspace=stored_conv.workspace, execution_status=status, confirmation_policy=stored_conv.confirmation_policy, @@ -1079,7 +1123,6 @@ async def test_search_conversations_sorting(self, conversation_service): for i in range(3): stored_conv = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1094,7 +1137,7 @@ async def test_search_conversations_sorting(self, conversation_service): mock_service.stored = stored_conv mock_state = ConversationState( id=stored_conv.id, - agent=stored_conv.agent, + agent=_sample_agent(), workspace=stored_conv.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored_conv.confirmation_policy, @@ -1156,7 +1199,6 @@ async def test_search_conversations_pagination(self, conversation_service): for i in range(5): stored_conv = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1169,7 +1211,7 @@ async def test_search_conversations_pagination(self, conversation_service): mock_service.stored = stored_conv mock_state = ConversationState( id=stored_conv.id, - agent=stored_conv.agent, + agent=_sample_agent(), workspace=stored_conv.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored_conv.confirmation_policy, @@ -1226,7 +1268,6 @@ async def test_search_conversations_combined_filter_and_sort( for status, created_at in conversations_data: stored_conv = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1239,7 +1280,7 @@ async def test_search_conversations_combined_filter_and_sort( mock_service.stored = stored_conv mock_state = ConversationState( id=stored_conv.id, - agent=stored_conv.agent, + agent=_sample_agent(), workspace=stored_conv.workspace, execution_status=status, confirmation_policy=stored_conv.confirmation_policy, @@ -1267,7 +1308,7 @@ async def test_search_conversations_invalid_page_id( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -1316,7 +1357,7 @@ async def test_count_conversations_basic( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -1343,7 +1384,6 @@ async def test_count_conversations_status_filter(self, conversation_service): for i, status in enumerate(statuses): stored_conv = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1356,7 +1396,7 @@ async def test_count_conversations_status_filter(self, conversation_service): mock_service.stored = stored_conv mock_state = ConversationState( id=stored_conv.id, - agent=stored_conv.agent, + agent=_sample_agent(), workspace=stored_conv.workspace, execution_status=status, confirmation_policy=stored_conv.confirmation_policy, @@ -1393,7 +1433,6 @@ async def test_count_conversations_includes_regular_and_acp( ): legacy_conversation = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1403,7 +1442,6 @@ async def test_count_conversations_includes_regular_and_acp( ) acp_conversation = StoredConversation( id=uuid4(), - agent=ACPAgent(acp_command=["echo", "test"]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1417,7 +1455,7 @@ async def test_count_conversations_includes_regular_and_acp( mock_service.stored = stored_conv mock_service.get_state.return_value = ConversationState( id=stored_conv.id, - agent=stored_conv.agent, + agent=_sample_agent(), workspace=stored_conv.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored_conv.confirmation_policy, @@ -1569,16 +1607,18 @@ async def test_start_conversation_with_worktree_uses_git_worktree( worktree=True, ) - captured: dict[str, StoredConversation] = {} + captured: dict[str, Any] = {} def _event_service_factory(**kwargs): stored = kwargs["stored"] + agent = cast(AgentBase, kwargs.get("agent")) captured["stored"] = stored + captured["agent"] = agent mock_event_service = AsyncMock(spec=EventService) mock_event_service.stored = stored mock_event_service.get_state.return_value = ConversationState( id=stored.id, - agent=stored.agent, + agent=agent or _sample_agent(), workspace=stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored.confirmation_policy, @@ -1607,8 +1647,9 @@ def _event_service_factory(**kwargs): ) == expected_branch ) - assert stored.agent.agent_context is not None - suffix = stored.agent.agent_context.system_message_suffix + agent = captured["agent"] + assert agent.agent_context is not None + suffix = agent.agent_context.system_message_suffix assert suffix is not None assert str(repo_dir.resolve()) in suffix assert str(expected_worktree) in suffix @@ -1633,16 +1674,18 @@ async def test_start_conversation_with_worktree_preserves_relative_workspace( worktree=True, ) - captured: dict[str, StoredConversation] = {} + captured: dict[str, Any] = {} def _event_service_factory(**kwargs): stored = kwargs["stored"] + agent = cast(AgentBase, kwargs.get("agent")) captured["stored"] = stored + captured["agent"] = agent mock_event_service = AsyncMock(spec=EventService) mock_event_service.stored = stored mock_event_service.get_state.return_value = ConversationState( id=stored.id, - agent=stored.agent, + agent=agent or _sample_agent(), workspace=stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored.confirmation_policy, @@ -1682,16 +1725,18 @@ async def test_start_conversation_with_worktree_ignores_non_git_workspace( worktree=True, ) - captured: dict[str, StoredConversation] = {} + captured: dict[str, Any] = {} def _event_service_factory(**kwargs): stored = kwargs["stored"] + agent = cast(AgentBase, kwargs.get("agent")) captured["stored"] = stored + captured["agent"] = agent mock_event_service = AsyncMock(spec=EventService) mock_event_service.stored = stored mock_event_service.get_state.return_value = ConversationState( id=stored.id, - agent=stored.agent, + agent=agent or _sample_agent(), workspace=stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored.confirmation_policy, @@ -1706,10 +1751,11 @@ def _event_service_factory(**kwargs): stored = captured["stored"] + agent = captured["agent"] assert stored.worktree is True assert stored.workspace.working_dir == str(workspace_dir) assert result.workspace.working_dir == str(workspace_dir) - assert stored.agent.agent_context is None + assert agent.agent_context is None assert not (worktree_root / str(conversation_id)).exists() def test_get_worktree_start_point_prefers_origin_default_branch(self, tmp_path): @@ -1881,7 +1927,6 @@ async def test_start_conversation_reuse_checks_is_open(self, conversation_servic mock_event_service.is_open.return_value = False mock_event_service.stored = StoredConversation( id=custom_id, - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1906,7 +1951,6 @@ async def test_start_conversation_reuse_checks_is_open(self, conversation_servic mock_new_service = AsyncMock(spec=EventService) mock_new_service.stored = StoredConversation( id=custom_id, - agent=request.agent, workspace=request.workspace, confirmation_policy=request.confirmation_policy, initial_message=request.initial_message, @@ -1941,7 +1985,6 @@ async def test_start_conversation_reuse_when_open(self, conversation_service): mock_event_service.is_open.return_value = True mock_event_service.stored = StoredConversation( id=custom_id, - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1951,7 +1994,7 @@ async def test_start_conversation_reuse_when_open(self, conversation_service): ) mock_state = ConversationState( id=custom_id, - agent=mock_event_service.stored.agent, + agent=_sample_agent(), workspace=mock_event_service.stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=mock_event_service.stored.confirmation_policy, @@ -1983,9 +2026,9 @@ async def test_start_conversation_returns_existing_acp_conversation( self, conversation_service ): custom_id = uuid4() + acp_agent = ACPAgent(acp_command=["echo", "test"]) stored = StoredConversation( id=custom_id, - agent=ACPAgent(acp_command=["echo", "test"]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1996,9 +2039,12 @@ async def test_start_conversation_returns_existing_acp_conversation( mock_event_service = AsyncMock(spec=EventService) mock_event_service.is_open.return_value = True mock_event_service.stored = stored + # The agent lives on base_state.json / the live conversation now. + mock_event_service._conversation = MagicMock() + mock_event_service._conversation.agent = acp_agent mock_event_service.get_state.return_value = ConversationState( id=stored.id, - agent=stored.agent, + agent=acp_agent, workspace=stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored.confirmation_policy, @@ -2035,7 +2081,6 @@ async def test_start_event_service_failure_cleanup(self, conversation_service): with tempfile.TemporaryDirectory() as temp_dir: stored = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir=temp_dir), confirmation_policy=NeverConfirm(), initial_message=None, @@ -2071,7 +2116,6 @@ async def test_start_event_service_success_stores_service( with tempfile.TemporaryDirectory() as temp_dir: stored = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir=temp_dir), confirmation_policy=NeverConfirm(), initial_message=None, @@ -2119,7 +2163,7 @@ async def test_update_conversation_success( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2150,7 +2194,7 @@ async def test_update_conversation_strips_whitespace( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2181,7 +2225,7 @@ async def test_update_conversation_tags_uses_state_lock( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2215,7 +2259,7 @@ async def test_update_conversation_tags_wait_does_not_block_event_loop( mock_service.stored = sample_stored_conversation state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2302,7 +2346,7 @@ async def test_update_conversation_notifies_webhooks( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2335,9 +2379,9 @@ async def test_update_conversation_notifies_webhooks( async def test_update_acp_conversation_notifies_webhooks_with_acp_shape( self, conversation_service ): + acp_agent = ACPAgent(acp_command=["echo", "test"]) stored_conversation = StoredConversation( id=uuid4(), - agent=ACPAgent(acp_command=["echo", "test"]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -2349,7 +2393,7 @@ async def test_update_acp_conversation_notifies_webhooks_with_acp_shape( mock_service.stored = stored_conversation mock_state = ConversationState( id=stored_conversation.id, - agent=stored_conversation.agent, + agent=acp_agent, workspace=stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=stored_conversation.confirmation_policy, @@ -2381,7 +2425,7 @@ async def test_update_conversation_persists_changes( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2413,7 +2457,7 @@ async def test_update_conversation_multiple_times( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2464,7 +2508,7 @@ async def test_update_conversation_sets_updated_at( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2509,7 +2553,6 @@ async def test_delete_conversation_success(self, conversation_service): mock_service.conversation_dir = "/tmp/test_conversation" mock_service.stored = StoredConversation( id=conversation_id, - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="/tmp/test_workspace"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -2519,7 +2562,7 @@ async def test_delete_conversation_success(self, conversation_service): ) mock_state = ConversationState( id=conversation_id, - agent=mock_service.stored.agent, + agent=_sample_agent(), workspace=mock_service.stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=mock_service.stored.confirmation_policy, @@ -2565,7 +2608,7 @@ async def test_delete_conversation_notifies_webhooks_with_deleting_status( mock_service.stored = sample_stored_conversation mock_state = ConversationState( id=sample_stored_conversation.id, - agent=sample_stored_conversation.agent, + agent=_sample_agent(), workspace=sample_stored_conversation.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=sample_stored_conversation.confirmation_policy, @@ -2619,7 +2662,6 @@ async def test_delete_conversation_webhook_failure(self, conversation_service): mock_service.conversation_dir = "/tmp/test_conversation" mock_service.stored = StoredConversation( id=conversation_id, - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="/tmp/test_workspace"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -2662,7 +2704,6 @@ async def test_delete_conversation_close_failure(self, conversation_service): mock_service.conversation_dir = "/tmp/test_conversation" mock_service.stored = StoredConversation( id=conversation_id, - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="/tmp/test_workspace"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -2672,7 +2713,7 @@ async def test_delete_conversation_close_failure(self, conversation_service): ) mock_state = ConversationState( id=conversation_id, - agent=mock_service.stored.agent, + agent=_sample_agent(), workspace=mock_service.stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=mock_service.stored.confirmation_policy, @@ -2711,13 +2752,12 @@ async def test_delete_conversation_retains_retryable_credential_close( mock_service.conversation_dir = conversation_dir mock_service.stored = StoredConversation( id=conversation_id, - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir=tmp_path / "workspace"), confirmation_policy=NeverConfirm(), ) mock_service.get_state.return_value = ConversationState( id=conversation_id, - agent=mock_service.stored.agent, + agent=_sample_agent(), workspace=mock_service.stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=mock_service.stored.confirmation_policy, @@ -2758,7 +2798,6 @@ async def test_delete_conversation_directory_removal_failure( mock_service.conversation_dir = "/tmp/test_conversation" mock_service.stored = StoredConversation( id=conversation_id, - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="/tmp/test_workspace"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -2768,7 +2807,7 @@ async def test_delete_conversation_directory_removal_failure( ) mock_state = ConversationState( id=conversation_id, - agent=mock_service.stored.agent, + agent=_sample_agent(), workspace=mock_service.stored.workspace, execution_status=ConversationExecutionStatus.IDLE, confirmation_policy=mock_service.stored.confirmation_policy, @@ -2884,9 +2923,9 @@ def _make_service( llm_model: str = "gpt-4o", llm_usage_id: str = "test-llm", ) -> AsyncMock: + agent = Agent(llm=LLM(model=llm_model, usage_id=llm_usage_id), tools=[]) stored = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model=llm_model, usage_id=llm_usage_id), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -2898,7 +2937,7 @@ def _make_service( service.stored = stored mock_conversation = MagicMock() - mock_conversation.agent.llm = stored.agent.llm + mock_conversation.agent.llm = agent.llm service._conversation = mock_conversation return service @@ -3565,7 +3604,6 @@ def _seed(conversations_dir: Path, count: int, workspace_dir: Path) -> list[UUID target.mkdir() stored = StoredConversation( id=conversation_id, - agent=Agent(llm=LLM(model="gpt-4o", usage_id=f"llm-{i}"), tools=[]), workspace=LocalWorkspace(working_dir=str(workspace_dir)), confirmation_policy=NeverConfirm(), ) @@ -3574,7 +3612,7 @@ def _seed(conversations_dir: Path, count: int, workspace_dir: Path) -> list[UUID (target / "meta.json").write_text(stored.model_dump_json()) state = ConversationState( id=conversation_id, - agent=stored.agent, + agent=_sample_agent(), workspace=stored.workspace, persistence_dir=str(target), ) diff --git a/tests/agent_server/test_conversation_service_plugin.py b/tests/agent_server/test_conversation_service_plugin.py index 7fa7ad723b..701c5d0576 100644 --- a/tests/agent_server/test_conversation_service_plugin.py +++ b/tests/agent_server/test_conversation_service_plugin.py @@ -197,7 +197,6 @@ async def test_start_conversation_with_plugins_list(conversation_service, tmp_pa mock_event_service.get_state.return_value = mock_state mock_event_service.stored = StoredConversation( id=mock_state.id, - agent=request.agent, **request.model_dump(exclude={"agent"}), created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -211,8 +210,11 @@ async def test_start_conversation_with_plugins_list(conversation_service, tmp_pa assert stored.plugins is not None assert len(stored.plugins) == 1 assert stored.plugins[0].source == str(plugin_dir) - # Agent context NOT populated yet (lazy loading in LocalConversation) - assert stored.agent.agent_context is None + # Agent is passed to EventService separately (persisted to + # base_state.json, not meta.json). Agent context NOT populated yet + # (lazy loading in LocalConversation). + agent = mock_event_service_class.call_args.kwargs["agent"] + assert agent.agent_context is None @pytest.mark.asyncio @@ -257,7 +259,6 @@ async def test_start_conversation_with_multiple_plugins(conversation_service, tm mock_event_service.get_state.return_value = mock_state mock_event_service.stored = StoredConversation( id=mock_state.id, - agent=request.agent, **request.model_dump(exclude={"agent"}), created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -310,7 +311,6 @@ async def test_plugins_persisted_in_stored_conversation_for_lazy_loading( mock_event_service.get_state.return_value = mock_state mock_event_service.stored = StoredConversation( id=mock_state.id, - agent=request.agent, **request.model_dump(exclude={"agent"}), created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -446,7 +446,6 @@ async def test_start_conversation_stores_both_hooks_and_plugins_for_lazy_merge( mock_event_service.get_state.return_value = mock_state mock_event_service.stored = StoredConversation( id=mock_state.id, - agent=request.agent, **request.model_dump(exclude={"agent"}), created_at=datetime.now(UTC), updated_at=datetime.now(UTC), diff --git a/tests/agent_server/test_conversation_tags.py b/tests/agent_server/test_conversation_tags.py index ab616b5ffd..f76a762f5d 100644 --- a/tests/agent_server/test_conversation_tags.py +++ b/tests/agent_server/test_conversation_tags.py @@ -236,7 +236,6 @@ async def test_event_service_start_forwards_tags_to_local_conversation(tmp_path) tags = {"source": "pipeline", "symbol": "gold"} stored = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir=str(tmp_path)), confirmation_policy=NeverConfirm(), tags=tags, @@ -246,6 +245,7 @@ async def test_event_service_start_forwards_tags_to_local_conversation(tmp_path) event_service = EventService( stored=stored, + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), conversations_dir=tmp_path / "conversations", ) @@ -277,7 +277,6 @@ async def test_event_service_start_forwards_observability_span_name(tmp_path): """EventService.start() must pass stored child span names to LocalConversation.""" stored = StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir=str(tmp_path)), confirmation_policy=NeverConfirm(), observability_span_name="pr_review_evaluation", @@ -287,6 +286,7 @@ async def test_event_service_start_forwards_observability_span_name(tmp_path): event_service = EventService( stored=stored, + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), conversations_dir=tmp_path / "conversations", ) diff --git a/tests/agent_server/test_credential_binding.py b/tests/agent_server/test_credential_binding.py index 513dca2759..01fbebbfb0 100644 --- a/tests/agent_server/test_credential_binding.py +++ b/tests/agent_server/test_credential_binding.py @@ -367,14 +367,14 @@ async def test_direct_conversations_share_rotated_canonical_value(tmp_path) -> N workspace = LocalWorkspace(working_dir=tmp_path / "workspace") first = await service._resolve_credential_bindings( - StoredConversation(id=uuid4(), agent=agent, workspace=workspace) + StoredConversation(id=uuid4(), workspace=workspace), agent=agent ) first_binding = first["CODEX_AUTH_JSON"] initial = await first_binding.load() await first_binding.replace(initial.version, "r1") second = await service._resolve_credential_bindings( - StoredConversation(id=uuid4(), agent=agent, workspace=workspace) + StoredConversation(id=uuid4(), workspace=workspace), agent=agent ) assert (await second["CODEX_AUTH_JSON"].load()).value == "r1" @@ -571,11 +571,13 @@ async def test_managed_start_scrubs_all_durable_credential_copies(tmp_path) -> N assert "CODEX_AUTH_JSON" not in event_service.stored.secrets assert "KEEP" in event_service.stored.secrets - assert event_service.stored.agent.agent_context is not None - assert "CODEX_AUTH_JSON" not in ( - event_service.stored.agent.agent_context.secrets or {} - ) - assert "KEEP" in (event_service.stored.agent.agent_context.secrets or {}) + # The agent is no longer stored on meta.json; it lives on the live + # conversation / base_state.json. Assert the scrub there. + assert event_service._conversation is not None + live_agent = event_service._conversation.agent + assert live_agent.agent_context is not None + assert "CODEX_AUTH_JSON" not in (live_agent.agent_context.secrets or {}) + assert "KEEP" in (live_agent.agent_context.secrets or {}) assert "CODEX_AUTH_JSON" not in state.secret_registry.secret_sources assert "KEEP" in state.secret_registry.secret_sources assert state.agent.agent_context is not None @@ -585,7 +587,8 @@ async def test_managed_start_scrubs_all_durable_credential_copies(tmp_path) -> N meta = json.loads((conversation_dir / "meta.json").read_text()) base_state = json.loads((conversation_dir / "base_state.json").read_text()) assert "CODEX_AUTH_JSON" not in meta["secrets"] - assert "CODEX_AUTH_JSON" not in meta["agent"]["agent_context"]["secrets"] + # meta.json no longer carries the agent at all. + assert "agent" not in meta assert "CODEX_AUTH_JSON" not in base_state["agent"]["agent_context"]["secrets"] assert "CODEX_AUTH_JSON" not in base_state["secret_registry"]["secret_sources"] artifacts = json.dumps(meta) + json.dumps(base_state) @@ -667,16 +670,17 @@ def fail_first_write(*args, **kwargs): assert event_service.credential_bindings["CODEX_AUTH_JSON"] is binding assert "CODEX_AUTH_JSON" not in event_service.stored.secrets assert "CODEX_AUTH_JSON" not in state.secret_registry.secret_sources - assert event_service.stored.agent.agent_context is not None - assert "CODEX_AUTH_JSON" not in ( - event_service.stored.agent.agent_context.secrets or {} - ) + # The agent is on the live conversation / base_state.json, not meta.json. + assert event_service._conversation is not None + live_agent = event_service._conversation.agent + assert live_agent.agent_context is not None + assert "CODEX_AUTH_JSON" not in (live_agent.agent_context.secrets or {}) conversation_dir = tmp_path / "conversations" / info.id.hex meta = json.loads((conversation_dir / "meta.json").read_text()) base_state = json.loads((conversation_dir / "base_state.json").read_text()) assert "CODEX_AUTH_JSON" not in meta["secrets"] - assert "CODEX_AUTH_JSON" not in meta["agent"]["agent_context"]["secrets"] + assert "agent" not in meta assert "CODEX_AUTH_JSON" not in base_state["agent"]["agent_context"]["secrets"] assert "CODEX_AUTH_JSON" not in base_state["secret_registry"]["secret_sources"] durable = json.dumps(meta) + json.dumps(base_state) @@ -1090,8 +1094,9 @@ async def test_resume_removes_legacy_persisted_credential(tmp_path) -> None: meta = json.loads((conversation_dir / "meta.json").read_text()) base_state = json.loads((conversation_dir / "base_state.json").read_text()) assert "CODEX_AUTH_JSON" not in meta["secrets"] - assert "CODEX_AUTH_JSON" not in meta["agent"]["agent_context"]["secrets"] - assert "KEEP" in meta["agent"]["agent_context"]["secrets"] + # meta.json no longer carries the agent; the agent (and its scrubbed + # agent_context) lives solely in base_state.json. + assert "agent" not in meta assert "CODEX_AUTH_JSON" not in base_state["agent"]["agent_context"]["secrets"] assert "KEEP" in base_state["agent"]["agent_context"]["secrets"] assert "CODEX_AUTH_JSON" not in base_state["secret_registry"]["secret_sources"] diff --git a/tests/agent_server/test_event_service.py b/tests/agent_server/test_event_service.py index 3f53ad3331..972352c17c 100644 --- a/tests/agent_server/test_event_service.py +++ b/tests/agent_server/test_event_service.py @@ -60,12 +60,23 @@ ) +# Agent for a new conversation. meta.json (StoredConversation) no longer carries +# the agent — base_state.json is its single source of truth — so tests pass it to +# EventService separately. +def _sample_agent() -> Agent: + return Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]) + + +@pytest.fixture +def sample_agent(): + return _sample_agent() + + @pytest.fixture def sample_stored_conversation(): """Create a sample StoredConversation for testing.""" return StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -76,10 +87,11 @@ def sample_stored_conversation(): @pytest.fixture -def event_service(sample_stored_conversation): +def event_service(sample_stored_conversation, sample_agent): """Create an EventService instance for testing.""" service = EventService( stored=sample_stored_conversation, + agent=sample_agent, conversations_dir=Path("test_conversation_dir"), ) return service @@ -1820,19 +1832,18 @@ async def test_save_meta_round_trips_agent_definition_mcp_secrets( assert env["TAVILY_API_KEY"].get_secret_value() == "${TAVILY_API_KEY}" @pytest.mark.asyncio - async def test_switch_acp_model_persists_to_meta(self, tmp_path): - """switch_acp_model mirrors the new model into meta.json. + async def test_switch_acp_model_persists_via_conversation(self, tmp_path): + """switch_acp_model delegates to the SDK conversation, which persists the + new model to base_state.json (the single source of truth). - start() rebuilds the runtime agent from meta.json (self.stored.agent), - and ConversationState.create() copies that agent over the persisted - base_state.json on resume. So the switched model must also be written - to meta.json, otherwise a restart silently reverts to the old model. + meta.json no longer carries the agent, so the event service must NOT + mirror the switch there. The SDK ``LocalConversation.switch_acp_model`` + sets ``state.agent`` to an agent carrying the new ``acp_model``, which the + autosave path writes to base_state.json; on resume the agent is rebuilt + from base_state. """ - from openhands.sdk.agent import ACPAgent - stored = StoredConversation( id=uuid4(), - agent=ACPAgent(acp_command=["echo", "test"], acp_model="old-model"), workspace=LocalWorkspace(working_dir=str(tmp_path)), confirmation_policy=NeverConfirm(), initial_message=None, @@ -1842,24 +1853,19 @@ async def test_switch_acp_model_persists_to_meta(self, tmp_path): conv_dir = tmp_path / stored.id.hex conv_dir.mkdir(parents=True, exist_ok=True) - # Stand in for a live conversation; the protocol-level switch is - # covered elsewhere — here we only assert the meta.json mirroring. + # Stand in for a live conversation; the protocol-level switch and the + # base_state persistence are covered by the SDK's own tests — here we + # only assert delegation and that meta.json is not written with an agent. service._conversation = MagicMock() await service.switch_acp_model("new-model") - # Live switch was delegated to the conversation... + # Live switch is delegated to the SDK conversation (which persists to + # base_state.json). service._conversation.switch_acp_model.assert_called_once_with("new-model") - # ...the in-memory stored agent was updated... - assert isinstance(service.stored.agent, ACPAgent) - assert service.stored.agent.acp_model == "new-model" - # ...and the new model was persisted to meta.json so it survives a - # restart. - loaded = StoredConversation.model_validate_json( - (conv_dir / "meta.json").read_text() - ) - assert isinstance(loaded.agent, ACPAgent) - assert loaded.agent.acp_model == "new-model" + # The event service does not write a meta.json agent mirror anymore. + assert not hasattr(service.stored, "agent") + assert not (conv_dir / "meta.json").exists() @pytest.mark.asyncio async def test_switch_acp_model_inactive_service_raises_value_error(self, tmp_path): @@ -1870,11 +1876,9 @@ async def test_switch_acp_model_inactive_service_raises_value_error(self, tmp_pa the first run(), so the only failure mode here is a closed/never-started service. """ - from openhands.sdk.agent import ACPAgent stored = StoredConversation( id=uuid4(), - agent=ACPAgent(acp_command=["echo", "test"], acp_model="old-model"), workspace=LocalWorkspace(working_dir=str(tmp_path)), confirmation_policy=NeverConfirm(), initial_message=None, @@ -3027,10 +3031,6 @@ class TestStatsCallbackNoDeadlock: def _make_service_with_callback(self): stored = StoredConversation( id=uuid4(), - agent=Agent( - llm=LLM(model="gpt-4o", usage_id="test-stats"), - tools=[], - ), workspace=LocalWorkspace(working_dir="workspace/project"), confirmation_policy=NeverConfirm(), initial_message=None, @@ -3040,6 +3040,7 @@ def _make_service_with_callback(self): ) service = EventService( stored=stored, + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-stats"), tools=[]), conversations_dir=Path("test_conversation_dir"), ) # A real FIFOLock on a Mock-ish state so the callback contends on @@ -3367,7 +3368,6 @@ def test_llm_log_callback_swallows_emit_failures( def _make_stored(tmp_path: Path) -> StoredConversation: return StoredConversation( id=uuid4(), - agent=Agent(llm=LLM(model="gpt-4o", usage_id="test"), tools=[]), workspace=LocalWorkspace(working_dir=str(tmp_path)), confirmation_policy=NeverConfirm(), initial_message=None, @@ -3393,6 +3393,7 @@ async def test_event_service_skips_lease_when_ttl_is_zero(tmp_path: Path) -> Non stored = _make_stored(tmp_path) service = EventService( stored=stored, + agent=_sample_agent(), conversations_dir=tmp_path, lease_ttl_seconds=0, ) @@ -3411,6 +3412,7 @@ async def test_event_service_creates_lease_with_custom_ttl(tmp_path: Path) -> No stored = _make_stored(tmp_path) service = EventService( stored=stored, + agent=_sample_agent(), conversations_dir=tmp_path, lease_ttl_seconds=10.0, ) diff --git a/tests/agent_server/test_event_streaming.py b/tests/agent_server/test_event_streaming.py index bfae85edc3..caab441261 100644 --- a/tests/agent_server/test_event_streaming.py +++ b/tests/agent_server/test_event_streaming.py @@ -52,17 +52,17 @@ def event_service(tmp_path): service = EventService( stored=StoredConversation( id=uuid4(), - agent=Agent( - llm=LLM( - usage_id="test-llm", - model="test-model", - api_key=SecretStr("test-key"), - stream=True, - ), - tools=[], - ), workspace=LocalWorkspace(working_dir=str(tmp_path / "workspace")), ), + agent=Agent( + llm=LLM( + usage_id="test-llm", + model="test-model", + api_key=SecretStr("test-key"), + stream=True, + ), + tools=[], + ), conversations_dir=tmp_path / "conversations", ) yield service @@ -192,17 +192,17 @@ async def test_token_callbacks_not_wired_when_stream_disabled(tmp_path): service = EventService( stored=StoredConversation( id=uuid4(), - agent=Agent( - llm=LLM( - usage_id="test-llm", - model="test-model", - api_key=SecretStr("test-key"), - stream=False, - ), - tools=[], - ), workspace=LocalWorkspace(working_dir=str(tmp_path / "workspace")), ), + agent=Agent( + llm=LLM( + usage_id="test-llm", + model="test-model", + api_key=SecretStr("test-key"), + stream=False, + ), + tools=[], + ), conversations_dir=tmp_path / "conversations", ) (tmp_path / "workspace").mkdir(exist_ok=True) @@ -224,9 +224,9 @@ async def test_acp_agents_wire_token_callback_without_llm_streaming(tmp_path): service = EventService( stored=StoredConversation( id=uuid4(), - agent=ACPAgent(acp_command=["echo", "test"]), workspace=LocalWorkspace(working_dir=str(tmp_path / "workspace")), ), + agent=ACPAgent(acp_command=["echo", "test"]), conversations_dir=tmp_path / "conversations", ) (tmp_path / "workspace").mkdir(exist_ok=True) @@ -248,9 +248,9 @@ async def test_acp_string_token_callback_publishes_delta(tmp_path): service = EventService( stored=StoredConversation( id=uuid4(), - agent=ACPAgent(acp_command=["echo", "test"]), workspace=LocalWorkspace(working_dir=str(tmp_path / "workspace")), ), + agent=ACPAgent(acp_command=["echo", "test"]), conversations_dir=tmp_path / "conversations", ) collector = _CollectorSubscriber() diff --git a/tests/agent_server/test_goal_loop.py b/tests/agent_server/test_goal_loop.py index c326ba80aa..942ab8c158 100644 --- a/tests/agent_server/test_goal_loop.py +++ b/tests/agent_server/test_goal_loop.py @@ -75,14 +75,12 @@ def event_service(tmp_path): service = EventService( stored=StoredConversation( id=uuid4(), - agent=Agent( - llm=LLM( - usage_id="agent", model="test-model", api_key=SecretStr("x") - ), - tools=[], - ), workspace=LocalWorkspace(working_dir=str(tmp_path / "workspace")), ), + agent=Agent( + llm=LLM(usage_id="agent", model="test-model", api_key=SecretStr("x")), + tools=[], + ), conversations_dir=tmp_path / "conversations", ) yield service diff --git a/tests/agent_server/test_webhook_subscriber.py b/tests/agent_server/test_webhook_subscriber.py index f3330093cb..2c134e127f 100644 --- a/tests/agent_server/test_webhook_subscriber.py +++ b/tests/agent_server/test_webhook_subscriber.py @@ -46,16 +46,16 @@ def mock_event_service(): service = EventService( stored=StoredConversation( id=uuid4(), - agent=Agent( - llm=LLM( - usage_id="test-llm", - model="test-model", - api_key=SecretStr("test-key"), - ), - tools=[], - ), workspace=LocalWorkspace(working_dir="workspace/project"), ), + agent=Agent( + llm=LLM( + usage_id="test-llm", + model="test-model", + api_key=SecretStr("test-key"), + ), + tools=[], + ), conversations_dir=temp_path / "conversations_dir", ) yield service @@ -1255,7 +1255,7 @@ async def test_post_conversation_info_success( # Create sample conversation info conversation_info = ConversationInfo( id=uuid4(), - agent=mock_event_service.stored.agent, + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=mock_event_service.stored.workspace, created_at=utc_now(), updated_at=utc_now(), @@ -1303,7 +1303,7 @@ async def test_post_conversation_info_with_session_api_key( # Create sample conversation info conversation_info = ConversationInfo( id=uuid4(), - agent=mock_event_service.stored.agent, + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=mock_event_service.stored.workspace, created_at=utc_now(), updated_at=utc_now(), @@ -1344,7 +1344,7 @@ async def test_post_conversation_info_http_error_with_retries( # Create sample conversation info conversation_info = ConversationInfo( id=uuid4(), - agent=mock_event_service.stored.agent, + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), workspace=mock_event_service.stored.workspace, created_at=utc_now(), updated_at=utc_now(), diff --git a/tests/sdk/conversation/test_base_state_single_source.py b/tests/sdk/conversation/test_base_state_single_source.py new file mode 100644 index 0000000000..b257b9af8d --- /dev/null +++ b/tests/sdk/conversation/test_base_state_single_source.py @@ -0,0 +1,81 @@ +"""Regression tests: base_state.json is the single source of truth for the agent. + +These pin the behavior behind the meta.json / base_state.json de-duplication: + +* ``ConversationState.create`` resumes the agent from ``base_state.json`` when the + caller supplies ``agent=None`` — a durable model switch survives a reload. +* Passing an explicit agent on resume keeps the legacy verify-and-override + behavior (back-compat for callers that reconfigure on resume). +""" + +import uuid + +import pytest + +from openhands.sdk import LLM, Agent +from openhands.sdk.conversation.state import ConversationState +from openhands.sdk.io import LocalFileStore +from openhands.sdk.workspace import LocalWorkspace + + +def _agent(model: str, usage_id: str = "default") -> Agent: + return Agent(llm=LLM(model=model, usage_id=usage_id), tools=[]) + + +def test_resume_with_agent_none_keeps_persisted_agent(tmp_path): + """A model change written to base_state.json survives a reload with agent=None.""" + file_store = LocalFileStore(str(tmp_path)) + workspace = LocalWorkspace(working_dir=str(tmp_path)) + + cid = uuid.uuid4() + state = ConversationState.create( + id=cid, + agent=_agent("model-a"), + workspace=workspace, + file_store=file_store, + ) + # Simulate a durable switch: rewrite the agent on base_state.json. + state.agent = _agent("model-b") + assert state.agent.llm.model == "model-b" + + # Reload without supplying an agent -> base_state.json is authoritative. + reloaded = ConversationState.create( + id=cid, + agent=None, + workspace=workspace, + file_store=LocalFileStore(str(tmp_path)), + ) + assert reloaded.agent.llm.model == "model-b" + + +def test_resume_with_explicit_agent_overrides(tmp_path): + """Passing an agent on resume keeps the legacy verify-and-override behavior.""" + file_store = LocalFileStore(str(tmp_path)) + workspace = LocalWorkspace(working_dir=str(tmp_path)) + cid = uuid.uuid4() + + ConversationState.create( + id=cid, + agent=_agent("model-a"), + workspace=workspace, + file_store=file_store, + ) + + reloaded = ConversationState.create( + id=cid, + agent=_agent("model-c"), + workspace=workspace, + file_store=LocalFileStore(str(tmp_path)), + ) + assert reloaded.agent.llm.model == "model-c" + + +def test_new_conversation_requires_agent(tmp_path): + """Creating a brand-new state (no base_state.json) still requires an agent.""" + with pytest.raises(ValueError, match="agent is required"): + ConversationState.create( + id=uuid.uuid4(), + agent=None, + workspace=LocalWorkspace(working_dir=str(tmp_path)), + file_store=LocalFileStore(str(tmp_path)), + )