From c02360a87331a7d045e7bd17d88249380765b055 Mon Sep 17 00:00:00 2001 From: Breadway Date: Wed, 26 Aug 2026 12:33:07 +0800 Subject: [PATCH] Fix bugs from a full audit: Tailscale recovery, SSID verification, captive portals, config races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The watch loop could spin forever on a stopped Tailscale daemon (the auto-start path was unreachable) while re-notifying every retry, report "connected" when NM autoconnect won a race onto a different SSID, and classify captive portals as healthy (200/302 accepted as internet). The bread-bus subscription thread also read/wrote config and state files concurrently with the watch loop. Fix all of those plus: EDITOR values with arguments, scan --to silently ignoring unknown profiles, deleted core profiles being resurrected, inline-network migration data loss, login never retried, XDG-unaware install-service, detect persisting a stale default profile, password length leaking through the mask, a stdin/stdout pipe deadlock, and several minor UI/robustness issues. Generated with Codebuff πŸ€– Co-Authored-By: Codebuff --- src/app.rs | 115 +++++++++++++++++++++++-------------- src/bread_events.rs | 49 ++++++++++------ src/config.rs | 67 ++++++++++++++++++++-- src/flow.rs | 137 ++++++++++++++++++++++++++++++-------------- src/nm.rs | 39 ++++++++++--- src/status.rs | 13 ++++- src/tailscale.rs | 34 ++++++++++- src/util.rs | 17 +++--- src/watch.rs | 133 ++++++++++++++++++++++++++++++------------ tests/cli.rs | 7 ++- tests/common/mod.rs | 22 +++++++ tests/flow_watch.rs | 94 ++++++++++++++++++++++-------- 12 files changed, 538 insertions(+), 189 deletions(-) diff --git a/src/app.rs b/src/app.rs index fd58751..31c35df 100644 --- a/src/app.rs +++ b/src/app.rs @@ -4,6 +4,7 @@ //! consumers (including the integration tests under `tests/`). use std::io::{BufRead, Write}; +use std::path::PathBuf; use std::process::Command; use std::time::Duration; @@ -66,9 +67,11 @@ enum Cmd { ssid: String, /// Password (prompted if omitted) password: Option, - /// Network is hidden (does not broadcast its SSID) - #[arg(long)] - hidden: bool, + /// Network is hidden (does not broadcast its SSID). + /// `--hidden` sets it; `--hidden=false` clears it on an existing + /// entry; omitted leaves an existing entry's flag untouched. + #[arg(long, num_args = 0..=1, default_missing_value = "true")] + hidden: Option, /// Attach this SSID to a profile's priority list #[arg(long)] to: Option, @@ -321,8 +324,15 @@ fn detect_profile(cfg: &Config) -> Option { } } - // Fall back to the default profile if no markers matched. - Some(cfg.settings.default_profile.clone()) + // Fall back to the default profile if no markers matched β€” but only if + // it actually exists: a stale `default_profile` name is a config error, + // not a detection result, and persisting it would wedge the watcher in + // UnknownProfile forever. + if cfg.profiles.contains_key(&cfg.settings.default_profile) { + Some(cfg.settings.default_profile.clone()) + } else { + None + } } fn cmd_detect(cfg: &mut Config, apply: bool) -> Result { @@ -330,18 +340,19 @@ fn cmd_detect(cfg: &mut Config, apply: bool) -> Result { Some(p) => { println!("{p}"); if apply { - State { - profile: p.clone(), - updated: crate::util::timestamp(), - } - .save()?; + // Route through state::set_profile (like the CLI and the + // bread bus do) so an unknown fallback is rejected with a + // proper error instead of being persisted as active. + state::set_profile(cfg, &p)?; let outcome = flow::run(cfg, &p); print_outcome(&p, &outcome); return Ok(if outcome.ok() { 0 } else { 1 }); } Ok(0) } - None => Err("could not detect a profile (no Wi-Fi adapter?)".into()), + None => Err("could not detect a profile (no Wi-Fi adapter, or no \ + profile matches and the default is misconfigured)" + .into()), } } @@ -385,7 +396,7 @@ fn cmd_add( cfg: &mut Config, ssid: String, password: Option, - hidden: bool, + hidden: Option, to: Option, at: Option, ) -> Result { @@ -397,12 +408,17 @@ fn cmd_add( match cfg.networks.iter_mut().find(|n| n.ssid == ssid) { Some(n) => { n.password = password; - n.hidden = hidden || n.hidden; + // `--hidden` / `--hidden=false` set the flag explicitly; when + // the flag is omitted, leave an existing entry's hidden state + // alone (a password-only update must not un-hide a network). + if let Some(h) = hidden { + n.hidden = h; + } } None => cfg.networks.push(NetworkDef { ssid: ssid.clone(), password, - hidden, + hidden: hidden.unwrap_or(false), }), } if let Some(prof_name) = to { @@ -443,6 +459,14 @@ fn cmd_forget(cfg: &mut Config, ssid: &str) -> Result { } fn cmd_scan(cfg: &mut Config, to: Option) -> Result { + // Validate `--to` up front, before any side effects (connecting is + // one): `add --to` errors on an unknown profile, so `scan --to` must + // too instead of silently saving a network that never gets attached. + if let Some(prof_name) = &to { + if !cfg.profiles.contains_key(prof_name) { + return Err(format!("unknown profile '{prof_name}'")); + } + } let iface = nm::wifi_interface().ok_or("no Wi-Fi adapter")?; nm::radio_on(); nm::rescan(&iface, &[]); @@ -502,12 +526,13 @@ fn cmd_scan(cfg: &mut Config, to: Option) -> Result { Ok(0) } -/// Mask a secret for display. Operates on chars (not bytes) so multi-byte -/// UTF-8 passwords don't panic on a mid-character byte slice, and never -/// echoes back any real character of the secret (previously the first byte -/// was shown unmasked). -fn mask(p: &str) -> String { - "β€’".repeat(p.chars().count().max(2)) +/// Mask a secret for display. Always renders the same fixed-length +/// placeholder so the output reveals neither the secret's length nor any +/// character of it (a fixed placeholder is what password managers show; +/// length-hiding also means multi-byte UTF-8 passwords need no special +/// handling). +fn mask(_p: &str) -> String { + "β€’".repeat(8) } fn cmd_list(cfg: &Config, show_pw: bool) -> Result { @@ -567,7 +592,15 @@ fn cmd_list(cfg: &Config, show_pw: bool) -> Result { fn cmd_edit() -> Result { let editor = std::env::var("EDITOR").unwrap_or_else(|_| "nano".into()); let path = config::config_path(); - let status = Command::new(&editor) + // EDITOR values routinely carry arguments ("code -w", "subl -w"), so + // split on whitespace: the first token is the program, the rest are its + // arguments. The path stays a separate argument β€” never interpolated + // into a shell string β€” so it can't be used for injection. + let mut parts = editor.split_whitespace(); + let prog = parts.next().unwrap_or("nano"); + let mut cmd = Command::new(prog); + cmd.args(parts); + let status = cmd .arg(&path) .status() .map_err(|e| format!("launching {editor}: {e}"))?; @@ -685,7 +718,13 @@ fn exec_replace(prog: &str, dir: &std::path::Path) -> String { } fn cmd_install_service(enable: bool) -> Result { - let unit_dir = home_dir().join(".config/systemd/user"); + // Honor XDG_CONFIG_HOME like the rest of the app: systemd --user units + // live in $XDG_CONFIG_HOME/systemd/user (default ~/.config/systemd/user). + let unit_dir = std::env::var_os("XDG_CONFIG_HOME") + .map(PathBuf::from) + .unwrap_or_else(|| home_dir().join(".config")) + .join("systemd") + .join("user"); std::fs::create_dir_all(&unit_dir) .map_err(|e| format!("creating {}: {e}", unit_dir.display()))?; let bin = std::env::current_exe().map_err(|e| format!("resolving current executable: {e}"))?; @@ -746,24 +785,15 @@ mod tests { use super::*; #[test] - fn mask_empty_password() { - // len() == 0 <= 2 branch: still at least 2 dots so an empty saved - // password doesn't visually collapse to nothing in `list`. - assert_eq!(mask(""), "β€’β€’"); - } - - #[test] - fn mask_short_passwords_reveal_nothing() { - assert_eq!(mask("a"), "β€’β€’"); - assert_eq!(mask("ab"), "β€’β€’"); - } - - #[test] - fn mask_never_echoes_a_real_character() { - let pw = "hunter2"; - let masked = mask(pw); - assert_eq!(masked, "β€’".repeat(pw.len())); - assert!(!masked.contains('h'), "masked output leaked the first char"); + fn mask_is_fixed_length_regardless_of_secret() { + // Fixed-length masking: the output must reveal neither the secret's + // length nor any character of it β€” for empty, short, and long + // secrets alike. + assert_eq!(mask(""), "β€’".repeat(8)); + assert_eq!(mask("a"), "β€’".repeat(8)); + assert_eq!(mask("ab"), "β€’".repeat(8)); + assert_eq!(mask("hunter2"), "β€’".repeat(8)); + assert!(!mask("hunter2").contains('h')); } #[test] @@ -773,14 +803,13 @@ mod tests { // emoji or accented character), since byte index 1 can land mid-char. let pw = "ζ—₯本θͺžγƒ‘γ‚Ήγƒ―ード"; let masked = mask(pw); - assert_eq!(masked.chars().count(), pw.chars().count()); + assert_eq!(masked, "β€’".repeat(8)); assert!(masked.chars().all(|c| c == 'β€’')); } #[test] fn mask_emoji_first_character_does_not_panic() { let pw = "πŸ”’password123"; - let masked = mask(pw); - assert_eq!(masked.chars().count(), pw.chars().count()); + assert_eq!(mask(pw), "β€’".repeat(8)); } } diff --git a/src/bread_events.rs b/src/bread_events.rs index e99f603..bdac0d8 100644 --- a/src/bread_events.rs +++ b/src/bread_events.rs @@ -37,37 +37,54 @@ pub fn emit_health_changed(client: &BreadClient, profile: &str, health: &str, ss ); } +/// What a `bread.command.crumbs.*` event asks the watch loop to do. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CommandAction { + /// Persist this profile via [`state::set_profile`]. Applied on the + /// watch loop thread β€” the single owner of config/state file access β€” + /// never on the subscription thread, which would race the loop's own + /// `Config::load`/`save`. + SetProfile(String), + /// Nothing to do: unknown verb, or a validation failure already + /// reported via `bread.crumbs.set_profile.failed`. + Ignore, +} + /// Reacts to `bread.command.crumbs.*` verbs. Only `set_profile` maps to /// real, existing breadcrumbs functionality today β€” there is no pin/select /// (or other) verb because breadcrumbs has no such concept. Unrecognized /// verbs are ignored, not stubbed as no-ops that pretend to succeed. /// -/// Returns `true` when a profile was actually persisted, so the watch loop -/// can wake immediately and re-evaluate instead of waiting out the current -/// poll interval. -/// -/// Emits `bread.crumbs.set_profile.done`/`.failed` per the confirmation -/// convention in bread's Documentation.md. -pub fn handle_command(event: &BreadEvent) -> bool { +/// This only *parses and validates* the command β€” it performs no file I/O +/// (that would race the watch loop's own config access from a second +/// thread). The returned [`CommandAction`] is forwarded to the loop, which +/// applies it via [`apply_set_profile`]. +pub fn handle_command(event: &BreadEvent) -> CommandAction { let Some(verb) = event.event.strip_prefix("bread.command.crumbs.") else { - return false; + return CommandAction::Ignore; }; match verb { - "set_profile" => handle_set_profile(event), + "set_profile" => match event.data.get("profile").and_then(|v| v.as_str()) { + Some(name) if !name.trim().is_empty() => CommandAction::SetProfile(name.to_string()), + _ => { + emit_set_profile_failed("missing string \"profile\" in command data"); + CommandAction::Ignore + } + }, other => { crate::notify::log(&format!( "watch: ignoring unrecognized bread.command.crumbs.{other}" )); - false + CommandAction::Ignore } } } -fn handle_set_profile(event: &BreadEvent) -> bool { - let Some(name) = event.data.get("profile").and_then(|v| v.as_str()) else { - emit_set_profile_failed("missing string \"profile\" in command data"); - return false; - }; +/// Apply a `set_profile` command on the watch loop thread and emit the +/// `done`/`failed` confirmation. Kept separate from [`handle_command`] so +/// the bread subscription thread never touches config/state files +/// concurrently with the loop. +pub fn apply_set_profile(name: &str) { match Config::load().and_then(|cfg| state::set_profile(&cfg, name)) { Ok(()) => { crate::notify::log(&format!( @@ -77,11 +94,9 @@ fn handle_set_profile(event: &BreadEvent) -> bool { "bread.crumbs.set_profile.done", serde_json::json!({ "profile": name }), ); - true } Err(e) => { emit_set_profile_failed(&e); - false } } } diff --git a/src/config.rs b/src/config.rs index 0ee601e..6f77af5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -28,6 +28,10 @@ fn default_ping_host() -> String { "1.1.1.1".to_string() } +fn is_false(b: &bool) -> bool { + !b +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Settings { #[serde(default = "default_dns")] @@ -44,6 +48,14 @@ pub struct Settings { pub connectivity_url: String, #[serde(default = "default_ping_host")] pub ping_host: String, + /// Set the first time the config is saved. Core profiles (`home` / + /// `work` / `away`) are only backfilled for genuinely fresh or legacy + /// configs; once the user owns the file, a profile they deliberately + /// deleted stays deleted instead of being silently resurrected on the + /// next load. Omits itself from the TOML until set, so existing + /// configs keep parsing exactly as before. + #[serde(default, skip_serializing_if = "is_false")] + pub core_profiles_initialized: bool, } impl Default for Settings { @@ -56,6 +68,7 @@ impl Default for Settings { watch_interval: default_watch_interval(), connectivity_url: default_connectivity_url(), ping_host: default_ping_host(), + core_profiles_initialized: false, } } } @@ -175,7 +188,7 @@ impl Config { pub fn load() -> Result { let path = config_path(); if !path.exists() { - let cfg = build_initial_config(); + let mut cfg = build_initial_config(); cfg.save()?; return Ok(cfg); } @@ -192,14 +205,31 @@ impl Config { .map_err(|e| format!("reading {}: {e}", net_path.display()))?; let nf: NetworksFile = toml::from_str(&net_text) .map_err(|e| format!("parsing {}: {e}", net_path.display()))?; - cfg.networks = nf.networks; + // Merge, don't overwrite: a legacy config can still carry an + // inline `[[networks]]` block, and those entries must survive + // even when networks.toml already exists β€” otherwise the next + // save() (which writes only networks.toml) would silently drop + // hand-added inline networks. networks.toml wins on SSID + // conflicts; inline-only entries are appended and migrated. + let mut merged = nf.networks; + for def in std::mem::take(&mut cfg.networks) { + if !merged.iter().any(|n| n.ssid == def.ssid) { + merged.push(def); + } + } + cfg.networks = merged; } // else: no networks.toml yet β€” keep whatever legacy inline networks // were read from breadcrumbs.toml above (or none, on a genuinely // fresh config). The next `save()` writes them to networks.toml and // stops writing them into breadcrumbs.toml, completing the migration. - // Self-heal: guarantee the three core profiles always exist. + // Enforce the documented minimum so `list` and the watch loop agree + // on the poll interval (watch silently clamps to 4 otherwise). + if cfg.settings.watch_interval < 4 { + cfg.settings.watch_interval = 4; + } + ensure_core_profiles(&mut cfg); Ok(cfg) } @@ -209,7 +239,12 @@ impl Config { /// `forget`, `scan`, `profile set`, and `flow::run`'s own credential /// clearing) goes through this single method so the two files never /// drift out of sync with each other. - pub fn save(&self) -> Result<(), String> { + pub fn save(&mut self) -> Result<(), String> { + // The first save marks the config as user-owned: core profiles are + // backfilled only for genuinely fresh/legacy configs, never + // resurrected after the user has edited (or deleted) them. + self.settings.core_profiles_initialized = true; + let dir = config_dir(); fs::create_dir_all(&dir).map_err(|e| format!("creating {}: {e}", dir.display()))?; @@ -296,6 +331,13 @@ fn core_profiles() -> BTreeMap { } fn ensure_core_profiles(cfg: &mut Config) { + // Backfill missing core profiles only until the user has taken + // ownership of the config (`core_profiles_initialized` is set by the + // first save). After that, a profile the user deliberately deleted + // stays deleted. + if cfg.settings.core_profiles_initialized { + return; + } for (name, prof) in core_profiles() { cfg.profiles.entry(name).or_insert(prof); } @@ -350,6 +392,23 @@ mod tests { assert!(cfg.profile("away").is_some()); } + #[test] + fn ensure_core_profiles_skips_backfill_once_initialized() { + // After the first save the config is user-owned: a deliberately + // deleted core profile must stay deleted instead of being + // resurrected on every load. + let mut cfg = Config { + settings: Settings { + core_profiles_initialized: true, + ..Default::default() + }, + networks: vec![], + profiles: BTreeMap::new(), + }; + ensure_core_profiles(&mut cfg); + assert!(cfg.profiles.is_empty(), "no backfill once user-owned"); + } + #[test] fn ensure_core_profiles_preserves_user_customized_core_profile() { // A user-edited "home" (custom SSIDs) must not be clobbered by the diff --git a/src/flow.rs b/src/flow.rs index 04e6f9e..05566b8 100644 --- a/src/flow.rs +++ b/src/flow.rs @@ -1,3 +1,6 @@ +use std::thread; +use std::time::Duration; + use crate::config::{Config, NetworkDef}; use crate::nm; use crate::notify::{log, notify, Urgency}; @@ -74,14 +77,45 @@ fn clear_password_if_used(cfg: &mut Config, ssid: &str) { } } -/// Try to connect + confirm it actually carries traffic. -/// Returns Ok(()) on success, Err(reason) on failure. +/// Try to connect + confirm the device actually landed on the *requested* +/// SSID. Returns Ok(()) on success, Err(reason) on failure. fn connect_and_verify(iface: &str, def: &NetworkDef, cfg: &Config) -> Result<(), String> { nm::connect_verbose(iface, def, cfg.settings.nmcli_wait, &cfg.settings.dns)?; - if !nm::device_connected(iface) { - return Err("device not connected after nmcli success".into()); + // Confirm the SSID, not just "device connected": NM autoconnect can win + // a race and leave the device on a different network, and the wifi list + // can lag activation by a moment β€” so poll briefly before giving up. + for _ in 0..8 { + match nm::active_ssid(iface) { + // Explicitly on the requested network β€” success. + Some(active) if active == def.ssid => return Ok(()), + // Associated with a *different* network β€” the failure this + // check exists to catch. + Some(_) => break, + // Scan list stale right after activation β€” keep polling while + // the device is at least connected. + None => { + if !nm::device_connected(iface) { + break; + } + } + } + thread::sleep(Duration::from_millis(250)); } - Ok(()) + Err(format!("not associated with '{}' after connect", def.ssid)) +} + +/// Run the connection state machine for `profile_name`, with desktop +/// notifications enabled. See [`run_quiet`] for the daemon-facing variant. +pub fn run(cfg: &mut Config, profile_name: &str) -> Outcome { + run_inner(cfg, profile_name, true) +} + +/// Same state machine, but suppresses desktop notifications. Used by the +/// watch loop, which does its own transition-gated notifications β€” without +/// this, a persistent failure (e.g. a stopped Tailscale daemon) would +/// re-notify on every recovery retry instead of once per state change. +pub fn run_quiet(cfg: &mut Config, profile_name: &str) -> Outcome { + run_inner(cfg, profile_name, false) } /// Run the connection state machine for `profile_name`. @@ -91,15 +125,17 @@ fn connect_and_verify(iface: &str, def: &NetworkDef, cfg: &Config) -> Result<(), /// immediately (see [`clear_password_if_used`]) β€” this is the only way that /// clearing happens for the `init` / `profile set --apply` / `detect --apply` /// commands and the watch loop, all of which route through here. -pub fn run(cfg: &mut Config, profile_name: &str) -> Outcome { +fn run_inner(cfg: &mut Config, profile_name: &str, notify_user: bool) -> Outcome { let profile = match cfg.profile(profile_name) { Some(p) => p.clone(), None => { - notify( - "breadcrumbs: unknown profile", - &format!("'{profile_name}' is not defined in breadcrumbs.toml"), - Urgency::Critical, - ); + if notify_user { + notify( + "breadcrumbs: unknown profile", + &format!("'{profile_name}' is not defined in breadcrumbs.toml"), + Urgency::Critical, + ); + } return Outcome::UnknownProfile(profile_name.to_string()); } }; @@ -107,11 +143,13 @@ pub fn run(cfg: &mut Config, profile_name: &str) -> Outcome { let iface = match nm::wifi_interface() { Some(i) => i, None => { - notify( - "breadcrumbs: no Wi-Fi adapter", - "Hardware issue β€” Wi-Fi device not found. Manual check needed.", - Urgency::Critical, - ); + if notify_user { + notify( + "breadcrumbs: no Wi-Fi adapter", + "Hardware issue β€” Wi-Fi device not found. Manual check needed.", + Urgency::Critical, + ); + } return Outcome::NoInterface; } }; @@ -173,15 +211,17 @@ pub fn run(cfg: &mut Config, profile_name: &str) -> Outcome { let ts = tailscale::ensure_exit_node(&exit_node); if !ts.is_ok() { let ssid = nm::active_ssid(&iface).or_else(|| profile.bootstrap.clone()); - notify( - "Tailscale Error", - &format!( - "{} β€” staying on {}", - ts.describe(), - ssid.clone().unwrap_or_else(|| "Wi-Fi".into()) - ), - Urgency::Critical, - ); + if notify_user { + notify( + "Tailscale Error", + &format!( + "{} β€” staying on {}", + ts.describe(), + ssid.clone().unwrap_or_else(|| "Wi-Fi".into()) + ), + Urgency::Critical, + ); + } return Outcome::TailscaleError { ssid, health: ts }; } log(&format!("tailscale healthy via exit node {exit_node}")); @@ -205,7 +245,7 @@ pub fn run(cfg: &mut Config, profile_name: &str) -> Outcome { } else { Some("associated but no internet yet".to_string()) }; - finish_connected(&def.ssid, profile_name, ¬e); + finish_connected(&def.ssid, profile_name, ¬e, notify_user); return Outcome::Connected { ssid: def.ssid.clone(), note, @@ -227,7 +267,7 @@ pub fn run(cfg: &mut Config, profile_name: &str) -> Outcome { } else { Some("associated but no internet yet".to_string()) }; - finish_connected(&def.ssid, profile_name, ¬e); + finish_connected(&def.ssid, profile_name, ¬e, notify_user); return Outcome::Connected { ssid: def.ssid.clone(), note, @@ -272,7 +312,9 @@ pub fn run(cfg: &mut Config, profile_name: &str) -> Outcome { } else { format!("target network not in range β€” staying on {bs_ssid} (Tailscale OK)") }; - notify("breadcrumbs: using bootstrap", &reason, Urgency::Normal); + if notify_user { + notify("breadcrumbs: using bootstrap", &reason, Urgency::Normal); + } log(&format!("flow end: on bootstrap {bs_ssid}; {reason}")); return Outcome::Connected { ssid: bs_ssid, @@ -286,33 +328,40 @@ pub fn run(cfg: &mut Config, profile_name: &str) -> Outcome { .map(|c| c.ssid.as_str()) .collect::>() .join(", "); - notify( - "breadcrumbs: no known networks", - &format!("profile '{profile_name}': none of [{names}] are in range"), - Urgency::Critical, - ); + let msg = if candidates.is_empty() { + format!("profile '{profile_name}' has no networks configured") + } else { + format!("profile '{profile_name}': none of [{names}] are in range") + }; + if notify_user { + notify("breadcrumbs: no known networks", &msg, Urgency::Critical); + } log(&format!( "flow end: no networks connected (profile={profile_name})" )); Outcome::NoNetworks } -fn finish_connected(ssid: &str, profile: &str, note: &Option) { +fn finish_connected(ssid: &str, profile: &str, note: &Option, notify_user: bool) { match note { None => { - notify( - "breadcrumbs: connected", - &format!("{ssid} ({profile})"), - Urgency::Low, - ); + if notify_user { + notify( + "breadcrumbs: connected", + &format!("{ssid} ({profile})"), + Urgency::Low, + ); + } log(&format!("flow end: connected {ssid} (profile={profile})")); } Some(n) => { - notify( - "breadcrumbs: connected (degraded)", - &format!("{ssid} ({profile}) β€” {n}"), - Urgency::Normal, - ); + if notify_user { + notify( + "breadcrumbs: connected (degraded)", + &format!("{ssid} ({profile}) β€” {n}"), + Urgency::Normal, + ); + } log(&format!( "flow end: connected {ssid} (profile={profile}) note={n}" )); diff --git a/src/nm.rs b/src/nm.rs index 4dabb0e..e395806 100644 --- a/src/nm.rs +++ b/src/nm.rs @@ -114,6 +114,15 @@ pub struct ScanEntry { pub security: String, } +/// Parse an nmcli SIGNAL value ("72" or "72 %") into a comparable number. +fn signal_strength(s: &str) -> i32 { + s.trim() + .trim_end_matches('%') + .trim() + .parse::() + .unwrap_or(-100) +} + pub fn scan_list(iface: &str) -> Vec { let o = run( "nmcli", @@ -129,8 +138,7 @@ pub fn scan_list(iface: &str) -> Vec { ], Duration::from_secs(12), ); - let mut seen = HashSet::new(); - let mut out = Vec::new(); + let mut out: Vec = Vec::new(); if !o.success { return out; } @@ -140,14 +148,29 @@ pub fn scan_list(iface: &str) -> Vec { continue; } let ssid = fields[0].trim().to_string(); - if ssid.is_empty() || !seen.insert(ssid.clone()) { + if ssid.is_empty() { + // Hidden networks have no SSID in the scan; they're not + // selectable here anyway (see `cmd_scan`), so skip them. continue; } - out.push(ScanEntry { - ssid, - signal: fields.get(1).cloned().unwrap_or_default(), - security: fields.get(2).cloned().unwrap_or_default(), - }); + let signal = fields.get(1).cloned().unwrap_or_default(); + let security = fields.get(2).cloned().unwrap_or_default(); + // One line per BSSID: dedup by SSID keeping the *strongest* signal, + // so a network broadcast by several APs shows once (at its best + // signal) instead of N times at the first listing. + match out.iter_mut().find(|e| e.ssid == ssid) { + Some(existing) => { + if signal_strength(&signal) > signal_strength(&existing.signal) { + existing.signal = signal; + existing.security = security; + } + } + None => out.push(ScanEntry { + ssid, + signal, + security, + }), + } } out } diff --git a/src/status.rs b/src/status.rs index b06dea9..0412125 100644 --- a/src/status.rs +++ b/src/status.rs @@ -21,8 +21,12 @@ pub fn internet_ok(cfg: &Config) -> bool { ], Duration::from_secs(6), ); - let code = o.stdout.trim(); - if code == "204" || code == "200" || code == "301" || code == "302" { + // Only a 204 counts as real internet. Captive/guest portals answer + // 200 (a login page) or 302 (a redirect to it) β€” accepting those + // would classify a portal-trapped device as "Up". The default + // endpoint is generate_204, which returns 204 precisely when + // traffic isn't being intercepted. + if o.stdout.trim() == "204" { return true; } } @@ -66,7 +70,10 @@ pub fn gather(cfg: &Config, profile_name: &str) -> Status { let iface = nm::wifi_interface(); let ssid = iface.as_deref().and_then(nm::active_ssid); let ip = iface.as_deref().and_then(ipv4); - let internet = internet_ok(cfg); + // Skip the (potentially 4s-blocking) connectivity probe when there's no + // Wi-Fi interface at all: the watch loop classifies NoAdapter and would + // otherwise burn a network round-trip (curl/ping) every tick for nothing. + let internet = iface.is_some() && internet_ok(cfg); let prof = cfg.profile(profile_name); let ts_required = prof.map(|p| p.tailscale).unwrap_or(false); diff --git a/src/tailscale.rs b/src/tailscale.rs index 50b7f11..a512728 100644 --- a/src/tailscale.rs +++ b/src/tailscale.rs @@ -21,6 +21,10 @@ pub enum TsHealth { ExitNodeMissing, /// The exit node exists but is offline. ExitNodeOffline, + /// The profile requires an exit node but none is configured + /// (`settings.exit_node` / per-profile `exit_node` empty). Cannot be + /// auto-fixed β€” the user must configure one. + NoExitNode, Error(String), } @@ -37,6 +41,7 @@ impl TsHealth { TsHealth::Stopped => "backend stopped".into(), TsHealth::ExitNodeMissing => "exit node not found in tailnet".into(), TsHealth::ExitNodeOffline => "exit node is offline".into(), + TsHealth::NoExitNode => "no exit node configured".into(), TsHealth::Error(e) => format!("error: {e}"), } } @@ -225,10 +230,30 @@ pub fn ensure_exit_node(node: &str) -> TsHealth { if !installed() { return TsHealth::NotInstalled; } + if node.trim().is_empty() { + // Never run `tailscale set --exit-node=` with an empty node β€” that + // would clear the user's current exit-node selection. This is a + // config error, surfaced as its own health state. + return TsHealth::NoExitNode; + } let v = match status_json() { Some(v) => v, - None => return TsHealth::Error("could not read tailscale status".into()), + None => { + // Daemon unreachable β€” usually *not running*: a stopped daemon + // prints its error to stderr and leaves stdout empty, which is + // exactly why this branch used to be dead code (the + // BackendState "Stopped" case below only fires when the daemon + // is up but the backend is stopped). Try to bring it up before + // giving up; `tailscale up` is idempotent when already running + // and fails fast (no sudo prompt β€” stdin is /dev/null) when + // the caller lacks permission to manage the daemon. + let _ = run("tailscale", &["up"], Duration::from_secs(20)); + match status_json() { + Some(v2) => v2, + None => return TsHealth::Error("could not read tailscale status".into()), + } + } }; match backend_state(&v).as_str() { @@ -286,6 +311,11 @@ pub fn check(node: &str) -> TsHealth { if !installed() { return TsHealth::NotInstalled; } + if node.trim().is_empty() { + // Read-only check, so never runs `tailscale set` β€” an empty node is + // a config error, not something this probe can fix. + return TsHealth::NoExitNode; + } let v = match status_json() { Some(v) => v, None => return TsHealth::Error("status unavailable".into()), @@ -433,6 +463,7 @@ mod tests { assert!(!TsHealth::Stopped.is_ok()); assert!(!TsHealth::ExitNodeMissing.is_ok()); assert!(!TsHealth::ExitNodeOffline.is_ok()); + assert!(!TsHealth::NoExitNode.is_ok()); assert!(!TsHealth::Error("x".into()).is_ok()); } @@ -443,6 +474,7 @@ mod tests { TsHealth::NeedsLogin.describe(), "not logged in (run: tailscale up)" ); + assert_eq!(TsHealth::NoExitNode.describe(), "no exit node configured"); assert_eq!(TsHealth::Error("boom".into()).describe(), "error: boom"); } diff --git a/src/util.rs b/src/util.rs index f697f8f..de8eed3 100644 --- a/src/util.rs +++ b/src/util.rs @@ -139,13 +139,6 @@ fn spawn_run(prog: &str, args: &[&str], stdin: Option<&str>, timeout: Duration) Err(_) => return Output::failed(), }; - if let Some(data) = stdin { - if let Some(mut sink) = child.stdin.take() { - let _ = sink.write_all(data.as_bytes()); - // Drop closes the pipe so the child's read sees EOF. - } - } - let mut stdout_pipe = child.stdout.take(); let mut stderr_pipe = child.stderr.take(); @@ -164,6 +157,16 @@ fn spawn_run(prog: &str, args: &[&str], stdin: Option<&str>, timeout: Duration) buf }); + // Feed stdin only now that the reader threads are draining stdout and + // stderr: a chatty child could otherwise fill its stdout pipe while we + // block writing stdin, deadlocking both sides. + if let Some(data) = stdin { + if let Some(mut sink) = child.stdin.take() { + let _ = sink.write_all(data.as_bytes()); + // Drop closes the pipe so the child's read sees EOF. + } + } + let start = Instant::now(); let status = loop { match child.try_wait() { diff --git a/src/watch.rs b/src/watch.rs index 8813245..2ff8881 100644 --- a/src/watch.rs +++ b/src/watch.rs @@ -64,9 +64,11 @@ pub fn classify(cfg: &Config, profile: &str) -> (Health, Option) { if s.tailscale_required { match s.tailscale { Some(TsHealth::Ok) => (Health::Up, ssid), - Some(TsHealth::NeedsLogin) | Some(TsHealth::NotInstalled) => { - (Health::DownTailscaleManual, ssid) - } + // NeedsLogin / NotInstalled / NoExitNode all need human action: + // a missing exit-node config can't be auto-fixed either. + Some(TsHealth::NeedsLogin) + | Some(TsHealth::NotInstalled) + | Some(TsHealth::NoExitNode) => (Health::DownTailscaleManual, ssid), Some(_) => (Health::DownTailscaleOther, ssid), None => (Health::DownTailscaleManual, ssid), } @@ -84,9 +86,18 @@ fn debounce_ready(last: Option, gap: Duration) -> bool { last.map(|t| t.elapsed() > gap).unwrap_or(true) } +/// A wake signal for the watch loop. `SetProfile` is an *action* (applied on +/// the loop thread), `LinkChurn` is just "go look" β€” the distinction keeps +/// every config/state file access on the single loop thread, so the bread +/// subscription thread can never race the loop's own `Config::load`/`save`. +enum Wake { + LinkChurn, + SetProfile(String), +} + /// Tail `nmcli monitor` and ping the channel on link-state churn so we react /// to drops within a second instead of waiting out the poll interval. -fn spawn_nm_monitor(tx: mpsc::Sender<()>) { +fn spawn_nm_monitor(tx: mpsc::Sender) { thread::spawn(move || loop { let child = Command::new("nmcli") .arg("monitor") @@ -113,11 +124,18 @@ fn spawn_nm_monitor(tx: mpsc::Sender<()>) { let mut last: Option = None; for line in reader.lines().map_while(Result::ok) { let l = line.to_lowercase(); - let interesting = - l.contains("disconnect") || l.contains("unavailable") || l.contains("failed"); + // `connectivity` lines catch drops that keep the device + // "connected" but lose the internet (captive portal, DHCP + // failure); `deactivating` covers teardown. Everything else + // waits out the poll interval. + let interesting = l.contains("disconnect") + || l.contains("unavailable") + || l.contains("failed") + || l.contains("deactivating") + || l.contains("connectivity"); if interesting && debounce_ready(last, Duration::from_millis(1500)) { last = Some(Instant::now()); - let _ = tx.send(()); + let _ = tx.send(Wake::LinkChurn); } } } @@ -127,17 +145,32 @@ fn spawn_nm_monitor(tx: mpsc::Sender<()>) { }); } -/// Sleep up to `dur`, but wake early if `nmcli monitor` signals link churn. -fn wait_for_tick(rx: &Receiver<()>, dur: Duration) { +/// Sleep up to `dur`, but wake early if `nmcli monitor` signals link churn or +/// a `set_profile` command arrives. Returns the pending action, if any. +fn wait_for_tick(rx: &Receiver, dur: Duration) -> Option { match rx.recv_timeout(dur) { - Ok(()) => { - // Drain any burst of events so we don't re-fire immediately. - while rx.try_recv().is_ok() {} + Ok(first) => { + // Drain any burst of churn signals so we don't re-fire + // immediately, but never drop a queued set_profile β€” it's an + // action, not a signal, and the earliest one wins. + let mut pending = match &first { + Wake::SetProfile(_) => Some(first), + Wake::LinkChurn => None, + }; + while let Ok(w) = rx.try_recv() { + if pending.is_none() && matches!(&w, Wake::SetProfile(_)) { + pending = Some(w); + } + } + pending } - Err(mpsc::RecvTimeoutError::Timeout) => {} + Err(mpsc::RecvTimeoutError::Timeout) => None, // Monitor thread gone (shouldn't happen: we hold the sender) β€” fall // back to a plain sleep so we don't busy-spin. - Err(mpsc::RecvTimeoutError::Disconnected) => thread::sleep(dur), + Err(mpsc::RecvTimeoutError::Disconnected) => { + thread::sleep(dur); + None + } } } @@ -150,21 +183,25 @@ pub fn run(mut cfg: Config, run_initial: bool) -> i32 { ); log("watch: started"); - let (tx, rx) = mpsc::channel::<()>(); + let (tx, rx) = mpsc::channel::(); spawn_nm_monitor(tx.clone()); // Long-lived, so this uses BreadClient::subscribe (a persistent // background thread with its own reconnect/backoff loop). breadd being // absent or restarting is transparent: the subscription just quietly - // stops delivering commands until it reconnects. A successful - // `set_profile` wakes this loop the same way `nmcli monitor` does, so - // the new profile is applied on the next tick instead of waiting out - // the current poll interval. + // stops delivering commands until it reconnects. The callback only + // *validates* the command and forwards an action through the channel β€” + // it never touches config/state files itself (that would race this + // loop's own Config::load/save), so all file access stays on this one + // thread. let bread = BreadClient::connect(bread_events::APP_ID); let wake = tx; let _commands = bread.subscribe("bread.command.crumbs.**", move |event| { - if bread_events::handle_command(&event) { - let _ = wake.send(()); + match bread_events::handle_command(&event) { + bread_events::CommandAction::SetProfile(name) => { + let _ = wake.send(Wake::SetProfile(name)); + } + bread_events::CommandAction::Ignore => {} } }); @@ -178,7 +215,7 @@ pub fn run(mut cfg: Config, run_initial: bool) -> i32 { )); } else { log(&format!("watch: initial flow for profile={profile}")); - let _ = flow::run(&mut cfg, &profile); + let _ = flow::run_quiet(&mut cfg, &profile); } } @@ -193,9 +230,10 @@ pub fn run(mut cfg: Config, run_initial: bool) -> i32 { // This always runs *before* `flow::run` below (never after, within // the same tick), so a password `flow::run` clears-and-saves this // iteration is durably on disk by the time the *next* iteration's - // reload runs β€” there's no window where a stale (still-has-password) - // reload could clobber the save, since the two never race on - // different threads: everything here is sequential on this loop. + // reload runs. All config/state file access happens on this loop + // thread: `set_profile` commands from the bread bus are queued as + // [`Wake::SetProfile`] and applied here (see the bottom of the + // loop), never on the subscription thread. if let Ok(fresh) = Config::load() { cfg = fresh; } @@ -248,29 +286,43 @@ pub fn run(mut cfg: Config, run_initial: bool) -> i32 { fail_streak = fail_streak.saturating_add(1); } Health::UnknownProfile => { - // flow::run() already notifies + logs the "unknown profile" - // critical error; re-running it here would just spam that on - // every tick, so only surface it once per transition. + // flow::run() is quiet from here, so surface the misconfig + // ourselves β€” once per transition/change, not every tick. if transition || profile_changed { - let _ = flow::run(&mut cfg, &profile); + notify( + "breadcrumbs: unknown profile", + &format!("'{profile}' is not defined in breadcrumbs.toml"), + Urgency::Critical, + ); } fail_streak = fail_streak.saturating_add(1); } Health::DownTailscaleManual => { - // Can't be auto-fixed (login / not installed). Notify once. + // Can't be auto-fixed (login / install / exit-node config). + // Notify once per transition. if transition { notify( "Tailscale Error", - "Tailscale needs manual attention (login / install). \ - Other Wi-Fi automation paused until resolved.", + "Tailscale needs manual attention (login / install / \ + exit node config). Other Wi-Fi automation paused \ + until resolved.", Urgency::Critical, ); } - // Re-run flow only on transition so we land on the bootstrap net. - if transition || profile_changed { - let _ = flow::run(&mut cfg, &profile); + // Re-attempt periodically and on the transition into this + // state: login may have completed since the last attempt, or + // the user may have missed the browser window. Quiet β€” a + // still-broken state must not re-notify on every retry. + let elapsed = last_flow_at.map(|t| t.elapsed().as_secs()).unwrap_or(u64::MAX); + if elapsed >= FLOW_COOLDOWN { + let outcome = flow::run_quiet(&mut cfg, &profile); + last_flow_at = Some(Instant::now()); + fail_streak = if outcome.ok() { + 0 + } else { + fail_streak.saturating_add(1) + }; } - fail_streak = fail_streak.saturating_add(1); } Health::DownNoNet | Health::DownTailscaleOther => { if transition { @@ -288,7 +340,7 @@ pub fn run(mut cfg: Config, run_initial: bool) -> i32 { "watch: down ({:?}) profile={profile} ssid={:?} β€” running flow", health, ssid )); - let outcome = flow::run(&mut cfg, &profile); + let outcome = flow::run_quiet(&mut cfg, &profile); log(&format!("watch: recovery outcome = {:?}", outcome)); last_flow_at = Some(Instant::now()); fail_streak = if outcome.ok() { @@ -310,7 +362,12 @@ pub fn run(mut cfg: Config, run_initial: bool) -> i32 { // Adaptive backoff: healthy -> base; failing -> grow up to ~6x. let mult = 1 + fail_streak.min(5); let dur = Duration::from_secs(base * mult as u64); - wait_for_tick(&rx, dur); + // Apply a queued set_profile on this thread β€” the single owner of + // config/state file access β€” and emit the confirmation. The next + // iteration's reload sees the new profile and recovers accordingly. + if let Some(Wake::SetProfile(name)) = wait_for_tick(&rx, dur) { + bread_events::apply_set_profile(&name); + } } } diff --git a/tests/cli.rs b/tests/cli.rs index 7d48b1b..87c7e2d 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -317,7 +317,10 @@ fn install_service_no_enable_writes_valid_unit_file() { let o = sb.cmd(&["install-service", "--no-enable"]); assert!(o.status.success(), "stderr: {}", stderr(&o)); - let unit_path = sb.root.join(".config/systemd/user/breadcrumbs.service"); + // The sandbox sets XDG_CONFIG_HOME=$root/config, so the unit lands + // under $XDG_CONFIG_HOME/systemd/user β€” the whole point of the fix is + // honoring XDG rather than hardcoding ~/.config. + let unit_path = sb.root.join("config/systemd/user/breadcrumbs.service"); assert!(unit_path.exists()); let text = fs::read_to_string(unit_path).unwrap(); assert!(text.contains("ExecStart=")); @@ -423,6 +426,8 @@ case "$args" in "device wifi rescan"*) ;; "-t -f SSID device wifi list ifname wlan0") echo "TestNet" ;; + "-t -f ACTIVE,SSID device wifi list ifname wlan0") + echo "yes:TestNet" ;; "-t -f NAME,TYPE connection show") if [ -f "$marker" ]; then echo "TestNet:802-11-wireless" diff --git a/tests/common/mod.rs b/tests/common/mod.rs index dca7888..67f5f2c 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -50,6 +50,7 @@ impl RecordedCall { } type Matcher = Box bool>; +type DynamicRule = (Matcher, Box Output>); /// A canned, rule-based [`Runner`]. Rules are tried in registration order; /// the first whose matcher returns `true` supplies the response. No rule @@ -58,6 +59,7 @@ type Matcher = Box bool>; /// (a wrong exit code) rather than silently returning success. pub struct FakeRunner { rules: Vec<(Matcher, Output)>, + dynamic_rules: Vec, commands: HashSet, calls: Rc>>, } @@ -66,6 +68,7 @@ impl FakeRunner { pub fn new() -> Self { FakeRunner { rules: Vec::new(), + dynamic_rules: Vec::new(), commands: HashSet::new(), calls: Rc::new(RefCell::new(Vec::new())), } @@ -99,6 +102,20 @@ impl FakeRunner { output, ) } + + /// Register a rule whose *response* is computed at call time (rather + /// than canned), enabling stateful fakes β€” e.g. answering "which SSID is + /// active?" with the SSID of the most recently dialed connection. + /// Dynamic rules are tried after the static ones. + pub fn on_dynamic( + mut self, + matcher: impl Fn(&str, &[&str]) -> bool + 'static, + out: impl Fn(&str, &[&str]) -> Output + 'static, + ) -> Self { + self.dynamic_rules + .push((Box::new(matcher), Box::new(out))); + self + } } impl Default for FakeRunner { @@ -119,6 +136,11 @@ impl Runner for FakeRunner { return out.clone(); } } + for (matcher, out) in &self.dynamic_rules { + if matcher(prog, args) { + return out(prog, args); + } + } Output::failed() } diff --git a/tests/flow_watch.rs b/tests/flow_watch.rs index d03877d..3fae31f 100644 --- a/tests/flow_watch.rs +++ b/tests/flow_watch.rs @@ -52,16 +52,53 @@ fn base_config() -> Config { /// the device reports connected after any successful connect attempt. fn base_nm(visible_ssids: &[&str]) -> FakeRunner { let visible = visible_ssids.join("\n"); - FakeRunner::new() + let runner = FakeRunner::new() .on_contains("nmcli", "DEVICE,TYPE", ok("wlan0:wifi")) .on_contains("nmcli", "radio wifi on", ok("")) .on_contains("nmcli", "wifi rescan", ok("")) - .on_contains("nmcli", "-f SSID device wifi list", ok(&visible)) + // Exact match: `-f ACTIVE,SSID` queries (which contain the substring + // "SSID device wifi list") must NOT be answered with the visible + // list β€” they go to the stateful rule below. + .on( + move |_prog, args| args.join(" ") == "-t -f SSID device wifi list ifname wlan0", + ok(&visible), + ) .on_contains("nmcli", "NAME,TYPE", ok("")) // no saved profiles .on_contains("nmcli", "GENERAL.CON-UUID", ok("uuid-1")) .on_contains("nmcli", "ipv4.ignore-auto-dns", ok("")) .on_contains("nmcli", "device reapply", ok("")) - .on_contains("nmcli", "DEVICE,STATE", ok("wlan0:connected")) + .on_contains("nmcli", "DEVICE,STATE", ok("wlan0:connected")); + let calls = runner.calls_handle(); + runner.on_dynamic( + move |_prog, args| args.join(" ") == "-t -f ACTIVE,SSID device wifi list ifname wlan0", + move |_prog, _args| { + // Stateful: answer with the SSID of the most recently dialed + // connection, so connect_and_verify's post-connect SSID check + // sees the network that was just activated (bootstrap first, + // then the target). + let rec = calls.borrow(); + let ssid = rec.iter().rev().find_map(|call| { + let j = call.args.join(" "); + if j.contains("connect") { + call.args + .iter() + .position(|a| a == "connect") + .map(|i| call.args[i + 1].clone()) + } else if j.contains("connection up") { + call.args + .iter() + .position(|a| a == "up") + .map(|i| call.args[i + 1].clone()) + } else { + None + } + }); + match ssid { + Some(s) => ok(&format!("yes:{s}")), + None => ok(""), + } + }, + ) } /// A successful `device wifi connect ...` for every ssid in `ssids`. @@ -494,12 +531,17 @@ fn set_profile_command_persists_even_with_no_daemon_reachable() { state::set_profile(&cfg, "away").unwrap(); assert_eq!(State::load("away").profile, "away"); - let acted = bread_events::handle_command(&command_event( + // handle_command only parses/validates (no file I/O on the subscription + // thread); the loop thread then applies the action. + let action = bread_events::handle_command(&command_event( "bread.command.crumbs.set_profile", serde_json::json!({ "profile": "home" }), )); - - assert!(acted, "known profile must persist"); + assert!( + matches!(action, bread_events::CommandAction::SetProfile(n) if n == "home"), + "a known profile must yield a SetProfile action" + ); + bread_events::apply_set_profile("home"); assert_eq!(State::load("away").profile, "home"); } @@ -509,12 +551,14 @@ fn set_profile_command_rejects_unknown_profile() { let cfg = Config::load().expect("fresh config"); state::set_profile(&cfg, "away").unwrap(); - let acted = bread_events::handle_command(&command_event( + let action = bread_events::handle_command(&command_event( "bread.command.crumbs.set_profile", serde_json::json!({ "profile": "bogus" }), )); - - assert!(!acted); + assert!(matches!(action, bread_events::CommandAction::SetProfile(n) if n == "bogus")); + // The rejection happens when the loop thread applies it: state is + // untouched and the failure event is emitted (a no-op without breadd). + bread_events::apply_set_profile("bogus"); assert_eq!( State::load("away").profile, "away", @@ -528,12 +572,11 @@ fn set_profile_command_rejects_missing_profile_field() { let cfg = Config::load().expect("fresh config"); state::set_profile(&cfg, "away").unwrap(); - let acted = bread_events::handle_command(&command_event( + let action = bread_events::handle_command(&command_event( "bread.command.crumbs.set_profile", serde_json::json!({}), )); - - assert!(!acted); + assert!(matches!(action, bread_events::CommandAction::Ignore)); assert_eq!(State::load("away").profile, "away"); } @@ -543,12 +586,11 @@ fn handle_command_ignores_unrecognized_verb() { let cfg = Config::load().expect("fresh config"); state::set_profile(&cfg, "away").unwrap(); - let acted = bread_events::handle_command(&command_event( + let action = bread_events::handle_command(&command_event( "bread.command.crumbs.pin", serde_json::json!({}), )); - - assert!(!acted); + assert!(matches!(action, bread_events::CommandAction::Ignore)); assert_eq!( State::load("away").profile, "away", @@ -562,14 +604,20 @@ fn handle_command_ignores_events_outside_its_own_command_namespace() { let cfg = Config::load().expect("fresh config"); state::set_profile(&cfg, "away").unwrap(); - assert!(!bread_events::handle_command(&command_event( - "bread.command.clip.clear", - serde_json::json!({}), - ))); - assert!(!bread_events::handle_command(&command_event( - "bread.crumbs.profile.changed", - serde_json::json!({ "from": "away", "to": "home" }), - ))); + assert!(matches!( + bread_events::handle_command(&command_event( + "bread.command.clip.clear", + serde_json::json!({}), + )), + bread_events::CommandAction::Ignore + )); + assert!(matches!( + bread_events::handle_command(&command_event( + "bread.crumbs.profile.changed", + serde_json::json!({ "from": "away", "to": "home" }), + )), + bread_events::CommandAction::Ignore + )); assert_eq!(State::load("away").profile, "away"); }