Skip to content

[py] generate the internal BiDi protocol layer from the shared binding-neutral schema - #17761

Open
titusfortner wants to merge 14 commits into
SeleniumHQ:trunkfrom
titusfortner:bidi-gen-py
Open

[py] generate the internal BiDi protocol layer from the shared binding-neutral schema#17761
titusfortner wants to merge 14 commits into
SeleniumHQ:trunkfrom
titusfortner:bidi-gen-py

Conversation

@titusfortner

@titusfortner titusfortner commented Jul 10, 2026

Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

  • Adds a new internal selenium.webdriver.common._bidi package, generated at build time from the shared binding-neutral schema (projected once from the CDDL). The domain modules are not checked in.
  • Adds the hand-written runtime that the generated modules build on: the serialization runtime, the transport seam, the Domain base, and the package __init__. These four are the only tracked files in the package; the domain modules are generated and gitignored.
  • Consumes those signals so the wire boundary is validated at runtime, not merely typed.
  • Links each generated element back to its spec definition.
  • This code is additive; nothing has been re-implemented to use it yet.

✅ Compliance with the behavioral contract (#17786)

The layer conforms to all three decisions in the ADR. Validation lives at the boundaries and the
value objects are permissive, so the two directions differ. Each behavior is backed by a runtime
test in bidi_serialization_tests.py (the ADR is runtime-not-declaration).

  • Decision 1 — outbound validated before sending. Enum values, discriminators, required-field
    presence, primitive types (a fractional value is rejected for an integer field), and
    nullable-constant literals are checked before a frame goes out (Record.__post_init__,
    Union.build, Record.as_json, which errors on an unset required field); an extension may not
    shadow a declared field. A caller mistake is a local BiDiSerializationError, not a round-trip.
  • Decision 2 — inbound validated against the resolved type. Error responses are surfaced first
    (Transport.execute), never turned into a local serialization failure. A corrupt value errors
    (null in a non-nullable field, wrong primitive, cardinality mismatch, non-object where an object is
    required, unknown enum/union member, a constant that is not its literal-or-null). A missing
    required field
    is tolerated — warned and left omitted (UNSET, distinct from null) — so a client
    generated from one spec revision keeps working against a browser on another. An undeclared
    property
    on an extensible type is spec-sanctioned and preserved in the type's extras map silently;
    on a non-extensible type it is a deviation, warned and dropped. strict_inbound() escalates the
    genuine deviations (missing required, undeclared-on-closed) to errors — never a valid extensible extra.
  • Decision 3 — all objects typed, spec-mirrored names. Parameters and results are typed value
    objects (enums for closed vocabularies, a distinct type per union variant branched on by type);
    Python's native ints give full numeric precision; names mirror the spec; received values are
    represented as they arrived (no normalization).

🔧 Implementation Notes

  • The underscore namespace marks it unsupported and outside the deprecation policy; nothing is publicly exposed on the driver. The existing common/bidi code and its CDDL generator are left untouched — retiring or migrating that is out of scope.
  • Generated at build time, not committed. A Starlark rule projects the schema into the domain modules under common/_bidi/; only serialization.py, transport.py, domain.py, and __init__.py are hand-written and tracked. Regenerate the checked-out tree for local development with bazel run //py:generate-bidi-protocol.
  • The package lives under common/ alongside the existing generated common/bidi and common/devtools packages, matching the established structure.
  • Because the shared schema is already resolved (types, unions, nullability):
    • the generated methods take precisely-typed arguments, argument values are validated before being sent over the socket, and inbound results are self-validating typed objects rather than raw dicts
    • no per-construct special-casing in the generator via manifests is required
  • The signals are derived once in the shared projector; the Python generator reads them, so the bindings stay in parity by construction — each is a one-line consume plus runtime support.
  • Records are frozen/immutable.
  • Validation is asymmetric by direction (per the ADR): outbound requires every required field and rejects bad enums/discriminators/primitives; inbound errors only on a corrupt value, tolerates a missing required field (warn, leave omitted), and keeps an undeclared property — preserved silently on an extensible type, warned-and-dropped on a closed one (strict_inbound() opts into erroring on the deviations). This lets a client built from one spec revision keep working against a browser on another.
  • Each domain wraps the session's BiDi connection in its own lightweight Transport; the driver owns only the shared WebSocketConnection (where the id counter and callbacks live). Nothing on the driver imports the generated package, so non-BiDi sessions never load it.
  • The Domain base (the class each generated domain subclasses) lives in its own common/_bidi/domain.py module.
  • Each generated class/command/type carries a docstring link to its spec definition.
  • Code is properly linted & type checked.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: the generator, serialization runtime, transport seam, and tests — authored and reviewed iteratively
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

  • Is this the right namespace and the right way to namespace this code?
  • Any deprecations and re-implementations will be managed in follow-up PRs.

🔄 Types of changes

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

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Jul 10, 2026
@titusfortner
titusfortner marked this pull request as ready for review July 10, 2026 15:10
@qodo-code-review

qodo-code-review Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

[py] Generate internal BiDi protocol layer from the shared binding-neutral schema

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

Grey Divider

AI Description

• Adds an internal selenium.webdriver.common._bidi package, generated at build time from the
 shared BiDi schema.
• Implements the hand-written runtime (serialization, transport, Domain) that generated
 modules depend on.
• Adds Bazel + local-dev wiring and a comprehensive test suite validating wire-shape and runtime
 contract.
Diagram

graph TD
  Schema[("Shared BiDi schema.json")] --> Generator["generate_bidi_protocol.py"] --> Domains["Generated _bidi domain modules"]
  Domains --> Serialization["serialization.py (Record/Union)"] --> Transport["transport.py (Transport)"] --> WSConn["WebSocketConnection.send_cmd"] --> Browser{{"Browser BiDi"}}
  Domains --> DomainBase["domain.py (Domain)"] --> Transport
  WebDriver["Remote WebDriver"] --> WSConn --> Browser
  WebDriver --> Transport

  subgraph Legend
    direction LR
    _db[(Data/schema)] ~~~ _svc([Python module]) ~~~ _ext{{External}}
  end
Loading
High-Level Assessment

Generating the Python BiDi protocol layer from the shared, binding-neutral schema is the right long-term approach: it prevents per-binding drift, matches the Ruby precedent, and enables schema-driven runtime validation (objectOnly/preserveExtras/scalar) required by the ADR contract. Keeping generated domain modules out of git (with only the runtime checked in) minimizes review noise while preserving debuggability and local regeneration via Bazel.

Files changed (41) +6170 / -7 · 14 not counted

Enhancement (6) +1621 / -0
generate_bidi_protocol.pyAdd schema→Python generator for internal BiDi protocol modules +1062/-0

Add schema→Python generator for internal BiDi protocol modules

• Adds the Python generator that consumes the projected schema JSON and emits domain modules into 'selenium/webdriver/common/_bidi/', including wiring of runtime metadata (wire names, nullability, enum/ref/primitive, map-scalar handling), union selectors, and spec links.

py/generate_bidi_protocol.py

__init__.pyIntroduce internal _bidi package with warnings and regeneration notes +30/-0

Introduce internal _bidi package with warnings and regeneration notes

• Adds internal-API documentation and explains that domain modules are generated at build time and not checked in, with local regeneration instructions.

py/selenium/webdriver/common/_bidi/init.py

domain.pyAdd Domain base class that reuses the session’s shared transport +54/-0

Add Domain base class that reuses the session’s shared transport

• Implements a base class that accepts either a 'Transport' or a 'WebDriver', starting BiDi lazily when needed and routing domain commands through '_execute'.

py/selenium/webdriver/common/_bidi/domain.py

serialization.pyAdd strict serialization runtime (Record/Union/UNSET) for generated BiDi types +410/-0

Add strict serialization runtime (Record/Union/UNSET) for generated BiDi types

• Implements strict inbound validation and outbound serialization for generated dataclass records and discriminated unions, including UNSET vs null semantics, lazy type registry resolution, enum/primitive checks, 'objectOnly' union rejection of non-objects, and 'scalar' map-key validation. Supports gated extras preservation via '_EXTENSIBLE' (driven by schema 'preserveExtras').

py/selenium/webdriver/common/_bidi/serialization.py

transport.pyAdd Transport seam from generated layer to the websocket round-trip +58/-0

Add Transport seam from generated layer to the websocket round-trip

• Introduces 'Transport.execute()' which serializes params (if present), sends via 'send_cmd', raises on BiDi 'error', and parses 'result' into the declared generated result type.

py/selenium/webdriver/common/_bidi/transport.py

webdriver.pyAdd shared '_bidi_transport' lifecycle to Remote WebDriver +7/-0

Add shared '_bidi_transport' lifecycle to Remote WebDriver

• Adds a '_bidi_transport' attribute, resets it on quit, and instantiates it during '_start_bidi()' via a lazy import so non-BiDi users don’t pay import cost for the internal generated package.

py/selenium/webdriver/remote/webdriver.py

Refactor (1) +18 / -3
websocket_connection.pyRefactor BiDi command send/receive and add 'send_cmd' for non-coroutine callers +18/-3

Refactor BiDi command send/receive and add 'send_cmd' for non-coroutine callers

• Extracts shared id/envelope/round-trip logic into '_round_trip()' and adds 'send_cmd(method, params)' that returns raw replies without raising, used by the new '_bidi' transport. Updates existing 'execute()' to delegate to '_round_trip()'.

py/selenium/webdriver/remote/websocket_connection.py

Tests (29) +4412 / -0
browser_tests.pyAdd generated _bidi Browser domain integration tests +274/-0

Add generated _bidi Browser domain integration tests

• Introduces browser integration coverage for the generated internal protocol layer via real browser sessions.

py/test/selenium/webdriver/common/_bidi/browser_tests.py

browsing_context_tests.pyAdd generated _bidi BrowsingContext integration tests +555/-0

Add generated _bidi BrowsingContext integration tests

• Adds end-to-end tests that exercise generated browsing context commands and typed results against real browsers.

py/test/selenium/webdriver/common/_bidi/browsing_context_tests.py

emulation_tests.pyAdd generated _bidi Emulation integration tests +527/-0

Add generated _bidi Emulation integration tests

• Adds integration tests that validate generated emulation commands work against a browser.

py/test/selenium/webdriver/common/_bidi/emulation_tests.py

errors_tests.pyAdd generated _bidi error-handling integration tests +121/-0

Add generated _bidi error-handling integration tests

• Adds integration tests validating error cases and raising behavior when driving the generated layer.

py/test/selenium/webdriver/common/_bidi/errors_tests.py

input_tests.pyAdd generated _bidi Input integration tests +542/-0

Add generated _bidi Input integration tests

• Adds browser integration tests for generated input commands and relevant payload shapes.

py/test/selenium/webdriver/common/_bidi/input_tests.py

network_tests.pyAdd generated _bidi Network integration tests +62/-0

Add generated _bidi Network integration tests

• Adds integration tests for generated network commands and types.

py/test/selenium/webdriver/common/_bidi/network_tests.py

permissions_tests.pyAdd generated _bidi Permissions integration tests +114/-0

Add generated _bidi Permissions integration tests

• Adds integration tests for generated permissions commands against a browser session.

py/test/selenium/webdriver/common/_bidi/permissions_tests.py

script_tests.pyAdd generated _bidi Script integration tests +623/-0

Add generated _bidi Script integration tests

• Adds integration tests for generated script commands and their typed results.

py/test/selenium/webdriver/common/_bidi/script_tests.py

session_tests.pyAdd generated _bidi Session integration tests +43/-0

Add generated _bidi Session integration tests

• Adds integration tests that validate generated session commands against a browser session.

py/test/selenium/webdriver/common/_bidi/session_tests.py

storage_tests.pyAdd generated _bidi Storage integration tests +525/-0

Add generated _bidi Storage integration tests

• Adds integration tests for generated storage commands and payload shapes, including extras behavior driven by 'preserveExtras'.

py/test/selenium/webdriver/common/_bidi/storage_tests.py

webextension_tests.pyAdd generated _bidi WebExtension integration tests +171/-0

Add generated _bidi WebExtension integration tests

• Adds integration tests for generated web extension commands against a browser session.

py/test/selenium/webdriver/common/_bidi/webextension_tests.py

browser_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout (no substantive behavior changes implied by this diff summary).

py/test/selenium/webdriver/common/bidi/browser_tests.py

browsing_context_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/browsing_context_tests.py

emulation_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/emulation_tests.py

errors_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/errors_tests.py

input_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/input_tests.py

integration_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/integration_tests.py

log_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout (and absorbs coverage removed from the deleted 'bidi_tests.py').

py/test/selenium/webdriver/common/bidi/log_tests.py

network_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/network_tests.py

permissions_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/permissions_tests.py

protocol_tests.pyAdd browser round-trip tests for generated internal '_bidi' layer +70/-0

Add browser round-trip tests for generated internal '_bidi' layer

• Adds end-to-end tests that drive generated '_bidi' modules directly against a real browser, validating real wire payload shapes (including nested record/list results).

py/test/selenium/webdriver/common/bidi/protocol_tests.py

quit_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/quit_tests.py

script_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout (and absorbs coverage removed from the deleted 'bidi_tests.py').

py/test/selenium/webdriver/common/bidi/script_tests.py

session_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/session_tests.py

storage_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/storage_tests.py

webextension_tests.pyReorganize existing BiDi tests under 'bidi/' folder not counted

Reorganize existing BiDi tests under 'bidi/' folder

• Moves/renames the test module into the new 'py/test/selenium/webdriver/common/bidi/' layout.

py/test/selenium/webdriver/common/bidi/webextension_tests.py

bidi_protocol_command_tests.pyAdd unit tests for generated command methods and schema-driven validation +185/-0

Add unit tests for generated command methods and schema-driven validation

• Tests generated domains using a stand-in connection to validate command dispatch, param serialization, typed result parsing, and pre-wire validation (enum values, discriminator membership). Includes tests for extras preservation vs dropping per 'preserveExtras'.

py/test/unit/selenium/webdriver/common/bidi_protocol_command_tests.py

bidi_serialization_tests.pyAdd unit tests for the serialization runtime behaviors (Record/Union/UNSET) +453/-0

Add unit tests for the serialization runtime behaviors (Record/Union/UNSET)

• Adds isolated runtime tests for required/optional/nullable semantics, strict primitive checks, enum validation, union selection rules (discriminator + presence), 'objectOnly' rejection of non-object payloads, and 'scalar' map-key passthrough with primitive validation.

py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py

bidi_transport_tests.pyAdd unit tests for Transport and Domain seam behavior +147/-0

Add unit tests for Transport and Domain seam behavior

• Adds tests that verify 'Transport.execute()' wire framing expectations against a driving stand-in and confirms Domain can accept a Transport, reuse a driver’s shared transport, or start BiDi lazily when needed.

py/test/unit/selenium/webdriver/common/bidi_transport_tests.py

Documentation (1) +1 / -1
TESTING.mdUpdate documented bazel test target path after test reorg +1/-1

Update documented bazel test target path after test reorg

• Adjusts the example command to reflect that BiDi tests now live under 'test/selenium/webdriver/common/bidi/' rather than flattened filenames.

py/TESTING.md

Other (4) +118 / -3
.gitignoreIgnore generated _bidi domain modules while keeping runtime files tracked +6/-0

Ignore generated _bidi domain modules while keeping runtime files tracked

• Adds ignore rules for all files under 'py/selenium/webdriver/common/_bidi/*' but explicitly un-ignores the four hand-written runtime files ('__init__.py', 'serialization.py', 'transport.py', 'domain.py').

.gitignore

BUILD.bazelAdd Bazel generation + library targets for the internal _bidi protocol layer +50/-2

Add Bazel generation + library targets for the internal _bidi protocol layer

• Introduces 'generate_bidi_protocol' plumbing (rule invocation, generator binaries, and 'bidi_protocol' py_library combining generated sources with the checked-in runtime). Updates test globs to split 'bidi/' and '_bidi/' and adds ':bidi_protocol' as a dependency where tests import generated modules directly.

py/BUILD.bazel

generate_bidi_protocol.bzlAdd Bazel rule to run the BiDi protocol generator over the shared schema +61/-0

Add Bazel rule to run the BiDi protocol generator over the shared schema

• Defines a 'generate_bidi_protocol' rule that executes the generator in the exec configuration, producing one '.py' output per BiDi domain module under the target package directory.

py/private/generate_bidi_protocol.bzl

python.rakeInclude 'common/_bidi' output in Python 'local_dev' task +1/-1

Include 'common/_bidi' output in Python 'local_dev' task

• Updates local-dev copy logic to include 'common/_bidi' alongside 'common/bidi' and 'common/devtools' so build-generated protocol modules land in the working tree for development workflows.

rake_tasks/python.rake

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new internal Python WebDriver BiDi protocol layer under selenium.webdriver._bidi, generated from the shared binding-neutral BiDi schema, plus a small handwritten runtime (serialization + transport seam) and Bazel/CI wiring to keep checked-in generated files in sync.

Changes:

  • Introduces the generated internal _bidi protocol package (domains, types, commands/events metadata) and a handwritten runtime (serialization.py, transport.py).
  • Wires RemoteWebDriver to create a per-session _bidi_transport lazily when BiDi is started.
  • Adds unit + end-to-end tests, plus a verify-bidi-protocol Bazel target and CI step to detect drift/hand-edits.

Reviewed changes

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

Show a summary per file
File Description
py/verify_bidi_protocol.py Verifies checked-in _bidi files match generator output.
py/test/unit/selenium/webdriver/common/bidi_transport_tests.py Unit coverage for the handwritten transport/domain seam.
py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py Unit coverage for strict (de)serialization runtime rules.
py/test/unit/selenium/webdriver/common/bidi_protocol_command_tests.py Exercises generated command surfaces over a stand-in transport.
py/test/selenium/webdriver/common/bidi_protocol_tests.py Browser-level sanity checks that generated types round-trip against a real BiDi session.
py/selenium/webdriver/remote/webdriver.py Adds _bidi_transport lifecycle + lazy import/creation in _start_bidi().
py/selenium/webdriver/_bidi/__init__.py Generated package initializer exporting domain classes.
py/selenium/webdriver/_bidi/serialization.py Handwritten serialization runtime used by generated records/unions (plus registry).
py/selenium/webdriver/_bidi/transport.py Handwritten websocket seam (Transport) and base domain (Domain).
py/selenium/webdriver/_bidi/browsing_context.py Generated browsingContext domain (types + commands + event metadata).
py/selenium/webdriver/_bidi/bluetooth.py Generated bluetooth domain (types + commands + event metadata).
py/selenium/webdriver/_bidi/browser.py Generated browser domain (types + commands).
py/selenium/webdriver/_bidi/emulation.py Generated emulation domain (types + commands).
py/selenium/webdriver/_bidi/input.py Generated input domain (types + commands + event metadata).
py/selenium/webdriver/_bidi/log.py Generated log domain (types + event metadata).
py/selenium/webdriver/_bidi/network.py Generated network domain (types + commands + event metadata).
py/selenium/webdriver/_bidi/permissions.py Generated permissions domain (types + commands).
py/selenium/webdriver/_bidi/script.py Generated script domain (types + commands + event metadata).
py/selenium/webdriver/_bidi/session.py Generated session domain (types + commands).
py/selenium/webdriver/_bidi/speculation.py Generated speculation domain (types + event metadata).
py/selenium/webdriver/_bidi/storage.py Generated storage domain (types + commands).
py/selenium/webdriver/_bidi/user_agent_client_hints.py Generated userAgentClientHints domain (types + commands).
py/selenium/webdriver/_bidi/web_extension.py Generated webExtension domain (types + commands).
py/BUILD.bazel Adds generator target, _bidi library, and verify test; packages _bidi.
.github/workflows/ci-python.yml Runs //py:verify-bidi-protocol in CI alongside unit tests.

Comment thread py/selenium/webdriver/_bidi/serialization.py
Comment thread py/selenium/webdriver/_bidi/serialization.py
Comment thread py/selenium/webdriver/_bidi/serialization.py
@qodo-code-review

qodo-code-review Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Grey Divider


Action required

1. Union scalar bypass ✗ Dismissed 🐞 Bug ≡ Correctness
Description
Union.from_json() returns any non-dict payload unchanged, so union-typed fields can silently
accept unexpected scalar values even when the union has no scalar arm. This weakens the PR’s strict
inbound validation because _read_scalar() routes union refs to Union.from_json() without
enforcing object-ness.
Code

py/selenium/webdriver/_bidi/serialization.py[R300-306]

+    @classmethod
+    def from_json(cls, payload: Any) -> Any:
+        # A bare scalar arm (e.g. input.Origin's "viewport") has no object to
+        # dispatch on, so it is returned unchanged.
+        if not isinstance(payload, dict):
+            return payload
+        variant = cls._select(payload)
Evidence
_read_scalar() allows non-dict values for union refs and delegates to Union.from_json(), and
Union.from_json() immediately returns non-dict values unchanged—so scalars bypass union validation
entirely.

py/selenium/webdriver/_bidi/serialization.py[262-277]
py/selenium/webdriver/_bidi/serialization.py[300-312]

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

## Issue description
`Union.from_json()` currently accepts any non-dict payload by returning it unchanged. This allows malformed payloads (scalar instead of object) to pass through for any union-ref field.

## Issue Context
`_read_scalar()` explicitly skips the "must be dict" check for unions and calls `klass.from_json(raw)`. With the current `Union.from_json` early return, that means *all* unions accept scalars, not just those that legitimately have scalar arms.

## Fix Focus Areas
- py/selenium/webdriver/_bidi/serialization.py[262-277]
- py/selenium/webdriver/_bidi/serialization.py[300-312]

## Implementation direction
- Make scalar acceptance opt-in per union (e.g., `_ALLOW_SCALAR: bool` or `_SCALAR_PRIMITIVE: str|None`), defaulting to rejecting non-dict payloads.
- Have the generator set this flag only for unions that truly include a scalar arm (and optionally validate the scalar type/value when enabled).

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


2. Presence keys mismatch ✗ Dismissed 🐞 Bug ≡ Correctness
Description
The generated script.RemoteReference union uses a presence key sharedId (wire name), but
Union.build() checks presence against Python kwargs keys; because the corresponding Python field
is shared_id, RemoteReference.build(shared_id=...) cannot match a variant and will raise
BiDiSerializationError. This makes outbound construction via Union.build() incorrect for this
union (and any similar presence union where wire keys differ from Python names).
Code

py/selenium/webdriver/_bidi/script.py[R817-820]

+@register("script.RemoteReference")
+class RemoteReference(Union):
+    _PRESENCE = (("script.SharedReference", ("sharedId",)), ("script.RemoteObjectReference", ("handle",)))
+
Evidence
RemoteReference presence selection is keyed on sharedId, but the generated record field name is
shared_id; Union._outbound() checks kwargs.get(key) for each presence key, so a call providing
shared_id=... will never satisfy the sharedId presence check and the union cannot be built via
build().

py/selenium/webdriver/_bidi/script.py[817-823]
py/selenium/webdriver/_bidi/script.py[309-324]
py/selenium/webdriver/_bidi/serialization.py[353-364]

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

## Issue description
`Union.build()` uses `_PRESENCE` keys to check `kwargs.get(key)`, but `script.RemoteReference._PRESENCE` contains the wire key `sharedId` while the Python field/kwargs name is `shared_id`. This prevents selecting the SharedReference arm when building outbound values.

## Issue Context
- Inbound selection wants wire keys (e.g., `sharedId` in the payload).
- Outbound selection wants Python keys (e.g., `shared_id` in kwargs).
Today these are forced to share one `_PRESENCE` table, which breaks when they differ.

## Fix Focus Areas
- py/selenium/webdriver/_bidi/serialization.py[353-364]
- py/selenium/webdriver/_bidi/script.py[817-820]

## Implementation direction
- Introduce separate presence tables for inbound vs outbound (e.g., `_PRESENCE_WIRE` and `_PRESENCE_FIELDS`), and have `_select()` use wire keys while `_outbound()` uses python field names.
- Update the generator to emit both sets for unions that use presence selection (or emit python names and have `_select()` also accept wire-name equivalents via a mapping).

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


3. _command() missing return annotation ✓ Resolved 📘 Rule violation ✧ Quality
Description
The newly added generator helper _command() has no explicit return type annotation, violating the
requirement that new function signatures include return annotations. This reduces type-checking
coverage and can cause inconsistent typing across the new BiDi transport seam.
Code

py/selenium/webdriver/_bidi/transport.py[R37-45]

+def _command(method: str, params: dict):
+    """A command_builder coroutine: yield the wire frame, return the raw result.
+
+    ``WebSocketConnection.execute`` speaks this protocol (``next`` then ``send``).
+    Params arrive already wire-shaped (from ``Record.as_json``), so the connection's
+    dataclass encoder passes them through untouched — no double camelCasing.
+    """
+    result = yield {"method": method, "params": params}
+    return result
Evidence
PR Compliance ID 337802 requires explicit type annotations on new function signatures, including
return types. The added _command() definition has parameter annotations but omits the -> ...
return annotation.

Rule 337802: Require type annotations on new function and method signatures
py/selenium/webdriver/_bidi/transport.py[37-45]

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

## Issue description
`py/selenium/webdriver/_bidi/transport.py` introduces `_command()` without an explicit return type annotation.

## Issue Context
Compliance requires all new/modified Python function and method signatures to include parameter annotations and an explicit return type annotation. `_command()` is a generator-style coroutine used with `WebSocketConnection.execute`, so its signature should still declare a return type (e.g., a `Generator[...]` type, or at minimum `-> Any`).

## Fix Focus Areas
- py/selenium/webdriver/_bidi/transport.py[37-45]

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



Remediation recommended

4. Import-order type failures ✗ Dismissed 🐞 Bug ☼ Reliability
Description
Cross-domain refs (e.g. browsingContext.ElementClipRectangle referencing script.SharedReference)
rely on the global registry, but domain modules don’t import their dependencies at runtime;
importing a single domain module can therefore fail at (de)serialization time with `unknown BiDi
type ... (module not imported?)`. This is an import-order dependent runtime failure (package-level
selenium.webdriver._bidi import avoids it by importing all domains).
Code

py/selenium/webdriver/_bidi/browsing_context.py[R192-196]

+@register("browsingContext.ElementClipRectangle")
+@dataclass(frozen=True)
+class ElementClipRectangle(Record):
+    element: SharedReference = field(metadata=meta("element", required=True, ref="script.SharedReference"))
+    type: str = field(default="element", init=False, metadata=meta("type", required=True, fixed="element"))
Evidence
The generated record contains a ref to script.SharedReference, but the module doesn’t import
script; if script hasn’t been imported elsewhere, resolve('script.SharedReference') raises
with an explicit “module not imported?” message.

py/selenium/webdriver/_bidi/browsing_context.py[23-30]
py/selenium/webdriver/_bidi/browsing_context.py[192-197]
py/selenium/webdriver/_bidi/serialization.py[132-137]

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

## Issue description
Some generated types reference schema names in other domain modules via `ref=...`, but the registry is only populated when those other modules have been imported. If users import only one domain module (not the package `selenium.webdriver._bidi`), deserialization of cross-domain refs can fail.

## Issue Context
This is primarily an import-order hazard. Package-level import (`import selenium.webdriver._bidi`) imports all domain modules and registers all types, but direct module imports do not.

## Fix Focus Areas
- py/selenium/webdriver/_bidi/serialization.py[132-137]
- py/selenium/webdriver/_bidi/browsing_context.py[192-197]
- py/selenium/webdriver/_bidi/browsing_context.py[23-30]

## Implementation direction
- Option A (runtime): Enhance `resolve()` to attempt a lazy import based on the schema name prefix before failing (e.g., importing `selenium.webdriver._bidi.script` when resolving `script.*`).
- Option B (generator): Emit minimal runtime imports for referenced domains (with care to avoid cycles), or provide a documented/obvious "load all" initializer that callers must invoke.

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



Informational

5. Verifier misses extra files 🐞 Bug ⚙ Maintainability
Description
verify_bidi_protocol.py only checks that files produced by the generator match the committed
copies; it never fails if _bidi/ contains extra .py files not produced by the generator. This
can allow removed/renamed generated modules to linger unnoticed.
Code

py/verify_bidi_protocol.py[R36-41]

+    rendered = gen.render_all(schema_path)
+    stale = [
+        name
+        for name, contents in rendered.items()
+        if not (bidi_dir / name).exists() or (bidi_dir / name).read_text() != contents
+    ]
Evidence
The script computes stale solely from rendered.items() and never compares the on-disk directory
listing against the rendered set, so extra files are ignored.

py/verify_bidi_protocol.py[32-41]

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 verification step does not detect extra files present on disk but not produced by `render_all()`, so stale generated modules can persist.

## Issue Context
This is a drift-guard completeness gap: it catches mismatches for expected outputs but not unexpected leftovers.

## Fix Focus Areas
- py/verify_bidi_protocol.py[36-41]

## Implementation direction
- After computing `rendered`, list `bidi_dir/*.py` (and possibly exclude hand-written runtime files like `serialization.py`, `transport.py`) and fail if any generated-looking file is not in `rendered.keys()`.
- Print the unexpected file list alongside stale mismatches.

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


Grey Divider

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

Qodo Logo

Comment thread py/selenium/webdriver/_bidi/transport.py Outdated
Comment thread py/selenium/webdriver/_bidi/script.py Outdated
Comment thread py/selenium/webdriver/common/_bidi/serialization.py
Comment thread py/selenium/webdriver/_bidi/browsing_context.py Outdated
Comment thread py/verify_bidi_protocol.py Outdated
@titusfortner
titusfortner marked this pull request as draft July 10, 2026 15:21
Comment thread py/selenium/webdriver/_bidi/browser.py Outdated
@AutomatedTester

Copy link
Copy Markdown
Member

The direction of this looks ok so far but need to see how this will be used as that code isn't here

@AutomatedTester

Copy link
Copy Markdown
Member

PR has a lot of unnecessary files in it now in case you're not aware

@titusfortner

Copy link
Copy Markdown
Member Author

@AutomatedTester yes, to work with the latest schema it pulled in #17784 which should merge soon

@titusfortner
titusfortner force-pushed the bidi-gen-py branch 3 times, most recently from 6057552 to a227d86 Compare July 20, 2026 16:10
@titusfortner

Copy link
Copy Markdown
Member Author

need to see how this will be used as that code isn't here

@AutomatedTester to clarify, the intention is that our users will write this code if they need it. The usage in selenium will be limited to re-implementing current API and whatever additional facades we agree to, so I can't give examples for any of that yet, bu to give a better view of what usage looks like, I ported all the existing integration tests to use this code.

py/test/selenium/webdriver/common/_bidi/. (instantiates Domain(driver), pass typed records in, read typed results out, against real browsers).

A few concrete comparisons

# convenience method — returns the handle
handle = driver.browsing_context.create(type=WindowTypes.TAB)

# generated classes — read .context off the typed result
handle = BrowsingContext(driver).create(type=CreateType.TAB).context
# convenience method — plain string name
driver.permissions.set_permission("geolocation", PermissionState.GRANTED, origin)

# generated classes — wrap the name in a descriptor
Permissions(driver).set_permission(PermissionDescriptor(name="geolocation"), PermissionState.GRANTED, origin)
# convenience method — dict in, raw dicts out
nodes = driver.browsing_context.locate_nodes(context=cid, locator={"type": "css", "value": "div.content"})
name = nodes[0]["value"]["localName"]

# generated classes — typed locator in, typed nodes out
nodes = BrowsingContext(driver).locate_nodes(context=cid, locator=CssLocator(value="div.content")).nodes
name = nodes[0].value.local_name
# convenience method — flat bool + folder
driver.browser.set_download_behavior(allowed=True, destination_folder=folder)

# generated classes — pass the typed variant
Browser(driver).set_download_behavior(DownloadBehaviorAllowed(destination_folder=folder))

@titusfortner
titusfortner force-pushed the bidi-gen-py branch 2 times, most recently from 5899a33 to 2b06fe9 Compare July 21, 2026 20:38
@titusfortner
titusfortner marked this pull request as ready for review July 21, 2026 20:38
@qodo-code-review

qodo-code-review Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Grey Divider


Action required

1. Map entries unvalidated outbound ✓ Resolved 🐞 Bug ≡ Correctness
Description
Outbound validation skips per-entry validation when w.scalar is set, so map-encoded fields (a list
of [key, value] pairs) are not checked for being 2-item pairs or for scalar-key type before
sending.
Inbound enforces the [key, value] pair shape and validates scalar keys, so outbound/inbound
validation is inconsistent for the same schema construct.
Code

py/selenium/webdriver/common/_bidi/serialization.py[R434-436]

+def _validate_outbound_scalar(owner: str, name: str, w: _Wire, value: Any) -> None:
+    if value is None or w.scalar is not None:
+        return  # map entries carry their own shape; null is handled upstream
Evidence
Outbound validation explicitly bails out for any field carrying scalar, which is exactly how
map-encoded [key, value] lists are represented. Inbound for the same construct strictly enforces
pair shape and scalar-key typing, demonstrating the outbound gap and inconsistency.

py/selenium/webdriver/common/_bidi/serialization.py[417-436]
py/selenium/webdriver/common/_bidi/serialization.py[350-363]
py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[173-181]

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

### Issue description
`_validate_outbound()` iterates list fields and calls `_validate_outbound_scalar()` for each element. When the field uses `scalar` (map-encoded `[key, value]` entries), `_validate_outbound_scalar()` returns immediately, so individual entries are never validated.

This allows malformed entries (wrong shape, wrong scalar key type) to be serialized and sent despite the ADR goal of outbound boundary validation.

### Issue Context
Inbound map handling is strict:
- Each element must be a 2-item list
- Scalar keys are type-checked

Outbound should reject obviously-malformed map entries similarly (at least pair shape + scalar-key type), before `_as_json()` serializes and sends.

### Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[417-454]
- py/selenium/webdriver/common/_bidi/serialization.py[350-379]

### Suggested fix
Implement explicit outbound validation for `scalar` map fields, e.g.:
- If `w.scalar is not None`, validate each list element is a 2-item list/tuple.
- Validate scalar keys against the same primitive checks as inbound (`_PRIMITIVE_CHECKS`) for the allowed scalar(s).
- (Optionally) validate that the value side is not a bare scalar (since inbound always deserializes it as an object-only union) and/or is serializable as a record.

Add/extend unit tests to cover:
- entry not a pair (len != 2)
- scalar key wrong type (e.g., int where `scalar="str"`)
- value being an unexpected scalar

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


2. Record.from_json non-dict crash ✓ Resolved 🐞 Bug ☼ Reliability
Description
Record.from_json() assumes payload is a dict and can raise raw TypeError/AttributeError when
the wire payload is not an object, leaking non-WebDriverException errors to callers. This can
surface via Transport.execute() which calls result.from_json(...) directly on websocket replies
without normalizing failures to BiDiSerializationError.
Code

py/selenium/webdriver/common/_bidi/serialization.py[R223-235]

+    @classmethod
+    def from_json(cls, payload: dict) -> Any:
+        kwargs: dict = {}
+        known: set[str] = set()
+        for f in _fields(cls):
+            w = _wire_of(f)
+            known.add(w.wire)
+            if w.fixed is not UNSET or f.name == "extensions":
+                continue
+            kwargs[f.name] = _read_field(cls, f.name, w, payload)
+        if cls._EXTENSIBLE:
+            kwargs["extensions"] = {k: v for k, v in payload.items() if k not in known}
+        return cls(**kwargs)
Evidence
Record.from_json uses dict operations (_read_field(..., payload) and payload.items()) without
a runtime type check; if payload is not a dict this will raise raw Python exceptions.
Transport.execute calls result.from_json(value) on the reply payload, so these raw exceptions
propagate instead of becoming BiDiSerializationError (a WebDriverException).

py/selenium/webdriver/common/_bidi/serialization.py[223-235]
py/selenium/webdriver/common/_bidi/transport.py[52-58]
py/selenium/webdriver/common/_bidi/serialization.py[44-45]

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

### Issue description
`Record.from_json` assumes the payload is a `dict` and will throw raw Python exceptions if it receives a non-dict payload (e.g., browser returns a scalar/array where an object is expected). This breaks the intended error boundary: deserialization failures should raise `BiDiSerializationError` (a `WebDriverException`), not `TypeError`/`AttributeError`.

### Issue Context
`Transport.execute` calls `result.from_json(value)` directly on the raw websocket reply result, so any non-`BiDiSerializationError` exceptions escape to callers.

### Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[223-235]
- py/selenium/webdriver/common/_bidi/transport.py[52-58]
- py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[1-453]

### Suggested fix
1. Change `Record.from_json` signature to accept `payload: Any` (or keep `dict` but still validate).
2. At the top of `Record.from_json`, add:
  - `if not isinstance(payload, dict): raise BiDiSerializationError(f"{cls.__name__} expected an object on the wire, got {type(payload).__name__} {payload!r}")`
3. Add a unit test that calls a small Record type’s `from_json` with a non-dict (e.g. `[]` or `"x"`) and asserts `BiDiSerializationError` is raised.

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


3. Schema drift breaks Bazel ✓ Resolved 🐞 Bug ☼ Reliability
Description
The Bazel rule declares a fixed set of generated domain module outputs, but the generator renders
modules dynamically from the schema’s domains; a schema domain add/remove/rename can cause missing
declared outputs or undeclared outputs, breaking builds.
Code

py/private/generate_bidi_protocol.bzl[R8-36]

+# Generated domain modules, snake_case (one per BiDi domain in the schema). The
+# package ``__init__.py`` is hand-written and checked in, so it is not generated here.
+_MODULES = [
+    "bluetooth",
+    "browser",
+    "browsing_context",
+    "emulation",
+    "input",
+    "log",
+    "network",
+    "permissions",
+    "script",
+    "session",
+    "speculation",
+    "storage",
+    "user_agent_client_hints",
+    "web_extension",
+]
+
+def _generate_bidi_protocol_impl(ctx):
+    outputs = [ctx.actions.declare_file(ctx.attr.package + "/" + name + ".py") for name in _MODULES]
+
+    ctx.actions.run(
+        inputs = [ctx.file.schema],
+        outputs = outputs,
+        executable = ctx.executable.generator,
+        # The generator writes one file per domain into the output dir.
+        arguments = [ctx.file.schema.path, outputs[-1].dirname],
+        use_default_shell_env = True,
Evidence
The Bazel rule hard-codes _MODULES and uses it to declare outputs, but the generator builds
modules from schema.domains() and writes all of them to the output directory, making the output
set schema-dependent rather than _MODULES-dependent.

py/private/generate_bidi_protocol.bzl[8-39]
py/generate_bidi_protocol.py[262-283]
py/generate_bidi_protocol.py[1026-1035]

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

### Issue description
`generate_bidi_protocol.bzl` declares a fixed `_MODULES` output list, but `generate_bidi_protocol.py` generates one module per domain discovered in the schema at runtime. When the schema changes, Bazel can either:
- fail because a declared output wasn’t created, or
- fail/sandbox-violate by writing extra (undeclared) outputs.

### Issue Context
Bazel actions should only write declared outputs; right now the generator is schema-driven while the rule is list-driven.

### Fix Focus Areas
- py/private/generate_bidi_protocol.bzl[8-39]
- py/generate_bidi_protocol.py[1026-1058]

### Suggested fix approach
Pick one single source of truth:
1) **Preferred**: change the Bazel rule to declare a single output directory (tree artifact) and have the generator write all modules into it; return that directory in `DefaultInfo`.
2) **Alternative**: pass the expected module list from the Bazel rule into the generator (e.g., a `--modules` arg), and make the generator:
  - generate *only* those modules, and
  - error out if the schema domains don’t exactly match the expected list (so drift is caught deterministically without writing undeclared files).

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


