.pr_agent_auto_best_practices

Pattern 1: Validate public API inputs and externally-sourced values early (null, type, shape, reserved keys), and fail with deterministic, actionable exceptions rather than allowing late NullReference/ClassCast behavior.

Example code before:

// external value, unsafe cast + no null validation
String browser = (String) caps.getCapability("se:browserName");
options.useTransport(() -> null); // later NRE

Example code after:

Object raw = caps.getCapability("se:browserName");
String browser = (raw instanceof String s && !s.isBlank()) ? s : caps.getBrowserName();

options.useTransport(() => {
  var t = factory();
  ArgumentNullException.ThrowIfNull(t);
  return t;
});

Relevant past accepted suggestions:

---

Suggestion 1:

[correctness] Null transport factory result

---

Null transport factory result

BiDiOptionsBuilder.UseTransport(Func) does not validate that the factory returns a non-null ITransport, so a null return will flow into BiDi.ConnectAsync and crash later when Broker calls SendAsync/ReceiveAsync on the transport.

Issue description

UseTransport(Func<ITransport>) validates the factory delegate itself, but not the object it returns. If factory() returns null, BiDi.ConnectAsync will pass it to Broker, leading to a NullReferenceException at runtime.

Issue Context

BiDi.ConnectAsync awaits TransportFactory and immediately constructs Broker(transport, bidi).

Fix Focus Areas

• dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[56-63]

Suggested fix

Update the wrapper delegate to validate the result:

• Call var transport = factory();
• ArgumentNullException.ThrowIfNull(transport);
• Return Task.FromResult(transport)

Optionally, wrap exceptions from factory() into a faulted Task<ITransport> for consistency with async factory patterns.

---

Suggestion 2:

[correctness] No transport null-check

---

No transport null-check

BiDiOptionsBuilder.UseTransport captures the provided transport without validating it, so passing null makes TransportFactory return null and later causes a NullReferenceException when Broker uses the transport. This turns a simple caller mistake into a late, non-actionable runtime crash during connection setup.

Issue description

BiDiOptionsBuilder.UseTransport(ITransport transport) does not validate transport and will happily set TransportFactory to return null when the caller passes null, leading to a runtime NullReferenceException downstream.

Issue Context

BiDi.ConnectAsync calls builder.TransportFactory(...) and passes the resulting transport into Broker, which unconditionally calls SendAsync, ReceiveAsync, and DisposeAsync on that instance.

Fix

• Add ArgumentNullException.ThrowIfNull(transport); at the start of UseTransport.

Fix Focus Areas

• dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-58]

---

Suggestion 3:

[correctness] Non-object JSON silently dropped

---

Non-object JSON silently dropped

`AdditionalData(string json)` accepts any JSON root (including arrays/null/primitives), but the rest of the type treats non-object roots as “empty”, so the provided data is silently ignored and won’t be merged into outgoing commands. This can hide user mistakes and make the new extensibility feature unreliable to debug.

Issue description

AdditionalData(string json) currently accepts any valid JSON, but AdditionalData is used as a property-bag (object) for extension properties. When callers provide a valid non-object JSON (e.g., "null", "[]", "123"), IsEmpty becomes true and the data is silently dropped (never merged/sent).

Issue Context

The API surface suggests the string overload is a convenience for providing additional properties. Silent dropping is surprising and makes debugging extensions difficult.

Fix

In AdditionalData(string json), validate doc.RootElement.ValueKind == JsonValueKind.Object.

• If not an object, throw ArgumentException (or JsonException) with a clear message (e.g., "AdditionalData JSON must be an object").

Fix Focus Areas

• dotnet/src/webdriver/BiDi/AdditionalData.cs[39-43]

---

Suggestion 4:

[correctness] TryGetValue throws on empty

---

TryGetValue throws on empty

`AdditionalData.TryGetValue` calls `JsonElement.TryGetProperty` without guarding for non-object/empty state, so calling it on `AdditionalData.Empty` (the default) can throw instead of returning `false`.

Issue description

AdditionalData.TryGetValue should follow the .NET Try-pattern and never throw for an empty value. Currently, AdditionalData.Empty is default (non-object JsonElement), but TryGetValue calls _data.TryGetProperty(...) unconditionally.

Issue Context

IsEmpty is defined as _data.ValueKind != JsonValueKind.Object, meaning Empty/default instances are explicitly non-object, yet TryGetValue does not check IsEmpty (or ValueKind) before accessing object-only APIs.

Fix Focus Areas

• dotnet/src/webdriver/BiDi/AdditionalData.cs[62-66]

Suggested change

Implement:

• if (IsEmpty) { value = default; return false; }
• otherwise call _data.TryGetProperty(key, out value)

Optionally also guard the indexer similarly (or leave it throwing by design, but ensure TryGetValue is safe).

---

Suggestion 5:

[reliability] Reserved keys overridable

---

Reserved keys overridable

Broker.ExecuteAsync writes options.AdditionalMessageData properties at the message root without filtering reserved keys, allowing duplicates of "id", "method", or "params" in the emitted JSON. If the remote end honors the last duplicate name, command correlation can break (timeouts or completing the wrong command).

Issue description

AdditionalMessageData is appended to the outgoing command envelope as arbitrary root-level JSON properties. Without validation, callers can provide keys like id, method, or params, producing duplicate JSON property names and potentially breaking protocol semantics and command correlation.

Issue Context

Outgoing envelope already writes id, method, and params before iterating AdditionalMessageData.

Fix Focus Areas

• dotnet/src/webdriver/BiDi/Broker.cs[95-109]

Implementation guidance

• Add a reserved-name check before writing each AdditionalMessageData entry.
  • At minimum reject: id, method, params.
  • Consider also rejecting other envelope keys that appear in incoming messages: type, result, error, message.
• On collision, throw an ArgumentException (or similar) with a clear message so failures are deterministic and debuggable.
• Add unit/serialization tests ensuring reserved keys are rejected and non-reserved keys serialize correctly.

---

Suggestion 6:

[reliability] Unsafe cast of `se:browserName`

---

Unsafe cast of `se:browserName`

`DriverFinder.toArguments()` directly casts `options.getCapability("se:browserName")` to `String`, which can throw `ClassCastException` if the capability is non-string or protocol-derived and thereby break Selenium Manager-based driver discovery. This violates the requirement to validate external/protocol inputs early and fail with deterministic, actionable exceptions (or safely fall back).

Issue description

DriverFinder.toArguments() reads the se:browserName capability via an unchecked cast to String. Since capability values are Object and may be protocol-/user-provided (including the new Electron override), a non-String value can trigger a ClassCastException, breaking Selenium Manager-based driver discovery instead of producing a deterministic, actionable validation failure or a safe fallback.

Issue Context

Capabilities are externally influenced (protocol/user) and getCapability() returns Object, so DriverFinder should validate type and null/emptiness before using se:browserName. When the value is absent/invalid, it should either ignore it and fall back to options.getBrowserName(), or throw a clear IllegalArgumentException describing the expected type/value; optionally, if the value is present but not a String, log a warning and ignore it.

Fix Focus Areas

• java/src/org/openqa/selenium/remote/service/DriverFinder.java[133-138]

---

Suggestion 7:

[maintainability] Useless ImmutableArray null-check

---

Useless ImmutableArray null-check

BiDi.SubscribeAsync/StreamAsync call ArgumentNullException.ThrowIfNull(descriptors) even though ImmutableArray is a value type and cannot be null, so this guard never triggers and can mislead about validation coverage. Callers passing default/empty arrays will fail later with a different exception path instead of a clear upfront argument validation.

Issue description

ArgumentNullException.ThrowIfNull(descriptors) is ineffective for ImmutableArray<T> parameters because ImmutableArray<T> is a struct (non-nullable value type). This can hide the real validation intention and doesn’t provide a clear exception for empty/default arrays.

Issue Context

The affected APIs are public entry points (IBiDi / BiDi) for multi-descriptor subscription/stream creation.

Fix Focus Areas

• Replace ThrowIfNull(descriptors) with explicit validation for default/empty immutable arrays (e.g., descriptors.IsDefaultOrEmpty or descriptors.Length == 0) and throw ArgumentException with a clear message.

• Apply the same validation to both SubscribeAsync(... ImmutableArray<EventDescriptor> ...) overloads and StreamAsync(... ImmutableArray<EventDescriptor> ...).

• dotnet/src/webdriver/BiDi/BiDi.cs[104-132]

---

Pattern 2: Make CI/workflow scripts and cleanup steps defensive: guard against empty inputs, use strict/safer argument passing, and ensure loops/commands have explicit failure paths instead of silently continuing or masking errors.

Example code before:

rm -rf $PATHS               # may be empty or start with '-'
while [ $i -lt 30 ]; do
  xdpyinfo >/dev/null 2>&1 && break
  sleep 1
