Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 52 additions & 5 deletions crates/execpolicy/src/matcher.rs
Original file line number Diff line number Diff line change
@@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] compiled_glob doc says * matches newlines; the compiled .* does not

The new doc comment on compiled_glob claims * matches 'any run of characters, newline included'. The regex it builds is format!("^{escaped}$") where * became .* and nothing enables (?s), and Rust's regex crate does not let . match \n; ^/$ are text anchors by default, so a pattern such as mcp__* or rm -rf * will not span a newline. Runtime behaviour is unchanged (pattern_matches and the old hook matcher built exactly this regex inline), so nothing is broken today — but the comment contradicts the property the PR description says must be preserved, and someone reasoning about multi-line command coverage (the deny path in toml_rules::evaluate relies on expanded_commands to split lines precisely because a single anchored glob cannot cross one) would be misled by it. Smallest fix: correct the parenthetical.

/// does not compile; callers treat that as "no match".
Comment on lines +11 to +12

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct the wildcard description so it matches the regex actually compiled: * becomes .*, and . does not match a newline unless (?s) is set, which it is not. Lines 11-12 of the new file read as one sentence with the following blank ///, so rewording both keeps the paragraph valid.

Suggested change
/// (matching any run of characters, newline included). `None` means the pattern
/// does not compile; callers treat that as "no match".
/// (matching any run of characters, but not a newline: `.` does not match
/// one). `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<Arc<Regex>> {
static CACHE: OnceLock<Mutex<HashMap<String, Option<Arc<Regex>>>>> = 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
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -195,4 +218,28 @@ mod tests {
let normalized = normalize_command("cat <<EOF > 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"));
}
}
4 changes: 2 additions & 2 deletions crates/execpolicy/src/toml_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, RuleSet>,
}

#[derive(Debug, Deserialize, Default)]
#[derive(Debug, Clone, Deserialize, Default)]
pub struct RuleSet {
#[serde(default)]
pub allow: Vec<String>,
Expand Down
72 changes: 44 additions & 28 deletions crates/tui/src/cloud_dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -1376,25 +1376,26 @@ impl LiveDaytonaLauncher {
const HARNESS_CLIENT_SLACK_SECS: u64 = 120;

fn blocking_client() -> Result<reqwest::blocking::Client> {
Self::blocking_client_with_timeout(Self::CONTROL_PLANE_TIMEOUT_SECS)
}

fn blocking_client_with_timeout(total_secs: u64) -> Result<reqwest::blocking::Client> {
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<reqwest::blocking::Client> {
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<Result<reqwest::blocking::Client, String>> =
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.
Expand Down Expand Up @@ -1453,21 +1454,30 @@ impl LiveDaytonaLauncher {
api_key: &str,
body: serde_json::Value,
) -> Result<reqwest::blocking::Response> {
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,
body: serde_json::Value,
) -> Result<reqwest::blocking::Response> {
client
.request(method, url.clone())
.timeout(std::time::Duration::from_secs(total_secs))
.bearer_auth(api_key)
.json(&body)
.send()
Expand Down Expand Up @@ -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() {
Expand Down
10 changes: 5 additions & 5 deletions crates/tui/src/hooks/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 29 additions & 2 deletions crates/tui/src/tools/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3534,13 +3534,40 @@ fn default_execpolicy_path() -> Option<std::path::PathBuf> {
}

fn load_default_policy() -> anyhow::Result<Option<ExecPolicyConfig>> {
/// A parsed rules file, tagged with the identity it was parsed from.
type PolicyKey = (std::path::PathBuf, u64, Option<std::time::SystemTime>);

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::Mutex<Option<(PolicyKey, ExecPolicyConfig)>>> =
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. \
Expand Down
34 changes: 28 additions & 6 deletions crates/tui/src/tui/history/tool_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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(""));
}
}
2 changes: 1 addition & 1 deletion crates/tui/src/working_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down
Loading