fix: address glob expansion review feedback (PR #541)

Charge legacy brace alternatives before formatting, admit brace-only requests through the expansion budget, and preserve configured path priority during deduplication. Add regressions for each behavior without changing pinned terminal-glob semantics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: karthiknadig

crates/pet-fs/src/glob.rs

@@ -136,6 +136,11 @@ fn expand_braces_inner(pattern: &str, results: &mut Vec<String>) {
             .and_then(|open| pattern[open..].find('}').map(|close| (open, open + close)));
         if let Some((open, close)) = group {
             for alternative in pattern[open + 1..close].split(',').rev() {
+                if steps == MAX_BRACE_EXPANSION_STEPS {
+                    log::warn!("Brace expansion exceeded its work limit, truncating '{pattern}'");
+                    return;
+                }
+                steps += 1;
                 pending.push(format!(
                     "{}{alternative}{}",
                     &pattern[..open],
@@ -749,6 +754,14 @@ mod tests {
         ));
     }
 
+    #[test]
+    fn legacy_brace_expansion_charges_alternatives_before_formatting() {
+        let pattern = format!("{{{}}}", vec!["a"; MAX_BRACE_EXPANSION_STEPS].join(","));
+        assert!(expand_braces(&pattern).is_empty());
+        assert!(expand_glob_pattern(&pattern).is_empty());
+        assert!(!is_recursive_glob_pattern(&pattern));
+    }
+
     #[test]
     fn duplicate_brace_alternatives_count_toward_work_limit() {
         let within_limit = format!("{{{}}}", vec!["a"; MAX_BRACE_EXPANSION_STEPS - 2].join(","));

crates/pet/src/jsonrpc.rs

@@ -22,7 +22,7 @@ use pet_core::{
 };
 use pet_env_var_path::get_search_paths_from_env_variables;
 use pet_fs::glob::{
-    expand_glob_patterns_bounded, is_recursive_glob_pattern, GlobExpansionError,
+    expand_glob_patterns_bounded, is_glob_pattern, is_recursive_glob_pattern, GlobExpansionError,
     DEFAULT_GLOB_EXPANSION_LIMITS,
 };
 use pet_fs::path::norm_case;
@@ -40,7 +40,7 @@ use pet_telemetry::report_inaccuracies_identified_after_resolving;
 use serde::{Deserialize, Serialize};
 use serde_json::json;
 use serde_json::{self, Value};
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashSet};
 use std::sync::atomic::{AtomicU64, Ordering};
 use std::time::Duration;
 use std::{
@@ -98,7 +98,7 @@ impl GlobExpansionAdmission {
     ) -> Result<Option<GlobExpansionPermit>, String> {
         if !patterns
             .into_iter()
-            .any(|pattern| pattern.to_string_lossy().contains(['*', '?', '[', ']']))
+            .any(|pattern| is_glob_pattern(&pattern.to_string_lossy()))
         {
             return Ok(None);
         }
@@ -820,13 +820,12 @@ fn normalize_refresh_params(params: Value) -> Value {
 }
 
 fn deduplicate_path_patterns(paths: &[PathBuf]) -> Vec<PathBuf> {
-    let mut patterns = BTreeMap::new();
-    for path in paths {
-        patterns
-            .entry(path.as_os_str().to_owned())
-            .or_insert_with(|| path.clone());
-    }
-    patterns.into_values().collect()
+    let mut seen = HashSet::new();
+    paths
+        .iter()
+        .filter(|path| seen.insert(path.as_os_str()))
+        .cloned()
+        .collect()
 }
 
 fn parse_refresh_options(params: Value) -> Result<RefreshOptions, serde_json::Error> {
@@ -1873,7 +1872,12 @@ mod tests {
             .try_acquire_for_patterns(std::iter::once(&literal))
             .unwrap()
             .is_none());
-        for text in ["workspace/*", "workspace/**", "malformed["] {
+        for text in [
+            "workspace/*",
+            "workspace/**",
+            "malformed[",
+            "workspace/{a,b}",
+        ] {
             let pattern = PathBuf::from(text);
             assert!(admission
                 .try_acquire_for_patterns(std::iter::once(&pattern))
@@ -1912,6 +1916,16 @@ mod tests {
             .expect("refresh without paths must bypass glob admission");
     }
 
+    #[test]
+    fn pattern_deduplication_preserves_configured_priority() {
+        let paths = [
+            PathBuf::from("z-priority"),
+            PathBuf::from("a-fallback"),
+            PathBuf::from("z-priority"),
+        ];
+        assert_eq!(deduplicate_path_patterns(&paths), paths[..2]);
+    }
+
     #[test]
     fn pattern_deduplication_preserves_directory_only_globs() {
         let patterns = [PathBuf::from("workspace/*"), PathBuf::from("workspace/*/")];

docs/JSONRPC.md

@@ -194,9 +194,10 @@ limits return a JSON-RPC error (`-4`) and no partial refresh inventory. Limits a
 checked between filesystem entries; they are not a timeout and cannot interrupt an
 operating-system filesystem call already in progress.
 
-PET admits at most two configure/refresh filesystem glob expansions concurrently.
-Requests without wildcard patterns do not consume these slots. Additional wildcard
+PET admits at most two configure/refresh glob or brace expansions concurrently.
+Requests containing only literal paths do not consume these slots. Additional expansion
 requests receive JSON-RPC error `-4` instead of creating an unbounded traversal queue.
+Input deduplication preserves the first occurrence order of configured paths.
 
 ## Refresh Progress Telemetry