[dotnet] [bidi] Make BiDi transport factories composable

[nvborisenko]

Advanced users now can add additional layer over default low-level transport. Examples: listeners, interceptions, diagnostics, logging...

🔧 Implementation Notes

Only one change: Now bidi options expose next delegate.

Examples

Note: Could be tuned later, but for now it is great opportunity for hackers.

1. Default behavior

var bidi = await BiDi.ConnectAsync(wsUrl);

2. Customize WebSocket options only

var bidi = await BiDi.ConnectAsync(wsUrl, opts =>
    opts.UseWebSocket(ws =>
    {
        ws.SetRequestHeader("X-Trace-Id", "123");
    }));

3. Replace transport completely

var bidi = await BiDi.ConnectAsync(wsUrl, opts =>
    opts.UseTransport(_ => (uri, ct) => MyCustomTransport.ConnectAsync(uri, ct)));

4. Customize WebSocket options and wrap transport

var bidi = await BiDi.ConnectAsync(wsUrl, opts =>
    opts
        .UseWebSocket(ws => ws.SetRequestHeader("X-Trace-Id", "123"))
        .UseTransport(next => async (uri, ct) =>
        {
            var inner = await next(uri, ct);
            return new LoggingTransport(inner);
        }));

5. Chain multiple wrappers

var bidi = await BiDi.ConnectAsync(wsUrl, opts =>
    opts
        .UseTransport(next => async (uri, ct) =>
            new MetricsTransport(await next(uri, ct)))
        .UseTransport(next => async (uri, ct) =>
            new RetryTransport(await next(uri, ct))));

🤖 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

🔄 Types of changes

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

[qodo-code-review]

PR Summary by Qodo

Make BiDi transport factories composable via decorator-style UseTransport

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Allow composing BiDi transport factories by exposing a next factory delegate.
• Enforce non-null ITransport results from the configured transport factory.
• Update BiDi session unit tests to use the new composable factory API.

Diagram

graph TD
  U(("Caller")) --> C["BiDi.ConnectAsync"] --> B["BiDiOptionsBuilder"] --> P[["Transport factory chain"]] --> WS["WebSocketTransport.ConnectAsync"] --> T[("ITransport")] --> S["BiDi session"]
  B --> D["UseTransport(next)"] -."decorates".-> P
  subgraph Legend
    direction LR
    _a(("Caller")) ~~~ _p["API method"] ~~~ _s[["Composable step"]] ~~~ _t[("Transport")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add a new decorator API while keeping the old UseTransport(Func) overload

• ➕ Avoids breaking existing consumers that pass a simple factory
• ➕ Provides an easy on-ramp (simple factory) and an advanced on-ramp (decorator)
• ➖ Slightly larger surface area to maintain
• ➖ Requires deciding composition order semantics when both overload styles are used

2. Introduce an ITransportFactory interface (or ITransportFactoryDecorator) instead of delegates

• ➕ More discoverable and self-documenting than nested delegates
• ➕ Easier to unit test and mock in isolation; enables DI-friendly patterns
• ➖ More types/boilerplate for a simple extension point
• ➖ Potentially heavier API change than the current delegate approach

3. Model composition as an ordered middleware list (pipeline builder)

• ➕ Very clear composition ordering and behavior (middleware-style)
• ➕ Allows multiple decorators without nesting delegate signatures
• ➖ More implementation complexity than the current single-delegate approach
• ➖ May be overkill if only a few advanced users need this

Recommendation: The decorator-style UseTransport(next) is a good fit for enabling interception/logging without redesigning the transport layer. Consider adding back a convenience overload for simple factories (or a small helper) to reduce friction and limit breaking changes, while keeping next as the advanced composition mechanism.

Files changed (3) +19 / -23

Enhancement (1) +15 / -21

BiDiOptionsBuilder.csReplace single factory setter with composable transport factory decorator +15/-21

Replace single factory setter with composable transport factory decorator

• Introduces a default transport factory and changes UseTransport to accept a decorator callback that composes the current factory into a chain. Updates UseWebSocket to directly set the transport factory and clarifies XML docs around ownership and composition semantics.

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs

Bug fix (1) +2 / -1

BiDi.csValidate non-null transport from the configured factory +2/-1

Validate non-null transport from the configured factory

• Adds a null-guard after invoking the configured TransportFactory. ConnectAsync now throws a clear InvalidOperationException if the factory returns null.

dotnet/src/webdriver/BiDi/BiDi.cs

Tests (1) +2 / -1

SessionUnitTests.csAdapt tests to new composable UseTransport signature +2/-1

Adapt tests to new composable UseTransport signature

• Updates test setup to provide a decorated transport factory that returns the FakeTransport via Task.FromResult. Keeps existing test behavior while exercising the new API shape.

dotnet/test/webdriver/BiDi/SessionUnitTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

1. Missing null transport tests ✗ Dismissed 📘 Rule violation ▣ Testability ⭐ New

Description

The new null-guard behavior in ConnectAsync is a bug fix/behavioral change but no regression test
covers the null task/transport scenarios. Add tests that assert InvalidOperationException (and
ideally the message) for these invalid factory returns.

Code

dotnet/src/webdriver/BiDi/BiDi.cs[R63-66]

+        var transportFactoryTask = builder.TransportFactory(url, cancellationToken)
+            ?? throw new InvalidOperationException("The transport factory must return a non-null Task<ITransport> instance.");
+
+        var transport = await transportFactoryTask.ConfigureAwait(false)

Evidence

The checklist requires tests for new functionality and bug fixes. The diff adds new error-handling
behavior in ConnectAsync, and the current BiDi unit tests only cover successful connection setup
without asserting the new null-return guard paths.

Rule 389273: Require tests for all new functionality and bug fixes
dotnet/src/webdriver/BiDi/BiDi.cs[63-66]
dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-40]

Agent prompt

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

## Issue description
`BiDi.ConnectAsync` now checks for `null` transport factory task and `null` transport results, but there are no tests asserting these error paths. This risks regressions where the behavior/message changes unintentionally.

## Issue Context
Existing BiDi tests exercise the happy-path `ConnectAsync` setup, but do not validate the new guard behavior.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDi.cs[63-66]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-40] (or add a new test file under dotnet/test/webdriver/BiDi/)

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

---

2. ConnectAsync adds new exception ✗ Dismissed 📘 Rule violation ≡ Correctness ⭐ New

Description

BiDi.ConnectAsync now throws InvalidOperationException with new user-visible messages when a
transport factory returns null task/transport. Per the cross-language consistency rule, this
behavior/message change should be compared with at least one other binding (or documented as an
intentional divergence).

Code

dotnet/src/webdriver/BiDi/BiDi.cs[R63-66]

+        var transportFactoryTask = builder.TransportFactory(url, cancellationToken)
+            ?? throw new InvalidOperationException("The transport factory must return a non-null Task<ITransport> instance.");
+
+        var transport = await transportFactoryTask.ConfigureAwait(false)

Evidence

The checklist requires cross-language comparison when changing user-visible behavior/messages. The
diff adds explicit exception messages in ConnectAsync for null-returning transport factories,
which is a user-visible behavior change.

Rule 389265: Compare cross-language bindings when changing user-visible behavior
dotnet/src/webdriver/BiDi/BiDi.cs[63-66]

Agent prompt

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

## Issue description
`BiDi.ConnectAsync` now has new user-visible failure behavior/messages when the transport factory returns `null` (task or transport). The compliance checklist requires comparing such user-visible behavior changes across at least one other language binding (or documenting an intentional divergence).

## Issue Context
The new behavior is introduced by explicit null-guards with specific exception messages.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDi.cs[63-66]
- (Add documentation/comment near the change OR update PR documentation to reference the other binding compared)

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

---

3. Null transport task NRE ✓ Resolved 🐞 Bug ☼ Reliability

Description

BiDi.ConnectAsync awaits the Task returned from TransportFactory directly; if a user-supplied
factory returns null instead of a Task, the await throws NullReferenceException before the intended
InvalidOperationException. Validate the returned task before awaiting so malformed factories fail
with a clear, actionable exception.

Code

dotnet/src/webdriver/BiDi/BiDi.cs[R63-64]

+        var transport = await builder.TransportFactory(url, cancellationToken).ConfigureAwait(false)
+            ?? throw new InvalidOperationException("The transport factory must return a non-null ITransport instance.");

Evidence

ConnectAsync awaits the TransportFactory invocation inline and only checks the *result* for null
afterward, so a null Task will fail earlier with a NullReferenceException. Because TransportFactory
is user-composable via UseTransport, a malformed decorator can create such a factory.

dotnet/src/webdriver/BiDi/BiDi.cs[56-65]
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[62-70]

Agent prompt

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

### Issue description
`BiDi.ConnectAsync` added a null check for the awaited transport result, but a malformed transport factory can still return a `null` *task*, causing a `NullReferenceException` at the `await` expression before the intended validation runs.

### Issue Context
With the new composable transport factory API, consumers are more likely to create custom factories/decorators. A clearer exception message when a factory returns `null` helps diagnose misconfigurations quickly.

### Fix Focus Areas
- Assign the result of `TransportFactory(url, cancellationToken)` to a local variable and check it for null before awaiting.
- Throw a targeted `InvalidOperationException` (or `ArgumentException`) when the returned task is null.

- dotnet/src/webdriver/BiDi/BiDi.cs[56-65]

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

---

View more (2)

4. UseTransport composition untested 📘 Rule violation ▣ Testability

Description

The new transport-factory composition behavior is new functionality, but there is no test verifying
that multiple UseTransport(next) calls compose correctly (order, invocation, and disposal
expectations). This increases the risk of regressions in an API intended for advanced customization.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[R66-70]

+        var factory = next(TransportFactory)
+            ?? throw new InvalidOperationException("The transport factory decorator must return a non-null factory.");

-            return Task.FromResult(transport);
-        });
-    }
-
-    private BiDiOptionsBuilder UseTransport(Func<Uri, CancellationToken, Task<ITransport>> factory)
-    {
        TransportFactory = factory;
        return this;

Evidence

The checklist requires tests for new functionality. The production code now composes a new transport
factory from the existing one via next(TransportFactory) and assigns it back, but the test changes
only update a single injection call site to the new signature and do not assert the new composition
semantics.

Rule 389273: Require tests for all new functionality and bug fixes
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[62-70]
dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-53]

Agent prompt

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

## Issue description
The new composable `UseTransport(next)` pipeline behavior is not covered by tests.

## Issue Context
This PR changes the transport customization model to a decorator chain. Without tests, ordering and chaining semantics (including non-null enforcement) can easily regress.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[62-70]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-63]

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

---

5. UseTransport signature breaks callers ✗ Dismissed 📘 Rule violation ≡ Correctness

Description

BiDiOptionsBuilder.UseTransport has been changed from its prior factory-delegate shape to a
decorator (next => ...) signature, which is a breaking public API/ABI change for existing
Selenium.WebDriver consumers and forces call-site updates. The previous API is not retained as an
overload or deprecated with guidance, and the change also risks losing the prior early-cancellation
behavior for simple/synchronous factories if the composed pipeline ignores the cancellation token.

Code

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

+    public BiDiOptionsBuilder UseTransport(Func<Func<Uri, CancellationToken, Task<ITransport>>, Func<Uri, CancellationToken, Task<ITransport>>> next)

Evidence

The compliance requirements called out in the preferred evidence emphasize preserving
backward-compatible public API/ABI and using guided deprecation before removal; however, the PR
modifies the public UseTransport(...) surface to a new decorator-based form without keeping the
previous overload or adding an [Obsolete("Use ... instead.")] transition path. The duplicate
evidence further supports this by noting that the only public overload now requires a nested-Func
decorator and that tests were rewritten to pass a decorator-shaped factory, demonstrating that
existing call sites no longer compile and that this is a consumer-visible breaking change in the
Selenium.WebDriver v4.0.0 package.

Rule 389266: Maintain backward-compatible public API and ABI
Rule 389271: Deprecate public APIs with guidance before removal
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[52-71]
dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-39]
dotnet/src/webdriver/Selenium.WebDriver.csproj[12-21]

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` is a public API and its signature was changed to a decorator-based callback, which breaks existing callers (and potentially compiled consumers expecting the old overloads). The prior factory-shaped API should be restored (at least temporarily) as backward-compatible overload(s) and deprecated with clear replacement guidance, while also preserving the earlier early-cancellation behavior for simple/synchronous factories.

## Issue Context
The current implementation exposes a composable decorator pipeline (`UseTransport(next => ...)`), but callers using the previous `UseTransport(() => transport)`-style pattern will fail to compile. This repository ships the `Selenium.WebDriver` package (version 4.0.0), so removing/changing public methods is a consumer-visible breaking change; additionally, the earlier simple factory overload provided an early-cancellation guard (returning a canceled task before invoking the factory) that can be lost if the composed factory ignores the token.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[28-71]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-39]

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

---

Context used

✅ Compliance rules (platform): 15 rules

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit 375ab25

Results up to commit 89db494 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Null transport task NRE ✓ Resolved 🐞 Bug ☼ Reliability

Description

BiDi.ConnectAsync awaits the Task returned from TransportFactory directly; if a user-supplied
factory returns null instead of a Task, the await throws NullReferenceException before the intended
InvalidOperationException. Validate the returned task before awaiting so malformed factories fail
with a clear, actionable exception.

Code

dotnet/src/webdriver/BiDi/BiDi.cs[R63-64]

+        var transport = await builder.TransportFactory(url, cancellationToken).ConfigureAwait(false)
+            ?? throw new InvalidOperationException("The transport factory must return a non-null ITransport instance.");

Evidence

ConnectAsync awaits the TransportFactory invocation inline and only checks the *result* for null
afterward, so a null Task will fail earlier with a NullReferenceException. Because TransportFactory
is user-composable via UseTransport, a malformed decorator can create such a factory.

dotnet/src/webdriver/BiDi/BiDi.cs[56-65]
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[62-70]

Agent prompt

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

### Issue description
`BiDi.ConnectAsync` added a null check for the awaited transport result, but a malformed transport factory can still return a `null` *task*, causing a `NullReferenceException` at the `await` expression before the intended validation runs.

### Issue Context
With the new composable transport factory API, consumers are more likely to create custom factories/decorators. A clearer exception message when a factory returns `null` helps diagnose misconfigurations quickly.

### Fix Focus Areas
- Assign the result of `TransportFactory(url, cancellationToken)` to a local variable and check it for null before awaiting.
- Throw a targeted `InvalidOperationException` (or `ArgumentException`) when the returned task is null.

- dotnet/src/webdriver/BiDi/BiDi.cs[56-65]

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

---

2. UseTransport composition untested 📘 Rule violation ▣ Testability

Description

The new transport-factory composition behavior is new functionality, but there is no test verifying
that multiple UseTransport(next) calls compose correctly (order, invocation, and disposal
expectations). This increases the risk of regressions in an API intended for advanced customization.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[R66-70]

+        var factory = next(TransportFactory)
+            ?? throw new InvalidOperationException("The transport factory decorator must return a non-null factory.");

-            return Task.FromResult(transport);
-        });
-    }
-
-    private BiDiOptionsBuilder UseTransport(Func<Uri, CancellationToken, Task<ITransport>> factory)
-    {
        TransportFactory = factory;
        return this;

Evidence

The checklist requires tests for new functionality. The production code now composes a new transport
factory from the existing one via next(TransportFactory) and assigns it back, but the test changes
only update a single injection call site to the new signature and do not assert the new composition
semantics.

Rule 389273: Require tests for all new functionality and bug fixes
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[62-70]
dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-53]

Agent prompt

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

## Issue description
The new composable `UseTransport(next)` pipeline behavior is not covered by tests.

## Issue Context
This PR changes the transport customization model to a decorator chain. Without tests, ordering and chaining semantics (including non-null enforcement) can easily regress.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[62-70]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-63]

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

---

3. UseTransport signature breaks callers ✗ Dismissed 📘 Rule violation ≡ Correctness

Description

BiDiOptionsBuilder.UseTransport has been changed from its prior factory-delegate shape to a
decorator (next => ...) signature, which is a breaking public API/ABI change for existing
Selenium.WebDriver consumers and forces call-site updates. The previous API is not retained as an
overload or deprecated with guidance, and the change also risks losing the prior early-cancellation
behavior for simple/synchronous factories if the composed pipeline ignores the cancellation token.

Code

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

+    public BiDiOptionsBuilder UseTransport(Func<Func<Uri, CancellationToken, Task<ITransport>>, Func<Uri, CancellationToken, Task<ITransport>>> next)

Evidence

The compliance requirements called out in the preferred evidence emphasize preserving
backward-compatible public API/ABI and using guided deprecation before removal; however, the PR
modifies the public UseTransport(...) surface to a new decorator-based form without keeping the
previous overload or adding an [Obsolete("Use ... instead.")] transition path. The duplicate
evidence further supports this by noting that the only public overload now requires a nested-Func
decorator and that tests were rewritten to pass a decorator-shaped factory, demonstrating that
existing call sites no longer compile and that this is a consumer-visible breaking change in the
Selenium.WebDriver v4.0.0 package.

Rule 389266: Maintain backward-compatible public API and ABI
Rule 389271: Deprecate public APIs with guidance before removal
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[52-71]
dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-39]
dotnet/src/webdriver/Selenium.WebDriver.csproj[12-21]

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` is a public API and its signature was changed to a decorator-based callback, which breaks existing callers (and potentially compiled consumers expecting the old overloads). The prior factory-shaped API should be restored (at least temporarily) as backward-compatible overload(s) and deprecated with clear replacement guidance, while also preserving the earlier early-cancellation behavior for simple/synchronous factories.

## Issue Context
The current implementation exposes a composable decorator pipeline (`UseTransport(next => ...)`), but callers using the previous `UseTransport(() => transport)`-style pattern will fail to compile. This repository ships the `Selenium.WebDriver` package (version 4.0.0), so removing/changing public methods is a consumer-visible breaking change; additionally, the earlier simple factory overload provided an early-cancellation guard (returning a canceled task before invoking the factory) that can be lost if the composed factory ignores the token.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[28-71]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[33-39]

ⓘ 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 375ab25