Merge branch 'clear-state-must-happen' into correctness-and-spec-skills

internal/android/android.go

@@ -123,14 +123,17 @@ func antiFreezeCommands() [][]string {
 // reinstalling it. This replaces `pm clear` for clear-state: ColorOS and other
 // hardened OEM builds deny CLEAR_APP_USER_DATA even to the adb shell user, so a
 // clear aborts the launch, whereas uninstall+install is always permitted.
-// The uninstall is best effort so a not-installed app is not an error.
+// A failed uninstall is not passed over: `install -r` keeps the app's data, so
+// the reinstall would report a clear-state that never happened.
 func ReinstallApp(ctx context.Context, serial, bundleID, apkPath string, stdout io.Writer) error {
 	adb, err := AdbBinary()
 	if err != nil {
 		return err
 	}
 	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 err := clearDataUninstallLeftBehind(ctx, adb, serial, bundleID, strings.TrimSpace(string(output)), stdout); err != nil {
+			return err
+		}
 	}
 	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)))
@@ -138,6 +141,37 @@ func ReinstallApp(ctx context.Context, serial, bundleID, apkPath string, stdout
 	return nil
 }
 
+// clearDataUninstallLeftBehind reaches first-launch state after `adb uninstall`
+// failed. The failure text cannot say why: an API 34 emulator answers
+// "Failure [DELETE_FAILED_INTERNAL_ERROR]" both for a package that was never
+// installed and for one it refuses to remove. So ask the package manager which
+// happened. Nothing installed means nothing to clear. Still installed
+// means the data survives the reinstall, and `pm clear` is the one remaining
+// way to reach first-launch state; when that fails too, so does clear-state.
+func clearDataUninstallLeftBehind(ctx context.Context, adb, serial, bundleID, uninstallOutput string, stdout io.Writer) error {
+	if !packageInstalled(ctx, adb, serial, bundleID) {
+		return nil
+	}
+	output, err := exec.CommandContext(ctx, adb, adbArgs(serial, "shell", "pm", "clear", bundleID)...).CombinedOutput()
+	cleared := strings.TrimSpace(string(output))
+	if err != nil || !strings.Contains(cleared, "Success") {
+		return fmt.Errorf(
+			"clear-state: %s is still installed after `adb uninstall` said %q, and `pm clear` said %q: its data was not cleared",
+			bundleID, uninstallOutput, cleared,
+		)
+	}
+	fmt.Fprintf(stdout, "clear-state: uninstall %s said %q and left it installed; cleared its data with `pm clear` instead\n", bundleID, uninstallOutput)
+	return nil
+}
+
+// packageInstalled reports whether the package manager resolves an APK path for
+// bundleID. The printed path is the signal rather than the exit status, which
+// `adb shell` does not forward from devices below API 24.
+func packageInstalled(ctx context.Context, adb, serial, bundleID string) bool {
+	output, _ := exec.CommandContext(ctx, adb, adbArgs(serial, "shell", "pm", "path", bundleID)...).Output()
+	return strings.HasPrefix(strings.TrimSpace(string(output)), "package:")
+}
+
 const threeButtonNavOverlay = "com.android.internal.systemui.navbar.threebutton"
 
 // navModeOverlays are the system navigation-mode overlays. Only one is active at

internal/android/android_test.go

@@ -441,3 +441,134 @@ func TestNavModeToRestore(t *testing.T) {
 		}
 	}
 }
+
+// scriptedAdb puts an adb under a fake SDK root that logs each invocation's
+// arguments and answers from replies, a `case "$*" in` body. SDK lookup is
+// isolated onto that root, so ReinstallApp runs its real command sequence
+// against the script and the log holds what reached adb.
+func scriptedAdb(t *testing.T, replies string) string {
+	t.Helper()
+	root := t.TempDir()
+	log := filepath.Join(root, "adb.log")
+	adb := filepath.Join(root, "platform-tools", "adb")
+	if err := os.MkdirAll(filepath.Dir(adb), 0o755); err != nil {
+		t.Fatalf("mkdir %s: %v", filepath.Dir(adb), err)
+	}
+	script := "#!/bin/sh\necho \"$*\" >> " + log + "\ncase \"$*\" in\n" + replies + "\nesac\n"
+	if err := os.WriteFile(adb, []byte(script), 0o755); err != nil {
+		t.Fatalf("write %s: %v", adb, err)
+	}
+	isolateSDKLookup(t, root)
+	return log
+}
+
+func adbCalls(t *testing.T, log string) []string {
+	t.Helper()
+	contents, err := os.ReadFile(log)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil
+		}
+		t.Fatalf("read %s: %v", log, err)
+	}
+	return strings.Split(strings.TrimSpace(string(contents)), "\n")
+}
+
+const (
+	uninstallFails  = `"uninstall "*) echo "Failure [DELETE_FAILED_INTERNAL_ERROR]"; exit 1;;`
+	stillInstalled  = `"shell pm path "*) echo "package:/data/app/app.example-1/base.apk";;`
+	notInstalled    = `"shell pm path "*) exit 1;;`
+	installSucceeds = `"install "*) echo "Success";;`
+)
+
+func TestReinstallApp_RefusedUninstallClearsTheDataItLeftBehind(t *testing.T) {
+	log := scriptedAdb(t, strings.Join([]string{
+		uninstallFails,
+		stillInstalled,
+		`"shell pm clear "*) echo "Success";;`,
+		installSucceeds,
+	}, "\n"))
+	output := &strings.Builder{}
+
+	if err := ReinstallApp(t.Context(), "", "app.example", "/tmp/app.apk", output); err != nil {
+		t.Fatalf("ReinstallApp: %v", err)
+	}
+
+	want := []string{
+		"uninstall app.example",
+		"shell pm path app.example",
+		"shell pm clear app.example",
+		"install -r /tmp/app.apk",
+	}
+	if got := adbCalls(t, log); !slices.Equal(got, want) {
+		t.Fatalf("adb calls = %v, want %v", got, want)
+	}
+	if !strings.Contains(output.String(), "pm clear") {
+		t.Errorf("output %q does not say the data was cleared some other way", output.String())
+	}
+}
+
+func TestReinstallApp_RefusedUninstallThatCannotBeClearedIsFatal(t *testing.T) {
+	log := scriptedAdb(t, strings.Join([]string{
+		uninstallFails,
+		stillInstalled,
+		`"shell pm clear "*) echo "Failed"; exit 1;;`,
+		installSucceeds,
+	}, "\n"))
+
+	err := ReinstallApp(t.Context(), "", "app.example", "/tmp/app.apk", &strings.Builder{})
+
+	if err == nil {
+		t.Fatal("ReinstallApp reported success while app.example kept the data clear-state was asked to remove")
+	}
+	for _, want := range []string{"app.example", "DELETE_FAILED_INTERNAL_ERROR", "Failed"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error %q does not quote %q", err, want)
+		}
+	}
+	if slices.Contains(adbCalls(t, log), "install -r /tmp/app.apk") {
+		t.Error("installed over an app whose data survived, which is the reinstall reporting a clear it did not perform")
+	}
+}
+
+func TestReinstallApp_UninstallFailureWithNothingInstalledIsQuiet(t *testing.T) {
+	log := scriptedAdb(t, strings.Join([]string{
+		uninstallFails,
+		notInstalled,
+		installSucceeds,
+	}, "\n"))
+	output := &strings.Builder{}
+
+	if err := ReinstallApp(t.Context(), "", "app.example", "/tmp/app.apk", output); err != nil {
+		t.Fatalf("ReinstallApp: %v", err)
+	}
+
+	calls := adbCalls(t, log)
+	if !slices.Contains(calls, "install -r /tmp/app.apk") {
+		t.Fatalf("adb calls = %v, want the install to go ahead: a first run has no app to uninstall", calls)
+	}
+	if slices.Contains(calls, "shell pm clear app.example") {
+		t.Errorf("adb calls = %v, want no data clear: there was no app holding data", calls)
+	}
+	if output.String() != "" {
+		t.Errorf("output = %q, want nothing: a first run has no app to uninstall", output.String())
+	}
+}
+
+func TestReinstallApp_SuccessfulUninstallNeedsNoFallback(t *testing.T) {
+	log := scriptedAdb(t, strings.Join([]string{
+		`"uninstall "*) echo "Success";;`,
+		stillInstalled,
+		`"shell pm clear "*) echo "Success";;`,
+		installSucceeds,
+	}, "\n"))
+
+	if err := ReinstallApp(t.Context(), "", "app.example", "/tmp/app.apk", &strings.Builder{}); err != nil {
+		t.Fatalf("ReinstallApp: %v", err)
+	}
+
+	want := []string{"uninstall app.example", "install -r /tmp/app.apk"}
+	if got := adbCalls(t, log); !slices.Equal(got, want) {
+		t.Fatalf("adb calls = %v, want %v", got, want)
+	}
+}

