[adr] Behavioral contract for the low-level WebDriver BiDi layer#17786
Open
titusfortner wants to merge 7 commits into
Open
[adr] Behavioral contract for the low-level WebDriver BiDi layer#17786titusfortner wants to merge 7 commits into
titusfortner wants to merge 7 commits into
Conversation
Contributor
PR Summary by QodoAdd ADR defining behavioral contract for low-level WebDriver BiDi layer
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
|
Code review by qodo was updated up to the latest commit 98e1858 |
3 tasks
Contributor
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.
Contributor
|
Code review by qodo was updated up to the latest commit dfe1c07 |
Contributor
|
Code review by qodo was updated up to the latest commit 5b77f31 |
3 tasks
Contributor
|
Code review by qodo was updated up to the latest commit e965829 |
Contributor
|
Code review by qodo was updated up to the latest commit 1b0e91a |
3 tasks
…uire only that malformed input raises
Contributor
Code Review by Qodo
1. Undefined error-code surfacing
|
| misrepresenting the response, and it is cheaper to loosen a strict contract than to tighten a lenient | ||
| one. When a real remote end is seen sending off-spec data, the specific field or command is relaxed | ||
| through a documented, reversible change (citing the offending implementation and the spec issue), | ||
| applied to every binding at once — never one alone, and never by starting lenient. One exception is |
Contributor
There was a problem hiding this comment.
1. Undefined error-code surfacing 🐞 Bug ⚙ Maintainability
Item 9 requires unknown ErrorResponse enum tokens to be “surfaced, not thrown” but does not define the minimum observable semantics (e.g., raw-token preservation and how callers access it), which can lead to divergent binding behavior despite the ADR’s stated goal of uniform wire-observable behavior.
Agent Prompt
### Issue description
Item 9 says an unknown `ErrorResponse` error-code token must be “surfaced, not thrown”, but that leaves implementers room to diverge on what is required (whether the raw token is preserved verbatim, and what shape it takes when exposed to callers).
### Issue Context
This ADR is explicitly intended to prevent cross-binding drift on wire-observable behavior; underspecified requirements defeat that goal.
### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[122-130]
### Suggested change (one possible wording)
Amend item 9 to define the binding-neutral minimum:
- parsing must not throw on unknown `ErrorResponse` code tokens
- the exact raw wire token must be preserved verbatim
- callers must be able to retrieve that raw token (via an idiomatic “unknown/other” representation)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| - The contract holds for any implementation regardless of language or production method; conformance is | ||
| checkable independently of how the layer was built. | ||
| - The per-type signals items 5/6/8 need are all derivable from the spec; a binding that discards one, or |
Contributor
There was a problem hiding this comment.
2. Broken consequences sentence 🐞 Bug ⚙ Maintainability
The final Consequences bullet ends with an ungrammatical fragment (“the items that signal feeds”), which reduces clarity for future readers quoting or implementing the summary.
Agent Prompt
### Issue description
The last sentence in Consequences is grammatically incorrect (“the items that signal feeds”), making the summary unclear.
### Issue Context
This is the concluding summary statement and is likely to be quoted in reviews.
### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[189-190]
### Suggested change
Rewrite to something like:
- “...falls out of conformance on exactly the items those signals feed.”
or
- “...falls out of conformance on exactly items 5, 6, or 8.”
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📄 The decision, its rationale, considered options, and consequences are in the record file
this PR adds; read it there. The sections below are proposal notes and review logistics.
🔗 Related
binding-neutral schema ([js] Add binding-neutral BiDi schema with cddl2ts-gated fidelity #17700).
📝 Proposal notes
and drifts on wire-observable behavior without a shared contract. This record fixes that behavior
once, so every binding is held to the same bar and a review can cite one place instead of re-deciding.
(generation is one way to satisfy the contract, not a requirement), the checked-in-vs-build-time
generation choice, and per-binding adoption. The record fixes behavior only.
Decisions this record owns — a strict, typed, validated design the spec permits but does not compel, so
review should focus there. Exactly one Decision turns on the internal-implementation premise: scoping
preserved extras tightly (item 8), affordable because an internal layer can widen later at no cost. The
rest — local outbound validation (3), strict-by-default inbound with reactive cross-binding relaxation
(6, 9), extensibility off the spec's per-type signal (4), typed and read-only objects (11, 12),
spec-mirroring names (10) — stand on their own merits, not on the premise.
📊 Conformance snapshot (point-in-time)
Measured against seven existing implementations of this layer. Columns — independent:
net,wdio.Selenium on trunk:
rb(generated, merged in #17731),py trunk,.NET(hand-written). In-flight PRs:py 17761(#17761),java 17777(#17777). A snapshot only — it changesas bindings converge, and moves to the tracking issue on acceptance.
Legend: ✅ meets it · ❌ does not (includes partial) · ①–② diverges by a deliberate design choice
(numbered below), not a defect.
net = webdriverbidi-net · wdio = WebdriverIO
① Lenient inbound parsing (wdio). WebdriverIO tolerates off-spec responses rather than raising —
reasonable for a public library facing a churning spec. This record raises instead, on merit rather than
because it is internal: a clear error beats silently misrepresenting the response. Note that net, also
public, is strict here (✅ on 5, 6, 9) — so strictness is a design choice, not a public-vs-internal one.
② Broad extras handling (net, wdio). Both public implementations keep unknown fields broadly, so they
never silently drop a vendor field a user relies on — the right call for a surface that can't narrow
later. This record scopes tightly because the layer is internal and can widen later at no cost. This is
the one decision that actually turns on internal-vs-public. (net stores extras broadly; wdio doesn't
build a store at all — same root, different degree.)
How to read it
🗣 Discussion
Notes from Slack and TLC minutes are recorded here as the proposal is discussed.
📌 Tracking
Tracking issue: (linked on acceptance)