[java] Make selenium.debug/webdriver.verbose configure the real logger (Mechanism Only)

[MohabMohie]

Related Issues

Part of #12892

Follow-up call-site migrations are tracked in #17835.

What does this PR do?

Makes -Dselenium.debug=true, the legacy -Dselenium.webdriver.verbose=true,
and SE_DEBUG show Java JUL diagnostics from the org.openqa.selenium logger.
The system properties only affect the Java bindings' JUL output; Grid and
driver-process output stay with SE_DEBUG.

Debug.configureLogger():

• reads the switches on every call, so a switch set after Selenium's classes load is honoured;
• is one-way: once a switch is seen, debug logging stays on for the life of the JVM (no snapshot/restore);
• raises the effective Selenium logger level to FINE only when it is less verbose, never lowering an explicit or inherited FINER/FINEST/ALL;
• installs one Selenium-owned console handler filtered to records below INFO, without changing user handlers or the root logger (users who also attach their own FINE handler will see FINE records twice);
• re-attaches that handler and FINE loggability if something resets them;
• gives SE_DEBUG and the properties separate levels (checked in that order) so either can become more verbose later; and
• is thread-safe via a private lock.

RemoteWebDriver applies the live configuration on each construction while keeping the early class-initialization hook for driver-discovery logs. getDebugLogLevel() is deprecated but keeps its legacy behavior while the remaining callers move to fixed levels in follow-up PRs.

Release note: enabling selenium.debug / selenium.webdriver.verbose now shows all Selenium FINE diagnostics, so output increases for existing users of the switch.

BiDi/CDP constructor wiring, Grid changes, and bulk call-site migrations are intentionally outside this mechanism-only PR.

Testing

• //java/test/org/openqa/selenium/internal:DebugTest
• //java/test/org/openqa/selenium/remote:RemoteWebDriverInitializationTest
• //java/src/org/openqa/selenium:core-lib-spotbugs

The tests cover each switch, live pickup, one-way behaviour, idempotence under concurrent calls, user handler and root logger preservation, explicit and inherited more-verbose levels, repair after external handler/level changes, duplicate INFO suppression, the documented duplicate FINE output for user handlers, and null-capabilities validation.

Types of changes

• Bug fix (backwards compatible)

AI assistance

• AI assisted
  • Tool(s): Claude (Anthropic), Codex (OpenAI), Grok Bot
  • What was generated: implementation and review assistance; the change was human-directed and reviewed.
  • I reviewed all AI output and can explain the change

[qodo-code-review]

PR Summary by Qodo

[java] Make selenium.debug/webdriver.verbose configure the real logger

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Unifies -Dselenium.debug/-Dselenium.webdriver.verbose with SE_DEBUG so both raise the real
 org.openqa.selenium logger to FINE and attach a filtered handler, instead of only changing what
 severity ~64 hand-picked statements report at.
• Debug.configureLogger() is now idempotent and reversible: it installs/removes its own filtered
 ConsoleHandler, restores the prior level when debugging turns off, and never lowers an already
 more-verbose level.
• Wires the new mechanism into RemoteWebDriver, LoggingOptions (Grid), and directly-constructed
 BiDi/CDP Connection instances so all entry points honor the same debug level; Grid's log handler
 gains a filter to avoid duplicate output.
• Adds extensive unit tests (DebugTest, LoggingOptionsTest, RemoteWebDriverInitializationTest,
 devtools/ConnectionTest) covering idempotency, level restoration, handler leakage, and
 duplicate-suppression behavior.

Diagram

graph TD
  A["selenium.debug / verbose / SE_DEBUG"] --> B["Debug.configureLogger()"]
  B --> C["org.openqa.selenium Logger"]
  B --> D["Installed ConsoleHandler"]
  E["RemoteWebDriver ctor"] --> B
  F["LoggingOptions.configureLogging()"] --> B
  G["bidi/Connection ctor"] --> B
  H["devtools/Connection ctor"] --> B
  F --> I["Grid Root Handler + Filter"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Migrate getDebugLogLevel() call sites onto Level.FINE reporting (Option 2)

• ➕ Fully consistent leveling across the codebase in one PR
• ➕ Removes the deprecated method entirely
• ➖ ~64 call sites is a large, mechanical, error-prone change
• ➖ Inflates this PR's diff and review risk
• ➖ Not required to fix the core inconsistency Titus flagged

2. Deprecate and delete Debug entirely (Option 1)

• ➕ Simplifies the logging story to a single well-known JUL mechanism
• ➕ Removes bespoke bookkeeping (handler tracking, level restoration)
• ➖ Breaking change for existing selenium.debug/verbose users
• ➖ Loses convenience of a single property to enable debug logging

Recommendation: Option 4 (raise the real logger level, adopted here) is the right mechanism: it makes the property-based switches behave identically to SE_DEBUG with no new public API, and idempotency/reversibility make it safe to call from every construction path. The PR explicitly considered and dismissed Option 2 (route the ~64 getDebugLogLevel() call sites onto the same mechanism) as a separate follow-up, and Option 1 (deprecate/delete Debug) as too disruptive given existing callers. Given the goal of closing #12892 with minimal surface-area risk, adopting Option 4 now and deferring the mechanical migration of getDebugLogLevel() call sites is the pragmatic choice, though it leaves a known inconsistency (those ~64 sites are visible but still misleveled) that should be tracked.

Files changed (12) +1051 / -22

Enhancement (1) +144 / -16

Debug.javaRework Debug into a stateful, reversible real-logger configurator +144/-16

Rework Debug into a stateful, reversible real-logger configurator

• Replaces the old one-shot 'change reported severity' mechanism with a synchronized configureLogger() that raises/restores the real org.openqa.selenium logger level, installs/removes a filtered ConsoleHandler, and exposes new introspection methods (isHandlerCurrentlyInstalled, isHandledBySeleniumDebugHandler). getDebugLogLevel() is deprecated.

java/src/org/openqa/selenium/internal/Debug.java

Bug fix (4) +96 / -6

LoggingOptions.javaHonor selenium.debug/verbose in Grid logging setup and avoid duplicate output +58/-4

Honor selenium.debug/verbose in Grid logging setup and avoid duplicate output

• setLoggingLevel() now also forces FINE when isDebugging() is true, not just SE_DEBUG. configureLogging() calls Debug.configureLogger() early, preserves org.openqa.selenium's handler when stripping loggers, and applies a rootHandlerFilter() to prevent duplicate FINE/CONFIG records when no log-file is configured.

java/src/org/openqa/selenium/grid/log/LoggingOptions.java

RemoteWebDriver.javaCall Debug.configureLogger() at both class-load and instance construction +25/-2

Call Debug.configureLogger() at both class-load and instance construction

• Adds an instance-time configureLogger() call in the canonical constructor alongside the existing static-initializer call, so a property changed after class load still takes effect for subsequently constructed drivers. Also fixes startSession to use the coalesced capabilities field instead of the raw parameter.

java/src/org/openqa/selenium/remote/RemoteWebDriver.java

Connection.javaConfigure debug logger in directly-constructed BiDi Connection +6/-0

Configure debug logger in directly-constructed BiDi Connection

• Adds a Debug.configureLogger() call at the start of the constructor so BiDi connections built outside RemoteWebDriver still honor the debug switches.

java/src/org/openqa/selenium/bidi/Connection.java

Connection.javaConfigure debug logger in directly-constructed CDP Connection +7/-0

Configure debug logger in directly-constructed CDP Connection

• Adds a Debug.configureLogger() call in the primary constructor so CDP connections built outside RemoteWebDriver still honor the debug switches; the deprecated 2-arg constructor delegates here.

java/src/org/openqa/selenium/devtools/Connection.java

Tests (5) +795 / -0

BUILD.bazelRegister new ConnectionTest in small test suite +1/-0

Register new ConnectionTest in small test suite

• Adds ConnectionTest.java to the SMALL_TESTS list for devtools tests.

java/test/org/openqa/selenium/devtools/BUILD.bazel

ConnectionTest.javaAdd test verifying direct Connection construction configures logger +116/-0

Add test verifying direct Connection construction configures logger

• New test asserting that constructing devtools.Connection directly (bypassing RemoteWebDriver/DriverFinder) still triggers Debug.configureLogger() and raises the logger level.

java/test/org/openqa/selenium/devtools/ConnectionTest.java

LoggingOptionsTest.javaAdd unit tests for LoggingOptions debug-integration behavior +259/-0

Add unit tests for LoggingOptions debug-integration behavior

• New test file covering forcing FINE via selenium.debug, interaction with external JUL config, handler preservation, and duplicate-suppression logic with/without a configured log-file.

java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java

DebugTest.javaAdd comprehensive unit tests for Debug.configureLogger() +331/-0

Add comprehensive unit tests for Debug.configureLogger()

• New test file covering isDebugging(), level raise/restore semantics, idempotency, handler installation/removal, duplicate suppression, and external-removal detection via isHandlerCurrentlyInstalled/isHandledBySeleniumDebugHandler.

java/test/org/openqa/selenium/internal/DebugTest.java

RemoteWebDriverInitializationTest.javaAdd tests for runtime debug-property pickup and null capabilities handling +88/-0

Add tests for runtime debug-property pickup and null capabilities handling

• Adds tests verifying that a second RemoteWebDriver construction picks up a debug property changed after the first, and that null capabilities are coalesced to an empty ImmutableCapabilities before startSession.

java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java

Other (2) +16 / -0

BUILD.bazelExpose grid log package to its new test package +1/-0

Expose grid log package to its new test package

• Adds test package visibility for the new java/test/org/openqa/selenium/grid/log test suite.

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

BUILD.bazelAdd new test target for grid log package +15/-0

Add new test target for grid log package

• New Bazel java_test_suite definition for grid/log unit tests.

java/test/org/openqa/selenium/grid/log/BUILD.bazel

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📜 Skill insights (0)

1. Nullable handler dereference ✓ Resolved 🐞 Bug ≡ Correctness

Description

Debug.configureLogger() dereferences the @Nullable installedHandler field (getLevel/setLevel)
without a local non-null proof, which is likely to trigger NullAway in error mode and fail
compilation. This also weakens static safety guarantees around the handler lifecycle.

Code

java/src/org/openqa/selenium/internal/Debug.java[R95-97]

+                && effectiveLevel(SELENIUM_LOGGER).intValue() <= Level.FINE.intValue()
+                && installedHandler.getLevel().intValue()
+                    <= effectiveLevel(SELENIUM_LOGGER).intValue()))) {

Evidence

The PR makes installedHandler explicitly nullable, but the focused new logic dereferences it
without a direct null check. Selenium’s Bazel macro enables NullAway for org.openqa.selenium, so
this pattern is likely to be flagged by the build’s static analysis.

java/src/org/openqa/selenium/internal/Debug.java[35-39]
java/src/org/openqa/selenium/internal/Debug.java[90-99]
java/src/org/openqa/selenium/internal/Debug.java[112-115]
java/private/java_library.bzl[18-36]

Agent prompt

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

### Issue description
`installedHandler` is declared `@Nullable`, but `configureLogger()` calls `installedHandler.getLevel()` and `installedHandler.setLevel(...)` after only checking `isHandlerCurrentlyInstalled()`. NullAway generally will not refine a field’s nullness based on a boolean-returning helper method, so this can be reported as a nullable dereference and (depending on build flags) fail compilation.

### Issue Context
Selenium enables NullAway for `org.openqa.selenium` packages and may treat findings as ERROR. The code is synchronized, but static analysis still requires a direct local null check or a local non-null variable.

### Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[90-125]
- java/src/org/openqa/selenium/internal/Debug.java[59-62]

### Suggested fix sketch
- In `configureLogger()`, capture `installedHandler` to a local variable `Handler handler = installedHandler;` and use `handler != null && loggerHasHandler(handler)` checks before dereferencing.
- Replace `if (isHandlerCurrentlyInstalled()) { installedHandler.setLevel(handlerLevel); }` with a local-variable guarded branch.
- In the early-return condition, compute `Level effective = effectiveLevel(SELENIUM_LOGGER);` once and use a `handler != null` check before calling `handler.getLevel()`.

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

---

2. Long line breaks formatter ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

RetryRequest’s server-error retry log call is now long enough that it is likely to be reformatted by
google-java-format, which can cause the repo’s format check to fail and block CI/merge. This was
introduced by inlining Debug.getDebugLogLevel() into the LOG.log call without reformatting the
resulting line.

Code

java/src/org/openqa/selenium/remote/http/RetryRequest.java[67]

+          LOG.log(Debug.getDebugLogLevel(), "Retry #" + (i + 1) + " on ServerError: " + response.getStatus());

Evidence

The PR change results in an unwrapped, very long LOG.log(Debug.getDebugLogLevel(), ...) line. The
repo’s formatting script runs google-java-format across java/**/*.java, so such a line will be
rewritten by the formatter and can cause format-check failure.

java/src/org/openqa/selenium/remote/http/RetryRequest.java[60-69]
scripts/format.sh[89-95]

Agent prompt

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

### Issue description
`RetryRequest` contains a very long single-line `LOG.log(...)` call that is unlikely to be google-java-format compliant after replacing the short `LOG_LEVEL` constant with `Debug.getDebugLogLevel()`. This can fail the repository’s Java formatting check.

### Issue Context
The repository’s formatter script runs google-java-format over all Java files and fails if formatting would introduce changes.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/http/RetryRequest.java[60-70]

### Suggested fix
Reformat the `LOG.log(Debug.getDebugLogLevel(), ...)` call (google-java-format will typically break it across multiple lines). Alternatively, run `./scripts/format.sh` (or the equivalent CI formatting target) and commit the formatter’s output.

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

---

3. Closed handler race ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

Debug.configureLogger() may close the previously installed Selenium handler on the “repair” path
without removing it from the org.openqa.selenium logger first. If another thread concurrently
re-attaches that handler between the membership check and close, the logger can retain a closed
handler, causing inconsistent/failed log emission.

Code

java/src/org/openqa/selenium/internal/Debug.java[R121-122]

+        if (currentHandler != null) {
+          currentHandler.close();

Evidence

The repair path closes currentHandler without removing it from the logger, while the debug-off
path explicitly removes then closes; because the installed check is based on a snapshot of
getHandlers(), concurrent external handler mutation can invalidate the check before close
executes.

java/src/org/openqa/selenium/internal/Debug.java[90-105]
java/src/org/openqa/selenium/internal/Debug.java[118-136]

Agent prompt

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

### Issue description
`Debug.configureLogger()` closes `currentHandler` in the debug-on repair branch without first calling `SELENIUM_LOGGER.removeHandler(currentHandler)`. Under concurrent external logger reconfiguration, the handler can be re-attached after the membership check and before `close()`, leaving a closed handler registered.

### Issue Context
`configureLogger()` is synchronized only against itself; it does not prevent other threads from mutating `SELENIUM_LOGGER` handlers concurrently.

### Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[118-123]

### Suggested change
In the `else` branch (debugging enabled, handler considered missing), if `currentHandler != null`, call `SELENIUM_LOGGER.removeHandler(currentHandler)` before `currentHandler.close()` (removal is safe even if the handler is not currently installed).

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

---

4. Repair test not isolated ✓ Resolved 🐞 Bug ☼ Reliability

Description

DebugTest.seleniumOwnedHandlerRepairsToAMoreVerboseLevel captures System.err and asserts the marker
appears only once, but it does not snapshot/remove pre-existing handlers attached directly to the
org.openqa.selenium logger. If any such handler also emits FINER to stderr, the test becomes
order/environment dependent and can flake.

Code

java/test/org/openqa/selenium/internal/DebugTest.java[R283-286]

+      seleniumLogger().setUseParentHandlers(false);
+      System.setErr(new PrintStream(capturedErr));
+      System.setProperty("selenium.debug", "true");
+      Debug.configureLogger();

Evidence

The test relies on stderr output uniqueness but does not reset or control pre-existing handlers on
the selenium logger; the class lifecycle helpers only manage properties and logger level, not
handlers.

java/test/org/openqa/selenium/internal/DebugTest.java[60-83]
java/test/org/openqa/selenium/internal/DebugTest.java[276-301]

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 `seleniumOwnedHandlerRepairsToAMoreVerboseLevel` assumes the only stderr-emitting path is Selenium’s owned handler, but it doesn’t isolate the logger’s direct handlers. Disabling parent handlers (`setUseParentHandlers(false)`) does not prevent other handlers already attached to `org.openqa.selenium` from also writing to `System.err`.

### Issue Context
`@BeforeEach` / `@AfterEach` restore properties and logger level, but they do not snapshot/restore the logger’s handler list. This makes assertions based on captured stderr potentially flaky under external JUL config or leaked handler state.

### Fix Focus Areas
- java/test/org/openqa/selenium/internal/DebugTest.java[60-83]
- java/test/org/openqa/selenium/internal/DebugTest.java[276-302]

### Suggested fix sketch
- At test start, snapshot `Handler[] oldHandlers = seleniumLogger().getHandlers()`.
- Temporarily remove them for the duration of the stderr-capture assertion (or replace with a dedicated in-memory handler) and restore them in `finally`.
- Alternatively, identify Selenium’s installed handler (the delta) and assert specifically on its behavior rather than the entire stderr stream.

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

---

5. FINER logs suppressed ✓ Resolved 🐞 Bug ≡ Correctness

Description

Debug.configureLogger() always sets the Selenium-owned ConsoleHandler to Level.FINE, so
FINER/FINEST/ALL records are dropped by that handler even when org.openqa.selenium (explicitly or
via inheritance) is configured more verbosely. This can hide detailed diagnostics from components
that log at FINER (e.g., HTTP/protocol tracing) despite the logger being loggable at those levels.

Code

java/src/org/openqa/selenium/internal/Debug.java[R116-118]

+        Handler handler = new ConsoleHandler();
+        handler.setLevel(Level.FINE);
+        Filter belowInfo = record -> record.getLevel().intValue() < Level.INFO.intValue();

Evidence

The focus-range change hard-codes the owned handler to Level.FINE. Meanwhile, Selenium has call
sites emitting FINER logs; with a more-verbose logger configuration, those records can be accepted
by the logger but still be rejected by the handler because the handler threshold is less verbose
than the logger’s effective level.

java/src/org/openqa/selenium/internal/Debug.java[105-122]
java/src/org/openqa/selenium/remote/codec/w3c/W3CHttpResponseCodec.java[74-85]
java/src/org/openqa/selenium/remote/http/DumpHttpExchangeFilter.java[29-46]

Agent prompt

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

## Issue description
`Debug.configureLogger()` creates/repairs a Selenium-owned `ConsoleHandler`, but it hard-codes the handler’s threshold to `Level.FINE`. If the `org.openqa.selenium` logger is configured to `FINER`/`FINEST`/`ALL` (explicitly or inherited), those records can pass the logger-level check but still be rejected by the handler-level check, so the owned handler won’t emit them.

## Issue Context
Some Selenium code logs at `Level.FINER` (e.g., response decoding and HTTP exchange dumping). When users intentionally configure JUL more verbosely than `FINE`, the debug mechanism should not silently clamp the owned handler to a less-verbose threshold.

## Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[110-122]

## Suggested fix approach
- When debugging is enabled, set the owned handler level to the *more verbose* of:
 - `Level.FINE` (minimum enabled by debug switches), and
 - the current effective level for `org.openqa.selenium` (or alternatively `Level.ALL`).
- Keep the existing `below INFO` filter behavior unchanged.
- Ensure the “repair” path updates the handler level using the same logic (not always `FINE`).

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

---

View review recommended (21)

6. Misleading idempotent Javadoc ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The Javadoc for Debug.configureLogger() says repeated calls with unchanged switches “do nothing”,
but the implementation explicitly repairs divergence (e.g., handler removed externally or logger
level reset) even when switch values are unchanged. This mismatch can mislead
maintainers/integrators about side effects and expected behavior under LogManager
resets/reconfiguration.

Code

java/src/org/openqa/selenium/internal/Debug.java[R128-131]

+   * attaches a handler Selenium owns, filtered to exclude {@link Level#INFO} and above. Caller-owned
+   * direct or ancestor handlers that accept {@link Level#FINE} can still receive and print FINE
+   * records, so FINE output can be duplicated. Idempotent: repeated calls while the switches are
+   * unchanged do nothing. Reversible: once every switch is off, the

Evidence

The Javadoc claims no-op behavior when switches are unchanged, but the code’s early-return condition
only returns when the handler is still installed and the logger is already sufficiently verbose;
otherwise it proceeds to repair state even though switch values did not change.

java/src/org/openqa/selenium/internal/Debug.java[123-137]
java/src/org/openqa/selenium/internal/Debug.java[149-163]

Agent prompt

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

### Issue description
`Debug.configureLogger()` Javadoc states that repeated calls with unchanged debug switches are a no-op, but the method intentionally performs repairs when the internal/logger state diverges (e.g., handler removed externally or effective logger level no longer satisfies the requested verbosity).

### Issue Context
The implementation’s early-return guard checks for handler presence and effective level and only returns when the configuration is already consistent. The Javadoc should describe this “no-op when consistent, repairs when divergent” behavior.

### Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[123-137]

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

---

7. Handler check-then-use race ⊘ Outdated 🐞 Bug ☼ Reliability

Description

Debug.isHandledBySeleniumDebugHandler calls isHandlerCurrentlyInstalled() and then dereferences
the mutable static installedHandler without holding the same lock, so a concurrent
configureLogger() can set installedHandler to null between the check and use, causing an
intermittent NPE (and/or incorrect duplicate-suppression decisions).

Code

java/src/org/openqa/selenium/internal/Debug.java[R85-95]

+  public static boolean isHandledBySeleniumDebugHandler(String loggerName, Level level) {
+    if (!isHandlerCurrentlyInstalled()) {
+      return false;
+    }
+    boolean withinSeleniumHierarchy =
+        loggerName != null
+            && (loggerName.equals("org.openqa.selenium")
+                || loggerName.startsWith("org.openqa.selenium."));
+    return withinSeleniumHierarchy
+        && level.intValue() >= installedHandler.getLevel().intValue()
+        && level.intValue() < Level.INFO.intValue();

Evidence

isHandledBySeleniumDebugHandler dereferences installedHandler outside synchronization after a
separate synchronized check, while configureLogger() can concurrently remove/close the handler and
set installedHandler to null under its own synchronized block.

java/src/org/openqa/selenium/internal/Debug.java[76-96]
java/src/org/openqa/selenium/internal/Debug.java[164-215]

Agent prompt

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

### Issue description
`Debug.isHandledBySeleniumDebugHandler()` performs a synchronized installation check, then reads `installedHandler` again outside the lock. If another thread runs `Debug.configureLogger()` concurrently and uninstalls the handler, `installedHandler` can become null after the check, leading to a `NullPointerException` (or stale decisions).

### Issue Context
- `configureLogger()` is synchronized and can remove the handler and set `installedHandler = null` while other threads may be logging/inspecting handlers.
- `isHandledBySeleniumDebugHandler()` is public and can be called from logging infrastructure concurrently.

### Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[85-96]

### Suggested fix
Make the check and dereference atomic by either:
1) Synchronizing `isHandledBySeleniumDebugHandler()` and using a local snapshot:
  - `Handler handler = installedHandler;`
  - verify `handler` is still attached (`SELENIUM_LOGGER.getHandlers()` contains it)
  - use `handler.getLevel()` for comparisons

or
2) Refactor `isHandlerCurrentlyInstalled()` to return the current installed handler (or null) under the same lock, so callers never need to read `installedHandler` unsafely.

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

---

8. Test clobbers debug property ✓ Resolved 🐞 Bug ☼ Reliability

Description

RetryRequestTest.retryRecordsStayFineWhenSystemPropertyDebuggingIsEnabled sets selenium.debug
and always clears it in finally, which can destroy a pre-existing value and make other tests in
the same JVM order-dependent/flaky.

Code

java/test/org/openqa/selenium/remote/http/RetryRequestTest.java[R384-395]

+    System.setProperty("selenium.debug", "true");
+    try {
+      new RetryRequest()
+          .andFinally(request -> new HttpResponse().setStatus(HTTP_UNAVAILABLE))
+          .execute(new HttpRequest(GET, "/"));
+
+      assertThat(records).extracting(LogRecord::getLevel).contains(Level.FINE);
+    } finally {
+      System.clearProperty("selenium.debug");
+      logger.removeHandler(handler);
+      logger.setLevel(oldLevel);
+    }

Evidence

The test directly sets and clears the global selenium.debug property, and the class has no
setup/teardown that snapshots/restores it.

java/test/org/openqa/selenium/remote/http/RetryRequestTest.java[54-68]
java/test/org/openqa/selenium/remote/http/RetryRequestTest.java[363-395]

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 mutates a global JVM system property (`selenium.debug`) but does not restore the previous value; it unconditionally clears the property in `finally`. If the property was set by the test runner/environment (or another test), this test will corrupt global state.

### Issue Context
This file already restores logger state (`logger.setLevel(oldLevel)` and `removeHandler(handler)`), but it does not snapshot/restore the `selenium.debug` property.

### Fix Focus Areas
- java/test/org/openqa/selenium/remote/http/RetryRequestTest.java[362-396]

### Suggested fix
Snapshot and restore the property:
- Before setting it, store `String old = System.getProperty("selenium.debug");`
- In `finally`, if `old != null` then `System.setProperty("selenium.debug", old)` else `System.clearProperty("selenium.debug")`.

(Alternatively, move this into a small @BeforeEach/@AfterEach pair if more tests in this class start using the property.)

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

---

9. RetryRequest logs ServerError at INFO 📘 Rule violation ◔ Observability

Description

RetryRequest logs retry attempts for both server-error (5xx) responses and connection failures
(ConnectException) using Debug.getDebugLogLevel() (INFO/FINE) instead of warning, reducing
visibility of potentially actionable transient failures and retry behavior in normal operational
logs.

Code

java/src/org/openqa/selenium/remote/http/RetryRequest.java[67]

+          LOG.log(Debug.getDebugLogLevel(), "Retry #" + (i + 1) + " on ServerError: " + response.getStatus());

Evidence

PR Compliance ID 330199 requires problematic conditions such as failures and retries to be logged at
warning (i.e., via logger.warning). The cited changes in RetryRequest show the retry logs for
both the server-error retry path and the ConnectException retry path are emitted at
Debug.getDebugLogLevel() (INFO/FINE) rather than warning, which can cause these retry/failure
signals to be missed when INFO/FINE logs are not collected.

Rule 330199: Use appropriate log levels for message intent
java/src/org/openqa/selenium/remote/http/RetryRequest.java[67-67]
java/src/org/openqa/selenium/remote/http/RetryRequest.java[52-52]

Agent prompt

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

## Issue description
`RetryRequest` currently logs retry attempts for server errors (5xx responses) and connection failures (`ConnectException`) at `Debug.getDebugLogLevel()` (INFO/FINE) rather than `warning`. Per PR Compliance ID 330199, problem/retry conditions should use `logger.warning` to ensure actionable transient failures are visible in normal operational logs.

## Issue Context
If INFO/FINE logs are not collected, these retries may be missed, reducing operator awareness and making it harder to diagnose intermittent server or connectivity issues.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/http/RetryRequest.java[52-52]
- java/src/org/openqa/selenium/remote/http/RetryRequest.java[67-67]

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

---

10. Debug level not repaired ✓ Resolved 🐞 Bug ☼ Reliability

Description

Debug.configureLogger() can reinstall its handler after external removal while leaving the
org.openqa.selenium logger level at INFO (or higher) if that level was also reset/lowered during the
same debug-on period, so FINE/CONFIG diagnostics remain suppressed despite debug switches being on.

Code

java/src/org/openqa/selenium/internal/Debug.java[R202-236]

+    if (shouldDebug) {
+      // Only run the level-raising bookkeeping on a genuine off->on transition. A call that
+      // reaches here merely to repair a missing handler (loggerConfigured already true) must not
+      // re-run this: doing so would overwrite levelRaisedByDebug/previousLevel with whatever the
+      // level happens to be right now, corrupting the snapshot that turning debug off later needs
+      // to restore the correct pre-debug level.
+      if (!loggerConfigured) {
+        // Only raise the level when the logger's EFFECTIVE level is currently LESS verbose than
+        // FINE (higher intValue). A more-verbose effective level (FINER, FINEST, ALL) is left
+        // alone: lowering it would make records like W3CHttpResponseCodec's FINER diagnostics
+        // unloggable while "debugging". This decides off the effective level rather than the
+        // logger's own (possibly null/inherited) level -- a null own level with a more-verbose
+        // level set on a parent logger already means FINER-or-better records are loggable right
+        // now, and forcing this logger's own level to FINE would clobber that inherited
+        // verbosity. What gets snapshotted/restored is still the logger's own level exactly as
+        // before; only the raise/no-raise decision changes.
+        Level currentLevel = SELENIUM_LOGGER.getLevel();
+        levelRaisedByDebug = effectiveLevel(SELENIUM_LOGGER).intValue() > Level.FINE.intValue();
+        if (levelRaisedByDebug) {
+          previousLevel = currentLevel;
+          SELENIUM_LOGGER.setLevel(Level.FINE);
+        } else {
+          previousLevel = null;
+        }
+      }
+
+      // Runs unconditionally whenever shouldDebug is true and we reach this point -- both on a
+      // genuine turn-on and on a handler-repair call -- since that's the actual state that needs
+      // fixing in the repair case.
+      Handler handler = new ConsoleHandler();
+      handler.setLevel(Level.FINE);
+      Filter belowInfo = record -> record.getLevel().intValue() < Level.INFO.intValue();
+      handler.setFilter(belowInfo);
+      SELENIUM_LOGGER.addHandler(handler);
+      installedHandler = handler;

Evidence

The code explicitly supports repairing externally removed handlers, but the level-raising logic is
gated to only run when loggerConfigured is false (off→on). During a repair call
(loggerConfigured already true), the handler is re-added but the level is not re-evaluated or
raised, so an externally-reset/lowered logger level can keep FINE/CONFIG records from ever being
created/propagated to any handler.

java/src/org/openqa/selenium/internal/Debug.java[73-106]
java/src/org/openqa/selenium/internal/Debug.java[191-257]

Agent prompt

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

### Issue description
`Debug.configureLogger()` has a repair path for when its installed handler is removed externally while debug is still enabled. In that repair path, it always reinstalls the handler, but it only performs the logger level-raising logic on a true off→on transition (`if (!loggerConfigured)`).

If an external action (e.g., `LogManager.reset()` or other logging reconfiguration) removes the handler **and** also clears/raises the effective level of `org.openqa.selenium` back to INFO, the subsequent repair call will reinstall the handler but may leave the logger level too high to allow FINE/CONFIG records to be logged at all.

### Issue Context
This is specifically relevant because the code explicitly calls out external removal scenarios like `LogManager.reset()`; those scenarios can affect both handlers and levels.

### Fix Focus Areas
- Ensure the “repair” path also re-checks whether `org.openqa.selenium` is currently loggable at `Level.FINE` and raises the logger level if needed **without overwriting** the original `previousLevel` snapshot.
- Keep the existing protection against clobbering user changes (don’t overwrite the pre-debug snapshot during repair).

- java/src/org/openqa/selenium/internal/Debug.java[191-257]

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

---

11. RemoteWebDriver ctor has \n` tokens ✓ Resolved 📘 Rule violation ✧ Quality

Description

The RemoteWebDriver(CommandExecutor, Capabilities, ClientConfig) constructor contains literal ``
n` sequences between statements (including around field assignments and
Debug.configureLogger()), which is invalid Java and will break compilation/format checks. This
formatting failure was introduced in the newly modified constructor lines and can block downstream
drivers that rely on RemoteWebDriver.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[230]

+    this.clientConfig = Require.nonNull("Client config", clientConfig);`n    this.executor = Require.nonNull("Command executor", executor);`n    Debug.configureLogger();

Evidence

The cited constructor change in java/src/org/openqa/selenium/remote/RemoteWebDriver.java shows
literal ` n`` tokens embedded directly in executable code, which is not a valid Java escape
sequence and therefore causes a compile-time syntax error. This also violates the
formatter/compliance rule (PR Compliance ID 389272) because the presence of these artifacts
indicates the code is not properly formatted as committed.

Rule 389272: Run code formatter via scripts/format.sh or configured git hooks before pushing
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[230-230]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[226-235]

Agent prompt

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

## Issue description
`java/src/org/openqa/selenium/remote/RemoteWebDriver.java` contains literal `` `n`` tokens embedded inside the canonical `RemoteWebDriver(CommandExecutor, Capabilities, ClientConfig)` constructor, making the file invalid Java and indicating the formatter was not applied.

## Issue Context
These tokens were introduced in the newly edited constructor body and appear to be accidental artifacts where the intent was to place statements on separate lines (e.g., between field assignments and `Debug.configureLogger()`), but instead literal `` `n`` sequences were committed.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[226-235]

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

---

12. BiDi Connection has \n` tokens ✓ Resolved 📘 Rule violation ✧ Quality

Description

The Connection(HttpClient, String) constructor in the BiDi Connection class contains literal ``
n` sequences in its declaration/body, which is invalid Java syntax and will fail
compilation/format checks. This suggests the formatter workflow was bypassed or the change was not
properly formatted before pushing, blocking the BiDi module build and preventing the intended
debug-logger configuration from shipping.

Code

java/src/org/openqa/selenium/bidi/Connection.java[R90-95]

+  public Connection(HttpClient client, String url) {`n
+    // Reflect the current debug switches before this connection starts logging its wire
+    // diagnostics at FINE -- callers that construct a Connection directly (never going through
+    // RemoteWebDriver or DriverFinder) would otherwise never trigger the raise. Idempotent and
+    // cheap, same pattern as DriverFinder.getBinaryPaths().
+        Require.nonNull("HTTP client", client);`n    Require.nonNull("URL to connect to", url);`n    Debug.configureLogger();

Evidence

PR Compliance ID 389272 requires running the formatter workflow and avoiding obvious formatting
issues before pushing, and the committed changes to
java/src/org/openqa/selenium/bidi/Connection.java show the constructor’s opening brace and
subsequent statements include the literal sequence ` n `` as raw tokens (not whitespace, not a
comment, and not inside a string). Because ` n`` is not valid Java syntax in this context, the
file becomes uncompilable, directly demonstrating a formatting/syntax break in the PR branch.

Rule 389272: Run code formatter via scripts/format.sh or configured git hooks before pushing
java/src/org/openqa/selenium/bidi/Connection.java[90-95]
java/src/org/openqa/selenium/bidi/Connection.java[90-99]

Agent prompt

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

## Issue description
`java/src/org/openqa/selenium/bidi/Connection.java` contains literal `` `n`` characters embedded in the `Connection(HttpClient, String)` constructor signature/body, which is invalid Java syntax and will fail compilation/formatting.

## Issue Context
These `` `n`` tokens appear to be accidental paste/formatting artifacts where intended newlines were inserted as literal text, indicating the formatter workflow was likely bypassed and the change was not properly formatted before pushing (per PR Compliance ID 389272).

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Connection.java[90-99]

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

---

13. CDP Connection has \n` tokens ✓ Resolved 📘 Rule violation ✧ Quality

Description

The 3-arg Connection(HttpClient, String, ClientConfig) constructor in DevTools Connection.java
contains literal ` n`` sequences embedded between statements, which is invalid Java and will fail
compilation/format checks. This suggests the formatter workflow was not correctly applied to the
recent edits and breaks the DevTools module regardless of which constructor overload is used.

Code

java/src/org/openqa/selenium/devtools/Connection.java[113]

+    this.client = Require.nonNull("HTTP client", client);`n    this.wsConfig = wsClientConfig(clientConfig, url);`n    Debug.configureLogger();

Evidence

PR Compliance ID 389272 requires running the formatter workflow and avoiding obvious formatting
issues, and the modified Connection(HttpClient, String, ClientConfig) constructor includes literal
` n` text between statements. Because  n`` is not valid Java tokenization/syntax in this
context, the class cannot compile, and since the two-arg constructor delegates to this constructor,
the syntax error impacts compilation even when other overloads are used.

Rule 389272: Run code formatter via scripts/format.sh or configured git hooks before pushing
java/src/org/openqa/selenium/devtools/Connection.java[113-113]
java/src/org/openqa/selenium/devtools/Connection.java[107-116]

Agent prompt

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

## Issue description
`java/src/org/openqa/selenium/devtools/Connection.java` contains literal `` `n`` tokens embedded in the `Connection(HttpClient, String, ClientConfig)` constructor body, concatenating multiple statements into invalid Java and preventing compilation.

## Issue Context
This appears to be the result of statements being accidentally joined using a literal token instead of actual newlines/formatting, indicating the formatter workflow was not correctly applied to the recent edits. The two-arg constructor delegates to the 3-arg constructor, so this syntax error is always on the compilation path and breaks the DevTools module regardless of which constructor overload is used.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/Connection.java[107-116]

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

---

14. Hot-path debug property reads 🐞 Bug ➹ Performance

Description

Debug.isDebugging() now performs live system-property reads on every call, and
Debug.getDebugLogLevel() is used in per-request/per-message logging (e.g., DevTools Connection.send
and Netty RequestConverter.channelRead0). This adds avoidable overhead in high-volume paths because
the property lookup is evaluated even when the logger ends up not emitting the record.

Code

java/src/org/openqa/selenium/internal/Debug.java[R51-53]

  public static boolean isDebugging() {
-    return IS_DEBUG;
+    return Boolean.getBoolean("selenium.debug") || Boolean.getBoolean("selenium.webdriver.verbose");
  }

Evidence

The new implementation of isDebugging() performs live property reads, and multiple high-frequency
logging call sites invoke getDebugLogLevel() (which calls isDebugging()) for each message/request,
causing repeated property lookups regardless of whether the record is ultimately emitted.

java/src/org/openqa/selenium/internal/Debug.java[43-71]
java/src/org/openqa/selenium/devtools/Connection.java[192-206]
java/src/org/openqa/selenium/netty/server/RequestConverter.java[59-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
`Debug.isDebugging()` now reads `selenium.debug` and `selenium.webdriver.verbose` via `Boolean.getBoolean(...)` on every call. Because many hot-path log statements call `getDebugLogLevel()` unconditionally (argument evaluation happens before `Logger.log(...)` can decide to emit), this introduces repeated property lookups in high-throughput code paths.

### Issue Context
This PR intentionally keeps `getDebugLogLevel()` semantics for legacy call sites, but those sites are frequently executed (wire/logging paths). Even if overall impact is small, it’s a real regression compared to a cached boolean.

### Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[43-71]
- java/src/org/openqa/selenium/devtools/Connection.java[200-205]
- java/src/org/openqa/selenium/netty/server/RequestConverter.java[59-66]

### Suggested remediation options
Pick one (or a combination):
1) **Migrate the hottest call sites off `getDebugLogLevel()`** (preferred long-term): log at a fixed level (typically `Level.FINE`) and rely on `Debug.configureLogger()` for visibility.
2) **Guard hot-path log statements** to avoid unnecessary work and debug-switch checks when they cannot be emitted (e.g., `if (LOG.isLoggable(Level.FINE)) { ... }`).
3) If keeping legacy behavior, consider a lightweight cached debug-switch value that is refreshed at known chokepoints (e.g., within `configureLogger()`), and have hot paths consult that cache instead of re-reading system properties every time.

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

---

15. Logging side-effect pre-validation ✓ Resolved 🐞 Bug ☼ Reliability

Description

RemoteWebDriver and BiDi/CDP Connection constructors call Debug.configureLogger() before validating
required arguments, so a failed construction (e.g., null executor/client/url) can still mutate the
global org.openqa.selenium logger level/handlers. This makes failure paths unexpectedly change
process-wide logging state.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R228-233]

+    // Instance-time (not class-load-time) so a property change made after this class has already
+    // loaded still takes effect for drivers constructed afterwards.
+    Debug.configureLogger();
    this.clientConfig = Require.nonNull("Client config", clientConfig);
    this.executor = Require.nonNull("Command executor", executor);
    this.capabilities = requireNonNullElseGet(capabilities, () -> new ImmutableCapabilities());

Evidence

The constructors now call Debug.configureLogger() before Require.nonNull checks.
Debug.configureLogger() mutates the shared org.openqa.selenium logger by potentially setting its
level to FINE and adding a ConsoleHandler; therefore those side effects can occur even if the
constructor later throws due to invalid arguments.

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[213-237]
java/src/org/openqa/selenium/bidi/Connection.java[81-101]
java/src/org/openqa/selenium/devtools/Connection.java[95-118]
java/src/org/openqa/selenium/internal/Debug.java[191-257]

Agent prompt

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

### Issue description
Several constructors invoke `Debug.configureLogger()` before `Require.nonNull(...)` argument validation. If validation fails, the constructor throws but `configureLogger()` may already have raised the shared `org.openqa.selenium` logger to `FINE` and/or installed a handler, leaving behind process-global side effects from a failed creation attempt.

### Issue Context
The goal of early configuration is valid (ensure debug wiring is in place before FINE-level diagnostics happen), but nothing in these constructors logs before argument checks. Reordering can preserve behavior while avoiding side effects on invalid inputs.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[226-237]
- java/src/org/openqa/selenium/bidi/Connection.java[90-101]
- java/src/org/openqa/selenium/devtools/Connection.java[107-118]
- java/src/org/openqa/selenium/internal/Debug.java[191-257]

### What to change
- **Move `Debug.configureLogger()` after non-null validation** (and any other cheap argument checks) but still before any socket/session start that can log at `FINE`.
 - `RemoteWebDriver(...)`: validate `clientConfig` and `executor` first, then call `Debug.configureLogger()`, then proceed to `startSession(...)`.
 - BiDi/CDP `Connection(...)`: validate `client`/`url` (and optionally `clientConfig`) first, then call `Debug.configureLogger()`, then open the socket.

### Acceptance criteria
- Passing null for required params throws the same exception as today.
- No logger/handler mutation occurs when those exceptions are thrown.
- Debug logging remains configured before any wire/session diagnostics can be emitted.

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

---

16. Duplicate test checks stdout ✓ Resolved 🐞 Bug ≡ Correctness

Description

LoggingOptionsTest’s “no duplicate” test runs with SE_DEBUG (so Grid’s root handler writes to
System.err) but asserts the marker is absent from captured stdout, which cannot detect duplicate
stderr output. This can let stderr duplication regressions pass CI unnoticed.

Code

java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java[R211-212]

+    // Grid's root handler (defaulting to stderr here, same as Debug's handler) must not ALSO
+    // print it.

Evidence

The test explicitly documents that with SE_DEBUG and no log-file, the root handler defaults to
System.err, but then it checks captured stdout to verify non-duplication; duplicates would occur on
stderr. LoggingOptions.getOutputStream confirms the root handler uses System.err under SE_DEBUG with
no log-file.

java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java[183-214]
java/src/org/openqa/selenium/grid/log/LoggingOptions.java[265-282]

Agent prompt

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

### Issue description
`configureLoggingDoesNotDuplicateSeleniumDebugRecordsWhenNoLogFileIsConfigured` intends to prove that the root handler does not duplicate org.openqa.selenium FINE records when SE_DEBUG routes both handlers to `System.err`, but it currently validates `captured.out()` instead of validating duplication on `captured.err()`.

### Issue Context
Under `SE_DEBUG=true` and no `log-file`, Grid’s `getOutputStream()` selects `System.err`, and Debug’s handler is also a `ConsoleHandler` targeting `System.err`. Any duplication would therefore appear in stderr.

### Fix Focus Areas
- java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java[183-214]

### Suggested change
Update the assertion to verify the marker occurs **exactly once** in `captured.err()` (e.g., count occurrences of `marker` in the string), and keep/adjust the stdout assertion only as a secondary sanity check if desired.

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

---

17. SE_DEBUG cleanup can leak ✓ Resolved 🐞 Bug ☼ Reliability

Description

LoggingOptionsTest sets SE_DEBUG via an environment stub but its @AfterEach calls
Debug.configureLogger() without first forcing SE_DEBUG off, so configureLogger can observe SE_DEBUG
still enabled and keep Debug’s handler installed. When the stub later restores the environment, no
further configureLogger call runs, so the handler can leak into subsequent tests in the same JVM.

Code

java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java[R189-199]

+    // With SE_DEBUG set and no log-file configured, getOutputStream() defaults that root handler
+    // to System.err too -- the exact same destination Debug's own ConsoleHandler always targets --
+    // so nothing stopped a FINE record from org.openqa.selenium(.*) printing twice, once from each
+    // handler. INFO-and-above records must be unaffected -- Debug's own handler already excludes
+    // those, so they only ever reached Grid's root handler in the first place. SE_DEBUG is set via
+    // an environment-variable stub rather than the selenium.debug property: this scenario is
+    // specifically about Debug.isDebugAll() (SE_DEBUG), which is what makes getOutputStream()
+    // route to System.err in the first place -- selenium.debug=true alone does not (see
+    // configureLoggingDoesNotSuppressSeleniumDebugRecordsOnRootHandlerWhenDebugPropertyIsSetWithoutSeDebug
+    // for that contrasting case).
+    environment.set("SE_DEBUG", "true");

Evidence

The test turns on SE_DEBUG via the environment stub, while the suite-level @AfterEach calls
Debug.configureLogger() but does not clear SE_DEBUG first; Debug’s handler removal only happens when
configureLogger observes debugging disabled.

java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java[61-87]
java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java[183-200]

Agent prompt

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

### Issue description
A test sets `SE_DEBUG` via `EnvironmentVariables`, but the class cleanup path calls `Debug.configureLogger()` without ensuring `SE_DEBUG` is already false/unset. If `Debug.configureLogger()` runs while `SE_DEBUG` is still visible, it will keep its handler installed; after the environment stub restores the variable, the handler can remain attached until some later call.

### Issue Context
`Debug.isHandledBySeleniumDebugHandler` keys off actual handler installation, not the live env/property. A leaked handler can change later tests’ expectations about logger handler lists and can produce unexpected output.

### Fix Focus Areas
- java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java[61-87]
- java/test/org/openqa/selenium/grid/log/LoggingOptionsTest.java[183-200]

### Suggested change
Before calling `Debug.configureLogger()` in `@AfterEach`, explicitly force `SE_DEBUG` to a non-true value using the stub (e.g. `environment.set("SE_DEBUG", "false")`) so configureLogger removes its handler. Alternatively/additionally, call `Debug.configureLogger()` in `@BeforeEach` after clearing properties to re-sync with the restored environment baseline.

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

---

18. Grid drops debug logs ✓ Resolved 🐞 Bug ◔ Observability

Description

LoggingOptions.rootHandlerFilter suppresses org.openqa.selenium FINE/CONFIG records whenever no
log-file is configured, even when Grid’s root handler is writing to System.out (the default under
-Dselenium.debug). This moves Selenium wire diagnostics out of Grid’s root-handler sink/format
(plain/JSON) and into Debug’s System.err ConsoleHandler only, which can be missed when stdout/stderr
are collected separately.

Code

java/src/org/openqa/selenium/grid/log/LoggingOptions.java[R237-242]

+  private Filter rootHandlerFilter() {
+    boolean logFileConfigured = config.get(LOGGING_SECTION, "log-file").isPresent();
+    return record ->
+        logFileConfigured
+            || !Debug.isHandledBySeleniumDebugHandler(record.getLoggerName(), record.getLevel());
+  }

Evidence

Grid installs rootHandlerFilter() on its root handlers, and that filter suppresses records that
Debug’s own handler would emit. But Grid defaults to stdout unless SE_DEBUG is set, while Debug’s
handler is a ConsoleHandler (stderr), so under -Dselenium.debug=true the filter removes Selenium
FINE/CONFIG records from stdout/structured output and leaves only stderr emission.

java/src/org/openqa/selenium/grid/log/LoggingOptions.java[205-219]
java/src/org/openqa/selenium/grid/log/LoggingOptions.java[222-242]
java/src/org/openqa/selenium/grid/log/LoggingOptions.java[261-278]
java/src/org/openqa/selenium/internal/Debug.java[191-236]

[Comment truncated to fit github's 65,536-char limit.]

[MohabMohie]

Thanks for the review, @qodo-code-review — addressed all six findings in two new commits (a31a75aab0, 4296a51e10), each backed by a regression test I watched fail before the fix and pass after.

1 & 2 — Missing Javadoc on the BiDi/CDP Connection constructors. Both constructors now document the client/url(/clientConfig) parameters and note the Debug.configureLogger() side effect, mirroring RemoteWebDriver's canonical-constructor doc block on this branch.

3 — Missing Javadoc on LoggingOptions.configureLogging(). Added, describing the early returns (logging disabled, external JUL config present) and the clean-slate handler reset, matching the style already used on setLoggingLevel() in the same file.

4 — Debug.configureLogger() couldn't recover from an externally-removed handler. The fast-path used to key only on shouldDebug == loggerConfigured, so if something outside Debug removed the installed handler while debugging stayed on (a LogManager reset, or unrelated code calling removeHandler()), it was never reinstalled — even though isHandlerCurrentlyInstalled() already existed to detect exactly that. The fast-path now also consults it. To keep this safe, the level-raising bookkeeping (levelRaisedByDebug/previousLevel) only runs on a genuine off→on transition, not on a repair-only call, so repairing the handler can never corrupt what turning debug off later needs to restore the correct pre-debug level. The turn-off path also now guards against installedHandler already being null (possible if it was removed externally and debug turned off before any repair call ran), since Logger.removeHandler(null) throws an NPE. New test: DebugTest#configureLoggerRepairsAnExternallyRemovedHandlerWithoutCorruptingRestoreBookkeeping.

5 — LoggingOptions' handler-reset loop preserved any handler on org.openqa.selenium, not just Debug's own. Added Debug.isOwnHandler(Handler), a narrow accessor that lets a caller identify specifically the handler configureLogger() installed without exposing the field itself. LoggingOptions now strips everything else from that logger and keeps only the one isOwnHandler() identifies. New test: LoggingOptionsTest#configureLoggingStripsUnrelatedHandlersFromSeleniumLoggerButKeepsDebugsOwn.

6 — LoggingOptionsTest#captureStdOutAndErrDuring leaked root-logger handlers. The helper swapped System.out/System.err back after the wrapped action, but configureLogging() installs root-logger handlers bound to whatever streams were current at call time — those handlers survived the restore and kept writing into the discarded ByteArrayOutputStream, which could pollute later tests/output in the same JVM. The helper now snapshots the root logger's handlers before running the action and removes+closes anything added during it, regardless of which test calls it. New test: LoggingOptionsTest#captureStdOutAndErrDuringDoesNotLeakRootLoggerHandlers.

One note on finding 6's suggested verification approach: the "snapshot handler count before/after" check in the original suggestion doesn't actually catch this bug in this codebase — configureLogging() itself always strips and reinstalls exactly one root handler as an intentional side effect of its own clean-slate reset, so a raw count comparison nets to the same value whether or not the leak is fixed (I verified this empirically both ways). The test instead diffs handler identity (which handlers are new after the call) rather than comparing counts, which correctly fails against the old helper and passes against the fix.

All four scoped Bazel test targets pass: internal:SmallTests, grid/log:SmallTests, remote:small-tests, devtools:small-tests (28/28 tests).

[qodo-code-review]

Thanks for the review, @qodo-code-review — addressed all six findings in two new commits (a31a75aab0, 4296a51e10), each backed by a regression test I watched fail before the fix and pass after.

1 & 2 — Missing Javadoc on the BiDi/CDP Connection constructors. Both constructors now document the client/url(/clientConfig) parameters and note the Debug.configureLogger() side effect, mirroring RemoteWebDriver's canonical-constructor doc block on this branch.

3 — Missing Javadoc on LoggingOptions.configureLogging(). Added, describing the early returns (logging disabled, external JUL config present) and the clean-slate handler reset, matching the style already used on setLoggingLevel() in the same file.

4 — Debug.configureLogger() couldn't recover from an externally-removed handler. The fast-path used to key only on shouldDebug == loggerConfigured, so if something outside Debug removed the installed handler while debugging stayed on (a LogManager reset, or unrelated code calling removeHandler()), it was never reinstalled — even though isHandlerCurrentlyInstalled() already existed to detect exactly that. The fast-path now also consults it. To keep this safe, the level-raising bookkeeping (levelRaisedByDebug/previousLevel) only runs on a genuine off→on transition, not on a repair-only call, so repairing the handler can never corrupt what turning debug off later needs to restore the correct pre-debug level. The turn-off path also now guards against installedHandler already being null (possible if it was removed externally and debug turned off before any repair call ran), since Logger.removeHandler(null) throws an NPE. New test: DebugTest#configureLoggerRepairsAnExternallyRemovedHandlerWithoutCorruptingRestoreBookkeeping.

5 — LoggingOptions' handler-reset loop preserved any handler on org.openqa.selenium, not just Debug's own. Added Debug.isOwnHandler(Handler), a narrow accessor that lets a caller identify specifically the handler configureLogger() installed without exposing the field itself. LoggingOptions now strips everything else from that logger and keeps only the one isOwnHandler() identifies. New test: LoggingOptionsTest#configureLoggingStripsUnrelatedHandlersFromSeleniumLoggerButKeepsDebugsOwn.

6 — LoggingOptionsTest#captureStdOutAndErrDuring leaked root-logger handlers. The helper swapped System.out/System.err back after the wrapped action, but configureLogging() installs root-logger handlers bound to whatever streams were current at call time — those handlers survived the restore and kept writing into the discarded ByteArrayOutputStream, which could pollute later tests/output in the same JVM. The helper now snapshots the root logger's handlers before running the action and removes+closes anything added during it, regardless of which test calls it. New test: LoggingOptionsTest#captureStdOutAndErrDuringDoesNotLeakRootLoggerHandlers.

One note on finding 6's suggested verification approach: the "snapshot handler count before/after" check in the original suggestion doesn't actually catch this bug in this codebase — configureLogging() itself always strips and reinstalls exactly one root handler as an intentional side effect of its own clean-slate reset, so a raw count comparison nets to the same value whether or not the leak is fixed (I verified this empirically both ways). The test instead diffs handler identity (which handlers are new after the call) rather than comparing counts, which correctly fails against the old helper and passes against the fix.

All four scoped Bazel test targets pass: internal:SmallTests, grid/log:SmallTests, remote:small-tests, devtools:small-tests (28/28 tests).

Thanks for the detailed follow-up. The two commits and regression coverage address all six findings:

• finding 1: Javadoc for LoggingOptions.configureLogging().
• findings 2 and 6: Selenium handler preservation and Debug.configureLogger() recovery.
• findings 3 and 4: CDP and BiDi Connection constructor Javadocs.
• finding 5: Root-handler cleanup in captureStdOutAndErrDuring.

The identity-based handler assertion is the appropriate verification here, since a count-only assertion would be masked by configureLogging()'s intentional reset and reinstall behavior. The reported 28/28 passing tests provide good regression coverage.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4296a51

[qodo-code-review]

Code review by qodo was updated up to the latest commit 072512c

[qodo-code-review]

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

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 325168c

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1c44aca

[qodo-code-review]

No code changes since the last review — review skipped

[MohabMohie]

@titusfortner any guidance as to what needs to be updated for this pr to be merged?

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. The Java driver fails to compile ✓ Resolved

Description

RemoteWebDriver calls requireNonNullElseGet without importing that method or qualifying it with
Objects. Compilation stops at the constructor, so the Java driver cannot be built.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[243]

+    this.capabilities = requireNonNullElseGet(capabilities, () -> new ImmutableCapabilities());

Evidence

The only Objects static import is requireNonNull; the new call uses a different method name
without qualification.

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[20-24]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[239-246]

Agent prompt

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

## Issue description
`requireNonNullElseGet` is called without a static import or an `Objects` qualifier, preventing compilation.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[20-32]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[242-243]

## Recommended Fix
Add the appropriate static import or qualify the call with `Objects`, then compile the Java driver.

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

---

2. Legacy diagnostics disappear without parent handlers ✗ Dismissed

Description

enable filters INFO out of Selenium's console handler, while getDebugLogLevel() still emits
legacy diagnostics at INFO when either debug property is enabled. If an application disables
parent handlers and has no other handler accepting INFO, messages from existing BiDi and CDP call
sites are discarded even though the debug switch is on.

Code

java/src/org/openqa/selenium/internal/Debug.java[R171-172]

+      // Leave INFO and above to the existing handlers so those records are not printed twice.
+      handler.setFilter(record -> record.getLevel().intValue() < Level.INFO.intValue());

Evidence

The legacy helper selects INFO under either system property, and BiDi logs through that helper. The
new handler rejects INFO; disabling parent handlers leaves no path for those records unless the
application adds another accepting handler.

java/src/org/openqa/selenium/internal/Debug.java[70-85]
java/src/org/openqa/selenium/internal/Debug.java[165-176]
java/src/org/openqa/selenium/bidi/Connection.java[150-150]

Agent prompt

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

## Issue description
Property-based debugging makes legacy call sites emit INFO, but the new handler rejects INFO. With parent handlers disabled and no other INFO-capable handler, their diagnostics disappear.
## Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[165-176]
- java/src/org/openqa/selenium/internal/Debug.java[75-85]
- java/test/org/openqa/selenium/internal/DebugTest.java[342-369]
## Recommended Fix
Keep INFO records available in configurations that lack an INFO-capable downstream handler without duplicating records in the usual configuration. Add a regression test for a legacy-level record with parent handlers disabled.

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

---

3. Null-capability test depends on a mock ✗ Dismissed

Description

constructorShouldThrowIfCapabilitiesAreNull creates a Mockito mock solely to verify that the
executor is untouched. Because CommandExecutor is functional and the constructor rejects null
capabilities before invoking it, a failing lambda can provide the same contract check without a
mock.

Code

java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[152]

+    CommandExecutor executor = mock(CommandExecutor.class);

Evidence

The checklist asks new tests to avoid mocks where practical. The changed test creates a Mockito mock
at line 152 and verifies no interactions at line 158, while CommandExecutor can be supplied as a
lambda that fails if called, preserving the same API contract check without a mock.

AGENTS.md: Keep Tests Small and Avoid Mocks Where Practical
java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[150-160]
java/src/org/openqa/selenium/remote/CommandExecutor.java[20-24]

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 null-capabilities test uses a Mockito mock even though the executor is a functional interface and the constructor should reject null capabilities before invoking it.

## Fix Focus Areas
- java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[152-158]

## Recommended Fix
Replace the mocked `CommandExecutor` and interaction verification with a lambda implementation that fails if invoked, while retaining the assertion that null capabilities produce the expected `IllegalArgumentException`.

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

---

4. Debug logging leaks into later driver tests ✓ Resolved

Description

restoreDebugState removes the test-added handler and restores the logger level but cannot clear
Debug.enabledLevel or its owned-handler state, so a later RemoteWebDriver construction calls
configureLogger() and re-enables FINE logging with a ConsoleHandler. This happens after the
live-switch test even when both debug properties have been cleared, making later tests
order-dependent; the test also leaves SE_DEBUG unstubbed, so its initial INFO assertion can fail
when that environment variable is true.

Code

java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[R106-111]

+    // Debug logging is one-way, so undo its effects on the logger directly.
+    for (Handler handler : seleniumLogger().getHandlers()) {
+      if (!oldHandlers.contains(handler)) {
+        seleniumLogger().removeHandler(handler);
+      }
+    }

Evidence

The RemoteWebDriver constructor calls configureLogger() on every construction, and
configureLogger() calls enable(enabledLevel) whenever that value remains non-null. The
live-switch test sets that one-way state, while its teardown changes only the logger and its
handlers; a later construction can therefore restore debug logging. Unlike DebugTest, the test
does not stub SE_DEBUG, leaving its initial INFO assertion dependent on the environment.

java/src/org/openqa/selenium/internal/Debug.java[133-144]
java/src/org/openqa/selenium/internal/Debug.java[193-204]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[254-258]
java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[94-128]
java/src/org/openqa/selenium/internal/Debug.java[133-175]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[254-260]

Agent prompt

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

## Issue description
`RemoteWebDriverInitializationTest.restoreDebugState` removes the logger attachment but cannot reset `Debug`'s one-way internal state. Later driver constructions can re-enable `FINE` logging and attach a `ConsoleHandler` after the debug properties are cleared. The test's initial `INFO` assertion also depends on `SE_DEBUG` being unset.

## Fix Focus Areas
- java/test/org/openqa/selenium/remote/RemoteWebDriverInitializationTest.java[94-129]
- java/src/org/openqa/selenium/internal/Debug.java[193-204]

## Recommended Fix
Provide cleanup reachable from the remote test that resets `Debug`'s enabled level and owned handler together, such as a test-only public helper or reflection to invoke `Debug.resetForTesting()`. Use it in `@AfterEach` before restoring the previous logger configuration, or isolate the live-pickup test in its own class or JVM. Add `@ExtendWith(SystemStubsExtension.class)` and `@SystemStub EnvironmentVariables` to set `SE_DEBUG=false`, and verify that a subsequent driver construction with debug properties cleared does not enable logging.

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

---

View medium (2)

5. Null capabilities crash session startup ✓ Resolved

Description

RemoteWebDriver stores a fallback in this.capabilities but passes the original capabilities
argument to startSession. Once the missing import is fixed, a null argument reaches
checkNonW3CCapabilities and is dereferenced instead of using the fallback or receiving the
previous explicit validation error.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[243]

+    this.capabilities = requireNonNullElseGet(capabilities, () -> new ImmutableCapabilities());

Evidence

The fallback is assigned only to the field; session startup receives the unmodified parameter and
dereferences it while checking capability keys.

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[239-246]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[290-293]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[1018-1023]

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 constructor assigns default capabilities for a null argument but passes that null argument to session startup.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[241-246]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[290-293]

## Recommended Fix
If null should be accepted, pass the normalized capabilities to `startSession`; otherwise retain the previous explicit non-null validation. Add a constructor test for null capabilities.

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

---

6. A user's logging level is lost 🐞 Bug ≡ Correctness

Description

configureLogger repairs an externally raised level by setting FINE without updating the earlier
previousLevel snapshot. If a user changes the Selenium logger from FINE to SEVERE during
debugging, a subsequent repair followed by disabling debugging restores the pre-debug level rather
than the user's SEVERE setting.

Code

java/src/org/openqa/selenium/internal/Debug.java[R112-114]

+      if (currentEffectiveLevel.intValue() > Level.FINE.intValue()) {
+        SELENIUM_LOGGER.setLevel(Level.FINE);
+        levelSetByDebug = Level.FINE;

Evidence

The snapshot is taken only when debugging first starts. Repair sets the logger back to FINE, after
which the disable path sees Debug's own FINE value and restores the stale snapshot.

java/src/org/openqa/selenium/internal/Debug.java[90-115]
java/src/org/openqa/selenium/internal/Debug.java[132-145]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[239-246]

Agent prompt

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

## Issue description
A repair overwrites an external logger-level change while retaining the stale pre-debug restoration snapshot.

## Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[107-115]
- java/src/org/openqa/selenium/internal/Debug.java[132-142]

## Recommended Fix
When repairing a level changed externally, preserve that new level as the value to restore when debugging is disabled. Test the sequence of enabling debugging, changing the logger level, repairing, and disabling debugging.

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

Dismiss ↗ | View ↗

---

Context sources

Review mode: Auto: 🧠 Deep: Substantial logger, concurrency, handler-repair, and constructor behavior changes span independent paths.

Tip of the day

💡 Did you know, you can import rules and skills only from the repositories your team follows

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 731522a

Results up to commit 1c48fba ⚖️ Balanced

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. The Java driver fails to compile ✓ Resolved

Description

RemoteWebDriver calls requireNonNullElseGet without importing that method or qualifying it with
Objects. Compilation stops at the constructor, so the Java driver cannot be built.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[243]

+    this.capabilities = requireNonNullElseGet(capabilities, () -> new ImmutableCapabilities());

Evidence

The only Objects static import is requireNonNull; the new call uses a different method name
without qualification.

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[20-24]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[239-246]

Agent prompt

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

## Issue description
`requireNonNullElseGet` is called without a static import or an `Objects` qualifier, preventing compilation.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[20-32]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[242-243]

## Recommended Fix
Add the appropriate static import or qualify the call with `Objects`, then compile the Java driver.

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

---

2. A user's logging level is lost 🐞 Bug ≡ Correctness

Description

configureLogger repairs an externally raised level by setting FINE without updating the earlier
previousLevel snapshot. If a user changes the Selenium logger from FINE to SEVERE during
debugging, a subsequent repair followed by disabling debugging restores the pre-debug level rather
than the user's SEVERE setting.

Code

java/src/org/openqa/selenium/internal/Debug.java[R112-114]

+      if (currentEffectiveLevel.intValue() > Level.FINE.intValue()) {
+        SELENIUM_LOGGER.setLevel(Level.FINE);
+        levelSetByDebug = Level.FINE;

Evidence

The snapshot is taken only when debugging first starts. Repair sets the logger back to FINE, after
which the disable path sees Debug's own FINE value and restores the stale snapshot.

java/src/org/openqa/selenium/internal/Debug.java[90-115]
java/src/org/openqa/selenium/internal/Debug.java[132-145]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[239-246]

Agent prompt

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

## Issue description
A repair overwrites an external logger-level change while retaining the stale pre-debug restoration snapshot.

## Fix Focus Areas
- java/src/org/openqa/selenium/internal/Debug.java[107-115]
- java/src/org/openqa/selenium/internal/Debug.java[132-142]

## Recommended Fix
When repairing a level changed externally, preserve that new level as the value to restore when debugging is disabled. Test the sequence of enabling debugging, changing the logger level, repairing, and disabling debugging.

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

Dismiss ↗ | View ↗

---

3. Null capabilities crash session startup ✓ Resolved

Description

RemoteWebDriver stores a fallback in this.capabilities but passes the original capabilities
argument to startSession. Once the missing import is fixed, a null argument reaches
checkNonW3CCapabilities and is dereferenced instead of using the fallback or receiving the
previous explicit validation error.

Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[243]

+    this.capabilities = requireNonNullElseGet(capabilities, () -> new ImmutableCapabilities());

Evidence

The fallback is assigned only to the field; session startup receives the unmodified parameter and
dereferences it while checking capability keys.

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[239-246]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[290-293]
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[1018-1023]

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 constructor assigns default capabilities for a null argument but passes that null argument to session startup.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[241-246]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[290-293]

## Recommended Fix
If null should be accepted, pass the normalized capabilities to `startSession`; otherwise retain the previous explicit non-null validation. Add a constructor test for null capabilities.

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

---

[diemol]

@titusfortner thanks for laying this out. My take on each point:

1. Purpose: Agree, mainly a compatibility shim. Users who want real control should set up their own JUL or SLF4J config. The switch is a quick way to see Selenium's debug logs.
2. Scope: Agree, Java bindings only. Grid and driver process output stay with SE_DEBUG.
3. Log levels: Agree. More output is fine because it's useful output, but we should mention it in the release notes. Splitting into FINE/FINER/FINEST can be a follow-up.
4. Existing JUL/SLF4J setups: Agree we should be explicit. Don't touch user handlers or the root logger, and don't lower a level that is already more verbose. I don't think we need to restore the previous level when the switch is turned off at runtime, though. Nobody turns debug logging on and off mid-run. You set it, run your tests, and run them again if needed. I'd make it one-way: read the property live, so it works no matter when Selenium classes load, and once it's enabled it stays on for the life of the JVM. That's still clearly defined behavior, and it removes the snapshot/restore logic and the edge cases that come with it.

@MohabMohie a few things before this can be merged:

• It doesn't compile. The last merge from trunk was resolved wrong:
  • RemoteWebDriver.java:243 uses requireNonNullElseGet without an import. Please restore trunk's line, this.capabilities = Require.nonNull("Capabilities", capabilities);. Only the two Debug.configureLogger() calls should change in that file.
  • RemoteWebDriverInitializationTest lost the WebDriverException import, but trunk's tests still use it.
• Please make enabling one-way as described above, and drop the restore logic and its tests.
• The configureLogger() Javadoc should say that users with their own FINE handler will see FINE records twice.

[MohabMohie]

Thanks @diemol, @titusfortner. I've pushed two commits addressing the open feedback:

• 47a495e: restores trunk's RemoteWebDriver lines lost in the trunk merge (Require.nonNull("Capabilities", capabilities) and the WebDriverException import in the test). The only remaining change in that file is the two Debug.configureLogger() calls. This fixes the compile error and the null-capabilities crash Qodo flagged, and it adds constructorShouldThrowIfCapabilitiesAreNull.
• baa8beb: makes enabling one-way as suggested. Switches are read live, and once seen, debug logging stays on for the JVM's lifetime. The snapshot/restore logic and its tests are removed. configureLogger() now uses a private lock instead of static synchronized (fixes the SpotBugs USO_UNSAFE_STATIC_METHOD_SYNCHRONIZATION failure). SE_DEBUG is checked first and has its own level constant. The Javadoc now notes that users with their own FINE handler will see FINE records twice.

Tests: DebugTest (24 tests) covers each switch, live pickup, one-way behaviour, a single handler under 16 concurrent calls, user and root handler preservation, never lowering FINER/FINEST/ALL, and handler repair. Locally, DebugTest, RemoteWebDriverInitializationTest, core-lib-spotbugs, remote:api-spotbugs and the docs targets pass, and google-java-format is clean.

I've also updated the description with a release note (enabling the switch now shows all Selenium FINE diagnostics). Could a maintainer approve the workflow runs so CI can run on the new head?

[diemol]

/agentic_review

[qodo-code-review]

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

[MohabMohie]

Thanks for the review! Pushed two small follow-ups:

• f0801df moves the debug-switch test into its own RemoteWebDriverDebugLoggingTest (separate JVM, env/properties stubbed), so the one-way debug state can't leak into other tests. The null-capabilities test now uses a failing lambda instead of a mock.
• 731522a documents that INFO and above stay with the existing handlers. Passing INFO through would bring the duplicates back, and trunk behaves the same today; those call sites move to FINE in [Feature]: Migrate remaining getDebugLogLevel() call sites (and any other ad-hoc debug-log-level logic) to fixed logger levels #17835.

The older "logging level is lost" finding no longer applies since the restore logic is gone.

Tests pass locally. Could someone approve the workflow runs when you get a chance?