[adr] Behavioral contract for the low-level WebDriver BiDi layer - #17786
[adr] Behavioral contract for the low-level WebDriver BiDi layer#17786titusfortner wants to merge 15 commits into
Conversation
PR Summary by QodoAdd ADR defining behavioral contract for low-level WebDriver BiDi layer
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
|
Code review by qodo was updated up to the latest commit 98e1858 |
There was a problem hiding this comment.
Pull request overview
Adds a new ADR to the cross-binding decision log that defines a behavioral contract for Selenium’s internal, low-level WebDriver BiDi protocol layer (validation, extensibility, parsing strictness, and surface shape), intended to keep binding implementations aligned on wire-observable behavior.
Changes:
- Introduces a decision record enumerating required outbound/inbound behaviors for the BiDi low-level layer.
- Defines a two-tier taxonomy (Compliance vs Decision) to separate spec-forced requirements from Selenium-standardized choices.
- Records rationale, alternatives, and consequences to support consistent adoption across bindings.
|
Code review by qodo was updated up to the latest commit dfe1c07 |
|
Code review by qodo was updated up to the latest commit 5b77f31 |
|
Code review by qodo was updated up to the latest commit e965829 |
|
Code review by qodo was updated up to the latest commit 1b0e91a |
…uire only that malformed input raises
Code Review by Qodo
1.
|
… split, relaxation scope, consistency pass
|
Code review by qodo was updated up to the latest commit 541031b |
…n, inbound validation & retention, typed surface
|
Code review by qodo was updated up to the latest commit deb870f |
|
I just pushed a substantial rewrite of both the ADR and updated the PR description to match it. The main behavioral change is inbound required-field absence: the proposal now leaves a missing required inbound field omitted and warns, rather than rejecting the whole message by default. That will likely end up being the main policy question for review: should Selenium tolerate that incorrectness in order to be more flexible to browser/schema drift. The ADR is now framed around representability: outbound stays strict because Selenium controls what it sends; inbound tolerates only departures that can be represented without inventing data; corrupt present values and unknown closed-vocabulary members still error. The implementation comparison in the PR body is there to show which current differences are implementation cost/adoption gaps and which ones are real policy choices. |
…validity floor, numeric boundary, event params, union resolution)
|
Code review by qodo was updated up to the latest commit fdb7525 |
…ound, and inbound
…le as an undeclared discriminator
|
Reorganized and clarified since the last push — the four-decision record (D1–D4) is now three sections (Representation, Outbound, Inbound) with ten numbered decisions, so a fresh read top to bottom helps. Substantive changes:
|
|
Code review by qodo was updated up to the latest commit 2efdaa4 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/decisions/17786-bidi-low-level-behavioral-contract.md:52
- The
integerdefinition mixes value semantics (“fractional”) with an ambiguous “float-encoded” requirement, and the5.0example depends on the JSON number token’s lexical form (which many JSON parsers discard). To make this contract implementable and testable across bindings, it should define “integer” acceptance in terms of the JSON number form (no decimal point / exponent) if lexical strictness is intended.
is expected. A primitive matches by JSON kind, not language representation: `number` admits any JSON
number, while `integer` rejects fractional or float-encoded values (including a whole-valued `5.0`).
docs/decisions/17786-bidi-low-level-behavioral-contract.md:44
- Decision 3’s numeric-range description conflates
js-intandjs-uintby using a single “±” range.js-uintis non-negative, so this phrasing can be read as allowing negative values for unsigned fields, and it also introduces a Unicode minus that’s inconsistent with the rest of the repo’s ASCII math formatting.
3. **Preserve a numeric value's full range and precision.** The native type chosen for a numeric value must
cover the full range the spec declares for it, with no narrowing or lossy conversion. BiDi integers stay
within the JS safe-integer range (`js-int`/`js-uint`, `±(2^53 − 1)`): too wide for a 32-bit integer,
though a 64-bit integer or a double holds them exactly. A field with a narrower declared range may use a
…e rule, clarify null-vs-omitted and error-response
|
Code review by qodo was updated up to the latest commit 96e22f0 |
📄 The decision, rationale, considered options, and consequences are in the record file this PR adds;
read it there. Below are proposal notes and review logistics.
🔗 Related
📝 Proposal notes
shared schema makes some behavior cheap to derive, but the contract is still the behavior every binding
exhibits, regardless of how it is implemented.
messages back into typed objects. Transport behavior is out of scope and should be decided separately if
needed.
ADR, showing where each implementation currently stands. The summaries after the table
differentiate the reasons for divergences (marked for replacement, minimizing maintenance costs, or
intentional design choices).
most of the cost versus handwriting: closed-type rejection (6a), null and type checks (7a/7b), typed value
objects (1). The exception is per-type extensibility in static languages: carrying and retaining undeclared
fields on extensible types (6b/9b) stays costly even generated, because preserving and re-serializing them
requires a separate deserialization path (though the webdriverbidi-net project shows it is possible even
when handwritten, despite the cost). Two valid alternatives to the ADR positions are:
implementations).
tolerate what can be represented without inventing data; reject only what would produce a false typed
object (a corrupt value or an unknown closed-union variant, 7a/7b/7c). That leaves the two ways a browser
drifts from the binding's schema, both of which the ADR tolerates: extra fields a newer browser adds
(9a/9b) and required fields an older browser omits (8a/8b). Two valid alternatives are to tighten one side:
(the layer breaks against a newer browser).
compatibility (the layer breaks against an older browser).
📊 Implementation comparison
How each implementation (current, proposed, and external) stands on selected comparison points against the ADR
The record states ten decisions across three sections (Representation, Outbound, Inbound). The table groups
its comparison points the same way: a row's number is the decision it tests, with letters marking multiple
points under one decision.
Per-implementation through-lines
time, while inbound is whatever falls out of casting the payload onto the object, with no runtime checks,
so 7a/7b/7c are ❌ and 8a passes only incidentally.
closed types reject extras, extensible types carry vendor extras, and malformed inbound values error.
Its 8a/8b divergence is intentional since strict inbound conformance is the more correct behavior,
but means each browser bug must be handled by shipping a manual, temporary relaxation until the browser
is fixed.
bag on the top-level payload (command params, result, event args), not on the nested types, so a per-type
extra has nowhere to attach, and missing fields get defaults instead of being tracked as absent. Those
choices avoid expensive per-type work, but they are lossy, which is why it diverges on things
that are costly by hand and cheap to generate.
fields on extensible types (9b) and sending them back (6b), which the PR currently defers. We may need to
sketch out how Java could support this before agreeing to these decisions in the ADR.
Notes
① Cheaper for static implementations: the type system gives it with little extra code, even
un-generated.
② Cheaper for dynamic implementations: a plain runtime value does the job, with no static wrapper or
typed map.
③ Narrows a small, bounded
js-uintfield (a status code, a line or column number) to a 32-bitint; thegenuinely wide fields (byte sizes, counts) stay
long, so the miss is limited to these small slots.④ Selenium's shared
NumberCoerceris too lenient to enforce this (it truncates fractional integers andaccepts float- or string-encoded numbers); the BiDi layer likely needs a stricter, BiDi-scoped coercer.
⑤ Returns
nullfor an unknownRemoteValuetype instead of erroring, so an undeclared closed-union variantis not rejected.
⑥ Performs no runtime union dispatch, so an undeclared closed-union variant is not rejected.
⑦ Compliant, but does not log the warning the ADR requires here.
⑧ Errors on a missing required field, so the whole message is rejected.
⑨ Fills a missing required field with a default (
0/null/false) a caller can't distinguish from a realvalue, so downstream code acts on fabricated data with no signal the field never arrived.
⑩ No implementation fails 8b independently: one that does not tolerate a missing required field (8a) never
reaches an omitted state to keep distinct from
null.🗣 Discussion
Notes from Slack and TLC minutes are recorded here as the proposal is discussed.
📌 Tracking
Tracking issue: (linked on acceptance)