Conversation
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR removes the /think command and session-level thinking toggle, making thinking solely driven by model config (thinking_budget). The refactoring is sound, but introduces 2 HIGH severity bugs that will break core functionality:
- Bedrock-Claude models silently ignore adaptive thinking - users configuring
thinking_budget: adaptivewill get no thinking at all - Gemini 3+ models with NoThinking will use default thinking - breaks title generation and other NoThinking use cases
Additionally, there's 1 MEDIUM severity issue with the new 'xhigh' effort level that may not be supported by the OpenAI API.
These issues should be addressed before merging.
| return nil | ||
| } | ||
| tokens := c.ModelConfig.ThinkingBudget.Tokens | ||
| if t, ok := c.ModelConfig.ThinkingBudget.EffortTokens(); ok { |
There was a problem hiding this comment.
🔴 HIGH: Adaptive thinking silently ignored for Bedrock Claude
The buildAdditionalModelRequestFields method checks ThinkingBudget.EffortTokens() which only maps 'minimal', 'low', 'medium', 'high' to token counts. For 'adaptive' or 'adaptive/' patterns, EffortTokens() returns (0, false), causing the code to skip thinking configuration entirely (line 308: if tokens <= 0 { return nil }). Users configuring thinking_budget: adaptive will get no thinking at all instead of adaptive thinking with the expected effort level.
Impact: Bedrock-Claude models silently ignore adaptive thinking configuration, breaking the feature for users who rely on Anthropic's adaptive thinking.
Recommendation: Either (a) return an error for adaptive on Bedrock with a clear message that Bedrock doesn't support adaptive thinking, or (b) map adaptive/ to token budgets using the anthropicEffortTokens helper (similar to the direct Anthropic client).
| modelConfig.MaxTokens = &mt | ||
| } | ||
| if tempOpts.NoThinking() { | ||
| modelConfig.ThinkingBudget = nil |
There was a problem hiding this comment.
🔴 HIGH: NoThinking() flag cleared by ThinkingBudget=nil causes Gemini 3+ models to use default thinking
When NoThinking() is true, line 31 sets modelConfig.ThinkingBudget = nil. However, in gemini/client.go buildConfig() (line 351+), there are two separate code paths:
- Lines 355-372 handle
NoThinking()(setting ThinkingBudget=0 for Gemini 2.5 or ThinkingLevel=Low for Gemini 3+) - Lines 373-379 handle
ThinkingBudget != nil(applying user-configured thinking)
After clone sets ThinkingBudget to nil, the buildConfig code will skip BOTH blocks because the NoThinking check at line 355 tests c.ModelOptions.NoThinking(), not whether ThinkingBudget is nil. For Gemini 3+ models that ALWAYS think, this means thinking config won't be applied at all, and the model will use its default thinking behavior instead of the minimal thinking requested by NoThinking.
Impact: Breaks title generation and other NoThinking use cases for Gemini 3+ models.
Recommendation: Preserve the NoThinking flag in the cloned ModelOptions, or ensure the Gemini client checks both NoThinking() and ThinkingBudget == nil.
| func openAIReasoningEffort(b *latest.ThinkingBudget) (string, error) { | ||
| effort := strings.TrimSpace(strings.ToLower(b.Effort)) | ||
| switch effort { | ||
| case "minimal", "low", "medium", "high", "xhigh": |
There was a problem hiding this comment.
🟡 MEDIUM: New 'xhigh' effort level may not be supported by OpenAI API
The openAIReasoningEffort function now accepts 'xhigh' as a valid effort level (line 903), but the error message at line 906 lists only 'minimal', 'low', 'medium', 'high' as valid values. The function returns shared.ReasoningEffort(effort) which is a string type from the SDK, but there's no documentation or tests confirming that the OpenAI API actually accepts 'xhigh'.
Impact: If the OpenAI API doesn't support 'xhigh', users configuring thinking_budget: {effort: xhigh} will get runtime errors. If it IS supported, the error message is misleading.
Recommendation: Either (a) verify 'xhigh' is supported by the OpenAI API and update the error message to include it, or (b) remove 'xhigh' from the switch case if it's not supported.
Thinking is now solely driven by the model's thinking_budget config. If a model has thinking_budget set or requires thinking (e.g. o-series), it uses thinking. There is no longer a per-session toggle. Removed: - /think slash command from TUI - Session.Thinking field and WithThinking option - options.WithThinking model option and applyOverrides - Thinking toggle from sidebar, server API, session state - ThinkingConfigured from agent and teamloader - All related handlers, messages, and tests Assisted-By: docker-agent
5 participants
Thinking is now solely driven by the model's thinking_budget config. If a model has thinking_budget set or requires thinking (e.g. o-series), it uses thinking. There is no longer a per-session toggle.
Removed:
Assisted-By: docker-agent