[rb] improve selenium manager testing

[titusfortner]

🔗 Related Issues

Supersedes #16767. Ruby implementation of #16741.

💥 What does this PR do?

1. service spec files do not need special tags other than not to run on Grid. The changes in [rb] fix using environment variables to set drivers #17571 now make sure that DriverFinder will use those values instead of calling out to Selenium Manager.

2. Create a new driver finder spec that tests all the browsers and marks them os-sensitive.

3. Deletes pre-installed browsers in addition to drivers from bazel runs to ensure that the tests (specifically Edge which has special behavior) is properly exercised.

🔧 Implementation Notes

• Sets environment variables within a test to ensure that Selenium Manager is handling the browser and driver downloads

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude Code
  • What was generated: spec structure, helper extraction, BUILD/CI cleanup
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

These tests should be run without pin-browsers being set. Already true on Windows, I'll adjust for mac and linux in another PR

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

[qodo-code-review]

Review Summary by Qodo

Add Selenium Manager DriverFinder integration tests and fix platform path handling

🧪 Tests 🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Adds DriverFinder integration tests for Selenium Manager
  - Resolves executable driver and browser paths
  - Downloads driver and browser into Selenium cache
• Fixes latent bug in Platform.unix_path for nil separator handling
• Refactors BUILD configuration to use OS-specific pinned browser targets
• Removes redundant test tags and improves test organization

Diagram

flowchart LR
  A["DriverFinder Tests"] --> B["Driver Path Resolution"]
  A --> C["Browser Path Resolution"]
  A --> D["Cache Downloads"]
  E["Platform.unix_path Fix"] --> F["Nil Separator Handling"]
  G["BUILD Configuration"] --> H["OS-Specific Targets"]
  G --> I["Remove Redundant Tags"]

Loading

File Changes

1. rb/lib/selenium/webdriver/common/platform.rb 🐞 Bug fix +1/-1

Fix unix_path nil separator handling

rb/lib/selenium/webdriver/common/platform.rb

---

2. rb/spec/integration/selenium/webdriver/driver_finder_spec.rb 🧪 Tests +58/-0

Add DriverFinder integration tests

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb

---

3. rb/spec/integration/selenium/webdriver/spec_support/helpers.rb 🧪 Tests +6/-0

Add includes_path helper for cache verification

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

---

View more (7)

4. java/src/org/openqa/selenium/grid/BUILD.bazel ⚙️ Configuration changes +4/-4

Update to OS-specific pinned browser targets

java/src/org/openqa/selenium/grid/BUILD.bazel

---

5. rb/sig/lib/selenium/webdriver/common/options.rbs ✨ Enhancement +1/-1

Make options parameter optional in initialize

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

---

6. rb/sig/lib/selenium/webdriver/support/guards.rbs 🐞 Bug fix +1/-1

Fix block parameter signature in add_condition

rb/sig/lib/selenium/webdriver/support/guards.rbs

---

7. rb/spec/integration/selenium/webdriver/BUILD.bazel ⚙️ Configuration changes +22/-1

Add driver_finder tests with no_grid configuration

rb/spec/integration/selenium/webdriver/BUILD.bazel

---

8. rb/spec/integration/selenium/webdriver/chrome/BUILD.bazel ⚙️ Configuration changes +0/-4

Remove redundant tags from service test

rb/spec/integration/selenium/webdriver/chrome/BUILD.bazel

---

9. rb/spec/integration/selenium/webdriver/edge/BUILD.bazel ⚙️ Configuration changes +0/-4

Remove redundant tags from service test

rb/spec/integration/selenium/webdriver/edge/BUILD.bazel

---

10. rb/spec/integration/selenium/webdriver/firefox/BUILD.bazel ⚙️ Configuration changes +0/-4

Remove redundant tags from service test

rb/spec/integration/selenium/webdriver/firefox/BUILD.bazel

---

[qodo-code-review]

Code Review by Qodo

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

1. Guard block arity wrong 🐞 Bug ≡ Correctness

Description

The RBS for Guards#add_condition declares an optional zero-arg block, but the Ruby implementation
stores the block and later calls it with one argument (the guarded list). Passing a lambda with zero
parameters will raise ArgumentError at runtime, and the signature misleads Steep users about the API
contract.

Code

rb/sig/lib/selenium/webdriver/support/guards.rbs[23]

Evidence

The signature specifies a zero-arg block, but the stored block is invoked with one argument (list)
during guard evaluation, so a strict-arity lambda would raise and Steep typing is inaccurate.

rb/sig/lib/selenium/webdriver/support/guards.rbs[21-25]
rb/lib/selenium/webdriver/support/guards/guard_condition.rb[32-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
`Guards#add_condition` is typed with an optional block that takes **no arguments**, but the implementation invokes the provided callable with one argument (the guarded values list). This creates a real runtime hazard for lambdas and produces incorrect type information.

### Issue Context
`GuardCondition#satisfied?` passes `list` into `@execution.call(list)`, and `@execution` is built from the user-supplied block.

### Fix Focus Areas
- Update the RBS block type to accept one parameter (likely an `Array[untyped]`), e.g.:
 - `def add_condition: (untyped name, ?untyped condition) ?{ (Array[untyped]) -> untyped } -> untyped`
 (or `?{ (untyped) -> untyped }` if you prefer to keep it looser).

- rb/sig/lib/selenium/webdriver/support/guards.rbs[21-25]

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

---

2. Exclusive guards always skip 🐞 Bug ≡ Correctness

Description

The new "resolves the browser to its system install location" example uses `exclusive: [{...},
{...}], but multiple exclusive` guards are evaluated conjunctively, so the example is skipped
whenever any one guard is not satisfied. Because one guard requires browser: %i[safari ie] and the
other requires browser: :edge, the example can never run and provides no coverage.

Code

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[R69-71]

Evidence

The guard engine flattens an exclusive: array into multiple Guard objects, then skips when it
finds any exclusive guard that is not satisfied; therefore all exclusive guards must be satisfied
simultaneously. With two different browser constraints, that cannot happen, so the example is
always skipped.

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[69-82]
rb/lib/selenium/webdriver/support/guards.rb[66-93]
rb/lib/selenium/webdriver/support/guards/guard_condition.rb[41-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
`exclusive: [{browser: %i[safari ie]}, {browser: :edge, platform: :windows}]` creates two separate exclusive guards, and the guard engine skips the example if **any** exclusive guard is not satisfied. Since no run can satisfy both `browser: %i[safari ie]` and `browser: :edge`, the example never executes.

### Issue Context
This is intended to run for Safari/IE (any platform) OR Edge-on-Windows. Current `exclusive` semantics treat multiple exclusive guards as an AND.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[69-82]

### Suggested fix
Split into two examples (or restructure guards) so each example has a single exclusive guard:
- Example A: `exclusive: {browser: %i[safari ie]}`
- Example B: `exclusive: {browser: :edge, platform: :windows}`

Keep the shared body via a helper method if you want to avoid duplication.

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

---

3. Cleanup removes Windows browsers 📘 Rule violation ☼ Reliability

Description

The Windows disk cleanup step now unconditionally deletes system-installed Chrome/Firefox/Edge
without checking whether the current CI job will need those browsers later, violating the
requirement to validate preconditions and fail safely. This can break Windows browser-test jobs
(including Bazel-based Ruby/Python/Java workflows), especially for Edge where reliable
download/installation fallback is not guaranteed on Windows, and can also increase runtime and
flakiness due to forced large downloads.

Code

scripts/github-actions/free-disk-space.ps1[R39-42]

Evidence

PR Compliance ID 7 requires CI/automation changes to validate preconditions and behave fail-safe,
but the updated scripts/github-actions/free-disk-space.ps1 adds unconditional deletions of core
browser installation directories. The shared Bazel GitHub Actions workflow runs this cleanup on
Windows before executing Bazel targets, and the Windows CI matrix includes browser integration tests
(e.g., Chrome/Firefox/Edge on Windows), so removing the system browsers creates a realistic failure
mode. This risk is amplified because the repo’s Bazel pinned-browser setup only provides pinned
browser binaries for Linux/macOS (Windows falls back to system/default), and Selenium
Manager-related tests explicitly skip Edge downloads on Windows due to admin-install requirements,
meaning removing system Edge is likely to cause test failures rather than a reliable download-based
recovery.

scripts/github-actions/free-disk-space.ps1[39-42]
.github/workflows/bazel.yml[134-140]
.github/workflows/ci-ruby.yml[58-89]
.github/workflows/ci-python.yml[69-87]
.github/workflows/ci-java.yml[10-33]
scripts/github-actions/free-disk-space.ps1[39-43]
common/browsers.bzl[21-52]
py/private/browsers.bzl[17-57]
rust/tests/browser_download_tests.rs[25-64]
rust/src/lib.rs[498-620]
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
`scripts/github-actions/free-disk-space.ps1` unconditionally removes system-installed Chrome/Firefox/Edge on Windows, even though subsequent CI steps (including Bazel-driven browser integration tests) may still depend on those browsers and Windows lacks a consistent pinned-browser fallback. Update the cleanup logic and workflow usage to validate preconditions and fail safely by default, so browser-test jobs are not broken and Edge is not removed unless a known-good alternative is guaranteed.

## Issue Context
- PR Compliance ID 7 requires CI/automation to validate preconditions and adopt fail-safe behavior.
- The reusable Bazel GitHub Actions workflow executes this cleanup on Windows before running Bazel targets.
- Windows CI includes browser-test jobs (Chrome/Firefox/Edge), and the repo’s pinned-browser wiring only supplies pinned browser binaries for Linux/macOS (Windows falls through to system/default behavior).
- Edge is particularly risky on Windows because download/installation fallback is not reliable (admin-install requirements are a known constraint), so deleting system Edge can directly cause Edge test failures.

## Fix Focus Areas
- scripts/github-actions/free-disk-space.ps1[39-43]
- .github/workflows/bazel.yml[134-140]

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

---

View more (3)

4. Unconditional debug logging ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

SpecSupport::Helpers#includes_path? unconditionally calls warn, emitting debug output (including
full paths) to STDERR on every invocation. This will spam CI logs during the browser-matrix
integration tests and can obscure real warnings or contribute to log-size related flakiness.

Code

rb/spec/integration/selenium/webdriver/spec_support/helpers.rb[R149-151]

Evidence

The helper logs on every call via warn and is invoked in the new integration spec for both driver
and browser cache assertions, so the warning will be emitted repeatedly across runs.

rb/spec/integration/selenium/webdriver/spec_support/helpers.rb[146-152]
rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[40-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
`includes_path?` prints a `warn "DEBUG ..."` line unconditionally, which pollutes integration test output.

### Issue Context
This helper is used by the new DriverFinder integration spec and will be called repeatedly across the browser matrix, so the debug output will be amplified in CI.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/helpers.rb[149-151]

### Suggested fix
Remove the `warn` line entirely, or guard it behind an opt-in flag (e.g., `if ENV['SE_DEBUG_INCLUDES_PATH'] == 'true'`) so normal CI runs are silent.

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

---

5. ENV[k] = v with nil 📘 Rule violation ☼ Reliability

Description

The integration spec snapshots prior ENV values using ENV.fetch(..., nil) and restores them with
ENV[k] = v, which raises when v is nil (meaning the key was previously unset) and can leave
the global ENV mutated for subsequent tests. This violates the requirement to restore prior process
state by deleting keys that were previously absent and can make the new tests fail in clean CI where
the variables are not pre-defined.

Code

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[R43-50]

Evidence

In rb/spec/integration/selenium/webdriver/driver_finder_spec.rb, both tmpdir/cache-related
examples capture “original” environment values using ENV.fetch(key, nil), so any missing/unset
variable is recorded as nil. In the corresponding ensure blocks they restore with
originals.each { |k, v| ENV[k] = v }, which will attempt ENV[k] = nil for previously-unset keys;
Ruby raises TypeError on assigning nil to ENV, causing the example to fail and preventing
cleanup, thereby leaking ENV mutations into subsequent tests. A correct, nil-safe restore pattern
(also demonstrated elsewhere in the repo’s unit specs) is to delete the key when the original value
was nil rather than assigning nil.

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[42-50]
rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[55-63]
rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[42-63]
rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[36-49]
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 integration specs mutate global `ENV` keys and attempt to restore them in `ensure`, but they capture missing keys as `nil` via `ENV.fetch(..., nil)` and then restore with `ENV[k] = v`. When `v` is `nil` (meaning the variable was originally unset), this restoration raises `TypeError` and can prevent reliable cleanup, leaking side effects across tests.

## Issue Context
The requirement is to preserve prior global process state: if an environment variable was previously missing, cleanup must remove it (delete the key) rather than trying to set it to `nil`. This affects both cache-related examples in `driver_finder_spec.rb`, and the repo already demonstrates the safe pattern elsewhere by deleting keys when the original was `nil`.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[42-50]
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[55-63]

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

---

6. Cache path check fails ✓ Resolved 🐞 Bug ≡ Correctness

Description

SpecSupport::Helpers#includes_path? converts paths using Platform.unix_path and then checks for
a "#{root}/" prefix, which will be false for Windows-style paths after unix_path turns / into
\. This will cause the new DriverFinder cache assertions to fail on Windows even when Selenium
Manager correctly downloads into the cache.

Code

rb/spec/integration/selenium/webdriver/spec_support/helpers.rb[R146-149]

Evidence

includes_path? uses Platform.unix_path then checks for a forward-slash-delimited prefix; on
Windows unix_path normalizes / to \, while other helper code uses windows_path specifically
to produce forward slashes for Windows paths.

rb/spec/integration/selenium/webdriver/spec_support/helpers.rb[146-150]
rb/lib/selenium/webdriver/common/platform.rb[121-127]
rb/spec/integration/selenium/webdriver/spec_support/helpers.rb[44-54]
rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[39-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
`includes_path?` currently mixes Windows normalization (via `Platform.unix_path`, which normalizes to OS separators) with a hard-coded `'/'` prefix check. On Windows this makes the check always return false.

### Issue Context
- `Platform.unix_path` normalizes by converting `File::ALT_SEPARATOR` to `File::SEPARATOR`. On Windows, that converts `/` to `\\`.
- `includes_path?` then assumes `/` separators when doing `start_with?("#{root}/")`.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/helpers.rb[146-150]

### Suggested fix
Update `includes_path?` to normalize *both* `path` and `root` to a consistent separator before comparing, e.g.:
- Convert backslashes to forward slashes using `tr('\\', '/')` (works cross-platform).
- `File.expand_path` both values to avoid relative-path surprises.
- On Windows, consider case-insensitive comparison for drive letters (`downcase`) before `start_with?`.

Example implementation:
```ruby
def includes_path?(path, root)
 path = File.expand_path(path).tr('\\', '/')
 root = File.expand_path(root).tr('\\', '/').chomp('/')
 if WebDriver::Platform.windows?
   path = path.downcase
   root = root.downcase
 end
 path.start_with?("#{root}/")
end
```

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

---

7. os-sensitive tag filtered ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new driver_finder_spec target sets tags = ["os-sensitive"], but rb_integration_test filters
that tag to only apply to specific browsers, so chrome/firefox/ie variants of this test will not
actually be tagged os-sensitive. This breaks tag-based selection/exclusion consistency (some browser
variants will be included/excluded while others won’t).

Code

rb/spec/integration/selenium/webdriver/BUILD.bazel[R69-71]

Evidence

BUILD.bazel assigns tags = ["os-sensitive"] to the driver_finder_spec target while also running it
on DEFAULT_BROWSERS + ["ie"]. In rb/spec/tests.bzl, os-sensitive is defined as a filtered tag
and is only applied (via local_tags) for browsers listed in _BROWSER_TAG_FILTERS["os-sensitive"]
(chrome-beta/edge/firefox-beta/safari), so the tag is dropped for other browsers like
chrome/firefox/ie.

rb/spec/integration/selenium/webdriver/BUILD.bazel[65-76]
rb/spec/tests.bzl[169-179]
rb/spec/tests.bzl[12-21]
rb/spec/tests.bzl[85-93]
rb/spec/tests.bzl[133-141]

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` is configured with `tags = ["os-sensitive"]`, but the `rb_integration_test` macro treats `os-sensitive` as a *browser-filtered* tag. As a result, the `driver_finder_spec-{chrome,firefox,ie}` targets won’t actually receive the `os-sensitive` tag, making tag filtering inconsistent across browser variants.

### Issue Context
- `rb/spec/integration/selenium/webdriver/BUILD.bazel` adds a dedicated `rb_integration_test` stanza for `_NO_GRID` with `tags = ["os-sensitive"]`.
- `rb/spec/tests.bzl` filters `os-sensitive` to only apply to `chrome-beta`, `edge`, `firefox-beta`, and `safari`.

### Fix Focus Areas
Choose one of these approaches:
1) If `driver_finder_spec` should be os-sensitive for *all* browsers, use a universal tag (one that is not filtered) or adjust tag filtering to include all browsers this target runs on.
2) If it is only os-sensitive for some browsers, set tags in a way that matches the intended browsers (e.g., update `_BROWSER_TAG_FILTERS["os-sensitive"]` or use a new tag name).

- rb/spec/integration/selenium/webdriver/BUILD.bazel[65-76]
- rb/spec/tests.bzl[169-179]

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

---

8. Pinned binaries bypassed 🐞 Bug ☼ Reliability

Description

driver_finder_spec constructs Options/Service directly (without consuming Bazel-provided pinned
browser/driver env vars), so DriverFinder is forced down the Selenium Manager resolution path even
when pin_browsers is enabled by default/on RBE. This creates a divergent execution mode from the
rest of the integration suite and can cause unnecessary managed resolution/download behavior in
environments that are explicitly configured to use pinned artifacts.

Code

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[R27-30]

Evidence

RBE explicitly enables pinned browsers, and the Bazel integration targets export pinned
browser/driver env vars that the Ruby harness consumes to avoid managed resolution. The new spec
doesn’t use those harness helpers (or otherwise apply pinned paths), so it necessarily runs
DriverFinder without pinned inputs even under a pinned-browsers build configuration.

.bazelrc.remote[10-12]
common/BUILD.bazel[5-15]
rb/spec/tests.bzl[12-107]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[295-359]
rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[27-30]

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` builds `options` and `service` via `WebDriver::Options.send(browser)` / `WebDriver::Service.send(browser)`, which does not apply the Bazel “pinned browser/driver” env vars (e.g., `CHROME_BINARY`, `CHROMEDRIVER_BINARY`, etc.) that the rest of the Ruby integration harness uses. When `pin_browsers` is enabled (default and explicitly enabled on RBE), this spec still forces DriverFinder to resolve via Selenium Manager.

### Issue Context
- Remote (RBE) builds explicitly set `--//common:pin_browsers`.
- The Bazel Ruby integration harness uses pinned env vars to set `service.executable_path` and `options.binary`.
- This spec bypasses those helpers, so it exercises a different code path than the rest of the suite under the same Bazel config.

### Fix Focus Areas
Choose one:
1) **Skip/guard these examples when pinned env vars are present** (aligns with the expectation that these tests should run only when unpinned).
2) **Plumb pinned paths into the spec’s `options`/`service`** (e.g., construct options/service the same way the harness does, then adjust the test goal accordingly).

- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[27-30]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[295-359]
- rb/spec/tests.bzl[12-107]
- .bazelrc.remote[10-12]
- common/BUILD.bazel[5-15]

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

---

9. Browser path nil flake 🐞 Bug ☼ Reliability

Description

driver_finder_spec always asserts Platform.assert_executable(driver_finder.browser_path), but
DriverFinder returns no :browser_path when a driver path is supplied via service.executable_path,
the SE_* driver env var, or Service.driver_path (paths_from_service only returns :driver_path). In
those environments browser_path becomes nil and Platform.assert_executable will raise, making the
new test non-hermetic and potentially bypassing Selenium Manager coverage.

Code

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[R36-38]

Evidence

The spec asserts browser_path is executable, but DriverFinder explicitly skips Selenium Manager when
a driver path is provided by service/env/class and in that path it returns only {driver_path: ...},
so browser_path becomes nil. Platform.assert_executable calls File.file?(path), which will raise
when path is nil.

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[26-38]
rb/lib/selenium/webdriver/common/driver_finder.rb[50-75]
rb/lib/selenium/webdriver/common/platform.rb[133-145]
rb/lib/selenium/webdriver/chrome/service.rb[23-28]

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` assumes `DriverFinder#browser_path` is always present and executable. However, `DriverFinder` skips Selenium Manager entirely when a driver path is configured via the service/env/class, and in that code path it does not populate `:browser_path`, making the spec fail (and/or not actually test Selenium Manager).

### Issue Context
`DriverFinder#paths` selects `paths_from_service` whenever a driver path is provided by the service (`executable_path`), the service’s env key, or `Service.driver_path`.

### Fix Focus Areas
- Ensure the spec is hermetic by temporarily clearing any driver-path overrides before asserting on `browser_path` (e.g., delete `ENV[service.class::DRIVER_PATH_ENV_KEY]`, and set `service.class.driver_path = nil` and `service.executable_path = nil` within the example).
- Alternatively, update the assertion to handle the documented behavior: only assert executability when `driver_finder.browser_path?` is true, and add a separate assertion that ensures the manager path is exercised when that’s the intent.

- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[26-38]

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

---

View more (5)

10. Cleanup removes Linux browsers 📘 Rule violation ☼ Reliability

Description

The Linux disk cleanup step now unconditionally removes installed browsers (chrome, firefox,
msedge) without validating that the current CI job won’t need system browsers later. This can
cause browser-related jobs on Ubuntu to fail if they depend on system installs, violating the CI
precondition/failed-safe automation requirement.

Code

scripts/github-actions/free-disk-space.sh[R49-52]

Evidence

PR Compliance ID 7 requires automation to validate assumptions and fail safe. The new lines in
free-disk-space.sh delete browser directories, while the Bazel workflow runs this cleanup step for
Ubuntu and workflows can request browser-related jobs on Ubuntu (non-empty browser input), so the
cleanup should be gated/validated.

scripts/github-actions/free-disk-space.sh[49-52]
.github/workflows/bazel.yml[134-136]
.github/workflows/ci-python.yml[58-67]
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
`free-disk-space.sh` now deletes pre-installed browsers unconditionally. CI workflows can run Bazel jobs with browser-related setup on Ubuntu; the cleanup should validate preconditions and avoid removing potentially-required system dependencies unless explicitly safe.

## Issue Context
The reusable Bazel workflow runs this script for Ubuntu jobs, including jobs that set a non-empty `browser` input.

## Fix Focus Areas
- scripts/github-actions/free-disk-space.sh[49-52]

### Suggested approach
- Make browser deletion opt-in via an environment variable (default off), or
- Only delete browsers when the workflow explicitly indicates browsers are pinned/SM-downloaded and not needed from system paths.

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

---

11. Edge alias bypasses guard 🐞 Bug ≡ Correctness

Description

The Windows Edge exception guard matches only browser: :edge, so running the suite with the
supported WD_SPEC_DRIVER=microsoftedge results in browser == :microsoftedge and the guard will
not apply. In that configuration, the example will run on Windows even though the guard’s reason
indicates Edge behavior there should be excluded.

Code

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[R55-56]

Evidence

The guard uses browser: :edge, but the test environment derives GlobalTestEnv.browser from
WD_SPEC_DRIVER and Options explicitly supports :microsoftedge; guard conditions perform exact
symbol matching, so :edge will not match :microsoftedge and the Windows Edge exclusion will not
trigger.

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[54-56]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[41-65]
rb/lib/selenium/webdriver/common/options.rb[45-52]
rb/lib/selenium/webdriver/support/guards/guard_condition.rb[32-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 `except:` metadata for the “downloads the browser into the Selenium cache” example excludes Windows Edge only when `GlobalTestEnv.browser` equals `:edge`, but the test environment can legitimately produce `:microsoftedge`. Because guard matching is exact, the Windows Edge exclusion is bypassed under `WD_SPEC_DRIVER=microsoftedge`.

### Issue Context
- `GlobalTestEnv.browser` comes from `WD_SPEC_DRIVER` and is converted to a symbol.
- `Options` supports the `:microsoftedge` alias, so this is a valid configuration.
- Guard matching uses direct `include?` against the configured value.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[55-56]

### Suggested fix
Update the guard to include the alias, e.g.:
- change `{browser: :edge, platform: :windows, ...}` to `{browser: %i[edge microsoftedge], platform: :windows, ...}`

(Alternative: normalize `GlobalTestEnv.browser` to `:edge` for edge aliases, but that’s broader scope.)

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

---

12. Edge cache test excluded ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The "downloads the browser into the Selenium cache" example is guarded with `except: {browser:
%i[safari ie edge]...}`, so it will never run for Edge even though this spec is executed in an Edge
browser matrix. This removes coverage for validating Edge browser downloads into SE_CACHE_PATH
under SE_FORCE_BROWSER_DOWNLOAD.

Code

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[55]

Evidence

The driver_finder spec is configured to run across a browser matrix including Edge, but this
specific example is marked except for browser: :edge. The guard system treats a satisfied
except as pending/skip behavior, and the suite wires the runtime :browser condition from
GlobalTestEnv.browser, so the Edge run will not execute this example.

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[54-56]
rb/spec/integration/selenium/webdriver/BUILD.bazel[65-80]
rb/spec/integration/selenium/webdriver/spec_helper.rb[72-85]
rb/lib/selenium/webdriver/support/guards.rb[50-57]
rb/lib/selenium/webdriver/support/guards.rb[90-93]

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.rb` excludes `browser: :edge` from the "downloads the browser into the Selenium cache" example. Since the spec target is executed for Edge via the Bazel browser matrix, this prevents validating browser-download caching behavior for Edge at all.

### Issue Context
The guard system supports combining multiple conditions (e.g., `browser` + `platform`), and the suite already provides `:platform` and `:browser` conditions.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[54-56]

### Suggested change
Decide the intended behavior:
- If Edge should only be excluded on specific platforms (e.g., Windows where Edge may already be present), make the exclusion conditional, e.g. add `platform: :windows` (or the relevant platform(s)).
- Otherwise, remove `:edge` from the exclusion list and/or update the guard reason to reflect the actual rationale for excluding Edge from browser-download caching verification.

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

---

13. Weak cache path assertion 🐞 Bug ≡ Correctness

Description

The cache-download specs only assert that the resolved path string contains
File.basename(cache_dir), which does not guarantee the driver/browser was actually downloaded
under the configured SE_CACHE_PATH. This can allow false-positive test passes (while still being
Windows-tolerant) if the returned path happens to contain the same basename substring but is not
inside the temp cache directory.

Code

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[R47-48]

Evidence

The test sets SE_CACHE_PATH to a temp directory but only checks for the cache dir *basename* as a
substring of the resolved path, which is not equivalent to asserting the resolved path is located
within that directory. The same weak assertion pattern is used for both driver and browser cache
tests.

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[42-52]
rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[56-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
The test currently checks `include(File.basename(cache_dir))`, which is only a substring match and does not prove the resolved path is actually *inside* `cache_dir`.

### Issue Context
The basename approach was chosen to avoid brittle comparisons on Windows (e.g., 8.3 short names in earlier path segments). We can keep that robustness while making the assertion structurally correct by verifying directory ancestry rather than substring containment.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[42-52]
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[56-66]

### Suggested approach
Replace the substring assertion with an ancestry check that’s resilient to path formatting/casing/short-name differences:

```ruby
require 'pathname'

resolved = Pathname.new(driver_finder.driver_path)
cache = Pathname.new(cache_dir)

expect(resolved.ascend.any? { |p| File.exist?(p) && File.identical?(p.to_s, cache.to_s) }).to be(true)
```

Apply the same pattern to `browser_path`.

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

---

14. Options RBS kwargs mismatch 🐞 Bug ⚙ Maintainability

Description

The RBS for Selenium::WebDriver::Options#initialize still declares an (optional) positional Hash
argument, but the Ruby implementation is initialize(**opts) with keyword arguments. This leaves
the type signature misleading and can allow typed call sites to type-check while raising
ArgumentError at runtime when passing a positional hash on Ruby 3.

Code

rb/sig/lib/selenium/webdriver/common/options.rbs[36]

Evidence

The Ruby implementation takes keyword arguments (**opts), while the RBS signature is a positional
hash (now optional), so the signature does not reflect how the method is actually called/validated
by Ruby 3 keyword arg rules.

rb/lib/selenium/webdriver/common/options.rb[69-78]
rb/sig/lib/selenium/webdriver/common/options.rbs[34-37]

Agent prompt

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

### Issue description
`Options#initialize` is defined as `def initialize(**opts)` in Ruby, but the RBS signature describes an optional positional `Hash`. That does not accurately model the public API and can mask real runtime errors in typed usage.

### Issue Context
The PR made the positional hash optional, which helps allow `Options.new` with no args, but it still doesn’t match the keyword-args form.

### Fix Focus Areas
- rb/sig/lib/selenium/webdriver/common/options.rbs[34-38]

### Suggested fix
Change the RBS initializer signature from a positional hash to keyword arguments to match the implementation. For example:
```rbs
def initialize: (**String | Symbol | Numeric | bool opts) -> void
```
(or `**untyped` if the exact keyword set is intentionally flexible).

Also verify the chosen union type matches what `Options#initialize` actually stores in `@options`.

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

---

[selenium-ci]

Thank you, @titusfortner for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8ebdeb8

[qodo-code-review]

Code review by qodo was updated up to the latest commit 201f569

[qodo-code-review]

Code review by qodo was updated up to the latest commit 267ab0c

[qodo-code-review]

Code review by qodo was updated up to the latest commit 47213cd

[qodo-code-review]

Code review by qodo was updated up to the latest commit 9ba26df

[qodo-code-review]

Code review by qodo was updated up to the latest commit 258053a

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0be3ea2

[qodo-code-review]

Code review by qodo was updated up to the latest commit 72a6e81

[qodo-code-review]

Code review by qodo was updated up to the latest commit 6c2dd73

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4601bd3

[qodo-code-review]

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