fix(tools): forward requires_confirmation/is_tool_speculatable/execute_tool_call_confirmed through remaining ToolExecutor wrappers - #6011
Merged
Conversation
…e_tool_call_confirmed through remaining ToolExecutor wrappers CompositeExecutor did not forward requires_confirmation or execute_tool_call_confirmed, and AdversarialPolicyGateExecutor did not forward is_tool_speculatable, so all three fell through to the trait's default and silently dropped whatever the wrapped chain actually reported. PolicyGateExecutor and AdversarialPolicyGateExecutor also did not forward requires_confirmation, masking TrustGateExecutor's real policy whenever tools.policy or tools.adversarial_policy is enabled. Add the missing forwards: OR-forwarding for requires_confirmation and first-match-wins for execute_tool_call_confirmed in CompositeExecutor (mirroring the existing is_tool_retryable/is_tool_speculatable and execute_confirmed overrides), plain delegation to self.inner in the two gate executors. Same defect class as #5899/#5905/#5906 (fixed by #5930), scoped out of that PR to keep it minimal. Closes #5900, #5938, #5931
bug-ops
enabled auto-merge (squash)
July 11, 2026 12:43
This was referenced Jul 11, 2026
bug-ops
added a commit
that referenced
this pull request
Jul 11, 2026
…ckpoint drop, add checkpoint-stack tests CompressedExecutor, ToolFilter, and Arc<ShellExecutor> now forward every cross-cutting ToolExecutor method to their inner/wrapped executor instead of silently falling through to trait defaults, closing the same shadow-impl forwarding gap fixed for other wrappers in #5930 and #6011. capture_snapshot_for no longer drops a checkpoint for a newly-created file under a symlinked allowed_paths prefix on macOS: a new canonicalize_or_nearest_ancestor helper resolves symlinks via the nearest existing ancestor when the target path itself does not exist yet. Adds regression coverage for the ShellExecutor checkpoint stack: redo without a prior undo, checkpoint_list ordering, and multi-step undo/redo depth bookkeeping.
bug-ops
added a commit
that referenced
this pull request
Jul 11, 2026
…ckpoint drop, add checkpoint-stack tests CompressedExecutor, ToolFilter, and Arc<ShellExecutor> now forward every cross-cutting ToolExecutor method to their inner/wrapped executor instead of silently falling through to trait defaults, closing the same shadow-impl forwarding gap fixed for other wrappers in #5930 and #6011. capture_snapshot_for no longer drops a checkpoint for a newly-created file under a symlinked allowed_paths prefix on macOS: a new canonicalize_or_nearest_ancestor helper resolves symlinks via the nearest existing ancestor when the target path itself does not exist yet. Adds regression coverage for the ShellExecutor checkpoint stack: redo without a prior undo, checkpoint_list ordering, and multi-step undo/redo depth bookkeeping.
bug-ops
added a commit
that referenced
this pull request
Jul 11, 2026
…ckpoint drop, add checkpoint-stack tests (#6036) CompressedExecutor, ToolFilter, and Arc<ShellExecutor> now forward every cross-cutting ToolExecutor method to their inner/wrapped executor instead of silently falling through to trait defaults, closing the same shadow-impl forwarding gap fixed for other wrappers in #5930 and #6011. capture_snapshot_for no longer drops a checkpoint for a newly-created file under a symlinked allowed_paths prefix on macOS: a new canonicalize_or_nearest_ancestor helper resolves symlinks via the nearest existing ancestor when the target path itself does not exist yet. Adds regression coverage for the ShellExecutor checkpoint stack: redo without a prior undo, checkpoint_list ordering, and multi-step undo/redo depth bookkeeping.
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
1 participant
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
CompositeExecutor::requires_confirmationnow OR-forwards to bothfirst/secondinner executors, mirroring the existingis_tool_retryable/is_tool_speculatablepattern. Previously fell through to the trait default (false), silently discarding whatever the wrapped chain actually reported (CompositeExecutor does not forward requires_confirmation; AdversarialPolicyGateExecutor does not forward is_tool_speculatable #5900).CompositeExecutor::execute_tool_call_confirmednow first-match-wins forwards, mirroring the existingexecute_confirmedoverride. Previously fell through to the trait default, which re-dispatched viaexecute_tool_calland re-ran the full (still-gating) confirmation check instead of bypassing it (CompositeExecutor does not override execute_tool_call_confirmed, silently re-running the full confirmation check on the composed tree #5938).AdversarialPolicyGateExecutor::is_tool_speculatableandAdversarialPolicyGateExecutor/PolicyGateExecutor::requires_confirmationnow plain-delegate toself.inner(CompositeExecutor does not forward requires_confirmation; AdversarialPolicyGateExecutor does not forward is_tool_speculatable #5900, PolicyGateExecutor/AdversarialPolicyGateExecutor do not forward requires_confirmation #5931).Same defect class as #5899/#5905/#5906, fixed by #5930 — these three wrapper layers were explicitly scoped out of that PR to keep it minimal. Confirmed via direct code read that #5905/#5906 (open on GitHub only due to a comma-separated
Closesauto-link limitation) and #5985 (Arc<ShellExecutor>checkpoint forwarding) are already fixed — no duplicate work in this PR.An adversarial critique pass (verdict: minor, non-blocking) flagged that the same latent gap exists in three sibling wrappers not covered by these three issues:
CompressedExecutor(missingrequires_confirmation),ToolFilter(forwards none of the cross-cutting methods), andArc<ShellExecutor>-style shadow-impls (partial). All are confirmed dormant —TrustGateExecutoris unconditionally the outermost confirmation authority in production wiring, so nothing is exploitable today. Kept out of scope for this PR; a follow-up issue will be filed separately.Test plan
cargo +nightly fmt --checkcleancargo clippy --profile ci --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler,testing" -- -D warningscleancargo nextest run --config-file .github/nextest.toml --workspace --features "desktop,ide,server,chat,pdf,scheduler" --lib --bins— 12699/12699 passedRUSTFLAGS="-D warnings" RUSTDOCFLAGS="--deny rustdoc::broken_intra_doc_links" cargo doc --no-deps --workspace --features "desktop,ide,server,chat,pdf,scheduler"cleancomposite.rsx6,adversarial_gate.rsx2,policy_gate.rsx1), each using a spy/stub inner executor returning a distinguishable non-default value; theexecute_tool_call_confirmedtest additionally asserts the unconfirmed path was not touchedCloses #5900, #5938, #5931