feat(desktop-host): add display keeper for private X screens - #21
Merged
Merged
Conversation
desklink-host keep --display :N claims SubstructureRedirect on one private X screen (refusing a second window manager by name), fills the screen with every normal top-level window, parks windows smaller than 120 px off-screen, keeps the input focus on the newest window, and reports the screen's windows as JSON lines on stdout - debounced 100 ms, capped at 300 ms - exiting by itself when the display goes away. A consumer reading the stream learns what is really mapped, which is how a pane's browser or emulator is seen without asking any tool. @desklink/host exports startDisplayKeeper() to spawn the engine in this mode and parse its stream, and docs/PROTOCOL.md documents it. resolveEngine's darwin-missing test now takes the package root explicitly so it no longer depends on whether this machine has a source build.
…tdout error line; focus never lands on parked helper windows
…ches content-window focus
…setup as normal exit 0
…ction exits 0, not refusal
Firstmate decision on review finding F5 (run 01M3MF5956J5B41EAG039T3GQD): the keeper accepted both the documented space-separated flag and an equals-sign spelling; only the documented form remains, with the parse pinned by a unit test.
…in-1, not UTF-8-lossy
umeranjum17
force-pushed
the
fm/dl-display-keeper1
branch
from
September 28, 2026 21:22
86a7f3e to
74ac42f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
The owner wants muxr to show the agent's browser and Android emulator live, but only when an agent is actually using one, smooth and high frame rate, shown in a proper classic way (e.g. a tooltip/chip), and agent-agnostic across browser tools; the previous in-app Browser was removed because "it never worked". Owner's words: "that browser preview should not always show it It should only show when it is done properly ... we should have like a browser link or something also with good attachment, Good frames per second ... it has to be shown in a classic like proper fashion for example maybe a tool tip shows when an agent is using a browser ... the preview is like smooth ... and it should be agent agentecnostics". The owner also asked to "Parallelize" and wants frontend "super polished and well designed" so competitors look worse.
What Changed
keepengine command: a kiosk window manager for one private X screen that fills content windows to the full screen, parks tiny helper windows (<120 px) off-screen, keeps input focus on the newest content window, and streams a JSON-lines window report (id, title, class, pid, size) after every screen change — exiting 0 when the display goes away and refusing with one{"error":...}line plus non-zero exit when another manager owns the screen or the display cannot open.startDisplayKeeperTypeScript wrapper that spawns the engine withkeep, parses its stdout into window/refusal/diagnostic callbacks, reports exit details, and stops it with SIGTERM-then-SIGKILL;resolveEnginegains an injectable package-root parameter.keepin the launcher and engine help, and add a scriptable X-client fixture plus a live Xvfb flow test and unit specs covering the keeper and report parsing.Risk Assessment
Testing
Drove the change live end-to-end: built the engine and fixture (cargo), built the TS dist, then ran the change's own keeper-flow proof plus a custom Latin-1 regression against the real running product — one private Xvfb per scenario, its own auth cookie, high display numbers, no capture or input ever touching the real desktop. The keeper claimed a private screen as its kiosk WM and reported {"windows":[]}; a content window was filled to 0,0,1280x720 and reported with title/class/pid; a 100x80 emulator-toolbar helper parked at -30000,-30000 while the content window stayed fullscreen; focus stayed on the newest content window and never on parked helpers; unmap/destroy updated reports within the debounce; a client resize request got the kiosk geometry back; a second keeper refused with {"error":"another window manager"} exit 1 while the first kept running; killing the Xvfb made the keeper exit 0 with no error line, and 11 startup-timing kills all honored the display-gone contract; an unopenable display and a bare
keepeach refused with exactly one error line and exit 1; the TS startDisplayKeeper wrapper drove the real engine (empty first report, then live window reports, one exit detail after stop(), refusal carried on the exit detail). A window with only a Latin-1 WM_NAME reported "Größe" — the target commit's fix working, and what the pre-fix code mangled. All 13 flow scenarios and the Latin-1 regression passed live; 8 Rust and 11 TS unit tests also passed but those are unit coverage, not a live result, so the unit-level-contracts scenario is recorded untested. One environment snag (shared /tmp tmpfs usrquota exhausted) broke vitest initially; redirecting TMPDIR=/var/tmp fixed it — not a product issue. Verdict: go.desklink-host keep --display :Non a private X screen; the keeper wins the WM election and reports the empty screen as one {"windows":[]} linekeepwith no --display refuses with {"error":"no display given"} and exit 1 — it never hunts for a screen to manageEvidence: Full keeper-flow live transcript: 13/13 scenarios pass with keeper stdout, fixture replies and exit details
Evidence: Live Latin-1 WM_NAME fallback regression (target commit) with its ctypes Xlib client
client: mapped 4194305 keeper report line: {"windows":[{"class":null,"height":600,"id":4194305,"pid":null,"title":"Größe","width":800}]} PASS: WM_NAME Latin-1 fallback decoded as "Größe" (not the pre-fix "Gr\uFFFD\uFFFD")Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
packages/desktop-host/engine/src/keeper.rs:442- The WM_NAME fallback title is decoded as UTF-8-lossy instead of Latin-1, contradicting the code's own contract (keeper.rs:88-89 'the WM_NAME fallback is Latin-1') and PROTOCOL.md:566. Concrete sequence within intended usage: the consumer spawns the keeper on its private Xvfb and launches a client that sets only WM_NAME with type STRING (Latin-1 per ICCCM — legacy toolkits, xterm in a non-UTF-8 locale) with a non-ASCII title, e.g. 'Größe' (0xDF,0x6F); snapshot()'s fallback (keeper.rs:439-443) runs those bytes through decode_title's from_utf8_lossy, so each byte >= 0x80 becomes U+FFFD and the report line carries 'Gr\uFFFD\uFFFD' instead of 'Größe' — a wrong value with no error, which is exactly what the consumer's tooltip would display. ASCII-only fallback titles are unaffected. Same invariant elsewhere in the changed code: decode_wm_class already decodes Latin-1 correctly via the existing latin1() helper (keeper.rs:85, 444-446), and the _NET_WM_NAME path (keeper.rs:438) is intentionally UTF-8 and must stay lossy. Fix: decode only the WM_NAME fallback bytes with the existing latin1() helper (trim, empty -> None) instead of reusing decode_title for both properties.🔧 Fix applied.
2 issues (1 warning, 1 info) still open:
packages/desktop-host/src/keeper.ts:45- parseKeeperLine crashes on the one JSON line its own contract says must return null: a line containingnull. Sequence: JSON.parse('null') yields the value null, and the very next statement reads parsed.error (keeper.ts:45) — a property access on null throws TypeError, while string/number/boolean primitives pass harmlessly. The docstring promises null for 'not JSON and not the keeper's business', and the sibling entry guard at keeper.ts:49 (typeof entry !== 'object' || entry === null) already applies exactly this invariant one level down, so only the top-level parsed value is unguarded. Concrete path: the function is exported from the package index as public API for consumers classifying engine stdout lines; inside startDisplayKeeper's readline 'line' handler (keeper.ts:87) the throw is an uncaught exception that kills the consumer's process. The shipped engine binary never printsnull, so the crash needs a non-conforming or wrapped engine's stdout — but the parser's whole job is to tolerate such lines. Fix:if (typeof parsed?.error === 'string')(or an explicitparsed === nullguard beside the entry guard).packages/desktop-host/engine/test/keeper-flow.mjs:23- The live proof defaults its evidence directory to import.meta.dirname when KEEPER_FLOW_EVIDENCE is unset, so a default run writes keeper-flow.log plus PNG screenshots into packages/desktop-host/engine/test/ inside the source tree. Project AGENTS.md keeps evidence artifacts out of PR branches and the sibling axi flows take their output paths from env vars rather than defaulting in-tree, so a default-on in-tree write is a mild footgun if a later phase stages the worktree wholesale. Header documents the env var, so this is awareness only; a mkdtemp default would remove the hazard.✅ **Test** - passed
✅ No issues found.
desklink-host keep --display :Non a private X screen; the keeper wins the WM election and reports the empty screen as one {"windows":[]} linekeepwith no --display refuses with {"error":"no display given"} and exit 1 — it never hunts for a screen to manageKEEPER_FLOW_EVIDENCE=<evidence dir> node packages/desktop-host/engine/test/keeper-flow.mjs — 13 live scenarios against the realdesklink-host keepbinary and the real TS wrapper on private task-owned Xvfb screens (own cookie, high display numbers, never the ambient desktop)node <evidence>/latin1-fallback.mjs — live regression for the target commit: raw ctypes Xlib client creating a window whose ONLY title property is WM_NAME (STRING) with non-UTF-8 Latin-1 bytes b"Gr\xf6\xdfe"; keeper reported title "Größe", not the pre-fix "Gr\uFFFD\uFFFD"cargo test keeper — 8 engine unit tests (kiosk geometry, WM_CLASS decode, title decode incl. Latin-1, report shape, refusal contract, single --display spelling)TMPDIR=/var/tmp npx vitest run packages/desktop-host/src/keeper.spec.ts packages/desktop-host/src/resolveEngine.spec.ts — 11 TS tests (parseKeeperLine contract, spawn wiring, refusal/never-spawned exit details, stop())Cleanup verified: worktree git-clean (build outputs node_modules, engine/target, dist removed); stale Xvfb locks/sockets from dead test runs removed from /tmppackages/desktop-host/README.md:7- Judgment call: the display keeper is absent from the package README's 'What it does' list; PROTOCOL.md's 'Display keeper' section is the sole documentation surface and is the owner of the contract. Verified accurate against the final code (single --display spelling, Latin-1 WM_NAME fallback, exit-0/display-gone and one-line-refusal contracts, 100/300 ms report bounds, null-not-absent fields). Left the README untouched deliberately: it never enumerates engine modes or commands, the change's own docs commit scoped updates to PROTOCOL.md plus launcher/engine help, and adding a keeper bullet would be opportunistic expansion. If the owner wants the public TS API (startDisplayKeeper/parseKeeperLine) introduced in the README, that is a one-bullet follow-up.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.