fix(task): forward custom-tool paths to subagents, rebind tools per session

Reviewer flagged that forwarding `LoadedCustomTool[]` from a parent session
to a subagent reused tool instances whose factories had closed over the
parent's `CustomToolAPI` — `cwd`, `exec`, `pushPendingAction`, and `ui` all
pointed at the parent. In isolated tasks the tool would `exec` against the
parent worktree and queue pending actions on the parent session.

Forward only the path list; let each session rebuild tools through
`loadCustomTools` so factories see the right `CustomToolAPI`.

- `extensibility/custom-tools/loader.ts`: extract `discoverCustomToolPaths`
  (FS scan only) from `discoverAndLoadCustomTools`; export
  `ToolPathWithSource`. The combined helper is now `discoverCustomToolPaths`
  + `loadCustomTools`.
- `sdk.ts`: replace `preloadedCustomTools` (`LoadedCustomTool[]`) with
  `preloadedCustomToolPaths` (`ToolPathWithSource[]`). The custom-tools
  block runs `loadCustomTools` unconditionally; only the path scan is
  skipped when the caller pre-discovered it.
- `tools/index.ts`: `ToolSession.loadedCustomTools` →
  `ToolSession.customToolPaths` for the same reason.
- `task/executor.ts` and `task/index.ts`: forward `customToolPaths`.
  Drop the forward for isolated subagents — the worktree shifts `cwd`, so
  the subagent re-discovers tools against its own working tree.
- New `test/sdk-custom-tools-per-session-binding.test.ts` pins the contract:
  two `loadCustomTools` calls on the same path with different `cwd` and
  different `pushPendingAction` callbacks yield distinct tool instances
  whose factories see the per-call bindings.
- Updated `executor-pass-through` and `sdk-preloaded-extensions-isolation`
  tests for the new option name and added a `ToolPathWithSource` fixture.

