From 73eb29bc0fb07ad6f410ff74bf7adc97100c1ab6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 13 Jul 2026 09:21:00 +0000 Subject: [PATCH] refactor(sandbox): guard detached against output callbacks, clearer cp errors - runCommand now rejects onStdout/onStderr combined with detached (they were silently ignored); the guard message points to command.kill()/logs() - sandbox cp reports a clear error for directory sources instead of a misleading "file not found", and stats the source once instead of twice - drop the repeated `as RemoteRef` casts in cp by narrowing ref once - reuse LogChunk for exec streaming chunks instead of a duplicated inline type - remove a dead `?? 0` fallback in collectExec's timeout path - trim redundant comments Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GrhrNWGpQR9qo7ovxQ13qY --- packages/cli/README.md | 2 +- packages/cli/src/commands/sandbox/cp.ts | 41 +++++++++++++------------ packages/sandbox/src/sandbox.test.ts | 9 ++++++ packages/sandbox/src/sandbox.ts | 24 ++++++++------- packages/sandbox/src/transport.ts | 13 ++++---- 5 files changed, 51 insertions(+), 38 deletions(-) diff --git a/packages/cli/README.md b/packages/cli/README.md index 3ba1237a..f2355926 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -961,7 +961,7 @@ bunny sandbox cp my-sandbox:/workplace/out.log ./out.log bunny sandbox cp my-sandbox:/workplace/out.log ./logs/ ``` -Uploads preserve the local file's Unix mode (so executables stay executable). Sandbox-to-sandbox copies are not supported. +Uploads preserve the local file's Unix mode (so executables stay executable). Only single files are supported — directory and sandbox-to-sandbox copies are not. #### `bunny sandbox ssh` diff --git a/packages/cli/src/commands/sandbox/cp.ts b/packages/cli/src/commands/sandbox/cp.ts index 8e81c7ca..c24d0182 100644 --- a/packages/cli/src/commands/sandbox/cp.ts +++ b/packages/cli/src/commands/sandbox/cp.ts @@ -83,13 +83,12 @@ export const sandboxCpCommand = defineCommand({ "Sandbox-to-sandbox copies are not supported. One side must be a local path.", ); } - if (!srcRemote && !destRemote) { + const ref = srcRemote ?? destRemote; + if (!ref) { throw new UserError( "One path must reference a sandbox as :.", ); } - - const ref = (srcRemote ?? destRemote) as RemoteRef; const record = getSandbox(ref.sandbox); if (!record) { throw new UserError( @@ -125,37 +124,39 @@ export const sandboxCpCommand = defineCommand({ let from: string; let to: string; try { - if (destRemote) { - // Local -> sandbox. + if (uploading) { const local = source; - const file = Bun.file(local); - if (!(await file.exists())) { + const info = await stat(local).catch(() => null); + if (!info) { throw new UserError(`Local file not found: ${local}`); } + if (info.isDirectory()) { + throw new UserError( + `${local} is a directory. Only single files can be copied.`, + ); + } // A trailing slash on the remote path means "into this directory". - const remotePath = destRemote.path.endsWith("/") - ? `${destRemote.path}${basename(local)}` - : destRemote.path; - const content = Buffer.from(await file.arrayBuffer()); - const mode = (await stat(local)).mode & 0o777; - await sandbox.writeFiles([{ path: remotePath, content, mode }]); + const remotePath = ref.path.endsWith("/") + ? `${ref.path}${basename(local)}` + : ref.path; + const content = Buffer.from(await Bun.file(local).arrayBuffer()); + await sandbox.writeFiles([ + { path: remotePath, content, mode: info.mode & 0o777 }, + ]); from = local; to = `${ref.sandbox}:${remotePath}`; } else { - // Sandbox -> local. - const content = await sandbox.readFile((srcRemote as RemoteRef).path); + const content = await sandbox.readFile(ref.path); if (content === null) { - throw new UserError( - `File not found in sandbox: ${(srcRemote as RemoteRef).path}`, - ); + throw new UserError(`File not found in sandbox: ${ref.path}`); } // An existing local directory (or trailing slash) means "into it". const localPath = dest.endsWith("/") || (await isDirectory(dest)) - ? join(dest, basename((srcRemote as RemoteRef).path)) + ? join(dest, basename(ref.path)) : dest; await Bun.write(localPath, content); - from = `${ref.sandbox}:${(srcRemote as RemoteRef).path}`; + from = `${ref.sandbox}:${ref.path}`; to = localPath; } } catch (err) { diff --git a/packages/sandbox/src/sandbox.test.ts b/packages/sandbox/src/sandbox.test.ts index 9f9444e1..e196774d 100644 --- a/packages/sandbox/src/sandbox.test.ts +++ b/packages/sandbox/src/sandbox.test.ts @@ -59,6 +59,15 @@ describe("Sandbox.runCommand validation", () => { }), ).rejects.toThrow("not supported with detached"); }); + + test("rejects output callbacks combined with detached", async () => { + await expect( + sandbox.runCommand({ cmd: "ls", detached: true, onStdout: () => {} }), + ).rejects.toThrow("not supported with detached"); + await expect( + sandbox.runCommand({ cmd: "ls", detached: true, onStderr: () => {} }), + ).rejects.toThrow("not supported with detached"); + }); }); function fakeStream() { diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index 8a4a97c0..49612dae 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -1,6 +1,6 @@ import { randomBytes } from "node:crypto"; import type { createMcClient } from "@bunny.net/openapi-client"; -import { Command, CommandFinished } from "./command.ts"; +import { Command, CommandFinished, type LogChunk } from "./command.ts"; import { SandboxError } from "./errors.ts"; import { addCdnEndpoint, @@ -189,9 +189,13 @@ export class Sandbox { "timeout must be a positive number of milliseconds.", ); } - if (opts.detached && (opts.timeout !== undefined || opts.signal)) { + const { onStdout, onStderr } = opts; + if ( + opts.detached && + (opts.timeout !== undefined || opts.signal || onStdout || onStderr) + ) { throw new SandboxError( - "timeout and signal are not supported with detached; use command.kill().", + "timeout, signal, onStdout, and onStderr are not supported with detached; use command.kill() and command.logs().", ); } const remote = buildRemoteCommand(opts); @@ -200,10 +204,10 @@ export class Sandbox { return new Command(await this.transport.execStream(remote)); } const onData = - opts.onStdout || opts.onStderr - ? (chunk: { stream: "stdout" | "stderr"; data: string }) => { - if (chunk.stream === "stdout") opts.onStdout?.(chunk.data); - else opts.onStderr?.(chunk.data); + onStdout || onStderr + ? ({ stream, data }: LogChunk) => { + if (stream === "stdout") onStdout?.(data); + else onStderr?.(data); } : undefined; const { stdout, stderr, exitCode } = await this.transport.exec(remote, { @@ -365,10 +369,8 @@ export class Sandbox { } /** - * Release the SSH connection when the sandbox leaves a `using` / - * `await using` scope. This mirrors `disconnect()` and deliberately does - * NOT delete the sandbox — the backing app keeps running. Call `delete()` - * explicitly to tear down an ephemeral sandbox. + * `using` / `await using` release the SSH connection but deliberately do + * NOT delete the sandbox — call `delete()` explicitly to tear one down. */ [Symbol.dispose](): void { this.disconnect(); diff --git a/packages/sandbox/src/transport.ts b/packages/sandbox/src/transport.ts index 4a35d1ff..132ae97a 100644 --- a/packages/sandbox/src/transport.ts +++ b/packages/sandbox/src/transport.ts @@ -4,6 +4,7 @@ import { type ConnectConfig, type SFTPWrapper, } from "ssh2"; +import type { LogChunk } from "./command.ts"; import { CommandTimeoutError, SandboxError } from "./errors.ts"; import type { SandboxFileEntry } from "./types.ts"; @@ -27,7 +28,7 @@ export interface ExecLimits { /** Abort to kill the command and reject with the signal's reason. */ signal?: AbortSignal; /** Called with each output chunk as it arrives, before the buffered result resolves. */ - onData?: (chunk: { stream: "stdout" | "stderr"; data: string }) => void; + onData?: (chunk: LogChunk) => void; } /** Minimal exec-channel surface, kept narrow so tests can fake it. */ @@ -85,17 +86,18 @@ export function collectExec( }); if (limits.signal?.aborted) return onAbort(); limits.signal?.addEventListener("abort", onAbort, { once: true }); - if (limits.timeoutMs !== undefined) { + const { timeoutMs } = limits; + if (timeoutMs !== undefined) { timer = setTimeout(() => { cleanup(); kill(); - reject(new CommandTimeoutError(limits.timeoutMs ?? 0, stdout, stderr)); - }, limits.timeoutMs); + reject(new CommandTimeoutError(timeoutMs, stdout, stderr)); + }, timeoutMs); } }); } -/** SFTP attrs surface used to build a SandboxFileEntry. */ +/** Minimal SFTP attrs surface, kept narrow so tests can fake it. */ export interface FileAttrsLike { size?: number; mode: number; @@ -104,7 +106,6 @@ export interface FileAttrsLike { isFile(): boolean; } -/** Map SFTP attrs onto a SandboxFileEntry. */ export function fileEntryFromAttrs( name: string, attrs: FileAttrsLike,