fix(sandbox): address review feedback on exec cancellation and typing

- collectExec stops delivering onData chunks once the promise has settled,
  so a timeout/abort rejection is the last thing a streaming caller sees
- transport.exec rejects an already-aborted signal before opening the SSH
  channel, so the remote command never starts
- RunCommandOptions is now a blocking/detached union: timeout, signal, and
  output callbacks no longer type-check with detached: true, and dynamic
  options resolve to Command | CommandFinished instead of the wrong overload

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GrhrNWGpQR9qo7ovxQ13qY

Author: claude

packages/sandbox/src/index.ts

@@ -2,7 +2,9 @@ export { Command, CommandFinished, type LogChunk } from "./command.ts";
 export { CommandTimeoutError, SandboxError } from "./errors.ts";
 export { Sandbox } from "./sandbox.ts";
 export type {
+  BlockingCommandOptions,
   CreateOptions,
+  DetachedCommandOptions,
   FileToWrite,
   GetOptions,
   RunCommandOptions,

packages/sandbox/src/sandbox.test.ts

@@ -14,6 +14,7 @@ import {
   shellQuote,
 } from "./sandbox.ts";
 import { collectExec, fileEntryFromAttrs } from "./transport.ts";
+import type { RunCommandOptions } from "./types.ts";
 
 describe("Sandbox.create", () => {
   test("rejects reserved env key AGENT_TOKEN before any network call", async () => {
@@ -44,9 +45,15 @@ describe("Sandbox.runCommand validation", () => {
     );
   });
 
+  // The union type forbids these statically; the casts stand in for
+  // un-typechecked JS callers, which the runtime guard still protects.
   test("rejects timeout combined with detached", async () => {
     await expect(
-      sandbox.runCommand({ cmd: "ls", detached: true, timeout: 5000 }),
+      sandbox.runCommand({
+        cmd: "ls",
+        detached: true,
+        timeout: 5000,
+      } as RunCommandOptions),
     ).rejects.toThrow("not supported with detached");
   });
 
@@ -56,16 +63,24 @@ describe("Sandbox.runCommand validation", () => {
         cmd: "ls",
         detached: true,
         signal: new AbortController().signal,
-      }),
+      } as RunCommandOptions),
     ).rejects.toThrow("not supported with detached");
   });
 
   test("rejects output callbacks combined with detached", async () => {
     await expect(
-      sandbox.runCommand({ cmd: "ls", detached: true, onStdout: () => {} }),
+      sandbox.runCommand({
+        cmd: "ls",
+        detached: true,
+        onStdout: () => {},
+      } as RunCommandOptions),
     ).rejects.toThrow("not supported with detached");
     await expect(
-      sandbox.runCommand({ cmd: "ls", detached: true, onStderr: () => {} }),
+      sandbox.runCommand({
+        cmd: "ls",
+        detached: true,
+        onStderr: () => {},
+      } as RunCommandOptions),
     ).rejects.toThrow("not supported with detached");
   });
 });
@@ -159,6 +174,21 @@ describe("collectExec", () => {
       { stream: "stderr", data: "err" },
     ]);
   });
