[rb] fix using environment variables to set drivers

[titusfortner]

🔗 Related Issues

the current implementation doesn't apply to the typical use case
Unless a user is attempting to start the driver service before initializing the driver, the environment variable won't be used

💥 What does this PR do?

Builds on #17564 to put the environment variable check where the service class check is in the DriverFinder

🤖 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

💡 Additional Considerations

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Fix environment variables in driver path resolution

🐞 Bug fix

Walkthroughs

Description

• Move environment variable check to DriverFinder for consistent resolution
• Remove redundant env_path calls from Service initialization
• Ensure environment variables apply in typical driver usage patterns
• Refactor path resolution order: service path, env vars, then class path

Diagram

flowchart LR
  A["Service.initialize"] -->|removed env_path| B["Service.launch"]
  B -->|calls DriverFinder| C["DriverFinder.paths"]
  C -->|checks in order| D["service.executable_path"]
  D -->|or| E["env_path"]
  E -->|or| F["class_path"]
  F -->|returns| G["driver executable"]

Loading

File Changes

1. rb/lib/selenium/webdriver/common/driver_finder.rb 🐞 Bug fix +6/-2

Add environment variable check to path resolution

• Refactored path resolution to check environment variables before class path
• Renamed resolve_class_path to class_path for clarity
• Added new env_path method to fetch driver path from environment variables
• Updated resolution order: service executable, env vars, then class path

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

---

2. rb/lib/selenium/webdriver/common/service.rb 🐞 Bug fix +1/-6

Remove environment variable handling from Service

• Removed env_path call from Service.initialize method
• Removed env_path call from Service.launch method
• Removed env_path private method definition
• Delegated all path resolution to DriverFinder

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

---

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

Update type signatures for refactored methods

• Renamed method signature from resolve_class_path to class_path
• Added new env_path method signature
• Updated type hints to reflect new method structure

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

---

[qodo-code-review]

Code Review by Qodo

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

1. Safari TP browserName not set 🐞 Bug ≡ Correctness

Description

Safari::Options#browser_name now mutates @options[:browser_name], but the driver creation path
serializes capabilities via options.as_json without ever calling browser_name, so browserName
remains the initialize default 'safari' even when Safari.technology_preview! is enabled. This
breaks the Safari Technology Preview flow that expects browser_name to be `'Safari Technology
Preview'` when the flag is set before or after options are created.

Code

rb/lib/selenium/webdriver/safari/options.rb[R39-40]

Evidence

Safari::Options now sets browserName only inside browser_name, but local driver initialization
calls options.as_json directly; WebDriver::Options#as_json reads from @options without calling
browser_name, while the integration spec expects Safari Technology Preview to be applied even if
the flag is set after Options.safari is created.

rb/lib/selenium/webdriver/safari/options.rb[26-41]
rb/lib/selenium/webdriver/common/local_driver.rb[43-56]
rb/lib/selenium/webdriver/common/options.rb[71-78]
rb/lib/selenium/webdriver/common/options.rb[116-134]
rb/spec/integration/selenium/webdriver/safari/driver_spec.rb[40-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
`Safari::Options#browser_name` updates `@options[:browser_name]`, but the code path that builds capabilities for a local driver uses `options.as_json` and does not call `browser_name`. As a result, enabling `Safari.technology_preview!` does not change the serialized W3C `browserName` from `'safari'` to `'Safari Technology Preview'`.

## Issue Context
`WebDriver::Options#as_json` serializes from a dup of `@options` and does not invoke getters, so any logic placed only in `Safari::Options#browser_name` is skipped unless called explicitly.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari/options.rb[29-41]

## Suggested fix
Add/restore a Safari::Options `as_json` override that ensures the `browser_name` logic runs before calling `super`, e.g.:

```ruby
def as_json(*)
 browser_name # ensures @options[:browser_name] reflects technology_preview?
 super
end
```

(Alternatively, set `@options[:browser_name]` in a hook that is guaranteed to run before serialization, but `as_json` is the minimal, spec-aligned place given existing driver initialization flow.)

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

---

2. Safari.path now nil ✓ Resolved 📘 Rule violation ≡ Correctness

Description

Selenium::WebDriver::Safari.path now defaults to nil, removing the previous behavior of
providing/validating a default Safari binary path and raising actionable errors. This is a
user-visible behavior change that can break callers relying on a non-nil default or on the earlier
deterministic exceptions.

Code

rb/lib/selenium/webdriver/safari.rb[49]

Evidence

The updated implementation of Safari.path explicitly memoizes nil (@path ||= nil), which
changes the method’s default return value/behavior in a user-visible way and can break existing
consumers that relied on a default path or early validation.

AGENTS.md: Maintain API/ABI compatibility (no breaking changes without explicit instruction)
rb/lib/selenium/webdriver/safari.rb[48-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
`Safari.path` now memoizes `nil`, changing the public method’s behavior so it no longer provides a default Safari path nor performs early validation/OS checks. This is a backward-incompatible, user-visible behavior change.

## Issue Context
`Safari.path` is a public singleton method. With the new implementation, any caller expecting a default path (or early, actionable errors) will instead receive `nil` and may fail later with less clear errors.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari.rb[48-50]

## Suggested approach
- Reintroduce the previous default path and validation/error behavior (e.g., default to the Safari binary on macOS and raise a clear `WebDriverError` when unsupported/unavailable).
- If the new `nil` default is intentional, add a deprecation path and update callers/documentation accordingly so upgrades do not silently change behavior.

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

---

3. Missing Safari::Options#browser_name= 📘 Rule violation ≡ Correctness

Description

Defining Safari::Options#browser_name prevents the base Options.set_capabilities from generating
the standard browser_name= writer, breaking API compatibility for consumers who set W3C
capabilities via the normal setter. It also forces a computed value on every read, making overrides
impossible and potentially changing downstream capability payloads.

Code

rb/lib/selenium/webdriver/safari/options.rb[R39-40]

Evidence

In Safari::Options, def browser_name is newly defined, which causes the base
Options.set_capabilities to skip defining the browser_name getter/setter pair (it uses `next if
method_defined? key). This breaks API compatibility by removing browser_name=` specifically for
Safari options.

AGENTS.md: Maintain API/ABI compatibility (no breaking changes without explicit instruction)
rb/lib/selenium/webdriver/safari/options.rb[39-41]
rb/lib/selenium/webdriver/common/options.rb[54-66]

Agent prompt

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

## Issue description
`Safari::Options` now defines a custom `browser_name` method, which prevents `Selenium::WebDriver::Options.set_capabilities` from generating the normal `browser_name=` writer. This is a backward-incompatible API change and also makes it impossible for callers to override `browser_name`.

## Issue Context
`Options.set_capabilities` only defines both getter and setter when the getter method is not already defined. Because `Safari::Options` defines `browser_name`, the pair is skipped.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari/options.rb[39-41]
- rb/lib/selenium/webdriver/common/options.rb[54-66]

## Suggested approach
- Prefer restoring the prior pattern: remove the `browser_name` override and instead override `as_json` (or another serialization hook) to set `@options[:browser_name]` based on `Safari.technology_preview?` right before serialization; this keeps the standard accessor pair (`browser_name` and `browser_name=`) available.
- If keeping `browser_name` is required, explicitly add a compatible `browser_name=` writer and ensure `browser_name` does not overwrite user-provided values unintentionally.

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

---

View more (2)

4. Service#env_path removed ✓ Resolved 📘 Rule violation ≡ Correctness

Description

Selenium::WebDriver::Service#env_path was removed without a deprecation path, which is a breaking
change for callers and leaves the RBI/RBS contract inconsistent with the runtime implementation.
This can trigger runtime NoMethodError for existing consumers and also break RBS/Steep validation
during upgrades.

Code

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

Evidence

Compliance requires preserving public APIs and deprecating them before removal, yet the current
Service implementation no longer defines env_path while the public RBS signature still declares
def env_path. This mismatch between rb/lib/.../service.rb and rb/sig/.../service.rbs
demonstrates that the method was deleted from runtime code but remains part of the declared public
interface, causing both runtime failures for callers and signature/type-checking errors.

AGENTS.md: Maintain API/ABI compatibility for public functionality
AGENTS.md: Deprecate public functionality before removal, including guidance to alternatives
rb/lib/selenium/webdriver/common/service.rb[89-103]
rb/sig/lib/selenium/webdriver/common/service.rbs[48-56]
rb/sig/lib/selenium/webdriver/common/service.rbs[48-60]
rb/lib/selenium/webdriver/common/service.rb[89-105]

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#env_path` was removed from the Ruby implementation without a deprecation path, but the corresponding RBS signature still declares it, creating an API break for callers and leaving the type/signature contract out of sync with runtime behavior.

## Issue Context
- The Ruby signature file still exposes `env_path` as a public method (`def env_path`).
- The `Service` implementation no longer defines `env_path`, so existing callers will hit `NoMethodError` at runtime.
- The mismatch will also fail RBS/Steep (or other signature validation) because the method is declared but not implemented.
- Compliance requires maintaining API compatibility and deprecating public functionality before removal.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/service.rb[89-105]
- rb/sig/lib/selenium/webdriver/common/service.rbs[48-60]

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

---

5. Env var Service init regression ✓ Resolved 🐞 Bug ≡ Correctness

Description

Service#initialize no longer applies the DRIVER_PATH_ENV_KEY environment variable, but existing unit
specs expect Service.new.executable_path to reflect that env var immediately. This will fail
multiple unit tests and is a behavioral break for any caller reading executable_path before
DriverFinder/launch runs.

Code

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

Evidence

Service now only assigns the passed path to @executable_path, while unit specs explicitly set
ENV['SE_*DRIVER'] and expect service.executable_path to match the driver name immediately after
construction.

rb/lib/selenium/webdriver/common/service.rb[69-87]
rb/spec/unit/selenium/webdriver/chrome/service_spec.rb[153-165]
rb/spec/unit/selenium/webdriver/safari/service_spec.rb[118-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
`Service#initialize` no longer reads the driver-path environment variable (e.g., `SE_CHROMEDRIVER`) to populate `@executable_path`. The repo’s unit specs currently assert that `Service.new.executable_path` is derived from the env var, so the change will cause test failures and breaks the previously-tested behavior.

## Issue Context
The env-var lookup was removed from `Service#initialize` and moved into `DriverFinder`. That makes env vars apply later (when DriverFinder runs), but it also means `service.executable_path` stays `nil` after construction.

## Fix Focus Areas
Choose one consistent behavior and align code + tests:

- **Preserve existing behavior (least breaking):** make `Service#executable_path` (reader) fall back to ENV when `@executable_path` is nil (or re-introduce env assignment in `initialize`).
- **If behavior change is intended:** update the affected unit specs to assert env usage via `DriverFinder` or `Service#launch`, not via `Service#executable_path` immediately after `new`.

Relevant locations:
- rb/lib/selenium/webdriver/common/service.rb[69-92]
- rb/spec/unit/selenium/webdriver/chrome/service_spec.rb[153-173]
- rb/spec/unit/selenium/webdriver/safari/service_spec.rb[118-138]
- rb/spec/unit/selenium/webdriver/edge/service_spec.rb[163-183]
- rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[215-236]
- rb/spec/unit/selenium/webdriver/ie/service_spec.rb[153-173]
- rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[22-99]

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

---

6. RBS missing browser_name= ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Safari::Options now defines browser_name= at runtime, but rb/sig/.../safari/options.rbs only
declares browser_name and not the setter, so typed consumers cannot call `options.browser_name =
...` without type errors.

Code

rb/lib/selenium/webdriver/safari/options.rb[R39-41]

Evidence

The runtime implementation adds browser_name= but the RBS file for the same class only declares
browser_name, so the type interface is incomplete versus the Ruby implementation.

rb/lib/selenium/webdriver/safari/options.rb[39-45]
rb/sig/lib/selenium/webdriver/safari/options.rbs[13-16]

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::Safari::Options` defines a `browser_name=` setter in Ruby, but the RBS signature file does not declare this method. This creates a mismatch between runtime API and the typed interface.

### Issue Context
The PR introduced/updated `Safari::Options#browser_name=` in Ruby, and updated the RBS to include `browser_name`, but the setter signature was not added.

### Fix Focus Areas
- rb/lib/selenium/webdriver/safari/options.rb[39-45]
- rb/sig/lib/selenium/webdriver/safari/options.rbs[13-16]

### Suggested fix
Add a setter signature in `rb/sig/lib/selenium/webdriver/safari/options.rbs`, e.g.:
- `def browser_name=: (String value) -> void`
(or return `String` if you want to model Ruby’s assignment return value).

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

---

7. Test clobbers SE_CHROMEDRIVER ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The new DriverFinder env-precedence unit test overwrites ENV['SE_CHROMEDRIVER'] and then
unconditionally deletes it in ensure, which can clobber a pre-existing value from a developer/CI
environment and leak side effects into later examples. This makes the suite potentially
order-dependent and flaky across environments, undermining the expectation that tests remain
isolated and reliable.

Code

rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[R36-48]

Evidence

PR Compliance ID 13 requires tests to be reliable across environments, but this spec changes global
process state by setting ENV['SE_CHROMEDRIVER'] and then always deleting the key during cleanup,
which will remove any value that was already present before the test run and thereby alter the
environment for subsequent specs in the same process. Other nearby unit specs (e.g., Firefox service
specs handling SE_DEBUG) illustrate the safer approach of preserving and restoring the original
environment variable value to avoid polluting the suite when variables are externally set.

rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[36-48]
rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[29-35]
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 DriverFinder env-precedence spec sets `ENV['SE_CHROMEDRIVER']` and then unconditionally calls `ENV.delete('SE_CHROMEDRIVER')` in `ensure`, which can wipe a pre-existing value from the parent environment and leak state across examples.

## Issue Context
This test is intended to validate env-var precedence, but to comply with the expectation that test changes are reliable across environments (PR Compliance ID 13) and to avoid order-dependent failures, it should not permanently mutate global process environment. Other unit specs in this repo follow a safer pattern by preserving/restoring prior env values rather than always deleting them.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[36-48]

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

---

8. No test for env_path ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The new behavior that prioritizes the driver environment variable in DriverFinder#paths is not
covered by unit tests. This risks regressions and CI behavior differences across environments.

Code

rb/lib/selenium/webdriver/common/driver_finder.rb[R50-55]

Evidence

ID 12 requires updating/extending unit tests when production behavior changes. The driver resolution
logic now includes env_path, but the existing DriverFinder unit tests do not cover any
environment-variable-driven resolution path.

rb/lib/selenium/webdriver/common/driver_finder.rb[50-68]
rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[24-99]
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
`DriverFinder` now checks an environment variable (`DRIVER_PATH_ENV_KEY`) when resolving the driver path, but there is no unit test asserting this precedence and behavior.

## Issue Context
There are existing unit tests for `DriverFinder` behavior (class path, proc path, Selenium Manager calls), so adding a small/unit test for ENV precedence is feasible and keeps tests aligned with the changed behavior.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/driver_finder.rb[50-68]
- rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[24-99]

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

---

View more (1)

9. Misleading log for env path 🐞 Bug ◔ Observability

Description

When the driver path comes from an environment variable, DriverFinder routes through
paths_from_service and logs that the path was "specified in service class", which is inaccurate.
This can mislead debugging/telemetry when env vars are used.

Code

rb/lib/selenium/webdriver/common/driver_finder.rb[52]

Evidence

env_path is now part of the resolution chain and any non-nil value flows into
paths_from_service, which emits a "specified in service class" message even when the source is
ENV.

rb/lib/selenium/webdriver/common/driver_finder.rb[50-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` now checks `env_path` before `class_path`, but it still calls `paths_from_service` for any explicit path. `paths_from_service` logs that the path was specified in the service class, which becomes misleading when the path actually came from ENV.

## Issue Context
This impacts only debug output but makes it harder to understand how the driver path was resolved.

## Fix Focus Areas
- Update logging to distinguish sources (e.g., ENV vs service instance vs class driver_path), or pass a "source" argument into `paths_from_service`.

- rb/lib/selenium/webdriver/common/driver_finder.rb[50-75]

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

---

10. Hardcoded env key 🐞 Bug ⚙ Maintainability

Description

The new env-precedence spec hardcodes SE_CHROMEDRIVER instead of using
Chrome::Service::DRIVER_PATH_ENV_KEY, making the test unnecessarily brittle and less aligned with
the production lookup logic.

Code

rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[R37-48]

Evidence

The spec hardcodes the env var name, while production uses the service’s DRIVER_PATH_ENV_KEY
constant (defined on Chrome::Service) to decide which env var to read; using the constant in the
test keeps it aligned with the code under test.

rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[36-49]
rb/lib/selenium/webdriver/chrome/service.rb[23-28]
rb/lib/selenium/webdriver/common/driver_finder.rb[61-63]

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 env-precedence spec hardcodes the env var name (`'SE_CHROMEDRIVER'`) instead of referencing the service constant (`Chrome::Service::DRIVER_PATH_ENV_KEY`). This makes the test less future-proof and less directly tied to the production behavior it intends to validate.

### Issue Context
Production code reads the env var name via `@service.class::DRIVER_PATH_ENV_KEY`, and Chrome’s service defines that constant.

### Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[37-48]

### Suggested change
In the spec, introduce `env_key = Chrome::Service::DRIVER_PATH_ENV_KEY` (or `env_key = Service.chrome.class::DRIVER_PATH_ENV_KEY`) and use `ENV[env_key]` consistently for fetch/set/restore.

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

---

[qodo-code-review]

Persistent review updated to latest commit afac4b5

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4c61289

[qodo-code-review]

Code review by qodo was updated up to the latest commit 89f1169

[qodo-code-review]

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