[rb] generate the BiDi protocol layer from the shared binding-neutral schema#17731
Open
titusfortner wants to merge 14 commits into
Open
[rb] generate the BiDi protocol layer from the shared binding-neutral schema#17731titusfortner wants to merge 14 commits into
titusfortner wants to merge 14 commits into
Conversation
selenium-ci
added
C-rb
Member
|
Thank you, @titusfortner for this code suggestion. The support packages contain example code that many users find helpful, but they do not necessarily represent After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
PR Summary by Qodo[rb] Generate Ruby BiDi protocol layer from shared schema
AI Description
Diagram
High-Level Assessment
Files changed (44)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
19 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…dation into build
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an auto-generated, binding-neutral Ruby BiDi protocol layer (plus runtime support) based on the shared BiDi schema so future Ruby BiDi work can build on a consistent, spec-aligned foundation without re-deriving types per domain.
Changes:
- Introduces a Bazel-driven Ruby generator + templates to emit Ruby protocol modules and matching RBS from the shared BiDi schema.
- Adds a small BiDi serialization runtime (
Data,Union,UNSET, outbound enum validation) and aTransportseam to send commands + parse typed results. - Adds unit specs validating round-trip JSON behavior, union dispatch, enum validation, and transport error handling; updates RuboCop/Steep config to accommodate generated/additive code.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rb/Steepfile | Narrows Steep ignores to only the remaining hand-written BiDi files while generated code lands. |
| rb/spec/unit/selenium/webdriver/bidi/transport_spec.rb | Adds unit coverage for Transport serialization/result parsing/error raising. |
| rb/spec/unit/selenium/webdriver/bidi/support/bidi_generate_spec.rb | Adds unit coverage for generator naming helpers (camel_to_snake, enum_key). |
| rb/spec/unit/selenium/webdriver/bidi/protocol_types_spec.rb | Adds unit coverage for generated types: (de)serialization, unions, enums, extensible records, and command/event wiring. |
| rb/sig/lib/selenium/webdriver/bidi/transport.rbs | Adds RBS for the new BiDi::Transport API surface. |
| rb/sig/lib/selenium/webdriver/bidi/serialization.rbs | Adds RBS for the BiDi serialization runtime (UNSET, Data, Union, validation). |
| rb/sig/lib/selenium/webdriver/bidi/protocol/web_extension.rbs | Generated RBS for the webExtension domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/user_agent_client_hints.rbs | Generated RBS for the userAgentClientHints domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/storage.rbs | Generated RBS for the storage domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/speculation.rbs | Generated RBS for the speculation domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/session.rbs | Generated RBS for the session domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/permissions.rbs | Generated RBS for the permissions domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/network.rbs | Generated RBS for the network domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/log.rbs | Generated RBS for the log domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/input.rbs | Generated RBS for the input domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/emulation.rbs | Generated RBS for the emulation domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/browsing_context.rbs | Generated RBS for the browsingContext domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/browser.rbs | Generated RBS for the browser domain protocol surface. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/bluetooth.rbs | Generated RBS for the bluetooth domain protocol surface. |
| rb/lib/selenium/webdriver/BUILD.bazel | Adds a rb_binary target to run the generator via bazel run //rb/lib/selenium/webdriver:bidi-generate. |
| rb/lib/selenium/webdriver/bidi/transport.rb | Adds BiDi::Transport for serializing params, sending commands, and parsing typed results. |
| rb/lib/selenium/webdriver/bidi/support/templates/module.rbs.erb | Adds the generator template for emitting per-domain RBS. |
| rb/lib/selenium/webdriver/bidi/support/templates/module.rb.erb | Adds the generator template for emitting per-domain Ruby protocol code. |
| rb/lib/selenium/webdriver/bidi/serialization/union.rb | Adds Serialization::Union runtime for discriminator/presence-based union dispatch (+ forward-compatible fallback behavior). |
| rb/lib/selenium/webdriver/bidi/serialization/data.rb | Adds Serialization::Data runtime for immutable records with wire-name mapping and JSON round-tripping. |
| rb/lib/selenium/webdriver/bidi/serialization.rb | Adds UNSET sentinel + outbound enum validation and wires in the Data/Union runtime. |
| rb/lib/selenium/webdriver/bidi/protocol/web_extension.rb | Generated Ruby protocol code for the webExtension domain. |
| rb/lib/selenium/webdriver/bidi/protocol/user_agent_client_hints.rb | Generated Ruby protocol code for the userAgentClientHints domain. |
| rb/lib/selenium/webdriver/bidi/protocol/storage.rb | Generated Ruby protocol code for the storage domain. |
| rb/lib/selenium/webdriver/bidi/protocol/speculation.rb | Generated Ruby protocol code for the speculation domain. |
| rb/lib/selenium/webdriver/bidi/protocol/session.rb | Generated Ruby protocol code for the session domain. |
| rb/lib/selenium/webdriver/bidi/protocol/permissions.rb | Generated Ruby protocol code for the permissions domain. |
| rb/lib/selenium/webdriver/bidi/protocol/log.rb | Generated Ruby protocol code for the log domain. |
| rb/lib/selenium/webdriver/bidi/protocol/input.rb | Generated Ruby protocol code for the input domain. |
| rb/lib/selenium/webdriver/bidi/protocol/emulation.rb | Generated Ruby protocol code for the emulation domain. |
| rb/lib/selenium/webdriver/bidi/protocol/browsing_context.rb | Generated Ruby protocol code for the browsingContext domain. |
| rb/lib/selenium/webdriver/bidi/protocol/browser.rb | Generated Ruby protocol code for the browser domain. |
| rb/lib/selenium/webdriver/bidi/protocol/bluetooth.rb | Generated Ruby protocol code for the bluetooth domain. |
| rb/lib/selenium/webdriver/bidi/protocol.rb | Adds the domain require list to load the generated protocol surface. |
| rb/.rubocop.yml | Excludes generated BiDi protocol code (and generator) from select complexity/naming cops to keep CI signal focused. |
Contributor
|
Code review by qodo was updated up to the latest commit 45c6cd3 |
Contributor
|
Code review by qodo was updated up to the latest commit c155ac0 |
Contributor
|
Code review by qodo was updated up to the latest commit 64e9f1a |
Contributor
|
Code review by qodo was updated up to the latest commit 2fdaa9c |
Contributor
|
Code review by qodo was updated up to the latest commit d5372e8 |
Contributor
|
Code review by qodo was updated up to the latest commit 88ef296 |
Contributor
|
Code review by qodo was updated up to the latest commit 2adb4f7 |
aguspe
reviewed
Jul 1, 2026
| # under the License. | ||
|
|
||
| # This file is automatically generated. DO NOT EDIT! | ||
| # Regenerate with: bazel run //rb/lib/selenium/webdriver:bidi-generate |
Contributor
There was a problem hiding this comment.
Can we add a check on PRs in case someone edits a file they shouldn't to trigger and validates that the files are not edited?
Member
Author
There was a problem hiding this comment.
Good call. In the process of working on this, found a few other things to improve, and saw that we still need: #17743
Part of me thinks we shouldn't check this in at all (like Python is doing), but it's not much code and I think it'll be easier to see changes and work with while we build up the higher level APIs.
| socket_url = @capabilities[:web_socket_url] | ||
| @bidi = Selenium::WebDriver::BiDi.new(url: socket_url) | ||
| # Share the BiDi object's socket until the bridge owns the connection directly. | ||
| @transport = BiDi::Transport.new(@bidi.ws) |
Contributor
There was a problem hiding this comment.
1. Bidi load-order failure 🐞 Bug ☼ Reliability
Remote::BiDiBridge#create_session constructs BiDi::Transport from @bidi.ws, but bidi/transport.rb can define Selenium::WebDriver::BiDi without initialize(url:) or #ws when this bridge file is loaded without selenium/webdriver (which sets the BiDi autoload). In that require order, Selenium::WebDriver::BiDi.new(url: ...) raises and the session cannot be created.
Agent Prompt
### Issue description
`rb/lib/selenium/webdriver/remote/bidi_bridge.rb` now requires `selenium/webdriver/bidi/transport` and then calls `Selenium::WebDriver::BiDi.new(url: ...)` and `@bidi.ws`. If this file is required without first loading `selenium/webdriver` (which installs `autoload :BiDi, 'selenium/webdriver/bidi'`), then `bidi/transport.rb` may define `Selenium::WebDriver::BiDi` as an empty class (only adding `Transport`), so `BiDi.new(url: ...)` and/or `#ws` will be missing.
### Issue Context
The bridge should be robust to load order, especially when internal files are required directly (e.g., by tooling/tests or alternative boot paths).
### Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[20-34]
### Suggested fix
Ensure `bidi.rb` is loaded before instantiating `Selenium::WebDriver::BiDi` / accessing `#ws`:
- Add `require 'selenium/webdriver/bidi'` (or `require 'selenium/webdriver'`) in `remote/bidi_bridge.rb` before using `Selenium::WebDriver::BiDi`.
Alternative (broader) fix:
- Make `rb/lib/selenium/webdriver/bidi/transport.rb` require `selenium/webdriver/bidi` so `BiDi` is fully defined regardless of load order.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 1afbccd |
…tor, run it in CI
Comment on lines
+39
to
+41
| warn "Generated BiDi protocol code is stale or hand-edited: #{stale.map { |m| "#{m.filename}.rb" }.sort.join(', ')}" | ||
| warn 'Regenerate with: bazel run //rb/lib/selenium/webdriver:bidi-generate' | ||
| exit 1 |
Contributor
There was a problem hiding this comment.
1. check_generated.rb uses warn 📘 Rule violation ◔ Observability
The new generator-verification script writes diagnostic output via warn (direct stderr) instead of WebDriver.logger, which violates the project logging standard and makes log routing/formatting inconsistent in CI.
Agent Prompt
## Issue description
`rb/lib/selenium/webdriver/bidi/support/check_generated.rb` emits messages via `warn`, which is direct stderr output. Per logging policy, new diagnostic output should go through `WebDriver.logger`.
## Issue Context
This script runs under Bazel as `rb_test(verify-bidi-generated)` and is intended to report stale/hand-edited generated protocol files. The messages should use the standard logger so output is consistent with other Ruby components.
## Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/support/check_generated.rb[39-41]
(If you choose `WebDriver.logger.warn`, ensure you pass a non-empty `id=` keyword argument per the warn-id requirement.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+48
to
+52
| name = "verify-bidi-generated", | ||
| size = "small", | ||
| srcs = ["bidi/support/check_generated.rb"], | ||
| args = ["$(location //javascript/selenium-webdriver:create-bidi-src_schema)"], | ||
| data = [ |
Contributor
There was a problem hiding this comment.
2. Schema path not runfiles-safe 🐞 Bug ☼ Reliability
verify-bidi-generated passes the BiDi schema via $(location ...), but check_generated.rb treats the argument as a directly readable runtime path; Bazel does not guarantee execpaths are present/valid at test runtime, so File.read(schema_path) can fail and break Ruby CI now that it runs bazel test //rb/.... Use a runfiles path ($(rootpath ...)/runfiles resolver) to make the test reliably locate the generated schema JSON.
Agent Prompt
### Issue description
`verify-bidi-generated` passes the generated schema JSON path using `$(location ...)`, but the Ruby verifier reads that argument as a normal filesystem path. In Bazel tests, execpaths (often under `bazel-out/...`) are not guaranteed to be directly readable at runtime; tests should prefer runfiles paths.
### Issue Context
- The schema artifact `//javascript/selenium-webdriver:create-bidi-src_schema` is produced as a generated output via `js_run_binary`.
- The verifier `check_generated.rb` reads the schema using `File.read(schema_path)` after a minimal `Dir.pwd`-based fallback.
- Ruby CI now runs `bazel test ... //rb/...`, so the new verify test will run in CI.
### Fix Focus Areas
- rb/lib/selenium/webdriver/BUILD.bazel[47-61]
- rb/lib/selenium/webdriver/bidi/support/check_generated.rb[27-31]
### Suggested fix
1. In `rb/lib/selenium/webdriver/BUILD.bazel`, change the verifier args to use a runfiles path:
- Replace `$(location //javascript/selenium-webdriver:create-bidi-src_schema)` with `$(rootpath //javascript/selenium-webdriver:create-bidi-src_schema)` (or the appropriate runfiles macro supported by your Bazel config).
2. In `check_generated.rb`, make schema path resolution explicitly runfiles-aware (optional but more robust), e.g. try:
- `File.join(ENV['TEST_SRCDIR'], ENV['TEST_WORKSPACE'], schema_path)` when `File.exist?(schema_path)` is false.
3. Ensure the schema target remains in `data` (it already is) so it is available in runfiles.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit dfe3025 |
Comment on lines
+97
to
+105
| def validate_values(attributes) | ||
| fields.each do |f| | ||
| value = attributes[f.name] | ||
| raise ::ArgumentError, "#{name}##{f.name} cannot be nil" if value.nil? && !f.nullable | ||
| next unless f.enum | ||
|
|
||
| Serialization.validate!("#{name}##{f.name}", value, Protocol.const_get(f.enum)) | ||
| end | ||
| end |
Comment on lines
+49
to
+55
| it 'removes an user context' do | ||
| to_remove = browser.create_user_context.user_context | ||
| browser.remove_user_context(user_context: to_remove) | ||
| remaining = browser.get_user_contexts.user_contexts.map(&:user_context) | ||
|
|
||
| expect(remaining).not_to include(to_remove) | ||
| end |
Comment on lines
+32
to
+34
| it 'creates an user context', | ||
| pending_if: {browser: %i[safari safari_preview], | ||
| reason: 'Safari does not support BiDi user contexts or getClientWindows'} do |
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.
🔗 Related Issues
💥 What does this PR do?
Generates the full typed Ruby BiDi protocol layer from the shared schema (all 14 domains, 77 commands, 27 events), plus the hand-written seam (
Protocol::Domainsuperclass + a thinTransport) that makes it usable. With a BiDi-enabled driver, users can now reach the entire BiDi spec directly:Nothing in Selenium consumes it yet — current
driveroperations still run the existing hand-written BiDi paths. To show the generated surface works, the hand-written integration specs have been re-implemented against this generated code for a side-by-side comparison. The generated API mirrors the spec's command names and shapes, exchanging typed objects instead of raw hashes:🔧 Implementation Notes
Generated code is updated with
bazel run //rb/lib/selenium/webdriver:bidi-generate, which consumes the shared schema produced by//javascript/selenium-webdriver:create-bidi-src_schema(#17700). The generator is a straight schema→code projection — the CDDL interpretation and its fidelity check live upstream in that projector, not here.bidi/supportbidi/protocoland subclasses the hand-writtenProtocol::Domainbidi/serializationRecord: base class for every generated value type, an immutableRecord.definevalue object (a::Datasubclass) that knows its field-to-wire-name mapping and round-trips to/from JSON, distinguishing omitted from explicit null via anUNSETsentinel. Enum-valued fields are idiomatic snake_case symbols (:before_request_sent), mapped to/from their wire tokens ('beforeRequestSent') on serialize/parse — symbol-in, symbol-out.Union: base class for a "one of N records" type, that selects the variant from the schema's selector; callers only ever hold the resolvedRecord.Protocol::Domain.newaccepts aDriver(reusing the session's transport) or aTransportdirectly; a driver without BiDi enabled raises the standard "enableweb_socket_url" error.Transportis socket-only and stays a thin pipe: send/receive over the connection plus generic error handling.@api privatebrowser.getUserContexts→get_user_contexts,sameSite→same_site), so the layer reads as the spec does and stays comparable across bindings. Ruby-idiomatic naming (e.g. droppingget_) is deliberately left to the hand-written public wrappers that will consume this layer, not baked into the generator.Validation. Two axes: we own what we send (outbound), the browser owns what it sends (inbound); and for inbound, strict on values but lenient on keys.
ArgumentError, not a round-trip.🤖 AI assistance
Domainsuperclass +Transport, thegenerated protocol modules + RBS, and the unit + integration specs
💡 Additional Considerations
Remote::BiDiBridge,WebDriver::Script, andBiDi::Networkagainst the generated layer instead of the current BiDi classes.🔄 Types of changes