[rb] add low-level BiDi protocol integration specs by titusfortner · Pull Request #17878 · SeleniumHQ/selenium

titusfortner · 2026-08-05T23:18:51Z

🔗 Related Issues

💥 What does this PR do?

Adds low-level BiDi Protocol integration specs covering every command across all domains, exercised happy-path against real browsers.
Wire-deserialization mismatches now raise a typed SerializationError instead of a bare WebDriverError.

🔧 Implementation Notes

Guards assert the specific error the remote returns, so tests notify when behavior changes rather than masking it:
- UnknownCommandError — command or whole module the browser doesn't recognize
- UnsupportedOperationError — recognized but not implemented / not permitted
- SerializationError — malformed or incomplete response (surfaced by strict mode)
Runs the suite in strict serialization mode, so a browser response missing a required field fails loudly instead of being silently tolerated as an omitted value.
Per-browser gaps are recorded as pending guards that flip to a failure the moment a browser starts supporting the command — a built-in "support landed" signal.
Timeout-prone commands are set to skip per browser+OS (not pending), so they don't burn the full timeout every run.
Support status verified against each browser's current BiDi implementation (Chrome/Firefox betas, stable Edge). Edge tracks Chromium on an older mapper, so it diverges on a few commands. Safari is immature and guarded separately.

Cells show the error class the guard asserts (linked where a bug is filed); n/a = works. * describes Windows behavior where different (TimeoutError* = works elsewhere but times out on Windows and is skipped there).

BiDi command	Chrome	Edge	Firefox
`emulation.setForcedColorsModeThemeOverride`	`UnsupportedOperationError`	`UnsupportedOperationError`	`UnknownCommandError`
`emulation.setGeolocationOverride` (`error:`)	n/a	n/a	`InvalidArgumentError`
`emulation.setScriptingEnabled`	n/a	n/a	`UnknownCommandError`
`emulation.setScrollbarTypeOverride`	n/a	`UnknownCommandError`	`UnknownCommandError`
`emulation.setTouchOverride`	n/a	n/a	`UnknownCommandError`
`browsingContext.setBypassCSP`	`UnsupportedOperationError`	`UnknownCommandError`	`UnknownCommandError`
`browsingContext.startScreencast`	`UnsupportedOperationError`	`UnknownCommandError`	n/a
`browsingContext.reload` (`ignoreCache: true`)	n/a	n/a	`UnsupportedOperationError`
`input.setFiles`	n/a	n/a	`UnsupportedOperationError`
`webExtension.install` (archivePath / base64)	`UnsupportedOperationError`	`UnsupportedOperationError`	n/a
`userAgentClientHints.setClientHintsOverride`	n/a	n/a	`UnknownCommandError`
`bluetooth.*` (entire module)	n/a	n/a	`UnknownCommandError`
`bluetooth.*` (device-response commands)	`TimeoutError*`	`TimeoutError*`	`UnknownCommandError`
`browser.setDownloadBehavior` (file download)	`TimeoutError*`	`TimeoutError*`	`TimeoutError*`
`browser.close`	n/a	n/a	`UnsupportedOperationError`
`session.end`	n/a	n/a	`UnsupportedOperationError`

🤖 AI assistance

AI assisted (complete below)
- Tool(s): Claude Code
- What was generated: protocol specs scaffolded from the BiDi schema; browser support status and error classes verified against upstream source and CI runs
- I reviewed all AI output and can explain the change

💡 Additional Considerations

Safari's BiDi is immature — under strict mode most of its commands return malformed data (SerializationError) or are unimplemented; those are guarded pending so they signal when Safari catches up.
Add test depth:
- verify the reset/clear branch actually took effect (re-read state instead of asserting an empty result);
- add browser-side error responses (invalid or closed context, denied permission);
- broaden alternate-value coverage (multiple enum variants, both boolean branches).

🔄 Types of changes

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

…imeouts

qodo-code-review · 2026-08-05T23:21:26Z

PR Summary by Qodo

Add Ruby BiDi protocol integration specs with strict serialization errors

🧪 Tests ✨ Enhancement ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Add BiDi Protocol integration specs covering commands across multiple domains.
• Run the BiDi protocol suite in strict wire-deserialization mode via Bazel.
• Raise typed SerializationError for schema mismatches instead of generic WebDriverError.

Diagram

graph TD
  specs["BiDi protocol integration specs"] --> harness["Bazel rb_integration_test (SE_BIDI_STRICT=true)"] --> browsers["Real browsers (Chrome/Edge/Firefox/Safari)"]
  specs --> protocol["BiDi Protocol Ruby client"] --> ser["Serialization (Record/Union)"] --> serr["Error::SerializationError"]
  browsers --> ser

High-Level Assessment

The following are alternative approaches to this PR:

1. Mocked wire-payload contract tests (no browsers)

➕ Much faster and more deterministic than real-browser integration runs
➕ Easier to cover edge cases and rare malformed payloads
➖ Does not validate actual browser/driver behavior or error codes
➖ Higher risk of drifting from real implementations and missing regressions

2. Keep strict mode off; only assert happy-path typing

➕ Fewer CI failures when browsers lag or return incomplete payloads
➕ Lower maintenance burden for per-browser guards
➖ Schema mismatches can be silently tolerated, masking breakages
➖ Weaker signal when browser behavior changes unexpectedly

3. Reuse WebDriverError for deserialization mismatches

➕ No new public error type to document/handle
➕ Less surface-area for downstream exception handling
➖ Conflates local schema/serialization failures with remote protocol errors
➖ Harder for callers and specs to assert the correct failure mode

Recommendation: The current approach (real-browser integration specs + strict serialization mode + typed SerializationError) is the best fit for catching BiDi drift early. It provides a clear separation between remote protocol errors and local schema mismatches, while pending/skip guards keep the suite actionable across uneven browser support.

Files changed (20) +2784 / -43

Enhancement (3) +13 / -9

error.rbIntroduce Error::SerializationError for BiDi schema mismatches +4/-0

Introduce Error::SerializationError for BiDi schema mismatches
• Adds a dedicated SerializationError subclass to represent local BiDi wire (de)serialization failures distinct from protocol error codes.
rb/lib/selenium/webdriver/bidi/error.rb

record.rbRaise SerializationError for record deserialization violations +7/-7

Raise SerializationError for record deserialization violations
• Switches record deserialization failures (wrong wire shape, missing required fields in strict mode, nullability/type mismatches) from WebDriverError to SerializationError.
rb/lib/selenium/webdriver/bidi/serialization/record.rb

union.rbRaise SerializationError for union variant/shape mismatches +2/-2

Raise SerializationError for union variant/shape mismatches
• Updates union deserialization to raise SerializationError when object-only unions receive scalars or when no schema variant matches the inbound payload.
rb/lib/selenium/webdriver/bidi/serialization/union.rb

Tests (13) +2741 / -30

bluetooth_spec.rbAdd Bluetooth protocol integration specs with browser/OS guards +596/-0

Add Bluetooth protocol integration specs with browser/OS guards
• Adds happy-path Bluetooth command coverage (adapter/device simulation, GATT events, characteristic/descriptor operations) plus per-browser pending/skip guards and cleanup hooks.
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb

browser_spec.rbAdd Browser domain protocol integration specs (contexts/windows/downloads/close) +180/-0

Add Browser domain protocol integration specs (contexts/windows/downloads/close)
• Covers Browser domain commands including user contexts, client window enumeration/state changes, download behavior, and close handling with Safari/Firefox guards.
rb/spec/integration/selenium/webdriver/bidi/protocol/browser_spec.rb

browsing_context_spec.rbAdd BrowsingContext protocol integration specs for core navigation and tooling +361/-0

Add BrowsingContext protocol integration specs for core navigation and tooling
• Adds broad coverage for browsing context operations (create/activate/close/tree, screenshot/print/navigate/reload, locateNodes, viewport, bypassCSP, screencast, history) with cross-browser pending/skip expectations.
rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb

emulation_spec.rbAdd Emulation domain protocol integration specs with support assertions +251/-0

Add Emulation domain protocol integration specs with support assertions
• Exercises emulation commands (geolocation, locale/timezone, network conditions, screen settings/orientation, scripting/touch/user agent) and asserts expected unsupported/unknown behaviors per browser.
rb/spec/integration/selenium/webdriver/bidi/protocol/emulation_spec.rb

input_spec.rbAdd Input domain protocol integration specs (actions, setFiles) +130/-0

Add Input domain protocol integration specs (actions, setFiles)
• Adds tests for pointer actions, releasing actions, and setting file input values via input.setFiles with known Firefox path limitations guarded.
rb/spec/integration/selenium/webdriver/bidi/protocol/input_spec.rb

network_spec.rbAdd Network domain protocol integration specs (intercepts, collectors, headers) +403/-0

Add Network domain protocol integration specs (intercepts, collectors, headers)
• Covers network intercept lifecycle, request/response continuation, auth challenges, failing/providing responses, data collection retrieval/disown, and extra headers with Safari strict-mode guards.
rb/spec/integration/selenium/webdriver/bidi/protocol/network_spec.rb

permissions_spec.rbAdd Permissions domain protocol integration specs (setPermission) +103/-0

Add Permissions domain protocol integration specs (setPermission)
• Validates permission setting (geolocation) and supports embedded origin/user context parameters with Safari guards for strict deserialization gaps.
rb/spec/integration/selenium/webdriver/bidi/protocol/permissions_spec.rb

script_spec.rbAdd Script domain protocol integration specs (evaluate/call/disown/preload/realms) +197/-0

Add Script domain protocol integration specs (evaluate/call/disown/preload/realms)
• Adds coverage for common Script commands and options (preload scripts, callFunction, evaluate with ownership/serialization, disown, getRealms) with Safari strict deserialization guards.
rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb

session_spec.rbAdd Session domain protocol integration specs (status/subscribe/unsubscribe/end) +111/-0

Add Session domain protocol integration specs (status/subscribe/unsubscribe/end)
• Exercises session status and subscription lifecycle plus negative coverage for session.new on established sessions; includes Safari strict-mode and Firefox classic-session limitations.
rb/spec/integration/selenium/webdriver/bidi/protocol/session_spec.rb

storage_spec.rbAdd Storage domain protocol integration specs for cookie operations +153/-0

Add Storage domain protocol integration specs for cookie operations
• Adds tests for setting, reading, and deleting cookies (including partition descriptors) and guards known Safari internal-error behavior.
rb/spec/integration/selenium/webdriver/bidi/protocol/storage_spec.rb

user_agent_client_hints_spec.rbAdd UserAgentClientHints protocol integration specs (override) +107/-0

Add UserAgentClientHints protocol integration specs (override)
• Covers client hints override behavior and user-context filtering, with Firefox/Safari unknown-command and strict-mode guards.
rb/spec/integration/selenium/webdriver/bidi/protocol/user_agent_client_hints_spec.rb

web_extension_spec.rbAdd WebExtension protocol integration specs (install/uninstall) +133/-0

Add WebExtension protocol integration specs (install/uninstall)
• Adds extension install/uninstall tests supporting directory, archive path, and base64 payloads; includes Chromium limitations and Safari unknown-command guards.
rb/spec/integration/selenium/webdriver/bidi/protocol/web_extension_spec.rb

protocol_browsing_context_spec.rbRefocus handleUserPrompt tests to assert payload acceptance without UI prompts +16/-30

Refocus handleUserPrompt tests to assert payload acceptance without UI prompts
• Replaces interactive alert/prompt flows with assertions that BiDi handle_user_prompt requests are accepted and return NoSuchAlertError when no prompt is open.
rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb

Other (4) +30 / -4

ci-ruby.ymlRun Edge BiDi-tagged Ruby CI jobs on Windows +1/-1

Run Edge BiDi-tagged Ruby CI jobs on Windows
• Adjusts the Windows job tag filters to include Edge BiDi-targeted runs alongside existing beta/local browser tags.
.github/workflows/ci-ruby.yml

BUILD.bazelInclude BiDi protocol integration spec targets in Bazel test graph +1/-0

Include BiDi protocol integration spec targets in Bazel test graph
• Registers the new rb/spec/integration/selenium/webdriver/bidi/protocol filegroup so the protocol specs are built and discoverable in Bazel.
rb/spec/BUILD.bazel

BUILD.bazelAdd Bazel targets for BiDi protocol integration specs (strict mode) +24/-0

Add Bazel targets for BiDi protocol integration specs (strict mode)
• Defines Bazel integration test targets for each *_spec.rb in the BiDi protocol directory, enabling BiDi-only execution with SE_BIDI_STRICT=true and required data deps.
rb/spec/integration/selenium/webdriver/bidi/protocol/BUILD.bazel

tests.bzlAllow per-test env overrides and merge into browser env +4/-3

Allow per-test env overrides and merge into browser env

• Extends rb_integration_test to accept an env map and merges it into each generated Bazel test rule, enabling suites (like BiDi protocol specs) to force strict serialization mode.

rb/spec/tests.bzl