View more (3)
4. Union scalar bypass ✗ Dismissed 🐞 Bug ≡ Correctness
Description
Union.from_json() returns any non-dict payload unchanged, so union-typed fields can silently
accept unexpected scalar values even when the union has no scalar arm. This weakens the PR’s strict
inbound validation because _read_scalar() routes union refs to Union.from_json() without
enforcing object-ness.
Code

py/selenium/webdriver/_bidi/serialization.py[R300-306]

+    @classmethod
+    def from_json(cls, payload: Any) -> Any:
+        # A bare scalar arm (e.g. input.Origin's "viewport") has no object to
+        # dispatch on, so it is returned unchanged.
+        if not isinstance(payload, dict):
+            return payload
+        variant = cls._select(payload)
Evidence
_read_scalar() allows non-dict values for union refs and delegates to Union.from_json(), and
Union.from_json() immediately returns non-dict values unchanged—so scalars bypass union validation
entirely.

py/selenium/webdriver/_bidi/serialization.py[262-277]
py/selenium/webdriver/_bidi/serialization.py[300-312]

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

## Issue description
`Union.from_json()` currently accepts any non-dict payload by returning it unchanged. This allows malformed payloads (scalar instead of object) to pass through for any union-ref field.

## Issue Context
`_read_scalar()` explicitly skips the "must be dict" check for unions and calls `klass.from_json(raw)`. With the current `Union.from_json` early return, that means *all* unions accept scalars, not just those that legitimately have scalar arms.

