.pr_agent_auto_best_practices

Pattern 1: Validate required inputs/outputs early (CLI args, env vars, workflow inputs, and step outputs) and fail fast with a clear, actionable error message instead of relying on null-forgiving operators, implicit defaults, or downstream crashes.

Example code before:

# bash
out="$1"
echo "tag=$TAG" >> "$GITHUB_OUTPUT"
cd "$DOTNET_DIR"
run_tool --output "$out"

Example code after:

# bash
set -euo pipefail
out="${1:-}"
if [ -z "${out//[[:space:]]/}" ]; then
  echo "ERROR: --output must be non-empty" >&2
  exit 2
fi
if [ -z "${GITHUB_OUTPUT:-}" ]; then
  echo "ERROR: GITHUB_OUTPUT not set" >&2
  exit 2
fi
if [ ! -d "$DOTNET_DIR" ]; then
  echo "ERROR: missing directory: $DOTNET_DIR" >&2
  exit 2
fi
run_tool --output "$out"

Relevant past accepted suggestions:

---

Suggestion 1:

Validate VNC address string

In findVncEndpoint, add a check to ensure the vncLocalAddress string is not empty or just whitespace before attempting to create a URI from it.

java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java [225-229]

 String vncLocalAddress = (String) caps.getCapability("se:vncLocalAddress");
-if (vncLocalAddress == null) {
-  LOG.warning("No VNC endpoint address in capabilities");
+if (vncLocalAddress == null || vncLocalAddress.trim().isEmpty()) {
+  LOG.warning("No valid VNC endpoint address in capabilities");
   return Optional.empty();
 }

---

Suggestion 2:

Validate required input early

Add an explicit validation for options.BrowserName and pass the validated value to Selenium Manager instead of using !, so missing/invalid browser names produce a deterministic, actionable error.

dotnet/src/webdriver/DriverFinder.cs [67-74]

 private async ValueTask DiscoverBinaryPathsAsync()
 {
-    BrowserDiscoveryResult smResult = await SeleniumManager.DiscoverBrowserAsync(options.BrowserName!, new BrowserDiscoveryOptions
+    if (string.IsNullOrWhiteSpace(options.BrowserName))
+    {
+        throw new NoSuchDriverException("Browser name must be specified to find the driver using Selenium Manager.");
+    }
+
+    BrowserDiscoveryResult smResult = await SeleniumManager.DiscoverBrowserAsync(options.BrowserName, new BrowserDiscoveryOptions
     {
         BrowserVersion = options.BrowserVersion,
         BrowserPath = options.BinaryLocation,
         Proxy = options.Proxy?.SslProxy ?? options.Proxy?.HttpProxy
     }).ConfigureAwait(false);

---

Suggestion 3:

Validate truncation length is non-negative

In the WithTruncation method, add a check to throw an ArgumentOutOfRangeException if the provided length is negative to prevent downstream errors.

dotnet/src/webdriver/Internal/Logging/LogContext.cs [131-135]

 public ILogContext WithTruncation(int length)
 {
+    if (length < 0)
+    {
+        throw new ArgumentOutOfRangeException(nameof(length), "Truncation length must be non-negative.");
+    }
+
     _truncationLength = length;
     return this;
 }

---

Suggestion 4:

Enforce mutually exclusive modes

Add a check to ensure that the --pre-commit and --pre-push flags are mutually exclusive, exiting with an error if both are provided.

scripts/format.sh [10-24]

 run_lint=false
 mode="default"
+seen_mode=""
 for arg in "$@"; do
     case "$arg" in
         --lint) run_lint=true ;;
 
-        --pre-commit) mode="pre-commit" ;;
-        --pre-push) mode="pre-push" ;;
+        --pre-commit|--pre-push)
+            if [[ -n "$seen_mode" && "$seen_mode" != "$arg" ]]; then
+                echo "ERROR: --pre-commit and --pre-push are mutually exclusive" >&2
+                exit 1
+            fi
+            seen_mode="$arg"
+            [[ "$arg" == "--pre-commit" ]] && mode="pre-commit" || mode="pre-push"
+            ;;
         *)
             echo "Unknown option: $arg" >&2
             echo "Usage: $0 [--pre-commit] [--pre-push] [--lint]" >&2
             exit 1
             ;;
     esac
 done

---

Suggestion 5:

Reject unknown CLI arguments

Treat any unrecognized argument as an error and print a short usage message so CI/manual runs don’t silently ignore typos or unsupported flags.

scripts/format.sh [10-15]

 run_lint=false
 for arg in "$@"; do
     case "$arg" in
         --lint) run_lint=true ;;
+        -h|--help)
+            echo "Usage: $0 [--lint]" >&2
+            exit 0
+            ;;
+        *)
+            echo "ERROR: unknown argument: $arg" >&2
+            echo "Usage: $0 [--lint]" >&2
+            exit 2
+            ;;
     esac
 done

---

Suggestion 6:

Gate Slack notify on inputs

Only run Slack reporting when the webhook secret exists and the disk step produced a valid output, otherwise this can fail the workflow or evaluate invalid comparisons.

.github/workflows/bazel.yml [277-287]

 - name: Report disk space
