Skip to content

arch: recurring ToolExecutor wrapper-forwarding defect class needs a compiler-enforced fix, not more one-off patches #6019

Description

@bug-ops

Description

The ToolExecutor wrapper-forwarding gap (a decorator's own impl ToolExecutor for Wrapper<T> block omits an override for a cross-cutting trait method, silently falling through to the trait's permissive default instead of forwarding to self.inner) has now recurred as at least three separate, independently-discovered groups within roughly two months:

  1. checkpoint_undo/redo/list not forwarded by TrustGateExecutor/PolicyGateExecutor/AdversarialPolicyGateExecutor, breaking /undo in standard production wiring #5899/checkpoint_undo/redo/list not forwarded by ScopedToolExecutor/ShadowProbeExecutor — breaks /undo even after #5899, since these sit outside the gated chain #5905/ScopedToolExecutor does not forward requires_confirmation, defaulting to false for the outermost wrapper #5906 (checkpoint trio + requires_confirmation on TrustGateExecutor/PolicyGateExecutor/AdversarialPolicyGateExecutor/ScopedToolExecutor) — fixed by fix(tools): forward checkpoint and confirmation methods through executor wrappers #5930
  2. CompositeExecutor does not forward requires_confirmation; AdversarialPolicyGateExecutor does not forward is_tool_speculatable #5900/CompositeExecutor does not override execute_tool_call_confirmed, silently re-running the full confirmation check on the composed tree #5938/PolicyGateExecutor/AdversarialPolicyGateExecutor do not forward requires_confirmation #5931 (requires_confirmation/is_tool_speculatable/execute_tool_call_confirmed on CompositeExecutor/AdversarialPolicyGateExecutor/PolicyGateExecutor) — fixed by fix(tools): forward requires_confirmation/is_tool_speculatable/execute_tool_call_confirmed through remaining ToolExecutor wrappers #6011
  3. checkpoint_undo/redo/list never reach ShellExecutor — Arc<ShellExecutor>'s own ToolExecutor impl doesn't forward them #5985 (Arc<ShellExecutor> shadow-impl checkpoint forwarding) — fixed by fix(tools): forward checkpoint methods through Arc<ShellExecutor> #5998
  4. CompressedExecutor/ToolFilter/Arc<ShellExecutor>-style shadow-impls have the same ToolExecutor forwarding gap as #5900/#5938/#5931 #6012 (open) — CompressedExecutor/ToolFilter/Arc<ShellExecutor>-style shadow-impls, same shape again, plus the CompressedExecutor checkpoint-trio gap noted in a follow-up comment on that issue

Every occurrence has the same root cause: ToolExecutor (crates/zeph-tools/src/executor.rs:676) gives every cross-cutting method (requires_confirmation, is_tool_speculatable, execute_tool_call_confirmed, checkpoint_undo/checkpoint_redo/checkpoint_list, is_tool_retryable, set_skill_env, set_effective_trust) a permissive default. Rust does not warn when a trait impl silently relies on a default instead of overriding it — there is no compiler signal distinguishing "this wrapper intentionally has no confirmation policy" from "this wrapper's author forgot to forward requires_confirmation to self.inner". Each fix so far has been a manual, per-PR audit of "which wrapper is missing which method this time", which does not scale and has already missed things twice within the same review cycle (#6011's own follow-up issue #6012 already needed a same-day amendment for a checkpoint-trio gap the issue author didn't catch on the first pass).

The trait doc itself already flags an adjacent instance of "manual duplication where the compiler won't catch a missed method" for the separate ToolExecutor/ErasedToolExecutor two-trait split (executor.rs:669-673, "D2 — deferred: consolidate ToolExecutor and ErasedToolExecutor... every new method must be added to both traits"), so this is a known, named pattern in the codebase's own TODOs, not a novel observation.

Proposed Fix (needs an architectural decision, not a code PR)

Two viable directions, either would close this permanently:

Option A — delegation macro. Most affected wrappers (ToolFilter, CompressedExecutor, ScopedToolExecutor, ShadowProbeExecutor, PolicyGateExecutor, AdversarialPolicyGateExecutor, TrustGateExecutor) hold exactly one inner: T field and plain-delegate the majority of methods, overriding only a handful for wrapper-specific behavior (e.g. execute, execute_confirmed). This is the textbook use case for a trait-delegation macro (e.g. the ambassador crate, which supports #[delegate(Trait, target = "inner")] with per-method #[delegate(automatic)]/skip overrides and has async-trait support) or a small hand-rolled macro_rules! that expands to a full pass-through impl block, with wrapper-specific methods written manually afterward and shadowing the macro-generated ones. Precedent in this codebase: #299 ("refactor: AnyProvider delegate macro", closed) solved the analogous problem for AnyProvider. CompositeExecutor (dual-inner, OR-forwarding semantics) doesn't fit the single-target delegate pattern and would keep its current hand-written impl — it's a smaller, already-well-tested surface.

Option B — remove defaults for the risk-bearing subset. Make requires_confirmation, checkpoint_undo, checkpoint_redo, checkpoint_list, is_tool_speculatable, and execute_tool_call_confirmed required (no default body) on ToolExecutor. Every current impl site (leaf and wrapper) must then explicitly implement them, which is a one-time compile-error-driven audit across the ~19 files with impl ToolExecutor for — after that, no future wrapper can silently inherit a wrong default, because the compiler forces an explicit choice at the call site. Leaf executors that genuinely have no checkpoint/confirmation semantics just write the same trivial body they'd get from the default today (CheckpointActionResult::unsupported(), false, etc.) — the point isn't to change behavior, it's to make "I looked at this method and chose the default" distinguishable from "I forgot this method exists" at compile time.

A is less invasive to leaf executors but requires picking and vetting a new dependency (or maintaining a hand-rolled macro); B needs zero new dependencies and is arguably more idiomatic for a trait this security-sensitive, but touches every existing impl site once.

Impact

Not itself a live vulnerability — every confirmed instance so far has been caught before reaching a currently-reachable production wiring (see #6012's dormancy note). The impact is velocity and review cost: this is the 4th grouped discovery of the same shape, each requiring a full manual re-audit of every impl ToolExecutor for site, and each audit has had a nonzero miss rate within the same session (#6012 itself needed same-day amendment). Left unaddressed, it will keep recurring every time a new cross-cutting method is added to the trait or a new wrapper is introduced.

Related

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

P1High ROI, low complexity — do next sprintarchArchitecture and designtech-debtTechnical debt

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions