Skip to content

[adr] Behavioral contract for the low-level WebDriver BiDi layer - #17786

Open
titusfortner wants to merge 9 commits into
SeleniumHQ:trunkfrom
titusfortner:adr-bidi-low-level-contract
Open

[adr] Behavioral contract for the low-level WebDriver BiDi layer#17786
titusfortner wants to merge 9 commits into
SeleniumHQ:trunkfrom
titusfortner:adr-bidi-low-level-contract

Conversation

@titusfortner

@titusfortner titusfortner commented Jul 15, 2026

Copy link
Copy Markdown
Member

📄 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

  • Automatic generation. This ADR requires behaviors, not code generation. Generation matters because the
    shared schema makes some behavior cheap to derive, but the contract is still the behavior every binding
    exhibits, regardless of how it is implemented.
  • Scope. This record stops at the serialization layer: turning typed calls into wire messages and wire
    messages back into typed objects. Transport behavior is out of scope and should be decided separately if
    needed.
  • How to read the implementation comparison. The table below is evidence for the proposal, not a second
    decision list. It separates three things that are easy to conflate: behavior that is already agreed,
    behavior that is unfinished or not-yet-adopted, and behavior where there is a real policy choice.
  • How much per-type precision should the layer require? The rows marked expensive by hand and cheaper
    from generation are the pressure points: closed-type rejection, vendor extras on extensible types,
    omitted-vs-null preservation, extra-property retention, and typed value objects. Selenium can choose a
    simpler common standard that is easier for handwritten implementations, but that permits lossy behavior. Or
    it can require the more precise per-type behavior everywhere. The table matters here because it shows
    handwritten does not automatically mean "cannot do this"; some handwritten implementations pay the per-type
    cost where they consider it correct.
  • How much browser/schema drift should the layer absorb? The organizing rule is representability:
    tolerate only what can be represented without inventing data; reject what would produce a false typed
    object.
    • Representable: a missing required field can remain omitted; an undeclared property can remain unknown
      and may be retained where useful.
    • Not representable: an unknown closed-vocabulary member or corrupt present value cannot fit the
      resolved type. The corrupt-value case is the hard floor, since accepting it means abandoning runtime
      validation entirely.
    • Main policy question (row 2.3a): should a missing required inbound field reject the whole message, or
      remain omitted and warn so Selenium does not block users when its schema is ahead of a browser
      implementation, accepting that this also tolerates a genuine defect because the layer cannot tell the two
      apart?

📊 Implementation comparison

Where each implementation (including current, proposed, and external) currently stands against the ADR

The record states three behavioral decisions (D1–D3); the table expands them into the comparison points
that best distinguish current implementations.

Decision Cost: Man→Gen Value wdio bidi-net .NET Trunk Ruby Trunk Python Trunk Python PR Java Trunk Java PR
Generated yes no no yes yes yes no yes
D1 · Outbound Validation (Local)
1.1 reject an undefined enum value 🟢 ●●○ ✅①
1.2 nullable constant: literal or null only 🟢 ●○○
1.3 required-field presence 🟢 ●●●
1.4 closed type rejects extras 🔴 → 🟢 ●●○
1.5 vendor extras reach the wire on extensible types 🔴 → 🟡 ●●● ❌②
D2 · Inbound Validation (incl. retention)
2.1 reject null in a non-nullable field 🔴⑥ → 🟢 ●●●
2.2 reject a malformed value 🔴⑥ → 🟢 ●●●
2.3a tolerate a missing required field 🟢 ●●● ✅③ ❌④ ❌⑤ ❌④ ❌⑤ ❌④ ❌④ ❌④
2.3b required + nullable: keep omitted distinct from null 🔴⑦ → 🟡 ●●○
2.4 accept an undeclared property 🟢 ●●○
2.5 an error surfaces even when its payload fails validation 🟢 ●●●
2.6 retain an inbound extra (readable; round-trip achievable) 🔴 → 🟡 ●●● ❌②
2.7 inbound-only / non-extensible types drop extras 🔴 → 🟢 ●○○
D3 · Surface (typed & names)
3.1 typed value objects 🟡 → 🟢 ●●●
3.2 surface names mirror spec 🟢 ●●●

Per-implementation through-lines

  • wdio — Generated objects, but no generated serialization layer: outbound is validated at compile
    time, while inbound is whatever falls out of casting the payload onto the object — no runtime checks, so
    2.1/2.2 are ❌ and 2.3a passes only incidentally.
  • bidi-net — Handwritten, but deliberately pays the per-type cost: closed types reject extras, extension
    data is carried on spec-extensible types, and corrupt or malformed inbound values error. Its intentional
    split from the ADR is policy, not cost: a missing required inbound field is an error because the payload is
    objectively invalid against the schema.
  • .NET trunk — Also handwritten, but uses broader shared mechanisms: unmodeled data goes into a single
    bag instead of being modeled per type, 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 its misses line up with the
    rows that are costly by hand and cheap to generate.
  • Selenium generated bindings — mostly aligned with earlier drafts of this ADR; current divergences are
    adoption gaps.

Notes

① A malformed outbound message fails wdio's TypeScript compilation and never reaches the wire, which the
ADR accepts as local validation for a statically-typed binding.

