Files
oh-my-pi/packages/coding-agent/test/sdk-custom-tools-per-session-binding.test.ts
T
roboomp dde53a8f18 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
2026-06-09 14:09:37 +00:00

103 lines
3.9 KiB
TypeScript

/**
* 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"]);
});
});