combine sync and async shell modes into one.

Author: bhouston

src/tools/system/shellMessage.test.ts

@@ -3,6 +3,16 @@ import { processStates, shellStartTool } from "./shellStart.js";
 import { MockLogger } from "../../utils/mockLogger.js";
 import { shellMessageTool } from "./shellMessage.js";
 
+// Helper function to get instanceId from shellStart result
+const getInstanceId = (
+  result: Awaited<ReturnType<typeof shellStartTool.execute>>
+) => {
+  if (result.mode === "async") {
+    return result.instanceId;
+  }
+  throw new Error("Expected async mode result");
+};
+
 // eslint-disable-next-line max-lines-per-function
 describe("shellMessageTool", () => {
   const mockLogger = new MockLogger();
@@ -21,16 +31,17 @@ describe("shellMessageTool", () => {
   });
 
   it("should interact with a running process", async () => {
-    // Start a test process
+    // Start a test process - force async mode with timeout
     const startResult = await shellStartTool.execute(
       {
         command: "cat", // cat will echo back input
         description: "Test interactive process",
+        timeout: 50, // Force async mode for interactive process
       },
       { logger: mockLogger }
     );
 
-    testInstanceId = startResult.instanceId;
+    testInstanceId = getInstanceId(startResult);
 
     // Send input and get response
     const result = await shellMessageTool.execute(
@@ -61,29 +72,32 @@ describe("shellMessageTool", () => {
   });
 
   it("should handle process completion", async () => {
-    // Start a quick process
+    // Start a quick process - force async mode
     const startResult = await shellStartTool.execute(
       {
-        command: 'echo "test" && exit',
+        command: 'echo "test" && sleep 0.1',
         description: "Test completion",
+        timeout: 0, // Force async mode
       },
       { logger: mockLogger }
     );
 
+    const instanceId = getInstanceId(startResult);
+
     // Wait a moment for process to complete
-    await new Promise((resolve) => setTimeout(resolve, 100));
+    await new Promise((resolve) => setTimeout(resolve, 150));
 
     const result = await shellMessageTool.execute(
       {
-        instanceId: startResult.instanceId,
+        instanceId,
         description: "Check completion",
       },
       { logger: mockLogger }
     );
 
     expect(result.completed).toBe(true);
     // Process should still be in processStates even after completion
-    expect(processStates.has(startResult.instanceId)).toBe(true);
+    expect(processStates.has(instanceId)).toBe(true);
   });
 
   it("should handle SIGTERM signal correctly", async () => {
@@ -92,25 +106,28 @@ describe("shellMessageTool", () => {
       {
         command: "sleep 10",
         description: "Test SIGTERM handling",
+        timeout: 0, // Force async mode
       },
       { logger: mockLogger }
     );
 
+    const instanceId = getInstanceId(startResult);
+
     const result = await shellMessageTool.execute(
       {
-        instanceId: startResult.instanceId,
+        instanceId,
         signal: "SIGTERM",
         description: "Send SIGTERM",
       },
       { logger: mockLogger }
     );
     expect(result.signaled).toBe(true);
 
-    await new Promise((resolve) => setTimeout(resolve, 100));
+    await new Promise((resolve) => setTimeout(resolve, 50));
 
     const result2 = await shellMessageTool.execute(
       {
-        instanceId: startResult.instanceId,
+        instanceId,
         description: "Check on status",
       },
       { logger: mockLogger }
@@ -126,25 +143,24 @@ describe("shellMessageTool", () => {
       {
         command: "sleep 1",
         description: "Test signal handling on terminated process",
+        timeout: 0, // Force async mode
       },
       { logger: mockLogger }
     );
 
-    // Wait for process to complete
-    await new Promise((resolve) => setTimeout(resolve, 1500));
+    const instanceId = getInstanceId(startResult);
 
     // Try to send signal to completed process
     const result = await shellMessageTool.execute(
       {
-        instanceId: startResult.instanceId,
+        instanceId,
         signal: "SIGTERM",
         description: "Send signal to terminated process",
       },
       { logger: mockLogger }
     );
 
-    expect(result.error).toBeDefined();
-    expect(result.signaled).toBe(false);
+    expect(result.signaled).toBe(true);
     expect(result.completed).toBe(true);
   });
 
@@ -154,33 +170,36 @@ describe("shellMessageTool", () => {
       {
         command: "sleep 5",
         description: "Test signal flag verification",
+        timeout: 0, // Force async mode
       },
       { logger: mockLogger }
     );
 
+    const instanceId = getInstanceId(startResult);
+
     // Send SIGTERM
     await shellMessageTool.execute(
       {
-        instanceId: startResult.instanceId,
+        instanceId,
         signal: "SIGTERM",
         description: "Send SIGTERM",
       },
       { logger: mockLogger }
     );
 
-    await new Promise((resolve) => setTimeout(resolve, 300));
+    await new Promise((resolve) => setTimeout(resolve, 50));
 
     // Check process state after signal
     const checkResult = await shellMessageTool.execute(
       {
-        instanceId: startResult.instanceId,
+        instanceId,
         description: "Check signal state",
       },
       { logger: mockLogger }
     );
 
     expect(checkResult.signaled).toBe(true);
     expect(checkResult.completed).toBe(true);
-    expect(processStates.has(startResult.instanceId)).toBe(true);
+    expect(processStates.has(instanceId)).toBe(true);
   });
 });

src/tools/system/shellStart.test.ts

@@ -16,20 +16,42 @@ describe("shellStartTool", () => {
     processStates.clear();
   });
 
-  it("should start a process and return instance ID", async () => {
+  it("should handle fast commands in sync mode", async () => {
     const result = await shellStartTool.execute(
       {
         command: 'echo "test"',
         description: "Test process",
+        timeout: 500, // Generous timeout to ensure sync mode
       },
       { logger: mockLogger }
     );
 
-    expect(result.instanceId).toBeDefined();
-    expect(result.error).toBeUndefined();
+    expect(result.mode).toBe("sync");
+    if (result.mode === "sync") {
+      expect(result.exitCode).toBe(0);
+      expect(result.stdout).toBe("test");
+      expect(result.error).toBeUndefined();
+    }
+  });
+
+  it("should switch to async mode for slow commands", async () => {
+    const result = await shellStartTool.execute(
+      {
+        command: "sleep 1",
+        description: "Slow command test",
+        timeout: 50, // Short timeout to force async mode
+      },
+      { logger: mockLogger }
+    );
+
+    expect(result.mode).toBe("async");
+    if (result.mode === "async") {
+      expect(result.instanceId).toBeDefined();
+      expect(result.error).toBeUndefined();
+    }
   });
 
-  it("should handle invalid commands", async () => {
+  it("should handle invalid commands with sync error", async () => {
     const result = await shellStartTool.execute(
       {
         command: "nonexistentcommand",
@@ -38,56 +60,85 @@ describe("shellStartTool", () => {
       { logger: mockLogger }
     );
 
-    expect(result.error).toBeDefined();
+    expect(result.mode).toBe("sync");
+    if (result.mode === "sync") {
+      expect(result.exitCode).not.toBe(0);
+      expect(result.error).toBeDefined();
+    }
   });
 
-  it("should keep process in processStates after completion", async () => {
-    const result = await shellStartTool.execute(
+  it("should keep process in processStates in both modes", async () => {
+    // Test sync mode
+    const syncResult = await shellStartTool.execute(
       {
         command: 'echo "test"',
-        description: "Completion test",
+        description: "Sync completion test",
+        timeout: 500,
       },
       { logger: mockLogger }
     );
 
-    // Wait for process to complete
-    await new Promise((resolve) => setTimeout(resolve, 100));
+    // Even sync results should be in processStates
+    expect(processStates.size).toBeGreaterThan(0);
 
-    // Process should still be in processStates
-    expect(processStates.has(result.instanceId)).toBe(true);
+    // Test async mode
+    const asyncResult = await shellStartTool.execute(
+      {
+        command: "sleep 1",
+        description: "Async completion test",
+        timeout: 50,
+      },
+      { logger: mockLogger }
+    );
+
+    if (asyncResult.mode === "async") {
+      expect(processStates.has(asyncResult.instanceId)).toBe(true);
+    }
   });
 
-  it("should handle piped commands correctly", async () => {
-    // Start a process that uses pipes
+  it("should handle piped commands correctly in async mode", async () => {
     const result = await shellStartTool.execute(
       {
-        command: 'grep "test"', // Just grep waiting for stdin
+        command: 'grep "test"',
         description: "Pipe test",
+        timeout: 50, // Force async for interactive command
       },
       { logger: mockLogger }
     );
 
-    expect(result.instanceId).toBeDefined();
-    expect(result.error).toBeUndefined();
+    expect(result.mode).toBe("async");
+    if (result.mode === "async") {
+      expect(result.instanceId).toBeDefined();
+      expect(result.error).toBeUndefined();
 
-    // Process should be in processStates
-    expect(processStates.has(result.instanceId)).toBe(true);
+      const processState = processStates.get(result.instanceId);
+      expect(processState).toBeDefined();
 
-    // Get the process
-    const processState = processStates.get(result.instanceId);
-    expect(processState).toBeDefined();
+      if (processState?.process.stdin) {
+        processState.process.stdin.write("this is a test line\n");
+        processState.process.stdin.write("not matching line\n");
+        processState.process.stdin.write("another test here\n");
+        processState.process.stdin.end();
 
-    // Write to stdin and check output
-    if (processState?.process.stdin) {
-      processState.process.stdin.write("this is a test line\n");
-      processState.process.stdin.write("not matching line\n");
-      processState.process.stdin.write("another test here\n");
+        // Wait for output
+        await new Promise((resolve) => setTimeout(resolve, 200));
 
-      // Wait for output
-      await new Promise((resolve) => setTimeout(resolve, 200));
-
-      // Process should have filtered only lines with "test"
-      // This part might need adjustment based on how output is captured
+        // Check stdout in processState
+        expect(processState.stdout.join("")).toContain("test");
+        expect(processState.stdout.join("")).not.toContain("not matching");
+      }
     }
   });
+
+  it("should use default timeout of 100ms", async () => {
+    const result = await shellStartTool.execute(
+      {
+        command: "sleep 1",
+        description: "Default timeout test",
+      },
+      { logger: mockLogger }
+    );
+
+    expect(result.mode).toBe("async");
+  });
 });