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 <noreply@anthropic.com>
This commit is contained in:
fettpl 2026-02-16 18:07:13 +01:00 committed by Chummy
parent 69a9adde33
commit 46b199c50f
3 changed files with 292 additions and 288 deletions

View file

@ -220,32 +220,34 @@ fn split_message(message: &str, max_bytes: usize) -> Vec<String> {
chunks
}
/// Configuration for constructing an `IrcChannel`.
pub struct IrcChannelConfig {
pub server: String,
pub port: u16,
pub nickname: String,
pub username: Option<String>,
pub channels: Vec<String>,
pub allowed_users: Vec<String>,
pub server_password: Option<String>,
pub nickserv_password: Option<String>,
pub sasl_password: Option<String>,
pub verify_tls: bool,
}
impl IrcChannel {
#[allow(clippy::too_many_arguments)]
pub fn new(
server: String,
port: u16,
nickname: String,
username: Option<String>,
channels: Vec<String>,
allowed_users: Vec<String>,
server_password: Option<String>,
nickserv_password: Option<String>,
sasl_password: Option<String>,
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,
})
}
}

View file

@ -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 {

View file

@ -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<BrowserAction> {
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 {