From ea10ae7d0fe6a2fb0614a32adb5e095853319355 Mon Sep 17 00:00:00 2001 From: jif Date: Mon, 17 Aug 2026 12:08:22 +0000 Subject: [PATCH] Evaluate recent and character-routed skill selection (#38993) ## What changed - Add shadow selectors that fuse recently invoked skills with character routing-card matches using reciprocal rank fusion. - Add a second variant that also incorporates weighted lexical matches. - Record both variants in the existing shadow-selection experiment, capped at 50 candidates. ## Testing - Cover cold starts, duplicate removal, short exact matches, deterministic ranking, result limits, and truncation metadata. - Extend the skills integration test to verify both selectors recover a skill invoked on an earlier turn. GitOrigin-RevId: 4ff9e91da8cb49ecbce54ab8e760820616e1a1d3 --- .../ext/skills/src/dynamic_skill_selector.rs | 3 + .../lru_plus_character_routing.rs | 136 +++++++++++++ .../lru_plus_character_routing_tests.rs | 179 ++++++++++++++++++ .../skills/src/shadow_selection_experiment.rs | 14 +- codex-rs/ext/skills/tests/skills_extension.rs | 18 +- 5 files changed, 348 insertions(+), 2 deletions(-) create mode 100644 codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_character_routing.rs create mode 100644 codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_character_routing_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 8f3ac48a74..6a2db5e473 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_character_routing; mod lru_plus_lexical; mod multi_query_lexical; mod routing_card_lexical; @@ -12,6 +13,8 @@ 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_character_routing::LruPlusCharacterRoutingSkillSelector; +pub(crate) use lru_plus_character_routing::LruPlusLexicalCharacterRoutingSkillSelector; pub(crate) use lru_plus_lexical::LruPlusLexicalSkillSelector; pub(crate) use multi_query_lexical::MultiQueryLexicalSkillSelector; pub(crate) use routing_card_lexical::RoutingCardLexicalSkillSelector; diff --git a/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_character_routing.rs b/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_character_routing.rs new file mode 100644 index 0000000000..e2ba103c1a --- /dev/null +++ b/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_character_routing.rs @@ -0,0 +1,136 @@ +use super::CharacterRoutingCardSkillSelector; +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 LruPlusCharacterRoutingSkillSelector { + lru_selector: LruSkillSelector, + character_selector: CharacterRoutingCardSkillSelector, +} + +impl LruPlusCharacterRoutingSkillSelector { + pub(crate) fn new( + lru_selector: LruSkillSelector, + character_selector: CharacterRoutingCardSkillSelector, + ) -> Self { + Self { + lru_selector, + character_selector, + } + } +} + +impl CheapSkillSelector for LruPlusCharacterRoutingSkillSelector { + fn method(&self) -> &'static str { + "lru_plus_character_routing_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 character = + self.character_selector + .select(query, documents, /*limit*/ MAX_RESULTS); + let candidate_ids = fuse_rankings_with_constant( + [&recent.candidate_ids, &character.candidate_ids], + limit.min(MAX_RESULTS), + /*rank_constant*/ RRF_K, + ); + + CheapSkillSelection { + candidate_ids, + query_term_count: recent.query_term_count.max(character.query_term_count), + query_truncated: recent.query_truncated || character.query_truncated, + candidate_set_truncated: recent.candidate_set_truncated + || character.candidate_set_truncated, + } + } +} + +#[derive(Clone, Debug, Default)] +pub(crate) struct LruPlusLexicalCharacterRoutingSkillSelector { + lru_selector: LruSkillSelector, + character_selector: CharacterRoutingCardSkillSelector, +} + +impl LruPlusLexicalCharacterRoutingSkillSelector { + pub(crate) fn new( + lru_selector: LruSkillSelector, + character_selector: CharacterRoutingCardSkillSelector, + ) -> Self { + Self { + lru_selector, + character_selector, + } + } +} + +impl CheapSkillSelector for LruPlusLexicalCharacterRoutingSkillSelector { + fn method(&self) -> &'static str { + "lru_plus_lexical_character_routing_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 character = + self.character_selector + .select(query, documents, /*limit*/ MAX_RESULTS); + let candidate_ids = fuse_rankings_with_constant( + [ + &recent.candidate_ids, + &lexical.candidate_ids, + &character.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) + .max(character.query_term_count), + query_truncated: recent.query_truncated + || lexical.query_truncated + || character.query_truncated, + candidate_set_truncated: recent.candidate_set_truncated + || lexical.candidate_set_truncated + || character.candidate_set_truncated, + } + } +} + +#[cfg(test)] +#[path = "lru_plus_character_routing_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_character_routing_tests.rs b/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_character_routing_tests.rs new file mode 100644 index 0000000000..17e4513c83 --- /dev/null +++ b/codex-rs/ext/skills/src/dynamic_skill_selector/lru_plus_character_routing_tests.rs @@ -0,0 +1,179 @@ +use codex_skills::SkillDependencies; +use codex_skills::SkillToolDependency; +use pretty_assertions::assert_eq; + +use super::*; + +#[test] +fn cold_start_uses_character_routing_metadata() { + let dependencies = SkillDependencies { + tools: vec![SkillToolDependency { + r#type: "app".to_string(), + value: "slack".to_string(), + description: None, + transport: None, + command: None, + url: None, + oauth_callback_port: None, + }], + }; + let documents = [ + document(/*id*/ 10, "compilation", "Compile Rust crates."), + SkillSelectionDocument { + dependencies: Some(&dependencies), + ..document(/*id*/ 20, "team-communication", "Share team updates.") + }, + ]; + let expected = CheapSkillSelection { + candidate_ids: vec![20], + query_term_count: 1, + ..Default::default() + }; + + for selector in selectors(Vec::new(), CharacterRoutingCardSkillSelector::default()) { + let method = selector.method(); + assert_eq!( + expected, + selector.select("slack", &documents, /*limit*/ 50), + "{method}" + ); + } +} + +#[test] +fn recent_and_text_matches_are_merged_without_duplicates() { + let documents = [ + document(/*id*/ 10, "rust-build", "Inspect compiler errors."), + document( + /*id*/ 20, + "python-tools", + "Manage Python environments.", + ), + document(/*id*/ 30, "monorepo", "Repository conventions."), + ]; + let expected = CheapSkillSelection { + candidate_ids: vec![20, 30, 10], + query_term_count: 1, + ..Default::default() + }; + + for selector in selectors( + vec![30, 10, 20, 20, 999], + CharacterRoutingCardSkillSelector::default(), + ) { + let method = selector.method(); + assert_eq!( + expected, + selector.select("python", &documents, /*limit*/ 50), + "{method}" + ); + } +} + +#[test] +fn three_way_fusion_retains_short_exact_matches() { + let documents = [ + document(/*id*/ 10, "ci", ""), + document(/*id*/ 20, "python", ""), + document(/*id*/ 30, "rust", ""), + ]; + let expected_rankings = [vec![20, 30], vec![20, 10, 30]]; + + for (selector, candidate_ids) in + selectors(vec![30], CharacterRoutingCardSkillSelector::default()) + .into_iter() + .zip(expected_rankings) + { + let method = selector.method(); + assert_eq!( + CheapSkillSelection { + candidate_ids, + query_term_count: 2, + ..Default::default() + }, + selector.select("ci python", &documents, /*limit*/ 50), + "{method}" + ); + } +} + +#[test] +fn fusion_is_deterministic_capped_and_reports_input_bounds() { + let documents = (0..1_001) + .map(|id| document(id, "python", "")) + .collect::>(); + let query = "python ".repeat(4 * 1024); + let expected_rankings = [ + [0, 999].into_iter().chain(1..49).collect::>(), + (0..11) + .chain(std::iter::once(999)) + .chain(11..49) + .collect::>(), + ]; + + for (selector, candidate_ids) in selectors( + vec![1_000, 999, 999, 1_002], + CharacterRoutingCardSkillSelector::default(), + ) + .into_iter() + .zip(expected_rankings) + { + let method = selector.method(); + let expected = CheapSkillSelection { + candidate_ids, + query_term_count: 64, + query_truncated: true, + candidate_set_truncated: true, + }; + assert_eq!( + expected, + selector.select(&query, &documents, /*limit*/ 50), + "{method}" + ); + assert_eq!( + expected, + selector.select(&query, &documents, usize::MAX), + "{method}" + ); + assert_eq!( + CheapSkillSelection { + candidate_ids: expected.candidate_ids[..7].to_vec(), + ..expected + }, + selector.select(&query, &documents, /*limit*/ 7), + "{method}" + ); + assert_eq!( + CheapSkillSelection::default(), + selector.select(&query, &documents, /*limit*/ 0), + "{method}" + ); + } +} + +fn selectors( + recent_skill_ids: Vec, + character_selector: CharacterRoutingCardSkillSelector, +) -> [Box; 2] { + let lru_selector = LruSkillSelector::new(recent_skill_ids); + [ + Box::new(LruPlusCharacterRoutingSkillSelector::new( + lru_selector.clone(), + character_selector.clone(), + )), + Box::new(LruPlusLexicalCharacterRoutingSkillSelector::new( + lru_selector, + character_selector, + )), + ] +} + +fn document<'a>(id: usize, name: &'a str, description: &'a str) -> SkillSelectionDocument<'a> { + SkillSelectionDocument { + id, + name, + short_description: None, + description, + dependencies: None, + } +} diff --git a/codex-rs/ext/skills/src/shadow_selection_experiment.rs b/codex-rs/ext/skills/src/shadow_selection_experiment.rs index c2b6f15a79..7abc98b781 100644 --- a/codex-rs/ext/skills/src/shadow_selection_experiment.rs +++ b/codex-rs/ext/skills/src/shadow_selection_experiment.rs @@ -21,6 +21,8 @@ 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::LruPlusCharacterRoutingSkillSelector; +use crate::dynamic_skill_selector::LruPlusLexicalCharacterRoutingSkillSelector; use crate::dynamic_skill_selector::LruPlusLexicalSkillSelector; use crate::dynamic_skill_selector::LruSkillSelector; use crate::dynamic_skill_selector::MultiQueryLexicalSkillSelector; @@ -122,7 +124,15 @@ impl ShadowSelectionExperiment { let routing_selector = CharacterRoutingCardSkillSelector::new(catalog, host_snapshot); let lru_selector = LruSkillSelector::new(recent_skill_ids); let lru_plus_lexical_selector = LruPlusLexicalSkillSelector::new(lru_selector.clone()); - let mut ranked_selections = Vec::with_capacity(self.selectors.len() + 3); + let lru_plus_character_selector = LruPlusCharacterRoutingSkillSelector::new( + lru_selector.clone(), + routing_selector.clone(), + ); + let lru_plus_lexical_character_selector = LruPlusLexicalCharacterRoutingSkillSelector::new( + lru_selector.clone(), + routing_selector.clone(), + ); + let mut ranked_selections = Vec::with_capacity(self.selectors.len() + 5); for selector in self .selectors @@ -132,6 +142,8 @@ impl ShadowSelectionExperiment { &routing_selector as &dyn CheapSkillSelector, &lru_selector as &dyn CheapSkillSelector, &lru_plus_lexical_selector as &dyn CheapSkillSelector, + &lru_plus_character_selector as &dyn CheapSkillSelector, + &lru_plus_lexical_character_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 52192c35de..8c4aaf8b44 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -948,7 +948,13 @@ 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" && method != "lru_plus_lexical_v1" { + if !matches!( + method.as_ref(), + "lru_v1" + | "lru_plus_lexical_v1" + | "lru_plus_character_routing_v1" + | "lru_plus_lexical_character_routing_v1" + ) { return None; } let hit = point @@ -966,6 +972,16 @@ async fn shadow_lru_selector_recovers_a_skill_invoked_on_an_earlier_turn() -> Te assert_eq!( vec![ + ( + "lru_plus_character_routing_v1".to_string(), + "true".to_string(), + 2, + ), + ( + "lru_plus_lexical_character_routing_v1".to_string(), + "true".to_string(), + 2, + ), ("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),