[build] Migrate workflows to use centralized bazel.yml by titusfortner · Pull Request #16955 · SeleniumHQ/selenium

PR Code Suggestions ✨

Latest suggestions up to c819f09

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid failing on missing artifacts

In the commit-fixes job, make the download-artifact step non-fatal by adding
continue-on-error: true and adjust the subsequent step to handle cases where the
artifact is missing. This prevents masking the original failure cause from the
format job.

.github/workflows/ci-lint.yml [56-95]

 commit-fixes:
   name: Commit fixes
   needs: format
   if: failure() && github.event_name == 'pull_request'
   runs-on: ubuntu-latest
   permissions:
     contents: write
     actions: read
   steps:
     - name: Check Permissions
       if: github.event.pull_request.head.repo.fork == true
       run: |
         echo "::error::Code needs formatting. Run ./scripts/format.sh locally and push changes."
         exit 1
     - name: Checkout PR
       uses: actions/checkout@v4
       with:
         ref: ${{ github.event.pull_request.head.ref }}
     - name: Download format changes
       uses: actions/download-artifact@v4
+      continue-on-error: true
       with:
         name: format-changes
     - name: Apply and commit fixes
       id: apply
       run: |
-        if [ -s changes.patch ]; then
+        if [ -f changes.patch ] && [ -s changes.patch ]; then
           git apply --index changes.patch
           rm changes.patch
           git config --local user.name "Selenium CI Bot"
           git config --local user.email "selenium-ci@users.noreply.github.com"
           git commit -m "Auto-format code"
           echo "has_changes=true" >> "$GITHUB_OUTPUT"
         else
-          echo "::notice::No formatting changes needed."
+          echo "::notice::No format patch artifact found; skipping auto-commit."
+          echo "has_changes=false" >> "$GITHUB_OUTPUT"
         fi
     - name: Push fixes
       if: steps.apply.outputs.has_changes == 'true'
       run: |
         git push
         echo "::notice::Auto-formatted and pushed. New CI run will start."
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the commit-fixes job could fail and mask the root cause if the format job fails for reasons other than formatting, thus not producing the expected artifact. Adding continue-on-error makes the workflow more robust and improves debuggability.

Medium
Upload only when files exist

In the bazel.yml workflow, modify the if condition for the Upload artifact step
to use hashFiles to ensure an artifact is only uploaded if the specified
artifact-path contains files or if changes.patch exists and is not empty.

.github/workflows/bazel.yml [257-264]

 - name: Upload artifact
-  if: always() && inputs.artifact-name != ''
+  if: |
+    always() && inputs.artifact-name != '' && (
+      (inputs.artifact-path != '' && hashFiles(inputs.artifact-path) != '') ||
+      (inputs.artifact-path == '' && hashFiles('changes.patch') != '')
+    )
   uses: actions/upload-artifact@v5
   with:
     name: ${{ inputs.artifact-name }}
     path: ${{ inputs.artifact-path || 'changes.patch' }}
     retention-days: 6
     if-no-files-found: ${{ inputs.artifact-path != '' && 'error' || 'warn' }}
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential failure point where a downstream job might fail if an expected artifact is not created. Conditionally running the upload step based on file existence makes the reusable workflow more robust and prevents downstream failures.

Medium
Incremental [*]
Include untracked files in patch

Before running git diff, stage untracked files using git add -N . to ensure they
are included in the generated changes.patch.

.github/workflows/bazel.yml [256]

-run: git diff --binary > changes.patch
+run: |
+  git add -N .
+  git diff --binary > changes.patch
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that git diff misses untracked files and proposes using git add -N . to include them in the patch, which prevents changes from being silently lost.

Medium
Avoid empty commits after patch

After applying a patch with git apply --index, check for staged changes using
git diff --cached --quiet before attempting to commit, to avoid errors from
empty commits.

.github/workflows/ci-renovate-rbe.yml [58-66]

 if [ -f changes.patch ] && [ -s changes.patch ]; then
   git apply --index changes.patch
+  if git diff --cached --quiet; then
+    echo "No changes to commit"
+    exit 0
+  fi
   git config --local user.email "selenium-ci@users.noreply.github.com"
   git config --local user.name "Selenium CI Bot"
   git commit -m 'Repin dependencies'
   git push
 else
   echo "No changes to commit"
 fi
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that git apply might not produce changes, which would cause git commit to fail, and proposes a robust check using git diff --cached --quiet to prevent the error.

Medium
  • More

Previous suggestions

✅ Suggestions up to commit 62af24c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix artifact path nesting
Suggestion Impact:The commit removed the explicit `path: docs/api` from the artifact download step, which effectively causes the artifact to be downloaded to the default workspace directory (equivalent to `path: .`), addressing the nested directory issue.

code diff:

@@ -95,7 +95,6 @@
         uses: actions/download-artifact@v4
         with:
           name: documentation
-          path: docs/api
       - name: Setup git

In .github/workflows/update-documentation.yml, change the download path for the
documentation artifact from docs/api to . to prevent creating a nested and
incorrect directory structure.

.github/workflows/update-documentation.yml [94-98]

 - name: Download documentation
   uses: actions/download-artifact@v4
   with:
     name: documentation
-    path: docs/api
+    path: .

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: This is a critical bug fix. The upload-artifact action preserves the full path, so downloading to a subdirectory creates a nested, incorrect path structure (docs/api/docs/api/...), which would break the documentation update logic.

High
Prevent nested dist directories
Suggestion Impact:The workflow no longer downloads the nightly-grid artifact into build/dist (the explicit 'path: build/dist' line was removed), which effectively avoids creating a nested build/dist/build/dist structure and aligns with downloading into the default location.

code diff:

@@ -192,7 +192,6 @@
         uses: actions/download-artifact@v4
         with:
           name: nightly-grid
-          path: build/dist
       - name: Create nightly release

In .github/workflows/nightly.yml, change the download path for the nightly-grid
artifact from build/dist to . to avoid creating a nested directory structure and
ensure the release step finds the files.

.github/workflows/nightly.yml [191-195]

 - name: Download grid packages
   uses: actions/download-artifact@v4
   with:
     name: nightly-grid
-    path: build/dist
+    path: .

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: This is a critical bug fix. The upload-artifact action preserves the full path, so downloading to build/dist would create a nested build/dist/build/dist/ path. This would cause the subsequent release step to fail as it would not find any files to upload.

High
Avoid doubled output directories
Suggestion Impact:The commit removed the explicit `path: build/dist` from the download-artifact step, causing the artifact to be downloaded to the default location (workspace, effectively like `.`), which addresses the doubled output directory issue the suggestion targeted.

code diff:

         uses: actions/download-artifact@v4
         with:
           name: release-packages
-          path: build/dist

In .github/workflows/release.yml, change the download path for the
release-packages artifact from build/dist to . to prevent a nested directory
issue that would cause the release creation to fail.

.github/workflows/release.yml [72-76]

 - name: Download release packages
   uses: actions/download-artifact@v4
   with:
     name: release-packages
-    path: build/dist
+    path: .

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: This is a critical bug fix. The upload-artifact action preserves the full path, so downloading to build/dist would create a nested build/dist/build/dist/ path. This would cause the ncipollo/release-action to fail as it would not find the packages for the draft release.

High
Incremental [*]
Capture complete patch diffs
Suggestion Impact:The workflow was changed to use `git diff --binary > changes.patch`, partially addressing the suggestion (binary handling). However, it did not add `git diff --cached --binary` nor the empty-file removal check.

code diff:

       - name: Save git diff
         if: always() && inputs.artifact-name != '' && inputs.artifact-path == ''
-        run: git diff > changes.patch
+        run: git diff --binary > changes.patch

Modify the Save git diff step to capture both staged and unstaged changes,
handle binary files, and avoid creating empty patch files. Update the run
command to git diff --binary > changes.patch; git diff --cached --binary >>
changes.patch; [ -s changes.patch ] || rm -f changes.patch.

.github/workflows/bazel.yml [254-258]

 - name: Save git diff
   if: always() && inputs.artifact-name != '' && inputs.artifact-path == ''
-  run: git diff > changes.patch
+  run: |
+    git diff --binary > changes.patch
+    git diff --cached --binary >> changes.patch
+    [ -s changes.patch ] || rm -f changes.patch
 - name: Upload artifact
   if: always() && inputs.artifact-name != ''

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that git diff alone is insufficient as it misses staged changes, which are used by downstream jobs (git apply --index). The proposed change to include git diff --cached and --binary is a crucial fix for the correctness of the new reusable workflow.

Medium
✅ Suggestions up to commit 28ce7c2
CategorySuggestion                                                                                                                                    Impact
Possible issue
Upload artifacts even on failure
Suggestion Impact:The workflow conditions for "Save git diff" and "Upload artifact" were updated to include always(), ensuring these steps run even if earlier steps fail. (The upload action version was also bumped to v5.)

code diff:

       - name: Save git diff
-        if: inputs.artifact-name != '' && inputs.artifact-path == ''
+        if: always() && inputs.artifact-name != '' && inputs.artifact-path == ''
         run: git diff > changes.patch
       - name: Upload artifact
-        if: inputs.artifact-name != ''
-        uses: actions/upload-artifact@v4
+        if: always() && inputs.artifact-name != ''
+        uses: actions/upload-artifact@v5

Add always() to the conditions for saving and uploading artifacts in bazel.yml
to ensure they run even if the main run step fails.

.github/workflows/bazel.yml [254-264]

 - name: Save git diff
-  if: inputs.artifact-name != '' && inputs.artifact-path == ''
+  if: always() && inputs.artifact-name != '' && inputs.artifact-path == ''
   run: git diff > changes.patch
 - name: Upload artifact
-  if: inputs.artifact-name != ''
+  if: always() && inputs.artifact-name != ''
   uses: actions/upload-artifact@v4
   with:
     name: ${{ inputs.artifact-name }}
     path: ${{ inputs.artifact-path || 'changes.patch' }}
     retention-days: 6
     if-no-files-found: ${{ inputs.artifact-path != '' && 'error' || 'warn' }}

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical flaw where artifacts are not generated if the main run step fails, which breaks several dependent jobs like auto-formatting. Adding always() is essential for the new workflow design to function as intended.

High
Avoid failing on missing artifacts

In the commit-fixes job, add continue-on-error: true to the artifact download
step and adjust the subsequent script to handle cases where the artifact is
missing.

.github/workflows/ci-lint.yml [56-96]

 commit-fixes:
   name: Commit fixes
   needs: format
   if: failure() && github.event_name == 'pull_request'
   runs-on: ubuntu-latest
   permissions:
     contents: write
     actions: read
   steps:
     - name: Check Permissions
       if: github.event.pull_request.head.repo.fork == true
       run: |
         echo "::error::Code needs formatting. Run ./scripts/format.sh locally and push changes."
         exit 1
     - name: Checkout PR
       uses: actions/checkout@v4
       with:
         ref: ${{ github.event.pull_request.head.ref }}
     - name: Download format changes
       uses: actions/download-artifact@v4
       with:
         name: format-changes
+      continue-on-error: true
     - name: Apply and commit fixes
       id: apply
       run: |
-        if [ -s changes.patch ]; then
+        if [ -f changes.patch ] && [ -s changes.patch ]; then
           git apply changes.patch
           rm changes.patch
           git config --local user.name "Selenium CI Bot"
           git config --local user.email "selenium-ci@users.noreply.github.com"
           git add -A
           git commit -m "Auto-format code"
           echo "has_changes=true" >> "$GITHUB_OUTPUT"
         else
-          echo "::notice::No formatting changes needed."
+          echo "::notice::No formatting patch artifact found; nothing to apply."
+          echo "has_changes=false" >> "$GITHUB_OUTPUT"
         fi
     - name: Push fixes
       if: steps.apply.outputs.has_changes == 'true'
       run: |
         git push
         echo "::notice::Auto-formatted and pushed. New CI run will start."
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that the download-artifact step will fail if the artifact doesn't exist, halting the job. Adding continue-on-error: true and checking for the patch file's existence makes the commit-fixes job more robust and resilient to this scenario.

Medium
Learned
best practice
Safely escape injected context strings

Use toJSON(...) to safely escape commit messages before putting them into a bash
variable, and normalize whitespace so unexpected quotes/newlines in messages
can’t break the script.

.github/workflows/ci-renovate-rbe.yml [19-38]

 run: |
-  COMMITS="${{ join(github.event.commits.*.message, ' ') }}"
+  COMMITS_JSON='${{ toJSON(join(github.event.commits.*.message, " ")) }}'
+  COMMITS="${COMMITS_JSON%\"}"; COMMITS="${COMMITS#\"}"
+  COMMITS="${COMMITS//\\n/ }"; COMMITS="${COMMITS//\\r/ }"
   if [[ "$COMMITS" == *"[java]"* ]]; then
     RULES_JVM_EXTERNAL_REPIN=1 bazel run @maven//:pin
   fi
   if [[ "$COMMITS" == *"[rust]"* ]]; then
     CARGO_BAZEL_REPIN=true bazel run @crates//:all
   fi
   if [[ "$COMMITS" == *"[js]"* ]]; then
     bazel run -- @pnpm//:pnpm install --dir "$PWD" --lockfile-only
   fi
   if [[ "$COMMITS" == *"[dotnet]"* ]]; then
     ./dotnet/update-deps.sh
   fi
   if [[ "$COMMITS" == *"[py]"* ]]; then
     bazel run //py:requirements.update
   fi
   if [[ "$COMMITS" == *"[rb]"* ]]; then
     ./go rb:pin
   fi
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Add validation/escaping at GitHub Actions integration boundaries; avoid directly injecting untrusted context strings into shell scripts without safe encoding.

Low
✅ Suggestions up to commit 26be08a
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle empty bazel targets file correctly
Suggestion Impact:The workflow step was changed to conditionally write the multiline targets output only when bazel-targets.txt is non-empty, and otherwise write `targets=` to GITHUB_OUTPUT, preventing an empty newline output.

code diff:

@@ -44,11 +44,15 @@
       - name: Read targets
         id: read
         run: |
-          {
-            echo "targets<<EOF"
-            cat bazel-targets.txt
-            echo "EOF"
-          } >> "$GITHUB_OUTPUT"
+          if [ -s bazel-targets.txt ]; then
+            {
+              echo "targets<<EOF"
+              cat bazel-targets.txt
+              echo "EOF"
+            } >> "$GITHUB_OUTPUT"
+          else
+            echo "targets=" >> "$GITHUB_OUTPUT"
+          fi

In the read-targets job, check if bazel-targets.txt is empty. If so, set the
targets output to an empty string to avoid it containing a newline, which could
cause issues in downstream jobs.

.github/workflows/ci.yml [33-51]

 read-targets:
   name: Read Targets
   needs: check
   runs-on: ubuntu-latest
   outputs:
     targets: ${{ steps.read.outputs.targets }}
   steps:
     - name: Download targets
       uses: actions/download-artifact@v4
       with:
         name: check-targets
     - name: Read targets
       id: read
       run: |
-        {
-          echo "targets<<EOF"
-          cat bazel-targets.txt
-          echo "EOF"
-        } >> "$GITHUB_OUTPUT"
+        if [ -s bazel-targets.txt ]; then
+          {
+            echo "targets<<EOF"
+            cat bazel-targets.txt
+            echo "EOF"
+          } >> "$GITHUB_OUTPUT"
+        else
+          echo "targets=" >> "$GITHUB_OUTPUT"
+        fi

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that an empty bazel-targets.txt file leads to a job output with a newline, which can cause issues in downstream jobs. The proposed fix ensures the output is a truly empty string, which is a critical improvement for the correctness of the workflow logic.

Medium
Prevent job failure if no changes
Suggestion Impact:The workflow was changed to check whether changes.patch exists and is non-empty before applying it and performing git commit/push. The implementation differs from the suggestion by combining steps and by failing the job when the patch is missing/empty (exit 1) instead of skipping commit/push.

code diff:

+      - name: Apply and push
+        run: |
+          if [ -f changes.patch ] && [ -s changes.patch ]; then
+            git apply --index changes.patch
+            git config --local user.email "selenium-ci@users.noreply.github.com"
+            git config --local user.name "Selenium CI Bot"
+            git commit -m "update selenium manager version and rust changelog"
+            git push origin HEAD:rust-release-${{ inputs.version }} --force
+          else
+            echo "::error::No changes to apply for rust version update"
+            exit 1
+          fi

In the push-rust-version job, add a condition to check if changes.patch is
non-empty before attempting to apply it and commit, preventing job failure when
there are no changes.

.github/workflows/pre-release.yml [43-66]

 push-rust-version:
   name: Push Rust Version
   needs: generate-rust-version
   runs-on: ubuntu-latest
   steps:
     - name: Checkout repo
       uses: actions/checkout@v4
       with:
         token: ${{ secrets.SELENIUM_CI_TOKEN }}
         fetch-depth: 0
     - name: Download rust version patch
       uses: actions/download-artifact@v4
       with:
         name: rust-version
     - name: Apply patch
-      run: git apply --index changes.patch
+      run: |
+        if [ -s changes.patch ]; then
+          git apply --index changes.patch
+          echo "CHANGES_APPLIED=true" >> "$GITHUB_ENV"
+        fi
     - name: Prep git
+      if: env.CHANGES_APPLIED == 'true'
       run: |
         git config --local user.email "selenium-ci@users.noreply.github.com"
         git config --local user.name "Selenium CI Bot"
     - name: Push changes
+      if: env.CHANGES_APPLIED == 'true'
       run: |
         git commit -m "update selenium manager version and rust changelog"
         git push origin HEAD:rust-release-${{ inputs.version }} --force

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: This is a valid improvement for workflow robustness, preventing a potential failure if no version changes are generated. This pattern of checking for a non-empty patch is used elsewhere in the PR, making this a consistent and valuable addition.

Medium
General
Fetch full git history
Suggestion Impact:The workflow was updated to support configuring actions/checkout's fetch-depth via a new reusable-workflow input, and the checkout step now uses that input. This enables callers to set fetch-depth to 0 for full history, addressing the underlying need, though it does not hardcode fetch-depth: 0 by default.

code diff:

@@ -81,6 +81,11 @@
         required: false
         type: boolean
         default: false
+      fetch-depth:
+        description: Number of commits to fetch (0 for full history)
+        required: false
+        type: number
+        default: 1
 
 jobs:
   bazel:
@@ -99,6 +104,7 @@
         uses: actions/checkout@v4
         with:
           ref: ${{ inputs.ref || github.ref }}
+          fetch-depth: ${{ inputs.fetch-depth }}

Add fetch-depth: 0 to the actions/checkout step in the bazel.yml reusable
workflow to ensure the full git history is available for subsequent git
operations.

.github/workflows/bazel.yml [98-101]

 - name: Checkout source tree
   uses: actions/checkout@v4
   with:
     ref: ${{ inputs.ref || github.ref }}
+    fetch-depth: 0

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: This is a critical fix for the reusable workflow, as several calling workflows rely on git diff or commit ranges which require the full git history. Without fetch-depth: 0, these operations would fail or produce incorrect results.

Medium
Learned
best practice
Avoid swallowing patch-apply errors
Suggestion Impact:The workflow was changed to conditionally run `git apply --index changes.patch` only when the patch file exists and is non-empty, and removed the unconditional `|| echo "No changes to apply"` that would mask real git apply failures. If no patch is present, it now emits a notice and skips apply/commit/push.

code diff:

+      - name: Apply and push
         run: |
-          git config --local user.email "selenium-ci@users.noreply.github.com"
-          git config --local user.name "Selenium CI Bot"
-      - name: Push version changes
-        run: |
-          git commit -m "[build] Reset versions to nightly after ${{ needs.prepare.outputs.tag }} release" || echo "No changes to commit"
-          git push
+          if [ -f changes.patch ] && [ -s changes.patch ]; then
+            git apply --index changes.patch
+            git config --local user.email "selenium-ci@users.noreply.github.com"
+            git config --local user.name "Selenium CI Bot"
+            git commit -m "[build] Reset versions to nightly after ${{ needs.prepare.outputs.tag }} release"
+            git push
+          else
+            echo "::notice::No version changes to apply"
+          fi

Replace the unconditional || echo ... with an explicit file existence/size check
so real git apply failures fail the job instead of being silently ignored.

.github/workflows/release.yml [189-190]

 - name: Apply patch
-  run: git apply --index changes.patch || echo "No changes to apply"
+  shell: bash
+  run: |
+    if [ ! -f changes.patch ] || [ ! -s changes.patch ]; then
+      echo "No changes to apply"
+      exit 0
+    fi
+    git apply --index changes.patch

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Add explicit validation/availability guards at integration boundaries (e.g., downloaded artifacts/files) and avoid swallowing errors from critical steps.

Low