[build] fix lint problems automatically

[titusfortner]

User description

💥 What does this PR do?

• Changes the ci-rbe job to automatically commit the formatting fix instead of requiring contributors to update it themselves
• Cancel-in-progress should mean that everything stops running and the new runs start

🔧 Implementation Notes

We could make everything wait for format to be done first, it might even be more performant, but this was easier for now.

This PR shows how it works.

I committed a badly formatted ruby file.
The Formatter run and failed, so the Commit Fixes job ran
https://github.com/SeleniumHQ/selenium/actions/runs/20449690944

New commit reran the tests, and formatter passed and commit fixes was skipped:
https://github.com/SeleniumHQ/selenium/actions/runs/20449865466

---

PR Type

Enhancement

---

Description

• Automatically commit formatting fixes to PRs instead of failing

• Add new commit-fixes job that applies patch and pushes changes

• Download format changes artifact from format job

• Handle same-repo vs fork PRs with different strategies

---

Diagram Walkthrough

flowchart LR
  format["Format Job<br/>Check format script"] -->|"artifact: format-changes"| commit["Commit Fixes Job<br/>Apply patch & push"]
  commit -->|"same-repo PR"| push["Auto-commit & push"]
  commit -->|"fork PR"| error["Error with diff output"]

Loading

File Walkthrough

Relevant files

Enhancement

