[java][bidi] Track events subscriptions using subscription ids

[pujagani]

🔗 Related Issues

💥 What does this PR do?

It ensures that all BiDi events are tracked using a subscription id. Subscription id is the unique identifier and source of truth to add or remove listener.

🔧 Implementation Notes

To match the latest version of spec, and as foundation for BiDi generated classes.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s):
  • What was generated: Claude
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)
• Breaking change (fix or feature that would cause existing functionality to change) - Breaking change is in classes marked Beta. Rest ensured proper deprecation cycle as per Selenium practice.

[qodo-code-review]

PR Summary by Qodo

Track BiDi event listeners by protocol subscription id

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Return BiDi subscription IDs from listener registration to match the latest spec.
• Unsubscribe listeners by subscription ID and clean up local callback bookkeeping.
• Update BiDi modules, remote Script API, and tests to use String subscription IDs.

Diagram

sequenceDiagram
  participant User as "Client code"
  participant BiDi as "BiDi"
  participant Conn as "Connection"
  participant Browser as "BiDi server"

  User->>BiDi: addListener(event, handler)
  BiDi->>Browser: session.subscribe(params)
  Browser-->>BiDi: { subscription: id }
  BiDi->>Conn: addListener(id, event, handler)

  User->>BiDi: removeListener(id)
  BiDi->>Conn: isSubscribed(id)
  BiDi->>Browser: session.unsubscribe(subscriptions=[id])
  BiDi->>Conn: removeListener(id)

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce a typed Subscription handle (instead of raw String)

• ➕ Avoids leaking protocol-level String ids into public APIs
• ➕ Allows future metadata (event, contexts) without more breaking changes
• ➕ Can prevent mixing up ids from different subsystems
• ➖ Additional API surface area and new type to maintain
• ➖ Still ultimately wraps a String, so benefits are mostly ergonomics/type-safety

2. Keep existing long listener IDs and map internally to subscription ids

• ➕ Would avoid a breaking change for Java callers
• ➕ Could preserve older patterns while still using protocol ids under the hood
• ➖ Requires maintaining a second identifier system
• ➖ Harder to make subscription id the single source of truth
• ➖ More bookkeeping and potential for leaks/mismatches

3. Track subscription metadata to support precise clearListener(contexts, event)

• ➕ Allows correct context-scoped unsubscribe without falling back to event-wide unsubscribe
• ➕ Reduces surprise for callers relying on context-specific clear
• ➖ Needs additional mapping (subscription id -> {event, contexts})
• ➖ More complex lifecycle management; may be moot if generated BiDi code replaces this path soon

Recommendation: The PR’s approach (treat protocol subscription id as the only identifier) is the right foundation for spec alignment and generated BiDi code. Consider a follow-up to wrap the String in a small Subscription type for API ergonomics, and/or to remove the temporary clearListener(contexts, event) fallback once handwritten modules are retired.

Files changed (11) +95 / -92

Enhancement (2) +50 / -56

BiDi.javaReturn subscription ids from addListener and unsubscribe by id +44/-50

Return subscription ids from addListener and unsubscribe by id

• Changes listener registration to call session.subscribe and return the browser-provided subscription id (String). Adds a subscribe(...) helper that validates the returned id, and updates removeListener to unsubscribe via session.unsubscribe(subscriptions=[id]) guarded by a local isSubscribed check. Simplifies/temporarily degrades context-specific clearListener to delegate to the event-wide clearListener with explanatory comments.

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

Script.javaUpdate Script interface to use String subscription ids +6/-6

Update Script interface to use String subscription ids

• Changes handler add/remove method signatures from long ids to String subscription ids for console messages, JS errors, and DOM mutation events.

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

Refactor (6) +31 / -22

Connection.javaKey event callback registry by subscription id String +18/-9

Key event callback registry by subscription id String

• Replaces the internal event callback map from long ids to subscription-id Strings and updates addListener/removeListener accordingly. Adds isSubscribed(String) to check whether an id is currently tracked, and switches isEventSubscribed to use a read lock (no mutation).

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

LogInspector.javaPropagate subscription id return types for log listeners +3/-3

Propagate subscription id return types for log listeners

• Updates onConsoleEntry/onJavaScriptException and the internal addLogEntryAddedListener helper to return subscription ids as Strings from BiDi.addListener.

java/src/org/openqa/selenium/bidi/module/LogInspector.java

Network.javaReturn subscription id for auth-required listener +1/-1

