[rb] construct the BiDi transport inside the domain from a connection

[titusfortner]

🔗 Related Issues

💥 What does this PR do?

Moves the BiDi Transport into the protocol Domain classes instead of having the bridge build one and hand it down.

• The Transport exists only to serve the generated domain classes — it's the seam that serializes params, sends them over the socket, and parses the typed reply.
• Anything else the bridge needs goes through those domain classes or the WebSocket connection directly, so the bridge has no reason to hold a Transport.
• The bridge now exposes just the raw connection, and each Domain wraps it in its own Transport.

🔧 Implementation Notes

Domain#initialize accepts a Driver or anything satisfying the connection contract (responds to send_cmd) and constructs the Transport itself, replacing the earlier Transport type check. The .rbs signatures loosen to untyped for the connection accordingly.

🤖 AI assistance

• AI assisted (complete below)
  • Tool(s): Claude Code
  • What was generated: the refactor and test updates
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)

[qodo-code-review]

PR Summary by Qodo

Move BiDi transport construction into protocol domains

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Stop building/owning BiDi Transport in the remote bridge; expose only the raw connection.
• Have BiDi protocol Domain classes wrap a connection with a Transport internally.
• Update RBS signatures and unit specs to match the new connection-driven construction.

Diagram

graph TD
  D["Driver"] --> B["Remote BiDiBridge"] --> C("WebSocket connection")
  D --> X["Protocol Domain"]
  X --> T["BiDi::Transport"] --> C

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep Transport owned by the bridge

• ➕ Single shared Transport instance across all domains
• ➕ Clearer place to add connection/transport lifecycle management if needed later
• ➖ Bridge retains a BiDi Protocol concern it doesn’t otherwise need
• ➖ Domains become less self-contained; harder to use domains with non-bridge connections in tests/other runtimes

2. Type a connection interface in RBS (instead of untyped)

• ➕ Preserves type safety while still using duck typing (e.g., an interface with send_cmd)
• ➕ Makes the new contract explicit for downstream type checkers
• ➖ Requires defining/maintaining an interface type in RBS and updating signatures accordingly

3. Accept both Transport and connection for compatibility

• ➕ Reduces risk of breakage for any internal callers passing Transport directly
• ➕ Allows incremental migration while keeping new design
• ➖ Longer deprecation period and more branching logic in Domain#initialize

Recommendation: The PR’s approach is directionally best: Domains are the natural owner of Transport since it only exists to serve generated domain APIs. Consider strengthening the RBS contract by introducing a small connection interface type (send_cmd) instead of widening to untyped, and optionally allowing Transport inputs during a short transition if there are any internal callers outside the unit tests.

Files changed (7) +25 / -20

Refactor (3) +8 / -7

domain.rbConstruct Transport inside Domain from a connection +4/-2

Construct Transport inside Domain from a connection

• Domain initialization now extracts a connection (from Driver.bridge.connection or the provided source) and builds a new Transport. Validation switches from an explicit Transport type check to a send_cmd contract check on the connection.

rb/lib/selenium/webdriver/bidi/protocol/domain.rb

bidi_bridge.rbExpose BiDi connection instead of Transport +3/-4

Expose BiDi connection instead of Transport

• Removes the explicit BiDi::Transport dependency and stops storing a transport on the bridge. The bridge now stores and exposes the raw websocket connection as @connection.

rb/lib/selenium/webdriver/remote/bidi_bridge.rb

bridge.rbRename required BiDi hook from transport to connection +1/-1

Rename required BiDi hook from transport to connection

• The abstract Bridge API changes from #transport to #connection for BiDi-enabled bridges. The default implementation continues to raise unless BiDi is enabled.

rb/lib/selenium/webdriver/remote/bridge.rb

Tests (1) +12 / -10

protocol_spec.rbUpdate protocol unit tests to pass a connection (not Transport) +12/-10

Update protocol unit tests to pass a connection (not Transport)

• Tests now provide a connection double that responds to send_cmd and assert domains construct a Transport internally. Updates failure messaging and all domain constructions accordingly.

rb/spec/unit/selenium/webdriver/bidi/protocol_spec.rb

Other (3) +5 / -3

domain.rbsLoosen Domain initializer signature to accept a connection-like source +1/-1

Loosen Domain initializer signature to accept a connection-like source

• Updates the RBS signature for Domain#initialize from (Driver | Transport) to untyped, reflecting the new duck-typed connection contract (send_cmd).

rb/sig/lib/selenium/webdriver/bidi/protocol/domain.rbs

bidi_bridge.rbsUpdate BiDiBridge RBS to expose connection +3/-1

Update BiDiBridge RBS to expose connection

• Adds @connection and an attr_reader for connection (untyped) and removes the transport reader typing. Aligns the type surface with the runtime bridge changes.

rb/sig/lib/selenium/webdriver/remote/bidi_bridge.rbs

bridge.rbsRename Bridge RBS method from transport to connection +1/-1

Rename Bridge RBS method from transport to connection

• Updates the Bridge interface in RBS to define #connection returning untyped instead of #transport returning BiDi::Transport.

rb/sig/lib/selenium/webdriver/remote/bridge.rbs

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 19 rules

1. RSpec mocks used for connection 📘 Rule violation ▣ Testability

Description

The updated unit spec uses RSpec mocking (instance_double and allow(...).to receive) for the
connection object. This violates the no-mocks testing requirement unless replaced with a real
integration or a contract-driven fake.

Code

rb/spec/unit/selenium/webdriver/bidi/protocol_spec.rb[R29-33]

          let(:connection) { instance_double(WebDriver::WebSocketConnection) }
-          let(:transport) { Transport.new(connection) }
+
+          # The double stands in for a real connection, which responds to send_cmd.
+          before { allow(connection).to receive(:send_cmd) }

Evidence

PR Compliance ID 389270 disallows use of mocking frameworks in touched tests unless the mock is
backed by a machine-checked contract. The spec constructs an RSpec instance_double and stubs
send_cmd via allow(...).to receive, which is mocking-framework usage.

Rule 389270: Avoid mocks in tests; use real or contract-driven integrations
rb/spec/unit/selenium/webdriver/bidi/protocol_spec.rb[29-33]

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 mocking framework is used in a touched test (`instance_double` + `allow(...).to receive(:send_cmd)`), which violates the rule to avoid mocks in tests unless they are contract-driven.

## Issue Context
The test only needs a minimal object that responds to `send_cmd`. This can be satisfied by introducing a small in-memory fake connection class (or struct) implementing `send_cmd`, avoiding RSpec mocks.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/bidi/protocol_spec.rb[29-33]

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

---

2. Bridge#connection lacks @api private 📘 Rule violation ✧ Quality

Description

The modified internal method connection in Remote::Bridge has no YARD doc block marking it as
private API. This violates the requirement to mark internal Ruby APIs with # @api private, risking
accidental public use and undocumented API surface.

Code

rb/lib/selenium/webdriver/remote/bridge.rb[R601-604]

+        def connection
          msg = 'BiDi must be enabled by setting #web_socket_url to true in options class'
          raise(WebDriver::Error::WebDriverError, msg)
        end

Evidence

PR Compliance ID 389237 requires internal Ruby APIs to be marked with # @api private in the YARD
doc block directly above the definition. The changed method def connection has no such doc
block/tag above it.

Rule 389237: Mark internal Ruby APIs with @api private in YARD docs
rb/lib/selenium/webdriver/remote/bridge.rb[601-604]

Agent prompt

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

## Issue description
`Selenium::WebDriver::Remote::Bridge#connection` was modified but is missing a YARD doc block containing `# @api private`, as required for internal APIs.

## Issue Context
This project uses YARD and already marks internal APIs with `@api private`. The `connection` method appears to be part of the internal bridge interface (it raises an error by default) and should be explicitly marked as private API.

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bridge.rb[601-604]

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

---

3. Loose connection contract check ✗ Dismissed 🐞 Bug ≡ Correctness

Description

Protocol::Domain#initialize accepts any object that respond_to?(:send_cmd), which can include
Selenium::WebDriver::BiDi (e.g., BrowsingContext.new(driver.bidi)) even though it does not match
the connection API/return-shape required by BiDi::Transport. This can compile and pass
construction but then fail at runtime when Transport calls send_cmd(method:, params:) and
expects a full reply hash with error/result keys.

Code

rb/lib/selenium/webdriver/bidi/protocol/domain.rb[R27-30]

+            connection = source.is_a?(Driver) ? source.send(:bridge).connection : source
+            raise(Error::WebDriverError, 'a Driver or connection is required') unless connection.respond_to?(:send_cmd)
+
+            @transport = Transport.new(connection)

Evidence

The constructor now only checks respond_to?(:send_cmd) and then instantiates Transport with that
object. Transport requires a connection whose send_cmd accepts keyword args (method:,
params:) and returns a wire reply hash, but BiDi#send_cmd takes a positional method and
returns only the result payload, so it can satisfy respond_to? yet be incompatible at runtime.

rb/lib/selenium/webdriver/bidi/protocol/domain.rb[26-31]
rb/lib/selenium/webdriver/bidi/transport.rb[27-38]
rb/lib/selenium/webdriver/bidi.rb[61-67]

Agent prompt

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

## Issue description
`Protocol::Domain#initialize` now accepts any `source` that responds to `send_cmd`, then wraps it with `BiDi::Transport`. This is too permissive: `Selenium::WebDriver::BiDi` (returned by `driver.bidi`) also responds to `send_cmd` but has an incompatible signature/return shape for `Transport`, leading to runtime errors when executing commands.

## Issue Context
`BiDi::Transport#execute` calls `@connection.send_cmd(method: ..., params: ...)` and expects a reply hash containing `"error"`/`"result"`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/protocol/domain.rb[26-31]

## Suggested fix
Update `Domain#initialize` to validate the *actual* connection contract expected by `Transport`, not just method existence. Options:
1) Special-case a `BiDi` instance (or any object responding to `ws`) by using `source.ws` as the connection if it responds to `send_cmd`.
2) Strengthen the duck-type check to ensure `send_cmd` can be called with keyword args (`method:` and `params:`) and that the returned value is the expected reply shape (at least a Hash with `"result"`/`"error"` keys). If not compatible, raise the constructor error early.

This prevents `BrowsingContext.new(driver.bidi)` and similar call patterns from being accepted but failing later during command execution.

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

---

[Copilot]

Pull request overview

This PR refactors the Ruby BiDi generated protocol layer so each protocol Domain constructs its own Transport from a raw connection (instead of the remote bridge creating a Transport and passing it down), and updates bridge APIs/signatures accordingly.

Changes:

• Move BiDi::Transport construction into BiDi::Protocol::Domain#initialize, accepting a “connection-like” object.
• Expose connection (raw WebSocket connection) from the remote bridge instead of transport.
• Update unit specs and RBS signatures to reflect the new construction flow.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
rb/spec/unit/selenium/webdriver/bidi/protocol_spec.rb	Updates protocol-layer unit tests to pass a connection and assert Transport is built internally.
rb/sig/lib/selenium/webdriver/remote/bridge.rbs	Replaces transport with connection in the bridge’s typed surface.
rb/sig/lib/selenium/webdriver/remote/bidi_bridge.rbs	Updates typed ivars/accessors to expose connection instead of transport.
rb/sig/lib/selenium/webdriver/bidi/protocol/domain.rbs	Loosens Domain#initialize type to accept an untyped source.
rb/lib/selenium/webdriver/remote/bridge.rb	Renames the base bridge API from transport to connection (raising when BiDi isn’t enabled).
rb/lib/selenium/webdriver/remote/bidi_bridge.rb	Stores/exposes the raw websocket as @connection instead of constructing/storing a Transport.
rb/lib/selenium/webdriver/bidi/protocol/domain.rb	Constructs a Transport over a connection derived from a Driver bridge or passed directly.