roboomp
fe34d0f392
Same shape of bug the reviewer flagged for custom tools: forwarding
`LoadExtensionsResult` from parent to subagent reused Extension instances
whose factories closed over the parent's `ExtensionAPI` — cwd, eventBus,
and runtime all pointed at the parent. Any tool/handler/command that
referenced `api.exec()`, `api.events`, or `api.runtime` still acted on the
parent session/worktree from inside an isolated subagent.
Forward only the path list; each session rebuilds extensions through
`loadExtensions` so factories see the right `ExtensionAPI`.
- `extensibility/extensions/loader.ts`: extract `discoverExtensionPaths`
(FS scan only) from `discoverAndLoadExtensions`. The combined helper now
composes the two. New export added to the package barrel.
- `sdk.ts`:
- Add `discoverSessionExtensionPaths()` (the `disableExtensionDiscovery`-aware
path-only counterpart of `loadSessionExtensions`).
- Add `preloadedExtensionPaths?: string[]` to `CreateAgentSessionOptions`.
Three loader branches: `preloadedExtensions` (CLI same-process reuse,
still shallow-cloned), `preloadedExtensionPaths` (subagent: skip scan,
reload locally), or full discovery.
- Document `preloadedExtensions` as same-process-only; subagent
forwarding MUST use `preloadedExtensionPaths`.
- `tools/index.ts`: `ToolSession.extensionsResult` → `extensionPaths:
string[]` for the same reason.
- `task/executor.ts` and `task/index.ts`: forward `extensionPaths`. Drop
the forward for the isolated `runSubprocess` branch — worktree cwd ≠
parent cwd, so the subagent re-discovers extensions against its own
tree.
- New `test/sdk-extensions-per-session-binding.test.ts` pins the contract:
two `loadExtensions` calls on the same path with different `cwd` and
different `EventBus` instances yield distinct Extension + runtime
objects whose factories close over the per-call bindings.
- Updated `executor-pass-through` and `sdk-preloaded-extensions-isolation`
tests for the new option name and comment context.
Refs PR review on #2193
86 lines
3.5 KiB
TypeScript
86 lines
3.5 KiB
TypeScript
/**
|
|
* 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);
|
|
});
|
|
});
|