fix: install browser and router runtimes - #874
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the scripts/install.sh script to support optional extras (browser and router) and introduces environment variables (XAGENT_SKIP_BROWSER_INSTALL and XAGENT_SKIP_ROUTER_INSTALL) to skip their respective installations. The feedback suggests making the environment variable checks more robust by supporting other common truthy values like true instead of strictly checking for 1.
rogercloud
left a comment
There was a problem hiding this comment.
Summary
scripts/install.sh (the curl | sh one-liner installer) previously installed only Xagent's core dependencies, so browser-enabled tasks triggered a first-use Playwright download and OpenRouter's auto model failed with No module named 'xrouter_llm' — while the Docker image already ships both layers at build time. This PR closes that installer↔Docker parity gap: it installs the xagent-ai[browser,router] extras, runs python -m playwright install chromium via the tool's own isolated interpreter, adds XAGENT_SKIP_BROWSER_INSTALL / XAGENT_SKIP_ROUTER_INSTALL opt-outs through a new is_truthy() helper, and extends the CI smoke test to assert import playwright, import xrouter_llm, and the presence of the Chromium executable. The diff is small and scoped: scripts/install.sh (+31/-3) and .github/workflows/install-script.yml (+4).
Re-review status
This is a re-review. The PR had 2 prior inline comments (both from gemini-code-assist[bot], none from humans), both making the same point: the skip-install truthy checks originally matched only the literal string "1", and suggested also accepting "true".
Both are FIXED (and exceeded.) The current diff replaces the literal "1" checks with a new is_truthy() helper (scripts/install.sh:38-43) that accepts 1, true, yes, and on case-insensitively for both env vars — going beyond the bot's suggested "1"/"true" pair.
Design verdict
Sound — acceptable with minor reservations. This is the simplest fix for the stated parity gap and fits the script's existing conventions (helper style, XAGENT_* env-var knobs, set -eu, isolated uv-tool interpreter). Two scope notes, neither blocking: (1) Docker's parity is functional rather than literal — Docker installs a broader backend-image dependency group that is a superset of these two extras, so this is not a byte-for-byte match, just a functional one. (2) Neither Docker nor this script uses --with-deps, so a minimal end-user Linux box lacking Chromium's shared libraries could still fail at first browser launch despite a successful playwright install chromium. That is a real limitation but fixing it requires root/apt access and is genuinely out of scope here.
Findings
1. medium — hard-fail on Chromium download under set -eu (scripts/install.sh:88)
"$tool_python" -m playwright install chromium runs with no || true, retry, or error suppression. By this point uv tool install --upgrade "$spec" (line 80) has already succeeded, so xagent itself is fully installed and usable — but a transient network failure downloading the Chromium binary aborts the whole script with a hard error, and the user never sees the "Installed. Next steps" success message even though the CLI works. This step is logically an optional enhancement over the base install, so a failure here shouldn't look like a failed install. See inline comment for a concrete suggestion.
2. low — CI exercises only the happy path (.github/workflows/install-script.yml)
The smoke job runs the installer with no skip vars set, so only the both-extras happy path is covered; is_truthy(), the XAGENT_SKIP_ROUTER_INSTALL → extras="browser" fallback, and the XAGENT_SKIP_BROWSER_INSTALL skip branch are all unexercised (confirmed via repo-wide grep — no other test references XAGENT_SKIP or is_truthy). Kept low since this is a support/installer script and shellcheck already provides static coverage. Optionally add a CI matrix leg (or a lightweight local sh test) that sets both skip vars and asserts the router/browser steps are skipped.
3. low — DRY: ${APP}[$extras] built twice (scripts/install.sh:71, :76)
spec="${APP}[$extras]" is constructed unconditionally at line 71 and again as spec="${APP}[$extras]==$version" inside the XAGENT_VERSION branch at line 76. Build the base once and append the version conditionally (spec="${spec}==$version"), removing the duplication with identical behavior. See inline comment for a concrete diff.
4. informational nit — Chromium check doesn't launch (.github/workflows/install-script.yml:57)
The new check only asserts Path(executable_path).is_file(); it never calls .launch(), so it wouldn't catch a missing-system-shared-library failure. Little practical bite here since it runs on GitHub's fully-provisioned ubuntu-latest/macos-latest runners — exactly the environment least likely to be missing those libs, and the only environment this CI can represent. Mentioned as optional strengthening, not a required fix.
Simplification opportunities
Finding #3 above (DRY on the spec construction) is the only lean-up worth making; it's tracked as a low-severity finding with its own inline comment. Otherwise lean already — is_truthy()'s broader value acceptance (true/yes/on beyond 1) is not over-engineering: it was added specifically in response to the two prior gemini-code-assist comments requesting exactly this. The only optional doc polish would be mentioning the fuller accepted value set in the header comment (scripts/install.sh:12-15), which is a trivial nit, not a required change.
Testing
sh -n scripts/install.sh— clean.shellcheck scripts/install.sh— clean.- No functional test run was needed; the diff touches only the installer script and a CI workflow, no Python/product code.
Recommendation
Approving. The design is sound and the change is well-scoped. Finding #1 (fatal Chromium download failure) is worth addressing in a follow-up — or explicitly acknowledging as intended — but it isn't blocking; everything else (#2, #3, #4) is optional polish.
- Warn and continue if the Playwright Chromium download fails; xagent is
already installed at that point, so a transient network error should not
abort the whole installer or hide the next-steps message.
- Build the package spec once and append the version suffix conditionally
instead of reconstructing ${APP}[$extras].
What changed
xagent-ai[browser,router]extras in the one-line installer.xrouter_llm.Why
The Docker image already installs Playwright Chromium and the router runtime during image construction, but
scripts/install.shonly installed Xagent's core dependencies. As a result, users of the one-line installer could start Xagent successfully but encounter a Playwright package/browser download during their first browser-enabled task, or this error when testing OpenRouter's virtualautomodel:Installing both layers up front makes the one-line installation consistent with the Docker runtime and avoids first-task setup latency or failure:
xagent-ai[browser,router]installs the Python Playwright package and OpenRouter auto-routing runtime.python -m playwright install chromiuminstalls the matching browser binary.User impact
Browser-enabled tasks can launch Chromium immediately after the one-line installer completes, and OpenRouter's
automodel can load its in-process router.XAGENT_SKIP_BROWSER_INSTALL=1skips the Chromium binary download;XAGENT_SKIP_ROUTER_INSTALL=1avoids the larger router/ML dependency set for users who do not need automatic model routing.Validation
sh -n scripts/install.shxagent-ai[browser,router]==<version>is a valid pinned PEP 508 requirementnode --check scripts/get.xagent.co/worker.jsnode --testinscripts/get.xagent.co— 6 passedgit diff --checkimport playwright,import xrouter_llm, and asserts Playwright's Chromium executable exists on Ubuntu and macOS