Skip to content

[build] Resolve Chrome milestone from Chrome-for-Testing channel version#17747

Open
titusfortner wants to merge 3 commits into
trunkfrom
resolve-chrome-milestone
Open

[build] Resolve Chrome milestone from Chrome-for-Testing channel version#17747
titusfortner wants to merge 3 commits into
trunkfrom
resolve-chrome-milestone

Conversation

@titusfortner

@titusfortner titusfortner commented Jul 3, 2026

Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

The devtools and pinned-browser updaters resolved the Chrome milestone from the chromiumdash release-event feed, taking the lowest of the latest push per platform. When an N-1 security respin lands after milestone N starts shipping, that feed reports N-1, so both updaters silently stuck on the previous version (e.g. the Update Devtools job produced no changes while Chrome 150 was already stable).

Both now resolve the milestone from Chrome-for-Testing's channel designation, which tracks the latest stable/beta milestone and is unaffected by respins.

🔧 Implementation Notes

The resolver is small and lives independently in each script. The two are likely to diverge over time (pinning may want to be conservative; devtools may want to lead via --chrome_channel), so duplication is preferred over a shared module that would become a poor fit.

Also hardens fetches to fail loudly on a non-200: previously a listed-but-404 download would pin a wrong sha256, and a missing protocol path would write a 404 body into a .pdl.

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: milestone-resolution fix and fetch hardening
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

pinned_browsers will roll Chrome/ChromeDriver to the new stable milestone on its next run (a real version bump, not just a code change). Follow-up worth considering: update_cdp currently can't distinguish "already current" from "failed to update" and prints a success message unconditionally.

🔄 Types of changes

  • Bug fix (backwards compatible)

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

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fix Chrome milestone resolution: conservative pinning, latest devtools

🐞 Bug fix ✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Split Chrome milestone resolution by purpose: conservative pinning vs latest devtools.
• Share milestone→download lookup via a new scripts/chrome_version.py Bazel py_library.
• Fail fast on non-200 downloads to avoid silent bad pins and corrupted protocol files.
Diagram

graph TD
  A["pinned_browsers.py"] --> C["chrome_version.py"] --> F[("Pinned browser pins")]
  B["update_cdp.py"] --> C --> G[("CDP protocol files")]
  C --> D["ChromiumDash releases"] --> C
  C --> E["Chrome-for-Testing JSON"] --> C
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use Chrome-for-Testing only (drop ChromiumDash for pinning)
  • ➕ Fewer external dependencies/endpoints
  • ➕ Simpler logic and fewer failure modes
  • ➖ Cannot enforce consumer-channel lock-step across pinned platforms
  • ➖ Reintroduces risk of getting ahead of a platform during staggered rollouts
2. Persist last accepted milestone/version in-repo (stateful pinning)
  • ➕ Fully respin-immune and deterministic across runs
  • ➕ Allows explicit promotion criteria and manual override
  • ➖ Adds state management and human/process overhead
  • ➖ More complex automation and review workflow
3. Query OmahaProxy-style endpoints per platform/channel
  • ➕ Purpose-built for consumer-channel platform/version tracking
  • ➕ Potentially more stable semantics than ChromiumDash event feeds
  • ➖ Different data shape; still requires mapping to Chrome-for-Testing downloads
  • ➖ May require additional parsing and platform normalization

Recommendation: Keep the PR’s split-policy approach. It cleanly separates concerns: devtools tracks the latest stable Chrome-for-Testing milestone, while pinning stays conservative and rollout-aware across platforms. The shared chrome_version module reduces duplication while still allowing different selection semantics, which is the core requirement for avoiding N-1 respin regressions without over-complicating the workflow.

Files changed (4) +82 / -63

Bug fix (2) +17 / -61
pinned_browsers.pyUse conservative milestone policy and harden downloads for pinning +7/-25

Use conservative milestone policy and harden downloads for pinning

• Replaces inline ChromiumDash/CfT milestone logic with conservative_for_channel from chrome_version. Adds explicit failure on non-200 download responses and validates chromedriver availability for the selected version.

