From 59d2a6445071e6c9c4adb04f1d4ee7ded8c081a1 Mon Sep 17 00:00:00 2001 From: greyfreedom Date: Wed, 24 Jun 2026 17:31:25 +0800 Subject: [PATCH] feat(tui): save exact file ask rules from write approvals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the approval modal's "save ask rule" action (previously exec_shell only) to write_file and edit_file. Pressing `S` on a write approval now persists an exact, workspace-relative ToolAskRule::file_path so a later edit/write of the same file is matched. The saved path is normalized through the same codewhale_execpolicy::normalize_workspace_relative_path helper that runtime ask-rule matching uses, so the stored rule equals what matching compares against later. When the path is empty, traversing, drive-relative, or outside the workspace, no rule is built: the preview stays hidden and the `S` shortcut stays disabled. read_file is intentionally excluded — this boundary is about persisting write approvals only. Persistence reuses the existing ConfigStore::append_ask_rules flow (dedup, atomic write, permission handling). No new persistence, directory rules, globs, allow/deny typing, or apply_patch support is added, and YOLO mode is unaffected (approvals are bypassed before the modal is built). Refs #1186 (partial) Refs #2242 (partial) Validation: - cargo fmt --all - git diff --check - cargo test -p codewhale-execpolicy --locked - cargo test -p codewhale-tui --bin codewhale-tui --locked file_ask_rule --- crates/execpolicy/src/lib.rs | 8 +- crates/tui/src/tui/approval.rs | 145 ++++++++++++++++++++++++++++-- crates/tui/src/tui/ui.rs | 1 + crates/tui/src/tui/widgets/mod.rs | 1 + 4 files changed, 148 insertions(+), 7 deletions(-) diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index ea829251e4..0c3b8a7c31 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -509,7 +509,13 @@ fn first_token(command: &str) -> String { /// must have the workspace as a whole-component prefix; relative paths are /// interpreted as workspace-relative. Backslashes are accepted so persisted /// rules and tool inputs behave consistently on Windows. -fn normalize_workspace_relative_path(value: &str, workspace_root: &str) -> Option { +/// +/// This is the canonical normalization shared by ask-rule matching and rule +/// persistence: callers that save a file ask rule should store the value this +/// returns so the saved path matches the same invocation later. `None` means +/// the path is empty, traversing, drive-relative, or outside the workspace and +/// must not be turned into a rule. +pub fn normalize_workspace_relative_path(value: &str, workspace_root: &str) -> Option { let path = parse_path_for_matching(value)?; let workspace = parse_path_for_matching(workspace_root)?; let workspace_root = workspace.root.as_ref()?; diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index 601fe9361a..f53aa9e063 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -162,7 +162,15 @@ impl ApprovalRequest { params: &Value, approval_key: &str, ) -> Self { - Self::new_with_intent(id, tool_name, description, params, approval_key, None) + Self::new_with_intent( + id, + tool_name, + description, + params, + approval_key, + None, + Path::new("/workspace"), + ) } pub fn new_with_intent( @@ -172,6 +180,7 @@ impl ApprovalRequest { params: &Value, approval_key: &str, intent_summary: Option<&str>, + workspace: &Path, ) -> Self { let category = get_tool_category(tool_name); let risk = classify_risk(tool_name, category, params); @@ -196,7 +205,7 @@ impl ApprovalRequest { Some(summary.to_string()) } }), - persistent_ask_rules: build_persistent_ask_rules(tool_name, params), + persistent_ask_rules: build_persistent_ask_rules(tool_name, params, workspace), } } @@ -252,10 +261,23 @@ impl ApprovalRequest { } #[must_use] -fn build_persistent_ask_rules(tool_name: &str, params: &Value) -> Vec { - if tool_name != "exec_shell" { - return Vec::new(); +fn build_persistent_ask_rules( + tool_name: &str, + params: &Value, + workspace: &Path, +) -> Vec { + match tool_name { + "exec_shell" => build_exec_shell_ask_rules(params), + // File writes save an exact, workspace-relative path so a later + // edit/write of the same file is matched. read_file stays out: this + // boundary is about persisting *write* approvals only. + "write_file" | "edit_file" => build_file_write_ask_rules(tool_name, params, workspace), + _ => Vec::new(), } +} + +#[must_use] +fn build_exec_shell_ask_rules(params: &Value) -> Vec { let Some(command) = params .get("command") .and_then(Value::as_str) @@ -267,6 +289,35 @@ fn build_persistent_ask_rules(tool_name: &str, params: &Value) -> Vec Vec { + let Some(path) = params + .get("path") + .and_then(Value::as_str) + .map(str::trim) + .filter(|path| !path.is_empty()) + else { + return Vec::new(); + }; + // Reuse the canonical matcher normalization so the saved rule equals what + // runtime matching compares against. `None` (and the degenerate + // workspace-root case) means the path is empty, traversing, drive-relative, + // or outside the workspace, so we save nothing and the `S` shortcut and + // preview stay disabled. + let workspace = workspace.to_string_lossy(); + let Some(relative) = + codewhale_execpolicy::normalize_workspace_relative_path(path, workspace.as_ref()) + .filter(|relative| !relative.is_empty()) + else { + return Vec::new(); + }; + vec![ToolAskRule::file_path(tool_name, relative)] +} + /// 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") { @@ -1634,13 +1685,72 @@ mod tests { } #[test] - fn non_shell_request_has_no_persistent_ask_rules() { + fn file_ask_rule_saved_for_write_file_approval() { + // A write_file approval offers an exact, workspace-relative file rule + // plus a preview so `S` can persist it. let request = destructive_request(); + assert_eq!( + request.persistent_ask_rules, + vec![ToolAskRule::file_path("write_file", "src/main.rs")] + ); + assert!(request.can_save_ask_rule()); + let preview = request.ask_rule_preview().expect("preview"); + assert!(preview.contains("[[rules]]")); + assert!(preview.contains("tool = \"write_file\"")); + assert!(preview.contains("path = \"src/main.rs\"")); + } + + #[test] + fn file_ask_rule_normalizes_absolute_edit_file_path_to_workspace_relative() { + // An absolute in-workspace path is stored in the workspace-relative + // form, matching how runtime ask-rule matching normalizes paths. + let request = ApprovalRequest::new( + "test-id", + "edit_file", + "Edit a file on disk", + &json!({"path": "/workspace/src/lib.rs"}), + "tool:edit_file", + ); + + assert_eq!( + request.persistent_ask_rules, + vec![ToolAskRule::file_path("edit_file", "src/lib.rs")] + ); + } + + #[test] + fn read_file_request_has_no_file_ask_rule() { + // The save boundary is write approvals only; read_file never offers a + // persistent rule. + let request = benign_request(); + assert!(request.persistent_ask_rules.is_empty()); + assert!(!request.can_save_ask_rule()); assert_eq!(request.ask_rule_preview(), None); } + #[test] + fn file_ask_rule_skipped_for_unsafe_empty_or_external_paths() { + // Traversal, empty, and outside-workspace paths must not become rules, + // so the preview and `S` shortcut stay disabled. + for path in ["../escape.rs", "/etc/passwd", " ", ""] { + let request = ApprovalRequest::new( + "test-id", + "write_file", + "Write a file to disk", + &json!({"path": path}), + "tool:write_file", + ); + assert!( + request.persistent_ask_rules.is_empty(), + "path {path:?} must not produce a rule" + ); + assert!(!request.can_save_ask_rule()); + 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 @@ -1722,6 +1832,29 @@ mod tests { ); } + #[test] + fn save_file_ask_rule_shortcut_emits_file_rule() { + // `S` on a write_file approval approves once and carries the exact + // workspace-relative file rule for persistence. + let mut view = ApprovalView::new(destructive_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::file_path("write_file", "src/main.rs")] + ); + } + #[test] fn save_ask_rule_shortcut_is_ignored_without_rule() { let mut view = ApprovalView::new(benign_request()); diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 7f78223d46..afc94176c7 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -9172,6 +9172,7 @@ fn push_approval_request_view( tool_input, approval_key, intent_summary, + &app.workspace, ); app.view_stack .push(ApprovalView::new_for_locale(request, app.ui_locale)); diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index 447afa76ed..384ba15ee2 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -4668,6 +4668,7 @@ mod tests { }), "exec_shell:cargo", Some("Need to verify the fallback build path before editing files."), + std::path::Path::new("/tmp/project"), ); let view = crate::tui::approval::ApprovalView::new(request.clone()); let widget = ApprovalWidget::new(&request, &view);