From fc982e920968f5852f224008ea2a3834a75ad624 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Sat, 11 Jul 2026 16:20:33 +0200 Subject: [PATCH] fix(tools): forward remaining ToolExecutor methods, fix symlinked checkpoint drop, add checkpoint-stack tests CompressedExecutor, ToolFilter, and Arc now forward every cross-cutting ToolExecutor method to their inner/wrapped executor instead of silently falling through to trait defaults, closing the same shadow-impl forwarding gap fixed for other wrappers in #5930 and #6011. capture_snapshot_for no longer drops a checkpoint for a newly-created file under a symlinked allowed_paths prefix on macOS: a new canonicalize_or_nearest_ancestor helper resolves symlinks via the nearest existing ancestor when the target path itself does not exist yet. Adds regression coverage for the ShellExecutor checkpoint stack: redo without a prior undo, checkpoint_list ordering, and multi-step undo/redo depth bookkeeping. --- CHANGELOG.md | 35 +++ .../zeph-tools/src/compression/decorator.rs | 92 ++++++ crates/zeph-tools/src/shell/mod.rs | 89 ++++-- crates/zeph-tools/src/shell/tests.rs | 271 ++++++++++++++++++ crates/zeph-tools/src/tool_filter.rs | 182 ++++++++++++ 5 files changed, 644 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4be9e36b5..3b3b32882 100644 --- a/CHANGELOG.md +++ b/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` 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` + 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 diff --git a/crates/zeph-tools/src/compression/decorator.rs b/crates/zeph-tools/src/compression/decorator.rs index 8a51db73e..b59197a80 100644 --- a/crates/zeph-tools/src/compression/decorator.rs +++ b/crates/zeph-tools/src/compression/decorator.rs @@ -150,6 +150,22 @@ impl ToolExecutor for CompressedExecutor { 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, ToolError> { + Ok(None) + } + async fn execute_tool_call(&self, _: &ToolCall) -> Result, 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); + } } diff --git a/crates/zeph-tools/src/shell/mod.rs b/crates/zeph-tools/src/shell/mod.rs index 26ff09f9a..04c7b7249 100644 --- a/crates/zeph-tools/src/shell/mod.rs +++ b/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 { self.as_ref().execute(response).await } + async fn execute_confirmed(&self, response: &str) -> Result, ToolError> { + self.as_ref().execute_confirmed(response).await + } + fn tool_definitions(&self) -> Vec { self.as_ref().tool_definitions() } @@ -1742,10 +1723,33 @@ impl ToolExecutor for std::sync::Arc { self.as_ref().execute_tool_call(call).await } + async fn execute_tool_call_confirmed( + &self, + call: &ToolCall, + ) -> Result, ToolError> { + self.as_ref().execute_tool_call_confirmed(call).await + } + fn set_skill_env(&self, env: Option>) { 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") } diff --git a/crates/zeph-tools/src/shell/tests.rs b/crates/zeph-tools/src/shell/tests.rs index 3d70f77a0..dbe8495a8 100644 --- a/crates/zeph-tools/src/shell/tests.rs +++ b/crates/zeph-tools/src/shell/tests.rs @@ -748,6 +748,80 @@ async fn execute_confirmed_skips_confirmation() { assert!(output.summary.contains("confirmed")); } +/// Calls `execute_confirmed` through the `ToolExecutor` trait bound (not as an inherent +/// method on a concrete `ShellExecutor`), forcing dynamic-style dispatch identical to how +/// `Arc` is invoked when composed under other wrapper executors. +async fn call_execute_confirmed_via_trait( + executor: &T, + response: &str, +) -> Result, ToolError> { + executor.execute_confirmed(response).await +} + +/// Regression test for #6012: `Arc`'s `ToolExecutor` impl did not override +/// `execute_confirmed`, so dispatch through the trait fell through to the base default +/// (`self.execute(..)`, i.e. `skip_confirm = false`) instead of forwarding to +/// `ShellExecutor::execute_confirmed`'s bypass logic — silently reintroducing the +/// confirmation prompt for any caller that only holds a generic `T: ToolExecutor` handle. +#[tokio::test] +async fn arc_shell_executor_execute_confirmed_bypasses_confirmation() { + let config = ShellConfig { + confirm_patterns: vec!["echo".into()], + ..default_config() + }; + let executor = std::sync::Arc::new(ShellExecutor::new(&config)); + let response = "```bash\necho confirmed\n```"; + + let result = call_execute_confirmed_via_trait(&executor, response).await; + assert!( + result.is_ok(), + "execute_confirmed via Arc's ToolExecutor impl must bypass confirmation" + ); + let output = result.unwrap().unwrap(); + assert!(output.summary.contains("confirmed")); +} + +/// Calls the trio of boolean cross-cutting methods through the `ToolExecutor` trait bound, +/// used to compare `Arc`'s trait dispatch against calling the same methods +/// directly on the wrapped `ShellExecutor`. +fn call_cross_cutting_bools(executor: &T, call: &ToolCall) -> (bool, bool, bool) { + ( + executor.requires_confirmation(call), + executor.is_tool_speculatable("bash"), + executor.is_tool_retryable("bash"), + ) +} + +/// Forward-looking consistency guard for #6012, not a regression test that can currently +/// distinguish fixed-vs-broken: `ShellExecutor` itself never overrides `requires_confirmation`, +/// `is_tool_speculatable`, or `is_tool_retryable` (both sides always equal the trait's default), +/// so this passes identically whether or not `Arc` forwards them. Its value is +/// pinning that dispatching these methods through `Arc`'s `ToolExecutor` impl +/// stays behaviorally identical to calling them directly on the wrapped `ShellExecutor` — if +/// `ShellExecutor` ever gains a real override for one of these methods without the `Arc` impl +/// being updated to forward it, this test will start failing. +#[tokio::test] +async fn arc_shell_executor_forwards_remaining_cross_cutting_methods() { + let executor = std::sync::Arc::new(ShellExecutor::new(&default_config())); + let call = ToolCall { + tool_id: ToolName::new("bash"), + params: serde_json::Map::new(), + caller_id: None, + context: None, + tool_call_id: String::new(), + skill_name: None, + }; + + let (via_arc_confirm, via_arc_speculatable, via_arc_retryable) = + call_cross_cutting_bools(&executor, &call); + let (via_inner_confirm, via_inner_speculatable, via_inner_retryable) = + call_cross_cutting_bools(executor.as_ref(), &call); + + assert_eq!(via_arc_confirm, via_inner_confirm); + assert_eq!(via_arc_speculatable, via_inner_speculatable); + assert_eq!(via_arc_retryable, via_inner_retryable); +} + // --- default confirm patterns test --- #[test] @@ -3443,3 +3517,200 @@ async fn arc_wrapped_executor_forwards_checkpoint_methods() { ); assert!(file_path.exists()); } + +// --- Checkpoint stack coverage gaps (#6001) --- + +/// Regression test for #6001: `checkpoint_redo` called with no prior `checkpoint_undo` +/// must be a no-op "nothing to redo" result, not a panic or corrupted stack state. +#[tokio::test] +#[cfg(not(target_os = "windows"))] +async fn checkpoint_redo_without_prior_undo_is_noop() { + let dir = tempfile::tempdir().unwrap(); + let dir_path = dir.path().canonicalize().unwrap(); + let file_path = dir_path.join("target.txt"); + + let config = ShellConfig { + checkpoints_enabled: true, + allowed_paths: vec![dir_path.to_string_lossy().into_owned()], + ..default_config() + }; + let executor = ShellExecutor::new(&config); + + // Redo on a completely empty stack (no checkpoints recorded at all). + let redo = executor.checkpoint_redo(); + assert!(redo.supported); + assert_eq!(redo.reverted_commands, 0); + assert_eq!(redo.message, "Nothing to redo."); + + // Record a checkpoint but never undo it — redo stack stays empty. + let command = format!("echo hello > {}", file_path.display()); + let response = format!("```bash\n{command}\n```"); + executor.execute(&response).await.unwrap(); + assert_eq!(executor.checkpoint_list().entries.len(), 1); + + let redo_after_record = executor.checkpoint_redo(); + assert!(redo_after_record.supported); + assert_eq!(redo_after_record.reverted_commands, 0); + assert_eq!(redo_after_record.message, "Nothing to redo."); + // Stack must be untouched by the no-op redo. + assert_eq!(executor.checkpoint_list().entries.len(), 1); + assert!(file_path.exists()); +} + +/// Regression test for #6001: `checkpoint_list` must report the undo stack most-recent +/// first when 2+ checkpoints exist — pinning the actual (implementation) ordering rather +/// than leaving it implied. This is user-visible via `/undo list`. +#[tokio::test] +#[cfg(not(target_os = "windows"))] +async fn checkpoint_list_orders_most_recent_first_with_multiple_entries() { + let dir = tempfile::tempdir().unwrap(); + let dir_path = dir.path().canonicalize().unwrap(); + let file_a = dir_path.join("a.txt"); + let file_b = dir_path.join("b.txt"); + let file_c = dir_path.join("c.txt"); + + let config = ShellConfig { + checkpoints_enabled: true, + allowed_paths: vec![dir_path.to_string_lossy().into_owned()], + ..default_config() + }; + let executor = ShellExecutor::new(&config); + + for file in [&file_a, &file_b, &file_c] { + let command = format!("echo hello > {}", file.display()); + let response = format!("```bash\n{command}\n```"); + executor.execute(&response).await.unwrap(); + } + + let list = executor.checkpoint_list(); + assert!(list.supported); + assert_eq!(list.entries.len(), 3); + // Most-recent-first: the last-recorded checkpoint (file_c's command) is entries[0]. + assert!(list.entries[0].command.contains("c.txt")); + assert!(list.entries[1].command.contains("b.txt")); + assert!(list.entries[2].command.contains("a.txt")); + // index field is 0-based with 0 = most recent, matching display order. + assert_eq!(list.entries[0].index, 0); + assert_eq!(list.entries[1].index, 1); + assert_eq!(list.entries[2].index, 2); +} + +/// Regression test for #6001: multi-checkpoint undo-stack depth under a sequence of +/// undo/redo/undo operations. Pins bookkeeping across: record x2, undo x2 (one at a time), +/// redo x1, undo x1 again. +#[tokio::test] +#[cfg(not(target_os = "windows"))] +async fn checkpoint_undo_redo_undo_sequence_tracks_depth() { + let dir = tempfile::tempdir().unwrap(); + let dir_path = dir.path().canonicalize().unwrap(); + let file_a = dir_path.join("a.txt"); + let file_b = dir_path.join("b.txt"); + + let config = ShellConfig { + checkpoints_enabled: true, + allowed_paths: vec![dir_path.to_string_lossy().into_owned()], + ..default_config() + }; + let executor = ShellExecutor::new(&config); + + for file in [&file_a, &file_b] { + let command = format!("echo hello > {}", file.display()); + let response = format!("```bash\n{command}\n```"); + executor.execute(&response).await.unwrap(); + } + assert_eq!(executor.checkpoint_list().entries.len(), 2); + assert!(file_a.exists()); + assert!(file_b.exists()); + + // Undo once: reverts the most recent checkpoint (b.txt creation). + let undo1 = executor.checkpoint_undo(1); + assert_eq!(undo1.reverted_commands, 1); + assert!(!file_b.exists(), "first undo must revert b.txt creation"); + assert!(file_a.exists(), "a.txt must be untouched by the first undo"); + assert_eq!(executor.checkpoint_list().entries.len(), 1); + + // Undo again: reverts the remaining checkpoint (a.txt creation). + let undo2 = executor.checkpoint_undo(1); + assert_eq!(undo2.reverted_commands, 1); + assert!(!file_a.exists(), "second undo must revert a.txt creation"); + assert_eq!(executor.checkpoint_list().entries.len(), 0); + + // Redo once: re-applies the a.txt creation (LIFO — the most recently undone entry). + let redo1 = executor.checkpoint_redo(); + assert_eq!(redo1.reverted_commands, 1); + assert!(file_a.exists(), "first redo must re-apply a.txt creation"); + assert_eq!(executor.checkpoint_list().entries.len(), 1); + + // Undo again: the redo we just applied moves back onto the undo stack and is undone. + let undo3 = executor.checkpoint_undo(1); + assert_eq!(undo3.reverted_commands, 1); + assert!( + !file_a.exists(), + "third undo must revert the re-applied a.txt creation" + ); + assert_eq!(executor.checkpoint_list().entries.len(), 0); +} + +// --- capture_snapshot_for symlinked-allowed_paths regression (#5999) --- + +/// Regression test for #5999: a checkpoint for a *newly-created* file must still be +/// captured when `allowed_paths` is configured with a raw (non-canonicalized) prefix that +/// is itself a symlink on this platform (e.g. macOS's `/tmp` -> `/private/tmp`). +/// +/// Before the fix, `capture_snapshot_for` canonicalized the file's own path to check +/// sandbox containment; for a file that does not exist yet `canonicalize()` fails, and +/// the code fell back to `std::path::absolute`, which does NOT resolve symlinks. The +/// file's path then stayed under the raw `/tmp/...` prefix while `allowed_paths_canonical` +/// (canonicalized at construction time, when the directory already existed) held the +/// resolved `/private/tmp/...` prefix — the `starts_with` containment check failed and +/// the checkpoint was silently dropped. +/// +/// Uses a raw, non-canonicalized `/tmp`-rooted directory as the `allowed_paths` entry +/// (not a pre-canonicalized tempdir, unlike the sibling test above) so the symlink +/// resolution path is actually exercised. +#[tokio::test] +#[cfg(target_os = "macos")] +async fn checkpoint_captures_new_file_under_symlinked_allowed_path() { + let raw_dir = tempfile::Builder::new() + .prefix("zeph_5999_") + .tempdir_in("/tmp") + .unwrap(); + let dir_raw = raw_dir.path().to_path_buf(); + // Sanity: this host must actually exhibit the /tmp -> /private/tmp symlink for the + // regression to be exercised; otherwise the raw and canonical prefixes are identical + // and the test would pass trivially even with the bug present. + let dir_canonical = dir_raw.canonicalize().unwrap(); + assert_ne!( + dir_raw, dir_canonical, + "test requires a symlinked /tmp on this host to exercise the regression" + ); + + let file_path = dir_raw.join("new_file.txt"); // does not exist until the command runs + + let config = ShellConfig { + checkpoints_enabled: true, + allowed_paths: vec![dir_raw.to_string_lossy().into_owned()], + ..default_config() + }; + let executor = ShellExecutor::new(&config); + + let command = format!("echo hello > {}", file_path.display()); + let response = format!("```bash\n{command}\n```"); + executor.execute(&response).await.unwrap(); + + assert!(file_path.exists()); + let list = executor.checkpoint_list(); + assert_eq!( + list.entries.len(), + 1, + "checkpoint must be captured for a new file under a symlinked allowed_paths prefix" + ); + + // The checkpoint must also be usable: undo should remove the newly-created file. + let undo = executor.checkpoint_undo(1); + assert_eq!(undo.reverted_commands, 1); + assert!( + !file_path.exists(), + "undo must delete the newly-created file" + ); +} diff --git a/crates/zeph-tools/src/tool_filter.rs b/crates/zeph-tools/src/tool_filter.rs index 4ceb07049..d86b0b804 100644 --- a/crates/zeph-tools/src/tool_filter.rs +++ b/crates/zeph-tools/src/tool_filter.rs @@ -45,6 +45,48 @@ impl ToolExecutor for ToolFilter { } self.inner.execute_tool_call(call).await } + + async fn execute_tool_call_confirmed( + &self, + call: &ToolCall, + ) -> Result, ToolError> { + if self.suppressed.contains(&call.tool_id.as_str()) { + return Ok(None); + } + self.inner.execute_tool_call_confirmed(call).await + } + + fn set_skill_env(&self, env: Option>) { + self.inner.set_skill_env(env); + } + + fn set_effective_trust(&self, level: crate::SkillTrustLevel) { + self.inner.set_effective_trust(level); + } + + fn is_tool_retryable(&self, tool_id: &str) -> bool { + self.inner.is_tool_retryable(tool_id) + } + + 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)] @@ -146,4 +188,144 @@ mod tests { let result = filter.execute_tool_call(&call).await.unwrap(); assert!(result.is_some()); } + + /// Inner executor whose cross-cutting methods return distinguishable non-default + /// values, used to prove `ToolFilter` forwards rather than falling through to the + /// base `ToolExecutor` defaults. + #[derive(Debug)] + struct CrossCuttingStubExecutor; + + impl ToolExecutor for CrossCuttingStubExecutor { + async fn execute(&self, _: &str) -> Result, ToolError> { + Ok(None) + } + async fn execute_tool_call( + &self, + call: &ToolCall, + ) -> Result, ToolError> { + Ok(Some(ToolOutput { + tool_name: call.tool_id.clone(), + summary: "stub".to_owned(), + blocks_executed: 1, + filter_stats: None, + diff: None, + streamed: false, + terminal_id: None, + locations: None, + raw_response: None, + claim_source: None, + })) + } + async fn execute_tool_call_confirmed( + &self, + call: &ToolCall, + ) -> Result, ToolError> { + Ok(Some(ToolOutput { + tool_name: call.tool_id.clone(), + summary: "stub-confirmed".to_owned(), + blocks_executed: 1, + filter_stats: None, + diff: None, + streamed: false, + terminal_id: None, + locations: None, + raw_response: None, + claim_source: None, + })) + } + fn is_tool_retryable(&self, _tool_id: &str) -> bool { + true + } + fn is_tool_speculatable(&self, _tool_id: &str) -> bool { + true + } + fn requires_confirmation(&self, _call: &ToolCall) -> bool { + true + } + fn checkpoint_undo(&self, _n: usize) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + reverted_commands: 1, + restored: 0, + deleted: 0, + supported: true, + message: "stub-undo".to_owned(), + } + } + fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + reverted_commands: 0, + restored: 1, + deleted: 0, + supported: true, + message: "stub-redo".to_owned(), + } + } + fn checkpoint_list(&self) -> crate::executor::CheckpointListResult { + crate::executor::CheckpointListResult { + entries: vec![], + redo_depth: 3, + supported: true, + } + } + } + + fn make_call(tool_id: &str) -> ToolCall { + ToolCall { + tool_id: ToolName::new(tool_id), + params: serde_json::Map::new(), + caller_id: None, + context: None, + tool_call_id: String::new(), + skill_name: None, + } + } + + /// Regression test for #6012: cross-cutting methods must be forwarded to `self.inner`. + /// Before the fix every one of these fell through to the base `ToolExecutor` default + /// (`false` / `unsupported()`) regardless of the inner executor's actual policy. + #[test] + fn cross_cutting_methods_delegated_to_inner() { + let filter = ToolFilter::new(CrossCuttingStubExecutor, &["read", "glob"]); + + assert!(filter.is_tool_retryable("edit")); + assert!(filter.is_tool_speculatable("edit")); + assert!(filter.requires_confirmation(&make_call("edit"))); + + let undo = filter.checkpoint_undo(1); + assert!(undo.supported); + assert_eq!(undo.message, "stub-undo"); + + let redo = filter.checkpoint_redo(); + assert!(redo.supported); + assert_eq!(redo.message, "stub-redo"); + + let list = filter.checkpoint_list(); + assert!(list.supported); + assert_eq!(list.redo_depth, 3); + } + + /// Suppression must also apply to the confirmed-call dispatch path, not just the + /// initial (unconfirmed) `execute_tool_call`. + #[tokio::test] + async fn suppressed_tool_call_confirmed_returns_none() { + let filter = ToolFilter::new(CrossCuttingStubExecutor, &["read", "glob"]); + let result = filter + .execute_tool_call_confirmed(&make_call("read")) + .await + .unwrap(); + assert!(result.is_none()); + } + + #[tokio::test] + async fn allowed_tool_call_confirmed_passes_through() { + let filter = ToolFilter::new(CrossCuttingStubExecutor, &["read", "glob"]); + let result = filter + .execute_tool_call_confirmed(&make_call("edit")) + .await + .unwrap() + .unwrap(); + // Distinguishes forwarding to execute_tool_call_confirmed from an (incorrect) + // fallback to execute_tool_call — the two return different summaries. + assert_eq!(result.summary, "stub-confirmed"); + } }