Add AdbRunner for adb CLI operations by rmarinho · Pull Request #283 · dotnet/android-tools

rmarinho · 2026-02-23T17:39:27Z

Summary

Wraps adb CLI operations for device management. Addresses #279.

Parsing/formatting/merging logic ported from dotnet/android GetAvailableAndroidDevices MSBuild task, enabling code sharing via the external/xamarin-android-tools submodule. See draft PR: dotnet/android#10880

Public API

public class AdbRunner
{
    // Constructors
    public AdbRunner (Func<string?> getSdkPath);
    public AdbRunner (Func<string?> getSdkPath, Func<string?>? getJdkPath);

    // Properties
    public string? AdbPath { get; }
    public bool IsAvailable { get; }

    // Instance methods (async, use adb process)
    public Task<IReadOnlyList<AdbDeviceInfo>> ListDevicesAsync (CancellationToken ct = default);
    public Task WaitForDeviceAsync (string? serial = null, TimeSpan? timeout = null, CancellationToken ct = default);
    public Task StopEmulatorAsync (string serial, CancellationToken ct = default);

    // Static helpers — public so dotnet/android can call without instantiating AdbRunner
    public static List<AdbDeviceInfo> ParseAdbDevicesOutput (string output);
    public static List<AdbDeviceInfo> ParseAdbDevicesOutput (IEnumerable<string> lines);
    public static AdbDeviceStatus MapAdbStateToStatus (string adbState);
    public static string BuildDeviceDescription (AdbDeviceInfo device, Action<TraceLevel, string>? logger = null);
    public static string FormatDisplayName (string avdName);
    public static List<AdbDeviceInfo> MergeDevicesAndEmulators (IReadOnlyList<AdbDeviceInfo> adbDevices, IReadOnlyList<string> availableEmulators, Action<TraceLevel, string>? logger = null);
}

Internal methods (not part of public API):

GetEmulatorAvdNameAsync — queries AVD name via adb emu avd name (logs failures via Trace.WriteLine)
ProcessUtils.ThrowIfFailed — shared exit code validation

Key Design Decisions

Static parsing methods are public static so dotnet/android can call them without instantiating AdbRunner (e.g., GetAvailableAndroidDevices MSBuild task passes List<string> to ParseAdbDevicesOutput)
IEnumerable<string> overload: ParseAdbDevicesOutput(string) delegates to ParseAdbDevicesOutput(IEnumerable<string>) — dotnet/android passes List<string> directly from output lines
Logger parameter: BuildDeviceDescription and MergeDevicesAndEmulators accept Action<TraceLevel, string>? — dotnet/android passes this.CreateTaskLogger() for MSBuild trace output
Regex with explicit state list: Uses \s+ separator to handle both space and tab-separated adb output. Matches explicit known states (device|offline|unauthorized|authorizing|no permissions|recovery|sideload|bootloader|connecting|host) with IgnoreCase to prevent false positives from non-device lines. Daemon startup lines (*) are pre-filtered.
Exit code checking: ListDevicesAsync and WaitForDeviceAsync throw InvalidOperationException with stderr context on non-zero exit via shared ProcessUtils.ThrowIfFailed (internal)
Timeout validation first: WaitForDeviceAsync validates timeout before RequireAdb() so ArgumentOutOfRangeException is thrown even when adb is not installed
FormatDisplayName: Lowercases before ToTitleCase to normalize mixed-case input (e.g., "PiXeL" → "Pixel")
One type per file: AdbDeviceInfo, AdbDeviceType, AdbDeviceStatus each in their own file
Exception logging in GetEmulatorAvdNameAsync: Catches exceptions with Trace.WriteLine instead of bare catch, so failures querying AVD names are debuggable while remaining non-fatal
Environment variables via StartProcess: Runners pass env vars dictionary to ProcessUtils.StartProcess instead of mutating ProcessStartInfo directly. AndroidEnvironmentHelper.GetEnvironmentVariables() builds the dict (ANDROID_HOME, JAVA_HOME, ANDROID_USER_HOME, PATH).

Tests