Return subscription id for auth-required listener

• Updates onAuthRequired to return a String subscription id (matching BiDi.addListener). Other network listeners remain fire-and-forget (void).

java/src/org/openqa/selenium/bidi/module/Network.java

Script.javaReturn subscription id for script message listeners +1/-1

Return subscription id for script message listeners

• Updates onMessage to return the String subscription id so callers can unsubscribe via BiDi.removeListener(id).

java/src/org/openqa/selenium/bidi/module/Script.java

SpeculationInspector.javaUse subscription ids for speculation event listener lifecycle +2/-2

Use subscription ids for speculation event listener lifecycle

• Changes onPrefetchStatusUpdated to return a String subscription id and updates removeListener to accept String and delegate to BiDi.removeListener.

java/src/org/openqa/selenium/bidi/module/SpeculationInspector.java

RemoteScript.javaRemote Script API now returns String ids for handler registration +6/-6

Remote Script API now returns String ids for handler registration

• Updates console/JS error/DOM mutation handler registration methods to return String subscription ids, and updates removal methods to accept String and call BiDi.removeListener(id).

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

Tests (3) +14 / -14

WebScriptTest.javaAdapt Script tests to String subscription ids +11/-11

Adapt Script tests to String subscription ids

• Updates tests to store handler ids as Strings and use them for removal. Keeps behavioral assertions intact while adjusting to the new return type.

java/test/org/openqa/selenium/WebScriptTest.java

NetworkCommandsTest.javaAssert auth callback subscription id is non-blank +2/-2

Assert auth callback subscription id is non-blank

• Updates the auth-required listener test to treat the returned id as a String and assert it is not blank (instead of > 0).

java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java

SpeculationInspectorTest.javaUse String subscription id in speculation unsubscribe test +1/-1

Use String subscription id in speculation unsubscribe test

• Updates speculation inspector tests to store the subscription id as a String when registering listeners.

java/test/org/openqa/selenium/bidi/speculation/SpeculationInspectorTest.java

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (3) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 Instance ChroneDriver Error: ConnectFailure 🎫 2.48 doesn't trigger javascript in link's href on click()

✅ Compliance rules (platform): 18 rules

1. Script handler IDs now String 📘 Rule violation ≡ Correctness ⭐ New

Description

The public org.openqa.selenium.remote.Script interface changed its handler registration/removal
API from long ids to String ids, which is a source- and binary-incompatible change for existing
clients. The change is not introduced via a deprecated compatibility layer (e.g., deprecated
overloads with guidance), so downstream users have no migration path.

Code

java/src/org/openqa/selenium/remote/Script.java[R29-39]

+  String addConsoleMessageHandler(Consumer<ConsoleLogEntry> consumer);

-  void removeConsoleMessageHandler(long id);
+  void removeConsoleMessageHandler(String id);

-  long addJavaScriptErrorHandler(Consumer<JavascriptLogEntry> consumer);
+  String addJavaScriptErrorHandler(Consumer<JavascriptLogEntry> consumer);

-  void removeJavaScriptErrorHandler(long id);
+  void removeJavaScriptErrorHandler(String id);

-  long addDomMutationHandler(Consumer<DomMutation> event);
+  String addDomMutationHandler(Consumer<DomMutation> event);

-  void removeDomMutationHandler(long id);
+  void removeDomMutationHandler(String id);

Evidence

PR Compliance ID 389266 forbids breaking changes to public APIs (including signature changes). PR
Compliance ID 389271 requires that breaking/removal-style changes be preceded by a deprecation
marker that includes guidance to the replacement; here the Script methods were directly changed to
String ids with no compatibility overloads in the same interface.

Rule 389266: Maintain backward-compatible public API and ABI
Rule 389271: Deprecate public APIs with guidance before removal
java/src/org/openqa/selenium/remote/Script.java[29-39]
java/src/org/openqa/selenium/remote/RemoteScript.java[65-120]

Agent prompt

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

## Issue description
`org.openqa.selenium.remote.Script` changed public method signatures from `long` ids to `String` ids, which breaks existing callers and binaries. The API should preserve backward compatibility via deprecated overloads (with clear guidance) or a versioned/new interface.

## Issue Context
These methods are public and part of the library API surface (`public interface Script`). The current change provides no deprecated bridge methods for existing `long`-based consumers.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/Script.java[29-39]
- java/src/org/openqa/selenium/remote/RemoteScript.java[65-120]

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

---

2. New Script methods lack Javadoc 📘 Rule violation ✧ Quality

Description

New public API methods were added to org.openqa.selenium.remote.Script without any Javadoc,
missing required @param/@return documentation for callers.

Code

java/src/org/openqa/selenium/remote/Script.java[R43-46]

+  String addConsoleMessageListener(Consumer<ConsoleLogEntry> consumer);
+
+  void removeConsoleMessageListener(String id);
+

Evidence

The checklist requires Javadoc immediately above each changed/added public method, including
@param tags for parameters and @return for non-void returns. The newly added
add*Listener/remove*Listener methods are public but have no Javadoc at all.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/remote/Script.java[43-46]
java/src/org/openqa/selenium/remote/Script.java[61-64]
java/src/org/openqa/selenium/remote/Script.java[79-82]

Agent prompt

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

## Issue description
New public methods were added to the `Script` interface without Javadoc blocks, which violates the requirement for complete Javadoc on public API methods.

## Issue Context
The deprecated long-id methods in the same interface have Javadoc, but the new String subscription-id methods do not.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/Script.java[43-46]
- java/src/org/openqa/selenium/remote/Script.java[61-64]
- java/src/org/openqa/selenium/remote/Script.java[79-82]

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

---

3. @deprecated missing forRemoval ✓ Resolved 📘 Rule violation ✧ Quality

Description

RemoteScript adds several @Deprecated annotations without forRemoval = true, which violates
the project deprecation policy and creates inconsistent removal intent across the API surface.

Code

java/src/org/openqa/selenium/remote/RemoteScript.java[R72-80]

+  @Deprecated
  public long addConsoleMessageHandler(Consumer<ConsoleLogEntry> consumer) {
-    return this.logInspector.onConsoleEntry(consumer);
+    return trackLegacyId(addConsoleMessageListener(consumer));
  }

  @Override
