address comments.

Author: xl-openai

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;
@@ -621,7 +618,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 +632,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 +1343,6 @@ impl Session {
         }
     }
 
-    async fn inject_skills(
-        &self,
-        turn_context: &TurnContext,
-        user_input: &[UserInput],
-    ) -> Vec<ResponseItem> {
-        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<ResponseItem> = 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 +2058,6 @@ async fn spawn_review_thread(
         .await;
 }
 
-fn collect_explicit_skill_mentions(
-    inputs: &[UserInput],
-    skills: &[SkillMetadata],
-) -> Vec<SkillMetadata> {
-    let mut selected: Vec<SkillMetadata> = Vec::new();
-    let mut seen: HashSet<String> = 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<SkillLoadOutcomeInfo> {
@@ -2196,16 +2110,21 @@ 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, warnings } =
+        build_skill_injections(&input, sess.services.skills.as_ref()).await;
+
+    for message in 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)
-            .await;
+    if !items.is_empty() {
+        sess.record_conversation_items(&turn_context, &items).await;
     }
 
     sess.maybe_start_ghost_snapshot(Arc::clone(&turn_context), cancellation_token.child_token())
@@ -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(

codex-rs/core/src/event_mapping.rs

@@ -205,7 +205,7 @@ mod tests {
                 id: None,
                 role: "user".to_string(),
                 content: vec![ContentItem::InputText {
-                    text: "<SKILL name=\"demo\" path=\"skills/demo/SKILL.md\">\nbody\n</SKILL>"
+                    text: "<skill>\n<name>demo</name>\n<path>skills/demo/SKILL.md</path>\nbody\n</skill>"
                         .to_string(),
                 }],
             },

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<ResponseItem>,
+    pub(crate) warnings: Vec<String>,
+}
+
+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<SkillMetadata> {
+    let mut selected: Vec<SkillMetadata> = Vec::new();
+    let mut seen: HashSet<String> = 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
+}

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;

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<ModelsManager>,
     pub(crate) otel_event_manager: OtelEventManager,
     pub(crate) tool_approvals: Mutex<ApprovalStore>,
+    pub(crate) skills: Option<SkillLoadOutcome>,
 }

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

@@ -7,29 +7,23 @@ 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`.
 pub(crate) struct SessionState {
     pub(crate) session_configuration: SessionConfiguration,
     pub(crate) history: ContextManager,
     pub(crate) latest_rate_limits: Option<RateLimitSnapshot>,
-    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<SkillLoadOutcome>,
-    ) -> Self {
+    pub(crate) fn new(session_configuration: SessionConfiguration) -> Self {
         let history = ContextManager::new();
         Self {
             session_configuration,
             history,
             latest_rate_limits: None,
-            skills,
         }
     }