AdbRunner: follow-ups from PR #351

[rmarinho]

Tracking the items that were deliberately deferred from dotnet/android-tools#351 ([AdbRunner] Add ADB forward port management) so they don't get lost.

Background

PR dotnet/android-tools#351 added ForwardPortAsync / RemoveForwardPortAsync / RemoveAllForwardPortsAsync / ListForwardPortsAsync to AdbRunner, mirroring the reverse-port API added in dotnet/android-tools#305. During review, jonathanpeppers raised the question of whether these should use the in-process AdbClient from dotnet/android-tools#327 instead of shelling out to adb. The decision was to keep shelling out for dotnet/android-tools#351 (to stay symmetric with the merged reverse implementation, and to avoid coupling dotnet/android-tools#351 to an unmerged dependency), and capture the broader migration here.

Items

1. Stdout parity for reverse-port methods

ReversePortAsync, RemoveReversePortAsync, and RemoveAllReversePortsAsync (src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs, lines ~267, 289, 304) still pass null for stdout to ProcessUtils.StartProcess and only stderr to ProcessUtils.ThrowIfFailed. After dotnet/android-tools#351, the parallel forward methods capture stdout and pass it to ThrowIfFailed as well, which matches the repo convention ("Include stdout in error diagnostics") and yields richer failure messages because adb sometimes writes errors to stdout. Bring the three reverse methods in line.

2. Migrate AdbRunner to use AdbClient once dotnet/android-tools#327 merges

After dotnet/android-tools#327 (in-process AdbClient for the ADB daemon socket protocol) lands, do a single consistent pass that switches the existing process-based call sites in AdbRunner to use AdbClient:

• ListDevicesAsync → host:devices-l (already used by AdbDeviceTracker)
• GetShellPropertyAsync / GetEmulatorAvdNameAsync → host:transport:<serial> + shell:<cmd>
• Forward operations → host-serial:<serial>:forward:… / killforward / killforward-all / host:list-forward
• Reverse operations → host:transport:<serial> + reverse:forward:… / killforward / killforward-all / list-forward

Open question to resolve as part of the migration: how to preserve the implicit adb start-server behaviour that shelling out gives us today. A bare connect("127.0.0.1", 5037) against a cold daemon will throw SocketException. Options include shelling out to adb start-server once on AdbClient failure, or documenting that callers must ensure the daemon is running.

3. Cross-platform failure-path tests

The current AdbRunner failure-path tests (e.g. GetEmulatorAvdNameAsync_*) rely on a fake adb bash script and are skipped on Windows (4 skips). The migration in item 2 enables driving these tests with an in-memory loopback TcpListener instead, which would run on all platforms. Add tests covering at minimum:

• ForwardPortAsync failure surfaces stdout in the thrown InvalidOperationException.Message (lock-in for the change made in [AdbRunner] Add ADB forward port management (follow-up to #305) android-tools#351).
• The reverse equivalents once item 1 lands.

cc @jonathanpeppers @rmarinho

[rmarinho]

Item 4 — Extract a shared "run-tool-or-throw" helper across all Runners/

While reviewing dotnet/android-tools#351 I noticed this boilerplate is open-coded ~13 times across AdbRunner.cs, AvdManagerRunner.cs, and EmulatorRunner.cs:

using var stdout = new StringWriter ();
using var stderr = new StringWriter ();
var psi = ProcessUtils.CreateProcessStartInfo (toolPath, args);
var exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, ct, env).ConfigureAwait (false);
ProcessUtils.ThrowIfFailed (exitCode, description, stderr, stdout);

Two practical wins from extracting it:

1. Centrally enforce the stdout-capture convention. The convention ("Include stdout in error diagnostics" in .github/copilot-instructions.md) is applied inconsistently today — AvdManagerRunner.cs:46, :122, and the three reverse mutation methods on AdbRunner still pass only stderr to ThrowIfFailed. Item 1 of this issue addresses the reverse trio individually; a helper would fix all of them at once and prevent regressions.
2. Cut ~13 call sites of boilerplate to one-liners, which also makes the post-Updated Build Requirements #327 AdbClient migration (Item 2) much more mechanical — there's one place to swap process invocation for socket invocation, not 13.

Sketch:

// e.g. in a new internal RunnerHelpers, or as a protected method on a new RunnerBase
internal static async Task<string> RunToolAsync (
 string toolPath, string description, string[] args,
 CancellationToken cancellationToken,
 IDictionary<string, string>? environmentVariables = null)
{
 using var stdout = new StringWriter ();
 using var stderr = new StringWriter ();
 var psi = ProcessUtils.CreateProcessStartInfo (toolPath, args);
 var exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, cancellationToken, environmentVariables).ConfigureAwait (false);
 ProcessUtils.ThrowIfFailed (exitCode, description, stderr, stdout);
 return stdout.ToString ();
}

Open questions for whoever picks this up:

• Helper location — static on ProcessUtils, new static class RunnerHelpers, or protected method on a new RunnerBase?
• Whether to also model the third shape that exists in the codebase (the "log-on-failure + nullable return" used by GetShellPropertyAsync / RunShellCommandAsync).
• Whether RemoveAllReversePortsAsync should stay in shell-out form for efficiency (it's correctly per-device via host:transport:<serial> + reverse:killforward-all — see [AdbRunner] Add ADB forward port management (follow-up to #305) android-tools#351 conversation), even after the rest move to a list+remove or socket-based approach.