From e522e4f93c0d925717724b202b1ddbbc6d5e8f9f Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 21:09:58 -0700 Subject: [PATCH] perf(tui): stop rebuilding per-call work on the shell, hook and cloud paths (#6208) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five mechanical fixes from the 2026-09-15 perf review (`codewhale-ops/PERF-OPPORTUNITIES-20260915.md` §1), each independently verifiable and behavior-preserving. Q3 — `output_is_image` lowercased the whole tool output, hundreds of kilobytes at worst, to sniff eight ASCII suffixes, once per MCP completion. It now scans the raw bytes case-insensitively. The historical `contains` contract is unchanged, including "weird.pngx" counting as an image; only the allocation is gone. Q4 — the mention browser's `sort_by_key(|entry| entry.to_lowercase())` allocated a fresh key O(n log n) times per keystroke. `sort_by_cached_key` computes it once per entry. Q5 — the cloud launcher built a fresh `reqwest::blocking::Client` per call, so all nine control-plane call sites paid a new TCP+TLS handshake and one of them is a poll loop. One process-wide client now owns the connection pool (`clone()` is a refcount bump) and the total timeout moved to the request. That is also the shape the harness turn needs: its budget comes from its own command and must not inherit the 120s control-plane cap, which is why the per-client timeout existed in the first place. Q6 — `execpolicy.toml` was re-read, re-parsed and its regexes recompiled on every shell execution. `load_default_policy` now caches the parsed rules against (path, length, mtime) — length joins the timestamp because a coarse-mtime filesystem can report the same instant for two revisions — and `matcher::compiled_glob` compiles each pattern once. Q7 — hook tool-name globs compiled a fresh `Regex` per hook per tool-call/stop event. `tool_name_matches_condition` now uses the same `compiled_glob`, which deletes its copy of the escape/anchoring logic. `compiled_glob` escapes every regex metacharacter except `*` exactly as `pattern_matches` always has, so what matches does not change — including the fact that `regex`'s `.` does not cross a newline. Two tests pin the new helper's contract (one compilation per pattern, escaping preserved), and one pins the image sniff. Verification on this machine (macOS, aarch64), `cargo check` and `cargo clippy` with the project's flags (`-D warnings` plus the three documented allowances) both clean: test result: ok. 217 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out (codewhale-execpolicy — lib, authorization_order, redirection_policy, doctests) test result: ok. 8 passed; 0 failed (tui::history::tool_output) test result: ok. 44 passed; 0 failed (working_set) test result: ok. 109 passed; 0 failed (hooks::executor) test result: ok. 151 passed; 0 failed; 1 ignored (tools::shell) test result: ok. 32 passed; 0 failed (cloud_dispatch) Not verified here: hosted CI, and the Q5 change has no automated test that pins connection reuse — it is exercised only through the existing cloud_dispatch tests, which cover the request/response shape rather than the pooling. Signed-off-by: CodeWhale Bot --- crates/execpolicy/src/matcher.rs | 57 ++++++++++++++++-- crates/execpolicy/src/toml_rules.rs | 4 +- crates/tui/src/cloud_dispatch.rs | 72 ++++++++++++++--------- crates/tui/src/hooks/executor.rs | 10 ++-- crates/tui/src/tools/shell.rs | 31 +++++++++- crates/tui/src/tui/history/tool_output.rs | 34 +++++++++-- crates/tui/src/working_set.rs | 2 +- 7 files changed, 161 insertions(+), 49 deletions(-) diff --git a/crates/execpolicy/src/matcher.rs b/crates/execpolicy/src/matcher.rs index e54e6b63df..4e40d4b5e4 100644 --- a/crates/execpolicy/src/matcher.rs +++ b/crates/execpolicy/src/matcher.rs @@ -1,7 +1,34 @@ //! Command matching helpers for execpolicy rules. +use std::collections::HashMap; +use std::sync::{Arc, Mutex, OnceLock}; + use regex::Regex; +/// `pattern` compiled once as an anchored `*`-glob, then reused. +/// +/// Every other regex metacharacter is escaped, so `*` is the only wildcard +/// (matching any run of characters, newline included). `None` means the pattern +/// does not compile; callers treat that as "no match". +/// +/// The patterns come from configuration and are stable between edits, but the +/// callers run per shell execution and per hook event. Compiling here on first +/// use keeps the fast path free of `Regex::new` without changing what matches. +pub fn compiled_glob(pattern: &str) -> Option> { + static CACHE: OnceLock>>>> = OnceLock::new(); + let cache = CACHE.get_or_init(|| Mutex::new(HashMap::new())); + let mut cache = cache + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + cache + .entry(pattern.to_string()) + .or_insert_with(|| { + let escaped = regex::escape(pattern).replace(r"\*", ".*"); + Regex::new(&format!("^{escaped}$")).ok().map(Arc::new) + }) + .clone() +} + /// Normalize a command string by shlex parsing and re-joining tokens. /// /// Strips heredoc bodies first (#419) so a command like @@ -110,11 +137,7 @@ pub fn pattern_matches(pattern: &str, command: &str) -> bool { return true; } - let escaped = regex::escape(&pattern).replace("\\*", ".*"); - let Ok(re) = Regex::new(&format!("^{escaped}$")) else { - return false; - }; - re.is_match(&command) + compiled_glob(&pattern).is_some_and(|re| re.is_match(&command)) } #[cfg(test)] @@ -195,4 +218,28 @@ mod tests { let normalized = normalize_command("cat < file.txt\nbody\nEOF"); assert!(pattern_matches("cat > file.txt", &normalized)); } + + #[test] + fn compiled_glob_is_compiled_once_per_pattern() { + // The per-shell-execution and per-hook-event callers rely on this + // returning the same compiled program rather than rebuilding it. + let first = compiled_glob("mcp__*").expect("glob compiles"); + let second = compiled_glob("mcp__*").expect("glob compiles"); + assert!(std::sync::Arc::ptr_eq(&first, &second)); + assert!(first.is_match("mcp__github__search")); + assert!(!first.is_match("read_file")); + } + + #[test] + fn compiled_glob_escapes_every_metacharacter_except_star() { + // `regex::escape` is what makes `a.b` a literal while `*` stays a + // wildcard — the same contract `pattern_matches` has always had. + let literal = compiled_glob("a.b").expect("glob compiles"); + assert!(literal.is_match("a.b")); + assert!(!literal.is_match("axb")); + + let wildcard = compiled_glob("a*b").expect("glob compiles"); + assert!(wildcard.is_match("ab")); + assert!(wildcard.is_match("a middle b")); + } } diff --git a/crates/execpolicy/src/toml_rules.rs b/crates/execpolicy/src/toml_rules.rs index 79a904e13a..5239d3d039 100644 --- a/crates/execpolicy/src/toml_rules.rs +++ b/crates/execpolicy/src/toml_rules.rs @@ -30,13 +30,13 @@ pub enum RuleDecision { AskUser(String), } -#[derive(Debug, Deserialize, Default)] +#[derive(Debug, Clone, Deserialize, Default)] pub struct ExecPolicyConfig { #[serde(default)] pub rules: BTreeMap, } -#[derive(Debug, Deserialize, Default)] +#[derive(Debug, Clone, Deserialize, Default)] pub struct RuleSet { #[serde(default)] pub allow: Vec, diff --git a/crates/tui/src/cloud_dispatch.rs b/crates/tui/src/cloud_dispatch.rs index 17424797ca..75d5e3f619 100644 --- a/crates/tui/src/cloud_dispatch.rs +++ b/crates/tui/src/cloud_dispatch.rs @@ -1366,7 +1366,7 @@ pub struct LiveDaytonaLauncher; impl LiveDaytonaLauncher { /// Total timeout for short control-plane calls (create/status/delete/ /// list). A dispatched harness turn is NOT a short call — see - /// [`Self::harness_client`]. + /// [`Self::harness_client_budget_secs`]. const CONTROL_PLANE_TIMEOUT_SECS: u64 = 120; /// Slack added to a harness command's declared budget for the client @@ -1376,25 +1376,26 @@ impl LiveDaytonaLauncher { const HARNESS_CLIENT_SLACK_SECS: u64 = 120; fn blocking_client() -> Result { - Self::blocking_client_with_timeout(Self::CONTROL_PLANE_TIMEOUT_SECS) - } - - fn blocking_client_with_timeout(total_secs: u64) -> Result { - crate::tls::reqwest_blocking_client_builder() - .connect_timeout(std::time::Duration::from_secs(8)) - .timeout(std::time::Duration::from_secs(total_secs)) - .redirect(reqwest::redirect::Policy::none()) - .build() - .context("failed to initialize the cloud agent client") - } - - /// A client scoped to one harness command: its total timeout is the - /// command's declared budget plus fixed slack. The declared turn budget - /// is an hour, so the 120s control-plane cap must NOT carry this call — - /// otherwise every dispatched turn longer than two minutes fails after - /// the spend has already started. - fn harness_client(command: &HarnessCommand) -> Result { - Self::blocking_client_with_timeout(Self::harness_client_budget_secs(command)) + // #6208: one client for the process. A `reqwest` client owns a + // connection pool and a TLS configuration, so building one per call + // paid a fresh TCP+TLS handshake on all nine call sites (one of them a + // poll loop). `clone()` here is a refcount bump on that shared pool. + // + // The total timeout is attached per request instead, because a harness + // turn carries a budget derived from its own command and must not + // inherit the control-plane cap. + static CLIENT: std::sync::OnceLock> = + std::sync::OnceLock::new(); + CLIENT + .get_or_init(|| { + crate::tls::reqwest_blocking_client_builder() + .connect_timeout(std::time::Duration::from_secs(8)) + .redirect(reqwest::redirect::Policy::none()) + .build() + .map_err(|error| error.to_string()) + }) + .clone() + .map_err(|message| anyhow!("failed to initialize the cloud agent client: {message}")) } /// The total-timeout budget for a harness-carrying client, in seconds. @@ -1453,14 +1454,22 @@ impl LiveDaytonaLauncher { api_key: &str, body: serde_json::Value, ) -> Result { - Self::send_json_on(&Self::blocking_client()?, method, url, api_key, body) + Self::send_json_on( + &Self::blocking_client()?, + Self::CONTROL_PLANE_TIMEOUT_SECS, + method, + url, + api_key, + body, + ) } - /// [`Self::send_json`] on a caller-supplied client, so a call whose - /// declared budget differs from the control-plane cap (the harness - /// turn) can carry a client scoped to its own budget. + /// [`Self::send_json`] with an explicit total timeout, so a call whose + /// declared budget differs from the control-plane cap (the harness turn) + /// can carry its own without needing a client of its own. fn send_json_on( client: &reqwest::blocking::Client, + total_secs: u64, method: reqwest::Method, url: &reqwest::Url, api_key: &str, @@ -1468,6 +1477,7 @@ impl LiveDaytonaLauncher { ) -> Result { client .request(method, url.clone()) + .timeout(std::time::Duration::from_secs(total_secs)) .bearer_auth(api_key) .json(&body) .send() @@ -1639,10 +1649,16 @@ impl DaytonaLauncher for LiveDaytonaLauncher { "timeout": command.timeout_secs, }); // This call carries the declared turn budget (an hour for the agent - // entry), so it rides a client scoped to that budget plus slack — - // never the 120s control-plane client that used to cap it. - let client = Self::harness_client(command)?; - let response = Self::send_json_on(&client, reqwest::Method::POST, &url, &api_key, body)?; + // entry), so it asks for that budget plus slack per request — never the + // 120s control-plane cap that used to bound it. + let response = Self::send_json_on( + &Self::blocking_client()?, + Self::harness_client_budget_secs(command), + reqwest::Method::POST, + &url, + &api_key, + body, + )?; let status = response.status(); let text = response.text().unwrap_or_default(); if !status.is_success() { diff --git a/crates/tui/src/hooks/executor.rs b/crates/tui/src/hooks/executor.rs index ce2bc08a34..ca4cb17af2 100644 --- a/crates/tui/src/hooks/executor.rs +++ b/crates/tui/src/hooks/executor.rs @@ -2059,11 +2059,11 @@ impl HookExecutor { if !pattern.contains('*') { return tool_name == pattern; } - // Escape regex metacharacters except `*`, which becomes `.*`. - let escaped = regex::escape(pattern); - let regex_pattern = escaped.replace(r"\*", ".*"); - let anchored = format!("^{regex_pattern}$"); - regex::Regex::new(&anchored).is_ok_and(|re| re.is_match(tool_name)) + // #6208: the pattern is fixed by configuration while this runs once per + // hook per tool-call/stop event, so compile it once and reuse it rather + // than building a fresh `Regex` on every event. + codewhale_execpolicy::matcher::compiled_glob(pattern) + .is_some_and(|re| re.is_match(tool_name)) } /// Check if a hook's condition matches the context diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index a9efdd1a78..c9498d8c0f 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -3534,13 +3534,40 @@ fn default_execpolicy_path() -> Option { } fn load_default_policy() -> anyhow::Result> { + /// A parsed rules file, tagged with the identity it was parsed from. + type PolicyKey = (std::path::PathBuf, u64, Option); + let Some(path) = default_execpolicy_path() else { return Ok(None); }; - if !path.exists() { + // An unreadable or missing file (including a permissions error, which + // `exists()` also swallows) means "no file rules" — the same answer as + // before, just reached with one `stat` instead of an existence check plus a + // full read. + let Ok(metadata) = std::fs::metadata(&path) else { return Ok(None); + }; + let key: PolicyKey = (path.clone(), metadata.len(), metadata.modified().ok()); + + // #6208: this runs on every shell execution, so the read and TOML parse + // happen only when the file's identity changes. Length joins the timestamp + // because a coarse-mtime filesystem can hand back the same instant for two + // different revisions. + static CACHE: std::sync::OnceLock>> = + std::sync::OnceLock::new(); + let cache = CACHE.get_or_init(|| std::sync::Mutex::new(None)); + let mut cache = cache + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if let Some((cached_key, config)) = cache.as_ref() + && *cached_key == key + { + return Ok(Some(config.clone())); } - ExecPolicyConfig::from_path(&path).map(Some) + + let config = ExecPolicyConfig::from_path(&path)?; + *cache = Some((key, config.clone())); + Ok(Some(config)) } const FOREGROUND_TIMEOUT_RECOVERY_HINT: &str = "Foreground Bash is for bounded commands. \ diff --git a/crates/tui/src/tui/history/tool_output.rs b/crates/tui/src/tui/history/tool_output.rs index be1aa03563..99c36f2f6b 100644 --- a/crates/tui/src/tui/history/tool_output.rs +++ b/crates/tui/src/tui/history/tool_output.rs @@ -338,13 +338,21 @@ pub fn summarize_mcp_output(output: &str) -> McpOutputSummary { #[must_use] pub fn output_is_image(output: &str) -> bool { - let lower = output.to_lowercase(); - - [ + // Sniff the extensions case-insensitively over the raw bytes. Lowercasing + // the whole output first copied every byte of a payload that can run to + // hundreds of kilobytes, once per MCP completion, to answer a question + // about eight ASCII suffixes. + const IMAGE_EXTENSIONS: [&str; 8] = [ ".png", ".jpg", ".jpeg", ".gif", ".webp", ".bmp", ".tiff", ".ppm", - ] - .iter() - .any(|ext| lower.contains(ext)) + ]; + + let bytes = output.as_bytes(); + IMAGE_EXTENSIONS.iter().any(|ext| { + let needle = ext.as_bytes(); + bytes + .windows(needle.len()) + .any(|window| window.eq_ignore_ascii_case(needle)) + }) } fn render_preserved_output_mode( @@ -903,4 +911,18 @@ mod ansi_colour_tests { assert_eq!(rows[0].text, "done"); assert!(rows[0].styled.is_none()); } + + #[test] + fn image_sniffing_is_case_insensitive_over_the_raw_bytes() { + assert!(output_is_image("saved to /tmp/Chart.PNG")); + assert!(output_is_image("shot.jpeg")); + assert!(output_is_image(".WEBP")); + assert!(!output_is_image("no image here")); + // The historical `contains` contract is preserved: a name that merely + // embeds an extension still counts. + assert!(output_is_image("weird.pngx")); + // A byte-window scan must not panic on multi-byte text. + assert!(!output_is_image("日本語のテキストのみ")); + assert!(!output_is_image("")); + } } diff --git a/crates/tui/src/working_set.rs b/crates/tui/src/working_set.rs index a61b2f41e8..a6f8c17ea2 100644 --- a/crates/tui/src/working_set.rs +++ b/crates/tui/src/working_set.rs @@ -537,7 +537,7 @@ impl Workspace { entries.push(candidate); } - entries.sort_by_key(|entry| entry.to_lowercase()); + entries.sort_by_cached_key(|entry| entry.to_lowercase()); entries } }