From e8622da9a85cf2c4fc3a745cf33f405b1394e128 Mon Sep 17 00:00:00 2001 From: Thibault Sottiaux Date: Sun, 30 Nov 2025 15:02:58 -0800 Subject: [PATCH 01/24] Add runtime skills discovery and validation --- codex-rs/Cargo.lock | 30 ++- codex-rs/Cargo.toml | 1 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/lib.rs | 1 + codex-rs/core/src/project_doc.rs | 103 ++++++++- codex-rs/core/src/skills.rs | 298 +++++++++++++++++++++++++ codex-rs/tui/src/app.rs | 17 ++ codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/skill_error_prompt.rs | 164 ++++++++++++++ skills_plan.md | 118 ++++++++++ 10 files changed, 720 insertions(+), 14 deletions(-) create mode 100644 codex-rs/core/src/skills.rs create mode 100644 codex-rs/tui/src/skill_error_prompt.rs create mode 100644 skills_plan.md diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 4c87b8dc05..100abd91aa 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -212,7 +212,7 @@ dependencies = [ "objc2-foundation", "parking_lot", "percent-encoding", - "windows-sys 0.52.0", + "windows-sys 0.59.0", "wl-clipboard-rs", "x11rb", ] @@ -1186,6 +1186,7 @@ dependencies = [ "seccompiler", "serde", "serde_json", + "serde_yaml", "serial_test", "sha1", "sha2", @@ -2536,7 +2537,7 @@ checksum = "0ce92ff622d6dadf7349484f42c93271a0d49b7cc4d466a936405bacbe10aa78" dependencies = [ "cfg-if", "rustix 1.0.8", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -3440,7 +3441,7 @@ checksum = "e04d7f318608d35d4b61ddd75cbdaee86b023ebe2bd5a66ee0915f0bf93095a9" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -5217,7 +5218,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.4.15", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -5766,6 +5767,19 @@ dependencies = [ "syn 2.0.104", ] +[[package]] +name = "serde_yaml" +version = "0.9.34+deprecated" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" +dependencies = [ + "indexmap 2.12.0", + "itoa", + "ryu", + "serde", + "unsafe-libyaml", +] + [[package]] name = "serial2" version = "0.2.31" @@ -6980,6 +6994,12 @@ version = "0.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ebc1c04c71510c7f702b52b7c350734c9ff1295c464a03335b00bb84fc54f853" +[[package]] +name = "unsafe-libyaml" +version = "0.2.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" + [[package]] name = "untrusted" version = "0.9.0" @@ -7369,7 +7389,7 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 053d79d7e0..6d2187cb1a 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -178,6 +178,7 @@ seccompiler = "0.5.0" sentry = "0.34.0" serde = "1" serde_json = "1" +serde_yaml = "0.9" serde_with = "3.14" serial_test = "3.2.0" sha1 = "0.10.6" diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 7bd2d652e1..8a329d0672 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -52,6 +52,7 @@ regex-lite = { workspace = true } reqwest = { workspace = true, features = ["json", "stream"] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +serde_yaml = { workspace = true } sha1 = { workspace = true } sha2 = { workspace = true } shlex = { workspace = true } diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 7a9440eb28..a6a7d7541e 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -72,6 +72,7 @@ mod rollout; pub(crate) mod safety; pub mod seatbelt; pub mod shell; +pub mod skills; pub mod spawn; pub mod terminal; mod tools; diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index c8076aa822..90b28d5ae4 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -14,6 +14,8 @@ //! 3. We do **not** walk past the Git root. use crate::config::Config; +use crate::skills::load_skills; +use crate::skills::render_skills_section; use dunce::canonicalize as normalize_path; use std::path::PathBuf; use tokio::io::AsyncReadExt; @@ -31,18 +33,33 @@ const PROJECT_DOC_SEPARATOR: &str = "\n\n--- project-doc ---\n\n"; /// Combines `Config::instructions` and `AGENTS.md` (if present) into a single /// string of instructions. pub(crate) async fn get_user_instructions(config: &Config) -> Option { - match read_project_docs(config).await { - Ok(Some(project_doc)) => match &config.user_instructions { - Some(original_instructions) => Some(format!( - "{original_instructions}{PROJECT_DOC_SEPARATOR}{project_doc}" - )), - None => Some(project_doc), - }, - Ok(None) => config.user_instructions.clone(), + let skills_outcome = load_skills(config); + for err in &skills_outcome.errors { + error!( + "failed to load skill {}: {}", + err.path.display(), + err.message + ); + } + let skills_section = render_skills_section(&skills_outcome.skills); + + let project_docs = match read_project_docs(config).await { + Ok(docs) => docs, Err(e) => { error!("error trying to find project doc: {e:#}"); - config.user_instructions.clone() + return config.user_instructions.clone(); } + }; + + let combined_project_docs = merge_project_docs_with_skills(project_docs, skills_section); + + match (config.user_instructions.clone(), combined_project_docs) { + (Some(instructions), Some(project_doc)) => Some(format!( + "{instructions}{PROJECT_DOC_SEPARATOR}{project_doc}" + )), + (Some(instructions), None) => Some(instructions), + (None, Some(project_doc)) => Some(project_doc), + (None, None) => None, } } @@ -195,12 +212,25 @@ fn candidate_filenames<'a>(config: &'a Config) -> Vec<&'a str> { names } +fn merge_project_docs_with_skills( + project_doc: Option, + skills_section: Option, +) -> Option { + match (project_doc, skills_section) { + (Some(doc), Some(skills)) => Some(format!("{doc}\n\n{skills}")), + (Some(doc), None) => Some(doc), + (None, Some(skills)) => Some(skills), + (None, None) => None, + } +} + #[cfg(test)] mod tests { use super::*; use crate::config::ConfigOverrides; use crate::config::ConfigToml; use std::fs; + use std::path::PathBuf; use tempfile::TempDir; /// Helper that returns a `Config` pointing at `root` and using `limit` as @@ -447,4 +477,59 @@ mod tests { .eq(DEFAULT_PROJECT_DOC_FILENAME) ); } + + #[tokio::test] + async fn skills_are_appended_to_project_doc() { + let tmp = tempfile::tempdir().expect("tempdir"); + fs::write(tmp.path().join("AGENTS.md"), "base doc").unwrap(); + + let cfg = make_config(&tmp, 4096, None); + create_skill( + cfg.codex_home.clone(), + "pdf-processing", + "extract from pdfs", + ); + + let res = get_user_instructions(&cfg) + .await + .expect("instructions expected"); + let expected_path = cfg + .codex_home + .join("skills/pdf-processing/SKILL.md") + .canonicalize() + .unwrap(); + let expected = format!( + "base doc\n\n## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- pdf-processing: extract from pdfs (file: {})", + expected_path.display() + ); + assert_eq!(res, expected); + } + + #[tokio::test] + async fn skills_render_without_project_doc() { + let tmp = tempfile::tempdir().expect("tempdir"); + let cfg = make_config(&tmp, 4096, None); + create_skill(cfg.codex_home.clone(), "linting", "run clippy"); + + let res = get_user_instructions(&cfg) + .await + .expect("instructions expected"); + let expected_path = cfg + .codex_home + .join("skills/linting/SKILL.md") + .canonicalize() + .unwrap(); + let expected = format!( + "## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- linting: run clippy (file: {})", + expected_path.display() + ); + assert_eq!(res, expected); + } + + fn create_skill(codex_home: PathBuf, name: &str, description: &str) { + let skill_dir = codex_home.join(format!("skills/{name}")); + fs::create_dir_all(&skill_dir).unwrap(); + let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + fs::write(skill_dir.join("SKILL.md"), content).unwrap(); + } } diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs new file mode 100644 index 0000000000..270e3fba95 --- /dev/null +++ b/codex-rs/core/src/skills.rs @@ -0,0 +1,298 @@ +use crate::config::Config; +use dunce::canonicalize as normalize_path; +use serde::Deserialize; +use std::collections::VecDeque; +use std::fs; +use std::path::Path; +use std::path::PathBuf; +use tracing::error; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillMetadata { + pub name: String, + pub description: String, + pub path: PathBuf, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillError { + pub path: PathBuf, + pub message: String, +} + +#[derive(Debug, Clone, Default)] +pub struct SkillLoadOutcome { + pub skills: Vec, + pub errors: Vec, +} + +#[derive(Debug, Deserialize)] +struct SkillFrontmatter { + name: String, + description: String, +} + +const SKILL_FILENAME: &str = "SKILL.md"; +const SKILLS_DIR_NAME: &str = "skills"; +const MAX_NAME_LEN: usize = 100; +const MAX_DESCRIPTION_LEN: usize = 500; + +pub fn load_skills(config: &Config) -> SkillLoadOutcome { + let mut outcome = SkillLoadOutcome::default(); + let roots = skill_roots(config); + for root in roots { + discover_skills_under_root(&root, &mut outcome); + } + + outcome + .skills + .sort_by(|a, b| a.name.cmp(&b.name).then_with(|| a.path.cmp(&b.path))); + + outcome +} + +pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { + if skills.is_empty() { + return None; + } + + let mut lines: Vec = Vec::new(); + lines.push("## Skills".to_string()); + lines.push("These skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.".to_string()); + + for skill in skills { + lines.push(format!( + "- {}: {} (file: {})", + skill.name, + skill.description, + skill.path.display() + )); + } + + Some(lines.join("\n")) +} + +fn skill_roots(config: &Config) -> Vec { + vec![config.codex_home.join(SKILLS_DIR_NAME)] +} + +fn discover_skills_under_root(root: &Path, outcome: &mut SkillLoadOutcome) { + let Ok(root) = normalize_path(root) else { + return; + }; + + if !root.is_dir() { + return; + } + + let mut queue: VecDeque = VecDeque::from([root]); + while let Some(dir) = queue.pop_front() { + let entries = match fs::read_dir(&dir) { + Ok(entries) => entries, + Err(e) => { + error!("failed to read skills dir {}: {e:#}", dir.display()); + continue; + } + }; + + for entry in entries.flatten() { + let path = entry.path(); + let file_name = match path.file_name().and_then(|f| f.to_str()) { + Some(name) => name, + None => continue, + }; + + if file_name.starts_with('.') { + continue; + } + + let Ok(metadata) = entry.metadata() else { + continue; + }; + + let file_type = metadata.file_type(); + if file_type.is_symlink() { + continue; + } + + if file_type.is_dir() { + queue.push_back(path); + continue; + } + + if file_type.is_file() && file_name == SKILL_FILENAME { + match parse_skill_file(&path) { + Ok(skill) => outcome.skills.push(skill), + Err(message) => outcome.errors.push(SkillError { path, message }), + } + } + } + } +} + +fn parse_skill_file(path: &Path) -> Result { + let contents = fs::read_to_string(path).map_err(|e| format!("failed to read file: {e}"))?; + + let frontmatter = extract_frontmatter(&contents) + .ok_or_else(|| "missing YAML frontmatter delimited by ---".to_string())?; + + let parsed: SkillFrontmatter = + serde_yaml::from_str(&frontmatter).map_err(|e| format!("invalid YAML: {e}"))?; + + let name = sanitize_single_line(&parsed.name); + let description = sanitize_single_line(&parsed.description); + + validate_field(&name, MAX_NAME_LEN, "name")?; + validate_field(&description, MAX_DESCRIPTION_LEN, "description")?; + + let resolved_path = normalize_path(path).unwrap_or_else(|_| path.to_path_buf()); + + Ok(SkillMetadata { + name, + description, + path: resolved_path, + }) +} + +fn sanitize_single_line(raw: &str) -> String { + raw.split_whitespace().collect::>().join(" ") +} + +fn validate_field(value: &str, max_len: usize, field_name: &str) -> Result<(), String> { + if value.is_empty() { + return Err(format!("{field_name} is required")); + } + if value.len() > max_len { + return Err(format!( + "{field_name} exceeds maximum length of {max_len} characters" + )); + } + Ok(()) +} + +fn extract_frontmatter(contents: &str) -> Option { + let mut lines = contents.lines(); + if !matches!(lines.next(), Some(line) if line.trim() == "---") { + return None; + } + + let mut frontmatter_lines: Vec<&str> = Vec::new(); + let mut found_closing = false; + for line in lines.by_ref() { + if line.trim() == "---" { + found_closing = true; + break; + } + frontmatter_lines.push(line); + } + + if frontmatter_lines.is_empty() || !found_closing { + return None; + } + + Some(frontmatter_lines.join("\n")) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::ConfigOverrides; + use crate::config::ConfigToml; + use tempfile::TempDir; + + fn make_config(codex_home: &TempDir) -> Config { + let mut config = Config::load_from_base_config_with_overrides( + ConfigToml::default(), + ConfigOverrides::default(), + codex_home.path().to_path_buf(), + ) + .expect("defaults for test should always succeed"); + + config.cwd = codex_home.path().to_path_buf(); + config + } + + fn write_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { + let skill_dir = codex_home.path().join(format!("skills/{dir}")); + fs::create_dir_all(&skill_dir).unwrap(); + let indented_description = description.replace('\n', "\n "); + let content = format!( + "---\nname: {name}\ndescription: |-\n {indented_description}\n---\n\n# Body\n" + ); + let path = skill_dir.join(SKILL_FILENAME); + fs::write(&path, content).unwrap(); + path + } + + #[test] + fn loads_valid_skill() { + let codex_home = tempfile::tempdir().expect("tempdir"); + write_skill(&codex_home, "demo", "demo-skill", "does things\ncarefully"); + let cfg = make_config(&codex_home); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + let skill = &outcome.skills[0]; + assert_eq!(skill.name, "demo-skill"); + assert_eq!(skill.description, "does things carefully"); + assert!( + skill + .path + .to_string_lossy() + .ends_with("skills/demo/SKILL.md"), + "unexpected path {}", + skill.path.display() + ); + } + + #[test] + fn skips_hidden_and_invalid() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let hidden_dir = codex_home.path().join("skills/.hidden"); + fs::create_dir_all(&hidden_dir).unwrap(); + fs::write( + hidden_dir.join(SKILL_FILENAME), + "---\nname: hidden\ndescription: hidden\n---\n", + ) + .unwrap(); + + // Invalid because missing closing frontmatter. + let invalid_dir = codex_home.path().join("skills/invalid"); + fs::create_dir_all(&invalid_dir).unwrap(); + fs::write(invalid_dir.join(SKILL_FILENAME), "---\nname: bad").unwrap(); + + let cfg = make_config(&codex_home); + let outcome = load_skills(&cfg); + assert_eq!(outcome.skills.len(), 0); + assert_eq!(outcome.errors.len(), 1); + assert!( + outcome.errors[0] + .message + .contains("missing YAML frontmatter"), + "expected frontmatter error" + ); + } + + #[test] + fn enforces_length_limits() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let long_desc = "a".repeat(MAX_DESCRIPTION_LEN + 1); + write_skill(&codex_home, "too-long", "toolong", &long_desc); + let cfg = make_config(&codex_home); + + let outcome = load_skills(&cfg); + assert_eq!(outcome.skills.len(), 0); + assert_eq!(outcome.errors.len(), 1); + assert!( + outcome.errors[0] + .message + .contains("description exceeds maximum"), + "expected length error" + ); + } +} diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index d0e057102c..2ba7e4d871 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -14,6 +14,8 @@ 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; @@ -36,6 +38,7 @@ use codex_core::protocol::Op; use codex_core::protocol::SessionSource; use codex_core::protocol::TokenUsage; use codex_core::protocol_config_types::ReasoningEffort as ReasoningEffortConfig; +use codex_core::skills::load_skills; use codex_protocol::ConversationId; use color_eyre::eyre::Result; use color_eyre::eyre::WrapErr; @@ -267,6 +270,20 @@ impl App { SessionSource::Cli, )); + let skills_outcome = load_skills(&config); + if !skills_outcome.errors.is_empty() { + match run_skill_error_prompt(tui, &skills_outcome.errors).await { + SkillErrorPromptOutcome::Exit => { + return Ok(AppExitInfo { + token_usage: TokenUsage::default(), + conversation_id: None, + update_action: None, + }); + } + SkillErrorPromptOutcome::Continue => {} + } + } + let enhanced_keys_supported = tui.enhanced_keys_supported(); let mut chat_widget = match resume_selection { diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 33bd18c437..15e9ccb92b 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -67,6 +67,7 @@ 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 new file mode 100644 index 0000000000..33d3b5dce1 --- /dev/null +++ b/codex-rs/tui/src/skill_error_prompt.rs @@ -0,0 +1,164 @@ +use crate::tui::FrameRequester; +use crate::tui::Tui; +use crate::tui::TuiEvent; +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 ratatui::widgets::Wrap; +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, + } + impl<'a> AltScreenGuard<'a> { + fn enter(tui: &'a mut Tui) -> Self { + let _ = tui.enter_alt_screen(); + Self { tui } + } + } + impl Drop for AltScreenGuard<'_> { + fn drop(&mut self) { + let _ = self.tui.leave_alt_screen(); + } + } + + 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, + lines: Vec>, + done: bool, + exit: bool, +} + +impl SkillErrorScreen { + fn new(request_frame: FrameRequester, errors: &[SkillError]) -> Self { + let mut lines: Vec> = Vec::new(); + lines.push(Line::from("Skill validation errors detected".bold())); + lines.push(Line::from( + "Fix these SKILL.md files and restart. Invalid skills are ignored until resolved. Press enter or esc to continue, Ctrl+C or Ctrl+D to exit.", + )); + lines.push(Line::from("")); + + for error in errors { + let message = format!("- {}: {}", error.path.display(), error.message); + lines.push(Line::from(message)); + } + + Self { + request_frame, + lines, + 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); + Paragraph::new(self.lines.clone()) + .block(block) + .wrap(Wrap { trim: true }) + .render(area, buf); + } +} diff --git a/skills_plan.md b/skills_plan.md new file mode 100644 index 0000000000..d8afddc870 --- /dev/null +++ b/skills_plan.md @@ -0,0 +1,118 @@ +Skills Injection Plan for codex-rs +================================== + +Context and motivation +---------------------- +codex-rs currently has no notion of reusable “skills” that can be discovered on disk and injected into the runtime context. We want to mirror the productive parts of Claude’s Agent Skills model while keeping the first iteration deliberately small and safe. The goals: +- Reduce repeated prompting by letting users package domain expertise once. +- Keep the runtime context lean by injecting only lightweight metadata (name, description, path) and not inlining full instructions. +- Avoid mutating source files; the feature should be entirely runtime-driven and reversible. +- Establish a structure that can grow to project-local skills later (e.g., alongside agents.md in a workspace). +- Provide strong validation and clear failure modes so misconfigured skills do not silently degrade agent behavior. + +Non-goals for v1: tool restrictions, hot reload, network/package installation, or inlining SKILL.md bodies. We also will not modify agents.md on disk. + +High-level behavior +------------------- +- On startup, codex discovers skills from the user home root `~/.codex/skills`, recursively. +- Each skill lives in a directory containing a file named exactly `SKILL.md` with YAML frontmatter (`name`, `description`) and a Markdown body. Extra keys are allowed and ignored. +- Valid skills are rendered into the agent context by dynamically appending a `## Skills` section to the in-memory content of the root `agents.md`. The file on disk remains unchanged. +- The `## Skills` section contains one-line bullets per skill: name, description, and absolute path to the `SKILL.md`, plus a brief intro paragraph explaining usage. No skill body content is inlined. +- Invalid skills (parse errors, missing required fields, overlength fields) block startup with a dismissible modal that lists every invalid path and error. Errors are also logged. After dismissal, invalid skills are ignored for rendering. The modal reappears on subsequent startups until the issues are fixed. +- If no valid skills are found, the Skills section is omitted entirely. +- Loading occurs once at startup; no hot reload in v1. + +Discovery rules +--------------- +- Roots: only `~/.codex/skills` in v1. Design the loader so additional roots can be added later (e.g., `/skills` alongside `/agents.md` for project-scoped skills). +- Recursion: traverse all subdirectories. +- Hidden entries: skip hidden directories and files (prefix “.”). +- Symlinks: do not follow symlinks. +- Recognition: only files named exactly `SKILL.md` qualify as skills. +- Ordering: collect all skills (no dedupe), then sort by `name`, then by absolute path for stable rendering. + +Skill format and validation +--------------------------- +- File: `SKILL.md` must start with YAML frontmatter delimited by `---` on its own lines; the next `---` ends the frontmatter. The remainder is body (ignored for rendering). +- Required fields: `name` (string), `description` (string). +- Length constraints: `name` non-empty, max 100 chars; `description` non-empty, max 500 chars. Enforce hard errors if exceeded. +- Sanitization for rendering: trim, collapse newlines/tabs/extra whitespace inside `name` and `description` to single spaces to preserve single-line output. +- Extra keys: allowed and ignored for forward compatibility. +- YAML parsing: use serde_yaml; accept CRLF; any parse error or missing required field is a hard error. + +Rendering model +--------------- +- Base content: read the root `agents.md` (do not mutate it). +- Synthetic section: append to the in-memory content a final section only if there is at least one valid skill. + - Heading: `## Skills` + - Intro paragraph (single line), suggested copy: “These skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.” + - Entries: one bullet per skill, single line, format `- : (file: /absolute/path/to/SKILL.md)`. Use sanitized fields to avoid line breaks. + - Paths: always absolute for clarity. +- If no valid skills: omit the section entirely. + +Error handling and UX +--------------------- +- Invalid skills: any validation or parse failure is treated as blocking. On startup, present a dismissible modal that lists every invalid skill with its path and detailed error message. Startup is paused until the user dismisses. +- Persistence: the modal reappears on every startup until all invalid skills are fixed or removed. +- Logging: also emit error-level logs for each invalid skill, matching what is shown in the modal. +- Post-dismissal behavior: invalid skills are ignored for rendering; valid skills still render. +- Reuse existing modal/popup pattern in the codebase for consistency; add a button to dismiss/continue. + +Update cadence +-------------- +- Load skills once at startup. No hot-reload, no file watching. Users must restart codex for changes to take effect. + +Future-proofing +--------------- +- Multiple roots: structure the loader to accept a list of roots (initially one: `~/.codex/skills`). Future additions: `/skills` co-located with `/agents.md`, or other configurable roots. +- Rendering target: keep the renderer generic—given base agents.md content and a skill list, return augmented content. This will let us plug in different agents.md locations or multiple agents files later. +- Allowed-tools: not implemented in v1, but the validator should ignore extra keys so later fields can be added without breaking old skills. +- Potential later enhancements: hot reload; per-project skills; UI for listing valid skills; richer summaries; optional inline previews under a size threshold; deduplication policies. + +Implementation outline +---------------------- +- Discovery: + - Expand `~` to absolute path. + - Walk directories recursively; skip hidden entries; do not follow symlinks. + - Collect paths exactly matching `SKILL.md`. +- Parsing/validation: + - For each path, read file; locate frontmatter between `---` lines. + - Parse with serde_yaml into a struct { name: String, description: String, …ignored }. + - Trim and sanitize; enforce non-empty and length limits. + - On failure, record an error entry (path + message). +- Rendering: + - Sort valid skills by name, then path. + - If none, return base agents.md unchanged. + - Otherwise, append the section with intro and bullets, ensuring one-line entries. +- Error surfacing: + - If any errors exist, build a blocking modal view at startup listing all (path + message), with a dismiss/continue button. Use existing modal screen as template. + - Log all errors via the standard logger at error level. + - After dismissal, proceed with rendering valid skills only. + +Testing approach +---------------- +- Unit tests: + - Frontmatter parsing and validation (missing fields, overlength, malformed YAML). + - Sanitization (newline/tab collapsing). + - Ordering and rendering format. + - Hidden/ symlink skipping. +- Integration tests: + - With a temp `~/.codex/skills` tree containing multiple valid and invalid skills, verify augmented agents content and that invalid ones are omitted. + - Modal trigger: simulate startup with invalid skills and assert the error list is produced (if test harness supports UI assertions). +- Manual checks: + - Create valid/invalid SKILLs under `~/.codex/skills`, start codex, observe modal and final context. + - Confirm agents.md on disk remains unchanged. + +Risks and mitigations +--------------------- +- Risk: Excessively long descriptions breaking layout. Mitigation: hard length limits and sanitization. +- Risk: Users surprised by blocking modal. Mitigation: clear messaging, dismiss to proceed, repeat only until fixed. +- Risk: Future roots complicate ordering. Mitigation: keep explicit ordering rules (name, then path) and stable formatting. +- Risk: Context bloat if too many skills. Mitigation: metadata-only render, short descriptions, length caps; consider future pagination or caps if needed. + +Acceptance criteria +------------------- +- With valid skills present, the runtime context includes a final `## Skills` section with the intro and one-line bullets (name, description, absolute path), sorted by name then path. agents.md on disk is untouched. +- If no skills are present, no Skills section is injected. +- Any invalid skill causes a blocking, dismissible startup modal listing all invalid paths and errors; errors also logged; invalid skills are excluded from rendering. The modal reappears on subsequent startups until resolved. + From f3552dc1fca4e897207bbbef580e5f59d29ee479 Mon Sep 17 00:00:00 2001 From: Thibault Sottiaux Date: Sun, 30 Nov 2025 15:05:00 -0800 Subject: [PATCH 02/24] Add skills plan diagram --- skills_plan.md | 58 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 5 deletions(-) diff --git a/skills_plan.md b/skills_plan.md index d8afddc870..1c7dfe4f0a 100644 --- a/skills_plan.md +++ b/skills_plan.md @@ -1,17 +1,22 @@ -Skills Injection Plan for codex-rs -================================== +Skills Injection Design Doc +=========================== Context and motivation ---------------------- -codex-rs currently has no notion of reusable “skills” that can be discovered on disk and injected into the runtime context. We want to mirror the productive parts of Claude’s Agent Skills model while keeping the first iteration deliberately small and safe. The goals: +Codex currently has no notion of reusable “skills” that can be discovered on disk and injected into the runtime context. We want to mirror the productive parts of other prior art in this space while keeping the first iteration deliberately small and safe. + +Goals: +- Act as an extension of the existing agents.md mechanism, which allows progressive disclosure of content and instructions driven by the agent's own decision making process. - Reduce repeated prompting by letting users package domain expertise once. + +Non-goals for first implementation (v1): tool restrictions, hot reload, network/package installation, hierarchies of skills with their own progressive disclosure or supporting skills defined elsewhere than the user home root. + +Implementation considerations: - Keep the runtime context lean by injecting only lightweight metadata (name, description, path) and not inlining full instructions. - Avoid mutating source files; the feature should be entirely runtime-driven and reversible. - Establish a structure that can grow to project-local skills later (e.g., alongside agents.md in a workspace). - Provide strong validation and clear failure modes so misconfigured skills do not silently degrade agent behavior. -Non-goals for v1: tool restrictions, hot reload, network/package installation, or inlining SKILL.md bodies. We also will not modify agents.md on disk. - High-level behavior ------------------- - On startup, codex discovers skills from the user home root `~/.codex/skills`, recursively. @@ -116,3 +121,46 @@ Acceptance criteria - If no skills are present, no Skills section is injected. - Any invalid skill causes a blocking, dismissible startup modal listing all invalid paths and errors; errors also logged; invalid skills are excluded from rendering. The modal reappears on subsequent startups until resolved. +ASCII data flow (current + future roots) +---------------------------------------- + +Current (v1): + +``` +~/.codex/skills/... + ├─ skill_a/SKILL.md + └─ skill_b/SKILL.md + | + v +[core] load_skills -> validated SkillMetadata list + | + v +[core] render_skills_section -> "## Skills\n- name: desc (file: ...)" + | + v +[core] read_project_docs(agents.md) --+ + | | + +----------- merge ---------+--> runtime instructions (agents.md + Skills section) +``` + +Future (multi-root example): + +``` +Roots: + ~/.codex/skills + /agents.md + /skills/ + /crates/foo/agents.md + /crates/foo/skills/ + +For each root: + - discover skills (same rules: recursive, skip hidden/symlinks, SKILL.md only) + - validate -> SkillMetadata + - render section (optional if any skills) + +Per agents.md: + - read agents.md content + - append its local Skills section (if any) + - concatenate higher-level agents.md + Skills sections (root -> cwd) + +Result: + runtime instructions = user_instructions? + "--- project-doc ---" + concatenated (agents.md + per-root Skills section) +``` From 5be690ad745f0fc98b21965b68f271136bc389c2 Mon Sep 17 00:00:00 2001 From: Thibault Sottiaux Date: Sun, 30 Nov 2025 15:06:38 -0800 Subject: [PATCH 03/24] Update skills plan diagram to mermaid --- skills_plan.md | 67 +++++++++++++++++++------------------------------- 1 file changed, 25 insertions(+), 42 deletions(-) diff --git a/skills_plan.md b/skills_plan.md index 1c7dfe4f0a..a2f3b7d61a 100644 --- a/skills_plan.md +++ b/skills_plan.md @@ -121,46 +121,29 @@ Acceptance criteria - If no skills are present, no Skills section is injected. - Any invalid skill causes a blocking, dismissible startup modal listing all invalid paths and errors; errors also logged; invalid skills are excluded from rendering. The modal reappears on subsequent startups until resolved. -ASCII data flow (current + future roots) ----------------------------------------- - -Current (v1): - -``` -~/.codex/skills/... - ├─ skill_a/SKILL.md - └─ skill_b/SKILL.md - | - v -[core] load_skills -> validated SkillMetadata list - | - v -[core] render_skills_section -> "## Skills\n- name: desc (file: ...)" - | - v -[core] read_project_docs(agents.md) --+ - | | - +----------- merge ---------+--> runtime instructions (agents.md + Skills section) -``` - -Future (multi-root example): - -``` -Roots: - ~/.codex/skills - /agents.md + /skills/ - /crates/foo/agents.md + /crates/foo/skills/ - -For each root: - - discover skills (same rules: recursive, skip hidden/symlinks, SKILL.md only) - - validate -> SkillMetadata - - render section (optional if any skills) - -Per agents.md: - - read agents.md content - - append its local Skills section (if any) - - concatenate higher-level agents.md + Skills sections (root -> cwd) - -Result: - runtime instructions = user_instructions? + "--- project-doc ---" + concatenated (agents.md + per-root Skills section) +Mermaid data flow (current + future roots) +------------------------------------------ + +```mermaid +flowchart TD + subgraph Current_v1["Current (v1)"] + skills_dir["~/.codex/skills/**/SKILL.md (skip hidden/symlinks)"] + load[load_skills -> validate + sanitize -> SkillMetadata] + render[render_skills_section -> \"## Skills\\n- name: desc (file: ...)\"] + agents[read_project_docs(agents.md)] + merge[merge agents.md + Skills section\n(runtime only, no disk changes)] + skills_dir --> load --> render --> merge + agents --> merge --> final_v1["runtime instructions"] + end + + subgraph Future_multi_root["Future (multi-root)"] + roots["Roots: ~/.codex/skills; /skills next to agents.md; nested crate skills"] + per_root_discover["per-root: discover skills (recursive, same rules)"] + per_root_validate["per-root: validate -> SkillMetadata"] + per_root_render["per-root: render optional Skills section"] + per_agents["per agents.md: read content, append its Skills section"] + concat["concatenate from repo root -> cwd"] + final_future["runtime instructions\n= user_instructions? + \"--- project-doc ---\" + concatenated agents+skills"] + roots --> per_root_discover --> per_root_validate --> per_root_render --> per_agents --> concat --> final_future + end ``` From 1705bf80cf35ae8a5da6133f0320937459045715 Mon Sep 17 00:00:00 2001 From: Thibault Sottiaux Date: Sun, 30 Nov 2025 15:10:23 -0800 Subject: [PATCH 04/24] Add ASCII skills data flow diagram --- skills_plan.md | 68 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/skills_plan.md b/skills_plan.md index a2f3b7d61a..d0c4f5bd6d 100644 --- a/skills_plan.md +++ b/skills_plan.md @@ -129,7 +129,7 @@ flowchart TD subgraph Current_v1["Current (v1)"] skills_dir["~/.codex/skills/**/SKILL.md (skip hidden/symlinks)"] load[load_skills -> validate + sanitize -> SkillMetadata] - render[render_skills_section -> \"## Skills\\n- name: desc (file: ...)\"] + render[render_skills_section -> \"## Skills\\n- name: desc (file: ... )\"] agents[read_project_docs(agents.md)] merge[merge agents.md + Skills section\n(runtime only, no disk changes)] skills_dir --> load --> render --> merge @@ -147,3 +147,69 @@ flowchart TD roots --> per_root_discover --> per_root_validate --> per_root_render --> per_agents --> concat --> final_future end ``` + +ASCII data flow (detailed v1) +----------------------------- + +``` + +-----------------------+ + | ~/.codex/skills/**/ | + | SKILL.md files | + | (skip hidden/symlink) | + +-----------+-----------+ + | + v + +--------------------------+ + | load_skills (core) | + | - walk dirs | + | - parse YAML frontmatter | + | - sanitize name/desc | + | - enforce length caps | + | - collect SkillMetadata | + | - collect SkillError | + +------+-------------------+ + | valid skills + v + +--------------------------+ + | render_skills_section | + | - build "## Skills" | + | - one-line bullets | + | - absolute paths | + +------+-------------------+ + | skills section (optional) + v + +-------------+---------------------------+ + | read_project_docs (core) | + | - read AGENTS.md chain (root->cwd) | + | - respect byte budget | + +-------------+---------------------------+ + | + v + +-------------+---------------------------+ + | merge_project_docs_with_skills | + | - doc only | + | - doc + skills | + | - skills only | + | - none -> None | + +-------------+---------------------------+ + | + v + +-------------+---------------------------+ + | get_user_instructions | + | - combine with user_instructions + | + | "--- project-doc ---" separator | + | - returned to session config | + +-----------------------------------------+ + +Invalid skills path (parallel): + + SkillError list --------------+ + v + +-----------------------------+ + | TUI startup modal | + | - lists path + error | + | - dismiss to continue | + | - Ctrl+C/Ctrl+D to exit | + | - logs at error level | + +-----------------------------+ +``` From e14249e04cd91289a60111c1d17d1460b4b806bb Mon Sep 17 00:00:00 2001 From: Thibault Sottiaux Date: Sun, 30 Nov 2025 15:12:42 -0800 Subject: [PATCH 05/24] Update skills plan --- skills_plan.md | 102 +------------------------------------------------ 1 file changed, 1 insertion(+), 101 deletions(-) diff --git a/skills_plan.md b/skills_plan.md index d0c4f5bd6d..5c4512ab79 100644 --- a/skills_plan.md +++ b/skills_plan.md @@ -61,7 +61,7 @@ Error handling and UX - Persistence: the modal reappears on every startup until all invalid skills are fixed or removed. - Logging: also emit error-level logs for each invalid skill, matching what is shown in the modal. - Post-dismissal behavior: invalid skills are ignored for rendering; valid skills still render. -- Reuse existing modal/popup pattern in the codebase for consistency; add a button to dismiss/continue. +- Reuse existing modal/popup pattern in the codebase for consistency; add an option to dismiss/continue. Update cadence -------------- @@ -108,108 +108,8 @@ Testing approach - Create valid/invalid SKILLs under `~/.codex/skills`, start codex, observe modal and final context. - Confirm agents.md on disk remains unchanged. -Risks and mitigations ---------------------- -- Risk: Excessively long descriptions breaking layout. Mitigation: hard length limits and sanitization. -- Risk: Users surprised by blocking modal. Mitigation: clear messaging, dismiss to proceed, repeat only until fixed. -- Risk: Future roots complicate ordering. Mitigation: keep explicit ordering rules (name, then path) and stable formatting. -- Risk: Context bloat if too many skills. Mitigation: metadata-only render, short descriptions, length caps; consider future pagination or caps if needed. - Acceptance criteria ------------------- - With valid skills present, the runtime context includes a final `## Skills` section with the intro and one-line bullets (name, description, absolute path), sorted by name then path. agents.md on disk is untouched. - If no skills are present, no Skills section is injected. - Any invalid skill causes a blocking, dismissible startup modal listing all invalid paths and errors; errors also logged; invalid skills are excluded from rendering. The modal reappears on subsequent startups until resolved. - -Mermaid data flow (current + future roots) ------------------------------------------- - -```mermaid -flowchart TD - subgraph Current_v1["Current (v1)"] - skills_dir["~/.codex/skills/**/SKILL.md (skip hidden/symlinks)"] - load[load_skills -> validate + sanitize -> SkillMetadata] - render[render_skills_section -> \"## Skills\\n- name: desc (file: ... )\"] - agents[read_project_docs(agents.md)] - merge[merge agents.md + Skills section\n(runtime only, no disk changes)] - skills_dir --> load --> render --> merge - agents --> merge --> final_v1["runtime instructions"] - end - - subgraph Future_multi_root["Future (multi-root)"] - roots["Roots: ~/.codex/skills; /skills next to agents.md; nested crate skills"] - per_root_discover["per-root: discover skills (recursive, same rules)"] - per_root_validate["per-root: validate -> SkillMetadata"] - per_root_render["per-root: render optional Skills section"] - per_agents["per agents.md: read content, append its Skills section"] - concat["concatenate from repo root -> cwd"] - final_future["runtime instructions\n= user_instructions? + \"--- project-doc ---\" + concatenated agents+skills"] - roots --> per_root_discover --> per_root_validate --> per_root_render --> per_agents --> concat --> final_future - end -``` - -ASCII data flow (detailed v1) ------------------------------ - -``` - +-----------------------+ - | ~/.codex/skills/**/ | - | SKILL.md files | - | (skip hidden/symlink) | - +-----------+-----------+ - | - v - +--------------------------+ - | load_skills (core) | - | - walk dirs | - | - parse YAML frontmatter | - | - sanitize name/desc | - | - enforce length caps | - | - collect SkillMetadata | - | - collect SkillError | - +------+-------------------+ - | valid skills - v - +--------------------------+ - | render_skills_section | - | - build "## Skills" | - | - one-line bullets | - | - absolute paths | - +------+-------------------+ - | skills section (optional) - v - +-------------+---------------------------+ - | read_project_docs (core) | - | - read AGENTS.md chain (root->cwd) | - | - respect byte budget | - +-------------+---------------------------+ - | - v - +-------------+---------------------------+ - | merge_project_docs_with_skills | - | - doc only | - | - doc + skills | - | - skills only | - | - none -> None | - +-------------+---------------------------+ - | - v - +-------------+---------------------------+ - | get_user_instructions | - | - combine with user_instructions + | - | "--- project-doc ---" separator | - | - returned to session config | - +-----------------------------------------+ - -Invalid skills path (parallel): - - SkillError list --------------+ - v - +-----------------------------+ - | TUI startup modal | - | - lists path + error | - | - dismiss to continue | - | - Ctrl+C/Ctrl+D to exit | - | - logs at error level | - +-----------------------------+ -``` From ca4680e91ee27a6b37c0a78ac94623ad66f85bb2 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 14:00:59 -0800 Subject: [PATCH 06/24] Normalize Windows paths in skills tests --- codex-rs/core/src/project_doc.rs | 32 ++++++++++++++++++++------------ codex-rs/core/src/skills.rs | 9 +++------ 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index 90b28d5ae4..ae5311438b 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -493,14 +493,18 @@ mod tests { let res = get_user_instructions(&cfg) .await .expect("instructions expected"); - let expected_path = cfg - .codex_home - .join("skills/pdf-processing/SKILL.md") - .canonicalize() - .unwrap(); + let expected_path = dunce::canonicalize( + cfg.codex_home + .join("skills/pdf-processing/SKILL.md") + .as_path(), + ) + .unwrap_or_else(|_| cfg.codex_home.join("skills/pdf-processing/SKILL.md")); + let expected_path_str = expected_path + .to_string_lossy() + .replace('\\', "/"); let expected = format!( "base doc\n\n## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- pdf-processing: extract from pdfs (file: {})", - expected_path.display() + expected_path_str ); assert_eq!(res, expected); } @@ -514,14 +518,18 @@ mod tests { let res = get_user_instructions(&cfg) .await .expect("instructions expected"); - let expected_path = cfg - .codex_home - .join("skills/linting/SKILL.md") - .canonicalize() - .unwrap(); + let expected_path = dunce::canonicalize( + cfg.codex_home + .join("skills/linting/SKILL.md") + .as_path(), + ) + .unwrap_or_else(|_| cfg.codex_home.join("skills/linting/SKILL.md")); + let expected_path_str = expected_path + .to_string_lossy() + .replace('\\', "/"); let expected = format!( "## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- linting: run clippy (file: {})", - expected_path.display() + expected_path_str ); assert_eq!(res, expected); } diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 270e3fba95..f9a125c254 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -240,13 +240,10 @@ mod tests { let skill = &outcome.skills[0]; assert_eq!(skill.name, "demo-skill"); assert_eq!(skill.description, "does things carefully"); + let path_str = skill.path.to_string_lossy().replace('\\', "/"); assert!( - skill - .path - .to_string_lossy() - .ends_with("skills/demo/SKILL.md"), - "unexpected path {}", - skill.path.display() + path_str.ends_with("skills/demo/SKILL.md"), + "unexpected path {path_str}" ); } From f3122f60c599e897a90a05738c37949f165d129a Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 14:58:13 -0800 Subject: [PATCH 07/24] fmt and lint fixes --- codex-rs/core/src/project_doc.rs | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index ae5311438b..13b14ebc00 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -499,12 +499,9 @@ mod tests { .as_path(), ) .unwrap_or_else(|_| cfg.codex_home.join("skills/pdf-processing/SKILL.md")); - let expected_path_str = expected_path - .to_string_lossy() - .replace('\\', "/"); + let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); let expected = format!( - "base doc\n\n## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- pdf-processing: extract from pdfs (file: {})", - expected_path_str + "base doc\n\n## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- pdf-processing: extract from pdfs (file: {expected_path_str})" ); assert_eq!(res, expected); } @@ -518,18 +515,12 @@ mod tests { let res = get_user_instructions(&cfg) .await .expect("instructions expected"); - let expected_path = dunce::canonicalize( - cfg.codex_home - .join("skills/linting/SKILL.md") - .as_path(), - ) - .unwrap_or_else(|_| cfg.codex_home.join("skills/linting/SKILL.md")); - let expected_path_str = expected_path - .to_string_lossy() - .replace('\\', "/"); + let expected_path = + dunce::canonicalize(cfg.codex_home.join("skills/linting/SKILL.md").as_path()) + .unwrap_or_else(|_| cfg.codex_home.join("skills/linting/SKILL.md")); + let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); let expected = format!( - "## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- linting: run clippy (file: {})", - expected_path_str + "## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- linting: run clippy (file: {expected_path_str})" ); assert_eq!(res, expected); } From 100a8bb4cc6e9c9376606d0aeab8aa3daeef0e6c Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 15:00:03 -0800 Subject: [PATCH 08/24] Add skills documentation (experimental) --- docs/skills.md | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 docs/skills.md diff --git a/docs/skills.md b/docs/skills.md new file mode 100644 index 0000000000..22481d9c57 --- /dev/null +++ b/docs/skills.md @@ -0,0 +1,54 @@ +# Skills (experimental) + +> **Warning:** This is an experimental and non-stable feature. If you depend on it, please expect breaking changes over the coming weeks and understand that there is currently no guarantee that this works well. Use at your own risk! + +Codex can automatically discover reusable "skills" you keep on disk. A skill is a small bundle with a name, a short description (what it does and when to use it), and an optional body of instructions you can open when needed. Codex injects only the name, description, and file path into the runtime context; the body stays on disk. + +## Where skills live +- Location (v1): `~/.codex/skills/**/SKILL.md` (recursive). Hidden entries and symlinks are skipped. Only files named exactly `SKILL.md` count. +- Sorting: rendered by name, then path for stability. + +## File format +- YAML frontmatter + body. + - Required: + - `name` (non-empty, ≤100 chars, sanitized to one line) + - `description` (non-empty, ≤500 chars, sanitized to one line) + - Extra keys are ignored. The body can contain any Markdown; it is not injected into context. + +## Loading and rendering +- Loaded once at startup. +- If valid skills exist, Codex appends a runtime-only `## Skills` section after `AGENTS.md`, one bullet per skill: `- : (file: /absolute/path/to/SKILL.md)`. +- If no valid skills exist, the section is omitted. On-disk files are never modified. + +## Validation and errors +- Invalid skills (missing/invalid YAML, empty/over-length fields) trigger a blocking, dismissible startup modal in the TUI that lists each path and error. Errors are also logged. You can dismiss to continue (invalid skills are ignored) or exit. Fix SKILL.md files and restart to clear the modal. + +## Create a skill +1. Create `~/.codex/skills//`. +2. Add `SKILL.md`: + ``` + --- + name: your-skill-name + description: what it does and when to use it (<=500 chars) + --- + + # Optional body + Add instructions, references, examples, or scripts (kept on disk). + ``` +3. Keep `name`/`description` within the limits; avoid newlines in those fields. +4. Restart Codex to load the new skill. + +## Example +``` +mkdir -p ~/.codex/skills/pdf-processing +cat <<'SKILL_EXAMPLE' > ~/.codex/skills/pdf-processing/SKILL.md +--- +name: pdf-processing +description: Extract text and tables from PDFs; use when PDFs, forms, or document extraction are mentioned. +--- + +# PDF Processing +- Use pdfplumber to extract text. +- For form filling, see FORMS.md. +SKILL_EXAMPLE +``` \ No newline at end of file From 417168a01ab2897746429ceaa7aa6c66ba9ad45a Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 15:01:08 -0800 Subject: [PATCH 09/24] Delete design doc --- skills_plan.md | 115 ------------------------------------------------- 1 file changed, 115 deletions(-) delete mode 100644 skills_plan.md diff --git a/skills_plan.md b/skills_plan.md deleted file mode 100644 index 5c4512ab79..0000000000 --- a/skills_plan.md +++ /dev/null @@ -1,115 +0,0 @@ -Skills Injection Design Doc -=========================== - -Context and motivation ----------------------- -Codex currently has no notion of reusable “skills” that can be discovered on disk and injected into the runtime context. We want to mirror the productive parts of other prior art in this space while keeping the first iteration deliberately small and safe. - -Goals: -- Act as an extension of the existing agents.md mechanism, which allows progressive disclosure of content and instructions driven by the agent's own decision making process. -- Reduce repeated prompting by letting users package domain expertise once. - -Non-goals for first implementation (v1): tool restrictions, hot reload, network/package installation, hierarchies of skills with their own progressive disclosure or supporting skills defined elsewhere than the user home root. - -Implementation considerations: -- Keep the runtime context lean by injecting only lightweight metadata (name, description, path) and not inlining full instructions. -- Avoid mutating source files; the feature should be entirely runtime-driven and reversible. -- Establish a structure that can grow to project-local skills later (e.g., alongside agents.md in a workspace). -- Provide strong validation and clear failure modes so misconfigured skills do not silently degrade agent behavior. - -High-level behavior -------------------- -- On startup, codex discovers skills from the user home root `~/.codex/skills`, recursively. -- Each skill lives in a directory containing a file named exactly `SKILL.md` with YAML frontmatter (`name`, `description`) and a Markdown body. Extra keys are allowed and ignored. -- Valid skills are rendered into the agent context by dynamically appending a `## Skills` section to the in-memory content of the root `agents.md`. The file on disk remains unchanged. -- The `## Skills` section contains one-line bullets per skill: name, description, and absolute path to the `SKILL.md`, plus a brief intro paragraph explaining usage. No skill body content is inlined. -- Invalid skills (parse errors, missing required fields, overlength fields) block startup with a dismissible modal that lists every invalid path and error. Errors are also logged. After dismissal, invalid skills are ignored for rendering. The modal reappears on subsequent startups until the issues are fixed. -- If no valid skills are found, the Skills section is omitted entirely. -- Loading occurs once at startup; no hot reload in v1. - -Discovery rules ---------------- -- Roots: only `~/.codex/skills` in v1. Design the loader so additional roots can be added later (e.g., `/skills` alongside `/agents.md` for project-scoped skills). -- Recursion: traverse all subdirectories. -- Hidden entries: skip hidden directories and files (prefix “.”). -- Symlinks: do not follow symlinks. -- Recognition: only files named exactly `SKILL.md` qualify as skills. -- Ordering: collect all skills (no dedupe), then sort by `name`, then by absolute path for stable rendering. - -Skill format and validation ---------------------------- -- File: `SKILL.md` must start with YAML frontmatter delimited by `---` on its own lines; the next `---` ends the frontmatter. The remainder is body (ignored for rendering). -- Required fields: `name` (string), `description` (string). -- Length constraints: `name` non-empty, max 100 chars; `description` non-empty, max 500 chars. Enforce hard errors if exceeded. -- Sanitization for rendering: trim, collapse newlines/tabs/extra whitespace inside `name` and `description` to single spaces to preserve single-line output. -- Extra keys: allowed and ignored for forward compatibility. -- YAML parsing: use serde_yaml; accept CRLF; any parse error or missing required field is a hard error. - -Rendering model ---------------- -- Base content: read the root `agents.md` (do not mutate it). -- Synthetic section: append to the in-memory content a final section only if there is at least one valid skill. - - Heading: `## Skills` - - Intro paragraph (single line), suggested copy: “These skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.” - - Entries: one bullet per skill, single line, format `- : (file: /absolute/path/to/SKILL.md)`. Use sanitized fields to avoid line breaks. - - Paths: always absolute for clarity. -- If no valid skills: omit the section entirely. - -Error handling and UX ---------------------- -- Invalid skills: any validation or parse failure is treated as blocking. On startup, present a dismissible modal that lists every invalid skill with its path and detailed error message. Startup is paused until the user dismisses. -- Persistence: the modal reappears on every startup until all invalid skills are fixed or removed. -- Logging: also emit error-level logs for each invalid skill, matching what is shown in the modal. -- Post-dismissal behavior: invalid skills are ignored for rendering; valid skills still render. -- Reuse existing modal/popup pattern in the codebase for consistency; add an option to dismiss/continue. - -Update cadence --------------- -- Load skills once at startup. No hot-reload, no file watching. Users must restart codex for changes to take effect. - -Future-proofing ---------------- -- Multiple roots: structure the loader to accept a list of roots (initially one: `~/.codex/skills`). Future additions: `/skills` co-located with `/agents.md`, or other configurable roots. -- Rendering target: keep the renderer generic—given base agents.md content and a skill list, return augmented content. This will let us plug in different agents.md locations or multiple agents files later. -- Allowed-tools: not implemented in v1, but the validator should ignore extra keys so later fields can be added without breaking old skills. -- Potential later enhancements: hot reload; per-project skills; UI for listing valid skills; richer summaries; optional inline previews under a size threshold; deduplication policies. - -Implementation outline ----------------------- -- Discovery: - - Expand `~` to absolute path. - - Walk directories recursively; skip hidden entries; do not follow symlinks. - - Collect paths exactly matching `SKILL.md`. -- Parsing/validation: - - For each path, read file; locate frontmatter between `---` lines. - - Parse with serde_yaml into a struct { name: String, description: String, …ignored }. - - Trim and sanitize; enforce non-empty and length limits. - - On failure, record an error entry (path + message). -- Rendering: - - Sort valid skills by name, then path. - - If none, return base agents.md unchanged. - - Otherwise, append the section with intro and bullets, ensuring one-line entries. -- Error surfacing: - - If any errors exist, build a blocking modal view at startup listing all (path + message), with a dismiss/continue button. Use existing modal screen as template. - - Log all errors via the standard logger at error level. - - After dismissal, proceed with rendering valid skills only. - -Testing approach ----------------- -- Unit tests: - - Frontmatter parsing and validation (missing fields, overlength, malformed YAML). - - Sanitization (newline/tab collapsing). - - Ordering and rendering format. - - Hidden/ symlink skipping. -- Integration tests: - - With a temp `~/.codex/skills` tree containing multiple valid and invalid skills, verify augmented agents content and that invalid ones are omitted. - - Modal trigger: simulate startup with invalid skills and assert the error list is produced (if test harness supports UI assertions). -- Manual checks: - - Create valid/invalid SKILLs under `~/.codex/skills`, start codex, observe modal and final context. - - Confirm agents.md on disk remains unchanged. - -Acceptance criteria -------------------- -- With valid skills present, the runtime context includes a final `## Skills` section with the intro and one-line bullets (name, description, absolute path), sorted by name then path. agents.md on disk is untouched. -- If no skills are present, no Skills section is injected. -- Any invalid skill causes a blocking, dismissible startup modal listing all invalid paths and errors; errors also logged; invalid skills are excluded from rendering. The modal reappears on subsequent startups until resolved. From 3c215feff59f804cc169eae1e30a1ecccfd570b2 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 15:14:20 -0800 Subject: [PATCH 10/24] prettier format new markdown file --- docs/skills.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/skills.md b/docs/skills.md index 22481d9c57..81bc278797 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -5,10 +5,12 @@ Codex can automatically discover reusable "skills" you keep on disk. A skill is a small bundle with a name, a short description (what it does and when to use it), and an optional body of instructions you can open when needed. Codex injects only the name, description, and file path into the runtime context; the body stays on disk. ## Where skills live + - Location (v1): `~/.codex/skills/**/SKILL.md` (recursive). Hidden entries and symlinks are skipped. Only files named exactly `SKILL.md` count. - Sorting: rendered by name, then path for stability. ## File format + - YAML frontmatter + body. - Required: - `name` (non-empty, ≤100 chars, sanitized to one line) @@ -16,16 +18,20 @@ Codex can automatically discover reusable "skills" you keep on disk. A skill is - Extra keys are ignored. The body can contain any Markdown; it is not injected into context. ## Loading and rendering + - Loaded once at startup. - If valid skills exist, Codex appends a runtime-only `## Skills` section after `AGENTS.md`, one bullet per skill: `- : (file: /absolute/path/to/SKILL.md)`. - If no valid skills exist, the section is omitted. On-disk files are never modified. ## Validation and errors + - Invalid skills (missing/invalid YAML, empty/over-length fields) trigger a blocking, dismissible startup modal in the TUI that lists each path and error. Errors are also logged. You can dismiss to continue (invalid skills are ignored) or exit. Fix SKILL.md files and restart to clear the modal. ## Create a skill + 1. Create `~/.codex/skills//`. 2. Add `SKILL.md`: + ``` --- name: your-skill-name @@ -35,10 +41,12 @@ Codex can automatically discover reusable "skills" you keep on disk. A skill is # Optional body Add instructions, references, examples, or scripts (kept on disk). ``` + 3. Keep `name`/`description` within the limits; avoid newlines in those fields. 4. Restart Codex to load the new skill. ## Example + ``` mkdir -p ~/.codex/skills/pdf-processing cat <<'SKILL_EXAMPLE' > ~/.codex/skills/pdf-processing/SKILL.md @@ -51,4 +59,4 @@ description: Extract text and tables from PDFs; use when PDFs, forms, or documen - Use pdfplumber to extract text. - For form filling, see FORMS.md. SKILL_EXAMPLE -``` \ No newline at end of file +``` From 2dfb2fb19740bf767a23296440d2569ea6798af6 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 15:24:11 -0800 Subject: [PATCH 11/24] Normalize path when extracting for Windows --- codex-rs/core/src/skills.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index f9a125c254..0ef17a5a15 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -61,11 +61,10 @@ pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { lines.push("These skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.".to_string()); for skill in skills { + let path_str = skill.path.to_string_lossy().replace('\\', "/"); lines.push(format!( "- {}: {} (file: {})", - skill.name, - skill.description, - skill.path.display() + skill.name, skill.description, path_str )); } From e401362a08784d11e943dda5ebdca3386ee3e808 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 15:47:14 -0800 Subject: [PATCH 12/24] Reorganize skills into folder --- .../core/src/{skills.rs => skills/loader.rs} | 42 ++----------------- codex-rs/core/src/skills/mod.rs | 7 ++++ codex-rs/core/src/skills/model.rs | 20 +++++++++ codex-rs/core/src/skills/render.rs | 23 ++++++++++ 4 files changed, 53 insertions(+), 39 deletions(-) rename codex-rs/core/src/{skills.rs => skills/loader.rs} (87%) create mode 100644 codex-rs/core/src/skills/mod.rs create mode 100644 codex-rs/core/src/skills/model.rs create mode 100644 codex-rs/core/src/skills/render.rs diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills/loader.rs similarity index 87% rename from codex-rs/core/src/skills.rs rename to codex-rs/core/src/skills/loader.rs index 0ef17a5a15..4607e096b9 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -1,4 +1,7 @@ use crate::config::Config; +use crate::skills::model::SkillError; +use crate::skills::model::SkillLoadOutcome; +use crate::skills::model::SkillMetadata; use dunce::canonicalize as normalize_path; use serde::Deserialize; use std::collections::VecDeque; @@ -7,25 +10,6 @@ use std::path::Path; use std::path::PathBuf; use tracing::error; -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct SkillMetadata { - pub name: String, - pub description: String, - pub path: PathBuf, -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct SkillError { - pub path: PathBuf, - pub message: String, -} - -#[derive(Debug, Clone, Default)] -pub struct SkillLoadOutcome { - pub skills: Vec, - pub errors: Vec, -} - #[derive(Debug, Deserialize)] struct SkillFrontmatter { name: String, @@ -51,26 +35,6 @@ pub fn load_skills(config: &Config) -> SkillLoadOutcome { outcome } -pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { - if skills.is_empty() { - return None; - } - - let mut lines: Vec = Vec::new(); - lines.push("## Skills".to_string()); - lines.push("These skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.".to_string()); - - for skill in skills { - let path_str = skill.path.to_string_lossy().replace('\\', "/"); - lines.push(format!( - "- {}: {} (file: {})", - skill.name, skill.description, path_str - )); - } - - Some(lines.join("\n")) -} - fn skill_roots(config: &Config) -> Vec { vec![config.codex_home.join(SKILLS_DIR_NAME)] } diff --git a/codex-rs/core/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs new file mode 100644 index 0000000000..1ebe69c03a --- /dev/null +++ b/codex-rs/core/src/skills/mod.rs @@ -0,0 +1,7 @@ +pub mod model; +pub mod render; +pub mod loader; + +pub use loader::{load_skills, SkillError, SkillLoadOutcome}; +pub use model::{SkillMetadata}; +pub use render::render_skills_section; diff --git a/codex-rs/core/src/skills/model.rs b/codex-rs/core/src/skills/model.rs new file mode 100644 index 0000000000..83a3317d57 --- /dev/null +++ b/codex-rs/core/src/skills/model.rs @@ -0,0 +1,20 @@ +use std::path::PathBuf; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillMetadata { + pub name: String, + pub description: String, + pub path: PathBuf, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillError { + pub path: PathBuf, + pub message: String, +} + +#[derive(Debug, Clone, Default)] +pub struct SkillLoadOutcome { + pub skills: Vec, + pub errors: Vec, +} diff --git a/codex-rs/core/src/skills/render.rs b/codex-rs/core/src/skills/render.rs new file mode 100644 index 0000000000..da2aee4ab4 --- /dev/null +++ b/codex-rs/core/src/skills/render.rs @@ -0,0 +1,23 @@ +use crate::skills::model::SkillMetadata; + +pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { + if skills.is_empty() { + return None; + } + + let mut lines: Vec = Vec::new(); + lines.push("## Skills".to_string()); + lines.push("These skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.".to_string()); + + for skill in skills { + let path_str = skill.path.to_string_lossy().replace('\\', "/"); + lines.push(format!( + "- {}: {} (file: {})", + skill.name, + skill.description, + path_str + )); + } + + Some(lines.join("\n")) +} From 9f5fef6084ba7dee81865aef51058f535c3d3d01 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 15:48:32 -0800 Subject: [PATCH 13/24] Rename constant --- codex-rs/core/src/skills/loader.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 4607e096b9..690bfc29a7 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -16,7 +16,7 @@ struct SkillFrontmatter { description: String, } -const SKILL_FILENAME: &str = "SKILL.md"; +const SKILLS_FILENAME: &str = "SKILL.md"; const SKILLS_DIR_NAME: &str = "skills"; const MAX_NAME_LEN: usize = 100; const MAX_DESCRIPTION_LEN: usize = 500; @@ -83,7 +83,7 @@ fn discover_skills_under_root(root: &Path, outcome: &mut SkillLoadOutcome) { continue; } - if file_type.is_file() && file_name == SKILL_FILENAME { + if file_type.is_file() && file_name == SKILLS_FILENAME { match parse_skill_file(&path) { Ok(skill) => outcome.skills.push(skill), Err(message) => outcome.errors.push(SkillError { path, message }), @@ -182,7 +182,7 @@ mod tests { let content = format!( "---\nname: {name}\ndescription: |-\n {indented_description}\n---\n\n# Body\n" ); - let path = skill_dir.join(SKILL_FILENAME); + let path = skill_dir.join(SKILLS_FILENAME); fs::write(&path, content).unwrap(); path } @@ -216,7 +216,7 @@ mod tests { let hidden_dir = codex_home.path().join("skills/.hidden"); fs::create_dir_all(&hidden_dir).unwrap(); fs::write( - hidden_dir.join(SKILL_FILENAME), + hidden_dir.join(SKILLS_FILENAME), "---\nname: hidden\ndescription: hidden\n---\n", ) .unwrap(); @@ -224,7 +224,7 @@ mod tests { // Invalid because missing closing frontmatter. let invalid_dir = codex_home.path().join("skills/invalid"); fs::create_dir_all(&invalid_dir).unwrap(); - fs::write(invalid_dir.join(SKILL_FILENAME), "---\nname: bad").unwrap(); + fs::write(invalid_dir.join(SKILLS_FILENAME), "---\nname: bad").unwrap(); let cfg = make_config(&codex_home); let outcome = load_skills(&cfg); From 95fc467a181c9d0730001a66c0e23810323ad4e0 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 15:53:37 -0800 Subject: [PATCH 14/24] fmt fixes --- codex-rs/core/src/skills/mod.rs | 8 +++++--- codex-rs/core/src/skills/render.rs | 4 +--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs index 1ebe69c03a..ebb1490c99 100644 --- a/codex-rs/core/src/skills/mod.rs +++ b/codex-rs/core/src/skills/mod.rs @@ -1,7 +1,9 @@ +pub mod loader; pub mod model; pub mod render; -pub mod loader; -pub use loader::{load_skills, SkillError, SkillLoadOutcome}; -pub use model::{SkillMetadata}; +pub use loader::load_skills; +pub use model::SkillError; +pub use model::SkillLoadOutcome; +pub use model::SkillMetadata; pub use render::render_skills_section; diff --git a/codex-rs/core/src/skills/render.rs b/codex-rs/core/src/skills/render.rs index da2aee4ab4..d547e21c28 100644 --- a/codex-rs/core/src/skills/render.rs +++ b/codex-rs/core/src/skills/render.rs @@ -13,9 +13,7 @@ pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { let path_str = skill.path.to_string_lossy().replace('\\', "/"); lines.push(format!( "- {}: {} (file: {})", - skill.name, - skill.description, - path_str + skill.name, skill.description, path_str )); } From 75e30f5d07d02a14315d2d94d36c4f09268597d7 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 16:31:31 -0800 Subject: [PATCH 15/24] Add CLI argument to feature gate loading of skills --- codex-rs/cli/src/main.rs | 12 ++++++++++++ codex-rs/core/src/features.rs | 8 ++++++++ codex-rs/core/src/project_doc.rs | 22 +++++++++++++--------- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 2b066197af..23bfe796aa 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -358,6 +358,15 @@ struct FeatureToggles { /// Disable a feature (repeatable). Equivalent to `-c features.=false`. #[arg(long = "disable", value_name = "FEATURE", action = clap::ArgAction::Append, global = true)] disable: Vec, + + /// [experimental] Enable skills loading/injection. + #[arg( + long = "enable-skills", + default_value_t = false, + global = true, + hide = true + )] + enable_skills: bool, } impl FeatureToggles { @@ -371,6 +380,9 @@ impl FeatureToggles { Self::validate_feature(feature)?; v.push(format!("features.{feature}=false")); } + if self.enable_skills { + v.push("features.skills=true".to_string()); + } Ok(v) } diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 0c67c9ff5c..14f0404b35 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -51,6 +51,8 @@ pub enum Feature { ShellTool, /// Allow model to call multiple tools in parallel (only for models supporting it). ParallelToolCalls, + /// Experimental skills injection (CLI flag-driven). + Skills, } impl Feature { @@ -326,4 +328,10 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: true, }, + FeatureSpec { + id: Feature::Skills, + key: "skills", + stage: Stage::Experimental, + default_enabled: false, + }, ]; diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index 13b14ebc00..972e216fe2 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -33,15 +33,19 @@ const PROJECT_DOC_SEPARATOR: &str = "\n\n--- project-doc ---\n\n"; /// Combines `Config::instructions` and `AGENTS.md` (if present) into a single /// string of instructions. pub(crate) async fn get_user_instructions(config: &Config) -> Option { - let skills_outcome = load_skills(config); - for err in &skills_outcome.errors { - error!( - "failed to load skill {}: {}", - err.path.display(), - err.message - ); - } - let skills_section = render_skills_section(&skills_outcome.skills); + let skills_section = if config.features.enabled(Feature::Skills) { + let skills_outcome = load_skills(config); + for err in &skills_outcome.errors { + error!( + "failed to load skill {}: {}", + err.path.display(), + err.message + ); + } + render_skills_section(&skills_outcome.skills) + } else { + None + }; let project_docs = match read_project_docs(config).await { Ok(docs) => docs, From e5fea7bd7d11f3d68abe5f81fb87f7010754030e Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 16:34:49 -0800 Subject: [PATCH 16/24] Fix import --- codex-rs/core/src/project_doc.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index 972e216fe2..5ad767e0b3 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -14,6 +14,7 @@ //! 3. We do **not** walk past the Git root. use crate::config::Config; +use crate::features::Feature; use crate::skills::load_skills; use crate::skills::render_skills_section; use dunce::canonicalize as normalize_path; From f94d631ea6547c5521007a9fee1d00fdf28ee83c Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 17:03:50 -0800 Subject: [PATCH 17/24] Add CLI arg feature flag to tests --- codex-rs/cli/src/main.rs | 2 ++ codex-rs/core/src/project_doc.rs | 1 + 2 files changed, 3 insertions(+) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 23bfe796aa..9f1ec702d2 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -934,6 +934,7 @@ mod tests { let toggles = FeatureToggles { enable: vec!["web_search_request".to_string()], disable: vec!["unified_exec".to_string()], + enable_skills: false, }; let overrides = toggles.to_overrides().expect("valid features"); assert_eq!( @@ -950,6 +951,7 @@ mod tests { let toggles = FeatureToggles { enable: vec!["does_not_exist".to_string()], disable: Vec::new(), + enable_skills: false, }; let err = toggles .to_overrides() diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index 5ad767e0b3..d8c1df2e26 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -254,6 +254,7 @@ mod tests { config.cwd = root.path().to_path_buf(); config.project_doc_max_bytes = limit; + config.features.enable(Feature::Skills); config.user_instructions = instructions.map(ToOwned::to_owned); config From f5c618417280ce24c3933d3da77e5249f6101976 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 17:17:50 -0800 Subject: [PATCH 18/24] Check whether target is symlink before following it --- codex-rs/core/src/skills/loader.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 690bfc29a7..51127ebfaa 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -69,11 +69,10 @@ fn discover_skills_under_root(root: &Path, outcome: &mut SkillLoadOutcome) { continue; } - let Ok(metadata) = entry.metadata() else { + let Ok(file_type) = entry.file_type() else { continue; }; - let file_type = metadata.file_type(); if file_type.is_symlink() { continue; } From 926fe0b24532e95aa8fc2ceb403cca696838aaa2 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 18:08:33 -0800 Subject: [PATCH 19/24] Use feature enum natively --- codex-rs/cli/src/main.rs | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 9f1ec702d2..d10cf937a9 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -35,6 +35,7 @@ use crate::mcp_cmd::McpCli; use codex_core::config::Config; use codex_core::config::ConfigOverrides; +use codex_core::features::Feature; use codex_core::features::is_known_feature_key; /// Codex CLI @@ -358,15 +359,6 @@ struct FeatureToggles { /// Disable a feature (repeatable). Equivalent to `-c features.=false`. #[arg(long = "disable", value_name = "FEATURE", action = clap::ArgAction::Append, global = true)] disable: Vec, - - /// [experimental] Enable skills loading/injection. - #[arg( - long = "enable-skills", - default_value_t = false, - global = true, - hide = true - )] - enable_skills: bool, } impl FeatureToggles { @@ -380,9 +372,6 @@ impl FeatureToggles { Self::validate_feature(feature)?; v.push(format!("features.{feature}=false")); } - if self.enable_skills { - v.push("features.skills=true".to_string()); - } Ok(v) } @@ -934,7 +923,6 @@ mod tests { let toggles = FeatureToggles { enable: vec!["web_search_request".to_string()], disable: vec!["unified_exec".to_string()], - enable_skills: false, }; let overrides = toggles.to_overrides().expect("valid features"); assert_eq!( @@ -951,7 +939,6 @@ mod tests { let toggles = FeatureToggles { enable: vec!["does_not_exist".to_string()], disable: Vec::new(), - enable_skills: false, }; let err = toggles .to_overrides() From 1cee47c78cc26bdae10ae2a536fecd93c7ff480b Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 18:13:08 -0800 Subject: [PATCH 20/24] Return error enum when parsing skills --- codex-rs/core/src/skills/loader.rs | 58 ++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 51127ebfaa..7b88db3534 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -5,6 +5,8 @@ use crate::skills::model::SkillMetadata; use dunce::canonicalize as normalize_path; use serde::Deserialize; use std::collections::VecDeque; +use std::error::Error; +use std::fmt; use std::fs; use std::path::Path; use std::path::PathBuf; @@ -21,6 +23,33 @@ const SKILLS_DIR_NAME: &str = "skills"; const MAX_NAME_LEN: usize = 100; const MAX_DESCRIPTION_LEN: usize = 500; +#[derive(Debug)] +enum SkillParseError { + Read(std::io::Error), + MissingFrontmatter, + InvalidYaml(serde_yaml::Error), + MissingField(&'static str), + InvalidField { field: &'static str, reason: String }, +} + +impl fmt::Display for SkillParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + SkillParseError::Read(e) => write!(f, "failed to read file: {e}"), + SkillParseError::MissingFrontmatter => { + write!(f, "missing YAML frontmatter delimited by ---") + } + SkillParseError::InvalidYaml(e) => write!(f, "invalid YAML: {e}"), + SkillParseError::MissingField(field) => write!(f, "missing field `{field}`"), + SkillParseError::InvalidField { field, reason } => { + write!(f, "invalid {field}: {reason}") + } + } + } +} + +impl Error for SkillParseError {} + pub fn load_skills(config: &Config) -> SkillLoadOutcome { let mut outcome = SkillLoadOutcome::default(); let roots = skill_roots(config); @@ -85,21 +114,23 @@ fn discover_skills_under_root(root: &Path, outcome: &mut SkillLoadOutcome) { if file_type.is_file() && file_name == SKILLS_FILENAME { match parse_skill_file(&path) { Ok(skill) => outcome.skills.push(skill), - Err(message) => outcome.errors.push(SkillError { path, message }), + Err(err) => outcome.errors.push(SkillError { + path, + message: err.to_string(), + }), } } } } } -fn parse_skill_file(path: &Path) -> Result { - let contents = fs::read_to_string(path).map_err(|e| format!("failed to read file: {e}"))?; +fn parse_skill_file(path: &Path) -> Result { + let contents = fs::read_to_string(path).map_err(SkillParseError::Read)?; - let frontmatter = extract_frontmatter(&contents) - .ok_or_else(|| "missing YAML frontmatter delimited by ---".to_string())?; + let frontmatter = extract_frontmatter(&contents).ok_or(SkillParseError::MissingFrontmatter)?; let parsed: SkillFrontmatter = - serde_yaml::from_str(&frontmatter).map_err(|e| format!("invalid YAML: {e}"))?; + serde_yaml::from_str(&frontmatter).map_err(SkillParseError::InvalidYaml)?; let name = sanitize_single_line(&parsed.name); let description = sanitize_single_line(&parsed.description); @@ -120,14 +151,19 @@ fn sanitize_single_line(raw: &str) -> String { raw.split_whitespace().collect::>().join(" ") } -fn validate_field(value: &str, max_len: usize, field_name: &str) -> Result<(), String> { +fn validate_field( + value: &str, + max_len: usize, + field_name: &'static str, +) -> Result<(), SkillParseError> { if value.is_empty() { - return Err(format!("{field_name} is required")); + return Err(SkillParseError::MissingField(field_name)); } if value.len() > max_len { - return Err(format!( - "{field_name} exceeds maximum length of {max_len} characters" - )); + return Err(SkillParseError::InvalidField { + field: field_name, + reason: format!("exceeds maximum length of {max_len} characters"), + }); } Ok(()) } From 9d354e508ec3e25ed981f4fb417771d85efe4934 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 18:18:47 -0800 Subject: [PATCH 21/24] Update test to match typed error --- codex-rs/core/src/skills/loader.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 7b88db3534..a9ea156f02 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -284,9 +284,7 @@ mod tests { assert_eq!(outcome.skills.len(), 0); assert_eq!(outcome.errors.len(), 1); assert!( - outcome.errors[0] - .message - .contains("description exceeds maximum"), + outcome.errors[0].message.contains("invalid description"), "expected length error" ); } From 2e282f5326bd371f10dbb5f340bf444ab42dc97f Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 18:21:14 -0800 Subject: [PATCH 22/24] Join strings instead of quadruple-match --- codex-rs/core/src/project_doc.rs | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index d8c1df2e26..ee3148e7e7 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -58,13 +58,23 @@ pub(crate) async fn get_user_instructions(config: &Config) -> Option { let combined_project_docs = merge_project_docs_with_skills(project_docs, skills_section); - match (config.user_instructions.clone(), combined_project_docs) { - (Some(instructions), Some(project_doc)) => Some(format!( - "{instructions}{PROJECT_DOC_SEPARATOR}{project_doc}" - )), - (Some(instructions), None) => Some(instructions), - (None, Some(project_doc)) => Some(project_doc), - (None, None) => None, + let mut parts: Vec = Vec::new(); + + if let Some(instructions) = config.user_instructions.clone() { + parts.push(instructions); + } + + if let Some(project_doc) = combined_project_docs { + if !parts.is_empty() { + parts.push(PROJECT_DOC_SEPARATOR.to_string()); + } + parts.push(project_doc); + } + + if parts.is_empty() { + None + } else { + Some(parts.concat()) } } From 334e98cbc06e03050ee8956f9c69579b67fad1ef Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 19:09:04 -0800 Subject: [PATCH 23/24] Add test suite for loading skills --- codex-rs/core/tests/suite/client.rs | 70 +++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 71bcd5192d..e074d29755 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -15,6 +15,7 @@ use codex_core::WireApi; use codex_core::auth::AuthCredentialsStoreMode; use codex_core::built_in_model_providers; use codex_core::error::CodexErr; +use codex_core::features::Feature; use codex_core::model_family::find_family_for_model; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; @@ -34,6 +35,7 @@ use core_test_support::skip_if_no_network; use core_test_support::test_codex::TestCodex; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; +use dunce::canonicalize as normalize_path; use futures::StreamExt; use serde_json::json; use std::io::Write; @@ -620,6 +622,74 @@ async fn includes_user_instructions_message_in_request() { assert_message_ends_with(&request_body["input"][1], ""); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn skills_append_to_instructions_when_feature_enabled() { + skip_if_no_network!(); + let server = MockServer::start().await; + + let resp_mock = responses::mount_sse_once(&server, sse_completed("resp1")).await; + + let model_provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + + let codex_home = TempDir::new().unwrap(); + let skill_dir = codex_home.path().join("skills/demo"); + std::fs::create_dir_all(&skill_dir).expect("create skill dir"); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: demo\ndescription: build charts\n---\n\n# body\n", + ) + .expect("write skill"); + + let mut config = load_default_config_for_test(&codex_home); + config.model_provider = model_provider; + config.features.enable(Feature::Skills); + config.cwd = codex_home.path().to_path_buf(); + + let conversation_manager = + ConversationManager::with_auth(CodexAuth::from_api_key("Test API Key")); + let codex = conversation_manager + .new_conversation(config) + .await + .expect("create new conversation") + .conversation; + + codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "hello".into(), + }], + }) + .await + .unwrap(); + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let request = resp_mock.single_request(); + let request_body = request.body_json(); + + assert_message_role(&request_body["input"][0], "user"); + let instructions_text = request_body["input"][0]["content"][0]["text"] + .as_str() + .expect("instructions text"); + assert!( + instructions_text.contains("## Skills"), + "expected skills section present" + ); + assert!( + instructions_text.contains("demo: build charts"), + "expected skill summary" + ); + let expected_path = normalize_path(skill_dir.join("SKILL.md")).unwrap(); + let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); + assert!( + instructions_text.contains(&expected_path_str), + "expected path {expected_path_str} in instructions" + ); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn includes_configured_effort_in_request() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); From 1a9a11d16852eb4f7b6782e3d0af9f0f91987220 Mon Sep 17 00:00:00 2001 From: Gav Verma Date: Mon, 1 Dec 2025 19:41:20 -0800 Subject: [PATCH 24/24] Remove unused import --- codex-rs/cli/src/main.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index d10cf937a9..2b066197af 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -35,7 +35,6 @@ use crate::mcp_cmd::McpCli; use codex_core::config::Config; use codex_core::config::ConfigOverrides; -use codex_core::features::Feature; use codex_core::features::is_known_feature_key; /// Codex CLI