fix(bot-screen): bound RFB input and repair viewer recovery and ownership - #109446
whyyagswhy wants to merge 14 commits into
Conversation
…with human take-over A bot running on a headless Linux gateway now gets its own desktop (TigerVNC Xvnc + a minimal Xfce session, one per profile) that Hermes Desktop streams live. The user can watch the bot work, take over to type a login / 2FA code / CAPTCHA, and hand control back; the bot refuses every computer_use action (screenshots included) while a human holds the screen, then resumes with the session cookies the human just created. Why this shape: - The screen lives on the machine Hermes runs on, not in a vendor cloud browser, so it works for any app the bot drives and keeps the session on the user's host. - Xfce components are launched individually (xfwm4, xfce4-panel, xfdesktop, xfsettingsd) under a private dbus session rather than xfce4-session/the metapackage: no screensaver, power manager or polkit agent to lock or prompt a headless desktop. - Transport is raw RFB over a WebSocket beside /api/ws, authenticated with a one-shot ticket minted through the already-authenticated RPC channel; noVNC runs in the Electron renderer. The bridge parses the RFB client stream and drops keyboard/pointer/clipboard (incl. QEMU Extended KeyEvent, which noVNC switches to once Xvnc advertises it) from any viewer that does not hold the lease, so viewOnly is enforced server-side, not by the client. - One lease per profile (agent | human viewer) is the single truth for the RFB bridge, the computer_use tool and the Desktop UI; taking control evicts other viewers' input with close code 4000 control-taken. - Auto-start happens only at the computer_use tool boundary (headless host, packages present, bot_desktop.auto_start=true); env builders stay pure so status probes and tests never spawn X servers. tests/tools/conftest.py pins the binaries to "missing" for the same reason the browser-use fixture does. Surfaces: Desktop (Bots → right-click → Open Screen; Take over / Hand back), CLI (`hermes computer-use screen status|start|stop|install-deps`), tool (`computer_use` actions request_handoff / wait_for_human), gateway RPCs (display.status/start/stop/observe/lease.acquire/lease.release + display.lease event), docs page user-guide/features/bot-screen.
After a server-side eviction (4000 control-taken) or stream loss noVNC has already torn the client down; detach() then called disconnect() on it and noVNC logged "Tried changing state of a disconnected RFB object". Clear the ref in the disconnect listener so teardown only touches a live client.
The declaration lived in src/types/novnc.d.ts, which .gitignore drops (apps/desktop/src/**/*.d.ts is ignored except for an allowlist), so the pushed tree failed typecheck with TS7016 while the local worktree passed. An ambient 'declare module' needs a script-scoped declaration file, so it joins vite-env.d.ts rather than the module-scoped global.d.ts.
…kage install from Desktop
Three ways in, one install path, no shell required.
- Screen portal box: a compact card ("Screen · Live / Stopped / Not installed on
host", who holds control) rendered at the top of a bot's Scheduled Jobs pane
above the routines, and under each gateway/profile group header in the
Sessions sidebar (new `sidebar.gatewayGroup.header` contribution area, so
the plugin owns the box and core only exposes the slot). Clicking it opens
the Screen pane; it reads the same display.status/lease events as the pane.
- Install from Desktop: `display.install` runs the distro package command on
the gateway host (apt/dnf/pacman) as a supervised child, streams
`display.install.log`, and finishes with `display.install.done`. When sudo
needs a password the host raises `display.install.sudo.request` on the
caller's own WebSocket; the renderer shows the existing masked SudoDialog
(pointed at `display.install.sudo.respond`), so the password never touches
the renderer store as plaintext beyond the field, is redacted from the
gateway trace like `sudo.respond`, and an empty answer cancels without
spawning the package manager. One install per profile at a time. The pane's
"packages missing" state is now an install card with the command shown for
the shell-inclined.
- Opt-in stays intact: nothing installs on `hermes update`; the only triggers
are the Desktop button and `hermes computer-use screen install`.
Why the SudoDialog fallback to the app-level card: the install belongs to the
connection, not to a chat turn, so the request has no session id; the focused
chat's dialog now also shows the session-less card instead of it dying unseen.
Live (headless Electron via CDP, `hermes serve` with the packages hidden):
portal in Scheduled Jobs → click → Screen pane → Install on host → sudo card →
Cancel → "Install cancelled" and the card returns; with the packages present:
portal → Start → live stream, portal flips to "Live · bot in control"; the
same portal renders under the `default` profile in the Sessions sidebar.
…ed Jobs pane The bot's computer is now the first thing in its pane: a big 16:10 box above the title that shows an actual picture of the desktop, refreshed every 4 s while the hero is on screen (paused when scrolled away or the window is hidden). Caption carries who holds control; the chip reads Open live / Start / Install. Clicking the picture expands into the live Screen pane where the user can take over. - `display.thumbnail` RPC: one JPEG grab of the profile's Xvnc display via Pillow's ImageGrab (XAUTHORITY from the launcher's published env), scaled to <=960x600, returned as a data URL. Read-only: it never touches the lease, so a human in control is not disturbed and the bot is not blocked. - `ScreenHero` replaces the small portal row in the routines pane; the compact `ScreenPortal` stays for the Sessions sidebar group header where a 16:10 box would crowd the list. Live: hero "Screen is off" → click → pane → Start → hero shows the Xfce desktop with "Live · bot in control"; an xmessage window opened on the bot's display appeared in the hero on the next refresh; clicking the hero opened the live pane (canvas + Take over).
… theme, curated dock The vendor Xfce panel layout (copied only to silence the first-run dialog) put a light grey bar with dead launchers on every bot screen: File Manager and Text Editor pointed at Thunar/Mousepad we deliberately do not install, Web Browser opened exo's "pick a browser" dialog. It read as an unconfigured VM, and so did the thumbnail in Desktop. - Wallpaper: `tools/bot_desktop/wallpaper.png` (the Nous gradient), seeded via xfconf as a zoomed backdrop over the existing colour fallback. - Dark theme: first dark GTK theme the host ships (Adwaita-dark, Breeze-Dark, Greybird-dark, Arc-Dark, else Adwaita), dark icon theme likewise, xfwm4 Default decorations, DejaVu fonts; both panels dark with slight translucency. - Own panel layout: top bar = menu · task list · tray · clock; bottom dock = only launchers whose program exists on this host, the browser pinned to the one the bot drives (chrome → chromium → firefox) so a human who takes over lands in the bot's own browser profile. Anything the user installs still appears in the Applications menu; the dock is the only curated part. - `launcher.sh` and the wallpaper declared as package data (the launcher was already missing from sealed wheels). - `HERMES_BD_SEED_ONLY=1` stops the launcher after seeding so the config tree is testable on a fake PATH without an X server. Live: fresh screen shows the gradient, dark panels, two dock icons; XTEST clicks on the dock opened xfce4-terminal and Chrome, both dark-themed and listed in the task bar; the Desktop hero and Screen pane show the same picture.
…ight actions, sudo reply pinned to origin, dock Browser is the bot's browser, safe display reuse, install keeps profile scope Independent review of the PR head found the control boundary only held inside one process and several claims the code did not back. Each item below was reproduced, fixed, covered by an invariant test proven red without the fix, and re-verified live on a real Xvnc/Xfce screen. - Lease authority on disk. `lease.json` under an fcntl lock in the profile's bot-desktop dir; every read goes to the file. `hermes serve` (viewer bridge), the messaging gateway, a CLI turn and isolated workers now agree. Live: a takeover in process A made `computer_use capture` in process B return human_has_control; release in C made B work again. - Takeover fences admitted actions. `handle_computer_use` re-checks the lease under the dispatch lock and discards a result produced after the lease epoch changed, so an action admitted before a takeover cannot picture what the human typed during approval / backend start-up waits. - Sudo reply pinned to its origin. `SudoRequest.origin` records the (connection, profile) the card came from; SudoDialog answers through `requestGatewayForAgent` on that socket, never the foreground gateway. A password typed for host A can no longer reach host B. `sudo.expire` and `display.install.sudo.expire` now tear the card down (the Desktop never handled sudo.expire). - Dock Browser IS the bot's browser. `tools/bot_desktop/browser.py` resolves one identity — the Chromium agent-browser drives + a persistent per-profile user-data-dir (`bot-desktop/browser-profile`) — and both sides use it: the agent env gets AGENT_BROWSER_EXECUTABLE_PATH / AGENT_BROWSER_PROFILE, the dock launcher gets the same exe + --user-data-dir. Live: the bot wrote localStorage on http://127.0.0.1:8765 through agent-browser; a dock click and a typed URL on the screen showed BOT-WROTE-THIS in Chrome for Testing. - Safe display reuse. Allocation under a host-wide lock; a recorded number is reused only when no live server holds it; the launcher never unlinks a lock whose pid is alive. Live: A stopped, B took :20, A restarted on :21, B kept running. - Install worker keeps the caller's profile scope (copy_context carries the HERMES_HOME override and the transport); the done event carries the requested profile's status. - Honest scope: `bot_desktop.auto_start` defaults to false (opt-in; Start lives in the Screen pane); request_handoff no longer claims a Telegram/Discord message was sent — the model relays the ask in its reply; docs match.
…ped viewer link keeps exclusion
…ames marked, lease fails closed Second review round (@Julientalbot): - Desktop `display.*` listeners (lease, install log/done) match on (connectionId, profile_key), not the profile path alone: two hosts with the same ~/.hermes path no longer repaint each other's screen pane or install card. One predicate, `isEventForBotScreen`. - Hero thumbnail: three consecutive failed refreshes dim the last frame and caption it "Last seen — screen unreachable"; a dead gateway never keeps looking live. - Lease file present but unparsable reads as HUMAN holds (fail closed); only a missing file is a fresh agent-held profile. The next successful write repairs it. - Taking over after `request_handoff` keeps the agent's reason on the lease and the pane shows it while the human acts, not only before they clicked Take over.
Reject oversized positive and extended clipboard lengths at the header. Process coalesced WebSocket data in bounded slices without rejecting valid multi-message frames. Includes fragmented-header and boundary regressions. Addresses the clipboard finding reported by carlotestor and corroborated by helix4u and other reviewers on NousResearch#108914.
Use the connection/profile event predicate in the portal, never match a legacy group to a remote namesake, and reset thumbnail state on owner changes. Component tests cover cross-host events, click routing, pending/rejected thumbnails, and stale responses. Addresses findings from Julientalbot, erosika, and BearHuddleston on NousResearch#108914.
Send close code 1000 before noVNC can send its statusless close. Keep reconnect teardown statusless so it does not release the human lease. Cover both lifecycle paths with component tests; verify the ordering against real noVNC and Xvnc separately. Addresses the close-code finding discussed by MrD1az, Xipong, and other reviewers on NousResearch#108914.
Match the existing gateway stop contract on supported hosts, including when the desktop has already exited. Keep the Linux lease import off unsupported hosts. Native Linux test exercises the CLI parser and real persisted lease. Addresses the CLI recovery finding from MrD1az and subsequent reviewers on NousResearch#108914.
gaoanze888
left a comment
gaoanze888
left a comment
There was a problem hiding this comment.
The clipboard-header rejection, viewer close ordering, and route-scoped renderer ownership changes are useful, but three high-risk lifecycle/resource gaps remain at exact head e38eb76c6fefb64a9de094a17c6d8b4b243d7731:
-
CLI recovery releases human exclusion before confirming Desktop stopped.
screen stopcallslease.release()beforeruntime.stop(). If stop raises (for examplekillpgreturnsPermissionError), the command exits with the Desktop still live but the persisted holder changed to agent, allowing automated screen actions while a human may still be interacting. I reproduced that state transition with a raising stop stub. Release only after a successful stop, while preserving the already-exited recovery case. -
The persisted PID is not identity-bound before killing its process group. Any numeric live PID from
launcher.pidis accepted, thenSIGTERM/SIGKILLtargets its entire process group. After crash/PID reuse—or same-user state-file modification—screen stopcan terminate an unrelated group. Persist and verify a process identity (at minimum start time plus expected launcher/session characteristics) and reject symlink/non-regular PID state before signaling. Add stale/reused PID tests. -
The advertised RFB bound does not bound frame/connection memory.
feed()consumes input in_MAX_BUFFERslices but appends every accepted message to one unbounded outputbytearray; the display WebSocket route accepts frames up to 384 MiB and passes the complete frame intofeed(). A coalesced frame of valid small RFB messages therefore retains the original frame plus an equivalently large output allocation. A 20 MiB local valid-message probe returned a 20 MiB result despite_MAX_BUFFERbeing ~256 KiB. Stream bounded output to the bridge or impose a suitably small display-route frame cap, and test peak/batch behavior rather than only parser-buffer size.
Residual concurrency risk: runtime.start() checks live PID before publishing its own PID without a lifecycle lock, so concurrent starts can both pass and overwrite launcher.pid.
Focused Python runtime/RFB/lease/CLI tests pass 19/19 with one platform skip; Ruff and diff checks are clean.
alt-glitch
added
type/bug
d947fc8 to
d65af42
Compare
|
Thanks — all four commits (f96333d, 7bc75c0, b9ccd54, e38eb76) are cherry-picked into #108914 with your authorship preserved (the |
d65af42 to
bc36ddb
Compare
|
Follow-up on the review findings against e38eb76:
Residual start/start race: agreed, the check-then-publish on launcher.pid wants the lifecycle lock. Thanks for the precise review, each finding reproduced or confirmed in code before answering. |
|
All three findings addressed on the rebased branch (now on current main), each red-proven:\n\n1. Lease-before-stop: CLI path reordered (raising stop stub keeps the lease, already-exited still releases). The gateway twin had the identical bug and is fixed the same way with 2 new tests.\n2. PID identity: launcher now records pid plus create_time, session id, and cmdline; stop signals only on exact match including session-leader check, rejects symlink/non-regular state, cleans stale records. 6 new tests (reused-PID killpg booby-trapped, real sleep child for the live path).\n3. Frame memory: feed() caps per-call output at 1 MiB, display route refuses single frames over 1 MiB with close 1009. 5 MB batch raises, chunked 5 MB streams with peak under 1 MiB byte-identical, oversize WS frame dropped unforwarded.\n\n46 Python tests green, ruff and diff checks clean, tsc clean, screen-connection plus isolation suites 7/7 green. The residual concurrent-starts lock is deferred: serializing needs a cross-process lock whose test requires multiprocess orchestration, fcntl won't exclude same-process threads. |
4 participants
Scope
Follow-up to #108914, based on
d947fc83a461. Targets the Bot Screen feature branch, not main. Converts six already-reported defects into four separately cherry-pickable commits. AI-assisted implementation and verification.Fixes
screen stoprecover a stranded lease even after the desktop has exited. Do not add a Unix-only import on unsupported hosts.Verification
tsc --noEmit, changed-file ESLint/Ruff, Windows-footgun scan of changed production Python, compatibility-pointer check, subprocess-stdin check andgit diff --checkpass.The broad macOS Python run was not green: 19,581 passed, 95 failed, 340 skipped. Re-ran all failing files on the unmodified PR head: the same 95 exact test node IDs fail, with no additional failing node from these patches.
The full Desktop run had 9,789 passing tests, one failing test and six skips, plus two collection failures from an initially uninstalled Electron binary. Installing the pinned binary resolved both collection failures. The remaining managed-SSH test fails identically on the unmodified PR on this macOS host.
Reproduction script, live receipt, exact baseline comparison and verification scope.
Limits
The live fixture exercises the production display route and ticket store, but mints tickets directly on loopback. It is not a packaged Electron or authenticated main-gateway acceptance test. No real credentials, login accounts or external sites were used. Native Windows, managed Cloud, distro installers and the other reported P1s remain outside this patch. This is not a blanket approval of #108914.
Credit
The original findings came from @carlotestor, @Julientalbot, @MrD1az, @Xipong, @erosika, @BearHuddleston, @helix4u and the reviewers who corroborated them. This PR supplies patches and additional executed evidence rather than re-filing their findings.