scripts/pinned_browsers.py

update_cdp.pyTrack latest stable milestone and fail fast on protocol fetch errors +10/-36

Track latest stable milestone and fail fast on protocol fetch errors

• Switches milestone selection to latest_for_channel with a --chrome_channel argument. Adds HTTP status checks to avoid writing 404 bodies into downloaded protocol artifacts and other generated files.

scripts/update_cdp.py

Refactor (1) +52 / -0
chrome_version.pyCentralize Chrome milestone resolution and CfT version lookup +52/-0

Centralize Chrome milestone resolution and CfT version lookup

• Adds shared functions to map a milestone to the newest Chrome-for-Testing version+downloads entry. Provides two policies: latest_for_channel (tracks CfT last-known-good) and conservative_for_channel (lock-step milestone across pinned platforms using ChromiumDash).

scripts/chrome_version.py

Other (1) +13 / -2
BUILD.bazelAdd chrome_version py_library and wire scripts to it +13/-2

Add chrome_version py_library and wire scripts to it

• Introduces a new Bazel py_library target for chrome_version.py and adds it as a dependency for pinned_browsers and update_cdp. Updates rules_python load to include py_library.

scripts/BUILD.bazel

@qodo-code-review

qodo-code-review Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 11 rules

Grey Divider


Action required

1. Milestone lookup IndexError ✓ Resolved 🐞 Bug ☼ Reliability
Description
chrome_for_milestone() indexes the filtered version list with [-1] and never checks HTTP status, so
missing milestones or non-JSON upstream responses will fail with IndexError/JSONDecodeError instead
of a clear, actionable error.
Code

scripts/chrome_version.py[R18-25]

+def chrome_for_milestone(milestone):
+    """Return the newest Chrome-for-Testing version entry (version + downloads) for a milestone."""
+    r = http.request("GET", KNOWN_GOOD_WITH_DOWNLOADS)
+    versions = json.loads(r.data)["versions"]
+    return sorted(
+        filter(lambda v: v["version"].split(".")[0] == str(milestone), versions),
+        key=lambda v: parse(v["version"]),
+    )[-1]
Evidence
The new resolver performs HTTP GETs and immediately parses/selects data without checking status, and
it assumes at least one matching milestone exists by indexing the last element of a possibly-empty
list.

