Skip to content

[build] do not auto-merge browser updates#17432

Merged
titusfortner merged 4 commits into
trunkfrom
no-merge
May 12, 2026
Merged

[build] do not auto-merge browser updates#17432
titusfortner merged 4 commits into
trunkfrom
no-merge

Conversation

@titusfortner
Copy link
Copy Markdown
Member

🔗 Related Issues

Decided on this one instead of #17431

💥 What does this PR do?

Same behavior, just no auto-merging

🔧 Implementation Notes

We can go back to using the create-pull-request action for this since no special behavior

💡 Additional Considerations

These will need to be merged before releases anyway, so not sure we need to do anything special with them for now.

Copilot AI review requested due to automatic review settings May 10, 2026 09:10
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Replace auto-merge with manual PR creation for browser updates

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Replace custom commit-push workflow with standard create-pull-request action
• Remove auto-merge functionality from browser update workflow
• Simplify workflow by downloading patch and applying locally
• Consolidate PR creation into single job without intermediate push step
Diagram
flowchart LR
  A["Update Pinned Browsers"] --> B["Create Pull Request"]
  B --> C["Checkout Repository"]
  C --> D["Download Patch Artifact"]
  D --> E["Apply Patch Locally"]
  E --> F["Create PR via peter-evans Action"]
Loading

Grey Divider

File Changes

1. .github/workflows/pin-browsers.yml ⚙️ Configuration changes +27/-33

Simplify browser update workflow to remove auto-merge

• Removed push-changes job that used custom commit-changes workflow
• Replaced two-step workflow with single create-pr job using peter-evans/create-pull-request
 action
• Changed from auto-merging PRs to manual merge requirement
• Simplified artifact handling by downloading patch and applying locally instead of pushing to
 branch
• Updated permissions to include pull-requests: write for PR creation
• Modified PR title and body text for clarity

.github/workflows/pin-browsers.yml


Grey Divider

Qodo Logo

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label May 10, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 10, 2026

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (2) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Wrong ref for trunk PR 🐞 Bug ≡ Correctness
Description
The pin-browsers workflow creates PRs with base=trunk but both the patch-producing job and the
PR-creation job operate on the workflow’s trigger ref by default. When workflow_dispatch is run from
a non-trunk branch, the automation PR can inadvertently include unrelated branch changes or the
downloaded patch may not apply cleanly to trunk.
Code

.github/workflows/pin-browsers.yml[R29-52]

+      - name: Checkout repository
        uses: actions/checkout@v4
-      - name: Create Pull Request
-        env:
-          GH_TOKEN: ${{ secrets.SELENIUM_CI_TOKEN }}
+        with:
+          persist-credentials: false
+      - name: Download patch
+        id: download
+        uses: actions/download-artifact@v4
+        with:
+          name: pinned-browsers
+        continue-on-error: true
+      - name: Apply Patch
+        if: steps.download.outcome == 'success'
        run: |
-          existing=$(gh pr list --head pinned-browser-updates --json number --jq '.[0].number // empty')
-          if [ -n "$existing" ]; then
-            echo "::notice::PR #$existing already exists"
-            pr="$existing"
-          else
-            pr_url=$(gh pr create \
-              --head pinned-browser-updates \
-              --base trunk \
-              --title "[dotnet][rb][java][js][py] Automated Browser Version Update" \
-              --body $'This is an automated pull request to update pinned browsers and drivers\n\nMerge after verify the new browser versions properly passing the tests and no bugs need to be filed')
-            pr="${pr_url##*/}"
-          fi
-          gh pr merge "$pr" --auto --squash --delete-branch
+          git apply --index changes.patch
+          rm changes.patch
+      - name: Create Pull Request
+        if: steps.download.outcome == 'success'
+        uses: peter-evans/create-pull-request@v6
+        with:
+          token: ${{ secrets.SELENIUM_CI_TOKEN }}
+          commit-message: "Update pinned browser versions"
+          author: Selenium CI Bot <selenium-ci@users.noreply.github.com>
+          base: trunk
+          title: "[build] Automated Browser Version Update"
Evidence
The update job does not pass a ref, and the reusable bazel workflow checks out `inputs.ref ||
github.ref`, so patch generation follows the triggering ref. The create-pr job likewise checks out
without a ref, but then opens a PR explicitly targeting base: trunk, creating a mismatch when the
run is dispatched from a non-trunk branch.

