[java][bidi] Add BiDi code generator

[pujagani]

🔗 Related Issues

Adheres to the ADR #17786 and matches also decisions there.
8th one was explicitly decided does not work for Java and so was dropped in discussion with the TLC.

💥 What does this PR do?

Adds the BiDi Java client generator: reads the shared, binding-neutral BiDi schema and emits the full typed Java BiDi protocol layer — module classes (one per domain, with commands and events) plus supporting POJOs, enums, and discriminated unions — across all 14 domain, on top of the hand-written seam (Module base class + Handle) that makes it usable.

Shape of a generated module class (real output, network.Network trimmed to 2 events + 2 commands as example):

// This file is generated. Do not edit — regenerate via BiDiGenerator.

package org.openqa.selenium.bidi.protocol.module;

import org.openqa.selenium.Beta;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.bidi.Command;
import org.openqa.selenium.bidi.ConverterFunctions;
import org.openqa.selenium.bidi.Event;
import org.openqa.selenium.bidi.Module;

/**
 * This is an unsupported API. No compatibility guarantees are provided.
 * It tracks the W3C WebDriver BiDi specification directly. As the specification
 * evolves, this API will change or be removed without prior notice.
 */
@Beta
@SuppressWarnings("unchecked")
public class Network extends Module {

  public static final Event<AuthRequiredParameters> AUTH_REQUIRED =
      new Event<>("network.authRequired", ConverterFunctions.fromMap(AuthRequiredParameters.class));

  public static final Event<BeforeRequestSentParameters> BEFORE_REQUEST_SENT =
      new Event<>("network.beforeRequestSent", ConverterFunctions.fromMap(BeforeRequestSentParameters.class));

  // ... 3 more events

  public Network(WebDriver driver) {
    super(driver);
  }

  public AddInterceptResult addIntercept(AddInterceptParameters params) {
    return send(new Command<>("network.addIntercept", params.toMap(), AddInterceptResult.class));
  }

  public void continueRequest(ContinueRequestParameters params) {
    send(new Command<>("network.continueRequest", params.toMap()));
  }

  // ... 11 more commands
}

Usage is symmetric regardless of whether a command has a result:

Network network = new Network(driver);

// command with a result
AddInterceptResult result = network.addIntercept(new AddInterceptParameters(List.of(InterceptPhase.BEFOREREQUESTSENT)));

// command with no result
network.removeIntercept(new RemoveInterceptParameters(result.getIntercept()));

// event: subscribe returns the browser-issued subscription id; unsubscribe takes it back
String subscriptionId = network.subscribe(Network.BEFORE_REQUEST_SENT, event -> { ... });
network.unsubscribe(subscriptionId);

🔧 Implementation Notes

Autogenerated code and not checked in

The build compiles the genrule's srcjar directly:

bazel build //java/src/org/openqa/selenium/bidi:bidi-generated

No generated .java is ever committed.

Trade-off accepted knowingly: less reviewable diff surface for generator changes. Given Java has a lot of generated classes, strict typing means one .java file per record/enum/union, not a handful of dynamically-typed modules like the JS or Python bindings. The actual generated code never appears in this PR's diff, only the generator that produces it.

How to see the generated output since it's not in the diff, pull it out locally:

bazel build //java/src/org/openqa/selenium/bidi:bidi-generated
 
mkdir -p /tmp/bidi-generated-src
unzip -o -q bazel-bin/java/src/org/openqa/selenium/bidi/bidi-generated.srcjar -d /tmp/bidi-generated-src
open /tmp/bidi-generated-src   # or just browse/grep it

If you'd rather regenerate directly without the Bazel packaging step (useful for diffing output after a generator change):

bazel build //java/src/org/openqa/selenium/bidi:bidi-client-generator
 
bazel run //java/src/org/openqa/selenium/bidi:bidi-client-generator -- \
    "$(bazel info bazel-genfiles)/javascript/selenium-webdriver/create-bidi-src_schema.json" \
    /tmp/bidi-out.srcjar
 
unzip -o -q /tmp/bidi-out.srcjar -d /tmp/bidi-generated-src

(Direct invocation of the built binary without bazel run fails with Cannot locate runfiles directory — it needs bazel run to set up its runfiles tree.)

---

🤖 AI assistance

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

💡 Additional Considerations

1. Test coverage is not uniform across domains.
   Only network, log, script, and browsingContext have dedicated generated-code tests (unit and/or browser-integration) added in this PR.
   session, browser, emulation, storage, input, webExtension, permissions, speculation, bluetooth, userAgentClientHints have none yet.
   Essentially, the modules that are needed for implementing high-level APIs for Selenium 5 have tests.

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

[qodo-code-review]

PR Summary by Qodo

Add Bazel-based Java BiDi protocol code generator

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Add a Java generator that emits typed BiDi modules/POJOs from the shared schema.
• Compile generated sources from a build-time srcjar; do not commit generated .java.
• Add unit and browser-integration tests for unions, nullability, and module events/commands.

Diagram

