[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. ~~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

3. ~~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

4. ~~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

View more (2)

5. ~~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

6. ~~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

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

Previous review results

Review updated until commit bee1210 ⚖️ Balanced

Results up to commit a6e40d6 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (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. ~~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

Results up to commit 187b2c4 ⚖️ Balanced

No changes from previous review

Results up to commit 4f5d62b ⚖️ Balanced

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

1. ~~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

2. ~~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

Results up to commit 8094039 ⚖️ Balanced

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

1. ~~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

Results up to commit 666b9ff ⚖️ Balanced

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

1. ~~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

… 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

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

Navigation Menu

Uh oh!

Uh oh!

[rb] add low-level BiDi protocol integration specs - #17878

[rb] add low-level BiDi protocol integration specs#17878
titusfortner wants to merge 12 commits into
trunkfrom
rb-bidi-protocol-specs

titusfortner commented Aug 5, 2026

Uh oh!

qodo-code-review Bot commented Aug 5, 2026

Uh oh!

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

edited

Loading

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Previous review results

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

edited

Loading