44 unit tests (AdbRunnerTests.cs):

ParseAdbDevicesOutput: real-world data, empty output, single/multiple devices, mixed states, daemon messages, IP:port, Windows newlines, recovery/sideload, tab-separated output
FormatDisplayName: underscores, title case, API capitalization, mixed case, special chars, empty
MapAdbStateToStatus: all known states + unknown (recovery, sideload)
MergeDevicesAndEmulators: no emulators, no running, mixed, case-insensitive dedup, sorting
AdbPath: SDK discovery, PATH fallback
WaitForDeviceAsync: timeout validation (negative, zero)

9 integration tests (RunnerIntegrationTests.cs):

Run only in CI (TF_BUILD or CI env var), skipped locally
Use pre-installed JDK (JAVA_HOME) and Android SDK (ANDROID_HOME) from CI agent
Verify AdbRunner, AvdManagerRunner, EmulatorRunner, SdkManager against real tools
Cover: IsAvailable, ListDevicesAsync, WaitForDeviceAsync timeout, ListAvdsAsync, ListAsync

Review Feedback Addressed

Feedback	Commit	Details
Port device listing from dotnet/android	`01ed6b9`	Ported ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators, 33 tests
`IEnumerable<string>` overload for ParseAdbDevicesOutput	`01ed6b9`	String overload delegates to IEnumerable; dotnet/android passes `List<string>` directly
`Action<TraceLevel, string>` logger	`9d4b4f5`	Restores 5 debug messages via `CreateTaskLogger` pattern
`ThrowIfFailed` visibility → `internal`	`71fff34`	Also fixes StopEmulatorAsync error handling
Bare catch logging in GetEmulatorAvdNameAsync	`9368682`	Logs exception via `Trace.WriteLine` instead of silently swallowing
Tab-separated adb output regex	`dbf54f9`	Explicit state list + `\s+` + `IgnoreCase` + new `ParseAdbDevicesOutput_TabSeparator` test

Depends On

