fix(tools)!: remove permissive defaults on ToolExecutor/ErasedToolExecutor#6067
Merged
Merged
Conversation
…cutor Closes the recurring wrapper-forwarding defect class where a decorator's impl block silently omits a cross-cutting method override and falls back to the trait's permissive default instead of forwarding to its inner executor. This has recurred five times in two months across separate, independently-discovered issues, each fixed with a one-off manual patch that did not prevent the next occurrence. Removes the default bodies for requires_confirmation, execute_tool_call_confirmed, checkpoint_undo/redo/list, and is_tool_speculatable on both ToolExecutor and ErasedToolExecutor, making them required at every impl site. Adds four macro_rules! helpers in the new crates/zeph-tools/src/executor_delegate.rs to absorb the resulting boilerplate: a forward macro and a no-inner-defaults macro for each trait. Fixes three production ErasedToolExecutor wrappers with live forwarding gaps on the subagent tool-dispatch path: FilteredExecutor and PlanModeExecutor now forward the checkpoint trio, speculatable, and trust-cap propagation; MemoryAwareExecutor's confirmed-call path replicates its existing sandbox-violation fallback instead of silently dropping it. BREAKING CHANGE: ToolExecutor and ErasedToolExecutor no longer provide default bodies for requires_confirmation, execute_tool_call_confirmed, checkpoint_undo/redo/list, and is_tool_speculatable (and their _erased counterparts). External implementors must provide explicit bodies for all six methods. Closes #6019
bug-ops
force-pushed
the
feat/issue-6019/toolexecutor-forwarding-fix
branch
from
July 11, 2026 18:02
3f1dba8 to
7d41a23
Compare
bug-ops
enabled auto-merge (squash)
July 11, 2026 18:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the recurring
ToolExecutor/ErasedToolExecutorwrapper-forwarding defect class: a decorator's ownimplblock silently omits an override for a cross-cutting method and falls back to the trait's permissive default instead of forwarding toself.inner. This has recurred five times in roughly two months (#5899/#5905/#5906, #5900/#5938/#5931, #5985, #6012, and a 5th recurrence inside the very commit that closed #6012), each fixed with a one-off manual patch that did not prevent the next occurrence.Mechanism (Option B + macro assist, architecturally reviewed and approved across two rounds):
requires_confirmation,execute_tool_call_confirmed,checkpoint_undo/checkpoint_redo/checkpoint_list, andis_tool_speculatableon bothToolExecutorandErasedToolExecutor(crates/zeph-tools/src/executor.rs). This makes them required, so the compiler forces an explicit choice at every impl site instead of allowing a silent fallback.macro_rules!helpers in the newcrates/zeph-tools/src/executor_delegate.rsto absorb the resulting boilerplate: a forward macro and a no-inner-defaults macro for each trait.ErasedToolExecutorwrappers on the subagent tool-dispatch path with live forwarding gaps, discovered during architecture review:FilteredToolExecutor/PlanModeExecutor(crates/zeph-subagent/src/filter.rs) — now forward the checkpoint trio andis_tool_speculatable_erased;PlanModeExecutoralso now forwardsset_effective_trust, which was previously silently dropped.MemoryAwareExecutor(crates/zeph-subagent/src/manager/spawn.rs) — its confirmed-call path now replicates the existing sandbox-violation-to-memory-executor fallback instead of bypassing it.src/, roottests/, and scattered#[cfg(test)]modules that a manualcrates/-only enumeration missed. This is the intended behavior of the compile-time-guarantee approach.ambassadordelegation-macro crate (RPITIT-incompatible with this trait's native-async-fn methods, and all-or-nothing per trait while most wrappers specializeexecute) and pure macro-only forwarding (still relies on remembering to invoke the macro — same review-dependent gap).Process
Two architecture review rounds: the first plan only covered
ToolExecutor, and the critic caught thatErasedToolExecutor— the actual substrate the subagent system dispatches through — was left out with 13 direct impl sites including 3 production wrappers with live gaps. The revised plan folded both traits in and was approved. Implementation was independently validated by four parallel reviewers (tests, performance, security, adversarial critique) with no blocking findings, then went through one code-review round (a documentation-only correction: an invented method nameset_effective_trust_erasedin the CHANGELOG/playbook that doesn't exist on either trait) before approval.Test plan
cargo +nightly fmt --checkcargo clippy --profile ci --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler,testing" -- -D warningscargo clippy --profile ci --workspace --all-targets --features bench -- -D warnings(zeph-bench files touched)cargo nextest run --config-file .github/nextest.toml --workspace --features "desktop,ide,server,chat,pdf,scheduler" --lib --bins— 12815 passed, 0 failed, 35 skipped (reproduced independently three times across the review chain)cargo test --doc --workspace --features "desktop,ide,server,chat,pdf,scheduler"— all 34 crate doctest binaries, 0 failedRUSTFLAGS="-D warnings" RUSTDOCFLAGS="--deny rustdoc::broken_intra_doc_links" cargo doc --no-deps --workspace --features "desktop,ide,server,chat,pdf,scheduler"Closes #6019