fix: address code review findings (F14 unicode, F18 hot path, F10 ANSI-C)

src/agent/recovery.rs

@@ -75,13 +75,56 @@ impl RecoveryPolicy {
 /// `:` being absent (some providers emit `retry-after 30`).
 pub(crate) fn retry_after_from_error_msg(msg: &str) -> Option<Duration> {
     fn parse_after_label(msg: &str, label: &str) -> Option<u64> {
-        let lower = msg.to_lowercase();
-        let idx = lower.find(label)?;
-        // Skip past label + optional `:` / whitespace / quote.
-        let tail = &msg[idx + label.len()..];
+        // Case-insensitive search WITHOUT lowercasing the whole
+        // message: previously we lowercased `msg` and then indexed
+        // into the ORIGINAL `msg` at the lowered string's byte
+        // offset. For ASCII that's identical, but `to_lowercase`
+        // can change byte length for some unicode (e.g. Turkish
+        // `İ` → `i̇` is 2 → 3 bytes). The mismatched offset could
+        // land mid-UTF-8 and panic on `&msg[...]`. Now we scan the
+        // original bytes window-by-window with case-insensitive
+        // ASCII comparison. The label itself is fixed-ASCII so this
+        // is sound — we just need to be case-insensitive against
+        // the message's casing.
+        let label_bytes = label.as_bytes();
+        let msg_bytes = msg.as_bytes();
+        if msg_bytes.len() < label_bytes.len() {
+            return None;
+        }
+        let mut idx = None;
+        for i in 0..=msg_bytes.len() - label_bytes.len() {
+            let window = &msg_bytes[i..i + label_bytes.len()];
+            if window
+                .iter()
+                .zip(label_bytes.iter())
+                .all(|(a, b)| a.eq_ignore_ascii_case(b))
+            {
+                idx = Some(i);
+                break;
+            }
+        }
+        let idx = idx?;
+        // `idx` is now a byte offset into the original `msg`.
+        // Land at a char boundary (the ASCII label match guarantees
+        // we're on a boundary, but `idx + label.len()` could still
+        // hit one — for ASCII labels it can't, but defend anyway).
+        let after = idx + label.len();
+        if !msg.is_char_boundary(after) {
+            return None;
+        }
+        let tail = &msg[after..];
         let tail = tail.trim_start_matches([':', ' ', '\t', '"']).trim_start();
-        // Consume contiguous digits.
-        let n: String = tail.chars().take_while(|c| c.is_ascii_digit()).collect();
+        // Consume contiguous digits, with a hard cap so a malformed
+        // header (`Retry-After: 999999999999999999999`) doesn't
+        // produce a parsed integer that overflows or is absurdly
+        // large before the 5-min cap applies in the caller. Cap at
+        // 10^10 — any value larger is clearly bogus, and the cap
+        // saturates rather than overflowing u64.
+        let n: String = tail
+            .chars()
+            .take_while(|c| c.is_ascii_digit())
+            .take(11)
+            .collect();
         if n.is_empty() {
             return None;
         }
@@ -479,6 +522,61 @@ mod tests {
         assert_eq!(retry_after_from_error_msg(msg), None);
     }
 
+    /// Regression: messages with multi-byte UTF-8 BEFORE the label
+    /// previously could panic — the original parser found the
+    /// label in a lowercased copy and indexed into the original
+    /// at that byte offset. `to_lowercase` can change byte length
+    /// (Turkish `İ` is 2 bytes lowercase as `i̇` = 3 bytes), so
+    /// the offsets disagreed and `&msg[idx + label.len()..]` could
+    /// land mid-UTF-8 → panic. Now the search is on byte windows
+    /// of the original string with case-insensitive ASCII compare.
+    #[test]
+    fn retry_after_handles_unicode_before_label() {
+        // Provider error message with a Turkish capital I before
+        // the label. Lowercasing produces a different byte length.
+        let msg = "İoError: Retry-After: 8";
+        assert_eq!(
+            retry_after_from_error_msg(msg),
+            Some(Duration::from_secs(8)),
+        );
+    }
+
+    /// Case-insensitive matching against the label name itself.
+    /// `RETRY-AFTER-MS` and `retry-after-ms` should both parse.
+    #[test]
+    fn retry_after_label_match_is_case_insensitive() {
+        assert_eq!(
+            retry_after_from_error_msg("rate limited: RETRY-AFTER-MS: 750"),
+            Some(Duration::from_millis(750)),
+        );
+        assert_eq!(
+            retry_after_from_error_msg("Retry-After-Ms: 750"),
+            Some(Duration::from_millis(750)),
+        );
+    }
+
+    /// Pathological huge digit run: cap at 11 digits before parse,
+    /// so `Retry-After: 999999999999999999999...` doesn't overflow
+    /// or produce a 100-year wait before the upper cap clamps.
+    #[test]
+    fn retry_after_caps_pathological_digit_run() {
+        let msg = "Retry-After: 99999999999999999999999";
+        let parsed = retry_after_from_error_msg(msg);
+        // 11 digits = max ~10^11 seconds — `backoff_duration_for_msg`
+        // will cap at 5 minutes, but the unsanitized parse must
+        // produce SOMETHING (not None, not a panic). We don't pin
+        // the exact value; just verify it's bounded by the cap
+        // behavior in `backoff_duration_for_msg`.
+        assert!(parsed.is_some(), "must parse, not return None");
+        let policy = RecoveryPolicy::default();
+        let d = policy.backoff_duration_for_msg(0, msg);
+        assert!(
+            d <= Duration::from_secs(300),
+            "backoff must cap at 5min; got {:?}",
+            d,
+        );
+    }
+
     /// `backoff_duration_for_msg` picks the longer of the
     /// computed exponential backoff and the server's retry-after,
     /// capped at 5 minutes.

src/agent/tools/bash.rs

@@ -263,13 +263,24 @@ async fn check_bash_segments(
         // a backslash escape.
         let segments = quote_aware_split(command);
 
-        // Flag command substitution / subshell constructs that need a
-        // full parser. Surface as one whole-command check so the user
-        // sees the unfamiliar form before any segment runs.
+        // Flag command substitution / subshell constructs / ANSI-C
+        // quoting that need a full parser. Surface as one
+        // whole-command check so the user sees the unfamiliar form
+        // before any segment runs.
+        //
+        // `$'...'` ANSI-C quoting was missing from the original
+        // check, leaving a small bypass: `echo $'hi\nrm -rf /; ls'`
+        // (with embedded literal newlines via `\n`) treated the body
+        // as one quoted token, so `quote_aware_split` didn't see the
+        // `;` as a separator. Adding `$'` to the substitution list
+        // makes the whole command get checked as a single string —
+        // the rules can still match the safe form, but the LLM
+        // doesn't get a free pass on obscure quoting.
         let has_substitution = command.contains("$(")
             || command.contains('`')
             || command.contains("<(")
-            || command.contains(">(");
+            || command.contains(">(")
+            || command.contains("$'");
         if has_substitution {
             return check_perm(permission, ask_tx, "bash", command).await;
         }

src/permission/checker.rs

@@ -20,6 +20,14 @@ pub struct PermissionChecker {
     ext_dir_rules: Vec<(Pattern, Action)>,
     doom_loop_action: Action,
     working_dir: String,
+    /// Cached canonical form of `working_dir`, computed once at
+    /// construction (and refreshed by `set_working_dir`). Used by
+    /// `is_external_path` to compare canonical paths without
+    /// hitting the filesystem on every permission check — the
+    /// canonicalize syscall is otherwise called once per
+    /// read/write/edit/grep call, accumulating to hundreds of
+    /// stat()s per session.
+    working_dir_canonical: String,
     session_allowlist: Vec<(String, Pattern)>,
     recent_calls: VecDeque<(String, String)>,
     mode: SecurityMode,
@@ -105,13 +113,15 @@ impl PermissionChecker {
             .unwrap_or_else(|| std::env::current_dir().unwrap_or_default())
             .to_string_lossy()
             .to_string();
+        let working_dir_canonical = canonicalize_for_cache(&working_dir);
 
         PermissionChecker {
             rules,
             default_action,
             ext_dir_rules,
             doom_loop_action,
             working_dir,
+            working_dir_canonical,
             session_allowlist: Vec::new(),
             recent_calls: VecDeque::with_capacity(16),
             mode,
@@ -320,6 +330,7 @@ impl PermissionChecker {
 
     pub fn set_working_dir(&mut self, dir: &str) {
         self.working_dir = dir.to_string();
+        self.working_dir_canonical = canonicalize_for_cache(dir);
     }
 
     fn is_path_tool(&self, tool: &str) -> bool {
@@ -349,12 +360,15 @@ impl PermissionChecker {
             return false;
         }
         let cwd = Path::new(&self.working_dir);
-        // Canonicalize cwd for the comparison too — if a symlinked
-        // working_dir or one with `..` segments was stored, a
-        // canonicalized `resolved` could no longer share the
-        // prefix even when it's the same directory.
-        let canonical_cwd = std::fs::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf());
-        !p.starts_with(&canonical_cwd) && !p.starts_with(cwd)
+        // Canonical cwd is precomputed (see `working_dir_canonical`).
+        // Comparing against BOTH the canonical and literal forms
+        // handles symlinked roots like macOS's `/tmp → /private/tmp`:
+        // `resolved` is canonical (`/private/tmp/...`) but `cwd`
+        // may still be the literal `/tmp` form. Without both checks
+        // every in-tree access in such a setup would classify as
+        // external.
+        let canonical_cwd = Path::new(&self.working_dir_canonical);
+        !p.starts_with(canonical_cwd) && !p.starts_with(cwd)
     }
 
     fn match_ext_dir(&self, path_str: &str) -> Option<Action> {
@@ -384,6 +398,18 @@ impl PermissionChecker {
     }
 }
 
+/// One-shot canonicalize for the working-directory cache. Best
+/// effort: if canonicalize fails (cwd doesn't exist on disk, e.g.
+/// in tests that pass a fixture path), fall back to the literal
+/// string so the `starts_with` comparisons in `is_external_path`
+/// still work for the literal form.
+fn canonicalize_for_cache(working_dir: &str) -> String {
+    std::fs::canonicalize(working_dir)
+        .ok()
+        .map(|p| p.to_string_lossy().into_owned())
+        .unwrap_or_else(|| working_dir.to_string())
+}
+
 fn resolve_absolute(path: &str, working_dir: &str) -> String {
     let p = Path::new(path);
     let joined = if p.is_absolute() {