From d34ce8c2d09e5a7694704b919b4b50eb7e03d3be Mon Sep 17 00:00:00 2001 From: Chris Tate Date: Wed, 4 Feb 2026 23:14:31 -0600 Subject: [PATCH] fix: only warn about ignored flags when explicitly passed via CLI (#373) The warning about launch-time options being ignored (when daemon is already running) was incorrectly shown when options were set via environment variables like AGENT_BROWSER_EXECUTABLE_PATH, even when no CLI flag was passed. Now the warning only appears when flags are explicitly passed on the command line, not when values come solely from environment variables. Fixes #372 --- cli/src/commands.rs | 8 ++++++ cli/src/flags.rs | 68 +++++++++++++++++++++++++++++++++++++++++++++ cli/src/main.rs | 53 +++++++++++++++++++++++++---------- 3 files changed, 115 insertions(+), 14 deletions(-) diff --git a/cli/src/commands.rs b/cli/src/commands.rs index 50118ab..0d135f9 100644 --- a/cli/src/commands.rs +++ b/cli/src/commands.rs @@ -1425,6 +1425,14 @@ mod tests { provider: None, ignore_https_errors: false, device: None, + cli_executable_path: false, + cli_extensions: false, + cli_profile: false, + cli_state: false, + cli_args: false, + cli_user_agent: false, + cli_proxy: false, + cli_proxy_bypass: false, } } diff --git a/cli/src/flags.rs b/cli/src/flags.rs index d97a275..1a10995 100644 --- a/cli/src/flags.rs +++ b/cli/src/flags.rs @@ -19,6 +19,17 @@ pub struct Flags { pub provider: Option, pub ignore_https_errors: bool, pub device: Option, + + // Track which launch-time options were explicitly passed via CLI + // (as opposed to being set only via environment variables) + pub cli_executable_path: bool, + pub cli_extensions: bool, + pub cli_profile: bool, + pub cli_state: bool, + pub cli_args: bool, + pub cli_user_agent: bool, + pub cli_proxy: bool, + pub cli_proxy_bypass: bool, } pub fn parse_flags(args: &[String]) -> Flags { @@ -51,6 +62,15 @@ pub fn parse_flags(args: &[String]) -> Flags { provider: env::var("AGENT_BROWSER_PROVIDER").ok(), ignore_https_errors: false, device: env::var("AGENT_BROWSER_IOS_DEVICE").ok(), + // Track CLI-passed flags (default false, set to true when flag is passed) + cli_executable_path: false, + cli_extensions: false, + cli_profile: false, + cli_state: false, + cli_args: false, + cli_user_agent: false, + cli_proxy: false, + cli_proxy_bypass: false, }; let mut i = 0; @@ -75,12 +95,14 @@ pub fn parse_flags(args: &[String]) -> Flags { "--executable-path" => { if let Some(s) = args.get(i + 1) { flags.executable_path = Some(s.clone()); + flags.cli_executable_path = true; i += 1; } } "--extension" => { if let Some(s) = args.get(i + 1) { flags.extensions.push(s.clone()); + flags.cli_extensions = true; i += 1; } } @@ -93,36 +115,42 @@ pub fn parse_flags(args: &[String]) -> Flags { "--profile" => { if let Some(s) = args.get(i + 1) { flags.profile = Some(s.clone()); + flags.cli_profile = true; i += 1; } } "--state" => { if let Some(s) = args.get(i + 1) { flags.state = Some(s.clone()); + flags.cli_state = true; i += 1; } } "--proxy" => { if let Some(p) = args.get(i + 1) { flags.proxy = Some(p.clone()); + flags.cli_proxy = true; i += 1; } } "--proxy-bypass" => { if let Some(s) = args.get(i + 1) { flags.proxy_bypass = Some(s.clone()); + flags.cli_proxy_bypass = true; i += 1; } } "--args" => { if let Some(s) = args.get(i + 1) { flags.args = Some(s.clone()); + flags.cli_args = true; i += 1; } } "--user-agent" => { if let Some(s) = args.get(i + 1) { flags.user_agent = Some(s.clone()); + flags.cli_user_agent = true; i += 1; } } @@ -311,4 +339,44 @@ mod tests { assert_eq!(flags.session, "test"); assert_eq!(flags.executable_path, Some("/custom/chrome".to_string())); } + + #[test] + fn test_cli_executable_path_tracking() { + // When --executable-path is passed via CLI, cli_executable_path should be true + let flags = parse_flags(&args("--executable-path /path/to/chrome snapshot")); + assert!(flags.cli_executable_path); + assert_eq!(flags.executable_path, Some("/path/to/chrome".to_string())); + } + + #[test] + fn test_cli_executable_path_not_set_without_flag() { + // When no --executable-path is passed, cli_executable_path should be false + // (even if env var sets executable_path to Some value, which we can't test here) + let flags = parse_flags(&args("snapshot")); + assert!(!flags.cli_executable_path); + } + + #[test] + fn test_cli_extension_tracking() { + let flags = parse_flags(&args("--extension /path/to/ext snapshot")); + assert!(flags.cli_extensions); + } + + #[test] + fn test_cli_profile_tracking() { + let flags = parse_flags(&args("--profile /path/to/profile snapshot")); + assert!(flags.cli_profile); + } + + #[test] + fn test_cli_multiple_flags_tracking() { + let flags = parse_flags(&args( + "--executable-path /chrome --profile /profile --proxy http://proxy snapshot", + )); + assert!(flags.cli_executable_path); + assert!(flags.cli_profile); + assert!(flags.cli_proxy); + assert!(!flags.cli_extensions); + assert!(!flags.cli_state); + } } diff --git a/cli/src/main.rs b/cli/src/main.rs index a79d62c..5619818 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -221,22 +221,51 @@ fn main() { } }; - // Warn if launch-time options were specified but daemon was already running + // Warn if launch-time options were explicitly passed via CLI but daemon was already running + // Only warn about flags that were passed on the command line, not those set via environment + // variables (since the daemon already uses the env vars when it starts). if daemon_result.already_running { - let has_extensions = !flags.extensions.is_empty(); let ignored_flags: Vec<&str> = [ - flags.executable_path.as_ref().map(|_| "--executable-path"), - if has_extensions { + if flags.cli_executable_path { + Some("--executable-path") + } else { + None + }, + if flags.cli_extensions { Some("--extension") } else { None }, - flags.profile.as_ref().map(|_| "--profile"), - flags.state.as_ref().map(|_| "--state"), - flags.args.as_ref().map(|_| "--args"), - flags.user_agent.as_ref().map(|_| "--user-agent"), - flags.proxy.as_ref().map(|_| "--proxy"), - flags.proxy_bypass.as_ref().map(|_| "--proxy-bypass"), + if flags.cli_profile { + Some("--profile") + } else { + None + }, + if flags.cli_state { + Some("--state") + } else { + None + }, + if flags.cli_args { + Some("--args") + } else { + None + }, + if flags.cli_user_agent { + Some("--user-agent") + } else { + None + }, + if flags.cli_proxy { + Some("--proxy") + } else { + None + }, + if flags.cli_proxy_bypass { + Some("--proxy-bypass") + } else { + None + }, flags.ignore_https_errors.then(|| "--ignore-https-errors"), ] .into_iter() @@ -250,10 +279,6 @@ fn main() { ignored_flags.join(", ") ); } - - if flags.ignore_https_errors { - eprintln!("{} --ignore-https-errors ignored: daemon already running. Use 'agent-browser close' first to restart with this option.", color::warning_indicator()); - } } // Validate mutually exclusive options