qodo-code-review · 2026-08-05T23:24:48Z

Code Review by Qodo

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

1. Unit specs expect WebDriverError ✓ Resolved 📘 Rule violation ≡ Correctness

Description

The serialization layer now raises Error::SerializationError for schema/wire mismatches, but
existing unit specs still assert Error::WebDriverError, so the test suite (and downstream
expectations) will be inconsistent with the new behavior. Update unit tests (and any docs/examples)
to expect SerializationError where applicable.

Code

rb/lib/selenium/webdriver/bidi/serialization/record.rb[R229-232]

            def missing_required(field)
              message = "#{name}##{field.name} is required but was missing from the response"
-              raise Error::WebDriverError, message if Serialization.strict?
+              raise Error::SerializationError, message if Serialization.strict?

Evidence
PR Compliance ID 3 requires updating tests alongside behavior changes. The PR changes strict
deserialization to raise Error::SerializationError (e.g., missing_required), while unit specs
still assert raise_error(Error::WebDriverError, ...), so tests and expectations are no longer
aligned with the new behavior.
AGENTS.md: Write/Update Tests for Fixes and Prefer Small Unit Tests Over Browser Tests
rb/lib/selenium/webdriver/bidi/serialization/record.rb[225-234]
rb/lib/selenium/webdriver/bidi/serialization/union.rb[59-70]
rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[78-88]
rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[644-650]

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 BiDi serialization code now raises `Error::SerializationError`, but unit tests still expect `Error::WebDriverError`, causing failing/incorrect assertions.

## Issue Context
This PR changes strict deserialization failures (and other schema mismatches) from `Error::WebDriverError` to the new typed `Error::SerializationError`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/serialization/record.rb[229-233]
- rb/lib/selenium/webdriver/bidi/serialization/union.rb[59-70]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[78-88]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[603-650]

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

2. ~~Overbroad strict-mode match~~ ⊘ Outdated 🐞 Bug ≡ Correctness

Description

The strict-serialization hook enables SE_BIDI_STRICT for any spec whose file_path contains
"bidi/protocol", which also matches pre-existing specs like bidi/protocol_browsing_context_spec.rb
(not under the new bidi/protocol/ directory). This unintentionally opts those specs into strict
deserialization and can cause unexpected failures/behavior changes despite the comment stating “only
these specs opt in.”

Code

rb/spec/integration/selenium/webdriver/spec_helper.rb[R126-128]
+  c.before do |example|
+    next unless example.metadata[:file_path].to_s.include?('bidi/protocol')
+

Evidence
The hook gates strict mode on file_path.include?('bidi/protocol'), which also matches a
pre-existing file whose path contains the substring but is not in the new bidi/protocol/ directory,
expanding strict mode beyond the stated intent.
rb/spec/integration/selenium/webdriver/spec_helper.rb[123-137]
rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[20-30]

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 strict-serialization RSpec hook uses `include?('bidi/protocol')`, which matches both the intended `.../bidi/protocol/...` directory and unrelated files whose names start with `protocol_` (e.g., `bidi/protocol_browsing_context_spec.rb`). This causes unintended specs to run with `SE_BIDI_STRICT=true`.

## Issue Context
The comment says only the low-level protocol specs should opt into strict mode, but the current substring match is not directory-bound.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_helper.rb[123-137]

## Suggested change
Update the predicate to be directory-specific (e.g., check for `"bidi/protocol/"` with a trailing slash, or use a path-segment-aware matcher like `File.fnmatch?("*/bidi/protocol/*", file_path)`), or add explicit metadata (tag) to the intended specs and gate on that tag instead.

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

3. set_viewport pending lacks exception ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The Safari pending_if guard for the #set_viewport example does not specify an exception:
matcher, which effectively makes the spec pending unconditionally on Safari and can mask real
regressions or unrelated infrastructure failures. This weakens test coverage and runs counter to the
intent of using guards to precisely detect behavior changes.

Code

rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[R321-322]

+               pending_if: {browser_family: :safari,
+                            reason: 'Safari accepts browsingContext.setViewport but does not resize the window'} do

