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

[MohabMohie]

🔗 Related Issues

Closes #12892

💥 What does this PR do?

Makes -Dselenium.debug=true and the legacy
-Dselenium.webdriver.verbose=true configure Java JUL diagnostics for
org.openqa.selenium. When enabled, Selenium raises that logger to FINE
only when needed and installs its own console handler for records below
INFO.

This is a Java-binding convenience, not a full SE_DEBUG alias. SE_DEBUG
remains the broader cross-binding maintainer/CI switch; the shared Java
configuration mechanism gives each switch an explicit branch and requested
level rather than inferring the scope from a combined boolean.

The change does not modify Grid root logging, Grid output routing, or
user-installed JUL handlers. Retry diagnostics now use the fixed FINE
level; migrating the remaining deprecated getDebugLogLevel() call sites is
left for a follow-up.

🧪 Testing

• //java/test/org/openqa/selenium/internal:DebugTest
• //java/test/org/openqa/selenium/remote/http:RetryRequestTest
• //java/test/org/openqa/selenium/devtools:ConnectionTest
• //java/test/org/openqa/selenium/remote:RemoteWebDriverInitializationTest

The logger tests cover preserving user handlers and the root logger,
retaining explicit/inherited FINER, FINEST, and ALL levels, and repairing
the Selenium-owned handler and logger level after external resets while debug
remains enabled.

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

Context used

✅ Compliance rules (platform): 18 rules

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

---

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

---

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

---

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

---

View more (17)

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

---

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

---

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

---

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

---

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

---

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

---

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

---

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

---

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

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

### Issue description
`LoggingOptions.configureLogging()` installs `rootHandlerFilter()` on Grid’s root handler(s). The filter suppresses `org.openqa.selenium` FINE/CONFIG records when Debug’s handler is installed and no `log-file` is configured.

However, when debug is enabled via system properties (`-Dselenium.debug=true` / `-Dselenium.webdriver.verbose=true`) **without** `SE_DEBUG`, `getOutputStream()` defaults Grid logging to **stdout**, while `Debug.configureLogger()` writes those suppressed records to **stderr** (ConsoleHandler). This causes Selenium debug diagnostics to disappear from Grid’s primary stdout/structured logs.

### Issue Context
Duplicate suppression should only happen when the Grid root handler is actually writing to the same destination as Debug’s handler (stderr), otherwise it’s not a duplicate and the record should remain in Grid’s configured output/format.

### Fix Focus Areas
- 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]

### Suggested fix
Adjust the suppression condition to account for the root handler’s destination. For example:

- Compute `OutputStream out = getOutputStream();`
- Derive `boolean sameAsDebugDestination = (out == System.err);` (or equivalently `Debug.isDebugAll() && log-file is empty`, matching `getOutputStream()`’s stderr case)
- Only suppress when `sameAsDebugDestination` is true:
 - `return record -> !sameAsDebugDestination || !Debug.isHandledBySeleniumDebugHandler(record.getLoggerName(), record.getLevel());`

This keeps de-duplication in the stderr case while preserving Selenium debug records in Grid’s stdout/plain/JSON output when stdout is the configured sink.

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

---

14. Brittle inherited-level test ✓ Resolved 🐞 Bug ☼ Reliability

Description

DebugTest.configureLoggerDoesNotClampAnInheritedMoreVerboseEffectiveLevel asserts
Logger.getLogger("org.openqa.selenium").getLevel() is null, but the test setup never clears that
level, so the test can fail under external JUL config or leaked JVM logger state before exercising
the behavior under test.

Code

java/test/org/openqa/selenium/internal/DebugTest.java[R255-260]

