Conversation
…r the agent The agent-server persisted a conversation's agent (LLM + condenser + tools) in TWO files: base_state.json (ConversationState) and meta.json (StoredConversation, which extended StartConversationRequest). On resume the agent was rebuilt from meta.json and overwrote base_state.json, so meta.json silently won. A model switch written to one file but not the other was reverted on an idle-eviction reload. This removes the duplication at its root: - SDK: extract ConversationConfig (everything except the agent) as the shared base. StartConversationRequest adds the agent; StoredConversation now extends the agent-less ConversationConfig, so the agent cannot appear in meta.json by construction. - SDK: ConversationState.create() and LocalConversation accept agent=None; on resume the persisted base_state.json agent is kept (a durable switch_llm/ switch_acp_model survives reload). Passing an explicit agent keeps the legacy verify-and-override behavior for back-compat. - agent-server: EventService takes the new-conversation agent separately and, on resume, loads it from base_state.json. switch_acp_model no longer mirrors the model into meta.json (the SDK persists it to base_state); the credential scrub and codex detection read the agent from base_state / the live conversation; telemetry reads the live agent. Old meta.json files with an 'agent' key still load (unknown keys are ignored), so no migration is needed. Adds regression coverage: base_state-authoritative resume at the SDK level, and an end-to-end check that meta.json has no agent and a fresh service reloads the agent from base_state.json. Note: the ACP/Codex-subscription persistence paths are covered at unit level only; they were not exercised against a live ACP/Codex session. Co-authored-by: smolpaws <engel@enyst.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAgents are no longer stored in ChangesAgent state migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConversationService
participant EventService
participant LocalConversation
participant ConversationState
participant base_state.json
Client->>ConversationService: start or resume conversation
ConversationService->>EventService: pass new agent or no agent
EventService->>LocalConversation: create conversation
LocalConversation->>ConversationState: restore or apply agent
ConversationState->>base_state.json: load persisted agent
ConversationState-->>EventService: active conversation agent
EventService-->>Client: started conversation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Python API breakage checks — ❌ FAILEDResult: ❌ FAILED
Log excerpt (first 1000 characters) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py (1)
321-343: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake resume reject or handle explicit
client_toolssafely.When
LocalConversationresumes withagent=None, the new client-tool handling is skipped and the caller-supplied non-emptyclient_toolsare not registered or copied into the resumed agent. Document this gap and add a test; or run the tool registration/injection againstself._state.agentafter state creation soclient_toolsapply even on resume.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py` around lines 321 - 343, Update LocalConversation resume handling so explicit non-empty client_tools are registered and injected into the resumed self._state.agent, rather than being silently skipped when agent is None. Reuse the existing register_client_tools and duplicate-name filtering behavior, and add a test covering resume with caller-supplied client_tools.
🧹 Nitpick comments (2)
tests/agent_server/test_conversation_service.py (1)
87-119: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a companion test for legacy
meta.json(with anagentkey) compatibility.This test proves a newly started conversation writes an agent-free
meta.json. It does not prove the PR's other explicit claim: that a pre-existingmeta.jsonfile which still contains anagentkey (written by older server versions) loads without error and without resurrecting the agent onStoredConversation.Add a test that writes a
meta.jsonfile by hand with anagentkey present (plus a matchingbase_state.json), then constructs aConversationServiceand callsget_conversation()/search_conversations()to confirm it loads successfully and the agent comes frombase_state.json, not from the staleagentkey inmeta.json.As per coding guidelines, "Write just enough tests to cover edge cases" — this is exactly the kind of edge case (backward-compatible on-disk format) the guideline calls out for coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agent_server/test_conversation_service.py` around lines 87 - 119, Add a companion async test near test_meta_json_has_no_agent_and_reload_uses_base_state that manually creates legacy meta.json containing a stale agent key alongside matching base_state.json, then initializes ConversationService and exercises get_conversation and/or search_conversations. Assert loading succeeds and StoredConversation uses the agent from base_state.json rather than the stale meta.json agent.Source: Coding guidelines
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py (1)
201-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the docstring for the new
agent=Nonesemantics.The
Args:entry foragentstill reads "The agent to use for the conversation." It does not mention thatagent=Nonenow means "resume the persisted agent frombase_state.json," and that a fresh conversation still requires a non-Noneagent (enforced byConversationState.create). Document this so SDK consumers understand the new contract without reading the implementation.📝 Suggested docstring update
Args: - agent: The agent to use for the conversation. + agent: The agent to use for the conversation. Pass ``None`` to + resume with the agent already persisted in ``base_state.json`` + (the single source of truth on resume). Required (non-``None``) + when creating a brand-new conversation. workspace: Working directory for agent operations and tool execution.Also applies to: 237-237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py` at line 201, Update the Args documentation for the agent parameter in the local conversation constructor to state that None resumes the persisted agent from base_state.json, while fresh conversations require a non-None agent as enforced by ConversationState.create.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openhands-agent-server/openhands/agent_server/models.py`:
- Around line 77-87: Update the StoredConversation compatibility behavior:
either configure StoredConversation (or its ConversationConfig base) to ignore
unknown fields so legacy meta.json files containing agent validate successfully,
or remove/replace the docstring claim that legacy compatibility is supported. Do
not restore an agent field.
In `@openhands-sdk/openhands/sdk/conversation/state.py`:
- Line 449: Update the create method docstring around the agent parameter to
document both behaviors: new conversations require an agent, while resume with
agent=None preserves the persisted agent and an explicitly provided agent
restores with that agent. Revise the “New conversation,” “Restored
conversation,” “Args,” and “Raises” sections accordingly, including the
applicable validation conditions.
In `@tests/agent_server/test_agent_profile_conv_start.py`:
- Around line 725-727: Rename the unused first unpacked value from stored to
_stored in both _start_from_profile calls around the affected test cases,
including the second occurrence, while leaving the agent assertions and other
behavior unchanged.
---
Outside diff comments:
In `@openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py`:
- Around line 321-343: Update LocalConversation resume handling so explicit
non-empty client_tools are registered and injected into the resumed
self._state.agent, rather than being silently skipped when agent is None. Reuse
the existing register_client_tools and duplicate-name filtering behavior, and
add a test covering resume with caller-supplied client_tools.
---
Nitpick comments:
In `@openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py`:
- Line 201: Update the Args documentation for the agent parameter in the local
conversation constructor to state that None resumes the persisted agent from
base_state.json, while fresh conversations require a non-None agent as enforced
by ConversationState.create.
In `@tests/agent_server/test_conversation_service.py`:
- Around line 87-119: Add a companion async test near
test_meta_json_has_no_agent_and_reload_uses_base_state that manually creates
legacy meta.json containing a stale agent key alongside matching
base_state.json, then initializes ConversationService and exercises
get_conversation and/or search_conversations. Assert loading succeeds and
StoredConversation uses the agent from base_state.json rather than the stale
meta.json agent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62d04be1-67b0-454b-9a10-c6ac2e2c55a4
📒 Files selected for processing (21)
openhands-agent-server/openhands/agent_server/conversation_service.pyopenhands-agent-server/openhands/agent_server/event_service.pyopenhands-agent-server/openhands/agent_server/models.pyopenhands-sdk/openhands/sdk/conversation/impl/local_conversation.pyopenhands-sdk/openhands/sdk/conversation/request.pyopenhands-sdk/openhands/sdk/conversation/state.pytests/agent_server/telemetry/test_telemetry_disabled_by_default.pytests/agent_server/telemetry/test_telemetry_subscriber.pytests/agent_server/test_agent_launch_additions.pytests/agent_server/test_agent_profile_conv_start.pytests/agent_server/test_auto_title_span_metadata.pytests/agent_server/test_conversation_info_model.pytests/agent_server/test_conversation_service.pytests/agent_server/test_conversation_service_plugin.pytests/agent_server/test_conversation_tags.pytests/agent_server/test_credential_binding.pytests/agent_server/test_event_service.pytests/agent_server/test_event_streaming.pytests/agent_server/test_goal_loop.pytests/agent_server/test_webhook_subscriber.pytests/sdk/conversation/test_base_state_single_source.py
💤 Files with no reviewable changes (3)
- tests/agent_server/test_auto_title_span_metadata.py
- tests/agent_server/test_conversation_info_model.py
- tests/agent_server/telemetry/test_telemetry_disabled_by_default.py
| 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``. | ||
| """ | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- model_config on ConversationConfig / StoredConversation ---"
rg -n "model_config" openhands-sdk/openhands/sdk/conversation/request.py openhands-agent-server/openhands/agent_server/models.py
echo "--- any extra= override in the SDK/agent-server that could apply to these classes via inheritance ---"
rg -n "extra\s*=\s*['\"](forbid|allow|ignore)['\"]" openhands-sdk openhands-agent-server -g '*.py'
echo "--- class hierarchy for ConversationConfig / OpenHandsModel base ---"
rg -n "class ConversationConfig|class OpenHandsModel" -A 5 openhands-sdk -g '*.py'
echo "--- existing tests referencing a legacy meta.json with an agent key ---"
rg -n "meta.json" tests/agent_server/test_conversation_service.py | rg -n "agent"Repository: enyst/agent-sdk
Length of output: 6191
Reject legacy meta.json compatibility or restore an ignored agent field.
StoredConversation inherits ModelConfig(extra="forbid") from ConversationConfig, so StoredConversation.model_validate_json() will reject an old meta.json that still contains an agent key. If the compatibility objective remains, add extra="ignore" on StoredConversation or ConversationConfig; otherwise remove/replace that compatibility claim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openhands-agent-server/openhands/agent_server/models.py` around lines 77 -
87, Update the StoredConversation compatibility behavior: either configure
StoredConversation (or its ConversationConfig base) to ignore unknown fields so
legacy meta.json files containing agent validate successfully, or remove/replace
the docstring claim that legacy compatibility is supported. Do not restore an
agent field.
| cls: type["ConversationState"], | ||
| id: ConversationID, | ||
| agent: AgentBase, | ||
| agent: AgentBase | None, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the create API documentation for agent=None.
agent=None is now valid during resume and retains the persisted agent. The docstring still describes agent as required and always used for restore. Update the “New conversation,” “Restored conversation,” Args, and Raises sections to state the two behaviors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openhands-sdk/openhands/sdk/conversation/state.py` at line 449, Update the
create method docstring around the agent parameter to document both behaviors:
new conversations require an agent, while resume with agent=None preserves the
persisted agent and an explicitly provided agent restores with that agent.
Revise the “New conversation,” “Restored conversation,” “Args,” and “Raises”
sections accordingly, including the applicable validation conditions.
| stored, agent = await _start_from_profile( | ||
| tmp_path, profile, resolved_settings, persisted | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the unused stored conversation value as intentionally unused.
stored is not read after either unpacking operation. Rename it to _stored to satisfy Ruff RUF059 and show that these tests validate only the resolved agent.
Proposed fix
- stored, agent = await _start_from_profile(
+ _stored, agent = await _start_from_profile(
tmp_path, profile, resolved_settings, persisted
)Apply the same change at Line 756.
Also applies to: 756-758
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 725-725: Unpacked variable stored is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/agent_server/test_agent_profile_conv_start.py` around lines 725 - 727,
Rename the unused first unpacked value from stored to _stored in both
_start_from_profile calls around the affected test cases, including the second
occurrence, while leaving the agent assertions and other behavior unchanged.
Source: Linters/SAST tools
…ange Removing the agent field from StoredConversation (it no longer extends StartConversationRequest) is a breaking API change, which the api-breakage check requires a minor version bump for. Co-authored-by: smolpaws <engel@enyst.org>
…ation change" This reverts commit e718eda.
1 participant
What & why
The agent-server persisted a conversation's agent (LLM + condenser + tools) in two files:
base_state.json(ConversationState), andmeta.json(StoredConversation, which extendedStartConversationRequest).On resume the agent was rebuilt from
meta.jsonand assigned over the state loaded frombase_state.json— someta.jsonsilently won. A model switch written to one file but not the other was reverted on an idle-eviction reload. This is the class of bug Neal reported in Slack (#general), and matches the earlier "two sources of truth" analysis.Design writeup (a/b/c alternatives + the resume/reattach edge): https://enyst.github.io/arch/meta-vs-base-state-alternatives.html
Approach (option c): remove the duplication at the root
ConversationConfig(everything except the agent) as a shared base.StartConversationRequestadds the agent;StoredConversationnow extends the agent-lessConversationConfig, so the agent cannot appear inmeta.jsonby construction.ConversationState.create()andLocalConversationacceptagent=None; on resume the persistedbase_state.jsonagent is kept. Passing an explicit agent keeps the legacy verify-and-override behavior (back-compat).EventServicetakes the new-conversation agent separately and, on resume, loads it frombase_state.json.switch_acp_modelno longer mirrors the model intometa.json(the SDK already persists it tobase_state); the credential scrub and codex detection read the agent frombase_state/ the live conversation; telemetry reads the live agent.base_state.jsonis now the single source of truth for the agent.Compatibility
Old
meta.jsonfiles that still contain anagentkey load fine — Pydantic ignores unknown keys — so no migration is needed.Tests
agent=Nonekeeps the persisted agent; a durable switch survives reload), explicit-agent override still works, new conversation still requires an agent.meta.jsonhas noagent, and a freshConversationService(restart) reloads the agent frombase_state.json.tests/agent_serversuite updated to the new API and green (1980 passing locally). Ruff + pyright clean.Reviewer note⚠️
The ACP / Codex-subscription persistence paths (
switch_acp_model, credential scrub, codex detection) are re-homed and covered at unit level only — I could not exercise them against a live ACP/Codex session. Please give those a live check before merge.Co-authored-by: smolpaws engel@enyst.org
Summary by CodeRabbit