graph TD
  A["BiDi schema (JSON)"] --> B["Bazel genrule"] --> C["BiDiGenerator (java_binary)"] --> D[("bidi-generated.srcjar")]
  D --> E["bidi-generated (java_library)"] --> F["Generated protocol modules/POJOs"] --> G["Unit + integration tests"]
  E --> H["BiDi runtime (Command/Event/Module)"]
  subgraph Legend
    direction LR
    _file["Build input/output"] ~~~ _proc["Build step"] ~~~ _jar[("Artifact")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Check in generated sources + verification test

• ➕ Much more reviewable diffs when generator changes
• ➕ Easier for downstream users to browse protocol surface without building
• ➖ Large churn in repo history and PR diffs (hundreds of files)
• ➖ Requires CI enforcement to keep generated sources in sync
• ➖ Adds friction when schema changes frequently

2. Use a codegen library (e.g., JavaPoet)

• ➕ Reduces hand-rolled string-concatenation and formatting risk
• ➕ Potentially simpler future refactors (imports, indentation, comments)
• ➖ Adds/expands dependencies in the build
• ➖ Does not remove the hard parts (schema modeling: unions, nullability, reachability)
• ➖ May be harder to keep output stable if library formatting changes

3. Generate fewer files via runtime reflection / dynamic maps

• ➕ Less generated surface area; smaller compile impact
• ➕ Potentially simpler generator
• ➖ Loses strong typing, IDE support, and compile-time safety
• ➖ Harder to evolve safely as schema grows; more runtime errors

Recommendation: The PR’s build-time srcjar generation approach is appropriate for Selenium’s Bazel setup (and aligns with existing CDP/devtools precedent), given the very large Java type surface. Keep the generator-in-repo (not the output), but consider adding a lightweight CI job/target that regenerates and sanity-checks the srcjar contents (e.g., file count, package layout, compilation) to mitigate the reduced diff visibility without committing generated code.

Files changed (21) +2882 / -6

Enhancement (3) +1458 / -4

BiDiGenerator.javaAdd schema-driven Java BiDi protocol generator +1435/-0

Add schema-driven Java BiDi protocol generator

• Implements a CLI generator that reads the binding-neutral BiDi schema JSON, computes reachable and sender-only types, and emits Java module classes plus record/enum/union types into a srcjar. Handles multi-parent unions via interfaces, discriminated/structural union dispatch via fromMap(), nullable-vs-omitted outbound semantics for optional nullable fields, nested synthetic types as static inner classes, and reserved-word escaping.

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

ConverterFunctions.javaAdd Map→POJO deserializer helper for event payloads +20/-0

Add Map→POJO deserializer helper for event payloads

• Introduces ConverterFunctions.fromMap(Class<T>) to deserialize Map<String,Object> event payloads into typed POJOs using Selenium’s JSON coercion. This becomes the default mapping strategy for generated Event constants (with special handling for union types).

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

Module.javaExpose subscribe/unsubscribe publicly for generated modules +3/-4

Expose subscribe/unsubscribe publicly for generated modules

• Changes Module.subscribe(...) and Module.unsubscribe(...) from protected to public so generated protocol modules can expose event subscription APIs directly through the shared Module seam. Keeps command sending protected/final while making event lifecycle accessible to consumers.

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

Tests (16) +1375 / -0

BUILD.bazelAdd unit test suite for generated browsingcontext types +14/-0

Add unit test suite for generated browsingcontext types

• Creates a small JUnit5 test suite target that depends on bidi-generated and json/assertj to validate generated browsingcontext POJOs and serialization/deserialization semantics.

java/test/org/openqa/selenium/bidi/protocol/browsingcontext/BUILD.bazel

InfoTest.javaTest required+nullable vs optional field deserialization semantics +94/-0

Test required+nullable vs optional field deserialization semantics

• Validates that required nullable fields accept explicit null but still require key presence, while optional nullable fields deserialize to Optional.empty() whether null or absent. Exercises Json/ConstructorCoercer behavior expected by the generated POJOs.

java/test/org/openqa/selenium/bidi/protocol/browsingcontext/InfoTest.java

SetViewportParametersTest.javaTest explicit-null vs omitted-key behavior in generated toMap() +84/-0

Test explicit-null vs omitted-key behavior in generated toMap()

• Asserts that unset optional fields are omitted from the wire payload, while nullable optional fields support explicit clearing by emitting a key with null. Also confirms that non-nullable optional fields cannot represent an explicit-null wire state and thus remain omitted when set to null.

java/test/org/openqa/selenium/bidi/protocol/browsingcontext/SetViewportParametersTest.java

BUILD.bazelAdd unit test suite for generated emulation types +14/-0

Add unit test suite for generated emulation types

• Adds a small JUnit5 test suite for generated emulation protocol types, depending on bidi-generated and JSON tooling.

java/test/org/openqa/selenium/bidi/protocol/emulation/BUILD.bazel

SetTimezoneOverrideParametersTest.javaTest required-but-nullable field handling in construction and toMap() +50/-0

Test required-but-nullable field handling in construction and toMap()

• Ensures required nullable fields can be constructed with null and still serialize as an explicit key with null, not omitted. Confirms generator’s boxed/@Nullable behavior for required nullable types.

java/test/org/openqa/selenium/bidi/protocol/emulation/SetTimezoneOverrideParametersTest.java

SetTouchOverrideParametersTest.javaTest required nullable primitive boxing and JSON round-trip +62/-0

Test required nullable primitive boxing and JSON round-trip

• Verifies required nullable primitive fields are represented as boxed types, serialize null explicitly, and deserialize explicit null without throwing. Confirms real-value deserialization works as expected.

java/test/org/openqa/selenium/bidi/protocol/emulation/SetTouchOverrideParametersTest.java

BUILD.bazelAdd unit test suite for generated log types +14/-0

Add unit test suite for generated log types

• Creates a small JUnit5 suite target for generated log protocol classes, pulling in bidi-generated and JSON/assertj.

java/test/org/openqa/selenium/bidi/protocol/log/BUILD.bazel

EntryTest.javaTest union dispatch for log.Entry fromMap() +47/-0

Test union dispatch for log.Entry fromMap()

• Parses a representative console log entry payload into a Map and asserts Entry.fromMap dispatches to the correct generated subtype (ConsoleLogEntry). Exercises generated union deserialization paths.

java/test/org/openqa/selenium/bidi/protocol/log/EntryTest.java

BUILD.bazelAdd browser integration suite for generated module classes +27/-0

Add browser integration suite for generated module classes

• Defines a large java_selenium_test_suite that runs against BiDi-capable browsers and depends on bidi-generated plus Selenium remote/test infrastructure. Provides end-to-end coverage for generated command and event wiring through WebDriver/BiDi.

java/test/org/openqa/selenium/bidi/protocol/module/BUILD.bazel

BrowsingContextModuleTest.javaIntegration tests for generated BrowsingContext commands and events +205/-0

Integration tests for generated BrowsingContext commands and events

• Exercises generated browsing context module commands (create, navigate, getTree, close) and verifies event subscriptions (context created/destroyed, navigation events) function correctly. Uses CompletableFuture synchronization to validate actual browser-driven payloads.

java/test/org/openqa/selenium/bidi/protocol/module/BrowsingContextModuleTest.java

LogModuleTest.javaIntegration tests for generated Log module event subscriptions +170/-0

Integration tests for generated Log module event subscriptions

• Validates receipt and typing of console/javascript log entries, stack trace presence, multiple subscriptions, and unsubscribe behavior. Confirms module-level subscribe/unsubscribe lifecycle works against real browser traffic.

java/test/org/openqa/selenium/bidi/protocol/module/LogModuleTest.java

NetworkEventsTest.javaIntegration tests for generated Network event payloads +179/-0

Integration tests for generated Network event payloads

• Subscribes to multiple network events and validates key fields (context, request/response metadata, cookies) against real navigation. Marks known-not-working auth/error events as NotYetImplemented for specific browsers.

java/test/org/openqa/selenium/bidi/protocol/module/NetworkEventsTest.java

NetworkModuleTest.javaIntegration tests for generated Network commands and event lifecycle +72/-0

Integration tests for generated Network commands and event lifecycle

• Verifies a representative command round-trip (addIntercept/removeIntercept) and confirms subscribe→receive→unsubscribe works for a generated event. Separates interception logic from event lifecycle to keep tests deterministic.

java/test/org/openqa/selenium/bidi/protocol/module/NetworkModuleTest.java

ScriptModuleTest.javaIntegration tests for generated Script commands and events +228/-0

Integration tests for generated Script commands and events

• Covers callFunction/evaluate success and exception results, preload script add/remove, realm queries, channel message events, and realm lifecycle events with browser-specific NotYetImplemented annotations. Validates union result types (EvaluateResult) and typed RemoteValue variants in practice.

java/test/org/openqa/selenium/bidi/protocol/module/ScriptModuleTest.java

BUILD.bazelAdd unit test suite for generated network types +14/-0

Add unit test suite for generated network types

• Creates a small JUnit5 suite for generated network protocol classes, depending on bidi-generated and JSON/assertj.

java/test/org/openqa/selenium/bidi/protocol/network/BUILD.bazel

ContinueWithAuthParametersTest.javaTest discriminated union defaulting and serialization for ContinueWithAuthParameters +101/-0

Test discriminated union defaulting and serialization for ContinueWithAuthParameters

• Validates selector-based union dispatch to the correct nested variant type, including schema-defined default handling for unrecognized discriminator values. Also asserts nested enum wire-value round-tripping and variant toMap() serialization shape.

java/test/org/openqa/selenium/bidi/protocol/network/ContinueWithAuthParametersTest.java

Other (2) +49 / -2

BUILD.bazelWire up BiDi codegen binary + genrule-produced generated library +48/-2

Wire up BiDi codegen binary + genrule-produced generated library

• Adds a Bazel java_binary for BiDiGenerator and a genrule that runs it over the shared JS-produced schema to emit a bidi-generated.srcjar. Introduces a bidi-generated java_library that compiles the srcjar directly (no generated .java checked in) and updates exclusions so generator sources aren’t compiled into the main bidi library.

java/src/org/openqa/selenium/bidi/BUILD.bazel

BUILD.bazelAdd generated BiDi library as a test dependency +1/-0

Add generated BiDi library as a test dependency

• Extends the BiDi test suite deps to include the new //java/src/org/openqa/selenium/bidi:bidi-generated target so tests can compile against generated protocol classes.

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

[qodo-code-review]

Code Review by Qodo

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

1. Module.subscribe() missing Javadoc ✓ Resolved 📘 Rule violation ✧ Quality

Description

Module.subscribe(...) and Module.unsubscribe(...) are public methods but have no Javadoc
comments. This violates the requirement that public API methods include Javadoc documentation.

Code

java/src/org/openqa/selenium/bidi/Module.java[R48-57]

+  public final <X> String subscribe(Event<X> event, Consumer<X> handler) {
    return handle.subscribe(event, handler);
  }

-  protected final <X> String subscribe(
-      Event<X> event, Consumer<X> handler, SubscriptionScope scope) {
+  public final <X> String subscribe(Event<X> event, Consumer<X> handler, SubscriptionScope scope) {
    return handle.subscribe(event, handler, scope);
  }

-  protected final void unsubscribe(String subscriptionId) {
+  public final void unsubscribe(String subscriptionId) {
    handle.unsubscribe(subscriptionId);

Evidence

PR Compliance ID 330200 requires Javadoc for public API methods. In Module.java, the methods
subscribe(...) and unsubscribe(...) are public and have no preceding /** ... */ Javadoc
blocks.

Rule 330200: Require Javadoc for all public API types and methods
java/src/org/openqa/selenium/bidi/Module.java[48-57]

Agent prompt

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

## Issue description
`Module.subscribe(...)` and `Module.unsubscribe(...)` are public but lack required Javadoc.

## Issue Context
Compliance requires Javadoc for public API methods.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Module.java[48-57]

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

---

2. fromMap() Javadoc missing tags ✓ Resolved 📘 Rule violation ✧ Quality

Description

ConverterFunctions.fromMap(Class<T> type) has a Javadoc block but it lacks required @param and
@return tags. This violates the requirement for complete Javadoc on public methods.

Code

java/src/org/openqa/selenium/bidi/ConverterFunctions.java[R39-43]

+  /**
+   * Returns a function that deserializes a {@code Map<String, Object>} event payload into an
+   * instance of {@code type} via the Selenium JSON library (ConstructorCoercer).
+   */
+  public static <T> Function<Map<String, Object>, T> fromMap(Class<T> type) {

Evidence

PR Compliance ID 330201 requires public methods to have complete Javadoc, including @param for
each parameter and @return for non-void returns. fromMap(Class<T> type) has no @param or
@return tags in its Javadoc.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/bidi/ConverterFunctions.java[39-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
`ConverterFunctions.fromMap(...)` is public and has Javadoc, but the Javadoc is missing required tags (`@param type` and `@return`).

## Issue Context
Compliance requires complete Javadoc on public methods including parameter/return tags.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/ConverterFunctions.java[39-52]

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

---

3. Reserved-key deserialization breaks ✓ Resolved 🐞 Bug ≡ Correctness

Description

BiDiGenerator escapes reserved JSON field names (e.g., wire key "this" becomes Java identifier
"this_"), but record deserialization uses ConstructorCoercer which matches JSON keys to constructor
parameter names. This causes required fields to fail deserialization (missing key) or optional
fields to be silently ignored whenever wire name != Java parameter name.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R1266-1276]

+    private FieldInfo parseField(Map<String, Object> raw) {
+      String rawName = str(raw, "name");
+      String wire = str(raw, "wire");
+      // Escape Java reserved words used as field names in the spec (e.g. "this").
+      String name = escapeReserved(rawName);
+      boolean required = Boolean.TRUE.equals(raw.get("required"));
+      @SuppressWarnings("unchecked")
+      Map<String, Object> type = (Map<String, Object>) raw.get("type");
+      // wire key stays as the original spec name for JSON serialization
+      return new FieldInfo(name, wire != null ? wire : rawName, required, type);
+    }

Evidence

The generator explicitly changes the Java name while preserving the wire key, but ConstructorCoercer
requires constructor parameter names to match JSON keys; protocol payloads include a reserved key
("this"), demonstrating this mismatch is reachable.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1266-1276]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1108-1113]
java/src/org/openqa/selenium/json/ConstructorCoercer.java[191-207]
java/src/org/openqa/selenium/json/Json.java[46-51]
javascript/selenium-webdriver/bidi/scriptManager.js[335-369]

Agent prompt

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

## Issue description
Generated record classes escape reserved words in Java identifiers (e.g. `this` -> `this_`) while keeping the JSON wire key as the original (`"this"`). Selenium JSON deserialization via `ConstructorCoercer` requires JSON property keys to match constructor parameter names, so any field where `wire != name` will not deserialize correctly.

## Issue Context
- `ConstructorCoercer` uses `Parameter.getName()` and checks `properties.containsKey(parameter.getName())`, meaning it cannot map JSON key `"this"` to a parameter named `this_`.
- The BiDi protocol payloads do include a `"this"` key (e.g. Script callFunction params).

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1266-1276]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1108-1113]

## Suggested fix
In the generator, for any **record** type that has at least one field where `f.wire` differs from `f.name` (currently happens via `escapeReserved`), emit a `static fromJson(Map<String,Object>)` factory for that record.
- The method name must be exactly `fromJson` (so `StaticInitializerCoercer` picks it up).
- Inside `fromJson`, read values by **wire key** (`f.wire`), coerce each value to the correct Java type (recursing into nested records/unions/enums/lists/maps as needed), and invoke the all-fields constructor.
- Do **not** call `ConverterFunctions.fromMap(<ThisClass>.class)` inside this method (would recurse back into `fromJson`).
- Keep `toMap()` using wire keys as-is.

This preserves the escaped Java identifiers for API ergonomics while keeping JSON deserialization correct.

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

---

4. Map fields serialize wrong ✓ Resolved 🐞 Bug ≡ Correctness

Description

serializeExpr handles primitives/refs/lists but not map type refs, so toMap() will emit
Map<String,T> values without converting nested record/union/enum values to their wire form. Any
sender-side schema field with map values of complex types will generate malformed command payloads.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R1164-1199]

+    private String serializeExpr(String varName, Map<String, Object> typeRef, String domain) {
+      if (typeRef == null) return varName;
+      if (typeRef.containsKey("primitive") || typeRef.containsKey("const")) return varName;
+      if (typeRef.containsKey("ref")) {
+        String resolvedKind = resolvedKindOf(str(typeRef, "ref"));
+        if ("enum".equals(resolvedKind)) return varName + ".toString()";
+        if ("record".equals(resolvedKind) || "union".equals(resolvedKind)) {
+          return varName + ".toMap()";
+        }
+        // alias: recurse through
+        Map<String, Object> aliasNode = types.get(str(typeRef, "ref"));
+        if (aliasNode != null && "alias".equals(str(aliasNode, "kind"))) {
+          @SuppressWarnings("unchecked")
+          Map<String, Object> inner = (Map<String, Object>) aliasNode.get("type");
+          return serializeExpr(varName, inner, domain);
+        }
+        return varName;
+      }
+      if (typeRef.containsKey("list")) {
+        @SuppressWarnings("unchecked")
+        Map<String, Object> elem = (Map<String, Object>) typeRef.get("list");
+        String elemKind = resolvedKindFromTypeRef(elem);
+        if ("enum".equals(elemKind)) {
+          return varName
+              + ".stream().map(Object::toString)"
+              + ".collect(java.util.stream.Collectors.toList())";
+        }
+        if ("record".equals(elemKind) || "union".equals(elemKind)) {
+          return varName
+              + ".stream().map(e -> e.toMap())"
+              + ".collect(java.util.stream.Collectors.toList())";
+        }
+        return varName;
+      }
+      return varName;
+    }

Evidence

The generator can resolve map type refs into Java Map types, and the schema vocabulary includes map
refs, but serializeExpr has no corresponding logic to convert map values to wire-compatible forms.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1164-1199]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1053-1057]
javascript/selenium-webdriver/project_bidi_schema.mjs[24-35]

Agent prompt

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

## Issue description
`serializeExpr` is used by generated `toMap()` methods to convert nested values into wire-compatible shapes. It supports primitives/const, refs (enum/record/union), and lists, but it does not handle `map` typeRefs.
As a result, fields typed as `Map<String, SomeRecord>` / `Map<String, SomeUnion>` / `Map<String, SomeEnum>` will be put on the wire as Java objects rather than `Map<String,Object>` / strings, leading to malformed payloads.

