fix(agent-server): propagate load_memory preference to all launch paths - #4566
fix(agent-server): propagate load_memory preference to all launch paths#4566vnktadithya wants to merge 4 commits into
Conversation
|
📁 PR Artifacts Notice This PR contains a |
There was a problem hiding this comment.
Pull request overview
Fixes propagation of the persisted load_memory preference across all conversation launch paths.
Changes:
- Centralizes memory-preference application after agent resolution.
- Removes the obsolete profile-resolution parameter.
- Adds regression tests and real-store verification.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Review notes |
|---|---|
tests/agent_server/test_agent_profile_conv_start.py |
Adds coverage for direct-agent and agent_settings launches. |
openhands-agent-server/openhands/agent_server/conversation_service.py |
Nit (2 votes): maintainer evaluation and prompt-impact review are requested. |
.pr/verify_load_memory_all_paths.py |
Critical (1 vote): verification can overwrite an existing settings.json; isolate or restore the settings store. |
Suppressed comments (1)
openhands-agent-server/openhands/agent_server/conversation_service.py:1470
FileSettingsStore.load()performs synchronous file I/O and full settings validation, but this call now runs on the event loop for every concrete-agent launch (not just profile launches). A slow or large settings file can therefore block unrelated requests while conversations start; move the load to the worker thread, as the surrounding profile resolution already does.
settings = get_settings_store().load() or PersistedSettings()
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
- 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 OpenHands#4566.
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
HUMAN:
Resolves #4542 - The memory preferences weren't propagating through two of the three existing paths (
agentandagent_settings).AGENT:
Why
agent_settings.agent_context.load_memoryis a global user preference, but it was only ever applied when a conversation launched viaagent_profile_id. Conversations started with a concreteagentor anagent_settingspayload silently lost the setting — no error, no warning, just a missing<MEMORY_CONTEXT>block, whileMemorySectionrendered its other variant so the transcript still looked healthy.Root cause:
_resolve_agent_from_profilewas the only place the stored preference got stamped onto the agent, and that function is only reachable via theagent_profile_idbranch. The other two request shapes (agent, andagent_settings— converted toagentby amode="before"validator before_start_conversationeven runs) never touched it.Summary
agent_profile_idbranch in_start_conversation, so theload_memorystamp applies once, after all three request shapes converge onrequest.agent, instead of only on the profile path._with_load_memory()next to_append_system_message_suffix, using the same two-levelmodel_copypattern (required becauseAgentBaseis frozen).load_memoryparameter from_resolve_agent_from_profile(signature, docstring, and inline body).Issue Number
Fixes #4542
How to Test
Unit tests:
Result: All 39 passed.
New coverage added:
test_direct_agent_launch_inherits_the_stored_memory_preferencetest_direct_agent_launch_leaves_memory_off_without_the_preference(parametrized)test_agent_settings_launch_inherits_the_stored_memory_preferencetest_agent_settings_launch_leaves_memory_off_without_the_preference(parametrized)test_agent_launch_preserves_context_when_load_memory_already_trueExisting
test_profile_launch_inherits_the_stored_memory_preferenceandtest_profile_launch_leaves_memory_off_without_the_preferencepass unchanged — they now exercise the post-branch stamp instead of the in-helper one.Unit tests alone rely on mocked stores, so I also ran an end-to-end pass against a real (non-mocked) settings store and a real
MEMORY.md:Output:
Acceptance criteria (from #4542)
load_memory: truestored, a conversation started with a concreteagenthasagent_context.load_memory is True—test_direct_agent_launch_inherits_the_stored_memory_preferenceagent_settingspayload —test_agent_settings_launch_inherits_the_stored_memory_preferenceagent_profile_id(existing behaviour preserved) —test_profile_launch_inherits_the_stored_memory_preference(unchanged)agent_contextabsent from stored settings, all three paths leaveload_memoryatFalse—test_profile_launch_leaves_memory_off_without_the_preference(unchanged),test_direct_agent_launch_leaves_memory_off_without_the_preferenceandtest_agent_settings_launch_leaves_memory_off_without_the_preferenceload_memory=Trueitself is unaffected —test_agent_launch_preserves_context_when_load_memory_already_true_resolve_agent_from_profileno longer takes aload_memoryparameter — confirmed in the diff (signature, docstring, and body)tests/agent_server/test_agent_profile_conv_start.pypass unchanged — confirmed, all 39 passedVideo/Screenshots
No video — this is a backend-only change with no UI. The script above is the functional evidence: it exercises the real settings store and a real
MEMORY.mdon disk rather than pytest mocks, and its output is included in How to Test section above.Type
Notes
User-facing: the global "load persistent memory" preference now applies to every conversation launch (
agent,agent_settings,agent_profile_id), not only ones started from a named agent profile._with_load_memoryoperates onAgentBasegenerically, so it isn't kind-specific — the existing profile tests already cover theopenhands/acpsplit, and since this fix's logic doesn't branch on agent kind, that split doesn't need re-testing for the new shapes.load_skills_from_agent_server()also discards a caller-suppliedAgentContextoutright — noted in the issue as an optional companion change. Filing that separately rather than bundling it here, since it's an independent code path.