From 3d7f9b463795cdbdd1f243cdf679c9276c3e1da6 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 12 Aug 2026 15:12:13 +0000 Subject: [PATCH] Fuse recent and lexical skills in shadow selection (#38204) ## What changed - Add the `lru_plus_lexical_v1` selector, combining the 50 most recent skills with weighted lexical matches through reciprocal rank fusion. - Include the combined selector in the shadow-selection experiment and propagate query and candidate truncation metadata from both inputs. - Generalize the existing reciprocal-rank fusion helper to accept a rank constant while preserving the existing lexical/character selector behavior. ## Testing - Cover lexical-only selection, merging recent and matching skills, promotion from agreement between both rankings, and weak overlaps. - Extend the skills extension test to verify invocation metrics for the new selector alongside `lru_v1`. GitOrigin-RevId: 1b7b82bc97ea919c719cd05a009351b701019306 --- .../ext/skills/src/dynamic_skill_selector.rs | 2 + .../lru_plus_lexical.rs | 60 +++++++++ .../lru_plus_lexical_tests.rs | 119 ++++++++++++++++++ .../rrf_lexical_char.rs | 10 +- .../skills/src/shadow_selection_experiment.rs | 5 +- codex-rs/ext/skills/tests/skills_extension.rs | 16 ++- 6 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_lexical.rs create mode 100644 codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_lexical_tests.rs diff --git a/codex-rs/ext/skills/src/dynamic_skill_selector.rs b/codex-rs/ext/skills/src/dynamic_skill_selector.rs index fa71e01f85..8f3ac48a74 100644 --- a/codex-rs/ext/skills/src/dynamic_skill_selector.rs +++ b/codex-rs/ext/skills/src/dynamic_skill_selector.rs @@ -2,6 +2,7 @@ mod character_ngram; mod character_routing_card; mod fielded_bm25; mod lru; +mod lru_plus_lexical; mod multi_query_lexical; mod routing_card_lexical; mod rrf_lexical_char; @@ -11,6 +12,7 @@ pub(crate) use character_routing_card::CharacterRoutingCardSkillSelector; use codex_skills::SkillDependencies; pub(crate) use fielded_bm25::FieldedBm25SkillSelector; pub(crate) use lru::LruSkillSelector; +pub(crate) use lru_plus_lexical::LruPlusLexicalSkillSelector; pub(crate) use multi_query_lexical::MultiQueryLexicalSkillSelector; pub(crate) use routing_card_lexical::RoutingCardLexicalSkillSelector; pub(crate) use rrf_lexical_char::RrfLexicalCharSkillSelector; diff --git a/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_lexical.rs b/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_lexical.rs new file mode 100644 index 0000000000..82330e3e31 --- /dev/null +++ b/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_lexical.rs @@ -0,0 +1,60 @@ +use super::CheapSkillSelection; +use super::CheapSkillSelector; +use super::LruSkillSelector; +use super::SkillSelectionDocument; +use super::WeightedLexicalSkillSelector; +use super::rrf_lexical_char::fuse_rankings_with_constant; + +const MAX_RESULTS: usize = 50; +const RRF_K: usize = 10; + +#[derive(Clone, Debug, Default)] +pub(crate) struct LruPlusLexicalSkillSelector { + lru_selector: LruSkillSelector, +} + +impl LruPlusLexicalSkillSelector { + pub(crate) fn new(lru_selector: LruSkillSelector) -> Self { + Self { lru_selector } + } +} + +impl CheapSkillSelector for LruPlusLexicalSkillSelector { + fn method(&self) -> &'static str { + "lru_plus_lexical_v1" + } + + fn select( + &self, + query: &str, + documents: &[SkillSelectionDocument<'_>], + limit: usize, + ) -> CheapSkillSelection { + if limit == 0 { + return CheapSkillSelection::default(); + } + + let recent = self + .lru_selector + .select(query, documents, /*limit*/ MAX_RESULTS); + let lexical = + WeightedLexicalSkillSelector.select(query, documents, /*limit*/ MAX_RESULTS); + let candidate_ids = fuse_rankings_with_constant( + [&recent.candidate_ids, &lexical.candidate_ids], + limit.min(MAX_RESULTS), + /*rank_constant*/ RRF_K, + ); + + CheapSkillSelection { + candidate_ids, + query_term_count: recent.query_term_count.max(lexical.query_term_count), + query_truncated: recent.query_truncated || lexical.query_truncated, + candidate_set_truncated: recent.candidate_set_truncated + || lexical.candidate_set_truncated, + } + } +} + +#[cfg(test)] +#[path = "lru_plus_lexical_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_lexical_tests.rs b/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_lexical_tests.rs new file mode 100644 index 0000000000..eea431c34d --- /dev/null +++ b/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_lexical_tests.rs @@ -0,0 +1,119 @@ +use pretty_assertions::assert_eq; + +use super::*; + +#[test] +fn combined_selector_uses_lexical_results_before_invocation_history_exists() { + let documents = [ + SkillSelectionDocument { + id: 10, + name: "ci", + short_description: None, + description: "Investigate failing checks.", + dependencies: None, + }, + SkillSelectionDocument { + id: 20, + name: "python-tools", + short_description: None, + description: "Manage Python environments.", + dependencies: None, + }, + ]; + + let selection = LruPlusLexicalSkillSelector::new(LruSkillSelector::default()).select( + "manage python environments", + &documents, + /*limit*/ 50, + ); + + assert_eq!(vec![20], selection.candidate_ids); +} + +#[test] +fn combined_selector_merges_recent_and_newly_matching_skills() { + let documents = [ + SkillSelectionDocument { + id: 10, + name: "ci", + short_description: None, + description: "Investigate failing checks.", + dependencies: None, + }, + SkillSelectionDocument { + id: 20, + name: "python-tools", + short_description: None, + description: "Manage Python environments.", + dependencies: None, + }, + SkillSelectionDocument { + id: 30, + name: "monorepo", + short_description: None, + description: "Follow repository conventions.", + dependencies: None, + }, + ]; + + let selection = LruPlusLexicalSkillSelector::new(LruSkillSelector::new(vec![30, 10])).select( + "manage python environments", + &documents, + /*limit*/ 50, + ); + + assert_eq!(vec![20, 30, 10], selection.candidate_ids); +} + +#[test] +fn combined_selector_promotes_skills_supported_by_both_signals() { + let documents = [ + SkillSelectionDocument { + id: 10, + name: "ci", + short_description: None, + description: "Investigate failing checks.", + dependencies: None, + }, + SkillSelectionDocument { + id: 30, + name: "monorepo", + short_description: None, + description: "Follow repository conventions.", + dependencies: None, + }, + ]; + + let selection = LruPlusLexicalSkillSelector::new(LruSkillSelector::new(vec![30, 10])).select( + "investigate failing ci checks", + &documents, + /*limit*/ 50, + ); + + assert_eq!(vec![10, 30], selection.candidate_ids); +} + +#[test] +fn combined_fusion_keeps_a_top_ranked_skill_ahead_of_a_weak_overlap() { + let recent = std::iter::once(1) + .chain(100..148) + .chain(std::iter::once(2)) + .collect::>(); + let lexical = (200..249).chain(std::iter::once(2)).collect::>(); + + let fused = fuse_rankings_with_constant( + [&recent, &lexical], + /*limit*/ MAX_RESULTS, + /*rank_constant*/ RRF_K, + ); + let top_ranked = fused + .iter() + .position(|candidate| *candidate == 1) + .expect("top-ranked skill should be selected"); + let weak_overlap = fused + .iter() + .position(|candidate| *candidate == 2) + .expect("weak overlapping skill should be selected"); + + assert!(top_ranked < weak_overlap); +} diff --git a/codex-rs/ext/skills/src/dynamic_skill_selector/rrf_lexical_char.rs b/codex-rs/ext/skills/src/dynamic_skill_selector/rrf_lexical_char.rs index c09b29c6c5..3171fe10e1 100644 --- a/codex-rs/ext/skills/src/dynamic_skill_selector/rrf_lexical_char.rs +++ b/codex-rs/ext/skills/src/dynamic_skill_selector/rrf_lexical_char.rs @@ -53,12 +53,20 @@ impl CheapSkillSelector for RrfLexicalCharSkillSelector { } fn fuse_rankings(rankings: [&[usize]; N], limit: usize) -> Vec { + fuse_rankings_with_constant(rankings, limit, RRF_K) +} + +pub(super) fn fuse_rankings_with_constant( + rankings: [&[usize]; N], + limit: usize, + rank_constant: usize, +) -> Vec { let mut scores = HashMap::::new(); let mut best_ranks = HashMap::::new(); for ranking in rankings { for (index, id) in ranking.iter().copied().enumerate() { let rank = index + 1; - *scores.entry(id).or_default() += 1.0 / (RRF_K + rank) as f64; + *scores.entry(id).or_default() += 1.0 / (rank_constant + rank) as f64; best_ranks .entry(id) .and_modify(|best_rank| *best_rank = (*best_rank).min(rank)) diff --git a/codex-rs/ext/skills/src/shadow_selection_experiment.rs b/codex-rs/ext/skills/src/shadow_selection_experiment.rs index 556ab58543..c2b6f15a79 100644 --- a/codex-rs/ext/skills/src/shadow_selection_experiment.rs +++ b/codex-rs/ext/skills/src/shadow_selection_experiment.rs @@ -21,6 +21,7 @@ use crate::dynamic_skill_selector::CharacterRoutingCardSkillSelector; use crate::dynamic_skill_selector::CheapSkillSelection; use crate::dynamic_skill_selector::CheapSkillSelector; use crate::dynamic_skill_selector::FieldedBm25SkillSelector; +use crate::dynamic_skill_selector::LruPlusLexicalSkillSelector; use crate::dynamic_skill_selector::LruSkillSelector; use crate::dynamic_skill_selector::MultiQueryLexicalSkillSelector; use crate::dynamic_skill_selector::RoutingCardLexicalSkillSelector; @@ -120,7 +121,8 @@ impl ShadowSelectionExperiment { .collect(); let routing_selector = CharacterRoutingCardSkillSelector::new(catalog, host_snapshot); let lru_selector = LruSkillSelector::new(recent_skill_ids); - let mut ranked_selections = Vec::with_capacity(self.selectors.len() + 2); + let lru_plus_lexical_selector = LruPlusLexicalSkillSelector::new(lru_selector.clone()); + let mut ranked_selections = Vec::with_capacity(self.selectors.len() + 3); for selector in self .selectors @@ -129,6 +131,7 @@ impl ShadowSelectionExperiment { .chain([ &routing_selector as &dyn CheapSkillSelector, &lru_selector as &dyn CheapSkillSelector, + &lru_plus_lexical_selector as &dyn CheapSkillSelector, ]) { let start = Instant::now(); diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index eb4bcf38cb..f8c320c57a 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -932,7 +932,7 @@ async fn shadow_lru_selector_recovers_a_skill_invoked_on_an_earlier_turn() -> Te .flat_map(opentelemetry_sdk::metrics::data::ScopeMetrics::metrics) .find(|metric| metric.name() == "codex.skills.shadow_selection.invocation") .ok_or("shadow invocation metric should be recorded")?; - let mut lru_hits = match metric.data() { + let mut selector_hits = match metric.data() { AggregatedMetrics::U64(MetricData::Sum(sum)) => sum .data_points() .filter_map(|point| { @@ -941,7 +941,7 @@ async fn shadow_lru_selector_recovers_a_skill_invoked_on_an_earlier_turn() -> Te .find(|attribute| attribute.key.as_str() == "method")? .value .as_str(); - if method != "lru_v1" { + if method != "lru_v1" && method != "lru_plus_lexical_v1" { return None; } let hit = point @@ -950,16 +950,20 @@ async fn shadow_lru_selector_recovers_a_skill_invoked_on_an_earlier_turn() -> Te .value .as_str() .to_string(); - Some((hit, point.value())) + Some((method.to_string(), hit, point.value())) }) .collect::>(), data => panic!("unexpected shadow invocation metric data: {data:?}"), }; - lru_hits.sort(); + selector_hits.sort(); assert_eq!( - vec![("false".to_string(), 1), ("true".to_string(), 1)], - lru_hits + vec![ + ("lru_plus_lexical_v1".to_string(), "true".to_string(), 2), + ("lru_v1".to_string(), "false".to_string(), 1), + ("lru_v1".to_string(), "true".to_string(), 1), + ], + selector_hits ); Ok(()) }