[build] Split Rakefile into per-language task files by titusfortner · Pull Request #16979 · SeleniumHQ/selenium

PR Code Suggestions ✨

Latest suggestions up to 40108b9

CategorySuggestion                                                                                                                                    Impact
Possible issue
Don’t misparse non-language flags

Fix the argument parsing in the all:lint task to correctly handle flags. The
current implementation incorrectly treats any argument starting with - as a
language to skip, which will break with flags like --verbose.

Rakefile [260-275]

 task :lint do |_task, arguments|
   all_langs = %w[java py rb node rust]
-  skip = arguments.to_a.select { |a| a.start_with?('-') }.map { |a| a.delete_prefix('-') }
+  skip = arguments.to_a
+                  .select { |a| a.start_with?('-') && !a.start_with?('--') }
+                  .map { |a| a.delete_prefix('-') }
   invalid = skip - all_langs
   raise "Unknown languages: #{invalid.join(', ')}. Valid: #{all_langs.join(', ')}" if invalid.any?
 
   langs = all_langs - skip
   failures = []
   langs.each do |lang|
     puts "Linting #{lang}..."
     Rake::Task["#{lang}:lint"].invoke
   rescue StandardError => e
     failures << "#{lang}: #{e.message}"
   end
   raise "Lint failed:\n#{failures.join("\n")}" unless failures.empty?
 end
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a bug in the argument parsing logic that would cause the task to fail with valid command-line flags like --verbose, and provides a correct fix.

Medium
Prevent nil version results

Add input validation and explicit error handling to the updated_version method
to prevent it from returning nil for unexpected version formats.

rake_tasks/common.rb [21-33]

 def self.updated_version(current, desired = nil, nightly = nil)
   if !desired.nil? && desired != 'nightly'
     # If desired is present, return full 3 digit version
-    desired.split('.').tap { |v| v << 0 while v.size < 3 }.join('.')
-  elsif current.split(/\.|-/).size > 3
+    return desired.split('.').tap { |v| v << 0 while v.size < 3 }.join('.')
+  end
+
+  parts = current.to_s.split(/\.|-/)
+
+  if parts.size > 3
+    raise 'Missing nightly suffix for prerelease bump' if nightly.nil? || nightly.empty?
+
     # if current version is already nightly, just need to bump it; this will be noop for some languages
     pattern = /-?\.?(nightly|SNAPSHOT|dev|\d{12})\d*$/
-    current.gsub(pattern, nightly)
-  elsif current.split(/\.|-/).size == 3
+    return current.gsub(pattern, nightly)
+  end
+
+  if parts.size == 3
+    raise 'Missing nightly suffix for nightly bump' if nightly.nil? || nightly.empty?
+
     # if current version is not nightly, need to bump the version and make nightly
-    "#{current.split(/\.|-/).tap { |i| (i[1] = i[1].to_i + 1) && (i[2] = 0) }.join('.')}#{nightly}"
+    bumped = parts.tap { |i| (i[1] = i[1].to_i + 1) && (i[2] = 0) }.join('.')
+    return "#{bumped}#{nightly}"
   end
+
+  raise "Unrecognized version format: #{current}"
 end
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a scenario where the updated_version method could return nil, leading to runtime errors, and proposes adding validation to fail fast with a clear error message, which improves robustness.

Medium
Validate lockfile and checksum block

Add checks to the rb:pin task to validate the format of Gemfile.lock and
MODULE.bazel, raising an error if parsing fails, to prevent silent failures or
NoMethodError exceptions.

rake_tasks/ruby.rake [149-196]

 lock_content = File.read(gemfile_lock)
 gem_section = lock_content[/GEM\n\s+remote:.*?\n\s+specs:\n(.*?)(?=\n[A-Z]|\Z)/m, 1]
+raise "Could not parse GEM section from #{gemfile_lock}" if gem_section.nil? || gem_section.empty?
+
 gems = gem_section.scan(/^    ([a-zA-Z0-9_-]+) \(([^)]+)\)$/)
+raise "No gems found in GEM specs section of #{gemfile_lock}" if gems.empty?
+
 needed_gems = gems.map { |name, version| "#{name}-#{version}" }
 ...
+unless module_content.match?(/    gem_checksums = \{[^}]+\},/m)
+  raise "Could not find gem_checksums block in #{module_bazel}"
+end
+
 new_content = module_content.sub(/    gem_checksums = \{[^}]+\},/m, formatted)
 File.write(module_bazel, new_content)

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that parsing external files without validation can lead to nil errors or silent failures, and the proposed checks make the rb:pin task more robust and its failures more explicit.

Medium
Prevent double-loading task files

Avoid reloading the same Rake file for namespace aliases. Instead, load each
file once and create aliases by defining tasks that delegate to the canonical
ones.

Rakefile [33-39]

+namespace(:java) { load 'rake_tasks/java.rake' }
 namespace(:rb) { load 'rake_tasks/ruby.rake' }
-namespace(:ruby) { load 'rake_tasks/ruby.rake' }
 namespace(:py) { load 'rake_tasks/python.rake' }
-namespace(:python) { load 'rake_tasks/python.rake' } # alias
 namespace(:node) { load 'rake_tasks/node.rake' }
