computerd: let the caller choose the exec shell (#139)

Every exec command runs through a hardcoded /bin/sh. On a Debian-family
image that is dash, where bash-only syntax is a parse error rather than
a missing feature, so the whole command aborts instead of degrading.
That matters most for the PIPESTATUS array: a caller that filters a
command's output, such as piping git through sed to redact a credential
from the URL, gets the pipeline's last exit status and reads a failed
push as a success. PIPESTATUS is how the real status is recovered, and
under dash reaching for it is worse than the problem it was reached for.

Add an optional shell to the runner options, defaulting to /bin/sh, and
read the same value from EXEC_SHELL so a deployed daemon can set it
without a rebuild. Existing callers are unaffected. The previous
workaround was to repoint /bin/sh inside the image, which changes echo
semantics for every other script there and is unavailable when the image
is prebuilt.

Co-authored-by: Harness Agent <agent@harness.local>

Author: aron-cf

.changeset/selectable-exec-shell.md

@@ -0,0 +1,5 @@
+---
+"@cloudflare/computerd": minor
+---
+
+The exec runner now takes an optional shell naming the interpreter each command runs under, and computerd reads the same value from EXEC_SHELL. Both default to /bin/sh, so existing behavior is unchanged. On a Debian-family image /bin/sh is dash, where bash-only syntax such as the PIPESTATUS array is a parse error that aborts the command rather than a missing feature, and that array is how a caller recovers the real exit status of a pipeline whose output it filters. Repointing /bin/sh in the image was the only previous workaround, which changes echo semantics for every other script in that image and is unavailable when the image is prebuilt.

packages/computerd/README.md

@@ -113,10 +113,13 @@ Additional environment variables:
 
 ```sh
 EXEC_LOG_MAX_BYTES=1048576        # cap the in-memory exec log buffer (bytes)
+EXEC_SHELL=/usr/bin/bash          # interpreter exec runs commands under (default /bin/sh)
 RPC_CLIENT_SECRET=<secret>        # require Authorization: Bearer <secret> on every route but /health
 COMPUTER_VAR_NODE_ENV=production  # forwarded into exec as NODE_ENV
 ```
 
+`EXEC_SHELL` must be an absolute path. It exists because `/bin/sh` is `dash` on a Debian-family image, where bash-only syntax is a parse error that aborts the command rather than a missing feature: `${PIPESTATUS[@]}`, arrays, `[[ ... ]]`, and process substitution all fail that way. `PIPESTATUS` is the usual way to recover the real exit status of a pipeline whose output is filtered — a command redacting a credential through `sed`, for instance — so a caller that needs it can select an interpreter that has it without repointing `/bin/sh` for every other script in the image.
+
 `FUSE_MOUNT=auto` is the friendly default: if `/dev/fuse` (or macFUSE) is available `computerd` mounts a real FUSE filesystem, otherwise it transparently falls back to the userspace shim. Pin the value (`fuse` / `macfuse` / `shim` / `none`) when a test needs to assert a specific code path.
 
 ## `FUSE_MOUNT=shim` — userspace dev shim

packages/computerd/src/cli/computerd.ts

@@ -636,12 +636,23 @@ async function main(): Promise<void> {
     }
     logMaxBytesOverride = parsed;
   }
+  // EXEC_SHELL picks the interpreter exec runs commands under, for images
+  // whose /bin/sh cannot be repointed. Default lives in the Runner.
+  const shellEnv = process.env.EXEC_SHELL;
+  let shellOverride: string | undefined;
+  if (shellEnv !== undefined && shellEnv !== "") {
+    if (!shellEnv.startsWith("/")) {
+      throw new Error(`EXEC_SHELL must be an absolute path; got ${JSON.stringify(shellEnv)}`);
+    }
+    shellOverride = shellEnv;
+  }
   const runner = new Runner({
     db,
     // When we have a mount (real FUSE or the shim) point spawned
     // children at it so writes from exec flow through the VFS.
     ...(fuse !== undefined ? { cwd: mountPoint } : {}),
     ...(logMaxBytesOverride !== undefined ? { logMaxBytes: logMaxBytesOverride } : {}),
+    ...(shellOverride !== undefined ? { shell: shellOverride } : {}),
   });
   // Heartbeat events are computerd-local and must not cross the RPC boundary.
   // Wrap the runner so every exec/get stream drops heartbeat events before

packages/computerd/src/exec/runner.test.ts

@@ -1,3 +1,5 @@
+import { existsSync } from "node:fs";
+
 import { Database, initializeSchema, WorkspaceFilesystem } from "@cloudflare/dofs";
 import { SQLiteTestStorage } from "@cloudflare/dofs/testing";
 import { expect, test } from "vitest";
@@ -517,3 +519,74 @@ test("a spawned command sees the allowlisted environment, not the daemon's", asy
     dispose();
   }
 });