## Issue Context
The shared schema vocabulary explicitly includes `map` type refs, and the generator already resolves them to `java.util.Map<String, ...>` types.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1164-1199]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1053-1057]

## Suggested fix
Extend `serializeExpr` with a `typeRef.containsKey("map")` branch:
- Determine the resolved kind of the map value type.
- For enum values: map `v -> v.toString()`.
- For record/union values: map `v -> v.toMap()`.
- For nested list/map values: recurse similarly.
- Preserve key strings unchanged.

Example emitted expression shape (use LinkedHashMap to preserve order):
`varName.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> <serializedValueExpr>, (a,b)->b, LinkedHashMap::new))`

This makes `toMap()` correct for map-typed schema fields.

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

---

5. Srcjar not reproducible ✓ Resolved 🐞 Bug ☼ Reliability

Description

packToJar writes bidi-generated.srcjar using Files.walkFileTree traversal order and default JarEntry
metadata, which can vary across filesystems/runs. This makes the generated srcjar non-deterministic,
reducing Bazel cache hits and harming reproducible builds.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R1365-1396]

+  private static void packToJar(Path tempDir, Path outputJar) throws IOException {
+    Files.createDirectories(outputJar.getParent());
+    try (OutputStream os = Files.newOutputStream(outputJar);
+        JarOutputStream jos = new JarOutputStream(os)) {
+      Files.walkFileTree(
+          tempDir,
+          new SimpleFileVisitor<Path>() {
+            @Override
+            public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs)
+                throws IOException {
+              String rel = tempDir.relativize(dir).toString().replace('\\', '/');
+              if (!rel.isEmpty()) {
+                JarEntry e = new JarEntry(rel + "/");
+                jos.putNextEntry(e);
+                jos.closeEntry();
+              }
+              return FileVisitResult.CONTINUE;
+            }
+
+            @Override
+            public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
+                throws IOException {
+              String rel = tempDir.relativize(file).toString().replace('\\', '/');
+              JarEntry e = new JarEntry(rel);
+              jos.putNextEntry(e);
+              try (InputStream is = Files.newInputStream(file)) {
+                is.transferTo(jos);
+              }
+              jos.closeEntry();
+              return FileVisitResult.CONTINUE;
+            }
+          });

Evidence

The generator currently walks the filesystem and emits JarEntry objects without ordering or
timestamp normalization, and the Bazel genrule uses this jar as the build output artifact.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1365-1396]
java/src/org/openqa/selenium/bidi/BUILD.bazel[59-65]

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 generator packs generated sources into a srcjar in a potentially non-deterministic way:
- `Files.walkFileTree` does not guarantee a stable traversal order.
- `JarEntry` is created without normalizing timestamps/metadata.
This can cause `bidi-generated.srcjar` to differ between builds even when input schema is unchanged.

## Issue Context
The srcjar is produced by a Bazel `genrule` and consumed directly as compilation input, so nondeterminism impacts caching and reproducibility.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1365-1396]
- java/src/org/openqa/selenium/bidi/BUILD.bazel[59-65]

## Suggested fix
Update `packToJar` to:
1. Collect all files under `tempDir` with `Files.walk(tempDir)`.
2. Sort paths by their normalized jar entry name (`tempDir.relativize(p).toString().replace('\\','/')`).
3. For each entry, set deterministic metadata (at minimum `entry.setTime(0L)`; optionally also set method/extra fields deterministically).
4. Write entries in sorted order.

This yields stable srcjar bytes for the same generated content.

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

---

Context sources

Review mode: ⚖️ Balanced: This localized runtime change alters numeric-error handling for untrusted JSON input, so it carries real correctness and robustness risk despite the small patch.

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[titusfortner]

I started reviewing this and ended up creating #17786
It's slightly different from what I did and Ruby and Python as well so I need to go back and fix those if we agree to each of these

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (3) 📜 Skill insights (0)

1. Default timeout bypassed 🐞 Bug ☼ Reliability

Description

Broker.ExecuteAsync no longer enforces DefaultCommandTimeout whenever the caller supplies a
cancellable CancellationToken (it sets cts to null), so commands can wait without the broker’s
documented default timeout. This contradicts the file’s stated intent to cancel commands after a
default response-wait duration.

Code

dotnet/src/webdriver/BiDi/Broker.cs[R81-84]

+        using CancellationTokenSource? cts = cancellationToken.CanBeCanceled
+            ? null
+            : new CancellationTokenSource(DefaultCommandTimeout);

-        var timeout = options?.Timeout ?? DefaultCommandTimeout;
-        cts.CancelAfter(timeout);

Evidence

The class defines and documents a default response timeout, but the new logic skips creating any
timeout CTS when cancellationToken.CanBeCanceled is true, meaning the broker’s default
cancel-after behavior no longer applies in that common case.

dotnet/src/webdriver/BiDi/Broker.cs[31-36]
dotnet/src/webdriver/BiDi/Broker.cs[81-86]

Agent prompt

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

### Issue description
`Broker.ExecuteAsync` currently creates a timeout `CancellationTokenSource` only when the passed `cancellationToken` cannot be canceled. If the caller passes any cancellable token (even one with no deadline), the broker stops enforcing `DefaultCommandTimeout`, despite the class comment indicating it should.

### Issue Context
`DefaultCommandTimeout` is documented as the response wait time before canceling a command.

### Fix
Create a linked CTS in all cases (or at least whenever `cancellationToken` is cancellable) and call `CancelAfter(DefaultCommandTimeout)` so the effective token always has the broker timeout *and* respects caller cancellation.

Example approach:
- `using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);`
- `cts.CancelAfter(DefaultCommandTimeout);`
- `var effectiveToken = cts.Token;`

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[34-36]
- dotnet/src/webdriver/BiDi/Broker.cs[81-86]
- dotnet/src/webdriver/BiDi/Broker.cs[134-137]
- dotnet/src/webdriver/BiDi/Broker.cs[151-151]

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

---

2. Percent double-escaped 🐞 Bug ≡ Correctness

Description

By.name(...) now replaces a literal % with %% while building the CSS selector, so elements
whose name contains % will never match. This contradicts the newly added ByTest expectations
and will cause that test to fail.

Code

java/src/org/openqa/selenium/By.java[245]

+          "*[name='" + name.replace("\\", "\\\\").replace("'", "\\'").replace("%", "%%") + "']");

Evidence

The selector builder explicitly transforms % into %%, while the new unit test asserts the
serialized selector must preserve % literally; these two are incompatible and the lookup will be
wrong for percent-containing names.

java/src/org/openqa/selenium/By.java[241-246]
java/test/org/openqa/selenium/ByTest.java[152-162]

Agent prompt

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

### Issue description
`By.ByName` builds a CSS selector string via concatenation, but it still applies `.replace("%", "%%")`. With concatenation there is no later formatting pass, so this emits the wrong selector and breaks lookups for names containing `%`.

### Issue Context
The added unit test expects `By.name("50%off")` to serialize to `*[name='50%off']`.

### Fix
Remove the `.replace("%", "%%")` call from selector construction (keep the backslash and single-quote escaping).

### Fix Focus Areas
- java/src/org/openqa/selenium/By.java[241-246]
- java/test/org/openqa/selenium/ByTest.java[152-162]

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

---

3. Void events never dispatched 🐞 Bug ≡ Correctness

Description

BiDiGenerator emits Event<Void> constants with a mapper that always returns null for events without
params, but Connection.handleEventResponse returns early when the mapper result is null. This
prevents any parameterless BiDi events from ever invoking subscribed handlers.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R532-538]