ci-rbe.yml Add automatic formatting fix commit workflow                          .github/workflows/ci-rbe.yml Added artifact-name: format-changes parameter to format job to export patch file Created new commit-fixes job that runs after format job on failure Downloads format changes artifact and applies patch using git apply Automatically commits and pushes fixes for same-repo PRs, shows error for forks Includes conditional logic to skip empty patches and handle different PR types +39/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	CI token privilege escalation Description: The workflow checks out and executes repository code (./scripts/format.sh) while potentially providing a high-privilege secret token (secrets.SELENIUM_CI_TOKEN) to the job, which can enable a privilege-escalation path where a same-repo PR author modifies scripts to exfiltrate/use the token (e.g., pushing arbitrary commits/tags or performing other API actions beyond the PR author's privileges). ci-rbe.yml [24-54] Referred Code runs-on: ubuntu-latest steps: - name: Checkout source tree uses: actions/checkout@v4 with: ref: ${{ github.head_ref || github.ref }} token: ${{ secrets.SELENIUM_CI_TOKEN || secrets.GITHUB_TOKEN }} - name: Run format script run: ./scripts/format.sh - name: Commit fixes and fail run: | if [ -z "$(git status --porcelain)" ]; then echo "No formatting changes needed" exit 0 fi # For same-repo PRs, commit the fix if [ "${{ github.event_name }}" = "pull_request" ] && \ [ "${{ github.event.pull_request.head.repo.full_name }}" = "${{ github.repository }}" ]; then git config --local user.name "Selenium CI Bot" ... (clipped 10 lines)
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: 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: 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
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing failure checks: The bash auto-commit logic does not robustly handle failures (e.g., git commit/git push errors) and can emit a success notice without verifying push success before failing the job. Referred Code - name: Commit fixes and fail run: | if [ -z "$(git status --porcelain)" ]; then echo "No formatting changes needed" exit 0 fi # For same-repo PRs, commit the fix if [ "${{ github.event_name }}" = "pull_request" ] && \ [ "${{ github.event.pull_request.head.repo.full_name }}" = "${{ github.repository }}" ]; then git config --local user.name "Selenium CI Bot" git config --local user.email "[email protected]" git add -A git commit -m "Auto-format code" git push echo "::notice::Auto-formatted and pushed. New CI run will start." else echo "::error::Code needs formatting. Run ./scripts/format.sh locally." git diff fi exit 1 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: Logs may expose data: Printing git diff to CI logs could unintentionally expose sensitive content if any secret/credential ever ends up in a tracked file or formatting changes touch sensitive material. Referred Code echo "::error::Code needs formatting. Run ./scripts/format.sh locally." git diff fi Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

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

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ fix exit code for push branch Suggestion Impact: The committed changes remove the unconditional failing `exit 1` at the end of the step and add `exit 1` inside the "needs formatting" else-branch, so the job no longer fails after a successful same-repo auto-fix push. The commit did not add an explicit `exit 0` on the push-success path, but by removing the final `exit 1` it still allows the step to exit successfully by default. code diff: + - name: Apply and commit fixes run: | - if [ -z "$(git status --porcelain)" ]; then + if [ ! -s changes.patch ]; then echo "No formatting changes needed" exit 0 fi + git apply changes.patch + rm changes.patch # For same-repo PRs, commit the fix if [ "${{ github.event_name }}" = "pull_request" ] && \ [ "${{ github.event.pull_request.head.repo.full_name }}" = "${{ github.repository }}" ]; then @@ -50,8 +64,8 @@ else echo "::error::Code needs formatting. Run ./scripts/format.sh locally." git diff + exit 1 fi - exit 1 Modify the script's exit logic to ensure the job passes after a successful auto-format push. Move exit 1 into the else block and add exit 0 to the success path. .github/workflows/ci-rbe.yml [42-54] if [ "${{ github.event_name }}" = "pull_request" ] && \ [ "${{ github.event.pull_request.head.repo.full_name }}" = "${{ github.repository }}" ]; then git config --local user.name "Selenium CI Bot" git config --local user.email "[email protected]" git add -A git commit -m "Auto-format code" git push echo "::notice::Auto-formatted and pushed. New CI run will start." + exit 0 else echo "::error::Code needs formatting. Run ./scripts/format.sh locally." git diff + exit 1 fi -exit 1 [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical bug in the workflow logic where the job would fail even after successfully auto-formatting and pushing a fix. The fix is correct and essential for the new workflow to function as intended.	High
High-level	✅ CI workflow design is inefficient Suggestion Impact: The workflow was restructured to run formatting as a separate reusable-workflow job and then gate the follow-up "commit fixes" job with `needs: format` (only running it when the format job fails). This introduces job dependency ordering around formatting, aligning with the goal of preventing downstream work from running until formatting completes, though the diff does not explicitly show test jobs being updated to `needs: format`. code diff: @@ -21,6 +21,18 @@ format: name: Format if: github.repository_owner == 'seleniumhq' && startsWith(github.head_ref, 'renovate/') != true + uses: ./.github/workflows/bazel.yml + with: + name: Check format script run + caching: false + ruby-version: jruby-10.0.0.0 + artifact-name: format-changes + run: ./scripts/github-actions/check-format.sh + + commit-fixes: + name: Commit fixes + needs: format + if: always() && needs.format.result == 'failure' && github.repository_owner == 'seleniumhq' && startsWith(github.head_ref, 'renovate/') != true The CI workflow is inefficient as it runs test jobs concurrently with the formatting job, causing wasted resources when auto-formatting triggers a new CI run. It is recommended to make the test jobs dependent on the successful completion of the format job. Examples: .github/workflows/ci-rbe.yml [21-64] format: name: Format if: github.repository_owner == 'seleniumhq' && startsWith(github.head_ref, 'renovate/') != true runs-on: ubuntu-latest steps: - name: Checkout source tree uses: actions/checkout@v4 with: ref: ${{ github.head_ref || github.ref }} token: ${{ secrets.SELENIUM_CI_TOKEN || secrets.GITHUB_TOKEN }} ... (clipped 34 lines) Solution Walkthrough: Before: jobs: format: # ... steps: - run: ./scripts/format.sh - run: | if [ ...changes... ]; then git commit ... git push # The push triggers a new run, cancelling this one. fi exit 1 test: # This job starts in parallel with 'format' and gets cancelled # if 'format' pushes a change, wasting resources. uses: ./.github/workflows/bazel.yml # ... After: jobs: format: # ... steps: - run: ./scripts/format.sh - run: | if [ ...changes... ]; then git commit ... git push exit 1 # Fail job to prevent 'test' from running in this workflow fi # Succeeds if no changes test: needs: format # This ensures 'test' only runs after 'format' succeeds. uses: ./.github/workflows/bazel.yml # ... Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant inefficiency in the CI workflow where the test job starts in parallel with the format job, leading to wasted resources when the latter pushes a commit and cancels the run.	Medium
General	enable full-depth checkout Add fetch-depth: 0 to the checkout step to ensure a full Git history is available, which is necessary for the git push command to succeed reliably. .github/workflows/ci-rbe.yml [26-30] - name: Checkout source tree uses: actions/checkout@v4 with: + fetch-depth: 0 ref: ${{ github.head_ref || github.ref }} token: ${{ secrets.SELENIUM_CI_TOKEN || secrets.GITHUB_TOKEN }} Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that a shallow clone (the default for actions/checkout) can cause the git push operation to fail. Adding fetch-depth: 0 is crucial for the reliability of the auto-formatting feature.	Medium
Update

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Workflow token misuse Description: The new commit-fixes job checks out PR code using a write-capable token (secrets.SELENIUM_CI_TOKEN || secrets.GITHUB_TOKEN) and then executes shell commands that apply a patch artifact and perform git push, which could be abused to perform unintended repository writes or token misuse if an attacker can influence the patch/artifact or the executed scripts in same-repo PR contexts (e.g., by modifying formatting scripts to generate arbitrary changes.patch that edits sensitive files and gets auto-committed). ci-rbe.yml [32-68] Referred Code commit-fixes: name: Commit fixes needs: format if: always() && needs.format.result == 'failure' && github.repository_owner == 'seleniumhq' && startsWith(github.head_ref, 'renovate/') != true runs-on: ubuntu-latest steps: - name: Checkout source tree uses: actions/checkout@v4 with: ref: ${{ github.head_ref || github.ref }} token: ${{ secrets.SELENIUM_CI_TOKEN || secrets.GITHUB_TOKEN }} - name: Download format changes uses: actions/download-artifact@v4 with: name: format-changes - name: Apply and commit fixes run: | if [ ! -s changes.patch ]; then echo "No formatting changes needed" exit 0 fi ... (clipped 16 lines)
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: Unhandled command failures: The new Apply and commit fixes step does not robustly handle failure cases for git apply, git commit, or git push (no explicit checks or set -e), which can lead to unclear/partial failures and non-actionable outcomes. Referred Code run: | if [ ! -s changes.patch ]; then echo "No formatting changes needed" exit 0 fi git apply changes.patch rm changes.patch # For same-repo PRs, commit the fix if [ "${{ github.event_name }}" = "pull_request" ] && \ [ "${{ github.event.pull_request.head.repo.full_name }}" = "${{ github.repository }}" ]; then git config --local user.name "Selenium CI Bot" git config --local user.email "[email protected]" git add -A git commit -m "Auto-format code" git push echo "::notice::Auto-formatted and pushed. New CI run will start." else echo "::error::Code needs formatting. Run ./scripts/format.sh locally." git diff fi exit 1 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: Diff exposed on error: On fork PRs the workflow prints git diff to logs after applying the patch, which could expose sensitive content if formatting changes touch confidential material (cannot be confirmed from the diff alone). Referred Code else echo "::error::Code needs formatting. Run ./scripts/format.sh locally." git diff fi 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: Unstructured diff logging: The workflow logs a raw git diff to GitHub Actions output which is unstructured and may leak sensitive data if present in the changed content. Referred Code else echo "::error::Code needs formatting. Run ./scripts/format.sh locally." git diff fi 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: Applies external patch: The job downloads and applies changes.patch from an artifact without validating its provenance/content beyond file existence, which may be risky if the artifact can be influenced by untrusted PR code (context not fully verifiable from this diff). Referred Code - name: Download format changes uses: actions/download-artifact@v4 with: name: format-changes - name: Apply and commit fixes run: | if [ ! -s changes.patch ]; then echo "No formatting changes needed" exit 0 fi git apply changes.patch rm changes.patch # For same-repo PRs, commit the fix 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]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Ensure full git history for push Add fetch-depth: 0 to the actions/checkout step to ensure the git push command has the full git history and can execute successfully. .github/workflows/ci-rbe.yml [38-42] - name: Checkout source tree uses: actions/checkout@v4 with: ref: ${{ github.head_ref || github.ref }} token: ${{ secrets.SELENIUM_CI_TOKEN || secrets.GITHUB_TOKEN }} + fetch-depth: 0 Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies that git push requires the full git history, which is not fetched by default, and fetch-depth: 0 is the necessary fix to prevent the push from failing.	High
High-level	Use a GitHub App for auto-commits Replace the Personal Access Token (PAT) used for auto-commits with a more secure, narrowly-scoped GitHub App. This aligns with security best practices and minimizes risks from token exposure. Examples: .github/workflows/ci-rbe.yml [42] token: ${{ secrets.SELENIUM_CI_TOKEN || secrets.GITHUB_TOKEN }} Solution Walkthrough: Before: # .github/workflows/ci-rbe.yml commit-fixes: steps: - name: Checkout source tree uses: actions/checkout@v4 with: ref: ${{ github.head_ref }} token: ${{ secrets.SELENIUM_CI_TOKEN || secrets.GITHUB_TOKEN }} - name: Apply and commit fixes run: | ... git apply changes.patch ... git commit -m "Auto-format code" git push After: # .github/workflows/ci-rbe.yml commit-fixes: steps: - name: Generate GitHub App token id: generate_token uses: actions/create-github-app-token@v1 with: app_id: ${{ secrets.YOUR_APP_ID }} private_key: ${{ secrets.YOUR_APP_PRIVATE_KEY }} - name: Checkout source tree uses: actions/checkout@v4 with: ref: ${{ github.head_ref }} token: ${{ steps.generate_token.outputs.token }} - name: Apply and commit fixes ... Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant security risk with using a PAT for commits and proposes the industry best-practice solution of using a GitHub App, which greatly improves repository security.	Medium
Possible issue	Add error handling to script Add set -e to the beginning of the run script in the Apply and commit fixes step to ensure it exits immediately if any command fails, preventing unexpected behavior. .github/workflows/ci-rbe.yml [47-68] - name: Apply and commit fixes run: | + set -e if [ ! -s changes.patch ]; then echo "No formatting changes needed" exit 0 fi git apply changes.patch rm changes.patch # For same-repo PRs, commit the fix if [ "${{ github.event_name }}" = "pull_request" ] && \ [ "${{ github.event.pull_request.head.repo.full_name }}" = "${{ github.repository }}" ]; then git config --local user.name "Selenium CI Bot" git config --local user.email "[email protected]" git add -A git commit -m "Auto-format code" git push echo "::notice::Auto-formatted and pushed. New CI run will start." else echo "::error::Code needs formatting. Run ./scripts/format.sh locally." git diff fi exit 1 Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue where the script would continue after git apply fails, and set -e is the right solution to make the script more robust by failing immediately.	Medium
More

[navin772]

Does the auto formatting only works if we are making a PR from this repo's branch only? Anyone else who makes a PR from their fork won't benefit from this change.
I don't think we can commit to a fork using github CI, so this seems like the only option.

[titusfortner]

Actually, good point, there's a checkbox to allow maintainers to edit on forks. Let me support that. Check out the update.

[navin772]

I don't know if its true but if I ask AI, it says this won't work - using github.event.pull_request.maintainer_can_modify == true does not grant GitHub Actions any extra rights.

And it advises against this practice due to security issues (fork case only). We should first check and validate this.