feat(cua-driver): vendor qwen-cua-driver with opt-in 0–1000 relative coordinates - #5896
Merged
Conversation
qwen-code-ci-bot
requested changes
Jun 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the PR! However, the PR body doesn't follow the pull request template. Several required sections are missing:
- What this PR does — describe the change in prose
- Why it's needed — motivation / problem being solved
- Reviewer Test Plan — how a reviewer can confirm this, with Before/After evidence and a Tested-on table
- Risk & Scope — main risk, what's out of scope, breaking changes
- Linked Issues — related issues
- 中文说明 — Chinese translation in a
<details>block
At 526 files / 145K+ additions, this is a very large PR — a clear Reviewer Test Plan is especially important so maintainers can verify the vendored driver, the coordinate-space toggle, and the release workflow without guessing.
Could you update the body to match the template? That will unblock the review.
中文说明
感谢贡献！不过 PR 正文没有按照 PR 模板 填写，缺少以下必填部分：
- What this PR does — 用文字描述改动内容
- Why it's needed — 动机 / 要解决的问题
- Reviewer Test Plan — 审核者如何验证，包括 Before/After 证据和测试平台表
- Risk & Scope — 主要风险、不在范围内的事项、破坏性变更
- Linked Issues — 关联的 issue
- 中文说明 — 放在
<details>里的中文翻译
这个 PR 涉及 526 个文件 / 14 万+ 行新增代码，体量很大——清晰的 Reviewer Test Plan 对审核者验证 vendor 驱动、坐标空间切换和发布流程尤其重要。
请按模板更新正文，之后我们继续审核。
— Qwen Code · qwen3.7-max
…inate 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.
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.
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.
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.
…om/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.
LaZzyMan
force-pushed
the
lazzy/cua-driver-relative-coords
branch
from
June 26, 2026 09:16
7d9b8f5 to
ed543e7
Compare
qwen-code-ci-bot
previously requested changes
Jun 26, 2026
| package_dir = Path(__file__).parent | ||
|
|
||
| if sys.platform == "win32": | ||
| binary_name = "cua-driver.exe" |
Collaborator
There was a problem hiding this comment.
[Critical] Binary name mismatch: the Python wrapper looks for cua-driver (line 23) and cua-driver.exe (line 24), but Cargo.toml declares the binary as qwen-cua-driver. The Python package will fail to find the bundled binary at runtime.
Suggested change
| binary_name = "cua-driver.exe" | |
| binary_name = "qwen-cua-driver.exe" |
Also update line 25 for the non-Windows case to qwen-cua-driver, and update build_wheel.py line 82 (binary_names = ["cua-driver"] → ["qwen-cua-driver"]).
— qwen3.7-max via Qwen Code /review
| - **Windows**: unsigned (x86_64 + arm64) | ||
|
|
||
| Enable relative coordinates: | ||
| `CUA_DRIVER_RS_COORDINATE_SPACE=normalized_1000` |
Collaborator
There was a problem hiding this comment.
[Critical] Release notes suggest CUA_DRIVER_RS_COORDINATE_SPACE=normalized_1000, but is_env_truthy() in bundle.rs only accepts 1, true, yes, on (case-insensitive). The documented opt-in value normalized_1000 is not in this set and would be treated as falsy, leaving the feature silently disabled.
Change to CUA_DRIVER_RS_COORDINATE_SPACE=1 to match the code's actual accepted values.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| /// Spawn the HTTP MCP listener bound to `127.0.0.1:port` (loopback only). | ||
| pub fn spawn(registry: Arc<ToolRegistry>, port: u16) { |
Collaborator
There was a problem hiding this comment.
[Critical] The HTTP MCP server binds to loopback with no authentication or authorization. Any local process — including compromised npm/pip packages, malicious browser extensions, or rogue Electron apps — can send arbitrary MCP tool calls (click, type_text, page JS execution, launch/kill apps, screenshot capture) to the daemon.
Consider adding a shared-secret token (generated at daemon startup, written to ~/.cua-driver/.http_token with 0600 permissions) required via Authorization: Bearer header. This is the pattern used by Jupyter, Ollama, and other local-first tools.
— qwen3.7-max via Qwen Code /review
| with tarfile.open(archive_path, "r:gz") as tf: | ||
| for binary_name in binary_names: | ||
| # The -binary tarballs have the binary at the root | ||
| tf.extract(binary_name, dest_dir) |
Collaborator
There was a problem hiding this comment.
[Critical] tarfile.extract() is called without path validation. Python's tarfile module does not prevent path traversal (../) by default (CVE-2007-4559). A malicious or compromised release archive could write arbitrary files outside dest_dir on the build machine.
Use tarfile.extractall with filter="data" (Python 3.12+) or validate each member's resolved path:
for member in tf.getmembers():
resolved = (dest_dir / member.name).resolve()
if not str(resolved).startswith(str(dest_dir.resolve())):
raise ValueError(f"Path traversal detected: {member.name}")
tf.extract(member, dest_dir)— qwen3.7-max via Qwen Code /review
| { | ||
| // Match the canonical curl-piped-to-bash invocation. install.sh | ||
| // delegates to the Rust implementation by default. | ||
| let bash_cmd = format!("curl -fsSL {CANONICAL_INSTALL_SH} | bash"); |
Collaborator
There was a problem hiding this comment.
[Critical] Self-update runs curl -fsSL <url> | bash with no integrity verification of the downloaded script. The URL at line 31 hardcodes trycua/cua (upstream), not QwenLM/qwen-code. A compromise of the upstream repo or a DNS hijack results in arbitrary code execution on every machine that runs cua-driver update --apply.
Additionally, all runtime URLs in the codebase (version_check.rs:69, updater.rs:31-34, build_wheel.py:108, skills.rs:469) reference trycua/cua instead of the vendored location. These should be parameterized or updated to point to QwenLM/qwen-code.
Consider: (a) pinning the install script URL to a specific commit SHA, (b) downloading the script and verifying a SHA256 checksum before execution, or (c) shipping pre-built binaries with integrity verification instead of piping to bash.
— qwen3.7-max via Qwen Code /review
| /// headers until CRLFCRLF, then `Content-Length` bytes. | ||
| async fn read_http_request(stream: &mut TcpStream) -> anyhow::Result<Option<HttpRequest>> { | ||
| let mut head = Vec::with_capacity(1024); | ||
| let mut byte = [0u8; 1]; |
Collaborator
There was a problem hiding this comment.
[Suggestion] HTTP headers are read one byte at a time (let mut byte = [0u8; 1]) in a loop with no per-read timeout. A stalled or slow client can hold the connection (and its tokio task) open indefinitely. The only bound is the 64KB header size limit.
Add a per-read timeout, e.g.:
let n = match tokio::time::timeout(Duration::from_secs(30), stream.read(&mut byte)).await {
Ok(Ok(n)) => n,
Ok(Err(e)) => return Err(e.into()),
Err(_) => anyhow::bail!("HTTP read timeout: client stalled"),
};Mitigated by loopback-only binding, but defense-in-depth is warranted for a long-running daemon.
— qwen3.7-max via Qwen Code /review
| /// (network, PostHog outage, timeout) leaves the marker absent so the | ||
| /// next launch retries — without this, a single bad network at install | ||
| /// time would silently drop the only adoption signal we have. | ||
| pub fn capture_install() { |
Collaborator
There was a problem hiding this comment.
[Suggestion] capture_install() intentionally bypasses the telemetry opt-out (CUA_DRIVER_RS_TELEMETRY_ENABLED=false). The code comment acknowledges this: "this is the only path that does so." A user who explicitly disables telemetry will still have their install event sent to PostHog.
This may violate GDPR expectations in jurisdictions with privacy regulations. Consider respecting the opt-out for capture_install(), or providing a separate, clearly documented flag for install-only telemetry.
— qwen3.7-max via Qwen Code /review
|
|
||
| [project] | ||
| name = "cua-driver" | ||
| version = "0.5.1" |
Collaborator
There was a problem hiding this comment.
[Critical] Python package version is 0.5.1 while the Rust workspace (Cargo.toml) is at 0.6.7. Additionally, build_wheel.py defaults to --version 0.5.1 and downloads binaries from trycua/cua releases (line 108) rather than QwenLM/qwen-code. The Python wrapper and the Rust binary it wraps will silently diverge in versioning.
Align versions and update build_wheel.py to fetch from the correct vendored repository.
— qwen3.7-max via Qwen Code /review
|
|
||
| def get_expected_sha256(version: str, archive_name: str) -> str: | ||
| """Fetch and parse checksums.txt from GitHub release.""" | ||
| checksums_url = f"https://github.com/trycua/cua/releases/download/cua-driver-rs-v{version}/checksums.txt" |
Collaborator
There was a problem hiding this comment.
[Critical] build_wheel.py fetches checksums.txt from trycua/cua releases (this URL), but the CD workflow (cd-cua-driver.yml) never generates or uploads a checksums.txt file to QwenLM/qwen-code releases. Python wheel builds will always fail at the checksum verification step.
Two fixes needed:
- Update this URL to point to
QwenLM/qwen-codereleases. - Add a step in the CD workflow to generate and upload
checksums.txtwith SHA256 hashes for all release assets.
— qwen3.7-max via Qwen Code /review
| - name: Determine version | ||
| id: version | ||
| shell: bash | ||
| run: | |
Collaborator
There was a problem hiding this comment.
[Critical] Missing LICENSE.md: the workflow references cp ../LICENSE.md "release/${STAGE}/LICENSE" in multiple packaging steps, but packages/cua-driver/LICENSE.md does not exist. The 2>/dev/null || true suppression means the failure is silent, and release tarballs ship without a LICENSE file.
Add packages/cua-driver/LICENSE.md (or copy from the repo-root LICENSE), and add a checksums.txt generation step to the release job so build_wheel.py can verify downloaded binaries.
— qwen3.7-max via Qwen Code /review
`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.
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.
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.
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.
…c 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.)
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.
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).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Summary
PR: #5896 — Vendor trycua/cua as qwen-cua-driver with opt-in relative coordinates
Reviewed SHA: 1c2d8ef
Review mode: Parallel multi-agent (9 agents: correctness, security, code quality, performance, test coverage, 3× undirected audit, build verification)
This PR vendors the trycua/cua background-automation driver and adds an opt-in 0–1000 normalized coordinate mode. The vendoring approach (direct copy, not subtree) is pragmatic given the upstream subtree-split hang, and the coordinate normalization design in coord_norm.rs is well-thought-out. However, several integration gaps need attention before merge.
Critical Findings (4 new, not in existing bot comments)
-
Recording-replay double-denormalization —
coord_norm.rs/tool.rs: Normalized coordinates are converted to pixels before recording, but replay re-invokesinvoke()which converts again. Every recorded trajectory in normalized mode replays with corrupted coordinates. Fix: record original pre-denormalization args. -
Pervasive stale upstream URLs —
version_check.rs,updater.rs,skills.rs,install.ps1, and ~15 other files still referencetrycua/cuawithlibs/cua-driver/paths. The macOSinstall.shwas correctly updated, but Windows install is completely broken (wrong repo, wrong binary name), and the compiled binary's self-update/version-check/skill-install all operate against the upstream repo. -
sync-from-upstream.shwrites.vendored-fromunconditionally — Even whengit apply --rejectfails with.rejfiles, the script advances the vendored ref. The next sync will skip the failed hunks permanently. -
UDS socket + Windows pipe permissions — The daemon's Unix domain socket is created without restrictive permissions (any local user can connect), and the Windows named pipe SDDL grants Generic All to Everyone. Both expose full desktop-control capabilities to any local process.
Suggestion Findings (4 new)
coord_norm.rs:87: Missingscreenshot_h == 0guard (asymmetric withpx_to_normwhich guardsdim == 0)- CD workflow: No
cargo testgate, no concurrency block serve.rs: Six callsites silently swallowstop_owner()errors (recording loss with no forensic trail)- Design doc in Chinese with no English summary (highest-impact documentation debt)
Excluded from this review
- 13 existing bot comments (binary name mismatch, env var value, HTTP auth, tarfile traversal, self-update security, missing coord tools, screenshot cache gap, accept() crash false positive, HTTP header timeout, telemetry bypass, version mismatch, wrong download URL, missing LICENSE.md) — all acknowledged
serve.rsaccept-loop error handling — bot comment is a false positive (code usesmatchwithwarn!+ 100ms sleep + continue, not?operator)
Overall Assessment
The coordinate normalization core (coord_norm.rs) is solid with good test coverage. The main risks are (a) the systematic failure to update upstream URLs across the vendored codebase, (b) the recording-replay data corruption in normalized mode, and (c) the socket/pipe security exposure inherited from upstream. The .vendored-from sync bug is a subtle landmine for future maintenance.
| None => continue, // no screen size cached yet → leave field as-is | ||
| } | ||
| } else if screenshot_w == 0 { | ||
| continue; // no window basis available → leave field as-is |
Collaborator
There was a problem hiding this comment.
[review] Missing screenshot_h == 0 guard (Suggestion)
The guard only checks screenshot_w == 0 to skip conversion, but does not check screenshot_h == 0. If a screenshot has nonzero width but zero height (minimized window, malformed result), the code falls through. Then norm_to_px(v, 0, scale) returns 0.0 for every y-axis field, collapsing all y-coordinates to the top edge.
Note: the inverse function px_to_norm (line 35) correctly guards dim == 0, but this path is asymmetric.
Suggested fix:
} else if screenshot_w == 0 || screenshot_h == 0 {
continue;Severity: Suggestion | Source: Correctness review
| status=1 | ||
| fi | ||
|
|
||
| echo "$NEW_REF" > "$VENDORED_FILE" |
Collaborator
There was a problem hiding this comment.
[review] .vendored-from written unconditionally — silent data loss on next sync (Critical)
Line 90 writes $NEW_REF to $VENDORED_FILE regardless of whether git apply --reject succeeded (status=0) or failed (status=1). When hunks fail and .rej files are left behind, the script still records the new ref.
The next sync uses this (incorrectly advanced) ref as OLD_REF, so the diff it generates skips the failed hunks permanently.
Suggested fix:
if [ "$status" -eq 0 ]; then
echo "$NEW_REF" > "$VENDORED_FILE"
else
echo "warning: .vendored-from NOT updated" >&2
fiSeverity: Critical | Source: Maintainer-audit review
| // Remove stale socket file (from a crashed previous daemon). | ||
| let _ = std::fs::remove_file(socket_path); | ||
|
|
||
| let listener = UnixListener::bind(socket_path) |
Collaborator
There was a problem hiding this comment.
[review] Unix domain socket created without restrictive permissions (Critical)
The daemon binds a Unix domain socket via UnixListener::bind() without calling set_permissions on the socket or parent directory. On systems with umask 022, the socket is world-readable and world-writable.
The protocol has zero authentication. Any local user can connect and invoke the full MCP tool suite: capture screenshots, synthesize clicks/keystrokes, launch apps, write recordings to arbitrary paths.
Suggested fix:
#[cfg(unix)]
std::fs::set_permissions(socket_path,
std::os::unix::fs::PermissionsExt::from_mode(0o600))?;Severity: Critical | Source: Security review
| security_descriptor_size: *mut u32, | ||
| ) -> i32; | ||
| } | ||
| let sddl: Vec<u16> = "D:(A;OICI;GA;;;WD)S:(ML;;NW;;;LW)\0" |
Collaborator
There was a problem hiding this comment.
[review] Windows named pipe grants Generic All to Everyone (Critical)
The SDDL D:(A;OICI;GA;;;WD)S:(ML;;NW;;;LW) grants Generic All to World (Everyone). The Low-integrity label means even sandboxed processes can connect.
Any process on the machine can connect and invoke arbitrary computer-control tools.
Suggested fix: Replace WD with CO (Creator Owner):
let sddl: Vec<u16> = "D:(A;OICI;GA;;;CO)S:(ML;;NW;;;LW)\0"
.encode_utf16().collect();Severity: Critical | Source: Security review
| /// plenty of headroom past the most recent stable release even when | ||
| /// pre-releases are sprinkled in between. | ||
| const RELEASES_URL: &str = | ||
| "https://api.github.com/repos/trycua/cua/releases?per_page=40"; |
Collaborator
There was a problem hiding this comment.
[review] Pervasive stale upstream URLs — systematic trycua/cua → QwenLM/qwen-code gap (Critical)
RELEASES_URL hardcodes trycua/cua. This is part of a broader pattern across many files. All point at trycua/cua with libs/cua-driver/ paths instead of QwenLM/qwen-code with packages/cua-driver/:
| File | Impact |
|---|---|
version_check.rs (69, 153, 157, 228, 356) |
Version check + install one-liners upstream |
updater.rs (31-34) |
update --apply downloads upstream scripts |
skills.rs (469, 485) |
Skill packs from upstream |
install.ps1 (100, 592, 1290) |
Windows install broken (wrong repo + binary name) |
swift/VersionCheck.swift (6) |
Swift backend version check |
install.sh (484), _install-rust.sh (843) |
Stale docs URLs |
post-install-hints.txt (60) |
User-facing docs link |
PARITY.md (~80 refs) |
Stale libs/cua-driver/ paths |
build-app.sh (14-37) |
cua-driver vs qwen-cua-driver daemon name |
macOS install.sh was correctly updated (REPO="QwenLM/qwen-code"), but Windows installer, all Rust URLs, and docs URLs still point upstream. Windows install is completely broken.
Fix: Bulk-update all references:
grep -rn 'trycua/cua' packages/cua-driver/
grep -rn 'libs/cua-driver/' packages/cua-driver/Severity: Critical | Source: Code quality + Maintainer review
| @@ -0,0 +1,353 @@ | |||
| name: "CD: cua-driver (relative-coordinate fork)" | |||
Collaborator
There was a problem hiding this comment.
[review] CD workflow: no test gate and no concurrency block (Suggestion)
1. No test gate: cargo build runs on all platforms but cargo test never runs. The 24+ unit tests in coord_norm.rs are never executed in CI/CD. Broken coordinate conversion could ship undetected.
Add before each build:
- name: Test
working-directory: packages/cua-driver/rust
run: cargo test --workspace2. No concurrency block: Other release workflows in this repo use concurrency with cancel-in-progress: false.
concurrency:
group: 'cd-cua-driver-${{ github.ref }}'
cancel-in-progress: falseSeverity: Suggestion | Source: Test coverage + Code quality
| let recording = recording.clone(); | ||
| let sid = sid.to_owned(); | ||
| std::thread::spawn(move || { | ||
| let _ = recording.stop_owner(Some(&sid)); |
Collaborator
There was a problem hiding this comment.
[review] Recording finalization errors silently swallowed at 6 callsites (Suggestion)
let _ = recording.stop_owner(Some(&sid)); discards the anyhow::Result. Repeats at lines 133, 773, 819, 1281, 1318.
If recording finalization fails (disk full, I/O error), the user believes the recording saved successfully. The mp4 is corrupted or missing, discovered only later. Silent recording loss with no forensic trail.
Suggested fix: Log at minimum:
if let Err(e) = recording.stop_owner(Some(&sid)) {
tracing::error!(session_id = %sid, error = %e,
"recording finalization failed");
}Severity: Suggestion | Source: Oncall-audit review
| @@ -0,0 +1,265 @@ | |||
| # cua-driver 相对坐标（1000×1000 归一化）改造设计 | |||
Collaborator
There was a problem hiding this comment.
[review] Design doc in Chinese — add English executive summary (Suggestion)
This 265-line doc contains critical architectural reasoning (the invoke intercept point, size-cache, move_cursor screen-basis vs click window-basis, from_zoom exclusion, and the Section 0 finding that the qwen failure was unrelated to coordinates).
The codebase is in English. Future maintainers won't understand why key decisions were made.
Suggested fix: Add an English executive summary at the top covering key decisions.
Severity: Suggestion | Source: Maintainer-audit review
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 #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.)
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Summary
Review scope: 528 files, +145,942 lines. Vendoring trycua/cua into packages/cua-driver with an opt-in 0–1000 normalized coordinate shim for Qwen-VL computer_use.
Pre-existing bot comments (21) — not duplicated
The CI bot already flagged binary name mismatch in wrapper.py, is_env_truthy gap, HTTP MCP no auth, tarfile.extract() traversal, curl|bash no integrity check, missing mouse_drag/mouse_button_down, screenshot cache gaps, listener.accept() error handling, telemetry opt-out bypass, Python version mismatch, missing checksums/LICENSE, .vendored-from unconditional write, Unix socket permissions, Windows pipe SDDL, stale upstream URLs, CD workflow gaps, recording finalization swallowing, and design doc language.
New findings from deep review (9 verified)
| # | Severity | Finding |
|---|---|---|
| 1 | Critical | replay_trajectory double-converts coordinates when normalized mode is enabled — recordings capture pixel-space args, but replay applies denormalize_args again |
| 2 | High | build_wheel.py hardcodes trycua/cua release URLs — should point to QwenLM/qwen-code |
| 3 | High | build_wheel.py expects binary name cua-driver but Cargo.toml produces qwen-cua-driver |
| 4 | Medium | PowerShell single-quote injection in spawn_uia_worker — executable path interpolated without escaping |
| 5 | Medium | start_recording accepts arbitrary output_dir with no path validation |
| 6 | Low | denormalize_args() acquires a mutex on every tool call, even coordinate-free tools |
| 7 | Low | build_wheel.py has zero test coverage for security-critical build logic |
| 8 | Low | MCP protocol integration tests are macOS-only despite Linux support |
| 9 | Info | Silent passthrough when screenshot-size cache is empty on cold start — no diagnostic logging |
Reviewed by qwen-code (qwen3.7-max) · 9 parallel review agents · findings verified against source
| else { None } | ||
| }) | ||
| .unwrap_or(""); | ||
| self.recording.record(name, &args, result_text, start_ms); |
Collaborator
There was a problem hiding this comment.
[Critical] replay_trajectory double-converts coordinates in normalized mode.
The recording at this line captures args AFTER denormalize_args has already converted them from 0–1000 to pixel space. When replay_trajectory reads these turn files and calls registry.invoke() (which routes through CoordNormRegistry), denormalize_args is applied a second time on already-pixel-valued args — producing garbage coordinates.
Repro: enable CUA_DRIVER_RS_COORDINATE_SPACE=normalized_1000, record a click at (500, 500) on an 800×600 window (recorded as ~400, 300 in pixels), then replay — the replay will convert 400→400*(800/1000)=320 instead of the original 400.
Fix: either (a) store the pre-denormalization args in the recording, or (b) detect in replay_trajectory that normalized mode is active and skip the second denormalize_args pass (e.g., by setting a flag that CoordNormRegistry::invoke checks).
| Returns: | ||
| Tuple of (download_url, list_of_binary_names_in_archive) | ||
| """ | ||
| base_url = f"https://github.com/trycua/cua/releases/download/cua-driver-rs-v{version}" |
Collaborator
There was a problem hiding this comment.
[High] Hardcoded trycua/cua release URL.
This points to the upstream trycua/cua repo, not the vendored QwenLM/qwen-code. The CD workflow will try to download binaries from a repo that doesn't host this fork's releases.
Same issue on line 108 (checksums URL). These should be configurable or point to QwenLM/qwen-code.
| base_url = f"https://github.com/trycua/cua/releases/download/cua-driver-rs-v{version}" | ||
|
|
||
| if platform_name == "darwin": | ||
| # Universal binary tarball |
Collaborator
There was a problem hiding this comment.
[High] Binary name mismatch with Cargo.toml.
binary_names = ["cua-driver"] expects a binary called cua-driver, but the Cargo workspace declares the binary as qwen-cua-driver (per Cargo.toml [[bin]] section). The wrapper.py Python package also references cua-driver at line 23.
This will cause the wheel build to fail — the binary won't be found in the release archive. The rename from upstream cua-driver → qwen-cua-driver wasn't propagated to the Python packaging layer.
| } | ||
| let uia_str = uia.display().to_string(); | ||
| let cmd = format!( | ||
| "(New-Object -ComObject Shell.Application).ShellExecute('{uia_str}','','','',0)" |
Collaborator
There was a problem hiding this comment.
[Medium] PowerShell single-quote injection.
uia_str (derived from current_exe().display()) is interpolated into a PowerShell command using single quotes without escaping. A single-quote character in the executable path would break out of the string literal:
let cmd = format!(
"(New-Object -ComObject Shell.Application).ShellExecute('{uia_str}','','','',0)"
);While the risk is low (the path comes from the running binary itself), this is a defense-in-depth concern. Consider using -EncodedCommand with a Base64-encoded command, or escaping single quotes by doubling them (' → '' in PowerShell string literals).
| // (session_end) only stops the recording its own session started. | ||
| let owner = args.opt_str("_session_id"); | ||
|
|
||
| match self.session.start(output_dir.as_deref().unwrap(), record_video, owner.as_deref()) { |
Collaborator
There was a problem hiding this comment.
[Medium] Arbitrary filesystem write via output_dir with no path validation.
output_dir from the client's tool call args is passed directly to session.start() without any validation — no canonicalization, no sandboxing, no check that the path is within an expected directory. A malicious client can request recordings be written to any writable path on the filesystem.
Consider validating that output_dir resolves to a path within an allowed base directory (e.g., ~/.cua-driver/recordings/), similar to how tarfile.extract() path traversal was already flagged elsewhere.
| } | ||
| let scale = coordinate_scale(); | ||
| let screen = screen_size(); | ||
| for &(field, is_x, screen_basis) in input_coord_fields(tool) { |
Collaborator
There was a problem hiding this comment.
[Low] screen_size() acquires a mutex on every denormalize_args call.
let screen = screen_size() takes a Mutex lock even for tools that only use window-basis coordinates (click, drag, etc.) where screen_basis is false. On hot paths with frequent tool calls, this is unnecessary contention.
Consider either (a) passing the screen size from the caller (who can cache it once per request), or (b) using an AtomicU64 (packing w/h into a u64) instead of a Mutex<Option<(u32, u32)>> for lock-free reads.
| @@ -0,0 +1,318 @@ | |||
| #!/usr/bin/env python3 | |||
Collaborator
There was a problem hiding this comment.
[Low] Zero test coverage for this security-critical build script.
build_wheel.py handles SHA256 verification, binary extraction, and wheel packaging — all security-sensitive operations. There are no tests for:
- SHA256 verification logic (correct/incorrect checksums)
- Path traversal prevention in
extract_binaries - Binary name matching
- Platform/arch detection edge cases
Given the tarfile.extract() path traversal already flagged by the CI bot, this script needs a test suite.
| match screen { | ||
| Some(s) => s, | ||
| None => continue, // no screen size cached yet → leave field as-is | ||
| } |
Collaborator
There was a problem hiding this comment.
[Info] Silent passthrough when screenshot-size cache is empty.
When screenshot_w == 0 (no window basis cached) or the screen size cache is empty (for screen-basis tools like move_cursor), the coordinate field is silently left as-is — it stays in 0–1000 normalized space while the tool receives it expecting pixel coordinates.
This is correct behavior (passing garbage pixel values would be worse), but it makes debugging confusing. Consider adding a tracing::debug! log when a field is skipped due to missing cache, so users can diagnose "my normalized coordinates didn't get converted" issues.
| crate::coord_norm::ingest_screen_size(resolved_name, &result); | ||
| crate::coord_norm::normalize_result(resolved_name, &mut result); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
[Low] No daemon-level tool call tracing.
The daemon dispatch path has no structured logging of tool calls (name, args summary, latency, success/failure). This makes production debugging difficult — when something goes wrong, there's no audit trail of what the daemon processed.
Consider adding tracing::info! or tracing::debug! at the dispatch boundary with the tool name, session ID, and result status.
DragonnZhang
approved these changes
Jun 26, 2026
LaZzyMan
enabled auto-merge
June 26, 2026 12:57
qwen-code-ci-bot
requested changes
Jun 26, 2026
| if sys.platform != "win32": | ||
| os.chmod(binary_path, 0o755) | ||
|
|
||
| return binary_path |
Collaborator
There was a problem hiding this comment.
[Critical] os.chmod(binary_path, 0o755) runs unconditionally on every invocation with no error handling. On read-only or root-owned site-packages (common in Docker/CI), this raises PermissionError and prevents the driver from loading entirely — the error surfaces as a generic "Error executing cua-driver" from the caller's except clause.
Suggested change
| return binary_path | |
| # Ensure binary is executable on Unix (best-effort; read-only installs are valid) | |
| if sys.platform != "win32": | |
| try: | |
| os.chmod(binary_path, 0o755) | |
| except OSError: | |
| pass |
Or remove the runtime chmod entirely — build_wheel.py already sets 0o755 at extraction time.
— qwen3.7-max via Qwen Code /review
| <plist version="1.0"> | ||
| <dict> | ||
| <key>com.apple.security.automation.apple-events</key> | ||
| <true/> |
Collaborator
There was a problem hiding this comment.
[Critical] This entitlements file is referenced by the CD workflow's codesign steps, but it is missing com.apple.security.device.screen-capture. The other entitlements file at rust/scripts/CuaDriver.entitlements includes it. Released macOS binaries signed with this file will silently fail when attempting screen capture via ScreenCaptureKit.
Suggested change
| <true/> | |
| <key>com.apple.security.automation.apple-events</key> | |
| <true/> | |
| <key>com.apple.security.device.screen-capture</key> | |
| <true/> |
— qwen3.7-max via Qwen Code /review
| type: 'boolean' | ||
| default: true | ||
| dry_run: | ||
| description: 'Build + package only; do NOT create/update a GitHub Release. |
Collaborator
There was a problem hiding this comment.
[Critical] permissions: contents: write is declared at the workflow top level, granting write access to all jobs (build-linux, build-windows, build-macos, release). Only the release job needs contents: write to create GitHub Releases. Build jobs should run with contents: read per the principle of least privilege — a compromised build job could modify repository contents.
Suggested change
| description: 'Build + package only; do NOT create/update a GitHub Release. | |
| permissions: | |
| contents: 'read' |
Then add permissions: contents: write to the release job only.
— qwen3.7-max via Qwen Code /review
| /// `scale` is the normalization full-scale (the "1000" in 1000×1000). Qwen | ||
| /// `computer_use` uses 1000; some cookbooks use 999 — see `coordinate_scale`. | ||
| pub fn norm_to_px(norm: f64, dim: u32, scale: f64) -> f64 { | ||
| (norm / scale * dim as f64).round() |
Collaborator
There was a problem hiding this comment.
[Critical] norm_to_px has no bounds clamping on the input norm value. Out-of-range normalized coordinates (e.g., x: 2000 with scale: 1000 and an 800px window) produce pixel values far outside the window (1600px). These are passed directly to platform click APIs (CGEventPostToPid, SendInput), which dispatch to whatever is at that screen position. The entire value proposition of 0–1000 normalized mode is a safe, bounded coordinate space — without clamping, a prompt injection that convinces the model to emit out-of-range values can redirect clicks to arbitrary screen locations.
Suggested change
| (norm / scale * dim as f64).round() | |
| pub fn norm_to_px(norm: f64, dim: u32, scale: f64) -> f64 { | |
| let clamped = norm.clamp(0.0, scale); | |
| (clamped / scale * dim as f64).round() | |
| } |
— qwen3.7-max via Qwen Code /review
| .filter(|s| !s.is_empty()) | ||
| .map(|s| s.to_owned()); | ||
| if let Some(obj) = args.as_object_mut() { | ||
| if !obj.contains_key("_session_id") { |
Collaborator
There was a problem hiding this comment.
[Suggestion] apply_session_identity checks if !obj.contains_key("_session_id") before inserting the server-resolved value. A client that sends _session_id directly in tool arguments (instead of the documented session parameter) gets its value accepted verbatim, bypassing server-side session resolution. Since _session_id drives recording ownership, idle-TTL tracking, session-scoped config, and cursor lifecycle, a malicious client can impersonate any active session.
Always overwrite _session_id from the server-side resolution — never trust a client-supplied value:
Suggested change
| if !obj.contains_key("_session_id") { | |
| // Always stamp the resolved identity; never trust client-supplied _session_id | |
| if let Some(id) = explicit.clone().or_else(|| minted.clone()) { | |
| obj.insert("_session_id".to_owned(), serde_json::Value::String(id)); | |
| } |
— qwen3.7-max via Qwen Code /review
| # live in the cua repo, not here, so a 3-way merge can't | ||
| # look them up — and plain apply is all-or-nothing). | ||
| cd "$REPO_ROOT" | ||
| if git apply --reject --directory=packages/cua-driver -p3 "$PATCH"; then |
Collaborator
There was a problem hiding this comment.
[Critical] git apply --reject creates .rej files for failed hunks, but *.rej is not in packages/cua-driver/.gitignore. The script's post-apply instructions (line 97) tell the user to run git add -A, which will stage any remaining .rej files if the user doesn't clean up first. Combined with the unconditional .vendored-from write on failure, this can produce a commit that includes stale .rej artifacts alongside a .vendored-from that claims a newer upstream base than the code actually reflects.
Add *.rej to packages/cua-driver/.gitignore, or change the suggested commit command to git add -A -- ':!*.rej'.
— qwen3.7-max via Qwen Code /review
| # nowhere on purpose: this script is a one-shot, the user can re-set it. | ||
| $ProgressPreference = "SilentlyContinue" | ||
|
|
||
| $Repo = "trycua/cua" |
Collaborator
There was a problem hiding this comment.
[Critical] $Repo = "trycua/cua" — the PowerShell install script references the upstream repository in 7+ locations (lines 8, 12, 100, 216, 592, 1290, 1299). The CD workflow in this PR publishes releases under QwenLM/qwen-code with the cua-driver-rs-v* tag prefix, so this install script would fail to find any releases. This is inconsistent with the bash install scripts which correctly reference QwenLM/qwen-code.
Update all references from trycua/cua to QwenLM/qwen-code and adjust paths from libs/cua-driver/scripts/ to packages/cua-driver/scripts/.
— qwen3.7-max via Qwen Code /review
| }; | ||
|
|
||
| attempted += 1; | ||
| let result = registry.invoke(&tool_name, tool_args).await; |
Collaborator
There was a problem hiding this comment.
[Suggestion] replay_trajectory replays any tool from a trajectory file with no allowlist, including destructive tools like kill_app and launch_app (which can execute arbitrary binaries). A trajectory shared for regression testing (a documented workflow) could contain embedded destructive actions that execute silently — the MCP approval prompt shows only the replay_trajectory call, not the individual embedded tool calls.
Add a default allowlist of safe replay tools (click, right_click, double_click, scroll, type_text, press_key, hotkey, set_value, drag, move_cursor) and require an explicit allow_all_tools: true flag to replay destructive or open-world tools.
— qwen3.7-max via Qwen Code /review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Vendors the trycua/cua background-automation driver into
packages/cua-driveras qwen-cua-driver, and adds an opt-in 0–1000 relative-coordinate mode so Qwen-VLcomputer_use(which emits 0–1000 normalized coordinates) can drive the otherwise pixel-based tool surface. It also renames the binary/bundle so the fork coexists with any upstream trycua install, adds a cross-platform release workflow, and adds an upstream-sync script.Why it's needed
Qwen-VL models emit coordinates normalized to 0–1000, but cua-driver natively expects window-local screenshot pixels, so a model click of
500is treated as pixel 500 instead of the window's midpoint — clicks land in the wrong place. This adds a gated normalization shim (input denormalization, output screenshot-dim rewrite, tool/param descriptions, and agent instructions) plus zoom/move_cursor support, all behind an env toggle. With the toggle off the path is byte-identical to upstream, so existing pixel-based callers are unaffected.Reviewer Test Plan
How to verify
Build + unit tests (macOS/Linux):
Coordinate toggle (run the built
qwen-cua-driver mcpand readtools/list→click.x.description):CUA_DRIVER_RS_COORDINATE_SPACE=0→ pixel wording (upstream, unchanged)CUA_DRIVER_RS_COORDINATE_SPACE=1→0–1000 normalizedwordingCUA_DRIVER_RS_COORDINATE_SPACE=1 CUA_DRIVER_RS_COORDINATE_SCALE=999→0–999wordingEvidence (Before & After)
click.xparameter description, by mode (verified end-to-end against the built binary):click.xdescriptionWindow-local screenshot X coordinate.SPACE=1X coordinate, 0–1000 normalized to window width (top-left origin).SPACE=1 SCALE=999X coordinate, 0–999 normalized to window width (top-left origin).Screenshots / TUI: N/A (no user-visible TUI change; this is a driver + build change).
Tested on
macOS verified locally (unit tests + release build + the toggle checks above). Linux/Windows release builds run in CI on their native runners — no platform
click/drag/zoomcode was touched, so they are expected to be unaffected.Environment (optional)
N/A —
cargounit tests + a localqwen-cua-driver mcpstdio session.Risk & Scope
CUA_DRIVER_RS_COORDINATE_SPACE; off ⇒ upstream behavior. One root-level config change is required to host the vendored tree:packages/cua-driver/**added toeslint.config.jsignores (vendored code is not linted, same aspackages/desktop/**).qwen-cua-driver/QwenCuaDriver.app/ idcom.qwencode.cua-driver) specifically so this vendored fork coexists with any upstreamtrycuainstall rather than colliding with it.Linked Issues
N/A — no linked issue; this is a new vendored capability for qwen-code's computer-use stack.
中文说明
这个 PR 做了什么
把 trycua/cua 的后台自动化驱动 vendor 进
packages/cua-driver,命名为 qwen-cua-driver,并加入可选开启的 0–1000 相对坐标模式,让输出 0–1000 归一化坐标的 Qwen-VLcomputer_use能驱动原本基于像素的工具。同时把二进制/bundle 重命名以便与上游 trycua 共存,加了跨平台发布 workflow,以及一个上游同步脚本。为什么需要
Qwen-VL 输出 0–1000 归一化坐标,但 cua-driver 原生期望窗口内截图像素,所以模型输出的
500被当成第 500 像素而非窗口中点,点击落错位置。本 PR 加了一层受开关门控的归一化层(输入反归一化、输出截图尺寸改写、工具/参数描述、agent 指令)以及 zoom/move_cursor 支持。开关关闭时与上游逐字节一致,不影响现有的像素调用方。审核者验证
构建 + 单测(macOS/Linux):见英文
cargo test -p cua-driver-core(128 通过)、cargo build -p cua-driver --release。坐标开关(跑构建出的
qwen-cua-driver mcp,看tools/list的click.x.description):=0→ 像素措辞(上游,不变)=1→0–1000 normalized措辞=1+SCALE=999→0–999措辞证据见上方英文表(已对构建二进制端到端验证)。无 TUI 变化,截图 N/A。
测试平台:macOS ✅ 本地验证(单测 + release 构建 + 上述开关检查);Linux/Windows 的 release 构建在 CI 原生 runner 上跑——未改动任何 platform 的 click/drag/zoom 代码,预期不受影响。
风险与范围
CUA_DRIVER_RS_COORDINATE_SPACE门控,关闭即上游行为。为托管 vendored 目录需要一处根级配置改动:eslint.config.js的 ignores 加入packages/cua-driver/**(不 lint vendored 代码,与packages/desktop/**同理)。qwen-cua-driver/QwenCuaDriver.app/ idcom.qwencode.cua-driver)正是为了和上游 trycua 共存而非冲突。关联 Issue
无——这是为 qwen-code computer-use 栈新增的 vendored 能力。