Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 14, 2026

User description

💥 What does this PR do?

Make NewAsync command publicly available.

🔧 Implementation Notes

Session module is still exposed via main BiDi object.

💡 Additional Considerations

I don't know how to use this command in our world, it fails with error session already exists. But I successfully use it when launched BiDi mapper (https://github.com/GoogleChromeLabs/chromium-bidi).

🔄 Types of changes

  • New feature

PR Type

Enhancement


Description

  • Expose NewAsync command publicly via BiDi object

  • Add UnhandledPromptBehavior property to Capability model

  • Rename Capability field to Capabilities in NewResult

  • Update StatusAsync to accept optional StatusOptions parameter

  • Add SessionTest with status verification test


Diagram Walkthrough

flowchart LR
  BiDi["BiDi object"]
  NewAsync["NewAsync command"]
  StatusAsync["StatusAsync with options"]
  Capability["Capability model"]
  UnhandledPrompt["UnhandledPromptBehavior property"]
  SessionTest["SessionTest.cs"]
  
  BiDi -- "expose" --> NewAsync
  BiDi -- "enhance" --> StatusAsync
  Capability -- "add" --> UnhandledPrompt
  SessionTest -- "verify" --> StatusAsync
Loading

File Walkthrough

Relevant files
Enhancement
BiDi.cs
Expose NewAsync and enhance StatusAsync methods                   

dotnet/src/webdriver/BiDi/BiDi.cs

  • Remove parameterless StatusAsync() method
  • Add StatusAsync(Session.StatusOptions? options) with optional
    parameters
  • Add new public NewAsync(CapabilitiesRequest capabilities,
    Session.NewOptions? options) method
  • Expose Session module commands at BiDi object level
+10/-5   
Bug fix
NewCommand.cs
Add UnhandledPromptBehavior and fix field naming                 

dotnet/src/webdriver/BiDi/Session/NewCommand.cs

  • Rename Capability field to Capabilities in NewResult record
  • Add UnhandledPromptBehavior property to Capability class
  • Maintain existing AcceptInsecureCerts, BrowserName, BrowserVersion,
    PlatformName, SetWindowRect, UserAgent properties
+3/-1     
Formatting
SessionModule.cs
Rename parameter for consistency                                                 

dotnet/src/webdriver/BiDi/Session/SessionModule.cs

  • Rename parameter from capabilitiesRequest to capabilities for
    consistency
  • Update internal parameter assignment to use renamed variable
  • No functional changes to NewAsync method logic
+2/-2     
Tests
SessionTest.cs
Add SessionTest with status verification                                 

dotnet/test/common/BiDi/Session/SessionTest.cs

  • Create new test class SessionTest extending BiDiTestFixture
  • Add CanGetStatus() test method verifying StatusAsync returns non-null
    result
  • Verify status message is not empty
  • Include proper copyright and license headers
+38/-0   

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 14, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logging: The newly exposed session-level actions (StatusAsync, NewAsync, EndAsync) do not add any
audit logging, which may be required if session lifecycle events are considered critical
actions in this library.

Referred Code
public Task<Session.StatusResult> StatusAsync(Session.StatusOptions? options = null)
{
    return SessionModule.StatusAsync(options);
}

public Task<Session.NewResult> NewAsync(Session.CapabilitiesRequest capabilities, Session.NewOptions? options = null)
{
    return SessionModule.NewAsync(capabilities, options);
}

public Task EndAsync(Session.EndOptions? options = null)
{
    return SessionModule.EndAsync(options);
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing input validation: The new public NewAsync(Session.CapabilitiesRequest capabilities, ...) forwards
capabilities without null/shape validation or additional failure context, relying on
downstream behavior that is not visible in the diff.

Referred Code
public Task<Session.NewResult> NewAsync(Session.CapabilitiesRequest capabilities, Session.NewOptions? options = null)
{
    return SessionModule.NewAsync(capabilities, options);
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unvalidated capability data: The newly added UnhandledPromptBehavior capability and the publicly exposed NewAsync
pathway accept externally provided capability/options data without visible
sanitization/validation in the diff, requiring confirmation that validation occurs
elsewhere.

Referred Code
public sealed record NewResult(string SessionId, Capability Capabilities) : EmptyResult;

public sealed record Capability(bool AcceptInsecureCerts, string BrowserName, string BrowserVersion, string PlatformName, bool SetWindowRect, string UserAgent)
{
    public ProxyConfiguration? Proxy { get; set; }

    public UserPromptHandler? UnhandledPromptBehavior { get; set; }

    public string? WebSocketUrl { get; set; }

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 14, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Re-evaluate exposing the session.new command

The session.new command is being exposed on the public API, but it's known to
fail with a "session already exists" error and lacks tests. Its utility and
placement should be reconsidered before merging.

Examples:

dotnet/src/webdriver/BiDi/BiDi.cs [77-80]
    public Task<Session.NewResult> NewAsync(Session.CapabilitiesRequest capabilities, Session.NewOptions? options = null)
    {
        return SessionModule.NewAsync(capabilities, options);
    }

Solution Walkthrough:

Before:

// file: dotnet/src/webdriver/BiDi/BiDi.cs
public sealed class BiDi : IAsyncDisposable
{
    // ...

    public Task<Session.StatusResult> StatusAsync(Session.StatusOptions? options = null)
    {
        return SessionModule.StatusAsync(options);
    }

    public Task<Session.NewResult> NewAsync(Session.CapabilitiesRequest capabilities, Session.NewOptions? options = null)
    {
        return SessionModule.NewAsync(capabilities, options);
    }

    // ...
}

After:

// file: dotnet/src/webdriver/BiDi/BiDi.cs
public sealed class BiDi : IAsyncDisposable
{
    // ...

    public Task<Session.StatusResult> StatusAsync(Session.StatusOptions? options = null)
    {
        return SessionModule.StatusAsync(options);
    }

    // The NewAsync method is removed from the public API
    // until its use case is clarified and it can be properly tested.
    // It could be kept as an internal method if needed for other purposes.

    // ...
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a major design flaw: exposing a NewAsync method that is likely unusable and untested, as confirmed by the PR author's own comments.

High
General
Respect options cancellation token

In ConnectAsync, use the cancellation token from the options parameter when
calling bidi.Broker.ConnectAsync instead of always passing
CancellationToken.None.

dotnet/src/webdriver/BiDi/BiDi.cs [63-70]

 public static async Task<BiDi> ConnectAsync(string url, BiDiOptions? options = null)
 {
     var bidi = new BiDi(url);
 
-    await bidi.Broker.ConnectAsync(CancellationToken.None).ConfigureAwait(false);
+    var token = options?.CancellationToken ?? CancellationToken.None;
+    await bidi.Broker.ConnectAsync(token).ConfigureAwait(false);
 
     return bidi;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: This suggestion identifies a bug where the options parameter in ConnectAsync is ignored, and the method incorrectly uses CancellationToken.None. The proposed fix correctly implements the expected behavior of honoring the provided cancellation token.

Medium
Possible issue
Add a test for new session creation

Add a test case for the new NewAsync method to verify that a new session can be
successfully created and returns the expected details.

dotnet/test/common/BiDi/Session/SessionTest.cs [26-38]

 internal class SessionTest : BiDiTestFixture
 {
     [Test]
     public async Task CanGetStatus()
     {
         var status = await bidi.StatusAsync();
 
         Assert.That(status, Is.Not.Null);
         Assert.That(status.Message, Is.Not.Empty);
     }
 
-    
+    [Test]
+    public async Task CanStartNewSession()
+    {
+        var capabilities = new CapabilitiesRequest();
+        var result = await bidi.NewAsync(capabilities);
+
+        Assert.That(result, Is.Not.Null);
+        Assert.That(result.SessionId, Is.Not.Empty);
+        Assert.That(result.Capabilities, Is.Not.Null);
+        Assert.That(result.Capabilities.BrowserName, Is.Not.Empty);
+    }
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the new NewAsync method, a key part of the PR, is not covered by tests and proposes adding one, which is a significant improvement for code quality and maintainability.

Medium
Learned
best practice
Add null argument validation

Guard against null inputs in this new public API by throwing
ArgumentNullException before delegating, to fail fast and avoid propagating
invalid values into the command pipeline.

dotnet/src/webdriver/BiDi/BiDi.cs [77-80]

 public Task<Session.NewResult> NewAsync(Session.CapabilitiesRequest capabilities, Session.NewOptions? options = null)
 {
+    ArgumentNullException.ThrowIfNull(capabilities);
     return SessionModule.NewAsync(capabilities, options);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation/null guards at integration boundaries (public APIs) before using inputs.

Low
  • Update

@nvborisenko nvborisenko merged commit abb8a7a into SeleniumHQ:trunk Jan 16, 2026
11 checks passed
@nvborisenko nvborisenko deleted the bidi-new-session branch January 16, 2026 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants