fix: validate provider prefixes in unverified model list#1668
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
Hi! I’m OpenHands (automated agent) working on this PR. I ran the repo-required checks and did live verification to ensure the provider list is correct and downstream apps (e.g. OpenHands-CLI) won’t need heuristics. Local checks (required by SDK README)
Unit tests
Live verification: provider list correctnessWhat was wrong before
What the fix doesWe now validate extracted provider prefixes against Results (after fix)
I also compared the SDK’s provider keys with the eval LiteLLM proxy’s provider catalog. Eval proxy provider catalogUsing the env-provided
Why
Live example runsHello world (OpenAI)Command: LLM_MODEL=openai/gpt-4o-mini LLM_API_KEY=$OPENAI_API_KEY uv run python examples/01_standalone_sdk/01_hello_world.pyResult: ✅ Completed successfully and created FACTS.txt. Responses reasoning exampleThis example defaults to an The env-provided
So I ran the reasoning example against the eval proxy explicitly: LLM_API_KEY=$LITELLM_API_KEY LLM_BASE_URL=https://llm-proxy.eval.all-hands.dev uv run python examples/01_standalone_sdk/23_responses_reasoning.pyResult: ✅ Completed successfully (conversation ran, created then deleted FACTS.txt). Hello world (Claude)Direct Anthropic run failed due to account credit balance too low (400 from Anthropic) in this environment: LLM_MODEL=anthropic/claude-3-5-haiku-latest LLM_API_KEY=$ANTHROPIC_API_KEY ...Result: ❌ Workaround for verification in this environment: use the eval proxy (same as above) or use an OpenAI model as demonstrated. If you’d like, I can also add an explicit unit test that asserts common bogus prefixes (e.g. |
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
The PR correctly validates provider prefixes against LiteLLM's provider list, but has a critical logic issue where validation is silently skipped if the provider list fails to load (line 150). Also, the broad exception handling could mask important errors. Details in inline comments below.
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
Found several issues that need attention: missing error handling, inconsistent fallback behavior, and a test that does not work as intended. Details in inline comments.
Co-authored-by: openhands <openhands@all-hands.dev>
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
1 similar comment
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
@OpenHands Read this PR, also the linked CLI PR 322 (you can also clone yourself the CLI repo from the same org), and the linked issue in the PR description. In other words, understand deeply what's going on here. Is this PR a reasonable fix, WDYT? Fix as necessary. Commit and push, if you had any fixes. |
|
I'm on it! enyst can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
Final summary (since my last summary)Checklist
What I changed
Git / delivery
|
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean fix for real problem. Validates provider prefixes against LiteLLM registry to prevent bogus providers (us, eu, low, 1024-x-1024) from polluting downstream UIs. Simple membership check, efficient caching, good tests. LGTM.
|
@OpenHands In CLI, we do something similar from PR 322. WDYT, can we cleanup that code now that we do it upstream? If yes, clone yourself the CLI from the same org and open a CLI PR |
|
I'm on it! enyst can track my progress at all-hands.dev |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean fix for real problem. Validates provider prefixes against LiteLLM registry to prevent bogus providers (us, eu, low, 1024-x-1024) from polluting downstream UIs. Simple membership check, efficient caching, good tests. LGTM.
Summary of work (OpenHands CLI)What you asked
What I did
Checklist against your request
Next step (what I need from you)To proceed with pushing + opening the PR, confirm which token you want embedded in the remote URL:
Once confirmed, I’ll run only:
|
Problem
get_unverified_models()currently splits LiteLLM model identifiers on/or.and treats the first token as a provider.LiteLLM’s registries include many non-provider prefixes (e.g.
us.*,eu.*,low/…,1024-x-1024/…). These prefixes are not LiteLLM providers, but they end up being surfaced as providers inUNVERIFIED_MODELS_EXCLUDING_BEDROCK, leaking noise into downstream apps (e.g. OpenHands-CLI provider dropdown).Fix
litellm.provider_list.otherbucket (do not emit it as a provider).Tests
tests/sdk/llm/test_model_list.pyto assert invalid prefixes likecustom-provider,us.*, and1024-x-1024/*do not become providers.This matches the direction discussed in OpenHands-CLI PR #322 (push provider validation upstream so consumers don’t need heuristics).
@enyst can click here to continue refining the PR
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.12-nodejs22golang: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:e14491d-pythonRun
All tags pushed for this build
About Multi-Architecture Support
e14491d-python) is a multi-arch manifest supporting both amd64 and arm64e14491d-python-amd64) are also available if needed