refactor(tools): migrate Arc<ShellExecutor> to DynExecutor erasure path

CHANGELOG.md

@@ -107,6 +107,21 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
   bespoke wrapper to drift. `DynSchedulerExecutor` was `pub(crate)`, not a public API, so this
   is an internal refactor with no behavior change (#6000).
 
+- **zeph-tools**: removed `impl ToolExecutor for Arc<ShellExecutor>`, a hand-maintained
+  forwarding wrapper structurally identical to the `DynSchedulerExecutor` anti-pattern above and
+  the direct cause of #5985. The shell executor's production composition slot
+  (`agent_setup.rs`) now wraps it as `zeph_tools::DynExecutor(Arc<ShellExecutor>)` and flows
+  through the same `ErasedToolExecutor` erasure path, which forwards all 13 methods by
+  construction. Removing the wrapper exposed a real gap it had been masking: `impl ToolExecutor
+  for ShellExecutor` itself never overrode `execute_confirmed`, so any caller reaching it
+  through a generic `T: ToolExecutor`/`dyn ToolExecutor`/erasure path (rather than the
+  concrete inherent `ShellExecutor::execute_confirmed` method) fell through to the trait
+  default (`self.execute(..)`, no bypass) — reintroducing #6012 for the production shell slot.
+  `ShellExecutor`'s `ToolExecutor` impl now explicitly overrides `execute_confirmed` to forward
+  to the same `execute_inner(response, true)` the inherent method already uses, closing the gap
+  for every dispatch path. `shell_executor_handle` (used only for TUI background-run metrics via
+  the concrete `ShellExecutor::background_runs_snapshot()` inherent method) is unaffected (#6224).
+
 ### Docs
 
 - **LLM**: `AnyProvider`/`Router`/`Triage`'s `capability_delegation_advisory()` rustdoc comments

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

@@ -1740,66 +1740,18 @@ impl ShellExecutor {
     }
 }
 
-impl ToolExecutor for std::sync::Arc<ShellExecutor> {
+impl ToolExecutor for ShellExecutor {
     async fn execute(&self, response: &str) -> Result<Option<ToolOutput>, ToolError> {
-        self.as_ref().execute(response).await
+        self.execute_inner(response, false).await
     }
 
+    // Overrides the trait default (`self.execute(response)`, i.e. no bypass) explicitly:
+    // the inherent `execute_confirmed` above is a separate method, invisible to callers that
+    // only hold a `&dyn ToolExecutor`/generic `T: ToolExecutor` handle (e.g. through
+    // `DynExecutor`'s erasure path). Without this override, dynamic dispatch silently loses
+    // the confirmation bypass — the exact regression #6012 was filed for.
     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()
-    }
-
-    async fn execute_tool_call(&self, call: &ToolCall) -> Result<Option<ToolOutput>, ToolError> {
-        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)
-    }
-
-    fn checkpoint_redo(&self) -> crate::executor::CheckpointActionResult {
-        self.as_ref().checkpoint_redo()
-    }
-
-    fn checkpoint_list(&self) -> crate::executor::CheckpointListResult {
-        self.as_ref().checkpoint_list()
-    }
-}
-
-impl ToolExecutor for ShellExecutor {
-    async fn execute(&self, response: &str) -> Result<Option<ToolOutput>, ToolError> {
-        self.execute_inner(response, false).await
+        self.execute_inner(response, true).await
     }
 
     fn tool_definitions(&self) -> Vec<crate::registry::ToolDef> {

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

@@ -750,40 +750,43 @@ async fn execute_confirmed_skips_confirmation() {
 
 /// 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<ShellExecutor>` is invoked when composed under other wrapper executors.
+/// a `DynExecutor`-wrapped `ShellExecutor` is invoked when composed under other wrapper
+/// executors.
 async fn call_execute_confirmed_via_trait<T: ToolExecutor>(
     executor: &T,
     response: &str,
 ) -> Result<Option<ToolOutput>, ToolError> {
     executor.execute_confirmed(response).await
 }
 
-/// Regression test for #6012: `Arc<ShellExecutor>`'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.
+/// Regression test for #6012: the (now-removed) hand-written `Arc<ShellExecutor>`
+/// `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. The erasure path (`DynExecutor`/`ErasedToolExecutor`) forwards
+/// all methods by construction, so this pins the same invariant through that path instead.
 #[tokio::test]
-async fn arc_shell_executor_execute_confirmed_bypasses_confirmation() {
+async fn dyn_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 executor = crate::executor::DynExecutor(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<ShellExecutor>'s ToolExecutor impl must bypass confirmation"
+        "execute_confirmed via DynExecutor'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<ShellExecutor>`'s trait dispatch against calling the same methods
-/// directly on the wrapped `ShellExecutor`.
+/// used to compare `DynExecutor`'s trait dispatch against calling the same methods directly
+/// on the wrapped `ShellExecutor`.
 fn call_cross_cutting_bools<T: ToolExecutor>(executor: &T, call: &ToolCall) -> (bool, bool, bool) {
     (
         executor.requires_confirmation(call),
@@ -795,14 +798,16 @@ fn call_cross_cutting_bools<T: ToolExecutor>(executor: &T, call: &ToolCall) -> (
 /// 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<ShellExecutor>` forwards them. Its value is
-/// pinning that dispatching these methods through `Arc<ShellExecutor>`'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()));
+/// so this passes identically regardless of erasure. Its value is pinning that dispatching
+/// these methods through the `DynExecutor`/`ErasedToolExecutor` erasure path 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 erasure
+/// path forwarding it, this test will start failing.
+#[tokio::test]
+async fn dyn_executor_forwards_remaining_cross_cutting_methods() {
+    let inner = std::sync::Arc::new(ShellExecutor::new(&default_config()));
+    let inner_clone = std::sync::Arc::clone(&inner);
+    let executor = crate::executor::DynExecutor(inner_clone);
     let call = ToolCall {
         tool_id: ToolName::new("bash"),
         params: serde_json::Map::new(),
@@ -812,14 +817,14 @@ async fn arc_shell_executor_forwards_remaining_cross_cutting_methods() {
         skill_name: None,
     };
 
-    let (via_arc_confirm, via_arc_speculatable, via_arc_retryable) =
+    let (via_dyn_confirm, via_dyn_speculatable, via_dyn_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);
+        call_cross_cutting_bools(inner.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);
+    assert_eq!(via_dyn_confirm, via_inner_confirm);
+    assert_eq!(via_dyn_speculatable, via_inner_speculatable);
+    assert_eq!(via_dyn_retryable, via_inner_retryable);
 }
 
 // --- default confirm patterns test ---
@@ -3494,11 +3499,11 @@ async fn supervised_background_run_limit_still_enforced() {
     cancel.cancel();
 }
 
-// --- Arc<ShellExecutor> checkpoint forwarding (regression for #5985) ---
+// --- DynExecutor-wrapped ShellExecutor checkpoint forwarding (regression for #5985) ---
 
 #[tokio::test]
 #[cfg(not(target_os = "windows"))]
-async fn arc_wrapped_executor_forwards_checkpoint_methods() {
+async fn dyn_executor_forwards_checkpoint_methods() {
     let dir = tempfile::tempdir().unwrap();
     // Canonicalize so path comparisons don't trip on macOS's /var -> /private/var symlink.
     let dir_path = dir.path().canonicalize().unwrap();
@@ -3510,7 +3515,7 @@ async fn arc_wrapped_executor_forwards_checkpoint_methods() {
         ..default_config()
     };
     // Wrapped exactly as agent_setup.rs wires the shell slot in the executor chain.
-    let executor: Arc<ShellExecutor> = Arc::new(ShellExecutor::new(&config));
+    let executor = crate::executor::DynExecutor(Arc::new(ShellExecutor::new(&config)));
 
     let command = format!("echo hello > {}", file_path.display());
     let response = format!("```bash\n{command}\n```");
@@ -3519,23 +3524,23 @@ async fn arc_wrapped_executor_forwards_checkpoint_methods() {
     let list = executor.checkpoint_list();
     assert!(
         list.supported,
-        "Arc<ShellExecutor>::checkpoint_list must forward to the real impl, not the trait default"
+        "DynExecutor::checkpoint_list must forward to the real impl, not the trait default"
     );
     assert_eq!(list.entries.len(), 1);
     assert!(file_path.exists());
 
     let undo = executor.checkpoint_undo(1);
     assert!(
         undo.supported,
-        "Arc<ShellExecutor>::checkpoint_undo must forward to the real impl, not the trait default"
+        "DynExecutor::checkpoint_undo must forward to the real impl, not the trait default"
     );
     assert_eq!(undo.deleted, 1);
     assert!(!file_path.exists());
 
     let redo = executor.checkpoint_redo();
     assert!(
         redo.supported,
-        "Arc<ShellExecutor>::checkpoint_redo must forward to the real impl, not the trait default"
+        "DynExecutor::checkpoint_redo must forward to the real impl, not the trait default"
     );
     assert!(file_path.exists());
 }

src/agent_setup.rs

@@ -599,7 +599,7 @@ pub(crate) async fn build_tool_setup(
     let diagnostics_executor = build_diagnostics_executor(config);
     let base_executor = build_base_executor_chain(
         file_executor,
-        shell_executor,
+        zeph_tools::DynExecutor(shell_executor),
         scrape_executor,
         diagnostics_executor,
         config