Skip to content

[ENHANCEMENT] Ollama native thinking: follow-up improvements from PR #832 review #846

Description

@edelauna

Problem

PR #832 added native thinking/reasoning support for Ollama. The review identified several follow-up improvements that don't block the feature but should be addressed to prevent subtle bugs and keep the codebase consistent with cross-provider patterns.

Context (who is affected and when)

Affects users of the Ollama provider with native thinking enabled (particularly with models like qwen3 that may emit reasoning via both channels), and future contributors working on the provider settings UI.

Desired behavior

  1. No duplicate reasoning output when a model emits via both message.thinking and <think> tags in message.content simultaneously.
  2. ThinkingBudget's internal state sync effects cannot conflict with Ollama's outer "Enable Thinking" checkbox.
  3. Code is consistent with cross-provider patterns for guard logic, return types, and test organization.

Acceptance criteria

Given a reasoning model that emits thinking in both message.thinking and <think> content tags simultaneously
  When native thinking is enabled
  Then reasoning is displayed once, not twice in the UI
  And disabling the checkbox fully stops the think parameter from being sent

Given the "Enable Thinking" checkbox is checked and ThinkingBudget is visible
  When ThinkingBudget's internal sync effects fire on mount
  Then enableReasoningEffort is not inadvertently overwritten by ThinkingBudget

Proposed approach

  • native-ollama.ts:391 — Skip the TagMatcher content pass for a chunk when message.thinking is non-empty, preventing dual-channel double-yield:

    if (chunk.message.thinking?.length) {
      yield { type: "reasoning", text: chunk.message.thinking }
    } else if (chunk.message.content?.length) {
      for (const c of matcher.update(chunk.message.content)) yield c
    }
  • Ollama.tsx:165 — Proxy setApiConfigurationField passed to ThinkingBudget (match the pattern in OpenAICompatible.tsx:256-278) so ThinkingBudget's internal enableReasoningEffort sync effects cannot override the outer checkbox.

  • native-ollama.ts:292 — Add a comment explaining why "none"/"minimal" map to boolean true (Ollama-specific deviation from the cross-provider spec in ThinkingBudget.tsx:15-19).

  • native-ollama.ts:317 — Return { chatOptions, thinkParam } from buildChatRequestOptions() instead of a positional tuple, consistent with other providers.

  • native-ollama.spec.ts:657, 747 — Move 4 think-param tests into their correct describe("createMessage") / describe("completePrompt") blocks.

Trade-offs / risks

The dual-channel guard changes behavior for models that currently get both channels active simultaneously — but duplicate reasoning is always wrong output, so the change is safe. The TagMatcher fallback continues to serve models that don't use native thinking (e.g. deepseek-r1 with <think> tags).


Identified during review of #832.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions