Skip to content

[adr] BiDi is an implementation mechanism, not a public API#17670

Open
titusfortner wants to merge 5 commits into
SeleniumHQ:trunkfrom
titusfortner:adr1
Open

[adr] BiDi is an implementation mechanism, not a public API#17670
titusfortner wants to merge 5 commits into
SeleniumHQ:trunkfrom
titusfortner:adr1

Conversation

@titusfortner

Copy link
Copy Markdown
Member

🔗 Related Issues

ADR process itself depends on #17665 merging

Decision

BiDi is an implementation mechanism, not a public API. New capabilities are exposed as
high-level, protocol-neutral APIs, idiomatic per language, subject to our standard
deprecation policy. Everything in a BiDi namespace is internal: still reachable for
anything not yet wrapped, but not documented and subject to change without warning.

A binding conforms when:

  1. The supported API never references BiDi — no types, methods, properties, or entry points.
  2. BiDi implementation code (including everything generated from CDDL) is visibly internal — marked per language convention as not intended for public consumption.

Considered options

  1. Expose the whole protocol as a public API (Rejected)
    • not user-friendly syntax
  2. A supported-but-separate public protocol namespace (Rejected)
    • multiple implementations are confusing
  3. Internal implementation mechanism only (Accepted)

@titusfortner titusfortner added the A-needs decision TLC needs to discuss and agree label Jun 11, 2026
@qodo-code-review

qodo-code-review Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 11 rules

Grey Divider


Remediation recommended

1. Option 3 ambiguous 🐞 Bug ≡ Correctness
Description
In ADR 0001, considered option #3 lacks an explicit Accepted/Rejected marker and its explanation
reads like the rationale for the *opposite* choice (hiding BiDi from users), leaving the decision
record unclear about what was rejected and why. This makes the ADR less self-contained and increases
the risk of misinterpreting the intended cross-binding direction.
Code

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[R38-40]

+3. Allow methods and classes to reference BiDi
+   * users shouldn't need to know the underlying implementation mechanism, things should just work without additional ceremony
+4. Internal implementation mechanism only (Accepted)
Evidence
Option #3 is missing an Accepted/Rejected label and its supporting bullet reads like an argument for
hiding the implementation mechanism, while the ADR template expects options to state why they were
accepted or rejected.

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[38-40]
docs/decisions/0000-template.md[23-26]

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

## Issue description
ADR 0001’s “Considered options” list contains an option (#3) that does not indicate whether it was accepted or rejected, and its rationale text appears to argue for the internal-only approach rather than for allowing BiDi references. This ambiguity reduces the ADR’s usefulness as a durable record.

## Issue Context
The ADR template expects each considered option to state why it was accepted or rejected; the current option #3 neither labels its status nor clearly explains why it lost relative to option #4.

## Fix Focus Areas
- docs/decisions/0001-bidi-is-an-implementation-mechanism.md[38-40]

## Suggested fix
- Mark option #3 explicitly as `(Rejected)`.
- Rewrite its bullet to explain *why* allowing public API surfaces to reference BiDi was rejected (e.g., it exposes protocol details, couples the supported API to an implementation mechanism, harms portability/compatibility), and ensure it no longer reads like support for option #4.

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


2. Discussion link not PR 🐞 Bug ⚙ Maintainability
Description
ADR 0001’s “Discussion” field uses “PR pending” and links to an issue instead of linking to the
ADR’s PR, which conflicts with the template/process that defines the PR thread as the discussion
record. This weakens traceability from the ADR to its canonical review/discussion history.
Code

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[5]

+- Discussion: _PR pending_, https://github.com/SeleniumHQ/selenium/issues/17628
Evidence
The template explicitly instructs Discussion to link to the record’s PR, and the README states the
PR thread is the discussion record; ADR 0001 currently does not provide that PR link.

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[3-6]
docs/decisions/0000-template.md[6-9]
docs/decisions/README.md[30-36]

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 template/process expects the `Discussion:` field to link to the decision record’s PR (the canonical discussion record). ADR 0001 currently says `_PR pending_` and links to an issue instead.

### Issue Context
It’s fine to reference related issues, but per the template those should go under **Context** (or otherwise be secondary), while `Discussion:` should point at the ADR PR.

### Fix Focus Areas
- docs/decisions/0001-bidi-is-an-implementation-mechanism.md[3-6]
- docs/decisions/0000-template.md[6-9]
- docs/decisions/README.md[30-36]

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



Informational

3. Hidden ADR index 🐞 Bug ⚙ Maintainability ⭐ New
Description
The ADR index is introduced as a dotfile (docs/decisions/.index.md) and is not referenced from
docs/decisions/README.md, so readers who follow the documented ADR entry point won’t discover the
index page. This reduces ADR discoverability and makes it harder to navigate the decisions log over
time.
Code

docs/decisions/.index.md[R1-3]

+# Index of Decisions
+
+* 0001 - BiDi is an implementation mechanism, not a public API
Evidence
The new index file exists but is not referenced by the ADR directory’s main README/process doc, so
navigation to the index is not discoverable from the documented entry point.

docs/decisions/.index.md[1-3]
docs/decisions/README.md[1-45]

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 index is stored in `docs/decisions/.index.md` but `docs/decisions/README.md` does not link to it, so readers landing on the ADR directory documentation won't find the index.

## Issue Context
The ADR process documentation lives in `docs/decisions/README.md`, which serves as the primary entry point for how to use/consume ADRs in this repo.

## Fix Focus Areas
- docs/decisions/README.md[1-45]
- docs/decisions/.index.md[1-3]

## Suggested fix
Do one of the following:
1. Add a prominent link to `.index.md` near the top of `README.md` (e.g., “Index of Decisions”).
2. Rename `.index.md` to `index.md` or `INDEX.md` and link to it from `README.md`.
3. Alternatively, merge the index list into `README.md` and drop the separate index file.

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


4. Untracked items claim 🐞 Bug ⚙ Maintainability
Description
ADR 0001 says non-conforming public surfaces are “tracked below”, but the Binding status table
provides no notes/tracking links for any binding, so the ADR doesn’t actually provide the promised
tracking references.
Code

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[R44-56]

+- Users never need to know which protocol services a command.
+- Each new capability requires API design work across bindings before it ships.
+- Existing public surfaces that fail the conformance tests are brought in line per the deprecation policy, tracked below.
+
+## Binding status
+
+| Binding    | Status  | Notes / tracking link |
+|------------|---------|-----------------------|
+| Java       | pending |                       |
+| Python     | pending |                       |
+| Ruby       | pending |                       |
+| .NET       | pending |                       |
+| JavaScript | pending |                       |
Evidence
The ADR explicitly claims tracking exists “below”, but the subsequent binding-status table leaves
the tracking column empty for every binding; the decision template indicates that column is intended
to carry “Notes / tracking link” information.

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[44-56]
docs/decisions/0000-template.md[39-50]

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 states that non-conforming public surfaces are “tracked below”, but the Binding status table has an empty “Notes / tracking link” column across all bindings, so there is no actual tracking information.

## Issue Context
This is a documentation consistency/traceability problem inside the ADR itself.

## Fix Focus Areas
- docs/decisions/0001-bidi-is-an-implementation-mechanism.md[44-56]

## Expected change
Either:
1) Populate the “Notes / tracking link” cells with the relevant tracking issues/PRs per binding, or
2) Remove/adjust the “tracked below” wording until concrete tracking links exist.

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


5. Index entry not linked 🐞 Bug ⚙ Maintainability
Description
docs/decisions/.index.md lists ADR 0001 as plain text instead of a markdown link, so the “Index of
Decisions” is not navigable. Readers can’t click through to the actual record file.
Code

docs/decisions/.index.md[3]

+* 0001 - BiDi is an implementation mechanism, not a public API
Evidence
The index contains only plain text for ADR 0001, while the actual record lives in a separate slugged
markdown file; without a link the index doesn’t provide navigation.

docs/decisions/.index.md[1-3]
docs/decisions/0001-bidi-is-an-implementation-mechanism.md[1-2]

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

### Issue description
`docs/decisions/.index.md` is intended to be an index, but the ADR entry is plain text rather than a markdown link to the decision record file, which makes the index non-navigable.

### Issue Context
The ADR itself is stored in a slugged filename (`0001-bidi-is-an-implementation-mechanism.md`), so a link also removes ambiguity about the destination.

### Fix Focus Areas
- docs/decisions/.index.md[1-3]
- docs/decisions/0001-bidi-is-an-implementation-mechanism.md[1-2]

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


Grey Divider

Previous review results

Review updated until commit d5fcb13

Results up to commit f32ab0f


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


Remediation recommended
1. Discussion link not PR 🐞 Bug ⚙ Maintainability
Description
ADR 0001’s “Discussion” field uses “PR pending” and links to an issue instead of linking to the
ADR’s PR, which conflicts with the template/process that defines the PR thread as the discussion
record. This weakens traceability from the ADR to its canonical review/discussion history.
Code

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[5]

+- Discussion: _PR pending_, https://github.com/SeleniumHQ/selenium/issues/17628
Evidence
The template explicitly instructs Discussion to link to the record’s PR, and the README states the
PR thread is the discussion record; ADR 0001 currently does not provide that PR link.

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[3-6]
docs/decisions/0000-template.md[6-9]
docs/decisions/README.md[30-36]

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 template/process expects the `Discussion:` field to link to the decision record’s PR (the canonical discussion record). ADR 0001 currently says `_PR pending_` and links to an issue instead.

### Issue Context
It’s fine to reference related issues, but per the template those should go under **Context** (or otherwise be secondary), while `Discussion:` should point at the ADR PR.

### Fix Focus Areas
- docs/decisions/0001-bidi-is-an-implementation-mechanism.md[3-6]
- docs/decisions/0000-template.md[6-9]
- docs/decisions/README.md[30-36]

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



Informational
2. Index entry not linked 🐞 Bug ⚙ Maintainability
Description
docs/decisions/.index.md lists ADR 0001 as plain text instead of a markdown link, so the “Index of
Decisions” is not navigable. Readers can’t click through to the actual record file.
Code

docs/decisions/.index.md[3]

+* 0001 - BiDi is an implementation mechanism, not a public API
Evidence
The index contains only plain text for ADR 0001, while the actual record lives in a separate slugged
markdown file; without a link the index doesn’t provide navigation.

docs/decisions/.index.md[1-3]
docs/decisions/0001-bidi-is-an-implementation-mechanism.md[1-2]

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

### Issue description
`docs/decisions/.index.md` is intended to be an index, but the ADR entry is plain text rather than a markdown link to the decision record file, which makes the index non-navigable.

### Issue Context
The ADR itself is stored in a slugged filename (`0001-bidi-is-an-implementation-mechanism.md`), so a link also removes ambiguity about the destination.

### Fix Focus Areas
- docs/decisions/.index.md[1-3]
- docs/decisions/0001-bidi-is-an-implementation-mechanism.md[1-2]

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


Results up to commit 3d21495


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


Remediation recommended
1. Option 3 ambiguous 🐞 Bug ≡ Correctness
Description
In ADR 0001, considered option #3 lacks an explicit Accepted/Rejected marker and its explanation
reads like the rationale for the *opposite* choice (hiding BiDi from users), leaving the decision
record unclear about what was rejected and why. This makes the ADR less self-contained and increases
the risk of misinterpreting the intended cross-binding direction.
Code

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[R38-40]

+3. Allow methods and classes to reference BiDi
+   * users shouldn't need to know the underlying implementation mechanism, things should just work without additional ceremony
+4. Internal implementation mechanism only (Accepted)
Evidence
Option #3 is missing an Accepted/Rejected label and its supporting bullet reads like an argument for
hiding the implementation mechanism, while the ADR template expects options to state why they were
accepted or rejected.

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[38-40]
docs/decisions/0000-template.md[23-26]

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

## Issue description
ADR 0001’s “Considered options” list contains an option (#3) that does not indicate whether it was accepted or rejected, and its rationale text appears to argue for the internal-only approach rather than for allowing BiDi references. This ambiguity reduces the ADR’s usefulness as a durable record.

## Issue Context
The ADR template expects each considered option to state why it was accepted or rejected; the current option #3 neither labels its status nor clearly explains why it lost relative to option #4.

## Fix Focus Areas
- docs/decisions/0001-bidi-is-an-implementation-mechanism.md[38-40]

## Suggested fix
- Mark option #3 explicitly as `(Rejected)`.
- Rewrite its bullet to explain *why* allowing public API surfaces to reference BiDi was rejected (e.g., it exposes protocol details, couples the supported API to an implementation mechanism, harms portability/compatibility), and ensure it no longer reads like support for option #4.

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


Results up to commit 7b86053


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


Informational
1. Untracked items claim 🐞 Bug ⚙ Maintainability
Description
ADR 0001 says non-conforming public surfaces are “tracked below”, but the Binding status table
provides no notes/tracking links for any binding, so the ADR doesn’t actually provide the promised
tracking references.
Code

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[R44-56]

+- Users never need to know which protocol services a command.
+- Each new capability requires API design work across bindings before it ships.
+- Existing public surfaces that fail the conformance tests are brought in line per the deprecation policy, tracked below.
+
+## Binding status
+
+| Binding    | Status  | Notes / tracking link |
+|------------|---------|-----------------------|
+| Java       | pending |                       |
+| Python     | pending |                       |
+| Ruby       | pending |                       |
+| .NET       | pending |                       |
+| JavaScript | pending |                       |
Evidence
The ADR explicitly claims tracking exists “below”, but the subsequent binding-status table leaves
the tracking column empty for every binding; the decision template indicates that column is intended
to carry “Notes / tracking link” information.

docs/decisions/0001-bidi-is-an-implementation-mechanism.md[44-56]
docs/decisions/0000-template.md[39-50]

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 states that non-conforming public surfaces are “tracked below”, but the Binding status table has an empty “Notes / tracking link” column across all bindings, so there is no actual tracking information.

## Issue Context
This is a documentation consistency/traceability problem inside the ADR itself.

## Fix Focus Areas
- docs/decisions/0001-bidi-is-an-implementation-mechanism.md[44-56]

## Expected change
Either:
1) Populate the “Notes / tracking link” cells with the relevant tracking issues/PRs per binding, or
2) Remove/adjust the “tracked below” wording until concrete tracking links exist.

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


Qodo Logo

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add ADR process docs and record BiDi as internal implementation detail
📝 Documentation 🕐 10-20 Minutes

Grey Divider

Walkthroughs

Description
• Add an ADR/decision-record process, template, and approval rules under docs/decisions.
• Record the decision that BiDi is internal and must not appear in supported public APIs.
• Add a decisions index to make adopted proposals discoverable and citable.
Diagram
graph TD
  A([Contributor]) --> F[["docs/decisions/"]] --> B["ADR template"] --> C["BiDi ADR 0001"] --> D["Index"] --> E["Process README"]

  subgraph Legend
    direction LR
    _p([Person]) ~~~ _dir[["Directory"]] ~~~ _doc["Document"]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use GitHub Discussions/Issues as the canonical decision log
  • ➕ No new docs structure to maintain
  • ➕ Discussion context and decision live together in one place
  • ➕ Searchable and linkable with existing GitHub tooling
  • ➖ Harder to keep a stable, curated, single-source-of-truth record
  • ➖ More difficult to enforce immutability rules (superseding vs editing)
  • ➖ Less friendly for offline/docs-site consumption
2. Adopt an ADR tool/convention (e.g., adr-tools) with CLI automation
  • ➕ Standardized workflows (create/supersede/index) reduce process drift
  • ➕ Consistent naming/metadata via automation
  • ➖ Adds tooling overhead and cross-platform considerations
  • ➖ May not fit Selenium’s multi-repo/multi-binding governance model
  • ➖ Still requires human review for content correctness

Recommendation: The current approach (plain Markdown ADRs with a lightweight documented process and a simple index) is the best fit here: it keeps the decision record close to the codebase, is language/tool neutral, and is easy for all binding maintainers to follow without introducing new tooling dependencies.

Grey Divider

File Changes

Documentation (4)
.index.md Add decisions index entry for ADR 0001 +3/-0

Add decisions index entry for ADR 0001

• Introduces an index page for the decisions directory. Adds a first entry pointing to decision 0001 for discoverability.

docs/decisions/.index.md


0000-template.md Add ADR template with required sections and binding status table +56/-0

Add ADR template with required sections and binding status table

• Adds a standardized decision record template covering context, decision, options, consequences, and per-binding status tracking. Includes guidance on writing, mutability, and what belongs in the record vs the PR thread.

docs/decisions/0000-template.md


0001-bidi-is-an-implementation-mechanism.md Add ADR 0001 defining BiDi as internal, not a supported public API +49/-0

Add ADR 0001 defining BiDi as internal, not a supported public API

• Records the project-wide decision that BiDi is an implementation mechanism and must not appear in supported API surfaces. Defines conformance criteria and documents rejected alternatives and consequences.

docs/decisions/0001-bidi-is-an-implementation-mechanism.md


README.md Document ADR scope, process, approval rules, and immutability constraints +71/-0

Document ADR scope, process, approval rules, and immutability constraints

• Adds the decision-log charter for cross-binding semantics, including what requires a record and the proposal/discussion/acceptance workflow. Defines TLC review expectations and rules for superseding decisions and numbering stability.

docs/decisions/README.md


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 3d21495


## Decision

BiDi is an implementation mechanism, not a public API. New capabilities are exposed as

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.

Low-level API should be publicly available, it is power. But without warranties, spec is living doc. Not clear how to emphasize it per language. What I see: spec is trying to be not breaking changeable.

In .NET/C# we already funded a plan how to react on breaking changes. I really don't understand why low-level implementation should be internal.. .NET is ready for changes.

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 low-level api will be accessible through the internal namespace, as mentioned below:

Everything in a BiDi namespace is internal: still reachable for 
anything not yet wrapped, but not documented and subject to change without warning

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 7b86053

@diemol diemol left a comment

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.

💯% agreeing with this.

@qodo-code-review

Copy link
Copy Markdown
Contributor

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

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