.github/workflows/pin-browsers.yml[11-18]
.github/workflows/pin-browsers.yml[29-57]
.github/workflows/bazel.yml[130-135]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pin-browsers.yml` generates/apply a patch from the workflow run’s trigger ref (default checkout behavior), but then opens a PR targeting `base: trunk`. If someone runs `workflow_dispatch` from a non-trunk branch, the PR content can be polluted with that branch’s unrelated commits/diffs, or the patch may not apply against trunk as intended.

## Issue Context
- `update` job calls the reusable `bazel.yml` without passing `ref`, so it checks out `${{ github.ref }}`.
- `create-pr` job also checks out without `ref`, so it again uses the triggering ref.
- The PR creation step hardcodes `base: trunk`.

## Fix Focus Areas
- .github/workflows/pin-browsers.yml[11-18]
- .github/workflows/pin-browsers.yml[29-57]
- .github/workflows/bazel.yml[130-135]

## Suggested fix
1. In `pin-browsers.yml`, force the `update` job to run on trunk by passing `ref: trunk` to the reusable `bazel.yml` workflow.
2. In `pin-browsers.yml`, force the `create-pr` job checkout to use `ref: trunk` (and optionally `fetch-depth: 0` if needed).
3. (Optional hardening) Add a guard so `workflow_dispatch` only runs on `refs/heads/trunk`, or document that dispatch should be done from trunk.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. PR base not set 🐞 Bug ≡ Correctness
Description
The pin-browsers workflow creates PRs via peter-evans/create-pull-request without setting base,
so a workflow_dispatch run from a non-trunk branch can open/update the automation PR against the
wrong target branch. This can misroute pinned browser updates and require manual cleanup/recreation
of the PR.
Code

.github/workflows/pin-browsers.yml[R41-52]

+      - name: Create Pull Request
+        uses: peter-evans/create-pull-request@v6
+        with:
+          token: ${{ secrets.SELENIUM_CI_TOKEN }}
+          commit-message: "Update pinned browser versions"
+          author: Selenium CI Bot <selenium-ci@users.noreply.github.com>
+          title: "[build] Automated Browser Version Update"
+          body: |
+            This is an automated pull request to update pinned browsers and drivers
+
+            Merge after verifying the new browser versions are properly passing the tests
+          branch: "pinned-browser-updates"
Evidence
pin-browsers.yml invokes the PR creation action with branch: pinned-browser-updates but does not
specify base, while pre-release.yml uses the same action and explicitly sets base: trunk,
showing the intended pattern/target branch.

.github/workflows/pin-browsers.yml[41-52]
.github/workflows/pre-release.yml[289-297]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`peter-evans/create-pull-request` is invoked without a `base:` input in `.github/workflows/pin-browsers.yml`. When the workflow is manually dispatched from a non-trunk branch, the action may create/update the PR targeting that branch instead of `trunk`.

### Issue Context
Another workflow using the same action explicitly sets `base: trunk`, indicating `trunk` is the intended target for automation PRs.

### Fix
Add `base: trunk` to the `peter-evans/create-pull-request` step (and optionally also ensure `actions/checkout` checks out `trunk` explicitly, if you want the whole job to always operate on trunk regardless of dispatch branch).

### Fix Focus Areas
- .github/workflows/pin-browsers.yml[41-52]
- .github/workflows/pre-release.yml[289-297]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Patch apply lacks guards 🐞 Bug ☼ Reliability
Description
The create-pr job downloads and applies changes.patch unconditionally; when the artifact is missing
or the patch is empty (common when there are no pinned browser updates), `git apply --index
changes.patch` will fail and the scheduled workflow will error instead of cleanly no-op.
Code

.github/workflows/pin-browsers.yml[R33-40]

+      - name: Download patch
+        uses: actions/download-artifact@v4
+        with:
+          name: pinned-browsers
+      - name: Apply Patch
        run: |
-          existing=$(gh pr list --head pinned-browser-updates --json number --jq '.[0].number // empty')
-          if [ -n "$existing" ]; then
-            echo "::notice::PR #$existing already exists"
-            pr="$existing"
-          else
-            pr_url=$(gh pr create \
-              --head pinned-browser-updates \
-              --base trunk \
-              --title "[dotnet][rb][java][js][py] Automated Browser Version Update" \
-              --body $'This is an automated pull request to update pinned browsers and drivers\n\nMerge after verify the new browser versions properly passing the tests and no bugs need to be filed')
-            pr="${pr_url##*/}"
-          fi
-          gh pr merge "$pr" --auto --squash --delete-branch
+          git apply --index changes.patch
+          rm changes.patch
Evidence
The reusable bazel workflow always produces and uploads a changes.patch artifact when
artifact-name is set, even if there are no diffs (empty patch file). The old flow used
commit-changes.yml, which explicitly handled both missing artifacts and empty patches before
calling git apply; the new pin-browsers.yml job removed those checks and now calls git apply
unconditionally.

.github/workflows/pin-browsers.yml[33-40]
.github/workflows/bazel.yml[280-290]
.github/workflows/commit-changes.yml[47-92]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pin-browsers.yml` now downloads `pinned-browsers` and runs `git apply --index changes.patch` without checking whether the artifact download succeeded or whether `changes.patch` is non-empty. This can cause the scheduled workflow to fail on runs where there are no updates.

