fix(coding-agent): guard custom tool loads with the shared exit guard
Replaces the bespoke batch-scoped process.exit interceptor with the
withExitGuard convention main established for extension/hook/plugin
loaders (500c39aa2): guard the module import and factory invocation so
a synchronous process.exit()/process.reallyExit() from a custom tool
becomes an ExtensionExitError handled as a recoverable load error,
while host exit paths stay untouched outside the guarded windows.
Tests cover the import-time exit (issue #1704 repro) and factory-time
exit; both would kill the test process without the guard.
This commit is contained in:
PR Evaluator
@@ -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
|
||||
|
||||
|
||||
@@ -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<T>(operation: () => Promise<T>): Promise<T> {
|
||||
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,
|
||||
|
||||
@@ -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<unknown>();
|
||||
(globalThis as Record<string, unknown>)[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<string, unknown>)[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)");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user