sandbox: Align create_process(stdout=None) to stdlib - #345
Open
scotttrinh wants to merge 4 commits into
Open
Conversation
Match Popen and run_process by inheriting output by default. Accept text streams so callers can stream directly without manually consuming readers. Drain the log transport before reporting completion so inherited streams receive all output and PIPE readers reach EOF reliably.
There was a problem hiding this comment.
Pull request overview
This PR updates vercel-sandbox process output handling to align create_process() with subprocess.Popen semantics: stdout/stderr are inherited by default, callers can pass writable text streams directly, and process completion now drains the log transport to ensure output delivery and reliable EOF for PIPE readers.
Changes:
- Default
create_process(stdout=None, stderr=None)now inherits local stdout/stderr unless explicitly set toPIPE/DEVNULL/STDOUT. - Extend output routing to support writable
TextIOdestinations in addition to subprocess sentinels. - Add log-transport draining behavior before reporting process completion; update tests + add an example + news fragment.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vercel-sandbox/vercel/sandbox/_internal/text_reader.py | Adds TextIO destinations, transport draining, and inherited-output routing in the log reader core. |
| src/vercel-sandbox/vercel/sandbox/_internal/async_runtime.py | Switches process handles to retain a transport and drains output in wait()/communicate(); changes create_process defaults to inherited output. |
| src/vercel-sandbox/vercel/sandbox/_internal/sync_runtime.py | Sync equivalent of async runtime changes (transport retention + drain + inherited defaults). |
| src/vercel-sandbox/vercel/sandbox/_internal/process_output.py | Updates live-stream destination validation to accept `TextIO |
| src/vercel-sandbox/tests/test_sandbox_text_reader.py | Adds regression tests ensuring closing one reader doesn’t break non-buffer destinations; updates imports. |
| src/vercel-sandbox/tests/test_sandbox_public_flow.py | Adjusts test to explicitly request PIPE now that defaults inherit output. |
| src/vercel-sandbox/tests/test_sandbox_process.py | Adds tests for inherited-default behavior and updates existing tests to request PIPE explicitly. |
| src/vercel-sandbox/examples/sandbox_08_create_process.py | New example demonstrating inherited vs captured output. |
| changes/vercel-sandbox/create-process-output.feature.md | News fragment documenting the behavioral change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+117
to
+121
| @@ -118,7 +118,7 @@ class Process(_ProcessHandleState): | |||
| ``subprocess.DEVNULL`` or merged with ``subprocess.STDOUT``. | |||
| """ | |||
|
|
|||
| __slots__ = ("_service", "stderr", "stdout") | |||
| __slots__ = ("_service", "_transport", "stderr", "stdout") | |||
Comment on lines
+120
to
+124
| @@ -118,7 +121,7 @@ class SyncProcess(_ProcessHandleState): | |||
| ``subprocess.DEVNULL`` or merged with ``subprocess.STDOUT``. | |||
| """ | |||
|
|
|||
| __slots__ = ("_service", "stderr", "stdout") | |||
| __slots__ = ("_service", "_transport", "stderr", "stdout") | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Match
Popenandrun_processby inheriting output by default. Accept text streams so callers can stream directly without manually consuming readers. Drain the log transport before reporting completion so inherited streams receive all output andPIPEreaders reach EOF reliably.