From 46b199c50f106fe961c6d2af15003743f50accb6 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Mon, 16 Feb 2026 18:07:13 +0100 Subject: [PATCH] refactor: extract browser action parsing and IRC config struct browser.rs: - Extract parse_browser_action() from Tool::execute, removing one #[allow(clippy::too_many_lines)] suppression irc.rs: - Replace 10-parameter IrcChannel::new() with IrcChannelConfig struct, removing #[allow(clippy::too_many_arguments)] suppression - Update all call sites (mod.rs and tests) Closes #366 Co-Authored-By: Claude Opus 4.6 --- src/channels/irc.rs | 216 ++++++++++++++--------------- src/channels/mod.rs | 48 +++---- src/tools/browser.rs | 316 ++++++++++++++++++++++--------------------- 3 files changed, 292 insertions(+), 288 deletions(-) diff --git a/src/channels/irc.rs b/src/channels/irc.rs index d63ad41..41c7d05 100644 --- a/src/channels/irc.rs +++ b/src/channels/irc.rs @@ -220,32 +220,34 @@ fn split_message(message: &str, max_bytes: usize) -> Vec { chunks } +/// Configuration for constructing an `IrcChannel`. +pub struct IrcChannelConfig { + pub server: String, + pub port: u16, + pub nickname: String, + pub username: Option, + pub channels: Vec, + pub allowed_users: Vec, + pub server_password: Option, + pub nickserv_password: Option, + pub sasl_password: Option, + pub verify_tls: bool, +} + impl IrcChannel { - #[allow(clippy::too_many_arguments)] - pub fn new( - server: String, - port: u16, - nickname: String, - username: Option, - channels: Vec, - allowed_users: Vec, - server_password: Option, - nickserv_password: Option, - sasl_password: Option, - verify_tls: bool, - ) -> Self { - let username = username.unwrap_or_else(|| nickname.clone()); + pub fn new(cfg: IrcChannelConfig) -> Self { + let username = cfg.username.unwrap_or_else(|| cfg.nickname.clone()); Self { - server, - port, - nickname, + server: cfg.server, + port: cfg.port, + nickname: cfg.nickname, username, - channels, - allowed_users, - server_password, - nickserv_password, - sasl_password, - verify_tls, + channels: cfg.channels, + allowed_users: cfg.allowed_users, + server_password: cfg.server_password, + nickserv_password: cfg.nickserv_password, + sasl_password: cfg.sasl_password, + verify_tls: cfg.verify_tls, writer: Arc::new(Mutex::new(None)), } } @@ -807,18 +809,18 @@ mod tests { #[test] fn specific_user_allowed() { - let ch = IrcChannel::new( - "irc.test".into(), - 6697, - "bot".into(), - None, - vec![], - vec!["alice".into(), "bob".into()], - None, - None, - None, - true, - ); + let ch = IrcChannel::new(IrcChannelConfig { + server: "irc.test".into(), + port: 6697, + nickname: "bot".into(), + username: None, + channels: vec![], + allowed_users: vec!["alice".into(), "bob".into()], + server_password: None, + nickserv_password: None, + sasl_password: None, + verify_tls: true, + }); assert!(ch.is_user_allowed("alice")); assert!(ch.is_user_allowed("bob")); assert!(!ch.is_user_allowed("eve")); @@ -826,18 +828,18 @@ mod tests { #[test] fn allowlist_case_insensitive() { - let ch = IrcChannel::new( - "irc.test".into(), - 6697, - "bot".into(), - None, - vec![], - vec!["Alice".into()], - None, - None, - None, - true, - ); + let ch = IrcChannel::new(IrcChannelConfig { + server: "irc.test".into(), + port: 6697, + nickname: "bot".into(), + username: None, + channels: vec![], + allowed_users: vec!["Alice".into()], + server_password: None, + nickserv_password: None, + sasl_password: None, + verify_tls: true, + }); assert!(ch.is_user_allowed("alice")); assert!(ch.is_user_allowed("ALICE")); assert!(ch.is_user_allowed("Alice")); @@ -845,18 +847,18 @@ mod tests { #[test] fn empty_allowlist_denies_all() { - let ch = IrcChannel::new( - "irc.test".into(), - 6697, - "bot".into(), - None, - vec![], - vec![], - None, - None, - None, - true, - ); + let ch = IrcChannel::new(IrcChannelConfig { + server: "irc.test".into(), + port: 6697, + nickname: "bot".into(), + username: None, + channels: vec![], + allowed_users: vec![], + server_password: None, + nickserv_password: None, + sasl_password: None, + verify_tls: true, + }); assert!(!ch.is_user_allowed("anyone")); } @@ -864,35 +866,35 @@ mod tests { #[test] fn new_defaults_username_to_nickname() { - let ch = IrcChannel::new( - "irc.test".into(), - 6697, - "mybot".into(), - None, - vec![], - vec![], - None, - None, - None, - true, - ); + let ch = IrcChannel::new(IrcChannelConfig { + server: "irc.test".into(), + port: 6697, + nickname: "mybot".into(), + username: None, + channels: vec![], + allowed_users: vec![], + server_password: None, + nickserv_password: None, + sasl_password: None, + verify_tls: true, + }); assert_eq!(ch.username, "mybot"); } #[test] fn new_uses_explicit_username() { - let ch = IrcChannel::new( - "irc.test".into(), - 6697, - "mybot".into(), - Some("customuser".into()), - vec![], - vec![], - None, - None, - None, - true, - ); + let ch = IrcChannel::new(IrcChannelConfig { + server: "irc.test".into(), + port: 6697, + nickname: "mybot".into(), + username: Some("customuser".into()), + channels: vec![], + allowed_users: vec![], + server_password: None, + nickserv_password: None, + sasl_password: None, + verify_tls: true, + }); assert_eq!(ch.username, "customuser"); assert_eq!(ch.nickname, "mybot"); } @@ -905,18 +907,18 @@ mod tests { #[test] fn new_stores_all_fields() { - let ch = IrcChannel::new( - "irc.example.com".into(), - 6697, - "zcbot".into(), - Some("zeroclaw".into()), - vec!["#test".into()], - vec!["alice".into()], - Some("serverpass".into()), - Some("nspass".into()), - Some("saslpass".into()), - false, - ); + let ch = IrcChannel::new(IrcChannelConfig { + server: "irc.example.com".into(), + port: 6697, + nickname: "zcbot".into(), + username: Some("zeroclaw".into()), + channels: vec!["#test".into()], + allowed_users: vec!["alice".into()], + server_password: Some("serverpass".into()), + nickserv_password: Some("nspass".into()), + sasl_password: Some("saslpass".into()), + verify_tls: false, + }); assert_eq!(ch.server, "irc.example.com"); assert_eq!(ch.port, 6697); assert_eq!(ch.nickname, "zcbot"); @@ -995,17 +997,17 @@ nickname = "bot" // ── Helpers ───────────────────────────────────────────── fn make_channel() -> IrcChannel { - IrcChannel::new( - "irc.example.com".into(), - 6697, - "zcbot".into(), - None, - vec!["#zeroclaw".into()], - vec!["*".into()], - None, - None, - None, - true, - ) + IrcChannel::new(IrcChannelConfig { + server: "irc.example.com".into(), + port: 6697, + nickname: "zcbot".into(), + username: None, + channels: vec!["#zeroclaw".into()], + allowed_users: vec!["*".into()], + server_password: None, + nickserv_password: None, + sasl_password: None, + verify_tls: true, + }) } } diff --git a/src/channels/mod.rs b/src/channels/mod.rs index 1a161ad..a132eae 100644 --- a/src/channels/mod.rs +++ b/src/channels/mod.rs @@ -672,18 +672,18 @@ pub async fn doctor_channels(config: Config) -> Result<()> { if let Some(ref irc) = config.channels_config.irc { channels.push(( "IRC", - Arc::new(IrcChannel::new( - irc.server.clone(), - irc.port, - irc.nickname.clone(), - irc.username.clone(), - irc.channels.clone(), - irc.allowed_users.clone(), - irc.server_password.clone(), - irc.nickserv_password.clone(), - irc.sasl_password.clone(), - irc.verify_tls.unwrap_or(true), - )), + Arc::new(IrcChannel::new(irc::IrcChannelConfig { + server: irc.server.clone(), + port: irc.port, + nickname: irc.nickname.clone(), + username: irc.username.clone(), + channels: irc.channels.clone(), + allowed_users: irc.allowed_users.clone(), + server_password: irc.server_password.clone(), + nickserv_password: irc.nickserv_password.clone(), + sasl_password: irc.sasl_password.clone(), + verify_tls: irc.verify_tls.unwrap_or(true), + })), )); } @@ -947,18 +947,18 @@ pub async fn start_channels(config: Config) -> Result<()> { } if let Some(ref irc) = config.channels_config.irc { - channels.push(Arc::new(IrcChannel::new( - irc.server.clone(), - irc.port, - irc.nickname.clone(), - irc.username.clone(), - irc.channels.clone(), - irc.allowed_users.clone(), - irc.server_password.clone(), - irc.nickserv_password.clone(), - irc.sasl_password.clone(), - irc.verify_tls.unwrap_or(true), - ))); + channels.push(Arc::new(IrcChannel::new(irc::IrcChannelConfig { + server: irc.server.clone(), + port: irc.port, + nickname: irc.nickname.clone(), + username: irc.username.clone(), + channels: irc.channels.clone(), + allowed_users: irc.allowed_users.clone(), + server_password: irc.server_password.clone(), + nickserv_password: irc.nickserv_password.clone(), + sasl_password: irc.sasl_password.clone(), + verify_tls: irc.verify_tls.unwrap_or(true), + }))); } if let Some(ref lk) = config.channels_config.lark { diff --git a/src/tools/browser.rs b/src/tools/browser.rs index fe3be26..c475969 100644 --- a/src/tools/browser.rs +++ b/src/tools/browser.rs @@ -854,7 +854,6 @@ impl BrowserTool { } } -#[allow(clippy::too_many_lines)] #[async_trait] impl Tool for BrowserTool { fn name(&self) -> &str { @@ -1031,165 +1030,13 @@ impl Tool for BrowserTool { return self.execute_computer_use_action(action_str, &args).await; } - let action = match action_str { - "open" => { - let url = args - .get("url") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'url' for open action"))?; - BrowserAction::Open { url: url.into() } - } - "snapshot" => BrowserAction::Snapshot { - interactive_only: args - .get("interactive_only") - .and_then(serde_json::Value::as_bool) - .unwrap_or(true), // Default to interactive for AI - compact: args - .get("compact") - .and_then(serde_json::Value::as_bool) - .unwrap_or(true), - depth: args - .get("depth") - .and_then(serde_json::Value::as_u64) - .map(|d| u32::try_from(d).unwrap_or(u32::MAX)), - }, - "click" => { - let selector = args - .get("selector") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for click"))?; - BrowserAction::Click { - selector: selector.into(), - } - } - "fill" => { - let selector = args - .get("selector") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for fill"))?; - let value = args - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'value' for fill"))?; - BrowserAction::Fill { - selector: selector.into(), - value: value.into(), - } - } - "type" => { - let selector = args - .get("selector") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for type"))?; - let text = args - .get("text") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'text' for type"))?; - BrowserAction::Type { - selector: selector.into(), - text: text.into(), - } - } - "get_text" => { - let selector = args - .get("selector") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for get_text"))?; - BrowserAction::GetText { - selector: selector.into(), - } - } - "get_title" => BrowserAction::GetTitle, - "get_url" => BrowserAction::GetUrl, - "screenshot" => BrowserAction::Screenshot { - path: args.get("path").and_then(|v| v.as_str()).map(String::from), - full_page: args - .get("full_page") - .and_then(serde_json::Value::as_bool) - .unwrap_or(false), - }, - "wait" => BrowserAction::Wait { - selector: args - .get("selector") - .and_then(|v| v.as_str()) - .map(String::from), - ms: args.get("ms").and_then(serde_json::Value::as_u64), - text: args.get("text").and_then(|v| v.as_str()).map(String::from), - }, - "press" => { - let key = args - .get("key") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'key' for press"))?; - BrowserAction::Press { key: key.into() } - } - "hover" => { - let selector = args - .get("selector") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for hover"))?; - BrowserAction::Hover { - selector: selector.into(), - } - } - "scroll" => { - let direction = args - .get("direction") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'direction' for scroll"))?; - BrowserAction::Scroll { - direction: direction.into(), - pixels: args - .get("pixels") - .and_then(serde_json::Value::as_u64) - .map(|p| u32::try_from(p).unwrap_or(u32::MAX)), - } - } - "is_visible" => { - let selector = args - .get("selector") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for is_visible"))?; - BrowserAction::IsVisible { - selector: selector.into(), - } - } - "close" => BrowserAction::Close, - "find" => { - let by = args - .get("by") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'by' for find"))?; - let value = args - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'value' for find"))?; - let action = args - .get("find_action") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'find_action' for find"))?; - BrowserAction::Find { - by: by.into(), - value: value.into(), - action: action.into(), - fill_value: args - .get("fill_value") - .and_then(|v| v.as_str()) - .map(String::from), - } - } - _ => { + let action = match parse_browser_action(action_str, &args) { + Ok(a) => a, + Err(e) => { return Ok(ToolResult { success: false, output: String::new(), - error: Some(format!( - "Action '{action_str}' is unavailable for backend '{}'", - match backend { - ResolvedBackend::AgentBrowser => "agent_browser", - ResolvedBackend::RustNative => "rust_native", - ResolvedBackend::ComputerUse => "computer_use", - } - )), + error: Some(e.to_string()), }); } }; @@ -1871,6 +1718,161 @@ mod native_backend { } } +// ── Action parsing ────────────────────────────────────────────── + +/// Parse a JSON `args` object into a typed `BrowserAction`. +fn parse_browser_action(action_str: &str, args: &Value) -> anyhow::Result { + match action_str { + "open" => { + let url = args + .get("url") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'url' for open action"))?; + Ok(BrowserAction::Open { url: url.into() }) + } + "snapshot" => Ok(BrowserAction::Snapshot { + interactive_only: args + .get("interactive_only") + .and_then(serde_json::Value::as_bool) + .unwrap_or(true), + compact: args + .get("compact") + .and_then(serde_json::Value::as_bool) + .unwrap_or(true), + depth: args + .get("depth") + .and_then(serde_json::Value::as_u64) + .map(|d| u32::try_from(d).unwrap_or(u32::MAX)), + }), + "click" => { + let selector = args + .get("selector") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for click"))?; + Ok(BrowserAction::Click { + selector: selector.into(), + }) + } + "fill" => { + let selector = args + .get("selector") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for fill"))?; + let value = args + .get("value") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'value' for fill"))?; + Ok(BrowserAction::Fill { + selector: selector.into(), + value: value.into(), + }) + } + "type" => { + let selector = args + .get("selector") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for type"))?; + let text = args + .get("text") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'text' for type"))?; + Ok(BrowserAction::Type { + selector: selector.into(), + text: text.into(), + }) + } + "get_text" => { + let selector = args + .get("selector") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for get_text"))?; + Ok(BrowserAction::GetText { + selector: selector.into(), + }) + } + "get_title" => Ok(BrowserAction::GetTitle), + "get_url" => Ok(BrowserAction::GetUrl), + "screenshot" => Ok(BrowserAction::Screenshot { + path: args.get("path").and_then(|v| v.as_str()).map(String::from), + full_page: args + .get("full_page") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false), + }), + "wait" => Ok(BrowserAction::Wait { + selector: args + .get("selector") + .and_then(|v| v.as_str()) + .map(String::from), + ms: args.get("ms").and_then(serde_json::Value::as_u64), + text: args.get("text").and_then(|v| v.as_str()).map(String::from), + }), + "press" => { + let key = args + .get("key") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'key' for press"))?; + Ok(BrowserAction::Press { key: key.into() }) + } + "hover" => { + let selector = args + .get("selector") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for hover"))?; + Ok(BrowserAction::Hover { + selector: selector.into(), + }) + } + "scroll" => { + let direction = args + .get("direction") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'direction' for scroll"))?; + Ok(BrowserAction::Scroll { + direction: direction.into(), + pixels: args + .get("pixels") + .and_then(serde_json::Value::as_u64) + .map(|p| u32::try_from(p).unwrap_or(u32::MAX)), + }) + } + "is_visible" => { + let selector = args + .get("selector") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'selector' for is_visible"))?; + Ok(BrowserAction::IsVisible { + selector: selector.into(), + }) + } + "close" => Ok(BrowserAction::Close), + "find" => { + let by = args + .get("by") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'by' for find"))?; + let value = args + .get("value") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'value' for find"))?; + let action = args + .get("find_action") + .and_then(|v| v.as_str()) + .ok_or_else(|| anyhow::anyhow!("Missing 'find_action' for find"))?; + Ok(BrowserAction::Find { + by: by.into(), + value: value.into(), + action: action.into(), + fill_value: args + .get("fill_value") + .and_then(|v| v.as_str()) + .map(String::from), + }) + } + other => anyhow::bail!("Unsupported browser action: {other}"), + } +} + // ── Helper functions ───────────────────────────────────────────── fn is_supported_browser_action(action: &str) -> bool {