fix(acp): honor server-reported model id; keep cleared id cleared across restarts#3409
fix(acp): honor server-reported model id; keep cleared id cleared across restarts#3409simonrosenberg wants to merge 1 commit into
Conversation
…across restarts Follow-up to #3407, addressing two correctness holes flagged in review after merge. Both let ConversationInfo.current_model_id advertise a model the live ACP server isn't actually running. 1. Cold read re-advertises a cleared override after an agent-server restart. The previous `_initialized` gate on the `acp_model` fallback doesn't survive a restart: a cold read reconstructs the agent with `acp_model` set and PrivateAttrs reset (`_initialized=False`), so for a conversation whose override `init_state` had deliberately cleared from `acp_current_model_id`, the fallback resurrected the model. Gate the fallback on NO persisted `acp_session_id` instead — once a session exists, the persisted `acp_current_model_id` is authoritative (present = ran, absent = cleared), so a cleared id stays cleared across restarts. The `acp_model` fallback now applies only pre-session, where it's the only hint. 2. `_meta` path reports the requested id, ignoring the server's reported id. For providers that select the initial model via session `_meta` (claude), `new_session` returns `models.currentModelId` *after* applying the override, so the reported id is authoritative. Real claude-agent-acp@0.30.0 reports `currentModelId="default"` even when a specific model (e.g. `claude-opus-4-7`) is requested — and both are distinct picker entries — so surfacing the requested id mismatched the server and the picker. Prefer `reported_model_id` on the `_meta` path (fall back to `acp_model` only when the server omitted model state). The `set_session_model` path (codex/gemini fresh + any resume reapply) still prefers `acp_model`, since that call happens after the response and the reported id predates the switch. Tests: cold-read-after-restart (cleared stays cleared; applied survives), `_meta`-path prefers-server-id (mirrors the real claude-acp behaviour) and omitted-models fallback. The three bug-path tests fail on the pre-fix code. 259 passed; ruff + pyright clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Solid follow-up fix. Both bugs are real and were correctly diagnosed.
conversation_service.py — Switching the gate from _initialized (a PrivateAttr that resets on cold restart) to has_acp_session (the persisted acp_session_id) is the right solution. Once a session exists, acp_current_model_id is authoritative by invariant: present = model the server ran, absent = override was cleared. Keying on the persisted session ID rather than the runtime flag is the only way this invariant survives agent-server restarts.
acp_agent.py — The three-way split (applied_via_meta / applied_via_call / neither) correctly maps to three semantically distinct scenarios. The core insight is captured clearly: new_session on the _meta path returns the server's post-override currentModelId in the same response (making it authoritative), whereas set_session_model fires after new_session/load_session (making reported_model_id stale and acp_model the right value to surface). The comment block explaining all three cases is clear and accurate.
Tests — Five targeted tests are added, three of which are confirmed to fail on the pre-fix code. Renaming test_cold_read_falls_back_to_acp_model_override → test_presession_cold_read_falls_back_to_acp_model_override and updating test_live_agent_does_not_fall_back_to_unapplied_override to use acp_session_id instead of _initialized correctly tracks the semantic shift. test_cold_read_after_restart_keeps_cleared_override_cleared directly reproduces the restart-after-cleared-override scenario, and its counterpart test_cold_read_after_restart_surfaces_persisted_applied_model guards the non-regression case. The two _meta-path tests mirror the documented real-world claude-agent-acp@0.30.0 behaviour.
No blocking issues. Logic is sound and test coverage is proportionate to the risk surface.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Verified the ACP model-id fixes with a real local ACP subprocess and conversation-info composition; both reported bug paths change from incorrect advertised IDs on main to correct IDs on the PR.
Does this PR achieve its stated goal?
Yes. The PR sets ConversationInfo.current_model_id to the ACP server-reported model on the _meta path (default), instead of the requested override (claude-opus-4-7), and preserves a deliberately cleared model id across a restart/cold read. I also verified side behavior remains intact: pre-session reads still fall back to acp_model, persisted applied ids still surface, and _meta sessions with no server model state still fall back to the requested id.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv sync --dev completed earlier; verification scripts ran through uv run |
| CI Status | ✅ PR checks observed green/skipped, with this QA check still in progress while running |
| Functional Verification | ✅ Reproduced both bugs on origin/main; PR commit fixes both and keeps fallback behavior |
Functional Verification
Test 1: _meta ACP initialization honors server-reported model id
I used a deterministic local ACP subprocess (/tmp/fake_acp_server.py) that speaks the ACP JSON-RPC methods used by ACPAgent.init_state(). It identifies as claude-agent-acp, receives the requested model via session _meta, and returns models.currentModelId = "default" with claude-opus-4-7 also listed in availableModels.
Step 1 — Reproduce on base (origin/main, fdc2bdf0):
Ran git checkout --quiet origin/main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_model_id.py:
{
"cold_reads": {
"cold_read_after_restart_with_cleared_id": "model-x",
"cold_read_after_restart_with_persisted_id": "model-x",
"pre_session_cold_read_fallback": "model-x"
},
"meta_path": {
"agent_current_model_id": "claude-opus-4-7",
"available_model_ids": ["default", "sonnet", "haiku", "claude-opus-4-7"],
"new_session_meta": {"claudeCode": {"options": {"model": "claude-opus-4-7"}}},
"persisted_current_model_id": "claude-opus-4-7",
"set_session_model_calls": []
}
}This confirms the bug: the server reported default, but the SDK surfaced/persisted the requested claude-opus-4-7 on the _meta path; no session/set_model call occurred after new_session, matching the claude provider path.
Step 2 — Apply the PR changes:
Checked out PR commit 37ba0bef8b28fd089484d52e68678eea0ec15aca.
Step 3 — Re-run with the fix in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_model_id.py:
{
"meta_path": {
"agent_current_model_id": "default",
"available_model_ids": ["default", "sonnet", "haiku", "claude-opus-4-7"],
"new_session_meta": {"claudeCode": {"options": {"model": "claude-opus-4-7"}}},
"persisted_current_model_id": "default",
"set_session_model_calls": []
},
"meta_path_omitted_models": {
"agent_current_model_id": "claude-opus-4-7",
"available_model_ids": [],
"persisted_current_model_id": "claude-opus-4-7",
"set_session_model_calls": []
}
}This confirms the fix: when the server reports default, the live agent and persisted state both use default; when the server omits model state, the requested id is still used as the fallback.
Test 2: Cold read after restart keeps a cleared ACP model id cleared
The script constructs conversation state the way a cold list/read would see it after restart: the serialized ACPAgent still has acp_model="model-x", but agent_state contains a prior acp_session_id and intentionally lacks acp_current_model_id because initialization cleared a rejected/unapplied override.
Step 1 — Reproduce on base (origin/main, fdc2bdf0):
Same base run as above returned:
{
"cold_reads": {
"cold_read_after_restart_with_cleared_id": "model-x",
"cold_read_after_restart_with_persisted_id": "model-x",
"pre_session_cold_read_fallback": "model-x"
}
}This confirms the bug: after a prior session exists and the current id was cleared, the cold read resurrects acp_model (model-x) and would advertise a model the live ACP server was not running.
Step 2 — Apply the PR changes:
Checked out PR commit 37ba0bef8b28fd089484d52e68678eea0ec15aca.
Step 3 — Re-run with the fix in place:
Same PR run returned:
{
"cold_reads": {
"cold_read_after_restart_with_cleared_id": null,
"cold_read_after_restart_with_persisted_id": "model-x",
"pre_session_cold_read_fallback": "model-x"
}
}This confirms the fix and guards the intended side cases: a cleared id stays cleared once a session exists, an applied persisted id still surfaces, and a pre-session cold read still falls back to the serialized acp_model.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
3 participants
Summary
Follow-up to #3407. Its post-merge review (review) raised two 🟠 Important correctness holes that I verified by running locally against the merged code. Both let
ConversationInfo.current_model_id(the picker/chip) advertise a model the live ACP server isn't running.1. Cold read re-advertises a cleared override after an agent-server restart
The
_initializedgate on theacp_modelfallback (added in #3407) doesn't survive a restart. On a cold list read the agent is reconstructed withacp_modelset and PrivateAttrs reset (_initialized=False), so for a conversation whose overrideinit_statehad deliberately cleared fromacp_current_model_id(override rejected/unapplied), the fallback resurrected it.Validated (merged code): persisted
acp_session_id="prior-sess",acp_current_model_idcleared, agentacp_model="model-x",_initialized=False→current_model_idreturned"model-x"❌.Fix: gate the fallback on no persisted
acp_session_id(pre-session). Once a session exists,acp_current_model_idis authoritative (present = ran, absent = cleared), so a cleared id stays cleared across restarts. The persisted-id and live-attr fallbacks are unchanged.2.
_metapath reports the requested id, ignoring the server's reported idFor providers that select the initial model via session
_meta(claude),new_sessionreturnsmodels.currentModelIdafter applying the override, so the reported id is authoritative.Validated with the SDK-pinned
claude-agent-acp@0.30.0: requestingacp_model="claude-opus-4-7"(which is even in the picker) yields servercurrentModelId="default", while the merged code reported"claude-opus-4-7"❌ — a mismatch on the default config with a normal request, and both are distinct entries inavailable_models.Fix: on the
_metapath, preferreported_model_id(fall back toacp_modelonly when the server omitted model state). Theset_session_modelpath (codex/gemini fresh + any resume reapply) still prefersacp_model, since that call happens after the response and the reported id predates the switch.Verification
claude-agent-acp@0.30.0and a deterministic_compose_conversation_inforepro to confirm both bugs on mergedmainbefore fixing._meta-path prefers-server-id (mirrors the real claude behaviour) and omitted-models fallback. The three bug-path tests fail on the pre-fix code; two non-regression guards pass on both.tests/sdk/agent/test_acp_agent.py+tests/agent_server/test_conversation_info_model.py: 259 passed.ruff/pyrightclean.🤖 Generated with Claude Code
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:37ba0be-pythonRun
All tags pushed for this build
About Multi-Architecture Support
37ba0be-python) is a multi-arch manifest supporting both amd64 and arm6437ba0be-python-amd64) are also available if needed