fix(agent): honour disabled reasoning on the profile-less chat_completions path - #66114
laurinaitis wants to merge 1 commit into
Conversation
…tions path
build_kwargs hardcoded reasoning {"enabled": True} for OpenAI-compatible
routes without a provider profile, so a caller that disabled reasoning got it
back on at medium effort. 5a6720b fixed the same class at the reporting site
and in the zai profile; this transport branch was missed.
Nous rejects enabled=false, so omit the field there rather than send it,
matching the nous profile's own handling.
|
#51940 should carry this. It's three weeks older, it's tied to #51903, and the sweeper has already reviewed it. Closing this one. My body said I found no open issue: true only because #51903 is closed, and I never searched PRs at all, which is how I missed the cluster. On the wire contract I don't think the in-tree evidence lands cleanly either way, so alt-glitch's ask for direction seems right to me. The hosts that reach this branch disagree. Nous rejects My version also leaves the GitHub Models sub-branch unguarded at the transport level. Rebase notes are on #51940. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Honours disabled reasoning on profile-less chat_completions path (39 additions / 0 deletions). Very small, targeted fix.
No security issues, no debug artifacts, no hardcoded secrets. Looks clean.
Note: 0 deletions may indicate this is an addition-only path extension rather than a fix of existing code.
Reviewed by Hermes Agent (cron batch 2026-07-17)
What
ChatCompletionsTransport.build_kwargshardcodesreasoning: {"enabled": True, "effort": ...}forreasoning-capable routes that resolve no provider profile, so a caller passing
{"enabled": False}getsreasoning back on at medium effort.
Why
5a6720b88fixed thinking-off at the reporting site and in the zai profile. The transport branch atagent/transports/chat_completions.py:481was not part of that change and still overrides an explicitdisable.
The branch is reached when a reasoning-capable base_url resolves no profile.
_supports_reasoning_extra_body(
run_agent.py:5425) admits Nous Portal, GitHub Models, LM Studio, Ollama and OpenRouter; profile lookup iskeyed on the provider string, which is optional (
agent.provider = provider_name or "",agent/agent_init.py:436), andget_provider_profile("")returns None. So an OpenRouter or Nous Portalbase_url configured without a provider name lands here.
AIAgent's own docstring (agent/agent_init.py:382)documents
reasoning_configas an OpenRouter override that can be set to disable, which is the case thatbreaks.
Reproduced on
7cb2d2cd4:Every profiled sibling already honours the disable: nous omits the field
(
plugins/model-providers/nous/__init__.py:35), openrouter forwards the config(
plugins/model-providers/openrouter/__init__.py:158), and the Kimi branch twenty lines up maps disabled tothinking: {"type": "disabled"}(chat_completions.py:462). Each defaults to medium only in the absence ofan explicit config. The legacy branch applies that default to explicit configs too, including an explicit off.
For the wire value I followed
agent/auxiliary_client.py:6632, which emits{"enabled": False}for the samedisabled-and-no-profile-reasoning case. Nous is the exception: it rejects
enabled: false, so the field isomitted there, which is what the nous profile does.
The origin looks like an oversight rather than intent.
f69a33794("fix: forward reasoning_effort for customproviders (GLM-5.2 on ARK)") rewrote this branch to stop hardcoding medium effort and honour the caller's
effort; it did not carry the enabled flag through.
I did not find an open issue for this, so the reproduction above stands in for one.
Testing
Three contracts in
tests/agent/transports/test_chat_completions.py, alongside the existingtest_nous_omits_disabled_reasoning:test_disabled_reasoning_honored_without_profile: disabled stays disabled.test_disabled_reasoning_omitted_for_nous_without_profile: Nous omits the field.test_requested_effort_preserved_without_profile: an explicit effort survives the new branch.The first two fail on stock
mainand pass with the fix. The third passes either way and pins theelse-branch this change restructures.
tests/agent/transports/test_chat_completions.py: 93 passed. The widertests/agent/transportsrun has thesame 52 pre-existing failures before and after (unrelated codex and bedrock suites, missing deps in my env),
plus the 3 new tests.
Assertions are on the request kwargs. I have not driven a live profile-less endpoint.