## Fix Focus Areas
- py/selenium/webdriver/_bidi/serialization.py[262-277]
- py/selenium/webdriver/_bidi/serialization.py[300-312]

## Implementation direction
- Make scalar acceptance opt-in per union (e.g., `_ALLOW_SCALAR: bool` or `_SCALAR_PRIMITIVE: str|None`), defaulting to rejecting non-dict payloads.
- Have the generator set this flag only for unions that truly include a scalar arm (and optionally validate the scalar type/value when enabled).

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


5. Presence keys mismatch ✗ Dismissed 🐞 Bug ≡ Correctness
Description
The generated script.RemoteReference union uses a presence key sharedId (wire name), but
Union.build() checks presence against Python kwargs keys; because the corresponding Python field
is shared_id, RemoteReference.build(shared_id=...) cannot match a variant and will raise
BiDiSerializationError. This makes outbound construction via Union.build() incorrect for this
union (and any similar presence union where wire keys differ from Python names).
Code

py/selenium/webdriver/_bidi/script.py[R817-820]

+@register("script.RemoteReference")
+class RemoteReference(Union):
+    _PRESENCE = (("script.SharedReference", ("sharedId",)), ("script.RemoteObjectReference", ("handle",)))
+
Evidence
RemoteReference presence selection is keyed on sharedId, but the generated record field name is
shared_id; Union._outbound() checks kwargs.get(key) for each presence key, so a call providing
shared_id=... will never satisfy the sharedId presence check and the union cannot be built via
build().