### Issue Context
The old reusable workflow (`commit-changes.yml`) guarded both missing artifacts (download outcome) and empty patches (`-s changes.patch`) before applying.

### Fix Focus Areas
- .github/workflows/pin-browsers.yml[33-40]
- .github/workflows/bazel.yml[280-290]
- .github/workflows/commit-changes.yml[47-92]

### Suggested change
1. Make the download step `continue-on-error: true` and capture its outcome (or use `if-no-files-found` behavior).
2. Before applying, check `test -s changes.patch` (and optionally check the download outcome) and exit 0 with a notice when there are no changes.
3. Only run the `create-pull-request` step when a patch was applied (e.g., via a step output like `patch_applied=true`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. rm missing -- guard 📘 Rule violation ⛨ Security
Description
The workflow deletes changes.patch using rm -f changes.patch without a -- argument terminator
(and without quoting), which is discouraged in CI glue for safe argument handling. This can lead to
unexpected behavior if the path ever becomes option-like or is later templated/variable-driven.
Code

.github/workflows/bazel.yml[285]

+          [ -s changes.patch ] || rm -f changes.patch
Evidence
PR Compliance ID 13 requires safe argument handling in build/CI scripts, including safer destructive
command forms. The changed step removes a file without --/quoting on the added line containing `rm
-f changes.patch`.

.github/workflows/bazel.yml[283-285]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow uses `rm -f changes.patch` without `--` (and without quoting), which is less safe for CI scripting and can behave unexpectedly if the filename ever starts with `-` or becomes variable-driven.

## Issue Context
This occurs in the `Save git diff` step where the patch is removed when empty.

## Fix Focus Areas
- .github/workflows/bazel.yml[283-285]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. No-op lacks notice 🐞 Bug ◔ Observability
Description
When the pinned-browsers artifact is missing, the create-pr job suppresses the download failure and
then silently skips applying the patch and opening a PR. This makes scheduled runs hard to diagnose
when updates are unexpectedly absent (artifact missing vs. genuinely no changes).
Code

.github/workflows/pin-browsers.yml[R33-46]

+      - name: Download patch
+        id: download
+        uses: actions/download-artifact@v4
+        with:
+          name: pinned-browsers
+        continue-on-error: true
+      - name: Apply Patch
+        if: steps.download.outcome == 'success'
        run: |
-          existing=$(gh pr list --head pinned-browser-updates --json number --jq '.[0].number // empty')
-          if [ -n "$existing" ]; then
-            echo "::notice::PR #$existing already exists"
-            pr="$existing"
-          else
-            pr_url=$(gh pr create \
-              --head pinned-browser-updates \
-              --base trunk \
-              --title "[dotnet][rb][java][js][py] Automated Browser Version Update" \
-              --body $'This is an automated pull request to update pinned browsers and drivers\n\nMerge after verify the new browser versions properly passing the tests and no bugs need to be filed')
-            pr="${pr_url##*/}"
-          fi
-          gh pr merge "$pr" --auto --squash --delete-branch
+          git apply --index changes.patch
+          rm changes.patch
+      - name: Create Pull Request
+        if: steps.download.outcome == 'success'
+        uses: peter-evans/create-pull-request@v6
Evidence
The pin-browsers workflow suppresses download errors and conditionally skips all later steps without
any logging, while the existing commit-changes workflow in this repo prints a notice when the
artifact is missing, showing an established pattern for observability.

.github/workflows/pin-browsers.yml[33-46]
.github/workflows/commit-changes.yml[55-60]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Download patch` is marked `continue-on-error: true`, and later steps are gated on `steps.download.outcome == 'success'`. When the artifact is absent, the job does nothing without emitting any message, reducing debuggability.

### Issue Context
The repo’s reusable `commit-changes.yml` handles the same situation by emitting a `::notice::` explaining the artifact was not found.

### Fix
Add an explicit step after `Download patch` that runs when `steps.download.outcome != 'success'` to log a `::notice::` (or `::warning::`) such as "No pinned browser updates (patch artifact not uploaded)" and then exit 0 (or set an output).

### Fix Focus Areas
- .github/workflows/pin-browsers.yml[33-46]
- .github/workflows/commit-changes.yml[55-60]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. PAT used for checkout ✓ Resolved 🐞 Bug ⛨ Security
Description
The pin-browsers create-pr job checks out the repository using the write-scoped SELENIUM_CI_TOKEN
even though checkout itself only needs read access, increasing the exposure surface of that secret
in the job workspace/git config. This violates least-privilege and makes accidental leakage more
impactful.
Code

.github/workflows/pin-browsers.yml[R29-32]

+      - name: Checkout repository
        uses: actions/checkout@v4
-      - name: Create Pull Request
-        env:
-          GH_TOKEN: ${{ secrets.SELENIUM_CI_TOKEN }}
+        with:
+          token: ${{ secrets.SELENIUM_CI_TOKEN }}
Evidence
The create-pr job explicitly passes the write-scoped secret token to the checkout action, whereas
the repo’s existing reusable commit workflow demonstrates using github.token as a safe fallback
for checkout and only using the PAT when needed.

.github/workflows/pin-browsers.yml[19-32]
.github/workflows/commit-changes.yml[42-52]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pin-browsers.yml` uses the write-scoped `secrets.SELENIUM_CI_TOKEN` for `actions/checkout`, which is broader than necessary for a read-only clone and increases secret exposure.

### Issue Context
The same job already provides `SELENIUM_CI_TOKEN` to `peter-evans/create-pull-request`, so checkout does not need to be performed with the PAT.

### Fix
- Change the checkout step to use the default `github.token` (or omit `token:` entirely).
- Optionally set `persist-credentials: false` on checkout to avoid leaving credentials in the local git config.

### Fix Focus Areas
- .github/workflows/pin-browsers.yml[29-32]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (2)
7. Unpinned action with PAT 🐞 Bug ⛨ Security
Description
The workflow runs peter-evans/create-pull-request@v6 (a moving tag) while passing a write-scoped
secret token, which increases supply-chain blast radius if the tag is ever compromised or
unexpectedly updated. Pinning to a commit SHA (or an immutable version) reduces this risk.
Code

.github/workflows/pin-browsers.yml[R42-45]

+        uses: peter-evans/create-pull-request@v6
+        with:
+          token: ${{ secrets.SELENIUM_CI_TOKEN }}
+          commit-message: "Update pinned browser versions"
Evidence
pin-browsers.yml uses peter-evans/create-pull-request@v6 and passes secrets.SELENIUM_CI_TOKEN,
and the job has write permissions, so the action executes with credentials capable of writing to the
repo and creating PRs.

.github/workflows/pin-browsers.yml[24-27]
.github/workflows/pin-browsers.yml[42-45]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A third-party GitHub Action is referenced by a moving major tag (`@v6`) while being provided a write-scoped token.

### Issue Context
This is a defense-in-depth hardening measure: pinning actions to an immutable ref prevents unexpected upstream changes from executing in your CI context.

### Fix
Update the action reference to a specific commit SHA (preferred) or an immutable release tag, and keep it updated via your dependency update process.

### Fix Focus Areas
- .github/workflows/pin-browsers.yml[42-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. rm changes.patch unsafe args 📘 Rule violation ⛨ Security
Description
The workflow shell step removes a file without using -- (and without quoting), which is
discouraged for safe argument handling in build/CI glue. If the filename ever becomes option-like
(e.g., starts with -) or is templated later, this can behave unexpectedly.
Code

.github/workflows/pin-browsers.yml[R39-40]

+          git apply --index changes.patch
+          rm changes.patch
Evidence
PR Compliance ID 14 requires safe argument handling in scripts, including using rm with -- and
proper quoting. The new workflow run step executes rm changes.patch directly.

.github/workflows/pin-browsers.yml[39-40]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow shell step deletes `changes.patch` using `rm changes.patch` without the safer `rm -- <path>` pattern (and without quoting).

## Issue Context
Compliance requires safe argument handling in CI/scripts to avoid option-injection edge cases and brittle behavior if filenames or paths change.

## Fix Focus Areas
- .github/workflows/pin-browsers.yml[39-40]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Copy link
Copy Markdown
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

This PR updates the scheduled “Pin Browsers” automation so it still opens/updates a PR with pinned browser version changes, but no longer enables auto-merge for those PRs.

Changes:

  • Remove the commit/push + gh pr merge --auto flow for pinned browser updates.
  • Switch to peter-evans/create-pull-request to create/update the pinned-browser-updates PR branch from the generated patch artifact.

Comment thread .github/workflows/pin-browsers.yml
Comment thread .github/workflows/pin-browsers.yml
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 12, 2026

Persistent review updated to latest commit ec78713

Comment thread .github/workflows/pin-browsers.yml
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 12, 2026

Persistent review updated to latest commit 19729b6

Copy link
Copy Markdown
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

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

Comment thread .github/workflows/bazel.yml
Comment thread .github/workflows/pin-browsers.yml
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 12, 2026

Persistent review updated to latest commit 6b1420d

Copy link
Copy Markdown
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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/pin-browsers.yml
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 12, 2026

Persistent review updated to latest commit 673bc4b

Comment thread .github/workflows/pin-browsers.yml
@titusfortner titusfortner merged commit 65bb32e into trunk May 12, 2026
27 checks passed
@titusfortner titusfortner deleted the no-merge branch May 12, 2026 03:06
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants