-
Notifications
You must be signed in to change notification settings - Fork 251
Move ModelSupportsReasoning calls to async bubbletea commands #1749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -328,32 +328,41 @@ func (a *appModel) handleToggleYolo() (tea.Model, tea.Cmd) { | |
| } | ||
|
|
||
| func (a *appModel) handleToggleThinking() (tea.Model, tea.Cmd) { | ||
| // Check if the current model supports reasoning | ||
| if a.cancelThinkingCheck != nil { | ||
| a.cancelThinkingCheck() | ||
| } | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| a.cancelThinkingCheck = cancel | ||
|
|
||
| currentModel := a.application.CurrentAgentModel() | ||
| // TODO: this can block for up to 30s on the first call if the cache is cold, | ||
| // which freezes the TUI. Move to an async command. | ||
| if !modelsdev.ModelSupportsReasoning(context.TODO(), currentModel) { | ||
| return a, func() tea.Msg { | ||
| supported := modelsdev.ModelSupportsReasoning(ctx, currentModel) | ||
| return messages.ToggleThinkingResultMsg{Supported: supported} | ||
| } | ||
| } | ||
|
|
||
| func (a *appModel) handleToggleThinkingResult(msg messages.ToggleThinkingResultMsg) (tea.Model, tea.Cmd) { | ||
| if !msg.Supported { | ||
| return a, notification.InfoCmd("Thinking/reasoning is not supported for the current model") | ||
| } | ||
|
|
||
| sess := a.application.Session() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Session state race condition The
This causes the thinking flag to be applied to the wrong session. Suggested fix: Capture the session ID when starting the async check and include it in func (a *appModel) handleToggleThinking() (tea.Model, tea.Cmd) {
// ...
sessionID := a.application.Session().ID
return a, func() tea.Msg {
supported := modelsdev.ModelSupportsReasoning(ctx, currentModel)
return messages.ToggleThinkingResultMsg{
Supported: supported,
SessionID: sessionID,
}
}
}
func (a *appModel) handleToggleThinkingResult(msg messages.ToggleThinkingResultMsg) (tea.Model, tea.Cmd) {
sess := a.application.Session()
if sess.ID != msg.SessionID {
return a, nil // Session changed, discard result
}
// ... rest of logic
}
``` <!-- cagent-review --> |
||
| sess.Thinking = !sess.Thinking | ||
| a.sessionState.SetThinking(sess.Thinking) | ||
|
|
||
| // Persist the change to the database immediately | ||
| if store := a.application.SessionStore(); store != nil { | ||
| if err := store.UpdateSession(context.Background(), sess); err != nil { | ||
| return a, notification.ErrorCmd(fmt.Sprintf("Failed to save session: %v", err)) | ||
| } | ||
| } | ||
|
|
||
| var msg string | ||
| var infoMsg string | ||
| if sess.Thinking { | ||
| msg = "Thinking/reasoning enabled for this session" | ||
| infoMsg = "Thinking/reasoning enabled for this session" | ||
| } else { | ||
| msg = "Thinking/reasoning disabled for this session" | ||
| infoMsg = "Thinking/reasoning disabled for this session" | ||
| } | ||
| return a, notification.InfoCmd(msg) | ||
| return a, notification.InfoCmd(infoMsg) | ||
| } | ||
|
|
||
| func (a *appModel) handleToggleHideToolResults() (tea.Model, tea.Cmd) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale reasoning support status displayed when switching agents
The guard
if msg.modelID == m.agentModelcorrectly prevents stale async results from updating the wrong agent's state. However, whenSetAgentInfois called with a new agent, thereasoningSupportedflag retains the value from the previous agent until the new async check completes.Scenario:
reasoningSupported = falsetrue(fail-open), so we should reset to this defaultSuggested fix: Reset
reasoningSupportedto the fail-open default when setting a new agent: