diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index cec0333d7100..b05739595d7d 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -46,6 +46,7 @@ use crate::render::MAX_SKILL_NAME_BYTES; use crate::render::MAX_SKILL_PATH_BYTES; use crate::render::SkillCatalogRenderPolicy; use crate::render::SkillMetadataBudget; +use crate::render::SkillRenderReport; use crate::render::capped_skill_metadata_budget; use crate::render::render_available_skills; use crate::render::truncate_main_prompt_contents; @@ -87,6 +88,12 @@ fn render_catalog( budget: SkillMetadataBudget, ) -> RenderedCatalog { let Some(rendered) = render_available_skills(catalog, policy, budget) else { + record_catalog_render( + extension_metrics, + catalog_surface, + budget, + &SkillRenderReport::default(), + ); return RenderedCatalog::default(); }; record_catalog_render(extension_metrics, catalog_surface, budget, &rendered.report); @@ -98,6 +105,10 @@ fn render_catalog( } } +#[cfg(test)] +#[path = "extension_tests.rs"] +mod tests; + impl ThreadLifecycleContributor for SkillsExtension where C: Send + Sync + 'static, diff --git a/codex-rs/ext/skills/src/extension_tests.rs b/codex-rs/ext/skills/src/extension_tests.rs new file mode 100644 index 000000000000..109db2a91aa5 --- /dev/null +++ b/codex-rs/ext/skills/src/extension_tests.rs @@ -0,0 +1,57 @@ +use std::sync::Mutex; + +use codex_extension_api::ExtensionMetrics; +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; +use codex_otel::THREAD_SKILLS_TRUNCATED_METRIC; +use pretty_assertions::assert_eq; + +use super::*; + +#[derive(Default)] +struct RecordingMetrics { + samples: Mutex>, +} + +impl ExtensionMetrics for RecordingMetrics { + fn histogram(&self, name: &str, value: i64, _tags: &[(&str, &str)]) { + self.samples + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push((name.to_string(), value)); + } +} + +#[test] +fn empty_catalog_records_zero_metrics_without_a_fragment() { + let metrics = RecordingMetrics::default(); + + let rendered = render_catalog( + Some(&metrics), + CatalogSurface::ThreadContext, + &SkillCatalog::default(), + /*include_skills_usage_instructions*/ false, + SkillCatalogRenderPolicy::ExtensionCompatible, + SkillMetadataBudget::Characters(8_000), + ); + + assert!(rendered.fragment.is_none()); + assert_eq!(rendered.warning_message, None); + + assert_eq!( + *metrics + .samples + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner), + vec![ + (THREAD_SKILLS_ENABLED_TOTAL_METRIC.to_string(), 0), + (THREAD_SKILLS_KEPT_TOTAL_METRIC.to_string(), 0), + (THREAD_SKILLS_TRUNCATED_METRIC.to_string(), 0), + ( + THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC.to_string(), + 0, + ), + ] + ); +} diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index f17f56e6186e..6e9503c61315 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -88,7 +88,7 @@ pub(crate) enum SkillMetadataBudget { Characters(usize), } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Default, Clone, PartialEq, Eq)] pub(crate) struct SkillRenderReport { pub(crate) total_count: usize, pub(crate) included_count: usize, diff --git a/codex-rs/ext/skills/src/world_state.rs b/codex-rs/ext/skills/src/world_state.rs index 4bb6bc88db92..378760fdc256 100644 --- a/codex-rs/ext/skills/src/world_state.rs +++ b/codex-rs/ext/skills/src/world_state.rs @@ -97,7 +97,20 @@ pub(crate) fn host_skills_world_state_section( } else { None }; - let render_report = available.as_ref().map(|available| available.report.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, @@ -128,14 +141,16 @@ pub(crate) fn host_skills_world_state_section( } } - if let Some(report) = &render_report { + if let Some((total_count, included_count, omitted_count, truncated_description_chars)) = + render_metrics + { record_catalog_metrics( extension_metrics.as_deref(), CatalogSurface::HostWorldState, - report.total_count, - report.included_count, - report.omitted_count, - report.truncated_description_chars, + total_count, + included_count, + omitted_count, + truncated_description_chars, ); } let body = match body.as_deref() { diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 5bf255e52727..8ac611436983 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -41,6 +41,7 @@ use codex_protocol::protocol::TruncationPolicy; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::user_input::UserInput; use codex_skills::SkillMetadata; +use codex_skills_extension::HostSkillProvider; use codex_skills_extension::SkillProviders; use codex_skills_extension::SkillsExtensionConfig; use codex_skills_extension::catalog::SkillAuthority; @@ -196,6 +197,7 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes path_to_skills_md: skill_path, scope: SkillScope::User, plugin_id: None, + remote_plugin_id: None, }); let turn_store = ExtensionData::new("turn-1"); turn_store.insert(HostSkillsSnapshot::new(Arc::new(outcome.clone()))); @@ -222,19 +224,13 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes .render_diff(PreviousWorldStateSection::Absent) .is_some() ); - let expected_samples = |count| { - vec![ - (THREAD_SKILLS_ENABLED_TOTAL_METRIC.to_string(), count), - (THREAD_SKILLS_KEPT_TOTAL_METRIC.to_string(), count), - (THREAD_SKILLS_TRUNCATED_METRIC.to_string(), 0), - ( - THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC.to_string(), - 0, - ), - ] - }; + let mut expected = expected_catalog_metric_samples("executor_world_state", /*count*/ 0); + expected.extend(expected_catalog_metric_samples( + "host_world_state", + /*count*/ 1, + )); assert!(startup_metrics.samples().is_empty()); - assert_eq!(turn_metrics.samples(), expected_samples(1)); + assert_eq!(turn_metrics.samples(), expected); let sections = registry.context_contributors()[0] .contribute_world_state(WorldStateContributionInput { @@ -254,8 +250,12 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes .render_diff(PreviousWorldStateSection::Known(&published_snapshot)) .is_none() ); + expected.extend(expected_catalog_metric_samples( + "executor_world_state", + /*count*/ 0, + )); assert!(startup_metrics.samples().is_empty()); - assert_eq!(turn_metrics.samples(), expected_samples(1)); + assert_eq!(turn_metrics.samples(), expected); let second_skill_path = AbsolutePathBuf::try_from(test_codex_home().join("skills/other/SKILL.md"))?; @@ -269,6 +269,7 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes path_to_skills_md: second_skill_path, scope: SkillScope::User, plugin_id: None, + remote_plugin_id: None, }); turn_store.insert(HostSkillsSnapshot::new(Arc::new(outcome))); let sections = registry.context_contributors()[0] @@ -289,10 +290,102 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes .render_diff(PreviousWorldStateSection::Known(&published_snapshot)) .is_some() ); - let mut published_and_changed_samples = expected_samples(1); - published_and_changed_samples.extend(expected_samples(2)); + expected.extend(expected_catalog_metric_samples( + "executor_world_state", + /*count*/ 0, + )); + expected.extend(expected_catalog_metric_samples( + "host_world_state", + /*count*/ 2, + )); assert!(startup_metrics.samples().is_empty()); - assert_eq!(turn_metrics.samples(), published_and_changed_samples); + assert_eq!(turn_metrics.samples(), expected); + + Ok(()) +} + +#[tokio::test] +async fn nonempty_executor_empty_host_records_catalog_metrics() -> TestResult { + let executor_provider = Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries: vec![test_entry( + SkillSourceKind::Executor, + "env-1", + "executor/lint-fix", + "lint-fix/SKILL.md", + )], + warnings: Vec::new(), + }, + read_requests: Arc::new(Mutex::new(Vec::new())), + list_calls: None, + fail_first_list: false, + }); + let providers = SkillProviders::new() + .with_host_provider(Arc::new(HostSkillProvider::new())) + .with_executor_provider(executor_provider); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers(&mut builder, providers, skills_extension_config); + let registry = builder.build(); + let metrics = Arc::new(RecordingMetrics::default()); + 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 selected_roots = vec![SelectedCapabilityRoot { + id: "lint-fix".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "env-1".to_string(), + path: PathUri::parse("file:///skills/lint-fix").expect("skill root URI"), + }, + }]; + 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: &selected_roots, + executor_capability_discovery: None, + extension_metrics: Some(metrics.clone()), + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + }) + .await; + + assert_eq!(sections.len(), 2); + assert!( + sections[0] + .render_diff(PreviousWorldStateSection::Absent) + .is_some() + ); + assert!( + sections[1] + .render_diff(PreviousWorldStateSection::Absent) + .is_none() + ); + let mut expected = expected_catalog_metric_samples("executor_world_state", /*count*/ 1); + expected.extend(expected_catalog_metric_samples( + "host_world_state", + /*count*/ 0, + )); + assert_eq!(metrics.samples(), expected); Ok(()) } @@ -1420,13 +1513,20 @@ impl ExtensionEventSink for ChannelEventSink { } } +#[derive(Clone, Debug, Eq, PartialEq)] +struct RecordedHistogram { + name: String, + value: i64, + tags: Vec<(String, String)>, +} + #[derive(Default)] struct RecordingMetrics { - samples: Mutex>, + samples: Mutex>, } impl RecordingMetrics { - fn samples(&self) -> Vec<(String, i64)> { + fn samples(&self) -> Vec { self.samples .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) @@ -1435,14 +1535,47 @@ impl RecordingMetrics { } impl ExtensionMetrics for RecordingMetrics { - fn histogram(&self, name: &str, value: i64, _tags: &[(&str, &str)]) { + fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) { self.samples .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) - .push((name.to_string(), value)); + .push(RecordedHistogram { + name: name.to_string(), + value, + tags: tags + .iter() + .map(|(key, value)| ((*key).to_string(), (*value).to_string())) + .collect(), + }); } } +fn expected_catalog_metric_samples(catalog_surface: &str, count: i64) -> Vec { + let tags = vec![("catalog_surface".to_string(), catalog_surface.to_string())]; + vec![ + RecordedHistogram { + name: THREAD_SKILLS_ENABLED_TOTAL_METRIC.to_string(), + value: count, + tags: tags.clone(), + }, + RecordedHistogram { + name: THREAD_SKILLS_KEPT_TOTAL_METRIC.to_string(), + value: count, + tags: tags.clone(), + }, + RecordedHistogram { + name: THREAD_SKILLS_TRUNCATED_METRIC.to_string(), + value: 0, + tags: tags.clone(), + }, + RecordedHistogram { + name: THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC.to_string(), + value: 0, + tags, + }, + ] +} + impl SkillProvider for StaticSkillProvider { fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { let list_call = self