[build] Split Rakefile into per-language task files by titusfortner · Pull Request #16979 · SeleniumHQ/selenium
PR Code Suggestions ✨
Latest suggestions up to 40108b9
| Category | Suggestion | Impact |
| Possible issue |
Don’t misparse non-language flagsFix the argument parsing in the 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
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 | Medium |
Prevent nil version resultsAdd input validation and explicit error handling to the 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
Suggestion importance[1-10]: 7__ Why: The suggestion correctly identifies a scenario where the | Medium | |
Validate lockfile and checksum blockAdd checks to the 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)
Suggestion importance[1-10]: 7__ Why: The suggestion correctly points out that parsing external files without validation can lead to | Medium | |
Prevent double-loading task filesAvoid reloading the same Rake file for namespace aliases. Instead, load each +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') } +
Suggestion importance[1-10]: 7__ Why: The suggestion correctly identifies that | Medium | |
Make wheel install deterministicModify the 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
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 | Low | |
| General |
Ensure lint tasks always runEnsure language-specific lint tasks can be re-run within the same process by 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
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 | Low |
| Learned best practice |
Fail fast on missing versionIf 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
Suggestion importance[1-10]: 6__ Why: | Low |
Reenable tasks invoked twiceRake tasks only run once per process, so the second 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
Suggestion importance[1-10]: 5__ Why: | Low | |
| ||
Previous suggestions
✅ Suggestions up to commit ac29f0c
| Category | Suggestion | Impact |
| Possible issue |
Fix retry-loop exception handlingFix a syntax error in the retry loop's exception handling by wrapping the loop's 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
endSuggestion importance[1-10]: 10__ Why: The suggestion correctly identifies a syntax error in the | High |
Parse JSON for package versionRobustly extract the package version by parsing 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 | Medium | |
Ensure credential checks rerunUse 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 | Medium | |
✅
| Low | |
| General |
Add timeouts to HTTP checksAdd 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!'
endSuggestion importance[1-10]: 7__ Why: The suggestion improves the robustness of the | Medium |
| Learned best practice |
Harden API calls and parsingUse 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: | Low |
✅ Suggestions up to commit a2ac5b7
| Category | Suggestion | Impact |
| Incremental [*] |
Normalize task arguments to stringsConvert Rake task arguments to strings using 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 | Medium |
Use absolute path for loadingMake the -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 | Low | |
| Possible issue |
Avoid reading wrong Maven credentialsImprove the parsing of 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
endSuggestion importance[1-10]: 7__ Why: The suggestion correctly identifies a potential bug where incorrect credentials could be parsed from | Medium |
Avoid hard dependency on gitWrap the -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 | Medium | |
Make git staging best-effortUse the safe navigation operator ( -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 | Medium | |
Handle missing outdated command outputAdd a check to ensure the 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_hSuggestion importance[1-10]: 6__ Why: The suggestion correctly adds a guard clause to prevent a | Low | |
Make local file copying robustMake the local file copying more robust by creating the destination directory if 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 |
✅
| Low |
✅ Suggestions up to commit 08841ce
| Category | Suggestion | Impact |
| Security |
✅
| Medium |
Avoid shelling out unsafelyReplace the backtick shell execution of -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 | Medium | |
| Possible issue |
✅
| Medium |
Avoid forwarding nil argumentsAdd 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 | Medium | |
Reuse sanitized forwarded argumentsIn the 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 | Medium | |
Harden argument parsing for skipsIn the 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 | Medium | |
| Learned best practice |
Harden external JSON parsingGuard against non-JSON or whitespace-only responses from Sonatype by stripping -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: | Low |
Validate parsed credential valuesStrip extracted values and only set 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?
endSuggestion importance[1-10]: 5__ Why: | Low | |
✅ Suggestions up to commit fb43c38
| Category | Suggestion | Impact |
| Possible issue |
✅
| Medium |
✅
| Medium | |
Fix linter argument parsingRefine the argument parsing for the 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 | Medium | |
Prevent undefined response usageInitialize 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
endSuggestion importance[1-10]: 4__ Why: The suggestion correctly identifies a potential | Low | |
| Learned best practice |
Validate URL before HTTP callValidate that 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: | Low |
| Security |
Restrict artifact file permissionsChange the file permissions for release artifacts from world-writable ( 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 ( | Low |
✅ Suggestions up to commit 5d81dcf
| Category | Suggestion | Impact |
| Possible issue |
Swap target diff logicCorrect the inverted logic for identifying missing and extra release targets by 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 argsPass the 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 | Medium | |
Fix incorrect linter failure handlingRemove the 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 | Medium | |
| General |
Fix memoization of target verificationFix the memoization in 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 ... |