Session log: MCP v2 PR-3 + PR-3.5 + polish bundle

Document the spawn-completion oneshot chain, the rate limiter,
SSH-extra confirm banner, two-switch SSH safeguards, the
spawn_pane SSH-schema split, instructions refresh, and the host
manager Connect button. Plus the four cross-IPC integration bugs
(Emitter trait, McpError 'static, StrictMode listen() race,
rename_all camelCase) and the ErrorBoundary that caught the last
one. Open follow-ups in priority order.
This commit is contained in:
megaproxy 2026-05-26 15:21:40 +01:00
parent 6da7523993
commit 71f330e934

View file

@ -51,6 +51,52 @@ Durable memory for this project. Read at session start, update before session en
## Session log
### 2026-05-26 — MCP v2 PR-3 + PR-3.5: powerful writes + SSH safeguards + host-manager Connect button
Commits `bf2810a` (PR-3 + PR-3.5) and `6da7523` (polish bundle). 8 of 9 planned v2 tools are now live — only `add_host` (PR-4) remains.
**PR-3 added the three highest-power tools:** `write_pane`, `spawn_pane`, `connect_host`.
- **`write_pane`** sends keystrokes to a pane's PTY. `args_repr` is the decoded text itself (not a summary) so the hard-deny matcher and user-policy globs evaluate against the exact bytes Claude wants to send. **Per-pane token bucket rate limiter**: 30 calls capacity + 3/s refill, sized so a sustained loop trips it within ~2s while normal use never hits it. Rate-limited calls don't emit audit rows (would just spam during an attack); they get a `tracing::warn!`. Frontend `truncateForSummary` caps text shown in the modal + audit row to ~60 chars and escapes control chars, so a pasted token doesn't echo verbatim into the UI.
- **`spawn_pane`** + **`connect_host`** required a new architectural piece: a **spawn-completion oneshot chain** in App.tsx. Backend MCP tools that mutate the tree can't reply until the new pane has been registered with a PaneId — and that only happens after React mounts XtermPane and the Tauri `spawn_pane` command returns. New `pendingPaneRegistrations` Map<NodeId, resolve_fn>; `registerPaneId` fires waiters; `waitForPaneRegistration(leafId, timeoutMs)` returns a Promise the handler awaits. 15s timeout for WSL (covers cold distro start), 30s for SSH (covers handshake + auth), 60s outer cap in `dispatch_action` as a fail-safe.
- New tree helper `splitLeafWith(root, parentId, orient, leaf)` — like `splitLeaf` but takes a caller-built `LeafNode` with a pre-generated id instead of minting one inside. The handler needs the id up front so it can register a waiter for it before setTree commits.
- **SSH-extra confirm modal**`McpConfirmSpec` carries an optional `ssh: {hostLabel}` context; when set, the modal renders a red warning banner explaining that pattern matching only sees the bytes we send (the remote shell expands aliases/subshells before executing) and the hard-deny still applies but this is best-effort. Detection lives in `buildConfirmInfo` (renamed from `buildConfirmSummary`).
**PR-3.5 — SSH safeguards.** Two new switches on `McpPolicy.sshSafeguards`, both default off (safest):
- `allowOpenSsh` — when off, `connect_host` and `spawn_pane(kind=ssh)` refuse server-side with a clear `ssh-disabled:` message pointing at the Policy tab. User opens SSH manually via the titlebar 🔑 picker.
- `autoAllowSpawnedSsh` — when off, SSH panes Claude spawns start with `mcpAllow=false`. User must explicitly toggle 🤖 before Claude can read scrollback or write keystrokes. UI disables the second checkbox when the first is off (visual "this depends on that").
Together: fresh install + safeguards = Claude has *no* ability to autonomously touch SSH. Power-user can flip switches independently for graduated trust.
**Polish bundle (`6da7523`) — three small follow-ups from PR-3 testing:**
1. **Removed SSH variant from `mcp::spawn_pane`'s schema entirely.** New `McpSpawnSpec` enum (Wsl | Powershell only), used only by `SpawnPaneArgs`. Internal `pty::SpawnSpec` keeps all three for the existing frontend-driven spawn path. Reason: `spawn_pane(kind=ssh)` was a half-broken path — required `host` as a mandatory field even when `hostId` was given, so serde rejected the natural "spawn to a saved host" shape. Claude now sees two clearly-scoped tools and routes "open a pane to testbox" to `connect_host` automatically (verified via natural-language test).
2. **Refreshed `with_instructions` + module header comment.** Both still claimed "read-only v1" long after the write surface landed; Claude was refusing tools on first contact citing the stale instructions. New text describes the actual surface, points at `connect_host` for SSH, mentions the policy/safeguards gates.
3. **Connect button in the SSH hosts manager.** The dialog previously had only Edit — users (rightly) expected clicking a saved host to spawn an SSH pane. Green button next to Edit, wrapped in a flex container so the row's `space-between` layout keeps both actions right-aligned. Closes the manager on click and splits off the active pane with smart-orient.
**Four integration bugs + recurring patterns worth remembering:**
1. **`Tauri 2` `AppHandle::emit` moved onto the `tauri::Emitter` trait** — needs `use tauri::Emitter;`. The error message tells you (well, with `--explain`), but it's an easy stumbling block.
2. **`McpError` constructors take `impl Into<Cow<'static, str>>`.** Pass owned `String` from `format!(...)`, not `&format!(...)` — the temporary is dropped before the `'static` lifetime can be satisfied.
3. **React 18 `StrictMode` race with `listen()` inside `useEffect`.** Always use the cancelled-flag pattern; never just `let un; .then(fn => un = fn)` because the cleanup runs before the Promise resolves on the pretend-unmount.
4. **Serde rename mismatch between Rust and TS.** Rust `pub ssh_safeguards` serializes as `ssh_safeguards` unless the struct has `#[serde(rename_all = "camelCase")]`. The frontend reading `policy.sshSafeguards` got `undefined`, threw during render, blanked the whole app. Add `rename_all` on every struct that crosses the IPC boundary.
**New defensive primitive: `ErrorBoundary.tsx`.** Wraps the App root + each MCP panel tab. A render exception anywhere shows a small red error card with the message + a "Try again" button instead of unmounting the entire React tree and showing a black window. Caught bug #4 above. Wrap any future high-risk component too (especially anything reading from MCP state).
**5 of 9 v2 tools verified end-to-end with Claude:** set_label, write_pane, spawn_pane (local), connect_host, close_pane (regression). The hard-deny + rate-limit + audit + confirm + Always-Allow flow all working.
Open follow-ups specific to this session, ordered by priority:
- **PR-4: `add_host` + `extraArgs` sanitiser.** Lets Claude register new SSH hosts in hosts.json. Sanitiser must reject `ProxyCommand`, `LocalCommand`, `KnownHostsCommand`, `PermitLocalCommand=yes`, and any `-o` keys that take a shell command — those are local-RCE-at-ssh-invocation primitives (CVE-2023-51385 class). Probably also bundle `delete_host` for symmetry. Consider a third SSH safeguard switch ("Allow Claude to save new SSH hosts", default off) to gate the new tool the same way `allowOpenSsh` gates `connect_host`. ~3-4 hours total.
- **v2.1 — wire the `PolicyClassifier` hook.** Currently scaffolded as `NoopClassifier`; calls falling through to Ask could optionally be upgraded to Allow by a small LLM (Haiku via Anthropic API is the cheapest path; Ollama for local). Trade-offs: API key surface in settings, latency on Ask calls, predictability vs. fewer prompts. Defer until the prompt fatigue actually starts biting in daily use.
- **PowerShell hard-deny patterns.** Currently the 10 baked-in patterns are POSIX-only (rm -rf /, mkfs, etc.). PowerShell equivalents (`Remove-Item -Recurse -Force C:\`, `Format-Volume`, etc.) deserve the same circuit-breaker. Add when users actually run write_pane against PowerShell panes in anger.
- **Per-leaf-shellKind policy scoping.** Today `write_pane(*)` in the Allow bucket allows ALL write_pane calls, including SSH ones — which bypasses the SSH-extra warning modal. Want something like `write_pane(local)` and `write_pane(ssh)` discriminators so users can silent-allow locally while still asking on SSH. Schema design needed: extend the glob matcher with shellKind predicates, or just hard-code that the bare-tool-name allow rule never applies to SSH targets. Probably the latter for simplicity.
- **`.mcpb` bundle** for one-click Claude Desktop install — would package the `mcp-remote` shim invocation + bearer placeholder. Same scope it was in earlier sessions.
- **Audit-log persistence.** Currently ephemeral ring of 200. A `mcp-audit.jsonl` append-only file in app data dir would let users see "what did Claude do overnight." Trade-off: secrets-in-summaries risk if `write_pane` text leaks past the 80-char truncation. Defer until requested.
- **Confirm-modal queue UX.** FIFO single-modal-at-a-time today. If Claude burst-fires many tool calls, the user serially clicks through them. Adding a "reject all pending" button is cheap if it ever annoys.
- **Module-level header in `mcp.rs` still calls out the 9-tool list** — keep this in sync if you add or rename tools. The MCP `with_instructions` text and the tool descriptions also need attention every time the surface changes (Claude reads both for routing decisions).
### 2026-05-26 — MCP v2 PR-2: tree-shape writes (close, swap, promote, apply_preset)
Commit `e0ce223`. Four more tools wired through the existing PR-1b dispatcher pipeline (`dispatch_action` → policy check → confirm modal → audit), all touching the layout tree but not PTYs or spawn. Mechanically the same shape as `set_label`: define args struct on backend, validate via `require_visible_leaf` (factored out — 5 tools now use it), dispatch with stable `args_repr`, frontend `runMcpHandler` case applies the mutation via the same setters the UI uses.