fix(ollama): send think=false by default so hybrid-reasoning models actually stop thinking - #2103
II-II-II-II wants to merge 1 commit into
Conversation
…ctually stop thinking buildOllamaChatBody() never set a top-level `think` field on outgoing requests. Ollama's hybrid-thinking models (e.g. Qwen3.x) interpret the field's absence as thinking enabled, so every Ollama request ran in thinking mode regardless of --effort, with no way to disable it. The existing client-side thinking filter only hides the trace from display — the model still generates the full reasoning chain either way, so the latency cost was paid regardless of whether it was visible. Mirrors the effort-handling already implemented for the Anthropic/Gemini paths just above this function: no --effort specified maps to think: false; an explicit effort level is passed through (xhigh/max/ ultracode collapse to Ollama's "high", matching its three-tier scale). Measured on qwen3.6:35b-a3b-coding (Ollama, Apple Silicon): - Before: 15-21s per trivial response, full visible reasoning trace - After: ~1.3s via the raw Ollama API with think=false, ~5s through the full CLI (Node startup + OpenAI<->Ollama format conversion overhead) - No visible reasoning trace in either case after the fix
📝 WalkthroughWalkthroughThe Ollama request planner now adds a ChangesOllama thinking control
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/openaiShim/requestPlanner.ts`:
- Around line 433-444: Update the native Ollama assertions in the request
planner tests to include think: false for absent reasoning effort, then add
focused cases covering preserved effort values and mapping xhigh, max, and
ultracode to high. Anchor the cases to the request-planning test flow exercising
the think value derived from request.reasoning.effort.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f4e8926-b041-4bfc-9501-e803f9be0cd0
📒 Files selected for processing (1)
src/services/api/openaiShim/requestPlanner.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and ESM imports throughout the source code.
Run
bun run typecheckandbun run typecheck:type-testsfor TypeScript changes when applicable.
Files:
src/services/api/openaiShim/requestPlanner.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use React and Ink patterns for terminal UI components.
Files:
src/services/api/openaiShim/requestPlanner.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color andexecafor child-process execution when those capabilities are needed.
Files:
src/services/api/openaiShim/requestPlanner.ts
src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use existing service and provider integration patterns when implementing API, MCP, OAuth, wiki, voice, or related integrations.
Files:
src/services/api/openaiShim/requestPlanner.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add new Python code, Python provider paths, or Python dependencies without explicit maintainer approval.
**/*.{ts,tsx,js,jsx}: Follow the existing code style in touched source files, prefer small readable changes, avoid unrelated reformatting, and keep comments useful and concise.
Preserve existing repository patterns unless intentionally refactoring them, and avoid broad rewrites or unnecessary generated changes.
Review AI-assisted code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting it.
Files:
src/services/api/openaiShim/requestPlanner.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when setup, commands, or user-facing behavior changes.
Files:
src/services/api/openaiShim/requestPlanner.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/openaiShim/requestPlanner.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim/requestPlanner.ts
| const think: boolean | string = request.reasoning?.effort | ||
| ? ['xhigh', 'max', 'ultracode'].includes(request.reasoning.effort) | ||
| ? 'high' | ||
| : request.reasoning.effort | ||
| : false | ||
|
|
||
| return { | ||
| model: request.resolvedModel, | ||
| messages: normalizeOllamaNativeMessages(body.messages), | ||
| stream: params.stream ?? false, | ||
| options, | ||
| think, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the native Ollama test contract before merge.
The exact-body assertion in src/services/api/openaiShim/requestPlanner.test.ts, Lines 307-341, omits think. The no-effort case now returns think: false, so that assertion will fail. Update it and add focused cases for preserved effort values and xhigh, max, and ultracode mapping to 'high'.
As per path instructions, AGENTS.md requires focused tests covering absent effort (false), preserved effort values, and xhigh/max/ultracode mapping to high.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/api/openaiShim/requestPlanner.ts` around lines 433 - 444, Update
the native Ollama assertions in the request planner tests to include think:
false for absent reasoning effort, then add focused cases covering preserved
effort values and mapping xhigh, max, and ultracode to high. Anchor the cases to
the request-planning test flow exercising the think value derived from
request.reasoning.effort.
Source: Path instructions
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore the native-Ollama planner test and cover the new contract
src/services/api/openaiShim/requestPlanner.test.ts:330
The only exact-payload assertion for this path still expects nothinkkey, while this PR now always serializesthink: false.bun test src/services/api/openaiShim/requestPlanner.test.tsfails deterministically (11 passing, 1 failing) with that extra key. This is also the unresolved CodeRabbit request: update the default assertion and add focused cases for the preserved levels and the normalized levels before this can be merged. -
[P1] Wire native Ollama into the effort-control path instead of always disabling it
src/services/api/openaiShim/requestPlanner.ts:433
The planner readsrequest.reasoning?.effort, butgetAnthropicClient()only supplies that override whenmodelSupportsShimReasoningEffort()is true. The Ollama route has no controllable reasoning metadata, so ordinary discovered models (including the Qwen3-style models this PR targets) fail that gate and arrive here with no effort. Consequently/effort high/--effort highstill emitsthink: falserather than enabling or selecting thinking, which contradicts the PR's stated behavior. Add an Ollama-specific controllable capability/serialization route and an end-to-end captured-request test. -
[P1] Preserve an explicit
?thinking=enabledrequest
src/services/api/openaiShim/requestPlanner.ts:433
resolveProviderRequest()preserves?thinking=enabledasrequest.thinking, but this new branch ignores it and falls back tofalsewhenever no effort is present. A native-Ollama user who selects--model 'qwen3?thinking=enabled'is therefore forcibly switched from the requested enabled mode to disabled. Resolve the explicit thinking state before applying the default; the native API accepts booleanthinkvalues.
2 participants
I have read
CONTRIBUTING.mdandAGENTS.md.What changed and why
buildOllamaChatBody()never set a top-levelthinkfield on outgoing Ollama requests. Ollama's hybrid-thinking models (e.g. Qwen3.x) interpret the field's absence as thinking enabled — so every Ollama request ran in thinking mode regardless of--effort, with no way to disable it. The existing client-side thinking filter only hides the trace from display; the model still generates the full reasoning chain either way, so the latency cost was paid whether or not it was visible.This mirrors the effort-handling already implemented for the Anthropic/Gemini paths just above this function in the same file: no
--effortspecified →think: false; an explicit effort level is passed through (xhigh/max/ultracodecollapse to Ollama's"high", matching its three-tier scale).Impact
Any user running a hybrid-thinking model through the Ollama provider gets a working, silent-by-default non-thinking mode instead of an unremovable reasoning trace on every request.
Measured on
qwen3.6:35b-a3b-coding(Apple Silicon, Ollama):think=false, ~5s through the full CLI (Node startup + OpenAI↔Ollama format conversion overhead) — no reasoning trace in either caseChecks run
bun run typecheck— cleanbun run check— 21 pre-existing failures, confirmed present on unmodifiedmain(same count before and after this change, all unrelated model-context-window tests, not touched by this diff)bun run smoke— builds and runs cleanlyProvider path tested
Ollama only — this change is scoped to
buildOllamaChatBody()and doesn't touch other providers.No linked issue — searched open/closed issues and PRs first (including #1442/#1443, a related but different and unmerged proposal for adaptive thinking budgets); didn't find an existing report of this specific bug, so per
CONTRIBUTING.mdthis is submitted as a small, self-explanatory fix.Summary by CodeRabbit