fix(agent): address edit-approval-gate review feedback

- Bypass the sub-agent tool allowlist for system-initiated ask_user prompts
  in both the edit and shell policy gates. A restricted sub-agent allowlist
  (e.g. {write_file} / {exec}) that omits ask_user previously caused the
  approval prompt itself to fail.
- Make the edit-policy Deny message context-aware (unattended vs plan mode)
  instead of hardcoding 'plan mode active', which is misleading for the
  unattended default-deny.
- Skip the approval prompt for no-op edits (old_text == new_text) and no-op
  writes (content identical to the existing file).
- Add regression tests covering each fix.

Addresses #62 review comments.

Author: efecnc

src/agent/mod.rs

@@ -424,6 +424,17 @@ fn edit_policy_mode_for_session(
     }
 }
 
+/// 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 {
@@ -494,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!(
@@ -826,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;
@@ -916,8 +950,7 @@ async fn execute_tool_call_with_activity(
                 ShellPolicyMode::Allow => {}
                 ShellPolicyMode::Deny => {
                     return ToolExecutionFinished::Completed(Err(
-                        "File edit blocked by policy: plan mode active — finalize or apply the plan first."
-                            .to_string(),
+                        edit_policy_block_reason(runtime.unattended_session).to_string(),
                     ));
                 }
                 ShellPolicyMode::Ask => {
@@ -951,11 +984,14 @@ async fn execute_tool_call_with_activity(
                                 }
                             }
                         });
+                        // 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,
-                                allow.as_deref(),
+                                None,
                                 is_subagent,
                             )
                             .await

src/tools.rs

@@ -368,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();

src/tools/builtin.rs

@@ -361,7 +361,14 @@ impl Tool for WriteFileTool {
             .ok_or("Missing 'content' argument")?;
         let actual_path = resolve_path(path_str, &self.workspace_dir, self.restrict_to_workspace)?;
         let before = match fs::read_to_string(&actual_path) {
-            Ok(content) => Some(content),
+            Ok(current_content) => {
+                // No-op write: the file already holds the exact content. Skip the
+                // approval prompt — there is no mutation to review.
+                if current_content == content {
+                    return Ok(None);
+                }
+                Some(current_content)
+            }
             Err(error) if error.kind() == std::io::ErrorKind::NotFound => None,
             Err(error) => return Err(format!("Could not preview write target: {error}")),
         };
@@ -515,6 +522,12 @@ impl Tool for EditFileTool {
             .get("replace_all")
             .and_then(|v| v.as_bool())
             .unwrap_or(false);
+        // No-op edit: identical old/new text produces an empty diff. Skip the
+        // approval prompt — there is no mutation to review. (execute() separately
+        // returns an "identical" error so the model learns the edit was a no-op.)
+        if old_text == new_text {
+            return Ok(None);
+        }
         let actual_path = resolve_path(path_str, &self.workspace_dir, self.restrict_to_workspace)?;
         let before = fs::read_to_string(&actual_path)
             .map_err(|error| format!("Could not preview edit target: {error}"))?;
@@ -2717,6 +2730,47 @@ mod mutation_preview_tests {
         assert!(preview.diff.contains("+gamma"), "{}", preview.diff);
         let _ = fs::remove_dir_all(root);
     }
+
+    #[tokio::test]
+    async fn write_preview_skips_noop_when_content_unchanged() {
+        // PR #62 review #4: writing identical content is a no-op; skip the prompt.
+        let root = workspace();
+        fs::write(root.join("same.txt"), "identical\n").unwrap();
+        let tool = WriteFileTool {
+            workspace_dir: root.clone(),
+            restrict_to_workspace: true,
+        };
+        let args = json!({ "path": "same.txt", "content": "identical\n" });
+        assert!(
+            tool.preview_mutation(&args).await.unwrap().is_none(),
+            "identical-content write must be a no-op preview"
+        );
+        // A genuinely different write still produces a preview.
+        let changed = json!({ "path": "same.txt", "content": "new\n" });
+        assert!(tool.preview_mutation(&changed).await.unwrap().is_some());
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[tokio::test]
+    async fn edit_preview_skips_noop_when_old_equals_new() {
+        // PR #62 review #3: identical old/new text is a no-op; skip the prompt.
+        let root = workspace();
+        fs::write(root.join("notes.txt"), "alpha\nbeta\n").unwrap();
+        let tool = EditFileTool {
+            workspace_dir: root.clone(),
+            restrict_to_workspace: true,
+        };
+        let args = json!({
+            "path": "notes.txt",
+            "old_text": "beta",
+            "new_text": "beta"
+        });
+        assert!(
+            tool.preview_mutation(&args).await.unwrap().is_none(),
+            "identical old/new text must be a no-op preview"
+        );
+        let _ = fs::remove_dir_all(root);
+    }
 }
 
 #[cfg(test)]