+        } else {
+          sb.append("  public static final Event<Void> ")
+              .append(evtConstant)
+              .append(" =\n")
+              .append("      new Event<>(\"")
+              .append(method)
+              .append("\", map -> null);\n\n");

Evidence

The generator explicitly creates Event<Void> with a mapper that returns null for no-params events,
and the runtime event dispatch loop returns early on null mapped values, so such events cannot reach
callbacks.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[512-539]
java/src/org/openqa/selenium/bidi/Connection.java[347-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
Parameterless BiDi events are generated as `Event<Void>` with a mapper `map -> null`. The current event dispatch loop treats a `null` mapped value as a signal to skip dispatch entirely, so handlers for such events never fire.

### Issue Context
- Generator emits `new Event<>(..., map -> null)` when an event has no `params` schema.
- Runtime dispatch (`Connection.handleEventResponse`) currently does `if (value == null) return;` before invoking callbacks.

### Fix
Update event dispatch to still invoke callbacks even when the mapped value is `null` (this is required for `Event<Void>`). The simplest fix is to remove the `if (value == null) return;` guard, since `Consumer.accept(null)` is valid and preserves delivery semantics for void events.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Connection.java[375-395]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[519-539]

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

---

View high (1)

4. Builder loses explicit null ✓ Resolved 🐞 Bug ≡ Correctness

Description

For immutable bidirectional record types, the generated nested Builder cannot represent “explicitly
set to null” for nullable optional fields, so toMap() will omit the key instead of sending an
explicit null. This breaks commands where nullable optional fields use explicit null as a distinct
wire state from omission (the generator explicitly documents this distinction via the <field>Set
flag).

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R905-912]

+      sb.append(bm).append("public ").append(cls).append(" build() {\n");
+      sb.append(bm).append("  return new ").append(cls).append("(");
+      sb.append(
+          fields.stream()
+              .map(f -> f.required ? f.name : "Optional.ofNullable(" + f.name + ")")
+              .collect(Collectors.joining(", ")));
+      sb.append(");\n");
+      sb.append(bm).append("}\n");

Evidence

The generator explicitly relies on a separate <field>Set boolean to distinguish “unset” from
“explicit null” for nullable optional fields, but the Builder path never sets that boolean when a
caller passes null, so toMap() will omit the field rather than emit an explicit null.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[679-705]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[799-815]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[905-912]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[982-998]

Agent prompt

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

### Issue description
Generated immutable record Builders lose the “explicitly set to null” signal for nullable optional fields. The outer record uses a boolean `<field>Set` to decide whether to omit the key or send an explicit `null`, but Builder.build() always passes `Optional.ofNullable(field)` and the constructor derives `<field>Set` solely from `Optional.isPresent()`, making explicit-null indistinguishable from “never set”.

### Issue Context
This only affects records that are both receivable and sendable (immutable + has Builder) and have optional fields whose schema type is nullable.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[679-836]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[845-914]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[960-999]

### Suggested fix approach
- In appendBuilder, for each optional field where `isNullable(f.typeRef)` is true, generate an additional boolean flag in the Builder (e.g., `private boolean <name>Set;`).
- In the Builder setter for that field, set `<name>Set = true` regardless of whether the value is null.
- Update the generated outer-class constructor for immutable types so that, for nullable optional fields, it can accept and store the explicit-set flag (either as an extra constructor parameter or by generating a dedicated private constructor used only by Builder).
- Ensure toMap() for immutable types uses the propagated flag so `setX(null)` results in a payload containing `"x": null`.

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

---

5. Builder copy lacks regression test ✓ Resolved 📘 Rule violation ▣ Testability ⭐ New

Description

The new defensive copy fixes mutation through a reused Builder, but no test reuses a Builder and
verifies that an already-built instance remains unchanged. Existing extension tests would still pass
if this fix were reverted.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[828]

+            sb.append(m).append("  this.extensions = new LinkedHashMap<>(extensions);\n");

Evidence

Rule 389273 requires a test assertion that fails against the pre-fix implementation. The generator
now copies extensions, while the existing Builder test only verifies the initial extension and
does not mutate the Builder after build(), so it does not detect the aliasing regression.

Rule 389273: Require tests for all new functionality and bug fixes
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[823-828]
java/test/org/openqa/selenium/bidi/protocol/script/SharedReferenceTest.java[37-45]

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 defensive copy of the Builder's extensions map lacks an automated regression test that would fail if the copy were removed.

## Issue Context
Reuse a `SharedReference.Builder`: build one instance, add another extension to the same Builder, then assert that the first instance's extensions and wire map remain unchanged.

## Fix Focus Areas
- java/test/org/openqa/selenium/bidi/protocol/script/SharedReferenceTest.java[37-45]

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

---

6. Builder shares extensions map ✓ Resolved 🐞 Bug ≡ Correctness

Description

BiDiGenerator emits Builders for extensible immutable records that pass the Builder’s mutable
extensions map directly into the constructed instance. Reusing the Builder after build() can
mutate previously-built instances’ getExtensions()/toMap() output, violating the intended
immutability semantics.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R1139-1141]

+      if (needsExtras) {
+        buildArgs.add("extensions");
+      }

Evidence

The generated Builder owns a mutable extensions map and mutates it via addExtension(...).
build() then passes this same map reference into the generated record constructor; the constructor
assigns it directly to the instance field, and getExtensions() only returns an unmodifiable view
of that same backing map, so subsequent Builder mutations are still observable through already-built
instances.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1084-1087]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1118-1127]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1139-1141]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[814-825]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[882-886]

Agent prompt

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

### Issue description
For extensible + immutable records, `appendBuilder` generates a `Builder` that stores extensions in a mutable `LinkedHashMap`. `build()` passes that same map reference into the record constructor, and the constructor assigns it directly to the instance field. If the Builder is reused after `build()`, the previously built object’s extensions change unexpectedly.

### Issue Context
This affects any **bidirectional + extensible** generated record (e.g., records like `script.SharedReference` in your tests) where the generator emits both an immutable value class and a nested `Builder`.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1084-1087]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1118-1127]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1139-1141]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[814-825]

### Suggested fix
Ensure the built instance does not share the Builder’s backing map, e.g.:
- In generated `build()`, pass `new LinkedHashMap<>(extensions)` instead of `extensions`, **or**
- In the generated record constructor path that accepts `extensions`, store `new LinkedHashMap<>(extensions)` (optionally wrapping with `Collections.unmodifiableMap(...)` if you want hard immutability internally).

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

---

7. subscribe(..., scope) lacks Javadoc ⊘ Outdated 📘 Rule violation ✧ Quality

Description

A new public subscribe overload (Module.subscribe(Event, Consumer, SubscriptionScope)) was added
without a Javadoc block, making it inconsistent with the already-documented `subscribe(Event,
Consumer)` method and reducing public API usability for consumers.

Code

java/src/org/openqa/selenium/bidi/Module.java[R60-62]

+  public final <X> String subscribe(Event<X> event, Consumer<X> handler, SubscriptionScope scope) {
+    return handle.subscribe(event, handler, scope);
+  }

Evidence

PR Compliance ID 330201 requires complete Javadoc on changed/new public methods, and in
Module.java the existing subscribe overload includes a full /** ... */ Javadoc block, followed
immediately by the newly added subscribe(Event<X>, Consumer<X>, SubscriptionScope) method which
has no preceding Javadoc, leaving the new public API surface undocumented.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/bidi/Module.java[60-62]
java/src/org/openqa/selenium/bidi/Module.java[48-62]

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 new public method overload `subscribe(Event<X>, Consumer<X>, SubscriptionScope)` was added without Javadoc. Public API methods must include a complete Javadoc block (including `@param` and `@return` tags) to satisfy documentation requirements and keep the API consistent.

## Issue Context
`Module.subscribe(Event, Consumer)` is already documented with a full Javadoc block, but the newly added overload that includes `SubscriptionScope` has no Javadoc at all. This also conflicts with PR Compliance ID 330201, which requires complete Javadoc on changed/new public methods.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Module.java[48-62]

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

---

View medium (8)

