Skip to content

fix: honour the stored setup_script_path when executing automations - #390

Open
santhiprakash wants to merge 1 commit into
OpenHands:mainfrom
santhiprakash:fix/setup-script-path
Open

fix: honour the stored setup_script_path when executing automations#390
santhiprakash wants to merge 1 commit into
OpenHands:mainfrom
santhiprakash:fix/setup-script-path

Conversation

@santhiprakash

@santhiprakash santhiprakash commented Aug 26, 2026

Copy link
Copy Markdown

Problem

setup_script_path is accepted, validated, persisted and echoed back by the API, but no executor ever reads it. Both execution paths build ... && cd {work_dir} && ([ ! -f setup.sh ] || bash setup.sh) && {entrypoint} with a hardcoded root setup.sh (execution.py:416, execution.py:581), so a tarball that names its setup script anything else silently skips its setup step and then fails in the entrypoint (typically ModuleNotFoundError) with nothing in the logs pointing at the skipped setup. The presets are unaffected only because they hardcode setup_script_path="setup.sh".

Fixes #343 (option "Honour the field" from the issue).

Triage / Root cause

grep -rn setup_script_path openhands/ on main returns only the request models (schemas.py), the router pass-through (router.py:137), the ORM model (models.py:84), the git-sync serializer and the preset constructions — the two cmd builders in execution.py never see the stored value, because execute_in_context does not receive it (the dispatcher has the Automation row in scope at its call site but only passes entrypoint).

Fix

  • execute_in_context takes a new setup_script_path: str | None = None parameter and interpolates it into the setup fragment with the existing _shell_quote helper, keeping the setup.sh default when unset: ([ ! -f '{path}' ] || bash '{path}'). The value is already validated at the request layer (relative, no .. segments, no shell metacharacters), and single-quoting keeps a ' in a stored path literal.
  • run_automation (blocking path, used by the e2e scripts) gains the same parameter and interpolation.
  • The dispatcher passes setup_script_path=automation.setup_script_path at its single production call site.
  • Docstrings that promised setup.sh now describe the configured setup script.

Verification

  • uv run pytest tests/ -q --ignore=tests/integration1460 passed (includes 4 new tests in tests/test_execution.py::TestSetupScriptPath: custom path reaches the dispatch command, default setup.sh when unset, a single quote in the path stays escaped, and the blocking path honours the stored path).
  • Rebased onto main @ 3efb91d (020_add_automation_disabled_reason landed after opening): full suite → 1473 passed, 0 failed; the dispatcher hunk composes cleanly with the new unhealthy-automation logic.
  • Rebased onto main @ f1b3244 (release 1.9.0, includes SDK 1.44.0 bump chore: bump SDK to 1.44.0 #393; no hunk overlap with dispatcher/execution): targeted suite tests/test_execution.py + tests/test_dispatcher.py85 passed, 0 failed on SDK 1.44.0; pre-commit (ruff format/lint, pycodestyle, pyright) all pass.
  • uv run pre-commit run --files openhands/automation/execution.py openhands/automation/dispatcher.py tests/test_execution.py → ruff format/lint, pycodestyle, pyright all passed.

Notes

  • tests/test_router.py's "valid setup_script_path is accepted" case (asserting 201 for "scripts/setup.sh") needed no change: the issue called it out as blessing a value the runner would never execute, and after this fix that value is genuinely executed, so the assertion is now correct end-to-end.
  • Open PR fix: make preset bootstraps shell-neutral across platforms #179 (cross-platform preset bootstrap) also threads setup_script_path into execute_in_context as part of a larger rework. That PR is aimed at a different problem (POSIX-free presets), has changes requested (duplicated bootstrap + Windows os.execv), and has been open since June, so I did not treat it as covering this issue — but if it revives, this change will need a small rebase on the same lines.
  • Presets keep setup_script_path="setup.sh" and are unaffected; git-synced automation.yaml values round-trip through the existing serializer and now take effect on the next run.

HUMAN: This PR was prepared with AI assistance (code search, patch, and test execution guided by a human reviewer); the fix follows the approach sketched in issue #343.

@github-actions github-actions Bot added the type: fix A bug fix label Aug 26, 2026
@santhiprakash
santhiprakash force-pushed the fix/setup-script-path branch from 79f8b17 to 0c24e92 Compare August 27, 2026 03:40
- 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 OpenHands#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 <changed> (ruff, pycodestyle,
  pyright all passed).

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@santhiprakash
santhiprakash force-pushed the fix/setup-script-path branch from 0c24e92 to 21efd5b Compare August 27, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

setup_script_path is validated, stored and tested — but never read; only a root setup.sh ever runs

1 participant