[AdbRunner] Add ADB forward port management (follow-up to #305)#351
Merged
Conversation
This was referenced Apr 30, 2026
rmarinho
added
copilot
Adds the symmetric forward-port pair to the reverse-port methods that landed in dotnet#305. Same `AdbPortSpec` / `AdbPortRule` / `AdbProtocol` types — just four new methods. | Method | adb command | |-------------------------------------------------|--------------------------------------------| | ForwardPortAsync(serial, local, remote) | adb -s <serial> forward <local> <remote> | | RemoveForwardPortAsync(serial, local) | adb -s <serial> forward --remove <local> | | RemoveAllForwardPortsAsync(serial) | adb -s <serial> forward --remove-all | | ListForwardPortsAsync(serial) | adb forward --list (filtered by serial) | `adb forward` and `adb reverse` are not interchangeable — they connect opposite directions. `forward` is host->device (the IDE/harness reaches a service running on the device) and is the path used for JDWP debugger attach (`forward tcp:N jdwp:<pid>`), perf-tracing endpoints exposed by the runtime, and host-side DevFlow agent connect when the agent listens on a device port. Output-format note for `--list`: `adb forward --list` emits one line per rule across all devices in the form `<serial> <local> <remote>` (different from `(reverse) <remote> <local>`). `ListForwardPortsAsync` uses the unscoped `adb forward --list` and filters to the requested serial in `ParseForwardListOutput`. Serial match is case-sensitive (matches adb). - 12 new parser tests in `ParseForwardListOutput_*` mirroring the reverse parser tests (single rule, multiple rules, serial filtering, empty output, malformed lines, non-tcp specs, Windows line endings, tab separation, case sensitivity). - 7 new parameter-validation tests covering empty serial / null spec for the four new public methods. - VS Code MAUI extension ServiceHub->CLI migration (`forwardPort` in `MauiAndroidPlatform.ts` — debugger configurations, perf tooling). - MAUI DevTools CLI (dotnet/maui-labs#197) — `maui android port forward` group, sibling of the existing `reverse` surface. - Visual Studio `ClientTools.Platform` — same paths that drive reverse today. Discussed in dotnet#305 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rmarinho
force-pushed
the
feature/adb-forward-port
branch
from
905c04a to
40c20d0
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds adb forward support to Xamarin.Android.Tools.AdbRunner, mirroring the existing reverse-port forwarding API surface added in #305, including parsing and parameter-validation tests plus PublicAPI updates.
Changes:
- Added
ForwardPortAsync,RemoveForwardPortAsync,RemoveAllForwardPortsAsync, andListForwardPortsAsynctoAdbRunner. - Implemented
ParseForwardListOutputto parse/filteradb forward --listoutput by device serial. - Added new NUnit tests for forward list parsing and forward method parameter validation; updated PublicAPI unshipped entries for both TFMs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs | Adds parser coverage for adb forward --list output and parameter validation tests for the new forward APIs. |
| src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs | Introduces the new forward-port management methods and parsing logic, mirroring the reverse-port implementation patterns. |
| src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt | Records new public API members for netstandard2.0. |
| src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/net10.0/PublicAPI.Unshipped.txt | Records new public API members for net10.0. |
Member
|
/review |
|
✅ Android Tools PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 ✅ LGTM — Clean, well-structured symmetric addition to the reverse-port API.
Summary:
- 4 new public methods (
ForwardPortAsync,RemoveForwardPortAsync,RemoveAllForwardPortsAsync,ListForwardPortsAsync) follow the established reverse-port patterns exactly - Parser handles the different
adb forward --listoutput format correctly (<serial> <local> <remote>vs reverse's(reverse) <remote> <local>) - Uses
ProcessUtilsthroughout, proper parameter validation,IReadOnlyList<T>return types ✅ - PublicAPI files updated for both TFMs ✅
- 19 new tests (12 parser + 7 validation) with good edge-case coverage ✅
- CI is green ✅
| Severity | Count |
|---|---|
1 (null-forgiving ! in 3 test lines — inconsistent with existing reverse tests) |
|
| 💡 suggestion | 1 (positive observation on parser correctness) |
Positive callouts:
- Test coverage mirrors the reverse parser tests comprehensively (serial filtering, tab/CRLF, case sensitivity, malformed lines, non-TCP specs)
- Good PR description explaining the
--listoutput format difference and why client-side serial filtering is needed for forward
Generated by Android Tools PR Reviewer for issue #351 · ● 3.3M
…metry comment, drop null!, remove test region dividers - ForwardPortAsync/RemoveForwardPortAsync/RemoveAllForwardPortsAsync now capture stdout and pass it to ProcessUtils.ThrowIfFailed (matches repo convention; adb sometimes writes errors to stdout). - Added <remarks> block on ParseForwardListOutput calling out the field-order asymmetry vs ParseReverseListOutput (forward: serial local remote; reverse: (reverse) remote local). - Replaced '(AdbPortSpec) null!' with '(AdbPortSpec) null' in 3 forward-port test sites to match reverse-test convention and repo no-null-forgiving rule. - Removed all '// --- ... ---' region-like divider comments in AdbRunnerTests.cs (per jonathanpeppers feedback in PR dotnet#351). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The underlying 'adb forward --remove-all' (and the wire-protocol equivalent 'host-serial:<serial>:killforward-all') is daemon-global -- the '-s <serial>' flag does not scope it. The previous implementation would silently remove forwards for every connected device despite the method's per-device API contract. Reimplement by listing forwards for the given serial via ListForwardPortsAsync and removing them individually via RemoveForwardPortAsync. Update the XML docs to describe the actual behaviour. Add two new tests using a recording subclass of AdbRunner that overrides ListForwardPortsAsync and RemoveForwardPortAsync to verify (1) only ports for the requested serial are removed, and (2) an empty listing is a no-op. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Follow-up to my Thread A reply above: while writing that analysis I flagged Fixed in
The deferred items from Thread A (stdout-parity for the three reverse mutation methods, plus the broader migration to Build clean, 112 AdbRunner tests passing. |
Member
|
/review |
|
✅ Android Tools PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 ✅ LGTM — Clean, well-structured addition of forward port management as the symmetric pair to the existing reverse port methods.
Highlights:
- ✅ All four methods follow the established
Reverse*patterns exactly (validation,ProcessUtils,ConfigureAwait,ThrowIfFailedwith stdout) - ✅
RemoveAllForwardPortsAsynccorrectly avoids the globaladb forward --remove-allpitfall with thorough<remarks>documentation - ✅
ParseForwardListOutputhandles the field-order asymmetry vs reverse correctly, with a clear cross-reference comment - ✅ Public API files updated for both
net10.0andnetstandard2.0TFMs - ✅ Comprehensive test coverage: 12 parser tests (mirroring reverse), 7 parameter validation tests, 2
RecordingAdbRunnercomposition tests - ✅ Banner separator comments removed (repo convention cleanup)
Issues: 0 ❌ | 0
CI note: Only license/cla check run is visible (passing). Build/test CI may be tracked via status checks or a separate pipeline.
Generated by Android Tools PR Reviewer for issue #351 · ● 5M
| using var stderr = new StringWriter (); | ||
| var psi = ProcessUtils.CreateProcessStartInfo (adbPath, "forward", "--list"); | ||
| var exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, cancellationToken, environmentVariables).ConfigureAwait (false); | ||
| ProcessUtils.ThrowIfFailed (exitCode, $"adb forward --list", stderr, stdout); |
There was a problem hiding this comment.
🤖 💡 Code style — The $ prefix is unnecessary here since there are no interpolation expressions. A plain string literal "adb forward --list" avoids the (slight) reader surprise.
Rule: Don't wrap a value in an interpolated string
Comment on lines
+422
to
+431
| public virtual async Task RemoveAllForwardPortsAsync (string serial, CancellationToken cancellationToken = default) | ||
| { | ||
| if (string.IsNullOrWhiteSpace (serial)) | ||
| throw new ArgumentException ("Serial must not be empty.", nameof (serial)); | ||
|
|
||
| var rules = await ListForwardPortsAsync (serial, cancellationToken).ConfigureAwait (false); | ||
| foreach (var rule in rules) { | ||
| cancellationToken.ThrowIfCancellationRequested (); | ||
| await RemoveForwardPortAsync (serial, rule.Local, cancellationToken).ConfigureAwait (false); | ||
| } |
There was a problem hiding this comment.
🤖 💡 Pattern — Excellent design choice. The list-then-remove-individually approach avoids the adb forward --remove-all global-scope pitfall, and the <remarks> documentation explaining why is exemplary — future maintainers will immediately understand the constraint. The virtual overrides also make the composition cleanly testable (as the RecordingAdbRunner tests demonstrate).
Rule: Document non-obvious design decisions
jonathanpeppers
approved these changes
Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the symmetric forward-port pair to the reverse-port methods that landed in #305. Same
AdbPortSpec/AdbPortRule/AdbProtocoltypes — just four new methods. Discussed in #305 (comment).ForwardPortAsync(serial, local, remote)adb -s <serial> forward <local> <remote>RemoveForwardPortAsync(serial, local)adb -s <serial> forward --remove <local>RemoveAllForwardPortsAsync(serial)adb -s <serial> forward --remove-allListForwardPortsAsync(serial)adb forward --list(filtered by serial)Why we need this in addition to reverse
adb forwardandadb reverseare not interchangeable — they connect opposite directions:reverse <remote-on-device> <local-on-host>— device-side socket forwards to host-side. Used by hot reload (the device app connects to a "device" port that's actually tunnelled to the IDE host).forward <local-on-host> <remote-on-device>— host-side socket forwards to device-side. Used when the IDE / harness needs to connect to a service running on the device:forward tcp:N jdwp:<pid>--listoutput format noteadb forward --listemits one line per rule across all devices in the form<serial> <local> <remote>(different from(reverse) <remote> <local>).ListForwardPortsAsyncuses the unscopedadb forward --listand filters to the requested serial inParseForwardListOutput. Serial match is case-sensitive to match adb's behaviour.Tests
ParseForwardListOutput_*) mirroring the reverse parser tests:All
Forward+Reversetests pass locally (36/36). Full suite has one pre-existing unrelatedJdkDirectory_JavaHomefailure onmainthat's not touched by this PR.Consumers
forwardPortinMauiAndroidPlatform.ts(debugger configurations, perf tooling).maui android port forward …group, sibling of the existingreversesurface.ClientTools.Platform— same paths that drive reverse today have parallel forward call-sites.Public API additions