feat: add process isolation for tool execution#410
Conversation
Add comprehensive process isolation feature using xoscar to ensure tools execute in isolated processes for improved security and reliability. Core changes: - Add ProcessService with dynamic sub-pool creation for tool execution - Add ToolExecutorActor for generic tool execution in isolated processes - Add PythonExecutorActor for Python code execution in isolated processes - Add ProcessIsolatedToolWrapper to automatically wrap tools with isolation Testing and validation: - Add comprehensive integration tests to verify Python executor runs in xoscar isolated processes (test_python_executor_process_isolation.py) - Add ProcessService lifecycle and execution tests - Add ProcessIsolatedToolWrapper tests with state isolation verification - Verify process ID separation between main and isolated processes Configuration and dependencies: - Add xoscar>=0.9.0 to core dependencies - Update mypy configuration to handle xoscar type stubs - Fix import paths for process isolation components Compatibility fixes: - Adapt ProcessService to new xoscar API (remove address/n_workers params) - Fix async/sync execution handling in ToolExecutorActor - Add proper type annotations and error handling - Ensure proper cleanup of actors and sub-pools after execution The implementation ensures: - Complete process isolation for tool execution - Proper resource cleanup (actors and sub-pools) - State isolation between executions - Support for concurrent tool execution - Compatibility with existing tool system
There was a problem hiding this comment.
Code Review
This pull request introduces a process-level isolation framework for tool execution using the xoscar library, including new executor actors, a managed ProcessService, and integration wrappers for existing tools. Feedback highlights several critical issues: a potential RuntimeError caused by improper use of asyncio.run() in an async context, resource leaks in sub-pool cleanup logic, and multiple API mismatches between the implementation and the test suite, such as missing methods and incorrect constructor signatures. Additionally, a duplicate return statement was identified in the tool executor actor.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces process-level isolation for tool execution using the xoscar framework. It adds a new execution service module, specialized actors for Python, JavaScript, and shell commands, and a wrapper to isolate existing tools. Key feedback includes critical security concerns regarding potential shell injection and permissive Python execution globals. Additionally, the reviewer highlighted a lack of concurrency limits in process management that could lead to resource exhaustion, potential runtime errors when calling asynchronous code from synchronous wrappers, and logic duplication in the base executor classes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a process-level isolation framework for tool execution using the xoscar actor library. It introduces a new execution module featuring specialized actors for Python, JavaScript, shell commands, and generic tools, alongside a ProcessService for managing dynamic sub-pools. Integration utilities are also provided to wrap existing tools for isolated execution. Feedback focuses on adhering to PEP 8 by moving local imports to the top level and ensuring that process termination is handled robustly when execution timeouts occur.
rogercloud
left a comment
There was a problem hiding this comment.
I found several integration and contract issues in the process-isolation implementation. Inline comments below.
rogercloud
left a comment
There was a problem hiding this comment.
Approved. I re-checked the latest head and the previously raised concerns have been addressed: process isolation is now wired into the production lifecycle, the wrapper preserves the tool contract, timeout cleanup no longer leaves stale actor references, command execution preserves existing shell semantics, and process isolation is gated behind explicit tool opt-in.
Summary
XAGENT_PROCESS_ISOLATION_ENABLED=falsesupports_process_isolation, with sandbox execution taking priority/api/process-service/statusand/api/process-service/healthCommandExecutorCoreRebase notes
feat/process-isolationwork onto currentmainpyproject.tomlconflict by keeping bothexa-pyfrom main andxoscarfrom this featureReview fixes
asyncio.run()inside async actor executionfinallypath, including timeout pathsn_workersand expose service resource infoPYTHONPATHexecute_python_code, verifies it is process-isolated, executes Python through it, and checks the child PID differs from the parent PIDVerification
ruff check src/xagent/core/execution/service/process.py tests/core/execution/test_process_service.pyruff format --check src/xagent/core/execution/service/process.py tests/core/execution/test_process_service.pyruff check src/xagent/core/execution/service/process.py src/xagent/core/execution/actors/command_executor_actor.py src/xagent/core/tools/adapters/vibe/process_isolated src/xagent/core/tools/adapters/vibe/factory.py src/xagent/core/tools/adapters/vibe/python_executor.py src/xagent/core/tools/adapters/vibe/output_filter_wrapper.py src/xagent/config.py src/xagent/web/app.py src/xagent/web/api/services.py tests/core/tools/test_process_isolated_wrapper.py tests/core/execution/test_process_service.py tests/core/test_config.py tests/web/api/test_process_service_api.pyPYTHONPATH=src pytest tests/core/test_config.py tests/web/api/test_process_service_api.py tests/core/tools/test_process_isolated_wrapper.py -qPYTHONPATH=src pytest tests/core/execution/test_process_service.py tests/core/execution/test_integration.py tests/core/tools/test_python_executor_process_isolation.py -qPYTHONPATH=src pytest tests/core/execution/test_process_service.py::test_python_execution_timeout tests/core/execution/test_process_service.py::test_command_execution_preserves_shell_semantics tests/core/tools/test_python_executor_process_isolation.py::test_python_executor_concurrent_executions -qPYTHONPATH=src pytest tests/core/test_config.py::TestProcessIsolationConfig -qPYTHONPATH=src pytest tests/integration/test_auto_migration_startup.py::test_startup_event_skips_when_auto_migrate_disabled tests/integration/test_auto_migration_startup.py::test_startup_event_triggers_background_auto_migration tests/integration/test_auto_migration_startup.py::test_startup_event_no_task_when_no_table_needs_migration -qPYTHONPATH=src pytest tests/core/execution/test_integration.py::test_tool_factory_process_isolated_python_executor_runs -qPYTHONPATH=src pytest tests/core/execution/test_integration.py tests/core/tools/test_process_isolated_wrapper.py tests/core/tools/test_python_executor_process_isolation.py -qNote: I did not run sandbox tool tests locally per the sandbox constraint; CI will cover the standard suite.