fix: preserve newlines in heredoc commands for exec()

[deathbyknowledge]

Fixes heredoc commands hanging in foreground execution by wrapping commands with newlines instead of placing them on a single line. Heredoc terminators must appear alone on their own line to work correctly.

Fixes #139

🤖 Generated with Claude Code

[changeset-bot]

🦋 Changeset detected

Latest commit: 6d38e26

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

This PR includes changesets to release 2 packages

Name	Type
@repo/sandbox-container	Patch
@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

[claude]

PR Review: Fix heredoc commands hanging in foreground execution

Summary

This PR fixes a critical bug where heredoc commands would hang when using exec(). The fix properly preserves newlines by wrapping commands in braces across multiple lines instead of placing them on a single line.

Code Quality & Best Practices ✅

Strengths:

• Correct fix: The solution properly addresses the root cause. Bash heredoc terminators must appear alone on their own line.
• Consistent application: The fix is applied to both locations where foreground commands are executed (with/without cwd).
• Minimal change: The fix is surgical and does not introduce unnecessary complexity.
• Clear commit message: Good description of the problem and solution.

Potential Issues & Considerations

1. Background mode consistency
The background execution mode already had the command on its own line. This PR updates the foreground mode to match, which is good. However, there is a slight inconsistency in indentation between modes.

2. Edge case: commands with trailing newlines
If a command already ends with newlines, this change adds an additional newline. This should still work correctly but creates extra blank lines.

3. Commands with leading/trailing whitespace
Ensure that command is properly validated elsewhere to not have leading newlines that could break the brace syntax.

Performance Considerations ✅

• Negligible impact: Adding newlines has no measurable performance impact.
• No additional process spawning: The change is purely syntactic.

Security Concerns ✅

• No new security issues: The change does not introduce command injection vulnerabilities.
• Command escaping: The escapeShellPath is still used for cwd, which is good.

Test Coverage ⚠️

Current state:
Looking at packages/sandbox-container/tests/session.test.ts, I do not see any tests specifically for heredoc commands. The test suite includes simple commands, multi-line output, shell pipes, and shell functions, but no heredoc tests.

Recommendation:
Add test cases for heredoc commands to prevent regression.

Recommendations

1. Add test coverage: Include heredoc-specific tests to ensure this bug does not regress.
2. Consider background mode consistency: Review if formatting should be more consistent.
3. Documentation: Consider adding a comment explaining why the command must be on its own line.

Verdict: ✅ APPROVE with suggestions

The fix is correct and solves the reported issue. Code quality is good with no security concerns. The main improvement would be adding test coverage for heredoc commands.

---

Generated with Claude Code

[claude]

Pull Request Review

Summary

This PR fixes a critical bug where heredoc commands would hang during foreground execution (exec). The fix properly preserves newlines by placing heredoc commands on separate lines instead of forcing them onto a single line, which is required for bash to correctly recognize heredoc terminators.

---

Code Quality & Best Practices ✅

Strengths:

• Correct fix: The solution properly addresses the root cause. Heredoc terminators must appear alone on their own line to work correctly in bash.
• Consistent pattern: The fix is applied to both code paths (with and without cwd), maintaining consistency.
• Clear intent: The change makes the generated shell script more readable and follows bash best practices.
• Good documentation: The PR description clearly explains the problem and links to the related issue.

Code change analysis:

// Before (problematic):
script += `  { ${command}; } > >(...)`;

// After (fixed):
script += `  {\n`;
script += `${command}\n`;
script += `  } > >(...)`;

This ensures commands with heredocs are properly formatted with the terminator on its own line.

---

Potential Issues & Edge Cases ⚠️

1. Workflow file change appears unrelated

The commented-out paths filter in .github/workflows/pkg-pr-new.yml seems unrelated to the heredoc fix. This was likely done to test the PR workflow, but should be reverted:

# These lines are commented out:
#paths:
#  - '!**/*.md'
#  - '!.changeset/**'

Recommendation: Revert this change or clarify why it is needed in a separate commit with explanation.

2. Consider edge cases for command formatting

The fix assumes commands do not need additional escaping when moved to separate lines. Consider testing:

• Commands with trailing backslashes
• Commands with embedded newlines
• Multi-line commands with line continuations

While the current implementation should handle these correctly, explicit testing would provide confidence.

3. Indentation consistency

The command line has no indentation. While this works correctly, consider if it should match the surrounding indentation for consistency in the generated script. This is a minor aesthetic point and does not affect functionality.

---

Performance Considerations ✅

Impact: Neutral to Positive

• No performance regression: The change only affects how the script is constructed, not execution performance.
• Potential improvement: Commands with heredocs will now execute correctly instead of hanging indefinitely.
• String concatenation: Using multiple script += statements is fine for this use case. The performance impact is negligible.

---

Security Considerations ✅

No new security concerns introduced.

• Path escaping preserved: The fix does not touch the escapeShellPath logic at session.ts:890-893, so path injection protection remains intact.
• Command interpolation unchanged: Commands are still interpolated using the command variable without additional modification, maintaining the existing security posture.
• No new injection vectors: Moving the command to separate lines does not introduce new shell injection opportunities.

Note: The existing code relies on proper sanitization of command before it reaches this point. This PR does not change that security model.

---

Test Coverage 📋

Current state: Limited heredoc-specific testing

Observations:

• ✅ Existing e2e tests cover general exec functionality
• ✅ Tests verify session state isolation and concurrent execution
• ❌ No specific test for heredoc commands found in the test suite

Recommendations:

1. Add a heredoc-specific test

Consider adding a test case in the e2e tests to verify heredoc commands work correctly and do not hang.

2. Test edge cases

• Nested heredocs
• Heredocs with different terminators (<<EOF, <<-EOF, <<"EOF")
• Commands mixing heredocs with other shell features

---

Recommendations

Must Address:

1. Revert or explain the workflow file change - The .github/workflows/pkg-pr-new.yml modification appears unrelated to the fix.

Should Consider:

2. Add heredoc-specific test - This would prevent regression and document the expected behavior.
3. Test with edge cases - Verify the fix works with various heredoc formats and complex commands.

Nice to Have:

4. Document the limitation - Add a code comment explaining why commands must be on separate lines (for heredoc support).

---

Overall Assessment

Status: Approve with minor suggestions ✅

This is a well-targeted fix that addresses a real bug. The core change is correct and improves the reliability of the exec functionality. The main concern is the unrelated workflow file change, which should be addressed before merging.

Code quality: Good
Bug fix correctness: Excellent
Test coverage: Could be improved
Security: No concerns
Performance: No concerns

Great work identifying and fixing this issue! The explanation in the issue and PR description made it easy to understand the problem and verify the solution.

[pkg-pr-new]

Open in StackBlitz

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

commit: 4d504b7

[github-actions]

🐳 Docker Image Published

FROM cloudflare/sandbox:0.0.0-pr-140-0cb42aa

Version: 0.0.0-pr-140-0cb42aa

You can use this Docker image with the preview package from this PR.