+
+// The interpreter is a per-consumer choice, not a property of the image.
+//
+// A caller that redacts credentials through a pipe -- `git push "$URL" 2>&1 |
+// sed -E 's#//[^@]*@#//***@#'` -- gets the pipeline's last exit status, so a
+// failed push reads as success. PIPESTATUS is the usual recovery, and under
+// dash it is a parse error that aborts the command rather than a missing
+// feature, which is worse than the problem it was reached for.
+const hasBash = existsSync("/usr/bin/bash");
+
+test("defaults to /bin/sh when no shell is given", async () => {
+  const { runner, dispose } = fixture();
+  try {
+    const handle = runner.exec("printf '%s' \"$0\"");
+    const events = await drain(handle.events);
+    const stdout = events
+      .filter((event) => event.name === "stdout")
+      .map((event) => decode(event.value as Uint8Array))
+      .join("");
+    expect(stdout).toBe("/bin/sh");
+  } finally {
+    dispose();
+  }
+});
+
+test.skipIf(!hasBash)("runs commands under an explicitly chosen shell", async () => {
+  const { runner, dispose } = fixture({ shell: "/usr/bin/bash" });
+  try {
+    const handle = runner.exec("printf '%s' \"$0\"");
+    const events = await drain(handle.events);
+    const stdout = events
+      .filter((event) => event.name === "stdout")
+      .map((event) => decode(event.value as Uint8Array))
+      .join("");
+    expect(stdout).toBe("/usr/bin/bash");
+  } finally {
+    dispose();
+  }
+});
+
+test.skipIf(!hasBash)("a chosen shell resolves PIPESTATUS instead of aborting", async () => {
+  const { runner, dispose } = fixture({ shell: "/usr/bin/bash" });
+  try {
+    // false | true leaves $? as true's 0 while the first pipeline stage's real
+    // failure survives in the PIPESTATUS array. The trailing marker proves the
+    // command was not aborted: under dash the expansion is fatal and "after"
+    // never prints. The expansion is assembled from parts so its braces are
+    // not linted as a JavaScript template placeholder.
+    const first = ['"$', "{PIPESTATUS[0]}", '"'].join("");
+    const handle = runner.exec(`false | true; printf '[%s]' ${first}; printf "after"`);
+    const events = await drain(handle.events);
+    const stdout = events
+      .filter((event) => event.name === "stdout")
+      .map((event) => decode(event.value as Uint8Array))
+      .join("");
+    expect(stdout).toBe("[1]after");
+  } finally {
+    dispose();
+  }
+});
+
+test("rejects a shell that is not an absolute path", () => {
+  const storage = new SQLiteTestStorage();
+  const db = new Database(storage);
+  initializeSchema(db, () => Date.now());
+  try {
+    expect(() => new Runner({ db, shell: "bash" })).toThrow(/absolute path/);
+  } finally {
+    storage.close?.();
+  }
+});

packages/computerd/src/exec/runner.ts

@@ -61,6 +61,7 @@ const DEFAULTS = {
   // After SIGTERM, give the child this long to exit on its own
   // before sending SIGKILL.
   killGraceMs: 5_000,
+  shell: "/bin/sh",
 } as const;
 
 export interface RunnerInit extends RunnerOptions {
@@ -80,6 +81,7 @@ export class Runner {
     sweepIntervalMs: number;
     defaultTimeoutMs: number;
     heartbeatIntervalMs: number;
+    shell: string;
     now: () => number;
   };
   private readonly records = new Map<string, ExecRecord>();
@@ -96,8 +98,14 @@ export class Runner {
       sweepIntervalMs: init.sweepIntervalMs ?? DEFAULTS.sweepIntervalMs,
       defaultTimeoutMs: init.defaultTimeoutMs ?? DEFAULTS.defaultTimeoutMs,
       heartbeatIntervalMs: init.heartbeatIntervalMs ?? 0,
+      shell: init.shell ?? DEFAULTS.shell,
       now: init.now ?? Date.now,
     };
+    // Fail here rather than letting every exec() surface a bare ENOENT from
+    // spawn, which names the interpreter but not the misconfiguration.
+    if (!this.opts.shell.startsWith("/")) {
+      throw new Error(`shell must be an absolute path; got ${JSON.stringify(this.opts.shell)}`);
+    }
     initializeExecSchema(this.db);
     if (init.resetSchema !== false) clearExecState(this.db);
   }
@@ -131,7 +139,7 @@ export class Runner {
     // status pipe. If cwd lives inside computerd's own FUSE mount, the
     // child's chdir issues a FUSE LOOKUP that computerd can't service
     // (its event loop is stuck in uv_spawn), and the whole
-    // process deadlocks. Have /bin/sh do the chdir instead: by
+    // process deadlocks. Have the shell do the chdir instead: by
     // the time the shell runs its `cd`, computerd's event loop is back
     // and can answer the FUSE callback normally.
     //
@@ -143,7 +151,7 @@ export class Runner {
     // (`spawn("/bin/sh", ["-c", command])`) was already a shell-
     // owned exec, so this is no change in process shape.
     const wrapped = cwd !== undefined ? `cd ${shellQuote(cwd)} && ${command}` : command;
-    const child = spawn("/bin/sh", ["-c", wrapped], {
+    const child = spawn(this.opts.shell, ["-c", wrapped], {
       env,
       stdio: [options.stdin === undefined ? "ignore" : "pipe", "pipe", "pipe"],
     });

packages/computerd/src/exec/types.ts

@@ -62,6 +62,13 @@ export interface RunnerOptions {
   // Emit a heartbeat event every this many milliseconds while a child
   // process is alive. When unset or zero, no heartbeat events are emitted.
   heartbeatIntervalMs?: number;
+  // Absolute path to the interpreter each command runs under. Defaults to
+  // /bin/sh, which on a Debian-family image is dash: bash-only syntax a
+  // caller may reach for, notably ${PIPESTATUS[@]}, is a parse error there
+  // and aborts the command rather than degrading. Not inferred from SHELL,
+  // which names the caller's login shell and is deliberately absent from the
+  // env allowlist.
+  shell?: string;
   // Test seam: replaces Date.now() for retention math and log ts.
   now?: () => number;
 }