done
echo "continuing..."        # no post-timeout failure

Example code after:

set -euo pipefail
paths=()
[[ -n "${CHROMEDRIVER:-}" ]] && paths+=("$CHROMEDRIVER")
if ((${#paths[@]} == 0)); then
  echo "No paths to delete; skipping"
  exit 0
fi
sudo rm -rf -- "${paths[@]}"

ok=false
for _ in $(seq 1 30); do
  xdpyinfo >/dev/null 2>&1 && { ok=true; break; }
  sleep 1
done
$ok || { echo "X server not ready"; exit 1; }

Relevant past accepted suggestions:

---

Suggestion 1:

[reliability] Empty driver path crash

---

Empty driver path crash

delete-browsers-drivers.ps1 now builds $paths only from WebDriver env vars and calls Remove-Item unconditionally. If none of those env vars are set on the Windows runner, $paths is empty and PowerShell parameter binding fails before -ErrorAction applies, breaking the workflow step and leaving preinstalled drivers in place.

Issue description

On Windows, Remove-Item -Path $paths ... is called even when $paths is empty (because it’s filtered from env vars only). PowerShell throws a parameter-binding error for an empty collection before -ErrorAction SilentlyContinue can suppress anything, causing the step to fail.

Issue Context

The workflow does not set these env vars explicitly for Windows; the step relies on the runner to provide them.

Fix Focus Areas

• scripts/github-actions/delete-browsers-drivers.ps1[12-20]
• .github/workflows/bazel.yml[202-208]

Suggested fix approach

• Add a guard:
  • If $paths.Count -eq 0, print a message and exit 0 (or discover driver locations via Get-Command chromedriver/msedgedriver/geckodriver and delete those paths).
• Optionally: set these env vars in the workflow step (if known stable locations exist), or add fallback known locations used by GitHub Windows runners.

---

Suggestion 2:

[reliability] `xdpyinfo` loop never fails

---

`xdpyinfo` loop never fails

The workflow’s X server readiness wait loop can time out without failing, so the step still proceeds to start `fluxbox` (backgrounded) even when `xdpyinfo` never succeeds and no display is actually available. This violates the requirement to validate preconditions and to have deterministic timeout failure paths for async waits, and can preserve the same intermittent/silent failure mode the wait was meant to eliminate.

Issue description

The X server (Xvfb) readiness polling loop does not have a deterministic failure path: if xdpyinfo never succeeds within the timeout, the workflow still starts fluxbox in the background and continues, allowing silent/intermittent failures when no display is actually ready.

Issue Context

This step is intended to eliminate a race where fluxbox starts before Xvfb is ready. CI/workflow scripts must validate preconditions and fail safe on errors, and async waits must have explicit timeouts with actionable failure when the timeout is exceeded; without a post-loop assertion, the race is only reduced, not eliminated.

Fix Focus Areas

• .github/workflows/bazel.yml[171-182]

---

Suggestion 3:

[reliability] `delete-browsers-drivers.sh` ignores errors

---

`delete-browsers-drivers.sh` ignores errors

The browser/driver deletion script does not validate inputs or command results, and it can pass empty path arguments to `rm`, producing noisy/undefined behavior while still continuing. This violates the requirement to fail safely and handle external command failures explicitly in CI automation.

Issue description

scripts/github-actions/delete-browsers-drivers.sh does not use strict mode and runs sudo rm -rf with possibly-empty env-derived paths, without validating that deletions succeeded.

Issue Context

In CI, silent cleanup failures can cause later steps to use unintended system browsers/drivers or run out of disk space without a clear signal.

Fix Focus Areas

• scripts/github-actions/delete-browsers-drivers.sh[7-21]

---

Suggestion 4:

[security] rm option injection risk

---

rm option injection risk

`delete-browsers-drivers.sh` passes env-var-derived paths directly to `sudo rm -rf` without a `--` terminator or safety checks, so values beginning with `-` can be parsed as options and lead to unintended deletions. This is a defensive-scripting regression versus the prior cleanup pattern and can break CI runners/jobs in hard-to-debug ways.

Issue description

sudo rm -rf is invoked with arguments sourced from environment variables (CHROMEWEBDRIVER, EDGEWEBDRIVER, GECKOWEBDRIVER) without a -- end-of-options marker or validation. If any value begins with -, rm can treat it as an option; combined with another argument like /, this can delete unintended paths.

Issue Context

This script is intended to delete specific preinstalled browser/driver binaries on CI runners, so it should be robust to unexpected env var contents and avoid option injection.

Fix

• Build a list/array of paths to delete.
• Filter out empty values.
• Reject dangerous values (/, ., ..) and any value starting with - (or force them to be treated as operands via --).
• Invoke rm as: sudo rm -rf -- "${paths[@]}".

Fix Focus Areas

• scripts/github-actions/delete-browsers-drivers.sh[11-21]

---

Suggestion 5:

[reliability] Disk status step can fail

---

Disk status step can fail

`disk-status.sh` runs `du -sh "$path"/* | sort -h` when the directory exists, which returns a non-zero status when the glob doesn’t match (e.g., empty directory), and this script is sourced directly inside multiple workflow steps. That non-zero can abort those steps, turning an observability checkpoint into a CI failure.

Issue description

scripts/github-actions/disk-status.sh is sourced from workflow steps. Inside measure(), the du pipelines can exit non-zero (e.g., when $path/* doesn’t match because the directory is empty, or due to permissions). When sourced, that failure can propagate and fail the workflow step.

Issue Context

This script is intended for diagnostics only; it should be best-effort and never fail CI.

Fix Focus Areas

• scripts/github-actions/disk-status.sh[29-37]
• .github/workflows/bazel.yml[206-214]
• .github/workflows/bazel.yml[239-247]
• .github/workflows/bazel.yml[293-300]

---

Suggestion 6:

[reliability] grep exits read-targets

---

grep exits read-targets

In ci.yml, process_binding assigns lang_targets via a pipeline containing grep; when there are no matching Ruby targets, grep exits non-zero and can fail the entire “Read targets” step instead of simply producing no rb_targets output.

Issue description

process_binding() uses grep inside a command substitution without guarding for the “no matches” exit code. When no Ruby targets exist, this can terminate the step early and fail CI.

Issue Context

The workflow should continue and simply omit rb_targets when there are no matching targets.

Fix Focus Areas

• .github/workflows/ci.yml[81-100]

Suggested fix

• Make the pipeline tolerant of no matches, e.g. append || true to the whole pipeline, or use grep ... || :.
  • Example:
    • lang_targets=$(echo "$targets" | tr ' ' '\n' | grep -E "^${pattern}[:/]" | tr '\n' ' ' | sed 's/ *$//' || true)
• Consider switching echo to printf '%s\n' "$targets" to avoid echo edge cases.

---

Suggestion 7:

[reliability] Bazel exit code 4 ignored

---

Bazel exit code 4 ignored

The `os-tests` job treats `bazel test` exit code `4` as success, which can mask misconfiguration (e.g., filters producing “no tests found”) and allow CI to go green without running any Ruby tests. This violates the requirement to avoid error-swallowing constructs in CI/scripts that hide failures.

Issue description

The os-tests Bazel invocation converts exit code 4 into success (exit 0). This can hide “no tests found” situations caused by incorrect target selection or tag filters, resulting in false-green CI.

Issue Context

bazel test exit code 4 typically indicates that no tests were executed. Treating this as success in PR CI can allow Ruby changes to merge without test coverage.

Fix Focus Areas

• .github/workflows/ci-ruby.yml[74-76]

---

Pattern 3: Keep tests reliable and meaningful: avoid order-dependent/string-substring JSON assertions, add assertions that prove the behavior, and prevent global state leakage (ENV/log spam) by restoring prior values and gating debug output.

Example code before:

expect(json).to include('{"baz":"qux",')   # ordering-dependent
ENV["HTTP_PROXY"] = "http://x"
ensure
  ENV.delete("HTTP_PROXY")                # clobbers preexisting
end
warn("DEBUG path=#{path}")                # unconditional noise

Example code after:

parsed = JSON.parse(json)
expect(parsed["baz"]).to eq("qux")

old = ENV["HTTP_PROXY"]
ENV["HTTP_PROXY"] = "http://x"
begin
  # test...
ensure
  old.nil? ? ENV.delete("HTTP_PROXY") : ENV["HTTP_PROXY"] = old
end

warn("DEBUG path=#{path}") if ENV["SE_DEBUG"] == "true"

Relevant past accepted suggestions:

---

Suggestion 1:

[reliability] Wrong `AdditionalMessageData` JSON assertion

---

Wrong `AdditionalMessageData` JSON assertion

`CommandAdditionalMessageDataIsSerializedAsTopLevelFields` asserts a substring that cannot appear in the produced command JSON because it assumes `AdditionalMessageData` fields are the first properties and followed by a trailing comma, while the broker writes `id`, `method`, and `params` first and appends `AdditionalMessageData` after `params` (often as the final property). This makes the test fail (or become ordering-dependent) even when the implementation is correct, blocking CI and undermining the required coverage for the new behavior.

Issue description

CommandAdditionalMessageDataIsSerializedAsTopLevelFields currently asserts the sent JSON contains the substring {"baz":"qux",, which assumes property ordering (that baz is first) and a trailing comma. In reality, the message writer serializes id, method, and params first, then appends CommandOptions.AdditionalMessageData fields after params, so baz will typically not be at the start and may be the last property (no trailing comma), making the test fail even when serialization is correct.

Issue Context

This is a new/updated unit test intended to validate that CommandOptions.AdditionalMessageData is serialized into the top-level command envelope. To satisfy the required test coverage (PR Compliance ID 18) without brittleness, the assertion should validate the semantic behavior (a top-level property baz with value qux) in an order/format-independent way, e.g., by parsing the outgoing JSON or by using a substring match that doesn’t depend on ordering or commas.

Fix Focus Areas

• dotnet/test/webdriver/BiDi/SessionUnitTests.cs[158-170]

---

Suggestion 2:

[reliability] `CanGetStatusWithAdditionalData` lacks assertions

---

`CanGetStatusWithAdditionalData` lacks assertions

The new `CanGetStatusWithAdditionalData` test exercises the API but only asserts that `status` is non-null, so it does not actually validate that `AdditionalData`/`AdditionalMessageData` are serialized/propagated and handled as intended. This leaves the new behavior effectively untested and risks regressions where additional data is silently dropped while the test still passes.

Issue description

CanGetStatusWithAdditionalData() currently acts as a smoke test (it only asserts status is non-null) and does not verify the PR’s intended behavior that AdditionalData/AdditionalMessageData are actually serialized onto the wire and/or propagated, so the feature it claims to test can regress without failing.

Issue Context

PR Compliance ID 19 expects meaningful coverage for changed behavior; this PR adds support for serializing/propagating extra JSON properties, but the test would still pass even if the additional data is ignored/dropped. Since StatusAsync talks to a real remote end, the response may not echo extension fields, so validating serialization typically requires inspecting the outgoing message (e.g., via a stub/mock transport or a broker-level unit test), or alternatively renaming the test to reflect that it only verifies the call succeeds without error.

Fix Focus Areas

• dotnet/test/webdriver/BiDi/Session/SessionTests.cs[46-59]

---

Suggestion 3:

[maintainability] Unconditional debug logging

---

Unconditional debug logging

`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.

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.

---

Suggestion 4:

[reliability] Test clobbers `SE_CHROMEDRIVER`

---

Test clobbers `SE_CHROMEDRIVER`

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.

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]

---

Suggestion 5:

[reliability] `with_env` deletes preexisting ENV

---

`with_env` deletes preexisting ENV

The new tests call `with_env`, which unconditionally deletes ENV keys on cleanup instead of restoring any pre-existing values. This can clobber global process state (e.g., `HTTP_PROXY`/`NO_PROXY`) and cause cross-test side effects or flaky behavior.

Issue description

with_env mutates ENV but its cleanup unconditionally deletes keys, which can remove pre-existing environment variables and leak global state changes across tests.

Issue Context

New tests added in default_spec.rb rely on with_env to set http_proxy and no_proxy/NO_PROXY. The helper currently deletes those keys in ensure rather than restoring prior values (or leaving them unset only if they were originally absent).

Fix Focus Areas

• rb/spec/unit/selenium/webdriver/spec_helper.rb[38-43]
• rb/spec/unit/selenium/webdriver/remote/http/default_spec.rb[127-139]

---

Suggestion 6:

[correctness] Cache path check fails

---

Cache path check fails

`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.

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:

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

---

Pattern 4: Avoid unintended breaking changes in public APIs and keep contracts consistent: when changing/removing behavior, provide deprecations/migrations and keep documentation/signature files (Javadoc/RBS) aligned with runtime behavior.

Example code before:

# runtime removed method but signature/docs still claim it exists
class Service
  # def env_path ... removed
end
# RBS still: def env_path: () -> String

Example code after:

# Option A: keep method with deprecation
def env_path
  warn("env_path is deprecated; use driver_path") if ENV["SE_DEPRECATIONS"] == "true"
  driver_path
end
# and update signatures/docs to match current behavior

Relevant past accepted suggestions:

---

Suggestion 1:

[quality] `match(matchAgainst, prefix)` Javadoc incomplete

---

`match(matchAgainst, prefix)` Javadoc incomplete

The modified public method `match(@Nullable String matchAgainst, @Nullable String prefix)` lacks a complete Javadoc block (no purpose sentence and no `@param` tags for `matchAgainst` and `prefix`). This violates the requirement that all changed public API methods have complete Javadoc, reducing API clarity and documentation quality.

Issue description

The public method match(@Nullable String matchAgainst, @Nullable String prefix) was modified, but its Javadoc is incomplete: it lacks a free-text purpose description and is missing @param tags for both parameters.

Issue Context

Compliance requires that any changed public API method in non-test code has complete Javadoc including a descriptive sentence and correct tags.

Fix Focus Areas

• java/src/org/openqa/selenium/remote/http/UrlTemplate.java[143-155]

---

Suggestion 2:

[correctness] `Safari.path` now nil

---

`Safari.path` now nil

`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.

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.

---

Suggestion 3:

[correctness] `Service#env_path` removed

---

`Service#env_path` removed

`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.

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]

---

Suggestion 4:

[maintainability] RBS missing browser_name=

---

RBS missing browser_name=

`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.

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).

---

Suggestion 5:

[correctness] Env var Service init regression

---

Env var Service init regression

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.

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]

---

Suggestion 6:

[correctness] Java rerun blocks other releases

---

Java rerun blocks other releases

The publish matrix exits with an error on any rerun for the Java entry before it checks whether Java is actually being released, so rerunning a ruby/dotnet/javascript-only release will still fail due to Java. This prevents reruns for non-Java patch releases (and any rerun attempt >1) even when Java would otherwise be skipped.

Issue description

The publish matrix currently fails on rerun attempts (github.run_attempt != '1') for the Java matrix entry even when the workflow is releasing a different language (e.g., selenium-4.28.1-ruby). This happens because the Java rerun guard runs before the “is this language selected?” check.

Issue Context

• publish runs a matrix over [java, ruby, dotnet, javascript].
• For patch releases, parse-tag sets outputs.language to the tag suffix (e.g., ruby).
• On reruns, the Java matrix entry should skip when outputs.language != 'java', but it currently fails early.

Fix

Move or gate the Java rerun guard so it only triggers when Java is actually being released:

Option A (recommended): nest the Java guard inside the selected-language branch:

if [ "${{ needs.parse-tag.outputs.language == 'all' || needs.parse-tag.outputs.language == matrix.language }}" = "true" ]; then
  if [ "${{ matrix.language == 'java' && github.run_attempt != '1' }}" = "true" ]; then
    echo "::error::Java release is not yet rerun-safe — ..."
    exit 1
  fi
  ./go ${{ matrix.language }}:release
else
  echo skipping
fi

Option B: add the selected-language predicate directly into the Java guard condition.

Fix Focus Areas

• .github/workflows/release.yml[144-153]

---

Suggestion 7:

[correctness] `check_error_with_driver_in_path` now errors

---

`check_error_with_driver_in_path` now errors

`check_error_with_driver_in_path` now suppresses errors only when `*is_driver_in_path` is true and `self.get_browser_path().is_empty()`, whereas it previously suppressed errors whenever `*is_driver_in_path` was true. Because `browser_path` can be populated during auto-detection, this changes user-visible behavior for driver-in-PATH fallback and can cause previously-successful setups to abort, breaking upgrades for consumers relying on the prior non-failing behavior.

Issue description

check_error_with_driver_in_path() has changed fallback semantics: it now propagates errors when *is_driver_in_path is true but get_browser_path() is non-empty, whereas previously driver-in-PATH mode would swallow these errors and continue. Because browser_path is frequently populated by auto-detection (not just explicit user input like --browser-path), this additional predicate can cause setup flows that intend to fall back to a PATH driver to abort during later failures (e.g., driver version discovery), creating a breaking behavior change for upgrades.

Issue Context

• setup() computes use_driver_in_path based on detecting a driver in PATH and the original browser version being empty, then routes errors from discovery/download/version steps through check_error_with_driver_in_path().
• Browser discovery/version logic calls detect_browser_path() when browser_path is empty; detect_browser_path() updates configuration via set_browser_path(), so get_browser_path() reflects derived/auto-detected state, not solely user-provided state.
• The PR’s main goal is switching command execution to argv, but this extra gating changes error/fallback semantics relied on by use_driver_in_path flows and may violate the “no breaking changes on upgrade” requirement.

Fix Focus Areas

• rust/src/lib.rs[804-854]
• rust/src/lib.rs[950-964]

---

Pattern 5: Align implementation with repo/toolchain constraints and build-system rules: avoid newer language APIs than the configured target, ensure formatter output matches CI, and prevent Bazel/Starlark output collisions by scoping generated artifact names per target/macro instance.

Example code before:

// Java 16+ API in a Java 11 repo
List<T> xs = stream.toList();

# Bazel macro emits fixed output names
outs = ["model.json", "ast.json"]

Example code after:

List<T> xs = stream.collect(Collectors.toUnmodifiableList());

# Scope outputs by name to avoid collisions
outs = [name + "-model.json", name + "-ast.json"]

Relevant past accepted suggestions:

---

Suggestion 1:

[correctness] Java 11 incompatible test

---

Java 11 incompatible test

EitherTest calls `Stream.toList()`, but the repo’s default Bazel compilation target is `--release 11`, where `Stream.toList()` does not exist. This will fail compilation for the new test under the default build configuration.

Issue description

java/test/org/openqa/selenium/internal/EitherTest.java uses either.stream().toList(). Stream.toList() was introduced in Java 16, but this repo’s Bazel defaults to --release 11, so the test will not compile under the default toolchain/CI settings.

Issue Context

Bazel is configured to target Java 11 by default (--javacopt="--release 11"). New tests should avoid APIs introduced after Java 11 unless the test target explicitly compiles with a higher --release.

Fix Focus Areas

• java/test/org/openqa/selenium/internal/EitherTest.java[22-40]
• .bazelrc[23-35]

Suggested fix

Update the test to avoid Stream.toList() (e.g., either.stream().collect(Collectors.toList())) and add the necessary Collectors import, or use Collectors.toUnmodifiableList() if you want closer behavior to Stream.toList() while still staying within Java 11.

---

Suggestion 2:

[maintainability] Import order not formatted

---

Import order not formatted

Some changed BiDi classes place `import org.openqa.selenium.Beta;` before `java.*` imports (e.g., `HasBiDi`), which is inconsistent with the repository’s google-java-format output. The CI “Format” job runs `./go format` (google-java-format) and fails if it produces diffs, so this will block CI until imports are reformatted.

Issue description

Some modified Java files have import blocks that are not in the canonical order produced by the repo’s formatter (google-java-format). This causes CI’s format check to modify files and then fail the job because the working tree is dirty.

Issue Context

CI runs ./go format, which applies google-java-format to all java/**.java files, and then fails if git diff is non-empty.

Fix

Run ./go format and commit the resulting changes, or manually reorder imports to match google-java-format output (then re-run ./go format to confirm no changes).

Fix Focus Areas

• java/src/org/openqa/selenium/bidi/HasBiDi.java[18-23]
• java/src/org/openqa/selenium/bidi/browser/ClientWindowInfo.java[18-23]
• java/src/org/openqa/selenium/bidi/browser/SetDownloadBehaviorParameters.java[18-27]

---

Suggestion 3:

[reliability] Artifact outputs can collide

---

Artifact outputs can collide

generate_bidi_library hardcodes the shared artifact output filenames (bidi-ast.json / bidi-model.json) independent of the macro instance name. A second invocation of this macro in the same Bazel package would attempt to declare the same output paths and fail Bazel analysis due to output collisions.

Issue description

generate_bidi_library always emits bidi-ast.json and bidi-model.json as output filenames. If the macro is ever instantiated more than once in the same Bazel package, Bazel will report output path collisions.

Issue Context

This PR introduces shared artifacts via js_run_binary (*_ast and *_json targets). These outputs are currently fixed strings instead of being scoped by the macro instance name.

Fix Focus Areas

• javascript/selenium-webdriver/private/generate_bidi.bzl[157-191]

Suggested fix

• Scope outputs by the macro instance name, e.g.:
  • ast_out = name + "-bidi-ast.json" (or name + "_ast.json")
  • model_out = name + "-bidi-model.json"
• Keep the public artifact names stable if needed by adding a thin filegroup (or alias target) that re-exports the uniquely named file under a stable label, rather than a stable filename.

---

Suggestion 4:

[maintainability] Format guidance mismatches CI

---

Format guidance mismatches CI

AGENTS.md now claims running `./scripts/format.sh` before pushing avoids CI formatter failures, but the CI format job still runs `./go format` and fails on any diff produced by that task. Since `./go format` runs the full Rake formatting suite unconditionally while `scripts/format.sh` is change-scoped, contributors may follow the new guidance yet still fail CI or receive conflicting instructions.

Issue description

AGENTS.md was updated to recommend ./scripts/format.sh / --pre-push to prevent CI formatter failures, but CI currently runs ./go format and reports failures based on that command. This creates inconsistent guidance and a real risk that contributors follow the documented workflow yet still hit CI failures.

Issue Context

• ./go format executes the Rake :format task, which formats across all languages unconditionally.
• ./scripts/format.sh is change-scoped (runs some language formatters only when matching paths changed).
• CI currently enforces formatting by running ./go format (and tells contributors to run it).

Fix Focus Areas

Pick one clear source-of-truth and make both CI + docs consistent:

• Update CI workflows to run ./scripts/format.sh (optionally --pre-push) and update error messages to reference it, OR
• Update AGENTS.md to explicitly state CI runs ./go format and recommend ./go format as the fallback/source-of-truth if CI fails.

References:

• AGENTS.md[69-78]
• .github/workflows/ci-lint.yml[47-74]
• Rakefile[128-144]
• scripts/format.sh[1-8], scripts/format.sh[35-137]

---

Suggestion 5:

[correctness] Broken version argv flag

---

Broken version argv flag

On non-Windows, `general_discover_browser_version` passes `cmd_version_arg` as a literal argv element, but Chrome/Edge/Firefox callers pass the format templates `DASH_DASH_VERSION`/`DASH_VERSION` (e.g. "{}{}{} --version"), so the browser is invoked with an invalid argument and version discovery will fail.

Issue description

general_discover_browser_version now builds a direct std::process::Command with args = [cmd_version_arg]. However, current callers pass DASH_VERSION / DASH_DASH_VERSION, which are format templates (contain {} placeholders) and were previously only safe because the old code formatted them into a full command string before going through sh -c.

This makes non-Windows browser version discovery invoke the browser with an invalid literal argument like "{}{}{} --version".

Issue Context

• DASH_VERSION / DASH_DASH_VERSION are currently defined as format templates.
• Chrome/Edge/Firefox pass these constants into general_discover_browser_version.
• The non-Windows path now directly uses cmd_version_arg.to_string() as an argv value.

Fix Focus Areas

• rust/src/lib.rs[87-88]
• rust/src/lib.rs[1169-1211]
• rust/src/chrome.rs[281-287]
• rust/src/edge.rs[165-189]
• rust/src/firefox.rs[194-200]

Suggested fix

Option A (minimal): Change the constants to be actual flags:

• DASH_VERSION = "-v"
• DASH_DASH_VERSION = "--version"

Option B (API cleanup): Change general_discover_browser_version to accept Vec<String> (or &[&str]) for version args, and update each manager to pass vec!["--version".into()] (or vec!["-v".into()]).

---

Suggestion 6:

[reliability] Cache triggers miss lockfiles

---

Cache triggers miss lockfiles

The gh-cache workflow’s push path filter omits pnpm-lock.yaml and multitool.lock.json, so updates to these Bazel dependency inputs won’t trigger cache population for macOS/Windows. Because MODULE.bazel uses these files to generate external repositories, CI may still need to download new artifacts (and hit the same transient 502/cert failures) until the next scheduled run.

Issue description

.github/workflows/gh-cache.yml uses a push.paths filter to decide when to repopulate the GitHub cache, but it does not include key dependency inputs (pnpm-lock.yaml, multitool.lock.json) that are used by Bazel to generate external repositories. This means macOS/Windows cache population will not run when those files change, leaving caches stale.

Issue Context

MODULE.bazel references both //:multitool.lock.json (rules_multitool hub) and //:pnpm-lock.yaml (npm_translate_lock). These files directly affect what external assets Bazel will download.

Fix Focus Areas

• .github/workflows/gh-cache.yml[4-15]

Proposed fix

Add the missing files to the on.push.paths list (at minimum pnpm-lock.yaml and multitool.lock.json). Consider also including other npm-related inputs referenced by npm_translate_lock (e.g. package.json, pnpm-workspace.yaml, .npmrc, and relevant javascript/**/package.json) if you want cache population to run immediately when those change.

---

[Auto-generated best practices - 2026-06-26]