diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 51573fb14c..47e3c35b0f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4099,6 +4099,7 @@ dependencies = [ "codex-utils-path-uri", "codex-utils-string", "futures", + "opentelemetry_sdk", "pretty_assertions", "schemars 0.8.22", "serde", diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index f977003877..b5645db50c 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -567,7 +567,7 @@ async fn turn_start_emits_thread_scoped_warning_notification_for_trimmed_skills( assert_eq!(warning.thread_id.as_deref(), Some(thread.id.as_str())); assert_eq!( warning.message, - "Exceeded skills context budget of 2%. All skill descriptions were removed and 7 additional skills were not included in the model-visible skills list." + "Exceeded skills context budget. All skill descriptions were removed and 7 additional skills were not included in the model-visible skills list." ); timeout( diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index b8ea0767ea..3040e0bf10 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -11,6 +11,7 @@ use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ExtensionWarning; use codex_features::Feature; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; @@ -35,18 +36,24 @@ use codex_skills_extension::provider::SkillProviderFuture; use codex_skills_extension::provider::SkillReadRequest; use codex_skills_extension::provider::SkillSearchRequest; use codex_utils_string::approx_token_count; +use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::apps_test_server::apps_enabled_builder; use core_test_support::responses; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_once; use core_test_support::responses::sse; use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_mcp_server; use pretty_assertions::assert_eq; use tempfile::TempDir; +use tokio::time::Duration; +use tokio::time::Instant; use toml::toml; struct StaticSkillProvider { catalog: SkillCatalog, + main_prompt_contents: Option, } struct ExecutorSkillProvider { @@ -92,12 +99,18 @@ impl SkillProvider for StaticSkillProvider { Box::pin(async move { Ok(catalog) }) } - fn read(&self, _request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { - Box::pin(async { - Err(SkillProviderError::new( - "production-flow catalog test does not read skills", - )) - }) + fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { + let result = self + .main_prompt_contents + .clone() + .map(|contents| SkillReadResult { + resource: request.resource, + contents, + }) + .ok_or_else(|| { + SkillProviderError::new("production-flow catalog test does not read skills") + }); + Box::pin(async move { result }) } fn search(&self, _request: SkillSearchRequest) -> SkillProviderFuture<'_, SkillSearchResult> { @@ -461,7 +474,10 @@ async fn production_turn_scales_extension_catalog_from_resolved_model_window() - SkillProviders::new().with_provider(SkillProviderSource::new( source_kind, "test", - Arc::new(StaticSkillProvider { catalog }), + Arc::new(StaticSkillProvider { + catalog, + main_prompt_contents: None, + }), )), |config: &Config| SkillsExtensionConfig { include_instructions: config.include_skill_instructions, @@ -576,13 +592,212 @@ async fn production_turn_shares_catalog_budget_across_host_and_executor_sections async fn production_turn_keeps_full_host_only_catalog_when_it_fits() -> Result<()> { let (developer_texts, _) = rendered_catalogs(&HOST_CATALOG, &[], FULL_CATALOG_CONTEXT_WINDOW).await?; - let host_lines = skill_lines(catalog_text(&developer_texts, "host"), "host"); + let host_lines = developer_texts + .iter() + .flat_map(|text| skill_lines(text, "host")) + .collect::>(); assert_full_descriptions(&host_lines, &HOST_CATALOG); Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn production_turn_uses_provider_host_catalog_and_core_snapshot_injection() -> Result<()> { + let server = responses::start_mock_server().await; + let apps_server = AppsTestServer::mount(&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 skill_name = "snapshot-backed"; + let snapshot_description = "This description comes from Core's host skills snapshot."; + write_host_skills(codex_home.path(), &[(skill_name, snapshot_description)])?; + let skill_path = codex_home + .path() + .join("skills") + .join(skill_name) + .join("SKILL.md"); + let snapshot_contents = format!( + "---\nname: {skill_name}\ndescription: {snapshot_description}\n---\n\nUse $calendar.\n" + ); + std::fs::write(&skill_path, &snapshot_contents)?; + let skill_resource = skill_path.to_string_lossy().into_owned(); + let provider_description = "This skill comes from the extension host provider."; + let provider_contents = "# Provider instructions that Core must not inject."; + let provider_catalog = SkillCatalog { + entries: vec![ + SkillCatalogEntry::new( + SkillPackageId(skill_resource.clone()), + SkillAuthority::new(SkillSourceKind::Host, "host"), + skill_name, + provider_description, + SkillResourceId::new(skill_resource.clone()), + ) + .with_display_path(skill_resource.replace('\\', "/")), + ], + warnings: Vec::new(), + }; + let mut extensions = ExtensionRegistryBuilder::::new(); + install_with_providers( + &mut extensions, + SkillProviders::new().with_host_provider(Arc::new(StaticSkillProvider { + catalog: provider_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 = apps_enabled_builder(apps_server.chatgpt_base_url) + .with_home(Arc::clone(&codex_home)) + .with_extensions(Arc::new(extensions.build())) + .with_config(configure_catalog_test); + let test = builder.build_with_auto_env(&server).await?; + wait_for_mcp_server(&test.codex, CODEX_APPS_MCP_SERVER_NAME).await?; + + test.submit_turn(&format!("Use ${skill_name}.")).await?; + let request = response.single_request(); + let developer_texts = request.message_input_texts("developer"); + let cutover_skill_lines = developer_texts + .iter() + .flat_map(|text| text.lines()) + .filter(|line| line.contains(skill_name)) + .collect::>(); + + assert_eq!( + cutover_skill_lines, + vec![format!( + "- {skill_name}: {provider_description} (file: {})", + skill_resource.replace('\\', "/") + )] + ); + let user_text = request.message_input_texts("user").join("\n"); + assert!(user_text.contains(&snapshot_contents)); + assert!(!user_text.contains(provider_contents)); + let deadline = Instant::now() + Duration::from_secs(10); + let app_mentioned_event = loop { + let requests = server.received_requests().await.unwrap_or_default(); + if let Some(event) = requests + .into_iter() + .filter(|request| request.url.path() == "/codex/analytics-events/events") + .find_map(|request| { + let payload: serde_json::Value = serde_json::from_slice(&request.body).ok()?; + payload["events"].as_array().and_then(|events| { + events + .iter() + .find(|event| event["event_type"] == "codex_app_mentioned") + .cloned() + }) + }) + { + break event; + } + if Instant::now() >= deadline { + panic!("timed out waiting for app mentioned analytics"); + } + tokio::time::sleep(Duration::from_millis(50)).await; + }; + assert_eq!( + app_mentioned_event["event_params"]["connector_id"], + "calendar" + ); + assert_eq!( + app_mentioned_event["event_params"]["invoke_type"], + "explicit" + ); + 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; + 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 skill_dir = codex_home.path().join("skills").join("long-host"); + std::fs::create_dir_all(&skill_dir)?; + let prompt_tail = "full host prompt tail"; + let skill_contents = format!( + "---\nname: long-host\ndescription: Long host skill.\n---\n\n# Long host skill\n\n{}\n{prompt_tail}\n", + "x".repeat(8_000) + ); + std::fs::write(skill_dir.join("SKILL.md"), &skill_contents)?; + 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?; + + test.submit_turn("Use $long-host.").await?; + let user_text = response + .single_request() + .message_input_texts("user") + .join("\n"); + + assert!(user_text.contains(&skill_contents)); + assert_eq!( + user_text.matches("\nlong-host").count(), + 1 + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn production_turn_keeps_core_host_injection_when_catalog_listings_are_disabled() -> 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()?); + let skill_dir = codex_home.path().join("skills").join("long-host"); + std::fs::create_dir_all(&skill_dir)?; + let prompt_tail = "full host prompt tail"; + let skill_contents = format!( + "---\nname: long-host\ndescription: Long host skill.\n---\n\n# Long host skill\n\n{}\n{prompt_tail}\n", + "x".repeat(8_000) + ); + std::fs::write(skill_dir.join("SKILL.md"), &skill_contents)?; + 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) + .with_config(|config| { + config.include_skill_instructions = false; + }); + let test = builder.build_with_auto_env(&server).await?; + + test.submit_turn("Use $long-host.").await?; + let user_text = response + .single_request() + .message_input_texts("user") + .join("\n"); + + assert!(user_text.contains(&skill_contents)); + assert_eq!( + user_text.matches("\nlong-host").count(), + 1 + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn production_turn_keeps_full_executor_only_catalog_when_it_fits() -> Result<()> { let (developer_texts, _) = @@ -621,7 +836,7 @@ async fn production_turn_omits_host_skills_under_extreme_host_only_pressure() -> skill_names(&host_lines), vec!["host-alpha", "host-beta", "host-delta"] ); - let expected_warning = "Exceeded skills context budget of 2%. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list."; + let expected_warning = "Exceeded skills context budget. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list."; assert_eq!( warning_messages .iter() @@ -633,6 +848,36 @@ async fn production_turn_omits_host_skills_under_extreme_host_only_pressure() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn production_turn_preserves_empty_core_compatible_host_fragment() -> Result<()> { + let host_skills = [( + "host-a-b-c-d-e-f-g-h-i-j-k-l-m-n-o-p-q-r-s-t-u-v-w-x-y-z-a-b", + "Host-only skill.", + )]; + let (developer_texts, warning_messages) = + rendered_catalogs(&host_skills, &[], /*context_window*/ 1_000).await?; + let host_fragment = developer_texts + .iter() + .find(|text| text.contains("## Skills")) + .unwrap_or_else(|| { + panic!( + "production request should preserve the empty host skills fragment, got {developer_texts:?}" + ) + }); + + assert!(!host_fragment.contains(&format!("- {}:", host_skills[0].0))); + assert!(!host_fragment.contains("## Host skills update")); + assert_eq!( + warning_messages, + vec![ + "Exceeded skills context budget. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list." + .to_string() + ] + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn successive_turns_do_not_repeat_unchanged_host_budget_warning() -> Result<()> { let (_, warning_messages) = rendered_catalogs_for_turns( @@ -642,7 +887,7 @@ async fn successive_turns_do_not_repeat_unchanged_host_budget_warning() -> Resul /*turn_count*/ 2, ) .await?; - let expected_warning = "Exceeded skills context budget of 2%. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list."; + let expected_warning = "Exceeded skills context budget. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list."; assert_eq!( warning_messages @@ -757,7 +1002,10 @@ async fn production_turn_fairly_shortens_extension_catalog_descriptions() -> Res SkillProviders::new().with_provider(SkillProviderSource::new( source_kind, "test", - Arc::new(StaticSkillProvider { catalog }), + Arc::new(StaticSkillProvider { + catalog, + main_prompt_contents: None, + }), )), |config: &Config| SkillsExtensionConfig { include_instructions: config.include_skill_instructions, diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index 59c793bca4..43518bd826 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -34,5 +34,6 @@ url = { workspace = true } [dev-dependencies] codex-models-manager = { workspace = true } codex-utils-absolute-path = { workspace = true } +opentelemetry_sdk = { workspace = true } pretty_assertions = { workspace = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 451ad4f711..835e213635 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -64,6 +64,7 @@ use crate::shadow_selection_experiment::ShadowSelectionExperiment; use crate::sources::SkillProviders; use crate::state::EmittedCatalogBudgetWarnings; use crate::state::ExecutorSkillsStepState; +use crate::state::HostSkillsStepState; use crate::state::SkillsSessionState; use crate::state::SkillsThreadState; use crate::state::SkillsTurnState; @@ -72,7 +73,6 @@ use crate::warnings::bounded_warnings; use crate::world_state::HostSkillsWarningEmitter; use crate::world_state::executor_skills_world_state_section; use crate::world_state::host_skills_world_state_section; -use crate::world_state::rendered_host_skills_world_state_section; struct SkillsExtension { providers: SkillProviders, @@ -283,42 +283,50 @@ where let metadata_budget = skill_metadata_budget(context_window); let extension_metrics = input.extension_metrics.clone(); let host_snapshot = input.turn_store.get::(); - let shared_rendered = if config.include_instructions + let needs_host_catalog = config.include_instructions || config.shadow_selection_enabled; + let host_catalog = if needs_host_catalog && let Some(host_snapshot) = host_snapshot.as_ref() && self.providers.has_host_provider() { - let host_catalog = self - .providers - .list_host_for_turn(SkillListQuery { - turn_id: input.turn_id.to_string(), - executor_roots: Vec::new(), - resolved_executor_roots: Vec::new(), - host_snapshot: Some(Arc::clone(host_snapshot)), - include_host_skills: true, - include_bundled_skills: false, - include_orchestrator_skills: false, - mcp_resources: None, - executor_capability_discovery: None, - }) - .await; - let (host_rendered, executor_rendered) = - render_combined_available_skills(&host_catalog, &catalog, metadata_budget); - if host_catalog + Some( + self.providers + .list_host_for_turn(SkillListQuery { + turn_id: input.turn_id.to_string(), + executor_roots: Vec::new(), + resolved_executor_roots: Vec::new(), + host_snapshot: Some(Arc::clone(host_snapshot)), + include_host_skills: true, + include_bundled_skills: false, + include_orchestrator_skills: false, + mcp_resources: None, + executor_capability_discovery: None, + }) + .await, + ) + } else { + None + }; + if let Some(host_catalog) = host_catalog.as_ref() { + input + .turn_store + .insert(HostSkillsStepState(host_catalog.clone())); + } + let host_catalog_for_rendering = if config.include_instructions { + host_catalog.as_ref() + } else { + None + }; + let shared_rendered = host_catalog_for_rendering.and_then(|host_catalog| { + (host_catalog .entries .iter() .any(SkillCatalogEntry::is_model_visible) && catalog .entries .iter() - .any(SkillCatalogEntry::is_model_visible) - { - Some((host_rendered, executor_rendered)) - } else { - None - } - } else { - None - }; + .any(SkillCatalogEntry::is_model_visible)) + .then(|| render_combined_available_skills(host_catalog, &catalog, metadata_budget)) + }); let (rendered, rendered_host) = if let Some((host_rendered, executor_rendered)) = shared_rendered { if let Some(host_rendered) = host_rendered.as_ref() { @@ -335,6 +343,16 @@ where host_rendered, ) } else { + let rendered_host = host_catalog_for_rendering.and_then(|host_catalog| { + render_available_skills( + host_catalog, + SkillCatalogRenderPolicy::CoreCompatible, + metadata_budget, + ) + }); + if let Some(rendered_host) = rendered_host.as_ref() { + trace_catalog_budget_pressure(metadata_budget, &rendered_host.report); + } ( if config.include_instructions { render_catalog( @@ -348,12 +366,29 @@ where } else { RenderedCatalog::default() }, - None, + rendered_host, ) }; let host_warning_message = rendered_host .as_ref() .and_then(|rendered_host| rendered_host.report.warning_message()); + let host_render_metrics = config.include_instructions.then(|| { + rendered_host + .as_ref() + .map(|rendered_host| { + let report = &rendered_host.report; + ( + report.total_count, + report.included_count, + report.omitted_count, + report.truncated_description_chars, + ) + }) + .unwrap_or_default() + }); + let host_body = rendered_host + .and_then(|rendered_host| rendered_host.into_fragment(include_usage)) + .map(|fragment| fragment.body()); let executor_body = rendered.fragment.map(|fragment| fragment.body()); let mut sections = vec![executor_skills_world_state_section( executor_body, @@ -362,9 +397,7 @@ where let emitted_warnings = input .turn_store .get_or_init(EmittedCatalogBudgetWarnings::default); - if let Some(host_snapshot) = host_snapshot - && self.providers.has_host_provider() - { + if host_snapshot.is_some() && self.providers.has_host_provider() { input.turn_store.insert(HostSkillsCatalogInWorldState); let event_sink = Arc::clone(&self.event_sink); let emitted_warnings = Arc::clone(&emitted_warnings); @@ -379,35 +412,14 @@ where }); } }); - if let Some(rendered_host) = rendered_host { - let report = &rendered_host.report; - let render_metrics = Some(( - report.total_count, - report.included_count, - report.omitted_count, - report.truncated_description_chars, - )); - let body = rendered_host - .into_fragment(include_usage) - .map(|fragment| fragment.body()); - sections.push(rendered_host_skills_world_state_section( - body, - config.include_instructions, - render_metrics, - extension_metrics, - host_warning_message, - warning_emitter, - )); - } else { - sections.push(host_skills_world_state_section( - &host_snapshot, - config.include_instructions, - include_usage, - metadata_budget, - extension_metrics, - warning_emitter, - )); - } + sections.push(host_skills_world_state_section( + host_body, + config.include_instructions, + host_render_metrics, + extension_metrics, + host_warning_message, + warning_emitter, + )); } if let Some(message) = rendered.warning_message && emitted_warnings.insert(&message) @@ -521,18 +533,18 @@ where let host_snapshot = turn_store.get::(); let host_catalog_in_world_state = turn_store.get::().is_some(); + let host_skills = turn_store.get::(); let query = SkillListQuery { turn_id: input.turn_id.clone(), executor_roots: Vec::new(), resolved_executor_roots: Vec::new(), host_snapshot: host_snapshot.clone(), - include_host_skills: !host_catalog_in_world_state, + include_host_skills: host_skills.is_none() && !host_catalog_in_world_state, include_bundled_skills: config.bundled_skills_enabled, include_orchestrator_skills: thread_state.orchestrator_skills_enabled(), mcp_resources: mcp_resources.clone(), executor_capability_discovery: None, }; - let host_query = query.clone(); let mut catalog = turn_store .get::() .map(|executor_skills| executor_skills.0.clone()) @@ -545,8 +557,8 @@ where let selected_entries = collect_explicit_skill_mentions(&input.user_input, &catalog); let shadow_selection_turn = if config.shadow_selection_enabled { let mut shadow_catalog = catalog.clone(); - if host_catalog_in_world_state && host_snapshot.is_some() { - shadow_catalog.extend(self.providers.list_host_for_turn(host_query).await); + if let Some(host_skills) = host_skills { + shadow_catalog.extend(host_skills.0.clone()); } let shadow_selected_entries = collect_explicit_skill_mentions(&input.user_input, &shadow_catalog); @@ -562,9 +574,7 @@ where thread_state .replace_shadow_selection_turn(input.turn_id.clone(), shadow_selection_turn); let mut fragments: Vec> = Vec::new(); - if config.include_instructions - && turn_store.get::().is_none() - { + if config.include_instructions && !host_catalog_in_world_state { let mut turn_catalog = catalog.clone(); turn_catalog.entries.retain(|entry| { entry.authority.kind != SkillSourceKind::Executor diff --git a/codex-rs/ext/skills/src/fragments.rs b/codex-rs/ext/skills/src/fragments.rs index 041fb5b3b7..c26d18535b 100644 --- a/codex-rs/ext/skills/src/fragments.rs +++ b/codex-rs/ext/skills/src/fragments.rs @@ -1,4 +1,3 @@ -use codex_core_skills::AvailableSkills; use codex_core_skills::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS; use codex_core_skills::SKILLS_HOW_TO_USE_WITH_ALIASES; use codex_core_skills::render_available_skills_body; @@ -32,26 +31,6 @@ impl AvailableSkillsInstructions { skill_lines, } } - - pub(crate) fn from_available_skills( - available: AvailableSkills, - include_skills_usage_instructions: bool, - ) -> Self { - let mut skill_lines = available.skill_lines; - if include_skills_usage_instructions { - skill_lines.push("### How to use skills".to_string()); - let instructions = if available.skill_root_lines.is_empty() { - SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS - } else { - SKILLS_HOW_TO_USE_WITH_ALIASES - }; - skill_lines.push(instructions.to_string()); - } - Self { - skill_root_lines: available.skill_root_lines, - skill_lines, - } - } } impl ContextualUserFragment for AvailableSkillsInstructions { diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 3bdd33dd02..c8b36fc1e3 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -440,6 +440,7 @@ struct RenderedCatalog { pub(crate) struct AvailableSkillsRender { skill_root_lines: Vec, skill_lines: Vec, + preserve_empty_fragment: bool, pub(crate) report: SkillRenderReport, } @@ -448,7 +449,7 @@ impl AvailableSkillsRender { self, include_skills_usage_instructions: bool, ) -> Option { - (!self.skill_lines.is_empty()).then(|| { + (self.preserve_empty_fragment || !self.skill_lines.is_empty()).then(|| { AvailableSkillsInstructions::from_skill_lines( self.skill_root_lines, self.skill_lines, @@ -501,6 +502,7 @@ pub(crate) fn render_available_skills( Some(AvailableSkillsRender { skill_root_lines: selected.skill_root_lines, skill_lines: selected.skill_lines, + preserve_empty_fragment: policy == SkillCatalogRenderPolicy::CoreCompatible, report: selected.report, }) } @@ -615,6 +617,7 @@ fn render_combined_group( AvailableSkillsRender { skill_root_lines, skill_lines: lines.into_iter().map(|rendered| rendered.line).collect(), + preserve_empty_fragment: false, report: SkillRenderReport { total_count: skill_lines.len(), included_count: skill_lines.len().saturating_sub(omitted_count), diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index 2904439ab4..29eb36ff33 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -387,6 +387,10 @@ fn mixed_catalog_reserves_executor_omission_marker_by_omitting_host_first() { "- 1 additional skill omitted from this bounded skills list.".to_string(), ] ); + assert!( + host.into_fragment(/*include_skills_usage_instructions*/ false) + .is_none() + ); } #[test] @@ -630,10 +634,11 @@ fn catalog_emits_omission_marker_when_every_minimum_skill_line_exceeds_budget() ) .expect("core-compatible report should render"); assert_eq!(core_render.report, expected_report); - assert_eq!( - core_render.into_fragment(/*include_skills_usage_instructions*/ false), - None - ); + let core_fragment = core_render + .into_fragment(/*include_skills_usage_instructions*/ false) + .expect("core-compatible rendering should preserve an empty skills fragment"); + assert!(core_fragment.body().contains("## Skills")); + assert!(!core_fragment.body().contains("- oversized:")); let render = render_available_skills( &catalog, SkillCatalogRenderPolicy::ExtensionCompatible, diff --git a/codex-rs/ext/skills/src/state.rs b/codex-rs/ext/skills/src/state.rs index 3f83a0857f..d2ea009bf1 100644 --- a/codex-rs/ext/skills/src/state.rs +++ b/codex-rs/ext/skills/src/state.rs @@ -375,3 +375,6 @@ pub(crate) struct SkillsTurnState { #[derive(Clone, Debug, Default)] pub(crate) struct ExecutorSkillsStepState(pub(crate) SkillCatalog); + +#[derive(Clone, Debug, Default)] +pub(crate) struct HostSkillsStepState(pub(crate) SkillCatalog); diff --git a/codex-rs/ext/skills/src/world_state.rs b/codex-rs/ext/skills/src/world_state.rs index 1ef108412d..d34055707f 100644 --- a/codex-rs/ext/skills/src/world_state.rs +++ b/codex-rs/ext/skills/src/world_state.rs @@ -1,9 +1,5 @@ use std::sync::Arc; -use codex_core_skills::HostSkillsSnapshot; -use codex_core_skills::build_available_skills; -use codex_core_skills::render::SkillRenderSideEffects; -use codex_extension_api::ContextualUserFragment; use codex_extension_api::ExtensionMetrics; use codex_extension_api::PreviousWorldStateSection; use codex_extension_api::RenderedWorldStateFragment; @@ -12,8 +8,6 @@ use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; use serde_json::json; -use crate::fragments::AvailableSkillsInstructions; -use crate::render::SkillMetadataBudget; use crate::render_observability::CatalogSurface; use crate::render_observability::record_catalog_metrics; @@ -82,60 +76,6 @@ pub(crate) fn executor_skills_world_state_section( } pub(crate) fn host_skills_world_state_section( - host_snapshot: &HostSkillsSnapshot, - include_instructions: bool, - include_skills_usage_instructions: bool, - metadata_budget: SkillMetadataBudget, - extension_metrics: Option>, - warning_emitter: HostSkillsWarningEmitter, -) -> WorldStateSectionContribution { - let outcome = host_snapshot.outcome(); - let metadata_budget = match metadata_budget { - SkillMetadataBudget::Tokens(limit) => codex_core_skills::SkillMetadataBudget::Tokens(limit), - SkillMetadataBudget::Characters(limit) => { - codex_core_skills::SkillMetadataBudget::Characters(limit) - } - }; - let available = if include_instructions { - build_available_skills(outcome, metadata_budget, SkillRenderSideEffects::None) - } else { - None - }; - let warning_message = available - .as_ref() - .and_then(|available| available.warning_message.clone()); - let render_metrics = include_instructions.then(|| { - available - .as_ref() - .map(|available| { - let report = &available.report; - ( - report.total_count, - report.included_count, - report.omitted_count, - report.truncated_description_chars, - ) - }) - .unwrap_or_default() - }); - let body = available.map(|available| { - AvailableSkillsInstructions::from_available_skills( - available, - include_skills_usage_instructions, - ) - .body() - }); - rendered_host_skills_world_state_section( - body, - include_instructions, - render_metrics, - extension_metrics, - warning_message, - warning_emitter, - ) -} - -pub(crate) fn rendered_host_skills_world_state_section( body: Option, include_instructions: bool, render_metrics: Option<(usize, usize, usize, usize)>, diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index a3d96a2ca0..e2c35896d3 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -26,6 +26,8 @@ use codex_extension_api::ToolPayload; use codex_extension_api::TurnInputContext; use codex_extension_api::WorldStateContributionInput; use codex_models_manager::model_info::model_info_from_slug; +use codex_otel::MetricsClient; +use codex_otel::MetricsConfig; use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC; use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_KEPT_TOTAL_METRIC; @@ -55,6 +57,7 @@ use codex_skills_extension::catalog::SkillSearchResult; use codex_skills_extension::catalog::SkillSourceKind; use codex_skills_extension::install; use codex_skills_extension::install_with_providers; +use codex_skills_extension::install_with_providers_and_metrics; use codex_skills_extension::provider::SkillListQuery; use codex_skills_extension::provider::SkillProvider; use codex_skills_extension::provider::SkillProviderFuture; @@ -62,6 +65,9 @@ use codex_skills_extension::provider::SkillReadRequest; use codex_skills_extension::provider::SkillSearchRequest; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use opentelemetry_sdk::metrics::InMemoryMetricExporter; +use opentelemetry_sdk::metrics::data::AggregatedMetrics; +use opentelemetry_sdk::metrics::data::MetricData; use pretty_assertions::assert_eq; use tokio::sync::Semaphore; @@ -492,7 +498,190 @@ async fn nonempty_executor_empty_host_records_catalog_metrics() -> TestResult { /*count*/ 0, )); assert_eq!(metrics.samples(), expected); + Ok(()) +} +#[tokio::test] +async fn host_world_state_uses_provider_catalog_with_core_compatible_rendering() -> TestResult { + let list_calls = Arc::new(AtomicUsize::new(0)); + let provider = Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries: vec![ + test_entry( + SkillSourceKind::Host, + "host", + "host/provider-skill", + "provider-skill/SKILL.md", + ) + .with_short_description(Some("Short description.".to_string())), + ], + warnings: Vec::new(), + }, + read_requests: Arc::new(Mutex::new(Vec::new())), + list_calls: Some(Arc::clone(&list_calls)), + fail_first_list: false, + }); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers( + &mut builder, + SkillProviders::new().with_host_provider(provider), + skills_extension_config, + ); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + let config = default_config(); + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &config, + session_source: &SessionSource::Cli, + persistent_thread_state_available: true, + environments: &[], + mcp_resource_client: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + let turn_store = ExtensionData::new("turn-1"); + turn_store.insert(HostSkillsSnapshot::new(Arc::new( + SkillLoadOutcome::default(), + ))); + + let sections = registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + thread_id: codex_protocol::ThreadId::new(), + turn_id: "turn-1", + environments: &[], + ready_selected_capability_roots: &[], + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + }) + .await; + let host_fragment = sections[1] + .render_diff(PreviousWorldStateSection::Absent) + .ok_or("host provider catalog should render")?; + + assert!(host_fragment.body().contains("Fix lint errors.")); + assert!(!host_fragment.body().contains("Short description.")); + assert_eq!(1, list_calls.load(Ordering::Relaxed)); + Ok(()) +} + +#[tokio::test] +async fn shadow_selection_uses_host_catalog_when_instructions_are_disabled() -> TestResult { + let list_calls = Arc::new(AtomicUsize::new(0)); + let provider = Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries: vec![test_entry( + SkillSourceKind::Host, + "host", + "host/lint-fix", + "lint-fix/SKILL.md", + )], + warnings: Vec::new(), + }, + read_requests: Arc::new(Mutex::new(Vec::new())), + list_calls: Some(Arc::clone(&list_calls)), + fail_first_list: false, + }); + let metrics = MetricsClient::new( + MetricsConfig::in_memory( + "test", + "codex-skills-extension", + env!("CARGO_PKG_VERSION"), + InMemoryMetricExporter::default(), + ) + .with_runtime_reader(), + )?; + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers_and_metrics( + &mut builder, + SkillProviders::new().with_host_provider(provider), + Some(metrics.clone()), + skills_extension_config, + ); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + let mut config = default_config(); + config.include_instructions = false; + config.shadow_selection_enabled = true; + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &config, + session_source: &SessionSource::Cli, + persistent_thread_state_available: true, + environments: &[], + mcp_resource_client: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + let turn_store = ExtensionData::new("turn-1"); + turn_store.insert(HostSkillsSnapshot::new(Arc::new( + SkillLoadOutcome::default(), + ))); + + let sections = registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + thread_id: codex_protocol::ThreadId::new(), + turn_id: "turn-1", + environments: &[], + ready_selected_capability_roots: &[], + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + }) + .await; + let fragments = registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + turn_id: "turn-1".to_string(), + user_input: vec![UserInput::Text { + text: "Fix lint errors.".to_string(), + text_elements: Vec::new(), + }], + environments: Vec::new(), + }, + /*extension_metrics*/ None, + &session_store, + &thread_store, + &turn_store, + ) + .await; + + assert!( + sections[1] + .render_diff(PreviousWorldStateSection::Absent) + .is_none() + ); + assert!(fragments.is_empty()); + let snapshot = metrics.snapshot()?; + let catalog_entry_counts = snapshot + .scope_metrics() + .flat_map(opentelemetry_sdk::metrics::data::ScopeMetrics::metrics) + .find(|metric| metric.name() == "codex.skills.shadow_selection.catalog_entries") + .map(|metric| match metric.data() { + AggregatedMetrics::F64(MetricData::Histogram(histogram)) => histogram + .data_points() + .map(opentelemetry_sdk::metrics::data::HistogramDataPoint::sum) + .collect::>(), + data => panic!("unexpected shadow catalog metric data: {data:?}"), + }) + .ok_or("shadow catalog metric should be recorded")?; + + assert!( + catalog_entry_counts.iter().all(|count| *count == 1.0), + "every shadow selector should see the cached host skill: {catalog_entry_counts:?}" + ); + assert_eq!(1, list_calls.load(Ordering::Relaxed)); Ok(()) }