py/selenium/webdriver/_bidi/script.py[817-823]
py/selenium/webdriver/_bidi/script.py[309-324]
py/selenium/webdriver/_bidi/serialization.py[353-364]

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

## Issue description
`Union.build()` uses `_PRESENCE` keys to check `kwargs.get(key)`, but `script.RemoteReference._PRESENCE` contains the wire key `sharedId` while the Python field/kwargs name is `shared_id`. This prevents selecting the SharedReference arm when building outbound values.

## Issue Context
- Inbound selection wants wire keys (e.g., `sharedId` in the payload).
- Outbound selection wants Python keys (e.g., `shared_id` in kwargs).
Today these are forced to share one `_PRESENCE` table, which breaks when they differ.

## Fix Focus Areas
- py/selenium/webdriver/_bidi/serialization.py[353-364]
- py/selenium/webdriver/_bidi/script.py[817-820]

## Implementation direction
- Introduce separate presence tables for inbound vs outbound (e.g., `_PRESENCE_WIRE` and `_PRESENCE_FIELDS`), and have `_select()` use wire keys while `_outbound()` uses python field names.
- Update the generator to emit both sets for unions that use presence selection (or emit python names and have `_select()` also accept wire-name equivalents via a mapping).

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


6. _command() missing return annotation ✓ Resolved 📘 Rule violation ✧ Quality
Description
The newly added generator helper _command() has no explicit return type annotation, violating the
requirement that new function signatures include return annotations. This reduces type-checking
coverage and can cause inconsistent typing across the new BiDi transport seam.
Code