-namespace(:js) { load 'rake_tasks/node.rake' } # alias
-namespace(:javascript) { load 'rake_tasks/node.rake' } # alias
+namespace(:dotnet) { load 'rake_tasks/dotnet.rake' }
+namespace(:rust) { load 'rake_tasks/rust.rake' }
+namespace(:grid) { load 'rake_tasks/grid.rake' }
+namespace(:bazel) { load 'rake_tasks/bazel.rake' }
+namespace(:appium) { load 'rake_tasks/appium.rake' }
 
+# Aliases (delegate instead of re-loading files)
+namespace(:ruby) { task(:build => :'rb:build'); task(:lint => :'rb:lint'); task(:docs => :'rb:docs'); task(:update => :'rb:update') }
+namespace(:python) { task(:build => :'py:build'); task(:lint => :'py:lint'); task(:docs => :'py:docs'); task(:update => :'py:update') }
+namespace(:js) { task(:build => :'node:build'); task(:lint => :'node:lint'); task(:docs => :'node:docs'); task(:update => :'node:update') }
+namespace(:javascript) { task(:build => :'node:build'); task(:lint => :'node:lint'); task(:docs => :'node:docs'); task(:update => :'node:update') }
+
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that loading the same file for aliases is bad practice and can cause warnings or errors, proposing a more robust delegation pattern which improves maintainability.

Medium
Make wheel install deterministic

Modify the py:install task to use Dir.glob to find the wheel file path instead
of relying on shell globbing, ensuring the command is more reliable across
different platforms.

rake_tasks/python.rake [118-122]

 desc 'Install Python wheel locally'
 task :install do
   Bazel.execute('build', [], '//py:selenium-wheel')
-  sh 'pip install bazel-bin/py/selenium-*.whl'
+  wheels = Dir.glob('bazel-bin/py/selenium-*.whl')
+  raise "Expected 1 wheel, found #{wheels.size}: #{wheels.join(', ')}" unless wheels.size == 1
+
+  sh 'pip', 'install', wheels.first
 end
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that shell globbing can be unreliable and proposes a more robust, platform-independent way to find the wheel file, which improves the reliability of the :install task.

Low
General
Ensure lint tasks always run

Ensure language-specific lint tasks can be re-run within the same process by
calling reenable on each task before invoking it. This prevents them from being
skipped on subsequent calls.

Rakefile [268-273]

 langs.each do |lang|
   puts "Linting #{lang}..."
-  Rake::Task["#{lang}:lint"].invoke
+  t = Rake::Task["#{lang}:lint"]
+  t.reenable
+  t.invoke
 rescue StandardError => e
   failures << "#{lang}: #{e.message}"
 end
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out a potential issue where tasks might not re-run if invoked multiple times, improving the task's reliability by using reenable before invoke.

Low
Learned
best practice
Fail fast on missing version

If SE_VERSION is not found, this returns nil and later URL/version uses will
fail unclearly; raise a descriptive error and validate the extracted value is
non-blank.

rake_tasks/python.rake [5-9]

 def python_version
   File.foreach('py/BUILD.bazel') do |line|
-    return line.split('=').last.strip.tr('"', '') if line.include?('SE_VERSION')
+    next unless line.include?('SE_VERSION')
+
+    v = line.split('=').last&.strip&.tr('"', '')
+    return v unless v.nil? || v.empty?
   end
+
+  raise "Could not determine Python version from py/BUILD.bazel (missing/blank SE_VERSION)"
 end
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation/guards at integration boundaries (e.g., file parsing) so missing/blank values fail fast with a clear error.

Low
Reenable tasks invoked twice

Rake tasks only run once per process, so the second invoke is a no-op; call
reenable before the second invocation to ensure dependencies are re-pinned after
edits.

rake_tasks/java.rake [337-361]

 task :update do
   puts 'Updating Maven dependencies'
   # Make sure things are in a good state to start with
   Rake::Task['java:pin'].invoke
 ...
   File.write(file_path, content)
 
-  Rake::Task['java:pin'].invoke
+  t = Rake::Task['java:pin']
+  t.reenable
+  t.invoke
 end

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Make lifecycle-sensitive task invocations robust by ensuring idempotent/repeatable execution (reenable tasks when invoking multiple times).

Low
  • Update

Previous suggestions

✅ Suggestions up to commit ac29f0c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix retry-loop exception handling

Fix a syntax error in the retry loop's exception handling by wrapping the loop's
body in a begin...rescue...end block.

rake_tasks/java.rake [144-165]

 def poll_and_publish_deployment(deployment_id, token)
   encoded_id = URI.encode_www_form_component(deployment_id.strip)
   status = {}
   max_attempts = 60
   delay = 5
 
   max_attempts.times do |attempt|
-    status = sonatype_api_post("https://central.sonatype.com/api/v1/publisher/status?id=#{encoded_id}", token)
-    state = status['deploymentState']
-    puts "Deployment state: #{state}"
+    begin
+      status = sonatype_api_post("https://central.sonatype.com/api/v1/publisher/status?id=#{encoded_id}", token)
+      state = status['deploymentState']
+      puts "Deployment state: #{state}"
 
-    case state
-    when 'VALIDATED', 'PUBLISHED' then break
-    when 'FAILED' then raise "Deployment failed: #{status['errors']}"
+      case state
+      when 'VALIDATED', 'PUBLISHED' then break
+      when 'FAILED' then raise "Deployment failed: #{status['errors']}"
+      end
+
+      sleep(delay) unless attempt == max_attempts - 1
+    rescue StandardError => e
+      raise if e.message.start_with?('Deployment failed')
+
+      warn "API error (attempt #{attempt + 1}/#{max_attempts}): #{e.message}"
+      sleep(delay) unless attempt == max_attempts - 1
     end
-    sleep(delay) unless attempt == max_attempts - 1
-  rescue StandardError => e
-    raise if e.message.start_with?('Deployment failed')
-
-    warn "API error (attempt #{attempt + 1}/#{max_attempts}): #{e.message}"
-    sleep(delay) unless attempt == max_attempts - 1
   end
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a syntax error in the poll_and_publish_deployment method where rescue is improperly used with a do...end block, which would prevent the script from running.

High
Parse JSON for package version

Robustly extract the package version by parsing package.json instead of using a
simple string match, which could return an incorrect value.

rake_tasks/node.rake [3-7]

 def node_version
-  File.foreach('javascript/selenium-webdriver/package.json') do |line|
-    return line.split(':').last.strip.tr('",', '') if line.include?('version')
-  end
+  json = JSON.parse(File.read('javascript/selenium-webdriver/package.json'))
+  json.fetch('version')
 end
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that the current line-based parsing of package.json is fragile and could extract an incorrect version, proposing a robust fix using JSON parsing.

Medium
Ensure credential checks rerun

Use t.reenable before t.invoke within the all:check_credentials task to ensure
that per-language credential checks can be run multiple times within the same
process.

Rakefile [226-235]

 task :check_credentials do |_task, arguments|
   args = arguments.to_a
   failures = []
   %w[java py rb dotnet node].each do |lang|
-    Rake::Task["#{lang}:check_credentials"].invoke(*args)
+    t = Rake::Task["#{lang}:check_credentials"]
+    t.reenable
+    t.invoke(*args)
   rescue StandardError => e
     failures << "#{lang}: #{e.message}"
   end
   raise "Credential check failed:\n#{failures.join("\n")}" unless failures.empty?
 end
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that Rake::Task#invoke only runs once and proposes using reenable to ensure the credential checks can be re-executed, improving the task's robustness.

Medium
Include nested task files
Suggestion Impact:The commit modified the same filegroup to include rake_tasks via a glob, but it used non-recursive patterns (rake_tasks/*.rake and rake_tasks/*.rb) rather than the suggested recursive ** patterns.

code diff:

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

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__"],
 )
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the glob pattern is not recursive and proposes a change to rake_tasks/**/*.rake to make the build more robust against future file organization changes.

Low
General
Add timeouts to HTTP checks

Add open_timeout and read_timeout to the HTTP request in
verify_package_published to prevent tasks from hanging on network stalls.

rake_tasks/common.rb [68-75]

 def self.verify_package_published(url)
   puts "Verifying #{url}..."
   uri = URI(url)
-  res = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https') { |http| http.request(Net::HTTP::Get.new(uri)) }
+  res = Net::HTTP.start(uri.hostname, uri.port,
+                        use_ssl: uri.scheme == 'https',
+                        open_timeout: 10,
+                        read_timeout: 30) do |http|
+    http.request(Net::HTTP::Get.new(uri))
+  end
   raise "Package not published: #{url}" unless res.is_a?(Net::HTTPSuccess)
 
   puts 'Verified!'
 end
Suggestion importance[1-10]: 7

__

Why: The suggestion improves the robustness of the verify_package_published method by adding network timeouts, preventing CI jobs from hanging indefinitely on network issues.

Medium
Learned
best practice
Harden API calls and parsing

Use use_ssl based on the URL scheme, add timeouts, and handle non-JSON/invalid
responses to avoid unexpected exceptions during release automation.

rake_tasks/java.rake [74-83]

 def sonatype_api_post(url, token)
-  uri = URI(url)
+  uri = URI.parse(url.to_s.strip)
   req = Net::HTTP::Post.new(uri)
   req['Authorization'] = "Basic #{token}"
 
-  res = Net::HTTP.start(uri.hostname, uri.port, use_ssl: true) { |http| http.request(req) }
+  res = Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == 'https', open_timeout: 10, read_timeout: 30) do |http|
+    http.request(req)
+  end
   raise "Sonatype API error (#{res.code}): #{res.body}" unless res.is_a?(Net::HTTPSuccess)
 
-  res.body.to_s.empty? ? {} : JSON.parse(res.body)
+  body = res.body.to_s
+  return {} if body.strip.empty?
+
+  JSON.parse(body)
+rescue JSON::ParserError => e
+  raise "Sonatype API returned non-JSON response: #{e.message}"
 end
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add validation/guards at integration boundaries (e.g., URLs/HTTP/JSON parsing) before use, and fail with clear errors.

Low
✅ Suggestions up to commit a2ac5b7
CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Normalize task arguments to strings

Convert Rake task arguments to strings using map(&:to_s) before processing to
prevent runtime errors from non-string values.

Rakefile [260-261]

 all_langs = %w[java py rb node rust]
-skip = arguments.to_a.select { |a| a.start_with?('-') }.map { |a| a.delete_prefix('-') }
+args = arguments.to_a.map(&:to_s)
+skip = args.select { |a| a.start_with?('-') }.map { |a| a.delete_prefix('-') }
Suggestion importance[1-10]: 7