-  if: always()
+  if: always() && secrets.SLACK_WEBHOOK_URL != '' && steps.disk.outputs.avail != ''
   uses: rtCamp/action-slack-notify@v2
   env:
     SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK_URL }}
     SLACK_CHANNEL: ci-disk-alerts
     SLACK_USERNAME: Disk Monitor
     SLACK_TITLE: "${{ steps.disk.outputs.avail }}GB remaining"
     SLACK_MESSAGE: "${{ inputs.name }} on ${{ inputs.os }}"
     SLACK_COLOR: ${{ steps.disk.outputs.avail < 10 && 'danger' || (steps.disk.outputs.avail < 30 && 'warning' || 'good') }}
     SLACK_ICON_EMOJI: ":floppy_disk:"

---

Suggestion 7:

Fail fast on missing inputs

Validate that $DOTNET_DIR exists and that at least one .csproj was found before running, so mis-invocations fail fast with a clear message.

dotnet/private/dotnet_format.bzl [49-59]

 # Find the workspace root
 WORKSPACE_ROOT="${{BUILD_WORKSPACE_DIRECTORY:-$RUNFILES_DIR/_main}}"
 DOTNET_DIR="$WORKSPACE_ROOT/dotnet"
 
+if [[ ! -d "$DOTNET_DIR" ]]; then
+    echo "ERROR: Could not find dotnet directory at $DOTNET_DIR" >&2
+    exit 1
+fi
+
 cd "$DOTNET_DIR"
 
 echo "Running dotnet format on all projects..."
-find "$DOTNET_DIR/src" "$DOTNET_DIR/test" -name "*.csproj" 2>/dev/null | while read -r proj; do
+mapfile -t projects < <(find "$DOTNET_DIR/src" "$DOTNET_DIR/test" -name "*.csproj" 2>/dev/null)
+if [[ "${#projects[@]}" -eq 0 ]]; then
+    echo "ERROR: No .csproj files found under $DOTNET_DIR/src or $DOTNET_DIR/test" >&2
+    exit 1
+fi
+for proj in "${projects[@]}"; do
     echo "  Formatting $proj..."
     "$DOTNET" format "$proj" || exit 1
-done || exit 1
+done

---

Suggestion 8:

Validate CLI/env inputs before use

Trim and validate --version/--output and require/validate BUILD_WORKSPACE_DIRECTORY (or explicitly define a fallback) so the script doesn't write to an unexpected relative path or accept invalid versions.

scripts/update_docfx.py [126-135]

-version = choose_version(versions, args.allow_prerelease, args.version)
+explicit_version = args.version.strip() if args.version else None
+if explicit_version:
+    try:
+        Version(explicit_version)
+    except InvalidVersion as e:
+        raise ValueError(f"Invalid --version: {explicit_version}") from e
+
+version = choose_version(versions, args.allow_prerelease, explicit_version)
 nupkg_url = NUGET_NUPKG_URL.format(version=version)
 sha256 = sha256_of_url(nupkg_url)
 
-output_path = Path(args.output)
+output_arg = (args.output or "").strip()
+if not output_arg:
+    raise ValueError("--output must be a non-empty path")
+output_path = Path(output_arg)
 if not output_path.is_absolute():
-    workspace_dir = os.environ.get("BUILD_WORKSPACE_DIRECTORY")
-    if workspace_dir:
-        output_path = Path(workspace_dir) / output_path
+    workspace_dir = (os.environ.get("BUILD_WORKSPACE_DIRECTORY") or "").strip()
+    if not workspace_dir:
+        raise EnvironmentError("BUILD_WORKSPACE_DIRECTORY is required when --output is a relative path")
+    output_path = Path(workspace_dir) / output_path
 output_path.write_text(render_docfx_repo(version, sha256))

---

Suggestion 9:

Validate required release inputs

Add validation to the prep_release task to ensure the required version argument is provided, preventing silent failures and providing a clear error message to the user if it is missing.

Rakefile [107-118]

 desc 'Update everything in preparation for a release'
 task :prep_release, [:version, :channel] do |_task, arguments|
   version = arguments[:version]
+  raise 'Missing required version: ./go prep_release[4.31.0,early-stable]' if version.nil? || version.empty?
 
-  Rake::Task['update_browsers'].invoke(arguments[:channel])
-  Rake::Task['update_cdp'].invoke(arguments[:channel])
+  channel = arguments[:channel] || 'stable'
+
+  Rake::Task['update_browsers'].invoke(channel)
+  Rake::Task['update_cdp'].invoke(channel)
   Rake::Task['update_manager'].invoke
   Rake::Task['java:update'].invoke
   Rake::Task['authors'].invoke
   Rake::Task['all:version'].invoke(version)
   Rake::Task['all:changelogs'].invoke
 end

---

Suggestion 10:

Validate inputs and fail fast

Validate that COMMIT_MESSAGE is non-empty (and optionally trim it) and enable strict shell options to prevent unexpected behavior from empty inputs or failing commands.

.github/workflows/commit-changes.yml [51-85]

 - name: Apply and commit
   id: commit
   run: |
+    set -euo pipefail
+
     if [ -f changes.patch ] && [ -s changes.patch ]; then
