From a8a58ff26b2efc1986a71257097c04e0acad9326 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Mon, 23 Mar 2026 02:33:52 +0100 Subject: [PATCH] fix(coding-agent): disambiguate duplicate slash commands, fixes #1061 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/docs/extensions.md | 4 +- .../coding-agent/src/core/agent-session.ts | 2 +- .../coding-agent/src/core/extensions/index.ts | 1 + .../src/core/extensions/runner.ts | 55 ++++++++++++------- .../coding-agent/src/core/extensions/types.ts | 4 ++ .../coding-agent/src/core/resource-loader.ts | 16 +----- packages/coding-agent/src/index.ts | 1 + .../src/modes/interactive/interactive-mode.ts | 7 ++- .../coding-agent/src/modes/rpc/rpc-mode.ts | 2 +- .../test/extensions-runner.test.ts | 24 ++++---- .../coding-agent/test/resource-loader.test.ts | 17 ++++-- .../coding-agent/test/test-harness.test.ts | 54 +++++++++++++++++- packages/coding-agent/test/test-harness.ts | 38 ++++++++++++- packages/coding-agent/test/utilities.ts | 45 ++++++++++++++- 15 files changed, 205 insertions(+), 66 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 80cd0b89..c3b9dfa3 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -35,6 +35,7 @@ Examples: ### Fixed +- Fixed extension command name conflicts so extensions with duplicate command names can load together. Conflicting extension commands now get numeric invocation suffixes in load order, for example `/review:1` and `/review:2` ([#1061](https://github.com/badlogic/pi-mono/issues/1061)) - Fixed slash command source attribution for extension commands, prompt templates, and skills in autocomplete and command discovery ([#1734](https://github.com/badlogic/pi-mono/issues/1734)) - Fixed auto-resized image handling to enforce the inline image size limit on the final base64 payload, return text-only fallbacks when resizing cannot produce a safe image, and avoid falling back to the original image in `read` and `@file` auto-resize paths ([#2055](https://github.com/badlogic/pi-mono/issues/2055)) - Fixed `pi update` for git packages to skip destructive reset, clean, and reinstall steps when the fetched target already matches the local checkout ([#2503](https://github.com/badlogic/pi-mono/issues/2503)) diff --git a/packages/coding-agent/docs/extensions.md b/packages/coding-agent/docs/extensions.md index a693d763..8a847434 100644 --- a/packages/coding-agent/docs/extensions.md +++ b/packages/coding-agent/docs/extensions.md @@ -1089,6 +1089,8 @@ Labels persist in the session and survive restarts. Use them to mark important p Register a command. +If multiple extensions register the same command name, pi keeps them all and assigns numeric invocation suffixes in load order, for example `/review:1` and `/review:2`. + ```typescript pi.registerCommand("stats", { description: "Show session statistics", @@ -1133,7 +1135,7 @@ Each entry has this shape: ```typescript { - name: string; // Command name without the leading slash + name: string; // Invokable command name without the leading slash. May be suffixed like "review:1" description?: string; source: "extension" | "prompt" | "skill"; sourceInfo: { diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index c1c2d09b..31534b25 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -2093,7 +2093,7 @@ export class AgentSession { private _bindExtensionCore(runner: ExtensionRunner): void { const getCommands = (): SlashCommandInfo[] => { const extensionCommands: SlashCommandInfo[] = runner.getRegisteredCommands().map((command) => ({ - name: command.name, + name: command.invocationName, description: command.description, source: "extension", sourceInfo: command.sourceInfo, diff --git a/packages/coding-agent/src/core/extensions/index.ts b/packages/coding-agent/src/core/extensions/index.ts index e0826562..90e6ca2f 100644 --- a/packages/coding-agent/src/core/extensions/index.ts +++ b/packages/coding-agent/src/core/extensions/index.ts @@ -101,6 +101,7 @@ export type { // Commands RegisteredCommand, RegisteredTool, + ResolvedCommand, // Events - Resources ResourcesDiscoverEvent, ResourcesDiscoverResult, diff --git a/packages/coding-agent/src/core/extensions/runner.ts b/packages/coding-agent/src/core/extensions/runner.ts index 6e2ce988..14a91d9f 100644 --- a/packages/coding-agent/src/core/extensions/runner.ts +++ b/packages/coding-agent/src/core/extensions/runner.ts @@ -37,6 +37,7 @@ import type { ProviderConfig, RegisteredCommand, RegisteredTool, + ResolvedCommand, ResourcesDiscoverEvent, ResourcesDiscoverResult, SessionBeforeCompactResult, @@ -467,41 +468,53 @@ export class ExtensionRunner { return undefined; } - private collectRegisteredCommands(diagnostics?: ResourceDiagnostic[]): RegisteredCommand[] { + private resolveRegisteredCommands(): ResolvedCommand[] { const commands: RegisteredCommand[] = []; - const commandOwners = new Map(); + const counts = new Map(); + for (const ext of this.extensions) { for (const command of ext.commands.values()) { - const existingOwner = commandOwners.get(command.name); - if (existingOwner) { - const message = `Extension command '${command.name}' from ${ext.path} conflicts with ${existingOwner}. Skipping.`; - diagnostics?.push({ type: "warning", message, path: ext.path }); - if (diagnostics && !this.hasUI()) { - console.warn(message); - } - continue; - } - - commandOwners.set(command.name, ext.path); commands.push(command); + counts.set(command.name, (counts.get(command.name) ?? 0) + 1); } } - return commands; + + const seen = new Map(); + const takenInvocationNames = new Set(); + + return commands.map((command) => { + const occurrence = (seen.get(command.name) ?? 0) + 1; + seen.set(command.name, occurrence); + + let invocationName = (counts.get(command.name) ?? 0) > 1 ? `${command.name}:${occurrence}` : command.name; + + if (takenInvocationNames.has(invocationName)) { + let suffix = occurrence; + do { + suffix++; + invocationName = `${command.name}:${suffix}`; + } while (takenInvocationNames.has(invocationName)); + } + + takenInvocationNames.add(invocationName); + return { + ...command, + invocationName, + }; + }); } - getRegisteredCommands(): RegisteredCommand[] { - const diagnostics: ResourceDiagnostic[] = []; - const commands = this.collectRegisteredCommands(diagnostics); - this.commandDiagnostics = diagnostics; - return commands; + getRegisteredCommands(): ResolvedCommand[] { + this.commandDiagnostics = []; + return this.resolveRegisteredCommands(); } getCommandDiagnostics(): ResourceDiagnostic[] { return this.commandDiagnostics; } - getCommand(name: string): RegisteredCommand | undefined { - return this.collectRegisteredCommands().find((command) => command.name === name); + getCommand(name: string): ResolvedCommand | undefined { + return this.resolveRegisteredCommands().find((command) => command.invocationName === name); } /** diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index 6ac089f5..d4a34f1a 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -967,6 +967,10 @@ export interface RegisteredCommand { handler: (args: string, ctx: ExtensionCommandContext) => Promise; } +export interface ResolvedCommand extends RegisteredCommand { + invocationName: string; +} + // ============================================================================ // Extension API // ============================================================================ diff --git a/packages/coding-agent/src/core/resource-loader.ts b/packages/coding-agent/src/core/resource-loader.ts index d781afd3..13d9f35b 100644 --- a/packages/coding-agent/src/core/resource-loader.ts +++ b/packages/coding-agent/src/core/resource-loader.ts @@ -849,9 +849,8 @@ export class DefaultResourceLoader implements ResourceLoader { private detectExtensionConflicts(extensions: Extension[]): Array<{ path: string; message: string }> { const conflicts: Array<{ path: string; message: string }> = []; - // Track which extension registered each tool, command, and flag + // Track which extension registered each tool and flag const toolOwners = new Map(); - const commandOwners = new Map(); const flagOwners = new Map(); for (const ext of extensions) { @@ -868,19 +867,6 @@ export class DefaultResourceLoader implements ResourceLoader { } } - // Check commands - for (const commandName of ext.commands.keys()) { - const existingOwner = commandOwners.get(commandName); - if (existingOwner && existingOwner !== ext.path) { - conflicts.push({ - path: ext.path, - message: `Command "/${commandName}" conflicts with ${existingOwner}`, - }); - } else { - commandOwners.set(commandName, ext.path); - } - } - // Check flags for (const flagName of ext.flags.keys()) { const existingOwner = flagOwners.get(flagName); diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index 8f64f0e9..50fb506b 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -97,6 +97,7 @@ export type { ReadToolCallEvent, RegisteredCommand, RegisteredTool, + ResolvedCommand, SessionBeforeCompactEvent, SessionBeforeForkEvent, SessionBeforeSwitchEvent, diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index 8071f733..32b20d55 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -332,7 +332,10 @@ export class InteractiveMode { .filter((command) => builtinNames.has(command.name)) .map((command) => ({ type: "warning" as const, - message: `Extension command '/${command.name}' conflicts with built-in interactive command. Skipping in autocomplete.`, + message: + command.invocationName === command.name + ? `Extension command '/${command.name}' conflicts with built-in interactive command. Skipping in autocomplete.` + : `Extension command '/${command.name}' conflicts with built-in interactive command. Available as '/${command.invocationName}'.`, path: command.sourceInfo.path, })); } @@ -386,7 +389,7 @@ export class InteractiveMode { const extensionCommands: SlashCommand[] = ( this.session.extensionRunner?.getRegisteredCommands().filter((cmd) => !builtinCommandNames.has(cmd.name)) ?? [] ).map((cmd) => ({ - name: cmd.name, + name: cmd.invocationName, description: this.prefixAutocompleteDescription(cmd.description, cmd.sourceInfo), getArgumentCompletions: cmd.getArgumentCompletions, })); diff --git a/packages/coding-agent/src/modes/rpc/rpc-mode.ts b/packages/coding-agent/src/modes/rpc/rpc-mode.ts index deadea8d..2529689c 100644 --- a/packages/coding-agent/src/modes/rpc/rpc-mode.ts +++ b/packages/coding-agent/src/modes/rpc/rpc-mode.ts @@ -546,7 +546,7 @@ export async function runRpcMode(session: AgentSession): Promise { for (const command of session.extensionRunner?.getRegisteredCommands() ?? []) { commands.push({ - name: command.name, + name: command.invocationName, description: command.description, source: "extension", sourceInfo: command.sourceInfo, diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index 3c36cc8b..3d18a78b 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/packages/coding-agent/test/extensions-runner.test.ts @@ -345,9 +345,10 @@ describe("ExtensionRunner", () => { expect(commands.length).toBe(2); expect(commands.map((c) => c.name).sort()).toEqual(["cmd-a", "cmd-b"]); + expect(commands.map((c) => c.invocationName).sort()).toEqual(["cmd-a", "cmd-b"]); }); - it("gets command by name", async () => { + it("gets command by invocation name", async () => { const cmdCode = ` export default function(pi) { pi.registerCommand("my-cmd", { @@ -364,13 +365,14 @@ describe("ExtensionRunner", () => { const cmd = runner.getCommand("my-cmd"); expect(cmd).toBeDefined(); expect(cmd?.name).toBe("my-cmd"); + expect(cmd?.invocationName).toBe("my-cmd"); expect(cmd?.description).toBe("My command"); const missing = runner.getCommand("not-exists"); expect(missing).toBeUndefined(); }); - it("filters out duplicate extension commands", async () => { + it("suffixes duplicate extension commands in insertion order", async () => { const cmdCode = (description: string) => ` export default function(pi) { pi.registerCommand("shared-cmd", { @@ -382,22 +384,18 @@ describe("ExtensionRunner", () => { fs.writeFileSync(path.join(extensionsDir, "cmd-a.ts"), cmdCode("First command")); fs.writeFileSync(path.join(extensionsDir, "cmd-b.ts"), cmdCode("Second command")); - 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 commands = runner.getRegisteredCommands(); const diagnostics = runner.getCommandDiagnostics(); - expect(commands).toHaveLength(1); - expect(commands[0]?.name).toBe("shared-cmd"); - expect(commands[0]?.description).toBe("First command"); - - expect(diagnostics).toHaveLength(1); - expect(diagnostics[0]?.path).toEqual(path.join(extensionsDir, "cmd-b.ts")); - - expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("conflicts with")); - warnSpy.mockRestore(); + expect(commands).toHaveLength(2); + expect(commands.map((command) => command.name)).toEqual(["shared-cmd", "shared-cmd"]); + expect(commands.map((command) => command.invocationName)).toEqual(["shared-cmd:1", "shared-cmd:2"]); + expect(commands.map((command) => command.description)).toEqual(["First command", "Second command"]); + expect(diagnostics).toEqual([]); + expect(runner.getCommand("shared-cmd:1")?.description).toBe("First command"); + expect(runner.getCommand("shared-cmd:2")?.description).toBe("Second command"); }); }); diff --git a/packages/coding-agent/test/resource-loader.test.ts b/packages/coding-agent/test/resource-loader.test.ts index 2e44f538..13678653 100644 --- a/packages/coding-agent/test/resource-loader.test.ts +++ b/packages/coding-agent/test/resource-loader.test.ts @@ -195,7 +195,7 @@ Project skill`, const extensionsResult = loader.getExtensions(); expect(extensionsResult.extensions).toHaveLength(2); - expect(extensionsResult.errors.some((e) => e.error.includes('Command "/deploy" conflicts'))).toBe(true); + expect(extensionsResult.errors.some((e) => e.error.includes('Command "/deploy" conflicts'))).toBe(false); const sessionManager = SessionManager.inMemory(); const authStorage = AuthStorage.create(join(tempDir, "auth.json")); @@ -208,12 +208,18 @@ Project skill`, modelRegistry, ); - expect(runner.getCommand("deploy")?.description).toBe("project deploy"); + expect(runner.getCommand("deploy:1")?.description).toBe("project deploy"); + expect(runner.getCommand("deploy:2")?.description).toBe("user deploy"); expect(runner.getCommand("project-only")?.description).toBe("project only"); expect(runner.getCommand("user-only")?.description).toBe("user only"); - const commandNames = runner.getRegisteredCommands().map((c) => c.name); - expect(commandNames.filter((name) => name === "deploy")).toHaveLength(1); + const commands = runner.getRegisteredCommands(); + expect(commands.map((command) => command.invocationName)).toEqual([ + "deploy:1", + "project-only", + "deploy:2", + "user-only", + ]); }); it("should honor overrides for auto-discovered resources", async () => { @@ -551,7 +557,8 @@ export default function(pi: ExtensionAPI) { modelRegistry, ); - expect(runner.getCommand("deploy")?.description).toBe("explicit command"); + expect(runner.getCommand("deploy:1")?.description).toBe("explicit command"); + expect(runner.getCommand("deploy:2")?.description).toBe("global command"); expect(runner.getToolDefinition("duplicate-tool")?.description).toBe("explicit tool"); }); }); diff --git a/packages/coding-agent/test/test-harness.test.ts b/packages/coding-agent/test/test-harness.test.ts index 0c7f19f2..71589194 100644 --- a/packages/coding-agent/test/test-harness.test.ts +++ b/packages/coding-agent/test/test-harness.test.ts @@ -7,7 +7,7 @@ import type { AgentTool } from "@mariozechner/pi-agent-core"; import type { AssistantMessage } from "@mariozechner/pi-ai"; import { Type } from "@sinclair/typebox"; import { afterEach, describe, expect, it } from "vitest"; -import { createHarness, type Harness } from "./test-harness.js"; +import { createHarness, createHarnessWithExtensions, type Harness } from "./test-harness.js"; describe("test harness", () => { let harness: Harness; @@ -257,6 +257,58 @@ describe("test harness", () => { expect(firstText).toBeLessThan(firstToolcall); }); + it("loads inline extension factories and disambiguates duplicate commands", async () => { + const calls: string[] = []; + + harness = await createHarnessWithExtensions({ + extensionFactories: [ + { + path: "", + factory: (pi) => { + pi.registerCommand("shared-cmd", { + description: "Alpha command", + handler: async (args) => { + calls.push(`alpha:${args}`); + }, + }); + }, + }, + { + path: "", + factory: (pi) => { + pi.registerCommand("shared-cmd", { + description: "Beta command", + handler: async (args) => { + calls.push(`beta:${args}`); + }, + }); + }, + }, + ], + }); + + const runner = harness.session.extensionRunner; + expect(runner).toBeDefined(); + + const commands = runner!.getRegisteredCommands(); + expect( + commands.map((command) => ({ + name: command.name, + invocationName: command.invocationName, + description: command.description, + path: command.sourceInfo.path, + })), + ).toEqual([ + { name: "shared-cmd", invocationName: "shared-cmd:1", description: "Alpha command", path: "" }, + { name: "shared-cmd", invocationName: "shared-cmd:2", description: "Beta command", path: "" }, + ]); + + await runner!.getCommand("shared-cmd:1")?.handler("first", runner!.createCommandContext()); + await runner!.getCommand("shared-cmd:2")?.handler("second", runner!.createCommandContext()); + + expect(calls).toEqual(["alpha:first", "beta:second"]); + }); + it("session persistence works", async () => { harness = createHarness({ responses: ["persisted"] }); diff --git a/packages/coding-agent/test/test-harness.ts b/packages/coding-agent/test/test-harness.ts index 0c428a8e..300ea2b5 100644 --- a/packages/coding-agent/test/test-harness.ts +++ b/packages/coding-agent/test/test-harness.ts @@ -32,7 +32,12 @@ import { ModelRegistry } from "../src/core/model-registry.js"; import { SessionManager } from "../src/core/session-manager.js"; import type { Settings } from "../src/core/settings-manager.js"; import { SettingsManager } from "../src/core/settings-manager.js"; -import { createTestResourceLoader } from "./utilities.js"; +import type { ExtensionFactory, ResourceLoader } from "../src/index.js"; +import { + type CreateTestExtensionsResultInput, + createTestExtensionsResult, + createTestResourceLoader, +} from "./utilities.js"; // ============================================================================ // Faux model @@ -327,6 +332,10 @@ export interface HarnessOptions { tools?: AgentTool[]; /** Base tools override (replaces built-in read/bash/edit/write). */ baseToolsOverride?: Record; + /** Optional resource loader override. */ + resourceLoader?: ResourceLoader; + /** Inline extensions to load into the session resource loader. */ + extensionFactories?: Array; } export interface Harness { @@ -346,10 +355,17 @@ export interface Harness { cleanup: () => void; } -export function createHarness(options: HarnessOptions = {}): Harness { +function createTempDir(): string { const tempDir = join(tmpdir(), `pi-harness-${Date.now()}-${Math.random().toString(36).slice(2)}`); mkdirSync(tempDir, { recursive: true }); + return tempDir; +} +function createHarnessWithResourceLoader( + options: HarnessOptions, + resourceLoader: ResourceLoader, + tempDir: string, +): Harness { const baseModel = options.model ?? fauxModel; const model: Model = options.contextWindow ? { ...baseModel, contextWindow: options.contextWindow } : baseModel; @@ -382,7 +398,7 @@ export function createHarness(options: HarnessOptions = {}): Harness { settingsManager, cwd: tempDir, modelRegistry, - resourceLoader: createTestResourceLoader(), + resourceLoader, baseToolsOverride: options.baseToolsOverride, }); @@ -412,3 +428,19 @@ export function createHarness(options: HarnessOptions = {}): Harness { cleanup, }; } + +export function createHarness(options: HarnessOptions = {}): Harness { + if (options.extensionFactories?.length) { + throw new Error("createHarness does not support extensionFactories. Use createHarnessWithExtensions()."); + } + + const tempDir = createTempDir(); + return createHarnessWithResourceLoader(options, options.resourceLoader ?? createTestResourceLoader(), tempDir); +} + +export async function createHarnessWithExtensions(options: HarnessOptions = {}): Promise { + const tempDir = createTempDir(); + const extensionsResult = await createTestExtensionsResult(options.extensionFactories ?? [], tempDir); + const resourceLoader = options.resourceLoader ?? createTestResourceLoader({ extensionsResult }); + return createHarnessWithResourceLoader(options, resourceLoader, tempDir); +} diff --git a/packages/coding-agent/test/utilities.ts b/packages/coding-agent/test/utilities.ts index fdefc26b..6a2f5d6a 100644 --- a/packages/coding-agent/test/utilities.ts +++ b/packages/coding-agent/test/utilities.ts @@ -10,7 +10,9 @@ import { getModel, type OAuthCredentials, type OAuthProvider } from "@mariozechn import { getOAuthApiKey } from "@mariozechner/pi-ai/oauth"; import { AgentSession } from "../src/core/agent-session.js"; import { AuthStorage } from "../src/core/auth-storage.js"; -import { createExtensionRuntime } from "../src/core/extensions/loader.js"; +import { createEventBus } from "../src/core/event-bus.js"; +import type { Extension, ExtensionFactory, LoadExtensionsResult } from "../src/core/extensions/index.js"; +import { createExtensionRuntime, loadExtensionFromFactory } from "../src/core/extensions/loader.js"; import { ModelRegistry } from "../src/core/model-registry.js"; import type { ResourceLoader } from "../src/core/resource-loader.js"; import { SessionManager } from "../src/core/session-manager.js"; @@ -175,9 +177,46 @@ export interface TestSessionContext { cleanup: () => void; } -export function createTestResourceLoader(): ResourceLoader { +export interface CreateTestExtensionsResultInput { + factory: ExtensionFactory; + path?: string; +} + +export async function createTestExtensionsResult( + inputs: Array, + cwd = process.cwd(), +): Promise { + const runtime = createExtensionRuntime(); + const eventBus = createEventBus(); + const extensions: Extension[] = []; + + for (const [index, input] of inputs.entries()) { + const factory = typeof input === "function" ? input : input.factory; + const extensionPath = + typeof input === "function" ? `` : (input.path ?? ``); + extensions.push(await loadExtensionFromFactory(factory, cwd, eventBus, runtime, extensionPath)); + } + return { - getExtensions: () => ({ extensions: [], errors: [], runtime: createExtensionRuntime() }), + extensions, + errors: [], + runtime, + }; +} + +export interface CreateTestResourceLoaderOptions { + extensionsResult?: LoadExtensionsResult; +} + +export function createTestResourceLoader(options: CreateTestResourceLoaderOptions = {}): ResourceLoader { + const extensionsResult = options.extensionsResult ?? { + extensions: [], + errors: [], + runtime: createExtensionRuntime(), + }; + + return { + getExtensions: () => extensionsResult, getSkills: () => ({ skills: [], diagnostics: [] }), getPrompts: () => ({ prompts: [], diagnostics: [] }), getThemes: () => ({ themes: [], diagnostics: [] }),