+  @Deprecated
  public void removeConsoleMessageHandler(long id) {
+    removeConsoleMessageListener(resolveLegacyId(id));

Evidence

The compliance rule requires every @Deprecated usage to explicitly set forRemoval = true. In
RemoteScript, multiple methods are annotated with plain @Deprecated (no forRemoval attribute).

Rule 330196: Use @Deprecated(forRemoval = true) for all deprecated elements
java/src/org/openqa/selenium/remote/RemoteScript.java[71-79]
java/src/org/openqa/selenium/remote/RemoteScript.java[93-101]
java/src/org/openqa/selenium/remote/RemoteScript.java[115-157]

Agent prompt

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

## Issue description
`RemoteScript` introduces `@Deprecated` annotations that omit `forRemoval = true`, which violates the deprecation compliance rule.

## Issue Context
This PR already uses `@Deprecated(since = "4.46", forRemoval = true)` in `org.openqa.selenium.remote.Script`, but the overriding/implementing methods in `RemoteScript` were annotated as plain `@Deprecated`.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteScript.java[71-159]

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

---

View more (1)

4. Context clear ignores contexts 🐞 Bug ≡ Correctness

Description

BiDi.clearListener(Set<String>, Event) now ignores the provided browsingContextIds and delegates to
clearListener(Event), which unsubscribes the event globally and clears all handlers for that event.
This can unintentionally stop events/listeners for other contexts (e.g., when multiple inspectors
are active) and changes behavior for existing call sites that expect per-context cleanup.

Code

java/src/org/openqa/selenium/bidi/BiDi.java[R127-134]

  public <X> void clearListener(Set<String> browsingContextIds, Event<X> event) {
    Require.nonEmpty("List of browsing context ids", browsingContextIds);
+    // This is not the correct implementation, everything needs to be tired to subscription id.
+    // This is currently used by hand-written BiDi code, soon to be replaced by generated BiDi code.
+    // After that this method will be removed.
+    // This is just to avoid changes to hand-written classes meanwhile.
+    this.clearListener(event);
+  }

Evidence

The scoped clear method explicitly discards the context set and calls the global clear method;
modules use the scoped method for per-context cleanup, so this becomes a broad unsubscribe/clear
affecting other listeners.

java/src/org/openqa/selenium/bidi/BiDi.java[127-145]
java/src/org/openqa/selenium/bidi/Connection.java[192-200]
java/src/org/openqa/selenium/bidi/module/LogInspector.java[157-165]
java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java[246-265]
java/src/org/openqa/selenium/bidi/module/SpeculationInspector.java[74-82]

Agent prompt

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

### Issue description
`BiDi.clearListener(Set<String>, Event)` currently ignores the contexts argument and performs a global unsubscribe/clear for the event, which can remove listeners belonging to other context-scoped subscriptions.

### Issue Context
Multiple BiDi modules call the scoped `clearListener(Set, Event)` to clean up a specific browsing context. With the current implementation, calling this for one context can unsubscribe the event entirely and clear all handlers.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[127-145]
- java/src/org/openqa/selenium/bidi/Connection.java[192-221]
- java/src/org/openqa/selenium/bidi/module/LogInspector.java[157-165]
- java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java[246-265]
- java/src/org/openqa/selenium/bidi/module/SpeculationInspector.java[74-82]

### Suggested direction
Track subscriptionIds created by the context-scoped `addListener(...)` overloads (store subscriptionId + associated contexts + event), and implement `clearListener(Set, Event)` by finding matching subscriptionIds and calling `removeListener(subscriptionId)` for each. This preserves the scoped behavior without reverting to event-based unsubscribe.

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

---

5. Legacy id lost on failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

RemoteScript.resolveLegacyId removes the legacy-id mapping before attempting to unsubscribe; if
unsubscribe fails, the legacy id is permanently lost even though the underlying subscription may
still be active. This can strand subscriptions and prevent retrying cleanup via the deprecated
long-based API.

Code

java/src/org/openqa/selenium/remote/RemoteScript.java[R172-178]

+  private String resolveLegacyId(long id) {
+    String subscriptionId = legacySubscriptionIds.remove(id);
+    if (subscriptionId == null) {
+      throw new WebDriverException("No listener registered with id " + id);
+    }
+    return subscriptionId;
+  }

Evidence

The mapping is removed inside resolveLegacyId and used by remove*Handler methods prior to calling
the real unsubscribe; a failure after that point makes subsequent removal attempts impossible.

java/src/org/openqa/selenium/remote/RemoteScript.java[77-91]
java/src/org/openqa/selenium/remote/RemoteScript.java[172-178]

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 deprecated long-based removal flow deletes the legacy-id mapping before the actual unsubscribe happens, so failures leave no way to retry removal.

### Issue Context
`resolveLegacyId` uses `legacySubscriptionIds.remove(id)` and is called before invoking `biDi.removeListener(subscriptionId)`.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteScript.java[77-91]
- java/src/org/openqa/selenium/remote/RemoteScript.java[166-178]

### Suggested direction
Change the flow to:
1) look up subscription id without removing it (e.g., `get`),
2) attempt `biDi.removeListener(subscriptionId)`,
3) only upon success, remove the legacy mapping.
If unsubscribe throws, keep the mapping so users can retry cleanup.

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

---

6. New Script methods not default ✓ Resolved 📘 Rule violation ⌂ Architecture

Description

The new Script interface methods are abstract and provide no default implementation or in-code
justification, forcing all implementers to update immediately.

Code

java/src/org/openqa/selenium/remote/Script.java[R43-46]

+  String addConsoleMessageListener(Consumer<ConsoleLogEntry> consumer);
+
+  void removeConsoleMessageListener(String id);
+

Evidence

The compliance rule requires newly added interface methods to be default (or otherwise have a
concrete implementation) or to include explicit justification in the same change set. The new
listener methods are added as abstract declarations with no default bodies and no adjacent
justification comment.

Rule 330197: Default implementations for new interface methods
java/src/org/openqa/selenium/remote/Script.java[43-46]
java/src/org/openqa/selenium/remote/Script.java[61-64]
java/src/org/openqa/selenium/remote/Script.java[79-82]

Agent prompt

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

## Issue description
New interface methods were added to `org.openqa.selenium.remote.Script` without default bodies and without a design note explaining why defaults are not feasible.

## Issue Context
Even though this API is `@Beta`, adding abstract methods to an existing public interface is a source/binary compatibility hazard for any external implementations.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/Script.java[43-46]
- java/src/org/openqa/selenium/remote/Script.java[61-64]
- java/src/org/openqa/selenium/remote/Script.java[79-82]

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

---

7. Local leak on unsubscribe error 🐞 Bug ☼ Reliability

Description

BiDi.removeListener unsubscribes remotely first and only then removes the local callback; if the
remote unsubscribe command throws, the local callback is never removed. This can leave callbacks
registered indefinitely and make listener cleanup inconsistent when removal fails for any reason.

