[build] derive PR diff base from HEAD^1 instead of trunk tip by titusfortner · Pull Request #17438 · SeleniumHQ/selenium

titusfortner · 2026-05-11T21:47:59Z

💥 What does this PR do?

Our check targets job for running PRs has been incorrectly comparing the PR to current trunk (with pull_request.base) instead of to the parent merge commit (HEAD^1), resulting in more things being tested than necessary.

GitHub's auto-merge commit (refs/pull/N/merge) is checked out as HEAD.
HEAD^1 is the base branch tip; diffing it against HEAD shows what the merge introduces - i.e. the PR's effective changes.

The bazel.yml workflow checks out to a depth of PR_COMMITS + 2 to ensure that the merge commits will be present

🔄 Types of changes

Bug fix (backwards compatible)

qodo-code-review · 2026-05-11T21:48:14Z

Review Summary by Qodo

Fix PR diff base calculation to use parent commit

🐞 Bug fix

Walkthroughs

Description

• Fix PR diff base calculation to use parent commit instead of trunk
• Remove unnecessary base ref fetch from bazel workflow
• Calculate BASE_SHA from HEAD~PR_COMMITS for accurate PR comparisons
• Fallback to github.event.before when PR_COMMITS unavailable

Diagram

flowchart LR
  A["PR Event"] -->|Extract PR_COMMITS| B["Calculate BASE_SHA"]
  B -->|HEAD~PR_COMMITS| C["Parent Commit"]
  C -->|Diff Range| D["Affected Targets"]
  A -->|Fallback| E["github.event.before"]
  E --> D

File Changes

1. .github/workflows/bazel.yml 🐞 Bug fix +0/-3

Remove base ref fetch step

• Removed step that fetches base ref for PR comparison
• Eliminated unnecessary git fetch of origin base SHA
• Simplifies checkout process by relying on fetch-depth calculation

.github/workflows/bazel.yml

2. .github/workflows/ci.yml 🐞 Bug fix +6/-1

Calculate BASE_SHA from parent commit

• Changed BASE_SHA calculation to derive from HEAD~PR_COMMITS instead of
 github.event.pull_request.base.sha
• Added PR_COMMITS variable extraction from github event
• Implemented conditional logic to use parent commit when PR_COMMITS available
• Fallback to github.event.before when PR_COMMITS is unavailable

.github/workflows/ci.yml

qodo-code-review · 2026-05-11T21:48:15Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (2)

1. HEAD^1 base unverified 📘 Rule violation ☼ Reliability

Description

The workflow sets BASE_SHA to HEAD^1 without verifying that commit exists in the checked-out
history, which can fail under shallow fetches or non-merge checkouts. This can make CI behavior
non-deterministic (affected targets may error or be computed from an unintended range).

Code

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

+          if [ -n "${{ github.event.pull_request.base.sha }}" ]; then
+            # GitHub's auto-merge commit (refs/pull/N/merge) is checked out as HEAD.
+            # HEAD^1 is the base branch tip; diffing it against HEAD shows what the
+            # merge introduces - i.e. the PR's effective changes.
+            BASE_SHA="HEAD^1"
+          else
+            BASE_SHA="${{ github.event.before }}"
+          fi
          if [ -n "$BASE_SHA" ]; then
-            ./go bazel:affected_targets "${BASE_SHA}..${HEAD_SHA}" bazel-test-file-index
+            ./go bazel:affected_targets "${BASE_SHA}..HEAD" bazel-test-file-index

Evidence

PR Compliance ID 15 requires CI/scripts to be hardened and deterministic. The added logic sets
BASE_SHA="HEAD^1" and immediately uses it in the diff range without verifying HEAD^1 is
present/resolvable in the local checkout.

.github/workflows/ci.yml[42-51]
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
`BASE_SHA` is set to `HEAD^1` for PRs, but the workflow does not check that `HEAD^1` exists locally (e.g., shallow checkout without the first parent). This can cause `./go bazel:affected_targets` to fail or compute a diff range that doesn't match intent.

## Issue Context
This is a build/CI script path where deterministic behavior is required. Add a guard that verifies `HEAD^1` is resolvable and, if not, fetch additional history (or fall back to a known PR base SHA) before running the affected-targets computation.

## Fix Focus Areas
- .github/workflows/ci.yml[42-51]

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

2. PR_COMMITS used without validation 📘 Rule violation ☼ Reliability

Description

The workflow uses the external github.event.pull_request.commits value directly to construct a Git
revision (${HEAD_SHA}~${PR_COMMITS}) without validating it is a non-negative integer or that it
matches the PR’s first-parent distance, which can fail with non-actionable errors or compute an
unintended base revision. If that base calculation fails or overshoots, CI can fall back to an
implicit HEAD^..HEAD diff and miss changes from earlier commits in the PR, making behavior less
deterministic and harder to debug.

Code

.github/workflows/ci.yml[R43-45]

