Skip to content

[build] update CDP from daily pin browser workflow - #17872

Merged
titusfortner merged 3 commits into
trunkfrom
pin-browsers-cdp
Aug 4, 2026
Merged

[build] update CDP from daily pin browser workflow#17872
titusfortner merged 3 commits into
trunkfrom
pin-browsers-cdp

Conversation

@titusfortner

Copy link
Copy Markdown
Member

💥 What does this PR do?

  • Moves CDP generation out of the build release preparation and into daily browser update workflow
  • Keeps pinned browsers in sync with CDP versions

🔧 Implementation Notes

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: the workflow changes and the update_browsers.sh script
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

  • We probably also want to move dependency updates out of build process in a future PR

🔄 Types of changes

  • New feature (non-breaking change which adds functionality)

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Aug 4, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Run full Ruby CI on major pinned-browser bumps and regen CDP in daily workflow

⚙️ Configuration changes 🧪 Tests ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Regenerate Chrome DevTools Protocol (CDP) during daily pinned browser updates when needed.
• Propagate update script outputs to label automated PRs and drive CI behavior.
• Run the full Ruby test matrix only on major Chrome/Firefox bumps; otherwise smoke tests.
Diagram

graph TD
  Pin["pin-browsers.yml (daily)"] --> Upd["update_browsers.sh"] --> Pinned["bazel //scripts:pinned_browsers"] --> Out["output tags (major/cdp)"]
  Upd -- "stable major w/out devtools" --> CDP["bazel //scripts:update_cdp"] --> Out
  Out --> PR["Automated PR title/body"]
  Out --> CI["ci.yml"] --> Ruby["ci-ruby.yml (smoke/full)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Emit structured bump metadata from pinned_browsers/update_cdp
  • ➕ Avoids parsing repositories.bzl with grep/regex
  • ➕ Keeps "what changed" decisions close to the scripts that perform the update
  • ➕ Easier to test than shell text processing
  • ➖ Requires changing the Bazel scripts and their invocation contracts
  • ➖ More up-front refactor work than the current workflow-only approach
2. Always regenerate CDP on the daily browser update
  • ➕ Simpler logic (no devtools-dir existence check)
  • ➕ Guarantees CDP freshness even if a folder exists but content is stale
  • ➖ More CI time and network usage on most runs
  • ➖ Increases the chance of transient failures blocking routine browser bumps
3. Always run the full Ruby matrix for pinned-browser update PRs
  • ➕ Maximum confidence for all browser bumps
  • ➕ Eliminates the need to detect major bumps
  • ➖ Significantly higher CI cost and longer feedback for routine patch/build bumps

Recommendation: Current approach is a good trade-off: it prevents Chrome major bumps from landing ahead of CDP and limits full Ruby CI to the higher-risk major bumps. If the grep-based major detection ever proves brittle, the best follow-up would be to have the Bazel scripts emit explicit metadata (e.g., JSON) rather than inferring change type from repositories.bzl text.

Files changed (5) +68 / -6

Other (5) +68 / -6
bazel.ymlExpose reusable Bazel workflow outputs +6/-0

Expose reusable Bazel workflow outputs

• Adds a workflow_call output and forwards the 'run-bazel' step output so callers can consume values written to $GITHUB_OUTPUT by the invoked run script.

.github/workflows/bazel.yml

ci-ruby.ymlMake Ruby CI smoke/full selectable for workflow_call +9/-3

Make Ruby CI smoke/full selectable for workflow_call

• Introduces a 'smoke' boolean input for workflow_call and simplifies job gating so scheduled runs always execute the full set while callers can request smoke-only vs full matrix.

.github/workflows/ci-ruby.yml

ci.ymlRun full Ruby matrix for major pinned-browser PRs +2/-0

Run full Ruby matrix for major pinned-browser PRs

• Passes 'smoke: false' to Ruby CI only when the PR comes from the pinned-browser automation branch and the title indicates a major bump, otherwise defaulting to smoke tests.

.github/workflows/ci.yml

pin-browsers.ymlMove pinned-browser update logic to update_browsers.sh and annotate PRs +3/-3

Move pinned-browser update logic to update_browsers.sh and annotate PRs

• Replaces the direct Bazel invocation with a wrapper script and uses the job output tags to conditionally append '(major)' and 'with CDP' to the automated PR title/body.

.github/workflows/pin-browsers.yml

update_browsers.shAdd pinned-browser update wrapper that regenerates CDP and emits tags +48/-0

Add pinned-browser update wrapper that regenerates CDP and emits tags

• New script runs the pinned browser update, regenerates CDP when the new stable Chrome major has no checked-in devtools directory, detects Chrome/Firefox major bumps by comparing old/new repositories.bzl, and writes space-separated tags ('major', 'cdp') to $GITHUB_OUTPUT.

scripts/github-actions/update_browsers.sh

@qodo-code-review

qodo-code-review Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. CDP regen wrong major ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New
Description
update_browsers.sh decides CDP regeneration based on the pinned stable Chrome major parsed from
common/repositories.bzl, but the regeneration command resolves the current channel "Stable" at
runtime and may generate a different major if Stable advances mid-workflow. Because the script
doesn’t verify that common/devtools/chromium/v${chrome} exists after regeneration, it can still
create a PR that pins Chrome ahead of its checked-in DevTools.
Code

scripts/github-actions/update_browsers.sh[R40-43]

+if [ -d "common/devtools/chromium/v${chrome}" ]; then
+  echo "DevTools for Chrome v${chrome} already present; skipping CDP regeneration"
+else
+  echo "No DevTools for Chrome v${chrome}; regenerating CDP"
Evidence
The workflow’s expected major is derived from pinned URLs in repositories.bzl, but the regeneration
script independently resolves the Stable milestone from network metadata; without verifying the
expected v${chrome} directory exists after regeneration, a mid-run Stable bump can leave the pinned
major without a matching checked-in DevTools directory while still allowing PR creation.

scripts/github-actions/update_browsers.sh[28-46]
scripts/update_cdp.py[17-43]
scripts/update_cdp.py[87-99]
scripts/pinned_browsers.py[552-560]

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

## Issue description
`scripts/github-actions/update_browsers.sh` regenerates CDP when `common/devtools/chromium/v${chrome}` is missing, but `//scripts:update_cdp` independently discovers the current Stable milestone at runtime. If Stable advances between `pinned_browsers` and `update_cdp`, the regen can produce `v<new>` while `v${chrome}` remains missing; the script then continues and writes `output=...`, allowing a PR to be created with pinned Chrome ahead of checked-in DevTools.

## Issue Context
- The script computes `chrome` from `common/repositories.bzl` (post-`pinned_browsers`).
- CDP regeneration is currently driven by `--chrome_channel=Stable` (runtime lookup), not by the parsed `chrome` major.

## Fix Focus Areas
- scripts/github-actions/update_browsers.sh[28-46]
- scripts/update_cdp.py[17-43]

### Suggested remediation (either option)
1) **Postcondition check (minimal change):** After `bazel run //scripts:update_cdp ...`, assert that `common/devtools/chromium/v${chrome}` now exists and is non-empty; if not, emit a `::error::` and `exit 1`.
2) **Make regen deterministic (stronger):** Extend `scripts/update_cdp.py` to accept an explicit Chrome version/major (or read it from `common/repositories.bzl`), and have `update_browsers.sh` pass the parsed `chrome` value so the generated directory must match the newly pinned major.

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


2. No logging in update_browsers.sh ✓ Resolved 📘 Rule violation ◔ Observability
Description
The new update_browsers.sh script conditionally regenerates CDP and sets PR-impacting output tags,
but it emits no explicit log lines explaining what decision was taken. This reduces troubleshooting
visibility in CI when a browser bump is blocked or when the PR is labeled as (major)/with CDP.
Code

scripts/github-actions/update_browsers.sh[R32-35]

+if [ ! -d "common/devtools/chromium/v${chrome}" ]; then
+  bazel run //scripts:update_cdp -- --chrome_channel=Stable
+  regen_cdp=true
+fi
Evidence
PR Compliance ID 5 requires adding user-relevant logging where operational visibility aids
understanding. The new script runs //scripts:update_cdp based on a directory check and computes
the space-separated output tags written to $GITHUB_OUTPUT, but it does not print any
echo/status lines describing whether CDP regeneration occurred or which tags were set.

AGENTS.md: Add User-Relevant Logging Where It Aids Understanding
scripts/github-actions/update_browsers.sh[32-35]
scripts/github-actions/update_browsers.sh[45-48]

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

## Issue description
`scripts/github-actions/update_browsers.sh` performs important conditional actions (CDP regeneration and “major” detection) but doesn’t print explicit status messages, making CI runs harder to understand.

## Issue Context
This script is executed by the pinned browsers workflow and its decisions influence whether CDP is regenerated and how the created PR is labeled via the `output` tags.

## Fix Focus Areas
- scripts/github-actions/update_browsers.sh[28-48]

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


3. Brittle major parsing ✓ Resolved 🐞 Bug ☼ Reliability
Description
In scripts/github-actions/update_browsers.sh, majors_for() is a grep pipeline executed under `set
-euo pipefail`, so any missing match (or format change in common/repositories.bzl) will terminate
the workflow immediately and prevent the pinned-browser PR from being created. This failure mode is
hard to diagnose because the script doesn’t validate/print which pattern/file produced an empty
major set before exiting.
Code

scripts/github-actions/update_browsers.sh[18]

+majors_for() { grep -oE "$2" "$1" | grep -oE '[0-9]+$' | sort -un; }
Evidence
The script enables set -euo pipefail and defines majors_for() as a grep pipeline; majors_for()
is then used to derive chrome_majors and to compare old/new major sets. Since the major extraction
depends on URL markers inside common/repositories.bzl, any mismatch causes the pipeline to return
non-zero and abort the workflow before it can proceed.