+      if [ -z "${COMMIT_MESSAGE//[[:space:]]/}" ]; then
+        echo "::error::Commit message is required"
+        echo "committed=false" >> "$GITHUB_OUTPUT"
+        exit 1
+      fi
+
       if ! git apply --index changes.patch; then
         echo "::error::Failed to apply patch"
         echo "committed=false" >> "$GITHUB_OUTPUT"
         exit 1
       fi
       git config --local user.email "selenium-ci@users.noreply.github.com"
       git config --local user.name "Selenium CI Bot"
       if ! git commit -m "$COMMIT_MESSAGE"; then
         echo "::error::Failed to commit changes"
         echo "committed=false" >> "$GITHUB_OUTPUT"
         exit 1
       fi
-      if [ -n "$PUSH_BRANCH" ]; then
+      if [ -n "${PUSH_BRANCH:-}" ]; then
         if ! git push origin HEAD:"$PUSH_BRANCH" --force; then
           echo "::error::Failed to push to $PUSH_BRANCH"
           echo "committed=false" >> "$GITHUB_OUTPUT"
           exit 1
         fi
       else
         if ! git push; then
           echo "::error::Failed to push"
           echo "committed=false" >> "$GITHUB_OUTPUT"
           exit 1
         fi
       fi
       echo "::notice::Changes committed and pushed"
       echo "committed=true" >> "$GITHUB_OUTPUT"
     else
       echo "::notice::No changes to commit"
       echo "committed=false" >> "$GITHUB_OUTPUT"
     fi

---

Suggestion 11:

Validate workflow inputs before use

Validate/sanitize COMMIT_MESSAGE and PUSH_BRANCH (non-empty, trimmed, and restricted charset) before using them, to avoid unexpected behavior or refspec injection.

.github/workflows/commit-changes.yml [51-72]

 - name: Apply and commit
   id: commit
   run: |