internal/driver/ioscompanion/device.go

@@ -224,8 +224,14 @@ func (d *Driver) respawnDevice(ctx context.Context) error {
 // devicectlReinstall uninstalls then installs the app bundle via devicectl,
 // keyed on the CoreDevice id. App lifecycle stays with devicectl: the runner's
 // own install path is simulator-specific.
+// A failed uninstall ends the reinstall: installing over an app keeps its data,
+// so clear-state would be reported without happening. Uninstalling an app that
+// is not installed exits 0 ("App uninstalled." on a paired iPhone running iOS
+// 26.5), so there is no benign failure here to sort out from a real one.
 func (d *Driver) devicectlReinstall(ctx context.Context) error {
-	_ = exec.CommandContext(ctx, "xcrun", "devicectl", "device", "uninstall", "app", "--device", d.coreDeviceID, d.bundleID).Run()
+	if output, err := exec.CommandContext(ctx, "xcrun", "devicectl", "device", "uninstall", "app", "--device", d.coreDeviceID, d.bundleID).CombinedOutput(); err != nil {
+		return fmt.Errorf("devicectl uninstall %s: %w: %s", d.bundleID, err, strings.TrimSpace(string(output)))
+	}
 	output, err := exec.CommandContext(ctx, "xcrun", "devicectl", "device", "install", "app", "--device", d.coreDeviceID, d.appPath).CombinedOutput()
 	if err != nil {
 		return fmt.Errorf("devicectl install: %w: %s", err, strings.TrimSpace(string(output)))

internal/driver/ioscompanion/device_test.go

@@ -7,6 +7,7 @@ import (
 	"net"
 	"os/exec"
 	"slices"
+	"strings"
 	"testing"
 
 	"github.com/priyanshujain/sanderling/internal/driver/ioscompanion/transport"
@@ -182,6 +183,45 @@ func TestNewDeviceReinstallsOnceBeforeTheRunnerSession(t *testing.T) {
 	}
 }
 
+func TestDevicectlReinstallStopsWhenTheUninstallFails(t *testing.T) {
+	log := scriptedXcrun(t, `"devicectl device uninstall "*) echo "ERROR: Internal logic error: Connection was invalidated"; exit 1;;
+"devicectl device install "*) :;;`)
+	d := &Driver{coreDeviceID: "CORE-DEVICE", bundleID: "app.example", appPath: "/tmp/Sample.app"}
+
+	err := d.devicectlReinstall(context.Background())
+
+	if err == nil {
+		t.Fatal("devicectlReinstall reported success while app.example kept the data clear-state was asked to remove")
+	}
+	for _, want := range []string{"app.example", "Connection was invalidated"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error %q does not quote %q", err, want)
+		}
+	}
+	calls := xcrunCalls(t, log)
+	if slices.ContainsFunc(calls, func(call string) bool { return strings.HasPrefix(call, "devicectl device install") }) {
+		t.Errorf("xcrun calls = %v: installing over the app carries its data into the run", calls)
+	}
+}
+
+func TestDevicectlReinstallProceedsWhenNothingIsInstalled(t *testing.T) {
+	log := scriptedXcrun(t, `"devicectl device uninstall "*) echo "App uninstalled.";;
+"devicectl device install "*) :;;`)
+	d := &Driver{coreDeviceID: "CORE-DEVICE", bundleID: "app.example", appPath: "/tmp/Sample.app"}
+
+	if err := d.devicectlReinstall(context.Background()); err != nil {
+		t.Fatalf("devicectlReinstall: %v", err)
+	}
+
+	want := []string{
+		"devicectl device uninstall app --device CORE-DEVICE app.example",
+		"devicectl device install app --device CORE-DEVICE /tmp/Sample.app",
+	}
+	if got := xcrunCalls(t, log); !slices.Equal(got, want) {
+		t.Fatalf("xcrun calls = %v, want %v", got, want)
+	}
+}
+
 func TestDeviceClearStateWithoutAppPathWarnsOnce(t *testing.T) {
 	output := &bytes.Buffer{}
 	d := &Driver{output: output, deviceMode: true}

internal/driver/ioscompanion/driver.go

@@ -615,8 +615,14 @@ func (d *Driver) clearAppState(ctx context.Context) error {
 // simctlReinstall uninstalls and reinstalls the app bundle via simctl. App
 // lifecycle stays with simctl: the companion's install RPC misreads current
 // simulator targets' architectures and rejects valid bundles.
+// A failed uninstall ends the reinstall: `simctl install` over an installed app
+// carries its data container across, so clear-state would be reported without
+// happening. Uninstalling an app that is not installed exits 0, so there is no
+// benign failure here to sort out from a real one.
 func (d *Driver) simctlReinstall(ctx context.Context) error {
-	_ = exec.CommandContext(ctx, "xcrun", "simctl", "uninstall", d.udid, d.bundleID).Run()
+	if output, err := exec.CommandContext(ctx, "xcrun", "simctl", "uninstall", d.udid, d.bundleID).CombinedOutput(); err != nil {
+		return fmt.Errorf("simctl uninstall %s: %w: %s", d.bundleID, err, strings.TrimSpace(string(output)))
+	}
 	output, err := exec.CommandContext(ctx, "xcrun", "simctl", "install", d.udid, d.appPath).CombinedOutput()
 	if err != nil {
 		return fmt.Errorf("simctl install: %w: %s", err, strings.TrimSpace(string(output)))

internal/driver/ioscompanion/driver_test.go

@@ -329,6 +329,68 @@ func TestNewRejectsClearStateWithoutBundleID(t *testing.T) {
 	}
 }
 
+// scriptedXcrun puts an xcrun on PATH that logs each invocation's arguments and
+// answers from replies, a `case "$*" in` body, so a reinstall runs its real
+// command sequence and the log holds what reached the tool.
+func scriptedXcrun(t *testing.T, replies string) string {
+	t.Helper()
+	directory := t.TempDir()
+	log := filepath.Join(directory, "xcrun.log")
+	script := "#!/bin/sh\necho \"$*\" >> " + log + "\ncase \"$*\" in\n" + replies + "\nesac\n"
+	if err := os.WriteFile(filepath.Join(directory, "xcrun"), []byte(script), 0o755); err != nil {
+		t.Fatalf("write xcrun: %v", err)
+	}
+	t.Setenv("PATH", directory)
+	return log
+}
+
+func xcrunCalls(t *testing.T, log string) []string {
+	t.Helper()
+	contents, err := os.ReadFile(log)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil
+		}
+		t.Fatalf("read %s: %v", log, err)
+	}
+	return strings.Split(strings.TrimSpace(string(contents)), "\n")
+}
+
+func TestSimctlReinstallStopsWhenTheUninstallFails(t *testing.T) {
+	log := scriptedXcrun(t, `"simctl uninstall "*) echo "Simulator device failed to uninstall app.example."; echo "Uninstall prohibited."; exit 22;;
+"simctl install "*) :;;`)
+	d := &Driver{udid: "SIM-UDID", bundleID: "app.example", appPath: "/tmp/Sample.app"}
+
+	err := d.simctlReinstall(context.Background())
+
+	if err == nil {
+		t.Fatal("simctlReinstall reported success while app.example kept the data clear-state was asked to remove")
+	}
+	for _, want := range []string{"app.example", "Uninstall prohibited."} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error %q does not quote %q", err, want)
+		}
+	}
+	if calls := xcrunCalls(t, log); slices.Contains(calls, "simctl install SIM-UDID /tmp/Sample.app") {
+		t.Errorf("xcrun calls = %v: installing over the app carries its data into the run", calls)
+	}
+}
+
+func TestSimctlReinstallProceedsWhenNothingIsInstalled(t *testing.T) {
+	log := scriptedXcrun(t, `"simctl uninstall "*) :;;
+"simctl install "*) :;;`)
+	d := &Driver{udid: "SIM-UDID", bundleID: "app.example", appPath: "/tmp/Sample.app"}
+
+	if err := d.simctlReinstall(context.Background()); err != nil {
+		t.Fatalf("simctlReinstall: %v", err)
+	}
+
+	want := []string{"simctl uninstall SIM-UDID app.example", "simctl install SIM-UDID /tmp/Sample.app"}
+	if got := xcrunCalls(t, log); !slices.Equal(got, want) {
+		t.Fatalf("xcrun calls = %v, want %v", got, want)
+	}
+}
+
 func TestLaunchRejectsEnvironment(t *testing.T) {
 	d := newTestDriver(&fakeCompanion{accessibilityJSON: "[]"})
 	err := d.Launch(context.Background(), "", false, map[string]string{"K": "V"})