scripts/github-actions/update_browsers.sh[9-43]
common/repositories.bzl[13-17]
common/repositories.bzl[201-205]

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 new `majors_for()` helper is implemented as a `grep | grep | sort` pipeline and is executed with `set -euo pipefail`. If the pattern fails to match (or stops matching due to a repositories.bzl format change), the script exits early and the scheduled pin-browsers workflow cannot open its update PR, without an explicit/targeted error message.

## Issue Context
This script is now the entrypoint for the daily `pin-browsers` workflow and is expected to be resilient and diagnosable when upstream URL formats change.

## Fix Focus Areas
- scripts/github-actions/update_browsers.sh[9-43]

Suggested implementation direction:
- Make `majors_for()` not fail implicitly on “no matches” (e.g., tolerate empty results), and instead add explicit checks after parsing (especially for Chrome stable major) that emit a clear `::error::` message including the file path + pattern.
- Ensure `chrome` is non-empty before using it in the devtools directory check, and fail with a clear message if it cannot be derived.

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


To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Previous review results

Review updated until commit c794ab9

Results up to commit 7bc0df1 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Brittle major parsing ✓ Resolved 🐞 Bug ☼ Reliability
Description
In scripts/github-actions/update_browsers.sh, majors_for() is a grep pipeline executed under `set
-euo pipefail`, so any missing match (or format change in common/repositories.bzl) will terminate
the workflow immediately and prevent the pinned-browser PR from being created. This failure mode is
hard to diagnose because the script doesn’t validate/print which pattern/file produced an empty
major set before exiting.
Code

scripts/github-actions/update_browsers.sh[18]

+majors_for() { grep -oE "$2" "$1" | grep -oE '[0-9]+$' | sort -un; }
Evidence
The script enables set -euo pipefail and defines majors_for() as a grep pipeline; majors_for()
is then used to derive chrome_majors and to compare old/new major sets. Since the major extraction
depends on URL markers inside common/repositories.bzl, any mismatch causes the pipeline to return
non-zero and abort the workflow before it can proceed.

scripts/github-actions/update_browsers.sh[9-43]
common/repositories.bzl[13-17]
common/repositories.bzl[201-205]

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 new `majors_for()` helper is implemented as a `grep | grep | sort` pipeline and is executed with `set -euo pipefail`. If the pattern fails to match (or stops matching due to a repositories.bzl format change), the script exits early and the scheduled pin-browsers workflow cannot open its update PR, without an explicit/targeted error message.

## Issue Context
This script is now the entrypoint for the daily `pin-browsers` workflow and is expected to be resilient and diagnosable when upstream URL formats change.

## Fix Focus Areas
- scripts/github-actions/update_browsers.sh[9-43]

Suggested implementation direction:
- Make `majors_for()` not fail implicitly on “no matches” (e.g., tolerate empty results), and instead add explicit checks after parsing (especially for Chrome stable major) that emit a clear `::error::` message including the file path + pattern.
- Ensure `chrome` is non-empty before using it in the devtools directory check, and fail with a clear message if it cannot be derived.

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


2. No logging in update_browsers.sh ✓ Resolved 📘 Rule violation ◔ Observability
Description
The new update_browsers.sh script conditionally regenerates CDP and sets PR-impacting output tags,
but it emits no explicit log lines explaining what decision was taken. This reduces troubleshooting
visibility in CI when a browser bump is blocked or when the PR is labeled as (major)/with CDP.
Code

scripts/github-actions/update_browsers.sh[R32-35]

+if [ ! -d "common/devtools/chromium/v${chrome}" ]; then
+  bazel run //scripts:update_cdp -- --chrome_channel=Stable
+  regen_cdp=true
+fi
Evidence
PR Compliance ID 5 requires adding user-relevant logging where operational visibility aids
understanding. The new script runs //scripts:update_cdp based on a directory check and computes
the space-separated output tags written to $GITHUB_OUTPUT, but it does not print any
echo/status lines describing whether CDP regeneration occurred or which tags were set.

AGENTS.md: Add User-Relevant Logging Where It Aids Understanding
scripts/github-actions/update_browsers.sh[32-35]
scripts/github-actions/update_browsers.sh[45-48]

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

## Issue description
`scripts/github-actions/update_browsers.sh` performs important conditional actions (CDP regeneration and “major” detection) but doesn’t print explicit status messages, making CI runs harder to understand.

## Issue Context
This script is executed by the pinned browsers workflow and its decisions influence whether CDP is regenerated and how the created PR is labeled via the `output` tags.

## Fix Focus Areas
- scripts/github-actions/update_browsers.sh[28-48]

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


Qodo Logo

Comment thread scripts/github-actions/update_browsers.sh Outdated
Comment thread scripts/github-actions/update_browsers.sh
Comment thread scripts/github-actions/update_browsers.sh
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 7ed3314

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit c794ab9

@titusfortner titusfortner changed the title [build] run full Ruby tests on major browser PRs and regenerate CDP in the daily browser workflow [build] update CDP from daily pin browser workflow Aug 4, 2026
@titusfortner
titusfortner merged commit a421014 into trunk Aug 4, 2026
27 checks passed
@titusfortner
titusfortner deleted the pin-browsers-cdp branch August 4, 2026 22:09
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.

2 participants