Repository navigation
Conversation
LocalConversation.switch_profile looked up profile:<name> in the LLM registry before reading the profile file, so a profile edited after the conversation's first switch to it was never installed (the route still answered 200). Read the profile on every call; reuse the registered LLM while the config is unchanged and replace it, keeping its metrics, when the profile was edited. Drop the F09.switch-profile-edited known-bug marker. Fixes #5839 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: this is SDK conversation behavior, which this repository owns, so the change belongs here. Current head 412e8fc35b005be78e5920bffdb26a11380600bb is unchanged from the requested SHA, PR is open and not a draft, and the current-head checks are green. Locally, tests/sdk/conversation/test_switch_model.py (27 passed) and tests/sdk/conversation + tests/sdk/tool/test_switch_llm.py + tests/agent_server/test_conversation_router.py (979 passed) both pass.
The core fix is correct: for a conversation whose agent was already initialized, re-switching to an edited profile replaces the registered profile:<name> LLM with the edited config, and an unchanged profile still reuses the registered object. I confirmed both behaviors directly, including repeated edits (edit → re-switch → edit → re-switch keeps the same metrics object and the stats entry stays in sync).
One material finding, on the new metrics-carry-over line:
switch_profile unconditionally calls installed.restore_metrics(stale.metrics) after the swap. That is only correct when stats.usage_to_metrics[usage_id] already references stale.metrics. When the first switch to the profile happened before the agent was initialized, the stale entry was registered before _ensure_agent_ready() wired ConversationStats.register_llm as the registry subscriber, so profile:<name> is not in stats.usage_to_metrics at all. On the edited re-switch, switch_llm → LLMRegistry.add → notify then records the new LLM's fresh metrics object into stats.usage_to_metrics[usage_id], and restore_metrics(stale.metrics) immediately swaps the live LLM onto the stale object. The result is that stats.usage_to_metrics["profile:<name>"] is left pointing at the now-orphaned fresh object: it never grows, get_combined_metrics() omits the calls made on the installed LLM, and the invariant asserted by the new test (stats.get_metrics_for_usage("profile:fast") is installed.metrics) is false.
Reachable sequence (agent initialized between the first switch and the re-switch — e.g. a conversation that switched to the profile, ran a turn, and then had the profile edited before switching again):
conv.switch_profile("p") # before agent init
conv.send_message("hi") # _ensure_agent_ready wires the subscriber; profile:p is skipped (already registered)
store.save("p", <edited llm>)
conv.switch_profile("p")
# conv.agent.llm.metrics is not conv.conversation_stats.usage_to_metrics["profile:p"]Measured on the current head: after the edited re-switch, stats.usage_to_metrics["profile:p"] exists but is not conv.agent.llm.metrics; a subsequent call's cost is recorded on the live LLM and get_combined_metrics() still reports 0. (The combined-cost under-report in this pre-init path is pre-existing; what this diff adds is the stats entry that is silently disconnected from the installed LLM, plus the failure of the PR's own "keeps the usage recorded so far" contract and test invariant in that path.)
The new tests only cover the initialized-first path, which is why they pass. Note that restoring the metrics onto profile_llm before switch_llm is not a sufficient fix either, because LLMRegistry._ensure_independent_metrics resets a metrics object whose id it already tracks. The carry-over needs to end with stats.usage_to_metrics[usage_id] referencing the same object the installed LLM uses (for example by re-pointing the stats entry after the restore), not just the LLM.
Everything else in the diff (the docstring, the F09 map prose and Gotchas line) matches the implemented behavior.
🔄 CHANGES REQUESTED
| self.llm_registry.remove(usage_id) | ||
| self.switch_llm(profile_llm) | ||
| installed = self.llm_registry.get(usage_id) | ||
| installed.restore_metrics(stale.metrics) |
There was a problem hiding this comment.
This carries the stale metrics onto the installed LLM only if stats.usage_to_metrics[usage_id] already references stale.metrics. When the profile's first switch happened before the agent was initialized, that entry does not exist: switch_llm -> LLMRegistry.add -> notify records the new LLM's fresh metrics into stats.usage_to_metrics[usage_id], and this line then swaps the live LLM onto stale.metrics, leaving the stats entry pointing at the now-orphaned fresh object. Subsequent usage accrues on the live LLM while get_combined_metrics() omits it, and the new test's invariant (stats.get_metrics_for_usage("profile:fast") is installed.metrics) is false in this path. Reachable when the conversation switched to the profile before its first send_message/run, then had the profile edited before switching again. Re-pointing the stats entry after the restore (so it references the same object the installed LLM uses) is needed; restoring onto profile_llm before switch_llm is not sufficient because LLMRegistry._ensure_independent_metrics resets a metrics object id it already tracks.
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
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 and current state. This is SDK conversation behavior (LocalConversation.switch_profile/switch_llm, LLMProfileStore), which this repository owns, so the change stays here. Re-read immediately before publishing: PR #5852 is open, not a draft, head is exactly a503c1964e6bff3dc9efaa629fee51eca4fa3633 as requested, no labels, and linked issue #5839 is open with bug / priority:low / ready-for-dev. Checks on this exact head: 49 success, 2 skipped, and one failure — Review Thread Gate / unresolved-review-threads, which reflects the earlier bot review's unresolved thread on the previous head, not a code failure; this review supersedes that review. Locally on this head: tests/sdk/conversation/test_switch_model.py 31 passed; tests/sdk/conversation + tests/sdk/tool/test_switch_llm.py 900 passed; tests/agent_server/test_conversation_router.py 114 passed. The claimed verification reproduces.
Merge note. This branch merged upstream main (ce5716a), and the merge rewrote the fix: switch_profile now always loads the profile and delegates to switch_llm, while switch_llm (now from main) replaces a registered entry whose config differs and carries its metrics over. The F09 feature map and the docstring were updated to match, and main's own tests cover the replacement semantics under the default usage id. The scope and tests in the PR description still describe the delivered behavior accurately.
Material finding — metrics desync when the first switch precedes agent initialization. The carry-over in switch_llm rests on ConversationStats tracking the registered LLM's metrics, but a profile LLM registered before the agent is initialized never enters stats.usage_to_metrics (_ensure_agent_ready() wires the register_llm subscriber and then skips usage ids that are already registered). When the profile is edited and switch_profile is called again, the swap and restore_metrics still work, but ConversationStats never tracks the installed LLM's metrics. Measured on this head (switch pre-init → send_message → edit profile → re-switch):
conv.agent.llm.model == "fast-model-v2"— the fix itself works;- installed
profile:fastLLMmetrics.accumulated_cost == 1.5(carried over) whilestats.usage_to_metrics["profile:fast"].accumulated_cost == 0.0andget_combined_metrics() == 0.0; stats.usage_to_metrics["profile:fast"]is a different object than the installed LLM's metrics, so post-switch usage on the profile accrues on an object the combined view never reads, and the carried-over cost is silently dropped.
In this path the docstring's "keeps the usage recorded so far" (and the F09 map's "keeping the usage recorded under that id") is false, and the PR's own invariant — stats.get_metrics_for_usage("profile:fast") is installed.metrics — fails; the new tests only cover the initialized-first path, so they pass. I confirmed the same failure on the pre-merge head 412e8fc in a separate worktree, so the merge carried the finding over rather than introducing it — either way it is present on the head under review. The sequence is a real workflow: switch to a profile, run a first turn, fix the profile, switch again.
Suggested remedy, checked against ConversationStats.register_llm and LLMRegistry._ensure_independent_metrics: in the carry-over branch, after new_llm.restore_metrics(registered.metrics), make stats.usage_to_metrics[usage_id] reference that same object when the usage id is not yet tracked (direct re-pointing is safe and idempotent here because register_llm only restores from usage_to_metrics when the id is not in _restored_usage_ids). Do not instead seed profile_llm.metrics before switch_llm — _ensure_independent_metrics resets a metrics object whose id the registry already tracks, so that would be discarded. Please add a regression test for the pre-init sequence (switch pre-init → send_message → edit → re-switch → assert the stats invariant).
Unrelated, pre-existing, and not required for this PR: profile LLMs switched in before initialization stay invisible to ConversationStats until their config changes, so those calls are omitted from get_combined_metrics().
🔄 CHANGES REQUESTED
| delegates the swap to :meth:`switch_llm` under the usage id | ||
| ``profile:{profile_name}``. The LLM registered under that id is reused | ||
| while the profile is unchanged; after the profile was edited it is | ||
| replaced by the edited config, which keeps the usage recorded so far. |
There was a problem hiding this comment.
The "keeps the usage recorded so far" contract does not hold when the first switch to the profile happened before the agent was initialized: the registered LLM never reaches stats.usage_to_metrics because _ensure_agent_ready() wires the ConversationStats.register_llm subscriber only at initialization and skips usage ids that are already registered. On the edited re-switch, switch_llm swaps the LLM and restore_metrics copies the registered cost onto it, but ConversationStats never tracks that metrics object. Measured on this head: installed profile:fast metrics show accumulated_cost == 1.5 while stats.usage_to_metrics["profile:fast"] stays at 0.0 and get_combined_metrics() reports 0.0 — the carried-over usage is silently dropped and later profile usage accrues outside the combined view. The PR's own test invariant (stats.get_metrics_for_usage("profile:fast") is installed.metrics) fails in this path; the new tests only exercise the initialized-first variant. Fix: in the carry-over branch, after new_llm.restore_metrics(registered.metrics), also re-point stats.usage_to_metrics[usage_id] at that object when the usage id is not yet tracked (direct re-pointing is safe — register_llm only restores when the id is not in _restored_usage_ids; seeding profile_llm.metrics before switch_llm is not sufficient because LLMRegistry._ensure_independent_metrics resets a tracked metrics id). A regression test for the pre-init sequence (switch → send_message → edit → re-switch) would pin this.
| assert installed.metrics is first.metrics | ||
| assert installed.metrics.accumulated_cost == 1.5 | ||
| stats = conv.conversation_stats | ||
| assert stats.get_metrics_for_usage("profile:fast") is installed.metrics |
There was a problem hiding this comment.
This is the invariant the untested pre-init path violates: when the first switch precedes agent initialization, stats.usage_to_metrics["profile:fast"] ends up as a different Metrics object than installed.metrics (measured on this head: entry at 0.0 while the installed LLM carries the restored 1.5, get_combined_metrics() still 0.0). Please add a variant of this test that performs the first switch_profile before send_message, then edits the profile and re-switches, and asserts this same line — it currently passes only because both switches happen after initialization.
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Summary
Reload a selected profile from disk, replace changed LLM configuration, and preserve its accumulated metrics.
Why
Re-selecting an edited LLM profile should install its new configuration without losing accumulated usage.
Issue Number
Fixes #5839
How to Test
Previously recorded verification:
Reproduction commands, results, and limits
uv run pytest tests/sdk/conversation/test_switch_model.py -k edited: both new tests fail on main ('fast-model' == 'fast-model-v2') and pass here.uv run pytest tests/sdk/conversation tests/sdk/tool/test_switch_llm.py tests/agent_server/test_conversation_router.py: 979 passed.control-agent-server map run --file F09 --only F09.switch-profile-edited --fresh --repeat 2:xfailin both attempts with--checkouton origin/main (6e7a853),xpassin both from this branch. After dropping the marker,map checkreports no problems (935 ids) and the same--freshrun reportspass.uv run pre-commit run --files <changed files>: all hooks pass.Video/Screenshots
No UI change; reproduction evidence is above.
Type
Notes
Behavior tradeoff: refresh on every switch (implemented) rather than retaining the documented conversation cache or adding a refresh flag. Subscription profiles re-materialize credentials on re-switch; materialization failure leaves the old registry/active LLM in place. Related switch work: #5779 and #5816.
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:a503c19-pythonRun
All tags pushed for this build
About Multi-Architecture Support
a503c19-python) is a multi-arch manifest supporting both amd64 and arm64a503c19-python-amd64) are also available if neededJev-Fast-Audit
⚡ Jev fast audit · estimates · 0.32s · commit 412e8fc
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 7/7 hunks, 3/3 files.
All estimates and evidence