② .NET trunk holds vendor extras only in a blanket additionalData sink on the message envelope, with no
per-type extension on the spec-extensible types themselves — so a vendor capability (goog:*/moz:*) or
cookie attribute can't attach to the type that carries it, and never reaches the wire or survives a
round-trip. (bidi-net has the same envelope hatch but also carries per-type extension data on the
extensible types, so its extras do reach the wire — which is why it passes 1.5 and 2.6.)

③ wdio tolerates a missing required field because it does not validate inbound payloads at runtime; absence
stays absent, though there is no field-specific warning path.

④ This binding errors on a missing required field instead of tolerating it as absent, so the whole message
is rejected where the ADR would keep going.

⑤ .NET trunk and Python trunk don't error, but instead of leaving the field absent they fill it with a
default (0/null/false) a caller can't distinguish from a genuine value — a missing size reads as 0, a
missing flag as false, and downstream code acts on fabricated data with nothing to signal the field never
arrived.

⑥ Cheaper for static bindings: an attribute-driven deserializer rejects nulls and wrong types (2.1–2.2)
from the type system, even un-generated.

⑦ Cheaper for dynamic bindings: a sentinel keeps omitted distinct from null (2.3b) without a static
Optional/wrapper.

🗣 Discussion

Notes from Slack and TLC minutes are recorded here as the proposal is discussed.

📌 Tracking

Tracking issue: (linked on acceptance)

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add ADR defining behavioral contract for low-level WebDriver BiDi layer

📝 Documentation 🕐 10-20 Minutes

Grey Divider

AI Description

• Add an ADR that standardizes wire-observable behaviors for the BiDi low-level layer.
• Distinguish spec-mandated compliance rules from binding-owned design decisions.
• Document rationale, rejected alternatives, and consequences to align all bindings.
Diagram

graph TD
  E["High-level API"] --> B["Low-level BiDi layer"]
  A["BiDi Spec (CDDL)"] --> B["Low-level BiDi layer"] --> C["Transport"] --> D["Remote end"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Mandate code generation as the contract mechanism
  • ➕ Can reduce per-binding implementation divergence by centralizing production
  • ➕ Potentially lowers ongoing maintenance for typed models
  • ➖ Does not guarantee runtime behaviors (validation/strictness/immutability) without additional rules
  • ➖ Imposes tooling and workflow constraints that may not fit every binding
2. Define a conformance test suite as the primary artifact
  • ➕ Makes the contract executable and objectively verifiable across bindings
  • ➕ Encodes edge cases precisely (e.g., nullability, union resolution, strictness relaxations)
  • ➖ Harder to review and discuss design intent without a written rationale
  • ➖ Still needs a normative written source for why tests exist and what’s intentional vs. incidental
3. Publish the contract as binding-specific guidelines instead of one ADR
  • ➕ Lets each binding tailor wording and examples to its language/runtime
  • ➕ May reduce friction for teams with different idioms
  • ➖ Reintroduces drift risk and repeated re-litigation of decisions across bindings
  • ➖ Makes cross-binding review and enforcement significantly harder

Recommendation: Keep the ADR as the single binding-neutral normative source of behavioral requirements (as this PR does). Consider following up with a shared conformance suite that directly exercises the ADR’s decision points (strict inbound validation, extensibility handling, extras preservation scope) to make adherence measurable without making generation or tooling a requirement.

Files changed (1) +188 / -0

Documentation (1) +188 / -0
nnnnn-bidi-low-level-behavioral-contract.mdAdd ADR specifying low-level BiDi behavioral contract +188/-0

Add ADR specifying low-level BiDi behavioral contract

• Introduces a proposed architecture decision record defining a binding-neutral behavioral contract for the low-level WebDriver BiDi layer. The document enumerates compliance vs. binding-owned decisions across outbound, inbound, extensibility, and surface design, and records considered alternatives and consequences.

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md

@qodo-code-review

qodo-code-review Bot commented Jul 15, 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


Remediation recommended

1. Placeholder ADR number ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new ADR is added with placeholder ID "NNNNN" in both the filename and document heading and uses
"Discussion: _PR pending_" instead of a PR link, which violates the documented ADR numbering/linking
rules and harms traceability for future citations.
Code

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[R1-5]

+# NNNNN. Behavioral contract for the low-level WebDriver BiDi layer
+
+- Status: Proposed
+- Discussion: _PR pending_
+
Evidence
The ADR process documentation in this repo requires decision records to be numbered by the proposing
PR number and renamed before merge, and the template indicates the number should be the PR number
and the discussion field should link to the PR. The newly added ADR still contains placeholders
instead.

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[1-5]
docs/decisions/README.md[3-5]
docs/decisions/README.md[42-45]
docs/decisions/README.md[90-91]
docs/decisions/0000-template.md[1-8]

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 ADR record is still using placeholders (`nnnnn-...` filename, `# NNNNN.` header, and `Discussion: _PR pending_`). The `docs/decisions` README defines ADR numbering as the proposing PR number and requires renaming before merge, and the template expects the discussion field to link to the PR.

## Issue Context
If merged with placeholders, this record will be harder to cite consistently and will not follow the project’s documented decision-record process.

## Fix Focus Areas
- docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[1-5]
- docs/decisions/README.md[42-45]

## Proposed fix
1. Rename `docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md` to `<PR_NUMBER>-bidi-low-level-behavioral-contract.md`.
2. Update the first heading to `# <PR_NUMBER>. Behavioral contract for the low-level WebDriver BiDi layer`.
3. Replace `Discussion: _PR pending_` with a link to this PR (and optionally additional relevant threads under Context, per the template).

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



Informational

2. Ambiguous const/null example 🐞 Bug ⚙ Maintainability ⭐ New
Description
In item 3, the parenthetical example “a true/null field rejects false” is mildly ambiguous
shorthand and can read like a generic boolean-validation example rather than explicitly “allowed set
is {literal, null}” for nullable constants. Tightening this wording would make the intended rule (as
defined in item 2) unambiguous for adopters implementing outbound validation.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R63-64]

+   as does a value neither the literal nor `null` for a nullable constant (a `true`/`null` field rejects
+   `false`; see item 2). A static
Evidence
Item 3’s example uses the true/null shorthand while item 2 is where the contract precisely
defines nullable-constant behavior (“literal or null”). Making item 3’s example explicitly reflect
that membership rule removes the ambiguity.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[59-65]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[50-57]

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 example in item 3 uses shorthand (“`true`/`null` field rejects `false`”) that can be read as a general boolean-type validation example, instead of explicitly conveying the nullable-constant membership rule.

### Issue Context
Item 2 already defines the rule for nullable constants (“send the literal or `null`”), and item 3 is illustrating that outbound validation should reject anything outside that allowed set.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[59-64]

### Suggested edit
Replace the parenthetical with wording that explicitly states the allowed set, e.g.
- "… (e.g., for a nullable constant whose literal is `true`, the only valid values are `true` and `null`; `false` must be rejected; see item 2)."

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


3. Mis-cited extras rules ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The ADR cites item 7 when describing closed types “accepts-but-ignores” unknown inbound properties
and when saying other types “tolerates and drops”, but item 7 only requires that unknown inbound
properties must not error while the preserve/drop rules are defined by item 8. This can mislead
adopters into treating “drop/ignore” as part of Compliance (item 7) rather than a Decision scoped by
item 8, increasing the risk of divergent extras handling across bindings.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R74-75]

+   while a closed type rejects unknown properties outbound (item 3) and accepts-but-ignores them inbound
+   (item 7). The one prohibition is never injecting arbitrary properties into a *closed* type, which would
Evidence
Item 7 defines only that unknown inbound properties must not cause an error, while item 8 defines
when extras are preserved vs dropped; the changed lines cite item 7 in contexts that describe
ignore/drop behavior.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[74-75]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[96-98]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[100-103]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[113-113]

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 ADR cross-references **item 7** (“Unknown inbound properties never cause an error”) in places where the text is actually describing **whether unknown properties are ignored/dropped vs preserved**, which is governed by **item 8** (“Preserving extras is scoped…”).

### Issue Context
- Item 7 is the *tolerance/no-error* rule.
- Item 8 is the *preserve vs drop* scoping rule.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[69-78]
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[109-114]

### Suggested edit direction
- In item 4, change the parenthetical so it distinguishes tolerance (item 7) from preservation/drop behavior (item 8), e.g. “...tolerates unknown inbound properties (item 7) and drops them unless scoped for preservation (item 8).”
- In item 8, change “tolerates and drops (item 7)” to reference item 8 (or “item 7 for tolerance; item 8 for drop/preserve scope”).

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


4. Ambiguous section reference ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The Decision intro says “the next section marks which is which,” but the Compliance/Decision labels
are applied inline on each numbered behavior and the next section primarily explains the tagging
scheme. This is a minor ambiguity that can be fixed by rephrasing the sentence to refer to the items
below being tagged (or the next section explaining the tags).
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R18-20]

+**Any implementation of this layer must exhibit the behaviors below.** Some follow from conforming to the
+WebDriver BiDi specification; the rest are choices this record standardizes so bindings don't diverge —
+the next section marks which is which.
Evidence
Lines 18–20 claim the “next section marks which is which,” while the subsequent “How to read”
section defines the tiers and the actual tier markers appear inline on the numbered behaviors (e.g.,
item 1 includes “*(Compliance.)*”).

docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[28-36]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[46-48]

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 Decision intro refers to “the next section” as doing the marking, but the Compliance/Decision markers are attached inline to each numbered item; the following section mainly explains what those tags mean.

### Issue Context
This is a minor clarity/readability fix to align the intro text with the structure readers see.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]

ⓘ 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.

Previous review results

Review updated until commit deb870f

Results up to commit 613cd02


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


Remediation recommended
1. Placeholder ADR number ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new ADR is added with placeholder ID "NNNNN" in both the filename and document heading and uses
"Discussion: _PR pending_" instead of a PR link, which violates the documented ADR numbering/linking
rules and harms traceability for future citations.
Code

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[R1-5]

+# NNNNN. Behavioral contract for the low-level WebDriver BiDi layer
+
+- Status: Proposed
+- Discussion: _PR pending_
+
Evidence
The ADR process documentation in this repo requires decision records to be numbered by the proposing
PR number and renamed before merge, and the template indicates the number should be the PR number
and the discussion field should link to the PR. The newly added ADR still contains placeholders
instead.

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[1-5]
docs/decisions/README.md[3-5]
docs/decisions/README.md[42-45]
docs/decisions/README.md[90-91]
docs/decisions/0000-template.md[1-8]

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 ADR record is still using placeholders (`nnnnn-...` filename, `# NNNNN.` header, and `Discussion: _PR pending_`). The `docs/decisions` README defines ADR numbering as the proposing PR number and requires renaming before merge, and the template expects the discussion field to link to the PR.

