diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b353bcb007..18c0260123 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2911,16 +2911,10 @@ dependencies = [ name = "codex-core-skills" version = "0.0.0" dependencies = [ - "codex-analytics", - "codex-context-fragments", "codex-exec-server", - "codex-otel", "codex-skills", "codex-utils-absolute-path", "codex-utils-path-uri", - "codex-utils-string", - "pretty_assertions", - "tracing", ] [[package]] diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index 4bc5fd5b09..f72b4ef8f7 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -8,20 +8,13 @@ version.workspace = true doctest = false name = "codex_core_skills" path = "src/lib.rs" +test = false [lints] workspace = true [dependencies] -codex-analytics = { workspace = true } -codex-context-fragments = { workspace = true } codex-exec-server = { workspace = true } -codex-otel = { workspace = true } codex-skills = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } -codex-utils-string = { workspace = true } -tracing = { workspace = true } - -[dev-dependencies] -pretty_assertions = { workspace = true } diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs deleted file mode 100644 index f50427f79a..0000000000 --- a/codex-rs/core-skills/src/injection.rs +++ /dev/null @@ -1,178 +0,0 @@ -use std::collections::HashSet; -use std::sync::Arc; - -use crate::SkillLoadOutcome; -use crate::SkillMetadata; -use codex_analytics::AnalyticsEventsClient; -use codex_analytics::InvocationType; -use codex_analytics::SkillInvocation; -use codex_analytics::TrackEventsContext; -use codex_exec_server::LOCAL_FS; -use codex_otel::SessionTelemetry; -use codex_otel::sanitize_metric_tag_value; -pub use codex_skills::ToolMentionKind; -pub use codex_skills::ToolMentions; -pub use codex_skills::app_id_from_path; -pub use codex_skills::extract_tool_mentions; -pub use codex_skills::extract_tool_mentions_with_sigil; -pub use codex_skills::normalize_skill_path; -pub use codex_skills::plugin_config_name_from_path; -pub use codex_skills::tool_kind_for_path; -use codex_utils_path_uri::PathUri; -use codex_utils_string::take_bytes_at_char_boundary; - -use crate::MAX_SKILL_PROMPT_BYTES; - -#[derive(Debug, Default)] -pub struct SkillInjections { - pub items: Vec, - pub warnings: Vec, -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct SkillInjection { - pub name: String, - pub path: String, - pub contents: String, -} - -/// Host skill prompts that have already been injected by an extension for this -/// turn. -/// -/// Core uses this to keep the legacy skill-injection path from sending the same -/// host `SKILL.md` body again while the skills extension is being wired in. -#[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct InjectedHostSkillPrompts { - paths: HashSet, -} - -/// Marks a turn whose skills extension projects the host skill catalog through -/// WorldState. -/// -/// Core uses this to keep its legacy thread-start catalog from duplicating the -/// extension-owned catalog. -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] -pub struct HostSkillsCatalogInWorldState; - -impl InjectedHostSkillPrompts { - pub fn insert_path(&mut self, path: impl Into) { - let path = path.into(); - self.paths.insert(normalize_host_skill_path(&path)); - self.paths.insert(path); - } - - pub fn is_empty(&self) -> bool { - self.paths.is_empty() - } - - pub fn contains_path(&self, path: &str) -> bool { - self.paths.contains(path) || self.paths.contains(&normalize_host_skill_path(path)) - } -} - -#[tracing::instrument( - level = "trace", - skip_all, - fields(mentioned_skill_count = mentioned_skills.len()) -)] -pub async fn build_skill_injections( - mentioned_skills: &[SkillMetadata], - loaded_skills: Option<&SkillLoadOutcome>, - otel: Option<&SessionTelemetry>, - analytics_client: &AnalyticsEventsClient, - tracking: TrackEventsContext, -) -> SkillInjections { - if mentioned_skills.is_empty() { - return SkillInjections::default(); - } - - let mut result = SkillInjections { - items: Vec::with_capacity(mentioned_skills.len()), - warnings: Vec::new(), - }; - let mut invocations = Vec::new(); - - for skill in mentioned_skills { - let fs = loaded_skills - .and_then(|outcome| outcome.file_system_for_skill(skill)) - .unwrap_or_else(|| Arc::clone(&LOCAL_FS)); - let path = PathUri::from_abs_path(&skill.path_to_skills_md); - match fs.read_file_text(&path, /*sandbox*/ None).await { - Ok(contents) => { - let (contents, truncated) = - if loaded_skills.is_some_and(|outcome| outcome.is_agent_plugin_skill(skill)) { - bounded_skill_prompt_contents(&contents) - } else { - (contents, false) - }; - if truncated { - result.warnings.push(format!( - "Skill `{}` exceeded the main prompt context limit and was truncated.", - skill.name - )); - } - emit_skill_injected_metric(otel, skill, "ok"); - invocations.push(SkillInvocation { - skill_name: skill.name.clone(), - skill_scope: skill.scope, - skill_path: skill.path_to_skills_md.to_path_buf(), - plugin_id: skill.plugin_id.clone(), - remote_plugin_id: skill.remote_plugin_id.clone(), - invocation_type: InvocationType::Explicit, - }); - result.items.push(SkillInjection { - name: skill.name.clone(), - path: skill.path_to_skills_md.to_string_lossy().into_owned(), - contents, - }); - } - Err(err) => { - emit_skill_injected_metric(otel, skill, "error"); - let message = format!( - "Failed to load skill {name} at {path}: {err:#}", - name = skill.name, - path = skill.path_to_skills_md.display() - ); - result.warnings.push(message); - } - } - } - - analytics_client.track_skill_invocations(tracking, invocations); - - result -} - -fn bounded_skill_prompt_contents(contents: &str) -> (String, bool) { - let bounded = take_bytes_at_char_boundary(contents, MAX_SKILL_PROMPT_BYTES); - (bounded.to_string(), bounded.len() < contents.len()) -} - -fn normalize_host_skill_path(path: &str) -> String { - normalize_skill_path(path).replace('\\', "/") -} - -fn emit_skill_injected_metric( - otel: Option<&SessionTelemetry>, - skill: &SkillMetadata, - status: &str, -) { - let Some(otel) = otel else { - return; - }; - let skill_name_tag = sanitize_metric_tag_value(skill.name.as_str()); - - otel.counter( - "codex.skill.injected", - /*inc*/ 1, - &[ - ("status", status), - ("skill", skill_name_tag.as_str()), - ("invoke_type", "explicit"), - ], - ); -} - -#[cfg(test)] -#[path = "prompt_injection_tests.rs"] -mod tests; diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 2e3a377405..abcf5fa6bb 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -1,7 +1,5 @@ -pub mod injection; pub(crate) mod invocation_utils; pub mod model; -mod skill_instructions; /// Hard byte limit for one model-visible skill instruction body. /// @@ -17,4 +15,3 @@ pub use model::SkillError; pub use model::SkillLoadOutcome; pub use model::SkillMetadata; pub use model::SkillPolicy; -pub use skill_instructions::SkillInstructions; diff --git a/codex-rs/core-skills/src/prompt_injection_tests.rs b/codex-rs/core-skills/src/prompt_injection_tests.rs deleted file mode 100644 index bd91481c5b..0000000000 --- a/codex-rs/core-skills/src/prompt_injection_tests.rs +++ /dev/null @@ -1,14 +0,0 @@ -use pretty_assertions::assert_eq; - -use super::MAX_SKILL_PROMPT_BYTES; -use super::bounded_skill_prompt_contents; - -#[test] -fn skill_prompt_contents_are_bounded_at_utf8_boundaries() { - let contents = format!("{}é", "a".repeat(MAX_SKILL_PROMPT_BYTES - 1)); - - let (bounded, truncated) = bounded_skill_prompt_contents(&contents); - - assert_eq!(bounded.len(), MAX_SKILL_PROMPT_BYTES - 1); - assert_eq!(truncated, true); -} diff --git a/codex-rs/core-skills/src/skill_instructions.rs b/codex-rs/core-skills/src/skill_instructions.rs deleted file mode 100644 index b2a002d902..0000000000 --- a/codex-rs/core-skills/src/skill_instructions.rs +++ /dev/null @@ -1,41 +0,0 @@ -use codex_context_fragments::ContextualUserFragment; - -use crate::injection::SkillInjection; - -#[derive(Debug, Clone, PartialEq)] -pub struct SkillInstructions { - name: String, - path: String, - contents: String, -} - -impl From<&SkillInjection> for SkillInstructions { - fn from(skill: &SkillInjection) -> Self { - Self { - name: skill.name.clone(), - path: skill.path.clone(), - contents: skill.contents.clone(), - } - } -} - -impl ContextualUserFragment for SkillInstructions { - fn role(&self) -> &'static str { - "user" - } - - fn markers(&self) -> (&'static str, &'static str) { - Self::type_markers() - } - - fn type_markers() -> (&'static str, &'static str) { - ("", "") - } - - fn body(&self) -> String { - format!( - "\n{}\n{}\n{}\n", - self.name, self.path, self.contents - ) - } -} diff --git a/codex-rs/core/src/context/contextual_user_message.rs b/codex-rs/core/src/context/contextual_user_message.rs index 9a1b5d85ff..b4b57bd6c7 100644 --- a/codex-rs/core/src/context/contextual_user_message.rs +++ b/codex-rs/core/src/context/contextual_user_message.rs @@ -9,7 +9,6 @@ use super::LegacyApplyPatchExecCommandWarning; use super::LegacyModelMismatchWarning; use super::LegacyUnifiedExecProcessLimitWarning; use super::RecommendedPluginsInstructions; -use super::SkillInstructions; use super::SubagentNotification; use super::TurnAborted; use super::UserInstructions; @@ -20,7 +19,7 @@ const CONTEXTUAL_USER_FRAGMENT_MATCHERS: &[fn(&str) -> bool] = &[ UserInstructions::matches_text, EnvironmentsState::matches_text, AdditionalContextUserFragment::matches_text, - SkillInstructions::matches_text, + codex_skills_extension::is_skill_prompt_fragment, UserShellCommand::matches_text, TurnAborted::matches_text, SubagentNotification::matches_text, diff --git a/codex-rs/core/src/context/contextual_user_message_tests.rs b/codex-rs/core/src/context/contextual_user_message_tests.rs index a7d6e408a5..dcfa924dda 100644 --- a/codex-rs/core/src/context/contextual_user_message_tests.rs +++ b/codex-rs/core/src/context/contextual_user_message_tests.rs @@ -15,6 +15,18 @@ fn detects_environment_context_fragment() { })); } +#[test] +fn detects_skill_instructions_fragment_case_insensitively() { + for text in [ + "\ndemo\n", + " \ndemo\n ", + ] { + assert!(is_contextual_user_fragment(&ContentItem::InputText { + text: text.to_string(), + })); + } +} + #[test] fn detects_agents_instructions_fragment() { for text in [ diff --git a/codex-rs/core/src/context/mod.rs b/codex-rs/core/src/context/mod.rs index 597a1268de..f5dd8a72a3 100644 --- a/codex-rs/core/src/context/mod.rs +++ b/codex-rs/core/src/context/mod.rs @@ -43,7 +43,6 @@ pub(crate) use available_plugins_instructions::AvailablePluginsInstructions; pub(crate) use codex_context_fragments::AdditionalContextDeveloperFragment; pub(crate) use codex_context_fragments::AdditionalContextUserFragment; pub use codex_context_fragments::ContextualUserFragment; -pub(crate) use codex_core_skills::SkillInstructions; pub(crate) use contextual_user_message::is_contextual_user_fragment; pub(crate) use contextual_user_message::parse_visible_hook_prompt_message; pub(crate) use current_time_reminder::CurrentTimeReminder; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 9138a139c1..adbde89877 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -92,12 +92,9 @@ mod session_prefix; mod session_startup_prewarm; pub mod skills; pub(crate) use skills::HostSkillsService; -pub(crate) use skills::SkillInjections; pub(crate) use skills::SkillMetadata; -pub(crate) use skills::build_skill_injections; pub(crate) use skills::build_skill_name_counts; pub(crate) use skills::collect_explicit_skill_mentions; -pub(crate) use skills::injection; pub(crate) use skills::maybe_emit_implicit_skill_invocation; pub(crate) use skills::skills_load_input_from_config; mod stream_events_utils; diff --git a/codex-rs/core/src/plugins/mentions.rs b/codex-rs/core/src/plugins/mentions.rs index a5e94345cb..50b007b075 100644 --- a/codex-rs/core/src/plugins/mentions.rs +++ b/codex-rs/core/src/plugins/mentions.rs @@ -3,13 +3,13 @@ use std::collections::HashSet; use codex_connectors::metadata::connector_mention_slug; use codex_protocol::user_input::UserInput; +use codex_skills::ToolMentionKind; +use codex_skills::app_id_from_path; +use codex_skills::extract_tool_mentions_with_sigil; +use codex_skills::plugin_config_name_from_path; +use codex_skills::tool_kind_for_path; use crate::connectors; -use crate::injection::ToolMentionKind; -use crate::injection::app_id_from_path; -use crate::injection::extract_tool_mentions_with_sigil; -use crate::injection::plugin_config_name_from_path; -use crate::injection::tool_kind_for_path; use crate::mention_syntax::PLUGIN_TEXT_MENTION_SIGIL; use crate::mention_syntax::TOOL_MENTION_SIGIL; diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index e5157c4953..b7fc003fbc 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -3,8 +3,6 @@ use std::collections::HashSet; use std::sync::Arc; use std::sync::atomic::Ordering; -use crate::SkillInjections; -use crate::build_skill_injections; use crate::client::ModelClientSession; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; @@ -24,9 +22,6 @@ use crate::hook_runtime::reject_pending_input; use crate::hook_runtime::run_legacy_after_agent_hook; use crate::hook_runtime::run_pending_session_start_hooks; use crate::hook_runtime::run_turn_stop_hooks; -use crate::injection::ToolMentionKind; -use crate::injection::app_id_from_path; -use crate::injection::tool_kind_for_path; use crate::mcp_skill_dependencies::maybe_prompt_and_install_mcp_dependencies; use crate::mentions::build_connector_slug_counts; use crate::mentions::build_skill_name_counts; @@ -44,6 +39,7 @@ use crate::session::TurnInput; use crate::session::session::Session; use crate::session::step_context::StepContext; use crate::session::turn_context::TurnContext; +use crate::skills::emit_explicit_skill_invocations; use crate::stream_events_utils::HandleOutputCtx; use crate::stream_events_utils::TurnItemContributorPolicy; use crate::stream_events_utils::finalize_non_tool_response_item; @@ -74,7 +70,6 @@ use codex_analytics::build_track_events_context; use codex_async_utils::OrCancelExt; use codex_connectors::AppToolPolicyEvaluator; use codex_core_plugins::RecommendedPluginCandidatesInput; -use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_extension_api::ExtensionData; use codex_extension_api::TurnInputContext; use codex_extension_api::TurnInputEnvironment; @@ -111,6 +106,11 @@ use codex_protocol::protocol::SafetyBufferingEvent; use codex_protocol::protocol::TurnDiffEvent; use codex_protocol::protocol::WarningEvent; use codex_protocol::user_input::UserInput; +use codex_skills::ToolMentionKind; +use codex_skills::app_id_from_path; +use codex_skills::tool_kind_for_path; +use codex_skills_extension::HostSkillPrompts; +use codex_skills_extension::InjectedHostSkillPrompts; use codex_tools::DiscoverableTool; use codex_tools::ToolName; use codex_tools::filter_request_plugin_install_discoverable_tools_for_client; @@ -791,27 +791,26 @@ async fn build_skills_and_plugins( let injected_host_skill_prompts = turn_context .extension_data .get::(); - let SkillInjections { - items: skill_injections, - warnings: skill_warnings, - } = build_skill_injections( + let HostSkillPrompts { + fragments, + injected: injected_host_skills, + warnings: host_skill_warnings, + } = skills_snapshot.load_skill_prompts(&mentioned_skills).await; + emit_explicit_skill_invocations( + sess, + turn_context, &mentioned_skills, - Some(skills_outcome), - Some(&turn_context.session_telemetry), - &sess.services.analytics_events_client, + &injected_host_skills, tracking.clone(), - ) - .await; - - for message in skill_warnings { + ); + for message in host_skill_warnings { sess.send_event(turn_context, EventMsg::Warning(WarningEvent { message })) .await; } - - let skill_items: Vec = skill_injections - .iter() - .map(|skill| ContextualUserFragment::into(crate::context::SkillInstructions::from(skill))) - .collect(); + let skill_items = fragments + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item) + .collect::>(); let skill_connector_ids = collect_explicit_app_ids_from_skill_items( &skill_items, &available_connectors, @@ -849,12 +848,14 @@ async fn build_skills_and_plugins( } } - let mut injection_items: Vec = match injected_host_skill_prompts { - Some(injected_host_skill_prompts) => skill_injections - .iter() - .filter(|skill| !injected_host_skill_prompts.contains_path(&skill.path)) - .map(|skill| { - ContextualUserFragment::into(crate::context::SkillInstructions::from(skill)) + let mut injection_items = match injected_host_skill_prompts { + Some(injected_host_skill_prompts) => skill_items + .into_iter() + .zip(injected_host_skills.iter()) + .filter_map(|(item, skill)| { + (!injected_host_skill_prompts + .contains_path(&skill.path_to_skills_md.to_string_lossy())) + .then_some(item) }) .collect(), None => skill_items, diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index f6638fb002..dea0437c59 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -3,6 +3,7 @@ use crate::session::session::Session; use crate::session::turn_context::TurnContext; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; +use codex_analytics::TrackEventsContext; use codex_analytics::build_track_events_context; use codex_extension_api::SkillInvocationInput; use codex_extension_api::SkillInvocationKind; @@ -15,15 +16,12 @@ use tokio::sync::Mutex; pub use codex_core_skills::SkillError; pub use codex_core_skills::SkillLoadOutcome; -pub use codex_core_skills::build_skill_name_counts; -pub use codex_core_skills::detect_implicit_skill_invocation_for_command; -pub use codex_core_skills::injection; -pub use codex_core_skills::injection::SkillInjections; -pub use codex_core_skills::injection::build_skill_injections; pub use codex_core_skills::model; pub use codex_skills::SkillMetadata; pub use codex_skills::SkillPolicy; +pub use codex_skills::build_skill_name_counts; pub use codex_skills::collect_explicit_skill_mentions; +pub use codex_skills::detect_implicit_skill_invocation_for_command; pub use codex_skills_extension::HostSkillsLoadInput; pub use codex_skills_extension::HostSkillsService; pub use codex_skills_extension::bundled_skills_enabled_from_stack; @@ -43,6 +41,51 @@ pub(crate) fn skills_load_input_from_config( ) } +pub(crate) fn emit_explicit_skill_invocations( + sess: &Session, + turn_context: &TurnContext, + mentioned_skills: &[SkillMetadata], + injected_skills: &[SkillMetadata], + tracking: TrackEventsContext, +) { + let injected_skill_paths = injected_skills + .iter() + .map(|skill| &skill.path_to_skills_md) + .collect::>(); + for skill in mentioned_skills { + let skill_name_tag = sanitize_metric_tag_value(skill.name.as_str()); + let status = if injected_skill_paths.contains(&skill.path_to_skills_md) { + "ok" + } else { + "error" + }; + turn_context.session_telemetry.counter( + "codex.skill.injected", + /*inc*/ 1, + &[ + ("status", status), + ("skill", skill_name_tag.as_str()), + ("invoke_type", "explicit"), + ], + ); + } + + let invocations = injected_skills + .iter() + .map(|skill| SkillInvocation { + skill_name: skill.name.clone(), + skill_scope: skill.scope, + skill_path: skill.path_to_skills_md.to_path_buf(), + plugin_id: skill.plugin_id.clone(), + remote_plugin_id: skill.remote_plugin_id.clone(), + invocation_type: InvocationType::Explicit, + }) + .collect(); + sess.services + .analytics_events_client + .track_skill_invocations(tracking, invocations); +} + pub(crate) async fn maybe_emit_implicit_skill_invocation( sess: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index 7148995049..a8d720a20e 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -649,6 +649,163 @@ async fn capability_sections_render_in_order_with_host_repo_and_plugin_skills() Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn agent_plugin_skill_prompt_stays_bounded_without_skills_extension() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = responses::start_mock_server().await; + let response = mount_sse_once( + &server, + sse(vec![ev_response_created("resp1"), ev_completed("resp1")]), + ) + .await; + + let codex_home = Arc::new(TempDir::new()?); + let plugin_root = codex_home + .path() + .join("plugins/cache/test/acme.tools/local"); + let skill_dir = plugin_root.join("skills/review"); + std::fs::create_dir_all(&skill_dir)?; + std::fs::write( + plugin_root.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"acme.tools","extensions":{"com.openai":{"interface":{"displayName":"Acme Developer Tools"}}}}"#, + )?; + std::fs::write( + skill_dir.join("SKILL.md"), + format!( + "---\nname: review\ndescription: Review code\n---\n\n{}\nAGENT_SKILL_TRUNCATED_TAIL\n", + "x".repeat(9_000) + ), + )?; + std::fs::write( + codex_home.path().join("config.toml"), + "[features]\nplugins = true\n\n[plugins.\"acme.tools@test\"]\nenabled = true\n", + )?; + let skill_path = dunce::canonicalize(skill_dir.join("SKILL.md"))?; + let mut builder = test_codex().with_home(codex_home); + let test = builder.build_with_auto_env(&server).await?; + + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Skill { + name: "acme.tools:review".into(), + path: skill_path, + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + let warning = core_test_support::wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::Warning(warning) + if warning.message.contains("main prompt context limit") + ) + }) + .await; + core_test_support::wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let user_text = response + .single_request() + .message_input_texts("user") + .join("\n"); + assert!(user_text.contains("acme.tools:review")); + assert!(!user_text.contains("AGENT_SKILL_TRUNCATED_TAIL")); + let EventMsg::Warning(warning) = warning else { + unreachable!("wait_for_event matched an Agent skill truncation warning") + }; + assert!(warning.message.contains("acme.tools:review")); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn explicit_skill_prompt_precedes_plugin_instructions() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = responses::start_mock_server().await; + let response = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + + let codex_home = Arc::new(TempDir::new()?); + let plugin_root = codex_home.path().join("plugins/cache/test/sample/local"); + let skill_dir = plugin_root.join("skills/sample-search"); + std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?; + std::fs::create_dir_all(&skill_dir)?; + std::fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"sample","description":"inspect sample data"}"#, + )?; + std::fs::write( + skill_dir.join("SKILL.md"), + "---\ndescription: inspect sample data\n---\n\n# body\n", + )?; + std::fs::write( + codex_home.path().join("config.toml"), + "[features]\nplugins = true\n\n[plugins.\"sample@test\"]\nenabled = true\n", + )?; + let skill_path = dunce::canonicalize(skill_dir.join("SKILL.md"))?; + let (extensions, _) = + catalog_extensions(SkillCatalog::default(), /*include_host_provider*/ true); + let mut builder = test_codex() + .with_home(codex_home) + .with_extensions(extensions); + let test = builder.build_with_auto_env(&server).await?; + + test.codex + .submit(Op::UserInput { + items: vec![ + UserInput::Skill { + name: "sample:sample-search".to_string(), + path: skill_path, + }, + UserInput::Mention { + name: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }, + ], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + core_test_support::wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let input = response.single_request().input(); + let prompt_position = |expected: &str| { + input + .iter() + .position(|item| { + item["content"].as_array().is_some_and(|content| { + content.iter().any(|part| { + part["text"] + .as_str() + .is_some_and(|text| text.contains(expected)) + }) + }) + }) + .unwrap_or_else(|| panic!("missing prompt containing `{expected}`: {input:?}")) + }; + let skill_position = prompt_position("\nsample:sample-search"); + let plugin_position = prompt_position("Capabilities from the `sample` plugin:"); + assert!( + skill_position < plugin_position, + "host skill prompts should precede plugin instructions: {input:?}" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn explicit_only_orchestrator_skill_is_hidden_but_can_be_invoked() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1510,6 +1667,181 @@ async fn production_turn_uses_provider_host_catalog_and_core_snapshot_injection( Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn production_turn_suppresses_only_the_superseded_host_skill_prompt() -> Result<()> { + let server = responses::start_mock_server().await; + let response = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + let codex_home = Arc::new(TempDir::new()?); + write_host_skills( + codex_home.path(), + &[ + ("first-host", "First host skill."), + ("second-host", "Second host skill."), + ], + )?; + let first_skill_path = codex_home.path().join("skills/first-host/SKILL.md"); + let second_skill_path = codex_home.path().join("skills/second-host/SKILL.md"); + let first_host_contents = + "---\nname: first-host\ndescription: First host skill.\n---\n\nFIRST_HOST_BODY\n"; + let second_host_contents = + "---\nname: second-host\ndescription: Second host skill.\n---\n\nSECOND_HOST_BODY\n"; + std::fs::write(&first_skill_path, first_host_contents)?; + std::fs::write(&second_skill_path, second_host_contents)?; + let second_skill_path = dunce::canonicalize(second_skill_path)?; + + let source_kind = SkillSourceKind::Custom("test".to_string()); + let provider_resource = "skill://test/first-host/SKILL.md"; + let provider_contents = "FIRST_PROVIDER_BODY"; + let catalog = SkillCatalog { + entries: vec![ + SkillCatalogEntry::new( + SkillPackageId("test/first-host".to_string()), + SkillAuthority::new(source_kind.clone(), "test"), + "first-host", + "Provider skill supersedes the matching host skill.", + SkillResourceId::new(provider_resource), + ) + .with_display_path(provider_resource), + ], + warnings: Vec::new(), + }; + let mut extensions = ExtensionRegistryBuilder::::new(); + install_with_providers( + &mut extensions, + SkillProviders::new().with_provider(SkillProviderSource::new( + source_kind, + "test", + Arc::new(StaticSkillProvider { + catalog, + main_prompt_contents: Some(provider_contents.to_string()), + }), + )), + |config: &Config| SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + bundled_skills_enabled: false, + orchestrator_skills_enabled: false, + shadow_selection_enabled: false, + }, + ); + let mut builder = test_codex() + .with_home(codex_home) + .with_extensions(Arc::new(extensions.build())) + .with_config(configure_catalog_test); + let test = builder.build_with_auto_env(&server).await?; + + test.submit_turn("Use $first-host and $second-host.") + .await?; + + let user_messages = response.single_request().message_input_texts("user"); + let skill_messages = user_messages + .into_iter() + .filter(|message| message.starts_with("")) + .collect::>(); + assert_eq!( + skill_messages, + vec![ + format!( + "\nsecond-host\n{}\n{second_host_contents}\n", + second_skill_path.display() + ), + format!( + "\nfirst-host\n{provider_resource}\n{provider_contents}\n" + ), + ] + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn production_turn_warns_and_omits_unreadable_host_skill() -> Result<()> { + let server = responses::start_mock_server().await; + let response = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + let codex_home = Arc::new(TempDir::new()?); + write_host_skills( + codex_home.path(), + &[ + ("missing-host", "Missing host skill."), + ("available-host", "Available host skill."), + ], + )?; + let missing_skill_path = + dunce::canonicalize(codex_home.path().join("skills/missing-host/SKILL.md"))?; + let available_skill_path = + dunce::canonicalize(codex_home.path().join("skills/available-host/SKILL.md"))?; + let available_skill_contents = std::fs::read_to_string(&available_skill_path)?; + let (extensions, _) = + catalog_extensions(SkillCatalog::default(), /*include_host_provider*/ true); + let mut builder = test_codex() + .with_home(Arc::clone(&codex_home)) + .with_extensions(extensions) + .with_config(configure_catalog_test); + let test = builder.build_with_auto_env(&server).await?; + + std::fs::remove_file(&missing_skill_path)?; + test.codex + .submit(Op::UserInput { + items: vec![ + UserInput::Skill { + name: "missing-host".to_string(), + path: missing_skill_path.clone(), + }, + UserInput::Skill { + name: "available-host".to_string(), + path: available_skill_path.clone(), + }, + ], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + + let mut warnings = Vec::new(); + loop { + match core_test_support::wait_for_event(&test.codex, |_| true).await { + EventMsg::Warning(warning) => warnings.push(warning.message), + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + + let expected_warning_prefix = format!( + "Failed to load skill missing-host at {}:", + missing_skill_path.display() + ); + assert_eq!(warnings.len(), 1); + assert!( + warnings[0].starts_with(&expected_warning_prefix), + "expected unreadable skill warning, got {warnings:?}" + ); + + let skill_messages = response + .single_request() + .message_input_texts("user") + .into_iter() + .filter(|message| message.starts_with("")) + .collect::>(); + assert_eq!( + skill_messages, + vec![format!( + "\navailable-host\n{}\n{available_skill_contents}\n", + available_skill_path.display() + )] + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn production_turn_keeps_full_snapshot_host_skill_prompt() -> Result<()> { let server = responses::start_mock_server().await; diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index c50df7d317..d0848ba138 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -2,8 +2,7 @@ use std::collections::HashMap; use std::sync::Arc; use crate::HostSkillsSnapshot; -use codex_core_skills::injection::HostSkillsCatalogInWorldState; -use codex_core_skills::injection::InjectedHostSkillPrompts; +use crate::InjectedHostSkillPrompts; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LOCAL_ENVIRONMENT_ID; @@ -61,6 +60,7 @@ use crate::selection::collect_explicit_skill_mentions; use crate::shadow_selection_experiment::ShadowSelectionExperiment; use crate::sources::SkillProviders; use crate::state::ExecutorSkillsStepState; +use crate::state::HostSkillsCatalogInWorldState; use crate::state::HostSkillsStepState; use crate::state::SkillsSessionState; use crate::state::SkillsThreadState; diff --git a/codex-rs/ext/skills/src/host_prompt.rs b/codex-rs/ext/skills/src/host_prompt.rs new file mode 100644 index 0000000000..27d8140c23 --- /dev/null +++ b/codex-rs/ext/skills/src/host_prompt.rs @@ -0,0 +1,97 @@ +use std::collections::HashSet; + +use codex_extension_api::ContextualUserFragment; +use codex_skills::SkillMetadata; +use codex_skills::normalize_skill_path; + +use crate::HostSkillsSnapshot; +use crate::fragments::SkillInstructions; +use crate::render::truncate_main_prompt_contents; + +/// Host skill prompts already supplied or superseded by an extension. +/// +/// Core preserves its host skill invocation lifecycle while avoiding duplicate +/// prompts and retaining executor/orchestrator skill precedence. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct InjectedHostSkillPrompts { + paths: HashSet, +} + +impl InjectedHostSkillPrompts { + pub fn insert_path(&mut self, path: impl Into) { + let path = path.into(); + self.paths.insert(normalize_host_skill_path(&path)); + self.paths.insert(path); + } + + pub fn is_empty(&self) -> bool { + self.paths.is_empty() + } + + pub fn contains_path(&self, path: &str) -> bool { + self.paths.contains(path) || self.paths.contains(&normalize_host_skill_path(path)) + } +} + +/// Prompt fragments and read outcomes for a set of selected host skills. +pub struct HostSkillPrompts { + pub fragments: Vec>, + pub injected: Vec, + pub warnings: Vec, +} + +fn normalize_host_skill_path(path: &str) -> String { + normalize_skill_path(path).replace('\\', "/") +} + +impl HostSkillsSnapshot { + /// Reads selected host skills and builds their model-visible prompt fragments. + /// + /// Core calls this directly, including for hosts without an installed skills extension. + #[tracing::instrument( + level = "trace", + skip_all, + fields(selected_skill_count = selected_skills.len()) + )] + pub async fn load_skill_prompts(&self, selected_skills: &[SkillMetadata]) -> HostSkillPrompts { + let mut prompts = HostSkillPrompts { + fragments: Vec::with_capacity(selected_skills.len()), + injected: Vec::with_capacity(selected_skills.len()), + warnings: Vec::new(), + }; + + for skill in selected_skills { + match self.read_skill_text(skill).await { + Ok(contents) => { + let (contents, truncated) = if self.outcome().is_agent_plugin_skill(skill) { + truncate_main_prompt_contents(&contents) + } else { + (contents, false) + }; + if truncated { + prompts.warnings.push(format!( + "Skill `{}` exceeded the main prompt context limit and was truncated.", + skill.name + )); + } + prompts.fragments.push(Box::new(SkillInstructions { + name: skill.name.clone(), + path: skill.path_to_skills_md.to_string_lossy().into_owned(), + contents, + resource_access: None, + })); + prompts.injected.push(skill.clone()); + } + Err(err) => { + prompts.warnings.push(format!( + "Failed to load skill {} at {}: {err:#}", + skill.name, + skill.path_to_skills_md.display() + )); + } + } + } + + prompts + } +} diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 4833056abc..b657abde1b 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -6,6 +6,7 @@ mod dynamic_skill_selector; mod extension; mod fragments; mod host_aliases; +mod host_prompt; mod host_roots; mod host_service; mod host_snapshot; @@ -26,6 +27,8 @@ pub use config::SkillsExtensionConfig; pub use extension::install; pub use extension::install_with_providers; pub use extension::install_with_providers_and_metrics; +pub use host_prompt::HostSkillPrompts; +pub use host_prompt::InjectedHostSkillPrompts; pub use host_service::HostSkillsLoadInput; pub use host_service::HostSkillsService; pub use host_service::bundled_skills_enabled_from_stack; @@ -37,3 +40,10 @@ pub use provider::OrchestratorSkillProvider; pub use provider::SkillProvider; pub use sources::SkillProviderSource; pub use sources::SkillProviders; + +/// Recognizes persisted explicit skill prompts without exposing their fragment implementation. +pub fn is_skill_prompt_fragment(text: &str) -> bool { + ::matches_text( + text, + ) +} diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index b199a9dcf9..48e735da12 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -20,6 +20,16 @@ use crate::catalog_prompt::render_available_skills_body; use crate::loader::HostSkillRoot; use crate::loader::load_and_merge_host_skill_roots; +#[test] +fn skill_prompt_contents_are_bounded_at_utf8_boundaries() { + let contents = format!("{}é", "a".repeat(MAX_SKILL_PROMPT_BYTES - 1)); + + let (bounded, truncated) = truncate_main_prompt_contents(&contents); + + assert_eq!(bounded.len(), MAX_SKILL_PROMPT_BYTES - 1); + assert_eq!(truncated, true); +} + fn entry(name: &str, description: &str, short_description: Option<&str>) -> SkillCatalogEntry { entry_with_path( name, diff --git a/codex-rs/ext/skills/src/selection.rs b/codex-rs/ext/skills/src/selection.rs index 946115e5c3..ba28ee866f 100644 --- a/codex-rs/ext/skills/src/selection.rs +++ b/codex-rs/ext/skills/src/selection.rs @@ -1,7 +1,7 @@ use std::collections::HashSet; -use codex_core_skills::injection::extract_tool_mentions; use codex_protocol::user_input::UserInput; +use codex_skills::extract_tool_mentions; use crate::catalog::SkillAuthority; use crate::catalog::SkillCatalog; diff --git a/codex-rs/ext/skills/src/state.rs b/codex-rs/ext/skills/src/state.rs index b741ba47b8..88f851a524 100644 --- a/codex-rs/ext/skills/src/state.rs +++ b/codex-rs/ext/skills/src/state.rs @@ -384,6 +384,9 @@ pub(crate) struct SkillsTurnState { pub(crate) main_prompts_injected: bool, } +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(crate) struct HostSkillsCatalogInWorldState; + #[derive(Clone, Debug, Default)] pub(crate) struct ExecutorSkillsStepState(pub(crate) SkillCatalog); diff --git a/codex-rs/ext/skills/src/world_state_catalogs.rs b/codex-rs/ext/skills/src/world_state_catalogs.rs index 74cb9787a7..8c685a1bce 100644 --- a/codex-rs/ext/skills/src/world_state_catalogs.rs +++ b/codex-rs/ext/skills/src/world_state_catalogs.rs @@ -1,6 +1,5 @@ use std::sync::Arc; -use codex_core_skills::injection::HostSkillsCatalogInWorldState; use codex_extension_api::ContextualUserFragment; use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionWarning; @@ -22,6 +21,7 @@ use crate::render_observability::record_catalog_render; use crate::sources::SkillProviders; use crate::state::EmittedCatalogBudgetWarnings; use crate::state::ExecutorSkillsStepState; +use crate::state::HostSkillsCatalogInWorldState; use crate::state::HostSkillsStepState; use crate::state::SkillsSessionState; use crate::state::SkillsThreadState; diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 9e6b1ee925..59b9ad990c 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -7,7 +7,6 @@ use std::sync::atomic::Ordering; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; use codex_core_skills::SkillLoadOutcome; -use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_exec_server::LOCAL_FS; use codex_extension_api::ConversationHistory; use codex_extension_api::ExtensionData; @@ -46,6 +45,7 @@ use codex_skills_extension::HostSkillProvider; use codex_skills_extension::HostSkillsLoadInput; use codex_skills_extension::HostSkillsService; use codex_skills_extension::HostSkillsSnapshot; +use codex_skills_extension::InjectedHostSkillPrompts; use codex_skills_extension::SkillProviders; use codex_skills_extension::SkillsExtensionConfig; use codex_skills_extension::catalog::SkillAuthority;