py/selenium/webdriver/_bidi/transport.py[R37-45]

+def _command(method: str, params: dict):
+    """A command_builder coroutine: yield the wire frame, return the raw result.
+
+    ``WebSocketConnection.execute`` speaks this protocol (``next`` then ``send``).
+    Params arrive already wire-shaped (from ``Record.as_json``), so the connection's
+    dataclass encoder passes them through untouched — no double camelCasing.
+    """
+    result = yield {"method": method, "params": params}
+    return result
Evidence
PR Compliance ID 337802 requires explicit type annotations on new function signatures, including
return types. The added _command() definition has parameter annotations but omits the -> ...
return annotation.

Rule 337802: Require type annotations on new function and method signatures
py/selenium/webdriver/_bidi/transport.py[37-45]

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

## Issue description
`py/selenium/webdriver/_bidi/transport.py` introduces `_command()` without an explicit return type annotation.

## Issue Context
Compliance requires all new/modified Python function and method signatures to include parameter annotations and an explicit return type annotation. `_command()` is a generator-style coroutine used with `WebSocketConnection.execute`, so its signature should still declare a return type (e.g., a `Generator[...]` type, or at minimum `-> Any`).

## Fix Focus Areas
- py/selenium/webdriver/_bidi/transport.py[37-45]

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



