Avoid native side effects during computer-server imports - #1569
Avoid native side effects during computer-server imports#15690xjohnnydev wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📦 Publishable packages changed
Add |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR defers module imports and removes initialization side effects across the Python computer server package. Package-level ChangesLazy Loading and Import Safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@libs/python/computer-server/tests/test_import_safety.py`:
- Around line 1-14: The test helper _assert_import_exits_cleanly inherits the
parent environment causing pre-set CUA_* variables to affect imports; fix it by
creating a sanitized env for subprocess.run (import os), e.g. copy os.environ
then remove any keys that start with "CUA_" (or explicitly pop "CUA_BACKEND" and
"CUA_VNC_*"), and pass that filtered env via the env= parameter to
subprocess.run so the child process runs in an isolated environment.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e833ae39-e356-415e-b820-1697b9b8357e
📒 Files selected for processing (4)
libs/python/computer-server/computer_server/__init__.pylibs/python/computer-server/computer_server/handlers/factory.pylibs/python/computer-server/computer_server/handlers/macos.pylibs/python/computer-server/tests/test_import_safety.py
| import platform | ||
| import subprocess | ||
| import sys | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| def _assert_import_exits_cleanly(import_statement: str): | ||
| result = subprocess.run( | ||
| [sys.executable, "-X", "faulthandler", "-c", f"{import_statement}; print('ok')"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=20, | ||
| ) |
There was a problem hiding this comment.
Isolate subprocess env to prevent flaky import-safety tests.
Line 9 currently inherits the parent environment, so pre-set CUA_BACKEND / CUA_VNC_* can alter HandlerFactory.create_handlers() and fail this test for non-import-safety reasons.
🔧 Suggested fix
+import os
import platform
import subprocess
import sys
@@
def _assert_import_exits_cleanly(import_statement: str):
+ env = os.environ.copy()
+ for key in ("CUA_BACKEND", "CUA_VNC_HOST", "CUA_VNC_PORT", "CUA_VNC_PASSWORD"):
+ env.pop(key, None)
+
result = subprocess.run(
[sys.executable, "-X", "faulthandler", "-c", f"{import_statement}; print('ok')"],
capture_output=True,
text=True,
timeout=20,
+ env=env,
)🤖 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 `@libs/python/computer-server/tests/test_import_safety.py` around lines 1 - 14,
The test helper _assert_import_exits_cleanly inherits the parent environment
causing pre-set CUA_* variables to affect imports; fix it by creating a
sanitized env for subprocess.run (import os), e.g. copy os.environ then remove
any keys that start with "CUA_" (or explicitly pop "CUA_BACKEND" and
"CUA_VNC_*"), and pass that filtered env via the env= parameter to
subprocess.run so the child process runs in an isolated environment.
📦 Publishable packages changed
Add |
8c0c94b to
583ea92
Compare
📦 Publishable packages changed
Add |
|
Closing this for now to keep the current review queue focused on Dependabot remediation. The branch and commits are being preserved so this code-security work can be reopened or superseded when we return to that backlog. |
1 participant
Summary
Serverfromcomputer_serverinstead of importing server code at package import timeHandlerFactory.create_handlers()Testing
cd libs/python/computer-server && uv run --frozen --with pytest --with pytest-asyncio pytest tests/test_import_safety.pySummary by CodeRabbit
Refactor
Tests