Skip to content

fix(rust): prevent empty driver version from being cached in metadata #17757

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
MathiasPaulenko wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

fix(rust): prevent empty driver version from being cached in metadata #17757

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c633ec8
fix(rust): prevent empty driver version from being cached in metadata
MathiasPaulenko Jul 9, 2026
015b4b3
fix: address Copilot review feedback
MathiasPaulenko Jul 9, 2026
c7e86d0
fix(rust): address qodo review — add regression test for empty driver…
MathiasPaulenko Jul 9, 2026
e6529d0
extract should_cache_driver_version helper for testable regression guard
MathiasPaulenko Jul 9, 2026
15ef2a7
add doc comment to should_cache_driver_version
MathiasPaulenko Jul 9, 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
5 changes: 3 additions & 2 deletions rust/src/chrome.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ use crate::downloads::{parse_json_from_url, read_version_from_link};
use crate::files::{BrowserPath, compose_driver_path_in_cache};
use crate::logger::Logger;
use crate::metadata::{
create_driver_metadata, get_driver_version_from_metadata, get_metadata, write_metadata,
create_driver_metadata, get_driver_version_from_metadata, get_metadata,
should_cache_driver_version, write_metadata,
};
use crate::{
BETA, DASH_DASH_VERSION, DEV, NIGHTLY, OFFLINE_REQUEST_ERR_MSG, REG_VERSION_ARG, STABLE,
Expand Down Expand Up @@ -339,7 +340,7 @@ impl SeleniumManager for ChromeManager {
};

let driver_ttl = self.get_ttl();
if driver_ttl > 0 && !major_browser_version.is_empty() && !driver_version.is_empty()
if should_cache_driver_version(driver_ttl, major_browser_version, &driver_version)
{
metadata.drivers.push(create_driver_metadata(
major_browser_version,
Expand Down
29 changes: 27 additions & 2 deletions rust/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,8 @@ pub enum ARCH {
impl ARCH {
pub fn to_str_vector(&self) -> Vec<&str> {
match self {
ARCH::X32 => vec![ARCH_X86, "i386", "x32"],
ARCH::X64 => vec![ARCH_X64, "amd64", "x64", "i686", "ia64"],
ARCH::X32 => vec![ARCH_X86, "i386", "x32", "i686"],
ARCH::X64 => vec![ARCH_X64, "amd64", "x64", "ia64"],
ARCH::ARM64 => vec![ARCH_ARM64, "aarch64", "arm"],
ARCH::ARMV7 => vec![ARCH_ARM7L, "armv7l"],
}
Expand Down Expand Up @@ -269,6 +269,31 @@ fn get_env_name(suffix: &str) -> String {
concat(ENV_PREFIX, suffix_uppercase.as_str())
}

#[cfg(test)]
mod env_name_tests {
use super::*;

#[test]
fn get_env_name_simple_key() {
assert_eq!(get_env_name("browser"), "SE_BROWSER");
}

#[test]
fn get_env_name_dashes_become_underscores() {
assert_eq!(get_env_name("browser-version"), "SE_BROWSER_VERSION");
}

#[test]
fn get_env_name_mixed_case_uppercased() {
assert_eq!(get_env_name("Cache-Path"), "SE_CACHE_PATH");
}

#[test]
fn get_env_name_empty_suffix() {
assert_eq!(get_env_name(""), "SE_");
}
}

fn get_config() -> Result<Table, Error> {
let cache_path = read_cache_path();
let config_path = Path::new(&cache_path).to_path_buf().join(CONFIG_FILE);
Expand Down
6 changes: 4 additions & 2 deletions rust/src/edge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ use crate::config::OS::{LINUX, MACOS, WINDOWS};
use crate::downloads::{parse_json_from_url, read_version_from_link};
use crate::files::{BrowserPath, compose_driver_path_in_cache};
use crate::metadata::{
create_driver_metadata, get_driver_version_from_metadata, get_metadata, write_metadata,
create_driver_metadata, get_driver_version_from_metadata, get_metadata,
should_cache_driver_version, write_metadata,
};
use crate::{
BETA, DASH_DASH_VERSION, DEV, ENV_PROGRAM_FILES, ENV_PROGRAM_FILES_X86, Logger, NIGHTLY,
Expand Down Expand Up @@ -252,7 +253,8 @@ impl SeleniumManager for EdgeManager {
read_version_from_link(self.get_http_client(), &driver_url, self.get_logger())?;

let driver_ttl = self.get_ttl();
if driver_ttl > 0 && !major_browser_version.is_empty() {
if should_cache_driver_version(driver_ttl, major_browser_version, &driver_version)
{
Comment on lines +256 to +257

@qodo-code-review qodo-code-review Bot Jul 9, 2026

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.

Action required

2. String passed to helper 🐞 Bug ≡ Correctness

EdgeManager::request_driver_version passes major_browser_version (a String) into
should_cache_driver_version, which requires &str, causing a compile-time type mismatch and
breaking the build.
Agent Prompt 
### Issue description
`should_cache_driver_version(driver_ttl, major_browser_version, &driver_version)` passes a `String` where the helper requires `&str`, which is a compile error in Rust.

### Issue Context
In `edge.rs`, `major_browser_version` is a `String` returned from `get_major_browser_version()`, and the helper signature is `(&str, &str)` for the version parameters.

### Fix Focus Areas
- rust/src/edge.rs[252-264]

### Suggested change
Update the call to explicitly pass a string slice, e.g.:
- `should_cache_driver_version(driver_ttl, major_browser_version.as_str(), &driver_version)`
  (or `should_cache_driver_version(driver_ttl, &major_browser_version, &driver_version)` relying on deref coercion)

Keep the rest of the logic unchanged.

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

metadata.drivers.push(create_driver_metadata(
major_browser_version.as_str(),
self.driver_name,
Expand Down
6 changes: 4 additions & 2 deletions rust/src/firefox.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ use crate::config::OS::{LINUX, MACOS, WINDOWS};
use crate::downloads::{parse_json_from_url, read_content_from_link, read_redirect_from_link};
use crate::files::{BrowserPath, compose_driver_path_in_cache};
use crate::metadata::{
create_driver_metadata, get_driver_version_from_metadata, get_metadata, write_metadata,
create_driver_metadata, get_driver_version_from_metadata, get_metadata,
should_cache_driver_version, write_metadata,
};
use crate::{
BETA, DASH_VERSION, DEV, ESR, LATEST_RELEASE, Logger, NIGHTLY, OFFLINE_REQUEST_ERR_MSG, STABLE,
Expand Down Expand Up @@ -296,7 +297,8 @@ impl SeleniumManager for FirefoxManager {
};

let driver_ttl = self.get_ttl();
if driver_ttl > 0 && !major_browser_version.is_empty() {
if should_cache_driver_version(driver_ttl, major_browser_version, &driver_version)
{
metadata.drivers.push(create_driver_metadata(
major_browser_version,
self.driver_name,
Expand Down
37 changes: 37 additions & 0 deletions rust/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1972,3 +1972,40 @@ fn get_index_version(full_version: &str, index: usize) -> Result<String, Error>
.ok_or(anyhow!(format!("Wrong version: {}", full_version)))?
.to_string())
}

#[cfg(test)]
mod index_version_tests {
use super::*;

#[test]
fn get_index_version_major() {
assert_eq!(get_index_version("120.0.6099.109", 0).unwrap(), "120");
}

#[test]
fn get_index_version_minor() {
assert_eq!(get_index_version("120.0.6099.109", 1).unwrap(), "0");
}

#[test]
fn get_index_version_patch() {
assert_eq!(get_index_version("120.0.6099.109", 2).unwrap(), "6099");
}

#[test]
fn get_index_version_single_component() {
assert_eq!(get_index_version("115", 0).unwrap(), "115");
}

#[test]
fn get_index_version_out_of_bounds_errors() {
let result = get_index_version("115", 1);
assert!(result.is_err());
}

#[test]
fn get_index_version_empty_string_errors() {
let result = get_index_version("", 0);
assert!(result.is_err());
}
}
8 changes: 8 additions & 0 deletions rust/src/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,14 @@ pub fn create_browser_metadata(
}
}

pub fn should_cache_driver_version(
driver_ttl: u64,
major_browser_version: &str,
driver_version: &str,
) -> bool {
driver_ttl > 0 && !major_browser_version.is_empty() && !driver_version.is_empty()
}
Comment on lines +188 to +194

@qodo-code-review qodo-code-review Bot Jul 9, 2026

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.

Action required

1. should_cache_driver_version lacks docs 📘 Rule violation ✧ Quality

The new public function should_cache_driver_version has no Rust doc comment (///) even though it
is part of the crate’s public API surface. This violates the requirement to document public API
items with structured doc comments, including # Arguments and # Returns where applicable.
Agent Prompt 
## Issue description
`pub fn should_cache_driver_version(...) -> bool` is a public API item but has no `///` documentation. Per the checklist, public functions should have a brief summary plus structured sections when applicable.

## Issue Context
This function takes multiple parameters and returns a non-`()` value, so the doc comment should include `# Arguments` and `# Returns`.

## Fix Focus Areas
- rust/src/metadata.rs[184-190]

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


pub fn create_driver_metadata(
major_browser_version: &str,
driver_name: &str,
Expand Down
Loading