+    Logger parentLogger = Logger.getLogger("org.openqa");
+    Level oldParentLevel = parentLogger.getLevel();
+    parentLogger.setLevel(Level.FINER);
+    try {
+      assertThat(seleniumLogger().getLevel()).isNull();
+

Evidence

The test asserts a null explicit level for org.openqa.selenium, but the shared logger’s explicit
level is only captured and restored, not reset to null, so the assertion is not guaranteed by the
test setup.

java/test/org/openqa/selenium/internal/DebugTest.java[53-60]
java/test/org/openqa/selenium/internal/DebugTest.java[249-267]

Agent prompt

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

### Issue description
`DebugTest.configureLoggerDoesNotClampAnInheritedMoreVerboseEffectiveLevel` assumes the `org.openqa.selenium` logger has no explicit level (i.e., `getLevel()` returns `null`) to test inherited effective levels. The test does not enforce this precondition, so it can fail when the logger is explicitly configured by the test JVM or leaked state.

### Issue Context
The test’s `@BeforeEach` stores the old level but does not reset it; the test later asserts it is `null`.

### Fix Focus Areas
- java/test/org/openqa/selenium/internal/DebugTest.java[249-277]
- java/test/org/openqa/selenium/internal/DebugTest.java[53-60]

### Suggested fix
In `configureLoggerDoesNotClampAnInheritedMoreVerboseEffectiveLevel`, explicitly set `seleniumLogger().setLevel(null)` before asserting `isNull()` (optionally save/restore the previous value in a `try/finally`, or rely on the existing `@AfterEach` restoration).

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

---

15. Inherited level clamped to FINE ✓ Resolved 🐞 Bug ≡ Correctness

Description

Debug.configureLogger() treats a null explicit level on the org.openqa.selenium logger as needing to
be raised and sets it to Level.FINE, which can lower verbosity when the logger was inheriting a
more-verbose effective level (e.g., FINER/FINEST) from a parent logger. This can silently hide
lower-than-FINE diagnostics when a user enables selenium.debug under an already-verbose JUL
configuration.

Code

java/src/org/openqa/selenium/internal/Debug.java[R189-202]

+        Level currentLevel = SELENIUM_LOGGER.getLevel();
+        // Only raise the level when the logger is currently LESS verbose than FINE (higher
+        // intValue). A null level inherits the parent's (default INFO), so raising applies then
+        // too. An already more-verbose level (FINER, FINEST, ALL) is left alone: lowering it
+        // would make records like W3CHttpResponseCodec's FINER diagnostics unloggable while
+        // "debugging". This reads the logger's own level, not its effective level -- a
+        // more-verbose level inherited from a parent while this logger's own level is unset is
+        // still pinned to FINE, since JUL offers no way to read the effective level.
+        levelRaisedByDebug =
+            currentLevel == null || currentLevel.intValue() > Level.FINE.intValue();
+        if (levelRaisedByDebug) {
+          previousLevel = currentLevel;
+          SELENIUM_LOGGER.setLevel(Level.FINE);
+        } else {

Evidence

The raise decision treats currentLevel == null as needing a raise and then pins the logger to
FINE, while the surrounding comment explicitly states the implementation does not consider the
effective inherited level; this is what causes lowering when a parent is already more verbose.

java/src/org/openqa/selenium/internal/Debug.java[188-204]

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()` decides whether to raise the `org.openqa.selenium` logger by checking only `SELENIUM_LOGGER.getLevel()`. When that explicit level is `null` (inherit), the code assumes it’s effectively INFO and forces `Level.FINE`, which can *lower* verbosity if a parent logger was already set to a more-verbose level (FINER/FINEST/ALL).

### Issue Context
The code comment notes it reads only the logger’s own level; however, JUL’s effective level can be computed by walking parent loggers to the first non-null level. The current behavior violates the “don’t lower an already more verbose level” intent in inherited-level setups.

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

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

---

16. configureLogging() missing required Javadoc ✓ Resolved 📘 Rule violation ✧ Quality

Description

The modified public method configureLogging() has no Javadoc block, violating the requirement for
complete Javadoc on changed public API methods. This reduces maintainability and makes the
debug-switch behavior hard to understand from the API surface.

Code

java/src/org/openqa/selenium/grid/log/LoggingOptions.java[R142-150]

+    // Reflect the current debug switches onto the shared org.openqa.selenium logger before
+    // anything else below -- in particular, before the external-JUL-config early return just
+    // below hands the rest of logging setup off entirely. Without this, Selenium's own FINE-level
+    // wire diagnostics (RequestConverter, the BiDi/CDP Connection classes) stay invisible under
+    // -Dselenium.debug=true whenever an external `java.util.logging.config.*` property is set,
+    // since nothing else on Grid's startup path would ever call this. Idempotent and cheap, same
+    // chokepoint pattern as DriverFinder.getBinaryPaths().
+    Debug.configureLogger();
+

Evidence

PR Compliance ID 330201 requires that each changed public method has a Javadoc block immediately
above the signature with at least one descriptive sentence (and tags as applicable).
configureLogging() is public and modified, but there is no /** ... */ block above its
declaration in the PR branch version.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/grid/log/LoggingOptions.java[124-160]

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 changed public method lacks a Javadoc block. The project requires complete Javadoc on public API methods touched in the change.

## Issue Context
`LoggingOptions.configureLogging()` now calls `Debug.configureLogger()` and contains non-obvious ordering/early-return behavior; this should be documented as part of the method contract.

## Fix Focus Areas
- java/src/org/openqa/selenium/grid/log/LoggingOptions.java[124-160]

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

---

17. Selenium logger excluded from reset ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

LoggingOptions.configureLogging() now skips removing handlers from the org.openqa.selenium logger
unconditionally, so pre-existing handlers on that logger can survive Grid’s logging reset and
potentially cause duplicated or inconsistently formatted output. This exception is broader than
needed to preserve only Debug’s debug-mode handler.

Code

java/src/org/openqa/selenium/grid/log/LoggingOptions.java[R169-172]

+      String name = names.nextElement();
+      if ("org.openqa.selenium".equals(name)) {
+        continue;
+      }

Evidence

The handler-removal loop explicitly skips the org.openqa.selenium logger, leaving its handlers
untouched, even though the method’s purpose is to clear existing handlers before installing Grid’s
root handlers. This can preserve arbitrary handlers, not just Debug’s, because the skip is
unconditional.

java/src/org/openqa/selenium/grid/log/LoggingOptions.java[142-150]
java/src/org/openqa/selenium/grid/log/LoggingOptions.java[160-180]

Agent prompt

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

### Issue description
`configureLogging()` removes handlers from all registered loggers to start from a clean slate, but now skips `org.openqa.selenium` entirely. That preserves Debug’s handler, but also preserves any unrelated/user/framework handlers on that logger, changing the reset semantics and possibly creating duplicate/mixed-format output.

### Issue Context
The motivation is to avoid stripping the handler that `Debug.configureLogger()` may have just installed. However, skipping the entire logger regardless of whether Debug’s handler is present is broader than necessary.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/log/LoggingOptions.java[142-180]

### Suggested fix
Narrow the exception to the actual case that needs preserving, for example:
- Only `continue` for `org.openqa.selenium` when `Debug.isHandlerCurrentlyInstalled()` is `true` (or when debug mode is enabled).
- Otherwise, treat it like other loggers and remove its handlers as before.

If you need to preserve only Debug’s handler (but remove others), consider extending `Debug` with a small API to identify/return the installed handler instance so Grid can remove everything except that handler.

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

---

18. DevTools Connection constructor lacks Javadoc ✓ Resolved 📘 Rule violation ✧ Quality

Description

The modified public constructor Connection(HttpClient, String, ClientConfig) has no Javadoc block,
violating the requirement to document public API constructors. This obscures the side effect of
configuring Selenium debug logging during construction.

Code

java/src/org/openqa/selenium/devtools/Connection.java[R96-101]

[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 59383f6

[MohabMohie]

@titusfortner all clean and ready for your review. 👍

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 529bb21

[qodo-code-review]

Code review by qodo was updated up to the latest commit 96ec9cf

[qodo-code-review]

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

[qodo-code-review]

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

[titusfortner]

My original thought was that we should pick one of the other options for addressing #12892, but after some research I think this direction is correct, but requires agreeing on the semantics based on thee 4 things. I put my opinions here. @diemol / @pujagani / @asolntsev do you have different opinions for these? Any other considerations?

1. Is this intended to be a recommended user-facing troubleshooting shortcut, or mainly a backwards compatibility shim that lets committers use normal Java logging levels consistently while suggesting users implement their own JUL/SLF4J logging implementations?
   • I lean toward the former latter
2. What is the scope of the shortcut? Java binding JUL diagnostics only, Grid logging too, or driver-process logging/stderr like SE_DEBUG? This PR expands the system properties into Grid log level behavior, but not stderr routing.
   • I think we should stick to bindings only for this.
3. What logging-level policy do we want to commit to? The current getDebugLogLevel() call sites are not a curated list of the most useful diagnostics, and indeed probably the most useful diagnostics (from RemoteWebDriver are not included). This PR will significantly increase output for current users of the toggle, but it will include many useful entries that are currently missed. (This also has implications for SE_DEBUG that we'll need to address eventually as well).
   • I think we probably need to plan to differentiate FINE/FINER/FINEST logging in a follow-up PR
4. What compatibility do we owe users with existing JUL/SLF4J bridge setups? SE_DEBUG was implemented primarily for maintainers and CI, without any public documentation or contract. This toggle is public, so we need clear behavior for existing handlers, inherited log levels, external JUL config, duplicate output, dropped records, and restoration when toggles change.
   • I think we should explicitly provide better support to the extent we can

For the Qodo feedback: I don't think we should re-cache isDebugging() as the hot-path fix, or change any retry entries to WARNING, and we can do the rest of the getDebugLogLevel() migration in a follow-up PR. The rest of the Qodo suggestions seem good.

Beyond that, I think this PR should:

1. Update the code so it doesn't assume that SE_DEBUG and this toggle will continue to use the same log level. They can share the same logger-configuration mechanism, but the level/scope should be explicit rather than implied by isDebugging() || isDebugAll().
2. Keep the system-property toggle scoped as a Java JUL diagnostics convenience, not a full SE_DEBUG alias. Please make the PR text/tests reflect that distinction. SE_DEBUG remains the broader cross-binding maintainer/CI switch.
3. Do not deepen reliance on getDebugLogLevel() in touched code. For RetryRequest, either leave the cached helper alone for the follow-up migration or change those retry-attempt logs to fixed Level.FINE; do not replace the cached field with repeated live helper calls.
4. Add/keep tests for the logging-state guarantees we care about:
   • user-installed handlers are not removed or mutated;
   • the root logger is not mutated by the system-property toggle;
   • explicit or inherited FINER/FINEST/ALL is not lowered to FINE;
   • if Debug’s handler and logger level are externally reset while debug remains enabled, configureLogger() repairs both handler installation and FINE loggability without corrupting later restoration.

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