[titusfortner]

🔗 Related Issues

• Regression from [rust] Validate TLS certificates in Selenium Manager HTTP client #18220 (Selenium Manager now validates TLS certificates)
• Builds on [rust] Add opt-in insecure mode to Selenium Manager #18222 (opt-in insecure mode)

💥 What does this PR do?

• Error for a missing CA store when making a network call instead of at startup
• Provide additional information about the reason for the certificate error
• Allow --insecure to work on systems without a CA store

🔧 Implementation Notes

• On Linux, rustls-platform-verifier loads the system CA roots when the reqwest client is built and errors if there are none, and every manager built its client in its constructor; the no-verify client used before validation was enabled never loaded roots.
• The client is now created on the first get_http_client() call and set_proxy/set_timeout/set_insecure reset it; set_proxy still parses the proxy URL up front so invalid proxies fail as early as before.
• Stats are skipped (debug log) if the client can't be built, so a missing CA store no longer aborts the run before setup and its driver-in-PATH fallback.
• Failures are now reported with their full cause chain, and the hint is added when that chain contains a rustls InvalidCertificate error; a missing CA store already reports "No CA certificates were loaded from the system".
• A Linux-only test runs an offline lookup with SSL_CERT_FILE=/dev/null to keep the missing-CA-store case working without network access.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude Code (Claude Opus 5.5)
  • What was generated: diagnosis, fix, tests, and this description
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• Bug fix (backwards compatible)

[selenium-ci]

/agentic_describe

[selenium-ci]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Defer Selenium Manager HTTP client creation and clarify TLS errors

🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Create HTTP clients only when needed so missing CA roots do not block local driver fallback.
• Skip stats when client creation fails, while preserving early proxy URL validation.
• Show full error causes and trust-store guidance for TLS certificate failures.

Diagram

graph TD
  M["Manager constructor"] --> C["Client cell"] --> S["Stats attempt"] --> D{"Client builds?"} -->|"yes"| U["Setup and downloads"]
  D -->|"no: skip stats"| U
  P["Proxy or timeout"] -->|"reset"| C
  U -->|"first network use"| C
  U -->|"TLS failure"| F["Error formatter"]

Loading

High-Level Assessment

Lazy initialization in the shared trait preserves client reuse and TLS validation while letting setup reach local driver fallbacks. Eager construction would retain the startup failure; disabling certificate validation would weaken security.

Files changed (10) +172 / -102

Bug fix (10) +172 / -102

chrome.rsDefer Chrome HTTP client creation +9/-11

Defer Chrome HTTP client creation

• ChromeManager now starts with an empty client cell instead of building a client in its constructor. Version-discovery requests propagate client-creation errors when network access is required.

rust/src/chrome.rs

edge.rsDefer Edge HTTP client creation +14/-12

Defer Edge HTTP client creation

• EdgeManager stores a lazy client cell. Its release and version lookup paths now propagate errors from acquiring the client.

rust/src/edge.rs

electron.rsDefer Electron HTTP client creation +12/-10

Defer Electron HTTP client creation

• ElectronManager no longer builds a client during construction. Its redirect-based version lookup acquires the client when needed and propagates failures.

rust/src/electron.rs

firefox.rsDefer Firefox HTTP client creation +13/-14

Defer Firefox HTTP client creation

• FirefoxManager uses a lazy client cell. Firefox and geckodriver discovery requests now propagate client-creation failures at their network call sites.

rust/src/firefox.rs

grid.rsDefer Grid HTTP client creation +8/-11

Defer Grid HTTP client creation

• GridManager starts without an HTTP client and acquires one when fetching Selenium releases.

rust/src/grid.rs

iexplorer.rsDefer Internet Explorer HTTP client creation +8/-9

Defer Internet Explorer HTTP client creation

• IExplorerManager uses a lazy client cell and propagates client-creation errors when fetching Selenium releases.

rust/src/iexplorer.rs

lib.rsCentralize lazy client access and TLS diagnostics +90/-15

Centralize lazy client access and TLS diagnostics

• The shared manager trait builds clients on first access and resets them after proxy or timeout changes; proxy URLs are still validated immediately. Stats skips client-build failures, while error formatting shows cause chains and adds trust-store guidance for recognized rustls errors. Unit tests cover lazy creation and error formatting.

