Skip to content

Fix prerelease#17442

Merged
titusfortner merged 5 commits into
trunkfrom
fix-prerelease
May 12, 2026
Merged

Fix prerelease#17442
titusfortner merged 5 commits into
trunkfrom
fix-prerelease

Conversation

@titusfortner
Copy link
Copy Markdown
Member

pre-release failed for a syntax error.

💥 What does this PR do?

  • fixed syntax error
  • avoids asking permission when TLC member is the one who kicks off the workflow
  • Notes in Slack message who is requesting to run it (prevent random person doing it)
  • automatically unlocks trunk if there's a failure in pre-release
  • Slack message that pre-release workflow failed

🔧 Implementation Notes

usernames hard coded to avoid needing token permissions

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): claude
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Bug fix (backwards compatible)

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Fix pre-release workflow and add automatic failure recovery

🐞 Bug fix ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Fixed syntax error in pre-release workflow dependency update command
• Added automatic trunk unlock on pre-release failure with Slack notification
• Skip approval step for TLC members and automatic recovery workflows
• Enhanced Slack notifications with requester identification and failure details
• Fixed Slack lock/unlock icon logic in restrict-trunk workflow
Diagram
flowchart LR
  A["Pre-release Workflow"] -->|"Failure"| B["Unlock Trunk on Failure"]
  A -->|"Failure"| C["Send Failure Notification"]
  B --> D["Restrict Trunk Workflow"]
  D -->|"skip_approval=true"| E["Manage Trunk"]
  F["Get Approval"] -->|"TLC Member Check"| G["Skip Approval"]
  G --> H["Slack Notification with Requester"]
Loading

Grey Divider

File Changes

1. .github/workflows/get-approval.yml ✨ Enhancement +3/-1

Skip approval for TLC members and add requester info

• Added condition to skip approval notification for TLC members using hardcoded username list
• Enhanced Slack message to include the triggering actor (requester) information
• Added conditional check to ensure authorize job only runs after successful notification

.github/workflows/get-approval.yml


2. .github/workflows/pre-release.yml 🐞 Bug fix +38/-1

Fix syntax error and add failure recovery jobs

• Fixed syntax error in dependency update command by removing conditional suffix
• Added new unlock-trunk-on-failure job to automatically unlock trunk on workflow failure
• Added new on-prerelease-failure job to send detailed Slack notification with job status details
• Both failure recovery jobs only trigger if restrict-trunk job succeeded

.github/workflows/pre-release.yml


3. .github/workflows/restrict-trunk.yml ✨ Enhancement +11/-8

Add skip_approval parameter and fix notification logic

• Added new skip_approval input parameter for automatic failure recovery workflows
• Updated get-approval job condition to respect skip_approval flag
• Updated manage-trunk job condition to proceed when skip_approval is true
• Fixed Slack notification logic to use inputs.restrict directly instead of environment variable

.github/workflows/restrict-trunk.yml


Grey Divider

Qodo Logo

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

qodo-code-review Bot commented May 12, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. Unlock gated on create-pr 📘 Rule violation ☼ Reliability
Description
unlock-trunk-on-failure only runs when needs.create-pr.result != 'success', so if the workflow
fails after create-pr succeeds, trunk may remain locked with no rollback. This violates the
requirement that cleanup/rollback executes on all failure paths.
Code

.github/workflows/pre-release.yml[342]

+    if: always() && needs.restrict-trunk.result == 'success' && needs.create-pr.result != 'success'
Evidence
Compliance ID 12 requires cleanup/rollback to run on all failure paths. The current condition ties
rollback solely to create-pr outcome, which does not cover failures occurring after PR creation.

.github/workflows/pre-release.yml[339-346]
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 `unlock-trunk-on-failure` job is intended to rollback trunk restriction when pre-release fails, but its `if:` condition only checks `needs.create-pr.result != 'success'`. If a later job fails after `create-pr` succeeds, trunk can remain restricted.

## Issue Context
This is a failure-recovery/rollback path and should trigger for any failure/cancellation after `restrict-trunk` has successfully restricted trunk.

## Fix Focus Areas
- .github/workflows/pre-release.yml[339-346]

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


2. Approval workflow skipped ✓ Resolved 🐞 Bug ≡ Correctness
Description
For whitelisted github.triggering_actor values, get-approval.yml skips notify, and authorize
is gated on needs.notify.result == 'success', so both jobs are skipped and the reusable workflow
can conclude as skipped instead of success. restrict-trunk.yml only proceeds with trunk
management when the reusable get-approval job result is success (unless skip_approval is set),
which can block trunk restriction at the start of pre-release/release workflows.
Code

.github/workflows/get-approval.yml[R38-41]

  authorize:
    name: Authorize
    needs: notify
+    if: needs.notify.result == 'success'
Evidence
get-approval.yml currently has no job that runs for whitelisted actors (notify is skipped;
authorize requires notify success), while restrict-trunk.yml requires the reusable approval job to
be success to proceed with trunk management for restrict: true calls (like in pre-release).

.github/workflows/get-approval.yml[20-46]
.github/workflows/restrict-trunk.yml[38-55]
.github/workflows/pre-release.yml[76-89]

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

## Issue description
Whitelisted actors cause the `Get Approval` reusable workflow to run zero jobs (notify is skipped; authorize is skipped because it requires notify success). This can make the reusable workflow job conclude as `skipped`, which downstream workflows treat as not-approved.

## Issue Context
- `restrict-trunk.yml` requires `needs.get-approval.result == 'success'` to run `manage-trunk` when `restrict: true`.
- `pre-release.yml` (and release flows) call `restrict-trunk.yml` with `restrict: true`, so a `skipped` approval job can prevent trunk restriction and cascade into the rest of the workflow being skipped.

## Fix Focus Areas
- .github/workflows/get-approval.yml[20-46]
- .github/workflows/restrict-trunk.yml[38-55]
- .github/workflows/pre-release.yml[76-89]

## Suggested change
Create an explicit success path for whitelisted actors (e.g., an `auto-authorize` job without an environment) so the called workflow concludes `success` when the actor is in the whitelist.

Example structure:
- Keep `notify` only for non-whitelisted actors.
- Keep `authorize` (with `environment: production`) only for non-whitelisted actors.
- Add `auto-authorize` (no environment) for whitelisted actors.

This ensures exactly one of `authorize` / `auto-authorize` runs, and the reusable workflow result is `success` in both cases.

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



Remediation recommended

3. Allowlist case mismatch 🐞 Bug ≡ Correctness
Description
needs-authorization computes required using a case-sensitive `contains([...],
github.triggering_actor)`, so allowlisted users whose GitHub login casing differs from the hardcoded
entries will be treated as unauthorized (triggering approval + Slack notify). This breaks the
intended “skip approval for TLC member” behavior for those users.
Code

.github/workflows/get-approval.yml[25]

+      required: ${{ !contains(fromJSON('["AutomatedTester","jimevans","p0deje","titusfortner","bonigarcia","diemol","pujagani","harsha509"]'), github.triggering_actor) }}
Evidence
The allowlist uses a direct contains([...], github.triggering_actor) comparison with mixed-case
entries and no normalization, so any casing difference causes the allowlist match to fail and
required to become true.

.github/workflows/get-approval.yml[21-27]

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 allowlist check is case-sensitive, which can incorrectly require approval for allowlisted users if `github.triggering_actor` casing doesn’t match the hardcoded list.

### Issue Context
`contains()` performs exact string matching, and the list currently includes mixed-case entries (e.g., `AutomatedTester`) while `github.triggering_actor` may present a different casing.

### Fix
Normalize both sides (e.g., compare lowercased values).

