fix: resolve ollama thinking support per model before sending reasoning_effort - #6053
Merged
Conversation
…ng_effort (#6050) A reviewer pinned to `ollama[<non-thinking model>]~effort=low` sent `reasoning_effort` on its first request, which ollama translates into its `thinking` parameter and hard-rejects with a 400 for a model that does not advertise thinking support. The existing recovery retried without the field, but only reactively: the "this model has no thinking" memo lives in a process-local Map, and every reviewer invocation from a claim or PR run is its own short-lived `node` process — so each review call re-uploaded the entire diff twice, once for a request that could never succeed. Ollama's `/api/show` reports the capability set per model, and PortOS already reads it (`ollamaManager.getModelCapabilities`, cached). Consult it before the request and omit `reasoning_effort` for a model that lacks `thinking`, so the doomed request is never sent. The 400-retry stays as the fail-safe for a backend with no such probe (LM Studio, MTPLX) and for a probe that cannot answer. `null` (probe failed) and `[]` (daemon answered with no capabilities) both mean *unknown*, not *unsupported* — collapsing them would silently strip a level a reasoning model does accept, so both fall through to the request. The probe is also skipped entirely when no effort is pinned, since there is nothing to drop; that also stops `effortUnsupported` being reported when the caller never asked for a level. The `runLocalCodeReview` doc comment now says the omission is decided per model, matching the behavior. Closes #6050
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A local reviewer pinned to
ollama[<non-thinking model>]~effort=lowsentreasoning_efforton its first request. Ollama translates that field into its ownthinkingparameter and hard-rejects the whole request with a 400 for a model that does not advertise thinking support.PR #5960 added a reactive recovery (retry without the field), but the "this model has no thinking" memo lives in a process-local
Map— and every reviewer invocation from a claim/PR run is its own short-livednodeprocess. So the cache never survived to the next call, and every review re-uploaded the entire diff twice: once for a request that could never succeed.Ollama's
/api/showreports the capability set per model, and PortOS already reads it (ollamaManager.getModelCapabilities, cached). This resolves it before the request and omitsreasoning_effortfor a model lackingthinking, so the doomed request is never sent. The 400-retry stays as the fail-safe for a backend with no such probe (LM Studio, MTPLX) and for a probe that cannot answer.Sentinel handling follows the repo's absent-vs-empty rule:
null(probe failed) and[](daemon answered reporting no capabilities) both mean unknown, not unsupported, so both fall through to the request rather than silently stripping a level a reasoning model does accept.The probe is skipped entirely when no effort is pinned — nothing to drop, so a capability round-trip buys nothing. That also stops
effortUnsupported: truebeing reported when the caller never asked for a level.The
runLocalCodeReviewdoc comment now states the omission is decided per model, not per backend, matching the behavior (this was called out in the issue as the contract the implementation had drifted from).Test plan
server/services/codeReview.test.js— 6 new cases pinning the body shape both ways and the fall-through paths:reasoning_effortabsent on the first request when/api/showomitsthinking(fails onmain, passes here — one fetch, not two)reasoning_effortpresent when/api/showreportsthinking[]capabilities treated as unknown → level still senteffortUnsupportedflagok:falsewith400 ... does not support thinking; nowok:truewith findings and no retry warning, confirming the first request was already correct.Closes #6050