## Issue Context
If merged with placeholders, this record will be harder to cite consistently and will not follow the project’s documented decision-record process.

## Fix Focus Areas
- docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[1-5]
- docs/decisions/README.md[42-45]

## Proposed fix
1. Rename `docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md` to `<PR_NUMBER>-bidi-low-level-behavioral-contract.md`.
2. Update the first heading to `# <PR_NUMBER>. Behavioral contract for the low-level WebDriver BiDi layer`.
3. Replace `Discussion: _PR pending_` with a link to this PR (and optionally additional relevant threads under Context, per the template).

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


Results up to commit dfe1c07


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


Informational
1. Ambiguous section reference 🐞 Bug ⚙ Maintainability
Description
The Decision intro says “the next section marks which is which,” but the Compliance/Decision labels
are applied inline on each numbered behavior and the next section primarily explains the tagging
scheme. This is a minor ambiguity that can be fixed by rephrasing the sentence to refer to the items
below being tagged (or the next section explaining the tags).
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R18-20]

+**Any implementation of this layer must exhibit the behaviors below.** Some follow from conforming to the
+WebDriver BiDi specification; the rest are choices this record standardizes so bindings don't diverge —
+the next section marks which is which.
Evidence
Lines 18–20 claim the “next section marks which is which,” while the subsequent “How to read”
section defines the tiers and the actual tier markers appear inline on the numbered behaviors (e.g.,
item 1 includes “*(Compliance.)*”).

docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[28-36]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[46-48]

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 Decision intro refers to “the next section” as doing the marking, but the Compliance/Decision markers are attached inline to each numbered item; the following section mainly explains what those tags mean.

### Issue Context
This is a minor clarity/readability fix to align the intro text with the structure readers see.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]

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


Results up to commit 5b77f31


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


Informational
1. Mis-cited extras rules ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The ADR cites item 7 when describing closed types “accepts-but-ignores” unknown inbound properties
and when saying other types “tolerates and drops”, but item 7 only requires that unknown inbound
properties must not error while the preserve/drop rules are defined by item 8. This can mislead
adopters into treating “drop/ignore” as part of Compliance (item 7) rather than a Decision scoped by
item 8, increasing the risk of divergent extras handling across bindings.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R74-75]

+   while a closed type rejects unknown properties outbound (item 3) and accepts-but-ignores them inbound
+   (item 7). The one prohibition is never injecting arbitrary properties into a *closed* type, which would
Evidence
Item 7 defines only that unknown inbound properties must not cause an error, while item 8 defines
when extras are preserved vs dropped; the changed lines cite item 7 in contexts that describe
ignore/drop behavior.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[74-75]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[96-98]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[100-103]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[113-113]

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 ADR cross-references **item 7** (“Unknown inbound properties never cause an error”) in places where the text is actually describing **whether unknown properties are ignored/dropped vs preserved**, which is governed by **item 8** (“Preserving extras is scoped…”).

### Issue Context
- Item 7 is the *tolerance/no-error* rule.
- Item 8 is the *preserve vs drop* scoping rule.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[69-78]
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[109-114]

### Suggested edit direction
- In item 4, change the parenthetical so it distinguishes tolerance (item 7) from preservation/drop behavior (item 8), e.g. “...tolerates unknown inbound properties (item 7) and drops them unless scoped for preservation (item 8).”
- In item 8, change “tolerates and drops (item 7)” to reference item 8 (or “item 7 for tolerance; item 8 for drop/preserve scope”).

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


Qodo Logo

Comment thread docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 98e1858

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

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.

Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 5b77f31

Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@qodo-code-review

qodo-code-review Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. Missing selector behavior undefined 🐞 Bug ≡ Correctness ⭐ New
Description
Decision 2 only defines missing-field handling after union resolution, but a missing discriminator
or insufficient structural selector fields can prevent a variant from resolving at all. The contract
therefore does not specify whether bindings should error, warn, or return another representation for
these inbound payloads.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R45-49]

+   Otherwise, once the spec's union rule resolves the payload's type (if applicable), each mismatch is
+   handled by its kind:
+   1. **Error if corrupted** — the value cannot fit the resolved type: a null in a non-nullable field, a
+      wrong primitive type, a cardinality mismatch, a non-object where an object is expected.
+   2. **Warn if a required field is missing** — the field must be left *omitted* (not an explicit *null*,
Evidence
The ADR handles missing required fields only after the union's type has resolved, while also
requiring each union variant to be represented by a distinct type. The shared schema's union
selectors resolve discriminated unions from payload[by] and structural unions from required-key
presence, so absent or insufficient selector fields can make that prerequisite fail without any
contract-defined outcome.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[41-70]
javascript/selenium-webdriver/project_bidi_schema.mjs[275-317]
javascript/selenium-webdriver/project_bidi_schema_test.mjs[206-239]

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

## Issue description
Define the required behavior when an inbound union cannot be resolved because its discriminator is absent or its structural selector is ambiguous. This case currently occurs before the documented missing-field policy can apply.

## Issue Context
Union selectors may depend on a discriminator value or the presence of required fields. The ADR should explicitly choose a uniform resolution-failure policy, such as rejecting unresolved closed unions, rather than leaving each binding to decide.

## Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[41-65]

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


2. Relaxation reporting undefined 🐞 Bug ◔ Observability
Description
Item 7 requires tolerated absences to be “still reported” but does not define the minimum
caller-observable semantics of that reporting, so bindings can diverge (e.g., logging only vs
structured diagnostics) while still claiming conformance.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R112-114]