Code

java/src/org/openqa/selenium/bidi/BiDi.java[R147-152]

+  public void removeListener(String id) {
+    Require.nonNull("Listener id", id);
+
+    send(new Command<>("session.unsubscribe", Map.of("subscriptions", List.of(id))));
    connection.removeListener(id);
  }

Evidence

The method performs a remote unsubscribe and then local removal; if the send throws, execution never
reaches Connection.removeListener, which is the code responsible for clearing handlers from
eventCallbacks.

java/src/org/openqa/selenium/bidi/BiDi.java[147-152]
java/src/org/openqa/selenium/bidi/Connection.java[202-221]

Agent prompt

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

### Issue description
`BiDi.removeListener(String)` can leave local callbacks registered if the remote `session.unsubscribe` fails/throws.

### Issue Context
Local handler removal is performed only after the remote unsubscribe succeeds, with no `try/finally`.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[147-152]
- java/src/org/openqa/selenium/bidi/Connection.java[202-221]

### Suggested direction
Wrap the remote unsubscribe in a try/finally and always call `connection.removeListener(id)` in the finally block. Optionally rethrow the remote exception after local cleanup (or selectively suppress “unknown subscription id” failures if that is considered non-fatal).

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

---

View more (1)

8. BiDi.addListener return type changed 📘 Rule violation ≡ Correctness

Description

BiDi.addListener and BiDi.removeListener were changed from long-based ids to String subscription
ids without preserving the old signatures, which can break existing callers compiled against the
previous API.

Code

java/src/org/openqa/selenium/bidi/BiDi.java[R86-93]

+  public <X> String addListener(Event<X> event, Consumer<X> handler) {
    Require.nonNull("Event to listen for", event);
    Require.nonNull("Handler to call", handler);

-    send(new Command<>("session.subscribe", Map.of("events", List.of(event.getMethod()))));
-
-    return connection.addListener(event, handler);
+    String subscriptionId = subscribe(Map.of("events", List.of(event.getMethod())));
+    connection.addListener(subscriptionId, event, handler);
+    return subscriptionId;
  }

Evidence

The compliance rule requires maintaining backward-compatible public APIs. The BiDi public methods
in the changed lines now use String ids, with no retained overloads for the prior long
signatures.

Rule 389266: Maintain backward-compatible public API and ABI
java/src/org/openqa/selenium/bidi/BiDi.java[86-93]
java/src/org/openqa/selenium/bidi/BiDi.java[147-152]

Agent prompt

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

## Issue description
Public `BiDi` listener APIs changed signature (long -> String), which is a backward-incompatible change for existing clients.

## Issue Context
Although `BiDi` is annotated `@Beta`, the compatibility rule still flags breaking changes unless the old API is preserved (typically via deprecated overloads) or otherwise version-gated.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[86-93]
- java/src/org/openqa/selenium/bidi/BiDi.java[147-152]

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

---

9. Unchecked subscription id ✓ Resolved 🐞 Bug ☼ Reliability

Description

BiDi.subscribe casts result.get("subscription") to String without validating presence/type, so a
malformed or unexpected response can produce a null/invalid subscriptionId that later fails in
listener registration with a less actionable error. Adding validation would improve robustness and
diagnostics for protocol mismatches.

Code

java/src/org/openqa/selenium/bidi/BiDi.java[R122-125]

+  private String subscribe(Map<String, Object> params) {
+    Map<String, Object> result = send(new Command<>("session.subscribe", params, Map.class));
+    return (String) result.get("subscription");
  }

Evidence

subscribe returns a raw cast from the result map, while Connection.addListener enforces non-null
subscription id and will fail if subscribe returns null.

java/src/org/openqa/selenium/bidi/BiDi.java[119-125]
java/src/org/openqa/selenium/bidi/Connection.java[178-187]

Agent prompt

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

### Issue description
`subscribe(...)` returns `(String) result.get("subscription")` without checking for null/type/blank.

### Issue Context
Downstream code requires a non-null subscription id; if the response is missing/invalid, the exception will be thrown later and may not clearly explain the real protocol problem.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[122-125]
- java/src/org/openqa/selenium/bidi/Connection.java[178-187]

### Suggested direction
Extract the value, validate it is a non-blank `String`, and if not, throw a `WebDriverException`/`BiDiException` including the returned result map (or at least the keys) to aid debugging.

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

---

Previous review results