8. Missing reason in skip_unless 📘 Rule violation ▣ Testability

Description

New/modified integration-spec guards use skip_unless without providing an explicit reason,
making it harder to audit and diagnose why specs are skipped. Add a non-empty reason (string or
issue id per project convention) to each guard condition that currently omits it.

Code

rb/spec/integration/selenium/webdriver/devtools_spec.rb[R24-25]

    describe DevTools, skip_unless: [{bidi: false, reason: 'Not yet implemented with BiDi'},
-                                     {browser: %i[chrome edge]}] do
+                                     {browser_family: :chromium}] do

Evidence

PR Compliance ID 922483 requires an explicit reason on integration test guards when supported. The
updated specs include skip_unless guard entries like {browser_family: :chromium} / `{browser:
:safari} without reason:`.

Rule 922483: Require explicit reason in integration test guards
rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[70-72]
rb/spec/integration/selenium/webdriver/fedcm_spec.rb[25-26]
rb/spec/integration/selenium/webdriver/virtual_authenticator_spec.rb[24-25]

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 RSpec integration test guards were changed to include conditions without an explicit `reason`, which violates the requirement to always include a reason on integration test guards.

## Issue Context
These specs already pass `reason:` for some guard conditions (e.g., `bidi: false`), so the guard mechanism clearly supports it; consistency and auditability require all guard entries to include a reason.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[70-72]
- rb/spec/integration/selenium/webdriver/fedcm_spec.rb[25-26]
- rb/spec/integration/selenium/webdriver/virtual_authenticator_spec.rb[24-25]

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

---

9. New file under third_party/ 📘 Rule violation § Compliance

Description

The PR adds third_party/bazel/rules_ruby_windows_batch_crlf.patch, which violates the policy
forbidding direct changes under third_party/. This increases maintenance and supply-chain risk
because third-party content should be updated only via the approved vendoring mechanism.

Code

third_party/bazel/rules_ruby_windows_batch_crlf.patch[R1-4]

+diff --git a/ruby/private/utils.bzl b/ruby/private/utils.bzl
+index bfc02aba..f10b2abf 100644
+--- a/ruby/private/utils.bzl
++++ b/ruby/private/utils.bzl

Evidence

PR Compliance ID 389269 forbids adding/modifying files under third_party/. The diff shows a new
file being introduced at third_party/bazel/rules_ruby_windows_batch_crlf.patch.

Rule 389269: Disallow modifications to third_party directory
third_party/bazel/rules_ruby_windows_batch_crlf.patch[1-4]

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 new file was added under `third_party/`, which is disallowed by the compliance checklist.

## Issue Context
Third-party changes should be performed via the repo’s vendoring/update tooling (or moved out of `third_party/` into a project-owned location), rather than committing direct edits/additions under `third_party/`.

## Fix Focus Areas
- third_party/bazel/rules_ruby_windows_batch_crlf.patch[1-39]

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

---

10. Unquoted genrule arguments ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new :generate-bidi genrule builds a shell command without quoting the tool path, schema path, or
output path, so the action can fail when any of those paths contain spaces or shell-special
characters. This makes BiDi generation brittle and can break Bazel builds depending on
workspace/execroot location.

Code

java/src/org/openqa/selenium/bidi/BUILD.bazel[R59-65]

+genrule(
+    name = "generate-bidi",
+    srcs = ["//javascript/selenium-webdriver:create-bidi-src_schema"],
+    outs = ["bidi-generated.srcjar"],
+    cmd = "$(execpath :bidi-client-generator) $(location //javascript/selenium-webdriver:create-bidi-src_schema) $@",
+    tools = [":bidi-client-generator"],
+)

Evidence

The genrule currently constructs an unquoted shell command string, and the repo’s JS BiDi generation
macro explicitly calls out that avoiding shell command strings prevents quoting/escaping
issues—highlighting this as a known concern in-repo.

java/src/org/openqa/selenium/bidi/BUILD.bazel[59-65]
javascript/selenium-webdriver/private/generate_bidi.bzl[150-154]

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 `genrule(name = "generate-bidi")` command concatenates `$(execpath ...)`, `$(location ...)`, and `$@` into a shell command without quoting. If any expanded path contains whitespace or shell-special characters, the generator will receive split/incorrect arguments and the build will fail.

### Issue Context
This genrule is new in this PR and is used to generate `bidi-generated.srcjar` during the build.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BUILD.bazel[59-65]

### Suggested fix
Update `cmd` to quote each argument as a single shell token, e.g.:

```bzl
cmd = "\"$(execpath :bidi-client-generator)\" \"$(location //javascript/selenium-webdriver:create-bidi-src_schema)\" \"$@\"",
```

(or equivalent quoting per the repo’s Bazel shell conventions), so paths with spaces are handled correctly.

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

---

11. Numeric discriminator misdispatch ✓ Resolved 🐞 Bug ≡ Correctness

Description

BiDiGenerator emits discriminated-union comparisons as string equality for any non-null, non-boolean
selector value, so numeric discriminator literals will never match the Long/Double values produced
by JSON parsing. This causes union fromMap() to fall through to the default variant (if present) or
throw BiDiException despite a valid payload.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R1253-1268]

+            Object value = sv.get("value");
+            String variantRef = str(sv, "ref");
+            String variantClass = resolveRefToJavaClass(variantRef, domain);
+            // A discriminator value isn't always a string (e.g.
+            // bluetooth.HandleRequestDevicePromptParameters dispatches on a boolean "accept"
+            // field) — the deserialized map value is a real Boolean there, so comparing it
+            // against a quoted string literal via String#equals would never match. Emit a literal
+            // of the value's own type instead of always quoting it.
+            String test;
+            if (value == null) {
+              test = "discriminator == null";
+            } else if (value instanceof Boolean) {
+              test = "Objects.equals(discriminator, " + value + ")";
+            } else {
+              test = "\"" + value + "\".equals(discriminator)";
+            }

Evidence

The generator currently treats all non-Boolean discriminator literals as strings, but the schema
projector can create const values from literals, and Selenium JSON parsing produces numeric objects
for JSON numbers—so numeric discriminators can’t match the generated string comparison.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1242-1270]
javascript/selenium-webdriver/project_bidi_schema.mjs[105-113]
java/src/org/openqa/selenium/json/JsonInput.java[227-298]

Agent prompt

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

### Issue description
`BiDiGenerator` generates discriminated-union dispatch code that only special-cases `null` and `Boolean` discriminator values. For any other discriminator literal, it generates a quoted string comparison (`"<value>".equals(discriminator)`), which fails for numeric discriminators because Selenium’s JSON parsing materializes numbers as `Long`/`Double`.

### Issue Context
- Schema projection can emit selector variant `value` from CDDL literals (`{ const: e.Value }`), which is not constrained to strings.
- Selenium JSON parsing reads integer numbers as `Long` and decimals as `Double`.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1253-1268]

### Suggested fix
Add an explicit `value instanceof Number` branch when computing `test`, emitting a typed Java numeric literal that matches the runtime discriminator type:
- If `value` is a `Long`, generate `Objects.equals(discriminator, 123L)`.
- If `value` is a `Double`, generate `Objects.equals(discriminator, 1.23)` (or `1.23d`).
This keeps dispatch behavior correct for schemas that use numeric discriminator tags.

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

---

12. Event payload double-parsed ✓ Resolved 🐞 Bug ➹ Performance

Description

ConverterFunctions.fromMap() re-serializes an event params Map back into JSON and reparses it to
build the typed payload object, adding avoidable CPU/allocation overhead for every event. Connection
already parses each websocket message into a Map before invoking the event mapper, so event handling
currently does extra encode/decode work per event.

Code

java/src/org/openqa/selenium/bidi/ConverterFunctions.java[R47-55]

