Repository navigation
fix(agent-server): keep the managed-key refresh hook after an LLM switch - #5904
Conversation
switch_llm / switch_profile (REST endpoints and the agent's switch tools) install a new LLM object, and the refresh-on-401 hook was registered only when the conversation was loaded. The server's state-change callback now re-runs register_managed_llm_key_refresh when the conversation's agent changes, so an allow-listed switched LLM refreshes its key on a 401. The F12.managed-key-switch map bullet drops its known-bug marker. Fixes #5890 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
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.
This review was posted by an AI agent (OpenHands).
Scope
The change belongs in this repository: it fixes agent-server behavior (the managed-key refresh hook for the Agent Server's REST/WebSocket lifecycle), touches only openhands-agent-server/ plus a test and a skill feature-map reference, and does not cross the ownership boundary into OpenHands/OpenHands, OpenHands/extensions, or OpenHands/automation.
What I verified
The root cause matches the linked issue #5890: register_managed_llm_key_refresh ran only from EventService.start, while LocalConversation.switch_llm (and switch_profile, plus the switch_llm / route_task_to_model tools, which both route through switch_llm) installs a fresh LLM that carries no hook.
The fix is minimal and reuses the existing idempotent, fail-closed mechanism rather than adding a parallel hook or allow-list:
EventService._state_change_callbackwraps the previousconversation._on_eventtarget. On aConversationStateUpdateEventwithkey == "agent"it re-runsregister_managed_llm_key_refresh(conversation._state.agent)and then forwards the event exactly as before, so state publishing to WebSocket subscribers is unchanged.- Every switch path assigns
state.agent, so the callback covers the REST endpoints and the agent tools. I confirmed the assignment inLocalConversation.switch_llm(whichswitch_profiledelegates to) and in the two switch tools. - I confirmed empirically on the head that the callback fires for a real
EventService+switch_llm, that an in-scope switched LLM ends up with a hook that feeds_resolve_refreshed_api_key, that a BYOKbase_urlgets no hook, and that withOH_LLM_API_KEY_REFRESH_URLunset nothing is registered.
Checks on the exact head 7a598030 are green (agent-server-tests, pre-commit, REST API/OpenAPI, integration, security, etc.). Focused tests pass locally: tests/agent_server/test_managed_llm_key.py (17), test_managed_llm_key_http.py + test_event_service.py (149), test_conversation_router.py + test_event_streaming.py + test_conversation_service.py (246). The acceptance criteria in #5890 are met, including the non-persistence of the hook and unchanged reload behavior.
Notes (non-blocking)
- The linked issue's "live evidence" criterion is satisfied by the author's
verify-agent-servermap run (F12.managed-key-switchxfail x2 on main, xpass x2 here, thenpassafter dropping the# bugmarker); this is a production lifecycle path, and that run exercises the real entry point. CI on this head also passes the agent-server integration/stress checks. - The related title-LLM gap (#5528) is explicitly a non-goal here and remains open; no action needed for this PR.
No material correctness, security, compatibility, or acceptance-criterion defect found.
✅ APPROVED
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.(totally human) This is interesting. I reported a couple of bugs with this a while ago, managed LLM lost auth after switch, and I assumed the fix would be in enterprise, but I wasn't aware that managed refresh happens here. Or maybe it happens in the sdk because the bug was fixed only for conversation start, not for switch.
AGENT:
Summary
Registration also covers switches deferred until the step ends.
Why
An LLM switch should retain managed-key refresh instead of failing the next eligible 401.
Issue Number
Fixes #5890
How to Test
Previously recorded verification:
Reproduction commands, results, and limits
uv run pytest tests/agent_server/test_managed_llm_key.py- new test fails on main (assert None is not None), passes here; 17 passed. Alsotest_managed_llm_key_http.py,test_conversation_router.py,test_event_service.py,test_event_streaming.py,test_conversation_service.py: all pass.control-agent-server map run --file F12 --only F12.managed-key-switch --fresh --repeat 2 --checkout <main>->xfailx2 on main (6e7a853); same from this branch ->xpassx2. After dropping the marker:map checkclean andmap run --only F12.managed-key-switch --fresh->pass.F12.managed-key-refreshandF12.managed-key-scopestillpass.pre-commiton the changed files: clean.Video/Screenshots
No UI change; reproduction evidence is above.
Type
Notes
This stays in Agent Server; SDK switch methods are unchanged. The separate title-LLM bypass remains #5528 (discussion). Tracked under #5653.
Visual explanation by Codex using HumanLayer show-me.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang: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:7a59803-pythonRun
All tags pushed for this build
About Multi-Architecture Support
7a59803-python) is a multi-arch manifest supporting both amd64 and arm647a59803-python-amd64) are also available if needed