From a845f321ab6e523a205289efd1e88f7c02fe0997 Mon Sep 17 00:00:00 2001 From: adithya Date: Fri, 21 Aug 2026 06:44:27 +0530 Subject: [PATCH 1/3] fix(agent-server): propagate load_memory preference to all launch paths Fixes #4542 --- .pr/verify_load_memory_all_paths.py | 141 ++++++++++++ .../agent_server/conversation_service.py | 65 +++--- .../test_agent_profile_conv_start.py | 207 +++++++++++++++++- 3 files changed, 382 insertions(+), 31 deletions(-) create mode 100644 .pr/verify_load_memory_all_paths.py diff --git a/.pr/verify_load_memory_all_paths.py b/.pr/verify_load_memory_all_paths.py new file mode 100644 index 0000000000..0daf8b6cab --- /dev/null +++ b/.pr/verify_load_memory_all_paths.py @@ -0,0 +1,141 @@ +"""Manual, less-mocked verification for #4542. + +Proves, against real file I/O instead of pytest mocks: + Part A - load_memory() actually reads a real MEMORY.md from disk. + Part B - a real, on-disk load_memory=True preference reaches the launched + agent for both previously-broken shapes, `agent` and + `agent_settings`. `agent_profile_id` is not re-checked here - + that path was already fixed by #4223 and is covered by the + existing pytest suite. + +Run with: uv run python .pr/verify_load_memory_all_paths.py +Requires OH_PERSISTENCE_DIR set first - without it this would write to your +real ~/.openhands directory. +""" + +import asyncio +import os +import tempfile +from datetime import UTC, datetime +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import uuid4 + +from openhands.agent_server.conversation_service import ConversationService +from openhands.agent_server.event_service import EventService +from openhands.agent_server.models import StartConversationRequest +from openhands.agent_server.persistence import ( + PersistedSettings, + get_settings_store, + reset_stores, +) +from openhands.sdk import LLM, Agent, AgentContext +from openhands.sdk.context.memory import load_memory +from openhands.sdk.conversation.state import ( + ConversationExecutionStatus, + ConversationState, +) +from openhands.sdk.settings.model import OpenHandsAgentSettings +from openhands.sdk.workspace import LocalWorkspace + + +def make_agent() -> Agent: + return Agent(llm=LLM(model="gpt-4o", usage_id="llm"), tools=[]) + + +async def launch_and_get_agent(tmp_path: Path, **request_kwargs): + """Launch through the real ConversationService; stub only the expensive + last-mile (actual agent execution / LLM call), never the settings store.""" + request = StartConversationRequest( + workspace=LocalWorkspace(working_dir=str(tmp_path)), + **request_kwargs, + ) + captured = {} + + async def capture_start(_stored, **kwargs): + agent = kwargs["agent"] + captured["agent"] = agent + es = AsyncMock(spec=EventService) + es.get_state.return_value = ConversationState( + id=uuid4(), + agent=agent, + workspace=request.workspace, + execution_status=ConversationExecutionStatus.IDLE, + ) + es.stored = MagicMock( + launched_agent_profile=None, + client_tools=[], + title=None, + metrics=None, + created_at=datetime.now(UTC), + updated_at=datetime.now(UTC), + forked_from_conversation_id=None, + forked_from_event_id=None, + parent_conversation_id=None, + ) + return es + + service = ConversationService(conversations_dir=tmp_path) + service._event_services = {} + with patch.object( + service, + "_start_event_service", + new_callable=AsyncMock, + side_effect=capture_start, + ): + await service.start_conversation(request) + return captured["agent"] + + +async def main(): + persistence_dir = Path(os.environ["OH_PERSISTENCE_DIR"]) + reset_stores() # safety net; harmless if there was nothing to reset + get_settings_store().save( + PersistedSettings( + agent_settings=OpenHandsAgentSettings( + agent_context=AgentContext(load_memory=True) + ) + ) + ) + print( + f"Persisted load_memory=True for real at {persistence_dir / 'settings.json'}\n" + ) + + with tempfile.TemporaryDirectory() as workspace_dir: + workspace = Path(workspace_dir) + memory_dir = workspace / ".openhands" / "memory" + memory_dir.mkdir(parents=True) + (memory_dir / "MEMORY.md").write_text( + "# Verification note\n" + "load_memory propagated correctly if you can read this.\n" + ) + + print("=== Part A: does load_memory() read the file back? ===") + content = load_memory(workspace) + print(content) + assert content is not None and "propagated correctly" in content + print("PASS\n") + + print("=== Part B: does the real settings store reach the launched agent? ===") + for label, kwargs in [ + ("agent", {"agent": make_agent()}), + ( + "agent_settings", + { + "agent_settings": { + "agent_kind": "openhands", + "llm": {"model": "gpt-4o", "usage_id": "llm"}, + } + }, + ), + ]: + agent = await launch_and_get_agent(workspace, **kwargs) + ok = agent.agent_context is not None and agent.agent_context.load_memory + print(f"{label:15s} -> agent_context.load_memory = {ok}") + assert ok, f"{label} did not inherit the persisted preference" + + print("\nAll checks passed.") + + +if __name__ == "__main__": + asyncio.run(main()) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index 32f9e30f59..e49c043d44 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -129,6 +129,19 @@ def _append_system_message_suffix(agent: AgentBase, addition: str) -> AgentBase: return agent.model_copy(update={"agent_context": updated_context}) +def _with_load_memory(agent: AgentBase) -> AgentBase: + """Stamp the global persistent-memory preference onto an agent. + + ``load_memory`` is a user-level setting, not part of any agent, profile or + client payload, so it is applied here regardless of how the agent reached + the request. + """ + context = agent.agent_context or AgentContext() + return agent.model_copy( + update={"agent_context": context.model_copy(update={"load_memory": True})} + ) + + def _has_git_remote(repo_root: Path, remote: str = "origin") -> bool: try: run_git_command(["git", "remote", "get-url", remote], repo_root) @@ -293,7 +306,6 @@ def _resolve_agent_from_profile( profile_id: "UUID", cipher: "Cipher | None", mcp_config: "dict[str, MCPServer]", - load_memory: bool = False, ) -> "tuple[AgentBase, LaunchedAgentProfile]": """Load and resolve an agent profile by id, returning the built agent + provenance. @@ -304,11 +316,6 @@ def _resolve_agent_from_profile( server's cipher. Passed explicitly so this free function never touches the settings-store singleton (which may not have been initialised with the correct cipher yet). - load_memory: The user's global persistent-memory preference - (``agent_settings.agent_context.load_memory``). An ``AgentProfile`` - has no ``agent_context`` field, so the preference cannot ride the - profile — it is stamped onto the resolved agent below, else a - profile-launched conversation would silently ignore the setting. Raises: ProfileNotFound: No stored profile has ``profile_id``. @@ -385,15 +392,7 @@ def _resolve_agent_from_profile( agent = agent.model_copy( update={"tools": [*agent.tools, Tool(name=BROWSER_TOOL_NAME)]} ) - # Persistent memory is a global user preference, not a profile field, so it - # is carried across the profile-resolution boundary the same way the global - # ``mcp_config`` is. Left untouched when off, so the resolved agent stays - # byte-identical for everyone who hasn't opted in. - if load_memory: - context = agent.agent_context or AgentContext() - agent = agent.model_copy( - update={"agent_context": context.model_copy(update={"load_memory": True})} - ) + launched = LaunchedAgentProfile( agent_profile_id=profile.id, revision=profile.revision, @@ -1456,31 +1455,39 @@ async def _start_conversation( f"to a different workspace" ) - # Profile resolution must happen before _prepare_request_workspace (which - # asserts request.agent is not None) and before model_dump so the resolved - # agent is captured in request_data. + # Profile resolution and the load_memory stamp must happen before + # _prepare_request_workspace (which asserts request.agent is not None) + # and before model_dump so the resolved agent is captured in request_data. launched_agent_profile: LaunchedAgentProfile | None = None - if request.agent_profile_id is not None: - # get_settings_store() is safe here: get_instance() initialises the - # singleton with the server cipher before any conversation can start. - from openhands.agent_server.persistence import ( - PersistedSettings, - get_settings_store, - ) - settings = get_settings_store().load() or PersistedSettings() + from openhands.agent_server.persistence import ( + PersistedSettings, + get_settings_store, + ) + + # get_settings_store() is safe here: get_instance() initialises the + # singleton with the server cipher before any conversation can start. + settings = get_settings_store().load() or PersistedSettings() + + # ``ACPAgentSettings.agent_context`` is nullable, hence the guard. + stored_context = settings.agent_settings.agent_context + load_memory = bool(stored_context and stored_context.load_memory) + + if request.agent_profile_id is not None: mcp_config = settings.agent_settings.mcp_config - # ``ACPAgentSettings.agent_context`` is nullable, hence the guard. - stored_context = settings.agent_settings.agent_context resolved_agent, launched_agent_profile = await asyncio.to_thread( _resolve_agent_from_profile, request.agent_profile_id, self.cipher, mcp_config, - load_memory=bool(stored_context and stored_context.load_memory), ) request = request.model_copy(update={"agent": resolved_agent}) + if load_memory and request.agent is not None: + request = request.model_copy( + update={"agent": _with_load_memory(request.agent)} + ) + additions = request.agent_launch_additions suffix = ( additions.system_message_suffix_append.strip() diff --git a/tests/agent_server/test_agent_profile_conv_start.py b/tests/agent_server/test_agent_profile_conv_start.py index d079bd4a32..c079cd7dae 100644 --- a/tests/agent_server/test_agent_profile_conv_start.py +++ b/tests/agent_server/test_agent_profile_conv_start.py @@ -10,7 +10,7 @@ from __future__ import annotations from datetime import UTC, datetime -from typing import Any +from typing import Any, cast from unittest.mock import AsyncMock, MagicMock, patch from uuid import UUID, uuid4 @@ -31,7 +31,7 @@ StoredConversation, ) from openhands.agent_server.persistence import PersistedSettings -from openhands.sdk import LLM, Agent, AgentContext +from openhands.sdk import LLM, Agent, AgentBase, AgentContext from openhands.sdk.conversation.state import ( ConversationExecutionStatus, ConversationState, @@ -603,6 +603,67 @@ async def capture_start(stored, **kwargs): return captured["stored"], captured["agent"] +async def _start_with_agent( + tmp_path, + persisted_settings: PersistedSettings, + *, + agent: Agent | None = None, + agent_settings: dict[str, Any] | None = None, +) -> Any: + """Launch via a concrete ``agent`` or a raw ``agent_settings`` payload and + return the captured agent. + + Neither shape touches profile resolution, so unlike ``_start_from_profile`` + only the settings-store read that feeds ``load_memory`` needs stubbing. + """ + request = StartConversationRequest( + agent=cast(AgentBase, agent), + agent_settings=agent_settings, + workspace=LocalWorkspace(working_dir=str(tmp_path)), + ) + captured: dict[str, Any] = {} + + async def capture_start(stored, **kwargs): + launched_agent = kwargs["agent"] + captured["agent"] = launched_agent + event_service = AsyncMock(spec=EventService) + event_service.get_state.return_value = ConversationState( + id=uuid4(), + agent=launched_agent, + workspace=request.workspace, + execution_status=ConversationExecutionStatus.IDLE, + ) + event_service.stored = MagicMock( + launched_agent_profile=None, + client_tools=[], + title=None, + metrics=None, + created_at=datetime.now(UTC), + updated_at=datetime.now(UTC), + forked_from_conversation_id=None, + forked_from_event_id=None, + parent_conversation_id=None, + ) + return event_service + + service = ConversationService(conversations_dir=tmp_path) + service._event_services = {} + + with ( + patch(_SETTINGS_STORE_PATH) as MockSettingsStore, + patch.object( + service, + "_start_event_service", + new_callable=AsyncMock, + side_effect=capture_start, + ), + ): + MockSettingsStore.return_value.load.return_value = persisted_settings + await service.start_conversation(request) + + return captured["agent"] + + class TestConversationServiceStartFromProfile: @pytest.mark.asyncio async def test_start_from_profile_stamps_launched_agent_profile_on_stored( @@ -761,6 +822,148 @@ async def test_profile_launch_leaves_memory_off_without_the_preference( assert agent.agent_context.load_memory is False +class TestConversationServiceStartWithDirectAgent: + @pytest.mark.asyncio + async def test_direct_agent_launch_inherits_the_stored_memory_preference( + self, tmp_path + ): + """Same guarantee as the profile launch, for the ``agent`` shape. + + ``request.agent`` is already set when a client sends ``agent`` + directly, so this path never touched ``_resolve_agent_from_profile``'s + stamp and silently dropped the global preference before this fix. + """ + persisted = PersistedSettings( + agent_settings=OpenHandsAgentSettings( + agent_context=AgentContext(load_memory=True) + ) + ) + + agent = await _start_with_agent(tmp_path, persisted, agent=_make_agent()) + + assert agent.agent_context is not None + assert agent.agent_context.load_memory is True + + @pytest.mark.parametrize( + "persisted_settings", + [ + pytest.param( + PersistedSettings( + agent_settings=OpenHandsAgentSettings(agent_context=AgentContext()) + ), + id="preference-off", + ), + pytest.param( + PersistedSettings(agent_settings=ACPAgentSettings()), + id="stored-settings-without-agent-context", + ), + ], + ) + @pytest.mark.asyncio + async def test_direct_agent_launch_leaves_memory_off_without_the_preference( + self, tmp_path, persisted_settings + ): + agent = await _start_with_agent( + tmp_path, persisted_settings, agent=_make_agent() + ) + + # A directly-constructed Agent has no AgentContext by default, so "off" + # surfaces as agent_context staying None rather than an explicit + # AgentContext(load_memory=False) — both mean the same thing to the + # runtime (see AgentBase's own check: agent_context is not None and + # agent_context.load_memory). + effective_load_memory = ( + agent.agent_context is not None and agent.agent_context.load_memory + ) + assert effective_load_memory is False + + @pytest.mark.asyncio + async def test_agent_settings_launch_inherits_the_stored_memory_preference( + self, tmp_path + ): + """Locks in the third shape: ``_populate_agent_from_settings`` converts + this to ``request.agent`` before ``_start_conversation`` even runs, so + it needs the same coverage as the ``agent`` shape above, not just the + two paths that were already tested pre-fix. + """ + persisted = PersistedSettings( + agent_settings=OpenHandsAgentSettings( + agent_context=AgentContext(load_memory=True) + ) + ) + + agent = await _start_with_agent( + tmp_path, + persisted, + agent_settings={ + "agent_kind": "openhands", + "llm": {"model": "gpt-4o", "usage_id": "llm"}, + }, + ) + + assert agent.agent_context is not None + assert agent.agent_context.load_memory is True + + @pytest.mark.parametrize( + "persisted_settings", + [ + pytest.param( + PersistedSettings( + agent_settings=OpenHandsAgentSettings(agent_context=AgentContext()) + ), + id="preference-off", + ), + pytest.param( + PersistedSettings(agent_settings=ACPAgentSettings()), + id="stored-settings-without-agent-context", + ), + ], + ) + @pytest.mark.asyncio + async def test_agent_settings_launch_leaves_memory_off_without_the_preference( + self, tmp_path, persisted_settings + ): + agent = await _start_with_agent( + tmp_path, + persisted_settings, + agent_settings={ + "agent_kind": "openhands", + "llm": {"model": "gpt-4o", "usage_id": "llm"}, + }, + ) + + assert agent.agent_context is not None + assert agent.agent_context.load_memory is False + + @pytest.mark.asyncio + async def test_agent_launch_preserves_context_when_load_memory_already_true( + self, tmp_path + ): + """The stamp must update load_memory in place, not replace the whole + AgentContext — a client who already opted in keeps every other field + they set (skills, suffix, etc.) untouched.""" + agent_with_context = Agent( + llm=LLM(model="gpt-4o", usage_id="llm"), + tools=[], + agent_context=AgentContext( + load_memory=True, + skills=[], + system_message_suffix="client-set suffix", + ), + ) + persisted = PersistedSettings( + agent_settings=OpenHandsAgentSettings( + agent_context=AgentContext(load_memory=True) + ) + ) + + agent = await _start_with_agent(tmp_path, persisted, agent=agent_with_context) + + assert agent.agent_context is not None + assert agent.agent_context.load_memory is True + assert agent.agent_context.system_message_suffix == "client-set suffix" + + # --------------------------------------------------------------------------- # Router-layer: HTTP error mapping # --------------------------------------------------------------------------- From cbf89fa4b7a18d03668347ed2604f26d6c0dd924 Mon Sep 17 00:00:00 2001 From: adithya Date: Fri, 21 Aug 2026 09:57:05 +0530 Subject: [PATCH 2/3] fix(agent-server): thread settings load, isolate verification script - Wrap the settings-store read in asyncio.to_thread so it no longer blocks the event loop on every launch, not just profile launches. - .pr/verify_load_memory_all_paths.py now creates and owns its own temporary OH_PERSISTENCE_DIR instead of depending on the caller to isolate it, so running it can no longer overwrite a real settings.json. Addresses Copilot review feedback on #4566. --- .pr/verify_load_memory_all_paths.py | 22 ++++++++++--------- .../agent_server/conversation_service.py | 4 +++- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/.pr/verify_load_memory_all_paths.py b/.pr/verify_load_memory_all_paths.py index 0daf8b6cab..a069225d7f 100644 --- a/.pr/verify_load_memory_all_paths.py +++ b/.pr/verify_load_memory_all_paths.py @@ -88,18 +88,20 @@ async def capture_start(_stored, **kwargs): async def main(): - persistence_dir = Path(os.environ["OH_PERSISTENCE_DIR"]) - reset_stores() # safety net; harmless if there was nothing to reset - get_settings_store().save( - PersistedSettings( - agent_settings=OpenHandsAgentSettings( - agent_context=AgentContext(load_memory=True) + with tempfile.TemporaryDirectory() as settings_dir: + os.environ["OH_PERSISTENCE_DIR"] = settings_dir + reset_stores() + get_settings_store().save( + PersistedSettings( + agent_settings=OpenHandsAgentSettings( + agent_context=AgentContext(load_memory=True) + ) ) ) - ) - print( - f"Persisted load_memory=True for real at {persistence_dir / 'settings.json'}\n" - ) + print( + f"Persisted load_memory=True for real at " + f"{Path(settings_dir) / 'settings.json'} (throwaway temp dir)\n" + ) with tempfile.TemporaryDirectory() as workspace_dir: workspace = Path(workspace_dir) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index e49c043d44..5db04b94e1 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -1467,7 +1467,9 @@ async def _start_conversation( # get_settings_store() is safe here: get_instance() initialises the # singleton with the server cipher before any conversation can start. - settings = get_settings_store().load() or PersistedSettings() + settings = await asyncio.to_thread( + lambda: get_settings_store().load() or PersistedSettings() + ) # ``ACPAgentSettings.agent_context`` is nullable, hence the guard. stored_context = settings.agent_settings.agent_context From 9e02afd382d02f861bae4cd1d0748e6304bcca20 Mon Sep 17 00:00:00 2001 From: adithya Date: Fri, 21 Aug 2026 10:31:49 +0530 Subject: [PATCH 3/3] chore: retrigger CI checks