+    set -euo pipefail
+
+    COMMIT_MESSAGE="${COMMIT_MESSAGE#"${COMMIT_MESSAGE%%[![:space:]]*}"}"
+    COMMIT_MESSAGE="${COMMIT_MESSAGE%"${COMMIT_MESSAGE##*[![:space:]]}"}"
+    if [ -z "$COMMIT_MESSAGE" ]; then
+      echo "::error::commit-message must be non-empty"
+      exit 1
+    fi
+
+    if [ -n "${PUSH_BRANCH:-}" ] && ! [[ "$PUSH_BRANCH" =~ ^[A-Za-z0-9._/-]+$ ]]; then
+      echo "::error::push-branch contains invalid characters"
+      exit 1
+    fi
+
     if [ -f changes.patch ] && [ -s changes.patch ]; then
       git apply --index changes.patch
       git config --local user.email "selenium-ci@users.noreply.github.com"
       git config --local user.name "Selenium CI Bot"
       git commit -m "$COMMIT_MESSAGE"
-      if [ -n "$PUSH_BRANCH" ]; then
+      if [ -n "${PUSH_BRANCH:-}" ]; then
         git push origin HEAD:"$PUSH_BRANCH" --force
       else
         git push
       fi
       echo "::notice::Changes committed and pushed"
       echo "committed=true" >> "$GITHUB_OUTPUT"
     else
       echo "::notice::No changes to commit"
       echo "committed=false" >> "$GITHUB_OUTPUT"
     fi
   env:
     COMMIT_MESSAGE: ${{ inputs.commit-message }}
     PUSH_BRANCH: ${{ inputs.push-branch }}

---

Suggestion 12:

Guard against null CLI output

Make the gh pr list parsing robust by returning an empty string when no PR exists (instead of null) before checking -n.

.github/workflows/pin-browsers.yml [39-54]

 - name: Create Pull Request
   env:
     GH_TOKEN: ${{ secrets.SELENIUM_CI_TOKEN }}
   run: |
-    existing=$(gh pr list --head pinned-browser-updates --json number --jq '.[0].number')
+    existing=$(gh pr list --head pinned-browser-updates --json number --jq '.[0].number // empty')
     if [ -n "$existing" ]; then
       echo "::notice::PR #$existing already exists"
     else
       gh pr create \
         --head pinned-browser-updates \
         --base trunk \
         --title "[dotnet][rb][java][js][py] Automated Browser Version Update" \
         --body "This is an automated pull request to update pinned browsers and drivers
 
     Merge after verify the new browser versions properly passing the tests and no bugs need to be filed"
     fi

---

Pattern 2: Make shell/script interactions with paths and file lists robust by using safe delimiters and quoting (find -print0 | xargs -0), avoiding brittle multiline quoting, and batching large argument lists to prevent command-line length overflow.

Example code before:

formatter="$(tool --print-path)"
find src -name '*.java' | xargs $formatter --replace
gh pr create --body "line1
line2"

Example code after:

formatter="$(tool --print-path)"
find "src" -name '*.java' -print0 | xargs -0 -r "$formatter" --replace

cat > pr-body.txt <<'EOF'
line1
line2
EOF
gh pr create --body-file pr-body.txt

Relevant past accepted suggestions:

---

Suggestion 1:

Harden formatter file piping

Improve the robustness of the find | xargs pipeline by using find -print0 and xargs -0 -r to correctly handle filenames containing spaces or newlines.

scripts/format.sh [82-87]

 if changed_matches '^java/'; then
     section "Java"
     echo "    google-java-format" >&2
     GOOGLE_JAVA_FORMAT="$(bazel run --run_under=echo //scripts:google-java-format)"
-    find "${WORKSPACE_ROOT}/java" -type f -name '*.java' | xargs "$GOOGLE_JAVA_FORMAT" --replace
+    find "${WORKSPACE_ROOT}/java" -type f -name '*.java' -print0 | xargs -0 -r "$GOOGLE_JAVA_FORMAT" --replace
 fi

---

Suggestion 2:

Handle filenames safely in xargs

Modify the find and xargs command to use null delimiters (-print0 and -0) to handle filenames containing spaces or special characters correctly.

scripts/format.sh [57]

-find "$PWD/java" -type f -name '*.java' | xargs "$GOOGLE_JAVA_FORMAT" --replace
+find "$PWD/java" -type f -name '*.java' -print0 | xargs -0 "$GOOGLE_JAVA_FORMAT" --replace

---

Suggestion 3:

Make formatter invocation robust

Refactor the java:format task to robustly handle file paths by using find -print0 | xargs -0 and a safer method to capture the formatter path.

rake_tasks/java.rake [396-399]

-formatter = `bazel run --run_under=echo //scripts:google-java-format 2>/dev/null`.strip
-raise 'Failed to get google-java-format path' if formatter.empty? || !$CHILD_STATUS.success?
+formatter = nil
+Bazel.execute('run', ['--run_under=echo'], '//scripts:google-java-format') do |output|
+  formatter = output.lines.last&.strip
+end
+raise 'Failed to get google-java-format path' if formatter.to_s.empty?
 
-sh "find #{Dir.pwd}/java -name '*.java' | xargs #{formatter} --replace"
+sh %(find "#{File.join(Dir.pwd, 'java')}" -name '*.java' -print0 | xargs -0 "#{formatter}" --replace)

---

Suggestion 4:

Avoid command line length overflow

Batch the list of Java files passed to Bazel.execute to avoid exceeding command-line length limits, which can cause the format task to fail.

rake_tasks/java.rake [396-397]

 files = Dir.glob("#{Dir.pwd}/java/**/*.java")
-Bazel.execute('run', ['--', '--replace'] + files, '//scripts:google-java-format')
+files.each_slice(200) do |batch|
+  Bazel.execute('run', ['--', '--replace'] + batch, '//scripts:google-java-format')
+end

---

Suggestion 5:

Avoid brittle multiline CLI quoting

Avoid embedding a multi-line --body string directly in the command; write the body via a heredoc and pass it using --body-file to prevent quoting/indentation issues.

.github/workflows/pin-browsers.yml [42-54]

 run: |
   existing=$(gh pr list --head pinned-browser-updates --json number --jq '.[0].number // empty')
   if [ -n "$existing" ]; then
     echo "::notice::PR #$existing already exists"
   else
+    cat > pr-body.txt <<'EOF'
+This is an automated pull request to update pinned browsers and drivers
+
+Merge after verify the new browser versions properly passing the tests and no bugs need to be filed
+EOF
     gh pr create \
       --head pinned-browser-updates \
       --base trunk \
       --title "[dotnet][rb][java][js][py] Automated Browser Version Update" \
-      --body "This is an automated pull request to update pinned browsers and drivers
-
-  Merge after verify the new browser versions properly passing the tests and no bugs need to be filed"
+      --body-file pr-body.txt
   fi

---

Suggestion 6:

Include nested task files

Update the glob in the rakefile filegroup to be recursive ("rake_tasks//*.rake") to prevent future build failures if task files are moved into subdirectories.**

BUILD.bazel [24-30]

 filegroup(
     name = "rakefile",
     srcs = [
         "Rakefile",
-    ] + glob(["rake_tasks/*.rake", "rake_tasks/*.rb"]),
+    ] + glob(["rake_tasks/**/*.rake", "rake_tasks/**/*.rb"]),
     visibility = ["//rb:__subpackages__"],
 )

---

Pattern 3: When tracking async operations (pending requests/events), make state updates atomic and thread-safe: use concurrent/immutable collections or locks, add/remove entries in the correct order, and snapshot+clear collections before iterating during shutdown/error handling.

Example code before:

// C#
pending[id] = tcs;
using var reg = token.Register(() => {
  tcs.TrySetCanceled();
  pending.Remove(id);
});

foreach (var item in pending.Values) {
  item.TrySetException(ex);
}
pending.Clear();

Example code after:

// C#
pending[id] = tcs;
using var reg = token.Register(() => {
  if (pending.TryRemove(id, out _)) {
    tcs.TrySetCanceled(token);
  }
});

var snapshot = pending.Values.ToList();
pending.Clear();
foreach (var item in snapshot) {
  item.TrySetException(ex);
}

Relevant past accepted suggestions:

---

Suggestion 1:

Fix race condition in pending commands

