Skip to content

[rust] honor --browser-version in Selenium Manager Electron driver resolution #17567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gaurav0107 wants to merge 2 commits into
base: trunk
Choose a base branch
from
+50 −11
Open

[rust] honor --browser-version in Selenium Manager Electron driver resolution #17567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6554d3
[rust] honor --browser-version in Selenium Manager Electron driver re…
gaurav0107 May 24, 2026
ff16a22
[rust] strengthen electron --browser-version regression test
gaurav0107 May 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions rust/src/electron.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,17 +119,30 @@ impl SeleniumManager for ElectronManager {
Ok(driver_version)
}
_ => {
self.assert_online_or_err(OFFLINE_REQUEST_ERR_MSG)?;

let latest_url = format!(
"{}{}",
self.get_driver_mirror_url_or_default(DRIVER_URL),
LATEST_RELEASE
);
let driver_version =
read_redirect_from_link(self.get_http_client(), latest_url, self.get_logger())?;
// Electron releases are tagged by Electron version, and the
// chromedriver asset shipped in each release matches that tag.
// When the user pins a concrete browser version (e.g.
// `--browser-version 36.2.1`), that version is the driver
// version we want; resolving via `/releases/latest` would
// discard the user's request and return the latest tag instead.
let browser_version = self.get_browser_version().to_string();
let driver_version = if !browser_version.is_empty()
&& !self.is_browser_version_stable()
&& !self.is_browser_version_unstable()
{
browser_version
} else {
Comment on lines +128 to +134
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Electron treats major as tag 📘 Rule violation ≡ Correctness

ElectronManager::request_driver_version() now treats any non-empty --browser-version that is not
stable/unstable as an exact Electron release tag, which incorrectly includes major-only inputs
like 36 that are documented as valid. This can break existing CLI usage by producing invalid
Electron download URLs (e.g., .../download/v36/...) and causing downloads to fail.
Agent Prompt 
## Issue description
`ElectronManager::request_driver_version()` currently returns `--browser-version` verbatim as the Electron `driver_version` for any non-empty input that is not the `stable`/`unstable` channel. This mistakenly treats major-only values (e.g., `36`)—which the CLI documents as valid major-version inputs—as exact Electron release tags, leading to invalid tag/URL construction (e.g., `.../download/v36/...`) and failed downloads.

## Issue Context
The codebase already distinguishes a “specific” version from a major version using the presence of a dot (`.`) (i.e., “specific” means it contains a dot). The CLI help for `--browser-version` explicitly describes it as a major version input and also supports channel names. Electron’s direct-return/short-circuit behavior should therefore only apply when the provided browser version is actually specific (e.g., `36.2.1`), while major-only values should continue to use the existing latest-redirect resolution (e.g., `/releases/latest`) or alternatively produce a deterministic validation error; stable/unstable handling should remain unchanged.

## Fix Focus Areas
- rust/src/electron.rs[103-155]
- rust/src/lib.rs[813-819]
- rust/src/main.rs[65-68]

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

Comment on lines 119 to +134
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. Cache overrides pinned patch 🐞 Bug ≡ Correctness

request_driver_version still checks metadata by major_browser_version before applying the pinned
--browser-version logic, so a cached entry for major "36" can override a user request for "36.2.1"
and return a different patch driver version.
This can silently violate the user’s explicit version pin.
Agent Prompt 
### Issue description
Even after this PR, `ElectronManager::request_driver_version()` may return a cached driver version based on `major_browser_version` before it considers the user’s pinned full `--browser-version`. Because the metadata key is only the major component, different patch pins under the same major can conflict.

### Issue Context
- Metadata lookup for driver version keys on `major_browser_version`.
- `get_major_browser_version()` returns only the major component for non-channel inputs.
- The new pinned-version behavior lives only in the metadata-miss branch.

### Fix Focus Areas
- rust/src/electron.rs[103-155]
- rust/src/metadata.rs[123-139]
- rust/src/lib.rs[1365-1374]

### What to change
One of the following (prefer the one consistent with project behavior):
- If `--browser-version` is a specific pinned version, bypass metadata lookup entirely and return the requested (normalized) version.
- Or, key metadata lookups/writes for Electron on the full pinned version (not just the major) when `is_browser_version_specific()` is true, so separate patch pins don’t collide.

Also consider adding a regression test that runs two different `--browser-version 36.x.y` values back-to-back and asserts the second run resolves to the second requested patch.

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

self.assert_online_or_err(OFFLINE_REQUEST_ERR_MSG)?;
let latest_url = format!(
"{}{}",
self.get_driver_mirror_url_or_default(DRIVER_URL),
LATEST_RELEASE
);
read_redirect_from_link(self.get_http_client(), latest_url, self.get_logger())?
};
Comment on lines +128 to +142
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Unnormalized pinned version 🐞 Bug ≡ Correctness

The pinned --browser-version is used verbatim as driver_version, but get_driver_url always prefixes
the driver version with "v", so inputs like "v36.2.1" will produce "vv36.2.1" and break downloads.
This is a regression from the redirect-based path that normalizes versions via parse_version().
Agent Prompt 
### Issue description
When `--browser-version` is treated as the driver version, it is not normalized. A common user input for GitHub tags is a leading `v` (e.g., `v36.2.1`), but `get_driver_url()` already adds its own `v` prefix, yielding `vv...` and a 404.

### Issue Context
The existing online-redirect resolution path calls `read_redirect_from_link()`, which normalizes the redirect URL via `parse_version()` (stripping non-numeric prefixes such as `v`). The new bypass path skips this normalization.

### Fix Focus Areas
- rust/src/electron.rs[103-176]
- rust/src/downloads.rs[89-99]
- rust/src/files.rs[530-548]

### What to change
- Before returning `browser_version` as `driver_version`, normalize it:
  - trim whitespace
  - if it starts with `v`/`V`, strip that leading character
  - (optional) validate it’s a specific version format used by Electron assets
- Ensure the normalized value is what gets used for URL construction and cache paths.

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

let driver_ttl = self.get_ttl();
if driver_ttl > 0 {
if driver_ttl > 0 && !major_browser_version.is_empty() && !driver_version.is_empty()
{
metadata.drivers.push(create_driver_metadata(
major_browser_version,
self.driver_name,
Expand Down
28 changes: 27 additions & 1 deletion rust/tests/electron_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// specific language governing permissions and limitations
// under the License.

use crate::common::get_selenium_manager;
use crate::common::{get_driver_path, get_selenium_manager};

use rstest::rstest;

Expand All @@ -37,3 +37,29 @@ fn electron_version_test(#[case] driver_version: String) {
.assert();
cmd_assert.success();
}

#[rstest]
#[case("36.2.1")]
fn electron_browser_version_test(#[case] browser_version: String) {
// Regression for issue #17549: when the user pins --browser-version, the
// resolved driver version must be derived from the requested version, not
// from the /releases/latest redirect. Asserting that the resolved driver
// path contains the requested version protects against silently falling
// back to the latest tag.
let mut cmd = get_selenium_manager();
cmd.args([
"--browser",
"electron",
"--browser-version",
&browser_version,
"--output",
"json",
]);
let driver_path = get_driver_path(&mut cmd);
assert!(
driver_path.contains(&browser_version),
"expected resolved driver path to contain requested version {}, got: {}",
browser_version,
driver_path
);
}
Loading