From a326074043502a1f2967d8daebcec833b45fe836 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Thu, 18 Jun 2026 16:47:38 +0800 Subject: [PATCH 1/3] feat(tui): save ask rules from approvals --- crates/tui/src/tui/approval.rs | 114 ++++++++++++++++++++++++++++++ crates/tui/src/tui/ui.rs | 43 +++++++++++ crates/tui/src/tui/ui/tests.rs | 54 ++++++++++++++ crates/tui/src/tui/views/mod.rs | 2 + crates/tui/src/tui/widgets/mod.rs | 46 ++++++++++++ 5 files changed, 259 insertions(+) diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index c5167028d..f227e1369 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -30,6 +30,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_config::ToolAskRule; use crossterm::event::{KeyCode, KeyEvent}; use serde_json::Value; use std::path::{Path, PathBuf}; @@ -138,6 +139,8 @@ pub struct ApprovalRequest { /// Displayed in the approval view so users understand *why* the change /// is being made before reviewing *what* will change. pub intent_summary: Option, + /// Ask-only persistent rules that can be saved with the approval. + pub persistent_ask_rules: Vec, } /// Key approval details rendered prominently in the approval card. @@ -193,6 +196,7 @@ impl ApprovalRequest { Some(summary.to_string()) } }), + persistent_ask_rules: build_persistent_ask_rules(tool_name, params), } } @@ -218,6 +222,22 @@ impl ApprovalRequest { } } + #[must_use] + pub fn can_save_ask_rule(&self) -> bool { + !self.persistent_ask_rules.is_empty() + } + + #[must_use] + pub fn ask_rule_preview(&self) -> Option { + if self.persistent_ask_rules.is_empty() { + return None; + } + let permissions = codewhale_config::PermissionsToml { + rules: self.persistent_ask_rules.clone(), + }; + toml::to_string_pretty(&permissions).ok() + } + /// Extract the most important params for the approval card. #[must_use] pub fn prominent_detail_items(&self, locale: Locale) -> Vec { @@ -231,6 +251,22 @@ impl ApprovalRequest { } } +#[must_use] +fn build_persistent_ask_rules(tool_name: &str, params: &Value) -> Vec { + if tool_name != "exec_shell" { + return Vec::new(); + } + let Some(command) = params + .get("command") + .and_then(Value::as_str) + .map(str::trim) + .filter(|command| !command.is_empty()) + else { + return Vec::new(); + }; + vec![ToolAskRule::exec_shell(command)] +} + /// Get the category for a tool by name pub fn get_tool_category(name: &str) -> ToolCategory { if matches!(name, "write_file" | "edit_file" | "apply_patch") { @@ -888,6 +924,15 @@ impl ApprovalView { } 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_ask_rules: Vec, + ) -> ViewAction { ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { tool_id: self.request.id.clone(), tool_name: self.request.tool_name.clone(), @@ -895,6 +940,7 @@ impl ApprovalView { timed_out, approval_key: self.request.approval_key.clone(), approval_grouping_key: self.request.approval_grouping_key.clone(), + persistent_ask_rules, }) } @@ -947,6 +993,12 @@ impl ModalView for ApprovalView { KeyCode::Char('a') | KeyCode::Char('A') | KeyCode::Char('2') => { self.commit_option(ApprovalOption::ApproveAlways) } + KeyCode::Char('s') | KeyCode::Char('S') if self.request.can_save_ask_rule() => self + .emit_decision_with_rules( + ReviewDecision::Approved, + false, + self.request.persistent_ask_rules.clone(), + ), KeyCode::Char('n') | KeyCode::Char('N') | KeyCode::Char('d') @@ -1261,6 +1313,16 @@ mod tests { ) } + fn shell_request() -> ApprovalRequest { + ApprovalRequest::new( + "test-id", + "exec_shell", + "Run a shell command", + &json!({"command": "cargo test --workspace"}), + "tool:exec_shell", + ) + } + // ======================================================================== // Tool Category Tests // ======================================================================== @@ -1549,6 +1611,28 @@ mod tests { assert_eq!(view.risk(), RiskLevel::Benign); } + #[test] + fn exec_shell_request_builds_ask_rule_preview() { + let request = shell_request(); + + assert_eq!( + request.persistent_ask_rules, + vec![ToolAskRule::exec_shell("cargo test --workspace")] + ); + let preview = request.ask_rule_preview().expect("preview"); + assert!(preview.contains("[[rules]]")); + assert!(preview.contains("tool = \"exec_shell\"")); + assert!(preview.contains("command = \"cargo test --workspace\"")); + } + + #[test] + fn non_shell_request_has_no_persistent_ask_rules() { + let request = destructive_request(); + + assert!(request.persistent_ask_rules.is_empty()); + assert_eq!(request.ask_rule_preview(), None); + } + #[test] fn tab_toggles_collapsed_card_so_transcript_stays_visible() { // Regression for PR #1455 / @tiger-dog: the approval modal @@ -1609,6 +1693,36 @@ mod tests { } } + #[test] + fn save_ask_rule_shortcut_approves_once_with_rule() { + let mut view = ApprovalView::new(shell_request()); + + let action = view.handle_key(create_key_event(KeyCode::Char('s'))); + let ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision, + persistent_ask_rules, + .. + }) = action + else { + panic!("expected approval decision"); + }; + + assert_eq!(decision, ReviewDecision::Approved); + assert_eq!( + persistent_ask_rules, + vec![ToolAskRule::exec_shell("cargo test --workspace")] + ); + } + + #[test] + fn save_ask_rule_shortcut_is_ignored_without_rule() { + let mut view = ApprovalView::new(benign_request()); + + let action = view.handle_key(create_key_event(KeyCode::Char('s'))); + + assert!(matches!(action, ViewAction::None)); + } + #[test] fn benign_one_key_approves_via_numeric_pad() { let mut view = ApprovalView::new(benign_request()); diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 51a2e8738..ac904f52e 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -8664,10 +8664,12 @@ async fn handle_view_events( timed_out, approval_key, approval_grouping_key, + persistent_ask_rules, } => { apply_approval_decision( app, engine_handle, + config, ApprovalDecisionEvent { tool_id, tool_name, @@ -8675,6 +8677,7 @@ async fn handle_view_events( timed_out, approval_key, approval_grouping_key, + persistent_ask_rules, }, ) .await; @@ -9014,11 +9017,13 @@ struct ApprovalDecisionEvent { timed_out: bool, approval_key: String, approval_grouping_key: String, + persistent_ask_rules: Vec, } async fn apply_approval_decision( app: &mut App, engine_handle: &mut EngineHandle, + config: &mut Config, event: ApprovalDecisionEvent, ) { if event.decision == ReviewDecision::ApprovedForSession { @@ -9031,6 +9036,15 @@ async fn apply_approval_decision( .insert(event.approval_grouping_key.clone()); } + if matches!( + event.decision, + ReviewDecision::Approved | ReviewDecision::ApprovedForSession + ) && !event.persistent_ask_rules.is_empty() + && !event.timed_out + { + persist_ask_rules_from_approval(app, config, &event.persistent_ask_rules); + } + match event.decision { ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { let _ = engine_handle.approve_tool_call(event.tool_id).await; @@ -9053,6 +9067,35 @@ async fn apply_approval_decision( } } +fn persist_ask_rules_from_approval( + app: &mut App, + config: &mut Config, + rules: &[codewhale_config::ToolAskRule], +) { + match codewhale_config::ConfigStore::load(app.config_path.clone()).and_then(|mut store| { + let added = store.append_ask_rules(rules)?; + let permissions_path = store.permissions_path(); + config.exec_policy_engine = store.exec_policy_engine(); + Ok((added, permissions_path)) + }) { + Ok((added, path)) if added > 0 => { + app.status_message = Some(format!( + "Saved {added} ask permission rule(s) to {}", + path.display() + )); + } + Ok((_added, path)) => { + app.status_message = Some(format!( + "Ask permission rule already saved in {}", + path.display() + )); + } + Err(err) => { + app.status_message = Some(format!("Failed to save ask permission rule: {err:#}")); + } + } +} + fn mark_active_turn_cancelled_locally(app: &mut App) { // #2739: every local cancel surface (Esc, Ctrl+C, approval abort, paused // command abort) must snapshot before it clears turn state. Otherwise diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index e6e90a587..3ba4513b7 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -8546,6 +8546,60 @@ fn approval_prompt_uses_event_input_after_message_complete_drain() { assert_ne!(content.trim(), "{}"); } +#[tokio::test] +async fn approval_decision_persists_ask_rules_to_permissions_file() { + let tmp = TempDir::new().expect("tempdir"); + let config_path = tmp.path().join("config.toml"); + let mut app = create_test_app(); + app.config_path = Some(config_path.clone()); + let mut config = Config::default(); + let mut engine = mock_engine_handle(); + let rule = codewhale_config::ToolAskRule::exec_shell("cargo test"); + + apply_approval_decision( + &mut app, + &mut engine.handle, + &mut config, + ApprovalDecisionEvent { + tool_id: "tool-1".to_string(), + tool_name: "exec_shell".to_string(), + decision: ReviewDecision::Approved, + timed_out: false, + approval_key: "approval-key".to_string(), + approval_grouping_key: "approval-group".to_string(), + persistent_ask_rules: vec![rule.clone()], + }, + ) + .await; + + assert_eq!( + engine.recv_approval_event().await, + Some(crate::core::engine::MockApprovalEvent::Approved { + id: "tool-1".to_string() + }) + ); + let store = codewhale_config::ConfigStore::load(Some(config_path)).expect("load config store"); + assert_eq!(store.permissions().rules, vec![rule]); + assert!( + app.status_message + .as_deref() + .is_some_and(|message| message.contains("Saved 1 ask permission rule")) + ); + + let decision = config + .exec_policy_engine + .check(codewhale_execpolicy::ExecPolicyContext { + command: "cargo test --workspace", + cwd: tmp.path().to_string_lossy().as_ref(), + tool: Some("exec_shell"), + path: None, + ask_for_approval: codewhale_execpolicy::AskForApproval::OnFailure, + sandbox_mode: None, + }) + .expect("check persisted runtime policy"); + assert!(decision.requires_approval); +} + #[test] fn second_thinking_block_appends_new_entry_in_same_active_cell() { // Real V4 turns can emit Thinking → Tool → Thinking → Tool before any diff --git a/crates/tui/src/tui/views/mod.rs b/crates/tui/src/tui/views/mod.rs index 35c293a2f..4e2e5494c 100644 --- a/crates/tui/src/tui/views/mod.rs +++ b/crates/tui/src/tui/views/mod.rs @@ -106,6 +106,8 @@ pub enum ViewEvent { approval_key: String, /// Lossy / arity-aware fingerprint, used to scope *approvals*. approval_grouping_key: String, + /// Ask-only permission rules to append when the decision approves. + persistent_ask_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 f388af4d8..60286b76c 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -1354,6 +1354,30 @@ impl Renderable for ApprovalWidget<'_> { } } + if let Some(preview) = self.request.ask_rule_preview() { + lines.push(Line::from("")); + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + label_ask_rule_preview(locale), + Style::default().fg(palette::TEXT_HINT), + ), + ])); + let max_width = card_area.width.saturating_sub(6) as usize; + for line in preview + .lines() + .filter(|line| !line.trim().is_empty()) + .take(4) + { + let truncated = + crate::utils::truncate_with_ellipsis(line.trim(), max_width.max(20), "..."); + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled(truncated, Style::default().fg(palette::TEXT_SECONDARY)), + ])); + } + } + lines.push(Line::from("")); let options = approval_options_for(risk, locale); @@ -1399,6 +1423,14 @@ impl Renderable for ApprovalWidget<'_> { footer_controls(locale), Style::default().fg(palette::TEXT_HINT), ), + if self.request.can_save_ask_rule() { + Span::styled( + save_ask_rule_hint(locale), + Style::default().fg(palette_colors.shortcut), + ) + } else { + Span::raw("") + }, ])); let title = format!( @@ -1602,6 +1634,20 @@ fn footer_controls(locale: Locale) -> &'static str { tr(locale, MessageId::ApprovalControlsHint) } +fn save_ask_rule_hint(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => " s 批准并保存询问规则", + _ => " s approve + save ask rule", + } +} + +fn label_ask_rule_preview(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "询问规则预览：", + _ => "Ask rule preview:", + } +} + fn selection_hint_prefix(locale: Locale) -> &'static str { tr(locale, MessageId::ApprovalChooseHint) } From e32c2456600e843742533a8313f8ba3b0ec0801d Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Thu, 18 Jun 2026 17:18:19 +0800 Subject: [PATCH 2/3] fix(ci): stabilize verifier and provider checks --- crates/tui/src/config.rs | 17 +++++++++++------ crates/tui/src/tools/verifier.rs | 30 ++++++++++++++++++++++++------ 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 179f8a8e1..9b2e14baa 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -5525,12 +5525,6 @@ pub fn active_provider_has_config_api_key(config: &Config) -> bool { // active_provider_has_env_api_key. return crate::oauth::auth_file_path().exists(); } - if matches!(provider, ApiProvider::Huggingface) - && std::env::var("HF_TOKEN").is_ok_and(|k| !k.trim().is_empty()) - { - return true; - } - if config .provider_config_string_with_runtime_fallback(provider, |entry| entry.api_key.clone()) .is_some_and(|k| !k.trim().is_empty() && k != API_KEYRING_SENTINEL) @@ -5736,6 +5730,17 @@ fn provider_config_table_name(provider: ApiProvider) -> Result { } fn provider_env_api_key(provider: ApiProvider) -> Option { + if provider == ApiProvider::Huggingface { + return std::env::var("HUGGINGFACE_API_KEY") + .ok() + .filter(|value| !value.trim().is_empty()) + .or_else(|| { + std::env::var("HF_TOKEN") + .ok() + .filter(|value| !value.trim().is_empty()) + }); + } + provider.env_vars().iter().find_map(|var| { std::env::var(var) .ok() diff --git a/crates/tui/src/tools/verifier.rs b/crates/tui/src/tools/verifier.rs index 3e452f594..db2293a85 100644 --- a/crates/tui/src/tools/verifier.rs +++ b/crates/tui/src/tools/verifier.rs @@ -1122,8 +1122,28 @@ fn char_boundary_index(text: &str, max_chars: usize) -> usize { mod tests { use super::*; use crate::tools::shell::ShellStatus; + use std::time::Duration; use tempfile::tempdir; + const BACKGROUND_COMPLETION_WAIT_MS: u64 = 30_000; + + fn wait_for_completed_shell( + manager: &mut crate::tools::shell::ShellManager, + task_id: &str, + ) -> crate::tools::shell::ShellResult { + let deadline = Instant::now() + Duration::from_millis(BACKGROUND_COMPLETION_WAIT_MS); + + loop { + let result = manager + .get_output(task_id, true, 1_000) + .expect("background output"); + if result.status != ShellStatus::Running || Instant::now() >= deadline { + return result; + } + std::thread::sleep(Duration::from_millis(50)); + } + } + #[test] fn run_verifiers_requires_user_approval() { let tool = RunVerifiersTool; @@ -1316,12 +1336,10 @@ mod tests { Some("nonblocking") ); - let output = ctx - .shell_manager - .lock() - .expect("shell manager") - .get_output(task_id, true, 10_000) - .expect("background output"); + let output = wait_for_completed_shell( + &mut ctx.shell_manager.lock().expect("shell manager"), + task_id, + ); assert_eq!(output.status, ShellStatus::Completed); assert!( output.stdout.contains("rustc"), From 675f63bf08b6511ec1e8043a6dd019ab5a3bdbdc Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Thu, 18 Jun 2026 18:10:39 +0800 Subject: [PATCH 3/3] fix(tests): isolate settings homes in ui tests --- crates/tui/src/tui/ui/tests.rs | 100 ++++++++++++++------------------- 1 file changed, 43 insertions(+), 57 deletions(-) diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 3ba4513b7..5aee47292 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -78,6 +78,11 @@ struct SettingsHomeGuard { _tmp: TempDir, previous_home: Option, previous_userprofile: Option, + previous_codewhale_home: Option, + previous_deepseek_config_path: Option, + previous_xdg_config_home: Option, + previous_appdata: Option, + previous_localappdata: Option, _lock: MutexGuard<'static, ()>, } @@ -87,15 +92,31 @@ impl SettingsHomeGuard { let tmp = TempDir::new().expect("settings tempdir"); let previous_home = std::env::var_os("HOME"); let previous_userprofile = std::env::var_os("USERPROFILE"); + let previous_codewhale_home = std::env::var_os("CODEWHALE_HOME"); + let previous_deepseek_config_path = std::env::var_os("DEEPSEEK_CONFIG_PATH"); + let previous_xdg_config_home = std::env::var_os("XDG_CONFIG_HOME"); + let previous_appdata = std::env::var_os("APPDATA"); + let previous_localappdata = std::env::var_os("LOCALAPPDATA"); + let codewhale_home = tmp.path().join(".codewhale"); // Safety: test-only environment mutation guarded by a global mutex. unsafe { std::env::set_var("HOME", tmp.path()); std::env::set_var("USERPROFILE", tmp.path()); + std::env::set_var("CODEWHALE_HOME", &codewhale_home); + std::env::set_var("DEEPSEEK_CONFIG_PATH", codewhale_home.join("config.toml")); + std::env::set_var("XDG_CONFIG_HOME", tmp.path().join("xdg-config")); + std::env::set_var("APPDATA", tmp.path().join("appdata")); + std::env::set_var("LOCALAPPDATA", tmp.path().join("localappdata")); } Self { _tmp: tmp, previous_home, previous_userprofile, + previous_codewhale_home, + previous_deepseek_config_path, + previous_xdg_config_home, + previous_appdata, + previous_localappdata, _lock: lock, } } @@ -103,17 +124,26 @@ impl SettingsHomeGuard { impl Drop for SettingsHomeGuard { fn drop(&mut self) { - // Safety: test-only environment mutation guarded by a global mutex. - unsafe { - match self.previous_home.take() { - Some(previous) => std::env::set_var("HOME", previous), - None => std::env::remove_var("HOME"), - } - match self.previous_userprofile.take() { - Some(previous) => std::env::set_var("USERPROFILE", previous), - None => std::env::remove_var("USERPROFILE"), + fn restore(key: &str, previous: Option) { + // Safety: test-only environment mutation guarded by a global mutex. + unsafe { + match previous { + Some(previous) => std::env::set_var(key, previous), + None => std::env::remove_var(key), + } } } + + restore("HOME", self.previous_home.take()); + restore("USERPROFILE", self.previous_userprofile.take()); + restore("CODEWHALE_HOME", self.previous_codewhale_home.take()); + restore( + "DEEPSEEK_CONFIG_PATH", + self.previous_deepseek_config_path.take(), + ); + restore("XDG_CONFIG_HOME", self.previous_xdg_config_home.take()); + restore("APPDATA", self.previous_appdata.take()); + restore("LOCALAPPDATA", self.previous_localappdata.take()); } } @@ -2457,54 +2487,10 @@ fn provider_picker_reselecting_active_provider_preserves_current_model() { #[tokio::test] async fn provider_switch_clears_turn_cache_history() { // `switch_provider` persists the new provider to `Settings`, which - // writes through `dirs::data_dir()` (`~/Library/Application - // Support/deepseek/settings.toml` on macOS). Without redirecting - // HOME / USERPROFILE we would clobber the developer's real - // preferences and leave `default_provider = "ollama"` behind — - // which then leaks into any subsequent test that constructs an - // `App`. Hold the process-wide env lock for the duration so we - // serialize with other tests that mutate the same env vars. - // Wrap the lock inside a guard struct so clippy's - // `await_holding_lock` doesn't fire on the `.await` below; the - // pattern matches other tests that guard HOME / USERPROFILE mutations. - struct HomeGuard { - _tmp: tempfile::TempDir, - prev_home: Option, - prev_userprofile: Option, - _lock: std::sync::MutexGuard<'static, ()>, - } - impl Drop for HomeGuard { - fn drop(&mut self) { - // SAFETY: still holding the process-wide env lock. - unsafe { - match self.prev_home.take() { - Some(v) => std::env::set_var("HOME", v), - None => std::env::remove_var("HOME"), - } - match self.prev_userprofile.take() { - Some(v) => std::env::set_var("USERPROFILE", v), - None => std::env::remove_var("USERPROFILE"), - } - } - } - } - let _home = { - let lock = crate::test_support::lock_test_env(); - let tmp = tempfile::TempDir::new().expect("tempdir"); - let prev_home = std::env::var_os("HOME"); - let prev_userprofile = std::env::var_os("USERPROFILE"); - // SAFETY: serialized by the process-wide test env lock. - unsafe { - std::env::set_var("HOME", tmp.path()); - std::env::set_var("USERPROFILE", tmp.path()); - } - HomeGuard { - _tmp: tmp, - prev_home, - prev_userprofile, - _lock: lock, - } - }; + // writes through settings path resolution. Without redirecting the + // CodeWhale/legacy config homes we would clobber the developer's real + // preferences and leave `default_provider = "ollama"` behind. + let _home = SettingsHomeGuard::new(); let mut app = create_test_app(); app.push_turn_cache_record(crate::tui::app::TurnCacheRecord {