change error handling to pass non-zero exit code

- also stop passing back message that was redundant and useless
- and make error logic slightly more consistent (albeit still sloppy)

Author: g0t4

TODO.md

@@ -1,15 +1,5 @@
 ## TODOs
 
-- Rename `cwd` to increase usage by Claude and other models?
-  - `workdir`, `workDir`, `workingDirectory` ??
-- Explicit return code on errors only (non-zero)
-  - do not show when zero
-  - separate content item?
-    "Exit code: -1"
-    or {"name": "exit code", text: "-1" }?
-- add `timeout` optional param
-
-
 - Every time Claude runs a python script, `python` is used as the interpreter. Which fails every time.
     - Thankfully, Claude retries with `python3` and uses that for the rest of the chat. 
         - Hence the idea to have some memory concept across chats! Very selective memory and very minimal.

src/exec-utils.ts

@@ -2,22 +2,26 @@ import { exec, ExecOptions } from "child_process";
 import { ObjectEncodingOptions } from "fs";
 
 type ExecResult = {
-    // FYI leave this type for now as a declaration of the expected shape of the result for BOTH success and failure (errors)
-    //   do not switch to using ExecException b/c that only applies to failures
+    // this is basically ExecException except I want my own type for it...
+    //   b/c I want this to represent all results
+    //   ... by the way throws put stdout/stderr on the error "result" object
+    //       hence I am replicating that here and in my promise reject calls
     stdout: string;
     stderr: string;
 
-    // TODO dear GOD... wes why the F did you call this message? error_message would've been better (at a minimum)
-    // message is the error message from the child process, not sure I like this naming
-    // - perhaps worth pushing the error logic out of messagesFor back into catch block above
-    message?: string;
+    // ONLY on errors:
+    message?: string; // FYI redundant b/c message ~= `Command failed: ${cmd}\n${stderr}\n`
+    code?: number;
+    killed?: boolean;
+    signal?: NodeJS.Signals | undefined;
+    cmd?: string; // FYI redundant
 };
 
 /**
  * Executes a file with the given arguments, piping input to stdin.
  * @param {string} interpreter - The file to execute.
  * @param {string} stdin - The string to pipe to stdin.
- * @returns {Promise<ExecResult>} A promise that resolves with the stdout and stderr of the command. `message` is provided on a failure to explain the error.
+ * @returns {Promise<ExecResult>}
  */
 function execFileWithInput(
     interpreter: string,
@@ -35,8 +39,13 @@ function execFileWithInput(
     return new Promise((resolve, reject) => {
         const child = exec(interpreter, options, (error, stdout, stderr) => {
             if (error) {
-                reject({ message: error.message, stdout, stderr });
+                // console.log("execFileWithInput ERROR:", error);
+                // mirror ExecException used by throws
+                error.stdout = stdout;
+                error.stderr = stderr;
+                reject(error);
             } else {
+                // I assume RC==0 else would trigger error?
                 resolve({ stdout, stderr });
             }
         });
@@ -68,7 +77,11 @@ async function fishWorkaround(
         exec(command, options, (error, stdout, stderr) => {
             // I like this style of error vs success handling! it's beautiful-est (prommises are underrated)
             if (error) {
-                reject({ message: error.message, stdout, stderr });
+                // console.log("fishWorkaround ERROR:", error);
+                // mirror ExecException used by throws
+                error.stdout = stdout;
+                error.stderr = stderr;
+                reject(error);
             } else {
                 resolve({ stdout, stderr });
             }

src/messages.ts

@@ -5,15 +5,40 @@ import { TextContent } from "@modelcontextprotocol/sdk/types.js";
  * Converts an ExecResult into an array of TextContent messages.
  */
 export function messagesFor(result: ExecResult): TextContent[] {
-    // TODO! RETURN CODE!!! add as RETURN_CODE and number type
     const messages: TextContent[] = [];
-    if (result.message) {
+
+    if (result.code !== undefined) {
         messages.push({
             type: "text",
-            text: result.message,
-            name: "ERROR",
+            text: `${result.code}`,
+            name: "EXIT_CODE",
         });
     }
+
+    // PRN any situation where I want to pass .message and/or .cmd?
+    // maybe on errors I should? that way there's a chance to make sure the command was as intended
+    // and maybe include message when it doesn't contain stderr?
+    // FYI if I put these back, start with tests first
+
+    // PRN use a test to add these, sleep 10s maybe and then kill that process?
+    //  definitely could be useful to know if a command was killed
+    //  make sure signal is not null, which is what's used when no signal killed the process
+    // if (result.signal) {
+    //     messages.push({
+    //         type: "text",
+    //         text: `Signal: ${result.signal}`,
+    //         name: "SIGNAL",
+    //     });
+    // } 
+    // if (!!result.killed) {
+    //     // killed == true is the only time to include this
+    //     messages.push({
+    //         type: "text",
+    //         text: "Process was killed",
+    //         name: "KILLED",
+    //     });
+    // }
+
     if (result.stdout) {
         messages.push({
             type: "text",

src/run-command.ts

@@ -9,6 +9,10 @@ import { ObjectEncodingOptions } from "node:fs";
 const execAsync = promisify(exec);
 
 async function execute(command: string, stdin: string, options: ExecOptions) {
+    // PRN merge calls to exec into one single paradigm with conditional STDIN handled in one spot?
+    //   right now no STDIN => exec directly and let it throw to catch failures
+    //   w/ STDIN => you manually glue together callbacks + promises (i.e. reject)
+    //     feels sloppy to say the least, notably the error handling with ExecExeption error that has stdin/stderr on it
     if (!stdin) {
         return await execAsync(command, options);
     }
@@ -18,9 +22,9 @@ async function execute(command: string, stdin: string, options: ExecOptions) {
 /**
  * Executes a command and returns the result as CallToolResult.
  */
-export async function runCommand(
-    args: Record<string, unknown> | undefined
-): Promise<CallToolResult> {
+export type RunCommandArgs = Record<string, unknown> | undefined;
+export async function runCommand(args: RunCommandArgs): Promise<CallToolResult> {
+
     const command = args?.command as string;
     if (!command) {
         const message = "Command is required, current value: " + command;
@@ -43,6 +47,24 @@ export async function runCommand(
             content: messagesFor(result),
         };
     } catch (error) {
+        // PRN do I want to differentiate non-command related error (i.e. if messagesFor blows up
+        //   or presumably if smth else goes wrong with the node code in exec that isn't command related
+        //   if so, write a test first
+
+        // console.log("ERROR_runCommand", error);
+        // ExecException (error + stdout/stderr) merged
+        // - IIUC this happens on uncaught failures
+        // - but if you catch an exec() promise failure (or use exec's callback) => you get separated values: error, stdout, stderr
+        // - which is why I mirror this response type in my reject(error) calls
+        //
+        // 'error' example:
+        // code: 127,
+        // killed: false,
+        // signal: null,
+        // cmd: 'nonexistentcommand',
+        // stdout: '',
+        // stderr: '/bin/sh: nonexistentcommand: command not found\n'
+
         const response = {
             isError: true,
             content: messagesFor(error as ExecResult),

tests/integration/exec-utils.test.ts

@@ -1,11 +1,9 @@
 import { execFileWithInput } from "../../src/exec-utils.js";
 
-// FYI Claude generated most of these, by dog fooding the run_command/script tools!
-// I am going to keep asking Claude to add new tests to see how I feel about that workflow
+// FYI these tests are largely to make sure I understand how exec works
+// + my changes to exec (i.e. reject promise on failure in STDIN path)
 
 describe("execFileWithInput integration tests", () => {
-    // ok, impressive choice of "seam" to add testing of the most critical part, executing the command!
-    // this is EXACTLY what I had in mind and didn't even tell Claude I wanted.
 
     test("should execute a simple bash command", async () => {
         const result = await execFileWithInput(
@@ -16,6 +14,7 @@ describe("execFileWithInput integration tests", () => {
         // console.log(result);
         expect(result.stdout).toBe("Hello World\n");
         expect(result.stderr).toBe("");
+        expect(result.code).toBeUndefined();
     });
 
     test("should handle command errors properly in bash", async () => {
@@ -25,8 +24,11 @@ describe("execFileWithInput integration tests", () => {
         } catch (result: any) {
             // FYI catch is so you can run assertions on the failed result, given the promise is rejected, it's then thrown here
             // console.log(result);
-            expect(result.stderr).toContain("bash: line 1: nonexistentcommand: command not found");
-            expect(result.message).toContain("Command failed: bash\nbash: line 1: nonexistentcommand: command not found\n");
+            const expected_stderr = "bash: line 1: nonexistentcommand: command not found";
+            expect(result.stderr).toContain(expected_stderr);
+            const expected_message = "Command failed: bash\n" + expected_stderr + "\n";
+            expect(result.message).toContain(expected_message);
+            expect(result.code).toBe(127);
         }
     });
 
@@ -39,6 +41,7 @@ describe("execFileWithInput integration tests", () => {
         // console.log(result);
         expect(result.stdout).toBe("Hello from Fish\n");
         expect(result.stderr).toBe("");
+        expect(result.code).toBeUndefined();
     });
 
     // TODO make sure to cover the fish workaround logic, in all its edge cases and then can leave those tests when I remove that or just nuke them
@@ -48,8 +51,17 @@ describe("execFileWithInput integration tests", () => {
             fail("Should have thrown an error");
         } catch (result: any) {
             // console.log(result);
-            expect(result.stderr).toContain("fish: Unknown command: totallynonexistentcommand\nfish: \ntotallynonexistentcommand\n^~~~~~~~~~~~~~~~~~~~~~~~^");
-            expect(result.message).toBeTruthy();
+
+            const expected_stderr = "fish: Unknown command: totallynonexistentcommand\nfish: \ntotallynonexistentcommand\n^~~~~~~~~~~~~~~~~~~~~~~~^";
+            expect(result.stderr).toContain(expected_stderr);
+            // TODO! this is why I don't think I should return error.message... or at least not in many cases
+            //    OR strip off the stderr overlap?
+            const expected_message = 'Command failed: fish -c "echo dG90YWxseW5vbmV4aXN0ZW50Y29tbWFuZA== | base64 -d | fish"' +
+                "\n" + expected_stderr;
+            expect(result.message).toContain(expected_message);
+            expect(result.code).toBe(127);
+            expect(result.killed).toBe(false);
+            expect(result.signal).toBeNull();
         }
     });
 
@@ -62,6 +74,7 @@ describe("execFileWithInput integration tests", () => {
         // console.log(result);
         expect(result.stdout).toBe("Hello from Zsh\n");
         expect(result.stderr).toBe("");
+        expect(result.code).toBeUndefined();
     });
 
     test("should handle command errors properly in zsh", async () => {
@@ -70,10 +83,13 @@ describe("execFileWithInput integration tests", () => {
             fail("Should have thrown an error");
         } catch (result: any) {
             // console.log(result);
-            // TODO why am I not reporting the exit code?! ==> 127 here (and in other cases above for missing command)
-            expect(result.stderr).toContain("zsh: command not found: completelynonexistentcommand");
-            // TODO why am I bothering to return message... it seems to just duplicate STDERR?!
-            expect(result.message).toBeTruthy();
+            const expected_stderr = "zsh: command not found: completelynonexistentcommand";
+            expect(result.stderr).toContain(expected_stderr);
+            const expected_message = "Command failed: zsh\n" + expected_stderr + "\n";
+            expect(result.message).toBe(expected_message);
+            expect(result.code).toBe(127);
+            expect(result.killed).toBe(false);
+            expect(result.signal).toBeNull();
         }
     });
 
@@ -85,15 +101,14 @@ describe("execFileWithInput integration tests", () => {
       done
     `;
         const result = await execFileWithInput("zsh", stdin, {});
-        //expect(lines[0]).toContain('Line 1 from Zsh');
-        //expect(lines[1]).toContain('Number 1');
-        //expect(lines[2]).toContain('Number 2');
-        //expect(lines[3]).toContain('Number 3');
+        // console.log(result);
         expect(result.stdout).toContain(`Line 1 from Zsh
 Number 1
 Number 2
 Number 3
 `);
+        expect(result.stderr).toBe("");
+        expect(result.code).toBeUndefined();
     });
 
     test("should respect working directory option", async () => {
@@ -102,7 +117,10 @@ Number 3
         // TODO make sure cwd is not already / in the test?
         // PRN use multiple paths would be another way around checking cwd of test runner
         const result = await execFileWithInput("bash", "pwd", { cwd: "/" });
-        expect(result.stdout.trim()).toBe("/");
+        // console.log(result);
+        expect(result.stdout).toBe("/\n");
+        expect(result.stderr).toBe("");
+        expect(result.code).toBeUndefined();
     });
 
     test("should handle bash multiline scripts", async () => {
@@ -113,9 +131,12 @@ Number 3
     `;
         const result = await execFileWithInput("bash", stdin, {});
         // validate all of output:
+        // console.log(result);
         expect(result.stdout).toContain(`Line 1
 Line 2
 Line 3`);
+        expect(result.stderr).toBe("");
+        expect(result.code).toBeUndefined();
     });
 });