Skip to content

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

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

[adr] BiDi is an implementation mechanism, not a public API#17670
titusfortner wants to merge 3 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 (3) 📘 Rule violations (0)

Grey Divider


Remediation recommended

1. Option 3 ambiguous 🐞 Bug ≡ Correctness ⭐ New
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. 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 3d21495

Results up to commit f32ab0f


🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (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


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

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.

1 participant