Skip to content

Commit df11cae

Browse files
committed
MCP: revert refresh to fire-and-forget, doc TODO
E2E flagged the right edge case: when the FE re-lists in response to `mcp-refresh` and the new listing is byte-identical to the cached state (common on MTP/SMB right after an operation already pushed fresh state), the FE skips the `update_*_pane_state` call to avoid a redundant generation bump. That left `refresh`'s ack helper waiting for a signal that never arrived, timing out at 1500 ms. - Revert `execute_refresh` to fire-and-forget with a `// TODO(mcp-ack):` comment documenting the two acceptable follow-ups (round-trip ack, or always-bump-generation on re-list). - Update both CLAUDE.md and the user-facing `docs/tooling/mcp.md` to call out that `refresh` is the one exception in the contract. - Every other ack-wrapped tool stays as-is; the smb/mtp E2E suite exercises `copy`/`move`/`delete`/`open_under_cursor`/`move_cursor` and they all passed against the new contract.
1 parent 48a9701 commit df11cae

3 files changed

Lines changed: 24 additions & 13 deletions

File tree

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ The mechanism lives in `executor/ack.rs`. Each tool:
6565

6666
| Variant | Fires when | Used by |
6767
| ------------------------ | ----------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- |
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`) |
68+
| `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. |
6969
| `SoftDialogAppeared(id)` | A soft dialog with that ID is in `SoftDialogTracker` | Confirmation dialogs from `copy`/`move`/`delete` (autoConfirm: false), `mkdir`, `mkfile`; `dialog open about` |
7070
| `WindowAppeared(label)` | A `webview_windows()` entry matches the label (exact, or `viewer-*`) | `dialog open settings|file-viewer`, `dialog focus` |
7171
| `WindowDisappeared` | The matching `webview_windows()` entry is gone | `dialog close settings|file-viewer|about` |
@@ -77,6 +77,8 @@ The 1500 ms budget is a backend-side latency budget, not a client-facing knob: M
7777

7878
**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.
7979

80+
**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.
81+
8082
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.
8183

8284
### Configuration (`config.rs`)

apps/desktop/src-tauri/src/mcp/executor/file_ops.rs

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -154,16 +154,21 @@ pub async fn execute_mkfile<R: Runtime>(app: &AppHandle<R>) -> ToolResult {
154154
Ok(json!("OK: Create file dialog opened."))
155155
}
156156

157-
/// Execute refresh command. Ack: pane generation advances after the FE re-pushes state.
157+
/// Execute refresh command.
158+
///
159+
/// TODO(mcp-ack): No reliable ack signal yet. `refresh` asks the FE to re-list the
160+
/// current pane. If the listing comes back byte-identical to the cached state (very
161+
/// common for MTP and SMB after an operation already pushed fresh state, or for any
162+
/// pane that hasn't drifted since the last update), the FE skips the
163+
/// `update_*_pane_state` call to avoid a redundant generation bump. That leaves
164+
/// `refresh` without a `GenerationAdvanced` signal.
165+
///
166+
/// Two acceptable follow-ups: (1) switch to `mcp_round_trip` so the FE explicitly
167+
/// emits `mcp-response` once re-list completes regardless of whether state changed;
168+
/// (2) always bump generation on re-list. Until one of those, refresh stays
169+
/// fire-and-forget. The original bug is less acute here than for destructive tools.
158170
pub async fn execute_refresh<R: Runtime>(app: &AppHandle<R>) -> ToolResult {
159-
let pre_gen = snapshot_generation(app);
160171
app.emit("mcp-refresh", ())?;
161-
wait_for_ack(
162-
app,
163-
AckSignal::GenerationAdvanced { from: pre_gen },
164-
DEFAULT_ACK_TIMEOUT,
165-
)
166-
.await?;
167172
Ok(json!("OK: Pane refreshed"))
168173
}
169174

docs/tooling/mcp.md

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,14 @@ Code's MCP integration is connected. Always call `tools/list` first if you're un
3636

3737
## Action-tool ack contract
3838

39-
Action tools (`copy`, `move`, `delete`, `mkdir`, `mkfile`, `refresh`, `select`, `toggle_hidden`, `set_view_mode`,
40-
`sort`, `tab`, `open_under_cursor`, `nav_to_parent`, `nav_back`, `nav_forward`, and `dialog` open/close/focus/confirm)
41-
now wait for the frontend to acknowledge the dispatched action before returning `OK`. Default budget is 1500 ms. If the
42-
FE is stalled, the tool returns a typed error naming the missing signal — no more false-positive `OK`s.
39+
Action tools (`copy`, `move`, `delete`, `mkdir`, `mkfile`, `select`, `toggle_hidden`, `set_view_mode`, `sort`, `tab`,
40+
`open_under_cursor`, `nav_to_parent`, `nav_back`, `nav_forward`, and `dialog` open/close/focus/confirm) now wait for the
41+
frontend to acknowledge the dispatched action before returning `OK`. Default budget is 1500 ms. If the FE is stalled,
42+
the tool returns a typed error naming the missing signal — no more false-positive `OK`s.
43+
44+
`refresh` is the one tool that stays fire-and-forget for now: the FE skips state pushes when the re-listing is
45+
byte-identical to the cached state, so there's no reliable ack signal. Search the Rust codebase for `TODO(mcp-ack):` to
46+
follow this.
4347

4448
What this means for automation:
4549

0 commit comments

Comments
 (0)