Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Jan 24, 2026

User description

2 clever ideas conflicting.

I've been using explicit git add commands in Rakefiles to ensure only files we cared about were
committed. Then I moved the actual commit logic from rakefiles to CI (better separation of concerns)
Recently we switched to using git diff in our bazel workflow to store a patch that can be used by
calling workflows. Except if you stage a file it's no longer in the diff.

💥 What does this PR do?

Removes all SeleniumRake.git.add() calls from rake tasks.

Files modified by rake tasks are now left unstaged, letting users review and stage changes explicitly.

Removed 36 lines across 8 files:

  • Rakefile - update_browsers, update_manager, update_multitool, update_cdp, authors, all:version
  • rake_tasks/java.rake - pin, version
  • rake_tasks/python.rake - version
  • rake_tasks/ruby.rake - version, pin, update
  • rake_tasks/rust.rake - version
  • rake_tasks/node.rake - pin, version
  • rake_tasks/dotnet.rake - version, pin
  • rake_tasks/common.rb - update_changelog

🔧 Implementation Notes

The only things that I know that can sully the working directory are the version changes we make to python/ruby/node,
but those should be limited to just unit tests so won't matter for these operations.

There are a bunch of ways this could have gone, but I think trying to control which exact files is... excessive at this point.

💡 Additional Considerations

The git gem is still used for read-only operations (listing tags for changelog generation).

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Remove all SeleniumRake.git.add() calls from rake tasks

  • Allow users to explicitly stage modified files

  • Enable git diff workflow for patch generation

  • Improve separation of concerns between build and CI


Diagram Walkthrough

flowchart LR
  A["Rake Tasks"] -->|"Previously auto-staged files"| B["Git Index"]
  A -->|"Now leaves files unstaged"| C["Working Directory"]
  C -->|"User explicitly stages"| B
  B -->|"Enables git diff workflow"| D["Patch Generation"]
Loading

File Walkthrough

Relevant files
Cleanup
common.rb
Remove git.add() from changelog update                                     

rake_tasks/common.rb

  • Removed git.add(changelog) call from update_changelog method
  • Changelog file now left unstaged after updates
+0/-1     
dotnet.rake
Remove git.add() from .NET tasks                                                 

rake_tasks/dotnet.rake

  • Removed SeleniumRake.git.add(file) from version task
  • Removed git.add() calls for paket.lock and paket.nuget.bzl in pin task
  • .NET version and dependency files now left unstaged
+0/-2     
java.rake
Remove git.add() from Java tasks                                                 

rake_tasks/java.rake

  • Removed git.add() calls for MODULE.bazel and maven_install.json in pin
    task
  • Removed git.add(file) from version task
  • Java version and Maven dependency files now left unstaged
+0/-2     
node.rake
Remove git.add() from Node tasks                                                 

rake_tasks/node.rake

  • Removed SeleniumRake.git.add('pnpm-lock.yaml') from pin task
  • Removed git.add() calls from version task for package.json and
    BUILD.bazel
  • Node package and lockfile changes now left unstaged
+0/-2     
python.rake
Remove git.add() from Python tasks                                             

rake_tasks/python.rake

  • Removed git.add(file) calls from version task for multiple Python
    files
  • Removed git.add(conf) from version task for conf.py
  • Python version files now left unstaged
+0/-2     
ruby.rake
Remove git.add() from Ruby tasks                                                 

rake_tasks/ruby.rake

  • Removed git.add(file) from version task
  • Removed git.add() calls from pin task for MODULE.bazel
  • Removed git.add() calls from update task for Gemfile.lock and
    rbs_collection.lock.yaml
  • Ruby version and dependency files now left unstaged
+0/-5     
rust.rake
Remove git.add() from Rust tasks                                                 

rake_tasks/rust.rake

  • Removed git.add(file) calls from version task for Cargo.toml and
    BUILD.bazel
  • Removed git.add() calls for Cargo.Bazel.lock and Cargo.lock
  • Rust version and lock files now left unstaged
+0/-3     
Rakefile
Remove git.add() from main Rakefile tasks                               

Rakefile

  • Removed git.add('common/repositories.bzl') from update_browsers task
  • Removed git.add('common/selenium_manager.bzl') from update_manager
    task
  • Removed git.add('multitool.lock.json') from update_multitool task
  • Removed large block of git.add() calls from update_cdp task (14 files)
  • Removed git.add('AUTHORS') from authors task
  • Removed git.add(file) from all:version task
  • All modified files now left unstaged for explicit user staging
+0/-19   

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Jan 24, 2026
@titusfortner titusfortner requested a review from Copilot January 24, 2026 06:05
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid modifying the same file twice

Refactor the code to avoid reading and writing to py/docs/source/conf.py twice
by combining the two separate version string replacements into a single
operation.

rake_tasks/python.rake [141-151]

-   'py/docs/source/conf.py'].each do |file|2
+   'py/README.md'].each do |file|2
     text = File.read(file).gsub(old_version, new_version)
     File.open(file, 'w') { |f| f.puts text }
   end
 
   old_short_version = old_version.split('.')[0..1].join('.')
   new_short_version = new_version.split('.')[0..1].join('.')
 
   conf = 'py/docs/source/conf.py'
-  text = File.read(conf).gsub(old_short_version, new_short_version)
+  text = File.read(conf).gsub(old_version, new_version).gsub(old_short_version, new_short_version)
   File.open(conf, 'w') { |f| f.puts text }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that a file is being read and written twice, and proposes a valid refactoring to combine these operations, which improves both performance and code clarity.

Low
  • More

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes implicit staging (SeleniumRake.git.add) from Rake tasks so task-generated changes remain unstaged, allowing workflows that capture git diff (e.g., git diff --binary) to reliably produce patches.

Changes:

  • Removed all SeleniumRake.git.add(...) calls across Rake tasks (version bumps, pin/update tasks, AUTHORS/CDP updates).
  • Updated shared changelog update helper to stop auto-staging changelog files.
  • Ensured task output is reviewable/stageable by the user (or CI) without relying on the index.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
Rakefile Stops auto-staging outputs from top-level update/version-related tasks.
rake_tasks/common.rb Prevents changelog updates from implicitly staging the changelog file.
rake_tasks/java.rake Removes staging from Maven pin/version tasks.
rake_tasks/node.rake Removes staging from pnpm lockfile pin and JS version bump tasks.
rake_tasks/python.rake Removes staging from Python version bump task (including docs config updates).
rake_tasks/ruby.rake Removes staging from Ruby version/pin/update tasks.
rake_tasks/rust.rake Removes staging from Rust version task (including lockfile outputs).
rake_tasks/dotnet.rake Removes staging from .NET version/pin tasks.

@titusfortner titusfortner merged commit c6f0f45 into trunk Jan 24, 2026
28 checks passed
@titusfortner titusfortner deleted the git-add branch January 24, 2026 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations Review effort 1/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants