[rb] separate concerns between Service, DriverFinder, and Options by titusfortner · Pull Request #17564 · SeleniumHQ/selenium

titusfortner · 2026-05-24T16:44:41Z

I made a small change yesterday to allow Grid to run beta versions of browsers when browsers aren't pinned by bazel.

The change broke the build and I realized I made it in the wrong place because the concerns aren't well separated in these classes.

💥 What does this PR do?

Unneeded browser versions will always be stripped before getting sent to the driver

🔧 Implementation Notes

LocalDriver always calls DriverFinder to parse values from Options and Service no shortcuts
Option mutations only happen in LocalDriver not DriverFinder
Service no longer reaches into Options with const_get, DriverFinder will parse driver name from the service class

🤖 AI assistance

No substantial AI assistance used
AI assisted (complete below)
- Tool(s):
- What was generated:
- I reviewed all AI output and can explain the change

qodo-code-review · 2026-05-24T16:44:59Z

Review Summary by Qodo

Refactor driver path resolution to separate concerns between Service, DriverFinder, and Options

✨ Enhancement 🐞 Bug fix

Walkthroughs

Description

• Separate concerns between Service, DriverFinder, and Options classes
• DriverFinder now accepts nil options and parses driver from Service
• LocalDriver handles all Options mutations instead of DriverFinder
• Service no longer uses reflection to instantiate Options
• Browser version is cleared when binary is resolved by Selenium Manager

Diagram

flowchart LR
  LD["LocalDriver"] -->|calls with options| DF["DriverFinder"]
  DF -->|accepts nil options| S["Service"]
  DF -->|queries| SM["SeleniumManager"]
  LD -->|mutates| OPT["Options"]
  LD -->|sets executable_path| S
  S -->|launch calls| DF

File Changes

1. rb/lib/selenium/webdriver/common/driver_finder.rb ✨ Enhancement +14/-16

Refactor to accept nil options and simplify path resolution

rb/lib/selenium/webdriver/common/driver_finder.rb

2. rb/lib/selenium/webdriver/common/local_driver.rb ✨ Enhancement +4/-8

Move Options mutations to LocalDriver from DriverFinder

rb/lib/selenium/webdriver/common/local_driver.rb

3. rb/lib/selenium/webdriver/common/service.rb ✨ Enhancement +1/-6

Remove find_driver_path and reflection-based Options instantiation

rb/lib/selenium/webdriver/common/service.rb

View more (14)

4. rb/spec/integration/selenium/webdriver/chrome/service_spec.rb 🧪 Tests +1/-1

Update test to pass nil instead of Options instance

rb/spec/integration/selenium/webdriver/chrome/service_spec.rb

5. rb/spec/integration/selenium/webdriver/edge/service_spec.rb 🧪 Tests +1/-1

Update test to pass nil instead of Options instance

rb/spec/integration/selenium/webdriver/edge/service_spec.rb

6. rb/spec/integration/selenium/webdriver/firefox/service_spec.rb 🧪 Tests +1/-1

Update test to pass nil instead of Options instance

rb/spec/integration/selenium/webdriver/firefox/service_spec.rb

7. rb/spec/integration/selenium/webdriver/ie/service_spec.rb 🧪 Tests +1/-1

Update test to pass nil instead of Options instance

rb/spec/integration/selenium/webdriver/ie/service_spec.rb

8. rb/spec/integration/selenium/webdriver/safari/service_spec.rb 🧪 Tests +1/-1

Update test to pass nil instead of Options instance

rb/spec/integration/selenium/webdriver/safari/service_spec.rb

9. rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb 🐞 Bug fix +3/-3

Always set browser_version in options regardless of binary env

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb

10. rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb 🧪 Tests +4/-3

Update test expectations for Selenium Manager behavior

rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb

11. rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb 🧪 Tests +11/-0

Add test for nil options driver path resolution

rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb

12. rb/spec/unit/selenium/webdriver/edge/driver_spec.rb 🧪 Tests +4/-3

Update test expectations for Selenium Manager behavior

rb/spec/unit/selenium/webdriver/edge/driver_spec.rb

13. rb/spec/unit/selenium/webdriver/firefox/driver_spec.rb 🧪 Tests +4/-3

Update test expectations for Selenium Manager behavior

rb/spec/unit/selenium/webdriver/firefox/driver_spec.rb

14. rb/spec/unit/selenium/webdriver/ie/driver_spec.rb 🧪 Tests +4/-3

Update test expectations for Selenium Manager behavior

rb/spec/unit/selenium/webdriver/ie/driver_spec.rb

15. rb/spec/unit/selenium/webdriver/safari/driver_spec.rb 🧪 Tests +4/-3

Update test expectations for Selenium Manager behavior

rb/spec/unit/selenium/webdriver/safari/driver_spec.rb

16. rb/sig/lib/selenium/webdriver/common/driver_finder.rbs 📝 Documentation +1/-1

Update type signature for resolve_class_path method

rb/sig/lib/selenium/webdriver/common/driver_finder.rbs

17. rb/sig/lib/selenium/webdriver/common/service.rbs 📝 Documentation +0/-2

Remove find_driver_path method signature

rb/sig/lib/selenium/webdriver/common/service.rbs

qodo-code-review · 2026-05-24T16:45:00Z

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (5)

1. process_options clears browser_version 📘 Rule violation ≡ Correctness ⭐ New

Description

LocalDriver#process_options now unconditionally sets options.browser_version = nil whenever
options.binary is present, even if the binary was user-specified. This changes user-visible
capability output and can break clients relying on browser_version being sent when providing a
custom binary.

Code

rb/lib/selenium/webdriver/common/local_driver.rb[R51-55]

Evidence
PR Compliance ID 2 requires maintaining backward compatibility for user-facing behavior. The changed
code explicitly nulls browser_version whenever binary is present, which alters the capabilities
serialized via options.as_json and sent to sessions.
AGENTS.md: Maintain API/ABI compatibility for user-facing interfaces
rb/lib/selenium/webdriver/common/local_driver.rb[51-55]

Agent prompt

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

## Issue description
`LocalDriver#process_options` clears `options.browser_version` whenever `options.binary` is set. This is a user-visible behavior change because `browser_version` is a W3C capability and will no longer be sent to the driver/grid even when users explicitly set it.

## Issue Context
Before this PR, `browser_version` was only cleared in narrower cases (tied to `DriverFinder#browser_path?`). The new unconditional clearing can break existing integrations that set both `binary` and `browser_version` intentionally.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/local_driver.rb[51-55]

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

2. PATH executable rejected 🐞 Bug ≡ Correctness

Description

LocalDriver#process_options now always calls DriverFinder, and DriverFinder treats
Service#executable_path as a filesystem path and validates it via Platform.assert_executable. If
executable_path is a bare command name intended to be resolved via PATH (which
ChildProcess/Process.spawn supports), driver startup now fails early with a wrapped
NoSuchDriverError/WebDriverError.

Code

rb/lib/selenium/webdriver/common/local_driver.rb[R51-54]

Evidence
The new LocalDriver flow always instantiates DriverFinder; DriverFinder prioritizes
service.executable_path and validates it with Platform.assert_executable, which requires an
on-disk file. But the process launch path uses Process.spawn, which can run a bare command name
via PATH, so this new validation can block previously-working configurations.
rb/lib/selenium/webdriver/common/local_driver.rb[43-56]
rb/lib/selenium/webdriver/common/driver_finder.rb[50-71]
rb/lib/selenium/webdriver/common/platform.rb[139-145]
rb/lib/selenium/webdriver/common/child_process.rb[53-62]

Agent prompt

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

### Issue description
`LocalDriver#process_options` now always constructs a `DriverFinder`, which will route `service.executable_path` through `paths_from_service` and call `Platform.assert_executable(path)`. `Platform.assert_executable` requires `File.file?(path)` and therefore rejects bare executable names (e.g., `"chromedriver"`) even though the service ultimately starts via `Process.spawn`, which can resolve commands via `PATH`.

### Issue Context
This is a behavioral regression for users passing `Service.*(path: 'chromedriver')` (or setting `service.executable_path = 'chromedriver'`) expecting PATH lookup.

### Fix Focus Areas
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-71]
- rb/lib/selenium/webdriver/common/local_driver.rb[43-56]

### Suggested fix approach
- In `DriverFinder#paths_from_service`, treat values without a directory component as command names:
 - Option A (preferred): resolve via `ENV['PATH']` to a concrete file (and then `Platform.assert_executable` the resolved path).
 - Option B: skip `Platform.assert_executable` when `path` has no path separators and defer failure to process launch.
- Update log message to reflect that the path can come from the service instance, not only the service class.

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

3. Service#find_driver_path removed 📘 Rule violation ⚙ Maintainability

Description

The PR removes the public Service#find_driver_path method, which is a breaking API change for
downstream code that calls it. The removal is not preceded by a deprecation notice that points to an
alternative.

Code

rb/lib/selenium/webdriver/common/service.rb[L99-102]

Evidence
PR Compliance ID 1 prohibits breaking public API removals, and PR Compliance ID 6 requires
deprecating public functionality before removal with an alternative. In the PR branch,
Service#launch now resolves the driver path directly via DriverFinder.new(nil, self).driver_path
and the Service RBS no longer exposes find_driver_path, indicating the method was removed from the
public surface without a deprecation path.
AGENTS.md: Maintain API/ABI compatibility for user-facing changes
AGENTS.md: Deprecate public functionality before removal and provide an alternative in the deprecation message
rb/lib/selenium/webdriver/common/service.rb[90-101]
rb/sig/lib/selenium/webdriver/common/service.rbs[52-60]

Agent prompt

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

## Issue description
`Selenium::WebDriver::Service#find_driver_path` was removed. This is a breaking user-facing API change and it was not deprecated with a message that points users to the alternative.

## Issue Context
The code now resolves the driver path via `DriverFinder.new(nil, self).driver_path`, but callers that used `find_driver_path` will fail after upgrade.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/service.rb[90-101]
- rb/sig/lib/selenium/webdriver/common/service.rbs[52-60]

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

View more (1)

4. Nil options crashes Manager path 🐞 Bug ☼ Reliability

Description

When DriverFinder is created with options=nil (e.g., Service#launch), it calls Selenium Manager with
only --driver, but paths_from_manager still unconditionally reads and validates
output['browser_path']. If Selenium Manager omits browser_path (it is nullable in
SeleniumManagerOutput), Platform.assert_executable(nil) will raise and prevent the Service from
launching.

Code

rb/lib/selenium/webdriver/common/driver_finder.rb[R83-84]

Evidence
DriverFinder now supports options=nil by emitting only a --driver query, but still formats and
asserts browser_path unconditionally. Platform.assert_executable requires a non-nil path, and the
repo’s SeleniumManagerOutput model explicitly allows browserPath to be null, so this new call path
can raise at runtime during Service#launch.
rb/lib/selenium/webdriver/common/driver_finder.rb[50-99]
rb/lib/selenium/webdriver/common/service.rb[90-93]
rb/lib/selenium/webdriver/common/platform.rb[112-145]
java/src/org/openqa/selenium/manager/SeleniumManagerOutput.java[119-157]
java/src/org/openqa/selenium/manager/SeleniumManagerOutput.java[193-226]

Agent prompt

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

## Issue description
`DriverFinder.new(nil, service)` triggers a Selenium Manager call using only `--driver <executable>`, but `paths_from_manager` always expects `output['browser_path']` and always calls `Platform.assert_executable` on it. If Selenium Manager returns no browser path for this mode, service startup fails.

## Issue Context
This code path is exercised by `Service#launch`, which now constructs `DriverFinder` with `options=nil`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-99]
- rb/lib/selenium/webdriver/common/service.rb[88-93]

### Implementation sketch
- In `paths_from_manager`, only add/validate `browser_path` if it is present in the Selenium Manager output (non-nil and non-empty).
- Alternatively, branch behavior based on `@options`:
 - when `@options.nil?`: return `{driver_path: ...}` only, and skip browser_path conversion/assertion.
 - when `@options` present: keep current behavior.
- Add/adjust a unit test to cover Manager output missing `browser_path` for the `options=nil` case.

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

5. No unit test for version stripping 📘 Rule violation ☼ Reliability ⭐ New

Description

The new behavior that strips browser_version when binary is set is not covered by a unit test,
increasing regression risk. Add/adjust a unit test to assert the intended contract for when
browser_version should be removed from capabilities.

Code

rb/lib/selenium/webdriver/common/local_driver.rb[R51-55]

Evidence

PR Compliance ID 11 requires updated/extended unit tests when production behavior changes. The
changed process_options logic introduces a new contract (stripping browser_version based on
binary presence) but the unit driver specs shown do not assert this behavior, leaving it
unverified.

rb/lib/selenium/webdriver/common/local_driver.rb[51-55]
Best Practice: Learned patterns

Agent prompt

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

## Issue description
Production behavior was changed to clear `options.browser_version` when `options.binary` is present, but there is no unit test asserting the intended rules for this behavior.

## Issue Context
`browser_version` is part of W3C capabilities and impacts grid/session selection. A focused unit test should lock in whether version stripping happens only when the binary is auto-resolved vs also when user-specified.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/local_driver.rb[51-55]
- rb/spec/unit/selenium/webdriver/**/driver_spec.rb[1-120]

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

6. Browser context lost in launch 🐞 Bug ≡ Correctness ⭐ New

Description

Service#launch now calls DriverFinder with options=nil, which makes DriverFinder invoke Selenium
Manager with only "--driver <EXECUTABLE>" and without browser_name/browser_version/browser-path
inputs. This removes the codebase’s existing mechanism for providing browser context to driver
resolution and can result in selecting a driver without considering the target browser details.

Code

rb/lib/selenium/webdriver/common/service.rb[91]

Evidence
Service#launch now resolves the executable through DriverFinder with options=nil, and
DriverFinder#to_args explicitly drops all browser-related arguments in that case; the same class
shows that when options are present it does pass --browser and optional version/path,
demonstrating the intended browser-context inputs are being bypassed for Service#launch.
rb/lib/selenium/webdriver/common/service.rb[90-93]
rb/lib/selenium/webdriver/common/driver_finder.rb[82-93]
rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[80-98]

Agent prompt

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

## Issue description
`Service#launch` now resolves the driver via `DriverFinder.new(nil, self).driver_path`. With `options=nil`, `DriverFinder#to_args` returns only `['--driver', EXECUTABLE]`, so Selenium Manager is not given browser context (browser name/version/binary path) that the codebase otherwise provides when options exist.

## Issue Context
This change specifically affects code paths that call `Service#launch` directly (i.e., when `executable_path` isn’t set and you’re not going through `LocalDriver#process_options`). When `DriverFinder` is called with real `Options`, it passes `--browser` and optionally `--browser-version` / `--browser-path`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/service.rb[90-93]
- rb/lib/selenium/webdriver/common/driver_finder.rb[82-99]

### Possible fix directions
- In `Service#launch`, construct an appropriate default `Options` instance (without `const_get`) and pass it to `DriverFinder`, so Manager receives `--browser ...`.
- Or enhance `DriverFinder#to_args` for `options=nil` to infer and include minimal browser context from the `service` (e.g., mapping service class/EXECUTABLE to a browser name), rather than using `--driver` alone.

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

7. Unconditional browser_version in spec helper 📘 Rule violation ☼ Reliability

Description

test_environment.rb now always sets opts[:browser_version] even when a browser binary is
provided via *_BINARY env vars, removing prior gating. This can cause cross-environment/CI
failures when the specified binary version does not match browser_version (or when environments
intentionally pin binaries).

Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[314]

Evidence
PR Compliance ID 11 requires gating tests based on environment/feature support to avoid
cross-environment CI failures. The changed lines in chrome_options, edge_options, and
firefox_options now force opts[:browser_version] regardless of whether
CHROME_BINARY/EDGE_BINARY/FIREFOX_BINARY is set, removing the prior environment-based gate.
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[314-316]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[325-327]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[336-338]
Best Practice: Learned patterns

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 spec support helpers now unconditionally set `opts[:browser_version] = browser_version`, removing the previous `unless ENV.key?('..._BINARY')` gating. This can make tests fail in environments where a browser binary is pinned via env vars (version mismatch).

## Issue Context
This impacts cross-environment reliability (local dev, CI, RBE) because pinned binaries may not match the requested `browser_version`.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[314-316]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[325-327]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[336-338]

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

View more (3)

8. Misleading driver path log 🐞 Bug ◔ Observability

Description

DriverFinder#paths_from_service always logs that the driver path was "specified in service class",
but DriverFinder#paths now prefers service.executable_path (often set from env/user) before
service.class.driver_path. This can mislead troubleshooting by attributing the resolved path to the
wrong source.

Code

rb/lib/selenium/webdriver/common/driver_finder.rb[R66-70]

Evidence
paths now prioritizes @service.executable_path, which can originate from Service
initialization/env or user assignment, but paths_from_service still logs as if the value came from
the service class.
rb/lib/selenium/webdriver/common/driver_finder.rb[50-54]
rb/lib/selenium/webdriver/common/driver_finder.rb[66-70]
rb/lib/selenium/webdriver/common/service.rb[69-75]

Agent prompt

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

## Issue description
`DriverFinder#paths_from_service` emits a debug log claiming the executable path was provided by the *service class*, but `DriverFinder#paths` now passes instance-level `service.executable_path` into `paths_from_service` as well. This makes logs inaccurate when the path came from `Service#initialize` (env) or user assignment.

## Issue Context
After the refactor, `paths()` chooses `@service.executable_path || resolve_class_path`, so `paths_from_service()` is no longer exclusively used for the class-level `driver_path` source.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-71]
- rb/lib/selenium/webdriver/common/service.rb[69-75]

## Suggested fix approach
- Either (a) split into two methods/log messages: one for instance-provided executable_path vs class-provided driver_path, or (b) pass a `source:` parameter to `paths_from_service` and log accordingly (e.g., "specified on service instance" / "specified in service class").
- Keep behavior unchanged; only adjust the log message/source attribution.

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

9. ENV-dependent DriverFinder spec 🐞 Bug ☼ Reliability

Description

The new DriverFinder unit spec assumes Selenium Manager is called, but it constructs
Service.chrome which auto-populates executable_path from ENV['SE_CHROMEDRIVER']. If that env
var is set, DriverFinder will skip Selenium Manager entirely, causing the test’s
have_received(:binary_paths) expectation to fail.

Code

rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[R25-34]

Evidence
The spec calls Service.chrome, whose initializer assigns @executable_path from env_path when
no path is provided. DriverFinder then prefers service.executable_path over manager resolution, so
a set env var bypasses the call the test asserts.
rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[24-34]
rb/lib/selenium/webdriver/common/service.rb[69-75]
rb/lib/selenium/webdriver/common/driver_finder.rb[50-54]

Agent prompt

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

### Issue description
`driver_finder_spec` creates a real `Service.chrome` which reads `ENV['SE_CHROMEDRIVER']` by default. When that env var is set, `DriverFinder#paths` will take `service.executable_path` and never call `SeleniumManager.binary_paths`, breaking the test expectation.

### Issue Context
This can make the unit suite flaky across developer/CI environments where driver env vars are present.

### Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[24-46]
- rb/lib/selenium/webdriver/common/service.rb[69-75]
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-59]

### Suggested fix approach
- Wrap the spec (or at least the new example) in an `around`/`before` hook that temporarily unsets `SE_CHROMEDRIVER` and restores it after.
- Alternatively, use a service test double with `executable_path: nil` to force the Selenium Manager code path.

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

10. New mocks for SeleniumManager.binary_paths 📘 Rule violation ☼ Reliability

Description

Modified unit tests add new mocks/stubs for SeleniumManager.binary_paths (and
Platform.assert_executable), which can misrepresent real API contracts and reduce test
reliability. This conflicts with the guideline to avoid mocks in tests in favor of real
implementations/fakes at appropriate boundaries.

Code

rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb[R57-64]

Evidence
PR Compliance ID 7 discourages introducing mocks in tests. The changed unit tests explicitly stub
SeleniumManager.binary_paths (and Platform.assert_executable) to assert behavior, increasing
reliance on mocks rather than exercising real/fake contract behavior.
AGENTS.md: Avoid Mocks in Tests
rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb[57-64]
rb/spec/unit/selenium/webdriver/edge/driver_spec.rb[57-64]
rb/spec/unit/selenium/webdriver/firefox/driver_spec.rb[60-67]
rb/spec/unit/selenium/webdriver/ie/driver_spec.rb[58-65]
rb/spec/unit/selenium/webdriver/safari/driver_spec.rb[57-64]

Agent prompt

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

## Issue description
Modified unit tests now stub `SeleniumManager.binary_paths` (and `Platform.assert_executable`) to force behavior, which can diverge from real contracts and make tests less meaningful.

## Issue Context
Compliance prefers tests that use real implementations/fakes/integration boundaries rather than mocks that can misrepresent the true contract.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb[57-64]
- rb/spec/unit/selenium/webdriver/edge/driver_spec.rb[57-64]
- rb/spec/unit/selenium/webdriver/firefox/driver_spec.rb[60-67]
- rb/spec/unit/selenium/webdriver/ie/driver_spec.rb[58-65]
- rb/spec/unit/selenium/webdriver/safari/driver_spec.rb[57-64]

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

Previous review results

Review updated until commit 2627733

Results up to commit bbc6f00

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. Service#find_driver_path removed 📘 Rule violation ⚙ Maintainability

Description

The PR removes the public Service#find_driver_path method, which is a breaking API change for
downstream code that calls it. The removal is not preceded by a deprecation notice that points to an
alternative.

Code

rb/lib/selenium/webdriver/common/service.rb[L99-102]

Evidence
PR Compliance ID 1 prohibits breaking public API removals, and PR Compliance ID 6 requires
deprecating public functionality before removal with an alternative. In the PR branch,
Service#launch now resolves the driver path directly via DriverFinder.new(nil, self).driver_path
and the Service RBS no longer exposes find_driver_path, indicating the method was removed from the
public surface without a deprecation path.
AGENTS.md: Maintain API/ABI compatibility for user-facing changes
AGENTS.md: Deprecate public functionality before removal and provide an alternative in the deprecation message
rb/lib/selenium/webdriver/common/service.rb[90-101]
rb/sig/lib/selenium/webdriver/common/service.rbs[52-60]

Agent prompt

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

## Issue description
`Selenium::WebDriver::Service#find_driver_path` was removed. This is a breaking user-facing API change and it was not deprecated with a message that points users to the alternative.

## Issue Context
The code now resolves the driver path via `DriverFinder.new(nil, self).driver_path`, but callers that used `find_driver_path` will fail after upgrade.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/service.rb[90-101]
- rb/sig/lib/selenium/webdriver/common/service.rbs[52-60]

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

2. Nil options crashes Manager path 🐞 Bug ☼ Reliability

Description

When DriverFinder is created with options=nil (e.g., Service#launch), it calls Selenium Manager with
only --driver, but paths_from_manager still unconditionally reads and validates
output['browser_path']. If Selenium Manager omits browser_path (it is nullable in
SeleniumManagerOutput), Platform.assert_executable(nil) will raise and prevent the Service from
launching.

Code

rb/lib/selenium/webdriver/common/driver_finder.rb[R83-84]

Evidence
DriverFinder now supports options=nil by emitting only a --driver query, but still formats and
asserts browser_path unconditionally. Platform.assert_executable requires a non-nil path, and the
repo’s SeleniumManagerOutput model explicitly allows browserPath to be null, so this new call path
can raise at runtime during Service#launch.
rb/lib/selenium/webdriver/common/driver_finder.rb[50-99]
rb/lib/selenium/webdriver/common/service.rb[90-93]
rb/lib/selenium/webdriver/common/platform.rb[112-145]
java/src/org/openqa/selenium/manager/SeleniumManagerOutput.java[119-157]
java/src/org/openqa/selenium/manager/SeleniumManagerOutput.java[193-226]

Agent prompt

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

## Issue description
`DriverFinder.new(nil, service)` triggers a Selenium Manager call using only `--driver <executable>`, but `paths_from_manager` always expects `output['browser_path']` and always calls `Platform.assert_executable` on it. If Selenium Manager returns no browser path for this mode, service startup fails.

## Issue Context
This code path is exercised by `Service#launch`, which now constructs `DriverFinder` with `options=nil`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-99]
- rb/lib/selenium/webdriver/common/service.rb[88-93]

### Implementation sketch
- In `paths_from_manager`, only add/validate `browser_path` if it is present in the Selenium Manager output (non-nil and non-empty).
- Alternatively, branch behavior based on `@options`:
 - when `@options.nil?`: return `{driver_path: ...}` only, and skip browser_path conversion/assertion.
 - when `@options` present: keep current behavior.
- Add/adjust a unit test to cover Manager output missing `browser_path` for the `options=nil` case.

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

Results up to commit b63f205

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. PATH executable rejected 🐞 Bug ≡ Correctness

Description

LocalDriver#process_options now always calls DriverFinder, and DriverFinder treats
Service#executable_path as a filesystem path and validates it via Platform.assert_executable. If
executable_path is a bare command name intended to be resolved via PATH (which
ChildProcess/Process.spawn supports), driver startup now fails early with a wrapped
NoSuchDriverError/WebDriverError.

Code

rb/lib/selenium/webdriver/common/local_driver.rb[R51-54]

Evidence
The new LocalDriver flow always instantiates DriverFinder; DriverFinder prioritizes
service.executable_path and validates it with Platform.assert_executable, which requires an
on-disk file. But the process launch path uses Process.spawn, which can run a bare command name
via PATH, so this new validation can block previously-working configurations.
rb/lib/selenium/webdriver/common/local_driver.rb[43-56]
rb/lib/selenium/webdriver/common/driver_finder.rb[50-71]
rb/lib/selenium/webdriver/common/platform.rb[139-145]
rb/lib/selenium/webdriver/common/child_process.rb[53-62]

Agent prompt

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

### Issue description
`LocalDriver#process_options` now always constructs a `DriverFinder`, which will route `service.executable_path` through `paths_from_service` and call `Platform.assert_executable(path)`. `Platform.assert_executable` requires `File.file?(path)` and therefore rejects bare executable names (e.g., `"chromedriver"`) even though the service ultimately starts via `Process.spawn`, which can resolve commands via `PATH`.

### Issue Context
This is a behavioral regression for users passing `Service.*(path: 'chromedriver')` (or setting `service.executable_path = 'chromedriver'`) expecting PATH lookup.

### Fix Focus Areas
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-71]
- rb/lib/selenium/webdriver/common/local_driver.rb[43-56]

