diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 5709b68d0..a6790abef 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed `task`-spawned subagents repeating filesystem scans the parent had already completed. `ExecutorOptions` and the `createAgentSession()` call inside `runSubprocess()` did not forward `rules`, `preloadedExtensions`, or the discovered `.omp/tools/` paths, so each subagent re-ran `loadCapability()`, `loadSessionExtensions()`, and the full `.omp/tools/` walk. The toolsession now caches `session.rules`, `session.extensionsResult`, and `session.customToolPaths`; `runSubprocess()` threads them through; and `createAgentSession()` accepts a new `preloadedCustomToolPaths` option backed by a new exported `discoverCustomToolPaths()` helper. Custom-tool factories are still re-bound per session so each `CustomToolAPI` (cwd, exec, pushPendingAction, UI) targets the right session — only the FS scan is skipped. `preloadedExtensions` is shallow-cloned inside the SDK so inline-extension augmentation (autoresearch + custom-tools wrapper) cannot bleed back into the caller's array ([#2190](https://github.com/can1357/oh-my-pi/issues/2190)). +- Fixed `task`-spawned subagents repeating filesystem scans the parent had already completed. `ExecutorOptions` and the `createAgentSession()` call inside `runSubprocess()` did not forward `rules`, the discovered extension paths, or the discovered `.omp/tools/` paths, so each subagent re-ran `loadCapability()`, `discoverAndLoadExtensions()`, and the full `.omp/tools/` walk. The toolsession now caches `session.rules`, `session.extensionPaths`, and `session.customToolPaths`; `runSubprocess()` threads them through; and `createAgentSession()` accepts new `preloadedExtensionPaths` and `preloadedCustomToolPaths` options backed by new exported `discoverExtensionPaths()` and `discoverCustomToolPaths()` helpers. Crucially, only path lists are forwarded — never loaded instances. Each session rebuilds its own `Extension` and `LoadedCustomTool` objects so the per-session `ExtensionAPI`/`CustomToolAPI` (cwd, eventBus, runtime, exec, pushPendingAction, UI) targets the right session; forwarding loaded instances would have routed extension handlers and custom-tool execution back through the parent. The CLI's `preloadedExtensions` short-circuit is preserved for same-process reuse and now shallow-clones the caller's `extensions` array so inline-extension augmentation (autoresearch + custom-tools wrapper) cannot bleed back into it ([#2190](https://github.com/can1357/oh-my-pi/issues/2190)). ## [15.10.8] - 2026-06-09 diff --git a/packages/coding-agent/src/extensibility/extensions/index.ts b/packages/coding-agent/src/extensibility/extensions/index.ts index 8a06778c7..c28e524bd 100644 --- a/packages/coding-agent/src/extensibility/extensions/index.ts +++ b/packages/coding-agent/src/extensibility/extensions/index.ts @@ -5,6 +5,7 @@ export type { SlashCommandInfo, SlashCommandLocation, SlashCommandSource } from "../slash-commands"; export { discoverAndLoadExtensions, + discoverExtensionPaths, ExtensionRuntimeNotInitializedError, loadExtensionFromFactory, loadExtensions, diff --git a/packages/coding-agent/src/extensibility/extensions/loader.ts b/packages/coding-agent/src/extensibility/extensions/loader.ts index 40f1c4da9..54639e380 100644 --- a/packages/coding-agent/src/extensibility/extensions/loader.ts +++ b/packages/coding-agent/src/extensibility/extensions/loader.ts @@ -475,16 +475,24 @@ async function discoverExtensionsInDir(dir: string): Promise { return discovered; } - /** - * Discover and load extensions from standard locations. + * Discover absolute paths of extensions to load, without importing or + * binding factories. Hot path on session startup — the scan walks native + * `.omp`/`.pi` extension capabilities, the installed-plugin tree, and any + * configured paths. + * + * Subagents reuse the parent's collected paths via the SDK's + * `preloadedExtensionPaths` option, then call {@link loadExtensions} themselves + * so each session rebuilds Extension instances bound to its OWN + * `ExtensionAPI` (cwd, eventBus, runtime). Forwarding the parent's + * `LoadExtensionsResult` directly would reuse handlers/tools/commands that + * closed over the parent's `cwd` and event bus. */ -export async function discoverAndLoadExtensions( +export async function discoverExtensionPaths( configuredPaths: string[], cwd: string, - eventBus?: EventBus, disabledExtensionIds: string[] = [], -): Promise { +): Promise { const allPaths: string[] = []; const seen = new Set(); const disabled = new Set(disabledExtensionIds); @@ -545,5 +553,20 @@ export async function discoverAndLoadExtensions( addPath(resolved); } - return loadExtensions(allPaths, cwd, eventBus); + return allPaths; +} + +/** + * Discover and load extensions from standard locations. Composed of + * {@link discoverExtensionPaths} (FS scan) + {@link loadExtensions} + * (per-session binding). + */ +export async function discoverAndLoadExtensions( + configuredPaths: string[], + cwd: string, + eventBus?: EventBus, + disabledExtensionIds: string[] = [], +): Promise { + const paths = await discoverExtensionPaths(configuredPaths, cwd, disabledExtensionIds); + return loadExtensions(paths, cwd, eventBus); } diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 95b4cf3b2..730c0bca1 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -66,6 +66,7 @@ import { discoverCustomToolPaths, loadCustomTools, type ToolPathWithSource } fro import type { CustomTool, CustomToolContext, CustomToolSessionEvent } from "./extensibility/custom-tools/types"; import { discoverAndLoadExtensions, + discoverExtensionPaths, type ExtensionContext, type ExtensionFactory, ExtensionRunner, @@ -337,10 +338,29 @@ export interface CreateAgentSessionOptions { /** Disable extension discovery (explicit paths still load). */ disableExtensionDiscovery?: boolean; /** - * Pre-loaded extensions (skips file discovery). - * @internal Used by CLI when extensions are loaded early to parse custom flags. + * Pre-loaded extensions (skips file discovery and the per-session factory + * call). Used by the CLI when extensions are loaded early to parse custom + * flags — the same process owns the returned instances, so reusing them is + * safe. + * + * NEVER pass this across session boundaries (e.g. parent → subagent). + * `Extension` instances close over a parent-bound `ExtensionAPI` (cwd, + * eventBus, runtime), and reusing them would route tools/handlers/commands + * back through the parent. For subagents, forward + * {@link preloadedExtensionPaths} instead. + * + * @internal */ preloadedExtensions?: LoadExtensionsResult; + /** + * Pre-discovered extension source paths. When provided, the filesystem-scan + * inside `discoverExtensionPaths()` is skipped — the session still calls + * `loadExtensions()` itself so each `Extension` is bound to THIS session's + * `ExtensionAPI` (cwd, eventBus, runtime). + * + * This is the safe pass-through for parent → subagent forwarding. + */ + preloadedExtensionPaths?: string[]; /** * Pre-discovered custom-tool source paths from `.omp/tools/`, `.claude/tools/`, * plugins, etc. When provided, the filesystem-scan inside @@ -577,6 +597,26 @@ export async function discoverExtensions(cwd?: string): Promise, + cwd: string, + settings: Settings, +): Promise { + if (options.disableExtensionDiscovery) { + return options.additionalExtensionPaths ?? []; + } + const configuredPaths = [...(options.additionalExtensionPaths ?? []), ...(settings.get("extensions") ?? [])]; + const disabledExtensionIds = settings.get("disabledExtensions") ?? []; + return discoverExtensionPaths(configuredPaths, cwd, disabledExtensionIds); +} + /** * Load the discovered/configured extensions for a session — everything {@link * createAgentSession} would load except the inline factory extensions it appends @@ -592,23 +632,8 @@ export async function loadSessionExtensions( settings: Settings, eventBus: EventBus, ): Promise { - let result: LoadExtensionsResult; - if (options.disableExtensionDiscovery) { - const configuredPaths = options.additionalExtensionPaths ?? []; - result = await logger.time("loadExtensions", loadExtensions, configuredPaths, cwd, eventBus); - } else { - // Merge CLI extension paths with settings extension paths. - const configuredPaths = [...(options.additionalExtensionPaths ?? []), ...(settings.get("extensions") ?? [])]; - const disabledExtensionIds = settings.get("disabledExtensions") ?? []; - result = await logger.time( - "discoverAndLoadExtensions", - discoverAndLoadExtensions, - configuredPaths, - cwd, - eventBus, - disabledExtensionIds, - ); - } + const paths = await discoverSessionExtensionPaths(options, cwd, settings); + const result = await logger.time("loadExtensions", loadExtensions, paths, cwd, eventBus); for (const { path, error } of result.errors) { logger.error("Failed to load extension", { path, error }); } @@ -1560,24 +1585,48 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} inlineExtensions.push(createCustomToolsExtension(customTools)); } - // Load extensions. A preloaded result (e.g. resolved by the CLI before - // session creation so it can classify `@file` args extension-aware without - // a session/breadcrumb existing yet, or threaded from a parent session into - // a subagent) is reused without redoing discovery; otherwise discover now - // through the shared helper. Preloaded wins over `disableExtensionDiscovery` - // because the preloaded result already reflects that choice — re-running - // the loader here would double-load. - // - // Shallow-clone `extensions` so the inline-extensions push below (and any - // other per-session augmentation) never mutates the caller's array; the - // `runtime` is intentionally shared so flag values set pre-creation flow - // into the live session. - const extensionsResult: LoadExtensionsResult = options.preloadedExtensions - ? { ...options.preloadedExtensions, extensions: [...options.preloadedExtensions.extensions] } - : await loadSessionExtensions(options, cwd, settings, eventBus); - // Forward the resolved extensions result to subagents so they reuse it - // instead of repeating `loadSessionExtensions` discovery. - toolSession.extensionsResult = extensionsResult; + // Load extensions. Three paths: + // 1. `preloadedExtensions` (CLI): caller already loaded — reuse the + // Extension instances. Shallow-clone `extensions` so the inline + // push below cannot mutate the caller's array. `runtime` is shared + // so flag values set pre-creation flow into the live session. + // 2. `preloadedExtensionPaths` (subagent): caller resolved paths; + // skip the FS scan but always re-call `loadExtensions` here so + // each `Extension` binds to THIS session's `ExtensionAPI` + // (cwd, eventBus, runtime). + // 3. No preload: run the full session discovery. + // `disableExtensionDiscovery` is honored implicitly: a caller that set + // the flag and pre-resolved the result already reflects that choice. + let extensionPaths: string[]; + let extensionsResult: LoadExtensionsResult; + if (options.preloadedExtensions) { + extensionsResult = { + ...options.preloadedExtensions, + extensions: [...options.preloadedExtensions.extensions], + }; + // Capture paths for downstream forwarding; filter inline-factory + // entries (``) — those are per-session, not source paths. + extensionPaths = extensionsResult.extensions + .map(ext => ext.resolvedPath) + .filter(p => !p.startsWith(" + discoverSessionExtensionPaths(options, cwd, settings), + ); + extensionsResult = await logger.time("loadExtensions", loadExtensions, extensionPaths, cwd, eventBus); + for (const { path, error } of extensionsResult.errors) { + logger.error("Failed to load extension", { path, error }); + } + } + // Forward the source-path list (NOT the loaded instances) so subagents + // rebuild their own session-scoped extensions. + toolSession.extensionPaths = extensionPaths; // Load inline extensions from factories if (inlineExtensions.length > 0) { diff --git a/packages/coding-agent/src/task/executor.ts b/packages/coding-agent/src/task/executor.ts index a95a92a25..0337e5579 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -18,7 +18,6 @@ import type { ToolPathWithSource } from "../extensibility/custom-tools"; import type { CustomTool } from "../extensibility/custom-tools/types"; import { runExtensionCompact, runExtensionSetModel } from "../extensibility/extensions/compact-handler"; import { getSessionSlashCommands } from "../extensibility/extensions/get-commands-handler"; -import type { LoadExtensionsResult } from "../extensibility/extensions/types"; import { buildSkillPromptMessage, type Skill } from "../extensibility/skills"; import type { HindsightSessionState } from "../hindsight/state"; import type { LocalProtocolOptions } from "../internal-urls"; @@ -195,8 +194,12 @@ export interface ExecutorOptions { workspaceTree?: WorkspaceTree; /** Parent-discovered rules, forwarded to skip rule discovery in the subagent. */ rules?: Rule[]; - /** Parent-loaded extensions, forwarded via `preloadedExtensions` to skip discovery. */ - preloadedExtensions?: LoadExtensionsResult; + /** + * Parent's discovered extension source paths. Forwarded to skip the + * extension FS scan in the subagent; the subagent then re-binds each + * extension against its own `ExtensionAPI` (cwd, eventBus, runtime). + */ + preloadedExtensionPaths?: string[]; /** * Parent's discovered custom-tool source paths. Forwarded to skip the * `.omp/tools/` FS scan in the subagent; the subagent then re-binds each @@ -1298,7 +1301,7 @@ export async function runSubprocess(options: ExecutorOptions): Promise { const subagentPrompt = prompt.render(subagentSystemPromptTemplate, { diff --git a/packages/coding-agent/src/task/index.ts b/packages/coding-agent/src/task/index.ts index 8a2464edb..26f2d116c 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -991,7 +991,7 @@ export class TaskTool implements AgentTool`) are NOT included — those are session-local. + */ + extensionPaths?: string[]; /** * Pre-discovered custom-tool source paths from `.omp/tools/`, `.claude/tools/`, * plugins, etc. Forwarded to subagents so they skip the FS scan but still diff --git a/packages/coding-agent/test/sdk-extensions-per-session-binding.test.ts b/packages/coding-agent/test/sdk-extensions-per-session-binding.test.ts new file mode 100644 index 000000000..18c03815c --- /dev/null +++ b/packages/coding-agent/test/sdk-extensions-per-session-binding.test.ts @@ -0,0 +1,85 @@ +/** + * Regression guard for PR review feedback on #2190. + * + * Subagents inherit the parent's extension source *paths* (a cheap FS scan + * the parent already paid for), but each session MUST rebuild its own + * `Extension` instances so factories see the subagent's `ExtensionAPI` + * (cwd, eventBus, runtime). Forwarding the parent's loaded Extension + * instances would have tools/handlers/commands close over the parent's + * `cwd` and event bus — wrong for isolated tasks. + * + * Pins down `loadExtensions()` so the SDK can rely on it returning fresh + * Extension instances per call. + */ +import { afterAll, beforeAll, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { loadExtensions } from "@oh-my-pi/pi-coding-agent/extensibility/extensions"; +import { EventBus } from "@oh-my-pi/pi-coding-agent/utils/event-bus"; + +describe("loadExtensions per-session binding (#2190 review fix)", () => { + let tmp: string; + let extPath: string; + + beforeAll(async () => { + tmp = await fs.mkdtemp(path.join(os.tmpdir(), "pi-ext-binding-")); + extPath = path.join(tmp, "record-cwd.ts"); + // Factory tags the extension with the cwd + events it was bound to so + // the test can inspect what closures captured. + await fs.writeFile( + extPath, + [ + "export default function (api) {", + " api.registerTool({", + " name: 'tag',", + " description: 'binding probe',", + " params: api.typebox.Type.Object({}),", + " async execute() { return { content: [{ type: 'text', text: '' }] }; },", + " });", + " Object.defineProperty(globalThis, '__lastExtBinding', {", + " value: { cwd: api.exec.toString().includes('cwd') ? api : api, events: api.events },", + " writable: true,", + " configurable: true,", + " });", + " globalThis.__bindings = globalThis.__bindings || [];", + " globalThis.__bindings.push({ events: api.events });", + "}", + ].join("\n"), + ); + }); + + afterAll(async () => { + await fs.rm(tmp, { recursive: true, force: true }); + delete (globalThis as { __bindings?: unknown }).__bindings; + delete (globalThis as { __lastExtBinding?: unknown }).__lastExtBinding; + }); + + it("creates a distinct Extension and ExtensionAPI per call (fresh eventBus + runtime)", async () => { + (globalThis as { __bindings?: { events: EventBus }[] }).__bindings = []; + + const parentEventBus = new EventBus(); + const subagentEventBus = new EventBus(); + expect(parentEventBus).not.toBe(subagentEventBus); + + const parent = await loadExtensions([extPath], "/tmp/parent-cwd", parentEventBus); + const subagent = await loadExtensions([extPath], "/tmp/subagent-cwd", subagentEventBus); + + expect(parent.errors).toEqual([]); + expect(subagent.errors).toEqual([]); + expect(parent.extensions).toHaveLength(1); + expect(subagent.extensions).toHaveLength(1); + + // Distinct Extension instances — the subagent must never share with parent. + expect(subagent.extensions[0]).not.toBe(parent.extensions[0]); + // Distinct ExtensionRuntime instances — flagValues and pendingProviderRegistrations + // MUST NOT be shared, or per-session flags/registrations bleed across. + expect(subagent.runtime).not.toBe(parent.runtime); + + // Each factory saw the eventBus passed to its own loadExtensions call. + const bindings = (globalThis as { __bindings?: { events: EventBus }[] }).__bindings ?? []; + expect(bindings).toHaveLength(2); + expect(bindings[0]?.events).toBe(parentEventBus); + expect(bindings[1]?.events).toBe(subagentEventBus); + }); +}); diff --git a/packages/coding-agent/test/sdk-preloaded-extensions-isolation.test.ts b/packages/coding-agent/test/sdk-preloaded-extensions-isolation.test.ts index f940c56ce..e37e757bf 100644 --- a/packages/coding-agent/test/sdk-preloaded-extensions-isolation.test.ts +++ b/packages/coding-agent/test/sdk-preloaded-extensions-isolation.test.ts @@ -1,12 +1,15 @@ /** - * Regression guard for issue #2190. + * Regression guard for issue #2190 / PR #2193 review. * - * Subagents pass the parent session's `extensionsResult` as - * `preloadedExtensions` so the extension-discovery work isn't repeated. - * `createAgentSession()` augments the result with inline extensions - * (autoresearch + custom-tools wrapper), so it MUST clone the caller's - * `extensions` array before mutating it — otherwise the parent's runtime - * accumulates every subagent's inline wrappers. + * The CLI loads extensions early to parse custom flags, then hands the result + * back through `preloadedExtensions` so its OWN session can reuse the loaded + * instances without redoing the FS scan. `createAgentSession()` augments the + * result with inline extensions (autoresearch + custom-tools wrapper), so it + * MUST clone the caller's `extensions` array before mutating it — otherwise + * the caller's array accumulates session-local wrappers it never authored. + * + * Subagent forwarding is a separate path (`preloadedExtensionPaths`) which + * reloads extensions per session so each session's `ExtensionAPI` is its own. */ import { afterAll, beforeAll, describe, expect, it } from "bun:test"; import * as fs from "node:fs"; diff --git a/packages/coding-agent/test/task/executor-pass-through.test.ts b/packages/coding-agent/test/task/executor-pass-through.test.ts index a516775ed..d6017dac0 100644 --- a/packages/coding-agent/test/task/executor-pass-through.test.ts +++ b/packages/coding-agent/test/task/executor-pass-through.test.ts @@ -94,16 +94,12 @@ describe("runSubprocess parent-discovery pass-through (issue #2190)", () => { vi.restoreAllMocks(); }); - it("forwards rules, preloadedExtensions, and preloadedCustomToolPaths to createAgentSession", async () => { + it("forwards rules, preloadedExtensionPaths, and preloadedCustomToolPaths to createAgentSession", async () => { const session = yieldEmittingSession(); const spy = vi.spyOn(sdkModule, "createAgentSession").mockResolvedValue(createSessionResult(session)); const rules: Rule[] = [{ name: "rule-a" } as unknown as Rule]; - const preloadedExtensions = { - extensions: [], - errors: [], - runtime: { sentinel: "runtime" } as unknown, - } as unknown as LoadExtensionsResult; + const preloadedExtensionPaths = ["/abs/parent/.omp/extensions/foo.ts"]; const preloadedCustomToolPaths: ToolPathWithSource[] = [ { path: "tools/x.ts", source: { provider: "config", providerName: "Config", level: "project" } }, ]; @@ -111,7 +107,7 @@ describe("runSubprocess parent-discovery pass-through (issue #2190)", () => { const result = await runSubprocess({ ...baseOptions, rules, - preloadedExtensions, + preloadedExtensionPaths, preloadedCustomToolPaths, }); @@ -120,7 +116,7 @@ describe("runSubprocess parent-discovery pass-through (issue #2190)", () => { const forwarded = spy.mock.calls[0]?.[0]; // Identity, not equality: passing a clone would defeat the perf fix. expect(forwarded?.rules).toBe(rules); - expect(forwarded?.preloadedExtensions).toBe(preloadedExtensions); + expect(forwarded?.preloadedExtensionPaths).toBe(preloadedExtensionPaths); expect(forwarded?.preloadedCustomToolPaths).toBe(preloadedCustomToolPaths); }); @@ -133,7 +129,7 @@ describe("runSubprocess parent-discovery pass-through (issue #2190)", () => { expect(result.exitCode).toBe(0); const forwarded = spy.mock.calls[0]?.[0]; expect(forwarded?.rules).toBeUndefined(); - expect(forwarded?.preloadedExtensions).toBeUndefined(); + expect(forwarded?.preloadedExtensionPaths).toBeUndefined(); expect(forwarded?.preloadedCustomToolPaths).toBeUndefined(); }); });