Remediation recommended

7. Union outbound unvalidated ✓ Resolved 🐞 Bug ≡ Correctness
Description
_validate_outbound_scalar() returns for union-typed refs without validating the provided value, so
an object-only union field can serialize/send a scalar or a non-variant object without raising
BiDiSerializationError locally. This breaks the PR’s outbound boundary-validation contract and can
surface as remote protocol errors for caller mistakes.
Code

py/selenium/webdriver/common/_bidi/serialization.py[R487-500]

+def _validate_outbound_scalar(owner: str, name: str, w: _Wire, value: Any) -> None:
+    if value is None:
+        return  # null is handled upstream
+    if w.enum is not None:
+        return  # enum membership is validated at construction (__post_init__)
+    if w.ref is not None:
+        klass = resolve(w.ref)
+        # Records must be passed as instances; unions/enums stay permissive (a union arm may be a
+        # bare scalar or any variant, selected via Union.build rather than checked here).
+        if isinstance(klass, type) and issubclass(klass, Record) and not isinstance(value, klass):
+            raise BiDiSerializationError(
+                f"{owner}.{name}: expected {klass.__name__}, got {type(value).__name__} {value!r}"
+            )
+        return
Evidence
Outbound validation only enforces concrete Record subclasses for refs; Union refs return without
checks. But unions can be explicitly marked object-only by the generator and are enforced as
object-only on inbound parsing, as demonstrated by unit tests, so outbound should enforce the same
constraint at the wire boundary.

py/selenium/webdriver/common/_bidi/serialization.py[487-535]
py/generate_bidi_protocol.py[863-888]
py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[174-189]
py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[592-602]

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

## Issue description
Outbound validation currently skips validation for `ref` fields that resolve to a `Union` subclass. This allows invalid values (notably scalars for `_OBJECT_ONLY` unions, or objects that match no union arm) to be serialized and sent without a local `BiDiSerializationError`.

## Issue Context
Inbound parsing enforces `_OBJECT_ONLY` unions must be objects (`Union.from_json` raises on non-dicts), and the generator emits `_OBJECT_ONLY = True` for schema unions marked `objectOnly`. Outbound should mirror this boundary strictness.

## Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[487-507]
- py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[592-629]

## Suggested fix
1. In `_validate_outbound_scalar`, when `w.ref` resolves to a `Union` subclass:
  - If `klass._OBJECT_ONLY` is true, reject any non-object/non-variant value before serialization (at minimum: require a `Record` instance; optionally also accept a dict but validate it dispatches to a known variant).
  - Validate that `Record` instances provided for union-typed fields are one of the union’s allowed variant classes (derive the allowed set from `klass._VARIANTS.values()`, `klass._PRESENCE` refs, and optional `_FALLBACK`).
2. Add a unit test that constructs a `Record` with a `ref="test.ObjectOnly"` field and asserts `as_json()` raises when given a scalar (and optionally raises for a dict that does not dispatch to any arm).

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


8. Map ref not validated ✓ Resolved 🐞 Bug ≡ Correctness
Description
_validate_outbound_map_entry() accepts any Record as an object key/value, but never verifies
that the Record is a valid member/variant of the referenced type w.ref (often a union). This can
send structurally wrong objects on the wire (local validation passes) even though inbound map
parsing always validates object keys/values via resolve(w.ref).from_json(...).
Code

py/selenium/webdriver/common/_bidi/serialization.py[R439-458]

+def _validate_outbound_map_entry(owner: str, name: str, w: _Wire, element: Any) -> None:
+    """Validate one outbound ``[key, value]`` map entry: pair shape, key type, object-only value.
+
+    Mirrors the inbound :func:`_read_map_entry` checks: an object key is a typed record instance and
+    a bare-scalar key must match the arm's ``scalar`` primitive, while the value is always an object.
+    """
+    if not (isinstance(element, list) and len(element) == 2):
+        raise BiDiSerializationError(f"{owner}.{name}: expected a [key, value] pair, got {element!r}")
+    key, value = element
+    if not isinstance(key, Record):
+        scalars = [s for s in (w.scalar if isinstance(w.scalar, list) else [w.scalar]) if s is not None]
+        checks = [_PRIMITIVE_CHECKS[s] for s in scalars if s in _PRIMITIVE_CHECKS]
+        if checks and not any(check(key) for check in checks):
+            raise BiDiSerializationError(
+                f"{owner}.{name}: map key expected {' or '.join(scalars)}, got {type(key).__name__} {key!r}"
+            )
+    if not isinstance(value, Record):
+        raise BiDiSerializationError(
+            f"{owner}.{name}: map value expected an object, got {type(value).__name__} {value!r}"
+        )
Evidence
Outbound map validation only checks Record for object key/value and never consults w.ref, while
inbound map parsing resolves and validates object keys/values through the referenced type’s
from_json, so outbound can accept objects that inbound would reject as not being a valid union
variant/type.

py/selenium/webdriver/common/_bidi/serialization.py[429-435]
py/selenium/webdriver/common/_bidi/serialization.py[439-458]
py/selenium/webdriver/common/_bidi/serialization.py[354-367]

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

### Issue description
`_validate_outbound_map_entry()` currently validates map entries only for pair shape, scalar-key primitive type, and that object keys/values are `Record` instances. It does **not** validate that those `Record` instances are valid for the schema type referenced by `w.ref` (typically a `Union`), so unrelated `Record` types can be serialized and sent.

Inbound map decoding enforces `w.ref` by deserializing object keys and values through `klass = resolve(w.ref)` and `klass.from_json(...)`, so outbound should mirror that contract to prevent invalid frames from leaving the client.

### Issue Context
- Scalar keys are intentionally treated specially (they bypass `klass.from_json` and are checked via primitive guards). The missing validation is for **object keys** and **values**.
- Because `w.ref` may resolve to a `Union`, `isinstance(value, resolve(w.ref))` is not sufficient (union classes aren’t the instance type). A practical validation approach is to round-trip-validate: serialize the `Record` to JSON (`record.as_json()`) and ensure `resolve(w.ref).from_json(serialized)` succeeds.

### Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[439-459]
- py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[292-310]

### Suggested implementation sketch
1. In `_validate_outbound_map_entry`, resolve `klass = resolve(w.ref)`.
2. If `key` is a `Record`, validate it is accepted by `klass.from_json(key.as_json())` (catch `BiDiSerializationError` and re-raise with owner/name context).
3. For `value`, require `Record` (as today) **and** validate with `klass.from_json(value.as_json())`.
4. Add a unit test that `StringMap(value=[["k", Point(x=1, y=2)]])` (or another unrelated `Record`) is rejected during `as_json()` with a clear `BiDiSerializationError`.

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


9. resolve() hides import failures ✓ Resolved 🐞 Bug ☼ Reliability
Description
serialization.resolve() catches and ignores any ImportError while importing the owning domain
module, which can mask real import-time failures inside an existing module and then raise a
misleading "unknown BiDi type" error.
This makes genuine packaging/generated-module failures significantly harder to diagnose and can
misdirect error handling.
Code

py/selenium/webdriver/common/_bidi/serialization.py[R202-206]

+    module = _camel_to_snake(schema_name.split(".", 1)[0])
+    try:
+        import_module(f"{__package__}.{module}")
+    except ImportError:
+        pass
Evidence
The new lazy-import logic is implemented by importing _bidi.<domain> on a registry miss, but the
broad except ImportError: pass will also swallow ImportErrors raised from inside a real module,
leading to a later generic "unknown BiDi type" error. The unit test documents the intended
lazy-import behavior but does not protect against this masking.

py/selenium/webdriver/common/_bidi/serialization.py[194-210]
py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[211-223]

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

### Issue description
`resolve()` currently does:
- `import_module(...)`
- `except ImportError: pass`

This suppresses *all* import-time `ImportError`s, including those raised from inside an existing domain module during its execution. That can turn a real import failure into an unrelated `BiDiSerializationError("unknown BiDi type ...")`.

