fix: omit think parameter for Ollama models without thinking support#67958
fix: omit think parameter for Ollama models without thinking support#67958Linux2010 wants to merge 4 commits into
Conversation
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟠 Untrusted workspace plugin manifests can inject arbitrary env var names into API key resolution (potential secret exfiltration)
Description
This allows an attacker-controlled workspace plugin (e.g., in an untrusted repo) to declare Impact:
Vulnerable logic:
Vulnerable code: if (plugin.origin !== "workspace" || params?.includeUntrustedWorkspacePlugins !== false) {
return true;
}
return isWorkspacePluginTrustedForProviderEnvVars(plugin, params?.config);RecommendationTreat workspace plugin manifests as untrusted by default for env-var candidate harvesting, and/or strictly constrain which env vars can be declared. Recommended changes:
Example default-deny fix: function shouldUsePluginProviderEnvVars(
plugin: PluginManifestRecord,
params: ProviderEnvVarLookupParams | undefined,
): boolean {
if (plugin.origin !== "workspace") return true;
// Default: do NOT trust workspace plugins for env var harvesting
if (params?.includeUntrustedWorkspacePlugins !== true) {
return isWorkspacePluginTrustedForProviderEnvVars(plugin, params?.config);
}
return true;
}Then ensure all call sites that should consider workspace plugins pass 2. 🟡 Potential chain-of-thought disclosure via Ollama `think` parameter enabled by heuristic model matching
DescriptionThe Ollama streaming wrapper enables the native Ollama In this codebase, those thinking deltas are turned into Key factors introduced/expanded by this change:
Vulnerable change site:
Vulnerable code: const supportsThinking = isReasoningModelHeuristic(ctx.modelId) || ctx.model?.reasoning === true;
if (supportsThinking && ctx.thinkingLevel === "off") {
streamFn = createOllamaThinkingWrapper(streamFn, false);
} else if (supportsThinking && ctx.thinkingLevel) {
streamFn = createOllamaThinkingWrapper(streamFn, true);
}RecommendationTreat chain-of-thought (“thinking”) as sensitive and do not enable it based on a loose model-id regex. Recommended fixes (pick one or combine):
const supportsThinking = ctx.model?.reasoning === true;
Analyzed PR: #67958 at commit Last updated on: 2026-04-22T13:26:45Z |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 434068aed7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const supportsThinking = isReasoningModelHeuristic(ctx.modelId); | ||
|
|
||
| if (supportsThinking && ctx.thinkingLevel === "off") { | ||
| streamFn = createOllamaThinkingWrapper(streamFn, false); | ||
| } else if (ctx.thinkingLevel) { | ||
| } else if (supportsThinking && ctx.thinkingLevel) { |
There was a problem hiding this comment.
Gate
think injection by capability, not name regex
This new gate uses isReasoningModelHeuristic(ctx.modelId) as the sole signal for thinking support, which excludes thinking-capable Ollama models whose IDs do not match r1|reasoning|think|reason (for example qwen3:*, which the plugin already treats as having "thinking" capability from /api/show). In those cases, thinkingLevel is now silently ignored because neither think=false nor think=true is added, so users lose control over native thinking mode on supported models.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes a 400 error that occurred when OpenClaw sent the
Confidence Score: 4/5Safe to merge for the reported bug; the narrow heuristic may silently regress qwen3/qwq thinking-mode users. The fix correctly solves the 400-error regression for non-thinking models and the tests are well-structured. Score is 4 rather than 5 because the heuristic regex misses qwen3 and qwq Ollama model families that support thinking, which is a silent behavior regression relative to the pre-PR behavior for those users. extensions/ollama/src/provider-models.ts — isReasoningModelHeuristic regex coverage Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/ollama/src/stream.ts
Line: 208
Comment:
**Heuristic may miss Qwen3 thinking models**
The regex `/r1|reasoning|think|reason/i` will return `false` for `qwen3:8b`, `qwen3:32b`, and `qwq:32b`, all of which support the `think` parameter in Ollama. Before this PR, those models would have received the `think` parameter because the guard didn't exist; after this PR they won't, silently disabling thinking for users with those models set to a non-`undefined` `thinkingLevel`.
The same heuristic already governs the catalog `reasoning` field (in `buildOllamaModelDefinition`), so this inconsistency pre-dates the PR — but adding `qwen3` (and possibly `qwq`) to the pattern here would avoid the regression. Consider updating `isReasoningModelHeuristic` itself to include `qwen3|qwq` so the catalog and stream wrapper stay aligned.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: omit think parameter for Ollama mod..." | Re-trigger Greptile |
| // Only apply the think parameter for models that are known to support thinking. | ||
| // Models like qwen2.5:0.5b don't support the think parameter at all and will | ||
| // return a 400 error if the parameter is sent (even with think=false). | ||
| const supportsThinking = isReasoningModelHeuristic(ctx.modelId); |
There was a problem hiding this comment.
The regex /r1|reasoning|think|reason/i will return false for qwen3:8b, qwen3:32b, and qwq:32b, all of which support the think parameter in Ollama. Before this PR, those models would have received the think parameter because the guard didn't exist; after this PR they won't, silently disabling thinking for users with those models set to a non-undefined thinkingLevel.
The same heuristic already governs the catalog reasoning field (in buildOllamaModelDefinition), so this inconsistency pre-dates the PR — but adding qwen3 (and possibly qwq) to the pattern here would avoid the regression. Consider updating isReasoningModelHeuristic itself to include qwen3|qwq so the catalog and stream wrapper stay aligned.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/ollama/src/stream.ts
Line: 208
Comment:
**Heuristic may miss Qwen3 thinking models**
The regex `/r1|reasoning|think|reason/i` will return `false` for `qwen3:8b`, `qwen3:32b`, and `qwq:32b`, all of which support the `think` parameter in Ollama. Before this PR, those models would have received the `think` parameter because the guard didn't exist; after this PR they won't, silently disabling thinking for users with those models set to a non-`undefined` `thinkingLevel`.
The same heuristic already governs the catalog `reasoning` field (in `buildOllamaModelDefinition`), so this inconsistency pre-dates the PR — but adding `qwen3` (and possibly `qwq`) to the pattern here would avoid the regression. Consider updating `isReasoningModelHeuristic` itself to include `qwen3|qwq` so the catalog and stream wrapper stay aligned.
How can I resolve this? If you propose a fix, please make it concise.Addresses review feedback from Codex/Greptile/Aisle on PR openclaw#67958
|
Updated Changes:
Commit: 0fa01fa |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fa01fa849
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const supportsThinking = isReasoningModelHeuristic(ctx.modelId); | ||
|
|
||
| if (supportsThinking && ctx.thinkingLevel === "off") { | ||
| streamFn = createOllamaThinkingWrapper(streamFn, false); | ||
| } else if (ctx.thinkingLevel) { | ||
| } else if (supportsThinking && ctx.thinkingLevel) { |
There was a problem hiding this comment.
Gate think injection by capability instead of name heuristic
createConfiguredOllamaCompatStreamWrapper now ignores thinkingLevel unless ctx.modelId matches a regex, so thinking-capable models with non-matching IDs silently lose think control. Fresh evidence in this commit: Ollama discovery already captures capabilities from /api/show (including "thinking") for models like glm-5.1:cloud, but this gate only checks isReasoningModelHeuristic(ctx.modelId), which does not match that ID. In those cases thinkingLevel is dropped and neither think=true nor think=false is sent.
Useful? React with 👍 / 👎.
0fa01fa to
9d38534
Compare
d76606b to
895407e
Compare
…y resolution The PROVIDER_ENV_API_KEY_CANDIDATES export was calling resolveProviderEnvApiKeyCandidates() at module load time, which triggered loadPluginManifestRegistry() before bundled plugins were fully available. This caused OpenRouter and other bundled provider auth env vars to be missing from the candidate map at startup. Fixes openclaw#67989: OpenRouter provider not routing requests to models The fix delegates to the existing lazy PROVIDER_AUTH_ENV_VAR_CANDIDATES proxy from provider-env-vars.ts, which defers manifest registry loading until first property access. This ensures bundled plugin metadata (including providerAuthEnvVars from manifests like OpenRouter's) is available when needed. Tests added to verify: - openrouter env var resolution works - lazy loading behavior is preserved
Addresses review feedback from Codex/Greptile/Aisle on PR openclaw#67958
Codex P2: isReasoningModelHeuristic alone misses models with explicit reasoning:true config. Also check ctx.model.reasoning. Refs: openclaw#67958
895407e to
82ab888
Compare
|
Closing this as implemented after Codex automated review. PR #67958 is obsolete for its linked Ollama bug. Current main and v2026.4.24 already address the reported qwen2.5 failure path by defaulting embedded runs to thinking off and forwarding native Ollama thinking control as top-level Best possible solution: Close this PR as implemented/obsolete for #67949. Keep the shipped v2026.4.24 native Ollama top-level What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Codex Review notes: model gpt-5.5, reasoning high; reviewed against d54d2d6b9b8a; fix evidence: release v2026.4.24, commit cbcfdf62c729. |
Summary
OpenClaw was sending a
thinkparameter to Ollama for all models whenthinkingLevelwas set, but some smaller models likeqwen2.5:0.5bdo not support thinking mode, causing a 400 error.Root Cause
The createConfiguredOllamaCompatStreamWrapper function was unconditionally adding a think parameter when ctx.thinkingLevel was set, but models without thinking capabilities reject this parameter entirely.
Changes
Fixes #67949