+   - **The tolerated absence is still reported**, never silently absorbed. A binding *may* additionally
+     let a caller admit an uncatalogued absence at runtime, so a user is not blocked until the next
+     release; that is a per-binding convenience, not required here, and it too reports.
Evidence
The ADR imposes a normative requirement (“still reported”) but provides no definition of how
reporting must be exposed, leaving implementation latitude that undermines uniformity.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[112-114]

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 ADR mandates that tolerated absences are “still reported,” but leaves “reported” undefined. This makes conformance hard to test and encourages cross-binding divergence in what users/maintainers can observe.

## Issue Context
This ADR’s stated goal is uniform wire-observable behavior across bindings. Relaxation reporting affects how drift is detected, tracked, and eventually retired.

## Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[112-114]

## Suggested change
Add a minimum requirement for reporting that is binding-idiomatic but caller-observable, e.g.:
- Require an explicit diagnostic surface (exception subtype, warning callback/hook, or returned structured warning object) that includes at least: the schema/type, the missing field name, and the command/event context.
- Clarify whether logging alone is sufficient or explicitly state it is not.

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


3. Undefined error-code surfacing ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
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.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R127-130]

+   applied to every binding at once — never one alone, and never by starting lenient. One exception is
+   sanctioned from the start: an enum that reports an error (an unrecognized error code in an
+   `ErrorResponse`), where raising would swallow the error being reported; there, an unknown token is
+   surfaced, not thrown.
Evidence
The ADR states its purpose is to standardize wire-observable behavior across bindings; leaving
“surfaced” undefined for unknown error codes reintroduces the same cross-binding drift risk the ADR
is meant to eliminate.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[13-15]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[122-130]

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

### 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


View more (1)
4. Placeholder ADR number ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new ADR is added with placeholder ID "NNNNN" in both the filename and document heading and uses
"Discussion: _PR pending_" instead of a PR link, which violates the documented ADR numbering/linking
rules and harms traceability for future citations.
Code

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[R1-5]

+# NNNNN. Behavioral contract for the low-level WebDriver BiDi layer
+
+- Status: Proposed
+- Discussion: _PR pending_
+
Evidence
The ADR process documentation in this repo requires decision records to be numbered by the proposing
PR number and renamed before merge, and the template indicates the number should be the PR number
and the discussion field should link to the PR. The newly added ADR still contains placeholders
instead.

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[1-5]
docs/decisions/README.md[3-5]
docs/decisions/README.md[42-45]
docs/decisions/README.md[90-91]
docs/decisions/0000-template.md[1-8]

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 ADR record is still using placeholders (`nnnnn-...` filename, `# NNNNN.` header, and `Discussion: _PR pending_`). The `docs/decisions` README defines ADR numbering as the proposing PR number and requires renaming before merge, and the template expects the discussion field to link to the PR.

## Issue Context
If merged with placeholders, this record will be harder to cite consistently and will not follow the project’s documented decision-record process.

## Fix Focus Areas
- docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[1-5]
- docs/decisions/README.md[42-45]

## Proposed fix
1. Rename `docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md` to `<PR_NUMBER>-bidi-low-level-behavioral-contract.md`.
2. Update the first heading to `# <PR_NUMBER>. Behavioral contract for the low-level WebDriver BiDi layer`.
3. Replace `Discussion: _PR pending_` with a link to this PR (and optionally additional relevant threads under Context, per the template).

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



Informational

5. Absence not a key ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Item 6 says “every key in the payload” falls into three cases, but one case is “Absence” (a missing
required field), which is not a payload key; this makes the validation taxonomy internally
inconsistent and easier to misread in isolation.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R90-99]

+6. **Inbound payloads are validated against the resolved type.** *(Decision.)* Once item 5 has resolved
+   the type, every key in the payload falls into one of three cases, and a declared field is checked
+   rather than populated silently:
+   - **Corruption — always raises.** A null in a non-nullable field, a value of the wrong primitive
+     type, a list/scalar cardinality mismatch, a non-object where a field expects an object. The wire
+     asserted something untrue, and absorbing it is what misrepresents protocol state.
+   - **Absence — raises.** A required field is missing. Nothing untrue was asserted and every field that
+     did arrive is still correct, so absence is the one case a relaxation may reach (item 7).
+   - **Undeclared key — never raises.** Every type tolerates a property it does not define, open or
+     closed; most parsers do this by default, and a reject-unmapped setting would violate it. The spec
Evidence
The text asserts the three cases apply to “every key in the payload,” but immediately defines
“Absence” as a required field being missing (i.e., not a key), making the classification statement
inaccurate.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[90-99]

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

## Issue description
Item 6 frames validation as applying to “every key in the payload,” but then includes “Absence” (missing required field) as one of the cases. This is internally inconsistent and can confuse readers about what is being classified.

## Issue Context
This section is defining a cross-binding behavioral contract; the validation taxonomy should be precise and unambiguous.

## Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[90-99]

## Suggested change
Rewrite the lead-in to separate declared-field validation from payload-key handling, e.g.:
- “Once item 5 has resolved the type, validation covers (a) declared fields (including required-field presence) and (b) any undeclared keys present in the payload.”
Or change “every key” to “every field/key case” and explicitly define what ‘absence’ refers to.

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


