From caa3adbbcefbe05c804f8616629865cfdc66509f Mon Sep 17 00:00:00 2001 From: Jon Saad-Falcon <41205309+jonsaadfalcon@users.noreply.github.com> Date: Fri, 29 May 2026 16:28:45 -0700 Subject: [PATCH] fix(windows): cross-platform python discovery + browser open helpers (#436) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reimplements the useful parts of #385 cleanly. Adds two small cross-platform helpers under `openjarvis.core.utils`: - `get_python_executable()` — prefers `python3`, falls back to `python` for Windows / minimal distros that only ship the unversioned name. - `open_browser(url)` — `webbrowser.open` by default; on Windows uses `cmd /c start "" ` to avoid console-host edge cases. Swapped at every hardcoded `python3` / `webbrowser.open` site: `connectors/oauth.py`, `evals/scorers/livecodebench.py`, `scripts/oauth_all.py`, `scripts/install/install.sh` (adds `PY_CMD` detection block), `scripts/quickstart.sh` (adds `MINGW*|MSYS*|CYGWIN*) cmd /c start` case), and the two affected test files. Test files wrap `get_python_executable()` in `shlex.quote()` before interpolating into `shell=True` strings — Windows interpreter paths often contain spaces. Deliberately different from #385: `openjarvis.core.__init__` does NOT re-export `DEFAULT_CONFIG_DIR` (would have raised ImportError because it's in `openjarvis.core.config`, not the package `__init__`; re-exporting would also force eager import of the heavy config module at every `import openjarvis.core`). `oauth.py` keeps `from openjarvis.core.config import DEFAULT_CONFIG_DIR` alongside the new `from openjarvis.core import open_browser`. Original API surface and call-site sweep by @sanjayravit in #385 — huge thanks for the careful Windows-compatibility audit. This PR preserves your design while fixing the ImportError edge cases caught during review. Co-Authored-By: sanjayravit Co-Authored-By: Claude Opus 4.7 --- scripts/install/install.sh | 18 +++++-- scripts/oauth_all.py | 9 ++-- scripts/quickstart.sh | 22 +++++--- src/openjarvis/connectors/oauth.py | 7 ++- src/openjarvis/core/__init__.py | 3 ++ src/openjarvis/core/utils.py | 52 +++++++++++++++++++ src/openjarvis/evals/scorers/livecodebench.py | 3 +- tests/security/test_subprocess_sandbox.py | 4 +- tests/tools/test_shell_exec.py | 7 ++- 9 files changed, 103 insertions(+), 22 deletions(-) create mode 100644 src/openjarvis/core/utils.py diff --git a/scripts/install/install.sh b/scripts/install/install.sh index 0dcba5c1..f368afbe 100755 --- a/scripts/install/install.sh +++ b/scripts/install/install.sh @@ -90,6 +90,18 @@ EOF need git need curl +# ---- python command ---- +# Prefer `python3` (Linux / macOS / WSL convention); fall back to `python` +# on minimal distros that only ship the unversioned name. This is used by +# the analytics beacon and state-file helpers below — uv installs its own +# interpreter into the venv later, so this is only for the bootstrap shims. +PY_CMD="python3" +if ! command -v python3 >/dev/null 2>&1; then + if command -v python >/dev/null 2>&1; then + PY_CMD="python" + fi +fi + # ---- env ---- OPENJARVIS_HOME="${OPENJARVIS_HOME:-$HOME/.openjarvis}" OPENJARVIS_REPO_URL="${OPENJARVIS_REPO_URL:-https://github.com/open-jarvis/OpenJarvis.git}" @@ -148,7 +160,7 @@ get_anon_id() { return fi local new_id - new_id="$(python3 -c 'import uuid; print(uuid.uuid4())' 2>/dev/null || echo "")" + new_id="$("$PY_CMD" -c 'import uuid; print(uuid.uuid4())' 2>/dev/null || echo "")" if [[ -z "$new_id" ]]; then return fi @@ -189,7 +201,7 @@ beacon() { os="$(detect_os)" arch="$(detect_arch)" - python3 - "$ANALYTICS_HOST" "$ANALYTICS_KEY" "$event" "$anon_id" \ + "$PY_CMD" - "$ANALYTICS_HOST" "$ANALYTICS_KEY" "$event" "$anon_id" \ "$os" "$arch" "$stage" "$elapsed_ms" "$exit_code" \ >/dev/null 2>&1 <<'PYEOF' || true import json @@ -251,7 +263,7 @@ mark_done() { if [[ ! -f "$STATE_FILE" ]]; then echo '{}' > "$STATE_FILE" fi - python3 - "$STATE_FILE" "$1" "$WSL" <<'PYEOF' + "$PY_CMD" - "$STATE_FILE" "$1" "$WSL" <<'PYEOF' import json, sys path, key, wsl = sys.argv[1], sys.argv[2], sys.argv[3] with open(path) as f: diff --git a/scripts/oauth_all.py b/scripts/oauth_all.py index 50755171..1603ff06 100644 --- a/scripts/oauth_all.py +++ b/scripts/oauth_all.py @@ -11,7 +11,6 @@ from __future__ import annotations import argparse import json import os -import webbrowser from http.server import BaseHTTPRequestHandler, HTTPServer from pathlib import Path from typing import Any, Dict, List @@ -19,6 +18,8 @@ from urllib.parse import parse_qs, urlencode, urlparse import httpx +from openjarvis.core import open_browser + CONFIG_DIR = Path.home() / ".openjarvis" / "connectors" CONFIG_DIR.mkdir(parents=True, exist_ok=True) @@ -144,7 +145,7 @@ def do_google() -> None: }) ) print(" Opening browser...") - webbrowser.open(url) + open_browser(url) code = _wait_for_code() resp = httpx.post( @@ -192,7 +193,7 @@ def do_strava() -> None: }) ) print(" Opening browser...") - webbrowser.open(url) + open_browser(url) code = _wait_for_code() resp = httpx.post( @@ -303,7 +304,7 @@ def do_spotify() -> None: }) ) print(" Opening browser...") - webbrowser.open(url) + open_browser(url) code = _wait_for_code(port=spotify_port) import base64 diff --git a/scripts/quickstart.sh b/scripts/quickstart.sh index 157bf0b6..7eb0d10a 100755 --- a/scripts/quickstart.sh +++ b/scripts/quickstart.sh @@ -47,19 +47,24 @@ echo " └─────────────────────── echo -e "${NC}" # ── 1. Check Python ────────────────────────────────────────────────── +# Prefer python3, fall back to python (Windows / minimal distros that ship +# only the unversioned name). info "Checking Python..." if command -v python3 &>/dev/null; then - PY_VERSION=$(python3 -c "import sys; print(f'{sys.version_info.major}.{sys.version_info.minor}')") - PY_MAJOR=$(echo "$PY_VERSION" | cut -d. -f1) - PY_MINOR=$(echo "$PY_VERSION" | cut -d. -f2) - if [ "$PY_MAJOR" -ge 3 ] && [ "$PY_MINOR" -ge 10 ]; then - ok "Python $PY_VERSION" - else - fail "Python 3.10+ required (found $PY_VERSION)" - fi + PY_CMD="python3" +elif command -v python &>/dev/null; then + PY_CMD="python" else fail "Python 3 not found. Install from https://python.org" fi +PY_VERSION=$("$PY_CMD" -c "import sys; print(f'{sys.version_info.major}.{sys.version_info.minor}')") +PY_MAJOR=$(echo "$PY_VERSION" | cut -d. -f1) +PY_MINOR=$(echo "$PY_VERSION" | cut -d. -f2) +if [ "$PY_MAJOR" -ge 3 ] && [ "$PY_MINOR" -ge 10 ]; then + ok "Python $PY_VERSION ($PY_CMD)" +else + fail "Python 3.10+ required (found $PY_VERSION)" +fi # ── 2. Check / install uv ─────────────────────────────────────────── info "Checking uv..." @@ -182,6 +187,7 @@ info "Opening $URL ..." case "$(uname -s)" in Darwin) open "$URL" ;; Linux) xdg-open "$URL" 2>/dev/null || true ;; + MINGW*|MSYS*|CYGWIN*) cmd /c start "" "$URL" 2>/dev/null || true ;; *) true ;; esac diff --git a/src/openjarvis/connectors/oauth.py b/src/openjarvis/connectors/oauth.py index ca5adbea..e7a042d7 100644 --- a/src/openjarvis/connectors/oauth.py +++ b/src/openjarvis/connectors/oauth.py @@ -16,6 +16,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple from urllib.parse import urlencode +from openjarvis.core import open_browser from openjarvis.core.config import DEFAULT_CONFIG_DIR # --------------------------------------------------------------------------- @@ -419,7 +420,6 @@ def run_oauth_flow( RuntimeError If the user denies authorization or the callback times out. """ - import webbrowser from http.server import BaseHTTPRequestHandler, HTTPServer from urllib.parse import parse_qs, urlparse @@ -492,7 +492,7 @@ def run_oauth_flow( server.timeout = 120 # 2 minute timeout # Open the consent page in the user's default browser - webbrowser.open(auth_url) + open_browser(auth_url) # Wait for the callback (blocking, with per-request timeout) while not auth_code and not error: @@ -661,7 +661,6 @@ def run_connector_oauth( Returns the raw token response dict. """ - import webbrowser provider = get_provider_for_connector(connector_id) if provider is None: @@ -694,7 +693,7 @@ def run_connector_oauth( auth_url = f"{provider.auth_endpoint}?{urlencode(params)}" # Open browser and wait for callback - webbrowser.open(auth_url) + open_browser(auth_url) code = _wait_for_callback_code( host=provider.callback_host, port=provider.callback_port, diff --git a/src/openjarvis/core/__init__.py b/src/openjarvis/core/__init__.py index 70fcb17c..47ed8380 100644 --- a/src/openjarvis/core/__init__.py +++ b/src/openjarvis/core/__init__.py @@ -19,6 +19,7 @@ from openjarvis.core.types import ( ToolCall, ToolResult, ) +from openjarvis.core.utils import get_python_executable, open_browser __all__ = [ "AgentRegistry", @@ -34,4 +35,6 @@ __all__ = [ "ToolCall", "ToolRegistry", "ToolResult", + "get_python_executable", + "open_browser", ] diff --git a/src/openjarvis/core/utils.py b/src/openjarvis/core/utils.py new file mode 100644 index 00000000..dda98e7c --- /dev/null +++ b/src/openjarvis/core/utils.py @@ -0,0 +1,52 @@ +"""Small cross-platform utilities used by the CLI, OAuth flow, and evals. + +Kept dependency-free so importing this module is cheap (the public re-export +from ``openjarvis.core`` must not pull in heavy modules at package init). +""" + +from __future__ import annotations + +import platform +import shutil +import subprocess +import webbrowser + + +def get_python_executable() -> str: + """Return the best ``python`` interpreter name on PATH. + + Prefers ``python3`` (Linux/macOS convention); falls back to ``python`` + (Windows / some minimal Linux distros that ship only ``python``). Returns + the literal string ``"python3"`` when neither is found, so callers still + get a usable command that will fail with a clear "command not found" + rather than an empty string. + + The result is a *command name or absolute path* that callers can hand to + :mod:`subprocess` directly when ``shell=False``, and must be shell-quoted + (:func:`shlex.quote`) before being interpolated into a ``shell=True`` + command string — paths on Windows often contain spaces. + """ + return shutil.which("python3") or shutil.which("python") or "python3" + + +def open_browser(url: str) -> None: + """Open *url* in the user's default browser, with a Windows fast-path. + + :func:`webbrowser.open` is the cross-platform default, but on Windows it + sometimes blocks or fails inside a console host. ``cmd /c start "" "URL"`` + is the canonical Windows incantation that hands the URL to the OS shell + and returns immediately. We try that first on Windows and fall back to + :func:`webbrowser.open` if the subprocess spawn fails. + """ + if platform.system() == "Windows": + try: + # The empty title argument after ``start`` is required: ``start`` + # treats a single quoted argument as a window title, not a URL. + subprocess.run(["cmd", "/c", "start", "", url], check=False) + return + except Exception: # noqa: BLE001 - any spawn failure -> fall back + pass + webbrowser.open(url) + + +__all__ = ["get_python_executable", "open_browser"] diff --git a/src/openjarvis/evals/scorers/livecodebench.py b/src/openjarvis/evals/scorers/livecodebench.py index 38ad7348..5f42ec86 100644 --- a/src/openjarvis/evals/scorers/livecodebench.py +++ b/src/openjarvis/evals/scorers/livecodebench.py @@ -16,6 +16,7 @@ import subprocess import tempfile from typing import Any, Dict, List, Optional, Tuple +from openjarvis.core import get_python_executable from openjarvis.evals.core.scorer import Scorer from openjarvis.evals.core.types import EvalRecord @@ -90,7 +91,7 @@ def _run_single_test( try: result = subprocess.run( - ["python3", script_path], + [get_python_executable(), script_path], input=test_input, capture_output=True, text=True, diff --git a/tests/security/test_subprocess_sandbox.py b/tests/security/test_subprocess_sandbox.py index 92fe2d95..11590457 100644 --- a/tests/security/test_subprocess_sandbox.py +++ b/tests/security/test_subprocess_sandbox.py @@ -3,8 +3,10 @@ from __future__ import annotations import os +import shlex import tempfile +from openjarvis.core import get_python_executable from openjarvis.security.subprocess_sandbox import ( build_safe_env, kill_process_tree, @@ -97,7 +99,7 @@ class TestRunSandboxed: def test_output_truncation(self) -> None: # Generate output larger than max_output_bytes result = run_sandboxed( - "python3 -c \"print('A' * 200)\"", + f"{shlex.quote(get_python_executable())} -c \"print('A' * 200)\"", timeout=10.0, max_output_bytes=50, ) diff --git a/tests/tools/test_shell_exec.py b/tests/tools/test_shell_exec.py index 50999e57..ab662db9 100644 --- a/tests/tools/test_shell_exec.py +++ b/tests/tools/test_shell_exec.py @@ -9,11 +9,13 @@ from __future__ import annotations import importlib import os +import shlex import sys from unittest.mock import MagicMock, patch import pytest +from openjarvis.core import get_python_executable from openjarvis.tools.shell_exec import ShellExecTool @@ -216,7 +218,10 @@ class TestShellExecTool: """Stdout exceeding 100 KB is truncated.""" tool = ShellExecTool() result = tool.execute( - command="python3 -c \"print('A' * 200000)\"", + command=( + f"{shlex.quote(get_python_executable())} " + "-c \"print('A' * 200000)\"" + ), ) assert "truncated" in result.content assert len(result.content) < 200_000