@smbcloud / cli / commits / 589c8e5

Document the adb-only baseline in android_capabilities (#77)

* Spell out the adb-only baseline in android_capabilities `android_capabilities()` is the hand-maintained adb baseline and reports the AndroidKit-backed semantic UI actions (`ui_describe`, `ui_element_list`, `ui_tap`) as false, which reads like a bug until you know that `device_capabilities` upgrades exactly those three to true once its `device.info` call reaches AndroidKit. Say so in the doc comment, and name "not available over adb alone" as the meaning of false. Reset all three in the failure branch rather than only `ui_tap`, mirroring the success branch. The output is unchanged because the baseline already defaults them to false; the point is that both branches now name the same three keys, so neither can be mistaken for the full set of AndroidKit-backed actions. Pin the baseline with a test so a later edit has to mean it. Co-Authored-By: siGit Code v1.7.0-acp <noreply@sigit.si> * Retry a helper that is still open for writing Unix refuses to exec a file that any process still holds open for writing, reporting ETXTBSY rather than a missing or broken binary. The xcrs test helper wrote its fake helper through std::fs::write and then re-opened it to set the mode, so a sibling test thread forking in that window left the fd open and the exec failed. macOS does not reject the exec, which is why the suite passed locally and failed in CI. Write the helper through a single OpenOptions handle carrying mode 0o755 and drop it before returning, and give HostInspect a short bounded retry on ETXTBSY: the same condition happens in production when `swift build` replaces a live helper, and it clears as soon as the other side closes its descriptor. Co-Authored-By: siGit Code v1.7.0-acp <noreply@sigit.si> --------- Co-authored-by: siGit Code v1.7.0-acp <noreply@sigit.si>

keypair34 committed Oct 17, 2026 at 14:37 UTC 589c8e5059f2a57634bc0b1211900ad5ce165f7a

2 files changed +120 -16

crates/xcrs/src/mcp.rs

+31

@@ -339,6 +339,15 @@ impl Orientation {

339 /// Static, hand-maintained capability description for Android targets. Unlike

340 /// Apple's `device.capabilities` ControlKit call, adb has no capability-discovery

341 /// RPC, so this mirrors exactly the actions `AndroidDebugBridge` implements.

342 +///

343 +/// This is the adb-only baseline, and the semantic UI actions it reports as

344 +/// `false` (`ui_describe`, `ui_element_list`, `ui_tap`) are the ones that only

345 +/// XCRS AndroidKit can provide. The `device_capabilities` handler starts from

346 +/// this value and, when it reaches AndroidKit via its `device.info` call,

347 +/// upgrades exactly those three to `true` and records an `androidkit` object;

348 +/// when the call fails, it leaves them `false` and records the setup hint

349 +/// instead. So a `false` here means "not available over adb alone", not "never

350 +/// available on this target".

351 pub fn android_capabilities() -> serde_json::Value {

352 serde_json::json!({

353 "screen_capture": true,

@@ -1050,6 +1059,8 @@ macro_rules! xcrs_mcp_tools {

1059 object.insert("androidkit".to_string(), ::serde_json::json!({ "available": true, "info": info }));

1060 }

1061 Err(error) => {

1062 + object.insert("ui_describe".to_string(), ::serde_json::json!(false));

1063 + object.insert("ui_element_list".to_string(), ::serde_json::json!(false));

1064 object.insert("ui_tap".to_string(), ::serde_json::json!(false));

1065 object.insert("androidkit".to_string(), ::serde_json::json!({

1066 "available": false,

@@ -2714,4 +2725,24 @@ mod tests {

2725 serde_json::json!(AndroidButton::ALL)

2726 );

2727 }

2728 +

2729 + /// The adb-only baseline reports the AndroidKit-backed semantic UI actions as

2730 + /// unavailable; `device_capabilities` may only ever upgrade them to `true`

2731 + /// after a successful `device.info` call, so a regression that flips them on

2732 + /// here would advertise actions that plain adb cannot perform.

2733 + #[test]

2734 + fn android_capabilities_reports_androidkit_backed_actions_as_unavailable() {

2735 + let capabilities = android_capabilities();

2736 + for action in ["ui_describe", "ui_element_list", "ui_tap"] {

2737 + assert_eq!(

2738 + capabilities[action],

2739 + serde_json::json!(false),

2740 + "{action} is AndroidKit-backed and must default to false"

2741 + );

2742 + }

2743 + assert!(

2744 + capabilities.get("androidkit").is_none(),

2745 + "the androidkit object is added by the device_capabilities handler, not the baseline"

2746 + );

2747 + }

2748 }

crates/xcrs/src/xcrs.rs

+89 -16

@@ -17,6 +17,16 @@ const ANDROIDKIT_START_TIMEOUT: Duration = Duration::from_secs(10);

17 const ANDROIDKIT_START_PROBE_TIMEOUT: Duration = Duration::from_millis(500);

18 const ANDROIDKIT_START_RETRY_DELAY: Duration = Duration::from_millis(100);

20 +/// `ETXTBSY`, for an executable that another process still holds open for

21 +/// writing. It has the same value on Linux and the BSDs, and `ErrorKind` has no

22 +/// variant for it.

23 +#[cfg(unix)]

24 +const TEXT_FILE_BUSY: i32 = 26;

25 +#[cfg(unix)]

26 +const TEXT_FILE_BUSY_RETRY_DELAY: Duration = Duration::from_millis(20);

27 +#[cfg(unix)]

28 +const TEXT_FILE_BUSY_RETRY_ATTEMPTS: usize = 5;

29 +

30 pub fn encode_base64(data: impl AsRef<[u8]>) -> String {

31 use base64::{engine::general_purpose::STANDARD, Engine};

@@ -1520,6 +1530,16 @@ pub struct HostInspect {

1530 helper_path: PathBuf,

1531 }

1532

1533 +#[cfg(unix)]

1534 +fn is_text_file_busy(result: &std::io::Result<Output>) -> bool {

1535 + matches!(result, Err(error) if error.raw_os_error() == Some(TEXT_FILE_BUSY))

1536 +}

1537 +

1538 +#[cfg(not(unix))]

1539 +fn is_text_file_busy(_result: &std::io::Result<Output>) -> bool {

1540 + false

1541 +}

1542 +

1543 impl Default for HostInspect {

1544 fn default() -> Self {

1545 Self {

@@ -1549,15 +1569,12 @@ impl HostInspect {

1569 /// here, so a caller can forward the code instead of re-deriving it. This

1570 /// only fails when the helper could not be started at all.

1571 pub fn invoke(&self, arguments: &[String]) -> Result<HostInspectInvocation> {

1552 - let output = Command::new(&self.helper_path)

1553 - .args(arguments)

1554 - .output()

1555 - .with_context(|| {

1556 - format!(

1557 - "failed to run {} (build it with `swift build` in xcrs-controlkit)",

1558 - self.helper_path.display()

1559 - )

1560 - })?;

1572 + let output = self.run_helper(arguments).with_context(|| {

1573 + format!(

1574 + "failed to run {} (build it with `swift build` in xcrs-controlkit)",

1575 + self.helper_path.display()

1576 + )

1577 + })?;

1578

1579 Ok(HostInspectInvocation {

1580 exit_code: output.status.code(),

@@ -1566,6 +1583,27 @@ impl HostInspect {

1583 })

1584 }

1585

1586 + /// Start the helper, waiting out a file that is briefly unstartable.

1587 + ///

1588 + /// Unix refuses to `exec` an executable that any process still holds open

1589 + /// for writing, and reports that as `ETXTBSY` rather than as a missing or

1590 + /// broken binary. A `swift build` replacing the helper does it, and so does

1591 + /// a sibling thread forking while a test writes a stand-in; both last only

1592 + /// until the other side closes its descriptor, so a short retry is the

1593 + /// difference between a transient condition and a reported failure.

1594 + fn run_helper(&self, arguments: &[String]) -> std::io::Result<Output> {

1595 + let mut attempt = 0;

1596 + loop {

1597 + let started = Command::new(&self.helper_path).args(arguments).output();

1598 + if !is_text_file_busy(&started) || attempt >= TEXT_FILE_BUSY_RETRY_ATTEMPTS {

1599 + return started;

1600 + }

1601 +

1602 + attempt += 1;

1603 + std::thread::sleep(TEXT_FILE_BUSY_RETRY_DELAY);

1604 + }

1605 + }

1606 +

1607 /// List the host's on-screen windows, ordered by layer and then id.

1608 pub fn windows(&self, owner: Option<&str>) -> Result<HostWindowList> {

1609 let mut arguments = vec!["windows".to_string()];

@@ -2480,15 +2518,24 @@ SERIAL456 no permissions (user denied) usb:0-2 transport_id:3

2518 /// without a built `xcrs-host-inspect` or a macOS host.

2519 #[cfg(unix)]

2520 fn write_fake_helper(directory: &Path, script: &str) -> PathBuf {

2483 - use std::os::unix::fs::PermissionsExt;

2521 + use std::io::Write;

2522 + use std::os::unix::fs::OpenOptionsExt;

2523

2524 let path = directory.join(HOST_INSPECT_HELPER);

2486 - std::fs::write(&path, script).expect("the fake helper should write");

2487 - let mut permissions = std::fs::metadata(&path)

2488 - .expect("the fake helper should stat")

2489 - .permissions();

2490 - permissions.set_mode(0o755);

2491 - std::fs::set_permissions(&path, permissions).expect("the fake helper should be executable");

2525 + // One handle writes the script and carries the executable bit, so the

2526 + // file is never re-opened and its descriptor is closed before the test

2527 + // starts it.

2528 + let mut helper = std::fs::OpenOptions::new()

2529 + .write(true)

2530 + .create(true)

2531 + .truncate(true)

2532 + .mode(0o755)

2533 + .open(&path)

2534 + .expect("the fake helper should open");

2535 + helper

2536 + .write_all(script.as_bytes())

2537 + .expect("the fake helper should write");

2538 + drop(helper);

2539

2540 path

2541 }

@@ -2562,6 +2609,32 @@ SERIAL456 no permissions (user denied) usb:0-2 transport_id:3

2609 );

2610 }

2611

2612 + #[cfg(unix)]

2613 + #[test]

2614 + fn waits_out_a_helper_that_is_still_open_for_writing() {

2615 + let directory = tempfile::tempdir().expect("a temporary directory");

2616 + let (helper, recorded) = recording_helper(directory.path(), WINDOW_LIST_JSON, 0);

2617 + let host = HostInspect::with_path(&helper);

2618 +

2619 + // Unix refuses to start a file that is open for writing anywhere, and

2620 + // that is what a sibling thread forking mid-write produces. Holding the

2621 + // descriptor here reproduces it on demand.

2622 + let held = std::fs::OpenOptions::new()

2623 + .write(true)

2624 + .open(&helper)

2625 + .expect("the helper should reopen for writing");

2626 + let release = std::thread::spawn(move || {

2627 + std::thread::sleep(Duration::from_millis(50));

2628 + drop(held);

2629 + });

2630 +

2631 + host.windows(None)

2632 + .expect("windows should wait out the open handle");

2633 + release.join().expect("the releasing thread should finish");

2634 +

2635 + assert_eq!(recorded_arguments(&recorded), vec!["windows"]);

2636 + }

2637 +

2638 #[cfg(unix)]

2639 #[test]

2640 fn builds_the_windows_arguments() {