Avoid fatal TUI errors on skills list failure#18061
Conversation
| }) | ||
| .await?; | ||
| self.handle_skills_list_response(response); | ||
| self.handle_skills_list_result( |
There was a problem hiding this comment.
Shouldn't this take the return value and return it with Ok instead of hardcoding Ok(true) below?
There was a problem hiding this comment.
[codex] I agree the ignored return value is awkward, but I don't think we should propagate false here. In this method, Ok(false) means "this op was not handled" and would fall through to a user-visible "Not available in TUI yet" message. For a failed skills refresh, we want the op to count as handled while the refresh itself degrades gracefully. I'll remove the helper's boolean return instead so this is clearer.
| app_server: &mut AppServerSession, | ||
| thread_id: ThreadId, | ||
| op: &AppCommand, | ||
| ) -> Result<bool> { |
There was a problem hiding this comment.
Should this be forced to return bool rather than Result?
There was a problem hiding this comment.
[codex] I think try_submit_active_thread_op_via_app_server should stay Result<bool> because most branches still represent important app-server failures that should bubble up. The narrower fix is to make only skills/list fail-open, since plugin/skills discovery is noncritical. I'll change the skills helper to return () so the intent is explicit without broadening error swallowing.
| } | ||
| } | ||
|
|
||
| fn handle_skills_list_result( |
There was a problem hiding this comment.
To make this a more meaningful helper, maybe this should take app_server, cwds, and force_reload and make the skills_list() call and then handle the Result internally?
Up to you: I don't feel strongly either way.
2 participants
Addresses #17951
Problem: The TUI treated skills/list failures as fatal during refresh, so proxy/firewall responses that break plugin discovery could crash the session.
Solution: Route startup and refresh skills/list responses through shared graceful handling that logs a warning and keeps the TUI running.