6. Broken Consequences sentence ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
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.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R189-190]

+- The per-type signals items 5/6/8 need are all derivable from the spec; a binding that discards one, or
+  parses into a lenient runtime, falls out of conformance on exactly the items that signal feeds.
Evidence
The typo is present in the final bullet of the ADR’s Consequences section.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[185-190]

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


7. Ambiguous const/null example 🐞 Bug ⚙ Maintainability
Description
In item 3, the parenthetical example “a true/null field rejects false” is mildly ambiguous
shorthand and can read like a generic boolean-validation example rather than explicitly “allowed set
is {literal, null}” for nullable constants. Tightening this wording would make the intended rule (as
defined in item 2) unambiguous for adopters implementing outbound validation.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R63-64]

+   as does a value neither the literal nor `null` for a nullable constant (a `true`/`null` field rejects
+   `false`; see item 2). A static
Evidence
Item 3’s example uses the true/null shorthand while item 2 is where the contract precisely
defines nullable-constant behavior (“literal or null”). Making item 3’s example explicitly reflect
that membership rule removes the ambiguity.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[59-65]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[50-57]

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 example in item 3 uses shorthand (“`true`/`null` field rejects `false`”) that can be read as a general boolean-type validation example, instead of explicitly conveying the nullable-constant membership rule.

### Issue Context
Item 2 already defines the rule for nullable constants (“send the literal or `null`”), and item 3 is illustrating that outbound validation should reject anything outside that allowed set.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[59-64]

### Suggested edit
Replace the parenthetical with wording that explicitly states the allowed set, e.g.
- "… (e.g., for a nullable constant whose literal is `true`, the only valid values are `true` and `null`; `false` must be rejected; see item 2)."

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


View more (2)
8. Mis-cited extras rules ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The ADR cites item 7 when describing closed types “accepts-but-ignores” unknown inbound properties
and when saying other types “tolerates and drops”, but item 7 only requires that unknown inbound
properties must not error while the preserve/drop rules are defined by item 8. This can mislead
adopters into treating “drop/ignore” as part of Compliance (item 7) rather than a Decision scoped by
item 8, increasing the risk of divergent extras handling across bindings.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R74-75]

+   while a closed type rejects unknown properties outbound (item 3) and accepts-but-ignores them inbound
+   (item 7). The one prohibition is never injecting arbitrary properties into a *closed* type, which would
Evidence
Item 7 defines only that unknown inbound properties must not cause an error, while item 8 defines
when extras are preserved vs dropped; the changed lines cite item 7 in contexts that describe
ignore/drop behavior.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[74-75]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[96-98]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[100-103]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[113-113]

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 ADR cross-references **item 7** (“Unknown inbound properties never cause an error”) in places where the text is actually describing **whether unknown properties are ignored/dropped vs preserved**, which is governed by **item 8** (“Preserving extras is scoped…”).

### Issue Context
- Item 7 is the *tolerance/no-error* rule.
- Item 8 is the *preserve vs drop* scoping rule.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[69-78]
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[109-114]

### Suggested edit direction
- In item 4, change the parenthetical so it distinguishes tolerance (item 7) from preservation/drop behavior (item 8), e.g. “...tolerates unknown inbound properties (item 7) and drops them unless scoped for preservation (item 8).”
- In item 8, change “tolerates and drops (item 7)” to reference item 8 (or “item 7 for tolerance; item 8 for drop/preserve scope”).

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


9. Ambiguous section reference ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The Decision intro says “the next section marks which is which,” but the Compliance/Decision labels
are applied inline on each numbered behavior and the next section primarily explains the tagging
scheme. This is a minor ambiguity that can be fixed by rephrasing the sentence to refer to the items
below being tagged (or the next section explaining the tags).
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R18-20]

+**Any implementation of this layer must exhibit the behaviors below.** Some follow from conforming to the
+WebDriver BiDi specification; the rest are choices this record standardizes so bindings don't diverge —
+the next section marks which is which.
Evidence
Lines 18–20 claim the “next section marks which is which,” while the subsequent “How to read”
section defines the tiers and the actual tier markers appear inline on the numbered behaviors (e.g.,
item 1 includes “*(Compliance.)*”).

docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[28-36]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[46-48]

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 Decision intro refers to “the next section” as doing the marking, but the Compliance/Decision markers are attached inline to each numbered item; the following section mainly explains what those tags mean.

### Issue Context
This is a minor clarity/readability fix to align the intro text with the structure readers see.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]

ⓘ 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.

Previous review results

Review updated until commit deb870f ⚖️ Balanced

Results up to commit 613cd02


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


Remediation recommended
1. Placeholder ADR number ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new ADR is added with placeholder ID "NNNNN" in both the filename and document heading and uses
"Discussion: _PR pending_" instead of a PR link, which violates the documented ADR numbering/linking
rules and harms traceability for future citations.
Code

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[R1-5]

+# NNNNN. Behavioral contract for the low-level WebDriver BiDi layer
+
+- Status: Proposed
+- Discussion: _PR pending_
+
Evidence
The ADR process documentation in this repo requires decision records to be numbered by the proposing
PR number and renamed before merge, and the template indicates the number should be the PR number
and the discussion field should link to the PR. The newly added ADR still contains placeholders
instead.

docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[1-5]
docs/decisions/README.md[3-5]
docs/decisions/README.md[42-45]
docs/decisions/README.md[90-91]
docs/decisions/0000-template.md[1-8]

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 ADR record is still using placeholders (`nnnnn-...` filename, `# NNNNN.` header, and `Discussion: _PR pending_`). The `docs/decisions` README defines ADR numbering as the proposing PR number and requires renaming before merge, and the template expects the discussion field to link to the PR.

## Issue Context
If merged with placeholders, this record will be harder to cite consistently and will not follow the project’s documented decision-record process.

## Fix Focus Areas
- docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md[1-5]
- docs/decisions/README.md[42-45]

## Proposed fix
1. Rename `docs/decisions/nnnnn-bidi-low-level-behavioral-contract.md` to `<PR_NUMBER>-bidi-low-level-behavioral-contract.md`.
2. Update the first heading to `# <PR_NUMBER>. Behavioral contract for the low-level WebDriver BiDi layer`.
3. Replace `Discussion: _PR pending_` with a link to this PR (and optionally additional relevant threads under Context, per the template).

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


Results up to commit 98e1858


No changes from previous review

Results up to commit dfe1c07


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


Informational
1. Ambiguous section reference ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The Decision intro says “the next section marks which is which,” but the Compliance/Decision labels
are applied inline on each numbered behavior and the next section primarily explains the tagging
scheme. This is a minor ambiguity that can be fixed by rephrasing the sentence to refer to the items
below being tagged (or the next section explaining the tags).
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R18-20]

+**Any implementation of this layer must exhibit the behaviors below.** Some follow from conforming to the
+WebDriver BiDi specification; the rest are choices this record standardizes so bindings don't diverge —
+the next section marks which is which.
Evidence
Lines 18–20 claim the “next section marks which is which,” while the subsequent “How to read”
section defines the tiers and the actual tier markers appear inline on the numbered behaviors (e.g.,
item 1 includes “*(Compliance.)*”).

docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[28-36]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[46-48]

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 Decision intro refers to “the next section” as doing the marking, but the Compliance/Decision markers are attached inline to each numbered item; the following section mainly explains what those tags mean.

### Issue Context
This is a minor clarity/readability fix to align the intro text with the structure readers see.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]

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


Results up to commit 5b77f31


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


Informational
1. Mis-cited extras rules ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The ADR cites item 7 when describing closed types “accepts-but-ignores” unknown inbound properties
and when saying other types “tolerates and drops”, but item 7 only requires that unknown inbound
properties must not error while the preserve/drop rules are defined by item 8. This can mislead
adopters into treating “drop/ignore” as part of Compliance (item 7) rather than a Decision scoped by
item 8, increasing the risk of divergent extras handling across bindings.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R74-75]

+   while a closed type rejects unknown properties outbound (item 3) and accepts-but-ignores them inbound
+   (item 7). The one prohibition is never injecting arbitrary properties into a *closed* type, which would
Evidence
Item 7 defines only that unknown inbound properties must not cause an error, while item 8 defines
when extras are preserved vs dropped; the changed lines cite item 7 in contexts that describe
ignore/drop behavior.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[74-75]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[96-98]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[100-103]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[113-113]

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 ADR cross-references **item 7** (“Unknown inbound properties never cause an error”) in places where the text is actually describing **whether unknown properties are ignored/dropped vs preserved**, which is governed by **item 8** (“Preserving extras is scoped…”).

### Issue Context
- Item 7 is the *tolerance/no-error* rule.
- Item 8 is the *preserve vs drop* scoping rule.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[69-78]
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[109-114]

### Suggested edit direction
- In item 4, change the parenthetical so it distinguishes tolerance (item 7) from preservation/drop behavior (item 8), e.g. “...tolerates unknown inbound properties (item 7) and drops them unless scoped for preservation (item 8).”
- In item 8, change “tolerates and drops (item 7)” to reference item 8 (or “item 7 for tolerance; item 8 for drop/preserve scope”).

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


Results up to commit e965829


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


Informational
1. Ambiguous const/null example 🐞 Bug ⚙ Maintainability
Description
In item 3, the parenthetical example “a true/null field rejects false” is mildly ambiguous
shorthand and can read like a generic boolean-validation example rather than explicitly “allowed set
is {literal, null}” for nullable constants. Tightening this wording would make the intended rule (as
defined in item 2) unambiguous for adopters implementing outbound validation.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R63-64]

+   as does a value neither the literal nor `null` for a nullable constant (a `true`/`null` field rejects
+   `false`; see item 2). A static
Evidence
Item 3’s example uses the true/null shorthand while item 2 is where the contract precisely
defines nullable-constant behavior (“literal or null”). Making item 3’s example explicitly reflect
that membership rule removes the ambiguity.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[59-65]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[50-57]

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 example in item 3 uses shorthand (“`true`/`null` field rejects `false`”) that can be read as a general boolean-type validation example, instead of explicitly conveying the nullable-constant membership rule.

### Issue Context
Item 2 already defines the rule for nullable constants (“send the literal or `null`”), and item 3 is illustrating that outbound validation should reject anything outside that allowed set.

### Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[59-64]

### Suggested edit
Replace the parenthetical with wording that explicitly states the allowed set, e.g.
- "… (e.g., for a nullable constant whose literal is `true`, the only valid values are `true` and `null`; `false` must be rejected; see item 2)."

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


Results up to commit 1b0e91a


No changes from previous review

Results up to commit 412d9f2


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


