From c48b13f3048e85efa51b2a26bc78b45279537f5e Mon Sep 17 00:00:00 2001 From: PJ Date: Wed, 10 Jun 2026 21:19:30 +0530 Subject: [PATCH] fix(android): target the selected device in adb reads; don't strand nav mode Review fixes: - ForegroundPackage/FocusedWindowPackage now take a serial and pass -s, so the foreground/scope guard works when several devices are attached (the --device path). Previously they ran bare `adb shell`, which errors with multiple devices, silently disabling app-scope enforcement. The sidecar client passes its serial through. - Extract an adbArgs helper and route every adb call through it, removing four duplicated serial-arg builders. - ForceThreeButtonNav now decides what to restore before changing anything: if the current mode is unknown or already 3-button it leaves nav untouched, instead of switching and then stranding the device in 3-button. Logic split into the pure navModeToRestore, now unit tested. --- internal/android/android.go | 77 ++++++++++++++------------- internal/android/android_test.go | 86 ++++++++++++++++++++++++------- internal/driver/sidecar/client.go | 4 +- 3 files changed, 111 insertions(+), 56 deletions(-) diff --git a/internal/android/android.go b/internal/android/android.go index cc59c80..4a0f501 100644 --- a/internal/android/android.go +++ b/internal/android/android.go @@ -63,6 +63,17 @@ func EnsureDevice(ctx context.Context, serial, avdName string, stdout io.Writer) // Every step is best effort: some OEM builds restrict or kill these commands // (e.g. HyperOS SIGKILLs `svc power stayon`), and none is required for a run to // proceed, so a failure is logged and skipped rather than aborting the run. +// adbArgs prepends the device selector when a serial is set, so every adb +// invocation targets the chosen device. Without it, `adb` fails on a host with +// more than one device attached, which silently disables anything that reads +// adb output (the foreground/scope guard). +func adbArgs(serial string, args ...string) []string { + if serial == "" { + return args + } + return append([]string{"-s", serial}, args...) +} + func PrepareDevice(ctx context.Context, serial string, stdout io.Writer) error { adb, err := AdbBinary() if err != nil { @@ -73,11 +84,7 @@ func PrepareDevice(ctx context.Context, serial string, stdout io.Writer) error { {"input", "keyevent", "KEYCODE_WAKEUP"}, {"wm", "dismiss-keyguard"}, }, antiFreezeCommands()...) { - args := []string{} - if serial != "" { - args = append(args, "-s", serial) - } - args = append(append(args, "shell"), shellCommand...) + args := adbArgs(serial, append([]string{"shell"}, shellCommand...)...) if err := exec.CommandContext(ctx, adb, args...).Run(); err != nil { fmt.Fprintf(stdout, "device prep: skipping `adb %s` (%v)\n", strings.Join(shellCommand, " "), err) } @@ -121,16 +128,10 @@ func ReinstallApp(ctx context.Context, serial, bundleID, apkPath string, stdout if err != nil { return err } - withSerial := func(args ...string) []string { - if serial != "" { - return append([]string{"-s", serial}, args...) - } - return args - } - if output, err := exec.CommandContext(ctx, adb, withSerial("uninstall", bundleID)...).CombinedOutput(); err != nil { + if output, err := exec.CommandContext(ctx, adb, adbArgs(serial, "uninstall", bundleID)...).CombinedOutput(); err != nil { fmt.Fprintf(stdout, "clear-state: uninstall %s skipped (%v: %s)\n", bundleID, err, strings.TrimSpace(string(output))) } - if output, err := exec.CommandContext(ctx, adb, withSerial("install", "-r", apkPath)...).CombinedOutput(); err != nil { + if output, err := exec.CommandContext(ctx, adb, adbArgs(serial, "install", "-r", apkPath)...).CombinedOutput(); err != nil { return fmt.Errorf("install %s: %w: %s", apkPath, err, strings.TrimSpace(string(output))) } return nil @@ -157,30 +158,39 @@ func ForceThreeButtonNav(ctx context.Context, serial string, stdout io.Writer) f if err != nil { return func() {} } - original := enabledNavOverlay(ctx, adb, serial) + // Decide before changing anything: if the current mode is unknown (an OEM + // overlay, or a parse failure) or already 3-button, there is nothing to + // restore, so leave navigation untouched rather than stranding the device in + // 3-button after the run. + restore := navModeToRestore(enabledNavOverlay(ctx, adb, serial)) + if restore == "" { + return func() {} + } if err := navOverlayCommand(ctx, adb, serial, threeButtonNavOverlay).Run(); err != nil { fmt.Fprintf(stdout, "device prep: skipping 3-button nav (%v)\n", err) return func() {} } - if original == "" || original == threeButtonNavOverlay { - return func() {} - } return func() { - if err := navOverlayCommand(context.Background(), adb, serial, original).Run(); err != nil { - fmt.Fprintf(stdout, "device prep: could not restore nav mode %s (%v)\n", original, err) + if err := navOverlayCommand(context.Background(), adb, serial, restore).Run(); err != nil { + fmt.Fprintf(stdout, "device prep: could not restore nav mode %s (%v)\n", restore, err) } } } +// navModeToRestore returns the navigation overlay to restore after forcing +// 3-button nav, or "" when nothing should change: an unknown current mode (not +// restorable) or one that is already 3-button. +func navModeToRestore(original string) string { + if original == "" || original == threeButtonNavOverlay { + return "" + } + return original +} + // enabledNavOverlay returns the currently active navigation-mode overlay, or "" // when it cannot be determined. func enabledNavOverlay(ctx context.Context, adb, serial string) string { - args := []string{} - if serial != "" { - args = append(args, "-s", serial) - } - args = append(args, "shell", "cmd", "overlay", "list") - output, err := exec.CommandContext(ctx, adb, args...).Output() + output, err := exec.CommandContext(ctx, adb, adbArgs(serial, "shell", "cmd", "overlay", "list")...).Output() if err != nil { return "" } @@ -204,12 +214,7 @@ func parseEnabledNavOverlay(overlayList string) string { } func navOverlayCommand(ctx context.Context, adb, serial, overlay string) *exec.Cmd { - args := []string{} - if serial != "" { - args = append(args, "-s", serial) - } - args = append(args, "shell", "cmd", "overlay", "enable-exclusive", overlay) - return exec.CommandContext(ctx, adb, args...) + return exec.CommandContext(ctx, adb, adbArgs(serial, "shell", "cmd", "overlay", "enable-exclusive", overlay)...) } // AdbReverse sets up adb reverse forwarding for a local abstract socket. @@ -418,13 +423,13 @@ func waitForBoot(ctx context.Context, timeout time.Duration) error { } // ForegroundPackage returns the package of the currently resumed activity on -// the connected device, or "" when it cannot be determined. -func ForegroundPackage(ctx context.Context) (string, error) { +// the given device, or "" when it cannot be determined. +func ForegroundPackage(ctx context.Context, serial string) (string, error) { adb, err := AdbBinary() if err != nil { return "", err } - output, err := exec.CommandContext(ctx, adb, "shell", "dumpsys", "activity", "activities").Output() + output, err := exec.CommandContext(ctx, adb, adbArgs(serial, "shell", "dumpsys", "activity", "activities")...).Output() if err != nil { return "", err } @@ -436,7 +441,7 @@ func ForegroundPackage(ctx context.Context) (string, error) { // Unlike ForegroundPackage, this reflects what is actually on screen: // ResumedActivity flips to a newly launched app before its first frame renders, // while mCurrentFocus only names the app once its window is up. -func FocusedWindowPackage(ctx context.Context) (string, error) { +func FocusedWindowPackage(ctx context.Context, serial string) (string, error) { adb, err := AdbBinary() if err != nil { return "", err @@ -444,7 +449,7 @@ func FocusedWindowPackage(ctx context.Context) (string, error) { // Grep the focus line on-device: the full dumpsys window output is large and // this runs on the per-step scope guard, so transferring it whole would add // latency to every step. - output, err := exec.CommandContext(ctx, adb, "shell", "dumpsys window | grep mCurrentFocus").Output() + output, err := exec.CommandContext(ctx, adb, adbArgs(serial, "shell", "dumpsys window | grep mCurrentFocus")...).Output() if err != nil { return "", err } diff --git a/internal/android/android_test.go b/internal/android/android_test.go index 735ee27..508f76e 100644 --- a/internal/android/android_test.go +++ b/internal/android/android_test.go @@ -2,6 +2,7 @@ package android import ( "reflect" + "slices" "strings" "testing" ) @@ -191,6 +192,16 @@ func TestParseFocusedWindowPackage(t *testing.T) { dumpsys: " mCurrentFocus=Window{885e289 u0 NotificationShade}", want: "com.android.systemui", }, + { + name: "quick settings focused", + dumpsys: " mCurrentFocus=Window{abc u0 QuickSettings}", + want: "com.android.systemui", + }, + { + name: "volume dialog focused", + dumpsys: " mCurrentFocus=Window{abc u0 VolumeUiDialog}", + want: "com.android.systemui", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -217,11 +228,21 @@ func TestParseEnabledNavOverlay(t *testing.T) { listing: "[x] com.android.internal.systemui.navbar.threebutton\n[ ] com.android.internal.systemui.navbar.gestural", want: "com.android.internal.systemui.navbar.threebutton", }, + { + name: "two-button active", + listing: "[x] com.android.internal.systemui.navbar.twobutton\n[ ] com.android.internal.systemui.navbar.gestural", + want: "com.android.internal.systemui.navbar.twobutton", + }, { name: "ignores enabled non-nav overlays", listing: "[x] com.some.other.overlay\n[ ] com.android.internal.systemui.navbar.gestural", want: "", }, + { + name: "no overlay enabled", + listing: "[ ] com.android.internal.systemui.navbar.gestural\n[ ] com.android.internal.systemui.navbar.threebutton", + want: "", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -234,29 +255,58 @@ func TestParseEnabledNavOverlay(t *testing.T) { func TestAntiFreezeCommands_DisablesFreezersAndExemptsDriver(t *testing.T) { commands := antiFreezeCommands() - joined := make([]string, len(commands)) - for i, c := range commands { - joined[i] = strings.Join(c, " ") + has := func(want ...string) bool { + return slices.ContainsFunc(commands, func(c []string) bool { return slices.Equal(c, want) }) + } + for _, want := range [][]string{ + {"device_config", "set_sync_disabled_for_tests", "persistent"}, + {"device_config", "put", "activity_manager_native_boot", "use_freezer", "false"}, + {"settings", "put", "global", "settings_enable_monitor_phantom_procs", "false"}, + {"dumpsys", "deviceidle", "disable"}, + } { + if !has(want...) { + t.Errorf("anti-freeze commands missing exact command %v", want) + } } - all := strings.Join(joined, "\n") - wantContains := []string{ - "device_config set_sync_disabled_for_tests persistent", - "device_config put activity_manager_native_boot use_freezer false", - "settings put global settings_enable_monitor_phantom_procs false", - "dumpsys deviceidle disable", - } - for _, want := range wantContains { - if !strings.Contains(all, want) { - t.Errorf("anti-freeze commands missing %q\ngot:\n%s", want, all) - } + // The freezer exemption must be one device_config command whose final + // argument lists every driver package, not just the package string + // appearing somewhere among the commands. + exemption := findCommand(commands, "device_config", "put", "activity_manager_native_boot", "freeze_exempt_inst_pkg") + if exemption == nil { + t.Fatalf("no freeze_exempt_inst_pkg command found in %v", commands) } + value := exemption[len(exemption)-1] for _, pkg := range driverPackages { - if !strings.Contains(all, "freeze_exempt_inst_pkg") || !strings.Contains(all, pkg) { - t.Errorf("driver package %q not exempted from freezer\ngot:\n%s", pkg, all) + if !strings.Contains(value, pkg) { + t.Errorf("freeze_exempt_inst_pkg value %q missing driver package %q", value, pkg) } - if !strings.Contains(all, "deviceidle whitelist +"+pkg) { - t.Errorf("driver package %q not whitelisted from doze\ngot:\n%s", pkg, all) + if !has("dumpsys", "deviceidle", "whitelist", "+"+pkg) { + t.Errorf("driver package %q not whitelisted from doze", pkg) + } + } +} + +// findCommand returns the first command whose leading tokens equal prefix. +func findCommand(commands [][]string, prefix ...string) []string { + for _, c := range commands { + if len(c) >= len(prefix) && slices.Equal(c[:len(prefix)], prefix) { + return c + } + } + return nil +} + +func TestNavModeToRestore(t *testing.T) { + cases := map[string]string{ + "com.android.internal.systemui.navbar.gestural": "com.android.internal.systemui.navbar.gestural", + "com.android.internal.systemui.navbar.twobutton": "com.android.internal.systemui.navbar.twobutton", + "com.android.internal.systemui.navbar.threebutton": "", // already 3-button: nothing to change + "": "", // unknown current mode: must not switch what cannot be restored + } + for original, want := range cases { + if got := navModeToRestore(original); got != want { + t.Errorf("navModeToRestore(%q) = %q, want %q", original, got, want) } } } diff --git a/internal/driver/sidecar/client.go b/internal/driver/sidecar/client.go index 961747b..04583d3 100644 --- a/internal/driver/sidecar/client.go +++ b/internal/driver/sidecar/client.go @@ -54,7 +54,7 @@ func (c *Client) ForegroundApp(ctx context.Context) (string, error) { if c.platform != "android" { return "", nil } - return android.ForegroundPackage(ctx) + return android.ForegroundPackage(ctx, c.serial) } // FocusedWindowApp reports the package owning the focused window. Only Android @@ -64,7 +64,7 @@ func (c *Client) FocusedWindowApp(ctx context.Context) (string, error) { if c.platform != "android" { return "", nil } - return android.FocusedWindowPackage(ctx) + return android.FocusedWindowPackage(ctx, c.serial) } // Dial connects to the sidecar gRPC server at the given address.