__

Why: This is a valid defensive programming suggestion that prevents a potential NoMethodError if the Rake task is invoked programmatically with non-string arguments, improving the script's robustness.

Medium
Use absolute path for loading

Make the load path absolute using File.expand_path to ensure it works correctly
regardless of the current working directory.

Rakefile [43]

-namespace(:appium) { load 'rake_tasks/appium.rake' }
+namespace(:appium) { load File.expand_path('rake_tasks/appium.rake', __dir__) }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that using a relative path with load can be fragile and makes the build script more robust by using an absolute path, which is a good practice.

Low
Possible issue
Avoid reading wrong Maven credentials

Improve the parsing of settings.xml to ensure credentials are only read from the
correct block by stopping when is found.

rake_tasks/java.rake [85-105]

 def read_m2_user_pass
   settings_path = File.join(Dir.home, '.m2', 'settings.xml')
   unless File.exist?(settings_path)
     warn "Maven settings file not found at #{settings_path}"
     return
   end
 
   puts 'Maven environment variables not set, inspecting ~/.m2/settings.xml.'
   settings = File.read(settings_path)
-  found_section = false
+  in_central_server = false
+
   settings.each_line do |line|
-    if !found_section
-      found_section = line.include? '<id>central</id>'
+    if !in_central_server
+      in_central_server = line.include?('<id>central</id>')
+      next
+    end
+
+    if line.include?('</server>')
+      break
     elsif line.include?('<username>')
       ENV['MAVEN_USER'] = line[%r{<username>(.*?)</username>}, 1]
     elsif line.include?('<password>')
       ENV['MAVEN_PASSWORD'] = line[%r{<password>(.*?)</password>}, 1]
     end
+
     break if ENV['MAVEN_PASSWORD'] && ENV['MAVEN_USER']
   end
 end
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential bug where incorrect credentials could be parsed from settings.xml and provides a more robust implementation to prevent this.

Medium
Avoid hard dependency on git

Wrap the Git.open call in a begin/rescue block to prevent crashes in non-git
environments, setting SeleniumRake.git to nil on failure.

Rakefile [29]

-SeleniumRake.git = Git.open(__dir__)
+begin
+  SeleniumRake.git = Git.open(__dir__)
+rescue StandardError
+  SeleniumRake.git = nil
+end
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that Git.open can fail and crash the Rakefile, making it more robust by handling the exception gracefully.

Medium
Make git staging best-effort

Use the safe navigation operator (&.) for all calls to SeleniumRake.git.add to
prevent NoMethodError if git is not available.

Rakefile [63]

-SeleniumRake.git.add('common/repositories.bzl')
+SeleniumRake.git&.add('common/repositories.bzl')
Suggestion importance[1-10]: 7

__

Why: This suggestion correctly points out that if git initialization fails, calls to SeleniumRake.git.add will raise an error, and proposes using the safe navigation operator (&.) to prevent crashes.

Medium
Handle missing outdated command output

Add a check to ensure the output from the Bazel command is not nil or empty
before attempting to parse it, preventing a potential crash.

rake_tasks/java.rake [344-349]

 output = nil
 Bazel.execute('run', [], '@maven//:outdated') do |out|
   output = out
 end
 
+if output.nil? || output.strip.empty?
+  raise 'Failed to fetch Maven outdated report (no output returned)'
+end
+
 versions = output.scan(/(\S+) \[\S+ -> (\S+)\]/).to_h
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly adds a guard clause to prevent a NoMethodError if the Bazel.execute command returns no output, improving the script's robustness.

Low
Make local file copying robust

Make the local file copying more robust by creating the destination directory if
it doesn't exist and raising an error if a source atom file is missing.

rake_tasks/python.rake [92-94]

+FileUtils.mkdir_p("#{lib_path}/remote")
+
 %w[getAttribute.js isDisplayed.js findElements.js].each do |atom|
-  FileUtils.cp("#{bazel_bin}/remote/#{atom}", "#{lib_path}/remote/#{atom}")
+  src = "#{bazel_bin}/remote/#{atom}"
+  dest = "#{lib_path}/remote/#{atom}"
+  raise "Missing generated atom: #{src}" unless File.exist?(src)
+
+  FileUtils.cp(src, dest)
 end
Suggestion importance[1-10]: 5

__

Why: The suggestion improves the robustness of the file copy operation by ensuring the destination directory exists and that source files are present, preventing potential script failures.

Low
General
Forward linter task arguments
Suggestion Impact:The commit rewrote the `all:lint` task to split incoming arguments into skip flags (those starting with "-") and forwardable args, then invoked each `#{lang}:lint` task with `*forward_args`. This implements the requested argument forwarding behavior while preserving skip-language functionality.

code diff:

+  desc 'Run linters for all languages (skip with: ./go all:lint -rb -rust)'
+  task :lint do |_task, arguments|
+    all_langs = %w[java py rb node rust]
+    args = arguments.to_a
+    skip_flags, forward_args = args.partition { |a| a.start_with?('-') }
+    skip = skip_flags.map { |a| a.delete_prefix('-') }
+    invalid = skip - all_langs
+    raise "Unknown languages: #{invalid.join(', ')}. Valid: #{all_langs.join(', ')}" if invalid.any?
+
+    langs = all_langs - skip
+    failures = []
+    langs.each do |lang|
+      puts "Linting #{lang}..."
+      Rake::Task["#{lang}:lint"].invoke(*forward_args)
+    rescue StandardError => e
+      failures << "#{lang}: #{e.message}"
     end
