From 069ef58add441c3ebef96dc5dc5b70a6c6b804dc Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Fri, 8 May 2026 17:16:54 +0800 Subject: [PATCH 01/19] feat(execpolicy): add typed permission rules --- Cargo.lock | 2 +- config.example.toml | 15 + crates/app-server/src/lib.rs | 3 +- crates/config/Cargo.toml | 1 + crates/config/src/lib.rs | 125 +++++++ crates/execpolicy/src/lib.rs | 634 +++++++++++++++++++++++++++++++---- 6 files changed, 715 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2d5bd8e14..b40a6476e 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", @@ -1391,7 +1392,6 @@ dependencies = [ "serde_json", ] -[[package]] name = "deltae" version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" 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..2bf86403a 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(deepseek_execpolicy::ExecPolicyContext { + command: "git status --porcelain", + cwd: ".", + ask_for_approval: deepseek_execpolicy::AskForApproval::UnlessTrusted, + sandbox_mode: Some("workspace-write"), + }) + .expect("policy decision"); + assert!(allowed.allow); + assert!(!allowed.requires_approval); + + let denied = engine + .check(deepseek_execpolicy::ExecPolicyContext { + command: "git status --dangerous", + cwd: ".", + ask_for_approval: deepseek_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/src/lib.rs b/crates/execpolicy/src/lib.rs index 14466124a..6e9907eef 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -8,7 +8,8 @@ use codewhale_protocol::{NetworkPolicyAmendment, NetworkPolicyRuleAction}; 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 +18,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 +120,7 @@ impl Ruleset { layer: RulesetLayer::BuiltinDefault, trusted_prefixes: vec![], denied_prefixes: vec![], + rules: vec![], } } @@ -39,6 +129,7 @@ impl Ruleset { layer: RulesetLayer::Agent, trusted_prefixes: trusted, denied_prefixes: denied, + rules: vec![], } } @@ -47,8 +138,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 +224,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 +314,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 +358,95 @@ 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![], + } + } + _ 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 +463,190 @@ 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 => { + normalize_command(command).starts_with(&normalize_command(pattern)) + } + 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; + } + glob_match(pattern.as_bytes(), path.as_bytes()) +} + +fn glob_match(pattern: &[u8], path: &[u8]) -> bool { + if pattern.is_empty() { + return path.is_empty(); + } + + match pattern[0] { + b'*' if pattern.get(1) == Some(&b'*') => { + let rest = &pattern[2..]; + (0..=path.len()).any(|idx| glob_match(rest, &path[idx..])) + } + b'*' => { + if glob_match(&pattern[1..], path) { + return true; + } + let mut idx = 0; + while idx < path.len() && path[idx] != b'/' { + idx += 1; + if glob_match(&pattern[1..], &path[idx..]) { + return true; + } + } + false + } + b'?' => !path.is_empty() && path[0] != b'/' && glob_match(&pattern[1..], &path[1..]), + literal => !path.is_empty() && path[0] == literal && glob_match(&pattern[1..], &path[1..]), + } +} + +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 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 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 +670,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 +684,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 +710,31 @@ 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 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 +778,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 +815,122 @@ 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")); + } + + #[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)); + } } From e356f96a1b3acf8c300974c7bcb8fc6696d781eb Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Fri, 8 May 2026 17:46:17 +0800 Subject: [PATCH 02/19] fix(execpolicy): harden path permission matching --- Cargo.lock | 1 + crates/execpolicy/Cargo.toml | 1 + crates/execpolicy/src/lib.rs | 41 ++++++++++-------------------------- 3 files changed, 13 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b40a6476e..9d12f18dc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -893,6 +893,7 @@ version = "0.8.44" dependencies = [ "anyhow", "codewhale-protocol", + "globset", "serde", ] 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 6e9907eef..a74bc1afb 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -5,6 +5,7 @@ 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. @@ -565,35 +566,11 @@ fn path_pattern_matches(pattern: &str, path: &str) -> bool { if !pattern.contains('*') && !pattern.contains('?') { return pattern == path; } - glob_match(pattern.as_bytes(), path.as_bytes()) -} - -fn glob_match(pattern: &[u8], path: &[u8]) -> bool { - if pattern.is_empty() { - return path.is_empty(); - } - - match pattern[0] { - b'*' if pattern.get(1) == Some(&b'*') => { - let rest = &pattern[2..]; - (0..=path.len()).any(|idx| glob_match(rest, &path[idx..])) - } - b'*' => { - if glob_match(&pattern[1..], path) { - return true; - } - let mut idx = 0; - while idx < path.len() && path[idx] != b'/' { - idx += 1; - if glob_match(&pattern[1..], &path[idx..]) { - return true; - } - } - false - } - b'?' => !path.is_empty() && path[0] != b'/' && glob_match(&pattern[1..], &path[1..]), - literal => !path.is_empty() && path[0] == literal && glob_match(&pattern[1..], &path[1..]), - } + GlobBuilder::new(&pattern) + .literal_separator(true) + .build() + .map(|glob| glob.compile_matcher().is_match(path)) + .unwrap_or(false) } fn rule_specificity(rule: &ToolPermissionRule) -> usize { @@ -629,7 +606,9 @@ fn normalize_path_pattern(value: &str) -> String { match segment { "" | "." => {} ".." => { - if segments.is_empty() || segments.last() == Some(&"..") { + if absolute { + segments.pop(); + } else if segments.is_empty() || segments.last() == Some(&"..") { segments.push(segment); } else { segments.pop(); @@ -916,6 +895,8 @@ mod tests { 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] From 3140c185a4386ba3705bc49f7a87eb00d9f216c4 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 11 May 2026 10:10:39 +0800 Subject: [PATCH 03/19] fix(execpolicy): preserve on-request approval semantics --- crates/execpolicy/src/lib.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index a74bc1afb..f58269719 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -433,6 +433,15 @@ impl ExecPolicyEngine { 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, From d3a122a613ddd5c621a17597bcc23c6b25c8cd5f Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Tue, 12 May 2026 17:10:23 +0800 Subject: [PATCH 04/19] fix(pandoc): validate binary output before resolving binary --- crates/tui/src/tools/pandoc.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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. \ From eedaa164b97c12996097c07a3a01856d219bf2bc Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Tue, 12 May 2026 17:29:09 +0800 Subject: [PATCH 05/19] fix(vision): use platform absolute path in boundary test --- crates/tui/src/vision/tools.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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!( From 42dfd4bfc2b453c4e51274154c2d5943683844a5 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Thu, 14 May 2026 11:41:54 +0800 Subject: [PATCH 06/19] test(tui): avoid instant underflow in sidebar test --- crates/tui/src/tui/sidebar.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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, From 4e58b9532f4d6597d8cfe92ed8c92721ee60d469 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Thu, 14 May 2026 12:26:59 +0800 Subject: [PATCH 07/19] ci: avoid caching cargo bin shims --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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: From 78e4243a2173e0d62085981afeb74a3a93cc5401 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 25 May 2026 10:48:26 +0800 Subject: [PATCH 08/19] fix(config): update rebased execpolicy references --- Cargo.lock | 1 + crates/config/src/lib.rs | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9d12f18dc..950be447a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1393,6 +1393,7 @@ dependencies = [ "serde_json", ] +[[package]] name = "deltae" version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 2bf86403a..6c752cf2f 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -1917,10 +1917,10 @@ mod tests { let engine = config.exec_policy_engine(); let allowed = engine - .check(deepseek_execpolicy::ExecPolicyContext { + .check(codewhale_execpolicy::ExecPolicyContext { command: "git status --porcelain", cwd: ".", - ask_for_approval: deepseek_execpolicy::AskForApproval::UnlessTrusted, + ask_for_approval: codewhale_execpolicy::AskForApproval::UnlessTrusted, sandbox_mode: Some("workspace-write"), }) .expect("policy decision"); @@ -1928,10 +1928,10 @@ mod tests { assert!(!allowed.requires_approval); let denied = engine - .check(deepseek_execpolicy::ExecPolicyContext { + .check(codewhale_execpolicy::ExecPolicyContext { command: "git status --dangerous", cwd: ".", - ask_for_approval: deepseek_execpolicy::AskForApproval::UnlessTrusted, + ask_for_approval: codewhale_execpolicy::AskForApproval::UnlessTrusted, sandbox_mode: Some("workspace-write"), }) .expect("policy decision"); From db1e3dfc8762a0de0d34fdbf7f11ac3f1ecdd8c2 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 25 May 2026 11:02:04 +0800 Subject: [PATCH 09/19] fix(tui): bound composer history writer batches --- crates/tui/src/composer_history.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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, From 75eeb313984294787118506901ed5201cf90f254 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 25 May 2026 11:10:42 +0800 Subject: [PATCH 10/19] fix(execpolicy): respect deny command boundaries --- crates/execpolicy/src/lib.rs | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index f58269719..6b436c4fe 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -555,9 +555,7 @@ fn command_rule_matches( arity_dict: &BashArityDict, ) -> bool { match decision { - PermissionDecision::Deny => { - normalize_command(command).starts_with(&normalize_command(pattern)) - } + PermissionDecision::Deny => command_prefix_matches(pattern, command), PermissionDecision::Allow | PermissionDecision::Ask => { arity_dict.allow_rule_matches(pattern, command) } @@ -606,6 +604,17 @@ fn normalize_command(value: &str) -> String { .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('/'); @@ -712,6 +721,20 @@ mod tests { ); } + #[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![]); From 501dc0df3acf7db4a3f3968fe40565e059a76bc5 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Sat, 9 May 2026 16:53:01 +0800 Subject: [PATCH 11/19] feat(tui): route shell and file tools through typed permission rules --- Cargo.lock | 1 + config.example.toml | 2 +- crates/config/src/lib.rs | 4 +- crates/execpolicy/src/lib.rs | 8 +- crates/tui/Cargo.toml | 1 + crates/tui/src/config.rs | 90 +++++ crates/tui/src/core/engine.rs | 11 +- crates/tui/src/core/engine/dispatch.rs | 461 +++++++++++++++++++++++- crates/tui/src/core/engine/tests.rs | 204 +++++++++++ crates/tui/src/core/engine/turn_loop.rs | 61 +++- crates/tui/src/main.rs | 115 ++++++ crates/tui/src/runtime_threads.rs | 1 + crates/tui/src/tui/ui.rs | 1 + 13 files changed, 935 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 950be447a..4a686dd6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -980,6 +980,7 @@ dependencies = [ "clap", "clap_complete", "codewhale-config", + "codewhale-execpolicy", "codewhale-secrets", "codewhale-tools", "colored", diff --git a/config.example.toml b/config.example.toml index c473565f6..79fd55be9 100644 --- a/config.example.toml +++ b/config.example.toml @@ -153,7 +153,7 @@ sandbox_mode = "workspace-write" # read-only | workspace-write | danger-full-acc # command = "cargo test" # # [[permissions.rules]] -# tool = "file_edit" +# tool = "edit_file" # decision = "ask" # path = "src/**" max_subagents = 10 # optional (1-20) diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 6c752cf2f..d8b316fdd 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -1881,7 +1881,7 @@ mod tests { command = "cargo test" [[permissions.rules]] - tool = "file_edit" + tool = "edit_file" decision = "ask" path = "src/**" "#, @@ -1897,7 +1897,7 @@ mod tests { ); assert_eq!( config.permissions.rules[1], - ToolPermissionRule::file_path("file_edit", PermissionDecision::Ask, "src/**") + ToolPermissionRule::file_path("edit_file", PermissionDecision::Ask, "src/**") ); } diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index 6b436c4fe..c60a9e5b9 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -565,10 +565,10 @@ fn command_rule_matches( 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 let Some(prefix) = pattern.strip_suffix("/**") { + if path == prefix || path.starts_with(&format!("{prefix}/")) { + return true; + } } if !pattern.contains('*') && !pattern.contains('?') { return pattern == path; diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index f78b57a42..2b8baf1ea 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -28,6 +28,7 @@ path = "src/bin/deepseek_tui_legacy_shim.rs" anyhow = "1.0.100" arboard = "3.4" codewhale-config = { path = "../config", version = "0.8.44" } +codewhale-execpolicy = { path = "../execpolicy", version = "0.8.44" } codewhale-secrets = { path = "../secrets", version = "0.8.44" } codewhale-tools = { path = "../tools", version = "0.8.44" } schemaui = { version = "0.12.0", default-features = false, optional = true } diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index b41712557..f4a769bc9 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -8,6 +8,7 @@ use std::io::Write as _; use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; +use codewhale_execpolicy::{ExecPolicyEngine, Ruleset, ToolPermissionRule}; use serde::{Deserialize, Serialize}; use serde_json::json; #[cfg(unix)] @@ -913,6 +914,12 @@ pub struct AutoConfig { pub cost_saving: Option, } +#[derive(Debug, Clone, Deserialize, Default)] +pub struct PermissionsConfig { + #[serde(default)] + pub rules: Vec, +} + /// Resolved CLI configuration, including defaults and environment overrides. #[derive(Debug, Clone, Default, Deserialize)] pub struct Config { @@ -947,6 +954,15 @@ pub struct Config { pub approval_policy: Option, pub sandbox_mode: Option, pub yolo: Option, + /// Legacy shell command permission rules. These are translated into typed + /// `exec_shell` allow/deny rules before tool execution. + #[serde(default)] + pub auto_allow: Vec, + #[serde(default)] + pub auto_deny: Vec, + /// Typed, persistent tool permission rules. + #[serde(default)] + pub permissions: PermissionsConfig, /// External sandbox backend: `"none"` or `"opensandbox"`. /// When set, exec_shell routes commands through the backend's HTTP API /// instead of spawning a local process. @@ -1818,6 +1834,20 @@ impl Config { self.allow_shell.unwrap_or(false) } + /// Build the user-layer permission ruleset used by the tool execution + /// approval path. + #[must_use] + pub fn permission_ruleset(&self) -> Ruleset { + Ruleset::user(self.auto_allow.clone(), self.auto_deny.clone()) + .with_rules(self.permissions.rules.clone()) + } + + /// Build the permission engine for shell/file tool approval decisions. + #[must_use] + pub fn exec_policy_engine(&self) -> ExecPolicyEngine { + ExecPolicyEngine::with_rulesets(vec![self.permission_ruleset()]) + } + /// Return the maximum number of concurrent sub-agents. /// Checks `[subagents] max_concurrent` first, then top-level `max_subagents`, /// then falls back to `DEFAULT_MAX_SUBAGENTS`. @@ -2917,6 +2947,9 @@ fn merge_config(base: Config, override_cfg: Config) -> Config { yolo: override_cfg.yolo.or(base.yolo), approval_policy: override_cfg.approval_policy.or(base.approval_policy), sandbox_mode: override_cfg.sandbox_mode.or(base.sandbox_mode), + auto_allow: merge_non_empty_vec(base.auto_allow, override_cfg.auto_allow), + auto_deny: merge_non_empty_vec(base.auto_deny, override_cfg.auto_deny), + permissions: merge_permissions_config(base.permissions, override_cfg.permissions), sandbox_backend: override_cfg.sandbox_backend.or(base.sandbox_backend), sandbox_url: override_cfg.sandbox_url.or(base.sandbox_url), sandbox_api_key: override_cfg.sandbox_api_key.or(base.sandbox_api_key), @@ -2974,6 +3007,25 @@ fn merge_config(base: Config, override_cfg: Config) -> Config { } } +fn merge_non_empty_vec(base: Vec, override_cfg: Vec) -> Vec { + if override_cfg.is_empty() { + base + } else { + override_cfg + } +} + +fn merge_permissions_config( + base: PermissionsConfig, + override_cfg: PermissionsConfig, +) -> PermissionsConfig { + if override_cfg.rules.is_empty() { + base + } else { + override_cfg + } +} + fn merge_provider_config(base: ProviderConfig, override_cfg: ProviderConfig) -> ProviderConfig { ProviderConfig { api_key: override_cfg.api_key.or(base.api_key), @@ -4046,6 +4098,44 @@ mod tests { ); } + #[test] + fn config_loads_typed_permission_rules_for_exec_policy_engine() -> Result<()> { + let _guard = lock_test_env(); + let temp = tempfile::tempdir()?; + let _env = EnvGuard::new(temp.path()); + let config_path = temp.path().join("config.toml"); + fs::write( + &config_path, + r#" +auto_allow = ["git status"] +auto_deny = ["rm -rf"] + +[[permissions.rules]] +tool = "read_file" +decision = "ask" +path = "secrets/**" +"#, + )?; + + let config = Config::load(Some(config_path), None)?; + assert_eq!(config.auto_allow, ["git status"]); + assert_eq!(config.auto_deny, ["rm -rf"]); + + let decision = config.exec_policy_engine().check_tool_permission( + codewhale_execpolicy::ToolPermissionContext { + tool: "read_file", + command: None, + path: Some("secrets/token.txt"), + }, + ); + + assert_eq!( + decision.decision, + Some(codewhale_execpolicy::PermissionDecision::Ask) + ); + Ok(()) + } + #[test] fn save_api_key_writes_config_file_under_cfg_test() -> Result<()> { // `save_api_key` writes to the shared user config file. This diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index fc286d583..3379d8403 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -171,6 +171,9 @@ pub struct EngineConfig { /// once at engine construction, then threaded onto every /// `SubAgentRuntime` the engine builds (#1806, #1808). pub subagent_api_timeout: Duration, + /// Typed permission rules used before falling back to the legacy approval + /// prompt path for shell and file tools. + pub exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine, } impl Default for EngineConfig { @@ -214,6 +217,7 @@ impl Default for EngineConfig { subagent_api_timeout: Duration::from_secs( crate::config::DEFAULT_SUBAGENT_API_TIMEOUT_SECS, ), + exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine::default(), } } } @@ -1981,10 +1985,11 @@ use self::approval::{ApprovalDecision, ApprovalResult, UserInputDecision}; use self::dispatch::should_parallelize_tool_batch; use self::dispatch::{ ParallelToolResult, ParallelToolResultEntry, ToolExecGuard, ToolExecOutcome, - ToolExecutionBatch, ToolExecutionPlan, caller_allowed_for_tool, caller_type_for_tool_use, - final_tool_input, format_tool_error, mcp_tool_approval_description, mcp_tool_is_parallel_safe, - mcp_tool_is_read_only, parse_parallel_tool_calls, parse_tool_input, + ToolExecutionBatch, ToolExecutionPlan, ToolPermissionOverride, caller_allowed_for_tool, + caller_type_for_tool_use, final_tool_input, format_tool_error, mcp_tool_approval_description, + mcp_tool_is_parallel_safe, mcp_tool_is_read_only, parse_parallel_tool_calls, parse_tool_input, plan_tool_execution_batches, should_force_update_plan_first, should_stop_after_plan_tool, + tool_permission_override_for_call, }; use self::loop_guard::{AttemptDecision, LoopGuard, OutcomeDecision}; #[cfg(test)] diff --git a/crates/tui/src/core/engine/dispatch.rs b/crates/tui/src/core/engine/dispatch.rs index 335639c4e..08dd1caf3 100644 --- a/crates/tui/src/core/engine/dispatch.rs +++ b/crates/tui/src/core/engine/dispatch.rs @@ -15,13 +15,18 @@ //! All items are `pub(super)`-only: the public engine surface (Op/Event, //! `EngineHandle`, `spawn_engine`) stays in `core/engine.rs`. -use serde_json::json; +use codewhale_execpolicy::{ + ExecPolicyEngine, PermissionDecision, ToolPermissionCheck, ToolPermissionContext, +}; +use serde_json::{Value, json}; +use std::path::{Component, Path, PathBuf}; use crate::models::{Tool, ToolCaller}; use crate::tools::spec::{ToolError, ToolResult}; use crate::tui::app::AppMode; use super::ToolUseState; +use super::tool_catalog::MULTI_TOOL_PARALLEL_NAME; // === Types ============================================================ @@ -70,6 +75,14 @@ pub(super) struct ParallelToolResult { pub(super) results: Vec, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum ToolPermissionOverride { + Unmatched, + Allow { reason: String }, + Ask { reason: String }, + Deny { reason: String }, +} + // Hold the lock guard for the duration of a tool execution. // The inner guards are held for RAII purposes (dropped when the guard is dropped). pub(super) enum ToolExecGuard<'a> { @@ -99,6 +112,452 @@ pub(super) fn caller_allowed_for_tool( requested == "direct" } +// === Typed permission rules ========================================== + +pub(super) fn tool_permission_override_for_call( + engine: &ExecPolicyEngine, + tool_name: &str, + input: &Value, + workspace: &Path, +) -> ToolPermissionOverride { + if tool_name == MULTI_TOOL_PARALLEL_NAME { + return permission_override_for_parallel_call(engine, input, workspace); + } + + if is_shell_permission_tool(tool_name) { + let command = input.get("command").and_then(Value::as_str); + return permission_override_for_context(engine, tool_name, command, None); + } + + if !is_file_permission_tool(tool_name) { + return ToolPermissionOverride::Unmatched; + } + + let paths = permission_paths_for_file_tool(tool_name, input); + if paths.is_empty() { + return permission_override_for_context(engine, tool_name, None, None); + } + + let mut saw_allow = false; + let mut saw_unmatched = false; + let mut ask_reason: Option = None; + + for path in paths { + match permission_override_for_path(engine, tool_name, &path, workspace) { + ToolPermissionOverride::Deny { reason } => { + return ToolPermissionOverride::Deny { reason }; + } + ToolPermissionOverride::Ask { reason } => { + ask_reason.get_or_insert(reason); + } + ToolPermissionOverride::Allow { .. } => { + saw_allow = true; + } + ToolPermissionOverride::Unmatched => { + saw_unmatched = true; + } + } + } + + if let Some(reason) = ask_reason { + ToolPermissionOverride::Ask { reason } + } else if saw_allow && !saw_unmatched { + ToolPermissionOverride::Allow { + reason: format!("Tool '{tool_name}' allowed by permission rule"), + } + } else { + ToolPermissionOverride::Unmatched + } +} + +fn permission_override_for_parallel_call( + engine: &ExecPolicyEngine, + input: &Value, + workspace: &Path, +) -> ToolPermissionOverride { + let Ok(calls) = parse_parallel_tool_calls(input) else { + return ToolPermissionOverride::Unmatched; + }; + + let mut saw_allow = false; + let mut saw_unmatched = false; + let mut ask_reason: Option = None; + + for (nested_tool_name, nested_input) in calls { + match tool_permission_override_for_call(engine, &nested_tool_name, &nested_input, workspace) + { + ToolPermissionOverride::Deny { reason } => { + return ToolPermissionOverride::Deny { reason }; + } + ToolPermissionOverride::Ask { reason } => { + ask_reason.get_or_insert(reason); + } + ToolPermissionOverride::Allow { .. } => { + saw_allow = true; + } + ToolPermissionOverride::Unmatched => { + saw_unmatched = true; + } + } + } + + if let Some(reason) = ask_reason { + ToolPermissionOverride::Ask { reason } + } else if saw_allow && !saw_unmatched { + ToolPermissionOverride::Allow { + reason: format!("Tool '{MULTI_TOOL_PARALLEL_NAME}' allowed by permission rule"), + } + } else { + ToolPermissionOverride::Unmatched + } +} + +fn permission_override_for_path( + engine: &ExecPolicyEngine, + tool_name: &str, + path: &str, + workspace: &Path, +) -> ToolPermissionOverride { + let mut override_result = ToolPermissionOverride::Unmatched; + for candidate in permission_path_candidates(path, workspace) { + let next = permission_override_for_context(engine, tool_name, None, Some(&candidate)); + override_result = combine_permission_overrides(override_result, next); + if matches!(override_result, ToolPermissionOverride::Deny { .. }) { + break; + } + } + override_result +} + +fn permission_override_for_context( + engine: &ExecPolicyEngine, + tool_name: &str, + command: Option<&str>, + path: Option<&str>, +) -> ToolPermissionOverride { + let mut override_result = ToolPermissionOverride::Unmatched; + for policy_tool_name in + std::iter::once(tool_name).chain(permission_policy_tool_aliases(tool_name).iter().copied()) + { + let next = permission_override_from_check( + tool_name, + engine.check_tool_permission(ToolPermissionContext { + tool: policy_tool_name, + command, + path, + }), + ); + override_result = combine_permission_overrides(override_result, next); + if matches!(override_result, ToolPermissionOverride::Deny { .. }) { + break; + } + } + override_result +} + +fn combine_permission_overrides( + current: ToolPermissionOverride, + next: ToolPermissionOverride, +) -> ToolPermissionOverride { + match (current, next) { + (ToolPermissionOverride::Deny { reason }, _) => ToolPermissionOverride::Deny { reason }, + (_, ToolPermissionOverride::Deny { reason }) => ToolPermissionOverride::Deny { reason }, + (ToolPermissionOverride::Ask { reason }, _) => ToolPermissionOverride::Ask { reason }, + (_, ToolPermissionOverride::Ask { reason }) => ToolPermissionOverride::Ask { reason }, + (ToolPermissionOverride::Allow { reason }, _) => ToolPermissionOverride::Allow { reason }, + (_, ToolPermissionOverride::Allow { reason }) => ToolPermissionOverride::Allow { reason }, + (ToolPermissionOverride::Unmatched, ToolPermissionOverride::Unmatched) => { + ToolPermissionOverride::Unmatched + } + } +} + +fn permission_override_from_check( + tool_name: &str, + check: ToolPermissionCheck, +) -> ToolPermissionOverride { + let Some(decision) = check.decision else { + return ToolPermissionOverride::Unmatched; + }; + let label = check + .matched_rule + .as_ref() + .map(|matched| matched.rule.pattern_label()) + .unwrap_or_else(|| format!("tool '{tool_name}'")); + match decision { + PermissionDecision::Allow => ToolPermissionOverride::Allow { + reason: format!("Tool '{tool_name}' allowed by permission rule ({label})"), + }, + PermissionDecision::Ask => ToolPermissionOverride::Ask { + reason: format!("Approval required by permission rule ({label})"), + }, + PermissionDecision::Deny => ToolPermissionOverride::Deny { + reason: format!("Tool '{tool_name}' denied by permission rule ({label})"), + }, + } +} + +fn is_shell_permission_tool(tool_name: &str) -> bool { + matches!( + tool_name, + "exec_shell" + | "exec_shell_wait" + | "exec_shell_interact" + | "exec_shell_cancel" + | "exec_wait" + | "exec_interact" + ) +} + +fn is_file_permission_tool(tool_name: &str) -> bool { + matches!( + tool_name, + "read_file" | "write_file" | "edit_file" | "list_dir" | "apply_patch" + ) +} + +fn permission_policy_tool_aliases(tool_name: &str) -> &'static [&'static str] { + match tool_name { + "read_file" => &["file_read"], + "write_file" => &["file_write"], + "edit_file" => &["file_edit"], + _ => &[], + } +} + +fn permission_paths_for_file_tool(tool_name: &str, input: &Value) -> Vec { + match tool_name { + "read_file" | "write_file" | "edit_file" => input + .get("path") + .and_then(Value::as_str) + .map(|path| vec![path.to_string()]) + .unwrap_or_default(), + "list_dir" => vec![ + input + .get("path") + .and_then(Value::as_str) + .unwrap_or(".") + .to_string(), + ], + "apply_patch" => apply_patch_permission_paths(input), + _ => Vec::new(), + } +} + +fn apply_patch_permission_paths(input: &Value) -> Vec { + let mut paths = Vec::new(); + if let Some(path) = input.get("path").and_then(Value::as_str) { + push_unique_path(&mut paths, path); + } + for key in ["changes", "files"] { + if let Some(entries) = input.get(key).and_then(Value::as_array) { + for entry in entries { + if let Some(path) = entry.get("path").and_then(Value::as_str) { + push_unique_path(&mut paths, path); + } + } + } + } + if paths.is_empty() { + if let Some(patch) = input.get("patch").and_then(Value::as_str) { + for path in parse_unified_diff_permission_paths(patch) { + push_unique_path(&mut paths, &path); + } + } + } + paths +} + +fn parse_unified_diff_permission_paths(patch: &str) -> Vec { + let mut paths = Vec::new(); + let mut old_path: Option = None; + + for line in patch.lines() { + if let Some(stripped) = line.strip_prefix("--- ") { + old_path = normalize_diff_permission_path(stripped); + continue; + } + if let Some(stripped) = line.strip_prefix("+++ ") { + let new_path = normalize_diff_permission_path(stripped); + if let Some(path) = new_path.or_else(|| old_path.clone()) { + push_unique_path(&mut paths, &path); + } + old_path = None; + } + } + + paths +} + +fn normalize_diff_permission_path(raw: &str) -> Option { + let raw = raw.trim(); + if raw.is_empty() || raw == "/dev/null" || raw == "dev/null" { + return None; + } + let raw = raw + .strip_prefix("a/") + .or_else(|| raw.strip_prefix("b/")) + .unwrap_or(raw); + Some(raw.to_string()) +} + +fn push_unique_path(paths: &mut Vec, path: &str) { + if !paths.iter().any(|existing| existing == path) { + paths.push(path.to_string()); + } +} + +fn permission_path_candidates(path: &str, workspace: &Path) -> Vec { + let mut candidates = Vec::new(); + push_unique_path(&mut candidates, path); + + let workspace_bases = permission_workspace_bases(workspace); + let workspace_base = workspace_bases + .first() + .cloned() + .unwrap_or_else(|| normalize_permission_path(workspace)); + let raw_path = Path::new(path); + let candidate_path = if raw_path.is_absolute() { + raw_path.to_path_buf() + } else { + workspace_base.join(raw_path) + }; + let candidate_path = normalize_permission_path(&candidate_path); + + for workspace_base in &workspace_bases { + if let Some(relative) = workspace_relative_permission_path(&candidate_path, workspace_base) + { + push_unique_path(&mut candidates, &relative); + } + } + + if let Some(canonical_path) = canonicalize_permission_path(&candidate_path) { + for workspace_base in &workspace_bases { + if let Some(relative) = + workspace_relative_permission_path(&canonical_path, workspace_base) + { + push_unique_path(&mut candidates, &relative); + } + } + } + + candidates +} + +fn permission_workspace_bases(workspace: &Path) -> Vec { + let mut bases = Vec::new(); + let workspace = if workspace.is_absolute() { + workspace.to_path_buf() + } else { + std::env::current_dir() + .map(|current_dir| current_dir.join(workspace)) + .unwrap_or_else(|_| workspace.to_path_buf()) + }; + push_unique_path_buf(&mut bases, normalize_permission_path(&workspace)); + if let Ok(canonical) = workspace.canonicalize() { + push_unique_path_buf(&mut bases, normalize_permission_path(&canonical)); + } + bases +} + +fn workspace_relative_permission_path(path: &Path, workspace: &Path) -> Option { + let normalized_path = normalize_permission_path(path); + normalized_path + .strip_prefix(workspace) + .ok() + .map(permission_path_to_string) +} + +fn canonicalize_permission_path(path: &Path) -> Option { + if path.exists() { + return path + .canonicalize() + .ok() + .map(|path| normalize_permission_path(&path)); + } + + let mut existing_ancestor = path.to_path_buf(); + let mut suffix_parts: Vec = Vec::new(); + + while !existing_ancestor.exists() { + if let Some(file_name) = existing_ancestor.file_name() { + suffix_parts.push(file_name.to_owned()); + } + match existing_ancestor.parent() { + Some(parent) if !parent.as_os_str().is_empty() => { + existing_ancestor = parent.to_path_buf(); + } + _ => return None, + } + } + + let mut canonical = existing_ancestor + .canonicalize() + .unwrap_or(existing_ancestor); + for part in suffix_parts.into_iter().rev() { + canonical.push(part); + } + Some(normalize_permission_path(&canonical)) +} + +fn normalize_permission_path(path: &Path) -> PathBuf { + let mut prefix: Option = None; + let mut is_root = false; + let mut stack: Vec = Vec::new(); + + for component in path.components() { + match component { + Component::Prefix(prefix_component) => { + prefix = Some(prefix_component.as_os_str().to_owned()); + } + Component::RootDir => { + is_root = true; + } + Component::CurDir => {} + Component::ParentDir => { + let parent = Component::ParentDir.as_os_str(); + if let Some(last) = stack.pop() { + if last == parent { + stack.push(last); + stack.push(parent.to_owned()); + } + } else if !is_root { + stack.push(parent.to_owned()); + } + } + Component::Normal(part) => { + stack.push(part.to_owned()); + } + } + } + + let mut normalized = PathBuf::new(); + if let Some(prefix) = prefix { + normalized.push(prefix); + } + if is_root { + normalized.push(Path::new(std::path::MAIN_SEPARATOR_STR)); + } + for part in stack { + normalized.push(part); + } + normalized +} + +fn permission_path_to_string(path: &Path) -> String { + if path.as_os_str().is_empty() { + ".".to_string() + } else { + path.to_string_lossy().replace('\\', "/") + } +} + +fn push_unique_path_buf(paths: &mut Vec, path: PathBuf) { + if !paths.iter().any(|existing| existing == &path) { + paths.push(path); + } +} + pub(super) fn format_tool_error(err: &ToolError, tool_name: &str) -> String { match err { ToolError::InvalidInput { message } => { diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 851b09ea0..2093c8e8e 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -152,6 +152,210 @@ fn make_plan_at( } } +fn permission_engine_with_rules( + rules: Vec, +) -> codewhale_execpolicy::ExecPolicyEngine { + codewhale_execpolicy::ExecPolicyEngine::with_rulesets(vec![ + codewhale_execpolicy::Ruleset::user(vec![], vec![]).with_rules(rules), + ]) +} + +fn permission_test_workspace() -> &'static Path { + Path::new(".") +} + +#[test] +fn typed_permission_allows_shell_command_before_approval_prompt() { + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::exec_shell( + codewhale_execpolicy::PermissionDecision::Allow, + "cargo test", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "exec_shell", + &json!({"command": "cargo test --workspace"}), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Allow { .. })); +} + +#[test] +fn typed_permission_can_force_read_file_approval() { + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "read_file", + codewhale_execpolicy::PermissionDecision::Ask, + "secrets/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "read_file", + &json!({"path": "secrets/a.env"}), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Ask { .. })); +} + +#[test] +fn typed_permission_matches_absolute_workspace_file_paths() { + let workspace = tempdir().expect("workspace"); + let absolute_path = workspace.path().join("secrets/a.env"); + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "read_file", + codewhale_execpolicy::PermissionDecision::Deny, + "secrets/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "read_file", + &json!({"path": absolute_path.to_string_lossy()}), + workspace.path(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Deny { .. })); +} + +#[cfg(unix)] +#[test] +fn typed_permission_matches_symlink_resolved_workspace_file_paths() { + use std::os::unix::fs::symlink; + + let workspace = tempdir().expect("workspace"); + let secrets_dir = workspace.path().join("secrets"); + fs::create_dir(&secrets_dir).expect("create secrets dir"); + fs::write(secrets_dir.join("a.env"), "token").expect("write secret"); + symlink(&secrets_dir, workspace.path().join("alias")).expect("symlink secrets"); + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "read_file", + codewhale_execpolicy::PermissionDecision::Deny, + "secrets/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "read_file", + &json!({"path": "alias/a.env"}), + workspace.path(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Deny { .. })); +} + +#[test] +fn typed_permission_checks_nested_parallel_tool_calls() { + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "read_file", + codewhale_execpolicy::PermissionDecision::Deny, + "secrets/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "multi_tool_use.parallel", + &json!({ + "tool_uses": [{ + "recipient_name": "read_file", + "parameters": { "path": "secrets/a.env" } + }] + }), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Deny { .. })); +} + +#[test] +fn typed_permission_matches_legacy_file_tool_aliases() { + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "file_edit", + codewhale_execpolicy::PermissionDecision::Ask, + "src/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "edit_file", + &json!({"path": "src/main.rs"}), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Ask { .. })); +} + +#[test] +fn typed_permission_denies_file_path_before_tool_execution() { + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "write_file", + codewhale_execpolicy::PermissionDecision::Deny, + "src/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "write_file", + &json!({"path": "src/main.rs", "content": "fn main() {}"}), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Deny { .. })); +} + +#[test] +fn typed_permission_requires_all_apply_patch_paths_to_be_allowed() { + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "apply_patch", + codewhale_execpolicy::PermissionDecision::Allow, + "docs/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "apply_patch", + &json!({ + "changes": [ + { "path": "docs/a.md", "content": "ok" }, + { "path": "src/lib.rs", "content": "needs existing approval" } + ] + }), + permission_test_workspace(), + ); + + assert_eq!(decision, ToolPermissionOverride::Unmatched); +} + +#[test] +fn typed_permission_allows_apply_patch_when_all_paths_match() { + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "apply_patch", + codewhale_execpolicy::PermissionDecision::Allow, + "docs/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "apply_patch", + &json!({ + "patch": "--- a/docs/a.md\n+++ b/docs/a.md\n@@ -1 +1 @@\n-old\n+new\n" + }), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Allow { .. })); +} + fn api_tool(name: &str) -> Tool { Tool { tool_type: Some("function".to_string()), diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 9f2da5ffd..7fa05c39c 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1239,6 +1239,7 @@ impl Engine { if blocked_error.is_none() && tool_def.is_none() && !McpPool::is_mcp_tool(&tool_name) + && tool_name != MULTI_TOOL_PARALLEL_NAME && tool_name != CODE_EXECUTION_TOOL_NAME && tool_name != JS_EXECUTION_TOOL_NAME && !is_tool_search_tool(&tool_name) @@ -1261,6 +1262,12 @@ impl Engine { approval_description = spec.description().to_string(); supports_parallel = spec.supports_parallel(); read_only = spec.is_read_only(); + } else if tool_name == MULTI_TOOL_PARALLEL_NAME { + approval_required = false; + approval_description = + "Execute multiple read-only tools in parallel".to_string(); + supports_parallel = false; + read_only = true; } else if tool_name == CODE_EXECUTION_TOOL_NAME { approval_required = true; approval_description = @@ -1283,26 +1290,52 @@ impl Engine { let should_emit_hydration_status = !deferred_tools_hydrated_this_batch.contains(&tool_name); - if blocked_error.is_none() - && let Some(result) = maybe_hydrate_requested_deferred_tool( + + if blocked_error.is_none() { + match tool_permission_override_for_call( + &self.config.exec_policy_engine, + &tool_name, + &tool_input, + &self.session.workspace, + ) { + ToolPermissionOverride::Allow { reason } => { + approval_required = false; + approval_description = reason; + } + ToolPermissionOverride::Ask { reason } => { + approval_required = true; + approval_description = reason; + } + ToolPermissionOverride::Deny { reason } => { + approval_required = false; + blocked_error = Some(ToolError::permission_denied(reason)); + } + ToolPermissionOverride::Unmatched => {} + } + } + + if blocked_error.is_none() { + if let Some(result) = maybe_hydrate_requested_deferred_tool( &tool_name, &tool_input, &tool_catalog, &active_tools_at_batch_start, &mut deferred_tools_hydrated_this_batch, - ) - { - if should_emit_hydration_status { - let status = if requested_tool_name == tool_name { - format!("Auto-loaded deferred tool '{tool_name}' after model request.") - } else { - format!( - "Auto-loaded deferred tool '{tool_name}' after resolving '{requested_tool_name}'." - ) - }; - let _ = self.tx_event.send(Event::status(status)).await; + ) { + if should_emit_hydration_status { + let status = if requested_tool_name == tool_name { + format!( + "Auto-loaded deferred tool '{tool_name}' after model request." + ) + } else { + format!( + "Auto-loaded deferred tool '{tool_name}' after resolving '{requested_tool_name}'." + ) + }; + let _ = self.tx_event.send(Event::status(status)).await; + } + guard_result = Some(result); } - guard_result = Some(result); } if blocked_error.is_none() diff --git a/crates/tui/src/main.rs b/crates/tui/src/main.rs index 34cb7fce7..77d6aa12a 100644 --- a/crates/tui/src/main.rs +++ b/crates/tui/src/main.rs @@ -8,6 +8,7 @@ use std::time::Duration; use anyhow::{Context, Result, anyhow, bail}; use clap::{Args, CommandFactory, Parser, Subcommand, ValueEnum}; use clap_complete::{Shell, generate}; +use codewhale_execpolicy::PermissionDecision; use dotenvy::dotenv; use tempfile::NamedTempFile; use wait_timeout::ChildExt; @@ -4696,6 +4697,11 @@ fn merge_project_config(config: &mut Config, workspace: &Path) { config.allow_shell = Some(v); } + // Project-level permission config is allowed only when it tightens the + // effective policy. A repository can require extra prompts or denials, but + // it cannot grant itself broader tool access. + merge_project_permission_config(config, table); + // #454: instructions array — project replaces user. Empty arrays // count: explicit `instructions = []` clears the user's list for // this repo, useful when the user has a verbose global file that @@ -4711,6 +4717,72 @@ fn merge_project_config(config: &mut Config, workspace: &Path) { } } +fn merge_project_permission_config( + config: &mut Config, + table: &toml::map::Map, +) { + if let Some(value) = table.get("auto_allow") { + let ignored = value.as_array().map_or(0, Vec::len); + if ignored > 0 { + eprintln!( + "warning: project-scope `auto_allow` is ignored — \ + project config cannot grant persistent command allow rules." + ); + } + } + + if let Some(value) = table.get("auto_deny") { + match string_array_from_project_value(value) { + Some(entries) => config.auto_deny.extend(entries), + None => eprintln!( + "warning: project-scope `auto_deny` is ignored — expected an array of strings." + ), + } + } + + let Some(value) = table.get("permissions") else { + return; + }; + let permissions: crate::config::PermissionsConfig = match value.clone().try_into() { + Ok(permissions) => permissions, + Err(err) => { + eprintln!( + "warning: project-scope `[permissions]` is ignored — failed to parse permission rules: {err}" + ); + return; + } + }; + + for rule in permissions.rules { + match rule.decision { + PermissionDecision::Deny | PermissionDecision::Ask => { + config.permissions.rules.push(rule); + } + PermissionDecision::Allow => { + eprintln!( + "warning: project-scope allow permission rule `{}` is ignored — \ + project config cannot grant persistent tool allow rules.", + rule.pattern_label() + ); + } + } + } +} + +fn string_array_from_project_value(value: &toml::Value) -> Option> { + Some( + value + .as_array()? + .iter() + .map(toml::Value::as_str) + .collect::>>()? + .into_iter() + .filter(|item| !item.trim().is_empty()) + .map(str::to_string) + .collect(), + ) +} + async fn run_interactive( cli: &Cli, config: &Config, @@ -5136,6 +5208,7 @@ async fn run_exec_agent( .and_then(|s| s.provider) .unwrap_or_default(), search_api_key: config.search.as_ref().and_then(|s| s.api_key.clone()), + exec_policy_engine: config.exec_policy_engine(), }; let engine_handle = spawn_engine(engine_config, config); @@ -6303,6 +6376,48 @@ allow_shell = false assert_eq!(config.allow_shell, Some(false)); } + #[test] + fn project_overlay_applies_only_tightening_permission_rules() { + let tmp = workspace_with_project_config( + r#" +auto_allow = ["cargo test"] +auto_deny = ["rm -rf"] + +[[permissions.rules]] +tool = "read_file" +decision = "ask" +path = "secrets/**" + +[[permissions.rules]] +tool = "write_file" +decision = "deny" +path = "src/**" + +[[permissions.rules]] +tool = "exec_shell" +decision = "allow" +command = "cargo test" +"#, + ); + let mut config = Config::default(); + merge_project_config(&mut config, tmp.path()); + + assert!( + config.auto_allow.is_empty(), + "project-scope auto_allow must not grant access" + ); + assert_eq!(config.auto_deny, ["rm -rf"]); + assert_eq!(config.permissions.rules.len(), 2); + assert_eq!( + config.permissions.rules[0].decision, + PermissionDecision::Ask + ); + assert_eq!( + config.permissions.rules[1].decision, + PermissionDecision::Deny + ); + } + #[test] fn project_overlay_clamps_max_subagents_to_safe_range() { let tmp = workspace_with_project_config( diff --git a/crates/tui/src/runtime_threads.rs b/crates/tui/src/runtime_threads.rs index 787142ba4..824a198a3 100644 --- a/crates/tui/src/runtime_threads.rs +++ b/crates/tui/src/runtime_threads.rs @@ -1985,6 +1985,7 @@ impl RuntimeThreadManager { .and_then(|s| s.provider) .unwrap_or_default(), search_api_key: self.config.search.as_ref().and_then(|s| s.api_key.clone()), + exec_policy_engine: self.config.exec_policy_engine(), }; let engine = spawn_engine(engine_cfg, &self.config); diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 1444f1341..672369427 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -723,6 +723,7 @@ fn build_engine_config(app: &App, config: &Config) -> EngineConfig { .and_then(|s| s.provider) .unwrap_or_default(), search_api_key: config.search.as_ref().and_then(|s| s.api_key.clone()), + exec_policy_engine: config.exec_policy_engine(), } } From b0da4d670977820625c36905c987e951621d0b8d Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Tue, 12 May 2026 16:20:12 +0800 Subject: [PATCH 12/19] feat(tui): persist permission rules from approvals --- crates/execpolicy/src/lib.rs | 65 ++- crates/tui/src/commands/config.rs | 125 +++++ crates/tui/src/commands/mod.rs | 7 + crates/tui/src/core/engine.rs | 2 +- crates/tui/src/core/engine/approval.rs | 12 +- crates/tui/src/core/engine/handle.rs | 15 +- crates/tui/src/core/engine/tests.rs | 30 ++ crates/tui/src/tui/approval.rs | 633 ++++++++++++++++++++++++- crates/tui/src/tui/ui.rs | 42 +- crates/tui/src/tui/views/mod.rs | 2 + crates/tui/src/tui/widgets/mod.rs | 104 ++++ 11 files changed, 1024 insertions(+), 13 deletions(-) diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index c60a9e5b9..fb383ff26 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -9,8 +9,8 @@ use globset::GlobBuilder; use serde::{Deserialize, Serialize}; /// Priority layer for a permission ruleset. Higher ordinal = higher priority. -/// Deny rules still win across layers; for non-deny ties the higher-priority -/// layer wins before pattern specificity is considered. +/// Deny rules still win across layers. For non-deny matches, higher-priority +/// layers win first, then narrower patterns, then ask/allow precedence. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum RulesetLayer { @@ -510,14 +510,23 @@ fn compare_rule_priority( current: &LayeredToolPermissionRule, current_specificity: usize, ) -> bool { + match (candidate.rule.decision, current.rule.decision) { + (PermissionDecision::Deny, PermissionDecision::Deny) => { + return (candidate.layer, candidate_specificity) > (current.layer, current_specificity); + } + (PermissionDecision::Deny, _) => return true, + (_, PermissionDecision::Deny) => return false, + _ => {} + } + ( - candidate.rule.decision.precedence(), candidate.layer, candidate_specificity, + candidate.rule.decision.precedence(), ) > ( - current.rule.decision.precedence(), current.layer, current_specificity, + current.rule.decision.precedence(), ) } @@ -879,6 +888,25 @@ mod tests { ); } + #[test] + fn more_specific_allow_rule_wins_over_broad_ask_rule() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_rules(vec![ + ToolPermissionRule::exec_shell(PermissionDecision::Ask, "cargo test"), + ToolPermissionRule::exec_shell(PermissionDecision::Allow, "cargo test --workspace"), + ])]); + + let decision = engine.check(exec_ctx("cargo test --workspace")).unwrap(); + + assert!(decision.allow); + assert!(!decision.requires_approval); + assert_eq!(decision.requirement.phase(), "allowed"); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool 'exec_shell', command 'cargo test --workspace'") + ); + } + #[test] fn ask_rule_overrides_on_failure_policy() { let engine = @@ -916,6 +944,35 @@ mod tests { assert_eq!(decision.decision, Some(PermissionDecision::Allow)); } + #[test] + fn exact_path_allow_rule_wins_over_broad_path_ask_rule() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_rules(vec![ + ToolPermissionRule::file_path("file_edit", PermissionDecision::Ask, "src/**"), + ToolPermissionRule::file_path( + "file_edit", + PermissionDecision::Allow, + "src/main.rs", + ), + ])]); + + let decision = engine.check_tool_permission(ToolPermissionContext { + tool: "file_edit", + command: None, + path: Some("src/main.rs"), + }); + + assert_eq!(decision.decision, Some(PermissionDecision::Allow)); + let label = decision + .matched_rule + .as_ref() + .map(|matched| matched.rule.pattern_label()); + assert_eq!( + label.as_deref(), + Some("tool 'file_edit', path 'src/main.rs'") + ); + } + #[test] fn path_star_does_not_cross_directory_separator() { assert!(path_pattern_matches("docs/*.md", "docs/readme.md")); diff --git a/crates/tui/src/commands/config.rs b/crates/tui/src/commands/config.rs index 40ffe1dcf..b440c6bf9 100644 --- a/crates/tui/src/commands/config.rs +++ b/crates/tui/src/commands/config.rs @@ -348,6 +348,82 @@ pub fn persist_root_string_key(key: &str, value: &str) -> anyhow::Result anyhow::Result { + use anyhow::Context; + use std::fs; + + let path = config_toml_path()?; + if let Some(parent) = path.parent() { + fs::create_dir_all(parent) + .with_context(|| format!("failed to create config directory {}", parent.display()))?; + } + + let mut doc: toml::Value = if path.exists() { + let raw = fs::read_to_string(&path) + .with_context(|| format!("failed to read config at {}", path.display()))?; + toml::from_str(&raw) + .with_context(|| format!("failed to parse config at {}", path.display()))? + } else { + toml::Value::Table(toml::value::Table::new()) + }; + let table = doc + .as_table_mut() + .context("config.toml root must be a table")?; + let permissions_entry = table + .entry("permissions".to_string()) + .or_insert_with(|| toml::Value::Table(toml::value::Table::new())); + let permissions_table = permissions_entry + .as_table_mut() + .context("`permissions` section in config.toml must be a table")?; + let rules_entry = permissions_table + .entry("rules".to_string()) + .or_insert_with(|| toml::Value::Array(Vec::new())); + let rules_array = rules_entry + .as_array_mut() + .context("`permissions.rules` in config.toml must be an array")?; + + for rule in rules { + let value = permission_rule_to_toml_value(rule); + if !rules_array.iter().any(|existing| existing == &value) { + rules_array.push(value); + } + } + + let body = toml::to_string_pretty(&doc).context("failed to serialize config.toml")?; + fs::write(&path, body) + .with_context(|| format!("failed to write config at {}", path.display()))?; + Ok(path) +} + +fn permission_rule_to_toml_value(rule: &deepseek_execpolicy::ToolPermissionRule) -> toml::Value { + let mut table = toml::value::Table::new(); + table.insert("tool".to_string(), toml::Value::String(rule.tool.clone())); + table.insert( + "decision".to_string(), + toml::Value::String(permission_decision_label(rule.decision).to_string()), + ); + if let Some(command) = rule.command.as_deref() { + table.insert( + "command".to_string(), + toml::Value::String(command.to_string()), + ); + } + if let Some(path) = rule.path.as_deref() { + table.insert("path".to_string(), toml::Value::String(path.to_string())); + } + toml::Value::Table(table) +} + +fn permission_decision_label(decision: deepseek_execpolicy::PermissionDecision) -> &'static str { + match decision { + deepseek_execpolicy::PermissionDecision::Allow => "allow", + deepseek_execpolicy::PermissionDecision::Deny => "deny", + deepseek_execpolicy::PermissionDecision::Ask => "ask", + } +} + /// Resolve the path to `~/.deepseek/config.toml` (or /// `$DEEPSEEK_CONFIG_PATH`). Mirrors what `Config::load` accepts so we /// never write to a different file than the one we read. @@ -1996,4 +2072,53 @@ mod tests { "expected status_items in {body}" ); } + + #[test] + fn persist_permission_rules_writes_deduped_rules_and_preserves_config() { + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let temp_root = env::temp_dir().join(format!( + "deepseek-permission-rules-{}-{}", + std::process::id(), + nanos + )); + fs::create_dir_all(&temp_root).unwrap(); + let _guard = EnvGuard::new(&temp_root); + + let path = temp_root.join(".deepseek").join("config.toml"); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write( + &path, + "api_key = \"sentinel-key\"\nmodel = \"deepseek-v4-pro\"\n", + ) + .unwrap(); + + let cargo_rule = deepseek_execpolicy::ToolPermissionRule::exec_shell( + deepseek_execpolicy::PermissionDecision::Allow, + "cargo test", + ); + let docs_rule = deepseek_execpolicy::ToolPermissionRule::file_path( + "read_file", + deepseek_execpolicy::PermissionDecision::Allow, + "docs/README.md", + ); + let written = + persist_permission_rules(&[cargo_rule.clone(), cargo_rule.clone(), docs_rule.clone()]) + .expect("persist should succeed"); + + let body = fs::read_to_string(&written).expect("written file should be readable"); + assert!( + body.contains("api_key = \"sentinel-key\""), + "round-trip lost api_key: {body}" + ); + assert!( + body.contains("model = \"deepseek-v4-pro\""), + "round-trip lost model: {body}" + ); + + let config = Config::load(Some(written), None).expect("written config should load"); + assert_eq!(config.permissions.rules, vec![cargo_rule, docs_rule]); + } } diff --git a/crates/tui/src/commands/mod.rs b/crates/tui/src/commands/mod.rs index b1e9f3dd4..5aab04811 100644 --- a/crates/tui/src/commands/mod.rs +++ b/crates/tui/src/commands/mod.rs @@ -706,6 +706,13 @@ pub fn persist_root_string_key(key: &str, value: &str) -> anyhow::Result anyhow::Result { + config::persist_permission_rules(rules) +} + pub fn switch_mode(app: &mut App, mode: crate::tui::app::AppMode) -> String { config::switch_mode(app, mode) } diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 3379d8403..94d2141cf 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -1921,7 +1921,7 @@ pub(crate) enum MockApprovalEvent { impl MockEngineHandle { pub(crate) async fn recv_approval_event(&mut self) -> Option { match self.rx_approval.recv().await? { - ApprovalDecision::Approved { id } => Some(MockApprovalEvent::Approved { id }), + ApprovalDecision::Approved { id, .. } => Some(MockApprovalEvent::Approved { id }), ApprovalDecision::Denied { id } => Some(MockApprovalEvent::Denied { id }), ApprovalDecision::RetryWithPolicy { id, policy } => { Some(MockApprovalEvent::RetryWithPolicy { id, policy }) diff --git a/crates/tui/src/core/engine/approval.rs b/crates/tui/src/core/engine/approval.rs index ac04900b4..af9285dcf 100644 --- a/crates/tui/src/core/engine/approval.rs +++ b/crates/tui/src/core/engine/approval.rs @@ -15,6 +15,7 @@ use super::Engine; pub(super) enum ApprovalDecision { Approved { id: String, + persistent_rules: Vec, }, Denied { id: String, @@ -86,7 +87,16 @@ impl Engine { )); }; match decision { - ApprovalDecision::Approved { id } if id == tool_id => { + ApprovalDecision::Approved { + id, + persistent_rules, + } if id == tool_id => { + if !persistent_rules.is_empty() { + self.config.exec_policy_engine.add_ruleset( + deepseek_execpolicy::Ruleset::user(vec![], vec![]) + .with_rules(persistent_rules), + ); + } return Ok(ApprovalResult::Approved); } ApprovalDecision::Denied { id } if id == tool_id => { diff --git a/crates/tui/src/core/engine/handle.rs b/crates/tui/src/core/engine/handle.rs index 676f1986f..5dbecd719 100644 --- a/crates/tui/src/core/engine/handle.rs +++ b/crates/tui/src/core/engine/handle.rs @@ -52,8 +52,21 @@ impl EngineHandle { /// Approve a pending tool call pub async fn approve_tool_call(&self, id: impl Into) -> Result<()> { + self.approve_tool_call_with_rules(id, Vec::new()).await + } + + /// Approve a pending tool call and add persistent permission rules to the + /// live engine policy for the rest of this session. + pub async fn approve_tool_call_with_rules( + &self, + id: impl Into, + persistent_rules: Vec, + ) -> Result<()> { self.tx_approval - .send(ApprovalDecision::Approved { id: id.into() }) + .send(ApprovalDecision::Approved { + id: id.into(), + persistent_rules, + }) .await?; Ok(()) } diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 2093c8e8e..f249bdd91 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -292,6 +292,36 @@ fn typed_permission_matches_legacy_file_tool_aliases() { assert!(matches!(decision, ToolPermissionOverride::Ask { .. })); } +#[test] +fn typed_permission_exact_saved_alias_allow_beats_broad_alias_ask() { + let engine = permission_engine_with_rules(vec![ + deepseek_execpolicy::ToolPermissionRule::file_path( + "file_edit", + deepseek_execpolicy::PermissionDecision::Ask, + "src/**", + ), + deepseek_execpolicy::ToolPermissionRule::file_path( + "edit_file", + deepseek_execpolicy::PermissionDecision::Allow, + "src/main.rs", + ), + deepseek_execpolicy::ToolPermissionRule::file_path( + "file_edit", + deepseek_execpolicy::PermissionDecision::Allow, + "src/main.rs", + ), + ]); + + let decision = tool_permission_override_for_call( + &engine, + "edit_file", + &json!({"path": "src/main.rs"}), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Allow { .. })); +} + #[test] fn typed_permission_denies_file_path_before_tool_execution() { let engine = diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index dc7e9cdcd..ebba59551 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -21,9 +21,9 @@ //! accidentally land on an approval. Any non-approve key clears the //! staging and keeps the user in selection mode. //! -//! The decision events emitted upstream are unchanged -//! (`ViewEvent::ApprovalDecision`), so `ui.rs` and the engine handle -//! both variants without modification. Auto-approve / YOLO bypasses +//! The decision events emitted upstream are still +//! `ViewEvent::ApprovalDecision`, with optional persistent permission +//! rules for the "save this rule" action. Auto-approve / YOLO bypasses //! happen *before* the view is constructed (see `tui/ui.rs`); this //! module always assumes the user is being asked. @@ -32,8 +32,9 @@ use crate::sandbox::SandboxPolicy; use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent}; use crate::tui::widgets::{ApprovalWidget, ElevationWidget, Renderable}; use crossterm::event::{KeyCode, KeyEvent}; +use deepseek_execpolicy::{PermissionDecision, ToolPermissionRule}; use serde_json::Value; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use std::time::{Duration, Instant}; /// Determines when tool executions require user approval @@ -135,15 +136,47 @@ pub struct ApprovalRequest { /// Lossy / arity-aware fingerprint, used to scope *approvals* so an /// "approve for session" covers later flag variants (v0.8.37). pub approval_grouping_key: String, + /// Generated persistent allow rules for the "save this rule" action. + pub persistent_rules: Vec, } impl ApprovalRequest { + #[cfg(test)] pub fn new( id: &str, tool_name: &str, description: &str, params: &Value, approval_key: &str, + ) -> Self { + Self::new_inner(id, tool_name, description, params, approval_key, None) + } + + pub fn new_with_workspace( + id: &str, + tool_name: &str, + description: &str, + params: &Value, + approval_key: &str, + workspace: &Path, + ) -> Self { + Self::new_inner( + id, + tool_name, + description, + params, + approval_key, + Some(workspace), + ) + } + + fn new_inner( + id: &str, + tool_name: &str, + description: &str, + params: &Value, + approval_key: &str, + workspace: Option<&Path>, ) -> Self { let category = get_tool_category(tool_name); let risk = classify_risk(tool_name, category, params); @@ -160,6 +193,7 @@ impl ApprovalRequest { params: params.clone(), approval_key: approval_key.to_string(), approval_grouping_key, + persistent_rules: build_persistent_permission_rules_inner(tool_name, params, workspace), } } @@ -184,6 +218,323 @@ impl ApprovalRequest { _ => self.impacts.clone(), } } + + pub fn permission_rule_preview(&self) -> Option { + if self.persistent_rules.is_empty() { + None + } else { + Some(format_permission_rules_preview(&self.persistent_rules)) + } + } +} + +#[cfg(test)] +pub fn build_persistent_permission_rules( + tool_name: &str, + params: &Value, +) -> Vec { + build_persistent_permission_rules_inner(tool_name, params, None) +} + +#[cfg(test)] +pub fn build_persistent_permission_rules_for_workspace( + tool_name: &str, + params: &Value, + workspace: &Path, +) -> Vec { + build_persistent_permission_rules_inner(tool_name, params, Some(workspace)) +} + +fn build_persistent_permission_rules_inner( + tool_name: &str, + params: &Value, + workspace: Option<&Path>, +) -> Vec { + match tool_name { + "exec_shell" + | "exec_shell_wait" + | "exec_shell_interact" + | "exec_wait" + | "exec_interact" => params + .get("command") + .and_then(Value::as_str) + .map(str::trim) + .filter(|command| !command.is_empty()) + .map(|command| { + let mut rule = ToolPermissionRule::new(tool_name, PermissionDecision::Allow); + rule.command = Some(command.to_string()); + vec![rule] + }) + .unwrap_or_default(), + "read_file" | "write_file" | "edit_file" => params + .get("path") + .and_then(Value::as_str) + .and_then(|path| normalize_persistent_permission_path(path, workspace)) + .map(|path| file_path_permission_rules(tool_name, &path)) + .unwrap_or_default(), + "list_dir" => { + let Some(path) = params + .get("path") + .and_then(Value::as_str) + .map(str::trim) + .filter(|path| !path.is_empty()) + .or(Some(".")) + .and_then(|path| normalize_persistent_permission_path(path, workspace)) + else { + return Vec::new(); + }; + file_path_permission_rules(tool_name, &path) + } + "apply_patch" => apply_patch_permission_paths(params) + .into_iter() + .filter_map(|path| normalize_persistent_permission_path(&path, workspace)) + .flat_map(|path| file_path_permission_rules(tool_name, &path)) + .fold(Vec::new(), |mut rules, rule| { + push_unique_permission_rule(&mut rules, rule); + rules + }), + _ => Vec::new(), + } +} + +pub fn format_permission_rules_preview(rules: &[ToolPermissionRule]) -> String { + rules + .iter() + .map(format_permission_rule_preview) + .collect::>() + .join("\n") +} + +fn format_permission_rule_preview(rule: &ToolPermissionRule) -> String { + let mut lines = vec![ + "[[permissions.rules]]".to_string(), + format!("tool = {}", toml_string(&rule.tool)), + format!( + "decision = {}", + toml_string(permission_decision_label(rule.decision)) + ), + ]; + if let Some(command) = rule.command.as_deref() { + lines.push(format!("command = {}", toml_string(command))); + } + if let Some(path) = rule.path.as_deref() { + lines.push(format!("path = {}", toml_string(path))); + } + lines.join("\n") +} + +fn permission_decision_label(decision: PermissionDecision) -> &'static str { + match decision { + PermissionDecision::Allow => "allow", + PermissionDecision::Deny => "deny", + PermissionDecision::Ask => "ask", + } +} + +fn toml_string(value: &str) -> String { + let mut out = String::with_capacity(value.len() + 2); + out.push('"'); + for ch in value.chars() { + match ch { + '\\' => out.push_str("\\\\"), + '"' => out.push_str("\\\""), + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + _ => out.push(ch), + } + } + out.push('"'); + out +} + +fn file_path_permission_rules(tool_name: &str, path: &str) -> Vec { + persistent_file_rule_tools(tool_name) + .iter() + .map(|tool| ToolPermissionRule::file_path(*tool, PermissionDecision::Allow, path)) + .collect() +} + +fn persistent_file_rule_tools(tool_name: &str) -> Vec<&str> { + match tool_name { + "read_file" => vec!["read_file", "file_read"], + "write_file" => vec!["write_file", "file_write"], + "edit_file" => vec!["edit_file", "file_edit"], + _ => vec![tool_name], + } +} + +fn normalize_persistent_permission_path(raw: &str, workspace: Option<&Path>) -> Option { + let raw = raw.trim(); + if raw.is_empty() { + return None; + } + + let raw_path = Path::new(raw); + let normalized = if let Some(workspace) = workspace { + let workspace = absolute_normalized_workspace(workspace); + let candidate = if raw_path.is_absolute() { + normalize_path(raw_path) + } else { + normalize_path(&workspace.join(raw_path)) + }; + candidate + .strip_prefix(&workspace) + .map(normalize_path) + .unwrap_or_else(|_| { + if raw_path.is_absolute() { + candidate + } else { + normalize_path(raw_path) + } + }) + } else { + normalize_path(raw_path) + }; + + let path = permission_path_to_string(&normalized); + if path_contains_glob_metachar(&path) { + return None; + } + Some(path) +} + +fn absolute_normalized_workspace(workspace: &Path) -> PathBuf { + let workspace = if workspace.is_absolute() { + workspace.to_path_buf() + } else { + std::env::current_dir() + .map(|current_dir| current_dir.join(workspace)) + .unwrap_or_else(|_| workspace.to_path_buf()) + }; + normalize_path(&workspace) +} + +fn normalize_path(path: &Path) -> PathBuf { + let mut prefix: Option = None; + let mut is_root = false; + let mut stack: Vec = Vec::new(); + + for component in path.components() { + match component { + Component::Prefix(prefix_component) => { + prefix = Some(prefix_component.as_os_str().to_owned()); + } + Component::RootDir => { + is_root = true; + } + Component::CurDir => {} + Component::ParentDir => { + let parent = Component::ParentDir.as_os_str(); + if let Some(last) = stack.pop() { + if last == parent { + stack.push(last); + stack.push(parent.to_owned()); + } + } else if !is_root { + stack.push(parent.to_owned()); + } + } + Component::Normal(part) => { + stack.push(part.to_owned()); + } + } + } + + let mut normalized = PathBuf::new(); + if let Some(prefix) = prefix { + normalized.push(prefix); + } + if is_root { + normalized.push(Path::new(std::path::MAIN_SEPARATOR_STR)); + } + for part in stack { + normalized.push(part); + } + normalized +} + +fn permission_path_to_string(path: &Path) -> String { + if path.as_os_str().is_empty() { + ".".to_string() + } else { + path.to_string_lossy().replace('\\', "/") + } +} + +fn path_contains_glob_metachar(path: &str) -> bool { + path.chars() + .any(|ch| matches!(ch, '*' | '?' | '[' | ']' | '{' | '}')) +} + +fn apply_patch_permission_paths(input: &Value) -> Vec { + let mut paths = Vec::new(); + if let Some(path) = input.get("path").and_then(Value::as_str) { + push_unique_permission_path(&mut paths, path); + } + for key in ["changes", "files"] { + if let Some(entries) = input.get(key).and_then(Value::as_array) { + for entry in entries { + if let Some(path) = entry.get("path").and_then(Value::as_str) { + push_unique_permission_path(&mut paths, path); + } + } + } + } + if paths.is_empty() + && let Some(patch) = input.get("patch").and_then(Value::as_str) + { + for path in parse_unified_diff_permission_paths(patch) { + push_unique_permission_path(&mut paths, &path); + } + } + paths +} + +fn parse_unified_diff_permission_paths(patch: &str) -> Vec { + let mut paths = Vec::new(); + let mut old_path: Option = None; + + for line in patch.lines() { + if let Some(stripped) = line.strip_prefix("--- ") { + old_path = normalize_diff_permission_path(stripped); + continue; + } + if let Some(stripped) = line.strip_prefix("+++ ") { + let new_path = normalize_diff_permission_path(stripped); + if let Some(path) = new_path.or_else(|| old_path.clone()) { + push_unique_permission_path(&mut paths, &path); + } + old_path = None; + } + } + + paths +} + +fn normalize_diff_permission_path(raw: &str) -> Option { + let raw = raw.trim(); + if raw.is_empty() || raw == "/dev/null" || raw == "dev/null" { + return None; + } + let raw = raw + .strip_prefix("a/") + .or_else(|| raw.strip_prefix("b/")) + .unwrap_or(raw); + Some(raw.to_string()) +} + +fn push_unique_permission_path(paths: &mut Vec, path: &str) { + let path = path.trim(); + if !path.is_empty() && !paths.iter().any(|existing| existing == path) { + paths.push(path.to_string()); + } +} + +fn push_unique_permission_rule(rules: &mut Vec, rule: ToolPermissionRule) { + if !rules.iter().any(|existing| existing == &rule) { + rules.push(rule); + } } /// Get the category for a tool by name @@ -508,6 +859,7 @@ pub struct ApprovalView { /// is waiting for the user to press the same key (or `Enter`) again. /// Any other key clears the staging. pending_confirm: Option, + pending_persist_rule: bool, timeout: Option, requested_at: Instant, /// Whether the approval card is collapsed to a single-line banner. @@ -526,6 +878,7 @@ impl ApprovalView { selected: 0, locale, pending_confirm: None, + pending_persist_rule: false, timeout: None, requested_at: Instant::now(), collapsed: false, @@ -537,11 +890,13 @@ impl ApprovalView { // Moving the selection abandons any staged confirmation; the // user is reconsidering. self.pending_confirm = None; + self.pending_persist_rule = false; } fn select_next(&mut self) { self.selected = (self.selected + 1).min(ApprovalOption::ORDER.len() - 1); self.pending_confirm = None; + self.pending_persist_rule = false; } fn current_option(&self) -> ApprovalOption { @@ -572,6 +927,10 @@ impl ApprovalView { self.pending_confirm } + pub(crate) fn pending_persist_rule(&self) -> bool { + self.pending_persist_rule + } + pub(crate) fn locale(&self) -> Locale { self.locale } @@ -580,6 +939,7 @@ impl ApprovalView { /// variant's confirmation policy. Returns the action the modal /// stack should apply. fn commit_or_stage(&mut self, option: ApprovalOption) -> ViewAction { + self.pending_persist_rule = false; if option.requires_confirm(self.request.risk) { // Two-step destructive flow: first press stages, second // press of the same option commits. @@ -593,10 +953,20 @@ impl ApprovalView { } // Benign variant or non-approve options commit immediately. self.pending_confirm = None; + self.pending_persist_rule = false; self.emit_decision(option.decision(), false) } fn emit_decision(&self, decision: ReviewDecision, timed_out: bool) -> ViewAction { + self.emit_decision_with_rules(decision, timed_out, Vec::new()) + } + + fn emit_decision_with_rules( + &self, + decision: ReviewDecision, + timed_out: bool, + persistent_rules: Vec, + ) -> ViewAction { ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { tool_id: self.request.id.clone(), tool_name: self.request.tool_name.clone(), @@ -604,9 +974,30 @@ impl ApprovalView { timed_out, approval_key: self.request.approval_key.clone(), approval_grouping_key: self.request.approval_grouping_key.clone(), + persistent_rules, }) } + fn commit_or_stage_persist_rule(&mut self) -> ViewAction { + if self.request.persistent_rules.is_empty() { + self.pending_confirm = None; + self.pending_persist_rule = false; + return ViewAction::None; + } + if matches!(self.request.risk, RiskLevel::Destructive) && !self.pending_persist_rule { + self.pending_confirm = None; + self.pending_persist_rule = true; + return ViewAction::None; + } + self.pending_confirm = None; + self.pending_persist_rule = false; + self.emit_decision_with_rules( + ReviewDecision::Approved, + false, + self.request.persistent_rules.clone(), + ) + } + fn emit_params_pager(&self) -> ViewAction { let content = serde_json::to_string_pretty(&self.request.params) .unwrap_or_else(|_| self.request.params.to_string()); @@ -616,6 +1007,16 @@ impl ApprovalView { }) } + fn emit_permission_rule_pager(&self) -> ViewAction { + let Some(content) = self.request.permission_rule_preview() else { + return ViewAction::None; + }; + ViewAction::Emit(ViewEvent::OpenTextPager { + title: format!("Permission Rule: {}", self.request.tool_name), + content, + }) + } + fn is_timed_out(&self) -> bool { match self.timeout { Some(timeout) => self.requested_at.elapsed() >= timeout, @@ -656,6 +1057,7 @@ impl ModalView for ApprovalView { KeyCode::Char('a') | KeyCode::Char('A') | KeyCode::Char('2') => { self.commit_or_stage(ApprovalOption::ApproveAlways) } + KeyCode::Char('s') | KeyCode::Char('S') => self.commit_or_stage_persist_rule(), KeyCode::Char('n') | KeyCode::Char('N') | KeyCode::Char('d') @@ -663,13 +1065,20 @@ impl ModalView for ApprovalView { | KeyCode::Char('3') => self.commit_or_stage(ApprovalOption::Deny), KeyCode::Char('v') | KeyCode::Char('V') => { self.pending_confirm = None; + self.pending_persist_rule = false; self.emit_params_pager() } + KeyCode::Char('p') | KeyCode::Char('P') => { + self.pending_confirm = None; + self.pending_persist_rule = false; + self.emit_permission_rule_pager() + } KeyCode::Esc => self.emit_decision(ReviewDecision::Abort, false), _ => { // Any unrecognised key cancels a staged confirmation — // the user is no longer aiming at "approve". self.pending_confirm = None; + self.pending_persist_rule = false; ViewAction::None } } @@ -1154,6 +1563,138 @@ mod tests { ); } + #[test] + fn persistent_permission_rule_preview_uses_exact_command() { + let request = ApprovalRequest::new( + "test-id", + "exec_shell", + "Run a shell command", + &json!({"command": "cargo test --workspace"}), + "test_key", + ); + + assert_eq!( + request.persistent_rules, + vec![ToolPermissionRule::exec_shell( + PermissionDecision::Allow, + "cargo test --workspace" + )] + ); + let preview = request + .permission_rule_preview() + .expect("exec_shell should produce a preview"); + assert!(preview.contains("[[permissions.rules]]")); + assert!(preview.contains("tool = \"exec_shell\"")); + assert!(preview.contains("decision = \"allow\"")); + assert!(preview.contains("command = \"cargo test --workspace\"")); + } + + #[test] + fn persistent_permission_rule_preserves_shell_alias_tool_name() { + let rules = build_persistent_permission_rules( + "exec_shell_wait", + &json!({"command": "cargo test --workspace"}), + ); + + let mut expected = ToolPermissionRule::new("exec_shell_wait", PermissionDecision::Allow); + expected.command = Some("cargo test --workspace".to_string()); + assert_eq!(rules, vec![expected]); + } + + #[test] + fn persistent_permission_rules_extract_apply_patch_diff_paths() { + let rules = build_persistent_permission_rules( + "apply_patch", + &json!({ + "patch": "\ + --- a/docs/old.md\n\ + +++ b/docs/new.md\n\ + @@ -1 +1 @@\n" + }), + ); + + assert_eq!( + rules, + vec![ToolPermissionRule::file_path( + "apply_patch", + PermissionDecision::Allow, + "docs/new.md" + )] + ); + } + + #[test] + fn persistent_permission_rules_skip_glob_like_file_paths() { + assert!( + build_persistent_permission_rules("read_file", &json!({"path": "src/**"})).is_empty() + ); + assert!( + build_persistent_permission_rules( + "apply_patch", + &json!({"files": [{"path": "docs/*.md"}]}) + ) + .is_empty() + ); + } + + #[test] + fn persistent_permission_rules_normalize_absolute_paths_to_workspace_relative() { + let workspace = tempfile::tempdir().expect("workspace"); + let path = workspace.path().join("src").join("main.rs"); + + let rules = build_persistent_permission_rules_for_workspace( + "write_file", + &json!({"path": path.to_string_lossy()}), + workspace.path(), + ); + + assert_eq!( + rules, + vec![ + ToolPermissionRule::file_path( + "write_file", + PermissionDecision::Allow, + "src/main.rs" + ), + ToolPermissionRule::file_path( + "file_write", + PermissionDecision::Allow, + "src/main.rs" + ) + ] + ); + } + + #[test] + fn persistent_permission_rules_extract_apply_patch_file_entries() { + let rules = build_persistent_permission_rules( + "apply_patch", + &json!({ + "files": [ + {"path": "src/lib.rs"}, + {"path": "src/lib.rs"}, + {"path": "src/main.rs"} + ] + }), + ); + + assert_eq!( + rules, + vec![ + ToolPermissionRule::file_path( + "apply_patch", + PermissionDecision::Allow, + "src/lib.rs" + ), + ToolPermissionRule::file_path( + "apply_patch", + PermissionDecision::Allow, + "src/main.rs" + ) + ] + ); + } + // ======================================================================== // ApprovalView Tests — Benign Variant (single-key approve) // ======================================================================== @@ -1253,6 +1794,53 @@ mod tests { )); } + #[test] + fn benign_s_saves_rule_and_approves() { + let mut view = ApprovalView::new(benign_request()); + let action = view.handle_key(create_key_event(KeyCode::Char('s'))); + + match action { + ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision, + persistent_rules, + .. + }) => { + assert_eq!(decision, ReviewDecision::Approved); + assert_eq!( + persistent_rules, + vec![ + ToolPermissionRule::file_path( + "read_file", + PermissionDecision::Allow, + "src/main.rs" + ), + ToolPermissionRule::file_path( + "file_read", + PermissionDecision::Allow, + "src/main.rs" + ) + ] + ); + } + other => panic!("expected persistent approval event, got {other:?}"), + } + } + + #[test] + fn p_opens_full_permission_rule_preview() { + let mut view = ApprovalView::new(benign_request()); + let action = view.handle_key(create_key_event(KeyCode::Char('p'))); + + match action { + ViewAction::Emit(ViewEvent::OpenTextPager { title, content }) => { + assert_eq!(title, "Permission Rule: read_file"); + assert!(content.contains("tool = \"read_file\"")); + assert!(content.contains("tool = \"file_read\"")); + } + other => panic!("expected permission rule pager, got {other:?}"), + } + } + #[test] fn benign_a_two_approves_for_session() { for code in [KeyCode::Char('a'), KeyCode::Char('A'), KeyCode::Char('2')] { @@ -1410,6 +1998,43 @@ mod tests { } } + #[test] + fn destructive_s_first_press_stages_then_second_saves_rule() { + let mut view = ApprovalView::new(destructive_request()); + + let action = view.handle_key(create_key_event(KeyCode::Char('s'))); + assert!(matches!(action, ViewAction::None)); + assert_eq!(view.pending_confirm(), None); + assert!(view.pending_persist_rule()); + + let action = view.handle_key(create_key_event(KeyCode::Char('s'))); + match action { + ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision, + persistent_rules, + .. + }) => { + assert_eq!(decision, ReviewDecision::Approved); + assert_eq!( + persistent_rules, + vec![ + ToolPermissionRule::file_path( + "write_file", + PermissionDecision::Allow, + "src/main.rs" + ), + ToolPermissionRule::file_path( + "file_write", + PermissionDecision::Allow, + "src/main.rs" + ) + ] + ); + } + other => panic!("expected persistent approval event, got {other:?}"), + } + } + #[test] fn destructive_enter_first_press_stages_then_second_commits() { let mut view = ApprovalView::new(destructive_request()); diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 672369427..703b00df9 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -1853,12 +1853,13 @@ async fn run_event_loop( } // Create approval request and show overlay - let request = ApprovalRequest::new( + let request = ApprovalRequest::new_with_workspace( &id, &tool_name, &description, &tool_input, &approval_key, + &app.workspace, ); log_sensitive_event( "tool.approval.prompted", @@ -5933,6 +5934,7 @@ async fn handle_view_events( timed_out, approval_key, approval_grouping_key, + persistent_rules, } => { if decision == ReviewDecision::ApprovedForSession { // Store the tool name (backward compat) and the lossy @@ -5943,9 +5945,45 @@ async fn handle_view_events( .insert(approval_grouping_key.clone()); } + let mut live_persistent_rules = Vec::new(); + if matches!( + decision, + ReviewDecision::Approved | ReviewDecision::ApprovedForSession + ) && !persistent_rules.is_empty() + { + match crate::commands::persist_permission_rules(&persistent_rules) { + Ok(path) => { + live_persistent_rules = persistent_rules; + let noun = if live_persistent_rules.len() == 1 { + "rule" + } else { + "rules" + }; + app.push_status_toast( + format!("Saved permission {noun} to {}", path.display()), + StatusToastLevel::Success, + Some(5_000), + ); + } + Err(err) => { + app.push_status_toast( + format!("Failed to save permission rule: {err}"), + StatusToastLevel::Error, + Some(10_000), + ); + } + } + } + match decision { ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { - let _ = engine_handle.approve_tool_call(tool_id).await; + if live_persistent_rules.is_empty() { + let _ = engine_handle.approve_tool_call(tool_id).await; + } else { + let _ = engine_handle + .approve_tool_call_with_rules(tool_id, live_persistent_rules) + .await; + } } ReviewDecision::Denied | ReviewDecision::Abort => { // Cache the denial so the model retry-loop doesn't diff --git a/crates/tui/src/tui/views/mod.rs b/crates/tui/src/tui/views/mod.rs index ed2e84f24..0753d91ef 100644 --- a/crates/tui/src/tui/views/mod.rs +++ b/crates/tui/src/tui/views/mod.rs @@ -96,6 +96,8 @@ pub enum ViewEvent { approval_key: String, /// Lossy / arity-aware fingerprint, used to scope *approvals*. approval_grouping_key: String, + /// Persistent allow rules the approval UI generated for "save this rule". + persistent_rules: Vec, }, ElevationDecision { tool_id: String, diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index a81797690..9178b60ff 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -1163,6 +1163,82 @@ impl Renderable for ApprovalWidget<'_> { lines.push(Line::from("")); + if let Some(preview) = self.request.permission_rule_preview() { + let preview_width = card_area.width.saturating_sub(16) as usize; + let preview_lines = preview.lines().collect::>(); + let preview_line_limit = 2; + for (idx, preview_line) in preview_lines.iter().take(preview_line_limit).enumerate() { + let rendered = crate::utils::truncate_with_ellipsis( + preview_line, + preview_width.max(20), + "...", + ); + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + if idx == 0 { + label_rule(locale) + } else { + " " + }, + Style::default().fg(palette::TEXT_HINT), + ), + Span::styled(rendered, Style::default().fg(palette::TEXT_SECONDARY)), + ])); + } + if preview_lines.len() > preview_line_limit { + let hidden = preview_lines.len() - preview_line_limit; + let hidden_line = hidden_rule_preview_line(locale, hidden); + let rendered = crate::utils::truncate_with_ellipsis( + &hidden_line, + preview_width.max(20), + "...", + ); + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled(" ", Style::default().fg(palette::TEXT_HINT)), + Span::styled(rendered, Style::default().fg(palette::TEXT_SECONDARY)), + ])); + } + let mut spans = vec![ + Span::raw(" "), + Span::styled( + "[s] ", + Style::default() + .fg(palette_colors.shortcut) + .add_modifier(Modifier::BOLD), + ), + Span::styled( + option_save_rule(locale), + Style::default().fg(palette::TEXT_BODY), + ), + ]; + if self.view.pending_persist_rule() { + spans.push(Span::raw(" ")); + spans.push(Span::styled( + staged_marker(locale), + Style::default() + .fg(palette_colors.accent) + .add_modifier(Modifier::BOLD), + )); + } + lines.push(Line::from(spans)); + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + "[p] ", + Style::default() + .fg(palette_colors.shortcut) + .add_modifier(Modifier::BOLD), + ), + Span::styled( + option_view_rule_preview(locale), + Style::default().fg(palette::TEXT_BODY), + ), + ])); + lines.push(Line::from("")); + } + let options = approval_options_for(risk, locale); let pending = self.view.pending_confirm(); @@ -1438,6 +1514,13 @@ fn label_params(locale: Locale) -> &'static str { } } +fn label_rule(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "规则：", + _ => "Rule: ", + } +} + fn staged_marker(locale: Locale) -> &'static str { match locale { Locale::ZhHans => "(待确认)", @@ -1445,6 +1528,13 @@ fn staged_marker(locale: Locale) -> &'static str { } } +fn hidden_rule_preview_line(locale: Locale, hidden: usize) -> String { + match locale { + Locale::ZhHans => format!("... 还有 {hidden} 行，按 [p] 查看完整规则"), + _ => format!("... +{hidden} more line(s); press [p] to view all"), + } +} + fn single_key_prefix(locale: Locale) -> &'static str { match locale { Locale::ZhHans => "单键批准：", @@ -1557,6 +1647,20 @@ fn option_approve_always(locale: Locale) -> &'static str { } } +fn option_save_rule(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "保存此规则并批准", + _ => "Save this rule and approve", + } +} + +fn option_view_rule_preview(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "查看完整规则预览", + _ => "View full rule preview", + } +} + fn option_deny(locale: Locale) -> &'static str { match locale { Locale::ZhHans => "拒绝本次调用", From 9ac9b69ad386085d64ea031032c78e5ec4a14c79 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Tue, 12 May 2026 17:53:10 +0800 Subject: [PATCH 13/19] fix(tui): address permission rule review findings --- Cargo.lock | 2 + crates/execpolicy/src/lib.rs | 93 ++++++++------ crates/tui/Cargo.toml | 1 + crates/tui/src/commands/config.rs | 153 ++++++++++++++++++------ crates/tui/src/core/engine/dispatch.rs | 10 +- crates/tui/src/core/engine/tests.rs | 49 ++++++++ crates/tui/src/core/engine/turn_loop.rs | 2 +- crates/tui/src/tui/approval.rs | 70 ++++++++--- 8 files changed, 284 insertions(+), 96 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4a686dd6f..156bc6600 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1019,6 +1019,7 @@ dependencies = [ "tokio", "tokio-util", "toml 0.9.11+spec-1.1.0", + "toml_edit", "tower-http", "tracing", "tracing-appender", @@ -5327,6 +5328,7 @@ dependencies = [ "indexmap", "toml_datetime 0.7.5+spec-1.1.0", "toml_parser", + "toml_writer", "winnow 0.7.14", ] diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index fb383ff26..fdfc448ff 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -276,6 +276,7 @@ pub struct ExecPolicyEngine { /// Layered rulesets (builtin → agent → user). When non-empty, takes precedence /// over the legacy flat lists below. rulesets: Vec, + layered_rules: Vec, /// Legacy flat lists kept for backward compatibility with `new()`. trusted_prefixes: Vec, denied_prefixes: Vec, @@ -287,8 +288,11 @@ pub struct ExecPolicyEngine { impl ExecPolicyEngine { /// Legacy constructor: wraps the two vecs into a User-layer ruleset. pub fn new(trusted_prefixes: Vec, denied_prefixes: Vec) -> Self { + let layered_rules = + build_layered_permission_rules(&[], &trusted_prefixes, &denied_prefixes); Self { rulesets: vec![], + layered_rules, trusted_prefixes, denied_prefixes, approved_for_session: HashSet::new(), @@ -300,8 +304,10 @@ impl ExecPolicyEngine { /// Rulesets are sorted by layer priority on construction. pub fn with_rulesets(mut rulesets: Vec) -> Self { rulesets.sort_by_key(|r| r.layer); + let layered_rules = build_layered_permission_rules(&rulesets, &[], &[]); Self { rulesets, + layered_rules, trusted_prefixes: vec![], denied_prefixes: vec![], approved_for_session: HashSet::new(), @@ -313,42 +319,15 @@ impl ExecPolicyEngine { pub fn add_ruleset(&mut self, ruleset: Ruleset) { self.rulesets.push(ruleset); self.rulesets.sort_by_key(|r| r.layer); - } - - fn layered_permission_rules(&self) -> Vec { - let mut rules = Vec::new(); - if self.rulesets.is_empty() { - rules.extend(legacy_command_rules( - RulesetLayer::User, - &self.trusted_prefixes, - &self.denied_prefixes, - )); - return rules; - } - - 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, - )); - } - rules.extend(legacy_command_rules( - RulesetLayer::User, + self.layered_rules = build_layered_permission_rules( + &self.rulesets, &self.trusted_prefixes, &self.denied_prefixes, - )); - rules + ); + } + + fn layered_permission_rules(&self) -> &[LayeredToolPermissionRule] { + &self.layered_rules } pub fn remember_session_approval(&mut self, approval_key: String) { @@ -361,7 +340,7 @@ impl ExecPolicyEngine { #[must_use] pub fn check_tool_permission(&self, ctx: ToolPermissionContext<'_>) -> ToolPermissionCheck { - let mut best: Option<(LayeredToolPermissionRule, usize)> = None; + let mut best: Option<(&LayeredToolPermissionRule, usize)> = None; for candidate in self.layered_permission_rules() { if !tool_rule_matches(&candidate.rule, &ctx, &self.arity_dict) { @@ -369,7 +348,7 @@ impl ExecPolicyEngine { } 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) + compare_rule_priority(candidate, specificity, current, *current_specificity) }); if should_replace { best = Some((candidate, specificity)); @@ -381,7 +360,7 @@ impl ExecPolicyEngine { decision: Some(matched.rule.decision), matched_rule: Some(MatchedToolPermissionRule { layer: matched.layer, - rule: matched.rule, + rule: matched.rule.clone(), }), }, None => ToolPermissionCheck::unmatched(), @@ -504,6 +483,46 @@ fn legacy_command_rules( .collect() } +fn build_layered_permission_rules( + rulesets: &[Ruleset], + trusted_prefixes: &[String], + denied_prefixes: &[String], +) -> Vec { + let mut rules = Vec::new(); + if rulesets.is_empty() { + rules.extend(legacy_command_rules( + RulesetLayer::User, + trusted_prefixes, + denied_prefixes, + )); + return rules; + } + + for ruleset in 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, + )); + } + rules.extend(legacy_command_rules( + RulesetLayer::User, + trusted_prefixes, + denied_prefixes, + )); + rules +} + fn compare_rule_priority( candidate: &LayeredToolPermissionRule, candidate_specificity: usize, diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index 2b8baf1ea..bfd28ce95 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -54,6 +54,7 @@ serde_json = { version = "1.0.149", features = ["preserve_order"] } schemars = { version = "1.2.1", features = ["derive", "preserve_order"] } shellexpand = "3" toml = "0.9.7" +toml_edit = "0.23.10" tokio = { version = "1.49.0", features = ["full"] } tokio-util = { version = "0.7.16", features = ["io"] } unicode-width = "0.2" diff --git a/crates/tui/src/commands/config.rs b/crates/tui/src/commands/config.rs index b440c6bf9..904cc3c4e 100644 --- a/crates/tui/src/commands/config.rs +++ b/crates/tui/src/commands/config.rs @@ -353,6 +353,7 @@ pub fn persist_permission_rules( ) -> anyhow::Result { use anyhow::Context; use std::fs; + use toml_edit::{ArrayOfTables, DocumentMut, Item, Table}; let path = config_toml_path()?; if let Some(parent) = path.parent() { @@ -360,60 +361,132 @@ pub fn persist_permission_rules( .with_context(|| format!("failed to create config directory {}", parent.display()))?; } - let mut doc: toml::Value = if path.exists() { + let mut doc: DocumentMut = if path.exists() { let raw = fs::read_to_string(&path) .with_context(|| format!("failed to read config at {}", path.display()))?; - toml::from_str(&raw) + raw.parse() .with_context(|| format!("failed to parse config at {}", path.display()))? } else { - toml::Value::Table(toml::value::Table::new()) + DocumentMut::new() }; - let table = doc - .as_table_mut() - .context("config.toml root must be a table")?; - let permissions_entry = table - .entry("permissions".to_string()) - .or_insert_with(|| toml::Value::Table(toml::value::Table::new())); + let permissions_entry = doc + .entry("permissions") + .or_insert_with(|| Item::Table(Table::new())); let permissions_table = permissions_entry .as_table_mut() .context("`permissions` section in config.toml must be a table")?; let rules_entry = permissions_table - .entry("rules".to_string()) - .or_insert_with(|| toml::Value::Array(Vec::new())); - let rules_array = rules_entry - .as_array_mut() - .context("`permissions.rules` in config.toml must be an array")?; - - for rule in rules { - let value = permission_rule_to_toml_value(rule); - if !rules_array.iter().any(|existing| existing == &value) { - rules_array.push(value); - } - } + .entry("rules") + .or_insert_with(|| Item::ArrayOfTables(ArrayOfTables::new())); - let body = toml::to_string_pretty(&doc).context("failed to serialize config.toml")?; + append_permission_rules(rules_entry, rules)?; + + let body = doc.to_string(); fs::write(&path, body) .with_context(|| format!("failed to write config at {}", path.display()))?; Ok(path) } -fn permission_rule_to_toml_value(rule: &deepseek_execpolicy::ToolPermissionRule) -> toml::Value { - let mut table = toml::value::Table::new(); - table.insert("tool".to_string(), toml::Value::String(rule.tool.clone())); +fn append_permission_rules( + rules_item: &mut toml_edit::Item, + rules: &[deepseek_execpolicy::ToolPermissionRule], +) -> anyhow::Result<()> { + match rules_item { + toml_edit::Item::ArrayOfTables(array) => { + for rule in rules { + if !array + .iter() + .any(|existing| permission_toml_table_matches_rule(existing, rule)) + { + array.push(permission_rule_to_toml_table(rule)); + } + } + } + toml_edit::Item::Value(value) => { + let Some(array) = value.as_array_mut() else { + anyhow::bail!("`permissions.rules` in config.toml must be an array"); + }; + for rule in rules { + if !array + .iter() + .any(|existing| permission_toml_value_matches_rule(existing, rule)) + { + array.push(permission_rule_to_toml_inline_table(rule)); + } + } + } + _ => anyhow::bail!("`permissions.rules` in config.toml must be an array"), + } + Ok(()) +} + +fn permission_rule_to_toml_table( + rule: &deepseek_execpolicy::ToolPermissionRule, +) -> toml_edit::Table { + let mut table = toml_edit::Table::new(); + table["tool"] = toml_edit::value(rule.tool.clone()); + table["decision"] = toml_edit::value(permission_decision_label(rule.decision)); + if let Some(command) = rule.command.as_deref() { + table["command"] = toml_edit::value(command); + } + if let Some(path) = rule.path.as_deref() { + table["path"] = toml_edit::value(path); + } + table +} + +fn permission_rule_to_toml_inline_table( + rule: &deepseek_execpolicy::ToolPermissionRule, +) -> toml_edit::Value { + let mut table = toml_edit::InlineTable::new(); + table.insert("tool", toml_edit::Value::from(rule.tool.clone())); table.insert( - "decision".to_string(), - toml::Value::String(permission_decision_label(rule.decision).to_string()), + "decision", + toml_edit::Value::from(permission_decision_label(rule.decision)), ); if let Some(command) = rule.command.as_deref() { - table.insert( - "command".to_string(), - toml::Value::String(command.to_string()), - ); + table.insert("command", toml_edit::Value::from(command)); } if let Some(path) = rule.path.as_deref() { - table.insert("path".to_string(), toml::Value::String(path.to_string())); + table.insert("path", toml_edit::Value::from(path)); } - toml::Value::Table(table) + toml_edit::Value::InlineTable(table) +} + +fn permission_toml_table_matches_rule( + table: &toml_edit::Table, + rule: &deepseek_execpolicy::ToolPermissionRule, +) -> bool { + table.get("tool").and_then(toml_edit::Item::as_str) == Some(rule.tool.as_str()) + && table.get("decision").and_then(toml_edit::Item::as_str) + == Some(permission_decision_label(rule.decision)) + && optional_toml_item_str_matches(table.get("command"), rule.command.as_deref()) + && optional_toml_item_str_matches(table.get("path"), rule.path.as_deref()) +} + +fn permission_toml_value_matches_rule( + value: &toml_edit::Value, + rule: &deepseek_execpolicy::ToolPermissionRule, +) -> bool { + let Some(table) = value.as_inline_table() else { + return false; + }; + table.get("tool").and_then(toml_edit::Value::as_str) == Some(rule.tool.as_str()) + && table.get("decision").and_then(toml_edit::Value::as_str) + == Some(permission_decision_label(rule.decision)) + && optional_toml_value_str_matches(table.get("command"), rule.command.as_deref()) + && optional_toml_value_str_matches(table.get("path"), rule.path.as_deref()) +} + +fn optional_toml_item_str_matches(item: Option<&toml_edit::Item>, expected: Option<&str>) -> bool { + item.and_then(toml_edit::Item::as_str) == expected +} + +fn optional_toml_value_str_matches( + value: Option<&toml_edit::Value>, + expected: Option<&str>, +) -> bool { + value.and_then(toml_edit::Value::as_str) == expected } fn permission_decision_label(decision: deepseek_execpolicy::PermissionDecision) -> &'static str { @@ -2052,13 +2125,25 @@ mod tests { // Seed the config with a sentinel key the picker MUST NOT clobber. fs::write( &path, - "api_key = \"sentinel-key\"\nmodel = \"deepseek-v4-pro\"\n", + "# user config header\napi_key = \"sentinel-key\" # keep inline\nmodel = \"deepseek-v4-pro\"\n\n[permissions]\n# user permission notes\n", ) .unwrap(); let written = persist_status_items(&[crate::config::StatusItem::Mode]) .expect("persist should succeed"); let body = fs::read_to_string(&written).expect("written file should be readable"); + assert!( + body.contains("# user config header"), + "round-trip lost top-level comment: {body}" + ); + assert!( + body.contains("# keep inline"), + "round-trip lost inline comment: {body}" + ); + assert!( + body.contains("# user permission notes"), + "round-trip lost permissions comment: {body}" + ); assert!( body.contains("api_key = \"sentinel-key\""), "round-trip lost api_key: {body}" diff --git a/crates/tui/src/core/engine/dispatch.rs b/crates/tui/src/core/engine/dispatch.rs index 08dd1caf3..5e2f925e3 100644 --- a/crates/tui/src/core/engine/dispatch.rs +++ b/crates/tui/src/core/engine/dispatch.rs @@ -358,11 +358,9 @@ fn apply_patch_permission_paths(input: &Value) -> Vec { } } } - if paths.is_empty() { - if let Some(patch) = input.get("patch").and_then(Value::as_str) { - for path in parse_unified_diff_permission_paths(patch) { - push_unique_path(&mut paths, &path); - } + if let Some(patch) = input.get("patch").and_then(Value::as_str) { + for path in parse_unified_diff_permission_paths(patch) { + push_unique_path(&mut paths, &path); } } paths @@ -390,7 +388,7 @@ fn parse_unified_diff_permission_paths(patch: &str) -> Vec { } fn normalize_diff_permission_path(raw: &str) -> Option { - let raw = raw.trim(); + let raw = raw.split('\t').next()?.trim(); if raw.is_empty() || raw == "/dev/null" || raw == "dev/null" { return None; } diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index f249bdd91..12eabbb92 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -365,6 +365,34 @@ fn typed_permission_requires_all_apply_patch_paths_to_be_allowed() { assert_eq!(decision, ToolPermissionOverride::Unmatched); } +#[test] +fn typed_permission_checks_apply_patch_diff_even_when_path_field_is_present() { + let engine = permission_engine_with_rules(vec![ + deepseek_execpolicy::ToolPermissionRule::file_path( + "apply_patch", + deepseek_execpolicy::PermissionDecision::Allow, + "docs/**", + ), + deepseek_execpolicy::ToolPermissionRule::file_path( + "apply_patch", + deepseek_execpolicy::PermissionDecision::Deny, + "secrets/**", + ), + ]); + + let decision = tool_permission_override_for_call( + &engine, + "apply_patch", + &json!({ + "path": "docs/a.md", + "patch": "--- a/docs/a.md\n+++ b/secrets/token.txt\n@@ -1 +1 @@\n-old\n+new\n" + }), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Deny { .. })); +} + #[test] fn typed_permission_allows_apply_patch_when_all_paths_match() { let engine = @@ -386,6 +414,27 @@ fn typed_permission_allows_apply_patch_when_all_paths_match() { assert!(matches!(decision, ToolPermissionOverride::Allow { .. })); } +#[test] +fn typed_permission_parses_apply_patch_paths_with_diff_timestamps() { + let engine = + permission_engine_with_rules(vec![deepseek_execpolicy::ToolPermissionRule::file_path( + "apply_patch", + deepseek_execpolicy::PermissionDecision::Allow, + "docs/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "apply_patch", + &json!({ + "patch": "--- a/docs/a.md\t2026-05-12 12:00:00\n+++ b/docs/a.md\t2026-05-12 12:00:01\n@@ -1 +1 @@\n-old\n+new\n" + }), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Allow { .. })); +} + fn api_tool(name: &str) -> Tool { Tool { tool_type: Some("function".to_string()), diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 7fa05c39c..319e866e9 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1263,7 +1263,7 @@ impl Engine { supports_parallel = spec.supports_parallel(); read_only = spec.is_read_only(); } else if tool_name == MULTI_TOOL_PARALLEL_NAME { - approval_required = false; + approval_required = true; approval_description = "Execute multiple read-only tools in parallel".to_string(); supports_parallel = false; diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index ebba59551..92bcf9262 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -332,20 +332,7 @@ fn permission_decision_label(decision: PermissionDecision) -> &'static str { } fn toml_string(value: &str) -> String { - let mut out = String::with_capacity(value.len() + 2); - out.push('"'); - for ch in value.chars() { - match ch { - '\\' => out.push_str("\\\\"), - '"' => out.push_str("\\\""), - '\n' => out.push_str("\\n"), - '\r' => out.push_str("\\r"), - '\t' => out.push_str("\\t"), - _ => out.push(ch), - } - } - out.push('"'); - out + toml_edit::Value::from(value).to_string() } fn file_path_permission_rules(tool_name: &str, path: &str) -> Vec { @@ -481,9 +468,7 @@ fn apply_patch_permission_paths(input: &Value) -> Vec { } } } - if paths.is_empty() - && let Some(patch) = input.get("patch").and_then(Value::as_str) - { + if let Some(patch) = input.get("patch").and_then(Value::as_str) { for path in parse_unified_diff_permission_paths(patch) { push_unique_permission_path(&mut paths, &path); } @@ -513,7 +498,7 @@ fn parse_unified_diff_permission_paths(patch: &str) -> Vec { } fn normalize_diff_permission_path(raw: &str) -> Option { - let raw = raw.trim(); + let raw = raw.split('\t').next()?.trim(); if raw.is_empty() || raw == "/dev/null" || raw == "dev/null" { return None; } @@ -1623,6 +1608,55 @@ mod tests { ); } + #[test] + fn persistent_permission_rules_extract_patch_even_with_path_field() { + let rules = build_persistent_permission_rules( + "apply_patch", + &json!({ + "path": "docs/declared.md", + "patch": "\ + --- a/docs/old.md\n\ + +++ b/docs/new.md\n\ + @@ -1 +1 @@\n" + }), + ); + + assert_eq!( + rules, + vec![ + ToolPermissionRule::file_path( + "apply_patch", + PermissionDecision::Allow, + "docs/declared.md" + ), + ToolPermissionRule::file_path( + "apply_patch", + PermissionDecision::Allow, + "docs/new.md" + ) + ] + ); + } + + #[test] + fn persistent_permission_rules_parse_diff_headers_with_timestamps() { + let rules = build_persistent_permission_rules( + "apply_patch", + &json!({ + "patch": "--- a/docs/a.md\t2026-05-12 12:00:00\n+++ b/docs/a.md\t2026-05-12 12:00:01\n@@ -1 +1 @@\n-old\n+new\n" + }), + ); + + assert_eq!( + rules, + vec![ToolPermissionRule::file_path( + "apply_patch", + PermissionDecision::Allow, + "docs/a.md" + )] + ); + } + #[test] fn persistent_permission_rules_skip_glob_like_file_paths() { assert!( From 8cdbddbc425cb8fd58345565129df96c6cb16263 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Tue, 12 May 2026 18:23:32 +0800 Subject: [PATCH 14/19] test(tui): keep comment assertions scoped to permission rules --- crates/tui/src/commands/config.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/crates/tui/src/commands/config.rs b/crates/tui/src/commands/config.rs index 904cc3c4e..4d6e0aee8 100644 --- a/crates/tui/src/commands/config.rs +++ b/crates/tui/src/commands/config.rs @@ -2125,25 +2125,13 @@ mod tests { // Seed the config with a sentinel key the picker MUST NOT clobber. fs::write( &path, - "# user config header\napi_key = \"sentinel-key\" # keep inline\nmodel = \"deepseek-v4-pro\"\n\n[permissions]\n# user permission notes\n", + "api_key = \"sentinel-key\"\nmodel = \"deepseek-v4-pro\"\n", ) .unwrap(); let written = persist_status_items(&[crate::config::StatusItem::Mode]) .expect("persist should succeed"); let body = fs::read_to_string(&written).expect("written file should be readable"); - assert!( - body.contains("# user config header"), - "round-trip lost top-level comment: {body}" - ); - assert!( - body.contains("# keep inline"), - "round-trip lost inline comment: {body}" - ); - assert!( - body.contains("# user permission notes"), - "round-trip lost permissions comment: {body}" - ); assert!( body.contains("api_key = \"sentinel-key\""), "round-trip lost api_key: {body}" @@ -2176,7 +2164,7 @@ mod tests { fs::create_dir_all(path.parent().unwrap()).unwrap(); fs::write( &path, - "api_key = \"sentinel-key\"\nmodel = \"deepseek-v4-pro\"\n", + "# user config header\napi_key = \"sentinel-key\" # keep inline\nmodel = \"deepseek-v4-pro\"\n\n[permissions]\n# user permission notes\n", ) .unwrap(); @@ -2194,6 +2182,18 @@ mod tests { .expect("persist should succeed"); let body = fs::read_to_string(&written).expect("written file should be readable"); + assert!( + body.contains("# user config header"), + "round-trip lost top-level comment: {body}" + ); + assert!( + body.contains("# keep inline"), + "round-trip lost inline comment: {body}" + ); + assert!( + body.contains("# user permission notes"), + "round-trip lost permissions comment: {body}" + ); assert!( body.contains("api_key = \"sentinel-key\""), "round-trip lost api_key: {body}" From e16c36daa3b8d1526d9068535e627c53bc64f93f Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 25 May 2026 12:11:59 +0800 Subject: [PATCH 15/19] fix(tui): adapt persisted permission UI to codewhale crates --- crates/execpolicy/src/lib.rs | 8 +++---- crates/tui/src/commands/config.rs | 28 +++++++++++------------ crates/tui/src/commands/mod.rs | 2 +- crates/tui/src/core/engine/approval.rs | 4 ++-- crates/tui/src/core/engine/handle.rs | 2 +- crates/tui/src/core/engine/tests.rs | 24 ++++++++++---------- crates/tui/src/core/engine/turn_loop.rs | 30 ++++++++++++------------- crates/tui/src/tui/approval.rs | 2 +- crates/tui/src/tui/views/mod.rs | 2 +- 9 files changed, 50 insertions(+), 52 deletions(-) diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index fdfc448ff..6ef908fa5 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -593,10 +593,10 @@ fn command_rule_matches( 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("/**") { - if path == prefix || path.starts_with(&format!("{prefix}/")) { - return true; - } + if let Some(prefix) = pattern.strip_suffix("/**") + && (path == prefix || path.starts_with(&format!("{prefix}/"))) + { + return true; } if !pattern.contains('*') && !pattern.contains('?') { return pattern == path; diff --git a/crates/tui/src/commands/config.rs b/crates/tui/src/commands/config.rs index 4d6e0aee8..26ec9a9f6 100644 --- a/crates/tui/src/commands/config.rs +++ b/crates/tui/src/commands/config.rs @@ -349,7 +349,7 @@ pub fn persist_root_string_key(key: &str, value: &str) -> anyhow::Result anyhow::Result { use anyhow::Context; use std::fs; @@ -389,7 +389,7 @@ pub fn persist_permission_rules( fn append_permission_rules( rules_item: &mut toml_edit::Item, - rules: &[deepseek_execpolicy::ToolPermissionRule], + rules: &[codewhale_execpolicy::ToolPermissionRule], ) -> anyhow::Result<()> { match rules_item { toml_edit::Item::ArrayOfTables(array) => { @@ -421,7 +421,7 @@ fn append_permission_rules( } fn permission_rule_to_toml_table( - rule: &deepseek_execpolicy::ToolPermissionRule, + rule: &codewhale_execpolicy::ToolPermissionRule, ) -> toml_edit::Table { let mut table = toml_edit::Table::new(); table["tool"] = toml_edit::value(rule.tool.clone()); @@ -436,7 +436,7 @@ fn permission_rule_to_toml_table( } fn permission_rule_to_toml_inline_table( - rule: &deepseek_execpolicy::ToolPermissionRule, + rule: &codewhale_execpolicy::ToolPermissionRule, ) -> toml_edit::Value { let mut table = toml_edit::InlineTable::new(); table.insert("tool", toml_edit::Value::from(rule.tool.clone())); @@ -455,7 +455,7 @@ fn permission_rule_to_toml_inline_table( fn permission_toml_table_matches_rule( table: &toml_edit::Table, - rule: &deepseek_execpolicy::ToolPermissionRule, + rule: &codewhale_execpolicy::ToolPermissionRule, ) -> bool { table.get("tool").and_then(toml_edit::Item::as_str) == Some(rule.tool.as_str()) && table.get("decision").and_then(toml_edit::Item::as_str) @@ -466,7 +466,7 @@ fn permission_toml_table_matches_rule( fn permission_toml_value_matches_rule( value: &toml_edit::Value, - rule: &deepseek_execpolicy::ToolPermissionRule, + rule: &codewhale_execpolicy::ToolPermissionRule, ) -> bool { let Some(table) = value.as_inline_table() else { return false; @@ -489,11 +489,11 @@ fn optional_toml_value_str_matches( value.and_then(toml_edit::Value::as_str) == expected } -fn permission_decision_label(decision: deepseek_execpolicy::PermissionDecision) -> &'static str { +fn permission_decision_label(decision: codewhale_execpolicy::PermissionDecision) -> &'static str { match decision { - deepseek_execpolicy::PermissionDecision::Allow => "allow", - deepseek_execpolicy::PermissionDecision::Deny => "deny", - deepseek_execpolicy::PermissionDecision::Ask => "ask", + codewhale_execpolicy::PermissionDecision::Allow => "allow", + codewhale_execpolicy::PermissionDecision::Deny => "deny", + codewhale_execpolicy::PermissionDecision::Ask => "ask", } } @@ -2168,13 +2168,13 @@ mod tests { ) .unwrap(); - let cargo_rule = deepseek_execpolicy::ToolPermissionRule::exec_shell( - deepseek_execpolicy::PermissionDecision::Allow, + let cargo_rule = codewhale_execpolicy::ToolPermissionRule::exec_shell( + codewhale_execpolicy::PermissionDecision::Allow, "cargo test", ); - let docs_rule = deepseek_execpolicy::ToolPermissionRule::file_path( + let docs_rule = codewhale_execpolicy::ToolPermissionRule::file_path( "read_file", - deepseek_execpolicy::PermissionDecision::Allow, + codewhale_execpolicy::PermissionDecision::Allow, "docs/README.md", ); let written = diff --git a/crates/tui/src/commands/mod.rs b/crates/tui/src/commands/mod.rs index 5aab04811..cebde5aae 100644 --- a/crates/tui/src/commands/mod.rs +++ b/crates/tui/src/commands/mod.rs @@ -708,7 +708,7 @@ pub fn persist_root_string_key(key: &str, value: &str) -> anyhow::Result anyhow::Result { config::persist_permission_rules(rules) } diff --git a/crates/tui/src/core/engine/approval.rs b/crates/tui/src/core/engine/approval.rs index af9285dcf..92a31f3af 100644 --- a/crates/tui/src/core/engine/approval.rs +++ b/crates/tui/src/core/engine/approval.rs @@ -15,7 +15,7 @@ use super::Engine; pub(super) enum ApprovalDecision { Approved { id: String, - persistent_rules: Vec, + persistent_rules: Vec, }, Denied { id: String, @@ -93,7 +93,7 @@ impl Engine { } if id == tool_id => { if !persistent_rules.is_empty() { self.config.exec_policy_engine.add_ruleset( - deepseek_execpolicy::Ruleset::user(vec![], vec![]) + codewhale_execpolicy::Ruleset::user(vec![], vec![]) .with_rules(persistent_rules), ); } diff --git a/crates/tui/src/core/engine/handle.rs b/crates/tui/src/core/engine/handle.rs index 5dbecd719..5e390b113 100644 --- a/crates/tui/src/core/engine/handle.rs +++ b/crates/tui/src/core/engine/handle.rs @@ -60,7 +60,7 @@ impl EngineHandle { pub async fn approve_tool_call_with_rules( &self, id: impl Into, - persistent_rules: Vec, + persistent_rules: Vec, ) -> Result<()> { self.tx_approval .send(ApprovalDecision::Approved { diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 12eabbb92..25a04e5c3 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -295,19 +295,19 @@ fn typed_permission_matches_legacy_file_tool_aliases() { #[test] fn typed_permission_exact_saved_alias_allow_beats_broad_alias_ask() { let engine = permission_engine_with_rules(vec![ - deepseek_execpolicy::ToolPermissionRule::file_path( + codewhale_execpolicy::ToolPermissionRule::file_path( "file_edit", - deepseek_execpolicy::PermissionDecision::Ask, + codewhale_execpolicy::PermissionDecision::Ask, "src/**", ), - deepseek_execpolicy::ToolPermissionRule::file_path( + codewhale_execpolicy::ToolPermissionRule::file_path( "edit_file", - deepseek_execpolicy::PermissionDecision::Allow, + codewhale_execpolicy::PermissionDecision::Allow, "src/main.rs", ), - deepseek_execpolicy::ToolPermissionRule::file_path( + codewhale_execpolicy::ToolPermissionRule::file_path( "file_edit", - deepseek_execpolicy::PermissionDecision::Allow, + codewhale_execpolicy::PermissionDecision::Allow, "src/main.rs", ), ]); @@ -368,14 +368,14 @@ fn typed_permission_requires_all_apply_patch_paths_to_be_allowed() { #[test] fn typed_permission_checks_apply_patch_diff_even_when_path_field_is_present() { let engine = permission_engine_with_rules(vec![ - deepseek_execpolicy::ToolPermissionRule::file_path( + codewhale_execpolicy::ToolPermissionRule::file_path( "apply_patch", - deepseek_execpolicy::PermissionDecision::Allow, + codewhale_execpolicy::PermissionDecision::Allow, "docs/**", ), - deepseek_execpolicy::ToolPermissionRule::file_path( + codewhale_execpolicy::ToolPermissionRule::file_path( "apply_patch", - deepseek_execpolicy::PermissionDecision::Deny, + codewhale_execpolicy::PermissionDecision::Deny, "secrets/**", ), ]); @@ -417,9 +417,9 @@ fn typed_permission_allows_apply_patch_when_all_paths_match() { #[test] fn typed_permission_parses_apply_patch_paths_with_diff_timestamps() { let engine = - permission_engine_with_rules(vec![deepseek_execpolicy::ToolPermissionRule::file_path( + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( "apply_patch", - deepseek_execpolicy::PermissionDecision::Allow, + codewhale_execpolicy::PermissionDecision::Allow, "docs/**", )]); diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 319e866e9..b6a50e02a 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1314,28 +1314,26 @@ impl Engine { } } - if blocked_error.is_none() { - if let Some(result) = maybe_hydrate_requested_deferred_tool( + if blocked_error.is_none() + && let Some(result) = maybe_hydrate_requested_deferred_tool( &tool_name, &tool_input, &tool_catalog, &active_tools_at_batch_start, &mut deferred_tools_hydrated_this_batch, - ) { - if should_emit_hydration_status { - let status = if requested_tool_name == tool_name { - format!( - "Auto-loaded deferred tool '{tool_name}' after model request." - ) - } else { - format!( - "Auto-loaded deferred tool '{tool_name}' after resolving '{requested_tool_name}'." - ) - }; - let _ = self.tx_event.send(Event::status(status)).await; - } - guard_result = Some(result); + ) + { + if should_emit_hydration_status { + let status = if requested_tool_name == tool_name { + format!("Auto-loaded deferred tool '{tool_name}' after model request.") + } else { + format!( + "Auto-loaded deferred tool '{tool_name}' after resolving '{requested_tool_name}'." + ) + }; + let _ = self.tx_event.send(Event::status(status)).await; } + guard_result = Some(result); } if blocked_error.is_none() diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index 92bcf9262..7afe80b54 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -31,8 +31,8 @@ use crate::localization::Locale; use crate::sandbox::SandboxPolicy; use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent}; use crate::tui::widgets::{ApprovalWidget, ElevationWidget, Renderable}; +use codewhale_execpolicy::{PermissionDecision, ToolPermissionRule}; use crossterm::event::{KeyCode, KeyEvent}; -use deepseek_execpolicy::{PermissionDecision, ToolPermissionRule}; use serde_json::Value; use std::path::{Component, Path, PathBuf}; use std::time::{Duration, Instant}; diff --git a/crates/tui/src/tui/views/mod.rs b/crates/tui/src/tui/views/mod.rs index 0753d91ef..2f1621fd3 100644 --- a/crates/tui/src/tui/views/mod.rs +++ b/crates/tui/src/tui/views/mod.rs @@ -97,7 +97,7 @@ pub enum ViewEvent { /// Lossy / arity-aware fingerprint, used to scope *approvals*. approval_grouping_key: String, /// Persistent allow rules the approval UI generated for "save this rule". - persistent_rules: Vec, + persistent_rules: Vec, }, ElevationDecision { tool_id: String, From d39434fd24240dada4d0f4477e09d2df73269a5a Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 25 May 2026 12:35:59 +0800 Subject: [PATCH 16/19] fix(tui): harden persisted permission rule handling --- crates/execpolicy/src/lib.rs | 4 +- crates/tui/src/commands/config.rs | 16 ++-- crates/tui/src/core/engine/dispatch.rs | 111 +++++++++++++++++++++++-- crates/tui/src/core/engine/tests.rs | 48 +++++++++++ crates/tui/src/tui/approval.rs | 7 +- 5 files changed, 168 insertions(+), 18 deletions(-) diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index 6ef908fa5..b90bdaf01 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -643,7 +643,9 @@ fn command_prefix_matches(pattern: &str, command: &str) -> bool { .is_some_and(|suffix| suffix.is_empty() || suffix.starts_with(' ')) } -fn normalize_path_pattern(value: &str) -> String { +/// Normalize permission path patterns to slash-separated paths with `.` and +/// safe `..` segments collapsed. +pub fn normalize_path_pattern(value: &str) -> String { let raw = value.trim().replace('\\', "/"); let absolute = raw.starts_with('/'); let mut segments = Vec::new(); diff --git a/crates/tui/src/commands/config.rs b/crates/tui/src/commands/config.rs index 26ec9a9f6..c80fb000c 100644 --- a/crates/tui/src/commands/config.rs +++ b/crates/tui/src/commands/config.rs @@ -315,8 +315,7 @@ pub fn persist_status_items(items: &[crate::config::StatusItem]) -> anyhow::Resu tui_table.insert("status_items".to_string(), toml::Value::Array(array)); let body = toml::to_string_pretty(&doc).context("failed to serialize config.toml")?; - fs::write(&path, body) - .with_context(|| format!("failed to write config at {}", path.display()))?; + write_config_toml(&path, &body)?; Ok(path) } @@ -343,8 +342,7 @@ pub fn persist_root_string_key(key: &str, value: &str) -> anyhow::Result anyhow::Result<()> { + use anyhow::Context; + + crate::utils::write_atomic(path, body.as_bytes()) + .with_context(|| format!("failed to write config at {}", path.display())) +} + fn append_permission_rules( rules_item: &mut toml_edit::Item, rules: &[codewhale_execpolicy::ToolPermissionRule], diff --git a/crates/tui/src/core/engine/dispatch.rs b/crates/tui/src/core/engine/dispatch.rs index 5e2f925e3..72f9d5432 100644 --- a/crates/tui/src/core/engine/dispatch.rs +++ b/crates/tui/src/core/engine/dispatch.rs @@ -17,6 +17,7 @@ use codewhale_execpolicy::{ ExecPolicyEngine, PermissionDecision, ToolPermissionCheck, ToolPermissionContext, + normalize_path_pattern, }; use serde_json::{Value, json}; use std::path::{Component, Path, PathBuf}; @@ -369,24 +370,115 @@ fn apply_patch_permission_paths(input: &Value) -> Vec { fn parse_unified_diff_permission_paths(patch: &str) -> Vec { let mut paths = Vec::new(); let mut old_path: Option = None; + let mut state = DiffHeaderState::ExpectOld; for line in patch.lines() { - if let Some(stripped) = line.strip_prefix("--- ") { - old_path = normalize_diff_permission_path(stripped); + if line.starts_with("diff --git ") || line.starts_with("Index: ") { + old_path = None; + state = DiffHeaderState::ExpectOld; continue; } - if let Some(stripped) = line.strip_prefix("+++ ") { - let new_path = normalize_diff_permission_path(stripped); - if let Some(path) = new_path.or_else(|| old_path.clone()) { - push_unique_path(&mut paths, &path); + + state = match state { + DiffHeaderState::ExpectOld => { + if let Some(stripped) = line.strip_prefix("--- ") { + old_path = normalize_diff_permission_path(stripped); + DiffHeaderState::ExpectNew + } else { + DiffHeaderState::ExpectOld + } } - old_path = None; - } + DiffHeaderState::ExpectNew => { + if let Some(stripped) = line.strip_prefix("+++ ") { + let new_path = normalize_diff_permission_path(stripped); + if let Some(path) = new_path.or_else(|| old_path.clone()) { + push_unique_path(&mut paths, &path); + } + old_path = None; + DiffHeaderState::AfterHeader + } else if let Some(stripped) = line.strip_prefix("--- ") { + old_path = normalize_diff_permission_path(stripped); + DiffHeaderState::ExpectNew + } else { + old_path = None; + DiffHeaderState::ExpectOld + } + } + DiffHeaderState::AfterHeader => { + if let Some((old_remaining, new_remaining)) = parse_unified_hunk_counts(line) { + DiffHeaderState::InHunk { + old_remaining, + new_remaining, + } + } else if let Some(stripped) = line.strip_prefix("--- ") { + old_path = normalize_diff_permission_path(stripped); + DiffHeaderState::ExpectNew + } else { + DiffHeaderState::AfterHeader + } + } + DiffHeaderState::InHunk { + old_remaining, + new_remaining, + } => update_diff_hunk_state(line, old_remaining, new_remaining), + }; } paths } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum DiffHeaderState { + ExpectOld, + ExpectNew, + AfterHeader, + InHunk { + old_remaining: usize, + new_remaining: usize, + }, +} + +fn parse_unified_hunk_counts(line: &str) -> Option<(usize, usize)> { + let header = line.strip_prefix("@@ ")?; + let header = header.split(" @@").next()?; + let mut parts = header.split_whitespace(); + let old_remaining = parse_unified_hunk_range_count(parts.next()?, '-')?; + let new_remaining = parse_unified_hunk_range_count(parts.next()?, '+')?; + Some((old_remaining, new_remaining)) +} + +fn parse_unified_hunk_range_count(raw: &str, prefix: char) -> Option { + let range = raw.strip_prefix(prefix)?; + range + .split_once(',') + .map(|(_, count)| count.parse().ok()) + .unwrap_or(Some(1)) +} + +fn update_diff_hunk_state( + line: &str, + old_remaining: usize, + new_remaining: usize, +) -> DiffHeaderState { + let (old_remaining, new_remaining) = match line.as_bytes().first().copied() { + Some(b' ') => ( + old_remaining.saturating_sub(1), + new_remaining.saturating_sub(1), + ), + Some(b'-') => (old_remaining.saturating_sub(1), new_remaining), + Some(b'+') => (old_remaining, new_remaining.saturating_sub(1)), + _ => (old_remaining, new_remaining), + }; + if old_remaining == 0 && new_remaining == 0 { + DiffHeaderState::ExpectOld + } else { + DiffHeaderState::InHunk { + old_remaining, + new_remaining, + } + } +} + fn normalize_diff_permission_path(raw: &str) -> Option { let raw = raw.split('\t').next()?.trim(); if raw.is_empty() || raw == "/dev/null" || raw == "dev/null" { @@ -396,7 +488,8 @@ fn normalize_diff_permission_path(raw: &str) -> Option { .strip_prefix("a/") .or_else(|| raw.strip_prefix("b/")) .unwrap_or(raw); - Some(raw.to_string()) + let path = normalize_path_pattern(raw); + (!path.is_empty()).then_some(path) } fn push_unique_path(paths: &mut Vec, path: &str) { diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 25a04e5c3..e8e16292f 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -435,6 +435,54 @@ fn typed_permission_parses_apply_patch_paths_with_diff_timestamps() { assert!(matches!(decision, ToolPermissionOverride::Allow { .. })); } +#[test] +fn typed_permission_ignores_diff_like_hunk_content() { + let engine = permission_engine_with_rules(vec![ + codewhale_execpolicy::ToolPermissionRule::file_path( + "apply_patch", + codewhale_execpolicy::PermissionDecision::Allow, + "docs/**", + ), + codewhale_execpolicy::ToolPermissionRule::file_path( + "apply_patch", + codewhale_execpolicy::PermissionDecision::Deny, + "secrets/**", + ), + ]); + + let decision = tool_permission_override_for_call( + &engine, + "apply_patch", + &json!({ + "patch": "--- a/docs/a.md\n+++ b/docs/a.md\n@@ -1,2 +1,2 @@\n--- secrets/token.txt\n+++ secrets/token.txt\n" + }), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Allow { .. })); +} + +#[test] +fn typed_permission_parses_multiple_unified_files_without_git_headers() { + let engine = + permission_engine_with_rules(vec![codewhale_execpolicy::ToolPermissionRule::file_path( + "apply_patch", + codewhale_execpolicy::PermissionDecision::Allow, + "docs/**", + )]); + + let decision = tool_permission_override_for_call( + &engine, + "apply_patch", + &json!({ + "patch": "--- a/docs/a.md\n+++ b/docs/a.md\n@@ -1 +1 @@\n-old\n+new\n--- a/docs/b.md\n+++ b/docs/b.md\n@@ -1 +1 @@\n-old\n+new\n" + }), + permission_test_workspace(), + ); + + assert!(matches!(decision, ToolPermissionOverride::Allow { .. })); +} + fn api_tool(name: &str) -> Tool { Tool { tool_type: Some("function".to_string()), diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index 7afe80b54..0b5b60798 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -31,7 +31,7 @@ use crate::localization::Locale; use crate::sandbox::SandboxPolicy; use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent}; use crate::tui::widgets::{ApprovalWidget, ElevationWidget, Renderable}; -use codewhale_execpolicy::{PermissionDecision, ToolPermissionRule}; +use codewhale_execpolicy::{PermissionDecision, ToolPermissionRule, normalize_path_pattern}; use crossterm::event::{KeyCode, KeyEvent}; use serde_json::Value; use std::path::{Component, Path, PathBuf}; @@ -379,7 +379,10 @@ fn normalize_persistent_permission_path(raw: &str, workspace: Option<&Path>) -> normalize_path(raw_path) }; - let path = permission_path_to_string(&normalized); + let mut path = normalize_path_pattern(&permission_path_to_string(&normalized)); + if path.is_empty() { + path = ".".to_string(); + } if path_contains_glob_metachar(&path) { return None; } From eb6e4588f884b55b3f94dfc15011bb8503a8e799 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 25 May 2026 17:30:13 +0800 Subject: [PATCH 17/19] fix(tui): pass approval input through events --- crates/tui/src/core/engine/turn_loop.rs | 1 + crates/tui/src/core/events.rs | 2 ++ crates/tui/src/runtime_threads.rs | 6 ++++++ crates/tui/src/tui/ui.rs | 12 +++--------- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index b6a50e02a..09620f375 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1717,6 +1717,7 @@ impl Engine { id: tool_id.clone(), tool_name: tool_name.clone(), description: plan.approval_description.clone(), + input: tool_input.clone(), approval_key, approval_grouping_key, }) diff --git a/crates/tui/src/core/events.rs b/crates/tui/src/core/events.rs index b02ba2f99..171edbd94 100644 --- a/crates/tui/src/core/events.rs +++ b/crates/tui/src/core/events.rs @@ -226,6 +226,8 @@ pub enum Event { id: String, tool_name: String, description: String, + /// Finalized tool input used for approval display and persistent rule generation. + input: Value, /// Exact-argument fingerprint, used to scope *denials* (#1617). approval_key: String, /// Lossy / arity-aware fingerprint, used to scope *approvals* so an diff --git a/crates/tui/src/runtime_threads.rs b/crates/tui/src/runtime_threads.rs index 824a198a3..beac4391f 100644 --- a/crates/tui/src/runtime_threads.rs +++ b/crates/tui/src/runtime_threads.rs @@ -2642,6 +2642,7 @@ impl RuntimeThreadManager { id, tool_name, description, + input, .. } => { self.emit_event( @@ -2654,6 +2655,7 @@ impl RuntimeThreadManager { "approval_id": id, "tool_name": tool_name, "description": description, + "input": input, }), ) .await?; @@ -4162,6 +4164,7 @@ mod tests { id: "tool_stale".to_string(), tool_name: "exec_command".to_string(), description: "stale approval".to_string(), + input: json!({}), }) .await?; @@ -4235,6 +4238,7 @@ mod tests { id: "tool_external_allow".to_string(), tool_name: "exec_command".to_string(), description: "external allow".to_string(), + input: json!({}), }) .await?; @@ -4312,6 +4316,7 @@ mod tests { id: "tool_external_deny".to_string(), tool_name: "exec_command".to_string(), description: "external deny".to_string(), + input: json!({}), }) .await?; @@ -4498,6 +4503,7 @@ mod tests { id: "tool_remember".to_string(), tool_name: "exec_command".to_string(), description: "remember=true".to_string(), + input: json!({}), }) .await?; diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 703b00df9..0dd104fba 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -1797,6 +1797,7 @@ async fn run_event_loop( id, tool_name, description, + input, approval_key, approval_grouping_key, } => { @@ -1841,15 +1842,8 @@ async fn run_event_loop( app.status_message = Some(format!("Blocked tool '{tool_name}' (approval_mode=never)")); } else { - let tool_input = app - .pending_tool_uses - .iter() - .find(|(tool_id, _, _)| tool_id == &id) - .map(|(_, _, input)| input.clone()) - .unwrap_or_else(|| serde_json::json!({})); - if tool_name == "apply_patch" { - maybe_add_patch_preview(app, &tool_input); + maybe_add_patch_preview(app, &input); } // Create approval request and show overlay @@ -1857,7 +1851,7 @@ async fn run_event_loop( &id, &tool_name, &description, - &tool_input, + &input, &approval_key, &app.workspace, ); From 87eef2fc70364f8745e6ab7a00f201ed82ac5d22 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 25 May 2026 17:33:59 +0800 Subject: [PATCH 18/19] fix(tui): persist permission rules to resolved config --- crates/tui/src/commands/config.rs | 65 +++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 12 deletions(-) diff --git a/crates/tui/src/commands/config.rs b/crates/tui/src/commands/config.rs index c80fb000c..5fa3ab59f 100644 --- a/crates/tui/src/commands/config.rs +++ b/crates/tui/src/commands/config.rs @@ -501,19 +501,9 @@ fn permission_decision_label(decision: codewhale_execpolicy::PermissionDecision) } } -/// Resolve the path to `~/.deepseek/config.toml` (or -/// `$DEEPSEEK_CONFIG_PATH`). Mirrors what `Config::load` accepts so we -/// never write to a different file than the one we read. +/// Resolve the user config path using the same precedence as config loading. pub(super) fn config_toml_path() -> anyhow::Result { - use anyhow::Context; - if let Ok(env) = std::env::var("DEEPSEEK_CONFIG_PATH") { - let trimmed = env.trim(); - if !trimmed.is_empty() { - return Ok(PathBuf::from(trimmed)); - } - } - let home = dirs::home_dir().context("failed to resolve home directory for config.toml path")?; - Ok(home.join(".deepseek").join("config.toml")) + codewhale_config::resolve_config_path(None) } /// Modify a setting at runtime @@ -1417,6 +1407,7 @@ mod tests { struct EnvGuard { home: Option, userprofile: Option, + codewhale_config_path: Option, deepseek_config_path: Option, _lock: std::sync::MutexGuard<'static, ()>, } @@ -1429,18 +1420,21 @@ mod tests { let config_str = OsString::from(config_path.as_os_str()); let home_prev = env::var_os("HOME"); let userprofile_prev = env::var_os("USERPROFILE"); + let codewhale_config_prev = env::var_os("CODEWHALE_CONFIG_PATH"); let deepseek_config_prev = env::var_os("DEEPSEEK_CONFIG_PATH"); // Safety: test-only environment mutation guarded by process-wide mutex. unsafe { env::set_var("HOME", &home_str); env::set_var("USERPROFILE", &home_str); + env::remove_var("CODEWHALE_CONFIG_PATH"); env::set_var("DEEPSEEK_CONFIG_PATH", &config_str); } Self { home: home_prev, userprofile: userprofile_prev, + codewhale_config_path: codewhale_config_prev, deepseek_config_path: deepseek_config_prev, _lock: lock, } @@ -1473,6 +1467,18 @@ mod tests { } } + if let Some(value) = self.codewhale_config_path.take() { + // Safety: test-only environment mutation guarded by a global mutex. + unsafe { + env::set_var("CODEWHALE_CONFIG_PATH", value); + } + } else { + // Safety: test-only environment mutation guarded by a global mutex. + unsafe { + env::remove_var("CODEWHALE_CONFIG_PATH"); + } + } + if let Some(value) = self.deepseek_config_path.take() { // Safety: test-only environment mutation guarded by a global mutex. unsafe { @@ -2210,4 +2216,39 @@ mod tests { let config = Config::load(Some(written), None).expect("written config should load"); assert_eq!(config.permissions.rules, vec![cargo_rule, docs_rule]); } + + #[test] + fn persist_permission_rules_prefers_codewhale_config_path() { + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let temp_root = env::temp_dir().join(format!( + "codewhale-permission-rules-{}-{}", + std::process::id(), + nanos + )); + fs::create_dir_all(&temp_root).unwrap(); + let _guard = EnvGuard::new(&temp_root); + + let codewhale_path = temp_root.join(".codewhale").join("config.toml"); + let codewhale_config_str = OsString::from(codewhale_path.as_os_str()); + unsafe { + env::set_var("CODEWHALE_CONFIG_PATH", &codewhale_config_str); + } + + let rule = codewhale_execpolicy::ToolPermissionRule::file_path( + "write_file", + codewhale_execpolicy::PermissionDecision::Allow, + "src/testfile2", + ); + let written = persist_permission_rules(&[rule]).expect("persist should succeed"); + + assert_eq!(written, codewhale_path); + assert!(written.exists()); + assert!( + !temp_root.join(".deepseek").join("config.toml").exists(), + "persist should not fall back to legacy config when CODEWHALE_CONFIG_PATH is set" + ); + } } From ebc1f549af30152d4b8351cb80d612aefa07c017 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Mon, 25 May 2026 18:20:28 +0800 Subject: [PATCH 19/19] fix(tui): persist approval rules separately --- config.example.toml | 8 ++- crates/app-server/src/lib.rs | 2 +- crates/config/src/lib.rs | 112 +++++++++++++++++++++++++++--- crates/tui/src/commands/config.rs | 75 ++++++++++++-------- crates/tui/src/config.rs | 60 ++++++++++++++++ crates/tui/src/tui/approval.rs | 4 +- 6 files changed, 214 insertions(+), 47 deletions(-) diff --git a/config.example.toml b/config.example.toml index 79fd55be9..24ff3e2fb 100644 --- a/config.example.toml +++ b/config.example.toml @@ -143,9 +143,11 @@ sandbox_mode = "workspace-write" # read-only | workspace-write | danger-full-acc # 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. +# Typed persistent permission rules. User-approved persistent rules are written +# to a sibling permissions.toml file as top-level [[rules]] entries so this +# config file does not grow without bound. The config.toml form below remains +# supported for manual configuration. Shell commands use the same arity-aware +# matching as auto_allow, and file paths are workspace-relative globs. # # [[permissions.rules]] # tool = "exec_shell" diff --git a/crates/app-server/src/lib.rs b/crates/app-server/src/lib.rs index ff403703a..55874e161 100644 --- a/crates/app-server/src/lib.rs +++ b/crates/app-server/src/lib.rs @@ -262,7 +262,7 @@ async fn app_handler( fn build_state(config_path: Option) -> Result { let store = ConfigStore::load(config_path.clone())?; - let config = store.config.clone(); + let config = store.effective_config(); let registry = ModelRegistry::default(); let state_db_path = config_path diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index d8b316fdd..2cee2f38d 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -16,6 +16,7 @@ use serde::{Deserialize, Serialize}; use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; pub const CONFIG_FILE_NAME: &str = "config.toml"; +pub const PERMISSIONS_FILE_NAME: &str = "permissions.toml"; const DEFAULT_DEEPSEEK_MODEL: &str = "deepseek-v4-pro"; const DEFAULT_NVIDIA_NIM_MODEL: &str = "deepseek-ai/deepseek-v4-pro"; const DEFAULT_NVIDIA_NIM_FLASH_MODEL: &str = "deepseek-ai/deepseek-v4-flash"; @@ -458,6 +459,10 @@ impl ConfigToml { .with_rules(self.permissions.rules.clone()) } + pub fn append_permissions(&mut self, permissions: PermissionsToml) { + self.permissions.rules.extend(permissions.rules); + } + #[must_use] pub fn exec_policy_engine(&self) -> ExecPolicyEngine { ExecPolicyEngine::with_rulesets(vec![self.permission_ruleset()]) @@ -1408,26 +1413,26 @@ pub struct ResolvedRuntimeOptions { pub struct ConfigStore { path: PathBuf, pub config: ConfigToml, + permissions: PermissionsToml, } impl ConfigStore { pub fn load(path: Option) -> Result { let path = resolve_config_path(path)?; - if !path.exists() { - return Ok(Self { - path, - config: ConfigToml::default(), - }); - } - - let raw = fs::read_to_string(&path) - .with_context(|| format!("failed to read config at {}", path.display()))?; - let parsed: ConfigToml = toml::from_str(&raw) - .with_context(|| format!("failed to parse config at {}", path.display()))?; + let parsed = if path.exists() { + let raw = fs::read_to_string(&path) + .with_context(|| format!("failed to read config at {}", path.display()))?; + toml::from_str(&raw) + .with_context(|| format!("failed to parse config at {}", path.display()))? + } else { + ConfigToml::default() + }; + let permissions = load_sibling_permissions(&path)?; Ok(Self { path, config: parsed, + permissions, }) } @@ -1469,6 +1474,13 @@ impl ConfigStore { pub fn path(&self) -> &Path { &self.path } + + #[must_use] + pub fn effective_config(&self) -> ConfigToml { + let mut config = self.config.clone(); + config.append_permissions(self.permissions.clone()); + config + } } /// Process-wide default [`Secrets`] façade. The first caller wins; the @@ -1582,6 +1594,37 @@ pub fn resolve_config_path(explicit: Option) -> Result { normalize_config_file_path(path) } +pub fn permissions_path_for_config_path(config_path: &Path) -> PathBuf { + config_path.with_file_name(PERMISSIONS_FILE_NAME) +} + +pub fn resolve_permissions_path(config_path: Option) -> Result { + Ok(permissions_path_for_config_path(&resolve_config_path( + config_path, + )?)) +} + +fn load_sibling_permissions(config_path: &Path) -> Result { + let permissions_path = permissions_path_for_config_path(config_path); + if !permissions_path.exists() { + return Ok(PermissionsToml::default()); + } + + let raw = fs::read_to_string(&permissions_path).with_context(|| { + format!( + "failed to read permissions at {}", + permissions_path.display() + ) + })?; + let permissions: PermissionsToml = toml::from_str(&raw).with_context(|| { + format!( + "failed to parse permissions at {}", + permissions_path.display() + ) + })?; + Ok(permissions) +} + pub fn default_config_path() -> Result { // Prefer ~/.codewhale/config.toml when it exists (fresh install or // migrated), otherwise fall back to ~/.deepseek/config.toml. @@ -1939,6 +1982,52 @@ mod tests { assert_eq!(denied.requirement.phase(), "forbidden"); } + #[test] + fn config_store_loads_sibling_permissions_toml() { + use std::time::{SystemTime, UNIX_EPOCH}; + + let unique = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("clock") + .as_nanos(); + let dir = std::env::temp_dir().join(format!( + "codewhale-permissions-{}-{unique}", + std::process::id() + )); + fs::create_dir_all(&dir).expect("mkdir"); + let config_path = dir.join(CONFIG_FILE_NAME); + fs::write(&config_path, "auto_allow = [\"git status\"]\n").expect("write config"); + fs::write( + dir.join(PERMISSIONS_FILE_NAME), + r#" +[[rules]] +tool = "read_file" +decision = "ask" +path = "secrets/**" +"#, + ) + .expect("write permissions"); + + let store = ConfigStore::load(Some(config_path)).expect("load config store"); + + assert_eq!(store.config.auto_allow, ["git status"]); + assert!( + store.config.permissions.rules.is_empty(), + "sibling permissions must not be written back into config.toml" + ); + let effective = store.effective_config(); + assert_eq!( + effective.permissions.rules, + vec![ToolPermissionRule::file_path( + "read_file", + PermissionDecision::Ask, + "secrets/**" + )] + ); + + let _ = fs::remove_dir_all(dir); + } + struct EnvGuard { deepseek_api_key: Option, deepseek_base_url: Option, @@ -2456,6 +2545,7 @@ mod tests { api_key: Some("new-secret".to_string()), ..ConfigToml::default() }, + permissions: PermissionsToml::default(), }; store.save().expect("save"); diff --git a/crates/tui/src/commands/config.rs b/crates/tui/src/commands/config.rs index 5fa3ab59f..913b73c92 100644 --- a/crates/tui/src/commands/config.rs +++ b/crates/tui/src/commands/config.rs @@ -351,29 +351,27 @@ pub fn persist_permission_rules( ) -> anyhow::Result { use anyhow::Context; use std::fs; - use toml_edit::{ArrayOfTables, DocumentMut, Item, Table}; + use toml_edit::{ArrayOfTables, DocumentMut, Item}; - let path = config_toml_path()?; + let path = permissions_toml_path()?; if let Some(parent) = path.parent() { - fs::create_dir_all(parent) - .with_context(|| format!("failed to create config directory {}", parent.display()))?; + fs::create_dir_all(parent).with_context(|| { + format!( + "failed to create permissions directory {}", + parent.display() + ) + })?; } let mut doc: DocumentMut = if path.exists() { let raw = fs::read_to_string(&path) - .with_context(|| format!("failed to read config at {}", path.display()))?; + .with_context(|| format!("failed to read permissions at {}", path.display()))?; raw.parse() - .with_context(|| format!("failed to parse config at {}", path.display()))? + .with_context(|| format!("failed to parse permissions at {}", path.display()))? } else { DocumentMut::new() }; - let permissions_entry = doc - .entry("permissions") - .or_insert_with(|| Item::Table(Table::new())); - let permissions_table = permissions_entry - .as_table_mut() - .context("`permissions` section in config.toml must be a table")?; - let rules_entry = permissions_table + let rules_entry = doc .entry("rules") .or_insert_with(|| Item::ArrayOfTables(ArrayOfTables::new())); @@ -408,7 +406,7 @@ fn append_permission_rules( } toml_edit::Item::Value(value) => { let Some(array) = value.as_array_mut() else { - anyhow::bail!("`permissions.rules` in config.toml must be an array"); + anyhow::bail!("`rules` in permissions.toml must be an array"); }; for rule in rules { if !array @@ -419,7 +417,7 @@ fn append_permission_rules( } } } - _ => anyhow::bail!("`permissions.rules` in config.toml must be an array"), + _ => anyhow::bail!("`rules` in permissions.toml must be an array"), } Ok(()) } @@ -506,6 +504,10 @@ pub(super) fn config_toml_path() -> anyhow::Result { codewhale_config::resolve_config_path(None) } +pub(super) fn permissions_toml_path() -> anyhow::Result { + codewhale_config::resolve_permissions_path(None) +} + /// Modify a setting at runtime pub fn set_config_value(app: &mut App, key: &str, value: &str, persist: bool) -> CommandResult { let key = key.to_lowercase(); @@ -2157,7 +2159,7 @@ mod tests { } #[test] - fn persist_permission_rules_writes_deduped_rules_and_preserves_config() { + fn persist_permission_rules_writes_deduped_rules_to_permissions_toml() { let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() @@ -2174,9 +2176,11 @@ mod tests { fs::create_dir_all(path.parent().unwrap()).unwrap(); fs::write( &path, - "# user config header\napi_key = \"sentinel-key\" # keep inline\nmodel = \"deepseek-v4-pro\"\n\n[permissions]\n# user permission notes\n", + "# user config header\napi_key = \"sentinel-key\" # keep inline\nmodel = \"deepseek-v4-pro\"\n", ) .unwrap(); + let permissions_path = temp_root.join(".deepseek").join("permissions.toml"); + fs::write(&permissions_path, "# user permission notes\n").unwrap(); let cargo_rule = codewhale_execpolicy::ToolPermissionRule::exec_shell( codewhale_execpolicy::PermissionDecision::Allow, @@ -2191,29 +2195,37 @@ mod tests { persist_permission_rules(&[cargo_rule.clone(), cargo_rule.clone(), docs_rule.clone()]) .expect("persist should succeed"); - let body = fs::read_to_string(&written).expect("written file should be readable"); + assert_eq!(written, permissions_path); + + let config_body = fs::read_to_string(&path).expect("config file should be readable"); assert!( - body.contains("# user config header"), - "round-trip lost top-level comment: {body}" + config_body.contains("# user config header"), + "permission persistence should not rewrite config.toml: {config_body}" ); assert!( - body.contains("# keep inline"), - "round-trip lost inline comment: {body}" + config_body.contains("# keep inline"), + "permission persistence should not rewrite config.toml: {config_body}" ); assert!( - body.contains("# user permission notes"), - "round-trip lost permissions comment: {body}" + config_body.contains("api_key = \"sentinel-key\""), + "config.toml lost api_key: {config_body}" ); assert!( - body.contains("api_key = \"sentinel-key\""), - "round-trip lost api_key: {body}" + config_body.contains("model = \"deepseek-v4-pro\""), + "config.toml lost model: {config_body}" ); + + let body = fs::read_to_string(&written).expect("written file should be readable"); assert!( - body.contains("model = \"deepseek-v4-pro\""), - "round-trip lost model: {body}" + body.contains("# user permission notes"), + "round-trip lost permissions comment: {body}" + ); + assert!( + body.contains("[[rules]]"), + "expected top-level rules in {body}" ); - let config = Config::load(Some(written), None).expect("written config should load"); + let config = Config::load(Some(path), None).expect("config and permissions should load"); assert_eq!(config.permissions.rules, vec![cargo_rule, docs_rule]); } @@ -2244,7 +2256,10 @@ mod tests { ); let written = persist_permission_rules(&[rule]).expect("persist should succeed"); - assert_eq!(written, codewhale_path); + assert_eq!( + written, + temp_root.join(".codewhale").join("permissions.toml") + ); assert!(written.exists()); assert!( !temp_root.join(".deepseek").join("config.toml").exists(), diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index f4a769bc9..cfc2135bf 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -1311,6 +1311,7 @@ impl Config { Config::default() }; + apply_user_permissions_file(&mut config, path.as_deref())?; apply_env_overrides(&mut config); apply_managed_overrides(&mut config)?; apply_requirements(&mut config)?; @@ -3068,6 +3069,30 @@ fn load_single_config_file(path: &Path) -> Result { Ok(parsed.base) } +fn apply_user_permissions_file(config: &mut Config, config_path: Option<&Path>) -> Result<()> { + let Some(config_path) = config_path else { + return Ok(()); + }; + let permissions_path = codewhale_config::permissions_path_for_config_path(config_path); + if !permissions_path.exists() { + return Ok(()); + } + let contents = fs::read_to_string(&permissions_path).with_context(|| { + format!( + "Failed to read permissions file: {}", + permissions_path.display() + ) + })?; + let permissions: PermissionsConfig = toml::from_str(&contents).with_context(|| { + format!( + "Failed to parse permissions file: {}", + permissions_path.display() + ) + })?; + config.permissions.rules.extend(permissions.rules); + Ok(()) +} + fn apply_managed_overrides(config: &mut Config) -> Result<()> { let path = config .managed_config_path @@ -4136,6 +4161,41 @@ path = "secrets/**" Ok(()) } + #[test] + fn config_loads_sibling_permissions_toml_for_exec_policy_engine() -> Result<()> { + let _guard = lock_test_env(); + let temp = tempfile::tempdir()?; + let _env = EnvGuard::new(temp.path()); + let config_path = temp.path().join("config.toml"); + fs::write(&config_path, "auto_allow = [\"git status\"]\n")?; + fs::write( + temp.path().join("permissions.toml"), + r#" +[[rules]] +tool = "read_file" +decision = "ask" +path = "secrets/**" +"#, + )?; + + let config = Config::load(Some(config_path), None)?; + assert_eq!(config.auto_allow, ["git status"]); + + let decision = config.exec_policy_engine().check_tool_permission( + codewhale_execpolicy::ToolPermissionContext { + tool: "read_file", + command: None, + path: Some("secrets/token.txt"), + }, + ); + + assert_eq!( + decision.decision, + Some(codewhale_execpolicy::PermissionDecision::Ask) + ); + Ok(()) + } + #[test] fn save_api_key_writes_config_file_under_cfg_test() -> Result<()> { // `save_api_key` writes to the shared user config file. This diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index 0b5b60798..772549669 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -307,7 +307,7 @@ pub fn format_permission_rules_preview(rules: &[ToolPermissionRule]) -> String { fn format_permission_rule_preview(rule: &ToolPermissionRule) -> String { let mut lines = vec![ - "[[permissions.rules]]".to_string(), + "[[rules]]".to_string(), format!("tool = {}", toml_string(&rule.tool)), format!( "decision = {}", @@ -1571,7 +1571,7 @@ mod tests { let preview = request .permission_rule_preview() .expect("exec_shell should produce a preview"); - assert!(preview.contains("[[permissions.rules]]")); + assert!(preview.contains("[[rules]]")); assert!(preview.contains("tool = \"exec_shell\"")); assert!(preview.contains("decision = \"allow\"")); assert!(preview.contains("command = \"cargo test --workspace\""));