Merge pull request #62 from efecnc/feat/edit-approval-gate

Add approval gate for file mutations

Author: monatis

src/agent/mod.rs

@@ -413,6 +413,28 @@ fn shell_policy_mode_for_session(
     }
 }
 
+fn edit_policy_mode_for_session(
+    policy: &ResolvedShellPolicy,
+    unattended_session: bool,
+) -> ShellPolicyMode {
+    if unattended_session {
+        policy.unattended_edit_mode
+    } else {
+        policy.interactive_edit_mode
+    }
+}
+
+/// Reason shown when the edit policy blocks a mutation. Unattended sessions
+/// default to Deny independently of plan mode, so the message distinguishes the
+/// two cases rather than hardcoding "plan mode active" everywhere (PR #62 review #2).
+fn edit_policy_block_reason(unattended_session: bool) -> &'static str {
+    if unattended_session {
+        "File edit blocked by policy: unattended edit mode is active."
+    } else {
+        "File edit blocked by policy: plan mode active — finalize or apply the plan first."
+    }
+}
+
 fn command_preview(command: &str) -> String {
     const MAX_PREVIEW: usize = 160;
     if command.len() <= MAX_PREVIEW {
@@ -433,6 +455,11 @@ fn is_code_exec_tool(tool_name: &str) -> bool {
     )
 }
 
+/// Tools that mutate a workspace file and therefore need the edit policy gate.
+fn is_file_mutate_tool(tool_name: &str) -> bool {
+    matches!(tool_name, "write_file" | "edit_file")
+}
+
 /// Code-exec tools that run *arbitrary* code (Python source / session cells) where the
 /// destructive-shell-pattern heuristic does not meaningfully apply, so any such call is
 /// treated as approval-worthy in ask/deny mode.
@@ -478,6 +505,26 @@ mod code_exec_gate_tests {
         assert!(!is_code_exec_tool("web_search"));
     }
 
+    #[test]
+    fn file_mutate_category_covers_write_and_edit_only() {
+        assert!(is_file_mutate_tool("write_file"));
+        assert!(is_file_mutate_tool("edit_file"));
+        assert!(!is_file_mutate_tool("read_file"));
+        assert!(!is_file_mutate_tool("exec"));
+        assert!(!is_file_mutate_tool("list_dir"));
+        assert!(!is_file_mutate_tool("search_text"));
+    }
+
+    #[test]
+    fn edit_block_reason_distinguishes_unattended_and_plan_mode() {
+        // PR #62 review #2: the Deny message must match why the edit was blocked.
+        let unattended = edit_policy_block_reason(true);
+        assert!(unattended.contains("unattended"), "{unattended}");
+        assert!(!unattended.contains("plan mode"), "{unattended}");
+        let plan = edit_policy_block_reason(false);
+        assert!(plan.contains("plan mode"), "{plan}");
+    }
+
     #[test]
     fn extracts_command_for_exec_and_code_for_execution_tools() {
         assert_eq!(
@@ -754,6 +801,7 @@ async fn execute_tool_call_with_activity(
 
     with_tool_exec_and_progress_scope(tool_exec_ctx, progress_emitter, async move {
         let mut args = args;
+        let mut approved_mutation_preview = None;
         let activity_handle = tool_execution_activity
             .as_ref()
             .map(|a| a.start(chat_id.as_str(), tool_name.as_str()));
@@ -809,11 +857,14 @@ async fn execute_tool_call_with_activity(
                                 "timeout_secs": 1800,
                                 "allow_empty": false
                             });
+                            // System-initiated approval prompt: bypass the sub-agent tool
+                            // allowlist so a restricted sub-agent (e.g. allowlist={exec})
+                            // can still surface the approval dialog.
                             let ask_result = tools
                                 .execute_tool_scoped(
                                     "ask_user",
                                     ask_payload,
-                                    allow.as_deref(),
+                                    None,
                                     is_subagent,
                                 )
                                 .await;
@@ -892,18 +943,96 @@ async fn execute_tool_call_with_activity(
             }
         }
 
+        // Run this after steering hooks: a hook may rewrite the arguments, and the
+        // user must see the exact mutation that will be executed.
+        if is_file_mutate_tool(&tool_name) {
+            match edit_policy_mode_for_session(&runtime.shell_policy, runtime.unattended_session) {
+                ShellPolicyMode::Allow => {}
+                ShellPolicyMode::Deny => {
+                    return ToolExecutionFinished::Completed(Err(
+                        edit_policy_block_reason(runtime.unattended_session).to_string(),
+                    ));
+                }
+                ShellPolicyMode::Ask => {
+                    let preview = match tools
+                        .preview_mutation_scoped(&tool_name, &args, allow.as_deref(), is_subagent)
+                        .await
+                    {
+                        Ok(preview) => preview,
+                        // Invalid/no-op edits retain their ordinary tool result; there is no
+                        // mutation to approve in that case.
+                        Err(error) => {
+                            return ToolExecutionFinished::Completed(Err(format!(
+                                "Could not prepare edit approval: {error}"
+                            )));
+                        }
+                    };
+                    if let Some(preview) = preview {
+                        let ask_payload = serde_json::json!({
+                            "prompt": format!(
+                                "Approve edit to `{}`? Review the attached diff, then reply with approve or deny.",
+                                preview.path
+                            ),
+                            "choices": ["approve", "deny"],
+                            "timeout_secs": 1800,
+                            "allow_empty": false,
+                            "metadata": {
+                                "edit_diff": {
+                                    "file": preview.path,
+                                    "diff": preview.diff,
+                                    "truncated": preview.diff_truncated,
+                                }
+                            }
+                        });
+                        // System-initiated approval prompt: bypass the sub-agent tool
+                        // allowlist so a restricted sub-agent (e.g. allowlist={write_file})
+                        // can still surface the edit approval dialog.
+                        let reply = match tools
+                            .execute_tool_scoped(
+                                "ask_user",
+                                ask_payload,
+                                None,
+                                is_subagent,
+                            )
+                            .await
+                        {
+                            Ok(reply) => reply,
+                            Err(error) => {
+                                return ToolExecutionFinished::Completed(Err(format!(
+                                    "Edit policy approval failed: {error}"
+                                )));
+                            }
+                        };
+                        if !shell_approval_reply_is_grant(&reply) {
+                            return ToolExecutionFinished::Completed(Err(
+                                "Edit not approved by user; mutation skipped.".to_string(),
+                            ));
+                        }
+                        approved_mutation_preview = Some(preview);
+                    }
+                }
+            }
+        }
+
         let args_for_post = args.clone();
         let completed = match cancel_owned.as_ref() {
             None => Some(
                 tools
-                    .execute_tool_scoped(&tool_name, args, allow.as_deref(), is_subagent)
+                    .execute_tool_scoped_with_approved_mutation(
+                        &tool_name,
+                        args,
+                        approved_mutation_preview.as_ref(),
+                        allow.as_deref(),
+                        is_subagent,
+                    )
                     .await,
             ),
             Some(token) => {
                 tokio::select! {
-                    res = tools.execute_tool_scoped(
+                    res = tools.execute_tool_scoped_with_approved_mutation(
                         &tool_name,
                         args,
+                        approved_mutation_preview.as_ref(),
                         allow.as_deref(),
                         is_subagent,
                     ) => Some(res),
@@ -4257,6 +4386,8 @@ mod tests {
             shell_policy: Arc::new(crate::config::ResolvedShellPolicy {
                 interactive_mode: crate::config::ShellPolicyMode::Ask,
                 unattended_mode: crate::config::ShellPolicyMode::Deny,
+                interactive_edit_mode: crate::config::ShellPolicyMode::Ask,
+                unattended_edit_mode: crate::config::ShellPolicyMode::Deny,
                 approval_patterns: Vec::new(),
             }),
             hook_tool_ctx: None,
@@ -4344,6 +4475,8 @@ mod tests {
             shell_policy: crate::config::ResolvedShellPolicy {
                 interactive_mode: crate::config::ShellPolicyMode::Ask,
                 unattended_mode: crate::config::ShellPolicyMode::Deny,
+                interactive_edit_mode: crate::config::ShellPolicyMode::Ask,
+                unattended_edit_mode: crate::config::ShellPolicyMode::Deny,
                 approval_patterns: Vec::new(),
             },
             hook_tool_ctx: None,
@@ -4402,6 +4535,8 @@ mod tests {
             shell_policy: crate::config::ResolvedShellPolicy {
                 interactive_mode: crate::config::ShellPolicyMode::Ask,
                 unattended_mode: crate::config::ShellPolicyMode::Deny,
+                interactive_edit_mode: crate::config::ShellPolicyMode::Ask,
+                unattended_edit_mode: crate::config::ShellPolicyMode::Deny,
                 approval_patterns: Vec::new(),
             },
             hook_tool_ctx: None,

src/agent/subagent.rs

@@ -1325,6 +1325,8 @@ mod tests {
             shell_policy: Arc::new(ResolvedShellPolicy {
                 interactive_mode: crate::config::ShellPolicyMode::Ask,
                 unattended_mode: crate::config::ShellPolicyMode::Deny,
+                interactive_edit_mode: crate::config::ShellPolicyMode::Ask,
+                unattended_edit_mode: crate::config::ShellPolicyMode::Deny,
                 approval_patterns: Vec::new(),
             }),
             hook_tool_ctx: None,

src/config.rs

@@ -183,6 +183,10 @@ pub struct ShellPolicyConfig {
     pub unattended_default: Option<String>,
     /// Extra lowercase substrings that should require approval in `ask` mode.
     pub interactive_requires_approval_for: Option<Vec<String>>,
+    /// File edit mode: `ask`, `deny`, or `allow` (default `ask`).
+    pub edit_mode: Option<String>,
+    /// File edit mode for unattended/autonomous sessions (default `deny`).
+    pub edit_unattended_default: Option<String>,
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -196,6 +200,9 @@ pub enum ShellPolicyMode {
 pub struct ResolvedShellPolicy {
     pub interactive_mode: ShellPolicyMode,
     pub unattended_mode: ShellPolicyMode,
+    /// File mutation policy, intentionally independent from shell execution.
+    pub interactive_edit_mode: ShellPolicyMode,
+    pub unattended_edit_mode: ShellPolicyMode,
     pub approval_patterns: Vec<String>,
 }
 
@@ -753,6 +760,14 @@ impl AppConfig {
             shell_cfg.and_then(|s| s.unattended_default.as_deref()),
             ShellPolicyMode::Deny,
         );
+        let interactive_edit_mode = parse_shell_policy_mode(
+            shell_cfg.and_then(|s| s.edit_mode.as_deref()),
+            ShellPolicyMode::Ask,
+        );
+        let unattended_edit_mode = parse_shell_policy_mode(
+            shell_cfg.and_then(|s| s.edit_unattended_default.as_deref()),
+            ShellPolicyMode::Deny,
+        );
         let mut approval_patterns = vec![
             "rm -rf".to_string(),
             "rm -fr".to_string(),
@@ -778,6 +793,8 @@ impl AppConfig {
         ResolvedShellPolicy {
             interactive_mode,
             unattended_mode,
+            interactive_edit_mode,
+            unattended_edit_mode,
             approval_patterns,
         }
     }

src/tools.rs

@@ -1,4 +1,4 @@
-use crate::traits::Tool;
+use crate::traits::{MutationPreview, Tool};
 use serde_json::Value;
 use std::collections::{HashMap, HashSet};
 use std::sync::{Arc, RwLock};
@@ -116,6 +116,63 @@ impl ToolRegistry {
         }
     }
 
+    /// Ask a tool for its file-mutation preview without executing it.
+    pub async fn preview_mutation_scoped(
+        &self,
+        name: &str,
+        args: &Value,
+        allowlist: Option<&HashSet<String>>,
+        is_subagent: bool,
+    ) -> Result<Option<MutationPreview>, String> {
+        self.authorize_scoped(name, allowlist, is_subagent)?;
+        match self.get_tool(name) {
+            Some(tool) => tool.preview_mutation(args).await,
+            None => Err(format!("Tool '{}' not found", name)),
+        }
+    }
+
+    /// Execute with a preview previously approved by the user.
+    pub async fn execute_tool_scoped_with_approved_mutation(
+        &self,
+        name: &str,
+        args: Value,
+        approved_preview: Option<&MutationPreview>,
+        allowlist: Option<&HashSet<String>>,
+        is_subagent: bool,
+    ) -> Result<String, String> {
+        self.authorize_scoped(name, allowlist, is_subagent)?;
+        match self.get_tool(name) {
+            Some(tool) => {
+                tool.execute_with_approved_mutation(args, approved_preview)
+                    .await
+            }
+            None => Err(format!("Tool '{}' not found", name)),
+        }
+    }
+
+    fn authorize_scoped(
+        &self,
+        name: &str,
+        allowlist: Option<&HashSet<String>>,
+        is_subagent: bool,
+    ) -> Result<(), String> {
+        if is_subagent && Self::is_subagent_restricted_tool(name) {
+            return Err(format!(
+                "Tool '{}' is not available inside a sub-agent run",
+                name
+            ));
+        }
+        if let Some(set) = allowlist {
+            if !set.is_empty() && !set.contains(name) {
+                return Err(format!(
+                    "Tool '{}' is not allowed for this sub-agent (allowlist)",
+                    name
+                ));
+            }
+        }
+        Ok(())
+    }
+
     /// Tools that must not run inside a sub-agent loop (prevents unbounded recursion).
     pub fn is_subagent_restricted_tool(name: &str) -> bool {
         matches!(name, "subagent_spawn" | "subagent_plan_execute")
@@ -182,20 +239,7 @@ impl ToolRegistry {
         allowlist: Option<&HashSet<String>>,
         is_subagent: bool,
     ) -> Result<String, String> {
-        if is_subagent && Self::is_subagent_restricted_tool(name) {
-            return Err(format!(
-                "Tool '{}' is not available inside a sub-agent run",
-                name
-            ));
-        }
-        if let Some(set) = allowlist {
-            if !set.is_empty() && !set.contains(name) {
-                return Err(format!(
-                    "Tool '{}' is not allowed for this sub-agent (allowlist)",
-                    name
-                ));
-            }
-        }
+        self.authorize_scoped(name, allowlist, is_subagent)?;
         self.execute_tool(name, args).await
     }
 }
@@ -324,6 +368,31 @@ mod scoped_tools_tests {
         assert!(err.contains("not available"));
     }
 
+    #[tokio::test]
+    async fn system_initiated_ask_user_bypasses_subagent_allowlist() {
+        // Regression for PR #62 review feedback: system-initiated approval prompts
+        // (shell + edit policy gates) call ask_user with allowlist=None so a
+        // restricted sub-agent allowlist (e.g. {write_file} / {exec}) can still
+        // surface the approval dialog.
+        let mut r = ToolRegistry::new();
+        r.register(Box::new(NamedTool { n: "ask_user" }));
+        r.register(Box::new(NamedTool { n: "write_file" }));
+
+        // A sub-agent allowlisted to {write_file} (no ask_user) blocks a direct
+        // ask_user call — this is the failure the None-allowlist bypass avoids.
+        let allow: HashSet<String> = ["write_file".to_string()].into_iter().collect();
+        let err = r
+            .execute_tool_scoped("ask_user", Value::Null, Some(&allow), true)
+            .await
+            .unwrap_err();
+        assert!(err.contains("not allowed"), "{err}");
+
+        // With allowlist=None the system-initiated ask_user succeeds.
+        r.execute_tool_scoped("ask_user", Value::Null, None, true)
+            .await
+            .expect("system-initiated ask_user must bypass the allowlist");
+    }
+
     #[test]
     fn list_scoped_filters_allowlist_and_nested_tools() {
         let mut r = ToolRegistry::new();