feature/tool-runner-base (PR Add EmulatorRunner for emulator CLI operations #284) — ProcessUtils shared helpers, AndroidEnvironmentHelper, EnvironmentVariableNames

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

jonathanpeppers · 2026-02-23T18:08:36Z

I'd like to get the System.Diagnostics.Process code unified like mentioned here:

Add base Android tool runner infrastructure #281 (comment)

Addresses PR #283 feedback to use existing ProcessUtils instead of the removed AndroidToolRunner. Simplifies API: - Methods now throw InvalidOperationException on failure - Uses ProcessUtils.RunToolAsync() for all tool invocations - Added AndroidDeviceInfo model - Removed complex ToolRunnerResult wrapper types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Addresses PR #283/#284 feedback to use existing ProcessUtils. Simplifies API by throwing exceptions on failure instead of returning result types with error states. Changes: - AdbRunner: Simplified using ProcessUtils.RunToolAsync() - EmulatorRunner: Uses ProcessUtils.StartToolBackground() - Removed duplicate AndroidDeviceInfo from Models directory Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

rmarinho · 2026-03-03T16:45:21Z

Implemented your suggested approach:

Ported the device listing logic from dotnet/android's GetAvailableAndroidDevices MSBuild task into AdbRunner in feature/adb-runner branch
AdbDeviceInfo now has all the same fields: Serial, Description, Type (enum), Status (enum), AvdName, Model, Product, Device, TransportId
Ported ParseAdbDevicesOutput (same regex pattern), BuildDeviceDescription (same priority order), FormatDisplayName (title case + API capitalization), MapAdbStateToStatus, and MergeDevicesAndEmulators (dedup + sorting)
Added GetEmulatorAvdNameAsync (async version of GetEmulatorAvdName)
33 unit tests ported from the dotnet/android test cases (parsing, display name formatting, status mapping, merging/dedup, path discovery)

Next steps per your plan:

✅ feature/adb-runner has the ported logic (pushed)
⬜ Open a draft PR in dotnet/android that updates the submodule + rewrites GetAvailableAndroidDevices.cs to consume the new shared API
⬜ Review/merge android-tools first, then dotnet/android

rmarinho · 2026-03-03T17:02:39Z

The dotnet/android side is now ready as a draft PR: dotnet/android#10880

It delegates GetAvailableAndroidDevices parsing/formatting/merging to the shared AdbRunner methods from this PR, removing ~200 lines of duplicated code. All 33 existing tests are preserved and updated to use AdbRunner/AdbDeviceInfo directly (no more reflection).

Workflow:

Merge this PR first
Update the dotnet/android submodule pointer from feature/adb-runner to main
Take Use shared AdbRunner from android-tools for device listing android#10880 out of draft and merge

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidEnvironmentHelper.cs

src/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

…eout - Broaden AdbDevicesRegex to match any device state (recovery, sideload, etc.) using \s{2,} separator to avoid matching random text lines - Skip daemon startup lines (starting with *) in ParseAdbDevicesOutput - ListDevicesAsync now captures stderr and throws on non-zero exit code - WaitForDeviceAsync now checks exit code and throws with stdout/stderr context - Validate timeout: reject negative and zero TimeSpan values - Add 6 tests: recovery/sideload parsing, state mapping, timeout validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Port adb device parsing, formatting, and merging logic from dotnet/android GetAvailableAndroidDevices MSBuild task into a shared AdbRunner class. New files: - AdbRunner.cs — ListDevicesAsync, WaitForDeviceAsync, StopEmulatorAsync, ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MapAdbStateToStatus, MergeDevicesAndEmulators - AdbDeviceInfo/AdbDeviceType/AdbDeviceStatus — device models - AndroidEnvironmentHelper — shared env var builder for all runners - ProcessUtils.ThrowIfFailed — shared exit code validation Modified files: - EnvironmentVariableNames — add ANDROID_USER_HOME, ANDROID_AVD_HOME - SdkManager.Process.cs — deduplicate env var logic via AndroidEnvironmentHelper Tests: - 43 unit tests (parsing, formatting, merging, path discovery, timeout) - 5 integration tests (CI-only, real SDK tools) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- StopEmulatorAsync now captures stderr and calls ThrowIfFailed for consistency with ListDevicesAsync/WaitForDeviceAsync. - ThrowIfFailed changed from public to internal since it is only used within the library. - Remove inaccurate cmdline-tools bootstrap claim from integration test doc comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@jonathanpeppers

Avoids allocating a joined string when callers already have individual lines (e.g., MSBuild LogEventsFromTextOutput). The existing string overload now delegates to the new one. Addresses review feedback from @jonathanpeppers on dotnet/android#10880. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…esAndEmulators Accepts Action<TraceLevel, string> to route debug messages through the caller's logging infrastructure (e.g., MSBuild TaskLoggingHelper). Restores log messages lost when logic moved from dotnet/android to android-tools: AVD name formatting, running emulator detection, and non-running emulator additions. Follows the existing CreateTaskLogger pattern used by JdkInstaller. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-04T14:46:01Z

tests/Xamarin.Android.Tools.AndroidSdk-Tests/RunnerIntegrationTests.cs

+				await sdkManager.InstallAsync (new [] { "platform-tools" }, acceptLicenses: true, cancellationToken: cts.Token);
+				TestContext.Progress.WriteLine ($"SDK bootstrapped to: {sdkPath}");
+			}
+			catch (Exception ex) when (ex is HttpRequestException || ex is TaskCanceledException || ex is InvalidOperationException) {


The bootstrap fallback catches TaskCanceledException, but SdkManager.BootstrapAsync/InstallAsync can also throw OperationCanceledException when the CancellationTokenSource.CancelAfter(...) triggers. In that case this setup will fail the test run instead of skipping. Consider catching OperationCanceledException (or Exception ex with an ex is OperationCanceledException pattern) alongside the existing exceptions so timeouts reliably result in Assert.Ignore as intended.

Suggested change

catch (Exception ex) when (ex is HttpRequestException || ex is TaskCanceledException || ex is InvalidOperationException) {

catch (Exception ex) when (ex is HttpRequestException || ex is TaskCanceledException || ex is OperationCanceledException || ex is InvalidOperationException) {

Delegates adb devices parsing, description building, and device/emulator merging from GetAvailableAndroidDevices to AdbRunner in the shared xamarin-android-tools submodule. Removes ~200 lines of duplicated logic. - ParseAdbDevicesOutput accepts IEnumerable<string> to avoid string.Join - BuildDeviceDescription/MergeDevicesAndEmulators accept optional Action<TraceLevel, string> logger for MSBuild diagnostics - Tests updated to use AdbRunner/AdbDeviceInfo directly Depends on dotnet/android-tools#283. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rmarinho · 2026-03-04T16:31:08Z

Review feedback addressed — commit references

Feedback	Commit	Details
`IEnumerable<string>` overload for `ParseAdbDevicesOutput`	`01ed6b9`	String overload delegates to IEnumerable; dotnet/android passes `List<string>` directly
`Action<TraceLevel, string>` logger for `BuildDeviceDescription`/`MergeDevicesAndEmulators`	`9d4b4f5`	Restores 5 debug messages via `CreateTaskLogger` pattern
`ThrowIfFailed` visibility → `internal`	`71fff34`	Also fixes StopEmulatorAsync error handling and doc comment

Corresponding dotnet/android consumer: 30124fa (PR #10880)

dotnet/android compiles the submodule as netstandard2.0 with WarningsAsErrors=Nullable. In netstandard2.0, string.IsNullOrEmpty lacks [NotNullWhen(false)], so the compiler doesn't narrow string? to string after null checks. Add null-forgiving operators where the preceding guard guarantees non-null. Fixes: CS8601 in AndroidEnvironmentHelper.cs (sdkPath, jdkPath) Fixes: CS8620 in AdbRunner.cs (serial in string[] array literal) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- Replace \s{2,} with \s+ to handle tab-separated adb output - Use explicit state list (device|offline|unauthorized|etc.) instead of \S+ to prevent false positives from non-device lines - Add ParseAdbDevicesOutput_TabSeparator test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

You can also share your feedback on Copilot code review. Take the survey.

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

Copilot · 2026-03-04T18:07:37Z

tests/Xamarin.Android.Tools.AndroidSdk-Tests/RunnerIntegrationTests.cs

+		if (Environment.GetEnvironmentVariable ("TF_BUILD") is null &&
+		    Environment.GetEnvironmentVariable ("CI") is null) {
+			Assert.Ignore ("Integration tests only run on CI (TF_BUILD or CI env var must be set).");


The XML doc says these tests only run when TF_BUILD=True or CI=true, but RequireCi() only checks that the variables are present (any non-null value). Please align the documentation with the actual gating logic (or tighten the check to require a truthy value).

Suggested change

if (Environment.GetEnvironmentVariable ("TF_BUILD") is null &&

Environment.GetEnvironmentVariable ("CI") is null) {

Assert.Ignore ("Integration tests only run on CI (TF_BUILD or CI env var must be set).");

var tfBuild = Environment.GetEnvironmentVariable ("TF_BUILD");

var ci = Environment.GetEnvironmentVariable ("CI");

if (!string.Equals (tfBuild, "true", StringComparison.OrdinalIgnoreCase) &&

!string.Equals (ci, "true", StringComparison.OrdinalIgnoreCase)) {

Assert.Ignore ("Integration tests only run on CI (TF_BUILD=True or CI=true).");

tests/Xamarin.Android.Tools.AndroidSdk-Tests/RunnerIntegrationTests.cs

Copilot · 2026-03-04T18:07:37Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+	// Pattern to match device lines: <serial> <state> [key:value ...]
+	// Requires 2+ spaces between serial and state (adb pads serials).
+	// Matches known adb device states. Uses \s+ to handle both space and tab separators.
+	// Explicit state list prevents false positives from non-device lines.
+	static readonly Regex AdbDevicesRegex = new Regex (
+		@"^([^\s]+)\s+(device|offline|unauthorized|authorizing|no permissions|recovery|sideload|bootloader|connecting|host)\s*(.*)$",
+		RegexOptions.Compiled | RegexOptions.IgnoreCase);


The comment above AdbDevicesRegex says it "Requires 2+ spaces between serial and state", but the regex pattern uses \s+ (1+ whitespace, including tabs). Please update the comment to match the actual behavior (or tighten the regex if 2+ spaces is truly required).

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

jonathanpeppers

🤖 AI Review Summary

Found 1 issue: error handling.

Error handling: Bare catch block in GetEmulatorAvdNameAsync silently swallows exceptions without logging (AdbRunner.cs:119)

👍 Excellent PR overall — thorough test coverage (44 unit + 9 integration tests), consistent use of ProcessUtils for all process creation, proper CancellationToken propagation throughout, correct OperationCanceledException rethrow before the bare catch, good use of EnvironmentVariableNames constants, and clean code organization (one type per file, file-scoped namespaces). The refactoring of SdkManager.GetEnvironmentVariables into the shared AndroidEnvironmentHelper is a nice DRY improvement.

This review was generated by the android-tools-reviewer skill based on review guidelines established by @jonathanpeppers.

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

Address review feedback: replace bare catch with catch(Exception ex) and log via Trace.WriteLine for debuggability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rmarinho · 2026-03-04T20:11:15Z

Addressed the bare catch feedback in 9368682: GetEmulatorAvdNameAsync now catches Exception ex and logs via Trace.WriteLine instead of silently swallowing. Also updated PR description to accurately reflect the current API surface (including IEnumerable<string> overload, logger parameters, internal methods).

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-04T20:17:15Z

tests/Xamarin.Android.Tools.AndroidSdk-Tests/RunnerIntegrationTests.cs

+		// Use pre-installed Android SDK from ANDROID_HOME (available on CI agents)
+		sdkPath = Environment.GetEnvironmentVariable (EnvironmentVariableNames.AndroidHome);
+		if (string.IsNullOrEmpty (sdkPath) || !Directory.Exists (sdkPath)) {
+			// Fall back to bootstrapping our own SDK
+			TestContext.Progress.WriteLine ("ANDROID_HOME not set — bootstrapping SDK...");
+			try {
+				bootstrappedSdkPath = Path.Combine (Path.GetTempPath (), $"runner-integration-{Guid.NewGuid ():N}", "android-sdk");
+				sdkManager = new SdkManager (Log);
+				sdkManager.JavaSdkPath = jdkPath;
+				using var cts = new CancellationTokenSource (TimeSpan.FromMinutes (10));
+				await sdkManager.BootstrapAsync (bootstrappedSdkPath, cancellationToken: cts.Token);
+				sdkPath = bootstrappedSdkPath;
+				sdkManager.AndroidSdkPath = sdkPath;
+
+				// Install platform-tools (provides adb)
+				await sdkManager.InstallAsync (new [] { "platform-tools" }, acceptLicenses: true, cancellationToken: cts.Token);
+				TestContext.Progress.WriteLine ($"SDK bootstrapped to: {sdkPath}");
+			}
+			catch (Exception ex) when (ex is HttpRequestException || ex is TaskCanceledException || ex is InvalidOperationException) {
+				Assert.Ignore ($"SDK bootstrap failed: {ex.Message}");
+				return;
+			}
+		}
+		else {
+			TestContext.Progress.WriteLine ($"Using SDK from ANDROID_HOME: {sdkPath}");
+			sdkManager = new SdkManager (Log);
+			sdkManager.JavaSdkPath = jdkPath;
+			sdkManager.AndroidSdkPath = sdkPath;
+		}


These “integration” tests will run in Azure Pipelines because TF_BUILD is set, but our pipeline doesn’t provision ANDROID_HOME (see azure-pipelines.yaml Run Tests step). That means this setup will often bootstrap/download an SDK (and install platform-tools) during CI test runs, adding significant runtime and flakiness due to network dependency. Consider skipping unless an explicit opt-in env var is set (e.g., ANDROID_TOOLS_RUN_INTEGRATION_TESTS), or at minimum Assert.Ignore when ANDROID_HOME is missing instead of bootstrapping in the test suite.

Suggested change

// Use pre-installed Android SDK from ANDROID_HOME (available on CI agents)

sdkPath = Environment.GetEnvironmentVariable (EnvironmentVariableNames.AndroidHome);

if (string.IsNullOrEmpty (sdkPath) || !Directory.Exists (sdkPath)) {

// Fall back to bootstrapping our own SDK

TestContext.Progress.WriteLine ("ANDROID_HOME not set — bootstrapping SDK...");

try {

bootstrappedSdkPath = Path.Combine (Path.GetTempPath (), $"runner-integration-{Guid.NewGuid ():N}", "android-sdk");

sdkManager = new SdkManager (Log);

sdkManager.JavaSdkPath = jdkPath;

using var cts = new CancellationTokenSource (TimeSpan.FromMinutes (10));

await sdkManager.BootstrapAsync (bootstrappedSdkPath, cancellationToken: cts.Token);

sdkPath = bootstrappedSdkPath;

sdkManager.AndroidSdkPath = sdkPath;

// Install platform-tools (provides adb)

await sdkManager.InstallAsync (new [] { "platform-tools" }, acceptLicenses: true, cancellationToken: cts.Token);

TestContext.Progress.WriteLine ($"SDK bootstrapped to: {sdkPath}");

}

catch (Exception ex) when (ex is HttpRequestException || ex is TaskCanceledException || ex is InvalidOperationException) {

Assert.Ignore ($"SDK bootstrap failed: {ex.Message}");

return;

}

}

else {

TestContext.Progress.WriteLine ($"Using SDK from ANDROID_HOME: {sdkPath}");

sdkManager = new SdkManager (Log);

sdkManager.JavaSdkPath = jdkPath;

sdkManager.AndroidSdkPath = sdkPath;

}

// Use pre-installed Android SDK from ANDROID_HOME (if available)

sdkPath = Environment.GetEnvironmentVariable (EnvironmentVariableNames.AndroidHome);

if (string.IsNullOrEmpty (sdkPath) || !Directory.Exists (sdkPath)) {

Assert.Ignore ("ANDROID_HOME not set or invalid — cannot run integration tests. Provision Android SDK and set ANDROID_HOME to enable these tests.");

return;

}

TestContext.Progress.WriteLine ($"Using SDK from ANDROID_HOME: {sdkPath}");

sdkManager = new SdkManager (Log);

sdkManager.JavaSdkPath = jdkPath;

sdkManager.AndroidSdkPath = sdkPath;

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

When 'adb emu avd name' fails (common on macOS), fall back to querying the emulator console directly via TCP on the console port extracted from the serial (emulator-XXXX -> port XXXX). This fixes duplicate device entries when running emulators can't be matched with their AVD definitions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

This skill let's you say: review this PR: #284 Some example code reviews: * #283 (review) * #284 (review) This is built off a combination of previous code reviews, saved in `docs/CODE_REVIEW_POSTMORTEM.md`, and the review rules in `references/review-rules.md`.

This skill lets you say: review this PR: #284 Some example code reviews: * #283 (review) * #284 (review) This is built off a combination of previous code reviews, saved in `docs/CODE_REVIEW_POSTMORTEM.md`, and the review rules in `references/review-rules.md`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers · 2026-03-04T22:57:52Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+	public AdbRunner (Func<string?> getSdkPath)
+		: this (getSdkPath, null)
+	{
+	}
+
+	public AdbRunner (Func<string?> getSdkPath, Func<string?>? getJdkPath)


Why do we pass in Func<string> of these? that seems really weird.

Can we just pass in a string adbPath of a full path to adb?

jonathanpeppers · 2026-03-04T22:58:20Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+			}
+			return ProcessUtils.FindExecutablesInPath ("adb").FirstOrDefault ();
+		}
+	}


Why do we have code here searching for adb in $PATH? Can we just pass in the full path to adb?

jonathanpeppers · 2026-03-04T22:58:45Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+	public bool IsAvailable => AdbPath is not null;
+
+	string RequireAdb ()
+	{
+		return AdbPath ?? throw new InvalidOperationException ("ADB not found.");
+	}


Again, it seems like you should require a path, and then you don't need any of this either.

jonathanpeppers · 2026-03-04T22:59:29Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+			using var client = new TcpClient ();
+			using var cts = CancellationTokenSource.CreateLinkedTokenSource (cancellationToken);
+			cts.CancelAfter (TimeSpan.FromSeconds (3));
+
+			// ConnectAsync with CancellationToken not available on netstandard2.0;
+			// use Task.Run + token check instead
+			var connectTask = client.ConnectAsync ("127.0.0.1", port);
+			var completed = await Task.WhenAny (connectTask, Task.Delay (3000, cts.Token)).ConfigureAwait (false);
+			if (completed != connectTask) {
+				return null;
+			}
+			await connectTask.ConfigureAwait (false); // observe exceptions


What is going on here? Why are we using a TcpClient?!?

jonathanpeppers · 2026-03-04T23:01:21Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+
+		var args = string.IsNullOrEmpty (serial)
+			? new [] { "wait-for-device" }
+			: new [] { "-s", serial!, "wait-for-device" };


We should avoid using !, do you even need it if you use our IsNullOrEmpty() extension method? If it's not in this repo, we could move it here from dotnet/android.

jonathanpeppers · 2026-03-04T23:02:04Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+
+		try {
+			var exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, cts.Token, envVars).ConfigureAwait (false);
+			ProcessUtils.ThrowIfFailed (exitCode, "adb wait-for-device", stderr.ToString (), stdout.ToString ());


Should ThrowIfFailed() just have an overload for StringWriter and you wouldn't have to call ToString() from callers?

jonathanpeppers · 2026-03-04T23:02:47Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+	/// <summary>
+	/// Parses the output of 'adb devices -l'.
+	/// Ported from dotnet/android GetAvailableAndroidDevices.ParseAdbDevicesOutput.
+	/// </summary>
+	public static List<AdbDeviceInfo> ParseAdbDevicesOutput (string output)
+	{
+		return ParseAdbDevicesOutput (output.Split ('\n'));
+	}


Can we delete this overload?

jonathanpeppers · 2026-03-04T23:03:16Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

+		switch (adbState.ToLowerInvariant ()) {
+		case "device": return AdbDeviceStatus.Online;
+		case "offline": return AdbDeviceStatus.Offline;
+		case "unauthorized": return AdbDeviceStatus.Unauthorized;
+		case "no permissions": return AdbDeviceStatus.NoPermissions;
+		default: return AdbDeviceStatus.Unknown;
+		}


can this use C# pattern matching, and update copilot-instructions.md to do this for all new code?

jonathanpeppers · 2026-03-04T23:06:06Z

src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidEnvironmentHelper.cs

+	/// Builds environment variables needed to run Android SDK tools.
+	/// Pass the result to <see cref="ProcessUtils.StartProcess"/> via the environmentVariables parameter.
+	/// </summary>
+	internal static Dictionary<string, string> GetEnvironmentVariables (string? sdkPath, string? jdkPath)


Why are the paths allowed to be null? Then we have ! below?

Can we make sure copilot-instructions.md says to never use ! and to actually fix nullable warnings correctly?

jonathanpeppers · 2026-03-04T23:06:59Z

tests/Xamarin.Android.Tools.AndroidSdk-Tests/RunnerIntegrationTests.cs

+
+	// ── Cross-runner: verify tools exist ───────────────────────────
+


It doesn't seem like we should be writing comments like this, they are basically #region and #endregion

Copilot AI review requested due to automatic review settings February 23, 2026 17:39

rmarinho mentioned this pull request Feb 23, 2026

Add EmulatorRunner for emulator CLI operations #284

Open

Copilot started reviewing on behalf of rmarinho February 23, 2026 17:40 View session

This comment was marked as resolved.

Sign in to view

rmarinho added the copilot `copilot-cli` or other AIs were used to author this label Feb 23, 2026

rmarinho force-pushed the feature/adb-runner branch from bb947a5 to b08dd3f Compare February 24, 2026 14:11

rmarinho force-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675f Compare March 2, 2026 11:42

jonathanpeppers reviewed Mar 2, 2026

View reviewed changes

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated Show resolved Hide resolved

rmarinho force-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6 Compare March 3, 2026 14:35

rmarinho mentioned this pull request Mar 3, 2026

Use shared AdbRunner from android-tools for device listing dotnet/android#10880

Draft

rmarinho requested a review from Copilot March 3, 2026 17:22

Copilot started reviewing on behalf of rmarinho March 3, 2026 17:23 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

rmarinho force-pushed the feature/adb-runner branch from 193203e to 5f9a212 Compare March 3, 2026 18:23

rmarinho force-pushed the feature/adb-runner branch from be56ade to 1d82e36 Compare March 3, 2026 19:18

rmarinho requested a review from Copilot March 4, 2026 09:24

Copilot started reviewing on behalf of rmarinho March 4, 2026 09:24 View session

This comment was marked as outdated.

Sign in to view

rmarinho force-pushed the feature/adb-runner branch from 72bc2ce to 9f316f4 Compare March 4, 2026 10:58

rmarinho requested a review from Copilot March 4, 2026 11:21

Copilot started reviewing on behalf of rmarinho March 4, 2026 11:21 View session

This comment was marked as outdated.

Sign in to view

rmarinho and others added 3 commits March 4, 2026 12:11

rmarinho requested a review from Copilot March 4, 2026 14:40

Copilot started reviewing on behalf of rmarinho March 4, 2026 14:41 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

rmarinho and others added 2 commits March 4, 2026 17:10

rmarinho requested a review from Copilot March 4, 2026 18:02

Copilot started reviewing on behalf of rmarinho March 4, 2026 18:03 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

jonathanpeppers reviewed Mar 4, 2026

View reviewed changes

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated Show resolved Hide resolved

Log exception in GetEmulatorAvdNameAsync bare catch block

9368682

Address review feedback: replace bare catch with catch(Exception ex) and log via Trace.WriteLine for debuggability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rmarinho requested a review from Copilot March 4, 2026 20:13

Copilot started reviewing on behalf of rmarinho March 4, 2026 20:13 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

jonathanpeppers mentioned this pull request Mar 4, 2026

[skills] create a skill for reviewing PRs #292

Merged

jonathanpeppers reviewed Mar 4, 2026

View reviewed changes

	catch (Exception ex) when (ex is HttpRequestException \|\| ex is TaskCanceledException \|\| ex is InvalidOperationException) {
	catch (Exception ex) when (ex is HttpRequestException \|\| ex is TaskCanceledException \|\| ex is OperationCanceledException \|\| ex is InvalidOperationException) {

-		if (Environment.GetEnvironmentVariable ("TF_BUILD") is null &&
-		    Environment.GetEnvironmentVariable ("CI") is null) {
-			Assert.Ignore ("Integration tests only run on CI (TF_BUILD or CI env var must be set).");
+		var tfBuild = Environment.GetEnvironmentVariable ("TF_BUILD");
+		var ci = Environment.GetEnvironmentVariable ("CI");
+		if (!string.Equals (tfBuild, "true", StringComparison.OrdinalIgnoreCase) &&
+		    !string.Equals (ci, "true", StringComparison.OrdinalIgnoreCase)) {
+			Assert.Ignore ("Integration tests only run on CI (TF_BUILD=True or CI=true).");


		// ── Cross-runner: verify tools exist ───────────────────────────

Conversation

rmarinho commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Public API

Key Design Decisions

Tests

Review Feedback Addressed

Depends On

Uh oh!

This comment was marked as resolved.

Uh oh!

jonathanpeppers commented Feb 23, 2026

Uh oh!

Uh oh!

rmarinho commented Mar 3, 2026

Uh oh!

rmarinho commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

rmarinho commented Mar 4, 2026

Review feedback addressed — commit references

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

🤖 AI Review Summary

Uh oh!

Uh oh!

rmarinho commented Mar 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmarinho commented Feb 23, 2026 •

edited

Loading