From 50dafbc31b7399c5c14332bcf500497dda6b6928 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Wed, 17 Dec 2025 23:41:04 -0800 Subject: [PATCH] Make loading malformed skills fail-open (#8243) Instead of failing to start Codex, clearly call out that N skills did not load and provide warnings so that the user may fix them. image --- codex-rs/tui/src/app.rs | 47 +++--- codex-rs/tui/src/lib.rs | 1 - codex-rs/tui/src/skill_error_prompt.rs | 193 ----------------------- codex-rs/tui/src/tui.rs | 8 - codex-rs/tui2/src/app.rs | 47 +++--- codex-rs/tui2/src/lib.rs | 1 - codex-rs/tui2/src/skill_error_prompt.rs | 194 ------------------------ codex-rs/tui2/src/tui.rs | 8 - 8 files changed, 44 insertions(+), 455 deletions(-) delete mode 100644 codex-rs/tui/src/skill_error_prompt.rs delete mode 100644 codex-rs/tui2/src/skill_error_prompt.rs diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 3ab189005..66e12f086 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -14,8 +14,6 @@ use crate::pager_overlay::Overlay; use crate::render::highlight::highlight_bash_to_lines; use crate::render::renderable::Renderable; use crate::resume_picker::ResumeSelection; -use crate::skill_error_prompt::SkillErrorPromptOutcome; -use crate::skill_error_prompt::run_skill_error_prompt; use crate::tui; use crate::tui::TuiEvent; use crate::update_action::UpdateAction; @@ -37,7 +35,6 @@ use codex_core::protocol::Op; use codex_core::protocol::SessionSource; use codex_core::protocol::SkillErrorInfo; use codex_core::protocol::TokenUsage; -use codex_core::skills::SkillError; use codex_protocol::ConversationId; use codex_protocol::openai_models::ModelPreset; use codex_protocol::openai_models::ModelUpgrade; @@ -89,16 +86,6 @@ fn session_summary( }) } -fn skill_errors_from_info(errors: &[SkillErrorInfo]) -> Vec { - errors - .iter() - .map(|err| SkillError { - path: err.path.clone(), - message: err.message.clone(), - }) - .collect() -} - fn errors_for_cwd(cwd: &Path, response: &ListSkillsResponseEvent) -> Vec { response .skills @@ -108,6 +95,27 @@ fn errors_for_cwd(cwd: &Path, response: &ListSkillsResponseEvent) -> Vec { - self.chat_widget.submit_op(Op::Shutdown); - return Ok(false); - } - SkillErrorPromptOutcome::Continue => {} - } + emit_skill_load_warnings(&self.app_event_tx, &errors); } self.chat_widget.handle_codex_event(event); } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 0d48c8c2e..005446c5f 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -67,7 +67,6 @@ mod resume_picker; mod selection_list; mod session_log; mod shimmer; -mod skill_error_prompt; mod slash_command; mod status; mod status_indicator_widget; diff --git a/codex-rs/tui/src/skill_error_prompt.rs b/codex-rs/tui/src/skill_error_prompt.rs deleted file mode 100644 index 9a9f803ad..000000000 --- a/codex-rs/tui/src/skill_error_prompt.rs +++ /dev/null @@ -1,193 +0,0 @@ -use crate::tui::FrameRequester; -use crate::tui::Tui; -use crate::tui::TuiEvent; -use crate::wrapping::RtOptions; -use crate::wrapping::word_wrap_line; -use codex_core::skills::SkillError; -use crossterm::event::KeyCode; -use crossterm::event::KeyEvent; -use crossterm::event::KeyEventKind; -use crossterm::event::KeyModifiers; -use ratatui::buffer::Buffer; -use ratatui::layout::Rect; -use ratatui::prelude::Stylize as _; -use ratatui::text::Line; -use ratatui::widgets::Block; -use ratatui::widgets::Borders; -use ratatui::widgets::Clear; -use ratatui::widgets::Paragraph; -use ratatui::widgets::Widget; -use ratatui::widgets::WidgetRef; -use tokio_stream::StreamExt; - -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) enum SkillErrorPromptOutcome { - Continue, - Exit, -} - -pub(crate) async fn run_skill_error_prompt( - tui: &mut Tui, - errors: &[SkillError], -) -> SkillErrorPromptOutcome { - struct AltScreenGuard<'a> { - tui: &'a mut Tui, - stashed_history_lines: Vec>, - } - impl<'a> AltScreenGuard<'a> { - fn enter(tui: &'a mut Tui) -> Self { - let _ = tui.enter_alt_screen(); - let stashed_history_lines = tui.stash_pending_history_lines(); - Self { - tui, - stashed_history_lines, - } - } - } - impl Drop for AltScreenGuard<'_> { - fn drop(&mut self) { - let _ = self.tui.leave_alt_screen(); - let stashed_history_lines = std::mem::take(&mut self.stashed_history_lines); - self.tui - .restore_pending_history_lines(stashed_history_lines); - } - } - - let alt = AltScreenGuard::enter(tui); - let mut screen = SkillErrorScreen::new(alt.tui.frame_requester(), errors); - - let _ = alt.tui.draw(u16::MAX, |frame| { - frame.render_widget_ref(&screen, frame.area()); - }); - - let events = alt.tui.event_stream(); - tokio::pin!(events); - - while !screen.is_done() { - if let Some(event) = events.next().await { - match event { - TuiEvent::Key(key_event) => screen.handle_key(key_event), - TuiEvent::Paste(_) => {} - TuiEvent::Draw => { - let _ = alt.tui.draw(u16::MAX, |frame| { - frame.render_widget_ref(&screen, frame.area()); - }); - } - } - } else { - screen.confirm_continue(); - break; - } - } - - screen.outcome() -} - -struct SkillErrorScreen { - request_frame: FrameRequester, - errors: Vec, - done: bool, - exit: bool, -} - -impl SkillErrorScreen { - fn new(request_frame: FrameRequester, errors: &[SkillError]) -> Self { - Self { - request_frame, - errors: errors.to_vec(), - done: false, - exit: false, - } - } - - fn is_done(&self) -> bool { - self.done - } - - fn confirm_continue(&mut self) { - self.done = true; - self.exit = false; - self.request_frame.schedule_frame(); - } - - fn confirm_exit(&mut self) { - self.done = true; - self.exit = true; - self.request_frame.schedule_frame(); - } - - fn outcome(&self) -> SkillErrorPromptOutcome { - if self.exit { - SkillErrorPromptOutcome::Exit - } else { - SkillErrorPromptOutcome::Continue - } - } - - fn handle_key(&mut self, key_event: KeyEvent) { - if key_event.kind == KeyEventKind::Release { - return; - } - - if key_event - .modifiers - .intersects(KeyModifiers::CONTROL | KeyModifiers::META) - && matches!(key_event.code, KeyCode::Char('c') | KeyCode::Char('d')) - { - self.confirm_exit(); - return; - } - - match key_event.code { - KeyCode::Enter | KeyCode::Esc | KeyCode::Char(' ') | KeyCode::Char('q') => { - self.confirm_continue(); - } - _ => {} - } - } -} - -impl WidgetRef for &SkillErrorScreen { - fn render_ref(&self, area: Rect, buf: &mut Buffer) { - Clear.render(area, buf); - - let block = Block::default() - .title("Skill errors".bold()) - .borders(Borders::ALL); - - let inner = block.inner(area); - let width = usize::from(inner.width).max(1); - - let mut base_lines: Vec> = vec![ - Line::from("Skill validation errors detected".bold()), - Line::from("Fix these SKILL.md files and restart."), - Line::from("Invalid skills are ignored until resolved."), - Line::from("Press enter or esc to continue. Ctrl+C or Ctrl+D to exit."), - Line::from(""), - ]; - - let error_start = base_lines.len(); - for error in &self.errors { - base_lines.push(Line::from(vec![ - error.path.display().to_string().dim(), - ": ".into(), - error.message.clone().red(), - ])); - } - - let error_wrap_opts = RtOptions::new(width) - .initial_indent(Line::from("- ")) - .subsequent_indent(Line::from(" ")); - - let mut lines: Vec> = Vec::new(); - for (idx, line) in base_lines.iter().enumerate() { - if idx < error_start { - lines.extend(word_wrap_line(line, width)); - } else { - lines.extend(word_wrap_line(line, error_wrap_opts.clone())); - } - } - - Paragraph::new(lines).block(block).render(area, buf); - } -} diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index bfb4f8e03..0770b7664 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -329,14 +329,6 @@ impl Tui { self.frame_requester().schedule_frame(); } - pub(crate) fn stash_pending_history_lines(&mut self) -> Vec> { - std::mem::take(&mut self.pending_history_lines) - } - - pub(crate) fn restore_pending_history_lines(&mut self, lines: Vec>) { - self.pending_history_lines = lines; - } - pub fn draw( &mut self, height: u16, diff --git a/codex-rs/tui2/src/app.rs b/codex-rs/tui2/src/app.rs index c518b51ac..1275c1a33 100644 --- a/codex-rs/tui2/src/app.rs +++ b/codex-rs/tui2/src/app.rs @@ -17,8 +17,6 @@ use crate::pager_overlay::Overlay; use crate::render::highlight::highlight_bash_to_lines; use crate::render::renderable::Renderable; use crate::resume_picker::ResumeSelection; -use crate::skill_error_prompt::SkillErrorPromptOutcome; -use crate::skill_error_prompt::run_skill_error_prompt; use crate::tui; use crate::tui::TuiEvent; use crate::tui::scrolling::TranscriptLineMeta; @@ -44,7 +42,6 @@ use codex_core::protocol::Op; use codex_core::protocol::SessionSource; use codex_core::protocol::SkillErrorInfo; use codex_core::protocol::TokenUsage; -use codex_core::skills::SkillError; use codex_protocol::ConversationId; use codex_protocol::openai_models::ModelPreset; use codex_protocol::openai_models::ModelUpgrade; @@ -118,16 +115,6 @@ fn session_summary( }) } -fn skill_errors_from_info(errors: &[SkillErrorInfo]) -> Vec { - errors - .iter() - .map(|err| SkillError { - path: err.path.clone(), - message: err.message.clone(), - }) - .collect() -} - fn errors_for_cwd(cwd: &Path, response: &ListSkillsResponseEvent) -> Vec { response .skills @@ -137,6 +124,27 @@ fn errors_for_cwd(cwd: &Path, response: &ListSkillsResponseEvent) -> Vec { - self.chat_widget.submit_op(Op::Shutdown); - return Ok(false); - } - SkillErrorPromptOutcome::Continue => {} - } + emit_skill_load_warnings(&self.app_event_tx, &errors); } self.chat_widget.handle_codex_event(event); } diff --git a/codex-rs/tui2/src/lib.rs b/codex-rs/tui2/src/lib.rs index 97a4ec5e8..a9b34c495 100644 --- a/codex-rs/tui2/src/lib.rs +++ b/codex-rs/tui2/src/lib.rs @@ -68,7 +68,6 @@ mod resume_picker; mod selection_list; mod session_log; mod shimmer; -mod skill_error_prompt; mod slash_command; mod status; mod status_indicator_widget; diff --git a/codex-rs/tui2/src/skill_error_prompt.rs b/codex-rs/tui2/src/skill_error_prompt.rs deleted file mode 100644 index b97fb02d1..000000000 --- a/codex-rs/tui2/src/skill_error_prompt.rs +++ /dev/null @@ -1,194 +0,0 @@ -use crate::tui::FrameRequester; -use crate::tui::Tui; -use crate::tui::TuiEvent; -use crate::wrapping::RtOptions; -use crate::wrapping::word_wrap_line; -use codex_core::skills::SkillError; -use crossterm::event::KeyCode; -use crossterm::event::KeyEvent; -use crossterm::event::KeyEventKind; -use crossterm::event::KeyModifiers; -use ratatui::buffer::Buffer; -use ratatui::layout::Rect; -use ratatui::prelude::Stylize as _; -use ratatui::text::Line; -use ratatui::widgets::Block; -use ratatui::widgets::Borders; -use ratatui::widgets::Clear; -use ratatui::widgets::Paragraph; -use ratatui::widgets::Widget; -use ratatui::widgets::WidgetRef; -use tokio_stream::StreamExt; - -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) enum SkillErrorPromptOutcome { - Continue, - Exit, -} - -pub(crate) async fn run_skill_error_prompt( - tui: &mut Tui, - errors: &[SkillError], -) -> SkillErrorPromptOutcome { - struct AltScreenGuard<'a> { - tui: &'a mut Tui, - stashed_history_lines: Vec>, - } - impl<'a> AltScreenGuard<'a> { - fn enter(tui: &'a mut Tui) -> Self { - let _ = tui.enter_alt_screen(); - let stashed_history_lines = tui.stash_pending_history_lines(); - Self { - tui, - stashed_history_lines, - } - } - } - impl Drop for AltScreenGuard<'_> { - fn drop(&mut self) { - let _ = self.tui.leave_alt_screen(); - let stashed_history_lines = std::mem::take(&mut self.stashed_history_lines); - self.tui - .restore_pending_history_lines(stashed_history_lines); - } - } - - let alt = AltScreenGuard::enter(tui); - let mut screen = SkillErrorScreen::new(alt.tui.frame_requester(), errors); - - let _ = alt.tui.draw(u16::MAX, |frame| { - frame.render_widget_ref(&screen, frame.area()); - }); - - let events = alt.tui.event_stream(); - tokio::pin!(events); - - while !screen.is_done() { - if let Some(event) = events.next().await { - match event { - TuiEvent::Key(key_event) => screen.handle_key(key_event), - TuiEvent::Mouse(_) => {} - TuiEvent::Paste(_) => {} - TuiEvent::Draw => { - let _ = alt.tui.draw(u16::MAX, |frame| { - frame.render_widget_ref(&screen, frame.area()); - }); - } - } - } else { - screen.confirm_continue(); - break; - } - } - - screen.outcome() -} - -struct SkillErrorScreen { - request_frame: FrameRequester, - errors: Vec, - done: bool, - exit: bool, -} - -impl SkillErrorScreen { - fn new(request_frame: FrameRequester, errors: &[SkillError]) -> Self { - Self { - request_frame, - errors: errors.to_vec(), - done: false, - exit: false, - } - } - - fn is_done(&self) -> bool { - self.done - } - - fn confirm_continue(&mut self) { - self.done = true; - self.exit = false; - self.request_frame.schedule_frame(); - } - - fn confirm_exit(&mut self) { - self.done = true; - self.exit = true; - self.request_frame.schedule_frame(); - } - - fn outcome(&self) -> SkillErrorPromptOutcome { - if self.exit { - SkillErrorPromptOutcome::Exit - } else { - SkillErrorPromptOutcome::Continue - } - } - - fn handle_key(&mut self, key_event: KeyEvent) { - if key_event.kind == KeyEventKind::Release { - return; - } - - if key_event - .modifiers - .intersects(KeyModifiers::CONTROL | KeyModifiers::META) - && matches!(key_event.code, KeyCode::Char('c') | KeyCode::Char('d')) - { - self.confirm_exit(); - return; - } - - match key_event.code { - KeyCode::Enter | KeyCode::Esc | KeyCode::Char(' ') | KeyCode::Char('q') => { - self.confirm_continue(); - } - _ => {} - } - } -} - -impl WidgetRef for &SkillErrorScreen { - fn render_ref(&self, area: Rect, buf: &mut Buffer) { - Clear.render(area, buf); - - let block = Block::default() - .title("Skill errors".bold()) - .borders(Borders::ALL); - - let inner = block.inner(area); - let width = usize::from(inner.width).max(1); - - let mut base_lines: Vec> = vec![ - Line::from("Skill validation errors detected".bold()), - Line::from("Fix these SKILL.md files and restart."), - Line::from("Invalid skills are ignored until resolved."), - Line::from("Press enter or esc to continue. Ctrl+C or Ctrl+D to exit."), - Line::from(""), - ]; - - let error_start = base_lines.len(); - for error in &self.errors { - base_lines.push(Line::from(vec![ - error.path.display().to_string().dim(), - ": ".into(), - error.message.clone().red(), - ])); - } - - let error_wrap_opts = RtOptions::new(width) - .initial_indent(Line::from("- ")) - .subsequent_indent(Line::from(" ")); - - let mut lines: Vec> = Vec::new(); - for (idx, line) in base_lines.iter().enumerate() { - if idx < error_start { - lines.extend(word_wrap_line(line, width)); - } else { - lines.extend(word_wrap_line(line, error_wrap_opts.clone())); - } - } - - Paragraph::new(lines).block(block).render(area, buf); - } -} diff --git a/codex-rs/tui2/src/tui.rs b/codex-rs/tui2/src/tui.rs index a10c3f99b..712c5cf55 100644 --- a/codex-rs/tui2/src/tui.rs +++ b/codex-rs/tui2/src/tui.rs @@ -335,14 +335,6 @@ impl Tui { self.frame_requester().schedule_frame(); } - pub(crate) fn stash_pending_history_lines(&mut self) -> Vec> { - std::mem::take(&mut self.pending_history_lines) - } - - pub(crate) fn restore_pending_history_lines(&mut self, lines: Vec>) { - self.pending_history_lines = lines; - } - pub fn draw( &mut self, height: u16,