[dotnet] [bidi] Possibility to use any external transport

[nvborisenko]

2 reasons:

• Anybody can provide transport implementation
• Our internal testing

💥 What does this PR do?

This pull request introduces improvements to the BiDi transport configuration in the WebDriver .NET implementation. The main changes add flexibility for specifying custom transport implementations and improve interface accessibility.

Transport configuration enhancements

• Added a new UseTransport(ITransport transport) method to the BiDiOptionsBuilder class, allowing consumers to inject a custom ITransport instance for BiDi connections. This makes it easier to provide alternative transport mechanisms for advanced scenarios or testing.

Interface accessibility

• Changed the ITransport interface from internal to public, making it accessible for external implementations and enabling the use of custom transports with the new builder method.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s):
  • What was generated:
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

[qodo-code-review]

Review Summary by Qodo

Make ITransport public and add custom transport injection

✨ Enhancement

Walkthroughs

Description

• Make ITransport interface public for external implementations
• Add UseTransport() method to inject custom transport instances
• Enable flexible transport configuration for advanced scenarios

Diagram

flowchart LR
  BiDiOptionsBuilder["BiDiOptionsBuilder"]
  UseTransport["UseTransport method"]
  ITransport["ITransport interface"]
  CustomTransport["Custom transport instance"]
  
  BiDiOptionsBuilder -- "new method" --> UseTransport
  UseTransport -- "accepts" --> CustomTransport
  CustomTransport -- "implements" --> ITransport
  ITransport -- "now public" --> CustomTransport

Loading

File Changes

1. dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs ✨ Enhancement +11/-0

Add custom transport injection method

• Added UseTransport(ITransport transport) method to builder
• Method sets TransportFactory to return provided transport instance
• Enables method chaining by returning builder instance
• Allows injection of custom transport implementations

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs

---

2. dotnet/src/webdriver/BiDi/ITransport.cs ✨ Enhancement +1/-1

Make ITransport interface publicly accessible

• Changed ITransport interface from internal to public
• Enables external implementations of transport interface
• Maintains all existing interface members and contracts

dotnet/src/webdriver/BiDi/ITransport.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. ThrowIfNull TFM break 🐞 Bug ≡ Correctness

Description

BiDiOptionsBuilder.UseTransport uses ArgumentNullException.ThrowIfNull, which is unavailable on
net462 and netstandard2.0, so multi-target builds for Selenium.WebDriver will fail to compile
for those TFMs.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[55]

+        ArgumentNullException.ThrowIfNull(transport);

Evidence