Remediation recommended
1. Undefined error-code surfacing ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
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.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R127-130]

+   applied to every binding at once — never one alone, and never by starting lenient. One exception is
+   sanctioned from the start: an enum that reports an error (an unrecognized error code in an
+   `ErrorResponse`), where raising would swallow the error being reported; there, an unknown token is
+   surfaced, not thrown.
Evidence
The ADR states its purpose is to standardize wire-observable behavior across bindings; leaving
“surfaced” undefined for unknown error codes reintroduces the same cross-binding drift risk the ADR
is meant to eliminate.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[13-15]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[18-20]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[122-130]

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

### 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



Informational
2. Broken Consequences sentence ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
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.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R189-190]

+- The per-type signals items 5/6/8 need are all derivable from the spec; a binding that discards one, or
+  parses into a lenient runtime, falls out of conformance on exactly the items that signal feeds.
Evidence
The typo is present in the final bullet of the ADR’s Consequences section.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[185-190]

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


Results up to commit 541031b ⚖️ Balanced


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


Remediation recommended
1. Relaxation reporting undefined 🐞 Bug ◔ Observability
Description
Item 7 requires tolerated absences to be “still reported” but does not define the minimum
caller-observable semantics of that reporting, so bindings can diverge (e.g., logging only vs
structured diagnostics) while still claiming conformance.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R112-114]

+   - **The tolerated absence is still reported**, never silently absorbed. A binding *may* additionally
+     let a caller admit an uncatalogued absence at runtime, so a user is not blocked until the next
+     release; that is a per-binding convenience, not required here, and it too reports.
Evidence
The ADR imposes a normative requirement (“still reported”) but provides no definition of how
reporting must be exposed, leaving implementation latitude that undermines uniformity.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[112-114]

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 ADR mandates that tolerated absences are “still reported,” but leaves “reported” undefined. This makes conformance hard to test and encourages cross-binding divergence in what users/maintainers can observe.

## Issue Context
This ADR’s stated goal is uniform wire-observable behavior across bindings. Relaxation reporting affects how drift is detected, tracked, and eventually retired.

## Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[112-114]

## Suggested change
Add a minimum requirement for reporting that is binding-idiomatic but caller-observable, e.g.:
- Require an explicit diagnostic surface (exception subtype, warning callback/hook, or returned structured warning object) that includes at least: the schema/type, the missing field name, and the command/event context.
- Clarify whether logging alone is sufficient or explicitly state it is not.

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



Informational
2. Absence not a key ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Item 6 says “every key in the payload” falls into three cases, but one case is “Absence” (a missing
required field), which is not a payload key; this makes the validation taxonomy internally
inconsistent and easier to misread in isolation.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R90-99]

+6. **Inbound payloads are validated against the resolved type.** *(Decision.)* Once item 5 has resolved
+   the type, every key in the payload falls into one of three cases, and a declared field is checked
+   rather than populated silently:
+   - **Corruption — always raises.** A null in a non-nullable field, a value of the wrong primitive
+     type, a list/scalar cardinality mismatch, a non-object where a field expects an object. The wire
+     asserted something untrue, and absorbing it is what misrepresents protocol state.
+   - **Absence — raises.** A required field is missing. Nothing untrue was asserted and every field that
+     did arrive is still correct, so absence is the one case a relaxation may reach (item 7).
+   - **Undeclared key — never raises.** Every type tolerates a property it does not define, open or
+     closed; most parsers do this by default, and a reject-unmapped setting would violate it. The spec
Evidence
The text asserts the three cases apply to “every key in the payload,” but immediately defines
“Absence” as a required field being missing (i.e., not a key), making the classification statement
inaccurate.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[90-99]

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

## Issue description
Item 6 frames validation as applying to “every key in the payload,” but then includes “Absence” (missing required field) as one of the cases. This is internally inconsistent and can confuse readers about what is being classified.

## Issue Context
This section is defining a cross-binding behavioral contract; the validation taxonomy should be precise and unambiguous.

## Fix Focus Areas
- docs/decisions/17786-bidi-low-level-behavioral-contract.md[90-99]

## Suggested change
Rewrite the lead-in to separate declared-field validation from payload-key handling, e.g.:
- “Once item 5 has resolved the type, validation covers (a) declared fields (including required-field presence) and (b) any undeclared keys present in the payload.”
Or change “every key” to “every field/key case” and explicitly define what ‘absence’ refers to.

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


Qodo Logo

Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
strict default (a Decision, item 9), not part of this compliance floor; for a union whose arms are
all objects, a non-object payload has no variant to select and raises.

6. **Inbound fields are validated strictly against the resolved type.** *(Decision.)* Once item 5 has

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This may lead to bad situation. Let's image one required (non-nullable) field is added into the spec. Selenium added it accordingly. Users will get serialization error because this field is missing in payload... because... some vendor still didn't implement it.
IMHO serialization process should be relaxed.
Real example: w3c/webdriver-bidi#1117

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So you know, this comment resulted in my spending the past 5 days overhauling the ADR. Not sure it was worth that much time investment, but I'm much happier with it now. The TL/DR is that I now think inbound should be strictly validated except for missing required parameters.

Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 541031b

…n, inbound validation & retention, typed surface
Comment thread docs/decisions/17786-bidi-low-level-behavioral-contract.md
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@titusfortner

Copy link
Copy Markdown
Member Author

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.

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

Labels

A-needs decision TLC needs to discuss and agree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants