Skip to content

feat(sandbox): runCommand timeout/AbortSignal and file ops#122

Open
jamie-at-bunny wants to merge 3 commits into
mainfrom
feat/sandbox-timeouts-file-ops
Open

feat(sandbox): runCommand timeout/AbortSignal and file ops#122
jamie-at-bunny wants to merge 3 commits into
mainfrom
feat/sandbox-timeouts-file-ops

Conversation

@jamie-at-bunny

Copy link
Copy Markdown
Member

@amir-at-bunny I'm experimenting with this for project "rabbit's foot".

@bogdan-at-bunny

Copy link
Copy Markdown

@codex review

@changeset-bot

changeset-bot Bot commented Jul 11, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9d50346

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@bunny.net/sandbox Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@greptile-apps

greptile-apps Bot commented Jul 11, 2026

Copy link
Copy Markdown

Greptile Summary

This PR extends the @bunny.net/sandbox SDK with two main capabilities: runCommand now accepts a timeout (ms) and an AbortSignal that kill the remote process and reject with CommandTimeoutError or the signal's reason respectively; and four new SFTP-backed file operations (listFiles, deleteFile, rename, exists) are exposed on the Sandbox class. The implementation is well-tested, with dedicated unit tests for the new collectExec helper and fileEntryFromAttrs classifier.

  • Timeout/cancellation (collectExec): registered data listeners outside the Promise executor, with a clean cleanup/kill path for both timer and abort cases; pre-aborted signal is handled before the timer is set, so clearTimeout(undefined) is a safe no-op.
  • New file ops: readDir/unlink/rename/stat added to SshTransport; rename adds an lstat pre-check to block silent OpenSSH overwrites, with the TOCTOU window acknowledged in code comments.
  • exists vs rename guard mismatch: exists calls sftp.stat (follows symlinks) while the rename guard calls sftp.lstat — a dangling symlink returns false from exists but blocks rename with "already exists", breaking the natural check-then-rename pattern.

Confidence Score: 4/5

Safe to merge with the exists/rename symlink inconsistency noted — it is unlikely to bite real users immediately but will surprise any caller who checks exists before renaming onto a dangling symlink.

The exists method uses sftp.stat (follows symlinks) while the rename guard uses sftp.lstat. A dangling symlink makes exists(path) return false but rename(x, path) throw "already exists" — a check-then-act pattern that the public API naturally invites will silently fail. Everything else in the PR is well-structured and covered by tests.

packages/sandbox/src/transport.ts — the stat method at line 250 and the rename/lexists guard at line 221 use different symlink-following semantics that create an observable inconsistency in the public API.

Important Files Changed

Filename Overview
packages/sandbox/src/transport.ts New collectExec function (timeout/signal), plus readDir/unlink/rename/stat/lexists SFTP ops. A symlink-follow inconsistency between stat (used by exists) and lstat (used by the rename guard) creates a surprising public API contract.
packages/sandbox/src/sandbox.ts Added listFiles, deleteFile, rename, exists delegates and validation for timeout/signal in runCommand. Logic is clean; validation correctly rejects non-positive timeouts and detached+timeout/signal combos.
packages/sandbox/src/sandbox.test.ts New unit tests for collectExec (timeout, pre-aborted, mid-flight abort), fileEntryFromAttrs, and runCommand validation. Tests are thorough and use a well-designed fake stream interface.
packages/sandbox/src/errors.ts Adds CommandTimeoutError extending SandboxError, carrying timeoutMs, stdout, and stderr. Clean implementation.
packages/sandbox/src/types.ts Adds timeout and signal to RunCommandOptions, and new SandboxFileEntry interface. Straightforward type additions.
packages/sandbox/src/integration.test.ts Integration tests for listFiles, rename (conflict detection), exists, deleteFile, and a 2-second timeout scenario. Coverage is good.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant Sandbox
    participant SshTransport
    participant collectExec
    participant RemoteProcess

    Note over Caller,RemoteProcess: Blocking runCommand with timeout
    Caller->>Sandbox: "runCommand({ cmd, timeout: 5000 })"
    Sandbox->>Sandbox: "validate timeout > 0 and !detached"
    Sandbox->>SshTransport: "exec(remote, { timeoutMs: 5000 })"
    SshTransport->>RemoteProcess: execStream(command)
    SshTransport->>collectExec: "collectExec(stream, { timeoutMs: 5000 })"
    collectExec->>collectExec: register data/stderr listeners
    collectExec->>collectExec: setTimeout(5000) → timer

    alt Command finishes before timeout
        RemoteProcess-->>collectExec: data events (stdout/stderr)
        RemoteProcess-->>collectExec: close(exitCode)
        collectExec->>collectExec: clearTimeout(timer)
        collectExec-->>Sandbox: "{ stdout, stderr, exitCode }"
        Sandbox-->>Caller: CommandFinished
    else Timeout fires first
        collectExec->>collectExec: timer fires
        collectExec->>collectExec: cleanup() + kill()
        collectExec->>RemoteProcess: signal(KILL) + close()
        collectExec-->>Sandbox: reject(CommandTimeoutError)
        Sandbox-->>Caller: throws CommandTimeoutError
    else AbortSignal fires
        Caller->>collectExec: controller.abort(reason)
        collectExec->>collectExec: onAbort() → cleanup() + kill()
        collectExec->>RemoteProcess: signal(KILL) + close()
        collectExec-->>Sandbox: reject(signal.reason)
        Sandbox-->>Caller: throws abort reason
    end

    Note over Caller,RemoteProcess: rename with lstat guard
    Caller->>Sandbox: rename(from, to)
    Sandbox->>SshTransport: rename(from, to)
    SshTransport->>SshTransport: lexists(to) via sftp.lstat
    alt destination exists (including dangling symlinks)
        SshTransport-->>Caller: SandboxError(already exists)
    else destination absent
        SshTransport->>RemoteProcess: sftp.rename(from, to)
        SshTransport-->>Caller: void
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant Sandbox
    participant SshTransport
    participant collectExec
    participant RemoteProcess

    Note over Caller,RemoteProcess: Blocking runCommand with timeout
    Caller->>Sandbox: "runCommand({ cmd, timeout: 5000 })"
    Sandbox->>Sandbox: "validate timeout > 0 and !detached"
    Sandbox->>SshTransport: "exec(remote, { timeoutMs: 5000 })"
    SshTransport->>RemoteProcess: execStream(command)
    SshTransport->>collectExec: "collectExec(stream, { timeoutMs: 5000 })"
    collectExec->>collectExec: register data/stderr listeners
    collectExec->>collectExec: setTimeout(5000) → timer

    alt Command finishes before timeout
        RemoteProcess-->>collectExec: data events (stdout/stderr)
        RemoteProcess-->>collectExec: close(exitCode)
        collectExec->>collectExec: clearTimeout(timer)
        collectExec-->>Sandbox: "{ stdout, stderr, exitCode }"
        Sandbox-->>Caller: CommandFinished
    else Timeout fires first
        collectExec->>collectExec: timer fires
        collectExec->>collectExec: cleanup() + kill()
        collectExec->>RemoteProcess: signal(KILL) + close()
        collectExec-->>Sandbox: reject(CommandTimeoutError)
        Sandbox-->>Caller: throws CommandTimeoutError
    else AbortSignal fires
        Caller->>collectExec: controller.abort(reason)
        collectExec->>collectExec: onAbort() → cleanup() + kill()
        collectExec->>RemoteProcess: signal(KILL) + close()
        collectExec-->>Sandbox: reject(signal.reason)
        Sandbox-->>Caller: throws abort reason
    end

    Note over Caller,RemoteProcess: rename with lstat guard
    Caller->>Sandbox: rename(from, to)
    Sandbox->>SshTransport: rename(from, to)
    SshTransport->>SshTransport: lexists(to) via sftp.lstat
    alt destination exists (including dangling symlinks)
        SshTransport-->>Caller: SandboxError(already exists)
    else destination absent
        SshTransport->>RemoteProcess: sftp.rename(from, to)
        SshTransport-->>Caller: void
    end
Loading

Reviews (3): Last reviewed commit: "fix(sandbox): rename guard uses lstat so..." | Re-trigger Greptile

Comment thread packages/sandbox/src/sandbox.ts
Comment thread packages/sandbox/src/transport.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7fe65490f8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

});
});
async exec(command: string, limits?: ExecLimits): Promise<ExecResult> {
return collectExec(await this.execStream(command), limits);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor AbortSignal before opening SSH exec

When limits.signal is already aborted, or aborts while the SSH channel is being opened, this still awaits execStream(command) first, so the remote command can be created and begin running before collectExec notices the cancellation. For callers that pass a shared or timed-out signal to prevent side effects, commands like file mutations or network calls can still execute briefly; check/race the signal before and during channel creation instead of only after the exec request succeeds.

Useful? React with 👍 / 👎.

async runCommand(
command: RunCommandOptions & { detached: true },
): Promise<Command>;
async runCommand(command: RunCommandOptions): Promise<CommandFinished>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve union return for dynamic detached options

When callers build an options object in a variable typed as RunCommandOptions (for example from a helper or config) and set detached: true, overload resolution falls through to this catch-all and types the result as CommandFinished, even though the runtime branch still returns a live Command. That lets consumer code type-check while treating a still-running command as finished (or prevents access to logs()/kill() without casts); narrow this overload to non-detached options or return the union for dynamic inputs.

Useful? React with 👍 / 👎.

Comment on lines +62 to +65
/** 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Split detached command options from blocking limits

Because timeout and signal live on the same public RunCommandOptions interface as detached, calls such as sandbox.runCommand({ cmd: "sleep", detached: true, timeout: 1000 }) type-check and even select the Promise<Command> overload, but the runtime validation throws for every such call. SDK users will only discover this unsupported combination at runtime; model the options as blocking-vs-detached unions so timeout/signal are not accepted with detached: true.

Useful? React with 👍 / 👎.

@jamie-at-bunny

Copy link
Copy Markdown
Member Author

@codex re-review

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants