Skip to content

Conversation

@xingyaoww
Copy link
Collaborator

@xingyaoww xingyaoww commented Dec 5, 2025

Problem

The openhands/ provider was failing when base_url was explicitly set to None, which broke the new user setup flow in the CLI. This occurs when CLI saves settings to JSON with base_url=null and then reloads them.

Related issues:

Root Cause

PR #1273 changed the openhands/ provider conversion logic in llm.py:

# Before (always set):
d["base_url"] = "https://llm-proxy.app.all-hands.dev/"

# After (broke with explicit None):
d["base_url"] = d.get("base_url", "https://llm-proxy.app.all-hands.dev/")

The Issue: Python's dict.get(key, default) only returns the default when the key is missing, not when it exists with value None. When CLI serializes config to JSON with base_url: null and deserializes it, the dictionary contains {"base_url": None}, causing dict.get() to return None instead of the default.

The Workaround in 1.4.0: Commit 54d93bc changed the test workflow to bypass the openhands/ provider entirely by using litellm_proxy/ directly with a hardcoded URL.

Solution

  1. Fixed llm.py: Changed to use d.get("base_url") or default pattern to handle explicit None values correctly
  2. Reverted workflow: Changed .github/workflows/run-examples.yml back to using openhands/ provider as intended
  3. Added tests: Comprehensive test coverage for all scenarios

Code Changes

# Use `or` operator to handle both missing keys and None values
d["base_url"] = d.get("base_url") or "https://llm-proxy.app.all-hands.dev/"

This correctly handles:

  • ✅ Key missing: d.get("base_url")Noneor returns default
  • ✅ Key exists with None: d.get("base_url")Noneor returns default
  • ✅ Key exists with value: d.get("base_url") → value → or short-circuits

Testing

Added comprehensive test cases in test_llm.py:

  • test_base_url_for_openhands_provider - Existing test for missing base_url
  • test_base_url_for_openhands_provider_with_explicit_none - New: Reproduces CLI bug with explicit None
  • test_base_url_for_openhands_provider_with_custom_url - New: Ensures custom URLs still work

All tests pass:

  • ✅ 467 LLM tests passed
  • ✅ Pre-commit hooks passed
  • ✅ Manual testing confirms fix works for all scenarios

Impact

Before Fix

  • ❌ CLI users couldn't use openhands/ provider (settings saved with base_url=null)
  • ❌ New user setup flow was broken
  • ⚠️ Workaround required using litellm_proxy/ directly

After Fix

  • ✅ CLI users can use openhands/ provider seamlessly
  • ✅ New user setup flow works correctly
  • ✅ Custom base_url override still works
  • ✅ Test suite covers all scenarios

Checklist

  • Tests added/updated
  • Pre-commit hooks pass
  • All existing tests pass
  • Changes are backward compatible

@xingyaoww 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

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.12-nodejs22 Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:7d075e5-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-7d075e5-python \
  ghcr.io/openhands/agent-server:7d075e5-python

All tags pushed for this build

ghcr.io/openhands/agent-server:7d075e5-golang-amd64
ghcr.io/openhands/agent-server:7d075e5-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:7d075e5-golang-arm64
ghcr.io/openhands/agent-server:7d075e5-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:7d075e5-java-amd64
ghcr.io/openhands/agent-server:7d075e5-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:7d075e5-java-arm64
ghcr.io/openhands/agent-server:7d075e5-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:7d075e5-python-amd64
ghcr.io/openhands/agent-server:7d075e5-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-amd64
ghcr.io/openhands/agent-server:7d075e5-python-arm64
ghcr.io/openhands/agent-server:7d075e5-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-arm64
ghcr.io/openhands/agent-server:7d075e5-golang
ghcr.io/openhands/agent-server:7d075e5-java
ghcr.io/openhands/agent-server:7d075e5-python

About Multi-Architecture Support

  • Each variant tag (e.g., 7d075e5-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 7d075e5-python-amd64) are also available if needed

The openhands/ provider conversion was not handling the case where base_url
is explicitly set to None (e.g., when CLI saves settings to JSON with
base_url=null and reloads them). This broke the new user setup flow in CLI.

Root cause:
- PR #1273 changed from always setting base_url to using d.get('base_url', default)
- When base_url key exists with None value, dict.get() returns None instead of default
- This caused LiteLLM to fail with missing base_url error

Fix:
- Use 'd.get("base_url") or default' pattern to handle explicit None values
- Revert workaround in run-examples workflow (use openhands/ provider again)
- Add comprehensive tests for all base_url scenarios

Tests added:
- Test with no base_url specified (existing)
- Test with explicit None base_url (new - reproduces CLI bug)
- Test with custom base_url (new - ensures override still works)

Co-authored-by: openhands <[email protected]>
@xingyaoww xingyaoww requested review from enyst and hieptl December 5, 2025 23:47
@xingyaoww xingyaoww marked this pull request as ready for review December 5, 2025 23:47
LLM_MODEL: litellm_proxy/claude-haiku-4-5-20251001
LLM_BASE_URL: https://llm-proxy.app.all-hands.dev
LLM_MODEL: openhands/claude-haiku-4-5-20251001
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}

TBH this line is strange, I think maybe we don't actually need it?

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-sdk/openhands/sdk/llm
   llm.py41615363%357, 362, 366, 370–371, 374, 378–379, 390–391, 393–394, 398, 415, 475, 505–507, 528, 532, 547, 553–554, 578–579, 589, 614–619, 640–641, 644, 648, 660, 665–668, 675, 678, 686–691, 695–698, 700, 713, 717–719, 721–722, 727–728, 730, 737, 740–745, 802–807, 864–865, 868–871, 913, 930, 984, 987, 990–998, 1002–1004, 1007, 1010–1012, 1019–1020, 1029, 1036–1038, 1042, 1044–1049, 1051–1068, 1071–1075, 1077–1078, 1084–1093, 1106, 1120, 1125
TOTAL12425564454% 

Copy link
Collaborator

@enyst enyst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants