From 0327a3ec34fd8fe9454eb57151a3798f127a53bf Mon Sep 17 00:00:00 2001 From: Omer Strulovich Date: Thu, 5 Nov 2026 17:22:07 +0000 Subject: [PATCH] Prioritize filename matches in fuzzy file search (#59709) ## Why Matches in parent directory names can crowd matching filenames out of limited search results. For example, searching for `ViewModel` should keep `ChatViewModel.kt` visible even when many directories are named `viewmodel`. ## What changed - Rank basename matches before applying the result limit for single positive fuzzy queries without path separators. - Prefer exact filenames, then the strongest exact directory match, then other matching filenames. Leave additional directory matches ranked by their paths. - Compute highlight indices from the basename for promoted results and return scores that reflect the new ranking. - Rank snapshots in resumable time slices so new queries and cancellation can interrupt the work. ## Testing Add regression coverage for filename matches surviving the result limit, exact file and directory priority, path queries, and resuming partial rankings. Update app-server score expectations while preserving highlight indices. GitOrigin-RevId: 41b3420581b4f6490859e809245370b737889544 --- .../tests/suite/fuzzy_file_search.rs | 9 +- codex-rs/file-search/src/lib.rs | 38 ++-- codex-rs/file-search/src/lib_tests.rs | 44 +++++ codex-rs/file-search/src/matcher_tests.rs | 64 ++++--- codex-rs/file-search/src/snapshot.rs | 181 ++++++++++++++++-- 5 files changed, 270 insertions(+), 66 deletions(-) diff --git a/codex-rs/app-server/tests/suite/fuzzy_file_search.rs b/codex-rs/app-server/tests/suite/fuzzy_file_search.rs index 34dd3cd9dc..af7233640c 100644 --- a/codex-rs/app-server/tests/suite/fuzzy_file_search.rs +++ b/codex-rs/app-server/tests/suite/fuzzy_file_search.rs @@ -262,7 +262,8 @@ async fn test_fuzzy_file_search_sorts_and_includes_indices() -> Result<()> { .await??; let value = resp.result; - let expected_score = 72; + // Each basename matches, so its score includes one filename priority bonus. + let filename_bonus = u32::from(u16::MAX) + 1; assert_eq!( value, @@ -273,7 +274,7 @@ async fn test_fuzzy_file_search_sorts_and_includes_indices() -> Result<()> { "path": "abexy", "match_type": "file", "file_name": "abexy", - "score": 84, + "score": filename_bonus + 84, "indices": [0, 1, 2], }, { @@ -281,7 +282,7 @@ async fn test_fuzzy_file_search_sorts_and_includes_indices() -> Result<()> { "path": sub_abce_rel, "match_type": "file", "file_name": "abce", - "score": expected_score, + "score": filename_bonus + 72, "indices": [4, 5, 7], }, { @@ -289,7 +290,7 @@ async fn test_fuzzy_file_search_sorts_and_includes_indices() -> Result<()> { "path": "abcde", "match_type": "file", "file_name": "abcde", - "score": 71, + "score": filename_bonus + 71, "indices": [0, 1, 4], }, ] diff --git a/codex-rs/file-search/src/lib.rs b/codex-rs/file-search/src/lib.rs index fd92dbd88c..6b750d09c3 100644 --- a/codex-rs/file-search/src/lib.rs +++ b/codex-rs/file-search/src/lib.rs @@ -41,7 +41,7 @@ pub use cli::Cli; /// A single match result returned from the search. /// -/// * `score` – Relevance score returned by `nucleo`. +/// * `score` – Relevance score used to select and order this result. /// * `path` – Path to the matched entry (file or directory), relative to the /// search directory. /// * `match_type` – Whether this match is a file or directory. @@ -520,7 +520,7 @@ fn matcher_worker( ) -> anyhow::Result<()> { const TICK_TIMEOUT_MS: u64 = 10; let config = Config::DEFAULT.match_paths(); - let mut indices_matcher = inner.compute_indices.then(|| Matcher::new(config.clone())); + let mut matcher = Matcher::new(config); let cancel_requested = || inner.cancelled.load(Ordering::Relaxed); let shutdown_requested = || inner.shutdown.load(Ordering::Relaxed); @@ -528,6 +528,7 @@ fn matcher_worker( let mut next_notify = never(); let mut will_notify = false; let mut walk_complete = false; + let mut ranking = None; loop { select! { @@ -537,6 +538,7 @@ fn matcher_worker( }; match signal { WorkSignal::QueryUpdated(query) => { + ranking = None; let append = query.starts_with(&last_query); nucleo.pattern.reparse( 0, @@ -569,20 +571,26 @@ fn matcher_worker( } recv(next_notify) -> _ => { will_notify = false; - let status = nucleo.tick(TICK_TIMEOUT_MS); - if status.changed - && let Some(snapshot) = snapshot::for_query( - &inner, - &nucleo, - &last_query, - &mut indices_matcher, - walk_complete, - ) - { - inner.reporter.on_update(&snapshot); + if ranking.is_none() { + let status = nucleo.tick(TICK_TIMEOUT_MS); + if status.changed { + ranking = snapshot::Ranking::start(&nucleo, &last_query, walk_complete); + } + if ranking.is_none() && !status.running && walk_complete { + inner.reporter.on_complete(); + } } - if !status.running && walk_complete { - inner.reporter.on_complete(); + if let Some(active) = &mut ranking { + if let Some(snapshot) = active.advance( + &inner, &nucleo, &last_query, &mut matcher, + Duration::from_millis(TICK_TIMEOUT_MS), + ) { + inner.reporter.on_update(&snapshot); + ranking = None; + } + // Resume this snapshot or collect work that arrived while ranking it. + will_notify = true; + next_notify = after(Duration::ZERO); } } default(Duration::from_millis(100)) => { diff --git a/codex-rs/file-search/src/lib_tests.rs b/codex-rs/file-search/src/lib_tests.rs index c2202b63ae..d2b78b4e57 100644 --- a/codex-rs/file-search/src/lib_tests.rs +++ b/codex-rs/file-search/src/lib_tests.rs @@ -47,6 +47,50 @@ fn tie_breakers_sort_by_path_when_scores_equal() { assert_eq!(matches, expected); } +#[test] +fn matching_filenames_survive_the_path_result_limit() { + let root = tempfile::tempdir().unwrap(); + let folder = root.path().join("viewmodel"); + fs::create_dir(&folder).unwrap(); + fs::write(folder.join("build.gradle.kts"), "").unwrap(); + let source = Path::new("feature").join("src"); + fs::create_dir_all(root.path().join(&source)).unwrap(); + let filename = source.join("ChatViewModel.kt"); + fs::write(root.path().join(&filename), "").unwrap(); + + let search = |query, limit| { + run( + query, + vec![root.path().to_path_buf()], + FileSearchOptions { + limit: NonZero::new(limit).unwrap(), + ..Default::default() + }, + /*cancel_flag*/ None, + ) + .unwrap() + .matches + }; + for i in 0..80 { + fs::create_dir_all(root.path().join(format!("parent-{i}/viewmodel"))).unwrap(); + } + let hits = search("ViewModel", 2); + assert_eq!(hits[0].match_type, MatchType::Directory); + assert_eq!(hits[1].path, filename); + assert_eq!(search("VM", 1)[0].path, filename); + fs::create_dir(root.path().join("src")).unwrap(); + for i in 0..80 { + fs::write(root.path().join(format!("source-{i}.rs")), "").unwrap(); + } + assert_eq!(search("src", 1)[0].path, Path::new("src")); + fs::write(root.path().join(&source).join("src"), "").unwrap(); + assert_eq!(search("src", 1)[0].path, source.join("src")); + assert_eq!( + search(&format!("viewmodel{}", std::path::MAIN_SEPARATOR), 1)[0].path, + Path::new("viewmodel").join("build.gradle.kts") + ); +} + #[test] fn file_name_from_path_uses_basename() { assert_eq!(file_name_from_path("foo/bar.txt"), "bar.txt"); diff --git a/codex-rs/file-search/src/matcher_tests.rs b/codex-rs/file-search/src/matcher_tests.rs index 551cc1858b..c7bb357e0d 100644 --- a/codex-rs/file-search/src/matcher_tests.rs +++ b/codex-rs/file-search/src/matcher_tests.rs @@ -40,15 +40,17 @@ fn query_update_does_not_relabel_pending_matches() { reporter: Arc::new(tests::RecordingReporter::default()), work_tx, }; - let mut indices_matcher = Some(Matcher::new(Config::DEFAULT.match_paths())); - let initial = snapshot::for_query( - &inner, - &nucleo, - "", - &mut indices_matcher, - /*walk_complete*/ true, - ) - .unwrap(); + let mut matcher = Matcher::new(Config::DEFAULT.match_paths()); + let snapshot = |nucleo: &Nucleo, query: &str, matcher: &mut Matcher| { + snapshot::Ranking::start(nucleo, query, /*walk_complete*/ true)?.advance( + &inner, + nucleo, + query, + matcher, + Duration::from_secs(5), + ) + }; + let initial = snapshot(&nucleo, "", &mut matcher).unwrap(); assert_eq!(initial.matches.len(), 2); nucleo.pattern.reparse( @@ -60,32 +62,17 @@ fn query_update_does_not_relabel_pending_matches() { ); // The requested pattern has changed, but no tick has replaced the previous // snapshot. This is the same intermediate state a busy tick can expose. - assert_eq!( - snapshot::for_query( - &inner, - &nucleo, - "bet", - &mut indices_matcher, - /*walk_complete*/ true - ), - None - ); + assert_eq!(snapshot(&nucleo, "bet", &mut matcher), None); while nucleo.tick(/*timeout*/ 10).running { work_rx.recv_timeout(Duration::from_secs(5)).unwrap(); } assert_eq!( - snapshot::for_query( - &inner, - &nucleo, - "bet", - &mut indices_matcher, - /*walk_complete*/ true - ), + snapshot(&nucleo, "bet", &mut matcher), Some(FileSearchSnapshot { query: "bet".to_string(), matches: vec![FileMatch { - score: 84, + score: 65_536 + 84, path: PathBuf::from("beta.txt"), match_type: MatchType::File, root: root.path().to_path_buf(), @@ -96,4 +83,27 @@ fn query_update_does_not_relabel_pending_matches() { walk_complete: true, }), ); + // Force the pending ranking to span worker turns, then resume it on the same snapshot. + for i in 0..256 { + injector.push( + IndexedEntry::Ascii { + root_idx: 0, + match_type: MatchType::File, + }, + |_, columns| columns[0] = Utf32String::from(format!("bet/path-{i}")), + ); + } + while nucleo.tick(/*timeout*/ 10).running { + work_rx.recv_timeout(Duration::from_secs(5)).unwrap(); + } + let mut partial = snapshot::Ranking::start(&nucleo, "bet", /*walk_complete*/ true).unwrap(); + assert!( + partial + .advance(&inner, &nucleo, "bet", &mut matcher, Duration::ZERO) + .is_none() + ); + assert_eq!( + partial.advance(&inner, &nucleo, "bet", &mut matcher, Duration::from_secs(5)), + snapshot(&nucleo, "bet", &mut matcher) + ); } diff --git a/codex-rs/file-search/src/snapshot.rs b/codex-rs/file-search/src/snapshot.rs index 513010f560..a012c0eea7 100644 --- a/codex-rs/file-search/src/snapshot.rs +++ b/codex-rs/file-search/src/snapshot.rs @@ -1,34 +1,162 @@ -//! Converts matcher snapshots without associating old results with a new query. +//! Ranks a stable matcher snapshot in slices so the worker can handle new queries. use crate::FileMatch; use crate::FileSearchSnapshot; use crate::IndexedEntry; +use crate::MatchType; use crate::SessionInner; +use nucleo::Match; use nucleo::Matcher; use nucleo::Nucleo; +use nucleo::Utf32Str; use nucleo::Utf32String; +use nucleo::pattern::AtomKind; +use std::cmp::Reverse; +use std::collections::BinaryHeap; use std::path::PathBuf; +use std::time::Duration; +use std::time::Instant; -pub(super) fn for_query( +// A single Nucleo atom returns a u16. Leave room for its full score within each group. +const NAME_BONUS: u32 = u16::MAX as u32 + 1; + +#[derive(Default)] +pub(super) struct Ranking { + next: usize, + best: BinaryHeap<(Reverse, usize)>, + found_exact_folder: bool, + rank_names: bool, + walk_complete: bool, +} + +impl Ranking { + pub(super) fn start( + nucleo: &Nucleo, + query: &str, + walk_complete: bool, + ) -> Option { + let pattern = nucleo.snapshot().pattern().column_pattern(0); + // A tick can collect the previous query while the new one is running. + if pattern.atoms != nucleo.pattern.column_pattern(0).atoms { + return None; + } + let rank_names = matches!(pattern.atoms.as_slice(), [atom] if atom.kind == AtomKind::Fuzzy && !atom.negative) + && !query.contains(['/', '\\']); + Some(Self { + rank_names, + walk_complete, + ..Self::default() + }) + } + + /// Yields to the worker between slices so a new query or cancellation can preempt this snapshot. + pub(super) fn advance( + &mut self, + inner: &SessionInner, + nucleo: &Nucleo, + query: &str, + matcher: &mut Matcher, + budget: Duration, + ) -> Option { + let snapshot = nucleo.snapshot(); + let ranked = snapshot.matches(); + if !self.rank_names { + return Some(collect( + inner, + nucleo, + query, + matcher, + self.walk_complete, + ranked + .iter() + .copied() + .take(inner.limit) + .map(|matched| (matched, false)), + )); + } + let name = &snapshot.pattern().column_pattern(0).atoms[0]; + let deadline = Instant::now() + budget; + for (matched, item) in ranked[self.next..] + .iter() + .zip(snapshot.matched_items(self.next as u32..)) + { + let kind = match item.data { + IndexedEntry::Ascii { match_type, .. } => *match_type, + IndexedEntry::Unicode(entry) => entry.match_type, + }; + let basename = basename(item.matcher_columns[0].slice(..)); + let exact_length = basename.len() == name.needle_text().len(); + let score = match kind { + MatchType::File => name.score(basename, matcher).map(|score| { + let bonus = if exact_length { + 3 * NAME_BONUS + } else { + NAME_BONUS + }; + bonus + u32::from(score) + }), + // Keep the strongest exact folder. More folders with the same name + // compete in path order so they cannot crowd out matching files. + MatchType::Directory + if !self.found_exact_folder + && exact_length + && name.score(basename, matcher).is_some() => + { + self.found_exact_folder = true; + Some(2 * NAME_BONUS + matched.score) + } + MatchType::Directory => None, + } + .unwrap_or(matched.score); + // The heap head is the lowest score, then the latest path on a tie. + let candidate = (Reverse(score), self.next); + if self.best.len() < inner.limit { + self.best.push(candidate); + } else if let Some(mut worst) = self.best.peek_mut() + && candidate < *worst + { + *worst = candidate; + } + self.next += 1; + if self.next.is_multiple_of(64) && Instant::now() >= deadline { + return None; + } + } + let selected = std::mem::take(&mut self.best) + .into_sorted_vec() + .into_iter() + .map(|(Reverse(score), index)| { + ( + Match { + score, + idx: ranked[index].idx, + }, + score >= NAME_BONUS, + ) + }); + Some(collect( + inner, + nucleo, + query, + matcher, + self.walk_complete, + selected, + )) + } +} + +fn collect( inner: &SessionInner, nucleo: &Nucleo, query: &str, - indices_matcher: &mut Option, + matcher: &mut Matcher, walk_complete: bool, -) -> Option { + candidates: impl Iterator, +) -> FileSearchSnapshot { let snapshot = nucleo.snapshot(); let pattern = snapshot.pattern().column_pattern(0); - // A tick can collect the previous query's results while the new query is - // still running. Wait for a matching snapshot before using the new label. - if pattern.atoms != nucleo.pattern.column_pattern(0).atoms { - return None; - } - let limit = inner.limit.min(snapshot.matched_item_count() as usize); - let matches = snapshot - .matches() - .iter() - .take(limit) - .filter_map(|match_| { + let matches = candidates + .filter_map(|(match_, matched_name)| { let item = snapshot.get_item(match_.idx)?; let (root_idx, relative_path, match_type) = match item.data { IndexedEntry::Ascii { @@ -46,10 +174,14 @@ pub(super) fn for_query( entry.match_type, ), }; - let indices = if let Some(indices_matcher) = indices_matcher.as_mut() { + let indices = if inner.compute_indices { let mut indices = Vec::::new(); - let haystack = item.matcher_columns[0].slice(..); - let _ = pattern.indices(haystack, indices_matcher, &mut indices); + let path = item.matcher_columns[0].slice(..); + let haystack = if matched_name { basename(path) } else { path }; + let _ = pattern.indices(haystack, matcher, &mut indices); + for index in &mut indices { + *index += (path.len() - haystack.len()) as u32; + } indices.sort_unstable(); indices.dedup(); Some(indices) @@ -65,11 +197,20 @@ pub(super) fn for_query( }) }) .collect(); - Some(FileSearchSnapshot { + FileSearchSnapshot { query: query.to_string(), matches, total_match_count: snapshot.matched_item_count() as usize, scanned_file_count: snapshot.item_count() as usize, walk_complete, - }) + } +} + +fn basename(path: Utf32Str<'_>) -> Utf32Str<'_> { + let len = path + .chars() + .rev() + .take_while(|c| !std::path::is_separator(*c)) + .count(); + path.slice(path.len() - len..) }