[rust] Change command execution to argv by bonigarcia · Pull Request #17576 · SeleniumHQ/selenium

bonigarcia · 2026-05-27T05:16:12Z

🔗 Related Issues

💥 What does this PR do?

This PR reworks shell::Command to carry a program plus argv, and executes commands directly with std::process::Command. This change makes not necessary to execute bash -c in Linux, which prevents #17521.

🔧 Implementation Notes

Updates browser/version probes, DMG/MSI install helpers, and test helpers to use direct arguments instead of sh -c style strings.
Simplifies get_escaped_path so it only canonicalizes paths and no longer shells out to bash.

🤖 AI assistance

[] No substantial AI assistance used
AI assisted (complete below)
- Tool(s): OpenCode
- What was generated: Shell logic
- I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

New feature (non-breaking change which adds functionality and tests!)

qodo-code-review · 2026-05-27T05:16:31Z

Review Summary by Qodo

Refactor command execution to use argv instead of shell strings

✨ Enhancement

Walkthroughs

Description

• Refactor command execution to use argv instead of shell strings
• Eliminate unnecessary bash/sh wrapper invocations for direct command execution
• Simplify path escaping by removing shell-based escaping logic
• Update all command builders to use program + arguments structure

Diagram

flowchart LR
  A["Shell Command Strings<br/>bash -c style"] -->|Refactor| B["Direct Execution<br/>program + argv"]
  B -->|Eliminates| C["Unnecessary Shell Wrappers"]
  B -->|Simplifies| D["Path Escaping Logic"]
  E["Command struct"] -->|Changed from| F["single_arg/multiple_args"]
  E -->|Changed to| G["program/args"]

File Changes

1. rust/src/shell.rs ✨ Enhancement +16/-58

Refactor Command struct for direct argv execution

• Simplified Command struct to use program and args fields instead of
 single_arg/multiple_args
• Removed run_shell_command_by_os() function that wrapped commands with shell interpreters
• Updated run_shell_command() to directly execute commands using std::process::Command
• Removed OS-specific shell selection logic (cmd/sh)

rust/src/shell.rs

2. rust/src/config.rs ✨ Enhancement +8/-7

Update uname commands to use argv structure

• Updated uname command calls to use Command::new() with argv instead of formatted strings
• Changed from format_one_arg(UNAME_COMMAND, "a") to `Command::new(UNAME_COMMAND,
 vec![String::from("-a")])`
• Moved UNAME_COMMAND constant definition from lib.rs to config.rs
• Updated function calls from run_shell_command_by_os() to run_shell_command()

rust/src/config.rs

3. rust/src/files.rs ✨ Enhancement +33/-20

Refactor DMG and MSI installation commands

• Refactored uncompress_dmg() to use direct argv commands for hdiutil and cp operations
• Updated install_msi() to construct msiexec command with explicit arguments
• Removed OS parameter from uncompress_dmg() function signature
• Changed all command construction from formatted strings to Command::new() with argument vectors

rust/src/files.rs

View more (3)

4. rust/src/firefox.rs ✨ Enhancement +3/-4

Remove unused registry argument constant

• Removed unused REG_CURRENT_VERSION_ARG import
• Updated registry query to use hardcoded "CurrentVersion" string instead of constant

rust/src/firefox.rs

5. rust/src/lib.rs ✨ Enhancement +46/-66

Refactor command execution and remove shell templates

• Removed run_shell_command_by_os() from imports and usage
• Removed shell command template constants (PLIST_COMMAND, HDIUTIL_ATTACH_COMMAND, etc.)
• Refactored find_driver_in_path() to use Command::new() with argv
• Refactored is_windows_admin() to use direct command execution
• Simplified get_escaped_path() to only canonicalize paths without shell escaping
• Updated detect_browser_version() to construct REG and version commands with argv
• Updated detect_browser_version_macos() to use PlistBuddy with explicit arguments
• Added test for get_escaped_path() to verify canonical path behavior

rust/src/lib.rs

6. rust/tests/common.rs 🧪 Tests +3/-4

Update test helpers for argv-based commands

• Updated exec_driver() test helper to use Command::new() with argv
• Changed from run_shell_command_by_os() to run_shell_command()
• Removed unused OS import

rust/tests/common.rs

qodo-code-review · 2026-05-27T05:16:32Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (5) 📎 Requirement gaps (0)

1. check_error_with_driver_in_path now errors ✓ Resolved 📘 Rule violation ≡ Correctness

Description

check_error_with_driver_in_path now suppresses errors only when *is_driver_in_path is true and
self.get_browser_path().is_empty(), whereas it previously suppressed errors whenever
*is_driver_in_path was true. Because browser_path can be populated during auto-detection, this
changes user-visible behavior for driver-in-PATH fallback and can cause previously-successful setups
to abort, breaking upgrades for consumers relying on the prior non-failing behavior.