Example:
```yaml
outputs:
 required: ${{ !contains(fromJSON('["automatedtester","jimevans","p0deje","titusfortner","bonigarcia","diemol","pujagani","harsha509"]'), toLower(github.triggering_actor)) }}
```

### Fix Focus Areas
- .github/workflows/get-approval.yml[25-25]

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


4. No unlock on cancel ✓ Resolved 🐞 Bug ☼ Reliability
Description
unlock-trunk-on-failure only runs when failure() is true, so if the pre-release workflow is
cancelled after restrict-trunk succeeds, the auto-unlock path will not trigger and trunk can
remain restricted. This can leave the repo locked until someone manually runs the Unlock Trunk
workflow.
Code

.github/workflows/pre-release.yml[R339-343]

+  unlock-trunk-on-failure:
+    name: Unlock Trunk on Failure
+    needs: [parse-tag, restrict-trunk, generate-rust-version, push-rust-version, selenium-manager, update-versions, commit-versions, update-devtools, commit-devtools, update-dependencies, calculate-changelog-depth, update-changelogs, release-updates, create-pr]
+    if: failure() && needs.restrict-trunk.result == 'success'
+    uses: ./.github/workflows/restrict-trunk.yml
Evidence
The trunk unlock job is guarded by failure() only, and the workflow clearly restricts trunk
earlier; therefore cancellation after restriction will not satisfy the unlock job condition.

.github/workflows/pre-release.yml[76-89]
.github/workflows/pre-release.yml[339-349]

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 auto-unlock job in pre-release only triggers on failures (`failure()`), not on cancellations, so a cancelled run after trunk is restricted can leave trunk locked.

## Issue Context
Trunk restriction is applied early in the workflow; cancellation mid-run is a plausible operational scenario (manual cancel, timeouts, concurrency cancellations), and leaving trunk restricted requires manual intervention.

## Fix Focus Areas
- .github/workflows/pre-release.yml[339-349]

## Suggested change
Adjust the condition to also cover cancellations, e.g.:
- `if: (failure() || cancelled()) && needs.restrict-trunk.result == 'success'`

If you find job-level cancellation prevents this job from starting in your environment, consider moving unlock into a separate `workflow_run` cleanup workflow that triggers on completion of the pre-release workflow and unlocks when the conclusion is `failure` or `cancelled` and trunk had been restricted.

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


Grey Divider

Qodo Logo

Comment thread .github/workflows/get-approval.yml Outdated
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 12, 2026

Persistent review updated to latest commit 3a58156

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

Updates Selenium’s GitHub Actions release-prep workflows to fix a syntax error, streamline trunk lock/unlock approvals, and improve failure handling/notifications during pre-release.

Changes:

  • Fixes the dependency update command in the pre-release workflow.
  • Adds an option to skip the approval gate when automatically unlocking trunk (e.g., failure recovery).
  • Adds authorization logic and improved Slack attribution for approval requests, plus new Slack notifications for pre-release failures.

Reviewed changes

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

File Description
.github/workflows/restrict-trunk.yml Adds skip_approval input and adjusts job gating + Slack notification templating for trunk lock/unlock flows.
.github/workflows/pre-release.yml Fixes the ./go ...:update invocation and adds failure-handling jobs (auto-unlock trunk + Slack failure notice).
.github/workflows/get-approval.yml Adds a TLC allowlist-based authorization check to skip environment approval and appends requester info to Slack messages.

Comment thread .github/workflows/get-approval.yml Outdated
Comment thread .github/workflows/get-approval.yml
@titusfortner titusfortner requested a review from Copilot May 12, 2026 16:44
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 12, 2026

Persistent review updated to latest commit 90adfe4

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 3 out of 3 changed files in this pull request and generated no new comments.

Comment thread .github/workflows/pre-release.yml
@titusfortner titusfortner merged commit d56867b into trunk May 12, 2026
31 checks passed
@titusfortner titusfortner deleted the fix-prerelease branch May 12, 2026 16:53
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