Repository navigation
fix(acp): persist pre-session model switch instead of raising - #3764
Conversation
A model switch on an ACP conversation before the first run() was silently
lost: LocalConversation.switch_acp_model called set_acp_model() first, which
raises RuntimeError when no live session exists yet, so the persist block
never ran and the first run kept the construction-time acp_model.
Branch switch_acp_model on a new public predicate, ACPAgent.has_live_acp_session
(conn + session id + executor all wired). With a live session, behavior is
unchanged (live session/set_model + release_runtime). Without one, skip the
live call and just persist the model_copy + agent_state hint; session creation
on the first run() already honors acp_model (_maybe_set_session_model for
codex/gemini, session/new _meta for Claude), so the first turn runs the
switched model. The deferred value survives a restart via base_state.json.
Agent-server: switch_acp_model no longer maps to 409 ("ACP session not
initialized yet") — the SDK no longer raises for the pre-session case, so it
returns 200. event_service.switch_acp_model now raises the shared
inactive_service ValueError (→400) for a closed/never-started service,
matching its sibling methods.
Fixes #3763
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 QA Report: PARTIAL
Verified the core pre-session ACP model-switch bug fix through SDK calls and real local HTTP API requests; the provider subprocess first-turn check could not complete because codex-acp is not installed in this environment.
Does this PR achieve its stated goal?
Yes for the core goal I could exercise: a pre-run() ACP model switch now persists instead of raising/dropping the requested model, and the agent-server route returns 200 with persisted state updated. On origin/main, the same SDK path raised RuntimeError and left acp_model/agent_state unchanged, while the HTTP endpoint returned 409; on the PR commit, the SDK path returned ok, persisted/reloaded model-b, and the HTTP endpoint returned 200 with current_model_id, agent.acp_model, and agent_state all updated to model-b. A full real ACP provider first-turn run was attempted but blocked by the missing codex-acp executable, so that provider-level end-to-end assertion remains unverified here.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv run created the project environment; SDK and agent-server imports worked; local agent-server /server_info returned 200. |
| CI Status | 🟡 At refresh, core checks such as sdk-tests, agent-server-tests, REST API/OpenAPI, pre-commit, and package checks were passing; image build jobs and this QA job were still pending. |
| Functional Verification | 🟡 SDK persistence and real HTTP route behavior verified before/after; full ACP subprocess run blocked by missing provider binary. |
Functional Verification
Test 1: SDK pre-session switch no longer raises and persists the model
Step 1 — Reproduce baseline without the fix (origin/main):
Ran a short SDK script that constructs an ACPAgent(acp_command=["codex-acp"], acp_model="model-a"), creates a LocalConversation, and calls conversation.switch_acp_model("model-b") before any run().
Observed:
initial_acp_model=model-a
has_live_session=property-missing
switch_result=raised:RuntimeError:ACP session is not initialized; the model can only be switched after the conversation has started (first run()).
current_acp_model=model-a
persisted_acp_model=model-a
persisted_agent_state={}
This confirms the original bug: the pre-session switch raises before persistence, so both live and persisted conversation state remain on model-a.
Step 2 — Apply the PR changes:
Checked out commit 7300512e97108a3c2217d6ebb059abf8663231ba.
Step 3 — Re-run with the fix in place:
Ran the same SDK scenario.
Observed:
initial_acp_model=model-a
has_live_session=False
switch_result=ok
current_acp_model=model-b
persisted_acp_model=model-b
persisted_llm_model=model-b
persisted_agent_state={'acp_current_model_id': 'model-b'}
This shows the exception is gone, the active conversation agent moves to model-b, base_state.json reloads with acp_model=model-b, and the cold-read chip/picker hint is persisted.
Test 2: Real agent-server HTTP endpoint returns 200 and persists state pre-run
Step 1 — Reproduce baseline without the fix (origin/main):
Started a local agent-server with uv run agent-server --host 127.0.0.1 --port 8765, created an ACP conversation via POST /api/conversations, then called POST /api/conversations/{id}/switch_acp_model before any run.
Observed:
POST /api/conversations -> HTTP_STATUS:201
POST /api/conversations/77284b3a-41d6-4d5f-93c6-a2c484ecf927/switch_acp_model
{"detail":"ACP session is not initialized; the model can only be switched after the conversation has started (first run())."}
HTTP_STATUS:409
current_model_id=model-a
agent_acp_model=model-a
agent_state={}
This confirms the old server behavior exposed the SDK failure as a 409 and did not persist the requested model.
Step 2 — Apply the PR changes:
Checked out commit 7300512e97108a3c2217d6ebb059abf8663231ba and restarted the local agent-server.
Step 3 — Re-run with the fix in place:
Created a fresh ACP conversation over HTTP and called the same switch endpoint before any run.
Observed:
POST /api/conversations -> HTTP_STATUS:201
POST /api/conversations/253e9178-11f6-4bcf-aa7e-c451034f1909/switch_acp_model
{"success":true}
HTTP_STATUS:200
current_model_id=model-b
agent_acp_model=model-b
agent_state={'acp_current_model_id': 'model-b'}
execution_status=idle
This verifies the user-facing API behavior promised by the PR: a pre-session ACP model switch now returns success and the visible/persisted conversation state reflects the selected model while the conversation remains idle.
Unable to Verify
I attempted a real first conversation.run() after a pre-session switch using acp_command=["codex-acp"] and max_iterations=1, but this environment does not have the provider binary installed:
Failed to start ACP server: [Errno 2] No such file or directory: 'codex-acp'
run_result=raised:FileNotFoundError:[Errno 2] No such file or directory: 'codex-acp'
final_acp_model=model-b
agent_state={'acp_current_model_id': 'model-b'}
Because of that environment limitation, I could not verify an actual codex/gemini/Claude ACP subprocess receives and runs with model-b on the first turn. Future QA would benefit from AGENTS.md guidance for a lightweight ACP provider fixture or installed test ACP binary suitable for manual end-to-end runs.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
…t_session_model claude-agent-acp 0.30.0 silently ignores the session/new _meta model payload (#3654), so _maybe_set_session_model issues a one-shot set_session_model for every built-in provider (codex, gemini, claude-code) at session creation — not a Claude-only _meta path. Fixes a stale line carried over from the issue text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ 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.
Code Review: fix(acp): persist pre-session model switch instead of raising
🟡 Acceptable - Solid fix with good test coverage; minor suggestions below.
Summary
This PR fixes issue #3763 where switch_acp_model() called before the first run() raised RuntimeError, silently losing the model change. The fix introduces a has_live_acp_session predicate to detect session state and defers the model switch (persist-only) when no live session exists yet.
The changes are well-scoped: ACPAgent gains a public predicate, LocalConversation branches on it, the router removes the 409 mapping, and the event service adopts a shared ValueError("inactive_service") convention.
[TESTING GAPS]
No gaps found. The test suite covers:
- Pre-session deferral and persistence (
test_switch_acp_model_before_session_defers_and_persists) - Inactive service error handling (
test_switch_acp_model_inactive_service_raises_value_error) - Session state predicate edge cases (
TestHasLiveACPSession) - HTTP route transitions from 409 to 200/400
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: LOW
This is a contained behavior change that:
- Adds a new public property (
has_live_acp_session) without modifying existing API contracts - Improves behavior for pre-session model switches (200 instead of raising)
- Maintains the RuntimeError for post-session callers of
set_acp_modeldirectly - Has comprehensive test coverage across all code paths
VERDICT:
Worth merging - Core logic is sound, behavior is improved, and tests are solid.
KEY INSIGHT:
The predicate has_live_acp_session elegantly separates "no session yet" (defer) from "session closed" (error), enabling a clean branch in LocalConversation.switch_acp_model without special-casing at the lower set_acp_model level.
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
Pre-session ACP model switching now persists and returns success through the SDK and agent-server API instead of raising/dropping the new model.
Does this PR achieve its stated goal?
Yes. The PR set out to fix pre-run() ACP model switching so the selected model is persisted before any live ACP session exists, and the actual SDK/API behavior now matches that: the same scenario that raised RuntimeError/HTTP 409 on origin/main succeeds on the PR commit, persists model-b, and surfaces model-b after reload/API reads.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and local agent-server started successfully |
| CI Status | 🟡 At review time: 23 successful checks, 7 still in progress (mostly image builds + qa-changes), 2 skipped |
| Functional Verification | ✅ SDK pre-session switch and real HTTP /switch_acp_model flow verified before/after |
Functional Verification
Test 1: SDK pre-session switch_acp_model persistence
Step 1 — Reproduce baseline without the fix:
Ran git switch --detach origin/main && uv run python /tmp/acp_switch_qa.py:
{
"switch_result": {
"raised": true,
"exception_type": "RuntimeError",
"message": "ACP session is not initialized; the model can only be switched after the conversation has started (first run())."
},
"after_switch": {"acp_model": "model-a", "llm_model": "model-a"},
"agent_state": {},
"persisted_base_state": {
"agent_acp_model": "model-a",
"reloaded_agent_acp_model": "model-a",
"reloaded_agent_llm_model": "model-a",
"reloaded_agent_state_acp_current_model_id": null
}
}This confirms the old bug path: a pre-session switch raised before persistence, so both live state and persisted/reloaded state stayed on model-a.
Step 2 — Apply the PR changes:
Checked out 4974780c8742bc3fe3c30892117fa25f3d148cda.
Step 3 — Re-run with the fix in place:
Ran uv run python /tmp/acp_switch_qa.py:
{
"switch_result": {"raised": false},
"after_switch": {
"acp_model": "model-b",
"has_live_acp_session": false,
"llm_model": "model-a"
},
"agent_state": {"acp_current_model_id": "model-b"},
"persisted_base_state": {
"agent_acp_model": "model-b",
"agent_state_acp_current_model_id": "model-b",
"reloaded_agent_acp_model": "model-b",
"reloaded_agent_llm_model": "model-b",
"reloaded_agent_state_acp_current_model_id": "model-b"
}
}This shows the fix works for the SDK entry point: no live session exists, no exception is raised, acp_model and the UI-facing acp_current_model_id hint persist as model-b, and a reload re-derives the ACP sentinel LLM model as model-b.
Test 2: Agent-server HTTP API pre-session switch
Step 1 — Reproduce baseline without the fix:
On origin/main, started the real server with uv run python -m openhands.agent_server, created an idle ACP conversation via POST /api/conversations, then ran:
curl -i -X POST http://127.0.0.1:8000/api/conversations/$CONV_ID/switch_acp_model -H 'Content-Type: application/json' --data @/tmp/switch_acp_body.jsonObserved:
HTTP/1.1 409 Conflict
{"detail":"ACP session is not initialized; the model can only be switched after the conversation has started (first run())."}This confirms the old server behavior exposed the SDK pre-session failure as HTTP 409.
Step 2 — Apply the PR changes:
Checked out 4974780c8742bc3fe3c30892117fa25f3d148cda and restarted uv run python -m openhands.agent_server.
Step 3 — Re-run with the fix in place:
Created the same idle ACP conversation and posted the same switch request. Observed:
HTTP/1.1 200 OK
{"success":true}Then GET /api/conversations/$CONV_ID summarized to:
{
"agent_acp_model": "model-b",
"agent_llm_model": "model-b",
"agent_state": {"acp_current_model_id": "model-b"},
"current_model_id": "model-b",
"execution_status": "idle"
}This verifies the user-facing API goal: a pre-first-run ACP switch now returns 200 and the conversation readback shows the selected model persisted while the conversation remains idle.
Unable to Verify
I did not run a real Codex/Gemini/Claude ACP provider turn after the switch because this QA environment does not have those provider CLIs installed:
codex-acp=NOT_FOUND
claude-agent-acp=NOT_FOUND
gemini=NOT_FOUND
The persisted/reloaded SDK state and the agent-server readback both show the model value that first session creation should consume, but a provider-subprocess end-to-end run would require one of those CLIs plus credentials.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
…otocol call Post-#3654, claude-code has supports_set_session_model=True, so _maybe_set_session_model issues set_session_model for every built-in provider (not a no-op for claude). The session/new _meta payload is a no-op on the pinned claude CLI (0.30.0) — kept only as backward-compat for older claude CLIs. - Fix the stale 'no-op for claude' comment that contradicted the post-#3654 behavior (and the function's own docstring). - Tighten override_applied from 'bool(session_meta) or applied_via_call' to 'applied_via_call'. Provably behavior-preserving: build_session_model_meta is truthy only for claude+acp_model, and in that exact case set_session_model either returns True or raises before this line — so the two expressions always agree. The result is that current_model_id's honesty now tracks the actual protocol call rather than the vestigial _meta payload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. |
|
✅ 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.
Code Review: fix(acp): persist pre-session model switch instead of raising
🟢 Good taste — Clean, minimal fix. The new has_live_acp_session predicate is exactly the right abstraction: it replaces an implicit RuntimeError contract with a first-class public interface that LocalConversation can branch on, eliminating the special-case ordering problem that caused the original bug.
Summary
Fixes #3763 where LocalConversation.switch_acp_model called ACPAgent.set_acp_model() before persisting. When no live session existed (pre first run()), set_acp_model raised RuntimeError, silently dropping both the live call and the persist — so the first session used the construction-time model, not the switched one. The fix introduces ACPAgent.has_live_acp_session so LocalConversation can skip the live call and persist-only when no session is active, letting session creation on first run() honor the deferred value.
Files changed: 8 (4 source, 4 test)
CI: 21 ✅ checks passing, 8 in progress (image builds + qa-changes)
Key Observations
-
The fix is surgical:
LocalConversation.switch_acp_modelis the only place that needed real logic change — everything else flows from the new predicate. The router/event-service changes are mechanical (remove a 409 mapping, adopt a shared error convention). -
has_live_acp_sessionis well-defined: All three handles (_conn,_session_id,_executor) must be present. The test suite verifies partial-wiring returnsFalse, which guards against a subtle class of teardown/bootstrap race. -
override_appliedsimplification is correct: Changingoverride_applied = bool(session_meta) or applied_via_callto justoverride_via_callin_init()is sound. The old code could claimoverride_applied = Truefrom_metaalone on the pinned Claude CLI (which ignores_meta), masking a failed or never-issued call. For older CLIs that honor_meta,_maybe_set_session_modelissues the call and returnsTrue, soapplied_via_callcaptures the signal correctly. -
The
inactive_serviceValueErrorconvention is a good cleanup: Consistency with other event-service methods makes the error handling surface more predictable. -
The new test (
test_switch_acp_model_before_session_defers_and_persists) is the right regression test: It exercises the full reload path and verifiesmodel_post_initre-derives the sentinelllm.modelfrom the persistedacp_model, which is the exact acceptance criterion.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
No breaking changes to existing APIs. The live-session path is unchanged. The HTTP route changes from 409 → 200 for the pre-session case (and 400 for inactive service) are strictly more permissive. Test coverage is thorough, including the reload-from-disk path.
VERDICT:
✅ Worth merging: Well-scoped, well-tested bug fix. No issues found.
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: PARTIAL
Pre-session ACP model switching now works through both the SDK and agent-server API, but I could not complete a real first-turn run against an external ACP provider in this environment.
Does this PR achieve its stated goal?
Partially verified. The core regression is fixed in the exercised user-facing paths: on main, a pre-run() ACP switch raises/returns 409; on this PR, the same SDK call succeeds, persists model-b into conversation state/base_state, and the HTTP endpoint returns 200 with current_model_id, agent.acp_model, and agent_state.acp_current_model_id all updated to model-b. I did not verify an actual first prompt against Codex/Gemini/Claude ACP because this environment lacks a usable real ACP provider session/credentials.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv sync --dev completed and local SDK/server entry points ran |
| CI Status | 🟡 Snapshot: 0 failing; 34 passing, 2 skipped, only qa-changes still in progress |
| Functional Verification | ✅ SDK deferral/persistence and agent-server API 409→200 behavior verified before/after |
Functional Verification
Test 1: SDK pre-session switch no longer raises and persists the new ACP model
Step 1 — Reproduce baseline on origin/main:
Ran a short SDK script that constructs an ACPAgent(acp_model="model-a"), creates a LocalConversation, and calls conversation.switch_acp_model("model-b") before any run():
cd /tmp/qa-sdk-main
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_pre_session_switch.pyObserved:
initial_model=model-a
switch_result=raised:RuntimeError:ACP session is not initialized; the model can only be switched after the conversation has started (first run()).
This confirms the reported bug exists on the base branch: a user switching models before the first run hits the no-session error before persistence can happen.
Step 2 — Apply the PR's changes:
Switched back to /home/runner/work/software-agent-sdk/software-agent-sdk/pr-repo at commit 4f6b6651054df125de2b5b64088b7e19e6b6fbff.
Step 3 — Re-run with the fix in place:
Ran the same SDK script on the PR branch:
cd /home/runner/work/software-agent-sdk/software-agent-sdk/pr-repo
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_pre_session_switch.pyObserved:
initial_model=model-a
switch_result=ok
conversation_agent_model=model-b
state_agent_model=model-b
agent_state_current_model=model-b
persisted_agent_model=model-b
persisted_llm_model=model-b
This shows the pre-session switch no longer raises, the active conversation/state both carry model-b, the UI-facing agent_state hint is updated, and a reload from persisted base_state reconstructs the ACP agent with model-b.
Test 2: Agent-server API returns 200 and exposes persisted model state
Step 1 — Reproduce baseline on origin/main:
Started the base server from a clean worktree and made real HTTP requests to create an ACP conversation and switch before running it:
cd /tmp/qa-sdk-main
OPENHANDS_SUPPRESS_BANNER=1 uv run python -m openhands.agent_server --host 127.0.0.1 --port 8767
curl -H 'Content-Type: application/json' --data @/tmp/qa_create_acp_base_clean.json http://127.0.0.1:8767/api/conversations
curl -H 'Content-Type: application/json' --data '{"model":"model-b"}' http://127.0.0.1:8767/api/conversations/a98c3f36-6f32-46b3-ba50-86a0c98f04df/switch_acp_modelObserved the create call returned 201 with current_model_id:"model-a", then the pre-session switch returned:
{"detail":"ACP session is not initialized; the model can only be switched after the conversation has started (first run())."}
HTTP_STATUS:409This confirms the old API behavior rejected the real pre-session user flow.
Step 2 — Apply the PR's changes:
Started the PR branch agent server on port 8765.
Step 3 — Re-run with the fix in place:
Created the same ACP conversation and switched before running it:
curl -H 'Content-Type: application/json' --data @/tmp/qa_create_acp.json http://127.0.0.1:8765/api/conversations
curl -H 'Content-Type: application/json' --data '{"model":"model-b"}' http://127.0.0.1:8765/api/conversations/6b44a48a-ed14-46f4-9d0d-e0764fc45d90/switch_acp_model
curl http://127.0.0.1:8765/api/conversationsObserved:
{"success":true}
HTTP_STATUS:200
The follow-up conversation state included:
{
"current_model_id": "model-b",
"supports_runtime_model_switch": false,
"agent_state": {"acp_current_model_id": "model-b"},
"agent": {"acp_model": "model-b", "kind": "ACPAgent"}
}This demonstrates the agent-server entry point now returns success for the pre-run switch and surfaces the deferred model consistently to clients.
Unable to Verify
I did not complete a real first-turn run against Codex/Gemini/Claude ACP. The environment does not provide a usable authenticated ACP provider session, and a quick attempt to use a minimal custom fake ACP subprocess hung before protocol initialization, so I did not count it as evidence. For future QA runs, it would help to document a credentials-free ACP demo server or a known-good local ACP command in AGENTS.md so the first run() provider handshake can be exercised end-to-end.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
Validated across all three ACP providers (real CLIs, both timings): - current_model_id docstring: note it is the authoritative session model — trust it over the model's self-description. gemini-cli reports a stale gemini-1.5-flash-latest regardless of the active model and codex won't name its tier, yet the switch is honored. - set_acp_model docstring: note the claude-agent-acp live-switch quirk — a mid-session set_session_model changes the session model and inference target but does not refresh the model-naming system context injected at session start, so the agent's self-reported identity can lag until a new session. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ 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.
Code Review: fix(acp): persist pre-session model switch instead of raising
🟢 Good taste — Elegant, minimal fix that eliminates the ordering dependency at its root.
Summary
Fixes #3763 where LocalConversation.switch_acp_model() called ACPAgent.set_acp_model() before persisting. When no live session existed (pre first run()), set_acp_model raised RuntimeError, silently dropping both the live call and the persist — so the first session used the construction-time model instead of the switched one.
The fix introduces ACPAgent.has_live_acp_session — a public predicate that replaces the implicit RuntimeError contract with a first-class interface LocalConversation can branch on. Pre-session switches now defer (persist-only), and session creation on first run() honors the deferred value.
Files changed: 8 (4 source, 4 test)
Additions/Deletions: +257/-48
Key Observations
-
has_live_acp_sessionis well-scoped: ReturnsTrueonly when all three runtime handles (_conn,_session_id,_executor) are present. This is tested exhaustively inTestHasLiveACPSessionwith parametrized cases for each partial-wiring scenario. -
The router change is mechanical: Removing the 409 mapping and updating the docstring is the natural consequence of the SDK change — no logic decision here.
-
inactive_serviceconvention adoption: The event service now raisesValueError("inactive_service")instead ofRuntimeError, aligning with other event-service methods and mapping to 400 instead of 409. This is a cleaner error taxonomy. -
The
override_appliedsimplification is correct: The comment explains it well —_metais a no-op on the pinned Claude CLI, soset_session_modelbeing the only authoritative signal is the right invariant. -
Test coverage is solid:
test_switch_acp_model_before_session_defers_and_persists: Covers the core regressiontest_switch_acp_model_pre_session_returns_200: HTTP route behaviortest_switch_acp_model_inactive_service_returns_400: Error pathtest_switch_acp_model_inactive_service_raises_value_error: Event service errorTestHasLiveACPSession: Predicate edge cases
[CRITICAL ISSUES]
None.
[IMPROVEMENT OPPORTUNITIES]
None. The changes are appropriately scoped and the implementation is clean.
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: 🟢 LOW
This is a contained behavior change that:
- Adds a new public property (
has_live_acp_session) without modifying existing API contracts - Improves behavior for pre-session model switches (200 instead of raising)
- Maintains backward compatibility for post-session callers
- Has comprehensive test coverage across all code paths
- The
release_runtime()call being conditional onliveis correct — pre-session agents have no runtime to release
VERDICT:
✅ Worth merging — Core logic is sound, minimal surface area, excellent test coverage.
KEY INSIGHT:
The new has_live_acp_session predicate elegantly solves the ordering problem: by making the session state explicit, LocalConversation can now decide the execution path upfront rather than relying on exception flow.
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 WITH ISSUES
Pre-session ACP model switching now works through both the SDK and live agent-server API, but QA found an input-validation regression for empty model IDs.
Does this PR achieve its stated goal?
Yes for the intended valid-model flow. On main, calling switch_acp_model("model-b") before the first run raised RuntimeError and left persisted state at model-a; on this PR, the same SDK call returns successfully, updates acp_model/agent_state, and a reloaded ConversationState reports llm.model == "model-b". The live HTTP API also changes from 409 on main to 200 on the PR and surfaces current_model_id: "model-b" afterward.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and installed the uv environment. |
| CI Status | 🟡 Most checks passing; Agent Server image builds and QA evaluation were still pending at refresh. |
| Functional Verification |
Functional Verification
Test 1: SDK pre-session ACP model switch persists and survives reload
Step 1 — Reproduce / establish baseline (without the fix):
Checked out origin/main and ran uv run python /tmp/qa_acp_switch.py, which creates an ACPAgent(acp_model="model-a"), creates a LocalConversation, and calls conv.switch_acp_model("model-b") before any run():
CASE=switch-to-model-b
initial_agent_acp_model=model-a
initial_has_live=attribute-missing
switch_result=ERROR RuntimeError: ACP session is not initialized; the model can only be switched after the conversation has started (first run()).
after_agent_acp_model='model-a'
after_state_agent_acp_model='model-a'
after_agent_state_current=None
base_state_agent_acp_model='model-a'
reloaded_agent_acp_model='model-a'
reloaded_agent_llm_model='model-a'
This confirms the bug: a pre-session switch fails before persistence, so the next session would still start from model-a.
Step 2 — Apply the PR's changes:
Checked out 07b678a93822785a7b9dbaa1ecedab9af957f1cb.
Step 3 — Re-run with the fix in place:
Ran the same command:
CASE=switch-to-model-b
initial_agent_acp_model=model-a
initial_has_live=False
switch_result=OK
after_agent_acp_model='model-b'
after_state_agent_acp_model='model-b'
after_agent_state_current='model-b'
base_state_agent_acp_model='model-b'
base_state_agent_state_current='model-b'
reloaded_agent_acp_model='model-b'
reloaded_agent_llm_model='model-b'
This shows the fix works for the core SDK path: the pre-session switch no longer raises, the switched model is persisted, and reload reconstructs the effective LLM sentinel from the switched ACP model.
Test 2: Live agent-server API returns 200 for a pre-session switch
Step 1 — Reproduce / establish baseline (without the fix):
On origin/main, started the server with uv run python -m openhands.agent_server --host 127.0.0.1 --port 8766, created an ACP conversation via POST /api/conversations, then called:
curl -X POST http://127.0.0.1:8766/api/conversations/$CONV_ID/switch_acp_model -H 'Content-Type: application/json' --data '{"model":"model-b"}'Observed:
{"detail":"ACP session is not initialized; the model can only be switched after the conversation has started (first run())."}
HTTP_STATUS:409
This confirms the old API behavior rejected the pre-session switch.
Step 2 — Apply the PR's changes:
Checked out 07b678a93822785a7b9dbaa1ecedab9af957f1cb and started the server with uv run python -m openhands.agent_server --host 127.0.0.1 --port 8765.
Step 3 — Re-run with the fix in place:
Created the same ACP conversation and called the same endpoint:
{"success":true}
HTTP_STATUS:200
A follow-up GET /api/conversations/$CONV_ID showed the deferred model exposed to clients:
{
"current_model_id": "model-b",
"agent_state": {"acp_current_model_id": "model-b"},
"agent": {"acp_model": "model-b"}
}This verifies the PR's user-facing API behavior: a created-but-not-yet-run ACP conversation can now switch models successfully before the first run.
Test 3: Empty model edge case regression
Step 1 — Reproduce / establish baseline (without the fix):
On origin/main, the SDK call conv.switch_acp_model("") rejected the invalid input before touching state:
CASE=switch-to-empty-model
switch_result=ERROR ValueError: model must be a non-empty string
after_agent_acp_model='model-a'
after_state_agent_acp_model='model-a'
Step 2 — Apply the PR's changes:
Checked out 07b678a93822785a7b9dbaa1ecedab9af957f1cb.
Step 3 — Re-run with the fix in place:
The same SDK script now accepts and persists the empty string:
CASE=switch-to-empty-model
initial_has_live=False
switch_result=OK
after_agent_acp_model=''
after_state_agent_acp_model=''
after_agent_state_current=''
base_state_agent_acp_model=''
base_state_agent_state_current=''
The live API mirrors that regression:
POST /api/conversations/$CONV_ID/switch_acp_model {"model":""}
{"success":true}
HTTP_STATUS:200
Follow-up summary:
{
"http_status": "200",
"current_model_id": "",
"agent_state": {"acp_current_model_id": ""},
"agent_acp_model": "",
"agent_llm_model": "model-a"
}This is a behavioral regression from the previous non-empty validation and leaves clients seeing an empty current model.
Unable to Verify
I did not run a real Codex/Gemini/Claude ACP subprocess through the first actual inference turn because this QA environment does not have those provider CLIs/credentials configured. I verified the SDK and HTTP deferral behavior plus persisted reload state that session startup consumes. Future QA would be stronger with an AGENTS.md-documented credentials-free fake ACP server or smoke command that records the model passed during session/new / set_session_model.
Issues Found
- 🟠 Issue: Pre-session
switch_acp_model("")now succeeds and persists an empty model ID; the same input previously raisedValueError, and the live API now returns200withcurrent_model_id: "".
This review was created by an AI agent (OpenHands) on behalf of the user.
| # fails we leave the persisted state untouched. Without one (pre | ||
| # first run()), there is nothing to switch live — skip the call and | ||
| # just persist; session creation applies the value (see docstring). | ||
| live = self.agent.has_live_acp_session |
There was a problem hiding this comment.
🟠 Important: Functional QA found that the pre-session path now bypasses the existing non-empty model validation. On main, conv.switch_acp_model("") raised ValueError: model must be a non-empty string; on this PR the same SDK call returns OK and persists acp_model == "", and the live API returns 200 with current_model_id: "". Please preserve the set_acp_model validation (or equivalent) before persisting in the no-live-session branch.
Deeper e2e validation found codex-acp 0.16.0 breaks the turn *after* a model
switch: the switch applies (set_config_option, current_model_id tracks) but the
next prompt fails with JSON-RPC -32603. Confirmed apples-to-apples against the
ChatGPT-subscription backend — same SDK/auth/machine/model, only the codex
version differs:
codex 0.15.0 (set_session_model): switch gpt-5.5/high -> turn OK
codex 0.16.0 (set_config_option): switch gpt-5.5/{xhigh,high,medium},
gpt-5.4-mini/low -> turn -32603 (6+ runs)
Bumping would regress ACP model switching for codex (the #3763/#3764 path), so
hold at 0.15.0 (its set_session_model switch + turn works fully). Keep the
configOptions detection/apply code — claude-agent-acp 0.44 needs it, and it
already handles codex 0.16 if/when codex fixes the set_config_option turn path
(detection is per-session, no further change needed to un-hold). claude stays at
0.44.0, gemini at 0.46.0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nds#3764) Co-authored-by: Simon Rosenberg <simon@openhands.dev> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
HUMAN:
Reviewed and authored with Claude Code. Root-cause fix for #3763: a pre-
run()ACP model switch was silently dropped because the live protocol call ran before
the persist. This persists the switched model in the no-session case so the
first session starts on it, and lets the downstream OpenHands/agent-canvas
band-aids be removed.
AGENT:
Why
A pre-session ACP model switch (changing the model on an ACP conversation
before the first
run()) was silently lost.LocalConversation.switch_acp_modelcalled
ACPAgent.set_acp_model()first, which raisesRuntimeErrorwhen nolive session exists yet, so the persist block never ran and the model the first
run actually used stayed the construction-time
acp_model. Downstream thismanifested as a misleading "success" in OpenHands/agent-canvas (the chip shows
the new model; the run uses the old one) or a hard
409, depending ondeployment. The apply-on-start machinery already existed — only the persist was
missing.
Summary
LocalConversation.switch_acp_modelnow branches on whether alive session exists. With one, behavior is unchanged (live
session/set_modelrelease_runtime). Without one (pre firstrun()), it skips the live calland just persists the
model_copy(+agent_state["acp_current_model_id"]hint). Session creation on the first
run()already honorsacp_model(
_maybe_set_session_modelfor codex/gemini,session/new_metaforClaude), so the first turn runs the switched model. The deferred value
survives a process restart via
base_state.json.ACPAgent.has_live_acp_session(conn + session idRuntimeError/ reaching into PrivateAttrs.set_acp_modelreuses it.switch_acp_modelroute no longer mapsRuntimeError→
409("ACP session not initialized yet") and drops that response from theOpenAPI doc: the SDK no longer raises for the pre-session case, so it returns
200.event_service.switch_acp_modelnow raises the sharedinactive_serviceValueError(→400) for a closed/never-started service,consistent with its sibling methods.
Downstream simplifications this unblocks (tracked separately): OpenHands#14510
can drop the
409 → 200 + display-only persistband-aid; agent-canvas#1390's"+ New Conversation" pre-run switch starts working with no change.
Issue Number
Fixes #3763
How to Test
Targeted + full affected test files (CI-pinned ruff 0.13.1, pyright clean):
Key new coverage:
test_switch_acp_model_before_session_defers_and_persists— a pre-run()switch persists
acp_model+ theacp_current_model_idhint without raising,and reconstructs from
base_state.jsonwith the switched model(
model_post_initre-derives the sentinelllm.model) — proving the firstsession starts on the new model and the value survives a restart (acceptance
criteria 1–3).
TestHasLiveACPSession— the predicate isFalsepre-session,Trueonlywhen all three runtime handles are wired,
Falseon partial wiring; thelow-level
set_acp_modelstill raises pre-session (kept in sync).test_switch_acp_model_pre_session_returns_200/_inactive_service_returns_400— route returns200for the deferral and400for an inactive service (no more409).test_switch_acp_model_persists_authoritative_model,_refreshes_surfaced_current_model_id,_disarms_discarded_agent_finalizer,TestSetACPModel) still pass — the post-run()in-place switch is unchanged.Note: a real ACP-subprocess end-to-end (codex/gemini live
set_session_model,Claude
_meta) is not exercised in this PR's automated run; the deferral paththat this change adds is covered by the reload-survival test above, and the live
path is unchanged.
Type
Notes
The in-memory sentinel
llm.modelstays at its prior value between a pre-sessionswitch and the first
run()(the shallowmodel_copydoes not re-runmodel_post_init); this is harmless — ACP inference is owned by the subprocess,the chip reads
acp_current_model_id(persisted here), andmodel_post_initre-derives the sentinel from
acp_modelon the first run / reload. The liveswitch's timeout path is unchanged.
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:07b678a-pythonRun
All tags pushed for this build
About Multi-Architecture Support
07b678a-python) is a multi-arch manifest supporting both amd64 and arm6407b678a-python-amd64) are also available if needed