Skip to content

fix(thinking): disable thinking for unsupported Ollama models - #1376

Merged
kevincodex1 merged 2 commits into
Twigpine:mainfrom
chioarub:fix/1371-ollama-thinking
May 26, 2026
Merged

kevincodex1 merged 2 commits into
Twigpine:mainfrom
chioarub:fix/1371-ollama-thinking

Conversation

@chioarub

@chioarub chioarub commented May 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1371

Overview

This pull request addresses the invalid_request_error: "llama3.1:8b" does not support thinking API failure encountered when using Ollama models that lack reasoning capabilities.

By centralizing the thinking gate and evaluating it against the model actually used for the request attempt, OpenClaude avoids constructing Anthropic-side thinking parameters for unsupported Ollama routes, including retry and fallback paths.

Changes

  • Capability Gating: Introduced shouldUseThinkingForModel to consistently combine app-level thinking settings, the global thinking disable flag, and model capability support.
  • Ollama Default Handling: Unknown or catalog-missing Ollama models now safely default to false for thinking support.
  • Retry Resilience: API parameter construction evaluates thinking support against retryContext.model instead of the stale base request model.
  • Regression Coverage: Moved the Ollama regression test to the thinking-gate boundary so it fails if claude.ts starts constructing unsupported thinking params again.

Verification

  • bun test src/utils/thinking.test.ts src/services/api/openaiShim.test.ts
  • git diff --check

Fixes Twigpine#1371

- Adds central `shouldUseThinkingForModel` gate that checks the actual route and model descriptor.
- Disables thinking parameters for the Ollama route when the model is unknown or unsupported.
- Updates API requests to evaluate the actual retry model against the capability gate instead of the initial request model.
- Adds targeted tests for Ollama logic and shim payloads.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [P2] Replace the Ollama shim test with coverage for the actual thinking gate
    src/services/api/openaiShim.test.ts:4955
    This test asserts that the OpenAI-compatible request body does not contain thinking, but the shim already only writes body.thinking for routes whose thinkingRequestFormat is deepseek-compatible; Ollama does not set that option. That means this new test can pass even if the claude.ts thinking gate regresses and still constructs Anthropic-side thinking params for llama3.1:8b, so it does not protect the #1371 failure path. Please move the regression coverage to the boundary that changed here, such as queryModel/the params passed into createOpenAIShimClient, or directly cover shouldUseThinkingForModel with the Ollama route so the test fails without the new gate.

@chioarub

Copy link
Copy Markdown
Contributor Author

Addressed the requested test change in 0732cc58.

  • Removed the downstream Ollama openaiShim request-body test; it did not protect the When using OLLAMA Model llama3 getting getting does not support thinking #1371 path because the shim already suppresses body.thinking unless the route is deepseek-compatible.
  • Moved regression coverage to shouldUseThinkingForModel under the Ollama route, including llama3.1:8b while app-level thinking is enabled.
  • Added coverage for a catalog-missing local model name so Ollama cannot fall through the Claude 4 heuristic path.

Verification:

  • bun test src/utils/thinking.test.ts src/services/api/openaiShim.test.ts — 108 pass, 0 fail
  • git diff --check

I also sanity-checked the regression shape by temporarily removing the model capability check from shouldUseThinkingForModel; the new test failed before restoring the fix.

@chioarub
chioarub requested a review from jatmn May 26, 2026 17:25

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.

@kevincodex1
kevincodex1 merged commit 8513178 into Twigpine:main May 26, 2026
2 checks passed
discopops pushed a commit to discopops/openclaude that referenced this pull request May 28, 2026
…ne#1376)

* fix(thinking): disable thinking for unsupported Ollama models

Fixes Twigpine#1371

- Adds central `shouldUseThinkingForModel` gate that checks the actual route and model descriptor.
- Disables thinking parameters for the Ollama route when the model is unknown or unsupported.
- Updates API requests to evaluate the actual retry model against the capability gate instead of the initial request model.
- Adds targeted tests for Ollama logic and shim payloads.

* test(thinking): cover Ollama thinking gate
Gravirei added a commit to Gravirei/openclaude that referenced this pull request May 28, 2026
- fix(autocompact): retry circuit breaker after cooldown (Twigpine#1375)
- fix(provider): require API key input when adding OpenGateway (Twigpine#1384)
- fix(provider): allow remote Ollama without OPENAI_API_KEY (Twigpine#952)
- fix(codex-stream): recover tool args delivered only via done events (Twigpine#1262)
- fix: route MiniMax compacting through Anthropic-compatible API (Twigpine#1154)
- fix(thinking): disable thinking for unsupported Ollama models (Twigpine#1376)
- feat(agents): set active session agent from agents menu (Twigpine#1349)
- fix(repl): show permission prompts while draft input is present (Twigpine#1393)
- fix(model): include profile models in descriptor picker (Twigpine#1361)
- Improve warning notice formatting (Twigpine#1415)
- fix(codex): allow credential storage fallback (Twigpine#1347)
- fix(attribution): make git attribution opt-in by default (Twigpine#1335)
- fix(agent): allow custom model overrides (Twigpine#1337)
- feat(query): robust multi-lingual and structural continuation nudge (Twigpine#1280)
- fix(watchers): debounce skills and settings reload bursts (Twigpine#1370)
- feat: configure API retry backoff (Twigpine#370) (Twigpine#1095)
- chore(main): release 0.15.0 (Twigpine#1325)
- ci: retrigger CodeQL after action download outage (Twigpine#1374)
- Fix launcher heap setup for long sessions (Twigpine#1242)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When using OLLAMA Model llama3 getting getting does not support thinking

3 participants