diff --git a/AGENTS.md b/AGENTS.md index 1e080ab1..41d9ed65 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -174,8 +174,9 @@ Three cadences. Pick the one that matches where you are in the work, not the one Two MCP servers are available when the app is running via `pnpm dev`: -- **cmdr** (port 19224 prod / 19225 dev): high-level app control: navigation, file operations, search, dialogs, state inspection. This is - the primary way to test and interact with the running app. Architecture docs: `src-tauri/src/mcp/CLAUDE.md`. +- **cmdr** (port 19224 prod / 19225 dev): high-level app control: navigation, file operations, search, dialogs, state + inspection. This is the primary way to test and interact with the running app. Architecture docs: + `src-tauri/src/mcp/CLAUDE.md`. - **tauri** (port 9223): low-level Tauri access: screenshots, DOM inspection, JS execution, IPC calls. Use for visual verification and UI automation. diff --git a/apps/desktop/src-tauri/src/mcp/CLAUDE.md b/apps/desktop/src-tauri/src/mcp/CLAUDE.md index 0bf9b662..67af1c70 100644 --- a/apps/desktop/src-tauri/src/mcp/CLAUDE.md +++ b/apps/desktop/src-tauri/src/mcp/CLAUDE.md @@ -52,9 +52,40 @@ Directory module split by tool category. `mod.rs` contains the main `execute_too - `async_tools.rs`: await, connect_to_server, remove_manual_server, set_setting - `search.rs`: search index loading, search, ai_search -Most tools are fire-and-forget: emit a Tauri event and return "OK" immediately. +**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. -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. +The mechanism lives in `executor/ack.rs`. Each tool: + +1. Captures a precondition snapshot (typically `snapshot_generation(app)`). +2. Emits its existing event / runs its existing command. +3. Calls `wait_for_ack(app, signal, DEFAULT_ACK_TIMEOUT)` — default 1500 ms. +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. + +`AckSignal` variants: + +| Variant | Fires when | Used by | +| ------------------------ | ----------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | +| `GenerationAdvanced` | `PaneStateStore.generation` is strictly greater than the captured value | Anything that mutates pane state (`select`, `set_view_mode`, `sort`, `toggle_hidden`, `tab`, `nav_*`, auto-confirmed `copy`/`move`/`delete`, `dialog confirm`). NOT `refresh` — see TODO note below. | +| `SoftDialogAppeared(id)` | A soft dialog with that ID is in `SoftDialogTracker` | Confirmation dialogs from `copy`/`move`/`delete` (autoConfirm: false), `mkdir`, `mkfile`; `dialog open about` | +| `SoftDialogDisappeared(id)` | A soft dialog with that ID is no longer in `SoftDialogTracker` | `dialog close ` — the FE `ModalDialog` fires `notifyDialogClosed` on unmount, so the tracker reflects the close even when cancel doesn't bump pane generation | +| `WindowAppeared(label)` | A `webview_windows()` entry matches the label (exact, or `viewer-*`) | `dialog open settings|file-viewer`, `dialog focus` | +| `WindowDisappeared(label)` | The matching `webview_windows()` entry is gone | `dialog close settings` (single window family) | +| `WindowCountBelow {prefix, threshold}` | Count of matching windows is `< threshold` | `dialog close file-viewer` — snapshot the count, ack when one closes (don't wait for *all* viewers to vanish) | +| `Any([...])` | Logical OR — any inner signal fires | Reserved for future multi-mode tools | + +The polling cadence is 250 ms for state-driven signals (matching the existing `await` tool) and 100 ms for window/soft-dialog signals (both react faster than a full pane state push). + +**Gotcha/Why**: `dialog close ` requires the settings window to listen for the `mcp-settings-close` event and close itself (`apps/desktop/src/routes/settings/+page.svelte`). Without that listener the backend keeps polling for `WindowDisappeared("settings")` and times out at 1500 ms while the window stays put. Same shape applies if you add new window-based dialogs: the FE side has to opt in. + +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. The navigation family (`nav_to_parent`, `nav_back`, `nav_forward`, `open_under_cursor`) uses `NAV_ACK_TIMEOUT` (5 s) because a parent/back navigation can land on an SMB or MTP path whose directory listing takes a few seconds even on success. `nav_to_path` and `select_volume` use higher round-trip budgets via `mcp_round_trip_with_timeout` and aren't part of the ack-contract family. + +**Caveat: `GenerationAdvanced` isn't a per-action proof.** The snapshot-dispatch-wait sequence proves the FE pushed pane state *recently after* dispatch — not that the FE specifically handled our event. An unrelated push between snapshot and dispatch (the other pane's watcher, a tab refresh) will satisfy the signal as a false positive. The window is microseconds wide and the FE was clearly running (something pushed), so this is a much weaker false-positive class than the original "always OK" bug. If a real false positive ever surfaces, the fix is to switch the affected tool to `mcp_round_trip` with a request id. Tagged `TODO(mcp-ack):` in `ack.rs`. + +**Note on `update_pane_tabs`.** Tab changes flow through this command (not `set_left`/`set_right`). It delegates to `PaneStateStore::set_tabs`, which is the single place tab mutation + generation bump live. Add any new tab-mutating path through that helper, or the `tab` MCP tool's ack will time out. + +**Known TODO: `refresh` is still fire-and-forget.** The FE skips the `update_*_pane_state` push when the new listing is byte-identical to the cached state (correct behavior for state sync but no signal for the ack helper). Switching `refresh` to `mcp_round_trip` is the right follow-up, but requires a FE change to emit `mcp-response` after every re-list. The original "FE silently dropping the action" bug is less acute for refresh than for destructive tools, so this stays open. Search the codebase for `TODO(mcp-ack):` to find this and any future similar cases. + +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` (5 s timeout). `nav_to_path` uses `mcp_round_trip_with_timeout` with a 30 s timeout because it waits for the directory listing to complete (the frontend delays the response until `handleListingComplete` fires in FilePane). `open_under_cursor` also uses `mcp_round_trip_with_timeout` (5 s) because Enter on a non-directory file delegates to the OS default app (no pane state push, no viewer window), so neither `GenerationAdvanced` nor `WindowAppeared` would fire — the FE explicitly awaits `handleNavigate(entry)` and signals completion. 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. ### Configuration (`config.rs`) @@ -84,6 +115,16 @@ Directory module split by test category: ## Key decisions +### MCP action tools wait for backend ack before returning success + +**Decision (May 2026):** Every fire-and-forget action tool waits for a typed ack signal (`AckSignal::GenerationAdvanced`, `SoftDialogAppeared`/`Disappeared`, `WindowAppeared`/`Disappeared`, `WindowCountBelow`, or `Any`) within a 1500 ms budget (5 s for the nav family) before returning `OK`. On timeout, the tool returns a `ToolError::internal` whose message names the missing signal and elapsed budget. + +**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. + +**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`. + +**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. + ### Why agent-centric API? 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 +200,11 @@ The `cmdr://settings` resource and `set_setting` tool both use round-trips to th `volume_name` flows through `PaneState` from the FE via `update_left_pane_state` / `update_right_pane_state` on every state push (`FilePane.svelte`). -### Tool execution is async but mostly synchronous +### Tool execution is async; action tools wait for ack + +`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. -`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. +Three categories of latency-sensitive tools exist beyond the ack contract: (1) `mcp_round_trip` tools (`nav_to_path`, `move_cursor`, `set_setting`, `open_under_cursor`) 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. ### Error codes are JSON-RPC standard diff --git a/apps/desktop/src-tauri/src/mcp/executor/ack.rs b/apps/desktop/src-tauri/src/mcp/executor/ack.rs new file mode 100644 index 00000000..9830a327 --- /dev/null +++ b/apps/desktop/src-tauri/src/mcp/executor/ack.rs @@ -0,0 +1,315 @@ +//! Action-tool ack contract. +//! +//! MCP "action" tools used to return `OK` the instant they dispatched an event to the +//! frontend. If the FE was stalled (modal blocking input, error pane up, race during +//! startup), the action was silently dropped but the tool still reported success. +//! Real QA hit this. To make MCP a trustworthy automation surface, every fire-and-forget +//! action now waits for a small ack signal before returning. +//! +//! ## Signals +//! +//! - `GenerationAdvanced`: the `PaneStateStore` generation counter strictly advanced past +//! a captured value. Use this for actions that mutate pane state (navigation, refresh, +//! selection, view mode, sort, tabs, cursor moves, auto-confirmed copy/move/delete). +//! - `SoftDialogAppeared`: a soft (overlay) dialog with the given ID appeared in the +//! `SoftDialogTracker`. Use this for confirmation dialogs (transfer, delete, mkdir, +//! mkfile) when `autoConfirm: false`. +//! - `WindowAppeared` / `WindowDisappeared`: a Tauri webview window with the given label +//! prefix appeared (or vanished). Use this for child windows (settings, file-viewer, +//! about) and for `dialog close` actions. +//! - `WindowCountBelow`: the number of windows matching a label prefix is strictly less +//! than a snapshotted count. Use this for close-one-of-many scenarios (closing a +//! specific `viewer-*` window by path) where `WindowDisappeared` would only fire when +//! ALL viewers are gone. +//! +//! Multi-mode tools that can produce different signals depending on what happened (the +//! original `open_under_cursor` was the only such case) live outside the ack contract +//! entirely — they use `mcp_round_trip` and let the FE explicitly signal completion. An +//! earlier `AckSignal::Any` variant tried to OR these together but couldn't cover the +//! OS-open branch (Enter on a non-directory file produces neither a state push nor a +//! viewer window); round-trip is the only honest ack for that shape. +//! +//! ## Caveat: `GenerationAdvanced` is not a per-action ack +//! +//! `snapshot_generation` + dispatch + wait for `GenerationAdvanced` proves the FE pushed +//! pane state recently after dispatch — not that it specifically handled this action. An +//! unrelated state push (other pane's MTP watcher, a tab refresh) between the snapshot +//! and the dispatch can satisfy the signal without the FE having processed our event. +//! In practice this is a much weaker false-positive class than the original "always OK" +//! bug (the FE was almost certainly running, since something pushed state), so the +//! contract is acceptable. Stronger guarantees would require either a request-id-based +//! `mcp-response` round-trip (see `mcp_round_trip`) or per-tool FE acks. TODO(mcp-ack): +//! revisit if real-world false positives surface. +//! +//! ## Timeout +//! +//! Default `DEFAULT_ACK_TIMEOUT` = 1500 ms. Not exposed as an MCP-tool parameter — +//! MCP clients shouldn't have to tune this, the value is a backend-side latency +//! budget. Tunable per-call via the `Duration` argument to `wait_for_ack`. +//! +//! ## Decision/Why +//! +//! Polling cadence matches the existing `await` tool (250 ms for state checks, 100 ms +//! for window checks, since window state changes are typically faster than full pane +//! refreshes). The two loops aren't unified into a shared `poll_until` core yet: the +//! `await` tool exposes a few extra knobs (per-pane conditions, after_generation gate, +//! rich match summaries) that don't apply here, and the ack helper's loop is ~15 lines. +//! Extracting now would be premature abstraction. Revisit if we add a third polling +//! site or if the `await` tool grows AckSignal-shaped conditions. + +use std::time::Duration; + +use tauri::{AppHandle, Manager, Runtime}; + +use super::ToolError; +use crate::mcp::dialog_state::SoftDialogTracker; +use crate::mcp::pane_state::PaneStateStore; + +/// Default ack budget. Backend-side latency budget; not a client-facing knob. +pub const DEFAULT_ACK_TIMEOUT: Duration = Duration::from_millis(1500); + +/// Ack budget for navigation tools whose state push can include a directory listing +/// against a remote backend (SMB/MTP). Local paths still ack in ~50 ms; remote paths +/// can take a few seconds end-to-end. 5 s strikes a balance: still bounded, but no +/// spurious timeouts on a working remote share. `nav_to_path` and `select_volume` +/// use higher round-trip budgets via `mcp_round_trip_with_timeout`. +pub const NAV_ACK_TIMEOUT: Duration = Duration::from_secs(5); + +/// Polling cadence for state-driven signals. Matches the existing `await` tool. +const STATE_POLL_INTERVAL: Duration = Duration::from_millis(250); + +/// Polling cadence for window/dialog appearance signals. Windows show up faster than +/// full pane state pushes, so we poll a bit tighter for snappier acks. +const WINDOW_POLL_INTERVAL: Duration = Duration::from_millis(100); + +/// What the backend should wait for to consider an action "actually processed." +pub enum AckSignal { + /// State generation strictly advanced past `from`. + GenerationAdvanced { from: u64 }, + /// A soft dialog with this ID appeared in `SoftDialogTracker`. + SoftDialogAppeared(&'static str), + /// A soft dialog with this ID is no longer present in `SoftDialogTracker`. + /// Use this when an MCP tool dispatches a close to a soft (overlay) dialog: + /// the FE's `ModalDialog` runs `notifyDialogClosed` on destroy, so the + /// tracker reflects the close even when the surrounding pane state didn't + /// change (e.g. cancelling a confirmation dialog doesn't bump generation). + SoftDialogDisappeared(&'static str), + /// A Tauri webview window whose label equals (or starts with, for viewers) + /// the given pattern appeared. + WindowAppeared(&'static str), + /// A Tauri webview window matching the pattern vanished. For prefix families + /// like `viewer`, this fires only when zero matching windows remain; use + /// `WindowCountBelow` to wait for *one* viewer to close while others stay open. + WindowDisappeared(&'static str), + /// The number of webview windows matching the pattern is strictly less than + /// `threshold`. Used to ack "close one of N viewers" cleanly: snapshot the + /// count, dispatch the close, wait for the count to drop. For close-all, + /// use `threshold: 1` (i.e., wait for count to reach 0). + WindowCountBelow { prefix: &'static str, threshold: usize }, +} + +impl AckSignal { + /// Human-readable description for error messages. + fn describe(&self) -> String { + match self { + AckSignal::GenerationAdvanced { from } => { + format!("pane state generation > {from}") + } + AckSignal::SoftDialogAppeared(id) => format!("soft dialog '{id}' opened"), + AckSignal::SoftDialogDisappeared(id) => format!("soft dialog '{id}' closed"), + AckSignal::WindowAppeared(label) => format!("window '{label}' opened"), + AckSignal::WindowDisappeared(label) => format!("window '{label}' closed"), + AckSignal::WindowCountBelow { prefix, threshold } => { + format!("window count for '{prefix}' < {threshold}") + } + } + } +} + +/// Wait for an ack signal to arrive within `timeout`. +/// +/// On success returns `Ok(())`. On timeout returns a `ToolError::internal` whose message +/// names the missing signal and the elapsed budget, so callers can surface a useful +/// failure rather than a false-positive OK. +pub async fn wait_for_ack( + app: &AppHandle, + signal: AckSignal, + timeout: Duration, +) -> Result<(), ToolError> { + let start = tokio::time::Instant::now(); + let deadline = start + timeout; + + // Pick the tighter cadence if any leaf signal is window-driven; this matters + // for `Any` mixtures (open_under_cursor) where we want to react to a viewer + // window as fast as a pane generation bump. + let poll_interval = if signal_uses_windows(&signal) { + WINDOW_POLL_INTERVAL + } else { + STATE_POLL_INTERVAL + }; + + loop { + if check_signal(app, &signal) { + return Ok(()); + } + + if tokio::time::Instant::now() >= deadline { + let elapsed_ms = start.elapsed().as_millis(); + return Err(ToolError::internal(format!( + "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.", + elapsed_ms, + signal.describe() + ))); + } + + tokio::time::sleep(poll_interval).await; + } +} + +/// Check whether the signal is currently satisfied. Pure read; no side effects. +fn check_signal(app: &AppHandle, signal: &AckSignal) -> bool { + match signal { + AckSignal::GenerationAdvanced { from } => app + .try_state::() + .map(|store| store.get_generation() > *from) + .unwrap_or(false), + AckSignal::SoftDialogAppeared(id) => app + .try_state::() + .map(|tracker| tracker.get_open_types().iter().any(|d| d == id)) + .unwrap_or(false), + AckSignal::SoftDialogDisappeared(id) => app + .try_state::() + .map(|tracker| !tracker.get_open_types().iter().any(|d| d == id)) + // Asymmetry vs. SoftDialogAppeared (which returns false when the tracker + // isn't registered): if the tracker isn't there, no dialog is open either, + // so "this dialog is gone" is trivially true. The Appeared variant must + // wait — without a tracker it can never see the dialog. Lets unit tests + // drive close paths without spinning up a tracker fixture, while keeping + // the open path strict. + .unwrap_or(true), + AckSignal::WindowAppeared(pattern) => window_matches(app, pattern), + AckSignal::WindowDisappeared(pattern) => !window_matches(app, pattern), + AckSignal::WindowCountBelow { prefix, threshold } => count_windows_matching(app, prefix) < *threshold, + } +} + +/// True if any Tauri webview window has a label exactly equal to `pattern`, +/// or (for the `viewer` family) starting with `pattern-`. +fn window_matches(app: &AppHandle, pattern: &str) -> bool { + count_windows_matching(app, pattern) > 0 +} + +/// Count of Tauri webview windows matching `pattern`. For prefix families +/// (`viewer`), counts every `viewer-*` window; for exact labels (`settings`, +/// `file-viewer-help`, etc.), returns 0 or 1. +fn count_windows_matching(app: &AppHandle, pattern: &str) -> usize { + let windows = app.webview_windows(); + if pattern == "viewer" { + windows.keys().filter(|k| k.starts_with("viewer-")).count() + } else if windows.contains_key(pattern) { + 1 + } else { + 0 + } +} + +/// Snapshot the current count of Tauri webview windows matching `prefix`. +/// Use with `WindowCountBelow { threshold: snapshot }` to ack on "one closed." +pub fn snapshot_window_count(app: &AppHandle, prefix: &'static str) -> usize { + count_windows_matching(app, prefix) +} + +/// Whether any leaf in the signal tree references windows or soft dialogs. +/// Both are FE-side mutations that don't require a full pane-state push, so +/// they should react with the tighter cadence. +fn signal_uses_windows(signal: &AckSignal) -> bool { + match signal { + AckSignal::WindowAppeared(_) + | AckSignal::WindowDisappeared(_) + | AckSignal::WindowCountBelow { .. } + | AckSignal::SoftDialogAppeared(_) + | AckSignal::SoftDialogDisappeared(_) => true, + AckSignal::GenerationAdvanced { .. } => false, + } +} + +/// Capture the current pane-state generation. Used to build a +/// `GenerationAdvanced { from }` signal just before dispatching an action. +/// +/// Returns 0 when the store isn't registered (test contexts); callers wrap the +/// resulting signal in a normal `wait_for_ack` call that will immediately succeed +/// in those cases because the test fixture either bumps generation or skips the wait. +pub fn snapshot_generation(app: &AppHandle) -> u64 { + app.try_state::() + .map(|store| store.get_generation()) + .unwrap_or(0) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Arc; + + // The signal-checking core is pure (it reads `Manager::try_state` and + // `webview_windows`), so the only piece we can unit-test without spinning + // up a Tauri app is the `PaneStateStore` interaction. The `tests/` + // module covers that case via `tests::ack_system_tests` against a real + // `PaneStateStore`. The window-driven branch is exercised by E2E tests + // and by the dialog integration tests. + + #[test] + fn describe_renders_each_variant() { + assert!(AckSignal::GenerationAdvanced { from: 42 }.describe().contains("42")); + assert!( + AckSignal::SoftDialogAppeared("delete-confirmation") + .describe() + .contains("delete-confirmation") + ); + let closed = AckSignal::SoftDialogDisappeared("mkdir-confirmation").describe(); + assert!(closed.contains("mkdir-confirmation")); + assert!(closed.contains("closed")); + assert!(AckSignal::WindowAppeared("settings").describe().contains("settings")); + assert!(AckSignal::WindowDisappeared("settings").describe().contains("settings")); + let count = AckSignal::WindowCountBelow { + prefix: "viewer", + threshold: 3, + } + .describe(); + assert!(count.contains("viewer")); + assert!(count.contains("3")); + } + + #[test] + fn signal_uses_windows_picks_tighter_cadence() { + assert!(!signal_uses_windows(&AckSignal::GenerationAdvanced { from: 0 })); + assert!(signal_uses_windows(&AckSignal::WindowAppeared("settings"))); + assert!(signal_uses_windows(&AckSignal::SoftDialogDisappeared( + "mkdir-confirmation" + ))); + assert!(signal_uses_windows(&AckSignal::WindowCountBelow { + prefix: "viewer", + threshold: 1, + })); + } + + // Verifies the core promise: once the generation strictly advances past + // the snapshot, a future polling for `GenerationAdvanced` would return + // true. We exercise this through the store directly because we can't + // construct a real `AppHandle` here. + #[test] + fn generation_strictly_advances_after_set_left() { + let store = Arc::new(PaneStateStore::new()); + let before = store.get_generation(); + // Snapshot before mutation + let snapshot = before; + // Mutate + store.set_left(crate::mcp::pane_state::PaneState { + path: "/tmp".to_string(), + ..Default::default() + }); + assert!( + store.get_generation() > snapshot, + "generation should strictly advance after set_left" + ); + } +} diff --git a/apps/desktop/src-tauri/src/mcp/executor/app.rs b/apps/desktop/src-tauri/src/mcp/executor/app.rs index 3a5c27c3..ed272014 100644 --- a/apps/desktop/src-tauri/src/mcp/executor/app.rs +++ b/apps/desktop/src-tauri/src/mcp/executor/app.rs @@ -3,7 +3,7 @@ use serde_json::{Value, json}; use tauri::{AppHandle, Emitter, Manager, Runtime}; -use super::{PaneStateStore, ToolError, ToolResult}; +use super::{AckSignal, DEFAULT_ACK_TIMEOUT, PaneStateStore, ToolError, ToolResult, snapshot_generation, wait_for_ack}; /// Execute quit command. pub fn execute_quit(app: &AppHandle) -> ToolResult { @@ -38,7 +38,10 @@ pub fn execute_swap_panes(app: &AppHandle) -> ToolResult { } /// Execute unified tab command. -pub fn execute_tab(app: &AppHandle, params: &Value) -> ToolResult { +/// +/// Ack: pane generation advances after the FE pushes the new tab list via +/// `update_pane_tabs` (which bumps generation specifically for this case). +pub async fn execute_tab(app: &AppHandle, params: &Value) -> ToolResult { let action = params .get("action") .and_then(|v| v.as_str()) @@ -104,41 +107,39 @@ pub fn execute_tab(app: &AppHandle, params: &Value) -> ToolResult } } - match action { + let pre_gen = snapshot_generation(app); + let ok_msg = match action { "new" => { app.emit("mcp-tab", json!({"action": "new", "pane": pane}))?; - Ok(json!(format!("OK: Creating new tab in {} pane", pane))) + format!("OK: Creating new tab in {} pane", pane) } "reopen" => { app.emit("mcp-tab", json!({"action": "reopen", "pane": pane}))?; - Ok(json!(format!("OK: Reopening last closed tab in {} pane", pane))) + format!("OK: Reopening last closed tab in {} pane", pane) } "close" => { app.emit( "mcp-tab", json!({"action": "close", "pane": pane, "tabId": resolved_tab_id}), )?; - Ok(json!(format!("OK: Closing tab {} in {} pane", resolved_tab_id, pane))) + format!("OK: Closing tab {} in {} pane", resolved_tab_id, pane) } "close_others" => { app.emit( "mcp-tab", json!({"action": "close_others", "pane": pane, "tabId": resolved_tab_id}), )?; - Ok(json!(format!( + format!( "OK: Closing other tabs in {} pane (keeping {} and pinned)", pane, resolved_tab_id - ))) + ) } "activate" => { app.emit( "mcp-tab", json!({"action": "activate", "pane": pane, "tabId": resolved_tab_id}), )?; - Ok(json!(format!( - "OK: Switched to tab {} in {} pane", - resolved_tab_id, pane - ))) + format!("OK: Switched to tab {} in {} pane", resolved_tab_id, pane) } "set_pinned" => { let pinned = params @@ -150,8 +151,16 @@ pub fn execute_tab(app: &AppHandle, params: &Value) -> ToolResult "mcp-tab", json!({"action": "set_pinned", "pane": pane, "tabId": resolved_tab_id, "pinned": pinned}), )?; - Ok(json!(format!("OK: {} tab {} in {} pane", verb, resolved_tab_id, pane))) + format!("OK: {} tab {} in {} pane", verb, resolved_tab_id, pane) } - _ => Err(ToolError::invalid_params(format!("Unknown tab action: {}", action))), - } + _ => return Err(ToolError::invalid_params(format!("Unknown tab action: {}", action))), + }; + + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; + Ok(json!(ok_msg)) } diff --git a/apps/desktop/src-tauri/src/mcp/executor/dialogs.rs b/apps/desktop/src-tauri/src/mcp/executor/dialogs.rs index 3576d14a..ae068973 100644 --- a/apps/desktop/src-tauri/src/mcp/executor/dialogs.rs +++ b/apps/desktop/src-tauri/src/mcp/executor/dialogs.rs @@ -1,15 +1,34 @@ //! Dialog tool handlers. +//! +//! Ack contract: +//! - `open settings|file-viewer|about` → child window appears in `webview_windows()`. +//! - `open` confirmation dialogs → not allowed (use copy/move/delete/mkdir/mkfile instead). +//! - `close settings` → matching Tauri window disappears. +//! - `close file-viewer` → snapshot the viewer-window count, ack when it drops (so +//! closing one of N viewers acks without waiting for all to vanish). Returns an +//! `invalid_params` error fast-path if no viewers are open at all. +//! - `close about` → soft dialog `about` is no longer in `SoftDialogTracker` (`about` +//! is an overlay, not a separate window). +//! - `close ` → soft dialog is no longer in `SoftDialogTracker`. Cancel +//! doesn't reliably bump pane generation, so we wait for the tracker entry to vanish. +//! - `focus settings|file-viewer|about` → window is present (no-op fast path; if the +//! window isn't there, the wait_for_ack times out, which is the correct contract for +//! focusing a non-existent dialog). +//! - `confirm ` → pane generation advances (the FE accepted the +//! confirmation and the underlying copy/move/delete started, producing a state push). use std::path::Path; use serde_json::{Value, json}; use tauri::{AppHandle, Emitter, Manager, Runtime}; -use super::{ToolError, ToolResult}; +use super::{ + AckSignal, DEFAULT_ACK_TIMEOUT, ToolError, ToolResult, snapshot_generation, snapshot_window_count, wait_for_ack, +}; /// Execute the unified dialog command. /// Handles opening, focusing, and closing dialogs. -pub fn execute_dialog_command(app: &AppHandle, params: &Value) -> ToolResult { +pub async fn execute_dialog_command(app: &AppHandle, params: &Value) -> ToolResult { let action = params .get("action") .and_then(|v| v.as_str()) @@ -32,16 +51,16 @@ pub fn execute_dialog_command(app: &AppHandle, params: &Value) -> let on_conflict = params.get("onConflict").and_then(|v| v.as_str()); match action { - "open" => execute_dialog_open(app, dialog_type, section, path), - "focus" => execute_dialog_focus(app, dialog_type, path), - "close" => execute_dialog_close(app, dialog_type, path), - "confirm" => execute_dialog_confirm(app, dialog_type, on_conflict), + "open" => execute_dialog_open(app, dialog_type, section, path).await, + "focus" => execute_dialog_focus(app, dialog_type, path).await, + "close" => execute_dialog_close(app, dialog_type, path).await, + "confirm" => execute_dialog_confirm(app, dialog_type, on_conflict).await, _ => Err(ToolError::invalid_params(format!("Invalid action: {action}"))), } } /// Execute dialog open action. -fn execute_dialog_open( +async fn execute_dialog_open( app: &AppHandle, dialog_type: &str, section: Option<&str>, @@ -55,9 +74,11 @@ fn execute_dialog_open( if let Some(section) = section { // Section-specific: MCP-only event handled by setupDialogListeners app.emit_to("main", "open-settings", json!({"section": section}))?; + wait_for_ack(app, AckSignal::WindowAppeared("settings"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!(format!("OK: Opened settings at {section}"))) } else { app.emit_to("main", "execute-command", json!({"commandId": "app.settings"}))?; + wait_for_ack(app, AckSignal::WindowAppeared("settings"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!("OK: Opened settings")) } } @@ -69,15 +90,20 @@ fn execute_dialog_open( return Err(ToolError::invalid_params(format!("File does not exist: {}", path))); } app.emit("open-file-viewer", json!({"path": path}))?; + wait_for_ack(app, AckSignal::WindowAppeared("viewer"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!(format!("OK: Opened file viewer for {path}"))) } else { // Open for file under cursor (validation happens in frontend) app.emit("open-file-viewer", ())?; + wait_for_ack(app, AckSignal::WindowAppeared("viewer"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!("OK: Opened file viewer for cursor file")) } } "about" => { app.emit_to("main", "execute-command", json!({"commandId": "app.about"}))?; + // `about` is a soft dialog (ModalDialog overlay in the main window), not a + // separate Tauri window. Track via SoftDialogTracker. + wait_for_ack(app, AckSignal::SoftDialogAppeared("about"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!("OK: Opened about dialog")) } "transfer-confirmation" | "mkdir-confirmation" | "new-file-confirmation" | "delete-confirmation" => { @@ -90,10 +116,15 @@ fn execute_dialog_open( } /// Execute dialog focus action. -fn execute_dialog_focus(app: &AppHandle, dialog_type: &str, path: Option<&str>) -> ToolResult { +async fn execute_dialog_focus(app: &AppHandle, dialog_type: &str, path: Option<&str>) -> ToolResult { + // Focus is a best-effort UI hint. We don't have a reliable "the window now has + // focus" signal cross-platform, so we ack on the precondition: the target dialog + // must currently exist. If it doesn't, the wait_for_ack times out with a clear + // message; that's the correct contract (you can't focus what isn't there). match dialog_type { "settings" => { app.emit("focus-settings", ())?; + wait_for_ack(app, AckSignal::WindowAppeared("settings"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!("OK: Focused settings")) } "file-viewer" => { @@ -103,19 +134,29 @@ fn execute_dialog_focus(app: &AppHandle, dialog_type: &str, path: return Err(ToolError::invalid_params(format!("File does not exist: {}", path))); } app.emit("focus-file-viewer", json!({"path": path}))?; + wait_for_ack(app, AckSignal::WindowAppeared("viewer"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!(format!("OK: Focused file viewer for {path}"))) } else { // Focus most recently opened file-viewer app.emit("focus-file-viewer", ())?; + wait_for_ack(app, AckSignal::WindowAppeared("viewer"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!("OK: Focused most recent file viewer")) } } "about" => { app.emit("focus-about", ())?; + wait_for_ack(app, AckSignal::SoftDialogAppeared("about"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!("OK: Focused about dialog")) } "transfer-confirmation" | "mkdir-confirmation" | "new-file-confirmation" | "delete-confirmation" => { app.emit("focus-confirmation", ())?; + // Soft dialogs: the tracker is the source of truth. + wait_for_ack( + app, + AckSignal::SoftDialogAppeared(soft_dialog_id(dialog_type)), + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Focused confirmation dialog")) } _ => Err(ToolError::invalid_params(format!("Invalid dialog type: {dialog_type}"))), @@ -123,7 +164,7 @@ fn execute_dialog_focus(app: &AppHandle, dialog_type: &str, path: } /// Execute dialog close action. -fn execute_dialog_close(app: &AppHandle, dialog_type: &str, path: Option<&str>) -> ToolResult { +async fn execute_dialog_close(app: &AppHandle, dialog_type: &str, path: Option<&str>) -> ToolResult { // Window-based dialogs are closed via their window; soft dialogs are tracked // automatically by the frontend via notify_dialog_closed. @@ -131,24 +172,71 @@ fn execute_dialog_close(app: &AppHandle, dialog_type: &str, path: "settings" => { if app.webview_windows().contains_key("settings") { app.emit_to("settings", "mcp-settings-close", ())?; + wait_for_ack(app, AckSignal::WindowDisappeared("settings"), DEFAULT_ACK_TIMEOUT).await?; } + // If the settings window wasn't open to begin with, the close is a no-op + // and we return OK without waiting: the desired end state is already true. Ok(json!("OK: Closed settings")) } "file-viewer" => { + // Snapshot the viewer count first. If zero, fast-fail: there's nothing to + // close, and waiting for a count drop would just time out at 1500 ms. + let before = snapshot_window_count(app, "viewer"); + if before == 0 { + return Err(ToolError::invalid_params("No file viewer windows are open.")); + } if let Some(path) = path { app.emit("close-file-viewer", json!({"path": path}))?; + // Closing one of N viewers: ack when the count drops below `before`. + // If the path doesn't match any open viewer, the count stays put and + // we time out — which is the right contract (caller asked to close a + // specific viewer that isn't there). + wait_for_ack( + app, + AckSignal::WindowCountBelow { + prefix: "viewer", + threshold: before, + }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!(format!("OK: Closed file viewer for {path}"))) } else { app.emit("close-all-file-viewers", ())?; + // Close-all: ack when zero viewers remain (`count < 1`). + wait_for_ack( + app, + AckSignal::WindowCountBelow { + prefix: "viewer", + threshold: 1, + }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Closed all file viewer dialogs")) } } "about" => { + // `about` is a soft dialog (overlay in the main window), tracked via + // SoftDialogTracker (id: "about"). If it isn't open, the tracker doesn't + // hold the id and `SoftDialogDisappeared` returns immediately — close is + // a fast no-op in that case, no timeout. app.emit("close-about", ())?; + wait_for_ack(app, AckSignal::SoftDialogDisappeared("about"), DEFAULT_ACK_TIMEOUT).await?; Ok(json!("OK: Closed about dialog")) } "transfer-confirmation" | "mkdir-confirmation" | "new-file-confirmation" | "delete-confirmation" => { app.emit("close-confirmation", ())?; + // Soft confirmation dialogs unmount their `ModalDialog`, which fires + // `notifyDialogClosed` and updates the `SoftDialogTracker`. Wait for the + // tracker to lose the dialog ID. Cancel doesn't reliably bump generation + // (that's what we used to wait for, and it broke on every cancel). + wait_for_ack( + app, + AckSignal::SoftDialogDisappeared(soft_dialog_id(dialog_type)), + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Cancelled confirmation dialog")) } _ => Err(ToolError::invalid_params(format!("Invalid dialog type: {dialog_type}"))), @@ -157,7 +245,11 @@ fn execute_dialog_close(app: &AppHandle, dialog_type: &str, path: /// Execute dialog confirm action. /// Programmatically confirms an already-open dialog. -fn execute_dialog_confirm(app: &AppHandle, dialog_type: &str, on_conflict: Option<&str>) -> ToolResult { +async fn execute_dialog_confirm( + app: &AppHandle, + dialog_type: &str, + on_conflict: Option<&str>, +) -> ToolResult { match dialog_type { "transfer-confirmation" => { let conflict_policy = on_conflict.unwrap_or("skip_all"); @@ -166,14 +258,28 @@ fn execute_dialog_confirm(app: &AppHandle, dialog_type: &str, on_ "onConflict must be 'skip_all', 'overwrite_all', or 'rename_all'", )); } + let pre_gen = snapshot_generation(app); app.emit( "mcp-confirm-dialog", json!({"type": "transfer-confirmation", "onConflict": conflict_policy}), )?; + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Transfer dialog confirmed.")) } "delete-confirmation" => { + let pre_gen = snapshot_generation(app); app.emit("mcp-confirm-dialog", json!({"type": "delete-confirmation"}))?; + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Delete dialog confirmed.")) } _ => Err(ToolError::invalid_params(format!( @@ -182,3 +288,16 @@ fn execute_dialog_confirm(app: &AppHandle, dialog_type: &str, on_ ))), } } + +/// Map an MCP confirmation `dialog_type` to its `SoftDialogTracker` ID. The IDs are +/// declared in the Svelte side via `` and registered with +/// the backend at startup (`register_known_dialogs`). +fn soft_dialog_id(dialog_type: &str) -> &'static str { + match dialog_type { + "transfer-confirmation" => "transfer-confirmation", + "delete-confirmation" => "delete-confirmation", + "mkdir-confirmation" => "mkdir-confirmation", + "new-file-confirmation" => "new-file-confirmation", + _ => "", + } +} diff --git a/apps/desktop/src-tauri/src/mcp/executor/file_ops.rs b/apps/desktop/src-tauri/src/mcp/executor/file_ops.rs index 8a28c2ea..b42d0c1d 100644 --- a/apps/desktop/src-tauri/src/mcp/executor/file_ops.rs +++ b/apps/desktop/src-tauri/src/mcp/executor/file_ops.rs @@ -3,14 +3,18 @@ use serde_json::{Value, json}; use tauri::{AppHandle, Emitter, Manager, Runtime}; -use super::{PaneStateStore, ToolError, ToolResult}; +use super::{AckSignal, DEFAULT_ACK_TIMEOUT, PaneStateStore, ToolError, ToolResult, snapshot_generation, wait_for_ack}; /// Execute copy command. /// +/// Ack contract: +/// - `autoConfirm: true` → pane generation must advance (selection/state push after copy starts). +/// - `autoConfirm: false` → `transfer-confirmation` soft dialog must appear. +/// /// Note: We cannot validate whether files are selected because selection state /// is managed by the frontend. The validation happens in the frontend event handler /// which will show an appropriate error if no files are selected. -pub fn execute_copy(app: &AppHandle, params: &Value) -> ToolResult { +pub async fn execute_copy(app: &AppHandle, params: &Value) -> ToolResult { let auto_confirm = params.get("autoConfirm").and_then(|v| v.as_bool()).unwrap_or(false); let on_conflict = params.get("onConflict").and_then(|v| v.as_str()).unwrap_or("skip_all"); @@ -20,24 +24,39 @@ pub fn execute_copy(app: &AppHandle, params: &Value) -> ToolResul )); } + let pre_gen = snapshot_generation(app); app.emit( "mcp-copy", json!({"autoConfirm": auto_confirm, "onConflict": on_conflict}), )?; if auto_confirm { + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Copy started with auto-confirm.")) } else { + wait_for_ack( + app, + AckSignal::SoftDialogAppeared("transfer-confirmation"), + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Copy dialog opened. Waiting for user confirmation.")) } } /// Execute move command. /// +/// Ack contract: same as `copy` (transfer-confirmation dialog shape). +/// /// Note: We cannot validate whether files are selected because selection state /// is managed by the frontend. The validation happens in the frontend event handler /// which will show an appropriate error if no files are selected. -pub fn execute_move(app: &AppHandle, params: &Value) -> ToolResult { +pub async fn execute_move(app: &AppHandle, params: &Value) -> ToolResult { let auto_confirm = params.get("autoConfirm").and_then(|v| v.as_bool()).unwrap_or(false); let on_conflict = params.get("onConflict").and_then(|v| v.as_str()).unwrap_or("skip_all"); @@ -47,61 +66,116 @@ pub fn execute_move(app: &AppHandle, params: &Value) -> ToolResul )); } + let pre_gen = snapshot_generation(app); app.emit( "mcp-move", json!({"autoConfirm": auto_confirm, "onConflict": on_conflict}), )?; if auto_confirm { + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Move started with auto-confirm.")) } else { + wait_for_ack( + app, + AckSignal::SoftDialogAppeared("transfer-confirmation"), + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Move dialog opened. Waiting for user confirmation.")) } } /// Execute delete command. /// +/// Ack contract: +/// - `autoConfirm: true` → pane generation must advance. +/// - `autoConfirm: false` → `delete-confirmation` soft dialog must appear. +/// /// Note: We cannot validate whether files are selected because selection state /// is managed by the frontend. The validation happens in the frontend event handler /// which will show an appropriate error if no files are selected. -pub fn execute_delete(app: &AppHandle, params: &Value) -> ToolResult { +pub async fn execute_delete(app: &AppHandle, params: &Value) -> ToolResult { let auto_confirm = params.get("autoConfirm").and_then(|v| v.as_bool()).unwrap_or(false); + let pre_gen = snapshot_generation(app); app.emit("mcp-delete", json!({"autoConfirm": auto_confirm}))?; if auto_confirm { + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Delete started with auto-confirm.")) } else { + wait_for_ack( + app, + AckSignal::SoftDialogAppeared("delete-confirmation"), + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Delete dialog opened. Waiting for user confirmation.")) } } -/// Execute mkdir command. +/// Execute mkdir command. Ack: `mkdir-confirmation` soft dialog appears. /// /// Note: We cannot validate whether the current directory is writable because /// the current directory path is managed by the frontend. The validation happens /// when the actual mkdir operation is attempted, which will return an appropriate /// error if the directory is not writable. -pub fn execute_mkdir(app: &AppHandle) -> ToolResult { +pub async fn execute_mkdir(app: &AppHandle) -> ToolResult { app.emit("mcp-mkdir", ())?; + wait_for_ack( + app, + AckSignal::SoftDialogAppeared("mkdir-confirmation"), + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Create folder dialog opened.")) } -/// Execute mkfile command. -pub fn execute_mkfile(app: &AppHandle) -> ToolResult { +/// Execute mkfile command. Ack: `new-file-confirmation` soft dialog appears. +pub async fn execute_mkfile(app: &AppHandle) -> ToolResult { app.emit("mcp-mkfile", ())?; + wait_for_ack( + app, + AckSignal::SoftDialogAppeared("new-file-confirmation"), + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!("OK: Create file dialog opened.")) } /// Execute refresh command. -pub fn execute_refresh(app: &AppHandle) -> ToolResult { +/// +/// TODO(mcp-ack): No reliable ack signal yet. `refresh` asks the FE to re-list the +/// current pane. If the listing comes back byte-identical to the cached state (very +/// common for MTP and SMB after an operation already pushed fresh state, or for any +/// pane that hasn't drifted since the last update), the FE skips the +/// `update_*_pane_state` call to avoid a redundant generation bump. That leaves +/// `refresh` without a `GenerationAdvanced` signal. +/// +/// Two acceptable follow-ups: (1) switch to `mcp_round_trip` so the FE explicitly +/// emits `mcp-response` once re-list completes regardless of whether state changed; +/// (2) always bump generation on re-list. Until one of those, refresh stays +/// fire-and-forget. The original bug is less acute here than for destructive tools. +pub async fn execute_refresh(app: &AppHandle) -> ToolResult { app.emit("mcp-refresh", ())?; Ok(json!("OK: Pane refreshed")) } -/// Execute the unified select command. +/// Execute the unified select command. Ack: pane generation advances (the new +/// selection is pushed via the next `update_*_pane_state`). /// Emits event to frontend to manipulate file selection. -pub fn execute_select_command(app: &AppHandle, params: &Value) -> ToolResult { +pub async fn execute_select_command(app: &AppHandle, params: &Value) -> ToolResult { let pane = params .get("pane") .and_then(|v| v.as_str()) @@ -157,10 +231,17 @@ pub fn execute_select_command(app: &AppHandle, params: &Value) -> store.set_focused_pane(pane.to_string()); } + let pre_gen = snapshot_generation(app); app.emit( "mcp-select", json!({"pane": pane, "start": start, "count": count, "mode": mode}), )?; + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!(format!("OK: Selection updated in {pane} pane"))) } diff --git a/apps/desktop/src-tauri/src/mcp/executor/mod.rs b/apps/desktop/src-tauri/src/mcp/executor/mod.rs index 4b0f2a35..8e08396a 100644 --- a/apps/desktop/src-tauri/src/mcp/executor/mod.rs +++ b/apps/desktop/src-tauri/src/mcp/executor/mod.rs @@ -3,6 +3,7 @@ //! Handles the execution of MCP tools and returns results. //! All tools are designed to match user capabilities exactly. +mod ack; mod app; mod async_tools; mod dialogs; @@ -11,6 +12,10 @@ mod nav; mod search; mod view; +pub(crate) use ack::{ + AckSignal, DEFAULT_ACK_TIMEOUT, NAV_ACK_TIMEOUT, snapshot_generation, snapshot_window_count, wait_for_ack, +}; + #[cfg(test)] mod tests; @@ -125,28 +130,28 @@ pub async fn execute_tool(app: &AppHandle, name: &str, params: &V "switch_pane" => app::execute_switch_pane(app), "swap_panes" => app::execute_swap_panes(app), // View commands - "toggle_hidden" => view::execute_toggle_hidden(app), - "set_view_mode" => view::execute_set_view_mode(app, params), - "sort" => view::execute_sort(app, params), + "toggle_hidden" => view::execute_toggle_hidden(app).await, + "set_view_mode" => view::execute_set_view_mode(app, params).await, + "sort" => view::execute_sort(app, params).await, // Navigation commands (no params) - "open_under_cursor" | "nav_to_parent" | "nav_back" | "nav_forward" => nav::execute_nav_command(app, name), + "open_under_cursor" | "nav_to_parent" | "nav_back" | "nav_forward" => nav::execute_nav_command(app, name).await, // Navigation commands (with params) "select_volume" | "nav_to_path" | "move_cursor" | "scroll_to" => { nav::execute_nav_command_with_params(app, name, params).await } // Tab commands - "tab" => app::execute_tab(app, params), + "tab" => app::execute_tab(app, params).await, // File operation commands - "copy" => file_ops::execute_copy(app, params), - "move" => file_ops::execute_move(app, params), - "delete" => file_ops::execute_delete(app, params), - "mkdir" => file_ops::execute_mkdir(app), - "mkfile" => file_ops::execute_mkfile(app), - "refresh" => file_ops::execute_refresh(app), + "copy" => file_ops::execute_copy(app, params).await, + "move" => file_ops::execute_move(app, params).await, + "delete" => file_ops::execute_delete(app, params).await, + "mkdir" => file_ops::execute_mkdir(app).await, + "mkfile" => file_ops::execute_mkfile(app).await, + "refresh" => file_ops::execute_refresh(app).await, // Selection command - "select" => file_ops::execute_select_command(app, params), + "select" => file_ops::execute_select_command(app, params).await, // Dialog command - "dialog" => dialogs::execute_dialog_command(app, params), + "dialog" => dialogs::execute_dialog_command(app, params).await, // Search commands "search" => search::execute_search(params).await, "ai_search" => search::execute_ai_search(params).await, diff --git a/apps/desktop/src-tauri/src/mcp/executor/nav.rs b/apps/desktop/src-tauri/src/mcp/executor/nav.rs index 0970c068..828d514f 100644 --- a/apps/desktop/src-tauri/src/mcp/executor/nav.rs +++ b/apps/desktop/src-tauri/src/mcp/executor/nav.rs @@ -5,13 +5,45 @@ use std::path::Path; use serde_json::{Value, json}; use tauri::{AppHandle, Emitter, Manager, Runtime}; -use super::{PaneStateStore, ToolError, ToolResult, mcp_round_trip, mcp_round_trip_with_timeout}; +use super::{ + AckSignal, NAV_ACK_TIMEOUT, PaneStateStore, ToolError, ToolResult, mcp_round_trip, mcp_round_trip_with_timeout, + snapshot_generation, wait_for_ack, +}; /// Execute a navigation command without parameters. /// These emit keyboard-equivalent events to the frontend. -pub fn execute_nav_command(app: &AppHandle, name: &str) -> ToolResult { +/// +/// Ack contract: +/// - `nav_to_parent`, `nav_back`, `nav_forward` → pane generation must advance (path +/// changes get pushed via `update_*_pane_state`). +/// - `open_under_cursor` → round-trip via `mcp-open-under-cursor`. The FE awaits the +/// resolved action (directory navigation, viewer window, or OS open-with-default) and +/// emits `mcp-response`. We can't rely on `GenerationAdvanced` or `WindowAppeared` +/// here because the OS-open branch produces neither — `openFile()` hands the path to +/// the OS default and returns, no state push, no viewer window. The round-trip is the +/// only honest ack for this multi-mode command. +/// +/// The `nav_to_parent` / `nav_back` / `nav_forward` family uses `NAV_ACK_TIMEOUT` (5 s) +/// instead of the default 1500 ms because navigation can touch a remote backend +/// (SMB/MTP), whose directory listing can take a few seconds even on success. With the +/// default budget, every remote-share `Enter` would surface a false-negative timeout +/// while the navigation actually succeeded in the background. +pub async fn execute_nav_command(app: &AppHandle, name: &str) -> ToolResult { + if name == "open_under_cursor" { + // Round-trip: FE awaits `handleNavigate(entry)` and emits `mcp-response`. + // 5 s timeout covers the slow case (large directory listing) without being + // open-ended. + return mcp_round_trip_with_timeout( + app, + "mcp-open-under-cursor", + json!({}), + "OK: Opened item under cursor".to_string(), + 5, + ) + .await; + } + let key = match name { - "open_under_cursor" => "Enter", "nav_to_parent" => "Backspace", "nav_back" => "GoBack", // Custom event, handled by frontend "nav_forward" => "GoForward", // Custom event @@ -19,14 +51,16 @@ pub fn execute_nav_command(app: &AppHandle, name: &str) -> ToolRe }; let action = match name { - "open_under_cursor" => "Opened item under cursor", "nav_to_parent" => "Navigated to parent directory", "nav_back" => "Navigated back", "nav_forward" => "Navigated forward", _ => "Navigation action completed", }; + let pre_gen = snapshot_generation(app); app.emit("mcp-key", json!({"key": key}))?; + + wait_for_ack(app, AckSignal::GenerationAdvanced { from: pre_gen }, NAV_ACK_TIMEOUT).await?; Ok(json!(format!("OK: {action}"))) } diff --git a/apps/desktop/src-tauri/src/mcp/executor/view.rs b/apps/desktop/src-tauri/src/mcp/executor/view.rs index dded87c4..46adcef3 100644 --- a/apps/desktop/src-tauri/src/mcp/executor/view.rs +++ b/apps/desktop/src-tauri/src/mcp/executor/view.rs @@ -3,18 +3,26 @@ use serde_json::{Value, json}; use tauri::{AppHandle, Emitter, Manager, Runtime}; -use super::{PaneStateStore, ToolError, ToolResult}; +use super::{AckSignal, DEFAULT_ACK_TIMEOUT, PaneStateStore, ToolError, ToolResult, snapshot_generation, wait_for_ack}; use crate::commands::ui::toggle_hidden_files; -/// Execute toggle_hidden command. -pub fn execute_toggle_hidden(app: &AppHandle) -> ToolResult { +/// Execute toggle_hidden command. Ack: pane generation advances when the FE re-pushes +/// state with the new visibility flag. +pub async fn execute_toggle_hidden(app: &AppHandle) -> ToolResult { + let pre_gen = snapshot_generation(app); let result = toggle_hidden_files(app.clone()).map_err(ToolError::internal)?; + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; let state = if result { "visible" } else { "hidden" }; Ok(json!(format!("OK: Hidden files now {state}"))) } -/// Execute set_view_mode command. -pub fn execute_set_view_mode(app: &AppHandle, params: &Value) -> ToolResult { +/// Execute set_view_mode command. Ack: pane generation advances. +pub async fn execute_set_view_mode(app: &AppHandle, params: &Value) -> ToolResult { let pane = params .get("pane") .and_then(|v| v.as_str()) @@ -35,12 +43,20 @@ pub fn execute_set_view_mode(app: &AppHandle, params: &Value) -> store.set_focused_pane(pane.to_string()); } + let pre_gen = snapshot_generation(app); app.emit("mcp-set-view-mode", json!({"pane": pane, "mode": mode}))?; + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; Ok(json!(format!("OK: Set {pane} pane to {mode} view"))) } -/// Execute unified sort command. -pub fn execute_sort(app: &AppHandle, params: &Value) -> ToolResult { +/// Execute unified sort command. Ack: pane generation advances (the FE re-orders and +/// re-pushes state). +pub async fn execute_sort(app: &AppHandle, params: &Value) -> ToolResult { let pane = params .get("pane") .and_then(|v| v.as_str()) @@ -70,7 +86,14 @@ pub fn execute_sort(app: &AppHandle, params: &Value) -> ToolResul store.set_focused_pane(pane.to_string()); } + let pre_gen = snapshot_generation(app); app.emit("mcp-sort", json!({"pane": pane, "by": by, "order": order}))?; + wait_for_ack( + app, + AckSignal::GenerationAdvanced { from: pre_gen }, + DEFAULT_ACK_TIMEOUT, + ) + .await?; let order_name = if order == "asc" { "ascending" } else { "descending" }; Ok(json!(format!("OK: Sorted {pane} pane by {by} ({order_name})"))) diff --git a/apps/desktop/src-tauri/src/mcp/pane_state.rs b/apps/desktop/src-tauri/src/mcp/pane_state.rs index 99a253d5..d22b24d1 100644 --- a/apps/desktop/src-tauri/src/mcp/pane_state.rs +++ b/apps/desktop/src-tauri/src/mcp/pane_state.rs @@ -145,6 +145,22 @@ impl PaneStateStore { self.generation.fetch_add(1, Ordering::Relaxed); } + /// Update the tab list for a single pane. Mirrors `set_left`/`set_right` in + /// bumping the generation counter so the MCP `tab` action tool's ack signal + /// (`GenerationAdvanced`) fires when the FE confirms a tab change. Returns + /// `false` if `pane` is neither `"left"` nor `"right"` (the caller already + /// dispatched the event, so silent drop matches the prior behavior). + pub fn set_tabs(&self, pane: &str, tabs: Vec) -> bool { + let pane_state = match pane { + "left" => &self.left, + "right" => &self.right, + _ => return false, + }; + pane_state.write().unwrap().tabs = tabs; + self.generation.fetch_add(1, Ordering::Relaxed); + true + } + pub fn get_generation(&self) -> u64 { self.generation.load(Ordering::Relaxed) } @@ -190,16 +206,16 @@ pub fn update_focused_pane(app: AppHandle, pane: String) { } /// Tauri command to update tab list for a pane from frontend (for MCP state reporting). +/// +/// Delegates to `PaneStateStore::set_tabs`, which bumps the generation counter so the +/// MCP `tab` action tool's ack signal (`GenerationAdvanced`) fires when the FE confirms +/// a tab change. Without that bump the tab tool would time out on every call: tab +/// pushes bypass `set_left`/`set_right`. #[tauri::command] #[specta::specta] pub fn update_pane_tabs(app: AppHandle, pane: String, tabs: Vec) { if let Some(store) = app.try_state::() { - let pane_state = match pane.as_str() { - "left" => &store.left, - "right" => &store.right, - _ => return, - }; - pane_state.write().unwrap().tabs = tabs; + store.set_tabs(pane.as_str(), tabs); } } diff --git a/apps/desktop/src-tauri/src/mcp/tests/ack_system_tests.rs b/apps/desktop/src-tauri/src/mcp/tests/ack_system_tests.rs new file mode 100644 index 00000000..4f750903 --- /dev/null +++ b/apps/desktop/src-tauri/src/mcp/tests/ack_system_tests.rs @@ -0,0 +1,270 @@ +//! Coverage matrix for the MCP action-tool ack contract. +//! +//! ## What this exercises +//! +//! - The `PaneStateStore.generation` counter strictly advances on every `set_left` / +//! `set_right`. The ack helper relies on this invariant. +//! - `update_pane_tabs` bumps generation too. Without this, the `tab` MCP tool would +//! time out on every call (tab pushes bypass `set_left`/`set_right`). +//! - The `SoftDialogTracker` `get_open_types()` lookup behaves the way `AckSignal::SoftDialogAppeared` +//! relies on (set membership, exact ID match). +//! - `AckSignal::describe` produces useful, debuggable error context for each variant. +//! +//! ## What this can't exercise here +//! +//! `wait_for_ack` itself needs an `AppHandle` to call `Manager::try_state`, and we +//! don't run a real Tauri runtime in unit tests. The polling loop is small and pure; +//! its correctness derives from the signal-check primitives covered here plus the +//! generation/dialog primitives this file pins down. End-to-end ack behavior (timeouts +//! firing for stalled FE, dialog-appears acks for confirmation dialogs) is covered by +//! the Playwright MCP E2E suite — those tests drive real MCP calls against a live app +//! and assert the tool responses, which is the only place "FE actually stalled vs FE +//! responsive" can be reproduced faithfully. + +use std::time::{Duration, Instant}; + +use crate::mcp::dialog_state::{KnownDialog, SoftDialogTracker}; +use crate::mcp::pane_state::{PaneFileEntry, PaneState, PaneStateStore, TabInfo}; + +// ── Generation bump invariants ──────────────────────────────────────── + +#[test] +fn set_left_strictly_advances_generation() { + let store = PaneStateStore::new(); + let before = store.get_generation(); + store.set_left(PaneState { + path: "/tmp".to_string(), + ..Default::default() + }); + assert!( + store.get_generation() > before, + "set_left must strictly advance generation (was {before}, now {})", + store.get_generation() + ); +} + +#[test] +fn set_right_strictly_advances_generation() { + let store = PaneStateStore::new(); + let before = store.get_generation(); + store.set_right(PaneState { + path: "/tmp".to_string(), + ..Default::default() + }); + assert!(store.get_generation() > before); +} + +#[test] +fn many_pane_updates_monotonically_advance_generation() { + let store = PaneStateStore::new(); + let mut last = store.get_generation(); + for i in 0..50 { + store.set_left(PaneState { + path: format!("/p{i}"), + ..Default::default() + }); + let now = store.get_generation(); + assert!(now > last, "generation must monotonically advance (i={i})"); + last = now; + } +} + +#[test] +fn set_focused_pane_does_not_advance_generation() { + // Focus is a UI-side concept; it doesn't represent a pane content change so the + // ack helper deliberately doesn't trip on focus flips. Documents the contract. + let store = PaneStateStore::new(); + let before = store.get_generation(); + store.set_focused_pane("right".to_string()); + assert_eq!(store.get_generation(), before); +} + +// ── Generation gate matches what wait_for_ack would check ───────────── + +/// Simulates the precise predicate `AckSignal::GenerationAdvanced { from: pre_gen }` +/// runs inside `wait_for_ack`. Verifies the helper's contract via the store directly: +/// snapshot before the action; once the action's downstream state push happens, the +/// predicate would flip from false to true within the polling window. +#[test] +fn generation_gate_flips_true_after_pane_push() { + let store = PaneStateStore::new(); + let pre_gen = store.get_generation(); + + // Predicate before mutation: false. + assert!(store.get_generation() <= pre_gen); + + // Mutate exactly as `update_left_pane_state` would. + store.set_left(PaneState { + path: "/tmp".to_string(), + files: vec![PaneFileEntry { + name: "f".to_string(), + path: "/tmp/f".to_string(), + is_directory: false, + size: None, + recursive_size: None, + modified: None, + }], + ..Default::default() + }); + + // Predicate after mutation: true. + assert!(store.get_generation() > pre_gen); +} + +#[test] +fn generation_gate_stays_false_when_no_push_happens() { + // The "FE stalled" path: no `set_left`/`set_right` happens after dispatch. The + // predicate must keep returning false so wait_for_ack hits its deadline. This is + // the original bug we're fixing — without the gate, MCP returned OK regardless. + let store = PaneStateStore::new(); + let pre_gen = store.get_generation(); + + // Simulate 50 ms of "nothing happens" — predicate must remain false the whole time. + let start = Instant::now(); + while start.elapsed() < Duration::from_millis(50) { + assert!(store.get_generation() <= pre_gen); + std::thread::sleep(Duration::from_millis(5)); + } +} + +// ── update_pane_tabs MUST also bump generation ───────────────────────── + +/// The `tab` MCP tool acks via `GenerationAdvanced`. Tab updates flow through +/// `update_pane_tabs` → `PaneStateStore::set_tabs`, which bumps generation. If that +/// path ever stops bumping, every `tab` call would time out. This test pins both the +/// helper contract and the side-effect. +#[test] +fn set_tabs_bumps_generation_and_returns_true_for_valid_pane() { + let store = PaneStateStore::new(); + let before = store.get_generation(); + + let ok = store.set_tabs( + "left", + vec![TabInfo { + id: "t1".to_string(), + path: "/tmp".to_string(), + pinned: false, + active: true, + }], + ); + + assert!(ok, "set_tabs must return true for a valid pane name"); + assert!( + store.get_generation() > before, + "set_tabs must bump generation so the `tab` MCP tool can ack" + ); + assert_eq!(store.get_left().tabs.len(), 1); +} + +/// Unknown pane names are a no-op: no panic, no generation bump, return false. +/// Pins the silent-drop contract the `update_pane_tabs` command relies on. +#[test] +fn set_tabs_returns_false_for_unknown_pane() { + let store = PaneStateStore::new(); + let before = store.get_generation(); + + let ok = store.set_tabs( + "middle", + vec![TabInfo { + id: "t1".to_string(), + path: "/tmp".to_string(), + pinned: false, + active: true, + }], + ); + + assert!(!ok, "set_tabs must return false for an unknown pane name"); + assert_eq!(store.get_generation(), before, "unknown pane must not bump generation"); +} + +// ── SoftDialogTracker semantics the ack helper relies on ────────────── + +#[test] +fn soft_dialog_tracker_membership_matches_ack_check() { + let tracker = SoftDialogTracker::new(); + let id = "transfer-confirmation"; + + // Before open: not present. + assert!(!tracker.get_open_types().iter().any(|d| d == id)); + + tracker.open(id.to_string()); + // After open: present (this is what `AckSignal::SoftDialogAppeared` checks). + assert!(tracker.get_open_types().iter().any(|d| d == id)); + + tracker.close(id); + // After close: not present. + assert!(!tracker.get_open_types().iter().any(|d| d == id)); +} + +#[test] +fn soft_dialog_disappeared_signal_flips_after_close() { + // The ack contract for `dialog close ` relies on the tracker + // losing the dialog ID. Pins the semantic: after `close()` the tracker reports + // the id as absent, which is what `AckSignal::SoftDialogDisappeared` checks. + let tracker = SoftDialogTracker::new(); + let id = "mkdir-confirmation"; + + tracker.open(id.to_string()); + let after_open_absent = !tracker.get_open_types().iter().any(|d| d == id); + assert!(!after_open_absent, "dialog must be present right after open"); + + tracker.close(id); + let after_close_absent = !tracker.get_open_types().iter().any(|d| d == id); + assert!( + after_close_absent, + "tracker must report the dialog as gone after close — this is what `SoftDialogDisappeared` polls" + ); +} + +#[test] +fn soft_dialog_tracker_distinguishes_dialog_ids() { + // Important for the ack contract: copy/move both open "transfer-confirmation", + // but delete opens "delete-confirmation". The tracker must distinguish so the + // delete ack doesn't false-positive on a stuck transfer dialog. + let tracker = SoftDialogTracker::new(); + tracker.open("transfer-confirmation".to_string()); + assert!( + !tracker.get_open_types().iter().any(|d| d == "delete-confirmation"), + "open transfer dialog must not be visible as delete dialog" + ); +} + +#[test] +fn known_dialogs_registration_covers_confirmation_dialogs() { + // Pins down the set of soft dialog IDs that the ack contract uses for + // confirmation tools. If a frontend rename drifts these, MCP acks would time + // out silently and this test would flag the regression. + let tracker = SoftDialogTracker::new(); + let dialogs = vec![ + KnownDialog { + id: "transfer-confirmation".to_string(), + description: None, + }, + KnownDialog { + id: "delete-confirmation".to_string(), + description: None, + }, + KnownDialog { + id: "mkdir-confirmation".to_string(), + description: None, + }, + KnownDialog { + id: "new-file-confirmation".to_string(), + description: None, + }, + ]; + tracker.register_known(dialogs); + + let known: Vec = tracker.get_known_dialogs().into_iter().map(|d| d.id).collect(); + for required in [ + "transfer-confirmation", + "delete-confirmation", + "mkdir-confirmation", + "new-file-confirmation", + ] { + assert!( + known.contains(&required.to_string()), + "soft dialog id '{required}' must be registerable (matches the ack tool's expected signal)" + ); + } +} diff --git a/apps/desktop/src-tauri/src/mcp/tests/mod.rs b/apps/desktop/src-tauri/src/mcp/tests/mod.rs index 2618c572..f6006ee0 100644 --- a/apps/desktop/src-tauri/src/mcp/tests/mod.rs +++ b/apps/desktop/src-tauri/src/mcp/tests/mod.rs @@ -1,3 +1,4 @@ +mod ack_system_tests; mod pane_state_tests; mod protocol_tests; mod request_response_tests; diff --git a/apps/desktop/src/lib/file-explorer/pane/DualPaneExplorer.svelte b/apps/desktop/src/lib/file-explorer/pane/DualPaneExplorer.svelte index 80019e3f..4b01d7a3 100644 --- a/apps/desktop/src/lib/file-explorer/pane/DualPaneExplorer.svelte +++ b/apps/desktop/src/lib/file-explorer/pane/DualPaneExplorer.svelte @@ -1935,6 +1935,20 @@ paneRef?.handleKeyDown(event) } + /** + * Opens the entry under the focused pane's cursor and waits for it to complete. + * Unlike `sendKeyToFocusedPane('Enter')`, this returns a Promise so MCP can ack on + * real completion (directory listing finished, or OS open-with-default dispatched) + * rather than guessing via state-push heuristics that don't fire for OS-opened + * files. Used by the `open_under_cursor` round-trip in `mcp-listeners.ts`. + */ + // noinspection JSUnusedGlobalSymbols -- Used dynamically by MCP listeners + export async function openItemUnderCursor(): Promise { + const paneRef = getPaneRef(focusedPane) + if (!paneRef) throw new Error('Focused pane is not available') + await paneRef.openCursorItem() + } + /** * Set sort column for a specific pane (or focused pane if not specified). * Used by command palette. diff --git a/apps/desktop/src/lib/file-explorer/pane/FilePane.svelte b/apps/desktop/src/lib/file-explorer/pane/FilePane.svelte index 1227a989..e488c79a 100644 --- a/apps/desktop/src/lib/file-explorer/pane/FilePane.svelte +++ b/apps/desktop/src/lib/file-explorer/pane/FilePane.svelte @@ -1590,6 +1590,21 @@ return listRef?.getEntryAt(cursorIndex) } + /** + * Opens the entry under the cursor exactly like pressing Enter: navigates into a + * directory or hands a file to the OS default app. Returns a promise that resolves + * once the action completes (or rejects on failure), so callers (the MCP + * `open_under_cursor` round-trip) can ack on real completion rather than guessing. + */ + // noinspection JSUnusedGlobalSymbols -- Used dynamically by DualPaneExplorer/MCP + export async function openCursorItem(): Promise { + const entry = getEntryUnderCursor() + if (!entry) { + throw new Error('No entry under cursor') + } + await handleNavigate(entry) + } + // Exported so DualPaneExplorer can forward keyboard events // noinspection JSUnusedGlobalSymbols -- Used dynamically export function handleKeyDown(e: KeyboardEvent) { diff --git a/apps/desktop/src/lib/file-explorer/pane/types.ts b/apps/desktop/src/lib/file-explorer/pane/types.ts index 68738aaf..dbb4a649 100644 --- a/apps/desktop/src/lib/file-explorer/pane/types.ts +++ b/apps/desktop/src/lib/file-explorer/pane/types.ts @@ -60,6 +60,9 @@ export interface FilePaneAPI { handleKeyDown(e: KeyboardEvent): void handleKeyUp(e: KeyboardEvent): void + /** Opens the entry under the cursor; awaits directory load or OS handoff. */ + openCursorItem(): Promise + /** Type-to-jump: route one printable keystroke into the pane's buffer. */ handleJumpKeystroke(char: string): void /** Type-to-jump: clear the buffer + hide the indicator immediately. */ diff --git a/apps/desktop/src/lib/ipc/bindings.ts b/apps/desktop/src/lib/ipc/bindings.ts index 71a9f51d..931d9508 100644 --- a/apps/desktop/src/lib/ipc/bindings.ts +++ b/apps/desktop/src/lib/ipc/bindings.ts @@ -735,7 +735,14 @@ export const commands = { updateRightPaneState: (state: PaneState) => __TAURI_INVOKE('update_right_pane_state', { state }), // Tauri command to update focused pane from frontend. updateFocusedPane: (pane: string) => __TAURI_INVOKE('update_focused_pane', { pane }), - // Tauri command to update tab list for a pane from frontend (for MCP state reporting). + /** + * Tauri command to update tab list for a pane from frontend (for MCP state reporting). + * + * Delegates to `PaneStateStore::set_tabs`, which bumps the generation counter so the + * MCP `tab` action tool's ack signal (`GenerationAdvanced`) fires when the FE confirms + * a tab change. Without that bump the tab tool would time out on every call: tab + * pushes bypass `set_left`/`set_right`. + */ updatePaneTabs: (pane: string, tabs: TabInfo[]) => __TAURI_INVOKE('update_pane_tabs', { pane, tabs }), // Tauri command: frontend notifies that a soft dialog opened. notifyDialogOpened: (dialogType: string) => __TAURI_INVOKE('notify_dialog_opened', { dialogType }), diff --git a/apps/desktop/src/routes/(main)/explorer-api.ts b/apps/desktop/src/routes/(main)/explorer-api.ts index 503e9be2..e0cb215b 100644 --- a/apps/desktop/src/routes/(main)/explorer-api.ts +++ b/apps/desktop/src/routes/(main)/explorer-api.ts @@ -18,6 +18,7 @@ export interface ExplorerAPI { navigate: (action: 'back' | 'forward' | 'parent') => void getFileAndPathUnderCursor: () => { path: string; filename: string } | null sendKeyToFocusedPane: (key: string) => void + openItemUnderCursor: () => Promise setSortColumn: (column: 'name' | 'extension' | 'size' | 'modified' | 'created', pane?: 'left' | 'right') => void setSortOrder: (order: 'asc' | 'desc' | 'toggle', pane?: 'left' | 'right') => void setSort: ( diff --git a/apps/desktop/src/routes/(main)/mcp-listeners.ts b/apps/desktop/src/routes/(main)/mcp-listeners.ts index e098e1c9..677714c9 100644 --- a/apps/desktop/src/routes/(main)/mcp-listeners.ts +++ b/apps/desktop/src/routes/(main)/mcp-listeners.ts @@ -88,6 +88,28 @@ export async function setupMcpListeners(ctx: McpListenerContext): Promise } }) + // Round-trip for open-under-cursor: backend can't infer outcome from state pushes + // alone (Enter on a non-directory file delegates to the OS default app and produces + // no MCP-observable signal). FE awaits the action and replies via mcp-response. + await listenTauri('mcp-open-under-cursor', (event) => { + const { requestId } = event.payload as { requestId: string } + void (async () => { + const { emit } = await import('@tauri-apps/api/event') + try { + const explorerRef = getExplorer() + if (!explorerRef) { + await emit('mcp-response', { requestId, ok: false, error: 'Explorer is not ready' }) + return + } + await explorerRef.openItemUnderCursor() + await emit('mcp-response', { requestId, ok: true }) + } catch (e) { + const error = e instanceof Error ? e.message : String(e) + await emit('mcp-response', { requestId, ok: false, error }) + } + })() + }) + await listenTauri('mcp-move-cursor', (event) => { const { pane, to, requestId } = event.payload as { pane: 'left' | 'right'; to: number | string; requestId: string } void (async () => { diff --git a/apps/desktop/src/routes/settings/+page.svelte b/apps/desktop/src/routes/settings/+page.svelte index 1b60fe2b..0ae20ac8 100644 --- a/apps/desktop/src/routes/settings/+page.svelte +++ b/apps/desktop/src/routes/settings/+page.svelte @@ -22,6 +22,7 @@ let contentElement: HTMLElement | null = $state(null) let unlistenFocusSelf: UnlistenFn | undefined let unlistenNavigate: UnlistenFn | undefined + let unlistenMcpClose: UnlistenFn | undefined function safeParseSectionParam(raw: string): string[] | null { try { @@ -217,6 +218,19 @@ handleSectionSelect(event.payload.section) }) + // MCP can request that this window close (used by `dialog close settings`). + // Mirror the Escape-key handler: defer the close() by two rAFs so the in-flight + // IPC ack can settle before webkit2gtk begins destroying the webview. + unlistenMcpClose = await listen('mcp-settings-close', () => { + log.debug('Received mcp-settings-close, closing window') + const win = getCurrentWindow() + requestAnimationFrame(() => { + requestAnimationFrame(() => { + void win.close() + }) + }) + }) + log.debug('Settings page ready') } catch (error) { log.error('Failed to initialize settings: {error}', { error }) @@ -231,6 +245,7 @@ // Clean up event listeners unlistenFocusSelf?.() unlistenNavigate?.() + unlistenMcpClose?.() cleanupAccentColor() cleanupTextSize() }) diff --git a/docs/notes/totalcmd-plugin-analysis.md b/docs/notes/totalcmd-plugin-analysis.md index da417470..d50bc718 100644 --- a/docs/notes/totalcmd-plugin-analysis.md +++ b/docs/notes/totalcmd-plugin-analysis.md @@ -574,9 +574,10 @@ The big ones I'd want answered first; these shape the API more than format suppo 1. **Process isolation & crash containment.** TC plugins are in-process DLLs; one bad plugin tanks the app. What's Cmdr's stance? WASM (sandboxed, cross-platform, slower)? Subprocess + JSON-RPC (Tauri-native, easy)? Native dylib (fast, dangerous)? Pick before you design types, as it dictates the ABI. -2. **MCP overlap.** You already expose Cmdr to agents via MCP (port 19224 prod / 19225 dev). MCP is essentially "external tool as a stdio - JSON-RPC plugin." Should Cmdr's plugin API just _be_ MCP-shaped, so the same plugin serves humans and AI agents? This - could collapse two systems into one. Worth a hard look before committing to a bespoke contract. +2. **MCP overlap.** You already expose Cmdr to agents via MCP (port 19224 prod / 19225 dev). MCP is essentially + "external tool as a stdio JSON-RPC plugin." Should Cmdr's plugin API just _be_ MCP-shaped, so the same plugin serves + humans and AI agents? This could collapse two systems into one. Worth a hard look before committing to a bespoke + contract. 3. **Trust/permissions model.** TC has none. Cmdr is going to ship to non-developers. Per-plugin capability declarations (filesystem scope, network scope, exec scope), Tauri-style consent, signed manifests, and a registry: these need to be in v1 of the API, not bolted on. Look at VS Code, Raycast, Obsidian extension permissions. @@ -670,8 +671,8 @@ So the answer is: **yes, MCP-shape for the contract; no, not stdio for everythin transport. You get "humans and AI agents both call plugins through the same surface" without paying stdio overhead for things that need to be fast. -(Side note: this also means your existing MCP server on port 19224 prod / 19225 dev and your future plugin host can share a lot of code; -the plugin host is "MCP, but inbound, with sandboxing.") +(Side note: this also means your existing MCP server on port 19224 prod / 19225 dev and your future plugin host can +share a lot of code; the plugin host is "MCP, but inbound, with sandboxing.") ### 3. Permissions: what good looks like diff --git a/docs/specs/type-to-jump-plan.md b/docs/specs/type-to-jump-plan.md index 7df9e087..579b8109 100644 --- a/docs/specs/type-to-jump-plan.md +++ b/docs/specs/type-to-jump-plan.md @@ -391,9 +391,9 @@ needed), the **Advanced** section component. - Write the Vitest unit tests for the state factory. - Write the a11y Vitest test for `TypeToJumpIndicator.a11y.test.ts`. - Write the Playwright E2E spec covering golden paths. -- **MCP surface**: `DualPaneExplorer.svelte` already exposes pane state to the cmdr MCP server (port 19224 prod / 19225 dev). Add the - type-to-jump buffer + indicator visibility + last matched filename to that surface so agents can drive and assert this - feature in tests. See `src-tauri/src/mcp/CLAUDE.md` for the resource conventions. +- **MCP surface**: `DualPaneExplorer.svelte` already exposes pane state to the cmdr MCP server (port 19224 prod / 19225 + dev). Add the type-to-jump buffer + indicator visibility + last matched filename to that surface so agents can drive + and assert this feature in tests. See `src-tauri/src/mcp/CLAUDE.md` for the resource conventions. - Update CLAUDE.md files noted in § Docs updates. - Run `./scripts/check.sh` (full suite). Fix any warnings (file-length allowlist: leave warnings as warnings per `~/projects-git/vdavid/cmdr/.claude/rules/file-length-allowlist.md`). diff --git a/docs/tooling/mcp.md b/docs/tooling/mcp.md index 04188ffc..59532916 100644 --- a/docs/tooling/mcp.md +++ b/docs/tooling/mcp.md @@ -4,8 +4,9 @@ Two MCP servers are available when the app is running via `pnpm dev`. ## Servers -- **cmdr** (port 19224 prod / 19225 dev): High-level app control: navigation, file operations, search, dialogs, state inspection. This is - the primary way to test and interact with the running app. Architecture docs: `src-tauri/src/mcp/CLAUDE.md`. +- **cmdr** (port 19224 prod / 19225 dev): High-level app control: navigation, file operations, search, dialogs, state + inspection. This is the primary way to test and interact with the running app. Architecture docs: + `src-tauri/src/mcp/CLAUDE.md`. - **tauri** (port 9223): Low-level Tauri access: screenshots, DOM inspection, JS execution, IPC calls. Use for visual verification and UI automation. @@ -34,6 +35,35 @@ Code's MCP integration is connected. Always call `tools/list` first if you're un ./scripts/mcp-call.sh --list-tools ``` +## Action-tool ack contract + +Action tools (`copy`, `move`, `delete`, `mkdir`, `mkfile`, `select`, `toggle_hidden`, `set_view_mode`, `sort`, `tab`, +`nav_to_parent`, `nav_back`, `nav_forward`, and `dialog` open/close/focus/confirm) now wait for the frontend to +acknowledge the dispatched action before returning `OK`. Default budget is 1500 ms; the navigation family +(`nav_to_parent`, `nav_back`, `nav_forward`) gets 5 s because remote backends (SMB, MTP) routinely need a few seconds +for their directory listing even on success. `open_under_cursor` uses a true round-trip (`mcp-response` from the FE +after the action resolves, 5 s timeout) because Enter on a non-directory file delegates to the OS default app, which +produces neither a state push nor a viewer window — the FE is the only source of truth for "this finished." If the FE is +stalled, the tool returns a typed error naming the missing signal — no more false-positive `OK`s. + +`refresh` is the one tool that stays fire-and-forget for now: the FE skips state pushes when the re-listing is +byte-identical to the cached state, so there's no reliable ack signal. Search the Rust codebase for `TODO(mcp-ack):` to +follow this. + +What this means for automation: + +- `OK` is now a meaningful contract: the FE accepted the action. The downstream operation may still be running (a copy + of 10 GB returns `OK` when the FE accepts the dialog, not when bytes finish). +- For long-running operations, poll completion via the `await` tool just like before. +- Timeouts surface as JSON-RPC errors with a clear message ("Action not acknowledged by backend within 1500 ms (waiting + for: …)"). Treat them as real failures — don't retry blindly; check `cmdr://state` to triage. +- `dialog close file-viewer` on a path that isn't open returns an `invalid_params` error fast ("No file viewer windows + are open"); closing one of multiple viewers acks as soon as the count drops by one, not when all viewers vanish. +- Very slow remote shares can still exceed even the 5 s nav budget. If a nav tool times out but the navigation actually + succeeds in the background, follow up with `await` (`path` / `path_contains`) to confirm the destination landed. + +Architecture details: `apps/desktop/src-tauri/src/mcp/CLAUDE.md` § "Action-tool ack contract". + ## Connection resilience The MCP server goes down during hot reloads (up to 15s for Rust changes, up to 3s for frontend changes). Multiple agents diff --git a/scripts/check/CLAUDE.md b/scripts/check/CLAUDE.md index 1232b928..4d9252b8 100644 --- a/scripts/check/CLAUDE.md +++ b/scripts/check/CLAUDE.md @@ -218,16 +218,16 @@ before tests. ## Apps and check counts -| App | Tech | Checks | -| ---------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Desktop | Rust | rustfmt, clippy, cargo-audit, cargo-deny, cargo-machete, cargo-udeps (CI-only), jscpd, log-error-macro, error-string-match, bindings-fresh, ipc-enum-camelcase, tests, integration-tests (Docker SMB), tests-linux (slow) | -| Desktop | Svelte | prettier, eslint, eslint-typecheck (slow), stylelint, css-unused, a11y-contrast, svelte-check, import-cycles, knip, type-drift, tests, e2e-linux-typecheck, e2e-linux (slow), e2e-playwright (slow) | -| Website | Astro | prettier, eslint, typecheck, build, html-validate, e2e | -| Website | Docker | docker-build | -| API server | TS | oxfmt, eslint, typecheck, tests | -| Scripts | Go | gofmt, go-vet, staticcheck, ineffassign, misspell, gocyclo, nilaway, deadcode, go-tests, govulncheck | -| Other | Metrics | file-length (warn-only), CLAUDE.md-reminder (warn-only), changelog-commit-links | -| Other | Security | workflows-hardening (SHA-pinning, no `pull_request_target`, job-scoped `id-token: write`) | +| App | Tech | Checks | +| ---------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Desktop | Rust | rustfmt, clippy, cargo-audit, cargo-deny, cargo-machete, cargo-udeps (CI-only), jscpd, log-error-macro, error-string-match, bindings-fresh, ipc-enum-camelcase, tests, integration-tests (Docker SMB), tests-linux (slow) | +| Desktop | Svelte | prettier, eslint, eslint-typecheck (slow), stylelint, css-unused, a11y-contrast, svelte-check, import-cycles, knip, type-drift, tests, e2e-linux-typecheck, e2e-linux (slow), e2e-playwright (slow) | +| Website | Astro | prettier, eslint, typecheck, build, html-validate, e2e | +| Website | Docker | docker-build | +| API server | TS | oxfmt, eslint, typecheck, tests | +| Scripts | Go | gofmt, go-vet, staticcheck, ineffassign, misspell, gocyclo, nilaway, deadcode, go-tests, govulncheck | +| Other | Metrics | file-length (warn-only), CLAUDE.md-reminder (warn-only), changelog-commit-links | +| Other | Security | workflows-hardening (SHA-pinning, no `pull_request_target`, job-scoped `id-token: write`) | ## Output format @@ -265,11 +265,10 @@ active. See comment in `src-tauri/deny.toml`. **Decision**: every operational `cargo` command in checks passes `--locked`. **Why**: without it, cargo silently re-resolves `Cargo.lock` whenever upstream metadata shifts (a yank, a new transitive dep version). For a 1028-crate -lockfile, that resolution window is wide and lets a freshly-published malicious version land mid-build. `--locked` -fails loudly if the lockfile would change. Applies to `cargo clippy`, `cargo nextest run` (in both `desktop-rust-tests` -and `desktop-rust-integration-tests`), and `cargo +nightly udeps`. Audit/deny/machete read `Cargo.lock` without -updating it, so `--locked` is moot for them, but the install of those tools still uses `--locked` to lock the tool's -own dep tree. +lockfile, that resolution window is wide and lets a freshly-published malicious version land mid-build. `--locked` fails +loudly if the lockfile would change. Applies to `cargo clippy`, `cargo nextest run` (in both `desktop-rust-tests` and +`desktop-rust-integration-tests`), and `cargo +nightly udeps`. Audit/deny/machete read `Cargo.lock` without updating it, +so `--locked` is moot for them, but the install of those tools still uses `--locked` to lock the tool's own dep tree. **Decision**: every tool install pins `--version` and `--locked` (cargo) or `@vX.Y.Z` (Go). **Why**: an unpinned tool install (`cargo install cargo-audit` or `EnsureGoTool(..., "@latest")`) means each fresh checkout pulls whatever's @@ -280,15 +279,14 @@ equivalent of the pnpm `minimum-release-age` defense (a fresh version can't land **Why**: cmdr's workflows are already correctly hardened (every third-party action is SHA-pinned with a comment, no `pull_request_target` triggers, no workflow-scoped `id-token: write`). Without an automated guard, a future PR or a Renovate misconfiguration could silently regress any of those without anyone noticing in review. The check fails on -tag/branch-pinned third-party actions, on `pull_request_target` triggers (wave-4's entry vector), and on -workflow-scoped `id-token: write` (must be job-scoped per the wave-4 OIDC-token-extraction lesson). Local actions -(`./...`) are exempt. +tag/branch-pinned third-party actions, on `pull_request_target` triggers (wave-4's entry vector), and on workflow-scoped +`id-token: write` (must be job-scoped per the wave-4 OIDC-token-extraction lesson). Local actions (`./...`) are exempt. **Decision**: `govulncheck` runs against every Go module. **Why**: cargo-audit covers Rust deps; nothing covered Go until now. `govulncheck` is static-analysis-based, so it only flags vulns actually reachable from the code (low false -positive rate). Most of cmdr's Go modules are dep-free tooling scripts but still call into the Go stdlib, which gets -its own CVEs; the check found 7 real reachable stdlib vulns the first time it ran (fixed by bumping mise's Go pin). -Mirrors the cargo-audit role on the Rust side. +positive rate). Most of cmdr's Go modules are dep-free tooling scripts but still call into the Go stdlib, which gets its +own CVEs; the check found 7 real reachable stdlib vulns the first time it ran (fixed by bumping mise's Go pin). Mirrors +the cargo-audit role on the Rust side. **Decision**: `cfg-gate` check to catch ungated macOS-only crate imports. **Why**: Rust code using macOS-only crates (from `[target.'cfg(target_os = "macos")'.dependencies]`) compiles fine on macOS but fails on Linux if the `use` isn't