+          PR_COMMITS="${{ github.event.pull_request.commits }}"
+          if [ -n "$PR_COMMITS" ]; then
+            BASE_SHA="$(git rev-parse "${HEAD_SHA}~${PR_COMMITS}")"

Evidence

PR Compliance ID 13 requires early validation of protocol-derived inputs with deterministic
exceptions, but in .github/workflows/ci.yml the workflow takes PR_COMMITS from
github.event.pull_request.commits and interpolates it into `git rev-parse
"${HEAD_SHA}~${PR_COMMITS}"` without any numeric/type validation, so empty/non-numeric/unexpected
values can lead to unclear rev-parse failures or incorrect base selection. The workflow then
computes BASE_SHA from that expression and, when BASE_SHA is empty, calls `./go
bazel:affected_targets` without an explicit range; the underlying rake task defaults to
HEAD^..HEAD, meaning only the last commit is considered and earlier PR commits can be missed.

.github/workflows/ci.yml[43-45]
.github/workflows/ci.yml[35-53]
rake_tasks/bazel.rake[15-31]
.github/workflows/bazel.yml[112-138]
Best Practice: Learned patterns
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
`PR_COMMITS` is sourced from the GitHub event payload (`github.event.pull_request.commits`) and is used directly in `git rev-parse "${HEAD_SHA}~${PR_COMMITS}"` to derive `BASE_SHA` without validating it is a non-negative integer or ensuring the underlying assumption (that it matches the PR’s first-parent distance) holds. When this rev calculation fails or overshoots, CI may either fail with confusing, non-deterministic errors or fall back to an implicit `HEAD^..HEAD` diff (via `bazel:affected_targets`/rake defaults), which can miss changes from earlier commits in the PR.

## Issue Context
This is build/CI scripting code and must comply with the requirement to validate external/protocol-derived inputs early and fail deterministically with actionable errors. The affected-targets flow ultimately runs a `git diff` between computed base/head revisions; if `BASE_SHA` is empty, the rake task defaults to `HEAD^..HEAD`, which does not cover the full PR.

## Fix Focus Areas
- .github/workflows/ci.yml[35-53]
- rake_tasks/bazel.rake[15-31]
- .github/workflows/bazel.yml[112-138]

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

Previous review results

Review updated until commit b82fd93

Results up to commit b026711

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. PR_COMMITS used without validation 📘 Rule violation ☼ Reliability

Description

The workflow uses the external github.event.pull_request.commits value directly to construct a Git
revision (${HEAD_SHA}~${PR_COMMITS}) without validating it is a non-negative integer or that it
matches the PR’s first-parent distance, which can fail with non-actionable errors or compute an
unintended base revision. If that base calculation fails or overshoots, CI can fall back to an
implicit HEAD^..HEAD diff and miss changes from earlier commits in the PR, making behavior less
deterministic and harder to debug.

Code

.github/workflows/ci.yml[R43-45]

+          PR_COMMITS="${{ github.event.pull_request.commits }}"
+          if [ -n "$PR_COMMITS" ]; then
+            BASE_SHA="$(git rev-parse "${HEAD_SHA}~${PR_COMMITS}")"

Evidence

PR Compliance ID 13 requires early validation of protocol-derived inputs with deterministic
exceptions, but in .github/workflows/ci.yml the workflow takes PR_COMMITS from
github.event.pull_request.commits and interpolates it into `git rev-parse
"${HEAD_SHA}~${PR_COMMITS}"` without any numeric/type validation, so empty/non-numeric/unexpected
values can lead to unclear rev-parse failures or incorrect base selection. The workflow then
computes BASE_SHA from that expression and, when BASE_SHA is empty, calls `./go
bazel:affected_targets` without an explicit range; the underlying rake task defaults to
HEAD^..HEAD, meaning only the last commit is considered and earlier PR commits can be missed.

.github/workflows/ci.yml[43-45]
.github/workflows/ci.yml[35-53]
rake_tasks/bazel.rake[15-31]
.github/workflows/bazel.yml[112-138]
Best Practice: Learned patterns
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
`PR_COMMITS` is sourced from the GitHub event payload (`github.event.pull_request.commits`) and is used directly in `git rev-parse "${HEAD_SHA}~${PR_COMMITS}"` to derive `BASE_SHA` without validating it is a non-negative integer or ensuring the underlying assumption (that it matches the PR’s first-parent distance) holds. When this rev calculation fails or overshoots, CI may either fail with confusing, non-deterministic errors or fall back to an implicit `HEAD^..HEAD` diff (via `bazel:affected_targets`/rake defaults), which can miss changes from earlier commits in the PR.

## Issue Context
This is build/CI scripting code and must comply with the requirement to validate external/protocol-derived inputs early and fail deterministically with actionable errors. The affected-targets flow ultimately runs a `git diff` between computed base/head revisions; if `BASE_SHA` is empty, the rake task defaults to `HEAD^..HEAD`, which does not cover the full PR.

