Skip to content

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

Merged
bug-ops merged 1 commit into
mainfrom
feat/issue-6224/shell-executor-dynexecutor
Jul 13, 2026
Merged

refactor(tools): migrate Arc<ShellExecutor> to DynExecutor erasure path#6230
bug-ops merged 1 commit into
mainfrom
feat/issue-6224/shell-executor-dynexecutor

Conversation

@bug-ops

@bug-ops bug-ops commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Summary

  • Removes the hand-maintained impl ToolExecutor for Arc<ShellExecutor> in crates/zeph-tools/src/shell/mod.rs, a bespoke forwarding wrapper structurally identical to the DynSchedulerExecutor anti-pattern removed in refactor(scheduler): route DynSchedulerExecutor through DynExecutor erasure path #6223.
  • The shell executor's production composition slot (src/agent_setup.rs) now wraps it as zeph_tools::DynExecutor(Arc<ShellExecutor>), flowing through the existing ErasedToolExecutor erasure adapter, which forwards all 13 ToolExecutor methods by construction instead of by hand.
  • Removing the wrapper exposed a real, previously-masked gap: impl ToolExecutor for ShellExecutor itself never overrode execute_confirmed (only execute). The old code was accidentally correct only because the deleted wrapper's override called self.as_ref().execute_confirmed(...), which Rust's inherent-method-priority rule silently resolved to ShellExecutor's separate inherent execute_confirmed method rather than the trait method. Once routed through DynExecutor's dynamic dispatch, that accidental shadowing no longer applies, and the trait's default (self.execute(..), no confirmation bypass) would have kicked in — reintroducing CompressedExecutor/ToolFilter/Arc<ShellExecutor>-style shadow-impls have the same ToolExecutor forwarding gap as #5900/#5938/#5931 #6012. Fixed by adding an explicit execute_confirmed override on ShellExecutor forwarding to the same execute_inner(response, true) the inherent method already uses.
  • Independently verified (developer, critic, and tester passes) that this gap is not reachable from the live agent turn loop today — only the structured execute_tool_call(_confirmed) path is used in production; the string-response execute/execute_confirmed path is otherwise dead code. The fix is still required for the refactor's own "behaves identically" contract and closes the gap for any future/erased caller.
  • shell_executor_handle (used only for TUI background-run metrics via the concrete ShellExecutor::background_runs_snapshot() inherent method) is unaffected.
  • 3 regression tests in crates/zeph-tools/src/shell/tests.rs renamed from arc_shell_executor_* to dyn_executor_* and adapted to exercise the same invariants (CompressedExecutor/ToolFilter/Arc<ShellExecutor>-style shadow-impls have the same ToolExecutor forwarding gap as #5900/#5938/#5931 #6012, checkpoint_undo/redo/list never reach ShellExecutor — Arc<ShellExecutor>'s own ToolExecutor impl doesn't forward them #5985) through the new DynExecutor-wrapped path.

Closes #6224

Test plan

  • cargo +nightly fmt --check
  • cargo clippy --profile ci --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler,testing" -- -D warnings
  • cargo nextest run --config-file .github/nextest.toml --workspace --features "desktop,ide,server,chat,pdf,scheduler" --lib --bins — 13453 passed, 35 skipped
  • RUSTFLAGS="-D warnings" RUSTDOCFLAGS="--deny rustdoc::broken_intra_doc_links" cargo doc --no-deps --workspace --features "desktop,ide,server,chat,pdf,scheduler"
  • Verified the adapted dyn_executor_execute_confirmed_bypasses_confirmation regression test genuinely fails without the execute_confirmed fix (reverted locally, confirmed failure, restored)
  • Confirmed no other Arc<ShellExecutor> production call site depends on the removed impl (only production site was agent_setup.rs, now migrated)
  • Confirmed no other ToolExecutor implementor shares the same execute_confirmed override gap

@github-actions github-actions Bot added documentation Improvements or additions to documentation rust Rust code changes refactor Code refactoring without functional changes size/M Medium PR (51-200 lines) labels Jul 13, 2026
@bug-ops
bug-ops enabled auto-merge (squash) July 13, 2026 17:18
@bug-ops
bug-ops force-pushed the feat/issue-6224/shell-executor-dynexecutor branch 2 times, most recently from eecff8e to 8f61348 Compare July 13, 2026 17:30
Removes the hand-maintained impl ToolExecutor for Arc<ShellExecutor> in
favor of the existing DynExecutor/ErasedToolExecutor adapter, mirroring
the DynSchedulerExecutor migration in #6223. Removing the wrapper exposed
a masked gap: ShellExecutor's ToolExecutor impl never overrode
execute_confirmed, silently falling back to the no-bypass trait default
for any caller reaching it through a generic/erased path. Adds the
missing override so the fix applies uniformly across all dispatch paths.

Closes #6224
@bug-ops
bug-ops force-pushed the feat/issue-6224/shell-executor-dynexecutor branch from 8f61348 to 57aa468 Compare July 13, 2026 17:40
@bug-ops
bug-ops merged commit 78accd4 into main Jul 13, 2026
43 checks passed
@bug-ops
bug-ops deleted the feat/issue-6224/shell-executor-dynexecutor branch July 13, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation refactor Code refactoring without functional changes rust Rust code changes size/M Medium PR (51-200 lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tech-debt(tools): migrate Arc<ShellExecutor> hand-forwarding ToolExecutor impl to DynExecutor for consistency

1 participant