+
+  test("stops streaming to onData once the promise has settled", async () => {
+    const stream = fakeStream();
+    const chunks: Array<{ stream: string; data: string }> = [];
+    const pending = collectExec(stream, {
+      timeoutMs: 5,
+      onData: (c) => chunks.push(c),
+    });
+    stream.emit("data", Buffer.from("before"));
+    await expect(pending).rejects.toThrow(CommandTimeoutError);
+    // Chunks still buffered in the channel arrive after rejection.
+    stream.emit("data", Buffer.from("after"));
+    stream.emitStderr(Buffer.from("late-err"));
+    expect(chunks).toEqual([{ stream: "stdout", data: "before" }]);
+  });
 });
 
 describe("fileEntryFromAttrs", () => {

packages/sandbox/src/sandbox.ts

@@ -21,7 +21,9 @@ import {
 } from "./provision.ts";
 import { SshTransport } from "./transport.ts";
 import type {
+  BlockingCommandOptions,
   CreateOptions,
+  DetachedCommandOptions,
   FileToWrite,
   GetOptions,
   RunCommandOptions,
@@ -171,16 +173,36 @@ export class Sandbox {
 
   /** Run a command, blocking for the result unless detached is set. */
   async runCommand(command: string, args?: string[]): Promise<CommandFinished>;
+  async runCommand(command: DetachedCommandOptions): Promise<Command>;
+  async runCommand(command: BlockingCommandOptions): Promise<CommandFinished>;
+  // Options not statically known to be blocking or detached get the union.
   async runCommand(
-    command: RunCommandOptions & { detached: true },
-  ): Promise<Command>;
-  async runCommand(command: RunCommandOptions): Promise<CommandFinished>;
+    command: RunCommandOptions,
+  ): Promise<CommandFinished | Command>;
   async runCommand(
     command: string | RunCommandOptions,
     args: string[] = [],
   ): Promise<CommandFinished | Command> {
     const opts: RunCommandOptions =
       typeof command === "string" ? { cmd: command, args } : command;
+    const remote = buildRemoteCommand(opts);
+
+    if (opts.detached) {
+      // The types forbid this, but un-typechecked callers can still pass blocking-only options.
+      const stray = opts as unknown as Partial<BlockingCommandOptions>;
+      if (
+        stray.timeout !== undefined ||
+        stray.signal ||
+        stray.onStdout ||
+        stray.onStderr
+      ) {
+        throw new SandboxError(
+          "timeout, signal, onStdout, and onStderr are not supported with detached; use command.kill() and command.logs().",
+        );
+      }
+      return new Command(await this.transport.execStream(remote));
+    }
+
     if (
       opts.timeout !== undefined &&
       (!Number.isFinite(opts.timeout) || opts.timeout <= 0)
@@ -190,19 +212,6 @@ export class Sandbox {
       );
     }
     const { onStdout, onStderr } = opts;
-    if (
-      opts.detached &&
-      (opts.timeout !== undefined || opts.signal || onStdout || onStderr)
-    ) {
-      throw new SandboxError(
-        "timeout, signal, onStdout, and onStderr are not supported with detached; use command.kill() and command.logs().",
-      );
-    }
-    const remote = buildRemoteCommand(opts);
-
-    if (opts.detached) {
-      return new Command(await this.transport.execStream(remote));
-    }
     const onData =
       onStdout || onStderr
         ? ({ stream, data }: LogChunk) => {

packages/sandbox/src/transport.ts

@@ -46,20 +46,24 @@ export function collectExec(
 ): Promise<ExecResult> {
   let stdout = "";
   let stderr = "";
+  // Chunks buffered in the channel can still arrive after a timeout/abort
+  // settles the promise; keep them out of onData once settled.
+  let settled = false;
   stream.on("data", (d: Buffer) => {
     const data = d.toString();
     stdout += data;
-    limits.onData?.({ stream: "stdout", data });
+    if (!settled) limits.onData?.({ stream: "stdout", data });
   });
   stream.stderr.on("data", (d: Buffer) => {
     const data = d.toString();
     stderr += data;
-    limits.onData?.({ stream: "stderr", data });
+    if (!settled) limits.onData?.({ stream: "stderr", data });
   });
 
   return new Promise((resolve, reject) => {
     let timer: ReturnType<typeof setTimeout> | undefined;
     const cleanup = () => {
+      settled = true;
       clearTimeout(timer);
       limits.signal?.removeEventListener("abort", onAbort);
     };
@@ -159,6 +163,9 @@ export class SshTransport {
 
   /** Run a command to completion and collect its output. */
   async exec(command: string, limits?: ExecLimits): Promise<ExecResult> {
+    // Don't open a channel (and start the command) for an already-cancelled call.
+    // An abort mid-open is caught by collectExec, which kills the fresh channel.
+    limits?.signal?.throwIfAborted();
     return collectExec(await this.execStream(command), limits);
   }
 

packages/sandbox/src/types.ts

@@ -49,26 +49,40 @@ export interface SandboxHandle {
   ports?: Record<number, string>;
 }
 
-export interface RunCommandOptions {
+interface RunCommandOptionsBase {
   cmd: string;
   args?: string[];
   /** Working directory. Defaults to the sandbox workplace. */
   cwd?: string;
   /** Extra environment variables for this command only. */
   env?: Record<string, string>;
   sudo?: boolean;
-  /** Return immediately with a live Command instead of blocking. */
-  detached?: boolean;
+}
+
+/** Options for a blocking command (the default). */
+export interface BlockingCommandOptions extends RunCommandOptionsBase {
+  detached?: false;
   /** Kill the command and reject with CommandTimeoutError after this many milliseconds. */
   timeout?: number;
   /** Abort to kill the command and reject with the signal's reason. */
   signal?: AbortSignal;
-  /** Called with stdout chunks as they arrive. Blocking mode only. */
+  /** Called with stdout chunks as they arrive. */
   onStdout?: (chunk: string) => void;
-  /** Called with stderr chunks as they arrive. Blocking mode only. */
+  /** Called with stderr chunks as they arrive. */
   onStderr?: (chunk: string) => void;
 }
 
+/**
+ * Options for a detached command, which returns a live Command immediately.
+ * A detached command manages its own lifetime — use `command.kill()` and
+ * `command.logs()` instead of `timeout`/`signal`/output callbacks.
+ */
+export interface DetachedCommandOptions extends RunCommandOptionsBase {
+  detached: true;
+}
+
+export type RunCommandOptions = BlockingCommandOptions | DetachedCommandOptions;
+
 /** A directory entry returned by listFiles. */
 export interface SandboxFileEntry {
   name: string;