rust/src/lib.rs

main.rsDisplay detailed errors in CLI fallback paths +4/-4

Display detailed errors in CLI fallback paths

• CLI failure and cached-driver fallback messages use the shared formatter to expose underlying causes and applicable TLS guidance.

rust/src/main.rs

safari.rsDefer Safari HTTP client creation +7/-8

Defer Safari HTTP client creation

• SafariManager replaces constructor-time client creation with an empty client cell, following the shared manager behavior.

rust/src/safari.rs

safaritp.rsDefer Safari Technology Preview HTTP client creation +7/-8

Defer Safari Technology Preview HTTP client creation

• SafariTPManager replaces constructor-time client creation with an empty client cell, following the shared manager behavior.

rust/src/safaritp.rs

[qodo-code-review]

Code Review by Qodo

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

1. Downstream Rust code stops compiling ✗ Dismissed

Description

SeleniumManager removes the public get_http_client and set_http_client methods and replaces
them with OnceLock accessors without deprecated compatibility methods. Downstream crates
implementing the public trait or relying on the public manager API must change their source before
upgrading.

Code

rust/src/lib.rs[R124-126]

+    fn get_http_client_cell(&self) -> &OnceLock<Client>;

-    fn set_http_client(&mut self, http_client: Client);
+    fn get_http_client_cell_mut(&mut self) -> &mut OnceLock<Client>;

Evidence

The checklist requires existing public API behavior to remain compatible and functionality to be
deprecated before removal. The public SeleniumManager trait replaces its existing HTTP-client
methods, while the public manager structs also change http_client from Client to
OnceLock<Client>, so downstream implementations and callers must adapt immediately.

AGENTS.md: Preserve Public API and ABI Compatibility and Deprecate Before Removal
rust/src/lib.rs[124-126]
rust/src/chrome.rs[77-77]

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 public `SeleniumManager` trait removes existing methods and introduces different required accessors without a deprecation period or compatibility alternative, breaking downstream Rust implementations and users of the public API.

## Fix Focus Areas
- rust/src/lib.rs[124-126]
- rust/src/chrome.rs[77-77]

## Recommended Fix
Preserve the existing public methods as deprecated compatibility shims that delegate to the new lazy-client implementation, or provide an equivalent public alternative before removal. Avoid changing the public manager field type unless the old field is retained or a documented migration path is supplied.

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

---

2. Debug runs omit the certificate fix hint ✓ Resolved

Description

The error handler in main calls format_error only in the non-debug branch, while the debug
branch formats the error directly. When a TLS certificate failure occurs with --debug enabled, the
output includes the cause chain but omits the new guidance about the system trust store and
SSL_CERT_FILE.

Code

rust/src/main.rs[345]

+                    format_error(&err)

Evidence

The debug branch at the final error handler renders err with {:?} and never invokes the
formatter added by this PR; only the alternative branch uses it. The formatter is the code that
appends the certificate guidance.

rust/src/main.rs[342-347]
rust/src/lib.rs[1874-1879]
rust/src/lib.rs[110-110]

Agent prompt

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

## Issue description
Debug-mode TLS failures bypass `format_error`, so users troubleshooting with `--debug` do not receive the new certificate guidance.
## Fix Focus Areas
- rust/src/main.rs[342-347]
- rust/src/lib.rs[1874-1879]
## Recommended Fix
Preserve the debug error details while also appending the certificate guidance when the error qualifies; add a test for debug-mode TLS output.

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

---

3. Laziness test fails on hosts with no CA store ✓ Resolved

Description

http_client_is_not_created_until_needed calls manager.get_http_client().unwrap() with the
default (secure) config, which builds the client through create_http_client and so loads the
platform verifier. On a Linux build or test host that has no system CA store, which is exactly the
setup this PR is meant to support, the build returns an error and the unit test panics even though
lazy creation is working.

Code

rust/src/lib.rs[R2029-2032]

+        let manager = crate::chrome::ChromeManager::new().unwrap();
+        assert!(manager.get_http_client_cell().get().is_none());
+
+        manager.get_http_client().unwrap();

