fix(mcp,hotkeys): harden v1.14–v1.18 surface before reelee-web wiring

.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 `<input>` 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.

.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.

.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
 

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 <input> — 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 });

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 `<input>` 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)

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', () => {

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('+');
 }
 

packages/hotkeys/src/react.ts

@@ -33,16 +33,43 @@ export function useHotkeys(
   const ctxRef = useRef<Context>(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

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<string, unknown> = { 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');
+  });
+});

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 }] };
 }