diff --git a/.changeset/hotkeys-capture-and-fire-robustness.md b/.changeset/hotkeys-capture-and-fire-robustness.md new file mode 100644 index 0000000..49010cc --- /dev/null +++ b/.changeset/hotkeys-capture-and-fire-robustness.md @@ -0,0 +1,25 @@ +--- +"acture-hotkeys": patch +--- + +Fix keymap-capture and hotkey-fire correctness bugs (adversarial pre-wiring +review): + +- **Spacebar is now bindable.** `tokenFromEvent` emitted `' '` for the space key, + which tinykeys' space-separated chord parser strips to an empty, never-matching + key — a captured Space shortcut silently never fired. It now emits `'Space'` + (matched via `event.code`). +- **A throwing `when`-clause no longer crashes the key handler.** A function + `when` that throws (e.g. reads a not-yet-populated context slice) previously + killed the whole `keydown` handler and swallowed the remaining fallback + candidates on that key; the fire loop now treats a throw as not-applicable and + tries the next command (fail-closed, matching `registry.dispatch`). +- **Shadow-DOM inputs are respected.** `DEFAULT_IGNORE` now resolves the real + target via `event.composedPath()[0]`, so typing in an `` inside a web + component's shadow DOM no longer triggers hotkeys. +- **No more phantom self-conflicts.** `resolveKeys` de-duplicates, so an `add` + override (or preset) that restates a command's existing key no longer binds it + twice or makes `detectConflicts` report the command as conflicting with itself. +- **The React `useHotkeys` hook no longer freezes stale callbacks.** `onDispatched` + and `shouldIgnoreEvent` are routed through refs, so a fresh inline closure each + render runs with current state instead of the values captured at first bind. diff --git a/.changeset/mcp-serialization-and-getstate-collision.md b/.changeset/mcp-serialization-and-getstate-collision.md new file mode 100644 index 0000000..cebcf3d --- /dev/null +++ b/.changeset/mcp-serialization-and-getstate-collision.md @@ -0,0 +1,22 @@ +--- +"acture-mcp-server": patch +--- + +Harden the MCP boundary against non-serializable app values and tool-name +collisions (adversarial pre-wiring review): + +- **Errors-as-data is never broken by a thrown `JSON.stringify`.** `callGetState`, + `readResource`, and `formatToolResponse` now serialize app-supplied values + through a guarded `safeStringify`. A `BigInt`, a circular reference, or a + throwing `toJSON` in a view value or command result previously threw a + `TypeError` past the tool-call handler (a JSON-RPC protocol crash instead of + an `isError` result); it now returns an `unserializable_state` payload. An + `err(...)` whose `details` is unserializable still delivers its `code`/`message`. +- **`ok(undefined)` no longer emits a malformed `text: undefined` content field** + (which drops on the wire and fails a strict client's `CallToolResult` schema); + it serializes as `"null"`. +- **`createMcpServer({ getStateTool })` fails fast on a name collision.** A command + whose sanitized wire name equals the getState tool name (e.g. `app.getState` → + `app_getState`) would emit a duplicate tool (strict hosts reject the entire + `tools/list`) and silently shadow the command on `tools/call`. Construction now + throws a clear, actionable error instead. diff --git a/.claude/skills/acture-ai-assistant/SKILL.md b/.claude/skills/acture-ai-assistant/SKILL.md index 31f3b90..1f6ac52 100644 --- a/.claude/skills/acture-ai-assistant/SKILL.md +++ b/.claude/skills/acture-ai-assistant/SKILL.md @@ -39,7 +39,7 @@ Two load-bearing points that pattern gets right and a naive gate gets wrong: - **The token is minted by the RUNTIME after a human approves, and never returned to the model.** If the gate put a usable token in the `confirmation_required` proposal, the model — which reads that tool result — would lift it and **self-approve**, defeating HITL. The proposal carries only `{command, params, preview}`; the runtime mints the one-use token out-of-band on human approval and re-dispatches. - **Tokens are one-use and bound to the exact `{command, params}`** (a `createApprovalStore` with `approve`/`consume`), so an approval can't be replayed against different args. -**Design settled: middleware + convention, not `CommandRecord` fields.** `getRisk(id)` is an external map, so the closed record is untouched (the fields alternative opens the guarded surface — needs the named-need test). And like macros, the ~40-line gate ships as a **pattern**, not a package (hard-don't #2; no natural package home). This keeps confirmation caller-independent, declarative, and schema-validated regardless of surface — and `preview` can call a *view* to show *what will change*. +**Design settled: middleware + convention, not `CommandRecord` fields.** `getRisk(id)` is an external map, so the closed record is untouched (the fields alternative opens the guarded surface — needs the named-need test). And like macros, the ~40-line gate ships as a **pattern**, not a package (hard-don't #2; no natural package home). This keeps confirmation declarative and schema-validated, uniform across every AI surface **that labels its channel** — the load-bearing wiring: the gate fires only on `channel: 'assistant'`, so `toAITools`/`createMcpServer` must be given that context or destructive calls run ungated (see the doc's Piece 1 warning). And `preview` can call a *view* to show *what will change*. ### 3. The dispatch chain is a macro — undo / replay / test fixtures diff --git a/docs/hand-written-assistant-runtime.md b/docs/hand-written-assistant-runtime.md index eb44657..46d90fb 100644 Binary files a/docs/hand-written-assistant-runtime.md and b/docs/hand-written-assistant-runtime.md differ diff --git a/packages/hotkeys/src/bind.test.ts b/packages/hotkeys/src/bind.test.ts index fd0b5f9..48e94f8 100644 --- a/packages/hotkeys/src/bind.test.ts +++ b/packages/hotkeys/src/bind.test.ts @@ -163,6 +163,54 @@ describe('acture-hotkeys', () => { input.remove(); }); + it('a throwing when-clause does not crash the handler — a fallback candidate still fires', async () => { + const registry = createRegistry(); + const execFallback = vi.fn(() => ok(undefined)); + // First candidate: a function when-clause that throws (reads a ctx slice + // that isn't populated). Must not swallow the fallback below. + registry.register( + defineCommand({ + id: 'thrower', + title: 'T', + keybinding: 'g', + when: (ctx: { editor?: { focused?: boolean } }) => ctx.editor!.focused!, + execute: () => ok(undefined), + }), + ); + registry.register( + defineCommand({ id: 'fallback', title: 'F', keybinding: 'g', execute: execFallback }), + ); + const stop = bindHotkeys(registry, { target: window, contextProvider: () => ({}) }); + pressKey(window, 'g'); + await new Promise((r) => setTimeout(r, 0)); + expect(execFallback).toHaveBeenCalledTimes(1); + stop(); + }); + + it('ignores hotkeys while typing in a shadow-DOM input (composedPath retargeting)', async () => { + const registry = createRegistry(); + const execute = vi.fn(() => ok(undefined)); + registry.register( + defineCommand({ id: 'cmd', title: 'C', keybinding: 'g', execute }), + ); + const host = document.createElement('div'); + document.body.appendChild(host); + const root = host.attachShadow({ mode: 'open' }); + const input = document.createElement('input'); + root.appendChild(input); + const stop = bindHotkeys(registry, { target: window }); + // composed: true so the event crosses the shadow boundary to the window + // listener; event.target retargets to the host, but composedPath()[0] is + // the real inner — which DEFAULT_IGNORE must detect. + input.dispatchEvent( + new KeyboardEvent('keydown', { key: 'g', bubbles: true, composed: true, cancelable: true }), + ); + await new Promise((r) => setTimeout(r, 0)); + expect(execute).not.toHaveBeenCalled(); + stop(); + host.remove(); + }); + it('rebinds on commandsChanged when a new keybinding is registered', async () => { const registry = createRegistry(); const stop = bindHotkeys(registry, { target: window }); diff --git a/packages/hotkeys/src/bind.ts b/packages/hotkeys/src/bind.ts index 9e7c00c..c72bfb1 100644 --- a/packages/hotkeys/src/bind.ts +++ b/packages/hotkeys/src/bind.ts @@ -69,8 +69,14 @@ export interface HotkeyBindingDescriptor { } const DEFAULT_IGNORE: (e: KeyboardEvent) => boolean = (event) => { - const t = event.target; - if (t === null || !(t instanceof Element)) return false; + // Resolve the innermost real target. For an `` rendered inside a web + // component's shadow DOM, `event.target` is retargeted to the shadow HOST, so + // a naive tagName check misses it and the hotkey fires while the user types. + // `composedPath()[0]` pierces the shadow boundary; fall back to `event.target` + // where composedPath is unavailable or empty (e.g. outside dispatch). + const path = typeof event.composedPath === 'function' ? event.composedPath() : []; + const t = path.length > 0 ? path[0] : event.target; + if (t === null || t === undefined || !(t instanceof Element)) return false; const tag = t.tagName; if (tag === 'INPUT' || tag === 'TEXTAREA' || tag === 'SELECT') return true; if ((t as HTMLElement).isContentEditable) return true; @@ -111,7 +117,18 @@ export function bindHotkeys( // First-registered-wins under matching context (research-1; user- // confirmed escalation #1). Iterate insertion-ordered descriptors. for (const desc of descriptors) { - if (!evaluateWhen(desc.when, ctx)) continue; + let applies: boolean; + try { + applies = evaluateWhen(desc.when, ctx); + } catch { + // A function when-clause that throws (e.g. reads a ctx slice that + // isn't populated yet) must not crash the whole key handler and + // swallow the remaining fallback candidates — treat it as + // not-applicable and try the next command, mirroring the + // fail-closed discipline of registry.dispatch. + continue; + } + if (!applies) continue; event.preventDefault(); void registry .dispatch(desc.commandId, undefined, ctx) diff --git a/packages/hotkeys/src/keymap.test.ts b/packages/hotkeys/src/keymap.test.ts index 8689d45..9d6e4aa 100644 --- a/packages/hotkeys/src/keymap.test.ts +++ b/packages/hotkeys/src/keymap.test.ts @@ -30,6 +30,19 @@ describe('resolveKeys', () => { expect(resolveKeys(cmd('a', 'g'), km({ a: { kind: 'add', keys: ['x'] } }))).toEqual(['g', 'x']); expect(resolveKeys(cmd('a', 'g'), km({ a: { kind: 'remove' } }))).toEqual([]); }); + + it('de-duplicates keys (an add/replace that restates an existing binding)', () => { + const km = (o: UserKeymap['overrides']): UserKeymap => ({ version: 1, overrides: o }); + // 'add' re-adding the record default must not bind the key twice. + expect(resolveKeys(cmd('a', 'g'), km({ a: { kind: 'add', keys: ['g'] } }))).toEqual(['g']); + expect(resolveKeys(cmd('a', 'g'), km({ a: { kind: 'add', keys: ['g', 'x', 'x'] } }))).toEqual([ + 'g', + 'x', + ]); + expect(resolveKeys(cmd('a', 'g'), km({ a: { kind: 'replace', keys: ['x', 'x'] } }))).toEqual([ + 'x', + ]); + }); }); describe('collectBindings with a keymap', () => { @@ -88,6 +101,15 @@ describe('detectConflicts', () => { expect(conflicts).toHaveLength(1); expect(conflicts[0]!.commandIds.sort()).toEqual(['a', 'b']); }); + + it('does not report a command as conflicting with itself (add restates its default)', () => { + const registry = createRegistry(); + registry.register(defineCommand({ id: 'a', title: 'A', keybinding: 'g', execute: () => ok(undefined) })); + // A preset / remap UI that re-adds the key the command already has must + // NOT surface a bogus "'a' conflicts with 'a'" entry. + const keymap: UserKeymap = { version: 1, overrides: { a: { kind: 'add', keys: ['g'] } } }; + expect(detectConflicts(registry, keymap)).toEqual([]); + }); }); describe('tokenFromEvent', () => { @@ -109,6 +131,15 @@ describe('tokenFromEvent', () => { '$mod+KeyW', ); }); + + it('emits a matchable "Space" token for the spacebar (not an empty key)', () => { + // event.key for the spacebar is a literal ' '; a ' ' token is stripped by + // tinykeys' space-separated chord parser and never fires. 'Space' matches + // via event.code. + expect(tokenFromEvent(ev({ key: ' ' }))).toBe('Space'); + expect(tokenFromEvent(ev({ key: ' ', ctrlKey: true }))).toBe('$mod+Space'); + expect(tokenFromEvent(ev({ key: ' ', shiftKey: true }))).toBe('Shift+Space'); + }); }); describe('isReservedCombo', () => { diff --git a/packages/hotkeys/src/keymap.ts b/packages/hotkeys/src/keymap.ts index ac731e8..6f92434 100644 --- a/packages/hotkeys/src/keymap.ts +++ b/packages/hotkeys/src/keymap.ts @@ -47,10 +47,18 @@ function normalizeKeybinding(kb: Keybindable['keybinding']): string[] { return typeof kb === 'string' ? [kb] : [...kb]; } +/** Drop duplicate key sequences, preserving first-seen order. */ +function dedupeKeys(keys: readonly string[]): string[] { + return [...new Set(keys)]; +} + /** * Effective key sequences for one command, given the user layer. Returns the * tinykeys tokens that should now trigger this command — the record default, - * replaced / augmented / removed per the override. + * replaced / augmented / removed per the override. The result is de-duplicated: + * an `add` override that re-adds a key already in the default (e.g. a preset + * that restates a binding) must not bind the command to the same key twice, nor + * make {@link detectConflicts} report the command as conflicting with itself. */ export function resolveKeys(cmd: Keybindable, keymap: UserKeymap): string[] { const override = keymap.overrides[cmd.id]; @@ -60,9 +68,9 @@ export function resolveKeys(cmd: Keybindable, keymap: UserKeymap): string[] { case 'remove': return []; case 'replace': - return [...override.keys]; + return dedupeKeys(override.keys); case 'add': - return [...base, ...override.keys]; + return dedupeKeys([...base, ...override.keys]); } } @@ -132,7 +140,18 @@ export function tokenFromEvent( if (event.metaKey || event.ctrlKey) mods.push('$mod'); // portable primary modifier if (event.shiftKey) mods.push('Shift'); if (event.altKey) mods.push('Alt'); - const base = options.mode === 'code' ? event.code : k.length === 1 ? k.toLowerCase() : k; + // The spacebar's `event.key` is a literal space; tinykeys parses a binding + // string by splitting on spaces (the chord separator), which would strip a + // space token to an empty, never-matching key. Emit the code-name 'Space', + // which tinykeys matches via `event.code` — so the captured shortcut fires. + const base = + options.mode === 'code' + ? event.code + : k === ' ' + ? 'Space' + : k.length === 1 + ? k.toLowerCase() + : k; return [...mods, base].join('+'); } diff --git a/packages/hotkeys/src/react.ts b/packages/hotkeys/src/react.ts index 39bb5b1..6259178 100644 --- a/packages/hotkeys/src/react.ts +++ b/packages/hotkeys/src/react.ts @@ -33,16 +33,43 @@ export function useHotkeys( const ctxRef = useRef(options.context ?? {}); ctxRef.current = options.context ?? {}; + // Route callbacks through refs so the LATEST closure runs at fire time + // without rebinding tinykeys. A fresh inline `onDispatched` / + // `shouldIgnoreEvent` each render (the common case) would otherwise freeze + // at first bind and run with stale captured state (e.g. a stale selection). + const onDispatchedRef = useRef(options.onDispatched); + onDispatchedRef.current = options.onDispatched; + const shouldIgnoreRef = useRef(options.shouldIgnoreEvent); + shouldIgnoreRef.current = options.shouldIgnoreEvent; + const enabled = options.enabled ?? true; - const { context: _ctx, enabled: _en, ...rest } = options; + const { + context: _ctx, + enabled: _en, + onDispatched: _od, + shouldIgnoreEvent: _sie, + ...rest + } = options; void _ctx; void _en; + void _od; + void _sie; useEffect(() => { if (!enabled) return; const stop = bindHotkeys(registry, { ...rest, contextProvider: () => ctxRef.current, + // Always-installed indirection: harmless when no onDispatched is set + // (the optional-call no-ops), and picks up the latest closure otherwise. + onDispatched: (cmd, result) => onDispatchedRef.current?.(cmd, result), + // Only override the binder's DEFAULT_IGNORE when the caller supplied a + // predicate at bind time; the ref keeps it current across renders. + // (Toggling its presence on/off mid-flight, like `target`/`tiers`, + // requires a remount.) + ...(shouldIgnoreRef.current + ? { shouldIgnoreEvent: (e: KeyboardEvent) => shouldIgnoreRef.current!(e) } + : {}), }); return stop; // Re-bind on `keymap` identity change so a live end-user remap UI takes diff --git a/packages/mcp/src/resources.test.ts b/packages/mcp/src/resources.test.ts index d10e700..527661b 100644 --- a/packages/mcp/src/resources.test.ts +++ b/packages/mcp/src/resources.test.ts @@ -167,3 +167,35 @@ describe('callGetState', () => { expect(JSON.parse(res.content[0]!.text).code).toBe('invalid_params'); }); }); + +describe('non-serializable view values — errors-as-data, never thrown', () => { + const bigintViews: ViewSource = { + list: () => [{ id: 'app.big', tier: 'stable' }], + read: (id) => (id === 'app.big' ? { frameId: 9007199254740993n } : undefined), + }; + const circularViews: ViewSource = { + list: () => [{ id: 'app.graph', tier: 'stable' }], + read: () => { + const node: Record = { name: 'root' }; + node.self = node; // circular back-reference (common for graph/tree state) + return node; + }, + }; + + it('callGetState returns isError (not a thrown TypeError) on a BigInt value', () => { + const res = callGetState(bigintViews, { view: 'app.big' }); + expect(res.isError).toBe(true); + expect(JSON.parse(res.content[0]!.text).code).toBe('unserializable_state'); + }); + + it('callGetState returns isError on a circular value', () => { + const res = callGetState(circularViews, { view: 'app.graph' }); + expect(res.isError).toBe(true); + expect(JSON.parse(res.content[0]!.text).code).toBe('unserializable_state'); + }); + + it('readResource surfaces the unserializable payload as the body (no error channel, never throws)', () => { + const contents = readResource(bigintViews, 'app://state/app.big'); + expect(JSON.parse(contents.contents[0]!.text).code).toBe('unserializable_state'); + }); +}); diff --git a/packages/mcp/src/resources.ts b/packages/mcp/src/resources.ts index f4a1d3b..932d50a 100644 --- a/packages/mcp/src/resources.ts +++ b/packages/mcp/src/resources.ts @@ -15,6 +15,7 @@ import type { Tier } from 'acture'; import type { McpToolDescriptor } from './tools.js'; +import { safeStringify } from './serialize.js'; /** A view descriptor as listed by a {@link ViewSource} — the read-side dual * of an MCP tool descriptor. The selector and state live in the app's @@ -93,6 +94,9 @@ export interface ResourceContents { * Read one view's current value as MCP resource contents. The value is * JSON-serialized; an unknown / filtered / `secret` view (where * `views.read` returns `undefined`) reads as `null` — no leak, no throw. + * A non-JSON-serializable view value (BigInt / circular / throwing `toJSON`) + * reads as an `unserializable_state` payload rather than throwing — the read + * side has no error channel, so the failure surfaces in the resource body. */ export function readResource( views: ViewSource, @@ -107,7 +111,7 @@ export function readResource( { uri, mimeType: 'application/json', - text: JSON.stringify(value ?? null, null, 2), + text: safeStringify(value).text, }, ], }; @@ -187,7 +191,11 @@ export interface GetStateResponse { * Execute a getState call — read the requested view and return its JSON value * as MCP tool-result content. Errors are **data** (never thrown): a missing / * non-string `view` returns `isError: true`; an unknown / internal / secret - * view reads as `null` (the `ViewSource` enforces that, per {@link readResource}). + * view reads as `null` (the `ViewSource` enforces that, per {@link readResource}); + * a non-JSON-serializable view value (BigInt / circular / throwing `toJSON`) + * also returns `isError: true` with an `unserializable_state` payload — the + * serialization is guarded so the errors-as-data boundary is never broken by a + * thrown `JSON.stringify`. */ export function callGetState(views: ViewSource, args: unknown): GetStateResponse { const view = (args as { view?: unknown } | null | undefined)?.view; @@ -205,8 +213,8 @@ export function callGetState(views: ViewSource, args: unknown): GetStateResponse isError: true, }; } - const value = views.read(view); - return { - content: [{ type: 'text', text: JSON.stringify(value ?? null, null, 2) }], - }; + const serialized = safeStringify(views.read(view)); + return serialized.error + ? { content: [{ type: 'text', text: serialized.text }], isError: true } + : { content: [{ type: 'text', text: serialized.text }] }; } diff --git a/packages/mcp/src/serialize.ts b/packages/mcp/src/serialize.ts new file mode 100644 index 0000000..18a42e9 --- /dev/null +++ b/packages/mcp/src/serialize.ts @@ -0,0 +1,56 @@ +/** + * Wire-safe JSON serialization for MCP content. + * + * Both the write side (tool results, `tools.ts`) and the read side + * (resource contents + the getState tool, `resources.ts`) serialize + * **app-supplied** values (typed `unknown`) whose JSON-serializability + * acture cannot guarantee. A raw `JSON.stringify` has two failure modes + * that break the errors-as-data boundary these modules promise: + * + * 1. It **throws** on a `BigInt`, a circular reference, or a value with a + * throwing `toJSON` — the exception then escapes the pure function and, + * with no try/catch at the SDK handler, the model receives a JSON-RPC + * protocol error instead of an `isError` tool result. + * 2. It **returns `undefined`** (not a string) for `undefined` and for + * function values — so `content: [{ type: 'text', text: undefined }]` + * is emitted, a malformed MCP content field (its `text` is required to + * be a string; the property drops on the wire). + * + * {@link safeStringify} closes both holes: it never throws and always + * returns a string. Callers decide whether a serialization failure should + * surface as `isError` (the write/getState side) or simply as the resource + * body (the read side, which has no error channel). + */ + +/** Outcome of {@link safeStringify}: the JSON text (always a string) plus + * whether serialization failed and `text` is the fallback error payload. */ +export interface SafeStringifyResult { + text: string; + error: boolean; +} + +/** + * `JSON.stringify(value, null, 2)` that never throws and always yields a + * string. `undefined` / function values (which `JSON.stringify` renders as + * `undefined`) serialize as `"null"`. On a serialization failure — a + * `BigInt`, a circular reference, or a throwing `toJSON` — returns a + * structured `unserializable_state` payload with `error: true`, so the + * caller can keep its errors-as-data boundary intact. + */ +export function safeStringify(value: unknown): SafeStringifyResult { + try { + const text = JSON.stringify(value, null, 2); + // JSON.stringify(undefined) === undefined; a function serializes the same + // way. Coerce to the JSON null literal so `text` is always a string. + return { text: text === undefined ? 'null' : text, error: false }; + } catch (e) { + const message = e instanceof Error ? e.message : String(e); + return { + text: JSON.stringify({ + code: 'unserializable_state', + message: `value is not JSON-serializable: ${message}`, + }), + error: true, + }; + } +} diff --git a/packages/mcp/src/server.test.ts b/packages/mcp/src/server.test.ts new file mode 100644 index 0000000..29b0e5e --- /dev/null +++ b/packages/mcp/src/server.test.ts @@ -0,0 +1,71 @@ +import { describe, it, expect } from 'vitest'; +import { createRegistry, defineCommand, ok } from 'acture'; +import { createMcpServer } from './server.js'; +import type { ViewSource } from './resources.js'; + +function makeViews(): ViewSource { + return { + list: () => [{ id: 'app.selection', tier: 'stable' }], + read: (id) => (id === 'app.selection' ? ['n1'] : undefined), + }; +} + +describe('createMcpServer — getState/command tool-name collision guard', () => { + it('throws when a command sanitizes to the getState tool name (default)', () => { + const registry = createRegistry(); + // `app.getState` → sanitized wire name `app_getState` === DEFAULT_GET_STATE_TOOL_NAME. + registry.register( + defineCommand({ id: 'app.getState', title: 'Get', execute: () => ok(undefined) }), + ); + expect(() => + createMcpServer(registry, { + name: 't', + version: '0.0.0', + views: makeViews(), + getStateTool: true, + }), + ).toThrow(/collides with a command/i); + }); + + it('throws when a command collides with a custom getState tool name', () => { + const registry = createRegistry(); + // `app.readState` sanitizes (dot → underscore) to `app_readState`. + registry.register( + defineCommand({ id: 'app.readState', title: 'Read', execute: () => ok(undefined) }), + ); + expect(() => + createMcpServer(registry, { + name: 't', + version: '0.0.0', + views: makeViews(), + getStateTool: { name: 'app_readState' }, + }), + ).toThrow(/collides with a command/i); + }); + + it('does not throw when there is no collision', () => { + const registry = createRegistry(); + registry.register( + defineCommand({ id: 'app.search', title: 'Search', execute: () => ok(undefined) }), + ); + expect(() => + createMcpServer(registry, { + name: 't', + version: '0.0.0', + views: makeViews(), + getStateTool: true, + }), + ).not.toThrow(); + }); + + it('does not check for a collision when the getState tool is disabled', () => { + const registry = createRegistry(); + // A command named app.getState is harmless when the getState tool is off. + registry.register( + defineCommand({ id: 'app.getState', title: 'Get', execute: () => ok(undefined) }), + ); + expect(() => + createMcpServer(registry, { name: 't', version: '0.0.0', views: makeViews() }), + ).not.toThrow(); + }); +}); diff --git a/packages/mcp/src/server.ts b/packages/mcp/src/server.ts index 7a0b68b..b847da4 100644 --- a/packages/mcp/src/server.ts +++ b/packages/mcp/src/server.ts @@ -91,6 +91,24 @@ export function createMcpServer( ? getStateOpts.name ?? DEFAULT_GET_STATE_TOOL_NAME : null; + // Fail fast on a getState-tool ↔ command-tool name collision. Otherwise + // `tools/list` emits two tools with the same name (strict hosts — the + // Anthropic API — reject the ENTIRE array, so every tool call in the session + // fails), and `tools/call` for that name is unconditionally routed to + // getState below, silently shadowing the command. A dotted command id like + // `app.getState` sanitizes to `app_getState` — exactly the default name — so + // this is a realistic footgun, not a theoretical one. + if (getStateName !== null) { + const clash = buildToolsList(registry, listOptions).some((t) => t.name === getStateName); + if (clash) { + throw new Error( + `createMcpServer: getState tool name '${getStateName}' collides with a command's wire ` + + `tool name. MCP/Anthropic reject duplicate tool names, and the command would be shadowed ` + + `on tools/call. Pass a distinct \`getStateTool: { name }\` or rename the command.`, + ); + } + } + server.setRequestHandler(ListToolsRequestSchema, async () => ({ tools: views && getStateOpts diff --git a/packages/mcp/src/tools.test.ts b/packages/mcp/src/tools.test.ts index 2249c66..3eed93a 100644 --- a/packages/mcp/src/tools.test.ts +++ b/packages/mcp/src/tools.test.ts @@ -150,4 +150,33 @@ describe('formatToolResponse', () => { expect(r.isError).toBe(true); expect(JSON.parse(r.content[0]!.text)).toMatchObject({ code: 'bad', message: 'failed' }); }); + + it('renders ok(undefined) as a well-formed "null" text (not a dropped text field)', () => { + const r = formatToolResponse(ok(undefined)); + expect(r.isError).toBeUndefined(); + // Regression: JSON.stringify(undefined) === undefined would drop `text` on + // the wire, failing the client's CallToolResult schema (text is required). + expect(typeof r.content[0]!.text).toBe('string'); + expect(r.content[0]!.text).toBe('null'); + expect(JSON.parse(r.content[0]!.text)).toBeNull(); + }); + + it('returns isError (not a thrown TypeError) for a non-serializable ok value', () => { + const circular: Record = {}; + circular.self = circular; + const r = formatToolResponse(ok(circular)); + expect(r.isError).toBe(true); + expect(JSON.parse(r.content[0]!.text).code).toBe('unserializable_state'); + }); + + it('preserves the error code/message when error.details is not serializable', () => { + const circular: Record = {}; + circular.self = circular; + const r = formatToolResponse(err('conflict', 'nope', circular)); + expect(r.isError).toBe(true); + const parsed = JSON.parse(r.content[0]!.text); + expect(parsed.code).toBe('conflict'); + expect(parsed.message).toBe('nope'); + expect(parsed.details).toBe('[unserializable]'); + }); }); diff --git a/packages/mcp/src/tools.ts b/packages/mcp/src/tools.ts index e9ace1f..5fd990b 100644 --- a/packages/mcp/src/tools.ts +++ b/packages/mcp/src/tools.ts @@ -17,6 +17,7 @@ import { isFunctionWhen, toJsonSchema, } from 'acture'; +import { safeStringify } from './serialize.js'; /** MCP tool envelope as understood by `tools/list`. The SDK's exact * type lives in `@modelcontextprotocol/sdk/types.js`; we mirror the @@ -153,20 +154,25 @@ function resolveDispatchId(registry: Registry, name: string): string { } /** Build an MCP response from an arbitrary acture Result. Exposed for - * hosts that want to dispatch directly and pre/post-process. */ + * hosts that want to dispatch directly and pre/post-process. Serialization + * is guarded ({@link safeStringify}): an `ok(undefined)` result yields a + * well-formed `"null"` text (not a dropped `text: undefined` field), and a + * non-JSON-serializable value or `error.details` surfaces as errors-as-data + * instead of throwing past the tool-call boundary. */ export function formatToolResponse(result: Result): CallToolResponse { if (result.ok) { - const payload = JSON.stringify(result.value, null, 2); - return { - content: [{ type: 'text', text: payload }], - _actureResult: result, - }; + const serialized = safeStringify(result.value); + return serialized.error + ? { content: [{ type: 'text', text: serialized.text }], isError: true, _actureResult: result } + : { content: [{ type: 'text', text: serialized.text }], _actureResult: result }; } - const errorPayload = JSON.stringify( - { code: result.error.code, message: result.error.message, details: result.error.details }, - null, - 2, - ); + const { code, message, details } = result.error; + // Preserve the error code/message even when `details` is not serializable — + // errors-as-data must never itself throw or lose the primary error. + const full = safeStringify({ code, message, details }); + const errorPayload = full.error + ? safeStringify({ code, message, details: '[unserializable]' }).text + : full.text; return { content: [{ type: 'text', text: errorPayload }], isError: true,