-
-    Bazel.execute('run', [], '//py:mypy')
-    Bazel.execute('run', [], '//py:ruff')
-    Bazel.execute('run', [], '//rb:steep')
-    shellcheck = Bazel.execute('build', [], '@multitool//tools/shellcheck')
-    Bazel.execute('run', ['--', '-shellcheck', shellcheck], '@multitool//tools/actionlint:cwd')
-  end
-
-  # Example: `./go all:prepare[4.31.0,early-stable]`
-  # Equivalent to .github/workflows/pre-release.yml in a single command
-  desc 'Update everything in preparation for a release'
-  task :prepare, [:version, :channel] do |_task, arguments|
-    version = arguments[:version]
-
-    Rake::Task['update_browsers'].invoke(arguments[:channel])
-    Rake::Task['all:update_cdp'].invoke(arguments[: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
+    raise "Lint failed:\n#{failures.join("\n")}" unless failures.empty?
   end

Modify the all:lint task to forward any command-line arguments (except for the
language skip flags) to each individual language's lint task.

Rakefile [258-274]

 desc 'Run linters for all languages (skip with: ./go all:lint -rb -rust)'
 task :lint do |_task, arguments|
   all_langs = %w[java py rb node rust]
-  skip = arguments.to_a.select { |a| a.start_with?('-') }.map { |a| a.delete_prefix('-') }
+  raw_args = arguments.to_a
+  skip_args = raw_args.select { |a| a.start_with?('-') }
+  skip = skip_args.map { |a| a.delete_prefix('-') }
+
   invalid = skip - all_langs
   raise "Unknown languages: #{invalid.join(', ')}. Valid: #{all_langs.join(', ')}" if invalid.any?
 
+  forward_args = raw_args - skip_args
   langs = all_langs - skip
+
   failures = []
   langs.each do |lang|
     puts "Linting #{lang}..."
-    Rake::Task["#{lang}:lint"].invoke
+    Rake::Task["#{lang}:lint"].invoke(*forward_args)
   rescue StandardError => e
     failures << "#{lang}: #{e.message}"
   end
+
   raise "Lint failed:\n#{failures.join("\n")}" unless failures.empty?
 end

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that arguments are not being passed to the individual linter tasks and provides a correct implementation to forward them, improving the task's functionality.

Low
✅ Suggestions up to commit 08841ce
CategorySuggestion                                                                                                                                    Impact
Security
Tighten artifact file permissions
Suggestion Impact:Updated the chmod permissions for both generated .NET release zip artifacts from world-writable (0o666) to 0o644 as suggested.

code diff:

   FileUtils.copy('bazel-bin/dotnet/release.zip', "build/dist/selenium-dotnet-#{dotnet_version}.zip")
-  FileUtils.chmod(0o666, "build/dist/selenium-dotnet-#{dotnet_version}.zip")
+  FileUtils.chmod(0o644, "build/dist/selenium-dotnet-#{dotnet_version}.zip")
   FileUtils.copy('bazel-bin/dotnet/strongnamed.zip', "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")
-  FileUtils.chmod(0o666, "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")
+  FileUtils.chmod(0o644, "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")

Change the file permissions for the generated .zip artifacts from world-writable
0o666 to a more secure 0o644 to prevent tampering.

rake_tasks/dotnet.rake [21-24]

 FileUtils.copy('bazel-bin/dotnet/release.zip', "build/dist/selenium-dotnet-#{dotnet_version}.zip")
-FileUtils.chmod(0o666, "build/dist/selenium-dotnet-#{dotnet_version}.zip")
+FileUtils.chmod(0o644, "build/dist/selenium-dotnet-#{dotnet_version}.zip")
 FileUtils.copy('bazel-bin/dotnet/strongnamed.zip', "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")
-FileUtils.chmod(0o666, "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")
+FileUtils.chmod(0o644, "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies overly permissive file permissions (0o666) on release artifacts, which is a security risk, and proposes a more secure alternative (0o644).

Medium
Avoid shelling out unsafely

Replace the backtick shell execution of git log with Open3.capture2 to prevent
potential shell injection vulnerabilities from the tag or path variables.

rake_tasks/common.rb [52-53]

-command = "git log #{tag}...HEAD --pretty=format:'%s' --reverse -- #{path}"
-log = `#{command}`
+require 'open3'
+...
+log, status = Open3.capture2('git', 'log', "#{tag}...HEAD", "--pretty=format:%s", '--reverse', '--', path.to_s)
+raise "git log failed for changelog generation (#{tag}..HEAD)" unless status.success?
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential shell injection vulnerability and recommends using Open3.capture2 with an argument array, which is a best practice for security and robustness.

Medium
Possible issue
Add HTTP timeouts for downloads
Suggestion Impact:The commit added `response = nil` before the retry loop, matching the first line of the suggested change, but it did not implement the main suggestion of using `Net::HTTP.start` with `open_timeout` and `read_timeout`.

code diff:

     uri = URI("https://rubygems.org/gems/#{key}.gem")
+    response = nil
 
     5.times do
       response = Net::HTTP.get_response(uri)

Add open_timeout and read_timeout to the Net::HTTP request when downloading gems
to prevent the task from hanging indefinitely on network issues.

rake_tasks/ruby.rake [168-181]

+response = nil
 5.times do
-  response = Net::HTTP.get_response(uri)
+  response = Net::HTTP.start(uri.hostname, uri.port,
+                            use_ssl: uri.scheme == 'https',
+                            open_timeout: 10,
+                            read_timeout: 30) do |http|
+    http.request(Net::HTTP::Get.new(uri))
+  end
   break unless response.is_a?(Net::HTTPRedirection)
 
   uri = URI(response['location'])
 end
 
 unless response.is_a?(Net::HTTPSuccess)
   puts "  #{key}: failed (HTTP #{response.code})"
   failed << key
   next
 end
 
 sha = Digest::SHA256.hexdigest(response.body)

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that the HTTP request lacks timeouts, which can cause the task to hang indefinitely, and proposes a robust solution using Net::HTTP.start with timeouts.

Medium
Avoid forwarding nil arguments

Add .compact to arguments.to_a in the docs task to filter out nil values before
forwarding them, preventing potential errors in sub-tasks.

Rakefile [200-207]

 task :docs do |_task, arguments|
-  args = arguments.to_a
+  args = arguments.to_a.compact
   Rake::Task['java:docs'].invoke(*args)
   Rake::Task['py:docs'].invoke(*args)
   Rake::Task['rb:docs'].invoke(*args)
   Rake::Task['dotnet:docs'].invoke(*args)
   Rake::Task['node:docs'].invoke(*args)
   Rake::Task['rust:docs'].invoke(*args)
 end
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that removing .compact can introduce nil arguments, which is a valid concern for robustness, and reverts a change made in the PR.

Medium
Reuse sanitized forwarded arguments

In the build task, sanitize arguments by calling arguments.to_a.compact once and
reuse the result for all invoke calls to prevent nil propagation.

Rakefile [210-216]

 task :build do |_task, arguments|
-  Rake::Task['java:build'].invoke(*arguments.to_a)
-  Rake::Task['py:build'].invoke(*arguments.to_a)
-  Rake::Task['rb:build'].invoke(*arguments.to_a)
-  Rake::Task['dotnet:build'].invoke(*arguments.to_a)
-  Rake::Task['node:build'].invoke(*arguments.to_a)
+  args = arguments.to_a.compact
+  Rake::Task['java:build'].invoke(*args)
+  Rake::Task['py:build'].invoke(*args)
+  Rake::Task['rb:build'].invoke(*args)
+  Rake::Task['dotnet:build'].invoke(*args)
+  Rake::Task['node:build'].invoke(*args)
 end
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential issue with forwarding nil arguments and proposes a good practice of sanitizing and reusing the arguments list, improving both robustness and readability.

Medium
Harden argument parsing for skips

In the lint task, add .compact.map(&:to_s) to arguments.to_a to prevent
NoMethodError from nil values when parsing arguments.

Rakefile [258-262]

 task :lint do |_task, arguments|
   all_langs = %w[java py rb node dotnet rust]
-  skip = arguments.to_a.select { |a| a.start_with?('-') }.map { |a| a.delete_prefix('-') }
+  raw_args = arguments.to_a.compact.map(&:to_s)
+  skip = raw_args.select { |a| a.start_with?('-') }.map { |a| a.delete_prefix('-') }
   invalid = skip - all_langs
   raise "Unknown languages: #{invalid.join(', ')}. Valid: #{all_langs.join(', ')}" if invalid.any?
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that arguments.to_a can contain nil values, which would cause a NoMethodError, and proposes a robust way to handle them.

Medium
Learned
best practice
Harden external JSON parsing

Guard against non-JSON or whitespace-only responses from Sonatype by stripping
the body and rescuing JSON::ParserError to raise a clearer error including the
raw response.

rake_tasks/java.rake [82]

-res.body.to_s.empty? ? {} : JSON.parse(res.body)
+body = res.body.to_s.strip
+return {} if body.empty?
 
+JSON.parse(body)
+
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation/guards at integration boundaries (external HTTP/JSON) to avoid failing with unclear errors on unexpected responses.

Low
Validate parsed credential values

Strip extracted values and only set ENV when a non-empty username/password was
actually parsed to prevent propagating blank/whitespace credentials.

rake_tasks/java.rake [98-102]

 elsif line.include?('<username>')
-  ENV['MAVEN_USER'] = line[%r{<username>(.*?)</username>}, 1]
+  user = line[%r{<username>(.*?)</username>}, 1].to_s.strip
+  ENV['MAVEN_USER'] = user unless user.empty?
 elsif line.include?('<password>')
-  ENV['MAVEN_PASSWORD'] = line[%r{<password>(.*?)</password>}, 1]
+  pass = line[%r{<password>(.*?)</password>}, 1].to_s.strip
+  ENV['MAVEN_PASSWORD'] = pass unless pass.empty?
 end
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Add explicit validation/guards at integration boundaries (environment variables and config files) by trimming and checking presence before use.

Low
✅ Suggestions up to commit fb43c38
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate required release inputs
Suggestion Impact:The commit added a guard clause that raises an explicit error when the version argument is nil or empty, matching the suggested validation to prevent silent failures.

code diff:

   version = arguments[:version]
+  raise 'Missing required version: ./go prep_release[4.31.0,early-stable]' if version.nil? || version.empty?

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 processed]

Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies that the new prep_release task lacks validation for the required version argument, which could lead to silent failures, and proposes adding an explicit check to improve robustness.

Medium
Handle missing settings file safely
Suggestion Impact:The commit introduces settings_path, checks File.exist? before reading settings.xml, emits a warning, and returns early, preventing a crash when the file is missing.

code diff:

+  settings_path = File.join(Dir.home, '.m2', 'settings.xml')
+  unless File.exist?(settings_path)
+    warn "Maven settings file not found at #{settings_path}"
+    return
+  end
+
   puts 'Maven environment variables not set, inspecting ~/.m2/settings.xml.'
-  settings = File.read("#{Dir.home}/.m2/settings.xml")
+  settings = File.read(settings_path)

Check if ~/.m2/settings.xml exists before attempting to read it to prevent the
program from crashing and provide a clear warning if the file is missing.

rake_tasks/java.rake [81-95]

 def read_m2_user_pass
   puts 'Maven environment variables not set, inspecting ~/.m2/settings.xml.'
-  settings = File.read("#{Dir.home}/.m2/settings.xml")
+  settings_path = File.join(Dir.home, '.m2', 'settings.xml')
+  unless File.exist?(settings_path)
+    warn "Maven settings file not found at #{settings_path}"
+    return
+  end
+
+  settings = File.read(settings_path)
   found_section = false
   settings.each_line do |line|
     if !found_section
       found_section = line.include? '<id>central</id>'
     elsif line.include?('<username>')
       ENV['MAVEN_USER'] = line[%r{<username>(.*?)</username>}, 1]
     elsif line.include?('<password>')
       ENV['MAVEN_PASSWORD'] = line[%r{<password>(.*?)</password>}, 1]
     end
     break if ENV['MAVEN_PASSWORD'] && ENV['MAVEN_USER']
   end
 end

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion improves error handling by proactively checking for the existence of the settings.xml file, preventing a crash and providing a more user-friendly warning message.

Medium
Fix linter argument parsing

Refine the argument parsing for the all:lint task to correctly differentiate
between language-skip flags (e.g., -rb) and other command-line options (e.g.,
--config), preventing incorrect behavior.

Rakefile [243-259]

 desc 'Run linters for all languages (skip with: ./go all:lint -rb -rust)'
 task :lint do |_task, arguments|
   all_langs = %w[java py rb node dotnet rust]
-  skip = arguments.to_a.select { |a| a.start_with?('-') }.map { |a| a.delete_prefix('-') }
-  invalid = skip - all_langs
+  args = arguments.to_a
+
+  # Only treat exact `-<lang>` tokens as skip flags; allow other `--foo` args to pass through.
+  skip_tokens, pass_through = args.partition { |a| all_langs.any? { |lang| a == "-#{lang}" } }
+  skip = skip_tokens.map { |a| a.delete_prefix('-') }
+
+  invalid = (args.grep(/\A-/).map { |a| a.delete_prefix('-') } - pass_through.grep(/\A--/).map { |a| a.delete_prefix('-') } - all_langs)
   raise "Unknown languages: #{invalid.join(', ')}. Valid: #{all_langs.join(', ')}" if invalid.any?
 
   langs = all_langs - skip
   failures = []
   langs.each do |lang|
     puts "Linting #{lang}..."
-    Rake::Task["#{lang}:lint"].invoke
+    Rake::Task["#{lang}:lint"].invoke(*pass_through)
   rescue StandardError => e
     failures << "#{lang}: #{e.message}"
   end
   raise "Lint failed:\n#{failures.join("\n")}" unless failures.empty?
 end
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a bug in argument parsing for the new all:lint task, where any flag starting with - is treated as a language to skip, and proposes a more robust implementation.

Medium
Prevent undefined response usage

Initialize response to nil before the redirect-following loop to prevent a
potential NameError and use the safe navigation operator when accessing its
properties.

rake_tasks/ruby.rake [167-181]

 to_download.each do |key|
   uri = URI("https://rubygems.org/gems/#{key}.gem")
 
+  response = nil
   5.times do
     response = Net::HTTP.get_response(uri)
     break unless response.is_a?(Net::HTTPRedirection)
 
     uri = URI(response['location'])
   end
 
   unless response.is_a?(Net::HTTPSuccess)
-    puts "  #{key}: failed (HTTP #{response.code})"
+    code = response&.code || 'no response'
+    puts "  #{key}: failed (HTTP #{code})"
     failed << key
     next
   end
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a potential NameError if the loop were not to execute, and the proposed change with pre-initialization and safe navigation makes the code more robust against unexpected control flow.

Low
Learned
best practice
Validate URL before HTTP call

Validate that url is a non-empty HTTP(S) URL before calling URI() to avoid
unexpected crashes and ambiguous errors.

rake_tasks/common.rb [68-75]

 def self.verify_package_published(url)
+  url = url.to_s.strip
+  raise 'Package URL must be provided' if url.empty?
+
+  uri = URI(url)
+  raise "Unsupported URL scheme: #{uri.scheme}" unless %w[http https].include?(uri.scheme)
+
   puts "Verifying #{url}..."
-  uri = URI(url)
-  res = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https') { |http| http.request(Net::HTTP::Get.new(uri)) }
+  res = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https') do |http|
+    http.request(Net::HTTP::Get.new(uri))
+  end
   raise "Package not published: #{url}" unless res.is_a?(Net::HTTPSuccess)
 
   puts 'Verified!'
 end
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation/availability guards at integration boundaries (e.g., URLs) before use.

Low
Security
Restrict artifact file permissions

Change the file permissions for release artifacts from world-writable (0o666) to
a more secure 0o644 to prevent unintended modifications.

rake_tasks/dotnet.rake [21-24]

 FileUtils.copy('bazel-bin/dotnet/release.zip', "build/dist/selenium-dotnet-#{dotnet_version}.zip")
-FileUtils.chmod(0o666, "build/dist/selenium-dotnet-#{dotnet_version}.zip")
+FileUtils.chmod(0o644, "build/dist/selenium-dotnet-#{dotnet_version}.zip")
 FileUtils.copy('bazel-bin/dotnet/strongnamed.zip', "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")
-FileUtils.chmod(0o666, "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")
+FileUtils.chmod(0o644, "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that world-writable permissions (0o666) are too permissive for release artifacts and recommends a more secure default (0o644), which is a good security practice.

Low
✅ Suggestions up to commit 5d81dcf
CategorySuggestion                                                                                                                                    Impact
Possible issue
Swap target diff logic

Correct the inverted logic for identifying missing and extra release targets by
swapping the operands in the array subtraction operations.

rake_tasks/java.rake [45-65]

 def verify_java_release_targets
-  query = 'kind(maven_publish, set(//java/... //third_party/...))'
-  current_targets = []
-
-  Bazel.execute('query', [], query) do |output|
-    current_targets = output.lines.map(&:strip).reject(&:empty?).select { |line| line.start_with?('//') }
-  end
-
-  missing_targets = current_targets - JAVA_RELEASE_TARGETS
-  extra_targets = JAVA_RELEASE_TARGETS - current_targets
-
-  return if missing_targets.empty? && extra_targets.empty?
-
-  error_message = 'Java release targets are out of sync with Bazel query results.'
-
-  error_message += "\nMissing targets: #{missing_targets.join(', ')}" unless missing_targets.empty?
-
-  error_message += "\nObsolete targets: #{extra_targets.join(', ')}" unless extra_targets.empty?
-
-  raise error_message
+  # ...
+  missing_targets = JAVA_RELEASE_TARGETS - current_targets
+  extra_targets   = current_targets - JAVA_RELEASE_TARGETS
+  # ...
 end
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a logical flaw in the target verification logic, which would report incorrect results and cause confusion. Fixing this is critical for the task's correctness.

High
Separate dry-run flag into args

Pass the --dry-run=true flag as a separate element in the arguments array for
the Bazel.execute call to ensure it is correctly interpreted.

rake_tasks/node.rake [72-74]

 target = '//javascript/selenium-webdriver:selenium-webdriver.publish'
-target += ' -- --dry-run=true' if dry_run
-Bazel.execute('run', ['--config=release'], target)
+args = ['--config=release']
+if dry_run
+  args += ['--', '--dry-run=true']
+end
+Bazel.execute('run', args, target)
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that command-line arguments are being incorrectly concatenated into the target string, which would cause the dry-run flag to be ignored, leading to an unintended release.

Medium
Fix incorrect linter failure handling

Remove the begin...rescue block around the Rake::Task['all:lint'].invoke call to
ensure the main lint task aborts immediately if any language-specific linter
fails.

Rakefile [121-153]

 task :lint do |_task, arguments|
   failures = []
 
-  begin
-    Rake::Task['all:lint'].invoke(*arguments.to_a)
-  rescue StandardError => e
-    failures << e.message
-  end
+  Rake::Task['all:lint'].invoke(*arguments.to_a)
 
   puts 'Linting Bazel files...'
   begin
     Bazel.execute('run', [], '//:buildifier')
   rescue StandardError => e
     failures << "buildifier: #{e.message}"
   end
 
   puts 'Linting shell scripts and GitHub Actions...'
   begin
     shellcheck = Bazel.execute('build', [], '@multitool//tools/shellcheck')
     Bazel.execute('run', ['--', '-shellcheck', shellcheck], '@multitool//tools/actionlint:cwd')
   rescue StandardError => e
     failures << "shellcheck/actionlint: #{e.message}"
   end
 
   puts 'Updating copyright headers...'
   begin
     Bazel.execute('run', [], '//scripts:update_copyright')
   rescue StandardError => e
     failures << "copyright: #{e.message}"
   end
 
   raise "Lint failed:\n#{failures.join("\n")}" unless failures.empty?
 end
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the lint task should fail fast if the all:lint sub-task fails, improving the task's logic and providing quicker feedback.

Medium
General
Fix memoization of target verification

Fix the memoization in java_release_targets by explicitly setting
@targets_verified to true after a successful verification, preventing redundant
checks.

rake_tasks/java.rake [39-43]

 def java_release_targets
-  @targets_verified ||= verify_java_release_targets
-
+  unless @targets_verified
+    verify_java_release_targets
+    @targets_verified = true
+  end
   JAVA_RELEASE_TARGETS
 end
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug in the memoization logic that causes an expensive verification process to ...