Merge pull request #1112 from corbitsdev/refactor/tools-shell-dedup

refactor(tools-shell): dedupe shared web-fetch, convert, signal, and timer helpers

Author: TheGreatAxios

src/exec/runner.ts

@@ -916,7 +916,7 @@ export async function runExec(config: Config): Promise<ExecResult> {
         handshake,
       ).catch((err: unknown) => {
         logger.warn("MCP connect failed: {error}", {
-          error: err instanceof Error ? err.message : String(err),
+          error: formatCaughtError(err),
         });
       });
       await awaitExecMcpThenResume(connecting, () => workflowHost.resume(), {

src/shell/background-shell.ts

@@ -155,6 +155,9 @@ export function createBackgroundShellRegistry(
     child.stdout?.on("data", (chunk: Buffer) => collector.append(chunk));
     child.stderr?.on("data", (chunk: Buffer) => collector.append(chunk));
     let timer: ReturnType<typeof setTimeout> | undefined;
+    // Clearing an unset timer is a no-op, so one unguarded closure covers
+    // every settle path below.
+    const clearTimer = (): void => clearTimeout(timer);
     // Per-call timeout only — background has no 120s default. Cancel reuses
     // killProcessTree (same path as shell_collect action=cancel).
     if (args.timeoutMs !== undefined && args.timeoutMs > 0) {
@@ -164,11 +167,11 @@ export function createBackgroundShellRegistry(
       }, args.timeoutMs);
     }
     child.on("error", () => {
-      if (timer !== undefined) clearTimeout(timer);
+      clearTimer();
       finish(1, false);
     });
     child.on("exit", (code, sig) => {
-      if (timer !== undefined) clearTimeout(timer);
+      clearTimer();
       if (!running.has(id)) return;
       const exitCode = code ?? (sig !== null ? 128 : 1);
       const settle = (): void => finish(exitCode, false);

src/tools/html-convert.ts

@@ -25,20 +25,27 @@ function stripTagsAndScripts(html: string): string {
     .replace(/<!--[\s\S]*?-->/g, "");
 }
 
-export function htmlToText(html: string): string {
-  const cleaned = stripTagsAndScripts(html)
-    .replace(/<br\s*\/?>/gi, "\n")
-    .replace(/<\/(p|div|li|h[1-6]|tr)>/gi, "\n")
-    .replace(/<[^>]+>/g, "");
-  return decodeEntities(cleaned)
+function normalizeWhitespace(
+  text: string,
+  mapLine: (line: string) => string,
+): string {
+  return text
     .replace(/[ \t]+/g, " ")
     .replace(/\n{3,}/g, "\n\n")
     .split("\n")
-    .map((line) => line.trim())
+    .map(mapLine)
     .join("\n")
     .trim();
 }
 
+export function htmlToText(html: string): string {
+  const cleaned = stripTagsAndScripts(html)
+    .replace(/<br\s*\/?>/gi, "\n")
+    .replace(/<\/(p|div|li|h[1-6]|tr)>/gi, "\n")
+    .replace(/<[^>]+>/g, "");
+  return normalizeWhitespace(decodeEntities(cleaned), (line) => line.trim());
+}
+
 // Best-effort structural markdown: headings, bold/italic, links, list items.
 // Anything not recognized falls through to plain text extraction.
 export function htmlToMarkdown(html: string): string {
@@ -76,11 +83,5 @@ export function htmlToMarkdown(html: string): string {
     .replace(/<\/(p|div|tr|table|ul|ol)>/gi, "\n")
     .replace(/<br\s*\/?>/gi, "\n");
   const text = decodeEntities(working.replace(/<[^>]+>/g, ""));
-  return text
-    .replace(/[ \t]+/g, " ")
-    .replace(/\n{3,}/g, "\n\n")
-    .split("\n")
-    .map((line) => line.replace(/[ \t]+$/g, ""))
-    .join("\n")
-    .trim();
+  return normalizeWhitespace(text, (line) => line.replace(/[ \t]+$/g, ""));
 }

src/tools/web-fetch.test.ts

@@ -12,6 +12,9 @@ import {
 
 let server: Server;
 let baseUrl: string;
+// Shared by every tool invocation below: none of these tests abort, so one
+// never-aborted signal covers the whole file.
+const neverAbortedSignal = new AbortController().signal;
 let handler: (
   req: import("node:http").IncomingMessage,
   res: import("node:http").ServerResponse,
@@ -194,7 +197,7 @@ describe("createExaMCPWebFetchTool", () => {
         name: "web_fetch",
         arguments: { url: "https://example.com", timeout: 1 },
       },
-      new AbortController().signal,
+      neverAbortedSignal,
     );
 
     expect(result).toEqual({
@@ -214,11 +217,8 @@ describe("createExaMCPWebFetchTool", () => {
       arguments: { url: "ftp://example.com/file" },
     };
 
-    const nativeResult = await nativeRunner.run(
-      call,
-      new AbortController().signal,
-    );
-    const exaResult = await exaRunner.run(call, new AbortController().signal);
+    const nativeResult = await nativeRunner.run(call, neverAbortedSignal);
+    const exaResult = await exaRunner.run(call, neverAbortedSignal);
 
     expect(nativeResult).toEqual({
       callId: "protocol-call",
@@ -249,8 +249,8 @@ describe("createExaMCPWebFetchTool", () => {
     };
 
     const [nativeResult, exaResult] = await Promise.all([
-      nativeRunner.run(call, new AbortController().signal),
-      exaRunner.run(call, new AbortController().signal),
+      nativeRunner.run(call, neverAbortedSignal),
+      exaRunner.run(call, neverAbortedSignal),
     ]);
 
     expect(nativeResult).toEqual({
@@ -273,7 +273,7 @@ describe("createExaMCPWebFetchTool", () => {
           name: "web_fetch",
           arguments: { url: `${baseUrl}/`, format },
         },
-        new AbortController().signal,
+        neverAbortedSignal,
       );
       if (typeof result === "string")
         throw new Error("expected a full tool result");

src/tools/web-fetch.ts

@@ -43,6 +43,37 @@ const WebFetchArgs = type({
 
 type WebFetchFormat = "text" | "markdown" | "html";
 
+type ParsedWebFetchArgs =
+  | { ok: true; url: string; format: WebFetchFormat; timeout: number }
+  | { ok: false; error: string };
+
+function parseWebFetchArgs(
+  rawArgs: Record<string, unknown>,
+): ParsedWebFetchArgs {
+  const parsed = WebFetchArgs(rawArgs);
+  if (parsed instanceof type.errors) {
+    return {
+      ok: false,
+      error:
+        "Error: web_fetch requires a non-empty url (http/https); format and timeout are optional.",
+    };
+  }
+  return {
+    ok: true,
+    url: parsed.url,
+    format: parsed.format ?? "markdown",
+    timeout: parsed.timeout ?? DEFAULT_TIMEOUT_S,
+  };
+}
+
+function truncationSuffix(truncated: boolean): string {
+  return truncated ? `\n\n[content truncated at ${MAX_FETCH_BYTES} bytes]` : "";
+}
+
+function fetchTimeoutMessage(url: string, seconds: number): string {
+  return `Request to ${url} timed out after ${seconds}s. Retry with a larger timeout parameter (up to 120s) if the site is slow.`;
+}
+
 export const webFetchDefinition: ToolDefinition = {
   name: "web_fetch",
   description:
@@ -166,7 +197,7 @@ export async function runWebFetch(
       if (err instanceof Error && err.name === "AbortError") {
         return {
           ok: false,
-          error: `Request to ${currentUrl} timed out after ${clampedSeconds}s. Retry with a larger timeout parameter (up to 120s) if the site is slow.`,
+          error: fetchTimeoutMessage(currentUrl, clampedSeconds),
         };
       }
       return {
@@ -233,18 +264,16 @@ export function createWebFetchTool(options?: WebFetchTimingOptions): AgentTool {
   return stringTool({
     definition: webFetchDefinition,
     handler: async (rawArgs: Record<string, unknown>): Promise<string> => {
-      const parsed = WebFetchArgs(rawArgs);
-      if (parsed instanceof type.errors) {
-        return "Error: web_fetch requires a non-empty url (http/https); format and timeout are optional.";
-      }
-      const format = parsed.format ?? "markdown";
-      const timeout = parsed.timeout ?? DEFAULT_TIMEOUT_S;
-      const outcome = await runWebFetch(parsed.url, format, timeout, options);
+      const parsed = parseWebFetchArgs(rawArgs);
+      if (!parsed.ok) return parsed.error;
+      const outcome = await runWebFetch(
+        parsed.url,
+        parsed.format,
+        parsed.timeout,
+        options,
+      );
       if (!outcome.ok) return `Error: ${outcome.error}`;
-      const suffix = outcome.truncated
-        ? `\n\n[content truncated at ${MAX_FETCH_BYTES} bytes]`
-        : "";
-      return outcome.content + suffix;
+      return outcome.content + truncationSuffix(outcome.truncated);
     },
   });
 }
@@ -266,13 +295,9 @@ export function createExaMCPWebFetchTool(
       call: ToolCall,
       signal: AbortSignal,
     ): Promise<ToolResult> => {
-      const parsed = WebFetchArgs(call.arguments);
-      if (parsed instanceof type.errors) {
-        return {
-          callId: call.id,
-          content:
-            "Error: web_fetch requires a non-empty url (http/https); format and timeout are optional.",
-        };
+      const parsed = parseWebFetchArgs(call.arguments);
+      if (!parsed.ok) {
+        return { callId: call.id, content: parsed.error };
       }
 
       let url: URL;
@@ -291,17 +316,17 @@ export function createExaMCPWebFetchTool(
         };
       }
 
-      const format = parsed.format ?? "markdown";
-      const timeout = parsed.timeout ?? DEFAULT_TIMEOUT_S;
+      const format = parsed.format;
+      const timeout = parsed.timeout;
       if (format !== "markdown") {
         const outcome = await runWebFetch(parsed.url, format, timeout);
         if (!outcome.ok) {
           return { callId: call.id, content: `Error: ${outcome.error}` };
         }
-        const suffix = outcome.truncated
-          ? `\n\n[content truncated at ${MAX_FETCH_BYTES} bytes]`
-          : "";
-        return { callId: call.id, content: outcome.content + suffix };
+        return {
+          callId: call.id,
+          content: outcome.content + truncationSuffix(outcome.truncated),
+        };
       }
 
       const timeoutSeconds = Math.min(Math.max(timeout, 1), MAX_TIMEOUT_S);
@@ -352,7 +377,10 @@ export function createExaMCPWebFetchTool(
         if (err === timeoutError) {
           return {
             callId: call.id,
-            content: `Error: Request to ${parsed.url} timed out after ${timeoutSeconds}s. Retry with a larger timeout parameter (up to 120s) if the site is slow.`,
+            content: `Error: ${fetchTimeoutMessage(
+              parsed.url,
+              timeoutSeconds,
+            )}`,
           };
         }
         return {

src/tools/web-search.test.ts

@@ -44,6 +44,10 @@ const { createPermissionGate } = await import("../permission/gate.js");
 
 const BUILTIN_EXA_MCP = createExaMCPServerConfig();
 
+// Shared by every tool invocation below: none of these tests abort, so one
+// never-aborted signal covers the whole file.
+const neverAbortedSignal = new AbortController().signal;
+
 async function connectConfiguredMCP(
   mcpServers?: ResolvedMCPServerConfig[],
 ): Promise<void> {
@@ -123,7 +127,7 @@ describe("createWebSearchTool", () => {
     if (tool.kind !== "string") throw new Error("expected a string tool");
     const result = await tool.handler(
       { query: "corbits code" },
-      new AbortController().signal,
+      neverAbortedSignal,
     );
     expect(result).toBe("mock result");
     expect(connectConfigs[0]?.url).toBe(EXA_MCP_URL);
@@ -148,7 +152,7 @@ describe("createWebSearchTool", () => {
         livecrawl: "preferred",
         contextMaxCharacters: 500,
       },
-      new AbortController().signal,
+      neverAbortedSignal,
     );
     expect(calls[0]?.args).toEqual({
       query: "q",
@@ -163,18 +167,15 @@ describe("createWebSearchTool", () => {
     process.env.CORBITS_WEB_SEARCH_PROVIDER = "parallel";
     const tool = createWebSearchTool();
     if (tool.kind !== "string") throw new Error("expected a string tool");
-    await tool.handler({ query: "corbits code" }, new AbortController().signal);
+    await tool.handler({ query: "corbits code" }, neverAbortedSignal);
     expect(connectConfigs[0]?.url).toBe(PARALLEL_MCP_URL);
     expect(calls[0]?.toolName).toBe("web_search");
   });
 
   test("rejects an empty query before ever connecting", async () => {
     const tool = createWebSearchTool();
     if (tool.kind !== "string") throw new Error("expected a string tool");
-    const result = await tool.handler(
-      { query: "" },
-      new AbortController().signal,
-    );
+    const result = await tool.handler({ query: "" }, neverAbortedSignal);
     expect(result).toContain("Error");
     expect(connectConfigs.length).toBe(0);
   });