update skill loading + add tests.

Author: xl-openai

codex-rs/core/src/codex.rs

@@ -100,6 +100,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 +114,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 +199,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 +238,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 +247,7 @@ impl Codex {
             tx_event.clone(),
             conversation_history,
             session_source_clone,
-            skills_cache,
+            skills_outcome.clone(),
         )
         .await
         .map_err(|e| {
@@ -501,7 +513,7 @@ impl Session {
         tx_event: Sender<Event>,
         initial_history: InitialHistory,
         session_source: SessionSource,
-        loaded_skills: Option<Vec<SkillMetadata>>,
+        skills: Option<SkillLoadOutcome>,
     ) -> anyhow::Result<Arc<Self>> {
         debug!(
             "Configuring session: model={}; provider={:?}",
@@ -608,7 +620,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 +649,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 +664,7 @@ impl Session {
                 history_log_id,
                 history_entry_count,
                 initial_messages,
+                skill_load_outcome,
                 rollout_path,
             }),
         })
@@ -1341,7 +1355,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 +2111,43 @@ fn collect_explicit_skill_mentions(
     inputs: &[UserInput],
     skills: &[SkillMetadata],
 ) -> Vec<SkillMetadata> {
-    let mut full_text: Vec<String> = Vec::new();
-    for input in inputs {
-        if let UserInput::Text { text } = input {
-            full_text.push(text.clone());
-        }
-    }
-    let combined = full_text.join(" ");
+    let mut selected: Vec<SkillMetadata> = Vec::new();
     let mut seen: HashSet<String> = HashSet::new();
-    let mut matches: Vec<SkillMetadata> = 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());
+    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());
         }
     }
 
-    matches
+    selected
+}
+
+fn skill_load_outcome_for_client(
+    outcome: Option<&SkillLoadOutcome>,
+) -> Option<SkillLoadOutcomeInfo> {
+    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

codex-rs/core/src/event_mapping.rs

@@ -201,14 +201,22 @@ mod tests {
                     text: "# AGENTS.md instructions for test_directory\n\n<INSTRUCTIONS>\ntest_text\n</INSTRUCTIONS>".to_string(),
                 }],
             },
-        ResponseItem::Message {
-            id: None,
-            role: "user".to_string(),
-            content: vec![ContentItem::InputText {
-                text: "<user_shell_command>echo 42</user_shell_command>".to_string(),
-            }],
-        },
-    ];
+            ResponseItem::Message {
+                id: None,
+                role: "user".to_string(),
+                content: vec![ContentItem::InputText {
+                    text: "<SKILL name=\"demo\" path=\"skills/demo/SKILL.md\">\nbody\n</SKILL>"
+                        .to_string(),
+                }],
+            },
+            ResponseItem::Message {
+                id: None,
+                role: "user".to_string(),
+                content: vec![ContentItem::InputText {
+                    text: "<user_shell_command>echo 42</user_shell_command>".to_string(),
+                }],
+            },
+        ];
 
         for item in items {
             let turn_item = parse_turn_item(&item);

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"));

codex-rs/core/src/state/session.rs

@@ -7,22 +7,22 @@ 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`.
 pub(crate) struct SessionState {
     pub(crate) session_configuration: SessionConfiguration,
     pub(crate) history: ContextManager,
     pub(crate) latest_rate_limits: Option<RateLimitSnapshot>,
-    pub(crate) skills: Option<Vec<SkillMetadata>>,
+    pub(crate) skills: Option<SkillLoadOutcome>,
 }
 
 impl SessionState {
     /// Create a new session state mirroring previous `State::default()` semantics.
     pub(crate) fn new(
         session_configuration: SessionConfiguration,
-        skills: Option<Vec<SkillMetadata>>,
+        skills: Option<SkillLoadOutcome>,
     ) -> Self {
         let history = ContextManager::new();
         Self {

codex-rs/core/src/user_instructions.rs

@@ -6,7 +6,7 @@ use codex_protocol::models::ResponseItem;
 
 pub const USER_INSTRUCTIONS_OPEN_TAG_LEGACY: &str = "<user_instructions>";
 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 = "<SKILL";
 
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
 #[serde(rename = "user_instructions", rename_all = "snake_case")]
@@ -67,7 +67,7 @@ impl From<SkillInstructions> for ResponseItem {
             role: "user".to_string(),
             content: vec![ContentItem::InputText {
                 text: format!(
-                    "{SKILL_INSTRUCTIONS_PREFIX}{name}\nPath: {path}\n\n<SKILL>\n{contents}\n</SKILL>",
+                    "<SKILL name=\"{name}\" path=\"{path}\">\n{contents}\n</SKILL>",
                     name = si.name,
                     path = si.path,
                     contents = si.contents
@@ -108,6 +108,7 @@ impl From<DeveloperInstructions> 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,
+            "<SKILL name=\"demo-skill\" path=\"skills/demo/SKILL.md\">\nbody\n</SKILL>",
+        );
+    }
+
+    #[test]
+    fn test_is_skill_instructions() {
+        assert!(SkillInstructions::is_skill_instructions(&[
+            ContentItem::InputText {
+                text: "<SKILL name=\"demo-skill\" path=\"skills/demo/SKILL.md\">\nbody\n</SKILL>"
+                    .to_string(),
+            }
+        ]));
+        assert!(!SkillInstructions::is_skill_instructions(&[
+            ContentItem::InputText {
+                text: "regular text".to_string(),
+            }
+        ]));
+    }
 }