From 21efd5b1ecaebaf634a246cd1dca9f912d51f85a Mon Sep 17 00:00:00 2001 From: Santhi Prakash Date: Wed, 26 Aug 2026 22:28:09 +0000 Subject: [PATCH] fix: honour the stored setup_script_path when executing automations - Problem: setup_script_path is validated, stored and echoed back by the API, but both executor paths run a hardcoded root setup.sh, so a tarball naming any other setup script silently skips its setup step and fails later in the entrypoint (issue #343). - Fix: thread automation.setup_script_path from the dispatcher into execute_in_context (and add the same parameter to run_automation for the blocking path), interpolating the value with the existing _shell_quote helper and keeping the setup.sh default when unset. - Verification: uv run pytest tests/ -q --ignore=tests/integration (1460 passed); uv run pre-commit run --files (ruff, pycodestyle, pyright all passed). Co-Authored-By: Paperclip --- openhands/automation/dispatcher.py | 1 + openhands/automation/execution.py | 25 +++++-- tests/test_execution.py | 113 +++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 6 deletions(-) diff --git a/openhands/automation/dispatcher.py b/openhands/automation/dispatcher.py index 57fd13a0..6feca239 100644 --- a/openhands/automation/dispatcher.py +++ b/openhands/automation/dispatcher.py @@ -421,6 +421,7 @@ async def _fail( env_vars=env_vars, timeout=effective_timeout, run_id=run_id, + setup_script_path=automation.setup_script_path, sandbox_id=ctx.sandbox_id, ) except PermanentDispatchError as exc: diff --git a/openhands/automation/execution.py b/openhands/automation/execution.py index 2c8e6d23..a643fe23 100644 --- a/openhands/automation/execution.py +++ b/openhands/automation/execution.py @@ -341,6 +341,7 @@ async def execute_in_context( timeout: int | None = None, run_id: str | None = None, sandbox_id: str | None = None, + setup_script_path: str | None = None, ) -> DispatchResult: """Execute automation code in an existing execution context. @@ -348,7 +349,7 @@ async def execute_in_context( The context (agent_url, session_key) is obtained from the backend. 1. Get tarball into environment (upload bytes OR download from URL). - 2. Extract it, run ``setup.sh`` (if present), then start *entrypoint*. + 2. Extract it, run the setup script (if present), then start *entrypoint*. 3. Return immediately without waiting for the entrypoint to complete. Args: @@ -364,6 +365,10 @@ async def execute_in_context( path (/tmp/automation-.tar.gz) that prevents collisions when concurrent runs share the same filesystem (sandboxless mode) sandbox_id: Sandbox ID for logging (Cloud mode only) + setup_script_path: Path to the setup script inside the extracted + tarball (default: ``setup.sh``). The value is validated by the + request layer (relative, no traversal, no shell metacharacters) + before it is stored on the automation. Returns: DispatchResult with success status @@ -408,12 +413,13 @@ def _log_ctx() -> dict[str, Any]: ) env_prefix = _env_command_prefix(env_path) + setup_cmd = _shell_quote(setup_script_path or "setup.sh") cmd = ( f"{env_prefix}mkdir -p {work_dir}" f" && tar xzf {tarball_path} -C {work_dir}" f" && rm -f {tarball_path}" f" && cd {work_dir}" - f" && ([ ! -f setup.sh ] || bash setup.sh)" + f" && ([ ! -f {setup_cmd} ] || bash {setup_cmd})" f" && {entrypoint}" ) @@ -484,6 +490,7 @@ async def run_automation( run_id: str | None = None, keep_sandbox: bool = False, work_dir: str = DEFAULT_WORK_DIR, + setup_script_path: str | None = None, ) -> AutomationResult: """Execute an automation end-to-end in a fresh sandbox (blocking). @@ -492,7 +499,7 @@ async def run_automation( 1. Create sandbox and wait until RUNNING. 2. Get tarball into sandbox (upload bytes OR download from URL). - 3. Extract it, run ``setup.sh`` (if present), then run *entrypoint*. + 3. Extract it, run the setup script (if present), then run *entrypoint*. 4. Wait for completion and return the result. 5. Delete the sandbox (unless *keep_sandbox* is True). @@ -500,8 +507,9 @@ async def run_automation( (downloaded directly inside sandbox via curl). URLs avoid downloading untrusted/large files on the automation service. - *env_vars* are exported before setup.sh and the entrypoint run, - so setup.sh can consume injected values such as ``AUTOMATION_API_URL``. + *env_vars* are exported before the setup script and the entrypoint run, + so the setup script can consume injected values such as + ``AUTOMATION_API_URL``. The sandbox identity env vars (``SANDBOX_ID``, ``SESSION_API_KEY``) are **always** injected so the SDK's ``local_agent_server_mode`` works. If *callback_url* / *run_id* are set they are injected as @@ -510,6 +518,10 @@ async def run_automation( *work_dir* is the working directory for tarball extraction (default: /workspace/project). + + *setup_script_path* is the path of the setup script inside the extracted + tarball (default: ``setup.sh``). Validated by the request layer before it + is stored on the automation. """ timeout = resolve_automation_timeout_seconds(timeout) http_timeout = get_config().http.http_long_timeout @@ -574,11 +586,12 @@ def _log_ctx() -> dict[str, Any]: ) env_prefix = _env_command_prefix(env_path) + setup_cmd = _shell_quote(setup_script_path or "setup.sh") cmd = ( f"{env_prefix}mkdir -p {work_dir}" f" && tar xzf {TARBALL_PATH} -C {work_dir}" f" && cd {work_dir}" - f" && ([ ! -f setup.sh ] || bash setup.sh)" + f" && ([ ! -f {setup_cmd} ] || bash {setup_cmd})" f" && {entrypoint}" ) diff --git a/tests/test_execution.py b/tests/test_execution.py index b65f21bf..cb5aa34a 100644 --- a/tests/test_execution.py +++ b/tests/test_execution.py @@ -326,6 +326,119 @@ async def test_success_returns_dispatch_result(self, mock_start_bash, mock_uploa assert result.sandbox_id == "test-sandbox-id" +class TestSetupScriptPath: + """The stored ``setup_script_path`` must reach the executed command. + + The field is validated, persisted and echoed back by the API, but the + executor historically ran a hardcoded root ``setup.sh`` — a tarball + naming anything else silently skipped its setup step (issue #343). + """ + + @pytest.mark.asyncio + @patch("openhands.automation.execution._start_bash", new_callable=AsyncMock) + @patch("openhands.automation.execution._upload", new_callable=AsyncMock) + async def test_custom_path_is_used_in_command(self, mock_upload, mock_start_bash): + """execute_in_context runs the stored setup script path, quoted.""" + mock_start_bash.return_value = "cmd-1" + + result = await execute_in_context( + client=AsyncMock(), + agent_url="https://agent.example.com", + session_key="session-key", + entrypoint="python main.py", + tarball_source=b"fake tarball bytes", + work_dir=DEFAULT_WORK_DIR, + setup_script_path="scripts/setup.sh", + ) + + assert result.success is True + command = mock_start_bash.await_args.args[3] + assert "([ ! -f 'scripts/setup.sh' ] || bash 'scripts/setup.sh')" in command + + @pytest.mark.asyncio + @patch("openhands.automation.execution._start_bash", new_callable=AsyncMock) + @patch("openhands.automation.execution._upload", new_callable=AsyncMock) + async def test_default_setup_sh_when_unset(self, mock_upload, mock_start_bash): + """Without a stored path the root setup.sh convention is kept.""" + mock_start_bash.return_value = "cmd-1" + + result = await execute_in_context( + client=AsyncMock(), + agent_url="https://agent.example.com", + session_key="session-key", + entrypoint="python main.py", + tarball_source=b"fake tarball bytes", + work_dir=DEFAULT_WORK_DIR, + ) + + assert result.success is True + command = mock_start_bash.await_args.args[3] + assert "([ ! -f 'setup.sh' ] || bash 'setup.sh')" in command + + @pytest.mark.asyncio + @patch("openhands.automation.execution._start_bash", new_callable=AsyncMock) + @patch("openhands.automation.execution._upload", new_callable=AsyncMock) + async def test_path_with_single_quote_is_escaped( + self, mock_upload, mock_start_bash + ): + """A quote in the path (allowed by the validator) stays literal.""" + mock_start_bash.return_value = "cmd-1" + + result = await execute_in_context( + client=AsyncMock(), + agent_url="https://agent.example.com", + session_key="session-key", + entrypoint="python main.py", + tarball_source=b"fake tarball bytes", + work_dir=DEFAULT_WORK_DIR, + setup_script_path="scripts/se'tup.sh", + ) + + assert result.success is True + command = mock_start_bash.await_args.args[3] + assert ( + "([ ! -f 'scripts/se'\\''tup.sh' ] || bash 'scripts/se'\\''tup.sh')" + in command + ) + + @pytest.mark.asyncio + @patch("openhands.automation.execution._bash", new_callable=AsyncMock) + @patch("openhands.automation.execution._upload", new_callable=AsyncMock) + @patch("openhands.automation.execution._create_and_wait", new_callable=AsyncMock) + @patch("openhands.automation.execution.httpx.AsyncClient") + async def test_blocking_path_uses_custom_setup_script( + self, + mock_async_client, + mock_create_and_wait, + mock_upload, + mock_bash, + ): + """run_automation (blocking mode) also honours the stored path.""" + context_manager = MagicMock() + context_manager.__aenter__ = AsyncMock(return_value=AsyncMock()) + context_manager.__aexit__ = AsyncMock(return_value=None) + mock_async_client.return_value = context_manager + mock_create_and_wait.return_value = ( + "sandbox-1", + "session-key", + "https://agent.example.com", + ) + mock_bash.return_value = (0, "", "") + + result = await run_automation( + api_url="https://api.example.com", + api_key="api-key", + entrypoint="python main.py", + tarball_source=b"fake tarball bytes", + keep_sandbox=True, + setup_script_path="scripts/setup.sh", + ) + + assert result.success is True + command = mock_bash.await_args.args[3] + assert "([ ! -f 'scripts/setup.sh' ] || bash 'scripts/setup.sh')" in command + + class TestPrivateEnvironmentInjection: def test_env_file_is_loaded_and_removed(self, tmp_path): env_path = tmp_path / "private.env"