fix(python): return only stdout from CodeBox.run()#248
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts the Python SDK’s CodeBox.run() convenience API to avoid leaking container/runtime warnings (e.g., seccomp/libcontainer warnings on macOS) into the returned string by returning stdout only.
Changes:
- Update
CodeBox.run()to return onlyresult.stdout(instead ofstdout + stderr). - Update
CodeBox.run()docstring to reflect the new stdout-only behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Execute Python code using python3 -c | ||
| result = await self.exec("/usr/local/bin/python", "-c", code) | ||
| return result.stdout + result.stderr | ||
| return result.stdout |
There was a problem hiding this comment.
run() now returns only result.stdout unconditionally. This will drop Python tracebacks/syntax errors (which are written to stderr) and will break existing behavior/tests that rely on error output being present (e.g., exceptions, NameError). Consider handling non-zero result.exit_code by either (a) raising an exception that includes result.stderr (and possibly error_message) or (b) returning stdout + stderr only on failure, while keeping stdout-only on success to avoid the macOS seccomp warning noise.
| Returns: | ||
| Execution output as a string (stdout + stderr) | ||
| Execution stdout as a string. Use exec() directly if you need | ||
| both stdout and stderr. |
There was a problem hiding this comment.
The docstring now states callers should use exec() if they need stderr, but run_script() delegates to run() and the sync wrapper (SyncCodeBox) is documented as mirroring the async API. To avoid inconsistent behavior across these convenience APIs, consider updating the related wrappers/docs (and any tests) to match the new stdout-only contract, or adopt an error-path exception/combined-output strategy as suggested below.
yingjunwu
force-pushed
the
fix/codebox-run-stdout-only
branch
from
e21265b to
23e83f5
Compare
|
Self-review after revision: This PR is now correct. The initial version had 3 bugs that have been fixed: Bug 1 — Broke tests. Three async tests and one sync test asserted that errors (ValueError, SyntaxError, NameError) appear in Bug 2 — SyncCodeBox not updated. Bug 3 — Stale run_script() docstring. Dependency note: If PR #249 is also merged, |
CodeBox.run() returned result.stdout + result.stderr, which caused seccomp/libcontainer warnings to leak into the returned string: 'hello\n\x1b[2m...\x1b[0m \x1b[33m WARN\x1b[0m ...seccomp not available...\n' This breaks any code that parses run() output, especially in the primary AI agent use case where LLMs need clean output. Fix: return only result.stdout. Users who need stderr can use exec() directly and access result.stderr separately. Closes #237 Co-Authored-By: Claude Opus 4.6 <[email protected]>
yingjunwu
force-pushed
the
fix/codebox-run-stdout-only
branch
from
23e83f5 to
0c5d12d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
CodeBox.run()to return only stdout (previously returned stdout + stderr)SyncCodeBox.run()to match (same change)run_script()docstrings in both async and sync APIsrun()output to useexec()insteadCloses #237
Note
If PR #249 is also merged,
run_guest_script()should be updated to return stdout only for consistency.Test plan
run()returns only stdout for successful coderun()outputexec()still returns both stdout and stderr🤖 Generated with Claude Code