(MOT-4001) feat(computer): add the computer worker - #673
Conversation
Full-desktop computer use on the bus, the sibling of the browser worker: `computer::*` starts a desktop session, screenshots it, and clicks, types, scrolls and drags by coordinate, so a model that can see an image can operate a GUI with no glue in between. One `Driver` trait, three ways to reach a desktop, picked per session: this machine (capture and input built in), a desktop booted inside an iii-sandbox microVM and driven through `sandbox::exec` alone, or a remote desktop reached through the executor inside it. Sessions are durable — mirrored into `state` and reconnected best-effort on boot — and a screencast pump keeps `computer:frames` fed so watchers follow the screen without polling. Sibling workers bind `computer::session-started` / `session-stopped` instead of asking. The worker also ships its own console page (`#/ext/computer`): a session rail, a live viewport, and click/type/scroll forwarding, plus how every `computer::*` call renders in chat. Injected at registration, so no console rebuild. macOS degrades screen capture and synthetic input silently rather than failing, so the native driver checks both permissions and fails loud instead of handing back a wallpaper-only screenshot or a click that never landed.
Register the worker with the pipeline it needs to ship: the Create Tag choice list, the `computer/v*` release trigger, and a row in the modules table. Deny the console plumbing (`screencast::*`, `frame`, the config-change hook) in the permissions defaults, the same split the browser worker uses. The desktop surface an agent actually calls stays at the needs-approval default, because every act is a real click on a real machine.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 52 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughChangesComputer worker
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
computer/images/desktop/Dockerfile (1)
30-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd and use a non-root guest user.
This image has no
USER, sosandbox::execstarts Xvfb/openbox/xdotool/imagemagick as the image default unless the engine execs them as a non-root UID. Add a desktop user, ensure/tmp/.X11-unixis world-writable with the right ownership, and switch the bootstrap/exec path to run as that user.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/images/desktop/Dockerfile` around lines 30 - 58, Add a dedicated non-root desktop user in the Dockerfile, configure `/tmp/.X11-unix` with appropriate ownership and world-writable permissions, and set the image’s `USER` to that account so Xvfb, openbox, xdotool, ImageMagick, and sandbox::exec run as the guest user. Ensure the user has the required home/runtime environment while preserving the existing package installation and ImageMagick policy setup.Source: Linters/SAST tools
🟡 Minor comments (21)
computer/architecture/internals.md-46-48 (1)
46-48: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDescribe restart recovery as best-effort.
The implementation attempts reconnection, but it cannot guarantee that every desktop remains available after restart.
computer/architecture/internals.md#L46-L48: Replace the unconditional final clause with a best-effort statement.computer/skills/SKILL.md#L20-L23: State that restoration can fail and that a new session may be required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/architecture/internals.md` around lines 46 - 48, Update the restart-recovery documentation in computer/architecture/internals.md lines 46-48 to describe Sessions::restore reconnection as best-effort rather than guaranteeing live desktops remain available. Also update computer/skills/SKILL.md lines 20-23 to state that restoration can fail and a new session may be required.computer/README.md-5-7 (1)
5-7: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument keyboard inputs separately from coordinate inputs.
The shared wording treats
type,press, andhotkeyas coordinate actions, but those operations usetextorkeys.
computer/README.md#L5-L7: Remove the claim that typing is by coordinate.computer/README.md#L105-L108: Remove “all by coordinate”.computer/skills/SKILL.md#L61-L62: State that coordinates apply only to pointer and scroll actions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/README.md` around lines 5 - 7, Update the computer documentation to distinguish keyboard inputs from coordinate-based actions: in computer/README.md lines 5-7, remove the claim that typing uses coordinates; in computer/README.md lines 105-108, remove “all by coordinate”; and in computer/skills/SKILL.md lines 61-62, state that coordinates apply only to pointer and scroll actions while keyboard operations use their text or keys inputs.computer/architecture/integration.md-7-7 (1)
7-7: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winLimit the session identifier rule to session-scoped calls.
Global functions such as
sessions::listanddisplaysdo not requiresession_id.
computer/architecture/integration.md#L7-L7: Replace “Every call” with “Every session-scoped call”.computer/skills/SKILL.md#L53-L56: Replace “every other function” with the same scoped wording.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/architecture/integration.md` at line 7, Limit the session identifier requirement to session-scoped calls: update the wording at computer/architecture/integration.md lines 7-7 from “Every call” to “Every session-scoped call”, and update computer/skills/SKILL.md lines 53-56 from “every other function” to the same scoped wording. Preserve the clarification that global functions such as sessions::list and displays do not require session_id.computer/README.md-53-56 (1)
53-56: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument worker-config driver defaults consistently.
The docs imply that an empty start request always selects native, but
sessions::startresolvessandbox_imageanddefault_endpointbefore native selection.
computer/README.md#L53-L56: State that the native example requires both configured defaults to be empty.computer/skills/SKILL.md#L12-L15: Qualify “with no endpoint” with configured default resolution.computer/skills/SKILL.md#L43-L45: Apply the same qualification to the boundary rules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/README.md` around lines 53 - 56, Update the native-session documentation to reflect configured default resolution: in computer/README.md lines 53-56, state that the empty start request requires both sandbox_image and default_endpoint to be empty; in computer/skills/SKILL.md lines 12-15, qualify “with no endpoint” by noting configured defaults are resolved first; and in lines 43-45, apply the same qualification to the boundary rules.computer/README.md-122-124 (1)
122-124: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the hot-reload statement with field lifecycles.
Line 135 documents
command_timeout_msas fixed at connect, andconnect_timeout_msapplies during session start. “Timeouts ... hot-reload” is too broad. Identify the fields that update live and the timeout changes that affect new connections only.Suggested wording
-Timeouts and the screencast rate hot-reload; `default_endpoint` and `os` apply to sessions started after the change. +`screencast_fps` hot-reloads; `default_endpoint` and `os` apply to new sessions, and driver timeout changes apply to new connections.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/README.md` around lines 122 - 124, Update the configuration lifecycle statement near the computer worker documentation to name only the fields that hot-reload live, such as the screencast rate, and clarify that timeout changes affect new connections or session starts rather than existing sessions. Keep the documented fixed-at-connect behavior for command_timeout_ms and the session-start behavior for connect_timeout_ms consistent.computer/images/desktop/README.md-10-14 (1)
10-14: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winQualify the CI headless claim.
The image is designed for headless desktop sessions, but the repo docs and workflows do not show CI exercising the sandbox screenshot/input flows. Change line 14 to “designed for headless use” until that coverage is present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/images/desktop/README.md` around lines 10 - 14, Update the README sentence describing headless operation to say the image is “designed for headless use” instead of claiming it works headlessly in CI, without changing the surrounding explanation.computer/ui/src/page/SessionRail.tsx-6-10 (1)
6-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winOrdering claim does not match the render order.
The docstring states "every live session, newest first". The component renders
sessionsin the order received without sorting.computer/ui/src/page/index.tsxline 49 selectssessions[sessions.length - 1]as the newest session, so the list arrives oldest-first and the rail shows oldest-first.Either sort in the rail or correct the comment.
♻️ Option: sort newest first in the rail
- {sessions.map((session) => { + {[...sessions] + .sort((a, b) => b.last_used_ms - a.last_used_ms) + .map((session) => {Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/page/SessionRail.tsx` around lines 6 - 10, Update the SessionRail component’s session rendering to sort sessions newest-first before mapping them into rows, matching the documented ordering and the index.tsx convention that the last received session is newest. Keep the existing session row rendering and selection behavior unchanged.computer/ui/src/page/useLiveFrames.ts-67-81 (1)
67-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe screencast failure stays hidden when the screenshot fallback succeeds.
The comment on lines 68-69 states the hook surfaces why the screencast is unavailable. The code sets
erroronly when the screenshot also fails. When the screenshot succeeds, the user sees one static frame that never updates and gets no reason.Set the error in both branches.
Viewportshows the frame when one exists, so the error remains available for a non-blocking notice.🐛 Proposed fix
const shot = await takeScreenshot(host.iii, sessionId).catch(() => null) if (cancelled) return + setError(e instanceof Error ? e.message : String(e)) if (shot?.dataUrl) { setFrame({ dataUrl: shot.dataUrl, width: shot.width, height: shot.height, }) - } else { - setError(e instanceof Error ? e.message : String(e)) } returnNote:
Viewportrenders the error text only whenframeis null, so pair this with a small banner if the message must stay visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/page/useLiveFrames.ts` around lines 67 - 81, Update the screencast failure handling in the catch block of the live-frame hook to always call setError with the original failure reason, regardless of whether takeScreenshot produces a fallback frame. Keep the existing fallback frame behavior and cancellation guard unchanged so the error remains available alongside the static frame.computer/ui/src/page/index.tsx-40-51 (1)
40-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA new session loses selection until the list refresh lands.
handleStartsetsselectedIdto the new session at line 92.sessionsstill holds the previous list at that moment, so this effect does not find the new id and resets the selection to the last session in the old list.useLiveFramesthen starts a screencast on the wrong session and stops it again after the refresh.Keep the selection when the id is not present but the list has not yet caught up. One way is to track ids the page started.
🐛 Proposed fix
+ const pendingIdRef = useRef<string | null>(null) + useEffect(() => { if (sessions.length === 0) { - setSelectedId(null) return } setSelectedId((current) => { if (current && sessions.some((s) => s.session_id === current)) { + pendingIdRef.current = null return current } + if (current && current === pendingIdRef.current) return current return sessions[sessions.length - 1].session_id }) }, [sessions])Set
pendingIdRef.current = started.session_idinhandleStartnext to line 92.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/page/index.tsx` around lines 40 - 51, Preserve newly started-session selection across the stale sessions list by adding a pending session-id ref alongside the existing selection state. Update handleStart to store started.session_id in that ref, and adjust the sessions effect to retain that pending id until it appears in sessions before falling back to the latest session.computer/ui/src/lib/errors.ts-5-9 (1)
5-9: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
JSON.stringifycan returnundefined, which breaks the declared return type.
JSON.stringifyreturnsundefinedforundefined, a function, or a symbol. It does not throw, so thecatchblock does not run.errorMessagethen returnsundefinedwhile its signature promisesstring, and the page renders an empty error.🐛 Proposed fix
try { - return JSON.stringify(err) + return JSON.stringify(err) ?? String(err) } catch { return String(err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/lib/errors.ts` around lines 5 - 9, Update errorMessage so the JSON.stringify result is validated before returning: when it is undefined, fall back to String(err), while preserving the existing catch fallback for serialization errors. Ensure every path in errorMessage returns a string as declared.computer/ui/styles.css-197-202 (1)
197-202: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep a visible focus indicator when the viewport is not live.
.cp-ui-vpsetsoutline: none, but the replacement indicator applies only to.cp-ui-vp.is-live:focus-visible. If the viewport is focusable without.is-live, keyboard focus becomes invisible. Apply a base:focus-visibleindicator, and keep the accent treatment for the live state.🐛 Proposed fix
+[data-iii-ui="computer"] .cp-ui-vp:focus-visible { + box-shadow: 0 0 0 1px var(--color-ring); +} [data-iii-ui="computer"] .cp-ui-vp.is-live:focus-visible { border-color: var(--color-accent); box-shadow: 0 0 0 1px var(--color-ring); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/styles.css` around lines 197 - 202, Update the `.cp-ui-vp` focus styles in `styles.css` to add a base `:focus-visible` indicator for viewports without `.is-live`, preserving the existing visible outline replacement. Keep the `.cp-ui-vp.is-live:focus-visible` rule as an accent-colored override for live viewports.computer/ui/src/function-trigger-message/ComputerViews.tsx-103-117 (1)
103-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSurface
ok: falsein the action card.
actResultSchemavalidatesok, but the view discards it. A failed action renders the same as a successful one. Reflect the flag in the card.🐛 Proposed fix to show the failure state
return ( <p className="cp-ui-line"> {typeof action === 'string' ? ( - <Badge variant="default" className="cp-ui-pill"> + <Badge + variant={parsed.data.ok ? 'default' : 'alert'} + className="cp-ui-pill" + > {action} </Badge> ) : null} <span className="cp-ui-detail">{parsed.data.detail}</span> </p> )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/function-trigger-message/ComputerViews.tsx` around lines 103 - 117, Update the action card rendering in the component containing the actResultSchema parsing so parsed.data.ok is reflected in the UI. Preserve the existing action badge and detail text, but visibly distinguish failed results where ok is false from successful results.computer/ui/src/page/StartSessionForm.tsx-42-47 (1)
42-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive feedback when the remote endpoint is empty.
In
remotemode, an empty endpoint returns fromsubmitsilently. The button stays enabled, and the user sees no reason for the ignored click. Disable submission while the endpoint is empty.🐛 Proposed fix
+ const canSubmit = mode !== 'remote' || endpoint.trim().length > 0 + const submit = () => {- <Button type="submit" variant="primary" size="sm" disabled={starting}> + <Button + type="submit" + variant="primary" + size="sm" + disabled={starting || !canSubmit} + > {starting ? 'starting...' : 'start session'} </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/page/StartSessionForm.tsx` around lines 42 - 47, Update the remote-mode validation in submit so an empty trimmed endpoint prevents submission and provides user-visible feedback, rather than returning silently. Ensure the start button is disabled whenever remote mode has an empty endpoint, while preserving normal submission for valid endpoints.computer/src/driver/native.rs-338-361 (1)
338-361: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unmapped multi-character key names in
key_for.The fallback uses the first character when no mapping exists, so
"f5"pressesfand"capslock"pressescwhilekeypressreports success. Map the function keys and return an error for multi-character names with no mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/driver/native.rs` around lines 338 - 361, Update key_for to map supported function-key names such as f1–f12 to their corresponding enigo keys, and change the fallback so unmapped multi-character names return an error instead of using the first character; preserve single-character Unicode handling and update callers as needed to propagate the new error.computer/src/driver/mod.rs-79-87 (1)
79-87: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
application/octet-streamreaches the model inside an image block.
detect_mimefalls back toapplication/octet-streamfor unrecognized bytes.Shot::newkeeps that value, and the screenshot handler incomputer/src/functions/mod.rs(lines 265-278) puts it intoContentBlock::image(shot.mime, ...)and intoScreenshotDetails.mime.ScreenshotDetails.mimeis documented incomputer/src/functions/screenshot.rs(line 53) asimage/pngorimage/jpeg. A driver that returns non-image bytes then produces an image content block with a non-image mime, which the harness cannot render.Reject unrecognized bytes at the capture boundary so the failure is explicit.
🛡️ Proposed guard in the screenshot path
pub fn detect_mime(bytes: &[u8]) -> &'static str { if bytes.starts_with(&[0x89, b'P', b'N', b'G']) { "image/png" } else if bytes.starts_with(&[0xFF, 0xD8, 0xFF]) { "image/jpeg" } else { "application/octet-stream" } } + +impl Shot { + /// Reject a capture whose bytes are not a recognized image. + pub fn require_image(self) -> Result<Self, String> { + if self.mime == "application/octet-stream" { + return Err("screenshot: capture is not a png or jpeg image".to_string()); + } + Ok(self) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/driver/mod.rs` around lines 79 - 87, Update detect_mime and the capture flow through Shot::new to reject unrecognized byte signatures instead of returning application/octet-stream. Propagate the explicit failure to the screenshot handler before constructing ContentBlock::image or ScreenshotDetails, while preserving PNG and JPEG detection and their documented MIME values.computer/src/driver/sandbox.rs-220-239 (1)
220-239: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCap the wheel repeat count before sending it to xdotool.
scroll_x/scroll_ycome from theactrequest, andwheeluses them directly asxdotool click --repeat. Large values keep the guest exec busy generating wheel events until the timeout; clamp the count used bywheel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/driver/sandbox.rs` around lines 220 - 239, Cap the repeat count in the wheel method before passing it to xdotool’s --repeat argument. Apply the bound to the count derived from the act request while preserving the existing early return for non-positive values and the current pointer movement behavior.computer/src/driver/native.rs-451-463 (1)
451-463: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
dragignores the requested button.The parameter is bound as
_buttonand the body always pressesButton::Left. TheDrivertrait declaresbutton: &str(computer/src/driver/mod.rs, line 103).RemoteClient::dragforwards the name to the guest, andIiiSandboxHost::dragmaps it throughbutton_number. A right-button drag therefore behaves as a left-button drag on the native driver only, and the call still reports success.Map the name to an
enigo::Button, or return an error for a button the native driver does not support.🐛 Proposed fix to honor the button
+/// X-style button name to an enigo button. +fn button_for(name: &str) -> Button { + match name { + "right" => Button::Right, + "middle" => Button::Middle, + _ => Button::Left, + } +} + - async fn drag(&self, from: (i64, i64), to: (i64, i64), _button: &str) -> Result<(), String> { + async fn drag(&self, from: (i64, i64), to: (i64, i64), button: &str) -> Result<(), String> { + let button = button_for(button); with_enigo(self.geom(), move |e, geom| { let (fx, fy) = to_global(from.0, from.1, geom); let (tx, ty) = to_global(to.0, to.1, geom); e.move_mouse(fx, fy, Coordinate::Abs).map_err(input_err)?; - e.button(Button::Left, Direction::Press) + e.button(button, Direction::Press) .map_err(input_err)?; e.move_mouse(tx, ty, Coordinate::Abs).map_err(input_err)?; - e.button(Button::Left, Direction::Release) + e.button(button, Direction::Release) .map_err(input_err) }) .await }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/driver/native.rs` around lines 451 - 463, Update the native driver’s drag method to use the requested button instead of hardcoding Button::Left: rename and parse the button parameter within drag, map supported names to the corresponding enigo::Button, and return an error for unsupported names before performing the drag.computer/src/functions/frame.rs-17-21 (1)
17-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the doc comment from the field to the struct.
The comment on
session_id(line 19) describes the wholecomputer::screencast::stopfunction's idempotent behavior, not the field itself.schemarsattaches doc comments to the JSON-schema node they annotate, so this text becomes the description of thesession_idparameter instead of the endpoint description. Move it to the struct-level doc comment.📝 Proposed fix
#[derive(Debug, Deserialize, JsonSchema)] +/// Stopping the screencast on an unknown session succeeds. pub struct ScreencastStopInput { - /// Stopping the screencast on an unknown session succeeds. pub session_id: String, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/functions/frame.rs` around lines 17 - 21, Move the “unknown session succeeds” documentation from the session_id field to a doc comment on the ScreencastStopInput struct, leaving the field documented only by its type and preserving the endpoint-level description in the generated schema.computer/src/functions/sessions.rs-26-30 (1)
26-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the
osfallback description.The doc states that omitting
osuses the configuredos.Sessions::startincomputer/src/session.rsLines 336-363 only usescfg.osfor the remote-endpoint path. A sandbox session falls back to the literal"linux", and a native session falls back tostd::env::consts::OS. This description is published in the function schema, so callers see it.📝 Proposed wording
/// Guest OS label recorded on the session and surfaced in - /// `session-started` (`linux`, `macos`, `windows`, `android`). Omit to - /// use the configured `os`. + /// `session-started` (`linux`, `macos`, `windows`, `android`). Omit to + /// use the configured `os` for a remote `endpoint`, `linux` for a sandbox + /// `image`, or this machine's OS for a native session. #[serde(default)] pub os: Option<String>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/functions/sessions.rs` around lines 26 - 30, Update the `os` field documentation in the session start arguments to describe the actual fallback behavior: remote sessions use configured `cfg.os`, sandbox sessions default to `"linux"`, and native sessions default to `std::env::consts::OS`; keep the existing supported labels and omission behavior accurate in the published schema.computer/src/main.rs-119-129 (1)
119-129: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFall back to defaults when the configuration load fails.
fetch_configfailures abort startup here. The repo convention is to logtracing::warn!and continue withWorkerConfig::default(), so the worker still registers instead of exiting.Based on learnings: worker binaries should handle config-load failures by logging `tracing::warn!` and falling back to `WorkerConfig::default()` rather than exiting with an error.♻️ Proposed fallback
- let cfg = configuration::fetch_config(&iii) - .await - .map_err(anyhow::Error::msg) - .context("loading computer configuration")?; + let cfg = match configuration::fetch_config(&iii).await { + Ok(cfg) => cfg, + Err(e) => { + tracing::warn!(error = %e, "failed to load computer configuration; using defaults"); + WorkerConfig::default() + } + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/main.rs` around lines 119 - 129, Update the configuration loading flow in main around configuration::fetch_config to catch failures, log them with tracing::warn!, and continue using WorkerConfig::default() instead of propagating the error. Preserve the existing configuration logging and shared-config initialization for both loaded and fallback configurations.Source: Learnings
computer/src/config.rs-33-41 (1)
33-41: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd configuration bounds for fields narrowed at runtime.
screenshot_qualityis narrowed withas u8, so a configured value outsideu8::MIN..=u8::MAXwraps instead of being rejected; for example,300becomes44. Add#[schemars(range(...))]toWorkerConfigfields where the config value is narrower thanu64, includingscreenshot_qualityandmax_screenshot_dimension, so invalid configuration is rejected before session reconnection/driver startup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/config.rs` around lines 33 - 41, Add schemars range constraints to the narrowed WorkerConfig fields shown here: constrain screenshot_quality to the valid JPEG quality range 1–100 and max_screenshot_dimension to the representable u8 range used at runtime. Ensure validation rejects out-of-range values before session reconnection or driver startup.
🧹 Nitpick comments (12)
computer/ui/src/page/Viewport.tsx (2)
202-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTyping depends on focus, and no code moves focus to the surface.
The hint in
computer/ui/src/page/index.tsxline 187 tells the user to click the desktop to focus it. Click focus on atabIndex={0}div is not reliable across browsers, so the first keystrokes can go elsewhere.Call
focus()on the surface in aonPointerDownhandler.♻️ Proposed change
ref={surfaceRef} role="application" // biome-ignore lint/a11y/noNoninteractiveTabindex: a live desktop surface forwarding raw mouse/keyboard input; focus is how typing reaches the desktop tabIndex={0} aria-label="computer viewport: clicks, scrolling, and typing forward to the desktop" + onPointerDown={() => surfaceRef.current?.focus()} onClick={handleClick}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/page/Viewport.tsx` around lines 202 - 234, Update the surface element in Viewport’s render to add an onPointerDown handler that calls focus() on surfaceRef, ensuring pointer interaction reliably focuses the desktop before keyboard input. Preserve the existing click and other event handlers.
191-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winChord characters come from
e.key, which modifiers can alter.
e.keyreports the produced character, not the physical key. On macOS,Altplus a letter produces a different character, soonPressKeysreceives a key name the user never pressed. Non-US layouts show the same effect forCtrlchords on some keys.For chords, derive the key name from
e.codewhen the code is aKeyXorDigitNvalue, and keepe.keyfor plain text.♻️ Proposed change
if (e.key.length !== 1) return e.preventDefault() e.stopPropagation() // A chord (cmd+c) is a hotkey; a bare character is text. if (modifiers.length > 0) { - onPressKeys([...modifiers, e.key.toLowerCase()]) + const code = /^(Key|Digit)/.test(e.code) + ? e.code.replace(/^(Key|Digit)/, '').toLowerCase() + : e.key.toLowerCase() + onPressKeys([...modifiers, code]) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/page/Viewport.tsx` around lines 191 - 199, Update the chord branch in the keyboard handler around onPressKeys so letter and digit chords derive their key name from e.code when it matches KeyX or DigitN, rather than using the modifier-altered e.key; preserve e.key for plain text input and existing behavior for other chord keys.computer/ui/src/lib/computer.ts (1)
245-250: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate
ok: falsefromactandstopSession.Both wrappers discard the result, so a response with
ok: falseresolves as success. The live viewport then reports nothing when the worker refuses a click, key press, or drag.actResultSchemaandsessionStopSchemaalready model these responses in this module.♻️ Proposed change
export async function act( iii: ExtensionIii, sessionId: string, payload: ActPayload, ): Promise<void> { - await iii.trigger(ACT_FUNCTION_ID, { session_id: sessionId, ...payload }) + const res = await iii.trigger<unknown>(ACT_FUNCTION_ID, { + session_id: sessionId, + ...payload, + }) + const parsed = actResultSchema.safeParse(decodeComputerResult(res)) + if (parsed.success && !parsed.data.ok) { + throw new Error(parsed.data.detail || `${payload.action} failed`) + } }Also applies to: 274-280
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/lib/computer.ts` around lines 245 - 250, Update the act and stopSession wrappers to return the result from iii.trigger instead of discarding it, using the existing actResultSchema and sessionStopSchema response types so ok: false propagates to callers. Preserve the existing function arguments and trigger identifiers.computer/ui/src/lib/events.ts (2)
129-131: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake a failed stream binding observable.
useComputerLifecycleEventsreports failure throughbound, so callers can fall back and show status.useComputerStreamreturnsvoidand swallows the error, so a failed stream binding is indistinguishable from an idle stream. Log a warning here, or return aboundflag as the lifecycle hook does, so the viewport can report that it fell back to polling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/lib/events.ts` around lines 129 - 131, Update useComputerStream so a failed stream binding is observable instead of being silently swallowed: either log a warning in its catch path or return a bound status consistent with useComputerLifecycleEvents. Ensure the viewport can distinguish a polling fallback from an idle stream.
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the callback ref writes into the subscription effects.
onEventRef.current = opts.onEventandonFrameRef.current = opts.onFramerun during render. Move these assignments into the dependency-freeuseEffectthat already registers the async handlers; the async handlers read the latest callback ref safely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/lib/events.ts` around lines 43 - 44, Move the onEventRef.current and onFrameRef.current assignments out of render and into the existing dependency-free useEffect blocks that register the asynchronous handlers. Keep each ref updated before its corresponding handler subscription uses it, while preserving the handlers’ access to the latest callbacks.computer/ui/src/function-trigger-message/ComputerViews.tsx (1)
67-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exporting the list envelope schema once.
lib/computer.tslistSessionsdeclares the same{ sessions: [...] }envelope. Export a singlesessionListSchemafromlib/computer.tsand use it in both places. This keeps the wire contract in one location if the worker adds fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/function-trigger-message/ComputerViews.tsx` around lines 67 - 71, Export a shared sessionListSchema from lib/computer.ts matching the { sessions: z.array(sessionInfoSchema) } envelope currently declared by listSessions. Update SessionListView to import and use sessionListSchema in its safeParse call, and replace the duplicate listSessions schema with the shared export.computer/ui/src/function-trigger-message/index.tsx (2)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport the React type explicitly.
This file uses the
React.ReactNodeglobal namespace but imports nothing fromreact.computer/ui/tsconfig.jsonsets"types": [], so the globalReactnamespace reaches the program only because sibling modules importreact. Import the type directly to make the dependency explicit.♻️ Proposed change
+import type { ReactNode } from 'react' import { type FunctionTriggerMessage,-function renderBody(message: FunctionTriggerMessage): React.ReactNode | null { +function renderBody(message: FunctionTriggerMessage): ReactNode | null {-function renderCall(message: FunctionTriggerMessage): React.ReactNode | null { +function renderCall(message: FunctionTriggerMessage): ReactNode | null {Also applies to: 122-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/function-trigger-message/index.tsx` at line 58, Update the imports in the module containing renderBody so the React namespace type used by renderBody and the additional React.ReactNode usage is explicitly imported from react. Keep the existing React.ReactNode return-type annotations unchanged while adding only the required type dependency.
83-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: render the typed view while a call is still running.
bodyis gated on!running. If a call reportsrunningtogether with a non-nulloutput, the card drops to raw JSON even though a typed view exists for the function id. Dropping the!runninggate keeps the card stable across the running-to-settled transition, because line 107 already handles the "running with no output" case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/function-trigger-message/index.tsx` around lines 83 - 87, Update the body assignment in the function-trigger message component to render renderBody(message) whenever message.output is non-null, regardless of running. Leave the fallback condition and the existing running-without-output handling unchanged so typed output remains stable during the transition.computer/src/driver/sandbox.rs (1)
359-368: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRaise the log level for a failed sandbox stop.
closemaps everysandbox::stopfailure toOk(())and logs atdebug. A stop that fails for a reason other than "already reaped", for example a bus timeout, leaves a live microVM. Theidle_timeout_secsbackstop then reclaims it later. Atdebuglevel the operator sees nothing in a default deployment.Log at
warnso a leaked sandbox is visible.♻️ Proposed change
Err(e) => { - tracing::debug!(sandbox = %self.sandbox_id, error = %e, "sandbox stop on close (treated as gone)"); + tracing::warn!(sandbox = %self.sandbox_id, error = %e, "sandbox stop on close failed; treating as gone (idle timeout will reclaim it)"); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/driver/sandbox.rs` around lines 359 - 368, Update the failed-stop logging in close to use the warn level instead of debug, while preserving the existing idempotent Ok(()) behavior and sandbox_id/error fields.computer/src/configuration.rs (1)
55-61: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider skipping retries for
NOT_FOUND.
try_get_valuetreatsNOT_FOUNDas "no value stored". This is the normal first-boot path.trigger_with_retrystill retries it three times and sleeps 250 ms and 500 ms before returning the error, so every fresh boot pays ~750 ms and logs two warnings. Detect the terminalNOT_FOUNDcase inside the retry loop, or pass a "do not retry" flag forconfiguration::get.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/configuration.rs` around lines 55 - 61, Update the retry handling used by try_get_value for the configuration::get request so NOT_FOUND is treated as terminal and returned immediately without additional retries, delays, or warning logs. Preserve the existing Ok value extraction and Err handling for other failures.computer/src/driver/native.rs (1)
381-387: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
screen_sizeperforms a full capture and JPEG encode.
capture_shotcaptures the display, downscales it, and JPEG-encodes it.screen_sizethen discards the bytes and keeps only the dimensions. On a Retina display this costs a full frame capture plus an encode for everyscreen_sizecall. Session start calls it, andcomputer/src/session.rscalls it again on reconnect.Split the capture so the dimension path skips the encode, or cache the last known target dimensions and return them when the display is already pinned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/driver/native.rs` around lines 381 - 387, Update screen_size to obtain the target dimensions without invoking capture_shot or performing JPEG encoding; split the capture flow to expose a dimension-only path, or reuse cached pinned dimensions when available. Preserve the existing Screen width and height values while ensuring session startup and reconnect calls avoid full-frame capture work.computer/Cargo.toml (1)
22-22: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider upgrading
tokio-tungstenitefrom 0.24.The crate is pinned to
"0.24". The latest published release is 0.29.0, and upstream notes describe versions newer than 0.26.2 as more performant. Since this dependency backs the remote WebSocket driver, an upgrade could improve throughput for screenshot/action round-trips without changing the worker's public contract.Please confirm whether
tokio-tungstenite0.24 → 0.29 introduces breaking API changes that would affectcomputer/src/driver/remote.rsbefore upgrading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/Cargo.toml` at line 22, Review the tokio-tungstenite usage in the remote WebSocket driver, particularly computer/src/driver/remote.rs, for API changes between versions 0.24 and 0.29; if compatible, update the Cargo.toml dependency to 0.29 and adjust any affected code while preserving the worker’s public contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@computer/src/driver/native.rs`:
- Around line 363-377: Replace the `OnceLock` used by `NativeHost::pinned` with
a thread-safe mutable cell that supports replacing the stored `(id, geom)`
value. Update `capture_shot` to write the latest capture result rather than
calling `set` on a one-time cell, and adjust `pinned_id`/`geom` access
accordingly so monitor changes and geometry updates are reflected in subsequent
input mapping.
In `@computer/src/driver/sandbox.rs`:
- Around line 59-71: Add a configurable network-access field to the desktop
sandbox configuration, defaulting to the worker’s documented value, and use it
in the sandbox::create payload instead of hardcoding network: true. Thread the
setting through the existing configuration path to the trigger call on iii,
preserving enabled network access only where required by the sandbox image.
In `@computer/src/session.rs`:
- Around line 310-316: Update the session-start flow around the map lock,
capacity check, and later insertion so it reserves a session slot while the lock
is held before any awaited driver connection or screen-size work. Replace that
reservation with the real session entry after successful setup, and remove the
reservation on every failure path so unsuccessful starts do not consume
capacity; preserve the existing max_sessions error behavior.
- Around line 482-501: Call sessions.restore() in computer/src/main.rs before
functions::register_all so restored sessions are loaded before new requests can
allocate IDs. In restore, make insertion defensive by checking whether
rec.session_id is already present and never replacing an existing live session;
preserve the existing session and handle the restored record as skipped.
In `@computer/ui/src/lib/computer.ts`:
- Around line 300-311: Update readFrame to pass the trigger response through
decodeComputerResult before applying frameSchema.safeParse, preserving
compatibility with both transcript-wrapped and direct bus results. Use the
decoded value for the existing success-or-null return path.
---
Outside diff comments:
In `@computer/images/desktop/Dockerfile`:
- Around line 30-58: Add a dedicated non-root desktop user in the Dockerfile,
configure `/tmp/.X11-unix` with appropriate ownership and world-writable
permissions, and set the image’s `USER` to that account so Xvfb, openbox,
xdotool, ImageMagick, and sandbox::exec run as the guest user. Ensure the user
has the required home/runtime environment while preserving the existing package
installation and ImageMagick policy setup.
---
Minor comments:
In `@computer/architecture/integration.md`:
- Line 7: Limit the session identifier requirement to session-scoped calls:
update the wording at computer/architecture/integration.md lines 7-7 from “Every
call” to “Every session-scoped call”, and update computer/skills/SKILL.md lines
53-56 from “every other function” to the same scoped wording. Preserve the
clarification that global functions such as sessions::list and displays do not
require session_id.
In `@computer/architecture/internals.md`:
- Around line 46-48: Update the restart-recovery documentation in
computer/architecture/internals.md lines 46-48 to describe Sessions::restore
reconnection as best-effort rather than guaranteeing live desktops remain
available. Also update computer/skills/SKILL.md lines 20-23 to state that
restoration can fail and a new session may be required.
In `@computer/images/desktop/README.md`:
- Around line 10-14: Update the README sentence describing headless operation to
say the image is “designed for headless use” instead of claiming it works
headlessly in CI, without changing the surrounding explanation.
In `@computer/README.md`:
- Around line 5-7: Update the computer documentation to distinguish keyboard
inputs from coordinate-based actions: in computer/README.md lines 5-7, remove
the claim that typing uses coordinates; in computer/README.md lines 105-108,
remove “all by coordinate”; and in computer/skills/SKILL.md lines 61-62, state
that coordinates apply only to pointer and scroll actions while keyboard
operations use their text or keys inputs.
- Around line 53-56: Update the native-session documentation to reflect
configured default resolution: in computer/README.md lines 53-56, state that the
empty start request requires both sandbox_image and default_endpoint to be
empty; in computer/skills/SKILL.md lines 12-15, qualify “with no endpoint” by
noting configured defaults are resolved first; and in lines 43-45, apply the
same qualification to the boundary rules.
- Around line 122-124: Update the configuration lifecycle statement near the
computer worker documentation to name only the fields that hot-reload live, such
as the screencast rate, and clarify that timeout changes affect new connections
or session starts rather than existing sessions. Keep the documented
fixed-at-connect behavior for command_timeout_ms and the session-start behavior
for connect_timeout_ms consistent.
In `@computer/src/config.rs`:
- Around line 33-41: Add schemars range constraints to the narrowed WorkerConfig
fields shown here: constrain screenshot_quality to the valid JPEG quality range
1–100 and max_screenshot_dimension to the representable u8 range used at
runtime. Ensure validation rejects out-of-range values before session
reconnection or driver startup.
In `@computer/src/driver/mod.rs`:
- Around line 79-87: Update detect_mime and the capture flow through Shot::new
to reject unrecognized byte signatures instead of returning
application/octet-stream. Propagate the explicit failure to the screenshot
handler before constructing ContentBlock::image or ScreenshotDetails, while
preserving PNG and JPEG detection and their documented MIME values.
In `@computer/src/driver/native.rs`:
- Around line 338-361: Update key_for to map supported function-key names such
as f1–f12 to their corresponding enigo keys, and change the fallback so unmapped
multi-character names return an error instead of using the first character;
preserve single-character Unicode handling and update callers as needed to
propagate the new error.
- Around line 451-463: Update the native driver’s drag method to use the
requested button instead of hardcoding Button::Left: rename and parse the button
parameter within drag, map supported names to the corresponding enigo::Button,
and return an error for unsupported names before performing the drag.
In `@computer/src/driver/sandbox.rs`:
- Around line 220-239: Cap the repeat count in the wheel method before passing
it to xdotool’s --repeat argument. Apply the bound to the count derived from the
act request while preserving the existing early return for non-positive values
and the current pointer movement behavior.
In `@computer/src/functions/frame.rs`:
- Around line 17-21: Move the “unknown session succeeds” documentation from the
session_id field to a doc comment on the ScreencastStopInput struct, leaving the
field documented only by its type and preserving the endpoint-level description
in the generated schema.
In `@computer/src/functions/sessions.rs`:
- Around line 26-30: Update the `os` field documentation in the session start
arguments to describe the actual fallback behavior: remote sessions use
configured `cfg.os`, sandbox sessions default to `"linux"`, and native sessions
default to `std::env::consts::OS`; keep the existing supported labels and
omission behavior accurate in the published schema.
In `@computer/src/main.rs`:
- Around line 119-129: Update the configuration loading flow in main around
configuration::fetch_config to catch failures, log them with tracing::warn!, and
continue using WorkerConfig::default() instead of propagating the error.
Preserve the existing configuration logging and shared-config initialization for
both loaded and fallback configurations.
In `@computer/ui/src/function-trigger-message/ComputerViews.tsx`:
- Around line 103-117: Update the action card rendering in the component
containing the actResultSchema parsing so parsed.data.ok is reflected in the UI.
Preserve the existing action badge and detail text, but visibly distinguish
failed results where ok is false from successful results.
In `@computer/ui/src/lib/errors.ts`:
- Around line 5-9: Update errorMessage so the JSON.stringify result is validated
before returning: when it is undefined, fall back to String(err), while
preserving the existing catch fallback for serialization errors. Ensure every
path in errorMessage returns a string as declared.
In `@computer/ui/src/page/index.tsx`:
- Around line 40-51: Preserve newly started-session selection across the stale
sessions list by adding a pending session-id ref alongside the existing
selection state. Update handleStart to store started.session_id in that ref, and
adjust the sessions effect to retain that pending id until it appears in
sessions before falling back to the latest session.
In `@computer/ui/src/page/SessionRail.tsx`:
- Around line 6-10: Update the SessionRail component’s session rendering to sort
sessions newest-first before mapping them into rows, matching the documented
ordering and the index.tsx convention that the last received session is newest.
Keep the existing session row rendering and selection behavior unchanged.
In `@computer/ui/src/page/StartSessionForm.tsx`:
- Around line 42-47: Update the remote-mode validation in submit so an empty
trimmed endpoint prevents submission and provides user-visible feedback, rather
than returning silently. Ensure the start button is disabled whenever remote
mode has an empty endpoint, while preserving normal submission for valid
endpoints.
In `@computer/ui/src/page/useLiveFrames.ts`:
- Around line 67-81: Update the screencast failure handling in the catch block
of the live-frame hook to always call setError with the original failure reason,
regardless of whether takeScreenshot produces a fallback frame. Keep the
existing fallback frame behavior and cancellation guard unchanged so the error
remains available alongside the static frame.
In `@computer/ui/styles.css`:
- Around line 197-202: Update the `.cp-ui-vp` focus styles in `styles.css` to
add a base `:focus-visible` indicator for viewports without `.is-live`,
preserving the existing visible outline replacement. Keep the
`.cp-ui-vp.is-live:focus-visible` rule as an accent-colored override for live
viewports.
---
Nitpick comments:
In `@computer/Cargo.toml`:
- Line 22: Review the tokio-tungstenite usage in the remote WebSocket driver,
particularly computer/src/driver/remote.rs, for API changes between versions
0.24 and 0.29; if compatible, update the Cargo.toml dependency to 0.29 and
adjust any affected code while preserving the worker’s public contract.
In `@computer/src/configuration.rs`:
- Around line 55-61: Update the retry handling used by try_get_value for the
configuration::get request so NOT_FOUND is treated as terminal and returned
immediately without additional retries, delays, or warning logs. Preserve the
existing Ok value extraction and Err handling for other failures.
In `@computer/src/driver/native.rs`:
- Around line 381-387: Update screen_size to obtain the target dimensions
without invoking capture_shot or performing JPEG encoding; split the capture
flow to expose a dimension-only path, or reuse cached pinned dimensions when
available. Preserve the existing Screen width and height values while ensuring
session startup and reconnect calls avoid full-frame capture work.
In `@computer/src/driver/sandbox.rs`:
- Around line 359-368: Update the failed-stop logging in close to use the warn
level instead of debug, while preserving the existing idempotent Ok(()) behavior
and sandbox_id/error fields.
In `@computer/ui/src/function-trigger-message/ComputerViews.tsx`:
- Around line 67-71: Export a shared sessionListSchema from lib/computer.ts
matching the { sessions: z.array(sessionInfoSchema) } envelope currently
declared by listSessions. Update SessionListView to import and use
sessionListSchema in its safeParse call, and replace the duplicate listSessions
schema with the shared export.
In `@computer/ui/src/function-trigger-message/index.tsx`:
- Line 58: Update the imports in the module containing renderBody so the React
namespace type used by renderBody and the additional React.ReactNode usage is
explicitly imported from react. Keep the existing React.ReactNode return-type
annotations unchanged while adding only the required type dependency.
- Around line 83-87: Update the body assignment in the function-trigger message
component to render renderBody(message) whenever message.output is non-null,
regardless of running. Leave the fallback condition and the existing
running-without-output handling unchanged so typed output remains stable during
the transition.
In `@computer/ui/src/lib/computer.ts`:
- Around line 245-250: Update the act and stopSession wrappers to return the
result from iii.trigger instead of discarding it, using the existing
actResultSchema and sessionStopSchema response types so ok: false propagates to
callers. Preserve the existing function arguments and trigger identifiers.
In `@computer/ui/src/lib/events.ts`:
- Around line 129-131: Update useComputerStream so a failed stream binding is
observable instead of being silently swallowed: either log a warning in its
catch path or return a bound status consistent with useComputerLifecycleEvents.
Ensure the viewport can distinguish a polling fallback from an idle stream.
- Around line 43-44: Move the onEventRef.current and onFrameRef.current
assignments out of render and into the existing dependency-free useEffect blocks
that register the asynchronous handlers. Keep each ref updated before its
corresponding handler subscription uses it, while preserving the handlers’
access to the latest callbacks.
In `@computer/ui/src/page/Viewport.tsx`:
- Around line 202-234: Update the surface element in Viewport’s render to add an
onPointerDown handler that calls focus() on surfaceRef, ensuring pointer
interaction reliably focuses the desktop before keyboard input. Preserve the
existing click and other event handlers.
- Around line 191-199: Update the chord branch in the keyboard handler around
onPressKeys so letter and digit chords derive their key name from e.code when it
matches KeyX or DigitN, rather than using the modifier-altered e.key; preserve
e.key for plain text input and existing behavior for other chord keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…nd sandbox egress Review follow-ups on the port. The native driver pinned display geometry on the first capture and never looked again, so a resolution or scale change mid-session sent clicks to coordinates the model never saw. The display stays pinned; its geometry now refreshes on every capture. Two session-lifecycle holes: the cap was checked before the driver connect awaited, so concurrent starts could all pass it, and restore could displace a session started while it was still running, stranding a live driver. The cap is re-checked under the lock that inserts, and restore leaves a live id alone. Input mapping: `drag` held the left button whatever the caller asked for, and an unmapped key name pressed its first character, so `f5` typed `f` and looked like it worked. Both are errors or honoured now, resolved before any key goes down so a failed chord cannot leave modifiers held. A capture the worker cannot identify as PNG or JPEG no longer reaches the model inside an image block, where it renders as nothing. Sandbox desktops took network access unconditionally; `sandbox_network` makes it an operator choice, on by default. Console: a just-started session keeps the selection until the list catches up, a failed act says so, the remote form needs its endpoint, and the viewport keeps a focus ring.
…restore before serving A start reserves its slot under the map lock, before the driver connect that can boot a whole microVM, and releases it on every failure path. The previous shape only noticed the cap after paying for the desktop it then threw away. Restore now runs before the functions are registered, so a start arriving at boot cannot claim an id a persisted desktop still owns; the defensive check in restore stays as the backstop. Also: f1-f12 are real keys rather than errors, `screen_size` answers from the last capture instead of grabbing and encoding a frame to read two numbers, a failed sandbox stop warns (a leaked VM is worth a line), the config schema carries the ranges the code already clamps to, and the console reads chords off the physical key so alt+c is not 'ç'. Docs say what the code does: coordinates address pointer actions, keyboard actions land on the focused window; a bare start picks the native driver only when nothing is configured; `os` defaults per driver.
…ules No behaviour change. The viewport's keyboard handler grew two subtle rules inline — when shift counts as a modifier, and why a chord reads the physical key. Both are now named functions with the reason next to them. The start form had its "a remote session needs an endpoint" rule written twice, once in submit and once in the button's disabled test; one `ready` value now feeds both, and the mode branch reads as a switch. The act card draws one badge instead of two, the page reads its error banner from a single value, and a capture no longer detours its dimensions through a tuple to store them.
…fter a broken screencast The README, skill, and integration doc all told callers to reach for `shell` when they wanted commands or files on the desktop. That is only true for a native session, where the desktop is this host. A sandboxed desktop takes `sandbox::exec` / `sandbox::fs` and a remote one its own guest executor; `shell` would have run on the worker's machine instead. The manifest also still advertised "run shell", a function this worker does not have. When a capture fails the pump now clears the frame stream and its buffer before stopping, so watchers stop seeing a desktop that is no longer there, and `stop_screencast` waits for the aborted task so a frame in flight cannot land after the delete. A session streaming to a console viewport counts as in use — the idle sweep was stopping desktops out from under people who were watching but not clicking. Restore also says when it drops an unreadable record instead of silently returning fewer sessions. The native capture pins a display id only when the OS reports one, and returns a named struct rather than a five-tuple. Console: viewport coordinates clamp to the last pixel, shift+esc leaves the surface (Tab and Escape belong to the desktop, so keyboard users had no way out), and an act payload can no longer retarget another session.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
computer/src/session.rs (1)
504-550: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore leaves cap-skipped records stranded and reusable for id collision.
At Line 520, once
map.len() >= cfg.max_sessions, the loop breaks. The remaining persisted records are neither reconnected, nor deleted viaforget(), nor advanced past viaadvance_counter_past().After boot,
Sessions::counteronly reflects ids that were actually reconnected (Line 526). A latercomputer::sessions::startcall generates the next id from that counter, which can collide with a skipped record's id (for example, a skippedc3and a freshly mintedc3).persist()then overwrites the skipped record's state entry (Line 618-623) with the new session's data. The original desktop behind the skipped record (potentially a live sandbox VM) loses its only persisted reference and becomes unreachable throughcomputer::sessions::stop, since it was never inserted intomapand its record was never forgotten.The doc comment at Line 504-505 says a record is "dropped" when its driver no longer answers, but this cap-reached path does not drop the record; it leaves it dangling.
Reserve every skipped id against future reuse, even when the record is not reconnected this run.
🔧 Proposed fix: advance the counter past skipped ids
for rec in records { - if (self.map.lock().await.len() as u64) >= cfg.max_sessions { - tracing::warn!("restore: cap reached; leaving remaining records for a later start"); - break; - } + if (self.map.lock().await.len() as u64) >= cfg.max_sessions { + tracing::warn!(session = %rec.session_id, "restore: cap reached; leaving record un-reconnected"); + // Reserve the id so a later `start()` cannot reuse it and + // overwrite this still-persisted (if orphaned) desktop. + self.advance_counter_past(&rec.session_id); + continue; + } match self.reconnect(&rec, &cfg).await {This still leaves the underlying desktop (e.g., a sandbox VM) unreachable until a future restart with enough cap headroom reconnects it, but it stops a fresh session from silently overwriting its state record.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/src/session.rs` around lines 504 - 550, Update the cap-reached branch in Sessions::restore to call advance_counter_past for every persisted record that will be skipped before stopping the loop, ensuring their ids cannot be reused by later start calls. Preserve the existing cap warning and break behavior, and leave records available for a future restart to reconnect.
🧹 Nitpick comments (1)
computer/ui/src/page/Viewport.tsx (1)
40-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit tests for the chord-parsing helpers.
chordModifiersandchordKeyNameare pure functions that decide how every keyboard chord maps onto the desktop. A regression here silently changes what keys reach the desktop.Add unit tests for cases such as bare Shift+letter (types the shifted character), Ctrl+letter (forwards a chord), Shift+F5 (forwards
["shift", "f5"]), and the physical-key extraction forKeyC/DigitNcodes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@computer/ui/src/page/Viewport.tsx` around lines 40 - 59, Add unit tests for the pure helpers chordModifiers and chordKeyName, covering bare Shift+letter, Ctrl+letter, Shift+F5 producing the expected modifier/key mapping, and physical extraction from KeyC and DigitN codes. Keep the tests focused on their existing keyboard-event inputs and expected outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@computer/src/session.rs`:
- Around line 504-550: Update the cap-reached branch in Sessions::restore to
call advance_counter_past for every persisted record that will be skipped before
stopping the loop, ensuring their ids cannot be reused by later start calls.
Preserve the existing cap warning and break behavior, and leave records
available for a future restart to reconnect.
---
Nitpick comments:
In `@computer/ui/src/page/Viewport.tsx`:
- Around line 40-59: Add unit tests for the pure helpers chordModifiers and
chordKeyName, covering bare Shift+letter, Ctrl+letter, Shift+F5 producing the
expected modifier/key mapping, and physical extraction from KeyC and DigitN
codes. Keep the tests focused on their existing keyboard-event inputs and
expected outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4021da4a-4995-4c7e-886c-7318d4bdd97e
📒 Files selected for processing (26)
computer/README.mdcomputer/architecture/integration.mdcomputer/architecture/internals.mdcomputer/images/desktop/README.mdcomputer/skills/SKILL.mdcomputer/src/config.rscomputer/src/driver/native.rscomputer/src/driver/remote.rscomputer/src/driver/sandbox.rscomputer/src/functions/frame.rscomputer/src/functions/mod.rscomputer/src/functions/sessions.rscomputer/src/main.rscomputer/src/manifest.rscomputer/src/session.rscomputer/tests/golden/schemas/computer.screencast.stop.jsoncomputer/tests/golden/schemas/computer.sessions.start.jsoncomputer/ui/src/function-trigger-message/ComputerViews.tsxcomputer/ui/src/lib/computer.tscomputer/ui/src/lib/errors.tscomputer/ui/src/lib/events.tscomputer/ui/src/page/SessionRail.tsxcomputer/ui/src/page/StartSessionForm.tsxcomputer/ui/src/page/Viewport.tsxcomputer/ui/src/page/index.tsxcomputer/ui/styles.css
🚧 Files skipped from review as they are similar to previous changes (19)
- computer/tests/golden/schemas/computer.screencast.stop.json
- computer/ui/src/lib/errors.ts
- computer/tests/golden/schemas/computer.sessions.start.json
- computer/ui/styles.css
- computer/src/main.rs
- computer/src/functions/frame.rs
- computer/architecture/integration.md
- computer/ui/src/page/SessionRail.tsx
- computer/images/desktop/README.md
- computer/ui/src/page/index.tsx
- computer/ui/src/lib/events.ts
- computer/src/manifest.rs
- computer/README.md
- computer/src/driver/remote.rs
- computer/src/functions/mod.rs
- computer/ui/src/lib/computer.ts
- computer/ui/src/function-trigger-message/ComputerViews.tsx
- computer/ui/src/page/StartSessionForm.tsx
- computer/src/config.rs
1 participant
Refs MOT-4001
What
Full-desktop computer use on the bus, the sibling of
browser. Wherebrowserhands an agent a Chromium tab,computerhands it a whole screen:computer::screenshotshows what is there,computer::actclicks, types, scrolls and drags by coordinate. The harness discoverscomputer::*as functions, so a model that can see an image can operate a GUI with nothing in between.Ten functions, golden wire-schema tested:
sessions::start/list/stop,displays,screenshot,observe,act, and the console plumbingscreencast::start/stop/frame. Trigger typescomputer::session-startedandcomputer::session-stopped.Drivers
One
Drivertrait, three ways to reach a desktop, resolved per session (image wins, else endpoint, else local):sandbox::exec/sandbox::fsalone, no socket into the guest. A fixed virtual display means 1:1 coordinates and nothing to grant on the host. The prebaked image lives incomputer/images/desktop.What iii adds
stateand reconnected best-effort on boot, so a restart does not lose live desktops.computer:frames(group = session id) for the console and any other watcher.Console
The worker ships its own injected page (
#/ext/computer) — session rail, screencast-fed viewport, and click / scroll / type forwarding straight into the desktop — plus the renderer for everycomputer::*call in chat and traces. Injected at registration: no console change, no console rebuild.Permission gates
macOS degrades both capabilities silently instead of failing: without Screen Recording a capture returns wallpaper with every window stripped, and without Accessibility synthetic input is dropped while the library reports success. The native driver checks both and fails loud rather than handing back a blank desktop or a click that never landed.
Action-level policy deliberately stays out of the worker:
screencast::*andframeare denied iniii-permissions.yamlas console plumbing, and the desktop surface stays at the needs-approval default, so holding anactfor a human belongs in the approval gate and dispatch policy where the human-facing surface already is.computer/architecture/integration.mdspells out that split.Testing
cargo fmt --check,cargo clippy --all-targets --all-features -D warnings, andcargo test --all-features(34 tests: golden schemas for all ten functions, no untyped schema, plus config, manifest, driver, events, and the embedded console assets) all pass. The UI project builds clean throughtsc --noEmit+ esbuild and lints clean under the console's biome config.Not yet exercised end to end against a live sandbox desktop in CI; that is the next step before the first release.
Summary by CodeRabbit
New Features
Documentation
Security