Merge pull request #1518 from corbitsdev/cl-8637-option-chords

fix(tui): fire composed option chords on every advertised surface

Author: TheGreatAxios

src/tui/command-surfaces.test.ts

@@ -24,8 +24,10 @@ import type { ThemeSetting } from "./theme-detect.js";
 import { focusOwner } from "./focus/index.js";
 import { BUNDLED_PLUGIN_MARKER as BUNDLED_MARKER } from "../plugins/origin-marker.js";
 import type { Harness } from "./harness";
+import type { AltChordLetter } from "./key-chords";
 import { projectPluginsRoot, userPluginsRoot } from "../plugins/uninstall.js";
 import type { AppShell } from "./shell/internals";
+import { shellInternals } from "./shell/internals";
 import { withAppShell } from "./test-helpers";
 import {
   acceptOverlaySelection,
@@ -585,6 +587,34 @@ describe("plugins surface admin actions", () => {
     });
   });
 
+  test("hybrid glyph events never reach the pane's bare s/v claims (I-4)", async () => {
+    await withShell(async (shell) => {
+      const { deps, calls } = pluginActionDeps({
+        credentialValues: { apiKey: "saved-key" },
+      });
+      openCommandSurface(shell, "plugins", deps);
+      expect(runOverlayAction(shell, altKey("c"))).toBe(true);
+      expect(shell.overlayKind).toBe("plugin_credentials");
+      // {name:"v", sequence:"√"} is the composed Option glyph arriving with
+      // no modifier bit — the input shape altChordLetterForKey reads via
+      // `sequence`. Read through `name` it used to fire save-and-verify.
+      expect(
+        runOverlayAction(shell, { ...charKey("√"), name: "v" } as KeyEvent),
+      ).toBe(false);
+      expect(
+        runOverlayAction(shell, { ...charKey("ß"), name: "s" } as KeyEvent),
+      ).toBe(false);
+      expect(calls.saveCredentials).toEqual([]);
+      expect(calls.verify).toEqual([]);
+      // A true bare key still claims: the guard is sequence-vs-name, not a
+      // ban on the letters themselves.
+      expect(runOverlayAction(shell, charKey("v"))).toBe(true);
+      await Promise.resolve();
+      expect(calls.saveCredentials.length).toBe(1);
+      expect(calls.verify.length).toBe(1);
+    });
+  });
+
   test("v verifies the currently saved credentials", async () => {
     await withShell(async (shell) => {
       const { deps, calls } = pluginActionDeps({
@@ -876,6 +906,212 @@ describe("plugins surface admin actions", () => {
   });
 });
 
+/**
+ * CL-8637: on stock macOS terminals Option+<letter> arrives as the composed
+ * layout glyph with no modifier bit. On the two surfaces that advertise
+ * Alt+<letter> chords, that glyph must run the advertised action and must
+ * never leak into the prompt buffer.
+ */
+describe("composed option glyphs on /plugins and /mcp (CL-8637)", () => {
+  test("every advertised plugins glyph runs its chord with the prompt left alone", async () => {
+    const glyphs = ["å", "∑", "ç", "√", "†", "≈"] as const;
+    for (const glyph of glyphs) {
+      await withShell(async (shell) => {
+        const { deps } = pluginActionDeps({ needsTrust: true });
+        openCommandSurface(shell, "plugins", deps);
+        shell.prompt.value = "";
+        expect(runOverlayAction(shell, charKey(glyph))).toBe(true);
+        expect(shell.prompt.value).toBe("");
+      });
+    }
+  });
+
+  test("å add-by-path and ∑ web chooser keep working through the glyph", async () => {
+    await withShell(async (shell) => {
+      const { deps, calls } = pluginActionDeps();
+      openCommandSurface(shell, "plugins", deps);
+      expect(runOverlayAction(shell, charKey("å"))).toBe(true);
+      typeOverlayText(shell, "/tmp/glyph-plugin");
+      acceptOverlaySelection(shell);
+      await Promise.resolve();
+      expect(calls.addPath).toEqual(["/tmp/glyph-plugin"]);
+    });
+    await withShell(async (shell) => {
+      const { deps, calls } = pluginActionDeps();
+      openCommandSurface(
+        shell,
+        "plugins",
+        patchPlugins(deps, { list: () => [] }),
+      );
+      expect(runOverlayAction(shell, charKey("∑"))).toBe(true);
+      moveOverlaySelection(shell, 1);
+      acceptOverlaySelection(shell);
+      await Promise.resolve();
+      expect(calls.setWebProvider).toEqual(["exa"]);
+    });
+  });
+
+  test("the trust glyph on a trusted plugin is still a no-op (I-7)", async () => {
+    await withShell(async (shell) => {
+      const { deps, calls } = pluginActionDeps();
+      openCommandSurface(shell, "plugins", deps);
+      expect(runOverlayAction(shell, charKey("†"))).toBe(false);
+      expect(calls.setEnabled).toEqual([]);
+    });
+  });
+
+  test("mcp glyphs stay text on /plugins: this surface declares no d or r", async () => {
+    await withShell(async (shell) => {
+      const { deps, calls } = pluginActionDeps();
+      openCommandSurface(shell, "plugins", deps);
+      expect(runOverlayAction(shell, charKey("∂"))).toBe(false);
+      expect(runOverlayAction(shell, charKey("®"))).toBe(false);
+      expect(calls.setEnabled).toEqual([]);
+      expect(calls.remove).toEqual([]);
+    });
+  });
+
+  test("å completes add-name → URL on /mcp with the prompt left alone", async () => {
+    await withShell(async (shell) => {
+      const added: { name: string; url: string }[] = [];
+      let liveEntries: readonly McpEntry[] = [
+        { name: "linear", state: "connected", toolCount: 12 },
+      ];
+      openMcp(shell, {
+        list: () => liveEntries,
+        openAuthURL: () => undefined,
+        addServer: async (name, url) => {
+          added.push({ name, url });
+          liveEntries = [...liveEntries, { name, state: "connecting" }];
+          return { ok: true, message: `Added ${name}.` };
+        },
+      });
+
+      expect(runOverlayAction(shell, charKey("å"))).toBe(true);
+      expect(shell.prompt.value).toBe("");
+      typeOverlayText(shell, "notion");
+      acceptOverlaySelection(shell);
+      typeOverlayText(shell, "https://mcp.notion.test/mcp");
+      acceptOverlaySelection(shell);
+      await flushSurface();
+
+      expect(added).toEqual([
+        { name: "notion", url: "https://mcp.notion.test/mcp" },
+      ]);
+      expect(shell.prompt.value).toBe("");
+    });
+  });
+
+  test("∂ disables the focused server on /mcp with the prompt left alone", async () => {
+    await withShell(async (shell) => {
+      const toggled: { name: string; enabled: boolean }[] = [];
+      const notes: string[] = [];
+      let liveEntries: readonly McpEntry[] = [
+        { name: "linear", state: "connected", toolCount: 12 },
+      ];
+      openMcp(
+        shell,
+        {
+          list: () => liveEntries,
+          openAuthURL: () => undefined,
+          setEnabled: async (name, enabled) => {
+            toggled.push({ name, enabled });
+            liveEntries = [
+              { name, state: enabled ? "connecting" : "disabled" },
+            ];
+            return { ok: true, message: `Disabled ${name}.` };
+          },
+        },
+        (note) => notes.push(note),
+      );
+      expect(shell.prompt.value).toBe("");
+      expect(runOverlayAction(shell, charKey("∂"))).toBe(true);
+      await flushSurface();
+      expect(toggled).toEqual([{ name: "linear", enabled: false }]);
+      expect(shell.prompt.value).toBe("");
+      expect(notes).toEqual(["Disabled linear."]);
+    });
+  });
+
+  test("® on /mcp opens the remove confirm and Enter completes it", async () => {
+    await withShell(async (shell) => {
+      let liveEntries: readonly McpEntry[] = [
+        { name: "linear", state: "connected", toolCount: 12 },
+        { name: "notion", state: "connected", toolCount: 3 },
+      ];
+      openMcp(shell, {
+        list: () => liveEntries,
+        openAuthURL: () => undefined,
+        removeServer: async (name) => {
+          liveEntries = liveEntries.filter((entry) => entry.name !== name);
+          return { ok: true, message: `Removed ${name}.` };
+        },
+      });
+      moveOverlaySelection(shell, 1);
+      expect(runOverlayAction(shell, charKey("®"))).toBe(true);
+      acceptOverlaySelection(shell);
+      await flushSurface();
+
+      expect(liveEntries.map((entry) => entry.name)).toEqual(["linear"]);
+      expect(shell.prompt.value).toBe("");
+    });
+  });
+
+  test("å is not a chord on /mcp while local settings shadow global (I-3)", async () => {
+    await withShell(async (shell) => {
+      openMcp(shell, {
+        list: () => [{ name: "linear", state: "connected", toolCount: 1 }],
+        openAuthURL: () => undefined,
+        mcpServersSource: "local",
+        addServer: async () => ({ ok: true, message: "should not run" }),
+      });
+      expect(runOverlayAction(shell, charKey("å"))).toBe(false);
+      expect(shell.prompt.value).toBe("");
+    });
+  });
+
+  test("ç is not a chord on /mcp: this surface declares no c", async () => {
+    await withShell(async (shell) => {
+      openMcp(shell, {
+        list: () => [{ name: "linear", state: "connected", toolCount: 1 }],
+        openAuthURL: () => undefined,
+      });
+      expect(runOverlayAction(shell, charKey("ç"))).toBe(false);
+    });
+  });
+
+  test("rendered footers only advertise declared letters, and never Alt+F (I-3)", async () => {
+    const advertised = (frame: string): string[] =>
+      [...frame.matchAll(/Alt\+([A-Za-z])/g)].map((m) =>
+        (m[1] ?? "").toLowerCase(),
+      );
+    await withWiredShell(async (shell, harness) => {
+      const { deps } = pluginActionDeps();
+      openCommandSurface(shell, "plugins", deps);
+      await harness.renderOnce();
+      const declared = shellInternals(shell)?.primaryBindings.altChords ?? [];
+      const letters = advertised(harness.captureCharFrame());
+      expect(letters.length).toBeGreaterThan(0);
+      for (const letter of letters) {
+        expect(declared.includes(letter as AltChordLetter)).toBe(true);
+      }
+    });
+    await withWiredShell(async (shell, harness) => {
+      openMcp(shell, {
+        list: () => [{ name: "linear", state: "connected", toolCount: 1 }],
+        openAuthURL: () => undefined,
+      });
+      await harness.renderOnce();
+      const declared = shellInternals(shell)?.primaryBindings.altChords ?? [];
+      const letters = advertised(harness.captureCharFrame());
+      expect(letters.length).toBeGreaterThan(0);
+      for (const letter of letters) {
+        expect(declared.includes(letter as AltChordLetter)).toBe(true);
+      }
+    });
+  });
+});
+
 describe("hooks surface", () => {
   test("lists discovered hooks with enabled state and Enter toggles", async () => {
     await withShell(async (shell) => {

src/tui/command-surfaces.ts

@@ -843,6 +843,13 @@ function openCredentialsPane(
       onAction: (_id, key) => {
         if (state.editing === null) {
           if (key.ctrl || key.meta || key.option) return false;
+          // Bare claims fire only for a true bare key: a hybrid
+          // {name:"v", sequence:"√"} is the composed Option glyph arriving
+          // with no modifier bit — exactly the input altChordLetterForKey
+          // reads through `sequence` — and must not reach save/verify via
+          // the name table. The text sinks below key on `sequence`, which
+          // is why they need no such guard (I-4).
+          if (key.sequence !== key.name) return false;
           if (key.name === "s") {
             void Promise.resolve(
               plugins.saveCredentials(entry.id, state.values),
@@ -1099,6 +1106,7 @@ export function openPluginsSurface(
       title: "plugins",
       frameId: "overlay-plugins",
       deferIfBusy: true,
+      altChords: ["a", "w", "c", "v", "t", "x"] as const,
       ...payload(rows),
       describe: (id) => {
         if (id === PLUGIN_LOAD_WARNINGS_ID) {
@@ -1140,26 +1148,21 @@ export function openPluginsSurface(
             deps.notify(`Plugin update failed: ${errorText(err)}`),
         );
       },
-      onAction: (id, key) => {
-        // Alt+<key>, never bare — bare c/v/t/a/w/x would be swallowed by the
-        // filter list. Alt+C is safe here: this branch returns before
-        // shell.ts's top-level onKey reaches the global copy-mode chord.
-        if (key.ctrl || !(key.meta || key.option)) return false;
-        const name = typeof key.name === "string" ? key.name.toLowerCase() : "";
-        // Surface-level chords — advertised in the how-to even when focus is on
-        // Close, the empty-list row, or load warnings.
-        if (name === "a") {
+      onAltChord: (id, letter) => {
+        // Surface-level chords run even when focus is on Close, the empty
+        // list, or load warnings.
+        if (letter === "a") {
           openAddPathPane(shell, deps, plugins);
           return true;
         }
-        if (name === "w") {
+        if (letter === "w") {
           openWebProviderChooser(shell, deps, plugins);
           return true;
         }
         if (id === PLUGIN_LOAD_WARNINGS_ID) return false;
         const target = byId.get(id);
         if (target === undefined) return false;
-        switch (name) {
+        switch (letter) {
           case "c":
             if (target.credentials.length === 0) return false;
             openCredentialsPane(shell, deps, plugins, target, {
@@ -1516,8 +1519,8 @@ export function openMcpSurface(
       // The flash below reports the outcome; the echo would quote the row's
       // pre-authorization label back at the operator forever.
       echoChoice: false,
-      mcpManageHint: true,
-      mcpAddHint: canAdd,
+      // `a` joins only when adding is possible.
+      altChords: canAdd ? (["a", "d", "r"] as const) : (["d", "r"] as const),
       ...payload(rows),
       describe: (id) => {
         const target = byName.get(id);
@@ -1596,17 +1599,14 @@ export function openMcpSurface(
         });
         closeReplaceableOverlay(shell);
       },
-      onAction: (id, key) => {
-        if (key.ctrl || !(key.meta || key.option)) return false;
-        const name = typeof key.name === "string" ? key.name.toLowerCase() : "";
-        if (name === "a") {
-          if (!canAdd) return false;
+      onAltChord: (id, letter) => {
+        if (letter === "a") {
           unsubscribe();
           openAddMcpNamePane(shell, deps, mcp);
           return true;
         }
         if (isMcpChromeRow(id)) return false;
-        if (name === "d") {
+        if (letter === "d") {
           const setEnabled = mcp.setEnabled;
           if (setEnabled === undefined) return false;
           unsubscribe();
@@ -1619,7 +1619,7 @@ export function openMcpSurface(
           );
           return true;
         }
-        if (name === "r") {
+        if (letter === "r") {
           const target = byName.get(id);
           if (target?.builtin === true) {
             deps.notify(