scripts/chrome_version.py[18-25]
scripts/chrome_version.py[28-35]
scripts/chrome_version.py[38-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
`scripts/chrome_version.py` fetches JSON from upstream endpoints and immediately parses/selects data without validating HTTP status codes or guarding against an empty milestone match. This can crash automation with `IndexError` (no matching milestone) or `JSONDecodeError` (e.g., HTML error body), producing low-signal failures.

### Issue Context
This module is shared by `pinned_browsers` and `update_cdp`, so an unclear failure here blocks scheduled pinning and release workflows.

### Fix Focus Areas
- scripts/chrome_version.py[18-25]
- scripts/chrome_version.py[28-35]
- scripts/chrome_version.py[38-52]

### What to change
- Check `r.status` for each `http.request(...)` and raise `ValueError` with the URL and status when non-200.
- In `chrome_for_milestone`, materialize the filtered list and raise a `ValueError` like `No Chrome-for-Testing versions found for milestone X` rather than indexing `[-1]`.
- Optionally, validate expected JSON shape (e.g., presence of `versions` / `channels[channel]`) to fail with clearer errors.

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


2. No tests for chrome_version 📘 Rule violation ▣ Testability
Description
Milestone resolution and failure handling behavior was added/changed, but no automated tests were
added/updated to cover latest_for_channel()/conservative_for_channel() and their HTTP/error
paths. This risks regressions that can silently break Chrome pinning and CDP updates.
Code

scripts/chrome_version.py[R18-52]

+def chrome_for_milestone(milestone):
+    """Return the newest Chrome-for-Testing version entry (version + downloads) for a milestone."""
+    r = http.request("GET", KNOWN_GOOD_WITH_DOWNLOADS)
+    versions = json.loads(r.data)["versions"]
+    return sorted(
+        filter(lambda v: v["version"].split(".")[0] == str(milestone), versions),
+        key=lambda v: parse(v["version"]),
+    )[-1]
+
+
+def latest_for_channel(channel):
+    """Newest version Chrome-for-Testing designates for a channel (e.g. "Stable", "Beta").
+
+    Adopts a new milestone as soon as its binaries ship, without waiting for consumer rollout.
+    """
+    r = http.request("GET", LAST_KNOWN_GOOD)
+    milestone = json.loads(r.data)["channels"][channel]["version"].split(".")[0]
+    return chrome_for_milestone(milestone)
+
+
+def conservative_for_channel(channel):
+    """Newest milestone that has reached the consumer channel on every pinned platform.
+
+    Per-platform we take the highest milestone seen (respin-immune: an N-1 security respin
+    can't hide that the platform also has N), then the lowest across platforms so a staggered
+    rollout steps back to N-1 until every platform has N.
+    """
+    milestones = []
+    for platform in PINNED_PLATFORMS:
+        r = http.request("GET", f"{RELEASES}?channel={channel}&num=20&platform={platform}")
+        seen = [release["milestone"] for release in json.loads(r.data) if release["milestone"]]
+        if not seen:
+            raise ValueError(f"No {channel} milestone found for platform '{platform}'")
+        milestones.append(max(seen))
+    return chrome_for_milestone(min(milestones))
Evidence
PR Compliance ID 389273 requires automated tests for new functionality and bug fixes. The PR
introduces new milestone-selection behavior in scripts/chrome_version.py and changes consumers to
use it, but no corresponding test changes are present in the change set.

Rule 389273: Require tests for all new functionality and bug fixes
scripts/chrome_version.py[18-52]
scripts/pinned_browsers.py[533-544]
scripts/update_cdp.py[16-22]

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

## Issue description
New/changed Chrome milestone resolution logic is untested, despite being a bug fix that affects build/update outputs.

## Issue Context
This PR adds `scripts/chrome_version.py` and changes `pinned_browsers.py` and `update_cdp.py` to rely on it (including new non-200 failure behavior). Add automated tests that would fail if the old (stuck-on-N-1) behavior returns, and that validate error handling for missing milestones / missing downloads.

## Fix Focus Areas
- scripts/chrome_version.py[18-52]
- scripts/pinned_browsers.py[21-28]
- scripts/pinned_browsers.py[533-544]
- scripts/update_cdp.py[16-22]
- scripts/update_cdp.py[47-54]

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



Remediation recommended

3. --chrome_channel restricts values ✓ Resolved 📘 Rule violation ≡ Correctness
Description
The new choices=["Stable", "Beta", "Dev", "Canary"] constraint changes the script’s CLI contract
by rejecting any other previously accepted --chrome_channel values (e.g., case variants used in
automation). This can break existing callers and violates the backward-compatibility requirement for
public interfaces.
Code

scripts/update_cdp.py[R210-215]

+    parser.add_argument(
+        "--chrome_channel",
+        default="Stable",
+        choices=["Stable", "Beta", "Dev", "Canary"],
+        help="Set the Chrome channel (use Beta for early stable)",
+    )
Evidence
PR Compliance ID 389266 requires backward-compatible public interfaces. The updated argument
definition adds strict choices validation for --chrome_channel, which narrows accepted inputs
and can break existing callers.

Rule 389266: Maintain backward-compatible public API and ABI
scripts/update_cdp.py[210-215]

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/update_cdp.py` now enforces a fixed set of `--chrome_channel` values via `choices=...`, which can break existing automation that previously passed other (but functionally equivalent) values such as different casing.

## Issue Context
This script is commonly used in CI/automation; CLI flags are a de facto public interface. The new strict validation is a behavior change that can be made backward-compatible by normalizing inputs (e.g., case-insensitive mapping) or expanding accepted aliases.

## Fix Focus Areas
- scripts/update_cdp.py[210-215]

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


4. Unchecked milestone HTTP status ✓ Resolved 🐞 Bug ☼ Reliability
Description
latest_for_channel() fetches Chrome-for-Testing JSON and immediately parses it without checking
response status, so upstream 404/500/proxy error bodies can surface as JSONDecodeError/KeyError
instead of a clear “fetch failed” exception. This reduces debuggability and is inconsistent with the
newly-hardened fetch helpers elsewhere in the same scripts.
Code

scripts/pinned_browsers.py[R36-39]

+    r = http.request("GET", "https://googlechromelabs.github.io/chrome-for-testing/last-known-good-versions.json")
+    milestone = json.loads(r.data)["channels"][channel]["version"].split(".")[0]
+    r = http.request("GET", "https://googlechromelabs.github.io/chrome-for-testing/known-good-versions-with-downloads.json")
+    versions = json.loads(r.data)["versions"]
Evidence
Both new latest_for_channel() implementations parse JSON from last-known-good-versions.json and
known-good-versions-with-downloads.json without any status checks, while nearby helpers in the
same files explicitly raise on non-200 responses.

scripts/pinned_browsers.py[30-39]
scripts/update_cdp.py[17-26]
scripts/pinned_browsers.py[19-24]
scripts/update_cdp.py[33-36]

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

### Issue description
`latest_for_channel()` performs `http.request()` and immediately `json.loads(r.data)` without verifying `r.status`. When Chrome-for-Testing endpoints return non-200 bodies (404, 500, proxy HTML, etc.), the scripts fail with parsing/KeyError exceptions that lack the URL and HTTP status.

### Issue Context
Other fetch paths in these scripts were hardened to explicitly raise on non-200, so this new path should behave similarly to keep failures actionable.

### Fix Focus Areas
- scripts/pinned_browsers.py[30-39]
- scripts/update_cdp.py[17-26]

### Implementation notes
- After each `http.request(...)`, check `resp.status != 200` and raise a `ValueError` that includes HTTP status and URL (similar to `calculate_hash()` / `fetch_and_save()`).
- Consider reading/including a short snippet of the body (optional) if it helps diagnose auth/proxy issues, but don’t log full bodies if they might be large.

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


5. Case-sensitive channel lookup ✓ Resolved 🐞 Bug ☼ Reliability
Description
latest_for_channel() indexes the upstream JSON using the provided channel string without
normalization/validation, so update_cdp can crash with a KeyError when users pass values like
"stable"/"beta" instead of "Stable"/"Beta".
Code

scripts/chrome_version.py[R20-21]

+    r = http.request("GET", LAST_KNOWN_GOOD)
+    milestone = json.loads(r.data)["channels"][channel]["version"].split(".")[0]
Evidence
The channel value is used as a direct key into the JSON response without any canonicalization, and
update_cdp forwards a user-controlled CLI argument into that function, making a KeyError likely for
non-canonical casing.

scripts/chrome_version.py[14-23]
scripts/update_cdp.py[185-191]

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

### Issue description
`latest_for_channel(channel)` performs a case-sensitive dictionary lookup (`channels[channel]`). When `update_cdp.py` passes a user-provided `--chrome_channel` value through unchanged, common casing variants (e.g., `stable`, `BETA`) crash the script with `KeyError`.

### Issue Context
This is a CLI-exposed footgun: the error is not actionable and will look like an internal failure instead of invalid input.

### Fix Focus Areas
- scripts/chrome_version.py[14-27]

### Suggested fix approach
- Normalize channel input (e.g., map lowercase to canonical keys) or validate against the keys returned by `last-known-good-versions.json`.
- On mismatch, raise `ValueError` with a clear message listing allowed channels.

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


View more (1)
6. HTTP response not released ✓ Resolved 🐞 Bug ☼ Reliability
Description
calculate_hash() streams downloads with preload_content=False but never releases the urllib3
connection, which can leak pooled connections across multiple large downloads in a pinning run.
Code

scripts/pinned_browsers.py[R21-28]

    print(f"Calculate hash for {url}", file=sys.stderr)
    h = hashlib.sha256()
    r = http.request("GET", url, preload_content=False)
+    if r.status != 200:
+        raise ValueError(f"Download unavailable (HTTP {r.status}): {url}")
    for b in iter(lambda: r.read(4096), b""):
        h.update(b)
    return h.hexdigest()
Evidence
pinned_browsers streams a response without releasing it, while another script in this repo uses the
same pattern and explicitly calls release_conn(), indicating the intended cleanup behavior.

scripts/pinned_browsers.py[20-28]
scripts/update_docfx.py[53-59]

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/pinned_browsers.py:calculate_hash()` streams response bodies (`preload_content=False`) but does not call `release_conn()` (or close the response) after reading. This can retain connections in the pool longer than needed and potentially exhaust the pool during the script's many downloads.

### Issue Context
The repo already has a similar streaming download pattern that explicitly releases the connection.

### Fix Focus Areas
- scripts/pinned_browsers.py[20-28]

### What to change
- Wrap the streaming read in `try/finally` and call `r.release_conn()` in the `finally` block.
- If `r.status != 200`, release the connection before raising.

Example structure:
```py
r = http.request(..., preload_content=False)
try:
   if r.status != 200:
       raise ...
   for chunk in ...:
       ...
   return ...
finally:
   r.release_conn()
```

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


Grey Divider

Previous review results

Review updated until commit ebd4d00

Results up to commit e907606


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


Action required
1. No tests for chrome_version 📘 Rule violation ▣ Testability
Description
Milestone resolution and failure handling behavior was added/changed, but no automated tests were
added/updated to cover latest_for_channel()/conservative_for_channel() and their HTTP/error
paths. This risks regressions that can silently break Chrome pinning and CDP updates.
Code

scripts/chrome_version.py[R18-52]

+def chrome_for_milestone(milestone):
+    """Return the newest Chrome-for-Testing version entry (version + downloads) for a milestone."""
+    r = http.request("GET", KNOWN_GOOD_WITH_DOWNLOADS)
+    versions = json.loads(r.data)["versions"]
+    return sorted(
+        filter(lambda v: v["version"].split(".")[0] == str(milestone), versions),
+        key=lambda v: parse(v["version"]),
+    )[-1]
+
+
+def latest_for_channel(channel):
+    """Newest version Chrome-for-Testing designates for a channel (e.g. "Stable", "Beta").
+
+    Adopts a new milestone as soon as its binaries ship, without waiting for consumer rollout.
+    """
+    r = http.request("GET", LAST_KNOWN_GOOD)
+    milestone = json.loads(r.data)["channels"][channel]["version"].split(".")[0]
+    return chrome_for_milestone(milestone)
+
+
+def conservative_for_channel(channel):
+    """Newest milestone that has reached the consumer channel on every pinned platform.
+
+    Per-platform we take the highest milestone seen (respin-immune: an N-1 security respin
+    can't hide that the platform also has N), then the lowest across platforms so a staggered
+    rollout steps back to N-1 until every platform has N.
+    """
+    milestones = []
+    for platform in PINNED_PLATFORMS:
+        r = http.request("GET", f"{RELEASES}?channel={channel}&num=20&platform={platform}")
+        seen = [release["milestone"] for release in json.loads(r.data) if release["milestone"]]
+        if not seen:
+            raise ValueError(f"No {channel} milestone found for platform '{platform}'")
+        milestones.append(max(seen))
+    return chrome_for_milestone(min(milestones))
Evidence
PR Compliance ID 389273 requires automated tests for new functionality and bug fixes. The PR
introduces new milestone-selection behavior in scripts/chrome_version.py and changes consumers to
use it, but no corresponding test changes are present in the change set.

Rule 389273: Require tests for all new functionality and bug fixes
scripts/chrome_version.py[18-52]
scripts/pinned_browsers.py[533-544]
scripts/update_cdp.py[16-22]

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

## Issue description
New/changed Chrome milestone resolution logic is untested, despite being a bug fix that affects build/update outputs.

## Issue Context
This PR adds `scripts/chrome_version.py` and changes `pinned_browsers.py` and `update_cdp.py` to rely on it (including new non-200 failure behavior). Add automated tests that would fail if the old (stuck-on-N-1) behavior returns, and that validate error handling for missing milestones / missing downloads.

## Fix Focus Areas
- scripts/chrome_version.py[18-52]
- scripts/pinned_browsers.py[21-28]
- scripts/pinned_browsers.py[533-544]
- scripts/update_cdp.py[16-22]
- scripts/update_cdp.py[47-54]

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


2. Milestone lookup IndexError ✓ Resolved 🐞 Bug ☼ Reliability
Description
chrome_for_milestone() indexes the filtered version list with [-1] and never checks HTTP status, so
missing milestones or non-JSON upstream responses will fail with IndexError/JSONDecodeError instead
of a clear, actionable error.
Code

scripts/chrome_version.py[R18-25]

+def chrome_for_milestone(milestone):
+    """Return the newest Chrome-for-Testing version entry (version + downloads) for a milestone."""
+    r = http.request("GET", KNOWN_GOOD_WITH_DOWNLOADS)
+    versions = json.loads(r.data)["versions"]
+    return sorted(
+        filter(lambda v: v["version"].split(".")[0] == str(milestone), versions),
+        key=lambda v: parse(v["version"]),
+    )[-1]
Evidence
The new resolver performs HTTP GETs and immediately parses/selects data without checking status, and
it assumes at least one matching milestone exists by indexing the last element of a possibly-empty
list.

scripts/chrome_version.py[18-25]
scripts/chrome_version.py[28-35]
scripts/chrome_version.py[38-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
`scripts/chrome_version.py` fetches JSON from upstream endpoints and immediately parses/selects data without validating HTTP status codes or guarding against an empty milestone match. This can crash automation with `IndexError` (no matching milestone) or `JSONDecodeError` (e.g., HTML error body), producing low-signal failures.

### Issue Context
This module is shared by `pinned_browsers` and `update_cdp`, so an unclear failure here blocks scheduled pinning and release workflows.

### Fix Focus Areas
- scripts/chrome_version.py[18-25]
- scripts/chrome_version.py[28-35]
- scripts/chrome_version.py[38-52]

### What to change
- Check `r.status` for each `http.request(...)` and raise `ValueError` with the URL and status when non-200.
- In `chrome_for_milestone`, materialize the filtered list and raise a `ValueError` like `No Chrome-for-Testing versions found for milestone X` rather than indexing `[-1]`.
- Optionally, validate expected JSON shape (e.g., presence of `versions` / `channels[channel]`) to fail with clearer errors.

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



Remediation recommended
3. HTTP response not released ✓ Resolved 🐞 Bug ☼ Reliability
Description
calculate_hash() streams downloads with preload_content=False but never releases the urllib3
connection, which can leak pooled connections across multiple large downloads in a pinning run.
Code

scripts/pinned_browsers.py[R21-28]

    print(f"Calculate hash for {url}", file=sys.stderr)
    h = hashlib.sha256()
    r = http.request("GET", url, preload_content=False)
+    if r.status != 200:
+        raise ValueError(f"Download unavailable (HTTP {r.status}): {url}")
    for b in iter(lambda: r.read(4096), b""):
        h.update(b)
    return h.hexdigest()
Evidence
pinned_browsers streams a response without releasing it, while another script in this repo uses the
same pattern and explicitly calls release_conn(), indicating the intended cleanup behavior.

scripts/pinned_browsers.py[20-28]
scripts/update_docfx.py[53-59]

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/pinned_browsers.py:calculate_hash()` streams response bodies (`preload_content=False`) but does not call `release_conn()` (or close the response) after reading. This can retain connections in the pool longer than needed and potentially exhaust the pool during the script's many downloads.

### Issue Context
The repo already has a similar streaming download pattern that explicitly releases the connection.

### Fix Focus Areas
- scripts/pinned_browsers.py[20-28]

### What to change
- Wrap the streaming read in `try/finally` and call `r.release_conn()` in the `finally` block.
- If `r.status != 200`, release the connection before raising.

Example structure:
```py
r = http.request(..., preload_content=False)
try:
   if r.status != 200:
       raise ...
   for chunk in ...:
       ...
   return ...
finally:
   r.release_conn()
```

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


Results up to commit 000e450


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


Remediation recommended
1. Case-sensitive channel lookup ✓ Resolved 🐞 Bug ☼ Reliability
Description
latest_for_channel() indexes the upstream JSON using the provided channel string without
normalization/validation, so update_cdp can crash with a KeyError when users pass values like
"stable"/"beta" instead of "Stable"/"Beta".
Code

scripts/chrome_version.py[R20-21]

+    r = http.request("GET", LAST_KNOWN_GOOD)
+    milestone = json.loads(r.data)["channels"][channel]["version"].split(".")[0]
Evidence
The channel value is used as a direct key into the JSON response without any canonicalization, and
update_cdp forwards a user-controlled CLI argument into that function, making a KeyError likely for
non-canonical casing.

scripts/chrome_version.py[14-23]
scripts/update_cdp.py[185-191]

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

### Issue description
`latest_for_channel(channel)` performs a case-sensitive dictionary lookup (`channels[channel]`). When `update_cdp.py` passes a user-provided `--chrome_channel` value through unchanged, common casing variants (e.g., `stable`, `BETA`) crash the script with `KeyError`.

### Issue Context
This is a CLI-exposed footgun: the error is not actionable and will look like an internal failure instead of invalid input.

### Fix Focus Areas
- scripts/chrome_version.py[14-27]

### Suggested fix approach
- Normalize channel input (e.g., map lowercase to canonical keys) or validate against the keys returned by `last-known-good-versions.json`.
- On mismatch, raise `ValueError` with a clear message listing allowed channels.

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


Results up to commit 0cbabf1


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


Remediation recommended
1. Unchecked milestone HTTP status ✓ Resolved 🐞 Bug ☼ Reliability
Description
latest_for_channel() fetches Chrome-for-Testing JSON and immediately parses it without checking
response status, so upstream 404/500/proxy error bodies can surface as JSONDecodeError/KeyError
instead of a clear “fetch failed” exception. This reduces debuggability and is inconsistent with the
newly-hardened fetch helpers elsewhere in the same scripts.
Code

scripts/pinned_browsers.py[R36-39]

+    r = http.request("GET", "https://googlechromelabs.github.io/chrome-for-testing/last-known-good-versions.json")
+    milestone = json.loads(r.data)["channels"][channel]["version"].split(".")[0]
+    r = http.request("GET", "https://googlechromelabs.github.io/chrome-for-testing/known-good-versions-with-downloads.json")
+    versions = json.loads(r.data)["versions"]
Evidence
Both new latest_for_channel() implementations parse JSON from last-known-good-versions.json and
known-good-versions-with-downloads.json without any status checks, while nearby helpers in the
same files explicitly raise on non-200 responses.

scripts/pinned_browsers.py[30-39]
scripts/update_cdp.py[17-26]
scripts/pinned_browsers.py[19-24]
scripts/update_cdp.py[33-36]

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

### Issue description
`latest_for_channel()` performs `http.request()` and immediately `json.loads(r.data)` without verifying `r.status`. When Chrome-for-Testing endpoints return non-200 bodies (404, 500, proxy HTML, etc.), the scripts fail with parsing/KeyError exceptions that lack the URL and HTTP status.

### Issue Context
Other fetch paths in these scripts were hardened to explicitly raise on non-200, so this new path should behave similarly to keep failures actionable.

### Fix Focus Areas
- scripts/pinned_browsers.py[30-39]
- scripts/update_cdp.py[17-26]

### Implementation notes
- After each `http.request(...)`, check `resp.status != 200` and raise a `ValueError` that includes HTTP status and URL (similar to `calculate_hash()` / `fetch_and_save()`).
- Consider reading/including a short snippet of the body (optional) if it helps diagnose auth/proxy issues, but don’t log full bodies if they might be large.

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


Results up to commit fab7a83


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


Remediation recommended
1. --chrome_channel restricts values ✓ Resolved 📘 Rule violation ≡ Correctness
Description
The new choices=["Stable", "Beta", "Dev", "Canary"] constraint changes the script’s CLI contract
by rejecting any other previously accepted --chrome_channel values (e.g., case variants used in
automation). This can break existing callers and violates the backward-compatibility requirement for
public interfaces.
Code

scripts/update_cdp.py[R210-215]

+    parser.add_argument(
+        "--chrome_channel",
+        default="Stable",
+        choices=["Stable", "Beta", "Dev", "Canary"],
+        help="Set the Chrome channel (use Beta for early stable)",
+    )
Evidence
PR Compliance ID 389266 requires backward-compatible public interfaces. The updated argument
definition adds strict choices validation for --chrome_channel, which narrows accepted inputs
and can break existing callers.

Rule 389266: Maintain backward-compatible public API and ABI
scripts/update_cdp.py[210-215]

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/update_cdp.py` now enforces a fixed set of `--chrome_channel` values via `choices=...`, which can break existing automation that previously passed other (but functionally equivalent) values such as different casing.

## Issue Context
This script is commonly used in CI/automation; CLI flags are a de facto public interface. The new strict validation is a behavior change that can be made backward-compatible by normalizing inputs (e.g., case-insensitive mapping) or expanding accepted aliases.

## Fix Focus Areas
- scripts/update_cdp.py[210-215]

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


Qodo Logo

Comment thread scripts/chrome_version.py Outdated
Comment thread scripts/chrome_version.py Outdated
Comment thread scripts/pinned_browsers.py
@titusfortner titusfortner changed the title [build] Resolve Chrome milestone: lock-step for pinning, latest for devtools [build] Resolve Chrome milestone from Chrome-for-Testing channel version Jul 3, 2026
Comment thread scripts/chrome_version.py Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 000e450

@titusfortner titusfortner requested a review from Copilot July 3, 2026 16:37
Comment thread scripts/pinned_browsers.py Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 0cbabf1

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 automation scripts to determine the current Chrome milestone via Chrome-for-Testing channel metadata (avoiding N-1 security respin edge cases) and improves robustness of some network fetches used by the CDP/devtools and pinned-browser update flows.

Changes:

  • Switch milestone selection to Chrome-for-Testing “last known good” channel versions, then select the newest matching version-with-downloads entry.
  • Add explicit non-200 handling for some download/fetch paths (e.g., .pdl fetches and SHA computation inputs).
  • Add a guard for missing chromedriver downloads in the selected Chrome-for-Testing entry.

Reviewed changes

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

File Description
scripts/update_cdp.py Resolves Chrome milestone/version from Chrome-for-Testing channel metadata; adds non-200 checks for protocol fetches.
scripts/pinned_browsers.py Resolves Chrome/ChromeDriver pinned versions from Chrome-for-Testing channel metadata; adds non-200/availability guards.

Comment thread scripts/update_cdp.py Outdated
Comment thread scripts/pinned_browsers.py Outdated
Comment thread scripts/pinned_browsers.py
Comment thread scripts/update_cdp.py
@titusfortner titusfortner force-pushed the resolve-chrome-milestone branch from 0cbabf1 to 0fa35a5 Compare July 3, 2026 16:53
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 0fa35a5

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 scripts/pinned_browsers.py
Comment thread scripts/update_cdp.py
Comment thread scripts/update_cdp.py Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@qodo-code-review

Copy link
Copy Markdown
Contributor

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

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