feat: add toggle for model thinking capability#342
Conversation
Support per-model thinking overrides with registry-aware injection, TUI/CLI persistence, and defense-in-depth validation for extra_body and extra_headers.
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func ValidateModelsThinking(m map[string]string) error { | ||
| for model, mode := range m { | ||
| if strings.TrimSpace(model) == "" { | ||
| return fmt.Errorf("models_thinking keys must be non-empty model names") | ||
| } | ||
| if err := ValidateModelsThinkingMode(mode); err != nil { | ||
| return fmt.Errorf("models_thinking[%q]: %w", model, err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
ValidateModelsThinking does not detect case-insensitive duplicate keys. A hand-edited config with entries like "Model-A": "on" and "model-a": "off" passes validation, but LookupModelsThinkingMode will arbitrarily pick one via sorted fallback. Since NormalizeModelsThinking (which deduplicates) is only called during CLI set operations, manually edited configs can silently have conflicting entries.
Consider adding case-insensitive duplicate detection here so that ambiguous configs are rejected early with a clear error message, rather than relying on downstream normalization and silent fallback selection.
Suggestion:
| func ValidateModelsThinking(m map[string]string) error { | |
| for model, mode := range m { | |
| if strings.TrimSpace(model) == "" { | |
| return fmt.Errorf("models_thinking keys must be non-empty model names") | |
| } | |
| if err := ValidateModelsThinkingMode(mode); err != nil { | |
| return fmt.Errorf("models_thinking[%q]: %w", model, err) | |
| } | |
| } | |
| return nil | |
| } | |
| func ValidateModelsThinking(m map[string]string) error { | |
| seen := make(map[string]string, len(m)) | |
| for model, mode := range m { | |
| if strings.TrimSpace(model) == "" { | |
| return fmt.Errorf("models_thinking keys must be non-empty model names") | |
| } | |
| if err := ValidateModelsThinkingMode(mode); err != nil { | |
| return fmt.Errorf("models_thinking[%q]: %w", model, err) | |
| } | |
| key := strings.ToLower(strings.TrimSpace(model)) | |
| if prev, ok := seen[key]; ok { | |
| return fmt.Errorf("models_thinking has duplicate case-insensitive key %q (conflicts with %q)", model, prev) | |
| } | |
| seen[key] = model | |
| } | |
| return nil | |
| } |
There was a problem hiding this comment.
Adopted! ValidateModelsThinking now rejects case-insensitive duplicate keys (e.g. "Model-A" and "model-a") at validation time with an ambiguous keys ... (case-insensitive duplicate) error, instead of letting hand-edited config pass validation and leaving LookupModelsThinkingMode to pick a winner at runtime.
The CLI config set path still goes through NormalizeModelsThinking / MergeModelsThinking for normalization; this change mainly closes the gap for resolver load and hand-edited JSON. Added TestValidateModelsThinking_RejectsCaseInsensitiveDuplicateKeys.
| for k := range m { | ||
| if isReservedExtraBodyKey(strings.ToLower(k)) { |
There was a problem hiding this comment.
Redundant double-lowering: strings.ToLower(k) is called here, and then isReservedExtraBodyKey calls strings.ToLower(key) again internally. Either remove the strings.ToLower call here and let isReservedExtraBodyKey handle it, or remove the strings.ToLower inside isReservedExtraBodyKey and document that callers must pass pre-lowered input. The current approach works correctly but is wasteful and inconsistent with how ValidateExtraHeadersMap handles it (it lowercases once inline without a helper).
Suggestion:
| for k := range m { | |
| if isReservedExtraBodyKey(strings.ToLower(k)) { | |
| for k := range m { | |
| if isReservedExtraBodyKey(k) { |
There was a problem hiding this comment.
Adopted! ValidateExtraBody now calls isReservedExtraBodyKey(k) directly and drops the extra outer strings.ToLower. Behavior is unchanged; this is a small cleanup.
| case manualStepExtraHeaders: | ||
| extraHeaders, err := parseExtraHeadersInput(m.manualExtraHeadersInput.Value()) | ||
| if err != nil { | ||
| m.formError = "" | ||
| return m, nil | ||
| } | ||
| m.manualExtraHeadersInput.Blur() | ||
| m.formError = "" | ||
| m.manualExtraHeaders = extraHeaders | ||
| m.confirmed = true | ||
| return m, tea.Quit |
There was a problem hiding this comment.
Bug: Unlike the custom provider form's cpStepExtraHeaders case (which re-parses cpExtraBodyInput to catch edits made after navigating back), the manual form's manualStepExtraHeaders case does NOT re-parse manualExtraBodyInput.
If a user presses Esc on the Extra Headers step to go back to Extra Body, edits the body text, then presses Enter twice to advance through both steps again, the stale m.manualExtraBody from the first pass is retained and ultimately returned by result() (line 2108). The freshly edited text in manualExtraBodyInput is never parsed.
Fix: Re-parse manualExtraBodyInput.Value() at the start of manualStepExtraHeaders, similar to how cpStepExtraHeaders handles it. If parsing fails, navigate back to manualStepExtraBody.
Suggestion:
| case manualStepExtraHeaders: | |
| extraHeaders, err := parseExtraHeadersInput(m.manualExtraHeadersInput.Value()) | |
| if err != nil { | |
| m.formError = "" | |
| return m, nil | |
| } | |
| m.manualExtraHeadersInput.Blur() | |
| m.formError = "" | |
| m.manualExtraHeaders = extraHeaders | |
| m.confirmed = true | |
| return m, tea.Quit | |
| case manualStepExtraHeaders: | |
| // Re-parse extra body to pick up any edits made after navigating back. | |
| extraBody, err := parseExtraBodyInput(m.manualExtraBodyInput.Value()) | |
| if err != nil { | |
| m.formError = "" | |
| m.manualExtraHeadersInput.Blur() | |
| m.manualStep = manualStepExtraBody | |
| return m, m.focusManualStep() | |
| } | |
| m.manualExtraBody = extraBody | |
| extraHeaders, err := parseExtraHeadersInput(m.manualExtraHeadersInput.Value()) | |
| if err != nil { | |
| m.formError = "" | |
| return m, nil | |
| } | |
| m.manualExtraHeadersInput.Blur() | |
| m.formError = "" | |
| m.manualExtraHeaders = extraHeaders | |
| m.confirmed = true | |
| return m, tea.Quit |
There was a problem hiding this comment.
Adopted! manualStepExtraHeaders now matches cpStepExtraHeaders: on final Enter it re-parses extra body from manualExtraBodyInput instead of relying only on cached m.manualExtraBody. On parse failure it falls back to manualStepExtraBody, keeps inline draft hints, and leaves formError empty.
Added TestProviderTUI_ManualFormExtraHeadersReparsesExtraBodyFromInput and TestProviderTUI_ManualFormExtraHeadersFallsBackToExtraBodyOnParseError.
| if err := llm.ValidateExtraBody(m); err != nil { | ||
| return fmt.Errorf("invalid llm.extra_body: %w", err) | ||
| } |
There was a problem hiding this comment.
Potential backward compatibility break: ValidateExtraBody is newly introduced here and in the resolver. It rejects top-level keys like max_tokens, max_completion_tokens, stream, system, etc. Users who previously stored these in extra_body (a common pattern for setting token limits or streaming preferences) will now get errors both when setting config via CLI and at runtime resolution.
Consider either:
- Adding a migration/warning period that logs deprecation notices before hard-blocking.
- Documenting this breaking change prominently in release notes.
- Or at minimum, providing a clear error message that suggests alternative config fields (e.g., "use the dedicated
modelfield instead of extra_body.model").
| entry.ModelsThinking = llm.PruneModelsThinking(entry.ModelsThinking, allowedModelsForThinking(false, cfg.Provider, entry)) | ||
| cfg.CustomProviders[cfg.Provider] = entry |
There was a problem hiding this comment.
Inconsistent pruning feedback: In applyCustomProviderConfig (lines 197-201), a warning is printed to stderr when models_thinking entries are pruned. However, here in runConfigModel for custom providers, pruning happens silently. The same silent pruning occurs in applyOfficialProviderConfig (line 281) and the official provider branch below (line 401).
Consider adding the same pruning warning consistently across all call sites, so users are informed when their models_thinking entries are discarded after a model change. For example:
beforeThinking := len(entry.ModelsThinking)
entry.ModelsThinking = llm.PruneModelsThinking(entry.ModelsThinking, allowedModelsForThinking(false, cfg.Provider, entry))
if beforeThinking > len(entry.ModelsThinking) {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: some models_thinking entries were pruned for custom provider %q\n", cfg.Provider)
}… body re-parse. Reject case-insensitive duplicate model keys at validation time, remove redundant ToLower in ValidateExtraBody, and align manual extra-headers step with custom form by re-parsing extra body from input.
… TUI. Drop Extra Body and Extra Headers wizard steps and read-only hints; preserve existing extra fields on manual save and custom edit. CLI config set and Official thinking toggle are unchanged.
Drop reserved-field checks for extra_body (JSON validity only) and
remove resolver load-time extra_headers validation. Fix cloneExtraBodyMap
so empty maps clone to {} instead of nil.
1 participant
Description
Support per-model thinking overrides with registry-aware injection, TUI/CLI persistence, and defense-in-depth validation for extra_body and extra_headers.
1.1 thinking: on
1.2 thinking: off
1.3 thinking: always on (e.g., kimi-k2.7-code)
2.1 extra_body
2.2 extra_headers
Type of Change
How Has This Been Tested?
make testpasses locallyChecklist
go fmt,go vet)Related Issues