Review updated until commit 320d009

Results up to commit f4b1cf1

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

1. New Script methods lack Javadoc 📘 Rule violation ✧ Quality

Description

New public API methods were added to org.openqa.selenium.remote.Script without any Javadoc,
missing required @param/@return documentation for callers.

Code

java/src/org/openqa/selenium/remote/Script.java[R43-46]

+  String addConsoleMessageListener(Consumer<ConsoleLogEntry> consumer);
+
+  void removeConsoleMessageListener(String id);
+

Evidence

The checklist requires Javadoc immediately above each changed/added public method, including
@param tags for parameters and @return for non-void returns. The newly added
add*Listener/remove*Listener methods are public but have no Javadoc at all.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/remote/Script.java[43-46]
java/src/org/openqa/selenium/remote/Script.java[61-64]
java/src/org/openqa/selenium/remote/Script.java[79-82]

Agent prompt

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

## Issue description
New public methods were added to the `Script` interface without Javadoc blocks, which violates the requirement for complete Javadoc on public API methods.

## Issue Context
The deprecated long-id methods in the same interface have Javadoc, but the new String subscription-id methods do not.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/Script.java[43-46]
- java/src/org/openqa/selenium/remote/Script.java[61-64]
- java/src/org/openqa/selenium/remote/Script.java[79-82]

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

---

2. Context clear ignores contexts 🐞 Bug ≡ Correctness

Description

BiDi.clearListener(Set<String>, Event) now ignores the provided browsingContextIds and delegates to
clearListener(Event), which unsubscribes the event globally and clears all handlers for that event.
This can unintentionally stop events/listeners for other contexts (e.g., when multiple inspectors
are active) and changes behavior for existing call sites that expect per-context cleanup.

Code

java/src/org/openqa/selenium/bidi/BiDi.java[R127-134]

  public <X> void clearListener(Set<String> browsingContextIds, Event<X> event) {
    Require.nonEmpty("List of browsing context ids", browsingContextIds);
+    // This is not the correct implementation, everything needs to be tired to subscription id.
+    // This is currently used by hand-written BiDi code, soon to be replaced by generated BiDi code.
+    // After that this method will be removed.
+    // This is just to avoid changes to hand-written classes meanwhile.
+    this.clearListener(event);
+  }

Evidence

The scoped clear method explicitly discards the context set and calls the global clear method;
modules use the scoped method for per-context cleanup, so this becomes a broad unsubscribe/clear
affecting other listeners.

java/src/org/openqa/selenium/bidi/BiDi.java[127-145]
java/src/org/openqa/selenium/bidi/Connection.java[192-200]
java/src/org/openqa/selenium/bidi/module/LogInspector.java[157-165]
java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java[246-265]
java/src/org/openqa/selenium/bidi/module/SpeculationInspector.java[74-82]

Agent prompt

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

### Issue description
`BiDi.clearListener(Set<String>, Event)` currently ignores the contexts argument and performs a global unsubscribe/clear for the event, which can remove listeners belonging to other context-scoped subscriptions.

### Issue Context
Multiple BiDi modules call the scoped `clearListener(Set, Event)` to clean up a specific browsing context. With the current implementation, calling this for one context can unsubscribe the event entirely and clear all handlers.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[127-145]
- java/src/org/openqa/selenium/bidi/Connection.java[192-221]
- java/src/org/openqa/selenium/bidi/module/LogInspector.java[157-165]
- java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java[246-265]
- java/src/org/openqa/selenium/bidi/module/SpeculationInspector.java[74-82]

### Suggested direction
Track subscriptionIds created by the context-scoped `addListener(...)` overloads (store subscriptionId + associated contexts + event), and implement `clearListener(Set, Event)` by finding matching subscriptionIds and calling `removeListener(subscriptionId)` for each. This preserves the scoped behavior without reverting to event-based unsubscribe.

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

---

3. @deprecated missing forRemoval ✓ Resolved 📘 Rule violation ✧ Quality

Description

RemoteScript adds several @Deprecated annotations without forRemoval = true, which violates
the project deprecation policy and creates inconsistent removal intent across the API surface.

Code

java/src/org/openqa/selenium/remote/RemoteScript.java[R72-80]

+  @Deprecated
  public long addConsoleMessageHandler(Consumer<ConsoleLogEntry> consumer) {
-    return this.logInspector.onConsoleEntry(consumer);
+    return trackLegacyId(addConsoleMessageListener(consumer));
  }

  @Override