### Issue Context
`resolve()` is intended to lazily import a domain module *only when the target module is missing from the registry*, to run its `@register` decorators.

### Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[194-210]

### Suggested fix
Only suppress the specific “module not found” case for the computed module name, and re-raise other import failures. For example:
- Catch `ModuleNotFoundError` and check `e.name` matches the attempted module (or starts with it), then suppress.
- Otherwise `raise` to preserve the real failure cause.

Also consider logging/debugging context when the module truly does not exist (optional).

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


View more (9)
10. Unbounded undeclared-key message ✓ Resolved 🐞 Bug ➹ Performance
Description
Record.from_json() builds the warning/error text by joining repr() of every undeclared key, so a
reply containing many unknown properties can create a very large string and emit an excessively long
log line (or strict_inbound exception message). Since Transport.execute feeds websocket reply
payloads into result.from_json(), this diagnostic amplification is reachable from remote-controlled
data and can add avoidable allocation/logging overhead.
Code

py/selenium/webdriver/common/_bidi/serialization.py[R282-284]

+            noun = "property" if len(undeclared) == 1 else "properties"
+            names = ", ".join(repr(k) for k in undeclared)
+            _tolerate(f"{cls.__name__}: undeclared {noun} {names} ({'kept' if cls._EXTENSIBLE else 'dropped'})")
Evidence
The new code constructs an aggregated message by joining all undeclared keys, and _tolerate() will
either log or raise with that full string; Transport.execute() passes websocket reply payloads
into result.from_json(), making this reachable on inbound remote data.

py/selenium/webdriver/common/_bidi/serialization.py[276-287]
py/selenium/webdriver/common/_bidi/transport.py[52-58]

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

### Issue description
`Record.from_json()` formats all undeclared keys into a single warning/error message (`", ".join(repr(k) ...)`). For payloads with many unknown keys, this can allocate a huge string and produce oversized log/exception messages.

### Issue Context
Inbound payloads are derived from websocket replies via `Transport.execute(...). result.from_json(...)`, so message content/size can be driven by the remote endpoint.

### Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[277-284]

### Suggested fix
- Truncate the rendered key list (e.g., show first N keys) and append a summary like `(+X more)`.
- Optionally also cap by total rendered character length.
- Keep singular/plural grammar and kept/dropped suffix behavior.

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


11. Unbounded warning spam ✓ Resolved 🐞 Bug ☼ Reliability
Description
Record.from_json() emits a separate WARNING for every undeclared property via _tolerate(), so a
single inbound payload with many unknown keys can flood logs and add significant per-message
overhead. Since Transport.execute() deserializes websocket replies via result.from_json(...), this
warning amplification can be triggered repeatedly by a BiDi endpoint returning large/verbose objects
in tolerant mode.
Code

py/selenium/webdriver/common/_bidi/serialization.py[R276-281]

+        undeclared = [k for k in payload if k not in known]
+        for key in undeclared:
+            # A property the type does not declare is tolerated for forward-compatibility
+            # (ADR decision 2.3): warned, and kept only on an extensible (re-sendable) type
+            # so a caller can echo it back on the wire — otherwise dropped.
+            _tolerate(f"{cls.__name__}: undeclared property {key!r} ({'kept' if cls._EXTENSIBLE else 'dropped'})")
Evidence
The deserializer enumerates all undeclared keys and calls _tolerate() for each; in tolerant mode
_tolerate() logs a WARNING for each call. Transport deserializes inbound websocket replies through
result.from_json(...), so this warning pattern is reachable from remote protocol responses; unit
tests currently assert per-key warning visibility, so any aggregation/capping must adjust
expectations.

py/selenium/webdriver/common/_bidi/serialization.py[73-78]
py/selenium/webdriver/common/_bidi/serialization.py[276-283]
py/selenium/webdriver/common/_bidi/transport.py[52-58]
py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[229-236]
py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[382-403]

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

### Issue description
`Record.from_json()` warns once per undeclared property. With payloads containing many extra keys, this can flood logs and create avoidable CPU/I/O overhead in tolerant mode.

### Issue Context
This warning path is reached on inbound websocket replies because `Transport.execute()` calls `result.from_json(value)` for typed results.

### Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[73-78]
- py/selenium/webdriver/common/_bidi/serialization.py[276-283]
- py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[229-239]
- py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[382-403]

### Implementation notes
- Prefer emitting **one** warning per payload (e.g., include a bounded list of keys and a count of remaining keys), or cap warnings to a small maximum (e.g., first N keys) to prevent log amplification.
- Keep `strict_inbound()` behavior raising an error (possibly including the first offending key, plus counts) and update unit tests accordingly.

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


12. Transport mis-detects error replies ✓ Resolved 🐞 Bug ≡ Correctness
Description
Transport.execute() uses if reply.get("error"): which only triggers on truthy error values; an
error envelope with a falsy error value will fall through and then raise KeyError on
reply["result"]. This is inconsistent with WebSocketConnection.execute() which treats the
presence of the error key as an error (if "error" in response).
Code

py/selenium/webdriver/common/_bidi/transport.py[R52-58]

+    def execute(self, cmd: str, params: Any = None, result: Any = None) -> Any:
+        reply = self._connection.send_cmd(cmd, params.as_json() if params is not None else {})
+        if reply.get("error"):
+            message = reply.get("message")
+            raise WebDriverException(f"{reply['error']}: {message}" if message else reply["error"])
+        value = reply["result"]
+        return result.from_json(value) if result is not None else value
Evidence
Transport’s error detection is currently based on truthiness, while the websocket connection’s own
execute path treats any presence of the error key as an error; this mismatch means Transport can
miss some error envelopes and then crash on missing result.

py/selenium/webdriver/common/_bidi/transport.py[52-58]
py/selenium/webdriver/remote/websocket_connection.py[148-160]

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

## Issue description
`Transport.execute()` currently uses `if reply.get("error"):` to detect error replies. This can miss replies that include an `error` key whose value is falsy (e.g., empty string / None), and then it will attempt `reply["result"]`, raising a `KeyError` rather than a `WebDriverException`.

## Issue Context
The underlying websocket implementation uses presence-based semantics (`if "error" in response:`), so Transport should match that contract.

## Fix Focus Areas
- py/selenium/webdriver/common/_bidi/transport.py[52-58]
- py/selenium/webdriver/remote/websocket_connection.py[148-160]

### Suggested fix
Change the condition to `if "error" in reply:` (and ideally also guard for missing `result` with a clearer exception). Keep using `message = reply.get("message")` for formatting.

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


13. Bazel picks local generated files ✓ Resolved 🐞 Bug ☼ Reliability
Description
The bidi_protocol py_library globs selenium/webdriver/common/_bidi/*.py in the workspace in
addition to depending on the generated outputs from :create-bidi-protocol-src, so after running
the documented local generator/copy workflows it can see both workspace copies and Bazel-generated
copies of the same domain modules. This can cause Bazel analysis/build conflicts (competing
artifacts for the same module paths) or inconsistent packaging inputs for _bidi.
Code

py/BUILD.bazel[R786-790]

+py_library(
+    name = "bidi_protocol",
+    # Hand-written runtime from source + the generated domain modules from the rule.
+    srcs = [":create-bidi-protocol-src"] + glob(["selenium/webdriver/common/_bidi/*.py"]),
+    imports = ["."],
Evidence
The build rule currently adds workspace _bidi/*.py files via glob, while the generator defaults to
writing generated domain modules into that same workspace directory under bazel run (and local_dev
copies _bidi into the tree). This makes it possible for the same domain module filenames to appear
both as workspace sources and as Bazel-generated outputs for :create-bidi-protocol-src.

py/BUILD.bazel[755-793]
py/generate_bidi_protocol.py[45-48]
py/generate_bidi_protocol.py[1051-1072]
rake_tasks/python.rake[72-90]

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

## Issue description
`py/BUILD.bazel` defines `:bidi_protocol` as `srcs = [":create-bidi-protocol-src"] + glob(["selenium/webdriver/common/_bidi/*.py"])`. The generator’s `bazel run //py:generate-bidi-protocol` default writes generated domain modules into the *workspace* under `py/selenium/webdriver/common/_bidi/`, and `rake local_dev` also copies `common/_bidi` into the source tree. Once those files exist locally, the glob will start including generated domain modules from the workspace *in addition to* the Bazel-generated outputs, creating conflicting/duplicated artifacts for the same import paths.

## Issue Context
The intention (per comments) is: runtime files are checked in, domain modules are generated by Bazel. Globbing `*.py` in that directory defeats this separation once local generation is performed.

## Fix Focus Areas
- py/BUILD.bazel[755-793]
- py/generate_bidi_protocol.py[1051-1072]
- rake_tasks/python.rake[72-90]

### Suggested fix
Replace the glob with an explicit list of the four hand-written runtime files (and keep `:create-bidi-protocol-src` for generated domain modules), e.g.:
- `srcs = [":create-bidi-protocol-src", "selenium/webdriver/common/_bidi/__init__.py", "selenium/webdriver/common/_bidi/domain.py", "selenium/webdriver/common/_bidi/serialization.py", "selenium/webdriver/common/_bidi/transport.py"]`
This prevents locally generated domain modules from being picked up as additional sources.

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


14. No outbound primitive/list validation ✓ Resolved 🐞 Bug ≡ Correctness
Description
Record.__post_init__() validates nullability and enums only, so wrong primitive types,
list-vs-scalar mismatches, and invalid nested ref values can be serialized by as_json() and sent
on the wire unchanged. This allows constructing protocol objects that violate the schema and only
fail later as remote/protocol errors.
Code

py/selenium/webdriver/common/_bidi/serialization.py[R178-194]

+    def __post_init__(self) -> None:
+        for f in _fields(self):
+            if f.name == "extensions":
+                continue
+            w = _wire_of(f)
+            if w.fixed is not UNSET:
+                continue
+            value = getattr(self, f.name)
+            if value is UNSET:
+                continue
+            if value is None:
+                if not w.nullable:
+                    raise BiDiSerializationError(f"{type(self).__name__}.{f.name} cannot be None")
+                continue
+            if w.enum is not None:
+                self._validate_enum(f.name, w, value)
+
Evidence
Record.__post_init__ only checks nullability and enums; it does not use
primitive/is_list/ref metadata for outbound validation. Meanwhile inbound
_read_field/_read_scalar do enforce list shape and primitive types using _PRIMITIVE_CHECKS,
demonstrating the intended strictness is currently one-sided.

py/selenium/webdriver/common/_bidi/serialization.py[178-194]
py/selenium/webdriver/common/_bidi/serialization.py[238-255]
py/selenium/webdriver/common/_bidi/serialization.py[289-329]

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 outbound construction path for generated Records does not validate primitives (`w.primitive`), list shape (`w.is_list`), or referenced nested types (`w.ref`). As a result, invalid values can be serialized and sent to the browser without any local `BiDiSerializationError`.

### Issue Context
Inbound parsing is strict (checks list-ness and primitive types), but outbound construction is not, despite having the schema facts (`primitive`, `is_list`, `ref`) and `_PRIMITIVE_CHECKS` available.

### Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[178-205]
- py/selenium/webdriver/common/_bidi/serialization.py[238-255]
- py/selenium/webdriver/common/_bidi/serialization.py[289-329]
- py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py[1-453]

### Suggested fix
1. Extend `Record.__post_init__` to validate, for each non-UNSET/non-None field:
  - If `w.is_list` is True, assert `isinstance(value, list)`.
  - If `w.is_list` is False, assert `not isinstance(value, list)` when the field is typed (`primitive`/`enum`/`ref`) to avoid emitting arrays accidentally.
  - If `w.primitive` is set, validate using `_PRIMITIVE_CHECKS[w.primitive]` (and for list fields validate each element).
  - If `w.ref` is set, require values to be `Record` instances (or list of them) rather than arbitrary dict/scalars (unless explicitly intended).
2. Add unit tests showing that constructing a Record with:
  - a wrong primitive (e.g., `int` field passed a `str`), and
  - a list field passed a scalar,
  raises `BiDiSerializationError` before serialization.

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


15. Transport.execute() error branch untested ✓ Resolved 📘 Rule violation ☼ Reliability
Description
Transport.execute() introduces new error-handling logic when the websocket reply contains error,
but the added unit tests only cover successful result replies. This risks regressions in the
failure path going unnoticed and weakens confidence in the new transport seam’s correctness.
Code

py/selenium/webdriver/common/_bidi/transport.py[R52-56]

+    def execute(self, cmd: str, params: Any = None, result: Any = None) -> Any:
+        reply = self._connection.send_cmd(cmd, params.as_json() if params is not None else {})
+        if reply.get("error"):
+            message = reply.get("message")
+            raise WebDriverException(f"{reply['error']}: {message}" if message else reply["error"])
Evidence
PR Compliance ID 5 requires new behavior to be covered by automated tests. The new
Transport.execute() error-raising logic is present in the transport implementation, while the
added unit tests’ stand-in connection only returns {"result": ...} and therefore never exercises
the error path.

AGENTS.md: Add/Prefer Automated Tests; Prefer Small Unit Tests and Avoid Mocks
py/selenium/webdriver/common/_bidi/transport.py[52-56]
py/test/unit/selenium/webdriver/common/bidi_transport_tests.py[49-107]

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

## Issue description
`Transport.execute()` has an error-handling branch (when the reply contains `error`) that is not covered by the new unit tests.

## Issue Context
The new internal BiDi transport seam is a core piece that will be shared by generated domains; its error path should be unit-tested to prevent silent regressions.

## Fix Focus Areas
- py/selenium/webdriver/common/_bidi/transport.py[52-56]
- py/test/unit/selenium/webdriver/common/bidi_transport_tests.py[49-107]

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


16. Extensions overwrite known fields ✗ Dismissed 🐞 Bug ≡ Correctness
Description
Record.as_json() merges extensions into the serialized payload via payload.update(...), so a
caller-supplied extension key can overwrite a known wire field and silently change what is sent on
the wire.
Code

py/selenium/webdriver/common/_bidi/serialization.py[R207-221]

+    def as_json(self) -> dict:
+        payload: dict = {}
+        for f in _fields(self):
+            if f.name == "extensions":
+                continue
+            w = _wire_of(f)
+            value = getattr(self, f.name)
+            if value is UNSET:
+                continue
+            if value is None and not w.nullable:
+                continue
+            payload[w.wire] = _as_json(value)
+        if self._EXTENSIBLE:
+            payload.update(getattr(self, "extensions", None) or {})
+        return payload
Evidence
The serialization code writes known wire keys first, then unconditionally updates with extensions,
which gives extensions precedence over declared fields.

py/selenium/webdriver/common/_bidi/serialization.py[207-235]

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

### Issue description
For extensible records, `Record.as_json()` does `payload.update(extensions)`. If `extensions` contains a key that matches a known wire field, it overwrites the serialized value from the actual dataclass field, producing an incorrect wire payload.

### Issue Context
`from_json()` only captures unknown keys into `extensions`, so round-trips won’t collide. But callers can still construct a record with arbitrary `extensions` and accidentally (or intentionally) override real fields.

### Fix Focus Areas
- py/selenium/webdriver/common/_bidi/serialization.py[207-221]

### Suggested fix approach
In `Record.as_json()` when `_EXTENSIBLE`:
- compute `extras = getattr(self, "extensions", None) or {}`
- if `set(extras) & set(payload)` is non-empty, raise `BiDiSerializationError` identifying the colliding keys (or, at minimum, ensure declared fields win by merging as `{**extras, **payload}` so extras cannot override).

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


17. send_cmd() lacks direct tests 📘 Rule violation ☼ Reliability
Description
WebSocketConnection.send_cmd() is newly added but no unit test exercises the real
WebSocketConnection send/reply path; existing BiDi tests use stand-in connections instead. This
increases the risk of regressions in the core BiDi command transport path going undetected.
Code

py/selenium/webdriver/remote/websocket_connection.py[R140-146]

+    def send_cmd(self, method, params):
+        """Send one command and return its raw reply (``result`` or ``error`` left intact).
+
+        The dumb-send counterpart to ``execute``: no coroutine, no error raising. The
+        caller parses the reply into its declared type and raises on ``error`` itself.
+        """
+        return self._round_trip({"method": method, "params": params})
Evidence
PR Compliance ID 3 requires new behavior to be covered by tests. The PR adds
WebSocketConnection.send_cmd() (used by the new BiDi transport), while the added tests only use
stand-in DrivingConnection.send_cmd() implementations, so the real
WebSocketConnection.send_cmd() path is not exercised by unit tests.

AGENTS.md: Add Tests for Fixes/Features and Prefer Small Unit Tests Over Browser Tests
py/selenium/webdriver/remote/websocket_connection.py[118-160]
py/test/unit/selenium/webdriver/common/bidi_transport_tests.py[49-63]
py/test/unit/selenium/webdriver/common/bidi_protocol_command_tests.py[44-58]

Agent prompt

[Comment truncated to fit github's 65,536-char limit.]

Comment thread py/selenium/webdriver/remote/websocket_connection.py
Comment thread py/private/generate_bidi_protocol.bzl
Comment thread py/selenium/webdriver/common/_bidi/serialization.py
Comment thread py/generate_bidi_protocol.py
Comment thread py/selenium/webdriver/common/_bidi/serialization.py Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 23d0038

Comment thread py/selenium/webdriver/common/_bidi/serialization.py Outdated
Comment thread py/test/unit/selenium/webdriver/common/bidi_serialization_tests.py Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 17fa362

Comment thread py/selenium/webdriver/common/_bidi/serialization.py Outdated
Comment thread py/selenium/webdriver/common/_bidi/serialization.py Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

Comment thread py/selenium/webdriver/common/_bidi/serialization.py
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

Comment thread py/selenium/webdriver/common/_bidi/serialization.py
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

Comment thread py/selenium/webdriver/common/_bidi/serialization.py
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 39c38a2

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 1d705ae

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants