From 686c34d3f6f3c433bbfed8f45b60f208a18849d1 Mon Sep 17 00:00:00 2001 From: IAvecilla Date: Wed, 23 Sep 2026 18:52:15 -0700 Subject: [PATCH] feat(bot_desktop): ship Bot Screen on hosted images (-desktop tags) The published image had no Xvnc/Xfce because nothing set the Dockerfile's HERMES_BOT_DESKTOP argument, and a hosted instance (unprivileged, no sudo, sealed /opt/hermes) cannot install at run time. The image layer is the only delivery path. - docker.yml: variant axis [slim, desktop]. :latest / :main / :v* stay the image they are today; :latest-desktop / :main-desktop / :v*-desktop carry the packages plus Playwright's headed Chromium. Slim owns the build cache scope; one manifest per variant so a desktop publish failure never skips slim's :latest. - Dockerfile / stage2-hook.sh: XDG_RUNTIME_DIR=/tmp/hermes-runtime seeded 0700 as hermes (containers have no logind; the $HOME/.cache fallback was the shared /opt/data volume), refused when foreign-owned; deterministic Chromium discovery exporting the headless shell for ordinary browsing. - bot_desktop: memory gate reads the cgroup working set (usage minus inactive_file) so it cannot tighten over uptime and refuse to restart a screen idle-stop just stopped; installable() gives three distinct dead-end messages instead of a sudo line nobody there can run; env_for_agent replaces a headless-shell pin so agent and dock share one Chromium. Squash of IAvecilla/hermes-agent:bot-desktop-cloud-image (#112381, 13 commits), which GitHub auto-closed when its base branch merged as #108914. Review fixes from pefontana (cache scope, per-variant merge, red browser test) are included. Co-authored-by: pefontana --- .github/workflows/docker.yml | 65 +++++++---- Dockerfile | 42 ++++++- docker/stage2-hook.sh | 47 ++++++-- hermes_cli/config.py | 3 + hermes_cli/config_defaults.py | 8 +- tests/tools/test_bot_desktop_browser.py | 37 ++++++ tests/tools/test_bot_desktop_resources.py | 34 ++++++ tests/tools/test_bot_desktop_runtime.py | 106 ++++++++++++++++++ tools/bot_desktop/browser.py | 19 +++- tools/bot_desktop/resources.py | 38 ++++++- tools/bot_desktop/runtime.py | 55 +++++++-- .../docs/user-guide/features/bot-screen.md | 24 +++- 12 files changed, 422 insertions(+), 56 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 7e6b916f2a..df6c3fb501 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -74,19 +74,21 @@ jobs: strategy: fail-fast: false matrix: + # Two images per arch. `slim` is what :latest has always been; `desktop` + # adds the Bot Screen packages and a headed Chromium (+1.4 GB) for the + # tier that offers a screen. Both are built on a PR so a change that only + # breaks the gated layers cannot reach publish. + arch: [amd64, arm64] + variant: [slim, desktop] include: - arch: amd64 runner: ubuntu-latest-32-core platform: linux/amd64 - cache-from: type=gha,scope=docker-amd64 - cache-to: type=gha,mode=max,scope=docker-amd64 # arm64 builds on the native arm64 larger runner. A build of # linux/arm64 on an x64 host uses emulation. - arch: arm64 runner: ubuntu-latest-32-arm-core platform: linux/arm64 - cache-from: type=gha,scope=docker-arm64 - cache-to: type=gha,mode=max,scope=docker-arm64 runs-on: ${{ matrix.runner }} timeout-minutes: 45 @@ -124,8 +126,11 @@ jobs: tags: ${{ env.IMAGE_NAME }}:test build-args: | HERMES_GIT_SHA=${{ github.sha }} - cache-from: ${{ matrix.cache-from }} - cache-to: ${{ (github.event_name != 'pull_request') && matrix.cache-to || '' }} + HERMES_BOT_DESKTOP=${{ matrix.variant == 'desktop' && '1' || '0' }} + # Slim owns the scope; desktop is slim plus two RUN steps, so it reads + # and writes nothing — a 5.5 GB mode=max scope would blow the 10 GB cap. + cache-from: type=gha,scope=docker-${{ matrix.arch }} + cache-to: ${{ (github.event_name != 'pull_request' && matrix.variant == 'slim') && format('type=gha,mode=max,scope=docker-{0}', matrix.arch) || '' }} # Run the docker-integration test suite against the freshly-built @@ -191,18 +196,16 @@ jobs: strategy: fail-fast: false matrix: + arch: [amd64, arm64] + variant: [slim, desktop] include: - arch: amd64 runner: ubuntu-latest-32-core platform: linux/amd64 - cache-from: type=gha,scope=docker-amd64 - cache-to: type=gha,mode=max,scope=docker-amd64 # Native arm64 for the same reason as the build matrix above. - arch: arm64 runner: ubuntu-latest-32-arm-core platform: linux/arm64 - cache-from: type=gha,scope=docker-arm64 - cache-to: type=gha,mode=max,scope=docker-arm64 runs-on: ${{ matrix.runner }} timeout-minutes: 30 steps: @@ -242,9 +245,10 @@ jobs: org.opencontainers.image.revision=${{ github.sha }} build-args: | HERMES_GIT_SHA=${{ github.sha }} + HERMES_BOT_DESKTOP=${{ matrix.variant == 'desktop' && '1' || '0' }} outputs: type=image,name=${{ env.IMAGE_NAME }},push-by-digest=true,name-canonical=true,push=true - cache-from: ${{ matrix.cache-from }} - cache-to: ${{ matrix.cache-to }} + cache-from: type=gha,scope=docker-${{ matrix.arch }} + cache-to: ${{ matrix.variant == 'slim' && format('type=gha,mode=max,scope=docker-{0}', matrix.arch) || '' }} - name: Export digest run: | @@ -255,7 +259,7 @@ jobs: - name: Upload digest artifact uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: - name: digest-${{ matrix.arch }} + name: digest-${{ matrix.variant }}-${{ matrix.arch }} path: /tmp/digests/* if-no-files-found: error retention-days: 1 @@ -269,17 +273,30 @@ jobs: # On releases: tags :. # --------------------------------------------------------------------------- merge: - if: github.repository == 'NousResearch/hermes-agent' && (github.event_name == 'push' && github.ref == 'refs/heads/main' || github.event_name == 'release') + # `needs` is the whole 4-leg matrix and a failed need skips the job, so a + # transient desktop push would take slim's :latest with it. Guard below. + if: ${{ !cancelled() && github.repository == 'NousResearch/hermes-agent' && (github.event_name == 'push' && github.ref == 'refs/heads/main' || github.event_name == 'release') }} runs-on: ubuntu-latest needs: [publish] timeout-minutes: 10 environment: container-publish + strategy: + fail-fast: false + matrix: + # One manifest list per variant. `slim` keeps the unsuffixed tags it has + # always had, so nothing that pulls :latest today changes; `desktop` + # publishes the same digests under a -desktop suffix. + include: + - variant: slim + suffix: "" + - variant: desktop + suffix: "-desktop" steps: - name: Download digests uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 with: path: /tmp/digests - pattern: digest-* + pattern: digest-${{ matrix.variant }}-* merge-multiple: true # Retry once on transient Docker Hub / buildkit pull failures. @@ -304,16 +321,25 @@ jobs: env: IMAGE_NAME: ${{ env.IMAGE_NAME }} RELEASE_TAG: ${{ github.event.release.tag_name }} + SUFFIX: ${{ matrix.suffix }} run: | set -euo pipefail + # Without nullglob an empty dir yields the literal `*`: one bogus entry. + shopt -s nullglob args=() for digest_file in *; do args+=("${IMAGE_NAME}@sha256:${digest_file}") done + # One per arch in the publish matrix; a short set means a leg failed, and + # stitching it would publish a single-arch :latest. Keep in step with it. + if [ "${#args[@]}" -ne 2 ]; then + echo "::error::variant ${{ matrix.variant }}: want 2 digests, found ${#args[@]} (a publish leg failed)" + exit 1 + fi if [ "${{ github.event_name }}" = "release" ]; then - tags=(-t "${IMAGE_NAME}:${RELEASE_TAG}") + tags=(-t "${IMAGE_NAME}:${RELEASE_TAG}${SUFFIX}") else - tags=(-t "${IMAGE_NAME}:main" -t "${IMAGE_NAME}:latest") + tags=(-t "${IMAGE_NAME}:main${SUFFIX}" -t "${IMAGE_NAME}:latest${SUFFIX}") fi # Retry: Docker Hub API + just-pushed digest eventual consistency # can transiently fail the create; the operation is idempotent. @@ -333,9 +359,10 @@ jobs: env: IMAGE_NAME: ${{ env.IMAGE_NAME }} RELEASE_TAG: ${{ github.event.release.tag_name }} + SUFFIX: ${{ matrix.suffix }} run: | if [ "${{ github.event_name }}" = "release" ]; then - docker buildx imagetools inspect "${IMAGE_NAME}:${RELEASE_TAG}" + docker buildx imagetools inspect "${IMAGE_NAME}:${RELEASE_TAG}${SUFFIX}" else - docker buildx imagetools inspect "${IMAGE_NAME}:main" + docker buildx imagetools inspect "${IMAGE_NAME}:main${SUFFIX}" fi diff --git a/Dockerfile b/Dockerfile index 1fece52ce7..ba9cbe8724 100644 --- a/Dockerfile +++ b/Dockerfile @@ -73,11 +73,14 @@ RUN apt-get -o Acquire::Retries=3 update && \ ca-certificates curl iputils-ping python3 python-is-python3 ripgrep ffmpeg gcc g++ make cmake python3-dev python3-venv libffi-dev libolm-dev libatomic1 procps git openssh-client docker-cli xz-utils && \ rm -rf /var/lib/apt/lists/* -# Bot Screen (opt-in): TigerVNC + the Xfce components + a headed chromium, so a -# container that cannot run apt at run time (unprivileged user, no sudo — every -# hosted instance) can still stream a desktop. ~550 MB. Nothing here starts at -# boot; the layer costs no memory until a screen is started. Same package list -# as tools/bot_desktop/runtime.py::PACKAGES["apt"]. +# Bot Screen (opt-in): PACKAGES["apt"] from tools/bot_desktop/runtime.py plus apt +# `chromium` for the dock's Browser icon. ~930 MB apt on debian:13.4 (~1.4 GB of +# image once the gated headed Chromium below is counted); nothing starts +# at boot. docker.yml builds both variants and publishes these packages under +# the `-desktop` tags: hosted sandboxes pull a prebuilt image and never run a +# build, and cannot apt at run time either (unprivileged, no sudo). Only this +# build step needs root — +# Xvnc is a userspace X server, so the runtime user can drive it. # docker build --build-arg HERMES_BOT_DESKTOP=1 . ARG HERMES_BOT_DESKTOP=0 RUN if [ "$HERMES_BOT_DESKTOP" = "1" ]; then \ @@ -211,13 +214,25 @@ COPY apps/shared/ apps/shared/ # guards against a future regression if the source npm version changes. ENV npm_config_install_links=false +# chrome-headless-shell: what the browser tool has always driven headlessly. +# Smaller, no window code paths. --with-deps pulls the shared system libraries. RUN npm install --prefer-offline --no-audit --fetch-retries=5 && \ for i in 1 2 3; do \ npx playwright install --with-deps chromium --only-shell && break || \ - { [ "$i" = 3 ] && exit 1; echo "playwright install failed (attempt $i); retrying in 10s"; sleep 10; }; \ + { [ "$i" = 3 ] && exit 1; echo "playwright headless-shell install failed (attempt $i); retrying in 10s"; sleep 10; }; \ done && \ npm cache clean --force +# chrome-headless-shell cannot open a window, so the dock's Browser icon needs the +# full build. Same Chromium family as the shell, so agent and human share one +# --user-data-dir. Gated: a build with no desktop has nothing to show it on. +RUN if [ "$HERMES_BOT_DESKTOP" = "1" ]; then \ + for i in 1 2 3; do \ + npx playwright install chromium && break || \ + { [ "$i" = 3 ] && exit 1; echo "playwright chromium install failed (attempt $i); retrying in 10s"; sleep 10; }; \ + done; \ + fi + # ---------- Photon iMessage sidecar deps (baked, NS-606) ---------- # The photon plugin's Node sidecar needs its own node_modules # (spectrum-ts). The install tree is immutable at runtime, so a lazy @@ -293,6 +308,15 @@ COPY apps/shared/ apps/shared/ RUN cd web && npm run build && \ cd ../ui-tui && npm run build +# ---------- Bot Screen X socket directory ---------- +# Xvnc would create this itself (/tmp is 1777); pre-creating it keeps ownership +# deterministic when HERMES_UID is remapped between boots. +RUN mkdir -p /tmp/.X11-unix && chmod 1777 /tmp/.X11-unix + +# XDG_RUNTIME_DIR (set below) sits under a predictable name in world-writable /tmp. +# Shipping it root-owned means stage2 finds a directory it trusts and chowns it. +RUN mkdir -p /tmp/hermes-runtime && chmod 0700 /tmp/hermes-runtime + # ---------- Source code ---------- # .dockerignore excludes node_modules, so the installs above survive. # --link decouples this layer from parents for cache purposes; --chmod bakes @@ -418,6 +442,12 @@ ENV HERMES_DISABLE_LAZY_INSTALLS=1 # updates (an ABI stamp invalidates it if a rebuild bumps the interpreter). ENV HERMES_LAZY_INSTALL_TARGET=/opt/data/lazy-packages +# Xfce, dbus and the display-allocation lock need one; containers have no logind +# to create /run/user/. The default fallback ($HOME/.cache) is the /opt/data +# volume, which a host-side install may share — two instances would then contend +# for one lock. Container-scoped instead; seeded 0700 by docker/stage2-hook.sh. +ENV XDG_RUNTIME_DIR=/tmp/hermes-runtime + # `docker exec` privilege-drop shim. When operators run # `docker exec hermes ...` they default to root, and any file the # command writes under $HERMES_HOME (auth.json, .env, config.yaml) ends diff --git a/docker/stage2-hook.sh b/docker/stage2-hook.sh index 61d5d12b57..9b8e489aa9 100755 --- a/docker/stage2-hook.sh +++ b/docker/stage2-hook.sh @@ -395,6 +395,31 @@ as_hermes mkdir -p \ "$HERMES_HOME/platforms/pairing" \ "$HERMES_HOME/lazy-packages" +# --- XDG_RUNTIME_DIR --- +# 0700 as dbus requires. It lives in world-writable /tmp under a predictable name +# and holds the display-allocation lock, so it is a security boundary: refuse a +# symlink or a directory someone else owns (chowning that one would hand hermes a +# directory whose creator keeps an fd into it), and chown rather than assume — +# `usermod -u` above does not chown outside the home dir, so a HERMES_UID remap +# would leave it owned by the old uid and every Xfce/dbus/lock open would EACCES. +if [ -n "${XDG_RUNTIME_DIR:-}" ]; then + xdg_owner="" + if [ -e "$XDG_RUNTIME_DIR" ]; then xdg_owner=$(stat -c %u "$XDG_RUNTIME_DIR" 2>/dev/null || echo unknown); fi + if refuse_symlinked_path "create" "$XDG_RUNTIME_DIR"; then + : + elif [ -n "$xdg_owner" ] && [ "$xdg_owner" != "0" ] && [ "$xdg_owner" != "$actual_hermes_uid" ]; then + echo "[stage2] Warning: $XDG_RUNTIME_DIR is owned by uid $xdg_owner (not root or hermes) — refusing to adopt it" + else + mkdir -p "$XDG_RUNTIME_DIR" 2>/dev/null || \ + echo "[stage2] Warning: could not create XDG_RUNTIME_DIR $XDG_RUNTIME_DIR (continuing)" + if [ -d "$XDG_RUNTIME_DIR" ]; then + chown hermes:hermes "$XDG_RUNTIME_DIR" 2>/dev/null || \ + echo "[stage2] Warning: could not chown XDG_RUNTIME_DIR $XDG_RUNTIME_DIR (rootless?)" + chmod 0700 "$XDG_RUNTIME_DIR" 2>/dev/null || true + fi + fi +fi + # --- Install-method stamp --- # The 'docker' stamp is baked into the immutable install tree at # /opt/hermes/.install_method (see Dockerfile), NOT written here into @@ -718,10 +743,10 @@ if [ -d "$INSTALL_DIR/skills" ]; then fi # --- Discover agent-browser's Chromium binary --- -# The image's Dockerfile runs `npx playwright install chromium`, which -# populates ``$PLAYWRIGHT_BROWSERS_PATH`` (=/opt/hermes/.playwright) with -# a ``chromium_headless_shell-/chrome-headless-shell-linux64/`` -# directory. agent-browser (the runtime CLI Hermes spawns for the +# The image populates ``$PLAYWRIGHT_BROWSERS_PATH`` (=/opt/hermes/.playwright) +# with ``chromium_headless_shell-/chrome-headless-shell-linux64/``, plus +# ``chromium-/chrome-linux64/`` on a HERMES_BOT_DESKTOP build. +# agent-browser (the runtime CLI Hermes spawns for the # browser tool) doesn't recognise this layout in its own cache scan and # fails with "Auto-launch failed: Chrome not found" — even though the # binary is right there (#15697). @@ -746,11 +771,19 @@ fi if [ -z "${AGENT_BROWSER_EXECUTABLE_PATH:-}" ] && \ [ -n "${PLAYWRIGHT_BROWSERS_PATH:-}" ] && \ [ -d "$PLAYWRIGHT_BROWSERS_PATH" ]; then + # Two ordered finds, not one with alternated -name predicates: that returns + # them in directory order, i.e. whichever Playwright unpacked first. Shell + # first, because this is what agent-browser launches for ordinary headless + # browsing everywhere and it is the lighter build; browser.py::env_for_agent + # swaps in the headed one for the agent while a screen is up. browser_bin=$(find "$PLAYWRIGHT_BROWSERS_PATH" -type f -executable \ - \( -name 'chrome' -o -name 'chromium' \ - -o -name 'chrome-headless-shell' -o -name 'headless_shell' \ - -o -name 'chromium-browser' \) \ + \( -name 'chrome-headless-shell' -o -name 'headless_shell' \) \ 2>/dev/null | head -n 1) + if [ -z "$browser_bin" ]; then + browser_bin=$(find "$PLAYWRIGHT_BROWSERS_PATH" -type f -executable \ + \( -name 'chrome' -o -name 'chromium' -o -name 'chromium-browser' \) \ + 2>/dev/null | head -n 1) + fi if [ -n "$browser_bin" ]; then echo "[stage2] Found agent-browser Chromium binary: $browser_bin" # Write to s6's container_environment so with-contenv picks it diff --git a/hermes_cli/config.py b/hermes_cli/config.py index bb29e67b37..ca846e01b7 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -355,6 +355,9 @@ Notes: won't move your container — pull the newer tag you actually want, or switch to ``:latest`` / ``:main`` for rolling updates. See available tags at https://hub.docker.com/r/nousresearch/hermes-agent/tags + • On a ``-desktop`` tag (the one carrying Bot Screen)? Keep the suffix: + the unsuffixed image has no Xvnc/Xfce and no sudo to add them, so + pulling it stops the bots' screens from starting. • Your config and session history live under ``$HERMES_HOME`` (``/opt/data`` in the container, typically bind-mounted from the host) and persist across image upgrades — re-pulling doesn't lose any state. diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index c2bfcce914..5ded10e044 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -2470,9 +2470,11 @@ DEFAULT_CONFIG = { # host. Off by default so installing TigerVNC for other reasons never yields a screen nobody asked # for; Hermes Desktop's Screen pane offers Start and this toggle. "auto_start": False, - # Refuse to start the screen while the host (or its container cgroup) has less than this free. - # Xvnc + Xfce idle at ~220 MB and a takeover's browser adds 0.5-1 GB; on a small instance the - # loser is Chromium mid-login or the gateway itself. 0 disables the check. + # Refuse to start below this much free memory (MB), measured on the host or its container cgroup, + # whichever is tighter. Xvnc + Xfce idle at ~220 MB and a takeover's browser adds 0.5-1 GB, so a + # screen with one page runs past 1 GB; the kernel OOM killer picks its victim by score, so on a + # small instance the loser is the dashboard or the gateway rather than the desktop. 0 disables the + # check. "min_free_memory_mb": 1536, # Stop a screen nobody has used (no computer_use action, browser spawn, viewer or takeover) for this # long; it restarts on the next use. Idle Xvnc + Xfce hold ~220 MB, an abandoned browser far more. diff --git a/tests/tools/test_bot_desktop_browser.py b/tests/tools/test_bot_desktop_browser.py index 61e57244bf..b5bdab304f 100644 --- a/tests/tools/test_bot_desktop_browser.py +++ b/tests/tools/test_bot_desktop_browser.py @@ -315,3 +315,40 @@ def test_daemon_idle_timer_defers_to_the_janitor_only_for_the_shared_headed_brow monkeypatch.setattr(session._cloud, "_is_headed_mode", lambda: True) monkeypatch.setattr(runtime, "published_env", lambda: {}) assert session._daemon_idle_timeout_seconds() == 120 + + +def test_a_headless_shell_pin_is_replaced_while_a_screen_is_up(tmp_path, monkeypatch): + """The boot hook exports a chrome-headless-shell path; leaving it would put the agent and the dock on + two binaries over one --user-data-dir, where the singleton swallows the dock's launch.""" + shell = tmp_path / "chrome-headless-shell" + shell.write_text("#!/bin/sh\n", encoding="utf-8") + shell.chmod(0o755) + headed = tmp_path / "chrome" + headed.write_text("#!/bin/sh\n", encoding="utf-8") + headed.chmod(0o755) + monkeypatch.setattr(runtime, "state_dir", lambda: tmp_path / "bot-desktop") + monkeypatch.setattr(browser, "_playwright_executable", lambda: str(headed)) + monkeypatch.delenv("AGENT_BROWSER_PROFILE", raising=False) + # Unpinned, the ubuntu runner (non-root, userns-restricted) flips executable() to + # its own /usr/bin/google-chrome; host policy is not the subject here. + monkeypatch.setattr(browser, "_userns_restricted", lambda: False) + + agent_env = browser.env_for_agent({"AGENT_BROWSER_EXECUTABLE_PATH": str(shell)}) + dock_exe, _ = browser.dock_launch() + assert agent_env["AGENT_BROWSER_EXECUTABLE_PATH"] == dock_exe == str(headed), \ + "the agent and the dock must share one binary once a screen is up" + + +def test_a_real_user_pin_is_still_honoured(tmp_path, monkeypatch): + """Only a headless-shell pin is overridden; a human's own headed browser stays put.""" + mine = tmp_path / "my-chrome" + mine.write_text("#!/bin/sh\n", encoding="utf-8") + mine.chmod(0o755) + other = tmp_path / "chrome" + other.write_text("#!/bin/sh\n", encoding="utf-8") + other.chmod(0o755) + monkeypatch.setattr(runtime, "state_dir", lambda: tmp_path / "bot-desktop") + monkeypatch.setattr(browser, "_playwright_executable", lambda: str(other)) + + env = browser.env_for_agent({"AGENT_BROWSER_EXECUTABLE_PATH": str(mine)}) + assert env["AGENT_BROWSER_EXECUTABLE_PATH"] == str(mine) diff --git a/tests/tools/test_bot_desktop_resources.py b/tests/tools/test_bot_desktop_resources.py index f49d90e3e1..5564b09376 100644 --- a/tests/tools/test_bot_desktop_resources.py +++ b/tests/tools/test_bot_desktop_resources.py @@ -62,3 +62,37 @@ def test_memory_info_takes_the_tighter_of_cgroup_and_host(tmp_path, monkeypatch) assert info.available_mb == 1024 and info.limit_mb == 2048 (v2 / "memory.max").write_text("max") # no limit: host numbers assert resources.memory_info() == resources.MemoryInfo(available_mb=3072, limit_mb=4096) + + +def _cgroup(monkeypatch, tmp_path, *, v2=True, limit, usage, cache): + """A cgroup tree: ``usage`` consumed, ``cache`` of it reclaimable.""" + root = tmp_path / "cg" + root.mkdir(exist_ok=True) + monkeypatch.setattr(resources, "_MEMINFO", tmp_path / "no-meminfo") # cgroup numbers only + if v2: + monkeypatch.setattr(resources, "_CGROUP_V2", root) + monkeypatch.setattr(resources, "_CGROUP_V1", tmp_path / "nope") + (root / "memory.max").write_text(str(limit)) + (root / "memory.current").write_text(str(usage)) + (root / "memory.stat").write_text(f"anon 123\ninactive_file {cache}\nslab 7\n") + else: + monkeypatch.setattr(resources, "_CGROUP_V2", tmp_path / "nope") + monkeypatch.setattr(resources, "_CGROUP_V1", root) + (root / "memory.limit_in_bytes").write_text(str(limit)) + (root / "memory.usage_in_bytes").write_text(str(usage)) + (root / "memory.stat").write_text(f"total_inactive_file {cache}\n") + + +@pytest.mark.parametrize("v2", [True, False], ids=["cgroup-v2", "cgroup-v1"]) +def test_page_cache_does_not_count_against_the_limit(tmp_path, monkeypatch, v2): + """643 MiB of mostly page cache must not read as 643 MiB consumed: charging it would tighten the gate + over uptime, and refuse to restart a screen the idle auto-stop had just stopped.""" + MB = 1024 * 1024 + _cgroup(monkeypatch, tmp_path, v2=v2, limit=4096 * MB, usage=643 * MB, cache=340 * MB) + assert resources.memory_info().available_mb == 4096 - (643 - 340) + + +def test_a_zero_floor_disables_the_gate(monkeypatch): + """config_defaults documents "0 disables the check".""" + monkeypatch.setattr(resources, "min_free_mb", lambda: 0) + assert resources.memory_blocker(resources.MemoryInfo(available_mb=10, limit_mb=4096)) is None diff --git a/tests/tools/test_bot_desktop_runtime.py b/tests/tools/test_bot_desktop_runtime.py index 45500aee83..e4a96c9304 100644 --- a/tests/tools/test_bot_desktop_runtime.py +++ b/tests/tools/test_bot_desktop_runtime.py @@ -3,6 +3,7 @@ from __future__ import annotations import contextlib +import re import threading import time import sys @@ -23,6 +24,21 @@ def test_every_required_binary_maps_to_an_installed_package(pm): assert not {"xorg-x11-server-utils", "xorg-x11-utils"} & set(runtime.PACKAGES["dnf"]), "retired on Fedora" +def test_the_image_bakes_the_same_apt_packages_the_runtime_would_install() -> None: + """The image layer is the only delivery path on a hosted instance, so a package added here but not + there stalls the screen with no error until someone presses Start.""" + dockerfile = Path(__file__).resolve().parents[2] / "Dockerfile" + text = dockerfile.read_text() + assert "ARG HERMES_BOT_DESKTOP" in text, "the Bot Screen apt layer is gone from the Dockerfile" + body = text.split("ARG HERMES_BOT_DESKTOP", 1)[1].split("--no-install-recommends", 1)[1].split("rm -rf", 1)[0] + baked = {tok for tok in re.split(r"[\s\\&]+", body) if tok and not tok.startswith("-")} + required = set(runtime.PACKAGES["apt"]) + assert required <= baked, f"the image would not install: {sorted(required - baked)}" + # apt `chromium` on top of the operator's list: a headed browser for the dock's Browser icon that + # does not depend on Playwright's copy being unpacked yet. + assert baked - required <= {"chromium"}, f"unexpected extra packages: {sorted(baked - required)}" + + def test_no_running_screen_returns_none_without_grabbing(monkeypatch): monkeypatch.setattr(runtime, "published_env", lambda: {"DISPLAY": ":99"}) monkeypatch.setattr(runtime, "_launcher_pid", lambda: None) @@ -365,3 +381,93 @@ def test_allocation_lock_is_released_once_xvnc_claims_the_number(in_process_runt t.join() assert st.running assert seen.get("free") is True, "allocation lock still held after Xvnc wrote its X lock" + + +def _startable_host(monkeypatch, tmp_path, *, running=False): + """A Linux host with the packages present, so only the check under test can block a start.""" + from tools.bot_desktop import resources + monkeypatch.setattr(runtime, "is_supported_host", lambda: True) + monkeypatch.setattr(runtime, "missing_binaries", lambda: []) + monkeypatch.setattr(runtime, "state_dir", lambda: tmp_path / "bd") + monkeypatch.setattr(runtime, "_launcher_pid", lambda: 4242 if running else None) + monkeypatch.setattr(runtime, "published_env", lambda: {"DISPLAY": ":7"} if running else {}) + monkeypatch.setattr(runtime, "_reap_orphaned_server", lambda sd: None) + monkeypatch.setattr(resources, "min_free_mb", lambda: 1536) + monkeypatch.setattr(resources, "memory_info", + lambda: resources.MemoryInfo(available_mb=400, limit_mb=4096)) + spawned: list = [] + monkeypatch.setattr(runtime, "_spawn_and_wait", lambda *a, **k: spawned.append(a)) + return spawned + + +def test_a_running_desktop_is_never_refused_for_the_memory_it_is_using(tmp_path, monkeypatch): + """The gate guards the allocation, not the session: a running desktop is itself what consumes the + memory, so checking before the running-check made Start fail on a healthy screen.""" + _startable_host(monkeypatch, tmp_path, running=True) + runtime.start() # returns status(); must not raise about headroom + + +def test_a_root_host_without_a_package_manager_is_told_the_truth(tmp_path, monkeypatch): + """Root with no package manager: installable() is False for a reason unrelated to privilege.""" + _startable_host(monkeypatch, tmp_path) + monkeypatch.setattr(runtime, "missing_binaries", lambda: ["Xvnc"]) + monkeypatch.setattr(runtime, "package_manager", lambda: None) + monkeypatch.setattr(runtime, "is_root", lambda: True) + assert runtime.installable() is False + with pytest.raises(RuntimeError) as excinfo: + runtime.start() + message = str(excinfo.value) + assert "package manager" in message + assert "unprivileged" not in message and "sudo" not in message, f"wrong diagnosis: {message}" + + +def test_an_unprivileged_host_is_pointed_at_the_image(tmp_path, monkeypatch): + """The published image: a package manager exists but there is no way to reach root.""" + _startable_host(monkeypatch, tmp_path) + monkeypatch.setattr(runtime, "missing_binaries", lambda: ["Xvnc"]) + monkeypatch.setattr(runtime, "package_manager", lambda: "apt") + monkeypatch.setattr(runtime, "is_root", lambda: False) + monkeypatch.setattr(runtime.shutil, "which", lambda name: None if name == "sudo" else "/usr/bin/" + name) + assert runtime.installable() is False + with pytest.raises(RuntimeError, match="baked in"): + runtime.start() + + +def test_a_host_that_can_install_gets_the_command(tmp_path, monkeypatch): + """The branch that used to be unreachable behind an `or` fallback.""" + _startable_host(monkeypatch, tmp_path) + monkeypatch.setattr(runtime, "missing_binaries", lambda: ["Xvnc"]) + monkeypatch.setattr(runtime, "package_manager", lambda: "apt") + monkeypatch.setattr(runtime, "is_root", lambda: True) + with pytest.raises(RuntimeError, match="tigervnc-standalone-server"): + runtime.start() + + +def test_a_tight_but_sufficient_start_is_logged(tmp_path, monkeypatch, caplog): + """Above the floor but below the derived threshold the start proceeds and says so. It is the only + signal an operator gets that a screen came up with no room for the browser that is the point of it, + so it has to actually fire rather than merely be computable.""" + from tools.bot_desktop import resources + + spawned = _startable_host(monkeypatch, tmp_path) + monkeypatch.setattr(resources, "min_free_mb", lambda: 1536) # threshold -> 2048 + monkeypatch.setattr(resources, "memory_info", + lambda: resources.MemoryInfo(available_mb=1800, limit_mb=2048)) + with caplog.at_level("WARNING", logger="tools.bot_desktop.runtime"): + runtime.start() + assert spawned, "1800 MB clears the 1536 MB floor, so the screen still starts" + logged = [r.getMessage() for r in caplog.records] + assert any("1800 MB available" in m for m in logged), f"no tight-headroom warning in {logged}" + + +def test_a_comfortable_start_is_not_logged(tmp_path, monkeypatch, caplog): + """And it stays quiet with real headroom, or it would fire on every start and mean nothing.""" + from tools.bot_desktop import resources + + _startable_host(monkeypatch, tmp_path) + monkeypatch.setattr(resources, "min_free_mb", lambda: 1536) + monkeypatch.setattr(resources, "memory_info", + lambda: resources.MemoryInfo(available_mb=7210, limit_mb=8182)) + with caplog.at_level("WARNING", logger="tools.bot_desktop.runtime"): + runtime.start() + assert not [m for m in (r.getMessage() for r in caplog.records) if "available" in m] diff --git a/tools/bot_desktop/browser.py b/tools/bot_desktop/browser.py index fa44c63719..f8e094c68f 100644 --- a/tools/bot_desktop/browser.py +++ b/tools/bot_desktop/browser.py @@ -62,9 +62,10 @@ def _playwright_executable() -> Optional[str]: def _is_headless_shell(exe: str) -> bool: - """Playwright's ``chrome-headless-shell`` can drive pages but cannot open a window: the official Docker - image ships only that build and its boot hook exports it as ``AGENT_BROWSER_EXECUTABLE_PATH``, so - trusting the override blindly would pin a windowless binary to the dock's Browser icon.""" + """Playwright's ``chrome-headless-shell`` can drive pages but cannot open a window. The image's boot + hook exports it as ``AGENT_BROWSER_EXECUTABLE_PATH`` (it is the only build the unsuffixed tags carry, + and the lighter one everywhere), so trusting that override blindly would pin a windowless binary to + the dock's Browser icon.""" return "headless" in os.path.basename(exe).lower() or "headless_shell" in exe @@ -168,9 +169,17 @@ def _pid_alive(pid: int) -> bool: def env_for_agent(env: dict) -> dict: - """Pin agent-browser to the screen's browser identity unless the user pinned their own.""" + """Pin agent-browser to the screen's browser identity unless the user pinned their own. + + A headless-shell pin is replaced, not kept: the image's boot hook exports one for ordinary browsing, + and leaving it would put the agent and the dock on different binaries over one ``--user-data-dir``, + where Chromium's singleton swallows the dock's launch into the windowless process. Only runs while a + screen is up (:func:`runtime.desktop_env`), so the heavier build is pinned just when it is the point. + """ env.setdefault("AGENT_BROWSER_PROFILE", str(profile_dir())) exe = executable() if exe: - env.setdefault("AGENT_BROWSER_EXECUTABLE_PATH", exe) + pinned = env.get("AGENT_BROWSER_EXECUTABLE_PATH", "").strip() + if not pinned or _is_headless_shell(pinned): + env["AGENT_BROWSER_EXECUTABLE_PATH"] = exe return env diff --git a/tools/bot_desktop/resources.py b/tools/bot_desktop/resources.py index 99f3e93e72..599d878863 100644 --- a/tools/bot_desktop/resources.py +++ b/tools/bot_desktop/resources.py @@ -54,14 +54,36 @@ def _meminfo() -> dict[str, int]: return out +def _stat_value(path: Path, key: str) -> Optional[int]: + """One `` `` line out of a cgroup ``memory.stat``.""" + try: + for line in path.read_text(encoding="utf-8").splitlines(): + name, _, rest = line.partition(" ") + if name == key: + return int(rest.strip()) + except (OSError, ValueError): + return None + return None + + def _cgroup_limit_and_usage() -> tuple[Optional[int], Optional[int]]: + """The cgroup's limit and its *working set* — usage minus reclaimable page cache. + + ``memory.current`` counts page cache, so a container reads several hundred MB above idle right after a + desktop stops with every process gone. Charging that would make the gate tighten over uptime, so we + subtract ``inactive_file``, the working-set convention kubelet uses. + """ limit = _read_int(_CGROUP_V2 / "memory.max") usage = _read_int(_CGROUP_V2 / "memory.current") + cache = _stat_value(_CGROUP_V2 / "memory.stat", "inactive_file") if limit is None and usage is None: limit = _read_int(_CGROUP_V1 / "memory.limit_in_bytes") usage = _read_int(_CGROUP_V1 / "memory.usage_in_bytes") + cache = _stat_value(_CGROUP_V1 / "memory.stat", "total_inactive_file") if limit is not None and limit >= 1 << 60: # v1 "unlimited" is a huge sentinel limit = None + if usage is not None and cache: + usage = max(usage - cache, 0) return limit, usage @@ -79,6 +101,8 @@ def memory_info() -> MemoryInfo: def min_free_mb() -> int: + """``bot_desktop.min_free_memory_mb``; 0 disables the gate. A hosted deployment sets it in the + instance's config.yaml (or the managed overlay), not an env var.""" from hermes_cli.config import load_config_readonly cfg = load_config_readonly().get("bot_desktop") or {} try: @@ -87,10 +111,18 @@ def min_free_mb() -> int: return DEFAULT_MIN_FREE_MB -def memory_blocker(info: Optional[MemoryInfo] = None) -> Optional[str]: +def tight_headroom_mb(floor: Optional[int] = None) -> int: + """Above the floor but below this, a start is allowed and logged: the desktop fits, a few browser tabs + would not. Derived from the floor so raising the floor cannot silently retire the warning.""" + floor = min_free_mb() if floor is None else floor + return floor + floor // 3 + + +def memory_blocker(info: Optional[MemoryInfo] = None, need: Optional[int] = None) -> Optional[str]: """Why the screen must not start now, or None. Unknown memory is not a blocker: a host we cannot - read is not a host we know to be small.""" - need = min_free_mb() + read is not a host we know to be small. ``need`` lets a caller that also wants + :func:`tight_headroom_mb` read the floor once instead of loading the config twice.""" + need = min_free_mb() if need is None else need if need == 0: return None info = info or memory_info() diff --git a/tools/bot_desktop/runtime.py b/tools/bot_desktop/runtime.py index 1af9d16625..f9d5591479 100644 --- a/tools/bot_desktop/runtime.py +++ b/tools/bot_desktop/runtime.py @@ -92,8 +92,12 @@ def package_manager() -> Optional[str]: def install_command() -> Optional[str]: """The distro command that installs the Bot Desktop packages, as the human would type it on THIS host: - prefixed with ``sudo`` unless Hermes already runs as root (the official Docker image is uid 0 with no - sudo binary), so it is both what the pane shows and what :mod:`tools.bot_desktop.install` runs.""" + prefixed with ``sudo`` unless Hermes already runs as root, so it is both what the pane shows and what + :mod:`tools.bot_desktop.install` runs. ``None`` when no package manager is present. + + Not a promise that it can run here: see :func:`installable`. The published Docker image supervises + every service under ``s6-setuidgid hermes`` (UID 10000 by default) and ships no ``sudo`` binary, so an + install on a hosted instance is impossible no matter what this returns.""" pm = package_manager() if pm is None: return None @@ -110,6 +114,20 @@ def is_root() -> bool: return hasattr(os, "geteuid") and os.geteuid() == 0 +def installable() -> bool: + """Whether :func:`install_command` could actually succeed on this host. + + False on an unprivileged process with no ``sudo`` to reach for, which is exactly the published Docker + image: services drop to the ``hermes`` user and no ``sudo`` binary is installed. The packages can only + arrive in the image there, so :func:`start` says that instead of printing a sudo line the user has no + way to run. ``status()`` still reports ``install_command`` for the pane; surfacing this there needs a + wire-contract change and is deliberately out of scope. + """ + if package_manager() is None: + return False + return is_root() or shutil.which("sudo") is not None + + @dataclass class DesktopStatus: profile: str @@ -285,8 +303,9 @@ def _kill_group_then_wait(pgid: Optional[int], pid: int, grace: float = 2.0) -> _reap_if_ours() -# Host-wide (every profile allocates from one band), so it lives outside any profile home — but not in -# world-writable /tmp, where a predictable name lets another local user pre-create or squat the file. +# Host-wide (every profile allocates from one band), so it lives outside any profile home. A predictable +# name must not be squattable: XDG_RUNTIME_DIR is the boundary — 0700 from logind, or from +# docker/stage2-hook.sh in containers, which have none. _ALLOC_LOCK = Path(os.environ.get("XDG_RUNTIME_DIR") or Path.home() / ".cache") / "hermes-bot-desktop-alloc.lock" @@ -475,6 +494,12 @@ def _profile_name() -> str: return "default" +# The gate lives in ``resources`` so start() and status() cannot disagree about it. Measured in the +# official image: gateway idle 304 MiB, +216 for Xvnc/Xfce, 1073 MiB with one Chromium page. The OOM +# killer picks by score, so on a small instance the casualty is the dashboard or the gateway, not the +# desktop that caused the pressure. + + def start(*, wait_seconds: float = 15.0) -> DesktopStatus: """Start this profile's desktop (idempotent). Blocks until the launcher publishes its env file or ``wait_seconds`` pass; raises ``RuntimeError`` naming the blocker. @@ -488,8 +513,18 @@ def start(*, wait_seconds: float = 15.0) -> DesktopStatus: raise RuntimeError("Bot Desktop runs on Linux gateway hosts only") missing = missing_binaries() if missing: - hint = install_command() or "install TigerVNC (Xvnc) and the Xfce core components" - raise RuntimeError(f"Bot Desktop needs {', '.join(missing)} on the gateway host. Install: {hint}") + # Three dead ends: an operator told "unprivileged, no sudo" while running as root hunts the wrong bug. + need = f"Bot Desktop needs {', '.join(missing)} on the gateway host" + if package_manager() is None: + raise RuntimeError( + f"{need}, and no supported package manager (apt/dnf/pacman) is available to install them. " + "Install TigerVNC (Xvnc) and the Xfce core components with this distro's own tooling.") + if not installable(): + raise RuntimeError( + f"{need}, and this host cannot install them: the process is unprivileged and there is no " + "sudo. On the published Docker image the packages have to be baked in, so this needs a " + "newer image rather than an install.") + raise RuntimeError(f"{need}. Install: {install_command()}") sd = state_dir() sd.mkdir(parents=True, exist_ok=True) os.chmod(sd, 0o700) @@ -497,8 +532,14 @@ def start(*, wait_seconds: float = 15.0) -> DesktopStatus: if _launcher_pid() is not None and published_env().get("DISPLAY"): return status() from tools.bot_desktop import resources - if (blocker := resources.memory_blocker()) is not None: + floor = resources.min_free_mb() + mem = resources.memory_info() + if (blocker := resources.memory_blocker(mem, need=floor)) is not None: raise RuntimeError(blocker) + if mem.available_mb is not None and mem.available_mb < resources.tight_headroom_mb(floor): + logger.warning( + "Bot Desktop starting with %d MB available; a browser with a few pages open can use most " + "of that.", mem.available_mb) if _launcher_pid() is None: _reap_orphaned_server(sd) _ALLOC_LOCK.parent.mkdir(parents=True, exist_ok=True, mode=0o700) diff --git a/website/docs/user-guide/features/bot-screen.md b/website/docs/user-guide/features/bot-screen.md index 52c7962a66..67a0d8f39e 100644 --- a/website/docs/user-guide/features/bot-screen.md +++ b/website/docs/user-guide/features/bot-screen.md @@ -76,11 +76,12 @@ reverse proxy's access log may record an already-spent ticket. 0.5–1 GB (one page: ~550 MB). Plan on **~1.1–1.5 GB per open screen with a browser**; the desktop alone is cheap, the browser is the cost. CPU is not a constraint (idle desktop ≈ 0.01 core, live streaming ≈ 0.03 core). The packages - take ~550 MB of disk on Debian. + take ~930 MB of disk on Debian 13. Before starting a screen, Hermes checks that the host — or its container cgroup, whichever is tighter — has `bot_desktop.min_free_memory_mb` free - (default 1536). Below that the pane shows why in place of **Start screen** and + (default 1536; `0` disables the check). Below that the pane shows why in place + of **Start screen** and `hermes computer-use screen start` refuses; a screen already running is never taken down by this check. A screen nobody uses is stopped after `bot_desktop.idle_stop_minutes` (default 30) and comes back on the next use, so @@ -91,16 +92,27 @@ reverse proxy's access log may record an already-spent ticket. ### Baking the packages into a container image An image for a hosted or unprivileged deployment cannot install anything at run -time, so build the packages in. The official `Dockerfile` has an opt-in build -argument: +time, so the packages have to be built in. CI publishes two variants of every +version: the unsuffixed tags (`:latest`, `:v*`) without them, and the +**`-desktop` tags** (`:latest-desktop`, `:v*-desktop`) with them. A hosted +deployment (Fly Machines, Azure container instances) gets Bot Screen by pulling +the suffixed tag; a build argument could not reach it anyway, since it never +runs a build. Nothing in the provisioner selects `-desktop` yet, so a hosted +instance still comes up slim; pulling the suffixed tag yourself works today. + +Build your own only if you want the packages in a custom image. The official +`Dockerfile` has an opt-in build argument, off by default so a plain +`docker build .` stays lean: ```bash docker build --build-arg HERMES_BOT_DESKTOP=1 -t hermes-agent:screen . ``` It adds TigerVNC, the Xfce components and a headed `chromium` (for the dock's -Browser icon) as one layer (~550 MB). Nothing starts at boot; an image built this -way costs no memory until a screen is started. +Browser icon), plus Playwright's headed Chromium build — about **1.4 GB** of +image (measured: 4.1 GB without the argument, 5.5 GB with it on arm64), of which +~930 MB is the apt layer. Nothing starts at boot; an image built this way costs +no memory until a screen is started. ## Using it