+  @Deprecated
  public void removeConsoleMessageHandler(long id) {
+    removeConsoleMessageListener(resolveLegacyId(id));

Evidence

The compliance rule requires every @Deprecated usage to explicitly set forRemoval = true. In
RemoteScript, multiple methods are annotated with plain @Deprecated (no forRemoval attribute).

Rule 330196: Use @Deprecated(forRemoval = true) for all deprecated elements
java/src/org/openqa/selenium/remote/RemoteScript.java[71-79]
java/src/org/openqa/selenium/remote/RemoteScript.java[93-101]
java/src/org/openqa/selenium/remote/RemoteScript.java[115-157]

Agent prompt

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

## Issue description
`RemoteScript` introduces `@Deprecated` annotations that omit `forRemoval = true`, which violates the deprecation compliance rule.

## Issue Context
This PR already uses `@Deprecated(since = "4.46", forRemoval = true)` in `org.openqa.selenium.remote.Script`, but the overriding/implementing methods in `RemoteScript` were annotated as plain `@Deprecated`.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteScript.java[71-159]

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

---

4. New Script methods not default 📘 Rule violation ⌂ Architecture

Description

The new Script interface methods are abstract and provide no default implementation or in-code
justification, forcing all implementers to update immediately.

Code

java/src/org/openqa/selenium/remote/Script.java[R43-46]

+  String addConsoleMessageListener(Consumer<ConsoleLogEntry> consumer);
+
+  void removeConsoleMessageListener(String id);
+

Evidence

The compliance rule requires newly added interface methods to be default (or otherwise have a
concrete implementation) or to include explicit justification in the same change set. The new
listener methods are added as abstract declarations with no default bodies and no adjacent
justification comment.

Rule 330197: Default implementations for new interface methods
java/src/org/openqa/selenium/remote/Script.java[43-46]
java/src/org/openqa/selenium/remote/Script.java[61-64]
java/src/org/openqa/selenium/remote/Script.java[79-82]

Agent prompt

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

## Issue description
New interface methods were added to `org.openqa.selenium.remote.Script` without default bodies and without a design note explaining why defaults are not feasible.

## Issue Context
Even though this API is `@Beta`, adding abstract methods to an existing public interface is a source/binary compatibility hazard for any external implementations.

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/Script.java[43-46]
- java/src/org/openqa/selenium/remote/Script.java[61-64]
- java/src/org/openqa/selenium/remote/Script.java[79-82]

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

---

5. BiDi.addListener return type changed 📘 Rule violation ≡ Correctness

Description

BiDi.addListener and BiDi.removeListener were changed from long-based ids to String subscription
ids without preserving the old signatures, which can break existing callers compiled against the
previous API.

Code

java/src/org/openqa/selenium/bidi/BiDi.java[R86-93]

+  public <X> String addListener(Event<X> event, Consumer<X> handler) {
    Require.nonNull("Event to listen for", event);
    Require.nonNull("Handler to call", handler);

-    send(new Command<>("session.subscribe", Map.of("events", List.of(event.getMethod()))));
-
-    return connection.addListener(event, handler);
+    String subscriptionId = subscribe(Map.of("events", List.of(event.getMethod())));
+    connection.addListener(subscriptionId, event, handler);
+    return subscriptionId;
  }

Evidence

The compliance rule requires maintaining backward-compatible public APIs. The BiDi public methods
in the changed lines now use String ids, with no retained overloads for the prior long
signatures.

Rule 389266: Maintain backward-compatible public API and ABI
java/src/org/openqa/selenium/bidi/BiDi.java[86-93]
java/src/org/openqa/selenium/bidi/BiDi.java[147-152]

Agent prompt

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

## Issue description
Public `BiDi` listener APIs changed signature (long -> String), which is a backward-incompatible change for existing clients.

## Issue Context
Although `BiDi` is annotated `@Beta`, the compatibility rule still flags breaking changes unless the old API is preserved (typically via deprecated overloads) or otherwise version-gated.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[86-93]
- java/src/org/openqa/selenium/bidi/BiDi.java[147-152]

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

---

6. Local leak on unsubscribe error 🐞 Bug ☼ Reliability

Description

BiDi.removeListener unsubscribes remotely first and only then removes the local callback; if the
remote unsubscribe command throws, the local callback is never removed. This can leave callbacks
registered indefinitely and make listener cleanup inconsistent when removal fails for any reason.

Code

java/src/org/openqa/selenium/bidi/BiDi.java[R147-152]

+  public void removeListener(String id) {
+    Require.nonNull("Listener id", id);
+
+    send(new Command<>("session.unsubscribe", Map.of("subscriptions", List.of(id))));
    connection.removeListener(id);
  }

Evidence

The method performs a remote unsubscribe and then local removal; if the send throws, execution never
reaches Connection.removeListener, which is the code responsible for clearing handlers from
eventCallbacks.

java/src/org/openqa/selenium/bidi/BiDi.java[147-152]
java/src/org/openqa/selenium/bidi/Connection.java[202-221]

Agent prompt

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

### Issue description
`BiDi.removeListener(String)` can leave local callbacks registered if the remote `session.unsubscribe` fails/throws.

### Issue Context
Local handler removal is performed only after the remote unsubscribe succeeds, with no `try/finally`.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[147-152]
- java/src/org/openqa/selenium/bidi/Connection.java[202-221]

### Suggested direction
Wrap the remote unsubscribe in a try/finally and always call `connection.removeListener(id)` in the finally block. Optionally rethrow the remote exception after local cleanup (or selectively suppress “unknown subscription id” failures if that is considered non-fatal).

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

---

View more (1)

7. Legacy id lost on failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

RemoteScript.resolveLegacyId removes the legacy-id mapping before attempting to unsubscribe; if
unsubscribe fails, the legacy id is permanently lost even though the underlying subscription may
still be active. This can strand subscriptions and prevent retrying cleanup via the deprecated
long-based API.

Code

java/src/org/openqa/selenium/remote/RemoteScript.java[R172-178]

+  private String resolveLegacyId(long id) {
+    String subscriptionId = legacySubscriptionIds.remove(id);
+    if (subscriptionId == null) {
+      throw new WebDriverException("No listener registered with id " + id);
+    }
+    return subscriptionId;
+  }

Evidence

The mapping is removed inside resolveLegacyId and used by remove*Handler methods prior to calling
the real unsubscribe; a failure after that point makes subsequent removal attempts impossible.

java/src/org/openqa/selenium/remote/RemoteScript.java[77-91]
java/src/org/openqa/selenium/remote/RemoteScript.java[172-178]

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 deprecated long-based removal flow deletes the legacy-id mapping before the actual unsubscribe happens, so failures leave no way to retry removal.

### Issue Context
`resolveLegacyId` uses `legacySubscriptionIds.remove(id)` and is called before invoking `biDi.removeListener(subscriptionId)`.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteScript.java[77-91]
- java/src/org/openqa/selenium/remote/RemoteScript.java[166-178]

### Suggested direction
Change the flow to:
1) look up subscription id without removing it (e.g., `get`),
2) attempt `biDi.removeListener(subscriptionId)`,
3) only upon success, remove the legacy mapping.
If unsubscribe throws, keep the mapping so users can retry cleanup.

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

---

8. Unchecked subscription id ✓ Resolved 🐞 Bug ☼ Reliability

Description

BiDi.subscribe casts result.get("subscription") to String without validating presence/type, so a
malformed or unexpected response can produce a null/invalid subscriptionId that later fails in
listener registration with a less actionable error. Adding validation would improve robustness and
diagnostics for protocol mismatches.

Code

java/src/org/openqa/selenium/bidi/BiDi.java[R122-125]

+  private String subscribe(Map<String, Object> params) {
+    Map<String, Object> result = send(new Command<>("session.subscribe", params, Map.class));
+    return (String) result.get("subscription");
  }

Evidence

subscribe returns a raw cast from the result map, while Connection.addListener enforces non-null
subscription id and will fail if subscribe returns null.

java/src/org/openqa/selenium/bidi/BiDi.java[119-125]
java/src/org/openqa/selenium/bidi/Connection.java[178-187]

Agent prompt

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

### Issue description
`subscribe(...)` returns `(String) result.get("subscription")` without checking for null/type/blank.

### Issue Context
Downstream code requires a non-null subscription id; if the response is missing/invalid, the exception will be thrown later and may not clearly explain the real protocol problem.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[122-125]
- java/src/org/openqa/selenium/bidi/Connection.java[178-187]

### Suggested direction
Extract the value, validate it is a non-blank `String`, and if not, throw a `WebDriverException`/`BiDiException` including the returned result map (or at least the keys) to aid debugging.

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

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 320d009