Code

rust/src/lib.rs[R955-958]

Evidence

PR Compliance ID 2 requires avoiding breaking user-facing behavior changes for upgrades, and the
updated condition at rust/src/lib.rs:955 changes when errors are suppressed versus propagated in
driver-in-PATH flows. In setup(), use_driver_in_path is computed from the presence of a driver
in PATH and an originally-empty browser version, and subsequent discovery/download/version errors
are routed through check_error_with_driver_in_path(); however, browser version discovery calls
detect_browser_path() when browser_path is empty, and detect_browser_path() mutates config via
set_browser_path(), so get_browser_path() commonly becomes non-empty during normal
auto-discovery. As a result, the new && self.get_browser_path().is_empty() guard often evaluates
false in common successful-discovery flows, preventing error suppression and defeating the intended
PATH-driver fallback, causing failures that previously would have been swallowed.

AGENTS.md: Maintain API/ABI compatibility for user-facing upgrades
rust/src/lib.rs[950-963]
rust/src/lib.rs[804-854]
rust/src/lib.rs[950-964]
rust/src/lib.rs[1169-1211]
rust/src/lib.rs[406-456]
rust/src/lib.rs[1370-1377]

Agent prompt

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

## Issue description
`check_error_with_driver_in_path()` has changed fallback semantics: it now propagates errors when `*is_driver_in_path` is true but `get_browser_path()` is non-empty, whereas previously driver-in-PATH mode would swallow these errors and continue. Because `browser_path` is frequently populated by auto-detection (not just explicit user input like `--browser-path`), this additional predicate can cause setup flows that intend to fall back to a PATH driver to abort during later failures (e.g., driver version discovery), creating a breaking behavior change for upgrades.

## Issue Context
- `setup()` computes `use_driver_in_path` based on detecting a driver in PATH and the original browser version being empty, then routes errors from discovery/download/version steps through `check_error_with_driver_in_path()`.
- Browser discovery/version logic calls `detect_browser_path()` when `browser_path` is empty; `detect_browser_path()` updates configuration via `set_browser_path()`, so `get_browser_path()` reflects derived/auto-detected state, not solely user-provided state.
- The PR’s main goal is switching command execution to argv, but this extra gating changes error/fallback semantics relied on by `use_driver_in_path` flows and may violate the “no breaking changes on upgrade” requirement.

## Fix Focus Areas
- rust/src/lib.rs[804-854]
- rust/src/lib.rs[950-964]

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

2. Changed DASH_VERSION constants 📘 Rule violation ⚙ Maintainability

Description

The public constants DASH_VERSION and DASH_DASH_VERSION were modified, which is a
behavior-breaking change for downstream consumers that may rely on the previous values/format.
Preserve backward compatibility by keeping the old constants (deprecated) and introducing new
argv-flag constants (or equivalent migration layer).

Code

rust/src/lib.rs[R87-88]

Evidence

PR Compliance ID 2 requires avoiding breaking changes to public APIs unless explicitly instructed.
DASH_VERSION and DASH_DASH_VERSION are public exported constants and were changed in the new
code at the cited lines, which can break downstream usage that depends on their prior values/format.

AGENTS.md: Maintain API/ABI compatibility unless explicitly instructed otherwise
rust/src/lib.rs[87-88]

Agent prompt

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

## Issue description
`DASH_VERSION` and `DASH_DASH_VERSION` are `pub const` and their values were changed, which can break downstream consumers relying on the previous constant semantics.

## Issue Context
This PR changes command execution to argv-style. To avoid breaking external users, keep backward-compatible constants (deprecated with a message pointing to the replacement) and add new constants for the argv flags.

## Fix Focus Areas
- rust/src/lib.rs[87-88]

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

3. ~~Broken version argv flag~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

On non-Windows, general_discover_browser_version passes cmd_version_arg as a literal argv
element, but Chrome/Edge/Firefox callers pass the format templates
DASH_DASH_VERSION/DASH_VERSION (e.g. "{}{}{} --version"), so the browser is invoked with an
invalid argument and version discovery will fail.

Code

rust/src/lib.rs[R1207-1210]

Evidence

The non-Windows code path uses cmd_version_arg directly as an argument, but the values provided by
the main browser managers are DASH_VERSION / DASH_DASH_VERSION, which still contain {}
placeholders. This results in executing the browser with a literal template argument, preventing
valid version output and breaking version parsing.

rust/src/lib.rs[1169-1211]
rust/src/lib.rs[81-88]
rust/src/chrome.rs[281-287]
rust/src/edge.rs[165-189]
rust/src/firefox.rs[194-200]