+  public static <T> Function<Map<String, Object>, T> fromMap(Class<T> type) {
+    Require.nonNull("Type", type);
+    return map -> {
+      String json = JSON.toJson(map);
+      try (StringReader reader = new StringReader(json);
+          JsonInput input = JSON.newInput(reader)) {
+        return input.readNonNull(type);
+      }
+    };

Evidence

Connection already parses the incoming frame into a Map and passes the params map to the event
mapper; ConverterFunctions.fromMap then serializes that map back to JSON and reparses it, creating
redundant encode/decode work.

java/src/org/openqa/selenium/bidi/Connection.java[299-313]
java/src/org/openqa/selenium/bidi/Connection.java[374-379]
java/src/org/openqa/selenium/bidi/ConverterFunctions.java[47-55]

Agent prompt

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

### Issue description
Event handling currently does redundant work:
1) `Connection.handle(...)` parses the websocket frame into a `Map<String,Object>`.
2) The event mapper produced by `ConverterFunctions.fromMap(...)` converts that map back to a JSON string and reparses it to construct the typed object.

This adds extra CPU and allocations on event-heavy paths.

### Issue Context
`Event` mappers are `Function<Map<String,Object>, X>`, so the current approach uses a JSON round-trip to reuse ConstructorCoercer-based deserialization.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/ConverterFunctions.java[47-55]
- java/src/org/openqa/selenium/bidi/Connection.java[299-313]
- java/src/org/openqa/selenium/bidi/Connection.java[374-379]

### Suggested fix
Prefer a single-parse event path by introducing a `JsonInput`-based event-mapping option:
- Add a new mapper shape (e.g., `Function<JsonInput, X>`) for `Event` (or add a parallel `Event2`/overload), similar to `Command` mapping.
- Update `Connection.handleEventResponse` to parse the *raw websocket message* once with `JsonInput`, position the reader at `params`, and invoke the `JsonInput` mapper.
- Update `BiDiGenerator` to generate event constants using the `JsonInput` mapper where possible.

This eliminates the per-event map→JSON→parse conversion while keeping deserialization behavior consistent.

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

---

13. Locale-dependent identifier casing ✓ Resolved 🐞 Bug ☼ Reliability

Description

BiDiGenerator derives Java identifiers using String#toUpperCase()/toLowerCase() without Locale.ROOT,
so builds under locales like Turkish can generate different enum/event constant names and fail
compilation for code expecting ASCII identifiers (e.g., Level.INFO). Use locale-stable casing to
make generated sources reproducible across environments.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R1659-1685]

+  static String domainPackage(String domain) {
+    return BASE_PKG + ".protocol." + domain.toLowerCase();
+  }
+
+  static String domainOf(String typeName) {
+    int dot = typeName.indexOf('.');
+    return dot >= 0 ? typeName.substring(0, dot) : typeName;
+  }
+
+  static String simpleNameOf(String typeName) {
+    int dot = typeName.indexOf('.');
+    return dot >= 0 ? typeName.substring(dot + 1) : typeName;
+  }
+
+  static String capitalize(String s) {
+    if (s == null || s.isEmpty()) return s;
+    return Character.toUpperCase(s.charAt(0)) + s.substring(1);
+  }
+
+  static String toConstantName(String camel) {
+    // Insert underscore before each uppercase letter, then upper-case the whole string
+    return camel.replaceAll("([A-Z])", "_$1").toUpperCase();
+  }
+
+  static String toEnumConstant(String wireValue) {
+    return wireValue.toUpperCase().replace('-', '_').replace('.', '_').replace(' ', '_');
+  }

Evidence

The generator currently uses locale-sensitive case conversions for generated identifiers, and the
test suite already references ASCII enum constants like Level.INFO; under Turkish locale the
generator could emit a different identifier (e.g., İNFO), causing compilation failures.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1659-1685]
java/test/org/openqa/selenium/bidi/protocol/module/LogModuleTest.java[56-65]

Agent prompt

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

### Issue description
`BiDiGenerator` uses locale-sensitive case conversions when generating Java identifiers (package names, event constants, enum constants). In non-English locales (notably Turkish), `toUpperCase()`/`toLowerCase()` can produce different characters (e.g., `i -> İ`), causing generated identifiers to differ and potentially breaking compilation of downstream code that references expected ASCII names.

### Issue Context
This affects `domainPackage`, `toConstantName`, and `toEnumConstant`, which all currently call `toLowerCase()`/`toUpperCase()` without specifying a locale.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1659-1685]

### Suggested fix
- Add `import java.util.Locale;`
- Change:
 - `domain.toLowerCase()` -> `domain.toLowerCase(Locale.ROOT)`
 - `.toUpperCase()` -> `.toUpperCase(Locale.ROOT)`
 - `wireValue.toUpperCase()` -> `wireValue.toUpperCase(Locale.ROOT)`

(Optionally) add a small unit test that temporarily sets the default locale to Turkish and asserts `toEnumConstant("info")` returns `INFO`.

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

---

14. BiDiGenerator.main() missing Javadoc 📘 Rule violation ✧ Quality

Description

BiDiGenerator.main(String[] args) is a new public method without any Javadoc. This violates the
requirement that changed public methods have complete Javadoc.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R136-140]

+  public static void main(String[] args) throws IOException {
+    if (args.length != 2) {
+      System.err.println("Usage: BiDiGenerator <schema.json> <output.srcjar>");
+      System.exit(1);
+    }

Evidence

PR Compliance ID 330201 requires a Javadoc block immediately above each changed public method.
BiDiGenerator.main(...) is public and appears without any preceding Javadoc.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[136-140]

Agent prompt

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

## Issue description
`BiDiGenerator.main(...)` is `public` but has no Javadoc.

## Issue Context
PR Compliance requires a Javadoc block for each changed public method, including tags for parameters/returns/throws as applicable.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[136-140]

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

---

15. Container results typed wrong ✓ Resolved 🐞 Bug ≡ Correctness

Description

For command results that are top-level list/map types, the generated module method return type is
parameterized (e.g., List<T>/Map<String,T>) but resolveCommandResultArg forces deserialization with
Object.class. This loses element/value coercion and yields raw Map/List structures, so callers can
get runtime type mismatches when they use the declared generic types.

Code

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[R1444-1449]

+    private String resolveCommandResultArg(Map<String, Object> resultRef, String contextDomain) {
+      if (resultRef == null) return null;
+      if (resultRef.containsKey("list") || resultRef.containsKey("map")) {
+        // Generic container types: use Object.class and rely on caller to cast
+        return "Object.class";
+      }

Evidence

resolveJavaType generates parameterized java.util.List<...>/java.util.Map<String,...> types, but
resolveCommandResultArg explicitly returns Object.class for list/map results, which feeds into
Command’s readNonNull(typeOfX) mapper and prevents typed coercion for the declared generic return
type.

java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1237-1264]
java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1444-1449]
java/src/org/openqa/selenium/bidi/Command.java[43-46]

Agent prompt

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

### Issue description
BiDiGenerator emits parameterized Java return types for `list` and `map` result schema refs, but `resolveCommandResultArg` returns `Object.class` for those shapes. That causes `Command` to deserialize using `JsonInput.readNonNull(Object.class)` which cannot preserve/carry generic element/value types, so the generated API promises `List<Foo>` but returns a raw `List` of uncoerced values.

### Issue Context
This affects any BiDi command whose `result` is directly a container (not wrapped in a record result type).

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1237-1264]
- java/src/org/openqa/selenium/bidi/BiDiGenerator.java[1444-1472]

### Suggested fix approach
Option A (preferred): generate a proper reflective `Type` for container results:
- For a list result, emit `new org.openqa.selenium.json.TypeToken<java.util.List<Elem>>() {}.getType()`.
- For a map result, emit `new org.openqa.selenium.json.TypeToken<java.util.Map<String, Val>>() {}.getType()`.
- Pass that `Type` into the `Command` constructor instead of `Object.class`.