## Fix Focus Areas
- .github/workflows/ci.yml[35-53]
- rake_tasks/bazel.rake[15-31]
- .github/workflows/bazel.yml[112-138]

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

Results up to commit 5836478

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. HEAD^1 base unverified 📘 Rule violation ☼ Reliability

Description

The workflow sets BASE_SHA to HEAD^1 without verifying that commit exists in the checked-out
history, which can fail under shallow fetches or non-merge checkouts. This can make CI behavior
non-deterministic (affected targets may error or be computed from an unintended range).

Code

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

+          if [ -n "${{ github.event.pull_request.base.sha }}" ]; then
+            # GitHub's auto-merge commit (refs/pull/N/merge) is checked out as HEAD.
+            # HEAD^1 is the base branch tip; diffing it against HEAD shows what the
+            # merge introduces - i.e. the PR's effective changes.
+            BASE_SHA="HEAD^1"
+          else
+            BASE_SHA="${{ github.event.before }}"
+          fi
          if [ -n "$BASE_SHA" ]; then
-            ./go bazel:affected_targets "${BASE_SHA}..${HEAD_SHA}" bazel-test-file-index
+            ./go bazel:affected_targets "${BASE_SHA}..HEAD" bazel-test-file-index

Evidence

PR Compliance ID 15 requires CI/scripts to be hardened and deterministic. The added logic sets
BASE_SHA="HEAD^1" and immediately uses it in the diff range without verifying HEAD^1 is
present/resolvable in the local checkout.

.github/workflows/ci.yml[42-51]
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
`BASE_SHA` is set to `HEAD^1` for PRs, but the workflow does not check that `HEAD^1` exists locally (e.g., shallow checkout without the first parent). This can cause `./go bazel:affected_targets` to fail or compute a diff range that doesn't match intent.

## Issue Context
This is a build/CI script path where deterministic behavior is required. Add a guard that verifies `HEAD^1` is resolvable and, if not, fetch additional history (or fall back to a known PR base SHA) before running the affected-targets computation.

## Fix Focus Areas
- .github/workflows/ci.yml[42-51]

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

Copilot

Pull request overview

This PR updates the GitHub Actions CI target-selection logic so PR “affected targets” are computed against the PR’s parent commit history rather than the current trunk head, reducing unnecessary Bazel test execution.

Changes:

In CI “Check Targets”, compute BASE_SHA for PR diffs using HEAD_SHA~PR_COMMITS (fallback to github.event.before for non-PR events).
Remove the extra git fetch of pull_request.base.sha in the reusable Bazel workflow.

Reviewed changes

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

File	Description
.github/workflows/ci.yml	Changes how the diff base SHA is computed for affected target calculation in PR/push contexts.
.github/workflows/bazel.yml	Removes an explicit fetch of the PR base SHA, relying on the initial checkout depth instead.

qodo-code-review · 2026-05-11T22:42:27Z

Persistent review updated to latest commit 5836478

…th HEAD^1

qodo-code-review · 2026-05-11T22:49:42Z

Persistent review updated to latest commit b82fd93

Copilot

Pull request overview

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

titusfortner requested a review from Copilot May 11, 2026 21:48

selenium-ci added the B-build Includes scripting, bazel and CI integrations label May 11, 2026

Copilot started reviewing on behalf of titusfortner May 11, 2026 21:48 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

titusfortner force-pushed the fix_check_targets branch from b026711 to 5836478 Compare May 11, 2026 22:42

qodo-code-review Bot reviewed May 11, 2026

View reviewed changes

Comment thread .github/workflows/ci.yml

[build] check-targets should not use pull_request.base can compare wi…

b82fd93

…th HEAD^1

titusfortner force-pushed the fix_check_targets branch from 5836478 to b82fd93 Compare May 11, 2026 22:49

titusfortner requested a review from Copilot May 11, 2026 23:15

Copilot started reviewing on behalf of titusfortner May 11, 2026 23:16 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

titusfortner changed the title ~~[build] derive PR diff base from HEAD~PR_COMMITS instead of trunk ti~~ [build] derive PR diff base from HEAD^1 instead of trunk tip May 11, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[build] derive PR diff base from HEAD^1 instead of trunk tip#17438

[build] derive PR diff base from HEAD^1 instead of trunk tip#17438
titusfortner wants to merge 1 commit into
trunkfrom
fix_check_targets

titusfortner commented May 11, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented May 11, 2026

Uh oh!

qodo-code-review Bot commented May 11, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

qodo-code-review Bot commented May 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-code-review Bot commented May 11, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

titusfortner commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💥 What does this PR do?

🔄 Types of changes

Uh oh!

qodo-code-review Bot commented May 11, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

qodo-code-review Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

titusfortner commented May 11, 2026 •

edited

Loading

qodo-code-review Bot commented May 11, 2026 •

edited

Loading

qodo-code-review Bot commented May 11, 2026 •

edited

Loading

qodo-code-review Bot commented May 11, 2026 •

edited

Loading