Skip to content

fix(tools): forward checkpoint methods through Arc<ShellExecutor>#5998

Merged
bug-ops merged 1 commit into
mainfrom
fix/5985-shellexecutor-checkpoint-forward
Jul 10, 2026
Merged

fix(tools): forward checkpoint methods through Arc<ShellExecutor>#5998
bug-ops merged 1 commit into
mainfrom
fix/5985-shellexecutor-checkpoint-forward

Conversation

@bug-ops

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

Copy link
Copy Markdown
Owner

Summary

Closes #5985

Test plan

  • Added arc_wrapped_executor_forwards_checkpoint_methods regression test in crates/zeph-tools/src/shell/tests.rs, constructing a real Arc<ShellExecutor> and calling checkpoint methods through the Arc handle as a dyn ToolExecutor — independently verified by two reviewers to fail pre-fix and pass post-fix (not a false-positive test).
  • cargo +nightly fmt --check
  • cargo clippy --profile ci --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler,testing" -- -D warnings
  • cargo nextest run --workspace --features "desktop,ide,server,chat,pdf,scheduler" --lib --bins — 12646 passed, 0 failed
  • cargo nextest run -p zeph-tools (full crate) — 1355 passed, 0 failed

Follow-ups (not in scope for this PR, to be filed separately)

  • DynSchedulerExecutor (src/scheduler_executor.rs:519) is a structurally similar shadow-wrapper pattern — parity-correct today but the same footgun shape.
  • Latent P2: capture_snapshot_for silently drops checkpoints for newly-created files under symlinked allowed_paths prefixes on macOS (canonicalize()/absolute() don't resolve /tmp/private/tmp), only a tracing::warn!, no user-visible error.
  • P3 test gap: no existing test covers checkpoint_redo-with-no-prior-undo, checkpoint_list ordering with 2+ entries, or multi-checkpoint undo-stack depth (pre-existing gap, not introduced by this fix).

impl ToolExecutor for std::sync::Arc<ShellExecutor> predates the
checkpoint feature and never overrode checkpoint_undo/redo/list, so
they silently fell through to the trait's unsupported default. Since
agent_setup.rs wraps the shell executor in an Arc before building the
executor chain, every checkpoint call resolved against this incomplete
impl instead of ShellExecutor's own correct one, breaking /undo, /redo,
and /undo list in the plain default production wiring.

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

Labels

bug Something isn't working documentation Improvements or additions to documentation rust Rust code changes size/M Medium PR (51-200 lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

checkpoint_undo/redo/list never reach ShellExecutor — Arc<ShellExecutor>'s own ToolExecutor impl doesn't forward them

1 participant