fix(sandbox): omit --user when os.getuid is unavailable - #585
fix(sandbox): omit --user when os.getuid is unavailable#585kurosawareiji7007-hub wants to merge 1 commit into
Conversation
windows has no getuid/getgid; building sandboxed docker argv raised AttributeError before docker ran. Fixes vouchdev#582 Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughChangesSandbox Docker argv
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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 `@tests/test_sandbox.py`:
- Around line 57-61: Update the test around the sandbox user-argument assertions
to use callable(getattr(...)) for both os.getuid and os.getgid, matching the
implementation’s availability check. Parameterize the regression cases so getuid
and getgid are removed or made unavailable independently, covering each
partial-availability combination instead of only removing both together.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 99446498-5cb8-431c-a2e0-532b5a260b2a
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/sandbox.pytests/test_sandbox.py
| if hasattr(os, "getuid") and hasattr(os, "getgid"): | ||
| assert "--user" in argv | ||
| assert f"{os.getuid()}:{os.getgid()}" in argv | ||
| else: | ||
| assert "--user" not in argv |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
make the test matrix match the implementation’s availability check
The implementation checks that both functions are callable, but the existing assertion only uses hasattr, and the regression test removes both functions together. This misses cases where only getuid or getgid is unavailable or non-callable. Use the same callable(getattr(...)) predicate and parameterize the regression for each function independently.
Also applies to: 78-85
🤖 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 `@tests/test_sandbox.py` around lines 57 - 61, Update the test around the
sandbox user-argument assertions to use callable(getattr(...)) for both
os.getuid and os.getgid, matching the implementation’s availability check.
Parameterize the regression cases so getuid and getgid are removed or made
unavailable independently, covering each partial-availability combination
instead of only removing both together.
what changed
sandbox docker argv only adds
--user uid:gidwhenos.getuid/os.getgidexist. on windows the flag is omitted so argv construction no longer raises.why
sandboxed dual-solve crashed with
AttributeErroron windows before docker ever ran (#582).validation
Fixes #582
Summary by CodeRabbit
Bug Fixes
Documentation