refactor(sandbox): guard detached against output callbacks, clearer c…

[jamie-at-bunny]

• 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

[bogdan-at-bunny]

@codex review

[changeset-bot]

⚠️ No Changeset found

Latest commit: 73eb29b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 73eb29bc0f

ℹ️ 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".

[greptile-apps]

Greptile Summary

This PR tightens the sandbox API surface with a set of targeted guards and cleanups. The most user-facing change is that cp now fails early with a clear message when the local source is a directory, and runCommand rejects onStdout/onStderr when combined with detached instead of silently ignoring them.

• cp.ts: stat is called once (down from two calls), a directory-source guard is added, and the as RemoteRef casts are dropped by narrowing ref through control flow.
• sandbox.ts / transport.ts: The detached guard is extended to cover output callbacks, the error message is updated to point callers to command.logs(), and the inline chunk type is unified with the existing LogChunk export.
• transport.ts: The dead ?? 0 in the timeout rejection path is removed; the local timeoutMs binding makes the closure type-safe without the fallback.

Confidence Score: 4/5

Safe to merge. All changes are narrowly scoped guards and type cleanups with no behavioural regressions on the happy path.

The refactor is well-contained and the new tests cover the added validation. The one noteworthy wrinkle is in cp.ts where stat().catch(() => null) silently maps permission-denied errors to a 'file not found' message, which could confuse users who have a real EPERM situation. Everything else — the detached guard extension, the dead-code removal in collectExec, the LogChunk unification — is straightforward and correct.

packages/cli/src/commands/sandbox/cp.ts — the stat error handling deserves a second look; all other files are low-risk cleanup.

Important Files Changed

Filename	Overview
packages/cli/src/commands/sandbox/cp.ts	Refactors ref narrowing to eliminate as RemoteRef casts, adds a directory-source guard with a clear error, and consolidates the two stat calls into one. The stat().catch(() => null) pattern silently maps permission-denied errors to "file not found".
packages/sandbox/src/sandbox.ts	Extends the detached guard to cover onStdout/onStderr, updates the error message to mention command.logs(), and reuses LogChunk for the onData handler. Logic is correct and well-structured.
packages/sandbox/src/transport.ts	Replaces the duplicated inline chunk type with the shared LogChunk import and removes the dead ?? 0 fallback safely inside the if (timeoutMs !== undefined) block.
packages/sandbox/src/sandbox.test.ts	Adds two new assertions covering onStdout-only and onStderr-only with detached. Both exercise the guard synchronously before any network call, consistent with the existing test pattern.
packages/cli/README.md	Documentation updated to mention the directory-copy restriction, matching the new runtime behaviour in cp.ts.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[sandboxCpCommand handler] --> B{Both src and dest remote?}
    B -- Yes --> C[Throw: sandbox-to-sandbox not supported]
    B -- No --> D[ref = srcRemote ?? destRemote]
    D --> E{ref is null?}
    E -- Yes --> F[Throw: one path must reference a sandbox]
    E -- No --> G[uploading = Boolean destRemote]
    G --> H{uploading?}
    H -- Yes: local to sandbox --> I[stat local file]
    I --> J{stat failed?}
    J -- Yes --> K[Throw: Local file not found]
    J -- No --> L{isDirectory?}
    L -- Yes --> M[Throw: X is a directory. Only single files can be copied.]
    L -- No --> N[Read file content + mode from stat]
    N --> O[writeFiles to sandbox]
    H -- No: sandbox to local --> P[readFile from sandbox via ref.path]
    P --> Q{content null?}
    Q -- Yes --> R[Throw: File not found in sandbox]
    Q -- No --> S{dest ends with slash or isDirectory?}
    S -- Yes --> T[join dest with basename of ref.path]
    S -- No --> U[dest as-is]
    T --> V[Bun.write localPath content]
    U --> V
    O --> W[Log: Copied from to]
    V --> W

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"}}}%%
flowchart TD
    A[sandboxCpCommand handler] --> B{Both src and dest remote?}
    B -- Yes --> C[Throw: sandbox-to-sandbox not supported]
    B -- No --> D[ref = srcRemote ?? destRemote]
    D --> E{ref is null?}
    E -- Yes --> F[Throw: one path must reference a sandbox]
    E -- No --> G[uploading = Boolean destRemote]
    G --> H{uploading?}
    H -- Yes: local to sandbox --> I[stat local file]
    I --> J{stat failed?}
    J -- Yes --> K[Throw: Local file not found]
    J -- No --> L{isDirectory?}
    L -- Yes --> M[Throw: X is a directory. Only single files can be copied.]
    L -- No --> N[Read file content + mode from stat]
    N --> O[writeFiles to sandbox]
    H -- No: sandbox to local --> P[readFile from sandbox via ref.path]
    P --> Q{content null?}
    Q -- Yes --> R[Throw: File not found in sandbox]
    Q -- No --> S{dest ends with slash or isDirectory?}
    S -- Yes --> T[join dest with basename of ref.path]
    S -- No --> U[dest as-is]
    T --> V[Bun.write localPath content]
    U --> V
    O --> W[Log: Copied from to]
    V --> W

Loading

_{Reviews (1): Last reviewed commit: "refactor(sandbox): guard detached agains..." | Re-trigger Greptile}

[greptile-apps]

The stat(local).catch(() => null) call swallows all errors, not just ENOENT. If the user lacks read permission on the file, info will be null and the error message will say "Local file not found" instead of something like "Permission denied". Narrowing the catch to only ENOENT and re-throwing everything else would surface a more accurate message.

Suggested change

	const info = await stat(local).catch(() => null);
	if (!info) {
	throw new UserError(`Local file not found: ${local}`);
	}
	const info = await stat(local).catch((err: NodeJS.ErrnoException) => {
	if (err.code === "ENOENT") return null;
	throw err;
	});
	if (!info) {
	throw new UserError(`Local file not found: ${local}`);
	}