-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Reimplement skills loading using SkillsManager + skills/list op. #7914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,9 @@ use codex_protocol::protocol::CreditsSnapshot as CoreCreditsSnapshot; | |
| use codex_protocol::protocol::RateLimitSnapshot as CoreRateLimitSnapshot; | ||
| use codex_protocol::protocol::RateLimitWindow as CoreRateLimitWindow; | ||
| use codex_protocol::protocol::SessionSource as CoreSessionSource; | ||
| use codex_protocol::protocol::SkillErrorInfo as CoreSkillErrorInfo; | ||
| use codex_protocol::protocol::SkillMetadata as CoreSkillMetadata; | ||
| use codex_protocol::protocol::SkillScope as CoreSkillScope; | ||
| use codex_protocol::protocol::TokenUsage as CoreTokenUsage; | ||
| use codex_protocol::protocol::TokenUsageInfo as CoreTokenUsageInfo; | ||
| use codex_protocol::user_input::UserInput as CoreUserInput; | ||
|
|
@@ -967,6 +970,87 @@ pub struct ThreadCompactParams { | |
| #[ts(export_to = "v2/")] | ||
| pub struct ThreadCompactResponse {} | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct SkillsListParams { | ||
| /// When empty, defaults to the current session working directory. | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub cwds: Vec<PathBuf>, | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct SkillsListResponse { | ||
| pub data: Vec<SkillsListEntry>, | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] | ||
| #[serde(rename_all = "snake_case")] | ||
| #[ts(rename_all = "snake_case")] | ||
| #[ts(export_to = "v2/")] | ||
| pub enum SkillScope { | ||
| User, | ||
| Repo, | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct SkillMetadata { | ||
| pub name: String, | ||
| pub description: String, | ||
| pub path: PathBuf, | ||
| pub scope: SkillScope, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'll want the icon fields too (cc @gverma-openai @edward-bayes)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could extend the API later. This is just a minimum set to start with. |
||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct SkillErrorInfo { | ||
| pub path: PathBuf, | ||
| pub message: String, | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct SkillsListEntry { | ||
| pub cwd: PathBuf, | ||
| pub skills: Vec<SkillMetadata>, | ||
| pub errors: Vec<SkillErrorInfo>, | ||
| } | ||
|
|
||
| impl From<CoreSkillMetadata> for SkillMetadata { | ||
| fn from(value: CoreSkillMetadata) -> Self { | ||
| Self { | ||
| name: value.name, | ||
| description: value.description, | ||
| path: value.path, | ||
| scope: value.scope.into(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<CoreSkillScope> for SkillScope { | ||
| fn from(value: CoreSkillScope) -> Self { | ||
| match value { | ||
| CoreSkillScope::User => Self::User, | ||
| CoreSkillScope::Repo => Self::Repo, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<CoreSkillErrorInfo> for SkillErrorInfo { | ||
| fn from(value: CoreSkillErrorInfo) -> Self { | ||
| Self { | ||
| path: value.path, | ||
| message: value.message, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,8 @@ use codex_app_server_protocol::ServerNotification; | |
| use codex_app_server_protocol::SessionConfiguredNotification; | ||
| use codex_app_server_protocol::SetDefaultModelParams; | ||
| use codex_app_server_protocol::SetDefaultModelResponse; | ||
| use codex_app_server_protocol::SkillsListParams; | ||
| use codex_app_server_protocol::SkillsListResponse; | ||
| use codex_app_server_protocol::Thread; | ||
| use codex_app_server_protocol::ThreadArchiveParams; | ||
| use codex_app_server_protocol::ThreadArchiveResponse; | ||
|
|
@@ -373,6 +375,9 @@ impl CodexMessageProcessor { | |
| self.send_unimplemented_error(request_id, "thread/compact") | ||
| .await; | ||
| } | ||
| ClientRequest::SkillsList { request_id, params } => { | ||
| self.skills_list(request_id, params).await; | ||
| } | ||
| ClientRequest::TurnStart { request_id, params } => { | ||
| self.turn_start(request_id, params).await; | ||
| } | ||
|
|
@@ -2615,6 +2620,42 @@ impl CodexMessageProcessor { | |
| .await; | ||
| } | ||
|
|
||
| async fn skills_list(&self, request_id: RequestId, params: SkillsListParams) { | ||
| let SkillsListParams { cwds } = params; | ||
| let cwds = if cwds.is_empty() { | ||
| vec![self.config.cwd.clone()] | ||
| } else { | ||
| cwds | ||
| }; | ||
|
|
||
| let data = if self.config.features.enabled(Feature::Skills) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no strong feelings but consider encapsulating this check in skills_manager
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m inclined to keep the Feature::Skills guard at the call site. SkillsManager doesn’t have access to config/session state, so pushing the check down would either require passing Config in (extra coupling) or just moving the same if behind another method without changing behavior. |
||
| let skills_manager = self.conversation_manager.skills_manager(); | ||
| cwds.into_iter() | ||
| .map(|cwd| { | ||
| let outcome = skills_manager.skills_for_cwd(&cwd); | ||
| let errors = errors_to_info(&outcome.errors); | ||
| let skills = skills_to_info(&outcome.skills); | ||
| codex_app_server_protocol::SkillsListEntry { | ||
| cwd, | ||
| skills, | ||
| errors, | ||
| } | ||
| }) | ||
| .collect() | ||
| } else { | ||
| cwds.into_iter() | ||
| .map(|cwd| codex_app_server_protocol::SkillsListEntry { | ||
| cwd, | ||
| skills: Vec::new(), | ||
| errors: Vec::new(), | ||
| }) | ||
| .collect() | ||
| }; | ||
| self.outgoing | ||
| .send_response(request_id, SkillsListResponse { data }) | ||
| .await; | ||
| } | ||
|
|
||
| async fn interrupt_conversation( | ||
| &mut self, | ||
| request_id: RequestId, | ||
|
|
@@ -3260,6 +3301,32 @@ impl CodexMessageProcessor { | |
| } | ||
| } | ||
|
|
||
| fn skills_to_info( | ||
| skills: &[codex_core::skills::SkillMetadata], | ||
| ) -> Vec<codex_app_server_protocol::SkillMetadata> { | ||
| skills | ||
| .iter() | ||
| .map(|skill| codex_app_server_protocol::SkillMetadata { | ||
| name: skill.name.clone(), | ||
| description: skill.description.clone(), | ||
| path: skill.path.clone(), | ||
| scope: skill.scope.into(), | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| fn errors_to_info( | ||
| errors: &[codex_core::skills::SkillError], | ||
| ) -> Vec<codex_app_server_protocol::SkillErrorInfo> { | ||
| errors | ||
| .iter() | ||
| .map(|err| codex_app_server_protocol::SkillErrorInfo { | ||
| path: err.path.clone(), | ||
| message: err.message.clone(), | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| async fn derive_config_from_params( | ||
| overrides: ConfigOverrides, | ||
| cli_overrides: Option<std::collections::HashMap<String, serde_json::Value>>, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you care which home is set on auth? |
||
| 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()) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider adding pagination like other endpoints (
models/list)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider having many directories open, do we want to wait for all of them to load skills before this method returns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! We can treat pagination/streaming as a follow-up optimization once we see more skills enabled in practice. For now, loading everything in one go keeps it simple, and clients that need finer-grained control can already split requests by cwd.