Refs PR review on #2193
This commit is contained in:
roboomp
2026-06-09 14:09:37 +00:00
parent e7645cec49
commit dde53a8f18
9 changed files with 203 additions and 69 deletions
+1 -1
View File
@@ -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/` (a.k.a. `LoadedCustomTool[]`) results, so each subagent re-ran `loadCapability<Rule>()`, `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)).
- 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<Rule>()`, `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)).
## [15.10.8] - 2026-06-09
@@ -66,8 +66,10 @@ async function loadTool(
}
}
/** Tool path with optional source metadata */
interface ToolPathWithSource {
/** Tool path with optional source metadata, suitable for forwarding from a
* parent session to a subagent so the subagent can re-bind tools to its own
* `CustomToolAPI` without redoing the filesystem scan. */
export interface ToolPathWithSource {
path: string;
source?: { provider: string; providerName: string; level: "user" | "project" };
}
@@ -189,26 +191,19 @@ export async function loadCustomTools(
}
/**
* Discover and load tools from standard locations via capability system:
* 1. User and project tools discovered by capability providers
* 2. Installed plugins (~/.omp/plugins/node_modules/*)
* 3. Explicitly configured paths from settings or CLI
* Collect the absolute tool-source paths to load, without importing or
* binding factories. Hot path on session startup — the scan walks
* `.omp/tools/`, `.claude/tools/`, the plugin tree, and any configured paths.
*
* Subagents reuse the parent's collected paths via the SDK's
* `preloadedCustomToolPaths` option, then call `loadCustomTools` themselves
* so each session re-binds factories with its own session-scoped
* `CustomToolAPI` (cwd, exec, pushPendingAction, UI).
*
* @param configuredPaths - Explicit paths from settings.json and CLI --tool flags
* @param cwd - Current working directory
* @param builtInToolNames - Names of built-in tools to check for conflicts
*/
export async function discoverAndLoadCustomTools(
configuredPaths: string[],
cwd: string,
builtInToolNames: string[],
pushPendingAction?: (action: {
label: string;
sourceToolName: string;
apply(reason: string): Promise<AgentToolResult<unknown>>;
reject?(reason: string): Promise<AgentToolResult<unknown> | undefined>;
}) => void,
) {
export async function discoverCustomToolPaths(configuredPaths: string[], cwd: string): Promise<ToolPathWithSource[]> {
const allPathsWithSources: ToolPathWithSource[] = [];
const seen = new Set<string>();
@@ -241,5 +236,34 @@ export async function discoverAndLoadCustomTools(
addPath(resolvePath(configPath, cwd), { provider: "config", providerName: "Config", level: "project" });
}
return loadCustomTools(allPathsWithSources, cwd, builtInToolNames, pushPendingAction);
return allPathsWithSources;
}
/**
* Discover and load tools from standard locations via capability system:
* 1. User and project tools discovered by capability providers
* 2. Installed plugins (~/.omp/plugins/node_modules/*)
* 3. Explicitly configured paths from settings or CLI
*
* Composed of {@link discoverCustomToolPaths} (FS scan) + {@link loadCustomTools}
* (per-session binding). Subagents skip the first step and just call
* `loadCustomTools` against the parent's collected paths.
*
* @param configuredPaths - Explicit paths from settings.json and CLI --tool flags
* @param cwd - Current working directory
* @param builtInToolNames - Names of built-in tools to check for conflicts
*/
export async function discoverAndLoadCustomTools(
configuredPaths: string[],
cwd: string,
builtInToolNames: string[],
pushPendingAction?: (action: {
label: string;
sourceToolName: string;
apply(reason: string): Promise<AgentToolResult<unknown>>;
reject?(reason: string): Promise<AgentToolResult<unknown> | undefined>;
}) => void,
) {
const pathsWithSources = await discoverCustomToolPaths(configuredPaths, cwd);
return loadCustomTools(pathsWithSources, cwd, builtInToolNames, pushPendingAction);
}
+32 -32
View File
@@ -62,13 +62,8 @@ import {
type LoadedCustomCommand,
loadCustomCommands as loadCustomCommandsInternal,
} from "./extensibility/custom-commands";
import { discoverAndLoadCustomTools } from "./extensibility/custom-tools";
import type {
CustomTool,
CustomToolContext,
CustomToolSessionEvent,
LoadedCustomTool,
} from "./extensibility/custom-tools/types";
import { discoverCustomToolPaths, loadCustomTools, type ToolPathWithSource } from "./extensibility/custom-tools";
import type { CustomTool, CustomToolContext, CustomToolSessionEvent } from "./extensibility/custom-tools/types";
import {
discoverAndLoadExtensions,
type ExtensionContext,
@@ -347,13 +342,17 @@ export interface CreateAgentSessionOptions {
*/
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.
* Pre-discovered custom-tool source paths from `.omp/tools/`, `.claude/tools/`,
* plugins, etc. When provided, the filesystem-scan inside
* `discoverCustomToolPaths()` is skipped — subagents inherit the parent's
* scan result and call `loadCustomTools()` themselves so each session binds
* tools to its OWN `CustomToolAPI` (cwd, exec, pushPendingAction, UI).
*
* Forwarding the loaded `LoadedCustomTool[]` instances directly would reuse
* the parent's session-bound API and route tool execution back through the
* parent — wrong for isolated tasks and for pending-action routing.
*/
preloadedCustomTools?: LoadedCustomTool[];
preloadedCustomToolPaths?: ToolPathWithSource[];
/** Shared event bus for tool/extension communication. Default: creates new bus. */
eventBus?: EventBus;
@@ -1532,27 +1531,28 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
}
// 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.
// Subagents reuse the parent's scan via `preloadedCustomToolPaths` to skip
// the FS walk, but ALWAYS re-call `loadCustomTools` here so factories bind
// to THIS session's `CustomToolAPI` (cwd, exec, pushPendingAction, UI).
// Forwarding the parent's `LoadedCustomTool[]` directly would route tool
// execution back through the parent — wrong for isolated tasks and for
// pending-action queueing.
const builtInToolNames = builtinTools.map(t => t.name);
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));
const customToolPaths: ToolPathWithSource[] =
options.preloadedCustomToolPaths ??
(await logger.time("discoverCustomToolPaths", () => discoverCustomToolPaths([], cwd)));
const customToolsLoadResult = await logger.time("loadCustomTools", () =>
loadCustomTools(customToolPaths, cwd, builtInToolNames, action => queueResolveHandler(toolSession, action)),
);
for (const { path, error } of customToolsLoadResult.errors) {
logger.error("Custom tool load failed", { path, error });
}
// Forward the discovered tools to subagents so they skip the FS scan.
toolSession.loadedCustomTools = loadedCustomTools;
if (customToolsLoadResult.tools.length > 0) {
customTools.push(...customToolsLoadResult.tools.map(loaded => loaded.tool));
}
// Forward the path list (NOT the loaded tools) to subagents so they
// re-bind under their own `CustomToolAPI` while skipping the FS scan.
toolSession.customToolPaths = customToolPaths;
const inlineExtensions: ExtensionFactory[] = options.extensions ? [...options.extensions] : [];
inlineExtensions.push((await import("./autoresearch")).createAutoresearchExtension);
+9 -4
View File
@@ -14,7 +14,8 @@ 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, LoadedCustomTool } from "../extensibility/custom-tools/types";
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";
@@ -196,8 +197,12 @@ export interface ExecutorOptions {
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[];
/**
* Parent's discovered custom-tool source paths. Forwarded to skip the
* `.omp/tools/` FS scan in the subagent; the subagent then re-binds each
* tool against its own `CustomToolAPI` (cwd, exec, pushPendingAction, UI).
*/
preloadedCustomToolPaths?: ToolPathWithSource[];
mcpManager?: MCPManager;
authStorage?: AuthStorage;
modelRegistry?: ModelRegistry;
@@ -1294,7 +1299,7 @@ export async function runSubprocess(options: ExecutorOptions): Promise<SingleRes
workspaceTree: options.workspaceTree,
rules: options.rules,
preloadedExtensions: options.preloadedExtensions,
preloadedCustomTools: options.preloadedCustomTools,
preloadedCustomToolPaths: options.preloadedCustomToolPaths,
systemPrompt: defaultPrompt => {
const subagentPrompt = prompt.render(subagentSystemPromptTemplate, {
agent: agent.systemPrompt,
+1 -2
View File
@@ -992,7 +992,7 @@ export class TaskTool implements AgentTool<TaskToolSchemaInstance, TaskToolDetai
promptTemplates,
rules: this.session.rules,
preloadedExtensions: this.session.extensionsResult,
preloadedCustomTools: this.session.loadedCustomTools,
preloadedCustomToolPaths: this.session.customToolPaths,
localProtocolOptions,
parentArtifactManager,
parentHindsightSessionState: this.session.getHindsightSessionState?.(),
@@ -1053,7 +1053,6 @@ export class TaskTool implements AgentTool<TaskToolSchemaInstance, TaskToolDetai
promptTemplates,
rules: this.session.rules,
preloadedExtensions: this.session.extensionsResult,
preloadedCustomTools: this.session.loadedCustomTools,
localProtocolOptions,
parentArtifactManager,
parentHindsightSessionState: this.session.getHindsightSessionState?.(),
+7 -3
View File
@@ -8,7 +8,7 @@ import type { PromptTemplate } from "../config/prompt-templates";
import type { Settings } from "../config/settings";
import { EditTool } from "../edit";
import { checkPythonKernelAvailability } from "../eval/py/kernel";
import type { LoadedCustomTool } from "../extensibility/custom-tools/types";
import type { ToolPathWithSource } from "../extensibility/custom-tools";
import type { LoadExtensionsResult } from "../extensibility/extensions/types";
import type { Skill } from "../extensibility/skills";
import type { GoalModeState, GoalRuntime } from "../goals";
@@ -161,8 +161,12 @@ export interface ToolSession {
rules?: Rule[];
/** Pre-loaded extensions result (forwarded to subagents via `preloadedExtensions`). */
extensionsResult?: LoadExtensionsResult;
/** Pre-loaded custom tools discovered from `.omp/tools/`, `.claude/tools/`, etc. (forwarded to subagents). */
loadedCustomTools?: LoadedCustomTool[];
/**
* Pre-discovered custom-tool source paths from `.omp/tools/`, `.claude/tools/`,
* plugins, etc. Forwarded to subagents so they skip the FS scan but still
* re-bind tools to their own session-scoped `CustomToolAPI`.
*/
customToolPaths?: ToolPathWithSource[];
/** Whether LSP integrations are enabled */
enableLsp?: boolean;
/** Whether an edit-capable tool is available in this session (controls hashline output) */
@@ -0,0 +1,102 @@
/**
* Regression guard for PR review feedback on #2190.
*
* Subagents inherit the parent's custom-tool source *paths* (a cheap FS scan
* the parent already paid for), but each session MUST rebuild its own
* `LoadedCustomTool[]` so factories see the subagent's `CustomToolAPI`
* (cwd, exec, pushPendingAction, UI). Forwarding the parent's loaded tool
* instances would route execution and pending actions back to the parent —
* wrong for isolated tasks and for queue routing.
*
* This file does not exercise the live SDK end-to-end (that path requires
* a real worker spawn and is covered by the broader test suite); it pins
* down the loader contract that the SDK now depends on.
*/
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 {
type CustomToolAPI,
loadCustomTools,
type ToolPathWithSource,
} from "@oh-my-pi/pi-coding-agent/extensibility/custom-tools";
describe("loadCustomTools per-session binding (#2190 review fix)", () => {
let tmp: string;
let toolPath: string;
beforeAll(async () => {
tmp = await fs.mkdtemp(path.join(os.tmpdir(), "pi-custom-tool-binding-"));
toolPath = path.join(tmp, "echo-cwd.ts");
// Factory exposes the API it was bound to so the test can inspect it.
await fs.writeFile(
toolPath,
[
"export default function (api) {",
" return {",
" name: 'echo_cwd_' + api.cwd.replace(/[^a-z0-9]/gi, '_'),",
" description: 'returns the cwd the factory was bound to',",
" params: api.typebox.Type.Object({}),",
" async execute() { return { content: [{ type: 'text', text: api.cwd }] }; },",
" __boundApi: api,",
" };",
"}",
].join("\n"),
);
});
afterAll(async () => {
await fs.rm(tmp, { recursive: true, force: true });
});
it("binds each load to the cwd passed to loadCustomTools", async () => {
const paths: ToolPathWithSource[] = [{ path: toolPath }];
const parentResult = await loadCustomTools(paths, "/tmp/parent-cwd", []);
const subagentResult = await loadCustomTools(paths, "/tmp/subagent-cwd", []);
expect(parentResult.errors).toEqual([]);
expect(subagentResult.errors).toEqual([]);
expect(parentResult.tools).toHaveLength(1);
expect(subagentResult.tools).toHaveLength(1);
const parentApi = (parentResult.tools[0]?.tool as unknown as { __boundApi: CustomToolAPI }).__boundApi;
const subagentApi = (subagentResult.tools[0]?.tool as unknown as { __boundApi: CustomToolAPI }).__boundApi;
expect(parentApi.cwd).toBe("/tmp/parent-cwd");
expect(subagentApi.cwd).toBe("/tmp/subagent-cwd");
expect(subagentApi).not.toBe(parentApi);
// Different tool instances — a session must never see the other's tool.
expect(subagentResult.tools[0]?.tool).not.toBe(parentResult.tools[0]?.tool);
});
it("routes pushPendingAction to the loader's own callback, not a shared one", async () => {
const parentLog: string[] = [];
const subagentLog: string[] = [];
const parentResult = await loadCustomTools([{ path: toolPath }], "/tmp/parent-cwd", [], action =>
parentLog.push(`parent:${action.label}`),
);
const subagentResult = await loadCustomTools([{ path: toolPath }], "/tmp/subagent-cwd", [], action =>
subagentLog.push(`subagent:${action.label}`),
);
const parentApi = (parentResult.tools[0]?.tool as unknown as { __boundApi: CustomToolAPI }).__boundApi;
const subagentApi = (subagentResult.tools[0]?.tool as unknown as { __boundApi: CustomToolAPI }).__boundApi;
// Cast: the test fixture exposes the runtime API verbatim.
parentApi.pushPendingAction({
label: "ping",
sourceToolName: "echo",
apply: async () => ({ content: [] }),
});
subagentApi.pushPendingAction({
label: "ping",
sourceToolName: "echo",
apply: async () => ({ content: [] }),
});
expect(parentLog).toEqual(["parent:ping"]);
expect(subagentLog).toEqual(["subagent:ping"]);
});
});
@@ -62,7 +62,7 @@ describe("createAgentSession preloadedExtensions isolation (issue #2190)", () =>
skipPythonPreflight: true,
skills: [],
rules: [],
preloadedCustomTools: [],
preloadedCustomToolPaths: [],
contextFiles: [],
promptTemplates: [],
});
@@ -7,7 +7,7 @@ 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 { ToolPathWithSource } from "@oh-my-pi/pi-coding-agent/extensibility/custom-tools";
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";
@@ -94,7 +94,7 @@ describe("runSubprocess parent-discovery pass-through (issue #2190)", () => {
vi.restoreAllMocks();
});
it("forwards rules, preloadedExtensions, and preloadedCustomTools to createAgentSession", async () => {
it("forwards rules, preloadedExtensions, and preloadedCustomToolPaths to createAgentSession", async () => {
const session = yieldEmittingSession();
const spy = vi.spyOn(sdkModule, "createAgentSession").mockResolvedValue(createSessionResult(session));
@@ -104,15 +104,15 @@ describe("runSubprocess parent-discovery pass-through (issue #2190)", () => {
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 preloadedCustomToolPaths: ToolPathWithSource[] = [
{ path: "tools/x.ts", source: { provider: "config", providerName: "Config", level: "project" } },
];
const result = await runSubprocess({
...baseOptions,
rules,
preloadedExtensions,
preloadedCustomTools,
preloadedCustomToolPaths,
});
expect(result.exitCode).toBe(0);
@@ -121,7 +121,7 @@ describe("runSubprocess parent-discovery pass-through (issue #2190)", () => {
// 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);
expect(forwarded?.preloadedCustomToolPaths).toBe(preloadedCustomToolPaths);
});
it("forwards undefined when the parent has not pre-discovered state", async () => {
@@ -134,6 +134,6 @@ describe("runSubprocess parent-discovery pass-through (issue #2190)", () => {
const forwarded = spy.mock.calls[0]?.[0];
expect(forwarded?.rules).toBeUndefined();
expect(forwarded?.preloadedExtensions).toBeUndefined();
expect(forwarded?.preloadedCustomTools).toBeUndefined();
expect(forwarded?.preloadedCustomToolPaths).toBeUndefined();
});
});