Fix a race condition when failing pending commands by atomically clearing the _pendingCommands dictionary and iterating over the removed items to prevent hangs.

dotnet/src/webdriver/BiDi/Broker.cs [272-280]

 // Fail all pending commands, as the connection is likely broken if we failed to receive messages.
-foreach (var pendingCommand in _pendingCommands.Values)
+var pendingCommands = _pendingCommands.Values.ToList();
+_pendingCommands.Clear();
+foreach (var pendingCommand in pendingCommands)
 {
     pendingCommand.TaskCompletionSource.TrySetException(ex);
 }
 
-_pendingCommands.Clear();
-
 throw;

---

Suggestion 2:

Fix race condition causing memory leak

Fix a race condition by adding the command to _pendingCommands before registering the cancellation callback to prevent a potential memory leak.

dotnet/src/webdriver/BiDi/Broker.cs [150-157]

-using var ctsRegistration = cts.Token.Register(() =>
-{
-    tcs.TrySetCanceled(cts.Token);
-    _pendingCommands.TryRemove(command.Id, out _);
-});
-var data = JsonSerializer.SerializeToUtf8Bytes(command, jsonCommandTypeInfo);
 var commandInfo = new CommandInfo(tcs, jsonResultTypeInfo);
 _pendingCommands[command.Id] = commandInfo;
 
+using var ctsRegistration = cts.Token.Register(() =>
+{
+    if (_pendingCommands.TryRemove(command.Id, out _))
+    {
+        tcs.TrySetCanceled(cts.Token);
+    }
+});
+var data = JsonSerializer.SerializeToUtf8Bytes(command, jsonCommandTypeInfo);
+

---

Suggestion 3:

Remove pending entry on send failure

Add a try/catch block around the _transport.SendAsync call to ensure the _pendingCommands entry is cleaned up if the send operation fails.

dotnet/src/webdriver/BiDi/Broker.cs [159]

-await _transport.SendAsync(data, cts.Token).ConfigureAwait(false);
+try
+{
+    await _transport.SendAsync(data, cts.Token).ConfigureAwait(false);
+}
+catch
+{
+    _pendingCommands.TryRemove(command.Id, out _);
+    throw;
+}

---

Suggestion 4:

Make event maps thread-safe

Protect _eventTypesMap and per-event handler lists with a lock (or use immutable/concurrent collections) because SubscribeAsync, UnsubscribeAsync, and event processing can run concurrently and List/Dictionary are not thread-safe.

dotnet/src/webdriver/BiDi/EventDispatcher.cs [35-65]

-private readonly ConcurrentDictionary<string, List<EventHandler>> _eventHandlers = new();
-private readonly Dictionary<string, JsonTypeInfo> _eventTypesMap = [];
+private readonly ConcurrentDictionary<string, ConcurrentBag<EventHandler>> _eventHandlers = new();
+private readonly ConcurrentDictionary<string, JsonTypeInfo> _eventTypesMap = new();
 ...
 _eventTypesMap[eventName] = jsonTypeInfo;
 
-var handlers = _eventHandlers.GetOrAdd(eventName, (a) => []);
+var handlers = _eventHandlers.GetOrAdd(eventName, _ => new ConcurrentBag<EventHandler>());
 ...
 handlers.Add(eventHandler);

---

Suggestion 5:

Use explicit channel options

Improve readability by using an explicit UnboundedChannelOptions object when creating the Channel.

dotnet/src/webdriver/BiDi/Broker.cs [40]

-private readonly Channel<(string Method, EventArgs Params)> _pendingEvents = Channel.CreateUnbounded<(string Method, EventArgs Params)>(new(){ SingleReader = true, SingleWriter = true });
+private readonly Channel<(string Method, EventArgs Params)> _pendingEvents =
+    Channel.CreateUnbounded<(string Method, EventArgs Params)>(new UnboundedChannelOptions
+    {
+        SingleReader = true,
+        SingleWriter = true
+    });

---

Pattern 4: Make error handling and logging resilient: avoid logging sensitive/nullable exception messages directly, log stable identifiers (exception type / request id), broaden or narrow catches intentionally, swallow expected cancellation during shutdown, and ensure cleanup in catch blocks.

Example code before:

try {
  DoWork(token);
} catch (UnsupportedOperationException e) {
  log("failed: " + e.getMessage());
  throw;
}

Example code after:

try {
  DoWork(token);
} catch (OperationCanceledException) {
  // expected during shutdown
} catch (RuntimeException e) {
  log("Request failed (" + e.getClass().getSimpleName() + ")");
  throw;
}

Relevant past accepted suggestions:

---

Suggestion 1:

Use safer exception details in logs

Avoid depending on e.getMessage() (may be null or include sensitive details); log a stable message and optionally include the exception type for debugging.

java/src/org/openqa/selenium/remote/http/DumpHttpExchangeFilter.java [60-62]

 } catch (UnsupportedOperationException e) {
-  builder.append("[Unable to log content: ").append(e.getMessage()).append("]");
+  builder
+      .append("[Unable to log content: ")
+      .append(e.getClass().getSimpleName())
+      .append("]");
 }

---

Suggestion 2:

Broaden exception handling for robustness

