Skip to content

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

Open
titusfortner wants to merge 15 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 15 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 comparison table below. Each row is a point of compliance with a decision in this
    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).
  • Two ways to consider decisions from this ADR (with table rows noted for reference as applicable)
    1. Per-type precision support. Generating from the CDDL dramatically reduces
      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:
      • Drop per-type behavior entirely for a simpler, lossy common standard (better supports handwritten
        implementations).
      • Drop only per-type extensibility (better supports static languages).
    2. Browser/schema drift support. The organizing rule is representability:
      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:
      • Reject undeclared extras instead of accepting them (9a/9b), at the cost of forward compatibility
        (the layer breaks against a newer browser).
      • Reject missing required fields instead of tolerating them (8a), at the cost of backward
        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.

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
1 · typed value objects 🟡① → 🟢 ●●●
2 · surface names mirror spec 🟢 ●●●
3 · preserve numeric range and precision 🟢 ●●○ ❌③ ❌③
5a · reject an undefined enum value 🟢 ●●○
5b · nullable constant: literal or null only 🟢 ●○○
5c · required-field presence 🟢 ●●●
6a · closed type rejects extras 🔴 → 🟢 ●●○
6b · extensible types carry vendor extras to the wire 🔴② → 🟡 ●●●
7a · reject a null in a non-nullable field 🔴 → 🟢 ●●●
7b · reject a malformed value 🔴① → 🟢 ●●● ❌④
7c · reject an unresolvable closed-union variant 🔴 → 🟢 ●●● ❌⑤ ❌⑥
8a · tolerate a missing required field 🟢 ●●● ✅⑦ ❌⑧ ❌⑨ ❌⑨ ❌⑧ ❌⑧
8b · required + nullable: keep omitted distinct from null 🔴② → 🟡 ●●○ ❌⑩ ❌⑩ ❌⑩ ❌⑩ ❌⑩
9a · accept an undeclared field 🟢 ●●○
9b · retain an inbound extra on an extensible type 🔴② → 🟡 ●●●
9c · non-extensible types drop extras 🔴 → 🟢 ●○○ ✅⑦ ✅⑦ ✅⑦ ✅⑦
10 · preserve received values faithfully 🟢 ●●○

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, with no runtime checks,
    so 7a/7b/7c are ❌ and 8a passes only incidentally.
  • bidi-net — Handwritten and strict by default, it deliberately pays the cost for per-type behavior:
    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.
  • .NET trunk — Handwritten with broad shared mechanisms: its only extension point is a single
    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.
  • Ruby trunk — This is the prototype implementation kept current with this draft PR.
  • Python trunk — Generated but not from shared schema, in process of being updated by the referenced PR.
  • Python PR — Generated adoption, up to date with the proposed ADR, but migrating existing code to it will be challenging.
  • Java trunk — Handwritten and in process of being updated by the referenced PR.
  • Java PR — Generated, and in progress. Its primary divergence is per-type extras: retaining undeclared
    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-uint field (a status code, a line or column number) to a 32-bit int; the
genuinely wide fields (byte sizes, counts) stay long, so the miss is limited to these small slots.

④ Selenium's shared NumberCoercer is too lenient to enforce this (it truncates fractional integers and
accepts float- or string-encoded numbers); the BiDi layer likely needs a stricter, BiDi-scoped coercer.

⑤ Returns null for an unknown RemoteValue type instead of erroring, so an undeclared closed-union variant
is 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 real
value, 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)

@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 (0) 📘 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 ✓ Resolved 🐞 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 96e22f0

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


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 (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Extras retention rule undefined ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Decision 3 retains unknown properties only when “the received type or its command-parameter
counterpart is extensible”, but it doesn’t define what a “command-parameter counterpart” is or how
to determine this mapping for results and events. This makes the retain-vs-drop behavior
non-deterministic across bindings even though it’s presented as part of the contract.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R57-61]

+   - **An undeclared property warns, and is tolerated** (forward-compatibility). Where the type is
+     round-trippable — the received type or its command-parameter counterpart is extensible — the unknown
+     property is kept on the object, so a caller can reproduce it on the wire: a vendor attribute received on
+     a `network.Cookie` can be set again through `storage.setCookie` unchanged. Other undeclared properties
+     are tolerated but not retained.
Evidence
The retention condition references “command-parameter counterpart” as a normative criterion but
provides no definition or general mapping rule.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[57-61]

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’s rule for when to retain unknown inbound properties depends on an undefined concept (“command-parameter counterpart”), which will lead to inconsistent implementations.

### Issue Context
This affects forward-compatibility behavior and round-tripping of vendor/future fields.

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

### Suggested change
Define the rule in a schema-addressable way, for example:
- explicitly define what “counterpart” means (e.g., a type pair linked by an explicit schema annotation or a named relationship), and how it applies to command results vs events.
- or simplify the retention condition to depend only on properties of the *received* type (e.g., “retain extras only on types marked extensible AND designated round-trippable by the shared schema”).

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



Remediation recommended

2. Omission/null mapping ambiguous ✓ Resolved 🐞 Bug ≡ Correctness
Description
The Consequences section says missing required inbound fields can be represented by setting the
field to null (“null marks omitted”), but the contract also requires rejecting an explicitly
present null for non-nullable fields. Without an explicit requirement to check wire-level field
presence before mapping omission to null, implementations can incorrectly accept explicit null
as “omitted.”
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R154-156]

+  field non-null yet leave it *omitted* when missing. A nullable slot suffices for most fields (real data is
+  never `null` there, so `null` marks *omitted*); a required *nullable* field (network `context`/`navigation`,
+  response sizes, log `text`, ~30 in all) instead needs *omitted* kept distinct from `null`. The trigger is
Evidence
Decision 4 defines null in a non-nullable field as structurally invalid, and Decision 7 requires
rejecting present invalid values; the Consequences text then recommends using null to mark
omission, which is ambiguous unless presence is checked before the mapping.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[49-52]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[74-78]
docs/decisions/17786-bidi-low-level-behavioral-contract.md[153-156]

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’s Consequences suggests using `null` as an internal marker for an *omitted* (missing) required inbound field. However, the ADR also defines `null` in a non-nullable field as structurally invalid and requires rejecting present-but-invalid values. Without a clear “check presence first” rule, a binding can collapse both cases to `null` and accidentally treat an explicit inbound `null` as omission.

### Issue Context
This is about inbound decoding/validation semantics: bindings must be able to reject explicit `null` values for non-nullable fields (invalid) while still tolerating missing required fields (omitted).

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

### Suggested change
Adjust the Consequences bullet to explicitly require presence-aware validation at the wire/deserialization boundary, e.g.:
- Explicit `null` for a non-nullable field MUST be rejected (decision 7), and
- Only after confirming the field is absent may a binding map omission to an internal `null`/`None`/`Optional.empty` representation for convenience.

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


3. Integer token handling unclear ✓ Resolved 🐞 Bug ≡ Correctness
Description
Decision 1 requires integer to reject whole-valued 5.0 as “float-encoded”, which depends on the
JSON number’s lexical form (not just its numeric value) and is not enforceable once a typical JSON
parser has normalized the token. Without explicitly requiring lexeme-preserving parsing (or
weakening the rule), bindings will either be unable to conform or will diverge in what they
accept/reject.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R31-33]

+     declared. A primitive matches by JSON kind, not language representation: `number` admits any JSON
+     number, while `integer` rejects a fractional or float-encoded one (a whole-valued `5.0` included);
+     neither direction coerces across the boundary silently.
Evidence
The ADR explicitly calls out rejecting “float-encoded” integers including 5.0, which implies
lexical inspection rather than only checking integral numeric value.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[31-33]

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 requires rejecting an `integer` value that was encoded as `5.0`, which implies inspecting the JSON numeric lexeme. Many JSON stacks do not preserve that distinction after parsing, so the behavior is underspecified and likely to diverge across bindings.

