fix(llm): resolve chat-option capabilities from the canonical model name

[JiataiWang]

HUMAN:

Chat options ignored model_canonical_name when detecting capabilities, so a proxied thinking model kept temperature and never enabled thinking. I verified the canonical model now strips temperature/top_p correctly and added a regression test.

---

AGENT:

Why

select_chat_options looked up model features with the raw llm.model, while every other capability lookup in the SDK (responses_options, is_caching_prompt_active, uses_responses_api, _to_chat_dicts, _inline_required) uses _model_name_for_capabilities() (= model_canonical_name or model). For a litellm_proxy/<alias> model with model_canonical_name set to the real model, the alias carries no feature info, so the chat path never detected reasoning-effort / extended-thinking / prompt-cache-retention support: it left temperature/top_p in the request (which Anthropic reasoning models reject) and never enabled thinking.

Summary

• Resolve the three get_features(...) calls in chat options from the canonical model name. The raw llm.model is still used for provider-routing string checks (azure/gemini prefixes).
• Add a regression test; give the chat-options test double a _model_name_for_capabilities() mirroring the real LLM.

Issue Number

N/A — found via code review; no pre-existing issue.

How to Test

Reproduced before/after with LLM(model="litellm_proxy/my-claude", model_canonical_name="claude-sonnet-4-5", temperature=0.5):

get_features("litellm_proxy/my-claude").supports_extended_thinking == False
get_features("claude-sonnet-4-5").supports_extended_thinking       == True

BEFORE: temperature survived in the request; thinking not enabled. AFTER: temperature/top_p stripped, same posture as the direct model.

Tests: .venv/bin/python -m pytest tests/sdk/llm/test_chat_options.py tests/sdk/llm/test_model_canonical_name_resolution.py -q — 19 passed. ruff clean.

Type

• Bug fix

Notes

Found via code review; no pre-existing issue.

🤖 Generated with Claude Code

[all-hands-bot]

[Automatic Post]: It has been a while since there was any activity on this PR. @JiataiWang, 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.

This comment was created by an AI agent (OpenHands) on behalf of the user.

[enyst]

👋 I'm an AI agent running on Claude Opus 5, reviewing on behalf of @enyst using the codereview-roasted skill.

Taste Rating: 🟢 Good taste — this is the version of "make it consistent" that I almost never get to approve: it fixes all three call sites rather than the one that bit you, and the test proves the fix instead of decorating it.

I checked both load-bearing claims rather than believing the description.

Claim 1 — nothing was left behind. All three get_features calls in the file now go through the canonical name; there is no straggler still reading llm.model:

$ grep -n "get_features(" openhands-sdk/openhands/sdk/llm/options/chat_options.py
60:    supports_reasoning_effort = get_features(model_for_caps).supports_reasoning_effort
73:    if get_features(model_for_caps).supports_extended_thinking:
105:        get_features(model_for_caps).supports_prompt_cache_retention

Claim 2 — "this matches every other capability lookup in the SDK." It does, and responses_options.py:53 was already doing the identical cross-module call:

openhands-sdk/openhands/sdk/llm/options/responses_options.py:53:    model_features = get_features(llm._model_name_for_capabilities())

So chat_options was the odd one out, and this makes the two sibling option-builders agree. (Yes, _model_name_for_capabilities is a private method being called across a module boundary — but that boat sailed in responses_options, and being inconsistently wrong is worse than being consistently informal. Promoting it to a public accessor is a separate, larger PR.)

Claim 3 — the test discriminates. I neutralized only your one changed line (model_for_caps = llm.model) and reran:

E   AssertionError: assert 'temperature' not in {'max_completion_tokens': 1024, 'temperature': 0.5, 'top_p': 1.0}
FAILED ...::test_canonical_name_drives_capability_detection_for_proxied_model

Fails without the fix, 17 passed with it. The second half of that test — asserting the raw alias still keeps temperature == 0.5 — is the part most people skip, and it's what stops the test from silently passing for the wrong reason later.

[IMPROVEMENT OPPORTUNITIES]

Nothing worth holding the PR for. The six-line comment above model_for_caps is longer than the change, but it documents a genuinely non-obvious split (capabilities key off the canonical name, provider routing keys off the raw one) that the next person would otherwise "simplify" straight back into a bug. It stays.

[TESTING GAPS]

None.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW — _model_name_for_capabilities() falls back to self.model when no canonical name is set, so every existing non-proxied configuration resolves byte-identically. The only behavior that changes is the case that was already wrong. No API surface, no dependencies.

VERDICT: ✅ Worth merging.

KEY INSIGHT: The bug wasn't the missing canonical lookup — it was that "which name do I ask about capabilities?" had two answers living in two sibling files, and only one of them was right.

[all-hands-bot]

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[JiataiWang]

#4200 has since landed the broader capability-resolution fix, and current main now uses _model_name_for_capabilities() on this chat-options path. There is no useful independent delta left here, so I’m closing this rather than asking maintainers to review duplicate code.