Broaden the exception handling in the logging filter by catching RuntimeException instead of UnsupportedOperationException to improve robustness.

java/src/org/openqa/selenium/remote/http/DumpHttpExchangeFilter.java [57-62]

 try {
   builder.append(message.contentAsString());
 }
-catch(UnsupportedOperationException e){
+catch(RuntimeException e){
   builder.append("[Unable to log content: ").append(e.getMessage()).append("]");
 }

---

Suggestion 3:

Make close handshake cancellation-safe

Use CancellationToken.None for the close handshake (and optionally guard it with try/catch) so a canceled receive token doesn’t prevent replying to the close frame and doesn’t obscure the real disconnect.

dotnet/src/webdriver/BiDi/WebSocketTransport.cs [70-76]

 if (result.MessageType == WebSocketMessageType.Close)
 {
-    await _webSocket.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, string.Empty, cancellationToken).ConfigureAwait(false);
+    try
+    {
+        await _webSocket.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, string.Empty, CancellationToken.None).ConfigureAwait(false);
+    }
+    catch
+    {
+        // Ignore close handshake failures; connection is already closing/closed.
+    }
 
     throw new WebSocketException(WebSocketError.ConnectionClosedPrematurely,
         $"The remote end closed the WebSocket connection. Status: {_webSocket.CloseStatus}, Description: {_webSocket.CloseStatusDescription}");
 }

---

Suggestion 4:

Avoid exception for unsubscribed events

In ProcessReceivedMessage, replace the thrown BiDiException for unsubscribed events with a logged warning to prevent the message receiving loop from crashing.

dotnet/src/webdriver/BiDi/Broker.cs [193-205]

 case "event":
     if (method is null) throw new JsonException("The remote end responded with 'event' message type, but missed required 'method' property.");
 
     if (_eventDispatcher.TryGetEventTypeInfo(method, out var eventInfo) && eventInfo is not null)
     {
         var eventArgs = (EventArgs)JsonSerializer.Deserialize(ref paramsReader, eventInfo)!;
 
         _eventDispatcher.EnqueueEvent(method, eventArgs);
     }
     else
     {
-        throw new BiDiException($"The remote end responded with 'event' message type, but no event type mapping for method '{method}' was found.");
+        if (_logger.IsEnabled(LogEventLevel.Warning))
+        {
+            _logger.Warning($"Received event for unsubscribed method '{method}'.");
+        }
     }

---

Suggestion 5:

Don’t throw on cancellation

Swallow OperationCanceledException when awaiting _receivingMessageTask (or inside ReceiveMessagesAsync) so DisposeAsync remains idempotent and doesn't fail during normal cancellation-based shutdown.

dotnet/src/webdriver/BiDi/Broker.cs [96-256]

 public async ValueTask DisposeAsync()
 {
     _receiveMessagesCancellationTokenSource.Cancel();
     _receiveMessagesCancellationTokenSource.Dispose();
 
-    await _receivingMessageTask.ConfigureAwait(false);
+    try
+    {
+        await _receivingMessageTask.ConfigureAwait(false);
+    }
+    catch (OperationCanceledException)
+    {
+        // Expected during disposal.
+    }
 
     _transport.Dispose();
 
     GC.SuppressFinalize(this);
 }
-...
-private async Task ReceiveMessagesAsync(CancellationToken cancellationToken)
-{
-    try
-    {
-        while (!cancellationToken.IsCancellationRequested)
-        {
-            var data = await _transport.ReceiveAsync(cancellationToken).ConfigureAwait(false);
-            ...
-        }
-    }
-    catch (Exception ex) when (ex is not OperationCanceledException)
-    {
-        ...
-        throw;
-    }
-}

---

Suggestion 6:

Use a distinct log prefix for failures

Use a different log prefix for exceptions, such as !!, to distinguish them from successful response logs which use <<.

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [467-475]

 catch (Exception ex)
 {
     if (_logger.IsEnabled(LogEventLevel.Trace))
     {
-        _logger.Trace($"<< [{requestId}] {ex}");
+        _logger.Trace($"!! [{requestId}] Request failed: {ex}");
     }
 
     throw;
 }

---

Suggestion 7:

Harden external log parsing

Make the regex and switch consistent (e.g., WARN vs WARNING) and use DateTimeOffset.TryParse (or TryParseExact) to avoid exceptions on unexpected log lines.

dotnet/src/webdriver/SeleniumManager.cs [340-362]

 const string LogMessageRegexPattern = @"^\[(.*) (INFO|WARN|ERROR|DEBUG|TRACE)\t?\] (.*)$";
 ...
