Skip to content

Local reviewer sends reasoning_effort to every ollama model, 400-ing on models without thinking support #6050

Description

@atomantic

Problem

runToolFreeLocalCompletion in server/services/codeReview.js attaches reasoning_effort to the OpenAI-compatible request whenever normalizeReviewerEffort(effort, backend) returns a level:

const resolvedEffort = normalizeReviewerEffort(effort, backend) || null
...
...(resolvedEffort ? { reasoning_effort: resolvedEffort } : {}),

That gate is keyed on the backend, not on the model. Ollama translates reasoning_effort into its thinking parameter and hard-rejects it for a model that does not advertise thinking support:

ollama API error 400: {"error":{"message":"\"<model>\" does not support thinking",
"type":"invalid_request_error","param":null,"code":null}}

So a claim/PR run pinned to ollama[<non-thinking model>]~effort=low can never get a verdict: run-local-code-review.mjs exits 1 with ok:false, and a REQUIRED reviewer configured that way makes every run review-blocked, leaving PRs open and unmergeable. Reproduced deterministically (two consecutive attempts) on a real claim run against a gemma-family coder GGUF; PR #6049 is the PR it blocked.

The doc comment on runLocalCodeReview already states the intended behavior — "Omitted from the body entirely when unset or not a level this backend accepts — a non-reasoning [model] …" — so this is the implementation not matching its own documented contract, and the contract itself should say model, not backend.

Note the failure is easy to miss in the claim-comment path: runLocalClaimCommentReview returns early on an empty comment list before it ever reaches the API, so the tool-free comment gate passes with the same model/effort that then 400s on the code review.

Work

  • Resolve thinking support per model before attaching reasoning_effort. PortOS already knows this: server/services/localLlm.js maps the ollama capability thinking -> reasoning (see the capability map around line 580), so the model list carries the answer.
  • When the resolved model does not support it, drop reasoning_effort from the body and proceed with the review rather than failing — a non-reasoning model still returns usable findings.
  • Fail closed only when the capability genuinely cannot be resolved AND the request still errors; keep the existing ok:false shape for that case so review-blocked still works.
  • Correct the runLocalCodeReview doc comment to say the omission is decided per model.

Acceptance criteria

  • runLocalCodeReview({ backend: 'ollama', model: <non-thinking model>, effort: 'low', diff }) returns ok:true with findings instead of a 400.
  • A test pins the body shape both ways: reasoning_effort present for a thinking-capable model, absent for one without the capability.
  • A model whose capabilities cannot be fetched does not regress the current behavior for thinking-capable models.

Files

  • server/services/codeReview.js (runToolFreeLocalCompletion, runLocalCodeReview, normalizeReviewerEffort)
  • server/services/localLlm.js (capability map, already present)
  • server/services/codeReview.test.js

Activity

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

Metadata

Metadata

Assignees

Labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions