From 54c33f2adef3a98a603a900964e15e5d2112d8e4 Mon Sep 17 00:00:00 2001 From: Richard Gill Date: Sun, 18 Jan 2026 13:01:12 +0000 Subject: [PATCH] Respect reserved keybindings when registering extensions --- .../src/core/extensions/runner.ts | 80 +++++++---- .../src/modes/interactive/interactive-mode.ts | 4 +- .../test/extensions-runner.test.ts | 124 +++++++++++++++++- 3 files changed, 179 insertions(+), 29 deletions(-) diff --git a/packages/coding-agent/src/core/extensions/runner.ts b/packages/coding-agent/src/core/extensions/runner.ts index 4ce99b14..1facfb21 100644 --- a/packages/coding-agent/src/core/extensions/runner.ts +++ b/packages/coding-agent/src/core/extensions/runner.ts @@ -6,6 +6,7 @@ import type { AgentMessage } from "@mariozechner/pi-agent-core"; import type { ImageContent, Model } from "@mariozechner/pi-ai"; import type { KeyId } from "@mariozechner/pi-tui"; import { type Theme, theme } from "../../modes/interactive/theme/theme.js"; +import type { KeyAction, KeybindingsConfig } from "../keybindings.js"; import type { ModelRegistry } from "../model-registry.js"; import type { SessionManager } from "../session-manager.js"; import type { @@ -42,6 +43,44 @@ import type { UserBashEventResult, } from "./types.js"; +// These keybindings cannot be overridden by extensions +const RESERVED_ACTIONS_FOR_EXTENSION_CONFLICTS: ReadonlyArray = [ + "interrupt", + "clear", + "exit", + "suspend", + "cycleThinkingLevel", + "cycleModelForward", + "cycleModelBackward", + "selectModel", + "expandTools", + "toggleThinking", + "externalEditor", + "followUp", + "submit", + "selectConfirm", + "selectCancel", + "deleteToLineEnd", +]; + +type BuiltInKeyBindings = Partial>; + +const buildBuiltinKeybindings = (effectiveKeybindings: Required): BuiltInKeyBindings => { + const builtinKeybindings = {} as BuiltInKeyBindings; + for (const [action, keys] of Object.entries(effectiveKeybindings)) { + const keyAction = action as KeyAction; + const keyList = Array.isArray(keys) ? keys : [keys]; + const restrictOverride = RESERVED_ACTIONS_FOR_EXTENSION_CONFLICTS.includes(keyAction); + for (const key of keyList) { + builtinKeybindings[key] = { + action: keyAction, + restrictOverride: restrictOverride, + }; + } + } + return builtinKeybindings; +}; + /** Combined result from all before_agent_start handlers */ interface BeforeAgentStartCombinedResult { messages?: NonNullable[]; @@ -224,46 +263,37 @@ export class ExtensionRunner { this.runtime.flagValues.set(name, value); } - private static readonly RESERVED_SHORTCUTS = new Set([ - "ctrl+c", - "ctrl+d", - "ctrl+z", - "ctrl+k", - "ctrl+p", - "ctrl+l", - "ctrl+o", - "ctrl+t", - "ctrl+g", - "shift+tab", - "shift+ctrl+p", - "alt+enter", - "escape", - "enter", - ]); - - getShortcuts(): Map { - const allShortcuts = new Map(); + getShortcuts(effectiveKeybindings: Required): Map { + const builtinKeybindings = buildBuiltinKeybindings(effectiveKeybindings); + const extensionShortcuts = new Map(); for (const ext of this.extensions) { for (const [key, shortcut] of ext.shortcuts) { const normalizedKey = key.toLowerCase() as KeyId; - if (ExtensionRunner.RESERVED_SHORTCUTS.has(normalizedKey)) { + const builtInKeybinding = builtinKeybindings[normalizedKey]; + if (builtInKeybinding?.restrictOverride === true) { console.warn( `Extension shortcut '${key}' from ${shortcut.extensionPath} conflicts with built-in shortcut. Skipping.`, ); continue; } - const existing = allShortcuts.get(normalizedKey); - if (existing) { + if (builtInKeybinding?.restrictOverride === false) { console.warn( - `Extension shortcut conflict: '${key}' registered by both ${existing.extensionPath} and ${shortcut.extensionPath}. Using ${shortcut.extensionPath}.`, + `Extension shortcut conflict: '${key}' is built-in shortcut for ${builtInKeybinding.action} and ${shortcut.extensionPath}. Using ${shortcut.extensionPath}.`, ); } - allShortcuts.set(normalizedKey, shortcut); + + const existingExtensionShortcut = extensionShortcuts.get(normalizedKey); + if (existingExtensionShortcut) { + console.warn( + `Extension shortcut conflict: '${key}' registered by both ${existingExtensionShortcut.extensionPath} and ${shortcut.extensionPath}. Using ${shortcut.extensionPath}.`, + ); + } + extensionShortcuts.set(normalizedKey, shortcut); } } - return allShortcuts; + return extensionShortcuts; } onError(listener: ExtensionErrorListener): () => void { diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index 5d923fa0..8cacc614 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -814,7 +814,7 @@ export class InteractiveMode { * Set up keyboard shortcuts registered by extensions. */ private setupExtensionShortcuts(extensionRunner: ExtensionRunner): void { - const shortcuts = extensionRunner.getShortcuts(); + const shortcuts = extensionRunner.getShortcuts(this.keybindings.getEffectiveConfig()); if (shortcuts.size === 0) return; // Create a context for shortcut handlers @@ -3505,7 +3505,7 @@ export class InteractiveMode { // Add extension-registered shortcuts const extensionRunner = this.session.extensionRunner; if (extensionRunner) { - const shortcuts = extensionRunner.getShortcuts(); + const shortcuts = extensionRunner.getShortcuts(this.keybindings.getEffectiveConfig()); if (shortcuts.size > 0) { hotkeys += ` **Extensions** diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index daa6cc98..6e139ff1 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/packages/coding-agent/test/extensions-runner.test.ts @@ -9,6 +9,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { AuthStorage } from "../src/core/auth-storage.js"; import { discoverAndLoadExtensions } from "../src/core/extensions/loader.js"; import { ExtensionRunner } from "../src/core/extensions/runner.js"; +import { DEFAULT_KEYBINDINGS, type KeyId } from "../src/core/keybindings.js"; import { ModelRegistry } from "../src/core/model-registry.js"; import { SessionManager } from "../src/core/session-manager.js"; @@ -47,7 +48,7 @@ describe("ExtensionRunner", () => { const result = await discoverAndLoadExtensions([], tempDir, tempDir); const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); - const shortcuts = runner.getShortcuts(); + const shortcuts = runner.getShortcuts(DEFAULT_KEYBINDINGS); expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("conflicts with built-in")); expect(shortcuts.has("ctrl+c")).toBe(false); @@ -55,6 +56,125 @@ describe("ExtensionRunner", () => { warnSpy.mockRestore(); }); + it("allows a shortcut when the reserved set no longer contains the default key", async () => { + const extCode = ` + export default function(pi) { + pi.registerShortcut("ctrl+p", { + description: "Uses freed default", + handler: async () => {}, + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "rebinding.ts"), extCode); + + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const result = await discoverAndLoadExtensions([], tempDir, tempDir); + const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); + const keybindings = { ...DEFAULT_KEYBINDINGS, cycleModelForward: "ctrl+n" as KeyId }; + const shortcuts = runner.getShortcuts(keybindings); + + expect(shortcuts.has("ctrl+p")).toBe(true); + expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining("conflicts with built-in")); + + warnSpy.mockRestore(); + }); + + it("warns but allows when extension uses non-reserved built-in shortcut", async () => { + const extCode = ` + export default function(pi) { + pi.registerShortcut("ctrl+v", { + description: "Overrides non-reserved", + handler: async () => {}, + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "non-reserved.ts"), extCode); + + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const result = await discoverAndLoadExtensions([], tempDir, tempDir); + const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); + const shortcuts = runner.getShortcuts(DEFAULT_KEYBINDINGS); + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("built-in shortcut for pasteImage")); + expect(shortcuts.has("ctrl+v")).toBe(true); + + warnSpy.mockRestore(); + }); + + it("blocks shortcuts for reserved actions even when rebound", async () => { + const extCode = ` + export default function(pi) { + pi.registerShortcut("ctrl+x", { + description: "Conflicts with rebound reserved", + handler: async () => {}, + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "rebound-reserved.ts"), extCode); + + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const result = await discoverAndLoadExtensions([], tempDir, tempDir); + const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); + const keybindings = { ...DEFAULT_KEYBINDINGS, interrupt: "ctrl+x" as KeyId }; + const shortcuts = runner.getShortcuts(keybindings); + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("conflicts with built-in")); + expect(shortcuts.has("ctrl+x")).toBe(false); + + warnSpy.mockRestore(); + }); + + it("blocks shortcuts when reserved action has multiple keys", async () => { + const extCode = ` + export default function(pi) { + pi.registerShortcut("ctrl+y", { + description: "Conflicts with multi-key reserved", + handler: async () => {}, + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "multi-reserved.ts"), extCode); + + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const result = await discoverAndLoadExtensions([], tempDir, tempDir); + const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); + const keybindings = { ...DEFAULT_KEYBINDINGS, clear: ["ctrl+x", "ctrl+y"] as KeyId[] }; + const shortcuts = runner.getShortcuts(keybindings); + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("conflicts with built-in")); + expect(shortcuts.has("ctrl+y")).toBe(false); + + warnSpy.mockRestore(); + }); + + it("warns but allows when non-reserved action has multiple keys", async () => { + const extCode = ` + export default function(pi) { + pi.registerShortcut("ctrl+y", { + description: "Overrides multi-key non-reserved", + handler: async () => {}, + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "multi-non-reserved.ts"), extCode); + + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const result = await discoverAndLoadExtensions([], tempDir, tempDir); + const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); + const keybindings = { ...DEFAULT_KEYBINDINGS, pasteImage: ["ctrl+x", "ctrl+y"] as KeyId[] }; + const shortcuts = runner.getShortcuts(keybindings); + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("built-in shortcut for pasteImage")); + expect(shortcuts.has("ctrl+y")).toBe(true); + + warnSpy.mockRestore(); + }); + it("warns when two extensions register same shortcut", async () => { // Use a non-reserved shortcut const extCode1 = ` @@ -80,7 +200,7 @@ describe("ExtensionRunner", () => { const result = await discoverAndLoadExtensions([], tempDir, tempDir); const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); - const shortcuts = runner.getShortcuts(); + const shortcuts = runner.getShortcuts(DEFAULT_KEYBINDINGS); expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("shortcut conflict")); // Last one wins