From 72ca18f33d39ea978dcee34227d4991d86e50c18 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Fri, 10 Jul 2026 23:30:58 +0200 Subject: [PATCH] fix(tools): forward checkpoint and confirmation methods through executor wrappers TrustGateExecutor, PolicyGateExecutor, and AdversarialPolicyGateExecutor never overrode checkpoint_undo/checkpoint_redo/checkpoint_list, so calls fell through to the ToolExecutor trait's no-op default instead of reaching ShellExecutor. ScopedToolExecutor and ShadowProbeExecutor had the same gap on the outer layers of the production wrapper chain. As a result, /undo, /redo, and /undo list always reported checkpoints as unsupported whenever trust/policy/adversarial gating, capability_scopes, or shadow_sentinel wrapped the executor chain, even with checkpoints_enabled = true set. ScopedToolExecutor also never overrode requires_confirmation, defaulting to false regardless of the real policy underneath, which would silently disable the speculative-dispatch engine's confirmation gate once any tool opts into speculative execution. Add one-line forwards to self.inner in all five wrapper files, mirroring the existing pass-through style used for is_tool_retryable/set_skill_env. Add regression tests per file proving delegation with distinguishable non-default return values, including an argument round-trip check on checkpoint_undo. Closes #5899, #5905, #5906 --- CHANGELOG.md | 15 +++++ crates/zeph-tools/src/adversarial_gate.rs | 63 ++++++++++++++++++++ crates/zeph-tools/src/policy_gate.rs | 70 +++++++++++++++++++++++ crates/zeph-tools/src/scope.rs | 67 ++++++++++++++++++++++ crates/zeph-tools/src/shadow_probe.rs | 58 +++++++++++++++++++ crates/zeph-tools/src/trust_gate.rs | 59 +++++++++++++++++++ 6 files changed, 332 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9718f3af5..c44c57e81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,21 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- `fix(tools)`: `checkpoint_undo`/`checkpoint_redo`/`checkpoint_list` are now forwarded to the + wrapped inner executor by `TrustGateExecutor`, `PolicyGateExecutor`, + `AdversarialPolicyGateExecutor` (#5899), `ScopedToolExecutor`, and `ShadowProbeExecutor` + (#5905). Previously none of these `ToolExecutor` wrappers overrode the three checkpoint + methods, so calls fell through to the trait's no-op default instead of reaching + `ShellExecutor` — `/undo`, `/redo`, and `/undo list` always reported "Checkpoints are not + enabled" whenever trust/policy/adversarial gating, `capability_scopes`, or `shadow_sentinel` + wrapped the executor chain, even with `[tools.shell] checkpoints_enabled = true` set — the + standard, default-recommended production configuration, not an edge case. Also fixes + `ScopedToolExecutor::requires_confirmation` (#5906), previously hardcoded to the trait + default `false` regardless of the real policy underneath, affecting the (currently dormant) + speculative-dispatch engine. + ### Added - `feat(acp)`: `[[acp.auth_clients]]` — named bearer-token clients for the ACP HTTP/WS diff --git a/crates/zeph-tools/src/adversarial_gate.rs b/crates/zeph-tools/src/adversarial_gate.rs index bc9d3152f..cb7340344 100644 --- a/crates/zeph-tools/src/adversarial_gate.rs +++ b/crates/zeph-tools/src/adversarial_gate.rs @@ -243,6 +243,18 @@ impl ToolExecutor for AdversarialPolicyGateExecutor { fn is_tool_retryable(&self, tool_id: &str) -> bool { self.inner.is_tool_retryable(tool_id) } + + 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() + } } fn params_summary(params: &serde_json::Map) -> String { @@ -525,6 +537,57 @@ mod tests { assert!(!retryable, "MockInner returns false for is_tool_retryable"); } + #[derive(Debug)] + struct CheckpointingInner; + + impl ToolExecutor for CheckpointingInner { + async fn execute(&self, _: &str) -> Result, ToolError> { + Ok(None) + } + async fn execute_tool_call(&self, _: &ToolCall) -> Result, ToolError> { + Ok(None) + } + fn checkpoint_undo(&self, n: usize) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + reverted_commands: n, + ..Default::default() + } + } + fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + ..Default::default() + } + } + fn checkpoint_list(&self) -> crate::executor::CheckpointListResult { + crate::executor::CheckpointListResult { + supported: true, + ..Default::default() + } + } + } + + #[tokio::test] + async fn delegation_checkpoint_methods() { + let (_, llm) = MockLlm::new("ALLOW"); + let gate = AdversarialPolicyGateExecutor::new( + CheckpointingInner, + make_validator(false), + Arc::new(llm), + ); + let undo_result = gate.checkpoint_undo(7); + assert!(undo_result.supported); + assert_eq!( + undo_result.reverted_commands, 7, + "n must be forwarded, not hardcoded" + ); + assert!(gate.checkpoint_redo().supported); + assert!(gate.checkpoint_list().supported); + } + #[tokio::test] async fn delegation_tool_definitions() { let (_, llm) = MockLlm::new("ALLOW"); diff --git a/crates/zeph-tools/src/policy_gate.rs b/crates/zeph-tools/src/policy_gate.rs index 3523dc8b7..c32f1d22d 100644 --- a/crates/zeph-tools/src/policy_gate.rs +++ b/crates/zeph-tools/src/policy_gate.rs @@ -380,6 +380,18 @@ impl ToolExecutor for PolicyGateExecutor { fn is_tool_speculatable(&self, tool_id: &str) -> bool { self.inner.is_tool_speculatable(tool_id) } + + 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() + } } fn truncate_params(params: &serde_json::Map) -> String { @@ -467,6 +479,64 @@ mod tests { } } + #[derive(Debug)] + struct CheckpointingExecutor; + + impl ToolExecutor for CheckpointingExecutor { + async fn execute(&self, _: &str) -> Result, ToolError> { + Ok(None) + } + async fn execute_tool_call(&self, _: &ToolCall) -> Result, ToolError> { + Ok(None) + } + fn checkpoint_undo(&self, n: usize) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + reverted_commands: n, + ..Default::default() + } + } + fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + ..Default::default() + } + } + fn checkpoint_list(&self) -> crate::executor::CheckpointListResult { + crate::executor::CheckpointListResult { + supported: true, + ..Default::default() + } + } + } + + #[test] + fn checkpoint_methods_delegated_to_inner() { + let config = PolicyConfig { + enabled: false, + default_effect: DefaultEffect::Allow, + rules: vec![], + policy_file: None, + policy_provider: ProviderName::default(), + }; + let enforcer = Arc::new(PolicyEnforcer::compile(&config).unwrap()); + let context = Arc::new(RwLock::new(PolicyContext { + trust_level: SkillTrustLevel::Trusted, + env: HashMap::new(), + })); + let gate = PolicyGateExecutor::new(CheckpointingExecutor, enforcer, context); + let undo_result = gate.checkpoint_undo(7); + assert!(undo_result.supported); + assert_eq!( + undo_result.reverted_commands, 7, + "n must be forwarded, not hardcoded" + ); + assert!(gate.checkpoint_redo().supported); + assert!(gate.checkpoint_list().supported); + } + #[tokio::test] async fn allow_by_default_when_default_allow() { let config = PolicyConfig { diff --git a/crates/zeph-tools/src/scope.rs b/crates/zeph-tools/src/scope.rs index df9b291f5..13316d3b7 100644 --- a/crates/zeph-tools/src/scope.rs +++ b/crates/zeph-tools/src/scope.rs @@ -611,6 +611,22 @@ impl ToolExecutor for ScopedToolExecutor { fn is_tool_speculatable(&self, tool_id: &str) -> bool { self.inner.is_tool_speculatable(tool_id) } + + 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() + } + + fn requires_confirmation(&self, call: &ToolCall) -> bool { + self.inner.requires_confirmation(call) + } } // ── Config-driven builder ────────────────────────────────────────────────────── @@ -724,6 +740,38 @@ mod tests { } } + struct CheckpointingExecutor; + + impl ToolExecutor for CheckpointingExecutor { + async fn execute(&self, _: &str) -> Result, ToolError> { + Ok(None) + } + fn checkpoint_undo(&self, n: usize) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + reverted_commands: n, + ..Default::default() + } + } + fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + ..Default::default() + } + } + fn checkpoint_list(&self) -> crate::executor::CheckpointListResult { + crate::executor::CheckpointListResult { + supported: true, + ..Default::default() + } + } + fn requires_confirmation(&self, _call: &ToolCall) -> bool { + true + } + } + fn null_def(id: &str) -> ToolDef { ToolDef { id: id.to_owned().into(), @@ -1060,6 +1108,25 @@ mod tests { assert!(!updated.admits("builtin:write")); } + #[test] + fn checkpoint_methods_delegated_to_inner() { + let executor = ScopedToolExecutor::new(CheckpointingExecutor, ToolScope::full()); + let undo_result = executor.checkpoint_undo(7); + assert!(undo_result.supported); + assert_eq!( + undo_result.reverted_commands, 7, + "n must be forwarded, not hardcoded" + ); + assert!(executor.checkpoint_redo().supported); + assert!(executor.checkpoint_list().supported); + } + + #[test] + fn requires_confirmation_delegated_to_inner() { + let executor = ScopedToolExecutor::new(CheckpointingExecutor, ToolScope::full()); + assert!(executor.requires_confirmation(&make_call("builtin:shell"))); + } + #[test] fn build_from_config_with_scopes() { let mut scopes = std::collections::HashMap::new(); diff --git a/crates/zeph-tools/src/shadow_probe.rs b/crates/zeph-tools/src/shadow_probe.rs index 282ce9edb..189b429b4 100644 --- a/crates/zeph-tools/src/shadow_probe.rs +++ b/crates/zeph-tools/src/shadow_probe.rs @@ -388,6 +388,18 @@ impl ToolExecutor for ShadowProbeExecutor { 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)] @@ -784,6 +796,52 @@ mod tests { assert_eq!(probe.recorded.lock().unwrap().len(), 1); } + struct CheckpointingInner; + impl ToolExecutor for CheckpointingInner { + async fn execute(&self, _: &str) -> Result, ToolError> { + Ok(None) + } + fn checkpoint_undo(&self, n: usize) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + reverted_commands: n, + ..Default::default() + } + } + fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + ..Default::default() + } + } + fn checkpoint_list(&self) -> crate::executor::CheckpointListResult { + crate::executor::CheckpointListResult { + supported: true, + ..Default::default() + } + } + } + + #[test] + fn checkpoint_methods_delegated_to_inner() { + let exec = ShadowProbeExecutor::new( + CheckpointingInner, + Arc::new(AllowProbe), + Arc::new(std::sync::atomic::AtomicU64::new(1)), + Arc::new(parking_lot::RwLock::new("calm".to_owned())), + ); + let undo_result = exec.checkpoint_undo(7); + assert!(undo_result.supported); + assert_eq!( + undo_result.reverted_commands, 7, + "n must be forwarded, not hardcoded" + ); + assert!(exec.checkpoint_redo().supported); + assert!(exec.checkpoint_list().supported); + } + #[test] fn is_tool_speculatable_always_false() { let exec = make_executor(AllowProbe); diff --git a/crates/zeph-tools/src/trust_gate.rs b/crates/zeph-tools/src/trust_gate.rs index 3437ae368..c4a5ef79c 100644 --- a/crates/zeph-tools/src/trust_gate.rs +++ b/crates/zeph-tools/src/trust_gate.rs @@ -291,6 +291,18 @@ impl ToolExecutor for TrustGateExecutor { self.inner.is_tool_speculatable(tool_id) } + 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() + } + fn set_effective_trust(&self, level: crate::SkillTrustLevel) { self.effective_trust .store(trust_to_u8(level), Ordering::Relaxed); @@ -744,6 +756,53 @@ mod tests { assert!(!gate.is_tool_retryable("bash")); } + #[test] + fn checkpoint_methods_delegated_to_inner() { + #[derive(Debug)] + struct CheckpointingExecutor; + impl ToolExecutor for CheckpointingExecutor { + async fn execute(&self, _: &str) -> Result, ToolError> { + Ok(None) + } + async fn execute_tool_call( + &self, + _: &ToolCall, + ) -> Result, ToolError> { + Ok(None) + } + fn checkpoint_undo(&self, n: usize) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + reverted_commands: n, + ..Default::default() + } + } + fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult { + crate::executor::CheckpointActionResult { + supported: true, + message: "stub".into(), + ..Default::default() + } + } + fn checkpoint_list(&self) -> crate::executor::CheckpointListResult { + crate::executor::CheckpointListResult { + supported: true, + ..Default::default() + } + } + } + let gate = TrustGateExecutor::new(CheckpointingExecutor, PermissionPolicy::default()); + let undo_result = gate.checkpoint_undo(7); + assert!(undo_result.supported); + assert_eq!( + undo_result.reverted_commands, 7, + "n must be forwarded, not hardcoded" + ); + assert!(gate.checkpoint_redo().supported); + assert!(gate.checkpoint_list().supported); + } + #[test] fn set_skill_env_forwarded_to_inner() { let inner = EnvCapture::new();