diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4203a17cd..1d74f4ca8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -57,7 +57,7 @@ jobs: - uses: Swatinem/rust-cache@v2 if: runner.os != 'Linux' with: - cache-bin: false + cache-bin: "false" - name: Run tests if: runner.os != 'Linux' run: cargo test --workspace --all-features --locked @@ -90,7 +90,7 @@ jobs: - uses: Swatinem/rust-cache@v2 if: runner.os != 'Linux' with: - cache-bin: false + cache-bin: "false" - name: Build wrapper binaries if: runner.os != 'Linux' run: cargo build --release --locked -p codewhale-cli -p codewhale-tui @@ -120,7 +120,7 @@ jobs: sudo apt-get install -y libdbus-1-dev pkg-config - uses: Swatinem/rust-cache@v2 with: - cache-bin: false + cache-bin: "false" - name: Build docs run: cargo doc --workspace --no-deps env: diff --git a/Cargo.lock b/Cargo.lock index 2d5bd8e14..950be447a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -861,6 +861,7 @@ name = "codewhale-config" version = "0.8.44" dependencies = [ "anyhow", + "codewhale-execpolicy", "codewhale-secrets", "dirs", "serde", @@ -892,6 +893,7 @@ version = "0.8.44" dependencies = [ "anyhow", "codewhale-protocol", + "globset", "serde", ] diff --git a/config.example.toml b/config.example.toml index 87af1a8e4..c473565f6 100644 --- a/config.example.toml +++ b/config.example.toml @@ -141,6 +141,21 @@ sandbox_mode = "workspace-write" # read-only | workspace-write | danger-full-acc # auto_allow = ["cargo check", "npm run"] # # auto_allow = [] +# auto_deny = ["rm -rf"] +# +# Typed persistent permission rules. These are intentionally conservative: +# shell commands use the same arity-aware matching as auto_allow, and file +# paths are workspace-relative globs. +# +# [[permissions.rules]] +# tool = "exec_shell" +# decision = "allow" # "allow", "deny", or "ask" +# command = "cargo test" +# +# [[permissions.rules]] +# tool = "file_edit" +# decision = "ask" +# path = "src/**" max_subagents = 10 # optional (1-20) # Optional sub-agent tuning. max_concurrent overrides top-level max_subagents. diff --git a/crates/app-server/src/lib.rs b/crates/app-server/src/lib.rs index e580ed322..ff403703a 100644 --- a/crates/app-server/src/lib.rs +++ b/crates/app-server/src/lib.rs @@ -9,7 +9,6 @@ use axum::{Json, Router}; use codewhale_agent::ModelRegistry; use codewhale_config::{CliRuntimeOverrides, ConfigStore}; use codewhale_core::Runtime; -use codewhale_execpolicy::ExecPolicyEngine; use codewhale_hooks::{HookDispatcher, JsonlHookSink, StdoutHookSink}; use codewhale_mcp::McpManager; use codewhale_protocol::{ @@ -285,7 +284,7 @@ fn build_state(config_path: Option) -> Result { state_store, Arc::new(ToolRegistry::default()), Arc::new(McpManager::default()), - ExecPolicyEngine::new(Vec::new(), Vec::new()), + config.exec_policy_engine(), hooks, ); diff --git a/crates/config/Cargo.toml b/crates/config/Cargo.toml index 2d9ea5228..4e27366e6 100644 --- a/crates/config/Cargo.toml +++ b/crates/config/Cargo.toml @@ -8,6 +8,7 @@ description = "Config schema and precedence model for DeepSeek workspace archite [dependencies] anyhow.workspace = true +codewhale-execpolicy = { path = "../execpolicy", version = "0.8.44" } codewhale-secrets = { path = "../secrets", version = "0.8.44" } dirs.workspace = true serde.workspace = true diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 9bfb089e0..6c752cf2f 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -6,6 +6,8 @@ use std::path::{Component, Path, PathBuf}; use std::sync::OnceLock; use anyhow::{Context, Result, bail}; +use codewhale_execpolicy::{ExecPolicyEngine, Ruleset}; +pub use codewhale_execpolicy::{PermissionDecision, ToolPermissionRule}; use codewhale_secrets::SecretSource; pub use codewhale_secrets::Secrets; use serde::{Deserialize, Serialize}; @@ -182,6 +184,19 @@ impl ProvidersToml { } } +#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq, Eq)] +pub struct PermissionsToml { + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub rules: Vec, +} + +impl PermissionsToml { + #[must_use] + pub fn is_empty(&self) -> bool { + self.rules.is_empty() + } +} + #[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct ConfigToml { /// TUI-compatible DeepSeek API key. Kept at the root so both `deepseek` @@ -205,6 +220,16 @@ pub struct ConfigToml { pub telemetry: Option, pub approval_policy: Option, pub sandbox_mode: Option, + /// Legacy command allow-list. Entries become `exec_shell` allow rules. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub auto_allow: Vec, + /// Legacy command deny-list. Entries become `exec_shell` deny rules. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub auto_deny: Vec, + /// Typed tool permission rules. First version supports tool name, command + /// prefix, and workspace-relative path glob matching. + #[serde(default, skip_serializing_if = "PermissionsToml::is_empty")] + pub permissions: PermissionsToml, #[serde(default)] pub providers: ProvidersToml, /// Per-domain network policy (#135). When absent, network tools fall back @@ -371,6 +396,15 @@ impl ConfigToml { if project.sandbox_mode.is_some() { self.sandbox_mode = project.sandbox_mode; } + if !project.auto_allow.is_empty() { + self.auto_allow = project.auto_allow; + } + if !project.auto_deny.is_empty() { + self.auto_deny = project.auto_deny; + } + if !project.permissions.is_empty() { + self.permissions = project.permissions; + } // Provider is only overridden if explicitly set (non-default). if project.provider != ProviderKind::Deepseek || has_api_key { self.provider = project.provider; @@ -418,6 +452,17 @@ impl ConfigToml { } } + #[must_use] + pub fn permission_ruleset(&self) -> Ruleset { + Ruleset::user(self.auto_allow.clone(), self.auto_deny.clone()) + .with_rules(self.permissions.rules.clone()) + } + + #[must_use] + pub fn exec_policy_engine(&self) -> ExecPolicyEngine { + ExecPolicyEngine::with_rulesets(vec![self.permission_ruleset()]) + } + #[must_use] pub fn get_value(&self, key: &str) -> Option { match key { @@ -435,6 +480,8 @@ impl ConfigToml { "telemetry" => self.telemetry.map(|v| v.to_string()), "approval_policy" => self.approval_policy.clone(), "sandbox_mode" => self.sandbox_mode.clone(), + "auto_allow" => serialize_string_list(&self.auto_allow), + "auto_deny" => serialize_string_list(&self.auto_deny), "providers.deepseek.api_key" => self.providers.deepseek.api_key.clone(), "providers.deepseek.base_url" => self.providers.deepseek.base_url.clone(), "providers.deepseek.model" => self.providers.deepseek.model.clone(), @@ -1621,6 +1668,13 @@ fn serialize_http_headers(headers: &BTreeMap) -> Option ) } +fn serialize_string_list(values: &[String]) -> Option { + if values.is_empty() { + return None; + } + Some(values.join(",")) +} + fn redact_secret(secret: &str) -> String { let chars: Vec = secret.chars().collect(); if chars.len() <= 16 { @@ -1814,6 +1868,77 @@ mod tests { assert!(policy.audit); } + #[test] + fn permissions_toml_deserializes_typed_rules_and_legacy_lists() { + let config: ConfigToml = toml::from_str( + r#" + auto_allow = ["git status"] + auto_deny = ["rm -rf"] + + [[permissions.rules]] + tool = "exec_shell" + decision = "allow" + command = "cargo test" + + [[permissions.rules]] + tool = "file_edit" + decision = "ask" + path = "src/**" + "#, + ) + .expect("permissions toml"); + + assert_eq!(config.auto_allow, ["git status"]); + assert_eq!(config.auto_deny, ["rm -rf"]); + assert_eq!(config.permissions.rules.len(), 2); + assert_eq!( + config.permissions.rules[0], + ToolPermissionRule::exec_shell(PermissionDecision::Allow, "cargo test") + ); + assert_eq!( + config.permissions.rules[1], + ToolPermissionRule::file_path("file_edit", PermissionDecision::Ask, "src/**") + ); + } + + #[test] + fn config_builds_exec_policy_engine_with_legacy_and_typed_rules() { + let config: ConfigToml = toml::from_str( + r#" + auto_allow = ["git status"] + + [[permissions.rules]] + tool = "exec_shell" + decision = "deny" + command = "git status --dangerous" + "#, + ) + .expect("permissions toml"); + let engine = config.exec_policy_engine(); + + let allowed = engine + .check(codewhale_execpolicy::ExecPolicyContext { + command: "git status --porcelain", + cwd: ".", + ask_for_approval: codewhale_execpolicy::AskForApproval::UnlessTrusted, + sandbox_mode: Some("workspace-write"), + }) + .expect("policy decision"); + assert!(allowed.allow); + assert!(!allowed.requires_approval); + + let denied = engine + .check(codewhale_execpolicy::ExecPolicyContext { + command: "git status --dangerous", + cwd: ".", + ask_for_approval: codewhale_execpolicy::AskForApproval::UnlessTrusted, + sandbox_mode: Some("workspace-write"), + }) + .expect("policy decision"); + assert!(!denied.allow); + assert_eq!(denied.requirement.phase(), "forbidden"); + } + struct EnvGuard { deepseek_api_key: Option, deepseek_base_url: Option, diff --git a/crates/execpolicy/Cargo.toml b/crates/execpolicy/Cargo.toml index 669759c49..bf89de939 100644 --- a/crates/execpolicy/Cargo.toml +++ b/crates/execpolicy/Cargo.toml @@ -9,4 +9,5 @@ description = "Execution policy and approval model parity for DeepSeek workspace [dependencies] anyhow.workspace = true codewhale-protocol = { path = "../protocol", version = "0.8.44" } +globset = "0.4.18" serde.workspace = true diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index 14466124a..6b436c4fe 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -5,10 +5,12 @@ use std::collections::HashSet; use anyhow::Result; use bash_arity::BashArityDict; use codewhale_protocol::{NetworkPolicyAmendment, NetworkPolicyRuleAction}; +use globset::GlobBuilder; use serde::{Deserialize, Serialize}; /// Priority layer for a permission ruleset. Higher ordinal = higher priority. -/// On conflict, the highest-priority layer's longest matching prefix wins. +/// Deny rules still win across layers; for non-deny ties the higher-priority +/// layer wins before pattern specificity is considered. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum RulesetLayer { @@ -17,12 +19,100 @@ pub enum RulesetLayer { User = 2, } +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum PermissionDecision { + Allow, + Deny, + Ask, +} + +impl PermissionDecision { + fn precedence(self) -> u8 { + match self { + Self::Deny => 3, + Self::Ask => 2, + Self::Allow => 1, + } + } + + fn as_str(self) -> &'static str { + match self { + Self::Allow => "allow", + Self::Deny => "deny", + Self::Ask => "ask", + } + } +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ToolPermissionRule { + pub tool: String, + pub decision: PermissionDecision, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub command: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub path: Option, +} + +impl ToolPermissionRule { + #[must_use] + pub fn new(tool: impl Into, decision: PermissionDecision) -> Self { + Self { + tool: tool.into(), + decision, + command: None, + path: None, + } + } + + #[must_use] + pub fn exec_shell(decision: PermissionDecision, command: impl Into) -> Self { + Self { + tool: "exec_shell".to_string(), + decision, + command: Some(command.into()), + path: None, + } + } + + #[must_use] + pub fn file_path( + tool: impl Into, + decision: PermissionDecision, + path: impl Into, + ) -> Self { + Self { + tool: tool.into(), + decision, + command: None, + path: Some(path.into()), + } + } + + #[must_use] + pub fn pattern_label(&self) -> String { + let mut parts = vec![format!("tool '{}'", self.tool)]; + if let Some(command) = self.command.as_deref() { + parts.push(format!("command '{command}'")); + } + if let Some(path) = self.path.as_deref() { + parts.push(format!("path '{path}'")); + } + parts.join(", ") + } +} + /// A named set of allow/deny prefix rules at a given priority layer. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct Ruleset { pub layer: RulesetLayer, + #[serde(default)] pub trusted_prefixes: Vec, + #[serde(default)] pub denied_prefixes: Vec, + #[serde(default)] + pub rules: Vec, } impl Ruleset { @@ -31,6 +121,7 @@ impl Ruleset { layer: RulesetLayer::BuiltinDefault, trusted_prefixes: vec![], denied_prefixes: vec![], + rules: vec![], } } @@ -39,6 +130,7 @@ impl Ruleset { layer: RulesetLayer::Agent, trusted_prefixes: trusted, denied_prefixes: denied, + rules: vec![], } } @@ -47,8 +139,15 @@ impl Ruleset { layer: RulesetLayer::User, trusted_prefixes: trusted, denied_prefixes: denied, + rules: vec![], } } + + #[must_use] + pub fn with_rules(mut self, rules: Vec) -> Self { + self.rules = rules; + self + } } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -126,6 +225,52 @@ pub struct ExecPolicyContext<'a> { pub sandbox_mode: Option<&'a str>, } +#[derive(Debug, Clone)] +pub struct ToolPermissionContext<'a> { + pub tool: &'a str, + pub command: Option<&'a str>, + pub path: Option<&'a str>, +} + +impl<'a> ToolPermissionContext<'a> { + #[must_use] + pub fn exec_shell(command: &'a str) -> Self { + Self { + tool: "exec_shell", + command: Some(command), + path: None, + } + } +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct MatchedToolPermissionRule { + pub layer: RulesetLayer, + pub rule: ToolPermissionRule, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ToolPermissionCheck { + pub decision: Option, + pub matched_rule: Option, +} + +impl ToolPermissionCheck { + #[must_use] + pub fn unmatched() -> Self { + Self { + decision: None, + matched_rule: None, + } + } +} + +#[derive(Debug, Clone)] +struct LayeredToolPermissionRule { + layer: RulesetLayer, + rule: ToolPermissionRule, +} + #[derive(Debug, Clone, Default)] pub struct ExecPolicyEngine { /// Layered rulesets (builtin → agent → user). When non-empty, takes precedence @@ -170,28 +315,40 @@ impl ExecPolicyEngine { self.rulesets.sort_by_key(|r| r.layer); } - /// Resolve the effective trusted/denied prefix sets by merging all rulesets. - /// - /// Collects all prefixes from every layer (builtin → agent → user) into flat - /// trusted/denied lists. The `check()` method then applies deny-always-wins - /// semantics: any matching deny prefix blocks the command regardless of layer. - /// Trusted rules are only consulted after deny checks pass. - fn resolve_prefixes(&self) -> (Vec, Vec) { + fn layered_permission_rules(&self) -> Vec { + let mut rules = Vec::new(); if self.rulesets.is_empty() { - return (self.trusted_prefixes.clone(), self.denied_prefixes.clone()); + rules.extend(legacy_command_rules( + RulesetLayer::User, + &self.trusted_prefixes, + &self.denied_prefixes, + )); + return rules; } - // Collect all trusted/denied across all layers, highest-priority last so they - // shadow lower-priority entries with the same prefix. - let mut trusted: Vec = vec![]; - let mut denied: Vec = vec![]; - for rs in &self.rulesets { - trusted.extend(rs.trusted_prefixes.iter().cloned()); - denied.extend(rs.denied_prefixes.iter().cloned()); + + for ruleset in &self.rulesets { + rules.extend( + ruleset + .rules + .iter() + .cloned() + .map(|rule| LayeredToolPermissionRule { + layer: ruleset.layer, + rule, + }), + ); + rules.extend(legacy_command_rules( + ruleset.layer, + &ruleset.trusted_prefixes, + &ruleset.denied_prefixes, + )); } - // Also merge legacy flat lists as user-layer. - trusted.extend(self.trusted_prefixes.iter().cloned()); - denied.extend(self.denied_prefixes.iter().cloned()); - (trusted, denied) + rules.extend(legacy_command_rules( + RulesetLayer::User, + &self.trusted_prefixes, + &self.denied_prefixes, + )); + rules } pub fn remember_session_approval(&mut self, approval_key: String) { @@ -202,62 +359,104 @@ impl ExecPolicyEngine { self.approved_for_session.contains(approval_key) } + #[must_use] + pub fn check_tool_permission(&self, ctx: ToolPermissionContext<'_>) -> ToolPermissionCheck { + let mut best: Option<(LayeredToolPermissionRule, usize)> = None; + + for candidate in self.layered_permission_rules() { + if !tool_rule_matches(&candidate.rule, &ctx, &self.arity_dict) { + continue; + } + let specificity = rule_specificity(&candidate.rule); + let should_replace = best.as_ref().is_none_or(|(current, current_specificity)| { + compare_rule_priority(&candidate, specificity, current, *current_specificity) + }); + if should_replace { + best = Some((candidate, specificity)); + } + } + + match best { + Some((matched, _)) => ToolPermissionCheck { + decision: Some(matched.rule.decision), + matched_rule: Some(MatchedToolPermissionRule { + layer: matched.layer, + rule: matched.rule, + }), + }, + None => ToolPermissionCheck::unmatched(), + } + } + pub fn check(&self, ctx: ExecPolicyContext<'_>) -> Result { - let normalized = normalize_command(ctx.command); - let (trusted_prefixes, denied_prefixes) = self.resolve_prefixes(); - // Deny rules use simple prefix matching (no arity semantics needed). - if let Some(rule) = denied_prefixes - .iter() - .find(|rule| normalized.starts_with(&normalize_command(rule))) + let tool_rule = self.check_tool_permission(ToolPermissionContext::exec_shell(ctx.command)); + if let Some(matched) = tool_rule + .matched_rule + .as_ref() + .filter(|matched| matched.rule.decision == PermissionDecision::Deny) { return Ok(ExecPolicyDecision { allow: false, requires_approval: false, - matched_rule: Some(rule.clone()), + matched_rule: Some(matched.rule.pattern_label()), requirement: ExecApprovalRequirement::Forbidden { - reason: format!("Command blocked by denied prefix rule '{rule}'"), + reason: format!( + "Command blocked by {} rule ({})", + matched.rule.decision.as_str(), + matched.rule.pattern_label() + ), }, }); } - // Allow (trusted) rules use arity-aware prefix matching so that - // `auto_allow = ["git status"]` matches `git status -s` but NOT - // `git push origin main`. - let trusted_rule = trusted_prefixes - .iter() - .find(|rule| self.arity_dict.allow_rule_matches(rule, ctx.command)) - .cloned(); - let is_trusted = trusted_rule.is_some(); - let requirement = match ctx.ask_for_approval { AskForApproval::Never => ExecApprovalRequirement::Skip { bypass_sandbox: false, proposed_execpolicy_amendment: None, }, - AskForApproval::UnlessTrusted if is_trusted => ExecApprovalRequirement::Skip { - bypass_sandbox: false, - proposed_execpolicy_amendment: None, + AskForApproval::Reject { rules, .. } if rules => ExecApprovalRequirement::Forbidden { + reason: "Policy is configured to reject rule-exceptions.".to_string(), }, + _ if matches!(tool_rule.decision, Some(PermissionDecision::Ask)) => { + ExecApprovalRequirement::NeedsApproval { + reason: tool_rule + .matched_rule + .as_ref() + .map(|matched| { + format!( + "Approval required by ask rule ({})", + matched.rule.pattern_label() + ) + }) + .unwrap_or_else(|| "Approval required by ask rule.".to_string()), + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: vec![], + } + } + AskForApproval::OnRequest + if matches!(tool_rule.decision, Some(PermissionDecision::Allow)) => + { + ExecApprovalRequirement::NeedsApproval { + reason: "Approval requested by policy mode.".to_string(), + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: vec![], + } + } + _ if matches!(tool_rule.decision, Some(PermissionDecision::Allow)) => { + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + } + } AskForApproval::OnFailure => ExecApprovalRequirement::Skip { bypass_sandbox: false, proposed_execpolicy_amendment: None, }, - AskForApproval::Reject { rules, .. } if rules => ExecApprovalRequirement::Forbidden { - reason: "Policy is configured to reject rule-exceptions.".to_string(), - }, _ => ExecApprovalRequirement::NeedsApproval { - reason: if is_trusted { - "Approval requested by policy mode.".to_string() - } else { - "Unmatched command prefix requires approval.".to_string() - }, - proposed_execpolicy_amendment: if is_trusted { - None - } else { - Some(ExecPolicyAmendment { - prefixes: vec![first_token(ctx.command)], - }) - }, + reason: "Unmatched command prefix requires approval.".to_string(), + proposed_execpolicy_amendment: Some(ExecPolicyAmendment { + prefixes: vec![first_token(ctx.command)], + }), proposed_network_policy_amendments: vec![NetworkPolicyAmendment { host: ctx.cwd.to_string(), action: NetworkPolicyRuleAction::Allow, @@ -274,14 +473,177 @@ impl ExecPolicyEngine { Ok(ExecPolicyDecision { allow, requires_approval, - matched_rule: trusted_rule, + matched_rule: tool_rule + .matched_rule + .as_ref() + .map(|matched| matched.rule.pattern_label()), requirement, }) } } +fn legacy_command_rules( + layer: RulesetLayer, + trusted_prefixes: &[String], + denied_prefixes: &[String], +) -> Vec { + trusted_prefixes + .iter() + .map(|command| LayeredToolPermissionRule { + layer, + rule: ToolPermissionRule::exec_shell(PermissionDecision::Allow, command.clone()), + }) + .chain( + denied_prefixes + .iter() + .map(|command| LayeredToolPermissionRule { + layer, + rule: ToolPermissionRule::exec_shell(PermissionDecision::Deny, command.clone()), + }), + ) + .collect() +} + +fn compare_rule_priority( + candidate: &LayeredToolPermissionRule, + candidate_specificity: usize, + current: &LayeredToolPermissionRule, + current_specificity: usize, +) -> bool { + ( + candidate.rule.decision.precedence(), + candidate.layer, + candidate_specificity, + ) > ( + current.rule.decision.precedence(), + current.layer, + current_specificity, + ) +} + +fn tool_rule_matches( + rule: &ToolPermissionRule, + ctx: &ToolPermissionContext<'_>, + arity_dict: &BashArityDict, +) -> bool { + if !rule.tool.eq_ignore_ascii_case(ctx.tool) { + return false; + } + if let Some(command_rule) = rule.command.as_deref() { + let Some(command) = ctx.command else { + return false; + }; + if !command_rule_matches(rule.decision, command_rule, command, arity_dict) { + return false; + } + } + if let Some(path_rule) = rule.path.as_deref() { + let Some(path) = ctx.path else { + return false; + }; + if !path_pattern_matches(path_rule, path) { + return false; + } + } + true +} + +fn command_rule_matches( + decision: PermissionDecision, + pattern: &str, + command: &str, + arity_dict: &BashArityDict, +) -> bool { + match decision { + PermissionDecision::Deny => command_prefix_matches(pattern, command), + PermissionDecision::Allow | PermissionDecision::Ask => { + arity_dict.allow_rule_matches(pattern, command) + } + } +} + +fn path_pattern_matches(pattern: &str, path: &str) -> bool { + let pattern = normalize_path_pattern(pattern); + let path = normalize_path_pattern(path); + if let Some(prefix) = pattern.strip_suffix("/**") + && (path == prefix || path.starts_with(&format!("{prefix}/"))) + { + return true; + } + if !pattern.contains('*') && !pattern.contains('?') { + return pattern == path; + } + GlobBuilder::new(&pattern) + .literal_separator(true) + .build() + .map(|glob| glob.compile_matcher().is_match(path)) + .unwrap_or(false) +} + +fn rule_specificity(rule: &ToolPermissionRule) -> usize { + let mut score = rule.tool.len(); + if let Some(command) = rule.command.as_deref() { + score += 1_000 + non_wildcard_len(command); + } + if let Some(path) = rule.path.as_deref() { + score += 1_000 + non_wildcard_len(path); + } + score +} + +fn non_wildcard_len(value: &str) -> usize { + value.chars().filter(|ch| *ch != '*' && *ch != '?').count() +} + fn normalize_command(value: &str) -> String { - value.trim().to_ascii_lowercase() + value + .trim() + .to_ascii_lowercase() + .split_whitespace() + .collect::>() + .join(" ") +} + +fn command_prefix_matches(pattern: &str, command: &str) -> bool { + let pattern = normalize_command(pattern); + if pattern.is_empty() { + return false; + } + let command = normalize_command(command); + command + .strip_prefix(&pattern) + .is_some_and(|suffix| suffix.is_empty() || suffix.starts_with(' ')) +} + +fn normalize_path_pattern(value: &str) -> String { + let raw = value.trim().replace('\\', "/"); + let absolute = raw.starts_with('/'); + let mut segments = Vec::new(); + + for segment in raw.split('/') { + match segment { + "" | "." => {} + ".." => { + if absolute { + segments.pop(); + } else if segments.is_empty() || segments.last() == Some(&"..") { + segments.push(segment); + } else { + segments.pop(); + } + } + _ => segments.push(segment), + } + } + + let normalized = segments.join("/"); + if absolute && normalized.is_empty() { + "/".to_string() + } else if absolute { + format!("/{normalized}") + } else { + normalized + } } fn first_token(command: &str) -> String { @@ -305,6 +667,10 @@ mod tests { } } + fn exec_ctx(command: &str) -> ExecPolicyContext<'_> { + ctx(command, AskForApproval::UnlessTrusted) + } + #[test] fn trusted_prefix_skips_approval_when_policy_is_unless_trusted() { let engine = ExecPolicyEngine::new(vec!["git status".to_string()], vec![]); @@ -315,7 +681,10 @@ mod tests { assert!(decision.allow); assert!(!decision.requires_approval); - assert_eq!(decision.matched_rule.as_deref(), Some("git status")); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool 'exec_shell', command 'git status'") + ); assert!(matches!( decision.requirement, ExecApprovalRequirement::Skip { @@ -338,14 +707,45 @@ mod tests { assert!(!decision.allow); assert!(!decision.requires_approval); - assert_eq!(decision.matched_rule.as_deref(), Some("git status")); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool 'exec_shell', command 'git status'") + ); assert!(matches!( decision.requirement, ExecApprovalRequirement::Forbidden { .. } )); assert_eq!( decision.reason(), - "Command blocked by denied prefix rule 'git status'" + "Command blocked by deny rule (tool 'exec_shell', command 'git status')" + ); + } + + #[test] + fn denied_prefix_respects_command_word_boundaries() { + let engine = ExecPolicyEngine::new(vec![], vec!["ls".to_string()]); + + let blocked = engine.check(exec_ctx("ls -la")).unwrap(); + assert!(!blocked.allow); + assert_eq!(blocked.requirement.phase(), "forbidden"); + + let separate_binary = engine.check(exec_ctx("ls-remote origin")).unwrap(); + assert!(separate_binary.allow); + assert!(separate_binary.requires_approval); + assert_eq!(separate_binary.matched_rule, None); + } + + #[test] + fn legacy_auto_allow_prefixes_become_exec_shell_rules() { + let engine = ExecPolicyEngine::new(vec!["git status".to_string()], vec![]); + + let decision = engine.check(exec_ctx("git status --porcelain")).unwrap(); + + assert!(decision.allow); + assert!(!decision.requires_approval); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool 'exec_shell', command 'git status'") ); } @@ -389,7 +789,10 @@ mod tests { assert!(decision.allow); assert!(decision.requires_approval); - assert_eq!(decision.matched_rule.as_deref(), Some("cargo test")); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool 'exec_shell', command 'cargo test'") + ); match decision.requirement { ExecApprovalRequirement::NeedsApproval { proposed_execpolicy_amendment, @@ -423,4 +826,124 @@ mod tests { "Policy is configured to reject rule-exceptions." ); } + + #[test] + fn legacy_auto_allow_uses_bash_arity() { + let engine = ExecPolicyEngine::new(vec!["git status".to_string()], vec![]); + + let decision = engine.check(exec_ctx("git push origin main")).unwrap(); + + assert!(decision.requires_approval); + assert_eq!(decision.matched_rule, None); + } + + #[test] + fn deny_rule_wins_over_user_allow_rule() { + let engine = ExecPolicyEngine::with_rulesets(vec![ + Ruleset::builtin_default().with_rules(vec![ToolPermissionRule::exec_shell( + PermissionDecision::Deny, + "cargo", + )]), + Ruleset::user(vec![], vec![]).with_rules(vec![ToolPermissionRule::exec_shell( + PermissionDecision::Allow, + "cargo test", + )]), + ]); + + let decision = engine.check(exec_ctx("cargo test --workspace")).unwrap(); + + assert!(!decision.allow); + assert_eq!(decision.requirement.phase(), "forbidden"); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool 'exec_shell', command 'cargo'") + ); + } + + #[test] + fn ask_rule_wins_over_allow_rule() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_rules(vec![ + ToolPermissionRule::exec_shell(PermissionDecision::Allow, "cargo"), + ToolPermissionRule::exec_shell(PermissionDecision::Ask, "cargo test"), + ])]); + + let decision = engine.check(exec_ctx("cargo test --workspace")).unwrap(); + + assert!(decision.allow); + assert!(decision.requires_approval); + assert_eq!(decision.requirement.phase(), "needs_approval"); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool 'exec_shell', command 'cargo test'") + ); + } + + #[test] + fn ask_rule_overrides_on_failure_policy() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_rules(vec![ + ToolPermissionRule::exec_shell(PermissionDecision::Ask, "cargo test"), + ])]); + + let decision = engine + .check(ExecPolicyContext { + command: "cargo test --workspace", + cwd: ".", + ask_for_approval: AskForApproval::OnFailure, + sandbox_mode: Some("workspace-write"), + }) + .unwrap(); + + assert!(decision.allow); + assert!(decision.requires_approval); + assert_eq!(decision.requirement.phase(), "needs_approval"); + } + + #[test] + fn path_rules_match_workspace_relative_globs() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_rules(vec![ + ToolPermissionRule::file_path("file_edit", PermissionDecision::Allow, "docs/**"), + ])]); + + let decision = engine.check_tool_permission(ToolPermissionContext { + tool: "file_edit", + command: None, + path: Some("./docs/guide/setup.md"), + }); + + assert_eq!(decision.decision, Some(PermissionDecision::Allow)); + } + + #[test] + fn path_star_does_not_cross_directory_separator() { + assert!(path_pattern_matches("docs/*.md", "docs/readme.md")); + assert!(!path_pattern_matches("docs/*.md", "docs/guides/readme.md")); + } + + #[test] + fn path_rules_normalize_dot_segments() { + assert!(path_pattern_matches("src/**", "src/./lib.rs")); + assert!(!path_pattern_matches("src/**", "src/../secret.txt")); + assert!(!path_pattern_matches("src/**", "../src/lib.rs")); + assert!(path_pattern_matches("/foo", "/../foo")); + assert!(!path_pattern_matches("/src/**", "/src/../secret.txt")); + } + + #[test] + fn tool_name_only_rule_matches_unknown_tool() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_rules(vec![ + ToolPermissionRule::new("agent_spawn", PermissionDecision::Ask), + ])]); + + let decision = engine.check_tool_permission(ToolPermissionContext { + tool: "agent_spawn", + command: None, + path: None, + }); + + assert_eq!(decision.decision, Some(PermissionDecision::Ask)); + } } diff --git a/crates/tui/src/composer_history.rs b/crates/tui/src/composer_history.rs index 4f8bb1ce9..ff487c9e0 100644 --- a/crates/tui/src/composer_history.rs +++ b/crates/tui/src/composer_history.rs @@ -33,6 +33,9 @@ use std::time::Duration; pub const MAX_HISTORY_ENTRIES: usize = 1000; const HISTORY_FILE_NAME: &str = "composer_history.txt"; +// Prevent a steady stream of test/UI writes from delaying persistence +// indefinitely while waiting for a 2ms idle window. +const WRITER_BATCH_LIMIT: usize = 128; fn default_history_path() -> Option { dirs::home_dir().map(|home| home.join(".deepseek").join(HISTORY_FILE_NAME)) @@ -114,7 +117,7 @@ fn writer_sender() -> &'static Sender<(PathBuf, String)> { fn append_history_batch(rx: &Receiver<(PathBuf, String)>, first: (PathBuf, String)) { let mut pending = vec![first]; - loop { + while pending.len() < WRITER_BATCH_LIMIT { match rx.recv_timeout(Duration::from_millis(2)) { Ok(next) => pending.push(next), Err(RecvTimeoutError::Timeout) => break, diff --git a/crates/tui/src/tools/pandoc.rs b/crates/tui/src/tools/pandoc.rs index c8a5b78d7..6af3cb4bd 100644 --- a/crates/tui/src/tools/pandoc.rs +++ b/crates/tui/src/tools/pandoc.rs @@ -144,7 +144,9 @@ impl ToolSpec for PandocConvertTool { // gated on resolve_pandoc(), but a concurrent uninstall between // catalog build and the model's call should produce a clear // error rather than the cryptic "program not found" from raw - // Command::spawn. + // Command::spawn. Input validation above intentionally runs first + // so tests and users get deterministic schema/actionable errors + // even on machines without pandoc installed. let pandoc = crate::dependencies::resolve_pandoc().ok_or_else(|| { ToolError::execution_failed( "pandoc_convert: pandoc binary not found on PATH. \ diff --git a/crates/tui/src/tui/sidebar.rs b/crates/tui/src/tui/sidebar.rs index bff5c51a0..5f557dd78 100644 --- a/crates/tui/src/tui/sidebar.rs +++ b/crates/tui/src/tui/sidebar.rs @@ -1851,8 +1851,8 @@ mod tests { use super::{ ACTIVE_TOOL_COMPLETED_ROW_TTL, ACTIVE_TOOL_STALE_RUNNING_ROW_TTL, AutoSidebarPanel, AutoSidebarState, SidebarAgentRow, SidebarSubagentSummary, SidebarWorkChecklistItem, - SidebarWorkStrategyStep, SidebarWorkSummary, auto_sidebar_panels, subagent_panel_lines, - task_panel_lines, work_panel_empty_hint, work_panel_lines, + SidebarWorkStrategyStep, SidebarWorkSummary, auto_sidebar_panels, duration_ms, + subagent_panel_lines, task_panel_lines, work_panel_empty_hint, work_panel_lines, }; use crate::config::Config; use crate::palette::PaletteMode; @@ -2195,6 +2195,8 @@ mod tests { fn tasks_panel_collapses_stale_running_tool_rows() { let mut app = create_test_app(); let mut active = ActiveCell::new(); + let stale_duration_ms = + duration_ms(ACTIVE_TOOL_STALE_RUNNING_ROW_TTL).saturating_add(1_000); for (idx, command) in ["long one", "long two"].into_iter().enumerate() { active.push_tool( format!("shell-{idx}"), @@ -2203,7 +2205,7 @@ mod tests { status: ToolStatus::Running, output: None, started_at: None, - duration_ms: Some(ACTIVE_TOOL_STALE_RUNNING_ROW_TTL.as_millis() as u64 + 1), + duration_ms: Some(stale_duration_ms), source: ExecSource::Assistant, interaction: None, output_summary: None, diff --git a/crates/tui/src/vision/tools.rs b/crates/tui/src/vision/tools.rs index bfce551df..d7ff37e61 100644 --- a/crates/tui/src/vision/tools.rs +++ b/crates/tui/src/vision/tools.rs @@ -270,13 +270,13 @@ mod tests { let tmp = tempdir().expect("tempdir"); let ctx = ToolContext::new(tmp.path().to_path_buf()); let tool = ImageAnalyzeTool::new(fake_config()); - let outside_workspace = if cfg!(windows) { + let absolute_path = if cfg!(windows) { r"C:\Windows\System32\drivers\etc\hosts" } else { "/etc/hosts" }; let err = tool - .execute(json!({"image_path": outside_workspace}), &ctx) + .execute(json!({"image_path": absolute_path}), &ctx) .await .expect_err("absolute path must reject"); assert!(