From bc3b62bf27f761296ce1c31a992452931d070014 Mon Sep 17 00:00:00 2001 From: Bryan Roscoe Date: Thu, 22 Oct 2026 23:07:09 -0500 Subject: [PATCH] Rewrite historical Rust comments as current rules Comment-only change across the shared core, desktop, mobile and tools crates. Comments that narrated how the code used to behave ("the old code", "used to", "until now", version numbers, "Phase 3", reviewer attributions) now state the constraint in present tense, keeping the issue numbers that point at reports and regression tests. Also corrects comments that no longer matched the code: the commitment number on compute_apply_plan, the unit on memory_mb, the mobile pull size cap (now enforced while streaming), the v1 adb fallback location, the compiled-out remote-input live test, a self-contradicting /proc/net/route example, a tempdir "fallback" that panics, and three doc comments attached to the wrong function. --- v2/crates/core/src/adb/batch.rs | 10 ++-- v2/crates/core/src/adb/parse.rs | 8 +-- v2/crates/core/src/adb/remote_input.rs | 24 ++++----- v2/crates/core/src/adb/sim/tests.rs | 7 ++- v2/crates/core/src/commands/apps.rs | 27 +++++------ v2/crates/core/src/commands/devices.rs | 62 ++++++++++++------------ v2/crates/core/src/commands/files.rs | 4 +- v2/crates/core/src/commands/health.rs | 13 +++-- v2/crates/core/src/commands/launcher.rs | 54 ++++++++++----------- v2/crates/core/src/commands/loader.rs | 18 +++---- v2/crates/core/src/commands/optimize.rs | 13 +++-- v2/crates/core/src/commands/snapshot.rs | 12 ++--- v2/crates/core/src/commands/state.rs | 11 ++--- v2/crates/core/src/commands/tuning.rs | 22 ++++----- v2/crates/core/src/engine/app_lists.rs | 8 +-- v2/crates/core/src/engine/detection.rs | 12 ++--- v2/crates/core/src/engine/safety.rs | 23 +++++---- v2/crates/core/src/engine/snapshot.rs | 4 +- v2/crates/core/src/engine/types.rs | 2 +- v2/crates/core/src/lib.rs | 4 +- v2/mobile/src-tauri/src/file_commands.rs | 13 ++--- v2/mobile/src-tauri/src/lib.rs | 4 +- v2/mobile/src-tauri/src/wireless_adb.rs | 3 +- v2/src-tauri/src/adb/driver.rs | 32 ++++++------ v2/src-tauri/src/adb/install.rs | 1 - v2/src-tauri/src/adb/scan.rs | 3 +- v2/src-tauri/src/commands/diagnostics.rs | 8 +-- v2/src-tauri/src/commands/quit.rs | 8 +-- v2/src-tauri/src/commands/scan.rs | 28 +++++------ v2/src-tauri/src/lib.rs | 2 +- 30 files changed, 214 insertions(+), 226 deletions(-) diff --git a/v2/crates/core/src/adb/batch.rs b/v2/crates/core/src/adb/batch.rs index 13edee61..cd044941 100644 --- a/v2/crates/core/src/adb/batch.rs +++ b/v2/crates/core/src/adb/batch.rs @@ -1,9 +1,8 @@ //! Batched device-side shell invocation. //! -//! Several read paths used to fan out N separate `adb shell` calls where one -//! compound shell would do. On the desktop driver each call is a process -//! spawn; on the mobile driver every call is serialized behind a single TCP -//! connection, so the fan-out costs N × Wi-Fi RTT for zero concurrency. +//! On the desktop driver each `adb shell` call is a process spawn; on the +//! mobile driver every call is serialized behind a single TCP connection, so +//! N separate reads cost N × Wi-Fi RTT for zero concurrency. //! `batch_command` folds them into one invocation whose stdout `split_batch` //! cuts back apart on a sentinel. @@ -76,8 +75,7 @@ pub fn parse_checked_batch( /// transport, which merges stdout and stderr into one stream. /// /// No double quotes are emitted, so the result survives being passed as a -/// single `adb -s shell ` argument the same way the compound -/// commands already in the tree (`devices.rs::harvest_properties`) do. +/// single `adb -s shell ` argument. pub fn batch_command(cmds: &[&str]) -> String { let mut out = String::new(); for (i, cmd) in cmds.iter().enumerate() { diff --git a/v2/crates/core/src/adb/parse.rs b/v2/crates/core/src/adb/parse.rs index cbdecb3c..be4326d2 100644 --- a/v2/crates/core/src/adb/parse.rs +++ b/v2/crates/core/src/adb/parse.rs @@ -468,7 +468,7 @@ pub struct TemperatureReading { } fn current_celsius(value: f64) -> bool { - // Preserve the previous parser's sanity range; this is not a health grade. + // A plausibility range for a sensor reading; this is not a health grade. (10.0..120.0).contains(&value) } @@ -1270,7 +1270,7 @@ mod tests { #[test] fn real_meminfo_package_map_never_trims_a_hal_into_a_package() { let map = parse_dumpsys_meminfo(SHIELD_MEMINFO); - // `foo@2.0-service` used to be cut at the `@` and filed as `foo`. + // `foo@2.0-service` must not be cut at the `@` and filed as `foo`. assert!(!map.contains_key("vendor.nvidia.hardware.graphics.composer")); assert!(!map.contains_key("android.hardware.audio")); assert!(!map.keys().any(|k| k.contains('@') || k.contains(':'))); @@ -1282,8 +1282,8 @@ mod tests { #[test] fn path_named_processes_are_kept_not_dropped() { - // Older builds name native daemons by path. The old pattern needed the - // name to start with a package character, so these rows vanished. + // Older builds name native daemons by path. Those rows must survive + // even though the name doesn't start with a package character. let input = "Total PSS by process:\n\ 40,112K: /system/bin/surfaceflinger (pid 312)\n\ 1,234K: /vendor/bin/hw/android.hardware.audio@2.0-service (pid 400)\n\ diff --git a/v2/crates/core/src/adb/remote_input.rs b/v2/crates/core/src/adb/remote_input.rs index e306eaac..c8b5a72f 100644 --- a/v2/crates/core/src/adb/remote_input.rs +++ b/v2/crates/core/src/adb/remote_input.rs @@ -1,11 +1,11 @@ //! Persistent scrcpy control-socket session for low-latency remote input. //! -//! Today each remote key press shells out `adb shell "input keyevent N"`, which -//! cold-starts a JVM on the TV (~690 ms per press). This module replaces the -//! transport with scrcpy's control-only server: push the jar once, run it -//! resident via `app_process`, and stream fixed binary control messages over a -//! persistent socket — dropping per-press cost to network RTT. Desktop reaches -//! that socket through `adb forward`; Android opens the device service directly. +//! `adb shell "input keyevent N"` cold-starts a JVM on the TV for every press +//! (~690 ms). This module uses scrcpy's control-only server instead: push the +//! jar once, run it resident via `app_process`, and stream fixed binary control +//! messages over a persistent socket — dropping per-press cost to network RTT. +//! Desktop reaches that socket through `adb forward`; Android opens the device +//! service directly. //! //! Layering note: every adb invocation goes through the `AdbDriver` trait, per //! the one-wrapper rule. Desktop's raw `TcpStream` to its adb-forwarded local @@ -714,7 +714,6 @@ mod tests { // Honest error-path coverage: with a driver that can't spawn a child // (MockAdb uses the trait's default `spawn`, which returns unsupported), // start() must surface the spawn failure rather than hang or fake success. - // The live socket path is left for the lead's on-device check. #[tokio::test] async fn start_fails_when_driver_cannot_spawn() { use crate::commands::test_support::MockAdb; @@ -731,11 +730,12 @@ mod tests { } } - // Live end-to-end test against a real device — ignored in CI, run by hand: - // SHIELD_TEST_SERIAL=192.168.x.x:5555 cargo test remote_input_live -- --ignored --nocapture - // Starts a real session, injects SLEEP (223) and verifies via `dumpsys - // power` that the device went to sleep, then WAKEUP (224) and verifies it - // woke, then tears down and checks no server process is left behind. + // Live end-to-end test against a real device. Compiled out with + // `cfg(any())`: it builds a `SubprocessAdb`, which lives in the desktop + // crate, not this one. Starts a real session, injects SLEEP (223) and + // verifies via `dumpsys power` that the device went to sleep, then WAKEUP + // (224) and verifies it woke, then tears down and checks no server process + // is left behind. #[cfg(any())] #[tokio::test] #[ignore] diff --git a/v2/crates/core/src/adb/sim/tests.rs b/v2/crates/core/src/adb/sim/tests.rs index af8b3b48..7887a228 100644 --- a/v2/crates/core/src/adb/sim/tests.rs +++ b/v2/crates/core/src/adb/sim/tests.rs @@ -193,7 +193,7 @@ async fn a_replayed_screenshot_is_a_png() { assert!(sim.world().replay.as_ref().unwrap().report().is_empty()); } -/// #122 on v2.3.0: stock disabled, Setup Wraith still enabled with a higher +/// #122: stock disabled, Setup Wraith still enabled with a higher /// HOME filter priority, and Monet set as default. Monet holds the HOME role /// and the Home key opens it, but `resolve-activity` still names Setup /// Wraith. The current launcher is the role holder. @@ -225,7 +225,7 @@ async fn setup_wraith_outranking_the_role_holder_is_not_the_current_launcher() { let mut w = sim.world(); let d = w.devices.get_mut("1324619053514").unwrap(); assert_eq!(d.home.role_holder.as_deref(), Some(MONET)); - // The resolver alone, which 2.3.0 displayed, still names Setup Wraith. + // The resolver alone still names Setup Wraith. assert!(d.resolve_home(&stock).unwrap().starts_with(WRAITH)); } @@ -367,8 +367,7 @@ async fn google_tv_takeover_failure_re_enables_stock_and_wraith() { assert!(enabled(&sim, WRAITH_PKG), "Setup Wraith re-enabled"); } -/// A device without Setup Wraith behaves exactly as before: no command ever -/// names the setup helper. +/// A device without Setup Wraith: no command ever names the setup helper. #[tokio::test] async fn takeover_without_setup_wraith_never_touches_it() { let sim = google_tv_world(false); diff --git a/v2/crates/core/src/commands/apps.rs b/v2/crates/core/src/commands/apps.rs index 0f222071..97e398f4 100644 --- a/v2/crates/core/src/commands/apps.rs +++ b/v2/crates/core/src/commands/apps.rs @@ -50,8 +50,9 @@ pub fn safety_info(state: State<'_, AppState>, package: String) -> Safety { /// Classify a package against the rule tables *and* the reviewed app lists. /// /// The catalog is a reviewed source and has to be consulted here: without it, -/// every curated app that was not also in one of the two rule tables reported -/// "role and effects unknown" while its own description sat in the next column. +/// every curated app that is not also in one of the two rule tables would +/// report "role and effects unknown" while its own description sits in the +/// next column. pub fn safety_for(state: &AppState, package: &str) -> Safety { let catalog = state.app_lists.find(package).map(|entry| CatalogVerdict { risk: entry.risk, @@ -439,12 +440,11 @@ pub(crate) async fn disable_package_impl( message: format!("Refusing to disable {package}: {reason}"), }); } - // The launcher screen has always refused to disable the last enabled HOME - // handler, but this is the path the app list and the optimize wizard use, - // so the same action was unguarded depending on where it was started from. - // A third-party launcher is Unknown to the safety rules, so nothing else - // stops it. Best-effort: if the handler query fails we proceed as before - // rather than blocking every disable on a transient read. + // The launcher screen refuses to disable the last enabled HOME handler, + // and this is the path the app list and the optimize wizard use, so the + // same guard applies here. A third-party launcher is Unknown to the safety + // rules, so nothing else stops it. Best-effort: if the handler query fails + // the disable proceeds rather than blocking on a transient read. if let Some(refusal) = refuse_last_home_handler(state, serial, package).await { return Ok(refusal); } @@ -930,10 +930,9 @@ async fn run(state: &AppState, serial: &str, cmd: &str) -> Result Result { /// input rather than something the app relies on. /// - `[IPv6]:port` — bracketed, as adb and every URL parser expect. /// - `adb-XXXX-YYYY._adb-tls-connect._tcp[:port]` — an mDNS service name. -/// The daemon resolves these itself; rejecting them meant a user could not -/// paste what discovery had just shown them (GitHub #88). +/// The daemon resolves these itself; rejecting them would stop a user +/// pasting what discovery has just shown them (GitHub #88). /// /// Rejects empty input and any port that is not a positive 16-bit number. pub fn normalize_connect_address(address: &str) -> Result { @@ -862,7 +863,7 @@ fn is_mdns_instance(host: &str) -> bool { /// `Get-Devices` and `Show-DeviceProfile`. /// /// Each read is its own sentinel-delimited section rather than one line of a -/// combined stdout. Positional parsing looked equivalent but was not: the +/// combined stdout. Positional parsing is not equivalent: the /// leading `settings get global device_name` can print nothing at all on some /// builds and a multi-line Exception on others, and either one shifts every /// later index — so a device would silently report its model as its Android @@ -893,8 +894,9 @@ async fn harvest_properties(adb: &dyn AdbDriver, serial: &str) -> Result String { format!("{}; true", batch_command(&PROPERTY_READS)) } @@ -1135,11 +1137,11 @@ mod tests { #[tokio::test] async fn a_device_that_answers_no_to_leanback_is_still_profiled() { - // The live regression: a batch reports only its last command's exit - // code, and the last read is `pm has-feature`, which exits 1 to say - // "no". A Pixel that answered every question was refused with - // "adb process failed (exit code Some(1))". The exit code carries no - // information about the reads that came before it. + // A batch reports only its last command's exit code, and the last + // read is `pm has-feature`, which exits 1 to say "no". A device that + // answered every question must not be refused with "adb process + // failed (exit code Some(1))". The exit code carries no information + // about the reads that came before it. let mock = MockAdb::default().on_shell_exit( "settings get global device_name", &batched_props(&[ @@ -1181,8 +1183,8 @@ mod tests { #[tokio::test] async fn silent_device_name_read_does_not_shift_the_android_version() { // `settings get global device_name` prints nothing on some builds. - // Under the old positional parse every later value slid up one, so the - // TV reported its brand as its friendly name and its model as its + // A positional parse would slide every later value up one, so the TV + // would report its brand as its friendly name and its model as its // Android version. Sections keep each read in its own slot. let mock = MockAdb::default() .on_raw( @@ -1219,8 +1221,8 @@ mod tests { #[tokio::test] async fn multiline_settings_exception_stays_inside_its_own_section() { // The other failure shape: `settings get` throws and prints a - // multi-line stack trace, which under positional parsing pushed every - // real property down by however many lines the trace happened to be. + // multi-line stack trace, which under positional parsing would push + // every real property down by however many lines the trace is. let mock = MockAdb::default() .on_raw( "devices", @@ -1286,8 +1288,8 @@ mod tests { #[test] fn one_device_reached_two_ways_collapses_to_the_usable_address() { - // The live regression: adb auto-connected the device under its mDNS - // service name, and the scan then dialled the same device by address. + // adb auto-connects the device under its mDNS service name, and the + // scan then dials the same device by address. let devices = vec![ row( "adb-58040DLCH005YV-jBeCEe._adb-tls-connect._tcp", @@ -1531,9 +1533,9 @@ mod tests { #[test] fn normalize_accepts_a_scoped_ipv6_endpoint_from_discovery() { - // Codex on #128: mDNS can report a link-local host with its scope, - // and endpoint() brackets it as-is. Rejecting it here meant the - // auto-connect retried for 45 s and gave up on a device it had found. + // #128: mDNS can report a link-local host with its scope, and + // endpoint() brackets it as-is. Rejecting it here would make the + // auto-connect retry for 45 s and give up on a device it had found. assert_eq!( normalize_connect_address("[fe80::1%en0]:41541").unwrap(), "[fe80::1%en0]:41541" @@ -1552,8 +1554,8 @@ mod tests { #[test] fn normalize_accepts_an_mdns_service_name_from_discovery() { - // Discovery shows these; refusing to accept one back meant a user - // could not paste what the app had just told them (GitHub #88). + // Discovery shows these; refusing to accept one back would stop a + // user pasting what the app has just told them (GitHub #88). let instance = "adb-58040DLCH005YV-jBeCEe._adb-tls-connect._tcp"; assert_eq!( normalize_connect_address(&format!("{instance}:41541")).unwrap(), @@ -1781,9 +1783,9 @@ mod tests { #[tokio::test] async fn forget_drops_every_transport_of_the_same_hardware() { - // What the owner's Pixel looked like: adb held it under its mDNS name - // *and* under ip:port. The row shows ip:port, so disconnecting only - // that left the mDNS transport behind and the row stayed. + // adb holds the device under its mDNS name *and* under ip:port. The + // row shows ip:port, so disconnecting only that would leave the mDNS + // transport behind and the row would stay. let mock = MockAdb::default() .on_raw( "devices", @@ -1914,8 +1916,8 @@ mod tests { #[test] fn probe_matches_the_connect_service_by_its_embedded_serial() { - // Codex on #128: the instances differ in suffix, so exact equality - // never auto-connected. The serial between them is the identity. + // #128: the instances differ in suffix, so exact equality would never + // auto-connect. The serial between them is the identity. let services = parse_mdns_services(&mdns_pixel(46545)); assert_eq!( paired_connect_probe("192.168.42.211:45439", PIXEL_PAIR, &services, &[]), @@ -2005,7 +2007,7 @@ mod tests { #[tokio::test] async fn a_stale_advertisement_pointing_at_another_attached_device_is_refused() { - // Codex on #128: the advertised endpoint names a transport adb holds, + // #128: the advertised endpoint names a transport adb holds, // but that transport is a different device. Its key must not be // enough to call it the one just paired. let mock = MockAdb::default() @@ -2127,8 +2129,8 @@ mod tests { #[test] fn a_bracketed_ipv6_transport_still_counts_as_advertised() { - // Codex on #128: adb keys the transport `[fe80::1]:41541`, the - // advertisement is printed bare. Compared raw, Forget stayed silent. + // #128: adb keys the transport `[fe80::1]:41541`, the advertisement + // is printed bare. Compared raw, Forget would stay silent. let services = parse_mdns_services( "List of discovered mdns services\n\ adb-V6SER-c2\t_adb-tls-connect._tcp\tfe80::1:41541\n", diff --git a/v2/crates/core/src/commands/files.rs b/v2/crates/core/src/commands/files.rs index a0bfc650..4e7f16e8 100644 --- a/v2/crates/core/src/commands/files.rs +++ b/v2/crates/core/src/commands/files.rs @@ -131,8 +131,8 @@ pub struct FindResult { /// Directories the search could not be run against at all, because the ADB /// call itself failed. A directory that simply does not exist is *not* /// listed here — that is a real "no matches". Without this split, a - /// dropped connection rendered as "export from the app first", telling the - /// user to redo something that had worked (GitHub #86). + /// dropped connection would read as "export from the app first", telling + /// the user to redo something that had worked (GitHub #86). pub unsearched: Vec, } diff --git a/v2/crates/core/src/commands/health.rs b/v2/crates/core/src/commands/health.rs index 53cf17f0..3343717c 100644 --- a/v2/crates/core/src/commands/health.rs +++ b/v2/crates/core/src/commands/health.rs @@ -284,11 +284,10 @@ async fn health_report_for(state: &AppState, serial: &str) -> Result Result Result, String> { // Installed + disabled package lists and HOME handlers in one round-trip. - // The old `tokio::join!` bought no concurrency on the mobile transport, - // which serializes every shell behind one connection. + // Separate reads would buy no concurrency on the mobile transport, which + // serializes every shell behind one connection. let adb = state.adb_snapshot().await; let cmd = crate::adb::checked_batch_command(&[ "pm list packages", @@ -77,7 +77,7 @@ pub async fn list_launchers_impl( } // HOME handlers only. Every TV app's launch activity declares // LEANBACK_LAUNCHER — that category is how an app gets a tile on the home - // screen, not a claim to *be* the home screen — so listing it turned + // screen, not a claim to *be* the home screen — so listing it would turn // YouTube, Plex and the Play Store into "Home apps". An app that doesn't // declare HOME can still be tried from the Advanced picker, which says // honestly when Android won't accept it. @@ -1610,8 +1610,8 @@ async fn stock_takeover( // with the last poll), a transient holder still settling, or some // third app — means disabling stock is not what's keeping the switch // from landing, so restoring it would only undo real progress - // (GitHub #122: the onn 4K Pro reported this as a failure and rolled - // stock back even though the switch had actually worked). + // (GitHub #122: on the onn 4K Pro that reports a working switch as a + // failure and rolls stock back). if let Some(reason) = paired_failure { reason } else if let Some(holder) = verification @@ -1726,7 +1726,7 @@ fn command_failure(result: &crate::adb::AdbResult) -> Opt let detail = command_output_detail(output); // `{:?}` on an Option renders as `Some(13)` / `None`, and this // string goes straight into the launcher error banner and a - // confirm() body. Users were reading "exit code 13". + // confirm() body, so it must read as a bare number. let code = match output.exit_code { Some(code) => format!("exit code {code}"), None => "no exit code".to_string(), @@ -1752,8 +1752,8 @@ fn command_output_detail(output: &crate::adb::AdbOutput) -> String { /// `cmd package set-home-activity` / `pm set-home-activity` acknowledge with a /// bare "Success" line on many builds (and stay silent on others). That's an -/// acceptance, not a diagnostic — surfacing it as an error produced the -/// infamous "Failed: Success" message. +/// acceptance, not a diagnostic — surfacing it as an error would read +/// "Failed: Success". fn is_success_ack(s: &str) -> bool { s.trim().eq_ignore_ascii_case("success") } @@ -1775,8 +1775,8 @@ async fn focus_home(adb: &dyn crate::adb::AdbDriver, serial: &str, progress: &Pr /// Backoff schedule for [`verify_active`], summing to ~5s — long enough for /// Android to settle after a role/set-home-activity change on real hardware -/// (GitHub #122: the onn 4K Pro hadn't finished switching within the old -/// ~1.6s window). Tiny under `cfg(test)` so the polling loop doesn't make the +/// (GitHub #122: the onn 4K Pro can take longer than ~1.6s to finish +/// switching). Tiny under `cfg(test)` so the polling loop doesn't make the /// test suite slow. #[cfg(not(test))] fn verify_poll_delays() -> &'static [u64] { @@ -1801,7 +1801,7 @@ struct VerifyOutcome { /// Poll the active-HOME resolver — and the HOME role's holder list — until /// one names `package`, with backoff. Propagation after set-home-activity / /// role changes isn't instant on every build — a single immediate check -/// produced false "failed" results even when the device had accepted the +/// reports false "failed" results even when the device has accepted the /// change. /// /// A device may hand HOME through a transient holder while it settles on the @@ -1904,8 +1904,6 @@ async fn active_launcher(adb: &dyn crate::adb::AdbDriver, serial: &str) -> Optio .map(|(p, _)| p.to_string()) } -/// Find a HOME activity for `package` via `cmd package query-activities`. -/// Returns a `pkg/activity` component string ready for set-home-activity. /// Components to try registering as the HOME activity, best first. /// /// The discovered component is authoritative when we can get one, but @@ -1968,8 +1966,8 @@ async fn try_home_setters( }; // set-home-activity prints a bare "Success" on many builds and // nothing on others — both are acceptance, NOT diagnostics. Only - // real error text goes into last_error; recording the ack produced - // "Failed: Success". + // real error text goes into last_error; recording the ack would + // read "Failed: Success". let msg = if out.stderr.trim().is_empty() { out.stdout.trim() } else { @@ -1990,6 +1988,8 @@ async fn try_home_setters( outcome } +/// Find a HOME activity for `package` via `cmd package query-activities`. +/// Returns a `pkg/activity` component string ready for set-home-activity. async fn discover_home_activity( adb: &dyn crate::adb::AdbDriver, serial: &str, @@ -2608,8 +2608,8 @@ mod tests { async fn a_leanback_only_app_is_not_listed_as_a_home_app() { // Every TV app (YouTube, Plex, the Play Store) declares // LEANBACK_LAUNCHER so it gets a tile on the home screen. Treating that - // as a Home handler flooded the Launcher tab with "HOME APP" rows for - // ordinary apps. It is installed and enabled, but it must not get a row, + // as a Home handler would flood the Launcher tab with "HOME APP" rows + // for ordinary apps. It is installed and enabled, but it must not get a row, // and the leanback category must not be queried at all. let mock = MockAdb::default().on_shell( BATCH_SEPARATOR, @@ -2876,12 +2876,12 @@ mod tests { /// GitHub #87 — Sony XBR-55X850D, Android 8.0, stock holding HOME. /// /// `cmd package query-activities` is Android 9+ and `cmd role` is Android - /// 10+, so on this device both come back "Unknown command". The stock fast - /// path used to call set-home-activity only for a component - /// `query-activities` had found, which meant no setter ran at all and the - /// first thing that actually happened was disabling the stock launcher — - /// handing HOME to a package nothing had registered. It has to try the - /// conventional component names before it touches stock. + /// 10+, so on this device both come back "Unknown command". Calling + /// set-home-activity only for a component `query-activities` found would + /// mean no setter runs at all and the first thing that happens is disabling + /// the stock launcher — handing HOME to a package nothing has registered. + /// The stock fast path has to try the conventional component names before + /// it touches stock. #[tokio::test] async fn android_8_registers_home_before_disabling_stock() { let mock = MockAdb::default() @@ -3060,9 +3060,9 @@ mod tests { ); } - /// GitHub #122 — onn 4K Pro: setting Projectivy as default reported a - /// failure and rolled stock back even though the switch had actually - /// worked. Three cases from the issue's polling/rollback fix. + /// GitHub #122 — onn 4K Pro: setting Projectivy as default must not report + /// a failure and roll stock back when the switch has worked. Three + /// polling/rollback cases. mod launcher_verification_122 { use super::*; @@ -3602,7 +3602,7 @@ mod tests { #[tokio::test] async fn rolls_back_only_when_stock_positively_still_holds_home() { - // The negative case this whole fix has to preserve: if stock + // The negative case the polling has to preserve: if stock // genuinely never let go of HOME, that's a real failure and // stock must be restored. let mock = MockAdb::default() diff --git a/v2/crates/core/src/commands/loader.rs b/v2/crates/core/src/commands/loader.rs index 44841945..3e31836a 100644 --- a/v2/crates/core/src/commands/loader.rs +++ b/v2/crates/core/src/commands/loader.rs @@ -12,8 +12,7 @@ use serde::Deserialize; use crate::engine::{AppListBundle, LauncherCatalog}; /// Embedded JSON for the three default app lists. Loaded at compile time so -/// the binary works offline. Future versions will additionally check a -/// versioned URL and prefer fresher copies; that goes here. +/// the binary works offline. const COMMON_JSON: &str = include_str!("../../data/app-lists/common.json"); const SHIELD_JSON: &str = include_str!("../../data/app-lists/shield.json"); const GOOGLETV_JSON: &str = include_str!("../../data/app-lists/googletv.json"); @@ -169,9 +168,8 @@ mod tests { assert!(!entry.optimize_description.contains("No effect")); } - /// The shipped lists, not a fixture. This is the assertion that would have - /// caught the regression where 82 of 89 curated apps reported "role and - /// effects unknown" beside their own description. + /// The shipped lists, not a fixture: no curated app may report "role and + /// effects unknown" beside its own description. #[test] fn every_shipped_catalog_entry_gets_a_reviewed_verdict() { let bundle = load_embedded_app_lists().expect("embedded lists parse"); @@ -284,9 +282,8 @@ mod tests { /// lists `AppListBundle::for_device` knows about. const DEVICE_SCOPES: &[&str] = &["shield", "googletv"]; - /// Safe entries with no `reviewed_at`. Every entry was dated from git - /// history when the field was introduced (#101), so this is empty; a new - /// safe entry goes in with a date, not on this list. + /// Safe entries with no `reviewed_at` (#101). Empty: a new safe entry goes + /// in with a date, not on this list. const SAFE_WITHOUT_REVIEW_DATE: &[&str] = &[]; fn named_lists(bundle: &AppListBundle) -> [(&'static str, &[crate::engine::AppEntry]); 3] { @@ -420,7 +417,7 @@ mod tests { } /// The shipped launcher catalog, not a fixture — the engine takes the - /// catalog as an argument now, so this file is the only place the real + /// catalog as an argument, so this file is the only place the real /// entries are checked. #[test] fn embedded_launchers_parse_with_every_shipped_entry() { @@ -428,8 +425,7 @@ mod tests { assert_eq!(cat.custom.len(), 7, "custom launcher count"); assert_eq!(cat.stock.len(), 4, "stock launcher count"); - // Critical correctness: the Dispatch package name change from v1's - // launcher selection fix. + // Dispatch's real package id; a wrong one breaks launcher selection. let dispatch = cat .custom .iter() diff --git a/v2/crates/core/src/commands/optimize.rs b/v2/crates/core/src/commands/optimize.rs index 05c97d2d..60c0aa8c 100644 --- a/v2/crates/core/src/commands/optimize.rs +++ b/v2/crates/core/src/commands/optimize.rs @@ -26,10 +26,9 @@ use super::AppState; /// pick the device-appropriate app list, and run the engine planner. /// /// `device_type` comes from the caller (the page already profiled the device on -/// load) instead of re-detecting here. Re-detection went through -/// `list_devices_impl`, which sequentially harvests properties for *every* -/// connected device — slow with several devices, and worse when an unauthorized -/// one stalls. The planner only needs the type, so the caller passes it. +/// load) instead of re-detecting here. Re-detecting through +/// `list_devices_impl` would harvest properties for *every* connected device — +/// slow with several devices, and worse when an unauthorized one stalls. #[tauri::command] pub async fn prepare_optimize( state: State<'_, AppState>, @@ -52,8 +51,8 @@ pub async fn prepare_optimize_impl( let adb = state.adb_snapshot().await; // One round-trip: the mobile transport serializes concurrent shells behind - // a single connection, so the old `tokio::join!` cost three Wi-Fi RTTs for - // no concurrency. + // a single connection, so three separate reads would cost three Wi-Fi RTTs + // for no concurrency. let cmd = checked_batch_command(&["pm list packages", "pm list packages -d", "dumpsys meminfo"]); let out = adb @@ -199,7 +198,7 @@ mod tests { shield: vec![], googletv: vec![], }; - // All three reads ride one batched shell now, so the mock answers the + // All three reads ride one batched shell, so the mock answers the // sentinel with the concatenated sections (installed / disabled / // meminfo) instead of one rule per sub-command — a per-command needle // would match the whole compound command. diff --git a/v2/crates/core/src/commands/snapshot.rs b/v2/crates/core/src/commands/snapshot.rs index 77ccb3ff..71558050 100644 --- a/v2/crates/core/src/commands/snapshot.rs +++ b/v2/crates/core/src/commands/snapshot.rs @@ -424,8 +424,8 @@ pub async fn apply_snapshot( // 1. Disable packages from the plan (additive only — never re-enable). // A stock launcher with a paired setup helper (Google TV's Setup Wraith) // needs the Home apps read before it goes off, so the helper can follow - // once Home is settled (#159). Snapshots taken before that fix list stock - // without its helper. + // once Home is settled (#159). Snapshots from older app versions list + // stock without its helper. let paired_stocks: Vec = plan .packages_to_disable .iter() @@ -587,10 +587,10 @@ async fn apply_settings_from_plan( let mut settings_written = Vec::new(); let mut settings_deleted = Vec::new(); let mut settings_failed = Vec::new(); - // Apply one setting per shell call. Batching with `;` meant a single failing - // `settings put` (e.g. a SecurityException on a protected key) was invisible - // — `adb shell` still exits 0, so the old code reported every key written. - // Per-key lets us report exactly which succeeded, and check the output. + // Apply one setting per shell call. In a `;` batch a single failing + // `settings put` (e.g. a SecurityException on a protected key) is invisible + // — `adb shell` exits 0 even when the write fails, so success is read back + // per key rather than assumed for the batch. let writes = plan .settings_to_write .iter() diff --git a/v2/crates/core/src/commands/state.rs b/v2/crates/core/src/commands/state.rs index 560fcab7..65022505 100644 --- a/v2/crates/core/src/commands/state.rs +++ b/v2/crates/core/src/commands/state.rs @@ -75,8 +75,7 @@ impl AppState { } } - /// Attach the curated package→name map. Builder-style so the existing - /// constructors (and their test callers) stay unchanged. + /// Attach the curated package→name map. pub fn with_known_names>( mut self, known_names: HashMap, @@ -190,8 +189,8 @@ impl AppState { } /// Inject a single key-down / key-up via the live session. Errors if no - /// session exists — Phase 3 calls `ensure_remote_session` first, and on a - /// write error should `drop_remote_session` and fall back to `input`. + /// session exists — callers run `ensure_remote_session` first, and on a + /// write error `drop_remote_session` and fall back to `input`. pub async fn remote_send_key( &self, serial: &str, @@ -268,8 +267,8 @@ fn entitlement_from_u8(value: u8) -> Entitlement { /// Tauri resource directory (production install); falls back to the /// crate-relative `resources/` dir for `cargo run` / `cargo test`. /// -/// Phase 3: the remote-input command calls this with its `AppHandle` to get the -/// jar path, then hands it to `AppState::ensure_remote_session`. +/// The remote-input command calls this with its `AppHandle` to get the jar +/// path, then hands it to `AppState::ensure_remote_session`. pub fn resolve_scrcpy_server_jar( app: &tauri::AppHandle, ) -> Result { diff --git a/v2/crates/core/src/commands/tuning.rs b/v2/crates/core/src/commands/tuning.rs index 0a7f8c3d..29f62541 100644 --- a/v2/crates/core/src/commands/tuning.rs +++ b/v2/crates/core/src/commands/tuning.rs @@ -25,10 +25,10 @@ pub struct TweaksState { pub window_animation_scale: Option, pub transition_animation_scale: Option, pub animator_duration_scale: Option, - /// `global.background_process_limit`, which earlier versions wrote. Android - /// does not read this key: on a Shield TV (Android 11) setting it to 1 - /// left the cached-process limit at 32 (#99). Read only so the UI can - /// offer to remove a leftover value. + /// `global.background_process_limit`. Android does not read this key: on a + /// Shield TV (Android 11) setting it to 1 left the cached-process limit at + /// 32 (#99). Read only so the UI can offer to remove a value an older app + /// version left behind. pub background_process_limit: Option, /// The limit Android is actually applying to cached background processes, /// `CUR_MAX_CACHED_PROCESSES` from `dumpsys activity settings`. Developer @@ -214,8 +214,7 @@ pub async fn get_display_scaling( serial: String, ) -> Result { let adb = state.adb_snapshot().await; - // One round-trip; both sections still degrade to an empty string, which is - // what the individual failures produced. + // One round-trip; a failed read degrades its section to an empty string. let cmd = crate::adb::batch_command(&["wm size", "wm density"]); let out = adb .shell(&serial, &cmd) @@ -231,11 +230,10 @@ pub async fn get_display_scaling( #[derive(Debug, Clone, Copy, serde::Deserialize)] pub enum DisplayScalePreset { - // Explicit renames: `rename_all = "snake_case"` produces `uhd4k` / - // `fhd1080p` (no underscore before the digit), which mismatched the - // frontend's `uhd_4k` / `fhd_1080p` and made every scaling click fail with - // "unknown variant `uhd_4k`". Keep in lockstep with DisplayScalePreset in - // src/lib/types.ts. + // Explicit renames: `rename_all = "snake_case"` would produce `uhd4k` / + // `fhd1080p` (no underscore before the digit), which doesn't match the + // frontend's `uhd_4k` / `fhd_1080p`. Keep in lockstep with + // DisplayScalePreset in src/lib/types.ts. /// 3839x2160 @ density 640. Shield TV won't accept 3840 width, and density /// 540 breaks some app menus (Disney+, HBO) — see issue #24. #[serde(rename = "uhd_4k")] @@ -626,7 +624,7 @@ mod tests { #[test] fn value_metacharacters_are_quoted_not_rejected() { - // Values containing shell metacharacters are now quoted rather than + // Values containing shell metacharacters are quoted rather than // rejected — the device shell cannot interpret them inside single quotes. assert!(build_setting_command("global", "k", "1; rm -rf /").is_ok()); assert!(build_setting_command("global", "k", "a|b").is_ok()); diff --git a/v2/crates/core/src/engine/app_lists.rs b/v2/crates/core/src/engine/app_lists.rs index 2f4e1e03..2a5d1f23 100644 --- a/v2/crates/core/src/engine/app_lists.rs +++ b/v2/crates/core/src/engine/app_lists.rs @@ -1,5 +1,5 @@ //! App list loading — engine-side type definitions and merging logic. -//! No I/O here; the host layer loads JSON from disk (or remote) and passes it in. +//! No I/O here; the host layer loads the embedded JSON and passes it in. use std::collections::HashSet; @@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize}; use super::detection::DeviceType; use super::types::AppEntry; -/// A named app list — one of common / shield / googletv (or any user/community add). +/// A named app list — one of common / shield / googletv. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct AppList { pub name: String, @@ -160,8 +160,8 @@ mod tests { #[test] fn duplicate_packages_within_a_list_are_collapsed() { - // A stray repeat inside one list (the catalog regression that blanked the - // App List + Optimize tables) must dedup, keeping the first occurrence. + // A stray repeat inside one list must dedup, keeping the first + // occurrence; a duplicate key blanks the App List and Optimize tables. let bundle = AppListBundle { common: vec![ entry("tv.pluto.android", "Pluto TV"), diff --git a/v2/crates/core/src/engine/detection.rs b/v2/crates/core/src/engine/detection.rs index 7d037994..16c63b4d 100644 --- a/v2/crates/core/src/engine/detection.rs +++ b/v2/crates/core/src/engine/detection.rs @@ -33,8 +33,8 @@ impl DeviceType { /// /// Deliberately three-valued. `DeviceType::Unknown` answers "which app list /// applies", which is not the same question — a TV nobody has catalogued is -/// still a TV, and saying "not an Android TV" about it was the 2.2.0 -/// regression (#120). Unreadable means unknown, and unknown claims nothing. +/// still a TV and must never be called "not an Android TV" (#120). +/// Unreadable means unknown, and unknown claims nothing. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum TvEvidence { @@ -251,10 +251,10 @@ mod tests { ); } - /// The #120 regression, in one test: a Xiaomi TV Box S reports - /// `nosdcard` — not `tv` — in `ro.build.characteristics`, and 2.2.0 read - /// that as "not an Android TV" and locked the user out of their own box. - /// The leanback feature is the second signal that settles it. + /// #120: a Xiaomi TV Box S reports `nosdcard` — not `tv` — in + /// `ro.build.characteristics`. Reading that as "not an Android TV" locks + /// the user out of their own box; the leanback feature is the second + /// signal that settles it. #[test] fn leanback_alone_is_enough_to_call_it_a_tv() { let props = DeviceProperties { diff --git a/v2/crates/core/src/engine/safety.rs b/v2/crates/core/src/engine/safety.rs index 0214df4d..d10d46c8 100644 --- a/v2/crates/core/src/engine/safety.rs +++ b/v2/crates/core/src/engine/safety.rs @@ -14,12 +14,12 @@ //! - `Unknown` — nothing reviewed covers this package, so we genuinely do not //! know what it does. //! -//! The catalog is a reviewed source. Ignoring it — which this module did until -//! the lists and the rule tables were connected — meant 82 of 89 hand-curated -//! apps reported "role and effects unknown" while the row beside them displayed -//! the role and the effect. It also made `Unknown` meaningless: when almost -//! everything is unknown, a genuinely unrecognised sideloaded APK no longer -//! stands out, which is exactly what the verdict exists to do. +//! The catalog is a reviewed source, so it is consulted. Ignoring it would +//! report hand-curated apps as "role and effects unknown" while the row beside +//! them displays the role and the effect. It would also make `Unknown` +//! meaningless: when almost everything is unknown, a genuinely unrecognised +//! sideloaded APK no longer stands out, which is exactly what the verdict +//! exists to do. use serde::Serialize; @@ -665,8 +665,8 @@ mod tests { #[test] fn a_reviewed_safe_app_is_safe_not_unknown() { use crate::engine::types::RiskTier; - // The regression: 82 of 89 curated apps reported "role and effects - // unknown" while their own description sat in the next column. + // A curated app must not report "role and effects unknown" while its + // own description sits in the next column. let verdict = classify_with_catalog( "com.android.gallery3d", Some(catalog( @@ -678,7 +678,7 @@ mod tests { Safety::Safe { reason, source } => { assert_eq!(source, SafetySource::ReviewedCatalog); // The curated sentence is the whole reason; the label carries - // "Safe to remove", so repeating it here said it twice. + // "Safe to remove", so repeating it here would say it twice. assert_eq!(reason, "Photo gallery — rarely used on TV."); } other => panic!("expected Safe, got {other:?}"), @@ -689,7 +689,7 @@ mod tests { fn a_risky_reviewed_app_is_caution_and_says_why() { use crate::engine::types::RiskTier; // com.google.android.tvlauncher: risk "high", and the app has a whole - // guarded wizard for it. Reporting "effects unknown" was plainly false. + // guarded wizard for it. Reporting "effects unknown" would be false. let verdict = classify_with_catalog( "com.google.android.tvlauncher", Some(catalog( @@ -759,8 +759,7 @@ mod tests { #[test] fn safe_is_never_reached_without_a_catalog_entry() { - // Safe must never be a fallback. That was the original sin the Unknown - // change set out to fix, and it must not come back through this door. + // Safe must never be a fallback, including through this door. for package in [ "com.example.anything", "com.netflix.ninja", diff --git a/v2/crates/core/src/engine/snapshot.rs b/v2/crates/core/src/engine/snapshot.rs index df165557..2b8ecb5e 100644 --- a/v2/crates/core/src/engine/snapshot.rs +++ b/v2/crates/core/src/engine/snapshot.rs @@ -41,7 +41,7 @@ pub struct Snapshot { /// ISO-8601 UTC timestamp. pub saved_at: String, /// Optional user-given name for the snapshot (e.g. "before debloat"). - /// Added in schema v2 — `serde(default)` loads v1 snapshots with `None`. + /// Schema v1 snapshots have none; `serde(default)` loads them with `None`. #[serde(default)] pub label: Option, pub device_name: String, @@ -166,7 +166,7 @@ pub struct ApplyPlanInputs<'a> { } /// Compute the plan for applying `snap` to a device in `inputs`' state. -/// Per commitment #2: this function is pure; the host layer executes the plan. +/// Per commitment #1: this function is pure; the host layer executes the plan. pub fn compute_apply_plan(snap: &Snapshot, inputs: &ApplyPlanInputs<'_>) -> SnapshotApplyPlan { let disabled_set: std::collections::HashSet<&str> = inputs .currently_disabled diff --git a/v2/crates/core/src/engine/types.rs b/v2/crates/core/src/engine/types.rs index 69dfd0d1..57687353 100644 --- a/v2/crates/core/src/engine/types.rs +++ b/v2/crates/core/src/engine/types.rs @@ -252,7 +252,7 @@ pub enum SkipReason { pub struct OptimizePlanItem { pub entry: AppEntry, pub action: OptimizeAction, - /// Bytes of RAM currently in use by this package's processes, if running. + /// MB of RAM currently in use by this package's processes, if running. #[serde(skip_serializing_if = "Option::is_none")] pub memory_mb: Option, } diff --git a/v2/crates/core/src/lib.rs b/v2/crates/core/src/lib.rs index ffdfe8e6..4e17b49d 100644 --- a/v2/crates/core/src/lib.rs +++ b/v2/crates/core/src/lib.rs @@ -1,9 +1,9 @@ //! Shared Shield Optimizer / ATV Optimizer core. //! //! This crate owns the pure safety engine, driver-generic ADB abstractions, and -//! shared Tauri command handlers used by the desktop and future Android app. +//! shared Tauri command handlers used by the desktop and Android apps. -// clippy 1.99 added `double_must_use`, which fires on the futures that +// clippy's `double_must_use` fires on the futures that // `#[async_trait]` generates for every trait method. The attribute is the // macro's, not ours, so there is nothing to change at the call sites. #![allow(clippy::double_must_use)] diff --git a/v2/mobile/src-tauri/src/file_commands.rs b/v2/mobile/src-tauri/src/file_commands.rs index b7a6b463..f8f2ef38 100644 --- a/v2/mobile/src-tauri/src/file_commands.rs +++ b/v2/mobile/src-tauri/src/file_commands.rs @@ -14,8 +14,9 @@ //! no power-user mode, so the system-path override is never offered. //! //! All device I/O rides the shared `AdbDriver` seam (shell + `raw_transfer`), -//! keeping `crates/core/engine` pure. The transfer itself is implemented by -//! `WirelessAdb::raw_transfer` via `adb_client`'s sync service. +//! keeping `crates/core/engine` pure. Transfers are implemented by +//! `WirelessAdb::pull_limited` and `WirelessAdb::raw_transfer` via +//! `adb_client`'s sync service. use std::path::{Path, PathBuf}; @@ -239,10 +240,10 @@ pub async fn list_remote_dir( Ok(entries) } -/// Hard cap on one pulled file — 2 GiB. `AdbDriver` exposes no stat, so the -/// remote size isn't knowable up front; the limit is enforced after the -/// transfer by deleting the oversized file rather than leaving it to fill the -/// phone's app storage. +/// Hard cap on one pulled file — 2 GiB. `pull_limited` refuses a larger file +/// before streaming and stops a growing one at the cap, and the size is checked +/// again before the download is kept, so it never fills the phone's app +/// storage. const MAX_PULL_BYTES: u64 = 2 * 1024 * 1024 * 1024; const BYTES_PER_GIB: f64 = (1024 * 1024 * 1024) as f64; diff --git a/v2/mobile/src-tauri/src/lib.rs b/v2/mobile/src-tauri/src/lib.rs index c9b5a3f0..f5635fdf 100644 --- a/v2/mobile/src-tauri/src/lib.rs +++ b/v2/mobile/src-tauri/src/lib.rs @@ -4,7 +4,7 @@ //! builds compile the same code with a stub transport so CI can validate the //! Rust command surface before Android hardware is available. -// clippy 1.99 added `double_must_use`, which fires on the futures that +// clippy's `double_must_use` fires on the futures that // `#[async_trait]` generates for every trait method. The attribute is the // macro's, not ours, so there is nothing to change at the call sites. #![allow(clippy::double_must_use)] @@ -130,7 +130,7 @@ fn default_data_dir() -> PathBuf { .join("ATVOptimizer") } -/// Install the tracing subscriber: logcat/stdout as before, PLUS a plain-text +/// Install the tracing subscriber: logcat/stdout plus a plain-text /// file layer at `data_dir/debug.log` that the More screen can surface. Called /// once from `setup`, after the data dir is known. fn init_logging(data_dir: &Path) { diff --git a/v2/mobile/src-tauri/src/wireless_adb.rs b/v2/mobile/src-tauri/src/wireless_adb.rs index c9414f36..3d4c7ac0 100644 --- a/v2/mobile/src-tauri/src/wireless_adb.rs +++ b/v2/mobile/src-tauri/src/wireless_adb.rs @@ -96,7 +96,8 @@ fn validate_requested_serial(active: &str, requested: &str) -> Result<(), String /// phone, so those are explicit methods the mobile-only `wireless_*` commands /// call; the trait surface synthesizes `raw(["devices"])`, routes /// `shell`/screencap through the owned device, maps `reboot`, and streams -/// `push`/`pull` through the sync service via `raw_transfer`. `pair` runs the +/// `push`/`pull` through the sync service via `raw_transfer` and +/// `pull_limited`. `pair` runs the /// Android-11 SPAKE2 code-pairing exchange in-crate; `forward` is structurally /// impossible on a phone and reports `Unsupported`. pub struct WirelessAdb { diff --git a/v2/src-tauri/src/adb/driver.rs b/v2/src-tauri/src/adb/driver.rs index 44936f03..7fb547c3 100644 --- a/v2/src-tauri/src/adb/driver.rs +++ b/v2/src-tauri/src/adb/driver.rs @@ -47,7 +47,7 @@ impl SubprocessAdb { discover_adb_binary().map(Self::new) } - /// Back-compat alias for the old PATH-only discovery. + /// Alias for [`Self::discover`]: searches every location, not only PATH. pub fn from_path() -> Option { Self::discover() } @@ -244,12 +244,6 @@ fn redact_stdout_for_log(command_text: &str, stdout: &str) -> String { } } -/// Strip the pairing PIN out of an argument list before it is logged. -/// -/// `adb pair ` is the one call this app makes that carries a -/// secret the user read off their own screen. Debug logging is meant to be -/// pasted into a public issue, so the PIN must never reach the file in the -/// first place — redacting at the read end would be too late. /// Typed text rides as `input text ''`, and that payload is whatever /// the user typed or pasted into Remote — passwords and URLs included. The /// debug log feeds the bug-report bundle, so the payload never reaches it. @@ -260,6 +254,12 @@ fn redact_typed_text(arg: &str) -> String { } } +/// Strip the pairing PIN out of an argument list before it is logged. +/// +/// `adb pair ` is the one call this app makes that carries a +/// secret the user read off their own screen. Debug logging is meant to be +/// pasted into a public issue, so the PIN must never reach the file in the +/// first place — redacting at the read end would be too late. fn redact_args(args: &[&str]) -> Vec { let Some(pair_at) = args.iter().position(|a| *a == "pair") else { return args.iter().map(|a| redact_typed_text(a)).collect(); @@ -684,8 +684,8 @@ fn custom_server_target(var: impl Fn(&str) -> Option) -> Option<&'stat /// "not found". Discovery is not free and, on a machine with no adb installed, /// its last resort is probing the launch directory — which is a removable /// volume when the app was opened straight from its DMG. `adb_status` runs on -/// every Devices refresh, so an uncached lookup re-probed that volume every -/// few seconds and kept macOS asking for permission (GitHub #89). +/// every Devices refresh, so an uncached lookup would re-probe that volume +/// every few seconds and keep macOS asking for permission (GitHub #89). static CACHED_ADB_PATH: std::sync::RwLock>> = std::sync::RwLock::new(None); /// [`discover_adb_binary`], resolved at most once per process until @@ -732,8 +732,8 @@ pub fn forget_cached_adb_binary() { /// - Linux: `/usr/bin/adb`, `/usr/local/bin/adb`, /// `~/Android/Sdk/platform-tools/adb` /// - Windows: `%LOCALAPPDATA%\Android\Sdk\platform-tools\adb.exe` -/// 6. Repo-local fallback for v1 coexistence: `../adb` relative to the -/// Cargo workspace, since the v1 repo ships an adb binary there. +/// 6. Repo-local fallback for v1 coexistence: `adb` in the launch directory +/// or its parent, where v1 downloads its adb binary (the repo root). pub fn discover_adb_binary() -> Option { let sources = AdbDiscoverySources { explicit_override: std::env::var_os("SHIELD_OPTIMIZER_ADB").map(PathBuf::from), @@ -798,7 +798,7 @@ where } } - // Repo-local fallback: the v1 repo ships `./adb` at the top level. Only + // Repo-local fallback: v1 downloads `adb` into the repo root. Only // inspect the launch CWD after every installed location has failed. if let Some(cwd) = sources.cwd { for candidate in [Some(cwd.join("adb")), cwd.parent().map(|p| p.join("adb"))] @@ -1402,10 +1402,10 @@ mod tests { ); } - /// `adb_status` runs on every Devices refresh. Before memoization that - /// meant a full re-walk — PATH included, launch directory included — every - /// few seconds on a machine with no adb installed, which is what kept - /// macOS re-asking for removable-volume access (GitHub #89). + /// `adb_status` runs on every Devices refresh. Without memoization that + /// is a full re-walk — PATH included, launch directory included — every + /// few seconds on a machine with no adb installed, which keeps macOS + /// re-asking for removable-volume access (GitHub #89). #[test] fn repeated_discovery_resolves_once_until_explicitly_forgotten() { use std::sync::atomic::{AtomicUsize, Ordering}; diff --git a/v2/src-tauri/src/adb/install.rs b/v2/src-tauri/src/adb/install.rs index 109cd156..0f45083f 100644 --- a/v2/src-tauri/src/adb/install.rs +++ b/v2/src-tauri/src/adb/install.rs @@ -207,7 +207,6 @@ mod tests { let buf = writer.finish().unwrap(); let temp = tempfile::tempdir().unwrap_or_else(|_| { - // Fall back to a known-good temp path if tempfile isn't available. panic!("could not get tempdir"); }); let cursor = std::io::Cursor::new(buf.into_inner()); diff --git a/v2/src-tauri/src/adb/scan.rs b/v2/src-tauri/src/adb/scan.rs index 73681dea..0e565694 100644 --- a/v2/src-tauri/src/adb/scan.rs +++ b/v2/src-tauri/src/adb/scan.rs @@ -257,8 +257,7 @@ mod tests { #[test] fn parses_linux_le_hex_ip() { - // 192.168.42.1 → little-endian hex bytes: 0xC0,0xA8,0x2A,0x01 → "0102A8C0" (reversed). - // Actually /proc/net/route stores it as the bytes appear in memory, + // /proc/net/route stores the address as the bytes appear in memory, // which on LE is host-low-byte first: 192.168.42.1 → 01 2A A8 C0 → "012AA8C0" let parsed = parse_le_hex_ip("012AA8C0").unwrap(); assert_eq!(parsed.to_string(), "192.168.42.1"); diff --git a/v2/src-tauri/src/commands/diagnostics.rs b/v2/src-tauri/src/commands/diagnostics.rs index a778da5a..e192b27f 100644 --- a/v2/src-tauri/src/commands/diagnostics.rs +++ b/v2/src-tauri/src/commands/diagnostics.rs @@ -1,7 +1,7 @@ //! Logging to disk, and the bug-report bundle. //! -//! A packaged app has nowhere for stdout to go, so until now every `tracing` -//! line the app emitted was thrown away on the machines where it mattered. +//! A packaged app has nowhere for stdout to go, so without a file every +//! `tracing` line is lost on the machines where it matters. //! [`init_logging`] adds a rolling daily file under the app data dir and keeps //! its filter behind a `reload` handle, so "Debug logging" can be switched on //! from the UI without a restart — which is the only way someone reproducing a @@ -262,8 +262,8 @@ fn log_tail(dir: &Path, lines: usize) -> Vec { /// the packages it reported. Reuses the launcher tab's own parser rather than /// a second one: Android's ResolveInfo dump exposes the class under `name=` /// and the package separately under `packageName=` — `name=` has no slash, so -/// treating it as a flattened `pkg/activity` component silently dropped every -/// real handler. +/// treating it as a flattened `pkg/activity` component would drop every real +/// handler. fn home_handler_components(stdout: &str) -> Vec { launcher::parse_home_handler_packages(stdout) } diff --git a/v2/src-tauri/src/commands/quit.rs b/v2/src-tauri/src/commands/quit.rs index b968f1b8..4d1f4846 100644 --- a/v2/src-tauri/src/commands/quit.rs +++ b/v2/src-tauri/src/commands/quit.rs @@ -528,8 +528,8 @@ mod tests { use super::*; /// A stand-in adb that logs its arguments and claims, on every call, - /// to have started the server: the text that could once be spoofed - /// into an ownership claim. + /// to have started the server: text that must never be read as an + /// ownership claim. fn fake_adb(dir: &Path) -> (PathBuf, PathBuf) { let log = dir.join("calls"); let adb = dir.join("adb"); @@ -560,8 +560,8 @@ mod tests { AppListBundle::default(), dir.to_path_buf(), ); - // History that once read as "this app owns the server": a device - // shell printing the start message, and a restart. + // Call history that must not read as "this app owns the server": a + // device shell printing the start message, and a restart. let adb = state.adb_snapshot().await; adb.shell("tv", "echo '* daemon started successfully'") .await diff --git a/v2/src-tauri/src/commands/scan.rs b/v2/src-tauri/src/commands/scan.rs index 03e978cc..23b4a100 100644 --- a/v2/src-tauri/src/commands/scan.rs +++ b/v2/src-tauri/src/commands/scan.rs @@ -82,14 +82,6 @@ struct ScanTargets { needs_pairing: Vec, } -/// Merge mDNS advertisements with the raw `:5555` port sweep. -/// -/// Android 11+ wireless debugging listens on a random port that changes every -/// time it is toggled, so a `:5555` sweep cannot see it at all — which is what -/// "device not supported" actually meant in GitHub #88. mDNS knows the real -/// port, so when a host advertises one we use it and drop the swept `:5555` -/// guess for that same host. Legacy devices (Shield with Network debugging) -/// advertise `_adb._tcp` on 5555 or nothing at all, and keep working either way. /// Would dialling this service hand adb a *second* key for a device it already /// holds? /// @@ -99,7 +91,7 @@ struct ScanTargets { /// us the instance, `adb-58040DLCH005YV-jBeCEe`, so the attached serial is the /// instance plus a dot-prefixed service suffix. Connecting the same device /// again by `host:port` registers a second transport under a different key, -/// which is how one device came to occupy two rows. +/// which puts one device in two rows. /// /// Deliberately *not* checked here: a device already attached under the very /// endpoint we would dial. That reconnect is a no-op adb answers with "already @@ -115,6 +107,14 @@ fn would_duplicate_transport(service: &MdnsService, attached: &[String]) -> bool .any(|serial| serial == &service.instance || serial.starts_with(&instance_prefix)) } +/// Merge mDNS advertisements with the raw `:5555` port sweep. +/// +/// Android 11+ wireless debugging listens on a random port that changes every +/// time it is toggled, so a `:5555` sweep cannot see it at all — which is what +/// "device not supported" actually meant in GitHub #88. mDNS knows the real +/// port, so when a host advertises one we use it and drop the swept `:5555` +/// guess for that same host. Legacy devices (Shield with Network debugging) +/// advertise `_adb._tcp` on 5555 or nothing at all, and keep working either way. fn merge_scan_targets( swept_ips: &[String], services: &[MdnsService], @@ -298,7 +298,7 @@ pub async fn scan_network(state: State<'_, AppState>) -> Result