From 84405a001c981667b75c5272dee59da9a3e1d0eb Mon Sep 17 00:00:00 2001 From: gustav-fff <286169375+gustav-fff@users.noreply.github.com> Date: Mon, 7 Sep 2026 08:24:11 -0700 Subject: [PATCH] fix(pi-fff): drop stale mode tool names on reload (#855) Pi carries the active tool list across /reload, and registerPendingTools only added names, so the previous mode's search tools stayed active. Prune the names the final mode did not register: FFF names unconditionally (only this extension registers them), override names only when the session actually ran in override mode, so a user-enabled builtin grep/find is kept. Closes #855 --- packages/pi-fff/src/index.ts | 67 +++++++++++++++------- packages/pi-fff/test/extension.test.ts | 79 +++++++++++++++++++++++++- 2 files changed, 125 insertions(+), 21 deletions(-) diff --git a/packages/pi-fff/src/index.ts b/packages/pi-fff/src/index.ts index 75d93cfe..63db6735 100644 --- a/packages/pi-fff/src/index.ts +++ b/packages/pi-fff/src/index.ts @@ -78,6 +78,10 @@ function resolveToolNames(mode: FffMode): ToolNames { return mode === "override" ? OVERRIDE_TOOL_NAMES : FFF_TOOL_NAMES; } +function toolNameList(names: ToolNames): string[] { + return [names.grep, names.find, names.multiGrep]; +} + // --------------------------------------------------------------------------- // Cursor store — simple bounded Map for pagination cursors // --------------------------------------------------------------------------- @@ -656,11 +660,19 @@ export default function fffExtension(pi: ExtensionAPI) { }); } - function registerPendingTools(): void { + // Pi carries the active tool list across /reload, so names activated under a + // previously used mode stay active unless we drop them here (#855). + function registerPendingTools(staleNames: readonly string[]): void { if (toolsRegistered) return; - const registeredNames = pendingTools.map((register) => register()); - pi.setActiveTools([...new Set([...pi.getActiveTools(), ...registeredNames])]); + const registeredNames = new Set(pendingTools.map((register) => register())); + const stale = new Set(staleNames.filter((name) => !registeredNames.has(name))); + pi.setActiveTools([ + ...new Set([ + ...pi.getActiveTools().filter((name) => !stale.has(name)), + ...registeredNames, + ]), + ]); toolsRegistered = true; } @@ -720,28 +732,24 @@ export default function fffExtension(pi: ExtensionAPI) { // Pi populates extension flag values after loading extensions. resolveStartupConfig(); + // FFF-named tools are ours alone, so they are always safe to drop. Override + // names collide with pi's builtins and are only stale once this session ran + // in override mode, which is the sole way we could have activated them. + const staleNames = toolNameList(FFF_TOOL_NAMES); + let usedOverride = currentMode === "override"; + // Restore persisted mode before registering tools so a saved override // can safely change their names after /reload or session resume. - const entries = ctx.sessionManager?.getEntries(); - if (entries) { - const modeEntry = [...entries] - .reverse() - .find( - (e: { type: string; customType?: string }) => - e.type === "custom" && e.customType === "fff-mode", - ); - if ( - modeEntry && - typeof (modeEntry as any).data?.mode === "string" && - VALID_MODES.includes((modeEntry as any).data.mode as FffMode) - ) { - const restored = (modeEntry as any).data.mode as FffMode; - if (restored !== currentMode) setMode(restored); - } + const modes = sessionModes(ctx.sessionManager?.getEntries()); + if (modes.length > 0) { + const restored = modes[modes.length - 1]; + if (restored !== currentMode) setMode(restored); + usedOverride = usedOverride || modes.includes("override"); } + if (usedOverride) staleNames.push(...toolNameList(OVERRIDE_TOOL_NAMES)); initializeFinderFactories(); - registerPendingTools(); + registerPendingTools(staleNames); } pi.on("session_start", async (_event, ctx) => { @@ -1358,3 +1366,22 @@ export default function fffExtension(pi: ExtensionAPI) { }, }); } + +// Every mode this session selected via /fff-mode, oldest first. +function sessionModes(entries: unknown): FffMode[] { + if (!Array.isArray(entries)) return []; + + const modes: FffMode[] = []; + for (const entry of entries as { + type?: string; + customType?: string; + data?: unknown; + }[]) { + if (entry?.type !== "custom" || entry.customType !== "fff-mode") continue; + const mode = (entry.data as { mode?: unknown } | undefined)?.mode; + if (typeof mode === "string" && VALID_MODES.includes(mode as FffMode)) { + modes.push(mode as FffMode); + } + } + return modes; +} diff --git a/packages/pi-fff/test/extension.test.ts b/packages/pi-fff/test/extension.test.ts index e9d7fd58..c03fa3fe 100644 --- a/packages/pi-fff/test/extension.test.ts +++ b/packages/pi-fff/test/extension.test.ts @@ -147,7 +147,7 @@ function createPi(mode?: string, flags: Record = {}) { registerTool: mock((_tool: any) => undefined), getActiveTools: mock(() => ["read"] as string[]), setActiveTools: mock((_names: string[]) => undefined), - appendEntry: mock(() => undefined), + appendEntry: mock((_customType: string, _data: unknown) => undefined), }; return { pi, events, commands }; @@ -400,6 +400,83 @@ describe("pi-fff session mode", () => { }); }); +// Regression for #855: pi carries the active tool list and the session entries across +// /reload, but drops the extension instance, so a mode switch must not leave the +// previous mode's tool names active. +describe("pi-fff mode switch across /reload", () => { + // Pi's own default active set: builtin grep/find ship inactive (dist/core/sdk.js). + const PI_DEFAULT_ACTIVE = ["read", "bash", "edit", "write"]; + + function createReloadableSession(startupMode?: string, active = PI_DEFAULT_ACTIVE) { + let activeTools = [...active]; + const entries: unknown[] = []; + + async function load() { + const setup = createPi(startupMode); + setup.pi.getActiveTools.mockImplementation(() => [...activeTools]); + setup.pi.setActiveTools.mockImplementation((names: string[]) => { + activeTools = [...names]; + }); + setup.pi.appendEntry.mockImplementation((customType: string, data: unknown) => { + entries.push({ type: "custom", customType, data }); + }); + + const ctx = createContext(); + ctx.sessionManager.getEntries.mockReturnValue(entries as any[]); + fffExtension(setup.pi as any); + await setup.events.get("session_start")?.({ reason: "startup" }, ctx); + return { ...setup, ctx }; + } + + return { load, getActiveTools: () => activeTools }; + } + + test("drops override tool names when switching back to a FFF-named mode", async () => { + const session = createReloadableSession("override"); + + const first = await session.load(); + expect(session.getActiveTools()).toEqual([...PI_DEFAULT_ACTIVE, "grep", "find"]); + await first.commands.get("fff-mode").handler("tools-and-ui", first.ctx); + await shutdown(first); + + const second = await session.load(); + expect(session.getActiveTools()).toEqual([...PI_DEFAULT_ACTIVE, "ffgrep", "fffind"]); + await shutdown(second); + }); + + test("drops FFF tool names when switching to override", async () => { + const session = createReloadableSession("tools-and-ui"); + + const first = await session.load(); + expect(session.getActiveTools()).toEqual([...PI_DEFAULT_ACTIVE, "ffgrep", "fffind"]); + await first.commands.get("fff-mode").handler("override", first.ctx); + await shutdown(first); + + const second = await session.load(); + expect(session.getActiveTools()).toEqual([...PI_DEFAULT_ACTIVE, "grep", "find"]); + await shutdown(second); + }); + + test("keeps user-enabled builtin grep and find when override was never used", async () => { + const session = createReloadableSession("tools-and-ui", [ + ...PI_DEFAULT_ACTIVE, + "grep", + "find", + ]); + + const setup = await session.load(); + + expect(session.getActiveTools()).toEqual([ + ...PI_DEFAULT_ACTIVE, + "grep", + "find", + "ffgrep", + "fffind", + ]); + await shutdown(setup); + }); +}); + // Regression for #743: launching from $HOME must be visible and interruptible. describe("pi-fff $HOME scan warning", () => { test("warns and pins a status when cwd is $HOME", async () => {