From fc834927e042c569b0a49dbcce8449770b6f9e9d Mon Sep 17 00:00:00 2001 From: Ravi Tharuma Date: Wed, 22 Jul 2026 22:12:10 +0200 Subject: [PATCH] fix: defer built-in tool overrides until session lifecycle Built-in tool overrides (including bash) were still registered at extension load time. That races with packages like pi-patty-bg-tasks that also own bash and produces a hard Pi startup conflict: Tool "bash" conflicts with .../pi-tool-display/index.ts Queue built-in overrides at load and only register them from session_start / before_agent_start after re-checking pi.getAllTools() ownership. Yield any tool already owned by a non-builtin source. Updates README ownership docs and adds regression coverage for late bash ownership (pi-patty-bg-tasks load order). --- CHANGELOG.md | 4 ++ README.md | 13 +++- src/tool-overrides.ts | 30 +++++++-- tests/reload-behavior.test.ts | 75 +++++++++++++++-------- tests/tool-overrides-registration.test.ts | 46 +++++++++++--- 5 files changed, 130 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 601d63d..b411ecc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- Actually defer built-in tool override registration (`read`/`grep`/`find`/`ls`/`bash`/`edit`/`write`) until `session_start` / `before_agent_start`, and re-evaluate ownership against the full tool graph before registering. This prevents hard startup conflicts when another extension owns `bash` (e.g. `pi-patty-bg-tasks`), which previously raced because overrides were still registered at extension load time despite README claims of deferred discovery. + + ## [0.5.0] - 2026-07-03 ### Added diff --git a/README.md b/README.md index 6a83c8a..27ea376 100644 --- a/README.md +++ b/README.md @@ -322,7 +322,18 @@ If another extension is already rendering one of the built-in tools: 2. Run `/reload` 3. Use `/tool-display show` to confirm the effective ownership state -Built-in tool overrides (including `bash`) are registered with deferred ownership discovery — the extension discovers which tools it owns via `pi.getAllTools()` during `before_agent_start` before overriding, preventing conflicts with other extensions that also register overrides. +Built-in tool overrides (including `bash`) are **queued at load** and only registered during `session_start` / `before_agent_start`. Before registering, the extension re-checks `pi.getAllTools()` and yields any tool already owned by a non-builtin source (for example `pi-patty-bg-tasks` owning `bash`). Immediate load-time registration is intentionally avoided so package order cannot create a hard `Tool "bash" conflicts with ...` diagnostic. + +If you still prefer an explicit config, set: + +```json +{ + "registerToolOverrides": { + "bash": false + } +} +``` + ### Config not loading diff --git a/src/tool-overrides.ts b/src/tool-overrides.ts index e9b47bb..7264801 100644 --- a/src/tool-overrides.ts +++ b/src/tool-overrides.ts @@ -1616,20 +1616,40 @@ export function registerToolDisplayOverrides( return false; }; + // Built-in overrides are collected at load time and applied after all + // extensions have registered tools. Immediate registration races with + // packages like pi-patty-bg-tasks that also own `bash`, producing a hard + // "Tool \"bash\" conflicts with ..." diagnostic at startup. + const pendingBuiltInToolOverrides = new Map void>(); + const registerIfOwned = ( toolName: BuiltInToolOverrideName, register: () => void, ): void => { if ( registeredBuiltInToolOverrides.has(toolName) || - !getConfig().registerToolOverrides[toolName] || - isExternallyOwnedBuiltInTool(toolName) + pendingBuiltInToolOverrides.has(toolName) || + !getConfig().registerToolOverrides[toolName] ) { return; } - register(); - registeredBuiltInToolOverrides.add(toolName); + pendingBuiltInToolOverrides.set(toolName, register); + }; + + const registerDeferredBuiltInToolOverrides = (): void => { + for (const [toolName, register] of pendingBuiltInToolOverrides) { + if ( + registeredBuiltInToolOverrides.has(toolName) || + !getConfig().registerToolOverrides[toolName] || + isExternallyOwnedBuiltInTool(toolName) + ) { + continue; + } + + register(); + registeredBuiltInToolOverrides.add(toolName); + } }; function createBuiltinToolBase(toolName: keyof BuiltInTools) { @@ -2110,11 +2130,13 @@ export function registerToolDisplayOverrides( pi.on("session_start", async () => { clearWriteExecutionMeta(writeExecutionMetaByToolCallId); + registerDeferredBuiltInToolOverrides(); registerMcpToolOverrides(); scheduleMcpToolOverrideDiscovery(); }); pi.on("before_agent_start", async () => { clearWriteExecutionMeta(writeExecutionMetaByToolCallId); + registerDeferredBuiltInToolOverrides(); registerMcpToolOverrides(); scheduleMcpToolOverrideDiscovery(); }); diff --git a/tests/reload-behavior.test.ts b/tests/reload-behavior.test.ts index ea50a01..47c37cf 100644 --- a/tests/reload-behavior.test.ts +++ b/tests/reload-behavior.test.ts @@ -144,27 +144,37 @@ test("1: after reload, new lifecycle handlers are registered", () => { // 2. Tool override restoration // --------------------------------------------------------------------------- -test("2: built-in tool overrides are re-registered on reload", () => { - const { api, capturedTools } = createApiStub(); +test("2: built-in tool overrides are re-registered on reload", async () => { + const { api, capturedTools, capturedHandlers } = createApiStub(); + const fireLifecycle = async () => { + for (const handler of capturedHandlers) { + if (handler.event === "session_start") { + await handler.handler({}, { ui: { theme: {}, notify: () => {} } }); + } + } + }; - // First call + // First call — tools register only after session_start. toolDisplayExtension(api); + assert.equal( + capturedTools.filter((t) => ["find", "ls", "write", "bash"].includes(t.name)).length, + 0, + "built-ins are deferred until session lifecycle", + ); + await fireLifecycle(); const firstTools = capturedTools.map((t) => t.name); - assert.ok(firstTools.includes("find"), "find registered on first call"); + assert.ok(firstTools.includes("find"), "find registered after session_start"); - // Simulate reload + // Simulate reload + lifecycle const countBeforeReload = capturedTools.length; toolDisplayExtension(api); + await fireLifecycle(); const countAfterReload = capturedTools.length; - - // Each call to registerToolDisplayOverrides registers the same built-in - // tools again (find, ls, write immediately; read/grep/edit/bash deferred). assert.ok( countAfterReload >= countBeforeReload + 3, - "at least 3 tools re-registered on reload", + "built-in tools re-registered on reload after lifecycle", ); - // Verify tool names appear multiple times, meaning they were re-registered const toolNameCounts = new Map(); for (const tool of capturedTools) { toolNameCounts.set(tool.name, (toolNameCounts.get(tool.name) ?? 0) + 1); @@ -211,30 +221,36 @@ test("2: re-registered tools have renderCall and renderResult functions after re } }); -test("2: built-in tool overrides register before lifecycle events and re-register on reload", async () => { +test("2: built-in tool overrides register on lifecycle events and re-register on reload", async () => { const { api, registeredTools, eventHandlers } = createExtensionApiStub(); registerToolDisplayOverrides(api, () => DEFAULT_TOOL_DISPLAY_CONFIG); - const firstImmediate = registeredTools.map((t) => t.name); + assert.equal(registeredTools.length, 0, "no built-ins registered before lifecycle events"); - for (const toolName of ["read", "edit", "grep", "bash"] as const) { - assert.ok(firstImmediate.includes(toolName), `${toolName} registered before lifecycle events`); + await eventHandlers.before_agent_start?.(); + const firstLifecycle = registeredTools.map((t) => t.name); + for (const toolName of ["read", "edit", "grep", "bash", "find", "ls", "write"] as const) { + assert.ok(firstLifecycle.includes(toolName), `${toolName} registered on before_agent_start`); } + const countAfterLifecycle = registeredTools.length; - const countBeforeLifecycle = registeredTools.length; await eventHandlers.before_agent_start?.(); assert.equal( registeredTools.length, - countBeforeLifecycle, + countAfterLifecycle, "before_agent_start does not duplicate already registered built-ins", ); registerToolDisplayOverrides(api, () => DEFAULT_TOOL_DISPLAY_CONFIG); - const countAfterReload = registeredTools.length; - + assert.equal( + registeredTools.length, + countAfterLifecycle, + "reload queues built-ins but does not register them until lifecycle", + ); + await eventHandlers.before_agent_start?.(); assert.ok( - countAfterReload >= countBeforeLifecycle + 7, - "built-in display overrides re-register during reload initialization", + registeredTools.length >= countAfterLifecycle + 7, + "built-in display overrides re-register on lifecycle after reload", ); }); @@ -710,30 +726,37 @@ test("8: session_start handler can be invoked after reload without errors", asyn // 9. Double reload safety // --------------------------------------------------------------------------- -test("9: calling toolDisplayExtension three times (double reload) is safe", () => { - const { api, capturedTools, capturedCommands } = createApiStub(); +test("9: calling toolDisplayExtension three times (double reload) is safe", async () => { + const { api, capturedTools, capturedCommands, capturedHandlers } = createApiStub(); + const fireLifecycle = async () => { + for (const handler of capturedHandlers) { + if (handler.event === "session_start") { + await handler.handler({}, { ui: { theme: {}, notify: () => {} } }); + } + } + }; // First call toolDisplayExtension(api); + await fireLifecycle(); const afterFirst = { tools: capturedTools.length, cmds: capturedCommands.length }; // First reload toolDisplayExtension(api); + await fireLifecycle(); const afterSecond = { tools: capturedTools.length, cmds: capturedCommands.length }; // Second reload (double reload) assert.doesNotThrow(() => toolDisplayExtension(api)); + await fireLifecycle(); const afterThird = { tools: capturedTools.length, cmds: capturedCommands.length }; - // Each call adds more registrations (no deduplication in the stub) + // Each call adds more registrations after lifecycle (no deduplication in the stub) assert.ok(afterThird.tools > afterSecond.tools, "tools registered on third call"); assert.ok(afterThird.cmds > afterSecond.cmds, "commands registered on third call"); // Verify all tool registrations have renderCall/renderResult for (const tool of capturedTools) { - if (tool.name === "read" || tool.name === "edit" || tool.name === "grep") { - continue; // Deferred tools - } if (tool.renderCall !== undefined) { assert.equal( typeof tool.renderCall, diff --git a/tests/tool-overrides-registration.test.ts b/tests/tool-overrides-registration.test.ts index f870f4d..b98e6d2 100644 --- a/tests/tool-overrides-registration.test.ts +++ b/tests/tool-overrides-registration.test.ts @@ -92,12 +92,15 @@ test("registerToolDisplayOverrides copies built-in prompt metadata onto overridd const { api, registeredTools, eventHandlers } = createExtensionApiStub(); registerToolDisplayOverrides(api, () => DEFAULT_TOOL_DISPLAY_CONFIG); + // Built-in overrides are deferred until session/agent lifecycle so other + // extensions can claim ownership first (e.g. pi-patty-bg-tasks for bash). + assert.deepEqual(registeredTools.map((tool) => tool.name).sort(), []); + await eventHandlers.before_agent_start?.(); + assert.deepEqual( registeredTools.map((tool) => tool.name).sort(), ["bash", "edit", "find", "grep", "ls", "read", "write"], ); - await eventHandlers.before_agent_start?.(); - assert.equal(registeredTools.length, 7); const byName = new Map(registeredTools.map((tool) => [tool.name, tool])); @@ -128,20 +131,49 @@ test("registerToolDisplayOverrides copies built-in prompt metadata onto overridd assert.equal(byName.get("bash")?.promptGuidelines, undefined); }); -test("registerToolDisplayOverrides registers built-in display renderers during extension load for pre-bind history rendering", () => { - const { api, registeredTools } = createExtensionApiStub(); +test("registerToolDisplayOverrides defers built-in display renderers until session lifecycle", async () => { + const { api, registeredTools, eventHandlers } = createExtensionApiStub(); registerToolDisplayOverrides(api, () => DEFAULT_TOOL_DISPLAY_CONFIG); + assert.equal(registeredTools.length, 0, "must not register built-ins during extension load"); + + await eventHandlers.session_start?.(); const byName = new Map(registeredTools.map((tool) => [tool.name, tool])); for (const name of ["read", "grep", "find", "ls", "bash", "edit", "write"] as const) { const registeredTool = byName.get(name); - assert.ok(registeredTool, `expected '${name}' to be available before session_start`); - assert.equal(typeof registeredTool.renderCall, "function", `${name} has renderCall before session_start`); - assert.equal(typeof registeredTool.renderResult, "function", `${name} has renderResult before session_start`); + assert.ok(registeredTool, `expected '${name}' after session_start`); + assert.equal(typeof registeredTool.renderCall, "function", `${name} has renderCall after session_start`); + assert.equal(typeof registeredTool.renderResult, "function", `${name} has renderResult after session_start`); } }); +test("defers built-in bash override when another extension owns bash after load", async () => { + // Simulate load-order: tool-display evaluates first (no external bash yet), + // then pi-patty-bg-tasks registers bash before session_start/before_agent_start. + const externalTools: unknown[] = []; + const { api, registeredTools, eventHandlers } = createExtensionApiStub(externalTools); + + registerToolDisplayOverrides(api, () => DEFAULT_TOOL_DISPLAY_CONFIG); + assert.equal(registeredTools.some((tool) => tool.name === "bash"), false); + + externalTools.push({ + name: "bash", + sourceInfo: { + source: "package", + path: "node_modules/pi-patty-bg-tasks/src/tools/bash.ts", + }, + }); + + await eventHandlers.session_start?.(); + await eventHandlers.before_agent_start?.(); + + const registeredNames = new Set(registeredTools.map((tool) => tool.name)); + assert.equal(registeredNames.has("bash"), false, "must yield bash to external owner"); + assert.equal(registeredNames.has("read"), true); + assert.equal(registeredNames.has("write"), true); +}); + test("registerToolDisplayOverrides clones built-in parameter schemas so Pi TUI keeps extension renderers active", async () => { const { api, registeredTools, eventHandlers } = createExtensionApiStub();