The new code calls ArgumentNullException.ThrowIfNull(transport), but the project explicitly
targets net462 and netstandard2.0, which do not provide this API, making the build fail for
those target frameworks.

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[53-60]
dotnet/src/webdriver/Selenium.WebDriver.csproj[1-6]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ArgumentNullException.ThrowIfNull(...)` is used for parameter validation in `BiDiOptionsBuilder.UseTransport(ITransport transport)`, but Selenium.WebDriver targets `net462` and `netstandard2.0` where this API does not exist, causing compilation failures.

### Issue Context
Selenium.WebDriver is multi-targeted (`net462;netstandard2.0;net8.0`). The null-check should be implemented with APIs available across all target frameworks (or conditionally compiled).

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[53-60]

### Suggested fix
Replace:
```csharp
ArgumentNullException.ThrowIfNull(transport);
```
with a framework-portable check:
```csharp
if (transport is null)
{
   throw new ArgumentNullException(nameof(transport));
}
```
(or use conditional compilation if you want to keep `ThrowIfNull` for newer TFMs).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. UseTransport ignores cancellationToken ✓ Resolved 📘 Rule violation ☼ Reliability

Description

BiDiOptionsBuilder.UseTransport(ITransport transport) sets TransportFactory to return
Task.FromResult(transport) and discards the provided CancellationToken, so
BiDi.ConnectAsync(..., cancellationToken) cannot be cancelled (and may even succeed when the token
is already cancelled) when this injected-transport path is used. This breaks expected .NET
cancellation semantics, is inconsistent with the default WebSocket transport path that honors
cancellation, and can lead to non-deterministic hangs or delayed cancellation during connection
setup.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[55]

+        return UseTransport((_, _) => Task.FromResult(transport));

Evidence

Rule 11 requires async/network-related waits and operations to honor cancellation for deterministic
failure paths. The evidence indicates that BiDi.ConnectAsync forwards the caller-provided
cancellationToken into TransportFactory(...), and the default WebSocket-based factory uses that
token during the actual connection (e.g., for ClientWebSocket.ConnectAsync). In contrast, the
instance-based UseTransport(ITransport) implementation assigns a factory like `(_, _) =>
Task.FromResult(transport)`, which completes synchronously regardless of cancellation state;
therefore the cancellation token is ignored and cancellation cannot stop (or prevent) transport
acquisition in this path.

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[53-56]
dotnet/src/webdriver/BiDi/BiDi.cs[56-62]
dotnet/src/webdriver/BiDi/BiDi.cs[56-66]
dotnet/src/webdriver/BiDi/WebSocketTransport.cs[32-41]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BiDiOptionsBuilder.UseTransport(ITransport transport)` creates a `TransportFactory` that ignores the `CancellationToken` parameter (returns a completed task via `Task.FromResult(transport)`), so `BiDi.ConnectAsync(..., cancellationToken)` cannot cancel transport acquisition and may even proceed successfully when the token is already cancelled.

## Issue Context
`BiDi.ConnectAsync` forwards the caller’s `cancellationToken` into `builder.TransportFactory(...)`. The default WebSocket transport factory honors cancellation during connection (e.g., via `ClientWebSocket.ConnectAsync`), so callers expect consistent .NET cancellation semantics across transport configuration paths; however, the injected transport-instance path currently discards the token.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[53-56]

## Suggested fix
Update the instance-based factory to observe cancellation before returning the provided transport instance, e.g.:
- implement `UseTransport((_, ct) => ct.IsCancellationRequested ? Task.FromCanceled<ITransport>(ct) : Task.FromResult(transport))`, or equivalently call `ct.ThrowIfCancellationRequested()` before returning the transport.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. No transport null-check ✓ Resolved 🐞 Bug ≡ Correctness

Description

BiDiOptionsBuilder.UseTransport captures the provided transport without validating it, so passing
null makes TransportFactory return null and later causes a NullReferenceException when Broker uses
the transport. This turns a simple caller mistake into a late, non-actionable runtime crash during
connection setup.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[R54-57]

+    public BiDiOptionsBuilder UseTransport(ITransport transport)
+    {
+        TransportFactory = (_, _) => Task.FromResult(transport);
+        return this;

Evidence

UseTransport sets TransportFactory to return the provided transport instance without guarding
against null. The transport returned by TransportFactory is immediately used to construct a Broker,
and Broker unconditionally invokes methods on _transport, so a null transport will crash with an
NRE.

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-58]
dotnet/src/webdriver/BiDi/BiDi.cs[56-66]
dotnet/src/webdriver/BiDi/Broker.cs[39-66]
dotnet/src/webdriver/BiDi/Broker.cs[118-140]
dotnet/src/webdriver/BiDi/Broker.cs[315-337]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`BiDiOptionsBuilder.UseTransport(ITransport transport)` does not validate `transport` and will happily set `TransportFactory` to return `null` when the caller passes `null`, leading to a runtime `NullReferenceException` downstream.

### Issue Context
`BiDi.ConnectAsync` calls `builder.TransportFactory(...)` and passes the resulting transport into `Broker`, which unconditionally calls `SendAsync`, `ReceiveAsync`, and `DisposeAsync` on that instance.

### Fix
- Add `ArgumentNullException.ThrowIfNull(transport);` at the start of `UseTransport`.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-58]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. UseTransport lacks tests 📘 Rule violation ☼ Reliability

Description

The PR introduces new public transport injection (BiDiOptionsBuilder.UseTransport and public
ITransport) without adding any tests to verify it is honored by BiDi.ConnectAsync and behaves
correctly (e.g., uses the provided instance and disposes it). This increases regression risk for a
new, user-facing configuration path.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[R49-58]

+    /// <summary>
+    /// Configures the BiDi connection to use the specified <see cref="ITransport"/> instance.
+    /// </summary>
+    /// <param name="transport">The transport instance to use.</param>
+    /// <returns>The current <see cref="BiDiOptionsBuilder"/> instance for chaining.</returns>
+    public BiDiOptionsBuilder UseTransport(ITransport transport)
+    {
+        TransportFactory = (_, _) => Task.FromResult(transport);
+        return this;
+    }

Evidence

PR Compliance ID 6 requires adding/updating tests for behavior changes when feasible. This PR adds a
new public UseTransport configuration method and exposes ITransport publicly, but no
corresponding test changes are present to validate this new code path.

AGENTS.md: Add/Update Tests for Changes; Prefer Small Unit Tests and Avoid Mocks
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-58]
dotnet/src/webdriver/BiDi/ITransport.cs[24-29]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new public API (`BiDiOptionsBuilder.UseTransport(ITransport transport)` and public `ITransport`) was added, but the PR includes no tests validating that `BiDi.ConnectAsync` actually uses the provided transport instance and that lifecycle/disposal expectations are correct.

## Issue Context
`BiDi.ConnectAsync` builds a transport via `BiDiOptionsBuilder.TransportFactory` and immediately constructs a `Broker` that starts background receive/processing tasks. This is a user-facing configuration path and should be covered by small tests to avoid regressions.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-58]
- dotnet/src/webdriver/BiDi/ITransport.cs[24-29]
- dotnet/src/webdriver/BiDi/BiDi.cs[56-73]
- dotnet/test/webdriver/BiDi/BiDiFixture.cs[28-76]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Transport disposal undocumented 🐞 Bug ⚙ Maintainability

Description

UseTransport’s XML docs don’t state that BiDi takes ownership of the supplied transport, but
Broker.DisposeAsync always disposes _transport. Callers that intend to manage or reuse the
injected transport can unexpectedly end up with a disposed instance.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[R49-56]

+    /// <summary>
+    /// Configures the BiDi connection to use the specified <see cref="ITransport"/> instance.
+    /// </summary>
+    /// <param name="transport">The transport instance to use.</param>
+    /// <returns>The current <see cref="BiDiOptionsBuilder"/> instance for chaining.</returns>
+    public BiDiOptionsBuilder UseTransport(ITransport transport)
+    {
+        TransportFactory = (_, _) => Task.FromResult(transport);

Evidence

The new UseTransport API documentation does not mention disposal, yet Broker.DisposeAsync always
disposes the transport, proving that injected transports are owned/disposed by BiDi.

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-58]
dotnet/src/webdriver/BiDi/Broker.cs[144-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`UseTransport(ITransport transport)` introduces a new public API for injecting a transport instance, but its XML documentation does not mention lifetime/ownership semantics.

### Issue Context
`Broker.DisposeAsync()` always calls `_transport.DisposeAsync()`, which means the BiDi connection owns the transport lifetime even when it’s injected.

### Fix
- Update the XML doc for `UseTransport` to explicitly state that the provided transport will be disposed when the BiDi connection (`IBiDi`) is disposed.
- (Optional, if desired) consider an overload/pattern to support "leave transport open" semantics for advanced callers.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-58]
- dotnet/src/webdriver/BiDi/Broker.cs[144-166]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3327798

[qodo-code-review]

Code review by qodo was updated up to the latest commit c4fe44e

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8513649

[nvborisenko]

Perfect, now we can test edge cases, what cannot be performed with real browser.