[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 configure Java JUL
diagnostics on the org.openqa.selenium logger.

The mechanism:

• reads the system properties on every call, so runtime changes are honored;
• raises the effective Selenium logger level to FINE only when needed;
• installs one Selenium-owned console handler filtered to records below
  INFO, without modifying caller-owned handlers or the root logger;
• repairs its handler and FINE loggability after external divergence; and
• removes only its own handler and restores only the logger level it changed
  when debugging is disabled.

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

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

The focused tests cover live properties, legacy compatibility, owned-handler
lifecycle and idempotence, explicit and inherited more-verbose levels, user
handler/root logger preservation, duplicate INFO suppression, SE_DEBUG, and
repair after external handler/level changes.

Types of changes

• Bug fix (backwards compatible)

AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude (Anthropic), Codex (OpenAI)
  • 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

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 medium (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]

Agent prompt

[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 9b8a93e

[qodo-code-review]

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

[MohabMohie]

@titusfortner updated accordingly. Kindly check.

[titusfortner]

Only 2 issues I see, but I still want other maintainers to agree to approach, I'll ping in Slack.

1. evaluate isDebugAll() first since we may want to change that to Level.FINEST
2. javadoc for configureLogger() isn't quite correct: users with their own FINE handler would still see duplicate FINE output. (which is fine we just need to clarify)

[MohabMohie]

Qodo's scan has completed for c7ba454. The two remaining summary recommendations are intentionally not adopted: changing retry diagnostics to WARNING and re-caching the live debug-property check both conflict with Titus's stated direction in #17841 (comment). No Qodo review threads are currently unresolved.

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7372c1e

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5ed151e

[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