-var dateTime = DateTimeOffset.Parse(match.Groups[1].Value);
-var logLevel = match.Groups[2].Value;
-var message = match.Groups[3].Value;
+if (DateTimeOffset.TryParse(match.Groups[1].Value, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out var dateTime))
+{
+    var logLevel = match.Groups[2].Value;
+    var message = match.Groups[3].Value;
 
-switch (logLevel)
+    switch (logLevel)
+    {
+        case "INFO":
+            _logger.LogMessage(dateTime, LogEventLevel.Info, message);
+            break;
+        case "WARN":
+            _logger.LogMessage(dateTime, LogEventLevel.Warn, message);
+            break;
+        case "ERROR":
+            _logger.LogMessage(dateTime, LogEventLevel.Error, message);
+            break;
+        case "DEBUG":
+            _logger.LogMessage(dateTime, LogEventLevel.Debug, message);
+            break;
+        case "TRACE":
+        default:
+            _logger.LogMessage(dateTime, LogEventLevel.Trace, message);
+            break;
+    }
+}
+else
 {
-    case "INFO":
-        _logger.LogMessage(dateTime, LogEventLevel.Info, message);
-        break;
-    case "WARNING":
-        _logger.LogMessage(dateTime, LogEventLevel.Warn, message);
-        break;
-    case "ERROR":
-        _logger.LogMessage(dateTime, LogEventLevel.Error, message);
-        break;
-    case "DEBUG":
-        _logger.LogMessage(dateTime, LogEventLevel.Debug, message);
-        break;
-    case "TRACE":
-    default:
-        _logger.LogMessage(dateTime, LogEventLevel.Trace, message);
-        break;
+    errOutputBuilder.AppendLine(e.Data);
 }

---

Pattern 5: Centralize duplicated business rules/parsing logic (especially in CI/workflows) into a shared script, composite action, or reusable workflow, and have all callers consume that single source of truth to prevent rule drift.

Example code before:

# workflow A
run: |
  # parse TAG ...
  # validate TAG ...
  echo "version=$VERSION" >> "$GITHUB_OUTPUT"

# workflow B
run: |
  # similar parse/validate with slightly different rules ...
  echo "version=$VERSION" >> "$GITHUB_OUTPUT"

Example code after:

# scripts/parse-tag.sh
# parse + validate once, then print key=value pairs

# workflow A/B
run: |
  ./scripts/parse-tag.sh "${{ inputs.tag }}" >> "$GITHUB_OUTPUT"

Relevant past accepted suggestions:

---

Suggestion 1:

Centralize duplicated release tag parsing logic

The release tag parsing and validation logic is duplicated across pre-release.yml and release.yml. This logic should be extracted into a single reusable workflow to serve as a single source of truth, improving maintainability.

Examples:

.github/workflows/pre-release.yml [117-166]

        run: |
          TAG="${{ inputs.tag }}"
          TAG="${TAG//[[:space:]]/}"

          # Validate tag format: selenium-X.Y.Z or selenium-X.Y.Z-lang
          if [[ ! "$TAG" =~ ^selenium-[0-9]+\.[0-9]+\.[0-9]+(-[a-z]+)?$ ]]; then
            echo "::error::Invalid tag format: '$TAG'. Expected selenium-X.Y.Z or selenium-X.Y.Z-lang"
            exit 1
          fi


 ... (clipped 40 lines)

