fix: make preset bootstraps shell-neutral across platforms#179
fix: make preset bootstraps shell-neutral across platforms#179jamiechicago312 wants to merge 15 commits into
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Replace hardcoded /tmp/ with tempfile.gettempdir() in both TARBALL_PATH (constants.py) and the per-run tarball path (execution.py). On Windows the agent-server rejects /tmp/... as non-absolute (os.path.isabs returns False without a drive-letter prefix), causing automation runs to fail with: 400 Bad Request: Path must be absolute Co-authored-by: openhands <openhands@all-hands.dev>
Refactor the legacy shell runner command builder so pyright can prove the setup path is only used when present. Also include the formatter-driven cleanup required by pre-commit on the touched files. Co-authored-by: openhands <openhands@all-hands.dev>
|
👀 OpenHands is reviewing this pull request. Conversation: https://nestable-nonremittably-sha.ngrok-free.dev/conversations/777b90bb-e1a8-4d73-9a71-85af83600359 This comment was generated by an AI agent (OpenHands) on behalf of the user. |
|
I did have OH write this but, FR I just ran into this when recording a demo, it sucks --jamie@malhotra5 I just hit this in a real Agent Canvas on Windows flow, and I want to emphasize that this PR feels critical for smooth Windows support. Automations really need to work on the first try for Windows users in agent-canvas. Right now, a simple automation can still fail before the actual prompt even starts if any POSIX-only path or shell assumptions leak through the local automation bootstrap/dispatch path. From the user side, that makes Windows automation support feel unreliable even when the actual automation prompt is fine. Getting this merged would meaningfully improve the out-of-box experience for Windows users and reduce a very sharp edge in local agent-canvas automations. This comment was posted by an AI agent (OpenHands) on behalf of Jamie Chicago. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — The PR solves a real cross-platform problem with a pragmatic approach. Backward compatibility is maintained via the setup_script_path parameter defaulting to "setup.sh". Two issues need addressing before merge.
[CRITICAL ISSUES]
- [openhands/automation/presets/plugin/bootstrap.py] DRY Violation:
plugin/bootstrap.pyis byte-for-byte identical toprompt/bootstrap.py(97 lines duplicated). Any bug fix or SDK change must be applied to both files — guaranteed to drift. The AGENTS.md even notes future preset types may be added, which will make this worse. Extract to a sharedpresets/bootstrap.pyincluded by both tarballs (or generate from a single source template).
[IMPROVEMENT OPPORTUNITIES]
-
[openhands/automation/presets/prompt/bootstrap.py, Line 14] Unused import:
import sysis never referenced in the file (same inplugin/bootstrap.py). Remove it. -
[openhands/automation/presets/prompt/bootstrap.py, Line 80] Implicit venv targeting:
uv pip installwithout an explicit--pythonor--venvargument relies onuv's auto-discovery of.venvin the current working directory. Works when CWD ==SCRIPT_DIR, but fragile if that ever changes. Prefer:uv pip install --python str(_venv_python_path()) ...to make the target venv explicit regardless of CWD.
[TESTING GAPS]
- [PR description] No Evidence section: The PR lists test commands (
uv run pytest ...) but provides no output. For a cross-platform fix, concrete evidence — e.g., CI run output showing the newTestPythonRunnerCommandtests passing, or a run log from a Windows agent-server — is required.uv run pytestwithout output doesn't prove the Windows path works end-to-end.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The new Python runner path (setup_script_path=None) routes all preset executions through a base64-encoded inline script. Correctness is well-unit-tested on POSIX. The Windows execution path (viapy -3) anduv pip installimplicit venv discovery are covered only by monkeypatched unit tests, not an end-to-end run on a Windows agent-server. Backward compatibility is solid — existing automations withsetup_script_path="setup.sh"are unaffected.
VERDICT:
❌ Needs rework: The duplicate bootstrap.py files are a maintenance trap, and the PR description lacks evidence. Both are addressable without restructuring the core logic.
KEY INSIGHT:
The cross-platform tarball path fix (tempfile.gettempdir()) and the Python runner design are correct; the main risk is the bifurcated bootstrap.py maintenance surface.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
@OpenHands can you resolve the request in this comment #179 (review) |
|
@jamiechicago312 it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again. |
|
I'm on it! jamiechicago312 can track my progress at all-hands.dev |
- Remove duplicate prompt/bootstrap.py and plugin/bootstrap.py (97 lines each, differing only in their docstring) - Add shared openhands/automation/presets/bootstrap.py with generic docstring - Update preset_router.py to reference BOOTSTRAP_PATH from the shared file for both prompt and plugin preset cache loaders - Remove unused 'import sys' from bootstrap - Fix uv pip install to use --python str(_venv_python_path()) so the venv target is explicit regardless of CWD (resolves implicit venv discovery) - Update tests to reference PRESETS_DIR / 'bootstrap.py' instead of the now-removed preset-specific paths Addresses review feedback in PR OpenHands#179: - Critical DRY violation (byte-for-byte duplicate bootstrap files) - Unused import sys - Implicit venv targeting in uv pip install Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed all three issues from the review (commit 9991fef): [Critical] DRY Violation — resolved [Suggestion] Unused [Suggestion] Implicit venv targeting — resolved Tests: 87 passed, 0 failed ( This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
@jamiechicago312 it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again. |
1 similar comment
|
@jamiechicago312 it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again. |
Sandboxes guarantee uv is present but may not have a system-level python binary in PATH. Replace the platform-specific _get_python_launcher() logic and the preset entrypoint with 'uv run python' which works on all platforms and resolves the interpreter via uv's own Python management. - execution.py: _get_python_launcher() now returns 'uv run python' on all platforms instead of 'python' (POSIX) / 'py -3' (Windows) - preset_router.py: PRESET_BOOTSTRAP_ENTRYPOINT and _get_preset_entrypoint() simplified to 'uv run python bootstrap.py'; import os removed (no longer needed after removing os.name branch) - test_preset_router.py: TestPresetEntrypoint condensed to a single test asserting 'uv run python bootstrap.py' is returned on any platform Addresses review feedback from @malhotra5. Co-authored-by: openhands <openhands@all-hands.dev>
TARBALL_PATH is not a protocol constant - no sandbox has a fixed expectation for this filename. It is a local temp path used only by run_automation() (E2E test helper) and as a last-resort fallback in execute_in_context() when run_id is absent. Move it to execution.py as _FALLBACK_TARBALL_PATH so constants.py stays limited to true protocol constants (WORK_DIR, MODEL_PROFILE_PATTERN). This also removes the os/tempfile imports that were added to constants.py only to support this value. Addresses review comment from malhotra5 on PR OpenHands#179. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
OpenHands encountered an error: Sandbox not running: a94b503105454537945c0cdc3d817306 See the conversation for more information. |
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addresses comments and retried 0618.1.mp4 |
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
I think everything has been addresses @malhotra5 tysm |
4 participants
Summary
Testing
Closes #178
This PR was created by an AI agent (OpenHands) on behalf of the user.