Repository navigation
feat(s16): watch and take over a desktop working computer (DESK16-01 slice 1) - #451
Conversation
Adds the DESK16-01 first slice: an opt-in desktop VM profile (virtio display, tablet and keyboard reached only through the controller's QMP socket), a host-side screen device (screendump frames, input-send-event input), exclusive owner screen control with generation fencing of every working-VM operation, the s16-computer-screen/v1 owner API and the Watch screen view on the Computers page in English and Thai.
[Medium impact] The reviewed changes appear safe to merge. SummaryThis PR adds opt-in screen viewing and owner control through QEMU, with work blocked during takeover.
|
…; keep Shift in shortcuts - Input request ids are kept for the current owner generation: a retry joins or returns a finished send and refuses one whose outcome is unknown (screen_input_unknown); only a refusal before sending releases the id. - The screen view ignores polls that started before a command, and a frame is shown only for the current available generation. - A browser double-click sends its two clicks only. - Shift is part of every forwarded key combination.
chayapats
left a comment
There was a problem hiding this comment.
Verdict: do not merge yet. 1 BLOCKER, 3 SHOULD, 1 NIT. Head e7323c8. This is a comment, not an approval.
I read the branch, the diff against main, contract s16-computer-screen/v1, ADR 0090, and the screen tests. CI on this head is green. I did not re-run the suite here (no local bun); the blocker below is a path those tests do not execute.
BLOCKER — production captures are stored
src/s15/vm/qemu.ts:292-297
capture() writes screen.png under the VM hypervisor directory and deletes it only when host.vmm is unset:
if (!self.host.vmm) rmSync(file, { force: true });
await self.qmp([{ execute: "screendump", arguments: { filename: file, format: "png" } }], 10_000);
try { return vmScreenFrame(new Uint8Array(readFileSync(file))); }
finally { if (!self.host.vmm) rmSync(file, { force: true }); }VM15-03 is the production path (UMask=0007, machine dir mode 0750, QEMU user owns the file). The controller can read a group-readable frame and cannot unlink it, so this branch never removes it. stop() / idle sleep do not remove it either. The last frame stays on disk after the owner stops watching, and after the QMP socket is gone.
That violates this PR's own contract (docs/contracts/s16-computer-screen-v1.md:49, "Frames are never stored") and #410's acceptance line "no credentials leak through captures/history". Anything visible on the display — a token the bot printed, or text the owner just typed — remains in a group-readable PNG. ADR 0090 line 26 records the leftover as the design; the contract and the ticket do not accept it.
The test that claims the file does not stay (tests/s16-computer-screen.test.ts:67-74) uses FakeQemuHost.options(), which sets no vmm. It cannot fail this path.
Fix direction: a launcher op that unlinks only $dir/screen.png as the QEMU user, called from that finally (and on stop). Add a vmm-host test that the unlink is invoked and the file is not left behind. Do not treat "next capture overwrites it" as deletion.
SHOULD
-
Double-click does not survive the Greptile fix.
src/s16/web/ComputerScreen.tsx:123always sendsdouble: false. A browser double-click is twoclickevents, so two separate HTTP inputs.vmScreenCommandsalready emits both clicks in one QMP batch whendouble: true(ADR 0090 promises double-click). Two round trips will usually miss the guest double-click threshold. Delay the first click briefly and send onedouble: truecommand if the second arrives; do not send the two singles as well. -
The driving copy is false, and the masked field looks like a protected channel.
src/i18n/en.ts:6178("Only your input reaches this computer") and the password input atComputerScreen.tsx:129. ADR 0090 already says a process the work started earlier keeps running, and guest root can read its own screen and input devices. The ticket fences the controller's SSH path, not/dev/input. Sensitive-site login is supposed to stay on the protected browser; this UI does not say that at the point of typing. Say that the bot can still see this screen, and that sensitive-site passwords do not belong here. Same keys inth.ts. -
docs/contracts/s15-working-vm-v1.md:41still says the only exact argv has no display. This PR makes a second argv exact and does not amend that contract. Update it in this PR so the two contracts do not contradict.
NIT
src/s16/web/ComputersView.tsx:122 still offers "sleep now" while the owner is watching, whenever activeSessions === 0. idleBlocked only covers the automatic sweep. Stopping the machine under an active takeover is an explicit click, but it is easy to hit. Disable it while screen state is not agent.
Checked, not broken
- Headless
qemuArgvis unchanged versusmainwhendesktopis omitted.desktopdefaults off, so an existing VM15-03 host does not start getting a sixthstartword until the flag is set. - Greptile's five comments match the code at
e7323c8: input ids are not evicted at 256, a send that may have reached QEMU isscreen_input_unknowninstead of resent, polls do not overwrite a newer command, frames render only for the current generation, Shift stays on modified shortcuts. - Agent ticket vs takeover:
agentInputand the takeover write are synchronous, andbetter-sqlite3transactions do not yield. I could not construct a shared writable generation. In-flight guest work stays insettlinguntil the ticket ends; hand-back waits forownerTail; a queued input is refused (#closing). Restart and lease lapse fence, and work does not get the screen back. - Frame GET is owner-authenticated,
no-store,CORP: same-origin, and the session cookie isSameSite=Strict. Cross-sitesec-fetch-siteis rejected. Typed text is not written to the screen journal. Protected-browser files are not in this diff. - CSP adds only
img-src blob:.
Open, not charged as findings
- Whether
-sandbox on,obsolete=deny,resourcecontrol=denyallowsscreendumpto create the file, and whether the cloud image actually shows a console on virtio-vga, are live-host questions. #431 owns that evidence. A black screen would not excuse the stored PNG. tests/s16-computer-screen.browser.test.tsis skipped unlessJIT_S16_BROWSER_TESTS=1. That variable is not set in CI, and the PR notes Playwright was not run. The skipped file is not rendered-path evidence.
Not in scope for this slice
No guest desktop session, no desktop-browser egress, no live jit-vm proof. Refs #410 rather than Closes #410 is correct. Do not merge the control plane while the production capture path stores frames.
…e click; honest driving copy - capture() always removes screen.png before it settles; under VM15-03 the launcher's new discard-frame <vm> op removes exactly <vm>/screen.png as the QEMU identity. A frame that cannot be removed fails the capture. - stop() (also the idle stop) discards any frame left behind; launcher start removes a stale frame with the stale QMP socket. - A double click is sent as one double:true click (single clicks wait 250 ms). - Driving notice no longer claims only owner input reaches the computer: guest programs can still see the screen; sensitive sign-ins use the protected view. - Sleep now is disabled while that computer's screen is open. - s15 working-VM contract, s16 screen contract and ADR 0090 updated. Review: #451 (review) Refs #410
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Fixed in 2d252f0 (pushed on top of e7323c8; no force push). BLOCKER: frame left on disk under VM15-03
SHOULD
NIT: "Sleep now" is disabled while that computer's screen is open. Verification (local): |
chayapats
left a comment
There was a problem hiding this comment.
Verdict: no BLOCKER left on head 2d252f0. This is a comment, not an approval. 1 SHOULD, 1 NIT.
I read the commit, capture/discardFrame/stop, the launcher, the screen contract, and the new tests. I did not re-run the suite here (no local bun). CI on this head is green: typecheck, all 12 test shards, and deterministic.
BLOCKER closed — frames do not survive a successful capture under VM15-03
src/s15/vm/qemu.ts:291-298 reads screen.png inside try and finally always calls discardFrame() before the capture promise settles. A throwing discard replaces the return, so a frame that is still on disk is not shown.
discardFrame() (qemu.ts:363-365) is the launcher op when host.vmm is set, otherwise rmSync. hypervisor(["discard-frame"]) sends only discard-frame <vm>. The launcher (vmm-launcher.ts:129-131) refuses extra arguments and runs rm -f -- "$dir/screen.png" as the QEMU identity. start also removes that file (vmm-launcher.ts:140). stop() (qemu.ts:371-373) discards after halt(), and idle sleep calls manager.stop().
The launcher test runs the rendered script: it deletes only that VM's frame, leaves the other VM's frame, is idempotent, and refuses discard-frame <vm> screen.png. The VM15-03 host test writes a real screendump into the QEMU directory and checks the file is gone after a capture, after vm_screen_invalid, and after stop. One device is cached per manager (composition.ts), so captures on that device stay serialized.
Not reopened: a crash between screendump and discardFrame can leave one file until the next capture, stop(), or launcher start. A discard that fails after a proven halt fails stop() and fences vm_stop_unconfirmed, and the file stays until a later successful discard. Both are fail-closed leftovers, not the old path that never deleted.
SHOULD — a second left click within 250 ms drops the first, unless the browser already calls it a double click
src/s16/web/ComputerScreen.tsx:108-116
leftClick clears the pending single before it looks at event.detail. Any second click inside doubleClickMs (250) replaces the first. Only detail === 2 while that timer is still pending becomes one double: true click.
Failure: owner is driving, clicks (100, 200) with detail 1, then 80 ms later clicks (100, 240) with detail 1 (a different target; the browser does not count it as a double click). The first timer is cleared. One single click is sent, at the second point, 250 ms later. The first click never reaches QMP. Click-then-key inside that window is also reordered: the key is sent now, the click after.
A double click slower than 250 ms (the usual OS interval is about 500 ms) still becomes two singles, because the first timer has already fired and detail === 2 && waiting is false. The browser test only does an instant Playwright dblclick, and it was not run here. CI does not start that suite (JIT_S16_BROWSER_TESTS).
Fix direction: flush a pending single immediately when the new click is not detail === 2; upgrade to double: true only for that case. Do not cancel a distinct click. 250 ms is also shorter than the interval the browser uses to set detail.
Fast double clicks are fixed. This is the remaining hole in that fix, not a new credential leak. This slice still has no guest desktop, so I am not calling it a BLOCKER.
NIT — the component comment still makes the claim the notice no longer makes
ComputerScreen.tsx:26-27 still says only the owner's input reaches the screen. The en/th s16.screen.driving strings now say programs on the computer can see the screen and what you type. The comment should match, or the next edit will copy the old claim back into the UI.
Dropped
Driving copy (en and th) and the s15 argv sentence are updated. Sleep now is disabled while that computer's screen is open (ComputersView.tsx, watching === item.id). Greptile's 5/5 is still on e7323c8, not this head.
…click window; honest screen comment
|
Fixed Duo's review 5820489898 in
Local: |
…o discard-frame - A waiting left click is sent before any later key, text, right click or scroll, and screen input goes out one at a time in order, so typing lands where the owner clicked. - stop() asks the launcher to discard a frame only when the controller has the desktop opt-in; a headless VM15-03 install with an older launcher keeps stopping and sleeping.
|
Fixed Greptile's two P1s on
Local: |
Greptile P1 on e580858 (Old keys cross takeovers): each queued input now carries the generation and takeover id from when the owner made it (a pending click keeps the ones from its press), and input still waiting when control changes or the panel closes is dropped instead of sent. The queue lives in screenInput.ts with unit tests.
|
Fixed Greptile's P1 on Old keys cross takeovers (
Local: |
chayapats
left a comment
There was a problem hiding this comment.
Verdict: no BLOCKER — Greptile P1 "Old keys cross takeovers" is fixed at 3c8bf90
Reviewed e580858..3c8bf90 against ComputerScreen.tsx, screenInput.ts, and the server check in src/s16/computers/screen.ts (held / input). CI on 3c8bf9080fce22732db086ebd6b06c95f9f35d87 is green. I did not re-run the suite locally.
The failure Greptile described was: waiting keys left the queue with the current generation and takeover id, so after another tab handed back and took control again they passed held() and acted in the new session.
That path is closed:
screenInputQueuestores the pin frompushandholds()drops the item unless the live view is stillownerwith that same generation and takeover id.close()drops anything not yet sent.enqueue/ the pending single click capture the pin when the owner made the input, not when it leaves the queue.send(..., pin)puts that pin on the command (generationandtakeoverId), notviewRef.- Polls and command results write
viewRefbeforesetView, so the drop does not wait for a re-render. - If a request is already on the wire, the server still refuses a mismatched pin (
stale_screen) immediately beforedevice.send. Hand-back waits on#ownerTail, so an in-flight send finishes under the old takeover instead of being rebound.
The new unit tests cover order, re-takeover, hand-back, fencing, panel close, and a failed send. They do not mount ComputerScreen; I checked that wiring by reading it.
SHOULD — composed "type on the computer" text is not waiting input, so it still crosses
src/s16/web/ComputerScreen.tsx form submit (the screen-type form) calls input({ kind: "text", text: value }), which pins at submit. driving is only view.state === "owner", so another tab's hand-back + new takeover leaves this form mounted and typed intact (the cleanup effect depends on [driving], which does not change). Submit then sends the old string with the new ids, and held() accepts it. Same harm as the P1 (wrong field / unwanted action), but only if the owner submits after the change — not automatic, and not a regression of 3c8bf90 (previously ids were taken even later, at send time).
Fix direction: when generation or takeoverId changes, clear typed (and the pending click timer). A test that a string typed under takeover A is not sent after the view becomes takeover B.
Not a merge blocker for the queued-key P1. No other finding survived a concrete scenario.
⚖️ Tria ruling on Duo's review at
|
The type-on-the-computer field keeps its text with the generation and takeover id it was typed under. A hand-back, fencing or a new takeover (any tab) clears the text, the show choice and a waiting click; Send queues the text under its own pin, so the queue drops it if that control has ended. Tests: text typed under takeover A is not sent after takeover B.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Fixed per Tria's ruling (#451 (comment)) in 8235ea8 (normal push):
Local: @greptileai please re-review 8235ea8. |
chayapats
left a comment
There was a problem hiding this comment.
Verdict: no BLOCKER on head 8235ea8. This is a comment, not an approval. The SHOULD from review 5820821899 is fixed. The optional NIT is done. No new finding.
Reviewed 3c8bf90..8235ea8 only (ComputerScreen.tsx, screenInput.ts, tests/s16-screen-input.test.ts) against the live queue and ScreenControl.held. Locally bun test tests/s16-screen-input.test.ts is 8 pass / 0 fail. CI on 8235ea8c48d816ffa492062b3ca215b482099472 is green (typecheck, 12 shards, deterministic). Greptile approved this commit; I did not treat that as evidence.
SHOULD closed — typed text does not cross a takeover
src/s16/web/screenInput.ts:21-33, src/s16/web/ComputerScreen.tsx:129-135, :178-181.
The failure was: typed survived a hand-back and a new takeover, and Submit then sent that string under the new generation and takeover id.
Now the field is a ScreenDraft pinned at edit time (editDraft). liveDraft returns it only while holds matches that pin, so the controlled value is empty after a hand-back, a fence, or another takeover. Submit recomputes liveDraft(draft, viewRef.current) and queues under value.pin, not under screenPin(viewRef) at send time. Passing null does not hit enqueue's default (that default applies only to undefined), so a dead draft cannot be re-pinned. The queue still drops a waiting input whose pin no longer holds, and held() still rejects a mismatched pin before QEMU. This commit does not touch that server check.
The cleanup effect also clears the draft, the show-password choice, and a waiting click when driving, generation, or takeoverId changes (ComputerScreen.tsx:129). That matches the ruling, including the optional click-timer clear. A renew does not change generation or takeover id (screen.ts renew only updates expires_at), so the effect does not wipe the field every 10 seconds.
The required test is in tests/s16-screen-input.test.ts: text typed under takeover A is not sent after takeover B, including a stale draft pushed with A's pin. It exercises the decision functions the component calls. I also read the component; Submit is the only path that sends the field, and it uses those functions.
No new BLOCKER, SHOULD, or NIT.
⚖️ Tria ruling on Duo's re-review at
|
Summary
First slice of DESK16-01 (#410): the owner can watch a working computer's screen, take exclusive control, type and click on it, and hand it back. Work is fenced the whole time. Design and alternatives are in ADR 0090, and the contract is
s16-computer-screen/v1.Chosen design. I compared the pinned Rakazo approach (
abf3c08). Rakazo runs Xvfb, x11vnc and noVNC inside the sandbox, uses a sealed screen capability and gates interaction with a control lease. Jit can't copy that directly: the agent has root in the Jit working VM, so it would control a guest VNC server and that server's input path. In this PR QEMU owns the display instead. Frames come from QMPscreendumpand input goes through QMPinput-send-event, both over the existing controller-only QMP socket. The PR adds no display server, port or guest channel, and the controller enforces fencing before any event reaches QEMU. No upstream source or dependency is imported.s15-runtime/v1getsdesktop: true, which requirescomputers. Jit-created VMs then start withvirtio-vga/virtio-gpu-pciat 1280×800,virtio-tablet-pciandvirtio-keyboard-pci.-display nonestays./procre-verification accepts either exact profile. A VM that is already running headless keeps working and reportsvm_desktop_restart_requireduntil its next start.startword,desktop.src/s15/vm/screen.ts,QemuVmManagement.screen()).screen_text_unsupportedotherwise, never approximated).src/s16/computers/screen.ts).agent,settling,ownerandfenced. Every transition advances the generation.WorkingVmServiceholds an agent ticket for every operation from admission to settlement, and checks it wherever it already checks session currency (claim, dispatch, before start, publish).settlinguntil earlier operations have settled. Owner input is refused until then.fenced. Work never gets the screen back just because a tab closed.requestId. Input is deduplicated in memory only, and no typed text, hash of it or frame is journaled.GET .../computers/<vm>/screen,GET .../screen/frame?generation=N(PNG,no-store) andPOST .../computers/screen/commands, owner-authenticated with CSRF.screen,screenFrameandscreenCommand.img-src blob:only, so in-memory frames can render.Not delivered here (honest pending)
jit-vm(real QMP screendump/input, VM15-03 file permissions, restart into the desktop profile) belongs to [Frontier] [LIVE16-01] Collect exact-build Linux, account and native-device evidence #431. An installed VM15-03 launcher must be refreshed by the installer before desktop starts there.Tests
bun test --timeout 30000 tests/s16-computer-screen.test.ts: 19 pass. It covers:FakeQemuHost: frames and input via QMP only, file removed, restart required for a headless VM.WorkingVmService: a response that arrives after takeover is not published, and nothing new is admitted until hand-back.tests/s15-vm-vmm.test.ts:startargv byte-for-byte for both architectures.id -unidentity check. The same 4 existing cases fail identically onmainhere. CI is the reference.s16-computers,s16-computer-files,s16-client,web-ui,s15-vm,s15-vm-qemu,s15-vm-pool,s15-vm-provisioning,s15-vm-local,s15-vm-enrollment,s16-upstream,s16-steering: 208 pass. The 1 failure isdescriptor-anchored search … real rg;rgis not installed on this sandbox, and it fails the same way onmain.tests/s16-computer-screen.browser.test.ts(opt-inJIT_S16_BROWSER_TESTS=1, 1440 en / 390 th) is added but was not run: Playwright cannot launch Chromium on this sandbox.img-src blob:CSP, which is fixed here.git diff --checkpasses.bun run typecheckpassed before the final test and docs commits. Later runs were OOM-killed in this 2 GB sandbox, so CI's typecheck is the reference for the head.Refs #410, Refs #392