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
@AutomatedTester

Copy link
Copy Markdown
Member Author

Thanks Titus — that's useful, and it changes what I think this PR should be.

On the tracking issue: understood, and agreed that #17761 / #17942 / #17966 / #17952 built the layer. I worked from the unchecked box, so this PR is framed as "implement the row" when it should be framed as "close four specific gaps in the row." Concretely, what's here beyond those four PRs is:

  1. Events never reached their typed payload — the generated domains emit EVENTS/EVENT_TYPES and nothing consumed them; Transport.execute only deserialized command results.
  2. as_json sent 5.0 on an integer field while _read_scalar normalized inbound to int — asymmetric against decision 4.
  3. The extras-shadowing rule (decision 1: "a key the type declares must never appear in that map") was only enforced at serialization, not construction.
  4. Nothing checked the contract across the whole schema, so generator drift would land silently. The sweep is 2200 assertions over 300+ types.

2–4 are small and I don't think they're contentious. 1 is the one your scope point lands on.

On scope: I think you're half right, and the half matters. The ADR puts "event subscription and routing" out of scope — agreed, and on()/off() are exactly that. But decision 1 requires "parameters, results, and event payloads" to be typed objects, and the Inbound section (7–10) governs any inbound payload, not just command results. Today Python satisfies those for results and not for events, which is the gap I was aiming at.

The tension is that a typed event payload with no seam at all is unobservable — it can't be demonstrated or tested. So the options as I see them:

  • (a) Keep Event/Event.from_json (typed payload, decision 1) and drop on()/off() (registration, orchestration). The descriptor already matches the connection's existing add_callback contract, so a caller can wire it without the layer owning dispatch. Small change, and it draws the line exactly where the ADR does.
  • (b) Drop the whole seam here and let the orchestration work bring it.

I lean (a), but it's your call on where the boundary sits. Worth noting for your dig into the remaining events: Ruby has the same gaprb/lib/selenium/webdriver/bidi/protocol/*.rb emits EVENT_TYPES with no typed-event consumer either. Whatever we decide here, it'd be good if both bindings landed one seam rather than two.

On the transition: yes, let's have that chat, and I'll treat it as blocking. The four module migrations in this PR (permissions, webExtension, browsingContext, browser.setDownloadBehavior) are internal swaps behind unchanged public signatures, but they are "extending the current implementation" in the sense you mean, and I'd rather not accumulate more of that before we've agreed the destination.

One thing I found while doing them that's probably input to that conversation: the manifest's hand-written methods are the easy part — the bulk of the user-facing surface is the generated command methods. I'd assumed those were blocked on the two generators reading different sources, but they aren't: py:create-bidi-src and javascript/selenium-webdriver:create-bidi-src_schema declare the same CDDL inputs, and the JS one adds the #1140 override, the Firefox vendor fields and spec anchors on top. The names already line up (script 70/70 classes, input 21/21, identical wire method strings), and _bidi records the canonical spec name in @register(...), so the mapping key exists on both sides today.

What differs is fidelity, all in one direction: 491 of 1226 facade fields are typed Any (40%) against 2 of 1202 in _bidi, unions are collapsed rather than variant-typed, and the CDDL parser is hand-rolled regex where the JS pipeline is cddl2ts → normalize → project with ~1,700 lines of test behind it. Concretely: bluetooth.characteristicEventGenerated and descriptorEventGenerated aren't defined in the pinned Web Bluetooth CDDL at all. The JS pipeline allowlists them as KNOWN_INCOMPLETE with a staleness check that fails once upstream defines them; the CDDL generator emits them as commands and ships them — command_builder("bluetooth.characteristicEventGenerated", params) is in selenium.webdriver.common.bidi.bluetooth today.

So I don't think this needs a decision about converging generators — it's "retire the CDDL parser, point the facade generator at schema.json." Still a big lift (1,711-line generator, 2,400-line manifest), but a mechanical one with a known destination, which might be a useful part of the answer to "what all needs to be done."

Proposal: I'll split this into the contract fixes (2–4 above, plus whichever event option you pick) and hold the module migrations until after we talk. Happy to do that now if you agree.

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