Skip to content

Commit 5f7a5d1

Browse files
xl-openaixl-openai
committed
address comments.
1 parent 85ff32b commit 5f7a5d1

File tree

10 files changed

+194
-175
lines changed

10 files changed

+194
-175
lines changed

‎codex-rs/core/src/codex.rs‎

Lines changed: 24 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
use std::collections::HashMap;
2-
use std::collections::HashSet;
32
use std::fmt::Debug;
4-
use std::path::Path;
53
use std::path::PathBuf;
64
use std::sync::Arc;
75
use std::sync::atomic::AtomicU64;
@@ -57,7 +55,6 @@ use mcp_types::ReadResourceResult;
5755
use mcp_types::RequestId;
5856
use serde_json;
5957
use serde_json::Value;
60-
use tokio::fs;
6158
use tokio::sync::Mutex;
6259
use tokio::sync::RwLock;
6360
use tokio::sync::oneshot;
@@ -116,8 +113,9 @@ use crate::rollout::RolloutRecorderParams;
116113
use crate::rollout::map_session_init_error;
117114
use crate::shell;
118115
use crate::shell_snapshot::ShellSnapshot;
116+
use crate::skills::SkillInjections;
119117
use crate::skills::SkillLoadOutcome;
120-
use crate::skills::SkillMetadata;
118+
use crate::skills::build_skill_injections;
121119
use crate::skills::load_skills;
122120
use crate::state::ActiveTurn;
123121
use crate::state::SessionServices;
@@ -135,7 +133,6 @@ use crate::tools::spec::ToolsConfigParams;
135133
use crate::turn_diff_tracker::TurnDiffTracker;
136134
use crate::unified_exec::UnifiedExecSessionManager;
137135
use crate::user_instructions::DeveloperInstructions;
138-
use crate::user_instructions::SkillInstructions;
139136
use crate::user_instructions::UserInstructions;
140137
use crate::user_notification::UserNotification;
141138
use crate::util::backoff;
@@ -204,12 +201,9 @@ impl Codex {
204201

205202
let user_instructions = get_user_instructions(
206203
&config,
207-
skills_outcome.as_ref().and_then(|outcome| {
208-
outcome
209-
.errors
210-
.is_empty()
211-
.then_some(outcome.skills.as_slice())
212-
}),
204+
skills_outcome
205+
.as_ref()
206+
.map(|outcome| outcome.skills.as_slice()),
213207
)
214208
.await;
215209

@@ -621,7 +615,7 @@ impl Session {
621615
.await
622616
.map(Arc::new);
623617
}
624-
let state = SessionState::new(session_configuration.clone(), skills.clone());
618+
let state = SessionState::new(session_configuration.clone());
625619

626620
let services = SessionServices {
627621
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())),
@@ -635,6 +629,7 @@ impl Session {
635629
otel_event_manager,
636630
models_manager: Arc::clone(&models_manager),
637631
tool_approvals: Mutex::new(ApprovalStore::default()),
632+
skills: skills.clone(),
638633
};
639634

640635
let sess = Arc::new(Session {
@@ -1345,54 +1340,6 @@ impl Session {
13451340
}
13461341
}
13471342

1348-
async fn inject_skills(
1349-
&self,
1350-
turn_context: &TurnContext,
1351-
user_input: &[UserInput],
1352-
) -> Vec<ResponseItem> {
1353-
if user_input.is_empty() || !self.enabled(Feature::Skills) {
1354-
return Vec::new();
1355-
}
1356-
1357-
let skills = {
1358-
let state = self.state.lock().await;
1359-
state
1360-
.skills
1361-
.as_ref()
1362-
.map(|outcome| outcome.skills.clone())
1363-
.unwrap_or_default()
1364-
};
1365-
1366-
let mentioned_skills = collect_explicit_skill_mentions(user_input, &skills);
1367-
if mentioned_skills.is_empty() {
1368-
return Vec::new();
1369-
}
1370-
1371-
let mut injections: Vec<ResponseItem> = Vec::with_capacity(mentioned_skills.len());
1372-
for skill in mentioned_skills {
1373-
match fs::read_to_string(&skill.path).await {
1374-
Ok(contents) => {
1375-
injections.push(ResponseItem::from(SkillInstructions {
1376-
name: skill.name,
1377-
path: skill.path.to_string_lossy().into_owned(),
1378-
contents,
1379-
}));
1380-
}
1381-
Err(err) => {
1382-
let message = format!(
1383-
"Failed to load skill {} at {}: {err:#}",
1384-
skill.name,
1385-
skill.path.display()
1386-
);
1387-
self.send_event(turn_context, EventMsg::Warning(WarningEvent { message }))
1388-
.await;
1389-
}
1390-
}
1391-
}
1392-
1393-
injections
1394-
}
1395-
13961343
pub(crate) async fn notify_background_event(
13971344
&self,
13981345
turn_context: &TurnContext,
@@ -2108,42 +2055,6 @@ async fn spawn_review_thread(
21082055
.await;
21092056
}
21102057

2111-
fn collect_explicit_skill_mentions(
2112-
inputs: &[UserInput],
2113-
skills: &[SkillMetadata],
2114-
) -> Vec<SkillMetadata> {
2115-
let mut selected: Vec<SkillMetadata> = Vec::new();
2116-
let mut seen: HashSet<String> = HashSet::new();
2117-
2118-
for input in inputs {
2119-
if let UserInput::Skill { name, path } = input
2120-
&& seen.insert(name.clone())
2121-
&& let Some(skill) = skills
2122-
.iter()
2123-
.find(|s| s.name == *name && paths_match(&s.path, path))
2124-
{
2125-
selected.push(skill.clone());
2126-
}
2127-
}
2128-
2129-
selected
2130-
}
2131-
2132-
fn paths_match(a: &Path, b: &Path) -> bool {
2133-
if a == b {
2134-
return true;
2135-
}
2136-
2137-
let Ok(ca) = std::fs::canonicalize(a) else {
2138-
return false;
2139-
};
2140-
let Ok(cb) = std::fs::canonicalize(b) else {
2141-
return false;
2142-
};
2143-
2144-
ca == cb
2145-
}
2146-
21472058
fn skill_load_outcome_for_client(
21482059
outcome: Option<&SkillLoadOutcome>,
21492060
) -> Option<SkillLoadOutcomeInfo> {
@@ -2196,15 +2107,23 @@ pub(crate) async fn run_task(
21962107
});
21972108
sess.send_event(&turn_context, event).await;
21982109

2199-
let skill_injections = sess.inject_skills(&turn_context, &input).await;
2110+
let SkillInjections {
2111+
items: skill_items,
2112+
warnings: skill_warnings,
2113+
} = build_skill_injections(&input, sess.services.skills.as_ref()).await;
2114+
2115+
for message in skill_warnings {
2116+
sess.send_event(&turn_context, EventMsg::Warning(WarningEvent { message }))
2117+
.await;
2118+
}
22002119

22012120
let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input);
22022121
let response_item: ResponseItem = initial_input_for_turn.clone().into();
22032122
sess.record_response_item_and_emit_turn_item(turn_context.as_ref(), response_item)
22042123
.await;
22052124

2206-
if !skill_injections.is_empty() {
2207-
sess.record_conversation_items(&turn_context, &skill_injections)
2125+
if !skill_items.is_empty() {
2126+
sess.record_conversation_items(&turn_context, &skill_items)
22082127
.await;
22092128
}
22102129

@@ -2761,7 +2680,7 @@ mod tests {
27612680
session_source: SessionSource::Exec,
27622681
};
27632682

2764-
let mut state = SessionState::new(session_configuration, None);
2683+
let mut state = SessionState::new(session_configuration);
27652684
let initial = RateLimitSnapshot {
27662685
primary: Some(RateLimitWindow {
27672686
used_percent: 10.0,
@@ -2832,7 +2751,7 @@ mod tests {
28322751
session_source: SessionSource::Exec,
28332752
};
28342753

2835-
let mut state = SessionState::new(session_configuration, None);
2754+
let mut state = SessionState::new(session_configuration);
28362755
let initial = RateLimitSnapshot {
28372756
primary: Some(RateLimitWindow {
28382757
used_percent: 15.0,
@@ -3038,7 +2957,7 @@ mod tests {
30382957
let otel_event_manager =
30392958
otel_event_manager(conversation_id, config.as_ref(), &model_family);
30402959

3041-
let state = SessionState::new(session_configuration.clone(), None);
2960+
let state = SessionState::new(session_configuration.clone());
30422961

30432962
let services = SessionServices {
30442963
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())),
@@ -3052,6 +2971,7 @@ mod tests {
30522971
otel_event_manager: otel_event_manager.clone(),
30532972
models_manager,
30542973
tool_approvals: Mutex::new(ApprovalStore::default()),
2974+
skills: None,
30552975
};
30562976

30572977
let turn_context = Session::make_turn_context(
@@ -3120,7 +3040,7 @@ mod tests {
31203040
let otel_event_manager =
31213041
otel_event_manager(conversation_id, config.as_ref(), &model_family);
31223042

3123-
let state = SessionState::new(session_configuration.clone(), None);
3043+
let state = SessionState::new(session_configuration.clone());
31243044

31253045
let services = SessionServices {
31263046
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())),
@@ -3134,6 +3054,7 @@ mod tests {
31343054
otel_event_manager: otel_event_manager.clone(),
31353055
models_manager,
31363056
tool_approvals: Mutex::new(ApprovalStore::default()),
3057+
skills: None,
31373058
};
31383059

31393060
let turn_context = Arc::new(Session::make_turn_context(

‎codex-rs/core/src/event_mapping.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ mod tests {
205205
id: None,
206206
role: "user".to_string(),
207207
content: vec![ContentItem::InputText {
208-
text: "<SKILL name=\"demo\" path=\"skills/demo/SKILL.md\">\nbody\n</SKILL>"
208+
text: "<skill>\n<name>demo</name>\n<path>skills/demo/SKILL.md</path>\nbody\n</skill>"
209209
.to_string(),
210210
}],
211211
},

‎codex-rs/core/src/skills/injection.rs‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
use std::collections::HashSet;
2+
3+
use crate::skills::SkillLoadOutcome;
4+
use crate::skills::SkillMetadata;
5+
use crate::user_instructions::SkillInstructions;
6+
use codex_protocol::models::ResponseItem;
7+
use codex_protocol::user_input::UserInput;
8+
use tokio::fs;
9+
10+
#[derive(Debug, Default)]
11+
pub(crate) struct SkillInjections {
12+
pub(crate) items: Vec<ResponseItem>,
13+
pub(crate) warnings: Vec<String>,
14+
}
15+
16+
pub(crate) async fn build_skill_injections(
17+
inputs: &[UserInput],
18+
skills: Option<&SkillLoadOutcome>,
19+
) -> SkillInjections {
20+
if inputs.is_empty() {
21+
return SkillInjections::default();
22+
}
23+
24+
let Some(outcome) = skills else {
25+
return SkillInjections::default();
26+
};
27+
28+
let mentioned_skills = collect_explicit_skill_mentions(inputs, &outcome.skills);
29+
if mentioned_skills.is_empty() {
30+
return SkillInjections::default();
31+
}
32+
33+
let mut result = SkillInjections {
34+
items: Vec::with_capacity(mentioned_skills.len()),
35+
warnings: Vec::new(),
36+
};
37+
38+
for skill in mentioned_skills {
39+
match fs::read_to_string(&skill.path).await {
40+
Ok(contents) => {
41+
result.items.push(ResponseItem::from(SkillInstructions {
42+
name: skill.name,
43+
path: skill.path.to_string_lossy().into_owned(),
44+
contents,
45+
}));
46+
}
47+
Err(err) => {
48+
let message = format!(
49+
"Failed to load skill {} at {}: {err:#}",
50+
skill.name,
51+
skill.path.display()
52+
);
53+
result.warnings.push(message);
54+
}
55+
}
56+
}
57+
58+
result
59+
}
60+
61+
fn collect_explicit_skill_mentions(
62+
inputs: &[UserInput],
63+
skills: &[SkillMetadata],
64+
) -> Vec<SkillMetadata> {
65+
let mut selected: Vec<SkillMetadata> = Vec::new();
66+
let mut seen: HashSet<String> = HashSet::new();
67+
68+
for input in inputs {
69+
if let UserInput::Skill { name, path } = input
70+
&& seen.insert(name.clone())
71+
&& let Some(skill) = skills.iter().find(|s| s.name == *name && s.path == *path)
72+
{
73+
selected.push(skill.clone());
74+
}
75+
}
76+
77+
selected
78+
}

‎codex-rs/core/src/skills/mod.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
1+
pub mod injection;
12
pub mod loader;
23
pub mod model;
34
pub mod render;
45

6+
pub(crate) use injection::SkillInjections;
7+
pub(crate) use injection::build_skill_injections;
58
pub use loader::load_skills;
69
pub use model::SkillError;
710
pub use model::SkillLoadOutcome;

‎codex-rs/core/src/state/service.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use crate::AuthManager;
44
use crate::RolloutRecorder;
55
use crate::mcp_connection_manager::McpConnectionManager;
66
use crate::openai_models::models_manager::ModelsManager;
7+
use crate::skills::SkillLoadOutcome;
78
use crate::tools::sandboxing::ApprovalStore;
89
use crate::unified_exec::UnifiedExecSessionManager;
910
use crate::user_notification::UserNotifier;
@@ -24,4 +25,5 @@ pub(crate) struct SessionServices {
2425
pub(crate) models_manager: Arc<ModelsManager>,
2526
pub(crate) otel_event_manager: OtelEventManager,
2627
pub(crate) tool_approvals: Mutex<ApprovalStore>,
28+
pub(crate) skills: Option<SkillLoadOutcome>,
2729
}

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

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,29 +7,23 @@ use crate::context_manager::ContextManager;
77
use crate::protocol::RateLimitSnapshot;
88
use crate::protocol::TokenUsage;
99
use crate::protocol::TokenUsageInfo;
10-
use crate::skills::SkillLoadOutcome;
1110
use crate::truncate::TruncationPolicy;
1211

1312
/// Persistent, session-scoped state previously stored directly on `Session`.
1413
pub(crate) struct SessionState {
1514
pub(crate) session_configuration: SessionConfiguration,
1615
pub(crate) history: ContextManager,
1716
pub(crate) latest_rate_limits: Option<RateLimitSnapshot>,
18-
pub(crate) skills: Option<SkillLoadOutcome>,
1917
}
2018

2119
impl SessionState {
2220
/// Create a new session state mirroring previous `State::default()` semantics.
23-
pub(crate) fn new(
24-
session_configuration: SessionConfiguration,
25-
skills: Option<SkillLoadOutcome>,
26-
) -> Self {
21+
pub(crate) fn new(session_configuration: SessionConfiguration) -> Self {
2722
let history = ContextManager::new();
2823
Self {
2924
session_configuration,
3025
history,
3126
latest_rate_limits: None,
32-
skills,
3327
}
3428
}
3529

0 commit comments

Comments
 (0)