Sync generic desktop-control backend fixes#25
Conversation
|
Ready for maintainer review. I verified this locally with:
I do not have merge or reviewer-request permissions on |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes legacy CODEX_ environment variables and renames the COSMIC helper binary. It refactors ydotool execution and KDE clipboard interactions to use asynchronous Tokio APIs and direct DBus connections via zbus, incorporating bounded timeouts and dynamic delays. Additionally, it introduces robust validation for window-relative click coordinates and clears cached AT-SPI nodes on extraction failure. The reviewer provided a valuable suggestion to use the bounds of the activated focused window rather than the requested window to avoid stale coordinates if the window moves during activation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@avifenesh the Gemini finding is addressed in Current local verification on the pushed head:
|
avifenesh
left a comment
avifenesh
left a comment
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the desktop-control sync. Most changes look solid (window-relative click verification, async ydotool with bounded waits, KDE clipboard via zbus, primary action-index preservation, env-alias removal with matching README/CHANGELOG/Cargo.toml updates).
One correctness concern in the ydotool type timeout scaling and one in the KDE klipper proxy migration — flagged inline.
| const KDE_CLIPBOARD_RESTORE_MIN_DELAY_MS: u64 = 1_500; | ||
| const KDE_CLIPBOARD_RESTORE_MAX_DELAY_MS: u64 = 5_000; | ||
| const KDE_CLIPBOARD_RESTORE_CHARS_PER_SECOND: u64 = 250; | ||
|
|
There was a problem hiding this comment.
This is an auto review done by revuto.
ydotool_type_timeout does not actually scale up with text length the way the changelog/test claim. div_ceil(20) yields the number of seconds at 20 chars/sec, but then .max(YDOTOOL_TIMEOUT.as_secs()) (10s) floors it. So for any text up to 200 chars the timeout is pinned at 10s, and only past 200 chars does the length term win. That means a 200-char paste that takes ~10s to inject is at the boundary. More importantly the intent ("bounded timeout scaled by text length") is only met above 200 chars — short-to-medium inputs get a flat 10s regardless. The .max() should arguably be applied to the per-char term combined with a base, e.g. base + chars/rate, rather than max(chars/rate, base). Confirm this matches the intended scaling; the test ydotool_type_timeout_scales_with_text_length only checks 5 chars (==10s) and 500 chars (>10s), so the flat region under 200 chars is untested.
There was a problem hiding this comment.
Fixed in a6a1f6b. type_text now uses the fixed ydotool process budget plus a per-character budget: empty input stays at 10s, 1 char is 11s, 200 chars is 20s, and 500 chars is 35s. The test now covers the short and 200-character boundary cases that exposed the flat region.
| .map_err(|error| format!("KDE clipboard setClipboardContents failed: {error}"))?; | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
This is an auto review done by revuto.
kde_clipboard_proxy now talks directly to org.kde.klipper /klipper org.kde.klipper.klipper via zbus, replacing the qdbus6 shell-out. getClipboardContents/setClipboardContents are activatable D-Bus methods, but klipper is only auto-started when the Klipper applet/service is present. The old qdbus6 path produced a clear command_output_error when the service was absent; the new path will surface a zbus proxy/call error. Verify that on a KDE session where klipper is not running the proxy creation or call returns an error (so should_prefer_kde_clipboard_text_backend callers fall through to the ydotool path) rather than hanging until the 3s timeout on every type_text. Per P4, also confirm the ZbusProxy::new itself is bounded — it is awaited without a timeout() wrapper, only the .call() invocations are bounded by KDE_CLIPBOARD_DBUS_TIMEOUT.
There was a problem hiding this comment.
Fixed in a6a1f6b. Proxy creation now goes through the same bounded DBus helper as getClipboardContents and setClipboardContents, so a pending ZbusProxy::new(...) path cannot hang unbounded. If proxy creation or either clipboard method fails before paste injection, that error is classified as pre-input and the caller can fall through to ydotool.
avifenesh
left a comment
avifenesh
left a comment
There was a problem hiding this comment.
Reviewed the follow-up fixes on the current head. The previous timeout-scaling and KDE Klipper proxy concerns are addressed: ydotool_type_timeout now uses a fixed process budget plus per-character budget, and kde_clipboard_proxy is covered by the bounded DBus helper.
Local verification on a6a1f6b:
cargo fmt --all -- --checkcargo test --locked --no-fail-fastcargo clippy --locked --all-targets -- -D warningscargo build --lockednode --check npm/install.jsnode --check npm/bin/computer-use-linux.jsscripts/mcp_safety_check.py --binary target/debug/computer-use-linux
No blocking findings from my pass.
|
@nisavid Thanks a lot! |
2 participants
Purpose
Propagate the generic Linux Computer Use fixes from
nisavid/codex-app-linux's vendored crate into the standaloneagent-sh/computer-use-linuxcrate.This keeps the standalone crate current with the generic desktop-control behavior already carried in
codex-app-linux, without bringing overcodex-app-specific glue or Codex-only runtime names.Behavior Covered
0.ydotoolfallbacks run asynchronously, drain stdout/stderr, and use bounded waits, including a fixed process budget plus a text-length budget fortype_text.computer-use-linux/COMPUTER_USE_LINUX_*naming.Test Coverage
The branch adds or updates targeted coverage for:
ydotool typetimeout scaling across empty, short, boundary, and long textydotooloutput draining before exitYDOTOOL_SOCKEThandlingFiles To Review
Full diff: Files changed.
src/server.rs: input targeting, accessibility cache handling, semantic action dispatch,ydotool, and KDE clipboard behaviorsrc/cosmic_helper.rs,Cargo.toml,README.md: standalone helper naming and runtime override surfaceCHANGELOG.md: unreleased behavior notesVerification
cargo fmt --all -- --checkcargo check --locked --all-targetscargo clippy --locked --all-targets -- -D warningscargo test --locked --no-fail-fastcargo build --lockednode --check npm/install.jsnode --check npm/bin/computer-use-linux.jsscripts/mcp_safety_check.py --binary target/debug/computer-use-linuxRefs nisavid/codex-app-linux#89.