Description
PR #6223 (closing #6000) deleted DynSchedulerExecutor — a hand-maintained Arc<SchedulerExecutor> ToolExecutor wrapper — in favor of the existing zeph_tools::DynExecutor/ErasedToolExecutor erasure adapter, which forwards all 13 trait methods by construction.
impl ToolExecutor for Arc<ShellExecutor> (crates/zeph-tools/src/shell/mod.rs) is a second, still-existing hand-forwarding wrapper of the same general shape. It is NOT buggy today — it correctly forwards all 13 methods (fixed in #5985/PR #5998) — so this is purely a consistency/tech-debt item, not a defect.
After #6223, the codebase has two different patterns for adapting an Arc-wrapped executor to ToolExecutor:
zeph_tools::DynExecutor(Arc<dyn ErasedToolExecutor>) — the erasure adapter, forwards by construction.impl ToolExecutor for Arc<ShellExecutor> — a bespoke, correct-but-manual impl.
Having two patterns for the same problem is a minor coherence smell that could confuse future maintainers deciding which pattern to follow for a new executor.
Suggested Fix
Migrate Arc<ShellExecutor>'s ToolExecutor impl to use DynExecutor instead, removing the bespoke impl. Verify all call sites that currently rely on Arc<ShellExecutor>: ToolExecutor directly (there are more call sites than the single one DynSchedulerExecutor had, per the #6000 review) still compile and behave identically.
Notes
Raised as a non-blocking follow-up during code review of PR #6223 (#6000 fix) — both the review and adversarial critique passes on that PR confirmed Arc<ShellExecutor> itself is not buggy, so this is scoped as optional consistency cleanup rather than an active bug.
Description
PR #6223 (closing #6000) deleted
DynSchedulerExecutor— a hand-maintainedArc<SchedulerExecutor>ToolExecutorwrapper — in favor of the existingzeph_tools::DynExecutor/ErasedToolExecutorerasure adapter, which forwards all 13 trait methods by construction.impl ToolExecutor for Arc<ShellExecutor>(crates/zeph-tools/src/shell/mod.rs) is a second, still-existing hand-forwarding wrapper of the same general shape. It is NOT buggy today — it correctly forwards all 13 methods (fixed in #5985/PR #5998) — so this is purely a consistency/tech-debt item, not a defect.After #6223, the codebase has two different patterns for adapting an
Arc-wrapped executor toToolExecutor:zeph_tools::DynExecutor(Arc<dyn ErasedToolExecutor>)— the erasure adapter, forwards by construction.impl ToolExecutor for Arc<ShellExecutor>— a bespoke, correct-but-manual impl.Having two patterns for the same problem is a minor coherence smell that could confuse future maintainers deciding which pattern to follow for a new executor.
Suggested Fix
Migrate
Arc<ShellExecutor>'sToolExecutorimpl to useDynExecutorinstead, removing the bespoke impl. Verify all call sites that currently rely onArc<ShellExecutor>: ToolExecutordirectly (there are more call sites than the single oneDynSchedulerExecutorhad, per the #6000 review) still compile and behave identically.Notes
Raised as a non-blocking follow-up during code review of PR #6223 (#6000 fix) — both the review and adversarial critique passes on that PR confirmed
Arc<ShellExecutor>itself is not buggy, so this is scoped as optional consistency cleanup rather than an active bug.