### Issue Context
This is in the normative validation rule for primitives.

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

### Suggested change
Amend the text to either:
1) explicitly require lexeme-preserving number parsing (or an equivalent exact-number/token API) for inbound validation when enforcing `5` vs `5.0`, **or**
2) redefine the rule in terms of numeric value only (e.g., “reject values with a fractional component”), removing the lexical-form requirement.

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


4. Error surfacing boundary unclear ✓ Resolved 🐞 Bug ☼ Reliability
Description
Decision 3 says remote errors must surface even if their payload fails validation, but it doesn’t
specify the minimum classification boundary for “this is an error response” versus “this is a
malformed/unclassifiable message”. Without that clarification, implementations can still
legitimately throw local serialization errors for malformed error envelopes, contradicting the
stated requirement to not let validation override remote errors.
Code

docs/decisions/17786-bidi-low-level-behavioral-contract.md[R47-51]

+3. **Inbound, those same two are tolerated.** Process error responses first: a remote error surfaces as an
+   error even if its payload fails validation — an unrecognized error code included — and the checks here
+   must not turn it into a local serialization failure; the error's contents are otherwise out of scope.
+   Otherwise these rules govern every typed payload the layer parses — a command's result and a
+   server-initiated event's parameters alike, with response correlation and event routing out of scope:
Evidence
The ADR requires error responses to bypass normal validation failures, but does not define the
classification boundary needed to apply that rule consistently.

docs/decisions/17786-bidi-low-level-behavioral-contract.md[47-51]

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 promises that remote errors are surfaced even when the error payload fails validation, but it doesn’t define what must be successfully parsed to classify a message as an error response, nor what happens when those identifying fields are missing/invalid.

### Issue Context
Transport behavior is said to be out of scope, but this section still imposes a behavioral requirement on error handling that depends on a clear classification boundary.

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

### Suggested change
Add a short normative clarification such as:
- which minimal envelope fields must be read to classify an inbound message as an error response (before full validation), and
- if those are missing/invalid, whether the message is treated as a transport/protocol error (out of scope here) versus a local serialization error.
This keeps the “errors must surface” promise implementable and consistent across bindings.

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


View more (4)
5. Missing selector behavior undefined ✓ Resolved 🐞 Bug ≡ Correctness
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


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


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


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

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


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


11. Ambiguous const/null example ✓ Resolved 🐞 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)
12. 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


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

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

…n, inbound validation & retention, typed surface
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 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.

…validity floor, numeric boundary, event params, union resolution)
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 fdb7525

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

Copy link
Copy Markdown
Member Author

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:

  • Numeric: BiDi integers are bounded to the JS safe-integer range (±(2^53−1)), so a 64-bit int or a double is exact; dropped the "int64/bigint, never a double" framing.
  • Extensibility: stated in decision 1, including that a declared key never appears in the extras map — closes a value-shadowing footgun found implementing Ruby.
  • Unions/RemoteValue: "unresolvable" now means a discriminator the spec doesn't declare; structurally-identical variants may share one type, so a shared carrier is conformant and only a genuinely unknown/newer type is rejected.
  • Fidelity: it's fidelity of the value, not its byte-form — native types are fine as long as nothing is lost.
  • Type validity: decision 4 now reads as a shared definition that decisions 5 (outbound) and 7 (inbound) enforce, not a behavior of its own — so it has no standalone compliance point.

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

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 no new comments.

Suppressed comments (2)

docs/decisions/17786-bidi-low-level-behavioral-contract.md:52

  • The integer definition mixes value semantics (“fractional”) with an ambiguous “float-encoded” requirement, and the 5.0 example 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-int and js-uint by using a single “±” range. js-uint is 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
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 96e22f0

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