Skip to content

Commit 48a9701

Browse files
committed
MCP: action tools wait for backend ack before OK
- 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.
1 parent 1181e0c commit 48a9701

13 files changed

Lines changed: 825 additions & 63 deletions

File tree

apps/desktop/src-tauri/src/mcp/CLAUDE.md

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,30 @@ Directory module split by tool category. `mod.rs` contains the main `execute_too
5252
- `async_tools.rs`: await, connect_to_server, remove_manual_server, set_setting
5353
- `search.rs`: search index loading, search, ai_search
5454

55-
Most tools are fire-and-forget: emit a Tauri event and return "OK" immediately.
55+
**Action-tool ack contract.** Every fire-and-forget action tool now waits for a backend ack signal before returning `OK`. Previously the tool returned `OK` the instant the event was dispatched; if the FE was stalled (modal blocking input, error pane up, race during startup), the action was silently dropped and MCP reported success anyway. The ack contract makes `OK` a meaningful promise: the FE has actually processed the dispatched action.
56+
57+
The mechanism lives in `executor/ack.rs`. Each tool:
58+
59+
1. Captures a precondition snapshot (typically `snapshot_generation(app)`).
60+
2. Emits its existing event / runs its existing command.
61+
3. Calls `wait_for_ack(app, signal, DEFAULT_ACK_TIMEOUT)` — default 1500 ms.
62+
4. Returns the original `OK` string on success, or a typed `ToolError::internal` whose message names the missing signal and the elapsed budget on timeout.
63+
64+
`AckSignal` variants:
65+
66+
| Variant | Fires when | Used by |
67+
| ------------------------ | ----------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- |
68+
| `GenerationAdvanced` | `PaneStateStore.generation` is strictly greater than the captured value | Anything that mutates pane state (`refresh`, `select`, `set_view_mode`, `sort`, `toggle_hidden`, `tab`, `nav_*`, auto-confirmed `copy`/`move`/`delete`, `dialog confirm`) |
69+
| `SoftDialogAppeared(id)` | A soft dialog with that ID is in `SoftDialogTracker` | Confirmation dialogs from `copy`/`move`/`delete` (autoConfirm: false), `mkdir`, `mkfile`; `dialog open about` |
70+
| `WindowAppeared(label)` | A `webview_windows()` entry matches the label (exact, or `viewer-*`) | `dialog open settings|file-viewer`, `dialog focus` |
71+
| `WindowDisappeared` | The matching `webview_windows()` entry is gone | `dialog close settings|file-viewer|about` |
72+
| `Any([...])` | Logical OR — any inner signal fires | `open_under_cursor` (directory case bumps generation, file case opens a viewer window) |
73+
74+
The polling cadence is 250 ms for state-driven signals (matching the existing `await` tool) and 100 ms for window/dialog appearance signals (windows show up faster than full pane state pushes).
75+
76+
The 1500 ms budget is a backend-side latency budget, not a client-facing knob: MCP clients shouldn't have to tune ack timeouts. Bump it per-call via the `Duration` argument to `wait_for_ack` if a specific operation has a known higher latency floor; don't expose it as a tool parameter.
77+
78+
**Note on `update_pane_tabs`.** Tab changes flow through this command (not `set_left`/`set_right`), so it also bumps the generation counter. Without that, the `tab` tool's ack would time out on every call.
5679

5780
Tools where the backend can't fully validate preconditions use `mcp_round_trip`: emit an event with a `requestId`, wait for the frontend to respond via `mcp-response` with `{ requestId, ok, error? }`, and return the actual outcome. Used by `move_cursor` and `set_setting` (5s timeout). `nav_to_path` uses `mcp_round_trip_with_timeout` with a 30s timeout because it waits for the directory listing to complete (the frontend delays the response until `handleListingComplete` fires in FilePane). Resources that need frontend data use `resource_round_trip` (same pattern but returns `data` from the response). Used by `cmdr://settings`. Use these patterns for any new tool/resource where the backend would otherwise need to replicate frontend knowledge.
5881

@@ -84,6 +107,16 @@ Directory module split by test category:
84107

85108
## Key decisions
86109

110+
### MCP action tools wait for backend ack before returning success
111+
112+
**Decision (May 2026):** Every fire-and-forget action tool waits for a typed ack signal (`AckSignal::GenerationAdvanced`, `SoftDialogAppeared`, `WindowAppeared`, `WindowDisappeared`, or `Any`) within a 1500 ms budget before returning `OK`. On timeout, the tool returns a `ToolError::internal` whose message names the missing signal and elapsed budget.
113+
114+
**Why.** Real QA hit a paper-cut: MCP tools were returning `OK` while the FE was stalled (modal blocking input, error pane up, race during startup), so the dispatched action was silently dropped. That made MCP unreliable as an automation surface. The ack contract makes `OK` a real promise: the FE actually processed the dispatched action.
115+
116+
**Why 1500 ms.** Most state pushes complete within ~100–300 ms in practice (FE debouncing, IPC round-trip). 1500 ms gives a generous margin for the slow cases (cold start, large directory listings) while still failing fast when the FE genuinely isn't responding. Latency-sensitive tools (`nav_to_path`) keep their existing higher budgets via `mcp_round_trip_with_timeout`.
117+
118+
**Why not a per-tool client-facing timeout knob.** The timeout is a backend-side latency budget, not a client concern. MCP clients shouldn't have to tune it per call — they expect tools to either succeed or report a clear failure.
119+
87120
### Why agent-centric API?
88121

89122
The original design mirrored keyboard shortcuts (43 tools like `nav_up`, `nav_down`). This forced agents to make dozens of calls to find a file. The agent-centric redesign (Jan 2026) consolidated to 24 semantic tools (`move_cursor(index=42)`, `nav_to_path("/Users")`). This reduced round-trips from 6+ reads to 1 (`cmdr://state` resource).
@@ -159,9 +192,11 @@ The `cmdr://settings` resource and `set_setting` tool both use round-trips to th
159192

160193
`volume_name` flows through `PaneState` from the FE via `update_left_pane_state` / `update_right_pane_state` on every state push (`FilePane.svelte`).
161194

162-
### Tool execution is async but mostly synchronous
195+
### Tool execution is async; action tools wait for ack
196+
197+
`execute_tool()` is an async function. Action tools follow the ack contract (see "Action-tool ack contract" above): dispatch the event, then `wait_for_ack` for a small backend-side signal before returning. The tool's reported "OK" thus means "the FE accepted the dispatched action," not "the underlying operation completed." For long-running operations (a copy of 10 GB), the agent still has to poll via the `await` tool to observe completion. The ack-contract change made the FE-accepted line meaningful — pre-May 2026, the tool returned `OK` even when the FE wasn't listening.
163198

164-
`execute_tool()` is an async function. Most tools are fire-and-forget: they emit a Tauri event and return immediately (for example, "OK: Copy dialog opened" not "OK: Files copied"). This applies even with `autoConfirm: true`: the tool returns when the operation starts, not when it completes. Three categories of async tools exist: (1) `mcp_round_trip` tools (`nav_to_path`, `move_cursor`) that wait up to 5s for the frontend to confirm success/failure, (2) search tools (`search`, `ai_search`) that load the search index via `spawn_blocking` and (for `ai_search`) call the LLM API.
199+
Three categories of latency-sensitive tools exist beyond the ack contract: (1) `mcp_round_trip` tools (`nav_to_path`, `move_cursor`, `set_setting`) that wait up to 5–30 s for an explicit `mcp-response` event with success/failure, (2) search tools (`search`, `ai_search`) that load the search index via `spawn_blocking` and (for `ai_search`) call the LLM API, (3) `select_volume` which polls until the target pane's `volume_name` matches.
165200

166201
### Error codes are JSON-RPC standard
167202

Lines changed: 244 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,244 @@
1+
//! Action-tool ack contract.
2+
//!
3+
//! MCP "action" tools used to return `OK` the instant they dispatched an event to the
4+
//! frontend. If the FE was stalled (modal blocking input, error pane up, race during
5+
//! startup), the action was silently dropped but the tool still reported success.
6+
//! Real QA hit this. To make MCP a trustworthy automation surface, every fire-and-forget
7+
//! action now waits for a small ack signal before returning.
8+
//!
9+
//! ## Signals
10+
//!
11+
//! - `GenerationAdvanced`: the `PaneStateStore` generation counter strictly advanced past
12+
//! a captured value. Use this for actions that mutate pane state (navigation, refresh,
13+
//! selection, view mode, sort, tabs, cursor moves, auto-confirmed copy/move/delete).
14+
//! - `SoftDialogAppeared`: a soft (overlay) dialog with the given ID appeared in the
15+
//! `SoftDialogTracker`. Use this for confirmation dialogs (transfer, delete, mkdir,
16+
//! mkfile) when `autoConfirm: false`.
17+
//! - `WindowAppeared` / `WindowDisappeared`: a Tauri webview window with the given label
18+
//! prefix appeared (or vanished). Use this for child windows (settings, file-viewer,
19+
//! about) and for `dialog close` actions.
20+
//! - `Any`: succeeds when any of the inner signals fire (logical OR). Used by
21+
//! `open_under_cursor` where opening a directory bumps the pane generation but opening
22+
//! a file launches a viewer window.
23+
//!
24+
//! ## Timeout
25+
//!
26+
//! Default `DEFAULT_ACK_TIMEOUT` = 1500 ms. Not exposed as an MCP-tool parameter —
27+
//! MCP clients shouldn't have to tune this, the value is a backend-side latency
28+
//! budget. Tunable per-call via the `Duration` argument to `wait_for_ack`.
29+
//!
30+
//! ## Decision/Why
31+
//!
32+
//! Polling cadence matches the existing `await` tool (250 ms for state checks, 100 ms
33+
//! for window checks, since window state changes are typically faster than full pane
34+
//! refreshes). The two loops aren't unified into a shared `poll_until` core yet: the
35+
//! `await` tool exposes a few extra knobs (per-pane conditions, after_generation gate,
36+
//! rich match summaries) that don't apply here, and the ack helper's loop is ~15 lines.
37+
//! Extracting now would be premature abstraction. Revisit if we add a third polling
38+
//! site or if the `await` tool grows AckSignal-shaped conditions.
39+
40+
use std::time::Duration;
41+
42+
use tauri::{AppHandle, Manager, Runtime};
43+
44+
use super::ToolError;
45+
use crate::mcp::dialog_state::SoftDialogTracker;
46+
use crate::mcp::pane_state::PaneStateStore;
47+
48+
/// Default ack budget. Backend-side latency budget; not a client-facing knob.
49+
pub const DEFAULT_ACK_TIMEOUT: Duration = Duration::from_millis(1500);
50+
51+
/// Polling cadence for state-driven signals. Matches the existing `await` tool.
52+
const STATE_POLL_INTERVAL: Duration = Duration::from_millis(250);
53+
54+
/// Polling cadence for window/dialog appearance signals. Windows show up faster than
55+
/// full pane state pushes, so we poll a bit tighter for snappier acks.
56+
const WINDOW_POLL_INTERVAL: Duration = Duration::from_millis(100);
57+
58+
/// What the backend should wait for to consider an action "actually processed."
59+
pub enum AckSignal {
60+
/// State generation strictly advanced past `from`.
61+
GenerationAdvanced { from: u64 },
62+
/// A soft dialog with this ID appeared in `SoftDialogTracker`.
63+
SoftDialogAppeared(&'static str),
64+
/// A Tauri webview window whose label equals (or starts with, for viewers)
65+
/// the given pattern appeared.
66+
WindowAppeared(&'static str),
67+
/// A Tauri webview window matching the pattern vanished.
68+
WindowDisappeared(&'static str),
69+
/// Succeeds when any inner signal fires. Used for tools where the ack
70+
/// kind depends on what got opened (for example `open_under_cursor`).
71+
Any(Vec<AckSignal>),
72+
}
73+
74+
impl AckSignal {
75+
/// Human-readable description for error messages.
76+
fn describe(&self) -> String {
77+
match self {
78+
AckSignal::GenerationAdvanced { from } => {
79+
format!("pane state generation > {from}")
80+
}
81+
AckSignal::SoftDialogAppeared(id) => format!("soft dialog '{id}' opened"),
82+
AckSignal::WindowAppeared(label) => format!("window '{label}' opened"),
83+
AckSignal::WindowDisappeared(label) => format!("window '{label}' closed"),
84+
AckSignal::Any(signals) => {
85+
let parts: Vec<String> = signals.iter().map(|s| s.describe()).collect();
86+
format!("any of [{}]", parts.join(", "))
87+
}
88+
}
89+
}
90+
}
91+
92+
/// Wait for an ack signal to arrive within `timeout`.
93+
///
94+
/// On success returns `Ok(())`. On timeout returns a `ToolError::internal` whose message
95+
/// names the missing signal and the elapsed budget, so callers can surface a useful
96+
/// failure rather than a false-positive OK.
97+
pub async fn wait_for_ack<R: Runtime>(
98+
app: &AppHandle<R>,
99+
signal: AckSignal,
100+
timeout: Duration,
101+
) -> Result<(), ToolError> {
102+
let start = tokio::time::Instant::now();
103+
let deadline = start + timeout;
104+
105+
// Pick the tighter cadence if any leaf signal is window-driven; this matters
106+
// for `Any` mixtures (open_under_cursor) where we want to react to a viewer
107+
// window as fast as a pane generation bump.
108+
let poll_interval = if signal_uses_windows(&signal) {
109+
WINDOW_POLL_INTERVAL
110+
} else {
111+
STATE_POLL_INTERVAL
112+
};
113+
114+
loop {
115+
if check_signal(app, &signal) {
116+
return Ok(());
117+
}
118+
119+
if tokio::time::Instant::now() >= deadline {
120+
let elapsed_ms = start.elapsed().as_millis();
121+
return Err(ToolError::internal(format!(
122+
"Action not acknowledged by backend within {} ms (waiting for: {}). The frontend may be stalled (modal blocking input, error pane up, race during startup). Inspect cmdr://state to triage.",
123+
elapsed_ms,
124+
signal.describe()
125+
)));
126+
}
127+
128+
tokio::time::sleep(poll_interval).await;
129+
}
130+
}
131+
132+
/// Check whether the signal is currently satisfied. Pure read; no side effects.
133+
fn check_signal<R: Runtime>(app: &AppHandle<R>, signal: &AckSignal) -> bool {
134+
match signal {
135+
AckSignal::GenerationAdvanced { from } => app
136+
.try_state::<PaneStateStore>()
137+
.map(|store| store.get_generation() > *from)
138+
.unwrap_or(false),
139+
AckSignal::SoftDialogAppeared(id) => app
140+
.try_state::<SoftDialogTracker>()
141+
.map(|tracker| tracker.get_open_types().iter().any(|d| d == id))
142+
.unwrap_or(false),
143+
AckSignal::WindowAppeared(pattern) => window_matches(app, pattern),
144+
AckSignal::WindowDisappeared(pattern) => !window_matches(app, pattern),
145+
AckSignal::Any(signals) => signals.iter().any(|s| check_signal(app, s)),
146+
}
147+
}
148+
149+
/// True if any Tauri webview window has a label exactly equal to `pattern`,
150+
/// or (for the `viewer` family) starting with `pattern-`.
151+
fn window_matches<R: Runtime>(app: &AppHandle<R>, pattern: &str) -> bool {
152+
let windows = app.webview_windows();
153+
// `viewer-` is a label prefix family — each opened file has its own
154+
// `viewer-<id>` window. Other tracked labels (settings, about) are exact.
155+
if pattern == "viewer" {
156+
windows.keys().any(|k| k.starts_with("viewer-"))
157+
} else {
158+
windows.contains_key(pattern)
159+
}
160+
}
161+
162+
/// Whether any leaf in the signal tree references windows. Drives poll cadence.
163+
fn signal_uses_windows(signal: &AckSignal) -> bool {
164+
match signal {
165+
AckSignal::WindowAppeared(_) | AckSignal::WindowDisappeared(_) => true,
166+
AckSignal::Any(signals) => signals.iter().any(signal_uses_windows),
167+
_ => false,
168+
}
169+
}
170+
171+
/// Capture the current pane-state generation. Used to build a
172+
/// `GenerationAdvanced { from }` signal just before dispatching an action.
173+
///
174+
/// Returns 0 when the store isn't registered (test contexts); callers wrap the
175+
/// resulting signal in a normal `wait_for_ack` call that will immediately succeed
176+
/// in those cases because the test fixture either bumps generation or skips the wait.
177+
pub fn snapshot_generation<R: Runtime>(app: &AppHandle<R>) -> u64 {
178+
app.try_state::<PaneStateStore>()
179+
.map(|store| store.get_generation())
180+
.unwrap_or(0)
181+
}
182+
183+
#[cfg(test)]
184+
mod tests {
185+
use super::*;
186+
use std::sync::Arc;
187+
188+
// The signal-checking core is pure (it reads `Manager::try_state` and
189+
// `webview_windows`), so the only piece we can unit-test without spinning
190+
// up a Tauri app is the `PaneStateStore` interaction. The `tests/`
191+
// module covers that case via `tests::ack_system_tests` against a real
192+
// `PaneStateStore`. The window-driven branch is exercised by E2E tests
193+
// and by the dialog integration tests.
194+
195+
#[test]
196+
fn describe_renders_each_variant() {
197+
assert!(AckSignal::GenerationAdvanced { from: 42 }.describe().contains("42"));
198+
assert!(
199+
AckSignal::SoftDialogAppeared("delete-confirmation")
200+
.describe()
201+
.contains("delete-confirmation")
202+
);
203+
assert!(AckSignal::WindowAppeared("settings").describe().contains("settings"));
204+
assert!(AckSignal::WindowDisappeared("settings").describe().contains("settings"));
205+
let any = AckSignal::Any(vec![
206+
AckSignal::GenerationAdvanced { from: 1 },
207+
AckSignal::WindowAppeared("viewer"),
208+
]);
209+
let s = any.describe();
210+
assert!(s.contains("any of"));
211+
assert!(s.contains("viewer"));
212+
}
213+
214+
#[test]
215+
fn signal_uses_windows_picks_tighter_cadence() {
216+
assert!(!signal_uses_windows(&AckSignal::GenerationAdvanced { from: 0 }));
217+
assert!(signal_uses_windows(&AckSignal::WindowAppeared("settings")));
218+
assert!(signal_uses_windows(&AckSignal::Any(vec![
219+
AckSignal::GenerationAdvanced { from: 0 },
220+
AckSignal::WindowAppeared("viewer"),
221+
])));
222+
}
223+
224+
// Verifies the core promise: once the generation strictly advances past
225+
// the snapshot, a future polling for `GenerationAdvanced` would return
226+
// true. We exercise this through the store directly because we can't
227+
// construct a real `AppHandle` here.
228+
#[test]
229+
fn generation_strictly_advances_after_set_left() {
230+
let store = Arc::new(PaneStateStore::new());
231+
let before = store.get_generation();
232+
// Snapshot before mutation
233+
let snapshot = before;
234+
// Mutate
235+
store.set_left(crate::mcp::pane_state::PaneState {
236+
path: "/tmp".to_string(),
237+
..Default::default()
238+
});
239+
assert!(
240+
store.get_generation() > snapshot,
241+
"generation should strictly advance after set_left"
242+
);
243+
}
244+
}

0 commit comments

Comments
 (0)