Skip to content

Avoid fatal TUI errors on skills list failure #18061

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.

Sign up for GitHub

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

Merged
etraut-openai merged 2 commits into from
Apr 16, 2026
Merged

Avoid fatal TUI errors on skills list failure #18061

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0d2d66
Avoid fatal TUI errors on skills list failure
etraut-openai Apr 16, 2026
4e293b9
Clarify skills list failure handling
etraut-openai Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 33 additions & 19 deletions codex-rs/tui/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2486,14 +2486,16 @@ impl App {
Ok(true)
}
AppCommandView::ListSkills { cwds, force_reload } => {
let response = app_server
.skills_list(codex_app_server_protocol::SkillsListParams {
cwds: cwds.to_vec(),
force_reload,
per_cwd_extra_user_roots: None,
})
.await?;
self.handle_skills_list_response(response);
self.handle_skills_list_result(
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this take the return value and return it with Ok instead of hardcoding Ok(true) below?

Copy link
Copy Markdown
Collaborator Author

@etraut-openai etraut-openai Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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
.skills_list(codex_app_server_protocol::SkillsListParams {
cwds: cwds.to_vec(),
force_reload,
per_cwd_extra_user_roots: None,
})
.await,
"failed to refresh skills",
);
Ok(true)
}
AppCommandView::Compact => {
Expand Down Expand Up @@ -2567,6 +2569,19 @@ impl App {
}
}

fn handle_skills_list_result(
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&mut self,
result: Result<SkillsListResponse>,
failure_message: &str,
) {
match result {
Ok(response) => self.handle_skills_list_response(response),
Err(err) => {
tracing::warn!("{failure_message}: {err:#}");
}
}
}

async fn try_resolve_app_server_request(
&mut self,
app_server: &AppServerSession,
Expand Down Expand Up @@ -3973,17 +3988,16 @@ impl App {
app.enqueue_primary_thread_session(started.session, started.turns)
.await?;
}
match app_server
.skills_list(codex_app_server_protocol::SkillsListParams {
cwds: vec![app.config.cwd.to_path_buf()],
force_reload: true,
per_cwd_extra_user_roots: None,
})
.await
{
Ok(response) => app.handle_skills_list_response(response),
Err(err) => tracing::warn!("failed to load skills on startup: {err:#}"),
}
app.handle_skills_list_result(
app_server
.skills_list(codex_app_server_protocol::SkillsListParams {
cwds: vec![app.config.cwd.to_path_buf()],
force_reload: true,
per_cwd_extra_user_roots: None,
})
.await,
"failed to load skills on startup",
);

// On startup, if Agent mode (workspace-write) or ReadOnly is active, warn about world-writable dirs on Windows.
#[cfg(target_os = "windows")]
Expand Down
Loading