From ece3915a8dc0fb4346361b55d2304bb309244ceb Mon Sep 17 00:00:00 2001 From: Xin Lin Date: Mon, 8 Dec 2025 21:52:18 -0800 Subject: [PATCH 1/3] Inject SKILL.md when it's explicitly mentioned. --- codex-rs/core/src/codex.rs | 121 +++++++++++++++++++++++-- codex-rs/core/src/event_mapping.rs | 5 +- codex-rs/core/src/project_doc.rs | 47 +++++----- codex-rs/core/src/state/session.rs | 8 +- codex-rs/core/src/user_instructions.rs | 36 ++++++++ 5 files changed, 185 insertions(+), 32 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 042ae1a37a5..5cf9fe88a74 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::collections::HashSet; use std::fmt::Debug; use std::path::PathBuf; use std::sync::Arc; @@ -55,6 +56,7 @@ use mcp_types::ReadResourceResult; use mcp_types::RequestId; use serde_json; use serde_json::Value; +use tokio::fs; use tokio::sync::Mutex; use tokio::sync::RwLock; use tokio::sync::oneshot; @@ -109,6 +111,8 @@ use crate::rollout::RolloutRecorder; use crate::rollout::RolloutRecorderParams; use crate::rollout::map_session_init_error; use crate::shell; +use crate::skills::SkillMetadata; +use crate::skills::load_skills; use crate::shell_snapshot::ShellSnapshot; use crate::state::ActiveTurn; use crate::state::SessionServices; @@ -126,6 +130,7 @@ use crate::tools::spec::ToolsConfigParams; use crate::turn_diff_tracker::TurnDiffTracker; use crate::unified_exec::UnifiedExecSessionManager; use crate::user_instructions::DeveloperInstructions; +use crate::user_instructions::SkillInstructions; use crate::user_instructions::UserInstructions; use crate::user_notification::UserNotification; use crate::util::backoff; @@ -174,7 +179,25 @@ impl Codex { let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_event, rx_event) = async_channel::unbounded(); - let user_instructions = get_user_instructions(&config).await; + let loaded_skills = if config.features.enabled(Feature::Skills) { + Some(load_skills(&config)) + } else { + None + }; + + if let Some(outcome) = &loaded_skills { + for err in &outcome.errors { + error!( + "failed to load skill {}: {}", + err.path.display(), + err.message + ); + } + } + + let user_instructions = + get_user_instructions(&config, loaded_skills.as_ref().map(|o| o.skills.as_slice())) + .await; let exec_policy = load_exec_policy_for_features(&config.features, &config.codex_home) .await @@ -202,6 +225,8 @@ impl Codex { // Generate a unique ID for the lifetime of this Codex session. let session_source_clone = session_configuration.session_source.clone(); + let skills_cache = loaded_skills.as_ref().map(|o| o.skills.clone()); + let session = Session::new( session_configuration, config.clone(), @@ -210,6 +235,7 @@ impl Codex { tx_event.clone(), conversation_history, session_source_clone, + skills_cache, ) .await .map_err(|e| { @@ -466,6 +492,7 @@ impl Session { } } + #[allow(clippy::too_many_arguments)] async fn new( session_configuration: SessionConfiguration, config: Arc, @@ -474,6 +501,7 @@ impl Session { tx_event: Sender, initial_history: InitialHistory, session_source: SessionSource, + loaded_skills: Option>, ) -> anyhow::Result> { debug!( "Configuring session: model={}; provider={:?}", @@ -580,7 +608,7 @@ impl Session { .await .map(Arc::new); } - let state = SessionState::new(session_configuration.clone()); + let state = SessionState::new(session_configuration.clone(), loaded_skills); let services = SessionServices { mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())), @@ -1302,6 +1330,50 @@ impl Session { } } + async fn inject_skills( + &self, + turn_context: &TurnContext, + user_input: &[UserInput], + ) -> Vec { + if user_input.is_empty() || !self.enabled(Feature::Skills) { + return Vec::new(); + } + + let skills = { + let state = self.state.lock().await; + state.skills.clone().unwrap_or_default() + }; + + let mentioned_skills = collect_explicit_skill_mentions(user_input, &skills); + if mentioned_skills.is_empty() { + return Vec::new(); + } + + let mut injections: Vec = Vec::with_capacity(mentioned_skills.len()); + for skill in mentioned_skills { + match fs::read_to_string(&skill.path).await { + Ok(contents) => { + injections.push(ResponseItem::from(SkillInstructions { + name: skill.name, + path: skill.path.to_string_lossy().into_owned(), + contents, + })); + } + Err(err) => { + let message = format!( + "Failed to load skill {} at {}: {err:#}", + skill.name, + skill.path.display() + ); + self.send_event(turn_context, EventMsg::Warning(WarningEvent { message })) + .await; + } + } + } + + injections + } + pub(crate) async fn notify_background_event( &self, turn_context: &TurnContext, @@ -2017,6 +2089,36 @@ async fn spawn_review_thread( .await; } +fn collect_explicit_skill_mentions( + inputs: &[UserInput], + skills: &[SkillMetadata], +) -> Vec { + let mut full_text: Vec = Vec::new(); + for input in inputs { + if let UserInput::Text { text } = input { + full_text.push(text.clone()); + } + } + let combined = full_text.join(" "); + let mut seen: HashSet = HashSet::new(); + let mut matches: Vec = Vec::new(); + + for skill in skills { + let name = skill.name.clone(); + if seen.contains(&name) { + continue; + } + let needle = format!("${name}"); + let hit = combined.contains(&needle); + if hit { + seen.insert(name); + matches.push(skill.clone()); + } + } + + matches +} + /// Takes a user message as input and runs a loop where, at each turn, the model /// replies with either: /// @@ -2045,11 +2147,18 @@ pub(crate) async fn run_task( }); sess.send_event(&turn_context, event).await; + let skill_injections = sess.inject_skills(&turn_context, &input).await; + let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); let response_item: ResponseItem = initial_input_for_turn.clone().into(); sess.record_response_item_and_emit_turn_item(turn_context.as_ref(), response_item) .await; + if !skill_injections.is_empty() { + sess.record_conversation_items(&turn_context, &skill_injections) + .await; + } + sess.maybe_start_ghost_snapshot(Arc::clone(&turn_context), cancellation_token.child_token()) .await; let mut last_agent_message: Option = None; @@ -2603,7 +2712,7 @@ mod tests { session_source: SessionSource::Exec, }; - let mut state = SessionState::new(session_configuration); + let mut state = SessionState::new(session_configuration, None); let initial = RateLimitSnapshot { primary: Some(RateLimitWindow { used_percent: 10.0, @@ -2674,7 +2783,7 @@ mod tests { session_source: SessionSource::Exec, }; - let mut state = SessionState::new(session_configuration); + let mut state = SessionState::new(session_configuration, None); let initial = RateLimitSnapshot { primary: Some(RateLimitWindow { used_percent: 15.0, @@ -2880,7 +2989,7 @@ mod tests { let otel_event_manager = otel_event_manager(conversation_id, config.as_ref(), &model_family); - let state = SessionState::new(session_configuration.clone()); + let state = SessionState::new(session_configuration.clone(), None); let services = SessionServices { mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())), @@ -2962,7 +3071,7 @@ mod tests { let otel_event_manager = otel_event_manager(conversation_id, config.as_ref(), &model_family); - let state = SessionState::new(session_configuration.clone()); + let state = SessionState::new(session_configuration.clone(), None); let services = SessionServices { mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())), diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index 6b4bed4db3a..bdff398dac3 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -13,6 +13,7 @@ use codex_protocol::user_input::UserInput; use tracing::warn; use uuid::Uuid; +use crate::user_instructions::SkillInstructions; use crate::user_instructions::UserInstructions; use crate::user_shell_command::is_user_shell_command_text; @@ -23,7 +24,9 @@ fn is_session_prefix(text: &str) -> bool { } fn parse_user_message(message: &[ContentItem]) -> Option { - if UserInstructions::is_user_instructions(message) { + if UserInstructions::is_user_instructions(message) + || SkillInstructions::is_skill_instructions(message) + { return None; } diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index 43a0034801a..1a112c372ca 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -15,7 +15,7 @@ use crate::config::Config; use crate::features::Feature; -use crate::skills::load_skills; +use crate::skills::SkillMetadata; use crate::skills::render_skills_section; use dunce::canonicalize as normalize_path; use std::path::PathBuf; @@ -33,17 +33,12 @@ 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 { +pub(crate) async fn get_user_instructions( + config: &Config, + skills: Option<&[SkillMetadata]>, +) -> Option { 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) + skills.and_then(render_skills_section) } else { None }; @@ -289,7 +284,7 @@ mod tests { async fn no_doc_file_returns_none() { let tmp = tempfile::tempdir().expect("tempdir"); - let res = get_user_instructions(&make_config(&tmp, 4096, None)).await; + let res = get_user_instructions(&make_config(&tmp, 4096, None), None).await; assert!( res.is_none(), "Expected None when AGENTS.md is absent and no system instructions provided" @@ -303,7 +298,7 @@ mod tests { let tmp = tempfile::tempdir().expect("tempdir"); fs::write(tmp.path().join("AGENTS.md"), "hello world").unwrap(); - let res = get_user_instructions(&make_config(&tmp, 4096, None)) + let res = get_user_instructions(&make_config(&tmp, 4096, None), None) .await .expect("doc expected"); @@ -322,7 +317,7 @@ mod tests { let huge = "A".repeat(LIMIT * 2); // 2 KiB fs::write(tmp.path().join("AGENTS.md"), &huge).unwrap(); - let res = get_user_instructions(&make_config(&tmp, LIMIT, None)) + let res = get_user_instructions(&make_config(&tmp, LIMIT, None), None) .await .expect("doc expected"); @@ -354,7 +349,9 @@ mod tests { let mut cfg = make_config(&repo, 4096, None); cfg.cwd = nested; - let res = get_user_instructions(&cfg).await.expect("doc expected"); + let res = get_user_instructions(&cfg, None) + .await + .expect("doc expected"); assert_eq!(res, "root level doc"); } @@ -364,7 +361,7 @@ mod tests { let tmp = tempfile::tempdir().expect("tempdir"); fs::write(tmp.path().join("AGENTS.md"), "something").unwrap(); - let res = get_user_instructions(&make_config(&tmp, 0, None)).await; + let res = get_user_instructions(&make_config(&tmp, 0, None), None).await; assert!( res.is_none(), "With limit 0 the function should return None" @@ -380,7 +377,7 @@ mod tests { const INSTRUCTIONS: &str = "base instructions"; - let res = get_user_instructions(&make_config(&tmp, 4096, Some(INSTRUCTIONS))) + let res = get_user_instructions(&make_config(&tmp, 4096, Some(INSTRUCTIONS)), None) .await .expect("should produce a combined instruction string"); @@ -397,7 +394,7 @@ mod tests { const INSTRUCTIONS: &str = "some instructions"; - let res = get_user_instructions(&make_config(&tmp, 4096, Some(INSTRUCTIONS))).await; + let res = get_user_instructions(&make_config(&tmp, 4096, Some(INSTRUCTIONS)), None).await; assert_eq!(res, Some(INSTRUCTIONS.to_string())); } @@ -426,7 +423,9 @@ mod tests { let mut cfg = make_config(&repo, 4096, None); cfg.cwd = nested; - let res = get_user_instructions(&cfg).await.expect("doc expected"); + let res = get_user_instructions(&cfg, None) + .await + .expect("doc expected"); assert_eq!(res, "root doc\n\ncrate doc"); } @@ -439,7 +438,7 @@ mod tests { let cfg = make_config(&tmp, 4096, None); - let res = get_user_instructions(&cfg) + let res = get_user_instructions(&cfg, None) .await .expect("local doc expected"); @@ -461,7 +460,7 @@ mod tests { let cfg = make_config_with_fallback(&tmp, 4096, None, &["EXAMPLE.md"]); - let res = get_user_instructions(&cfg) + let res = get_user_instructions(&cfg, None) .await .expect("fallback doc expected"); @@ -477,7 +476,7 @@ mod tests { let cfg = make_config_with_fallback(&tmp, 4096, None, &["EXAMPLE.md", ".example.md"]); - let res = get_user_instructions(&cfg) + let res = get_user_instructions(&cfg, None) .await .expect("AGENTS.md should win"); @@ -506,7 +505,7 @@ mod tests { "extract from pdfs", ); - let res = get_user_instructions(&cfg) + let res = get_user_instructions(&cfg, None) .await .expect("instructions expected"); let expected_path = dunce::canonicalize( @@ -529,7 +528,7 @@ mod tests { let cfg = make_config(&tmp, 4096, None); create_skill(cfg.codex_home.clone(), "linting", "run clippy"); - let res = get_user_instructions(&cfg) + let res = get_user_instructions(&cfg, None) .await .expect("instructions expected"); let expected_path = diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index c61d1883735..39c2824356f 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -7,6 +7,7 @@ use crate::context_manager::ContextManager; use crate::protocol::RateLimitSnapshot; use crate::protocol::TokenUsage; use crate::protocol::TokenUsageInfo; +use crate::skills::SkillMetadata; use crate::truncate::TruncationPolicy; /// Persistent, session-scoped state previously stored directly on `Session`. @@ -14,16 +15,21 @@ pub(crate) struct SessionState { pub(crate) session_configuration: SessionConfiguration, pub(crate) history: ContextManager, pub(crate) latest_rate_limits: Option, + pub(crate) skills: Option>, } impl SessionState { /// Create a new session state mirroring previous `State::default()` semantics. - pub(crate) fn new(session_configuration: SessionConfiguration) -> Self { + pub(crate) fn new( + session_configuration: SessionConfiguration, + skills: Option>, + ) -> Self { let history = ContextManager::new(); Self { session_configuration, history, latest_rate_limits: None, + skills, } } diff --git a/codex-rs/core/src/user_instructions.rs b/codex-rs/core/src/user_instructions.rs index 61f8d7fde4f..e3028610204 100644 --- a/codex-rs/core/src/user_instructions.rs +++ b/codex-rs/core/src/user_instructions.rs @@ -6,6 +6,7 @@ use codex_protocol::models::ResponseItem; pub const USER_INSTRUCTIONS_OPEN_TAG_LEGACY: &str = ""; pub const USER_INSTRUCTIONS_PREFIX: &str = "# AGENTS.md instructions for "; +pub const SKILL_INSTRUCTIONS_PREFIX: &str = "# SKILL.md instructions for "; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(rename = "user_instructions", rename_all = "snake_case")] @@ -41,6 +42,41 @@ impl From for ResponseItem { } } +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(rename = "skill_instructions", rename_all = "snake_case")] +pub(crate) struct SkillInstructions { + pub name: String, + pub path: String, + pub contents: String, +} + +impl SkillInstructions { + pub fn is_skill_instructions(message: &[ContentItem]) -> bool { + if let [ContentItem::InputText { text }] = message { + text.starts_with(SKILL_INSTRUCTIONS_PREFIX) + } else { + false + } + } +} + +impl From for ResponseItem { + fn from(si: SkillInstructions) -> Self { + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { + text: format!( + "{SKILL_INSTRUCTIONS_PREFIX}{name}\nPath: {path}\n\n\n{contents}\n", + name = si.name, + path = si.path, + contents = si.contents + ), + }], + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(rename = "developer_instructions", rename_all = "snake_case")] pub(crate) struct DeveloperInstructions { From 85ff32b5059867dcda4a1cfb048e830f69d2286a Mon Sep 17 00:00:00 2001 From: Xin Lin Date: Tue, 9 Dec 2025 16:58:05 -0800 Subject: [PATCH 2/3] update skill loading + add tests. --- codex-rs/core/src/codex.rs | 103 +++++++++++++----- codex-rs/core/src/event_mapping.rs | 24 ++-- codex-rs/core/src/project_doc.rs | 21 +++- codex-rs/core/src/state/session.rs | 6 +- codex-rs/core/src/user_instructions.rs | 45 +++++++- codex-rs/core/tests/common/test_codex.rs | 45 ++++++++ codex-rs/core/tests/suite/mod.rs | 1 + codex-rs/core/tests/suite/skills.rs | 84 ++++++++++++++ .../tests/event_processor_with_json_output.rs | 1 + codex-rs/mcp-server/src/outgoing_message.rs | 2 + codex-rs/protocol/src/models.rs | 3 + codex-rs/protocol/src/protocol.rs | 23 ++++ codex-rs/protocol/src/user_input.rs | 6 + codex-rs/tui/src/app.rs | 60 +++++----- codex-rs/tui/src/app_backtrack.rs | 1 - codex-rs/tui/src/bottom_pane/chat_composer.rs | 4 + codex-rs/tui/src/bottom_pane/mod.rs | 9 ++ codex-rs/tui/src/chatwidget.rs | 52 ++++++++- codex-rs/tui/src/chatwidget/tests.rs | 2 +- 19 files changed, 408 insertions(+), 84 deletions(-) create mode 100644 codex-rs/core/tests/suite/skills.rs diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5cf9fe88a74..cde1a0defff 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::collections::HashSet; use std::fmt::Debug; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicU64; @@ -100,6 +101,9 @@ use crate::protocol::ReviewDecision; use crate::protocol::SandboxCommandAssessment; use crate::protocol::SandboxPolicy; use crate::protocol::SessionConfiguredEvent; +use crate::protocol::SkillErrorInfo; +use crate::protocol::SkillInfo; +use crate::protocol::SkillLoadOutcomeInfo; use crate::protocol::StreamErrorEvent; use crate::protocol::Submission; use crate::protocol::TokenCountEvent; @@ -111,9 +115,10 @@ use crate::rollout::RolloutRecorder; use crate::rollout::RolloutRecorderParams; use crate::rollout::map_session_init_error; use crate::shell; +use crate::shell_snapshot::ShellSnapshot; +use crate::skills::SkillLoadOutcome; use crate::skills::SkillMetadata; use crate::skills::load_skills; -use crate::shell_snapshot::ShellSnapshot; use crate::state::ActiveTurn; use crate::state::SessionServices; use crate::state::SessionState; @@ -195,9 +200,18 @@ impl Codex { } } - let user_instructions = - get_user_instructions(&config, loaded_skills.as_ref().map(|o| o.skills.as_slice())) - .await; + let skills_outcome = loaded_skills.clone(); + + let user_instructions = get_user_instructions( + &config, + skills_outcome.as_ref().and_then(|outcome| { + outcome + .errors + .is_empty() + .then_some(outcome.skills.as_slice()) + }), + ) + .await; let exec_policy = load_exec_policy_for_features(&config.features, &config.codex_home) .await @@ -225,7 +239,6 @@ impl Codex { // Generate a unique ID for the lifetime of this Codex session. let session_source_clone = session_configuration.session_source.clone(); - let skills_cache = loaded_skills.as_ref().map(|o| o.skills.clone()); let session = Session::new( session_configuration, @@ -235,7 +248,7 @@ impl Codex { tx_event.clone(), conversation_history, session_source_clone, - skills_cache, + skills_outcome.clone(), ) .await .map_err(|e| { @@ -501,7 +514,7 @@ impl Session { tx_event: Sender, initial_history: InitialHistory, session_source: SessionSource, - loaded_skills: Option>, + skills: Option, ) -> anyhow::Result> { debug!( "Configuring session: model={}; provider={:?}", @@ -608,7 +621,7 @@ impl Session { .await .map(Arc::new); } - let state = SessionState::new(session_configuration.clone(), loaded_skills); + let state = SessionState::new(session_configuration.clone(), skills.clone()); let services = SessionServices { mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())), @@ -637,6 +650,7 @@ impl Session { // Dispatch the SessionConfiguredEvent first and then report any errors. // If resuming, include converted initial messages in the payload so UIs can render them immediately. let initial_messages = initial_history.get_event_msgs(); + let skill_load_outcome = skill_load_outcome_for_client(skills.as_ref()); let events = std::iter::once(Event { id: INITIAL_SUBMIT_ID.to_owned(), @@ -651,6 +665,7 @@ impl Session { history_log_id, history_entry_count, initial_messages, + skill_load_outcome, rollout_path, }), }) @@ -1341,7 +1356,11 @@ impl Session { let skills = { let state = self.state.lock().await; - state.skills.clone().unwrap_or_default() + state + .skills + .as_ref() + .map(|outcome| outcome.skills.clone()) + .unwrap_or_default() }; let mentioned_skills = collect_explicit_skill_mentions(user_input, &skills); @@ -2093,30 +2112,60 @@ fn collect_explicit_skill_mentions( inputs: &[UserInput], skills: &[SkillMetadata], ) -> Vec { - let mut full_text: Vec = Vec::new(); + let mut selected: Vec = Vec::new(); + let mut seen: HashSet = HashSet::new(); + for input in inputs { - if let UserInput::Text { text } = input { - full_text.push(text.clone()); + if let UserInput::Skill { name, path } = input + && seen.insert(name.clone()) + && let Some(skill) = skills + .iter() + .find(|s| s.name == *name && paths_match(&s.path, path)) + { + selected.push(skill.clone()); } } - let combined = full_text.join(" "); - let mut seen: HashSet = HashSet::new(); - let mut matches: Vec = Vec::new(); - for skill in skills { - let name = skill.name.clone(); - if seen.contains(&name) { - continue; - } - let needle = format!("${name}"); - let hit = combined.contains(&needle); - if hit { - seen.insert(name); - matches.push(skill.clone()); - } + selected +} + +fn paths_match(a: &Path, b: &Path) -> bool { + if a == b { + return true; } - matches + let Ok(ca) = std::fs::canonicalize(a) else { + return false; + }; + let Ok(cb) = std::fs::canonicalize(b) else { + return false; + }; + + ca == cb +} + +fn skill_load_outcome_for_client( + outcome: Option<&SkillLoadOutcome>, +) -> Option { + outcome.map(|outcome| SkillLoadOutcomeInfo { + skills: outcome + .skills + .iter() + .map(|skill| SkillInfo { + name: skill.name.clone(), + description: skill.description.clone(), + path: skill.path.clone(), + }) + .collect(), + errors: outcome + .errors + .iter() + .map(|err| SkillErrorInfo { + path: err.path.clone(), + message: err.message.clone(), + }) + .collect(), + }) } /// Takes a user message as input and runs a loop where, at each turn, the model diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index bdff398dac3..21934c4251b 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -201,14 +201,22 @@ mod tests { text: "# AGENTS.md instructions for test_directory\n\n\ntest_text\n".to_string(), }], }, - ResponseItem::Message { - id: None, - role: "user".to_string(), - content: vec![ContentItem::InputText { - text: "echo 42".to_string(), - }], - }, - ]; + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { + text: "\nbody\n" + .to_string(), + }], + }, + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { + text: "echo 42".to_string(), + }], + }, + ]; for item in items { let turn_item = parse_turn_item(&item); diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index 1a112c372ca..cd05520110f 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -239,6 +239,7 @@ mod tests { use super::*; use crate::config::ConfigOverrides; use crate::config::ConfigToml; + use crate::skills::load_skills; use std::fs; use std::path::PathBuf; use tempfile::TempDir; @@ -505,9 +506,13 @@ mod tests { "extract from pdfs", ); - let res = get_user_instructions(&cfg, None) - .await - .expect("instructions expected"); + let skills = load_skills(&cfg); + let res = get_user_instructions( + &cfg, + skills.errors.is_empty().then_some(skills.skills.as_slice()), + ) + .await + .expect("instructions expected"); let expected_path = dunce::canonicalize( cfg.codex_home .join("skills/pdf-processing/SKILL.md") @@ -528,9 +533,13 @@ mod tests { let cfg = make_config(&tmp, 4096, None); create_skill(cfg.codex_home.clone(), "linting", "run clippy"); - let res = get_user_instructions(&cfg, None) - .await - .expect("instructions expected"); + let skills = load_skills(&cfg); + let res = get_user_instructions( + &cfg, + skills.errors.is_empty().then_some(skills.skills.as_slice()), + ) + .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")); diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index 39c2824356f..35b7b6c63ec 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -7,7 +7,7 @@ use crate::context_manager::ContextManager; use crate::protocol::RateLimitSnapshot; use crate::protocol::TokenUsage; use crate::protocol::TokenUsageInfo; -use crate::skills::SkillMetadata; +use crate::skills::SkillLoadOutcome; use crate::truncate::TruncationPolicy; /// Persistent, session-scoped state previously stored directly on `Session`. @@ -15,14 +15,14 @@ pub(crate) struct SessionState { pub(crate) session_configuration: SessionConfiguration, pub(crate) history: ContextManager, pub(crate) latest_rate_limits: Option, - pub(crate) skills: Option>, + pub(crate) skills: Option, } impl SessionState { /// Create a new session state mirroring previous `State::default()` semantics. pub(crate) fn new( session_configuration: SessionConfiguration, - skills: Option>, + skills: Option, ) -> Self { let history = ContextManager::new(); Self { diff --git a/codex-rs/core/src/user_instructions.rs b/codex-rs/core/src/user_instructions.rs index e3028610204..1c4fcf666fd 100644 --- a/codex-rs/core/src/user_instructions.rs +++ b/codex-rs/core/src/user_instructions.rs @@ -6,7 +6,7 @@ use codex_protocol::models::ResponseItem; pub const USER_INSTRUCTIONS_OPEN_TAG_LEGACY: &str = ""; pub const USER_INSTRUCTIONS_PREFIX: &str = "# AGENTS.md instructions for "; -pub const SKILL_INSTRUCTIONS_PREFIX: &str = "# SKILL.md instructions for "; +pub const SKILL_INSTRUCTIONS_PREFIX: &str = " for ResponseItem { role: "user".to_string(), content: vec![ContentItem::InputText { text: format!( - "{SKILL_INSTRUCTIONS_PREFIX}{name}\nPath: {path}\n\n\n{contents}\n", + "\n{contents}\n", name = si.name, path = si.path, contents = si.contents @@ -108,6 +108,7 @@ impl From for ResponseItem { #[cfg(test)] mod tests { use super::*; + use pretty_assertions::assert_eq; #[test] fn test_user_instructions() { @@ -151,4 +152,44 @@ mod tests { } ])); } + + #[test] + fn test_skill_instructions() { + let skill_instructions = SkillInstructions { + name: "demo-skill".to_string(), + path: "skills/demo/SKILL.md".to_string(), + contents: "body".to_string(), + }; + let response_item: ResponseItem = skill_instructions.into(); + + let ResponseItem::Message { role, content, .. } = response_item else { + panic!("expected ResponseItem::Message"); + }; + + assert_eq!(role, "user"); + + let [ContentItem::InputText { text }] = content.as_slice() else { + panic!("expected one InputText content item"); + }; + + assert_eq!( + text, + "\nbody\n", + ); + } + + #[test] + fn test_is_skill_instructions() { + assert!(SkillInstructions::is_skill_instructions(&[ + ContentItem::InputText { + text: "\nbody\n" + .to_string(), + } + ])); + assert!(!SkillInstructions::is_skill_instructions(&[ + ContentItem::InputText { + text: "regular text".to_string(), + } + ])); + } } diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index 23bcadadf15..5c738246bb9 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -27,6 +27,7 @@ use crate::responses::start_mock_server; use crate::wait_for_event; type ConfigMutator = dyn FnOnce(&mut Config) + Send; +type PreBuildHook = dyn FnOnce(&Path) + Send + 'static; /// A collection of different ways the model can output an apply_patch call #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] @@ -50,6 +51,7 @@ pub enum ShellModelOutput { pub struct TestCodexBuilder { config_mutators: Vec>, auth: CodexAuth, + pre_build_hooks: Vec>, } impl TestCodexBuilder { @@ -73,6 +75,14 @@ impl TestCodexBuilder { }) } + pub fn with_pre_build_hook(mut self, hook: F) -> Self + where + F: FnOnce(&Path) + Send + 'static, + { + self.pre_build_hooks.push(Box::new(hook)); + self + } + pub async fn build(&mut self, server: &wiremock::MockServer) -> anyhow::Result { let home = Arc::new(TempDir::new()?); self.build_with_home(server, home, None).await @@ -135,6 +145,9 @@ impl TestCodexBuilder { let mut config = load_default_config_for_test(home); config.cwd = cwd.path().to_path_buf(); config.model_provider = model_provider; + for hook in self.pre_build_hooks.drain(..) { + hook(home.path()); + } if let Ok(cmd) = assert_cmd::Command::cargo_bin("codex") { config.codex_linux_sandbox_exe = Some(PathBuf::from(cmd.get_program().to_os_string())); } @@ -169,6 +182,10 @@ impl TestCodex { self.cwd.path() } + pub fn codex_home_path(&self) -> &Path { + self.config.codex_home.as_path() + } + pub fn workspace_path(&self, rel: impl AsRef) -> PathBuf { self.cwd_path().join(rel) } @@ -219,6 +236,33 @@ impl TestCodex { .await; Ok(()) } + + pub async fn submit_items( + &self, + items: Vec, + approval_policy: AskForApproval, + sandbox_policy: SandboxPolicy, + ) -> Result<()> { + let session_model = self.session_configured.model.clone(); + self.codex + .submit(Op::UserTurn { + items, + final_output_json_schema: None, + cwd: self.cwd.path().to_path_buf(), + approval_policy, + sandbox_policy, + model: session_model, + effort: None, + summary: ReasoningSummary::Auto, + }) + .await?; + + wait_for_event(&self.codex, |event| { + matches!(event, EventMsg::TaskComplete(_)) + }) + .await; + Ok(()) + } } pub struct TestCodexHarness { @@ -355,5 +399,6 @@ pub fn test_codex() -> TestCodexBuilder { TestCodexBuilder { config_mutators: vec![], auth: CodexAuth::from_api_key("dummy"), + pre_build_hooks: vec![], } } diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 29cc3ffb191..e047899d722 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -50,6 +50,7 @@ mod seatbelt; mod shell_command; mod shell_serialization; mod shell_snapshot; +mod skills; mod stream_error_allows_next_turn; mod stream_no_completed; mod text_encoding_fix; diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs new file mode 100644 index 00000000000..1fe8c9f7038 --- /dev/null +++ b/codex-rs/core/tests/suite/skills.rs @@ -0,0 +1,84 @@ +#![cfg(not(target_os = "windows"))] +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use anyhow::Result; +use codex_core::features::Feature; +use codex_core::protocol::AskForApproval; +use codex_core::protocol::SandboxPolicy; +use codex_protocol::user_input::UserInput; +use core_test_support::responses::ev_assistant_message; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_sse_once; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::test_codex; +use std::fs; +use std::path::Path; + +fn write_skill(home: &Path, name: &str, description: &str, body: &str) -> std::path::PathBuf { + let skill_dir = home.join("skills").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(())); + + let server = start_mock_server().await; + let skill_body = "skill body"; + let mut builder = test_codex() + .with_config(|cfg| { + cfg.features.enable(Feature::Skills); + }) + .with_pre_build_hook(|home| { + write_skill(home, "demo", "demo skill", skill_body); + }); + let test = builder.build(&server).await?; + + let skill_path = test.codex_home_path().join("skills/demo/SKILL.md"); + + let mock = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-1"), + ]), + ) + .await; + + test.submit_items( + vec![ + UserInput::Text { + text: "please use $demo".to_string(), + }, + UserInput::Skill { + name: "demo".to_string(), + path: skill_path.clone(), + }, + ], + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let request = mock.single_request(); + let user_texts = request.message_input_texts("user"); + let skill_path_str = skill_path.to_string_lossy(); + assert!( + user_texts.iter().any(|text| { + text.contains("> for ResponseInputItem { } } }, + UserInput::Skill { .. } => ContentItem::InputText { + text: String::new(), + }, }) .collect::>(), } diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 89b5fd315a6..7b6c5b6db4c 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1612,6 +1612,25 @@ pub struct ListCustomPromptsResponseEvent { pub custom_prompts: Vec, } +#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] +pub struct SkillInfo { + pub name: String, + pub description: String, + pub path: PathBuf, +} + +#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] +pub struct SkillErrorInfo { + pub path: PathBuf, + pub message: String, +} + +#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS, Default)] +pub struct SkillLoadOutcomeInfo { + pub skills: Vec, + pub errors: Vec, +} + #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct SessionConfiguredEvent { /// Name left as session_id instead of conversation_id for backwards compatibility. @@ -1647,6 +1666,9 @@ pub struct SessionConfiguredEvent { #[serde(skip_serializing_if = "Option::is_none")] pub initial_messages: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub skill_load_outcome: Option, + pub rollout_path: PathBuf, } @@ -1774,6 +1796,7 @@ mod tests { history_log_id: 0, history_entry_count: 0, initial_messages: None, + skill_load_outcome: None, rollout_path: rollout_file.path().to_path_buf(), }), }; diff --git a/codex-rs/protocol/src/user_input.rs b/codex-rs/protocol/src/user_input.rs index 881b9965145..26773e1a1a8 100644 --- a/codex-rs/protocol/src/user_input.rs +++ b/codex-rs/protocol/src/user_input.rs @@ -21,4 +21,10 @@ pub enum UserInput { LocalImage { path: std::path::PathBuf, }, + + /// Skill selected by the user (name + path to SKILL.md). + Skill { + name: String, + path: std::path::PathBuf, + }, } diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 0a09b15e7c8..e01ca2391e2 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -25,6 +25,7 @@ use codex_core::AuthManager; use codex_core::ConversationManager; use codex_core::config::Config; use codex_core::config::edit::ConfigEditsBuilder; +#[cfg(target_os = "windows")] use codex_core::features::Feature; use codex_core::openai_models::model_presets::HIDE_GPT_5_1_CODEX_MAX_MIGRATION_PROMPT_CONFIG; use codex_core::openai_models::model_presets::HIDE_GPT5_1_MIGRATION_PROMPT_CONFIG; @@ -33,9 +34,9 @@ use codex_core::protocol::EventMsg; use codex_core::protocol::FinalOutput; use codex_core::protocol::Op; use codex_core::protocol::SessionSource; +use codex_core::protocol::SkillLoadOutcomeInfo; use codex_core::protocol::TokenUsage; -use codex_core::skills::load_skills; -use codex_core::skills::model::SkillMetadata; +use codex_core::skills::SkillError; use codex_protocol::ConversationId; use codex_protocol::openai_models::ModelPreset; use codex_protocol::openai_models::ModelUpgrade; @@ -88,6 +89,17 @@ fn session_summary( }) } +fn skill_errors_from_outcome(outcome: &SkillLoadOutcomeInfo) -> Vec { + outcome + .errors + .iter() + .map(|err| SkillError { + path: err.path.clone(), + message: err.message.clone(), + }) + .collect() +} + #[derive(Debug, Clone, PartialEq, Eq)] struct SessionSummary { usage_line: String, @@ -235,8 +247,6 @@ pub(crate) struct App { // One-shot suppression of the next world-writable scan after user confirmation. skip_world_writable_scan_once: bool, - - pub(crate) skills: Option>, } impl App { @@ -281,26 +291,6 @@ impl App { return Ok(exit_info); } - 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 skills = if config.features.enabled(Feature::Skills) { - Some(skills_outcome.skills.clone()) - } else { - None - }; - let enhanced_keys_supported = tui.enhanced_keys_supported(); let model_family = conversation_manager .get_models_manager() @@ -318,7 +308,6 @@ impl App { auth_manager: auth_manager.clone(), models_manager: conversation_manager.get_models_manager(), feedback: feedback.clone(), - skills: skills.clone(), is_first_run, model_family, }; @@ -345,7 +334,6 @@ impl App { auth_manager: auth_manager.clone(), models_manager: conversation_manager.get_models_manager(), feedback: feedback.clone(), - skills: skills.clone(), is_first_run, model_family, }; @@ -382,7 +370,6 @@ impl App { pending_update_action: None, suppress_shutdown_complete: false, skip_world_writable_scan_once: false, - skills, }; // On startup, if Agent mode (workspace-write) or ReadOnly is active, warn about world-writable dirs on Windows. @@ -508,7 +495,6 @@ impl App { auth_manager: self.auth_manager.clone(), models_manager: self.server.get_models_manager(), feedback: self.feedback.clone(), - skills: self.skills.clone(), is_first_run: false, model_family, }; @@ -558,7 +544,6 @@ impl App { auth_manager: self.auth_manager.clone(), models_manager: self.server.get_models_manager(), feedback: self.feedback.clone(), - skills: self.skills.clone(), is_first_run: false, model_family: model_family.clone(), }; @@ -649,6 +634,19 @@ impl App { self.suppress_shutdown_complete = false; return Ok(true); } + if let EventMsg::SessionConfigured(cfg) = &event.msg + && let Some(outcome) = cfg.skill_load_outcome.as_ref() + && !outcome.errors.is_empty() + { + let errors = skill_errors_from_outcome(outcome); + match run_skill_error_prompt(tui, &errors).await { + SkillErrorPromptOutcome::Exit => { + self.chat_widget.submit_op(Op::Shutdown); + return Ok(false); + } + SkillErrorPromptOutcome::Continue => {} + } + } self.chat_widget.handle_codex_event(event); } AppEvent::ConversationHistory(ev) => { @@ -1193,7 +1191,6 @@ mod tests { pending_update_action: None, suppress_shutdown_complete: false, skip_world_writable_scan_once: false, - skills: None, } } @@ -1231,7 +1228,6 @@ mod tests { pending_update_action: None, suppress_shutdown_complete: false, skip_world_writable_scan_once: false, - skills: None, }, rx, op_rx, @@ -1339,6 +1335,7 @@ mod tests { history_log_id: 0, history_entry_count: 0, initial_messages: None, + skill_load_outcome: None, rollout_path: PathBuf::new(), }; Arc::new(new_session_info( @@ -1393,6 +1390,7 @@ mod tests { history_log_id: 0, history_entry_count: 0, initial_messages: None, + skill_load_outcome: None, rollout_path: PathBuf::new(), }; diff --git a/codex-rs/tui/src/app_backtrack.rs b/codex-rs/tui/src/app_backtrack.rs index ca9de52e2a5..651e2ac498f 100644 --- a/codex-rs/tui/src/app_backtrack.rs +++ b/codex-rs/tui/src/app_backtrack.rs @@ -349,7 +349,6 @@ impl App { auth_manager: self.auth_manager.clone(), models_manager: self.server.get_models_manager(), feedback: self.feedback.clone(), - skills: self.skills.clone(), is_first_run: false, }; self.chat_widget = diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 4deb5125c12..75d40d35af9 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -801,6 +801,10 @@ impl ChatComposer { self.skills.as_ref().is_some_and(|s| !s.is_empty()) } + pub fn skills(&self) -> Option<&Vec> { + self.skills.as_ref() + } + /// Extract a token prefixed with `prefix` under the cursor, if any. /// /// The returned string **does not** include the prefix. diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 8a4336f6fe8..1d5bc6a0168 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -131,10 +131,19 @@ impl BottomPane { } } + pub fn set_skills(&mut self, skills: Option>) { + self.composer.set_skill_mentions(skills); + self.request_redraw(); + } + pub fn status_widget(&self) -> Option<&StatusIndicatorWidget> { self.status.as_ref() } + pub fn skills(&self) -> Option<&Vec> { + self.composer.skills() + } + #[cfg(test)] pub(crate) fn context_window_percent(&self) -> Option { self.context_window_percent diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index a0b42ddbe4d..b28c59e7b52 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -44,6 +44,7 @@ use codex_core::protocol::PatchApplyBeginEvent; use codex_core::protocol::RateLimitSnapshot; use codex_core::protocol::ReviewRequest; use codex_core::protocol::ReviewTarget; +use codex_core::protocol::SkillLoadOutcomeInfo; use codex_core::protocol::StreamErrorEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TokenUsage; @@ -260,7 +261,6 @@ pub(crate) struct ChatWidgetInit { pub(crate) auth_manager: Arc, pub(crate) models_manager: Arc, pub(crate) feedback: codex_feedback::CodexFeedback, - pub(crate) skills: Option>, pub(crate) is_first_run: bool, pub(crate) model_family: ModelFamily, } @@ -389,6 +389,7 @@ impl ChatWidget { fn on_session_configured(&mut self, event: codex_core::protocol::SessionConfiguredEvent) { self.bottom_pane .set_history_metadata(event.history_log_id, event.history_entry_count); + self.set_skills_from_outcome(event.skill_load_outcome.as_ref()); self.conversation_id = Some(event.session_id); self.current_rollout_path = Some(event.rollout_path.clone()); let initial_messages = event.initial_messages.clone(); @@ -412,6 +413,11 @@ impl ChatWidget { } } + fn set_skills_from_outcome(&mut self, outcome: Option<&SkillLoadOutcomeInfo>) { + let skills = outcome.map(skills_from_outcome); + self.bottom_pane.set_skills(skills); + } + pub(crate) fn open_feedback_note( &mut self, category: crate::app_event::FeedbackCategory, @@ -1259,7 +1265,6 @@ impl ChatWidget { auth_manager, models_manager, feedback, - skills, is_first_run, model_family, } = common; @@ -1279,7 +1284,7 @@ impl ChatWidget { placeholder_text: placeholder, disable_paste_burst: config.disable_paste_burst, animations_enabled: config.animations, - skills, + skills: None, }), active_cell: None, config: config.clone(), @@ -1342,7 +1347,6 @@ impl ChatWidget { auth_manager, models_manager, feedback, - skills, model_family, .. } = common; @@ -1364,7 +1368,7 @@ impl ChatWidget { placeholder_text: placeholder, disable_paste_burst: config.disable_paste_burst, animations_enabled: config.animations, - skills, + skills: None, }), active_cell: None, config: config.clone(), @@ -1731,6 +1735,16 @@ impl ChatWidget { items.push(UserInput::LocalImage { path }); } + if let Some(skills) = self.bottom_pane.skills() { + let skill_mentions = find_skill_mentions(&text, skills); + for skill in skill_mentions { + items.push(UserInput::Skill { + name: skill.name.clone(), + path: skill.path.clone(), + }); + } + } + self.codex_op_tx .send(Op::UserInput { items }) .unwrap_or_else(|e| { @@ -3450,5 +3464,33 @@ pub(crate) fn show_review_commit_picker_with_entries( }); } +fn skills_from_outcome(outcome: &SkillLoadOutcomeInfo) -> Vec { + outcome + .skills + .iter() + .map(|skill| SkillMetadata { + name: skill.name.clone(), + description: skill.description.clone(), + path: skill.path.clone(), + }) + .collect() +} + +fn find_skill_mentions(text: &str, skills: &[SkillMetadata]) -> Vec { + let mut seen: HashSet = HashSet::new(); + let mut matches: Vec = Vec::new(); + for skill in skills { + if seen.contains(&skill.name) { + continue; + } + let needle = format!("${}", skill.name); + if text.contains(&needle) { + seen.insert(skill.name.clone()); + matches.push(skill.clone()); + } + } + matches +} + #[cfg(test)] pub(crate) mod tests; diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 0135abff733..df4dfe6e6eb 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -121,6 +121,7 @@ fn resumed_initial_messages_render_history() { message: "assistant reply".to_string(), }), ]), + skill_load_outcome: None, rollout_path: rollout_file.path().to_path_buf(), }; @@ -361,7 +362,6 @@ async fn helpers_are_available_and_do_not_panic() { auth_manager, models_manager: conversation_manager.get_models_manager(), feedback: codex_feedback::CodexFeedback::new(), - skills: None, is_first_run: true, model_family, }; From 5f7a5d171f16d4ab7ff798ef89b26be65d044a23 Mon Sep 17 00:00:00 2001 From: Xin Lin Date: Wed, 10 Dec 2025 11:54:40 -0800 Subject: [PATCH 3/3] address comments. --- codex-rs/core/src/codex.rs | 127 +++++------------------ codex-rs/core/src/event_mapping.rs | 2 +- codex-rs/core/src/skills/injection.rs | 78 ++++++++++++++ codex-rs/core/src/skills/mod.rs | 3 + codex-rs/core/src/state/service.rs | 2 + codex-rs/core/src/state/session.rs | 8 +- codex-rs/core/src/user_instructions.rs | 12 +-- codex-rs/core/tests/common/test_codex.rs | 27 ----- codex-rs/core/tests/suite/skills.rs | 82 ++++++++++++--- codex-rs/protocol/src/models.rs | 28 +++-- 10 files changed, 194 insertions(+), 175 deletions(-) create mode 100644 codex-rs/core/src/skills/injection.rs diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index cde1a0defff..65b593feb31 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1,7 +1,5 @@ use std::collections::HashMap; -use std::collections::HashSet; use std::fmt::Debug; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicU64; @@ -57,7 +55,6 @@ use mcp_types::ReadResourceResult; use mcp_types::RequestId; use serde_json; use serde_json::Value; -use tokio::fs; use tokio::sync::Mutex; use tokio::sync::RwLock; use tokio::sync::oneshot; @@ -116,8 +113,9 @@ use crate::rollout::RolloutRecorderParams; use crate::rollout::map_session_init_error; use crate::shell; use crate::shell_snapshot::ShellSnapshot; +use crate::skills::SkillInjections; use crate::skills::SkillLoadOutcome; -use crate::skills::SkillMetadata; +use crate::skills::build_skill_injections; use crate::skills::load_skills; use crate::state::ActiveTurn; use crate::state::SessionServices; @@ -135,7 +133,6 @@ use crate::tools::spec::ToolsConfigParams; use crate::turn_diff_tracker::TurnDiffTracker; use crate::unified_exec::UnifiedExecSessionManager; use crate::user_instructions::DeveloperInstructions; -use crate::user_instructions::SkillInstructions; use crate::user_instructions::UserInstructions; use crate::user_notification::UserNotification; use crate::util::backoff; @@ -204,12 +201,9 @@ impl Codex { let user_instructions = get_user_instructions( &config, - skills_outcome.as_ref().and_then(|outcome| { - outcome - .errors - .is_empty() - .then_some(outcome.skills.as_slice()) - }), + skills_outcome + .as_ref() + .map(|outcome| outcome.skills.as_slice()), ) .await; @@ -621,7 +615,7 @@ impl Session { .await .map(Arc::new); } - let state = SessionState::new(session_configuration.clone(), skills.clone()); + let state = SessionState::new(session_configuration.clone()); let services = SessionServices { mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())), @@ -635,6 +629,7 @@ impl Session { otel_event_manager, models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), + skills: skills.clone(), }; let sess = Arc::new(Session { @@ -1345,54 +1340,6 @@ impl Session { } } - async fn inject_skills( - &self, - turn_context: &TurnContext, - user_input: &[UserInput], - ) -> Vec { - if user_input.is_empty() || !self.enabled(Feature::Skills) { - return Vec::new(); - } - - let skills = { - let state = self.state.lock().await; - state - .skills - .as_ref() - .map(|outcome| outcome.skills.clone()) - .unwrap_or_default() - }; - - let mentioned_skills = collect_explicit_skill_mentions(user_input, &skills); - if mentioned_skills.is_empty() { - return Vec::new(); - } - - let mut injections: Vec = Vec::with_capacity(mentioned_skills.len()); - for skill in mentioned_skills { - match fs::read_to_string(&skill.path).await { - Ok(contents) => { - injections.push(ResponseItem::from(SkillInstructions { - name: skill.name, - path: skill.path.to_string_lossy().into_owned(), - contents, - })); - } - Err(err) => { - let message = format!( - "Failed to load skill {} at {}: {err:#}", - skill.name, - skill.path.display() - ); - self.send_event(turn_context, EventMsg::Warning(WarningEvent { message })) - .await; - } - } - } - - injections - } - pub(crate) async fn notify_background_event( &self, turn_context: &TurnContext, @@ -2108,42 +2055,6 @@ async fn spawn_review_thread( .await; } -fn collect_explicit_skill_mentions( - inputs: &[UserInput], - skills: &[SkillMetadata], -) -> Vec { - let mut selected: Vec = Vec::new(); - let mut seen: HashSet = HashSet::new(); - - for input in inputs { - if let UserInput::Skill { name, path } = input - && seen.insert(name.clone()) - && let Some(skill) = skills - .iter() - .find(|s| s.name == *name && paths_match(&s.path, path)) - { - selected.push(skill.clone()); - } - } - - selected -} - -fn paths_match(a: &Path, b: &Path) -> bool { - if a == b { - return true; - } - - let Ok(ca) = std::fs::canonicalize(a) else { - return false; - }; - let Ok(cb) = std::fs::canonicalize(b) else { - return false; - }; - - ca == cb -} - fn skill_load_outcome_for_client( outcome: Option<&SkillLoadOutcome>, ) -> Option { @@ -2196,15 +2107,23 @@ pub(crate) async fn run_task( }); sess.send_event(&turn_context, event).await; - let skill_injections = sess.inject_skills(&turn_context, &input).await; + let SkillInjections { + items: skill_items, + warnings: skill_warnings, + } = build_skill_injections(&input, sess.services.skills.as_ref()).await; + + for message in skill_warnings { + sess.send_event(&turn_context, EventMsg::Warning(WarningEvent { message })) + .await; + } let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); let response_item: ResponseItem = initial_input_for_turn.clone().into(); sess.record_response_item_and_emit_turn_item(turn_context.as_ref(), response_item) .await; - if !skill_injections.is_empty() { - sess.record_conversation_items(&turn_context, &skill_injections) + if !skill_items.is_empty() { + sess.record_conversation_items(&turn_context, &skill_items) .await; } @@ -2761,7 +2680,7 @@ mod tests { session_source: SessionSource::Exec, }; - let mut state = SessionState::new(session_configuration, None); + let mut state = SessionState::new(session_configuration); let initial = RateLimitSnapshot { primary: Some(RateLimitWindow { used_percent: 10.0, @@ -2832,7 +2751,7 @@ mod tests { session_source: SessionSource::Exec, }; - let mut state = SessionState::new(session_configuration, None); + let mut state = SessionState::new(session_configuration); let initial = RateLimitSnapshot { primary: Some(RateLimitWindow { used_percent: 15.0, @@ -3038,7 +2957,7 @@ mod tests { let otel_event_manager = otel_event_manager(conversation_id, config.as_ref(), &model_family); - let state = SessionState::new(session_configuration.clone(), None); + let state = SessionState::new(session_configuration.clone()); let services = SessionServices { mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())), @@ -3052,6 +2971,7 @@ mod tests { otel_event_manager: otel_event_manager.clone(), models_manager, tool_approvals: Mutex::new(ApprovalStore::default()), + skills: None, }; let turn_context = Session::make_turn_context( @@ -3120,7 +3040,7 @@ mod tests { let otel_event_manager = otel_event_manager(conversation_id, config.as_ref(), &model_family); - let state = SessionState::new(session_configuration.clone(), None); + let state = SessionState::new(session_configuration.clone()); let services = SessionServices { mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())), @@ -3134,6 +3054,7 @@ mod tests { otel_event_manager: otel_event_manager.clone(), models_manager, tool_approvals: Mutex::new(ApprovalStore::default()), + skills: None, }; let turn_context = Arc::new(Session::make_turn_context( diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index 21934c4251b..6ab6291a4bb 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -205,7 +205,7 @@ mod tests { id: None, role: "user".to_string(), content: vec![ContentItem::InputText { - text: "\nbody\n" + text: "\ndemo\nskills/demo/SKILL.md\nbody\n" .to_string(), }], }, diff --git a/codex-rs/core/src/skills/injection.rs b/codex-rs/core/src/skills/injection.rs new file mode 100644 index 00000000000..a143fce1f22 --- /dev/null +++ b/codex-rs/core/src/skills/injection.rs @@ -0,0 +1,78 @@ +use std::collections::HashSet; + +use crate::skills::SkillLoadOutcome; +use crate::skills::SkillMetadata; +use crate::user_instructions::SkillInstructions; +use codex_protocol::models::ResponseItem; +use codex_protocol::user_input::UserInput; +use tokio::fs; + +#[derive(Debug, Default)] +pub(crate) struct SkillInjections { + pub(crate) items: Vec, + pub(crate) warnings: Vec, +} + +pub(crate) async fn build_skill_injections( + inputs: &[UserInput], + skills: Option<&SkillLoadOutcome>, +) -> SkillInjections { + if inputs.is_empty() { + return SkillInjections::default(); + } + + let Some(outcome) = skills else { + return SkillInjections::default(); + }; + + let mentioned_skills = collect_explicit_skill_mentions(inputs, &outcome.skills); + if mentioned_skills.is_empty() { + return SkillInjections::default(); + } + + let mut result = SkillInjections { + items: Vec::with_capacity(mentioned_skills.len()), + warnings: Vec::new(), + }; + + for skill in mentioned_skills { + match fs::read_to_string(&skill.path).await { + Ok(contents) => { + result.items.push(ResponseItem::from(SkillInstructions { + name: skill.name, + path: skill.path.to_string_lossy().into_owned(), + contents, + })); + } + Err(err) => { + let message = format!( + "Failed to load skill {} at {}: {err:#}", + skill.name, + skill.path.display() + ); + result.warnings.push(message); + } + } + } + + result +} + +fn collect_explicit_skill_mentions( + inputs: &[UserInput], + skills: &[SkillMetadata], +) -> Vec { + let mut selected: Vec = Vec::new(); + let mut seen: HashSet = HashSet::new(); + + for input in inputs { + if let UserInput::Skill { name, path } = input + && seen.insert(name.clone()) + && let Some(skill) = skills.iter().find(|s| s.name == *name && s.path == *path) + { + selected.push(skill.clone()); + } + } + + selected +} diff --git a/codex-rs/core/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs index ebb1490c99f..b2ab935ce53 100644 --- a/codex-rs/core/src/skills/mod.rs +++ b/codex-rs/core/src/skills/mod.rs @@ -1,7 +1,10 @@ +pub mod injection; pub mod loader; pub mod model; pub mod render; +pub(crate) use injection::SkillInjections; +pub(crate) use injection::build_skill_injections; pub use loader::load_skills; pub use model::SkillError; pub use model::SkillLoadOutcome; diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 7387bcedae0..0270f3411c8 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -4,6 +4,7 @@ use crate::AuthManager; use crate::RolloutRecorder; use crate::mcp_connection_manager::McpConnectionManager; use crate::openai_models::models_manager::ModelsManager; +use crate::skills::SkillLoadOutcome; use crate::tools::sandboxing::ApprovalStore; use crate::unified_exec::UnifiedExecSessionManager; use crate::user_notification::UserNotifier; @@ -24,4 +25,5 @@ pub(crate) struct SessionServices { pub(crate) models_manager: Arc, pub(crate) otel_event_manager: OtelEventManager, pub(crate) tool_approvals: Mutex, + pub(crate) skills: Option, } diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index 35b7b6c63ec..c61d1883735 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -7,7 +7,6 @@ use crate::context_manager::ContextManager; use crate::protocol::RateLimitSnapshot; use crate::protocol::TokenUsage; use crate::protocol::TokenUsageInfo; -use crate::skills::SkillLoadOutcome; use crate::truncate::TruncationPolicy; /// Persistent, session-scoped state previously stored directly on `Session`. @@ -15,21 +14,16 @@ pub(crate) struct SessionState { pub(crate) session_configuration: SessionConfiguration, pub(crate) history: ContextManager, pub(crate) latest_rate_limits: Option, - pub(crate) skills: Option, } impl SessionState { /// Create a new session state mirroring previous `State::default()` semantics. - pub(crate) fn new( - session_configuration: SessionConfiguration, - skills: Option, - ) -> Self { + pub(crate) fn new(session_configuration: SessionConfiguration) -> Self { let history = ContextManager::new(); Self { session_configuration, history, latest_rate_limits: None, - skills, } } diff --git a/codex-rs/core/src/user_instructions.rs b/codex-rs/core/src/user_instructions.rs index 1c4fcf666fd..22b5ad7bbe5 100644 --- a/codex-rs/core/src/user_instructions.rs +++ b/codex-rs/core/src/user_instructions.rs @@ -6,7 +6,7 @@ use codex_protocol::models::ResponseItem; pub const USER_INSTRUCTIONS_OPEN_TAG_LEGACY: &str = ""; pub const USER_INSTRUCTIONS_PREFIX: &str = "# AGENTS.md instructions for "; -pub const SKILL_INSTRUCTIONS_PREFIX: &str = " for ResponseItem { role: "user".to_string(), content: vec![ContentItem::InputText { text: format!( - "\n{contents}\n", - name = si.name, - path = si.path, - contents = si.contents + "\n{}\n{}\n{}\n", + si.name, si.path, si.contents ), }], } @@ -174,7 +172,7 @@ mod tests { assert_eq!( text, - "\nbody\n", + "\ndemo-skill\nskills/demo/SKILL.md\nbody\n", ); } @@ -182,7 +180,7 @@ mod tests { fn test_is_skill_instructions() { assert!(SkillInstructions::is_skill_instructions(&[ ContentItem::InputText { - text: "\nbody\n" + text: "\ndemo-skill\nskills/demo/SKILL.md\nbody\n" .to_string(), } ])); diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index 5c738246bb9..3a5eed8e242 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -236,33 +236,6 @@ impl TestCodex { .await; Ok(()) } - - pub async fn submit_items( - &self, - items: Vec, - approval_policy: AskForApproval, - sandbox_policy: SandboxPolicy, - ) -> Result<()> { - let session_model = self.session_configured.model.clone(); - self.codex - .submit(Op::UserTurn { - items, - final_output_json_schema: None, - cwd: self.cwd.path().to_path_buf(), - approval_policy, - sandbox_policy, - model: session_model, - effort: None, - summary: ReasoningSummary::Auto, - }) - .await?; - - wait_for_event(&self.codex, |event| { - matches!(event, EventMsg::TaskComplete(_)) - }) - .await; - Ok(()) - } } pub struct TestCodexHarness { diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 1fe8c9f7038..d6ced3c1dc3 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -4,7 +4,9 @@ use anyhow::Result; use codex_core::features::Feature; use codex_core::protocol::AskForApproval; +use codex_core::protocol::Op; use codex_core::protocol::SandboxPolicy; +use codex_core::protocol::SkillLoadOutcomeInfo; use codex_protocol::user_input::UserInput; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -42,6 +44,7 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { let test = builder.build(&server).await?; let skill_path = test.codex_home_path().join("skills/demo/SKILL.md"); + let skill_path = std::fs::canonicalize(skill_path)?; let mock = mount_sse_once( &server, @@ -53,27 +56,40 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { ) .await; - test.submit_items( - vec![ - UserInput::Text { - text: "please use $demo".to_string(), - }, - UserInput::Skill { - name: "demo".to_string(), - path: skill_path.clone(), - }, - ], - AskForApproval::Never, - SandboxPolicy::DangerFullAccess, - ) - .await?; + let session_model = test.session_configured.model.clone(); + test.codex + .submit(Op::UserTurn { + items: vec![ + UserInput::Text { + text: "please use $demo".to_string(), + }, + UserInput::Skill { + name: "demo".to_string(), + path: skill_path.clone(), + }, + ], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: session_model, + effort: None, + summary: codex_protocol::config_types::ReasoningSummary::Auto, + }) + .await?; + + core_test_support::wait_for_event(test.codex.as_ref(), |event| { + matches!(event, codex_core::protocol::EventMsg::TaskComplete(_)) + }) + .await; let request = mock.single_request(); let user_texts = request.message_input_texts("user"); let skill_path_str = skill_path.to_string_lossy(); assert!( user_texts.iter().any(|text| { - text.contains("\ndemo") + && text.contains("") && text.contains(skill_body) && text.contains(skill_path_str.as_ref()) }), @@ -82,3 +98,39 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn skill_load_errors_surface_in_session_configured() -> 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| { + let skill_dir = home.join("skills").join("broken"); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write(skill_dir.join("SKILL.md"), "not yaml").unwrap(); + }); + let test = builder.build(&server).await?; + + let SkillLoadOutcomeInfo { skills, errors } = test + .session_configured + .skill_load_outcome + .as_ref() + .expect("skill outcome present"); + + assert!( + skills.is_empty(), + "expected no skills loaded, got {skills:?}" + ); + assert_eq!(errors.len(), 1, "expected one load error"); + let error_path = errors[0].path.to_string_lossy(); + assert!( + error_path.ends_with("skills/broken/SKILL.md"), + "unexpected error path: {error_path}" + ); + + Ok(()) +} diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 06f9eb9828d..1dfb268a233 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -281,39 +281,37 @@ impl From> for ResponseInputItem { role: "user".to_string(), content: items .into_iter() - .map(|c| match c { - UserInput::Text { text } => ContentItem::InputText { text }, - UserInput::Image { image_url } => ContentItem::InputImage { image_url }, + .filter_map(|c| match c { + UserInput::Text { text } => Some(ContentItem::InputText { text }), + UserInput::Image { image_url } => Some(ContentItem::InputImage { image_url }), UserInput::LocalImage { path } => match load_and_resize_to_fit(&path) { - Ok(image) => ContentItem::InputImage { + Ok(image) => Some(ContentItem::InputImage { image_url: image.into_data_url(), - }, + }), Err(err) => { if matches!(&err, ImageProcessingError::Read { .. }) { - local_image_error_placeholder(&path, &err) + Some(local_image_error_placeholder(&path, &err)) } else if err.is_invalid_image() { - invalid_image_error_placeholder(&path, &err) + Some(invalid_image_error_placeholder(&path, &err)) } else { let Some(mime_guess) = mime_guess::from_path(&path).first() else { - return local_image_error_placeholder( + return Some(local_image_error_placeholder( &path, "unsupported MIME type (unknown)", - ); + )); }; let mime = mime_guess.essence_str().to_owned(); if !mime.starts_with("image/") { - return local_image_error_placeholder( + return Some(local_image_error_placeholder( &path, format!("unsupported MIME type `{mime}`"), - ); + )); } - unsupported_image_error_placeholder(&path, &mime) + Some(unsupported_image_error_placeholder(&path, &mime)) } } }, - UserInput::Skill { .. } => ContentItem::InputText { - text: String::new(), - }, + UserInput::Skill { .. } => None, // Skill bodies are injected later in core }) .collect::>(), }