Fix 'allow always' patterns matching paths in bash/grep commands

src/permission/checker.rs

@@ -25,6 +25,22 @@ pub struct PermissionChecker {
     mode: SecurityMode,
 }
 
+/// Tool names where the input is a filesystem path. For these, `*` keeps
+/// classic glob semantics (one segment, doesn't cross `/`). Everything else
+/// is treated as shell/text where `*` means "any chars including /".
+pub(crate) fn is_path_tool_name(tool: &str) -> bool {
+    matches!(tool, "read" | "write" | "edit" | "list_dir")
+}
+
+/// Build a Pattern with the right `*` semantics for the given tool.
+pub(crate) fn pattern_for_tool(tool: &str, pat: &str) -> Pattern {
+    if is_path_tool_name(tool) {
+        Pattern::new(pat)
+    } else {
+        Pattern::new_command(pat)
+    }
+}
+
 impl PermissionChecker {
     pub fn new(
         config: &PermissionConfig,
@@ -49,11 +65,11 @@ impl PermissionChecker {
             let mut entries = Vec::new();
             match tp {
                 ToolPerm::Simple(action) => {
-                    entries.push((Pattern::new("*"), *action));
+                    entries.push((pattern_for_tool(tool_name, "*"), *action));
                 }
                 ToolPerm::Granular(map) => {
                     for (pat, action) in map {
-                        entries.push((Pattern::new(pat), *action));
+                        entries.push((pattern_for_tool(tool_name, pat), *action));
                     }
                 }
             }
@@ -63,11 +79,12 @@ impl PermissionChecker {
         if !rules.contains_key("bash") {
             let mut defaults = Vec::new();
             for (pat, action) in crate::permission::default_bash_rules() {
-                defaults.push((Pattern::new(pat), action));
+                defaults.push((pattern_for_tool("bash", pat), action));
             }
             rules.insert("bash".to_string(), defaults);
         }
 
+        // External-directory rules are always path patterns by definition.
         let ext_dir_rules = config
             .external_directory
             .as_ref()
@@ -239,14 +256,14 @@ impl PermissionChecker {
     }
 
     pub fn add_session_allowlist(&mut self, tool: String, pattern_str: &str) {
-        let pattern = Pattern::new(pattern_str);
+        let pattern = pattern_for_tool(&tool, pattern_str);
         self.session_allowlist.push((tool, pattern));
     }
 
     pub fn load_session_allowlist(&mut self, entries: &[(String, String)]) {
         for (tool, pat) in entries {
             self.session_allowlist
-                .push((tool.clone(), Pattern::new(pat)));
+                .push((tool.clone(), pattern_for_tool(tool, pat)));
         }
     }
 
@@ -318,3 +335,88 @@ fn resolve_absolute(path: &str, working_dir: &str) -> String {
         Path::new(working_dir).join(p).to_string_lossy().to_string()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::permission::PermissionConfig;
+
+    fn fresh_checker() -> PermissionChecker {
+        PermissionChecker::new(
+            &PermissionConfig::default(),
+            SecurityMode::Standard,
+            Some(std::path::PathBuf::from("/tmp")),
+        )
+    }
+
+    // Regression: "allow always" → `cd *` saved to session allowlist must
+    // satisfy the NEXT bash check for `cd /absolute/path`. Before the fix,
+    // path-glob semantics on `*` (`[^/]*`) refused to match the absolute
+    // path, so the user was re-prompted every command.
+    #[test]
+    fn regression_session_allowlist_cd_star_matches_path_arg() {
+        let mut checker = fresh_checker();
+        checker.add_session_allowlist("bash".to_string(), "cd *");
+
+        // The exact scenario from the bug report.
+        let r1 = checker.check(
+            "bash",
+            "cd /Users/yogthos/src/work/rigging-workshop && git diff",
+        );
+        assert!(
+            matches!(r1, CheckResult::Allowed),
+            "expected Allowed, got {:?}",
+            r1
+        );
+
+        let r2 = checker.check("bash", "cd /Users/yogthos/src/work/rigging-workshop");
+        assert!(matches!(r2, CheckResult::Allowed));
+    }
+
+    // Path-tool patterns still get filesystem-glob semantics — adding
+    // `src/*` doesn't allow nested files. Force default Ask so we can read
+    // the session-allowlist contribution in isolation from the default.
+    #[test]
+    fn path_tool_session_allowlist_keeps_one_segment_semantics() {
+        let mut cfg = PermissionConfig::default();
+        cfg.default = Some(Action::Ask);
+        let mut checker = PermissionChecker::new(
+            &cfg,
+            SecurityMode::Standard,
+            Some(std::path::PathBuf::from("/tmp")),
+        );
+        checker.add_session_allowlist("read".to_string(), "src/*");
+
+        // One-segment hit from the session allowlist.
+        assert!(matches!(
+            checker.check_path("read", "src/main.rs"),
+            CheckResult::Allowed
+        ));
+        // Nested path: not in allowlist, falls through to default Ask.
+        let nested = checker.check_path("read", "src/agent/main.rs");
+        assert!(
+            matches!(nested, CheckResult::Ask),
+            "src/* must not match nested path; got {:?}",
+            nested
+        );
+    }
+
+    // load_session_allowlist roundtrip: persisted patterns from a previous
+    // session should match the way they did when saved.
+    #[test]
+    fn regression_load_session_allowlist_preserves_command_semantics() {
+        let mut checker = fresh_checker();
+        let saved = vec![("bash".to_string(), "cd *".to_string())];
+        checker.load_session_allowlist(&saved);
+
+        let r = checker.check("bash", "cd /home/me/project");
+        assert!(matches!(r, CheckResult::Allowed));
+    }
+
+    #[test]
+    fn pattern_for_tool_distinguishes_path_and_command_tools() {
+        assert!(pattern_for_tool("bash", "cd *").matches("cd /a/b/c"));
+        assert!(!pattern_for_tool("read", "cd *").matches("cd /a/b/c"));
+        assert!(pattern_for_tool("read", "cd *").matches("cd file"));
+    }
+}

src/permission/pattern.rs

@@ -8,9 +8,27 @@ pub struct Pattern {
 }
 
 impl Pattern {
+    /// Filesystem-style glob: `*` matches one path segment (no `/`); `**`
+    /// matches any depth. Use for path tools (`read`, `write`, `edit`,
+    /// `list_dir`).
     pub fn new(pattern: &str) -> Self {
+        Self::compile(pattern, /* path_style */ true)
+    }
+
+    /// Shell-style glob for non-path inputs: `*` matches any chars including
+    /// `/`. Use for `bash` command patterns, `grep`/`find_files` patterns,
+    /// and other tools where the input isn't a filesystem path.
+    ///
+    /// Without this, a user pattern like `cd *` (suggested by the harness
+    /// for `bash` after the user accepts "allow always") would NOT match
+    /// `cd /Users/foo/bar` because `[^/]*` stops at the first slash.
+    pub fn new_command(pattern: &str) -> Self {
+        Self::compile(pattern, /* path_style */ false)
+    }
+
+    fn compile(pattern: &str, path_style: bool) -> Self {
         let expanded = expand_home(pattern);
-        let regex_str = glob_to_regex(&expanded);
+        let regex_str = glob_to_regex(&expanded, path_style);
         let regex = Regex::new(&regex_str).unwrap_or_else(|_| Regex::new("^$").unwrap());
         Pattern {
             regex,
@@ -44,7 +62,84 @@ fn expand_home(pattern: &str) -> String {
     pattern.to_string()
 }
 
-fn glob_to_regex(pattern: &str) -> String {
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // Regression: `cd *` saved via "allow always" must match the user's NEXT
+    // `cd /absolute/path` command. The original bug was filesystem-glob
+    // semantics applied to a shell-command pattern: `*` compiled to `[^/]*`,
+    // refusing to cross slashes. Allowlist entries for bash never fired and
+    // the agent re-prompted on every command.
+    #[test]
+    fn regression_command_pattern_cd_star_matches_path_arg() {
+        let pat = Pattern::new_command("cd *");
+        assert!(pat.matches("cd /Users/yogthos/src/work/foo"));
+        assert!(pat.matches("cd /Users/yogthos/src/work/foo && git diff"));
+        assert!(pat.matches("cd foo"));
+    }
+
+    #[test]
+    fn regression_command_pattern_anchors_to_start() {
+        // Don't over-rotate: `cd *` shouldn't match commands that merely
+        // contain `cd ` somewhere later.
+        let pat = Pattern::new_command("cd *");
+        assert!(!pat.matches("xcd foo"));
+        assert!(!pat.matches("echo cd foo"));
+    }
+
+    #[test]
+    fn path_pattern_star_still_excludes_slash() {
+        let pat = Pattern::new("src/*");
+        assert!(pat.matches("src/main.rs"));
+        // Single segment only — `*` doesn't span directory boundaries.
+        assert!(!pat.matches("src/agent/main.rs"));
+    }
+
+    #[test]
+    fn path_pattern_double_star_spans_directories() {
+        let pat = Pattern::new("src/**");
+        assert!(pat.matches("src/main.rs"));
+        assert!(pat.matches("src/agent/main.rs"));
+        assert!(pat.matches("src/agent/tools/foo.rs"));
+    }
+
+    #[test]
+    fn command_pattern_question_mark_matches_any_char() {
+        let pat = Pattern::new_command("file.?");
+        assert!(pat.matches("file.a"));
+        // For commands, `?` is unrestricted.
+        assert!(pat.matches("file./"));
+    }
+
+    #[test]
+    fn path_pattern_question_mark_excludes_slash() {
+        let pat = Pattern::new("file.?");
+        assert!(pat.matches("file.a"));
+        assert!(!pat.matches("file./"));
+    }
+
+    #[test]
+    fn home_expansion_works_for_both_styles() {
+        if let Some(home) = dirs::home_dir() {
+            let expected = format!("{}/foo/bar", home.display());
+            assert!(Pattern::new("~/foo/*").matches(&expected));
+            assert!(Pattern::new_command("~/foo/*").matches(&expected));
+        }
+    }
+
+    // Regex metachars in pattern text must be escaped, not interpreted.
+    #[test]
+    fn special_chars_are_escaped() {
+        let pat = Pattern::new_command("npm test (unit)");
+        assert!(pat.matches("npm test (unit)"));
+        // Without escaping, `(unit)` would be a regex group and not require
+        // the literal parens.
+        assert!(!pat.matches("npm test unit"));
+    }
+}
+
+fn glob_to_regex(pattern: &str, path_style: bool) -> String {
     let mut re = String::with_capacity(pattern.len() * 2);
     re.push('^');
     let mut chars = pattern.chars().peekable();
@@ -59,10 +154,13 @@ fn glob_to_regex(pattern: &str) -> String {
                     } else {
                         re.push_str(".*");
                     }
-                } else {
+                } else if path_style {
                     re.push_str("[^/]*");
+                } else {
+                    re.push_str(".*");
                 }
             }
+            '?' if path_style => re.push_str("[^/]"),
             '?' => re.push('.'),
             '.' => re.push_str("\\."),
             '\\' => re.push_str("\\\\"),