[codex] Deduplicate skill load warnings (#26698)

Skill reloads can get noisy when the watcher keeps triggering
`skills/list` and the same invalid `SKILL.md` error comes back each
time.

This keeps the first warning visible, then suppresses repeats while the
same `(path, message)` is still active. If the error clears and later
comes back, or if the message changes, it will show again.

Validation:
- `just fmt`
- `just test -p codex-tui skill_load_warning_state`
This commit is contained in:
xl-openai
2026-06-05 18:37:47 -07:00
committed by GitHub
Unverified
parent 2f108f9fd9
commit cbac22dabe
6 changed files with 199 additions and 0 deletions
+3
View File
@@ -513,6 +513,8 @@ pub(crate) struct App {
status_line_invalid_items_warned: Arc<AtomicBool>,
// Shared across ChatWidget instances so invalid terminal-title config warnings only emit once.
terminal_title_invalid_items_warned: Arc<AtomicBool>,
// Tracks active skill-load warnings so refreshes do not duplicate history cells.
skill_load_warnings: SkillLoadWarningState,
// Esc-backtracking state grouped
pub(crate) backtrack: crate::app_backtrack::BacktrackState,
@@ -1015,6 +1017,7 @@ See the Codex keymap documentation for supported actions and examples."
commit_anim_running: Arc::new(AtomicBool::new(false)),
status_line_invalid_items_warned: status_line_invalid_items_warned.clone(),
terminal_title_invalid_items_warned: terminal_title_invalid_items_warned.clone(),
skill_load_warnings: SkillLoadWarningState::default(),
backtrack: BacktrackState::default(),
backtrack_render_pending: false,
feedback: feedback.clone(),
+1
View File
@@ -113,6 +113,7 @@ impl App {
self.initial_history_replay_buffer = None;
self.backtrack = BacktrackState::default();
self.backtrack_render_pending = false;
self.skill_load_warnings.clear();
}
}
+163
View File
@@ -4,6 +4,45 @@
//! catalog state into one-time TUI prompts or warning cells without owning the main event loop.
use super::*;
use std::collections::HashSet;
use std::path::PathBuf;
#[derive(Debug, PartialEq, Eq, Hash)]
struct SkillLoadWarningKey {
path: PathBuf,
message: String,
}
#[derive(Debug, Default)]
pub(super) struct SkillLoadWarningState {
active: HashSet<SkillLoadWarningKey>,
}
impl SkillLoadWarningState {
pub(super) fn clear(&mut self) {
self.active.clear();
}
pub(super) fn newly_active_errors(&mut self, errors: &[SkillErrorInfo]) -> Vec<SkillErrorInfo> {
let previous = std::mem::take(&mut self.active);
let mut current = HashSet::new();
let mut newly_active = Vec::new();
for error in errors {
let key = SkillLoadWarningKey {
path: error.path.clone(),
message: error.message.clone(),
};
let was_active = previous.contains(&key);
if current.insert(key) && !was_active {
newly_active.push(error.clone());
}
}
self.active = current;
newly_active
}
}
pub(super) fn emit_skill_load_warnings(app_event_tx: &AppEventSender, errors: &[SkillErrorInfo]) {
if errors.is_empty() {
@@ -336,8 +375,10 @@ mod tests {
use super::*;
use crate::test_support::PathBufExt;
use pretty_assertions::assert_eq;
use ratatui::text::Line;
use std::path::PathBuf;
use tempfile::tempdir;
use tokio::sync::mpsc::unbounded_channel;
#[test]
fn normalize_harness_overrides_resolves_relative_add_dirs() -> Result<()> {
@@ -357,4 +398,126 @@ mod tests {
);
Ok(())
}
fn skill_error(path: &str, message: &str) -> SkillErrorInfo {
SkillErrorInfo {
path: PathBuf::from(path),
message: message.to_string(),
}
}
fn render_line_text(line: &Line<'static>) -> String {
line.spans
.iter()
.map(|span| span.content.as_ref())
.collect()
}
fn render_skill_load_warning_cells(errors: &[SkillErrorInfo]) -> String {
let (tx, mut rx) = unbounded_channel();
let app_event_tx = AppEventSender::new(tx);
emit_skill_load_warnings(&app_event_tx, errors);
let mut rendered = Vec::new();
while let Ok(AppEvent::InsertHistoryCell(cell)) = rx.try_recv() {
rendered.extend(
cell.display_lines(/*width*/ 120)
.iter()
.map(render_line_text),
);
}
rendered.join("\n")
}
#[test]
fn skill_load_warning_state_suppresses_repeated_active_errors() {
let mut state = SkillLoadWarningState::default();
let error = skill_error("/repo/.codex/skills/abc/SKILL.md", "invalid description");
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&error)),
vec![error.clone()]
);
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&error)),
Vec::<SkillErrorInfo>::new()
);
}
#[test]
fn skill_load_warning_state_reemits_after_error_clears() {
let mut state = SkillLoadWarningState::default();
let error = skill_error("/repo/.codex/skills/abc/SKILL.md", "invalid description");
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&error)),
vec![error.clone()]
);
assert_eq!(state.newly_active_errors(&[]), Vec::<SkillErrorInfo>::new());
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&error)),
vec![error]
);
}
#[test]
fn skill_load_warning_state_displays_new_message_for_active_path() {
let mut state = SkillLoadWarningState::default();
let initial = skill_error("/repo/.codex/skills/abc/SKILL.md", "invalid description");
let changed = skill_error("/repo/.codex/skills/abc/SKILL.md", "invalid frontmatter");
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&initial)),
vec![initial]
);
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&changed)),
vec![changed]
);
}
#[test]
fn skill_load_warning_state_clear_allows_active_error_again() {
let mut state = SkillLoadWarningState::default();
let error = skill_error("/repo/.codex/skills/abc/SKILL.md", "invalid description");
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&error)),
vec![error.clone()]
);
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&error)),
Vec::<SkillErrorInfo>::new()
);
state.clear();
assert_eq!(
state.newly_active_errors(std::slice::from_ref(&error)),
vec![error]
);
}
#[test]
fn repeated_active_skill_load_warning_renders_once() {
let mut state = SkillLoadWarningState::default();
let error = skill_error("/repo/.codex/skills/abc/SKILL.md", "invalid description");
let first_errors = state.newly_active_errors(std::slice::from_ref(&error));
let repeated_errors = state.newly_active_errors(std::slice::from_ref(&error));
let rendered = [
render_skill_load_warning_cells(&first_errors),
render_skill_load_warning_cells(&repeated_errors),
]
.into_iter()
.filter(|output| !output.is_empty())
.collect::<Vec<_>>()
.join("\n");
insta::assert_snapshot!(rendered, @r"
⚠ Skipped loading 1 skill(s) due to invalid SKILL.md files.
⚠ /repo/.codex/skills/abc/SKILL.md: invalid description
");
}
}
+1
View File
@@ -39,6 +39,7 @@ pub(super) async fn make_test_app() -> App {
commit_anim_running: Arc::new(AtomicBool::new(false)),
status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)),
terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)),
skill_load_warnings: SkillLoadWarningState::default(),
backtrack: BacktrackState::default(),
backtrack_render_pending: false,
feedback: codex_feedback::CodexFeedback::new(),
+30
View File
@@ -3788,6 +3788,7 @@ async fn make_test_app() -> App {
commit_anim_running: Arc::new(AtomicBool::new(false)),
status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)),
terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)),
skill_load_warnings: SkillLoadWarningState::default(),
backtrack: BacktrackState::default(),
backtrack_render_pending: false,
feedback: codex_feedback::CodexFeedback::new(),
@@ -3851,6 +3852,7 @@ async fn make_test_app_with_channels() -> (
commit_anim_running: Arc::new(AtomicBool::new(false)),
status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)),
terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)),
skill_load_warnings: SkillLoadWarningState::default(),
backtrack: BacktrackState::default(),
backtrack_render_pending: false,
feedback: codex_feedback::CodexFeedback::new(),
@@ -5576,6 +5578,34 @@ async fn clear_only_ui_reset_preserves_chat_session_state() {
assert_eq!(app.chat_widget.composer_text_with_pending(), "draft prompt");
}
#[tokio::test]
async fn clear_only_ui_reset_allows_active_skill_warning_to_render_again() {
let mut app = make_test_app().await;
let error = SkillErrorInfo {
path: test_path_buf("/tmp/project/.codex/skills/abc/SKILL.md"),
message: "invalid description".to_string(),
};
assert_eq!(
app.skill_load_warnings
.newly_active_errors(std::slice::from_ref(&error)),
vec![error.clone()]
);
assert_eq!(
app.skill_load_warnings
.newly_active_errors(std::slice::from_ref(&error)),
Vec::<SkillErrorInfo>::new()
);
app.reset_app_ui_state_after_clear();
assert_eq!(
app.skill_load_warnings
.newly_active_errors(std::slice::from_ref(&error)),
vec![error]
);
}
#[tokio::test]
async fn backtrack_esc_does_not_steal_empty_vim_insert_escape() {
let mut app = make_test_app().await;
+1
View File
@@ -1342,6 +1342,7 @@ impl App {
pub(super) fn handle_skills_list_response(&mut self, response: SkillsListResponse) {
let cwd = self.chat_widget.config_ref().cwd.clone();
let errors = errors_for_cwd(&cwd, &response);
let errors = self.skill_load_warnings.newly_active_errors(&errors);
emit_skill_load_warnings(&self.app_event_tx, &errors);
self.chat_widget.handle_skills_list_response(response);
}