.github/workflows/release.yml [38-79]

        run: |
          if [ "$EVENT_NAME" == "workflow_dispatch" ]; then
            TAG="$INPUT_TAG"
          else
            # Extract tag from branch name: release-preparation-selenium-4.28.1-ruby -> selenium-4.28.1-ruby
            TAG=$(echo "$PR_HEAD_REF" | sed 's/^release-preparation-//')
          fi

          # Extract version
          VERSION=$(echo "$TAG" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+')

 ... (clipped 32 lines)

Solution Walkthrough:

Before:

# In .github/workflows/pre-release.yml
jobs:
  parse-tag:
    steps:
      - name: Parse tag
        run: |
          TAG="${{ inputs.tag }}"
          # ... complex parsing and validation logic ...
          if [[ "$PATCH" -gt 0 && -z "$LANG_SUFFIX" ]]; then
            echo "::error::Patch releases must specify a language"
            exit 1
          fi
          # ... more validation ...

# In .github/workflows/release.yml
jobs:
  prepare:
    steps:
      - name: Extract and parse tag
        run: |
          # ... logic to get TAG from branch or input ...
          # ... complex parsing and validation logic (duplicated) ...
          if [[ "$PATCH" -gt 0 && -z "$LANG_SUFFIX" ]]; then
            echo "::error::Patch releases must specify a language"
            exit 1
          fi
          # ... more validation (duplicated) ...

After:

# In .github/workflows/parse-release-tag.yml (new file)
on:
  workflow_call:
    inputs:
      tag_string:
        type: string
    outputs:
      tag:
      version:
      language:
jobs:
  parse:
    steps:
      - name: Parse tag
        run: |
          # ... single source of truth for parsing and validation ...
          echo "tag=$TAG" >> "$GITHUB_OUTPUT"
          echo "version=$VERSION" >> "$GITHUB_OUTPUT"
          echo "language=$LANGUAGE" >> "$GITHUB_OUTPUT"

# In .github/workflows/pre-release.yml and release.yml
jobs:
  parse-tag: # or 'prepare' job
    uses: ./.github/workflows/parse-release-tag.yml
    with:
      tag_string: ${{ inputs.tag }} # or ${{ github.event.pull_request.head.ref }}

---

Suggestion 2:

Centralize tag parsing logic

Move tag parsing/validation into a single shared script (or reusable workflow/composite action) and call it from all workflows to prevent future drift and inconsistent rules.

.github/workflows/pre-release.yml [114-166]

 - name: Parse tag
   id: parse
   shell: bash
   run: |
-    TAG="${{ inputs.tag }}"
-    TAG="${TAG//[[:space:]]/}"
+    ./scripts/parse-release-tag.sh "${{ inputs.tag }}" >> "$GITHUB_OUTPUT"
 
-    # Validate tag format: selenium-X.Y.Z or selenium-X.Y.Z-lang
-    if [[ ! "$TAG" =~ ^selenium-[0-9]+\.[0-9]+\.[0-9]+(-[a-z]+)?$ ]]; then
-      echo "::error::Invalid tag format: '$TAG'. Expected selenium-X.Y.Z or selenium-X.Y.Z-lang"
-      exit 1
-    fi
-
-    # Extract version (strip 'selenium-' prefix and optional language suffix)
-    VERSION=$(echo "$TAG" | sed -E 's/^selenium-([0-9]+\.[0-9]+\.[0-9]+)(-[a-z]+)?$/\1/')
-    PATCH=$(echo "$VERSION" | cut -d. -f3)
-
-    # Extract language suffix (default to 'all' if no suffix)
-    if [[ "$TAG" =~ -([a-z]+)$ ]]; then
-      LANG_SUFFIX="${BASH_REMATCH[1]}"
-    else
-      LANG_SUFFIX=""
-    fi
-
-    # Patch releases must have a language suffix
-    if [[ "$PATCH" -gt 0 && -z "$LANG_SUFFIX" ]]; then
-      echo "::error::Patch releases must specify a language (e.g., selenium-${VERSION}-ruby)"
-      exit 1
-    fi
-
-    # Full releases (X.Y.0) must not have a language suffix
-    if [[ "$PATCH" -eq 0 && -n "$LANG_SUFFIX" ]]; then
-      echo "::error::Full releases (X.Y.0) cannot have a language suffix"
-      exit 1
-    fi
-
-    # Validate language suffix (rake namespace aliases allow full names)
-    case "$LANG_SUFFIX" in
-      ruby|python|javascript|java|dotnet)
-        LANGUAGE="$LANG_SUFFIX"
-        ;;
-      "")
-        LANGUAGE="all"
-        ;;
-      *)
-        echo "::error::Invalid language suffix: '$LANG_SUFFIX'. Expected ruby, python, javascript, java, or dotnet"
-        exit 1
-        ;;
-    esac
-
-    echo "tag=$TAG" >> "$GITHUB_OUTPUT"
-    echo "version=$VERSION" >> "$GITHUB_OUTPUT"
-    echo "language=$LANGUAGE" >> "$GITHUB_OUTPUT"
-

---

Suggestion 3:

Add missing tag validation logic

Add validation to the prepare job in release.yml to ensure full releases do not have a language suffix, aligning its logic with the pre-release.yml workflow.

.github/workflows/release.yml [32-79]

 - name: Extract and parse tag
   id: parse
   env:
     EVENT_NAME: ${{ github.event_name }}
     INPUT_TAG: ${{ inputs.tag }}
     PR_HEAD_REF: ${{ github.event.pull_request.head.ref }}
   run: |
     if [ "$EVENT_NAME" == "workflow_dispatch" ]; then
       TAG="$INPUT_TAG"
     else
       # Extract tag from branch name: release-preparation-selenium-4.28.1-ruby -> selenium-4.28.1-ruby
       TAG=$(echo "$PR_HEAD_REF" | sed 's/^release-preparation-//')
     fi
 
     # Extract version
     VERSION=$(echo "$TAG" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+')
     PATCH=$(echo "$VERSION" | cut -d. -f3)
 
     # Extract language suffix
     if [[ "$TAG" =~ -([a-z]+)$ ]]; then
       LANG_SUFFIX="${BASH_REMATCH[1]}"
     else
       LANG_SUFFIX=""
     fi
 
     # Patch releases must have a language suffix
     if [[ "$PATCH" -gt 0 && -z "$LANG_SUFFIX" ]]; then
       echo "::error::Patch releases must specify a language (e.g., selenium-${VERSION}-ruby)"
       exit 1
     fi
 
+    # Full releases (X.Y.0) must not have a language suffix
+    if [[ "$PATCH" -eq 0 && -n "$LANG_SUFFIX" ]]; then
+      echo "::error::Full releases (X.Y.0) cannot have a language suffix"
+      exit 1
+    fi
+
     # Validate language suffix (rake namespace aliases allow full names)
     case "$LANG_SUFFIX" in
       ruby|python|javascript|java|dotnet)
         LANGUAGE="$LANG_SUFFIX"
         ;;
       "")
         LANGUAGE="all"
         ;;
       *)
         echo "::error::Invalid language suffix: '$LANG_SUFFIX'. Expected ruby, python, javascript, java, or dotnet"
         exit 1
         ;;
     esac
 
     echo "tag=$TAG" >> "$GITHUB_OUTPUT"
     echo "version=$VERSION" >> "$GITHUB_OUTPUT"
     echo "language=$LANGUAGE" >> "$GITHUB_OUTPUT"

---

[Auto-generated best practices - 2026-02-26]