From e7645cec4902acad952f18a32494a0dd2c199db3 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 9 Jun 2026 13:58:21 +0000 Subject: [PATCH] fix(task): forward parent-discovered rules, extensions, and custom tools to subagents Each `runSubprocess` call re-ran `loadCapability()`, `loadSessionExtensions()`, and `discoverAndLoadCustomTools()` because `ExecutorOptions` and the `createAgentSession()` call inside the executor omitted three pass-through fields the parent had already paid for. The already-correct paths (skills, context files, workspace tree, MCP manager) showed the intended pattern. - Cache `rules`, `extensionsResult`, and `loadedCustomTools` on the parent's `ToolSession`. - Add `rules` / `preloadedExtensions` / `preloadedCustomTools` to `ExecutorOptions`; forward them from both `runSubprocess` call sites in `task/index.ts` and into the executor's `createAgentSession()`. - Add `preloadedCustomTools` to `CreateAgentSessionOptions` and skip `discoverAndLoadCustomTools()` when it is supplied. - Shallow-clone `extensionsResult.extensions` when reusing `preloadedExtensions`, so the per-session autoresearch + custom-tools inline wrappers never leak back into the caller's array. Fixes #2190 --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/src/sdk.ts | 107 +++++++++----- packages/coding-agent/src/task/executor.ts | 13 +- packages/coding-agent/src/task/index.ts | 6 + packages/coding-agent/src/tools/index.ts | 9 ++ ...sdk-preloaded-extensions-isolation.test.ts | 75 ++++++++++ .../test/task/executor-pass-through.test.ts | 139 ++++++++++++++++++ 7 files changed, 315 insertions(+), 38 deletions(-) create mode 100644 packages/coding-agent/test/sdk-preloaded-extensions-isolation.test.ts create mode 100644 packages/coding-agent/test/task/executor-pass-through.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index bef9ae4ac..c9159d46e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### 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/` (a.k.a. `LoadedCustomTool[]`) results, so each subagent re-ran `loadCapability()`, `loadSessionExtensions()`, and `discoverAndLoadCustomTools()`. The toolsession now caches each (`session.rules`, `session.extensionsResult`, `session.loadedCustomTools`), `runSubprocess()` threads them through, and `createAgentSession()` accepts a new `preloadedCustomTools` option. `preloadedExtensions` is now shallow-cloned inside the SDK so inline-extension augmentation (autoresearch + custom-tools wrapper) can never bleed back into the caller's array ([#2190](https://github.com/can1357/oh-my-pi/issues/2190)). + ## [15.10.8] - 2026-06-09 ### Added diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 75fdb3966..c5db5814a 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -63,7 +63,12 @@ import { loadCustomCommands as loadCustomCommandsInternal, } from "./extensibility/custom-commands"; import { discoverAndLoadCustomTools } from "./extensibility/custom-tools"; -import type { CustomTool, CustomToolContext, CustomToolSessionEvent } from "./extensibility/custom-tools/types"; +import type { + CustomTool, + CustomToolContext, + CustomToolSessionEvent, + LoadedCustomTool, +} from "./extensibility/custom-tools/types"; import { discoverAndLoadExtensions, type ExtensionContext, @@ -341,6 +346,14 @@ export interface CreateAgentSessionOptions { * @internal Used by CLI when extensions are loaded early to parse custom flags. */ preloadedExtensions?: LoadExtensionsResult; + /** + * Pre-loaded custom tools from `.omp/tools/`, `.claude/tools/`, plugins, etc. + * When provided, the filesystem-scan inside `discoverAndLoadCustomTools()` is + * skipped — subagents inherit the parent's discovery result. MCP/image/tts/ + * web-search tools are still resolved per-session because they depend on the + * session's own model and tool selection. + */ + preloadedCustomTools?: LoadedCustomTool[]; /** Shared event bus for tool/extension communication. Default: creates new bus. */ eventBus?: EventBus; @@ -1193,23 +1206,26 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} } // Discover rules and bucket them in one pass to avoid repeated scans over large rule sets. - const { ttsrManager, rulebookRules, alwaysApplyRules } = await logger.time("discoverTtsrRules", async () => { - const { TtsrManager } = await import("./export/ttsr"); - const ttsrSettings = settings.getGroup("ttsr"); - const ttsrManager = new TtsrManager(ttsrSettings); - const rulesResult = - options.rules !== undefined - ? { items: options.rules, warnings: undefined } - : await loadCapability(ruleCapability.id, { cwd }); - const { rulebookRules, alwaysApplyRules } = bucketRules(rulesResult.items, ttsrManager, { - builtinRules: ttsrSettings.builtinRules, - disabledRules: ttsrSettings.disabledRules, - }); - if (existingSession.injectedTtsrRules.length > 0) { - ttsrManager.restoreInjected(existingSession.injectedTtsrRules); - } - return { ttsrManager, rulebookRules, alwaysApplyRules }; - }); + const { ttsrManager, rulebookRules, alwaysApplyRules, allRules } = await logger.time( + "discoverTtsrRules", + async () => { + const { TtsrManager } = await import("./export/ttsr"); + const ttsrSettings = settings.getGroup("ttsr"); + const ttsrManager = new TtsrManager(ttsrSettings); + const rulesResult = + options.rules !== undefined + ? { items: options.rules, warnings: undefined } + : await loadCapability(ruleCapability.id, { cwd }); + const { rulebookRules, alwaysApplyRules } = bucketRules(rulesResult.items, ttsrManager, { + builtinRules: ttsrSettings.builtinRules, + disabledRules: ttsrSettings.disabledRules, + }); + if (existingSession.injectedTtsrRules.length > 0) { + ttsrManager.restoreInjected(existingSession.injectedTtsrRules); + } + return { ttsrManager, rulebookRules, alwaysApplyRules, allRules: rulesResult.items }; + }, + ); // Resolve contextFiles up-front (it's needed before tool creation). The // workspace tree scan is slow on large repos and we MUST NOT block startup on @@ -1331,6 +1347,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} contextFiles, workspaceTree: resolvedWorkspaceTree, skills, + rules: allRules, eventBus, outputSchema: options.outputSchema, requireYieldTool: options.requireYieldTool, @@ -1514,22 +1531,28 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} customTools.push(...getSearchTools()); } - // Discover and load custom tools from .omp/tools/, .claude/tools/, etc. + // Discover custom tools from `.omp/tools/`, `.claude/tools/`, plugins, etc. + // Subagents reuse the parent's discovery via `preloadedCustomTools` so the + // filesystem scan only runs once per process tree. MCP/image/tts/web-search + // tools above are still resolved per-session because they depend on the + // session's own model and tool selection. const builtInToolNames = builtinTools.map(t => t.name); - const discoveredCustomTools = await logger.time( - "discoverAndLoadCustomTools", - discoverAndLoadCustomTools, - [], - cwd, - builtInToolNames, - action => queueResolveHandler(toolSession, action), - ); - for (const { path, error } of discoveredCustomTools.errors) { - logger.error("Custom tool load failed", { path, error }); - } - if (discoveredCustomTools.tools.length > 0) { - customTools.push(...discoveredCustomTools.tools.map(loaded => loaded.tool)); + const loadedCustomTools: LoadedCustomTool[] = + options.preloadedCustomTools ?? + (await logger.time("discoverAndLoadCustomTools", async () => { + const result = await discoverAndLoadCustomTools([], cwd, builtInToolNames, action => + queueResolveHandler(toolSession, action), + ); + for (const { path, error } of result.errors) { + logger.error("Custom tool load failed", { path, error }); + } + return result.tools; + })); + if (loadedCustomTools.length > 0) { + customTools.push(...loadedCustomTools.map(loaded => loaded.tool)); } + // Forward the discovered tools to subagents so they skip the FS scan. + toolSession.loadedCustomTools = loadedCustomTools; const inlineExtensions: ExtensionFactory[] = options.extensions ? [...options.extensions] : []; inlineExtensions.push((await import("./autoresearch")).createAutoresearchExtension); @@ -1539,12 +1562,22 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} // 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) is reused as-is; otherwise discover now + // 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. - const extensionsResult: LoadExtensionsResult = - options.preloadedExtensions ?? (await loadSessionExtensions(options, cwd, settings, eventBus)); + // 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 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 94d3f7a17..6c3b3c9c6 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -8,14 +8,16 @@ import path from "node:path"; import type { AgentEvent, AgentIdentity, AgentTelemetryConfig, ThinkingLevel } from "@oh-my-pi/pi-agent-core"; import { recordHandoff, resolveTelemetry } from "@oh-my-pi/pi-agent-core"; import { logger, prompt, untilAborted } from "@oh-my-pi/pi-utils"; +import type { Rule } from "../capability/rule"; import { ModelRegistry } from "../config/model-registry"; import { resolveModelOverrideWithAuthFallback } from "../config/model-resolver"; import type { PromptTemplate } from "../config/prompt-templates"; import { Settings } from "../config/settings"; import { SETTINGS_SCHEMA, type SettingPath } from "../config/settings-schema"; -import type { CustomTool } from "../extensibility/custom-tools/types"; +import type { CustomTool, LoadedCustomTool } 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"; @@ -190,6 +192,12 @@ export interface ExecutorOptions { skills?: Skill[]; promptTemplates?: PromptTemplate[]; 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-discovered custom tools, forwarded to skip the `.omp/tools/` scan. */ + preloadedCustomTools?: LoadedCustomTool[]; mcpManager?: MCPManager; authStorage?: AuthStorage; modelRegistry?: ModelRegistry; @@ -1284,6 +1292,9 @@ export async function runSubprocess(options: ExecutorOptions): Promise { const subagentPrompt = prompt.render(subagentSystemPromptTemplate, { agent: agent.systemPrompt, diff --git a/packages/coding-agent/src/task/index.ts b/packages/coding-agent/src/task/index.ts index a6fdf4d6b..6d3157b25 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -990,6 +990,9 @@ export class TaskTool implements AgentTool { + let sharedDir: string; + let authStorage: AuthStorage; + let modelRegistry: ModelRegistry; + + beforeAll(async () => { + sharedDir = fs.mkdtempSync(path.join(os.tmpdir(), "pi-preloaded-ext-")); + authStorage = await AuthStorage.create(path.join(sharedDir, "auth.db")); + modelRegistry = new ModelRegistry(authStorage, path.join(sharedDir, "models.yml")); + }); + + afterAll(() => { + authStorage.close(); + fs.rmSync(sharedDir, { recursive: true, force: true }); + }); + + it("does not mutate the caller's extensions array when preloadedExtensions is provided", async () => { + const preloaded: LoadExtensionsResult = { + extensions: [], + errors: [], + runtime: { + flagValues: new Map(), + pendingProviderRegistrations: [], + // Cast: only the fields we touch matter; the SDK happily accepts a + // minimal runtime when no extension hooks fire. + } as unknown as LoadExtensionsResult["runtime"], + }; + const beforeLength = preloaded.extensions.length; + const beforeArrayRef = preloaded.extensions; + + await createAgentSession({ + cwd: sharedDir, + agentDir: sharedDir, + sessionManager: SessionManager.inMemory(), + modelRegistry, + settings: Settings.isolated(), + preloadedExtensions: preloaded, + // Disable everything that would touch the network / FS scans. + enableLsp: false, + enableMCP: false, + skipPythonPreflight: true, + skills: [], + rules: [], + preloadedCustomTools: [], + contextFiles: [], + promptTemplates: [], + }); + + // The session's own `extensionsResult` carries inline wrappers, but the + // caller's array (and its identity) must be untouched. + expect(preloaded.extensions).toBe(beforeArrayRef); + expect(preloaded.extensions.length).toBe(beforeLength); + }); +}); diff --git a/packages/coding-agent/test/task/executor-pass-through.test.ts b/packages/coding-agent/test/task/executor-pass-through.test.ts new file mode 100644 index 000000000..629bf0830 --- /dev/null +++ b/packages/coding-agent/test/task/executor-pass-through.test.ts @@ -0,0 +1,139 @@ +/** + * Verifies parent-discovered rules, extensions, and custom tools are forwarded + * to `createAgentSession` so subagents skip the FS scans the parent already + * paid for. Regression guard for issue #2190. + */ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import type { Rule } from "@oh-my-pi/pi-coding-agent/capability/rule"; +import type { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import type { LoadedCustomTool } from "@oh-my-pi/pi-coding-agent/extensibility/custom-tools/types"; +import type { LoadExtensionsResult } from "@oh-my-pi/pi-coding-agent/extensibility/extensions/types"; +import type { CreateAgentSessionResult } from "@oh-my-pi/pi-coding-agent/sdk"; +import * as sdkModule from "@oh-my-pi/pi-coding-agent/sdk"; +import type { AgentSession, AgentSessionEvent, PromptOptions } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { runSubprocess } from "@oh-my-pi/pi-coding-agent/task/executor"; +import type { AgentDefinition } from "@oh-my-pi/pi-coding-agent/task/types"; +import { EventBus } from "@oh-my-pi/pi-coding-agent/utils/event-bus"; + +function createMockSession(onPrompt: (params: { emit: (event: AgentSessionEvent) => void }) => void): AgentSession { + const listeners: Array<(event: AgentSessionEvent) => void> = []; + const emit = (event: AgentSessionEvent) => { + for (const listener of listeners) listener(event); + }; + const session = { + state: { messages: [] }, + agent: { state: { systemPrompt: ["test"] } }, + model: undefined, + extensionRunner: undefined, + sessionManager: { appendSessionInit: () => {} }, + getActiveToolNames: () => ["read", "yield"], + setActiveToolsByName: async (_toolNames: string[]) => {}, + subscribe: (listener: (event: AgentSessionEvent) => void) => { + listeners.push(listener); + return () => { + const index = listeners.indexOf(listener); + if (index >= 0) listeners.splice(index, 1); + }; + }, + prompt: async (_text: string, _options?: PromptOptions) => { + onPrompt({ emit }); + }, + waitForIdle: async () => {}, + getLastAssistantMessage: () => undefined, + abort: async () => {}, + dispose: async () => {}, + }; + return session as unknown as AgentSession; +} + +function yieldEmittingSession(): AgentSession { + return createMockSession(({ emit }) => { + emit({ + type: "tool_execution_end", + toolCallId: "tool-pass-through", + toolName: "yield", + result: { + content: [{ type: "text", text: "Result submitted." }], + details: { status: "success", data: { ok: true } }, + }, + isError: false, + }); + }); +} + +function createSessionResult(session: AgentSession): CreateAgentSessionResult { + return { + session, + extensionsResult: { extensions: [], errors: [], runtime: {} as unknown } as unknown as LoadExtensionsResult, + setToolUIContext: () => {}, + eventBus: new EventBus(), + }; +} + +const baseAgent: AgentDefinition = { + name: "task", + description: "test", + systemPrompt: "test", + source: "bundled", +}; + +const baseOptions = { + cwd: "/tmp", + agent: baseAgent, + task: "do work", + index: 0, + id: "subagent-pass-through", + settings: Settings.isolated(), + modelRegistry: { refresh: async () => {} } as unknown as ModelRegistry, + enableLsp: false, +}; + +describe("runSubprocess parent-discovery pass-through (issue #2190)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("forwards rules, preloadedExtensions, and preloadedCustomTools 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 preloadedCustomTools: LoadedCustomTool[] = [ + { path: "tools/x.ts", resolvedPath: "/abs/tools/x.ts", tool: { name: "x" } } as unknown as LoadedCustomTool, + ]; + + const result = await runSubprocess({ + ...baseOptions, + rules, + preloadedExtensions, + preloadedCustomTools, + }); + + expect(result.exitCode).toBe(0); + expect(spy).toHaveBeenCalledTimes(1); + 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?.preloadedCustomTools).toBe(preloadedCustomTools); + }); + + it("forwards undefined when the parent has not pre-discovered state", async () => { + const session = yieldEmittingSession(); + const spy = vi.spyOn(sdkModule, "createAgentSession").mockResolvedValue(createSessionResult(session)); + + const result = await runSubprocess({ ...baseOptions }); + + expect(result.exitCode).toBe(0); + const forwarded = spy.mock.calls[0]?.[0]; + expect(forwarded?.rules).toBeUndefined(); + expect(forwarded?.preloadedExtensions).toBeUndefined(); + expect(forwarded?.preloadedCustomTools).toBeUndefined(); + }); +});