Skip to content

[py] conform the internal BiDi layer to ADR 17786 and move four modules onto it - #18052

Open
AutomatedTester wants to merge 1 commit into
trunkfrom
py-bidi-low-level-contract
Open

AutomatedTester wants to merge 1 commit into
trunkfrom
py-bidi-low-level-contract

Conversation

@AutomatedTester

Copy link
Copy Markdown
Member

User description

Implements ADR 17786 — the behavioral contract for the low-level WebDriver BiDi layer — for Python, and starts moving the supported modules onto the layer that satisfies it. Tracking issue: #18020.

The generated _bidi layer already met most of the contract for command results (#17761, #17942, #17966, #17952). This closes the remaining gaps and puts four supported modules on it.

Contract gaps closed

Events never reached their typed payload. Every generated domain emitted EVENTS and EVENT_TYPES tables that nothing consumed — Transport.execute only deserialized command results, so decisions 1 and 7–10 held for half the inbound surface and not the other half. Domain gains event() / on() / off() and an Event descriptor shaped for the connection's existing add_callback contract. A deserialization failure is logged as well as raised, because callbacks run on a daemon thread where a raise would otherwise be invisible.

on() deliberately does not subscribe — that's orchestration, which the ADR puts out of scope. Callers still use session.subscribe; this governs only how what arrives is typed.

Outbound integers weren't narrowed. _read_scalar normalized a whole float inbound; as_json sent 5.0. A whole float is a valid integer either way (decision 4), but it now reaches the wire as one.

The extras-shadowing rule fired late. Decision 1 says a declared key must never appear in the extras map — an invariant of the representation, so it's now checked at construction. as_json keeps checking, because a frozen record can still be mutated through object.__setattr__.

Nothing checked the contract across the whole schema. A new sweep walks every generated type from the registry and asserts it mechanically: wire metadata, _EXTENSIBLE consistency, union dispatch, required fields, undeclared fields, round-trip stability. 2200 assertions over 300+ types. I mutation-tested it — deleting the undeclared-field warning turns 253 of them red.

Supported modules moved onto the layer

permissions, webExtension, browsingContext and browser.setDownloadBehavior now build their wire frames with the generated types instead of by hand. Public signatures, documented exceptions and return shapes are unchanged, with one exception noted below.

Two of these had been hand-rolling what the layer models directly:

  • browsingContext.setViewport used an ... sentinel so an explicit None serialized but an omitted argument didn't — that is exactly the omitted-vs-null distinction the contract's baseline provides.
  • browser.setDownloadBehavior carried the comment "downloadBehavior is a REQUIRED field in the BiDi spec (can be null but must be present). Do NOT use a generic None-filter on it" — a field opted out of the facade's blanket None-stripping by hand.

generate_bidi.py gains an extra_imports manifest hook, and now only imports command_builder when something still uses it.

Behavior change

permissions.set_permission rejects a missing origin locally instead of sending an incomplete frame for the remote end to reject. origin is required by the spec; the signature had it optional. Same outcome, without the round trip, per decision 5.

Not in this PR

  • browser.setClientWindowState — its state argument folds the spec's named-state and rect-state variants together and adds a "normal" the spec's enum doesn't have. It also appears to send the rect state as state rather than as sibling width/height/x/y, which looks wrong against the schema; I'd rather check that on its own than move a possible bug.
  • storage (3 methods) and emulation (7) — same mechanical pattern, not yet done. session and input have no hand-written frames at all.
  • The generated command methods, which are the bulk of the user-facing surface. Those are emitted by CddlCommand.to_python_method and apply extract_field/deserialize rules to a raw result dict — the inbound path where users are most exposed. Delegating them generically is blocked on the two generators being independent projections (generate_bidi.py reads CDDL; generate_bidi_protocol.py reads the JS-projected schema), so there's no shared identity to map a facade type onto its _bidi counterpart. Closing that likely means pointing the CDDL generator at the same projected schema, which wants its own decision.

Testing

Gate Result
//py:unit 38/38 pass
//py:test-chrome-bidi pass
//py:mypy clean, 108 files
./scripts/format.sh --pre-push clean

New unit coverage: 15 event-dispatch tests, 2200 conformance assertions, and per-module tests for permissions, webExtension, browsingContext and browser. protocol_tests.py gains two real-browser event round-trips — a Chrome-pushed log.entryAdded arriving as ConsoleLogEntry with its enum restored, its js-uint timestamp intact past 2^31, and its nested script.RemoteValue args dispatched to their variants.

One flaky test surfaced during verification and is not related to this change: test_activate_browsing_context (a window-focus test) failed then passed on two consecutive identical runs, in a file this PR doesn't touch. Worth a separate look.

Cross-binding

The event gap is not Python-specific — Ruby emits EVENTS/EVENT_TYPES in rb/lib/selenium/webdriver/bidi/protocol/*.rb with no typed-event consumer either. Worth agreeing one event seam across bindings rather than inventing two. The conformance sweep is a pattern the other bindings could copy, since the schema is shared.

AI assistance disclosure

Per CONTRIBUTING.md: substantial parts of this PR were written with Claude Code (Claude Opus). It produced the Event dispatch seam, the conformance sweep, the manifest migrations and the tests, working from the ADR and the existing layer. I've reviewed the result and own it. Opened as a draft for that reason — happy to split it (the _bidi contract work and the module migrations are separable) if that reads better for review.

Co-Authored-By: Copse noreply@copse.dev
Copse-Models: acp:claude-agent-acp#opus[1m]

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Sep 18, 2026
@titusfortner

Copy link
Copy Markdown
Member

Sorry, yes, I created the generic tracking issue with #18020 but then didn't update it to reflect current state. The Python row is already completely covered by work done in #17761, #17942, #17966, #17952

#17786 scopes event subscription and routing to a separate orchestration layer, which should(?) be language-specific implementation details that aren't surfaced directly to the user. So, I don't think there's anything put into another ADR for that layer beyond the kind of things we're specifying for Script & Network. Actually, let me dig into what additional behavior we need to define for the other events we haven't discussed, yet to see if there's something more here.

I brought it up in the last TLC meeting and you and I probably should have a chat about how we're going to transition from the .bidi implementation to the ._bidi implementation, and move things off of the driver object per #17670. I don't think we want to continue extending the current implementation, but I don't have a clear idea of what all needs to be done for the transition to work the way we need it to.

@AutomatedTester
AutomatedTester marked this pull request as ready for review September 19, 2026 06:58
…es onto it

ADR 17786 fixes the behavior the low-level BiDi layer must exhibit at the wire
boundary. The generated `_bidi` layer already met most of it for command
results; this closes the gaps and starts moving the supported modules onto it.

Events now reach their generated payload type. Every domain emitted EVENTS and
EVENT_TYPES tables that nothing consumed, so decisions 1 and 7-10 held for
command results and not for the other half of the inbound surface. Domain gains
event()/on()/off() and an Event descriptor shaped for the connection's existing
add_callback contract. Deserialization failures are logged as well as raised,
because callbacks run on a daemon thread where a raise would otherwise be
invisible.

Outbound integers are narrowed the way inbound ones already are: a whole float
is a valid integer either way, but it now reaches the wire as an integer rather
than as 5.0.

The rule that an extras key may not shadow a declared field is enforced at
construction rather than only at serialization, since it is an invariant of the
representation. as_json keeps checking, because a frozen record can still be
mutated through object.__setattr__.

A new conformance sweep walks every generated type from the registry and checks
the contract mechanically -- wire metadata, extensibility, union dispatch,
required fields, undeclared fields, round-trip stability. It is what makes
generator drift loud rather than silent.

permissions, webExtension, browsingContext and browser.setDownloadBehavior now
build their wire frames with the generated types instead of by hand. Public
signatures, documented exceptions and return shapes are unchanged, except that
permissions.set_permission now rejects a missing origin locally rather than
letting the remote end reject it. Two of these had been hand-rolling what the
layer models directly: setViewport's `...` sentinel is the omitted-vs-null
distinction, and setDownloadBehavior carried a field opted out of the facade's
blanket None-stripping.

generate_bidi.py gains an extra_imports manifest hook, and only imports
command_builder when something still uses it.

Refs #18020
@AutomatedTester
AutomatedTester force-pushed the py-bidi-low-level-contract branch from ec35e41 to 3dbb0ec Compare September 19, 2026 11:55

This branch has not been deployed

No deployments
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 B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants