feat(desktop-use): control engine pin, Atmos Desktop Use host, unified AppShot TCC - #202
Conversation
… host Replace placeholder control-engine ensure with a real pin of the cua-driver-rs v0.17.0 release artifact, white-labeled as Atmos Desktop Use: - Embedded manifest (URL + sha256) and download/extract into ~/.atmos/desktop-use - Managed binary atmos-desktop-control + rebranded Atmos Desktop Use.app host - Socket daemon + drive click/type/verify via call wrap (no public MCP) - doctor + grant-permissions for unified Settings TCC surface - Capture remains Atmos-owned; vendor brands scrubbed from user text Strategy: pin release artifacts (not monorepo compile-in-tree).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughDesktop Use now installs a pinned host engine, manages its daemon, exposes permission and drive commands, and routes AppShot capture through the host engine when installed. Web settings, AppShot permissions, localization, documentation, and slash commands use the shared Desktop Use integration. Browser Use is added as a separate feature with CUA external backend and fail-closed embedded fallback. ChangesDesktop Use host-engine integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant DesktopUseSettings
participant DesktopUseManager
participant ensure_daemon
participant HostEngine
participant DriveExecutor
participant AppShot
User->>DesktopUseSettings: install desktop-use engine
DesktopUseSettings->>DesktopUseManager: download_and_install()
DesktopUseManager->>ensure_daemon: start daemon
ensure_daemon->>HostEngine: spawn via LaunchServices
HostEngine-->>DesktopUseManager: socket ready
User->>DesktopUseSettings: grant permissions
DesktopUseSettings->>HostEngine: open System Settings privacy pane
User->>AppShot: take screenshot
AppShot->>DesktopUseManager: use host-engine capture
DesktopUseManager->>DriveExecutor: call_tool(get_desktop_state)
HostEngine->>DriveExecutor: return PNG and window metadata
DriveExecutor-->>AppShot: frontmost window with via: host_engine
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
Address skeptic gaps: start the control-engine daemon through LaunchServices (open -a Atmos Desktop Use.app --args serve) so live process Identifier is com.atmos.desktop.use, matching grant-permissions and Settings doctor. - doctor parses real accessibility/screen_recording from health_report - AppShot permissions panel uses host doctor + host grant when engine installed - screenshot drive prefers host get_desktop_state when engine present
…alled When the control engine is ready, dual-shift AppShot capture uses atmos-desktop-control via Atmos Desktop Use.app so Screen Recording and Accessibility grants match control (no Electron TCC split). Electron in-process capture remains only the pre-ensure fallback.
CI Format Check and Clippy failed: rustfmt diffs in control/host/manager, and spawn_daemon host_app is only used on the macOS LaunchServices path.
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
AppShot dual-shift production path is host-engine when installed; the in-process capture helper must not be described as the hot path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (7)
crates/desktop-use/src/engine_manifest.rs (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScrub vendor tokens from the manifest parse error.
parsereturns raw serde text.download_and_installpropagates this error to the CLI and Settings UI. A serde message can quote manifest content, which contains vendor identifiers such as the upstream tag. Route the message throughcrate::strings::scrub_vendorto keep the crate rule "Never expose third-party vendor brands in public strings" enforced on every path.♻️ Proposed change
pub fn parse(json: &str) -> Result<Self, String> { - serde_json::from_str(json).map_err(|e| format!("invalid engine manifest: {e}")) + serde_json::from_str(json) + .map_err(|e| crate::strings::scrub_vendor(&format!("invalid engine manifest: {e}"))) }🤖 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 `@crates/desktop-use/src/engine_manifest.rs` around lines 31 - 33, Update EngineManifest::parse to pass the formatted serde error through crate::strings::scrub_vendor before returning it, preserving the existing “invalid engine manifest” context while ensuring vendor identifiers cannot reach download_and_install or the CLI/Settings UI.crates/desktop-use/src/host.rs (1)
103-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
Childinstead of leaking it.
std::mem::forget(child)leaks the handle and prevents reaping. Dropping aChilddoes not kill the process in Rust, so the detach works withoutforget. Thelet _ = child.id();line has no effect.♻️ Proposed change
- let child = Command::new(engine_bin) + // Dropping Child detaches the process; Rust does not kill on drop. + let _child = Command::new(engine_bin) .args(["serve", "--socket", &socket_str, "--no-permissions-gate"]) .env("CUA_DRIVER_RS_PERMISSIONS_GATE", "0") .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) .spawn() .map_err(|e| scrub_vendor(&format!("failed to start control engine: {e}")))?; - let _ = child.id(); - std::mem::forget(child); 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 `@crates/desktop-use/src/host.rs` around lines 103 - 113, In the control-engine startup flow, remove the unnecessary child.id() call and replace std::mem::forget(child) with a normal drop of the Child handle. Keep the existing spawn, error mapping, and detached-process behavior unchanged while allowing the handle to be reclaimed and reaped.crates/desktop-use/src/install.rs (2)
257-287: 🩺 Stability & Availability | 🔵 TrivialAd-hoc re-signing resets macOS TCC grants on every install.
install_host_apprewritesInfo.plistand then ad-hoc signs the bundle. macOS binds Accessibility and Screen Recording grants to the code signature identity. Everyensure --forceproduces a new ad-hoc signature, so the user must grant permissions again. Track this with the pending production signing work, and document the re-grant behavior in the Settings flow.🤖 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 `@crates/desktop-use/src/install.rs` around lines 257 - 287, Update install_host_app and the related Settings flow documentation to account for TCC grants being invalidated when the host bundle is re-signed during ensure --force. Keep the current ad-hoc signing behavior tied to the pending production-signing work, and clearly document that users may need to grant Accessibility and Screen Recording permissions again after reinstalling or updating the host.
307-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the empty best-effort branch.
The
ifbody is empty, so the condition has no effect. Clippy reports this pattern. Discard the status directly.♻️ Proposed change
for (key, value) in sets { - let status = Command::new("plutil") + // best-effort: plist rebranding must not fail installation. + let _ = Command::new("plutil") .args(["-replace", key, "-string", value]) .arg(plist) .status(); - if status.map(|s| !s.success()).unwrap_or(true) { - // best-effort - } }🤖 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 `@crates/desktop-use/src/install.rs` around lines 307 - 315, In the loop over sets, update the plutil invocation in the status handling to discard the returned status directly instead of evaluating it in an empty if branch. Remove the unused success-condition logic while preserving the existing command arguments and iteration.apps/desktop-electron/src/appshot/frontmost-route.test.ts (1)
10-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the source-text assertions with behavior assertions.
These tests read source files and match identifiers and prose. Two consequences follow:
- Any comment rewording breaks the suite. Lines 36 and 37 assert documentation text, not behavior.
- A refactor that keeps the correct routing but renames a local symbol also breaks the suite, while a real routing regression can still pass.
Mock
resolveAppShotCaptureRouteandcaptureFrontmostViaHostEngine, then assert thatcaptureFrontmostWindowreturnsvia: "host_engine"when the driver is installed andvia: "electron_fallback"when it is not. Theviafield added inapps/desktop-electron/src/appshot/frontmost.tsLine 30 exists for this purpose.🤖 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 `@apps/desktop-electron/src/appshot/frontmost-route.test.ts` around lines 10 - 38, Replace the source-text checks in the frontmost-route tests with behavioral tests that mock resolveAppShotCaptureRoute and captureFrontmostViaHostEngine. Exercise captureFrontmostWindow with an installed driver and assert it returns via: "host_engine", then with no installed driver and assert it returns via: "electron_fallback"; remove assertions tied to identifier names or documentation text.apps/desktop-electron/src/desktop-use/host-capture.test.ts (1)
57-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for missing
z_index.The tests cover explicit
z_indexvalues only.pickFrontmostWindowmaps a missing ornullz_indexto-1, and equal values select the last row. Add a case with noz_indexon any row to pin the intended tie-break, which is the behavior questioned in the comment onapps/desktop-electron/src/desktop-use/host-capture.tsLines 44-55.🤖 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 `@apps/desktop-electron/src/desktop-use/host-capture.test.ts` around lines 57 - 63, Add a test alongside the existing pickFrontmostWindow fallback case with multiple off-screen rows that omit z_index, and assert the last row is selected when all mapped values tie at -1. Keep the test focused on the missing-z_index tie-break behavior.crates/desktop-use/src/control.rs (1)
128-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate engine resolution.
drivealready callsmanager.require_engine().is_ok()before it callsscreenshot_via_engine(Line 90). Theelsebranch here is unreachable, and the engine is resolved twice. Pass the resolved engine into the function, asrun_enginealready does.♻️ Proposed refactor
-fn screenshot_via_engine( - manager: &DesktopUseManager, - req: &DriveRequest, - action_name: &str, -) -> DriveResult { - let Ok(engine) = manager.require_engine() else { - return DriveResult { - ok: false, - action: action_name.into(), - detail: None, - capture: None, - result: None, - error: Some(ERR_ENGINE_NOT_INSTALLED.into()), - error_code: Some("control_engine_not_installed".into()), - }; - }; +fn screenshot_via_engine( + manager: &DesktopUseManager, + engine: &Path, + req: &DriveRequest, + action_name: &str, +) -> DriveResult {Update the call site in
drive:match manager.require_engine() { Ok(engine) => return screenshot_via_engine(manager, &engine, &req, action_name), Err(_) => { /* fall through to local capture */ } }🤖 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 `@crates/desktop-use/src/control.rs` around lines 128 - 143, Remove the second engine lookup from screenshot_via_engine by adding a resolved engine parameter and using it there as run_engine does. Update drive to pass the engine returned by manager.require_engine() on the successful path while preserving the existing fallback behavior for errors, and delete the now-unreachable engine-not-installed branch.
🤖 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 `@apps/cli/src/commands/desktop_use.rs`:
- Around line 176-185: Update driver_grant and the underlying
DesktopUseManager::open_permission_grant flow so non-macOS no-op behavior is not
reported as a successful applied grant. Return a platform-specific response or
expose an applied indicator, ensuring Linux and Windows callers can distinguish
the real macOS permission flow from the no-op while preserving the existing
success response for macOS.
In `@apps/desktop-electron/src/appshot/frontmost.ts`:
- Around line 44-57: Replace the metadata path in readFrontmostWindow with a new
exported host-capture helper that uses desktopUseDriveVerify(),
pickFrontmostWindow, and hostWindowToFrontmost without invoking
desktopUseDriveScreenshot(); preserve the existing fallback to
desktopUseReadFrontmost() on failure. Also add short-lived caching for the
installed-state lookup used by resolveAppShotCaptureRoute() to avoid spawning
the CLI on every call.
In `@apps/desktop-electron/src/desktop-use/host-capture.ts`:
- Around line 133-140: Wrap the desktopUseDriveScreenshot() call in
captureFrontmostViaHostEngine with a try/catch, matching the existing
desktopUseDriveVerify() handling. Preserve the current unsuccessful-result
warning and also push the caught rejection’s error details, so screenshot
failures do not abort the subsequent window-list call or discard available
metadata.
In `@apps/web/src/features/appshot/components/AppshotPermissionsPanel.tsx`:
- Around line 301-311: Localize all new host-permission copy: in
apps/web/src/features/appshot/components/AppshotPermissionsPanel.tsx lines
301-311, pass translated labels and manual instructions into
statusFromHostDoctor; in
apps/web/src/features/settings/components/DesktopUseSettingsSection.tsx line
148, replace the host-name fallback with a translation lookup; add the
corresponding English keys in apps/web/messages/en.json lines 5530-5573 and
matching Chinese translations in apps/web/messages/zh.json lines 5530-5573.
In `@crates/desktop-use/src/control.rs`:
- Around line 157-179: Update the screenshot-writing logic in the
get_desktop_state success branch to report failures when req.out_path is set:
require a screenshot_base64 or png_base64 payload, report missing image data,
and propagate base64 decoding or std::fs::write errors through a failed
DriveResult instead of returning ok: true. Preserve the existing successful
result when no output path is requested.
In `@crates/desktop-use/src/host.rs`:
- Around line 134-151: Update call_tool to replace blocking Command::output()
with spawned-child handling and a 12-second deadline, matching ensure_daemon’s
timeout. Poll or wait with the deadline, kill the child when it expires, and
return a scrubbed timeout error; preserve existing argument construction and
error handling for successful or failed subprocess execution.
- Around line 21-23: Replace the socket.exists() check in is_daemon_alive with
an actual connection attempt to the Unix socket, returning true only when the
connection succeeds and false when it fails. In crates/desktop-use/src/doctor.rs
lines 45-73, retain the existing daemon_running || is_daemon_alive fallback; no
direct change is needed there because it is corrected by the host probe.
- Around line 116-131: Update stop_daemon to run the macOS pkill fallback only
when the specified socket exists, and scope its pattern to the escaped socket
path so it cannot terminate unrelated host daemons. Add a small helper for
escaping regex metacharacters before constructing the pkill pattern, while
preserving the existing engine_bin stop attempt and socket cleanup.
In `@crates/desktop-use/src/install.rs`:
- Around line 241-255: Update find_named_file so an unreadable directory is
skipped rather than returning None from the entire search; handle fs::read_dir
failure for the current stack entry by continuing to the remaining directories,
while preserving the existing recursive traversal and matching behavior.
- Around line 340-349: Update chrono_lite_now to request UTC output by adding
the date command’s UTC option before the existing RFC3339 format argument,
preserving the current fallback behavior and returned timestamp format.
- Around line 130-140: Update the ATMOS_DESKTOP_USE_ENGINE_ARCHIVE handling in
the installer to support an explicit opt-out from SHA-256 verification for local
fixture archives, while retaining verification by default. Use a clearly named
environment variable consistent with the existing configuration and document or
preserve the error behavior when the opt-out is not enabled.
- Around line 185-198: Update the "zip" branch of extract_archive to avoid
relying on Command::new("unzip"), which is unavailable on Windows. Implement ZIP
extraction with an existing Rust extractor dependency, or use a Windows-native
PowerShell Expand-Archive path, while preserving extraction into dest and the
current error propagation and scrubbing behavior.
In `@crates/desktop-use/src/manager.rs`:
- Around line 357-372: The test
ensure_without_network_or_fixture_fails_vendor_free mutates process-wide
environment variables without synchronization. Protect its
ATMOS_DESKTOP_USE_ENGINE_SOURCE, ATMOS_DESKTOP_USE_ENGINE_ARCHIVE, and
ATMOS_DESKTOP_USE_SKIP_DOWNLOAD changes with the shared environment-test
serialization guard used by affected tests, preserving cleanup while the guard
remains held.
In `@specs/APP/APP-052_desktop-use/TECH.md`:
- Line 32: Update sections 4 (“Capture execution identity (M1 lock)”) and 8 to
align with item 6: describe AppShot dual-shift capture as using the host engine
for `drive screenshot` and window listing after installation, with Electron
in-process capture limited to the pre-ensure fallback.
---
Nitpick comments:
In `@apps/desktop-electron/src/appshot/frontmost-route.test.ts`:
- Around line 10-38: Replace the source-text checks in the frontmost-route tests
with behavioral tests that mock resolveAppShotCaptureRoute and
captureFrontmostViaHostEngine. Exercise captureFrontmostWindow with an installed
driver and assert it returns via: "host_engine", then with no installed driver
and assert it returns via: "electron_fallback"; remove assertions tied to
identifier names or documentation text.
In `@apps/desktop-electron/src/desktop-use/host-capture.test.ts`:
- Around line 57-63: Add a test alongside the existing pickFrontmostWindow
fallback case with multiple off-screen rows that omit z_index, and assert the
last row is selected when all mapped values tie at -1. Keep the test focused on
the missing-z_index tie-break behavior.
In `@crates/desktop-use/src/control.rs`:
- Around line 128-143: Remove the second engine lookup from
screenshot_via_engine by adding a resolved engine parameter and using it there
as run_engine does. Update drive to pass the engine returned by
manager.require_engine() on the successful path while preserving the existing
fallback behavior for errors, and delete the now-unreachable
engine-not-installed branch.
In `@crates/desktop-use/src/engine_manifest.rs`:
- Around line 31-33: Update EngineManifest::parse to pass the formatted serde
error through crate::strings::scrub_vendor before returning it, preserving the
existing “invalid engine manifest” context while ensuring vendor identifiers
cannot reach download_and_install or the CLI/Settings UI.
In `@crates/desktop-use/src/host.rs`:
- Around line 103-113: In the control-engine startup flow, remove the
unnecessary child.id() call and replace std::mem::forget(child) with a normal
drop of the Child handle. Keep the existing spawn, error mapping, and
detached-process behavior unchanged while allowing the handle to be reclaimed
and reaped.
In `@crates/desktop-use/src/install.rs`:
- Around line 257-287: Update install_host_app and the related Settings flow
documentation to account for TCC grants being invalidated when the host bundle
is re-signed during ensure --force. Keep the current ad-hoc signing behavior
tied to the pending production-signing work, and clearly document that users may
need to grant Accessibility and Screen Recording permissions again after
reinstalling or updating the host.
- Around line 307-315: In the loop over sets, update the plutil invocation in
the status handling to discard the returned status directly instead of
evaluating it in an empty if branch. Remove the unused success-condition logic
while preserving the existing command arguments and iteration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f84e306-1457-4601-b1a2-8af6e1ae0eca
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
apps/cli/src/commands/desktop_use.rsapps/desktop-electron/src/appshot/frontmost-route.test.tsapps/desktop-electron/src/appshot/frontmost.tsapps/desktop-electron/src/desktop-use/capture.tsapps/desktop-electron/src/desktop-use/client.tsapps/desktop-electron/src/desktop-use/host-capture.test.tsapps/desktop-electron/src/desktop-use/host-capture.tsapps/desktop-electron/src/ipc/handlers.tsapps/web/messages/en.jsonapps/web/messages/zh.jsonapps/web/src/features/appshot/components/AppshotPermissionsPanel.tsxapps/web/src/features/settings/components/DesktopUseSettingsSection.tsxcrates/desktop-use/Cargo.tomlcrates/desktop-use/manifest/default.jsoncrates/desktop-use/src/control.rscrates/desktop-use/src/doctor.rscrates/desktop-use/src/engine_manifest.rscrates/desktop-use/src/host.rscrates/desktop-use/src/install.rscrates/desktop-use/src/lib.rscrates/desktop-use/src/manager.rscrates/desktop-use/src/strings.rsspecs/APP/APP-052_desktop-use/TECH.md
| export async function readFrontmostWindow(): Promise<FrontmostWindow> { | ||
| const route = await resolveAppShotCaptureRoute(); | ||
| if (route === "host_engine") { | ||
| try { | ||
| const cap = await captureFrontmostViaHostEngine({ | ||
| selfAppNames: SELF_APP_NAMES, | ||
| }); | ||
| return cap.frontmost; | ||
| } catch { | ||
| /* fall through */ | ||
| } | ||
| } | ||
| return desktopUseReadFrontmost(); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid a full screenshot for a metadata-only read.
readFrontmostWindow documents a metadata-only read, but captureFrontmostViaHostEngine first calls desktopUseDriveScreenshot() (apps/desktop-electron/src/desktop-use/host-capture.ts Line 133). Each metadata read therefore spawns the CLI twice, transfers a base64 screenshot through stdout, and decodes it. The screenshot call allows up to 45 seconds. The trigger overlay path uses this function, so the added latency is user-visible.
Export a metadata-only helper from host-capture.ts that calls desktopUseDriveVerify() and reuses pickFrontmostWindow plus hostWindowToFrontmost, then call it here.
resolveAppShotCaptureRoute() also spawns the CLI on every call at Lines 45 and 66. Consider caching the installed state for a short interval, because the engine install state changes rarely.
🤖 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 `@apps/desktop-electron/src/appshot/frontmost.ts` around lines 44 - 57, Replace
the metadata path in readFrontmostWindow with a new exported host-capture helper
that uses desktopUseDriveVerify(), pickFrontmostWindow, and
hostWindowToFrontmost without invoking desktopUseDriveScreenshot(); preserve the
existing fallback to desktopUseReadFrontmost() on failure. Also add short-lived
caching for the installed-state lookup used by resolveAppShotCaptureRoute() to
avoid spawning the CLI on every call.
| const shot = await desktopUseDriveScreenshot(); | ||
| if (!(shot as { ok?: boolean })?.ok) { | ||
| const err = | ||
| typeof (shot as { error?: string }).error === "string" | ||
| ? (shot as { error: string }).error | ||
| : "host engine screenshot failed"; | ||
| warnings.push(err); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the screenshot call like the window-list call.
Line 133 calls desktopUseDriveScreenshot() outside a try block. runDesktopUseJson uses execFile with a 45 second timeout, so a CLI spawn error, a timeout, or unparsable output rejects the promise. The rejection aborts captureFrontmostViaHostEngine before the list_windows call, and captureFrontmostWindow then returns "Unknown App" with no bounds. The window list is available in that situation, so the metadata is lost without need.
Wrap the call and record a warning, as the desktopUseDriveVerify() call already does.
🐛 Proposed fix
- const shot = await desktopUseDriveScreenshot();
- if (!(shot as { ok?: boolean })?.ok) {
- const err =
- typeof (shot as { error?: string }).error === "string"
- ? (shot as { error: string }).error
- : "host engine screenshot failed";
- warnings.push(err);
- }
+ let shot: unknown = null;
+ try {
+ shot = await desktopUseDriveScreenshot();
+ } catch (e) {
+ warnings.push(
+ `host_engine_screenshot_failed: ${e instanceof Error ? e.message : String(e)}`,
+ );
+ }
+ if (shot && !(shot as { ok?: boolean })?.ok) {
+ const err =
+ typeof (shot as { error?: string }).error === "string"
+ ? (shot as { error: string }).error
+ : "host engine screenshot failed";
+ warnings.push(err);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shot = await desktopUseDriveScreenshot(); | |
| if (!(shot as { ok?: boolean })?.ok) { | |
| const err = | |
| typeof (shot as { error?: string }).error === "string" | |
| ? (shot as { error: string }).error | |
| : "host engine screenshot failed"; | |
| warnings.push(err); | |
| } | |
| let shot: unknown = null; | |
| try { | |
| shot = await desktopUseDriveScreenshot(); | |
| } catch (e) { | |
| warnings.push( | |
| `host_engine_screenshot_failed: ${e instanceof Error ? e.message : String(e)}`, | |
| ); | |
| } | |
| if (shot && !(shot as { ok?: boolean })?.ok) { | |
| const err = | |
| typeof (shot as { error?: string }).error === "string" | |
| ? (shot as { error: string }).error | |
| : "host engine screenshot failed"; | |
| warnings.push(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 `@apps/desktop-electron/src/desktop-use/host-capture.ts` around lines 133 -
140, Wrap the desktopUseDriveScreenshot() call in captureFrontmostViaHostEngine
with a try/catch, matching the existing desktopUseDriveVerify() handling.
Preserve the current unsuccessful-result warning and also push the caught
rejection’s error details, so screenshot failures do not abort the subsequent
window-list call or discard available metadata.
| "zip" => { | ||
| // Prefer system unzip for windows zips in M1 to avoid heavy zip feature matrix. | ||
| let status = Command::new("unzip") | ||
| .args(["-q", "-o"]) | ||
| .arg(archive) | ||
| .arg("-d") | ||
| .arg(dest) | ||
| .status() | ||
| .map_err(|e| scrub_vendor(&format!("unzip failed: {e}")))?; | ||
| if !status.success() { | ||
| return Err(scrub_vendor("failed to extract engine package (zip)")); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the workspace already depends on a zip crate usable here.
set -uo pipefail
fd -t f 'Cargo.toml' | xargs rg -n 'zip|sevenz|Expand-Archive|tar\.exe' || echo "no zip crate / windows extractor found"
rg -n 'unzip|Expand-Archive|tar\.exe' --type=rustRepository: AruNi-01/atmos
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- install.rs outline ---\n'
ast-grep outline crates/desktop-use/src/install.rs --view expanded || true
printf '\n--- install.rs relevant lines 150-220 ---\n'
sed -n '150,220p' crates/desktop-use/src/install.rs | nl -ba -v150
printf '\n--- desktop-use Cargo.toml ---\n'
sed -n '1,120p' crates/desktop-use/Cargo.toml
printf '\n--- workspace dependency declarations containing desktop/use ---\n'
fd -t f 'Cargo.toml' . -x sh -c 'echo "### $1"; grep -n "desktop-use\|= { path = \"../desktop-use\"" "$1" || true' sh {}Repository: AruNi-01/atmos
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- install.rs outline ---'
ast-grep outline crates/desktop-use/src/install.rs --view expanded || true
printf '%s\n' ''
printf '%s\n' '--- install.rs relevant lines 150-220 ---'
nl -ba -v150 crates/desktop-use/src/install.rs | sed -n '150,220p'
printf '%s\n' ''
printf '%s\n' '--- desktop-use Cargo.toml ---'
nl -ba crates/desktop-use/Cargo.toml | sed -n '1,120p'
printf '%s\n' ''
printf '%s\n' '--- workspace dependency declarations containing desktop-use ---'
fd -t f Cargo.toml . -x sh -c 'echo "### $1"; grep -n "desktop-use" "$1" || true' sh {}Repository: AruNi-01/atmos
Length of output: 1334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- install.rs relevant lines 150-220 ---'
sed -n '150,220p' crates/desktop-use/src/install.rs
printf '%s\n' ''
printf '%s\n' '--- desktop-use Cargo.toml ---'
cat -n crates/desktop-use/Cargo.toml | sed -n '1,120p'
printf '%s\n' ''
printf '%s\n' '--- workspace dependency declarations containing desktop-use ---'
fd -t f Cargo.toml . -x sh -c 'echo "### $1"; grep -n "desktop-use" "$1" || true' sh {}Repository: AruNi-01/atmos
Length of output: 4118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- manifest zip entries ---'
rg -n '"zip"|\.zip|engine' crates -g '*.rs' -g '*.toml' | head -200
printf '%s\n' ''
printf '%s\n' '--- extract_archive call sites and command imports ---'
rg -n 'extract_archive|Command::new\("unzip"|use std::process::Command' crates/desktop-use/src/install.rs crates -g '*.rs'Repository: AruNi-01/atmos
Length of output: 20373
Replace the Windows-unavailable Command::new("unzip") path.
extract_archive() uses Command::new("unzip") for archive_kind = "zip", while desktop-use has no Windows zip extractor dependency or tar.exe fallback. This returns NotFound to the caller, so Windows installs that need the zip entry fail. Use a Rust zip extractor, or route ZIP extraction through a Windows-native option like PowerShell Expand-Archive.
🤖 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 `@crates/desktop-use/src/install.rs` around lines 185 - 198, Update the "zip"
branch of extract_archive to avoid relying on Command::new("unzip"), which is
unavailable on Windows. Implement ZIP extraction with an existing Rust extractor
dependency, or use a Windows-native PowerShell Expand-Archive path, while
preserving extraction into dest and the current error propagation and scrubbing
behavior.
| fn ensure_without_network_or_fixture_fails_vendor_free() { | ||
| let dir = tempdir().unwrap(); | ||
| let engine = dir.path().join("bin").join("atmos-desktop-control"); | ||
| let mgr = DesktopUseManager::with_paths(dir.path(), &engine); | ||
| let out = mgr.ensure_driver_from(false, None); | ||
| // Force path that ignores env: pass empty non-existent path via ensure that | ||
| // only uses explicit None and with env cleared for this process key if set. | ||
| let out = if std::env::var_os("ATMOS_DESKTOP_USE_ENGINE_SOURCE").is_some() { | ||
| // Explicit missing path should still fail | ||
| mgr.ensure_driver_from(true, Some(Path::new("/nonexistent/desktop-use-engine"))) | ||
| } else { | ||
| out | ||
| }; | ||
| std::env::remove_var("ATMOS_DESKTOP_USE_ENGINE_SOURCE"); | ||
| std::env::remove_var("ATMOS_DESKTOP_USE_ENGINE_ARCHIVE"); | ||
| std::env::set_var("ATMOS_DESKTOP_USE_SKIP_DOWNLOAD", "1"); | ||
| let out = mgr.ensure_driver_from(true, None); | ||
| std::env::remove_var("ATMOS_DESKTOP_USE_SKIP_DOWNLOAD"); | ||
| match out { | ||
| EnsureOutcome::Failed { error } => { | ||
| assert!(!error.is_empty()); | ||
| assert!(!strings::contains_vendor_brand(&error)); | ||
| } | ||
| other => panic!("expected failed, got {other:?}"), | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the crate edition and any other tests that mutate these env vars.
set -uo pipefail
rg -n 'edition' crates/desktop-use/Cargo.toml Cargo.toml
rg -n 'set_var|remove_var' --type=rustRepository: AruNi-01/atmos
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== manager env mutation tests =="
rg -n 'ensure_without_network_or_fixture_fails_vendor_free|ensure_from_source_file|stop_and_uninstall|ATMOS_DESKTOP_USE_(ENGINE_SOURCE|ENGINE_ARCHIVE|SKIP_DOWNLOAD)|set_var|remove_var' crates/desktop-use/src/manager.rs || true
echo
echo "== manager outline =="
ast-grep outline crates/desktop-use/src/manager.rs --view expanded | sed -n '1,220p' || true
echo
echo "== ensure_driver_from definitions/usages =="
rg -n 'fn ensure_driver_from|fn install|download_and_install|ATMOS_DESKTOP_USE' crates/desktop-use/src -SRepository: AruNi-01/atmos
Length of output: 5384
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== crates/desktop-use/src/manager.rs lines 320-392 =="
sed -n '320,392p' crates/desktop-use/src/manager.rs | cat -n | sed 's/^/ /'
echo
echo "== crates/desktop-use/src/install.rs lines 94-165 =="
sed -n '94,165p' crates/desktop-use/src/install.rs | cat -n | sed 's/^/ /'Repository: AruNi-01/atmos
Length of output: 7242
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== all Rust files with ATMOS_DESKTOP_USE env mutation/reads =="
rg -n 'std::env::\b(set_var|remove_var|var|var_os)\b|ATMOS_DESKTOP_USE_' --type=rust
echo
echo "== list desktop-use test sources =="
git ls-files 'crates/desktop-use/**/*.rs'Repository: AruNi-01/atmos
Length of output: 215
Guard the env mutation used by this test.
std::env::set_var and remove_var affect the process-wide environment while tests run in the same process. Thread-local reads of ATMOS_DESKTOP_USE_SKIP_DOWNLOAD and fixture env vars such as ATMOS_DESKTOP_USE_ENGINE_ARCHIVE can be affected by this test on other paths. Put the env mutation behind a shared serialization guard for affected tests, or avoid mutating ATMOS_DESKTOP_USE_SKIP_DOWNLOAD in shared fixtures.
crates/desktop-use/Cargo.toml uses Rust 2021, so std::env::set_var is not unsafe here.
🤖 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 `@crates/desktop-use/src/manager.rs` around lines 357 - 372, The test
ensure_without_network_or_fixture_fails_vendor_free mutates process-wide
environment variables without synchronization. Protect its
ATMOS_DESKTOP_USE_ENGINE_SOURCE, ATMOS_DESKTOP_USE_ENGINE_ARCHIVE, and
ATMOS_DESKTOP_USE_SKIP_DOWNLOAD changes with the shared environment-test
serialization guard used by affected tests, preserving cleanup while the guard
remains held.
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
Call tool no longer treats exit-0 plain-text engine failures as ok/raw. Screenshot path uses --screenshot-out-file + MCP image / screenshot_file_path fixtures, normalizes png_base64 for AppShot host capture, and fails drive when image bytes are missing.
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
AruNi-01
changed the title
PR board cleanup (same Desktop Use epic)
Title/body refreshed to cover the full #202 stack (not just the first pin commit). |
…tices Improve APP-052 Desktop Use end-to-end: Settings install/permissions and AppShot wiring, system skill + slash command, click coordinate modes and background delivery, blinking z-order-aware operation borders with prefs and session-end clear, plus NOTICE/AGENTS guidance for redistributed third-party engines (cua-driver, llama.cpp, catalog models).
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (8)
apps/web/src/features/appshot/lib/appshot-client.ts (1)
141-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused locale parameter and its default call.
_localeis never read, butcurrentAppLocale()still runs on every call. Remove the parameter and update the remaining call sites, or mark the function@deprecatedand point callers atopenDesktopUseSettingsInApp.🤖 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 `@apps/web/src/features/appshot/lib/appshot-client.ts` around lines 141 - 147, Remove the unused _locale parameter and currentAppLocale() default evaluation from showAppshotPermissionsWindow, then update all call sites to invoke it without an argument. Keep the existing dynamic import and openDesktopUseSettingsInApp invocation unchanged.apps/web/src/features/appshot/components/AppshotsHistoryPopover.tsx (1)
212-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the status watcher from this handler, or start it before closing the popover.
onClose?.()closes the Radix popover, soPopoverContentunmounts. The unmount effect at Lines 194-200 callspermissionWatcherRef.current?.(), which cancels the watcher created on Line 217 almost immediately. The watcher therefore performs no useful refresh, and any refresh that does land updates an unmounting component.Settings now owns the permission recovery flow, so the popover does not need its own watcher.
♻️ Proposed simplification
const handleOpenPermission = React.useCallback(() => { // Header Appshots → authorize: open Settings → Desktop Use (only path). onClose?.(); openDesktopUseSettings(); - permissionWatcherRef.current?.(); - permissionWatcherRef.current = watchAppshotStatusAfterPermissionOpen(refreshStatus); - }, [onClose, openDesktopUseSettings, refreshStatus]); + }, [onClose, openDesktopUseSettings]);🤖 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 `@apps/web/src/features/appshot/components/AppshotsHistoryPopover.tsx` around lines 212 - 218, Remove the permission watcher setup from handleOpenPermission, including the watchAppshotStatusAfterPermissionOpen call and related cleanup state if it is only used by this handler; retain closing the popover, opening Desktop Use settings, and the existing callback dependencies needed for those actions.apps/web/src/features/settings/components/DesktopUsePermissionsPanel.tsx (2)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
Buttonfrom the atomic component path.The coding guidelines require atomic UI components from
@workspace/ui/components/ui/*. Keepcnfrom the utility export.♻️ Proposed change
-import { Button, cn } from "`@workspace/ui`"; +import { cn } from "`@workspace/ui`"; +import { Button } from "`@workspace/ui/components/ui/button`";As per coding guidelines: "Use
@workspace/ui/components/ui/*for atomic UI components."🤖 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 `@apps/web/src/features/settings/components/DesktopUsePermissionsPanel.tsx` at line 9, Update the imports in DesktopUsePermissionsPanel so Button comes from the atomic component path under `@workspace/ui/components/ui/`*, while retaining cn from the existing `@workspace/ui` utility export.Source: Coding guidelines
99-108: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueStop the poll when both permissions are granted.
The interval runs for the full 45 seconds even after
doctorreports both permissions granted. Each tick issues adesktop_use_doctorIPC call and a re-render. Stop the poll whenaccessibilityandscreen_recordingare bothtrue.🤖 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 `@apps/web/src/features/settings/components/DesktopUsePermissionsPanel.tsx` around lines 99 - 108, Update the polling flow in DesktopUsePermissionsPanel to stop the interval immediately when refresh reports both accessibility and screen_recording as true. Reuse the existing stopPoll function and ensure the completion check applies to the initial refresh and subsequent interval ticks while preserving the 45-second timeout.apps/web/src/features/appshot/components/AppshotPermissionsWindow.tsx (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
Buttonfrom the atomic component path.The coding guidelines require atomic UI components from
@workspace/ui/components/ui/*.DesktopUseSettingsSection.tsxalready follows this forSwitch.♻️ Proposed change
-import { Button } from "`@workspace/ui`"; +import { Button } from "`@workspace/ui/components/ui/button`";As per coding guidelines: "Use
@workspace/ui/components/ui/*for atomic UI components."🤖 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 `@apps/web/src/features/appshot/components/AppshotPermissionsWindow.tsx` at line 13, Update the Button import in AppshotPermissionsWindow.tsx to use the atomic component path under `@workspace/ui/components/ui/`*, matching the established import pattern used by DesktopUseSettingsSection.tsx.Source: Coding guidelines
crates/desktop-use/src/control.rs (1)
604-700: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the desktop PNG scale instead of capturing a full screenshot per click.
to_engine_desktop_xycallsdesktop_png_scalefor everyCoordSpace::Pointsclick.desktop_png_scaleruns a fullget_desktop_statecapture only to read four metadata numbers. Each points-space click therefore pays a complete desktop capture before the click itself, which doubles latency and writes an extra temp PNG.Cache the scale per socket for the process lifetime, or reuse metadata from the last
screenshot_via_engineresult.🤖 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 `@crates/desktop-use/src/control.rs` around lines 604 - 700, The points-coordinate path in to_engine_desktop_xy currently invokes desktop_png_scale for every click, causing redundant screenshot captures. Cache the computed desktop PNG scale and metadata per socket for the process lifetime, or reuse metadata from the latest screenshot_via_engine result, so repeated CoordSpace::Points clicks avoid calling get_desktop_state while preserving PNG-coordinate conversion.crates/desktop-use/tests/fixtures/engine_0_17_0/README.md (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code block.
markdownlint reports MD040 for this block.
♻️ Proposed change
-``` +```text cua-driver call --socket <sock> [--screenshot-out-file <path>] <tool> <json-args></details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@crates/desktop-use/tests/fixtures/engine_0_17_0/README.mdaround lines 7 -
9, Add the text language identifier to the fenced code block containing the
cua-driver command in the README, changing the opening fence to use ```text
while leaving the command unchanged.</details> <!-- cr-comment:v1:39c54c2b1e8e6f009c35755d --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>crates/desktop-use/src/highlight.rs (1)</summary><blockquote> `111-129`: _🩺 Stability & Availability_ | _🔵 Trivial_ | _🏗️ Heavy lift_ **Consider shipping a pre-built helper instead of compiling Swift at runtime.** `ensure_helper` invokes `swiftc` on the user machine. `swiftc` is part of the Xcode Command Line Tools and is absent on a stock macOS install. On such machines every highlight call fails with "swiftc not available to build highlight helper", so the operation border never appears. Build the helper during packaging and install it next to the other `resources/bin` binaries, and keep the runtime compile only as a development fallback. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/desktop-use/src/highlight.rs` around lines 111 - 129, Update ensure_helper so packaged installations use a pre-built highlight helper from resources/bin without invoking swiftc. Arrange packaging to build and install that helper alongside the other binaries, while retaining the existing runtime compilation path only as a development fallback when the packaged helper is unavailable. ``` </details> <!-- cr-comment:v1:52810c7cdc57dd894e403efa --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@apps/cli/src/commands/desktop_use.rs:
- Around line 401-402: Update the DriveCommand::Type handling to validate
a.highlight using the same rejecting/error-reporting path as the click command,
instead of unwrap_or(HighlightMode::Auto). Preserve the existing valid
HighlightMode behavior while ensuring invalid --highlight values return the same
clear error as click.In
@apps/desktop-electron/src/branding-paths.ts:
- Around line 59-65: Prioritize hiResPng over pngPath when resolving
dockIconPath in apps/desktop-electron/src/branding-paths.ts lines 59-65, and use
dockIconPath before pngPath in apps/desktop-electron/src/branding.ts lines
114-126. In apps/desktop-electron/src/branding.test.ts lines 54-66, add a
128x128@2x.png alongside icon.png and assert the high-resolution path is
selected.In
@apps/web/src/features/appshot/components/AppshotsHistoryPopover.tsx:
- Around line 452-491: Move the duplicated permission-state mapping from
statusFromDesktopUseDoctor and AppshotPermissionsPanel’s mk helper into a shared
appshot lib helper that accepts translated display labels and recovery-action
text. Update both components to obtain these strings through
useTranslations("appshot.components"), using the existing history.enable wording
where appropriate, and add matching keys/translations to every app locale
including en.json and zh.json; remove the hardcoded user-facing literals from
the components.In
@apps/web/src/features/settings/components/__tests__/desktop-use-settings-section.test.ts:
- Around line 61-63: Update the test covering the DesktopUse section parent to
assert that it does not invoke the desktop_use_grant_permissions IPC, in
addition to verifying DesktopUsePermissionsPanel is rendered. Use the existing
section source or rendered output assertion approach to detect the grant call
and prevent duplicated permission handling in the parent.In
@apps/web/src/features/settings/components/DesktopUseSettingsSection.tsx:
- Around line 147-153: Update the disabled branch in DesktopUseSettingsSection
so it clears only the live border without invoking
desktopInvoke("desktop_use_drive_session_end"), which can terminate active
Desktop Use control. Replace that call with the existing highlight-only cleanup
mechanism while preserving best-effort error handling.- Around line 363-375: Remount DesktopUsePermissionsPanel when the installed
state changes so its mount effect refreshes doctor diagnostics; update
apps/web/src/features/settings/components/DesktopUseSettingsSection.tsx lines
363-375 around DesktopUsePermissionsPanel. Add a regression in
apps/web/src/features/settings/components/tests/desktop-use-settings-section.test.ts
lines 100-111 that changes installed state without closing Settings and verifies
the doctor status refreshes.- Around line 140-146: Update the preference handler around
desktopInvoke("desktop_use_prefs_set") to handle res.ok === false before
applying res.prefs: restore the previous preference in status, clear the error
state, and return early. Preserve the existing successful-response path that
applies returned preferences and the catch-based requery behavior.In
@crates/desktop-use/assets/highlight_overlay.swift:
- Around line 101-103: Update the screen selection expression in the highlight
overlay to remove the NSScreen.screens[0] fallback, retaining NSScreen.main
followed by NSScreen.screens.first and handling the resulting optional safely
without introducing an array-indexing crash.In
@crates/desktop-use/src/control.rs:
- Around line 219-238: Update run_session_end and the corresponding
run_highlight helper to accept the manager passed into drive(), then reuse that
reference for require_engine, socket_path, and host_app_path instead of calling
DesktopUseManager::new(). Update all helper call sites and preserve existing
behavior while ensuring custom manager paths are honored.In
@crates/desktop-use/src/engine_protocol.rs:
- Around line 101-115: Update looks_like_image to accept only recognized image
signatures, removing the bytes.len() > 32 fallback for unknown payloads.
Preserve PNG and JPEG magic checks as appropriate, while ensuring plain-text or
other arbitrary engine output is rejected so screenshot_via_engine cannot report
non-image data as a successful image.In
@crates/desktop-use/src/highlight.rs:
- Around line 17-21: Fix the non-macOS Clippy failures in highlight.rs by
applying #[cfg(target_os = "macos")] to the macOS-only SWIFT_SOURCE constant and
source_newer_than_binary function, and replace the needless return at the
indicated macOS code path with a direct expression while preserving its
behavior.- Around line 409-423: Remove the std::mem::forget(child) call in the
highlight-spawn success path and retain ownership of the Child so it can be
reaped. Update the lifecycle around show_with_args and clear_highlight to wait
on the helper when it exits or is cleared, while preserving the existing PID and
metadata file behavior.In
@crates/desktop-use/src/install.rs:
- Around line 300-313: Update the installer flow containing the macOS plist
updates and codesign invocation to propagate a scrubbed error when both plutil
replacement/insertion attempts fail or when codesign exits unsuccessfully.
Preserve best-effort behavior for LaunchServices refresh and touch, but ensure
the host rebranding operation cannot report success after plist or signing
failure.In
@crates/desktop-use/src/manager.rs:
- Around line 235-246: Update the raw-binary handling around
EngineManifest::embedded so an unverified executable never persists the embedded
manifest’s engine_version as its installed pin. Probe the selected binary for
its actual version before writing installed.json, or persist an unknown version
and ensure status() skips the pinned-version comparison for it; preserve
verified-source pinning behavior.In
@crates/infra/src/utils/system_skill_sync.rs:
- Around line 42-43: Update the repo_skill_root mapping in system_skill_sync.rs
to return skills/atmos-desktop-use for the atmos-desktop-use skill, ensuring
sync_skill_from_raw_github can use the raw-GitHub fallback when bundled and
source-workspace roots are unavailable.In
@specs/APP/APP-052_desktop-use/REVIEW.md:
- Line 17: Refresh the metadata in the review log: update the “next” identifier
to REV-006, and reconcile the Rust test count at the earlier verification entry
with the later recorded result of 29, or explicitly label that entry as an
earlier snapshot. Preserve the zero-padded monotonic REV-NNN format.
Nitpick comments:
In@apps/web/src/features/appshot/components/AppshotPermissionsWindow.tsx:
- Line 13: Update the Button import in AppshotPermissionsWindow.tsx to use the
atomic component path under@workspace/ui/components/ui/*, matching the
established import pattern used by DesktopUseSettingsSection.tsx.In
@apps/web/src/features/appshot/components/AppshotsHistoryPopover.tsx:
- Around line 212-218: Remove the permission watcher setup from
handleOpenPermission, including the watchAppshotStatusAfterPermissionOpen call
and related cleanup state if it is only used by this handler; retain closing the
popover, opening Desktop Use settings, and the existing callback dependencies
needed for those actions.In
@apps/web/src/features/appshot/lib/appshot-client.ts:
- Around line 141-147: Remove the unused _locale parameter and
currentAppLocale() default evaluation from showAppshotPermissionsWindow, then
update all call sites to invoke it without an argument. Keep the existing
dynamic import and openDesktopUseSettingsInApp invocation unchanged.In
@apps/web/src/features/settings/components/DesktopUsePermissionsPanel.tsx:
- Line 9: Update the imports in DesktopUsePermissionsPanel so Button comes from
the atomic component path under@workspace/ui/components/ui/*, while retaining
cn from the existing@workspace/uiutility export.- Around line 99-108: Update the polling flow in DesktopUsePermissionsPanel to
stop the interval immediately when refresh reports both accessibility and
screen_recording as true. Reuse the existing stopPoll function and ensure the
completion check applies to the initial refresh and subsequent interval ticks
while preserving the 45-second timeout.In
@crates/desktop-use/src/control.rs:
- Around line 604-700: The points-coordinate path in to_engine_desktop_xy
currently invokes desktop_png_scale for every click, causing redundant
screenshot captures. Cache the computed desktop PNG scale and metadata per
socket for the process lifetime, or reuse metadata from the latest
screenshot_via_engine result, so repeated CoordSpace::Points clicks avoid
calling get_desktop_state while preserving PNG-coordinate conversion.In
@crates/desktop-use/src/highlight.rs:
- Around line 111-129: Update ensure_helper so packaged installations use a
pre-built highlight helper from resources/bin without invoking swiftc. Arrange
packaging to build and install that helper alongside the other binaries, while
retaining the existing runtime compilation path only as a development fallback
when the packaged helper is unavailable.In
@crates/desktop-use/tests/fixtures/engine_0_17_0/README.md:
- Around line 7-9: Add the text language identifier to the fenced code block
containing the cua-driver command in the README, changing the opening fence to
use ```text while leaving the command unchanged.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `762d84a2-7ac9-4aa7-abff-ee960d87c114` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6a46fe13944d243f5ae7a40bb851502f9a381b41 and 4a5c561d4ac6e9ce63bfd7891159237e7339effb. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `crates/desktop-use/tests/fixtures/engine_0_17_0/tiny.png` is excluded by `!**/*.png` </details> <details> <summary>📒 Files selected for processing (52)</summary> * `AGENTS.md` * `NOTICE` * `apps/cli/src/commands/desktop_use.rs` * `apps/desktop-electron/src/branding-paths.ts` * `apps/desktop-electron/src/branding.test.ts` * `apps/desktop-electron/src/branding.ts` * `apps/desktop-electron/src/desktop-use/client.ts` * `apps/desktop-electron/src/desktop-use/host-capture.test.ts` * `apps/desktop-electron/src/desktop-use/host-capture.ts` * `apps/desktop-electron/src/ipc/handlers.ts` * `apps/web/messages/en.json` * `apps/web/messages/zh.json` * `apps/web/src/features/appshot/__tests__/appshots-history-popover.test.tsx` * `apps/web/src/features/appshot/components/AppshotPermissionsPanel.tsx` * `apps/web/src/features/appshot/components/AppshotPermissionsWindow.tsx` * `apps/web/src/features/appshot/components/AppshotsHeaderButton.tsx` * `apps/web/src/features/appshot/components/AppshotsHistoryPopover.tsx` * `apps/web/src/features/appshot/lib/appshot-client.ts` * `apps/web/src/features/appshot/lib/open-desktop-use-settings.ts` * `apps/web/src/features/settings/components/DesktopUsePermissionsPanel.tsx` * `apps/web/src/features/settings/components/DesktopUseSettingsSection.tsx` * `apps/web/src/features/settings/components/__tests__/desktop-use-settings-section.test.ts` * `apps/web/src/features/settings/components/settings-modal-sidebar.tsx` * `apps/web/src/features/terminal/components/TerminalAgentInputOverlay.tsx` * `apps/web/src/features/welcome/components/SlashCommandPopover.tsx` * `apps/web/src/features/welcome/components/WelcomePage.tsx` * `apps/web/src/features/welcome/hooks/use-welcome-slash-search.ts` * `apps/web/src/features/welcome/lib/__tests__/slash-desktop-use.test.ts` * `apps/web/src/features/welcome/lib/slash-desktop-use.ts` * `crates/desktop-use/assets/highlight_overlay.swift` * `crates/desktop-use/assets/host-app-icon.icns` * `crates/desktop-use/src/control.rs` * `crates/desktop-use/src/doctor.rs` * `crates/desktop-use/src/engine_protocol.rs` * `crates/desktop-use/src/highlight.rs` * `crates/desktop-use/src/host.rs` * `crates/desktop-use/src/install.rs` * `crates/desktop-use/src/lib.rs` * `crates/desktop-use/src/manager.rs` * `crates/desktop-use/src/prefs.rs` * `crates/desktop-use/tests/fixtures/engine_0_17_0/README.md` * `crates/desktop-use/tests/fixtures/engine_0_17_0/get_desktop_state_fail_plain_text.txt` * `crates/desktop-use/tests/fixtures/engine_0_17_0/get_desktop_state_success_file_path.json` * `crates/desktop-use/tests/fixtures/engine_0_17_0/get_desktop_state_success_mcp_image.json` * `crates/infra/src/utils/system_skill_sync.rs` * `packages/ui/src/components/icons/desktop-use-icon-static.tsx` * `packages/ui/src/components/icons/desktop-use-icon.tsx` * `skills/atmos-desktop-use/SKILL.md` * `skills/atmos-desktop-use/references/cli.md` * `skills/system-skills-manifest.json` * `specs/APP/APP-052_desktop-use/REVIEW.md` * `specs/APP/APP-052_desktop-use/TECH.md` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (4)</summary> * specs/APP/APP-052_desktop-use/TECH.md * apps/desktop-electron/src/desktop-use/host-capture.test.ts * crates/desktop-use/src/doctor.rs * apps/desktop-electron/src/desktop-use/host-capture.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| // Dock: prefer high-res PNG. Electron's nativeImage.createFromPath(.icns) | ||
| // often loads only a low-res representation, which then appears as a tiny | ||
| // glyph centered in the macOS Dock tile. Bundle CFBundleIconFile can still | ||
| // use .icns for Finder / Get Info. | ||
| const dockIconPath = | ||
| platform === "darwin" | ||
| ? icnsPath ?? pngPath ?? hiResPng | ||
| ? pngPath ?? hiResPng ?? icnsPath |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor the high-resolution Dock icon priority.
hiResPng resolves the high-resolution assets before icon.png. Both the resolver and the Dock application path select icon.png first, so macOS receives the regular PNG when both files exist.
apps/desktop-electron/src/branding-paths.ts#L59-L65: selecthiResPngbefore the regular PNG fordockIconPath.apps/desktop-electron/src/branding.ts#L114-L126: usedockIconPathbeforepngPath.apps/desktop-electron/src/branding.test.ts#L54-L66: create128x128@2x.pngalongsideicon.pngand assert that the high-resolution path wins.
Proposed fix
- ? pngPath ?? hiResPng ?? icnsPath
+ ? hiResPng ?? icnsPath
: windowIconPath;- const dockPath =
- icons.pngPath ?? icons.dockIconPath ?? icons.windowIconPath;
+ const dockPath = icons.dockIconPath ?? icons.windowIconPath;The supplied change detail specifies high-resolution PNG before regular PNG.
📍 Affects 3 files
apps/desktop-electron/src/branding-paths.ts#L59-L65(this comment)apps/desktop-electron/src/branding.ts#L114-L126apps/desktop-electron/src/branding.test.ts#L54-L66
🤖 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 `@apps/desktop-electron/src/branding-paths.ts` around lines 59 - 65, Prioritize
hiResPng over pngPath when resolving dockIconPath in
apps/desktop-electron/src/branding-paths.ts lines 59-65, and use dockIconPath
before pngPath in apps/desktop-electron/src/branding.ts lines 114-126. In
apps/desktop-electron/src/branding.test.ts lines 54-66, add a 128x128@2x.png
alongside icon.png and assert the high-resolution path is selected.
| /** Map Desktop Use doctor → AppshotStatus shape for denied-permission UI. */ | ||
| function statusFromDesktopUseDoctor(doctor: { | ||
| accessibility?: boolean | null; | ||
| screen_recording?: boolean | null; | ||
| }): AppshotStatus { | ||
| const ax = doctor.accessibility === true; | ||
| const screen = doctor.screen_recording === true; | ||
| const mk = ( | ||
| name: "accessibility" | "screen_recording", | ||
| granted: boolean, | ||
| ): AppshotPermissionState => ({ | ||
| name, | ||
| display_name: name === "accessibility" ? "Accessibility" : "Screen Recording", | ||
| granted, | ||
| required_for: | ||
| name === "accessibility" | ||
| ? ["accessibility_tree", "control"] | ||
| : ["capture", "control"], | ||
| recovery_action: granted | ||
| ? null | ||
| : { | ||
| label: "Open Desktop Use settings", | ||
| target: name, | ||
| manual_steps: [], | ||
| }, | ||
| }); | ||
| return { | ||
| supported: true, | ||
| platform: "macos", | ||
| reason: null, | ||
| trigger: { | ||
| mode: "macos_modifier_gesture", | ||
| enabled: ax, | ||
| required_modifiers: [], | ||
| last_error: null, | ||
| permissions: [mk("accessibility", ax)], | ||
| }, | ||
| permissions: [mk("accessibility", ax), mk("screen_recording", screen)], | ||
| }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Localize the permission labels and reuse one mapping helper.
statusFromDesktopUseDoctor hardcodes "Accessibility", "Screen Recording", and "Open Desktop Use settings". apps/web/src/features/appshot/components/AppshotPermissionsPanel.tsx contains a near-identical mk helper with the same literals, so the mapping is duplicated in two feature components. The literal CTA text also diverges from appshot.components.history.enable ("Open Desktop Use") in apps/web/messages/en.json, and apps/web/messages/zh.json has no matching Chinese text for these literals.
Move the mapping into a shared helper under apps/web/src/features/appshot/lib/ and pass translated strings from useTranslations("appshot.components").
As per coding guidelines, "Avoid hardcoded user-facing copy in web components" and "When changing user-facing UI text, update the matching keys in every locale file for that app".
🤖 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 `@apps/web/src/features/appshot/components/AppshotsHistoryPopover.tsx` around
lines 452 - 491, Move the duplicated permission-state mapping from
statusFromDesktopUseDoctor and AppshotPermissionsPanel’s mk helper into a shared
appshot lib helper that accepts translated display labels and recovery-action
text. Update both components to obtain these strings through
useTranslations("appshot.components"), using the existing history.enable wording
where appropriate, and add matching keys/translations to every app locale
including en.json and zh.json; remove the hardcoded user-facing literals from
the components.
Source: Coding guidelines
| // Engine card does not call grant; permissions panel owns it | ||
| expect(section).toContain("DesktopUsePermissionsPanel"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the parent does not invoke the grant IPC.
The test only checks that DesktopUsePermissionsPanel is rendered. It still passes if the parent later adds desktop_use_grant_permissions and duplicates the permission flow.
Proposed test update
expect(section).toContain("DesktopUsePermissionsPanel");
+ expect(section).not.toContain("desktop_use_grant_permissions");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Engine card does not call grant; permissions panel owns it | |
| expect(section).toContain("DesktopUsePermissionsPanel"); | |
| }); | |
| // Engine card does not call grant; permissions panel owns it | |
| expect(section).toContain("DesktopUsePermissionsPanel"); | |
| expect(section).not.toContain("desktop_use_grant_permissions"); | |
| }); |
🤖 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
`@apps/web/src/features/settings/components/__tests__/desktop-use-settings-section.test.ts`
around lines 61 - 63, Update the test covering the DesktopUse section parent to
assert that it does not invoke the desktop_use_grant_permissions IPC, in
addition to verifying DesktopUsePermissionsPanel is rendered. Use the existing
section source or rendered output assertion approach to detect the grant call
and prevent duplicated permission handling in the parent.
| const res = await desktopInvoke<{ | ||
| ok: boolean; | ||
| prefs: DesktopUsePrefs; | ||
| }>("desktop_use_prefs_set", { operationBorder: enabled }); | ||
| if (res?.prefs) { | ||
| setStatus((prev) => (prev ? { ...prev, prefs: res.prefs } : prev)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Verify whether failed preference mutations reject or resolve with `ok: false`.
rg -n -C 12 --glob '*.{ts,tsx,rs}' \
'desktop_use_prefs_set|operationBorder|operation_border_enabled' apps cratesRepository: AruNi-01/atmos
Length of output: 37906
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the Rust CLI path for prefs set and the web component error handling.
sed -n '1,180p' crates/desktop-use/src/bin/main.py 2>/dev/null || rg -n -C 8 --glob '*.{py,rs,ts,tsx}' \
'prefs set|DesktopUsePrefs|update_prefs|result_json|json.loads|json.dumps|ok' crates/desktop-use apps/desktop-electron apps/web/src/features/settings/components/DesktopUseSettingsSection.tsx
# List desktop-use executable CLI entrypoints.
git ls-files crates/desktop-use | sed 's#^##' | rg 'bin|main|prefs|prefs|prefs' || trueRepository: AruNi-01/atmos
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== files matching desktop-use cli/entrypoint/prefs =="
git ls-files | rg 'crates/desktop-use|apps/desktop-electron/src/desktop-use|apps/web/src/features/settings/components/DesktopUseSettingsSection.tsx'
echo
echo "== desktop-use cli command parser =="
rg -n -C 8 --glob '*.{rs,py,ts,tsx}' 'prefs|DesktopUsePrefs|update_prefs|main\(|Command|Subcommand|atmos-de' crates/desktop-use apps/desktop-electron/src/desktop-use apps/web/src/features/settings/components/DesktopUseSettingsSection.tsx
echo
echo "== focused main.rs around prefs =="
MAIN="$(git ls-files | rg '^crates/desktop-use/src/main\.rs$|^crates/desktop-use/src/.*/main\.rs$' | head -n1 || true)"
if [ -n "${MAIN:-}" ]; then
wc -l "$MAIN"
sed -n '1,220p' "$MAIN"
fi
echo
echo "== focused settings section error handling =="
sed -n '120,175p' apps/web/src/features/settings/components/DesktopUseSettingsSection.tsxRepository: AruNi-01/atmos
Length of output: 50370
Reapply the previous preference when prefs.set resolves with ok: false.
crates/desktop-use/src/prefs.rs::update_prefs returns Result on Rust, but apps/desktop-electron/src/desktop-use/client.ts::desktopUsePrefsSet still resolves an "ok": false payload if the wrapper does not reject. If the CLI sends that shape, this handler leaves the optimistic UI changed and only requeries data in the catch. Add a guard for res.ok === false that resets status, clears the error, and exits before applying the returned prefs.
🤖 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 `@apps/web/src/features/settings/components/DesktopUseSettingsSection.tsx`
around lines 140 - 146, Update the preference handler around
desktopInvoke("desktop_use_prefs_set") to handle res.ok === false before
applying res.prefs: restore the previous preference in status, clear the error
state, and return early. Preserve the existing successful-response path that
applies returned preferences and the catch-based requery behavior.
| // Ad-hoc sign only when branding bits changed. | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| let _ = Command::new("codesign") | ||
| .args(["--force", "--deep", "-s", "-"]) | ||
| .arg(dest) | ||
| .status(); | ||
| // Refresh LaunchServices name + icon (best-effort). | ||
| let _ = Command::new("/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Support/lsregister") | ||
| .args(["-f"]) | ||
| .arg(dest) | ||
| .status(); | ||
| // Touch bundle so Finder/Dock icon cache notices the change. | ||
| let _ = Command::new("touch").arg(dest).status(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return an error when host rebranding fails.
If plutil -replace and plutil -insert both fail, this function ignores the failure. It also ignores a failed codesign command after plist or icon changes. The installer can then report success while the host keeps an incorrect or invalid identity. Permission grants for Atmos Desktop Use cannot apply to that host.
Return a scrubbed error when plist insertion or re-signing fails.
Also applies to: 387-391
🤖 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 `@crates/desktop-use/src/install.rs` around lines 300 - 313, Update the
installer flow containing the macOS plist updates and codesign invocation to
propagate a scrubbed error when both plutil replacement/insertion attempts fail
or when codesign exits unsuccessfully. Preserve best-effort behavior for
LaunchServices refresh and touch, but ensure the host rebranding operation
cannot report success after plist or signing failure.
| // Record pin so status can detect future updates. | ||
| if let Ok(manifest) = EngineManifest::embedded() { | ||
| let meta = serde_json::json!({ | ||
| "engine_version": manifest.engine_version, | ||
| "host_app_name": manifest.host_app_name, | ||
| "host_bundle_id": manifest.host_bundle_id, | ||
| "source": "raw_binary", | ||
| }); | ||
| let _ = fs::write( | ||
| self.data_dir.join("installed.json"), | ||
| serde_json::to_vec_pretty(&meta).unwrap_or_default(), | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not record the manifest pin for an unverified raw binary.
source_path and ATMOS_DESKTOP_USE_ENGINE_SOURCE can select any executable. This code writes the pinned manifest version as engine_version regardless. status() then reports that version and sets update_available to false because both values match. The Settings UI can show an arbitrary binary as pinned v0.17.0.
Probe and persist the actual binary version, or mark raw-binary versions as unknown and avoid the pinned-version comparison.
🤖 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 `@crates/desktop-use/src/manager.rs` around lines 235 - 246, Update the
raw-binary handling around EngineManifest::embedded so an unverified executable
never persists the embedded manifest’s engine_version as its installed pin.
Probe the selected binary for its actual version before writing installed.json,
or persist an unknown version and ensure status() skips the pinned-version
comparison for it; preserve verified-source pinning behavior.
| // Desktop capture / drive (APP-052) | ||
| "atmos-desktop-use", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'fn repo_skill_root|fn sync_skill_from_raw_github|fn sync_skill_from_available_sources|atmos-desktop-use' \
crates/infra/src/utils/system_skill_sync.rsRepository: AruNi-01/atmos
Length of output: 3734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the new entry, raw-GitHub path resolution, and the skill-source fallback chain.
sed -n '1,90p;140,200p;395,440p;544,575p' crates/infra/src/utils/system_skill_sync.rs
# Programmatically model the reported source-flow invariant:
# - atmos-desktop-use is an inventory member,
# - repo_skill_root("atmos-desktop-use") returns None,
# - sync_skill_from_raw_github rejects None,
# - sync_skill_from_available_sources only returns true when it calls sync_skill_from_root and that returns true.
python3 - <<'PY'
from pathlib import Path
import ast, sys
src = Path("crates/infra/src/utils/system_skill_sync.rs").read_text()
tree = ast.parse(src)
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name == "repo_skill_root":
start = node.lineno
end = node.end_lineno
func = "\n".join(src.splitlines()[start - 1:end])
print("\n---- repo_skill_root ----")
print(func)
print("contains atmos-desktop-use mapping:", '"atmos-desktop-use"' in func)
if isinstance(node, ast.FunctionDef) and node.name == "sync_skill_from_raw_github":
start = node.lineno
end = node.end_lineno
print("\n---- sync_skill_from_raw_github start ----")
lines = src.splitlines()
for i in range(start - 1, min(end, start + 20)):
print(f"{i+1}: {lines[i]}")
text = "\n".join(lines[start - 1:min(end, start + 20)])
print("raw function checks repo_skill_root before proceeding:", "repo_skill_root(skill_name)" in text and "let Some(skill_root) = repo_skill_root(skill_name)" in text)
if isinstance(node, ast.FunctionDef) and node.name == "sync_skill_from_available_sources":
print("\n--- sync_skill_from_available_sources outline ---")
lines = src.splitlines()
for i in range(node.lineno - 1, node.end_lineno):
if "atmos-desktop-use" in lines[i] or "sync_skill_from_root(" in lines[i] or "return true" in lines[i]:
print(f"{i+1}: {lines[i]}")
PYRepository: AruNi-01/atmos
Length of output: 8611
Add the raw-source mapping for atmos-desktop-use.
atmos-desktop-use is added to the sync inventory but repo_skill_root still returns None, so sync_skill_from_raw_github logs “No raw GitHub skill root mapping” and skips raw-GitHub sync. If the bundled or source-workspace skill roots are unavailable, the raw fallback cannot download the skill; add skills/atmos-desktop-use to repo_skill_root.
🤖 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 `@crates/infra/src/utils/system_skill_sync.rs` around lines 42 - 43, Update the
repo_skill_root mapping in system_skill_sync.rs to return
skills/atmos-desktop-use for the atmos-desktop-use skill, ensuring
sync_skill_from_raw_github can use the raw-GitHub fallback when bundled and
source-workspace roots are unavailable.
| | Rule | Detail | | ||
| |------|--------| | ||
| | **When to add** | After code implementation reaches review and findings need durable tracking. | | ||
| | **Entry id** | `REV-NNN` - zero-padded, monotonic in this file (next: **REV-004**). | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refresh the review log metadata.
Line 17 says the next ID is REV-004, but the index already contains REV-004 and REV-005. The next ID is REV-006.
Line 29 reports 26 Rust tests, but Line 185 records 29 tests passed on August 4, 2026. Update both values or label Line 29 as an earlier snapshot.
As per coding guidelines, specs/APP/APP-052_desktop-use/REVIEW.md must use zero-padded monotonic REV-NNN identifiers and record current verification details.
Also applies to: 29-29
🤖 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 `@specs/APP/APP-052_desktop-use/REVIEW.md` at line 17, Refresh the metadata in
the review log: update the “next” identifier to REV-006, and reconcile the Rust
test count at the earlier verification entry with the later recorded result of
29, or explicitly label that entry as an earlier snapshot. Preserve the
zero-padded monotonic REV-NNN format.
Source: Coding guidelines
…atched chrome - Add atmos browser-use crate/CLI/skill for page CDP (CUA external + embedded stub) - Expand desktop drive tools (token/index click, window-state bounds, phases 1–2) - Classify window AX surfaces (ax_ok/empty/heavy) with agent ladder guidance - Match operation border color to session agent-cursor palette - Document Electron shells as Desktop Use pixel path, not browser-use
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
skills/atmos-desktop-use/references/cli.md (2)
1-64: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign
desktop-use drivedocs with the nested command structure.
DriveCommanddefines nested subcommands such asdrive screenshot,drive click,drive type,drive window-state, and clipboard. Thedrive clickdocs should include--element-token, because that is the preferred background path instead of--coord-space/--delivery-mode. The current examples look like flat flags and miss the primary element-click contract.🤖 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 `@skills/atmos-desktop-use/references/cli.md` around lines 1 - 64, Update the drive section of the CLI reference to reflect DriveCommand’s nested subcommands, including screenshot, click, type, window-state, and clipboard operations. Revise the drive click example to show the preferred --element-token usage and document it as the primary background interaction path, while retaining coordinate and delivery options only as applicable alternatives.
144-157: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEnsure the documented data directory is accurate for other platforms.
~/.atmos/desktop-use/matches the macOS/desktop-use home path, but this path only applies if the shared Runtime API is used on non-Darwin platforms. Do not present it as the universal default unless the Runtime API enforces this path there.🤖 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 `@skills/atmos-desktop-use/references/cli.md` around lines 144 - 157, The “Paths” section currently presents ~/.atmos/desktop-use/ as universal; revise the documentation to state that this directory applies to macOS/desktop-use or when using the shared Runtime API on non-Darwin platforms, unless the Runtime API enforces it universally. Clarify the platform-specific behavior without changing the listed path contents or Offline / CI variables.
♻️ Duplicate comments (1)
specs/APP/APP-052_desktop-use/TECH.md (1)
31-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale capture-identity sections.
Line 32 correctly locks installed-engine AppShot capture to the host engine. Sections 4 and 8 still state that AppShot runs in-process in Electron. Update those sections to state that the host engine handles production capture after installation, with Electron
capture.tslimited to the pre-ensure fallback.🤖 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 `@specs/APP/APP-052_desktop-use/TECH.md` around lines 31 - 32, Sections 4 and 8 in the specifications contain stale information incorrectly stating that AppShot runs in-process in Electron. Update both sections to accurately reflect that the host engine handles AppShot capture after installation, with Electron capture.ts serving only as the pre-ensure fallback. Align these sections with the correct behavior already documented in lines 31-32, which correctly identify that installed-engine AppShot uses the host engine (not Electron in-process capture) for production use.Source: Coding guidelines
🧹 Nitpick comments (5)
crates/browser-use/src/types.rs (1)
10-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider dropping the
ExternalandAtmosalias variants.
parsenever returnsExternalorAtmos. It maps the alias strings directly toCuaandEmbeddedat Lines 27-28. The two extra variants only add match arms, andlib.rsLines 23-28 must keep pairing them. Keep the alias strings inparseand reduce the enum toCuaandEmbedded. Deserialization of the alias names still needs support, so add#[serde(alias = "external")]and#[serde(alias = "atmos")]if the JSON surface must accept them.Based on learnings: implement only the minimum requested solution; avoid speculative features, single-use abstractions, and unnecessary configurability.
♻️ Proposed enum reduction
pub enum BrowserBackendKind { /// System Chromium via managed control engine (CUA tools). Default. #[default] + #[serde(alias = "external")] Cua, - /// Alias for Cua. - External, /// Atmos in-app browser (APP-053 webview) — stub until PR `#203` merges. + #[serde(alias = "atmos")] Embedded, - /// Alias for Embedded. - Atmos, }Then simplify the dispatch in
crates/browser-use/src/lib.rs:pub fn execute(req: BrowserRequest) -> BrowserResult { match req.backend { BrowserBackendKind::Cua => CuaExternalBackend::default().execute(req), BrowserBackendKind::Embedded => EmbeddedBackend::default().execute(req), } }🤖 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 `@crates/browser-use/src/types.rs` around lines 10 - 32, Remove the unused External and Atmos variants from BrowserBackendKind, keeping Cua and Embedded as the only enum variants while preserving all existing alias strings in BrowserBackendKind::parse. Add serde aliases for external and atmos if deserialization must continue accepting those names, and update the execute dispatch in lib.rs to match only Cua and Embedded.Source: Learnings
crates/browser-use/src/lib.rs (1)
220-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
no_mcp_in_error_stringstest the stated property, or remove it.The test asserts that a literal constant contains its own substring. It never inspects an error string, so the name does not match the behavior. Either assert the real rule against produced results, or delete the test.
💚 Proposed test that checks produced error text
#[test] fn no_mcp_in_error_strings() { - let _ = json!({ "note": ERR_NO_MCP }); - assert!(ERR_NO_MCP.contains("MCP")); + let res = execute(BrowserRequest { + backend: BrowserBackendKind::Embedded, + action: BrowserAction::Navigate, + ..Default::default() + }); + let text = serde_json::to_string(&res).unwrap(); + assert!(!text.to_ascii_lowercase().contains("mcp")); }
json!then becomes unused in the test module, so drop theuse serde_json::json;import at Line 42 if no other test needs it.🤖 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 `@crates/browser-use/src/lib.rs` around lines 220 - 224, Update or remove no_mcp_in_error_strings: if retained, exercise the relevant error-producing path and assert its generated error text satisfies the intended MCP rule instead of checking ERR_NO_MCP.contains("MCP"). Remove the serde_json::json import if it becomes unused.apps/cli/src/commands/desktop_use.rs (2)
875-889: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo copies of the engine 0.17 pid rule can drift apart.
click_likeenforces the pid requirement fordouble-clickandright-clickhere, andbuild_engine_callenforces the same rule again (crates/desktop-use/src/drive_tools.rsLines 183-191 and 204-206). The two error strings already differ: "double-click/right-click require --pid (engine 0.17 has no desktop-scope path)" here, and "{tool} requires --pid (engine 0.17)" in the crate.Engine capability rules belong in the
desktop-usecrate, which is also the path used by thebrowser-useCUA backend. Keep the authoritative check inbuild_engine_calland reduce this block to the CLI-specific concern, which is stripping a bare--pidso screen-absolute clicks stay in desktop scope.As per coding guidelines: "Do not duplicate
core-servicebusiness rules in the CLI."🤖 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 `@apps/cli/src/commands/desktop_use.rs` around lines 875 - 889, Remove the duplicated engine 0.17 PID validation from click_like, including the needs_pid check and its error return. Keep build_engine_call as the authoritative capability check, while preserving the CLI-specific logic that retains a PID for element/window actions and strips a bare PID for screen-absolute clicks.Source: Coding guidelines
115-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the internal roadmap labels from user-facing help text.
The doc comments on these variants become the
--helpdescriptions foratmos desktop-use drive. "(Phase 1)" and "(Phase 2)" describe internal delivery planning, and a CLI user cannot act on them. Describe the behavior instead, for example "Right-click at coordinates or an AX element". Keep the useful qualifier onFront, which explains that fronting is explicit only.🤖 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 `@apps/cli/src/commands/desktop_use.rs` around lines 115 - 173, Remove all “(Phase 1)” and “(Phase 2)” roadmap labels from the enum variant doc comments in the desktop-use command, replacing them with concise user-facing behavior descriptions such as coordinate or AX-element targets where applicable. Preserve the meaningful “explicit only” qualifier on Front and keep each command’s help text focused on its action.crates/desktop-use/src/drive_tools.rs (1)
423-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the exported policy functions.
The module doc states these builders are unit-tested without a daemon.
wants_pre_move_cursorandwants_action_highlightare public policies thatrun_enginecalls to decide cursor pre-movement and highlight chrome, but no test covers them. Add cases for the element-targeted path, thewindow_id-present path, and each non-interactive action in the highlight exclusion list. These tests also pin thepidquestion raised above.🤖 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 `@crates/desktop-use/src/drive_tools.rs` around lines 423 - 427, Add unit tests in the existing tests module for the exported wants_pre_move_cursor and wants_action_highlight policies. Cover element-targeted requests, requests with window_id present, and every non-interactive action excluded from highlighting, including assertions that pin the expected pid behavior; keep the tests daemon-free and reuse existing DriveRequest builders or fixtures.
🤖 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 `@apps/cli/src/commands/browser_use.rs`:
- Around line 78-79: Update the `Type` command help text to state that the bound
tab reference is required, matching the required `TypeArgs.element_ref` field
and backend contract. Change only the misleading “optional ref” wording while
preserving the command’s existing behavior.
In `@apps/cli/src/commands/desktop_use.rs`:
- Around line 423-428: Enforce the element_index, snapshot_id, and window_id
pairing for type and set-value before forwarding the request. Update the
validation used by drive_cmd or build_engine_call, alongside the existing
click_like check, so Type and SetValue reject a lone or incomplete
--element-index combination with the established CLI validation behavior.
- Around line 719-730: Update the DriveCommand::Menu parsing flow to validate
that the deserialized serde_json::Value is specifically an array before
constructing the DriveRequest. Preserve the existing “menu --path must be JSON
array” argument error for both invalid JSON and valid non-array values, and only
pass array values as menu_path.
In `@crates/browser-use/src/backends/cua.rs`:
- Line 226: Update the call path around host::call_tool in the browser backend
to enforce a bounded timeout for engine invocations, using the existing
host/engine command mechanism and propagating timeout failures through the
current result handling. Ensure blocked call subcommands cannot leave browser
actions hanging indefinitely.
- Around line 40-66: Update the strategy handling in the request-building match
to compare values exactly rather than using substring checks: accept only
existing_profile, isolated_new, and isolated_named, preserving their current
payload behavior. For any other Some(strategy) value, return an error instead of
proceeding without a strategy; leave the None detect-only behavior unchanged.
In `@crates/desktop-use/src/engine_protocol.rs`:
- Around line 27-47: Update the refusal branch in the engine failure parser to
require that the `refusal` value exists and is non-null before extracting its
message or code. Preserve the existing detail-selection and return behavior for
actual refusal data, while allowing `"refusal": null` payloads to continue to
subsequent success/code handling.
In `@crates/desktop-use/src/window_surface.rs`:
- Around line 104-121: Update classify_surface so degraded payloads with nonzero
element counts follow the intended protocol decision: classify them as AxEmpty
if degraded trees are unusable, or remove the redundant degraded condition and
add a test covering a degraded/ax_window_unresolved payload with nonzero
elements if they remain usable. Preserve existing count-based classifications
for valid non-degraded payloads.
In `@crates/infra/src/utils/system_skill_sync.rs`:
- Line 44: Update repo_skill_root to add raw-source mappings for both
atmos-browser-use and atmos-desktop-use. Ensure
sync_skill_from_available_sources can resolve each name through
sync_skill_from_raw_github when bundled or workspace sources are unavailable,
while preserving existing mappings.
In `@skills/atmos-browser-use/SKILL.md`:
- Around line 59-64: Add the control_engine_failed entry to the error-code table
in SKILL.md, documenting that it indicates host::ensure_daemon failure such as
missing macOS Accessibility or Screen Recording permissions and directing agents
to inspect the result and resolve the required permissions.
In `@skills/atmos-desktop-use/references/cli.md`:
- Around line 88-91: Update the command examples around the desktop-use drive
subcommands to avoid Bash pipeline notation: represent alternatives in comments
or provide separate executable command lines, while preserving the documented
subcommand choices and JSON usage.
In `@skills/atmos-desktop-use/SKILL.md`:
- Around line 101-106: Make the Electron fallback guidance consistent with the
surface table: update the “ax_empty / electron_likely:true” rule in the
desktop-use instructions so pixel-only handling applies only to ax_empty, while
ax_sparse continues permitting a matching element_token unless the table is
intentionally changed to make Electron detection override it.
- Line 33: Update the “Click / type / full desktop shell” row in the decision
table to prevent the literal command separators in the drive shorthand from
being parsed as Markdown column delimiters; replace them with comma-separated
command names or escape each pipe while preserving the command list meaning.
In `@specs/APP/APP-052_desktop-use/TECH.md`:
- Around line 137-146: The pipe character in `clipboard get|set` within the
Phase 1 row of the desktop drive phases table is being interpreted as a Markdown
table column separator instead of part of the command text, causing MD038 and
MD056 warnings and incorrect rendering. Escape the pipe character in the Phase 1
command list by replacing `clipboard get|set` with `clipboard get\|set` so the
pipe is treated as literal text rather than a table delimiter.
---
Outside diff comments:
In `@skills/atmos-desktop-use/references/cli.md`:
- Around line 1-64: Update the drive section of the CLI reference to reflect
DriveCommand’s nested subcommands, including screenshot, click, type,
window-state, and clipboard operations. Revise the drive click example to show
the preferred --element-token usage and document it as the primary background
interaction path, while retaining coordinate and delivery options only as
applicable alternatives.
- Around line 144-157: The “Paths” section currently presents
~/.atmos/desktop-use/ as universal; revise the documentation to state that this
directory applies to macOS/desktop-use or when using the shared Runtime API on
non-Darwin platforms, unless the Runtime API enforces it universally. Clarify
the platform-specific behavior without changing the listed path contents or
Offline / CI variables.
---
Duplicate comments:
In `@specs/APP/APP-052_desktop-use/TECH.md`:
- Around line 31-32: Sections 4 and 8 in the specifications contain stale
information incorrectly stating that AppShot runs in-process in Electron. Update
both sections to accurately reflect that the host engine handles AppShot capture
after installation, with Electron capture.ts serving only as the pre-ensure
fallback. Align these sections with the correct behavior already documented in
lines 31-32, which correctly identify that installed-engine AppShot uses the
host engine (not Electron in-process capture) for production use.
---
Nitpick comments:
In `@apps/cli/src/commands/desktop_use.rs`:
- Around line 875-889: Remove the duplicated engine 0.17 PID validation from
click_like, including the needs_pid check and its error return. Keep
build_engine_call as the authoritative capability check, while preserving the
CLI-specific logic that retains a PID for element/window actions and strips a
bare PID for screen-absolute clicks.
- Around line 115-173: Remove all “(Phase 1)” and “(Phase 2)” roadmap labels
from the enum variant doc comments in the desktop-use command, replacing them
with concise user-facing behavior descriptions such as coordinate or AX-element
targets where applicable. Preserve the meaningful “explicit only” qualifier on
Front and keep each command’s help text focused on its action.
In `@crates/browser-use/src/lib.rs`:
- Around line 220-224: Update or remove no_mcp_in_error_strings: if retained,
exercise the relevant error-producing path and assert its generated error text
satisfies the intended MCP rule instead of checking ERR_NO_MCP.contains("MCP").
Remove the serde_json::json import if it becomes unused.
In `@crates/browser-use/src/types.rs`:
- Around line 10-32: Remove the unused External and Atmos variants from
BrowserBackendKind, keeping Cua and Embedded as the only enum variants while
preserving all existing alias strings in BrowserBackendKind::parse. Add serde
aliases for external and atmos if deserialization must continue accepting those
names, and update the execute dispatch in lib.rs to match only Cua and Embedded.
In `@crates/desktop-use/src/drive_tools.rs`:
- Around line 423-427: Add unit tests in the existing tests module for the
exported wants_pre_move_cursor and wants_action_highlight policies. Cover
element-targeted requests, requests with window_id present, and every
non-interactive action excluded from highlighting, including assertions that pin
the expected pid behavior; keep the tests daemon-free and reuse existing
DriveRequest builders or fixtures.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b268addc-91c9-4f5c-9d6d-cb485928d17d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
apps/cli/Cargo.tomlapps/cli/src/commands/browser_use.rsapps/cli/src/commands/desktop_use.rsapps/cli/src/commands/mod.rsapps/cli/src/main.rsapps/cli/src/output.rscrates/browser-use/Cargo.tomlcrates/browser-use/src/backends/cua.rscrates/browser-use/src/backends/embedded.rscrates/browser-use/src/backends/mod.rscrates/browser-use/src/lib.rscrates/browser-use/src/types.rscrates/desktop-use/assets/highlight_overlay.swiftcrates/desktop-use/src/control.rscrates/desktop-use/src/drive_tools.rscrates/desktop-use/src/engine_protocol.rscrates/desktop-use/src/highlight.rscrates/desktop-use/src/lib.rscrates/desktop-use/src/window_surface.rscrates/infra/src/utils/system_skill_sync.rsskills/atmos-browser-use/SKILL.mdskills/atmos-desktop-use/SKILL.mdskills/atmos-desktop-use/references/cli.mdskills/system-skills-manifest.jsonspecs/APP/APP-052_desktop-use/TECH.md
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/desktop-use/assets/highlight_overlay.swift
- skills/system-skills-manifest.json
- crates/desktop-use/src/lib.rs
- crates/desktop-use/src/highlight.rs
- crates/desktop-use/src/control.rs
| /// Type into the bound tab (optional ref). | ||
| Type(TypeArgs), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the Type help text: --ref is required.
The help text says the ref is optional. TypeArgs.element_ref at Line 147 is a required String, and crates/browser-use/src/backends/cua.rs Line 129 also requires it. The text misleads users of atmos browser-use type --help.
📝 Proposed help-text fix
- /// Type into the bound tab (optional ref).
+ /// Type text into a page element by ref in a bound tab.
Type(TypeArgs),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Type into the bound tab (optional ref). | |
| Type(TypeArgs), | |
| /// Type text into a page element by ref in a bound tab. | |
| Type(TypeArgs), |
🤖 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 `@apps/cli/src/commands/browser_use.rs` around lines 78 - 79, Update the `Type`
command help text to state that the bound tab reference is required, matching
the required `TypeArgs.element_ref` field and backend contract. Change only the
misleading “optional ref” wording while preserving the command’s existing
behavior.
| /// AX element index (requires --snapshot-id + --window-id). Prefer --element-token. | ||
| #[arg(long)] | ||
| pub element_index: Option<i32>, | ||
| /// Snapshot id from `drive window-state` (with --element-index). | ||
| #[arg(long)] | ||
| pub snapshot_id: Option<String>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce the documented --element-index pairing for type.
The help text states that --element-index requires --snapshot-id and --window-id. Nothing enforces this. drive_cmd forwards all three fields as-is (Lines 745-746), and build_engine_call validates the pairing only for the click actions (crates/desktop-use/src/drive_tools.rs Lines 200-211). For type it calls inject_element unconditionally, so a lone --element-index reaches the engine and returns a generic soft failure.
Validate the combination in the CLI, as click_like already does, or extend the check in build_engine_call to Type and SetValue. The same gap applies to set-value at Lines 807-817.
🤖 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 `@apps/cli/src/commands/desktop_use.rs` around lines 423 - 428, Enforce the
element_index, snapshot_id, and window_id pairing for type and set-value before
forwarding the request. Update the validation used by drive_cmd or
build_engine_call, alongside the existing click_like check, so Type and SetValue
reject a lone or incomplete --element-index combination with the established CLI
validation behavior.
| DriveCommand::Menu(a) => { | ||
| let path: serde_json::Value = serde_json::from_str(&a.path) | ||
| .map_err(|e| format!("menu --path must be JSON array: {e}"))?; | ||
| DriveRequest { | ||
| action: DriveAction::InvokeMenu, | ||
| pid: Some(a.pid), | ||
| window_id: a.window_id, | ||
| menu_path: Some(path), | ||
| highlight: HighlightMode::Auto, | ||
| ..Default::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check that --path parses to a JSON array.
The error text promises a JSON array, but serde_json::from_str accepts any valid JSON value. --path '"File"', --path '5', and --path '{}' all pass this check and reach the engine as the path argument of invoke_menu. The user then sees a generic engine failure instead of the argument error stated here.
🐛 Proposed fix
let path: serde_json::Value = serde_json::from_str(&a.path)
.map_err(|e| format!("menu --path must be JSON array: {e}"))?;
+ if !path.is_array() {
+ return Err("menu --path must be a JSON array, e.g. '[\"File\",\"New\"]'".into());
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DriveCommand::Menu(a) => { | |
| let path: serde_json::Value = serde_json::from_str(&a.path) | |
| .map_err(|e| format!("menu --path must be JSON array: {e}"))?; | |
| DriveRequest { | |
| action: DriveAction::InvokeMenu, | |
| pid: Some(a.pid), | |
| window_id: a.window_id, | |
| menu_path: Some(path), | |
| highlight: HighlightMode::Auto, | |
| ..Default::default() | |
| } | |
| } | |
| DriveCommand::Menu(a) => { | |
| let path: serde_json::Value = serde_json::from_str(&a.path) | |
| .map_err(|e| format!("menu --path must be JSON array: {e}"))?; | |
| if !path.is_array() { | |
| return Err("menu --path must be a JSON array, e.g. '[\"File\",\"New\"]'".into()); | |
| } | |
| DriveRequest { | |
| action: DriveAction::InvokeMenu, | |
| pid: Some(a.pid), | |
| window_id: a.window_id, | |
| menu_path: Some(path), | |
| highlight: HighlightMode::Auto, | |
| ..Default::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 `@apps/cli/src/commands/desktop_use.rs` around lines 719 - 730, Update the
DriveCommand::Menu parsing flow to validate that the deserialized
serde_json::Value is specifically an array before constructing the DriveRequest.
Preserve the existing “menu --path must be JSON array” argument error for both
invalid JSON and valid non-array values, and only pass array values as
menu_path.
| match strategy { | ||
| Some(s) if s.contains("existing") => { | ||
| if req.window_id.is_none() { | ||
| return Err( | ||
| "prepare with existing_profile requires --window-id (engine 0.17)" | ||
| .into(), | ||
| ); | ||
| } | ||
| a["strategy"] = json!({ "kind": "existing_profile" }); | ||
| } | ||
| Some(s) if s.contains("isolated_named") => { | ||
| a["profile"] = json!({ "mode": "isolated_named", "name": "atmos" }); | ||
| a["allow_launch"] = json!(true); | ||
| } | ||
| Some(s) if s.contains("isolated") => { | ||
| a["profile"] = json!({ "mode": "isolated_new" }); | ||
| a["allow_launch"] = json!(true); | ||
| } | ||
| Some(_) => { | ||
| // Unknown strategy string: only attach window_id when present; omit strategy. | ||
| } | ||
| None => { | ||
| // Detect-only prepare (no strategy) when window_id absent. | ||
| // With window_id, still omit strategy unless caller asked — avoids | ||
| // forcing existing_profile without explicit consent intent. | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unknown --strategy values instead of ignoring them.
The match uses substring tests. Two effects follow from this.
- A mistyped value falls into the
Some(_)arm at Line 58. The request then runsbrowser_preparewith no strategy, and the user gets a success result for a different behavior than requested. - A value such as
non_existing_profilematchescontains("existing")and selectsexisting_profile.
Match the documented values exactly and return an error for anything else. The CLI help at apps/cli/src/commands/browser_use.rs Line 96 already documents the closed set existing_profile | isolated_new | isolated_named.
🔧 Proposed fix for exact strategy matching
match strategy {
- Some(s) if s.contains("existing") => {
+ Some("existing_profile") => {
if req.window_id.is_none() {
return Err(
"prepare with existing_profile requires --window-id (engine 0.17)"
.into(),
);
}
a["strategy"] = json!({ "kind": "existing_profile" });
}
- Some(s) if s.contains("isolated_named") => {
+ Some("isolated_named") => {
a["profile"] = json!({ "mode": "isolated_named", "name": "atmos" });
a["allow_launch"] = json!(true);
}
- Some(s) if s.contains("isolated") => {
+ Some("isolated_new") => {
a["profile"] = json!({ "mode": "isolated_new" });
a["allow_launch"] = json!(true);
}
- Some(_) => {
- // Unknown strategy string: only attach window_id when present; omit strategy.
- }
+ Some(other) => {
+ return Err(format!(
+ "invalid --strategy {other:?} (use existing_profile|isolated_new|isolated_named)"
+ ));
+ }
None => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| match strategy { | |
| Some(s) if s.contains("existing") => { | |
| if req.window_id.is_none() { | |
| return Err( | |
| "prepare with existing_profile requires --window-id (engine 0.17)" | |
| .into(), | |
| ); | |
| } | |
| a["strategy"] = json!({ "kind": "existing_profile" }); | |
| } | |
| Some(s) if s.contains("isolated_named") => { | |
| a["profile"] = json!({ "mode": "isolated_named", "name": "atmos" }); | |
| a["allow_launch"] = json!(true); | |
| } | |
| Some(s) if s.contains("isolated") => { | |
| a["profile"] = json!({ "mode": "isolated_new" }); | |
| a["allow_launch"] = json!(true); | |
| } | |
| Some(_) => { | |
| // Unknown strategy string: only attach window_id when present; omit strategy. | |
| } | |
| None => { | |
| // Detect-only prepare (no strategy) when window_id absent. | |
| // With window_id, still omit strategy unless caller asked — avoids | |
| // forcing existing_profile without explicit consent intent. | |
| } | |
| } | |
| match strategy { | |
| Some("existing_profile") => { | |
| if req.window_id.is_none() { | |
| return Err( | |
| "prepare with existing_profile requires --window-id (engine 0.17)" | |
| .into(), | |
| ); | |
| } | |
| a["strategy"] = json!({ "kind": "existing_profile" }); | |
| } | |
| Some("isolated_named") => { | |
| a["profile"] = json!({ "mode": "isolated_named", "name": "atmos" }); | |
| a["allow_launch"] = json!(true); | |
| } | |
| Some("isolated_new") => { | |
| a["profile"] = json!({ "mode": "isolated_new" }); | |
| a["allow_launch"] = json!(true); | |
| } | |
| Some(other) => { | |
| return Err(format!( | |
| "invalid --strategy {other:?} (use existing_profile|isolated_new|isolated_named)" | |
| )); | |
| } | |
| None => { | |
| // Detect-only prepare (no strategy) when window_id absent. | |
| // With window_id, still omit strategy unless caller asked — avoids | |
| // forcing existing_profile without explicit consent intent. | |
| } | |
| } |
🤖 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 `@crates/browser-use/src/backends/cua.rs` around lines 40 - 66, Update the
strategy handling in the request-building match to compare values exactly rather
than using substring checks: accept only existing_profile, isolated_new, and
isolated_named, preserving their current payload behavior. For any other
Some(strategy) value, return an error instead of proceeding without a strategy;
leave the None detect-only behavior unchanged.
| }; | ||
| } | ||
|
|
||
| match host::call_tool(&engine, &socket, tool, &args) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect call_tool_inner for read/write timeouts on the engine socket.
ast-grep outline crates/desktop-use/src/host.rs --items all
rg -n -C 5 'fn call_tool_inner|set_read_timeout|set_write_timeout|Duration::from_' crates/desktop-use/src/host.rsRepository: AruNi-01/atmos
Length of output: 2706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant call_tool_inner implementation and declarations around host functions.
sed -n '120,190p' crates/desktop-use/src/host.rs
printf '\n--- all host.rs timeout/deadline/time mentions ---\n'
rg -n -C 3 'timeout|deadline|Instant|set_read_timeout|set_write_timeout|Duration::|call_tool_inner|call_tool\(' crates/desktop-use/src/host.rs crates/browser-use/src/backends/cua.rsRepository: AruNi-01/atmos
Length of output: 7463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- crates/browser-use/src/backends/cua.rs call path around host::call_tool ---'
sed -n '160,260p' crates/browser-use/src/backends/cua.rs
printf '\n%s\n' '--- all call_tool usages in browser-use and desktop-use ---'
rg -n -C 4 'call_tool\(' crates/browser-use crates/desktop-useRepository: AruNi-01/atmos
Length of output: 13369
Apply a timeout to host::call_tool.
call_tool_inner invokes the engine via Command::output() with no set_read_timeout / set_write_timeout / deadline wrapper visible in crates/desktop-use/src/host.rs. If the engine call subcommand blocks, browser actions in crates/browser-use/src/backends/cua.rs can hang.
🤖 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 `@crates/browser-use/src/backends/cua.rs` at line 226, Update the call path
around host::call_tool in the browser backend to enforce a bounded timeout for
engine invocations, using the existing host/engine command mechanism and
propagating timeout failures through the current result handling. Ensure blocked
call subcommands cannot leave browser actions hanging indefinitely.
| | Code | Meaning | | ||
| |------|---------| | ||
| | `embedded_browser_not_implemented` | Stub; wait for APP-053 webview | | ||
| | `control_engine_not_installed` | `atmos desktop-use driver ensure` first | | ||
| | `invalid_args` | Missing pid / window_id / target_id / tab_id / ref | | ||
| | `browser_engine_failed` | Engine refusal (consent, setup, scope) — read `result` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the control_engine_failed error code to the table.
crates/browser-use/src/backends/cua.rs Line 222 returns control_engine_failed when host::ensure_daemon fails. That is the code an agent receives when the engine is installed but macOS Accessibility or Screen Recording is not granted. The table omits it, so the agent has no documented recovery step for the most common runtime failure.
📝 Proposed error-table addition
| `control_engine_not_installed` | `atmos desktop-use driver ensure` first |
+| `control_engine_failed` | Engine did not become ready — grant Accessibility and Screen Recording in Settings → Desktop Use, then retry |
| `invalid_args` | Missing pid / window_id / target_id / tab_id / ref |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Code | Meaning | | |
| |------|---------| | |
| | `embedded_browser_not_implemented` | Stub; wait for APP-053 webview | | |
| | `control_engine_not_installed` | `atmos desktop-use driver ensure` first | | |
| | `invalid_args` | Missing pid / window_id / target_id / tab_id / ref | | |
| | `browser_engine_failed` | Engine refusal (consent, setup, scope) — read `result` | | |
| | Code | Meaning | | |
| |------|---------| | |
| | `embedded_browser_not_implemented` | Stub; wait for APP-053 webview | | |
| | `control_engine_not_installed` | `atmos desktop-use driver ensure` first | | |
| | `control_engine_failed` | Engine did not become ready — grant Accessibility and Screen Recording in Settings → Desktop Use, then retry | | |
| | `invalid_args` | Missing pid / window_id / target_id / tab_id / ref | | |
| | `browser_engine_failed` | Engine refusal (consent, setup, scope) — read `result` | |
🤖 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 `@skills/atmos-browser-use/SKILL.md` around lines 59 - 64, Add the
control_engine_failed entry to the error-code table in SKILL.md, documenting
that it indicates host::ensure_daemon failure such as missing macOS
Accessibility or Screen Recording permissions and directing agents to inspect
the result and resolve the required permissions.
| # Phase 1–2 desktop shell (subset) | ||
| atmos desktop-use --json drive double-click|right-click|drag|scroll|hotkey|key|move|apps|launch|quit|… | ||
| atmos desktop-use --json drive clipboard get|set | ||
| atmos desktop-use --json drive screen|cursor|menu|ax-tree|front|set-value|window-frame|zoom|verify-state |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use non-shell notation for command alternatives.
Lines 89-91 are inside a Bash code block. Bash treats | as a pipeline, so these examples do not show selectable subcommands. List alternatives in comments or provide one executable command per line.
Proposed fix
- atmos desktop-use --json drive double-click|right-click|drag|scroll|hotkey|key|move|apps|launch|quit|…
- atmos desktop-use --json drive clipboard get|set
- atmos desktop-use --json drive screen|cursor|menu|ax-tree|front|set-value|window-frame|zoom|verify-state
+# Phase 1–2 commands include double-click, right-click, drag, scroll, hotkey,
+# key, move, apps, launch, quit, clipboard get/set, screen, cursor, menu,
+# ax-tree, front, set-value, window-frame, zoom, and verify-state.
+atmos desktop-use --json drive double-click📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Phase 1–2 desktop shell (subset) | |
| atmos desktop-use --json drive double-click|right-click|drag|scroll|hotkey|key|move|apps|launch|quit|… | |
| atmos desktop-use --json drive clipboard get|set | |
| atmos desktop-use --json drive screen|cursor|menu|ax-tree|front|set-value|window-frame|zoom|verify-state | |
| # Phase 1–2 desktop shell (subset) | |
| # Phase 1–2 commands include double-click, right-click, drag, scroll, hotkey, | |
| # key, move, apps, launch, quit, clipboard get/set, screen, cursor, menu, | |
| # ax-tree, front, set-value, window-frame, zoom, and verify-state. | |
| atmos desktop-use --json drive double-click |
🤖 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 `@skills/atmos-desktop-use/references/cli.md` around lines 88 - 91, Update the
command examples around the desktop-use drive subcommands to avoid Bash pipeline
notation: represent alternatives in comments or provide separate executable
command lines, while preserving the documented subcommand choices and JSON
usage.
| | List windows | `drive verify` (**not** `drive windows`) | | ||
| | Full-display screenshot | `drive screenshot --out …` (preferred for click loops) | | ||
| | Frontmost window only | `capture --out …` | | ||
| | Click / type / full desktop shell | `drive click|type|double-click|scroll|hotkey|…` (Phase 1–2; see cli.md) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the command separators in the decision table.
Line 33 contains literal | characters inside a Markdown table cell. markdownlint-cli2 parses them as column separators and reports seven cells instead of two. Replace the pipe-delimited shorthand with comma-separated command names, or escape the separators.
Proposed fix
-| Click / type / full desktop shell | `drive click|type|double-click|scroll|hotkey|…` (Phase 1–2; see cli.md) |
+| Click / type / full desktop shell | `drive click`, `drive type`, `drive double-click`, `drive scroll`, `drive hotkey`, … (Phase 1–2; see cli.md) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Click / type / full desktop shell | `drive click|type|double-click|scroll|hotkey|…` (Phase 1–2; see cli.md) | | |
| | Click / type / full desktop shell | `drive click`, `drive type`, `drive double-click`, `drive scroll`, `drive hotkey`, … (Phase 1–2; see cli.md) | |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 33-33: Table column count
Expected: 2; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@skills/atmos-desktop-use/SKILL.md` at line 33, Update the “Click / type /
full desktop shell” row in the decision table to prevent the literal command
separators in the drive shorthand from being parsed as Markdown column
delimiters; replace them with comma-separated command names or escape each pipe
while preserving the command list meaning.
Source: Linters/SAST tools
| # ax_empty / electron_likely:true → pixel path only | ||
| atmos desktop-use --json drive screenshot --out /tmp/du.png | ||
| atmos desktop-use --json drive click --x <png_x> --y <png_y> # background first | ||
| # only if UI unchanged: | ||
| atmos desktop-use --json drive click --x <png_x> --y <png_y> \ | ||
| --delivery-mode foreground --window-id <id> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the Electron fallback rule consistent with the surface table.
Line 91 permits a matching element_token for ax_sparse. Line 101 sends every electron_likely:true surface directly to pixels. This gives sparse Electron/Chromium surfaces two different action rules. Restrict the pixel-only example to ax_empty, or update the ax_sparse row if Electron detection must override token use.
Proposed fix
-# ax_empty / electron_likely:true → pixel path only
+# ax_empty → pixel path only; for ax_sparse, use a matching token before pixels📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ax_empty / electron_likely:true → pixel path only | |
| atmos desktop-use --json drive screenshot --out /tmp/du.png | |
| atmos desktop-use --json drive click --x <png_x> --y <png_y> # background first | |
| # only if UI unchanged: | |
| atmos desktop-use --json drive click --x <png_x> --y <png_y> \ | |
| --delivery-mode foreground --window-id <id> | |
| # ax_empty → pixel path only; for ax_sparse, use a matching token before pixels | |
| atmos desktop-use --json drive screenshot --out /tmp/du.png | |
| atmos desktop-use --json drive click --x <png_x> --y <png_y> # background first | |
| # only if UI unchanged: | |
| atmos desktop-use --json drive click --x <png_x> --y <png_y> \ | |
| --delivery-mode foreground --window-id <id> |
🤖 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 `@skills/atmos-desktop-use/SKILL.md` around lines 101 - 106, Make the Electron
fallback guidance consistent with the surface table: update the “ax_empty /
electron_likely:true” rule in the desktop-use instructions so pixel-only
handling applies only to ax_empty, while ax_sparse continues permitting a
matching element_token unless the table is intentionally changed to make
Electron detection override it.
| ### 5.1 Desktop drive phases (product CLI, not 1:1 engine dump) | ||
|
|
||
| | Phase | Goal | `atmos desktop-use drive` (representative) | | ||
| |-------|------|-----------------------------------------------| | ||
| | **0 (shipped)** | Install + capture + basic click/type | `screenshot`, `click`, `type`, `verify`, `window-state`, `highlight`, `session-end` | | ||
| | **1** | Full computer shell for agents | `double-click`, `right-click`, `drag`, `scroll`, `hotkey`, `key`, `move`, `apps`, `launch`, `quit`, `clipboard get|set`, `screen`, `cursor`, `menu`, `ax-tree` | | ||
| | **2** | Explicit extras (never default steal focus) | `front` (bring_to_front), `set-value`, `window-frame`, `zoom`, `verify-state` | | ||
| | **3** | Page CDP — **not under Desktop Use** | See **§5.2 Browser Use** (`atmos browser-use`) | | ||
|
|
||
| Defaults: `delivery_mode=background`; optional session + operation border chrome; foreground only when requested. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the pipe in the Phase 1 table row.
Line 142 contains clipboard get|set inside a Markdown table. Markdown treats the pipe as a column separator. This causes the reported MD038 and MD056 warnings and can render the row incorrectly. Split the command into clipboard get and clipboard set, or escape the pipe.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 142-142: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 142-142: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 142-142: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 142-142: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 142-142: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@specs/APP/APP-052_desktop-use/TECH.md` around lines 137 - 146, The pipe
character in `clipboard get|set` within the Phase 1 row of the desktop drive
phases table is being interpreted as a Markdown table column separator instead
of part of the command text, causing MD038 and MD056 warnings and incorrect
rendering. Escape the pipe character in the Phase 1 command list by replacing
`clipboard get|set` with `clipboard get\|set` so the pipe is treated as literal
text rather than a table delimiter.
Source: Linters/SAST tools
- Gate macOS-only highlight helper symbols so Linux CI dead_code passes - Fix overly complex surface classify and prefs field_reassign lint - Box large DriveCommand enum variant; drop unit-struct Default calls - Validate type --highlight; wrap host screenshot spawn errors
…rlay - Skip unreadable dirs in find_named_file; stamp installed_at in real UTC - Require PNG/JPEG magic for screenshot bytes (reject text failures) - Reap highlight helper children; remove NSScreen[0] crash path - Propagate screenshot write errors; session-end uses caller manager
E2E report: ✅ Passed30 passed · 0 failed · 2 flaky · 0 skipped · 3m 29s · 93.8% pass rate Run
Overview
By file
By project
All selected E2E suites passed. |
Absorb PR #202 (Desktop Use host engine, AppShot host capture, slash/skill) into the browser-use branch while keeping the real embedded host plane. Resolution: - CUA browser-use routes through desktop_use::host / DesktopUseManager - Embedded backend remains APP-053 control.json loopback (not stub) - CLI type --ref optional for embedded, required on CUA via validation - Both /desktop-use and /browser-use slash commands + system skills - Packaged dock.setIcon still skipped (Dock zero-width fix)
This branch was successfully deployed
1 participant
Summary
Completes the Desktop Use control-engine path on top of #199: a real installable engine (pinned Cua Driver runtime), white-labeled as Atmos Desktop Use.app, with one macOS TCC identity for AppShot capture + desktop control. No public Cua branding, no MCP product surface.
Control engine (pin + ensure)
cua-driver-rsv0.17.0 release artifacts (URL + sha256) incrates/desktop-use/manifest/default.jsonatmos desktop-use driver ensuredownloads, verifies, extracts into~/.atmos/desktop-use/atmos-desktop-control+ vendor-scrubbed user-facing stringsstatus·doctor·driver ensure|stop|uninstall|grant-permissions·drive screenshot|click|type|verifyAtmos Desktop Use.app (unified TCC host)
com.atmos.desktop.use) + ad-hoc codesign /lsregisteropen -n -g -a "…/Atmos Desktop Use.app" --args serve --socket …so live process Identifier matches grant target (not a naked binary, not CuaDriver.app)
driver grant-permissionslaunches hostpermissions grantAppShot capture = host when engine installed
resolveAppShotCaptureRoute→captureFrontmostViaHostEnginewhendriver.installedosascript/screencaptureis pre-ensure fallback onlyScreenshot wire protocol (0.17.0 reality)
call --screenshot-out-file+ tool argscreenshot_out_file, MCP imagecontent[],screenshot_file_pathscreenshot_base64/png_base64on the engine (verified absent in binary strings)parse_call_tool_output: exit-0 plain-text engine errors → Err (not softok:true+ raw)extract_screenshot_png+ fixtures undercrates/desktop-use/tests/fixtures/engine_0_17_0/png_base64/png_path/capturefor clientsSpecs / product language
specs/APP/APP-052_desktop-use/TECH.md: host TCC rules, AppShot host route, screenshot wireRelated
Type of Change
Validation
cargo test -p desktop-use --lib(26 tests, includingengine_protocolfixtures)cargo fmt/cargo clippy -p desktop-use -- -D warningshost-capture+frontmost-routeunit testsatmos desktop-use --help|status|doctor|driver ensure(skip-download path)Checklist
apps/desktop-electrononly~/Applicationsfor easier System Settings discovery (follow-up)Out of scope
Summary by CodeRabbit