diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6d01454c3..ee3028ed2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed custom tool discovery treating `process.exit()` from an imported tool module as a host process exit instead of a recoverable tool load failure ([#1704](https://github.com/can1357/oh-my-pi/issues/1704)). +- Fixed custom tool loading treating `process.exit()` from a tool module's import or factory as a host process exit instead of a recoverable load failure. Custom tools now load under the shared extension exit guard, so an exiting tool is skipped with a load error while remaining tools still load ([#1704](https://github.com/can1357/oh-my-pi/issues/1704)). ## [15.8.0] - 2026-06-02 diff --git a/packages/coding-agent/src/extensibility/custom-tools/loader.ts b/packages/coding-agent/src/extensibility/custom-tools/loader.ts index e819db60a..9d99a2b78 100644 --- a/packages/coding-agent/src/extensibility/custom-tools/loader.ts +++ b/packages/coding-agent/src/extensibility/custom-tools/loader.ts @@ -15,67 +15,9 @@ import { execCommand } from "../../exec/exec"; import type { HookUIContext } from "../../extensibility/hooks/types"; import { getAllPluginToolPaths } from "../../extensibility/plugins/loader"; import * as typebox from "../typebox"; -import { createNoOpUIContext, resolvePath } from "../utils"; +import { createNoOpUIContext, resolvePath, withExitGuard } from "../utils"; import type { CustomToolAPI, CustomToolFactory, LoadedCustomTool, ToolLoadError } from "./types"; -/** - * Depth counter for the surrounding `loadCustomTools` batch. While greater - * than zero, any `process.exit()` call is logged and dropped instead of - * terminating the host. The batch stays open through every awaited - * `loadTool` and through the microtask drain that happens at each `await` - * point, so deferred exits scheduled by a tool's import or factory (for - * example `void main().catch(() => process.exit(1))`) fire while the guard - * is still active. - */ -let activeBatchDepth = 0; -/** Captured reference to the original `process.exit`, set when the guard is first installed. */ -let originalProcessExit: typeof process.exit | null = null; - -/** - * Install a permanent `process.exit` interceptor on the first custom tool load. - * - * The interceptor logs and drops any `process.exit` call made while a batch is - * open (`activeBatchDepth > 0`). Outside the batch window the call delegates - * to the original `process.exit` so omp's own shutdown paths behave normally. - * - * The guard is intentionally never restored: tool-scheduled microtask work - * runs across `await` points inside the batch (Bun's dynamic import body runs - * on a separate microtask), and once installed the wrapper costs one extra - * function call per exit. It does *not* attempt to catch exits scheduled by - * `setTimeout`/`queueMicrotask` that fire after the batch has resolved — - * those require process isolation, which is out of scope here. - */ -function installExitGuardOnce(): void { - if (originalProcessExit !== null) return; - const captured = process.exit; - originalProcessExit = captured; - const guarded: typeof process.exit = code => { - if (activeBatchDepth > 0) { - logger.error("Custom tool attempted to exit the process during load; call ignored", { code }); - return undefined as never; - } - return captured(code); - }; - process.exit = guarded; -} - -/** - * Run a `loadCustomTools` batch under the exit guard. Sync exits from a - * tool's import body or factory and async microtask follow-ups scheduled - * during the batch are both intercepted while the batch is open, because the - * microtask drain at each `await` point happens before the surrounding batch - * promise resolves. - */ -async function withCustomToolBatch(operation: () => Promise): Promise { - installExitGuardOnce(); - activeBatchDepth++; - try { - return await operation(); - } finally { - activeBatchDepth--; - } -} - /** * Load a single tool module using native Bun import. */ @@ -100,14 +42,14 @@ async function loadTool( } try { - const module = await import(resolvedPath); + const module = await withExitGuard(() => import(resolvedPath)); const factory = (module.default ?? module) as CustomToolFactory; if (typeof factory !== "function") { return { tools: null, error: { path: toolPath, error: "Tool must export a default function", source } }; } - const toolResult = await factory(sharedApi); + const toolResult = await withExitGuard(async () => factory(sharedApi)); const toolsArray = Array.isArray(toolResult) ? toolResult : [toolResult]; const loadedTools: LoadedCustomTool[] = toolsArray.map(tool => ({ @@ -236,7 +178,7 @@ export async function loadCustomTools( builtInToolNames, pushPendingAction, ); - await withCustomToolBatch(() => loader.load(pathsWithSources)); + await loader.load(pathsWithSources); return { tools: loader.tools, errors: loader.errors, diff --git a/packages/coding-agent/test/extensibility/custom-tool-loader.test.ts b/packages/coding-agent/test/extensibility/custom-tool-loader.test.ts index 9ceabf091..6975ad17e 100644 --- a/packages/coding-agent/test/extensibility/custom-tool-loader.test.ts +++ b/packages/coding-agent/test/extensibility/custom-tool-loader.test.ts @@ -1,14 +1,12 @@ -import { afterEach, describe, expect, it, vi } from "bun:test"; +import { afterEach, 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 { logger } from "@oh-my-pi/pi-utils"; import { loadCustomTools } from "../../src/extensibility/custom-tools/loader"; let tempRoot: string | undefined; afterEach(async () => { - vi.restoreAllMocks(); if (tempRoot) { await fs.rm(tempRoot, { recursive: true, force: true }); tempRoot = undefined; @@ -27,10 +25,23 @@ function requireTempRoot(): string { return tempRoot; } -describe("custom tool loader", () => { - it("survives a tool that calls process.exit synchronously at import time and still loads later valid tools", async () => { - const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); +const VALID_TOOL_SOURCE = [ + "export default api => ({", + '\tname: "safe_custom_tool",', + '\tlabel: "Safe Custom Tool",', + '\tdescription: "Returns a fixed response",', + "\tparameters: api.zod.object({}),", + "\tasync execute() {", + '\t\treturn { content: [{ type: "text", text: "ok" }] };', + "\t},", + "});", +].join("\n"); +describe("custom tool loader", () => { + it("skips a tool that calls process.exit synchronously at import time and still loads later valid tools", async () => { + // CLI-shaped module: main() at the bottom, exit on failure (issue #1704). + // Without the exit guard this terminates the test process before the + // assertions run. const exitingTool = await writeTool( "sync-exit.js", [ @@ -44,82 +55,28 @@ describe("custom tool loader", () => { "main();", ].join("\n"), ); - const validTool = await writeTool( - "valid.js", - [ - "export default api => ({", - '\tname: "safe_custom_tool",', - '\tlabel: "Safe Custom Tool",', - '\tdescription: "Returns a fixed response",', - "\tparameters: api.zod.object({}),", - "\tasync execute() {", - '\t\treturn { content: [{ type: "text", text: "ok" }] };', - "\t},", - "});", - ].join("\n"), - ); + const validTool = await writeTool("valid.js", VALID_TOOL_SOURCE); const result = await loadCustomTools([{ path: exitingTool }, { path: validTool }], requireTempRoot(), []); expect(result.tools.map(tool => tool.tool.name)).toEqual(["safe_custom_tool"]); expect(result.errors).toHaveLength(1); expect(result.errors[0]?.path).toBe(exitingTool); - - const loggedExit = errorSpy.mock.calls.find( - call => - typeof call[0] === "string" && call[0].startsWith("Custom tool attempted to exit the process during load"), - ); - expect(loggedExit).toBeDefined(); - expect(loggedExit?.[1]).toMatchObject({ code: 1 }); + expect(result.errors[0]?.error).toContain("process.exit(1)"); }); - it("survives a tool that schedules a deferred process.exit via void promise.catch and still registers it", async () => { - const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); + it("skips a tool whose factory calls process.exit and still loads later valid tools", async () => { + const factoryExitTool = await writeTool( + "factory-exit.js", + ["export default () => {", "\tprocess.exit(3);", "};"].join("\n"), + ); + const validTool = await writeTool("valid.js", VALID_TOOL_SOURCE); - const signalKey = `__omp1704_async_signal_${Date.now()}_${Math.random().toString(36).slice(2)}`; - const { promise, resolve } = Promise.withResolvers(); - (globalThis as Record)[signalKey] = resolve; + const result = await loadCustomTools([{ path: factoryExitTool }, { path: validTool }], requireTempRoot(), []); - try { - const asyncTool = await writeTool( - "async-exit.js", - [ - `const signal = globalThis[${JSON.stringify(signalKey)}];`, - "async function main() { throw new Error('startup failure'); }", - "void main().catch(() => {", - "\tprocess.exit(7);", - "\tsignal('survived');", - "});", - "export default api => ({", - '\tname: "async_exit_tool",', - '\tlabel: "Async Exit Tool",', - '\tdescription: "Schedules a deferred exit during import",', - "\tparameters: api.zod.object({}),", - "\tasync execute() {", - '\t\treturn { content: [{ type: "text", text: "ok" }] };', - "\t},", - "});", - ].join("\n"), - ); - - const result = await loadCustomTools([{ path: asyncTool }], requireTempRoot(), []); - - // The tool's deferred exit attempt fires after the import promise settles. - // If the guard did not intercept it, the test process would die before this resolves. - const outcome = await promise; - expect(outcome).toBe("survived"); - expect(result.tools.map(tool => tool.tool.name)).toEqual(["async_exit_tool"]); - expect(result.errors).toEqual([]); - - const loggedExit = errorSpy.mock.calls.find( - call => - typeof call[0] === "string" && - call[0].startsWith("Custom tool attempted to exit the process during load"), - ); - expect(loggedExit).toBeDefined(); - expect(loggedExit?.[1]).toMatchObject({ code: 7 }); - } finally { - delete (globalThis as Record)[signalKey]; - } + expect(result.tools.map(tool => tool.tool.name)).toEqual(["safe_custom_tool"]); + expect(result.errors).toHaveLength(1); + expect(result.errors[0]?.path).toBe(factoryExitTool); + expect(result.errors[0]?.error).toContain("process.exit(3)"); }); });