Option B: if full generic coercion is not supported/desired, downgrade the generated method return type for top-level container results to `Object` (or `List<Object>` / `Map<String,Object>`) to match the actual deserialization behavior and avoid a misleading API contract.

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

---

16. Pinned IntelliJ JDK name 🐞 Bug ⚙ Maintainability

Description

The checked-in IntelliJ config sets project-jdk-name="25" while keeping languageLevel="JDK_11",
which requires contributors to have an IntelliJ SDK entry named "25" or manually adjust project
settings. This reduces portability of the committed IDE configuration.

Code

.idea/misc.xml[7]

+  <component name="ProjectRootManager" version="2" languageLevel="JDK_11" default="false" project-jdk-name="25" project-jdk-type="JavaSDK">

Evidence

The IntelliJ project metadata explicitly hardcodes project-jdk-name="25" while the same element
declares languageLevel="JDK_11", showing the portability mismatch introduced by the change.

.idea/misc.xml[4-8]

Agent prompt

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

### Issue description
`.idea/misc.xml` pins the IntelliJ project JDK name to `25` even though the language level remains `JDK_11`, which can force manual SDK remapping for many contributors.

### Issue Context
This is about the committed IDE metadata being portable across developer machines.

### Fix
Either:
- set `project-jdk-name` to a more commonly available/expected value consistent with the repo (e.g., `11`), or
- avoid committing IDE-specific SDK names (depending on project conventions).

### Fix Focus Areas
- .idea/misc.xml[4-8]

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

---

17. Missing trailing newline ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Module.java now ends without a final newline, which can violate standard source formatting
conventions and can trip formatting/lint checks in some environments.

Code

java/src/org/openqa/selenium/bidi/Module.java[68]

+}

Evidence

The file ends at the closing brace and the PR diff marks it as lacking a newline at EOF.

java/src/org/openqa/selenium/bidi/Module.java[60-68]

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/Module.java` ends without a trailing newline.

### Issue Context
The PR diff explicitly indicates `No newline at end of file` on the final `}`.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Module.java[68-68]

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

---

18. Ambiguous Javadoc link ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Module.unsubscribe’s Javadoc uses {@link #subscribe} even though Module declares two overloaded
subscribe methods, which can cause ambiguous/unresolved Javadoc links under doclint. This can break
strict Javadoc generation or produce incorrect API docs.

Code

java/src/org/openqa/selenium/bidi/Module.java[R73-79]

+  /**
+   * Cancels a previously registered event subscription.
+   *
+   * @param subscriptionId a subscription id previously returned by {@link #subscribe}
+   */
+  public final void unsubscribe(String subscriptionId) {
    handle.unsubscribe(subscriptionId);

Evidence

The file defines two subscribe overloads, and the unsubscribe Javadoc references #subscribe
without parameter types, making the link ambiguous.

java/src/org/openqa/selenium/bidi/Module.java[56-71]
java/src/org/openqa/selenium/bidi/Module.java[73-79]

Agent prompt

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

### Issue description
`Module` now has two overloaded `subscribe(...)` methods, but `unsubscribe`’s Javadoc links to `{@link #subscribe}` without a signature, which is ambiguous for Javadoc resolution.

### Issue Context
This PR introduces public `subscribe` overloads and adds Javadoc.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Module.java[56-80]

### Suggested fix
Change the `unsubscribe` Javadoc to reference the specific overload(s), for example:

```java
@param subscriptionId a subscription id previously returned by
   {@link #subscribe(Event, Consumer)} or {@link #subscribe(Event, Consumer, SubscriptionScope)}
```

(or remove the link and refer to “subscribe methods” in plain text).

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

---

Context sources

Review mode: ⚖️ Balanced: This localized runtime change alters numeric-error handling for untrusted JSON input, so it carries real correctness and robustness risk despite the small patch.

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7b9799f

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1760de4

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3d4a143

[titusfortner]

Good progress. Heads up in case you missed it, #17784 added the preserveExtras signal to the projected schema (alongside the existing extensible), so extensibility is now fully derivable per-type: extensible marks the open types, and preserveExtras is pre-scoped to the ones that are both extensible and re-sendable so you don't have to work out that scoping in the generator.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7e5e00c

[qodo-code-review]

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

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3dc9bc8

[qodo-code-review]

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

[qodo-code-review]

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

[qodo-code-review]

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

[titusfortner]

Earlier rounds are addressed. Three things still before merge and a few we can do after the merge if we want just to get this out there. Let me know if any of these don't make sense or if you want me to push anything to this branch.

Blockers:

1. emulation.MediaFeaturesGrid is the one non-string enum in the schema (values:[0,1], primitive:"integer"), and new MediaFeatures().setGrid(Grid._1).toMap() gives "grid":"1" — decision 4 matches a primitive by JSON kind, so that field can't be set correctly today. The note above appendEnumBody doesn't hold here: toMap() puts the value into the map itself, and Json writes a Long as a number. Threading the node's primitive into appendEnumBody and emitting a toWireValue() next to toString() looks like enough, with a fromJson that takes the Long inbound.
2. script.NumberValue accepts new NumberValue("number", true) and serializes {"type":"number","value":true}, and takes the same boolean inbound, though its declared arms are a JSON number or script.SpecialNumber.
3. new DisownParameters(Arrays.asList((String) null), new RealmTarget("r")).toMap() serializes "handles":[null] — serializeExpr's list branch returns the list as-is when the element itself needs no conversion. The Objects.requireNonNull(..., "<wire> is required") the constructor already emits seems like the thing to reuse per element.

Follow-ups:

4. Typed representation for the rest of the inline unions. The Object fallback in resolveJavaType catches four sites — NumberValue.value, MappingRemoteValue, MappingLocalValue, PrintParameters.pageRanges — so MapRemoteValue.value is Optional<List<List<Object>>> and a nested remote value comes back a raw LinkedHashMap (decision 1). input.Origin already works, since its scalarValues put it on the isScalarUnion path; these four have union + scalar but no literal vocabulary, so they'd want a second carrier shape. Given every generated type is @Beta/@ApiStatus.Internal, this seems reasonable to land after merge — item 2 is the part I'd want now.
5. {"sharedId":"node","handle":null} accepts the null; SharedReference.handle is optional with no nullable, and MediaFeatures's optionals do carry it. Python rejects this in _read_field, so it's a parity gap too.
6. JsonInput.nextNumber() routes decimals through double, so a retained extra 9007199254740993.0 returns ...992 (decision 10). Shared JSON, probably its own change.
7. Explicit null vs omission inbound: Info.parent gives parentSet == false either way. The mechanism already exists via SetOverride — decode just doesn't populate it. You noted this in the needsBuilder block of fromJson.
8. The schema's vendor section isn't consumed; the ADR scopes "the spec" to the vendor extensions a binding supports, so this wants an in-or-out call.

[qodo-code-review]

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

[qodo-code-review]

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

[qodo-code-review]

1. Module tests cannot compile 🐞 Bug ≡ Correctness

The new protocol/module test target omits direct dependencies on Selenium core and the BiDi client
even though BrowsingContextModuleTest imports WindowType and BiDiException. The suite macro
compiles all test sources into a support library using only this target's declared dependencies, so
Bazel strict-deps rejects those imports.

Agent Prompt

Issue description
The protocol module test suite directly imports Selenium core and BiDi client classes but does not declare their Bazel targets, causing strict-deps compilation failures.

Fix Focus Areas
- java/test/org/openqa/selenium/bidi/protocol/module/BUILD.bazel[12-18]

Recommended Fix
Add `//java/src/org/openqa/selenium:core` and `//java/src/org/openqa/selenium/bidi` to the suite's `deps` list. Keep `bidi-generated` as the direct dependency for generated protocol classes.

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

Dismiss ↗ | View ↗

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 115e85a

[qodo-code-review]

Code review by qodo was updated up to the latest commit 88d9de1

[pujagani]

@titusfortner I have addressed the blockers, is this good to merge? I will address the rest in the next PR.