Evidence
PR Compliance ID 4 requires behavior changes to be validated with reliable tests, but in
browsing_context_spec.rb[321-322] the Safari guard is a pending_if without an exception:
specification, meaning any Safari failure is treated as acceptable (pending) rather than being
validated against an expected failure shape. As noted, the guard framework only treats a pending as
exception-validated when an explicit exception spec is present (i.e., Guard#exception? is true),
otherwise pending_if is applied up front, which can hide unrelated errors such as
transport/timeout/unknown-command failures and obscure genuine behavior changes.
AGENTS.md: Write/Update Tests for Code Changes; Prefer Small Unit Tests and Avoid Mocks
rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[320-322]
rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[319-330]
rb/lib/selenium/webdriver/support/guards.rb[51-58]
rb/lib/selenium/webdriver/support/guards/guard.rb[87-100]

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 Safari `pending_if` guard in the `#set_viewport` example is missing an `exception:` matcher, causing the spec to be treated as pending unconditionally on Safari and potentially masking unrelated failures or real regressions. Update the guard to be exception-specific (or use `skip_if` if the behavior is permanently unsupported) so that only the known, expected Safari failure mode is marked pending.

## Issue Context
This spec is intentionally guarded for Safari, but without an expected exception shape it can hide unexpected failures. The guard system only defers/validates pending behavior when `exception:` is provided; otherwise `pending_if` is marked pending immediately, making all Safari failures look like the same known issue and reducing the signal when support changes.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[319-330]

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

4. ~~Overbroad Chromium Bluetooth skips~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Several Bluetooth protocol examples are now skipped for all Chromium-family browsers based only on
browser_family: :chromium, even though the skip reason states the issue is CI-specific. This
permanently removes Chromium coverage (including local runs) and prevents the suite from ever
signaling when Chromium Bluetooth becomes stable again.

Code

rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[R292-293]

+               skip_if: {browser_family: :chromium,
+                         reason: 'navigator.bluetooth unreliable in CI (undefined on Linux, times out on Windows)'} do

Evidence
bluetooth_spec.rb skips are keyed only by browser_family: :chromium, which applies in both CI
and local runs. The guard system supports a :ci condition and other specs already constrain
skips/flaky behavior by ci: :github, showing the intended mechanism exists.
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[290-301]
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[343-347]
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[432-436]
rb/spec/integration/selenium/webdriver/spec_helper.rb[50-61]
rb/spec/integration/selenium/webdriver/window_spec.rb[116-120]

Agent prompt

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

### Issue description
Bluetooth protocol examples are skipped for all Chromium-family browsers using only `browser_family: :chromium`, but the stated reason is CI-specific flakiness. This suppresses the tests even in non-CI runs and reduces the suite’s ability to detect improvements/regressions.

### Issue Context
Guard conditions support CI and platform qualifiers. Use those qualifiers to align the predicate with the stated reason (CI/Linux+Windows) so local runs (and/or non-affected OSes) still exercise the Bluetooth coverage.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[291-293]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[299-301]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[343-346]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[363-366]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[375-377]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[433-435]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[518-520]

### Suggested change
Update each `skip_if` to include `ci: :github` (and optionally `platform: %i[linux windows]` if that’s the actual scope), or adjust the reason text if the skip truly applies to all Chromium environments.

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

View review recommended (5)

5. ~~Callback leak on subscribe~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

Bluetooth protocol specs register a BiDi callback before calling session.subscribe, but if
session.subscribe raises, the callback can remain registered because cleanup in the caller’s
ensure depends on the (never-assigned) callback value. This can leave stale callbacks active for
the remainder of the driver lifetime, potentially interfering with later event handling in the same
process.

Code

rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[R167-170]

+          def select_device
+            enable_adapter
+            events, callback = subscribe('bluetooth.requestDevicePromptUpdated')
+            start_request_device

Evidence
subscribe registers the callback before subscribing and does not remove it on error; the caller
only unsubscribes/removes the callback when the returned callback variable is truthy, which won’t
happen if subscribe raises before returning.
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[106-111]
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[167-190]

Agent prompt

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

### Issue description
`subscribe(event)` adds a callback before attempting `session.subscribe`. If `session.subscribe` raises, the callback can remain registered because the caller’s `ensure` only unsubscribes/removes the callback when `callback` is truthy, but the multiple assignment never completes.

### Issue Context
This is in the new BiDi Bluetooth protocol integration specs.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[106-111]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[167-190]

### Suggested fix
Wrap `session.subscribe` in `subscribe(event)` with `rescue/ensure` so that if subscribing fails, you immediately remove the callback you just registered (and then re-raise). Keep the ordering (add callback first) to avoid missing events, but ensure cleanup on failure.

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

6. ~~Unqualified Safari pending guard~~ ✗ Dismissed 🐞 Bug ☼ Reliability

Description

In Script#call_function specs, the new Safari pending_if guards omit an exception: matcher, so
any Safari failure in those examples is treated as expected and won’t signal behavior changes. This
bypasses the guard framework’s deferred exception validation path and can hide real regressions or
infrastructure errors under a pending status.

Code

rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb[R85-87]

+            it 'calls a function with local value arguments',
+               pending_if: {browser_family: :safari,
+                            reason: 'Safari remote value fails deserialization'} do

Evidence
The new specs add Safari pending_if guards without exception:, and the guard framework marks
such guards pending up-front (without validating the failure) because Guard#exception? is false
when :exception is missing. Only exception-qualified pending guards are deferred and validated via
pending_exception_guard/resolve_pending_exception.
rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb[84-115]
rb/lib/selenium/webdriver/support/guards.rb[51-65]
rb/lib/selenium/webdriver/support/guards/guard.rb[87-100]
rb/spec/integration/selenium/webdriver/spec_helper.rb[64-83]

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 two new Safari `pending_if` guards in `Script#call_function` are unconditional (no `exception:`), so any failure is marked pending instead of validating that Safari fails in the expected way.

### Issue Context
The integration spec harness supports *exception-qualified* pending guards that only apply when the observed failure matches the expected class/message; otherwise the example is treated as a real failure.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb[85-115]

### Suggested fix
- Update both `pending_if` entries to include `exception: { class: ..., message: ... }` for the known Safari failure mode.
 - For the “fails deserialization” case, prefer matching `Error::SerializationError` (and add a message regex once you’ve captured the actual error text from Safari).
 - For the “unexpected result” case, either:
   - make Safari return a known protocol error and guard on that exception, or
   - (less ideal) guard on `RSpec::Expectations::ExpectationNotMetError` with a targeted message regex so only the known mismatch is treated as pending.

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

7. ~~Probe uninstalls nonexistent extension~~ ✗ Dismissed 🐞 Bug ≡ Correctness

Description

The Safari web extensions support probe calls webExtension.uninstall with a hard-coded id that is
never installed, so once Safari implements the module the probe will likely still fail with a “no
such extension” style error instead of going green.
This prevents the probe from reliably detecting that WebExtension support landed.

Code

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[R89-90]
+            expect(WebExtension.new(driver).uninstall(extension: 'ruby-bidi-probe')).to be_empty
+          end

Evidence
Existing integration coverage shows uninstall is performed against an installed extension id, and
the protocol implementation has no built-in mechanism to resolve or create the extension id being
uninstalled.
rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[86-90]
rb/spec/integration/selenium/webdriver/bidi/protocol/web_extension_spec.rb[81-89]
rb/spec/integration/selenium/webdriver/bidi/protocol/web_extension_spec.rb[117-126]
rb/lib/selenium/webdriver/bidi/protocol/web_extension.rb[104-110]

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 Safari WebExtension support probe uninstalls a hard-coded extension id (`'ruby-bidi-probe'`) without ever installing it. If/when Safari implements `webExtension.uninstall`, a compliant implementation will likely error for an unknown extension id, so the probe will not flip to “pending fixed” when support lands.

### Issue Context
- The real integration tests install an extension and then uninstall using the returned `result.extension` id.
- `WebExtension#uninstall` is a direct protocol call that accepts only the provided `extension` string.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[86-90]

### Suggested change
Update the probe to install a known test extension from `//common/extensions` first, capture the returned id, then uninstall it:
- Build the extension path similarly to `web_extension_spec.rb` (e.g., `File.expand_path("../../../../../../../common/extensions/webextensions-selenium-example-signed", __dir__)`).
- `id = WebExtension.new(driver).install(extension_data: WebExtension::ExtensionPath.new(path: path)).extension`
- `expect(WebExtension.new(driver).uninstall(extension: id)).to be_empty`
- Ensure cleanup in an `ensure` block if needed.

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

8. ~~Invalid origin in probe~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

The Safari permissions support probe passes url_for('blank.html') (a full URL with a path) as
origin, so when Safari implements permissions.setPermission the probe can still fail with
invalid-argument behavior and never signal support.
This breaks the probe’s purpose of flipping from pending to fixed when support lands.

Code

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[R63-66]

+            result = Permissions.new(driver).set_permission(
+              descriptor: Permissions::PermissionDescriptor.new(name: 'geolocation'),
+              state: :granted,
+              origin: url_for('blank.html')

Evidence
The probe currently passes a full URL as origin, while the existing permissions integration spec
explicitly uses window.location.origin, and url_for is implemented as a full URL generator via
the app server.
rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[60-68]
rb/spec/integration/selenium/webdriver/bidi/protocol/permissions_spec.rb[53-65]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[220-222]

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 Safari permissions support probe uses `origin: url_for('blank.html')`, but `url_for` returns a full URL (including a path). The existing permissions integration spec derives `origin` from `window.location.origin` (origin-only). If Safari implements the command and validates `origin`, the probe may continue failing and will not provide the intended “support landed” signal.

### Issue Context
- The probe should send a *valid* payload so that success indicates module support.
- Existing integration coverage shows `origin` should be `window.location.origin`.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[60-68]

### Suggested change
1. Navigate to a known page (e.g., `blank.html`).
2. Compute origin via JS: `driver.execute_script('return window.location.origin')`.
3. Pass that value as `origin:` to `set_permission` (and similarly for `embedded_origin` if used later).

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

9. ~~Prompt success path untested~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The PR replaces the existing handle_user_prompt integration tests with assertions that only cover
the "no prompt open" error case, removing success-path coverage for accepting/dismissing a real
alert/prompt. This reduces regression detection for browsingContext.handleUserPrompt behavior
(including user_text handling) when a prompt is actually present.

Code

rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[R87-90]

+          it 'returns a no such alert error when no prompt is open' do
+            expect {
+              browsing_context.handle_user_prompt(context: driver.window_handle, accept: true)
+            }.to raise_error(Error::NoSuchAlertError)

Evidence
The updated high-level spec only asserts NoSuchAlertError without opening any prompt, and the new
low-level protocol spec similarly only tests the no-prompt error path, leaving no integration
coverage for successful prompt handling.
rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[87-107]
rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[154-170]

Agent prompt

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

### Issue description
`handle_user_prompt` specs no longer exercise a real JavaScript alert/prompt flow (open dialog → call `handle_user_prompt` → verify the dialog closes / text is submitted). The new tests only assert `NoSuchAlertError` when no prompt is open, which can’t catch regressions in the actual prompt-handling behavior.

### Issue Context
This change appears to have been made to avoid known browser-specific failures, but it removed all visible success-path coverage from the Ruby BiDi integration specs.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[87-107]
- rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[154-170]

### Suggested fix
Reintroduce at least one success-path test that:
- Navigates to `alerts.html`, opens an alert/prompt.
- Calls `browsing_context.handle_user_prompt(...)`.
- Waits for the prompt to close and asserts expected page state.

If specific browsers are currently broken, gate only those browsers behind existing `pending_if`/`skip_if` guards rather than removing the success-path entirely for all browsers.

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

10. ~~Duplicated Chromium skip metadata~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Several Bluetooth protocol examples repeat the same skip_if condition and reason string for
Chromium. This makes future updates to the Chromium Bluetooth skip (scope/reason) more error-prone
because changes must be applied consistently across multiple places.

Code

rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[R291-293]

+            it 'accepts a prompt',
+               skip_if: {browser_family: :chromium,
+                         reason: 'chromium bluetooth device-response: undefined on Linux, times out elsewhere'} do

Evidence
The same skip_if clause (browser_family + identical reason) is duplicated across multiple
Bluetooth examples in the new protocol spec, indicating avoidable repetition introduced by the
latest changes.
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[290-301]
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[343-347]
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[363-367]
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[374-377]
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[432-435]
rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[517-520]

Agent prompt

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

### Issue description
Multiple Bluetooth specs duplicate identical `skip_if: {browser_family: :chromium, reason: ...}` metadata. This duplication increases the chance of inconsistent edits when the Chromium Bluetooth instability changes.

### Issue Context
Not all Bluetooth examples are skipped on Chromium—only a subset that depends on prompt/device-response behavior—so moving the guard to the top-level `describe Bluetooth` would be too broad.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[291-301]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[343-347]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[363-367]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[374-378]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[432-436]
- rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb[517-520]

### What to change
Create a shared constant/helper for the Chromium Bluetooth skip metadata (e.g., `CHROMIUM_BLUETOOTH_DEVICE_RESPONSE_SKIP = {browser_family: :chromium, reason: '...'}`) and reuse it in each affected `it` block, or group just the affected examples inside a nested context that applies `skip_if` once.

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

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

… handle_user_prompt specs

qodo-code-review · 2026-08-06T00:22:21Z

Code review by qodo was updated up to the latest commit 187b2c4

…probes

qodo-code-review · 2026-08-06T13:09:46Z

Code review by qodo was updated up to the latest commit 4f5d62b

qodo-code-review · 2026-08-06T16:45:46Z

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

…l Safari permissions probe

qodo-code-review · 2026-08-08T22:53:59Z

Code review by qodo was updated up to the latest commit 666b9ff

…back leak

qodo-code-review · 2026-08-09T01:15:04Z

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

…artScreencast guards

qodo-code-review · 2026-08-09T15:21:21Z

Code review by qodo was updated up to the latest commit 57638eb

…hromium bluetooth unconditionally

qodo-code-review · 2026-08-09T19:18:40Z

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

…ed guard

…ntext specs

… of shared locals

qodo-code-review · 2026-08-09T23:41:51Z

Code review by qodo was updated up to the latest commit 628da2f

…ss, not the build env

qodo-code-review · 2026-08-10T13:41:07Z

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

… environment

qodo-code-review · 2026-08-10T16:22:38Z

Code review by qodo was updated up to the latest commit 1858dcb

titusfortner added 7 commits August 5, 2026 18:00

[rb] scaffold BiDi protocol integration specs from schema

c8b37e0

[build] run edge-bidi targets in ci-ruby full suite

1375d0b

[rb] raise BiDi SerializationError on wire deserialization mismatches

fbe42ec

[rb] guard bidi protocol specs for unsupported browser commands and t…

e494908

…imeouts

[rb] add edge guards and edge timeout skips for bidi protocol specs

6397cfa

[build] run bidi protocol specs in strict serialization mode

2c3a80c

[rb] guard Safari bidi protocol specs for strict-mode SerializationError

a6e40d6

selenium-ci added C-rb Ruby Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Aug 5, 2026

qodo-code-review Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread rb/lib/selenium/webdriver/bidi/serialization/record.rb

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb Outdated

[rb] type inbound BiDi enum errors as SerializationError and relocate…

187b2c4

… handle_user_prompt specs

[rb] skip unsupported BiDi modules on Safari with per-module support …

4f5d62b

…probes

qodo-code-review Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb Outdated

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb

[rb] drop exception class from volatile Safari BiDi pending guards

8094039

qodo-code-review Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb

[rb] scope BiDi protocol spec guards by browser/OS and mirror the rea…

666b9ff

…l Safari permissions probe

qodo-code-review Bot reviewed Aug 8, 2026

View reviewed changes

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb

[rb] skip Edge bluetooth device-response in CI and fix subscribe call…

bee1210

…back leak

[rb] skip chromium bluetooth in CI and correct firefox setViewport/st…

57638eb

…artScreencast guards

qodo-code-review Bot reviewed Aug 9, 2026

View reviewed changes

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb Outdated

[rb] scope chromium bluetooth skip to CI so local runs still exercise it

9358d79

[rb] pin Safari setViewport pending to the assertion error and skip c…

c76c1af

…hromium bluetooth unconditionally

qodo-code-review Bot reviewed Aug 9, 2026

View reviewed changes

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb Outdated

titusfortner added 3 commits August 9, 2026 15:24

[rb] dedupe the chromium bluetooth device-response skip into one shar…

da67f4e

…ed guard

[rb] dedupe repeated Safari/Chromium guards in script and browsing_co…

58556b2

…ntext specs

[rb] move duplicated BiDi guards onto describe/context blocks instead…

628da2f

… of shared locals

[rb] enable strict BiDi serialization for protocol specs in the harne…

bc12b2c

…ss, not the build env

qodo-code-review Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread rb/spec/integration/selenium/webdriver/spec_helper.rb Outdated

[rb] enable strict BiDi serialization for all bidi specs via the test…

1858dcb

… environment

Uh oh!

Uh oh!

Conversation

titusfortner commented Aug 5, 2026

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

🤖 AI assistance

💡 Additional Considerations

🔄 Types of changes

Uh oh!

qodo-code-review Bot commented Aug 5, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 6, 2026

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 6, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 6, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 8, 2026

Uh oh!

qodo-code-review Bot commented Aug 9, 2026

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 9, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 9, 2026

Uh oh!

qodo-code-review Bot commented Aug 9, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qodo-code-review Bot commented Aug 5, 2026 •

edited

Loading