fix(python): raise ExecError for command start failures#958
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCommand-start failures are classified as execution errors across Rust, Go, and Python layers. Python wrappers translate them into ChangesExecution error handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutionInterface
participant PythonBinding
participant SimpleBox
ExecutionInterface->>PythonBinding: return BoxliteError::Execution(detail)
PythonBinding->>SimpleBox: raise CommandStartError(detail)
SimpleBox->>SimpleBox: derive exit code 126 or 127
SimpleBox-->>PythonBinding: raise ExecError(command, exit_code, detail)
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
📦 BoxLite review — looks good ·
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdks/python/src/util.rs (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a docstring to the exported exception.
As per coding guidelines, comprehensive docstrings should be written for all public classes. Since
CommandStartErroris exported to Python as a public exception class, consider providing a docstring.💡 Proposed refactor
-create_exception!(boxlite_python, CommandStartError, PyRuntimeError); +create_exception!(boxlite_python, CommandStartError, PyRuntimeError, "Raised when a command fails to start.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/src/util.rs` at line 4, Add a descriptive docstring to the exported Python exception created by CommandStartError, using the create_exception! declaration in boxlite_python. Ensure the resulting public exception class exposes documentation describing when command startup failures are raised.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@sdks/python/src/util.rs`:
- Line 4: Add a descriptive docstring to the exported Python exception created
by CommandStartError, using the create_exception! declaration in boxlite_python.
Ensure the resulting public exception class exposes documentation describing
when command startup failures are raised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae3f57a8-52f6-4b37-b2ef-f22532b656fc
📒 Files selected for processing (10)
sdks/python/boxlite/errors.pysdks/python/boxlite/exec.pysdks/python/boxlite/simplebox.pysdks/python/boxlite/sync_api/_simplebox.pysdks/python/src/box_handle.rssdks/python/src/lib.rssdks/python/src/util.rssdks/python/tests/test_simplebox.pysdks/python/tests/test_sync_simplebox.pysrc/boxlite/src/portal/interfaces/exec.rs
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
0e56100 to
e348e9f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
sdks/python/boxlite/simplebox.py (1)
234-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRevise the comment to explain why the code exists, not what it does.
The comment
# Execute via Rust (returns PyExecution)describes what the code does rather than why it exists. As per coding guidelines, write clear and concise comments explaining why code exists, not what it does. You can either remove it if the code is self-explanatory or update it to clarify the design intent (e.g.,# Delegate to the native extension for isolated execution).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/boxlite/simplebox.py` at line 234, Update or remove the comment immediately before the native execution call in the SimpleBox execution flow; if retained, state the design intent—delegating execution to the native extension for isolation—rather than describing the return type or implementation mechanics.Source: Coding guidelines
sdks/python/tests/test_errors.py (1)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a docstring to the test function.
As per coding guidelines, write comprehensive docstrings for all public functions and classes. Although this is a test function, it acts as a public API for the Pytest runner and falls under this requirement.
📝 Proposed fix
def test_command_start_exit_code(message, expected): + """Verify that command start failure messages map to the correct exit codes.""" assert _command_start_exit_code(message) == expected🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/tests/test_errors.py` around lines 26 - 27, Add a concise, comprehensive docstring to the test_command_start_exit_code test function describing its inputs and the expected exit-code assertion, while leaving the test logic unchanged.Source: Coding guidelines
sdks/python/tests/test_sync_simplebox.py (1)
85-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing assertion for stderr.
For completeness and consistency with the async equivalent
test_simplebox_command_not_found_raises_exec_error, consider asserting that the stderr contains an indicator that the command was not found.💡 Proposed fix
def test_command_not_found_raises_exec_error(self, shared_sync_runtime): """Direct command start failures raise ExecError, not bare RuntimeError.""" with SyncSimpleBox(image="alpine:latest", runtime=shared_sync_runtime) as box: with pytest.raises(ExecError) as exc: box.exec("definitely-not-a-boxlite-command-xyz") assert exc.value.exit_code == 127 + assert "not found" in exc.value.stderr.lower()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/tests/test_sync_simplebox.py` around lines 85 - 90, Extend test_command_not_found_raises_exec_error to assert that the captured ExecError stderr indicates the command was not found, matching the coverage in test_simplebox_command_not_found_raises_exec_error while preserving the existing exit_code assertion.sdks/python/tests/test_simplebox.py (1)
30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing docstrings to test methods.
As per coding guidelines, write comprehensive docstrings for all public functions and classes. Although these are test methods, providing docstrings ensures consistency with the rest of the test suite.
sdks/python/tests/test_simplebox.py#L30-L36: Add a docstring totest_simplebox_unexecutable_command_raises_exit_code_126.sdks/python/tests/test_sync_simplebox.py#L92-L103: Add a docstring totest_unexecutable_command_raises_exit_code_126.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/tests/test_simplebox.py` around lines 30 - 36, Add comprehensive docstrings describing the expected exit code and permission-denied behavior to test_simplebox_unexecutable_command_raises_exit_code_126 in sdks/python/tests/test_simplebox.py (lines 30-36) and test_unexecutable_command_raises_exit_code_126 in sdks/python/tests/test_sync_simplebox.py (lines 92-103), without changing their test logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@sdks/python/boxlite/simplebox.py`:
- Line 234: Update or remove the comment immediately before the native execution
call in the SimpleBox execution flow; if retained, state the design
intent—delegating execution to the native extension for isolation—rather than
describing the return type or implementation mechanics.
In `@sdks/python/tests/test_errors.py`:
- Around line 26-27: Add a concise, comprehensive docstring to the
test_command_start_exit_code test function describing its inputs and the
expected exit-code assertion, while leaving the test logic unchanged.
In `@sdks/python/tests/test_simplebox.py`:
- Around line 30-36: Add comprehensive docstrings describing the expected exit
code and permission-denied behavior to
test_simplebox_unexecutable_command_raises_exit_code_126 in
sdks/python/tests/test_simplebox.py (lines 30-36) and
test_unexecutable_command_raises_exit_code_126 in
sdks/python/tests/test_sync_simplebox.py (lines 92-103), without changing their
test logic.
In `@sdks/python/tests/test_sync_simplebox.py`:
- Around line 85-90: Extend test_command_not_found_raises_exec_error to assert
that the captured ExecError stderr indicates the command was not found, matching
the coverage in test_simplebox_command_not_found_raises_exec_error while
preserving the existing exit_code assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80696e48-763f-4161-8f07-d51b7631aaac
📒 Files selected for processing (15)
apps/runner/pkg/api/controllers/boxlite_exec.goapps/runner/pkg/api/controllers/boxlite_exec_test.gosdks/go/boxlite_test.gosdks/go/errors.gosdks/python/boxlite/errors.pysdks/python/boxlite/exec.pysdks/python/boxlite/simplebox.pysdks/python/boxlite/sync_api/_simplebox.pysdks/python/src/box_handle.rssdks/python/src/lib.rssdks/python/src/util.rssdks/python/tests/test_errors.pysdks/python/tests/test_simplebox.pysdks/python/tests/test_sync_simplebox.pysrc/boxlite/src/portal/interfaces/exec.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- sdks/go/boxlite_test.go
- sdks/go/errors.go
- src/boxlite/src/portal/interfaces/exec.rs
- sdks/python/src/lib.rs
- sdks/python/src/box_handle.rs
- apps/runner/pkg/api/controllers/boxlite_exec.go
- sdks/python/boxlite/sync_api/_simplebox.py
- sdks/python/boxlite/exec.py
- sdks/python/src/util.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test/e2e/cases/test_exec_comprehensive.py`:
- Around line 256-258: Update
scripts/test/e2e/cases/test_exec_comprehensive.py:256-258 to expect ExecError
from SimpleBox.exec instead of CommandStartError. Also update
scripts/test/e2e/cases/test_exec_comprehensive.py:278-281 to capture ExecError
as exc_info, optionally asserting its exit_code is 127.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: acdc4783-64a2-4ae9-b90d-d6b95a395468
📒 Files selected for processing (2)
scripts/test/e2e/cases/test_error_code_mapping.pyscripts/test/e2e/cases/test_exec_comprehensive.py
| def _command_start_exit_code(message: str) -> int: | ||
| """Translate a pre-exec spawn failure to the conventional shell code.""" | ||
| detail = message.casefold() | ||
| not_found_markers = ( | ||
| "not found in $path", | ||
| "no such file or directory", | ||
| "os error 2", | ||
| ) | ||
| return 127 if any(marker in detail for marker in not_found_markers) else 126 |
There was a problem hiding this comment.
🧹 exit-code heuristic can't distinguish cwd vs binary
guest spawn errors for missing cwd and missing binary both render as 'No such file or directory (os error 2)', so a nonexistent cwd is reported as ExecError exit_code=127 (command-not-found) even though the command exists.
| fn is_exec_start_failure(err: &BoxliteError) -> bool { | ||
| let BoxliteError::Internal(message) = err else { | ||
| return false; | ||
| }; | ||
| let detail = message.to_ascii_lowercase(); | ||
| detail.contains("spawn_failed") && detail.contains("failed to spawn") |
There was a problem hiding this comment.
is_exec_start_failure matches lowercase substrings 'spawn_failed' and 'failed to spawn' from the formatted error text built in portal/interfaces/exec.rs and guest/service/exec/executor.rs; a future wording change in either unrelated file silently downgrades ExecError back to a bare RuntimeError with no compile-time link or shared constant.
| ) | ||
| except _ExecStartError as e: | ||
| message = str(e) | ||
| raise ExecError(command_display, 126, message) from e |
There was a problem hiding this comment.
🧹 All exec-start failures hardcoded to exit_code 126
Command-not-found (ENOENT) and permission-denied (EACCES) both map to exit_code=126, diverging from POSIX convention (127 vs 126) that users may rely on when branching on exit codes; confirmed by tests asserting 126 for both cases in test_simplebox.py.
1 participant
Raise ExecError from Python SimpleBox when native exec startup fails before an execution handle exists.
Test plan: