fix(tools): ToolExecutor forwarding gaps, symlinked checkpoint drop, checkpoint tests

CHANGELOG.md

@@ -69,6 +69,41 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Fixed
 
+- `fix(tools)`: `CompressedExecutor`, `ToolFilter`, and `Arc<ShellExecutor>` now forward the
+  remaining cross-cutting `ToolExecutor` methods to their inner/wrapped executor instead of
+  silently falling through to the trait's no-op defaults (#6012). `CompressedExecutor` now
+  forwards `requires_confirmation` and the `checkpoint_undo`/`checkpoint_redo`/`checkpoint_list`
+  trio. `ToolFilter` (wrapping the ACP `FileExecutor`) previously forwarded none of the
+  cross-cutting methods — it now forwards `execute_tool_call_confirmed` (respecting tool
+  suppression), `set_skill_env`, `set_effective_trust`, `is_tool_retryable`,
+  `is_tool_speculatable`, `requires_confirmation`, and the checkpoint trio. `Arc<ShellExecutor>`
+  now also forwards `execute_confirmed`, `execute_tool_call_confirmed`, `set_effective_trust`,
+  `is_tool_retryable`, `is_tool_speculatable`, and `requires_confirmation` — the
+  `execute_confirmed` forward closes a currently-dormant gap (its only caller today,
+  `handle_confirmation_required` in `tool_result.rs`, is `#[cfg(test)]`-gated; production
+  confirmation dispatch goes through `execute_tool_call_confirmed` via `tier_loop.rs` instead)
+  but is worth fixing now as defense-in-depth, matching the pattern of every other wrapper, in
+  case that path is ever re-enabled. Same defect class as #5899/#5905/#5906 (fixed by #5930) and
+  #5900/#5938/#5931 (fixed by #6011).
+- `fix(tools)`: `capture_snapshot_for` no longer silently drops a checkpoint for a
+  newly-created file whose path lives under a symlinked `allowed_paths` prefix on macOS (e.g.
+  `/tmp` -> `/private/tmp`, `/var` -> `/private/var`) (#5999). For a file that does not exist
+  yet, `canonicalize()` fails, and the previous fallback (`std::path::absolute`) does not
+  resolve symlinks, so the file's path stayed under the raw prefix while `allowed_paths`
+  (canonicalized at construction time) held the resolved prefix — the containment check failed
+  and the checkpoint was dropped with only a `tracing::warn!`. Both `capture_snapshot_for` and
+  `validate_sandbox_with_cwd` now share a new `canonicalize_or_nearest_ancestor` helper that
+  walks up to the nearest existing ancestor, canonicalizes it, and reattaches the non-existent
+  suffix.
+
+### Testing
+
+- `test(tools)`: added regression coverage for the `ShellExecutor` checkpoint stack (#6001):
+  `checkpoint_redo` with no prior `checkpoint_undo` (no-op "Nothing to redo.", not a panic),
+  `checkpoint_list` ordering with 3 recorded checkpoints (most-recent-first, matching the
+  `index` field), and a multi-step undo/redo/undo sequence pinning undo-stack depth
+  bookkeeping.
+
 - `fix(tools)`: `CompositeExecutor`, `AdversarialPolicyGateExecutor`, and `PolicyGateExecutor`
   now forward `requires_confirmation`/`is_tool_speculatable`/`execute_tool_call_confirmed` to
   their inner executors instead of silently falling through to the `ToolExecutor` trait's

crates/zeph-tools/src/compression/decorator.rs

@@ -150,6 +150,22 @@ impl<E: ToolExecutor> ToolExecutor for CompressedExecutor<E> {
     fn is_tool_speculatable(&self, tool_id: &str) -> bool {
         self.inner.is_tool_speculatable(tool_id)
     }
+
+    fn requires_confirmation(&self, call: &ToolCall) -> bool {
+        self.inner.requires_confirmation(call)
+    }
+
+    fn checkpoint_undo(&self, n: usize) -> crate::executor::CheckpointActionResult {
+        self.inner.checkpoint_undo(n)
+    }
+
+    fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult {
+        self.inner.checkpoint_redo()
+    }
+
+    fn checkpoint_list(&self) -> crate::executor::CheckpointListResult {
+        self.inner.checkpoint_list()
+    }
 }
 
 #[cfg(test)]
@@ -344,4 +360,80 @@ mod tests {
         // Error compressor → raw output preserved (T4 safety invariant).
         assert_eq!(out.summary, raw);
     }
+
+    /// Inner executor whose cross-cutting methods return distinguishable non-default
+    /// values, used to prove `CompressedExecutor` forwards rather than falling through
+    /// to the base `ToolExecutor` defaults.
+    #[derive(Debug)]
+    struct CheckpointStubExecutor;
+
+    impl ToolExecutor for CheckpointStubExecutor {
+        async fn execute(&self, _: &str) -> Result<Option<ToolOutput>, ToolError> {
+            Ok(None)
+        }
+        async fn execute_tool_call(&self, _: &ToolCall) -> Result<Option<ToolOutput>, ToolError> {
+            Ok(None)
+        }
+        fn requires_confirmation(&self, _call: &ToolCall) -> bool {
+            true
+        }
+        fn checkpoint_undo(&self, _n: usize) -> crate::executor::CheckpointActionResult {
+            crate::executor::CheckpointActionResult {
+                reverted_commands: 1,
+                restored: 2,
+                deleted: 3,
+                supported: true,
+                message: "stub-undo".to_owned(),
+            }
+        }
+        fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult {
+            crate::executor::CheckpointActionResult {
+                reverted_commands: 4,
+                restored: 5,
+                deleted: 6,
+                supported: true,
+                message: "stub-redo".to_owned(),
+            }
+        }
+        fn checkpoint_list(&self) -> crate::executor::CheckpointListResult {
+            crate::executor::CheckpointListResult {
+                entries: vec![],
+                redo_depth: 7,
+                supported: true,
+            }
+        }
+    }
+
+    /// Regression test for #6012: `requires_confirmation` and the checkpoint trio must be
+    /// forwarded to `self.inner`. Before the fix they fell through to the base
+    /// `ToolExecutor` defaults (`false` / `unsupported()`) regardless of the inner
+    /// executor's actual policy or checkpoint state.
+    #[test]
+    fn requires_confirmation_and_checkpoints_delegated_to_inner() {
+        let executor =
+            CompressedExecutor::new(CheckpointStubExecutor, Arc::new(StubCompressor), 10);
+
+        let call = ToolCall {
+            tool_id: ToolName::new("spy"),
+            params: serde_json::Map::new(),
+            caller_id: None,
+            context: None,
+
+            tool_call_id: String::new(),
+            skill_name: None,
+        };
+        assert!(executor.requires_confirmation(&call));
+
+        let undo = executor.checkpoint_undo(1);
+        assert!(undo.supported);
+        assert_eq!(undo.message, "stub-undo");
+
+        let redo = executor.checkpoint_redo();
+        assert!(redo.supported);
+        assert_eq!(redo.message, "stub-redo");
+
+        let list = executor.checkpoint_list();
+        assert!(list.supported);
+        assert_eq!(list.redo_depth, 7);
+    }
 }

crates/zeph-tools/src/shell/mod.rs

@@ -1010,10 +1010,7 @@ impl ShellExecutor {
                     return false;
                 }
                 if !self.allowed_paths_canonical.is_empty() {
-                    let canonical = p
-                        .canonicalize()
-                        .or_else(|_| std::path::absolute(p))
-                        .unwrap_or_else(|_| p.clone());
+                    let canonical = canonicalize_or_nearest_ancestor(p);
                     if !self
                         .allowed_paths_canonical
                         .iter()
@@ -1509,27 +1506,7 @@ impl ShellExecutor {
             // For non-existent paths, canonicalize the nearest existing ancestor and
             // reattach the suffix: this rejects `allowed/../../etc/shadow` while
             // allowing references to not-yet-created files within allowed dirs.
-            let canonical = if let Ok(c) = path.canonicalize() {
-                c
-            } else {
-                // Collect path components so we can walk up from the full path.
-                let components: Vec<_> = path.components().collect();
-                let mut base_len = components.len();
-                let canonical_base = loop {
-                    if base_len == 0 {
-                        break PathBuf::new();
-                    }
-                    let candidate: PathBuf = components[..base_len].iter().collect();
-                    if let Ok(c) = candidate.canonicalize() {
-                        break c;
-                    }
-                    base_len -= 1;
-                };
-                // Reattach the non-existent suffix (components after base_len).
-                components[base_len..]
-                    .iter()
-                    .fold(canonical_base, |acc, c| acc.join(c))
-            };
+            let canonical = canonicalize_or_nearest_ancestor(&path);
             if !self
                 .allowed_paths_canonical
                 .iter()
@@ -1734,6 +1711,10 @@ impl ToolExecutor for std::sync::Arc<ShellExecutor> {
         self.as_ref().execute(response).await
     }
 
+    async fn execute_confirmed(&self, response: &str) -> Result<Option<ToolOutput>, ToolError> {
+        self.as_ref().execute_confirmed(response).await
+    }
+
     fn tool_definitions(&self) -> Vec<crate::registry::ToolDef> {
         self.as_ref().tool_definitions()
     }
@@ -1742,10 +1723,33 @@ impl ToolExecutor for std::sync::Arc<ShellExecutor> {
         self.as_ref().execute_tool_call(call).await
     }
 
+    async fn execute_tool_call_confirmed(
+        &self,
+        call: &ToolCall,
+    ) -> Result<Option<ToolOutput>, ToolError> {
+        self.as_ref().execute_tool_call_confirmed(call).await
+    }
+
     fn set_skill_env(&self, env: Option<std::collections::HashMap<String, String>>) {
         self.as_ref().set_skill_env(env);
     }
 
+    fn set_effective_trust(&self, level: crate::SkillTrustLevel) {
+        self.as_ref().set_effective_trust(level);
+    }
+
+    fn is_tool_retryable(&self, tool_id: &str) -> bool {
+        self.as_ref().is_tool_retryable(tool_id)
+    }
+
+    fn is_tool_speculatable(&self, tool_id: &str) -> bool {
+        self.as_ref().is_tool_speculatable(tool_id)
+    }
+
+    fn requires_confirmation(&self, call: &ToolCall) -> bool {
+        self.as_ref().requires_confirmation(call)
+    }
+
     fn checkpoint_undo(&self, n: usize) -> crate::executor::CheckpointActionResult {
         self.as_ref().checkpoint_undo(n)
     }
@@ -2745,6 +2749,41 @@ fn has_traversal(path: &str) -> bool {
     path.split(['/', '\\']).any(|seg| seg == "..")
 }
 
+/// Canonicalize `path`, resolving symlinks even when `path` itself does not exist yet.
+///
+/// `Path::canonicalize` requires the full path to exist, which fails for a file that is
+/// about to be created (e.g. a checkpoint capture taken before a write). Falling back to
+/// `std::path::absolute` in that case does not resolve symlinks, so on macOS a path under
+/// `/tmp`/`/var` never becomes `/private/tmp`/`/private/var` — silently breaking any
+/// subsequent `starts_with(allowed_paths_canonical)` containment check (#5999).
+///
+/// This walks up from `path` to the nearest existing ancestor, canonicalizes that
+/// ancestor (resolving its symlinks), and reattaches the non-existent suffix. Falls back
+/// to `std::path::absolute` (or `path` itself) only when no ancestor can be canonicalized.
+fn canonicalize_or_nearest_ancestor(path: &std::path::Path) -> std::path::PathBuf {
+    if let Ok(c) = path.canonicalize() {
+        return c;
+    }
+    let components: Vec<_> = path.components().collect();
+    let mut base_len = components.len();
+    let canonical_base = loop {
+        if base_len == 0 {
+            break None;
+        }
+        let candidate: std::path::PathBuf = components[..base_len].iter().collect();
+        if let Ok(c) = candidate.canonicalize() {
+            break Some(c);
+        }
+        base_len -= 1;
+    };
+    match canonical_base {
+        Some(base) => components[base_len..]
+            .iter()
+            .fold(base, |acc, c| acc.join(c)),
+        None => std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()),
+    }
+}
+
 fn extract_bash_blocks(text: &str) -> Vec<&str> {
     crate::executor::extract_fenced_blocks(text, "bash")
 }