Evidence

The test builds a real client with insecure=false. The PR description says rustls-platform-verifier
errors at build time when no system roots exist, and create_http_client only skips that check when
insecure is set.

rust/src/lib.rs[1881-1893]
rust/src/lib.rs[2027-2035]

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 test `http_client_is_not_created_until_needed` builds a verifying reqwest client. On Linux hosts without a system CA store, the build fails and `unwrap()` panics.

## Fix Focus Areas
- rust/src/lib.rs[2027-2035]

## Recommended Fix
Before calling `get_http_client()`, make the manager mutable and call `manager.set_insecure(true).unwrap()` (or set `get_config_mut().insecure = true`). The laziness assertion then no longer depends on the host trust store.

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

---

4. Unrelated TLS failures get a certificate hint ✓ Resolved

Description

is_tls_certificate_error treats any rustls::Error::General(_) in the cause chain as a
certificate error, so format_error adds TLS_CERTIFICATE_ERR_MSG ("TLS certificate validation
failed... add it to the system trust store"). Rustls and the platform verifier also use General
for failures that aren't about trust, such as crypto-provider setup or other OS errors. In those
cases users are told to fix their CA store when the cause is something else.

Code

rust/src/lib.rs[R2087-2090]

+        matches!(
+            current.downcast_ref::<rustls::Error>(),
+            Some(rustls::Error::InvalidCertificate(_) | rustls::Error::General(_))
+        )

Evidence

The matcher accepts every General variant, whatever its message. Only the 'missing CA store' case
named in the PR description is actually a certificate-trust problem.

rust/src/lib.rs[2077-2092]
rust/src/lib.rs[1874-1880]

Agent prompt

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

## Issue description
`is_tls_certificate_error` matches every `rustls::Error::General(_)`, so the CA-store hint is added to TLS errors that have nothing to do with trust.

## Fix Focus Areas
- rust/src/lib.rs[2077-2092]

## Recommended Fix
Keep matching `InvalidCertificate(_)`. For `General(msg)`, add the hint only when the message points to a missing or empty CA store (e.g. it contains "CA certificates"). Add a test showing that an unrelated `General` error gets no hint.

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

---

Context sources

✅ Web pages:

  🌐 Releases · rustls/rustls-platform-verifier · GitHub

  🌐 others.rs - source

  🌐 rustls-platform-verifier 0.7.1 - Docs.rs

  +16 more

Review mode: Auto: 🚀 Fast: Localized TLS error classification and focused regression test; limited behavioral risk.

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 0115af9 🚀 Fast

Results up to commit 2e6f9e4 🧠 Deep

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

1. Debug runs omit the certificate fix hint ✓ Resolved

Description

The error handler in main calls format_error only in the non-debug branch, while the debug
branch formats the error directly. When a TLS certificate failure occurs with --debug enabled, the
output includes the cause chain but omits the new guidance about the system trust store and
SSL_CERT_FILE.

Code

rust/src/main.rs[345]

+                    format_error(&err)

Evidence

The debug branch at the final error handler renders err with {:?} and never invokes the
formatter added by this PR; only the alternative branch uses it. The formatter is the code that
appends the certificate guidance.

rust/src/main.rs[342-347]
rust/src/lib.rs[1874-1879]
rust/src/lib.rs[110-110]

Agent prompt

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

## Issue description
Debug-mode TLS failures bypass `format_error`, so users troubleshooting with `--debug` do not receive the new certificate guidance.
## Fix Focus Areas
- rust/src/main.rs[342-347]
- rust/src/lib.rs[1874-1879]
## Recommended Fix
Preserve the debug error details while also appending the certificate guidance when the error qualifies; add a test for debug-mode TLS output.

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

---

2. Unrelated TLS failures get a certificate hint ✓ Resolved

Description

is_tls_certificate_error treats any rustls::Error::General(_) in the cause chain as a
certificate error, so format_error adds TLS_CERTIFICATE_ERR_MSG ("TLS certificate validation
failed... add it to the system trust store"). Rustls and the platform verifier also use General
for failures that aren't about trust, such as crypto-provider setup or other OS errors. In those
cases users are told to fix their CA store when the cause is something else.

Code

rust/src/lib.rs[R2087-2090]

+        matches!(
+            current.downcast_ref::<rustls::Error>(),
+            Some(rustls::Error::InvalidCertificate(_) | rustls::Error::General(_))
+        )

Evidence

The matcher accepts every General variant, whatever its message. Only the 'missing CA store' case
named in the PR description is actually a certificate-trust problem.

rust/src/lib.rs[2077-2092]
rust/src/lib.rs[1874-1880]

Agent prompt

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

## Issue description
`is_tls_certificate_error` matches every `rustls::Error::General(_)`, so the CA-store hint is added to TLS errors that have nothing to do with trust.

## Fix Focus Areas
- rust/src/lib.rs[2077-2092]

## Recommended Fix
Keep matching `InvalidCertificate(_)`. For `General(msg)`, add the hint only when the message points to a missing or empty CA store (e.g. it contains "CA certificates"). Add a test showing that an unrelated `General` error gets no hint.

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

---

Results up to commit e5e53e4 🧠 Deep

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

1. Downstream Rust code stops compiling ✗ Dismissed

Description

SeleniumManager removes the public get_http_client and set_http_client methods and replaces
them with OnceLock accessors without deprecated compatibility methods. Downstream crates
implementing the public trait or relying on the public manager API must change their source before
upgrading.

Code

rust/src/lib.rs[R124-126]

+    fn get_http_client_cell(&self) -> &OnceLock<Client>;

-    fn set_http_client(&mut self, http_client: Client);
+    fn get_http_client_cell_mut(&mut self) -> &mut OnceLock<Client>;

Evidence

The checklist requires existing public API behavior to remain compatible and functionality to be
deprecated before removal. The public SeleniumManager trait replaces its existing HTTP-client
methods, while the public manager structs also change http_client from Client to
OnceLock<Client>, so downstream implementations and callers must adapt immediately.

AGENTS.md: Preserve Public API and ABI Compatibility and Deprecate Before Removal
rust/src/lib.rs[124-126]
rust/src/chrome.rs[77-77]

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 public `SeleniumManager` trait removes existing methods and introduces different required accessors without a deprecation period or compatibility alternative, breaking downstream Rust implementations and users of the public API.

## Fix Focus Areas
- rust/src/lib.rs[124-126]
- rust/src/chrome.rs[77-77]

## Recommended Fix
Preserve the existing public methods as deprecated compatibility shims that delegate to the new lazy-client implementation, or provide an equivalent public alternative before removal. Avoid changing the public manager field type unless the old field is retained or a documented migration path is supplied.

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

---

2. Laziness test fails on hosts with no CA store ✓ Resolved

Description

http_client_is_not_created_until_needed calls manager.get_http_client().unwrap() with the
default (secure) config, which builds the client through create_http_client and so loads the
platform verifier. On a Linux build or test host that has no system CA store, which is exactly the
setup this PR is meant to support, the build returns an error and the unit test panics even though
lazy creation is working.

Code

rust/src/lib.rs[R2029-2032]

+        let manager = crate::chrome::ChromeManager::new().unwrap();
+        assert!(manager.get_http_client_cell().get().is_none());
+
+        manager.get_http_client().unwrap();

Evidence

The test builds a real client with insecure=false. The PR description says rustls-platform-verifier
errors at build time when no system roots exist, and create_http_client only skips that check when
insecure is set.

rust/src/lib.rs[1881-1893]
rust/src/lib.rs[2027-2035]

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 test `http_client_is_not_created_until_needed` builds a verifying reqwest client. On Linux hosts without a system CA store, the build fails and `unwrap()` panics.

## Fix Focus Areas
- rust/src/lib.rs[2027-2035]

## Recommended Fix
Before calling `get_http_client()`, make the manager mutable and call `manager.set_insecure(true).unwrap()` (or set `get_config_mut().insecure = true`). The laziness assertion then no longer depends on the host trust store.

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

---

Results up to commit 2efa093 🚀 Fast

No changes from previous review

[selenium-ci]

/agentic_review

[qodo-code-review]

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

[selenium-ci]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 2efa093

[selenium-ci]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0115af9

[bonigarcia]

LGTM