Agent prompt

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

### Issue description
`general_discover_browser_version` now builds a direct `std::process::Command` with `args = [cmd_version_arg]`. However, current callers pass `DASH_VERSION` / `DASH_DASH_VERSION`, which are *format templates* (contain `{}` placeholders) and were previously only safe because the old code formatted them into a full command string before going through `sh -c`.

This makes non-Windows browser version discovery invoke the browser with an invalid literal argument like `"{}{}{} --version"`.

### Issue Context
- `DASH_VERSION` / `DASH_DASH_VERSION` are currently defined as format templates.
- Chrome/Edge/Firefox pass these constants into `general_discover_browser_version`.
- The non-Windows path now directly uses `cmd_version_arg.to_string()` as an argv value.

### Fix Focus Areas
- rust/src/lib.rs[87-88]
- rust/src/lib.rs[1169-1211]
- rust/src/chrome.rs[281-287]
- rust/src/edge.rs[165-189]
- rust/src/firefox.rs[194-200]

### Suggested fix
Option A (minimal): Change the constants to be actual flags:
- `DASH_VERSION = "-v"`
- `DASH_DASH_VERSION = "--version"`

Option B (API cleanup): Change `general_discover_browser_version` to accept `Vec<String>` (or `&[&str]`) for version args, and update each manager to pass `vec!["--version".into()]` (or `vec!["-v".into()]`).

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

View more (3)

4. Removed public command constants 📘 Rule violation ⚙ Maintainability

Description

Multiple pub const command-related constants were removed from lib.rs, which breaks any external
consumers importing them. Reintroduce them as deprecated constants (or provide a documented
replacement) for at least one deprecation cycle.

Code

rust/src/lib.rs[88]

Evidence
The checklist requires deprecating public functionality before removal and maintaining API
compatibility. The public constants section in lib.rs no longer defines previously exported
command constants (e.g., registry/version and command template constants), indicating they were
removed from the public API surface.
AGENTS.md: Maintain API/ABI compatibility for user-facing interfaces
AGENTS.md: Deprecate public functionality before removal and provide an alternative
rust/src/lib.rs[78-103]

Agent prompt

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

## Issue description
Public constants were removed from `rust/src/lib.rs`, which is a breaking change for downstream users who import these constants.

## Issue Context
This repo is non-semver per checklist expectations; removals should be deprecated first and kept for at least one cycle with guidance to alternatives.

## Fix Focus Areas
- rust/src/lib.rs[78-103]

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

5. uncompress_dmg() signature changed 📘 Rule violation ⚙ Maintainability

Description

Public function uncompress_dmg removed the os: &str parameter, which is a breaking signature
change for external users. Keep the previous signature as a deprecated wrapper that forwards to the
new implementation.

Code

rust/src/files.rs[R242-246]

Evidence
The checklist forbids breaking public API/signature changes without a deprecation path. files is
publicly exported and uncompress_dmg is public with a new signature, indicating a breaking change
for existing consumers.
AGENTS.md: Maintain API/ABI compatibility for user-facing interfaces
rust/src/lib.rs[57-76]
rust/src/files.rs[241-246]
Best Practice: Learned patterns

Agent prompt

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

## Issue description
`uncompress_dmg` is `pub` and its parameter list changed (removed `os: &str`), which breaks callers compiled against the previous signature.

## Issue Context
The `files` module is publicly exported, so this is a user-facing API surface.

## Fix Focus Areas
- rust/src/files.rs[241-246]
- rust/src/lib.rs[57-76]

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

6. shell::Command API breaking 📘 Rule violation ⚙ Maintainability

Description

Public selenium_manager::shell::Command and command runners were reshaped (now program + args,
and run_shell_command_with_log no longer takes os), which breaks downstream callers expecting
the prior constructors/helpers. Preserve backward compatibility by keeping the old API as deprecated
wrappers or providing an explicit migration layer.

Code

rust/src/shell.rs[R25-58]

Evidence
The checklist requires maintaining public API/signature compatibility (or deprecating before
breaking). shell is publicly exported, and the Command type plus
run_shell_command_with_log/run_shell_command now have a different public shape/signature,
implying a breaking change for existing consumers.
AGENTS.md: Maintain API/ABI compatibility for user-facing interfaces
rust/src/lib.rs[57-76]
rust/src/shell.rs[25-58]
Best Practice: Learned patterns

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 `selenium_manager::shell` API changed in a breaking way: `Command` structure/constructors and runner function signatures were altered, and prior entrypoints are no longer available.

## Issue Context
`pub mod shell;` makes this surface public, so removing/changing public constructors/functions breaks external crates that import and call them.

## Fix Focus Areas
- rust/src/shell.rs[25-58]
- rust/src/lib.rs[57-76]

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

7. No tests for new fallback gating 📘 Rule violation ☼ Reliability

Description

The new condition *is_driver_in_path && !self.is_fallback_driver_from_cache() changes
error-suppression behavior but no focused unit test was added to prevent regressions in this
user-visible decision path.

Code

rust/src/lib.rs[955]

Evidence

PR Compliance ID 4 requires adding appropriate tests for behavior changes when feasible. The
modified conditional at rust/src/lib.rs:955 changes control flow (returning Ok(()) vs
Err(err)), but there is no accompanying test change in this focused area to lock in the new
behavior.

AGENTS.md: Prefer tests-first and favor small unit tests over browser tests; avoid mocks
rust/src/lib.rs[950-963]

Agent prompt

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

## Issue description
`check_error_with_driver_in_path` now suppresses errors only when `*is_driver_in_path` is true **and** `fallback_driver_from_cache` is false. This behavior change is not covered by a unit test, increasing regression risk.

## Issue Context
The conditional at `rust/src/lib.rs:955` is a key decision point for whether Selenium Manager aborts or continues when a driver is found in PATH.

## Fix Focus Areas
- rust/src/lib.rs[950-963]

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

8. ~~Trace logs empty browser path~~ ✓ Resolved 🐞 Bug ◔ Observability

Description

The trace log at line 1177 fires before detect_browser_path() is called (lines 1178-1181), so when
get_browser_path() returns an empty string the trace message shows an empty path, giving a
misleading diagnostic when --trace is used to debug browser detection failures.

Code

rust/src/lib.rs[R1176-1177]

Evidence

browser_path is assigned from get_browser_path() at line 1175 (may be empty), then immediately
traced at 1176-1177. The actual path is only populated later at line 1180 via detect_browser_path().
The trace therefore always reflects the pre-detection state, which is empty in the common case where
no explicit --browser-path was supplied.

rust/src/lib.rs[1175-1181]

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 trace log `*** Browser path {}` is emitted before `detect_browser_path()` is called, so it always shows the raw (often empty) value from `get_browser_path()` rather than the resolved path that will actually be used.

## Issue Context
In `general_discover_browser_version`, `browser_path` starts as `self.get_browser_path().to_string()` (empty when no `--browser-path` flag is given). The trace is logged immediately, then `detect_browser_path()` may populate it. The trace should reflect the final resolved path.

## Fix Focus Areas
- rust/src/lib.rs[1175-1181]: Move the `.trace(...)` call to after the `if browser_path.is_empty()` block (i.e., after line 1181), so it logs the path that will actually be used for version probing.

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

qodo-code-review · 2026-05-27T05:38:03Z

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

qodo-code-review · 2026-05-27T06:07:33Z

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

qodo-code-review · 2026-05-27T07:55:26Z

Code review by qodo was updated up to the latest commit 9d65292

qodo-code-review · 2026-05-27T08:06:21Z

Code review by qodo was updated up to the latest commit 03591e5

qodo-code-review · 2026-05-27T08:23:32Z

Code review by qodo was updated up to the latest commit 519d841

[rust] Change command execution to argv

4b0d4e9

selenium-ci added C-rust Rust code is mostly Selenium Manager B-manager Selenium Manager labels May 27, 2026

qodo-code-review Bot reviewed May 27, 2026

View reviewed changes

Comment thread rust/src/shell.rs

Comment thread rust/src/files.rs

Comment thread rust/src/lib.rs

Comment thread rust/src/lib.rs

[rust] Fix dash version constants

ebdf852

qodo-code-review Bot reviewed May 27, 2026

View reviewed changes

Comment thread rust/src/lib.rs

[rust] Include trace log in browser path

b7a4b56

[rust] Check browser path in driver error logic

9d65292

qodo-code-review Bot reviewed May 27, 2026

View reviewed changes

Comment thread rust/src/lib.rs Outdated

[rust] Check fallback flag

03591e5

[rust] Fix condition for checking fallback

519d841

bonigarcia merged commit ab453da into trunk May 27, 2026
77 checks passed

bonigarcia deleted the sm_command_exec branch May 27, 2026 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[rust] Change command execution to argv#17576

[rust] Change command execution to argv#17576
bonigarcia merged 6 commits into
trunkfrom
sm_command_exec

bonigarcia commented May 27, 2026

Uh oh!

qodo-code-review Bot commented May 27, 2026

Uh oh!

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bonigarcia commented May 27, 2026

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

🤖 AI assistance

💡 Additional Considerations

🔄 Types of changes

Uh oh!

qodo-code-review Bot commented May 27, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

qodo-code-review Bot commented May 27, 2026 •

edited

Loading

qodo-code-review Bot commented May 27, 2026 •

edited

Loading