MCP: action tools wait for backend ack before returning OK#18
Merged
Conversation
1 task
- Real QA paper-cut: action tools returned `OK` the instant they dispatched an event, even when the FE was stalled (modal blocking input, error pane up, race during startup). The action was silently dropped but MCP reported success.
- Introduce `executor/ack.rs` with a typed `AckSignal` enum (`GenerationAdvanced`, `SoftDialogAppeared`, `WindowAppeared`, `WindowDisappeared`, `Any`) and a `wait_for_ack` helper. 1500 ms budget, polled at 250 ms (state) / 100 ms (windows). Not exposed as a tool param — it's a backend latency budget.
- Wrap every fire-and-forget action tool (`copy`, `move`, `delete`, `mkdir`, `mkfile`, `refresh`, `select`, `toggle_hidden`, `set_view_mode`, `sort`, `tab`, `open_under_cursor`, `nav_to_parent`, `nav_back`, `nav_forward`, and `dialog` open/close/focus/confirm). On timeout the tool returns a typed `ToolError::internal` whose message names the missing signal and the elapsed budget.
- `open_under_cursor` uses `Any(GenerationAdvanced, WindowAppeared("viewer"))` because directories bump pane state and files open a viewer window.
- Bump generation in `update_pane_tabs` too — without it the `tab` tool would time out on every call since tab pushes bypass `set_left`/`set_right`.
- Tests: 11 new unit tests pinning down the ack contract's primitives (generation gate flips, `update_pane_tabs` bumps, soft dialog ID matching, FE-stalled stays false). E2E behavior is covered by the existing Playwright MCP suite — only place a "FE actually stalled" scenario can be reproduced faithfully.
- Docs: `mcp/CLAUDE.md` gets a new "Action-tool ack contract" section and a Decision/Why entry; `docs/tooling/mcp.md` documents the new contract for adopting agents.
E2E flagged the right edge case: when the FE re-lists in response to `mcp-refresh` and the new listing is byte-identical to the cached state (common on MTP/SMB right after an operation already pushed fresh state), the FE skips the `update_*_pane_state` call to avoid a redundant generation bump. That left `refresh`'s ack helper waiting for a signal that never arrived, timing out at 1500 ms. - Revert `execute_refresh` to fire-and-forget with a `// TODO(mcp-ack):` comment documenting the two acceptable follow-ups (round-trip ack, or always-bump-generation on re-list). - Update both CLAUDE.md and the user-facing `docs/tooling/mcp.md` to call out that `refresh` is the one exception in the contract. - Every other ack-wrapped tool stays as-is; the smb/mtp E2E suite exercises `copy`/`move`/`delete`/`open_under_cursor`/`move_cursor` and they all passed against the new contract.
Real-MCP testing of the ack contract caught two false negatives the unit + Playwright coverage missed: - `dialog close <confirmation>` (mkdir / mkfile / transfer / delete): the close path waited on `GenerationAdvanced`, but cancelling a confirmation dialog doesn't touch pane state, so the bump never arrives. Every cancel returned `ActionNotAcknowledged` even though the dialog actually closed. - `dialog close settings`: the Rust side emitted `mcp-settings-close` to the settings webview but `routes/settings/+page.svelte` had no listener. Tool returned `ActionNotAcknowledged` AND the window stayed open — straight-up broken, not a false negative. Add a new `AckSignal::SoftDialogDisappeared(id)` that waits for the `SoftDialogTracker` membership to drop (the `notifyDialogClosed` IPC from `ModalDialog.svelte` is the trustworthy signal). Wire the soft-dialog-close path in `dialogs.rs` to it. Add the missing `mcp-settings-close` listener with the same two-rAF defer as the production Escape handler, so the close behaves consistently regardless of trigger. Verified end-to-end via the real MCP transport: settings open/close in ~150 ms; mkdir/mkfile/transfer/delete confirmation cancels in ~130–170 ms; all 176 MCP unit tests + clippy + rustfmt + oxfmt + svelte-check green.
vdavid
force-pushed
the
feature/mcp-action-ack
branch
from
7c87113 to
39f204d
Compare
Fixes a batch of issues found while reviewing the original ack-contract
PR end-to-end via real MCP traffic.
- `dialog close about` now waits for `SoftDialogDisappeared("about")`
instead of `WindowDisappeared("about")`. The about dialog is a soft
overlay, not a Tauri window, so the prior signal was always
immediately true and the close was effectively fire-and-forget. Now
it ack's on the tracker losing the id; absent → instant OK, present
→ wait for the FE to close it.
- `dialog close file-viewer` by path no longer hangs on the
ALL-viewers-gone signal. Added `AckSignal::WindowCountBelow {
prefix, threshold }` + `snapshot_window_count` helper, so the tool
snapshots the viewer count, dispatches, and ack's when the count
drops by one. Close-all uses `threshold: 1`. Fast-fail with
`invalid_params` when zero viewers are open instead of timing out.
- `open_under_cursor` no longer false-times-out on files. The OS-open
branch produces neither a state push nor a viewer window, so the
previous `Any([GenerationAdvanced, WindowAppeared("viewer")])`
signal would always time out at 5 s. Routed through
`mcp_round_trip` instead: FE awaits `handleNavigate(entry)` and
signals completion explicitly. Removed the now-unused `AckSignal::Any`
variant; round-trip is the only honest ack for multi-mode commands.
- `nav_to_parent` / `nav_back` / `nav_forward` get a `NAV_ACK_TIMEOUT`
of 5 s (up from 1500 ms) because a parent/back jump can land on an
SMB or MTP path whose directory listing takes a few seconds even
on success.
- `update_pane_tabs` now delegates to `PaneStateStore::set_tabs`, a
new helper that owns both the tab-list write and the generation bump.
Eliminates the inline `pane.write().unwrap().tabs = ...` +
`generation.fetch_add(1, Relaxed)` pair that a future contributor
would forget to keep in sync.
- Documented the `GenerationAdvanced` TOCTOU caveat (an unrelated
state push between snapshot and dispatch can satisfy the signal) and
the `SoftDialogDisappeared` test-context asymmetry (returns true
when the tracker isn't registered, so close paths don't need a
fixture).
- `mcp/CLAUDE.md`, `docs/tooling/mcp.md`, the ack-signal table, and
the round-trip list updated. `bindings.ts` regenerated for the
refreshed `update_pane_tabs` doc-comment.
End-to-end verified via the real MCP transport: dialog open/close for
settings/about/file-viewer (single and multi-viewer); mkdir/mkfile
cancel; copy/move/delete autoConfirm and manual; tab new/close;
toggle_hidden/set_view_mode/sort; nav_to_parent/back/forward;
open_under_cursor on both a .txt file (OS-opens) and a directory
(navigates). All Rust checks (1907 unit + 30 integration tests,
clippy, rustfmt, cargo-deny, cargo-audit) and Svelte (svelte-check,
1928 vitest tests, oxfmt) green.
oxfmt picked these up while running checks on a feature branch: re-wraps long lines and aligns the `scripts/check/CLAUDE.md` table's column padding. No semantic changes.
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.
Real QA paper-cut: MCP action tools returned
OKthe instant they dispatched an event to the FE. If the FE was stalled (modal blocking input, error pane up, race during startup), the action was silently dropped but MCP reported success. The whole point of MCP-driven automation is to trust thoseOKs; this broke that trust.Fix: every fire-and-forget action tool now waits for a typed backend ack signal before returning. On timeout (default 1500 ms), the tool returns a typed
ToolError::internalwhose message names the missing signal and the elapsed budget. Input/output shapes for every tool are unchanged —OKjust becomes a meaningful contract.What changed:
executor/ack.rswithAckSignalenum (GenerationAdvanced,SoftDialogAppeared,WindowAppeared,WindowDisappeared,Any) and await_for_ackhelper. Polled at 250 ms for state-driven signals (matching the existingawaittool), 100 ms for window-driven signals (windows show up faster than full pane state pushes).Durationargument.copy,move,delete,mkdir,mkfile,refresh,select,toggle_hidden,set_view_mode,sort,tab,open_under_cursor,nav_to_parent,nav_back,nav_forward, and thedialogtool's open/close/focus/confirm actions.copy/move/deletewithautoConfirm: trueack viaGenerationAdvanced; withautoConfirm: falsethey ack via the matching confirmation soft dialog appearing (transfer-confirmation,delete-confirmation).mkdir/mkfileack viamkdir-confirmation/new-file-confirmationsoft dialogs.open_under_cursorusesAny(GenerationAdvanced, WindowAppeared("viewer"))because directories bump pane state but files open a viewer window.update_pane_tabsnow bumps the generation counter too — without it thetabtool would time out on every call since tab pushes bypassset_left/set_right.mcp/tests/ack_system_tests.rspin down the contract's primitives: generation gate flips on/off correctly,update_pane_tabsbumps generation,SoftDialogTrackerID semantics match what the ack helper checks. The doc-comment matrix at the top of that file explains what unit tests can and can't cover (signal-check primitives, yes; fullwait_for_ackagainst a real Tauri runtime, no — that's the Playwright MCP E2E suite's job).apps/desktop/src-tauri/src/mcp/CLAUDE.mdgets a new "Action-tool ack contract" section, aDecision/Whyentry, and an updated "Tool execution is async..." gotcha.docs/tooling/mcp.mdexplains the new contract for adopting agents.Test plan