[adr] Selenium Manager released API#17741
Open
titusfortner wants to merge 3 commits into
Open
Conversation
Contributor
PR Summary by QodoAdd ADR for independently released Selenium Manager API contract
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+5
| # 17741. Selenium Manager ships as an official, independently released tool | ||
|
|
||
| - Status: Proposed | ||
| - Discussion: https://github.com/SeleniumHQ/selenium/pull/17741 | ||
|
|
Contributor
There was a problem hiding this comment.
1. Missing decision date 🐞 Bug ⚙ Maintainability
The decision record omits the required - Date: YYYY-MM-DD metadata line, so the record doesn’t state when its status last changed and doesn’t conform to the decision template used in this repo.
Agent Prompt
### Issue description
The new decision record is missing the required `Date` metadata line (date the status last changed).
### Issue Context
The decision template includes `Status`, `Date`, and `Discussion` fields at the top of the file.
### Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[1-5]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+82
to
+85
| ## Consequences | ||
|
|
||
| - Once released, the contract constrains every `rust/` change: no switch, value, or output field | ||
| changes without a deprecation cycle, and the current defaults are locked. |
Contributor
There was a problem hiding this comment.
2. Missing binding status table 🐞 Bug ⚙ Maintainability
The record lacks a ## Binding status section/table, which the documented process uses to track per-binding implementation and which is the only section intended to be editable after acceptance.
Agent Prompt
### Issue description
The decision record is missing the `## Binding status` section and tracking table.
### Issue Context
The template includes a binding-status table, and the process doc says bindings track convergence there and that updating that table is the only post-acceptance edit allowed (besides the status line).
### Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[82-85]
- docs/decisions/0000-template.md[39-51]
- docs/decisions/README.md[28-45]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
11 rules 1. Missing binding status table
|
titusfortner
force-pushed
the
c/wizardly-blackwell-68a46f
branch
from
f87abc8 to
e90b495
Compare
Comment on lines
+37
to
+38
| - Current defaults must not change once released, and must all be documented | ||
| ([configuration reference](https://www.selenium.dev/documentation/selenium_manager/#configuration)). |
Contributor
There was a problem hiding this comment.
1. Defaults not enumerated 🐞 Bug ⚙ Maintainability
The ADR says “current defaults” are locked as part of the stable contract, but it doesn’t actually list which defaults are being locked and instead points to an external reference. This conflicts with the repo’s rule that decision records must be understandable without following links and leaves the contract ambiguous when defaults change.
Agent Prompt
### Issue description
The ADR locks “current defaults” but doesn’t enumerate them, requiring readers to follow an external link to know what is actually committed.
### Issue Context
`docs/decisions/README.md` states decision records must stand alone without requiring links. The ADR currently references an external configuration page instead of listing the defaults it intends to lock.
### Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[8-12]
- docs/decisions/17741-selenium-manager-released-api.md[37-38]
- docs/decisions/17741-selenium-manager-released-api.md[86-87]
- docs/decisions/README.md[63-64]
### Suggested change
Add an explicit, in-record list of the defaults that are being made part of the immutable contract (and keep the link as “background/further reading” if desired).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+28
| - *Unknown switch* → warn (tell the user to update SM) and ignore; the binding still gets valid | ||
| paths. A switch only adjusts resolution, so dropping it is safe. |
Contributor
There was a problem hiding this comment.
2. Overbroad unknown-switch claim 🐞 Bug ≡ Correctness
The ADR justifies ignoring unknown switches by claiming switches “only adjust resolution,” but Selenium Manager already has switches that affect telemetry, network/offline behavior, and cache deletion. Leaving this phrasing as-is is misleading and could cause future contract confusion about what kinds of new switches are safe to ignore under version skew.
Agent Prompt
### Issue description
The ADR says unknown switches can be ignored because “a switch only adjusts resolution,” which is not generally true for Selenium Manager switches.
### Issue Context
Selenium Manager’s current CLI already includes switches that change behavior beyond “resolution” (e.g., offline mode, telemetry opt-out, cache deletion). The ADR should avoid a blanket statement and instead define a constraint like: *any future switch added after 1.0 must be safe to ignore (advisory-only), otherwise it must be introduced via a mechanism that won’t be silently dropped by older binaries.*
### Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[24-34]
- rust/src/main.rs[115-155]
### Suggested change
Replace the sentence “A switch only adjusts resolution, so dropping it is safe.” with a more precise contract statement about which classes of future switches are permitted (i.e., only those that are safe to ignore), and call out that privacy/safety-affecting controls must not be introduced as ignorable switches.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit e90b495 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📄 The decision, rationale, options, and consequences are in the record file this PR adds
(
docs/decisions/selenium-manager-released-api.md). This body is proposal notes and logistics only.🔗 Related
📝 Proposal notes
so a binding and the Selenium Manager it invokes can differ in version. This record fixes the
forwards- and backwards-compatible contract, and the defaults, that requires.
committed default — and the obligations releasing independently creates: independent
versioning/distribution and telemetry. Additive features and bugs stay ordinary issues, not
contract.
🗣 Discussion
📌 Tracking
Tracking issue: (linked on acceptance)