fix: enable LiteLLM virtual key cost tracking for default agent#604
fix: enable LiteLLM virtual key cost tracking for default agent#604simonrosenberg wants to merge 1 commit intomainfrom
Conversation
The evaluation orchestrator already creates per-instance LiteLLM virtual keys, but only ACP agents injected them into their LLM client. The default OpenHands agent used the master API key directly, so proxy_cost was always $0.00 for non-ACP runs. Add `apply_virtual_key()` to litellm_proxy.py that returns a copy of the LLM config with the virtual key as api_key (thread-safe via model_copy + threading.local). Update all benchmarks to use it when creating the default Agent. Fixes #603 Co-Authored-By: Claude Opus 4.6 <[email protected]>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Pragmatic fix for a real production issue. The design is solid: simple helper, thread-safe, backwards compatible. The core logic is good, but violates project standards on type hints.
Verdict: ✅ Worth merging after fixing type hints - The solution is fundamentally sound, just needs polish.
Key Insight: This is a textbook example of solving a real problem simply—one function, eight lines, no special cases. Just needs proper type annotations per project standards.
| return getattr(_thread_local, "virtual_key", None) | ||
|
|
||
|
|
||
| def apply_virtual_key(llm): # type: ignore[no-untyped-def] |
There was a problem hiding this comment.
🟠 Important: Missing type hints violates project standards.
Per AGENTS.md: "Avoid # type: ignore unless absolutely necessary". The LLM type is available from the SDK:
| def apply_virtual_key(llm): # type: ignore[no-untyped-def] | |
| def apply_virtual_key(llm: LLM) -> LLM: | |
| """Return an LLM config copy with the per-instance virtual key as api_key. |
You'll need to add from openhands.sdk.llm import LLM at the top of the file.
| virtual_key = get_current_virtual_key() | ||
| if virtual_key is None: | ||
| return llm | ||
| from pydantic import SecretStr |
There was a problem hiding this comment.
🟡 Suggestion: Move import to top of file.
Per project guidelines: "Place all imports at the top of the file unless... circular imports, conditional imports, or imports that need to be delayed for specific reasons."
No circular import risk here since pydantic is external. Move this to the import block at the top:
from pydantic import SecretStr
2 participants
Summary
apply_virtual_key()helper tolitellm_proxy.pythat returns an LLM config copy with the per-instance virtual key asapi_keyAgentmodel_copy()+threading.local(no shared state mutation)Why
The evaluation orchestrator already creates per-instance LiteLLM virtual keys for every run (
evaluation.py:850), but only ACP agents injected them into their LLM client (viabuild_acp_agent()inacp.py). The default OpenHands agent used the master API key directly, soproxy_costwas always$0.00for non-ACP runs.This was discovered during PR validation runs for OpenHands/software-agent-sdk#2656 (eval_limit=5, 3 benchmarks × 3 agent types). The evidence:
Test plan
test_result.proxy_costis non-zeroapply_virtual_key()is a no-op when no virtual key is active (returns original LLM unchanged)Fixes #603
🤖 Generated with Claude Code