fix(cua-driver)(windows): list empty-/null-title top-level windows (WPF, borderless, custom-chrome) (#2020) - #2021
Conversation
…rycua#2020) list_windows dropped any visible top-level window with an empty caption because both enumeration sources (EnumWindows + UIA) filtered on a non-empty title. WPF (HwndWrapper[...]), borderless and custom-chrome apps were therefore untargetable: get_window_state, click and scroll all resolve windows through list_windows, so they failed with "No window with window_id" / "No windows found for pid" even though debug_window_info could still see the window. Replace the non-empty-title proxy with a shared is_listable_top_level predicate (visible + non-iconic + owner-less + non-DWM-cloaked) used by both enumeration sources so they can't drift apart again. The owner check (GW_OWNER null) is what lets us drop the title gate without admitting noise; the EnumWindows path previously had no owner check at all. The title is now read for display only via a shared window_title helper; empty captions are listed (the tool layer already renders "(no title)"). Add an #[ignore] regression test that creates a real empty-title top-level window and asserts list_windows enumerates it. Cross-checked with cargo check --target x86_64-pc-windows-gnu (lib + tests); not yet run on real Windows hardware.
|
@LaZzyMan is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughWindows top-level window enumeration now includes empty-caption HWNDs when they pass the shared visibility, ownership, and cloaking checks. The Win32 and UIA paths both use shared helpers, a regression test covers an untitled top-level window, and PARITY.md reflects the updated contract. ChangesWindows top-level window listability parity
Sequence Diagram(s)sequenceDiagram
participant list_windows
participant enum_windows_cb
participant is_listable_top_level
participant window_info_from_uia_element
participant window_title
list_windows->>enum_windows_cb: enumerate top-level HWNDs
enum_windows_cb->>is_listable_top_level: filter visible, owner-less, non-cloaked HWNDs
enum_windows_cb->>window_title: read caption for display
window_info_from_uia_element->>is_listable_top_level: gate resolved HWND
window_info_from_uia_element->>window_title: read caption for display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@libs/cua-driver/rust/crates/platform-windows/src/win32/windows.rs`:
- Around line 294-330: The test window teardown in the CreateWindowExW flow is
not panic-safe because the precondition assertions in the window_title and
is_listable_top_level checks can panic before DestroyWindow runs. Add a cleanup
guard around the hwnd lifetime in this test path so the window is always
destroyed even if assert_eq! or assert! fails, and keep the existing
list_windows verification using hwnd as the unique handle reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f29cc920-5271-4879-a5d2-cea21f902451
📒 Files selected for processing (3)
libs/cua-driver/rust/PARITY.mdlibs/cua-driver/rust/crates/platform-windows/src/uia/windows_enum.rslibs/cua-driver/rust/crates/platform-windows/src/win32/windows.rs
…-safe Wrap the test HWND in a RAII guard so DestroyWindow runs even if a precondition assertion panics before the explicit teardown. Addresses CodeRabbit review feedback on trycua#2021.
Ports the fix from upstream trycua/cua#2021 into the vendored driver. list_windows filtered out any top-level window whose title was empty or null, so legitimate targets (splash screens, some Electron/game windows, tool windows) were invisible to the agent and unclickable. Include empty-title windows, using class name / process as a fallback label. (platform-windows crate is not built on macOS; verified by clean upstream apply and covered by upstream + release-workflow Windows CI.)
The vendored copy is actually at cua-driver-rs-v0.6.7 (workspace version and all 0.6.7->0.6.8 delta files confirm it), but .vendored-from had drifted to 0.6.8 during an earlier sync-script trial whose code delta was not kept. Left as-is it would make a future sync diff 0.6.8->newer and silently skip the real 0.6.7->0.6.8 fixes. Correct it back to 0.6.7. Also record the four not-yet-merged upstream PRs we carry as cherry-picks (trycua/cua#2021/#2025/#2035/#2036) in .vendored-patches.md, and have sync-from-upstream.sh point at it so the next sync reconciles them.
…coordinates (QwenLM#5896) * feat(cua-driver): vendor trycua/cua driver with 1000-normalized coordinate support Vendor libs/cua-driver from trycua/cua into packages/cua-driver as the basis for qwen-code's computer-use backend, adding an opt-in relative (1000x1000 normalized) coordinate mode for Qwen-VL clients. - coord_norm.rs: 0-1000 <-> pixel conversion, per-(pid,window_id) size cache, tools/list description rewrite (TDD, 27 tests) - ToolRegistry: normalized field + invoke input/output hooks - protocol.rs: system-instruction coordinate wording switched by mode - serve.rs: daemon list path description rewrite (input_schema aware) - main.rs: CUA_DRIVER_RS_COORDINATE_SPACE env seed Default coordinate_space=pixels => zero behavior change for existing pixel clients. Set CUA_DRIVER_RS_COORDINATE_SPACE=normalized_1000 to enable. Excludes rust/target build output. * feat(cua-driver): make normalized coordinate scale configurable Add CUA_DRIVER_RS_COORDINATE_SCALE (default 1000) so the normalization full-scale can absorb the Qwen 999-vs-1000 cookbook ambiguity without a recompile. norm_to_px/px_to_norm now take an explicit scale; denormalize_args reads the process-wide COORDINATE_SCALE seeded once at startup from env. * ci(cua-driver): add cross-platform release workflow for vendored driver Standalone GitHub Action that builds, signs, and releases the vendored cua-driver under packages/cua-driver. Adapted from upstream trycua/cua cd-rust-cua-driver.yml: - macOS: universal binary (lipo arm64+x86_64), codesigned + notarized into CuaDriver.app using qwen-code's existing secrets (MAC_CSC_LINK cert + App Store Connect API key notarization); Developer ID identity is auto-discovered from the imported cert. - Linux: x86_64 + arm64, built in debian:11 for a glibc 2.31 floor. - Windows: x86_64 + arm64, unsigned (no EV cert, matches upstream). - Release: softprops/action-gh-release on cua-driver-rs-v* tags or manual dispatch, prerelease. Triggered by tag push (cua-driver-rs-v*) or workflow_dispatch. * chore(cua-driver): rebrand vendored driver as qwen-cua-driver Rename the vendored trycua/cua driver so the fork installs and runs independently of any upstream trycua install: - binary cua-driver -> qwen-cua-driver - bundle CuaDriver.app -> QwenCuaDriver.app - bundle id com.trycua.driver -> com.qwencode.cua-driver Updates the cargo/uia manifests, Info.plist, bundle/proxy launch paths, permission/health-report wording, the install/build scripts, and the cross-platform release workflow. * feat(cua-driver): finish relative-coordinate mode — toggle, scale, zoom/move_cursor - CUA_DRIVER_RS_COORDINATE_SPACE is now a 1/0 toggle (via is_env_truthy); default off keeps pixel mode byte-identical to upstream. - Thread CUA_DRIVER_RS_COORDINATE_SCALE through every coordinate surface (was hardcoded 1000): input denormalization already used it; now the rewritten screenshot dims, the tool/param descriptions, and the agent instructions track the configured scale too. - Normalize zoom (window basis) and move_cursor (screen basis) inputs and rewrite their descriptions, alongside click/double_click/right_click/drag. - Fix zoom on downscaled (Retina) windows: apply the get_window_state resize ratio so the crop lands on the region the agent saw. Normalized mode only; pixel-mode zoom unchanged. All coordinate behavior stays gated on the normalized flag, so the default (pixels) path is unchanged from upstream. * chore(cua-driver): add upstream-sync script (git subtree unusable here) `git subtree split --prefix=libs/cua-driver` hangs on a commit deep in trycua/cua's history, so the subtree add/pull workflow isn't usable for the vendored driver (and a pull would re-split + re-hang every time). Add scripts/sync-from-upstream.sh instead: it git-diffs two upstream refs (never walks the full history, so it dodges the hang), reprefixes the libs/cua-driver delta to packages/cua-driver, and `git apply --reject`s it on top of our local changes — conflicts land as *.rej for manual fixup. Record the vendored version in .vendored-from and document the migration + sync method in the design doc. * chore(cua-driver): exclude vendored driver from qwen-code ESLint The vendored packages/cua-driver tree carries upstream JS (e.g. the test-harness Electron app) that doesn't follow qwen-code's lint rules and fails CI. It is not a workspace package (no package.json) and is not qwen-code TypeScript, so add it to eslint.config.js global ignores — alongside packages/desktop/** — the standard treatment for vendored code. * fix(cua-driver): let start_session revive an idle-reaped session Ports the fix from upstream trycua/cua#2035 into the vendored driver. When a session is reaped for idleness, a subsequent start_session with the same id failed instead of resuming it. Revive the ended session in place so the agent can continue rather than getting a hard error. * fix(cua-driver): retry daemon socket writes on EAGAIN Ports the fix from upstream trycua/cua#2036 into the vendored driver. A non-blocking daemon socket can return EAGAIN/EWOULDBLOCK mid-write when the peer's receive buffer is momentarily full. The driver treated that as fatal and dropped the connection. Add a bounded retry/poll loop (mirror of the read-side socket_io helper) so transient back-pressure no longer kills the session; only a real timeout or hard error fails the write. * fix(cua-driver/linux): stop reporting bare "Clicked" for X11 synthetic clicks Ports the fix from upstream trycua/cua#2025 into the vendored driver. On X11, clicks are delivered via XSendEvent synthetic events, which many toolkits (GTK/SDL/Allegro) ignore because send_event is set. The driver still reported a flat success ("Clicked"), masking that nothing happened. Report the synthetic-delivery caveat honestly so the agent can fall back instead of assuming the click landed. (platform-linux crate is not built on macOS; verified by clean upstream apply and covered by upstream + release-workflow Linux CI.) * fix(cua-driver/windows): list empty-/null-title top-level windows Ports the fix from upstream trycua/cua#2021 into the vendored driver. list_windows filtered out any top-level window whose title was empty or null, so legitimate targets (splash screens, some Electron/game windows, tool windows) were invisible to the agent and unclickable. Include empty-title windows, using class name / process as a fallback label. (platform-windows crate is not built on macOS; verified by clean upstream apply and covered by upstream + release-workflow Windows CI.) * chore(cua-driver): track cherry-picked upstream PRs; fix vendored-from The vendored copy is actually at cua-driver-rs-v0.6.7 (workspace version and all 0.6.7->0.6.8 delta files confirm it), but .vendored-from had drifted to 0.6.8 during an earlier sync-script trial whose code delta was not kept. Left as-is it would make a future sync diff 0.6.8->newer and silently skip the real 0.6.7->0.6.8 fixes. Correct it back to 0.6.7. Also record the four not-yet-merged upstream PRs we carry as cherry-picks (trycua/cua#2021/QwenLM#2025/QwenLM#2035/QwenLM#2036) in .vendored-patches.md, and have sync-from-upstream.sh point at it so the next sync reconciles them. * ci(cua-driver): satisfy repo yamllint on the release workflow The vendored-driver release workflow tripped 114 quoted-strings violations under the repo's .yamllint (quote-type: single, required). Single-quote all string scalars to match every other workflow in .github/workflows. While reformatting, the release-notes body also got its paragraph blank lines collapsed and still referenced the old CUA_DRIVER_RS_COORDINATE_SPACE= normalized_1000 value — restore the blank lines and update it to the current 0/1 toggle (default 0 = off; optional CUA_DRIVER_RS_COORDINATE_SCALE=1000). * chore(cua-driver): sync vendored driver to cua-driver-rs-v0.6.8 First real run of scripts/sync-from-upstream.sh: it 3-way-applied the upstream 0.6.7->0.6.8 delta onto our local fork. 10/12 files applied cleanly; the 2 rejects (install.ps1, _install-rust.sh) were already-applied baked-version bumps (0.6.6->0.6.7, our copies were already at 0.6.7), i.e. no real conflict. 0.6.8 brings: Wayland input path (platform-linux), linux health_report + overlay tweaks, a platform-macos build.rs step, and dependency bumps. Version moved to 0.6.8 across the workspace. Verified our work survived the sync untouched: the relative-coordinate shim (coord_norm/protocol) and all four cherry-picked PRs (socket_io/session + linux/windows) are intact — in particular the 0.6.8 edit to platform-linux tools/impl_.rs landed alongside our QwenLM#2025 change with no collision. macOS cargo check + 132 core tests green. (platform-linux/windows + the binary integration test build only on their own runners; upstream CI covers those.) * ci(cua-driver): add a dry_run gate to the release workflow Mirror the desktop-release / release dry-run pattern: a workflow_dispatch dry_run boolean input (default true). The cross-platform build + package jobs always run and upload their artifacts; the GitHub Release job now publishes only on a tag push or an explicit dry_run=false dispatch. Lets us rehearse the whole build/package pipeline (dry_run=true, notarize=false) and inspect the produced artifacts without cutting a release. A branch push (no tag, not a dispatch) likewise builds without releasing.
|
Real, still-open bug (#2020) — main still drops empty-title top-level windows. The shared Blockers before merge: (1) it's conflicting against 0.7.0 — |
Closes #2020.
Problem
On Windows,
list_windowssilently dropped any visible top-level window whosetitle is empty/null. Because
get_window_state,click,scroll, etc. allresolve a pid → windows through
crate::win32::list_windows, such a window wascompletely untargetable by the agent:
debug_window_info {"pid": <pid>}listed the window (e.g. a WPFRuler.Wpf.exe, classHwndWrapper[App.exe;;<guid>], hwnd393542, titlenull).
list_windows {"pid": <pid>}returned an empty array.get_window_state {"window_id": 393542}→"No window with window_id … exists".click/scroll→"No windows found for pid <pid>".WPF, borderless, and custom-chrome apps legitimately ship empty-caption
top-level windows, so this is a hard wall for a whole class of apps.
Root cause
Both enumeration sources hard-filtered on a non-empty title before the window
could reach the merged list:
EnumWindowspath —enum_windows_cb,win32/windows.rs(title_len == 0 → skip).window_info_from_uia_element,uia/windows_enum.rs(
title_len == 0 → None).The module doc-comment even stated it as the contract: "Both sources apply the
same filters (visible, non-iconic, non-empty title)." This non-empty-title gate
was introduced as "filter parity" in #1542 (whose actual goal was the opposite —
UWP/WebView2 frames returning an empty array), and was never reconsidered for
honest untitled windows. Meanwhile
debug_window_info(#1597) enumerated with alooser filter (visible + no owner, title ignored), which is why the window showed
up there but nowhere else.
Fix
Replace the title proxy with an explicit predicate for "real, targetable
top-level window", shared by both enumeration sources so they can't drift
again (the drift was the root cause):
GW_OWNERnull → excludes tool-tips / owned pop-ups / transient children(the EnumWindows path previously had no owner check — this is what lets us
safely drop the title gate without admitting noise). Mirrors the top-level
test
debug_window_infoalready uses, so the two tools now agree.DWMWA_CLOAKED == 0→ excludes suspended-UWP /ApplicationFrameHostbackground frames that keep
WS_VISIBLEbut aren't on screen (the noise classfeat(list_windows): UIA-first top-level window enumeration on Windows #1542 cared about).
window_titlehelper; emptycaption → empty string. The tool layer already renders
(no title)for theserecords, so nothing downstream needed to change.
Touches:
win32/windows.rs(predicate + helpers +enum_windows_cb),uia/windows_enum.rs(reuse the predicate, drop the title gate),PARITY.md(enumeration-source contract).Tests
Added
empty_title_top_level_window_is_listedinwin32/windows.rs— creates areal visible, owner-less, empty-caption top-level window via
CreateWindowExWand asserts it appears in
list_windows(Some(own_pid))with an empty title. It's#[ignore](needs an interactive window station), matching the existing WindowsGUI tests; run via the sandbox harness runner or
cargo test -p platform-windows -- --ignored empty_title.Verification status
cargo check -p platform-windows --target x86_64-pc-windows-gnu— clean (lib).cargo check … --tests— clean, including the newCreateWindowExW-basedtest module (the part I was least sure about, since it exercises Win32 API
signatures directly). The only warning is a pre-existing unused import in
impl_.rs, unrelated to this change.overlay.rsforCreateWindowExW/RegisterClassExW,impl_.rsforGetWindow(GW_OWNER).unwrap_or_default().is_invalid(),get_window_boundsfor the
DwmGetWindowAttributeshape).checkproves it compiles for theWindows target, but I have no rig / interactive desktop to execute the new
#[ignore]test or run a real-desktop regression. The owner/cloaked gates'effect on real noise windows still needs a hardware run — ideally the same
folks helping with Help wanted: validate multi-monitor (negative-X origin) fix on real Windows hardware — we have no local rig #1981.
Happy to adjust the predicate (e.g. add a non-zero-area check, or keep/drop the
cloaked gate) based on what a real run shows.
Summary by CodeRabbit