From 4897efcced06c6084132f30c5c6b0b13ef0630f7 Mon Sep 17 00:00:00 2001 From: xl-openai Date: Wed, 17 Dec 2025 01:35:49 -0800 Subject: [PATCH] Add public skills + improve repo skill discovery and error UX (#8098) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Adds SkillScope::Public end-to-end (core + protocol) and loads skills from the public cache directory 2. Improves repo skill discovery by searching upward for the nearest .codex/skills within a git repo 3. Deduplicates skills by name with deterministic ordering to avoid duplicates across sources 4. Fixes garbled “Skill errors” overlay rendering by preventing pending history lines from being injected during the modal 5. Updates the project docs “Skills” intro wording to avoid hardcoded paths --- .../app-server-protocol/src/protocol/v2.rs | 2 + codex-rs/core/src/codex.rs | 35 +- codex-rs/core/src/project_doc.rs | 4 +- codex-rs/core/src/rollout/policy.rs | 3 +- codex-rs/core/src/skills/loader.rs | 387 +++++++++++++++++- codex-rs/core/src/skills/manager.rs | 48 ++- codex-rs/core/src/skills/mod.rs | 1 + codex-rs/core/src/skills/public.rs | 368 +++++++++++++++++ codex-rs/core/src/skills/render.rs | 9 +- codex-rs/core/tests/suite/skills.rs | 77 +++- codex-rs/docs/protocol_v1.md | 3 +- .../src/event_processor_with_human_output.rs | 1 + codex-rs/mcp-server/src/codex_tool_runner.rs | 1 + codex-rs/protocol/src/protocol.rs | 8 + codex-rs/tui/src/chatwidget.rs | 11 +- codex-rs/tui/src/skill_error_prompt.rs | 69 +++- codex-rs/tui/src/tui.rs | 8 + codex-rs/tui2/src/chatwidget.rs | 11 +- codex-rs/tui2/src/skill_error_prompt.rs | 69 +++- codex-rs/tui2/src/tui.rs | 8 + 20 files changed, 1050 insertions(+), 73 deletions(-) create mode 100644 codex-rs/core/src/skills/public.rs diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 2acea04e9..b2d546656 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -987,6 +987,7 @@ pub struct SkillsListResponse { pub enum SkillScope { User, Repo, + Public, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -1032,6 +1033,7 @@ impl From for SkillScope { match value { CoreSkillScope::User => Self::User, CoreSkillScope::Repo => Self::Repo, + CoreSkillScope::Public => Self::Public, } } } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index bcfe8e811..6322c471f 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -729,6 +729,28 @@ impl Session { // record_initial_history can emit events. We record only after the SessionConfiguredEvent is emitted. sess.record_initial_history(initial_history).await; + if sess.enabled(Feature::Skills) { + let mut rx = sess + .services + .skills_manager + .subscribe_skills_update_notifications(); + let sess = Arc::clone(&sess); + tokio::spawn(async move { + loop { + match rx.recv().await { + Ok(()) => { + let turn_context = + sess.new_turn(SessionSettingsUpdate::default()).await; + sess.send_event(turn_context.as_ref(), EventMsg::SkillsUpdateAvailable) + .await; + } + Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => continue, + Err(tokio::sync::broadcast::error::RecvError::Closed) => break, + } + } + }); + } + Ok(sess) } @@ -1584,8 +1606,8 @@ async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiv Op::ListCustomPrompts => { handlers::list_custom_prompts(&sess, sub.id.clone()).await; } - Op::ListSkills { cwds } => { - handlers::list_skills(&sess, sub.id.clone(), cwds).await; + Op::ListSkills { cwds, force_reload } => { + handlers::list_skills(&sess, sub.id.clone(), cwds, force_reload).await; } Op::Undo => { handlers::undo(&sess, sub.id.clone()).await; @@ -1885,7 +1907,12 @@ mod handlers { sess.send_event_raw(event).await; } - pub async fn list_skills(sess: &Session, sub_id: String, cwds: Vec) { + pub async fn list_skills( + sess: &Session, + sub_id: String, + cwds: Vec, + force_reload: bool, + ) { let cwds = if cwds.is_empty() { let state = sess.state.lock().await; vec![state.session_configuration.cwd.clone()] @@ -1896,7 +1923,7 @@ mod handlers { let skills_manager = &sess.services.skills_manager; cwds.into_iter() .map(|cwd| { - let outcome = skills_manager.skills_for_cwd(&cwd); + let outcome = skills_manager.skills_for_cwd_with_options(&cwd, force_reload); let errors = super::errors_to_info(&outcome.errors); let skills = super::skills_to_info(&outcome.skills); SkillsListEntry { diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index cd0552011..208b03556 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -522,7 +522,7 @@ mod tests { let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); let usage_rules = "- Discovery: Available skills are listed in project docs and may also appear in a runtime \"## Skills\" section (name + description + file path). These are the sources of truth; skill bodies live on disk at the listed paths.\n- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned.\n- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback.\n- How to use a skill (progressive disclosure):\n 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow.\n 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything.\n 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks.\n 4) If `assets/` or templates exist, reuse them instead of recreating from scratch.\n- Description as trigger: The YAML `description` in `SKILL.md` is the primary trigger signal; rely on it to decide applicability. If unsure, ask a brief clarification before proceeding.\n- Coordination and sequencing:\n - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them.\n - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why.\n- Context hygiene:\n - Keep context small: summarize long sections instead of pasting them; only load extra files when needed.\n - Avoid deeply nested references; prefer one-hop files explicitly linked from `SKILL.md`.\n - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice.\n- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."; 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})\n{usage_rules}" + "base doc\n\n## Skills\nThese skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.\n- pdf-processing: extract from pdfs (file: {expected_path_str})\n{usage_rules}" ); assert_eq!(res, expected); } @@ -546,7 +546,7 @@ mod tests { let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); let usage_rules = "- Discovery: Available skills are listed in project docs and may also appear in a runtime \"## Skills\" section (name + description + file path). These are the sources of truth; skill bodies live on disk at the listed paths.\n- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned.\n- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback.\n- How to use a skill (progressive disclosure):\n 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow.\n 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything.\n 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks.\n 4) If `assets/` or templates exist, reuse them instead of recreating from scratch.\n- Description as trigger: The YAML `description` in `SKILL.md` is the primary trigger signal; rely on it to decide applicability. If unsure, ask a brief clarification before proceeding.\n- Coordination and sequencing:\n - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them.\n - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why.\n- Context hygiene:\n - Keep context small: summarize long sections instead of pasting them; only load extra files when needed.\n - Avoid deeply nested references; prefer one-hop files explicitly linked from `SKILL.md`.\n - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice.\n- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."; 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})\n{usage_rules}" + "## Skills\nThese skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.\n- linting: run clippy (file: {expected_path_str})\n{usage_rules}" ); assert_eq!(res, expected); } diff --git a/codex-rs/core/src/rollout/policy.rs b/codex-rs/core/src/rollout/policy.rs index 2980e768c..07c8af114 100644 --- a/codex-rs/core/src/rollout/policy.rs +++ b/codex-rs/core/src/rollout/policy.rs @@ -88,6 +88,7 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool { | EventMsg::ItemCompleted(_) | EventMsg::AgentMessageContentDelta(_) | EventMsg::ReasoningContentDelta(_) - | EventMsg::ReasoningRawContentDelta(_) => false, + | EventMsg::ReasoningRawContentDelta(_) + | EventMsg::SkillsUpdateAvailable => false, } } diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 54b859caa..2596f3d76 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -3,9 +3,11 @@ use crate::git_info::resolve_root_git_project_for_trust; use crate::skills::model::SkillError; use crate::skills::model::SkillLoadOutcome; use crate::skills::model::SkillMetadata; +use crate::skills::public::public_cache_root_dir; use codex_protocol::protocol::SkillScope; use dunce::canonicalize as normalize_path; use serde::Deserialize; +use std::collections::HashSet; use std::collections::VecDeque; use std::error::Error; use std::fmt; @@ -71,6 +73,11 @@ where discover_skills_under_root(&root.path, root.scope, &mut outcome); } + let mut seen: HashSet = HashSet::new(); + outcome + .skills + .retain(|skill| seen.insert(skill.name.clone())); + outcome .skills .sort_by(|a, b| a.name.cmp(&b.name).then_with(|| a.path.cmp(&b.path))); @@ -85,22 +92,57 @@ pub(crate) fn user_skills_root(codex_home: &Path) -> SkillRoot { } } +pub(crate) fn public_skills_root(codex_home: &Path) -> SkillRoot { + SkillRoot { + path: public_cache_root_dir(codex_home), + scope: SkillScope::Public, + } +} + pub(crate) fn repo_skills_root(cwd: &Path) -> Option { - resolve_root_git_project_for_trust(cwd).map(|repo_root| SkillRoot { - path: repo_root - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - scope: SkillScope::Repo, + let base = if cwd.is_dir() { cwd } else { cwd.parent()? }; + let base = normalize_path(base).unwrap_or_else(|_| base.to_path_buf()); + + let repo_root = + resolve_root_git_project_for_trust(&base).map(|root| normalize_path(&root).unwrap_or(root)); + + let scope = SkillScope::Repo; + if let Some(repo_root) = repo_root.as_deref() { + for dir in base.ancestors() { + let skills_root = dir.join(REPO_ROOT_CONFIG_DIR_NAME).join(SKILLS_DIR_NAME); + if skills_root.is_dir() { + return Some(SkillRoot { + path: skills_root, + scope, + }); + } + + if dir == repo_root { + break; + } + } + return None; + } + + let skills_root = base.join(REPO_ROOT_CONFIG_DIR_NAME).join(SKILLS_DIR_NAME); + skills_root.is_dir().then_some(SkillRoot { + path: skills_root, + scope, }) } fn skill_roots(config: &Config) -> Vec { - let mut roots = vec![user_skills_root(&config.codex_home)]; + let mut roots = Vec::new(); if let Some(repo_root) = repo_skills_root(&config.cwd) { roots.push(repo_root); } + // Load order matters: we dedupe by name, keeping the first occurrence. + // This makes repo/user skills win over public skills. + roots.push(user_skills_root(&config.codex_home)); + roots.push(public_skills_root(&config.codex_home)); + roots } @@ -149,11 +191,17 @@ fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut Skil if file_type.is_file() && file_name == SKILLS_FILENAME { match parse_skill_file(&path, scope) { - Ok(skill) => outcome.skills.push(skill), - Err(err) => outcome.errors.push(SkillError { - path, - message: err.to_string(), - }), + Ok(skill) => { + outcome.skills.push(skill); + } + Err(err) => { + if scope != SkillScope::Public { + outcome.errors.push(SkillError { + path, + message: err.to_string(), + }); + } + } } } } @@ -233,6 +281,7 @@ mod tests { use super::*; use crate::config::ConfigOverrides; use crate::config::ConfigToml; + use codex_protocol::protocol::SkillScope; use pretty_assertions::assert_eq; use std::path::Path; use std::process::Command; @@ -251,11 +300,11 @@ mod tests { } fn write_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { - write_skill_at(codex_home.path(), dir, name, description) + write_skill_at(&codex_home.path().join("skills"), dir, name, description) } fn write_skill_at(root: &Path, dir: &str, name: &str, description: &str) -> PathBuf { - let skill_dir = root.join(format!("skills/{dir}")); + let skill_dir = root.join(dir); fs::create_dir_all(&skill_dir).unwrap(); let indented_description = description.replace('\n', "\n "); let content = format!( @@ -375,4 +424,316 @@ mod tests { assert_eq!(skill.name, "repo-skill"); assert!(skill.path.starts_with(&repo_root)); } + + #[test] + fn loads_skills_from_nearest_codex_dir_under_repo_root() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + + let status = Command::new("git") + .arg("init") + .current_dir(repo_dir.path()) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + let nested_dir = repo_dir.path().join("nested/inner"); + fs::create_dir_all(&nested_dir).unwrap(); + + write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "root", + "root-skill", + "from root", + ); + write_skill_at( + &repo_dir + .path() + .join("nested") + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "nested", + "nested-skill", + "from nested", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = nested_dir; + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "nested-skill"); + } + + #[test] + fn loads_skills_from_codex_dir_when_not_git_repo() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let work_dir = tempfile::tempdir().expect("tempdir"); + + write_skill_at( + &work_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "local", + "local-skill", + "from cwd", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = work_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "local-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + } + + #[test] + fn deduplicates_by_name_preferring_repo_over_user() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + + let status = Command::new("git") + .arg("init") + .current_dir(repo_dir.path()) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + write_skill(&codex_home, "user", "dupe-skill", "from user"); + write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "repo", + "dupe-skill", + "from repo", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = repo_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "dupe-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + } + + #[test] + fn repo_skills_search_does_not_escape_repo_root() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let outer_dir = tempfile::tempdir().expect("tempdir"); + let repo_dir = outer_dir.path().join("repo"); + fs::create_dir_all(&repo_dir).unwrap(); + + write_skill_at( + &outer_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "outer", + "outer-skill", + "from outer", + ); + + let status = Command::new("git") + .arg("init") + .current_dir(&repo_dir) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + let mut cfg = make_config(&codex_home); + cfg.cwd = repo_dir; + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 0); + } + + #[test] + fn loads_skills_when_cwd_is_file_in_repo() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + + let status = Command::new("git") + .arg("init") + .current_dir(repo_dir.path()) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "repo", + "repo-skill", + "from repo", + ); + let file_path = repo_dir.path().join("some-file.txt"); + fs::write(&file_path, "contents").unwrap(); + + let mut cfg = make_config(&codex_home); + cfg.cwd = file_path; + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "repo-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + } + + #[test] + fn non_git_repo_skills_search_does_not_walk_parents() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let outer_dir = tempfile::tempdir().expect("tempdir"); + let nested_dir = outer_dir.path().join("nested/inner"); + fs::create_dir_all(&nested_dir).unwrap(); + + write_skill_at( + &outer_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "outer", + "outer-skill", + "from outer", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = nested_dir; + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 0); + } + + #[test] + fn loads_skills_from_public_cache_when_present() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let work_dir = tempfile::tempdir().expect("tempdir"); + + write_skill_at( + &codex_home.path().join("skills").join(".public"), + "public", + "public-skill", + "from public", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = work_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "public-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Public); + } + + #[test] + fn deduplicates_by_name_preferring_user_over_public() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let work_dir = tempfile::tempdir().expect("tempdir"); + + write_skill(&codex_home, "user", "dupe-skill", "from user"); + write_skill_at( + &codex_home.path().join("skills").join(".public"), + "public", + "dupe-skill", + "from public", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = work_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "dupe-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::User); + } + + #[test] + fn deduplicates_by_name_preferring_repo_over_public() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + + let status = Command::new("git") + .arg("init") + .current_dir(repo_dir.path()) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "repo", + "dupe-skill", + "from repo", + ); + write_skill_at( + &codex_home.path().join("skills").join(".public"), + "public", + "dupe-skill", + "from public", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = repo_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "dupe-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + } } diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index a031d66a9..f1c9f36a3 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -2,38 +2,82 @@ use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; use std::sync::RwLock; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use crate::skills::SkillLoadOutcome; use crate::skills::loader::load_skills_from_roots; +use crate::skills::loader::public_skills_root; use crate::skills::loader::repo_skills_root; use crate::skills::loader::user_skills_root; +use crate::skills::public::refresh_public_skills; +use tokio::sync::broadcast; pub struct SkillsManager { codex_home: PathBuf, cache_by_cwd: RwLock>, + attempted_public_refresh: AtomicBool, + skills_update_tx: broadcast::Sender<()>, } impl SkillsManager { pub fn new(codex_home: PathBuf) -> Self { + let (skills_update_tx, _skills_update_rx) = broadcast::channel(1); Self { codex_home, cache_by_cwd: RwLock::new(HashMap::new()), + attempted_public_refresh: AtomicBool::new(false), + skills_update_tx, } } + pub(crate) fn subscribe_skills_update_notifications(&self) -> broadcast::Receiver<()> { + self.skills_update_tx.subscribe() + } + pub fn skills_for_cwd(&self, cwd: &Path) -> SkillLoadOutcome { + self.skills_for_cwd_with_options(cwd, false) + } + + pub(crate) fn skills_for_cwd_with_options( + &self, + cwd: &Path, + force_reload: bool, + ) -> SkillLoadOutcome { + // Best-effort refresh: attempt at most once per manager instance. + if self + .attempted_public_refresh + .compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) + .is_ok() + { + let codex_home = self.codex_home.clone(); + let skills_update_tx = self.skills_update_tx.clone(); + std::thread::spawn(move || match refresh_public_skills(&codex_home) { + Ok(outcome) => { + if outcome.updated() { + let _ = skills_update_tx.send(()); + } + } + Err(err) => { + tracing::error!("failed to refresh public skills: {err}"); + } + }); + } + let cached = match self.cache_by_cwd.read() { Ok(cache) => cache.get(cwd).cloned(), Err(err) => err.into_inner().get(cwd).cloned(), }; - if let Some(outcome) = cached { + if !force_reload && let Some(outcome) = cached { return outcome; } - let mut roots = vec![user_skills_root(&self.codex_home)]; + let mut roots = Vec::new(); if let Some(repo_root) = repo_skills_root(cwd) { roots.push(repo_root); } + roots.push(user_skills_root(&self.codex_home)); + roots.push(public_skills_root(&self.codex_home)); let outcome = load_skills_from_roots(roots); match self.cache_by_cwd.write() { Ok(mut cache) => { diff --git a/codex-rs/core/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs index 9d15f0333..4872b6a2f 100644 --- a/codex-rs/core/src/skills/mod.rs +++ b/codex-rs/core/src/skills/mod.rs @@ -2,6 +2,7 @@ pub mod injection; pub mod loader; pub mod manager; pub mod model; +pub mod public; pub mod render; pub(crate) use injection::SkillInjections; diff --git a/codex-rs/core/src/skills/public.rs b/codex-rs/core/src/skills/public.rs new file mode 100644 index 000000000..711dad979 --- /dev/null +++ b/codex-rs/core/src/skills/public.rs @@ -0,0 +1,368 @@ +use std::fs; +use std::path::Path; +use std::path::PathBuf; +use std::process::ExitStatus; +use std::time::SystemTime; +use std::time::UNIX_EPOCH; + +use thiserror::Error; + +const PUBLIC_SKILLS_REPO_URL: &str = "https://github.com/openai/skills.git"; +const PUBLIC_SKILLS_DIR_NAME: &str = ".public"; +const SKILLS_DIR_NAME: &str = "skills"; + +pub(crate) fn public_cache_root_dir(codex_home: &Path) -> PathBuf { + codex_home + .join(SKILLS_DIR_NAME) + .join(PUBLIC_SKILLS_DIR_NAME) +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum PublicSkillsRefreshOutcome { + Skipped, + Updated, +} + +impl PublicSkillsRefreshOutcome { + pub(crate) fn updated(self) -> bool { + matches!(self, Self::Updated) + } +} + +pub(crate) fn refresh_public_skills( + codex_home: &Path, +) -> Result { + // Keep tests deterministic and offline-safe. Tests that want to exercise the + // refresh behavior should call `refresh_public_skills_from_repo_url`. + if cfg!(test) { + return Ok(PublicSkillsRefreshOutcome::Skipped); + } + refresh_public_skills_inner(codex_home, PUBLIC_SKILLS_REPO_URL) +} + +#[cfg(test)] +pub(crate) fn refresh_public_skills_from_repo_url( + codex_home: &Path, + repo_url: &str, +) -> Result { + refresh_public_skills_inner(codex_home, repo_url) +} + +fn refresh_public_skills_inner( + codex_home: &Path, + repo_url: &str, +) -> Result { + // Best-effort refresh: clone the repo to a temp dir, stage its `skills/`, then atomically swap + // the staged directory into the public cache. + let skills_root_dir = codex_home.join(SKILLS_DIR_NAME); + fs::create_dir_all(&skills_root_dir) + .map_err(|source| PublicSkillsError::io("create skills root dir", source))?; + + let dest_public = public_cache_root_dir(codex_home); + + let tmp_dir = skills_root_dir.join(format!(".public-tmp-{}", rand_suffix())); + if tmp_dir.exists() { + fs::remove_dir_all(&tmp_dir).map_err(|source| { + PublicSkillsError::io("remove existing public skills tmp dir", source) + })?; + } + fs::create_dir_all(&tmp_dir) + .map_err(|source| PublicSkillsError::io("create public skills tmp dir", source))?; + + let checkout_dir = tmp_dir.join("checkout"); + clone_repo(repo_url, &checkout_dir)?; + + let src_skills = checkout_dir.join(SKILLS_DIR_NAME); + let src_skills_metadata = fs::symlink_metadata(&src_skills) + .map_err(|source| PublicSkillsError::io("read skills dir metadata", source))?; + let src_skills_type = src_skills_metadata.file_type(); + if src_skills_type.is_symlink() || !src_skills_type.is_dir() { + return Err(PublicSkillsError::RepoMissingSkillsDir { + skills_dir_name: SKILLS_DIR_NAME, + }); + } + + let staged_public = tmp_dir.join(PUBLIC_SKILLS_DIR_NAME); + stage_skills_dir(&src_skills, &staged_public)?; + + atomic_swap_dir(&staged_public, &dest_public, &skills_root_dir)?; + + fs::remove_dir_all(&tmp_dir) + .map_err(|source| PublicSkillsError::io("remove public skills tmp dir", source))?; + Ok(PublicSkillsRefreshOutcome::Updated) +} + +fn stage_skills_dir(src: &Path, staged: &Path) -> Result<(), PublicSkillsError> { + fs::rename(src, staged).map_err(|source| PublicSkillsError::io("stage skills dir", source))?; + + prune_symlinks_and_special_files(staged)?; + Ok(()) +} + +fn prune_symlinks_and_special_files(root: &Path) -> Result<(), PublicSkillsError> { + let mut stack: Vec = vec![root.to_path_buf()]; + while let Some(dir) = stack.pop() { + for entry in fs::read_dir(&dir) + .map_err(|source| PublicSkillsError::io("read staged skills dir", source))? + { + let entry = entry + .map_err(|source| PublicSkillsError::io("read staged skills dir entry", source))?; + let file_type = entry + .file_type() + .map_err(|source| PublicSkillsError::io("read staged skills entry type", source))?; + let path = entry.path(); + + if file_type.is_symlink() { + fs::remove_file(&path).map_err(|source| { + PublicSkillsError::io("remove symlink from staged skills", source) + })?; + continue; + } + + if file_type.is_dir() { + stack.push(path); + continue; + } + + if file_type.is_file() { + continue; + } + + fs::remove_file(&path).map_err(|source| { + PublicSkillsError::io("remove special file from staged skills", source) + })?; + } + } + + Ok(()) +} + +fn clone_repo(repo_url: &str, checkout_dir: &Path) -> Result<(), PublicSkillsError> { + let out = std::process::Command::new("git") + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_ASKPASS", "true") + .arg("clone") + .arg("--depth") + .arg("1") + .arg(repo_url) + .arg(checkout_dir) + .stdin(std::process::Stdio::null()) + .output() + .map_err(|source| PublicSkillsError::io("spawn `git clone`", source))?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr); + let stderr = stderr.trim(); + return if stderr.is_empty() { + Err(PublicSkillsError::GitCloneFailed { status: out.status }) + } else { + Err(PublicSkillsError::GitCloneFailedWithStderr { + status: out.status, + stderr: stderr.to_owned(), + }) + }; + } + Ok(()) +} + +fn atomic_swap_dir(staged: &Path, dest: &Path, parent: &Path) -> Result<(), PublicSkillsError> { + if let Some(dest_parent) = dest.parent() { + fs::create_dir_all(dest_parent) + .map_err(|source| PublicSkillsError::io("create public skills dest parent", source))?; + } + + let backup_base = dest + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("skills"); + let backup = parent.join(format!("{backup_base}.old-{}", rand_suffix())); + if backup.exists() { + fs::remove_dir_all(&backup) + .map_err(|source| PublicSkillsError::io("remove old public skills backup", source))?; + } + + if dest.exists() { + fs::rename(dest, &backup) + .map_err(|source| PublicSkillsError::io("rename public skills to backup", source))?; + } + + if let Err(err) = fs::rename(staged, dest) { + if backup.exists() { + let _ = fs::rename(&backup, dest); + } + return Err(PublicSkillsError::io( + "rename staged public skills into place", + err, + )); + } + + if backup.exists() { + fs::remove_dir_all(&backup) + .map_err(|source| PublicSkillsError::io("remove public skills backup", source))?; + } + + Ok(()) +} + +fn rand_suffix() -> String { + let pid = std::process::id(); + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_nanos(); + format!("{pid:x}-{nanos:x}") +} + +#[derive(Debug, Error)] +pub(crate) enum PublicSkillsError { + #[error("io error while {action}: {source}")] + Io { + action: &'static str, + #[source] + source: std::io::Error, + }, + + #[error("repo did not contain a `{skills_dir_name}` directory")] + RepoMissingSkillsDir { skills_dir_name: &'static str }, + + #[error("`git clone` failed with status {status}")] + GitCloneFailed { status: ExitStatus }, + + #[error("`git clone` failed with status {status}: {stderr}")] + GitCloneFailedWithStderr { status: ExitStatus, stderr: String }, +} + +impl PublicSkillsError { + fn io(action: &'static str, source: std::io::Error) -> Self { + Self::Io { action, source } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use tempfile::TempDir; + + fn write_public_skill(repo_dir: &TempDir, name: &str, description: &str) { + let skills_dir = repo_dir.path().join("skills").join(name); + fs::create_dir_all(&skills_dir).unwrap(); + let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + fs::write(skills_dir.join("SKILL.md"), content).unwrap(); + } + + fn git(repo_dir: &TempDir, args: &[&str]) { + let status = std::process::Command::new("git") + .args([ + "-c", + "user.name=codex-test", + "-c", + "user.email=codex-test@example.com", + ]) + .args(args) + .current_dir(repo_dir.path()) + .status() + .unwrap(); + assert!(status.success(), "git command failed: {args:?}"); + } + + #[tokio::test] + async fn refresh_copies_skills_subdir_into_public_cache() { + let codex_home = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + git(&repo_dir, &["init"]); + write_public_skill(&repo_dir, "demo", "from repo"); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "init"]); + + refresh_public_skills_from_repo_url(codex_home.path(), repo_dir.path().to_str().unwrap()) + .unwrap(); + + let path = public_cache_root_dir(codex_home.path()) + .join("demo") + .join("SKILL.md"); + let contents = fs::read_to_string(path).unwrap(); + assert!(contents.contains("name: demo")); + assert!(contents.contains("description: from repo")); + } + + #[tokio::test] + async fn refresh_overwrites_existing_public_cache() { + let codex_home = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + git(&repo_dir, &["init"]); + write_public_skill(&repo_dir, "demo", "v1"); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "v1"]); + + refresh_public_skills_from_repo_url(codex_home.path(), repo_dir.path().to_str().unwrap()) + .unwrap(); + + write_public_skill(&repo_dir, "demo", "v2"); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "v2"]); + + refresh_public_skills_from_repo_url(codex_home.path(), repo_dir.path().to_str().unwrap()) + .unwrap(); + + let path = public_cache_root_dir(codex_home.path()) + .join("demo") + .join("SKILL.md"); + let contents = fs::read_to_string(path).unwrap(); + assert_eq!(contents.matches("description:").count(), 1); + assert!(contents.contains("description: v2")); + } + + #[cfg(unix)] + #[tokio::test] + async fn refresh_prunes_symlinks_inside_skills_dir() { + use std::os::unix::fs::symlink; + + let codex_home = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + git(&repo_dir, &["init"]); + write_public_skill(&repo_dir, "demo", "from repo"); + + let demo_dir = repo_dir.path().join("skills").join("demo"); + symlink("SKILL.md", demo_dir.join("link-to-skill")).unwrap(); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "init"]); + + refresh_public_skills_from_repo_url(codex_home.path(), repo_dir.path().to_str().unwrap()) + .unwrap(); + + assert!( + !public_cache_root_dir(codex_home.path()) + .join("demo") + .join("link-to-skill") + .exists() + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn refresh_rejects_symlinked_skills_dir() { + use std::os::unix::fs::symlink; + + let codex_home = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + git(&repo_dir, &["init"]); + + let skills_target = repo_dir.path().join("skills-target"); + fs::create_dir_all(skills_target.join("demo")).unwrap(); + fs::write( + skills_target.join("demo").join("SKILL.md"), + "---\nname: demo\ndescription: from repo\n---\n", + ) + .unwrap(); + symlink("skills-target", repo_dir.path().join("skills")).unwrap(); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "init"]); + + let err = refresh_public_skills_from_repo_url( + codex_home.path(), + repo_dir.path().to_str().unwrap(), + ) + .unwrap_err(); + assert!(err.to_string().contains("repo did not contain")); + } +} diff --git a/codex-rs/core/src/skills/render.rs b/codex-rs/core/src/skills/render.rs index b66456545..f767849b2 100644 --- a/codex-rs/core/src/skills/render.rs +++ b/codex-rs/core/src/skills/render.rs @@ -7,14 +7,13 @@ pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { 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()); + lines.push("These skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.".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 - )); + let name = skill.name.as_str(); + let description = skill.description.as_str(); + lines.push(format!("- {name}: {description} (file: {path_str})")); } lines.push( diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 25a68be76..b23b3fd5b 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -27,6 +27,20 @@ fn write_skill(home: &Path, name: &str, description: &str, body: &str) -> std::p path } +fn write_public_skill( + home: &Path, + name: &str, + description: &str, + body: &str, +) -> std::path::PathBuf { + let skill_dir = home.join("skills").join(".public").join(name); + fs::create_dir_all(&skill_dir).unwrap(); + let contents = format!("---\nname: {name}\ndescription: {description}\n---\n\n{body}\n"); + let path = skill_dir.join("SKILL.md"); + fs::write(&path, contents).unwrap(); + path +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn user_turn_includes_skill_instructions() -> Result<()> { skip_if_no_network!(Ok(())); @@ -115,7 +129,10 @@ async fn skill_load_errors_surface_in_session_configured() -> Result<()> { let test = builder.build(&server).await?; test.codex - .submit(Op::ListSkills { cwds: Vec::new() }) + .submit(Op::ListSkills { + cwds: Vec::new(), + force_reload: false, + }) .await?; let response = core_test_support::wait_for_event_match(test.codex.as_ref(), |event| match event { @@ -133,8 +150,13 @@ async fn skill_load_errors_surface_in_session_configured() -> Result<()> { .unwrap_or_default(); assert!( - skills.is_empty(), - "expected no skills loaded, got {skills:?}" + skills.iter().all(|skill| { + !skill + .path + .to_string_lossy() + .ends_with("skills/broken/SKILL.md") + }), + "expected broken skill not loaded, got {skills:?}" ); assert_eq!(errors.len(), 1, "expected one load error"); let error_path = errors[0].path.to_string_lossy(); @@ -145,3 +167,52 @@ async fn skill_load_errors_surface_in_session_configured() -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn list_skills_includes_public_cache_entries() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mut builder = test_codex() + .with_config(|cfg| { + cfg.features.enable(Feature::Skills); + }) + .with_pre_build_hook(|home| { + write_public_skill(home, "public-demo", "public skill", "public body"); + }); + let test = builder.build(&server).await?; + + test.codex + .submit(Op::ListSkills { + cwds: Vec::new(), + force_reload: true, + }) + .await?; + let response = + core_test_support::wait_for_event_match(test.codex.as_ref(), |event| match event { + codex_core::protocol::EventMsg::ListSkillsResponse(response) => Some(response.clone()), + _ => None, + }) + .await; + + let cwd = test.cwd_path(); + let (skills, _errors) = response + .skills + .iter() + .find(|entry| entry.cwd.as_path() == cwd) + .map(|entry| (entry.skills.clone(), entry.errors.clone())) + .unwrap_or_default(); + + let skill = skills + .iter() + .find(|skill| skill.name == "public-demo") + .expect("expected public skill to be present"); + assert_eq!(skill.scope, codex_protocol::protocol::SkillScope::Public); + let path_str = skill.path.to_string_lossy().replace('\\', "/"); + assert!( + path_str.ends_with("/skills/.public/public-demo/SKILL.md"), + "unexpected skill path: {path_str}" + ); + + Ok(()) +} diff --git a/codex-rs/docs/protocol_v1.md b/codex-rs/docs/protocol_v1.md index 075377f61..682a38bfc 100644 --- a/codex-rs/docs/protocol_v1.md +++ b/codex-rs/docs/protocol_v1.md @@ -68,7 +68,7 @@ For complete documentation of the `Op` and `EventMsg` variants, refer to [protoc - `Op::UserInput` – Any input from the user to kick off a `Task` - `Op::Interrupt` – Interrupts a running task - `Op::ExecApproval` – Approve or deny code execution - - `Op::ListSkills` – Request skills for one or more cwd values + - `Op::ListSkills` – Request skills for one or more cwd values (optionally `force_reload`) - `EventMsg` - `EventMsg::AgentMessage` – Messages from the `Model` - `EventMsg::ExecApprovalRequest` – Request approval from user to execute a command @@ -77,6 +77,7 @@ For complete documentation of the `Op` and `EventMsg` variants, refer to [protoc - `EventMsg::Warning` – A non-fatal warning that the client should surface to the user - `EventMsg::TurnComplete` – Contains a `response_id` bookmark for last `response_id` executed by the task. This can be used to continue the task at a later point in time, perhaps with additional user input. - `EventMsg::ListSkillsResponse` – Response payload with per-cwd skill entries (`cwd`, `skills`, `errors`) + - `EventMsg::SkillsUpdateAvailable` – Notification that skills may have changed and clients may want to reload The `response_id` returned from each task matches the OpenAI `response_id` stored in the API's `/responses` endpoint. It can be stored and used in future `Sessions` to resume threads of work. diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index a833426dc..a43718d56 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -585,6 +585,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { | EventMsg::AgentMessageContentDelta(_) | EventMsg::ReasoningContentDelta(_) | EventMsg::ReasoningRawContentDelta(_) + | EventMsg::SkillsUpdateAvailable | EventMsg::UndoCompleted(_) | EventMsg::UndoStarted(_) => {} } diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 73e56a605..39ae7486e 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -304,6 +304,7 @@ async fn run_codex_tool_session_inner( | EventMsg::AgentMessageContentDelta(_) | EventMsg::ReasoningContentDelta(_) | EventMsg::ReasoningRawContentDelta(_) + | EventMsg::SkillsUpdateAvailable | EventMsg::UndoStarted(_) | EventMsg::UndoCompleted(_) | EventMsg::ExitedReviewMode(_) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 201aba07e..2fdb022b8 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -193,6 +193,10 @@ pub enum Op { /// When empty, the session default working directory is used. #[serde(default, skip_serializing_if = "Vec::is_empty")] cwds: Vec, + + /// When true, recompute skills even if a cached result exists. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + force_reload: bool, }, /// Request the agent to summarize the current conversation context. @@ -609,6 +613,9 @@ pub enum EventMsg { /// List of skills available to the agent. ListSkillsResponse(ListSkillsResponseEvent), + /// Notification that skill data may have been updated and clients may want to reload. + SkillsUpdateAvailable, + PlanUpdate(UpdatePlanArgs), TurnAborted(TurnAbortedEvent), @@ -1683,6 +1690,7 @@ pub struct ListSkillsResponseEvent { pub enum SkillScope { User, Repo, + Public, } #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index ca9e0525d..464d3441f 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -410,7 +410,10 @@ impl ChatWidget { } // Ask codex-core to enumerate custom prompts for this session. self.submit_op(Op::ListCustomPrompts); - self.submit_op(Op::ListSkills { cwds: Vec::new() }); + self.submit_op(Op::ListSkills { + cwds: Vec::new(), + force_reload: false, + }); if let Some(user_message) = self.initial_user_message.take() { self.submit_user_message(user_message); } @@ -1886,6 +1889,12 @@ impl ChatWidget { EventMsg::McpListToolsResponse(ev) => self.on_list_mcp_tools(ev), EventMsg::ListCustomPromptsResponse(ev) => self.on_list_custom_prompts(ev), EventMsg::ListSkillsResponse(ev) => self.on_list_skills(ev), + EventMsg::SkillsUpdateAvailable => { + self.submit_op(Op::ListSkills { + cwds: Vec::new(), + force_reload: true, + }); + } EventMsg::ShutdownComplete => self.on_shutdown_complete(), EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) => self.on_turn_diff(unified_diff), EventMsg::DeprecationNotice(ev) => self.on_deprecation_notice(ev), diff --git a/codex-rs/tui/src/skill_error_prompt.rs b/codex-rs/tui/src/skill_error_prompt.rs index 33d3b5dce..9a9f803ad 100644 --- a/codex-rs/tui/src/skill_error_prompt.rs +++ b/codex-rs/tui/src/skill_error_prompt.rs @@ -1,6 +1,8 @@ use crate::tui::FrameRequester; use crate::tui::Tui; use crate::tui::TuiEvent; +use crate::wrapping::RtOptions; +use crate::wrapping::word_wrap_line; use codex_core::skills::SkillError; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; @@ -16,7 +18,6 @@ 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)] @@ -31,16 +32,24 @@ pub(crate) async fn run_skill_error_prompt( ) -> SkillErrorPromptOutcome { struct AltScreenGuard<'a> { tui: &'a mut Tui, + stashed_history_lines: Vec>, } impl<'a> AltScreenGuard<'a> { fn enter(tui: &'a mut Tui) -> Self { let _ = tui.enter_alt_screen(); - Self { tui } + let stashed_history_lines = tui.stash_pending_history_lines(); + Self { + tui, + stashed_history_lines, + } } } impl Drop for AltScreenGuard<'_> { fn drop(&mut self) { let _ = self.tui.leave_alt_screen(); + let stashed_history_lines = std::mem::take(&mut self.stashed_history_lines); + self.tui + .restore_pending_history_lines(stashed_history_lines); } } @@ -76,28 +85,16 @@ pub(crate) async fn run_skill_error_prompt( struct SkillErrorScreen { request_frame: FrameRequester, - lines: Vec>, + errors: 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, + errors: errors.to_vec(), done: false, exit: false, } @@ -153,12 +150,44 @@ impl SkillErrorScreen { 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); + + let inner = block.inner(area); + let width = usize::from(inner.width).max(1); + + let mut base_lines: Vec> = vec![ + Line::from("Skill validation errors detected".bold()), + Line::from("Fix these SKILL.md files and restart."), + Line::from("Invalid skills are ignored until resolved."), + Line::from("Press enter or esc to continue. Ctrl+C or Ctrl+D to exit."), + Line::from(""), + ]; + + let error_start = base_lines.len(); + for error in &self.errors { + base_lines.push(Line::from(vec![ + error.path.display().to_string().dim(), + ": ".into(), + error.message.clone().red(), + ])); + } + + let error_wrap_opts = RtOptions::new(width) + .initial_indent(Line::from("- ")) + .subsequent_indent(Line::from(" ")); + + let mut lines: Vec> = Vec::new(); + for (idx, line) in base_lines.iter().enumerate() { + if idx < error_start { + lines.extend(word_wrap_line(line, width)); + } else { + lines.extend(word_wrap_line(line, error_wrap_opts.clone())); + } + } + + Paragraph::new(lines).block(block).render(area, buf); } } diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index 0770b7664..bfb4f8e03 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -329,6 +329,14 @@ impl Tui { self.frame_requester().schedule_frame(); } + pub(crate) fn stash_pending_history_lines(&mut self) -> Vec> { + std::mem::take(&mut self.pending_history_lines) + } + + pub(crate) fn restore_pending_history_lines(&mut self, lines: Vec>) { + self.pending_history_lines = lines; + } + pub fn draw( &mut self, height: u16, diff --git a/codex-rs/tui2/src/chatwidget.rs b/codex-rs/tui2/src/chatwidget.rs index 20ecb8bac..ece866b07 100644 --- a/codex-rs/tui2/src/chatwidget.rs +++ b/codex-rs/tui2/src/chatwidget.rs @@ -410,7 +410,10 @@ impl ChatWidget { } // Ask codex-core to enumerate custom prompts for this session. self.submit_op(Op::ListCustomPrompts); - self.submit_op(Op::ListSkills { cwds: Vec::new() }); + self.submit_op(Op::ListSkills { + cwds: Vec::new(), + force_reload: false, + }); if let Some(user_message) = self.initial_user_message.take() { self.submit_user_message(user_message); } @@ -1886,6 +1889,12 @@ impl ChatWidget { EventMsg::McpListToolsResponse(ev) => self.on_list_mcp_tools(ev), EventMsg::ListCustomPromptsResponse(ev) => self.on_list_custom_prompts(ev), EventMsg::ListSkillsResponse(ev) => self.on_list_skills(ev), + EventMsg::SkillsUpdateAvailable => { + self.submit_op(Op::ListSkills { + cwds: Vec::new(), + force_reload: true, + }); + } EventMsg::ShutdownComplete => self.on_shutdown_complete(), EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) => self.on_turn_diff(unified_diff), EventMsg::DeprecationNotice(ev) => self.on_deprecation_notice(ev), diff --git a/codex-rs/tui2/src/skill_error_prompt.rs b/codex-rs/tui2/src/skill_error_prompt.rs index 41aaefa4c..b97fb02d1 100644 --- a/codex-rs/tui2/src/skill_error_prompt.rs +++ b/codex-rs/tui2/src/skill_error_prompt.rs @@ -1,6 +1,8 @@ use crate::tui::FrameRequester; use crate::tui::Tui; use crate::tui::TuiEvent; +use crate::wrapping::RtOptions; +use crate::wrapping::word_wrap_line; use codex_core::skills::SkillError; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; @@ -16,7 +18,6 @@ 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)] @@ -31,16 +32,24 @@ pub(crate) async fn run_skill_error_prompt( ) -> SkillErrorPromptOutcome { struct AltScreenGuard<'a> { tui: &'a mut Tui, + stashed_history_lines: Vec>, } impl<'a> AltScreenGuard<'a> { fn enter(tui: &'a mut Tui) -> Self { let _ = tui.enter_alt_screen(); - Self { tui } + let stashed_history_lines = tui.stash_pending_history_lines(); + Self { + tui, + stashed_history_lines, + } } } impl Drop for AltScreenGuard<'_> { fn drop(&mut self) { let _ = self.tui.leave_alt_screen(); + let stashed_history_lines = std::mem::take(&mut self.stashed_history_lines); + self.tui + .restore_pending_history_lines(stashed_history_lines); } } @@ -77,28 +86,16 @@ pub(crate) async fn run_skill_error_prompt( struct SkillErrorScreen { request_frame: FrameRequester, - lines: Vec>, + errors: 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, + errors: errors.to_vec(), done: false, exit: false, } @@ -154,12 +151,44 @@ impl SkillErrorScreen { 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); + + let inner = block.inner(area); + let width = usize::from(inner.width).max(1); + + let mut base_lines: Vec> = vec![ + Line::from("Skill validation errors detected".bold()), + Line::from("Fix these SKILL.md files and restart."), + Line::from("Invalid skills are ignored until resolved."), + Line::from("Press enter or esc to continue. Ctrl+C or Ctrl+D to exit."), + Line::from(""), + ]; + + let error_start = base_lines.len(); + for error in &self.errors { + base_lines.push(Line::from(vec![ + error.path.display().to_string().dim(), + ": ".into(), + error.message.clone().red(), + ])); + } + + let error_wrap_opts = RtOptions::new(width) + .initial_indent(Line::from("- ")) + .subsequent_indent(Line::from(" ")); + + let mut lines: Vec> = Vec::new(); + for (idx, line) in base_lines.iter().enumerate() { + if idx < error_start { + lines.extend(word_wrap_line(line, width)); + } else { + lines.extend(word_wrap_line(line, error_wrap_opts.clone())); + } + } + + Paragraph::new(lines).block(block).render(area, buf); } } diff --git a/codex-rs/tui2/src/tui.rs b/codex-rs/tui2/src/tui.rs index 712c5cf55..a10c3f99b 100644 --- a/codex-rs/tui2/src/tui.rs +++ b/codex-rs/tui2/src/tui.rs @@ -335,6 +335,14 @@ impl Tui { self.frame_requester().schedule_frame(); } + pub(crate) fn stash_pending_history_lines(&mut self) -> Vec> { + std::mem::take(&mut self.pending_history_lines) + } + + pub(crate) fn restore_pending_history_lines(&mut self, lines: Vec>) { + self.pending_history_lines = lines; + } + pub fn draw( &mut self, height: u16,