### Suggested fix approach
- In `DriverFinder#paths_from_service`, treat values without a directory component as command names:
 - Option A (preferred): resolve via `ENV['PATH']` to a concrete file (and then `Platform.assert_executable` the resolved path).
 - Option B: skip `Platform.assert_executable` when `path` has no path separators and defer failure to process launch.
- Update log message to reflect that the path can come from the service instance, not only the service class.

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

2. New mocks for SeleniumManager.binary_paths 📘 Rule violation ☼ Reliability

Description

Modified unit tests add new mocks/stubs for SeleniumManager.binary_paths (and
Platform.assert_executable), which can misrepresent real API contracts and reduce test
reliability. This conflicts with the guideline to avoid mocks in tests in favor of real
implementations/fakes at appropriate boundaries.

Code

rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb[R57-64]

Evidence
PR Compliance ID 7 discourages introducing mocks in tests. The changed unit tests explicitly stub
SeleniumManager.binary_paths (and Platform.assert_executable) to assert behavior, increasing
reliance on mocks rather than exercising real/fake contract behavior.
AGENTS.md: Avoid Mocks in Tests
rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb[57-64]
rb/spec/unit/selenium/webdriver/edge/driver_spec.rb[57-64]
rb/spec/unit/selenium/webdriver/firefox/driver_spec.rb[60-67]
rb/spec/unit/selenium/webdriver/ie/driver_spec.rb[58-65]
rb/spec/unit/selenium/webdriver/safari/driver_spec.rb[57-64]

Agent prompt

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

## Issue description
Modified unit tests now stub `SeleniumManager.binary_paths` (and `Platform.assert_executable`) to force behavior, which can diverge from real contracts and make tests less meaningful.

## Issue Context
Compliance prefers tests that use real implementations/fakes/integration boundaries rather than mocks that can misrepresent the true contract.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/chrome/driver_spec.rb[57-64]
- rb/spec/unit/selenium/webdriver/edge/driver_spec.rb[57-64]
- rb/spec/unit/selenium/webdriver/firefox/driver_spec.rb[60-67]
- rb/spec/unit/selenium/webdriver/ie/driver_spec.rb[58-65]
- rb/spec/unit/selenium/webdriver/safari/driver_spec.rb[57-64]

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

3. ENV-dependent DriverFinder spec 🐞 Bug ☼ Reliability

Description

The new DriverFinder unit spec assumes Selenium Manager is called, but it constructs
Service.chrome which auto-populates executable_path from ENV['SE_CHROMEDRIVER']. If that env
var is set, DriverFinder will skip Selenium Manager entirely, causing the test’s
have_received(:binary_paths) expectation to fail.

Code

rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[R25-34]

Evidence
The spec calls Service.chrome, whose initializer assigns @executable_path from env_path when
no path is provided. DriverFinder then prefers service.executable_path over manager resolution, so
a set env var bypasses the call the test asserts.
rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[24-34]
rb/lib/selenium/webdriver/common/service.rb[69-75]
rb/lib/selenium/webdriver/common/driver_finder.rb[50-54]

Agent prompt

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

### Issue description
`driver_finder_spec` creates a real `Service.chrome` which reads `ENV['SE_CHROMEDRIVER']` by default. When that env var is set, `DriverFinder#paths` will take `service.executable_path` and never call `SeleniumManager.binary_paths`, breaking the test expectation.

### Issue Context
This can make the unit suite flaky across developer/CI environments where driver env vars are present.

### Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[24-46]
- rb/lib/selenium/webdriver/common/service.rb[69-75]
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-59]

### Suggested fix approach
- Wrap the spec (or at least the new example) in an `around`/`before` hook that temporarily unsets `SE_CHROMEDRIVER` and restores it after.
- Alternatively, use a service test double with `executable_path: nil` to force the Selenium Manager code path.

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

Results up to commit 12435d6

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. Unconditional browser_version in spec helper 📘 Rule violation ☼ Reliability

Description

test_environment.rb now always sets opts[:browser_version] even when a browser binary is
provided via *_BINARY env vars, removing prior gating. This can cause cross-environment/CI
failures when the specified binary version does not match browser_version (or when environments
intentionally pin binaries).

Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[314]

Evidence
PR Compliance ID 11 requires gating tests based on environment/feature support to avoid
cross-environment CI failures. The changed lines in chrome_options, edge_options, and
firefox_options now force opts[:browser_version] regardless of whether
CHROME_BINARY/EDGE_BINARY/FIREFOX_BINARY is set, removing the prior environment-based gate.
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[314-316]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[325-327]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[336-338]
Best Practice: Learned patterns

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 spec support helpers now unconditionally set `opts[:browser_version] = browser_version`, removing the previous `unless ENV.key?('..._BINARY')` gating. This can make tests fail in environments where a browser binary is pinned via env vars (version mismatch).

## Issue Context
This impacts cross-environment reliability (local dev, CI, RBE) because pinned binaries may not match the requested `browser_version`.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[314-316]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[325-327]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[336-338]

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

2. Misleading driver path log 🐞 Bug ◔ Observability

Description

DriverFinder#paths_from_service always logs that the driver path was "specified in service class",
but DriverFinder#paths now prefers service.executable_path (often set from env/user) before
service.class.driver_path. This can mislead troubleshooting by attributing the resolved path to the
wrong source.

Code

rb/lib/selenium/webdriver/common/driver_finder.rb[R66-70]

Evidence
paths now prioritizes @service.executable_path, which can originate from Service
initialization/env or user assignment, but paths_from_service still logs as if the value came from
the service class.
rb/lib/selenium/webdriver/common/driver_finder.rb[50-54]
rb/lib/selenium/webdriver/common/driver_finder.rb[66-70]
rb/lib/selenium/webdriver/common/service.rb[69-75]

Agent prompt

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

## Issue description
`DriverFinder#paths_from_service` emits a debug log claiming the executable path was provided by the *service class*, but `DriverFinder#paths` now passes instance-level `service.executable_path` into `paths_from_service` as well. This makes logs inaccurate when the path came from `Service#initialize` (env) or user assignment.

## Issue Context
After the refactor, `paths()` chooses `@service.executable_path || resolve_class_path`, so `paths_from_service()` is no longer exclusively used for the class-level `driver_path` source.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-71]
- rb/lib/selenium/webdriver/common/service.rb[69-75]

## Suggested fix approach
- Either (a) split into two methods/log messages: one for instance-provided executable_path vs class-provided driver_path, or (b) pass a `source:` parameter to `paths_from_service` and log accordingly (e.g., "specified on service instance" / "specified in service class").
- Keep behavior unchanged; only adjust the log message/source attribution.

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

qodo-code-review · 2026-05-24T21:46:26Z

Persistent review updated to latest commit b63f205

qodo-code-review · 2026-05-25T00:19:23Z

Persistent review updated to latest commit 12435d6

qodo-code-review · 2026-05-25T02:15:31Z

Persistent review updated to latest commit 2627733

titusfortner requested review from aguspe and p0deje May 24, 2026 16:44

selenium-ci added the C-rb Ruby Bindings label May 24, 2026

qodo-code-review Bot reviewed May 24, 2026

View reviewed changes

Comment thread rb/lib/selenium/webdriver/common/driver_finder.rb

qodo-code-review Bot reviewed May 24, 2026

View reviewed changes

Comment thread rb/lib/selenium/webdriver/common/service.rb

[rb] separate concerns between Service, DriverFinder, and Options

b63f205

titusfortner force-pushed the beta_grid_rb branch from bbc6f00 to b63f205 Compare May 24, 2026 21:46

qodo-code-review Bot reviewed May 24, 2026

View reviewed changes

Comment thread rb/lib/selenium/webdriver/common/local_driver.rb

Merge branch 'trunk' into beta_grid_rb

12435d6

Merge branch 'trunk' into beta_grid_rb

2627733

qodo-code-review Bot reviewed May 25, 2026

View reviewed changes

Comment thread rb/lib/selenium/webdriver/common/local_driver.rb

titusfortner merged commit 5aade02 into trunk May 25, 2026
39 checks passed

titusfortner deleted the beta_grid_rb branch May 25, 2026 16:23

titusfortner mentioned this pull request May 25, 2026

[rb] fix using environment variables to set drivers #17571

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[rb] separate concerns between Service, DriverFinder, and Options#17564

[rb] separate concerns between Service, DriverFinder, and Options#17564
titusfortner merged 3 commits into
trunkfrom
beta_grid_rb

titusfortner commented May 24, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented May 24, 2026

Uh oh!

qodo-code-review Bot commented May 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented May 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-code-review Bot commented May 25, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented May 25, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

titusfortner commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💥 What does this PR do?

🔧 Implementation Notes

🤖 AI assistance

Uh oh!

qodo-code-review Bot commented May 24, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

titusfortner commented May 24, 2026 •

edited

Loading

qodo-code-review Bot commented May 24, 2026 •

edited

Loading

qodo-code-review Bot commented May 24, 2026 •

edited

Loading

qodo-code-review Bot commented May 25, 2026 •

edited

Loading

qodo-code-review Bot commented May 25, 2026 •

edited

Loading