Reimplement skills loading using SkillsManager + skills/list op. (#7914)

refactor the way we load and manage skills:
1. Move skill discovery/caching into SkillsManager and reuse it across
sessions.
2. Add the skills/list API (Op::ListSkills/SkillsListResponse) to fetch
skills for one or more cwds. Also update app-server for VSCE/App;
3. Trigger skills/list during session startup so UIs preload skills and
handle errors immediately.
This commit is contained in:
xl-openai
2025-12-14 09:58:17 -08:00
committed by GitHub
parent a2c86e5d88
commit 5d77d4db6b
29 changed files with 579 additions and 137 deletions
+13
View File
@@ -64,6 +64,7 @@ const REFRESH_TOKEN_UNKNOWN_MESSAGE: &str =
const REFRESH_TOKEN_URL: &str = "https://auth.openai.com/oauth/token";
pub const REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR: &str = "CODEX_REFRESH_TOKEN_URL_OVERRIDE";
#[cfg(any(test, feature = "test-support"))]
static TEST_AUTH_TEMP_DIRS: Lazy<Mutex<Vec<TempDir>>> = Lazy::new(|| Mutex::new(Vec::new()));
#[derive(Debug, Error)]
@@ -1111,6 +1112,18 @@ impl AuthManager {
})
}
#[cfg(any(test, feature = "test-support"))]
/// Create an AuthManager with a specific CodexAuth and codex home, for testing only.
pub fn from_auth_for_testing_with_home(auth: CodexAuth, codex_home: PathBuf) -> Arc<Self> {
let cached = CachedAuth { auth: Some(auth) };
Arc::new(Self {
codex_home,
inner: RwLock::new(cached),
enable_codex_api_key_env: false,
auth_credentials_store_mode: AuthCredentialsStoreMode::File,
})
}
/// Current cached auth (clone). May be `None` if not logged in or load failed.
pub fn auth(&self) -> Option<CodexAuth> {
self.inner.read().ok().and_then(|c| c.auth.clone())
+89 -39
View File
@@ -106,8 +106,7 @@ use crate::protocol::ReviewDecision;
use crate::protocol::SandboxPolicy;
use crate::protocol::SessionConfiguredEvent;
use crate::protocol::SkillErrorInfo;
use crate::protocol::SkillInfo;
use crate::protocol::SkillLoadOutcomeInfo;
use crate::protocol::SkillMetadata as ProtocolSkillMetadata;
use crate::protocol::StreamErrorEvent;
use crate::protocol::Submission;
use crate::protocol::TokenCountEvent;
@@ -120,10 +119,11 @@ use crate::rollout::RolloutRecorderParams;
use crate::rollout::map_session_init_error;
use crate::shell;
use crate::shell_snapshot::ShellSnapshot;
use crate::skills::SkillError;
use crate::skills::SkillInjections;
use crate::skills::SkillLoadOutcome;
use crate::skills::SkillMetadata;
use crate::skills::SkillsManager;
use crate::skills::build_skill_injections;
use crate::skills::load_skills;
use crate::state::ActiveTurn;
use crate::state::SessionServices;
use crate::state::SessionState;
@@ -207,6 +207,7 @@ impl Codex {
config: Config,
auth_manager: Arc<AuthManager>,
models_manager: Arc<ModelsManager>,
skills_manager: Arc<SkillsManager>,
conversation_history: InitialHistory,
session_source: SessionSource,
) -> CodexResult<CodexSpawnOk> {
@@ -214,7 +215,7 @@ impl Codex {
let (tx_event, rx_event) = async_channel::unbounded();
let loaded_skills = if config.features.enabled(Feature::Skills) {
Some(load_skills(&config))
Some(skills_manager.skills_for_cwd(&config.cwd))
} else {
None
};
@@ -229,11 +230,9 @@ impl Codex {
}
}
let skills_outcome = loaded_skills.clone();
let user_instructions = get_user_instructions(
&config,
skills_outcome
loaded_skills
.as_ref()
.map(|outcome| outcome.skills.as_slice()),
)
@@ -279,7 +278,7 @@ impl Codex {
tx_event.clone(),
conversation_history,
session_source_clone,
skills_outcome.clone(),
skills_manager,
)
.await
.map_err(|e| {
@@ -546,7 +545,7 @@ impl Session {
tx_event: Sender<Event>,
initial_history: InitialHistory,
session_source: SessionSource,
skills: Option<SkillLoadOutcome>,
skills_manager: Arc<SkillsManager>,
) -> anyhow::Result<Arc<Self>> {
debug!(
"Configuring session: model={}; provider={:?}",
@@ -666,7 +665,7 @@ impl Session {
otel_manager,
models_manager: Arc::clone(&models_manager),
tool_approvals: Mutex::new(ApprovalStore::default()),
skills: skills.clone(),
skills_manager,
};
let sess = Arc::new(Session {
@@ -682,8 +681,6 @@ 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(),
msg: EventMsg::SessionConfigured(SessionConfiguredEvent {
@@ -697,7 +694,6 @@ impl Session {
history_log_id,
history_entry_count,
initial_messages,
skill_load_outcome,
rollout_path,
}),
})
@@ -1585,6 +1581,9 @@ async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiv
Op::ListCustomPrompts => {
handlers::list_custom_prompts(&sess, sub.id.clone()).await;
}
Op::ListSkills { cwds } => {
handlers::list_skills(&sess, sub.id.clone(), cwds).await;
}
Op::Undo => {
handlers::undo(&sess, sub.id.clone()).await;
}
@@ -1629,6 +1628,7 @@ mod handlers {
use crate::codex::spawn_review_thread;
use crate::config::Config;
use crate::features::Feature;
use crate::mcp::auth::compute_auth_statuses;
use crate::mcp::collect_mcp_snapshot_from_manager;
use crate::review_prompts::resolve_review_request;
@@ -1642,9 +1642,11 @@ mod handlers {
use codex_protocol::protocol::Event;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ListCustomPromptsResponseEvent;
use codex_protocol::protocol::ListSkillsResponseEvent;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::ReviewRequest;
use codex_protocol::protocol::SkillsListEntry;
use codex_protocol::protocol::TurnAbortReason;
use codex_protocol::protocol::WarningEvent;
@@ -1652,6 +1654,7 @@ mod handlers {
use codex_rmcp_client::ElicitationAction;
use codex_rmcp_client::ElicitationResponse;
use mcp_types::RequestId;
use std::path::PathBuf;
use std::sync::Arc;
use tracing::info;
use tracing::warn;
@@ -1879,6 +1882,43 @@ mod handlers {
sess.send_event_raw(event).await;
}
pub async fn list_skills(sess: &Session, sub_id: String, cwds: Vec<PathBuf>) {
let cwds = if cwds.is_empty() {
let state = sess.state.lock().await;
vec![state.session_configuration.cwd.clone()]
} else {
cwds
};
let skills = if sess.enabled(Feature::Skills) {
let skills_manager = &sess.services.skills_manager;
cwds.into_iter()
.map(|cwd| {
let outcome = skills_manager.skills_for_cwd(&cwd);
let errors = super::errors_to_info(&outcome.errors);
let skills = super::skills_to_info(&outcome.skills);
SkillsListEntry {
cwd,
skills,
errors,
}
})
.collect()
} else {
cwds.into_iter()
.map(|cwd| SkillsListEntry {
cwd,
skills: Vec::new(),
errors: Vec::new(),
})
.collect()
};
let event = Event {
id: sub_id,
msg: EventMsg::ListSkillsResponse(ListSkillsResponseEvent { skills }),
};
sess.send_event_raw(event).await;
}
pub async fn undo(sess: &Arc<Session>, sub_id: String) {
let turn_context = sess
.new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default())
@@ -2061,28 +2101,26 @@ async fn spawn_review_thread(
.await;
}
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(),
})
fn skills_to_info(skills: &[SkillMetadata]) -> Vec<ProtocolSkillMetadata> {
skills
.iter()
.map(|skill| ProtocolSkillMetadata {
name: skill.name.clone(),
description: skill.description.clone(),
path: skill.path.clone(),
scope: skill.scope,
})
.collect()
}
fn errors_to_info(errors: &[SkillError]) -> Vec<SkillErrorInfo> {
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
@@ -2113,10 +2151,20 @@ pub(crate) async fn run_task(
});
sess.send_event(&turn_context, event).await;
let skills_outcome = if sess.enabled(Feature::Skills) {
Some(
sess.services
.skills_manager
.skills_for_cwd(&turn_context.cwd),
)
} else {
None
};
let SkillInjections {
items: skill_items,
warnings: skill_warnings,
} = build_skill_injections(&input, sess.services.skills.as_ref()).await;
} = build_skill_injections(&input, skills_outcome.as_ref()).await;
for message in skill_warnings {
sess.send_event(&turn_context, EventMsg::Warning(WarningEvent { message }))
@@ -3013,6 +3061,7 @@ mod tests {
);
let state = SessionState::new(session_configuration.clone());
let skills_manager = Arc::new(SkillsManager::new(config.codex_home.clone()));
let services = SessionServices {
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())),
@@ -3026,7 +3075,7 @@ mod tests {
otel_manager: otel_manager.clone(),
models_manager,
tool_approvals: Mutex::new(ApprovalStore::default()),
skills: None,
skills_manager,
};
let turn_context = Session::make_turn_context(
@@ -3103,6 +3152,7 @@ mod tests {
);
let state = SessionState::new(session_configuration.clone());
let skills_manager = Arc::new(SkillsManager::new(config.codex_home.clone()));
let services = SessionServices {
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::default())),
@@ -3116,7 +3166,7 @@ mod tests {
otel_manager: otel_manager.clone(),
models_manager,
tool_approvals: Mutex::new(ApprovalStore::default()),
skills: None,
skills_manager,
};
let turn_context = Arc::new(Session::make_turn_context(
+1
View File
@@ -49,6 +49,7 @@ pub(crate) async fn run_codex_conversation_interactive(
config,
auth_manager,
models_manager,
Arc::clone(&parent_session.services.skills_manager),
initial_history.unwrap_or(InitialHistory::New),
SessionSource::SubAgent(SubAgentSource::Review),
)
+37 -1
View File
@@ -1,4 +1,5 @@
use crate::AuthManager;
#[cfg(any(test, feature = "test-support"))]
use crate::CodexAuth;
#[cfg(any(test, feature = "test-support"))]
use crate::ModelProviderInfo;
@@ -14,6 +15,7 @@ use crate::protocol::Event;
use crate::protocol::EventMsg;
use crate::protocol::SessionConfiguredEvent;
use crate::rollout::RolloutRecorder;
use crate::skills::SkillsManager;
use codex_protocol::ConversationId;
use codex_protocol::items::TurnItem;
use codex_protocol::models::ResponseItem;
@@ -24,6 +26,8 @@ use codex_protocol::protocol::SessionSource;
use std::collections::HashMap;
use std::path::PathBuf;
use std::sync::Arc;
#[cfg(any(test, feature = "test-support"))]
use tempfile::TempDir;
use tokio::sync::RwLock;
/// Represents a newly created Codex conversation, including the first event
@@ -40,16 +44,23 @@ pub struct ConversationManager {
conversations: Arc<RwLock<HashMap<ConversationId, Arc<CodexConversation>>>>,
auth_manager: Arc<AuthManager>,
models_manager: Arc<ModelsManager>,
skills_manager: Arc<SkillsManager>,
session_source: SessionSource,
#[cfg(any(test, feature = "test-support"))]
_test_codex_home_guard: Option<TempDir>,
}
impl ConversationManager {
pub fn new(auth_manager: Arc<AuthManager>, session_source: SessionSource) -> Self {
let skills_manager = Arc::new(SkillsManager::new(auth_manager.codex_home().to_path_buf()));
Self {
conversations: Arc::new(RwLock::new(HashMap::new())),
auth_manager: auth_manager.clone(),
session_source,
models_manager: Arc::new(ModelsManager::new(auth_manager)),
skills_manager,
#[cfg(any(test, feature = "test-support"))]
_test_codex_home_guard: None,
}
}
@@ -57,12 +68,30 @@ impl ConversationManager {
/// Construct with a dummy AuthManager containing the provided CodexAuth.
/// Used for integration tests: should not be used by ordinary business logic.
pub fn with_models_provider(auth: CodexAuth, provider: ModelProviderInfo) -> Self {
let auth_manager = crate::AuthManager::from_auth_for_testing(auth);
let temp_dir = tempfile::tempdir().unwrap_or_else(|err| panic!("temp codex home: {err}"));
let codex_home = temp_dir.path().to_path_buf();
let mut manager = Self::with_models_provider_and_home(auth, provider, codex_home);
manager._test_codex_home_guard = Some(temp_dir);
manager
}
#[cfg(any(test, feature = "test-support"))]
/// Construct with a dummy AuthManager containing the provided CodexAuth and codex home.
/// Used for integration tests: should not be used by ordinary business logic.
pub fn with_models_provider_and_home(
auth: CodexAuth,
provider: ModelProviderInfo,
codex_home: PathBuf,
) -> Self {
let auth_manager = crate::AuthManager::from_auth_for_testing_with_home(auth, codex_home);
let skills_manager = Arc::new(SkillsManager::new(auth_manager.codex_home().to_path_buf()));
Self {
conversations: Arc::new(RwLock::new(HashMap::new())),
auth_manager: auth_manager.clone(),
session_source: SessionSource::Exec,
models_manager: Arc::new(ModelsManager::with_provider(auth_manager, provider)),
skills_manager,
_test_codex_home_guard: None,
}
}
@@ -70,6 +99,10 @@ impl ConversationManager {
self.session_source.clone()
}
pub fn skills_manager(&self) -> Arc<SkillsManager> {
self.skills_manager.clone()
}
pub async fn new_conversation(&self, config: Config) -> CodexResult<NewConversation> {
self.spawn_conversation(
config,
@@ -92,6 +125,7 @@ impl ConversationManager {
config,
auth_manager,
models_manager,
self.skills_manager.clone(),
InitialHistory::New,
self.session_source.clone(),
)
@@ -169,6 +203,7 @@ impl ConversationManager {
config,
auth_manager,
self.models_manager.clone(),
self.skills_manager.clone(),
initial_history,
self.session_source.clone(),
)
@@ -210,6 +245,7 @@ impl ConversationManager {
config,
auth_manager,
self.models_manager.clone(),
self.skills_manager.clone(),
history,
self.session_source.clone(),
)
+1
View File
@@ -79,6 +79,7 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool {
| EventMsg::McpStartupUpdate(_)
| EventMsg::McpStartupComplete(_)
| EventMsg::ListCustomPromptsResponse(_)
| EventMsg::ListSkillsResponse(_)
| EventMsg::PlanUpdate(_)
| EventMsg::ShutdownComplete
| EventMsg::ViewImageToolCall(_)
+38 -13
View File
@@ -3,6 +3,7 @@ use crate::git_info::resolve_root_git_project_for_trust;
use crate::skills::model::SkillError;
use crate::skills::model::SkillLoadOutcome;
use crate::skills::model::SkillMetadata;
use codex_protocol::protocol::SkillScope;
use dunce::canonicalize as normalize_path;
use serde::Deserialize;
use std::collections::VecDeque;
@@ -53,10 +54,21 @@ impl fmt::Display for SkillParseError {
impl Error for SkillParseError {}
pub fn load_skills(config: &Config) -> SkillLoadOutcome {
load_skills_from_roots(skill_roots(config))
}
pub(crate) struct SkillRoot {
pub(crate) path: PathBuf,
pub(crate) scope: SkillScope,
}
pub(crate) fn load_skills_from_roots<I>(roots: I) -> SkillLoadOutcome
where
I: IntoIterator<Item = SkillRoot>,
{
let mut outcome = SkillLoadOutcome::default();
let roots = skill_roots(config);
for root in roots {
discover_skills_under_root(&root, &mut outcome);
discover_skills_under_root(&root.path, root.scope, &mut outcome);
}
outcome
@@ -66,21 +78,33 @@ pub fn load_skills(config: &Config) -> SkillLoadOutcome {
outcome
}
fn skill_roots(config: &Config) -> Vec<PathBuf> {
let mut roots = vec![config.codex_home.join(SKILLS_DIR_NAME)];
pub(crate) fn user_skills_root(codex_home: &Path) -> SkillRoot {
SkillRoot {
path: codex_home.join(SKILLS_DIR_NAME),
scope: SkillScope::User,
}
}
if let Some(repo_root) = resolve_root_git_project_for_trust(&config.cwd) {
roots.push(
repo_root
.join(REPO_ROOT_CONFIG_DIR_NAME)
.join(SKILLS_DIR_NAME),
);
pub(crate) fn repo_skills_root(cwd: &Path) -> Option<SkillRoot> {
resolve_root_git_project_for_trust(cwd).map(|repo_root| SkillRoot {
path: repo_root
.join(REPO_ROOT_CONFIG_DIR_NAME)
.join(SKILLS_DIR_NAME),
scope: SkillScope::Repo,
})
}
fn skill_roots(config: &Config) -> Vec<SkillRoot> {
let mut roots = vec![user_skills_root(&config.codex_home)];
if let Some(repo_root) = repo_skills_root(&config.cwd) {
roots.push(repo_root);
}
roots
}
fn discover_skills_under_root(root: &Path, outcome: &mut SkillLoadOutcome) {
fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut SkillLoadOutcome) {
let Ok(root) = normalize_path(root) else {
return;
};
@@ -124,7 +148,7 @@ fn discover_skills_under_root(root: &Path, outcome: &mut SkillLoadOutcome) {
}
if file_type.is_file() && file_name == SKILLS_FILENAME {
match parse_skill_file(&path) {
match parse_skill_file(&path, scope) {
Ok(skill) => outcome.skills.push(skill),
Err(err) => outcome.errors.push(SkillError {
path,
@@ -136,7 +160,7 @@ fn discover_skills_under_root(root: &Path, outcome: &mut SkillLoadOutcome) {
}
}
fn parse_skill_file(path: &Path) -> Result<SkillMetadata, SkillParseError> {
fn parse_skill_file(path: &Path, scope: SkillScope) -> Result<SkillMetadata, SkillParseError> {
let contents = fs::read_to_string(path).map_err(SkillParseError::Read)?;
let frontmatter = extract_frontmatter(&contents).ok_or(SkillParseError::MissingFrontmatter)?;
@@ -156,6 +180,7 @@ fn parse_skill_file(path: &Path) -> Result<SkillMetadata, SkillParseError> {
name,
description,
path: resolved_path,
scope,
})
}
+48
View File
@@ -0,0 +1,48 @@
use std::collections::HashMap;
use std::path::Path;
use std::path::PathBuf;
use std::sync::RwLock;
use crate::skills::SkillLoadOutcome;
use crate::skills::loader::load_skills_from_roots;
use crate::skills::loader::repo_skills_root;
use crate::skills::loader::user_skills_root;
pub struct SkillsManager {
codex_home: PathBuf,
cache_by_cwd: RwLock<HashMap<PathBuf, SkillLoadOutcome>>,
}
impl SkillsManager {
pub fn new(codex_home: PathBuf) -> Self {
Self {
codex_home,
cache_by_cwd: RwLock::new(HashMap::new()),
}
}
pub fn skills_for_cwd(&self, cwd: &Path) -> SkillLoadOutcome {
let cached = match self.cache_by_cwd.read() {
Ok(cache) => cache.get(cwd).cloned(),
Err(err) => err.into_inner().get(cwd).cloned(),
};
if let Some(outcome) = cached {
return outcome;
}
let mut roots = vec![user_skills_root(&self.codex_home)];
if let Some(repo_root) = repo_skills_root(cwd) {
roots.push(repo_root);
}
let outcome = load_skills_from_roots(roots);
match self.cache_by_cwd.write() {
Ok(mut cache) => {
cache.insert(cwd.to_path_buf(), outcome.clone());
}
Err(err) => {
err.into_inner().insert(cwd.to_path_buf(), outcome.clone());
}
}
outcome
}
}
+2
View File
@@ -1,11 +1,13 @@
pub mod injection;
pub mod loader;
pub mod manager;
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 manager::SkillsManager;
pub use model::SkillError;
pub use model::SkillLoadOutcome;
pub use model::SkillMetadata;
+3
View File
@@ -1,10 +1,13 @@
use std::path::PathBuf;
use codex_protocol::protocol::SkillScope;
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct SkillMetadata {
pub name: String,
pub description: String,
pub path: PathBuf,
pub scope: SkillScope,
}
#[derive(Debug, Clone, PartialEq, Eq)]
+2 -2
View File
@@ -4,7 +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::skills::SkillsManager;
use crate::tools::sandboxing::ApprovalStore;
use crate::unified_exec::UnifiedExecSessionManager;
use crate::user_notification::UserNotifier;
@@ -25,5 +25,5 @@ pub(crate) struct SessionServices {
pub(crate) models_manager: Arc<ModelsManager>,
pub(crate) otel_manager: OtelManager,
pub(crate) tool_approvals: Mutex<ApprovalStore>,
pub(crate) skills: Option<SkillLoadOutcome>,
pub(crate) skills_manager: Arc<SkillsManager>,
}