Skip to content

Conversation

@whoiskatrin
Copy link
Collaborator

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Dec 10, 2025

🦋 Changeset detected

Latest commit: d639183

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

This PR includes changesets to release 1 package
Name Type
@cloudflare/sandbox Patch

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 10, 2025

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/sandbox-sdk/@cloudflare/sandbox@289

commit: d639183

@github-actions
Copy link
Contributor

github-actions bot commented Dec 10, 2025

🐳 Docker Images Published

Default (no Python):

FROM cloudflare/sandbox:0.0.0-pr-289-a50189d

With Python:

FROM cloudflare/sandbox:0.0.0-pr-289-a50189d-python

Version: 0.0.0-pr-289-a50189d

Use the -python variant if you need Python code execution.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

Fixes a bug in the workspace related to sandbox functionality.
@whoiskatrin whoiskatrin marked this pull request as ready for review December 10, 2025 17:56
Copy link
Member

@ghostwriternr ghostwriternr left a comment

Choose a reason for hiding this comment

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

Good core fix, but the tests are overly verbose and redundant. We should be able to have the same level of guards with much more minimal tests.

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

Copy link
Member

@ghostwriternr ghostwriternr left a comment

Choose a reason for hiding this comment

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

minor nits, lgtm otherwise!

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

All previous feedback has been addressed - the fallback now uses home directory and tests are comprehensive.

Critical Issue: Changeset Description

The changeset description is too vague for release notes:

fix workspace bug

Users need to understand what was fixed. Update to:

---
"@cloudflare/sandbox": patch
---

Fix session initialization failure when /workspace directory doesn't exist

Session creation now falls back to the home directory when the specified
working directory (default: /workspace) doesn't exist, preventing
"Unknown Error" failures. This resolves issues with deleted or unmounted
workspace directories.

Implementation Review

The core fix is solid:

  • stat() check correctly handles symlinks (follows them by default)
  • ✅ Fallback to home directory is appropriate for shell sessions
  • ✅ Debug logging provides troubleshooting context
  • ✅ Test coverage is excellent (unit + E2E)

Minor Suggestions

Documentation clarity (session.ts:49-56): The comment "This only affects where the shell starts" is misleading. The shell's cwd persists and affects subsequent commands until changed. Consider:

/**
 * Initial working directory for the shell.
 *
 * This determines where the shell starts and becomes the default working
 * directory for subsequent commands (until changed via cd). Individual
 * commands can override this via exec options.
 * 
 * If the specified directory doesn't exist when the session initializes,
 * the session will fall back to the home directory.
 */

Config constant (session.ts:149): Consider extracting process.env.HOME || '/root' to CONFIG for consistency with DEFAULT_CWD.

Verdict: Approve after changeset description is updated.

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

The fallback logic is sound and tests are comprehensive. However, three items from the previous review were marked resolved but not actually implemented:

  1. Changeset description still says "fix workspace bug" (too vague)
  2. homeDir constant not extracted to config.ts
  3. Missing comment about what happens if homeDir doesn't exist

Please address these before merging.

@whoiskatrin whoiskatrin merged commit 67100d0 into main Dec 10, 2025
19 of 20 checks passed
@whoiskatrin whoiskatrin deleted the fix-exec-workspace-bug branch December 10, 2025 19:07
@github-actions github-actions bot mentioned this pull request Dec 10, 2025
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